Re: [PATCH v2 01/18] scsi: Expand all create*_workqueue() invocations

[Damien Le Moal <dlemoal@kernel.org>]

On 8/20/24 02:17, Bart Van Assche wrote:
> On 8/18/24 4:25 PM, Damien Le Moal wrote:
>> On 8/17/24 06:55, Bart Van Assche wrote:
>>> diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
>>> index 1078c20c5ef6..f49783b89d04 100644
>>> --- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
>>> +++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
>>> @@ -2363,8 +2363,8 @@ static int _bnx2fc_create(struct net_device *netdev,
>>>   	interface->vlan_id = vlan_id;
>>>   	interface->tm_timeout = BNX2FC_TM_TIMEOUT;
>>>   
>>> -	interface->timer_work_queue =
>>> -			create_singlethread_workqueue("bnx2fc_timer_wq");
>>> +	interface->timer_work_queue = alloc_ordered_workqueue(
>>> +		"%s", WQ_MEM_RECLAIM, "bnx2fc_timer_wq");
>>
>> Very odd line split. And there are a few more like this one. Maybe your patch
>> needs some manual tuning after running the script ?
>>
>> The patch overall looks good to me, but it would be nice to have consistency in
>> the line splitting. Personnally, I prefer the pattern such as:
>>
>> -	kmpath_rdacd = create_singlethread_workqueue("kmpath_rdacd");
>> +	kmpath_rdacd =
>> +		alloc_ordered_workqueue("%s", WQ_MEM_RECLAIM, "kmpath_rdacd");
>>
>> instead of:
>>
>> -	lio_wq = create_singlethread_workqueue("efct_lio_worker");
>> +	lio_wq = alloc_ordered_workqueue("%s", WQ_MEM_RECLAIM,
>> +					 "efct_lio_worker");
>>
>> Though I guess that is a matter of taste :)
> 
> (reduced cc-list)
> 
> If I run "git clang-format HEAD^" on this patch, no code is changed. 
> Does this perhaps mean that the .clang-format style file in the kernel
> tree needs further tuning? The most recent change in that file other
> than adding for-each macro names is from two years ago (see also commit 
> 781121a7f6d1 ("clang-format: Fix space after for_each macros")). Or does
> this perhaps mean that there is broad agreement about the coding style
> parameters in the .clang-format file?

As I said, most likely a matter of taste :)
The pattern:

	lio_wq = alloc_ordered_workqueue("%s", WQ_MEM_RECLAIM,
					 "efct_lio_worker");

follows the regular kernel coding style.
I only meant to say that I find the pattern:

	lio_wq =
		alloc_ordered_workqueue("%s", WQ_MEM_RECLAIM, "efct_lio_worker");

more pleasing visually. But the line may be too long anyway...

> 
> Thanks,
> 
> Bart.
> 
> 

-- 
Damien Le Moal
Western Digital Research