linux-kernel - Re: [PATCH v6 1/7] perf evlist: Add perf_evlist__go_system

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <49d6639e-66ac-16e9-8a54-4b0279f808ce@huawei.com>
Date:   Fri, 25 Aug 2023 14:28:52 +0800
From:   Yang Jihong <yangjihong1@...wei.com>
To:     Ian Rogers <irogers@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>
CC:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        "Liang, Kan" <kan.liang@...ux.intel.com>,
        James Clark <james.clark@....com>,
        Thomas Richter <tmricht@...ux.ibm.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Anshuman Khandual <anshuman.khandual@....com>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-perf-users <linux-perf-users@...r.kernel.org>
Subject: Re: [PATCH v6 1/7] perf evlist: Add perf_evlist__go_system_wide()
 helper

Hello,

On 2023/8/25 13:45, Ian Rogers wrote:
> 
> 
> On Thu, Aug 24, 2023, 10:41 PM Yang Jihong <yangjihong1@...wei.com 
> <mailto:yangjihong1@...wei.com>> wrote:
> 
>     Hello,
> 
>     On 2023/8/25 12:51, Ian Rogers wrote:
>      > On Sun, Aug 20, 2023 at 6:30 PM Yang Jihong
>     <yangjihong1@...wei.com <mailto:yangjihong1@...wei.com>> 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@...wei.com
>     <mailto:yangjihong1@...wei.com>>
>      >> Acked-by: Adrian Hunter <adrian.hunter@...el.com
>     <mailto:adrian.hunter@...el.com>>
>      >> ---
>      >>   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@intel.com/
>     <https://lore.kernel.org/linux-perf-users/206972a3-d44d-1c75-3fbc-426427614543@intel.com/>
> 
>     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