| Message ID | 20220511044300.4069-1-yamamoto.rei@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm, compaction: fast_find_migrateblock() should return pfn in the target zone | expand |
On 2022/5/11 12:43, Rei Yamamoto wrote: > Prevent returning a pfn outside the target zone in case that not > aligned with pageblock boundary. > Otherwise isolate_migratepages_block() would handle pages not in > the target zone. > IIUC, the sole caller isolate_migratepages will ensure the pfn won't outside the target zone. So the below code change might not be necessary. Or am I miss something ? Thanks! > Signed-off-by: Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> > --- > mm/compaction.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/mm/compaction.c b/mm/compaction.c > index fe915db6149b..de42b8e48758 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -1858,6 +1858,8 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc) > > update_fast_start_pfn(cc, free_pfn); > pfn = pageblock_start_pfn(free_pfn); > + if (pfn < cc->zone->zone_start_pfn) > + pfn = cc->zone->zone_start_pfn; > cc->fast_search_fail = 0; > found_block = true; > set_pageblock_skip(freepage); >
On Wed, 11 May 2022 14:25:34 Miaohe Lin wrote: > On 2022/5/11 12:43, Rei Yamamoto wrote: >> Prevent returning a pfn outside the target zone in case that not >> aligned with pageblock boundary. >> Otherwise isolate_migratepages_block() would handle pages not in >> the target zone. >> > > IIUC, the sole caller isolate_migratepages will ensure the pfn won't outside > the target zone. So the below code change might not be necessary. Or am I miss > something ? While block_start_pfn is ensured, this variable is not used as the argument for isolate_migratepages_block(): ----- static isolate_migrate_t isolate_migratepages(struct compact_control *cc) { : low_pfn = fast_find_migrateblock(cc); block_start_pfn = pageblock_start_pfn(low_pfn); if (block_start_pfn < cc->zone->zone_start_pfn) block_start_pfn = cc->zone->zone_start_pfn; <--- block_start_pfn is ensured not outside the target zone : block_end_pfn = pageblock_end_pfn(low_pfn); : for (; block_end_pfn <= cc->free_pfn; fast_find_block = false, cc->migrate_pfn = low_pfn = block_end_pfn, block_start_pfn = block_end_pfn, block_end_pfn += pageblock_nr_pages) { : if (isolate_migratepages_block(cc, low_pfn, block_end_pfn, <--- low_pfn is passed as the argument isolate_mode)) return ISOLATE_ABORT; ----- So, the low_pfn passed to isolate_migratepages_block() can be outside the target zone. Thanks, Rei
On 2022/5/11 15:07, Rei Yamamoto wrote: > On Wed, 11 May 2022 14:25:34 Miaohe Lin wrote: >> On 2022/5/11 12:43, Rei Yamamoto wrote: >>> Prevent returning a pfn outside the target zone in case that not >>> aligned with pageblock boundary. >>> Otherwise isolate_migratepages_block() would handle pages not in >>> the target zone. >>> >> >> IIUC, the sole caller isolate_migratepages will ensure the pfn won't outside >> the target zone. So the below code change might not be necessary. Or am I miss >> something ? > > While block_start_pfn is ensured, this variable is not used as the argument for > isolate_migratepages_block(): > ----- > static isolate_migrate_t isolate_migratepages(struct compact_control *cc) > { > : > low_pfn = fast_find_migrateblock(cc); > block_start_pfn = pageblock_start_pfn(low_pfn); > if (block_start_pfn < cc->zone->zone_start_pfn) > block_start_pfn = cc->zone->zone_start_pfn; <--- block_start_pfn is ensured not outside > the target zone > : > block_end_pfn = pageblock_end_pfn(low_pfn); > : > for (; block_end_pfn <= cc->free_pfn; > fast_find_block = false, > cc->migrate_pfn = low_pfn = block_end_pfn, > block_start_pfn = block_end_pfn, > block_end_pfn += pageblock_nr_pages) { > : > if (isolate_migratepages_block(cc, low_pfn, block_end_pfn, <--- low_pfn is passed as > the argument Sorry, I think you're right. And could you please add the runtime effect of this issue? Anyway, this patch looks good to me now. Thanks! Reviewed-by: Miaohe Lin <linmiaohe@huawei.com> > isolate_mode)) > return ISOLATE_ABORT; > ----- > > So, the low_pfn passed to isolate_migratepages_block() can be outside the target zone. > > Thanks, > Rei > . >
On Wed, 11 May 2022 17:26:16 Miaohe Lin wrote: > On 2022/5/11 15:07, Rei Yamamoto wrote: >> On Wed, 11 May 2022 14:25:34 Miaohe Lin wrote: >>> On 2022/5/11 12:43, Rei Yamamoto wrote: >>>> Prevent returning a pfn outside the target zone in case that not >>>> aligned with pageblock boundary. >>>> Otherwise isolate_migratepages_block() would handle pages not in >>>> the target zone. >>>> >>> >>> IIUC, the sole caller isolate_migratepages will ensure the pfn won't outside >>> the target zone. So the below code change might not be necessary. Or am I miss >>> something ? >> >> While block_start_pfn is ensured, this variable is not used as the argument for >> isolate_migratepages_block(): >> ----- >> static isolate_migrate_t isolate_migratepages(struct compact_control *cc) >> { >> : >> low_pfn = fast_find_migrateblock(cc); >> block_start_pfn = pageblock_start_pfn(low_pfn); >> if (block_start_pfn < cc->zone->zone_start_pfn) >> block_start_pfn = cc->zone->zone_start_pfn; <--- block_start_pfn is ensured not outside >> the target zone >> : >> block_end_pfn = pageblock_end_pfn(low_pfn); >> : >> for (; block_end_pfn <= cc->free_pfn; >> fast_find_block = false, >> cc->migrate_pfn = low_pfn = block_end_pfn, >> block_start_pfn = block_end_pfn, >> block_end_pfn += pageblock_nr_pages) { >> : >> if (isolate_migratepages_block(cc, low_pfn, block_end_pfn, <--- low_pfn is passed as >> the argument > > Sorry, I think you're right. And could you please add the runtime effect of this issue? > > Anyway, this patch looks good to me now. Thanks! Thank you for your review. The runtime effect is that compaction become unintended behavior. For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block(). As a result, pages migrate between nodes unintentionally. Thanks, Rei
On Thu, 12 May 2022 10:47:36 +0900 Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> wrote: > ... > > > Sorry, I think you're right. And could you please add the runtime effect of this issue? > > > > Anyway, this patch looks good to me now. Thanks! > > Thank you for your review. > The runtime effect is that compaction become unintended behavior. > For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block(). > As a result, pages migrate between nodes unintentionally. Thanks. I updated the changelog thusly: : At present, pages not in the target zone are added to cc->migratepages : list in isolate_migratepages_block(). As a result, pages may migrate : between nodes unintentionally. : : Avoid returning a pfn outside the target zone in the case that it is : not aligned with a pageblock boundary. Otherwise : isolate_migratepages_block() will handle pages not in the target zone.
On 2022/5/12 9:47, Rei Yamamoto wrote: > On Wed, 11 May 2022 17:26:16 Miaohe Lin wrote: >> On 2022/5/11 15:07, Rei Yamamoto wrote: >>> On Wed, 11 May 2022 14:25:34 Miaohe Lin wrote: >>>> On 2022/5/11 12:43, Rei Yamamoto wrote: >>>>> Prevent returning a pfn outside the target zone in case that not >>>>> aligned with pageblock boundary. >>>>> Otherwise isolate_migratepages_block() would handle pages not in >>>>> the target zone. >>>>> >>>> >>>> IIUC, the sole caller isolate_migratepages will ensure the pfn won't outside >>>> the target zone. So the below code change might not be necessary. Or am I miss >>>> something ? >>> >>> While block_start_pfn is ensured, this variable is not used as the argument for >>> isolate_migratepages_block(): >>> ----- >>> static isolate_migrate_t isolate_migratepages(struct compact_control *cc) >>> { >>> : >>> low_pfn = fast_find_migrateblock(cc); >>> block_start_pfn = pageblock_start_pfn(low_pfn); >>> if (block_start_pfn < cc->zone->zone_start_pfn) >>> block_start_pfn = cc->zone->zone_start_pfn; <--- block_start_pfn is ensured not outside >>> the target zone >>> : >>> block_end_pfn = pageblock_end_pfn(low_pfn); >>> : >>> for (; block_end_pfn <= cc->free_pfn; >>> fast_find_block = false, >>> cc->migrate_pfn = low_pfn = block_end_pfn, >>> block_start_pfn = block_end_pfn, >>> block_end_pfn += pageblock_nr_pages) { >>> : >>> if (isolate_migratepages_block(cc, low_pfn, block_end_pfn, <--- low_pfn is passed as >>> the argument >> >> Sorry, I think you're right. And could you please add the runtime effect of this issue? >> >> Anyway, this patch looks good to me now. Thanks! > > Thank you for your review. > The runtime effect is that compaction become unintended behavior. > For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block(). > As a result, pages migrate between nodes unintentionally. Many thanks for clarifying. :) Is this worth a Fixes tag or even CC stable? Thanks! > > Thanks, > Rei > . >
On Thu, 12 May 2022 10:27:44 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote: > On 2022/5/12 9:47, Rei Yamamoto wrote: >> On Wed, 11 May 2022 17:26:16 Miaohe Lin wrote: >>> On 2022/5/11 15:07, Rei Yamamoto wrote: >>>> On Wed, 11 May 2022 14:25:34 Miaohe Lin wrote: >>>>> On 2022/5/11 12:43, Rei Yamamoto wrote: >>>>>> Prevent returning a pfn outside the target zone in case that not >>>>>> aligned with pageblock boundary. >>>>>> Otherwise isolate_migratepages_block() would handle pages not in >>>>>> the target zone. >>>>>> >>>>> >>>>> IIUC, the sole caller isolate_migratepages will ensure the pfn won't outside >>>>> the target zone. So the below code change might not be necessary. Or am I miss >>>>> something ? >>>> >>>> While block_start_pfn is ensured, this variable is not used as the argument for >>>> isolate_migratepages_block(): >>>> ----- >>>> static isolate_migrate_t isolate_migratepages(struct compact_control *cc) >>>> { >>>> : >>>> low_pfn = fast_find_migrateblock(cc); >>>> block_start_pfn = pageblock_start_pfn(low_pfn); >>>> if (block_start_pfn < cc->zone->zone_start_pfn) >>>> block_start_pfn = cc->zone->zone_start_pfn; <--- block_start_pfn is ensured not outside >>>> the target zone >>>> : >>>> block_end_pfn = pageblock_end_pfn(low_pfn); >>>> : >>>> for (; block_end_pfn <= cc->free_pfn; >>>> fast_find_block = false, >>>> cc->migrate_pfn = low_pfn = block_end_pfn, >>>> block_start_pfn = block_end_pfn, >>>> block_end_pfn += pageblock_nr_pages) { >>>> : >>>> if (isolate_migratepages_block(cc, low_pfn, block_end_pfn, <--- low_pfn is passed as >>>> the argument >>> >>> Sorry, I think you're right. And could you please add the runtime effect of this issue? >>> >>> Anyway, this patch looks good to me now. Thanks! >> >> Thank you for your review. >> The runtime effect is that compaction become unintended behavior. >> For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block(). >> As a result, pages migrate between nodes unintentionally. > > Many thanks for clarifying. :) Is this worth a Fixes tag or even CC stable? > > Thanks! Thank you for your reply. If add a Fixes tag, I think the following commit: Fixes: 70b4459 ("mm, compaction: use free lists to quickly locate a migration source") Andrew, how do you think about this? Thanks, Rei
On Wed, May 11, 2022 at 01:43:00PM +0900, Rei Yamamoto wrote: > Prevent returning a pfn outside the target zone in case that not > aligned with pageblock boundary. > Otherwise isolate_migratepages_block() would handle pages not in > the target zone. > > Signed-off-by: Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> Acked-by: Mel Gorman <mgorman@techsingularity.net>
On Thu, 12 May 2022 13:27:33 +0900 Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> wrote: > >> Thank you for your review. > >> The runtime effect is that compaction become unintended behavior. > >> For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block(). > >> As a result, pages migrate between nodes unintentionally. > > > > Many thanks for clarifying. :) Is this worth a Fixes tag or even CC stable? > > > > Thanks! > > Thank you for your reply. > > If add a Fixes tag, I think the following commit: > Fixes: 70b4459 ("mm, compaction: use free lists to quickly locate a migration source") > > Andrew, how do you think about this? Thanks, I added that and also a paragraph describing the effect of the bug: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-compaction-fast_find_migrateblock-should-return-pfn-in-the-target-zone.patch I assume this problem isn't sufficiently serious to require a -stable backport of the fix?
On Thu, 12 May 2022 13:49:45 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > On Thu, 12 May 2022 13:27:33 +0900 Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> wrote: > >> >> Thank you for your review. >> >> The runtime effect is that compaction become unintended behavior. >> >> For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block(). >> >> As a result, pages migrate between nodes unintentionally. >> > >> > Many thanks for clarifying. :) Is this worth a Fixes tag or even CC stable? >> > >> > Thanks! >> >> Thank you for your reply. >> >> If add a Fixes tag, I think the following commit: >> Fixes: 70b4459 ("mm, compaction: use free lists to quickly locate a migration source") >> >> Andrew, how do you think about this? > > Thanks, I added that and also a paragraph describing the effect of the bug: > > https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-compaction-fast_find_migrateblock-should-return-pfn-in-the-target-zone.patch > > I assume this problem isn't sufficiently serious to require a -stable > backport of the fix? This would be a serious problem for older kernels without commit a984226, because it can corrupt the lru list by handling pages in list without holding proper lru_lock. Thanks, Rei
On Wed, May 11, 2022 at 01:43:00PM +0900, Rei Yamamoto wrote: > Prevent returning a pfn outside the target zone in case that not > aligned with pageblock boundary. > Otherwise isolate_migratepages_block() would handle pages not in > the target zone. > > Signed-off-by: Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> Reviewed-by: Oscar Salvador <osalvador@suse.de> > --- > mm/compaction.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/mm/compaction.c b/mm/compaction.c > index fe915db6149b..de42b8e48758 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -1858,6 +1858,8 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc) > > update_fast_start_pfn(cc, free_pfn); > pfn = pageblock_start_pfn(free_pfn); > + if (pfn < cc->zone->zone_start_pfn) > + pfn = cc->zone->zone_start_pfn; > cc->fast_search_fail = 0; > found_block = true; > set_pageblock_skip(freepage); > -- > 2.27.0 > >
On Fri, 13 May 2022 13:11:12 +0900 Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> wrote: > On Thu, 12 May 2022 13:49:45 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > > On Thu, 12 May 2022 13:27:33 +0900 Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> wrote: > > > >> >> Thank you for your review. > >> >> The runtime effect is that compaction become unintended behavior. > >> >> For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block(). > >> >> As a result, pages migrate between nodes unintentionally. > >> > > >> > Many thanks for clarifying. :) Is this worth a Fixes tag or even CC stable? > >> > > >> > Thanks! > >> > >> Thank you for your reply. > >> > >> If add a Fixes tag, I think the following commit: > >> Fixes: 70b4459 ("mm, compaction: use free lists to quickly locate a migration source") > >> > >> Andrew, how do you think about this? > > > > Thanks, I added that and also a paragraph describing the effect of the bug: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-compaction-fast_find_migrateblock-should-return-pfn-in-the-target-zone.patch > > > > I assume this problem isn't sufficiently serious to require a -stable > > backport of the fix? > > This would be a serious problem for older kernels without commit a984226, > because it can corrupt the lru list by handling pages in list without holding proper lru_lock. Thanks, I added the above to the changelog. The patch applies OK to older kernels (I tried v5.10). So I guess we put a cc:stable in this, so it gets backported?
On Fri, 13 May 2022 14:01:41 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > On Fri, 13 May 2022 13:11:12 +0900 Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> wrote: > >> On Thu, 12 May 2022 13:49:45 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: >> > On Thu, 12 May 2022 13:27:33 +0900 Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> wrote: >> > >> >> >> Thank you for your review. >> >> >> The runtime effect is that compaction become unintended behavior. >> >> >> For example, pages not in the target zone are added to cc->migratepages list in isolate_migratepages_block(). >> >> >> As a result, pages migrate between nodes unintentionally. >> >> > >> >> > Many thanks for clarifying. :) Is this worth a Fixes tag or even CC stable? >> >> > >> >> > Thanks! >> >> >> >> Thank you for your reply. >> >> >> >> If add a Fixes tag, I think the following commit: >> >> Fixes: 70b4459 ("mm, compaction: use free lists to quickly locate a migration source") >> >> >> >> Andrew, how do you think about this? >> > >> > Thanks, I added that and also a paragraph describing the effect of the bug: >> > >> > https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-compaction-fast_find_migrateblock-should-return-pfn-in-the-target-zone.patch >> > >> > I assume this problem isn't sufficiently serious to require a -stable >> > backport of the fix? >> >> This would be a serious problem for older kernels without commit a984226, >> because it can corrupt the lru list by handling pages in list without holding proper lru_lock. > > Thanks, I added the above to the changelog. > > The patch applies OK to older kernels (I tried v5.10). So I guess we > put a cc:stable in this, so it gets backported? Sounds great. I think that's fine. Thanks, Rei
diff --git a/mm/compaction.c b/mm/compaction.c index fe915db6149b..de42b8e48758 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -1858,6 +1858,8 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc) update_fast_start_pfn(cc, free_pfn); pfn = pageblock_start_pfn(free_pfn); + if (pfn < cc->zone->zone_start_pfn) + pfn = cc->zone->zone_start_pfn; cc->fast_search_fail = 0; found_block = true; set_pageblock_skip(freepage);
Prevent returning a pfn outside the target zone in case that not aligned with pageblock boundary. Otherwise isolate_migratepages_block() would handle pages not in the target zone. Signed-off-by: Rei Yamamoto <yamamoto.rei@jp.fujitsu.com> --- mm/compaction.c | 2 ++ 1 file changed, 2 insertions(+)