diff mbox series

[v3] mm/vmstat: add events for ksm cow

Message ID 20220324104332.2350482-1-yang.yang29@zte.com.cn (mailing list archive)
State New
Headers show
Series [v3] mm/vmstat: add events for ksm cow | expand

Commit Message

CGEL March 24, 2022, 10:43 a.m. UTC 
From: Yang Yang <yang.yang29@zte.com.cn>

Users may use ksm by calling madvise(, , MADV_MERGEABLE) when they want
to save memory, it's a tradeoff by suffering delay on ksm cow. Users can
get to know how much memory ksm saved by reading
/sys/kernel/mm/ksm/pages_sharing, but they don't know what's the costs
of ksm cow, and this is important of some delay sensitive tasks.

So add ksm cow events to help users evaluate whether or how to use ksm.

Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: xu xin <xu.xin16@zte.com.cn>
Reviewed-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
---
v2:
- fix compile error when CONFIG_KSM is not set
v3:
- delete KSM_COW_FAIL event
---
 include/linux/vm_event_item.h | 1 +
 mm/memory.c                   | 4 ++++
 mm/vmstat.c                   | 1 +
 3 files changed, 6 insertions(+)

Comments

Matthew Wilcox March 24, 2022, 12:45 p.m. UTC | #1 
On Thu, Mar 24, 2022 at 10:43:33AM +0000, cgel.zte@gmail.com wrote:
> +++ b/include/linux/vm_event_item.h
> @@ -131,6 +131,7 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
>  		SWAP_RA_HIT,
>  #ifdef CONFIG_KSM
>  		KSM_SWPIN_COPY,
> +		COW_KSM,

Shouldn't we call this KSM_COW?

> +++ b/mm/vmstat.c
> @@ -1390,6 +1390,7 @@ const char * const vmstat_text[] = {
>  	"swap_ra_hit",
>  #ifdef CONFIG_KSM
>  	"ksm_swpin_copy",
> +	"cow_ksm",

... and this "ksm_cow"?
David Hildenbrand March 24, 2022, 12:49 p.m. UTC | #2 
On 24.03.22 13:45, Matthew Wilcox wrote:
> On Thu, Mar 24, 2022 at 10:43:33AM +0000, cgel.zte@gmail.com wrote:
>> +++ b/include/linux/vm_event_item.h
>> @@ -131,6 +131,7 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
>>  		SWAP_RA_HIT,
>>  #ifdef CONFIG_KSM
>>  		KSM_SWPIN_COPY,
>> +		COW_KSM,
> 
> Shouldn't we call this KSM_COW?

I guess I proposed COW_KSM because ideally we'll have COW_ANON,
COW_ZERO, COW_OTHER, ... so it'd be under the "COW" namespace. No strong
opinion, though.
Andrew Morton March 25, 2022, 8:09 p.m. UTC | #3 
On Thu, 24 Mar 2022 10:43:33 +0000 cgel.zte@gmail.com wrote:

> From: Yang Yang <yang.yang29@zte.com.cn>
> 
> Users may use ksm by calling madvise(, , MADV_MERGEABLE) when they want
> to save memory, it's a tradeoff by suffering delay on ksm cow. Users can
> get to know how much memory ksm saved by reading
> /sys/kernel/mm/ksm/pages_sharing, but they don't know what's the costs
> of ksm cow, and this is important of some delay sensitive tasks.
> 
> So add ksm cow events to help users evaluate whether or how to use ksm.

It's unclear (to me) how anyone will actually use this, how they will
interpret the output.

Some tutorial words added to Documentation/vm/ksm.rst would be helpful.
While in there, please check for any other /proc/vmstat fields which
we forgot to document.

> --- a/include/linux/vm_event_item.h
> +++ b/include/linux/vm_event_item.h
> @@ -131,6 +131,7 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
>  		SWAP_RA_HIT,
>  #ifdef CONFIG_KSM
>  		KSM_SWPIN_COPY,
> +		COW_KSM,

I agree that this name looks unpleasingly backwards.  Do we have an
expectation that we actually will be adding more COW_* fields?
David Hildenbrand March 25, 2022, 8:19 p.m. UTC | #4 
On 25.03.22 21:09, Andrew Morton wrote:
> On Thu, 24 Mar 2022 10:43:33 +0000 cgel.zte@gmail.com wrote:
> 
>> From: Yang Yang <yang.yang29@zte.com.cn>
>>
>> Users may use ksm by calling madvise(, , MADV_MERGEABLE) when they want
>> to save memory, it's a tradeoff by suffering delay on ksm cow. Users can
>> get to know how much memory ksm saved by reading
>> /sys/kernel/mm/ksm/pages_sharing, but they don't know what's the costs
>> of ksm cow, and this is important of some delay sensitive tasks.
>>
>> So add ksm cow events to help users evaluate whether or how to use ksm.
> 
> It's unclear (to me) how anyone will actually use this, how they will
> interpret the output.
> 
> Some tutorial words added to Documentation/vm/ksm.rst would be helpful.
> While in there, please check for any other /proc/vmstat fields which
> we forgot to document.
> 
>> --- a/include/linux/vm_event_item.h
>> +++ b/include/linux/vm_event_item.h
>> @@ -131,6 +131,7 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
>>  		SWAP_RA_HIT,
>>  #ifdef CONFIG_KSM
>>  		KSM_SWPIN_COPY,
>> +		COW_KSM,
> 
> I agree that this name looks unpleasingly backwards.  Do we have an
> expectation that we actually will be adding more COW_* fields?

As raised previously (also when proposing this), I'd like to have
COW_ANON, COW_ZERO, COW_OTHER. Ideally, we'd have added all via a single
patch for them. They would at least be of value to me.
diff mbox series

Patch

diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
index 16a0a4fd000b..d817ba541851 100644
--- a/include/linux/vm_event_item.h
+++ b/include/linux/vm_event_item.h
@@ -131,6 +131,7 @@  enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
 		SWAP_RA_HIT,
 #ifdef CONFIG_KSM
 		KSM_SWPIN_COPY,
+		COW_KSM,
 #endif
 #endif
 #ifdef CONFIG_X86
diff --git a/mm/memory.c b/mm/memory.c
index 4111f97c91a0..12925ceaf745 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3339,6 +3339,10 @@  static vm_fault_t do_wp_page(struct vm_fault *vmf)
 	get_page(vmf->page);
 
 	pte_unmap_unlock(vmf->pte, vmf->ptl);
+#ifdef CONFIG_KSM
+	if (PageKsm(vmf->page))
+		count_vm_event(COW_KSM);
+#endif
 	return wp_page_copy(vmf);
 }
 
diff --git a/mm/vmstat.c b/mm/vmstat.c
index d5cc8d739fac..710d71af2285 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1390,6 +1390,7 @@  const char * const vmstat_text[] = {
 	"swap_ra_hit",
 #ifdef CONFIG_KSM
 	"ksm_swpin_copy",
+	"cow_ksm",
 #endif
 #endif
 #ifdef CONFIG_X86