[PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update()

[PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update()

[Cody P Schafer]

__zone_pcp_update() is called via stop_machine(), which already disables
local irq.

Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
---
 mm/page_alloc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index bac3107..b46b54a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -6179,7 +6179,7 @@ static int __meminit __zone_pcp_update(void *data)
 {
 	struct zone *zone = data;
 	int cpu;
-	unsigned long batch = zone_batchsize(zone), flags;
+	unsigned long batch = zone_batchsize(zone);
 
 	for_each_possible_cpu(cpu) {
 		struct per_cpu_pageset *pset;
@@ -6188,12 +6188,10 @@ static int __meminit __zone_pcp_update(void *data)
 		pset = per_cpu_ptr(zone->pageset, cpu);
 		pcp = &pset->pcp;
 
-		local_irq_save(flags);
 		if (pcp->count > 0)
 			free_pcppages_bulk(zone, pcp->count, pcp);
 		drain_zonestat(zone, pset);
 		setup_pageset(pset, batch);
-		local_irq_restore(flags);
 	}
 	return 0;
 }
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update()

[Srivatsa S. Bhat]

On 06/19/2013 03:40 AM, Cody P Schafer wrote:
> __zone_pcp_update() is called via stop_machine(), which already disables
> local irq.
> 
> Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>

Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Regards,
Srivatsa S. Bhat

> ---
>  mm/page_alloc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index bac3107..b46b54a 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -6179,7 +6179,7 @@ static int __meminit __zone_pcp_update(void *data)
>  {
>  	struct zone *zone = data;
>  	int cpu;
> -	unsigned long batch = zone_batchsize(zone), flags;
> +	unsigned long batch = zone_batchsize(zone);
> 
>  	for_each_possible_cpu(cpu) {
>  		struct per_cpu_pageset *pset;
> @@ -6188,12 +6188,10 @@ static int __meminit __zone_pcp_update(void *data)
>  		pset = per_cpu_ptr(zone->pageset, cpu);
>  		pcp = &pset->pcp;
> 
> -		local_irq_save(flags);
>  		if (pcp->count > 0)
>  			free_pcppages_bulk(zone, pcp->count, pcp);
>  		drain_zonestat(zone, pset);
>  		setup_pageset(pset, batch);
> -		local_irq_restore(flags);
>  	}
>  	return 0;
>  }
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update()

[David Rientjes]

On Wed, 19 Jun 2013, Srivatsa S. Bhat wrote:

> > __zone_pcp_update() is called via stop_machine(), which already disables
> > local irq.
> > 
> > Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
> 
> Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> 

What was reviewed?

> > ---
> >  mm/page_alloc.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index bac3107..b46b54a 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -6179,7 +6179,7 @@ static int __meminit __zone_pcp_update(void *data)
> >  {
> >  	struct zone *zone = data;
> >  	int cpu;
> > -	unsigned long batch = zone_batchsize(zone), flags;
> > +	unsigned long batch = zone_batchsize(zone);
> > 
> >  	for_each_possible_cpu(cpu) {
> >  		struct per_cpu_pageset *pset;
> > @@ -6188,12 +6188,10 @@ static int __meminit __zone_pcp_update(void *data)
> >  		pset = per_cpu_ptr(zone->pageset, cpu);
> >  		pcp = &pset->pcp;
> > 
> > -		local_irq_save(flags);
> >  		if (pcp->count > 0)
> >  			free_pcppages_bulk(zone, pcp->count, pcp);
> >  		drain_zonestat(zone, pset);
> >  		setup_pageset(pset, batch);
> > -		local_irq_restore(flags);

This seems like a fine cleanup because stop_machine() disable irqs, but it 
appears like there is two problems with this function already:

 - it's doing for_each_possible_cpu() internally, why?  local_irq_save()
   works on the local cpu and won't protect
   per_cpu_ptr(zone->pageset, cpu)->pcp of some random cpu, and

 - setup_pageset() is what is ultimately responsible for doing 
   pcp->count = 0 after free_pcppages_bulk(), but what happens if 
   pcp->count is read in between the two on the cpu that has not disabled 
   irqs?

You can't just do

	for_each_possible_cpu(cpu) {
		unsigned long flags;

		local_irq_save(flags);
		...
		local_irq_restore(flags);
	}

This is just disabling irqs locally over and over again, not on the cpu 
you're manipulating in its per-cpu critical section.

I don't think we hit this because onlining and offlining memory isn't a 
very common operation, but it doesn't change the fact that it's broken.

> >  	}
> >  	return 0;
> >  }
> > 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update()

[Cody P Schafer]

On 06/19/2013 03:53 PM, David Rientjes wrote:
> On Wed, 19 Jun 2013, Srivatsa S. Bhat wrote:
>
>>> __zone_pcp_update() is called via stop_machine(), which already disables
>>> local irq.
>>>
>>>   mm/page_alloc.c | 4 +---
>>>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> This seems like a fine cleanup because stop_machine() disable irqs, but it
> appears like there is two problems with this function already:
>

Re-examining this, I've realized that my previous patchset containing
	"mm/page_alloc: convert zone_pcp_update() to rely on memory barriers 
instead of stop_machine()"

already went through and fixed this up (the right way). So ignore this 
patch.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update()

[David Rientjes]

On Wed, 19 Jun 2013, Cody P Schafer wrote:

> Re-examining this, I've realized that my previous patchset containing
> 	"mm/page_alloc: convert zone_pcp_update() to rely on memory barriers
> instead of stop_machine()"
> 
> already went through and fixed this up (the right way). So ignore this patch.
> 

If you're referring to

mm-page_alloc-factor-out-setting-of-pcp-high-and-pcp-batch.patch
mm-page_alloc-prevent-concurrent-updaters-of-pcp-batch-and-high.patch
mm-page_alloc-insert-memory-barriers-to-allow-async-update-of-pcp-batch-and-high.patch
mm-page_alloc-protect-pcp-batch-accesses-with-access_once.patch
mm-page_alloc-convert-zone_pcp_update-to-rely-on-memory-barriers-instead-of-stop_machine.patch
mm-page_alloc-when-handling-percpu_pagelist_fraction-dont-unneedly-recalulate-high.patch
mm-page_alloc-factor-setup_pageset-into-pageset_init-and-pageset_set_batch.patch
mm-page_alloc-relocate-comment-to-be-directly-above-code-it-refers-to.patch
mm-page_alloc-factor-zone_pageset_init-out-of-setup_zone_pageset.patch
mm-page_alloc-in-zone_pcp_update-uze-zone_pageset_init.patch
mm-page_alloc-rename-setup_pagelist_highmark-to-match-naming-of-pageset_set_batch.patch

from -mm then I'll review them separately because they have their own 
issues.  Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update()

[Cody P Schafer]

On 06/19/2013 04:17 PM, David Rientjes wrote:
> On Wed, 19 Jun 2013, Cody P Schafer wrote:
>
>> Re-examining this, I've realized that my previous patchset containing
>> 	"mm/page_alloc: convert zone_pcp_update() to rely on memory barriers
>> instead of stop_machine()"
>>
>> already went through and fixed this up (the right way). So ignore this patch.
>>
>
> If you're referring to
>
> mm-page_alloc-factor-out-setting-of-pcp-high-and-pcp-batch.patch
> mm-page_alloc-prevent-concurrent-updaters-of-pcp-batch-and-high.patch
> mm-page_alloc-insert-memory-barriers-to-allow-async-update-of-pcp-batch-and-high.patch
> mm-page_alloc-protect-pcp-batch-accesses-with-access_once.patch
> mm-page_alloc-convert-zone_pcp_update-to-rely-on-memory-barriers-instead-of-stop_machine.patch
> mm-page_alloc-when-handling-percpu_pagelist_fraction-dont-unneedly-recalulate-high.patch
> mm-page_alloc-factor-setup_pageset-into-pageset_init-and-pageset_set_batch.patch
> mm-page_alloc-relocate-comment-to-be-directly-above-code-it-refers-to.patch
> mm-page_alloc-factor-zone_pageset_init-out-of-setup_zone_pageset.patch
> mm-page_alloc-in-zone_pcp_update-uze-zone_pageset_init.patch
> mm-page_alloc-rename-setup_pagelist_highmark-to-match-naming-of-pageset_set_batch.patch
>
> from -mm then I'll review them separately because they have their own
> issues.  Thanks.
>

I am. You may also want to note
mm-page_alloc-dont-re-init-pageset-in-zone_pcp_update.patch

which fixes a bug in that patchset.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update()

[Srivatsa S. Bhat]

On 06/20/2013 04:23 AM, David Rientjes wrote:
> On Wed, 19 Jun 2013, Srivatsa S. Bhat wrote:
> 
>>> __zone_pcp_update() is called via stop_machine(), which already disables
>>> local irq.
>>>
>>> Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
>>
>> Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>>
> 
> What was reviewed?
>

See below.
 
>>> ---
>>>  mm/page_alloc.c | 4 +---
>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index bac3107..b46b54a 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -6179,7 +6179,7 @@ static int __meminit __zone_pcp_update(void *data)
>>>  {
>>>  	struct zone *zone = data;
>>>  	int cpu;
>>> -	unsigned long batch = zone_batchsize(zone), flags;
>>> +	unsigned long batch = zone_batchsize(zone);
>>>
>>>  	for_each_possible_cpu(cpu) {
>>>  		struct per_cpu_pageset *pset;
>>> @@ -6188,12 +6188,10 @@ static int __meminit __zone_pcp_update(void *data)
>>>  		pset = per_cpu_ptr(zone->pageset, cpu);
>>>  		pcp = &pset->pcp;
>>>
>>> -		local_irq_save(flags);
>>>  		if (pcp->count > 0)
>>>  			free_pcppages_bulk(zone, pcp->count, pcp);
>>>  		drain_zonestat(zone, pset);
>>>  		setup_pageset(pset, batch);
>>> -		local_irq_restore(flags);
> 
> This seems like a fine cleanup because stop_machine() disable irqs,

I hope you are not missing the fact that stop_machine() disables irqs
on *all* CPUs.

> but it 
> appears like there is two problems with this function already:
> 
>  - it's doing for_each_possible_cpu() internally, why?  local_irq_save()
>    works on the local cpu and won't protect
>    per_cpu_ptr(zone->pageset, cpu)->pcp of some random cpu, and
> 

stop_machine() allows only _your function_ to run and nothing else, on
the entire system. All other CPUs loop with interrupts disabled until
the function is completed.

>  - setup_pageset() is what is ultimately responsible for doing 
>    pcp->count = 0 after free_pcppages_bulk(), but what happens if 
>    pcp->count is read in between the two on the cpu that has not disabled 
>    irqs?
> 

Nobody can do anything else when this function runs. That's precisely
why its named as stop-*machine*.

> You can't just do
> 
> 	for_each_possible_cpu(cpu) {
> 		unsigned long flags;
> 
> 		local_irq_save(flags);
> 		...
> 		local_irq_restore(flags);
> 	}
> 
> This is just disabling irqs locally over and over again, not on the cpu 
> you're manipulating in its per-cpu critical section.
>

stop-machine() takes care of disabling irqs on every online CPU.
 
> I don't think we hit this because onlining and offlining memory isn't a 
> very common operation, but it doesn't change the fact that it's broken.
>

If __zone_pcp_update() is called only from stop_machine() (and looking at
the current code, that's true), then there is no problem, due to the reasons
explained above.

Regards,
Srivatsa S. Bhat

 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>