On Wed, Oct 2, 2024 at 8:43 AM Huang, Ying <ying.huang@xxxxxxxxx> wrote:
>
> Barry Song <21cnbao@xxxxxxxxx> writes:
>
> > On Tue, Oct 1, 2024 at 7:43 AM Huang, Ying <ying.huang@xxxxxxxxx> wrote:
> >>
> >> Barry Song <21cnbao@xxxxxxxxx> writes:
> >>
> >> > On Sun, Sep 29, 2024 at 3:43 PM Huang, Ying <ying.huang@xxxxxxxxx> wrote:
> >> >>
> >> >> Hi, Barry,
> >> >>
> >> >> Barry Song <21cnbao@xxxxxxxxx> writes:
> >> >>
> >> >> > From: Barry Song <v-songbaohua@xxxxxxxx>
> >> >> >
> >> >> > 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
>
> > 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.
>
> 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:
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;
--
2.34.1
> --
> Best Regards,
> Huang, Ying
Thanks
Barry
