Received-SPF: pass (google.com: domain of linux-kernel+bounces-48888-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Sender: Tejun Heo <htejun@gmail.com>
Date: Thu, 1 Feb 2024 11:47:40 -1000
From: Tejun Heo <tj@kernel.org>
To: Lai Jiangshan <jiangshanlai@gmail.com>
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
Subject: Re: [PATCH 3/8] workqueue: Implement BH workqueues to eventually
 replace tasklets
Message-ID: <ZbwRfHCfss2THOeX@slm.duckdns.org>
References: <20240130091300.2968534-1-tj@kernel.org>
 <20240130091300.2968534-4-tj@kernel.org>
 <CAJhGHyCk_cjD-Ex_OAd=DrBkTEe0Xvs81=On65Sp7sS8zNBfGQ@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAJhGHyCk_cjD-Ex_OAd=DrBkTEe0Xvs81=On65Sp7sS8zNBfGQ@mail.gmail.com>

Hello, Lai.

On Thu, Feb 01, 2024 at 07:02:27PM +0800, Lai Jiangshan wrote:
> On Tue, Jan 30, 2024 at 5:16 PM 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 == 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().

Hmmm.... maybe. Yeah, that'd also make it a tiny bit cheaper for hot paths.
Lemme see how that looks.

> 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.

Good point. Will update.

> 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.

Note that BH worker pools are always DISASSOCIATED, so
worker_attach_to_pool() shouldn't call kthread_set_per_cpu(). Also, BH
workers and worker pools are never destroyed, so worker_detach_from_pool()
shouldn't be called either. I'll add WARN_ONs to clarify.

> > @@ -5605,7 +5731,12 @@ static void pr_cont_pool_info(struct worker_pool *pool)
> >         pr_cont(" cpus=%*pbl", nr_cpumask_bits, pool->attrs->cpumask);
> >         if (pool->node != NUMA_NO_NODE)
> >                 pr_cont(" node=%d", pool->node);
> > -       pr_cont(" flags=0x%x nice=%d", pool->flags, pool->attrs->nice);
> > +       pr_cont(" flags=0x%x", pool->flags);
> > +       if (pool->flags & POOL_BH)
> > +               pr_cont(" bh%s",
> > +                       pool->attrs->nice == HIGHPRI_NICE_LEVEL ? "-hi" : "");
> > +       else
> > +               pr_cont(" nice=%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.

Ah, right, I'll hunt them down. 

Thanks.

-- 
tejun