| Message ID | 20241114065720.3665-1-liuq131@chinatelecom.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mm/compaction: remove unnecessary detection code. | expand |
On 11/14/24 07:57, Qiang Liu wrote: > It is impossible for the situation where blockpfn > end_pfn to arise, > The if statement here is not only unnecessary, but may also lead to > a misunderstanding that blockpfn > end_pfn could potentially happen. > so these unnecessary checking code should be removed. > > Signed-off-by: Qiang Liu <liuq131@chinatelecom.cn> I see that's since 3da0272a4c7d ("mm/compaction: correctly return failure with bogus compound_order in strict mode") I think that commit introduced a risk of overflow due to a bogus order (which we read in a racy way), and once blockpfn overflows it will satisfy <= end_pfn and might e.g. end up scanning a completely different zone? if (blockpfn + (1UL << order) <= end_pfn) { blockpfn += (1UL << order) - 1; page += (1UL << order) - 1; nr_scanned += (1UL << order) - 1; } We should better add back the MAX_ORDER sanity check? > --- > mm/compaction.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/mm/compaction.c b/mm/compaction.c > index a2b16b08cbbf..baeda7132252 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -682,12 +682,6 @@ static unsigned long isolate_freepages_block(struct compact_control *cc, > if (locked) > spin_unlock_irqrestore(&cc->zone->lock, flags); > > - /* > - * Be careful to not go outside of the pageblock. > - */ > - if (unlikely(blockpfn > end_pfn)) > - blockpfn = end_pfn; > - > trace_mm_compaction_isolate_freepages(*start_pfn, blockpfn, > nr_scanned, total_isolated); >
On 11/14/24 08:44, Vlastimil Babka wrote: > On 11/14/24 07:57, Qiang Liu wrote: >> It is impossible for the situation where blockpfn > end_pfn to arise, >> The if statement here is not only unnecessary, but may also lead to >> a misunderstanding that blockpfn > end_pfn could potentially happen. >> so these unnecessary checking code should be removed. >> >> Signed-off-by: Qiang Liu <liuq131@chinatelecom.cn> > > I see that's since 3da0272a4c7d ("mm/compaction: correctly return failure > with bogus compound_order in strict mode") Hm but we still have: for (; blockpfn < end_pfn; blockpfn += stride, page += stride) { and this advance by stride can mix up with advance by isolated, initial pfn might not be aligned... I don't see any guarantee that the for loop will exit with exactly blockpfn == end_pfn, it may easily advance beyond end_pfn so we shouldn't remove the check? > I think that commit introduced a risk of overflow due to a bogus order > (which we read in a racy way), and once blockpfn overflows it will satisfy > <= end_pfn and might e.g. end up scanning a completely different zone? > > if (blockpfn + (1UL << order) <= end_pfn) { > > blockpfn += (1UL << order) - 1; > page += (1UL << order) - 1; > nr_scanned += (1UL << order) - 1; > } > > We should better add back the MAX_ORDER sanity check? > >> --- >> mm/compaction.c | 6 ------ >> 1 file changed, 6 deletions(-) >> >> diff --git a/mm/compaction.c b/mm/compaction.c >> index a2b16b08cbbf..baeda7132252 100644 >> --- a/mm/compaction.c >> +++ b/mm/compaction.c >> @@ -682,12 +682,6 @@ static unsigned long isolate_freepages_block(struct compact_control *cc, >> if (locked) >> spin_unlock_irqrestore(&cc->zone->lock, flags); >> >> - /* >> - * Be careful to not go outside of the pageblock. >> - */ >> - if (unlikely(blockpfn > end_pfn)) >> - blockpfn = end_pfn; >> - >> trace_mm_compaction_isolate_freepages(*start_pfn, blockpfn, >> nr_scanned, total_isolated); >> > >
Hello on 11/14/2024 3:44 PM, Vlastimil Babka wrote: > On 11/14/24 07:57, Qiang Liu wrote: >> It is impossible for the situation where blockpfn > end_pfn to arise, >> The if statement here is not only unnecessary, but may also lead to >> a misunderstanding that blockpfn > end_pfn could potentially happen. >> so these unnecessary checking code should be removed. >> >> Signed-off-by: Qiang Liu <liuq131@chinatelecom.cn> > As stride could 32, if isolate_freepages_range() is called with start_pfn not aligned with 32, we could bail out look with blockpfn > end_pfn in isolate_freepages_block(). Please correct if I miss something. > I see that's since 3da0272a4c7d ("mm/compaction: correctly return failure > with bogus compound_order in strict mode") > > I think that commit introduced a risk of overflow due to a bogus order > (which we read in a racy way), and once blockpfn overflows it will satisfy > <= end_pfn and might e.g. end up scanning a completely different zone? > > if (blockpfn + (1UL << order) <= end_pfn) { > > blockpfn += (1UL << order) - 1; > page += (1UL << order) - 1; > nr_scanned += (1UL << order) - 1; > } > > We should better add back the MAX_ORDER sanity check? As order of pageblock is <= MAX_ORDER, if bogus order is > MAX_ORDER, then blockpfn + (1UL << order) must be > end_pfn, I think the sanity check is not needed. Thanks. Kemeng > >> --- >> mm/compaction.c | 6 ------ >> 1 file changed, 6 deletions(-) >> >> diff --git a/mm/compaction.c b/mm/compaction.c >> index a2b16b08cbbf..baeda7132252 100644 >> --- a/mm/compaction.c >> +++ b/mm/compaction.c >> @@ -682,12 +682,6 @@ static unsigned long isolate_freepages_block(struct compact_control *cc, >> if (locked) >> spin_unlock_irqrestore(&cc->zone->lock, flags); >> >> - /* >> - * Be careful to not go outside of the pageblock. >> - */ >> - if (unlikely(blockpfn > end_pfn)) >> - blockpfn = end_pfn; >> - >> trace_mm_compaction_isolate_freepages(*start_pfn, blockpfn, >> nr_scanned, total_isolated); >> >
On 11/14/24 10:21, Kemeng Shi wrote: > > Hello > on 11/14/2024 3:44 PM, Vlastimil Babka wrote: >> On 11/14/24 07:57, Qiang Liu wrote: >>> It is impossible for the situation where blockpfn > end_pfn to arise, >>> The if statement here is not only unnecessary, but may also lead to >>> a misunderstanding that blockpfn > end_pfn could potentially happen. >>> so these unnecessary checking code should be removed. >>> >>> Signed-off-by: Qiang Liu <liuq131@chinatelecom.cn> >> > As stride could 32, if isolate_freepages_range() is called with start_pfn not > aligned with 32, we could bail out look with blockpfn > end_pfn in > isolate_freepages_block(). Please correct if I miss something. >> I see that's since 3da0272a4c7d ("mm/compaction: correctly return failure >> with bogus compound_order in strict mode") >> >> I think that commit introduced a risk of overflow due to a bogus order >> (which we read in a racy way), and once blockpfn overflows it will satisfy >> <= end_pfn and might e.g. end up scanning a completely different zone? >> >> if (blockpfn + (1UL << order) <= end_pfn) { >> >> blockpfn += (1UL << order) - 1; >> page += (1UL << order) - 1; >> nr_scanned += (1UL << order) - 1; >> } >> >> We should better add back the MAX_ORDER sanity check? > As order of pageblock is <= MAX_ORDER, if bogus order is > MAX_ORDER, then > blockpfn + (1UL << order) must be > end_pfn, I think the sanity check is > not needed. Hm I guess we could only overflow with blockpfn being initially >= 1UL << 63 and reading a bogus order of 63. So it can't realistically happen. > Thanks. > Kemeng >> >>> --- >>> mm/compaction.c | 6 ------ >>> 1 file changed, 6 deletions(-) >>> >>> diff --git a/mm/compaction.c b/mm/compaction.c >>> index a2b16b08cbbf..baeda7132252 100644 >>> --- a/mm/compaction.c >>> +++ b/mm/compaction.c >>> @@ -682,12 +682,6 @@ static unsigned long isolate_freepages_block(struct compact_control *cc, >>> if (locked) >>> spin_unlock_irqrestore(&cc->zone->lock, flags); >>> >>> - /* >>> - * Be careful to not go outside of the pageblock. >>> - */ >>> - if (unlikely(blockpfn > end_pfn)) >>> - blockpfn = end_pfn; >>> - >>> trace_mm_compaction_isolate_freepages(*start_pfn, blockpfn, >>> nr_scanned, total_isolated); >>> >> >
On 2024/11/14 15:52, Vlastimil Babka wrote: > On 11/14/24 08:44, Vlastimil Babka wrote: >> On 11/14/24 07:57, Qiang Liu wrote: >>> It is impossible for the situation where blockpfn > end_pfn to arise, >>> The if statement here is not only unnecessary, but may also lead to >>> a misunderstanding that blockpfn > end_pfn could potentially happen. >>> so these unnecessary checking code should be removed. >>> >>> Signed-off-by: Qiang Liu <liuq131@chinatelecom.cn> >> >> I see that's since 3da0272a4c7d ("mm/compaction: correctly return failure >> with bogus compound_order in strict mode") > > Hm but we still have: > > for (; blockpfn < end_pfn; blockpfn += stride, page += stride) { > > and this advance by stride can mix up with advance by isolated, initial pfn > might not be aligned... I don't see any guarantee that the for loop will > exit with exactly blockpfn == end_pfn, it may easily advance beyond end_pfn > so we shouldn't remove the check? Agreed. >> I think that commit introduced a risk of overflow due to a bogus order >> (which we read in a racy way), and once blockpfn overflows it will satisfy >> <= end_pfn and might e.g. end up scanning a completely different zone? >> >> if (blockpfn + (1UL << order) <= end_pfn) { >> >> blockpfn += (1UL << order) - 1; >> page += (1UL << order) - 1; >> nr_scanned += (1UL << order) - 1; >> } >> >> We should better add back the MAX_ORDER sanity check? >> >>> --- >>> mm/compaction.c | 6 ------ >>> 1 file changed, 6 deletions(-) >>> >>> diff --git a/mm/compaction.c b/mm/compaction.c >>> index a2b16b08cbbf..baeda7132252 100644 >>> --- a/mm/compaction.c >>> +++ b/mm/compaction.c >>> @@ -682,12 +682,6 @@ static unsigned long isolate_freepages_block(struct compact_control *cc, >>> if (locked) >>> spin_unlock_irqrestore(&cc->zone->lock, flags); >>> >>> - /* >>> - * Be careful to not go outside of the pageblock. >>> - */ >>> - if (unlikely(blockpfn > end_pfn)) >>> - blockpfn = end_pfn; >>> - >>> trace_mm_compaction_isolate_freepages(*start_pfn, blockpfn, >>> nr_scanned, total_isolated); >>> >> >>
diff --git a/mm/compaction.c b/mm/compaction.c index a2b16b08cbbf..baeda7132252 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -682,12 +682,6 @@ static unsigned long isolate_freepages_block(struct compact_control *cc, if (locked) spin_unlock_irqrestore(&cc->zone->lock, flags); - /* - * Be careful to not go outside of the pageblock. - */ - if (unlikely(blockpfn > end_pfn)) - blockpfn = end_pfn; - trace_mm_compaction_isolate_freepages(*start_pfn, blockpfn, nr_scanned, total_isolated);
It is impossible for the situation where blockpfn > end_pfn to arise, The if statement here is not only unnecessary, but may also lead to a misunderstanding that blockpfn > end_pfn could potentially happen. so these unnecessary checking code should be removed. Signed-off-by: Qiang Liu <liuq131@chinatelecom.cn> --- mm/compaction.c | 6 ------ 1 file changed, 6 deletions(-)