Re: [PATCH 4/5] workqueue: Automatically mark CPU-hogging work items CPU_INTENSIVE

[Lai Jiangshan]

On Wed, Apr 19, 2023 at 4:52 AM Tejun Heo <tj@xxxxxxxxxx> wrote:
>
> If a per-cpu work item hogs the CPU, it can prevent other work items from
> starting through concurrency management. A per-cpu workqueue which intends
> to host such CPU-hogging work items can choose to not participate in
> concurrency management by setting %WQ_CPU_INTENSIVE; however, this can be
> error-prone and difficult to debug when missed.
>
> This patch adds an automatic CPU usage based detection. If a
> concurrency-managed work item consumes more CPU time than the threshold (5ms
> by default), it's marked CPU_INTENSIVE automatically on schedule-out.
>
> The mechanism isn't foolproof in that the 5ms detection delays can add up if
> many CPU-hogging work items are queued at the same time. However, in such
> situations, the bigger problem likely is the CPU being saturated with
> per-cpu work items and the solution would be making them UNBOUND.
>
> For occasional CPU hogging, the new automatic mechanism should provide
> reasonable protection with minimal increase in code complexity.
>
> Signed-off-by: Tejun Heo <tj@xxxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Lai Jiangshan <jiangshanlai@xxxxxxxxx>
> ---
>  kernel/workqueue.c          | 77 ++++++++++++++++++++++++++-----------
>  kernel/workqueue_internal.h |  1 +
>  2 files changed, 56 insertions(+), 22 deletions(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index b9e8dc54272d..d24b887ddd86 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -306,6 +306,14 @@ static struct kmem_cache *pwq_cache;
>  static cpumask_var_t *wq_numa_possible_cpumask;
>                                         /* possible CPUs of each node */
>
> +/*
> + * Per-cpu work items which run for longer than the following threshold are
> + * automatically considered CPU intensive and excluded from concurrency
> + * management to prevent them from noticeably delaying other per-cpu work items.
> + */
> +static unsigned long wq_cpu_intensive_thresh_us = 5000;
> +module_param_named(cpu_intensive_thresh_us, wq_cpu_intensive_thresh_us, ulong, 0644);
> +
>  static bool wq_disable_numa;
>  module_param_named(disable_numa, wq_disable_numa, bool, 0444);
>
> @@ -951,9 +959,6 @@ void wq_worker_stopping(struct task_struct *task)
>         struct worker *worker = kthread_data(task);
>         struct worker_pool *pool;
>
> -       if (task_is_running(task))
> -               return;
> -
>         /*
>          * Rescuers, which may not have all the fields set up like normal
>          * workers, also reach here, let's not access anything before
> @@ -964,24 +969,49 @@ void wq_worker_stopping(struct task_struct *task)
>
>         pool = worker->pool;
>
> -       /* Return if preempted before wq_worker_running() was reached */
> -       if (worker->sleeping)
> -               return;
> +       if (task_is_running(task)) {
> +               /*
> +                * Concurrency-managed @worker is still RUNNING. See if the
> +                * current work is hogging CPU stalling other per-cpu work
> +                * items. If so, mark @worker CPU_INTENSIVE to exclude it from
> +                * concurrency management. @worker->current_* are stable as they
> +                * can only be modified by @task which is %current.

Hello

wq_worker_stopping() and sched_submit_work() are only called from
schedule() and are not called for other various kinds of scheduling,
such as schedule_rtlock(), preempt_schedule_*(), __cond_resched().

A work item hogging CPU may not call the bare schedule().  To make
the new wq_worker_stopping() works, it has to be added to other kinds
of scheduling, IMO.

Thanks
Lai