From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <3D38886B.4050806@us.ibm.com> Date: Fri, 19 Jul 2002 14:45:15 -0700 From: Matthew Dobson Reply-To: colpatch@us.ibm.com MIME-Version: 1.0 Subject: Re: [patch] Useless locking in mm/numa.c References: <20020719183646.32486.qmail@web14310.mail.yahoo.com> <3D387702.6010306@us.ibm.com> Content-Type: multipart/mixed; boundary="------------020408020308000404040702" Sender: owner-linux-mm@kvack.org Return-Path: To: linux-mm@kvack.org Cc: Kanoj Sarcar , Andrew Morton , Martin Bligh , Michael Hohnbaum List-ID: This is a multi-part message in MIME format. --------------020408020308000404040702 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit 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 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) { --------------020408020308000404040702 Content-Type: text/plain; name="node_lock.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="node_lock.patch" --- 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); --------------020408020308000404040702-- -- 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/