diff mbox series

[RFC,3/5] mm, page_alloc(): remove setup_pageset()

Message ID 20200907163628.26495-4-vbabka@suse.cz
State New
Headers show
Series disable pcplists during page isolation | expand

Commit Message

Vlastimil Babka Sept. 7, 2020, 4:36 p.m. UTC
We initialize boot-time pagesets with setup_pageset(), which sets high and
batch values that effectively disable pcplists.

We can remove this wrapper if we just set these values for all pagesets in
pageset_init(). Non-boot pagesets then subsequently update them to specific
values.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/page_alloc.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Oscar Salvador Sept. 10, 2020, 9:23 a.m. UTC | #1
On Mon, Sep 07, 2020 at 06:36:26PM +0200, Vlastimil Babka wrote:
> We initialize boot-time pagesets with setup_pageset(), which sets high and
> batch values that effectively disable pcplists.
> 
> We can remove this wrapper if we just set these values for all pagesets in
> pageset_init(). Non-boot pagesets then subsequently update them to specific
> values.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

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

Just one question below:

> -static void setup_pageset(struct per_cpu_pageset *p)
> -{
> -	pageset_init(p);
> -	pageset_update(&p->pcp, 0, 1);
> +	/*
> +	 * Set batch and high values safe for a boot pageset. Proper pageset's
> +	 * initialization will update them.
> +	 */
> +	pcp->high = 0;
> +	pcp->batch  = 1;

pageset_update was manipulating these values with barriers in between.
I guess we do not care here because we are not really updating but
initializing them, right?
Oscar Salvador Sept. 10, 2020, 9:57 a.m. UTC | #2
On Thu, Sep 10, 2020 at 11:23:07AM +0200, Oscar Salvador wrote:
> On Mon, Sep 07, 2020 at 06:36:26PM +0200, Vlastimil Babka wrote:
> > We initialize boot-time pagesets with setup_pageset(), which sets high and
> > batch values that effectively disable pcplists.
> > 
> > We can remove this wrapper if we just set these values for all pagesets in
> > pageset_init(). Non-boot pagesets then subsequently update them to specific
> > values.
> > 
> > Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> Reviewed-by: Oscar Salvador <osalvador@suse.de>

You would need to squash here the remove of setup_pageset's declaration.
And then remove it from patch#4.
David Hildenbrand Sept. 10, 2020, 11 a.m. UTC | #3
On 07.09.20 18:36, Vlastimil Babka wrote:
> We initialize boot-time pagesets with setup_pageset(), which sets high and
> batch values that effectively disable pcplists.
> 
> We can remove this wrapper if we just set these values for all pagesets in
> pageset_init(). Non-boot pagesets then subsequently update them to specific
> values.
> 

subject: s/page_alloc()/page_alloc/ to make it look consistent

> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/page_alloc.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index f669a251f654..a0cab2c6055e 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5902,7 +5902,7 @@ build_all_zonelists_init(void)
>  	 * (a chicken-egg dilemma).
>  	 */
>  	for_each_possible_cpu(cpu)
> -		setup_pageset(&per_cpu(boot_pageset, cpu));
> +		pageset_init(&per_cpu(boot_pageset, cpu));
>  
>  	mminit_verify_zonelist();
>  	cpuset_init_current_mems_allowed();
> @@ -6228,12 +6228,13 @@ static void pageset_init(struct per_cpu_pageset *p)
>  	pcp = &p->pcp;
>  	for (migratetype = 0; migratetype < MIGRATE_PCPTYPES; migratetype++)
>  		INIT_LIST_HEAD(&pcp->lists[migratetype]);
> -}
>  
> -static void setup_pageset(struct per_cpu_pageset *p)
> -{
> -	pageset_init(p);
> -	pageset_update(&p->pcp, 0, 1);
> +	/*
> +	 * Set batch and high values safe for a boot pageset. Proper pageset's
> +	 * initialization will update them.
> +	 */
> +	pcp->high = 0;
> +	pcp->batch  = 1;
>  }
>  
>  static void zone_set_pageset_high_and_batch(struct zone *zone)
> 

Had the same thought about barriers being useless when the pageset is
not hot.

LGTM

Reviewed-by: David Hildenbrand <david@redhat.com>
Vlastimil Babka Sept. 18, 2020, 9:37 a.m. UTC | #4
On 9/10/20 11:23 AM, Oscar Salvador wrote:
> On Mon, Sep 07, 2020 at 06:36:26PM +0200, Vlastimil Babka wrote:
>> We initialize boot-time pagesets with setup_pageset(), which sets high and
>> batch values that effectively disable pcplists.
>> 
>> We can remove this wrapper if we just set these values for all pagesets in
>> pageset_init(). Non-boot pagesets then subsequently update them to specific
>> values.
>> 
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> Reviewed-by: Oscar Salvador <osalvador@suse.de>

Thanks!

> Just one question below:
> 
>> -static void setup_pageset(struct per_cpu_pageset *p)
>> -{
>> -	pageset_init(p);
>> -	pageset_update(&p->pcp, 0, 1);
>> +	/*
>> +	 * Set batch and high values safe for a boot pageset. Proper pageset's
>> +	 * initialization will update them.
>> +	 */
>> +	pcp->high = 0;
>> +	pcp->batch  = 1;
> 
> pageset_update was manipulating these values with barriers in between.
> I guess we do not care here because we are not really updating but
> initializing them, right?

Sure. We just initialized all the list heads, so there can be no concurrent
access at this point. But I'll mention it in the comment.
diff mbox series

Patch

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f669a251f654..a0cab2c6055e 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5902,7 +5902,7 @@  build_all_zonelists_init(void)
 	 * (a chicken-egg dilemma).
 	 */
 	for_each_possible_cpu(cpu)
-		setup_pageset(&per_cpu(boot_pageset, cpu));
+		pageset_init(&per_cpu(boot_pageset, cpu));
 
 	mminit_verify_zonelist();
 	cpuset_init_current_mems_allowed();
@@ -6228,12 +6228,13 @@  static void pageset_init(struct per_cpu_pageset *p)
 	pcp = &p->pcp;
 	for (migratetype = 0; migratetype < MIGRATE_PCPTYPES; migratetype++)
 		INIT_LIST_HEAD(&pcp->lists[migratetype]);
-}
 
-static void setup_pageset(struct per_cpu_pageset *p)
-{
-	pageset_init(p);
-	pageset_update(&p->pcp, 0, 1);
+	/*
+	 * Set batch and high values safe for a boot pageset. Proper pageset's
+	 * initialization will update them.
+	 */
+	pcp->high = 0;
+	pcp->batch  = 1;
 }
 
 static void zone_set_pageset_high_and_batch(struct zone *zone)