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: <c447f442-2cc5-ab04-071e-1c26976dc828@huawei.com>
Date:   Fri, 25 Aug 2023 14:15:07 +0800
From:   Yang Jihong <yangjihong1@...wei.com>
To:     Ian Rogers <irogers@...gle.com>
CC:     Adrian Hunter <adrian.hunter@...el.com>,
        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.
> 
Okay, I'll follow the above changes in the next version.

Thanks,
Yang