From: Ammar Faizi <[email protected]>
Hi Jens,
io_uring uapi updates:
1) Don't force linux/time_types.h for userspace. Linux's io_uring.h is
synced 1:1 into liburing's io_uring.h. liburing has a configure
check to detect the need for linux/time_types.h (Stefan).
2) Do not use a zero-size array because it doesn't allow the user to
compile an app that uses liburing with the `-pedantic-errors` flag:
io_uring.h:611:28: error: zero size arrays are an extension [-Werror,-Wzero-length-array]
Replace the array size from 0 to 1 (me).
Signed-off-by: Ammar Faizi <[email protected]>
---
Ammar Faizi (1):
io_uring: uapi: Don't use a zero-size array
Stefan Metzmacher (1):
io_uring: uapi: Don't force linux/time_types.h for userspace
include/uapi/linux/io_uring.h | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
base-commit: 5576035f15dfcc6cb1cec236db40c2c0733b0ba4
--
Ammar Faizi
From: Ammar Faizi <[email protected]>
Don't use a zero-size array because it doesn't allow the user to
compile an app that uses liburing with the `-pedantic-errors` flag:
io_uring.h:611:28: error: zero size arrays are an extension [-Werror,-Wzero-length-array]
Replace the array size from 0 to 1.
- No functional change is intended.
- No struct/union size change.
Signed-off-by: Ammar Faizi <[email protected]>
---
include/uapi/linux/io_uring.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 77027cbaf786..0890784fcc9e 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -85,25 +85,25 @@ struct io_uring_sqe {
__u16 __pad3[1];
};
};
union {
struct {
__u64 addr3;
__u64 __pad2[1];
};
/*
* If the ring is initialized with IORING_SETUP_SQE128, then
* this field is used for 80 bytes of arbitrary command data
*/
- __u8 cmd[0];
+ __u8 cmd[1];
};
};
/*
* If sqe->file_index is set to this for opcodes that instantiate a new
* direct descriptor (like openat/openat2/accept), then io_uring will allocate
* an available direct descriptor instead of having the application pass one
* in. The picked direct descriptor will be returned in cqe->res, or -ENFILE
* if the space is full.
*/
#define IORING_FILE_INDEX_ALLOC (~0U)
@@ -599,25 +599,25 @@ struct io_uring_buf {
struct io_uring_buf_ring {
union {
/*
* To avoid spilling into more pages than we need to, the
* ring tail is overlaid with the io_uring_buf->resv field.
*/
struct {
__u64 resv1;
__u32 resv2;
__u16 resv3;
__u16 tail;
};
- struct io_uring_buf bufs[0];
+ struct io_uring_buf bufs[1];
};
};
/* argument for IORING_(UN)REGISTER_PBUF_RING */
struct io_uring_buf_reg {
__u64 ring_addr;
__u32 ring_entries;
__u16 bgid;
__u16 pad;
__u64 resv[3];
};
--
Ammar Faizi
On 11/15/22 2:29 PM, Ammar Faizi wrote:
> From: Ammar Faizi <[email protected]>
>
> Don't use a zero-size array because it doesn't allow the user to
> compile an app that uses liburing with the `-pedantic-errors` flag:
>
> io_uring.h:611:28: error: zero size arrays are an extension [-Werror,-Wzero-length-array]
>
> Replace the array size from 0 to 1.
>
> - No functional change is intended.
> - No struct/union size change.
The only reason why they don't grow the struct, is because it's in
a union. I don't like this patch, as the zero sized array is a clear
sign that this struct has data past it. If it's a single entry, that's
very different.
Yes that apparently makes pendantic errors unhappy, but I care more
about the readability of it.
--
Jens Axboe
On Wed, 16 Nov 2022 04:29:51 +0700, Ammar Faizi wrote:
> From: Ammar Faizi <[email protected]>
>
> Hi Jens,
>
> io_uring uapi updates:
>
> 1) Don't force linux/time_types.h for userspace. Linux's io_uring.h is
> synced 1:1 into liburing's io_uring.h. liburing has a configure
> check to detect the need for linux/time_types.h (Stefan).
>
> [...]
Applied, thanks!
[1/2] io_uring: uapi: Don't force linux/time_types.h for userspace
commit: 958bfdd734b6074ba88ee3abc69d0053e26b7b9c
Best regards,
--
Jens Axboe
On 11/16/22 6:14 AM, Jens Axboe wrote:
> On Wed, 16 Nov 2022 04:29:51 +0700, Ammar Faizi wrote:
>> From: Ammar Faizi <[email protected]>
>>
>> Hi Jens,
>>
>> io_uring uapi updates:
>>
>> 1) Don't force linux/time_types.h for userspace. Linux's io_uring.h is
>> synced 1:1 into liburing's io_uring.h. liburing has a configure
>> check to detect the need for linux/time_types.h (Stefan).
>>
>> [...]
>
> Applied, thanks!
>
> [1/2] io_uring: uapi: Don't force linux/time_types.h for userspace
> commit: 958bfdd734b6074ba88ee3abc69d0053e26b7b9c
Jens, please drop this commit. It breaks the build:
All errors (new ones prefixed by >>):
In file included from <command-line>:
>> ./usr/include/linux/io_uring.h:654:41: error: field 'timeout' has incomplete type
654 | struct __kernel_timespec timeout;
| ^~~~~~~
--
Ammar Faizi
On 11/16/22 1:34 PM, Ammar Faizi wrote:
> On 11/16/22 6:14 AM, Jens Axboe wrote:
>> On Wed, 16 Nov 2022 04:29:51 +0700, Ammar Faizi wrote:
>>> From: Ammar Faizi <[email protected]>
>>>
>>> Hi Jens,
>>>
>>> io_uring uapi updates:
>>>
>>> 1) Don't force linux/time_types.h for userspace. Linux's io_uring.h is
>>> synced 1:1 into liburing's io_uring.h. liburing has a configure
>>> check to detect the need for linux/time_types.h (Stefan).
>>>
>>> [...]
>>
>> Applied, thanks!
>>
>> [1/2] io_uring: uapi: Don't force linux/time_types.h for userspace
>> commit: 958bfdd734b6074ba88ee3abc69d0053e26b7b9c
>
> Jens, please drop this commit. It breaks the build:
>
> All errors (new ones prefixed by >>):
>
> In file included from <command-line>:
>>> ./usr/include/linux/io_uring.h:654:41: error: field 'timeout' has incomplete type
> 654 | struct __kernel_timespec timeout;
> | ^~~~~~~
https://lore.kernel.org/r/[email protected]
--
Ammar Faizi
On 11/15/22 21:29, Ammar Faizi wrote:
> From: Ammar Faizi <[email protected]>
>
> Don't use a zero-size array because it doesn't allow the user to
> compile an app that uses liburing with the `-pedantic-errors` flag:
Ammar, I'd strongly encourage you to at least compile your
patches or even better actually test them. There is an explicit
BUILD_BUG_ON() violated by this change.
--
Pavel Begunkov
On 11/16/22 5:14 PM, Pavel Begunkov wrote:
> On 11/15/22 21:29, Ammar Faizi wrote:
>> From: Ammar Faizi <[email protected]>
>>
>> Don't use a zero-size array because it doesn't allow the user to
>> compile an app that uses liburing with the `-pedantic-errors` flag:
>
> Ammar, I'd strongly encourage you to at least compile your
> patches or even better actually test them. There is an explicit
> BUILD_BUG_ON() violated by this change.
Oh yeah, I didn't realize that. This patch breaks this assertion:
BUILD_BUG_ON failed: sizeof_field(struct io_uring_sqe, cmd) != 0
This assertion wants the size of cmd[] to be zero. Which is obviously
violated in this patch.
I only tested a liburing app that uses this header and validated
that the struct size is the same, but not its field. That's my
mistake.
I'm *not* going to send a v2 per Jens' comment.
--
Ammar Faizi
On 11/15/22 11:34 PM, Ammar Faizi wrote:
> On 11/16/22 6:14 AM, Jens Axboe wrote:
>> On Wed, 16 Nov 2022 04:29:51 +0700, Ammar Faizi wrote:
>>> From: Ammar Faizi <[email protected]>
>>>
>>> Hi Jens,
>>>
>>> io_uring uapi updates:
>>>
>>> 1) Don't force linux/time_types.h for userspace. Linux's io_uring.h is
>>> synced 1:1 into liburing's io_uring.h. liburing has a configure
>>> check to detect the need for linux/time_types.h (Stefan).
>>>
>>> [...]
>>
>> Applied, thanks!
>>
>> [1/2] io_uring: uapi: Don't force linux/time_types.h for userspace
>> commit: 958bfdd734b6074ba88ee3abc69d0053e26b7b9c
>
> Jens, please drop this commit. It breaks the build:
Dropped - please actually build your patches, or make it clear that
they were not built at all. None of these 2 patches were any good.
--
Jens Axboe
Am 16.11.22 um 14:50 schrieb Jens Axboe:
> On 11/15/22 11:34 PM, Ammar Faizi wrote:
>> On 11/16/22 6:14 AM, Jens Axboe wrote:
>>> On Wed, 16 Nov 2022 04:29:51 +0700, Ammar Faizi wrote:
>>>> From: Ammar Faizi <[email protected]>
>>>>
>>>> Hi Jens,
>>>>
>>>> io_uring uapi updates:
>>>>
>>>> 1) Don't force linux/time_types.h for userspace. Linux's io_uring.h is
>>>> synced 1:1 into liburing's io_uring.h. liburing has a configure
>>>> check to detect the need for linux/time_types.h (Stefan).
>>>>
>>>> [...]
>>>
>>> Applied, thanks!
>>>
>>> [1/2] io_uring: uapi: Don't force linux/time_types.h for userspace
>>> commit: 958bfdd734b6074ba88ee3abc69d0053e26b7b9c
>>
>> Jens, please drop this commit. It breaks the build:
>
> Dropped - please actually build your patches, or make it clear that
> they were not built at all. None of these 2 patches were any good.
Is it tools/testing/selftests/net/io_uring_zerocopy_tx.c that doesn't build?
and needs a '#define HAVE_LINUX_TIME_TYPES_H 1'
BTW, the original commit I posted was here:
https://lore.kernel.org/io-uring/c7782923deeb4016f2ac2334bc558921e8d91a67.1666605446.git.metze@samba.org/
What's the magic to compile tools/testing/selftests/net/io_uring_zerocopy_tx.c ?
My naive tries both fail (even without my patch):
All other files including any io_uring.h build and the patch was also included
in a branch where I build a complete working kernel with 'make -j33 bindeb-pkg'
metze@SERNOX19:~/devel/kernel/linux-4.4$ LANG=C make $(find io_uring/*.c fs/file_table.c fs/exec.c kernel/exit.c kernel/fork.c net/socket.c net/unix/scm.c
security/selinux/hooks.c security/smack/smack_lsm.c tools/testing/selftests/net/io_uring_zerocopy_tx.c | sed -e 's!\.c$!.o!') EXTRA_CFLAGS="-Wfatal-errors"
UPD include/config/kernel.release
UPD include/generated/utsrelease.h
CALL scripts/checksyscalls.sh
DESCEND objtool
DESCEND bpf/resolve_btfids
CC fs/file_table.o
CC fs/exec.o
CC io_uring/advise.o
CC io_uring/cancel.o
CC io_uring/epoll.o
CC io_uring/fdinfo.o
CC io_uring/filetable.o
CC io_uring/fs.o
CC io_uring/io_uring.o
CC io_uring/io-wq.o
CC io_uring/kbuf.o
CC io_uring/msg_ring.o
CC io_uring/net.o
CC io_uring/nop.o
CC io_uring/notif.o
CC io_uring/opdef.o
CC io_uring/openclose.o
CC io_uring/poll.o
CC io_uring/rsrc.o
CC io_uring/rw.o
CC io_uring/splice.o
CC io_uring/sqpoll.o
CC io_uring/statx.o
CC io_uring/sync.o
CC io_uring/tctx.o
CC io_uring/timeout.o
CC io_uring/uring_cmd.o
CC io_uring/xattr.o
CC kernel/exit.o
CC kernel/fork.o
CC net/socket.o
CC net/unix/scm.o
CC security/selinux/hooks.o
CC security/smack/smack_lsm.o
CC tools/testing/selftests/net/io_uring_zerocopy_tx.o
tools/testing/selftests/net/io_uring_zerocopy_tx.c:3:10: fatal error: assert.h: No such file or directory
3 | #include <assert.h>
| ^~~~~~~~~~
compilation terminated.
make[1]: *** [scripts/Makefile.build:258: tools/testing/selftests/net/io_uring_zerocopy_tx.o] Error 1
make: *** [Makefile:1997: .] Error 2
metze@SERNOX19:~/devel/kernel/linux-4.4/tools/testing/selftests/net$ make io_uring_zerocopy_tx.o
gcc -Wall -Wl,--no-as-needed -O2 -g -I../../../../usr/include/ -isystem /home/metze/devel/kernel/linux-4.4/tools/testing/selftests/../../../usr/include -c -o
io_uring_zerocopy_tx.o io_uring_zerocopy_tx.c
io_uring_zerocopy_tx.c: In function ‘io_uring_prep_sendzc’:
io_uring_zerocopy_tx.c:287:30: error: ‘IORING_OP_SEND_ZC’ undeclared (first use in this function); did you mean ‘IORING_OP_SEND’?
287 | sqe->opcode = (__u8) IORING_OP_SEND_ZC;
| ^~~~~~~~~~~~~~~~~
| IORING_OP_SEND
io_uring_zerocopy_tx.c:287:30: note: each undeclared identifier is reported only once for each function it appears in
io_uring_zerocopy_tx.c: In function ‘do_tx’:
io_uring_zerocopy_tx.c:407:56: error: ‘IORING_RECVSEND_FIXED_BUF’ undeclared (first use in this function)
407 | sqe->ioprio |= IORING_RECVSEND_FIXED_BUF;
| ^~~~~~~~~~~~~~~~~~~~~~~~~
io_uring_zerocopy_tx.c:429:42: error: ‘IORING_CQE_F_NOTIF’ undeclared (first use in this function); did you mean ‘IORING_CQE_F_MORE’?
429 | if (cqe->flags & IORING_CQE_F_NOTIF) {
| ^~~~~~~~~~~~~~~~~~
| IORING_CQE_F_MORE
make: *** [<eingebaut>: io_uring_zerocopy_tx.o] Fehler 1
On 11/16/22 7:22 AM, Stefan Metzmacher wrote:
> Am 16.11.22 um 14:50 schrieb Jens Axboe:
>> On 11/15/22 11:34 PM, Ammar Faizi wrote:
>>> On 11/16/22 6:14 AM, Jens Axboe wrote:
>>>> On Wed, 16 Nov 2022 04:29:51 +0700, Ammar Faizi wrote:
>>>>> From: Ammar Faizi <[email protected]>
>>>>>
>>>>> Hi Jens,
>>>>>
>>>>> io_uring uapi updates:
>>>>>
>>>>> 1) Don't force linux/time_types.h for userspace. Linux's io_uring.h is
>>>>> ???? synced 1:1 into liburing's io_uring.h. liburing has a configure
>>>>> ???? check to detect the need for linux/time_types.h (Stefan).
>>>>>
>>>>> [...]
>>>>
>>>> Applied, thanks!
>>>>
>>>> [1/2] io_uring: uapi: Don't force linux/time_types.h for userspace
>>>> ??????? commit: 958bfdd734b6074ba88ee3abc69d0053e26b7b9c
>>>
>>> Jens, please drop this commit. It breaks the build:
>>
>> Dropped - please actually build your patches, or make it clear that
>> they were not built at all. None of these 2 patches were any good.
>
> Is it tools/testing/selftests/net/io_uring_zerocopy_tx.c that doesn't build?
Honestly not sure, but saw a few reports come in. Here's the one from
linux-next:
https://lore.kernel.org/all/[email protected]/
> and needs a '#define HAVE_LINUX_TIME_TYPES_H 1'
>
> BTW, the original commit I posted was here:
> https://lore.kernel.org/io-uring/c7782923deeb4016f2ac2334bc558921e8d91a67.1666605446.git.metze@samba.org/
>
> What's the magic to compile tools/testing/selftests/net/io_uring_zerocopy_tx.c ?
Some variant of make kselftests-foo?
> My naive tries both fail (even without my patch):
Mine does too, in various other tests. Stephen?
--
Jens Axboe
Am 16.11.22 um 20:46 schrieb Jens Axboe:
> On 11/16/22 7:22 AM, Stefan Metzmacher wrote:
>> Am 16.11.22 um 14:50 schrieb Jens Axboe:
>>> On 11/15/22 11:34 PM, Ammar Faizi wrote:
>>>> On 11/16/22 6:14 AM, Jens Axboe wrote:
>>>>> On Wed, 16 Nov 2022 04:29:51 +0700, Ammar Faizi wrote:
>>>>>> From: Ammar Faizi <[email protected]>
>>>>>>
>>>>>> Hi Jens,
>>>>>>
>>>>>> io_uring uapi updates:
>>>>>>
>>>>>> 1) Don't force linux/time_types.h for userspace. Linux's io_uring.h is
>>>>>> ???? synced 1:1 into liburing's io_uring.h. liburing has a configure
>>>>>> ???? check to detect the need for linux/time_types.h (Stefan).
>>>>>>
>>>>>> [...]
>>>>>
>>>>> Applied, thanks!
>>>>>
>>>>> [1/2] io_uring: uapi: Don't force linux/time_types.h for userspace
>>>>> ??????? commit: 958bfdd734b6074ba88ee3abc69d0053e26b7b9c
>>>>
>>>> Jens, please drop this commit. It breaks the build:
>>>
>>> Dropped - please actually build your patches, or make it clear that
>>> they were not built at all. None of these 2 patches were any good.
>>
>> Is it tools/testing/selftests/net/io_uring_zerocopy_tx.c that doesn't build?
>
> Honestly not sure, but saw a few reports come in. Here's the one from
> linux-next:
>
> https://lore.kernel.org/all/[email protected]/
Yes, but the output is pretty useless as it doesn't show what
.c file and what command is failing.
>> and needs a '#define HAVE_LINUX_TIME_TYPES_H 1'
Just guessing, but adding this into the commit has a chance to work...
--- a/tools/testing/selftests/net/io_uring_zerocopy_tx.c
+++ b/tools/testing/selftests/net/io_uring_zerocopy_tx.c
@@ -15,6 +15,7 @@
#include <arpa/inet.h>
#include <linux/errqueue.h>
#include <linux/if_packet.h>
+#define HAVE_LINUX_TIME_TYPES_H 1
#include <linux/io_uring.h>
#include <linux/ipv6.h>
#include <linux/socket.h>
>> BTW, the original commit I posted was here:
>> https://lore.kernel.org/io-uring/c7782923deeb4016f2ac2334bc558921e8d91a67.1666605446.git.metze@samba.org/
>>
>> What's the magic to compile tools/testing/selftests/net/io_uring_zerocopy_tx.c ?
>
> Some variant of make kselftests-foo?
>
>> My naive tries both fail (even without my patch):
>
> Mine does too, in various other tests. Stephen?
Pavel, as you created that file, do you remember how you build it?
metze
Hi Jens,
>> and needs a '#define HAVE_LINUX_TIME_TYPES_H 1'
>>
>> BTW, the original commit I posted was here:
>> https://lore.kernel.org/io-uring/c7782923deeb4016f2ac2334bc558921e8d91a67.1666605446.git.metze@samba.org/
I'll push a better version soon, it inverts the ifdef logic like this:
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -10,7 +10,15 @@
#include <linux/fs.h>
#include <linux/types.h>
+/*
+ * this file is shared with liburing and that has to autodetect
+ * if linux/time_types.h is available or not, it can
+ * define UAPI_LINUX_IO_URING_H_SKIP_LINUX_TIME_TYPES_H
+ * if linux/time_types.h is not available
+ */
+#ifndef UAPI_LINUX_IO_URING_H_SKIP_LINUX_TIME_TYPES_H
#include <linux/time_types.h>
+#endif
It also means that projects without liburing usage are not affected.
And developers only have to care if they want to build on legacy systems
without time_types.h
Once that's accepted into the kernel I'll adjust the logic in liburing
Does that sound like a plan?
metze
On 11/16/22 20:03, Stefan Metzmacher wrote:
> Am 16.11.22 um 20:46 schrieb Jens Axboe:
>> On 11/16/22 7:22 AM, Stefan Metzmacher wrote:
>>> Am 16.11.22 um 14:50 schrieb Jens Axboe:
>>>> On 11/15/22 11:34 PM, Ammar Faizi wrote:
>>>>> On 11/16/22 6:14 AM, Jens Axboe wrote:
>>>>>> On Wed, 16 Nov 2022 04:29:51 +0700, Ammar Faizi wrote:
>>>>>>> From: Ammar Faizi <[email protected]>
>>>>>>>
>>>>>>> Hi Jens,
>>>>>>>
>>>>>>> io_uring uapi updates:
>>>>>>>
>>>>>>> 1) Don't force linux/time_types.h for userspace. Linux's io_uring.h is
>>>>>>> ???? synced 1:1 into liburing's io_uring.h. liburing has a configure
>>>>>>> ???? check to detect the need for linux/time_types.h (Stefan).
>>>>>>>
>>>>>>> [...]
>>>>>>
>>>>>> Applied, thanks!
>>>>>>
>>>>>> [1/2] io_uring: uapi: Don't force linux/time_types.h for userspace
>>>>>> ??????? commit: 958bfdd734b6074ba88ee3abc69d0053e26b7b9c
>>>>>
>>>>> Jens, please drop this commit. It breaks the build:
>>>>
>>>> Dropped - please actually build your patches, or make it clear that
>>>> they were not built at all. None of these 2 patches were any good.
>>>
>>> Is it tools/testing/selftests/net/io_uring_zerocopy_tx.c that doesn't build?
>>
>> Honestly not sure, but saw a few reports come in. Here's the one from
>> linux-next:
>>
>> https://lore.kernel.org/all/[email protected]/
>
> Yes, but the output is pretty useless as it doesn't show what
> .c file and what command is failing.
>
>>> and needs a '#define HAVE_LINUX_TIME_TYPES_H 1'
>
> Just guessing, but adding this into the commit has a chance to work...
>
> --- a/tools/testing/selftests/net/io_uring_zerocopy_tx.c
> +++ b/tools/testing/selftests/net/io_uring_zerocopy_tx.c
> @@ -15,6 +15,7 @@
> #include <arpa/inet.h>
> #include <linux/errqueue.h>
> #include <linux/if_packet.h>
> +#define HAVE_LINUX_TIME_TYPES_H 1
> #include <linux/io_uring.h>
> #include <linux/ipv6.h>
> #include <linux/socket.h>
>
>>> BTW, the original commit I posted was here:
>>> https://lore.kernel.org/io-uring/c7782923deeb4016f2ac2334bc558921e8d91a67.1666605446.git.metze@samba.org/
>>>
>>> What's the magic to compile tools/testing/selftests/net/io_uring_zerocopy_tx.c ?
>>
>> Some variant of make kselftests-foo?
>>
>>> My naive tries both fail (even without my patch):
>>
>> Mine does too, in various other tests. Stephen?
>
> Pavel, as you created that file, do you remember how you build it?
make headers_install
make -C tools/testing/selftests/net/
IIRC, it uses system uapi headers and apparently yours don't have
IORING_CQE_F_NOTIF, etc. And I don't think it uses the right Makefile,
so -C executes it from the selftest/net folder.
--
Pavel Begunkov