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