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 C5A1AC77B7A for ; Wed, 7 Jun 2023 09:31:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4D66C6B0071; Wed, 7 Jun 2023 05:31:56 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 486AB8E0002; Wed, 7 Jun 2023 05:31:56 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 326788E0001; Wed, 7 Jun 2023 05:31:56 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 1FBC66B0071 for ; Wed, 7 Jun 2023 05:31:56 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id E6AE8401B4 for ; Wed, 7 Jun 2023 09:31:55 +0000 (UTC) X-FDA: 80875434990.28.C097191 Received: from mail-ej1-f47.google.com (mail-ej1-f47.google.com [209.85.218.47]) by imf24.hostedemail.com (Postfix) with ESMTP id D1577180018 for ; Wed, 7 Jun 2023 09:31:53 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=d1YSvvcA; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf24.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.47 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1686130314; 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=vwYzR2o8WcAa8FfzYaLSns1WLIjgOiUcygsuRU8mknw=; b=K9G0UcUj4c78exDQe5zMT5pMZJvO98DrlDBIiZv/048wrQmG1LdCF/DJE1nWbAkW3WYDDq R8DtZVYV8ErMuegfc0CCmWU1+SuUfF6ck4DG6pQBn8OPd7lCZa5RLAQ+rFMmm5zHCdQSv6 hwJXiQz06EVz/LygxmiEarzH0giRlAM= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=d1YSvvcA; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf24.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.47 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1686130314; a=rsa-sha256; cv=none; b=d0FmgfTxIBnSlyyvbwsA0YZSDSMuw4X+ghoOZBKIVum7ssodmTRspCDHNNXjtOclCCyAn9 J3114QSUvS3Vb7v4+RCHfj81Lwf8E8+7wVUoBK/0pirkeTUGlG0gt56MgG1ggspGLl5d3f m1qA0uTnh1z2hHqJt0b08vG1Cmr8VkE= Received: by mail-ej1-f47.google.com with SMTP id a640c23a62f3a-977d02931d1so561380266b.0 for ; Wed, 07 Jun 2023 02:31:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1686130312; x=1688722312; 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=vwYzR2o8WcAa8FfzYaLSns1WLIjgOiUcygsuRU8mknw=; b=d1YSvvcAKQjI9TUdpM+UopsaTzWP2zetxJBwq5hkUwpByIItXy5b/+xShV3YaahRD9 Z/1/zxiXuLzdSakyQMHE9A8freGIRNnWw4X4WCWvTRxP26Qc09jZKhq61H6FIvcvTFwK oIw9iA0KtCyRtNRohz/LnWIjZnXKZ0h8HQx0UspymZavnEI+ruyjzJ5hCO+QOMYz+C4D MTJfMfoeFcV8m8MYiQlEb0TBODo8SCrUirO8fp4sfKEQ6sKg+ebTCul13hhNdpgvHfCC wNsbovZGE+OeNf7dRmuLPJbC5hctx/YpSwAG4MgThLG8Mkd4B++enai1lwADU6dpjIjN PbtQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686130312; x=1688722312; 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=vwYzR2o8WcAa8FfzYaLSns1WLIjgOiUcygsuRU8mknw=; b=iEJgqHe0GsBP3WyBDI/TiphsOFM8SAeRcd4NmmbOTAoQvPlWxJUonhBRDpylPiY6hd HxUHdkQjzoYNIBqgHP6Hgp97X4u5VUrg7fzRJbRTuaXPKTBsX4OYXYj4ejwd/vo5BEna o4fESuaR1vVo18FG1phWcIsF+a2TvSLFsSCuHu9RQHNJ8bU/ptTCwuTMtWXFBf9sWdNt yN0Vr8CcCUMe76lzsgYHwV872cV7+kX8iUE6qND+MWVHK0eKyj4rKEMJLmGctDQqaPlN x50q2S4OutExQunhRwre15jLDeHAOcjzswU/dSqdpjULcf+3Gur22hTjiThYhSIbmZo2 B7Vg== X-Gm-Message-State: AC+VfDyH4ydqvjNN9wt/SD4ERISG7vE99xxYUT4pDuGPQqzNqHFiQfIn O95hY9nIy0tTiAzchpz7bOBeHPc/OhXvdI/wANyRoQ== X-Google-Smtp-Source: ACHHUZ7QY+KBO9LyrQdx7ajNY2km1lfdMJ0tZIqwSdP7UlZ1kFzU0bYIEnBN7e3JcPAunccbhOnZoiosXw494Bl8JtE= X-Received: by 2002:a17:907:d93:b0:959:5407:3e65 with SMTP id go19-20020a1709070d9300b0095954073e65mr5702271ejc.55.1686130312231; Wed, 07 Jun 2023 02:31:52 -0700 (PDT) MIME-Version: 1.0 References: <20230606145611.704392-1-cerasuolodomenico@gmail.com> <20230606145611.704392-2-cerasuolodomenico@gmail.com> In-Reply-To: From: Yosry Ahmed Date: Wed, 7 Jun 2023 02:31:15 -0700 Message-ID: Subject: Re: [RFC PATCH v2 1/7] mm: zswap: add pool shrinking mechanism To: Domenico Cerasuolo Cc: vitaly.wool@konsulko.com, minchan@kernel.org, senozhatsky@chromium.org, linux-mm@kvack.org, ddstreet@ieee.org, sjenning@redhat.com, nphamcs@gmail.com, hannes@cmpxchg.org, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, kernel-team@meta.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: D1577180018 X-Stat-Signature: xsibc51bqj3znzmdwzq1ieike7de43sn X-Rspam-User: X-HE-Tag: 1686130313-71621 X-HE-Meta: U2FsdGVkX1+bz7DrcS2OSyz6O+I9gb/bjKrynTSC5y0PY3MnZLtgZIJzPy36oKxCkEgRexT/rm0AOwbDf/RdKE6AF6MV+tTMp0+mE53WsFFJs+t5Ir853bBY89D+C6DshoHpCnkd3YobFM+KrjYUddJ/VdHhXlzkqvEIODweyknFq8uC2rJPqVJkUi1bM3eF3SJjFlTOmtRQ1NGq4yCTvcnSDHJelbc1B5XDBleHJP0TVnvtYfGA7LGOk+HzG8CHNpi95CSl6IJHh06c8k+zSUe6HtNrBi5Ju5Qlz8YPjb7AK+S7Gln+C1ehZPz01Q5jgTl7+AKriF8KeTgIPExrL/gcG9Bl1bvcNJboKlQVjmBeR5I9LVEZR0Z6xFel+RIFUdgW9RTbrqhTkI21KcyvyY3L+EGvvm8siAU8Qz1TszkhB1b9dHJ4hSRfPco8K6zJNxjs3u5wPi5ZugynEmBoX713Ry605jAFYNKD7K5MRMCozH4yVkbEuyCnN6whogGTFecjyCEaIhPf2dasSUIhFnyqQzx2LQgcbKf990srFswCfRUxL5pkEDbs8yC0he2wRlxdazEFzGfv/AG/yDXnTPmOBK7nbixCufUhxq9qss+vbGJerlvQ2zl/Dk9ZolH5T6deeQZf8wcX9UZQgkBZeSt1DQiE47wQWx1aOHC5MgXgTk0BEqPri8QhsXld1LV55aOd2zvI8mXInuyQKjo8ej19A/aS8L0M9PF5lH7oVcYPdqJm+D08bnnf9OxCR28VyCorPxrWhPejy9i0U8GGsrkH/fRcM/Gl7qXkZS4VXtTxaqBCzQ2quHzdQSAH+bWgQSHutI5q5S6SHWZVU3W93mfbSpjRgV+t/lcwopPzC4LCqKty6m9BJal59pkKXv29d1WTm/JGvXzdxJerZqEDPtkpIsqUUN+CO17UE44Rx0gsReye4+Zf21P/sC0NTq/DJvHb+XD2w6+g38bLsd7 foUaPSk6 LrJ6g5kaP5W9XmL4NGv6wnj8sodX4zQoPpcvT05Hmo/SgJTxPo4rfW3ZFnuQRFOHxGQeuMowFGsmbCVBkXZDPvU6ebS17dXjlkHglWVHybQ4YBowa4Gi8w10Ce/wWuhCYQ68jKyNoLssqNrSjAIfr0wVTemTo0UIWQyvK4kFpOnpbmUjhlNcyMcT9g2OOb+Z4lOaWZV24wJnSqXWYMFXISmbULEw5DXc+pD2uMepf5QSInxmkF8rqX9bX/9E1ZnAnZhzqwQropKJYe10igPaOFLQfJS9hR0dl2lFxsXqdkifBbkSjSyp8PvnOcV6UVk1KRHp5xbLnEdL+2hY= 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 Wed, Jun 7, 2023 at 2:22=E2=80=AFAM Domenico Cerasuolo wrote: > > On Wed, Jun 7, 2023 at 10:14=E2=80=AFAM Yosry Ahmed wrote: > > > > On Tue, Jun 6, 2023 at 7:56=E2=80=AFAM Domenico Cerasuolo > > wrote: > > > > > > Each zpool driver (zbud, z3fold and zsmalloc) implements its own shri= nk > > > function, which is called from zpool_shrink. However, with this commi= t, > > > a unified shrink function is added to zswap. The ultimate goal is to > > > eliminate the need for zpool_shrink once all zpool implementations ha= ve > > > dropped their shrink code. > > > > > > To ensure the functionality of each commit, this change focuses solel= y > > > on adding the mechanism itself. No modifications are made to > > > the backends, meaning that functionally, there are no immediate chang= es. > > > The zswap mechanism will only come into effect once the backends have > > > removed their shrink code. The subsequent commits will address the > > > modifications needed in the backends. > > > > > > Signed-off-by: Domenico Cerasuolo > > > --- > > > mm/zswap.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++-= -- > > > 1 file changed, 91 insertions(+), 5 deletions(-) > > > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > > index bcb82e09eb64..c99bafcefecf 100644 > > > --- a/mm/zswap.c > > > +++ b/mm/zswap.c > > > @@ -150,6 +150,12 @@ struct crypto_acomp_ctx { > > > struct mutex *mutex; > > > }; > > > > > > +/* > > > + * The lock ordering is zswap_tree.lock -> zswap_pool.lru_lock. > > > + * The only case where lru_lock is not acquired while holding tree.l= ock is > > > + * when a zswap_entry is taken off the lru for writeback, in that ca= se it > > > + * needs to be verified that it's still valid in the tree. > > > + */ > > > struct zswap_pool { > > > struct zpool *zpool; > > > struct crypto_acomp_ctx __percpu *acomp_ctx; > > > @@ -159,6 +165,8 @@ struct zswap_pool { > > > struct work_struct shrink_work; > > > struct hlist_node node; > > > char tfm_name[CRYPTO_MAX_ALG_NAME]; > > > + struct list_head lru; > > > + spinlock_t lru_lock; > > > }; > > > > > > /* > > > @@ -176,10 +184,12 @@ struct zswap_pool { > > > * be held while changing the refcount. Since the lock m= ust > > > * be held, there is no reason to also make refcount atom= ic. > > > * length - the length in bytes of the compressed page data. Needed= during > > > - * decompression. For a same value filled page length is 0. > > > + * decompression. For a same value filled page length is 0,= and both > > > + * pool and lru are invalid and must be ignored. > > > * pool - the zswap_pool the entry's data is in > > > * handle - zpool allocation handle that stores the compressed page = data > > > * value - value of the same-value filled pages which have same cont= ent > > > + * lru - handle to the pool's lru used to evict pages. > > > */ > > > struct zswap_entry { > > > struct rb_node rbnode; > > > @@ -192,6 +202,7 @@ struct zswap_entry { > > > unsigned long value; > > > }; > > > struct obj_cgroup *objcg; > > > + struct list_head lru; > > > }; > > > > > > struct zswap_header { > > > @@ -364,6 +375,12 @@ static void zswap_free_entry(struct zswap_entry = *entry) > > > if (!entry->length) > > > atomic_dec(&zswap_same_filled_pages); > > > else { > > > + /* zpool_evictable will be removed once all 3 backends have m= igrated */ > > > + if (!zpool_evictable(entry->pool->zpool)) { > > > + spin_lock(&entry->pool->lru_lock); > > > + list_del(&entry->lru); > > > + spin_unlock(&entry->pool->lru_lock); > > > + } > > > zpool_free(entry->pool->zpool, entry->handle); > > > zswap_pool_put(entry->pool); > > > } > > > @@ -584,14 +601,70 @@ static struct zswap_pool *zswap_pool_find_get(c= har *type, char *compressor) > > > return NULL; > > > } > > > > > > +static int zswap_shrink(struct zswap_pool *pool) > > > > Nit: rename to zswap_shrink_one() so that it's clear we always > > writeback one entry per call? > > I named it like that to mirror zpool_shrink but I think that you've got a= point > in that it might not be very clear that it is shrinking by one page only. > What about zswap_reclaim_entry? I'm not a native speaker, but with > zswap_shrink_one I wouldn't obviously intend that the "one" refers to an > entry. I am not a native speaker either, but zswap_reclaim_entry() sounds much better :) > > > > > > +{ > > > + struct zswap_entry *lru_entry, *tree_entry =3D NULL; > > > + struct zswap_header *zhdr; > > > + struct zswap_tree *tree; > > > + int swpoffset; > > > + int ret; > > > + > > > + /* get a reclaimable entry from LRU */ > > > + spin_lock(&pool->lru_lock); > > > + if (list_empty(&pool->lru)) { > > > + spin_unlock(&pool->lru_lock); > > > + return -EINVAL; > > > + } > > > + lru_entry =3D list_last_entry(&pool->lru, struct zswap_entry,= lru); > > > + list_del_init(&lru_entry->lru); > > > + zhdr =3D zpool_map_handle(pool->zpool, lru_entry->handle, ZPO= OL_MM_RO); > > > + tree =3D zswap_trees[swp_type(zhdr->swpentry)]; > > > + zpool_unmap_handle(pool->zpool, lru_entry->handle); > > > + /* > > > + * Once the pool lock is dropped, the lru_entry might get fre= ed. The > > > > Nit: lru lock* > > > > > + * swpoffset is copied to the stack, and lru_entry isn't dere= f'd again > > > + * until the entry is verified to still be alive in the tree. > > > + */ > > > + swpoffset =3D swp_offset(zhdr->swpentry); > > > + spin_unlock(&pool->lru_lock); > > > + > > > + /* hold a reference from tree so it won't be freed during wri= teback */ > > > + spin_lock(&tree->lock); > > > + tree_entry =3D zswap_entry_find_get(&tree->rbroot, swpoffset)= ; > > > + if (tree_entry !=3D lru_entry) { > > > + if (tree_entry) > > > + zswap_entry_put(tree, tree_entry); > > > + spin_unlock(&tree->lock); > > > + return -EAGAIN; > > > + } > > > + spin_unlock(&tree->lock); > > > + > > > + ret =3D zswap_writeback_entry(pool->zpool, lru_entry->handle)= ; > > > + > > > + spin_lock(&tree->lock); > > > + if (ret) { > > > + spin_lock(&pool->lru_lock); > > > + list_move(&lru_entry->lru, &pool->lru); > > > + spin_unlock(&pool->lru_lock); > > > + } > > > + zswap_entry_put(tree, tree_entry); > > > + spin_unlock(&tree->lock); > > > + > > > + return ret ? -EAGAIN : 0; > > > +} > > > + > > > static void shrink_worker(struct work_struct *w) > > > { > > > struct zswap_pool *pool =3D container_of(w, typeof(*pool), > > > shrink_work); > > > int ret, failures =3D 0; > > > > > > + /* zpool_evictable will be removed once all 3 backends have m= igrated */ > > > do { > > > - ret =3D zpool_shrink(pool->zpool, 1, NULL); > > > + if (zpool_evictable(pool->zpool)) > > > + ret =3D zpool_shrink(pool->zpool, 1, NULL); > > > + else > > > + ret =3D zswap_shrink(pool); > > > if (ret) { > > > zswap_reject_reclaim_fail++; > > > if (ret !=3D -EAGAIN) > > > @@ -655,6 +728,8 @@ static struct zswap_pool *zswap_pool_create(char = *type, char *compressor) > > > */ > > > kref_init(&pool->kref); > > > INIT_LIST_HEAD(&pool->list); > > > + INIT_LIST_HEAD(&pool->lru); > > > + spin_lock_init(&pool->lru_lock); > > > INIT_WORK(&pool->shrink_work, shrink_worker); > > > > > > zswap_pool_debug("created", pool); > > > @@ -1270,7 +1345,7 @@ static int zswap_frontswap_store(unsigned type,= pgoff_t offset, > > > } > > > > > > /* store */ > > > - hlen =3D zpool_evictable(entry->pool->zpool) ? sizeof(zhdr) := 0; > > > + hlen =3D sizeof(zhdr); > > > gfp =3D __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM; > > > if (zpool_malloc_support_movable(entry->pool->zpool)) > > > gfp |=3D __GFP_HIGHMEM | __GFP_MOVABLE; > > > @@ -1313,6 +1388,12 @@ static int zswap_frontswap_store(unsigned type= , pgoff_t offset, > > > zswap_entry_put(tree, dupentry); > > > } > > > } while (ret =3D=3D -EEXIST); > > > + /* zpool_evictable will be removed once all 3 backends have m= igrated */ > > > + if (entry->length && !zpool_evictable(entry->pool->zpool)) { > > > + spin_lock(&entry->pool->lru_lock); > > > + list_add(&entry->lru, &entry->pool->lru); > > > + spin_unlock(&entry->pool->lru_lock); > > > + } > > > spin_unlock(&tree->lock); > > > > > > /* update stats */ > > > @@ -1384,8 +1465,7 @@ static int zswap_frontswap_load(unsigned type, = pgoff_t offset, > > > /* decompress */ > > > dlen =3D PAGE_SIZE; > > > src =3D zpool_map_handle(entry->pool->zpool, entry->handle, Z= POOL_MM_RO); > > > - if (zpool_evictable(entry->pool->zpool)) > > > - src +=3D sizeof(struct zswap_header); > > > + src +=3D sizeof(struct zswap_header); > > > > > > if (!zpool_can_sleep_mapped(entry->pool->zpool)) { > > > memcpy(tmp, src, entry->length); > > > @@ -1415,6 +1495,12 @@ static int zswap_frontswap_load(unsigned type,= pgoff_t offset, > > > freeentry: > > > spin_lock(&tree->lock); > > > zswap_entry_put(tree, entry); > > > + /* zpool_evictable will be removed once all 3 backends have m= igrated */ > > > + if (entry->length && !zpool_evictable(entry->pool->zpool)) { > > > + spin_lock(&entry->pool->lru_lock); > > > + list_move(&entry->lru, &entry->pool->lru); > > > + spin_unlock(&entry->pool->lru_lock); > > > + } > > > > It's not really this patch's fault, but when merged with commit > > fe1d1f7d0fb5 ("mm: zswap: support exclusive loads") from mm-unstable > > [1], and with CONFIG_ZSWAP_EXCLUSIVE_LOADS=3Dy, this causes a crash. > > > > This happens because fe1d1f7d0fb5 makes the loads exclusive, so > > zswap_entry_put(tree, entry) above the added code causes the entry to > > be freed, then we go ahead and deference multiple fields within it in > > the added chunk. Moving the chunk above zswap_entry_put() (and > > consequently also above zswap_invalidate_entry() from fe1d1f7d0fb5) > > makes this work correctly. > > > > Perhaps it would be useful to rebase on top of fe1d1f7d0fb5 for your > > next version(s), if any. > > Will definitely rebase, I just now saw that you tested the suggested reso= lution > below, thanks, it does make sense. > > > > > Maybe the outcome would be something like: > > > > zswap_entry_put(tree, entry); > > if (!ret && IS_ENABLED(CONFIG_ZSWAP_EXCLUSIVE_LOADS)) { > > zswap_invalidate_entry(tree, entry); > > } else if (entry->length && !zpool_evictable(entry->pool->zpool)) { > > spin_lock(&entry->pool->lru_lock); > > list_move(&entry->lru, &entry->pool->lru); > > spin_unlock(&entry->pool->lru_lock); > > } > > > > I am assuming if we are going to invalidate the entry anyway there is > > no need to move it to the front of the lru -- but I didn't really > > think it through. > > > > [1]https://lore.kernel.org/lkml/20230530210251.493194-1-yosryahmed@goog= le.com/ > > > > > spin_unlock(&tree->lock); > > > > > > return ret; > > > -- > > > 2.34.1 > > >