AIO calculation of remaining elements to send is wrong. about libkcapi HOT 27 CLOSED

Very good point. And not trivial to fix. We would need to calculate the tosend variable after the _kcapi_aio_send_iov call. But that call returns the number of bytes the kernel received. Now, that needs to be turned back into the number of IOVECs that were sent. And what happens if some IOVECs were only sent partially?

I need to think about that... Maybe we need to turn the tosend variable from a number-of-IOVECs into a number-of-bytes variable?

from libkcapi.

Agreed it is fiddly to deal with. I hadn't thought about the partial IOVEC option. Will look at the options today and see if I can see a clean way forwards. Obviously you'll probably get there first but I want to know more about how libkcapi internals work anyway so a useful exercise!

from libkcapi.

Hi Stephan, I'd forgotten how much I hate sockets ;)

Anyhow, the only 'small' change that will result in the code at least working is to turning it into a synchronous batch process by adding a while loop at the end of _kcapi_aio_read_iov_fd to check wehave received iovlen using handle->aio.completed_reads. Not nice and really doesn't test things properly as a result.

There are several places where the code is assuming we received what we asked for and it will take a much more invasive patch to rip them all out.

Jonathan

from libkcapi.

Still mulling things over.

But can you please send me a pointer to the code where you think the code assumes an equal amount of data went in that was submitted?

from libkcapi.

Let me spend a few more hours on it and I'll see if I can put together some sort of rough patch. I'm tempted to separate the case of partial iov (just spin on that) and do the case where the iov is missing entirely separately but we'll see how it falls out.

from libkcapi.

Hi Stephan,

The biggest complexity so far has been around the cio structures being a simple linear array and the fact we are reusing them. So we need to keep any left over ones untouched when we do the new io_submit. First time we fail to keep up it is simple as they are at the end of the array and we can leave them there. Second time however they will be in the middle of the array - this means we need to break our io_submit it two - before and after our outstanding cio entries. Right now I'm getting some ENOMEM errors coming out from the reads so trying to track down whether this is driver bug my end or not.

Also when running in aio mode I'm not sure what the purpose of the event interface is except in ensuring there is at least one IOV ready to be handled... For now I've dropped it entirely, for now, as it makes the code easier to keep track of.

Jonathan

from libkcapi.

from libkcapi.

from libkcapi.

from libkcapi.

from libkcapi.

Hi Stephan,

This has taken me a lot longer than I hoped due to some local bug chasing and the current result is hideous, but I think we need something along the lines of:
https://github.com/hisilicon/libkcapi/commit/2c63885834e26bf246d0795dbcdcaf48b55c14a9

Right now it doesn't correctly account for the last used element in the ring, but instead does a dodgy search to find the hole. That's obviously stupid but it works. Didn't want to sit on this longer and risk straying into the silly season.

Hopefully I'll get the issue I'm seeing in our driver fixed and get back to tidying this up.
That branch also includes a bodged up version of cryptoperf to allow easy hammering of the aio paths. It just runs 2048 blocks of data for everyone you ask for and effectively resynchronizes at the end of each 2048. This adds some overhead to the timing loop but hopefully not that much. Doing it right is obviously rather harder to code up!

Jonathan

from libkcapi.

from libkcapi.

from libkcapi.

from libkcapi.

Hi Stephen,

Sorry for the false message a few minutes ago. A dumb error meant my tests weren't quite what I thought they were. Getting an out of FD error then all sorts of nastiness.

Need to do some debugging - hopefully get back to you later.

Jonathan

from libkcapi.

Hi again Stephan.

A few issues identified.

1. You can end up not consuming events because you are calling _kcapi_aio_read_all instead of _kcapi_aio_poll_data in the 'catch up' section at the end of _kcapi_cipher_crypto_aio. This is fine for a single run before closing the socket, but if we are feeding more data you then get too many events. Fix is fairly simple case of using the poll form instead. However be careful to only do it if you actually have anything outstanding.
2. You have a buffer overrun as a result of setting iovlen_tmp to iovlen. This is then used to index into arrays that are on KCAPI_AIO_CONCURRENT long.
3. Transfer accounting is missing some of the data (this is tied up with the iovlen_tmp issue). If you fix that by setting it to KCAPI_AIO_CONCURRENT then you account for only the last pass of 64 iovs. I thought that would be easy to fix but there are some fiddly corners:
   A) Be careful of double accounting if we count after each call to aio_read_iov - this I fixed by adding an AIO_ACCOUNTED to the special return values.
   B) Misses the data received in the catch up within _kcapi_aio_read_iov_fd. (I haven't fixed this case)
   To my mind we need to make all the read calls return what the successfully read rather than trying to account for it at the end.
   https://github.com/hisilicon/libkcapi/tree/aiofixes

I see you've pushed some other patches out in the meantime. Guess you may have fixed this already!

Jonathan

from libkcapi.

Rebased my tree on the changes you pushed - whilst you had tidied up a bit in that area, the issues were still present.

from libkcapi.

from libkcapi.

from libkcapi.

from libkcapi.

from libkcapi.

from libkcapi.

With the current code, I would think that the issue is solved.

The AIO implementation could receive some more love (e.g. not fail the entire AIO operation if one IOCB fails), but this should be covered by a separate patch set.

from libkcapi.

from libkcapi.

from libkcapi.

from libkcapi.

from libkcapi.