* [PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update() @ 2013-06-18 22:10 Cody P Schafer 2013-06-19 9:15 ` Srivatsa S. Bhat 0 siblings, 1 reply; 7+ messages in thread From: Cody P Schafer @ 2013-06-18 22:10 UTC (permalink / raw) To: Andrew Morton; +Cc: Cody P Schafer, Linux MM __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> ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update() 2013-06-18 22:10 [PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update() Cody P Schafer @ 2013-06-19 9:15 ` Srivatsa S. Bhat 2013-06-19 22:53 ` David Rientjes 0 siblings, 1 reply; 7+ messages in thread From: Srivatsa S. Bhat @ 2013-06-19 9:15 UTC (permalink / raw) To: Cody P Schafer; +Cc: Andrew Morton, Linux MM 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update() 2013-06-19 9:15 ` Srivatsa S. Bhat @ 2013-06-19 22:53 ` David Rientjes 2013-06-19 23:06 ` Cody P Schafer 2013-06-20 6:32 ` Srivatsa S. Bhat 0 siblings, 2 replies; 7+ messages in thread From: David Rientjes @ 2013-06-19 22:53 UTC (permalink / raw) To: Srivatsa S. Bhat; +Cc: Cody P Schafer, Andrew Morton, Linux MM 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update() 2013-06-19 22:53 ` David Rientjes @ 2013-06-19 23:06 ` Cody P Schafer 2013-06-19 23:17 ` David Rientjes 2013-06-20 6:32 ` Srivatsa S. Bhat 1 sibling, 1 reply; 7+ messages in thread From: Cody P Schafer @ 2013-06-19 23:06 UTC (permalink / raw) To: David Rientjes; +Cc: Srivatsa S. Bhat, Andrew Morton, Linux MM 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update() 2013-06-19 23:06 ` Cody P Schafer @ 2013-06-19 23:17 ` David Rientjes 2013-06-19 23:21 ` Cody P Schafer 0 siblings, 1 reply; 7+ messages in thread From: David Rientjes @ 2013-06-19 23:17 UTC (permalink / raw) To: Cody P Schafer; +Cc: Srivatsa S. Bhat, Andrew Morton, Linux MM 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update() 2013-06-19 23:17 ` David Rientjes @ 2013-06-19 23:21 ` Cody P Schafer 0 siblings, 0 replies; 7+ messages in thread From: Cody P Schafer @ 2013-06-19 23:21 UTC (permalink / raw) To: David Rientjes; +Cc: Srivatsa S. Bhat, Andrew Morton, Linux MM 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update() 2013-06-19 22:53 ` David Rientjes 2013-06-19 23:06 ` Cody P Schafer @ 2013-06-20 6:32 ` Srivatsa S. Bhat 1 sibling, 0 replies; 7+ messages in thread From: Srivatsa S. Bhat @ 2013-06-20 6:32 UTC (permalink / raw) To: David Rientjes; +Cc: Cody P Schafer, Andrew Morton, Linux MM 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-06-20 6:35 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-06-18 22:10 [PATCH] mm/page_alloc: remove repetitious local_irq_save() in __zone_pcp_update() Cody P Schafer 2013-06-19 9:15 ` Srivatsa S. Bhat 2013-06-19 22:53 ` David Rientjes 2013-06-19 23:06 ` Cody P Schafer 2013-06-19 23:17 ` David Rientjes 2013-06-19 23:21 ` Cody P Schafer 2013-06-20 6:32 ` Srivatsa S. Bhat
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.