| Message ID | 1553141595-26907-1-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/isolation: Remove redundant pfn_valid_within() in __first_valid_page() | expand |
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
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.
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?
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.
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 >
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.
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 >
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;
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(-)