[PATCH] mm/page_alloc: call set_pageblock_order() once for each node

[PATCH] mm/page_alloc: call set_pageblock_order() once for each node

[Wei Yang]

set_pageblock_order() is a standalone function which sets pageblock_order,
while current implementation calls this function on each ZONE of each node
in free_area_init_core().

Since free_area_init_node() is the only user of free_area_init_core(),
this patch moves set_pageblock_order() up one level to invoke
set_pageblock_order() only once on each node.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 8c964dcc3a9e..828f5014b006 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -6169,7 +6169,6 @@ static void __paginginit free_area_init_core(struct pglist_data *pgdat)
 		if (!size)
 			continue;
 
-		set_pageblock_order();
 		setup_usemap(pgdat, zone, zone_start_pfn, size);
 		init_currently_empty_zone(zone, zone_start_pfn, size);
 		memmap_init(size, nid, j, zone_start_pfn);
@@ -6254,6 +6253,7 @@ void __paginginit free_area_init_node(int nid, unsigned long *zones_size,
 	alloc_node_mem_map(pgdat);
 
 	reset_deferred_meminit(pgdat);
+	set_pageblock_order();
 	free_area_init_core(pgdat);
 }
 
-- 
2.15.1

Re: [PATCH] mm/page_alloc: call set_pageblock_order() once for each node

[Mel Gorman]

On Thu, Mar 29, 2018 at 11:36:07AM +0800, Wei Yang wrote:
> set_pageblock_order() is a standalone function which sets pageblock_order,
> while current implementation calls this function on each ZONE of each node
> in free_area_init_core().
> 
> Since free_area_init_node() is the only user of free_area_init_core(),
> this patch moves set_pageblock_order() up one level to invoke
> set_pageblock_order() only once on each node.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

The patch looks ok but given that set_pageblock_order returns immediately
if it has already been called, I expect the benefit is marginal. Was any
improvement in boot time measured?

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm/page_alloc: call set_pageblock_order() once for each node

[Wei Yang]

On Thu, Mar 29, 2018 at 01:11:09PM +0100, Mel Gorman wrote:
>On Thu, Mar 29, 2018 at 11:36:07AM +0800, Wei Yang wrote:
>> set_pageblock_order() is a standalone function which sets pageblock_order,
>> while current implementation calls this function on each ZONE of each node
>> in free_area_init_core().
>> 
>> Since free_area_init_node() is the only user of free_area_init_core(),
>> this patch moves set_pageblock_order() up one level to invoke
>> set_pageblock_order() only once on each node.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>
>The patch looks ok but given that set_pageblock_order returns immediately
>if it has already been called, I expect the benefit is marginal. Was any
>improvement in boot time measured?

No, I don't expect measurable improvement from this since the number of nodes
and zones are limited.

This is just a code refine from logic point of view.

>
>-- 
>Mel Gorman
>SUSE Labs

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm/page_alloc: call set_pageblock_order() once for each node

[Michal Hocko]

On Fri 30-03-18 09:02:43, Wei Yang wrote:
> On Thu, Mar 29, 2018 at 01:11:09PM +0100, Mel Gorman wrote:
> >On Thu, Mar 29, 2018 at 11:36:07AM +0800, Wei Yang wrote:
> >> set_pageblock_order() is a standalone function which sets pageblock_order,
> >> while current implementation calls this function on each ZONE of each node
> >> in free_area_init_core().
> >> 
> >> Since free_area_init_node() is the only user of free_area_init_core(),
> >> this patch moves set_pageblock_order() up one level to invoke
> >> set_pageblock_order() only once on each node.
> >> 
> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> >
> >The patch looks ok but given that set_pageblock_order returns immediately
> >if it has already been called, I expect the benefit is marginal. Was any
> >improvement in boot time measured?
> 
> No, I don't expect measurable improvement from this since the number of nodes
> and zones are limited.
> 
> This is just a code refine from logic point of view.

Then, please make sure it is a real refinement. Calling this function
per node is only half way to get there as the function is by no means
per node.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/page_alloc: call set_pageblock_order() once for each node

[Wei Yang]

On Tue, Apr 03, 2018 at 09:57:37AM +0200, Michal Hocko wrote:
>On Fri 30-03-18 09:02:43, Wei Yang wrote:
>> On Thu, Mar 29, 2018 at 01:11:09PM +0100, Mel Gorman wrote:
>> >On Thu, Mar 29, 2018 at 11:36:07AM +0800, Wei Yang wrote:
>> >> set_pageblock_order() is a standalone function which sets pageblock_order,
>> >> while current implementation calls this function on each ZONE of each node
>> >> in free_area_init_core().
>> >> 
>> >> Since free_area_init_node() is the only user of free_area_init_core(),
>> >> this patch moves set_pageblock_order() up one level to invoke
>> >> set_pageblock_order() only once on each node.
>> >> 
>> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> >
>> >The patch looks ok but given that set_pageblock_order returns immediately
>> >if it has already been called, I expect the benefit is marginal. Was any
>> >improvement in boot time measured?
>> 
>> No, I don't expect measurable improvement from this since the number of nodes
>> and zones are limited.
>> 
>> This is just a code refine from logic point of view.
>
>Then, please make sure it is a real refinement. Calling this function
>per node is only half way to get there as the function is by no means
>per node.
>

Hi, Michal

I guess you are willing to see this function is only called once for the whole
system.

Yes, that is the ideal way, well I don't come up with an elegant way. The best
way is to move this to free_area_init_nodes(), while you can see not all arch
use this function.

Then I have two options:

A: Move this to free_area_init_nodes() for those arch using it. Call it
specifically for those arch not using free_area_init_nodes().

B: call it before setup_arch() in start_kernel()

Hmm... which one you would prefer? If you have a better idea, that would be
great.

>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm/page_alloc: call set_pageblock_order() once for each node

[Michal Hocko]

On Wed 04-04-18 09:27:34, Wei Yang wrote:
> On Tue, Apr 03, 2018 at 09:57:37AM +0200, Michal Hocko wrote:
> >On Fri 30-03-18 09:02:43, Wei Yang wrote:
> >> On Thu, Mar 29, 2018 at 01:11:09PM +0100, Mel Gorman wrote:
> >> >On Thu, Mar 29, 2018 at 11:36:07AM +0800, Wei Yang wrote:
> >> >> set_pageblock_order() is a standalone function which sets pageblock_order,
> >> >> while current implementation calls this function on each ZONE of each node
> >> >> in free_area_init_core().
> >> >> 
> >> >> Since free_area_init_node() is the only user of free_area_init_core(),
> >> >> this patch moves set_pageblock_order() up one level to invoke
> >> >> set_pageblock_order() only once on each node.
> >> >> 
> >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> >> >
> >> >The patch looks ok but given that set_pageblock_order returns immediately
> >> >if it has already been called, I expect the benefit is marginal. Was any
> >> >improvement in boot time measured?
> >> 
> >> No, I don't expect measurable improvement from this since the number of nodes
> >> and zones are limited.
> >> 
> >> This is just a code refine from logic point of view.
> >
> >Then, please make sure it is a real refinement. Calling this function
> >per node is only half way to get there as the function is by no means
> >per node.
> >
> 
> Hi, Michal
> 
> I guess you are willing to see this function is only called once for the whole
> system.
> 
> Yes, that is the ideal way, well I don't come up with an elegant way. The best
> way is to move this to free_area_init_nodes(), while you can see not all arch
> use this function.
> 
> Then I have two options:
> 
> A: Move this to free_area_init_nodes() for those arch using it. Call it
> specifically for those arch not using free_area_init_nodes().
> 
> B: call it before setup_arch() in start_kernel()
> 
> Hmm... which one you would prefer? If you have a better idea, that would be
> great.

or
C: do nothing and/or just document why do we call it this way
(convenience).
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm/page_alloc: call set_pageblock_order() once for each node

[Wei Yang]

On Thu, Apr 05, 2018 at 11:55:50AM +0200, Michal Hocko wrote:
>On Wed 04-04-18 09:27:34, Wei Yang wrote:
>> On Tue, Apr 03, 2018 at 09:57:37AM +0200, Michal Hocko wrote:
>> >On Fri 30-03-18 09:02:43, Wei Yang wrote:
>> >> On Thu, Mar 29, 2018 at 01:11:09PM +0100, Mel Gorman wrote:
>> >> >On Thu, Mar 29, 2018 at 11:36:07AM +0800, Wei Yang wrote:
>> >> >> set_pageblock_order() is a standalone function which sets pageblock_order,
>> >> >> while current implementation calls this function on each ZONE of each node
>> >> >> in free_area_init_core().
>> >> >> 
>> >> >> Since free_area_init_node() is the only user of free_area_init_core(),
>> >> >> this patch moves set_pageblock_order() up one level to invoke
>> >> >> set_pageblock_order() only once on each node.
>> >> >> 
>> >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> >> >
>> >> >The patch looks ok but given that set_pageblock_order returns immediately
>> >> >if it has already been called, I expect the benefit is marginal. Was any
>> >> >improvement in boot time measured?
>> >> 
>> >> No, I don't expect measurable improvement from this since the number of nodes
>> >> and zones are limited.
>> >> 
>> >> This is just a code refine from logic point of view.
>> >
>> >Then, please make sure it is a real refinement. Calling this function
>> >per node is only half way to get there as the function is by no means
>> >per node.
>> >
>> 
>> Hi, Michal
>> 
>> I guess you are willing to see this function is only called once for the whole
>> system.
>> 
>> Yes, that is the ideal way, well I don't come up with an elegant way. The best
>> way is to move this to free_area_init_nodes(), while you can see not all arch
>> use this function.
>> 
>> Then I have two options:
>> 
>> A: Move this to free_area_init_nodes() for those arch using it. Call it
>> specifically for those arch not using free_area_init_nodes().
>> 
>> B: call it before setup_arch() in start_kernel()
>> 
>> Hmm... which one you would prefer? If you have a better idea, that would be
>> great.
>
>or
>C: do nothing and/or just document why do we call it this way
>(convenience).

Ok, maybe we can leave it alone now.

>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me