On Mon, Apr 01, 2024 at 11:15:18PM +0800, Kairui Song wrote:
> On Mon, Apr 1, 2024 at 10:15 PM Zhaoyu Liu
> wrote:
> >
>
> Hi Zhaoyu
>
> Not sure why but I can't apply your patch, maybe you need to fix your
> email client?
Thanks for your comment, kairui and Nhat.
OK, I'll check the mail 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.
Sorry, I don't quite understand. Cluster has be lock/unlock in func.
Same approach as swap_swapcount().
>
> > /*
> > * 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.
Yeah, you are right. SWAP_HAS_CACHE wouldn't mean that entry must be in
swap cache.
I guess you want to confirm through swap_count that the entry is about
to be added to the swap cache rather than about to be deleted from the
swap cache. But sometimes, when an entry is about to be removed from the
swap cache, the swap_count is not equal to 0,
eg. should_try_to_free_swap() in do_swap_page().
>
> 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.
Awesome, I'll study it!
>
> 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)?
That's right. However, even without adding this check swapcache_prepare()
still equals -EEXIST and returns NULL when skip_if_exists equals true.
>
> 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?
As I experimented at the beginning of the introduction, if the system
memory pressure is large, alloc_pages_mpol will be very time-consuming,
so when the entry is about to join the swap cache, I don't think it is
necessary to go to the alloc folio, because swapcache_prepare is likely
to return -EEXIST. Just retry filemap, get the page from the swap cache,
and reduce the impact on memory management.
