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 ?
