Received-SPF: pass (google.com: domain of linux-kernel+bounces-210456-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <622bc591-ad14-448e-a9f3-988976fbb98a@gmail.com>
Date: Tue, 11 Jun 2024 19:43:41 +0100
Precedence: bulk
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 <yosryahmed@google.com>
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 <shakeel.butt@linux.dev>
References: <20240610121820.328876-1-usamaarif642@gmail.com>
 <20240610121820.328876-2-usamaarif642@gmail.com>
 <CAJD7tkbpXjg00CRSrXU_pbaHwEaW1b3k8AQgu8y2PAh7EkTOug@mail.gmail.com>
 <d822ec8b-3b7d-4e5c-ac96-b6fda63da5d4@gmail.com>
 <CAJD7tkZoaM=dWim7GPqEERDZyxp3PqMNctzXQCPDMO=8kQSwfw@mail.gmail.com>
 <9ddfe544-636d-4638-ae0e-053674e47322@gmail.com>
 <CAJD7tkY0F-Tczq4j861HxKATzEOfkVv=76m8zFcJHuh8E3VMEw@mail.gmail.com>
 <08ea43f2-13d2-4b27-ae62-42cebc185c7b@gmail.com>
 <CAJD7tkZC8e8ZTBSOGZH-1srTeC=jqxwWchd-BjvNsV2FR0oT8Q@mail.gmail.com>
Content-Language: en-US
From: Usama Arif <usamaarif642@gmail.com>
In-Reply-To: <CAJD7tkZC8e8ZTBSOGZH-1srTeC=jqxwWchd-BjvNsV2FR0oT8Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


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);