linux-kernel - Re: [PATCH 3/8] workqueue: Implement BH workqueues to eventually replace tasklets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 1 Feb 2024 11:47:40 -1000
From: Tejun Heo <tj@...nel.org>
To: Lai Jiangshan <jiangshanlai@...il.com>
Cc: torvalds@...ux-foundation.org, mpatocka@...hat.com,
	linux-kernel@...r.kernel.org, dm-devel@...ts.linux.dev,
	msnitzer@...hat.com, ignat@...udflare.com, damien.lemoal@....com,
	bob.liu@...cle.com, houtao1@...wei.com, peterz@...radead.org,
	mingo@...nel.org, netdev@...r.kernel.org, allen.lkml@...il.com,
	kernel-team@...a.com
Subject: Re: [PATCH 3/8] workqueue: Implement BH workqueues to eventually
 replace tasklets

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