Re: [PATCH v2 2/2] perf: arm_pmuv3: Don't use PMCCNTR_EL0 on SMT cores

[Will Deacon <will@kernel.org>]

On Fri, Sep 19, 2025 at 06:27:54PM +0800, Yicong Yang wrote:
> On 2025/9/19 17:16, Mark Rutland wrote:
> > On Fri, Sep 19, 2025 at 04:56:18PM +0800, Yicong Yang wrote:
> >> On 2025/9/18 21:32, Will Deacon wrote:
> >>> On Wed, Aug 20, 2025 at 04:45:34PM +0800, Yicong Yang wrote:
> > 
> >>>> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> >>>> index 5c310e803dd7..137ef55d6973 100644
> >>>> --- a/drivers/perf/arm_pmu.c
> >>>> +++ b/drivers/perf/arm_pmu.c
> >>>> @@ -901,6 +901,9 @@ struct arm_pmu *armpmu_alloc(void)
> >>>>  
> >>>>  		events = per_cpu_ptr(pmu->hw_events, cpu);
> >>>>  		events->percpu_pmu = pmu;
> >>>> +
> >>>> +		if (!pmu->has_smt && topology_core_has_smt(cpu))
> >>>> +			pmu->has_smt = true;
> >>>
> >>> Why isn't that just:
> >>>
> >>> 	pmu->has_smt = topology_core_has_smt(cpu);
> >>>
> >>> ?
> >>
> >> also works. since one pmu only contains one type of CPU, so just thought
> >> no need to set it multiple times.
> >>
> >>> but then if that's the case, why do we need to stash the result in the
> >>> PMU at all?
> >>
> >> should based on the discussion here [1]. stash it during probe will avoid
> >> calling {raw_}smp_processor_id() in pmu::event_init() which may be
> >> horrible for debug in some condition.
> >>
> >> [1] https://lore.kernel.org/linux-arm-kernel/aJsV7nzlILHd_ZMa@J2N7QTR9R3/
> > 
> > This isn't about being 'horrible for debug'; my comment there was saying
> > that the proposed patch was incorrect AND it would be horrible to debug
> > that in practice when it inevitably went wrong.
> > 
> > The key details are:
> > 
> > (1) We need pmu::event_init() to know whether the cycle counter can be
> >     used such that it doesn't permit a group to be created which can
> >     *NEVER* be scheduled in hardware. Otherwise, the core perf code will
> >     waste time periodically trying to schedule that group when it will
> >     *ALWAYS* be rejected by pmu::add().
> > 
> > (2) The pmu::event_init() call runs in a preemptible context and can
> >     run on any CPU in the system, completely independent of the PMU's
> >     supported CPUs. Thus [raw_]smp_processor_id() tells you nothing
> >     about the CPU(s) the event will run on.
> > 
> >     Note that for task-bound events, the event->cpu is -1, so that
> >     doesn't tell us either. Only the PMU instance tells us the set of
> >     CPUs.
> > 
> 
> yes this is the problem in the last approach using [raw_]smp_processor_id()
> in pmu::event_init().. sorry for the wrong information replied above and
> thanks for help me recall this..
> 
> > We can solve that by either stashing this boolean flag at probe time OR
> > having pmu::event_init() check something like:
> > 
> > 	topology_core_has_smt(cpumask_first(pmu->supported_cpus));
> > 
> 
> this works. I didn't think of this approach... pmu->supported_cpus may contain
> offline CPUs but it doesn't matter since topology_core_has_smt() can also
> retieve the SMT implementation for offline CPU.
> 
> > ... and I think stashing at probe time is nicer/clearer.
> > 
> 
> I feel similar. will wait for Will's comments :)

Seems like it's two against one, so we'll go with pmu->has_smt and I'll
live with it.

Will