Re: [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers

On Tue, Jan 29, 2019 at 12:50 AM Jens Axboe [off-list ref] wrote:

On 1/28/19 4:35 PM, Jann Horn wrote:

quoted

On Mon, Jan 28, 2019 at 10:36 PM Jens Axboe [off-list ref] wrote:

quoted

If we have fixed user buffers, we can map them into the kernel when we
setup the io_context. That avoids the need to do get_user_pages() for
each and every IO.

[...]

quoted

+static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
+                              void __user *arg, unsigned nr_args)
+{
+       int ret;
+
+       /* Drop our initial ref and wait for the ctx to be fully idle */
+       percpu_ref_put(&ctx->refs);

The line above drops a reference that you just got in the caller...

Right

quoted

quoted

+       percpu_ref_kill(&ctx->refs);
+       wait_for_completion(&ctx->ctx_done);
+
+       switch (opcode) {
+       case IORING_REGISTER_BUFFERS:
+               ret = io_sqe_buffer_register(ctx, arg, nr_args);
+               break;
+       case IORING_UNREGISTER_BUFFERS:
+               ret = -EINVAL;
+               if (arg || nr_args)
+                       break;
+               ret = io_sqe_buffer_unregister(ctx);
+               break;
+       default:
+               ret = -EINVAL;
+               break;
+       }
+
+       /* bring the ctx back to life */
+       reinit_completion(&ctx->ctx_done);
+       percpu_ref_resurrect(&ctx->refs);
+       percpu_ref_get(&ctx->refs);

And then this line takes a reference that the caller will immediately
drop again? Why?

Just want to keep it symmetric and avoid having weird "this function drops
a reference" use cases.

quoted

quoted

+       return ret;
+}
+
+SYSCALL_DEFINE4(io_uring_register, unsigned int, fd, unsigned int, opcode,
+               void __user *, arg, unsigned int, nr_args)
+{
+       struct io_ring_ctx *ctx;
+       long ret = -EBADF;
+       struct fd f;
+
+       f = fdget(fd);
+       if (!f.file)
+               return -EBADF;
+
+       ret = -EOPNOTSUPP;
+       if (f.file->f_op != &io_uring_fops)
+               goto out_fput;
+
+       ret = -ENXIO;
+       ctx = f.file->private_data;
+       if (!percpu_ref_tryget(&ctx->refs))
+               goto out_fput;

If you are holding the uring_lock of a ctx that can be accessed
through a file descriptor (which you do just after this point), you
know that the percpu_ref isn't zero, right? Why are you doing the
tryget here?

Not sure I follow... We don't hold the lock at this point. I guess your
point is that since the descriptor is open (or we'd fail the above
check), then there's no point doing the tryget variant here? That's
strictly true, that could just be a get().

As far as I can tell, you could do the following without breaking anything:

========================

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 6916dc3222cf..c2d82765eefe 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c

@@ -2485,7 +2485,6 @@ static int __io_uring_register(struct

io_ring_ctx *ctx, unsigned opcode,
        int ret;

        /* Drop our initial ref and wait for the ctx to be fully idle */
-       percpu_ref_put(&ctx->refs);
        percpu_ref_kill(&ctx->refs);
        wait_for_completion(&ctx->ctx_done);

@@ -2516,7 +2515,6 @@ static int __io_uring_register(struct

io_ring_ctx *ctx, unsigned opcode,
        /* bring the ctx back to life */
        reinit_completion(&ctx->ctx_done);
        percpu_ref_resurrect(&ctx->refs);
-       percpu_ref_get(&ctx->refs);
        return ret;
 }

@@ -2535,17 +2533,13 @@ SYSCALL_DEFINE4(io_uring_register, unsigned

int, fd, unsigned int, opcode,
        if (f.file->f_op != &io_uring_fops)
                goto out_fput;

-       ret = -ENXIO;
        ctx = f.file->private_data;
-       if (!percpu_ref_tryget(&ctx->refs))
-               goto out_fput;

        ret = -EBUSY;
        if (mutex_trylock(&ctx->uring_lock)) {
                ret = __io_uring_register(ctx, opcode, arg, nr_args);
                mutex_unlock(&ctx->uring_lock);
        }
-       io_ring_drop_ctx_refs(ctx, 1);
 out_fput:
        fdput(f);
        return ret;
========================

The two functions that can drop the initial ref of the percpu refcount are:

1. io_ring_ctx_wait_and_kill(); this is only used on ->release() or on
setup failure, meaning that as long as you have a reference to the
file from fget()/fdget(), io_ring_ctx_wait_and_kill() can't have been
called on your context
2. __io_uring_register(); this temporarily kills the percpu refcount
and resurrects it, all under ctx->uring_lock, meaning that as long as
you're holding ctx->uring_lock, __io_uring_register() can't have
killed the percpu refcount

Therefore, I think that as long as you're in sys_io_uring_register and
holding the ctx->uring_lock, you know that the percpu refcount is
alive, and bumping and dropping non-initial references has no effect.

Perhaps this makes more sense when you view the percpu refcount as a
read/write lock - percpu_ref_tryget() takes a read lock, the
percpu_ref_kill() dance takes a write lock.

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>