Re: [PATCH v9 perf,bpf 00/15] perf annotation of BPF programs

[Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>]

Em Thu, Mar 21, 2019 at 10:10:16AM +0100, Jiri Olsa escreveu:
> On Mon, Mar 11, 2019 at 10:30:36PM -0700, Song Liu wrote:
> > Changes v8 to v9:
> > 1. Rebase on top of torvalds/master:
> >     commit ea295481b6e313b4ea3ca2720ffcafd6005b5643 ;
> > 2. Change perf-record option --bpf-event to --no-bpf-event.
> > 
> > Changes v7 to v8:
> > 1. Address issues suggested by Jiri and Stanislav.
> > 
> > Changes v6 to v7:
> > 1. Fix minor issues suggested by Jiri.
> > 
> > Changes v5 to v6:
> > 1. Improve side band evlist interface;
> > 2. Minor style fixes.
> > 
> > Changes v4 to v5:
> > 1. Rebase to latest bpf-next;
> > 2. Add dependency of 94816add0005 from Arnaldo's tree;
> > 3. More details in change logs;
> > 4. Add perf_env__init() to init bpf related lock and rbtrees;
> > 5. Small clean ups.
> > 
> > Changes v3 to v4:
> > 1. Incorporate feedbacks from Jiri and Namhyung;
> > 2. Fixed compilation error with different feature-disassembler-four-args;
> > 3. Split some patches to smaller patches;
> > 4. Improved error handleing in symbol__disassemble_bpf();
> > 5. Made side band thread more generic;
> > 6. Added comments as suggested.
> > 
> > Changes v2 to v3:
> > 1. Remove unnecessary include in header files;
> > 2. Improved error handling;
> > 3. Better naming of functions, variables, etc.;
> > 4. Enable bpf events by default for perf-top.
> > 
> > Changes v1 to v2:
> > 1. Fix compilation error with different feature-disassembler-four-args;
> > 2. Fix a segfault in perf-record;
> > 3. Split patches 5/9 and 6/9 so that perf_env changes and perf.data changes
> >    are in separate patches.
> > 
> > This series enables annotation of BPF programs in perf.
> > 
> > perf tool gathers information via sys_bpf and (optionally) stores them in
> > perf.data as headers.
> > 
> > Patch 1/15 changes --bpf-event option to --no-bpf-event;
> > Patch 2/15 to 4/15 introduce new helper functions and use them in perf and
> >      bpftool;
> > Patch 5/15 to 9/15 saves information of bpf program in perf_env;
> > Patch 10/15 adds --bpf-event options to perf-top;
> > Patch 11/15 to 13/15 enables annotation of bpf progs based on information
> >      gathered in 5/15 to 9/15;
> > Patch 14/15 introduces side band polling thread that gathers information
> >      for special kernel events during perf-record or perf-top.
> > Patch 15/15 handles information of short living BPF program using the new
> >      side band polling thread.
> > 
> > Commands tested during developments are perf-top, perf-record, perf-report,
> > and perf-annotate.
> > 
> > This set is also available at:
> > 
> > https://github.com/liu-song-6/linux/tree/bpf-annotation-v9
> > 
> > Thanks!!
> > 
> > Song Liu (15):
> >   perf-record: replace option --bpf-event with --no-bpf-event
> >   bpf: libbpf: introduce bpf_program__get_prog_info_linear()
> >   bpf: bpftool: use bpf_program__get_prog_info_linear() in
> >     prog.c:do_dump()
> >   perf, bpf: synthesize bpf events with
> >     bpf_program__get_prog_info_linear()
> >   perf: change prototype of perf_event__synthesize_bpf_events()
> >   perf, bpf: save bpf_prog_info in a rbtree in perf_env
> >   perf, bpf: save bpf_prog_info information as headers to perf.data
> >   perf, bpf: save btf in a rbtree in perf_env
> >   perf, bpf: save btf information as headers to perf.data
> >   perf-top: add option --no-bpf-event
> >   perf: add -lopcodes to feature-libbfd
> >   perf, bpf: enable annotation of bpf program
> >   perf, bpf: process PERF_BPF_EVENT_PROG_LOAD for annotation
> >   perf: introduce side band thread
> >   perf, bpf: save bpf_prog_info and btf of short living bpf programs
> 
> it still crashes on standard user, like:
> 
> 
> [jolsa@krava perf]$ ./perf record  ./perf bench sched messaging
> # Running 'sched/messaging' benchmark:
> # 20 sender and receiver processes per group
> # 10 groups == 400 processes run
> 
>      Total time: 0.143 [sec]
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.282 MB perf.data (6341 samples) ]
> perf: Segmentation fault
> Obtained 14 stack frames.
> ./perf(dump_stack+0x2d) [0x531f8b]
> ./perf(sighandler_dump_stack+0x2d) [0x53206b]
> ./perf() [0x444cd9]
> /lib64/libc.so.6(+0x36f2f) [0x7f64a0ef2f2f]
> /lib64/libpthread.so.0(+0x88bc) [0x7f64a32518bc]
> ./perf(perf_evlist__stop_sb_thread+0x47) [0x4f30fc]
> ./perf() [0x447909]
> ./perf(cmd_record+0x815) [0x448afc]
> ./perf() [0x4d4c52]
> ./perf() [0x4d4ebf]
> ./perf() [0x4d500e]
> ./perf(main+0x267) [0x4d537a]
> /lib64/libc.so.6(__libc_start_main+0xea) [0x7f64a0edf11a]
> ./perf(_start+0x29) [0x42cbd9]
> Segmentation fault (core dumped)
> 
> 
> the reason is that you try to open system wide counter
> and that's not allow for standard user.. so the thread
> won't get started, but there's code to stop it unconsidtionaly
> which crashes
> 
> 
> the patch below fixes that for me, but I guess we want
> to check if th eerror was -EACCESS, if not we should fail

Right, I'll apply your patch as a stop-gap solution so that I can
proceed with pushing what we already have, and then we need to polish
this thing for good.


- Arnaldo
 
> jirka
> 
> 
> ---
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 08832fdaa26b..766059bf26ee 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1240,7 +1240,8 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  
>  	if (!opts->no_bpf_event)
>  		bpf_event__add_sb_event(&sb_evlist, &session->header.env);
> -	perf_evlist__start_sb_thread(sb_evlist, &rec->opts.target);
> +	if (perf_evlist__start_sb_thread(sb_evlist, &rec->opts.target))
> +		opts->no_bpf_event = true;
>  
>  	err = record__synthesize(rec, false);
>  	if (err < 0)
> @@ -1493,7 +1494,8 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  out_delete_session:
>  	perf_session__delete(session);
>  
> -	perf_evlist__stop_sb_thread(sb_evlist);
> +	if (!opts->no_bpf_event)
> +		perf_evlist__stop_sb_thread(sb_evlist);
>  	return status;
>  }
>  

-- 

- Arnaldo