2020-07-16 12:49:38

by Stefano Garzarella

Subject: [PATCH RFC v2 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes

The enumeration allows us to keep track of the last
io_uring_register(2) opcode available.

Behaviour and opcodes names don't change.

Signed-off-by: Stefano Garzarella <[email protected]>
---
include/uapi/linux/io_uring.h | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 7843742b8b74..efc50bd0af34 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -253,17 +253,22 @@ struct io_uring_params {
/*
* io_uring_register(2) opcodes and arguments
*/
-#define IORING_REGISTER_BUFFERS 0
-#define IORING_UNREGISTER_BUFFERS 1
-#define IORING_REGISTER_FILES 2
-#define IORING_UNREGISTER_FILES 3
-#define IORING_REGISTER_EVENTFD 4
-#define IORING_UNREGISTER_EVENTFD 5
-#define IORING_REGISTER_FILES_UPDATE 6
-#define IORING_REGISTER_EVENTFD_ASYNC 7
-#define IORING_REGISTER_PROBE 8
-#define IORING_REGISTER_PERSONALITY 9
-#define IORING_UNREGISTER_PERSONALITY 10
+enum {
+ IORING_REGISTER_BUFFERS,
+ IORING_UNREGISTER_BUFFERS,
+ IORING_REGISTER_FILES,
+ IORING_UNREGISTER_FILES,
+ IORING_REGISTER_EVENTFD,
+ IORING_UNREGISTER_EVENTFD,
+ IORING_REGISTER_FILES_UPDATE,
+ IORING_REGISTER_EVENTFD_ASYNC,
+ IORING_REGISTER_PROBE,
+ IORING_REGISTER_PERSONALITY,
+ IORING_UNREGISTER_PERSONALITY,
+
+ /* this goes last */
+ IORING_REGISTER_LAST
+};

struct io_uring_files_update {
__u32 offset;
--
2.26.2

2020-07-16 20:19:08

by Pavel Begunkov

Subject: Re: [PATCH RFC v2 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes

On 16/07/2020 15:48, Stefano Garzarella wrote:
> The enumeration allows us to keep track of the last
> io_uring_register(2) opcode available.
>
> Behaviour and opcodes names don't change.
>
> Signed-off-by: Stefano Garzarella <[email protected]>
> ---
> include/uapi/linux/io_uring.h | 27 ++++++++++++++++-----------
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index 7843742b8b74..efc50bd0af34 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -253,17 +253,22 @@ struct io_uring_params {
> /*
> * io_uring_register(2) opcodes and arguments
> */
> -#define IORING_REGISTER_BUFFERS 0
> -#define IORING_UNREGISTER_BUFFERS 1
> -#define IORING_REGISTER_FILES 2
> -#define IORING_UNREGISTER_FILES 3
> -#define IORING_REGISTER_EVENTFD 4
> -#define IORING_UNREGISTER_EVENTFD 5
> -#define IORING_REGISTER_FILES_UPDATE 6
> -#define IORING_REGISTER_EVENTFD_ASYNC 7
> -#define IORING_REGISTER_PROBE 8
> -#define IORING_REGISTER_PERSONALITY 9
> -#define IORING_UNREGISTER_PERSONALITY 10
> +enum {
> + IORING_REGISTER_BUFFERS,
> + IORING_UNREGISTER_BUFFERS,
> + IORING_REGISTER_FILES,
> + IORING_UNREGISTER_FILES,
> + IORING_REGISTER_EVENTFD,
> + IORING_UNREGISTER_EVENTFD,
> + IORING_REGISTER_FILES_UPDATE,
> + IORING_REGISTER_EVENTFD_ASYNC,
> + IORING_REGISTER_PROBE,
> + IORING_REGISTER_PERSONALITY,
> + IORING_UNREGISTER_PERSONALITY,
> +
> + /* this goes last */
> + IORING_REGISTER_LAST
> +};

It breaks userspace API. E.g.

#ifdef IORING_REGISTER_BUFFERS

>
> struct io_uring_files_update {
> __u32 offset;
>

--
Pavel Begunkov

2020-07-16 20:44:54

by Jens Axboe

Subject: Re: [PATCH RFC v2 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes

On 7/16/20 2:16 PM, Pavel Begunkov wrote:
> On 16/07/2020 15:48, Stefano Garzarella wrote:
>> The enumeration allows us to keep track of the last
>> io_uring_register(2) opcode available.
>>
>> Behaviour and opcodes names don't change.
>>
>> Signed-off-by: Stefano Garzarella <[email protected]>
>> ---
>> include/uapi/linux/io_uring.h | 27 ++++++++++++++++-----------
>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>> index 7843742b8b74..efc50bd0af34 100644
>> --- a/include/uapi/linux/io_uring.h
>> +++ b/include/uapi/linux/io_uring.h
>> @@ -253,17 +253,22 @@ struct io_uring_params {
>> /*
>> * io_uring_register(2) opcodes and arguments
>> */
>> -#define IORING_REGISTER_BUFFERS 0
>> -#define IORING_UNREGISTER_BUFFERS 1
>> -#define IORING_REGISTER_FILES 2
>> -#define IORING_UNREGISTER_FILES 3
>> -#define IORING_REGISTER_EVENTFD 4
>> -#define IORING_UNREGISTER_EVENTFD 5
>> -#define IORING_REGISTER_FILES_UPDATE 6
>> -#define IORING_REGISTER_EVENTFD_ASYNC 7
>> -#define IORING_REGISTER_PROBE 8
>> -#define IORING_REGISTER_PERSONALITY 9
>> -#define IORING_UNREGISTER_PERSONALITY 10
>> +enum {
>> + IORING_REGISTER_BUFFERS,
>> + IORING_UNREGISTER_BUFFERS,
>> + IORING_REGISTER_FILES,
>> + IORING_UNREGISTER_FILES,
>> + IORING_REGISTER_EVENTFD,
>> + IORING_UNREGISTER_EVENTFD,
>> + IORING_REGISTER_FILES_UPDATE,
>> + IORING_REGISTER_EVENTFD_ASYNC,
>> + IORING_REGISTER_PROBE,
>> + IORING_REGISTER_PERSONALITY,
>> + IORING_UNREGISTER_PERSONALITY,
>> +
>> + /* this goes last */
>> + IORING_REGISTER_LAST
>> +};
>
> It breaks userspace API. E.g.
>
> #ifdef IORING_REGISTER_BUFFERS

It can, yes, but we have done that in the past. In this one, for
example:

commit 9e3aa61ae3e01ce1ce6361a41ef725e1f4d1d2bf (tag: io_uring-5.5-20191212)
Author: Jens Axboe <[email protected]>
Date: Wed Dec 11 15:55:43 2019 -0700

io_uring: ensure we return -EINVAL on unknown opcod

But it would be safer/saner to do this like we have the done the IOSQE_
flags.

--
Jens Axboe

2020-07-16 20:52:04

by Jens Axboe

Subject: Re: [PATCH RFC v2 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes

On 7/16/20 2:47 PM, Pavel Begunkov wrote:
> On 16/07/2020 23:42, Jens Axboe wrote:
>> On 7/16/20 2:16 PM, Pavel Begunkov wrote:
>>> On 16/07/2020 15:48, Stefano Garzarella wrote:
>>>> The enumeration allows us to keep track of the last
>>>> io_uring_register(2) opcode available.
>>>>
>>>> Behaviour and opcodes names don't change.
>>>>
>>>> Signed-off-by: Stefano Garzarella <[email protected]>
>>>> ---
>>>> include/uapi/linux/io_uring.h | 27 ++++++++++++++++-----------
>>>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>>>> index 7843742b8b74..efc50bd0af34 100644
>>>> --- a/include/uapi/linux/io_uring.h
>>>> +++ b/include/uapi/linux/io_uring.h
>>>> @@ -253,17 +253,22 @@ struct io_uring_params {
>>>> /*
>>>> * io_uring_register(2) opcodes and arguments
>>>> */
>>>> -#define IORING_REGISTER_BUFFERS 0
>>>> -#define IORING_UNREGISTER_BUFFERS 1
>>>> -#define IORING_REGISTER_FILES 2
>>>> -#define IORING_UNREGISTER_FILES 3
>>>> -#define IORING_REGISTER_EVENTFD 4
>>>> -#define IORING_UNREGISTER_EVENTFD 5
>>>> -#define IORING_REGISTER_FILES_UPDATE 6
>>>> -#define IORING_REGISTER_EVENTFD_ASYNC 7
>>>> -#define IORING_REGISTER_PROBE 8
>>>> -#define IORING_REGISTER_PERSONALITY 9
>>>> -#define IORING_UNREGISTER_PERSONALITY 10
>>>> +enum {
>>>> + IORING_REGISTER_BUFFERS,
>>>> + IORING_UNREGISTER_BUFFERS,
>>>> + IORING_REGISTER_FILES,
>>>> + IORING_UNREGISTER_FILES,
>>>> + IORING_REGISTER_EVENTFD,
>>>> + IORING_UNREGISTER_EVENTFD,
>>>> + IORING_REGISTER_FILES_UPDATE,
>>>> + IORING_REGISTER_EVENTFD_ASYNC,
>>>> + IORING_REGISTER_PROBE,
>>>> + IORING_REGISTER_PERSONALITY,
>>>> + IORING_UNREGISTER_PERSONALITY,
>>>> +
>>>> + /* this goes last */
>>>> + IORING_REGISTER_LAST
>>>> +};
>>>
>>> It breaks userspace API. E.g.
>>>
>>> #ifdef IORING_REGISTER_BUFFERS
>>
>> It can, yes, but we have done that in the past. In this one, for
>
> Ok, if nobody on the userspace side cares, then better to do that
> sooner than later.
>
>
>> example:
>>
>> commit 9e3aa61ae3e01ce1ce6361a41ef725e1f4d1d2bf (tag: io_uring-5.5-20191212)
>> Author: Jens Axboe <[email protected]>
>> Date: Wed Dec 11 15:55:43 2019 -0700
>>
>> io_uring: ensure we return -EINVAL on unknown opcod
>>
>> But it would be safer/saner to do this like we have the done the IOSQE_
>> flags.
>
> IOSQE_ are a bitmask, but this would look peculiar
>
> enum {
> __IORING_REGISTER_BUFFERS,
> ...
> };
> define IORING_REGISTER_BUFFERS __IORING_REGISTER_BUFFERS

Yeah true of course, that won't really work for this case at all.

That said, I don't think it's a huge deal to turn it into an enum.


--
Jens Axboe

2020-07-16 20:52:47

by Pavel Begunkov

Subject: Re: [PATCH RFC v2 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes

On 16/07/2020 23:42, Jens Axboe wrote:
> On 7/16/20 2:16 PM, Pavel Begunkov wrote:
>> On 16/07/2020 15:48, Stefano Garzarella wrote:
>>> The enumeration allows us to keep track of the last
>>> io_uring_register(2) opcode available.
>>>
>>> Behaviour and opcodes names don't change.
>>>
>>> Signed-off-by: Stefano Garzarella <[email protected]>
>>> ---
>>> include/uapi/linux/io_uring.h | 27 ++++++++++++++++-----------
>>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>>> index 7843742b8b74..efc50bd0af34 100644
>>> --- a/include/uapi/linux/io_uring.h
>>> +++ b/include/uapi/linux/io_uring.h
>>> @@ -253,17 +253,22 @@ struct io_uring_params {
>>> /*
>>> * io_uring_register(2) opcodes and arguments
>>> */
>>> -#define IORING_REGISTER_BUFFERS 0
>>> -#define IORING_UNREGISTER_BUFFERS 1
>>> -#define IORING_REGISTER_FILES 2
>>> -#define IORING_UNREGISTER_FILES 3
>>> -#define IORING_REGISTER_EVENTFD 4
>>> -#define IORING_UNREGISTER_EVENTFD 5
>>> -#define IORING_REGISTER_FILES_UPDATE 6
>>> -#define IORING_REGISTER_EVENTFD_ASYNC 7
>>> -#define IORING_REGISTER_PROBE 8
>>> -#define IORING_REGISTER_PERSONALITY 9
>>> -#define IORING_UNREGISTER_PERSONALITY 10
>>> +enum {
>>> + IORING_REGISTER_BUFFERS,
>>> + IORING_UNREGISTER_BUFFERS,
>>> + IORING_REGISTER_FILES,
>>> + IORING_UNREGISTER_FILES,
>>> + IORING_REGISTER_EVENTFD,
>>> + IORING_UNREGISTER_EVENTFD,
>>> + IORING_REGISTER_FILES_UPDATE,
>>> + IORING_REGISTER_EVENTFD_ASYNC,
>>> + IORING_REGISTER_PROBE,
>>> + IORING_REGISTER_PERSONALITY,
>>> + IORING_UNREGISTER_PERSONALITY,
>>> +
>>> + /* this goes last */
>>> + IORING_REGISTER_LAST
>>> +};
>>
>> It breaks userspace API. E.g.
>>
>> #ifdef IORING_REGISTER_BUFFERS
>
> It can, yes, but we have done that in the past. In this one, for

Ok, if nobody on the userspace side cares, then better to do that
sooner than later.


> example:
>
> commit 9e3aa61ae3e01ce1ce6361a41ef725e1f4d1d2bf (tag: io_uring-5.5-20191212)
> Author: Jens Axboe <[email protected]>
> Date: Wed Dec 11 15:55:43 2019 -0700
>
> io_uring: ensure we return -EINVAL on unknown opcod
>
> But it would be safer/saner to do this like we have the done the IOSQE_
> flags.

IOSQE_ are a bitmask, but this would look peculiar

enum {
__IORING_REGISTER_BUFFERS,
...
};
define IORING_REGISTER_BUFFERS __IORING_REGISTER_BUFFERS

--
Pavel Begunkov

2020-07-16 21:23:50

by Jens Axboe

Subject: Re: [PATCH RFC v2 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes

On 7/16/20 2:51 PM, Jens Axboe wrote:
> On 7/16/20 2:47 PM, Pavel Begunkov wrote:
>> On 16/07/2020 23:42, Jens Axboe wrote:
>>> On 7/16/20 2:16 PM, Pavel Begunkov wrote:
>>>> On 16/07/2020 15:48, Stefano Garzarella wrote:
>>>>> The enumeration allows us to keep track of the last
>>>>> io_uring_register(2) opcode available.
>>>>>
>>>>> Behaviour and opcodes names don't change.
>>>>>
>>>>> Signed-off-by: Stefano Garzarella <[email protected]>
>>>>> ---
>>>>> include/uapi/linux/io_uring.h | 27 ++++++++++++++++-----------
>>>>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>>>>> index 7843742b8b74..efc50bd0af34 100644
>>>>> --- a/include/uapi/linux/io_uring.h
>>>>> +++ b/include/uapi/linux/io_uring.h
>>>>> @@ -253,17 +253,22 @@ struct io_uring_params {
>>>>> /*
>>>>> * io_uring_register(2) opcodes and arguments
>>>>> */
>>>>> -#define IORING_REGISTER_BUFFERS 0
>>>>> -#define IORING_UNREGISTER_BUFFERS 1
>>>>> -#define IORING_REGISTER_FILES 2
>>>>> -#define IORING_UNREGISTER_FILES 3
>>>>> -#define IORING_REGISTER_EVENTFD 4
>>>>> -#define IORING_UNREGISTER_EVENTFD 5
>>>>> -#define IORING_REGISTER_FILES_UPDATE 6
>>>>> -#define IORING_REGISTER_EVENTFD_ASYNC 7
>>>>> -#define IORING_REGISTER_PROBE 8
>>>>> -#define IORING_REGISTER_PERSONALITY 9
>>>>> -#define IORING_UNREGISTER_PERSONALITY 10
>>>>> +enum {
>>>>> + IORING_REGISTER_BUFFERS,
>>>>> + IORING_UNREGISTER_BUFFERS,
>>>>> + IORING_REGISTER_FILES,
>>>>> + IORING_UNREGISTER_FILES,
>>>>> + IORING_REGISTER_EVENTFD,
>>>>> + IORING_UNREGISTER_EVENTFD,
>>>>> + IORING_REGISTER_FILES_UPDATE,
>>>>> + IORING_REGISTER_EVENTFD_ASYNC,
>>>>> + IORING_REGISTER_PROBE,
>>>>> + IORING_REGISTER_PERSONALITY,
>>>>> + IORING_UNREGISTER_PERSONALITY,
>>>>> +
>>>>> + /* this goes last */
>>>>> + IORING_REGISTER_LAST
>>>>> +};
>>>>
>>>> It breaks userspace API. E.g.
>>>>
>>>> #ifdef IORING_REGISTER_BUFFERS
>>>
>>> It can, yes, but we have done that in the past. In this one, for
>>
>> Ok, if nobody on the userspace side cares, then better to do that
>> sooner than later.

I actually don't think it's a huge issue. Normally if applications
do this, it's because they are using it and need it. Ala:

#ifndef IORING_REGISTER_SOMETHING
#define IORING_REGISTER_SOMETHING fooval
#endif

and that'll still work just fine, even if an identical enum is there.

--
Jens Axboe

2020-07-17 08:16:21

by Stefano Garzarella

Subject: Re: [PATCH RFC v2 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes

On Thu, Jul 16, 2020 at 03:20:53PM -0600, Jens Axboe wrote:
> On 7/16/20 2:51 PM, Jens Axboe wrote:
> > On 7/16/20 2:47 PM, Pavel Begunkov wrote:
> >> On 16/07/2020 23:42, Jens Axboe wrote:
> >>> On 7/16/20 2:16 PM, Pavel Begunkov wrote:
> >>>> On 16/07/2020 15:48, Stefano Garzarella wrote:
> >>>>> The enumeration allows us to keep track of the last
> >>>>> io_uring_register(2) opcode available.
> >>>>>
> >>>>> Behaviour and opcodes names don't change.
> >>>>>
> >>>>> Signed-off-by: Stefano Garzarella <[email protected]>
> >>>>> ---
> >>>>> include/uapi/linux/io_uring.h | 27 ++++++++++++++++-----------
> >>>>> 1 file changed, 16 insertions(+), 11 deletions(-)
> >>>>>
> >>>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> >>>>> index 7843742b8b74..efc50bd0af34 100644
> >>>>> --- a/include/uapi/linux/io_uring.h
> >>>>> +++ b/include/uapi/linux/io_uring.h
> >>>>> @@ -253,17 +253,22 @@ struct io_uring_params {
> >>>>> /*
> >>>>> * io_uring_register(2) opcodes and arguments
> >>>>> */
> >>>>> -#define IORING_REGISTER_BUFFERS 0
> >>>>> -#define IORING_UNREGISTER_BUFFERS 1
> >>>>> -#define IORING_REGISTER_FILES 2
> >>>>> -#define IORING_UNREGISTER_FILES 3
> >>>>> -#define IORING_REGISTER_EVENTFD 4
> >>>>> -#define IORING_UNREGISTER_EVENTFD 5
> >>>>> -#define IORING_REGISTER_FILES_UPDATE 6
> >>>>> -#define IORING_REGISTER_EVENTFD_ASYNC 7
> >>>>> -#define IORING_REGISTER_PROBE 8
> >>>>> -#define IORING_REGISTER_PERSONALITY 9
> >>>>> -#define IORING_UNREGISTER_PERSONALITY 10
> >>>>> +enum {
> >>>>> + IORING_REGISTER_BUFFERS,
> >>>>> + IORING_UNREGISTER_BUFFERS,
> >>>>> + IORING_REGISTER_FILES,
> >>>>> + IORING_UNREGISTER_FILES,
> >>>>> + IORING_REGISTER_EVENTFD,
> >>>>> + IORING_UNREGISTER_EVENTFD,
> >>>>> + IORING_REGISTER_FILES_UPDATE,
> >>>>> + IORING_REGISTER_EVENTFD_ASYNC,
> >>>>> + IORING_REGISTER_PROBE,
> >>>>> + IORING_REGISTER_PERSONALITY,
> >>>>> + IORING_UNREGISTER_PERSONALITY,
> >>>>> +
> >>>>> + /* this goes last */
> >>>>> + IORING_REGISTER_LAST
> >>>>> +};
> >>>>
> >>>> It breaks userspace API. E.g.
> >>>>
> >>>> #ifdef IORING_REGISTER_BUFFERS
> >>>
> >>> It can, yes, but we have done that in the past. In this one, for
> >>
> >> Ok, if nobody on the userspace side cares, then better to do that
> >> sooner than later.
>
> I actually don't think it's a huge issue. Normally if applications
> do this, it's because they are using it and need it. Ala:
>
> #ifndef IORING_REGISTER_SOMETHING
> #define IORING_REGISTER_SOMETHING fooval
> #endif
>
> and that'll still work just fine, even if an identical enum is there.
>

Thank you both for the review!

Then if you agree, I'll leave this patch as it is by introducing the enum.

Stefano