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 9F3D7CD11DD for ; Thu, 28 Mar 2024 18:49:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2AE806B008C; Thu, 28 Mar 2024 14:49:46 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 25F016B0092; Thu, 28 Mar 2024 14:49:46 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 128146B0093; Thu, 28 Mar 2024 14:49:46 -0400 (EDT) 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 E84666B008C for ; Thu, 28 Mar 2024 14:49:45 -0400 (EDT) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 78C6FA15D6 for ; Thu, 28 Mar 2024 18:49:45 +0000 (UTC) X-FDA: 81947336730.03.22113F1 Received: from mail-ej1-f45.google.com (mail-ej1-f45.google.com [209.85.218.45]) by imf05.hostedemail.com (Postfix) with ESMTP id A1AE910000D for ; Thu, 28 Mar 2024 18:49:43 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=NCHsCrf+; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf05.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.45 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=1711651783; 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=wdHBlrZAB65x1VfSv8cps7MnEoo8Xx6SN37qBYrgmeg=; b=WyoncWrBFOta2FIKyQIC9RjMYKAYo98Ld3pNG5PeSyJCfMy8jH1ZxCPX6sI4WjuSGEe8Ja Ol1GNBGfn4jwwyHBf5ZRNIGllnYbX2PZG/fwU0YVfbCuPzFi7HocuYCWW6cnvaJ2QlE4Zs bzuLql8B/yawC8K349QqtOWz/PKq1l8= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=NCHsCrf+; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf05.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.45 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1711651783; a=rsa-sha256; cv=none; b=dcV1VlyeWnRcCKKIWx23nKDYXDbV0OI5VIiFepgn5FQJI2WtcydNfvhA4X90ul6291aj/P 9ECwtNDspQ7zhqyRssmIf4x6+6un31OeTAAVvGaYrh0rePOvgbbBAcq0YFNS2r2Ag9zXHP yhoQ2ZvBhJSgKm1cvWZ8UiAQflHcEJk= Received: by mail-ej1-f45.google.com with SMTP id a640c23a62f3a-a4a393b699fso213631966b.0 for ; Thu, 28 Mar 2024 11:49:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1711651782; x=1712256582; 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=wdHBlrZAB65x1VfSv8cps7MnEoo8Xx6SN37qBYrgmeg=; b=NCHsCrf+Ehdj1VkMwCI9x8yROmSEbZR4HuQEWXzc98x/zHyJPY8VgKXCA08EErpS+j CkBb591XBgMARD4WtTRqYgCF77l9mBrrQInjABb0eSHltieFbUKNstMNxgKEjWqgInSF KFePbqyUFf2Os9lVtQHJghjMB0eAjJJ11IBJn1XJeUSgSWbClbOZnoRDkzIzlxP5sSvK 3gnu4TYoBnFok/yNeHX/QW7yjUEu8kwVcefBabIT88cvdtQg192qL6BD6IdKg6Qxk4/f TCgGzyVZvqRCtF1EQWgaoR6F0n9nxC3PFb00ii2z254mMqHHHHsuMfc4c1LCwl5H4GZ9 sr/w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1711651782; x=1712256582; 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=wdHBlrZAB65x1VfSv8cps7MnEoo8Xx6SN37qBYrgmeg=; b=wKJd2AnyIxCSkeFaro07sbCUM1zeFTnBTxJNzfciRUn3GFWDOYQ1BhNfZpB9SkCUe1 epBKb9vi7xVwD6uL9/KEjWjkrPdVFIP09ajkGHqZcASroAyc/XpJrywDx7LU1WUZDv2A ggM99CX/u9WNzLC3GRUukDxeZLxhq+l8oMTgoOsEkH+w6Lexi9jUYqGIzWeF28nVv8f1 hRT9nnx6wTBbCF3NSxlOYyw3JgGO3hTrfX5rn6FoHDTC+u+ixXt0CKzVmnUYyIgVlGRS T0NZax9o/dUIw/G8enqfbWq31ZXbC1eBYG569A09gBScqav8vtND5g6V5aQ1D/dI92hs CbBg== X-Forwarded-Encrypted: i=1; AJvYcCV26wDarYkGXcmisia0eMy+fNVqCrFB5vchj4A1oZAXhZ9GV1WXW8YHCK1btOlK35n9qV/saYszNMbQCqfOuGBRs3M= X-Gm-Message-State: AOJu0YxauhYH1QOysfhAGTNLvCkSUZFgjN3Y9u7dyvRxE75ZxTdIP3Y0 LIX1FhhZu4qwIBnj90WJ0VjM48m0byl31C6oba65n5BO089zBZRhXpSgsbYsOJ3VXaoSc4xzizm w/LDDV/qzeW5sM8zt+L1qI8RfVpvAtdMRp+S3 X-Google-Smtp-Source: AGHT+IHYmFh/INcEoVPMr6ixtdwubF7uOfhWzJkkfI4MhRm+6XvnlLhUU23rZ/elxyWop/TyT/OotjlPE2jyvPgEYE0= X-Received: by 2002:a17:906:484:b0:a47:2011:11c1 with SMTP id f4-20020a170906048400b00a47201111c1mr248066eja.8.1711651781809; Thu, 28 Mar 2024 11:49:41 -0700 (PDT) MIME-Version: 1.0 References: <20240325235018.2028408-1-yosryahmed@google.com> <20240325235018.2028408-10-yosryahmed@google.com> <6352794b-f5a2-4525-8185-c910739683e6@linux.dev> In-Reply-To: <6352794b-f5a2-4525-8185-c910739683e6@linux.dev> From: Yosry Ahmed Date: Thu, 28 Mar 2024 11:49:05 -0700 Message-ID: Subject: Re: [RFC PATCH 9/9] mm: zswap: use zswap_entry_free() for partially initialized entries To: Chengming Zhou Cc: Andrew Morton , Johannes Weiner , Nhat Pham , linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: gm3pdu3b8ej9x8ykfgbssjk6aod5phgb X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: A1AE910000D X-HE-Tag: 1711651783-373209 X-HE-Meta: U2FsdGVkX1/gGYshsuQWbx2t4enQk1BZ+Y8PoloV5jZpO7BaIrZzGWA6FqB930GN58AVlwkqeL0AW6HyGc8JbhkyY7lx9IbG8FhnDl+RukFPeTFMVrTM1K2Uic0lS5+pqx9zI52/JE0O+4BsDLCuW9b53KlR/oFYbJ1V723V/nT/tVOmwBXURO2xHSkdp/TXAqUx/wrow8O/+PIQIen6ZIVO+52SB0+FtXJV/wv/Y+R1K09yqWwMJdg4VxPJ4QWgFa1wPe33Bqp2s+X8ozD+wpGY1QtIzT+gp/85i2I+LA3+fs35IuFz7hm9SMnHhPNqiqJyeY/SBAOXltJDLMlVoCUwRrjKUuCcrV+L4yx6W5cbgjI1qVojnMeskNpcKoWHlPiUWeqtZed65tmKTUKobIBI90I/07kuTLSWb2G1q0Lce8Xjenc1MW0cPQbptqkm/nZQCniWIulJSVyEcIVcRnrlslhrJyjl/HdAjhJWbte+AruQGEGJNmQg6SzIAm32C2TAC2lukioa0xdMDxv9/JFUuuP/w72DXV+wVcpw+l3ZtUAVd7N3DI9hzVXJ2dpM2E3OsyMOKMl4gKdZTG3dCxCZEGEPc3hMBgySDDU5ECDp4YG1dlD1cJCZJGrV9KTDbogCmbZlWVyU2zAY2CfNuxlTNuTQMUMt73yn7iZA/T8Ma8rEUj8FKdvPh7TYwsPGDYfV3MUtE3e2NkJrOCi/lUTMGyL7WyiVwp/2NflutL3+4jZ7SLFg8cuahD2k2wqMJ/h1bGvYf4fhUpPUVY6nSA1zlfV6yvA5ZFEXx1YKd9oyWYAh8uMPB/m+iAB+h7PNM7GuiySBcwa3Q2Dqb8HXaM61OLjHwDbfbO3VtZzMQI4fb03HakS1KmXJormbJpUruAjZ9uQa/2NnZJqF7DWTM/e4x11YqEEDSskLtomPTDmCfDPTRzRN1UQbTvR+a/WPR65IMQIzOhSM6hDFyRf MSSjSzxV tlY2gY0aynzH/aMuu1LSauux2PNKirdvjqovV0WGhYmxp/PKap9jSWwdlFLYGi3BYgaKeYExYd0cysumu7iAaOuB4UVMrcrrd8LkwW/4B2ea5jlSKmjIinuHWoMwNJVkTog0Yfb/j6mgysgA3iCeS8a5aMAEWyEkPHe7stBlJuPxptlD3urc7avPddOcm4B0OpeEBXB8RuyCTVYUDlsShlho1MSCJ1eBwcwu6J0DTkqHruW7kzTvwZ4s8blKoXaFDyijz06wbVRUAjOURv1RmPIubXFTJmb75/IM6AmemLzrflM8= 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 Thu, Mar 28, 2024 at 1:31=E2=80=AFAM Chengming Zhou wrote: > > On 2024/3/26 07:50, Yosry Ahmed wrote: > > zswap_entry_free() performs four types of cleanups before freeing a > > zswap_entry: > > - Deletes the entry from the LRU. > > - Frees compressed memory. > > - Puts the pool reference. > > - Uncharges the compressed memory and puts the objcg. > > > > zswap_entry_free() always expects a fully initialized entry. Allow > > zswap_entry_free() to handle partially initialized entries by making it > > possible to identify what cleanups are needed as follows: > > - Allocate entries with __GFP_ZERO and initialize zswap_entry.lru when > > the entry is allocated. Points are NULL and length is zero upon > > initialization. > > - Use zswap_entry.length to identify if there is compressed memory to > > free. This is possible now that zero-filled pages are handled > > separately, so a length of zero means we did not successfully compres= s > > the page. > > - Only initialize entry->objcg after the memory is charged in > > zswap_store(). > > > > With this in place, use zswap_entry_free() in the failure path of > > zswap_store() to cleanup partially initialized entries. This simplifies > > the cleanup code in zswap_store(). While we are at it, rename the > > remaining cleanup labels to more meaningful names. > > Not sure if it's worthwhile to clean up the fail path with the normal pat= h > gets a little verbose. I was on the fence about this, so I thought I would just send it and see what others think. On one hand it makes the initialization more robust because the zswap_entry is always in a clean identifiable state, but on the other hand it adds churn to the normal path as you noticed. Also after removing handling zero-length entries from the failure path it isn't that bad without this patch anyway. So if no one else thinks this is useful, I will drop the patch in the next version. Thanks! > > > > > Signed-off-by: Yosry Ahmed > > --- > > mm/zswap.c | 62 ++++++++++++++++++++++++++---------------------------- > > 1 file changed, 30 insertions(+), 32 deletions(-) > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > index 9357328d940af..c50f9df230ca3 100644 > > --- a/mm/zswap.c > > +++ b/mm/zswap.c > > @@ -774,12 +774,13 @@ void zswap_memcg_offline_cleanup(struct mem_cgrou= p *memcg) > > **********************************/ > > static struct kmem_cache *zswap_entry_cache; > > > > -static struct zswap_entry *zswap_entry_cache_alloc(gfp_t gfp, int nid) > > +static struct zswap_entry *zswap_entry_cache_alloc(int nid) > > { > > struct zswap_entry *entry; > > - entry =3D kmem_cache_alloc_node(zswap_entry_cache, gfp, nid); > > - if (!entry) > > - return NULL; > > + entry =3D kmem_cache_alloc_node(zswap_entry_cache, > > + GFP_KERNEL | __GFP_ZERO, nid); > > + if (entry) > > + INIT_LIST_HEAD(&entry->lru); > > return entry; > > } > > > > @@ -795,9 +796,12 @@ static struct zpool *zswap_find_zpool(struct zswap= _entry *entry) > > > > static void zswap_entry_free(struct zswap_entry *entry) > > { > > - zswap_lru_del(&zswap_list_lru, entry); > > - zpool_free(zswap_find_zpool(entry), entry->handle); > > - zswap_pool_put(entry->pool); > > + if (!list_empty(&entry->lru)) > > + zswap_lru_del(&zswap_list_lru, entry); > > + if (entry->length) > > + zpool_free(zswap_find_zpool(entry), entry->handle); > > + if (entry->pool) > > + zswap_pool_put(entry->pool); > > if (entry->objcg) { > > obj_cgroup_uncharge_zswap(entry->objcg, entry->length); > > obj_cgroup_put(entry->objcg); > > @@ -1447,7 +1451,7 @@ bool zswap_store(struct folio *folio) > > return false; > > > > if (!zswap_enabled) > > - goto check_old; > > + goto erase_old; > > > > /* Check cgroup limits */ > > objcg =3D get_obj_cgroup_from_folio(folio); > > @@ -1455,54 +1459,52 @@ bool zswap_store(struct folio *folio) > > memcg =3D get_mem_cgroup_from_objcg(objcg); > > if (shrink_memcg(memcg)) { > > mem_cgroup_put(memcg); > > - goto reject; > > + goto put_objcg; > > } > > mem_cgroup_put(memcg); > > } > > > > if (zswap_is_folio_zero_filled(folio)) { > > if (zswap_store_zero_filled(tree, offset, objcg)) > > - goto reject; > > + goto put_objcg; > > goto stored; > > } > > > > if (!zswap_non_zero_filled_pages_enabled) > > - goto reject; > > + goto put_objcg; > > > > if (!zswap_check_limit()) > > - goto reject; > > + goto put_objcg; > > > > - entry =3D zswap_entry_cache_alloc(GFP_KERNEL, folio_nid(folio)); > > + entry =3D zswap_entry_cache_alloc(folio_nid(folio)); > > if (!entry) { > > zswap_reject_kmemcache_fail++; > > - goto reject; > > + goto put_objcg; > > } > > > > - /* if entry is successfully added, it keeps the reference */ > > entry->pool =3D zswap_pool_current_get(); > > if (!entry->pool) > > - goto freepage; > > + goto free_entry; > > > > if (objcg) { > > memcg =3D get_mem_cgroup_from_objcg(objcg); > > if (memcg_list_lru_alloc(memcg, &zswap_list_lru, GFP_KERN= EL)) { > > mem_cgroup_put(memcg); > > - goto put_pool; > > + goto free_entry; > > } > > mem_cgroup_put(memcg); > > } > > > > if (!zswap_compress(folio, entry)) > > - goto put_pool; > > - > > - entry->swpentry =3D swp; > > - entry->objcg =3D objcg; > > + goto free_entry; > > > > if (zswap_tree_store(tree, offset, entry)) > > - goto store_failed; > > + goto free_entry; > > > > - if (objcg) > > + if (objcg) { > > obj_cgroup_charge_zswap(objcg, entry->length); > > + entry->objcg =3D objcg; > > + } > > > > /* > > * We finish initializing the entry while it's already in xarray. > > @@ -1514,7 +1516,7 @@ bool zswap_store(struct folio *folio) > > * The publishing order matters to prevent writeback from seei= ng > > * an incoherent entry. > > */ > > - INIT_LIST_HEAD(&entry->lru); > > + entry->swpentry =3D swp; > > zswap_lru_add(&zswap_list_lru, entry); > > > > stored: > > @@ -1525,17 +1527,13 @@ bool zswap_store(struct folio *folio) > > > > return true; > > > > -store_failed: > > - zpool_free(zswap_find_zpool(entry), entry->handle); > > -put_pool: > > - zswap_pool_put(entry->pool); > > -freepage: > > - zswap_entry_cache_free(entry); > > -reject: > > +free_entry: > > + zswap_entry_free(entry); > > +put_objcg: > > obj_cgroup_put(objcg); > > if (zswap_pool_reached_full) > > queue_work(shrink_wq, &zswap_shrink_work); > > -check_old: > > +erase_old: > > /* > > * If the zswap store fails or zswap is disabled, we must invalid= ate the > > * possibly stale entry which was previously stored at this offse= t.