Re: [PATCH] mm/isolate: Drop pre-validating migrate type in undo_isolate_page_range()

[Anshuman Khandual]

On 07/05/2019 01:29 PM, Oscar Salvador wrote:
> On Fri, Jul 05, 2019 at 11:42:41AM +0530, Anshuman Khandual wrote:
>> unset_migratetype_isolate() already validates under zone lock that a given
>> page has already been isolated as MIGRATE_ISOLATE. There is no need for
>> another check before. Hence just drop this redundant validation.
>>
>> Cc: Oscar Salvador <osalvador@xxxxxxx>
>> Cc: Michal Hocko <mhocko@xxxxxxxx>
>> Cc: Qian Cai <cai@xxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: linux-mm@xxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> ---
>> Is there any particular reason to do this migratetype pre-check without zone
>> lock before calling unsert_migrate_isolate() ? If not this should be removed.
> 
> I have seen this kinda behavior-checks all over the kernel.
> I guess that one of the main goals is to avoid lock contention, so we check
> if the page has the right migratetype, and then we check it again under the lock
> to see whether that has changed.

So the worst case when it becomes redundant might not affect the performance much ?

> 
> e.g: simultaneous calls to undo_isolate_page_range

Right.

> 
> But I am not sure if the motivation behind was something else, as the changelog
> that added this code was quite modest.

Agreed.

> 
> Anyway, how did you come across with this?
> Do things get speed up without this check? Or what was the motivation to remove it?

Detected this during a code audit. I figured it can help save some cycles. The other
call site start_isolate_page_range() does not check migrate type because the page
block is guaranteed to be MIGRATE_ISOLATE ? I am not sure if a non-lock check first
in this case is actually improving performance. In which case should we just leave
the check as is ?