Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220316071049.2368250-1-irogers@google.com> <YjHvB4q7VZyKsIUU@krava>
In-Reply-To: <YjHvB4q7VZyKsIUU@krava>
From:   Ian Rogers <irogers@google.com>
Date:   Wed, 16 Mar 2022 13:16:30 -0700
Message-ID: <CAP-5=fUkgp0G+Hj8fnO0RdMdbcKMesEHHE5SgS5OPpj_iW_D9w@mail.gmail.com>
Subject: Re: [PATCH] perf evlist: Avoid iteration for empty evlist.
To:     Jiri Olsa <olsajiri@gmail.com>
Cc:     Namhyung Kim <namhyung@kernel.org>, Jiri Olsa <jolsa@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
> >