Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[Tetsuo Handa]

On 2022/05/20 17:01, Tejun Heo wrote:
>> +/*
>> + * Detect attempt to flush system-wide workqueues at compile time when possible.
>> + * See https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@xxxxxxxxxxxxxxxxxxx for
>> + * reasons and steps for converting system-wide workqueues into local workqueues.
>> + */
>> +#define flush_workqueue(wq)						\
>> +({									\
>> +	BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_wq) &&	\
>> +			 &(wq) == &system_wq,				\
>> +			 "Please avoid flushing system_wq.");		\
> 
> It kinda bothers me that this causes a build failure. It'd be better if we
> can trigger #warning instead. I'm not sure whether there'd be a clean way to
> do it tho. Maybe just textual matching would provide similar coverage? How
> did you test this?

This does not cause a build failure, for this wrapping happens only if
flush_workqueue() appears between "#define flush_workqueue(wq)" and
"#undef flush_workqueue". Only flush_scheduled_work() in include/linux/workqueue.h
calls flush_workqueue(system_wq), and flush_scheduled_work() is defined
before the "#define flush_workqueue(wq)" is defined.

And use of #warning directive breaks building with -Werror option.

> 
>>  #endif
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index 0d2514b4ff0d..08255c332e4b 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -2794,6 +2794,7 @@ static bool flush_workqueue_prep_pwqs(struct workqueue_struct *wq,
>>   * This function sleeps until all work items which were queued on entry
>>   * have finished execution, but it is not livelocked by new incoming ones.
>>   */
>> +#undef flush_workqueue
>>  void flush_workqueue(struct workqueue_struct *wq)
> 
> Maybe rename the function to __flush_workqueue() instead of undef'ing the
> macro?

I prefer not adding __ prefix, for flush_workqueue() is meant as a public function.
For easier life of kernel message parsers, I don't feel reason to dare to rename.

But if you still prefer renaming, I will change flush_workqueue() as an inline function
in include/linux/workqueue.h which calls __flush_workqueue() in kernel/workqueue.c.