A common pattern in the Linux kernel is that sprintf(), snprintf() or
kasprintf() is used to format the workqueue name that is passed to
create_workqueue(), create_freezable_workqueue() or
create_singlethread_workqueue(). Prepare for simplifying such code by
introducing the create*_workqueue2() macros that accept a printf-style
format string and argument list. A later patch will remove the
create*_workqueue() macros and will rename the create*_workqueue2()
macros into create*_workqueue().

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/linux/workqueue.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index d9968bfc8eac..762aaedaba56 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -525,11 +525,20 @@ alloc_workqueue(const char *fmt, unsigned int flags, int max_active, ...);
 
 #define create_workqueue(name)						\
 	alloc_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, 1, (name))
+#define create_workqueue2(fmt, args...) \
+	alloc_workqueue(fmt, __WQ_LEGACY | WQ_MEM_RECLAIM, 1, ##args)
 #define create_freezable_workqueue(name)				\
 	alloc_workqueue("%s", __WQ_LEGACY | WQ_FREEZABLE | WQ_UNBOUND |	\
 			WQ_MEM_RECLAIM, 1, (name))
+#define create_freezable_workqueue2(fmt, args...)                               \
+	alloc_workqueue(                                                       \
+		fmt, __WQ_LEGACY | WQ_FREEZABLE | WQ_UNBOUND | WQ_MEM_RECLAIM, \
+		1, ##args)
 #define create_singlethread_workqueue(name)				\
 	alloc_ordered_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, name)
+#define create_singlethread_workqueue2(fmt, args...)			\
+	alloc_ordered_workqueue(fmt, __WQ_LEGACY | WQ_MEM_RECLAIM, ##args)
+
 
 #define from_work(var, callback_work, work_fieldname)	\
 	container_of(callback_work, typeof(*var), work_fieldname)

Hello

On Mon, Jul 1, 2024 at 6:29 AM Bart Van Assche <bvanassche@acm.org> wrote:

> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -525,11 +525,20 @@ alloc_workqueue(const char *fmt, unsigned int flags, int max_active, ...);
>
>  #define create_workqueue(name)                                         \
>         alloc_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, 1, (name))
> +#define create_workqueue2(fmt, args...) \
> +       alloc_workqueue(fmt, __WQ_LEGACY | WQ_MEM_RECLAIM, 1, ##args)
>  #define create_freezable_workqueue(name)                               \
>         alloc_workqueue("%s", __WQ_LEGACY | WQ_FREEZABLE | WQ_UNBOUND | \
>                         WQ_MEM_RECLAIM, 1, (name))

Is there any possible preprocessor hack to avoid the renaming of the functions?

Thanks
Lai

On 6/30/24 7:51 PM, Lai Jiangshan wrote:
> On Mon, Jul 1, 2024 at 6:29 AM Bart Van Assche <bvanassche@acm.org> wrote:
> 
>> --- a/include/linux/workqueue.h
>> +++ b/include/linux/workqueue.h
>> @@ -525,11 +525,20 @@ alloc_workqueue(const char *fmt, unsigned int flags, int max_active, ...);
>>
>>   #define create_workqueue(name)                                         \
>>          alloc_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, 1, (name))
>> +#define create_workqueue2(fmt, args...) \
>> +       alloc_workqueue(fmt, __WQ_LEGACY | WQ_MEM_RECLAIM, 1, ##args)
>>   #define create_freezable_workqueue(name)                               \
>>          alloc_workqueue("%s", __WQ_LEGACY | WQ_FREEZABLE | WQ_UNBOUND | \
>>                          WQ_MEM_RECLAIM, 1, (name))
> 
> Is there any possible preprocessor hack to avoid the renaming of the functions?

Thanks Lai for having taken a look. As one can see here the last patch 
of this patch series renames create_workqueue2() back to 
create_workqueue(): 
https://lore.kernel.org/linux-kernel/20240630222904.627462-1-bvanassche@acm.org/

Bart.

On Mon, Jul 01, 2024 at 09:42:50AM -0700, Bart Van Assche wrote:
> On 6/30/24 7:51 PM, Lai Jiangshan wrote:
> > On Mon, Jul 1, 2024 at 6:29 AM Bart Van Assche <bvanassche@acm.org> wrote:
> > 
> > > --- a/include/linux/workqueue.h
> > > +++ b/include/linux/workqueue.h
> > > @@ -525,11 +525,20 @@ alloc_workqueue(const char *fmt, unsigned int flags, int max_active, ...);
> > > 
> > >   #define create_workqueue(name)                                         \
> > >          alloc_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, 1, (name))
> > > +#define create_workqueue2(fmt, args...) \
> > > +       alloc_workqueue(fmt, __WQ_LEGACY | WQ_MEM_RECLAIM, 1, ##args)
> > >   #define create_freezable_workqueue(name)                               \
> > >          alloc_workqueue("%s", __WQ_LEGACY | WQ_FREEZABLE | WQ_UNBOUND | \
> > >                          WQ_MEM_RECLAIM, 1, (name))
> > 
> > Is there any possible preprocessor hack to avoid the renaming of the functions?
> 
> Thanks Lai for having taken a look. As one can see here the last patch of
> this patch series renames create_workqueue2() back to create_workqueue(): https://lore.kernel.org/linux-kernel/20240630222904.627462-1-bvanassche@acm.org/

This can be done with the preprocessor to detect how many arguments
are being used, so then there is no need to do the renaming passes and
conversions can land via subsystems:


diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index fb3993894536..00420f85b881 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -523,13 +523,24 @@ alloc_workqueue(const char *fmt, unsigned int flags, int max_active, ...);
 #define alloc_ordered_workqueue(fmt, flags, args...)			\
 	alloc_workqueue(fmt, WQ_UNBOUND | __WQ_ORDERED | (flags), 1, ##args)
 
-#define create_workqueue(name)						\
-	alloc_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, 1, (name))
-#define create_freezable_workqueue(name)				\
-	alloc_workqueue("%s", __WQ_LEGACY | WQ_FREEZABLE | WQ_UNBOUND |	\
-			WQ_MEM_RECLAIM, 1, (name))
-#define create_singlethread_workqueue(name)				\
-	alloc_ordered_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, name)
+#define __has_one_arg(args...)  __is_defined(COUNT_ARGS(args))
+
+#define __create_workqueue1(flags, name) \
+                alloc_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM | (flags), \
+				1, name)
+#define __create_workqueue0(flags, fmt, args...) \
+                alloc_workqueue(fmt, __WQ_LEGACY | WQ_MEM_RECLAIM | (flags), \
+				1, args)
+
+#define create_workqueue(args...) \
+        CONCATENATE(__create_workqueue, __has_one_arg(args))\
+                (0, args)
+#define create_freezeable_workqueue(args...) \
+        CONCATENATE(__create_workqueue, __has_one_arg(args))\
+                (WQ_UNBOUND | WQ_FREEZABLE, args)
+#define create_singlethread_workqueue(args...) \
+        CONCATENATE(__create_workqueue, __has_one_arg(args))\
+                (WQ_UNBOUND | __WQ_ORDERED, args)
 
 #define from_work(var, callback_work, work_fieldname)	\
 	container_of(callback_work, typeof(*var), work_fieldname)



Now conversions are one step, e.g.:

diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
index d74e829de51c..f7f27c6f1a15 100644
--- a/fs/smb/client/smbdirect.c
+++ b/fs/smb/client/smbdirect.c
@@ -1458,8 +1458,7 @@ static int allocate_caches_and_workqueue(struct smbd_connection *info)
 	if (!info->response_mempool)
 		goto out3;
 
-	scnprintf(name, MAX_NAME_LEN, "smbd_%p", info);
-	info->workqueue = create_workqueue(name);
+	info->workqueue = create_workqueue("smbd_%p", info);
 	if (!info->workqueue)
 		goto out4;
 
-- 
Kees Cook

On 7/3/24 1:24 PM, Kees Cook wrote:
> This can be done with the preprocessor to detect how many arguments
> are being used, so then there is no need to do the renaming passes and
> conversions can land via subsystems:

Thanks Kees, this is very useful feedback.

As one can see here, Tejun requested not to add support for a format
string in the create*_workqueue() macros:
https://lore.kernel.org/linux-kernel/ZoMF1ZydZUusxRcf@slm.duckdns.org/

Hence a different approach for the SCSI create*_workqueue() macros:
https://lore.kernel.org/linux-scsi/20240702215228.2743420-1-bvanassche@acm.org/

Best regards,

Bart.

On Wed, Jul 03, 2024 at 02:28:34PM -0700, Bart Van Assche wrote:
> On 7/3/24 1:24 PM, Kees Cook wrote:
> > This can be done with the preprocessor to detect how many arguments
> > are being used, so then there is no need to do the renaming passes and
> > conversions can land via subsystems:
> 
> Thanks Kees, this is very useful feedback.
> 
> As one can see here, Tejun requested not to add support for a format
> string in the create*_workqueue() macros:
> https://lore.kernel.org/linux-kernel/ZoMF1ZydZUusxRcf@slm.duckdns.org/

Ah! I should have read the thread more fully. :)

> Hence a different approach for the SCSI create*_workqueue() macros:
> https://lore.kernel.org/linux-scsi/20240702215228.2743420-1-bvanassche@acm.org/

Gotcha. Okay, well, that's a lot of flags to open-code, but I guess
that's fine? :P

-- 
Kees Cook

Hello,

On Wed, Jul 03, 2024 at 03:20:17PM -0700, Kees Cook wrote:
> > Hence a different approach for the SCSI create*_workqueue() macros:
> > https://lore.kernel.org/linux-scsi/20240702215228.2743420-1-bvanassche@acm.org/
> 
> Gotcha. Okay, well, that's a lot of flags to open-code, but I guess
> that's fine? :P

The flag in question is WQ_MEM_RECLAIM and that flag has a direct cost of
creating a dedicated kthread which mostly sits idle, so we want it to be
explicit. In SCSI, the naive expansion is likely to be correct for most
cases.

It's a bit tricky in that the failure mode of incorrectly missing the flag
is critical (system deadlock under memory pressure) but can be difficult to
trigger and the depencency chain may not be immediately evident when looking
at the code. The upsides are that when it happens, it's usually not too
difficult to tell from the backtraces and lockdep annotation sometimes helps
with finding missing cases (which is suppressed when using the old interface
because it's unclear whether MEM_RECLAIM is there intentionally or not).

Anyways, yeah, we want it to be explicit.

Thanks.

-- 
tejun