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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 15 Feb 2019 18:13:35 +0000
From:   Song Liu <songliubraving@...com>
To:     Arnaldo Carvalho de Melo <acme@...hat.com>
CC:     Netdev <netdev@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "jolsa@...nel.org" <jolsa@...nel.org>,
        "namhyung@...nel.org" <namhyung@...nel.org>
Subject: Re: [PATCH v2 perf,bpf 11/11] perf, bpf: save information about short
 living bpf programs



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