boot warnings due to swap: make each swap partition have one address_space

PATCH In review Sasha Levin 4 participants 11 messages Jan 25, 2013 – Feb 4, 2013

Subsystems (2)

Memory management MM swap

lore

Share & tools

lore

Sasha Levin

author

2013-01-25 03:47 UTC #0 lore raw

Hi folks,

Commit "swap: make each swap partition have one address_space" is triggering a series of warnings on boot:

[    3.446071] ------------[ cut here ]------------
[    3.446664] WARNING: at lib/debugobjects.c:261 debug_print_object+0x8e/0xb0()
[    3.447715] ODEBUG: init active (active state 0) object type: percpu_counter hint:           (null)
[    3.450360] Modules linked in:
[    3.451593] Pid: 1, comm: swapper/0 Tainted: G        W    3.8.0-rc4-next-20130124-sasha-00004-g838a1b4 #266
[    3.454508] Call Trace:
[    3.455248]  [<ffffffff8110d1bc>] warn_slowpath_common+0x8c/0xc0
[    3.455248]  [<ffffffff8110d291>] warn_slowpath_fmt+0x41/0x50
[    3.455248]  [<ffffffff81a2bb5e>] debug_print_object+0x8e/0xb0
[    3.455248]  [<ffffffff81a2c26b>] __debug_object_init+0x20b/0x290
[    3.455248]  [<ffffffff81a2c305>] debug_object_init+0x15/0x20
[    3.455248]  [<ffffffff81a3fbed>] __percpu_counter_init+0x6d/0xe0
[    3.455248]  [<ffffffff81231bdc>] bdi_init+0x1ac/0x270
[    3.455248]  [<ffffffff8618f20b>] swap_setup+0x3b/0x87
[    3.455248]  [<ffffffff8618f257>] ? swap_setup+0x87/0x87
[    3.455248]  [<ffffffff8618f268>] kswapd_init+0x11/0x7c
[    3.455248]  [<ffffffff810020ca>] do_one_initcall+0x8a/0x180
[    3.455248]  [<ffffffff86168cfd>] do_basic_setup+0x96/0xb4
[    3.455248]  [<ffffffff861685ae>] ? loglevel+0x31/0x31
[    3.455248]  [<ffffffff861885cd>] ? sched_init_smp+0x150/0x157
[    3.455248]  [<ffffffff86168ded>] kernel_init_freeable+0xd2/0x14c
[    3.455248]  [<ffffffff83cade10>] ? rest_init+0x140/0x140
[    3.455248]  [<ffffffff83cade19>] kernel_init+0x9/0xf0
[    3.455248]  [<ffffffff83d5727c>] ret_from_fork+0x7c/0xb0
[    3.455248]  [<ffffffff83cade10>] ? rest_init+0x140/0x140
[    3.455248] ---[ end trace 0b176d5c0f21bffb ]---

I haven't looked deeper into it yet, and will do so tomorrow, unless this spew is obvious to anyone.

Thanks,
Sasha

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

Shaohua Li

2013-01-25 04:25 UTC #1 lore raw

Quoted 32 lines · Sasha Levin

Hi folks,

Commit "swap: make each swap partition have one address_space" is triggering a series of warnings on boot:

[    3.446071] ------------[ cut here ]------------
[    3.446664] WARNING: at lib/debugobjects.c:261 debug_print_object+0x8e/0xb0()
[    3.447715] ODEBUG: init active (active state 0) object type: percpu_counter hint:           (null)
[    3.450360] Modules linked in:
[    3.451593] Pid: 1, comm: swapper/0 Tainted: G        W    3.8.0-rc4-next-20130124-sasha-00004-g838a1b4 #266
[    3.454508] Call Trace:
[    3.455248]  [<ffffffff8110d1bc>] warn_slowpath_common+0x8c/0xc0
[    3.455248]  [<ffffffff8110d291>] warn_slowpath_fmt+0x41/0x50
[    3.455248]  [<ffffffff81a2bb5e>] debug_print_object+0x8e/0xb0
[    3.455248]  [<ffffffff81a2c26b>] __debug_object_init+0x20b/0x290
[    3.455248]  [<ffffffff81a2c305>] debug_object_init+0x15/0x20
[    3.455248]  [<ffffffff81a3fbed>] __percpu_counter_init+0x6d/0xe0
[    3.455248]  [<ffffffff81231bdc>] bdi_init+0x1ac/0x270
[    3.455248]  [<ffffffff8618f20b>] swap_setup+0x3b/0x87
[    3.455248]  [<ffffffff8618f257>] ? swap_setup+0x87/0x87
[    3.455248]  [<ffffffff8618f268>] kswapd_init+0x11/0x7c
[    3.455248]  [<ffffffff810020ca>] do_one_initcall+0x8a/0x180
[    3.455248]  [<ffffffff86168cfd>] do_basic_setup+0x96/0xb4
[    3.455248]  [<ffffffff861685ae>] ? loglevel+0x31/0x31
[    3.455248]  [<ffffffff861885cd>] ? sched_init_smp+0x150/0x157
[    3.455248]  [<ffffffff86168ded>] kernel_init_freeable+0xd2/0x14c
[    3.455248]  [<ffffffff83cade10>] ? rest_init+0x140/0x140
[    3.455248]  [<ffffffff83cade19>] kernel_init+0x9/0xf0
[    3.455248]  [<ffffffff83d5727c>] ret_from_fork+0x7c/0xb0
[    3.455248]  [<ffffffff83cade10>] ? rest_init+0x140/0x140
[    3.455248] ---[ end trace 0b176d5c0f21bffb ]---

Quoted 2 lines

I haven't looked deeper into it yet, and will do so tomorrow, unless this spew is obvious to anyone.

Does this one help?

Subject: give-each-swapper-space-separate-backing_dev_info

The backing_dev_info can't be shared by all swapper address space.

Reported-by Sasha Levin <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

---
 mm/swap.c       |    1 +
 mm/swap_state.c |   11 +++++++----
 2 files changed, 8 insertions(+), 4 deletions(-)

mm/swap.c +1 -0

Index: linux/mm/swap.c
===================================================================
@@ -859,6 +859,7 @@ void __init swap_setup(void)
 	int i;
 
 	for (i = 0; i < MAX_SWAPFILES; i++) {
+		swapper_spaces[i].backing_dev_info += i;
 		bdi_init(swapper_spaces[i].backing_dev_info);
 		spin_lock_init(&swapper_spaces[i].tree_lock);
 		INIT_LIST_HEAD(&swapper_spaces[i].i_mmap_nonlinear);

mm/swap_state.c +7 -4

Index: linux/mm/swap_state.c
===================================================================
@@ -31,16 +31,19 @@ static const struct address_space_operat
 	.migratepage	= migrate_page,
 };
 
-static struct backing_dev_info swap_backing_dev_info = {
-	.name		= "swap",
-	.capabilities	= BDI_CAP_NO_ACCT_AND_WRITEBACK | BDI_CAP_SWAP_BACKED,
+static struct backing_dev_info swap_backing_dev_info[MAX_SWAPFILES] = {
+	[0 ... MAX_SWAPFILES - 1] = {
+		.name		= "swap",
+		.capabilities	= BDI_CAP_NO_ACCT_AND_WRITEBACK |
+			BDI_CAP_SWAP_BACKED,
+	}
 };
 
 struct address_space swapper_spaces[MAX_SWAPFILES] = {
 	[0 ... MAX_SWAPFILES - 1] = {
 		.page_tree	= RADIX_TREE_INIT(GFP_ATOMIC|__GFP_NOWARN),
 		.a_ops		= &swap_aops,
-		.backing_dev_info = &swap_backing_dev_info,
+		.backing_dev_info = &swap_backing_dev_info[0],
 	}
 };

Sasha Levin

author

2013-01-25 23:37 UTC #2 lore raw

Quoted 33 lines · Shaohua Li

Quoted 32 lines · Sasha Levin

Hi folks,

Commit "swap: make each swap partition have one address_space" is triggering a series of warnings on boot:

[    3.446071] ------------[ cut here ]------------
[    3.446664] WARNING: at lib/debugobjects.c:261 debug_print_object+0x8e/0xb0()
[    3.447715] ODEBUG: init active (active state 0) object type: percpu_counter hint:           (null)
[    3.450360] Modules linked in:
[    3.451593] Pid: 1, comm: swapper/0 Tainted: G        W    3.8.0-rc4-next-20130124-sasha-00004-g838a1b4 #266
[    3.454508] Call Trace:
[    3.455248]  [<ffffffff8110d1bc>] warn_slowpath_common+0x8c/0xc0
[    3.455248]  [<ffffffff8110d291>] warn_slowpath_fmt+0x41/0x50
[    3.455248]  [<ffffffff81a2bb5e>] debug_print_object+0x8e/0xb0
[    3.455248]  [<ffffffff81a2c26b>] __debug_object_init+0x20b/0x290
[    3.455248]  [<ffffffff81a2c305>] debug_object_init+0x15/0x20
[    3.455248]  [<ffffffff81a3fbed>] __percpu_counter_init+0x6d/0xe0
[    3.455248]  [<ffffffff81231bdc>] bdi_init+0x1ac/0x270
[    3.455248]  [<ffffffff8618f20b>] swap_setup+0x3b/0x87
[    3.455248]  [<ffffffff8618f257>] ? swap_setup+0x87/0x87
[    3.455248]  [<ffffffff8618f268>] kswapd_init+0x11/0x7c
[    3.455248]  [<ffffffff810020ca>] do_one_initcall+0x8a/0x180
[    3.455248]  [<ffffffff86168cfd>] do_basic_setup+0x96/0xb4
[    3.455248]  [<ffffffff861685ae>] ? loglevel+0x31/0x31
[    3.455248]  [<ffffffff861885cd>] ? sched_init_smp+0x150/0x157
[    3.455248]  [<ffffffff86168ded>] kernel_init_freeable+0xd2/0x14c
[    3.455248]  [<ffffffff83cade10>] ? rest_init+0x140/0x140
[    3.455248]  [<ffffffff83cade19>] kernel_init+0x9/0xf0
[    3.455248]  [<ffffffff83d5727c>] ret_from_fork+0x7c/0xb0
[    3.455248]  [<ffffffff83cade10>] ? rest_init+0x140/0x140
[    3.455248] ---[ end trace 0b176d5c0f21bffb ]---

Quoted 4 lines

Quoted 2 lines
I haven't looked deeper into it yet, and will do so tomorrow, unless this spew is obvious to anyone.
Does this one help?

[snip]

Yup, it did. Thanks!

Thanks,
Sasha

Hugh Dickins

2013-01-27 02:16 UTC #3 lore raw

In reply to Shaohua Li

Quoted 33 lines · Shaohua Li

Quoted 32 lines · Sasha Levin

Hi folks,

Commit "swap: make each swap partition have one address_space" is triggering a series of warnings on boot:

[    3.446071] ------------[ cut here ]------------
[    3.446664] WARNING: at lib/debugobjects.c:261 debug_print_object+0x8e/0xb0()
[    3.447715] ODEBUG: init active (active state 0) object type: percpu_counter hint:           (null)
[    3.450360] Modules linked in:
[    3.451593] Pid: 1, comm: swapper/0 Tainted: G        W    3.8.0-rc4-next-20130124-sasha-00004-g838a1b4 #266
[    3.454508] Call Trace:
[    3.455248]  [<ffffffff8110d1bc>] warn_slowpath_common+0x8c/0xc0
[    3.455248]  [<ffffffff8110d291>] warn_slowpath_fmt+0x41/0x50
[    3.455248]  [<ffffffff81a2bb5e>] debug_print_object+0x8e/0xb0
[    3.455248]  [<ffffffff81a2c26b>] __debug_object_init+0x20b/0x290
[    3.455248]  [<ffffffff81a2c305>] debug_object_init+0x15/0x20
[    3.455248]  [<ffffffff81a3fbed>] __percpu_counter_init+0x6d/0xe0
[    3.455248]  [<ffffffff81231bdc>] bdi_init+0x1ac/0x270
[    3.455248]  [<ffffffff8618f20b>] swap_setup+0x3b/0x87
[    3.455248]  [<ffffffff8618f257>] ? swap_setup+0x87/0x87
[    3.455248]  [<ffffffff8618f268>] kswapd_init+0x11/0x7c
[    3.455248]  [<ffffffff810020ca>] do_one_initcall+0x8a/0x180
[    3.455248]  [<ffffffff86168cfd>] do_basic_setup+0x96/0xb4
[    3.455248]  [<ffffffff861685ae>] ? loglevel+0x31/0x31
[    3.455248]  [<ffffffff861885cd>] ? sched_init_smp+0x150/0x157
[    3.455248]  [<ffffffff86168ded>] kernel_init_freeable+0xd2/0x14c
[    3.455248]  [<ffffffff83cade10>] ? rest_init+0x140/0x140
[    3.455248]  [<ffffffff83cade19>] kernel_init+0x9/0xf0
[    3.455248]  [<ffffffff83d5727c>] ret_from_fork+0x7c/0xb0
[    3.455248]  [<ffffffff83cade10>] ? rest_init+0x140/0x140
[    3.455248] ---[ end trace 0b176d5c0f21bffb ]---

Quoted 8 lines

Quoted 2 lines
I haven't looked deeper into it yet, and will do so tomorrow, unless this spew is obvious to anyone.
Does this one help?
Subject: give-each-swapper-space-separate-backing_dev_info
The backing_dev_info can't be shared by all swapper address space.

Whyever not? It's perfectly normal for different inodes/address_spaces to share a single backing_dev! Sasha's trace says that it's wrong to initialize it MAX_SWAPFILES times: fair enough. But why should I now want to spend 32kB (not even counting their __percpu counters) on all these pseudo-backing_devs?

Hugh

p.s. a grand little change would be to move page_cluster and swap_setup() from mm/swap.c to mm/swap_state.c: they have nothing to do with the other contents of swap.c, and everything to do with the contents of swap_state.c. Why swap.c is called swap.c is rather a mystery.

Quoted 39 lines

Reported-by Sasha Levin <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

---
 mm/swap.c       |    1 +
 mm/swap_state.c |   11 +++++++----
 2 files changed, 8 insertions(+), 4 deletions(-)

Index: linux/mm/swap.c
===================================================================

mm/swap.c +1 -0

@@ -859,6 +859,7 @@ void __init swap_setup(void)
 	int i;
 
 	for (i = 0; i < MAX_SWAPFILES; i++) {
+		swapper_spaces[i].backing_dev_info += i;
 		bdi_init(swapper_spaces[i].backing_dev_info);
 		spin_lock_init(&swapper_spaces[i].tree_lock);
 		INIT_LIST_HEAD(&swapper_spaces[i].i_mmap_nonlinear);

Index: linux/mm/swap_state.c
===================================================================

mm/swap_state.c +6 -3

@@ -31,16 +31,19 @@ static const struct address_space_operat
 	.migratepage	= migrate_page,
 };
 
-static struct backing_dev_info swap_backing_dev_info = {
-	.name		= "swap",
-	.capabilities	= BDI_CAP_NO_ACCT_AND_WRITEBACK | BDI_CAP_SWAP_BACKED,
+static struct backing_dev_info swap_backing_dev_info[MAX_SWAPFILES] = {
+	[0 ... MAX_SWAPFILES - 1] = {
+		.name		= "swap",
+		.capabilities	= BDI_CAP_NO_ACCT_AND_WRITEBACK |
+			BDI_CAP_SWAP_BACKED,
+	}
 };

mm/swap_state.c static const struct address_space_operat · line 49

Quoted 10 lines

mm/swap_state.c +1 -1

 
 struct address_space swapper_spaces[MAX_SWAPFILES] = {
 	[0 ... MAX_SWAPFILES - 1] = {
 		.page_tree	= RADIX_TREE_INIT(GFP_ATOMIC|__GFP_NOWARN),
 		.a_ops		= &swap_aops,
-		.backing_dev_info = &swap_backing_dev_info,
+		.backing_dev_info = &swap_backing_dev_info[0],
 	}
 };

Shaohua Li

2013-01-27 14:13 UTC #4 lore raw

Quoted 41 lines · Hugh Dickins

Quoted 40 lines · Shaohua Li

Quoted 32 lines · Sasha Levin

Hi folks,

Commit "swap: make each swap partition have one address_space" is triggering a series of warnings on boot:

[    3.446071] ------------[ cut here ]------------
[    3.446664] WARNING: at lib/debugobjects.c:261 debug_print_object+0x8e/0xb0()
[    3.447715] ODEBUG: init active (active state 0) object type: percpu_counter hint:           (null)
[    3.450360] Modules linked in:
[    3.451593] Pid: 1, comm: swapper/0 Tainted: G        W    3.8.0-rc4-next-20130124-sasha-00004-g838a1b4 #266
[    3.454508] Call Trace:
[    3.455248]  [<ffffffff8110d1bc>] warn_slowpath_common+0x8c/0xc0
[    3.455248]  [<ffffffff8110d291>] warn_slowpath_fmt+0x41/0x50
[    3.455248]  [<ffffffff81a2bb5e>] debug_print_object+0x8e/0xb0
[    3.455248]  [<ffffffff81a2c26b>] __debug_object_init+0x20b/0x290
[    3.455248]  [<ffffffff81a2c305>] debug_object_init+0x15/0x20
[    3.455248]  [<ffffffff81a3fbed>] __percpu_counter_init+0x6d/0xe0
[    3.455248]  [<ffffffff81231bdc>] bdi_init+0x1ac/0x270
[    3.455248]  [<ffffffff8618f20b>] swap_setup+0x3b/0x87
[    3.455248]  [<ffffffff8618f257>] ? swap_setup+0x87/0x87
[    3.455248]  [<ffffffff8618f268>] kswapd_init+0x11/0x7c
[    3.455248]  [<ffffffff810020ca>] do_one_initcall+0x8a/0x180
[    3.455248]  [<ffffffff86168cfd>] do_basic_setup+0x96/0xb4
[    3.455248]  [<ffffffff861685ae>] ? loglevel+0x31/0x31
[    3.455248]  [<ffffffff861885cd>] ? sched_init_smp+0x150/0x157
[    3.455248]  [<ffffffff86168ded>] kernel_init_freeable+0xd2/0x14c
[    3.455248]  [<ffffffff83cade10>] ? rest_init+0x140/0x140
[    3.455248]  [<ffffffff83cade19>] kernel_init+0x9/0xf0
[    3.455248]  [<ffffffff83d5727c>] ret_from_fork+0x7c/0xb0
[    3.455248]  [<ffffffff83cade10>] ? rest_init+0x140/0x140
[    3.455248] ---[ end trace 0b176d5c0f21bffb ]---

Quoted 2 lines

I haven't looked deeper into it yet, and will do so tomorrow, unless this spew is obvious to anyone.

Does this one help?

Subject: give-each-swapper-space-separate-backing_dev_info

Quoted 7 lines

Quoted 1 line
The backing_dev_info can't be shared by all swapper address space.
Whyever not? It's perfectly normal for different inodes/address_spaces to share a single backing_dev! Sasha's trace says that it's wrong to initialize it MAX_SWAPFILES times: fair enough. But why should I now want to spend 32kB (not even counting their __percpu counters) on all these pseudo-backing_devs?

That's correct, silly me. Updated it.

Quoted 5 lines

p.s. a grand little change would be to move page_cluster and swap_setup() from mm/swap.c to mm/swap_state.c: they have nothing to do with the other contents of swap.c, and everything to do with the contents of swap_state.c. Why swap.c is called swap.c is rather a mystery.

Tried, but looks page_cluster is used in sysctl, moving to swap_state.c will make it optional. don't want to add another #ifdef, so give up.

Subject: init-swap-space-backing-dev-info-once

Sasha reported:
Commit "swap: make each swap partition have one address_space" is triggering a series of warnings on boot:

[    3.446071] ------------[ cut here ]------------
[    3.446664] WARNING: at lib/debugobjects.c:261 debug_print_object+0x8e/0xb0()
[    3.447715] ODEBUG: init active (active state 0) object type: percpu_counter hint:           (null)
[    3.450360] Modules linked in:
[    3.451593] Pid: 1, comm: swapper/0 Tainted: G        W    3.8.0-rc4-next-20130124-sasha-00004-g838a1b4 #266
[    3.454508] Call Trace:
[    3.455248]  [<ffffffff8110d1bc>] warn_slowpath_common+0x8c/0xc0
[    3.455248]  [<ffffffff8110d291>] warn_slowpath_fmt+0x41/0x50
[    3.455248]  [<ffffffff81a2bb5e>] debug_print_object+0x8e/0xb0
[    3.455248]  [<ffffffff81a2c26b>] __debug_object_init+0x20b/0x290
[    3.455248]  [<ffffffff81a2c305>] debug_object_init+0x15/0x20
[    3.455248]  [<ffffffff81a3fbed>] __percpu_counter_init+0x6d/0xe0
[    3.455248]  [<ffffffff81231bdc>] bdi_init+0x1ac/0x270
[    3.455248]  [<ffffffff8618f20b>] swap_setup+0x3b/0x87
[    3.455248]  [<ffffffff8618f257>] ? swap_setup+0x87/0x87
[    3.455248]  [<ffffffff8618f268>] kswapd_init+0x11/0x7c
[    3.455248]  [<ffffffff810020ca>] do_one_initcall+0x8a/0x180
[    3.455248]  [<ffffffff86168cfd>] do_basic_setup+0x96/0xb4
[    3.455248]  [<ffffffff861685ae>] ? loglevel+0x31/0x31
[    3.455248]  [<ffffffff861885cd>] ? sched_init_smp+0x150/0x157
[    3.455248]  [<ffffffff86168ded>] kernel_init_freeable+0xd2/0x14c
[    3.455248]  [<ffffffff83cade10>] ? rest_init+0x140/0x140
[    3.455248]  [<ffffffff83cade19>] kernel_init+0x9/0xf0
[    3.455248]  [<ffffffff83d5727c>] ret_from_fork+0x7c/0xb0
[    3.455248]  [<ffffffff83cade10>] ? rest_init+0x140/0x140
[    3.455248] ---[ end trace 0b176d5c0f21bffb ]---

Initialize swap space backing_dev_info once to avoid the warning.

Reported-by Sasha Levin <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

---
 mm/swap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/swap.c +1 -1

Index: linux/mm/swap.c
===================================================================
@@ -858,8 +858,8 @@ void __init swap_setup(void)
 #ifdef CONFIG_SWAP
 	int i;
 
+	bdi_init(swapper_spaces[0].backing_dev_info);
 	for (i = 0; i < MAX_SWAPFILES; i++) {
-		bdi_init(swapper_spaces[i].backing_dev_info);
 		spin_lock_init(&swapper_spaces[i].tree_lock);
 		INIT_LIST_HEAD(&swapper_spaces[i].i_mmap_nonlinear);
 	}

Hugh Dickins

2013-01-27 21:40 UTC #5 lore raw

Quoted 8 lines · Shaohua Li

Quoted 7 lines · Hugh Dickins
Quoted 5 lines · Shaohua Li
On Thu, Jan 24, 2013 at 10:45:57PM -0500, Sasha Levin wrote:
Subject: give-each-swapper-space-separate-backing_dev_info
The backing_dev_info can't be shared by all swapper address space.

Quoted 7 lines

Quoted 5 lines
Whyever not? It's perfectly normal for different inodes/address_spaces to share a single backing_dev! Sasha's trace says that it's wrong to initialize it MAX_SWAPFILES times: fair enough. But why should I now want to spend 32kB (not even counting their __percpu counters) on all these pseudo-backing_devs?
That's correct, silly me. Updated it.

Looks much more to my taste, thank you!

Quoted 8 lines

Quoted 5 lines
p.s. a grand little change would be to move page_cluster and swap_setup() from mm/swap.c to mm/swap_state.c: they have nothing to do with the other contents of swap.c, and everything to do with the contents of swap_state.c. Why swap.c is called swap.c is rather a mystery.
Tried, but looks page_cluster is used in sysctl, moving to swap_state.c will make it optional. don't want to add another #ifdef, so give up.

Good point, thanks for trying, maybe I'll attack it next time it irritates me.

I don't yet know whether I approve of your changes or not, but running with them to see (and I'll send another bugfix separately in a moment).

I was the one who removed the swap_device_lock() which 2.4 used, because it almost always ended up having to take both swap_list_lock() and swap_device_lock(si). You seem to have done a much better job of separating them usefully, but I need to convince myself that it does end up safely.

My reservations so far would be: how many installations actually have more than one swap area, so is it a good tradeoff to add more overhead to help those at the (slight) expense of everyone else? The increasingly ugly page_mapping() worries me, and the static array of swapper_spaces annoys me a little.

I'm glad Minchan has now pointed you to Rik's posting of two years ago:
I think there are more important changes to be made in that direction.

Hugh

Quoted 50 lines

Subject: init-swap-space-backing-dev-info-once

Sasha reported:
Commit "swap: make each swap partition have one address_space" is triggering a series of warnings on boot:

[    3.446071] ------------[ cut here ]------------
[    3.446664] WARNING: at lib/debugobjects.c:261 debug_print_object+0x8e/0xb0()
[    3.447715] ODEBUG: init active (active state 0) object type: percpu_counter hint:           (null)
[    3.450360] Modules linked in:
[    3.451593] Pid: 1, comm: swapper/0 Tainted: G        W    3.8.0-rc4-next-20130124-sasha-00004-g838a1b4 #266
[    3.454508] Call Trace:
[    3.455248]  [<ffffffff8110d1bc>] warn_slowpath_common+0x8c/0xc0
[    3.455248]  [<ffffffff8110d291>] warn_slowpath_fmt+0x41/0x50
[    3.455248]  [<ffffffff81a2bb5e>] debug_print_object+0x8e/0xb0
[    3.455248]  [<ffffffff81a2c26b>] __debug_object_init+0x20b/0x290
[    3.455248]  [<ffffffff81a2c305>] debug_object_init+0x15/0x20
[    3.455248]  [<ffffffff81a3fbed>] __percpu_counter_init+0x6d/0xe0
[    3.455248]  [<ffffffff81231bdc>] bdi_init+0x1ac/0x270
[    3.455248]  [<ffffffff8618f20b>] swap_setup+0x3b/0x87
[    3.455248]  [<ffffffff8618f257>] ? swap_setup+0x87/0x87
[    3.455248]  [<ffffffff8618f268>] kswapd_init+0x11/0x7c
[    3.455248]  [<ffffffff810020ca>] do_one_initcall+0x8a/0x180
[    3.455248]  [<ffffffff86168cfd>] do_basic_setup+0x96/0xb4
[    3.455248]  [<ffffffff861685ae>] ? loglevel+0x31/0x31
[    3.455248]  [<ffffffff861885cd>] ? sched_init_smp+0x150/0x157
[    3.455248]  [<ffffffff86168ded>] kernel_init_freeable+0xd2/0x14c
[    3.455248]  [<ffffffff83cade10>] ? rest_init+0x140/0x140
[    3.455248]  [<ffffffff83cade19>] kernel_init+0x9/0xf0
[    3.455248]  [<ffffffff83d5727c>] ret_from_fork+0x7c/0xb0
[    3.455248]  [<ffffffff83cade10>] ? rest_init+0x140/0x140
[    3.455248] ---[ end trace 0b176d5c0f21bffb ]---

Initialize swap space backing_dev_info once to avoid the warning.

Reported-by Sasha Levin <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

---
 mm/swap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/mm/swap.c
===================================================================

mm/swap.c void __init swap_setup(void) · line 865

Quoted 9 lines

mm/swap.c +1 -1

@@ -858,8 +858,8 @@ void __init swap_setup(void)
 #ifdef CONFIG_SWAP
 	int i;
 
+	bdi_init(swapper_spaces[0].backing_dev_info);
 	for (i = 0; i < MAX_SWAPFILES; i++) {
-		bdi_init(swapper_spaces[i].backing_dev_info);
 		spin_lock_init(&swapper_spaces[i].tree_lock);
 		INIT_LIST_HEAD(&swapper_spaces[i].i_mmap_nonlinear);
 	}

[email protected]

2013-01-29 15:58 UTC #6 lore raw

Quoted 5 lines · Hugh Dickins

My reservations so far would be: how many installations actually have more than one swap area, so is it a good tradeoff to add more overhead to help those at the (slight) expense of everyone else? The increasingly ugly page_mapping() worries me, and the static array of swapper_spaces annoys me a little.

Right now, probably few. But the number may go up a lot if the whole 'zram-for-swapspace' thing catches on and/or ships in a distro...

Shaohua Li

2013-01-30 10:00 UTC #7 lore raw

In reply to Hugh Dickins

Quoted 40 lines · Hugh Dickins

Quoted 8 lines · Shaohua Li
Quoted 7 lines · Hugh Dickins
Quoted 5 lines · Shaohua Li
On Thu, Jan 24, 2013 at 10:45:57PM -0500, Sasha Levin wrote:
Subject: give-each-swapper-space-separate-backing_dev_info
The backing_dev_info can't be shared by all swapper address space.
Quoted 7 lines
Quoted 5 lines
Whyever not? It's perfectly normal for different inodes/address_spaces to share a single backing_dev! Sasha's trace says that it's wrong to initialize it MAX_SWAPFILES times: fair enough. But why should I now want to spend 32kB (not even counting their __percpu counters) on all these pseudo-backing_devs?
That's correct, silly me. Updated it.
Looks much more to my taste, thank you!
Quoted 8 lines
Quoted 5 lines
p.s. a grand little change would be to move page_cluster and swap_setup() from mm/swap.c to mm/swap_state.c: they have nothing to do with the other contents of swap.c, and everything to do with the contents of swap_state.c. Why swap.c is called swap.c is rather a mystery.
Tried, but looks page_cluster is used in sysctl, moving to swap_state.c will make it optional. don't want to add another #ifdef, so give up.
Good point, thanks for trying, maybe I'll attack it next time it irritates me.
I don't yet know whether I approve of your changes or not, but running with them to see (and I'll send another bugfix separately in a moment).
I was the one who removed the swap_device_lock() which 2.4 used, because it almost always ended up having to take both swap_list_lock() and swap_device_lock(si). You seem to have done a much better job of separating them usefully, but I need to convince myself that it does end up safely.

Quoted 8 lines

My reservations so far would be: how many installations actually have more than one swap area, so is it a good tradeoff to add more overhead to help those at the (slight) expense of everyone else? The increasingly ugly page_mapping() worries me, and the static array of swapper_spaces annoys me a little.
I'm glad Minchan has now pointed you to Rik's posting of two years ago:
I think there are more important changes to be made in that direction.

Not sure how others use multiple swaps, but current lock contention forces us to use multiple swaps. I haven't carefully think about Rik's posting, but looks it doesn't solve the lock contention problem.

Thanks,
Shaohua

Hugh Dickins

2013-02-04 05:12 UTC #8 lore raw

Quoted 8 lines · Shaohua Li

Quoted 3 lines · Hugh Dickins
I'm glad Minchan has now pointed you to Rik's posting of two years ago:
I think there are more important changes to be made in that direction.
Not sure how others use multiple swaps, but current lock contention forces us to use multiple swaps. I haven't carefully think about Rik's posting, but looks it doesn't solve the lock contention problem.

Nobody had reported any swap lock contention problem before your patch, so no, Rik's posting wasn't directed at that. I always thought swap writing patterns a much bigger problem.

But if lock contention there is, then I think it can be implemented with reducing that in mind. There are two levels of allocation: one to allocate the tokens which we will insert in page tables, and one to allocate the final diskspace to which those tokens will point.

(I may be using totally different language from Rik, it's the principles that I have in mind, not his actual posting.)

Allocating the tokens can very well be done with per-cpu batches, perhaps of SWAP_CLUSTER_MAX 32 to match vmscan.c's batching: there is no significance to their ordering. And allocating the diskspace would want to be done in batches, to maximize contiguous writing.

That may not solve all the swap_info_get() contention which you saw, but should help some.

I'm thinking that we go with your per-swapper-space locking for now; but I wouldn't mind taking it out again later, if we arrive at a better solution which benefits even those with a single swap area.

Hugh

Hugh Dickins

2013-01-27 21:47 UTC #9 lore raw

PATCH 1/1 swap: add per-partition lock for swapfile fix

In reply to Shaohua Li

I had all cpus spinning in swap_info_get(), for the lock on an area being swapped off: probably because get_swap_page() forgot to unlock.

Signed-off-by Hugh Dickins <[email protected]>

---

 mm/swapfile.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

mm/swapfile.c +2 -3

@@ -470,10 +470,9 @@ swp_entry_t get_swap_page(void)
 		spin_unlock(&swap_lock);
 		/* This is called for allocating swap entry for cache */
 		offset = scan_swap_map(si, SWAP_HAS_CACHE);
-		if (offset) {
-			spin_unlock(&si->lock);
+		spin_unlock(&si->lock);
+		if (offset)
 			return swp_entry(type, offset);
-		}
 		spin_lock(&swap_lock);
 		next = swap_list.next;
 	}

Shaohua Li

2013-01-28 00:07 UTC #10 lore raw

Re: [PATCH next/mmotm] swap: add per-partition lock for swapfile fix

Quoted 4 lines · Hugh Dickins

I had all cpus spinning in swap_info_get(), for the lock on an area being swapped off: probably because get_swap_page() forgot to unlock.
Signed-off-by Hugh Dickins <[email protected]>

Good catch. Thanks!

Quoted 10 lines

---

 mm/swapfile.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

mm/swapfile.c +0 -0

@@ -470,10 +470,9 @@ swp_entry_t get_swap_page(void)
 		spin_unlock(&swap_lock);
 		/* This is called for allocating swap entry for cache */

mm/swapfile.c swp_entry_t get_swap_page(void) · line 478

Quoted 10 lines

mm/swapfile.c +2 -3

 		offset = scan_swap_map(si, SWAP_HAS_CACHE);
-		if (offset) {
-			spin_unlock(&si->lock);
+		spin_unlock(&si->lock);
+		if (offset)
 			return swp_entry(type, offset);
-		}
 		spin_lock(&swap_lock);
 		next = swap_list.next;
 	}