Re: [PATCH 1/8] mm/compaction: avoid missing last page block in section after skip offline sections
From: Baolin Wang
Date: Tue Aug 01 2023 - 21:11:06 EST
On 8/1/2023 8:33 PM, Kemeng Shi wrote:
on 8/1/2023 5:32 PM, Baolin Wang wrote:
On 8/1/2023 4:42 PM, Kemeng Shi wrote:
on 8/1/2023 4:01 PM, Baolin Wang wrote:
On 8/1/2023 2:08 PM, Kemeng Shi wrote:
on 8/1/2023 11:53 AM, Baolin Wang wrote:
On 8/1/2023 10:36 AM, Kemeng Shi wrote:
on 8/1/2023 10:18 AM, Kemeng Shi wrote:
on 7/31/2023 8:01 PM, Baolin Wang wrote:
On 7/29/2023 1:10 AM, Kemeng Shi wrote:
skip_offline_sections_reverse will return the last pfn in found online
section. Then we set block_start_pfn to start of page block which
contains the last pfn in section. Then we continue, move one page
block forward and ignore the last page block in the online section.
Make block_start_pfn point to first page block after online section to fix
this:
1. make skip_offline_sections_reverse return end pfn of online section,
i.e. pfn of page block after online section.
2. assign block_start_pfn with next_pfn.
Fixes: f63224525309 ("mm: compaction: skip the memory hole rapidly when isolating free pages")
Signed-off-by: Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx>
---
mm/compaction.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/mm/compaction.c b/mm/compaction.c
index 9b7a0a69e19f..ce7841363b12 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -259,7 +259,7 @@ static unsigned long skip_offline_sections_reverse(unsigned long start_pfn)
while (start_nr-- > 0) {
if (online_section_nr(start_nr))
- return section_nr_to_pfn(start_nr) + PAGES_PER_SECTION - 1;
+ return section_nr_to_pfn(start_nr + 1);
This is incorrect, you returned the start pfn of this section.
}
return 0;
@@ -1670,8 +1670,7 @@ static void isolate_freepages(struct compact_control *cc)
next_pfn = skip_offline_sections_reverse(block_start_pfn);
if (next_pfn)
- block_start_pfn = max(pageblock_start_pfn(next_pfn),
- low_pfn);
+ block_start_pfn = max(next_pfn, low_pfn);
'block_start_pfn' should be pageblock aligned. If the 'next_pfn' is not pageblock-aligned (though this is not the common case), we should skip it.
But if the 'next_pfn' is pageblock-aligned, yes, the commit f63224525309 still ignores the last pageblock, which is not right. So I think it should be:
block_start_pfn = pageblock_aligned(next_pfn) ? : pageblock_start_pfn(next_pfn);
block_start_pfn = max(block_start_pfn, low_pfn);
Hi Baolin, thanks for reply! As skip_offline_sections_reverse is based
on skip_offline_sections. I make the assumption that section is pageblock
aligned based on that we use section start from skip_offline_sections as
block_start_fpn without align check.
If section size is not pageblock aligned in real world, the pageblock aligned
check should be added to skip_offline_sections and skip_offline_sections_reverse.
If no one is against this, I will fix this in next version. THanks!
More information of aligment of section. For powerpc arch, we have SECTION_SIZE_BITS
with 24 while PAGE_SHIFT could be configured to 18.
Pageblock order is (18 + MAX_ORDER) which coule be 28 and is > SECTION_SZIE_BITS 24,
The maximum pageblock order is MAX_ORDER. But after thinking more, I think return the start pfn or end pfn of a section is okay, and it should be aligned to a pageblock order IIUC.
Right, I mixed up the unit.
So I think your change is good:
+ block_start_pfn = max(next_pfn, low_pfn);
But in skip_offline_sections_reverse(), we should still return the last pfn of the online section.
Sure, then we should assign block_start_pfn with following change. Is this good to you?
- block_start_pfn = max(pageblock_start_pfn(next_pfn),
+ block_start_pfn = max(pageblock_end_pfn(next_pfn),
low_pfn);
The last pfn of a section is already section aligned, so I think no need to call pageblock_end_pfn(), just like your original change is okay to me.
block_start_pfn = max(next_pfn, low_pfn);
Um, if we keep "block_start_pfn = max(next_pfn, low_pfn);", should we also keep
returning end of section "section_nr_to_pfn(start_nr + 1);" instead of original last
pfn of the section "section_nr_to_pfn(start_nr) + PAGES_PER_SECTION - 1;" which seems
not aligned.
Assume SECTION_SIZE_BITS = 27, PAGE_SHIFT = 12, pageblock order = 10
Last pfn of the section 0 is 0x7fff, end pfn of section 0 is 0x8000. The last pfn
is not aligned.
Please tell me if I misunderstand anything. Thanks!
Ah, you are right, sorry for my bad arithmetic. Maybe we should return the end pfn (section_nr_to_pfn(start_nr) + PAGES_PER_SECTION) of the section in skip_offline_sections_reverse() with adding some comments to explain the return value like David suggested. Then we can remove the pageblock_end_pfn() in isolate_freepages().
Sure, I will add comments in next version. As (section_nr_to_pfn(start_nr) + PAGES_PER_SECTION)
is = section_nr_to_pfn(start_nr + 1), I will keep the change to skip_offline_sections_reverse
IMO, next section is confusing. We need return the end pfn of the
current online section, and we usually get it by
"section_nr_to_pfn(start_nr) + PAGES_PER_SECTION".