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 3F62EE69E8A for ; Mon, 2 Dec 2024 19:34:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CACF06B0085; Mon, 2 Dec 2024 14:34:31 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C5BB86B0088; Mon, 2 Dec 2024 14:34:31 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AFBCC6B0089; Mon, 2 Dec 2024 14:34:31 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 918CA6B0085 for ; Mon, 2 Dec 2024 14:34:31 -0500 (EST) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 1D50E16019C for ; Mon, 2 Dec 2024 19:34:31 +0000 (UTC) X-FDA: 82851020154.03.35C5B86 Received: from mail-oi1-f170.google.com (mail-oi1-f170.google.com [209.85.167.170]) by imf28.hostedemail.com (Postfix) with ESMTP id D90AAC001E for ; Mon, 2 Dec 2024 19:34:13 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=0YrZ0Lhx; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf28.hostedemail.com: domain of yosryahmed@google.com designates 209.85.167.170 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1733168063; a=rsa-sha256; cv=none; b=VwbhXDNQB80NfOv3rdJaDtEe0lppXNLJVOdcxFscroDo/wCu47ZJB4sUYgaoY1HSxSuPct 7548UzhsCotRDlDqbkSTqZ9jsJv8utToG+DaCjBPxIUXnKfVqLqT2FTWEigb51Mzf6M02A x/oQz1dGAI35p/Z/9lsnzayMjlWw5fA= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=0YrZ0Lhx; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf28.hostedemail.com: domain of yosryahmed@google.com designates 209.85.167.170 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=1733168063; 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=Uiyiz80ESC4vZ8IK2v5FDLx2PlcNaf/VwX618n4FUPE=; b=d5LgucQaqph3vK7wWbBp8IbPzAGvoGhCvayNEvgkOiiw8N/DNFl9dA8CTVfd8n0iTbSg7/ b5sJTtEGAHkpwWrWbXPfZs+JgseLE/A7DEMS3v7Eqcq/cFSqVxrUhaqmUlq1vEKuPqJJwi zzUo0A/CEo08zFZbbubMw2K+qNHFobI= Received: by mail-oi1-f170.google.com with SMTP id 5614622812f47-3e786167712so2189158b6e.3 for ; Mon, 02 Dec 2024 11:34:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1733168068; x=1733772868; 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=Uiyiz80ESC4vZ8IK2v5FDLx2PlcNaf/VwX618n4FUPE=; b=0YrZ0LhxEK1QpOu2uo1Gz1IejQfoygpmMc9miCj4Be9zNGn5kH/6NtjxG7azIgoX7I Z8LV1EsgX5dj6RgCINXoJkx1vG4UL7GQqbSMY6h8RIyf4OgPxQHb9I6NCRztyTnMf4PE Ce0msA3AkTePUJlJmypo+G/nK77YZNGxuJaVW2zJ4XKqey3MlrJHeaCeVa/kAc6nOLVk IMIoE0RP4E9aagppmkA0M197r5jT9oAe3znuEDy1imUpsdXztd+hj/XzuYVzteF6ebZg 993Ng6uv1mnbUpb/OCVOBSmkzhmJvVW0bJ/qxAEgyO1db9Rw1Gj2WmPT5RZ9gnuL1w3X 3b2A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1733168068; x=1733772868; 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=Uiyiz80ESC4vZ8IK2v5FDLx2PlcNaf/VwX618n4FUPE=; b=P5A6fzo12VfKR7x7U5AVgDP9zrsKnHSm0G7qCjoXOPQRF7AL+7T3jenfgQ9ZXRhWTt AkmTGVRM4PB/BvX9tRsX21R11UDUqVBBXSrPQ20zXzs1HNmVEMrpeMPbyrnX+XxcKu6u gYCCZYnt/XUYzzM47tTxByh5RCHU8Ee5FwnMHBwH/8trYP1kDitv3TI8oiuMtNYlE/hx 8StrrxEdsxNeYHzs4+V8imDXzKZfpy4xhHFWxOL/cy0K5M9U9vuFgZH0pXsgoEwcS2lQ UDPZqwI6nVIATfZUJabqQrHJDIhBXmML7j0tTYRzicDxnJDNYJ10698dnVuSCkHS4EL4 /xoA== X-Forwarded-Encrypted: i=1; AJvYcCX2hFRzSiUmXK8PDr5NnY94NRyJHjGDARr8qBre8vpASn+ae+aitW9DnDpnmX/EbAa8RdkmTLPTFQ==@kvack.org X-Gm-Message-State: AOJu0Yz+CjvVFJIUjTy6gI7OPoQuv3mFMKokOFZSM/tPNhtfQe7tqVZm M2JyCXRXzksBFHJjAnf8iAXHKWCTlWpnz0D6l+zsv0aJ9RnxgY8+Yk+IRL42N8rRwKpYTE1+VLk DiDMNNfBGzdu4LmaLInVy8BnukoHAsfkQUhRNzzB14s16Nh+owA== X-Gm-Gg: ASbGncv+yCfLn0D1vYsT6MDCYS4fouW+s8TntLk2APbEtbPSMFbcOekGaTFzsh1mM5I BXA5pUo4QU5eb7HQ2BcZLZfa9kMR+ X-Google-Smtp-Source: AGHT+IGGI8g8ewmbrMxXErj4uwLi6IbwzhzBJpnlRWbhdc+GYm9qHmPPswp49Euo4v1tqdwQHREP3F054TW2GM7Cmv4= X-Received: by 2002:a05:6808:1a28:b0:3ea:5413:1a24 with SMTP id 5614622812f47-3ea6dbcea00mr20710021b6e.13.1733168068046; Mon, 02 Dec 2024 11:34:28 -0800 (PST) MIME-Version: 1.0 References: <20241127225324.6770-1-kanchana.p.sridhar@intel.com> <20241127225324.6770-2-kanchana.p.sridhar@intel.com> In-Reply-To: <20241127225324.6770-2-kanchana.p.sridhar@intel.com> From: Yosry Ahmed Date: Mon, 2 Dec 2024 11:33:51 -0800 Message-ID: Subject: Re: [PATCH v1 1/2] mm: zswap: Modified zswap_store_page() to process multiple pages in a folio. To: Kanchana P Sridhar Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, hannes@cmpxchg.org, nphamcs@gmail.com, chengming.zhou@linux.dev, usamaarif642@gmail.com, ryan.roberts@arm.com, 21cnbao@gmail.com, akpm@linux-foundation.org, wajdi.k.feghali@intel.com, vinodh.gopal@intel.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: D90AAC001E X-Stat-Signature: 6oa88qsc6xq375z794j15wumha3aa41f X-HE-Tag: 1733168053-251659 X-HE-Meta: U2FsdGVkX18qWQCUGyIiqaLbcs179Yxmu3uXESJN3Q9lH/16cv5MfHYG5BDtjP3A28Y95WzWIH5zRd0vpI81nOGhEmsCivda9/JYKYNlZHr6bzM/Pf5IqdMZm07TNni0UYKu2aUME0HHC5Wx4UT2qO7+fa30sLbmg3lVwE2HWaSweVMVrq2183keUc8Rtnavzri0lhnJyNZnZsJc6AI5AQcarFfB9V8koRaTxku0syJlmUuB/4zvlR3awnIQ7prXRwzHovXAYXwCT9ZepilBSI/Hvy2QQH/z7hEN2XYmVUWh1DIqSOhC/ECdZkrGWyZspKX1Y6XeYHz9G6jk2RHGLdeizoK7bKrNrni6Mva1iN3j7UMwbr5vY3T/0uh9Usbt1Tp/gS3qlcnu69T8dUKbFWRtzeiM2LBl4wcR4BaJMv3W2w8Gj7/2yi4GXnWF+fQ776/ruBexpinxbsIkPZQgoQVg8+uxzDp6LYPklEaIX1CCnRbeVoUcLdRy3X92C0oZSAlrv2lvfltI4foCCXFd5g0WcwZLYalygimJ67jni9RU+cRoDh3FBNqM17Woc1jkFGXnkbQXqX6hjmaiC00sI1Iz5/uzi8no3FUByfpU2MRBKggABNHWNNQ9IVkkXRSkVzhBOqEMYrkH4vYotmZn04doXaBcVHMJw3Idg54R+Xsl6L9dEQadMpc8cY1j48kzgrtQLx16rwlvyVAwE9RbrsK47YEyq8pxQD6KRkYIN8APoILTfM4ww8ufVnz0htVJtikE6XDmfxbqMgfnmdZTmSr6oYX9lfnlJHu/d6P5K+9f5nHGeIu0Jjjinto43f6pcqMeacHVXFp1rtWYyJ/QGJFKo2PXffPeRE58U+6SCP2oPEXz5Z1kjMSnnZewhcYqxEkiI8uwcozFFtonMYIlpmNRhZwcNy6IhhNO3rWFpBrJNufNYB3nOIRE0KIbaX7u/BFVIq7WED3zkaZNDm1 pGE7pfH6 fyL/4feXXljxro6oGFsmWaBK86FPps+N8NRd14k0U6/tPvs0l3gXEcCXiwYo5ZIOmFQnHm2pV4lvvqUwoPNLECP7eJg1F0AgMjf7C5jU5IDqewe9gA5+z28YMMmsIsDfAl+5awCfxPLifKjGTVcMOP6bf8wsqa1tDMoIMmAqTc+3eSlbjbLVNK7BsI3nARZbcsRynJ6JoKlgU+FQBaqAB7+G+6+B5KFmaocQjc3td/AxtY1/11DDVclqmU75NUmSfiFgYTQTJwUuIq81hJhBtG0Pmf5BeGpINd5mv 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 Wed, Nov 27, 2024 at 2:53=E2=80=AFPM Kanchana P Sridhar wrote: > > Modified zswap_store() to store the folio in batches of > SWAP_CRYPTO_BATCH_SIZE pages. Accordingly, refactored zswap_store_page() > into zswap_store_pages() that processes a range of pages in the folio. > zswap_store_pages() is a vectorized version of zswap_store_page(). > > For now, zswap_store_pages() will sequentially compress these pages with > zswap_compress(). > > These changes are follow-up to code review comments received for [1], and > are intended to set up zswap_store() for batching with Intel IAA. > > [1]: https://patchwork.kernel.org/project/linux-mm/patch/20241123070127.3= 32773-11-kanchana.p.sridhar@intel.com/ > > Signed-off-by: Kanchana P Sridhar > --- > include/linux/zswap.h | 1 + > mm/zswap.c | 154 ++++++++++++++++++++++++------------------ > 2 files changed, 88 insertions(+), 67 deletions(-) > > diff --git a/include/linux/zswap.h b/include/linux/zswap.h > index d961ead91bf1..05a81e750744 100644 > --- a/include/linux/zswap.h > +++ b/include/linux/zswap.h > @@ -7,6 +7,7 @@ > > struct lruvec; > > +#define SWAP_CRYPTO_BATCH_SIZE 8UL > extern atomic_long_t zswap_stored_pages; > > #ifdef CONFIG_ZSWAP > diff --git a/mm/zswap.c b/mm/zswap.c > index f6316b66fb23..b09d1023e775 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -1409,78 +1409,96 @@ static void shrink_worker(struct work_struct *w) > * main API > **********************************/ > > -static ssize_t zswap_store_page(struct page *page, > - struct obj_cgroup *objcg, > - struct zswap_pool *pool) > +/* > + * Store multiple pages in @folio, starting from the page at index @si u= p to > + * and including the page at index @ei. > + */ > +static ssize_t zswap_store_pages(struct folio *folio, > + long si, > + long ei, > + struct obj_cgroup *objcg, > + struct zswap_pool *pool) > { > - swp_entry_t page_swpentry =3D page_swap_entry(page); > + struct page *page; > + swp_entry_t page_swpentry; > struct zswap_entry *entry, *old; > + size_t compressed_bytes =3D 0; > + u8 nr_pages =3D ei - si + 1; > + u8 i; > + > + for (i =3D 0; i < nr_pages; ++i) { > + page =3D folio_page(folio, si + i); > + page_swpentry =3D page_swap_entry(page); > + > + /* allocate entry */ > + entry =3D zswap_entry_cache_alloc(GFP_KERNEL, page_to_nid= (page)); > + if (!entry) { > + zswap_reject_kmemcache_fail++; > + return -EINVAL; > + } I think this patch is wrong on its own, for example if an allocation fails in the above loop we exit without cleaning up previous allocations. I think it's fixed in patch 2 but we cannot introduce bugs in-between patches. I think the helpers in patch 2 don't really help as I mentioned. Please combine the changes and keep them in the main series (unless you have a reason not to). > > - /* allocate entry */ > - entry =3D zswap_entry_cache_alloc(GFP_KERNEL, page_to_nid(page)); > - if (!entry) { > - zswap_reject_kmemcache_fail++; > - return -EINVAL; > - } > - > - if (!zswap_compress(page, entry, pool)) > - goto compress_failed; > + if (!zswap_compress(page, entry, pool)) > + goto compress_failed; > > - old =3D xa_store(swap_zswap_tree(page_swpentry), > - swp_offset(page_swpentry), > - entry, GFP_KERNEL); > - if (xa_is_err(old)) { > - int err =3D xa_err(old); > + old =3D xa_store(swap_zswap_tree(page_swpentry), > + swp_offset(page_swpentry), > + entry, GFP_KERNEL); > + if (xa_is_err(old)) { > + int err =3D xa_err(old); > > - WARN_ONCE(err !=3D -ENOMEM, "unexpected xarray error: %d\= n", err); > - zswap_reject_alloc_fail++; > - goto store_failed; > - } > + WARN_ONCE(err !=3D -ENOMEM, "unexpected xarray er= ror: %d\n", err); > + zswap_reject_alloc_fail++; > + goto store_failed; > + } > > - /* > - * We may have had an existing entry that became stale when > - * the folio was redirtied and now the new version is being > - * swapped out. Get rid of the old. > - */ > - if (old) > - zswap_entry_free(old); > + /* > + * We may have had an existing entry that became stale wh= en > + * the folio was redirtied and now the new version is bei= ng > + * swapped out. Get rid of the old. > + */ > + if (old) > + zswap_entry_free(old); > > - /* > - * The entry is successfully compressed and stored in the tree, t= here is > - * no further possibility of failure. Grab refs to the pool and o= bjcg. > - * These refs will be dropped by zswap_entry_free() when the entr= y is > - * removed from the tree. > - */ > - zswap_pool_get(pool); > - if (objcg) > - obj_cgroup_get(objcg); > + /* > + * The entry is successfully compressed and stored in the= tree, there is > + * no further possibility of failure. Grab refs to the po= ol and objcg. > + * These refs will be dropped by zswap_entry_free() when = the entry is > + * removed from the tree. > + */ > + zswap_pool_get(pool); > + if (objcg) > + obj_cgroup_get(objcg); > > - /* > - * We finish initializing the entry while it's already in xarray. > - * This is safe because: > - * > - * 1. Concurrent stores and invalidations are excluded by folio l= ock. > - * > - * 2. Writeback is excluded by the entry not being on the LRU yet= . > - * The publishing order matters to prevent writeback from seei= ng > - * an incoherent entry. > - */ > - entry->pool =3D pool; > - entry->swpentry =3D page_swpentry; > - entry->objcg =3D objcg; > - entry->referenced =3D true; > - if (entry->length) { > - INIT_LIST_HEAD(&entry->lru); > - zswap_lru_add(&zswap_list_lru, entry); > - } > + /* > + * We finish initializing the entry while it's already in= xarray. > + * This is safe because: > + * > + * 1. Concurrent stores and invalidations are excluded by= folio lock. > + * > + * 2. Writeback is excluded by the entry not being on the= LRU yet. > + * The publishing order matters to prevent writeback f= rom seeing > + * an incoherent entry. > + */ > + entry->pool =3D pool; > + entry->swpentry =3D page_swpentry; > + entry->objcg =3D objcg; > + entry->referenced =3D true; > + if (entry->length) { > + INIT_LIST_HEAD(&entry->lru); > + zswap_lru_add(&zswap_list_lru, entry); > + } > > - return entry->length; > + compressed_bytes +=3D entry->length; > + continue; > > store_failed: > - zpool_free(pool->zpool, entry->handle); > + zpool_free(pool->zpool, entry->handle); > compress_failed: > - zswap_entry_cache_free(entry); > - return -EINVAL; > + zswap_entry_cache_free(entry); > + return -EINVAL; > + } > + > + return compressed_bytes; > } > > bool zswap_store(struct folio *folio) > @@ -1492,7 +1510,7 @@ bool zswap_store(struct folio *folio) > struct zswap_pool *pool; > size_t compressed_bytes =3D 0; > bool ret =3D false; > - long index; > + long si, ei, incr =3D SWAP_CRYPTO_BATCH_SIZE; > > VM_WARN_ON_ONCE(!folio_test_locked(folio)); > VM_WARN_ON_ONCE(!folio_test_swapcache(folio)); > @@ -1526,11 +1544,13 @@ bool zswap_store(struct folio *folio) > mem_cgroup_put(memcg); > } > > - for (index =3D 0; index < nr_pages; ++index) { > - struct page *page =3D folio_page(folio, index); > + /* Store the folio in batches of SWAP_CRYPTO_BATCH_SIZE pages. */ > + for (si =3D 0, ei =3D min(si + incr - 1, nr_pages - 1); > + ((si < nr_pages) && (ei < nr_pages)); > + si =3D ei + 1, ei =3D min(si + incr - 1, nr_pages - 1)) { > ssize_t bytes; > > - bytes =3D zswap_store_page(page, objcg, pool); > + bytes =3D zswap_store_pages(folio, si, ei, objcg, pool); > if (bytes < 0) > goto put_pool; > compressed_bytes +=3D bytes; > @@ -1565,9 +1585,9 @@ bool zswap_store(struct folio *folio) > struct zswap_entry *entry; > struct xarray *tree; > > - for (index =3D 0; index < nr_pages; ++index) { > - tree =3D swap_zswap_tree(swp_entry(type, offset += index)); > - entry =3D xa_erase(tree, offset + index); > + for (si =3D 0; si < nr_pages; ++si) { > + tree =3D swap_zswap_tree(swp_entry(type, offset += si)); > + entry =3D xa_erase(tree, offset + si); > if (entry) > zswap_entry_free(entry); > } > -- > 2.27.0 >