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@xxxxxxxxxxxxxxxxxx>
>>
>> Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>
>>
>
> 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@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>