2024-02-09 00:12:11

by Tejun Heo

[permalink] [raw]
Subject: [PATCH 1/2 wq/for-6.9] workqueue: Implement workqueue_set_min_active()

Since 5797b1c18919 ("workqueue: Implement system-wide nr_active enforcement
for unbound workqueues"), unbound workqueues have separate min_active which
sets the number of interdependent work items that can be handled. This value
is currently initialized to WQ_DFL_MIN_ACTIVE which is 8. This isn't high
enough for some users, let's add an interface to adjust the setting.

Signed-off-by: Tejun Heo <[email protected]>
---
include/linux/workqueue.h | 2 ++
kernel/workqueue.c | 27 +++++++++++++++++++++++++++
2 files changed, 29 insertions(+)

--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -553,6 +553,8 @@ extern bool flush_rcu_work(struct rcu_wo

extern void workqueue_set_max_active(struct workqueue_struct *wq,
int max_active);
+extern void workqueue_set_min_active(struct workqueue_struct *wq,
+ int min_active);
extern struct work_struct *current_work(void);
extern bool current_is_workqueue_rescuer(void);
extern bool workqueue_congested(int cpu, struct workqueue_struct *wq);
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -5627,6 +5627,33 @@ void workqueue_set_max_active(struct wor
EXPORT_SYMBOL_GPL(workqueue_set_max_active);

/**
+ * workqueue_set_min_active - adjust min_active of an unbound workqueue
+ * @wq: target unbound workqueue
+ * @min_active: new min_active value
+ *
+ * Set min_active of an unbound workqueue. Unlike other types of workqueues, an
+ * unbound workqueue is not guaranteed to be able to process max_active
+ * interdependent work items. Instead, an unbound workqueue is guaranteed to be
+ * able to process min_active number of interdependent work items which is
+ * %WQ_DFL_MIN_ACTIVE by default.
+ *
+ * Use this function to adjust the min_active value between 0 and the current
+ * max_active.
+ */
+void workqueue_set_min_active(struct workqueue_struct *wq, int min_active)
+{
+ /* min_active is only meaningful for non-ordered unbound workqueues */
+ if (WARN_ON((wq->flags & (WQ_BH | WQ_UNBOUND | __WQ_ORDERED)) !=
+ WQ_UNBOUND))
+ return;
+
+ mutex_lock(&wq->mutex);
+ wq->saved_min_active = clamp(min_active, 0, wq->saved_max_active);
+ wq_adjust_max_active(wq);
+ mutex_unlock(&wq->mutex);
+}
+
+/**
* current_work - retrieve %current task's work struct
*
* Determine if %current task is a workqueue worker and what it's working on.


2024-02-09 00:14:30

by Tejun Heo

[permalink] [raw]
Subject: [PATCH 2/2 wq/for-6.9] async: Use a dedicated unbound workqueue with raised min_active

Async can schedule a number of interdependent work items. However, since
5797b1c18919 ("workqueue: Implement system-wide nr_active enforcement for
unbound workqueues"), unbound workqueues have separate min_active which sets
the number of interdependent work items that can be handled. This default
value is 8 which isn't sufficient for async and can lead to stalls during
resume from suspend in some cases.

Let's use a dedicated unbound workqueue with raised min_active.

Signed-off-by: Tejun Heo <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Reported-by: Marek Szyprowski <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
---
Hello,

Marek, can you please see whether this solves the hang while resuming? If it
does and Rafael is okay with it, I'll route this patch through wq/for-6.9.

Thanks.

include/linux/async.h | 1 +
init/main.c | 1 +
kernel/async.c | 17 ++++++++++++++++-
3 files changed, 18 insertions(+), 1 deletion(-)

--- a/include/linux/async.h
+++ b/include/linux/async.h
@@ -120,4 +120,5 @@ extern void async_synchronize_cookie(asy
extern void async_synchronize_cookie_domain(async_cookie_t cookie,
struct async_domain *domain);
extern bool current_is_async(void);
+extern void async_init(void);
#endif
--- a/init/main.c
+++ b/init/main.c
@@ -1545,6 +1545,7 @@ static noinline void __init kernel_init_
sched_init_smp();

workqueue_init_topology();
+ async_init();
padata_init();
page_alloc_init_late();

--- a/kernel/async.c
+++ b/kernel/async.c
@@ -64,6 +64,7 @@ static async_cookie_t next_cookie = 1;
static LIST_HEAD(async_global_pending); /* pending from all registered doms */
static ASYNC_DOMAIN(async_dfl_domain);
static DEFINE_SPINLOCK(async_lock);
+static struct workqueue_struct *async_wq;

struct async_entry {
struct list_head domain_list;
@@ -174,7 +175,7 @@ static async_cookie_t __async_schedule_n
spin_unlock_irqrestore(&async_lock, flags);

/* schedule for execution */
- queue_work_node(node, system_unbound_wq, &entry->work);
+ queue_work_node(node, async_wq, &entry->work);

return newcookie;
}
@@ -345,3 +346,17 @@ bool current_is_async(void)
return worker && worker->current_func == async_run_entry_fn;
}
EXPORT_SYMBOL_GPL(current_is_async);
+
+void __init async_init(void)
+{
+ /*
+ * Async can schedule a number of interdependent work items. However,
+ * unbound workqueues can handle only upto min_active interdependent
+ * work items. The default min_active of 8 isn't sufficient for async
+ * and can lead to stalls. Let's use a dedicated workqueue with raised
+ * min_active.
+ */
+ async_wq = alloc_workqueue("async", WQ_UNBOUND, 0);
+ BUG_ON(!async_wq);
+ workqueue_set_min_active(async_wq, WQ_DFL_ACTIVE);
+}

2024-02-09 08:15:19

by Marek Szyprowski

[permalink] [raw]
Subject: Re: [PATCH 2/2 wq/for-6.9] async: Use a dedicated unbound workqueue with raised min_active

On 09.02.2024 01:14, Tejun Heo wrote:
> Async can schedule a number of interdependent work items. However, since
> 5797b1c18919 ("workqueue: Implement system-wide nr_active enforcement for
> unbound workqueues"), unbound workqueues have separate min_active which sets
> the number of interdependent work items that can be handled. This default
> value is 8 which isn't sufficient for async and can lead to stalls during
> resume from suspend in some cases.
>
> Let's use a dedicated unbound workqueue with raised min_active.
>
> Signed-off-by: Tejun Heo <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> Reported-by: Marek Szyprowski <[email protected]>
> Cc: Rafael J. Wysocki <[email protected]>
> ---
> Hello,
>
> Marek, can you please see whether this solves the hang while resuming? If it
> does and Rafael is okay with it, I'll route this patch through wq/for-6.9.

Works fine here and fixes the suspend/resume issue.

Tested-by: Marek Szyprowski <[email protected]>


> Thanks.
>
> include/linux/async.h | 1 +
> init/main.c | 1 +
> kernel/async.c | 17 ++++++++++++++++-
> 3 files changed, 18 insertions(+), 1 deletion(-)
>
> --- a/include/linux/async.h
> +++ b/include/linux/async.h
> @@ -120,4 +120,5 @@ extern void async_synchronize_cookie(asy
> extern void async_synchronize_cookie_domain(async_cookie_t cookie,
> struct async_domain *domain);
> extern bool current_is_async(void);
> +extern void async_init(void);
> #endif
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1545,6 +1545,7 @@ static noinline void __init kernel_init_
> sched_init_smp();
>
> workqueue_init_topology();
> + async_init();
> padata_init();
> page_alloc_init_late();
>
> --- a/kernel/async.c
> +++ b/kernel/async.c
> @@ -64,6 +64,7 @@ static async_cookie_t next_cookie = 1;
> static LIST_HEAD(async_global_pending); /* pending from all registered doms */
> static ASYNC_DOMAIN(async_dfl_domain);
> static DEFINE_SPINLOCK(async_lock);
> +static struct workqueue_struct *async_wq;
>
> struct async_entry {
> struct list_head domain_list;
> @@ -174,7 +175,7 @@ static async_cookie_t __async_schedule_n
> spin_unlock_irqrestore(&async_lock, flags);
>
> /* schedule for execution */
> - queue_work_node(node, system_unbound_wq, &entry->work);
> + queue_work_node(node, async_wq, &entry->work);
>
> return newcookie;
> }
> @@ -345,3 +346,17 @@ bool current_is_async(void)
> return worker && worker->current_func == async_run_entry_fn;
> }
> EXPORT_SYMBOL_GPL(current_is_async);
> +
> +void __init async_init(void)
> +{
> + /*
> + * Async can schedule a number of interdependent work items. However,
> + * unbound workqueues can handle only upto min_active interdependent
> + * work items. The default min_active of 8 isn't sufficient for async
> + * and can lead to stalls. Let's use a dedicated workqueue with raised
> + * min_active.
> + */
> + async_wq = alloc_workqueue("async", WQ_UNBOUND, 0);
> + BUG_ON(!async_wq);
> + workqueue_set_min_active(async_wq, WQ_DFL_ACTIVE);
> +}
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland


2024-02-09 21:20:50

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH 2/2 wq/for-6.9] async: Use a dedicated unbound workqueue with raised min_active

On Thu, Feb 08, 2024 at 02:14:16PM -1000, Tejun Heo wrote:
> Async can schedule a number of interdependent work items. However, since
> 5797b1c18919 ("workqueue: Implement system-wide nr_active enforcement for
> unbound workqueues"), unbound workqueues have separate min_active which sets
> the number of interdependent work items that can be handled. This default
> value is 8 which isn't sufficient for async and can lead to stalls during
> resume from suspend in some cases.
>
> Let's use a dedicated unbound workqueue with raised min_active.
>
> Signed-off-by: Tejun Heo <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> Reported-by: Marek Szyprowski <[email protected]>
> Cc: Rafael J. Wysocki <[email protected]>

Applying to wq/for-6.9. Rafael, if you have any objections, please holler.

Thanks.

--
tejun