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 <[email protected]>
Link: https://lore.kernel.org/r/[email protected]/
Signed-off-by: David Howells <[email protected]>
cc: Matthieu Baerts <[email protected]>
cc: Arnaldo Carvalho de Melo <[email protected]>
cc: "David S. Miller" <[email protected]>
cc: Eric Dumazet <[email protected]>
cc: Jakub Kicinski <[email protected]>
cc: Paolo Abeni <[email protected]>
cc: Jens Axboe <[email protected]>
cc: Matthew Wilcox <[email protected]>
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
---
include/linux/socket.h | 1 +
msg_flags.c | 3 +++
2 files changed, 4 insertions(+)
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
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
--
Tessares | Belgium | Hybrid Access Solutions
http://www.tessares.net
On 26/06/2023 15:50, David Howells wrote:
> Matthieu Baerts <[email protected]> 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
--
Tessares | Belgium | Hybrid Access Solutions
http://www.tessares.net
Matthieu Baerts <[email protected]> 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
Hello,
Sorry I missed the conversation and the original change.
On Mon, Jun 26, 2023 at 6:56 AM Matthieu Baerts
<[email protected]> wrote:
>
> On 26/06/2023 15:50, David Howells wrote:
> > Matthieu Baerts <[email protected]> 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 <[email protected]>
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, Jun 26, 2023 at 2:53 PM Jakub Kicinski <[email protected]> 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 <[email protected]>
>
> 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, 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 <[email protected]>
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, 2023-06-26 at 14:59 -0700, Namhyung Kim wrote:
> On Mon, Jun 26, 2023 at 2:53 PM Jakub Kicinski <[email protected]> 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 <[email protected]>
> >
> > 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