[PATCH] scsi/fcoe: simplify fcoe_select_cpu()

[PATCH] scsi/fcoe: simplify fcoe_select_cpu()

[Yury Norov]

cpumask_next() followed by cpumask_first() opencodes
cpumask_next_wrap(). Fix it.

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 drivers/scsi/fcoe/fcoe.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index b911fdb387f3..07eddafe52ff 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1312,10 +1312,7 @@ static inline unsigned int fcoe_select_cpu(void)
 {
 	static unsigned int selected_cpu;
 
-	selected_cpu = cpumask_next(selected_cpu, cpu_online_mask);
-	if (selected_cpu >= nr_cpu_ids)
-		selected_cpu = cpumask_first(cpu_online_mask);
-
+	selected_cpu = cpumask_next_wrap(selected_cpu, cpu_online_mask);
 	return selected_cpu;
 }
 
-- 
2.43.0

Re: [PATCH] scsi/fcoe: simplify fcoe_select_cpu()

[Bart Van Assche]

On 6/5/25 7:42 AM, Yury Norov wrote:
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index b911fdb387f3..07eddafe52ff 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -1312,10 +1312,7 @@ static inline unsigned int fcoe_select_cpu(void)
>   {
>   	static unsigned int selected_cpu;
>   
> -	selected_cpu = cpumask_next(selected_cpu, cpu_online_mask);
> -	if (selected_cpu >= nr_cpu_ids)
> -		selected_cpu = cpumask_first(cpu_online_mask);
> -
> +	selected_cpu = cpumask_next_wrap(selected_cpu, cpu_online_mask);
>   	return selected_cpu;
>   }

Why does this algorithm occur in the FCoE driver? Isn't
WORK_CPU_UNBOUND good enough for this driver? And if it isn't
good enough, shouldn't this kind of functionality be integrated in
kernel/workqueue.c rather than having the above algorithm in a
kernel driver?

Thanks,

Bart.

Re: [PATCH] scsi/fcoe: simplify fcoe_select_cpu()

[Yury Norov]

+ Tejun, Lai

On Thu, Jun 05, 2025 at 08:13:53AM +0800, Bart Van Assche wrote:
> On 6/5/25 7:42 AM, Yury Norov wrote:
> > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> > index b911fdb387f3..07eddafe52ff 100644
> > --- a/drivers/scsi/fcoe/fcoe.c
> > +++ b/drivers/scsi/fcoe/fcoe.c
> > @@ -1312,10 +1312,7 @@ static inline unsigned int fcoe_select_cpu(void)
> >   {
> >   	static unsigned int selected_cpu;
> > -	selected_cpu = cpumask_next(selected_cpu, cpu_online_mask);
> > -	if (selected_cpu >= nr_cpu_ids)
> > -		selected_cpu = cpumask_first(cpu_online_mask);
> > -
> > +	selected_cpu = cpumask_next_wrap(selected_cpu, cpu_online_mask);
> >   	return selected_cpu;
> >   }
> 
> Why does this algorithm occur in the FCoE driver? Isn't
> WORK_CPU_UNBOUND good enough for this driver? And if it isn't
> good enough, shouldn't this kind of functionality be integrated in
> kernel/workqueue.c rather than having the above algorithm in a
> kernel driver?

(I'm obviously not an expert in this driver, and just wanted to cleanup
the cpumask API usage.)

It looks like the intention is to distribute the workload among CPUs
sequentially. If you move this function out of the driver, someone
else may call the function, and sequential distribution may get
broken.

If sequential distribution doesn't matter here, and the real
intention is just to distribute workload more or less evenly,
we already have cpumask_any_distribute() for this.

Thanks,
Yury

Re: [PATCH] scsi/fcoe: simplify fcoe_select_cpu()

[Hannes Reinecke]

On 6/5/25 02:35, Yury Norov wrote:
> + Tejun, Lai
> 
> On Thu, Jun 05, 2025 at 08:13:53AM +0800, Bart Van Assche wrote:
>> On 6/5/25 7:42 AM, Yury Norov wrote:
>>> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
>>> index b911fdb387f3..07eddafe52ff 100644
>>> --- a/drivers/scsi/fcoe/fcoe.c
>>> +++ b/drivers/scsi/fcoe/fcoe.c
>>> @@ -1312,10 +1312,7 @@ static inline unsigned int fcoe_select_cpu(void)
>>>    {
>>>    	static unsigned int selected_cpu;
>>> -	selected_cpu = cpumask_next(selected_cpu, cpu_online_mask);
>>> -	if (selected_cpu >= nr_cpu_ids)
>>> -		selected_cpu = cpumask_first(cpu_online_mask);
>>> -
>>> +	selected_cpu = cpumask_next_wrap(selected_cpu, cpu_online_mask);
>>>    	return selected_cpu;
>>>    }
>>
>> Why does this algorithm occur in the FCoE driver? Isn't
>> WORK_CPU_UNBOUND good enough for this driver? And if it isn't
>> good enough, shouldn't this kind of functionality be integrated in
>> kernel/workqueue.c rather than having the above algorithm in a
>> kernel driver?
> 
> (I'm obviously not an expert in this driver, and just wanted to cleanup
> the cpumask API usage.)
> 
> It looks like the intention is to distribute the workload among CPUs
> sequentially. If you move this function out of the driver, someone
> else may call the function, and sequential distribution may get
> broken.
> 
> If sequential distribution doesn't matter here, and the real
> intention is just to distribute workload more or less evenly,
> we already have cpumask_any_distribute() for this.
> 
This function is used to distribute incoming skbs onto a work
cpu. And it's actually quite pointless, as the skb already
has a field (skb->sk->sk_incoming_cpu) which tells you exactly
on which CPU this skb was received, so we should use that
here.

I'll send a patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich