Re: [PATCH 2/5] libperf: Propagate maps only if necessary

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Fri, Sep 30, 2022 at 09:44:49AM -0700, Namhyung Kim escreveu:
> On Fri, Sep 30, 2022 at 5:50 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >
> > On 29/09/22 23:42, Namhyung Kim wrote:
> > > On Wed, Sep 28, 2022 at 10:19 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > >>
> > >> On 29/09/22 08:09, Namhyung Kim wrote:
> > >>> On Wed, Sep 28, 2022 at 7:08 PM Ian Rogers <irogers@google.com> wrote:
> > >>>>
> > >>>> On Wed, Sep 28, 2022 at 4:46 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >>>>>
> > >>>>> On Wed, Sep 28, 2022 at 12:54 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > >>>>>>
> > >>>>>> On 27/09/22 20:28, Namhyung Kim wrote:
> > >>>>>>> Hi Adrian,
> > >>>>>>>
> > >>>>>>> On Tue, Sep 27, 2022 at 12:06 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > >>>>>>>>
> > >>>>>>>> On 24/09/22 19:57, Namhyung Kim wrote:
> > >>>>>>>>> The current code propagate evsel's cpu map settings to evlist when it's
> > >>>>>>>>> added to an evlist.  But the evlist->all_cpus and each evsel's cpus will
> > >>>>>>>>> be updated in perf_evlist__set_maps() later.  No need to do it before
> > >>>>>>>>> evlist's cpus are set actually.
> > >>>>>>>>>
> > >>>>>>>>> Actually we discarded this intermediate all_cpus maps at the beginning
> > >>>>>>>>> of perf_evlist__set_maps().  Let's not do this.  It's only needed when
> > >>>>>>>>> an evsel is added after the evlist cpu maps are set.
> > >>>>>>>>
> > >>>>>>>> That might not be true.  Consider evlist__fix_hybrid_cpus() which fiddles
> > >>>>>>>> with evsel->core.cpus and evsel->core.own_cpus after the evsel has been
> > >>>>>>>> added to the evlist.  It can also remove an evsel from the evlist.
> > >>>>>>>
> > >>>>>>> Thanks for your review.  I think it's fine to change evsel cpus or to remove
> > >>>>>>> an evsel from evlist before calling evlist__create_maps().  The function
> > >>>>>>> will take care of setting evlist's all_cpus from the evsels in the evlist.
> > >>>>>>> So previous changes in evsel/cpus wouldn't be any special.
> > >>>>>>>
> > >>>>>>> After this point, adding a new evsel needs to update evlist all cpus by
> > >>>>>>> propagating cpu maps.  So I think hybrid cpus should be fine.
> > >>>>>>> Did I miss something?
> > >>>>>>
> > >>>>>> I wondered how it might play out if evlist__fix_hybrid_cpus() reduced the
> > >>>>>> cpus from the target->cpu_list (using perf record -C) , since after this
> > >>>>>> patch all_cpus always starts with the target->cpu_list instead of an empty
> > >>>>>> list.  But then, in the hybrid case, it puts a dummy event that uses the
> > >>>>>> target cpu list anyway, so the result is the same.
> > >>>>>>
> > >>>>>> I don't know if there are any cases where all_cpus would actually need to
> > >>>>>> exclude some of the cpus from target->cpu_list.
> > >>>>>
> > >>>>> I'm not aware of other cases to reduce cpu list.  I think it'd be fine
> > >>>>> if it has a cpu in the evlist->all_cpus even if it's not used.  The evsel
> > >>>>> should have a correct list anyway and we mostly use the evsel cpus
> > >>>>> to do the real work.
> > >>>>>
> > >>>>> Thanks,
> > >>>>> Namhyung
> > >>>>
> > >>>> The affinity changes made it so that we use all_cpus probably more
> > >>>> often than the evsel CPU maps for real work. The reason being we want
> > >>>> to avoid IPIs so we do all the work on 1 CPU and then move to the next
> > >>>> CPU in evlist all_cpus. evsel CPU maps are used to make sure the
> > >>>> indices are kept accurate - for example, if an uncore event is
> > >>>> measured with a CPU event:
> > >>>> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/evlist.h?h=perf/core#n366
> > >>>> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/evlist.c?h=perf/core#n404
> > >>>
> > >>> Right, I meant it'd check the evsel cpus eventually even if it iterates
> > >>> on the evlist all_cpus.  The evlist_cpu_iterator__next() will skip a
> > >>> CPU if it's not in the evsel cpus.
> > >>>
> > >>> Thanks,
> > >>> Namhyung
> > >>
> > >> Perhaps an alternative is to be explicit about deferring map
> > >> propagation e.g.
> > >
> > > Thanks for your patch.  Yeah, we can use this.
> > >
> > > But I still think it'd be better doing it unconditionally
> > > since any propagation before perf_evlist__set_maps
> > > will be discarded anyway.  With this change, other
> > > than perf record will collect all cpus before _set_maps
> > > and then discard it.  It seems like a waste, no?
> > >
> > > Or else, we can have allow_map_propagation initialized
> > > to false and set it to true in perf_evlist__set_maps().
> > >
> >
> > That sounds fine.
> 
> Thanks!
> 
> Arnaldo, how do you want to handle it?  I can send v2 for the
> whole series, but I think you already applied it.  Then do you
> want me to send this change on top?

Send v2 for the whole series, I haven't yet published it so I can
replace.

- Arnaldo