Message-Id: <B1456234-7D08-4174-B374-F809FE371612@163.com>
Date:	Thu, 3 Sep 2015 06:24:30 +0800
From:	pi3orama <pi3orama@....com>
To:	Arnaldo Carvalho de Melo <acme@...hat.com>
Cc:	Jiri Olsa <jolsa@...hat.com>, "Wangnan (F)" <wangnan0@...wei.com>,
	"masami.hiramatsu.pt@...achi.com" <masami.hiramatsu.pt@...achi.com>,
	Alexei Starovoitov <ast@...mgrid.com>,
	Jiri Olsa <jolsa@...nel.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Zefan Li <lizefan@...wei.com>,
	"acme@...nel.org" <acme@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel



发自我的 iPhone

> 在 2015年9月2日，下午10:43，Arnaldo Carvalho de Melo <acme@...hat.com> 写道：
> 
> Em Wed, Sep 02, 2015 at 10:04:21PM +0800, pi3orama escreveu:
>> 发自我的 iPhone
>>> 在 2015年9月2日，下午9:55，Arnaldo Carvalho de Melo <acme@...hat.com> 写道：
>>> Em Wed, Sep 02, 2015 at 08:05:54PM +0800, pi3orama escreveu:
>>>> 发自我的 iPhone
>>>>> 在 2015年9月2日，下午7:54，Jiri Olsa <jolsa@...hat.com> 写道：
>>>>>>> On Wed, Sep 02, 2015 at 02:53:58PM +0800, Wangnan (F) wrote:
>>>>>>> @@ -1252,7 +1262,13 @@ foreach_evsel_in_last_glob(struct perf_evlist *evlist,
>>>>>>>  struct perf_evsel *last = NULL;
>>>>>>>  int err;
>>>>>>> -    if (evlist->nr_entries > 0)
>>>>>>> +    /*
>>>>>>> +     * Don't return when list_empty, give func a chance to report
>>>>>>> +     * error when it found last == NULL.
>>>>>>> +     *
>>>>>>> +     * So no need to WARN here, let *func do this.
>>>>>>> +     */
>>>>>>> +    if (!list_empty(&evlist->entries))
> 
>>>>> why is it better than to check evlist->nr_entries?
>>>>> evlist->nr_entries is equivalent to !list_empty(&evlist->entries) in here, right?
> 
>>>> By checking list we won't rely on the assumption that nr_entries reflects the
>>>> actual number of elements in that list, makes the logic of this code more compact.
> 
>>> But why would we want to break that assumption?
> 
>>> If I see FOO->entries and FOO->nr_entries, it is reasonable to expect
>>> that whatever data structure FOO->entries may be has FOO->nr_entries in
>>> it, lets not break that assumption.
> 
>> Then we should enforce it.
> 
> Agreed, but it is a reasonable expectation, right? Its a general
> pattern, one that we expect and when it breaks like that, that may lead
> to bugs :-)
> 
>> For example, check the list collected by parser, report an error if
>> the list is empty, to avoid someone like me adding nothing on the list
>> but report success. I'm not insistent on this patch. In my newest
>> patch set I use real dummy evsel as placeholder so we won't meet empty
>> list again.
> 
> Ok, I'll look at the new patch then, I keep thinking that if you need to
> have a separate list for eBPF, that you will do something special on it,
> etc, then that is not a problem just keep it as a separate list till you
> can insert it in the evlist to then open the evlist, mmap it, etc.
> 
> If in the parsing routines you have access only to a perf_evlist
> pointer, well, then we can have something like an
> evlist->pending_entries + evlist->nr_pending_entries. Something like
> that.
> 
> If you have detailed why you need it to be left in the evlist (will some
> operation be done on all evsels, even the ones that need eBPF specific
> work before you do this final eBPF specific stuff?), I'll try to find
> it, if not, describing the sequence of events that justifies this or the
> "dummy evsel as a placeholder" would help reviewing this, treat us like
> 7 year old kids (aka use patience + details) :-)

Just because adding placeholder make things simpler, because which makes
bpf object "events" become compatible with other types of events, so we
don't need to maintain a separated mechanism during parsing.

I think you should remember how we sync filters between place holder and real
events. Actually, filter is not the only setting can be made to an event after the evsel
is collected. We also have config terms, groups and modifiers. Although currently
we only support filter, we are planning adding config terms to config bpf objects,
so we can use commands like:
 # perf record --event abc.o/key=value/ ...

Modifier is also useful:
 # perf record --event abc.o:G ...

And also group:
 # perf record --event {abc.o,def.o,cycles}/key=value/...

Think about how we can do this if we use a separate list for BPF object. Then in
all the above processing, we must change current code, detect whether we are
dealing with BPF, and treat BPF object and normal events differently.

Instead, with placeholder dummy event, we don't need to modify existing implementation. And we also don't need to consider BPF if we decide to
add more settings by new syntax. BPF object "events" would be naturally
compatible with normal dummy events (only thing we should consider is we
need to treat it as TRACEPOINT event, not a SOFTWARE event). We use it to
collect settings, and when real BPF events created, sync settings between them.

I believe the above should be strong enough to support dummy event, do you think
so?

And you can review how I do this now from github (sorry I can't send patch
because I'm at home and won't be able to access company's SMTP server for
3 days due holiday):
https://github.com/WangNan0/linux/commit/c6fe9842d27ae1d228be2c7bb6c20216ddc49632
https://github.com/WangNan0/linux/commit/802426eddb9ea15386e70063d935d95caa30c045

Thank you.

> - Arnaldo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists