Re: [PATCH v3 1/3] perf sample: Remove arch notion of sample parsing

[Namhyung Kim <namhyung@kernel.org>]

On Wed, May 21, 2025 at 02:15:24PM -0700, Ian Rogers wrote:
> On Wed, May 21, 2025 at 1:27 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Wed, May 21, 2025 at 09:53:15AM -0700, Ian Rogers wrote:
> > > By definition arch sample parsing and synthesis will inhibit certain
> > > kinds of cross-platform record then analysis (report, script,
> > > etc.). Remove arch_perf_parse_sample_weight and
> > > arch_perf_synthesize_sample_weight replacing with a common
> > > implementation. Combine perf_sample p_stage_cyc and retire_lat to
> > > capture the differing uses regardless of compiled for architecture.
> >
> > Can you please do this without renaming?  It can be a separate patch but
> > I think we can just leave it.
> 
> It is not clear what the use of the union is. Presumably it is a
> tagged union but there is no tag as the union value to use is implied
> by either being built on x86_64 or powerpc. The change removes the
> notion of this code being built for x86_64 or powerpc and so the union
> value to use isn't clear (e.g. should arm use p_stage_cyc or
> retire_lat from the union), hence combining to show that it could be
> one or the other. The code could be:
> ```
> #ifdef __x86_64__
>        u16 p_stage_cyc;
> #elif defined(powerpc)
>        u16 retire_lat;
> #endif
> ```
> but this isn't cross-platform.

Right, we probably don't want it.


> The change in hist.h of
> ```
> @@ -255,7 +255,7 @@ struct hist_entry {
>         u64                     code_page_size;
>         u64                     weight;
>         u64                     ins_lat;
> -       u64                     p_stage_cyc;
> +       u64                     p_stage_cyc_or_retire_lat;
> ```
> could be a follow up CL, but then we lose something of what the field
> is holding given the value is just a copy of that same named value in
> perf_sample. The code only inserts 34 lines and so the churn of doing
> that seemed worse than having the change in a single patch for
> clarity.

Assuming other archs can add something later, we won't rename the field
again.  So I can live with the ugly union fields.  If we really want to
rename it, I prefer calling it just 'weight3' and let the archs handle
the display name only.

Thanks,
Namhyung