| Message ID | 3065880.1687785614@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] tools: Fix MSG_SPLICE_PAGES build error in trace tools | expand |
Hi David, Thank you for the patch! On 26/06/2023 15:20, David Howells wrote: > The following error is being seen the perf tools because they have their > own copies of a lot of kernel headers: > > In file included from builtin-trace.c:907: > trace/beauty/msg_flags.c: In function 'syscall_arg__scnprintf_msg_flags': > trace/beauty/msg_flags.c:28:21: error: 'MSG_SPLICE_PAGES' undeclared (first use in this function) > 28 | if (flags & MSG_##n) { \ > | ^~~~ > trace/beauty/msg_flags.c:50:9: note: in expansion of macro 'P_MSG_FLAG' > 50 | P_MSG_FLAG(SPLICE_PAGES); > | ^~~~~~~~~~ > trace/beauty/msg_flags.c:28:21: note: each undeclared identifier is reported only once for each function it appears in > 28 | if (flags & MSG_##n) { \ > | ^~~~ > trace/beauty/msg_flags.c:50:9: note: in expansion of macro 'P_MSG_FLAG' > 50 | P_MSG_FLAG(SPLICE_PAGES); > | ^~~~~~~~~~ > > Fix this by (1) adding MSG_SPLICE_PAGES to > tools/perf/trace/beauty/include/linux/socket.h - which looks like it ought > to work, but doesn't Commit 58277f502f42 ("perf trace beauty: Add script to autogenerate socket families table") seems to suggest this file is only used by tools/perf/trace/beauty/socket.sh script (and later by sockaddr.sh) but not by msg_flags.c. If I'm not mistaken, it is then not required to modify this file to fix the issue you mention. But it is still needed to modify it to avoid another warning: Warning: Kernel ABI header at 'tools/perf/trace/beauty/include/linux/socket.h' differs from latest version at 'include/linux/socket.h' diff -u tools/perf/trace/beauty/include/linux/socket.h include/linux/socket.h So another issue. When checking the differences between the two files, I see there are still also other modifications to import, e.g. it looks like you also added MSG_INTERNAL_SENDMSG_FLAGS macro in socket.h. Do you plan to fix that too? It might be clearer to have all these modifications of tools/perf/trace/beauty/include/linux/socket.h in a separated commit as it is fixing a different issue but that's a detail. As long as we can have perf compiling without issues when using the net-next tree :) Cheers, Matt
Matthieu Baerts <matthieu.baerts@tessares.net> wrote: > So another issue. When checking the differences between the two files, I > see there are still also other modifications to import, e.g. it looks > like you also added MSG_INTERNAL_SENDMSG_FLAGS macro in socket.h. Do you > plan to fix that too? That's just a list of other flags that we need to prevent userspace passing in - it's not a flag in its own right. David
On 26/06/2023 15:50, David Howells wrote: > Matthieu Baerts <matthieu.baerts@tessares.net> wrote: > >> So another issue. When checking the differences between the two files, I >> see there are still also other modifications to import, e.g. it looks >> like you also added MSG_INTERNAL_SENDMSG_FLAGS macro in socket.h. Do you >> plan to fix that too? > > That's just a list of other flags that we need to prevent userspace passing > in - it's not a flag in its own right. I agree. This file is not even used by C files, only by scripts parsing it if I'm not mistaken. But if there are differences with include/linux/socket.h, the warning I mentioned will be visible when compiling Perf. Cheers, Matt
Hello, Sorry I missed the conversation and the original change. On Mon, Jun 26, 2023 at 6:56 AM Matthieu Baerts <matthieu.baerts@tessares.net> wrote: > > On 26/06/2023 15:50, David Howells wrote: > > Matthieu Baerts <matthieu.baerts@tessares.net> wrote: > > > >> So another issue. When checking the differences between the two files, I > >> see there are still also other modifications to import, e.g. it looks > >> like you also added MSG_INTERNAL_SENDMSG_FLAGS macro in socket.h. Do you > >> plan to fix that too? > > > > That's just a list of other flags that we need to prevent userspace passing > > in - it's not a flag in its own right. > > I agree. This file is not even used by C files, only by scripts parsing > it if I'm not mistaken. > > But if there are differences with include/linux/socket.h, the warning I > mentioned will be visible when compiling Perf. Right, we want to keep the headers files in the tools in sync with the kernel ones. And they are used to generate some tables. Full explanation from Arnaldo below. So I'm ok for the msg_flags.c changes, but please refrain from changing the header directly. We will update it later. With that, Acked-by: Namhyung Kim <namhyung@kernel.org> Also I wonder if the tool needs to keep the original flag (SENDPAGE_NOTLAST) for the older kernels. In Arnaldo's explanation: There used to be no copies, with tools/ code using kernel headers directly. From time to time tools/perf/ broke due to legitimate kernel hacking. At some point Linus complained about such direct usage. Then we adopted the current model. The way these headers are used in perf are not restricted to just including them to compile something. They are sometimes used in scripts that convert defines into string tables, etc, so some change may break one of these scripts, or new MSRs may use some different #define pattern, etc. E.g.: $ ls -1 tools/perf/trace/beauty/*.sh | head -5 tools/perf/trace/beauty/arch_errno_names.sh tools/perf/trace/beauty/drm_ioctl.sh tools/perf/trace/beauty/fadvise.sh tools/perf/trace/beauty/fsconfig.sh tools/perf/trace/beauty/fsmount.sh $ $ tools/perf/trace/beauty/fadvise.sh static const char *fadvise_advices[] = { [0] = "NORMAL", [1] = "RANDOM", [2] = "SEQUENTIAL", [3] = "WILLNEED", [4] = "DONTNEED", [5] = "NOREUSE", }; $ The tools/perf/check-headers.sh script, part of the tools/ build process, points out changes in the original files. So its important not to touch the copies in tools/ when doing changes in the original kernel headers, that will be done later, when check-headers.sh inform about the change to the perf tools hackers.
On Mon, 26 Jun 2023 14:31:39 -0700 Namhyung Kim wrote: > Right, we want to keep the headers files in the tools in sync with > the kernel ones. And they are used to generate some tables. > Full explanation from Arnaldo below. > > So I'm ok for the msg_flags.c changes, but please refrain from > changing the header directly. We will update it later. > > With that, > Acked-by: Namhyung Kim <namhyung@kernel.org> Ah, missed this email, sounds like this is preferred to Matthieu's fix, we'll take this one. > Also I wonder if the tool needs to keep the original flag > (SENDPAGE_NOTLAST) for the older kernels. That's a bit unclear, because it's just a kernel-internal flag. Future kernels may well use that bit for something else. Better long term solution would be to use an enum so that the values are included in debuginfo and perf can read them at runtime :(
On Mon, Jun 26, 2023 at 2:53 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 26 Jun 2023 14:31:39 -0700 Namhyung Kim wrote: > > Right, we want to keep the headers files in the tools in sync with > > the kernel ones. And they are used to generate some tables. > > Full explanation from Arnaldo below. > > > > So I'm ok for the msg_flags.c changes, but please refrain from > > changing the header directly. We will update it later. > > > > With that, > > Acked-by: Namhyung Kim <namhyung@kernel.org> > > Ah, missed this email, sounds like this is preferred to Matthieu's > fix, we'll take this one. Hmm.. I believe they are the same when it comes to the changes in the .c file. > > > Also I wonder if the tool needs to keep the original flag > > (SENDPAGE_NOTLAST) for the older kernels. > > That's a bit unclear, because it's just a kernel-internal flag. > Future kernels may well use that bit for something else. Ah, ok then. I thought it's a user-visible flag. > > Better long term solution would be to use an enum so that the values > are included in debuginfo and perf can read them at runtime :( Right, we also consider BTF to read the values and hopefully get rid of the business of copying kernel headers. Thanks, Namhyung
On Mon, 2023-06-26 at 14:59 -0700, Namhyung Kim wrote: > On Mon, Jun 26, 2023 at 2:53 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Mon, 26 Jun 2023 14:31:39 -0700 Namhyung Kim wrote: > > > Right, we want to keep the headers files in the tools in sync with > > > the kernel ones. And they are used to generate some tables. > > > Full explanation from Arnaldo below. > > > > > > So I'm ok for the msg_flags.c changes, but please refrain from > > > changing the header directly. We will update it later. > > > > > > With that, > > > Acked-by: Namhyung Kim <namhyung@kernel.org> > > > > Ah, missed this email, sounds like this is preferred to Matthieu's > > fix, we'll take this one. > > Hmm.. I believe they are the same when it comes to the > changes in the .c file. > > > > > > Also I wonder if the tool needs to keep the original flag > > > (SENDPAGE_NOTLAST) for the older kernels. > > > > That's a bit unclear, because it's just a kernel-internal flag. > > Future kernels may well use that bit for something else. > > Ah, ok then. I thought it's a user-visible flag. > > > > > Better long term solution would be to use an enum so that the values > > are included in debuginfo and perf can read them at runtime :( > > Right, we also consider BTF to read the values and hopefully > get rid of the business of copying kernel headers. I read all the above as the preferred solution is the one provided by Mat's patch (not touching socket.h, same changes as here in msg_flags.c. As such, I'll restore Mat's patch in PW and will obsolete this one. Please raise a flag if I'm wrong ;) Cheers, Paolo
diff --git a/tools/perf/trace/beauty/include/linux/socket.h b/tools/perf/trace/beauty/include/linux/socket.h index 3bef212a24d7..77cb707a566a 100644 --- a/tools/perf/trace/beauty/include/linux/socket.h +++ b/tools/perf/trace/beauty/include/linux/socket.h @@ -326,6 +326,7 @@ struct ucred { */ #define MSG_ZEROCOPY 0x4000000 /* Use user data in kernel path */ +#define MSG_SPLICE_PAGES 0x8000000 /* Splice the pages from the iterator in sendmsg() */ #define MSG_FASTOPEN 0x20000000 /* Send data in TCP SYN */ #define MSG_CMSG_CLOEXEC 0x40000000 /* Set close_on_exec for file descriptor received through diff --git a/tools/perf/trace/beauty/msg_flags.c b/tools/perf/trace/beauty/msg_flags.c index 5cdebd7ece7e..aa9934020232 100644 --- a/tools/perf/trace/beauty/msg_flags.c +++ b/tools/perf/trace/beauty/msg_flags.c @@ -8,6 +8,9 @@ #ifndef MSG_WAITFORONE #define MSG_WAITFORONE 0x10000 #endif +#ifndef MSG_SPLICE_PAGES +#define MSG_SPLICE_PAGES 0x8000000 +#endif #ifndef MSG_FASTOPEN #define MSG_FASTOPEN 0x20000000 #endif
The following error is being seen the perf tools because they have their own copies of a lot of kernel headers: In file included from builtin-trace.c:907: trace/beauty/msg_flags.c: In function 'syscall_arg__scnprintf_msg_flags': trace/beauty/msg_flags.c:28:21: error: 'MSG_SPLICE_PAGES' undeclared (first use in this function) 28 | if (flags & MSG_##n) { \ | ^~~~ trace/beauty/msg_flags.c:50:9: note: in expansion of macro 'P_MSG_FLAG' 50 | P_MSG_FLAG(SPLICE_PAGES); | ^~~~~~~~~~ trace/beauty/msg_flags.c:28:21: note: each undeclared identifier is reported only once for each function it appears in 28 | if (flags & MSG_##n) { \ | ^~~~ trace/beauty/msg_flags.c:50:9: note: in expansion of macro 'P_MSG_FLAG' 50 | P_MSG_FLAG(SPLICE_PAGES); | ^~~~~~~~~~ Fix this by (1) adding MSG_SPLICE_PAGES to tools/perf/trace/beauty/include/linux/socket.h - which looks like it ought to work, but doesn't, and (2) defining it conditionally in the file on which the error occurs (suggested by Matthieu Baerts - this is also done for some other flags). Fixes: b848b26c6672 ("net: Kill MSG_SENDPAGE_NOTLAST") Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> Link: https://lore.kernel.org/r/20230626112847.2ef3d422@canb.auug.org.au/ Signed-off-by: David Howells <dhowells@redhat.com> cc: Matthieu Baerts <matthieu.baerts@tessares.net> cc: Arnaldo Carvalho de Melo <acme@redhat.com> cc: "David S. Miller" <davem@davemloft.net> cc: Eric Dumazet <edumazet@google.com> cc: Jakub Kicinski <kuba@kernel.org> cc: Paolo Abeni <pabeni@redhat.com> cc: Jens Axboe <axboe@kernel.dk> cc: Matthew Wilcox <willy@infradead.org> cc: bpf@vger.kernel.org cc: dccp@vger.kernel.org cc: linux-crypto@vger.kernel.org cc: mptcp@lists.linux.dev cc: netdev@vger.kernel.org cc: tipc-discussion@lists.sourceforge.net cc: virtualization@lists.linux-foundation.org --- include/linux/socket.h | 1 + msg_flags.c | 3 +++ 2 files changed, 4 insertions(+)