[PATCH 00/10] use folio in ksm

[PATCH 00/10] use folio in ksm

[alexs]

From: "Alex Shi (tencent)" <alexs@kernel.org>

This is the 2nd part to use folio in ksm, it saves 82kbytes in ksm.o and
passed all ksm testcases in kselftest.

KSM still split same folios to single page if they are good to be KSM,
but will skip the rest subpage checking if a folio couldn't suit for
ksm.

Alex Shi (tencent) (10):
  mm/ksm: reduce the flush action for ksm merging page
  mm/ksm: skip subpages of compound pages
  mm/ksm: use folio in try_to_merge_one_page
  mm/ksm: add identical_folio func
  mm/ksm: use folio in stable_tree_search
  mm/ksm: remove page_stable_node
  mm/ksm: use folio in unstable_tree_search_insert
  mm/ksm: use folio in try_to_merge_xx serie funcs
  mm/ksm: calc_checksum for folio
  m/ksm: use folio in ksm scan path

 include/linux/mm.h |  17 ++++
 mm/ksm.c           | 230 +++++++++++++++++++++++----------------------
 2 files changed, 135 insertions(+), 112 deletions(-)

-- 
2.43.0

[PATCH 01/10] mm/ksm: reduce the flush action for ksm merging page

[alexs]

From: "Alex Shi (tencent)" <alexs@kernel.org>

We can put off the flush action util a merging is realy coming. That
could reduce some unmerge page flushing.
BTW, flushing only do at arm, mips and few other archs.

Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
---
 mm/ksm.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index f5138f43f0d2..97e5b41f8c4b 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -783,10 +783,7 @@ static struct page *get_mergeable_page(struct ksm_rmap_item *rmap_item)
 		goto out;
 	if (is_zone_device_page(page))
 		goto out_putpage;
-	if (PageAnon(page)) {
-		flush_anon_page(vma, page, addr);
-		flush_dcache_page(page);
-	} else {
+	if (!PageAnon(page)) {
 out_putpage:
 		put_page(page);
 out:
@@ -1473,8 +1470,8 @@ static int replace_page(struct vm_area_struct *vma, struct page *page,
  *
  * This function returns 0 if the pages were merged, -EFAULT otherwise.
  */
-static int try_to_merge_one_page(struct vm_area_struct *vma,
-				 struct page *page, struct page *kpage)
+static int try_to_merge_one_page(struct vm_area_struct *vma, struct page *page,
+				 struct ksm_rmap_item *rmap_item, struct page *kpage)
 {
 	pte_t orig_pte = __pte(0);
 	int err = -EFAULT;
@@ -1500,6 +1497,9 @@ static int try_to_merge_one_page(struct vm_area_struct *vma,
 			goto out_unlock;
 	}
 
+	flush_anon_page(vma, page, rmap_item->address);
+	flush_dcache_page(page);
+
 	/*
 	 * If this anonymous page is mapped only here, its pte may need
 	 * to be write-protected.  If it's mapped elsewhere, all of its
@@ -1550,7 +1550,7 @@ static int try_to_merge_with_ksm_page(struct ksm_rmap_item *rmap_item,
 	if (!vma)
 		goto out;
 
-	err = try_to_merge_one_page(vma, page, kpage);
+	err = try_to_merge_one_page(vma, page, rmap_item, kpage);
 	if (err)
 		goto out;
 
@@ -2385,7 +2385,7 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 		mmap_read_lock(mm);
 		vma = find_mergeable_vma(mm, rmap_item->address);
 		if (vma) {
-			err = try_to_merge_one_page(vma, page,
+			err = try_to_merge_one_page(vma, page, rmap_item,
 					ZERO_PAGE(rmap_item->address));
 			trace_ksm_merge_one_page(
 				page_to_pfn(ZERO_PAGE(rmap_item->address)),
@@ -2663,8 +2663,6 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 			if (is_zone_device_page(*page))
 				goto next_page;
 			if (PageAnon(*page)) {
-				flush_anon_page(vma, *page, ksm_scan.address);
-				flush_dcache_page(*page);
 				rmap_item = get_next_rmap_item(mm_slot,
 					ksm_scan.rmap_list, ksm_scan.address);
 				if (rmap_item) {
-- 
2.43.0

Re: [PATCH 01/10] mm/ksm: reduce the flush action for ksm merging page

[David Hildenbrand]

On 04.06.24 06:24, alexs@kernel.org wrote:
> From: "Alex Shi (tencent)" <alexs@kernel.org>
> 
> We can put off the flush action util a merging is realy coming. That
> could reduce some unmerge page flushing.
> BTW, flushing only do at arm, mips and few other archs.
> 

I'm no expert on that flushing, but I thought we would have to do the 
flushing before accessing page content -- before calculating the 
checksum etc.

Now you would only do it before the pages_identical() check, but not 
when calculating the checksum.

-- 
Cheers,

David / dhildenb

Re: [PATCH 01/10] mm/ksm: reduce the flush action for ksm merging page

[Alex Shi]

On 6/4/24 4:07 PM, David Hildenbrand wrote:
> On 04.06.24 06:24, alexs@kernel.org wrote:
>> From: "Alex Shi (tencent)" <alexs@kernel.org>
>>
>> We can put off the flush action util a merging is realy coming. That
>> could reduce some unmerge page flushing.
>> BTW, flushing only do at arm, mips and few other archs.
>>
> 
> I'm no expert on that flushing, but I thought we would have to do the flushing before accessing page content -- before calculating the checksum etc.
> 
> Now you would only do it before the pages_identical() check, but not when calculating the checksum.
> 

Hi David,

Thanks a lot for comments!

If calc_checksum() is wrong before pages_idential(), (that's just after page was write_protected, that's a real guarantee for page context secured) pages_identical could recheck and make thing right. 

And as to 2 flush functions here, I didn't see the guarantee for other writer from any other place. So maybe we should remove these flush action?

Thanks
Alex

Re: [PATCH 01/10] mm/ksm: reduce the flush action for ksm merging page

[David Hildenbrand]

On 04.06.24 12:26, Alex Shi wrote:
> 
> 
> On 6/4/24 4:07 PM, David Hildenbrand wrote:
>> On 04.06.24 06:24, alexs@kernel.org wrote:
>>> From: "Alex Shi (tencent)" <alexs@kernel.org>
>>>
>>> We can put off the flush action util a merging is realy coming. That
>>> could reduce some unmerge page flushing.
>>> BTW, flushing only do at arm, mips and few other archs.
>>>
>>
>> I'm no expert on that flushing, but I thought we would have to do the flushing before accessing page content -- before calculating the checksum etc.
>>
>> Now you would only do it before the pages_identical() check, but not when calculating the checksum.
>>
> 
> Hi David,
> 
> Thanks a lot for comments!
> 
> If calc_checksum() is wrong before pages_idential(), (that's just after page was write_protected, that's a real guarantee for page context secured) pages_identical could recheck and make thing right.
> 

Yes, but you would get more wrong checksums, resulting in more 
unnecessary pages_identical() checks.

That is missing from the description, and why we want to change that 
behavior.

What's the net win?

> And as to 2 flush functions here, I didn't see the guarantee for other writer from any other place. So maybe we should remove these flush action?

"I didn't see the guarantee for other writer from any other place" can 
you rephrase your comment?

If you mean "the process could modify that page concurrently", then you 
are right. But that's different than "the process modified the page in 
the past and we are reading stale content because we missed a flush".

-- 
Cheers,

David / dhildenb

Re: [PATCH 01/10] mm/ksm: reduce the flush action for ksm merging page

[Alex Shi]

On 6/4/24 6:45 PM, David Hildenbrand wrote:
> On 04.06.24 12:26, Alex Shi wrote:
>>
>>
>> On 6/4/24 4:07 PM, David Hildenbrand wrote:
>>> On 04.06.24 06:24, alexs@kernel.org wrote:
>>>> From: "Alex Shi (tencent)" <alexs@kernel.org>
>>>>
>>>> We can put off the flush action util a merging is realy coming. That
>>>> could reduce some unmerge page flushing.
>>>> BTW, flushing only do at arm, mips and few other archs.
>>>>
>>>
>>> I'm no expert on that flushing, but I thought we would have to do the flushing before accessing page content -- before calculating the checksum etc.
>>>
>>> Now you would only do it before the pages_identical() check, but not when calculating the checksum.
>>>
>>
>> Hi David,
>>
>> Thanks a lot for comments!
>>
>> If calc_checksum() is wrong before pages_idential(), (that's just after page was write_protected, that's a real guarantee for page context secured) pages_identical could recheck and make thing right.
>>
> 
> Yes, but you would get more wrong checksums, resulting in more unnecessary pages_identical() checks.
> 
> That is missing from the description, and why we want to change that behavior.
> 
> What's the net win?
> 
>> And as to 2 flush functions here, I didn't see the guarantee for other writer from any other place. So maybe we should remove these flush action?
> 
> "I didn't see the guarantee for other writer from any other place" can you rephrase your comment?
> 
> If you mean "the process could modify that page concurrently", then you are right. But that's different than "the process modified the page in the past and we are reading stale content because we missed a flush".


Maybe moving the flush before checksum could relief some worries. :) 
But still no one knows what flush really help, since if page content only syncs to memory by the flush, the kernel or process can't be work with current code. 

thanks
Alex 

Re: [PATCH 01/10] mm/ksm: reduce the flush action for ksm merging page

[David Hildenbrand]

On 04.06.24 15:02, Alex Shi wrote:
> 
> 
> On 6/4/24 6:45 PM, David Hildenbrand wrote:
>> On 04.06.24 12:26, Alex Shi wrote:
>>>
>>>
>>> On 6/4/24 4:07 PM, David Hildenbrand wrote:
>>>> On 04.06.24 06:24, alexs@kernel.org wrote:
>>>>> From: "Alex Shi (tencent)" <alexs@kernel.org>
>>>>>
>>>>> We can put off the flush action util a merging is realy coming. That
>>>>> could reduce some unmerge page flushing.
>>>>> BTW, flushing only do at arm, mips and few other archs.
>>>>>
>>>>
>>>> I'm no expert on that flushing, but I thought we would have to do the flushing before accessing page content -- before calculating the checksum etc.
>>>>
>>>> Now you would only do it before the pages_identical() check, but not when calculating the checksum.
>>>>
>>>
>>> Hi David,
>>>
>>> Thanks a lot for comments!
>>>
>>> If calc_checksum() is wrong before pages_idential(), (that's just after page was write_protected, that's a real guarantee for page context secured) pages_identical could recheck and make thing right.
>>>
>>
>> Yes, but you would get more wrong checksums, resulting in more unnecessary pages_identical() checks.
>>
>> That is missing from the description, and why we want to change that behavior.
>>
>> What's the net win?
>>
>>> And as to 2 flush functions here, I didn't see the guarantee for other writer from any other place. So maybe we should remove these flush action?
>>
>> "I didn't see the guarantee for other writer from any other place" can you rephrase your comment?
>>
>> If you mean "the process could modify that page concurrently", then you are right. But that's different than "the process modified the page in the past and we are reading stale content because we missed a flush".
> 
> 
> Maybe moving the flush before checksum could relief some worries. :)
> But still no one knows what flush really help, since if page content only syncs to memory by the flush, the kernel or process can't be work with current code.

Please explain to me why we care about moving the flushs at all :)

If they are NOP on most architectures either way, why not simply leave 
them there and call it a day?

-- 
Cheers,

David / dhildenb

Re: [PATCH 01/10] mm/ksm: reduce the flush action for ksm merging page

[Alex Shi]

On 6/5/24 3:26 PM, David Hildenbrand wrote:
> On 04.06.24 15:02, Alex Shi wrote:
>>
>>
>> On 6/4/24 6:45 PM, David Hildenbrand wrote:
>>> On 04.06.24 12:26, Alex Shi wrote:
>>>>
>>>>
>>>> On 6/4/24 4:07 PM, David Hildenbrand wrote:
>>>>> On 04.06.24 06:24, alexs@kernel.org wrote:
>>>>>> From: "Alex Shi (tencent)" <alexs@kernel.org>
>>>>>>
>>>>>> We can put off the flush action util a merging is realy coming. That
>>>>>> could reduce some unmerge page flushing.
>>>>>> BTW, flushing only do at arm, mips and few other archs.
>>>>>>
>>>>>
>>>>> I'm no expert on that flushing, but I thought we would have to do the flushing before accessing page content -- before calculating the checksum etc.
>>>>>
>>>>> Now you would only do it before the pages_identical() check, but not when calculating the checksum.
>>>>>
>>>>
>>>> Hi David,
>>>>
>>>> Thanks a lot for comments!
>>>>
>>>> If calc_checksum() is wrong before pages_idential(), (that's just after page was write_protected, that's a real guarantee for page context secured) pages_identical could recheck and make thing right.
>>>>
>>>
>>> Yes, but you would get more wrong checksums, resulting in more unnecessary pages_identical() checks.
>>>
>>> That is missing from the description, and why we want to change that behavior.
>>>
>>> What's the net win?
>>>
>>>> And as to 2 flush functions here, I didn't see the guarantee for other writer from any other place. So maybe we should remove these flush action?
>>>
>>> "I didn't see the guarantee for other writer from any other place" can you rephrase your comment?
>>>
>>> If you mean "the process could modify that page concurrently", then you are right. But that's different than "the process modified the page in the past and we are reading stale content because we missed a flush".
>>
>>
>> Maybe moving the flush before checksum could relief some worries. :)
>> But still no one knows what flush really help, since if page content only syncs to memory by the flush, the kernel or process can't be work with current code.
> 
> Please explain to me why we care about moving the flushs at all :)
> 
> If they are NOP on most architectures either way, why not simply leave them there and call it a day?
Uh, 2 reasons:
1, it uses page and can't convert to folio now.
2, as you pointed, flush action w/o page reading seems just waste time.

Thanks
Alex
> 

Re: [PATCH 01/10] mm/ksm: reduce the flush action for ksm merging page

[David Hildenbrand]

On 05.06.24 11:10, Alex Shi wrote:
> 
> 
> On 6/5/24 3:26 PM, David Hildenbrand wrote:
>> On 04.06.24 15:02, Alex Shi wrote:
>>>
>>>
>>> On 6/4/24 6:45 PM, David Hildenbrand wrote:
>>>> On 04.06.24 12:26, Alex Shi wrote:
>>>>>
>>>>>
>>>>> On 6/4/24 4:07 PM, David Hildenbrand wrote:
>>>>>> On 04.06.24 06:24, alexs@kernel.org wrote:
>>>>>>> From: "Alex Shi (tencent)" <alexs@kernel.org>
>>>>>>>
>>>>>>> We can put off the flush action util a merging is realy coming. That
>>>>>>> could reduce some unmerge page flushing.
>>>>>>> BTW, flushing only do at arm, mips and few other archs.
>>>>>>>
>>>>>>
>>>>>> I'm no expert on that flushing, but I thought we would have to do the flushing before accessing page content -- before calculating the checksum etc.
>>>>>>
>>>>>> Now you would only do it before the pages_identical() check, but not when calculating the checksum.
>>>>>>
>>>>>
>>>>> Hi David,
>>>>>
>>>>> Thanks a lot for comments!
>>>>>
>>>>> If calc_checksum() is wrong before pages_idential(), (that's just after page was write_protected, that's a real guarantee for page context secured) pages_identical could recheck and make thing right.
>>>>>
>>>>
>>>> Yes, but you would get more wrong checksums, resulting in more unnecessary pages_identical() checks.
>>>>
>>>> That is missing from the description, and why we want to change that behavior.
>>>>
>>>> What's the net win?
>>>>
>>>>> And as to 2 flush functions here, I didn't see the guarantee for other writer from any other place. So maybe we should remove these flush action?
>>>>
>>>> "I didn't see the guarantee for other writer from any other place" can you rephrase your comment?
>>>>
>>>> If you mean "the process could modify that page concurrently", then you are right. But that's different than "the process modified the page in the past and we are reading stale content because we missed a flush".
>>>
>>>
>>> Maybe moving the flush before checksum could relief some worries. :)
>>> But still no one knows what flush really help, since if page content only syncs to memory by the flush, the kernel or process can't be work with current code.
>>
>> Please explain to me why we care about moving the flushs at all :)
>>
>> If they are NOP on most architectures either way, why not simply leave them there and call it a day?
> Uh, 2 reasons:
> 1, it uses page and can't convert to folio now.
> 2, as you pointed, flush action w/o page reading seems just waste time.

Alex, I don't think the approach you take for coming up with the current 
set of patches is a good idea.

Please reconsider what you can actually convert to folios and what must 
stay pages for now due to support for large folios in that code.

Then, please explain properly why changes are required and why they are 
safe.

For example, for in scan_get_next_rmap_item() we really *need* the page 
and not just the folio. So just leave the flushing there and be done 
with it.

-- 
Cheers,

David / dhildenb

Re: [PATCH 01/10] mm/ksm: reduce the flush action for ksm merging page

[Alex Shi]

On 6/5/24 5:14 PM, David Hildenbrand wrote:
> On 05.06.24 11:10, Alex Shi wrote:
>>
>>
>> On 6/5/24 3:26 PM, David Hildenbrand wrote:
>>> On 04.06.24 15:02, Alex Shi wrote:
>>>>
>>>>
>>>> On 6/4/24 6:45 PM, David Hildenbrand wrote:
>>>>> On 04.06.24 12:26, Alex Shi wrote:
>>>>>>
>>>>>>
>>>>>> On 6/4/24 4:07 PM, David Hildenbrand wrote:
>>>>>>> On 04.06.24 06:24, alexs@kernel.org wrote:
>>>>>>>> From: "Alex Shi (tencent)" <alexs@kernel.org>
>>>>>>>>
>>>>>>>> We can put off the flush action util a merging is realy coming. That
>>>>>>>> could reduce some unmerge page flushing.
>>>>>>>> BTW, flushing only do at arm, mips and few other archs.
>>>>>>>>
>>>>>>>
>>>>>>> I'm no expert on that flushing, but I thought we would have to do the flushing before accessing page content -- before calculating the checksum etc.
>>>>>>>
>>>>>>> Now you would only do it before the pages_identical() check, but not when calculating the checksum.
>>>>>>>
>>>>>>
>>>>>> Hi David,
>>>>>>
>>>>>> Thanks a lot for comments!
>>>>>>
>>>>>> If calc_checksum() is wrong before pages_idential(), (that's just after page was write_protected, that's a real guarantee for page context secured) pages_identical could recheck and make thing right.
>>>>>>
>>>>>
>>>>> Yes, but you would get more wrong checksums, resulting in more unnecessary pages_identical() checks.
>>>>>
>>>>> That is missing from the description, and why we want to change that behavior.
>>>>>
>>>>> What's the net win?
>>>>>
>>>>>> And as to 2 flush functions here, I didn't see the guarantee for other writer from any other place. So maybe we should remove these flush action?
>>>>>
>>>>> "I didn't see the guarantee for other writer from any other place" can you rephrase your comment?
>>>>>
>>>>> If you mean "the process could modify that page concurrently", then you are right. But that's different than "the process modified the page in the past and we are reading stale content because we missed a flush".
>>>>
>>>>
>>>> Maybe moving the flush before checksum could relief some worries. :)
>>>> But still no one knows what flush really help, since if page content only syncs to memory by the flush, the kernel or process can't be work with current code.
>>>
>>> Please explain to me why we care about moving the flushs at all :)
>>>
>>> If they are NOP on most architectures either way, why not simply leave them there and call it a day?
>> Uh, 2 reasons:
>> 1, it uses page and can't convert to folio now.
>> 2, as you pointed, flush action w/o page reading seems just waste time.
> 
> Alex, I don't think the approach you take for coming up with the current set of patches is a good idea.
> 
> Please reconsider what you can actually convert to folios and what must stay pages for now due to support for large folios in that code.
> 
> Then, please explain properly why changes are required and why they are safe.
> 
> For example, for in scan_get_next_rmap_item() we really *need* the page and not just the folio. So just leave the flushing there and be done with it.
> 

Hi David,

Thanks a lot for your review.
Though all patches are passed in kernel selftest, but if we do care the saving more than quick processing, the main purpose of this patchset is gone. I'll drop this series.

Thanks
Alex

Re: [PATCH 01/10] mm/ksm: reduce the flush action for ksm merging page

[David Hildenbrand]

On 05.06.24 11:49, Alex Shi wrote:
> 
> 
> On 6/5/24 5:14 PM, David Hildenbrand wrote:
>> On 05.06.24 11:10, Alex Shi wrote:
>>>
>>>
>>> On 6/5/24 3:26 PM, David Hildenbrand wrote:
>>>> On 04.06.24 15:02, Alex Shi wrote:
>>>>>
>>>>>
>>>>> On 6/4/24 6:45 PM, David Hildenbrand wrote:
>>>>>> On 04.06.24 12:26, Alex Shi wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 6/4/24 4:07 PM, David Hildenbrand wrote:
>>>>>>>> On 04.06.24 06:24, alexs@kernel.org wrote:
>>>>>>>>> From: "Alex Shi (tencent)" <alexs@kernel.org>
>>>>>>>>>
>>>>>>>>> We can put off the flush action util a merging is realy coming. That
>>>>>>>>> could reduce some unmerge page flushing.
>>>>>>>>> BTW, flushing only do at arm, mips and few other archs.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I'm no expert on that flushing, but I thought we would have to do the flushing before accessing page content -- before calculating the checksum etc.
>>>>>>>>
>>>>>>>> Now you would only do it before the pages_identical() check, but not when calculating the checksum.
>>>>>>>>
>>>>>>>
>>>>>>> Hi David,
>>>>>>>
>>>>>>> Thanks a lot for comments!
>>>>>>>
>>>>>>> If calc_checksum() is wrong before pages_idential(), (that's just after page was write_protected, that's a real guarantee for page context secured) pages_identical could recheck and make thing right.
>>>>>>>
>>>>>>
>>>>>> Yes, but you would get more wrong checksums, resulting in more unnecessary pages_identical() checks.
>>>>>>
>>>>>> That is missing from the description, and why we want to change that behavior.
>>>>>>
>>>>>> What's the net win?
>>>>>>
>>>>>>> And as to 2 flush functions here, I didn't see the guarantee for other writer from any other place. So maybe we should remove these flush action?
>>>>>>
>>>>>> "I didn't see the guarantee for other writer from any other place" can you rephrase your comment?
>>>>>>
>>>>>> If you mean "the process could modify that page concurrently", then you are right. But that's different than "the process modified the page in the past and we are reading stale content because we missed a flush".
>>>>>
>>>>>
>>>>> Maybe moving the flush before checksum could relief some worries. :)
>>>>> But still no one knows what flush really help, since if page content only syncs to memory by the flush, the kernel or process can't be work with current code.
>>>>
>>>> Please explain to me why we care about moving the flushs at all :)
>>>>
>>>> If they are NOP on most architectures either way, why not simply leave them there and call it a day?
>>> Uh, 2 reasons:
>>> 1, it uses page and can't convert to folio now.
>>> 2, as you pointed, flush action w/o page reading seems just waste time.
>>
>> Alex, I don't think the approach you take for coming up with the current set of patches is a good idea.
>>
>> Please reconsider what you can actually convert to folios and what must stay pages for now due to support for large folios in that code.
>>
>> Then, please explain properly why changes are required and why they are safe.
>>
>> For example, for in scan_get_next_rmap_item() we really *need* the page and not just the folio. So just leave the flushing there and be done with it.
>>
> 
> Hi David,
> 
> Thanks a lot for your review.
> Though all patches are passed in kernel selftest, but if we do care the saving more than quick processing, the main purpose of this patchset is gone. I'll drop this series.

I think there is value in more folio conversion, but we really have to 
be careful when we still want to/have to work on pages (folio+page pair).

-- 
Cheers,

David / dhildenb

[PATCH 02/10] mm/ksm: skip subpages of compound pages

[alexs]

From: "Alex Shi (tencent)" <alexs@kernel.org>

When a folio isn't fit for KSM, the subpages are unlikely to be good,
So let's skip the rest page checking to save some actions.

Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
---
 mm/ksm.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 97e5b41f8c4b..e2fdb9dd98e2 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2644,6 +2644,8 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 		goto no_vmas;
 
 	for_each_vma(vmi, vma) {
+		int nr = 1;
+
 		if (!(vma->vm_flags & VM_MERGEABLE))
 			continue;
 		if (ksm_scan.address < vma->vm_start)
@@ -2660,6 +2662,9 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 				cond_resched();
 				continue;
 			}
+
+			VM_WARN_ON(PageTail(*page));
+			nr = compound_nr(*page);
 			if (is_zone_device_page(*page))
 				goto next_page;
 			if (PageAnon(*page)) {
@@ -2672,7 +2677,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 					if (should_skip_rmap_item(*page, rmap_item))
 						goto next_page;
 
-					ksm_scan.address += PAGE_SIZE;
+					ksm_scan.address += nr * PAGE_SIZE;
 				} else
 					put_page(*page);
 				mmap_read_unlock(mm);
@@ -2680,7 +2685,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 			}
 next_page:
 			put_page(*page);
-			ksm_scan.address += PAGE_SIZE;
+			ksm_scan.address += nr * PAGE_SIZE;
 			cond_resched();
 		}
 	}
-- 
2.43.0

Re: [PATCH 02/10] mm/ksm: skip subpages of compound pages

[David Hildenbrand]

On 04.06.24 06:24, alexs@kernel.org wrote:
> From: "Alex Shi (tencent)" <alexs@kernel.org>
> 
> When a folio isn't fit for KSM, the subpages are unlikely to be good,
> So let's skip the rest page checking to save some actions.
> 
> Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
> ---
>   mm/ksm.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 97e5b41f8c4b..e2fdb9dd98e2 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -2644,6 +2644,8 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>   		goto no_vmas;
>   
>   	for_each_vma(vmi, vma) {
> +		int nr = 1;
> +
>   		if (!(vma->vm_flags & VM_MERGEABLE))
>   			continue;
>   		if (ksm_scan.address < vma->vm_start)
> @@ -2660,6 +2662,9 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>   				cond_resched();
>   				continue;
>   			}
> +
> +			VM_WARN_ON(PageTail(*page));
> +			nr = compound_nr(*page);
>   			if (is_zone_device_page(*page))
>   				goto next_page;
>   			if (PageAnon(*page)) {
> @@ -2672,7 +2677,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>   					if (should_skip_rmap_item(*page, rmap_item))
>   						goto next_page;
>   
> -					ksm_scan.address += PAGE_SIZE;
> +					ksm_scan.address += nr * PAGE_SIZE;
>   				} else
>   					put_page(*page);
>   				mmap_read_unlock(mm);
> @@ -2680,7 +2685,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>   			}
>   next_page:
>   			put_page(*page);
> -			ksm_scan.address += PAGE_SIZE;
> +			ksm_scan.address += nr * PAGE_SIZE;
>   			cond_resched();
>   		}
>   	}

You might be jumping over pages that don't belong to that folio. What 
you would actually want to do is somehow use folio_pte_batch() to really 
know the PTEs point at the same folio, so you can skip them. But that's 
not that easy when using follow_page() ...

So I suggest dropping this change for now.

-- 
Cheers,

David / dhildenb

Re: [PATCH 02/10] mm/ksm: skip subpages of compound pages

[Alex Shi]

On 6/4/24 4:12 PM, David Hildenbrand wrote:
> On 04.06.24 06:24, alexs@kernel.org wrote:
>> From: "Alex Shi (tencent)" <alexs@kernel.org>
>>
>> When a folio isn't fit for KSM, the subpages are unlikely to be good,
>> So let's skip the rest page checking to save some actions.
>>
>> Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
>> ---
>>   mm/ksm.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/ksm.c b/mm/ksm.c
>> index 97e5b41f8c4b..e2fdb9dd98e2 100644
>> --- a/mm/ksm.c
>> +++ b/mm/ksm.c
>> @@ -2644,6 +2644,8 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>>           goto no_vmas;
>>         for_each_vma(vmi, vma) {
>> +        int nr = 1;
>> +
>>           if (!(vma->vm_flags & VM_MERGEABLE))
>>               continue;
>>           if (ksm_scan.address < vma->vm_start)
>> @@ -2660,6 +2662,9 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>>                   cond_resched();
>>                   continue;
>>               }
>> +
>> +            VM_WARN_ON(PageTail(*page));
>> +            nr = compound_nr(*page);
>>               if (is_zone_device_page(*page))
>>                   goto next_page;
>>               if (PageAnon(*page)) {
>> @@ -2672,7 +2677,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>>                       if (should_skip_rmap_item(*page, rmap_item))
>>                           goto next_page;
>>   -                    ksm_scan.address += PAGE_SIZE;
>> +                    ksm_scan.address += nr * PAGE_SIZE;
>>                   } else
>>                       put_page(*page);
>>                   mmap_read_unlock(mm);
>> @@ -2680,7 +2685,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>>               }
>>   next_page:
>>               put_page(*page);
>> -            ksm_scan.address += PAGE_SIZE;
>> +            ksm_scan.address += nr * PAGE_SIZE;
>>               cond_resched();
>>           }
>>       }
> 
> You might be jumping over pages that don't belong to that folio. What you would actually want to do is somehow use folio_pte_batch() to really know the PTEs point at the same folio, so you can skip them. But that's not that easy when using follow_page() ...
> 
> So I suggest dropping this change for now.
> 

Hi David,

Forgive my stupidity, where I jump over normal page that not to belong to the folio?

Thanks
Alex

Re: [PATCH 02/10] mm/ksm: skip subpages of compound pages

[David Hildenbrand]

On 04.06.24 12:31, Alex Shi wrote:
> 
> 
> On 6/4/24 4:12 PM, David Hildenbrand wrote:
>> On 04.06.24 06:24, alexs@kernel.org wrote:
>>> From: "Alex Shi (tencent)" <alexs@kernel.org>
>>>
>>> When a folio isn't fit for KSM, the subpages are unlikely to be good,
>>> So let's skip the rest page checking to save some actions.
>>>
>>> Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
>>> ---
>>>    mm/ksm.c | 9 +++++++--
>>>    1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/ksm.c b/mm/ksm.c
>>> index 97e5b41f8c4b..e2fdb9dd98e2 100644
>>> --- a/mm/ksm.c
>>> +++ b/mm/ksm.c
>>> @@ -2644,6 +2644,8 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>>>            goto no_vmas;
>>>          for_each_vma(vmi, vma) {
>>> +        int nr = 1;
>>> +
>>>            if (!(vma->vm_flags & VM_MERGEABLE))
>>>                continue;
>>>            if (ksm_scan.address < vma->vm_start)
>>> @@ -2660,6 +2662,9 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>>>                    cond_resched();
>>>                    continue;
>>>                }
>>> +
>>> +            VM_WARN_ON(PageTail(*page));
>>> +            nr = compound_nr(*page);
>>>                if (is_zone_device_page(*page))
>>>                    goto next_page;
>>>                if (PageAnon(*page)) {
>>> @@ -2672,7 +2677,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>>>                        if (should_skip_rmap_item(*page, rmap_item))
>>>                            goto next_page;
>>>    -                    ksm_scan.address += PAGE_SIZE;
>>> +                    ksm_scan.address += nr * PAGE_SIZE;
>>>                    } else
>>>                        put_page(*page);
>>>                    mmap_read_unlock(mm);
>>> @@ -2680,7 +2685,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>>>                }
>>>    next_page:
>>>                put_page(*page);
>>> -            ksm_scan.address += PAGE_SIZE;
>>> +            ksm_scan.address += nr * PAGE_SIZE;
>>>                cond_resched();
>>>            }
>>>        }
>>
>> You might be jumping over pages that don't belong to that folio. What you would actually want to do is somehow use folio_pte_batch() to really know the PTEs point at the same folio, so you can skip them. But that's not that easy when using follow_page() ...
>>
>> So I suggest dropping this change for now.
>>
> 
> Hi David,
> 
> Forgive my stupidity, where I jump over normal page that not to belong to the folio?

IIUC, you assume that the folio is fully mapped by all PTEs that could 
span it, and that follow_page() would give you the head page, correct?

As a simple example, assume only a single page of a large folio is still 
mapped, which could be any tail page. You couldn't jump over any PTEs.

Or am I missing something?

-- 
Cheers,

David / dhildenb

Re: [PATCH 02/10] mm/ksm: skip subpages of compound pages

[Alex Shi]

On 6/4/24 6:43 PM, David Hildenbrand wrote:
>>>>
>>>> @@ -2680,7 +2685,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>>>>                }
>>>>    next_page:
>>>>                put_page(*page);
>>>> -            ksm_scan.address += PAGE_SIZE;
>>>> +            ksm_scan.address += nr * PAGE_SIZE;
>>>>                cond_resched();
>>>>            }
>>>>        }
>>>
>>> You might be jumping over pages that don't belong to that folio. What you would actually want to do is somehow use folio_pte_batch() to really know the PTEs point at the same folio, so you can skip them. But that's not that easy when using follow_page() ...
>>>
>>> So I suggest dropping this change for now.
>>>
>>
>> Hi David,
>>
>> Forgive my stupidity, where I jump over normal page that not to belong to the folio?
> 
> IIUC, you assume that the folio is fully mapped by all PTEs that could span it, and that follow_page() would give you the head page, correct?
> 
> As a simple example, assume only a single page of a large folio is still mapped, which could be any tail page. You couldn't jump over any PTEs.
> 
> Or am I missing something?

Uh, thanks for explanations. for what's we concerned, the following code could take care of the FULL or ERR pages. And it still keep the step of single page.  
                        page = follow_page(vma, ksm_scan.address, FOLL_GET);
                        if (IS_ERR_OR_NULL(page)) { 
                                ksm_scan.address += PAGE_SIZE;
                                cond_resched();
                                continue;
                        }
And after the above code, step folio_nr_pages on address should be safe, isn't it?

Thanks a lot
Alex

Re: [PATCH 02/10] mm/ksm: skip subpages of compound pages

[David Hildenbrand]

On 04.06.24 15:10, Alex Shi wrote:
> 
> 
> On 6/4/24 6:43 PM, David Hildenbrand wrote:
>>>>>
>>>>> @@ -2680,7 +2685,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>>>>>                 }
>>>>>     next_page:
>>>>>                 put_page(*page);
>>>>> -            ksm_scan.address += PAGE_SIZE;
>>>>> +            ksm_scan.address += nr * PAGE_SIZE;
>>>>>                 cond_resched();
>>>>>             }
>>>>>         }
>>>>
>>>> You might be jumping over pages that don't belong to that folio. What you would actually want to do is somehow use folio_pte_batch() to really know the PTEs point at the same folio, so you can skip them. But that's not that easy when using follow_page() ...
>>>>
>>>> So I suggest dropping this change for now.
>>>>
>>>
>>> Hi David,
>>>
>>> Forgive my stupidity, where I jump over normal page that not to belong to the folio?
>>
>> IIUC, you assume that the folio is fully mapped by all PTEs that could span it, and that follow_page() would give you the head page, correct?
>>
>> As a simple example, assume only a single page of a large folio is still mapped, which could be any tail page. You couldn't jump over any PTEs.
>>
>> Or am I missing something?
> 
> Uh, thanks for explanations. for what's we concerned, the following code could take care of the FULL or ERR pages. And it still keep the step of single page.
>                          page = follow_page(vma, ksm_scan.address, FOLL_GET);
>                          if (IS_ERR_OR_NULL(page)) {
>                                  ksm_scan.address += PAGE_SIZE;
>                                  cond_resched();
>                                  continue;
>                          }
> And after the above code, step folio_nr_pages on address should be safe, isn't it?

Not sure if I follow. Let me try explaining once again:

Assume a PTE maps some tail page of the large anonymous folio. The other 
PTEs around it map some other anonymous folios, not pages of that large 
anonymous folio.

Without looking at the other PTEs you don't know how much you can skip.

-- 
Cheers,

David / dhildenb

Re: [PATCH 02/10] mm/ksm: skip subpages of compound pages

[Alex Shi]

On 6/4/24 9:14 PM, David Hildenbrand wrote:
> On 04.06.24 15:10, Alex Shi wrote:
>>
>>
>> On 6/4/24 6:43 PM, David Hildenbrand wrote:
>>>>>>
>>>>>> @@ -2680,7 +2685,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>>>>>>                 }
>>>>>>     next_page:
>>>>>>                 put_page(*page);
>>>>>> -            ksm_scan.address += PAGE_SIZE;
>>>>>> +            ksm_scan.address += nr * PAGE_SIZE;
>>>>>>                 cond_resched();
>>>>>>             }
>>>>>>         }
>>>>>
>>>>> You might be jumping over pages that don't belong to that folio. What you would actually want to do is somehow use folio_pte_batch() to really know the PTEs point at the same folio, so you can skip them. But that's not that easy when using follow_page() ...
>>>>>
>>>>> So I suggest dropping this change for now.
>>>>>
>>>>
>>>> Hi David,
>>>>
>>>> Forgive my stupidity, where I jump over normal page that not to belong to the folio?
>>>
>>> IIUC, you assume that the folio is fully mapped by all PTEs that could span it, and that follow_page() would give you the head page, correct?
>>>
>>> As a simple example, assume only a single page of a large folio is still mapped, which could be any tail page. You couldn't jump over any PTEs.
>>>
>>> Or am I missing something?
>>
>> Uh, thanks for explanations. for what's we concerned, the following code could take care of the FULL or ERR pages. And it still keep the step of single page.
>>                          page = follow_page(vma, ksm_scan.address, FOLL_GET);
>>                          if (IS_ERR_OR_NULL(page)) {
>>                                  ksm_scan.address += PAGE_SIZE;
>>                                  cond_resched();
>>                                  continue;
>>                          }
>> And after the above code, step folio_nr_pages on address should be safe, isn't it?
> 
> Not sure if I follow. Let me try explaining once again:
> 
> Assume a PTE maps some tail page of the large anonymous folio. The other PTEs around it map some other anonymous folios, not pages of that large anonymous folio.


Sorry, David,

Do you meaning there are 2 folio pages, in a same vma, in their address, 'ksm_scan.address', would be overlapped in a folio size space?
If so, that do out of my expect. I do have no idea of this thing. Could you give me more hints of this problem or how things work on it in current kernel?

Thanks a lot!
Alex
 
> 
> Without looking at the other PTEs you don't know how much you can skip.
 
> 

Re: [PATCH 02/10] mm/ksm: skip subpages of compound pages

[David Hildenbrand]

On 05.06.24 05:58, Alex Shi wrote:
> 
> 
> On 6/4/24 9:14 PM, David Hildenbrand wrote:
>> On 04.06.24 15:10, Alex Shi wrote:
>>>
>>>
>>> On 6/4/24 6:43 PM, David Hildenbrand wrote:
>>>>>>>
>>>>>>> @@ -2680,7 +2685,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>>>>>>>                  }
>>>>>>>      next_page:
>>>>>>>                  put_page(*page);
>>>>>>> -            ksm_scan.address += PAGE_SIZE;
>>>>>>> +            ksm_scan.address += nr * PAGE_SIZE;
>>>>>>>                  cond_resched();
>>>>>>>              }
>>>>>>>          }
>>>>>>
>>>>>> You might be jumping over pages that don't belong to that folio. What you would actually want to do is somehow use folio_pte_batch() to really know the PTEs point at the same folio, so you can skip them. But that's not that easy when using follow_page() ...
>>>>>>
>>>>>> So I suggest dropping this change for now.
>>>>>>
>>>>>
>>>>> Hi David,
>>>>>
>>>>> Forgive my stupidity, where I jump over normal page that not to belong to the folio?
>>>>
>>>> IIUC, you assume that the folio is fully mapped by all PTEs that could span it, and that follow_page() would give you the head page, correct?
>>>>
>>>> As a simple example, assume only a single page of a large folio is still mapped, which could be any tail page. You couldn't jump over any PTEs.
>>>>
>>>> Or am I missing something?
>>>
>>> Uh, thanks for explanations. for what's we concerned, the following code could take care of the FULL or ERR pages. And it still keep the step of single page.
>>>                           page = follow_page(vma, ksm_scan.address, FOLL_GET);
>>>                           if (IS_ERR_OR_NULL(page)) {
>>>                                   ksm_scan.address += PAGE_SIZE;
>>>                                   cond_resched();
>>>                                   continue;
>>>                           }
>>> And after the above code, step folio_nr_pages on address should be safe, isn't it?
>>
>> Not sure if I follow. Let me try explaining once again:
>>
>> Assume a PTE maps some tail page of the large anonymous folio. The other PTEs around it map some other anonymous folios, not pages of that large anonymous folio.
> 
> 
> Sorry, David,
> 
> Do you meaning there are 2 folio pages, in a same vma, in their address, 'ksm_scan.address', would be overlapped in a folio size space?
> If so, that do out of my expect. I do have no idea of this thing. Could you give me more hints of this problem or how things work on it in current kernel?

We do fully support partially mapping of THPs/large folios. That means, 
you could map a single page of a large pagecache folio and the other 
PTEs could map anonymous folios (due to COW).

Simply because follow_page() returned a page of a large folio doesn't 
generally say that the PTEs around it map the same large folio.

-- 
Cheers,

David / dhildenb

Re: [PATCH 02/10] mm/ksm: skip subpages of compound pages

[Matthew Wilcox]

On Tue, Jun 04, 2024 at 12:24:44PM +0800, alexs@kernel.org wrote:
> From: "Alex Shi (tencent)" <alexs@kernel.org>
> 
> When a folio isn't fit for KSM, the subpages are unlikely to be good,
> So let's skip the rest page checking to save some actions.

Why would you say that is true?  We have plenty of evidence that
userspace allocators can allocate large folios, then use only the first
few bytes, leaving many tail pages full of zeroes.

> @@ -2660,6 +2662,9 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>  				cond_resched();
>  				continue;
>  			}
> +
> +			VM_WARN_ON(PageTail(*page));
> +			nr = compound_nr(*page);

And this is simply wrong.  *page can obviously be a tail page.

Re: [PATCH 02/10] mm/ksm: skip subpages of compound pages

[Alex Shi]

On 6/5/24 11:52 AM, Matthew Wilcox wrote:
> On Tue, Jun 04, 2024 at 12:24:44PM +0800, alexs@kernel.org wrote:
>> From: "Alex Shi (tencent)" <alexs@kernel.org>
>>
>> When a folio isn't fit for KSM, the subpages are unlikely to be good,
>> So let's skip the rest page checking to save some actions.
> 
> Why would you say that is true?  We have plenty of evidence that
> userspace allocators can allocate large folios, then use only the first
> few bytes, leaving many tail pages full of zeroes.

Um, that do need tail pages... 
Is there some way to use more folio in ksm?

> 
>> @@ -2660,6 +2662,9 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>>  				cond_resched();
>>  				continue;
>>  			}
>> +
>> +			VM_WARN_ON(PageTail(*page));
>> +			nr = compound_nr(*page);
> 
> And this is simply wrong.  *page can obviously be a tail page.
> 

Got it. Thanks a lot!
Alex

Re: [PATCH 02/10] mm/ksm: skip subpages of compound pages

[David Hildenbrand]

On 05.06.24 08:14, Alex Shi wrote:
> 
> 
> On 6/5/24 11:52 AM, Matthew Wilcox wrote:
>> On Tue, Jun 04, 2024 at 12:24:44PM +0800, alexs@kernel.org wrote:
>>> From: "Alex Shi (tencent)" <alexs@kernel.org>
>>>
>>> When a folio isn't fit for KSM, the subpages are unlikely to be good,
>>> So let's skip the rest page checking to save some actions.
>>
>> Why would you say that is true?  We have plenty of evidence that
>> userspace allocators can allocate large folios, then use only the first
>> few bytes, leaving many tail pages full of zeroes.
> 
> Um, that do need tail pages...
> Is there some way to use more folio in ksm?

My take, and Willy can correct me if I am wrong:

"struct page" is not going to away any time soon, but it might shrink at 
some point.

That is, we can use the "struct page" pointer to point at a page frame, 
and use "struct folio" to lookup/manage the metadata.

That is, use "struct page" when accessing the actual memory content 
(checksum, testing for identical content), but use the folio part when 
looking up metadata (folio_test_anon() etc). In the future we might want 
to replace the "struct page" pointer by an index into the folio, but 
that doesn't have to happen right now.

For KSM, that would mean that you have a folio+page (late folio+index) 
pair when possibly dealing with large folios, but you can use a folio 
without a page when dealing with KSM folios, that are always small.

-- 
Cheers,

David / dhildenb

Re: [PATCH 02/10] mm/ksm: skip subpages of compound pages

[Matthew Wilcox]

On Wed, Jun 05, 2024 at 09:47:10AM +0200, David Hildenbrand wrote:
> On 05.06.24 08:14, Alex Shi wrote:
> > 
> > 
> > On 6/5/24 11:52 AM, Matthew Wilcox wrote:
> > > On Tue, Jun 04, 2024 at 12:24:44PM +0800, alexs@kernel.org wrote:
> > > > From: "Alex Shi (tencent)" <alexs@kernel.org>
> > > > 
> > > > When a folio isn't fit for KSM, the subpages are unlikely to be good,
> > > > So let's skip the rest page checking to save some actions.
> > > 
> > > Why would you say that is true?  We have plenty of evidence that
> > > userspace allocators can allocate large folios, then use only the first
> > > few bytes, leaving many tail pages full of zeroes.
> > 
> > Um, that do need tail pages...
> > Is there some way to use more folio in ksm?
> 
> My take, and Willy can correct me if I am wrong:
> 
> "struct page" is not going to away any time soon, but it might shrink at
> some point.
> 
> That is, we can use the "struct page" pointer to point at a page frame, and
> use "struct folio" to lookup/manage the metadata.

Right.

> That is, use "struct page" when accessing the actual memory content
> (checksum, testing for identical content), but use the folio part when
> looking up metadata (folio_test_anon() etc). In the future we might want to
> replace the "struct page" pointer by an index into the folio, but that
> doesn't have to happen right now.

My current thinking is that folio->pfn is how we know where the memory
described by the folio is.  Using an index would be memmap[folio->pfn +
index] which isn't terribly expensive, but we may as well pass around the
(folio, page) pair and save the reference to memmap.

> For KSM, that would mean that you have a folio+page (late folio+index) pair
> when possibly dealing with large folios, but you can use a folio without a
> page when dealing with KSM folios, that are always small.

Yes, agreed.

Re: [PATCH 02/10] mm/ksm: skip subpages of compound pages

[David Hildenbrand]

On 05.06.24 23:12, Matthew Wilcox wrote:
> On Wed, Jun 05, 2024 at 09:47:10AM +0200, David Hildenbrand wrote:
>> On 05.06.24 08:14, Alex Shi wrote:
>>>
>>>
>>> On 6/5/24 11:52 AM, Matthew Wilcox wrote:
>>>> On Tue, Jun 04, 2024 at 12:24:44PM +0800, alexs@kernel.org wrote:
>>>>> From: "Alex Shi (tencent)" <alexs@kernel.org>
>>>>>
>>>>> When a folio isn't fit for KSM, the subpages are unlikely to be good,
>>>>> So let's skip the rest page checking to save some actions.
>>>>
>>>> Why would you say that is true?  We have plenty of evidence that
>>>> userspace allocators can allocate large folios, then use only the first
>>>> few bytes, leaving many tail pages full of zeroes.
>>>
>>> Um, that do need tail pages...
>>> Is there some way to use more folio in ksm?
>>
>> My take, and Willy can correct me if I am wrong:
>>
>> "struct page" is not going to away any time soon, but it might shrink at
>> some point.
>>
>> That is, we can use the "struct page" pointer to point at a page frame, and
>> use "struct folio" to lookup/manage the metadata.
> 
> Right.
> 
>> That is, use "struct page" when accessing the actual memory content
>> (checksum, testing for identical content), but use the folio part when
>> looking up metadata (folio_test_anon() etc). In the future we might want to
>> replace the "struct page" pointer by an index into the folio, but that
>> doesn't have to happen right now.
> 
> My current thinking is that folio->pfn is how we know where the memory
> described by the folio is.  Using an index would be memmap[folio->pfn +
> index] which isn't terribly expensive, but we may as well pass around the
> (folio, page) pair and save the reference to memmap.

Right, as soon as the folio does not overlay the head page it's going to 
be a bit different.

A (folio,page) pair, like we use in the RMAP code, is likely the best 
option for now and gives us sufficient flexibility for the future design.

-- 
Cheers,

David / dhildenb

[PATCH 03/10] mm/ksm: use folio in try_to_merge_one_page

[alexs]

From: "Alex Shi (tencent)" <alexs@kernel.org>

scan_get_next_rmap_item() return folio actually now. So in the calling
path to try_to_merge_one_page() parameter pages are actually folios.
So let's use folio instead of of page in the function to save few
compound checking in callee functions.

The 'page' left here since flush functions still not support folios yet.

Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
---
 mm/ksm.c | 61 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 35 insertions(+), 26 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index e2fdb9dd98e2..21bfa9bfb210 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1462,24 +1462,29 @@ static int replace_page(struct vm_area_struct *vma, struct page *page,
 }
 
 /*
- * try_to_merge_one_page - take two pages and merge them into one
- * @vma: the vma that holds the pte pointing to page
- * @page: the PageAnon page that we want to replace with kpage
- * @kpage: the PageKsm page that we want to map instead of page,
- *         or NULL the first time when we want to use page as kpage.
+ * try_to_merge_one_page - take two folios and merge them into one
+ * @vma: the vma that holds the pte pointing to folio
+ * @folio: the PageAnon page that we want to replace with kfolio
+ * @kfolio: the PageKsm page that we want to map instead of folio,
+ *         or NULL the first time when we want to use folio as kfolio.
  *
- * This function returns 0 if the pages were merged, -EFAULT otherwise.
+ * This function returns 0 if the folios were merged, -EFAULT otherwise.
  */
-static int try_to_merge_one_page(struct vm_area_struct *vma, struct page *page,
-				 struct ksm_rmap_item *rmap_item, struct page *kpage)
+static int try_to_merge_one_page(struct vm_area_struct *vma, struct folio *folio,
+				 struct ksm_rmap_item *rmap_item, struct folio *kfolio)
 {
 	pte_t orig_pte = __pte(0);
 	int err = -EFAULT;
+	struct page *page = folio_page(folio, 0);
+	struct page *kpage;
 
-	if (page == kpage)			/* ksm page forked */
+	if (kfolio)
+		kpage = folio_page(kfolio, 0);
+
+	if (folio == kfolio)			/* ksm page forked */
 		return 0;
 
-	if (!PageAnon(page))
+	if (!folio_test_anon(folio))
 		goto out;
 
 	/*
@@ -1489,11 +1494,11 @@ static int try_to_merge_one_page(struct vm_area_struct *vma, struct page *page,
 	 * prefer to continue scanning and merging different pages,
 	 * then come back to this page when it is unlocked.
 	 */
-	if (!trylock_page(page))
+	if (!folio_trylock(folio))
 		goto out;
 
-	if (PageTransCompound(page)) {
-		if (split_huge_page(page))
+	if (folio_test_large(folio)) {
+		if (split_folio(folio))
 			goto out_unlock;
 	}
 
@@ -1506,35 +1511,36 @@ static int try_to_merge_one_page(struct vm_area_struct *vma, struct page *page,
 	 * ptes are necessarily already write-protected.  But in either
 	 * case, we need to lock and check page_count is not raised.
 	 */
-	if (write_protect_page(vma, page_folio(page), &orig_pte) == 0) {
-		if (!kpage) {
+	if (write_protect_page(vma, folio, &orig_pte) == 0) {
+		if (!kfolio) {
 			/*
 			 * While we hold page lock, upgrade page from
 			 * PageAnon+anon_vma to PageKsm+NULL stable_node:
 			 * stable_tree_insert() will update stable_node.
 			 */
-			folio_set_stable_node(page_folio(page), NULL);
-			mark_page_accessed(page);
+			folio_set_stable_node(folio, NULL);
+			folio_mark_accessed(folio);
 			/*
 			 * Page reclaim just frees a clean page with no dirty
 			 * ptes: make sure that the ksm page would be swapped.
 			 */
-			if (!PageDirty(page))
-				SetPageDirty(page);
+			if (!folio_test_dirty(folio))
+				folio_set_dirty(folio);
 			err = 0;
 		} else if (pages_identical(page, kpage))
 			err = replace_page(vma, page, kpage, orig_pte);
 	}
 
 out_unlock:
-	unlock_page(page);
+	folio_unlock(folio);
 out:
 	return err;
 }
 
 /*
  * try_to_merge_with_ksm_page - like try_to_merge_two_pages,
- * but no new kernel page is allocated: kpage must already be a ksm page.
+ * but no new kernel page is allocated, kpage is a ksm page or NULL
+ * if we use page as first ksm page.
  *
  * This function returns 0 if the pages were merged, -EFAULT otherwise.
  */
@@ -1544,13 +1550,17 @@ static int try_to_merge_with_ksm_page(struct ksm_rmap_item *rmap_item,
 	struct mm_struct *mm = rmap_item->mm;
 	struct vm_area_struct *vma;
 	int err = -EFAULT;
+	struct folio *kfolio;
 
 	mmap_read_lock(mm);
 	vma = find_mergeable_vma(mm, rmap_item->address);
 	if (!vma)
 		goto out;
 
-	err = try_to_merge_one_page(vma, page, rmap_item, kpage);
+	if (kpage)
+		kfolio = page_folio(kpage);
+
+	err = try_to_merge_one_page(vma, page_folio(page), rmap_item, kfolio);
 	if (err)
 		goto out;
 
@@ -2385,8 +2395,8 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 		mmap_read_lock(mm);
 		vma = find_mergeable_vma(mm, rmap_item->address);
 		if (vma) {
-			err = try_to_merge_one_page(vma, page, rmap_item,
-					ZERO_PAGE(rmap_item->address));
+			err = try_to_merge_one_page(vma, page_folio(page), rmap_item,
+						    page_folio(ZERO_PAGE(rmap_item->address)));
 			trace_ksm_merge_one_page(
 				page_to_pfn(ZERO_PAGE(rmap_item->address)),
 				rmap_item, mm, err);
@@ -2671,8 +2681,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 				rmap_item = get_next_rmap_item(mm_slot,
 					ksm_scan.rmap_list, ksm_scan.address);
 				if (rmap_item) {
-					ksm_scan.rmap_list =
-							&rmap_item->rmap_list;
+					ksm_scan.rmap_list = &rmap_item->rmap_list;
 
 					if (should_skip_rmap_item(*page, rmap_item))
 						goto next_page;
-- 
2.43.0

Re: [PATCH 03/10] mm/ksm: use folio in try_to_merge_one_page

[David Hildenbrand]

On 04.06.24 06:24, alexs@kernel.org wrote:
> From: "Alex Shi (tencent)" <alexs@kernel.org>
> 
> scan_get_next_rmap_item() return folio actually now. So in the calling
> path to try_to_merge_one_page() parameter pages are actually folios.
> So let's use folio instead of of page in the function to save few
> compound checking in callee functions.
> 
> The 'page' left here since flush functions still not support folios yet.
> 
> Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
> ---
>   mm/ksm.c | 61 ++++++++++++++++++++++++++++++++------------------------
>   1 file changed, 35 insertions(+), 26 deletions(-)
> 
> diff --git a/mm/ksm.c b/mm/ksm.c
> index e2fdb9dd98e2..21bfa9bfb210 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -1462,24 +1462,29 @@ static int replace_page(struct vm_area_struct *vma, struct page *page,
>   }
>   
>   /*
> - * try_to_merge_one_page - take two pages and merge them into one
> - * @vma: the vma that holds the pte pointing to page
> - * @page: the PageAnon page that we want to replace with kpage
> - * @kpage: the PageKsm page that we want to map instead of page,
> - *         or NULL the first time when we want to use page as kpage.
> + * try_to_merge_one_page - take two folios and merge them into one
> + * @vma: the vma that holds the pte pointing to folio
> + * @folio: the PageAnon page that we want to replace with kfolio
> + * @kfolio: the PageKsm page that we want to map instead of folio,
> + *         or NULL the first time when we want to use folio as kfolio.
>    *
> - * This function returns 0 if the pages were merged, -EFAULT otherwise.
> + * This function returns 0 if the folios were merged, -EFAULT otherwise.
>    */
> -static int try_to_merge_one_page(struct vm_area_struct *vma, struct page *page,
> -				 struct ksm_rmap_item *rmap_item, struct page *kpage)
> +static int try_to_merge_one_page(struct vm_area_struct *vma, struct folio *folio,
> +				 struct ksm_rmap_item *rmap_item, struct folio *kfolio)
>   {
>   	pte_t orig_pte = __pte(0);
>   	int err = -EFAULT;
> +	struct page *page = folio_page(folio, 0);
> +	struct page *kpage;
>   
> -	if (page == kpage)			/* ksm page forked */
> +	if (kfolio)
> +		kpage = folio_page(kfolio, 0);
> +
> +	if (folio == kfolio)			/* ksm page forked */
>   		return 0;
>   
> -	if (!PageAnon(page))
> +	if (!folio_test_anon(folio))
>   		goto out;
>   
>   	/*
> @@ -1489,11 +1494,11 @@ static int try_to_merge_one_page(struct vm_area_struct *vma, struct page *page,
>   	 * prefer to continue scanning and merging different pages,
>   	 * then come back to this page when it is unlocked.
>   	 */
> -	if (!trylock_page(page))
> +	if (!folio_trylock(folio))
>   		goto out;
>   
> -	if (PageTransCompound(page)) {
> -		if (split_huge_page(page))
> +	if (folio_test_large(folio)) {
> +		if (split_folio(folio))
>   			goto out_unlock;
>   	}

Careful: there is a subtle change:

In the old code, after split_huge_page() succeeded, you would have a ref 
on *page* that you have to drop.

Now, you would have a ref on *folio* -- IOW the head page that calling 
code has to drop.

Is that handled accordingly? IOW, is there no code that would assume it 
would drop the reference on the *page* instead of on the *folio* (that, 
after split succeeds, would be an order-0 folio)

If so, worth spelling out in the description (you say "So in the calling 
path to try_to_merge_one_page() parameter pages are actually folios", 
but I am not sure if that means that all page refcounting code was 
changed to folio refcounting code).

-- 
Cheers,

David / dhildenb

Re: [PATCH 03/10] mm/ksm: use folio in try_to_merge_one_page

[Alex Shi]

On 6/4/24 4:19 PM, David Hildenbrand wrote:
>>
>> @@ -1489,11 +1494,11 @@ static int try_to_merge_one_page(struct vm_area_struct *vma, struct page *page,
>>        * prefer to continue scanning and merging different pages,
>>        * then come back to this page when it is unlocked.
>>        */
>> -    if (!trylock_page(page))
>> +    if (!folio_trylock(folio))
>>           goto out;
>>   -    if (PageTransCompound(page)) {
>> -        if (split_huge_page(page))
>> +    if (folio_test_large(folio)) {
>> +        if (split_folio(folio))
>>               goto out_unlock;
>>       }
> 
> Careful: there is a subtle change:
> 
> In the old code, after split_huge_page() succeeded, you would have a ref on *page* that you have to drop.
> 
> Now, you would have a ref on *folio* -- IOW the head page that calling code has to drop.
> 
> Is that handled accordingly? IOW, is there no code that would assume it would drop the reference on the *page* instead of on the *folio* (that, after split succeeds, would be an order-0 folio)
> 
> If so, worth spelling out in the description (you say "So in the calling path to try_to_merge_one_page() parameter pages are actually folios", but I am not sure if that means that all page refcounting code was changed to folio refcounting code).

Hi David,

Thanks a lot for comments. Yes, the ref on folio was handled. I will mention it again in commit log.
The 2nd patch in this series would just pass folios into next step, tail page will be warned, so the ref on 'page' here is also the ref on folio. they are same.


Thanks
Alex

[PATCH 04/10] mm/ksm: add identical_folio func

[alexs]

From: "Alex Shi (tencent)" <alexs@kernel.org>

To replace pages_identical() func and further reduce the 'page' using in
try_to_merge_one_page function.

Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
---
 include/linux/mm.h | 17 +++++++++++++++++
 mm/ksm.c           |  2 +-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 9849dfda44d4..2d1f16ffc6ac 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -4164,6 +4164,23 @@ static inline void setup_nr_node_ids(void) {}
 
 extern int memcmp_pages(struct page *page1, struct page *page2);
 
+static inline int identical_folio(struct folio *folio1, struct folio *folio2)
+{
+	int i, nr1, nr2, ret = 0;
+
+	nr1 = folio_nr_pages(folio1);
+	nr2 = folio_nr_pages(folio2);
+	if (nr1 != nr2)
+		return 0;
+
+	for (i = 0; i < nr1; i++) {
+		ret = memcmp_pages(folio_page(folio1, i), folio_page(folio2, i));
+		if (ret)
+			break;
+	}
+
+	return !ret;
+}
 static inline int pages_identical(struct page *page1, struct page *page2)
 {
 	return !memcmp_pages(page1, page2);
diff --git a/mm/ksm.c b/mm/ksm.c
index 21bfa9bfb210..2e4790842515 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1527,7 +1527,7 @@ static int try_to_merge_one_page(struct vm_area_struct *vma, struct folio *folio
 			if (!folio_test_dirty(folio))
 				folio_set_dirty(folio);
 			err = 0;
-		} else if (pages_identical(page, kpage))
+		} else if (identical_folio(folio, kfolio))
 			err = replace_page(vma, page, kpage, orig_pte);
 	}
 
-- 
2.43.0

Re: [PATCH 04/10] mm/ksm: add identical_folio func

[David Hildenbrand]

On 04.06.24 06:24, alexs@kernel.org wrote:
> From: "Alex Shi (tencent)" <alexs@kernel.org>
> 
> To replace pages_identical() func and further reduce the 'page' using in
> try_to_merge_one_page function.
> 
> Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
> ---
>   include/linux/mm.h | 17 +++++++++++++++++
>   mm/ksm.c           |  2 +-
>   2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 9849dfda44d4..2d1f16ffc6ac 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -4164,6 +4164,23 @@ static inline void setup_nr_node_ids(void) {}
>   
>   extern int memcmp_pages(struct page *page1, struct page *page2);
>   
> +static inline int identical_folio(struct folio *folio1, struct folio *folio2)
> +{
> +	int i, nr1, nr2, ret = 0;
> +
> +	nr1 = folio_nr_pages(folio1);
> +	nr2 = folio_nr_pages(folio2);
> +	if (nr1 != nr2)
> +		return 0;
> +
> +	for (i = 0; i < nr1; i++) {
> +		ret = memcmp_pages(folio_page(folio1, i), folio_page(folio2, i));
> +		if (ret)
> +			break;
> +	}
> +
> +	return !ret;
> +}

We never get there with large folios. So handling large folios is 
completely unnecessary.

Keep it simple: either VM_WARN_ON_ONCE when a large folio is passed, or 
simply return "false". Then, check only a single page.

-- 
Cheers,

David / dhildenb

[PATCH 05/10] mm/ksm: use folio in stable_tree_search

[alexs]

From: "Alex Shi (tencent)" <alexs@kernel.org>

The stable_tree_search() function is actually using folio internal, just
pass the folio parameter from cmp_and_merge_page().

And in cmp_and_merge_page(), the parameter 'page' is actually a folio
too, so use folio to save a few compound check in necessnary context.

Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
---
 mm/ksm.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 2e4790842515..f68779651841 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1825,7 +1825,7 @@ static __always_inline struct folio *chain(struct ksm_stable_node **s_n_d,
  * This function returns the stable tree node of identical content if found,
  * NULL otherwise.
  */
-static struct page *stable_tree_search(struct page *page)
+static struct folio *stable_tree_search(struct folio *folio)
 {
 	int nid;
 	struct rb_root *root;
@@ -1833,14 +1833,12 @@ static struct page *stable_tree_search(struct page *page)
 	struct rb_node *parent;
 	struct ksm_stable_node *stable_node, *stable_node_dup, *stable_node_any;
 	struct ksm_stable_node *page_node;
-	struct folio *folio;
 
-	folio = page_folio(page);
 	page_node = folio_stable_node(folio);
 	if (page_node && page_node->head != &migrate_nodes) {
 		/* ksm page forked */
 		folio_get(folio);
-		return &folio->page;
+		return folio;
 	}
 
 	nid = get_kpfn_nid(folio_pfn(folio));
@@ -1907,7 +1905,7 @@ static struct page *stable_tree_search(struct page *page)
 			goto again;
 		}
 
-		ret = memcmp_pages(page, &tree_folio->page);
+		ret = memcmp_pages(folio_page(folio, 0), &tree_folio->page);
 		folio_put(tree_folio);
 
 		parent = *new;
@@ -1973,7 +1971,7 @@ static struct page *stable_tree_search(struct page *page)
 				folio_put(tree_folio);
 				goto replace;
 			}
-			return &tree_folio->page;
+			return tree_folio;
 		}
 	}
 
@@ -1987,7 +1985,7 @@ static struct page *stable_tree_search(struct page *page)
 out:
 	if (is_page_sharing_candidate(page_node)) {
 		folio_get(folio);
-		return &folio->page;
+		return folio;
 	} else
 		return NULL;
 
@@ -2037,7 +2035,7 @@ static struct page *stable_tree_search(struct page *page)
 	}
 	stable_node_dup->head = &migrate_nodes;
 	list_add(&stable_node_dup->list, stable_node_dup->head);
-	return &folio->page;
+	return folio;
 
 chain_append:
 	/* stable_node_dup could be null if it reached the limit */
@@ -2324,6 +2322,7 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 	unsigned int checksum;
 	int err;
 	bool max_page_sharing_bypass = false;
+	struct folio *folio, *kfolio;
 
 	stable_node = page_stable_node(page);
 	if (stable_node) {
@@ -2346,30 +2345,31 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 	}
 
 	/* We first start with searching the page inside the stable tree */
-	kpage = stable_tree_search(page);
-	if (kpage == page && rmap_item->head == stable_node) {
-		put_page(kpage);
+	folio = page_folio(page);
+	kfolio = stable_tree_search(folio);
+	if (kfolio == folio && rmap_item->head == stable_node) {
+		folio_put(kfolio);
 		return;
 	}
 
 	remove_rmap_item_from_tree(rmap_item);
 
-	if (kpage) {
-		if (PTR_ERR(kpage) == -EBUSY)
+	if (kfolio) {
+		if (kfolio == ERR_PTR(-EBUSY))
 			return;
 
-		err = try_to_merge_with_ksm_page(rmap_item, page, kpage);
+		err = try_to_merge_with_ksm_page(rmap_item, page, folio_page(kfolio, 0));
 		if (!err) {
 			/*
 			 * The page was successfully merged:
 			 * add its rmap_item to the stable tree.
 			 */
-			lock_page(kpage);
-			stable_tree_append(rmap_item, page_stable_node(kpage),
+			folio_lock(kfolio);
+			stable_tree_append(rmap_item, folio_stable_node(kfolio),
 					   max_page_sharing_bypass);
-			unlock_page(kpage);
+			folio_unlock(kfolio);
 		}
-		put_page(kpage);
+		folio_put(kfolio);
 		return;
 	}
 
-- 
2.43.0

[PATCH 06/10] mm/ksm: remove page_stable_node

[alexs]

From: "Alex Shi (tencent)" <alexs@kernel.org>

Since folio_stable_node could replace page_stable_node now, the latter
function could be removed and save a bit context.

Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
---
 mm/ksm.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index f68779651841..24de562b64e1 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1086,11 +1086,6 @@ static inline struct ksm_stable_node *folio_stable_node(struct folio *folio)
 	return folio_test_ksm(folio) ? folio_raw_mapping(folio) : NULL;
 }
 
-static inline struct ksm_stable_node *page_stable_node(struct page *page)
-{
-	return folio_stable_node(page_folio(page));
-}
-
 static inline void folio_set_stable_node(struct folio *folio,
 					 struct ksm_stable_node *stable_node)
 {
@@ -2324,7 +2319,8 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 	bool max_page_sharing_bypass = false;
 	struct folio *folio, *kfolio;
 
-	stable_node = page_stable_node(page);
+	folio = page_folio(page);
+	stable_node = folio_stable_node(folio);
 	if (stable_node) {
 		if (stable_node->head != &migrate_nodes &&
 		    get_kpfn_nid(READ_ONCE(stable_node->kpfn)) !=
@@ -2345,7 +2341,6 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 	}
 
 	/* We first start with searching the page inside the stable tree */
-	folio = page_folio(page);
 	kfolio = stable_tree_search(folio);
 	if (kfolio == folio && rmap_item->head == stable_node) {
 		folio_put(kfolio);
-- 
2.43.0

[PATCH 07/10] mm/ksm: use folio in unstable_tree_search_insert

[alexs]

From: "Alex Shi (tencent)" <alexs@kernel.org>

The calling path are using folio actually, so start to folio in
unstable_tree_search_insert to save few compound checks.

Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
---
 mm/ksm.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 24de562b64e1..14a7ca53fc91 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2194,15 +2194,14 @@ static struct ksm_stable_node *stable_tree_insert(struct folio *kfolio)
  */
 static
 struct ksm_rmap_item *unstable_tree_search_insert(struct ksm_rmap_item *rmap_item,
-					      struct page *page,
-					      struct page **tree_pagep)
+						  struct folio *folio, struct folio **tree_foliop)
 {
 	struct rb_node **new;
 	struct rb_root *root;
 	struct rb_node *parent = NULL;
 	int nid;
 
-	nid = get_kpfn_nid(page_to_pfn(page));
+	nid = get_kpfn_nid(folio_pfn(folio));
 	root = root_unstable_tree + nid;
 	new = &root->rb_node;
 
@@ -2220,12 +2219,12 @@ struct ksm_rmap_item *unstable_tree_search_insert(struct ksm_rmap_item *rmap_ite
 		/*
 		 * Don't substitute a ksm page for a forked page.
 		 */
-		if (page == tree_page) {
+		if (folio == page_folio(tree_page)) {
 			put_page(tree_page);
 			return NULL;
 		}
 
-		ret = memcmp_pages(page, tree_page);
+		ret = memcmp_pages(folio_page(folio, 0), tree_page);
 
 		parent = *new;
 		if (ret < 0) {
@@ -2244,7 +2243,7 @@ struct ksm_rmap_item *unstable_tree_search_insert(struct ksm_rmap_item *rmap_ite
 			put_page(tree_page);
 			return NULL;
 		} else {
-			*tree_pagep = tree_page;
+			*tree_foliop = page_folio(tree_page);
 			return tree_rmap_item;
 		}
 	}
@@ -2312,6 +2311,7 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 	struct mm_struct *mm = rmap_item->mm;
 	struct ksm_rmap_item *tree_rmap_item;
 	struct page *tree_page = NULL;
+	struct folio *tree_folio = NULL;
 	struct ksm_stable_node *stable_node;
 	struct page *kpage;
 	unsigned int checksum;
@@ -2411,7 +2411,7 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 			return;
 	}
 	tree_rmap_item =
-		unstable_tree_search_insert(rmap_item, page, &tree_page);
+		unstable_tree_search_insert(rmap_item, folio, &tree_folio);
 	if (tree_rmap_item) {
 		bool split;
 
-- 
2.43.0

[PATCH 08/10] mm/ksm: use folio in try_to_merge_xx serie funcs

[alexs]

From: "Alex Shi (tencent)" <alexs@kernel.org>

The try_to_merge_ serie funcs are using folios, so change the parameter
and variable 'page' to folio to save compound checks.

Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
---
 mm/ksm.c | 54 ++++++++++++++++++++++++------------------------------
 1 file changed, 24 insertions(+), 30 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 14a7ca53fc91..b9c04ce677b9 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1540,22 +1540,18 @@ static int try_to_merge_one_page(struct vm_area_struct *vma, struct folio *folio
  * This function returns 0 if the pages were merged, -EFAULT otherwise.
  */
 static int try_to_merge_with_ksm_page(struct ksm_rmap_item *rmap_item,
-				      struct page *page, struct page *kpage)
+				      struct folio *folio, struct folio *kfolio)
 {
 	struct mm_struct *mm = rmap_item->mm;
 	struct vm_area_struct *vma;
 	int err = -EFAULT;
-	struct folio *kfolio;
 
 	mmap_read_lock(mm);
 	vma = find_mergeable_vma(mm, rmap_item->address);
 	if (!vma)
 		goto out;
 
-	if (kpage)
-		kfolio = page_folio(kpage);
-
-	err = try_to_merge_one_page(vma, page_folio(page), rmap_item, kfolio);
+	err = try_to_merge_one_page(vma, folio, rmap_item, kfolio);
 	if (err)
 		goto out;
 
@@ -1567,8 +1563,8 @@ static int try_to_merge_with_ksm_page(struct ksm_rmap_item *rmap_item,
 	get_anon_vma(vma->anon_vma);
 out:
 	mmap_read_unlock(mm);
-	trace_ksm_merge_with_ksm_page(kpage, page_to_pfn(kpage ? kpage : page),
-				rmap_item, mm, err);
+	trace_ksm_merge_with_ksm_page(kfolio, folio_pfn(kfolio ? kfolio : folio),
+				      rmap_item, mm, err);
 	return err;
 }
 
@@ -1582,17 +1578,17 @@ static int try_to_merge_with_ksm_page(struct ksm_rmap_item *rmap_item,
  * Note that this function upgrades page to ksm page: if one of the pages
  * is already a ksm page, try_to_merge_with_ksm_page should be used.
  */
-static struct page *try_to_merge_two_pages(struct ksm_rmap_item *rmap_item,
-					   struct page *page,
+static struct folio *try_to_merge_two_pages(struct ksm_rmap_item *rmap_item,
+					   struct folio *folio,
 					   struct ksm_rmap_item *tree_rmap_item,
-					   struct page *tree_page)
+					   struct folio *tree_folio)
 {
 	int err;
 
-	err = try_to_merge_with_ksm_page(rmap_item, page, NULL);
+	err = try_to_merge_with_ksm_page(rmap_item, folio, NULL);
 	if (!err) {
 		err = try_to_merge_with_ksm_page(tree_rmap_item,
-							tree_page, page);
+							tree_folio, folio);
 		/*
 		 * If that fails, we have a ksm page with only one pte
 		 * pointing to it: so break it.
@@ -1600,7 +1596,7 @@ static struct page *try_to_merge_two_pages(struct ksm_rmap_item *rmap_item,
 		if (err)
 			break_cow(rmap_item);
 	}
-	return err ? NULL : page;
+	return err ? NULL : folio;
 }
 
 static __always_inline
@@ -2310,14 +2306,13 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 {
 	struct mm_struct *mm = rmap_item->mm;
 	struct ksm_rmap_item *tree_rmap_item;
-	struct page *tree_page = NULL;
 	struct folio *tree_folio = NULL;
 	struct ksm_stable_node *stable_node;
-	struct page *kpage;
+	struct folio *kfolio;
 	unsigned int checksum;
 	int err;
 	bool max_page_sharing_bypass = false;
-	struct folio *folio, *kfolio;
+	struct folio *folio;
 
 	folio = page_folio(page);
 	stable_node = folio_stable_node(folio);
@@ -2353,7 +2348,7 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 		if (kfolio == ERR_PTR(-EBUSY))
 			return;
 
-		err = try_to_merge_with_ksm_page(rmap_item, page, folio_page(kfolio, 0));
+		err = try_to_merge_with_ksm_page(rmap_item, folio, kfolio);
 		if (!err) {
 			/*
 			 * The page was successfully merged:
@@ -2415,8 +2410,8 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 	if (tree_rmap_item) {
 		bool split;
 
-		kpage = try_to_merge_two_pages(rmap_item, page,
-						tree_rmap_item, tree_page);
+		kfolio = try_to_merge_two_pages(rmap_item, folio,
+						tree_rmap_item, tree_folio);
 		/*
 		 * If both pages we tried to merge belong to the same compound
 		 * page, then we actually ended up increasing the reference
@@ -2427,23 +2422,22 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 		 * afterwards, the reference count will be correct and
 		 * split_huge_page should succeed.
 		 */
-		split = PageTransCompound(page)
-			&& compound_head(page) == compound_head(tree_page);
-		put_page(tree_page);
-		if (kpage) {
+		split = folio_test_large(folio) && folio == kfolio;
+		folio_put(tree_folio);
+		if (kfolio) {
 			/*
 			 * The pages were successfully merged: insert new
 			 * node in the stable tree and add both rmap_items.
 			 */
-			lock_page(kpage);
-			stable_node = stable_tree_insert(page_folio(kpage));
+			folio_lock(kfolio);
+			stable_node = stable_tree_insert(kfolio);
 			if (stable_node) {
 				stable_tree_append(tree_rmap_item, stable_node,
 						   false);
 				stable_tree_append(rmap_item, stable_node,
 						   false);
 			}
-			unlock_page(kpage);
+			folio_unlock(kfolio);
 
 			/*
 			 * If we fail to insert the page into the stable tree,
@@ -2465,10 +2459,10 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 			 * the page is locked, it is better to skip it and
 			 * perhaps try again later.
 			 */
-			if (!trylock_page(page))
+			if (!folio_trylock(folio))
 				return;
-			split_huge_page(page);
-			unlock_page(page);
+			split_folio(folio);
+			folio_unlock(folio);
 		}
 	}
 }
-- 
2.43.0

[PATCH 09/10] mm/ksm: calc_checksum for folio

[alexs]

From: "Alex Shi (tencent)" <alexs@kernel.org>

Let's check the whole folio for contents change, don't count it if the
folio changed.

Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
---
 mm/ksm.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index b9c04ce677b9..dc2b5e6a9659 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1258,11 +1258,13 @@ static int unmerge_and_remove_all_rmap_items(void)
 }
 #endif /* CONFIG_SYSFS */
 
-static u32 calc_checksum(struct page *page)
+static u32 calc_checksum(struct folio *folio)
 {
 	u32 checksum;
-	void *addr = kmap_local_page(page);
-	checksum = xxhash(addr, PAGE_SIZE, 0);
+	int nr = folio_nr_pages(folio);
+	void *addr = kmap_local_page(folio_page(folio, 0));
+
+	checksum = xxhash(addr, nr * PAGE_SIZE, 0);
 	kunmap_local(addr);
 	return checksum;
 }
@@ -2369,7 +2371,7 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 	 * don't want to insert it in the unstable tree, and we don't want
 	 * to waste our time searching for something identical to it there.
 	 */
-	checksum = calc_checksum(page);
+	checksum = calc_checksum(folio);
 	if (rmap_item->oldchecksum != checksum) {
 		rmap_item->oldchecksum = checksum;
 		return;
@@ -2385,7 +2387,7 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 		mmap_read_lock(mm);
 		vma = find_mergeable_vma(mm, rmap_item->address);
 		if (vma) {
-			err = try_to_merge_one_page(vma, page_folio(page), rmap_item,
+			err = try_to_merge_one_page(vma, folio, rmap_item,
 						    page_folio(ZERO_PAGE(rmap_item->address)));
 			trace_ksm_merge_one_page(
 				page_to_pfn(ZERO_PAGE(rmap_item->address)),
@@ -3916,7 +3918,7 @@ static int __init ksm_init(void)
 	int err;
 
 	/* The correct value depends on page size and endianness */
-	zero_checksum = calc_checksum(ZERO_PAGE(0));
+	zero_checksum = calc_checksum(page_folio(ZERO_PAGE(0)));
 	/* Default to false for backwards compatibility */
 	ksm_use_zero_pages = false;
 
-- 
2.43.0

Re: [PATCH 09/10] mm/ksm: calc_checksum for folio

[David Hildenbrand]

On 04.06.24 06:24, alexs@kernel.org wrote:
> From: "Alex Shi (tencent)" <alexs@kernel.org>
> 
> Let's check the whole folio for contents change, don't count it if the
> folio changed.
> 
> Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
> ---
>   mm/ksm.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/ksm.c b/mm/ksm.c
> index b9c04ce677b9..dc2b5e6a9659 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -1258,11 +1258,13 @@ static int unmerge_and_remove_all_rmap_items(void)
>   }
>   #endif /* CONFIG_SYSFS */
>   
> -static u32 calc_checksum(struct page *page)
> +static u32 calc_checksum(struct folio *folio)
>   {
>   	u32 checksum;
> -	void *addr = kmap_local_page(page);
> -	checksum = xxhash(addr, PAGE_SIZE, 0);
> +	int nr = folio_nr_pages(folio);
> +	void *addr = kmap_local_page(folio_page(folio, 0));
> +
> +	checksum = xxhash(addr, nr * PAGE_SIZE, 0);
>   	kunmap_local(addr);
>   	return checksum;
>   }
> @@ -2369,7 +2371,7 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
>   	 * don't want to insert it in the unstable tree, and we don't want
>   	 * to waste our time searching for something identical to it there.
>   	 */
> -	checksum = calc_checksum(page);
> +	checksum = calc_checksum(folio);

So for a large folio you suddenly checksum more than a single page? 
That's wrong.

Or am I missing something?


-- 
Cheers,

David / dhildenb

Re: [PATCH 09/10] mm/ksm: calc_checksum for folio

[Alex Shi]

On 6/4/24 9:18 PM, David Hildenbrand wrote:
>> @@ -2369,7 +2371,7 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
>>        * don't want to insert it in the unstable tree, and we don't want
>>        * to waste our time searching for something identical to it there.
>>        */
>> -    checksum = calc_checksum(page);
>> +    checksum = calc_checksum(folio);
> 
> So for a large folio you suddenly checksum more than a single page? That's wrong.
> 
> Or am I missing something?

I am not sure if this change are good too, anyway, comparing the whole folio may have it advantages on efficiency, but more splitting do save more pages.

Anyway, this change could be dropped.

Thanks!

Re: [PATCH 09/10] mm/ksm: calc_checksum for folio

[David Hildenbrand]

On 05.06.24 05:44, Alex Shi wrote:
> 
> 
> On 6/4/24 9:18 PM, David Hildenbrand wrote:
>>> @@ -2369,7 +2371,7 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
>>>         * don't want to insert it in the unstable tree, and we don't want
>>>         * to waste our time searching for something identical to it there.
>>>         */
>>> -    checksum = calc_checksum(page);
>>> +    checksum = calc_checksum(folio);
>>
>> So for a large folio you suddenly checksum more than a single page? That's wrong.
>>
>> Or am I missing something?
> 
> I am not sure if this change are good too, anyway, comparing the whole folio may have it advantages on efficiency, but more splitting do save more pages.

Calculating the checksum of something that could be a large folio when 
you might want to deduplicate subpages of the folio (and decide if you 
might want to split it) sound very wrong.

Please pay more attention to the details how the current code works and 
how it all works with tail pages.

-- 
Cheers,

David / dhildenb

[PATCH 10/10] m/ksm: use folio in ksm scan path

[alexs]

From: "Alex Shi (tencent)" <alexs@kernel.org>

Now we could safely use folio in ksm_do_scan path to save a few compound
checks. The 'page' still used since follow_page function still no folio
version.

Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
---
 mm/ksm.c | 47 +++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index dc2b5e6a9659..57904b3b18dc 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2304,7 +2304,7 @@ static void stable_tree_append(struct ksm_rmap_item *rmap_item,
  * @page: the page that we are searching identical page to.
  * @rmap_item: the reverse mapping into the virtual address of this page
  */
-static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_item)
+static void cmp_and_merge_page(struct folio *folio, struct ksm_rmap_item *rmap_item)
 {
 	struct mm_struct *mm = rmap_item->mm;
 	struct ksm_rmap_item *tree_rmap_item;
@@ -2314,9 +2314,7 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 	unsigned int checksum;
 	int err;
 	bool max_page_sharing_bypass = false;
-	struct folio *folio;
 
-	folio = page_folio(page);
 	stable_node = folio_stable_node(folio);
 	if (stable_node) {
 		if (stable_node->head != &migrate_nodes &&
@@ -2518,12 +2516,12 @@ static unsigned int skip_age(rmap_age_t age)
 }
 
 /*
- * Determines if a page should be skipped for the current scan.
+ * Determines if a folio should be skipped for the current scan.
  *
- * @page: page to check
- * @rmap_item: associated rmap_item of page
+ * @folio: folio to check
+ * @rmap_item: associated rmap_item of folio
  */
-static bool should_skip_rmap_item(struct page *page,
+static bool should_skip_rmap_item(struct folio *folio,
 				  struct ksm_rmap_item *rmap_item)
 {
 	rmap_age_t age;
@@ -2536,7 +2534,7 @@ static bool should_skip_rmap_item(struct page *page,
 	 * will essentially ignore them, but we still have to process them
 	 * properly.
 	 */
-	if (PageKsm(page))
+	if (folio_test_ksm(folio))
 		return false;
 
 	age = rmap_item->age;
@@ -2566,7 +2564,7 @@ static bool should_skip_rmap_item(struct page *page,
 	return true;
 }
 
-static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
+static struct ksm_rmap_item *scan_get_next_rmap_item(struct folio **folio)
 {
 	struct mm_struct *mm;
 	struct ksm_mm_slot *mm_slot;
@@ -2655,36 +2653,41 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 			ksm_scan.address = vma->vm_end;
 
 		while (ksm_scan.address < vma->vm_end) {
+			struct page *page;
+
 			if (ksm_test_exit(mm))
 				break;
-			*page = follow_page(vma, ksm_scan.address, FOLL_GET);
-			if (IS_ERR_OR_NULL(*page)) {
+			page = follow_page(vma, ksm_scan.address, FOLL_GET);
+			if (IS_ERR_OR_NULL(page)) {
 				ksm_scan.address += PAGE_SIZE;
 				cond_resched();
 				continue;
 			}
 
-			VM_WARN_ON(PageTail(*page));
-			nr = compound_nr(*page);
-			if (is_zone_device_page(*page))
+			*folio = page_folio(page);
+			VM_WARN_ON(PageTail(page));
+			nr = folio_nr_pages(*folio);
+
+			if (folio_is_zone_device(*folio))
 				goto next_page;
-			if (PageAnon(*page)) {
+
+			if (folio_test_anon(*folio)) {
 				rmap_item = get_next_rmap_item(mm_slot,
 					ksm_scan.rmap_list, ksm_scan.address);
 				if (rmap_item) {
 					ksm_scan.rmap_list = &rmap_item->rmap_list;
 
-					if (should_skip_rmap_item(*page, rmap_item))
+					if (should_skip_rmap_item(*folio, rmap_item))
 						goto next_page;
 
 					ksm_scan.address += nr * PAGE_SIZE;
 				} else
-					put_page(*page);
+					folio_put(*folio);
 				mmap_read_unlock(mm);
 				return rmap_item;
 			}
 next_page:
-			put_page(*page);
+			folio_put(*folio);
 			ksm_scan.address += nr * PAGE_SIZE;
 			cond_resched();
 		}
@@ -2755,16 +2758,16 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 static void ksm_do_scan(unsigned int scan_npages)
 {
 	struct ksm_rmap_item *rmap_item;
-	struct page *page;
+	struct folio *folio;
 	unsigned int npages = scan_npages;
 
 	while (npages-- && likely(!freezing(current))) {
 		cond_resched();
-		rmap_item = scan_get_next_rmap_item(&page);
+		rmap_item = scan_get_next_rmap_item(&folio);
 		if (!rmap_item)
 			return;
-		cmp_and_merge_page(page, rmap_item);
-		put_page(page);
+		cmp_and_merge_page(folio, rmap_item);
+		folio_put(folio);
 	}
 
 	ksm_pages_scanned += scan_npages - npages;
-- 
2.43.0

Re: [PATCH 00/10] use folio in ksm

[David Hildenbrand]

On 04.06.24 06:24, alexs@kernel.org wrote:
> From: "Alex Shi (tencent)" <alexs@kernel.org>
> 
> This is the 2nd part to use folio in ksm, it saves 82kbytes in ksm.o and
> passed all ksm testcases in kselftest.
> 
> KSM still split same folios to single page if they are good to be KSM,
> but will skip the rest subpage checking if a folio couldn't suit for
> ksm.
> 
> Alex Shi (tencent) (10):
>    mm/ksm: reduce the flush action for ksm merging page
>    mm/ksm: skip subpages of compound pages
>    mm/ksm: use folio in try_to_merge_one_page
>    mm/ksm: add identical_folio func
>    mm/ksm: use folio in stable_tree_search
>    mm/ksm: remove page_stable_node
>    mm/ksm: use folio in unstable_tree_search_insert
>    mm/ksm: use folio in try_to_merge_xx serie funcs
>    mm/ksm: calc_checksum for folio
>    m/ksm: use folio in ksm scan path

I skimmed over most patches, but I'm afraid you introduce quite some 
subtle problems.

Part of the rework should not be do convert things to folios that really 
operate on pages (of possibly large folios).

We really have to be careful about pages vs. folios before we have KSM 
folios. Only then, we know that we have small folios only.

-- 
Cheers,

David / dhildenb

Re: [PATCH 00/10] use folio in ksm

[Alex Shi]

On 6/4/24 9:28 PM, David Hildenbrand wrote:
> On 04.06.24 06:24, alexs@kernel.org wrote:
>> From: "Alex Shi (tencent)" <alexs@kernel.org>
>>
>> This is the 2nd part to use folio in ksm, it saves 82kbytes in ksm.o and
>> passed all ksm testcases in kselftest.
>>
>> KSM still split same folios to single page if they are good to be KSM,
>> but will skip the rest subpage checking if a folio couldn't suit for
>> ksm.
>>
>> Alex Shi (tencent) (10):
>>    mm/ksm: reduce the flush action for ksm merging page
>>    mm/ksm: skip subpages of compound pages
>>    mm/ksm: use folio in try_to_merge_one_page
>>    mm/ksm: add identical_folio func
>>    mm/ksm: use folio in stable_tree_search
>>    mm/ksm: remove page_stable_node
>>    mm/ksm: use folio in unstable_tree_search_insert
>>    mm/ksm: use folio in try_to_merge_xx serie funcs
>>    mm/ksm: calc_checksum for folio
>>    m/ksm: use folio in ksm scan path
> 
> I skimmed over most patches, but I'm afraid you introduce quite some subtle problems.
> 
> Part of the rework should not be do convert things to folios that really operate on pages (of possibly large folios).
> 
> We really have to be careful about pages vs. folios before we have KSM folios. Only then, we know that we have small folios only.
> 

Thanks a lot for your reviewing and comments. Let's if we'd do it better with your comments! :)

Thanks
Alex