Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1215686 > unrolled thread

[PATCH 02/31] perf tools: Don't set cmdline_group_boundary if no evsel is collected

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 02/31] perf tools: Don't set cmdline_group_boundary if no evsel is collected Wang Nan <wangnan0@huawei.com> - 2015-08-29 06:30 +0200
    Re: [PATCH 02/31] perf tools: Don't set cmdline_group_boundary if no  evsel is collected Arnaldo Carvalho de Melo <acme@redhat.com> - 2015-08-31 21:30 +0200
      Re: [PATCH 02/31] perf tools: Don't set cmdline_group_boundary if no  evsel is collected Jiri Olsa <jolsa@redhat.com> - 2015-09-01 12:40 +0200
        Re: [PATCH 02/31] perf tools: Don't set cmdline_group_boundary if  no evsel is collected "Wangnan (F)" <wangnan0@huawei.com> - 2015-09-01 14:50 +0200
      Re: [PATCH 02/31] perf tools: Don't set cmdline_group_boundary if  no evsel is collected "Wangnan (F)" <wangnan0@huawei.com> - 2015-09-01 13:00 +0200
    [PATCH] perf tools: Don't set leader if parser doesn't collect an evsel Wang Nan <wangnan0@huawei.com> - 2015-09-02 05:00 +0200
      Re: [PATCH] perf tools: Don't set leader if parser doesn't collect  an evsel "Wangnan (F)" <wangnan0@huawei.com> - 2015-09-02 05:10 +0200
      RE: [PATCH] perf tools: Don't set leader if parser doesn't collect  an evsel 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-09-02 08:00 +0200
        Re: [PATCH] perf tools: Don't set leader if parser doesn't collect  an evsel "Wangnan (F)" <wangnan0@huawei.com> - 2015-09-02 08:10 +0200
        Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect  evsel "Wangnan (F)" <wangnan0@huawei.com> - 2015-09-02 09:00 +0200
          RE: [PATCH] perf tools: Don't write to evsel if parser doesn't  collect evsel 平松雅巳 / HIRAMATU，MASAMI   <masami.hiramatsu.pt@hitachi.com> - 2015-09-02 12:40 +0200
          Re: [PATCH] perf tools: Don't write to evsel if parser doesn't  collect evsel Jiri Olsa <jolsa@redhat.com> - 2015-09-02 14:00 +0200
            Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel pi3orama <pi3orama@163.com> - 2015-09-02 14:10 +0200
              Re: [PATCH] perf tools: Don't write to evsel if parser doesn't  collect evsel Jiri Olsa <jolsa@redhat.com> - 2015-09-02 14:50 +0200
              Re: [PATCH] perf tools: Don't write to evsel if parser doesn't  collect evsel Arnaldo Carvalho de Melo <acme@redhat.com> - 2015-09-02 16:00 +0200
                Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel pi3orama <pi3orama@163.com> - 2015-09-02 16:10 +0200
                  Re: [PATCH] perf tools: Don't write to evsel if parser doesn't  collect evsel Arnaldo Carvalho de Melo <acme@redhat.com> - 2015-09-02 16:50 +0200
                    Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel pi3orama <pi3orama@163.com> - 2015-09-03 00:30 +0200

#1215686 — [PATCH 02/31] perf tools: Don't set cmdline_group_boundary if no evsel is collected

If parse_events__scanner() collects no entry, perf_evlist__last(evlist)
is invalid. Then setting of cmdline_group_boundary touches invalid.

It could happend in currect BPF implementation. See [1]. Although it
can be fixed, for safety reason it whould be better to introduce this
check.

Instead of checking number of entries, check data.list instead, so we
can add dummy evsel here.

[1]: http://lkml.kernel.org/n/1436445342-1402-19-git-send-email-wangnan0@huawei.com

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Zefan Li <lizefan@huawei.com>
Cc: pi3orama@163.com
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Link: http://lkml.kernel.org/r/1440742821-44548-3-git-send-email-wangnan0@huawei.com
---
 tools/perf/util/parse-events.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index d826e6f..14cd7e3 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1143,10 +1143,14 @@ int parse_events(struct perf_evlist *evlist, const char *str,
 		int entries = data.idx - evlist->nr_entries;
 		struct perf_evsel *last;
 
+		if (!list_empty(&data.list)) {
+			last = list_entry(data.list.prev,
+					  struct perf_evsel, node);
+			last->cmdline_group_boundary = true;
+		}
+
 		perf_evlist__splice_list_tail(evlist, &data.list, entries);
 		evlist->nr_groups += data.nr_groups;
-		last = perf_evlist__last(evlist);
-		last->cmdline_group_boundary = true;
 
 		return 0;
 	}
-- 
2.1.0

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

[toc] | [next] | [standalone]

#1216395 — Re: [PATCH 02/31] perf tools: Don't set cmdline_group_boundary if no evsel is collected

Em Sat, Aug 29, 2015 at 04:21:36AM +0000, Wang Nan escreveu:
> If parse_events__scanner() collects no entry, perf_evlist__last(evlist)
> is invalid. Then setting of cmdline_group_boundary touches invalid.
> 
> It could happend in currect BPF implementation. See [1]. Although it
> can be fixed, for safety reason it whould be better to introduce this
> check.
> 
> Instead of checking number of entries, check data.list instead, so we
> can add dummy evsel here.

Event parsing fixes should have Jiri Olsa on the CC list, Jiri, is this
ok?

From what I can see it looks Ok, my question, just from looking at this
patch, is if it is valid to get to this point with an empty data.list,
i.e. was it ever possible and this is a bug irrespective of eBPF?

- Arnaldo
 
> [1]: http://lkml.kernel.org/n/1436445342-1402-19-git-send-email-wangnan0@huawei.com
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Alexei Starovoitov <ast@plumgrid.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Zefan Li <lizefan@huawei.com>
> Cc: pi3orama@163.com
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Link: http://lkml.kernel.org/r/1440742821-44548-3-git-send-email-wangnan0@huawei.com
> ---
>  tools/perf/util/parse-events.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index d826e6f..14cd7e3 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1143,10 +1143,14 @@ int parse_events(struct perf_evlist *evlist, const char *str,
>  		int entries = data.idx - evlist->nr_entries;
>  		struct perf_evsel *last;
>  
> +		if (!list_empty(&data.list)) {
> +			last = list_entry(data.list.prev,
> +					  struct perf_evsel, node);
> +			last->cmdline_group_boundary = true;
> +		}
> +
>  		perf_evlist__splice_list_tail(evlist, &data.list, entries);
>  		evlist->nr_groups += data.nr_groups;
> -		last = perf_evlist__last(evlist);
> -		last->cmdline_group_boundary = true;
>  
>  		return 0;
>  	}
> -- 
> 2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1216756 — Re: [PATCH 02/31] perf tools: Don't set cmdline_group_boundary if no evsel is collected

On Mon, Aug 31, 2015 at 04:20:03PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sat, Aug 29, 2015 at 04:21:36AM +0000, Wang Nan escreveu:
> > If parse_events__scanner() collects no entry, perf_evlist__last(evlist)
> > is invalid. Then setting of cmdline_group_boundary touches invalid.
> > 
> > It could happend in currect BPF implementation. See [1]. Although it
> > can be fixed, for safety reason it whould be better to introduce this
> > check.
> > 
> > Instead of checking number of entries, check data.list instead, so we
> > can add dummy evsel here.
> 
> Event parsing fixes should have Jiri Olsa on the CC list, Jiri, is this
> ok?
> 
> From what I can see it looks Ok, my question, just from looking at this
> patch, is if it is valid to get to this point with an empty data.list,
> i.e. was it ever possible and this is a bug irrespective of eBPF?

good point, I believe it's either fail or event(s) added to the list
I haven't checked how's eBPF connected with event parsing, is there a
git tree I could check?

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

[toc] | [prev] | [next] | [standalone]

#1216819 — Re: [PATCH 02/31] perf tools: Don't set cmdline_group_boundary if no evsel is collected

On 2015/9/1 18:38, Jiri Olsa wrote:
> On Mon, Aug 31, 2015 at 04:20:03PM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Sat, Aug 29, 2015 at 04:21:36AM +0000, Wang Nan escreveu:
>>> If parse_events__scanner() collects no entry, perf_evlist__last(evlist)
>>> is invalid. Then setting of cmdline_group_boundary touches invalid.
>>>
>>> It could happend in currect BPF implementation. See [1]. Although it
>>> can be fixed, for safety reason it whould be better to introduce this
>>> check.
>>>
>>> Instead of checking number of entries, check data.list instead, so we
>>> can add dummy evsel here.
>> Event parsing fixes should have Jiri Olsa on the CC list, Jiri, is this
>> ok?
>>
>>  From what I can see it looks Ok, my question, just from looking at this
>> patch, is if it is valid to get to this point with an empty data.list,
>> i.e. was it ever possible and this is a bug irrespective of eBPF?
> good point, I believe it's either fail or event(s) added to the list
> I haven't checked how's eBPF connected with event parsing, is there a
> git tree I could check?

Please check:

https://git.kernel.org/cgit/linux/kernel/git/pi3orama/linux.git/log/?h=perf/ebpf

commit d7d91228cad0a78eae5ea9526a8a78debf3cf584
commit 2606fe61219899cb386823eddc1bc231ff5067a6

related to parsing.

Thank you.

> thanks,
> jirka


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

[toc] | [prev] | [next] | [standalone]

#1216773 — Re: [PATCH 02/31] perf tools: Don't set cmdline_group_boundary if no evsel is collected

On 2015/9/1 3:20, Arnaldo Carvalho de Melo wrote:
> Em Sat, Aug 29, 2015 at 04:21:36AM +0000, Wang Nan escreveu:
>> If parse_events__scanner() collects no entry, perf_evlist__last(evlist)
>> is invalid. Then setting of cmdline_group_boundary touches invalid.
>>
>> It could happend in currect BPF implementation. See [1]. Although it
>> can be fixed, for safety reason it whould be better to introduce this
>> check.
>>
>> Instead of checking number of entries, check data.list instead, so we
>> can add dummy evsel here.
> Event parsing fixes should have Jiri Olsa on the CC list, Jiri, is this
> ok?
>
>  From what I can see it looks Ok, my question, just from looking at this
> patch, is if it is valid to get to this point with an empty data.list,
> i.e. was it ever possible and this is a bug irrespective of eBPF?

It should not be a existing bug in perf. There are other places rely on
non-empty of the list. For example, in parse_events__set_leader(). 
Furtunately,
it won't triggered problem because we don't allow a BPF object to be 
wrapped with "{}"
lexically ("{./aaa.o}" will be interpreterd as file '{./aaa.o' and a 
extra '}').


> - Arnaldo
>   
>> [1]: http://lkml.kernel.org/n/1436445342-1402-19-git-send-email-wangnan0@huawei.com
>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> Cc: Alexei Starovoitov <ast@plumgrid.com>
>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Zefan Li <lizefan@huawei.com>
>> Cc: pi3orama@163.com
>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Link: http://lkml.kernel.org/r/1440742821-44548-3-git-send-email-wangnan0@huawei.com
>> ---
>>   tools/perf/util/parse-events.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index d826e6f..14cd7e3 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -1143,10 +1143,14 @@ int parse_events(struct perf_evlist *evlist, const char *str,
>>   		int entries = data.idx - evlist->nr_entries;
>>   		struct perf_evsel *last;
>>   
>> +		if (!list_empty(&data.list)) {
>> +			last = list_entry(data.list.prev,
>> +					  struct perf_evsel, node);
>> +			last->cmdline_group_boundary = true;
>> +		}
>> +
>>   		perf_evlist__splice_list_tail(evlist, &data.list, entries);
>>   		evlist->nr_groups += data.nr_groups;
>> -		last = perf_evlist__last(evlist);
>> -		last->cmdline_group_boundary = true;
>>   
>>   		return 0;
>>   	}
>> -- 
>> 2.1.0


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

[toc] | [prev] | [next] | [standalone]

#1217294 — [PATCH] perf tools: Don't set leader if parser doesn't collect an evsel

Similar to patch 'perf tools: Don't set cmdline_group_boundary if no
evsel is collected', in case when parser collects no evsel (at this
point it shouldn't happen), parse_events__set_leader() is not safe.

This patch checks list_empty becore calling __perf_evlist__set_leader()
for safty reason.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexei Starovoitov <ast@plumgrid.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Zefan Li <lizefan@huawei.com>
Cc: pi3orama@163.com
---

I'd like to queue this patch into my next pull request. Since it is not
a real bug, it may be dropped.

---
 tools/perf/util/parse-events.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index f2c0317..836d226 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -793,6 +793,9 @@ void parse_events__set_leader(char *name, struct list_head *list)
 {
 	struct perf_evsel *leader;
 
+	if (list_empty(list))
+		return;
+
 	__perf_evlist__set_leader(list);
 	leader = list_entry(list->next, struct perf_evsel, node);
 	leader->group_name = name ? strdup(name) : NULL;
-- 
1.8.3.4

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

[toc] | [prev] | [next] | [standalone]

#1217298 — Re: [PATCH] perf tools: Don't set leader if parser doesn't collect an evsel

On 2015/9/2 10:53, Wang Nan wrote:
> Similar to patch 'perf tools: Don't set cmdline_group_boundary if no
> evsel is collected', in case when parser collects no evsel (at this
> point it shouldn't happen), parse_events__set_leader() is not safe.
>
> This patch checks list_empty becore calling __perf_evlist__set_leader()
> for safty reason.
>
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Alexei Starovoitov <ast@plumgrid.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Zefan Li <lizefan@huawei.com>
> Cc: pi3orama@163.com
> ---
>
> I'd like to queue this patch into my next pull request. Since it is not
> a real bug, it may be dropped.

I think merging this into patch 2/31 should be better. If we decide to 
drop then
only one patch should be considered.

> ---
>   tools/perf/util/parse-events.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index f2c0317..836d226 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -793,6 +793,9 @@ void parse_events__set_leader(char *name, struct list_head *list)
>   {
>   	struct perf_evsel *leader;
>   
> +	if (list_empty(list))
> +		return;
> +
>   	__perf_evlist__set_leader(list);
>   	leader = list_entry(list->next, struct perf_evsel, node);
>   	leader->group_name = name ? strdup(name) : NULL;


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

[toc] | [prev] | [next] | [standalone]

#1217327 — RE: [PATCH] perf tools: Don't set leader if parser doesn't collect an evsel

PiBGcm9tOiBXYW5nIE5hbiBbbWFpbHRvOndhbmduYW4wQGh1YXdlaS5jb21dDQo+IA0KPiBTaW1p
bGFyIHRvIHBhdGNoICdwZXJmIHRvb2xzOiBEb24ndCBzZXQgY21kbGluZV9ncm91cF9ib3VuZGFy
eSBpZiBubw0KPiBldnNlbCBpcyBjb2xsZWN0ZWQnLCBpbiBjYXNlIHdoZW4gcGFyc2VyIGNvbGxl
Y3RzIG5vIGV2c2VsIChhdCB0aGlzDQo+IHBvaW50IGl0IHNob3VsZG4ndCBoYXBwZW4pLCBwYXJz
ZV9ldmVudHNfX3NldF9sZWFkZXIoKSBpcyBub3Qgc2FmZS4NCj4gDQo+IFRoaXMgcGF0Y2ggY2hl
Y2tzIGxpc3RfZW1wdHkgYmVjb3JlIGNhbGxpbmcgX19wZXJmX2V2bGlzdF9fc2V0X2xlYWRlcigp
DQo+IGZvciBzYWZ0eSByZWFzb24uDQo+IA0KPiBTaWduZWQtb2ZmLWJ5OiBXYW5nIE5hbiA8d2Fu
Z25hbjBAaHVhd2VpLmNvbT4NCj4gQ2M6IEFybmFsZG8gQ2FydmFsaG8gZGUgTWVsbyA8YWNtZUBy
ZWRoYXQuY29tPg0KPiBDYzogQWxleGVpIFN0YXJvdm9pdG92IDxhc3RAcGx1bWdyaWQuY29tPg0K
PiBDYzogSmlyaSBPbHNhIDxqb2xzYUBrZXJuZWwub3JnPg0KPiBDYzogTWFzYW1pIEhpcmFtYXRz
dSA8bWFzYW1pLmhpcmFtYXRzdS5wdEBoaXRhY2hpLmNvbT4NCj4gQ2M6IE5hbWh5dW5nIEtpbSA8
bmFtaHl1bmdAa2VybmVsLm9yZz4NCj4gQ2M6IFplZmFuIExpIDxsaXplZmFuQGh1YXdlaS5jb20+
DQo+IENjOiBwaTNvcmFtYUAxNjMuY29tDQo+IC0tLQ0KPiANCj4gSSdkIGxpa2UgdG8gcXVldWUg
dGhpcyBwYXRjaCBpbnRvIG15IG5leHQgcHVsbCByZXF1ZXN0LiBTaW5jZSBpdCBpcyBub3QNCj4g
YSByZWFsIGJ1ZywgaXQgbWF5IGJlIGRyb3BwZWQuDQo+IA0KPiAtLS0NCj4gIHRvb2xzL3BlcmYv
dXRpbC9wYXJzZS1ldmVudHMuYyB8IDMgKysrDQo+ICAxIGZpbGUgY2hhbmdlZCwgMyBpbnNlcnRp
b25zKCspDQo+IA0KPiBkaWZmIC0tZ2l0IGEvdG9vbHMvcGVyZi91dGlsL3BhcnNlLWV2ZW50cy5j
IGIvdG9vbHMvcGVyZi91dGlsL3BhcnNlLWV2ZW50cy5jDQo+IGluZGV4IGYyYzAzMTcuLjgzNmQy
MjYgMTAwNjQ0DQo+IC0tLSBhL3Rvb2xzL3BlcmYvdXRpbC9wYXJzZS1ldmVudHMuYw0KPiArKysg
Yi90b29scy9wZXJmL3V0aWwvcGFyc2UtZXZlbnRzLmMNCj4gQEAgLTc5Myw2ICs3OTMsOSBAQCB2
b2lkIHBhcnNlX2V2ZW50c19fc2V0X2xlYWRlcihjaGFyICpuYW1lLCBzdHJ1Y3QgbGlzdF9oZWFk
ICpsaXN0KQ0KPiAgew0KPiAgCXN0cnVjdCBwZXJmX2V2c2VsICpsZWFkZXI7DQo+IA0KPiArCWlm
IChsaXN0X2VtcHR5KGxpc3QpKQ0KDQpXb3VsZCB3ZSBuZWVkIHRvIHdhcm4vZGVidWcgc29tZXRo
aW5nIGhlcmU/DQoNClRoYW5rIHlvdSwNCg0KPiArCQlyZXR1cm47DQo+ICsNCj4gIAlfX3BlcmZf
ZXZsaXN0X19zZXRfbGVhZGVyKGxpc3QpOw0KPiAgCWxlYWRlciA9IGxpc3RfZW50cnkobGlzdC0+
bmV4dCwgc3RydWN0IHBlcmZfZXZzZWwsIG5vZGUpOw0KPiAgCWxlYWRlci0+Z3JvdXBfbmFtZSA9
IG5hbWUgPyBzdHJkdXAobmFtZSkgOiBOVUxMOw0KPiAtLQ0KPiAxLjguMy40DQoNCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1217334 — Re: [PATCH] perf tools: Don't set leader if parser doesn't collect an evsel

On 2015/9/2 13:57, 平松雅巳 / HIRAMATU，MASAMI wrote:
>> From: Wang Nan [mailto:wangnan0@huawei.com]
>>
>> Similar to patch 'perf tools: Don't set cmdline_group_boundary if no
>> evsel is collected', in case when parser collects no evsel (at this
>> point it shouldn't happen), parse_events__set_leader() is not safe.
>>
>> This patch checks list_empty becore calling __perf_evlist__set_leader()
>> for safty reason.
>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Cc: Alexei Starovoitov <ast@plumgrid.com>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Zefan Li <lizefan@huawei.com>
>> Cc: pi3orama@163.com
>> ---
>>
>> I'd like to queue this patch into my next pull request. Since it is not
>> a real bug, it may be dropped.
>>
>> ---
>>   tools/perf/util/parse-events.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index f2c0317..836d226 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -793,6 +793,9 @@ void parse_events__set_leader(char *name, struct list_head *list)
>>   {
>>   	struct perf_evsel *leader;
>>
>> +	if (list_empty(list))
> Would we need to warn/debug something here?

OK, let's add a WARN message here and other 2 places.

Thank you.

> Thank you,
>
>> +		return;
>> +
>>   	__perf_evlist__set_leader(list);
>>   	leader = list_entry(list->next, struct perf_evsel, node);
>>   	leader->group_name = name ? strdup(name) : NULL;
>> --
>> 1.8.3.4


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

[toc] | [prev] | [next] | [standalone]

#1217358 — Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel

Sorry, forget to CC kernel mailing list...

On 2015/9/2 14:49, Wang Nan wrote:
> If parse_events__scanner() collects no entry, perf_evlist__last(evlist)
> is invalid.
>
> Although it shouldn't happen at this point, before calling
> perf_evlist__last(), we should ensure the list is not empty for safety
> reason.
>
> There are 3 places need this checking:
>
>   1. Before setting cmdline_group_boundary;
>   2. Before __perf_evlist__set_leader();
>   3. In foreach_evsel_in_last_glob.
>
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Alexei Starovoitov <ast@plumgrid.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Zefan Li <lizefan@huawei.com>
> Cc: pi3orama@163.com
> ---
>
> Merge all 3 list_empty() test together into one patch.
>
> Add warning messages.
>
> Improve commit message.
>
> ---
>   tools/perf/util/parse-events.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index d826e6f..069848d 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -793,6 +793,11 @@ void parse_events__set_leader(char *name, struct list_head *list)
>   {
>   	struct perf_evsel *leader;
>   
> +	if (list_empty(list)) {
> +		__WARN_printf("WARNING: failed to set leader: empty list");
> +		return;
> +	}
> +
>   	__perf_evlist__set_leader(list);
>   	leader = list_entry(list->next, struct perf_evsel, node);
>   	leader->group_name = name ? strdup(name) : NULL;
> @@ -1143,10 +1148,15 @@ int parse_events(struct perf_evlist *evlist, const char *str,
>   		int entries = data.idx - evlist->nr_entries;
>   		struct perf_evsel *last;
>   
> +		if (!list_empty(&data.list)) {
> +			last = list_entry(data.list.prev,
> +					  struct perf_evsel, node);
> +			last->cmdline_group_boundary = true;
> +		} else
> +			__WARN_printf("WARNING: event parser found nothing");
> +
>   		perf_evlist__splice_list_tail(evlist, &data.list, entries);
>   		evlist->nr_groups += data.nr_groups;
> -		last = perf_evlist__last(evlist);
> -		last->cmdline_group_boundary = true;
>   
>   		return 0;
>   	}
> @@ -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))
>   		last = perf_evlist__last(evlist);
>   
>   	do {


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

[toc] | [prev] | [next] | [standalone]

#1217504 — RE: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel

PiBGcm9tOiBXYW5nbmFuIChGKSBbbWFpbHRvOndhbmduYW4wQGh1YXdlaS5jb21dDQo+IA0KPiBT
b3JyeSwgZm9yZ2V0IHRvIENDIGtlcm5lbCBtYWlsaW5nIGxpc3QuLi4NCj4gDQo+IE9uIDIwMTUv
OS8yIDE0OjQ5LCBXYW5nIE5hbiB3cm90ZToNCj4gPiBJZiBwYXJzZV9ldmVudHNfX3NjYW5uZXIo
KSBjb2xsZWN0cyBubyBlbnRyeSwgcGVyZl9ldmxpc3RfX2xhc3QoZXZsaXN0KQ0KPiA+IGlzIGlu
dmFsaWQuDQo+ID4NCj4gPiBBbHRob3VnaCBpdCBzaG91bGRuJ3QgaGFwcGVuIGF0IHRoaXMgcG9p
bnQsIGJlZm9yZSBjYWxsaW5nDQo+ID4gcGVyZl9ldmxpc3RfX2xhc3QoKSwgd2Ugc2hvdWxkIGVu
c3VyZSB0aGUgbGlzdCBpcyBub3QgZW1wdHkgZm9yIHNhZmV0eQ0KPiA+IHJlYXNvbi4NCj4gPg0K
PiA+IFRoZXJlIGFyZSAzIHBsYWNlcyBuZWVkIHRoaXMgY2hlY2tpbmc6DQo+ID4NCj4gPiAgIDEu
IEJlZm9yZSBzZXR0aW5nIGNtZGxpbmVfZ3JvdXBfYm91bmRhcnk7DQo+ID4gICAyLiBCZWZvcmUg
X19wZXJmX2V2bGlzdF9fc2V0X2xlYWRlcigpOw0KPiA+ICAgMy4gSW4gZm9yZWFjaF9ldnNlbF9p
bl9sYXN0X2dsb2IuDQo+ID4NCg0KVGhpcyBsb29rcyBPSyB0byBtZS4NCg0KUmV2aWV3ZWQtYnk6
IE1hc2FtaSBIaXJhbWF0c3UgPG1hc2FtaS5oaXJhbWF0c3UucHRAaGl0YWNoaS5jb20+DQoNClRo
YW5rcyENCg0KPiA+IFNpZ25lZC1vZmYtYnk6IFdhbmcgTmFuIDx3YW5nbmFuMEBodWF3ZWkuY29t
Pg0KPiA+IENjOiBBcm5hbGRvIENhcnZhbGhvIGRlIE1lbG8gPGFjbWVAcmVkaGF0LmNvbT4NCj4g
PiBDYzogQWxleGVpIFN0YXJvdm9pdG92IDxhc3RAcGx1bWdyaWQuY29tPg0KPiA+IENjOiBKaXJp
IE9sc2EgPGpvbHNhQGtlcm5lbC5vcmc+DQo+ID4gQ2M6IE1hc2FtaSBIaXJhbWF0c3UgPG1hc2Ft
aS5oaXJhbWF0c3UucHRAaGl0YWNoaS5jb20+DQo+ID4gQ2M6IE5hbWh5dW5nIEtpbSA8bmFtaHl1
bmdAa2VybmVsLm9yZz4NCj4gPiBDYzogWmVmYW4gTGkgPGxpemVmYW5AaHVhd2VpLmNvbT4NCj4g
PiBDYzogcGkzb3JhbWFAMTYzLmNvbQ0KPiA+IC0tLQ0KPiA+DQo+ID4gTWVyZ2UgYWxsIDMgbGlz
dF9lbXB0eSgpIHRlc3QgdG9nZXRoZXIgaW50byBvbmUgcGF0Y2guDQo+ID4NCj4gPiBBZGQgd2Fy
bmluZyBtZXNzYWdlcy4NCj4gPg0KPiA+IEltcHJvdmUgY29tbWl0IG1lc3NhZ2UuDQo+ID4NCj4g
PiAtLS0NCj4gPiAgIHRvb2xzL3BlcmYvdXRpbC9wYXJzZS1ldmVudHMuYyB8IDIyICsrKysrKysr
KysrKysrKysrKystLS0NCj4gPiAgIDEgZmlsZSBjaGFuZ2VkLCAxOSBpbnNlcnRpb25zKCspLCAz
IGRlbGV0aW9ucygtKQ0KPiA+DQo+ID4gZGlmZiAtLWdpdCBhL3Rvb2xzL3BlcmYvdXRpbC9wYXJz
ZS1ldmVudHMuYyBiL3Rvb2xzL3BlcmYvdXRpbC9wYXJzZS1ldmVudHMuYw0KPiA+IGluZGV4IGQ4
MjZlNmYuLjA2OTg0OGQgMTAwNjQ0DQo+ID4gLS0tIGEvdG9vbHMvcGVyZi91dGlsL3BhcnNlLWV2
ZW50cy5jDQo+ID4gKysrIGIvdG9vbHMvcGVyZi91dGlsL3BhcnNlLWV2ZW50cy5jDQo+ID4gQEAg
LTc5Myw2ICs3OTMsMTEgQEAgdm9pZCBwYXJzZV9ldmVudHNfX3NldF9sZWFkZXIoY2hhciAqbmFt
ZSwgc3RydWN0IGxpc3RfaGVhZCAqbGlzdCkNCj4gPiAgIHsNCj4gPiAgIAlzdHJ1Y3QgcGVyZl9l
dnNlbCAqbGVhZGVyOw0KPiA+DQo+ID4gKwlpZiAobGlzdF9lbXB0eShsaXN0KSkgew0KPiA+ICsJ
CV9fV0FSTl9wcmludGYoIldBUk5JTkc6IGZhaWxlZCB0byBzZXQgbGVhZGVyOiBlbXB0eSBsaXN0
Iik7DQo+ID4gKwkJcmV0dXJuOw0KPiA+ICsJfQ0KPiA+ICsNCj4gPiAgIAlfX3BlcmZfZXZsaXN0
X19zZXRfbGVhZGVyKGxpc3QpOw0KPiA+ICAgCWxlYWRlciA9IGxpc3RfZW50cnkobGlzdC0+bmV4
dCwgc3RydWN0IHBlcmZfZXZzZWwsIG5vZGUpOw0KPiA+ICAgCWxlYWRlci0+Z3JvdXBfbmFtZSA9
IG5hbWUgPyBzdHJkdXAobmFtZSkgOiBOVUxMOw0KPiA+IEBAIC0xMTQzLDEwICsxMTQ4LDE1IEBA
IGludCBwYXJzZV9ldmVudHMoc3RydWN0IHBlcmZfZXZsaXN0ICpldmxpc3QsIGNvbnN0IGNoYXIg
KnN0ciwNCj4gPiAgIAkJaW50IGVudHJpZXMgPSBkYXRhLmlkeCAtIGV2bGlzdC0+bnJfZW50cmll
czsNCj4gPiAgIAkJc3RydWN0IHBlcmZfZXZzZWwgKmxhc3Q7DQo+ID4NCj4gPiArCQlpZiAoIWxp
c3RfZW1wdHkoJmRhdGEubGlzdCkpIHsNCj4gPiArCQkJbGFzdCA9IGxpc3RfZW50cnkoZGF0YS5s
aXN0LnByZXYsDQo+ID4gKwkJCQkJICBzdHJ1Y3QgcGVyZl9ldnNlbCwgbm9kZSk7DQo+ID4gKwkJ
CWxhc3QtPmNtZGxpbmVfZ3JvdXBfYm91bmRhcnkgPSB0cnVlOw0KPiA+ICsJCX0gZWxzZQ0KPiA+
ICsJCQlfX1dBUk5fcHJpbnRmKCJXQVJOSU5HOiBldmVudCBwYXJzZXIgZm91bmQgbm90aGluZyIp
Ow0KPiA+ICsNCj4gPiAgIAkJcGVyZl9ldmxpc3RfX3NwbGljZV9saXN0X3RhaWwoZXZsaXN0LCAm
ZGF0YS5saXN0LCBlbnRyaWVzKTsNCj4gPiAgIAkJZXZsaXN0LT5ucl9ncm91cHMgKz0gZGF0YS5u
cl9ncm91cHM7DQo+ID4gLQkJbGFzdCA9IHBlcmZfZXZsaXN0X19sYXN0KGV2bGlzdCk7DQo+ID4g
LQkJbGFzdC0+Y21kbGluZV9ncm91cF9ib3VuZGFyeSA9IHRydWU7DQo+ID4NCj4gPiAgIAkJcmV0
dXJuIDA7DQo+ID4gICAJfQ0KPiA+IEBAIC0xMjUyLDcgKzEyNjIsMTMgQEAgZm9yZWFjaF9ldnNl
bF9pbl9sYXN0X2dsb2Ioc3RydWN0IHBlcmZfZXZsaXN0ICpldmxpc3QsDQo+ID4gICAJc3RydWN0
IHBlcmZfZXZzZWwgKmxhc3QgPSBOVUxMOw0KPiA+ICAgCWludCBlcnI7DQo+ID4NCj4gPiAtCWlm
IChldmxpc3QtPm5yX2VudHJpZXMgPiAwKQ0KPiA+ICsJLyoNCj4gPiArCSAqIERvbid0IHJldHVy
biB3aGVuIGxpc3RfZW1wdHksIGdpdmUgZnVuYyBhIGNoYW5jZSB0byByZXBvcnQNCj4gPiArCSAq
IGVycm9yIHdoZW4gaXQgZm91bmQgbGFzdCA9PSBOVUxMLg0KPiA+ICsJICoNCj4gPiArCSAqIFNv
IG5vIG5lZWQgdG8gV0FSTiBoZXJlLCBsZXQgKmZ1bmMgZG8gdGhpcy4NCj4gPiArCSAqLw0KPiA+
ICsJaWYgKCFsaXN0X2VtcHR5KCZldmxpc3QtPmVudHJpZXMpKQ0KPiA+ICAgCQlsYXN0ID0gcGVy
Zl9ldmxpc3RfX2xhc3QoZXZsaXN0KTsNCj4gPg0KPiA+ICAgCWRvIHsNCj4gDQoNCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1217546 — Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel

On Wed, Sep 02, 2015 at 02:53:58PM +0800, Wangnan (F) wrote:
> Sorry, forget to CC kernel mailing list...
> 
> On 2015/9/2 14:49, Wang Nan wrote:
> >If parse_events__scanner() collects no entry, perf_evlist__last(evlist)
> >is invalid.
> >
> >Although it shouldn't happen at this point, before calling
> >perf_evlist__last(), we should ensure the list is not empty for safety
> >reason.
> >
> >There are 3 places need this checking:
> >
> >  1. Before setting cmdline_group_boundary;
> >  2. Before __perf_evlist__set_leader();
> >  3. In foreach_evsel_in_last_glob.
> >
> >Signed-off-by: Wang Nan <wangnan0@huawei.com>
> >Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> >Cc: Alexei Starovoitov <ast@plumgrid.com>
> >Cc: Jiri Olsa <jolsa@kernel.org>
> >Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> >Cc: Namhyung Kim <namhyung@kernel.org>
> >Cc: Zefan Li <lizefan@huawei.com>
> >Cc: pi3orama@163.com
> >---
> >
> >Merge all 3 list_empty() test together into one patch.
> >
> >Add warning messages.
> >
> >Improve commit message.
> >
> >---
> >  tools/perf/util/parse-events.c | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> >
> >diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> >index d826e6f..069848d 100644
> >--- a/tools/perf/util/parse-events.c
> >+++ b/tools/perf/util/parse-events.c
> >@@ -793,6 +793,11 @@ void parse_events__set_leader(char *name, struct list_head *list)
> >  {
> >  	struct perf_evsel *leader;
> >+	if (list_empty(list)) {
> >+		__WARN_printf("WARNING: failed to set leader: empty list");
> >+		return;
> >+	}
> >+
> >  	__perf_evlist__set_leader(list);
> >  	leader = list_entry(list->next, struct perf_evsel, node);
> >  	leader->group_name = name ? strdup(name) : NULL;
> >@@ -1143,10 +1148,15 @@ int parse_events(struct perf_evlist *evlist, const char *str,
> >  		int entries = data.idx - evlist->nr_entries;
> >  		struct perf_evsel *last;
> >+		if (!list_empty(&data.list)) {
> >+			last = list_entry(data.list.prev,
> >+					  struct perf_evsel, node);
> >+			last->cmdline_group_boundary = true;
> >+		} else
> >+			__WARN_printf("WARNING: event parser found nothing");

we need to unify error printing in this object ;-) with this one it's 3

__WARN_printf(...
fprintf(stderr,...
printf(...
WARN_ONCE(...

;-)


> >+
> >  		perf_evlist__splice_list_tail(evlist, &data.list, entries);
> >  		evlist->nr_groups += data.nr_groups;
> >-		last = perf_evlist__last(evlist);
> >-		last->cmdline_group_boundary = true;
> >  		return 0;
> >  	}
> >@@ -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?


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

[toc] | [prev] | [next] | [standalone]

#1217548 — Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel

发自我的 iPhone

> 在 2015年9月2日，下午7:54，Jiri Olsa <jolsa@redhat.com> 写道：
> 
>> On Wed, Sep 02, 2015 at 02:53:58PM +0800, Wangnan (F) wrote:
>> Sorry, forget to CC kernel mailing list...
>> 
>>> On 2015/9/2 14:49, Wang Nan wrote:
>>> If parse_events__scanner() collects no entry, perf_evlist__last(evlist)
>>> is invalid.
>>> 
>>> Although it shouldn't happen at this point, before calling
>>> perf_evlist__last(), we should ensure the list is not empty for safety
>>> reason.
>>> 
>>> There are 3 places need this checking:
>>> 
>>> 1. Before setting cmdline_group_boundary;
>>> 2. Before __perf_evlist__set_leader();
>>> 3. In foreach_evsel_in_last_glob.
>>> 
>>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>>> Cc: Alexei Starovoitov <ast@plumgrid.com>
>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>> Cc: Zefan Li <lizefan@huawei.com>
>>> Cc: pi3orama@163.com
>>> ---
>>> 
>>> Merge all 3 list_empty() test together into one patch.
>>> 
>>> Add warning messages.
>>> 
>>> Improve commit message.
>>> 
>>> ---
>>> tools/perf/util/parse-events.c | 22 +++++++++++++++++++---
>>> 1 file changed, 19 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>>> index d826e6f..069848d 100644
>>> --- a/tools/perf/util/parse-events.c
>>> +++ b/tools/perf/util/parse-events.c
>>> @@ -793,6 +793,11 @@ void parse_events__set_leader(char *name, struct list_head *list)
>>> {
>>>    struct perf_evsel *leader;
>>> +    if (list_empty(list)) {
>>> +        __WARN_printf("WARNING: failed to set leader: empty list");
>>> +        return;
>>> +    }
>>> +
>>>    __perf_evlist__set_leader(list);
>>>    leader = list_entry(list->next, struct perf_evsel, node);
>>>    leader->group_name = name ? strdup(name) : NULL;
>>> @@ -1143,10 +1148,15 @@ int parse_events(struct perf_evlist *evlist, const char *str,
>>>        int entries = data.idx - evlist->nr_entries;
>>>        struct perf_evsel *last;
>>> +        if (!list_empty(&data.list)) {
>>> +            last = list_entry(data.list.prev,
>>> +                      struct perf_evsel, node);
>>> +            last->cmdline_group_boundary = true;
>>> +        } else
>>> +            __WARN_printf("WARNING: event parser found nothing");
> 
> we need to unify error printing in this object ;-) with this one it's 3
> 
> __WARN_printf(...
> fprintf(stderr,...
> printf(...
> WARN_ONCE(...
> 
> ;-)
> 
> 
>>> +
>>>        perf_evlist__splice_list_tail(evlist, &data.list, entries);
>>>        evlist->nr_groups += data.nr_groups;
>>> -        last = perf_evlist__last(evlist);
>>> -        last->cmdline_group_boundary = true;
>>>        return 0;
>>>    }
>>> @@ -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.
Don't you think so?

At this point they are equivalent, but the whole patch is preventive action.

Thank you.

> 
> jirka

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

[toc] | [prev] | [next] | [standalone]

#1217574 — Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel

On Wed, Sep 02, 2015 at 08:05:54PM +0800, pi3orama wrote:

SNIP

> >>>        perf_evlist__splice_list_tail(evlist, &data.list, entries);
> >>>        evlist->nr_groups += data.nr_groups;
> >>> -        last = perf_evlist__last(evlist);
> >>> -        last->cmdline_group_boundary = true;
> >>>        return 0;
> >>>    }
> >>> @@ -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.
> Don't you think so?
> 
> At this point they are equivalent, but the whole patch is preventive action.

ok, fair enough ;-)

Acked-by: Jiri Olsa <jolsa@kernel.org>

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

[toc] | [prev] | [next] | [standalone]

#1217613 — Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel

Em Wed, Sep 02, 2015 at 08:05:54PM +0800, pi3orama escreveu:
> 发自我的 iPhone
> > 在 2015年9月2日，下午7:54，Jiri Olsa <jolsa@redhat.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.

- Arnaldo

> Don't you think so?
> 
> At this point they are equivalent, but the whole patch is preventive action.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1217618 — Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel

发自我的 iPhone

> 在 2015年9月2日，下午9:55，Arnaldo Carvalho de Melo <acme@redhat.com> 写道：
> 
> Em Wed, Sep 02, 2015 at 08:05:54PM +0800, pi3orama escreveu:
>> 发自我的 iPhone
>>> 在 2015年9月2日，下午7:54，Jiri Olsa <jolsa@redhat.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. 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.

Thank you.

> 
> - Arnaldo
> 
>> Don't you think so?
>> 
>> At this point they are equivalent, but the whole patch is preventive action.

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

[toc] | [prev] | [next] | [standalone]

#1217663 — Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel

Em Wed, Sep 02, 2015 at 10:04:21PM +0800, pi3orama escreveu:
> 发自我的 iPhone
> > 在 2015年9月2日，下午9:55，Arnaldo Carvalho de Melo <acme@redhat.com> 写道：
> > Em Wed, Sep 02, 2015 at 08:05:54PM +0800, pi3orama escreveu:
> >> 发自我的 iPhone
> >>> 在 2015年9月2日，下午7:54，Jiri Olsa <jolsa@redhat.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) :-)

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

[toc] | [prev] | [next] | [standalone]

#1217880 — 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@redhat.com> 写道：
> 
> Em Wed, Sep 02, 2015 at 10:04:21PM +0800, pi3orama escreveu:
>> 发自我的 iPhone
>>> 在 2015年9月2日，下午9:55，Arnaldo Carvalho de Melo <acme@redhat.com> 写道：
>>> Em Wed, Sep 02, 2015 at 08:05:54PM +0800, pi3orama escreveu:
>>>> 发自我的 iPhone
>>>>> 在 2015年9月2日，下午7:54，Jiri Olsa <jolsa@redhat.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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web