Messages in this thread | | | From | Song Liu <> | Subject | Re: [PATCH v2 perf,bpf 11/11] perf, bpf: save information about short living bpf programs | Date | Fri, 15 Feb 2019 18:13:35 +0000 |
| |
> On Feb 15, 2019, at 10:09 AM, Arnaldo Carvalho de Melo <acme@redhat.com> wrote: > > Em Fri, Feb 15, 2019 at 05:13:01PM +0000, Song Liu escreveu: >>> On Feb 15, 2019, at 6:41 AM, Arnaldo Carvalho de Melo <acme@redhat.com> wrote: >>> Em Thu, Feb 14, 2019 at 04:00:45PM -0800, Song Liu escreveu: >>>> +pthread_t poll_thread; >>>> + >>>> +int bpf_event__start_polling_thread(struct bpf_event_poll_args *args) >>>> +{ >>>> + struct perf_evsel *counter; >>>> + >>>> + args->evlist = perf_evlist__new(); >>>> + >>>> + if (args->evlist == NULL) >>>> + return -1; >>>> + >>>> + if (perf_evlist__create_maps(args->evlist, args->target)) >>> goto out_delete_evlist; >>>> + >>>> + if (perf_evlist__add_bpf_tracker(args->evlist)) >>> goto out_delete_evlist; >>>> + >>>> + evlist__for_each_entry(args->evlist, counter) { >>>> + if (perf_evsel__open(counter, args->evlist->cpus, >>>> + args->evlist->threads) < 0) >>> goto out_delete_evlist; >>>> + } >>>> + >>>> + if (perf_evlist__mmap(args->evlist, UINT_MAX)) >>> goto out_delete_evlist; >>>> + >>>> + evlist__for_each_entry(args->evlist, counter) { >>>> + if (perf_evsel__enable(counter)) >>> goto out_delete_evlist; >>>> + } >>>> + >>>> + if (pthread_create(&poll_thread, NULL, bpf_poll_thread, args)) >>> goto out_delete_evlist; >>>> + >>>> + return 0; >>> out_delete_evlist: >>> perf_evlist__delete(args->evlist); >>> args->evlist = NULL; > > Have you seen the error handling suggestion above?
Yes! I will include these changes, as always. :)
> >>>> +int perf_evlist__add_bpf_tracker(struct perf_evlist *evlist) >>>> +{ >>>> + struct perf_event_attr attr = { >>>> + .type = PERF_TYPE_SOFTWARE, >>>> + .config = PERF_COUNT_SW_DUMMY, >>>> + .watermark = 1, >>>> + .bpf_event = 1, >>>> + .wakeup_watermark = 1, >>>> + .size = sizeof(attr), /* to capture ABI version */ >>>> + }; >>>> + struct perf_evsel *evsel = perf_evsel__new_idx(&attr, >>>> + evlist->nr_entries); >>>> + >>>> + if (evsel == NULL) >>>> + return -ENOMEM; >>>> + >>>> + perf_evlist__add(evlist, evsel); > >>> You could use: > >>> struct perf_evlist *evlist = perf_evlist__new_dummy(); >>> if (evlist != NULL) { >>> struct perf_evsel *evsel == perf_evlist__first(evlist); >>> evsel->attr.bpf_event = evsel->attr.watermark = evsel->attr.wakeup_watermark = 1; >>> return 0; >>> } >>> return -1; > >> This looks cleaner. Let me fix in next version. > >>> Because in this case all you'll have in this evlist is the bpf tracker, >>> right? The add_bpf_tracker would be handy if we would want to have a >>> pre-existing evlist with some other events and wanted to add a bpf >>> tracker, no? > >> I think all we need is a side-band evlist instead of the main evlist. May >> be we should call it side-band evlist, and make it more generic? > > Sure, you could for instance have something like: > > struct perf_event_attr attr = { > .watermark = 1, > .bpf_event = 1, > .wakeup_watermark = 1, > } > struct perf_evlist *evlist = perf_evlist__new_side_band(&attr); > > > And the other details will be set by it, i.e. the .config > > .type = PERF_TYPE_SOFTWARE, > .config = PERF_COUNT_SW_DUMMY, > .size = sizeof(attr), /* to capture ABI version */ > > And the idx arg. > > - Arnaldo
This looks good. Let me revise the patch in that direction.
Thanks, Song
| |