[PATCH] perf/core: Drop __weak attribute from arch_perf_update_userpage() prototype

[PATCH] perf/core: Drop __weak attribute from arch_perf_update_userpage() prototype

[Marc Zyngier]

Reiji reports that the arm64 implementation of arch_perf_update_userpage()
is now ignored and replaced by the dummy stub in core code.
This seems to happen since the PMUv3 driver was moved to driver/perf.

As it turns out, dropping the __weak attribute from the *prototype*
of the function solves the problem. You're right, this doesn't seem
to make much sense. And yet... It appears that both symbols get
flagged as weak, and that the first one to appear in the link order
wins:

$ nm drivers/perf/arm_pmuv3.o|grep arch_perf_update_userpage
0000000000001db0 W arch_perf_update_userpage

Dropping the attribute from the prototype restores the expected
behaviour, and arm64 is able to enjoy arch_perf_update_userpage()
again.

Reported-by: Reiji Watanabe <reijiw@google.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 include/linux/perf_event.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index d5628a7b5eaa..c8dcfdbda1f4 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1845,9 +1845,9 @@ int perf_event_exit_cpu(unsigned int cpu);
 #define perf_event_exit_cpu	NULL
 #endif
 
-extern void __weak arch_perf_update_userpage(struct perf_event *event,
-					     struct perf_event_mmap_page *userpg,
-					     u64 now);
+extern void arch_perf_update_userpage(struct perf_event *event,
+				      struct perf_event_mmap_page *userpg,
+				      u64 now);
 
 #ifdef CONFIG_MMU
 extern __weak u64 arch_perf_get_page_size(struct mm_struct *mm, unsigned long addr);
-- 
2.34.1

Re: [PATCH] perf/core: Drop __weak attribute from arch_perf_update_userpage() prototype

[Reiji Watanabe]

On Sat, Jun 03, 2023 at 09:25:19AM +0100, Marc Zyngier wrote:
> Reiji reports that the arm64 implementation of arch_perf_update_userpage()
> is now ignored and replaced by the dummy stub in core code.
> This seems to happen since the PMUv3 driver was moved to driver/perf.
> 
> As it turns out, dropping the __weak attribute from the *prototype*
> of the function solves the problem. You're right, this doesn't seem
> to make much sense. And yet... It appears that both symbols get
> flagged as weak, and that the first one to appear in the link order
> wins:
> 
> $ nm drivers/perf/arm_pmuv3.o|grep arch_perf_update_userpage
> 0000000000001db0 W arch_perf_update_userpage
> 
> Dropping the attribute from the prototype restores the expected
> behaviour, and arm64 is able to enjoy arch_perf_update_userpage()
> again.
> 
> Reported-by: Reiji Watanabe <reijiw@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Will Deacon <will@kernel.org>

Tested-by: Reiji Watanabe <reijiw@google.com>

Re: [PATCH] perf/core: Drop __weak attribute from arch_perf_update_userpage() prototype

[Mark Rutland]

On Sat, Jun 03, 2023 at 09:25:19AM +0100, Marc Zyngier wrote:
> Reiji reports that the arm64 implementation of arch_perf_update_userpage()
> is now ignored and replaced by the dummy stub in core code.
> This seems to happen since the PMUv3 driver was moved to driver/perf.

I guess we should have a Cc stable then?

The below implies this has always been on dodgy ground, and so it's probably
inaccurate to give this a Fixes tag pointing to the move.

> As it turns out, dropping the __weak attribute from the *prototype*
> of the function solves the problem. You're right, this doesn't seem
> to make much sense. And yet... It appears that both symbols get
> flagged as weak, and that the first one to appear in the link order
> wins:
> 
> $ nm drivers/perf/arm_pmuv3.o|grep arch_perf_update_userpage
> 0000000000001db0 W arch_perf_update_userpage

Ah, so having it on th *declaration* will apply to any *definition*. :/

That suggests this is a bad pattern generally, and we should probably remove
the other __weak instances in headers. Lukcily it seems there aren't that many:

[mark@lakrids:~/src/linux]% git grep __weak -- **/*.h | wc -l
50

IMO we'd should aim to remove __weak entirely; it causes a number of weird
things like this and it'd be much easier to manage with a small amount of
ifdeffery.

Peter, thoughts?

> Dropping the attribute from the prototype restores the expected
> behaviour, and arm64 is able to enjoy arch_perf_update_userpage()
> again.
> 
> Reported-by: Reiji Watanabe <reijiw@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Tested-by: Reiji Watanabe <reijiw@google.com>

FWIW, regardless of the above:

Acked-by: Mark Rutland <mark.rutland@arm.com>

> ---
>  include/linux/perf_event.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index d5628a7b5eaa..c8dcfdbda1f4 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -1845,9 +1845,9 @@ int perf_event_exit_cpu(unsigned int cpu);
>  #define perf_event_exit_cpu	NULL
>  #endif
>  
> -extern void __weak arch_perf_update_userpage(struct perf_event *event,
> -					     struct perf_event_mmap_page *userpg,
> -					     u64 now);
> +extern void arch_perf_update_userpage(struct perf_event *event,
> +				      struct perf_event_mmap_page *userpg,
> +				      u64 now);
>  
>  #ifdef CONFIG_MMU
>  extern __weak u64 arch_perf_get_page_size(struct mm_struct *mm, unsigned long addr);

Luckily, arch_perf_get_page_size() has no callers or definition since commit:

  8af26be062721e52 ("perf/core: Fix arch_perf_get_page_size()")

... so we can just delete that prototype.

Mark

Re: [PATCH] perf/core: Drop __weak attribute from arch_perf_update_userpage() prototype

[Marc Zyngier]

On Mon, 12 Jun 2023 14:16:28 +0100,
Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Sat, Jun 03, 2023 at 09:25:19AM +0100, Marc Zyngier wrote:
> > Reiji reports that the arm64 implementation of arch_perf_update_userpage()
> > is now ignored and replaced by the dummy stub in core code.
> > This seems to happen since the PMUv3 driver was moved to driver/perf.
> 
> I guess we should have a Cc stable then?

Potentially. I don't think anyone else is affected though, as we're
the only one implementing this outside of the arch code.

> 
> The below implies this has always been on dodgy ground, and so it's probably
> inaccurate to give this a Fixes tag pointing to the move.

Indeed. We just didn't notice.

> 
> > As it turns out, dropping the __weak attribute from the *prototype*
> > of the function solves the problem. You're right, this doesn't seem
> > to make much sense. And yet... It appears that both symbols get
> > flagged as weak, and that the first one to appear in the link order
> > wins:
> > 
> > $ nm drivers/perf/arm_pmuv3.o|grep arch_perf_update_userpage
> > 0000000000001db0 W arch_perf_update_userpage
> 
> Ah, so having it on th *declaration* will apply to any *definition*. :/
> 
> That suggests this is a bad pattern generally, and we should probably remove
> the other __weak instances in headers. Lukcily it seems there aren't that many:
> 
> [mark@lakrids:~/src/linux]% git grep __weak -- **/*.h | wc -l
> 50

The majority seems to be in tools (BPF FTW!), but get_c0_perfcount_int
in the MIPS code count be an interesting one...

> 
> IMO we'd should aim to remove __weak entirely; it causes a number of weird
> things like this and it'd be much easier to manage with a small amount of
> ifdeffery.
>
> Peter, thoughts?
> 
> > Dropping the attribute from the prototype restores the expected
> > behaviour, and arm64 is able to enjoy arch_perf_update_userpage()
> > again.
> > 
> > Reported-by: Reiji Watanabe <reijiw@google.com>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Tested-by: Reiji Watanabe <reijiw@google.com>
> 
> FWIW, regardless of the above:
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>

Cheers for that.

> 
> > ---
> >  include/linux/perf_event.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> > index d5628a7b5eaa..c8dcfdbda1f4 100644
> > --- a/include/linux/perf_event.h
> > +++ b/include/linux/perf_event.h
> > @@ -1845,9 +1845,9 @@ int perf_event_exit_cpu(unsigned int cpu);
> >  #define perf_event_exit_cpu	NULL
> >  #endif
> >  
> > -extern void __weak arch_perf_update_userpage(struct perf_event *event,
> > -					     struct perf_event_mmap_page *userpg,
> > -					     u64 now);
> > +extern void arch_perf_update_userpage(struct perf_event *event,
> > +				      struct perf_event_mmap_page *userpg,
> > +				      u64 now);
> >  
> >  #ifdef CONFIG_MMU
> >  extern __weak u64 arch_perf_get_page_size(struct mm_struct *mm, unsigned long addr);
> 
> Luckily, arch_perf_get_page_size() has no callers or definition since commit:
> 
>   8af26be062721e52 ("perf/core: Fix arch_perf_get_page_size()")
> 
> ... so we can just delete that prototype.

Yeah, I have a patch for that too, but didn't want to distract with
something that is just a basic cleanup, and I'd rather see a sweeping
cleanup.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] perf/core: Drop __weak attribute from arch_perf_update_userpage() prototype

[Peter Zijlstra]

On Mon, Jun 12, 2023 at 02:16:28PM +0100, Mark Rutland wrote:
> On Sat, Jun 03, 2023 at 09:25:19AM +0100, Marc Zyngier wrote:
> > Reiji reports that the arm64 implementation of arch_perf_update_userpage()
> > is now ignored and replaced by the dummy stub in core code.
> > This seems to happen since the PMUv3 driver was moved to driver/perf.
> 
> I guess we should have a Cc stable then?
> 
> The below implies this has always been on dodgy ground, and so it's probably
> inaccurate to give this a Fixes tag pointing to the move.
> 
> > As it turns out, dropping the __weak attribute from the *prototype*
> > of the function solves the problem. You're right, this doesn't seem
> > to make much sense. And yet... It appears that both symbols get
> > flagged as weak, and that the first one to appear in the link order
> > wins:
> > 
> > $ nm drivers/perf/arm_pmuv3.o|grep arch_perf_update_userpage
> > 0000000000001db0 W arch_perf_update_userpage
> 
> Ah, so having it on th *declaration* will apply to any *definition*. :/

Yikes..

> That suggests this is a bad pattern generally, and we should probably remove
> the other __weak instances in headers. Lukcily it seems there aren't that many:
> 
> [mark@lakrids:~/src/linux]% git grep __weak -- **/*.h | wc -l
> 50
> 
> IMO we'd should aim to remove __weak entirely; it causes a number of weird
> things like this and it'd be much easier to manage with a small amount of
> ifdeffery.
> 
> Peter, thoughts?

Not a fan of __weak myself, after having had to deal with how the
compilers actually make it work.

Where do I queue this? perf/urgent?

Re: [PATCH] perf/core: Drop __weak attribute from arch_perf_update_userpage() prototype

[Marc Zyngier]

On Mon, 12 Jun 2023 15:54:23 +0100,
Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Mon, Jun 12, 2023 at 02:16:28PM +0100, Mark Rutland wrote:
> > On Sat, Jun 03, 2023 at 09:25:19AM +0100, Marc Zyngier wrote:
> > > Reiji reports that the arm64 implementation of arch_perf_update_userpage()
> > > is now ignored and replaced by the dummy stub in core code.
> > > This seems to happen since the PMUv3 driver was moved to driver/perf.
> > 
> > I guess we should have a Cc stable then?
> > 
> > The below implies this has always been on dodgy ground, and so it's probably
> > inaccurate to give this a Fixes tag pointing to the move.
> > 
> > > As it turns out, dropping the __weak attribute from the *prototype*
> > > of the function solves the problem. You're right, this doesn't seem
> > > to make much sense. And yet... It appears that both symbols get
> > > flagged as weak, and that the first one to appear in the link order
> > > wins:
> > > 
> > > $ nm drivers/perf/arm_pmuv3.o|grep arch_perf_update_userpage
> > > 0000000000001db0 W arch_perf_update_userpage
> > 
> > Ah, so having it on th *declaration* will apply to any *definition*. :/
> 
> Yikes..
> 
> > That suggests this is a bad pattern generally, and we should probably remove
> > the other __weak instances in headers. Lukcily it seems there aren't that many:
> > 
> > [mark@lakrids:~/src/linux]% git grep __weak -- **/*.h | wc -l
> > 50
> > 
> > IMO we'd should aim to remove __weak entirely; it causes a number of weird
> > things like this and it'd be much easier to manage with a small amount of
> > ifdeffery.
> > 
> > Peter, thoughts?
> 
> Not a fan of __weak myself, after having had to deal with how the
> compilers actually make it work.
> 
> Where do I queue this? perf/urgent?

That'd be my preference, as arm64 is currently a bit broken and I'd
like 6.4 to be functional.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] perf/core: Drop __weak attribute from arch_perf_update_userpage() prototype

[Peter Zijlstra]

On Mon, Jun 12, 2023 at 04:03:24PM +0100, Marc Zyngier wrote:
> On Mon, 12 Jun 2023 15:54:23 +0100,
> Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > On Mon, Jun 12, 2023 at 02:16:28PM +0100, Mark Rutland wrote:
> > > On Sat, Jun 03, 2023 at 09:25:19AM +0100, Marc Zyngier wrote:
> > > > Reiji reports that the arm64 implementation of arch_perf_update_userpage()
> > > > is now ignored and replaced by the dummy stub in core code.
> > > > This seems to happen since the PMUv3 driver was moved to driver/perf.
> > > 
> > > I guess we should have a Cc stable then?
> > > 
> > > The below implies this has always been on dodgy ground, and so it's probably
> > > inaccurate to give this a Fixes tag pointing to the move.
> > > 
> > > > As it turns out, dropping the __weak attribute from the *prototype*
> > > > of the function solves the problem. You're right, this doesn't seem
> > > > to make much sense. And yet... It appears that both symbols get
> > > > flagged as weak, and that the first one to appear in the link order
> > > > wins:
> > > > 
> > > > $ nm drivers/perf/arm_pmuv3.o|grep arch_perf_update_userpage
> > > > 0000000000001db0 W arch_perf_update_userpage
> > > 
> > > Ah, so having it on th *declaration* will apply to any *definition*. :/
> > 
> > Yikes..
> > 
> > > That suggests this is a bad pattern generally, and we should probably remove
> > > the other __weak instances in headers. Lukcily it seems there aren't that many:
> > > 
> > > [mark@lakrids:~/src/linux]% git grep __weak -- **/*.h | wc -l
> > > 50
> > > 
> > > IMO we'd should aim to remove __weak entirely; it causes a number of weird
> > > things like this and it'd be much easier to manage with a small amount of
> > > ifdeffery.
> > > 
> > > Peter, thoughts?
> > 
> > Not a fan of __weak myself, after having had to deal with how the
> > compilers actually make it work.
> > 
> > Where do I queue this? perf/urgent?
> 
> That'd be my preference, as arm64 is currently a bit broken and I'd
> like 6.4 to be functional.

Can I get a Fixes tag?