| Message ID | 20230728171037.2219226-1-shikemeng@huaweicloud.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fixes and cleanups to compaction | expand |
On 28.07.23 19:10, Kemeng Shi wrote: > Remove unnecessary return for void function > > Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> > --- > mm/compaction.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/mm/compaction.c b/mm/compaction.c > index 6052cb519de1..188d610eb3b6 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -1420,8 +1420,6 @@ fast_isolate_around(struct compact_control *cc, unsigned long pfn) > /* Skip this pageblock in the future as it's full or nearly full */ > if (start_pfn == end_pfn) > set_pageblock_skip(page); > - > - return; > } > > /* Search orders in round-robin fashion */ > @@ -2863,7 +2861,7 @@ int compaction_register_node(struct node *node) > > void compaction_unregister_node(struct node *node) > { > - return device_remove_file(&node->dev, &dev_attr_compact); > + device_remove_file(&node->dev, &dev_attr_compact); > } > #endif /* CONFIG_SYSFS && CONFIG_NUMA */ > Reviewed-by: David Hildenbrand <david@redhat.com>
On 28.07.23 19:10, 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@huaweicloud.com> > --- > 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 Can we add a short comment which kind of PFN we return (first pfn of first offline section after an online section)? > @@ -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); > } > > 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); So block_start_pfn() will now point at the first PFN of the offline section. Confusing stuff, but I get the idea and I think it makes sense to me :)
on 7/28/2023 6:41 PM, David Hildenbrand wrote: > On 28.07.23 19:10, 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@huaweicloud.com> >> --- >> 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 > > Can we add a short comment which kind of PFN we return (first pfn of first offline section after an online section)? > Hi David, thanks for the review. Sure, I will add comment to skip_offline_sections_reverse.
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@huaweicloud.com> > --- > 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);
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@huaweicloud.com> >> --- >> 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!
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@huaweicloud.com> >>> --- >>> 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, then section start is not aligned with pageblock size. Please correct me if I miss anything. Thanks!
On 7/29/2023 1:10 AM, Kemeng Shi wrote: > Remove unnecessary return for void function > > Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com> > --- > mm/compaction.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/mm/compaction.c b/mm/compaction.c > index 6052cb519de1..188d610eb3b6 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -1420,8 +1420,6 @@ fast_isolate_around(struct compact_control *cc, unsigned long pfn) > /* Skip this pageblock in the future as it's full or nearly full */ > if (start_pfn == end_pfn) > set_pageblock_skip(page); > - > - return; > } > > /* Search orders in round-robin fashion */ > @@ -2863,7 +2861,7 @@ int compaction_register_node(struct node *node) > > void compaction_unregister_node(struct node *node) > { > - return device_remove_file(&node->dev, &dev_attr_compact); > + device_remove_file(&node->dev, &dev_attr_compact); > } > #endif /* CONFIG_SYSFS && CONFIG_NUMA */ >
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@huaweicloud.com> >>>> --- >>>> 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. 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.
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@huaweicloud.com> >>>>> --- >>>>> 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);
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@huaweicloud.com> >>>>>> --- >>>>>> 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);
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@huaweicloud.com> >>>>>>> --- >>>>>>> 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!
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@huaweicloud.com> >>>>>>>> --- >>>>>>>> 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().
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@huaweicloud.com> >>>>>>>>> --- >>>>>>>>> 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 if it does not bother you.
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@huaweicloud.com> >>>>>>>>>> --- >>>>>>>>>> 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".
on 8/2/2023 9:11 AM, Baolin Wang wrote: > > > 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@huaweicloud.com> >>>>>>>>>>> --- >>>>>>>>>>> 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". > Thanks for the reply! I will do this in next version.
Hi all, this series contains some random cleanups and fixes to compation. Details can be found in respective patches. This patchset is base on another cleanups to lock in compaction at [1]. Thanks! [1] https://lore.kernel.org/all/20230725180456.2146626-1-shikemeng@huaweicloud.com/ Kemeng Shi (8): mm/compaction: avoid missing last page block in section after skip offline sections mm/compaction: correct last_migrated_pfn update in compact_zone mm/compaction: skip page block marked skip in isolate_migratepages_block mm/compaction: remove stale fast_find_block flag in isolate_migratepages mm/compaction: corret comment of cached migrate pfn update mm/compaction: correct comment to complete migration failure mm/compaction: remove unnecessary return for void function mm/compaction: only set skip flag if cc->no_set_skip_hint is false mm/compaction.c | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-)