> On Feb 15, 2019, at 10:09 AM, Arnaldo Carvalho de Melo <a...@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 <a...@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
