Received-SPF: pass (google.com: domain of linux-kernel+bounces-30869-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240113002911.406791-1-tj@kernel.org> <20240113002911.406791-10-tj@kernel.org>
In-Reply-To: <20240113002911.406791-10-tj@kernel.org>
From: Lai Jiangshan <jiangshanlai@gmail.com>
Date: Fri, 19 Jan 2024 15:54:24 +0800
Message-ID: <CAJhGHyDaNCoXaK4g4fKg3vfBYuQ7r+e8TT8ObrtT3nBcTeeuMg@mail.gmail.com>
Subject: Re: [PATCH 9/9] workqueue: Implement system-wide nr_active
 enforcement for unbound workqueues
To: Tejun Heo <tj@kernel.org>
Cc: linux-kernel@vger.kernel.org, Naohiro.Aota@wdc.com, kernel-team@meta.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hello, Tejun

On Sat, Jan 13, 2024 at 8:29=E2=80=AFAM Tejun Heo <tj@kernel.org> wrote:
> + */
> +static int wq_node_max_active(struct workqueue_struct *wq, int node)
> +{
> +       int min_active =3D READ_ONCE(wq->min_active);
> +       int max_active =3D READ_ONCE(wq->max_active);
> +       int node_max_active;
> +
> +       if (node =3D=3D NUMA_NO_NODE)
> +               return min_active;
> +
> +       node_max_active =3D 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 =3D
cpumask_weight_and(pwq->pool->attrs->__pod_cpumask, cpu_online_mask);

wq->wq_nr_online_cpus =3D
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_ac=
tive,
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 =3D pwq->wq;
>         struct worker_pool *pool =3D pwq->pool;
>         struct wq_node_nr_active *nna =3D wq_node_nr_active(wq, pool->nod=
e);
> -       bool obtained;
> +       bool obtained =3D false;
> +       int node_max_active;
>
>         lockdep_assert_held(&pool->lock);
>
> -       obtained =3D pwq->nr_active < READ_ONCE(wq->max_active);
> +       if (!nna) {
> +               /* per-cpu workqueue, pwq->nr_active is sufficient */
> +               obtained =3D 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 increa=
sed as
> +        * pwq_dec_nr_active() can only maintain the concurrency level bu=
t not
> +        * increase it. This is indicated by @fill.
> +        */
> +       if (!list_empty(&pwq->pending_node) && likely(!fill))
> +               goto out;
> +
> +       node_max_active =3D 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 o=
n a pwq
>   * @pwq: pool_workqueue of interest
> + * @fill: max_active may have increased, try to increase concurrency lev=
el

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

Thanks
Lai