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 43DEEC5478C for ; Sat, 2 Mar 2024 18:33:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7BF896B009F; Sat, 2 Mar 2024 13:33:31 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 76FBF6B00A0; Sat, 2 Mar 2024 13:33:31 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 611BE6B00A1; Sat, 2 Mar 2024 13:33:31 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 4F0436B009F for ; Sat, 2 Mar 2024 13:33:31 -0500 (EST) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 2BC4880563 for ; Sat, 2 Mar 2024 18:33:31 +0000 (UTC) X-FDA: 81852947022.14.3B73BD7 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf29.hostedemail.com (Postfix) with ESMTP id 1D20712000B for ; Sat, 2 Mar 2024 18:33:28 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=BCRaLnrS; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf29.hostedemail.com: domain of chrisl@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=chrisl@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1709404409; 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=cp7g0jUA5yCMOjgyW9EutfofHwYGSwLU6eNE0OnzIBI=; b=ODhNpBd2FejIiGOK/F6Y58voQ8tOZpV9pP9zw4If/xADT7wdOLuUQIPW62VCoInxv6AV6g 8Xj6M/LakNkLl7h0P+rqcgaVoLzhuEcVnwW3WSIqKOjQ/WpW9fRZBp2qwMbX7xKjNC/5eN 1QMtvgFFNfSMF3fipA3XnNO3lr9AODU= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=BCRaLnrS; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf29.hostedemail.com: domain of chrisl@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=chrisl@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1709404409; a=rsa-sha256; cv=none; b=R+wSlwXcRYp/UdTNQOBZlb1aS9jV16uaJXpGKQYDbzVlovRFJwhMfj1ZdUT9KWivbW4DBW uZ6+lM16YDl+LTL9ywwyvLAuFuB1eBoSRWo2UO16GrFcLfaIMcMNs1WOQ+qQPRhIxSKkOL P5LVbtdp8jyUUVHYiPIees4HZEaiBzs= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id DE358616BF for ; Sat, 2 Mar 2024 18:33:27 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0DB1EC43330 for ; Sat, 2 Mar 2024 18:33:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1709404407; bh=RTTjHmiboWXLQDiyXg9A2O5yKL5Ymun77agihIVZJms=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=BCRaLnrSX2IdQ88eggOfOTNU1SMHh+WRzKGwb+waCXOAFZiJwpfFLW+Ufp9cGAmmk 03cHDO6tHwjlBnBOry7rF70lJPLJn52bc1dphI3gEjDTsa/p0az20E8i/wvw3ABCey MPJIKpvRBTE9s3v0QnIG8bD2kuiyhERSptNeoOqXUfe4nvhGiEZE3VlLMOCu6L+nRp kyn21kNgJGMjmmxJBfRJ0rqwXLO2CHsIvLF4Q97t74k/S6fslQDOp9xIBWJLLJUf9M Qi3k1nH7Ed55R+SOJNpSI3/wKoImeCRObuPOEaMekQMSdCxqKQ+xMocZvaIbM81pUI q/RxTBbuk78ow== Received: by mail-io1-f44.google.com with SMTP id ca18e2360f4ac-7c845e17477so409139f.1 for ; Sat, 02 Mar 2024 10:33:26 -0800 (PST) X-Forwarded-Encrypted: i=1; AJvYcCUohVzoLgI+Q76yS+X4HspgL9gnKg3TIEXI5RJdHas5c0B8ekKouN8Wyp4BTRRVXeT5UniLh58zgbMnLlpxd49O5WY= X-Gm-Message-State: AOJu0YyHlsCrMMhppdTMMBzUu9D/Km6ECfKWORPZnX35k+MpCaPZwHBz JjUy4ROIcQxAiDL8FhuBfnDLOx/ofgcDvgh1UyKqA4NdrcRkQ32/3mu133bJ6DOhv8oGR70NugK id0d59LpSLzVQZrOjCZYEMXD1H27V6tB03BRb X-Google-Smtp-Source: AGHT+IEL93zfajRRBL76nsxI3/u5MZ3Z/Svvi3cxBLy2AOHdkYIWiFxKbGwZQ5aykquJIbVmhmclfDCYi1lB0FxTmI0= X-Received: by 2002:a05:6e02:1b08:b0:365:1dbc:a4f0 with SMTP id i8-20020a056e021b0800b003651dbca4f0mr4887316ilv.31.1709404406181; Sat, 02 Mar 2024 10:33:26 -0800 (PST) MIME-Version: 1.0 References: <20240229-zswap-xarray-v2-1-e50284dfcdb1@kernel.org> <66a86e12-e567-4705-b683-0485276007d2@bytedance.com> <72d6388f-119d-421e-a326-3fdc2bbc373e@bytedance.com> In-Reply-To: From: Chris Li Date: Sat, 2 Mar 2024 10:33:13 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2] zswap: replace RB tree with xarray To: Chengming Zhou Cc: Andrew Morton , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Yosry Ahmed , Nhat Pham , Johannes Weiner , "Matthew Wilcox (Oracle)" , Barry Song Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 1D20712000B X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: 3rzt8eq8s4gt6abu7hayx66aifug4o7x X-HE-Tag: 1709404408-104613 X-HE-Meta: U2FsdGVkX18ygpZ9nwJx2PNKCMphjeD0tFlcpjoUjxu2qsykatbyxzFgbIU15hQQ2VadDKEwoOPTSp9DvKAQ38HwKi0if9y+lLK3vt6XlHmqHMJPgfZ9w3fAQgku56Ws9tBfMurbyBD7SabphflUX5vG1kK0hcyZSL36WRPQ4eK8KxwuGg1DI7L88l5qiEEW27zaqPNHQgStLcWJmzJZd8bAzSEnLtaCOwFslWDQ4qYyIkBHJiAyeqbdpkQmYVr7BIpl7m2WT+mODo2DzpqZVXi56xkqeiSHd4BuzVPJuVZxLGqoyhl5PR1F1PiK3jC/Qhzih0yyZdCMqaS1wAyw+MPIdMRYaexxeo/+Sh1hXb/F1jISn9/5u26X8U67BVShYY/5h7QBV4Ap1PtDw9lIQUoA90CHrjvez0C37CNFrLHfVMYfcOBhBMWq6l0EtBm2zFZOF5/SW5TJNa2ikwz3r/hcIn41kL4nK6+NhB5jN4WfVTHR8e6JEsSXWLGCGXLIfpehNARRuWwIfU/McUPuknPGAxIzu+Ux3weRim+BZC6Dm3+g1oNQYvPpc3/udQJu5nmJfhJ90/J80F4n1zRCoZ/Ts+gR0I2mfIhG2b4RwNAls/6RKxwgtRgpPut4HawS9mOfrqczfis0ZPp+sglioRnbRcxg/E4XblyOyfRcAln2Rlrj9jH25cRK5BwJWrKfo8nfVwQTnbhVYVrZlzVgcY+1JPzV5/FYYTaG1SBaHvnzyFHRMFyZapG8TsAiAgnbCBtAbjcEv3ChA3jb2TFO8o5ij8NyZpwIvn8eXTJe18wCq/08CY7TsDgMvIiXHjE7kvxzhNRG599dVQiR0b0lIAIe2pn4HVnx5vGQec6BQ1E/sVifURxVztyYIuEn1HcgblPvPO50x+O4BdMFM96YGBv4SQfVumwMbhs8kqCKA0FaWBcTpyyIcYji0xN4Mq+uZCITh7Q0/GBFDuCpAJA 381FqCpC +o3zLLkxmp8cABu9Y7sywOCtu1F9sT6noz0ok0yzl+mZtW1SRUJpxyv+C1owdhYSI4zoSBwZ6tMFl2giu+PlMxQEhJVNyoefgeh4KCZWd+sCwRbLoQpVFPuxFqs7FQ6FwaAirS3i3i/goqD/zQ3RPBl4uN/OdUrUF+skqMrnDMgdltoXGl+freP7oMPZVOAqqUojkOvT1TZTCQ2o2r1SZ5JS6y6T4WYLoMM6tbjcDoNEHPiDg9X7s91hxAs++0MlR9jbbuUyeIX9bLyUK7lDs4b7bnE5FCDLwo/vGTmyaiVZz7tQCJQ+U2hQ7sqwaaraC34aXshwrZjX9YJtHI8I/cIAYFw== 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: Hi Chengming, On Fri, Mar 1, 2024 at 1:38=E2=80=AFAM Chris Li wrote: > > On Thu, Feb 29, 2024 at 11:24=E2=80=AFPM Chengming Zhou > wrote: > > > > On 2024/3/1 02:58, Chris Li wrote: > > > Hi Chengming, > > > > > > Thanks for the review and feedback. > > > > > > On Thu, Feb 29, 2024 at 1:44=E2=80=AFAM Chengming Zhou > > > wrote: > > >> > > >> Hi Chris, > > >> > > >> On 2024/2/29 16:46, Chris Li wrote: > > >>> Very deep RB tree requires rebalance at times. That > > >>> contributes to the zswap fault latencies. Xarray does not > > >>> need to perform tree rebalance. Replacing RB tree to xarray > > >>> can have some small performance gain. > > >>> > > >>> One small difference is that xarray insert might fail with > > >>> ENOMEM, while RB tree insert does not allocate additional > > >>> memory. > > >>> > > >>> The zswap_entry size will reduce a bit due to removing the > > >>> RB node, which has two pointers and a color field. Xarray > > >>> store the pointer in the xarray tree rather than the > > >>> zswap_entry. Every entry has one pointer from the xarray > > >>> tree. Overall, switching to xarray should save some memory, > > >>> if the swap entries are densely packed. > > >>> > > >>> Notice the zswap_rb_search and zswap_rb_insert always > > >>> followed by zswap_rb_erase. Fold the entry erase into > > >>> zswap_xa_search_and_erase and zswap_xa_insert. That saves > > >>> one tree lookup as well. > > >>> > > >>> Remove zswap_invalidate_entry due to no need to call > > >>> zswap_rb_erase any more. Use zswap_free_entry instead. > > >>> > > >>> The "struct zswap_tree" has been replaced by "struct xarray". > > >>> The tree spin lock has transferred to the xarray lock. > > >>> > > >>> Thanks to Chengming for providing the kernel build test number. > > >>> > > >>> Run the kernel build testing 5 times for each version, averages: > > >>> (memory.max=3D2GB, zswap shrinker and writeback enabled, one 50GB s= wapfile.) > > >>> > > >>> mm-266f922c0b5e zswap-xarray-te= st > > >>> real 63.43 63.12 > > >>> user 1063.78 1062.59 > > >>> sys 272.49 265.66 > > >>> > > >>> The sys time is about 2.5% faster. > > >>> > > >>> Tested-by: Chengming Zhou > > >>> --- > > >>> > > >>> > > >>> Signed-off-by: Chris Li > > >>> --- > > >>> Changes in v2: > > >>> - Replace struct zswap_tree with struct xarray. > > >>> - Remove zswap_tree spinlock, use xarray lock instead. > > >>> - Fold zswap_rb_erase() into zswap_xa_search_and_delete() and zswap= _xa_insert(). > > >>> - Delete zswap_invalidate_entry(), use zswap_free_entry() instead. > > >>> - Link to v1: https://lore.kernel.org/r/20240117-zswap-xarray-v1-0-= 6daa86c08fae@kernel.org > > >>> --- > > >>> mm/zswap.c | 173 +++++++++++++++++++++++--------------------------= ------------ > > >>> 1 file changed, 64 insertions(+), 109 deletions(-) > > >>> > > >>> diff --git a/mm/zswap.c b/mm/zswap.c > > >>> index 011e068eb355..ac9ef14d88be 100644 > > >>> --- a/mm/zswap.c > > >>> +++ b/mm/zswap.c > > >>> @@ -20,7 +20,6 @@ > > >>> #include > > >>> #include > > >>> #include > > >>> -#include > > >>> #include > > >>> #include > > >>> #include > > >>> @@ -71,6 +70,8 @@ static u64 zswap_reject_compress_poor; > > >>> static u64 zswap_reject_alloc_fail; > > >>> /* Store failed because the entry metadata could not be allocated = (rare) */ > > >>> static u64 zswap_reject_kmemcache_fail; > > >>> +/* Store failed because xarray can't insert the entry*/ > > >>> +static u64 zswap_reject_xarray_fail; > > >>> > > >>> /* Shrinker work queue */ > > >>> static struct workqueue_struct *shrink_wq; > > >>> @@ -196,7 +197,6 @@ static struct { > > >>> * This structure contains the metadata for tracking a single comp= ressed > > >>> * page within zswap. > > >>> * > > >>> - * rbnode - links the entry into red-black tree for the appropriat= e swap type > > >>> * swpentry - associated swap entry, the offset indexes into the r= ed-black tree > > >>> * length - the length in bytes of the compressed page data. Need= ed during > > >>> * decompression. For a same value filled page length is = 0, and both > > >>> @@ -208,7 +208,6 @@ static struct { > > >>> * lru - handle to the pool's lru used to evict pages. > > >>> */ > > >>> struct zswap_entry { > > >>> - struct rb_node rbnode; > > >>> swp_entry_t swpentry; > > >>> unsigned int length; > > >>> struct zswap_pool *pool; > > >>> @@ -220,12 +219,7 @@ struct zswap_entry { > > >>> struct list_head lru; > > >>> }; > > >>> > > >>> -struct zswap_tree { > > >>> - struct rb_root rbroot; > > >>> - spinlock_t lock; > > >>> -}; > > >>> - > > >>> -static struct zswap_tree *zswap_trees[MAX_SWAPFILES]; > > >>> +static struct xarray *zswap_trees[MAX_SWAPFILES]; > > >>> static unsigned int nr_zswap_trees[MAX_SWAPFILES]; > > >>> > > >>> /* RCU-protected iteration */ > > >>> @@ -253,10 +247,10 @@ static bool zswap_has_pool; > > >>> * helpers and fwd declarations > > >>> **********************************/ > > >>> > > >>> -static inline struct zswap_tree *swap_zswap_tree(swp_entry_t swp) > > >>> +static inline struct xarray *swap_zswap_tree(swp_entry_t swp) > > >>> { > > >>> - return &zswap_trees[swp_type(swp)][swp_offset(swp) > > >>> - >> SWAP_ADDRESS_SPACE_SHIFT]; > > >>> + return zswap_trees[swp_type(swp)] + (swp_offset(swp) > > >>> + >> SWAP_ADDRESS_SPACE_SHIFT); > > >>> } > > >>> > > >>> #define zswap_pool_debug(msg, p) \ > > >>> @@ -805,60 +799,38 @@ void zswap_memcg_offline_cleanup(struct mem_c= group *memcg) > > >>> } > > >>> > > >>> /********************************* > > >>> -* rbtree functions > > >>> +* xarray functions > > >>> **********************************/ > > >>> -static struct zswap_entry *zswap_rb_search(struct rb_root *root, p= goff_t offset) > > >>> +static struct zswap_entry *zswap_xa_search_and_erase(struct xarray= *tree, pgoff_t offset) > > >>> { > > >>> - struct rb_node *node =3D root->rb_node; > > >>> - struct zswap_entry *entry; > > >>> - pgoff_t entry_offset; > > >>> - > > >>> - while (node) { > > >>> - entry =3D rb_entry(node, struct zswap_entry, rbnode); > > >>> - entry_offset =3D swp_offset(entry->swpentry); > > >>> - if (entry_offset > offset) > > >>> - node =3D node->rb_left; > > >>> - else if (entry_offset < offset) > > >>> - node =3D node->rb_right; > > >>> - else > > >>> - return entry; > > >>> - } > > >>> - return NULL; > > >>> + return xa_erase(tree, offset); > > >>> } > > >>> > > >>> /* > > >>> + * Expects xa_lock to be held on entry. > > >>> * In the case that a entry with the same offset is found, a point= er to > > >>> - * the existing entry is stored in dupentry and the function retur= ns -EEXIST > > >>> + * the existing entry is stored in old and erased from the tree. > > >>> + * Function return error on insert. > > >>> */ > > >>> -static int zswap_rb_insert(struct rb_root *root, struct zswap_entr= y *entry, > > >>> - struct zswap_entry **dupentry) > > >>> +static int zswap_xa_insert(struct xarray *tree, struct zswap_entry= *entry, > > >>> + struct zswap_entry **old) > > >>> { > > >>> - struct rb_node **link =3D &root->rb_node, *parent =3D NULL; > > >>> - struct zswap_entry *myentry; > > >>> - pgoff_t myentry_offset, entry_offset =3D swp_offset(entry->sw= pentry); > > >>> - > > >>> - while (*link) { > > >>> - parent =3D *link; > > >>> - myentry =3D rb_entry(parent, struct zswap_entry, rbno= de); > > >>> - myentry_offset =3D swp_offset(myentry->swpentry); > > >>> - if (myentry_offset > entry_offset) > > >>> - link =3D &(*link)->rb_left; > > >>> - else if (myentry_offset < entry_offset) > > >>> - link =3D &(*link)->rb_right; > > >>> - else { > > >>> - *dupentry =3D myentry; > > >>> - return -EEXIST; > > >>> - } > > >>> - } > > >>> - rb_link_node(&entry->rbnode, parent, link); > > >>> - rb_insert_color(&entry->rbnode, root); > > >>> - return 0; > > >>> -} > > >>> + int err; > > >>> + struct zswap_entry *e; > > >>> + pgoff_t offset =3D swp_offset(entry->swpentry); > > >>> > > >>> -static void zswap_rb_erase(struct rb_root *root, struct zswap_entr= y *entry) > > >>> -{ > > >>> - rb_erase(&entry->rbnode, root); > > >>> - RB_CLEAR_NODE(&entry->rbnode); > > >>> + e =3D __xa_store(tree, offset, entry, GFP_KERNEL); > > >>> + err =3D xa_err(e); > > >>> + > > >>> + if (err) { > > >>> + e =3D __xa_erase(tree, offset); > > > > > > zswap_xa_insert will always erase the old entry, even when __xa_store= fails. > > > > > >>> + if (err =3D=3D -ENOMEM) > > >>> + zswap_reject_alloc_fail++; > > >>> + else > > >>> + zswap_reject_xarray_fail++; > > >>> + } > > >>> + *old =3D e; > > > > > > Old pointer is set regardless of the error. > > > > Ok, I get it. The "old" pointer is always set on return. > > > > > > > >>> + return err; > > >>> } > > >>> > > >>> /********************************* > > >>> @@ -872,7 +844,6 @@ static struct zswap_entry *zswap_entry_cache_al= loc(gfp_t gfp, int nid) > > >>> entry =3D kmem_cache_alloc_node(zswap_entry_cache, gfp, nid); > > >>> if (!entry) > > >>> return NULL; > > >>> - RB_CLEAR_NODE(&entry->rbnode); > > >>> return entry; > > >>> } > > >>> > > >>> @@ -914,17 +885,6 @@ static void zswap_entry_free(struct zswap_entr= y *entry) > > >>> zswap_update_total_size(); > > >>> } > > >>> > > >>> -/* > > >>> - * The caller hold the tree lock and search the entry from the tre= e, > > >>> - * so it must be on the tree, remove it from the tree and free it. > > >>> - */ > > >>> -static void zswap_invalidate_entry(struct zswap_tree *tree, > > >>> - struct zswap_entry *entry) > > >>> -{ > > >>> - zswap_rb_erase(&tree->rbroot, entry); > > >>> - zswap_entry_free(entry); > > >>> -} > > >>> - > > >>> /********************************* > > >>> * compressed storage functions > > >>> **********************************/ > > >>> @@ -1113,7 +1073,9 @@ static void zswap_decompress(struct zswap_ent= ry *entry, struct page *page) > > >>> static int zswap_writeback_entry(struct zswap_entry *entry, > > >>> swp_entry_t swpentry) > > >>> { > > >>> - struct zswap_tree *tree; > > >>> + struct xarray *tree; > > >>> + pgoff_t offset =3D swp_offset(swpentry); > > >>> + struct zswap_entry *e; > > >>> struct folio *folio; > > >>> struct mempolicy *mpol; > > >>> bool folio_was_allocated; > > >>> @@ -1150,19 +1112,14 @@ static int zswap_writeback_entry(struct zsw= ap_entry *entry, > > >>> * be dereferenced. > > >>> */ > > >>> tree =3D swap_zswap_tree(swpentry); > > >>> - spin_lock(&tree->lock); > > >>> - if (zswap_rb_search(&tree->rbroot, swp_offset(swpentry)) !=3D= entry) { > > >>> - spin_unlock(&tree->lock); > > >>> + e =3D zswap_xa_search_and_erase(tree, offset); > > >>> + if (e !=3D entry) { > > >> > > >> IIUC, here we should use xa_cmpxchg() instead of erasing it uncondit= ionally. > > > > > > Good catch, I agree with your suggestion. I will spin a V3 to correct= that. > > > > > >> > > >>> delete_from_swap_cache(folio); > > >>> folio_unlock(folio); > > >>> folio_put(folio); > > >>> return -ENOMEM; > > >>> } > > >>> > > >>> - /* Safe to deref entry after the entry is verified above. */ > > >>> - zswap_rb_erase(&tree->rbroot, entry); > > >>> - spin_unlock(&tree->lock); > > >>> - > > >>> zswap_decompress(entry, &folio->page); > > >>> > > >>> count_vm_event(ZSWPWB); > > >>> @@ -1471,10 +1428,11 @@ bool zswap_store(struct folio *folio) > > >>> { > > >>> swp_entry_t swp =3D folio->swap; > > >>> pgoff_t offset =3D swp_offset(swp); > > >>> - struct zswap_tree *tree =3D swap_zswap_tree(swp); > > >>> - struct zswap_entry *entry, *dupentry; > > >>> + struct xarray *tree =3D swap_zswap_tree(swp); > > >>> + struct zswap_entry *entry, *old; > > >>> struct obj_cgroup *objcg =3D NULL; > > >>> struct mem_cgroup *memcg =3D NULL; > > >>> + int err; > > >>> > > >>> VM_WARN_ON_ONCE(!folio_test_locked(folio)); > > >>> VM_WARN_ON_ONCE(!folio_test_swapcache(folio)); > > >>> @@ -1562,21 +1520,25 @@ bool zswap_store(struct folio *folio) > > >>> } > > >>> > > >>> /* map */ > > >>> - spin_lock(&tree->lock); > > >>> + xa_lock(tree); > > >>> /* > > >>> * The folio may have been dirtied again, invalidate the > > >>> * possibly stale entry before inserting the new entry. > > >>> */ > > >>> - if (zswap_rb_insert(&tree->rbroot, entry, &dupentry) =3D=3D -= EEXIST) { > > >>> - zswap_invalidate_entry(tree, dupentry); > > >>> - WARN_ON(zswap_rb_insert(&tree->rbroot, entry, &dupent= ry)); > > >>> + err =3D zswap_xa_insert(tree, entry, &old); > > >>> + if (old) > > >>> + zswap_entry_free(old); > > >> > > >> Maybe it's safer to check old after !err, since "old" variable is no= t initialized > > >> to NULL, and zswap_xa_insert() maybe won't overwrite "old" to NULL w= hen err return? > > > > > > That is the intended behavior. > > > > > > See the above in zswap_xa_insert(). It will always erase and return > > > "old" even when the __xa_store() has an error. > > > That is because by the time zswap needs to store a new entry at this > > > swap entry. The old data is already outdated. We should just remove > > > the old data. If __xa_store failed due to out of memory. That is the > > > same as allocating an entry out of memory. It is fine to fail > > > swap_store. Then the folio will just stay in the swap cache for the > > > next time. > > > > > > Do you see any ill effects can be caused by deleting the old entry on > > > xa_insert error? > > > > No, you're right, we should always delete/free old zswap entry no matte= r > > store success or fail. > > > > > > > >>> + if (err) { > > >>> + xa_unlock(tree); > > >>> + goto free_zpool; > > >>> } > > >>> + > > >>> if (entry->length) { > > >>> INIT_LIST_HEAD(&entry->lru); > > >>> zswap_lru_add(&zswap.list_lru, entry); > > >>> atomic_inc(&zswap.nr_stored); > > >>> } > > >> > > >> It seems that we can put this part out of the xarray lock section, t= hen it's enough to > > >> just use xa_insert(). > > > > I wanted to mean xa_store() here. > > > > > > > > It is not enough protection. Consider this race: > > > > > > CPU1 CPU2 > > > > > > xa_insert() > > > entry =3D swap_xa_search_and_erase= () > > > zswap_free_entry(entry) > > > > > > if (entry->length) > > > ... > > > CPU1 is using entry after free. > > > > Hmm, right, but I don't know how could this race happen? Since the foli= o we store is > > the owner of swap entry, which couldn't be deleted meanwhile, right? > > I will need to think about it more. Agree the current folio can't > delete itself. It is possible the folio lock was enough to prevent the > race. After sleeping on it a bit, I think it should be safe to reduce the unlock range and use xa_store directly. Does anyone have any other objects? If not, I will use xa_store directly fo= r V3. Chris