linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()
@ 2019-03-21  4:13 Anshuman Khandual
  2019-03-21  5:01 ` Zi Yan
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Anshuman Khandual @ 2019-03-21  4:13 UTC (permalink / raw)
  To: linux-mm, linux-kernel; +Cc: mike.kravetz, zi.yan, osalvador, mhocko, akpm

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()
  2019-03-21  4:13 [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page() Anshuman Khandual
@ 2019-03-21  5:01 ` Zi Yan
  2019-03-21  5:33   ` Anshuman Khandual
  2019-03-21  9:42 ` Oscar Salvador
  2019-03-22  9:23 ` Michal Hocko
  2 siblings, 1 reply; 8+ messages in thread
From: Zi Yan @ 2019-03-21  5:01 UTC (permalink / raw)
  To: Anshuman Khandual
  Cc: linux-mm, linux-kernel, mike.kravetz, osalvador, mhocko, akpm

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()
  2019-03-21  5:01 ` Zi Yan
@ 2019-03-21  5:33   ` Anshuman Khandual
  2019-03-21  8:07     ` Michal Hocko
  0 siblings, 1 reply; 8+ messages in thread
From: Anshuman Khandual @ 2019-03-21  5:33 UTC (permalink / raw)
  To: Zi Yan; +Cc: linux-mm, linux-kernel, mike.kravetz, osalvador, mhocko, akpm



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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()
  2019-03-21  5:33   ` Anshuman Khandual
@ 2019-03-21  8:07     ` Michal Hocko
  2019-03-21  8:13       ` Anshuman Khandual
  0 siblings, 1 reply; 8+ messages in thread
From: Michal Hocko @ 2019-03-21  8:07 UTC (permalink / raw)
  To: Anshuman Khandual
  Cc: Zi Yan, linux-mm, linux-kernel, mike.kravetz, osalvador, akpm

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()
  2019-03-21  8:07     ` Michal Hocko
@ 2019-03-21  8:13       ` Anshuman Khandual
  0 siblings, 0 replies; 8+ messages in thread
From: Anshuman Khandual @ 2019-03-21  8:13 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Zi Yan, linux-mm, linux-kernel, mike.kravetz, osalvador, akpm



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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()
  2019-03-21  4:13 [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page() Anshuman Khandual
  2019-03-21  5:01 ` Zi Yan
@ 2019-03-21  9:42 ` Oscar Salvador
  2019-03-21 10:03   ` Michal Hocko
  2019-03-22  9:23 ` Michal Hocko
  2 siblings, 1 reply; 8+ messages in thread
From: Oscar Salvador @ 2019-03-21  9:42 UTC (permalink / raw)
  To: Anshuman Khandual
  Cc: linux-mm, linux-kernel, mike.kravetz, zi.yan, mhocko, akpm

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()
  2019-03-21  9:42 ` Oscar Salvador
@ 2019-03-21 10:03   ` Michal Hocko
  0 siblings, 0 replies; 8+ messages in thread
From: Michal Hocko @ 2019-03-21 10:03 UTC (permalink / raw)
  To: Oscar Salvador
  Cc: Anshuman Khandual, linux-mm, linux-kernel, mike.kravetz, zi.yan, akpm

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page()
  2019-03-21  4:13 [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page() Anshuman Khandual
  2019-03-21  5:01 ` Zi Yan
  2019-03-21  9:42 ` Oscar Salvador
@ 2019-03-22  9:23 ` Michal Hocko
  2 siblings, 0 replies; 8+ messages in thread
From: Michal Hocko @ 2019-03-22  9:23 UTC (permalink / raw)
  To: Anshuman Khandual
  Cc: linux-mm, linux-kernel, mike.kravetz, zi.yan, osalvador, akpm

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-03-22  9:23 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-21  4:13 [PATCH] mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page() Anshuman Khandual
2019-03-21  5:01 ` Zi Yan
2019-03-21  5:33   ` Anshuman Khandual
2019-03-21  8:07     ` Michal Hocko
2019-03-21  8:13       ` Anshuman Khandual
2019-03-21  9:42 ` Oscar Salvador
2019-03-21 10:03   ` Michal Hocko
2019-03-22  9:23 ` Michal Hocko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).