Re: [PATCH v6 1/7] perf evlist: Add perf_evlist__go_system_wide() helper

[Yang Jihong]

Hello,

On 2023/8/25 13:45, Ian Rogers wrote:
> On Thu, Aug 24, 2023, 10:41 PM Yang Jihong <yangjihong1@xxxxxxxxxx 
> <mailto:yangjihong1@xxxxxxxxxx>> wrote:
>
>     Hello,
>
>     On 2023/8/25 12:51, Ian Rogers wrote:
>      > On Sun, Aug 20, 2023 at 6:30 PM Yang Jihong
>     <yangjihong1@xxxxxxxxxx <mailto:yangjihong1@xxxxxxxxxx>> wrote:
>      >>
>      >> For dummy events that keep tracking, we may need to modify its
>     cpu_maps.
>      >> For example, change the cpu_maps to record sideband events for
>     all CPUS.
>      >> Add perf_evlist__go_system_wide() helper to support this scenario.
>      >>
>      >> Signed-off-by: Yang Jihong <yangjihong1@xxxxxxxxxx
>     <mailto:yangjihong1@xxxxxxxxxx>>
>      >> Acked-by: Adrian Hunter <adrian.hunter@xxxxxxxxx
>     <mailto:adrian.hunter@xxxxxxxxx>>
>      >> ---
>      >>   tools/lib/perf/evlist.c                  | 9 +++++++++
>      >>   tools/lib/perf/include/internal/evlist.h | 2 ++
>      >>   2 files changed, 11 insertions(+)
>      >>
>      >> diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
>      >> index b8b066d0dc5e..3acbbccc1901 100644
>      >> --- a/tools/lib/perf/evlist.c
>      >> +++ b/tools/lib/perf/evlist.c
>      >> @@ -738,3 +738,12 @@ int perf_evlist__nr_groups(struct
>     perf_evlist *evlist)
>      >>          }
>      >>          return nr_groups;
>      >>   }
>      >> +
>      >> +void perf_evlist__go_system_wide(struct perf_evlist *evlist,
>     struct perf_evsel *evsel)
>      >> +{
>      >> +       if (!evsel->system_wide) {
>      >> +               evsel->system_wide = true;
>      >> +               if (evlist->needs_map_propagation)
>      >> +                       __perf_evlist__propagate_maps(evlist,
>     evsel);
>      >> +       }
>      >> +}
>      >
>      > I think this should be:
>      >
>      > void evsel__set_system_wide(struct evsel *evsel)
>      > {
>      >          if (evsel->system_wide)
>      >                 return;
>      >          evsel->system_wide = true;
>      >          if (evsel->evlist->core.needs_map_propagation)
>      > ...
>      >
>      > The API being on evlist makes it look like all evsels are affected.
>      >
>     This part of the code is implemented according to Adrian's suggestion.
>     Refer to:
>
>     https://lore.kernel.org/linux-perf-users/206972a3-d44d-1c75-3fbc-426427614543@xxxxxxxxx/
>     <https://lore.kernel.org/linux-perf-users/206972a3-d44d-1c75-3fbc-426427614543@xxxxxxxxx/>
>
>     The logic of both is the same, and either is OK for me.
>     If really want to change it, please let me know.
>
>
> Yes, I think the naming isn't correct and the function being on evlist 
> is misleading.
Uh, I have a little problem here, too.

Because perf_evlist__go_system_wide() needs to invoke the 
__perf_evlist__propagate_maps(), which is a local function and is 
located in the evlist.c file in tools/lib/perf/.
So perf_evlist__go_system_wide() can only be located in this file. The 
prefixes of all funcstions in this file are "perf_evlist__". Therefore, 
it is better to use the original names.

In addition, __perf_evlist__propagate_maps affects the evlist, so it is 
not misleading.

Thanks,
Yang