Re: [PATCH v6] 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/3/2026 11:55 AM, Guo, Wangyang wrote:
> 
> Why not use original topology_sibling_cpumask(cpu) if snapshot not
> available?
>> -		siblmsk = topology_sibling_cpumask(cpu);
>> +		siblmsk = sibling_snapshot ? sibling_snapshot[cpu] : cpumask_of(cpu);
> 
As Andrew noted, topology_sibling_cpumask() changes during CPU hotplug, 
so using it as the fallback could change the grouping. cpumask_of(cpu) 
deliberately drops SMT preference when no snapshot is available, keeping 
grouping independent of the online CPU set.

> 
>> - * Return: true if success, false otherwise.
>> + * Return: 1 if successful, 0 if inapplicable, or -ENOMEM on allocation failure.
>>    */
>> -static bool __try_group_cluster_cpus(unsigned int ncpus,
>> +static int __try_group_cluster_cpus(unsigned int ncpus,
> 
> If use ncluster as ret: >= 1 successful, 0 inapplicable, and -ENOMEM for
> error.
> the code should become simpler.

Sure, I will include this change in next patch revision.

>>   	ncluster = alloc_cluster_groups(ncpus, ngroups, node_cpumask, nmsk,
>>   					&clusters, &cluster_groups);
>>
>> -	if (ncluster == 0)
>> +	ret = ncluster;
>> +	if (ncluster <= 0)
>>   		goto fail_no_clusters;
>>
>>   	for (i = 0; i < ncluster; i++) {
>> @@ -392,12 +477,11 @@ static bool __try_group_cluster_cpus(unsigned int ncpus,
>>   		assign_cpus_to_groups(nc, nmsk, nv, masks, curgrp, last_grp);
>>   	}
>>
>> -	ret = true;
>> +	ret = 1;
>>   	kfree(cluster_groups);
>>   	kfree(clusters);
>>    fail_no_clusters:
>>   	free_cpumask_var(nmsk);
>> - fail_nmsk_alloc:
>                return ncluster;
>>   	return ret;
>>   }
> 
> 
>> @@ -442,6 +526,7 @@ static int __group_cpus_evenly(unsigned int startgrp, unsigned int numgrps,
>> -		if (__try_group_cluster_cpus(ncpus, nv->ngroups, nmsk,
>> -					     masks, &curgrp, last_grp)) {
>> +		ret = __try_group_cluster_cpus(ncpus, nv->ngroups, nmsk,
>> +					       masks, &curgrp, last_grp);
>> +		if (ret < 0) {
>> +			kfree(node_groups);
>> +			return ret;
>> +		}
>> +		if (ret) {
>>   			done += nv->ngroups;
>>   			continue;
>>   		}
> 
> There is duplication for "done += nv->ngroups", my suggestion:
>      If (ret < 0) { kfree(node_groups); return ret;}
>      elIf (!ret) assign_cpus_to_groups(ncpus, nmsk, nv, masks, &curgrp, last_grp);
>      done += nv->ngroups;
> }  // end of "for" loop
Thanks for the suggestion. I'll consolidate the done increment in the 
next revision.