Re: [PATCH] mm: avoid unconditional one-tick sleep when swapcache_prepare fails

[Chris Li <chrisl@kernel.org>]

On Thu, Sep 26, 2024 at 2:20 PM Barry Song <21cnbao@gmail.com> wrote:
>
> From: Barry Song <v-songbaohua@oppo.com>
>
> Commit 13ddaf26be32 ("mm/swap: fix race when skipping swapcache")
> introduced an unconditional one-tick sleep when `swapcache_prepare()`
> fails, which has led to reports of UI stuttering on latency-sensitive
> Android devices. To address this, we can use a waitqueue to wake up
> tasks that fail `swapcache_prepare()` sooner, instead of always
> sleeping for a full tick. While tasks may occasionally be woken by an
> unrelated `do_swap_page()`, this method is preferable to two scenarios:
> rapid re-entry into page faults, which can cause livelocks, and
> multiple millisecond sleeps, which visibly degrade user experience.
>
> Oven's testing shows that a single waitqueue resolves the UI
> stuttering issue. If a 'thundering herd' problem becomes apparent
> later, a waitqueue hash similar to `folio_wait_table[PAGE_WAIT_TABLE_SIZE]`
> for page bit locks can be introduced.
>
> Fixes: 13ddaf26be32 ("mm/swap: fix race when skipping swapcache")
> Cc: Kairui Song <kasong@tencent.com>
> Cc: "Huang, Ying" <ying.huang@intel.com>
> Cc: Yu Zhao <yuzhao@google.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Chris Li <chrisl@kernel.org>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Yosry Ahmed <yosryahmed@google.com>
> Cc: SeongJae Park <sj@kernel.org>
> Cc: Kalesh Singh <kaleshsingh@google.com>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: <stable@vger.kernel.org>
> Reported-by: Oven Liyang <liyangouwen1@oppo.com>
> Tested-by: Oven Liyang <liyangouwen1@oppo.com>
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> ---
>  mm/memory.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 2366578015ad..6913174f7f41 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4192,6 +4192,8 @@ static struct folio *alloc_swap_folio(struct vm_fault *vmf)
>  }
>  #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>
> +static DECLARE_WAIT_QUEUE_HEAD(swapcache_wq);
> +
>  /*
>   * We enter with non-exclusive mmap_lock (to exclude vma changes,
>   * but allow concurrent faults), and pte mapped but not yet locked.
> @@ -4204,6 +4206,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>  {
>         struct vm_area_struct *vma = vmf->vma;
>         struct folio *swapcache, *folio = NULL;
> +       DECLARE_WAITQUEUE(wait, current);
>         struct page *page;
>         struct swap_info_struct *si = NULL;
>         rmap_t rmap_flags = RMAP_NONE;
> @@ -4302,7 +4305,9 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>                                          * Relax a bit to prevent rapid
>                                          * repeated page faults.
>                                          */
> +                                       add_wait_queue(&swapcache_wq, &wait);
>                                         schedule_timeout_uninterruptible(1);
> +                                       remove_wait_queue(&swapcache_wq, &wait);

There is only one "swapcache_wq", if we don't care about the memory
overhead, ideally should be per swap entry that fails to grab the
HAS_CACHE bit and has one wait queue. Currently all swap entries using
one wait queue will likely cause other swap entries (if any) get wait
up then find out the swap entry it cares hasn't been served yet.

Another thing to consider is that, if we are using a wait queue, the
1ms is not relevant any more. It can be longer than 1ms and it is
getting waited up by the wait queue anyway. Here you might use
indefinitely sleep to reduce the unnecessary wait up and the
complexity of the timer.

>                                         goto out_page;
>                                 }
>                                 need_clear_cache = true;
> @@ -4609,8 +4614,10 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>                 pte_unmap_unlock(vmf->pte, vmf->ptl);
>  out:
>         /* Clear the swap cache pin for direct swapin after PTL unlock */
> -       if (need_clear_cache)
> +       if (need_clear_cache) {
>                 swapcache_clear(si, entry, nr_pages);
> +               wake_up(&swapcache_wq);

Agree with Ying that here the common path will need to take a lock to
wait up the wait queue.

Chris


> +       }
>         if (si)
>                 put_swap_device(si);
>         return ret;
> @@ -4625,8 +4632,10 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>                 folio_unlock(swapcache);
>                 folio_put(swapcache);
>         }
> -       if (need_clear_cache)
> +       if (need_clear_cache) {
>                 swapcache_clear(si, entry, nr_pages);
> +               wake_up(&swapcache_wq);
> +       }
>         if (si)
>                 put_swap_device(si);
>         return ret;
> --
> 2.34.1
>