Re: [PATCH] sched/psi: use for_each_set_bit() in psi_group_change() task-count walk

[Usama Arif <usama.arif@linux.dev>]

On 15/07/2026 04:49, K Prateek Nayak wrote:
> Hello Usama,
> 
> On 7/14/2026 7:50 PM, Usama Arif wrote:
>> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
>> index d9c9d9480a45..f5ae1ceb21a7 100644
>> --- a/kernel/sched/psi.c
>> +++ b/kernel/sched/psi.c
>> @@ -798,7 +798,8 @@ static void psi_group_change(struct psi_group *group, int cpu,
>>  			     u64 now, bool wake_clock)
>>  {
>>  	struct psi_group_cpu *groupc;
>> -	unsigned int t, m;
>> +	unsigned long clear_bits, set_bits;
>> +	unsigned int t;
>>  	u32 state_mask;
>>  
>>  	lockdep_assert_rq_held(cpu_rq(cpu));
>> @@ -824,9 +825,8 @@ static void psi_group_change(struct psi_group *group, int cpu,
>>  	 * The rest of the state mask is calculated based on the task
>>  	 * counts. Update those first, then construct the mask.
>>  	 */
>> -	for (t = 0, m = clear; m; m &= ~(1 << t), t++) {
>> -		if (!(m & (1 << t)))
>> -			continue;
>> +	clear_bits = clear;
> 
> nit.
> 
> Can't we convert the arguments to unsigned long instead of assigning
> them to these local variables?

I kind of prefer the explicit conversion. Also all the callers take int
instead of long. The printk would also change from %x to %lx.

The assignment itself should hopefully be free? The compiler hopefully
optimizes it away.

> 
> Apart form that, for_each_set_bit() is indeed much cleaner. Feel free
> to include:
> 
> Reviewed-by: K Prateek Nayak <kprateek.nayak@amd.com>
> 

Thanks for the review!

>> +	for_each_set_bit(t, &clear_bits, NR_PSI_TASK_COUNTS) {
>>  		if (groupc->tasks[t]) {
>>  			groupc->tasks[t]--;
>>  		} else if (!psi_bug) {
>