diff mbox series

[net-next] perf trace: fix MSG_SPLICE_PAGES build error

Message ID 20230626090239.899672-1-matthieu.baerts@tessares.net (mailing list archive)
State Accepted
Commit 2553a5270d6c281b8fc43bd34611197bff67d715
Delegated to: Netdev Maintainers
Headers show
Series [net-next] perf trace: fix MSG_SPLICE_PAGES build error | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers success CCed 14 of 14 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 9 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Matthieu Baerts June 26, 2023, 9:02 a.m. UTC 
Our MPTCP CI and Stephen got this error:

    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);
          |         ^~~~~~~~~~

The fix is similar to what was done with MSG_FASTOPEN: the new macro is
defined if it is not defined in the system headers.

Fixes: b848b26c6672 ("net: Kill MSG_SENDPAGE_NOTLAST")
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Closes: https://lore.kernel.org/r/20230626112847.2ef3d422@canb.auug.org.au/
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---

Notes:
    @David: I solved it like that in MPTCP tree. Does it work for you too?

    I guess tools/perf/trace/beauty/include/linux/socket.h file still needs
    to be updated, not just to add MSG_SPLICE_PAGES but also other
    modifications done in this file. Maybe best to sync with Arnaldo because
    he might do it soon during the coming merge window I guess.

Cc: David Howells <dhowells@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>

 tools/perf/trace/beauty/msg_flags.c | 3 +++
 1 file changed, 3 insertions(+)


base-commit: 9ae440b8fdd6772b6c007fa3d3766530a09c9045

Comments

Jakub Kicinski June 26, 2023, 9:27 p.m. UTC | #1 
On Mon, 26 Jun 2023 11:02:39 +0200 Matthieu Baerts wrote:
> Our MPTCP CI and Stephen got this error:
> 
>     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);
>           |         ^~~~~~~~~~
> 
> The fix is similar to what was done with MSG_FASTOPEN: the new macro is
> defined if it is not defined in the system headers.
> 
> Fixes: b848b26c6672 ("net: Kill MSG_SENDPAGE_NOTLAST")
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Closes: https://lore.kernel.org/r/20230626112847.2ef3d422@canb.auug.org.au/
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> 
> Notes:
>     @David: I solved it like that in MPTCP tree. Does it work for you too?
> 
>     I guess tools/perf/trace/beauty/include/linux/socket.h file still needs
>     to be updated, not just to add MSG_SPLICE_PAGES but also other
>     modifications done in this file. Maybe best to sync with Arnaldo because
>     he might do it soon during the coming merge window I guess.
> 
> Cc: David Howells <dhowells@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>

Hi Arnaldo, are you okay with us taking this into the networking tree?
Or do you prefer to sync the header after everything lands in Linus's
tree?
Namhyung Kim June 26, 2023, 9:41 p.m. UTC | #2 
Hello,

On Mon, Jun 26, 2023 at 2:27 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 26 Jun 2023 11:02:39 +0200 Matthieu Baerts wrote:
> > Our MPTCP CI and Stephen got this error:
> >
> >     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);
> >           |         ^~~~~~~~~~
> >
> > The fix is similar to what was done with MSG_FASTOPEN: the new macro is
> > defined if it is not defined in the system headers.
> >
> > Fixes: b848b26c6672 ("net: Kill MSG_SENDPAGE_NOTLAST")
> > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > Closes: https://lore.kernel.org/r/20230626112847.2ef3d422@canb.auug.org.au/
> > Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> > ---
> >
> > Notes:
> >     @David: I solved it like that in MPTCP tree. Does it work for you too?
> >
> >     I guess tools/perf/trace/beauty/include/linux/socket.h file still needs
> >     to be updated, not just to add MSG_SPLICE_PAGES but also other
> >     modifications done in this file. Maybe best to sync with Arnaldo because
> >     he might do it soon during the coming merge window I guess.
> >
> > Cc: David Howells <dhowells@redhat.com>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> Hi Arnaldo, are you okay with us taking this into the networking tree?
> Or do you prefer to sync the header after everything lands in Linus's
> tree?

Arnaldo is on vacation now, and I'm taking care of the patches
on behalf of him.

As it's introduced in the networking tree, it should be fine to
carry the fix together.  I'll sync the header later.

But in general you don't need to change the copy of the tools
headers together.  It also needs to support old & new kernels
so different care should be taken.  Please separate tooling
changes and let us handle them.

Thanks,
Namhyung
Jakub Kicinski June 26, 2023, 9:49 p.m. UTC | #3 
On Mon, 26 Jun 2023 14:41:56 -0700 Namhyung Kim wrote:
> > Hi Arnaldo, are you okay with us taking this into the networking tree?
> > Or do you prefer to sync the header after everything lands in Linus's
> > tree?  
> 
> Arnaldo is on vacation now, and I'm taking care of the patches
> on behalf of him.
> 
> As it's introduced in the networking tree, it should be fine to
> carry the fix together.  I'll sync the header later.

Will do, thanks!

> But in general you don't need to change the copy of the tools
> headers together.  It also needs to support old & new kernels
> so different care should be taken.  Please separate tooling
> changes and let us handle them.

Ack, I'm not sure what makes this a special case, from Stephen's
original report:

https://lore.kernel.org/all/20230626112847.2ef3d422@canb.auug.org.au/

it sounded like perf won't build without the fix.
Matthieu Baerts June 27, 2023, 3:31 a.m. UTC | #4 
Hi Jakub, Namhyung,

@Namhyung: thank you for the explanations about the header files in the other thread!


26 Jun 2023 23:49:36 Jakub Kicinski <kuba@kernel.org>:
> On Mon, 26 Jun 2023 14:41:56 -0700 Namhyung Kim wrote:
>
>> But in general you don't need to change the copy of the tools
>> headers together.  It also needs to support old & new kernels
>> so different care should be taken.  Please separate tooling
>> changes and let us handle them.
>
> Ack, I'm not sure what makes this a special case, from Stephen's
> original report:
>
> https://lore.kernel.org/all/20230626112847.2ef3d422@canb.auug.org.au/
> > it sounded like perf won't build without the fix.

The copy of the header file is confusing :)

If I understood properly, here, we need the modification in msg_flags.c from Perf code because this file has been recently modified by David's series in net-next to support a new flag (not present in the system header files yet). Without this patch, Perf doesn't build.

Now regarding the "internal" copy of header files: we don't need to modify it because it is just used by Perf maintainers to monitor the modifications in the original file. If I'm not mistaken, it might even be better to revert the line that has been removed in David's original series in net-next in this header file. But now that Namhyung is aware of that modification, I guess the revert is probably not needed. But in short, it is then better not to modify this header file in the networking tree :-)

Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
patchwork-bot+netdevbpf@kernel.org June 27, 2023, 2:10 p.m. UTC | #5 
Hello:

This patch was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Mon, 26 Jun 2023 11:02:39 +0200 you wrote:
> Our MPTCP CI and Stephen got this error:
> 
>     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);
>           |         ^~~~~~~~~~
> 
> [...]

Here is the summary with links:
  - [net-next] perf trace: fix MSG_SPLICE_PAGES build error
    https://git.kernel.org/netdev/net-next/c/2553a5270d6c

You are awesome, thank you!
diff mbox series

Patch

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