linux-kernel - Re: [PATCH v2 3/3] perf sort: Use perf_env to set arch sort keys and header

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9aa2c899-80e0-4626-acb7-5331fbf46a0d@linux.intel.com>
Date: Wed, 21 May 2025 14:13:54 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>,
 Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
 James Clark <james.clark@...aro.org>,
 "Masami Hiramatsu (Google)" <mhiramat@...nel.org>,
 Ravi Bangoria <ravi.bangoria@....com>, Leo Yan <leo.yan@....com>,
 Yujie Liu <yujie.liu@...el.com>, Graham Woodward <graham.woodward@....com>,
 Howard Chu <howardchu95@...il.com>, Weilin Wang <weilin.wang@...el.com>,
 Dmitry Vyukov <dvyukov@...gle.com>, Andi Kleen <ak@...ux.intel.com>,
 Thomas Falcon <thomas.falcon@...el.com>, Matt Fleming
 <matt@...dmodwrite.com>, Chun-Tse Shao <ctshao@...gle.com>,
 Ben Gainey <ben.gainey@....com>, Song Liu <song@...nel.org>,
 linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v2 3/3] perf sort: Use perf_env to set arch sort keys and
 header



On 2025-05-21 12:16 p.m., Ian Rogers wrote:
>>> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
>>> index 7b6cde87d2af..13ef0d188a96 100644
>>> --- a/tools/perf/builtin-top.c
>>> +++ b/tools/perf/builtin-top.c
>>> @@ -1747,7 +1747,14 @@ int cmd_top(int argc, const char **argv)
>>>
>>>       setup_browser(false);
>>>
>>> -     if (setup_sorting(top.evlist) < 0) {
>>> +     top.session = perf_session__new(/*data=*/NULL, /*tool=*/NULL);
>>> +     if (IS_ERR(top.session)) {
>>> +             status = PTR_ERR(top.session);
>>> +             top.session = NULL;
>>> +             goto out_delete_evlist;
>>> +     }
>>> +
>>> +     if (setup_sorting(top.evlist, &top.session->header.env) < 0) {
>> I doubt a valide env can be got in perf_session__new(), since there is
>> no perf.data in perf top.
>> Maybe just need to invoke the perf_env__raw_arch() instead to fill the
>> env->arch.
> I think the current code is making things harder than it should be, we
> should work away from perf_env__arch and strings, instead using EM_
> values which we can default to EM_HOST avoiding any runtime costs.
> Looking at perf_env__arch:
> ```
> const char *perf_env__arch(struct perf_env *env)
> {
>         char *arch_name;
> 
>         if (!env || !env->arch) { /* Assume local operation */
>                 static struct utsname uts = { .machine[0] = '\0', };
>                 if (uts.machine[0] == '\0' && uname(&uts) < 0)
>                         return NULL;
>                 arch_name = uts.machine;
>         } else
>                 arch_name = env->arch;
> 
>         return normalize_arch(arch_name);
> }
> ```
> in this case env->arch == NULL and so the uname machine will be used.
> For perf_env__raw_arch the behavior is similar but it populates the
> env:
> ```
> static int perf_env__read_arch(struct perf_env *env)
> {
>         struct utsname uts;
> 
>         if (env->arch)
>                 return 0;
> 
>         if (!uname(&uts))
>                 env->arch = strdup(uts.machine);
> 
>         return env->arch ? 0 : -ENOMEM;
> }
> 
> const char *perf_env__raw_arch(struct perf_env *env)
> {
>         return env && !perf_env__read_arch(env) ? env->arch : "unknown";
> }
> ```
> Aside from caching the arch, the main difference is that
> normalize_arch isn't called. Not having normalize_arch means the code
> in arch_support_sort_key and arch_perf_header_entry would need to
> handle strings "ppc" as well as "powerpc", "i386" as well as "x86",
> etc. As I'd prefer not handle all those cases I think the way the code
> is is best given how the env code is currently structured.

Right. The perf_env__raw_arch() doesn't improve anything.
But I still don't like &top.session->header.env.
Because I don't think you can get any useful information from
top.session->header.env. It just brings confusions. (It seems an env is
retrieved, but it is actually not.)

In the perf top, &perf_env is used for the existing cases. If any env
fields are not available, perf_env__read_XXX() is invoked to get the
information.
I think we may follow the existing usage, e.g.,
setup_sorting(top.evlist, &perf_env).

Alternatively, it looks like the perf top doesn't support --weight. The
env->arch should never be used. If so, a NULL can be used as well, E.g.,
setup_sorting(top.evlist, NULL).

Thanks,
Kan