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

4 Oct 2024

On Fri, Oct 4, 2024 at 7:03 AM Chris Li chrisl@kernel.org wrote:
...
On Wed, Oct 2, 2024 at 5:35 PM Huang, Ying ying.huang@intel.com wrote:
...
Barry Song 21cnbao@gmail.com writes:
...
On Wed, Oct 2, 2024 at 8:43 AM Huang, Ying ying.huang@intel.com wrote:
...
Barry Song 21cnbao@gmail.com writes:
...
On Tue, Oct 1, 2024 at 7:43 AM Huang, Ying ying.huang@intel.com wrote:
...
Barry Song 21cnbao@gmail.com writes:
> On Sun, Sep 29, 2024 at 3:43 PM Huang, Ying ying.huang@intel.com wrote:
>>
>> Hi, Barry,
>>
>> Barry Song 21cnbao@gmail.com writes:
>>
>> > 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.
>>
>> In general, I think that this works.  Why not extend the solution to
>> cover schedule_timeout_uninterruptible() in __read_swap_cache_async()
>> too?  We can call wake_up() when we clear SWAP_HAS_CACHE.  To avoid
>
> Hi Ying,
> Thanks for your comments.
> I feel extending the solution to __read_swap_cache_async() should be done
> in a separate patch. On phones, I've never encountered any issues reported
> on that path, so it might be better suited for an optimization rather than a
> hotfix?
Yes.  It's fine to do that in another patch as optimization.
Ok. I'll prepare a separate patch for optimizing that path.
Thanks!
...
...
>> overhead to call wake_up() when there's no task waiting, we can use an
>> atomic to count waiting tasks.
>
> I'm not sure it's worth adding the complexity, as wake_up() on an empty
> waitqueue should have a very low cost on its own?
wake_up() needs to call spin_lock_irqsave() unconditionally on a global
shared lock.  On systems with many CPUs (such servers), this may cause
severe lock contention.  Even the cache ping-pong may hurt performance
much.
I understand that cache synchronization was a significant issue before
qspinlock, but it seems to be less of a concern after its implementation.
Unfortunately, qspinlock cannot eliminate cache ping-pong issue, as
discussed in the following thread.
https://lore.kernel.org/lkml/20220510192708.GQ76023@worktop.programming.kick...
...
However, using a global atomic variable would still trigger cache broadcasts,
correct?
We can only change the atomic variable to non-zero when
swapcache_prepare() returns non-zero, and call wake_up() when the atomic
variable is non-zero.  Because swapcache_prepare() returns 0 most times,
the atomic variable is 0 most times.  If we don't change the value of
atomic variable, cache ping-pong will not be triggered.
yes. this can be implemented by adding another atomic variable.
Just realized that we don't need another atomic variable for this, just
use waitqueue_active() before wake_up() should be enough.
...
...
Hi, Kairui,
Do you have some test cases to test parallel zram swap-in?  If so, that
can be used to verify whether cache ping-pong is an issue and whether it
can be fixed via a global atomic variable.
Yes, Kairui please run a test on your machine with lots of cores before
and after adding a global atomic variable as suggested by Ying. I am
sorry I don't have a server machine.
if it turns out you find cache ping-pong can be an issue, another
approach would be a waitqueue hash:
Yes.  waitqueue hash may help reduce lock contention.  And, we can have
both waitqueue_active() and waitqueue hash if necessary.  As the first
step, waitqueue_active() appears simpler.
Interesting. Just take a look at the waitqueue_active(), it requires
smp_mb() if using without holding the lock.
Quote from the comment of waitqueue_active():

Also note that this 'optimization' trades a spin_lock() for an smp_mb(),
which (when the lock is uncontended) are of roughly equal cost.

probably not a problem in our case. two reasons:
1. we don't have a condition here
2. false postive/negative wake_up() won't cause a problem here.
We used to always sleep at least 4ms for an embedded system, if we can
kill 99% of the possibilities, it is all good.
Ideally, we could combine wait queue hash and wakeup_active(), but
Kairui's test shows even if we did neither of the above, it is still acceptable
in performance. so probably we can make things simple by just
adding a if(waitqueue_active()) before wake_up().
...
Chris
...
...

diff --git a/mm/memory.c b/mm/memory.c
index 2366578015ad..aae0e532d8b6 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4192,6 +4192,23 @@ static struct folio *alloc_swap_folio(struct vm_fault *vmf)
 }
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
+/*


Alleviating the 'thundering herd' phenomenon using a waitqueue hash



when multiple do_swap_page() operations occur simultaneously.


*/

+#define SWAPCACHE_WAIT_TABLE_BITS 5
+#define SWAPCACHE_WAIT_TABLE_SIZE (1 << SWAPCACHE_WAIT_TABLE_BITS)
+static wait_queue_head_t swapcache_wqs[SWAPCACHE_WAIT_TABLE_SIZE];



+static int __init swapcache_wqs_init(void)
+{

for (int i = 0; i < SWAPCACHE_WAIT_TABLE_SIZE; i++)


        init_waitqueue_head(&swapcache_wqs[i]);



   return 0;



+}
+late_initcall(swapcache_wqs_init);



/*

We enter with non-exclusive mmap_lock (to exclude vma changes,
but allow concurrent faults), and pte mapped but not yet locked.

@@ -4204,6 +4221,8 @@ 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);


wait_queue_head_t *swapcache_wq;
struct page *page;
struct swap_info_struct *si = NULL;
rmap_t rmap_flags = RMAP_NONE;



@@ -4297,12 +4316,16 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
                               * undetectable as pte_same() returns true due
                               * to entry reuse.
                               */

                        swapcache_wq = &swapcache_wqs[hash_long(vmf->address & PMD_MASK,


                                                SWAPCACHE_WAIT_TABLE_BITS)];
                        if (swapcache_prepare(entry, nr_pages)) {
                                /*
                                 * 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);
                                goto out_page;
                        }
                        need_clear_cache = true;



@@ -4609,8 +4632,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);


}
if (si)
        put_swap_device(si);
return ret;



@@ -4625,8 +4650,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;



--
Best Regards,
Huang, Ying
Thanks
Barry

    

2024

2023

2022

2021

2020

2019

2018

2017

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