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 74A9EC0032E for ; Fri, 20 Oct 2023 19:58:34 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0AA6C28000C; Fri, 20 Oct 2023 15:58:34 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 05919280009; Fri, 20 Oct 2023 15:58:34 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E3C4A28000C; Fri, 20 Oct 2023 15:58:33 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id CE8CB280009 for ; Fri, 20 Oct 2023 15:58:33 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 8DA991A0894 for ; Fri, 20 Oct 2023 19:58:33 +0000 (UTC) X-FDA: 81366902106.29.B3BB38A Received: from mail-il1-f172.google.com (mail-il1-f172.google.com [209.85.166.172]) by imf10.hostedemail.com (Postfix) with ESMTP id 93547C001C for ; Fri, 20 Oct 2023 19:58:31 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=cF2xpR77; spf=pass (imf10.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.166.172 as permitted sender) smtp.mailfrom=nphamcs@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1697831911; 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=DMaTpdy0i75iy95ltl1vU/e0Nep1hsEs0WMhGjfkl4k=; b=C7KDI7yY2ZAzTDPWe2HQ6cRBZt85QLTuFEHFaZnm7MBazEAmpYQdusZBUOUvaLwZ6q2aiQ 4Z8kkvB2vbuFp8aEB95Ee7rfA+o7J8VLyhEcRqD+nF24AxBAfrag6faIaVqjLrmoXBSLx2 rNJVGS/RhVmFSpEtJlcN0iURlMmdW6s= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=cF2xpR77; spf=pass (imf10.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.166.172 as permitted sender) smtp.mailfrom=nphamcs@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1697831911; a=rsa-sha256; cv=none; b=DsPaT5ZC1hnQKxqrMBWc0ONR2W6zdGOs8frTMTM9C/3aimmATc8j2C596GfamDYAUChtVn YbrCprIkqgvwthBlpBWPkzqc416stwyVmkykYi/tdWqSfA3K1D3Nh2yg9THbThwUIt4MrX AQzmyzR0xrHvC4xPHsu7k8Br6XlzHM8= Received: by mail-il1-f172.google.com with SMTP id e9e14a558f8ab-357c3dc2ffbso548045ab.2 for ; Fri, 20 Oct 2023 12:58:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1697831911; x=1698436711; 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=DMaTpdy0i75iy95ltl1vU/e0Nep1hsEs0WMhGjfkl4k=; b=cF2xpR77Ok306vJPmYkrVoDXiv0TN9pzvjBB1MpaiFtxamIPG0W68/zCHODvTG7PnD E+qtIwqv8lWNokN3WMfb9cXSphWap5nlXbcjMwOGROcrwsyA9uuIXRTkmoTRRzCOepYz IUGdjVKiTPuwLKFjJeTDtcZwwZFcGOTgybevZRnQnV9CbBdNWTPI4bBGb6z50HKn2Tul Fvy+VOKIuN3oFsuWqQbkCgeF7UVqkvN7qnfM0HbJyGD0oetBm2OHr93oZ6Ztf4zfe1Ho yOf02UjIP2NGETMEG0b7TbwlvLoFJFnqT+IOKyhfI4CJ7+ZjnnrRYzoakOBjH9Xee5sF mMig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697831911; x=1698436711; 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=DMaTpdy0i75iy95ltl1vU/e0Nep1hsEs0WMhGjfkl4k=; b=YhHPV8k0iGVAQwE6HWrONJ1976Q/tY+6jnlaiCKPByvKCqomEj9ZvLoQGgCRHDSYo9 Zr2O43yfdrnpuEtN8HBTej8d1+R7l56Z5hlJo0OIxupPcrH7U9ivi+G3EBCnH1w95c81 5SADEEWyoObLmdwu2lsA3hM4H4SDHovxH0wZpDfe/ER8Lmr4SCUdeAlSvGPGzA9P75SA s6lUxppj07n8/VepGu+n3jOOuK2LO1GLnYVrWWBgH6xF9b6SGd8rFolU8tJ/tt2yzHCd C6MLRPG9ufvLncL2TS5IaZsFU8te/g7c7KZS/ebjGapappJGsOInhDEbXOUyPRn20JTJ 55UQ== X-Gm-Message-State: AOJu0YzS6Pe44gYg2r1xiN5t0stwzfUcj5GT90L0VEf/bL3lXiUst4qA m1M69dXt/y3KXLCVJ+ARGXispdqywQwvGEeFbCA= X-Google-Smtp-Source: AGHT+IGR8W7Xi0YQ/7dCUZ2Jb3c+qTsFkmiWPSTao7GyIUJ9F1uL0JV4qYZmZW/AoBcAKMHA3vnqiBBpxyI24w52tdA= X-Received: by 2002:a92:c26a:0:b0:34f:f2d3:ea70 with SMTP id h10-20020a92c26a000000b0034ff2d3ea70mr4009375ild.6.1697831910562; Fri, 20 Oct 2023 12:58:30 -0700 (PDT) MIME-Version: 1.0 References: <20231017232152.2605440-1-nphamcs@gmail.com> <20231017232152.2605440-3-nphamcs@gmail.com> In-Reply-To: From: Nhat Pham Date: Fri, 20 Oct 2023 12:58:19 -0700 Message-ID: Subject: Re: [PATCH v3 2/5] zswap: make shrinking memcg-aware To: Yosry Ahmed Cc: Domenico Cerasuolo , akpm@linux-foundation.org, hannes@cmpxchg.org, sjenning@redhat.com, ddstreet@ieee.org, vitaly.wool@konsulko.com, mhocko@kernel.org, roman.gushchin@linux.dev, shakeelb@google.com, muchun.song@linux.dev, linux-mm@kvack.org, kernel-team@meta.com, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org, shuah@kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 93547C001C X-Rspam-User: X-Stat-Signature: w1gr45hipuxmh76m94kx88gpnbna1c9i X-Rspamd-Server: rspam01 X-HE-Tag: 1697831911-219054 X-HE-Meta: U2FsdGVkX19rmzJfMH+SfRr1dFDzjSBv9aA5FG1f4htD5EFmK3de6cVJfcWjC/+z+hZJFbOi8YSHHIlgIGu5FozFauZHGoZG5zXkIL7n7RtzGAz0kHLweMPHzhLSj4dYW4B6xsgZ+57lEqBnBW0TfjLxWsVtvTXDZg0POL8P09nvFe05MclljiZcqbrtzK3XShyt0xxRJF0N8f5Q8JJUdfz+2Mb8P/0/xtKGhcPOdHUceN8I6tFN7VTN61kfCjrgveu0OjwfM5jhMX4oZXn2deM8qvclnbrWBK2Rtlzx3aZkmEjPfTUPmzxIGEmY453yPwzMZC6FcA9wotQM0hCSIbPEHTTUC7yk48fCQaU1sVMYoi9bKLAOGDAFj29UXrrgp3gUZTIRRjit9gmYdw8mY7tIbNlDNCJawiNzRrwOtT7RKVY1PszhR3CNLW8X1bTeABGFC27M1ItY/5S+thN0SqhoSJMgm2tbczAGpQLVt+xF3UrKLumH3a1Pa4QfbK/y8d6BDWxbcKZLgmMnOb8ZnUC+xfZ1CcDj9nhQiRVzQf7aDuaV+fmdAWoQSyNcim6YnjAOhZbz0mVf+H+58eJsDj5TdmUkTWVm3c112Tj5GK+hwueW04fjcrwNLwj0IfZgmrEDXpxRVPeXrzM7JkjfybR1TH7vg00nLjRqnAmgNWvzfYNqCNNOf+ZNHA2Hx2m6PXeZLG7MC/g8D+iPXomVeHAbVeNi/VvdLWfecAtaWB9FzqFN9AfBzVwa0lbLzLRjcLNu8ZpFHio1vTQZHLh9G3LfOXOMyBLR62MlBpiAsCHR/xqso8VQuqypnw3ih+iktxecHJFqI02vfNY31ONE2g+2Mf30InnK1rDMUbTLs1M9vNlutSo7Q1dyRBDHjrmEA60Aq1euz7+IXxlMN1caQnRfbYBjsfbUTiNb+pKbulCpchHWZ6xCse8eV/mt/q3a8bvxPaeF1jdtLyJh+DE C/KkUnsL sqR942oBot4TF5F8YfSRjZnlLeqbVWYA0C75g6AmsXcROIYF9AsL5AAMDz6Q9kuOmPt2KStF+KI32ZgDhHIb7x9Ig6pNYQAD2JAIhQ5AaIS4Awv+RoBkqcucw79wj17XWXxafoxqzvg4uhyWMyyxH9Nb57Y4k6cH7yURAow3Z3+d6u4TLQSmIR9fjbUqEDciQZ8xkhvFEU7ifrpcdOCAHWwyhYh/64EnKsNKWkG5SStznOca0GQLAb+8MK1e2f5y86bFEXN8IZqC8rYd0GIiiaHac6PjSyr+QUH8bxfghrWWmy4Vh71uqn2m1q4q2caanegWfet76UYY48rw4ULhUPLYptJ1th3qLm33xkp0/76jAdo4EVCiy9TnSnBWssWlHOsxP 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: On Thu, Oct 19, 2023 at 9:15=E2=80=AFAM Yosry Ahmed = wrote: > > [..] > > > > > > > > +/********************************* > > > > +* lru functions > > > > +**********************************/ > > > > +static bool zswap_lru_add(struct list_lru *list_lru, struct zswap_= entry *entry) > > > > +{ > > > > + struct mem_cgroup *memcg =3D get_mem_cgroup_from_entry(entr= y); > > > > > > Could we avoid the need for get/put with an rcu_read_lock() instead? > > > > I think we can, I'm not entirely sure of the consequences though. By th= e > > look of it I'd say it's safe but I wouldn't trust my judgement on this. > > It just seems like we have a pattern of short-lived get/put. If RCU > gives enough protection it should be simpler. IIUC taking a reference > does not protect against offlining or reparenting, so I am not sure if > taking a reference here would provide any more protection than > I'd keep it for now. Sounds like an optimization to me, which could always be done as a follow-up :) Unless of course, if somebody has a really strong opinion regarding this. > > > > > > [..] > > > > @@ -686,7 +716,36 @@ static int zswap_reclaim_entry(struct zswap_po= ol *pool) > > > > zswap_entry_put(tree, entry); > > > > unlock: > > > > spin_unlock(&tree->lock); > > > > - return ret ? -EAGAIN : 0; > > > > + spin_lock(lock); > > > > + return ret; > > > > +} > > > > + > > > > +static int shrink_memcg(struct mem_cgroup *memcg) > > > > +{ > > > > + struct zswap_pool *pool; > > > > + int nid, shrunk =3D 0; > > > > + > > > > + pool =3D zswap_pool_current_get(); > > > > + if (!pool) > > > > + return -EINVAL; > > > > + > > > > + /* > > > > + * Skip zombies because their LRUs are reparented and we wo= uld be > > > > + * reclaiming from the parent instead of the dead memcgroup= . > > > > > > nit: s/memcgroup/memcg. > > > > > > > + */ > > > > + if (memcg && !mem_cgroup_online(memcg)) > > > > + goto out; > > > > > > If we move this above zswap_pool_current_get(), we can return directl= y > > > and remove the label. I noticed we will return -EAGAIN if memcg is > > > offline. IIUC -EAGAIN for the caller will move on to the next memcg, > > > but I am wondering if a different errno would be clearer here. > > > > True, I remember spending some time staring at error codes but couldn't= find a > > better one. What if we use -EINVAL for retryable errors, and use someth= ing else > > for the one where there is no pool? -ENODEV? > > Do you mean -EINVAL for non-retryable errors? Perhaps -ENOENT is more > appropriate as a return for offline memcgs? > > > > > > > [..] > > > > static void shrink_worker(struct work_struct *w) > > > > @@ -695,10 +754,13 @@ static void shrink_worker(struct work_struct = *w) > > > > shrink_work); > > > > int ret, failures =3D 0; > > > > > > > > + /* global reclaim will select cgroup in a round-robin fashi= on. */ > > > > do { > > > > - ret =3D zswap_reclaim_entry(pool); > > > > + pool->next_shrink =3D mem_cgroup_iter(NULL, pool->n= ext_shrink, NULL); > > > > > > Perhaps next_shrink_memcg is a better name here? > > > > Will change if you have a strong preference, I'd keep it shorter becaus= e it's > > always used in conjunction with a memcg type or function. > > I'd rather have the more explicit name unless it causes some annoying > line breaks or so. > > > > > > > > > > + > > > > + ret =3D shrink_memcg(pool->next_shrink); > > > > + > > > > if (ret) { > > > > - zswap_reject_reclaim_fail++; > > > > if (ret !=3D -EAGAIN) > > > > break; > > > > if (++failures =3D=3D MAX_RECLAIM_RETRIES) > > > > @@ -764,8 +826,7 @@ static struct zswap_pool *zswap_pool_create(cha= r *type, char *compressor) > > > > */ > > > > kref_init(&pool->kref); > > > > INIT_LIST_HEAD(&pool->list); > > > > - INIT_LIST_HEAD(&pool->lru); > > > > - spin_lock_init(&pool->lru_lock); > > > > + list_lru_init_memcg(&pool->list_lru, NULL); > > > > INIT_WORK(&pool->shrink_work, shrink_worker); > > > > > > > > zswap_pool_debug("created", pool); > > > > @@ -831,6 +892,9 @@ static void zswap_pool_destroy(struct zswap_poo= l *pool) > > > > > > > > cpuhp_state_remove_instance(CPUHP_MM_ZSWP_POOL_PREPARE, &po= ol->node); > > > > free_percpu(pool->acomp_ctx); > > > > + list_lru_destroy(&pool->list_lru); > > > > + if (pool->next_shrink) > > > > + mem_cgroup_put(pool->next_shrink); > > > > for (i =3D 0; i < ZSWAP_NR_ZPOOLS; i++) > > > > zpool_destroy_pool(pool->zpools[i]); > > > > kfree(pool); > > > > @@ -1076,7 +1140,7 @@ static int zswap_writeback_entry(struct zswap= _entry *entry, > > > > > > > > /* try to allocate swap cache page */ > > > > page =3D __read_swap_cache_async(swpentry, GFP_KERNEL, NULL= , 0, > > > > - &page_was_allocated); > > > > + &page_was_allocated, true); > > > > if (!page) { > > > > ret =3D -ENOMEM; > > > > goto fail; > > > > @@ -1142,7 +1206,6 @@ static int zswap_writeback_entry(struct zswap= _entry *entry, > > > > /* start writeback */ > > > > __swap_writepage(page, &wbc); > > > > put_page(page); > > > > - zswap_written_back_pages++; > > > > > > > > return ret; > > > > > > > > @@ -1199,8 +1262,10 @@ bool zswap_store(struct folio *folio) > > > > struct scatterlist input, output; > > > > struct crypto_acomp_ctx *acomp_ctx; > > > > struct obj_cgroup *objcg =3D NULL; > > > > + struct mem_cgroup *memcg =3D NULL; > > > > struct zswap_pool *pool; > > > > struct zpool *zpool; > > > > + int lru_alloc_ret; > > > > unsigned int dlen =3D PAGE_SIZE; > > > > unsigned long handle, value; > > > > char *buf; > > > > @@ -1230,15 +1295,15 @@ bool zswap_store(struct folio *folio) > > > > zswap_invalidate_entry(tree, dupentry); > > > > } > > > > spin_unlock(&tree->lock); > > > > - > > > > - /* > > > > - * XXX: zswap reclaim does not work with cgroups yet. Witho= ut a > > > > - * cgroup-aware entry LRU, we will push out entries system-= wide based on > > > > - * local cgroup limits. > > > > - */ > > > > objcg =3D get_obj_cgroup_from_folio(folio); > > > > - if (objcg && !obj_cgroup_may_zswap(objcg)) > > > > - goto reject; > > > > + if (objcg && !obj_cgroup_may_zswap(objcg)) { > > > > + memcg =3D get_mem_cgroup_from_objcg(objcg); > > > > + if (shrink_memcg(memcg)) { > > > > + mem_cgroup_put(memcg); > > > > + goto reject; > > > > + } > > > > + mem_cgroup_put(memcg); > > > > + } > > > > > > > > /* reclaim space if needed */ > > > > if (zswap_is_full()) { > > > > @@ -1254,10 +1319,15 @@ bool zswap_store(struct folio *folio) > > > > zswap_pool_reached_full =3D false; > > > > } > > > > > > > > + pool =3D zswap_pool_current_get(); > > > > + if (!pool) > > > > + goto reject; > > > > + > > > > > > Why do we need to move zswap_pool_current_get() up here? > > > > Ah, thanks. This is a leftover from a previous version where the pool w= as needed > > to allocate the entry. > > > > > > > > > > > /* allocate entry */ > > > > - entry =3D zswap_entry_cache_alloc(GFP_KERNEL); > > > > + entry =3D zswap_entry_cache_alloc(GFP_KERNEL, page_to_nid(p= age)); > > > > if (!entry) { > > > > zswap_reject_kmemcache_fail++; > > > > + zswap_pool_put(pool); > > > > goto reject; > > > > } > > > > > > > > @@ -1269,6 +1339,7 @@ bool zswap_store(struct folio *folio) > > > > entry->length =3D 0; > > > > entry->value =3D value; > > > > atomic_inc(&zswap_same_filled_pages); > > > > + zswap_pool_put(pool); > > > > goto insert_entry; > > > > } > > > > kunmap_atomic(src); > > > > @@ -1278,9 +1349,15 @@ bool zswap_store(struct folio *folio) > > > > goto freepage; > > > > > > > > /* if entry is successfully added, it keeps the reference *= / > > > > - entry->pool =3D zswap_pool_current_get(); > > > > - if (!entry->pool) > > > > - goto freepage; > > > > + entry->pool =3D pool; > > > > + if (objcg) { > > > > + memcg =3D get_mem_cgroup_from_objcg(objcg); > > > > + lru_alloc_ret =3D memcg_list_lru_alloc(memcg, &pool= ->list_lru, GFP_KERNEL); > > > > + mem_cgroup_put(memcg); > > > > + > > > > + if (lru_alloc_ret) > > > > + goto freepage; > > > > + } > > > > > > > > /* compress */ > > > > acomp_ctx =3D raw_cpu_ptr(entry->pool->acomp_ctx); > > > > @@ -1358,9 +1435,8 @@ bool zswap_store(struct folio *folio) > > > > zswap_invalidate_entry(tree, dupentry); > > > > } > > > > if (entry->length) { > > > > - spin_lock(&entry->pool->lru_lock); > > > > - list_add(&entry->lru, &entry->pool->lru); > > > > - spin_unlock(&entry->pool->lru_lock); > > > > + INIT_LIST_HEAD(&entry->lru); > > > > + zswap_lru_add(&pool->list_lru, entry); > > > > } > > > > spin_unlock(&tree->lock); > > > > > > > > @@ -1373,8 +1449,8 @@ bool zswap_store(struct folio *folio) > > > > > > > > put_dstmem: > > > > mutex_unlock(acomp_ctx->mutex); > > > > - zswap_pool_put(entry->pool); > > > > freepage: > > > > + zswap_pool_put(entry->pool); > > > > zswap_entry_cache_free(entry); > > > > reject: > > > > if (objcg) > > > > @@ -1467,9 +1543,8 @@ bool zswap_load(struct folio *folio) > > > > zswap_invalidate_entry(tree, entry); > > > > folio_mark_dirty(folio); > > > > } else if (entry->length) { > > > > - spin_lock(&entry->pool->lru_lock); > > > > - list_move(&entry->lru, &entry->pool->lru); > > > > - spin_unlock(&entry->pool->lru_lock); > > > > + zswap_lru_del(&entry->pool->list_lru, entry); > > > > + zswap_lru_add(&entry->pool->list_lru, entry); > > > > } > > > > zswap_entry_put(tree, entry); > > > > spin_unlock(&tree->lock); > > > > -- > > > > 2.34.1