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

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

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>