lkml.org 
[lkml]   [2022]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro
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

\
 
 \ /
  Last update: 2022-05-20 13:13    [W:0.095 / U:0.240 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site