Useless locking in mm/numa.c

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

Quoted 1 line

--- 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

mm/numa.c.fixed renamed from mm/numa.c +0 -4

@@ -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);
 }
 

/*

mm/numa.c.fixed renamed from mm/numa.c +0 -0

@@ -106,11 +102,9 @@
 #ifdef CONFIG_NUMA

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

Quoted 1 line · Kanoj Sarcar

I think I put in the locks in the initial version

of

Quoted 2 lines

the file becase the idea was that
show_free_areas_node() could be invoked from any

cpu

Quoted 4 lines

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

Quoted 1 line

providing protection in the pgdat list chain

walking

Quoted 2 lines

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

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

Quoted 42 lines · --- Matthew Dobson

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

Quoted 1 line

--- 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

(quoted diff) +0 -4

@@ -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);
 }
 

/*

(quoted diff) +0 -2

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

Quoted 1 line

__________________________________________________
Do You Yahoo!?
Yahoo! Autos - Get free new car price quotes
http://autos.yahoo.com

Quoted 4 lines · Kanoj Sarcar

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.

Quoted 3 lines

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

Quoted 48 lines

Kanoj

Quoted 43 lines · --- Matthew Dobson

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

Quoted 1 line

--- 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

(quoted diff) +0 -4

@@ -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);
 }
 

/*

(quoted diff) +0 -2

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

Quoted 9 lines

Quoted 1 line

__________________________________________________
Do You Yahoo!?
Yahoo! Autos - Get free new car price quotes
http://autos.yahoo.com

------------------------------------------------------------------------

mm/numa.c.fixed renamed from mm/numa.c +0 -11

@@ -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