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;
Message-ID: <3b8222aa-ac58-6a69-ac36-dfdc66f4d66e@cornelisnetworks.com>
Date:   Mon, 22 May 2023 08:27:52 -0400
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.11.0
Subject: Re: [PATCH 09/24] workqueue: Make unbound workqueues to use per-cpu
 pool_workqueues
Content-Language: en-US
To:     Leon Romanovsky <leon@kernel.org>, Tejun Heo <tj@kernel.org>
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,
        Jason Gunthorpe <jgg@ziepe.ca>,
        Karsten Graul <kgraul@linux.ibm.com>,
        Wenjia Zhang <wenjia@linux.ibm.com>,
        Jan Karcher <jaka@linux.ibm.com>
References: <20230519001709.2563-1-tj@kernel.org>
 <20230519001709.2563-10-tj@kernel.org> <20230522064134.GA786644@unreal>
From:   Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
In-Reply-To: <20230522064134.GA786644@unreal>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?d0xBMXY5QTREVDFXVHFlY0tJNkFic0VZUG5sUWlrQUF0K05SMDIzc29mN0NK?=
 =?utf-8?B?TFI3SjlWb3hLZytwRUN2N0xZWTBRQ05DM29kRFRueGJId2xFelBqQzk4NG9u?=
 =?utf-8?B?bmtuU05sWHFRNFZMT09ncW9EVUI3UmE0TTdlQUVnNzdVWitmaVhmZms5YnRZ?=
 =?utf-8?B?VGt1OWpuUnF1ZlZXTlE4KzB4WFpXY2p0M0cxZkNQanZjS0dJcVlGbStuQkVj?=
 =?utf-8?B?OVY5YnFMUXpoRjRhYkNPc2h5M2J6Y1dVdWM5ZUExZmYrL1k4SGtJRGExVGMx?=
 =?utf-8?B?cERUZ2ZsRHhwbmpHZ3lDSVhqb3Y3MU0xckNhcEl1Ly9NRmlpb0NtdWRKc1lj?=
 =?utf-8?B?YzhBZ1R4Y2lwM2JDWmdpa3dGSEhMaEZNK2FLcHBwQ0swUzJyTzNvQ1Q3Z3pJ?=
 =?utf-8?B?cjdWZk5Qb0Rud05jcHZ2M2pzeEMrKzYzMHBBNytuSzZ1UEJVSjV0N2xlZ0Qz?=
 =?utf-8?B?T3dTUHdQMzlhbGthSE5aTHUrdXZDV0ZmYm5TVk90cWxDdlgwT1JvUEJjOWRW?=
 =?utf-8?B?aWR5bk8rZzkxd1dvbXhPV3UwejB4eDhnc2lvaEFKRDRyWEhsbW52eXNVYU9j?=
 =?utf-8?B?cERQQ2h1VFd5V2NjWEQzYVNLbUdNb1BWdXZnMjB2eUk5SXBiYkk5dnV4NzF3?=
 =?utf-8?B?cnFxU2NXUk9LS0hmcVNDUlhGdHRCcnJ2bUl3dlBvRTFhSGJNck40UVVlL3VM?=
 =?utf-8?B?SHhvVVMwcmdOVVFSaDF5Zkgrd1JkcVVTbWloWXNlSWxOQ3I1ZlREV3crWnkx?=
 =?utf-8?B?emFTb1FIbTExWWxTa2VJRW5JTmRRVHpHNHFCcWdWUTZDeTd4S2J0RkM5ZUJ5?=
 =?utf-8?B?VFJtT1IvN1FGOWpyUUxuNTdueGlwWDdhOFdkV21xL1RHYWFGdjA5Tk9CaVRr?=
 =?utf-8?B?a1d2WGxWWGkwcldkYUsrMUtJejA0cWxyM0xRaW45dXpwOGtoM2R3cGNrTzJL?=
 =?utf-8?B?MDNFUUUrWEtjS1VKVXlQenFya2VFQm1sSFNrQ1BJQWRkRFVNMVlPeFUxSXJ4?=
 =?utf-8?B?YlJ5WjZrRWY2Q3F0U216TEFYR2lXT1prRmd0MDVSVW9wTWI2Mnc3SnNpRWJp?=
 =?utf-8?B?Z3lndmlMdjl3Z0ZkUVhaVFA2bmhYTitTN21mdTNTTlNrazhFNXlCNFBzUWp6?=
 =?utf-8?B?bEFQRDFRcVhqY2x2NWdwdjVpZHRKN3ltdS92TGpDeFdDai84WEpYVVFBRlE0?=
 =?utf-8?B?d2Z3R0I5MjFzOG9xYUdOYjlhb0w0ZGk5ckxvaWozcWFjVjVPRTZ0UzVBUGdW?=
 =?utf-8?B?MFd2SU5JZnFlaGw1bHJ3RzdMeDduQUhiT1NFa0h4SHQxQzIzVTJOZGhXOTFN?=
 =?utf-8?B?MWZYYVI2aHZ1cnBZVXNicmdmQUNBZHZtYTVZOHZob3QxQlh1VUdKcm03SEEw?=
 =?utf-8?B?U3FQTTBqVms0cXBxSHpqaWU2Wk5qL0VWang4LzhndTU1MHNnOE51SkFJMGM5?=
 =?utf-8?B?M3R0V2JaU2MwK3hNM1lmK213bGNQYnBPT1BlM3Z0SjZBOWRmanBzVmk1bFNE?=
 =?utf-8?B?cVBZcTJKS0NnODl5NENQb0FXOEZFblRpcXVIWi90WUl5OTdnMmVWR2FqUDFP?=
 =?utf-8?B?aWtoRVBJUTJISHZyQnNTV2pRR3RhVXBqUWU3TkFJS3h4L0ZmR2RRay9NQ0Zk?=
 =?utf-8?B?eXlLUkZsd0Y4WVhyeGRtODFuSDdTT1ZuYlRyZXl3NGd6L0c0MFVLOGhFOHRq?=
 =?utf-8?B?NnVReU5XeHlwdCsvMkFwSWprSmtYdXplbm9EeWdxKzhIT3AxRW1ZS3h0b0hM?=
 =?utf-8?B?NDVoenh3SUJIS3laNUdXakI2NmgxanJXVENTY0ZQblQ1NzBjUENJMUdkVyt0?=
 =?utf-8?B?a1NOZklSb1R1SWV4Skwza3UyTm92QjdCN0NzU3JhOWVJMGlseTVyTytMQnJ5?=
 =?utf-8?B?WGNmNVkxeGJUMGdId0Robm9kZWVKSWY1Wkx5T21nUjVVVlVsNm01MVIzS00v?=
 =?utf-8?B?cFhJRzFMNmgyeVNnd0wzYXJhRWY0SmpzOGN6T2VJOVp1MUxYMW9aeVRlOGlm?=
 =?utf-8?B?YjRHUllGbk92SFZ3TjR6MHZURDNkbTVrRkJwVytwbGo0cFlXM0tjOUJpRVNJ?=
 =?utf-8?B?bUx4R3NqUnRuem9xSlVHNkxaYnp1NVBwVk5JQnVIMGtVaGh1OUhySmNDVVkz?=
 =?utf-8?B?UEdqdWh1K1hWTGxjL0ZxZCt0TmkyR0wrNEdocEtCR1J6WERpQS9sTDZZMDky?=
 =?utf-8?Q?hOGtQeXlZGEb/fl+OKYGjLY=3D?=
X-OriginatorOrg: cornelisnetworks.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f642e61b-d25c-4e7a-c9e6-08db5abff8e1
X-MS-Exchange-CrossTenant-AuthSource: BL0PR01MB4099.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 May 2023 12:27:56.5843
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4dbdb7da-74ee-4b45-8747-ef5ce5ebe68a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: UQeaohdxm+AxmrPgGdCAfQyjdJ86RZOCVhzOPWFfL1unkg4/sjMwD9HQEsowzQY1B9EqBYPB9N2/8isEI1ZRT9izQx+FMk+KINqsh6gyDDc9vV0br2hgriPAeohfXXc/
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR01MB4777
X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 5/22/23 2:41 AM, Leon Romanovsky wrote:
> 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

I can see there being an impact as to when things are considered congested since
it's now CPU based vs NUMA. However, this seems like it's a good thing for hfi1.
The purpose of the code in hfi1 is to decide if QP processing should yield the
CPU and allow other QPs to make progress.

Acked-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>