Received-SPF: pass (google.com: domain of linux-kernel+bounces-47996-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240130091300.2968534-1-tj@kernel.org> <20240130091300.2968534-4-tj@kernel.org>
In-Reply-To: <20240130091300.2968534-4-tj@kernel.org>
From: Lai Jiangshan <jiangshanlai@gmail.com>
Date: Thu, 1 Feb 2024 19:02:27 +0800
Message-ID: <CAJhGHyCk_cjD-Ex_OAd=DrBkTEe0Xvs81=On65Sp7sS8zNBfGQ@mail.gmail.com>
Subject: Re: [PATCH 3/8] workqueue: Implement BH workqueues to eventually
 replace tasklets
To: Tejun Heo <tj@kernel.org>
Cc: torvalds@linux-foundation.org, mpatocka@redhat.com, 
	linux-kernel@vger.kernel.org, dm-devel@lists.linux.dev, msnitzer@redhat.com, 
	ignat@cloudflare.com, damien.lemoal@wdc.com, bob.liu@oracle.com, 
	houtao1@huawei.com, peterz@infradead.org, mingo@kernel.org, 
	netdev@vger.kernel.org, allen.lkml@gmail.com, kernel-team@meta.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hello, Tejun

On Tue, Jan 30, 2024 at 5:16=E2=80=AFPM Tejun Heo <tj@kernel.org> wrote:

> @@ -1184,6 +1211,14 @@ static bool kick_pool(struct worker_pool *pool)
>         if (!need_more_worker(pool) || !worker)
>                 return false;
>
> +       if (pool->flags & POOL_BH) {
> +               if (pool->attrs->nice =3D=3D HIGHPRI_NICE_LEVEL)
> +                       tasklet_hi_schedule(&worker->bh_tasklet);
> +               else
> +                       tasklet_schedule(&worker->bh_tasklet);
> +               return true;
> +       }

I think it is more straightforward to call bh_worker_taskletfn[_hi]()
in tasklet_action() and tasklet_hi_action() rather than add a
worker->bh_tasklet.

raise_softirq_irqoff() can be used here (kick_pool()) instead.

As the changelog said, once the conversion is complete, tasklet can be
removed and BH workqueues can directly take over the tasklet softirqs,
in which case, then, bh_worker_taskletfn[_hi]() can directly take over
the tasklet_action() and tasklet_hi_action().


> +
>         p =3D worker->task;
>
>  #ifdef CONFIG_SMP
> @@ -1663,8 +1698,16 @@ static bool pwq_tryinc_nr_active(struct pool_workq=
ueue *pwq, bool fill)
>         lockdep_assert_held(&pool->lock);
>
>         if (!nna) {
> -               /* per-cpu workqueue, pwq->nr_active is sufficient */
> -               obtained =3D pwq->nr_active < READ_ONCE(wq->max_active);
> +               /*
> +                * BH workqueues always share a single execution context =
per CPU
> +                * and don't impose any max_active limit, so tryinc alway=
s
> +                * succeeds. For a per-cpu workqueue, checking pwq->nr_ac=
tive is
> +                * sufficient.
> +                */
> +               if (wq->flags & WQ_BH)
> +                       obtained =3D true;
> +               else
> +                       obtained =3D pwq->nr_active < READ_ONCE(wq->max_a=
ctive);

I think wq->max_active can be forced to be UINT_MAX or ULONG_MAX
in the max_active management code to avoid a branch here.

>                 goto out;
>         }
>
> @@ -2599,27 +2642,31 @@ static struct worker *create_worker(struct worker=
_pool *pool)
>
>         worker->id =3D id;
>
> -       if (pool->cpu >=3D 0)
> -               snprintf(id_buf, sizeof(id_buf), "%d:%d%s", pool->cpu, id=
,
> -                        pool->attrs->nice < 0  ? "H" : "");
> -       else
> -               snprintf(id_buf, sizeof(id_buf), "u%d:%d", pool->id, id);
> -
> -       worker->task =3D kthread_create_on_node(worker_thread, worker, po=
ol->node,
> -                                             "kworker/%s", id_buf);
> -       if (IS_ERR(worker->task)) {
> -               if (PTR_ERR(worker->task) =3D=3D -EINTR) {
> -                       pr_err("workqueue: Interrupted when creating a wo=
rker thread \"kworker/%s\"\n",
> -                              id_buf);
> -               } else {
> -                       pr_err_once("workqueue: Failed to create a worker=
 thread: %pe",
> -                                   worker->task);
> +       if (pool->flags & POOL_BH) {
> +               tasklet_setup(&worker->bh_tasklet, bh_worker_taskletfn);
> +       } else {
> +               if (pool->cpu >=3D 0)
> +                       snprintf(id_buf, sizeof(id_buf), "%d:%d%s", pool-=
>cpu, id,
> +                                pool->attrs->nice < 0  ? "H" : "");
> +               else
> +                       snprintf(id_buf, sizeof(id_buf), "u%d:%d", pool->=
id, id);
> +
> +               worker->task =3D kthread_create_on_node(worker_thread, wo=
rker,
> +                                       pool->node, "kworker/%s", id_buf)=
;
> +               if (IS_ERR(worker->task)) {
> +                       if (PTR_ERR(worker->task) =3D=3D -EINTR) {
> +                               pr_err("workqueue: Interrupted when creat=
ing a worker thread \"kworker/%s\"\n",
> +                                      id_buf);
> +                       } else {
> +                               pr_err_once("workqueue: Failed to create =
a worker thread: %pe",
> +                                           worker->task);
> +                       }
> +                       goto fail;
>                 }
> -               goto fail;
> -       }
>
> -       set_user_nice(worker->task, pool->attrs->nice);
> -       kthread_bind_mask(worker->task, pool_allowed_cpus(pool));
> +               set_user_nice(worker->task, pool->attrs->nice);
> +               kthread_bind_mask(worker->task, pool_allowed_cpus(pool));
> +       }
>
>         /* successful, attach the worker to the pool */
>         worker_attach_to_pool(worker, pool);

worker_attach_to_pool() and worker_detach_from_pool also access to
worker->task with kthread_set_per_cpu() and luckily to_kthread()
checks the NULL pointer for it.

IMO, it is better to avoid calling kthread_set_per_cpu() for bh workers.


> @@ -5605,7 +5731,12 @@ static void pr_cont_pool_info(struct worker_pool *=
pool)
>         pr_cont(" cpus=3D%*pbl", nr_cpumask_bits, pool->attrs->cpumask);
>         if (pool->node !=3D NUMA_NO_NODE)
>                 pr_cont(" node=3D%d", pool->node);
> -       pr_cont(" flags=3D0x%x nice=3D%d", pool->flags, pool->attrs->nice=
);
> +       pr_cont(" flags=3D0x%x", pool->flags);
> +       if (pool->flags & POOL_BH)
> +               pr_cont(" bh%s",
> +                       pool->attrs->nice =3D=3D HIGHPRI_NICE_LEVEL ? "-h=
i" : "");
> +       else
> +               pr_cont(" nice=3D%d", pool->attrs->nice);

There are also some "worker->task" in show_pwq(), show_one_worker_pool(),
and show_cpu_pool_hog() needing taking care of.

Thanks
Lai