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

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

[Anshuman Khandual]

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@suse.de>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Qian Cai <cai@lca.pw>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.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.

 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

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@suse.de>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Qian Cai <cai@lca.pw>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.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.

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

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

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

[Michal Hocko]

On Fri 05-07-19 11:42:41, 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.

unset_migratetype_isolate take zone lock and it is always preferable to
skip not take this lock if we know it would be pointless. Besides that
undo_isolate_page_range is a slow path so a nano optimizing it is not
worth it.

> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Qian Cai <cai@lca.pw>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.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.
> 
>  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

-- 
Michal Hocko
SUSE Labs