Re: io_uring memory ordering (and broken queue-is-full checks)

["Stefan Bühler" <source@stbuehler.de>]

Hi,

On 17.04.19 17:02, Jens Axboe wrote:
> [...]
>
> Care to turn the pseudo code into a real patch? Would be easier to
> digest. But thanks for taking a look at this!

Fair enough; I'm working on it.

I'll start with the actual bugs, and will try to cleanup unneeded
barriers/docs later.

>> The "userspace" liburing looks even worse.  For starters,
>> `io_uring_get_completion` cannot safely return a pointer to the CQ entry
>> *AND* increment head: you need to actually read the entry before
>> incrementing head.
> 
> That's a good point, we should actually have this split in two to avoid
> the case where the kernel will then fill in a new entry for us.

I see you already did so in the liburing repo.

When going through liburing code again, I noticed io_uring_submit
assumes there might be pending submission entries in the SQ queue.

But those entries are not "reserved" in the SQE queue; so
io_uring_get_sqe might overwrite SQE data that isn't read by the kernel
through SQ yet.

Is there any motivation behind the indirection?  I see two possible ideas:
- allocate fixed SQE entries for operations, and just add them to SQ
  when needed
- have multiple threads fill SQE in parallel, and only add them to SQ
  when done

Are those actually worth the cost?

liburing doesn't look like it is going to take advantage of it, and the
library I'm writing in Rust right now isn't going to either (actually
even using a fixed sq_ndx == sqe_ndx mapping, i.e. it'll only write
sq->array on startup).

At least consider documenting the motivation :)

>> Last but not least the kernel side has two lines it checks whether a
>> queue is full or not and uses `tail + 1 == head`: this is only true if
>> there were U32_MAX entries in the queue.  You should check `(tail -
>> head) == SIZE` instead.
> 
> Good catch, this is a leftover from when the rings were indeed capped by
> the mask of the entries. I've fixed this one and pushed it out, and
> added a liburing test case for it as well.

I think you missed the second one in io_uring_poll (looking at your
for-linus branch), so I will send a patch for that too.

cheers,
Stefan