Re: [PATCH v7 7/9] perf: arm64: Add test for userspace counter access on heterogeneous systems

[Masayoshi Mizuma <msys.mizuma@gmail.com>]

On Fri, Apr 30, 2021 at 01:20:58PM -0500, Rob Herring wrote:
> On Fri, Apr 30, 2021 at 1:17 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Apr 30, 2021 at 11:46 AM Masayoshi Mizuma <msys.mizuma@gmail.com> wrote:
> > >
> > > On Mon, Apr 19, 2021 at 10:15:09PM -0500, Rob Herring wrote:
> > > > Userspace counter access only works on heterogeneous systems with some
> > > > restrictions. The userspace process must be pinned to a homogeneous
> > > > subset of CPUs and must open the corresponding PMU for those CPUs. This
> > > > commit adds a test implementing these requirements.
> > > >
> > > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > > ---
> > > > v6:
> > > >  - Add a check on cap_user_rdpmc
> > > > v5:
> > > >  - Adapt to libperf mmap API changes
> > > > v4:
> > > >  - Update perf_evsel__mmap params
> > > > v2:
> > > >  - Drop all but heterogeneous test as others covered by libperf tests
> > > >  - Rework to use libperf
> > > > ---
> > > >  tools/perf/arch/arm64/include/arch-tests.h |   7 +
> > > >  tools/perf/arch/arm64/tests/Build          |   1 +
> > > >  tools/perf/arch/arm64/tests/arch-tests.c   |   4 +
> > > >  tools/perf/arch/arm64/tests/user-events.c  | 177 +++++++++++++++++++++
> > > >  4 files changed, 189 insertions(+)
> > > >  create mode 100644 tools/perf/arch/arm64/tests/user-events.c
> > > >
> > > > diff --git a/tools/perf/arch/arm64/include/arch-tests.h b/tools/perf/arch/arm64/include/arch-tests.h
> > > > index 90ec4c8cb880..380ad34a3f09 100644
> > > > --- a/tools/perf/arch/arm64/include/arch-tests.h
> > > > +++ b/tools/perf/arch/arm64/include/arch-tests.h
> > > > @@ -2,11 +2,18 @@
> > > >  #ifndef ARCH_TESTS_H
> > > >  #define ARCH_TESTS_H
> > > >
> > > > +#include <linux/compiler.h>
> > > > +
> > > >  #ifdef HAVE_DWARF_UNWIND_SUPPORT
> > > >  struct thread;
> > > >  struct perf_sample;
> > > > +int test__arch_unwind_sample(struct perf_sample *sample,
> > > > +                          struct thread *thread);
> > > >  #endif
> > >
> > > Hello,
> > >
> > > I got the following compile error with aarch64 on Fedora33.
> > >
> > >     # make tools/perf
> > >     ...
> > >     In file included from arch/arm64/tests/arch-tests.c:4:
> > >     /root//libperf_v7/tools/perf/arch/arm64/include/arch-tests.h:10:5: error: redundant redeclaration of ‘test__arch_unwind_sample’ [-Werror=redundant-decls]
> > >        10 | int test__arch_unwind_sample(struct perf_sample *sample,
> > >           |     ^~~~~~~~~~~~~~~~~~~~~~~~
> > >     In file included from arch/arm64/tests/arch-tests.c:3:
> > >     /root//libperf_v7/tools/perf/tests/tests.h:140:5: note: previous declaration of ‘test__arch_unwind_sample’ was here
> > >       140 | int test__arch_unwind_sample(struct perf_sample *sample,
> > >           |     ^~~~~~~~~~~~~~~~~~~~~~~~
> > >     cc1: all warnings being treated as errors
> > >     make[8]: *** [/root//libperf_v7/tools/build/Makefile.build:97: /root/libperf_v7/tools/perf/arch/arm64/tests/arch-tests.o] Error 1
> > >     make[8]: *** Waiting for unfinished jobs....
> > >     In file included from arch/arm64/tests/user-events.c:13:
> > >     /root//libperf_v7/tools/perf/arch/arm64/include/arch-tests.h:10:5: error: redundant redeclaration of ‘test__arch_unwind_sample’ [-Werror=redundant-decls]
> > >        10 | int test__arch_unwind_sample(struct perf_sample *sample,
> > >           |     ^~~~~~~~~~~~~~~~~~~~~~~~
> > >     In file included from arch/arm64/tests/user-events.c:12:
> > >     /root//libperf_v7/tools/perf/tests/tests.h:140:5: note: previous declaration of ‘test__arch_unwind_sample’ was here
> > >       140 | int test__arch_unwind_sample(struct perf_sample *sample,
> > >           |     ^~~~~~~~~~~~~~~~~~~~~~~~
> > >     cc1: all warnings being treated as errors
> > >     ...
> > >
> > > The error is gone after the following patch is applied.
> >
> > Thanks. Honestly, I'm not sure why it was there in the first place.
> > Looking at the git history and this series history doesn't give any
> > clues.
> 
> Well, except that both x86 and powerpc have the same hunk in their
> arch-tests.h. Do you see errors on those arches?

I didn't see the errors on x86_64.
It seems that the errors happen on aarch64 because 
test__arch_unwind_sample() is defined only if the arch
is arm or arm64 in tools/perf/tests/tests.h:

    #if defined(__arm__) || defined(__aarch64__)
    #ifdef HAVE_DWARF_UNWIND_SUPPORT
    struct thread;
    struct perf_sample;
    int test__arch_unwind_sample(struct perf_sample *sample,
                                 struct thread *thread);
    #endif
    #endif

The following patch may be another solution which is same way as
commit d8b167f9d8af ("perf tests: Move x86 tests into arch directory").

---
 tools/perf/arch/arm64/tests/dwarf-unwind.c | 1 +
 tools/perf/tests/dwarf-unwind.c            | 2 +-
 tools/perf/tests/tests.h                   | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/arch/arm64/tests/dwarf-unwind.c b/tools/perf/arch/arm64/tests/dwarf-unwind.c
index 46147a483..02ba87f2b 100644
--- a/tools/perf/arch/arm64/tests/dwarf-unwind.c
+++ b/tools/perf/arch/arm64/tests/dwarf-unwind.c
@@ -7,6 +7,7 @@
 #include "event.h"
 #include "debug.h"
 #include "tests/tests.h"
+#include "arch-tests.h"
 
 #define STACK_SIZE 8192
 
diff --git a/tools/perf/tests/dwarf-unwind.c b/tools/perf/tests/dwarf-unwind.c
index 83638097c..daffe2d66 100644
--- a/tools/perf/tests/dwarf-unwind.c
+++ b/tools/perf/tests/dwarf-unwind.c
@@ -17,7 +17,7 @@
 #include "callchain.h"
 #include "util/synthetic-events.h"
 
-#if defined (__x86_64__) || defined (__i386__) || defined (__powerpc__)
+#if defined (__x86_64__) || defined (__i386__) || defined (__powerpc__) || defined(__aarch64__)
 #include "arch-tests.h"
 #endif
 
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index b85f00530..40cbdfa46 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -133,7 +133,7 @@ bool test__bp_account_is_supported(void);
 bool test__wp_is_supported(void);
 bool test__tsc_is_supported(void);
 
-#if defined(__arm__) || defined(__aarch64__)
+#if defined(__arm__)
 #ifdef HAVE_DWARF_UNWIND_SUPPORT
 struct thread;
 struct perf_sample;
-- 

Thanks,
Masa