Re: [PATCH v2] mm/compaction: remove unnecessary detection code.

[Baolin Wang <baolin.wang@linux.alibaba.com>]

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