Re: [External] Re: [PATCH v4 2/3] io_uring: avoid ring quiesce while registering/unregistering eventfd

[Usama Arif]

On 03/02/2022 18:49, Jens Axboe wrote:
> On 2/3/22 11:24 AM, Usama Arif wrote:
> > -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();
> >   }
>
> This still needs what we discussed in v3, something ala:
>
> /*
>   * This will potential race with eventfd registration, but that's
>   * always going to be the case if there is IO inflight while an eventfd
>   * descriptor is being registered.
>   */
> if (!rcu_dereference_raw(ctx->io_ev_fd))
> 	return;
>
> rcu_read_lock();

Hmm, so i am not so worried about the registeration, but actually 
worried about unregisteration.
If after the check and before the rcu_read_lock, the eventfd is 
unregistered won't we get a NULL pointer exception at 
eventfd_signal(ev_fd->cq_ev_fd, 1)?

I guess checking for NULL twice would work, so something like this is ok 
then?

static void io_eventfd_signal(struct io_ring_ctx *ctx)
{
	struct io_ev_fd *ev_fd;

	/* Return quickly if ctx->io_ev_fd doesn't exist */
	if (likely(!rcu_dereference_raw(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);

	/*
	 * Check again if ev_fd exists incase an io_eventfd_unregister call 
completed between
	 * the NULL check of ctx->io_ev_fd at the start of the function and 
rcu_read_lock.
	 */
	if (unlikely(!ev_fd))
		goto out;
	if (READ_ONCE(ctx->rings->cq_flags) & IORING_CQ_EVENTFD_DISABLED)
		goto out;

	if (!ev_fd->eventfd_async || io_wq_current_is_worker())
		eventfd_signal(ev_fd->cq_ev_fd, 1);

out:
	rcu_read_unlock();
}


> ...
>
> which I think is cheap enough and won't hit sparse complaints. The
>
> > @@ -9353,35 +9370,70 @@ 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))) {
> > +		rcu_barrier();
> > +		if(rcu_dereference_protected(ctx->io_ev_fd, lockdep_is_held(&ctx->ev_fd_lock)))
> > +			goto out;
> > +	}
>
> I wonder if we can get away with assigning ctx->io_ev_fd to NULL when we
> do the call_rcu(). The struct itself will remain valid as long as we're
> under rcu_read_lock() protection, so I think we'd be fine? If we do
> that, then we don't need any rcu_barrier() or synchronize_rcu() calls,
> as we can register a new one while the previous one is still being
> killed.
>
> Hmm?

We would have to remove the check that ctx->io_ev_fd != NULL. That we 
would also result in 2 successive calls to io_eventfd_register without 
any unregister in between being successful? Which i dont think is the 
right behaviour?

I think the likelihood of hitting the rcu_barrier itself is quite low, 
so probably the cost is low as well.

> >   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) {
> > +		call_rcu(&ev_fd->rcu, io_eventfd_put);
> > +		ret = 0;
> > +		goto out;
> >   	}
> > +	ret = -ENXIO;
> >   
> > -	return -ENXIO;
> > +out:
> > +	mutex_unlock(&ctx->ev_fd_lock);
> > +	return ret;
> >   }
>
> I also think that'd be cleaner without the goto:
>
> {
> 	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) {
> 		call_rcu(&ev_fd->rcu, io_eventfd_put);
> 		mutex_unlock(&ctx->ev_fd_lock);
> 		return 0;
> 	}
>
> 	mutex_unlock(&ctx->ev_fd_lock);
> 	return -ENXIO;
> }
Thanks, will do that this in the next patchset with the above 
io_eventfd_signal changes if those look ok as well?