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

["Liang, Kan" <kan.liang@linux.intel.com>]

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