Re: [PATCH 9/9] workqueue: Implement system-wide nr_active enforcement for unbound workqueues

[Lai Jiangshan]

Hello, Tejun

On Sat, Jan 13, 2024 at 8:29 AM Tejun Heo <tj@xxxxxxxxxx> wrote:
> + */
> +static int wq_node_max_active(struct workqueue_struct *wq, int node)
> +{
> +       int min_active = READ_ONCE(wq->min_active);
> +       int max_active = READ_ONCE(wq->max_active);
> +       int node_max_active;
> +
> +       if (node == NUMA_NO_NODE)
> +               return min_active;
> +
> +       node_max_active = DIV_ROUND_UP(max_active * node_nr_cpus[node],
> +                                      num_online_cpus());

node_nr_cpus[node] and num_online_cpus() are global values, they might
not suitable
for this particular workqueue and might cause skewed proportions.

the cache values:

pwq->pool->attrs->pool_nr_online_cpus =
cpumask_weight_and(pwq->pool->attrs->__pod_cpumask, cpu_online_mask);

wq->wq_nr_online_cpus =
cpumask_weight_and(wq->unbound_attrs->cpumask,  cpu_online_mask);

can be used instead. They can be calculated at creation and cpuhotplug.
pool_nr_online_cpus doesn't contribute to the pool's hash value.

Or the result of wq_node_max_active() can be cached in struct wq_node_nr_active,
see the comment next.

> -static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq)
> +static bool pwq_tryinc_nr_active(struct pool_workqueue *pwq, bool fill)
>  {
>         struct workqueue_struct *wq = pwq->wq;
>         struct worker_pool *pool = pwq->pool;
>         struct wq_node_nr_active *nna = wq_node_nr_active(wq, pool->node);
> -       bool obtained;
> +       bool obtained = false;
> +       int node_max_active;
>
>         lockdep_assert_held(&pool->lock);
>
> -       obtained = pwq->nr_active < READ_ONCE(wq->max_active);
> +       if (!nna) {
> +               /* per-cpu workqueue, pwq->nr_active is sufficient */
> +               obtained = pwq->nr_active < READ_ONCE(wq->max_active);
> +               goto out;
> +       }
> +
> +       /*
> +        * Unbound workqueue uses per-node shared nr_active $nna. If @pwq is
> +        * already waiting on $nna, pwq_dec_nr_active() will maintain the
> +        * concurrency level. Don't jump the line.
> +        *
> +        * We need to ignore the pending test after max_active has increased as
> +        * pwq_dec_nr_active() can only maintain the concurrency level but not
> +        * increase it. This is indicated by @fill.
> +        */
> +       if (!list_empty(&pwq->pending_node) && likely(!fill))
> +               goto out;
> +
> +       node_max_active = wq_node_max_active(wq, pool->node);

It is a hot path for unbound workqueues, I think the result of
wq_node_max_active()
should be cached in struct wq_node_nr_active.

The result be calculated at creation, cpuhotplug, and changing max_active.


>
>  /**
>   * pwq_activate_first_inactive - Activate the first inactive work item on a pwq
>   * @pwq: pool_workqueue of interest
> + * @fill: max_active may have increased, try to increase concurrency level

I think it is also legitimate to increase the concurrency level ASAP
when called from try_to_grab_pending() path.

Thanks
Lai