* [patch] Useless locking in mm/numa.c
@ 2002-07-19 1:03 Matthew Dobson
2002-07-19 18:36 ` Kanoj Sarcar
0 siblings, 1 reply; 6+ messages in thread
From: Matthew Dobson @ 2002-07-19 1:03 UTC (permalink / raw)
To: Andrew Morton, Martin Bligh, linux-mm, Michael Hohnbaum
[-- Attachment #1: Type: text/plain, Size: 193 bytes --]
There is a lock that is apparently protecting nothing. The node_lock spinlock
in mm/numa.c is protecting read-only accesses to pgdat_list. Here is a patch
to get rid of it.
Cheers!
-Matt
[-- Attachment #2: node_lock.patch --]
[-- Type: text/plain, Size: 752 bytes --]
--- linux-2.5.26-vanilla/mm/numa.c Tue Jul 16 16:49:30 2002
+++ linux-2.5.26-vanilla/mm/numa.c.fixed Thu Jul 18 17:59:35 2002
@@ -44,15 +44,11 @@
#define LONG_ALIGN(x) (((x)+(sizeof(long))-1)&~((sizeof(long))-1))
-static spinlock_t node_lock = SPIN_LOCK_UNLOCKED;
-
void show_free_areas_node(pg_data_t *pgdat)
{
unsigned long flags;
- spin_lock_irqsave(&node_lock, flags);
show_free_areas_core(pgdat);
- spin_unlock_irqrestore(&node_lock, flags);
}
/*
@@ -106,11 +102,9 @@
#ifdef CONFIG_NUMA
temp = NODE_DATA(numa_node_id());
#else
- spin_lock_irqsave(&node_lock, flags);
if (!next) next = pgdat_list;
temp = next;
next = next->node_next;
- spin_unlock_irqrestore(&node_lock, flags);
#endif
start = temp;
while (temp) {
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] Useless locking in mm/numa.c 2002-07-19 1:03 [patch] Useless locking in mm/numa.c Matthew Dobson @ 2002-07-19 18:36 ` Kanoj Sarcar 2002-07-19 18:38 ` William Lee Irwin III 2002-07-19 20:30 ` Matthew Dobson 0 siblings, 2 replies; 6+ messages in thread From: Kanoj Sarcar @ 2002-07-19 18:36 UTC (permalink / raw) To: colpatch, Andrew Morton, Martin Bligh, linux-mm, Michael Hohnbaum I think I put in the locks in the initial version of the file becase the idea was that show_free_areas_node() could be invoked from any cpu in a multinode system (via the sysrq keys or other intr sources), and the spin lock would provide sanity in the print out. For nonnuma discontig machines, isn't the spin lock providing protection in the pgdat list chain walking in _alloc_pages()? Kanoj --- Matthew Dobson <colpatch@us.ibm.com> wrote: > There is a lock that is apparently protecting > nothing. The node_lock spinlock > in mm/numa.c is protecting read-only accesses to > pgdat_list. Here is a patch > to get rid of it. > > Cheers! > > -Matt > > --- linux-2.5.26-vanilla/mm/numa.c Tue Jul 16 > 16:49:30 2002 > +++ linux-2.5.26-vanilla/mm/numa.c.fixed Thu Jul 18 > 17:59:35 2002 > @@ -44,15 +44,11 @@ > > #define LONG_ALIGN(x) > (((x)+(sizeof(long))-1)&~((sizeof(long))-1)) > > -static spinlock_t node_lock = SPIN_LOCK_UNLOCKED; > - > void show_free_areas_node(pg_data_t *pgdat) > { > unsigned long flags; > > - spin_lock_irqsave(&node_lock, flags); > show_free_areas_core(pgdat); > - spin_unlock_irqrestore(&node_lock, flags); > } > > /* > @@ -106,11 +102,9 @@ > #ifdef CONFIG_NUMA > temp = NODE_DATA(numa_node_id()); > #else > - spin_lock_irqsave(&node_lock, flags); > if (!next) next = pgdat_list; > temp = next; > next = next->node_next; > - spin_unlock_irqrestore(&node_lock, flags); > #endif > start = temp; > while (temp) { > __________________________________________________ Do You Yahoo!? Yahoo! Autos - Get free new car price quotes http://autos.yahoo.com -- 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/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Useless locking in mm/numa.c 2002-07-19 18:36 ` Kanoj Sarcar @ 2002-07-19 18:38 ` William Lee Irwin III 2002-07-19 18:40 ` Kanoj Sarcar 2002-07-19 20:30 ` Matthew Dobson 1 sibling, 1 reply; 6+ messages in thread From: William Lee Irwin III @ 2002-07-19 18:38 UTC (permalink / raw) To: Kanoj Sarcar Cc: colpatch, Andrew Morton, Martin Bligh, linux-mm, Michael Hohnbaum On Fri, Jul 19, 2002 at 11:36:46AM -0700, Kanoj Sarcar wrote: > I think I put in the locks in the initial version of > the file becase the idea was that > show_free_areas_node() could be invoked from any cpu > in a multinode system (via the sysrq keys or other > intr sources), and the spin lock would provide > sanity in the print out. > For nonnuma discontig machines, isn't the spin lock > providing protection in the pgdat list chain walking > in _alloc_pages()? > Kanoj Since I just posted a patch removing the entire function, exactly where is this called from? A grep of current 2.5 shows that it's never called from anywhere. Cheers, Bill -- 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/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Useless locking in mm/numa.c 2002-07-19 18:38 ` William Lee Irwin III @ 2002-07-19 18:40 ` Kanoj Sarcar 0 siblings, 0 replies; 6+ messages in thread From: Kanoj Sarcar @ 2002-07-19 18:40 UTC (permalink / raw) To: William Lee Irwin III Cc: colpatch, Andrew Morton, Martin Bligh, linux-mm, Michael Hohnbaum --- William Lee Irwin III <wli@holomorphy.com> wrote: > On Fri, Jul 19, 2002 at 11:36:46AM -0700, Kanoj > Sarcar wrote: > > I think I put in the locks in the initial version > of > > the file becase the idea was that > > show_free_areas_node() could be invoked from any > cpu > > in a multinode system (via the sysrq keys or other > > intr sources), and the spin lock would provide > > sanity in the print out. > > For nonnuma discontig machines, isn't the spin > lock > > providing protection in the pgdat list chain > walking > > in _alloc_pages()? > > Kanoj > > Since I just posted a patch removing the entire > function, exactly > where is this called from? A grep of current 2.5 > shows that it's > never called from anywhere. > > > Cheers, > Bill Ok. I was just pointing out why the lock was added initially. The reasons might not make sense anymore. Kanoj __________________________________________________ Do You Yahoo!? Yahoo! Autos - Get free new car price quotes http://autos.yahoo.com -- 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/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Useless locking in mm/numa.c 2002-07-19 18:36 ` Kanoj Sarcar 2002-07-19 18:38 ` William Lee Irwin III @ 2002-07-19 20:30 ` Matthew Dobson 2002-07-19 21:45 ` Matthew Dobson 1 sibling, 1 reply; 6+ messages in thread From: Matthew Dobson @ 2002-07-19 20:30 UTC (permalink / raw) To: Kanoj Sarcar; +Cc: Andrew Morton, Martin Bligh, linux-mm, Michael Hohnbaum [-- Attachment #1: Type: text/plain, Size: 2334 bytes --] Kanoj Sarcar wrote: > I think I put in the locks in the initial version of > the file becase the idea was that > show_free_areas_node() could be invoked from any cpu > in a multinode system (via the sysrq keys or other > intr sources), and the spin lock would provide > sanity in the print out. As Bill mentioned, a grep through the source shows that show_free_areas_node() is never called, and since it boils down to *just* a call to show_free_areas_core() w/out the locking, the revised patch pulls it out entirely. > For nonnuma discontig machines, isn't the spin lock > providing protection in the pgdat list chain walking > in _alloc_pages()? Uhh... kinda? Since *next is static, it means that at best case, 2 processes walking the pgdat_list chain will hip-hop over nodes... If it is racy code, the *best* that lock is currently doing is making it mildy less racy, and at worst, hiding the fact that there is a race there. I'm sure the lock was useful at some point, but it no longer is... Attatched is the new version, please apply.. Cheers! -Matt > > Kanoj > > --- Matthew Dobson <colpatch@us.ibm.com> wrote: > >>There is a lock that is apparently protecting >>nothing. The node_lock spinlock >>in mm/numa.c is protecting read-only accesses to >>pgdat_list. Here is a patch >>to get rid of it. >> >>Cheers! >> >>-Matt >> >>>--- linux-2.5.26-vanilla/mm/numa.c Tue Jul 16 >> >>16:49:30 2002 >>+++ linux-2.5.26-vanilla/mm/numa.c.fixed Thu Jul 18 >>17:59:35 2002 >>@@ -44,15 +44,11 @@ >> >> #define LONG_ALIGN(x) >>(((x)+(sizeof(long))-1)&~((sizeof(long))-1)) >> >>-static spinlock_t node_lock = SPIN_LOCK_UNLOCKED; >>- >> void show_free_areas_node(pg_data_t *pgdat) >> { >> unsigned long flags; >> >>- spin_lock_irqsave(&node_lock, flags); >> show_free_areas_core(pgdat); >>- spin_unlock_irqrestore(&node_lock, flags); >> } >> >> /* >>@@ -106,11 +102,9 @@ >> #ifdef CONFIG_NUMA >> temp = NODE_DATA(numa_node_id()); >> #else >>- spin_lock_irqsave(&node_lock, flags); >> if (!next) next = pgdat_list; >> temp = next; >> next = next->node_next; >>- spin_unlock_irqrestore(&node_lock, flags); >> #endif >> start = temp; >> while (temp) { >> > > > > __________________________________________________ > Do You Yahoo!? > Yahoo! Autos - Get free new car price quotes > http://autos.yahoo.com > [-- Attachment #2: node_lock.patch --] [-- Type: text/plain, Size: 843 bytes --] --- linux-2.5.26-vanilla/mm/numa.c Tue Jul 16 16:49:30 2002 +++ linux-2.5.26-vanilla/mm/numa.c.fixed Thu Jul 18 17:59:35 2002 @@ -43,17 +43,6 @@ #ifdef CONFIG_DISCONTIGMEM #define LONG_ALIGN(x) (((x)+(sizeof(long))-1)&~((sizeof(long))-1)) - -static spinlock_t node_lock = SPIN_LOCK_UNLOCKED; - -void show_free_areas_node(pg_data_t *pgdat) -{ - unsigned long flags; - - spin_lock_irqsave(&node_lock, flags); - show_free_areas_core(pgdat); - spin_unlock_irqrestore(&node_lock, flags); -} /* * Nodes can be initialized parallely, in no particular order. @@ -106,11 +103,9 @@ #ifdef CONFIG_NUMA temp = NODE_DATA(numa_node_id()); #else - spin_lock_irqsave(&node_lock, flags); if (!next) next = pgdat_list; temp = next; next = next->node_next; - spin_unlock_irqrestore(&node_lock, flags); #endif start = temp; while (temp) { ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Useless locking in mm/numa.c 2002-07-19 20:30 ` Matthew Dobson @ 2002-07-19 21:45 ` Matthew Dobson 0 siblings, 0 replies; 6+ messages in thread From: Matthew Dobson @ 2002-07-19 21:45 UTC (permalink / raw) To: linux-mm; +Cc: Kanoj Sarcar, Andrew Morton, Martin Bligh, Michael Hohnbaum [-- Attachment #1: Type: text/plain, Size: 3642 bytes --] Whoops! wli pointed out that I forgot a reference to show_free_areas_node() in mm.h. This patch should remedy that! Cheers! -Matt Matthew Dobson wrote: > Kanoj Sarcar wrote: > >> I think I put in the locks in the initial version of >> the file becase the idea was that show_free_areas_node() could be >> invoked from any cpu >> in a multinode system (via the sysrq keys or other >> intr sources), and the spin lock would provide sanity in the print out. > > As Bill mentioned, a grep through the source shows that > show_free_areas_node() is never called, and since it boils down to > *just* a call to show_free_areas_core() w/out the locking, the revised > patch pulls it out entirely. > >> For nonnuma discontig machines, isn't the spin lock >> providing protection in the pgdat list chain walking >> in _alloc_pages()? > > Uhh... kinda? Since *next is static, it means that at best case, 2 > processes walking the pgdat_list chain will hip-hop over nodes... If it > is racy code, the *best* that lock is currently doing is making it mildy > less racy, and at worst, hiding the fact that there is a race there. > > I'm sure the lock was useful at some point, but it no longer is... > Attatched is the new version, please apply.. > > Cheers! > > -Matt > >> >> Kanoj >> >> --- Matthew Dobson <colpatch@us.ibm.com> wrote: >> >>> There is a lock that is apparently protecting >>> nothing. The node_lock spinlock in mm/numa.c is protecting read-only >>> accesses to >>> pgdat_list. Here is a patch to get rid of it. >>> >>> Cheers! >>> >>> -Matt >>> >>>> --- linux-2.5.26-vanilla/mm/numa.c Tue Jul 16 >>> >>> >>> 16:49:30 2002 >>> +++ linux-2.5.26-vanilla/mm/numa.c.fixed Thu Jul 18 >>> 17:59:35 2002 >>> @@ -44,15 +44,11 @@ >>> >>> #define LONG_ALIGN(x) >>> (((x)+(sizeof(long))-1)&~((sizeof(long))-1)) >>> >>> -static spinlock_t node_lock = SPIN_LOCK_UNLOCKED; >>> - >>> void show_free_areas_node(pg_data_t *pgdat) >>> { >>> unsigned long flags; >>> >>> - spin_lock_irqsave(&node_lock, flags); >>> show_free_areas_core(pgdat); >>> - spin_unlock_irqrestore(&node_lock, flags); >>> } >>> >>> /* >>> @@ -106,11 +102,9 @@ >>> #ifdef CONFIG_NUMA >>> temp = NODE_DATA(numa_node_id()); >>> #else >>> - spin_lock_irqsave(&node_lock, flags); >>> if (!next) next = pgdat_list; >>> temp = next; >>> next = next->node_next; >>> - spin_unlock_irqrestore(&node_lock, flags); >>> #endif >>> start = temp; >>> while (temp) { >>> >> >> >> >> __________________________________________________ >> Do You Yahoo!? >> Yahoo! Autos - Get free new car price quotes >> http://autos.yahoo.com >> > > > ------------------------------------------------------------------------ > > --- linux-2.5.26-vanilla/mm/numa.c Tue Jul 16 16:49:30 2002 > +++ linux-2.5.26-vanilla/mm/numa.c.fixed Thu Jul 18 17:59:35 2002 > @@ -43,17 +43,6 @@ > #ifdef CONFIG_DISCONTIGMEM > > #define LONG_ALIGN(x) (((x)+(sizeof(long))-1)&~((sizeof(long))-1)) > - > -static spinlock_t node_lock = SPIN_LOCK_UNLOCKED; > - > -void show_free_areas_node(pg_data_t *pgdat) > -{ > - unsigned long flags; > - > - spin_lock_irqsave(&node_lock, flags); > - show_free_areas_core(pgdat); > - spin_unlock_irqrestore(&node_lock, flags); > -} > > /* > * Nodes can be initialized parallely, in no particular order. > @@ -106,11 +103,9 @@ > #ifdef CONFIG_NUMA > temp = NODE_DATA(numa_node_id()); > #else > - spin_lock_irqsave(&node_lock, flags); > if (!next) next = pgdat_list; > temp = next; > next = next->node_next; > - spin_unlock_irqrestore(&node_lock, flags); > #endif > start = temp; > while (temp) { [-- Attachment #2: node_lock.patch --] [-- Type: text/plain, Size: 1266 bytes --] --- linux-2.5.26-vanilla/mm/numa.c Tue Jul 16 16:49:30 2002 +++ linux-2.5.26-vanilla/mm/numa.c.fixed Thu Jul 18 17:59:35 2002 @@ -43,17 +43,6 @@ #ifdef CONFIG_DISCONTIGMEM #define LONG_ALIGN(x) (((x)+(sizeof(long))-1)&~((sizeof(long))-1)) - -static spinlock_t node_lock = SPIN_LOCK_UNLOCKED; - -void show_free_areas_node(pg_data_t *pgdat) -{ - unsigned long flags; - - spin_lock_irqsave(&node_lock, flags); - show_free_areas_core(pgdat); - spin_unlock_irqrestore(&node_lock, flags); -} /* * Nodes can be initialized parallely, in no particular order. @@ -106,11 +103,9 @@ #ifdef CONFIG_NUMA temp = NODE_DATA(numa_node_id()); #else - spin_lock_irqsave(&node_lock, flags); if (!next) next = pgdat_list; temp = next; next = next->node_next; - spin_unlock_irqrestore(&node_lock, flags); #endif start = temp; while (temp) { --- linux-2.5.26-vanilla/include/linux/mm.h Tue Jul 16 16:49:24 2002 +++ linux-2.5.26-vanilla/include/linux/mm.h.fixed Fri Jul 19 14:40:29 2002 @@ -319,7 +319,6 @@ extern struct page *mem_map; extern void show_free_areas(void); -extern void show_free_areas_node(pg_data_t *pgdat); extern int fail_writepage(struct page *); struct page * shmem_nopage(struct vm_area_struct * vma, unsigned long address, int unused); ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2002-07-19 21:45 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2002-07-19 1:03 [patch] Useless locking in mm/numa.c Matthew Dobson 2002-07-19 18:36 ` Kanoj Sarcar 2002-07-19 18:38 ` William Lee Irwin III 2002-07-19 18:40 ` Kanoj Sarcar 2002-07-19 20:30 ` Matthew Dobson 2002-07-19 21:45 ` Matthew Dobson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox