Re: [PATCH 05/19] Add io_uring IO interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2/10/19 5:03 AM, Thomas Gleixner wrote:
> On Sat, 9 Feb 2019, Jens Axboe wrote:
>> +static void io_commit_cqring(struct io_ring_ctx *ctx)
>> +{
>> +	struct io_cq_ring *ring = ctx->cq_ring;
>> +
>> +	if (ctx->cached_cq_tail != READ_ONCE(ring->r.tail)) {
>> +		/* order cqe stores with ring update */
> 
> This lacks a reference to the matching rmb()
> 
>> +		smp_wmb();
>> +		WRITE_ONCE(ring->r.tail, ctx->cached_cq_tail);
>> +		/* write side barrier of tail update, app has read side */
> 
> That's a bit meager. Can you please document the barriers which are paired
> with user space barriers very elaborate?

Agree, I added a lengthier comment at the top of the file to explain the
relationship between the kernel and application barriers.

Also documented all of them explicitly now, or referenced the top
comment.

>> +		if (READ_ONCE(ring->r.head) != READ_ONCE(ring->r.tail))
>> +			break;
>> +
> 
> There are undocumented smp_wmb()'s in 'io_uring: Add submission polling' as
> well. It's really hard to tell where the corresponding barriers are and
> what they are supposed to order.

Fixed those too.

> Especially the barriers which are paired with user space barriers need some
> careful documentation. What are the side effects if user space is missing a
> barrier? Just user space seing unconsistent data or is there something
> which goes the other way round and might cause havoc in the kernel?

The only side effect is that the application may not proceed as quickly
as it should, if it fails to notice a completion event come in. It'll
lead to it calling io_cqring_wait() ultimately, through a system call,
which will then ensure that it makes progress. But it may not have
needed to do that system call at all, if it used the barriers correctly.

I've also added a reference to the liburing git repo which has examples
of how to do it.

Thanks for your comments!

-- 
Jens Axboe




[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux