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 77813C27C75 for ; Tue, 11 Jun 2024 18:43:47 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 12B0F6B00A4; Tue, 11 Jun 2024 14:43:47 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0DA5B6B00A8; Tue, 11 Jun 2024 14:43:47 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EBC6D6B00AC; Tue, 11 Jun 2024 14:43:46 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id CBE8E6B00A4 for ; Tue, 11 Jun 2024 14:43:46 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 41D47A0F6F for ; Tue, 11 Jun 2024 18:43:46 +0000 (UTC) X-FDA: 82219481652.01.51DAC18 Received: from mail-ed1-f44.google.com (mail-ed1-f44.google.com [209.85.208.44]) by imf12.hostedemail.com (Postfix) with ESMTP id 342DE4000B for ; Tue, 11 Jun 2024 18:43:43 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="g4gCdTO/"; spf=pass (imf12.hostedemail.com: domain of usamaarif642@gmail.com designates 209.85.208.44 as permitted sender) smtp.mailfrom=usamaarif642@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1718131424; 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=3B4dRZiAZKLlssSkuibM6VAi10SgS6TbPgd/0zmbDxc=; b=mNzUbM7tC4qd9thPgQIA2w6zT0DeQZKpjysj4hOvV/xhL/Qiiz6MCsK72dUw3FEHL3VysM h/v+BTE2TY1VxUvdHDpupyN0NHd8MiPsTlv2BAh1WmitCNT2arTTn4olE66/c/7PjD5aiA gp7Ge23uBqTtX0ZJ30g1VZeuIzzwG1g= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="g4gCdTO/"; spf=pass (imf12.hostedemail.com: domain of usamaarif642@gmail.com designates 209.85.208.44 as permitted sender) smtp.mailfrom=usamaarif642@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1718131424; a=rsa-sha256; cv=none; b=PNpZURPyV0EbCOk076U8KARQXYQ86mNU6BPOJfQdhiy30vzpdwWPK374JuHcumV9ADl6XK KFGKc0DQQCAdYBkbb6j+5lekKGHsHaCduIfq0qhJ3uBv4jqOo7j6Eq+w4kO0zJrn7s6iI5 nsm76HmgEu4+YyjKBX7xhUeiSrveenw= Received: by mail-ed1-f44.google.com with SMTP id 4fb4d7f45d1cf-57c923e03caso1646953a12.3 for ; Tue, 11 Jun 2024 11:43:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1718131422; x=1718736222; darn=kvack.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=3B4dRZiAZKLlssSkuibM6VAi10SgS6TbPgd/0zmbDxc=; b=g4gCdTO/f8cJF11v391Mop2OHLbsl1QZDeHDbsPu20dIuU+1GdZYjhx1y9aM6OxMvO CmhKkXn3qZ4hAQPHhNztTDt+gxQO194ON9AxhuwS+5zw32LINXvA0pOlB/X6YrnGRr5U VoIpdampdcL1h3xfeL2LwKJkMLY/B3/9kDUZEcka9IPjtrDlb9/5rPzIyj9VBraGsal4 X0j6GE6QynDoECboKK9bGrC/xuSoaSy75OFLb8yP9Zai7CzerjUuzq09vEYCyYtdsYXq mgDzzhrPngPINObwa7/9sk399lMo+V24Gw0U1nx9DuQ3Ag+DRhZ11k/9VXf95q4i3An/ 3Mzw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718131422; x=1718736222; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=3B4dRZiAZKLlssSkuibM6VAi10SgS6TbPgd/0zmbDxc=; b=p5Z0HYnrJl/L86/ALQM5q8I1zIcEVx/P8WRKU03pqI7bZ4Nn5YaVMWu2UqgUrcmzHJ cD8GHvpXSjFAyY7YskGutIWvoE9iCoT4IOfAGAQrtVaDSew0YGRykuSdxAYYTSb3Y6zr 3cExHXI/Q4rlNO7/LRuTPgqTpDzfExZ3dAtkYQhjJHlwyrh1EYvFipijz+ayRoLKcbtl tJAI+6p5wY3/S83vHdpM1sWGJOMukvn1bXAMFs/0NXQaYHOXnwIhpJOqQQkynzr5pHhH 8mGUcmtE7n51aIszL61KZoizExfeGGDcdmFqM1+dK8M8eA8WfCnOQzWBx2RyE/y6xsCw H3HQ== X-Forwarded-Encrypted: i=1; AJvYcCUmos0yvVdb2WynDz6ETuYNe/ZbjArnDucnXLODk8qpjDs5JQeeKHEWx+/oPYPlHG/8V3FJcPktZ7V0kUyz3ClTa0U= X-Gm-Message-State: AOJu0YzfyIa6nVbnC/E+LMR+1N7yebuauMt2nNC6BgLDaD3/+w+1J3UV NmUIOFf4U+QIpmwnpn9bnJRaRdMOB5Kavj6pbbZhJyWjqx1vW22O X-Google-Smtp-Source: AGHT+IE3zSTRh8xAH8Py1sMJDx/BjKoKZNHDkSiAjCEUIwefZi48SYXnfFRQ122iGGSTZAIEXbm/XA== X-Received: by 2002:a50:8e11:0:b0:57c:8049:a9a with SMTP id 4fb4d7f45d1cf-57c80490adfmr4008291a12.2.1718131422174; Tue, 11 Jun 2024 11:43:42 -0700 (PDT) Received: from ?IPV6:2a03:83e0:1126:4:eb:d0d0:c7fd:c82c? ([2620:10d:c092:500::7:57b4]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-57c7fcfc67fsm4635790a12.31.2024.06.11.11.43.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 11 Jun 2024 11:43:41 -0700 (PDT) Message-ID: <622bc591-ad14-448e-a9f3-988976fbb98a@gmail.com> Date: Tue, 11 Jun 2024 19:43:41 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 1/2] mm: store zero pages to be swapped out in a bitmap To: Yosry Ahmed Cc: 21cnbao@gmail.com, akpm@linux-foundation.org, hannes@cmpxchg.org, david@redhat.com, ying.huang@intel.com, hughd@google.com, willy@infradead.org, nphamcs@gmail.com, chengming.zhou@linux.dev, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel-team@meta.com, Shakeel Butt References: <20240610121820.328876-1-usamaarif642@gmail.com> <20240610121820.328876-2-usamaarif642@gmail.com> <9ddfe544-636d-4638-ae0e-053674e47322@gmail.com> <08ea43f2-13d2-4b27-ae62-42cebc185c7b@gmail.com> Content-Language: en-US From: Usama Arif In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 342DE4000B X-Rspam-User: X-Rspamd-Server: rspam09 X-Stat-Signature: yt4879j736hy6i6rqcbe9b3e5zsijq5h X-HE-Tag: 1718131423-674337 X-HE-Meta: U2FsdGVkX1/KomVxwFPss/dfAHYayE/grJGJty1upe+34ANkc4QB7RmT7wrzTxHhrS5uulApcFR+53/lChr2kU5wkkuw7gISk5pNQRj963x8ALlGDXPJpdneN1aUCDowXvUqc26glOUnMPuRZf6zkDo0YtsG1VNJFClMtYDdrAbvQ39WmFQbxBZiqTlp19qlx14viNV2iiALbyhJwLI9caZ3gFA30rFTr/NWtBZ+Epnv4Gx7sTfDrSoiWJroK+WA7/h59VrZH39gp08sumTkwGfS08igFOBbCj5OdJlwHEjtRuuRjVYcGjOaxE2wdTgqlvF2Osh6P3LoiPiMn6vkT/ZelcHis3fYvuWm73l9VZlKqUiN6cfqlvUXFvzl0nn2Q7zX8mwfhw+dGKv5EyLWWgcU/ctNAfaTOBDH3w1HgqE8qkMyxJhIk79JaimQv92NkqWr1vWzZjSEeBjVzksjOj5XnbGyrhWIdj7PhB8bS1Ogut1+X3jeGj5weWOzKx+WhBc14CNI6xQRwxN1sHWVhIpZPa+e1nIfcOghVMv+fHCXWpVHJFpo7/sW/OCPtQBOqC7aEWvKNWo1OkTUvAkgAZ4jNDo6XCiYIlqlqff9lwQQ/kcqFYPWwHGKQO9jXEsuACqfif7dcLMK+7PwqQfS9kcBq8LIfqJTo8NZIbqpjCfb1f+HE2YGhvdNPGFnZUdicE6ekH1yPa4QrxXn64xvHRWqeV118T6nLJ7vk/4+NRNZSLtUuGSC/lbkyzeyiEn4UflnA3j7Y7zihov0LzAGsYZyQMKTx0moMS6OHN04zV3AjlsY4Dtk/UhBfUUwe88SfgPC3lX8KNMT3g9YuUaLW+osWAibEZ5WRIWE7sUQKKRtLUdIbOADanhTDg8G8uloT02M66K1WvJ4DeteK7WDhf91E9xwqEgx90IOxUoFYrGFxjPXNJ2UGJ2YMKRQ1TnWgaDnh7JJ/5t5AgvAlT5 Bpc95wiL XX8MBpmm1hqIzzym3Cf2nIXncYoywM/l76wGhLK22o3tKTBZjsz1s2DSiA6BVReVnWCJZ7j6Ii6+peo1d9NyYHpK6YIt6t/9vKTgDO5MvqZW7k/PP+FlrOmzrT6KbcAYmUT0BQMCbvUfrDWWvOYn2HDjjF5xIJKXPdzUul+EMYmPE3SVfDBbmh7pEGyPJsy4j8TZVAnEFGKLfiKkjufZBHxjdqsQrEGmV8uubVGqWY+e0dKBFjApbn4MLYTu5gM4aG6pnE2jr0fSiDwZoiD8C9fzSYCUQvDHZtk0vtnuv/Qe/2U+TMNBDvVMuv8dJUDBekBuqnBg3QhL79KmrfE6a8lPdV/oblPvYPOIcYVIOsZjdVVKUwNIJSuV1BskxsWz77Geb63HcIDAVeL/mZmCmnXFCKqAtZQ67rpPtnnWqIqYapoOCI7bdd8KmneHOqj1cnOf2PiQ9BDtRAT6VUPM8DndrhSI42kwcktFOQt3YolQYrFwZCYkxbCjpirznnQhiyvV7 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 11/06/2024 18:51, Yosry Ahmed wrote: > [..] >>>> I think its better to handle this in Barrys patch. I feel this series is >>>> close to its final state, i.e. the only diff I have for the next >>>> revision is below to remove start/end_writeback for zer_filled case. I >>>> will comment on Barrys patch once the I send out the next revision of this. >>> Sorry I did not make myself clearer. I did not mean that you should >>> handle the large folio swapin here. This needs to be handled at a >>> higher level because as you mentioned, a large folio may be partially >>> in the zeromap, zswap, swapcache, disk, etc. >>> >>> What I meant is that we should probably have a debug check to make >>> sure this doesn't go unhandled. For zswap, I am trying to add a >>> warning and fail the swapin operation if a large folio slips through >>> to zswap. We can do something similar here if folks agree this is the >>> right way in the interim: >>> https://lore.kernel.org/lkml/20240611024516.1375191-3-yosryahmed@google.com/. >>> >>> Maybe I am too paranoid, but I think it's easy to mess up these things >>> when working on large folio swapin imo. >> So there is a difference between zswap and this optimization. In this >> optimization, if the zeromap is set for all the folio bits, then we >> should do large folio swapin. There still needs to be a change in Barrys >> patch in alloc_swap_folio, but apart from that does the below diff over >> v3 make it better? I will send a v4 with this if it sounds good. >> >> >> diff --git a/mm/page_io.c b/mm/page_io.c >> index 6400be6e4291..bf01364748a9 100644 >> --- a/mm/page_io.c >> +++ b/mm/page_io.c >> @@ -234,18 +234,24 @@ static void swap_zeromap_folio_clear(struct folio >> *folio) >> } >> } >> >> -static bool swap_zeromap_folio_test(struct folio *folio) >> +/* >> + * 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 long swap_zeromap_folio_test(struct folio *folio) >> { >> struct swap_info_struct *sis = swp_swap_info(folio->swap); >> swp_entry_t entry; >> - unsigned int i; >> + long i; > Why long? folio_nr_pages returns long, but I just checked that folio->_folio_nr_pages is unsigned int, but that will probably be typecasted to long :). I will switch to unsigned int as its not really going to go to long for CONFIG_64BIT >> 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 false; >> + return i; >> } >> - return true; >> + return i; >> } >> >> /* >> @@ -581,6 +587,7 @@ void swap_read_folio(struct folio *folio, bool >> synchronous, >> { >> struct swap_info_struct *sis = swp_swap_info(folio->swap); >> bool workingset = folio_test_workingset(folio); >> + long first_non_zero_page_idx; >> unsigned long pflags; >> bool in_thrashing; >> >> @@ -598,10 +605,19 @@ void swap_read_folio(struct folio *folio, bool >> synchronous, >> psi_memstall_enter(&pflags); >> } >> delayacct_swapin_start(); >> - if (swap_zeromap_folio_test(folio)) { >> + first_non_zero_page_idx = swap_zeromap_folio_test(folio); >> + if (first_non_zero_page_idx == folio_nr_pages(folio)) { >> folio_zero_fill(folio); >> folio_mark_uptodate(folio); >> folio_unlock(folio); >> + } else if (first_non_zero_page_idx != 0) { >> + /* >> + * The case for when only *some* of subpages being >> swapped-in were recorded >> + * in sis->zeromap, while the rest are in zswap/disk is >> currently not handled. >> + * WARN in this case and return without marking the >> folio uptodate so that >> + * an IO error is emitted (e.g. do_swap_page() will sigbus). >> + */ >> + WARN_ON_ONCE(1); >> } else if (zswap_load(folio)) { >> folio_mark_uptodate(folio); >> folio_unlock(folio); >> >> > This is too much noise for swap_read_folio(). How about adding > swap_read_folio_zeromap() that takes care of this and decides whether > or not to call folio_mark_uptodate()? Sounds good, will do as below. Thanks! > > -static bool swap_zeromap_folio_test(struct folio *folio) > +/* > + * 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; > @@ -243,9 +248,9 @@ static bool swap_zeromap_folio_test(struct folio *folio) > 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 false; > + return i; > } > - return true; > + return i; > } > > /* > @@ -511,6 +516,25 @@ static void sio_read_complete(struct kiocb *iocb, long ret) > mempool_free(sio, sio_pool); > } > > +static bool swap_read_folio_zeromap(struct folio *folio) > +{ > + unsigned int idx = swap_zeromap_folio_test(folio); > + > + if (idx == 0) > + return false; > + > + /* > + * Swapping in a large folio that is partially in the zeromap is not > + * currently handled. Return true without marking the folio uptodate so > + * that an IO error is emitted (e.g. do_swap_page() will sigbus). > + */ > + if (WARN_ON_ONCE(idx < folio_nr_pages(folio))) > + return true; > + > + folio_zero_fill(folio); > + folio_mark_uptodate(folio); > + return true > +} > + > static void swap_read_folio_fs(struct folio *folio, struct swap_iocb **plug) > { > struct swap_info_struct *sis = swp_swap_info(folio->swap); > @@ -600,9 +624,7 @@ void swap_read_folio(struct folio *folio, bool synchronous, > psi_memstall_enter(&pflags); > } > delayacct_swapin_start(); > - if (swap_zeromap_folio_test(folio)) { > - folio_zero_fill(folio); > - folio_mark_uptodate(folio); > + if (swap_read_folio_zeromap(folio)) { > folio_unlock(folio); > } else if (zswap_load(folio)) { > folio_mark_uptodate(folio);