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@xxxxxxxxxxxxxxx>
>
> 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);
>>
>
>
