Messages in this thread | | | Date | Fri, 20 May 2022 01:11:07 -1000 | From | Tejun Heo <> | Subject | Re: [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
| |