Re: [PATCH v3 2/8] drivers/perf: hisi: Improve the detection of associated CPUs

[Will Deacon <will@kernel.org>]

On Sat, Oct 26, 2024 at 03:24:18PM +0800, Yicong Yang wrote:
> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> Currently the associated CPUs are detected in the cpuhp online
> callback. If the CPU's sccl_id or the ccl_id matches the PMU's,
> they're associated. There's an exception that some PMUs locate
> on the SICL and will match no CPUs. The events of these PMUs
> can be opened on any online CPUs. To handle this we just check
> whether the PMU's sccl_id is -1, if so we know it locates on
> SICL and make any CPU associated to it.
> 
> This can be tweaked so in this patch just do the below changes:
> - If the PMU doesn't match any CPU then associated it to online CPUs
> - Choose the target CPU according to the NUMA affinity for opening
>   events
> 
> The function is implemented by hisi_pmu_init_associated_cpus() and
> invoked in hisi_pmu_init().
> 
> Also the associated_cpus are maintained with all the online CPUs. This
> is redundant since we'll always schedule the events on the online CPUs.
> Get rid of this and make associated_cpus contain offline CPUs as well.

I don't really understand the rationale for this change. Why is the new
behaviour better than the old one?

> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/perf/hisilicon/hisi_uncore_pmu.c | 56 +++++++++++++++++++-----
>  drivers/perf/hisilicon/hisi_uncore_pmu.h |  5 +++
>  2 files changed, 49 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
> index 416f72a813fc..c3549e16e0c3 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
> @@ -399,6 +399,27 @@ void hisi_uncore_pmu_disable(struct pmu *pmu)
>  }
>  EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_disable, HISI_PMU);
>  
> +static void hisi_pmu_init_associated_cpus(struct hisi_pmu *hisi_pmu)
> +{
> +	/*
> +	 * If the associated_cpus has already been initialized, for example
> +	 * determined by comparing the sccl_id and ccl_id with the CPU's
> +	 * mpidr_el1, then do nothing here. Otherwise the PMU has no affinity
> +	 * and could be opened on any online CPU.
> +	 */
> +	if (!cpumask_empty(&hisi_pmu->associated_cpus))
> +		return;
> +
> +	cpumask_copy(&hisi_pmu->associated_cpus, cpu_online_mask);

Is it always safe to access 'cpu_online_mask' here?

Will