Date:   Tue, 13 Apr 2021 17:22:27 +0000
From:   <Tim.Bird@...y.com>
To:     <alexei.starovoitov@...il.com>
CC:     <yang.lee@...ux.alibaba.com>, <shuah@...nel.org>, <ast@...nel.org>,
        <daniel@...earbox.net>, <andrii@...nel.org>, <kafai@...com>,
        <songliubraving@...com>, <yhs@...com>, <john.fastabend@...il.com>,
        <kpsingh@...nel.org>, <linux-kselftest@...r.kernel.org>,
        <netdev@...r.kernel.org>, <bpf@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] selftests/bpf: use !E instead of comparing with NULL

> -----Original Message-----
> From: Alexei Starovoitov <alexei.starovoitov@...il.com>
> 
> 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.

Thanks.  It might be useful to put a comment near the code,
to explain the nature of the code and let people know to avoid
"fixing" it.
 -- Tim

Powered by blists - more mailing lists