Messages in this thread | | | Date | Fri, 20 May 2022 20:43:41 +0900 | Subject | Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro | From | Tetsuo Handa <> |
| |
On 2022/05/20 20:11, Tejun Heo wrote: >>> 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.
All flush_workqueue(system_*_wq) users are gone in linux-next.git, and this patch is for preventing new flush_workqueue(system_*_wq) users from coming in.
Therefore, triggering a build error (by sending this patch to linux.git right before 5.19-rc1 in order to make sure that developers will not use flush_workqueue(system_*_wq) again) is what this patch is for.
We will also remove flush_scheduled_work() after all flush_scheduled_work() users are gone.
> >> 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.
How can #warning directive be utilized inside #define or inline function, for we can't do like
#define flush_workqueue(wq) \ #if wq == "system_wq" \ #warning Please avoid flushing system_wq. \ #endif \ __flush_workqueue(wq)
or
static inline void flush_workqueue(struct workqueue_struct *wq) { #if wq == "system_wq" #warning Please avoid flushing system_wq. #endif __flush_workqueue(wq); }
. We can use BUiLD_BUG_ON() but I don't think we can use #warning directive.
> >>> 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.
WARN_ON() by passing system-wide workqueues should not happen. But backtrace of a warning message while inside __flush_workqueue() will be still possible.
| |