linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] psi: Treat ksm swapping in copy as memstall
@ 2022-01-16 15:21 cgel.zte
  2022-01-17 12:14 ` Johannes Weiner
  0 siblings, 1 reply; 11+ messages in thread
From: cgel.zte @ 2022-01-16 15:21 UTC (permalink / raw)
  To: akpm, hannes, sfr; +Cc: linux-mm, linux-kernel, Yang Yang

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.

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);
 	return new_page;
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] psi: Treat ksm swapping in copy as memstall
  2022-01-16 15:21 [PATCH] psi: Treat ksm swapping in copy as memstall cgel.zte
@ 2022-01-17 12:14 ` Johannes Weiner
  2022-01-19  6:13   ` CGEL
  0 siblings, 1 reply; 11+ messages in thread
From: Johannes Weiner @ 2022-01-17 12:14 UTC (permalink / raw)
  To: cgel.zte; +Cc: akpm, sfr, linux-mm, linux-kernel, Yang Yang

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?

> 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.

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?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] psi: Treat ksm swapping in copy as memstall
  2022-01-17 12:14 ` Johannes Weiner
@ 2022-01-19  6:13   ` CGEL
  2022-01-19 12:58     ` Johannes Weiner
  0 siblings, 1 reply; 11+ messages in thread
From: CGEL @ 2022-01-19  6:13 UTC (permalink / raw)
  To: Johannes Weiner; +Cc: akpm, sfr, linux-mm, linux-kernel, Yang Yang

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()

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] psi: Treat ksm swapping in copy as memstall
  2022-01-19  6:13   ` CGEL
@ 2022-01-19 12:58     ` Johannes Weiner
  2022-01-21  9:51       ` CGEL
  0 siblings, 1 reply; 11+ messages in thread
From: Johannes Weiner @ 2022-01-19 12:58 UTC (permalink / raw)
  To: CGEL; +Cc: akpm, sfr, linux-mm, linux-kernel, Yang Yang

On Wed, Jan 19, 2022 at 06:13:54AM +0000, CGEL wrote:
> 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.

Ok, that sounds reasonable to me as well. Please add the
PageWorkingset() check and resubmit the patch. Thanks!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] psi: Treat ksm swapping in copy as memstall
  2022-01-19 12:58     ` Johannes Weiner
@ 2022-01-21  9:51       ` CGEL
  2022-01-28  1:29         ` Johannes Weiner
  0 siblings, 1 reply; 11+ messages in thread
From: CGEL @ 2022-01-21  9:51 UTC (permalink / raw)
  To: Johannes Weiner; +Cc: akpm, sfr, linux-mm, linux-kernel, Yang Yang

 Wed, Jan 19, 2022 at 07:58:23AM -0500, Johannes Weiner wrote:
> On Wed, Jan 19, 2022 at 06:13:54AM +0000, CGEL wrote:
> > 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.
> 
> Ok, that sounds reasonable to me as well. Please add the
> PageWorkingset() check and resubmit the patch. Thanks!
I am a litte confused about adding PageWorkingset(), since I
think ksm_might_need_to_copy() memstall is like swap_readpage()
memstall and swap_readpage() doesn't add PageWorkingset().

Would please make it cleaner? Thanks!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] psi: Treat ksm swapping in copy as memstall
  2022-01-21  9:51       ` CGEL
@ 2022-01-28  1:29         ` Johannes Weiner
  2022-01-28  2:31           ` CGEL
  0 siblings, 1 reply; 11+ messages in thread
From: Johannes Weiner @ 2022-01-28  1:29 UTC (permalink / raw)
  To: CGEL; +Cc: akpm, sfr, linux-mm, linux-kernel, Yang Yang

On Fri, Jan 21, 2022 at 09:51:08AM +0000, CGEL wrote:
>  Wed, Jan 19, 2022 at 07:58:23AM -0500, Johannes Weiner wrote:
> > On Wed, Jan 19, 2022 at 06:13:54AM +0000, CGEL wrote:
> > > 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.
> > 
> > Ok, that sounds reasonable to me as well. Please add the
> > PageWorkingset() check and resubmit the patch. Thanks!
> I am a litte confused about adding PageWorkingset(), since I
> think ksm_might_need_to_copy() memstall is like swap_readpage()
> memstall and swap_readpage() doesn't add PageWorkingset().

That's actually a bug! It should do that.

That psi_memstall_enter() in swap_readpage() was added by commit
937790699be9c8100e5358625e7dfa8b32bd33f2. It's for catching the
scenarios that don't go through submit_bio(). When you look at
submit_bio(), it counts stalls only if we have workingset pages:

	/*
	 * If we're reading data that is part of the userspace workingset, count
	 * submission time as memory stall.  When the device is congested, or
	 * the submitting cgroup IO-throttled, submission can be a significant
	 * part of overall IO time.
	 */
	if (unlikely(bio_op(bio) == REQ_OP_READ &&
	    bio_flagged(bio, BIO_WORKINGSET))) {
		unsigned long pflags;

		psi_memstall_enter(&pflags);
		submit_bio_noacct(bio);
		psi_memstall_leave(&pflags);
		return;
	}

I hope that clarifies it. I'll send a patch to fix up swap_readpage().

Thanks

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] psi: Treat ksm swapping in copy as memstall
  2022-01-28  1:29         ` Johannes Weiner
@ 2022-01-28  2:31           ` CGEL
  2022-02-08  3:22             ` Hugh Dickins
  0 siblings, 1 reply; 11+ messages in thread
From: CGEL @ 2022-01-28  2:31 UTC (permalink / raw)
  To: Johannes Weiner; +Cc: akpm, sfr, linux-mm, linux-kernel, Yang Yang

On Thu, Jan 27, 2022 at 08:29:08PM -0500, Johannes Weiner wrote:
> On Fri, Jan 21, 2022 at 09:51:08AM +0000, CGEL wrote:
> >  Wed, Jan 19, 2022 at 07:58:23AM -0500, Johannes Weiner wrote:
> > > On Wed, Jan 19, 2022 at 06:13:54AM +0000, CGEL wrote:
> > > > 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.
> > > 
> > > Ok, that sounds reasonable to me as well. Please add the
> > > PageWorkingset() check and resubmit the patch. Thanks!
> > I am a litte confused about adding PageWorkingset(), since I
> > think ksm_might_need_to_copy() memstall is like swap_readpage()
> > memstall and swap_readpage() doesn't add PageWorkingset().
> 
> That's actually a bug! It should do that.
I recently found that too. Please CC to me your new patch, thanks!
And I will send V2 of this patch "psi: Treat ksm swapping in copy
as memstall" with PageWorkingset().

> That psi_memstall_enter() in swap_readpage() was added by commit
> 937790699be9c8100e5358625e7dfa8b32bd33f2. It's for catching the
> scenarios that don't go through submit_bio(). When you look at
> submit_bio(), it counts stalls only if we have workingset pages:
> 
> 	/*
> 	 * If we're reading data that is part of the userspace workingset, count
> 	 * submission time as memory stall.  When the device is congested, or
> 	 * the submitting cgroup IO-throttled, submission can be a significant
> 	 * part of overall IO time.
> 	 */
> 	if (unlikely(bio_op(bio) == REQ_OP_READ &&
> 	    bio_flagged(bio, BIO_WORKINGSET))) {
> 		unsigned long pflags;
> 
> 		psi_memstall_enter(&pflags);
> 		submit_bio_noacct(bio);
> 		psi_memstall_leave(&pflags);
> 		return;
> 	}
> 
> I hope that clarifies it. I'll send a patch to fix up swap_readpage().
> 
> Thanks

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] psi: Treat ksm swapping in copy as memstall
  2022-01-28  2:31           ` CGEL
@ 2022-02-08  3:22             ` Hugh Dickins
  2022-02-08 13:09               ` CGEL
  0 siblings, 1 reply; 11+ messages in thread
From: Hugh Dickins @ 2022-02-08  3:22 UTC (permalink / raw)
  To: CGEL; +Cc: Johannes Weiner, akpm, sfr, linux-mm, linux-kernel, Yang Yang

On Fri, 28 Jan 2022, CGEL wrote:
> On Thu, Jan 27, 2022 at 08:29:08PM -0500, Johannes Weiner wrote:
> > On Fri, Jan 21, 2022 at 09:51:08AM +0000, CGEL wrote:
> > >  Wed, Jan 19, 2022 at 07:58:23AM -0500, Johannes Weiner wrote:
> > > > On Wed, Jan 19, 2022 at 06:13:54AM +0000, CGEL wrote:
> > > > > 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.
> > > > 
> > > > Ok, that sounds reasonable to me as well. Please add the
> > > > PageWorkingset() check and resubmit the patch. Thanks!
> > > I am a litte confused about adding PageWorkingset(), since I
> > > think ksm_might_need_to_copy() memstall is like swap_readpage()
> > > memstall and swap_readpage() doesn't add PageWorkingset().
> > 
> > That's actually a bug! It should do that.
> I recently found that too. Please CC to me your new patch, thanks!
> And I will send V2 of this patch "psi: Treat ksm swapping in copy
> as memstall" with PageWorkingset().

I'm entirely PSI-ignorant, and reluctant to disagree with Johannes,
but I don't see how your patch to ksm_might_need_to_copy() could be
correct - maybe the "swapping" in your subject is confusing.

There is no PSI enter and exit around the page allocation and copying
in wp_page_copy(), so why in the analogous ksm_might_need_to_copy()?

Hugh

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] psi: Treat ksm swapping in copy as memstall
  2022-02-08  3:22             ` Hugh Dickins
@ 2022-02-08 13:09               ` CGEL
  2022-02-09  5:55                 ` Hugh Dickins
  0 siblings, 1 reply; 11+ messages in thread
From: CGEL @ 2022-02-08 13:09 UTC (permalink / raw)
  To: Hugh Dickins
  Cc: Johannes Weiner, akpm, sfr, linux-mm, linux-kernel, Yang Yang

On Mon, Feb 07, 2022 at 07:22:22PM -0800, Hugh Dickins wrote:
> On Fri, 28 Jan 2022, CGEL wrote:
> > On Thu, Jan 27, 2022 at 08:29:08PM -0500, Johannes Weiner wrote:
> > > On Fri, Jan 21, 2022 at 09:51:08AM +0000, CGEL wrote:
> > > >  Wed, Jan 19, 2022 at 07:58:23AM -0500, Johannes Weiner wrote:
> > > > > On Wed, Jan 19, 2022 at 06:13:54AM +0000, CGEL wrote:
> > > > > > 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.
> > > > > 
> > > > > Ok, that sounds reasonable to me as well. Please add the
> > > > > PageWorkingset() check and resubmit the patch. Thanks!
> > > > I am a litte confused about adding PageWorkingset(), since I
> > > > think ksm_might_need_to_copy() memstall is like swap_readpage()
> > > > memstall and swap_readpage() doesn't add PageWorkingset().
> > > 
> > > That's actually a bug! It should do that.
> > I recently found that too. Please CC to me your new patch, thanks!
> > And I will send V2 of this patch "psi: Treat ksm swapping in copy
> > as memstall" with PageWorkingset().
> 
> I'm entirely PSI-ignorant, and reluctant to disagree with Johannes,
> but I don't see how your patch to ksm_might_need_to_copy() could be
> correct - maybe the "swapping" in your subject is confusing.
> 
> There is no PSI enter and exit around the page allocation and copying
> in wp_page_copy(), so why in the analogous ksm_might_need_to_copy()?
>
I think it's two questions, first why PSI didn't treat wp_page_copy() as
memstall, second why should PSI treat ksm_might_need_to_copy() as memstall.

The first question is unrelated with this patch. I think the reason is PSI
focous on memory contending(see Documentation/accounting/psi.rst), and
wp_page_copy() is not caused by memory contending. Actually wp_page_copy()
will still be called if memory is not contending.

For the second question, ksm_might_need_to_copy() is called only becaused
of swapping, and swap is caused by memory contending, so PSI better treat
it as memstall.

Thanks.
> Hugh

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] psi: Treat ksm swapping in copy as memstall
  2022-02-08 13:09               ` CGEL
@ 2022-02-09  5:55                 ` Hugh Dickins
  2022-02-10  6:52                   ` CGEL
  0 siblings, 1 reply; 11+ messages in thread
From: Hugh Dickins @ 2022-02-09  5:55 UTC (permalink / raw)
  To: CGEL
  Cc: Hugh Dickins, Johannes Weiner, akpm, sfr, linux-mm, linux-kernel,
	Yang Yang

On Tue, 8 Feb 2022, CGEL wrote:
> On Mon, Feb 07, 2022 at 07:22:22PM -0800, Hugh Dickins wrote:
> > On Fri, 28 Jan 2022, CGEL wrote:
> > > On Thu, Jan 27, 2022 at 08:29:08PM -0500, Johannes Weiner wrote:
> > > > On Fri, Jan 21, 2022 at 09:51:08AM +0000, CGEL wrote:
> > > > >  Wed, Jan 19, 2022 at 07:58:23AM -0500, Johannes Weiner wrote:
> > > > > > On Wed, Jan 19, 2022 at 06:13:54AM +0000, CGEL wrote:
> > > > > > > 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.
> > > > > > 
> > > > > > Ok, that sounds reasonable to me as well. Please add the
> > > > > > PageWorkingset() check and resubmit the patch. Thanks!
> > > > > I am a litte confused about adding PageWorkingset(), since I
> > > > > think ksm_might_need_to_copy() memstall is like swap_readpage()
> > > > > memstall and swap_readpage() doesn't add PageWorkingset().
> > > > 
> > > > That's actually a bug! It should do that.
> > > I recently found that too. Please CC to me your new patch, thanks!
> > > And I will send V2 of this patch "psi: Treat ksm swapping in copy
> > > as memstall" with PageWorkingset().
> > 
> > I'm entirely PSI-ignorant, and reluctant to disagree with Johannes,
> > but I don't see how your patch to ksm_might_need_to_copy() could be
> > correct - maybe the "swapping" in your subject is confusing.
> > 
> > There is no PSI enter and exit around the page allocation and copying
> > in wp_page_copy(), so why in the analogous ksm_might_need_to_copy()?
> >
> I think it's two questions, first why PSI didn't treat wp_page_copy() as
> memstall, second why should PSI treat ksm_might_need_to_copy() as memstall.
> 
> The first question is unrelated with this patch. I think the reason is PSI
> focous on memory contending(see Documentation/accounting/psi.rst), and
> wp_page_copy() is not caused by memory contending. Actually wp_page_copy()
> will still be called if memory is not contending.

Agreed.

> 
> For the second question, ksm_might_need_to_copy() is called only becaused
> of swapping, and swap is caused by memory contending, so PSI better treat
> it as memstall.

But there I'm not at all convinced.

 * psi_memstall_enter - mark the beginning of a memory stall section
 * @flags: flags to handle nested sections
 *
 * Marks the calling task as being stalled due to a lack of memory,
 * such as waiting for a refault or performing reclaim.

psi_memstall_enter() will have been called if do_swap_page() had to
read back from swap or wait on page lock; and psi_memstall_enter()
will be called if ksm_might_need_to_copy()'s alloc_page_vma() or
mem_cgroup_charge() goes into page reclaim.  Being stalled due to
a lack of memory is fully covered there.

Your argument is that copy_user_highpage() is a significant overhead
(but not a memstall), and it might have been avoided if there was
no KSM or no swapping - though doing the copy there often(?) saves
doing the wp_page_copy() when do_swap_page() goes on to do_wp_page()
(and that one you're not asking to count).

I can understand you wanting to keep track of page copying overhead;
and I've grown uneasy recently about the way CONFIG_KSM=y can add a
ksm_might_need_to_copy() overhead for pages which never went near KSM;
but I still don't see how psi_memstall is appropriate here.

Hugh

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] psi: Treat ksm swapping in copy as memstall
  2022-02-09  5:55                 ` Hugh Dickins
@ 2022-02-10  6:52                   ` CGEL
  0 siblings, 0 replies; 11+ messages in thread
From: CGEL @ 2022-02-10  6:52 UTC (permalink / raw)
  To: Hugh Dickins
  Cc: Johannes Weiner, akpm, sfr, linux-mm, linux-kernel, Yang Yang,
	ran.xiaokai, wang.yong12

On Tue, Feb 08, 2022 at 09:55:48PM -0800, Hugh Dickins wrote:
> On Tue, 8 Feb 2022, CGEL wrote:
> > On Mon, Feb 07, 2022 at 07:22:22PM -0800, Hugh Dickins wrote:
> > > On Fri, 28 Jan 2022, CGEL wrote:
> > > > On Thu, Jan 27, 2022 at 08:29:08PM -0500, Johannes Weiner wrote:
> > > > > On Fri, Jan 21, 2022 at 09:51:08AM +0000, CGEL wrote:
> > > > > >  Wed, Jan 19, 2022 at 07:58:23AM -0500, Johannes Weiner wrote:
> > > > > > > On Wed, Jan 19, 2022 at 06:13:54AM +0000, CGEL wrote:
> > > > > > > > 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.
> > > > > > > 
> > > > > > > Ok, that sounds reasonable to me as well. Please add the
> > > > > > > PageWorkingset() check and resubmit the patch. Thanks!
> > > > > > I am a litte confused about adding PageWorkingset(), since I
> > > > > > think ksm_might_need_to_copy() memstall is like swap_readpage()
> > > > > > memstall and swap_readpage() doesn't add PageWorkingset().
> > > > > 
> > > > > That's actually a bug! It should do that.
> > > > I recently found that too. Please CC to me your new patch, thanks!
> > > > And I will send V2 of this patch "psi: Treat ksm swapping in copy
> > > > as memstall" with PageWorkingset().
> > > 
> > > I'm entirely PSI-ignorant, and reluctant to disagree with Johannes,
> > > but I don't see how your patch to ksm_might_need_to_copy() could be
> > > correct - maybe the "swapping" in your subject is confusing.
> > > 
> > > There is no PSI enter and exit around the page allocation and copying
> > > in wp_page_copy(), so why in the analogous ksm_might_need_to_copy()?
> > >
> > I think it's two questions, first why PSI didn't treat wp_page_copy() as
> > memstall, second why should PSI treat ksm_might_need_to_copy() as memstall.
> > 
> > The first question is unrelated with this patch. I think the reason is PSI
> > focous on memory contending(see Documentation/accounting/psi.rst), and
> > wp_page_copy() is not caused by memory contending. Actually wp_page_copy()
> > will still be called if memory is not contending.
> 
> Agreed.
> 
> > 
> > For the second question, ksm_might_need_to_copy() is called only becaused
> > of swapping, and swap is caused by memory contending, so PSI better treat
> > it as memstall.
> 
> But there I'm not at all convinced.
> 
>  * psi_memstall_enter - mark the beginning of a memory stall section
>  * @flags: flags to handle nested sections
>  *
>  * Marks the calling task as being stalled due to a lack of memory,
>  * such as waiting for a refault or performing reclaim.
> 
> psi_memstall_enter() will have been called if do_swap_page() had to
> read back from swap or wait on page lock; and psi_memstall_enter()
> will be called if ksm_might_need_to_copy()'s alloc_page_vma() or
> mem_cgroup_charge() goes into page reclaim.  Being stalled due to
> a lack of memory is fully covered there.
PSI counts "read back from swap time" as memstall time, because the
reason why we had to "read back from swap" is memory contending had
happened before and caused swap out. ksm_might_need_to_copy() is just
like it, we had to do this copy because of swap out happened before,
not because ksm_might_need_to_copy() may goes into page reclaim. And
if page reclaim happens, doesn't matter, psi_memstall_enter() will
handle nested sections.

> Your argument is that copy_user_highpage() is a significant overhead
> (but not a memstall), and it might have been avoided if there was
> no KSM or no swapping - though doing the copy there often(?) saves
> doing the wp_page_copy() when do_swap_page() goes on to do_wp_page()
> (and that one you're not asking to count).
"Doing the copy" doesn't always means "saves doing the wp_page_copy()".
For example, if there is no FAULT_FLAG_WRITE flag. What about we only
count this case as memstall: ksm_might_need_to_copy() do the copy and
not "saves doing the wp_page_copy()"? As this copy takes 2 times longer
than swap_readpage() when use zram(see early mail on this maillist),
we'd better not just leave it.

> I can understand you wanting to keep track of page copying overhead;
> and I've grown uneasy recently about the way CONFIG_KSM=y can add a
> ksm_might_need_to_copy() overhead for pages which never went near KSM;
> but I still don't see how psi_memstall is appropriate here.
For "ksm_might_need_to_copy() overhead for pages which never went near KSM",
is there a plan to optimize it? Or should I work on this?

Thanks.

> Hugh

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-02-10  6:52 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-16 15:21 [PATCH] psi: Treat ksm swapping in copy as memstall cgel.zte
2022-01-17 12:14 ` Johannes Weiner
2022-01-19  6:13   ` CGEL
2022-01-19 12:58     ` Johannes Weiner
2022-01-21  9:51       ` CGEL
2022-01-28  1:29         ` Johannes Weiner
2022-01-28  2:31           ` CGEL
2022-02-08  3:22             ` Hugh Dickins
2022-02-08 13:09               ` CGEL
2022-02-09  5:55                 ` Hugh Dickins
2022-02-10  6:52                   ` CGEL

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).