Re: [PATCH 1/1] io_uring: fix missing mb() before waitqueue_active

From: Jens Axboe <axboe@kernel.dk>
To: Pavel Begunkov <asml.silence@gmail.com>, io-uring@vger.kernel.org
Subject: Re: [PATCH 1/1] io_uring: fix missing mb() before waitqueue_active
Date: Wed, 8 Sep 2021 14:24:42 -0600	[thread overview]
Message-ID: <1c8641b7-9817-e611-0cc9-d98ff551e13c@kernel.dk> (raw)
In-Reply-To: <c6fa008c-2705-90cf-b3ff-31b9b58e323b@gmail.com>

On 9/8/21 2:22 PM, Pavel Begunkov wrote:
> On 9/8/21 9:15 PM, Jens Axboe wrote:
>> On 9/8/21 2:09 PM, Pavel Begunkov wrote:
>>> On 9/8/21 8:57 PM, Jens Axboe wrote:
>>>> On 9/8/21 1:49 PM, Pavel Begunkov wrote:
>>>>> In case of !SQPOLL, io_cqring_ev_posted_iopoll() doesn't provide a
>>>>> memory barrier required by waitqueue_active(&ctx->poll_wait). There is
>>>>> a wq_has_sleeper(), which does smb_mb() inside, but it's called only for
>>>>> SQPOLL.
>>>>
>>>> We can probably get rid of the need to even do so by having the slow
>>>> path (eg someone waiting on cq_wait or poll_wait) a bit more expensive,
>>>> but this should do for now.
>>>
>>> You have probably seen smp_mb__after_spin_unlock() trick [1], easy way
>>> to get rid of it for !IOPOLL. Haven't figured it out for IOPOLL, though
>>>
>>> [1] https://github.com/isilence/linux/commit/bb391b10d0555ba2d55aa8ee0a08dff8701a6a57
>>
>> We can just synchronize the poll_wait() with a spinlock. It's kind of silly,
>> and it's especially silly since I bet nobody does poll(2) on the ring fd for
>> IOPOLL, but...
> 
> fwiw, for the ebpf cat ev_posted() -> smb_mb() for taking ~3-5%.
> And there are non-bpf cases that may benefit from it.
> 
> On my list to publish a refined version of the patch.  

Maybe let's postpone this patch then and see if we can't do better...

-- 
Jens Axboe