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

[Oscar Salvador]

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.

e.g: simultaneous calls to undo_isolate_page_range

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

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

thanks


> 
>  mm/page_isolation.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index e3638a5bafff..f529d250c8a5 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -243,7 +243,7 @@ int undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
>  	     pfn < end_pfn;
>  	     pfn += pageblock_nr_pages) {
>  		page = __first_valid_page(pfn, pageblock_nr_pages);
> -		if (!page || !is_migrate_isolate_page(page))
> +		if (!page)
>  			continue;
>  		unset_migratetype_isolate(page, migratetype);
>  	}
> -- 
> 2.20.1
> 

-- 
Oscar Salvador
SUSE L3