Linux-kselftest Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] selftests/bpf: use !E instead of comparing with NULL
@ 2021-04-13  9:52 Yang Li
  2021-04-13 15:17 ` Alexei Starovoitov
  0 siblings, 1 reply; 9+ messages in thread
From: Yang Li @ 2021-04-13  9:52 UTC (permalink / raw)
  To: shuah
  Cc: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend,
	kpsingh, linux-kselftest, netdev, bpf, linux-kernel, Yang Li

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)
 			break;
 		ppid = BPF_CORE_READ(parent, tgid);
 		if (is_init_process(ppid))
@@ -361,7 +361,7 @@ static INLINE void* populate_var_metadata(struct var_metadata_t* metadata,
 	int zero = 0;
 	struct var_kill_data_t* kill_data = bpf_map_lookup_elem(&data_heap, &zero);
 
-	if (kill_data == NULL)
+	if (!kill_data)
 		return NULL;
 	struct task_struct* task = (struct task_struct*)bpf_get_current_task();
 
@@ -386,14 +386,14 @@ static INLINE int trace_var_sys_kill(void* ctx, int tpid, int sig)
 	u32 spid = get_userspace_pid();
 	struct var_kill_data_arr_t* arr_struct = bpf_map_lookup_elem(&var_tpid_to_data, &tpid);
 
-	if (arr_struct == NULL) {
+	if (!arr_struct) {
 		struct var_kill_data_t* kill_data = get_var_kill_data(ctx, spid, tpid, sig);
 		int zero = 0;
 
-		if (kill_data == NULL)
+		if (!kill_data)
 			return 0;
 		arr_struct = bpf_map_lookup_elem(&data_heap, &zero);
-		if (arr_struct == NULL)
+		if (!arr_struct)
 			return 0;
 		bpf_probe_read(&arr_struct->array[0], sizeof(arr_struct->array[0]), kill_data);
 	} else {
@@ -402,7 +402,7 @@ static INLINE int trace_var_sys_kill(void* ctx, int tpid, int sig)
 		if (index == -1) {
 			struct var_kill_data_t* kill_data =
 				get_var_kill_data(ctx, spid, tpid, sig);
-			if (kill_data == NULL)
+			if (!kill_data)
 				return 0;
 #ifdef UNROLL
 #pragma unroll
@@ -433,7 +433,7 @@ static INLINE int trace_var_sys_kill(void* ctx, int tpid, int sig)
 		} else {
 			struct var_kill_data_t* kill_data =
 				get_var_kill_data(ctx, spid, tpid, sig);
-			if (kill_data == NULL)
+			if (!kill_data)
 				return 0;
 			bpf_probe_read(&arr_struct->array[index],
 				       sizeof(arr_struct->array[index]),
@@ -534,14 +534,14 @@ static INLINE bool is_dentry_allowed_for_filemod(struct dentry* file_dentry,
 	*device_id = dev_id;
 	bool* allowed_device = bpf_map_lookup_elem(&allowed_devices, &dev_id);
 
-	if (allowed_device == NULL)
+	if (!allowed_device)
 		return false;
 
 	u64 ino = BPF_CORE_READ(file_dentry, d_inode, i_ino);
 	*file_ino = ino;
 	bool* allowed_file = bpf_map_lookup_elem(&allowed_file_inodes, &ino);
 
-	if (allowed_file == NULL)
+	if (!allowed_fil)
 		if (!is_ancestor_in_allowed_inodes(BPF_CORE_READ(file_dentry, d_parent)))
 			return false;
 	return true;
@@ -625,7 +625,7 @@ int raw_tracepoint__sched_process_exit(void* ctx)
 	struct var_kill_data_arr_t* arr_struct = bpf_map_lookup_elem(&var_tpid_to_data, &tpid);
 	struct var_kill_data_t* kill_data = bpf_map_lookup_elem(&data_heap, &zero);
 
-	if (arr_struct == NULL || kill_data == NULL)
+	if (!arr_struct || !kill_data)
 		goto out;
 
 	struct task_struct* task = (struct task_struct*)bpf_get_current_task();
@@ -767,7 +767,7 @@ int kprobe_ret__do_filp_open(struct pt_regs* ctx)
 
 	struct file* filp = (struct file*)PT_REGS_RC_CORE(ctx);
 
-	if (filp == NULL || IS_ERR(filp))
+	if (!filp || IS_ERR(filp))
 		goto out;
 	unsigned int flags = BPF_CORE_READ(filp, f_flags);
 	if ((flags & (O_RDWR | O_WRONLY)) == 0)
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] selftests/bpf: use !E instead of comparing with NULL
  2021-04-13  9:52 [PATCH] selftests/bpf: use !E instead of comparing with NULL Yang Li
@ 2021-04-13 15:17 ` Alexei Starovoitov
  2021-04-13 16:10   ` Tim.Bird
  0 siblings, 1 reply; 9+ messages in thread
From: Alexei Starovoitov @ 2021-04-13 15:17 UTC (permalink / raw)
  To: Yang Li
  Cc: Shuah Khan, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, open list:KERNEL SELFTEST FRAMEWORK,
	Network Development, bpf, LKML

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.
They might not adhere to kernel coding style in some cases.
The code could be grossly inefficient and even buggy.
Please don't run spell checks, coccicheck, checkpatch.pl on them.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] selftests/bpf: use !E instead of comparing with NULL
  2021-04-13 15:17 ` Alexei Starovoitov
@ 2021-04-13 16:10   ` Tim.Bird
  2021-04-13 16:13     ` Alexei Starovoitov
  0 siblings, 1 reply; 9+ messages in thread
From: Tim.Bird @ 2021-04-13 16:10 UTC (permalink / raw)
  To: alexei.starovoitov, yang.lee
  Cc: shuah, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, linux-kselftest, netdev, bpf,
	linux-kernel



> -----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?

> Please don't run spell checks, coccicheck, checkpatch.pl on them.

 -- Tim


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] selftests/bpf: use !E instead of comparing with NULL
  2021-04-13 16:10   ` Tim.Bird
@ 2021-04-13 16:13     ` Alexei Starovoitov
  2021-04-13 16:19       ` Tim.Bird
  0 siblings, 1 reply; 9+ messages in thread
From: Alexei Starovoitov @ 2021-04-13 16:13 UTC (permalink / raw)
  To: Bird, Tim
  Cc: Yang Li, Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, open list:KERNEL SELFTEST FRAMEWORK,
	Network Development, bpf, LKML

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] selftests/bpf: use !E instead of comparing with NULL
  2021-04-13 16:13     ` Alexei Starovoitov
@ 2021-04-13 16:19       ` Tim.Bird
  2021-04-13 16:26         ` Alexei Starovoitov
  0 siblings, 1 reply; 9+ messages in thread
From: Tim.Bird @ 2021-04-13 16:19 UTC (permalink / raw)
  To: alexei.starovoitov
  Cc: yang.lee, shuah, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, linux-kselftest, netdev, bpf,
	linux-kernel

> -----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.
 -- Tim



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] selftests/bpf: use !E instead of comparing with NULL
  2021-04-13 16:19       ` Tim.Bird
@ 2021-04-13 16:26         ` Alexei Starovoitov
  2021-04-13 16:32           ` Tim.Bird
  0 siblings, 1 reply; 9+ messages in thread
From: Alexei Starovoitov @ 2021-04-13 16:26 UTC (permalink / raw)
  To: Bird, Tim
  Cc: Yang Li, Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, open list:KERNEL SELFTEST FRAMEWORK,
	Network Development, bpf, LKML

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] selftests/bpf: use !E instead of comparing with NULL
  2021-04-13 16:26         ` Alexei Starovoitov
@ 2021-04-13 16:32           ` Tim.Bird
  2021-04-13 16:56             ` Alexei Starovoitov
  0 siblings, 1 reply; 9+ messages in thread
From: Tim.Bird @ 2021-04-13 16:32 UTC (permalink / raw)
  To: alexei.starovoitov
  Cc: yang.lee, shuah, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, linux-kselftest, netdev, bpf,
	linux-kernel

> -----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.
 -- Tim


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] selftests/bpf: use !E instead of comparing with NULL
  2021-04-13 16:32           ` Tim.Bird
@ 2021-04-13 16:56             ` Alexei Starovoitov
  2021-04-13 17:22               ` Tim.Bird
  0 siblings, 1 reply; 9+ messages in thread
From: Alexei Starovoitov @ 2021-04-13 16:56 UTC (permalink / raw)
  To: Bird, Tim
  Cc: Yang Li, Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, open list:KERNEL SELFTEST FRAMEWORK,
	Network Development, bpf, LKML

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] selftests/bpf: use !E instead of comparing with NULL
  2021-04-13 16:56             ` Alexei Starovoitov
@ 2021-04-13 17:22               ` Tim.Bird
  0 siblings, 0 replies; 9+ messages in thread
From: Tim.Bird @ 2021-04-13 17:22 UTC (permalink / raw)
  To: alexei.starovoitov
  Cc: yang.lee, shuah, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, linux-kselftest, netdev, bpf,
	linux-kernel

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

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13  9:52 [PATCH] selftests/bpf: use !E instead of comparing with NULL Yang Li
2021-04-13 15:17 ` Alexei Starovoitov
2021-04-13 16:10   ` Tim.Bird
2021-04-13 16:13     ` Alexei Starovoitov
2021-04-13 16:19       ` Tim.Bird
2021-04-13 16:26         ` Alexei Starovoitov
2021-04-13 16:32           ` Tim.Bird
2021-04-13 16:56             ` Alexei Starovoitov
2021-04-13 17:22               ` Tim.Bird

Linux-kselftest Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kselftest/0 linux-kselftest/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kselftest linux-kselftest/ https://lore.kernel.org/linux-kselftest \
		linux-kselftest@vger.kernel.org
	public-inbox-index linux-kselftest

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kselftest


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git