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

[Yicong Yang]

On 2024/10/18 20:48, Jonathan Cameron wrote:
> On Fri, 18 Oct 2024 17:57:39 +0800
> Yicong Yang <yangyicong at huawei.com> wrote:
> 
>> From: Yicong Yang <yangyicong at 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 at hisilicon.com>
> 
> One trivial comment inline otherwise LGTM
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron at 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.

I mean the newly onlined CPU hasn't been "tested" by us. Will refine the comments.

Thanks.

> 
>> +	 */
>> +	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);
>> +	}
> .
>