On Mon, Jan 17, 2022 at 07:14:53AM -0500, Johannes Weiner wrote:
> Hello Yang,
>
> On Sun, Jan 16, 2022 at 03:21:51PM +0000, cgel.zte@xxxxxxxxx wrote:
> > From: Yang Yang <yang.yang29@xxxxxxxxxx>
> >
> > When faults in from swap what used to be a ksm page and that page
> > had been swapped in before, system has to make a copy. Obviously
> > this kind of copy is related to high memory pressure, so we treat
> > it as memstall. Although ksm page merging is not because of high
> > memory pressure.
> >
> > Information of this new kind of stall will help psi to account
> > memory pressure more precise.
>
> Thanks for your patch. I'm curious if you have a concrete use case
> where this makes a difference, or if this is something you found while
> reading the code?
>
Thanks for your reply. I found it while reading the code, and did a test,
please see below.
> > Signed-off-by: Yang Yang <yang.yang29@xxxxxxxxxx>
> > ---
> > mm/ksm.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/mm/ksm.c b/mm/ksm.c
> > index 4a7f8614e57d..d4ec6773f9b8 100644
> > --- a/mm/ksm.c
> > +++ b/mm/ksm.c
> > @@ -39,6 +39,7 @@
> > #include <linux/freezer.h>
> > #include <linux/oom.h>
> > #include <linux/numa.h>
> > +#include <linux/psi.h>
> >
> > #include <asm/tlbflush.h>
> > #include "internal.h"
> > @@ -2569,6 +2570,7 @@ struct page *ksm_might_need_to_copy(struct page *page,
> > {
> > struct anon_vma *anon_vma = page_anon_vma(page);
> > struct page *new_page;
> > + unsigned long pflags;
> >
> > if (PageKsm(page)) {
> > if (page_stable_node(page) &&
> > @@ -2583,6 +2585,7 @@ struct page *ksm_might_need_to_copy(struct page *page,
> > if (!PageUptodate(page))
> > return page; /* let do_swap_page report the error */
> >
> > + psi_memstall_enter(&pflags);
> > new_page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, address);
> > if (new_page &&
> > mem_cgroup_charge(page_folio(new_page), vma->vm_mm, GFP_KERNEL)) {
> > @@ -2600,6 +2603,7 @@ struct page *ksm_might_need_to_copy(struct page *page,
> > #endif
> > }
> >
> > + psi_memstall_leave(&pflags);
>
> This does unconditional stall accounting for a swapin operation. But
> if you take a look at workingset_refault() -> folio_wait_bit_common(),
> we only count memory stalls when the page is thrashing, not when it's
> a transitionary refault (which happen even when there is enough memory
> to hold the workingset). You need to check PageWorkingset() at least.
>
I see PSI already does stall accounting for a swapin operation of zram
in most conditions. See swap_readpage(), it calls psi_memstall_enter().
> But again I'd be curious first if this is a practical concern. Swapins
> should be IO dominated - or in the case of zswap dominated by the
> decompression. Does a page copy really matter?
I did a test, when we use zram, it takes longer time for ksm copying than
swap_readpage(). Ksm copying average takes 147263ns, swap_readpage()
average takes 55639ns. So I think this patch is reasonable.
I use ktime_get_ts64() for time calcuating in swap_readpage() and
ksm_might_need_to_copy(). Code likes:
psi_memstall_enter()
ktime_get_ts64(&ts_start)
//ksm copy or swapin
ktime_get_ts64(&ts_end)
time = timespec64_sub(ts_end, ts_start)
psi_memstall_leave()
