Re: [patch] Useless locking in mm/numa.c

[Matthew Dobson <colpatch@us.ibm.com>]

[-- 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);