Re: [PATCH v7] lib/group_cpus: Snapshot cluster masks to keep grouping hotplug invariant

["Badole, Vishal" <vishal.badole@amd.com>]

Hello Wangyang,
Thanks for reviewing this patch.

On 11/12/2026 7:50 AM, Guo, Wangyang wrote:
> On Wed, 11 Nov 2026 01:46:11, Vishal Badole <Vishal.Badole@amd.com> wrote:
>>   static int alloc_cluster_groups(unsigned int ncpus,
>>   				unsigned int ngroups,
>>   				struct cpumask *node_cpumask,
>> @@ -299,6 +380,9 @@ static int alloc_cluster_groups(unsigned int ncpus,
>>   	const struct cpumask **clusters;
>>   	struct node_groups *cluster_groups;
>>
>> +	if (!cluster_snapshot)
>> +		goto no_cluster;
>> +
>>   	cpumask_copy(msk, node_cpumask);
>>
>>   	/* Probe how many clusters in this node. */
>> @@ -307,7 +391,7 @@ static int alloc_cluster_groups(unsigned int ncpus,
>>   		if (cpu >= nr_cpu_ids)
>>   			break;
>>
>> -		cluster_mask = topology_cluster_cpumask(cpu);
>> +		cluster_mask = cluster_snapshot[cpu];
>>   		if (!cpumask_weight(cluster_mask))
>>   			goto no_cluster;
>>   		/* Clean out CPUs on the same cluster. */
>> @@ -322,7 +406,7 @@ static int alloc_cluster_groups(unsigned int ncpus,
>>   	/* Allocate memory based on cluster number. */
>>   	clusters = kzalloc_objs(*clusters, ncluster);
>>   	if (!clusters)
>> -		goto no_cluster;
>> +		return -ENOMEM;
>>   	cluster_groups = kzalloc_objs(struct node_groups, ncluster);
>>   	if (!cluster_groups)
>>   		goto fail_cluster_groups;
>> @@ -331,7 +415,7 @@ static int alloc_cluster_groups(unsigned int ncpus,
>>   	cpumask_copy(msk, node_cpumask);
>>   	for (n = 0; n < ncluster; n++) {
>>   		cpu = cpumask_first(msk);
>> -		cluster_mask = topology_cluster_cpumask(cpu);
>> +		cluster_mask = cluster_snapshot[cpu];
>>   		nc = cpumask_weight_and(cluster_mask, node_cpumask);
>>   		clusters[n] = cluster_mask;
>>   		cluster_groups[n].id = n;
>> @@ -347,6 +431,7 @@ static int alloc_cluster_groups(unsigned int ncpus,
>>
>>    fail_cluster_groups:
>>   	kfree(clusters);
>> +	return -ENOMEM;
>>    no_cluster:
>>   	return 0;
>>   }
>> @@ -354,9 +439,9 @@ static int alloc_cluster_groups(unsigned int ncpus,
>>   /*
>>    * Try group CPUs evenly for cluster locality within a NUMA node.
>>    *
>> - * Return: true if success, false otherwise.
>> + * Return: cluster count on success, 0 if inapplicable, or -ENOMEM on failure.
>>    */
>> -static bool __try_group_cluster_cpus(unsigned int ncpus,
>> +static int __try_group_cluster_cpus(unsigned int ncpus,
>>   				     unsigned int ngroups,
>>   				     struct cpumask *node_cpumask,
>>   				     struct cpumask *masks,
>> @@ -365,18 +450,17 @@ static bool __try_group_cluster_cpus(unsigned int ncpus,
>>   {
>>   	struct node_groups *cluster_groups;
>>   	const struct cpumask **clusters;
>> -	unsigned int ncluster;
>> -	bool ret = false;
>> +	int ncluster;
>>   	cpumask_var_t nmsk;
>>   	unsigned int i, nc;
> 
> If we could move "if (!cluster_snapshot)" from alloc_cluster_groups() to
> here, the unnecessary cpumask alloc/free could be avoid if snapshot if
> unavailable.
> 
Noted; I’ll address this in the next patch revision.

>>   	if (!zalloc_cpumask_var(&nmsk, GFP_KERNEL))
>> -		goto fail_nmsk_alloc;
>> +		return -ENOMEM;
>>
>>   	ncluster = alloc_cluster_groups(ncpus, ngroups, node_cpumask, nmsk,
>>   					&clusters, &cluster_groups);
>>
>> -	if (ncluster == 0)
>> +	if (ncluster <= 0)
>>   		goto fail_no_clusters;
>>
>>   	for (i = 0; i < ncluster; i++) {
>> @@ -392,13 +476,11 @@ static bool __try_group_cluster_cpus(unsigned int ncpus,
>>   		assign_cpus_to_groups(nc, nmsk, nv, masks, curgrp, last_grp);
>>   	}
>>
>> -	ret = true;
>>   	kfree(cluster_groups);
>>   	kfree(clusters);
>>    fail_no_clusters:
>>   	free_cpumask_var(nmsk);
>> - fail_nmsk_alloc:
>> -	return ret;
>> +	return ncluster;
>>   }
>>