[PATCH] checkpatch: warn about flushing system-wide workqueues

[PATCH] checkpatch: warn about flushing system-wide workqueues

[Tetsuo Handa]

Flushing the system-wide WQ has possibility of deadlock, for the caller
waits for completion of all works in that WQ even if the caller cannot
wait for completion of one of works due to locking dependency. Since
it is difficult to catch such attempts using lockdep, try to catch also
using checkpatch.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 scripts/checkpatch.pl | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 577e02998701..d114652ba837 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7226,6 +7226,13 @@ sub process {
 			     "Deprecated use of '$deprecated_api', prefer '$new_api' instead\n" . $herecurr);
 		}
 
+# check for flushing system-wide workqueues
+		if ($line =~ /\bflush_scheduled_work\b\s*\(/ || $line =~ /\bflush_workqueue\b\s*\(\s*\bsystem_wq\b\s*\)/ ||
+		    $line =~ /\bflush_workqueue\b\s*\(\s*\bsystem_(highpri|long|unbound|freezable|power_efficient|freezable_power_efficient)_wq\b\s*\)/) {
+			ERROR("DEPRECATED_API",
+			      "Flushing system-wide workqueues is dangerous and will be forbidden - see https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236\@I-love.SAKURA.ne.jp\n" . $herecurr);
+		}
+
 # check for various structs that are normally const (ops, kgdb, device_tree)
 # and avoid what seem like struct definitions 'struct foo {'
 		if (defined($const_structs) &&
-- 
2.34.1

Re: [PATCH] checkpatch: warn about flushing system-wide workqueues

[Joe Perches]

On Mon, 2022-04-25 at 08:31 +0900, Tetsuo Handa wrote:
> Flushing the system-wide WQ has possibility of deadlock, for the caller
> waits for completion of all works in that WQ even if the caller cannot
> wait for completion of one of works due to locking dependency. Since
> it is difficult to catch such attempts using lockdep, try to catch also
> using checkpatch.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7226,6 +7226,13 @@ sub process {
>  			     "Deprecated use of '$deprecated_api', prefer '$new_api' instead\n" . $herecurr);
>  		}
>  
> +# check for flushing system-wide workqueues
> +		if ($line =~ /\bflush_scheduled_work\b\s*\(/ || $line =~ /\bflush_workqueue\b\s*\(\s*\bsystem_wq\b\s*\)/ ||
> +		    $line =~ /\bflush_workqueue\b\s*\(\s*\bsystem_(highpri|long|unbound|freezable|power_efficient|freezable_power_efficient)_wq\b\s*\)/) {

Several of the uses of \b are not necessary.

And it's probably more readable using separate lines and it looks as
if the 3rd test is unnecessary as it could be combined with the 2nd.

		if ($line =~ /\bflush_scheduled_work\s*\(/ ||
		    $line =~ /\bflush_workqueue\s*\(\s*system(_\w*)?_wq\s*\)/) {

> +			ERROR("DEPRECATED_API",
> +			      "Flushing system-wide workqueues is dangerous and will be forbidden - see https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236\@I-love.SAKURA.ne.jp\n" . $herecurr);

Re: [PATCH] checkpatch: warn about flushing system-wide workqueues

[Tetsuo Handa]

On 2022/04/25 8:45, Joe Perches wrote:
> And it's probably more readable using separate lines and it looks as
> if the 3rd test is unnecessary as it could be combined with the 2nd.
> 
> 		if ($line =~ /\bflush_scheduled_work\s*\(/ ||
> 		    $line =~ /\bflush_workqueue\s*\(\s*system(_\w*)?_wq\s*\)/) {

We don't need to worry about possibility like

	flush_workqueue(system_module1_wq);

? Then, we can simplify like you suggested.

From 06a4c861eed2062ba58c3023ec28b1b3020bd1f8 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Mon, 25 Apr 2022 09:28:27 +0900
Subject: [PATCH v2] checkpatch: warn about flushing system-wide workqueues

Flushing the system-wide WQ has possibility of deadlock, for the caller
waits for completion of all works in that WQ even if the caller cannot
wait for completion of one of works due to locking dependency. Since
it is difficult to catch such attempts using lockdep, try to catch also
using checkpatch.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Changes in v2:
  Simplify matching, suggested by Joe Perches <joe@perches.com>

 scripts/checkpatch.pl | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 577e02998701..88491cfcfd59 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7226,6 +7226,13 @@ sub process {
 			     "Deprecated use of '$deprecated_api', prefer '$new_api' instead\n" . $herecurr);
 		}
 
+# check for flushing system-wide workqueues
+		if ($line =~ /\bflush_scheduled_work\s*\(/ ||
+		    $line =~ /\bflush_workqueue\s*\(\s*system(_\w*)?_wq\s*\)/) {
+			ERROR("DEPRECATED_API",
+			      "Flushing system-wide workqueues is dangerous and will be forbidden - see https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236\@I-love.SAKURA.ne.jp\n" . $herecurr);
+		}
+
 # check for various structs that are normally const (ops, kgdb, device_tree)
 # and avoid what seem like struct definitions 'struct foo {'
 		if (defined($const_structs) &&
-- 
2.34.1

Re: [PATCH] checkpatch: warn about flushing system-wide workqueues

[Tetsuo Handa]

On 2022/04/25 9:33, Tetsuo Handa wrote:
> On 2022/04/25 8:45, Joe Perches wrote:
>> And it's probably more readable using separate lines and it looks as
>> if the 3rd test is unnecessary as it could be combined with the 2nd.
>>
>> 		if ($line =~ /\bflush_scheduled_work\s*\(/ ||
>> 		    $line =~ /\bflush_workqueue\s*\(\s*system(_\w*)?_wq\s*\)/) {
> 
> We don't need to worry about possibility like
> 
> 	flush_workqueue(system_module1_wq);
> 
> ? Then, we can simplify like you suggested.

I initially thought that also doing static checks by scripts/checkpatch.pl
is better than only doing runtime WARN_ON(). But not all patches are checked
by scripts/checkpatch.pl . Thus, as an attempt to check without exemptions,
I now think that doing static checks via BUILD_BUG_ON() is better than
scripts/checkpatch.pl . I sent below patch to linux-next.git , and so far
it seems working (I mean, no build failure reports caused by compilers).

Subject: workqueue: Wrap flush_workqueue() using a macro

A conversion to stop flushing kernel-global workqueues is in progress.
Wrap flush_workqueue() and inject BUILD_BUG_ON() checks, in order to
prevent users who are not aware of this conversion from again start
flushing kernel-global workqueues.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 include/linux/workqueue.h | 21 +++++++++++++++++++++
 kernel/workqueue.c        |  1 +
 2 files changed, 22 insertions(+)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 7b13fae377e2e..9f78414d507c8 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -654,4 +654,25 @@ 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.
+ * Checks for system_wq and system_{long,unbound,highpri}_wq will be added after
+ * all in-tree users stopped flushing these workqueues.
+ */
+#define flush_workqueue(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 abcc9a2ac3197..5c612532f3e93 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2813,6 +2813,7 @@ static void warn_flush_attempt(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 = {
-- 

Tejun, can we use this approach? If yes, when to apply?

If we can include this patch into 5.18, can be applied as-is.
If we can include this patch into 5.19, can be applied with checks for
system_{long,unbound,highpri}_wq added because all flush_workqueue() users
on system_*_wq are gone in next-20220505.

Now that all flush_workqueue() users on system_*_wq are gone in next-20220505,
next step is to remove all flush_scheduled_work() users. Therefore, instead of
using /\bflush_workqueue\s*\(\s*system(_\w*)?_wq\s*\)/ in scripts/checkpatch.pl ,
I think we can ask BUILD_BUG_ON() for blocking new system_*_wq users, and ask
scripts/checkpatch.pl for blocking new system_wq users.

Re: [PATCH] checkpatch: warn about flushing system-wide workqueues

[Joe Perches]

On Thu, 2022-05-05 at 22:42 +0900, Tetsuo Handa wrote:
> On 2022/04/25 9:33, Tetsuo Handa wrote:
> > On 2022/04/25 8:45, Joe Perches wrote:
> > > And it's probably more readable using separate lines and it looks as
> > > if the 3rd test is unnecessary as it could be combined with the 2nd.
> > > 
> > > 		if ($line =~ /\bflush_scheduled_work\s*\(/ ||
> > > 		    $line =~ /\bflush_workqueue\s*\(\s*system(_\w*)?_wq\s*\)/) {
> > 
> > We don't need to worry about possibility like
> > 
> > 	flush_workqueue(system_module1_wq);
> > 
> > ? Then, we can simplify like you suggested.
> 
> I initially thought that also doing static checks by scripts/checkpatch.pl
> is better than only doing runtime WARN_ON(). But not all patches are checked
> by scripts/checkpatch.pl . Thus, as an attempt to check without exemptions,
> I now think that doing static checks via BUILD_BUG_ON() is better than
> scripts/checkpatch.pl . I sent below patch to linux-next.git , and so far
> it seems working (I mean, no build failure reports caused by compilers).
> 
> Subject: workqueue: Wrap flush_workqueue() using a macro
> 
> A conversion to stop flushing kernel-global workqueues is in progress.
> Wrap flush_workqueue() and inject BUILD_BUG_ON() checks, in order to
> prevent users who are not aware of this conversion from again start
> flushing kernel-global workqueues.
[]
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
[]
> @@ -2813,6 +2813,7 @@ static void warn_flush_attempt(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)

The #undef flush_workqueue could be removed by using

void (flush_workqueue)(struct workqueue_struct *wq)
{
	...
}

Re: [PATCH] checkpatch: warn about flushing system-wide workqueues

[Tejun Heo]

Hello,

On Thu, May 05, 2022 at 10:42:19PM +0900, Tetsuo Handa wrote:
> Tejun, can we use this approach? If yes, when to apply?
>
> If we can include this patch into 5.18, can be applied as-is.
> If we can include this patch into 5.19, can be applied with checks for
> system_{long,unbound,highpri}_wq added because all flush_workqueue() users
> on system_*_wq are gone in next-20220505.
> 
> Now that all flush_workqueue() users on system_*_wq are gone in next-20220505,
> next step is to remove all flush_scheduled_work() users. Therefore, instead of
> using /\bflush_workqueue\s*\(\s*system(_\w*)?_wq\s*\)/ in scripts/checkpatch.pl ,
> I think we can ask BUILD_BUG_ON() for blocking new system_*_wq users, and ask
> scripts/checkpatch.pl for blocking new system_wq users.

Given that we'll need runtime check anyway, why not resurrect the original
runtime warning but exempt flush_schedule_work() if that's the only thing
remaining right now (using a special flag or whatever)? If we're sure that
we aren't triggering it spuriously, we can ask Andrew to take the warning
patch into -mm so that it floats on top of everything else and gets pulled
into the trunk during the coming merge window.

Thanks.

-- 
tejun

Re: [PATCH] checkpatch: warn about flushing system-wide workqueues

[Tetsuo Handa]

On 2022/05/06 2:32, Tejun Heo wrote:
> Given that we'll need runtime check anyway, why not resurrect the original
> runtime warning but exempt flush_schedule_work() if that's the only thing
> remaining right now (using a special flag or whatever)?

Yes, we will also need runtime check for robustness, for we can't catch usage
like

	struct workqueue_struct *my_wq = alloc_workqueue();
	if (!my_wq)
		my_wq = system_long_wq;
	flush_workqueue(my_wq);

using compile time checks.

I found that it is not easy to trigger flush_workqueue() paths. For example,
several modules are using flush_workqueue() only upon module unloading.
Therefore, I'm trying to catch obvious flush_workqueue() paths at compile
time when possible.

>                                                         If we're sure that
> we aren't triggering it spuriously, we can ask Andrew to take the warning
> patch into -mm so that it floats on top of everything else and gets pulled
> into the trunk during the coming merge window.

OK, the coming merge window means 5.19.

The original runtime checking will be used anyway. Is "workqueue: Wrap
flush_workqueue() using a macro" OK for you as a compile time check?

Re: [PATCH] checkpatch: warn about flushing system-wide workqueues

[Tejun Heo]

Hello,

On Fri, May 06, 2022 at 08:29:07AM +0900, Tetsuo Handa wrote:
> On 2022/05/06 2:32, Tejun Heo wrote:
> > Given that we'll need runtime check anyway, why not resurrect the original
> > runtime warning but exempt flush_schedule_work() if that's the only thing
> > remaining right now (using a special flag or whatever)?
> 
> Yes, we will also need runtime check for robustness, for we can't catch usage
> like
> 
> 	struct workqueue_struct *my_wq = alloc_workqueue();
> 	if (!my_wq)
> 		my_wq = system_long_wq;
> 	flush_workqueue(my_wq);
> 
> using compile time checks.
> 
> I found that it is not easy to trigger flush_workqueue() paths. For example,
> several modules are using flush_workqueue() only upon module unloading.

Ah, yeah, good point.

> Therefore, I'm trying to catch obvious flush_workqueue() paths at compile
> time when possible.
> 
> >                                                         If we're sure that
> > we aren't triggering it spuriously, we can ask Andrew to take the warning
> > patch into -mm so that it floats on top of everything else and gets pulled
> > into the trunk during the coming merge window.
> 
> OK, the coming merge window means 5.19.
> 
> The original runtime checking will be used anyway. Is "workqueue: Wrap
> flush_workqueue() using a macro" OK for you as a compile time check?

Sounds good to me.

Thanks.

-- 
tejun

[PATCH v2] workqueue: Wrap flush_workqueue() using a macro

[Tetsuo Handa]

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 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        |  2 +-
 2 files changed, 32 insertions(+), 1 deletion(-)

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..071a4eb7fd94 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2794,7 +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.
  */
-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),
-- 
2.18.4

[PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[Tetsuo Handa]

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.1

Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[Joe Perches]

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.

> Changes in v2:
>   Use "void (flush_workqueue)(struct workqueue_struct *wq)" and remove
>   "#undef flush_workqueue", suggested by Joe Perches <joe@perches.com>.

Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[Rasmus Villemoes]

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

Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[Tejun Heo]

On 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

Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[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.

Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[Tejun Heo]

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

Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[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.

Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[Tejun Heo]

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

Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[Tetsuo Handa]

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.

Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[Tejun Heo]

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

Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[Tetsuo Handa]

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

Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[Tejun Heo]

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

Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[Tetsuo Handa]

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?

[PATCH v4] workqueue: Wrap flush_workqueue() using an inline function

[Tetsuo Handa]

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.4