mbox series

[v3,0/5] ksm: support tracking KSM-placed zero-pages

Message ID 20221011022006.322158-1-xu.xin16@zte.com.cn (mailing list archive)
Headers show
Series ksm: support tracking KSM-placed zero-pages | expand

Message

xu xin Oct. 11, 2022, 2:20 a.m. UTC
From: xu xin <xu.xin16@zte.com.cn>

use_zero_pages is good, not just because of cache colouring as described
in doc, but also because use_zero_pages can accelerate merging empty pages
when there are plenty of empty pages (full of zeros) as the time of
page-by-page comparisons (unstable_tree_search_insert) is saved.

But there is something to improve, that is, when enabling use_zero_pages,
all empty pages will be merged with kernel zero pages instead of with each
other as use_zero_pages is disabled, and then these zero-pages are no longer
managed and monitor by KSM, which leads to two issues at least:

1) MADV_UNMERGEABLE and other ways to trigger unsharing will *not*
   unshare the shared zeropage as placed by KSM (which is against the 
   MADV_UNMERGEABLE documentation at least); see the link:
   https://lore.kernel.org/lkml/4a3daba6-18f9-d252-697c-197f65578c44@redhat.com/

2) we cannot know how many pages are zero pages placed by KSM when
   enabling use_zero_pages, which leads to KSM not being transparent
   with all actual merged pages by KSM.

Zero pages may be the most common merged pages in actual environment(not only VM but
also including other application like containers). Enabling use_zero_pages in the
environment with plenty of empty pages(full of zeros) will be very useful. Users and
app developer can also benefit from knowing the proportion of zero pages in all
merged pages to optimize applications.

With the patch series, we can both unshare zero-pages(KSM-placed) accurately
and count ksm zero pages with enabling use_zero_pages.

---
v2->v3:

1) Add more descriptive information in cover letter.

2) In [patch 2/5], add more commit log for explaining reasons.

3) In [patch 2/5], fix misuse of break_ksm() in unmerge_ksm_pages():
   break_ksm(vma, addr, NULL) -> break_ksm(vma, addr, false);

---
v1->v2:

[patch 4/5] fix build warning, mm/ksm.c:550, misleading indentation; statement 
'rmap_item->mm->ksm_zero_pages_sharing--;' is not part of the previous 'if'.



*** BLURB HERE ***

xu xin (5):
  ksm: abstract the function try_to_get_old_rmap_item
  ksm: support unsharing zero pages placed by KSM
  ksm: count all zero pages placed by KSM
  ksm: count zero pages for each process
  ksm: add zero_pages_sharing documentation

 Documentation/admin-guide/mm/ksm.rst |  10 +-
 fs/proc/base.c                       |   1 +
 include/linux/mm_types.h             |   7 +-
 mm/ksm.c                             | 177 +++++++++++++++++++++------
 4 files changed, 157 insertions(+), 38 deletions(-)

Comments

Andrew Morton Oct. 17, 2022, 11:55 p.m. UTC | #1
On Tue, 11 Oct 2022 02:20:06 +0000 xu.xin.sc@gmail.com wrote:

> From: xu xin <xu.xin16@zte.com.cn>
> 
> use_zero_pages is good, not just because of cache colouring as described
> in doc, but also because use_zero_pages can accelerate merging empty pages
> when there are plenty of empty pages (full of zeros) as the time of
> page-by-page comparisons (unstable_tree_search_insert) is saved.
> 
> But there is something to improve, that is, when enabling use_zero_pages,
> all empty pages will be merged with kernel zero pages instead of with each
> other as use_zero_pages is disabled, and then these zero-pages are no longer
> managed and monitor by KSM, which leads to two issues at least:

Sorry, but I'm struggling to understand what real value this patchset
offers.

> 1) MADV_UNMERGEABLE and other ways to trigger unsharing will *not*
>    unshare the shared zeropage as placed by KSM (which is against the 
>    MADV_UNMERGEABLE documentation at least); see the link:
>    https://lore.kernel.org/lkml/4a3daba6-18f9-d252-697c-197f65578c44@redhat.com/

Is that causing users any real-world problem?  If not, just change the
documentation?

> 2) we cannot know how many pages are zero pages placed by KSM when
>    enabling use_zero_pages, which leads to KSM not being transparent
>    with all actual merged pages by KSM.

Why is this a problem?

A full description of the real-world end-user operational benefits of
these changes would help, please.
xu xin Oct. 18, 2022, 9 a.m. UTC | #2
>> From: xu xin <xu.xin16@zte.com.cn>
>> 
>> use_zero_pages is good, not just because of cache colouring as described
>> in doc, but also because use_zero_pages can accelerate merging empty pages
>> when there are plenty of empty pages (full of zeros) as the time of
>> page-by-page comparisons (unstable_tree_search_insert) is saved.
>> 
>> But there is something to improve, that is, when enabling use_zero_pages,
>> all empty pages will be merged with kernel zero pages instead of with each
>> other as use_zero_pages is disabled, and then these zero-pages are no longer
>> managed and monitor by KSM, which leads to two issues at least:
>
>Sorry, but I'm struggling to understand what real value this patchset
>offers.
>
>> 1) MADV_UNMERGEABLE and other ways to trigger unsharing will *not*
>>    unshare the shared zeropage as placed by KSM (which is against the 
>>    MADV_UNMERGEABLE documentation at least); see the link:
>>    https://lore.kernel.org/lkml/4a3daba6-18f9-d252-697c-197f65578c44@redhat.com/
>
>Is that causing users any real-world problem?  If not, just change the
>documentation?
>
>> 2) we cannot know how many pages are zero pages placed by KSM when
>>    enabling use_zero_pages, which leads to KSM not being transparent
>>    with all actual merged pages by KSM.
>
>Why is this a problem?
>
>A full description of the real-world end-user operational benefits of
>these changes would help, please.
>

The core idea of this patch set is to enable users to perceive the number of any
pages merged by KSM, regardless of whether use_zero_page switch has been turned
on, so that users can know how much free memory increase is really due to their
madvise(MERGEABLE) actions. The motivation for me to do this is that when I do
an application optimization of KSM on embedded Linux for 5G platform, I find
that ksm_merging_pages of some process becomes very small(but used to be large),
which led me to think that there was any problem with the application KSM-madvise
strategy, but in fact, it was only because use_zero_pages is on.
Andrew Morton Oct. 18, 2022, 10:54 p.m. UTC | #3
On Tue, 18 Oct 2022 09:00:22 +0000 xu xin <xu.xin.sc@gmail.com> wrote:

> >A full description of the real-world end-user operational benefits of
> >these changes would help, please.
> >
> 
> The core idea of this patch set is to enable users to perceive the number of any
> pages merged by KSM, regardless of whether use_zero_page switch has been turned
> on, so that users can know how much free memory increase is really due to their
> madvise(MERGEABLE) actions.

OK, thanks.

> The motivation for me to do this is that when I do
> an application optimization of KSM on embedded Linux for 5G platform, I find
> that ksm_merging_pages of some process becomes very small(but used to be large),
> which led me to think that there was any problem with the application KSM-madvise
> strategy, but in fact, it was only because use_zero_pages is on.

Please expand on the above motivation and experience, and include it in
the [0/n] changelog.  But let's leave it a few days to see if there's
additional reviewer input.
David Hildenbrand Oct. 21, 2022, 10:18 a.m. UTC | #4
On 19.10.22 00:54, Andrew Morton wrote:
> On Tue, 18 Oct 2022 09:00:22 +0000 xu xin <xu.xin.sc@gmail.com> wrote:
> 
>>> A full description of the real-world end-user operational benefits of
>>> these changes would help, please.
>>>
>>
>> The core idea of this patch set is to enable users to perceive the number of any
>> pages merged by KSM, regardless of whether use_zero_page switch has been turned
>> on, so that users can know how much free memory increase is really due to their
>> madvise(MERGEABLE) actions.
> 
> OK, thanks.
> 
>> The motivation for me to do this is that when I do
>> an application optimization of KSM on embedded Linux for 5G platform, I find
>> that ksm_merging_pages of some process becomes very small(but used to be large),
>> which led me to think that there was any problem with the application KSM-madvise
>> strategy, but in fact, it was only because use_zero_pages is on.
> 
> Please expand on the above motivation and experience, and include it in
> the [0/n] changelog.  But let's leave it a few days to see if there's
> additional reviewer input.
> 

I just posted a selftest:

https://lore.kernel.org/all/20221021101141.84170-5-david@redhat.com/T/#u

That could (should) be extended to test if unmerging works as expected.


Having that said, I think we really want a second pair of (KSM-expert) 
eyes on these changes before moving forward with them.
xu xin Oct. 24, 2022, 3:07 a.m. UTC | #5
>
>>>> A full description of the real-world end-user operational benefits of
>>>> these changes would help, please.
>>>>
>>>
>>> The core idea of this patch set is to enable users to perceive the number of any
>>> pages merged by KSM, regardless of whether use_zero_page switch has been turned
>>> on, so that users can know how much free memory increase is really due to their
>>> madvise(MERGEABLE) actions.
>> 
>> OK, thanks.
>> 
>>> The motivation for me to do this is that when I do
>>> an application optimization of KSM on embedded Linux for 5G platform, I find
>>> that ksm_merging_pages of some process becomes very small(but used to be large),
>>> which led me to think that there was any problem with the application KSM-madvise
>>> strategy, but in fact, it was only because use_zero_pages is on.
>> 
>> Please expand on the above motivation and experience, and include it in
>> the [0/n] changelog.  But let's leave it a few days to see if there's
>> additional reviewer input.
>> 
>
>I just posted a selftest:
>
>https://lore.kernel.org/all/20221021101141.84170-5-david@redhat.com/T/#u
>
>That could (should) be extended to test if unmerging works as expected.
>

Yes. As you said, these selftests can be extended to test if unsharing KSM-placed
zero pages works as expected, and I'm happy to do the extending after they are merged.

>
>Having that said, I think we really want a second pair of (KSM-expert) 
>eyes on these changes before moving forward with them.

OK, don't worry. Let it be reviewed for a more time, so as to absorb more views later.
If necessary, I will resend the patches to adjust to break_ksm()'s changes.