include/linux/workqueue.h | 31 +++++++++++++++++++++++++++++++ kernel/workqueue.c | 1 + 2 files changed, 32 insertions(+)
Since flush operation synchronously waits for completion, flushing
system-wide WQs (e.g. system_wq) might introduce possibility of deadlock
due to unexpected locking dependency. Tejun Heo commented at [1] that it
makes no sense at all to call flush_workqueue() on the shared WQs as the
caller has no idea what it's gonna end up waiting for.
Although there is flush_scheduled_work() which flushes system_wq WQ with
"Think twice before calling this function! It's very easy to get into
trouble if you don't take great care." warning message, syzbot found a
circular locking dependency caused by flushing system_wq WQ [2].
Therefore, let's change the direction to that developers had better use
their local WQs if flush_scheduled_work()/flush_workqueue(system_*_wq) is
inevitable.
Steps for converting system-wide WQs into local WQs are explained at [3],
and a conversion to stop flushing system-wide WQs is in progress. Now we
want some mechanism for preventing developers who are not aware of this
conversion from again start flushing system-wide WQs.
Since I found that WARN_ON() is complete but awkward approach for teaching
developers about this problem, let's use BUILD_BUG_ON() for incomplete but
handy approach. For completeness, we will also insert WARN_ON() into
flush_workqueue() after all users stopped calling flush_scheduled_work().
Link: https://lore.kernel.org/all/YgnQGZWT%2Fn3VAITX@slm.duckdns.org/ [1]
Link: https://syzkaller.appspot.com/bug?extid=bde0f89deacca7c765b8 [2]
Link: https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp [3]
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Changes in v3:
Revert suggested change in v2, for kernel test robot <lkp@intel.com> found
warning: Function parameter or member 'flush_workqueue' not described in 'void'
warning: expecting prototype for flush_workqueue(). Prototype was for void() instead
when built with W=1 option.
Changes in v2:
Use "void (flush_workqueue)(struct workqueue_struct *wq)" and remove
"#undef flush_workqueue", suggested by Joe Perches <joe@perches.com>.
This patch is expected to be sent to linux.git immediately before the merge
window for 5.19 closes, for we need to wait until patches for removing
flush_workqueue(system_*_wq) calls reached linux.git during the merge window
for 5.19. Due to this ordering dependency, the kernel build bots complain like
https://lkml.kernel.org/r/202205021448.6SzhD1Cz-lkp@intel.com , but it seems
that simply ignoring these reports is the best choice.
If it is inconvenient for you to send pull request twice from your wq.git tree
(once for immediately after 5.18 is released, the other for immediately before
the merge window for 5.19 closes), I can send pull request for this patch from
my tree. In that case, please give me Acked-by: or Reviewed-by: response regarding
this patch.
include/linux/workqueue.h | 31 +++++++++++++++++++++++++++++++
kernel/workqueue.c | 1 +
2 files changed, 32 insertions(+)
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 7fee9b6cfede..5bad503925de 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -663,4 +663,35 @@ int workqueue_offline_cpu(unsigned int cpu);
void __init workqueue_init_early(void);
void __init workqueue_init(void);
+/*
+ * 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."); \
+ BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_highpri_wq) && \
+ &(wq) == &system_highpri_wq, \
+ "Please avoid flushing system_highpri_wq."); \
+ BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_long_wq) && \
+ &(wq) == &system_long_wq, \
+ "Please avoid flushing system_long_wq."); \
+ BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_unbound_wq) && \
+ &(wq) == &system_unbound_wq, \
+ "Please avoid flushing system_unbound_wq."); \
+ BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_freezable_wq) && \
+ &(wq) == &system_freezable_wq, \
+ "Please avoid flushing system_freezable_wq."); \
+ BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_power_efficient_wq) && \
+ &(wq) == &system_power_efficient_wq, \
+ "Please avoid flushing system_power_efficient_wq."); \
+ BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_freezable_power_efficient_wq) && \
+ &(wq) == &system_freezable_power_efficient_wq, \
+ "Please avoid flushing system_freezable_power_efficient_wq."); \
+ flush_workqueue(wq); \
+})
+
#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)
{
struct wq_flusher this_flusher = {
--
2.34.1On Mon, May 16, 2022 at 02:00:37PM +0900, Tetsuo Handa wrote:
> This patch is expected to be sent to linux.git immediately before the merge
> window for 5.19 closes, for we need to wait until patches for removing
> flush_workqueue(system_*_wq) calls reached linux.git during the merge window
> for 5.19. Due to this ordering dependency, the kernel build bots complain like
> https://lkml.kernel.org/r/202205021448.6SzhD1Cz-lkp@intel.com , but it seems
> that simply ignoring these reports is the best choice.
>
> If it is inconvenient for you to send pull request twice from your wq.git tree
> (once for immediately after 5.18 is released, the other for immediately before
> the merge window for 5.19 closes), I can send pull request for this patch from
> my tree. In that case, please give me Acked-by: or Reviewed-by: response regarding
> this patch.
What we can do is sending the patch to Andrew. The akpm patches float above
-next and get applied the last. I won't need any special treatment.
> +/*
> + * 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?
> #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?
Thanks.
--
tejun
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.
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
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.
Hello,
On Fri, May 20, 2022 at 08:43:41PM +0900, Tetsuo Handa wrote:
> 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.
Are we fully sure? Also, there can be other changes in flight which aren't
covered. It's just not nice in general to intentionally trigger build
failures without an easy way to remediate it.
> 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.
What I'm trying to say is that, if we can trigger build warnings, that'd be
a better way to go about it.
> We will also remove flush_scheduled_work() after
> all flush_scheduled_work() users are gone.
Yeah, that'd be great.
> >> 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.
Hmm.... there's __compiletime_warning() which uses gcc's __warning__
attribute, so we can define a function which is tagged with it and call it
on constant_p match. Would that work?
> >>> 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.
I think it'd be fine to rename the function.
Thanks.
--
tejun
On 2022/05/21 2:10, Tejun Heo wrote: > On Fri, May 20, 2022 at 08:43:41PM +0900, Tetsuo Handa wrote: >> 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. > > Are we fully sure? Also, there can be other changes in flight which aren't > covered. It's just not nice in general to intentionally trigger build > failures without an easy way to remediate it. Yes, we are fully sure. Subset of this patch is already in linux-next.git without problems. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220520&id=5015b3b61696f8f44e7113e5bc14f4a20cbf57ff There aren't other changes in flight which aren't covered. I believe that it is safe to replace the commit above with this patch when Linus released 5.18 final (or maybe 5.18-rc8) is released next Sunday. I also believe that it is safe to send this patch right before Linus releases 5.19-rc1. I guess that there are several out-of-tree kernel modules which will start failing with this patch. But they can use #undef flush_workqueue as a temporary workaround (if they can't remediate easily) until we add WARN_ON() as a run-time check. We will need to wait for several months until we can add WARN_ON() as a run-time check, for that happens after all flush_scheduled_work() users are gone. >> 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. > > What I'm trying to say is that, if we can trigger build warnings, that'd be > a better way to go about it. Some unlucky users (if any) can workaround this build failure using #undef. Nothing to bother with how to emit warning messages instead of error messages.
On Sat, May 21, 2022 at 10:14:36AM +0900, Tetsuo Handa wrote: > On 2022/05/21 2:10, Tejun Heo wrote: > > On Fri, May 20, 2022 at 08:43:41PM +0900, Tetsuo Handa wrote: > >> 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. > > > > Are we fully sure? Also, there can be other changes in flight which aren't > > covered. It's just not nice in general to intentionally trigger build > > failures without an easy way to remediate it. > > Yes, we are fully sure. Subset of this patch is already in linux-next.git without problems. > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220520&id=5015b3b61696f8f44e7113e5bc14f4a20cbf57ff > There aren't other changes in flight which aren't covered. > > I believe that it is safe to replace the commit above with this patch when Linus released > 5.18 final (or maybe 5.18-rc8) is released next Sunday. I also believe that it is safe to > send this patch right before Linus releases 5.19-rc1. > > I guess that there are several out-of-tree kernel modules which will start > failing with this patch. But they can use > > #undef flush_workqueue > > as a temporary workaround (if they can't remediate easily) until we add WARN_ON() > as a run-time check. We will need to wait for several months until we can add > WARN_ON() as a run-time check, for that happens after all flush_scheduled_work() > users are gone. How is that better tho? If we can just trigger build warning, that's way better than asking people to hunt down some random define and shoot it down. How would they do that? > >> 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. > > > > What I'm trying to say is that, if we can trigger build warnings, that'd be > > a better way to go about it. > > Some unlucky users (if any) can workaround this build failure using #undef. > Nothing to bother with how to emit warning messages instead of error messages. We're talking in circles. If we can trigger warning, I don't see a reason why we'd want to trigger build failures. It's a really bad user experience for anybody who doesn't know what is going on exactly. So, nack on the current patch. Thanks. -- tejun
On 2022/05/21 13:57, Tejun Heo wrote:
> How is that better tho? If we can just trigger build warning, that's way
> better than asking people to hunt down some random define and shoot it down.
> How would they do that?
Subset of this patch already in linux-next.git without problems suggests that
in-tree kernel modules (including the ones which will be added during next merge
window) will not hit build failures with this patch.
We don't take care of build failures for out-of-tree kernel modules, but
some out-of-tree kernel module which cannot rewrite in a timely manner can
workaround by just adding a #undef line. Patching out-of-tree kernel module is
easier than patching .config or Makefile or include/linux/workqueue.h .
> We're talking in circles. If we can trigger warning, I don't see a reason
> why we'd want to trigger build failures. It's a really bad user experience
> for anybody who doesn't know what is going on exactly. So, nack on the
> current patch.
I wonder if we can trigger warning using __compiletime_warning, for init/Kconfig
recommends CONFIG_WERROR=y (i.e. __compiletime_warning == __compiletime_error).
----------
config WERROR
bool "Compile the kernel with warnings as errors"
default COMPILE_TEST
help
A kernel build should not cause any compiler warnings, and this
enables the '-Werror' flag to enforce that rule by default.
However, if you have a new (or very old) compiler with odd and
unusual warnings, or you have some architecture with problems,
you may need to disable this config option in order to
successfully build the kernel.
If in doubt, say Y.
----------
Anyway, something like below updated approach?
---
include/linux/workqueue.h | 49 ++++++++++++++++++++++++++++++++++++---
kernel/workqueue.c | 9 +++++++
2 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 7fee9b6cfede..243d87fc0b85 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -563,15 +563,21 @@ static inline bool schedule_work(struct work_struct *work)
return queue_work(system_wq, work);
}
+/*
+ * 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.
+ */
+extern void __warn_flushing_systemwide_wq(void)
+ __compiletime_warning("Please avoid flushing system-wide workqueues.");
+
/**
* flush_scheduled_work - ensure that any scheduled work has run to completion.
*
* Forces execution of the kernel-global workqueue and blocks until its
* completion.
*
- * Think twice before calling this function! It's very easy to get into
- * trouble if you don't take great care. Either of the following situations
- * will lead to deadlock:
+ * It's very easy to get into trouble if you don't take great care.
+ * Either of the following situations will lead to deadlock:
*
* One of the work items currently on the workqueue needs to acquire
* a lock held by your code or its caller.
@@ -586,6 +592,10 @@ static inline bool schedule_work(struct work_struct *work)
* need to know that a particular work item isn't queued and isn't running.
* In such cases you should use cancel_delayed_work_sync() or
* cancel_work_sync() instead.
+ *
+ * Please stop calling this function! A conversion to stop flushing system-wide
+ * workqueues is in progress. This function will be removed after all in-tree
+ * users stopped calling this function.
*/
static inline void flush_scheduled_work(void)
{
@@ -663,4 +673,37 @@ int workqueue_offline_cpu(unsigned int cpu);
void __init workqueue_init_early(void);
void __init workqueue_init(void);
+/*
+ * Detect attempt to flush system-wide workqueues at compile time when possible.
+ *
+ * Always warn, for there is no in-tree flush_workqueue(system_*_wq) user.
+ */
+#define flush_workqueue(wq) \
+do { \
+ if ((__builtin_constant_p(&(wq) == &system_wq) && \
+ &(wq) == &system_wq) || \
+ (__builtin_constant_p(&(wq) == &system_highpri_wq) && \
+ &(wq) == &system_highpri_wq) || \
+ (__builtin_constant_p(&(wq) == &system_long_wq) && \
+ &(wq) == &system_long_wq) || \
+ (__builtin_constant_p(&(wq) == &system_unbound_wq) && \
+ &(wq) == &system_unbound_wq) || \
+ (__builtin_constant_p(&(wq) == &system_freezable_wq) && \
+ &(wq) == &system_freezable_wq) || \
+ (__builtin_constant_p(&(wq) == &system_power_efficient_wq) && \
+ &(wq) == &system_power_efficient_wq) || \
+ (__builtin_constant_p(&(wq) == &system_freezable_power_efficient_wq) && \
+ &(wq) == &system_freezable_power_efficient_wq)) \
+ __warn_flushing_systemwide_wq(); \
+ flush_workqueue(wq); \
+} while (0)
+
+/*
+ * Warn only if emitting warning message does not cause build failure and
+ * the developer wants warning about possibility of deadlock.
+ */
+#if !defined(CONFIG_WERROR) && defined(CONFIG_PROVE_LOCKING)
+#define flush_scheduled_work() flush_workqueue(system_wq)
+#endif
+
#endif
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 0d2514b4ff0d..3391e0d10f90 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)
{
struct wq_flusher this_flusher = {
@@ -6111,3 +6112,11 @@ void __init workqueue_init(void)
wq_online = true;
wq_watchdog_init();
}
+
+/*
+ * Despite the naming, this is a no-op function which is here only for avoiding
+ * link error. Since compile-time warning may fail to catch, we will need to
+ * emit run-time warning from flush_workqueue().
+ */
+void __warn_flushing_systemwide_wq(void) { }
+EXPORT_SYMBOL(__warn_flushing_systemwide_wq);
--
2.18.4
On Sat, May 21, 2022 at 08:37:19PM +0900, Tetsuo Handa wrote:
> +/*
> + * Detect attempt to flush system-wide workqueues at compile time when possible.
> + *
> + * Always warn, for there is no in-tree flush_workqueue(system_*_wq) user.
> + */
> +#define flush_workqueue(wq) \
> +do { \
> + if ((__builtin_constant_p(&(wq) == &system_wq) && \
> + &(wq) == &system_wq) || \
> + (__builtin_constant_p(&(wq) == &system_highpri_wq) && \
> + &(wq) == &system_highpri_wq) || \
> + (__builtin_constant_p(&(wq) == &system_long_wq) && \
> + &(wq) == &system_long_wq) || \
> + (__builtin_constant_p(&(wq) == &system_unbound_wq) && \
> + &(wq) == &system_unbound_wq) || \
> + (__builtin_constant_p(&(wq) == &system_freezable_wq) && \
> + &(wq) == &system_freezable_wq) || \
> + (__builtin_constant_p(&(wq) == &system_power_efficient_wq) && \
> + &(wq) == &system_power_efficient_wq) || \
> + (__builtin_constant_p(&(wq) == &system_freezable_power_efficient_wq) && \
> + &(wq) == &system_freezable_power_efficient_wq)) \
> + __warn_flushing_systemwide_wq(); \
> + flush_workqueue(wq); \
> +} while (0)
Please just rename the backend function.
Thanks.
--
tejun
Linus, I'm planning to send a patch during this merge window for announcing
that flushing system-wide workqueues should be avoided. So far I made three
patterns shown below.
Pattern 1: Use BUILD_BUG_ON() for flush_workqueue(system_*_wq), nothing for flush_scheduled_work().
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220520&id=5015b3b61696f8f44e7113e5bc14f4a20cbf57ff
Pattern 2: Use __compiletime_warning() for both flush_workqueue(system_*_wq) and flush_scheduled_work().
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220523&id=84baad17cb8286b6b53b675f8c3d7343ee6a990c
Pattern 3: Use __compiletime_warning() for both flush_workqueue(system_*_wq) and flush_scheduled_work().
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20220524&id=e449c388913ccd36641f7cc0c335029a7cc161f4
Unfortunately, there is no way to emit compiletime message as neither
warning nor error. Any approach that uses compiler attributes causes warning,
but not everybody use approach that doesn't use compiler attributes.
Not only init/Kconfig recommends CONFIG_WERROR=y (i.e. recommends
__compiletime_warning == __compiletime_error), commit 771c035372a036f8
("deprecate the '__deprecated' attribute warnings entirely and for good")
also discourages use of compiler warning. Thus, I can't tell which way to go.
In this merge window, all flush_workqueue(system_*_wq) users and some of
flush_scheduled_work() users will be removed. But I worry that new users
come in before I complete removing the remaining flush_scheduled_work() users.
Actually, https://syzkaller.appspot.com/bug?extid=bde0f89deacca7c765b8 was
an example of such new users found after I announced at
https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp .
I somehow want to teach in-tree, to-be-in-tree and out-of-tree users
about this change.
Must I drop __compiletime_warning() for flush_scheduled_work() part if I want to
send this change before all in-tree flush_scheduled_work() users are removed?
Since flush operation synchronously waits for completion, flushing
system-wide WQs (e.g. system_wq) might introduce possibility of deadlock
due to unexpected locking dependency. Tejun Heo commented at [1] that it
makes no sense at all to call flush_workqueue() on the shared WQs as the
caller has no idea what it's gonna end up waiting for.
Although there is flush_scheduled_work() which flushes system_wq WQ with
"Think twice before calling this function! It's very easy to get into
trouble if you don't take great care." warning message, syzbot found a
circular locking dependency caused by flushing system_wq WQ [2].
Therefore, let's change the direction to that developers had better use
their local WQs if flush_scheduled_work()/flush_workqueue(system_*_wq) is
inevitable.
Steps for converting system-wide WQs into local WQs are explained at [3],
and a conversion to stop flushing system-wide WQs is in progress. Now we
want some mechanism for preventing developers who are not aware of this
conversion from again start flushing system-wide WQs.
Since I found that WARN_ON() is complete but awkward approach for teaching
developers about this problem, let's use __compiletime_warning() for
incomplete but handy approach. For completeness, we will also insert
WARN_ON() into __flush_workqueue() after all users stopped calling
flush_scheduled_work().
Link: https://lore.kernel.org/all/YgnQGZWT%2Fn3VAITX@slm.duckdns.org/ [1]
Link: https://syzkaller.appspot.com/bug?extid=bde0f89deacca7c765b8 [2]
Link: https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp [3]
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Since all flush_workqueue(system_*_wq) users are removed by now, and I removed
flush_scheduled_work() part from this patch, this patch is ready to go to linux.git.
Changes in v4:
It turned out that attempt to emit warning message to flush_scheduled_work() users
based on "!defined(CONFIG_WERROR)" does not work, for Talla, RavitejaX Goud
<ravitejax.goud.talla@intel.com> found that one of modules which call
flush_scheduled_work() locally applies -Werror option.
Therefore, convert BUILD_BUG_ON() to __compiletime_warning() and
rename the backend function to __flush_workqueue().
Changes in v3:
Revert suggested change in v2, for kernel test robot <lkp@intel.com> found
warning: Function parameter or member 'flush_workqueue' not described in 'void'
warning: expecting prototype for flush_workqueue(). Prototype was for void() instead
when built with W=1 option.
Changes in v2:
Use "void (flush_workqueue)(struct workqueue_struct *wq)" and remove
"#undef flush_workqueue", suggested by Joe Perches <joe@perches.com>.
include/linux/workqueue.h | 51 ++++++++++++++++++++++++++++++++++-----
kernel/workqueue.c | 16 +++++++++---
2 files changed, 57 insertions(+), 10 deletions(-)
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 7fee9b6cfede..3d63104a41b7 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -445,7 +445,7 @@ extern bool mod_delayed_work_on(int cpu, struct workqueue_struct *wq,
struct delayed_work *dwork, unsigned long delay);
extern bool queue_rcu_work(struct workqueue_struct *wq, struct rcu_work *rwork);
-extern void flush_workqueue(struct workqueue_struct *wq);
+extern void __flush_workqueue(struct workqueue_struct *wq);
extern void drain_workqueue(struct workqueue_struct *wq);
extern int schedule_on_each_cpu(work_func_t func);
@@ -563,15 +563,23 @@ static inline bool schedule_work(struct work_struct *work)
return queue_work(system_wq, work);
}
+/*
+ * 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.
+ */
+extern void __warn_flushing_systemwide_wq(void)
+ __compiletime_warning("Please avoid flushing system-wide workqueues.");
+
/**
* flush_scheduled_work - ensure that any scheduled work has run to completion.
*
* Forces execution of the kernel-global workqueue and blocks until its
* completion.
*
- * Think twice before calling this function! It's very easy to get into
- * trouble if you don't take great care. Either of the following situations
- * will lead to deadlock:
+ * It's very easy to get into trouble if you don't take great care.
+ * Either of the following situations will lead to deadlock:
*
* One of the work items currently on the workqueue needs to acquire
* a lock held by your code or its caller.
@@ -586,10 +594,41 @@ static inline bool schedule_work(struct work_struct *work)
* need to know that a particular work item isn't queued and isn't running.
* In such cases you should use cancel_delayed_work_sync() or
* cancel_work_sync() instead.
+ *
+ * Please stop calling this function! A conversion to stop flushing system-wide
+ * workqueues is in progress. This function will be removed after all in-tree
+ * users stopped calling this function.
+ */
+static inline void __deprecated flush_scheduled_work(void)
+{
+ __flush_workqueue(system_wq);
+}
+
+/**
+ * flush_workqueue - ensure that any scheduled work has run to completion.
+ * @wq: workqueue to flush
+ *
+ * This function sleeps until all work items which were queued on entry
+ * have finished execution, but it is not livelocked by new incoming ones.
*/
-static inline void flush_scheduled_work(void)
+static __always_inline void flush_workqueue(struct workqueue_struct *wq)
{
- flush_workqueue(system_wq);
+ if ((__builtin_constant_p(wq == system_wq) &&
+ wq == system_wq) ||
+ (__builtin_constant_p(wq == system_highpri_wq) &&
+ wq == system_highpri_wq) ||
+ (__builtin_constant_p(wq == system_long_wq) &&
+ wq == system_long_wq) ||
+ (__builtin_constant_p(wq == system_unbound_wq) &&
+ wq == system_unbound_wq) ||
+ (__builtin_constant_p(wq == system_freezable_wq) &&
+ wq == system_freezable_wq) ||
+ (__builtin_constant_p(wq == system_power_efficient_wq) &&
+ wq == system_power_efficient_wq) ||
+ (__builtin_constant_p(wq == system_freezable_power_efficient_wq) &&
+ wq == system_freezable_power_efficient_wq))
+ __warn_flushing_systemwide_wq();
+ __flush_workqueue(wq);
}
/**
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 4056f2a3f9d5..1ea50f6be843 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2788,13 +2788,13 @@ static bool flush_workqueue_prep_pwqs(struct workqueue_struct *wq,
}
/**
- * flush_workqueue - ensure that any scheduled work has run to completion.
+ * __flush_workqueue - ensure that any scheduled work has run to completion.
* @wq: workqueue to flush
*
* This function sleeps until all work items which were queued on entry
* have finished execution, but it is not livelocked by new incoming ones.
*/
-void flush_workqueue(struct workqueue_struct *wq)
+void __flush_workqueue(struct workqueue_struct *wq)
{
struct wq_flusher this_flusher = {
.list = LIST_HEAD_INIT(this_flusher.list),
@@ -2943,7 +2943,7 @@ void flush_workqueue(struct workqueue_struct *wq)
out_unlock:
mutex_unlock(&wq->mutex);
}
-EXPORT_SYMBOL(flush_workqueue);
+EXPORT_SYMBOL(__flush_workqueue);
/**
* drain_workqueue - drain a workqueue
@@ -2971,7 +2971,7 @@ void drain_workqueue(struct workqueue_struct *wq)
wq->flags |= __WQ_DRAINING;
mutex_unlock(&wq->mutex);
reflush:
- flush_workqueue(wq);
+ __flush_workqueue(wq);
mutex_lock(&wq->mutex);
@@ -6111,3 +6111,11 @@ void __init workqueue_init(void)
wq_online = true;
wq_watchdog_init();
}
+
+/*
+ * Despite the naming, this is a no-op function which is here only for avoiding
+ * link error. Since compile-time warning may fail to catch, we will need to
+ * emit run-time warning from __flush_workqueue().
+ */
+void __warn_flushing_systemwide_wq(void) { }
+EXPORT_SYMBOL(__warn_flushing_systemwide_wq);
--
2.18.4On Mon, 2022-05-16 at 14:00 +0900, Tetsuo Handa wrote: [] > Changes in v3: > Revert suggested change in v2, for kernel test robot <lkp@intel.com> found > > warning: Function parameter or member 'flush_workqueue' not described in 'void' > warning: expecting prototype for flush_workqueue(). Prototype was for void() instead > > when built with W=1 option. Odd, perhaps that not a valid error message and is a defect in gcc's parsing of function definitions. > Changes in v2: > Use "void (flush_workqueue)(struct workqueue_struct *wq)" and remove > "#undef flush_workqueue", suggested by Joe Perches <joe@perches.com>.
On 16/05/2022 09.18, Joe Perches wrote: > On Mon, 2022-05-16 at 14:00 +0900, Tetsuo Handa wrote: > [] >> Changes in v3: >> Revert suggested change in v2, for kernel test robot <lkp@intel.com> found >> >> warning: Function parameter or member 'flush_workqueue' not described in 'void' >> warning: expecting prototype for flush_workqueue(). Prototype was for void() instead >> >> when built with W=1 option. > > Odd, perhaps that not a valid error message and is a defect > in gcc's parsing of function definitions. Nah, gcc isn't buggy in such a fundamental part of parsing C. It's not a warning from gcc, nor from sparse (or smatch). It's from the perl script scripts/kernel-doc . Rasmus
© 2016 - 2026 Red Hat, Inc.