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

[Jens Axboe]

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.

> @@ -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.

> @@ -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).

-- 
Jens Axboe