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

[Jens Axboe <axboe@kernel.dk>]

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);

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