lkml.org 
[lkml]   [2022]   [Feb]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2] io_uring: avoid ring quiesce while registering/unregistering eventfd
From
Date


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

\
 
 \ /
  Last update: 2022-02-03 17:50    [W:0.444 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site