From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9E572C4167B for ; Thu, 14 Dec 2023 00:28:28 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DD6618D0081; Wed, 13 Dec 2023 19:28:27 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id D7CDF8D0080; Wed, 13 Dec 2023 19:28:27 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BF7678D0081; Wed, 13 Dec 2023 19:28:27 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id A93818D0080 for ; Wed, 13 Dec 2023 19:28:27 -0500 (EST) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 7F1791C155F for ; Thu, 14 Dec 2023 00:28:27 +0000 (UTC) X-FDA: 81563537454.09.F8BF712 Received: from mail-il1-f173.google.com (mail-il1-f173.google.com [209.85.166.173]) by imf18.hostedemail.com (Postfix) with ESMTP id C1A241C0013 for ; Thu, 14 Dec 2023 00:28:25 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=mFSUF7t1; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf18.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.166.173 as permitted sender) smtp.mailfrom=nphamcs@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1702513705; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=IOSXxu0Ty5SRYxvtBv47EJc9VazQTq5FR1JKdC1EXXI=; b=Kvyd76G20VYwu2wYcU8029DfjEKUQtd0NL+DJpZOiBSrdyGTQ4IttKG1MSE7YzoqDIJOWk XRmwbEZp5Gn6UfS6rmBBvuDR+XVxvsUL6D0R7m2CGu7tFi1HBFZ2RYKdPhvdkrLSoy/R3O xFXI3IcKgPhMr1KNrlAwwY4JORPKoYA= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=mFSUF7t1; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf18.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.166.173 as permitted sender) smtp.mailfrom=nphamcs@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1702513705; a=rsa-sha256; cv=none; b=Ie9Ikp/C0WjvR7YxDc5WE+czCgG/9sh30NgExi97igLgCjoDHOULcSyQbprNQ9BSxZ7V6y k/w5leRC7+OgPb5EuiR+BPeSBugqhydIcLr73fdcXnieaNrlfzgTQH5+wkh3QyRupJP3ch ZZ+rDZBoRuUjmVkZi1d4cRs6lc5sEhU= Received: by mail-il1-f173.google.com with SMTP id e9e14a558f8ab-35d718382b7so31291255ab.1 for ; Wed, 13 Dec 2023 16:28:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1702513705; x=1703118505; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=IOSXxu0Ty5SRYxvtBv47EJc9VazQTq5FR1JKdC1EXXI=; b=mFSUF7t1SWhxtK4TVIzVrF7JYH0EsQwIMNuvP3Ux4PTSuO/Qk49lhfHzHbiIW4Mq0g JiknjQIlW5i63yDP7t43qbYNC5n08Xkdjk2FifaIrz2H0wFsJioTR+F/Wz0pMsDLoQwy 266cJOAiq6qmgPvWl1RamQ9LFlLg2WiFkXncN7UbtXe7GdJVCfGjQP8soNAipIJ1n1P8 pC/jtBdVsDKxMFG8gZk/v1TJAxf5kSwWKj51FMGAA7toaICa/BeaInjECR5LddAOuBHC XenbgfMkwzHx4mbMIVYzAEL9zUIzay1C4wOYUt6dLBJvPpsveQjraMd+lBg278ByiIXR i/sg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702513705; x=1703118505; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=IOSXxu0Ty5SRYxvtBv47EJc9VazQTq5FR1JKdC1EXXI=; b=Tih/8CLXUgyD1SfdaWFyDJc0QldCdLiWKcZdiTo/P5PM/WoD4+PlCgDNUmy6CwW4s/ wjXnuZclz3naN0u1hwJkMFV93pkbayt6172lBDkFlU1NDg/LVNE+OWEh/Y9rmGudetc5 5RCeiDvsX59Pd1QS4WK9G5tgaYZRaZ303h5V4dT0WzbvTlGmQmn2Bh5TJAqFL4nNJw6q 0qJ/Y7xlxnyUhgi4uUInG9Am+2wj31Bu+UWHcUEXVcI2mgIECX6O95dWGdyxWOci+fjh 0OfAeL5POIga0csyQHwSgykoLzDM2vmzTf6AJ+6qXh7+wzbdyaVjTgeIyGiO4iivJ+iT oW3w== X-Gm-Message-State: AOJu0Yxtw/G77/E8uQ74vkK97QfxGXMVbpvzJXG/igaFd0uqNl+jKpjE qNFmNj9pdeJaXEj500w0xSquTzb35jxrY0KjLxc= X-Google-Smtp-Source: AGHT+IHuxrOR2RIzgNSFCxEWboJMMjmHD7ppE3QLpc5tJ6emMuXh2ut+6Ofzb0AHwCe/jRYRvWJa70U8yx6St+i+tjo= X-Received: by 2002:a05:6e02:1c4d:b0:35f:7629:87d3 with SMTP id d13-20020a056e021c4d00b0035f762987d3mr1990455ilg.43.1702513704644; Wed, 13 Dec 2023 16:28:24 -0800 (PST) MIME-Version: 1.0 References: <20231211052850.3513230-1-debug.penguin32@gmail.com> In-Reply-To: From: Nhat Pham Date: Wed, 13 Dec 2023 16:28:13 -0800 Message-ID: Subject: Re: [PATCH] mm/zswap: Improve with alloc_workqueue() call To: Ronald Monthero Cc: sjenning@redhat.com, akpm@linux-foundation.org, Dan Streetman , Vitaly Wool , linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: C1A241C0013 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: 4jgnbfrio3uc76qeumu4baanpr88x8ej X-HE-Tag: 1702513705-765315 X-HE-Meta: U2FsdGVkX19mDb3yOfCo+j34LWKYTm366o957n5GS7ztQq4cxkDe9NqrjbQtiDOtiXFPOmF7fyjT/d+loe3eieKnZqkK9YfYomssIjzOgyL89Ne5jXpdCyIxQ5I/IMFfDRrvL8E+yQrMkTni0/E6/23uEAOVYlJVInFakzS9fw32Do25xiexnSR9ITxbQyv+foSOGoQPJ5xqtY/fNdF8Z6ZHW3CNhqL/zuLQsdI4CB0vZzO7e4y7ju+lhD3GbeFsDjRq7jOju3xiNH0NUhGGYfTm0AK/9KFd8dY63F0W+LxxgBNdY/0OV9VU7/oI4j2pdwm97IYOcLi18rxf97ZO+PiKonYgF8zBGhIJODpO3w5d4X7ac5HWFi/0a0H4eZp2KNtHK03cZdrx4vPzNiKyBijfeMptPm9V7U8AHzXRrLHEC9/qZgPcnZXFYjjn3+5i/5Q0LDSHi3X+jtDk2pjK06awhJvnFhbcIrUFkI1yl7jSi//+oUYQbiYzYOFWgbqB4teTTAyre7iE/MJkCHyO0R3QKXEMuY4/t6guCw5U/asQUTwFT29HyWbGdEdFXZhQ4o/Fck2Aak6wF9es9Xiq2ixLgi7J73VcINOi8DLjBk9dtxQKwVqqUaUJxtL95gE4p4UePNm+Pkq4IrFMFoaJ4DTLnrp57NszieLP4h6XiLBzDSK7gOmJaWIqm+tvJlviFYQeJ8jUSD+Gu+uiTg/9PtSKyYpOGcoj5Sg8IGOdO9+DrzsVVM7hfaiLBege6Kygt+mye6bUvJK8vI5X8I/Mtk3D8aPauXbCpxKrVEpN34ZWez7OW+YuUNRoH/LwWruNrbh3BtddMDlfyVP40IeUtuN8RpamDILrb07saVBjlY0FGbcJJYWOKuN5+cyhPKa0nOs8uvFoUJYWGAXVRmrWrH5wZVrVnpocelHZ5bRW2BkU//UKYYg/YVTpsVUkSBmzCMWn3bo9lb9JY2G7kyH vnLePa2A jWm8wk1AnqpNZj8HXrvJH6TLXDg5EsYJxRULohyeT7338/QtJe7gl6cli3vllmN3wfNjwrIAKiqyUCBG2XQYJ6feguwMTMcRvAzh+WlaI2effe//RYYO4u8kWhEvG2ngyrgDki+TTjXtuKIc8oNyRTP70ijmqqhFuhGenkjUJzM0PUfSAYr36MvPj1AYmd0ebCzZaDblzGrFx07pzkVSVNRhmQ/h7T4cZje9HvtonR0CrSyvKPG6AC62bS35a/WOtzcHEJy1yrh1M07DnTobwPAQAUFTQlogDjiAqFXVaLKNF/LiwmF3mjxsk+lK/+sk+EXuyPq5Pg0TWgMxyIWYYfJNtIWkVvPebcXKzbUPLxJRTN9cHkU8ccKXTfWnQaVLwOiC8dd8UKmyhLdl7OzP29wMQGkg8DwXtPVrIfGjIxuOAyApnppUjYxvh2StWZEKyZx0KzpMHktQT/Wzp7TJYJWJ1nBBxiXbNEBwmxUJd5uMGl8LZOXHivy0LttD2DbBBjkPvvIjNpSBfECB6naJFBOu/KZNMIzLDjl7PnTTX1b72Qruqz3CvugZJdCcR2UkxUVBs X-Bogosity: Ham, tests=bogofilter, spamicity=0.000008, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Wed, Dec 13, 2023 at 5:20=E2=80=AFAM Ronald Monthero wrote: > > Hi Nhat, > Thanks for checking. > On Tue, Dec 12, 2023 at 12:16=E2=80=AFAM Nhat Pham wr= ote: > > > > On Sun, Dec 10, 2023 at 9:31=E2=80=AFPM Ronald Monthero > > wrote: > > > > > > Use alloc_workqueue() to create and set finer > > > work item attributes instead of create_workqueue() > > > which is to be deprecated. > > > > > > Signed-off-by: Ronald Monthero > > > --- > > > mm/zswap.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > > index 74411dfdad92..64dbe3e944a2 100644 > > > --- a/mm/zswap.c > > > +++ b/mm/zswap.c > > > @@ -1620,7 +1620,8 @@ static int zswap_setup(void) > > > zswap_enabled =3D false; > > > } > > > > > > - shrink_wq =3D create_workqueue("zswap-shrink"); > > > + shrink_wq =3D alloc_workqueue("zswap-shrink", > > > + WQ_UNBOUND|WQ_MEM_RECLAIM, 0); > > > > Hmmm this changes the current behavior a bit right? create_workqueue() > > is currently defined as: > > > > alloc_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, 1, (name)) > > create_workqueue is deprecated and it's observed that most of the > subsystems have changed to using alloc_workqueue. So it's a small > minority of few remnant instances in the kernel and some drivers still > using create_workqueue. With the create_workqueue defined as is , it > hardcodes the workqueue flags to be per cpu and unbound in nature and > not giving the flexibility of using any other wait queue > flags/attributes. ( WQ_CPU_INTENSIVE, WQ_HIGHPRI, WQ_RESCUER, > WQ_FREEZEABLE, WQ_UNBOUND) . Hence most of the subsystems and drivers > use the alloc_workqueue( ) api. > #define create_workqueue(name) \ > alloc_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, 1, (name)) > > > I think this should be noted in the changelog, at the very least, even > > if it is fine. We should be as explicit as possible about behavior > > changes. > > > imo, it's sort of known and consistently changed for quite some time alre= ady. > https://lists.openwall.net/linux-kernel/2016/06/07/1086 > https://lists.openwall.net/linux-kernel/2011/01/03/124 > https://lwn.net/Articles/403891/ =3D> quoted: "The long-term plan, it > seems, is to convert all create_workqueue() users over to an > appropriate alloc_workqueue() call; eventually create_workqueue() will > be removed" > > glad to take some suggestions , thoughts ? > > BR, > ronald I should have been clearer. I'm not against the change per-se - I agree that we should replace create_workqueue() with alloc_workqueue(). What I meant was, IIUC, there are two behavioral changes with this new workqueue creation: a) We're replacing a bounded workqueue (which as you noted, is fixed by create_workqueue()) with an unbounded one (WQ_UNBOUND). This seems fine to me - I doubt locality buys us much here. b) create_workqueue() limits the number of concurrent per-cpu execution contexts at 1 (i.e only one single global reclaimer), whereas after this patch this is set to the default value. This seems fine to me too - I don't remember us taking advantage of the previous concurrency limitation. Also, in practice, the task_struct is one-to-one with the zswap_pool's anyway, and most of the time, there is just a single pool being used. (But it begs the question - what's the point of using 0 instead of 1 here?) Both seem fine (to me anyway - other reviewers feel free to take a look and fact-check everything). I just feel like this should be explicitly noted in the changelog, IMHO, in case we are mistaken and need to revisit this :) Either way, not a NACK from me.