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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 18 Oct 2024 17:57:39 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> Currently we detect the associated CPUs in the cpuhp online callback.
Should really avoid we in patch descriptions.

Currently associated CPUs are detected in the cphup online callback.
etc

> 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 we maintained the associated_cpus 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.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

One trivial comment inline otherwise LGTM

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

>  /*
>   * The Super CPU Cluster (SCCL) and CPU Cluster (CCL) IDs can be
> @@ -446,10 +467,6 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu)
>  {
>  	int sccl_id, ccl_id;
>  
> -	/* If SCCL_ID is -1, the PMU is in a SICL and has no CPU affinity */
> -	if (hisi_pmu->sccl_id == -1)
> -		return true;
> -
>  	if (hisi_pmu->ccl_id == -1) {
>  		/* If CCL_ID is -1, the PMU only shares the same SCCL */
>  		hisi_read_sccl_and_ccl_id(&sccl_id, NULL);
> @@ -467,13 +484,29 @@ int hisi_uncore_pmu_online_cpu(unsigned int cpu, struct hlist_node *node)
>  	struct hisi_pmu *hisi_pmu = hlist_entry_safe(node, struct hisi_pmu,
>  						     node);
>  
> -	if (!hisi_pmu_cpu_is_associated_pmu(hisi_pmu))
> -		return 0;
> +	/*
> +	 * If the CPU is not in the associated_cpus, it maybe a new CPU we didn't
> +	 * access. Test whether it's associated or not.
it maybe a new CPU.  Test whether...

I'm not sure what the "didn't access" adds.

> +	 */
> +	if (!cpumask_test_cpu(cpu, &hisi_pmu->associated_cpus)) {
> +		if (!hisi_pmu_cpu_is_associated_pmu(hisi_pmu))
> +			return 0;
> +
> +		/*
> +		 * We found an associated CPU so we don't need to use the dummy
> +		 * associated CPUs. Update it.
> +		 */
> +		if (hisi_pmu->dummy_associated_cpus) {
> +			cpumask_clear(&hisi_pmu->associated_cpus);
> +			hisi_pmu->dummy_associated_cpus = false;
> +		}
>  
> -	cpumask_set_cpu(cpu, &hisi_pmu->associated_cpus);
> +		cpumask_set_cpu(cpu, &hisi_pmu->associated_cpus);
> +	}