From: Namhyung Kim <namhyung@kernel.org>
To: Adrian Hunter <adrian.hunter@intel.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Ian Rogers <irogers@google.com>, Jiri Olsa <jolsa@kernel.org>,
Ingo Molnar <mingo@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
LKML <linux-kernel@vger.kernel.org>,
linux-perf-users <linux-perf-users@vger.kernel.org>,
Kan Liang <kan.liang@linux.intel.com>,
Leo Yan <leo.yan@linaro.org>
Subject: Re: [PATCH 2/5] libperf: Propagate maps only if necessary
Date: Fri, 30 Sep 2022 09:44:49 -0700 [thread overview]
Message-ID: <CAM9d7chuK=QqmM=YVFZa9fO6RG7o2gJSrrTMZhegj3sFdLD3mQ@mail.gmail.com> (raw)
In-Reply-To: <37452f10-365d-f496-3cb2-b80481f5f079@intel.com>
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?
Namhyung
next prev parent reply other threads:[~2022-09-30 16:45 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-24 16:57 [PATCH 0/5] perf tools: Clean up cpu map handling for a system-wide evsel (v1) Namhyung Kim
2022-09-24 16:57 ` [PATCH 1/5] libperf: Populate system-wide evsel maps Namhyung Kim
2022-09-27 6:56 ` Adrian Hunter
2022-09-24 16:57 ` [PATCH 2/5] libperf: Propagate maps only if necessary Namhyung Kim
2022-09-27 7:06 ` Adrian Hunter
2022-09-27 17:28 ` Namhyung Kim
2022-09-28 7:53 ` Adrian Hunter
2022-09-28 23:46 ` Namhyung Kim
2022-09-29 2:07 ` Ian Rogers
2022-09-29 5:09 ` Namhyung Kim
2022-09-29 5:18 ` Adrian Hunter
2022-09-29 20:42 ` Namhyung Kim
2022-09-30 12:49 ` Adrian Hunter
2022-09-30 16:44 ` Namhyung Kim [this message]
2022-09-30 16:56 ` Arnaldo Carvalho de Melo
2022-09-24 16:57 ` [PATCH 3/5] perf tools: Get rid of evlist__add_on_all_cpus() Namhyung Kim
2022-09-27 7:07 ` Adrian Hunter
2022-09-24 16:57 ` [PATCH 4/5] perf tools: Add evlist__add_sched_switch() Namhyung Kim
2022-09-27 7:07 ` Adrian Hunter
2022-09-24 16:57 ` [PATCH 5/5] perf tools: Remove special handling of system-wide evsel Namhyung Kim
2022-09-27 7:09 ` Adrian Hunter
2022-09-26 19:24 ` [PATCH 0/5] perf tools: Clean up cpu map handling for a system-wide evsel (v1) Arnaldo Carvalho de Melo
2022-09-30 17:27 [PATCH 0/5] perf tools: Clean up cpu map handling for system-wide evsel (v2) Namhyung Kim
2022-09-30 17:27 ` [PATCH 2/5] libperf: Propagate maps only if necessary Namhyung Kim
2022-10-03 5:36 ` Adrian Hunter
2022-10-04 5:20 ` Adrian Hunter
2022-10-03 20:46 [PATCH 0/5] perf tools: Clean up cpu map handling for system-wide evsel (v3) Namhyung Kim
2022-10-03 20:46 ` [PATCH 2/5] libperf: Propagate maps only if necessary Namhyung Kim
2022-10-04 12:14 ` Arnaldo Carvalho de Melo
2022-10-04 13:55 ` Adrian Hunter
2022-10-06 18:52 ` Ian Rogers
2022-10-06 23:21 ` Namhyung Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAM9d7chuK=QqmM=YVFZa9fO6RG7o2gJSrrTMZhegj3sFdLD3mQ@mail.gmail.com' \
--to=namhyung@kernel.org \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=leo.yan@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).