lkml.org 
[lkml]   [2024]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [RFC PATCH 04/10] mm/swap: remove cache bypass swapin
    Date
    Kairui Song <ryncsn@gmail.com> writes:

    > On Wed, Mar 27, 2024 at 2:32 PM Huang, Ying <ying.huang@intel.com> wrote:
    >>
    >> Kairui Song <ryncsn@gmail.com> writes:
    >>
    >> > From: Kairui Song <kasong@tencent.com>
    >> >
    >> > We used to have the cache bypass swapin path for better performance,
    >> > but by removing it, more optimization can be applied and have
    >> > an even better overall performance and less hackish.
    >> >
    >> > And these optimizations are not easily doable or not doable at all
    >> > without this.
    >> >
    >> > This patch simply removes it, and the performance will drop heavily
    >> > for simple swapin, things won't get this worse for real workloads
    >> > but still observable. Following commits will fix this and archive a
    >> > better performance.
    >> >
    >> > Swapout/in 30G zero pages from ZRAM (This mostly measures overhead
    >> > of swap path itself, because zero pages are not compressed but simply
    >> > recorded in ZRAM, and performance drops more as SWAP device is getting
    >> > full):
    >> >
    >> > Test result of sequential swapin/out:
    >> >
    >> > Before (us) After (us)
    >> > Swapout: 33619409 33624641
    >> > Swapin: 32393771 41614858 (-28.4%)
    >> > Swapout (THP): 7817909 7795530
    >> > Swapin (THP) : 32452387 41708471 (-28.4%)
    >> >
    >> > Signed-off-by: Kairui Song <kasong@tencent.com>
    >> > ---
    >> > mm/memory.c | 18 ++++-------------
    >> > mm/swap.h | 10 +++++-----
    >> > mm/swap_state.c | 53 ++++++++++---------------------------------------
    >> > mm/swapfile.c | 13 ------------
    >> > 4 files changed, 19 insertions(+), 75 deletions(-)
    >> >
    >> > diff --git a/mm/memory.c b/mm/memory.c
    >> > index dfdb620a9123..357d239ee2f6 100644
    >> > --- a/mm/memory.c
    >> > +++ b/mm/memory.c
    >> > @@ -3932,7 +3932,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
    >> > struct page *page;
    >> > struct swap_info_struct *si = NULL;
    >> > rmap_t rmap_flags = RMAP_NONE;
    >> > - bool need_clear_cache = false;
    >> > bool exclusive = false;
    >> > swp_entry_t entry;
    >> > pte_t pte;
    >> > @@ -4000,14 +3999,9 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
    >> > if (!folio) {
    >> > if (data_race(si->flags & SWP_SYNCHRONOUS_IO) &&
    >> > __swap_count(entry) == 1) {
    >> > - /* skip swapcache and readahead */
    >> > folio = swapin_direct(entry, GFP_HIGHUSER_MOVABLE, vmf);
    >> > - if (PTR_ERR(folio) == -EBUSY)
    >> > - goto out;
    >> > - need_clear_cache = true;
    >> > } else {
    >> > folio = swapin_readahead(entry, GFP_HIGHUSER_MOVABLE, vmf);
    >> > - swapcache = folio;
    >> > }
    >> >
    >> > if (!folio) {
    >> > @@ -4023,6 +4017,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
    >> > goto unlock;
    >> > }
    >> >
    >> > + swapcache = folio;
    >> > page = folio_file_page(folio, swp_offset(entry));
    >> >
    >> > /* Had to read the page from swap area: Major fault */
    >> > @@ -4187,7 +4182,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
    >> > vmf->orig_pte = pte;
    >> >
    >> > /* ksm created a completely new copy */
    >> > - if (unlikely(folio != swapcache && swapcache)) {
    >> > + if (unlikely(folio != swapcache)) {
    >> > folio_add_new_anon_rmap(folio, vma, vmf->address);
    >> > folio_add_lru_vma(folio, vma);
    >> > } else {
    >> > @@ -4201,7 +4196,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
    >> > arch_do_swap_page(vma->vm_mm, vma, vmf->address, pte, vmf->orig_pte);
    >> >
    >> > folio_unlock(folio);
    >> > - if (folio != swapcache && swapcache) {
    >> > + if (folio != swapcache) {
    >> > /*
    >> > * Hold the lock to avoid the swap entry to be reused
    >> > * until we take the PT lock for the pte_same() check
    >> > @@ -4227,9 +4222,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
    >> > if (vmf->pte)
    >> > pte_unmap_unlock(vmf->pte, vmf->ptl);
    >> > out:
    >> > - /* Clear the swap cache pin for direct swapin after PTL unlock */
    >> > - if (need_clear_cache)
    >> > - swapcache_clear(si, entry);
    >> > if (si)
    >> > put_swap_device(si);
    >> > return ret;
    >> > @@ -4240,12 +4232,10 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
    >> > folio_unlock(folio);
    >> > out_release:
    >> > folio_put(folio);
    >> > - if (folio != swapcache && swapcache) {
    >> > + if (folio != swapcache) {
    >> > folio_unlock(swapcache);
    >> > folio_put(swapcache);
    >> > }
    >> > - if (need_clear_cache)
    >> > - swapcache_clear(si, entry);
    >> > if (si)
    >> > put_swap_device(si);
    >> > return ret;
    >> > diff --git a/mm/swap.h b/mm/swap.h
    >> > index aee134907a70..ac9573b03432 100644
    >> > --- a/mm/swap.h
    >> > +++ b/mm/swap.h
    >> > @@ -41,7 +41,6 @@ void __delete_from_swap_cache(struct folio *folio,
    >> > void delete_from_swap_cache(struct folio *folio);
    >> > void clear_shadow_from_swap_cache(int type, unsigned long begin,
    >> > unsigned long end);
    >> > -void swapcache_clear(struct swap_info_struct *si, swp_entry_t entry);
    >> > struct folio *swap_cache_get_folio(swp_entry_t entry,
    >> > struct vm_area_struct *vma, unsigned long addr);
    >> > struct folio *filemap_get_incore_folio(struct address_space *mapping,
    >> > @@ -100,14 +99,15 @@ static inline struct folio *swapin_readahead(swp_entry_t swp, gfp_t gfp_mask,
    >> > {
    >> > return NULL;
    >> > }
    >> > -
    >> > -static inline int swap_writepage(struct page *p, struct writeback_control *wbc)
    >> > +static inline struct folio *swapin_direct(swp_entry_t entry, gfp_t flag,
    >> > + struct vm_fault *vmf);
    >> > {
    >> > - return 0;
    >> > + return NULL;
    >> > }
    >> >
    >> > -static inline void swapcache_clear(struct swap_info_struct *si, swp_entry_t entry)
    >> > +static inline int swap_writepage(struct page *p, struct writeback_control *wbc)
    >> > {
    >> > + return 0;
    >> > }
    >> >
    >> > static inline struct folio *swap_cache_get_folio(swp_entry_t entry,
    >> > diff --git a/mm/swap_state.c b/mm/swap_state.c
    >> > index 2a9c6bdff5ea..49ef6250f676 100644
    >> > --- a/mm/swap_state.c
    >> > +++ b/mm/swap_state.c
    >> > @@ -880,61 +880,28 @@ static struct folio *swap_vma_readahead(swp_entry_t targ_entry, gfp_t gfp_mask,
    >> > }
    >> >
    >> > /**
    >> > - * swapin_direct - swap in folios skipping swap cache and readahead
    >> > + * swapin_direct - swap in folios skipping readahead
    >> > * @entry: swap entry of this memory
    >> > * @gfp_mask: memory allocation flags
    >> > * @vmf: fault information
    >> > *
    >> > - * Returns the struct folio for entry and addr after the swap entry is read
    >> > - * in.
    >> > + * Returns the folio for entry after it is read in.
    >> > */
    >> > struct folio *swapin_direct(swp_entry_t entry, gfp_t gfp_mask,
    >> > struct vm_fault *vmf)
    >> > {
    >> > - struct vm_area_struct *vma = vmf->vma;
    >> > + struct mempolicy *mpol;
    >> > struct folio *folio;
    >> > - void *shadow = NULL;
    >> > -
    >> > - /*
    >> > - * Prevent parallel swapin from proceeding with
    >> > - * the cache flag. Otherwise, another thread may
    >> > - * finish swapin first, free the entry, and swapout
    >> > - * reusing the same entry. It's undetectable as
    >> > - * pte_same() returns true due to entry reuse.
    >> > - */
    >> > - if (swapcache_prepare(entry)) {
    >> > - /* Relax a bit to prevent rapid repeated page faults */
    >> > - schedule_timeout_uninterruptible(1);
    >> > - return ERR_PTR(-EBUSY);
    >> > - }
    >> > -
    >> > - /* skip swapcache */
    >> > - folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0,
    >> > - vma, vmf->address, false);
    >> > - if (folio) {
    >> > - __folio_set_locked(folio);
    >> > - __folio_set_swapbacked(folio);
    >> > -
    >> > - if (mem_cgroup_swapin_charge_folio(folio,
    >> > - vma->vm_mm, GFP_KERNEL,
    >> > - entry)) {
    >> > - folio_unlock(folio);
    >> > - folio_put(folio);
    >> > - return NULL;
    >> > - }
    >> > - mem_cgroup_swapin_uncharge_swap(entry);
    >> > -
    >> > - shadow = get_shadow_from_swap_cache(entry);
    >> > - if (shadow)
    >> > - workingset_refault(folio, shadow);
    >> > + bool page_allocated;
    >> > + pgoff_t ilx;
    >> >
    >> > - folio_add_lru(folio);
    >> > + mpol = get_vma_policy(vmf->vma, vmf->address, 0, &ilx);
    >> > + folio = __read_swap_cache_async(entry, gfp_mask, mpol, ilx,
    >> > + &page_allocated, false);
    >> > + mpol_cond_put(mpol);
    >> >
    >> > - /* To provide entry to swap_read_folio() */
    >> > - folio->swap = entry;
    >> > + if (page_allocated)
    >> > swap_read_folio(folio, true, NULL);
    >> > - folio->private = NULL;
    >> > - }
    >> >
    >> > return folio;
    >> > }
    >>
    >> This looks similar as read_swap_cache_async(). Can we merge them?
    >
    > Yes, that's doable. But I may have to split it out again for later
    > optimizations though.
    >
    >>
    >> And, we should avoid to readahead in swapin_readahead() or
    >> swap_vma_readahead() for SWP_SYNCHRONOUS_IO anyway. So, it appears that
    >> we can change and use swapin_readahead() directly?
    >
    > Good point, SWP_SYNCHRONOUS_IO check can be extended more after this
    > series, but readahead optimization could be another series (like the
    > previous one which tried to unify readahead for shmem/anon), so I
    > thought it's better to keep it untouched for now.

    Just want to check whether we can reduce the special processing for
    SWP_SYNCHRONOUS_IO as much as possible.

    --
    Best Regards,
    Huang, Ying

    \
     
     \ /
      Last update: 2024-05-27 16:10    [W:4.420 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site