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

[Usama Arif <usama.arif@bytedance.com>]

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