Re: [PATCH 1/2] io_uring: avoid ring quiesce while registering/unregistering eventfd

[Usama Arif]

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