Message-ID: <CAADnVQJ1+EmGD3CAc4N8Jq7ACYZvbzkxZrhz8hVf4dJHGamoXw@mail.gmail.com>
Date:   Tue, 13 Apr 2021 09:56:57 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     "Bird, Tim" <Tim.Bird@...y.com>
Cc:     Yang Li <yang.lee@...ux.alibaba.com>,
        Shuah Khan <shuah@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] selftests/bpf: use !E instead of comparing with NULL

On Tue, Apr 13, 2021 at 9:32 AM <Tim.Bird@...y.com> wrote:
>
> > -----Original Message-----
> > From: Alexei Starovoitov <alexei.starovoitov@...il.com>
> >
> > On Tue, Apr 13, 2021 at 9:19 AM <Tim.Bird@...y.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Alexei Starovoitov <alexei.starovoitov@...il.com>
> > > >
> > > > On Tue, Apr 13, 2021 at 9:10 AM <Tim.Bird@...y.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Alexei Starovoitov <alexei.starovoitov@...il.com>
> > > > > >
> > > > > > On Tue, Apr 13, 2021 at 2:52 AM Yang Li <yang.lee@...ux.alibaba.com> wrote:
> > > > > > >
> > > > > > > Fix the following coccicheck warnings:
> > > > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:189:7-11: WARNING
> > > > > > > comparing pointer to 0, suggest !E
> > > > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:361:7-11: WARNING
> > > > > > > comparing pointer to 0, suggest !E
> > > > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:386:14-18: WARNING
> > > > > > > comparing pointer to 0, suggest !E
> > > > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:402:14-18: WARNING
> > > > > > > comparing pointer to 0, suggest !E
> > > > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:433:7-11: WARNING
> > > > > > > comparing pointer to 0, suggest !E
> > > > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:534:14-18: WARNING
> > > > > > > comparing pointer to 0, suggest !E
> > > > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:625:7-11: WARNING
> > > > > > > comparing pointer to 0, suggest !E
> > > > > > > ./tools/testing/selftests/bpf/progs/profiler.inc.h:767:7-11: WARNING
> > > > > > > comparing pointer to 0, suggest !E
> > > > > > >
> > > > > > > Reported-by: Abaci Robot <abaci@...ux.alibaba.com>
> > > > > > > Signed-off-by: Yang Li <yang.lee@...ux.alibaba.com>
> > > > > > > ---
> > > > > > >  tools/testing/selftests/bpf/progs/profiler.inc.h | 22 +++++++++++-----------
> > > > > > >  1 file changed, 11 insertions(+), 11 deletions(-)
> > > > > > >
> > > > > > > diff --git a/tools/testing/selftests/bpf/progs/profiler.inc.h b/tools/testing/selftests/bpf/progs/profiler.inc.h
> > > > > > > index 4896fdf8..a33066c 100644
> > > > > > > --- a/tools/testing/selftests/bpf/progs/profiler.inc.h
> > > > > > > +++ b/tools/testing/selftests/bpf/progs/profiler.inc.h
> > > > > > > @@ -189,7 +189,7 @@ static INLINE void populate_ancestors(struct task_struct* task,
> > > > > > >  #endif
> > > > > > >         for (num_ancestors = 0; num_ancestors < MAX_ANCESTORS; num_ancestors++) {
> > > > > > >                 parent = BPF_CORE_READ(parent, real_parent);
> > > > > > > -               if (parent == NULL)
> > > > > > > +               if (!parent)
> > > > > >
> > > > > > Sorry, but I'd like the progs to stay as close as possible to the way
> > > > > > they were written.
> > > > > Why?
> > > > >
> > > > > > They might not adhere to kernel coding style in some cases.
> > > > > > The code could be grossly inefficient and even buggy.
> > > > > There would have to be a really good reason to accept
> > > > > grossly inefficient and even buggy code into the kernel.
> > > > >
> > > > > Can you please explain what that reason is?
> > > >
> > > > It's not the kernel. It's a test of bpf program.
> > > That doesn't answer the question of why you don't want any changes.
> > >
> > > Why would we not use kernel coding style guidelines and quality thresholds for
> > > testing code?  This *is* going into the kernel source tree, where it will be
> > > maintained and used by other developers.
> >
> > because the way the C code is written makes llvm generate a particular
> > code pattern that may not be seen otherwise.
> > Like removing 'if' because it's useless to humans, but not to the compiler
> > will change generated code which may or may not trigger the behavior
> > the prog intends to cover.
> > In particular this profiler.inc.h test is compiled three different ways to
> > maximize code generation differences.
> > It may not be checking error paths in some cases which can be considered
> > a bug, but that's the intended behavior of the C code as it was written.
> > So it has nothing to do with "quality of kernel code".
> > and it should not be used by developers. It's neither sample nor example.
>
> Ok - in this case it looks like a program, but it is essentially test data (for testing
> the compiler).  Thanks for the explanation.

yes. That's a good way of saying it.
Of course not all tests are like this.
Majority of bpf progs in selftests/bpf/progs/ are carefully written,
short and designed
as a unit test. While few are "test data" for llvm.

Powered by blists - more mailing lists