Re: FIX Re: [PATCH v7 3/3] perf-stat: enable counting events for BPF programs

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Wed, Jan 20, 2021 at 04:40:46PM +0000, Song Liu escreveu:
> 
> 
> > On Jan 20, 2021, at 8:30 AM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > 
> > Em Wed, Jan 20, 2021 at 10:50:13AM -0300, Arnaldo Carvalho de Melo escreveu:
> >> So sizeof(struct bpf_perf_event_value) == 24 and it is a per-cpu array, the
> >> machine has 24 cpus, why is the kernel thinking it has more and end up zeroing
> >> entries after the 24 cores? Some percpu map subtlety (or obvious thing ;-\) I'm
> >> missing?
> >> 
> >> Checking lookups into per cpu maps in sample code now...
> > 
> > (gdb) run stat -b 244 -I 1000 -e cycles
> > Starting program: /root/bin/perf stat -b 244 -I 1000 -e cycles
> > [Thread debugging using libthread_db enabled]
> > Using host libthread_db library "/lib64/libthread_db.so.1".
> > libbpf: elf: skipping unrecognized data section(9) .eh_frame
> > libbpf: elf: skipping relo section(15) .rel.eh_frame for section(9) .eh_frame
> > 
> > Breakpoint 1, bpf_program_profiler__read (evsel=0xce02c0) at util/bpf_counter.c:217
> > 217		if (list_empty(&evsel->bpf_counter_list))
> > (gdb) p num_
> > num_cpu              num_groups           num_leaps            num_print_iv         num_stmts            num_transitions      num_warnings_issued
> > num_cpu_bpf          num_ifs              num_print_interval   num_srcfiles         num_to_str           num_types
> > (gdb) p num_cpu
> > $1 = 24
> > (gdb) p num_cpu_bpf
> > $2 = 32
> > (gdb)
> > 
> > Humm, why?
> > 
> > But then libbpf and the sample/bpf/ code use it this way:
> > 
> > 
> > diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
> > index 8c977f038f497fc1..7dd3d57aba4f620c 100644
> > --- a/tools/perf/util/bpf_counter.c
> > +++ b/tools/perf/util/bpf_counter.c
> > @@ -207,7 +207,8 @@ static int bpf_program_profiler__enable(struct evsel *evsel)
> > static int bpf_program_profiler__read(struct evsel *evsel)
> > {
> > 	int num_cpu = evsel__nr_cpus(evsel);
> > -	struct bpf_perf_event_value values[num_cpu];
> > +	int num_cpu_bpf = libbpf_num_possible_cpus();
> > +	struct bpf_perf_event_value values[num_cpu > num_cpu_bpf ? num_cpu : num_cpu_bpf];
> > 	struct bpf_counter *counter;
> > 	int reading_map_fd;
> > 	__u32 key = 0;
> > 
> > -------------------------------------------------------------
> > 
> > [root@five ~]# cat /sys/devices/system/cpu/possible
> > 0-31
> > [root@five ~]#
> > 
> > I bet that in your test systems evsel__nr_cpus(evsel) matches
> > /sys/devices/system/cpu/possible and thus you don't see the problem.
> 
> Thanks Arnaldo!
> 
> Yes, my system have same online and possible CPUs. 
> 
> Since possible_cpu >= online_cpu, maybe we can use num_cpu_bpf in 
> bpf_program_profiler__read() without he extra check? 

That is what I'll do, no need to resubmit, I'll audit the other bits to
see if something else needs changing.

- Arnaldo