Re: [PATCH] mm: swap: prejudgement swap_has_cache to avoid page allocation

[Kairui Song]

On Mon, Apr 1, 2024 at 10:15 PM Zhaoyu Liu
<liuzhaoyu.zackary@xxxxxxxxxxxxx> wrote:
>

Hi Zhaoyu

Not sure why but I can't apply your patch, maybe you need to fix your
email client?

> Based on qemu arm64 - latest kernel + 100M memory + 1024M swapfile.
> Create 1G anon mmap and set it to shared, and has two processes
> randomly access the shared memory. When they are racing on swap cache,
> on average, each "alloc_pages_mpol + swapcache_prepare + folio_put"
> took about 1475 us.
>
> So skip page allocation if SWAP_HAS_CACHE was set, just
> schedule_timeout_uninterruptible and continue to acquire page
> via filemap_get_folio() from swap cache, to speedup
> __read_swap_cache_async.
>
> Signed-off-by: Zhaoyu Liu
> ---
> include/linux/swap.h | 6 ++++++
> mm/swap_state.c | 10 ++++++++++
> mm/swapfile.c | 15 +++++++++++++++
> 3 files changed, 31 insertions(+)
>
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index a211a0383425..8a0013299f38 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -480,6 +480,7 @@ extern sector_t swapdev_block(int, pgoff_t);
> extern int __swap_count(swp_entry_t entry);
> extern int swap_swapcount(struct swap_info_struct *si, swp_entry_t entry);
> extern int swp_swapcount(swp_entry_t entry);
> +extern bool swap_has_cache(struct swap_info_struct *si, swp_entry_t entry);
> struct swap_info_struct *swp_swap_info(swp_entry_t entry);
> struct backing_dev_info;
> extern int init_swap_address_space(unsigned int type, unsigned long nr_pages);
> @@ -570,6 +571,11 @@ static inline int swp_swapcount(swp_entry_t entry)
> return 0;
> }
>
> +static inline bool swap_has_cache(struct swap_info_struct *si, swp_entry_t entry)
> +{
> + return false;
> +}
> +
> static inline swp_entry_t folio_alloc_swap(struct folio *folio)
> {
> swp_entry_t entry;
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index bfc7e8c58a6d..f130cfc669ce 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -462,6 +462,15 @@ struct folio *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
> if (!swap_swapcount(si, entry) && swap_slot_cache_enabled)
> goto fail_put_swap;
>
> + /*
> + * Skipping page allocation if SWAP_HAS_CACHE was set,
> + * just schedule_timeout_uninterruptible and continue to
> + * acquire page via filemap_get_folio() from swap cache,
> + * to speedup __read_swap_cache_async.
> + */
> + if (swap_has_cache(si, entry))
> + goto skip_alloc;
> +

But will this cause more lock contention? You need to lock the cluster
for the has_cache now.

> /*
> * Get a new folio to read into from swap. Allocate it now,
> * before marking swap_map SWAP_HAS_CACHE, when -EEXIST will
> @@ -483,6 +492,7 @@ struct folio *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
> if (err != -EEXIST)
> goto fail_put_swap;
>
> +skip_alloc:
> /*
> * Protect against a recursive call to __read_swap_cache_async()
> * on the same entry waiting forever here because SWAP_HAS_CACHE
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index cf900794f5ed..5388950c4ca6 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1513,6 +1513,21 @@ int swp_swapcount(swp_entry_t entry)
> return count;
> }
>
> +/*
> + * Verify that a swap entry has been tagged with SWAP_HAS_CACHE
> + */
> +bool swap_has_cache(struct swap_info_struct *si, swp_entry_t entry)
> +{
> + pgoff_t offset = swp_offset(entry);
> + struct swap_cluster_info *ci;
> + bool has_cache;
> +
> + ci = lock_cluster_or_swap_info(si, offset);
> + has_cache = !!(si->swap_map[offset] & SWAP_HAS_CACHE);

I think you also need to check swap_count here, if an entry was just
freed or loaded into slot cache, it will also have SWAP_HAS_CACHE set.

I have a very similar function in my another series (see __swap_has_cache):
https://lore.kernel.org/all/20240326185032.72159-10-ryncsn@xxxxxxxxx/

The situation is different with this patch though. But this check is
not reliable in both patches, having SWAP_HAS_CACHE doesn't mean the
folio is in the cache, and even if it's in the cache, it might get
freed very soon. So you need to ensure later checks can ensure the
final result is not affected.

eg. If swap_has_cache returns true, then swap cache is freed, and
skip_if_exists is set to true, __read_swap_cache_async will return
NULL for an entry that it should be able to alloc and cache, could
this be a problem (for example, causing zswap writeback to fail with
ENOMEM due to readahead)?

Also the race window that you are trying to avoid seems to be very
short and rare? Not sure if the whole idea is worth it and actually
affects performance in a positive way, any data on that?