Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <5791ec06-7174-9ae5-4fe4-6969ed110165@tessares.net>
 <3065880.1687785614@warthog.procyon.org.uk> <3067876.1687787456@warthog.procyon.org.uk>
 <2cb3b411-9010-a44b-ebee-1914e7fd7b9c@tessares.net>
In-Reply-To: <2cb3b411-9010-a44b-ebee-1914e7fd7b9c@tessares.net>
From:   Namhyung Kim <namhyung@kernel.org>
Date:   Mon, 26 Jun 2023 14:31:39 -0700
Message-ID: <CAM9d7ch_mWUQGW8G221bZmCPn3wB2mjZm=ZdmhDkczhich9xZA@mail.gmail.com>
Subject: Re: [PATCH net-next] tools: Fix MSG_SPLICE_PAGES build error in trace tools
To:     Matthieu Baerts <matthieu.baerts@tessares.net>
Cc:     David Howells <dhowells@redhat.com>, netdev@vger.kernel.org,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        David Miller <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, Jens Axboe <axboe@kernel.dk>,
        Matthew Wilcox <willy@infradead.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>, Ian Rogers <irogers@google.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        linux-perf-users@vger.kernel.org, bpf@vger.kernel.org,
        linux-next@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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.