linux-kernel - Re: [PATCH] mm/isolate: Drop pre-validating migrate type in undo_isolate_page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 5 Jul 2019 14:00:22 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     Oscar Salvador <osalvador@...e.de>
Cc:     linux-mm@...ck.org, Michal Hocko <mhocko@...e.com>,
        Qian Cai <cai@....pw>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/isolate: Drop pre-validating migrate type in
 undo_isolate_page_range()



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@...e.de>
>> Cc: Michal Hocko <mhocko@...e.com>
>> Cc: Qian Cai <cai@....pw>
>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>> Cc: linux-mm@...ck.org
>> Cc: linux-kernel@...r.kernel.org
>>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
>> ---
>> 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 ?