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 00EB7C2BD09 for ; Thu, 27 Jun 2024 16:19:02 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6371C6B0088; Thu, 27 Jun 2024 12:19:02 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5E7076B0089; Thu, 27 Jun 2024 12:19:02 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 487326B008A; Thu, 27 Jun 2024 12:19:02 -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 2D55E6B0088 for ; Thu, 27 Jun 2024 12:19:02 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 88EED140BCD for ; Thu, 27 Jun 2024 16:19:01 +0000 (UTC) X-FDA: 82277177682.27.07BCE15 Received: from mail-qk1-f175.google.com (mail-qk1-f175.google.com [209.85.222.175]) by imf09.hostedemail.com (Postfix) with ESMTP id 3957314001F for ; Thu, 27 Jun 2024 16:18:58 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=cmpxchg-org.20230601.gappssmtp.com header.s=20230601 header.b="OAU35y/u"; dmarc=pass (policy=none) header.from=cmpxchg.org; spf=pass (imf09.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.222.175 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1719505126; a=rsa-sha256; cv=none; b=lXS/hG5drv7oo5BT18xoHqNjpRfDiufYbr5rgSS356LgOLUXdsB7s0KbJFLe4Z2ypUwQ7u k6D8iRasItAiZZuI0FyKyhGFTZRxp33FgXGVu10l8VFXt5vkK34ZjujcYC24u1w+aXgNGk w/BvQlmDRPyav93AwIp31ek8Uoqd4y4= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=cmpxchg-org.20230601.gappssmtp.com header.s=20230601 header.b="OAU35y/u"; dmarc=pass (policy=none) header.from=cmpxchg.org; spf=pass (imf09.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.222.175 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1719505126; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=O7FVo5HpSfwEv2hYMANEqIL76iocT4x+eD/q02M8eV4=; b=zniIvZOHw+Hm8AVvmanq6kjbPo4LqEIZ4jpzIM/yfS/KQu/OKN9gzyCZNLU6xexguTjjXr a65U60Pxvz7s/1bnyxQpd1oC//Z8EkRTPM0x2cZ1azQWQnRUuYvHUcSafWk6tjSjtm8/Hz 4SBQd7UzY74V+b5ovPDMkew53QJlC4A= Received: by mail-qk1-f175.google.com with SMTP id af79cd13be357-79c10f03a94so171701785a.1 for ; Thu, 27 Jun 2024 09:18:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20230601.gappssmtp.com; s=20230601; t=1719505138; x=1720109938; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=O7FVo5HpSfwEv2hYMANEqIL76iocT4x+eD/q02M8eV4=; b=OAU35y/utwz4U7VpRNhkKn3/oAxdFqtp/vvN9W+f0iPHR+9oiIiS4t/n5k9p82ww7P iKm6LQxlzo2ARFUTc2nsQVyjQcZHnOD05hdTl6MjrVX9BHYcwLwc+k1A2s0S50sTf4yQ LZXmV4PK87wBfnzPl9HVzyep8IPGP66skyAF8zbPlpBZRsKhalV+BFDQi3gImD0vsIOc S7ucQiKNjzs2GSFof/3wosGU2D/h/EIzBcwmzup6cRztzDTHs4uW4w/mPkEHu7oEVdCH FoYvyOOLCSZOWsSa32UHN3Na46XLxKNcU7HbZVsNgNLRVcOS2alqIUXoqVUsGXLJGm3q DHKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719505138; x=1720109938; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=O7FVo5HpSfwEv2hYMANEqIL76iocT4x+eD/q02M8eV4=; b=T8gwN0C1bJDA0BsVSOCoIcEp3JgrN6gj0kAg1QgMKjxiIQrILCUETUzYgRnx2Aihlx rj74zCQagq7Zt/lkwCACELDejDPwBDDIb7nw4yowEjwAw55TvO9XyQWKH5GFiylnJSF6 SpffojdtIFUW6h4rU902tUreGMp5HV5rBJPbqGYH810doR3nmP4HcYvusx78Ef5rD/0M EDZ3lGn7N37S9G2pWHZiI2PZGxrMkoDijJHoZmO2W1QWfuRq4Yk1KuaVfQarI5FvxzFs IycJnz6KWc9RR/SS1wdbsRmTyuSB+7IiEh5mxuH+xq0fnvgAdQ606QCy2ZmN0q4Mdxhh 8ibw== X-Forwarded-Encrypted: i=1; AJvYcCUHimNMRjaX7qYqFg5JcwOcLOY6AdbPOdaWB9YdT52GmVx1Zqk6XKyXikPW+UtO/9ntSvNk2llUM1Qd7COwuXqVY70= X-Gm-Message-State: AOJu0YzQuTH752Byv53SVH0lOnod3LTEH85z9BuLkgACJbJ2imxxpWj4 g8Y2V3+Pz4CTKTZPVZ6cFCW9XYmB09K9bdgBZS8n2tS3iEypRzL36SFRJuD8OpE= X-Google-Smtp-Source: AGHT+IEt2rugRoeVt7UaZce69jY4aS+O51iQgv/urf5rviUlkJZXNrtVZ33N0ME4mRKC9LlnAsT1XA== X-Received: by 2002:a05:620a:22c:b0:79b:a8df:7829 with SMTP id af79cd13be357-79d5cffa5f2mr330503185a.14.1719505137718; Thu, 27 Jun 2024 09:18:57 -0700 (PDT) Received: from localhost ([2603:7000:c01:2716:da5e:d3ff:fee7:26e7]) by smtp.gmail.com with ESMTPSA id af79cd13be357-79d5c8baca1sm67991585a.113.2024.06.27.09.18.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 Jun 2024 09:18:57 -0700 (PDT) Date: Thu, 27 Jun 2024 12:18:52 -0400 From: Johannes Weiner To: Usama Arif Cc: akpm@linux-foundation.org, shakeel.butt@linux.dev, david@redhat.com, ying.huang@intel.com, hughd@google.com, willy@infradead.org, yosryahmed@google.com, nphamcs@gmail.com, chengming.zhou@linux.dev, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel-team@meta.com, Andi Kleen Subject: Re: [PATCH v7 1/2] mm: store zero pages to be swapped out in a bitmap Message-ID: <20240627161852.GA469122@cmpxchg.org> References: <20240627105730.3110705-1-usamaarif642@gmail.com> <20240627105730.3110705-2-usamaarif642@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240627105730.3110705-2-usamaarif642@gmail.com> X-Rspamd-Queue-Id: 3957314001F X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: sqh8go1qhr5qwboni78wr4id4yid7oy4 X-HE-Tag: 1719505138-785867 X-HE-Meta: U2FsdGVkX19Ib7Qi4sIiFN6KkUTpm4v6Orxbq/wWqolqthbK674L3sYRx3kt69S6s4rQJ3jadJZmGutrD9gycRxSqK0LruUk1KM0A2oPgxhzsMAijJXFBe9ynTHX2fm+1eRYENPx8eSi9JFFJmz7vChYyyvPcgSF3h4508/x/bMgZ0b90MRpivjNk5csZKlKTithIHP6hPJzGX7VTcGQrq8ipBUAGV94M5mdN+XyuImzNnCtAAQ4CI8p3kcp+7cJFAnxPfp29gPBYXWRyHrzqpPpoOwjeBObF0Qve0xj6riVFzYkiQKIAISiahqD/cVnwbwPEPh09R793xZv0jyEEyEyGmQcQ8PEleKtT8q931P1onETxgfmpEU22LxGIbmQePVwbsp8ozLVlCpA52moP2Qjm2ZsZzLqSCrzOepJP20/M6WoUFvhwtIIZVwe2Cxo5pDf9CB5fHAY6AvnxqkW4+h7+sFfGue+ikCsKY3ERgkARxOsJvf78qZsW8+bpIoioekMQok4rmxuOA5OZnvK2XzxB4Eiyx0qGSR6zfaXwI4N56geHy9Y/cHtmV9YeJGpMRu9e0cjjofbNcNKX0PEme3ikYBoYfn1PFjKIRB30f87jYy5VOF2p5OGD817QaoX8w3CuGuCzWvUnqiPyv57ClOBuCzjVqC3VrGQ4mLgHgbBcVWkk2G/uf5tqC1vx/Na8Od6uWj2rEVHpb5BdEh+VXH3DF3Ge+9+WLcQfA6ziu7jVx1Ow2R1bxyC/6H2S2WYIxWiXO7Vtv3XGwIokEOJOLxX6Y3OKsxA6lgZAJovBMOv1YJjxWMPZ8W5IIj2Z0NCIIkatAzEQITAbWr4AlFh9DAtDHy+PkVkHr85H8kSR37Hn4b2BkhRbUrF/1tIbD1eG+pm4XyU4nu2VJUIUJkSSiCRSVqeP95nTgNDpslh7PNnAzKLV0cerH/AQXTz9rXxIGmXL8szriquqCNdFJY pzN3fCai pYdFPdUb+poctPQ+Mij1HDMSVXtPtm3eHFqurDC/cJo4hWa1P4T3BJUjSOIWab0/yAE5uxbP6Kl9NAmhiC2KKF+s06QEW6g0whrUODJNWohrwiNXXvScKw2sdfdbSbhHM5cCgWHfNd76V7CwVO1+AIbeD/9FaglFcQw6br5N+LH7Q7e6E68dealK6F/cGTYhWdIpL44Wuoi+joFq0E2Q9XiBgSUeyHh2+SfiQh21Ozl4RAHXUg9XmiECQU19LdJS574qq6YMXpjgG8TAFYlyu2X58+1mvnlXMbIRzgtYLXXvIF9BoUWa/Yq7B6LZeoelexLUlbv6xqyagZ8upf47u/+7h+D8HYDPzyBF/AhGYdX6WeMdntuvMaB6EtB11jJf2/heVO8MD9XVkLnmG2iYKs4Mubggqwqm4Hgth/sods6vdNV42akQa1mtSi/ZnskTFMxRLYD1MXVBMMtLn/Y3cR5cRvTly9azU6LxwztzDhAUvFvqEosgtluroscZT7ZNgDXvgCGoEZXs0UEguc8LZ3MZz6aFxngPmjxFUIKJd7KZ44qWWf/s3i2Q12gA+3m8qZAff9gXm9ftPfIyrgEQQ8YfHrnS0xFydCWspyxh9zmBPB5i0FBpBY6mE1sZ/Ca+kt6xLPMV329M7Si/PDgifsCkabWP8F9TMaER3WWvQwVqDhOZingBdO61IEel4J3MVioPY0XNuaYt8bhR0907ivYWb+Fm3oDZM/wJx8Fl1LrljDduMBemlytezZUYNBLbYc82z223Wx8o496dHpRF0lV8i+ISa8d0Presc 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 Usama, On Thu, Jun 27, 2024 at 11:55:29AM +0100, Usama Arif wrote: > Approximately 10-20% of pages to be swapped out are zero pages [1]. > Rather than reading/writing these pages to flash resulting > in increased I/O and flash wear, a bitmap can be used to mark these > pages as zero at write time, and the pages can be filled at > read time if the bit corresponding to the page is set. > With this patch, NVMe writes in Meta server fleet decreased > by almost 10% with conventional swap setup (zswap disabled). > > [1] https://lore.kernel.org/all/20171018104832epcms5p1b2232e2236258de3d03d1344dde9fce0@epcms5p1/ > > Signed-off-by: Usama Arif > Reviewed-by: Chengming Zhou > Reviewed-by: Yosry Ahmed > Reviewed-by: Nhat Pham > Cc: David Hildenbrand > Cc: "Huang, Ying" > Cc: Hugh Dickins > Cc: Johannes Weiner > Cc: Matthew Wilcox (Oracle) > Cc: Shakeel Butt > Cc: Usama Arif > Cc: Andi Kleen > Signed-off-by: Andrew Morton This looks great to me, and the numbers speak for themselves. A few minor comments below: > --- > include/linux/swap.h | 1 + > mm/page_io.c | 113 ++++++++++++++++++++++++++++++++++++++++++- > mm/swapfile.c | 20 ++++++++ > 3 files changed, 133 insertions(+), 1 deletion(-) > > diff --git a/include/linux/swap.h b/include/linux/swap.h > index 3df75d62a835..ed03d421febd 100644 > --- a/include/linux/swap.h > +++ b/include/linux/swap.h > @@ -299,6 +299,7 @@ struct swap_info_struct { > signed char type; /* strange name for an index */ > unsigned int max; /* extent of the swap_map */ > unsigned char *swap_map; /* vmalloc'ed array of usage counts */ > + unsigned long *zeromap; /* vmalloc'ed bitmap to track zero pages */ > struct swap_cluster_info *cluster_info; /* cluster info. Only for SSD */ > struct swap_cluster_list free_clusters; /* free clusters list */ > unsigned int lowest_bit; /* index of first free in swap_map */ > diff --git a/mm/page_io.c b/mm/page_io.c > index 6c1c1828bb88..480b8f221d90 100644 > --- a/mm/page_io.c > +++ b/mm/page_io.c > @@ -172,6 +172,88 @@ int generic_swapfile_activate(struct swap_info_struct *sis, > goto out; > } > It might be good to have a short comment that gives 1) an overview, that we're using a bitmap to avoid doing IO for zero-filled pages and 2) the locking, that the bits are protected by the locked swapcache folio and atomic updates are used to protect against RMW corruption due to other zero swap entries seeing concurrent updates. > +static bool is_folio_page_zero_filled(struct folio *folio, int i) > +{ > + unsigned long *data; > + unsigned int pos, last_pos = PAGE_SIZE / sizeof(*data) - 1; > + bool ret = false; > + > + data = kmap_local_folio(folio, i * PAGE_SIZE); > + if (data[last_pos]) > + goto out; This shortcut is interesting. It's what zswap does, for which git blame finds a commit with test results: commit 62bf1258ec90554c3c0925951149cfe4b67a3e98 Author: Taejoon Song Date: Mon Feb 6 04:00:36 2023 +0900 mm/zswap: try to avoid worst-case scenario on same element pages The worst-case scenario on finding same element pages is that almost all elements are same at the first glance but only last few elements are different. Since the same element tends to be grouped from the beginning of the pages, if we check the first element with the last element before looping through all elements, we might have some chances to quickly detect non-same element pages. 1. Test is done under LG webOS TV (64-bit arch) 2. Dump the swap-out pages (~819200 pages) 3. Analyze the pages with simple test script which counts the iteration number and measures the speed at off-line Under 64-bit arch, the worst iteration count is PAGE_SIZE / 8 bytes = 512. The speed is based on the time to consume page_same_filled() function only. The result, on average, is listed as below: Num of Iter Speed(MB/s) Looping-Forward (Orig) 38 99265 Looping-Backward 36 102725 Last-element-check (This Patch) 33 125072 The result shows that the average iteration count decreases by 13% and the speed increases by 25% with this patch. This patch does not increase the overall time complexity, though. I also ran simpler version which uses backward loop. Just looping backward also makes some improvement, but less than this patch. A similar change has already been made to zram in 90f82cbfe502 ("zram: try to avoid worst-case scenario on same element pages"). Since you add it here, and then delete the zswap code in the next patch, this link to the justification will be lost. Can you please add a short comment that explains that this is a measure to avoid worst-case behavior for tail-filled pages, which are common in real life workloads? > + for (pos = 0; pos < PAGE_SIZE / sizeof(*data); pos++) { > + if (data[pos]) > + goto out; > + } > + ret = true; > +out: > + kunmap_local(data); > + return ret; > +} > + > +static bool is_folio_zero_filled(struct folio *folio) > +{ > + unsigned int i; > + > + for (i = 0; i < folio_nr_pages(folio); i++) { > + if (!is_folio_page_zero_filled(folio, i)) > + return false; > + } > + return true; > +} I think these two functions can be merged. It's less code and fewer similar-sounding names in the namespace. > +static void folio_zero_fill(struct folio *folio) > +{ > + unsigned int i; > + > + for (i = 0; i < folio_nr_pages(folio); i++) > + clear_highpage(folio_page(folio, i)); > +} Should this be in highmem.h next to the other folio_zero_* functions? > +static void swap_zeromap_folio_set(struct folio *folio) > +{ > + struct swap_info_struct *sis = swp_swap_info(folio->swap); > + swp_entry_t entry; > + unsigned int i; > + > + for (i = 0; i < folio_nr_pages(folio); i++) { > + entry = page_swap_entry(folio_page(folio, i)); > + set_bit(swp_offset(entry), sis->zeromap); > + } > +} > + > +static void swap_zeromap_folio_clear(struct folio *folio) > +{ > + struct swap_info_struct *sis = swp_swap_info(folio->swap); > + swp_entry_t entry; > + unsigned int i; > + > + for (i = 0; i < folio_nr_pages(folio); i++) { > + entry = page_swap_entry(folio_page(folio, i)); > + clear_bit(swp_offset(entry), sis->zeromap); > + } > +} > + > +/* > + * Return the index of the first subpage which is not zero-filled > + * according to swap_info_struct->zeromap. > + * If all pages are zero-filled according to zeromap, it will return > + * folio_nr_pages(folio). > + */ > +static unsigned int swap_zeromap_folio_test(struct folio *folio) > +{ > + struct swap_info_struct *sis = swp_swap_info(folio->swap); > + swp_entry_t entry; > + unsigned int i; > + > + for (i = 0; i < folio_nr_pages(folio); i++) { > + entry = page_swap_entry(folio_page(folio, i)); > + if (!test_bit(swp_offset(entry), sis->zeromap)) > + return i; > + } > + return i; > +} > + > /* > * We may have stale swap cache pages in memory: notice > * them here and get rid of the unnecessary final write. > @@ -195,6 +277,13 @@ int swap_writepage(struct page *page, struct writeback_control *wbc) > folio_unlock(folio); > return ret; > } > + > + if (is_folio_zero_filled(folio)) { > + swap_zeromap_folio_set(folio); > + folio_unlock(folio); > + return 0; > + } > + swap_zeromap_folio_clear(folio); Just for clarity, it would be good to put this into an else branch and add a comment about overwrites due to changes in the in-memory data. Losing this would cause nasty data corruption, so let's make sure the reason for why it's there is spelled out, and stands out. > if (zswap_store(folio)) { > folio_unlock(folio); > return 0; > @@ -3161,6 +3170,17 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags) > goto bad_swap_unlock_inode; > } > > + /* > + * Use kvmalloc_array instead of bitmap_zalloc as the allocation order might > + * be above MAX_PAGE_ORDER incase of a large swap file. > + */ > + p->zeromap = kvmalloc_array(BITS_TO_LONGS(maxpages), sizeof(unsigned long), > + GFP_KERNEL | __GFP_ZERO); sizeof(long) would get this below 80 characters ;) > + if (!p->zeromap) { > + error = -ENOMEM; > + goto bad_swap_unlock_inode; > + } > + With that, Acked-by: Johannes Weiner