Skip to contentSkip to conversation

memory hotplug: run lru_add_drain_all() on each cpu

PATCH In review Gerald Schaefer 5 participants 11 messages Dec 3, 2008 – Dec 8, 2008 Memory management

lore
Share & tools
lore

Gerald Schaefer

author
#0 lore raw

PATCH 1/1 memory hotplug: run lru_add_drain_all() on each cpu

From: Gerald Schaefer <[email protected]>

offline_pages() calls lru_add_drain_all() followed by drain_all_pages(). While drain_all_pages() works on each cpu, lru_add_drain_all() only runs on the current cpu for architectures w/o CONFIG_NUMA. This let us run into the BUG_ON(!PageBuddy(page)) in __offline_isolated_pages() during memory hotplug stress test on s390. The page in question was still on the pcp list, because of a race with lru_add_drain_all() and drain_all_pages() on different cpus.

This is fixed with this patch by adding CONFIG_MEMORY_HOTREMOVE to the lru_add_drain_all() #ifdef, to let it run on each cpu.

Signed-off-by Gerald Schaefer <[email protected]>
---
 mm/swap.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
mm/swap.c +2 -1
Index: linux-2.6/mm/swap.c
===================================================================
@@ -299,7 +299,8 @@ void lru_add_drain(void)
 	put_cpu();
 }
 
-#if defined(CONFIG_NUMA) || defined(CONFIG_UNEVICTABLE_LRU)
+#if defined(CONFIG_NUMA) || defined(CONFIG_UNEVICTABLE_LRU) || \
+    defined(CONFIG_MEMORY_HOTREMOVE)
 static void lru_add_drain_per_cpu(struct work_struct *dummy)
 {
 	lru_add_drain();

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#1 lore raw
Quoted 3 lines · Gerald Schaefer

offline_pages() calls lru_add_drain_all() followed by drain_all_pages(). While drain_all_pages() works on each cpu, lru_add_drain_all() only runs on the current cpu for architectures w/o CONFIG_NUMA.

I'm a bit confused why this is. Is this because the LRUs are per-zone and we expected !CONFIG_NUMA systems to only have LRUs sitting on the same (only) node as the current CPU?

This doesn't make any sense, though. The pagevecs that drain_cpu_pagevecs() actually empties out are per-cpu.

Quoted 8 lines

This let us run
into the BUG_ON(!PageBuddy(page)) in __offline_isolated_pages() during memory hotplug stress test on s390. The page in question was still on the pcp list, because of a race with lru_add_drain_all() and drain_all_pages() on different cpus.

This is fixed with this patch by adding CONFIG_MEMORY_HOTREMOVE to the lru_add_drain_all() #ifdef, to let it run on each cpu.

This doesn't seem right to me. CONFIG_MEMORY_HOTREMOVE doesn't change the layout of the LRUs, unlike NUMA or UNEVICTABLE_LRU. So, I think this bug is more due to the hotremove code mis-expecting behavior out of lru_add_drain_all().

Why does this not affect the other lru_add_drain_all() users?

-- Dave

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#2 lore raw
Quoted 6 lines · On Wed, 03 Dec 2008 14:16:07 -0800 Dave Hansen <[email protected]> wrote:
Quoted 6 lines

This let us run
into the BUG_ON(!PageBuddy(page)) in __offline_isolated_pages() during memory hotplug stress test on s390. The page in question was still on the pcp list, because of a race with lru_add_drain_all() and drain_all_pages() on different cpus.

Quoted 8 lines
Quoted 2 lines

This is fixed with this patch by adding CONFIG_MEMORY_HOTREMOVE to the lru_add_drain_all() #ifdef, to let it run on each cpu.

This doesn't seem right to me. CONFIG_MEMORY_HOTREMOVE doesn't change the layout of the LRUs, unlike NUMA or UNEVICTABLE_LRU. So, I think this bug is more due to the hotremove code mis-expecting behavior out of lru_add_drain_all().

How about

#ifdef CONFIG_SMP

#else..

#endif

rather than

-#if defined(CONFIG_NUMA) || defined(CONFIG_UNEVICTABLE_LRU)
+#if defined(CONFIG_NUMA) || defined(CONFIG_UNEVICTABLE_LRU) || \

+ defined(CONFIG_MEMORY_HOTREMOVE)
...

thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#3 lore raw

PATCH 1/1 mm: remove UP version lru_add_drain_all()

(CC to Christoph Lameter and Lee Schermerhorn)

Quoted 26 lines
Quoted 6 lines · On Wed, 03 Dec 2008 14:16:07 -0800 Dave Hansen <[email protected]> wrote:
Quoted 6 lines

This let us run
into the BUG_ON(!PageBuddy(page)) in __offline_isolated_pages() during memory hotplug stress test on s390. The page in question was still on the pcp list, because of a race with lru_add_drain_all() and drain_all_pages() on different cpus.

Quoted 8 lines
Quoted 2 lines

This is fixed with this patch by adding CONFIG_MEMORY_HOTREMOVE to the lru_add_drain_all() #ifdef, to let it run on each cpu.

This doesn't seem right to me. CONFIG_MEMORY_HOTREMOVE doesn't change the layout of the LRUs, unlike NUMA or UNEVICTABLE_LRU. So, I think this bug is more due to the hotremove code mis-expecting behavior out of lru_add_drain_all().

How about

#ifdef CONFIG_SMP

#else..

#endif

rather than

Review

Quoted 4 lines
(quoted diff) +2 -1
-#if defined(CONFIG_NUMA) || defined(CONFIG_UNEVICTABLE_LRU)
+#if defined(CONFIG_NUMA) || defined(CONFIG_UNEVICTABLE_LRU) || \
+    defined(CONFIG_MEMORY_HOTREMOVE)
...

The default value of CONFIG_UNEVICTABLE_LRU is ON. Then, almost machine use CONFIG_NUMA version lru_add_drain_all().

Therefore, this config option is not so valuable.
I like simple removing.

following patch can boot on UP machine.
===
Currently, lru_add_drain_all() has two version.
(1) use schedule_on_each_cpu()
(2) don't use schedule_on_each_cpu()

Gerald Schaefer reported it doesn't works well on SMP (not NUMA) S390 machine.

offline_pages() calls lru_add_drain_all() followed by drain_all_pages(). While drain_all_pages() works on each cpu, lru_add_drain_all() only runs on the current cpu for architectures w/o CONFIG_NUMA. This let us run into the BUG_ON(!PageBuddy(page)) in __offline_isolated_pages() during memory hotplug stress test on s390. The page in question was still on the pcp list, because of a race with lru_add_drain_all() and drain_all_pages() on different cpus.

Actually, Almost machine has CONFIG_UNEVICTABLE_LRU=y. Then almost machine use
(1) version lru_add_drain_all although the machine is UP.

Then this ifdef is not valueable.
simple removing is better.

Signed-off-by KOSAKI Motohiro <[email protected]>
CC Christoph Lameter <[email protected]>
CC Lee Schermerhorn <[email protected]>
---
 mm/swap.c |   13 -------------
 1 file changed, 13 deletions(-)
mm/swap.c +0 -13
Index: b/mm/swap.c
===================================================================
@@ -299,7 +299,6 @@ void lru_add_drain(void)
 	put_cpu();
 }
 
-#if defined(CONFIG_NUMA) || defined(CONFIG_UNEVICTABLE_LRU)
 static void lru_add_drain_per_cpu(struct work_struct *dummy)
 {
 	lru_add_drain();

@@ -313,18 +312,6 @@ int lru_add_drain_all(void)
 	return schedule_on_each_cpu(lru_add_drain_per_cpu);
 }
 
-#else
-
-/*
- * Returns 0 for success
- */
-int lru_add_drain_all(void)
-{
-	lru_add_drain();
-	return 0;
-}
-#endif
-
 /*
  * Batched page_cache_release().  Decrement the reference count on all the
  * passed pages.  If it fell to zero then remove the page from the LRU and

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#4 lore raw

Re: [PATCH] mm: remove UP version lru_add_drain_all()

silly mistake.
sorry.

Quoted 5 lines
Signed-off-by KOSAKI Motohiro <[email protected]>
CC Christoph Lameter <[email protected]>
CC Lee Schermerhorn <[email protected]>
Reported-by Gerald Schaefer <[email protected]>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author Acked-by on v1 1/1
#5 lore raw

Re: [PATCH] mm: remove UP version lru_add_drain_all()

In reply to KOSAKI Motohiro
Quoted 7 lines · KOSAKI Motohiro

Then this ifdef is not valueable.
simple removing is better.

Signed-off-by KOSAKI Motohiro <[email protected]>
CC Christoph Lameter <[email protected]>
CC Lee Schermerhorn <[email protected]>

Thanks, works for me.

Acked-by Gerald Schaefer <[email protected]>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#6 lore raw
In reply to Dave Hansen
Quoted 6 lines · Dave Hansen

I'm a bit confused why this is. Is this because the LRUs are per-zone and we expected !CONFIG_NUMA systems to only have LRUs sitting on the same (only) node as the current CPU?

This doesn't make any sense, though. The pagevecs that drain_cpu_pagevecs() actually empties out are per-cpu.

Right, the pagevecs are per-cpu, independent from any CONFIG_NUMA settings, and this is exactly why I would expect that lru_add_drain_all() works on all cpus, as opposed to lru_add_drain() which works only on the current cpu.

Quoted 6 lines

This doesn't seem right to me. CONFIG_MEMORY_HOTREMOVE doesn't change the layout of the LRUs, unlike NUMA or UNEVICTABLE_LRU. So, I think this bug is more due to the hotremove code mis-expecting behavior out of lru_add_drain_all().

Why does this not affect the other lru_add_drain_all() users?

Good question, there are only a few other users and most of them were added just recently with the unevictable lru patches. The only exception is migrate_prep(), but this is only called from sys_move_pages(), which is not implemented w/o CONFIG_NUMA afaik.

As explained above, the per-cpu pagevec layout should be independent from NUMA or UNEVICTABLE_LRU, so I guess the right thing to do here is completely remove the #ifdef as in the patch from Kosaki Motohiro (or at least replace it with a CONFIG_SMP as suggested by Kamezawa Hiroyuki).

Thanks,
Gerald

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#7 lore raw
Quoted 6 lines · Gerald Schaefer

As explained above, the per-cpu pagevec layout should be independent from NUMA or UNEVICTABLE_LRU, so I guess the right thing to do here is completely remove the #ifdef as in the patch from Kosaki Motohiro (or at least replace it with a CONFIG_SMP as suggested by Kamezawa Hiroyuki).

Thanks for looking into it deeper. That CONFIG_SMP thing really does look like the right solution.

-- Dave

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#8 lore raw

CC to Lee Schermerhorn

Quoted 10 lines · Gerald Schaefer
Quoted 6 lines

As explained above, the per-cpu pagevec layout should be independent from NUMA or UNEVICTABLE_LRU, so I guess the right thing to do here is completely remove the #ifdef as in the patch from Kosaki Motohiro (or at least replace it with a CONFIG_SMP as suggested by Kamezawa Hiroyuki).

Thanks for looking into it deeper. That CONFIG_SMP thing really does look like the right solution.

Lee, Could you read this thread and explain why you add ifdef CONFIG_UNEVICTABLE_LRU? I am not sure about that Dave's proposal is safe change. (but I guess he is right)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#9 lore raw
Quoted 11 lines · KOSAKI Motohiro

CC to Lee Schermerhorn

Quoted 8 lines · Gerald Schaefer
Quoted 6 lines

As explained above, the per-cpu pagevec layout should be independent from NUMA or UNEVICTABLE_LRU, so I guess the right thing to do here is completely remove the #ifdef as in the patch from Kosaki Motohiro (or at least replace it with a CONFIG_SMP as suggested by Kamezawa Hiroyuki).

Quoted 5 lines
Quoted 2 lines

Thanks for looking into it deeper. That CONFIG_SMP thing really does look like the right solution.

Lee, Could you read this thread and explain why you add ifdef CONFIG_UNEVICTABLE_LRU? I am not sure about that Dave's proposal is safe change. (but I guess he is right)

I added that back in Patch 17/25 "Mlocked Pages are non-reclaimable" [before nonreclaimable became unevictable". I did this because "lru_add_drain_all()" was only used by numa code prior to this, and was under #ifdef CONFIG_NUMA". I called lru_add_drain_all() from __mlock_vma_pages_range() [since removed] and I wanted the nonreclaimable/unevictable mlocked pages feature to be independent of numa. So, I had to ensure that we defined the function for nonreclaimable/unevictable lru as well as numa.

Now it appears that hotplug and memcg also depend on lru_add_drain_all(), so making it depend on 'SMP looks reasonable to me.

Lee

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#10 lore raw
Quoted 12 lines
Quoted 2 lines

Lee, Could you read this thread and explain why you add ifdef CONFIG_UNEVICTABLE_LRU? I am not sure about that Dave's proposal is safe change. (but I guess he is right)

I added that back in Patch 17/25 "Mlocked Pages are non-reclaimable" [before nonreclaimable became unevictable". I did this because "lru_add_drain_all()" was only used by numa code prior to this, and was under #ifdef CONFIG_NUMA". I called lru_add_drain_all() from __mlock_vma_pages_range() [since removed] and I wanted the nonreclaimable/unevictable mlocked pages feature to be independent of numa. So, I had to ensure that we defined the function for nonreclaimable/unevictable lru as well as numa.

Quoted 2 lines

Now it appears that hotplug and memcg also depend on lru_add_drain_all(), so making it depend on 'SMP looks reasonable to me.

Thanks a lot.

I'll make that patch.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>