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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 2 Feb 2022 18:39:15 +0000
From:   Pavel Begunkov <asml.silence@...il.com>
To:     Jens Axboe <axboe@...nel.dk>,
        Usama Arif <usama.arif@...edance.com>,
        io-uring@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     fam.zheng@...edance.com
Subject: Re: [RFC] io_uring: avoid ring quiesce while
 registering/unregistering eventfd

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