Messages in this thread | | | Date | Fri, 30 Sep 2022 13:56:05 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 2/5] libperf: Propagate maps only if necessary |
| |
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
| |