Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 15 Feb 2019 16:09:16 -0200
From:   Arnaldo Carvalho de Melo <acme@redhat.com>
To:     Song Liu <songliubraving@fb.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
Message-ID: <20190215180916.GJ5784@redhat.com>
References: <20190214235624.2579307-1-songliubraving@fb.com>
 <20190215000045.2592135-1-songliubraving@fb.com>
 <20190215000045.2592135-2-songliubraving@fb.com>
 <20190215144146.GF5784@redhat.com>
 <62DB92A4-F708-4E89-B668-4EAFD0FA6731@fb.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <62DB92A4-F708-4E89-B668-4EAFD0FA6731@fb.com>
User-Agent: Mutt/1.5.20 (2009-12-10)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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?

> >> +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