Re: [PATCH v2 perf,bpf 11/11] perf, bpf: save information about short living bpf programs

From: Song Liu <songliubraving@fb.com>
To: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Netdev <netdev@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	"ast@kernel.org" <ast@kernel.org>,
	"daniel@iogearbox.net" <daniel@iogearbox.net>,
	Kernel Team <Kernel-team@fb.com>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"jolsa@kernel.org" <jolsa@kernel.org>,
	"namhyung@kernel.org" <namhyung@kernel.org>
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	[thread overview]
Message-ID: <D4559155-4993-4319-81C9-AC4ADDDAE3B6@fb.com> (raw)
In-Reply-To: <20190215180916.GJ5784@redhat.com>

> 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