[PATCH] add a helper function to verify io_uring functionality

[PATCH] add a helper function to verify io_uring functionality

[Glauber Costa]

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.

Signed-off-by: Glauber Costa <glauber@scylladb.com>
CC: Avi Kivity <avi@scylladb.com>
---
 src/include/liburing.h | 13 +++++++++++++
 src/liburing.map       |  1 +
 src/setup.c            | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+)

diff --git a/src/include/liburing.h b/src/include/liburing.h
index 83d11dd..d740083 100644
--- a/src/include/liburing.h
+++ b/src/include/liburing.h
@@ -72,6 +72,19 @@ struct io_uring {
 /*
  * Library interface
  */
+
+/* Checks that io_uring is modern enough for a particular case.
+ * Check it by verifying that:
+ *
+ *  - io_uring is available
+ *  - the io_uring_probe call is available, so opcodes can be checked
+ *  - all opcodes the application wants to use are supported
+ *  - the features requested are present.
+ *
+ *  return 0 if io_uring is not usable, 1 otherwise.
+ */
+extern int io_uring_check_minimum_support(const int* operations, int noperations, int features);
+
 extern int io_uring_queue_init_params(unsigned entries, struct io_uring *ring,
 	struct io_uring_params *p);
 extern int io_uring_queue_init(unsigned entries, struct io_uring *ring,
diff --git a/src/liburing.map b/src/liburing.map
index b45f373..579d4de 100644
--- a/src/liburing.map
+++ b/src/liburing.map
@@ -72,4 +72,5 @@ LIBURING_0.4 {
 		io_uring_register_probe;
 		io_uring_register_personality;
 		io_uring_unregister_personality;
+		io_uring_check_minimum_support;
 } LIBURING_0.3;
diff --git a/src/setup.c b/src/setup.c
index c53f234..7e46219 100644
--- a/src/setup.c
+++ b/src/setup.c
@@ -4,6 +4,7 @@
 #include <unistd.h>
 #include <errno.h>
 #include <string.h>
+#include <stdlib.h>
 
 #include "liburing/compat.h"
 #include "liburing/io_uring.h"
@@ -167,3 +168,41 @@ void io_uring_queue_exit(struct io_uring *ring)
 	io_uring_unmap_rings(sq, cq);
 	close(ring->ring_fd);
 }
+
+int io_uring_check_minimum_support(const int* operations, int noperations, int features)
+{
+	struct io_uring_params p;
+	struct io_uring_probe* probe;
+	struct io_uring ring;
+	int r;
+	int i;
+	int ret = 0;
+
+	memset(&p, 0, sizeof(p));
+	r = io_uring_queue_init_params(2, &ring, &p);
+	if (r < 0)
+		return ret;
+
+	if ((p.features & features) != features)
+		goto exit;
+
+	size_t len = sizeof(*probe) + 256 * sizeof(struct io_uring_probe_op);
+	probe = malloc(len);
+	memset(probe, 0, len);
+	r = io_uring_register_probe(&ring, probe, 256);
+	if (r < 0)
+		goto exit;
+
+	for (i = 0; i < noperations; i++) {
+		int op = operations[i];
+		if (probe->last_op < op)
+			goto exit;
+
+		if (!(probe->ops[op].flags & IO_URING_OP_SUPPORTED))
+			goto exit;
+	}
+	ret = 1;
+exit:
+	io_uring_queue_exit(&ring);
+	return ret;
+}
-- 
2.20.1

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

[Jens Axboe]

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?

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?

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

-- 
Jens Axboe

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

[Jens Axboe]

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

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

[Glauber Costa]

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
>