Re: [RFC] io_uring: avoid ring quiesce while registering/unregistering eventfd

[Pavel Begunkov <asml.silence@gmail.com>]

On 2/2/22 18:32, Pavel Begunkov wrote:
> On 2/2/22 16:57, Jens Axboe wrote:
>> On 2/2/22 8:59 AM, Usama Arif wrote:
>>> Acquire completion_lock at the start of __io_uring_register before
>>> registering/unregistering eventfd and release it at the end. Hence
>>> all calls to io_cqring_ev_posted which adds to the eventfd counter
>>> will finish before acquiring the spin_lock in io_uring_register, and
>>> all new calls will wait till the eventfd is registered. This avoids
>>> ring quiesce which is much more expensive than acquiring the spin_lock.
>>>
>>> On the system tested with this patch, io_uring_reigster with
>>> IORING_REGISTER_EVENTFD takes less than 1ms, compared to 15ms before.
>>
>> This seems like optimizing for the wrong thing, so I've got a few
>> questions. Are you doing a lot of eventfd registrations (and unregister)
>> in your workload? Or is it just the initial pain of registering one? In
>> talking to Pavel, he suggested that RCU might be a good use case here,
>> and I think so too. That would still remove the need to quiesce, and the
>> posted side just needs a fairly cheap rcu read lock/unlock around it.
> 
> A bit more context:
> 
> 1) there is io_cqring_ev_posted_iopoll() which doesn't hold the lock
> and adding it will be expensive
> 
> 2) there is a not posted optimisation for io_cqring_ev_posted() relying
> on it being after spin_unlock.
> 
> 3) we don't want to unnecessarily extend the spinlock section, it's hot
> 
> 4) there is wake_up_all() inside, so there will be nested locks. That's
> bad for perf, but also because of potential deadlocking. E.g. poll
> requests do locking in reverse order. There might be more reasons.

5) there won't be sync with ctx->cq_ev_fd, and so no rules when
it will start to be visible. Will need to be solved if RCU.

there are probably more caveats, those are off the top of my head


> But there will be no complaints if you do,
> 
> if (evfd) {
>      rcu_read_lock();
>      eventfd_signal();
>      rcu_read_unlock();
> }
> 
> + some sync on the registration front
> 

-- 
Pavel Begunkov