Re: [PATCH] add a helper function to verify io_uring functionality

[Glauber Costa <glauber@scylladb.com>]

On Wed, Jan 29, 2020 at 9:28 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 1/29/20 5:42 PM, Glauber Costa wrote:
> >
> >
> > On Wed, Jan 29, 2020 at 3:55 PM Jens Axboe <axboe@kernel.dk <mailto:axboe@kernel.dk>> wrote:
> >
> >     On 1/29/20 12:20 PM, Glauber Costa wrote:
> >     > It is common for an application using an ever-evolving interface to want
> >     > to inquire about the presence of certain functionality it plans to use.
> >     >
> >     > The boilerplate to do that is about always the same: find places that
> >     > have feature bits, match that with what we need, rinse, repeat.
> >     > Therefore it makes sense to move this to a library function.
> >     >
> >     > We have two places in which we can check for such features: the feature
> >     > flag returned by io_uring_init_params(), and the resulting array
> >     > returning from io_uring_probe.
> >     >
> >     > I tried my best to communicate as well as possible in the function
> >     > signature the fact that this is not supposed to test the availability
> >     > of io_uring (which is straightforward enough), but rather a minimum set
> >     > of requirements for usage.
> >
> >     I wonder if we should have a helper that returns the fully allocated
> >     io_uring_probe struct filled out by probing the kernel. My main worry
> >     here is that some applications will probe for various things, each of
> >     which will setup/teardown a ring, and do the query.
> >
> >     Maybe it'd be enough to potentially pass in a ring?
> >
> >
> > Passing the ring is definitely doable.
>
> I think it's important we have both, so that an app can query without
> having a ring setup. But if it does, we should have the option of using
> that ring.
>
> >     While this patch works with a sparse command opcode field, not sure it's
> >     the most natural way. If we do the above, maybe we can just have a
> >     is_this_op_supported() query, since it'd be cheap if we already have the
> >     probe struct filled out?
> >
> >
> > So the user will be the one calling io_register_probe?
>
> Not necessarily, I'm thinking something ala:
>
> struct io_uring_probe *p
>
> p = io_uring_get_probe();
> /* call helper functions using 'p' */
> free(p);

ok. That makes  sense.

Thanks.

>
> and have io_uring_get_probe_ring() that takes the ring, for example. All
> depends on what the helpers might be then, I think that's the important
> part. The rest is just infrastructure to support it.
>
> Something like that, hope that makes sense.
>
> >     Outside of this discussion, some style changes are needed:
> >
> >     - '*' goes next to the name, struct foo *ptr, not struct foo* ptr
> >     - Some lines over 80 chars
> >
> >
> > Thanks! If you ever feel trapped with the 80 char stuff come write
> > some c++ seastar code with us!
>
> Such a tempting sell, C++ AND long lines ;-)
>
> > It's my bad for forgetting, I actually had a last pass on the patch
> > removing the {} after 1-line ifs so that was fun too
>
> No worries.
>
> --
> Jens Axboe
>