Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 22 May 2023 09:41:34 +0300
From:   Leon Romanovsky <leon@kernel.org>
To:     Tejun Heo <tj@kernel.org>,
        Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
Cc:     jiangshanlai@gmail.com, torvalds@linux-foundation.org,
        peterz@infradead.org, linux-kernel@vger.kernel.org,
        kernel-team@meta.com, joshdon@google.com, brho@google.com,
        briannorris@chromium.org, nhuck@google.com, agk@redhat.com,
        snitzer@kernel.org, void@manifault.com,
        Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>,
        Jason Gunthorpe <jgg@ziepe.ca>,
        Karsten Graul <kgraul@linux.ibm.com>,
        Wenjia Zhang <wenjia@linux.ibm.com>,
        Jan Karcher <jaka@linux.ibm.com>
Subject: Re: [PATCH 09/24] workqueue: Make unbound workqueues to use per-cpu
 pool_workqueues
Message-ID: <20230522064134.GA786644@unreal>
References: <20230519001709.2563-1-tj@kernel.org>
 <20230519001709.2563-10-tj@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230519001709.2563-10-tj@kernel.org>
Precedence: bulk

On Thu, May 18, 2023 at 02:16:54PM -1000, Tejun Heo wrote:
> A pwq (pool_workqueue) represents an association between a workqueue and a
> worker_pool. When a work item is queued, the workqueue selects the pwq to
> use, which in turn determines the pool, and queues the work item to the pool
> through the pwq. pwq is also what implements the maximum concurrency limit -
> @max_active.
> 
> As a per-cpu workqueue should be assocaited with a different worker_pool on
> each CPU, it always had per-cpu pwq's that are accessed through wq->cpu_pwq.
> However, unbound workqueues were sharing a pwq within each NUMA node by
> default. The sharing has several downsides:
> 
> * Because @max_active is per-pwq, the meaning of @max_active changes
>   depending on the machine configuration and whether workqueue NUMA locality
>   support is enabled.
> 
> * Makes per-cpu and unbound code deviate.
> 
> * Gets in the way of making workqueue CPU locality awareness more flexible.
> 
> This patch makes unbound workqueues use per-cpu pwq's the same way per-cpu
> workqueues do by making the following changes:
> 
> * wq->numa_pwq_tbl[] is removed and unbound workqueues now use wq->cpu_pwq
>   just like per-cpu workqueues. wq->cpu_pwq is now RCU protected for unbound
>   workqueues.
> 
> * numa_pwq_tbl_install() is renamed to install_unbound_pwq() and installs
>   the specified pwq to the target CPU's wq->cpu_pwq.
> 
> * apply_wqattrs_prepare() now always allocates a separate pwq for each CPU
>   unless the workqueue is ordered. If ordered, all CPUs use wq->dfl_pwq.
>   This makes the return value of wq_calc_node_cpumask() unnecessary. It now
>   returns void.
> 
> * @max_active now means the same thing for both per-cpu and unbound
>   workqueues. WQ_UNBOUND_MAX_ACTIVE now equals WQ_MAX_ACTIVE and
>   documentation is updated accordingly. WQ_UNBOUND_MAX_ACTIVE is no longer
>   used in workqueue implementation and will be removed later.
> 
> * All unbound pwq operations which used to be per-numa-node are now per-cpu.
> 
> For most unbound workqueue users, this shouldn't cause noticeable changes.
> Work item issue and completion will be a small bit faster, flush_workqueue()
> would become a bit more expensive, and the total concurrency limit would
> likely become higher. All @max_active==1 use cases are currently being
> audited for conversion into alloc_ordered_workqueue() and they shouldn't be
> affected once the audit and conversion is complete.
> 
> One area where the behavior change may be more noticeable is
> workqueue_congested() as the reported congestion state is now per CPU
> instead of NUMA node. There are only two users of this interface -
> drivers/infiniband/hw/hfi1 and net/smc. Maintainers of both subsystems are
> cc'd. Inputs on the behavior change would be very much appreciated.

At least for hfi1, it seems like your changes won't cause to any
differences as NUMA node is expected to be connected to closest CPU
anyway in setups relevant to hfi1.

Dennis, am I right?

Thanks