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@xxxxxxxxxxxxxxxxxxx
+ * 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