Re: [PATCH 04/11] perf parse-event: Fix cpu map leaks

From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>, Ingo Molnar <mingo@kernel.org>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Stephane Eranian <eranian@google.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Andi Kleen <ak@linux.intel.com>, Ian Rogers <irogers@google.com>
Subject: Re: [PATCH 04/11] perf parse-event: Fix cpu map leaks
Date: Tue, 15 Sep 2020 13:51:56 -0300	[thread overview]
Message-ID: <20200915165156.GL720847@kernel.org> (raw)
In-Reply-To: <CAM9d7chORRmiE2R4LnzGh0uS2nKqL_tk+qxV2TfeqZctvCsJ5w@mail.gmail.com>

Em Tue, Sep 15, 2020 at 11:39:56PM +0900, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Tue, Sep 15, 2020 at 9:18 PM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Tue, Sep 15, 2020 at 12:18:12PM +0900, Namhyung Kim escreveu:
> > > Like evlist cpu map, evsel's cpu map should have proper refcount by
> > > releasing the original count after creation.
> >
> > "releasing original count after creation"?
> >
> > There are two fixes here, one its legit, i.e. when failing to create
> > the new evsel, if you created the perf_cpu_map, drop the refcount,
> > which, in this case, will free it since perf_cpu_map__new() sets it to
> > 1.
> >
> > But what about the other? Humm, I see, since a new refcount is being
> > obtained, then we need to drop the first.
> >
> > This all got complicated, perhaps the following patch is simpler?
> >
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index c4d2394e2b2dc60f..3dceeacf8669bc5d 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -353,18 +353,20 @@ __add_event(struct list_head *list, int *idx,
> >             const char *cpu_list)
> >  {
> >         struct evsel *evsel;
> > -       struct perf_cpu_map *cpus = pmu ? pmu->cpus :
> > +       struct perf_cpu_map *cpus = pmu ? perf_cpu_map__get(pmu->cpus) :
> >                                cpu_list ? perf_cpu_map__new(cpu_list) : NULL;
> >
> >         if (init_attr)
> >                 event_attr_init(attr);
> >
> >         evsel = evsel__new_idx(attr, *idx);
> > -       if (!evsel)
> > +       if (!evsel) {
> > +               perf_cpu_map__put(cpus);
> >                 return NULL;
> > +       }
> >
> >         (*idx)++;
> > -       evsel->core.cpus   = perf_cpu_map__get(cpus);
> > +       evsel->core.cpus     = cpus;
> >         evsel->core.own_cpus = perf_cpu_map__get(cpus);
> >         evsel->core.system_wide = pmu ? pmu->is_uncore : false;
> >         evsel->auto_merge_stats = auto_merge_stats;
> >
> >
> > ---
> >
> > I.e. if we're going to share pmu->cpus, grab the necessary refcount at
> > that point, if we're going to create one (pmu == NULL), then
> > perf_cpu_map__new() will have the refcount we need (will set it to 1).
> >
> > Then, if we fail to create the new evsel, we just drop the refcount we
> > got either from perf_cpu_map__get(pmu->cpus) or from
> > perf_cpu_map__new(cpu_list) (NULL is ok for __put() operations, that
> > covers that last ': NULL').
> >
> > And then, when we go make evsel->core.cpus share that cpu_map, we know
> > we have the necessary refcount already, right?
> 
> Indeed! This looks a lot better.  Do you want me to resend?

Please, feel free to use whatever snippets from my explanations.

But please consider breaking it into two patches, without thinking too
much about it at this time, it seems there are two fixes to be done in
this case.

- Arnaldo