Message-ID: <CAP-5=fV29jLxFUDwMFvnaV6R4ZkQg=KZ_FwU+ueQQLof1R3tRw@mail.gmail.com>
Date: Mon, 7 Oct 2024 08:32:09 -0700
From: Ian Rogers <irogers@...gle.com>
To: Masami Hiramatsu <mhiramat@...nel.org>
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>, Kan Liang <kan.liang@...ux.intel.com>, 
	John Garry <john.g.garry@...cle.com>, Will Deacon <will@...nel.org>, 
	James Clark <james.clark@...aro.org>, Mike Leach <mike.leach@...aro.org>, 
	Leo Yan <leo.yan@...ux.dev>, Guo Ren <guoren@...nel.org>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, 
	Albert Ou <aou@...s.berkeley.edu>, Nick Terrell <terrelln@...com>, 
	"Steven Rostedt (Google)" <rostedt@...dmis.org>, Guilherme Amadio <amadio@...too.org>, 
	Changbin Du <changbin.du@...wei.com>, Daniel Bristot de Oliveira <bristot@...nel.org>, 
	Daniel Wagner <dwagner@...e.de>, Aditya Gupta <adityag@...ux.ibm.com>, 
	Athira Rajeev <atrajeev@...ux.vnet.ibm.com>, Masahiro Yamada <masahiroy@...nel.org>, 
	Kajol Jain <kjain@...ux.ibm.com>, Huacai Chen <chenhuacai@...nel.org>, 
	Bibo Mao <maobibo@...ngson.cn>, Anup Patel <anup@...infault.org>, 
	Atish Patra <atishp@...osinc.com>, Shenlin Liang <liangshenlin@...incomputing.com>, 
	Oliver Upton <oliver.upton@...ux.dev>, "Steinar H. Gunderson" <sesse@...gle.com>, 
	"Dr. David Alan Gilbert" <linux@...blig.org>, Chen Pei <cp0613@...ux.alibaba.com>, 
	Dima Kogan <dima@...retsauce.net>, Yury Norov <yury.norov@...il.com>, 
	Alexander Lobakin <aleksander.lobakin@...el.com>, linux-kernel@...r.kernel.org, 
	linux-perf-users@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-csky@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v2 18/31] perf dwarf-regs: Move x86 dwarf-regs out of arch

On Mon, Oct 7, 2024 at 1:35 AM Masami Hiramatsu <mhiramat@...nel.org> wrote:
>
> On Sat,  5 Oct 2024 12:55:28 -0700
> Ian Rogers <irogers@...gle.com> wrote:
>
> > Move arch/x86/util/dwarf-regs.c to util/dwarf-regs-x86.c and compile
> > in unconditionally. To avoid get_arch_regnum being duplicated, rename
> > to get_x86_regnum and add to get_dwarf_regnum switch.
> >
> > For get_arch_regstr, this was unused on x86 unless the machine type
> > was EM_NONE. Map that case to EM_HOST and remove get_arch_regstr from
> > dwarf-regs-x86.c.
> >
>
> Hmm, I'm not sure this change. I feel like to keep the arch dependent
> part under arch/ directory

Code in the arch directory is built with this condition:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/arch/Build?h=perf-tools-next
perf-util-y += $(SRCARCH)/
That is we only enter an arch directory if it matches the architecture
we're building for.
I think what you are saying is that it'd be nice if x86 related code
were in the arch directory, but with things as they are that means the
x86 related code wouldn't be built on say ARM.

To workaround this there is plenty of architecture specific code in
tools/perf/util:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util?h=perf-tools-next
amd-sample-raw.c
arm64-frame-pointer-unwind-support.c
arm64-frame-pointer-unwind-support.h
arm-spe.c
arm-spe-decoder
arm-spe.h
intel-bts.c
intel-bts.h
intel-pt.c
intel-pt-decoder
intel-pt.h
intel-tpebs.c
intel-tpebs.h
s390-cpumcf-kernel.h
s390-cpumsf.c
s390-cpumsf.h
s390-cpumsf-kernel.h
s390-sample-raw.c

With this change added are:
dwarf-regs-csky.c
dwarf-regs-powerpc.c
dwarf-regs-x86.c

So I feel this way of doing things is most consistent and achieves the
best result.

In general I think the arch directory in perf is a mistake:
1) it breaks the profile on one platform, analyze on another model -
in this series it is shown that using the ELF machine is just better
than hard coding to the host architecture;
2) it is used as something of an ifdef __<arch>__ replacement but in a
way that makes understanding behavior in the code harder to fathom as
you need to understand weak functions (not a C feature) and the build
logic;
3) much of the code is really PMU driver dependent rather than
architecture dependent, moving such code out of arch would enable
things like user space emulation of the perf tool.

> But if this is just for reverse mapping of
> regname -> regnum, since we already have regnum -> regname maps in the
> header file, we can reimplement get_dwarf_regnum() as a generic function.
> And that is what you does in this series, correct?

That work could be done for get_dwarf_regnum but isn't done in this
series. Most architectures are missing get_arch_regnum.
For get_dwarf_regstr and get_arch_regstr there was the redundancy as
get_dwarf_regstr would compute a regstr if the machine wasn't E_NONE
in most cases. get_arch_regstr was only used for E_NONE. This code
changes E_NONE to E_HOST and then the get_arch_regstr code isn't
needed. There are #ifdefs adding complexity as I didn't do a single
monolithic patch - the maintainers usually complain if you do big
monolithic patches. Patches at the end of the series remove all the
#ifdefs, it is just without them the series will break cross
compilation if you bisect into the middle of it.

> If so, can you make a generic get_dwarf_regnum() at first, and remove
> the architecture dependent part which is not needed anymore.
> That should be more simpler to review.

I think this is out of scope for this series, not least because of the
testing challenge, but could be follow up work.

Thanks,
Ian

> > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > ---
> >  tools/perf/arch/x86/util/Build                |  3 ---
> >  tools/perf/util/Build                         |  1 +
> >  .../dwarf-regs.c => util/dwarf-regs-x86.c}    | 24 +------------------
> >  tools/perf/util/dwarf-regs.c                  | 17 +++++++++++++
> >  tools/perf/util/include/dwarf-regs.h          |  8 +++++++
> >  5 files changed, 27 insertions(+), 26 deletions(-)
> >  rename tools/perf/{arch/x86/util/dwarf-regs.c => util/dwarf-regs-x86.c} (77%)
> >
> > diff --git a/tools/perf/arch/x86/util/Build b/tools/perf/arch/x86/util/Build
> > index 9705cda4f240..70af491a6dd7 100644
> > --- a/tools/perf/arch/x86/util/Build
> > +++ b/tools/perf/arch/x86/util/Build
> > @@ -12,9 +12,6 @@ perf-util-y += evsel.o
> >  perf-util-y += iostat.o
> >  perf-util-y += env.o
> >
> > -perf-util-$(CONFIG_LIBDW) += dwarf-regs.o
> > -perf-util-$(CONFIG_BPF_PROLOGUE) += dwarf-regs.o
> > -
> >  perf-util-$(CONFIG_LOCAL_LIBUNWIND)    += unwind-libunwind.o
> >  perf-util-$(CONFIG_LIBDW_DWARF_UNWIND) += unwind-libdw.o
> >
> > diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> > index 1d08608b7e1b..c2221ef431f3 100644
> > --- a/tools/perf/util/Build
> > +++ b/tools/perf/util/Build
> > @@ -202,6 +202,7 @@ endif
> >  perf-util-$(CONFIG_LIBDW) += probe-finder.o
> >  perf-util-$(CONFIG_LIBDW) += dwarf-aux.o
> >  perf-util-$(CONFIG_LIBDW) += dwarf-regs.o
> > +perf-util-$(CONFIG_LIBDW) += dwarf-regs-x86.o
> >  perf-util-$(CONFIG_LIBDW) += debuginfo.o
> >  perf-util-$(CONFIG_LIBDW) += annotate-data.o
> >
> > diff --git a/tools/perf/arch/x86/util/dwarf-regs.c b/tools/perf/util/dwarf-regs-x86.c
> > similarity index 77%
> > rename from tools/perf/arch/x86/util/dwarf-regs.c
> > rename to tools/perf/util/dwarf-regs-x86.c
> > index 530905118cd4..7a55c65e8da6 100644
> > --- a/tools/perf/arch/x86/util/dwarf-regs.c
> > +++ b/tools/perf/util/dwarf-regs-x86.c
> > @@ -11,28 +11,6 @@
> >  #include <linux/kernel.h> /* for ARRAY_SIZE */
> >  #include <dwarf-regs.h>
> >
> > -#define DEFINE_DWARF_REGSTR_TABLE 1
> > -#include "dwarf-regs-table.h"
> > -
> > -/* Return architecture dependent register string (for kprobe-tracer) */
> > -const char *get_arch_regstr(unsigned int n)
> > -{
> > -#if defined(__i386__)
> > -     size_t len = ARRAY_SIZE(x86_32_regstr_tbl);
> > -#else
> > -     size_t len = ARRAY_SIZE(x86_64_regstr_tbl);
> > -#endif
> > -
> > -     if (n >= len)
> > -             return NULL;
> > -
> > -#if defined(__i386__)
> > -     return x86_32_regstr_tbl[n];
> > -#else
> > -     return x86_64_regstr_tbl[n];
> > -#endif
> > -}
> > -
> >  struct dwarf_regs_idx {
> >       const char *name;
> >       int idx;
> > @@ -58,7 +36,7 @@ static const struct dwarf_regs_idx x86_regidx_table[] = {
> >       { "rip", DWARF_REG_PC },
> >  };
> >
> > -int get_arch_regnum(const char *name)
> > +int get_x86_regnum(const char *name)
> >  {
> >       unsigned int i;
> >
> > diff --git a/tools/perf/util/dwarf-regs.c b/tools/perf/util/dwarf-regs.c
> > index 86b3ef638fbb..eac99a246737 100644
> > --- a/tools/perf/util/dwarf-regs.c
> > +++ b/tools/perf/util/dwarf-regs.c
> > @@ -32,9 +32,17 @@
> >  const char *get_dwarf_regstr(unsigned int n, unsigned int machine,
> >                            unsigned int flags __maybe_unused)
> >  {
> > +#if EM_HOST == EM_X86_64 || EM_HOST == EM_386
> > +     if (machine == EM_NONE) {
> > +             /* Generic arch - use host arch */
> > +             machine = EM_HOST;
> > +     }
> > +#endif
> >       switch (machine) {
> > +#if EM_HOST != EM_X86_64 && EM_HOST != EM_386
> >       case EM_NONE:   /* Generic arch - use host arch */
> >               return get_arch_regstr(n);
> > +#endif
> >       case EM_386:
> >               return __get_dwarf_regstr(x86_32_regstr_tbl, n);
> >       case EM_X86_64:
> > @@ -65,10 +73,12 @@ const char *get_dwarf_regstr(unsigned int n, unsigned int machine,
> >       return NULL;
> >  }
> >
> > +#if EM_HOST != EM_X86_64 && EM_HOST != EM_386
> >  __weak int get_arch_regnum(const char *name __maybe_unused)
> >  {
> >       return -ENOTSUP;
> >  }
> > +#endif
> >
> >  /* Return DWARF register number from architecture register name */
> >  int get_dwarf_regnum(const char *name, unsigned int machine, unsigned int flags __maybe_unused)
> > @@ -90,9 +100,16 @@ int get_dwarf_regnum(const char *name, unsigned int machine, unsigned int flags
> >               machine = EM_HOST;
> >       }
> >       switch (machine) {
> > +#if EM_HOST != EM_X86_64 && EM_HOST != EM_386
> >       case EM_HOST:
> >               reg = get_arch_regnum(regname);
> >               break;
> > +#endif
> > +     case EM_X86_64:
> > +             fallthrough;
> > +     case EM_386:
> > +             reg = get_x86_regnum(regname);
> > +             break;
> >       default:
> >               pr_err("ELF MACHINE %x is not supported.\n", machine);
> >       }
> > diff --git a/tools/perf/util/include/dwarf-regs.h b/tools/perf/util/include/dwarf-regs.h
> > index 925525405e2d..062623aefd5a 100644
> > --- a/tools/perf/util/include/dwarf-regs.h
> > +++ b/tools/perf/util/include/dwarf-regs.h
> > @@ -79,7 +79,10 @@
> >  #define DWARF_REG_FB  0xd3affb /* random number */
> >
> >  #ifdef HAVE_LIBDW_SUPPORT
> > +#if !defined(__x86_64__) && !defined(__i386__)
> >  const char *get_arch_regstr(unsigned int n);
> > +#endif
> > +
> >  /**
> >   * get_dwarf_regstr() - Returns ftrace register string from DWARF regnum.
> >   * @n: DWARF register number.
> > @@ -88,7 +91,12 @@ const char *get_arch_regstr(unsigned int n);
> >   */
> >  const char *get_dwarf_regstr(unsigned int n, unsigned int machine, unsigned int flags);
> >
> > +int get_x86_regnum(const char *name);
> > +
> > +#if !defined(__x86_64__) && !defined(__i386__)
> >  int get_arch_regnum(const char *name);
> > +#endif
> > +
> >  /*
> >   * get_dwarf_regnum - Returns DWARF regnum from register name
> >   * name: architecture register name
> > --
> > 2.47.0.rc0.187.ge670bccf7e-goog
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@...nel.org>

Powered by blists - more mailing lists