Messages in this thread | | | Date | Fri, 20 May 2022 18:51:12 +0900 | Subject | Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro | From | 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@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.
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.
| |