[PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()

[PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()

[Anshuman Khandual]

pfn_valid_within() calls pfn_valid() when CONFIG_HOLES_IN_ZONE making it
redundant for both definitions (w/wo CONFIG_MEMORY_HOTPLUG) of the helper
pfn_to_online_page() which either calls pfn_valid() or pfn_valid_within().
pfn_valid_within() being 1 when !CONFIG_HOLES_IN_ZONE is irrelevant either
way. This does not change functionality.

Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages")
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 mm/page_isolation.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index ce323e56b34d..d9b02bb13d60 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -150,8 +150,6 @@ __first_valid_page(unsigned long pfn, unsigned long nr_pages)
 	for (i = 0; i < nr_pages; i++) {
 		struct page *page;
 
-		if (!pfn_valid_within(pfn + i))
-			continue;
 		page = pfn_to_online_page(pfn + i);
 		if (!page)
 			continue;
-- 
2.20.1

Re: [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()

[Zi Yan]

On 20 Mar 2019, at 21:13, Anshuman Khandual wrote:

> pfn_valid_within() calls pfn_valid() when CONFIG_HOLES_IN_ZONE making 
> it
> redundant for both definitions (w/wo CONFIG_MEMORY_HOTPLUG) of the 
> helper
> pfn_to_online_page() which either calls pfn_valid() or 
> pfn_valid_within().
> pfn_valid_within() being 1 when !CONFIG_HOLES_IN_ZONE is irrelevant 
> either
> way. This does not change functionality.
>
> Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages")

I would not say this patch fixes the commit 2ce13640b3f4 from 2017,
because the pfn_valid_within() in pfn_to_online_page() was introduced by
a recent commit b13bc35193d9e last month. :)

> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  mm/page_isolation.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index ce323e56b34d..d9b02bb13d60 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -150,8 +150,6 @@ __first_valid_page(unsigned long pfn, unsigned 
> long nr_pages)
>  	for (i = 0; i < nr_pages; i++) {
>  		struct page *page;
>
> -		if (!pfn_valid_within(pfn + i))
> -			continue;
>  		page = pfn_to_online_page(pfn + i);
>  		if (!page)
>  			continue;

This makes sense to me. You can add Reviewed-by: Zi Yan 
<ziy@nvidia.com>.

--
Best Regards,
Yan Zi

Re: [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()

[Anshuman Khandual]

On 03/21/2019 10:31 AM, Zi Yan wrote:
> On 20 Mar 2019, at 21:13, Anshuman Khandual wrote:
> 
>> pfn_valid_within() calls pfn_valid() when CONFIG_HOLES_IN_ZONE making it
>> redundant for both definitions (w/wo CONFIG_MEMORY_HOTPLUG) of the helper
>> pfn_to_online_page() which either calls pfn_valid() or pfn_valid_within().
>> pfn_valid_within() being 1 when !CONFIG_HOLES_IN_ZONE is irrelevant either
>> way. This does not change functionality.
>>
>> Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages")
> 
> I would not say this patch fixes the commit 2ce13640b3f4 from 2017,
> because the pfn_valid_within() in pfn_to_online_page() was introduced by
> a recent commit b13bc35193d9e last month. :)

Right, will update the tag with this commit.

Re: [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()

[Michal Hocko]

On Thu 21-03-19 11:03:18, Anshuman Khandual wrote:
> 
> 
> On 03/21/2019 10:31 AM, Zi Yan wrote:
> > On 20 Mar 2019, at 21:13, Anshuman Khandual wrote:
> > 
> >> pfn_valid_within() calls pfn_valid() when CONFIG_HOLES_IN_ZONE making it
> >> redundant for both definitions (w/wo CONFIG_MEMORY_HOTPLUG) of the helper
> >> pfn_to_online_page() which either calls pfn_valid() or pfn_valid_within().
> >> pfn_valid_within() being 1 when !CONFIG_HOLES_IN_ZONE is irrelevant either
> >> way. This does not change functionality.
> >>
> >> Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages")
> > 
> > I would not say this patch fixes the commit 2ce13640b3f4 from 2017,
> > because the pfn_valid_within() in pfn_to_online_page() was introduced by
> > a recent commit b13bc35193d9e last month. :)
> 
> Right, will update the tag with this commit.

The patch is correct but I wouldn't bother to add Fixes tag at all. The
current code is obviously not incorrect. Do you see any actual
performance issue?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()

[Anshuman Khandual]

On 03/21/2019 01:37 PM, Michal Hocko wrote:
> On Thu 21-03-19 11:03:18, Anshuman Khandual wrote:
>>
>>
>> On 03/21/2019 10:31 AM, Zi Yan wrote:
>>> On 20 Mar 2019, at 21:13, Anshuman Khandual wrote:
>>>
>>>> pfn_valid_within() calls pfn_valid() when CONFIG_HOLES_IN_ZONE making it
>>>> redundant for both definitions (w/wo CONFIG_MEMORY_HOTPLUG) of the helper
>>>> pfn_to_online_page() which either calls pfn_valid() or pfn_valid_within().
>>>> pfn_valid_within() being 1 when !CONFIG_HOLES_IN_ZONE is irrelevant either
>>>> way. This does not change functionality.
>>>>
>>>> Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages")
>>>
>>> I would not say this patch fixes the commit 2ce13640b3f4 from 2017,
>>> because the pfn_valid_within() in pfn_to_online_page() was introduced by
>>> a recent commit b13bc35193d9e last month. :)
>>
>> Right, will update the tag with this commit.
> 
> The patch is correct but I wouldn't bother to add Fixes tag at all. The
> current code is obviously not incorrect. Do you see any actual

Sure.

> performance issue?
> 

No. Just from code inspection. pfn_valid() is anyways expensive on arm64
because of the memblock search so why to make it redundant as well.

Re: [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()

[Oscar Salvador]

On Thu, Mar 21, 2019 at 09:43:15AM +0530, Anshuman Khandual wrote:
> pfn_valid_within() calls pfn_valid() when CONFIG_HOLES_IN_ZONE making it
> redundant for both definitions (w/wo CONFIG_MEMORY_HOTPLUG) of the helper
> pfn_to_online_page() which either calls pfn_valid() or pfn_valid_within().
> pfn_valid_within() being 1 when !CONFIG_HOLES_IN_ZONE is irrelevant either
> way. This does not change functionality.
> 
> Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages")
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>

About the "Fixes:" tag issue, I agree with Michal that the code is not
really broken, but perhaps "suboptimal" depending on how much can affect
performance on those systems where pfn_valid_within() is more complicated than
simple returning true.

I see that on arm64, that calls memblock_is_map_memory()->memblock_search(),
to trigger a search for the region containing the address, so I guess it
is an expensive operation.

Depending on how much time we can shave, it might be worth to have the tag
Fixes, but the removal of the code is fine anyway, so:

Reviewed-by: Oscar Salvador <osalvador@suse.de>

> ---
>  mm/page_isolation.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index ce323e56b34d..d9b02bb13d60 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -150,8 +150,6 @@ __first_valid_page(unsigned long pfn, unsigned long nr_pages)
>  	for (i = 0; i < nr_pages; i++) {
>  		struct page *page;
>  
> -		if (!pfn_valid_within(pfn + i))
> -			continue;
>  		page = pfn_to_online_page(pfn + i);
>  		if (!page)
>  			continue;
> -- 
> 2.20.1
> 

-- 
Oscar Salvador
SUSE L3

Re: [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()

[Michal Hocko]

On Thu 21-03-19 10:42:40, Oscar Salvador wrote:
> On Thu, Mar 21, 2019 at 09:43:15AM +0530, Anshuman Khandual wrote:
> > pfn_valid_within() calls pfn_valid() when CONFIG_HOLES_IN_ZONE making it
> > redundant for both definitions (w/wo CONFIG_MEMORY_HOTPLUG) of the helper
> > pfn_to_online_page() which either calls pfn_valid() or pfn_valid_within().
> > pfn_valid_within() being 1 when !CONFIG_HOLES_IN_ZONE is irrelevant either
> > way. This does not change functionality.
> > 
> > Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages")
> > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> 
> About the "Fixes:" tag issue, I agree with Michal that the code is not
> really broken, but perhaps "suboptimal" depending on how much can affect
> performance on those systems where pfn_valid_within() is more complicated than
> simple returning true.
> 
> I see that on arm64, that calls memblock_is_map_memory()->memblock_search(),
> to trigger a search for the region containing the address, so I guess it
> is an expensive operation.
> 
> Depending on how much time we can shave, it might be worth to have the tag
> Fixes, but the removal of the code is fine anyway, so:

Yeah, seeing a noticesable slowdown (actual numbers) would warrant a
backport to 5.0.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()

[Michal Hocko]

On Thu 21-03-19 09:43:15, Anshuman Khandual wrote:
> pfn_valid_within() calls pfn_valid() when CONFIG_HOLES_IN_ZONE making it
> redundant for both definitions (w/wo CONFIG_MEMORY_HOTPLUG) of the helper
> pfn_to_online_page() which either calls pfn_valid() or pfn_valid_within().
> pfn_valid_within() being 1 when !CONFIG_HOLES_IN_ZONE is irrelevant either
> way. This does not change functionality.
> 
> Fixes: 2ce13640b3f4 ("mm: __first_valid_page skip over offline pages")
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>

Forgot about
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/page_isolation.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index ce323e56b34d..d9b02bb13d60 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -150,8 +150,6 @@ __first_valid_page(unsigned long pfn, unsigned long nr_pages)
>  	for (i = 0; i < nr_pages; i++) {
>  		struct page *page;
>  
> -		if (!pfn_valid_within(pfn + i))
> -			continue;
>  		page = pfn_to_online_page(pfn + i);
>  		if (!page)
>  			continue;
> -- 
> 2.20.1
> 

-- 
Michal Hocko
SUSE Labs