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

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

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