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