[v1] perf evlist: Avoid iteration for empty evlist.

[PATCH] perf evlist: Avoid iteration for empty evlist.

Posted by Ian Rogers 4 years, 3 months ago

As seen with 'perf stat --null ..' and reported in:
https://lore.kernel.org/lkml/YjCLcpcX2peeQVCH@kernel.org/

Fixes: 472832d2c000 ("perf evlist: Refactor evlist__for_each_cpu()")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evlist.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 8134d45e2164..a2dba9e00765 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -354,7 +354,10 @@ struct evlist_cpu_iterator evlist__cpu_begin(struct evlist *evlist, struct affin
 		.affinity = affinity,
 	};
 
-	if (itr.affinity) {
+	if (evlist__empty(evlist)) {
+		/* Ensure the empty list doesn't iterate. */
+		itr.evlist_cpu_map_idx = itr.evlist_cpu_map_nr;
+	} else if (itr.affinity) {
 		itr.cpu = perf_cpu_map__cpu(evlist->core.all_cpus, 0);
 		affinity__set(itr.affinity, itr.cpu.cpu);
 		itr.cpu_map_idx = perf_cpu_map__idx(itr.evsel->core.cpus, itr.cpu);
-- 
2.35.1.723.g4982287a31-goog

Re: [PATCH] perf evlist: Avoid iteration for empty evlist.

Posted by Jiri Olsa 4 years, 3 months ago

On Wed, Mar 16, 2022 at 12:10:49AM -0700, Ian Rogers wrote:
> As seen with 'perf stat --null ..' and reported in:
> https://lore.kernel.org/lkml/YjCLcpcX2peeQVCH@kernel.org/
> 
> Fixes: 472832d2c000 ("perf evlist: Refactor evlist__for_each_cpu()")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/evlist.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 8134d45e2164..a2dba9e00765 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -354,7 +354,10 @@ struct evlist_cpu_iterator evlist__cpu_begin(struct evlist *evlist, struct affin
>  		.affinity = affinity,
>  	};
>  
> -	if (itr.affinity) {
> +	if (evlist__empty(evlist)) {
> +		/* Ensure the empty list doesn't iterate. */
> +		itr.evlist_cpu_map_idx = itr.evlist_cpu_map_nr;

I can't see the crash anymore, but I'm bit confused with the code

if evlist is empty then itr.evsel is bogus.. and the loop code
__run_perf_stat is just lucky, right?

I think we need to set itr.evsel to NULL and skip the loop in
case evlist is empty

thanks,
jirka

> +	} else if (itr.affinity) {
>  		itr.cpu = perf_cpu_map__cpu(evlist->core.all_cpus, 0);
>  		affinity__set(itr.affinity, itr.cpu.cpu);
>  		itr.cpu_map_idx = perf_cpu_map__idx(itr.evsel->core.cpus, itr.cpu);
> -- 
> 2.35.1.723.g4982287a31-goog
>

Re: [PATCH] perf evlist: Avoid iteration for empty evlist.

Posted by Ian Rogers 4 years, 3 months ago

On Wed, Mar 16, 2022 at 7:08 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Mar 16, 2022 at 12:10:49AM -0700, Ian Rogers wrote:
> > As seen with 'perf stat --null ..' and reported in:
> > https://lore.kernel.org/lkml/YjCLcpcX2peeQVCH@kernel.org/
> >
> > Fixes: 472832d2c000 ("perf evlist: Refactor evlist__for_each_cpu()")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/evlist.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index 8134d45e2164..a2dba9e00765 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -354,7 +354,10 @@ struct evlist_cpu_iterator evlist__cpu_begin(struct evlist *evlist, struct affin
> >               .affinity = affinity,
> >       };
> >
> > -     if (itr.affinity) {
> > +     if (evlist__empty(evlist)) {
> > +             /* Ensure the empty list doesn't iterate. */
> > +             itr.evlist_cpu_map_idx = itr.evlist_cpu_map_nr;
>
> I can't see the crash anymore, but I'm bit confused with the code
>
> if evlist is empty then itr.evsel is bogus.. and the loop code
> __run_perf_stat is just lucky, right?

The itr.evsel is the list head, so bogus.

> I think we need to set itr.evsel to NULL and skip the loop in
> case evlist is empty

So that's the effect of this change except that the evsel is the list
head. I'm not sure it is worth adding the condition to setting the
evsel to capture that given that with the end condition having been
met it would be invalid to read it. There's a similar problem for the
other fields of the iterator.

Thanks,
Ian

> thanks,
> jirka
>
> > +     } else if (itr.affinity) {
> >               itr.cpu = perf_cpu_map__cpu(evlist->core.all_cpus, 0);
> >               affinity__set(itr.affinity, itr.cpu.cpu);
> >               itr.cpu_map_idx = perf_cpu_map__idx(itr.evsel->core.cpus, itr.cpu);
> > --
> > 2.35.1.723.g4982287a31-goog
> >

Re: [PATCH] perf evlist: Avoid iteration for empty evlist.

Posted by Jiri Olsa 4 years, 3 months ago

On Wed, Mar 16, 2022 at 01:16:30PM -0700, Ian Rogers wrote:
> On Wed, Mar 16, 2022 at 7:08 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Wed, Mar 16, 2022 at 12:10:49AM -0700, Ian Rogers wrote:
> > > As seen with 'perf stat --null ..' and reported in:
> > > https://lore.kernel.org/lkml/YjCLcpcX2peeQVCH@kernel.org/
> > >
> > > Fixes: 472832d2c000 ("perf evlist: Refactor evlist__for_each_cpu()")
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/util/evlist.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > > index 8134d45e2164..a2dba9e00765 100644
> > > --- a/tools/perf/util/evlist.c
> > > +++ b/tools/perf/util/evlist.c
> > > @@ -354,7 +354,10 @@ struct evlist_cpu_iterator evlist__cpu_begin(struct evlist *evlist, struct affin
> > >               .affinity = affinity,
> > >       };
> > >
> > > -     if (itr.affinity) {
> > > +     if (evlist__empty(evlist)) {
> > > +             /* Ensure the empty list doesn't iterate. */
> > > +             itr.evlist_cpu_map_idx = itr.evlist_cpu_map_nr;
> >
> > I can't see the crash anymore, but I'm bit confused with the code
> >
> > if evlist is empty then itr.evsel is bogus.. and the loop code
> > __run_perf_stat is just lucky, right?
> 
> The itr.evsel is the list head, so bogus.
> 
> > I think we need to set itr.evsel to NULL and skip the loop in
> > case evlist is empty
> 
> So that's the effect of this change except that the evsel is the list
> head. I'm not sure it is worth adding the condition to setting the
> evsel to capture that given that with the end condition having been
> met it would be invalid to read it. There's a similar problem for the
> other fields of the iterator.

ah ok, I got it.. but it seems to be just evsel that's wrong,
just as a precaution, could we set it only when it's valid?
with something like below

thanks,
jirka


---
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index a2dba9e00765..76a6c70c3c40 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -346,7 +346,7 @@ struct evlist_cpu_iterator evlist__cpu_begin(struct evlist *evlist, struct affin
 {
 	struct evlist_cpu_iterator itr = {
 		.container = evlist,
-		.evsel = evlist__first(evlist),
+		.evsel = evlist__empty(evlist) ? NULL : evlist__first(evlist),
 		.cpu_map_idx = 0,
 		.evlist_cpu_map_idx = 0,
 		.evlist_cpu_map_nr = perf_cpu_map__nr(evlist->core.all_cpus),