[bug report] perf lock: Print the number of lost entries for BPF

[bug report] perf lock: Print the number of lost entries for BPF

[Dan Carpenter]

Hello Namhyung Kim,

Commit 6d499a6b3d90 ("perf lock: Print the number of lost entries for
BPF") from Aug 2, 2022 (linux-next), leads to the following Smatch
static checker warning:

	./tools/perf/util/bpf_skel/lock_contention.bpf.c:379 contention_begin()
	warn: unsigned 'pelem->stack_id' is never less than zero.

./tools/perf/util/bpf_skel/lock_contention.bpf.c
    360 SEC("tp_btf/contention_begin")
    361 int contention_begin(u64 *ctx)
    362 {
    363         struct tstamp_data *pelem;
    364 
    365         if (!enabled || !can_record(ctx))
    366                 return 0;
    367 
    368         pelem = get_tstamp_elem(ctx[1]);
    369         if (pelem == NULL)
    370                 return 0;
    371 
    372         pelem->timestamp = bpf_ktime_get_ns();
    373         pelem->lock = (__u64)ctx[0];
    374         pelem->flags = (__u32)ctx[1];
    375 
    376         if (needs_callstack) {
    377                 pelem->stack_id = bpf_get_stackid(ctx, &stacks,
    378                                                   BPF_F_FAST_STACK_CMP | stack_skip);
--> 379                 if (pelem->stack_id < 0)

->stack_id is a u32 so it can't be less than zero

    380                         __sync_fetch_and_add(&stack_fail, 1);
    381         } else if (aggr_mode == LOCK_AGGR_TASK) {
    382                 struct task_struct *task;
    383 
    384                 if (lock_owner) {
    385                         task = get_lock_owner(pelem->lock, pelem->flags);
    386 
    387                         /* The flags is not used anymore.  Pass the owner pid. */
    388                         if (task)
    389                                 pelem->flags = BPF_CORE_READ(task, pid);

regards,
dan carpenter

[PATCH] perf lock contention: Change stack_id type to s32

[Namhyung Kim]

The bpf_get_stackid() helper returns a signed type to check whether it
failed to get a stacktrace or not.  But it saved the result in u32 and
checked if the value is negative.

      376         if (needs_callstack) {
      377                 pelem->stack_id = bpf_get_stackid(ctx, &stacks,
      378                                                   BPF_F_FAST_STACK_CMP | stack_skip);
  --> 379                 if (pelem->stack_id < 0)

  ./tools/perf/util/bpf_skel/lock_contention.bpf.c:379 contention_begin()
  warn: unsigned 'pelem->stack_id' is never less than zero.

Let's change the type to s32 instead.

Fixes: 6d499a6b3d90 ("perf lock: Print the number of lost entries for BPF")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/bpf_skel/lock_data.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/bpf_skel/lock_data.h b/tools/perf/util/bpf_skel/lock_data.h
index 36af11faad03..de12892f992f 100644
--- a/tools/perf/util/bpf_skel/lock_data.h
+++ b/tools/perf/util/bpf_skel/lock_data.h
@@ -7,11 +7,11 @@ struct tstamp_data {
 	u64 timestamp;
 	u64 lock;
 	u32 flags;
-	u32 stack_id;
+	s32 stack_id;
 };
 
 struct contention_key {
-	u32 stack_id;
+	s32 stack_id;
 	u32 pid;
 	u64 lock_addr_or_cgroup;
 };
-- 
2.46.0.76.ge559c4bf1a-goog

Re: [PATCH] perf lock contention: Change stack_id type to s32

[kernel test robot]

Hi Namhyung,

kernel test robot noticed the following build errors:

[auto build test ERROR on perf-tools-next/perf-tools-next]
[also build test ERROR on tip/perf/core perf-tools/perf-tools linus/master v6.11-rc2 next-20240809]
[cannot apply to acme/perf/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Namhyung-Kim/perf-lock-contention-Change-stack_id-type-to-s32/20240811-031933
base:   https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git perf-tools-next
patch link:    https://lore.kernel.org/r/20240810191704.1948365-1-namhyung%40kernel.org
patch subject: [PATCH] perf lock contention: Change stack_id type to s32
:::::: branch date: 23 hours ago
:::::: commit date: 23 hours ago
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240812/202408120233.2JuKgpj9-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202408120233.2JuKgpj9-lkp@intel.com/

All errors (new ones prefixed by >>):

   Makefile.config:671: No libunwind found. Please install libunwind-dev[el] >= 1.1 and/or set LIBUNWIND_DIR
     PERF_VERSION = 6.11.rc2.gcbe444bb08ed
   In file included from util/bpf_skel/lock_contention.bpf.c:9:
>> util/bpf_skel/lock_data.h:10:2: error: unknown type name 's32'; did you mean 'u32'?
      10 |         s32 stack_id;
         |         ^~~
         |         u32
   util/bpf_skel/vmlinux.h:17:15: note: 'u32' declared here
      17 | typedef __u32 u32;
         |               ^
   In file included from util/bpf_skel/lock_contention.bpf.c:9:
   util/bpf_skel/lock_data.h:14:2: error: unknown type name 's32'; did you mean 'u32'?
      14 |         s32 stack_id;
         |         ^~~
         |         u32
   util/bpf_skel/vmlinux.h:17:15: note: 'u32' declared here
      17 | typedef __u32 u32;
         |               ^
   2 errors generated.
   make[5]: *** [Makefile.perf:1247: tools/perf/util/bpf_skel/.tmp/lock_contention.bpf.o] Error 1
   make[5]: *** Waiting for unfinished jobs....
   make[4]: *** [Makefile.perf:292: sub-make] Error 2
   make[3]: *** [Makefile:76: all] Error 2


vim +10 tools/perf/util/bpf_skel/lock_data.h

fd507d3e359c7e Namhyung Kim 2022-12-09   5  
b44d6653685939 Namhyung Kim 2024-02-27   6  struct tstamp_data {
b44d6653685939 Namhyung Kim 2024-02-27   7  	u64 timestamp;
b44d6653685939 Namhyung Kim 2024-02-27   8  	u64 lock;
b44d6653685939 Namhyung Kim 2024-02-27   9  	u32 flags;
cbe444bb08ed25 Namhyung Kim 2024-08-10 @10  	s32 stack_id;
b44d6653685939 Namhyung Kim 2024-02-27  11  };
b44d6653685939 Namhyung Kim 2024-02-27  12  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] perf lock contention: Change stack_id type to s32

[Arnaldo Carvalho de Melo]

On Sat, Aug 10, 2024 at 12:17:04PM -0700, Namhyung Kim wrote:
> The bpf_get_stackid() helper returns a signed type to check whether it
> failed to get a stacktrace or not.  But it saved the result in u32 and
> checked if the value is negative.
> 
>       376         if (needs_callstack) {
>       377                 pelem->stack_id = bpf_get_stackid(ctx, &stacks,
>       378                                                   BPF_F_FAST_STACK_CMP | stack_skip);
>   --> 379                 if (pelem->stack_id < 0)
> 
>   ./tools/perf/util/bpf_skel/lock_contention.bpf.c:379 contention_begin()
>   warn: unsigned 'pelem->stack_id' is never less than zero.
> 
> Let's change the type to s32 instead.
> 
> Fixes: 6d499a6b3d90 ("perf lock: Print the number of lost entries for BPF")

Thanks, applied to perf-tools-next,

- Arnaldo

> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/bpf_skel/lock_data.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/bpf_skel/lock_data.h b/tools/perf/util/bpf_skel/lock_data.h
> index 36af11faad03..de12892f992f 100644
> --- a/tools/perf/util/bpf_skel/lock_data.h
> +++ b/tools/perf/util/bpf_skel/lock_data.h
> @@ -7,11 +7,11 @@ struct tstamp_data {
>  	u64 timestamp;
>  	u64 lock;
>  	u32 flags;
> -	u32 stack_id;
> +	s32 stack_id;
>  };
>  
>  struct contention_key {
> -	u32 stack_id;
> +	s32 stack_id;
>  	u32 pid;
>  	u64 lock_addr_or_cgroup;
>  };
> -- 
> 2.46.0.76.ge559c4bf1a-goog

Re: [PATCH] perf lock contention: Change stack_id type to s32

[Arnaldo Carvalho de Melo]

On Mon, Aug 12, 2024 at 01:09:40PM -0300, Arnaldo Carvalho de Melo wrote:
> On Sat, Aug 10, 2024 at 12:17:04PM -0700, Namhyung Kim wrote:
> > The bpf_get_stackid() helper returns a signed type to check whether it
> > failed to get a stacktrace or not.  But it saved the result in u32 and
> > checked if the value is negative.
> > 
> >       376         if (needs_callstack) {
> >       377                 pelem->stack_id = bpf_get_stackid(ctx, &stacks,
> >       378                                                   BPF_F_FAST_STACK_CMP | stack_skip);
> >   --> 379                 if (pelem->stack_id < 0)
> > 
> >   ./tools/perf/util/bpf_skel/lock_contention.bpf.c:379 contention_begin()
> >   warn: unsigned 'pelem->stack_id' is never less than zero.
> > 
> > Let's change the type to s32 instead.
> > 
> > Fixes: 6d499a6b3d90 ("perf lock: Print the number of lost entries for BPF")
> 
> Thanks, applied to perf-tools-next,

I'll try to fix this later, but now it fails the first 'make -C
tools/perf build-test' target, that you can run directly as:

⬢[acme@toolbox perf-tools-next]$ tools/perf/tests/perf-targz-src-pkg tools/perf
<SNIP>
  CLANG   /tmp/tmp.FRZLVEwqdz/perf-6.11.0-rc2/tools/perf/util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o
In file included from util/bpf_skel/lock_contention.bpf.c:9:
util/bpf_skel/lock_data.h:10:2: error: unknown type name 's32'; did you mean 'u32'?
   10 |         s32 stack_id;
      |         ^~~
      |         u32
util/bpf_skel/vmlinux.h:17:15: note: 'u32' declared here
   17 | typedef __u32 u32;
      |               ^
In file included from util/bpf_skel/lock_contention.bpf.c:9:
util/bpf_skel/lock_data.h:14:2: error: unknown type name 's32'; did you mean 'u32'?
   14 |         s32 stack_id;
      |         ^~~
      |         u32
util/bpf_skel/vmlinux.h:17:15: note: 'u32' declared here
   17 | typedef __u32 u32;
      |               ^
2 errors generated.
make[2]: *** [Makefile.perf:1247: /tmp/tmp.FRZLVEwqdz/perf-6.11.0-rc2/tools/perf/util/bpf_skel/.tmp/lock_contention.bpf.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [Makefile.perf:292: sub-make] Error 2
make: *** [Makefile:76: all] Error 2
make: Leaving directory '/tmp/tmp.FRZLVEwqdz/perf-6.11.0-rc2/tools/perf'
⬢[acme@toolbox perf-tools-next]$

Re: [PATCH] perf lock contention: Change stack_id type to s32

[Namhyung Kim]

On Mon, Aug 12, 2024 at 9:52 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Mon, Aug 12, 2024 at 01:09:40PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Sat, Aug 10, 2024 at 12:17:04PM -0700, Namhyung Kim wrote:
> > > The bpf_get_stackid() helper returns a signed type to check whether it
> > > failed to get a stacktrace or not.  But it saved the result in u32 and
> > > checked if the value is negative.
> > >
> > >       376         if (needs_callstack) {
> > >       377                 pelem->stack_id = bpf_get_stackid(ctx, &stacks,
> > >       378                                                   BPF_F_FAST_STACK_CMP | stack_skip);
> > >   --> 379                 if (pelem->stack_id < 0)
> > >
> > >   ./tools/perf/util/bpf_skel/lock_contention.bpf.c:379 contention_begin()
> > >   warn: unsigned 'pelem->stack_id' is never less than zero.
> > >
> > > Let's change the type to s32 instead.
> > >
> > > Fixes: 6d499a6b3d90 ("perf lock: Print the number of lost entries for BPF")
> >
> > Thanks, applied to perf-tools-next,
>
> I'll try to fix this later, but now it fails the first 'make -C
> tools/perf build-test' target, that you can run directly as:
>
> ⬢[acme@toolbox perf-tools-next]$ tools/perf/tests/perf-targz-src-pkg tools/perf
> <SNIP>
>   CLANG   /tmp/tmp.FRZLVEwqdz/perf-6.11.0-rc2/tools/perf/util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o
> In file included from util/bpf_skel/lock_contention.bpf.c:9:
> util/bpf_skel/lock_data.h:10:2: error: unknown type name 's32'; did you mean 'u32'?
>    10 |         s32 stack_id;
>       |         ^~~
>       |         u32
> util/bpf_skel/vmlinux.h:17:15: note: 'u32' declared here
>    17 | typedef __u32 u32;
>       |               ^

Oops, sorry about this.  There was a kernel test robot report.
It seems we need 'typedef __s32 s32;' too.

Thanks,
Namhyung


> In file included from util/bpf_skel/lock_contention.bpf.c:9:
> util/bpf_skel/lock_data.h:14:2: error: unknown type name 's32'; did you mean 'u32'?
>    14 |         s32 stack_id;
>       |         ^~~
>       |         u32
> util/bpf_skel/vmlinux.h:17:15: note: 'u32' declared here
>    17 | typedef __u32 u32;
>       |               ^
> 2 errors generated.
> make[2]: *** [Makefile.perf:1247: /tmp/tmp.FRZLVEwqdz/perf-6.11.0-rc2/tools/perf/util/bpf_skel/.tmp/lock_contention.bpf.o] Error 1
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [Makefile.perf:292: sub-make] Error 2
> make: *** [Makefile:76: all] Error 2
> make: Leaving directory '/tmp/tmp.FRZLVEwqdz/perf-6.11.0-rc2/tools/perf'
> ⬢[acme@toolbox perf-tools-next]$

Re: [PATCH] perf lock contention: Change stack_id type to s32

[Arnaldo Carvalho de Melo]

On Mon, Aug 12, 2024 at 09:57:27AM -0700, Namhyung Kim wrote:
> On Mon, Aug 12, 2024 at 9:52 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > On Mon, Aug 12, 2024 at 01:09:40PM -0300, Arnaldo Carvalho de Melo wrote:
> > > On Sat, Aug 10, 2024 at 12:17:04PM -0700, Namhyung Kim wrote:
> > > > The bpf_get_stackid() helper returns a signed type to check whether it
> > > > failed to get a stacktrace or not.  But it saved the result in u32 and
> > > > checked if the value is negative.
> > > >
> > > >       376         if (needs_callstack) {
> > > >       377                 pelem->stack_id = bpf_get_stackid(ctx, &stacks,
> > > >       378                                                   BPF_F_FAST_STACK_CMP | stack_skip);
> > > >   --> 379                 if (pelem->stack_id < 0)
> > > >
> > > >   ./tools/perf/util/bpf_skel/lock_contention.bpf.c:379 contention_begin()
> > > >   warn: unsigned 'pelem->stack_id' is never less than zero.
> > > >
> > > > Let's change the type to s32 instead.
> > > >
> > > > Fixes: 6d499a6b3d90 ("perf lock: Print the number of lost entries for BPF")
> > >
> > > Thanks, applied to perf-tools-next,
> >
> > I'll try to fix this later, but now it fails the first 'make -C
> > tools/perf build-test' target, that you can run directly as:
> >
> > ⬢[acme@toolbox perf-tools-next]$ tools/perf/tests/perf-targz-src-pkg tools/perf
> > <SNIP>
> >   CLANG   /tmp/tmp.FRZLVEwqdz/perf-6.11.0-rc2/tools/perf/util/bpf_skel/.tmp/augmented_raw_syscalls.bpf.o
> > In file included from util/bpf_skel/lock_contention.bpf.c:9:
> > util/bpf_skel/lock_data.h:10:2: error: unknown type name 's32'; did you mean 'u32'?
> >    10 |         s32 stack_id;
> >       |         ^~~
> >       |         u32
> > util/bpf_skel/vmlinux.h:17:15: note: 'u32' declared here
> >    17 | typedef __u32 u32;
> >       |               ^
> 
> Oops, sorry about this.  There was a kernel test robot report.

Cool, I didn't see it, but its good its doing this work.

> It seems we need 'typedef __s32 s32;' too.

Please send a v2 then,

Thanks,

- Arnaldo
 
> Thanks,
> Namhyung
> 
> 
> > In file included from util/bpf_skel/lock_contention.bpf.c:9:
> > util/bpf_skel/lock_data.h:14:2: error: unknown type name 's32'; did you mean 'u32'?
> >    14 |         s32 stack_id;
> >       |         ^~~
> >       |         u32
> > util/bpf_skel/vmlinux.h:17:15: note: 'u32' declared here
> >    17 | typedef __u32 u32;
> >       |               ^
> > 2 errors generated.
> > make[2]: *** [Makefile.perf:1247: /tmp/tmp.FRZLVEwqdz/perf-6.11.0-rc2/tools/perf/util/bpf_skel/.tmp/lock_contention.bpf.o] Error 1
> > make[2]: *** Waiting for unfinished jobs....
> > make[1]: *** [Makefile.perf:292: sub-make] Error 2
> > make: *** [Makefile:76: all] Error 2
> > make: Leaving directory '/tmp/tmp.FRZLVEwqdz/perf-6.11.0-rc2/tools/perf'
> > ⬢[acme@toolbox perf-tools-next]$