lkml.org 
[lkml]   [2022]   [Feb]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [External] Re: [PATCH v3 2/3] io_uring: avoid ring quiesce while registering/unregistering eventfd
From
Date


On 03/02/2022 17:56, Jens Axboe wrote:
> On 2/3/22 10:41 AM, Usama Arif wrote:
>> @@ -1726,13 +1732,24 @@ static inline struct io_uring_cqe *io_get_cqe(struct io_ring_ctx *ctx)
>> return &rings->cqes[tail & mask];
>> }
>>
>> -static inline bool io_should_trigger_evfd(struct io_ring_ctx *ctx)
>> +static void io_eventfd_signal(struct io_ring_ctx *ctx)
>> {
>> - if (likely(!ctx->cq_ev_fd))
>> - return false;
>> + struct io_ev_fd *ev_fd;
>> +
>> + rcu_read_lock();
>> + /* rcu_dereference ctx->io_ev_fd once and use it for both for checking and eventfd_signal */
>> + ev_fd = rcu_dereference(ctx->io_ev_fd);
>> +
>> + if (likely(!ev_fd))
>> + goto out;
>> if (READ_ONCE(ctx->rings->cq_flags) & IORING_CQ_EVENTFD_DISABLED)
>> - return false;
>> - return !ctx->eventfd_async || io_wq_current_is_worker();
>> + goto out;
>> +
>> + if (!ctx->eventfd_async || io_wq_current_is_worker())
>> + eventfd_signal(ev_fd->cq_ev_fd, 1);
>> +
>> +out:
>> + rcu_read_unlock();
>> }
>
> Like Pavel pointed out, we still need the fast path (of not having an
> event fd registered at all) to just do the cheap check and not need rcu
> lock/unlock. Outside of that, I think this looks fine.
>

Hmm, maybe i didn't understand you and Pavel correctly. Are you
suggesting to do the below diff over patch 3? I dont think that would be
correct, as it is possible that just after checking if ctx->io_ev_fd is
present unregister can be called by another thread and set ctx->io_ev_fd
to NULL that would cause a NULL pointer exception later? In the current
patch, the check of whether ev_fd exists happens as the first thing
after rcu_read_lock and the rcu_read_lock are extremely cheap i believe.

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 25ed86533910..0cf282fba14d 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1736,12 +1736,13 @@ static void io_eventfd_signal(struct io_ring_ctx
*ctx)
{
struct io_ev_fd *ev_fd;

+ if (likely(!ctx->io_ev_fd))
+ return;
+
rcu_read_lock();
/* rcu_dereference ctx->io_ev_fd once and use it for both for
checking and eventfd_signal */
ev_fd = rcu_dereference(ctx->io_ev_fd);

- if (likely(!ev_fd))
- goto out;
if (READ_ONCE(ctx->rings->cq_flags) & IORING_CQ_EVENTFD_DISABLED)
goto out;



>> static int io_eventfd_unregister(struct io_ring_ctx *ctx)
>> {
>> - if (ctx->cq_ev_fd) {
>> - eventfd_ctx_put(ctx->cq_ev_fd);
>> - ctx->cq_ev_fd = NULL;
>> - return 0;
>> + struct io_ev_fd *ev_fd;
>> + int ret;
>> +
>> + mutex_lock(&ctx->ev_fd_lock);
>> + ev_fd = rcu_dereference_protected(ctx->io_ev_fd, lockdep_is_held(&ctx->ev_fd_lock));
>> + if (!ev_fd) {
>> + ret = -ENXIO;
>> + goto out;
>> }
>> + synchronize_rcu();
>> + eventfd_ctx_put(ev_fd->cq_ev_fd);
>> + kfree(ev_fd);
>> + rcu_assign_pointer(ctx->io_ev_fd, NULL);
>> + ret = 0;
>>
>> - return -ENXIO;
>> +out:
>> + mutex_unlock(&ctx->ev_fd_lock);
>> + return ret;
>> }
>
> synchronize_rcu() can take a long time, and I think this is in the wrong
> spot. It should be on the register side, IFF we need to expedite the
> completion of a previous event fd unregistration. If we do it that way,
> at least it'll only happen if it's necessary. What do you think?
>


How about the approach in v4? so switching back to call_rcu as in v2 and
if ctx->io_ev_fd is NULL then we call rcu_barrier to make sure all rcu
callbacks are finished and check for NULL again.

Thanks!
Usama

\
 
 \ /
  Last update: 2022-02-03 19:26    [W:1.320 / U:0.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site