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

[Tejun Heo <tj@kernel.org>]

Hello,

On Fri, May 20, 2022 at 06:51:12PM +0900, Tetsuo Handa wrote:
> 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@I-love.SAKURA.ne.jp 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.

What I mean is that if there's a file which didn't get tested or another
pull request which raced and that thing flushes one of the system_wq's,
it'll trigger a build error instead of a warning, which is a bit of an
overkill.

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

If the user wants to fail build on warnings, sure. That's different from
kernel failing to build in a way which may require non-trivial changes to
fix.

> > 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.

You mean the WARN_ON messages? Given how they never trigger, I doubt there's
much to break. Maybe some kprobe users? But they can survive.

> 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.

Please just do something straight forward.

Thanks.

-- 
tejun