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