Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1618307549-78149-1-git-send-email-yang.lee@linux.alibaba.com>
 <CAADnVQJmsipci_ou6OOFGC6O9z935jFw4+pe7YQvvh2=eCoarQ@mail.gmail.com>
 <BN7PR13MB24996213858443821CA7E400FD4F9@BN7PR13MB2499.namprd13.prod.outlook.com>
 <CAADnVQJJ03L5H11P0NYNC9kYy=YkQdWrbpytB2Jps8AuxgamFA@mail.gmail.com>
 <BN7PR13MB2499152182E86AD94A3B0E22FD4F9@BN7PR13MB2499.namprd13.prod.outlook.com>
 <CAADnVQKjVDLBCLMzXoAQhJ5a+rZ1EKqc3dyYqpeG9M2KzGREMA@mail.gmail.com> <BN7PR13MB2499EF2A7B6F043FE4E62D51FD4F9@BN7PR13MB2499.namprd13.prod.outlook.com>
In-Reply-To: <BN7PR13MB2499EF2A7B6F043FE4E62D51FD4F9@BN7PR13MB2499.namprd13.prod.outlook.com>
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date:   Tue, 13 Apr 2021 09:56:57 -0700
Message-ID: <CAADnVQJ1+EmGD3CAc4N8Jq7ACYZvbzkxZrhz8hVf4dJHGamoXw@mail.gmail.com>
Subject: Re: [PATCH] selftests/bpf: use !E instead of comparing with NULL
To:     "Bird, Tim" <Tim.Bird@sony.com>
Cc:     Yang Li <yang.lee@linux.alibaba.com>,
        Shuah Khan <shuah@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        Network Development <netdev@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Apr 13, 2021 at 9:32 AM <Tim.Bird@sony.com> wrote:
>
> > -----Original Message-----
> > From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> >
> > On Tue, Apr 13, 2021 at 9:19 AM <Tim.Bird@sony.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > > >
> > > > On Tue, Apr 13, 2021 at 9:10 AM <Tim.Bird@sony.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > > > > >
> > > > > > On Tue, Apr 13, 2021 at 2:52 AM Yang Li <yang.lee@linux.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@linux.alibaba.com>
> > > > > > > Signed-off-by: Yang Li <yang.lee@linux.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.