Re: [PATCH] psi: Treat ksm swapping in copy as memstall

[CGEL <cgel.zte@gmail.com>]

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@gmail.com wrote:
> > From: Yang Yang <yang.yang29@zte.com.cn>
> > 
> > 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@zte.com.cn>
> > ---
> >  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()