Messages in this thread | | | Subject | Re: [PATCH 1/2] io_uring: avoid ring quiesce while registering/unregistering eventfd | From | Usama Arif <> | Date | Thu, 3 Feb 2022 16:49:30 +0000 |
| |
On 03/02/2022 15:55, Jens Axboe wrote: > On 2/3/22 8:11 AM, Usama Arif wrote: >> +static void io_eventfd_signal(struct io_ring_ctx *ctx) >> +{ >> + struct io_ev_fd *ev_fd; >> + >> + rcu_read_lock(); >> + ev_fd = rcu_dereference(ctx->io_ev_fd); >> + >> + if (!io_should_trigger_evfd(ctx, ev_fd)) >> + goto out; >> + >> + eventfd_signal(ev_fd->cq_ev_fd, 1); >> +out: >> + rcu_read_unlock(); >> +} > > Would be cleaner as: > > static void io_eventfd_signal(struct io_ring_ctx *ctx) > { > struct io_ev_fd *ev_fd; > > rcu_read_lock(); > ev_fd = rcu_dereference(ctx->io_ev_fd); > > if (io_should_trigger_evfd(ctx, ev_fd)) > eventfd_signal(ev_fd->cq_ev_fd, 1); > > rcu_read_unlock(); > } > > and might be worth considering pulling in the io_should_trigger_evfd() > code rather than have it be a separate helper now with just the one > caller.
Hi, Thanks for the review. Have pulled in the code for io_should_trigger_evfd into io_eventfd_signal. > >> @@ -9353,35 +9374,67 @@ static int __io_sqe_buffers_update(struct io_ring_ctx *ctx, >> >> static int io_eventfd_register(struct io_ring_ctx *ctx, void __user *arg) >> { >> + struct io_ev_fd *ev_fd; >> __s32 __user *fds = arg; >> - int fd; >> + int fd, ret; >> >> - if (ctx->cq_ev_fd) >> - return -EBUSY; >> + mutex_lock(&ctx->ev_fd_lock); >> + ret = -EBUSY; >> + if (rcu_dereference_protected(ctx->io_ev_fd, lockdep_is_held(&ctx->ev_fd_lock))) >> + goto out; >> >> + ret = -EFAULT; >> if (copy_from_user(&fd, fds, sizeof(*fds))) >> - return -EFAULT; >> + goto out; >> >> - ctx->cq_ev_fd = eventfd_ctx_fdget(fd); >> - if (IS_ERR(ctx->cq_ev_fd)) { >> - int ret = PTR_ERR(ctx->cq_ev_fd); >> + ret = -ENOMEM; >> + ev_fd = kmalloc(sizeof(*ev_fd), GFP_KERNEL); >> + if (!ev_fd) >> + goto out; >> >> - ctx->cq_ev_fd = NULL; >> - return ret; >> + ev_fd->cq_ev_fd = eventfd_ctx_fdget(fd); >> + if (IS_ERR(ev_fd->cq_ev_fd)) { >> + ret = PTR_ERR(ev_fd->cq_ev_fd); >> + kfree(ev_fd); >> + goto out; >> } >> + ev_fd->ctx = ctx; >> >> - return 0; >> + rcu_assign_pointer(ctx->io_ev_fd, ev_fd); >> + ret = 0; >> + >> +out: >> + mutex_unlock(&ctx->ev_fd_lock); >> + return ret; >> +} > > One thing that both mine and your version suffers from is if someone > does an eventfd unregister, and then immediately does an eventfd > register. If the rcu grace period hasn't passed, we'll get -EBUSY on > trying to do that, when I think the right behavior there would be to > wait for the grace period to pass. > > I do think we need to handle that gracefully, spurious -EBUSY is > impossible for an application to deal with.
I don't think my version would suffer from this as its protected by locks? The mutex_unlock on ev_fd_lock in unregister happens only after the call_rcu. And the mutex is locked in io_eventfd_register at the start, so wouldnt get the -EBUSY if there is a register immediately after unregister? > >> @@ -11171,8 +11226,10 @@ SYSCALL_DEFINE4(io_uring_register, unsigned int, fd, unsigned int, opcode, >> mutex_lock(&ctx->uring_lock); >> ret = __io_uring_register(ctx, opcode, arg, nr_args); >> mutex_unlock(&ctx->uring_lock); >> + rcu_read_lock(); >> trace_io_uring_register(ctx, opcode, ctx->nr_user_files, ctx->nr_user_bufs, >> - ctx->cq_ev_fd != NULL, ret); >> + rcu_dereference(ctx->io_ev_fd) != NULL, ret); >> + rcu_read_unlock(); >> out_fput: >> fdput(f); >> return ret; > > We should probably just modify that tracepoint, kill that ev_fd argument > (it makes very little sense). >
Thanks! have added that in patch 1 in v2.
Regards, Usama
| |