Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 12 Jun 2023 15:16:27 +0100
Message-ID: <86sfawbwas.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Mark Rutland <mark.rutland@arm.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        linux-kernel@vger.kernel.org, Reiji Watanabe <reijiw@google.com>,
        Will Deacon <will@kernel.org>
Subject: Re: [PATCH] perf/core: Drop __weak attribute from arch_perf_update_userpage() prototype
In-Reply-To: <ZIcarHx0qbfC2iyy@FVFF77S0Q05N>
References: <20230603082519.1088285-1-maz@kernel.org>
        <ZIcarHx0qbfC2iyy@FVFF77S0Q05N>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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.