mm: swap: properly update readahead statistics in unuse_pte_range()

In unuse_pte_range() we blindly swap-in pages without checking if the swap entry is already present in the swap cache.

By doing this, the hit/miss ratio used by the swap readahead heuristic is not properly updated and this leads to non-optimal performance during swapoff.

Tracing the distribution of the readahead size returned by the swap readahead heuristic during swapoff shows that a small readahead size is used most of the time as if we had only misses (this happens both with cluster and vma readahead), for example:

r::swapin_nr_pages(unsigned long offset):unsigned long:$retval

        COUNT      EVENT
        36948      $retval = 8
        44151      $retval = 4
        49290      $retval = 1
        527771     $retval = 2

Checking if the swap entry is present in the swap cache, instead, allows to properly update the readahead statistics and the heuristic behaves in a better way during swapoff, selecting a bigger readahead size:

r::swapin_nr_pages(unsigned long offset):unsigned long:$retval

        COUNT      EVENT
        1618       $retval = 1
        4960       $retval = 2
        41315      $retval = 4
        103521     $retval = 8

In terms of swapoff performance the result is the following:

Testing environment
===================

- Host:

   CPU: 1.8GHz Intel Core i7-8565U (quad-core, 8MB cache)
   HDD: PC401 NVMe SK hynix 512GB
   MEM: 16GB

- Guest (kvm):
8GB of RAM
virtio block driver
16GB swap file on ext4 (/swapfile)

Test case
=========
- allocate 85% of memory
- systemctl hibernate to force all the pages to be swapped-out to the swap file
- resume the system
- measure the time that swapoff takes to complete:

   # /usr/bin/time swapoff /swapfile

Result (swapoff time)
======
5.6 vanilla 5.6 w/ this patch
----------- -----------------

cluster-readahead      22.09s              12.19s
    vma-readahead      18.20s              15.33s

Signed-off-by "Huang, Ying" <[email protected]>

Quoted 3 lines · Andrea Righi

In unuse_pte_range() we blindly swap-in pages without checking if the swap entry is already present in the swap cache.

Quoted 3 lines

By doing this, the hit/miss ratio used by the swap readahead heuristic is not properly updated and this leads to non-optimal performance during swapoff.

It's more important to describe why we need this patch in the patch description. So, please add some information about your use case. And please focus on the technical part instead of the business part.

Thanks! But you don't need to do this. You can add my Reviewed-by after we have finished the work on patch description.

Quoted 9 lines · Ying

Quoted 6 lines · Andrea Righi

In unuse_pte_range() we blindly swap-in pages without checking if the swap entry is already present in the swap cache.

By doing this, the hit/miss ratio used by the swap readahead heuristic is not properly updated and this leads to non-optimal performance during swapoff.

Quoted 3 lines

It's more important to describe why we need this patch in the patch description. So, please add some information about your use case. And please focus on the technical part instead of the business part.

Confused. I thought the changelog was quite good. If "business part" means "end user effect of the patch" then that's a very important thing.

Quoted 2 lines

Thanks! But you don't need to do this. You can add my Reviewed-by after we have finished the work on patch description.

Can you be more specific about how you want this changed?

Quoted 11 lines · Andrew Morton

Quoted 9 lines · Ying

Quoted 6 lines · Andrea Righi

In unuse_pte_range() we blindly swap-in pages without checking if the swap entry is already present in the swap cache.

By doing this, the hit/miss ratio used by the swap readahead heuristic is not properly updated and this leads to non-optimal performance during swapoff.

Quoted 7 lines

Quoted 3 lines

It's more important to describe why we need this patch in the patch description. So, please add some information about your use case. And please focus on the technical part instead of the business part.

Confused. I thought the changelog was quite good. If "business part" means "end user effect of the patch" then that's a very important thing.

Previously, Andrea has described their use case in the cloud environment to hiberate the guest and swapoff after resuming. So swapoff performance is important for them. I think that should be included. For the business part, I mean something like "Ubuntu used in AWS EC2", I think that isn't important for the patch description.