Re: [PATCH] io_uring: add io_uring_enter(2) fixed file support

[Vito Caputo <vcaputo@pengaru.com>]

On Thu, Mar 03, 2022 at 01:41:50PM -0700, Jens Axboe wrote:
> On 3/3/22 10:18 AM, Jens Axboe wrote:
> > On 3/3/22 9:31 AM, Jens Axboe wrote:
> >> On 3/3/22 7:40 AM, Jens Axboe wrote:
> >>> On 3/3/22 7:36 AM, Jens Axboe wrote:
> >>>> The only potential oddity here is that the fd passed back is not a
> >>>> legitimate fd. io_uring does support poll(2) on its file descriptor, so
> >>>> that could cause some confusion even if I don't think anyone actually
> >>>> does poll(2) on io_uring.
> >>>
> >>> Side note - the only implication here is that we then likely can't make
> >>> the optimized behavior the default, it has to be an IORING_SETUP_REG
> >>> flag which tells us that the application is aware of this limitation.
> >>> Though I guess close(2) might mess with that too... Hmm.
> >>
> >> Not sure I can find a good approach for that. Tried out your patch and
> >> made some fixes:
> >>
> >> - Missing free on final tctx free
> >> - Rename registered_files to registered_rings
> >> - Fix off-by-ones in checking max registration count
> >> - Use kcalloc
> >> - Rename ENTER_FIXED_FILE -> ENTER_REGISTERED_RING
> >> - Don't pass in tctx to io_uring_unreg_ringfd()
> >> - Get rid of forward declaration for adding tctx node
> >> - Get rid of extra file pointer in io_uring_enter()
> >> - Fix deadlock in io_ringfd_register()
> >> - Use io_uring_rsrc_update rather than add a new struct type
> > 
> > - Allow multiple register/unregister instead of enforcing just 1 at the
> >   time
> > - Check for it really being a ring fd when registering
> > 
> > For different batch counts, nice improvements are seen. Roughly:
> > 
> > Batch==1	15% faster
> > Batch==2	13% faster
> > Batch==4	11% faster
> > 
> > This is just in microbenchmarks where the fdget/fdput play a bigger
> > factor, but it will certainly help with real world situations where
> > batching is more limited than in benchmarks.
> 

Certainly seems worthwhile.

> In trying this out in applications, I think the better registration API
> is to allow io_uring to pick the offset. The application doesn't care,
> it's just a magic integer there. And if we allow io_uring to pick it,
> then that makes things a lot easier to deal with.
> 
> For registration, pass in an array of io_uring_rsrc_update structs, just
> filling in the ring_fd in the data field. Return value is number of ring
> fds registered, and up->offset now contains the chosen offset for each
> of them.
> 
> Unregister is the same struct, but just with offset filled in.
> 
> For applications using io_uring, which is all of them as far as I'm
> aware, we can also easily hide this. This means we can get the optimized
> behavior by default.
> 

Did you mean s/using io_uring/using liburing/ here?

Regards,
Vito Caputo