| Message ID | 1441162427-114014-1-git-send-email-wangnan0@huawei.com |
|---|---|
| State | New |
| Headers | show |
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/
> 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? 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
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/
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/
> From: Wangnan (F) [mailto:wangnan0@huawei.com] > > 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. > > This looks OK to me. Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> Thanks! > > 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 { >
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/
发自我的 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/
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/
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/
发自我的 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/
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/
发自我的 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/
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;
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(+)