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/[email protected]/
>
> Fixes: 472832d2c000 ("perf evlist: Refactor evlist__for_each_cpu()")
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> 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
>
On Wed, Mar 16, 2022 at 7:08 AM Jiri Olsa <[email protected]> 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/[email protected]/
> >
> > Fixes: 472832d2c000 ("perf evlist: Refactor evlist__for_each_cpu()")
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > 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
> >
On Wed, Mar 16, 2022 at 01:16:30PM -0700, Ian Rogers wrote:
> On Wed, Mar 16, 2022 at 7:08 AM Jiri Olsa <[email protected]> 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/[email protected]/
> > >
> > > Fixes: 472832d2c000 ("perf evlist: Refactor evlist__for_each_cpu()")
> > > Signed-off-by: Ian Rogers <[email protected]>
> > > ---
> > > 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),