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 35A49C48BC3 for ; Wed, 14 Feb 2024 20:10:23 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A00618D0014; Wed, 14 Feb 2024 15:10:22 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 9B0488D0001; Wed, 14 Feb 2024 15:10:22 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 829BE8D0014; Wed, 14 Feb 2024 15:10:22 -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 6A5608D0001 for ; Wed, 14 Feb 2024 15:10:22 -0500 (EST) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 67104140C85 for ; Wed, 14 Feb 2024 20:10:21 +0000 (UTC) X-FDA: 81791501442.26.3B1A1BB Received: from mail-yb1-f202.google.com (mail-yb1-f202.google.com [209.85.219.202]) by imf22.hostedemail.com (Postfix) with ESMTP id AF365C000A for ; Wed, 14 Feb 2024 20:10:18 +0000 (UTC) Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=Bg2gG0rL; spf=pass (imf22.hostedemail.com: domain of 3KR7NZQoKCM4I8CBIu16yx08805y.w86527EH-664Fuw4.8B0@flex--yosryahmed.bounces.google.com designates 209.85.219.202 as permitted sender) smtp.mailfrom=3KR7NZQoKCM4I8CBIu16yx08805y.w86527EH-664Fuw4.8B0@flex--yosryahmed.bounces.google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707941418; a=rsa-sha256; cv=none; b=YT6g2FJV3vEYdL5H71DAclhg53JdPSvGvbq7etaN7wMSdQtexO5jbYb2JtunS1/vrrEAJG eUseDEsqQAEXl6LYQuH/WTxY49/poj1poRHbqMmVEStcmxieSIrmOo+EC+OFIf40xzHGiF xkPme8IiwaDzR/bU6V87j5pDpnCWqac= ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=Bg2gG0rL; spf=pass (imf22.hostedemail.com: domain of 3KR7NZQoKCM4I8CBIu16yx08805y.w86527EH-664Fuw4.8B0@flex--yosryahmed.bounces.google.com designates 209.85.219.202 as permitted sender) smtp.mailfrom=3KR7NZQoKCM4I8CBIu16yx08805y.w86527EH-664Fuw4.8B0@flex--yosryahmed.bounces.google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707941418; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=A3emXcyJNvJgc1uWaTQMfWeSm7Af4PAyEZnHoBa2y9Y=; b=zWfsa0sC6/fXA4qv6a3sqM+qL6fZwm0LXkHXL/UGhLfpOGSiS79VLGWY/rTl/2+qQgNskd MqgiHtNHPD3U/n+D5kSjMoMQ/CJj2U1gEJUfYUvPJZ0tQIMoQBv2kYE8a2/8LesAb+vrXf gHRY7j7HOsAO8XNCXbsnIsLHB2hB8IU= Received: by mail-yb1-f202.google.com with SMTP id 3f1490d57ef6-dcd94cc48a1so76345276.3 for ; Wed, 14 Feb 2024 12:10:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1707941417; x=1708546217; darn=kvack.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=A3emXcyJNvJgc1uWaTQMfWeSm7Af4PAyEZnHoBa2y9Y=; b=Bg2gG0rLor1PYVwssROT03PgtCm427j29DsxdWN/T8st9MCX1uQ00ZX/QGsccExI90 a0V8+Xn4tTYDCG25QooSGQHMms89Q7XfuTkzBdYO6WY+NIr+ikV3mcpdKuBOVfBGwcTT gQepzMb7oejb7IZ/3q2j4GYVmGjMEeZwrQy+WAtNqVTgGMaTX2ye4YqPeKrrDiiV4kEW IkmScTz7Ymp2O/opbBjYI8ku4FJKWsBweNi1s0zzWO9bQkx4QQrTf2E3z5vJ05FWKEIz F5ftTnoF06PrL1VDi3SJrj7roHl+p9PVYrDwgmDjrxH+zuIvUaNGsBXr224MG6sTp/u5 fnlQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707941417; x=1708546217; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=A3emXcyJNvJgc1uWaTQMfWeSm7Af4PAyEZnHoBa2y9Y=; b=F1wFFaAR7HaJKoR747ge5ulyd3b9eXol28hqktN/xxIZr3qYa0NumoaL0EgyLHX0FY gWDPT43jTCLrq42zrOWc4WF4kUyV1WmTLwgHJy76Nupg7f6Dnm54Ow1LIcXPNrQQH9g6 GAaPO4a3DJ6K/pCb2FH++9HQdUXWguguU/l+9xJ9hZizHtfw8NgcldfqeY1O83aSwhDi ONdXYxG6LdvHhU7zzjchxXyAQ+/L0Ts+pV6DNneXby01yy5uEl5Pwu1vj/HeqrLBGS8D hyuqGbX2J7YnMo48L7h4E6cCUNGBN+96wSwLmsdBPgJ6VSKgPnR/gGCz99mmi3ZqrB4q PcCQ== X-Forwarded-Encrypted: i=1; AJvYcCUr+oAbUfT5yGpu7HzDaWticunK8IDgOlF6qWAWr0cH7lWPKKdqrkGH8ql4xabpWxq2GqbGg9rIdwrMXeW2RBGimoc= X-Gm-Message-State: AOJu0Yz6VC13wLog7Kf+nRO6hRD7kZqNCDxjKYdjDKSM38vMaFaUUSuu cpzYGVZ+HxyCx0GY6aA1Q4Ee0KyfjWQGpFuQi+ySWczZSCs+8iObU1FjdFH6Fmz3R6tcTc3AT6m Cy9uOVwF0+DiFGBwaXg== X-Google-Smtp-Source: AGHT+IHBLukHbP00sGK4xmHtUcurDGukWxk53yrUcFWOBVYTNAm6XckUHU8SZszcT68dDDhBzLftJHp7gQoTkb1b X-Received: from yosry.c.googlers.com ([fda3:e722:ac3:cc00:20:ed76:c0a8:29b4]) (user=yosryahmed job=sendgmr) by 2002:a25:d842:0:b0:dc6:e8a7:fdba with SMTP id p63-20020a25d842000000b00dc6e8a7fdbamr754527ybg.4.1707941417656; Wed, 14 Feb 2024 12:10:17 -0800 (PST) Date: Wed, 14 Feb 2024 20:10:15 +0000 In-Reply-To: <20240210-zswap-global-lru-v2-2-fbee3b11a62e@bytedance.com> Mime-Version: 1.0 References: <20240210-zswap-global-lru-v2-0-fbee3b11a62e@bytedance.com> <20240210-zswap-global-lru-v2-2-fbee3b11a62e@bytedance.com> Message-ID: Subject: Re: [PATCH v2 2/2] mm/zswap: change zswap_pool kref to percpu_ref From: Yosry Ahmed To: Chengming Zhou Cc: Johannes Weiner , Andrew Morton , Nhat Pham , linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="us-ascii" X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: AF365C000A X-Stat-Signature: yqzwxg3mrpk9atoodigideotewpcsiod X-Rspam-User: X-HE-Tag: 1707941418-599437 X-HE-Meta: U2FsdGVkX19SHImo1k25oMfPl3FcvlnsADDlpRss9/vNhPBYvpG9iW7NTYLOuQZR51856hxSGqbIqImITmAPfkf1Q/Xy3xrcJk9aL6olfYCZQQaSOq6LKnka3m7K9Qh4UPvkxcw4/khb/mSM5UCo4fAb9QuwaeEnIY1MsMIBGMHRT7sDUq/ZXimK5WL1b9Evue8w9uDBosN+827gZ6Bps/PcAuhmYz6CkPhjCk6KTpyas7mTv+CAAKVFkxZWRfm0LzSfAqlvRBGbptmhmtVpIXfIZSti45+P/g31BiyJMnVo5zArxZ91KNo4yK2Sa2F1GzuvN6i5iGbDxJwM1ii2tTK5fwbczGzMOLigwsLhVUYMPjhGsc6BV77f0FCtQ3C0hBZNJhBk0v1KT8qo7abf6B06nE6msoRkn5uWfC2swb9Qe80pVdTFyfgF10GfT7ET4eIq9RsxZognpoo66ifzBe288MP2pkUI9tB/ZJJUCoW5oZG/SS3Ay7hW4rkU5COsHo/9Pj5djRFDucfOz2Lvba0m12a9sFgQzWDAU9l1WSLdCi6H6gpbY0rPlRDEnc5OTxfWsmh1xKh7QpnauWSgHreHU+TUnGNT+dTZx2nccm5WJCJ9dV2/ZCFvUyFqshQSrrHJ9bEnunimK4gLAi7/66vBIXCYjWOEbDtIGq97G0gDNRYuI+QJrZOp2a4aAgZpRloInbeojQwQg1nAlWsCU3HmbOSWurBUlickvqfphsQAzO3kuR1pHszdUfB7jWaWNo/f/SrM2r3zElk+Ldo7yPDj4/j7TlTyUHRt24QlYASkiupdMWOyWp3n2rEUCwMIAhxiERrC2wVtWGH8Tl0kHYlO3RT4UpuSggA+M71iJJnhd2WsjJ0KRH4lA0mdwSPkmpSJmhMzwzfFnfAsLKyemMXPwgrHcAWz7GerDqIRgzgjVnD8OIUooqdNTWq/QiTBhUPdMnw6gl/b54oXEcA +dFmpcYp 8kYJ6khheRTYXVjqVwKsCE+t4wiB66dsZJB3MU0Ic5wltdtamgJPuZoDt1hnxUKMTnLulTo0XUpFhhyLeMO2URPW+mGKwazVw1Hywgc1qfp2NcbohZM2q7MiIEdQFx+Hqxvcn74utHZjljn7jKpyNh/YAbRKsycx1wjbthVLFdGXpb/+E99SL43sxoOljtSwre6xJCYFXwabJnbbvesHYwNqF5MGeftrlrMCvtYMnxkavZjzZrEMG1U2uyb/0LhMXqWmC2i+0wko4M6dL3gyp5ZeIC8Bob1nCB7kgDlvnbVuWmhNvYHJ9g/0Ksbg6Ni32Wus+PuZr7Y7+ZU+tOApJiKMIwFg9OSVeAMl6al4GUA/960n9TKtfv5FwWiq5NpL88XWy2w0GZAGrKXmOku7RhrylZxzNe2QFzdrUBIeZ1IM66AfLQehANO6uIBFcMekP8linhHyUzEKiV2qbb53aW1yLQGi6svqX3ve8fOWVfA2eRzAD/Gvi5akbxLB0FspxluRAEbgcNpjnfvyfJHk085SFVMUQ1UJB526LXkrwxtB+fuAzBPUXLKeibtT9svisJK7i/lPUvY/7D8OPEZ9hngwuecDyzAsBKexl7jjXWfccl2AYzSeVUeGqiU5ASuyFmwI199u2wdR3312PRpQt4a1VFp6jWLDfcN9/yxi9jkyj4uWqUYzZhDdHLJdMndL1x2b8rq4/2wVYl7DfDb4bMhhBY0zMQeFJuiUP X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, 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, Feb 14, 2024 at 08:54:38AM +0000, Chengming Zhou wrote: > All zswap entries will take a reference of zswap_pool when > zswap_store(), and drop it when free. Change it to use the > percpu_ref is better for scalability performance. > > Although percpu_ref use a bit more memory which should be ok > for our use case, since we almost have only one zswap_pool to > be using. The performance gain is for zswap_store/load hotpath. > > Testing kernel build (32 threads) in tmpfs with memory.max=2GB. > (zswap shrinker and writeback enabled with one 50GB swapfile, > on a 128 CPUs x86-64 machine, below is the average of 5 runs) > > mm-unstable zswap-global-lru > real 63.20 63.12 > user 1061.75 1062.95 > sys 268.74 264.44 > > Signed-off-by: Chengming Zhou > --- > mm/zswap.c | 31 ++++++++++++++++++++++--------- > 1 file changed, 22 insertions(+), 9 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index dbff67d7e1c7..f6470d30d337 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -173,7 +173,7 @@ struct crypto_acomp_ctx { > struct zswap_pool { > struct zpool *zpools[ZSWAP_NR_ZPOOLS]; > struct crypto_acomp_ctx __percpu *acomp_ctx; > - struct kref kref; > + struct percpu_ref ref; > struct list_head list; > struct work_struct release_work; > struct hlist_node node; > @@ -304,6 +304,7 @@ static void zswap_update_total_size(void) > /********************************* > * pool functions > **********************************/ > +static void __zswap_pool_empty(struct percpu_ref *ref); > > static struct zswap_pool *zswap_pool_create(char *type, char *compressor) > { > @@ -357,13 +358,18 @@ static struct zswap_pool *zswap_pool_create(char *type, char *compressor) > /* being the current pool takes 1 ref; this func expects the > * caller to always add the new pool as the current pool > */ > - kref_init(&pool->kref); > + ret = percpu_ref_init(&pool->ref, __zswap_pool_empty, > + PERCPU_REF_ALLOW_REINIT, GFP_KERNEL); > + if (ret) > + goto ref_fail; > INIT_LIST_HEAD(&pool->list); > > zswap_pool_debug("created", pool); > > return pool; > > +ref_fail: > + cpuhp_state_remove_instance(CPUHP_MM_ZSWP_POOL_PREPARE, &pool->node); > error: > if (pool->acomp_ctx) > free_percpu(pool->acomp_ctx); > @@ -436,8 +442,9 @@ static void __zswap_pool_release(struct work_struct *work) > > synchronize_rcu(); > > - /* nobody should have been able to get a kref... */ > - WARN_ON(kref_get_unless_zero(&pool->kref)); > + /* nobody should have been able to get a ref... */ > + WARN_ON(percpu_ref_tryget(&pool->ref)); Just curious, was there any value from using kref_get_unless_zero() over kref_read() here? If not, I think percpu_ref_is_zero() is more intuitive. This also seems like it fits more as a debug check. > + percpu_ref_exit(&pool->ref); > > /* pool is now off zswap_pools list and has no references. */ > zswap_pool_destroy(pool); > @@ -445,11 +452,11 @@ static void __zswap_pool_release(struct work_struct *work) > > static struct zswap_pool *zswap_pool_current(void); > > -static void __zswap_pool_empty(struct kref *kref) > +static void __zswap_pool_empty(struct percpu_ref *ref) > { > struct zswap_pool *pool; > > - pool = container_of(kref, typeof(*pool), kref); > + pool = container_of(ref, typeof(*pool), ref); > > spin_lock(&zswap_pools_lock); > > @@ -468,12 +475,12 @@ static int __must_check zswap_pool_get(struct zswap_pool *pool) > if (!pool) > return 0; > > - return kref_get_unless_zero(&pool->kref); > + return percpu_ref_tryget(&pool->ref); > } > > static void zswap_pool_put(struct zswap_pool *pool) > { > - kref_put(&pool->kref, __zswap_pool_empty); > + percpu_ref_put(&pool->ref); > } > > static struct zswap_pool *__zswap_pool_current(void) > @@ -603,6 +610,12 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp, > > if (!pool) > pool = zswap_pool_create(type, compressor); > + else { > + /* Resurrect percpu_ref to percpu mode. */ > + percpu_ref_resurrect(&pool->ref); I think this is not very clear. The previous code relied on the ref from zswap_pool_find_get() to replace the initial ref that we had dropped before. This is not needed with percpu_ref_resurrect() because it already restores the initial ref dropped by percpu_ref_kill(). Perhaps something like: /* * Restore the initial ref dropped by percpu_ref_kill() * when the pool was decommissioned and switch it again * to percpu mode. / , or am I overthinking this? > + /* Drop the ref from zswap_pool_find_get(). */ > + zswap_pool_put(pool); > + } > > if (pool) > ret = param_set_charp(s, kp); > @@ -641,7 +654,7 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp, > * or the new pool we failed to add > */ > if (put_pool) > - zswap_pool_put(put_pool); > + percpu_ref_kill(&put_pool->ref); > > return ret; > } > > -- > b4 0.10.1