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 492D6C5475B for ; Fri, 1 Mar 2024 07:24:31 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 647BE6B007E; Fri, 1 Mar 2024 02:24:30 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 5F8A16B0080; Fri, 1 Mar 2024 02:24:30 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 498E16B0081; Fri, 1 Mar 2024 02:24:30 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 390716B007E for ; Fri, 1 Mar 2024 02:24:30 -0500 (EST) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id DD16DA12BD for ; Fri, 1 Mar 2024 07:24:29 +0000 (UTC) X-FDA: 81847632258.11.FF1DBB3 Received: from mail-pl1-f181.google.com (mail-pl1-f181.google.com [209.85.214.181]) by imf15.hostedemail.com (Postfix) with ESMTP id 351A3A0006 for ; Fri, 1 Mar 2024 07:24:26 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=bytedance.com header.s=google header.b=kFrTY+Jq; spf=pass (imf15.hostedemail.com: domain of zhouchengming@bytedance.com designates 209.85.214.181 as permitted sender) smtp.mailfrom=zhouchengming@bytedance.com; dmarc=pass (policy=quarantine) header.from=bytedance.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1709277868; 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=NOrmy6acJP+0Y09rod9AHEhXLlkCs9TJsVIoLyciKIo=; b=647dg+fxPHWQ0mIDOhdu1jy5kSmVqZSapaQnbiOefQSODj1jyNyynkToDCJt9AxMpooZb8 i7gj8BeDM3fDax+PktrNbti2sbKAMvamH0PgGQz2RgzuRp3vWJtrGVaK8oQEkRpk2t+Lxe ZdTiNUWoHcYXiaRll6k9vY1yAU7hWfU= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=bytedance.com header.s=google header.b=kFrTY+Jq; spf=pass (imf15.hostedemail.com: domain of zhouchengming@bytedance.com designates 209.85.214.181 as permitted sender) smtp.mailfrom=zhouchengming@bytedance.com; dmarc=pass (policy=quarantine) header.from=bytedance.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1709277868; a=rsa-sha256; cv=none; b=R7/lZk7EWGosMYMeI6kFLFqkKV0j4xzizBLAC3/2wBUAT58zxyvjd8LP4gVrDFQx05bo/7 YDZeJkzuRe6AmLDbWinTdOKkccX7uENVVw2S40LE0knz/+e3g5y1OU0y5eDtZcmYML6K/9 lmfNEPKENgZzLKctmfryc77TbLLwoOs= Received: by mail-pl1-f181.google.com with SMTP id d9443c01a7336-1dc139ed11fso24331935ad.0 for ; Thu, 29 Feb 2024 23:24:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance.com; s=google; t=1709277866; x=1709882666; darn=kvack.org; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=NOrmy6acJP+0Y09rod9AHEhXLlkCs9TJsVIoLyciKIo=; b=kFrTY+JqjEzySTmgR+8QDQwDsY6ZDCkB3m2VFmaG/jpyA5YT4lTU0nyFvZ3PLaQZVm Z5r2Uej/iOd1Ow14a1L5LhZhjz9ErVpQppFxpaB9ZsMuJVqOpFEM7w/xeKA+o7N21Z9D FiKqLuEOaR8FbX1ARQumg/buEggyplNVnrLYaUm2gFe6v2uJ5vf+H772z/SO16RxL5mK 1l35J6ozObQtqWE1BJkeX9hH1Fil6CeYkvzLQWzfr6xVJt92yONIXa3dRRYfI/tJlNcq kss4O1SevXXwOuxbNLRorT/X041AEDdj89WjZayC9W6Yo1Y83GEaEE+kDw3TEItNV0Xl 5WQA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709277866; x=1709882666; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=NOrmy6acJP+0Y09rod9AHEhXLlkCs9TJsVIoLyciKIo=; b=Frlc1jQq6HlYiV39nwajCtqNHUDEJvpBawe9UPIUcciStD6yQPpUPKUXWA53n2jAJe JMmIZVIKppi2nnU8oofIHs8AHD7LAklFSsCUkfOOBYWgaOcmH68stPB6LosW47samIXx qh0J1ke7tq5u++Q550Leo8y0TPT9YgSVaOkznWyQOk76ywe+SGBKx6yEu4d48P3mBCCA +MS/jg6JdiG8Avgh8DCh6/et0VTmU6bRo6IuinnKn7xpQme6S4ta7jC7klfytl86tWTC ZflUQxZjRJUZHjwbf15PiTtNX7mttbNUnoNQCCo7zAYQyCJXjwgKv1lUcGmI1pNGUnyD aVpQ== X-Forwarded-Encrypted: i=1; AJvYcCWQtH69/fLRhZYg+hzbCxYp/AFTkqDWQl8xt4in3qvtxcH5EfQerrRDdKWwtC9x9iTjXyJd8N6f1AOw8CFT/OYEmfQ= X-Gm-Message-State: AOJu0YxpAKP1mG28j7BAyTxsaBXMr5KjFPbhMS16d4CsZ6Y+WvVmwW0d wtiG8qKfkOrssBBHDXhi4Pun8eERfnq779PzRkFdUidpnwBRVv7JyqCdEp1i2Z4= X-Google-Smtp-Source: AGHT+IHMct0SYdbWMVvgj/4+cizcyC9zhwqaW6P2v7AVpzJkP/7c8eDn41n4idBPsGx8Kvx9xdrJLw== X-Received: by 2002:a17:902:db0a:b0:1dc:e481:8a4 with SMTP id m10-20020a170902db0a00b001dce48108a4mr1312558plx.26.1709277865717; Thu, 29 Feb 2024 23:24:25 -0800 (PST) Received: from [10.254.45.144] ([139.177.225.229]) by smtp.gmail.com with ESMTPSA id c24-20020a170902b69800b001db93340f9bsm2681972pls.205.2024.02.29.23.24.21 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 29 Feb 2024 23:24:25 -0800 (PST) Message-ID: <72d6388f-119d-421e-a326-3fdc2bbc373e@bytedance.com> Date: Fri, 1 Mar 2024 15:24:19 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] zswap: replace RB tree with xarray Content-Language: en-US To: Chris Li Cc: Andrew Morton , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Yosry Ahmed , Nhat Pham , Johannes Weiner , "Matthew Wilcox (Oracle)" , Barry Song References: <20240229-zswap-xarray-v2-1-e50284dfcdb1@kernel.org> <66a86e12-e567-4705-b683-0485276007d2@bytedance.com> From: Chengming Zhou In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 351A3A0006 X-Rspam-User: X-Stat-Signature: jema6q3h71e1usb5h6jaajtwwhi7rsuz X-Rspamd-Server: rspam01 X-HE-Tag: 1709277866-579944 X-HE-Meta: U2FsdGVkX1/zwj/PJ7N79afJxcoFnWQbk2v9v3EcIBi1/uM5F4/sJV6MkXlpW5gHG403/GW6aWpXFx7ulJeEUztdvoxver1OMUnkeXNgvUeikqeNsukJR5yVwSecayVZUpMxx8DS0EHNsnfWsGIvxU5dTh4GPopX9fApJrTe0qEX+9zgIgIJJTkXtBI7txsiJPaLcw7dxz1q/R1BN4IKGw7SAVNQFHsnv8lCVYKqc75ZPdadd/kib5VzwJ+4Oj3ECbJGeo07ojDlyavdTS8/uqBX+t766DrN6N7d2xi0izQmIPRO6sGQnvZ6IOK5XwmwlKF/IM2HWxbfYPWDPTmJsDfIFBnPXd9VwEJhFvbwtnZK/k52GttIAgAu/73S9UBrnyasbnRykS8G+mhfc6/ir7EjyVtolRW7qsls4ghPcmgpTP9zsbx3mqNIYlJju1x9OY1p7x+fDJBKlwDb5lgzABmbGffKl4nSiBdVnY8oQTN5EbIH2QnripL8IrJct78uqz3fzy7B5YgnUGUkZIOx98boHaQhcL5B70Znvkuj/XedxkTib16YkVaR3L7CQbHF9IBt4WbGsIE0FRsbUDFVDYPSPGUyZulTYNZpM2Iziwi4rttrnoFg6RbTgqHTXzSUYwr3Xbot3T0AXU1+8OjwRZpHdJrGR8M2eTISdmgcro0gGKpdgG80IVdcYi6S6lHKxEsHDzD6Il/tXfqIw2hcdl7NON2QrRpbq7LUNAWEjfnHzevVsCDn/SZqiMww7CxZLHuc/S3J/r0GWI+B3Lcdhqim8R6ufC6X1SVTTioy/UxZ8B50nvpGQWNnPe03/GYQ2hfewJzlTqWJvEPClXTbAivses+KgeP/eVHX0c+vZef9io+FSJLldzWx4wfHbG07bhIBUvpj/03ts6MR5sBE46pynLtrm70fwi34YAR8qy91l1zhC5gXhIWLEBn4IcFZ5G6gE4CCpqCoMtk31lt cD7iBoG8 ytihDkS07/x1VMJPesXkYiDoTe0GhKj04v/B1oHKJ0gKpE5mZIwiMGajvOZhLEu1oaWIavjaGzOEvcJUEMsr/FYztrylpe8ebynKxNmlaTqf2m0D4PjAfKKojKOUL1f5Jou5t 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 2024/3/1 02:58, Chris Li wrote: > Hi Chengming, > > Thanks for the review and feedback. > > On Thu, Feb 29, 2024 at 1:44 AM 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=2GB, zswap shrinker and writeback enabled, one 50GB swapfile.) >>> >>> mm-266f922c0b5e zswap-xarray-test >>> 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 compressed >>> * page within zswap. >>> * >>> - * rbnode - links the entry into red-black tree for the appropriate swap type >>> * swpentry - associated swap entry, the offset indexes into the red-black tree >>> * length - the length in bytes of the compressed page data. Needed 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_cgroup *memcg) >>> } >>> >>> /********************************* >>> -* rbtree functions >>> +* xarray functions >>> **********************************/ >>> -static struct zswap_entry *zswap_rb_search(struct rb_root *root, pgoff_t offset) >>> +static struct zswap_entry *zswap_xa_search_and_erase(struct xarray *tree, pgoff_t offset) >>> { >>> - struct rb_node *node = root->rb_node; >>> - struct zswap_entry *entry; >>> - pgoff_t entry_offset; >>> - >>> - while (node) { >>> - entry = rb_entry(node, struct zswap_entry, rbnode); >>> - entry_offset = swp_offset(entry->swpentry); >>> - if (entry_offset > offset) >>> - node = node->rb_left; >>> - else if (entry_offset < offset) >>> - node = 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 pointer to >>> - * the existing entry is stored in dupentry and the function returns -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_entry *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 = &root->rb_node, *parent = NULL; >>> - struct zswap_entry *myentry; >>> - pgoff_t myentry_offset, entry_offset = swp_offset(entry->swpentry); >>> - >>> - while (*link) { >>> - parent = *link; >>> - myentry = rb_entry(parent, struct zswap_entry, rbnode); >>> - myentry_offset = swp_offset(myentry->swpentry); >>> - if (myentry_offset > entry_offset) >>> - link = &(*link)->rb_left; >>> - else if (myentry_offset < entry_offset) >>> - link = &(*link)->rb_right; >>> - else { >>> - *dupentry = 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 = swp_offset(entry->swpentry); >>> >>> -static void zswap_rb_erase(struct rb_root *root, struct zswap_entry *entry) >>> -{ >>> - rb_erase(&entry->rbnode, root); >>> - RB_CLEAR_NODE(&entry->rbnode); >>> + e = __xa_store(tree, offset, entry, GFP_KERNEL); >>> + err = xa_err(e); >>> + >>> + if (err) { >>> + e = __xa_erase(tree, offset); > > zswap_xa_insert will always erase the old entry, even when __xa_store fails. > >>> + if (err == -ENOMEM) >>> + zswap_reject_alloc_fail++; >>> + else >>> + zswap_reject_xarray_fail++; >>> + } >>> + *old = 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_alloc(gfp_t gfp, int nid) >>> entry = 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_entry *entry) >>> zswap_update_total_size(); >>> } >>> >>> -/* >>> - * The caller hold the tree lock and search the entry from the tree, >>> - * 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_entry *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 = 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 zswap_entry *entry, >>> * be dereferenced. >>> */ >>> tree = swap_zswap_tree(swpentry); >>> - spin_lock(&tree->lock); >>> - if (zswap_rb_search(&tree->rbroot, swp_offset(swpentry)) != entry) { >>> - spin_unlock(&tree->lock); >>> + e = zswap_xa_search_and_erase(tree, offset); >>> + if (e != entry) { >> >> IIUC, here we should use xa_cmpxchg() instead of erasing it unconditionally. > > 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 = folio->swap; >>> pgoff_t offset = swp_offset(swp); >>> - struct zswap_tree *tree = swap_zswap_tree(swp); >>> - struct zswap_entry *entry, *dupentry; >>> + struct xarray *tree = swap_zswap_tree(swp); >>> + struct zswap_entry *entry, *old; >>> struct obj_cgroup *objcg = NULL; >>> struct mem_cgroup *memcg = 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) == -EEXIST) { >>> - zswap_invalidate_entry(tree, dupentry); >>> - WARN_ON(zswap_rb_insert(&tree->rbroot, entry, &dupentry)); >>> + err = 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 not initialized >> to NULL, and zswap_xa_insert() maybe won't overwrite "old" to NULL when 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 matter 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, then 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 = 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 folio we store is the owner of swap entry, which couldn't be deleted meanwhile, right? Another problem I just notice is that if xa_store() failed, zswap_same_filled_pages won't be correct. (Maybe we should move zswap_same_filled_pages inc) Thanks.