exit: fix oops in sync_mm_rss

PATCH In review Michael S. Tsirkin 6 participants 17 messages Mar 16, 2010 – Mar 31, 2010

Subsystems (4)

Memory management MM core Scheduler exec and binfmt

lore

Share & tools

lore

Michael S. Tsirkin

author

2010-03-16 17:12 UTC #0 lore raw

PATCH 1/1 exit: fix oops in sync_mm_rss

In 2.6.34-rc1, removing vhost_net module causes an oops in sync_mm_rss (called from do_exit) when workqueue is destroyed. This does not happen on net-next, or with vhost on top of to 2.6.33.

The issue seems to be introduced by 34e55232e59f7b19050267a05ff1226e5cd122a5: that commit added function sync_mm_rss that is passed task->mm, and dereferences it without checking. If task is a kernel thread, mm might be NULL.
I think this might also happen e.g. with aio.

This patch fixes the oops by calling sync_mm_rss when task->mm is set to NULL. I also added BUG_ON to detect any other cases where counters get incremented while mm is NULL.

The oops I observed looks like this:

BUG: unable to handle kernel NULL pointer dereference at 00000000000002a8
IP: [<ffffffff810b436d>] sync_mm_rss+0x33/0x6f
PGD 0
Oops: 0002 [#1] SMP
last sysfs file: /sys/devices/system/cpu/cpu7/cache/index2/shared_cpu_map
CPU 2
Modules linked in: vhost_net(-) tun bridge stp sunrpc ipv6 cpufreq_ondemand acpi_cpufreq freq_table kvm_intel kvm i5000_edac edac_core rtc_cmos bnx2 button i2c_i801 i2c_core rtc_core e1000e sg joydev ide_cd_mod serio_raw pcspkr rtc_lib cdrom virtio_net virtio_blk virtio_pci virtio_ring virtio af_packet e1000 shpchp aacraid uhci_hcd ohci_hcd ehci_hcd [last unloaded: microcode]

Pid: 2046, comm: vhost Not tainted 2.6.34-rc1-vhost #25 System Planar/IBM System x3550 -[7978B3G]-

RIP: 0010:[<ffffffff810b436d>]  [<ffffffff810b436d>] sync_mm_rss+0x33/0x6f
RSP: 0018:ffff8802379b7e60  EFLAGS: 00010202
RAX: 0000000000000008 RBX: ffff88023f2390c0 RCX: 0000000000000000
RDX: ffff88023f2396b0 RSI: 0000000000000000 RDI: ffff88023f2390c0
RBP: ffff8802379b7e60 R08: 0000000000000000 R09: 0000000000000000
R10: ffff88023aecfbc0 R11: 0000000000013240 R12: 0000000000000000
R13: ffffffff81051a6c R14: ffffe8ffffc0f540 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff880001e80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00000000000002a8 CR3: 000000023af23000 CR4: 00000000000406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process vhost (pid: 2046, threadinfo ffff8802379b6000, task ffff88023f2390c0)
Stack:
 ffff8802379b7ee0 ffffffff81040687 ffffe8ffffc0f558 ffffffffa00a3e2d
<0> 0000000000000000 ffff88023f2390c0 ffffffff81055817 ffff8802379b7e98
<0> ffff8802379b7e98 0000000100000286 ffff8802379b7ee0 ffff88023ad47d78
Call Trace:
 [<ffffffff81040687>] do_exit+0x147/0x6c4
 [<ffffffffa00a3e2d>] ? handle_rx_net+0x0/0x17 [vhost_net]
 [<ffffffff81055817>] ? autoremove_wake_function+0x0/0x39
 [<ffffffff81051a6c>] ? worker_thread+0x0/0x229
 [<ffffffff810553c9>] kthreadd+0x0/0xf2
 [<ffffffff810038d4>] kernel_thread_helper+0x4/0x10
 [<ffffffff81055342>] ? kthread+0x0/0x87
 [<ffffffff810038d0>] ? kernel_thread_helper+0x0/0x10
Code: 00 8b 87 6c 02 00 00 85 c0 74 14 48 98 f0 48 01 86 a0 02 00 00 c7 87 6c 02 00 00 00 00 00 00 8b 87 70 02 00 00 85 c0 74 14 48 98 <f0> 48 01 86 a8 02 00 00 c7 87 70 02 00 00 00 00 00 00 8b 87 74
RIP  [<ffffffff810b436d>] sync_mm_rss+0x33/0x6f
 RSP <ffff8802379b7e60>
CR2: 00000000000002a8
---[ end trace 41603ba922beddd2 ]---

Fixing recursive fault but reboot is needed!

(note: handle_rx_net is a work item using workqueue in question). sync_mm_rss+0x33/0x6f gave me a hint. I also tried reverting 34e55232e59f7b19050267a05ff1226e5cd122a5 and the oops goes away.

The module in question calls use_mm and later unuse_mm from a kernel thread. It is when this kernel thread is destroyed that the crash happens.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 mm/memory.c      |    1 +
 mm/mmu_context.c |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

mm/memory.c +1 -0

index d1153e3..27022b3 100644
@@ -130,6 +130,7 @@ void __sync_task_rss_stat(struct task_struct *task, struct mm_struct *mm)
 
 	for (i = 0; i < NR_MM_COUNTERS; i++) {
 		if (task->rss_stat.count[i]) {
+			BUG_ON(!mm);
 			add_mm_counter(mm, i, task->rss_stat.count[i]);
 			task->rss_stat.count[i] = 0;
 		}

mm/mmu_context.c +1 -0

index 0777654..9e82e93 100644
@@ -53,6 +53,7 @@ void unuse_mm(struct mm_struct *mm)
 	struct task_struct *tsk = current;
 
 	task_lock(tsk);
+	sync_mm_rss(tsk, mm);
 	tsk->mm = NULL;
 	/* active_mm is still 'mm' */
 	enter_lazy_tlb(mm, tsk);

Signature · 7 lines

1.7.0.18.g0d53a5

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

Andrea Arcangeli

2010-03-16 17:52 UTC #1 lore raw

Quoted 3 lines · Michael S. Tsirkin

The module in question calls use_mm and later unuse_mm from a kernel thread. It is when this kernel thread is destroyed that the crash happens.

Looks good to me so the stats are transferred to mm before we lose track of it.

Ack!

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

Rik van Riel

Reviewed-by on v1 1/1

2010-03-16 17:54 UTC #2 lore raw

In reply to Michael S. Tsirkin

Quoted 9 lines · Michael S. Tsirkin

(note: handle_rx_net is a work item using workqueue in question). sync_mm_rss+0x33/0x6f gave me a hint. I also tried reverting 34e55232e59f7b19050267a05ff1226e5cd122a5 and the oops goes away.
The module in question calls use_mm and later unuse_mm from a kernel thread. It is when this kernel thread is destroyed that the crash happens.
Signed-off-by Michael S. Tsirkin<[email protected]>

Reviewed-by Rik van Riel <[email protected]>

KAMEZAWA Hiroyuki

Reviewed-by on v1 1/1

2010-03-16 23:45 UTC #3 lore raw

In reply to Michael S. Tsirkin

Quoted 62 lines · On Tue, 16 Mar 2010 19:08:08 +0200 "Michael S. Tsirkin" <[email protected]> wrote:

In 2.6.34-rc1, removing vhost_net module causes an oops in sync_mm_rss (called from do_exit) when workqueue is destroyed. This does not happen on net-next, or with vhost on top of to 2.6.33.

This patch fixes the oops by calling sync_mm_rss when task->mm is set to NULL. I also added BUG_ON to detect any other cases where counters get incremented while mm is NULL.

The oops I observed looks like this:

BUG: unable to handle kernel NULL pointer dereference at 00000000000002a8
IP: [<ffffffff810b436d>] sync_mm_rss+0x33/0x6f
PGD 0
Oops: 0002 [#1] SMP
last sysfs file: /sys/devices/system/cpu/cpu7/cache/index2/shared_cpu_map
CPU 2
Modules linked in: vhost_net(-) tun bridge stp sunrpc ipv6 cpufreq_ondemand acpi_cpufreq freq_table kvm_intel kvm i5000_edac edac_core rtc_cmos bnx2 button i2c_i801 i2c_core rtc_core e1000e sg joydev ide_cd_mod serio_raw pcspkr rtc_lib cdrom virtio_net virtio_blk virtio_pci virtio_ring virtio af_packet e1000 shpchp aacraid uhci_hcd ohci_hcd ehci_hcd [last unloaded: microcode]

Pid: 2046, comm: vhost Not tainted 2.6.34-rc1-vhost #25 System Planar/IBM System x3550 -[7978B3G]-

RIP: 0010:[<ffffffff810b436d>]  [<ffffffff810b436d>] sync_mm_rss+0x33/0x6f
RSP: 0018:ffff8802379b7e60  EFLAGS: 00010202
RAX: 0000000000000008 RBX: ffff88023f2390c0 RCX: 0000000000000000
RDX: ffff88023f2396b0 RSI: 0000000000000000 RDI: ffff88023f2390c0
RBP: ffff8802379b7e60 R08: 0000000000000000 R09: 0000000000000000
R10: ffff88023aecfbc0 R11: 0000000000013240 R12: 0000000000000000
R13: ffffffff81051a6c R14: ffffe8ffffc0f540 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff880001e80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00000000000002a8 CR3: 000000023af23000 CR4: 00000000000406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process vhost (pid: 2046, threadinfo ffff8802379b6000, task ffff88023f2390c0)
Stack:
 ffff8802379b7ee0 ffffffff81040687 ffffe8ffffc0f558 ffffffffa00a3e2d
<0> 0000000000000000 ffff88023f2390c0 ffffffff81055817 ffff8802379b7e98
<0> ffff8802379b7e98 0000000100000286 ffff8802379b7ee0 ffff88023ad47d78
Call Trace:
 [<ffffffff81040687>] do_exit+0x147/0x6c4
 [<ffffffffa00a3e2d>] ? handle_rx_net+0x0/0x17 [vhost_net]
 [<ffffffff81055817>] ? autoremove_wake_function+0x0/0x39
 [<ffffffff81051a6c>] ? worker_thread+0x0/0x229
 [<ffffffff810553c9>] kthreadd+0x0/0xf2
 [<ffffffff810038d4>] kernel_thread_helper+0x4/0x10
 [<ffffffff81055342>] ? kthread+0x0/0x87
 [<ffffffff810038d0>] ? kernel_thread_helper+0x0/0x10
Code: 00 8b 87 6c 02 00 00 85 c0 74 14 48 98 f0 48 01 86 a0 02 00 00 c7 87 6c 02 00 00 00 00 00 00 8b 87 70 02 00 00 85 c0 74 14 48 98 <f0> 48 01 86 a8 02 00 00 c7 87 70 02 00 00 00 00 00 00 8b 87 74
RIP  [<ffffffff810b436d>] sync_mm_rss+0x33/0x6f
 RSP <ffff8802379b7e60>
CR2: 00000000000002a8
---[ end trace 41603ba922beddd2 ]---

Fixing recursive fault but reboot is needed!

(note: handle_rx_net is a work item using workqueue in question). sync_mm_rss+0x33/0x6f gave me a hint. I also tried reverting 34e55232e59f7b19050267a05ff1226e5cd122a5 and the oops goes away.

Quoted 5 lines

The module in question calls use_mm and later unuse_mm from a kernel thread. It is when this kernel thread is destroyed that the crash happens.
Signed-off-by Michael S. Tsirkin <[email protected]>

Thank you very much.

Reviewed-by KAMEZAWA Hiroyuki <[email protected]>

Quoted 32 lines

---
 mm/memory.c      |    1 +
 mm/mmu_context.c |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

mm/memory.c +1 -0

index d1153e3..27022b3 100644
@@ -130,6 +130,7 @@ void __sync_task_rss_stat(struct task_struct *task, struct mm_struct *mm)
 
 	for (i = 0; i < NR_MM_COUNTERS; i++) {
 		if (task->rss_stat.count[i]) {
+			BUG_ON(!mm);
 			add_mm_counter(mm, i, task->rss_stat.count[i]);
 			task->rss_stat.count[i] = 0;
 		}

mm/mmu_context.c +1 -0

index 0777654..9e82e93 100644
@@ -53,6 +53,7 @@ void unuse_mm(struct mm_struct *mm)
 	struct task_struct *tsk = current;
 
 	task_lock(tsk);
+	sync_mm_rss(tsk, mm);
 	tsk->mm = NULL;
 	/* active_mm is still 'mm' */
 	enter_lazy_tlb(mm, tsk);

Signature · 1 line

1.7.0.18.g0d53a5

Minchan Kim

Reviewed-by on v1 1/1

2010-03-17 02:26 UTC #4 lore raw

In reply to Michael S. Tsirkin

Quoted 62 lines · Michael S. Tsirkin

In 2.6.34-rc1, removing vhost_net module causes an oops in sync_mm_rss (called from do_exit) when workqueue is destroyed. This does not happen on net-next, or with vhost on top of to 2.6.33.
The issue seems to be introduced by 34e55232e59f7b19050267a05ff1226e5cd122a5: that commit added function sync_mm_rss that is passed task->mm, and dereferences it without checking. If task is a kernel thread, mm might be NULL.
I think this might also happen e.g. with aio.
This patch fixes the oops by calling sync_mm_rss when task->mm is set to NULL. I also added BUG_ON to detect any other cases where counters get incremented while mm is NULL.
The oops I observed looks like this:
BUG: unable to handle kernel NULL pointer dereference at 00000000000002a8
IP: [<ffffffff810b436d>] sync_mm_rss+0x33/0x6f
PGD 0
Oops: 0002 [#1] SMP
last sysfs file: /sys/devices/system/cpu/cpu7/cache/index2/shared_cpu_map
CPU 2
Modules linked in: vhost_net(-) tun bridge stp sunrpc ipv6 cpufreq_ondemand acpi_cpufreq freq_table kvm_intel kvm i5000_edac edac_core rtc_cmos bnx2 button i2c_i801 i2c_core rtc_core e1000e sg joydev ide_cd_mod serio_raw pcspkr rtc_lib cdrom virtio_net virtio_blk virtio_pci virtio_ring virtio af_packet e1000 shpchp aacraid uhci_hcd ohci_hcd ehci_hcd [last unloaded: microcode]
Pid: 2046, comm: vhost Not tainted 2.6.34-rc1-vhost #25 System Planar/IBM System x3550 -[7978B3G]-
RIP: 0010:[<ffffffff810b436d>]  [<ffffffff810b436d>] sync_mm_rss+0x33/0x6f
RSP: 0018:ffff8802379b7e60 EFLAGS: 00010202
RAX: 0000000000000008 RBX: ffff88023f2390c0 RCX: 0000000000000000
RDX: ffff88023f2396b0 RSI: 0000000000000000 RDI: ffff88023f2390c0
RBP: ffff8802379b7e60 R08: 0000000000000000 R09: 0000000000000000
R10: ffff88023aecfbc0 R11: 0000000000013240 R12: 0000000000000000
R13: ffffffff81051a6c R14: ffffe8ffffc0f540 R15: 0000000000000000
FS: 0000000000000000(0000) GS:ffff880001e80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00000000000002a8 CR3: 000000023af23000 CR4: 00000000000406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 Process vhost (pid: 2046, threadinfo ffff8802379b6000, task ffff88023f2390c0)
Stack:
 ffff8802379b7ee0 ffffffff81040687 ffffe8ffffc0f558 ffffffffa00a3e2d
<0> 0000000000000000 ffff88023f2390c0 ffffffff81055817 ffff8802379b7e98
<0> ffff8802379b7e98 0000000100000286 ffff8802379b7ee0 ffff88023ad47d78
Call Trace:
 [<ffffffff81040687>] do_exit+0x147/0x6c4
 [<ffffffffa00a3e2d>] ? handle_rx_net+0x0/0x17 [vhost_net]
 [<ffffffff81055817>] ? autoremove_wake_function+0x0/0x39
 [<ffffffff81051a6c>] ? worker_thread+0x0/0x229
 [<ffffffff810553c9>] kthreadd+0x0/0xf2
 [<ffffffff810038d4>] kernel_thread_helper+0x4/0x10
 [<ffffffff81055342>] ? kthread+0x0/0x87
 [<ffffffff810038d0>] ? kernel_thread_helper+0x0/0x10
Code: 00 8b 87 6c 02 00 00 85 c0 74 14 48 98 f0 48 01 86 a0 02 00 00 c7 87 6c 02 00 00 00 00 00 00 8b 87 70 02 00 00 85 c0 74 14 48 98 <f0> 48 01 86 a8 02 00 00 c7 87 70 02 00 00 00 00 00 00 8b 87 74
RIP  [<ffffffff810b436d>] sync_mm_rss+0x33/0x6f
 RSP <ffff8802379b7e60>
CR2: 00000000000002a8
---[ end trace 41603ba922beddd2 ]---
Fixing recursive fault but reboot is needed!
(note: handle_rx_net is a work item using workqueue in question). sync_mm_rss+0x33/0x6f gave me a hint. I also tried reverting 34e55232e59f7b19050267a05ff1226e5cd122a5 and the oops goes away.

Quoted 5 lines

The module in question calls use_mm and later unuse_mm from a kernel thread. It is when this kernel thread is destroyed that the crash happens.
Signed-off-by Michael S. Tsirkin <[email protected]>

Reviewed-by Minchan Kim <[email protected]>

Nice catch.

Signature · 8 lines

Kind regards,
Minchan Kim

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

Andrew Morton

maintainer

2010-03-30 20:57 UTC #5 lore raw

In reply to Michael S. Tsirkin

Quoted 89 lines · On Tue, 16 Mar 2010 19:08:08 +0200 "Michael S. Tsirkin" <[email protected]> wrote:

In 2.6.34-rc1, removing vhost_net module causes an oops in sync_mm_rss (called from do_exit) when workqueue is destroyed. This does not happen on net-next, or with vhost on top of to 2.6.33.

This patch fixes the oops by calling sync_mm_rss when task->mm is set to NULL. I also added BUG_ON to detect any other cases where counters get incremented while mm is NULL.

The oops I observed looks like this:

BUG: unable to handle kernel NULL pointer dereference at 00000000000002a8
IP: [<ffffffff810b436d>] sync_mm_rss+0x33/0x6f
PGD 0
Oops: 0002 [#1] SMP
last sysfs file: /sys/devices/system/cpu/cpu7/cache/index2/shared_cpu_map
CPU 2
Modules linked in: vhost_net(-) tun bridge stp sunrpc ipv6 cpufreq_ondemand acpi_cpufreq freq_table kvm_intel kvm i5000_edac edac_core rtc_cmos bnx2 button i2c_i801 i2c_core rtc_core e1000e sg joydev ide_cd_mod serio_raw pcspkr rtc_lib cdrom virtio_net virtio_blk virtio_pci virtio_ring virtio af_packet e1000 shpchp aacraid uhci_hcd ohci_hcd ehci_hcd [last unloaded: microcode]

Pid: 2046, comm: vhost Not tainted 2.6.34-rc1-vhost #25 System Planar/IBM System x3550 -[7978B3G]-

RIP: 0010:[<ffffffff810b436d>]  [<ffffffff810b436d>] sync_mm_rss+0x33/0x6f
RSP: 0018:ffff8802379b7e60  EFLAGS: 00010202
RAX: 0000000000000008 RBX: ffff88023f2390c0 RCX: 0000000000000000
RDX: ffff88023f2396b0 RSI: 0000000000000000 RDI: ffff88023f2390c0
RBP: ffff8802379b7e60 R08: 0000000000000000 R09: 0000000000000000
R10: ffff88023aecfbc0 R11: 0000000000013240 R12: 0000000000000000
R13: ffffffff81051a6c R14: ffffe8ffffc0f540 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff880001e80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00000000000002a8 CR3: 000000023af23000 CR4: 00000000000406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process vhost (pid: 2046, threadinfo ffff8802379b6000, task ffff88023f2390c0)
Stack:
 ffff8802379b7ee0 ffffffff81040687 ffffe8ffffc0f558 ffffffffa00a3e2d
<0> 0000000000000000 ffff88023f2390c0 ffffffff81055817 ffff8802379b7e98
<0> ffff8802379b7e98 0000000100000286 ffff8802379b7ee0 ffff88023ad47d78
Call Trace:
 [<ffffffff81040687>] do_exit+0x147/0x6c4
 [<ffffffffa00a3e2d>] ? handle_rx_net+0x0/0x17 [vhost_net]
 [<ffffffff81055817>] ? autoremove_wake_function+0x0/0x39
 [<ffffffff81051a6c>] ? worker_thread+0x0/0x229
 [<ffffffff810553c9>] kthreadd+0x0/0xf2
 [<ffffffff810038d4>] kernel_thread_helper+0x4/0x10
 [<ffffffff81055342>] ? kthread+0x0/0x87
 [<ffffffff810038d0>] ? kernel_thread_helper+0x0/0x10
Code: 00 8b 87 6c 02 00 00 85 c0 74 14 48 98 f0 48 01 86 a0 02 00 00 c7 87 6c 02 00 00 00 00 00 00 8b 87 70 02 00 00 85 c0 74 14 48 98 <f0> 48 01 86 a8 02 00 00 c7 87 70 02 00 00 00 00 00 00 8b 87 74
RIP  [<ffffffff810b436d>] sync_mm_rss+0x33/0x6f
 RSP <ffff8802379b7e60>
CR2: 00000000000002a8
---[ end trace 41603ba922beddd2 ]---

Fixing recursive fault but reboot is needed!

(note: handle_rx_net is a work item using workqueue in question). sync_mm_rss+0x33/0x6f gave me a hint. I also tried reverting 34e55232e59f7b19050267a05ff1226e5cd122a5 and the oops goes away.

The module in question calls use_mm and later unuse_mm from a kernel thread. It is when this kernel thread is destroyed that the crash happens.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 mm/memory.c      |    1 +
 mm/mmu_context.c |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

mm/memory.c +1 -0

index d1153e3..27022b3 100644
@@ -130,6 +130,7 @@ void __sync_task_rss_stat(struct task_struct *task, struct mm_struct *mm)
 
 	for (i = 0; i < NR_MM_COUNTERS; i++) {
 		if (task->rss_stat.count[i]) {
+			BUG_ON(!mm);
 			add_mm_counter(mm, i, task->rss_stat.count[i]);
 			task->rss_stat.count[i] = 0;
 		}

mm/mmu_context.c +0 -0

index 0777654..9e82e93 100644

mm/mmu_context.c void unuse_mm(struct mm_struct *mm) · line 59

Quoted 7 lines

mm/mmu_context.c +1 -0

@@ -53,6 +53,7 @@ void unuse_mm(struct mm_struct *mm)
 	struct task_struct *tsk = current;
 
 	task_lock(tsk);
+	sync_mm_rss(tsk, mm);
 	tsk->mm = NULL;
 	/* active_mm is still 'mm' */
 	enter_lazy_tlb(mm, tsk);

That new BUG_ON() is triggering in Troels's machine when a bluetooth keyboard is enabled or disabled. See
(https://bugzilla.kernel.org/show_bug.cgi?id=15648.

I guess the question is: how did a kernel thread get a non-zero task->rss_stat.count[i]? If that's expected and OK then we will need to take some kernel-thread-avoidance action there.

Could whoever fixes this please also make __sync_task_rss_stat() static.

I'll toss this over to Rafael/Maciej for tracking as a post-2.6.33 regression.

Thanks.

KAMEZAWA Hiroyuki

2010-03-31 00:32 UTC #6 lore raw

Quoted 8 lines · On Tue, 30 Mar 2010 13:56:34 -0700 Andrew Morton <[email protected]>…

That new BUG_ON() is triggering in Troels's machine when a bluetooth keyboard is enabled or disabled. See
(https://bugzilla.kernel.org/show_bug.cgi?id=15648.
I guess the question is: how did a kernel thread get a non-zero task->rss_stat.count[i]? If that's expected and OK then we will need to take some kernel-thread-avoidance action there.

It seems my fault that it's not initialized to be 0 at do_fork(), copy_process.

About do_exit, do_exit() does this check. So, tsk->mm can be NULL.

949 if (group_dead) {

 950                 hrtimer_cancel(&tsk->signal->real_timer);
 951                 exit_itimers(tsk->signal);
 952                 if (tsk->mm)

953 setmax_mm_hiwater_rss(&tsk->signal->maxrss, tsk->mm);
954 }

Quoted 3 lines

Could whoever fixes this please also make __sync_task_rss_stat() static.

Ah, yes. I should do so.

Quoted 5 lines

I'll toss this over to Rafael/Maciej for tracking as a post-2.6.33 regression.
Thanks.

task->rss_stat wasn't initialized to 0 at copy_process(). at exit, tsk->mm may be NULL.
And __sync_task_rss_stat() should be static.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 kernel/exit.c |    3 ++-
 kernel/fork.c |    3 +++
 mm/memory.c   |    2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)

kernel/exit.c +2 -1

Index: mmotm-2.6.34-Mar24/kernel/exit.c
===================================================================
@@ -950,7 +950,8 @@ NORET_TYPE void do_exit(long code)
 
 	acct_update_integrals(tsk);
 	/* sync mm's RSS info before statistics gathering */
-	sync_mm_rss(tsk, tsk->mm);
+	if (tsk->mm)
+		sync_mm_rss(tsk, tsk->mm);
 	group_dead = atomic_dec_and_test(&tsk->signal->live);
 	if (group_dead) {
 		hrtimer_cancel(&tsk->signal->real_timer);

mm/memory.c +1 -1

Index: mmotm-2.6.34-Mar24/mm/memory.c
===================================================================
@@ -124,7 +124,7 @@ core_initcall(init_zero_pfn);
 
 #if defined(SPLIT_RSS_COUNTING)
 
-void __sync_task_rss_stat(struct task_struct *task, struct mm_struct *mm)
+static void __sync_task_rss_stat(struct task_struct *task, struct mm_struct *mm)
 {
 	int i;

kernel/fork.c +3 -0

Index: mmotm-2.6.34-Mar24/kernel/fork.c
===================================================================
@@ -1060,6 +1060,9 @@ static struct task_struct *copy_process(
 	p->prev_utime = cputime_zero;
 	p->prev_stime = cputime_zero;
 #endif
+#if defined(SPLIT_RSS_COUNTING)
+	memset(&p->rss_stat, 0, sizeof(p->rss_stat));
+#endif
 
 	p->default_timer_slack_ns = current->timer_slack_ns;

Andrew Morton

maintainer

2010-03-31 00:39 UTC #7 lore raw

Quoted 79 lines · On Wed, 31 Mar 2010 09:28:15 +0900 KAMEZAWA Hiroyuki…

Quoted 8 lines · On Tue, 30 Mar 2010 13:56:34 -0700 Andrew Morton <[email protected]>…
That new BUG_ON() is triggering in Troels's machine when a bluetooth keyboard is enabled or disabled. See
(https://bugzilla.kernel.org/show_bug.cgi?id=15648.
I guess the question is: how did a kernel thread get a non-zero task->rss_stat.count[i]? If that's expected and OK then we will need to take some kernel-thread-avoidance action there.
It seems my fault that it's not initialized to be 0 at do_fork(), copy_process.
About do_exit, do_exit() does this check. So, tsk->mm can be NULL.
949 if (group_dead) {
 950                 hrtimer_cancel(&tsk->signal->real_timer);
 951                 exit_itimers(tsk->signal);
 952                 if (tsk->mm)
953 setmax_mm_hiwater_rss(&tsk->signal->maxrss, tsk->mm);
954 }
Quoted 3 lines
Could whoever fixes this please also make __sync_task_rss_stat() static.
Ah, yes. I should do so.
Quoted 5 lines
I'll toss this over to Rafael/Maciej for tracking as a post-2.6.33 regression.
Thanks.
==
task->rss_stat wasn't initialized to 0 at copy_process(). at exit, tsk->mm may be NULL.
And __sync_task_rss_stat() should be static.
Signed-off-by KAMEZAWA Hiroyuki <[email protected]>
---
 kernel/exit.c |    3 ++-
 kernel/fork.c |    3 +++
 mm/memory.c   |    2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)
Index: mmotm-2.6.34-Mar24/kernel/exit.c
===================================================================
kernel/exit.c +2 -1
@@ -950,7 +950,8 @@ NORET_TYPE void do_exit(long code)
 
 	acct_update_integrals(tsk);
 	/* sync mm's RSS info before statistics gathering */
-	sync_mm_rss(tsk, tsk->mm);
+	if (tsk->mm)
+		sync_mm_rss(tsk, tsk->mm);
 	group_dead = atomic_dec_and_test(&tsk->signal->live);
 	if (group_dead) {
 		hrtimer_cancel(&tsk->signal->real_timer);
Index: mmotm-2.6.34-Mar24/mm/memory.c
===================================================================
mm/memory.c +1 -1
@@ -124,7 +124,7 @@ core_initcall(init_zero_pfn);
 
 #if defined(SPLIT_RSS_COUNTING)
 
-void __sync_task_rss_stat(struct task_struct *task, struct mm_struct *mm)
+static void __sync_task_rss_stat(struct task_struct *task, struct mm_struct *mm)
 {
 	int i;
 
Index: mmotm-2.6.34-Mar24/kernel/fork.c
===================================================================

kernel/fork.c static struct task_struct *copy_process( · line 1067

Quoted 8 lines

kernel/fork.c +3 -0

@@ -1060,6 +1060,9 @@ static struct task_struct *copy_process(
 	p->prev_utime = cputime_zero;
 	p->prev_stime = cputime_zero;
 #endif
+#if defined(SPLIT_RSS_COUNTING)
+	memset(&p->rss_stat, 0, sizeof(p->rss_stat));
+#endif
 
 	p->default_timer_slack_ns = current->timer_slack_ns;

OK, so the kenrel thread inherited a non-zero rss_stat from a userspace parent?

With this fixed, the test for non-zero tsk->mm is't really needed in do_exit(), is it? I guess it makes sense though - sync_mm_rss() only really works for kernel threads by luck..

KAMEZAWA Hiroyuki

2010-03-31 00:45 UTC #8 lore raw

Quoted 88 lines · On Tue, 30 Mar 2010 17:37:21 -0400 Andrew Morton <[email protected]>…

Quoted 86 lines · On Wed, 31 Mar 2010 09:28:15 +0900 KAMEZAWA Hiroyuki…
Quoted 8 lines · On Tue, 30 Mar 2010 13:56:34 -0700 Andrew Morton <[email protected]>…
That new BUG_ON() is triggering in Troels's machine when a bluetooth keyboard is enabled or disabled. See
(https://bugzilla.kernel.org/show_bug.cgi?id=15648.
I guess the question is: how did a kernel thread get a non-zero task->rss_stat.count[i]? If that's expected and OK then we will need to take some kernel-thread-avoidance action there.
It seems my fault that it's not initialized to be 0 at do_fork(), copy_process.
About do_exit, do_exit() does this check. So, tsk->mm can be NULL.
949 if (group_dead) {
 950                 hrtimer_cancel(&tsk->signal->real_timer);
 951                 exit_itimers(tsk->signal);
 952                 if (tsk->mm)
953 setmax_mm_hiwater_rss(&tsk->signal->maxrss, tsk->mm);
954 }
Quoted 3 lines
Could whoever fixes this please also make __sync_task_rss_stat() static.
Ah, yes. I should do so.
Quoted 5 lines
I'll toss this over to Rafael/Maciej for tracking as a post-2.6.33 regression.
Thanks.
==
task->rss_stat wasn't initialized to 0 at copy_process(). at exit, tsk->mm may be NULL.
And __sync_task_rss_stat() should be static.
Signed-off-by KAMEZAWA Hiroyuki <[email protected]>
---
 kernel/exit.c |    3 ++-
 kernel/fork.c |    3 +++
 mm/memory.c   |    2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)
Index: mmotm-2.6.34-Mar24/kernel/exit.c
===================================================================
kernel/exit.c +2 -1
@@ -950,7 +950,8 @@ NORET_TYPE void do_exit(long code)
 
 	acct_update_integrals(tsk);
 	/* sync mm's RSS info before statistics gathering */
-	sync_mm_rss(tsk, tsk->mm);
+	if (tsk->mm)
+		sync_mm_rss(tsk, tsk->mm);
 	group_dead = atomic_dec_and_test(&tsk->signal->live);
 	if (group_dead) {
 		hrtimer_cancel(&tsk->signal->real_timer);
Index: mmotm-2.6.34-Mar24/mm/memory.c
===================================================================
mm/memory.c +1 -1
@@ -124,7 +124,7 @@ core_initcall(init_zero_pfn);
 
 #if defined(SPLIT_RSS_COUNTING)
 
-void __sync_task_rss_stat(struct task_struct *task, struct mm_struct *mm)
+static void __sync_task_rss_stat(struct task_struct *task, struct mm_struct *mm)
 {
 	int i;
 
Index: mmotm-2.6.34-Mar24/kernel/fork.c
===================================================================
kernel/fork.c +3 -0
@@ -1060,6 +1060,9 @@ static struct task_struct *copy_process(
 	p->prev_utime = cputime_zero;
 	p->prev_stime = cputime_zero;
 #endif
+#if defined(SPLIT_RSS_COUNTING)
+	memset(&p->rss_stat, 0, sizeof(p->rss_stat));
+#endif
 

Quoted 5 lines

kernel/fork.c static struct task_struct *copy_process( · line 1067
Quoted 1 line
kernel/fork.c +0 -0
 	p->default_timer_slack_ns = current->timer_slack_ns;
OK, so the kenrel thread inherited a non-zero rss_stat from a userspace parent?

I think so.

Quoted 3 lines

With this fixed, the test for non-zero tsk->mm is't really needed in do_exit(), is it? I guess it makes sense though - sync_mm_rss() only really works for kernel threads by luck..

At first, I considered so, too. But I changed my mind to show "we know tsk->mm can be NULL here!" by code.
Because __sync_mm_rss_stat() has BUG_ON(!mm), the code reader will think tsk->mm shouldn't be NULL always.

Doesn't make sense ?

Thanks,
-Kame

Andrew Morton

maintainer

2010-03-31 01:24 UTC #9 lore raw

Quoted 10 lines · On Wed, 31 Mar 2010 09:41:24 +0900 KAMEZAWA Hiroyuki…

Quoted 3 lines
With this fixed, the test for non-zero tsk->mm is't really needed in do_exit(), is it? I guess it makes sense though - sync_mm_rss() only really works for kernel threads by luck..
At first, I considered so, too. But I changed my mind to show "we know tsk->mm can be NULL here!" by code.
Because __sync_mm_rss_stat() has BUG_ON(!mm), the code reader will think tsk->mm shouldn't be NULL always.
Doesn't make sense ?

uh, not really ;)

I think we should do this too:

mm/memory.c renamed from mm/memory.c~exit-fix-oops-in-sync_mm_rss-fix +0 -1

@@ -131,7 +131,6 @@ static void __sync_task_rss_stat(struct 
 
 	for (i = 0; i < NR_MM_COUNTERS; i++) {
 		if (task->rss_stat.count[i]) {
-			BUG_ON(!mm);
 			add_mm_counter(mm, i, task->rss_stat.count[i]);
 			task->rss_stat.count[i] = 0;
 		}

Because we just made sure it can't happen, and if it _does_ happen, the oops will tell us the samme thing that the BUG_ON() would have.

KAMEZAWA Hiroyuki

2010-03-31 01:31 UTC #10 lore raw

Quoted 30 lines · On Tue, 30 Mar 2010 18:22:58 -0400 Andrew Morton <[email protected]>…

Quoted 10 lines · On Wed, 31 Mar 2010 09:41:24 +0900 KAMEZAWA Hiroyuki…
Quoted 3 lines
With this fixed, the test for non-zero tsk->mm is't really needed in do_exit(), is it? I guess it makes sense though - sync_mm_rss() only really works for kernel threads by luck..
At first, I considered so, too. But I changed my mind to show "we know tsk->mm can be NULL here!" by code.
Because __sync_mm_rss_stat() has BUG_ON(!mm), the code reader will think tsk->mm shouldn't be NULL always.
Doesn't make sense ?
uh, not really ;)
I think we should do this too:
mm/memory.c renamed from mm/memory.c~exit-fix-oops-in-sync_mm_rss-fix +0 -1
@@ -131,7 +131,6 @@ static void __sync_task_rss_stat(struct 
 
 	for (i = 0; i < NR_MM_COUNTERS; i++) {
 		if (task->rss_stat.count[i]) {
-			BUG_ON(!mm);
 			add_mm_counter(mm, i, task->rss_stat.count[i]);
 			task->rss_stat.count[i] = 0;
 		}
_

Quoted 3 lines

Because we just made sure it can't happen, and if it _does_ happen, the oops will tell us the samme thing that the BUG_ON() would have.

Hmm, then, finaly..
==

task->rss_stat wasn't initialized to 0 at copy_process(). And __sync_task_rss_stat() should be static.
removed BUG_ON(!mm) in __sync_task_rss_stat() for avoiding to show wrong information to code readers. Anyway, if !mm && task->rss_stat has some value, panic will happen.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 kernel/fork.c |    3 +++
 mm/memory.c   |    3 +--
 2 files changed, 4 insertions(+), 2 deletions(-)

mm/memory.c +1 -2

Index: mmotm-2.6.34-Mar24/mm/memory.c
===================================================================
@@ -124,13 +124,12 @@ core_initcall(init_zero_pfn);
 
 #if defined(SPLIT_RSS_COUNTING)
 
-void __sync_task_rss_stat(struct task_struct *task, struct mm_struct *mm)
+static void __sync_task_rss_stat(struct task_struct *task, struct mm_struct *mm)
 {
 	int i;
 
 	for (i = 0; i < NR_MM_COUNTERS; i++) {
 		if (task->rss_stat.count[i]) {
-			BUG_ON(!mm);
 			add_mm_counter(mm, i, task->rss_stat.count[i]);
 			task->rss_stat.count[i] = 0;
 		}

kernel/fork.c +3 -0

Index: mmotm-2.6.34-Mar24/kernel/fork.c
===================================================================
@@ -1060,6 +1060,9 @@ static struct task_struct *copy_process(
 	p->prev_utime = cputime_zero;
 	p->prev_stime = cputime_zero;
 #endif
+#if defined(SPLIT_RSS_COUNTING)
+	memset(&p->rss_stat, 0, sizeof(p->rss_stat));
+#endif
 
 	p->default_timer_slack_ns = current->timer_slack_ns;

Minchan Kim

Reviewed-by

2010-03-31 02:53 UTC #11 lore raw

Quoted 40 lines · KAMEZAWA Hiroyuki

Quoted 30 lines · On Tue, 30 Mar 2010 18:22:58 -0400 Andrew Morton <[email protected]>…
Quoted 10 lines · On Wed, 31 Mar 2010 09:41:24 +0900 KAMEZAWA Hiroyuki…
Quoted 3 lines
With this fixed, the test for non-zero tsk->mm is't really needed in do_exit(), is it? I guess it makes sense though - sync_mm_rss() only really works for kernel threads by luck..
At first, I considered so, too. But I changed my mind to show "we know tsk->mm can be NULL here!" by code.
Because __sync_mm_rss_stat() has BUG_ON(!mm), the code reader will think tsk->mm shouldn't be NULL always.
Doesn't make sense ?
uh, not really ;)
I think we should do this too:
mm/memory.c renamed from mm/memory.c~exit-fix-oops-in-sync_mm_rss-fix +0 -1
@@ -131,7 +131,6 @@ static void __sync_task_rss_stat(struct
 
       for (i = 0; i < NR_MM_COUNTERS; i++) {
               if (task->rss_stat.count[i]) {
-                     BUG_ON(!mm);
                       add_mm_counter(mm, i, task->rss_stat.count[i]);
                       task->rss_stat.count[i] = 0;
               }
_
Quoted 3 lines
Because we just made sure it can't happen, and if it _does_ happen, the oops will tell us the samme thing that the BUG_ON() would have.
Hmm, then, finaly..
==

Quoted 7 lines

task->rss_stat wasn't initialized to 0 at copy_process(). And __sync_task_rss_stat() should be static.
removed BUG_ON(!mm) in __sync_task_rss_stat() for avoiding to show wrong information to code readers. Anyway, if !mm && task->rss_stat has some value, panic will happen.
Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

Reviewed-by Minchan Kim <[email protected]>

Signature · 8 lines

Kind regards,
Minchan Kim

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

Andrew Morton

maintainer

2010-03-31 03:05 UTC #12 lore raw

Quoted 6 lines · On Wed, 31 Mar 2010 11:53:00 +0900 Minchan Kim <[email protected]> wrote:

Quoted 6 lines
Quoted 6 lines
I think we should do this too:
mm/memory.c renamed from mm/memory.c~exit-fix-oops-in-sync_mm_rss-fix +0 -0
@@ -131,7 +131,6 @@ static void __sync_task_rss_stat(struct
 

Quoted 6 lines

Quoted 6 lines
mm/memory.c static void __sync_task_rss_stat(struct · line 136
Quoted 6 lines
mm/memory.c renamed from mm/memory.c~exit-fix-oops-in-sync_mm_rss-fix +0 -1
 __ __ __ for (i = 0; i < NR_MM_COUNTERS; i++) {
 __ __ __ __ __ __ __ if (task->rss_stat.count[i]) {
- __ __ __ __ __ __ __ __ __ __ BUG_ON(!mm);
 __ __ __ __ __ __ __ __ __ __ __ add_mm_counter(mm, i, task->rss_stat.count[i]);
 __ __ __ __ __ __ __ __ __ __ __ task->rss_stat.count[i] = 0;
 __ __ __ __ __ __ __ }

^^ gargh, gmail.

Quoted 8 lines

Quoted 8 lines
Quoted 4 lines
Because we just made sure it can't happen, and if it _does_ happen, the oops will tell us the samme thing that the BUG_ON() would have.
Hmm, then, finaly..
==

Quoted 8 lines

Quoted 7 lines
task->rss_stat wasn't initialized to 0 at copy_process(). And __sync_task_rss_stat() should be static.
removed BUG_ON(!mm) in __sync_task_rss_stat() for avoiding to show wrong information to code readers. Anyway, if !mm && task->rss_stat has some value, panic will happen.
Signed-off-by KAMEZAWA Hiroyuki <[email protected]>
Reviewed-by Minchan Kim <[email protected]>

I think we should keep the

kernel/exit.c renamed from kernel/exit.c~exit-fix-oops-in-sync_mm_rss +2 -1

@@ -953,7 +953,8 @@ NORET_TYPE void do_exit(long code)
 
 	acct_update_integrals(tsk);
 	/* sync mm's RSS info before statistics gathering */
-	sync_mm_rss(tsk, tsk->mm);
+	if (tsk->mm)
+		sync_mm_rss(tsk, tsk->mm);
 	group_dead = atomic_dec_and_test(&tsk->signal->live);
 	if (group_dead) {
 		hrtimer_cancel(&tsk->signal->real_timer);

really. Apart from the fact that we'll otherwise perform an empty NR_MM_COUNTERS loop in __sync_task_rss_stat(), sync_mm_rss() just isn't set up to handle kernel threads. Given that the function of sync_task_mm(from, to) is to move stuff from "from" and into "to", it's daft to call it with a NULL value of `to'!

KAMEZAWA Hiroyuki

Reviewed-by Minchan Kim

2010-03-31 03:15 UTC #13 lore raw

Quoted 8 lines · On Tue, 30 Mar 2010 20:03:36 -0400 Andrew Morton <[email protected]>…

On Wed, 31 Mar 2010 11:53:00 +0900 Minchan Kim <[email protected]> wrote:
really. Apart from the fact that we'll otherwise perform an empty NR_MM_COUNTERS loop in __sync_task_rss_stat(), sync_mm_rss() just isn't set up to handle kernel threads. Given that the function of sync_task_mm(from, to) is to move stuff from "from" and into "to", it's daft to call it with a NULL value of `to'!

Updated again.

task->rss_stat wasn't initialized to 0 at copy_process(). At exit, tsk->mm may be NULL. It's not valid to call sync_mm_rss() against not exisiting mm_struct, We should check it. And __sync_task_rss_stat() should be static.
This patch also removes BUG_ON(!mm) in __sync_task_rss_stat(). The code will panic if !mm without it.

Changelog:
- removed BUG_ON()
- added check task->mm in exit.c before calling sync_mm_rss().

Reviewed-by Minchan Kim <[email protected]>

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 kernel/exit.c |    3 ++-
 kernel/fork.c |    3 +++
 mm/memory.c   |    3 +--
 3 files changed, 6 insertions(+), 3 deletions(-)

mm/memory.c +1 -2

Index: mmotm-2.6.34-Mar24/mm/memory.c
===================================================================
@@ -124,13 +124,12 @@ core_initcall(init_zero_pfn);
 
 #if defined(SPLIT_RSS_COUNTING)
 
-void __sync_task_rss_stat(struct task_struct *task, struct mm_struct *mm)
+static void __sync_task_rss_stat(struct task_struct *task, struct mm_struct *mm)
 {
 	int i;
 
 	for (i = 0; i < NR_MM_COUNTERS; i++) {
 		if (task->rss_stat.count[i]) {
-			BUG_ON(!mm);
 			add_mm_counter(mm, i, task->rss_stat.count[i]);
 			task->rss_stat.count[i] = 0;
 		}

kernel/fork.c +3 -0

Index: mmotm-2.6.34-Mar24/kernel/fork.c
===================================================================
@@ -1060,6 +1060,9 @@ static struct task_struct *copy_process(
 	p->prev_utime = cputime_zero;
 	p->prev_stime = cputime_zero;
 #endif
+#if defined(SPLIT_RSS_COUNTING)
+	memset(&p->rss_stat, 0, sizeof(p->rss_stat));
+#endif
 
 	p->default_timer_slack_ns = current->timer_slack_ns;

kernel/exit.c +2 -1

Index: mmotm-2.6.34-Mar24/kernel/exit.c
===================================================================
@@ -950,7 +950,8 @@ NORET_TYPE void do_exit(long code)
 
 	acct_update_integrals(tsk);
 	/* sync mm's RSS info before statistics gathering */
-	sync_mm_rss(tsk, tsk->mm);
+	if (tsk->mm)
+		sync_mm_rss(tsk, tsk->mm);
 	group_dead = atomic_dec_and_test(&tsk->signal->live);
 	if (group_dead) {
 		hrtimer_cancel(&tsk->signal->real_timer);

Minchan Kim

2010-03-31 01:57 UTC #14 lore raw

In reply to KAMEZAWA Hiroyuki

Quoted 102 lines · KAMEZAWA Hiroyuki

Quoted 88 lines · On Tue, 30 Mar 2010 17:37:21 -0400 Andrew Morton <[email protected]>…
Quoted 86 lines · On Wed, 31 Mar 2010 09:28:15 +0900 KAMEZAWA Hiroyuki…
Quoted 8 lines · On Tue, 30 Mar 2010 13:56:34 -0700 Andrew Morton <[email protected]>…
That new BUG_ON() is triggering in Troels's machine when a bluetooth keyboard is enabled or disabled. See
(https://bugzilla.kernel.org/show_bug.cgi?id=15648.
I guess the question is: how did a kernel thread get a non-zero task->rss_stat.count[i]? If that's expected and OK then we will need to take some kernel-thread-avoidance action there.
It seems my fault that it's not initialized to be 0 at do_fork(), copy_process.
About do_exit, do_exit() does this check. So, tsk->mm can be NULL.
949 if (group_dead) {
950 hrtimer_cancel(&tsk->signal->real_timer);
951 exit_itimers(tsk->signal);
952 if (tsk->mm)
953 setmax_mm_hiwater_rss(&tsk->signal->maxrss, tsk->mm); 954 }
Quoted 3 lines
Could whoever fixes this please also make __sync_task_rss_stat() static.
Ah, yes. I should do so.
Quoted 5 lines
I'll toss this over to Rafael/Maciej for tracking as a post-2.6.33 regression.
Thanks.
==
task->rss_stat wasn't initialized to 0 at copy_process(). at exit, tsk->mm may be NULL.
And __sync_task_rss_stat() should be static.
Signed-off-by KAMEZAWA Hiroyuki <[email protected]>
---
kernel/exit.c | 3 ++-
kernel/fork.c | 3 +++
mm/memory.c | 2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)
Index: mmotm-2.6.34-Mar24/kernel/exit.c
===================================================================
kernel/exit.c +2 -1
@@ -950,7 +950,8 @@ NORET_TYPE void do_exit(long code)
 
     acct_update_integrals(tsk);
     /* sync mm's RSS info before statistics gathering */
-   sync_mm_rss(tsk, tsk->mm);
+   if (tsk->mm)
+           sync_mm_rss(tsk, tsk->mm);
     group_dead = atomic_dec_and_test(&tsk->signal->live);
     if (group_dead) {
             hrtimer_cancel(&tsk->signal->real_timer);
Index: mmotm-2.6.34-Mar24/mm/memory.c
===================================================================
mm/memory.c +1 -1
@@ -124,7 +124,7 @@ core_initcall(init_zero_pfn);
 
  #if defined(SPLIT_RSS_COUNTING)
 
-void __sync_task_rss_stat(struct task_struct *task, struct mm_struct *mm)
+static void __sync_task_rss_stat(struct task_struct *task, struct mm_struct *mm)
  {
     int i;
 
Index: mmotm-2.6.34-Mar24/kernel/fork.c
===================================================================
kernel/fork.c +3 -0
@@ -1060,6 +1060,9 @@ static struct task_struct *copy_process(
     p->prev_utime = cputime_zero;
     p->prev_stime = cputime_zero;
  #endif
+#if defined(SPLIT_RSS_COUNTING)
+   memset(&p->rss_stat, 0, sizeof(p->rss_stat));
+#endif
 
Quoted 5 lines
kernel/fork.c static struct task_struct *copy_process( · line 1067
Quoted 1 line
kernel/fork.c +0 -0
     p->default_timer_slack_ns = current->timer_slack_ns;
OK, so the kenrel thread inherited a non-zero rss_stat from a userspace parent?
I think so.
Quoted 3 lines
With this fixed, the test for non-zero tsk->mm is't really needed in do_exit(), is it? I guess it makes sense though - sync_mm_rss() only really works for kernel threads by luck..

Quoted 7 lines

At first, I considered so, too. But I changed my mind to show "we know tsk->mm can be NULL here!" by code.
Because __sync_mm_rss_stat() has BUG_ON(!mm), the code reader will think tsk->mm shouldn't be NULL always.
Doesn't make sense ?

Nitpick.
How about moving sync_mm_rss into after check !mm of exit_mm?

Signature · 8 lines

Kind regards,
Minchan Kim

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

KAMEZAWA Hiroyuki

2010-03-31 02:15 UTC #15 lore raw

Quoted 8 lines · On Wed, 31 Mar 2010 10:57:18 +0900 Minchan Kim <[email protected]> wrote:

On Wed, Mar 31, 2010 at 9:41 AM, KAMEZAWA Hiroyuki
Quoted 2 lines
Doesn't make sense ?
Nitpick.
How about moving sync_mm_rss into after check !mm of exit_mm?

Hmm.
==

        sync_mm_rss(tsk, tsk->mm);
        group_dead = atomic_dec_and_test(&tsk->signal->live);
        if (group_dead) {
                hrtimer_cancel(&tsk->signal->real_timer);
                exit_itimers(tsk->signal);
                if (tsk->mm)
                        setmax_mm_hiwater_rss(&tsk->signal->maxrss, tsk->mm); ---(**)
        }
        acct_collect(code, group_dead);
        if (group_dead)
                tty_audit_exit();
        if (unlikely(tsk->audit_context))
                audit_free(tsk);

        tsk->exit_code = code;
        taskstats_exit(tsk, group_dead); --------(*)
	
        exit_mm(tsk);

==
task_acct routine has to handle mm information (*).
So, we have to sync somewhere before exit_mm() or tsk->mm goes to NULL.

I think taskstat is an only acct hook which gatheres mm's rss information but I placed sync_mm_rss() before all accounting routine. Anyway, sync_mm_rss() should be before (**) setmax_mm_hiwater_rss(&tsk->signal->maxrss, tsk->mm);

Thanks,
-Kame

Minchan Kim

2010-03-31 02:48 UTC #16 lore raw

Quoted 27 lines · KAMEZAWA Hiroyuki

Quoted 8 lines · On Wed, 31 Mar 2010 10:57:18 +0900 Minchan Kim <[email protected]> wrote:
On Wed, Mar 31, 2010 at 9:41 AM, KAMEZAWA Hiroyuki
Quoted 2 lines
Doesn't make sense ?
Nitpick.
How about moving sync_mm_rss into after check !mm of exit_mm?
Hmm.
==
sync_mm_rss(tsk, tsk->mm);
group_dead = atomic_dec_and_test(&tsk->signal->live);
if (group_dead) {
hrtimer_cancel(&tsk->signal->real_timer); exit_itimers(tsk->signal);
if (tsk->mm) setmax_mm_hiwater_rss(&tsk->signal->maxrss, tsk->mm); ---(**) }
acct_collect(code, group_dead);
if (group_dead)
tty_audit_exit();
if (unlikely(tsk->audit_context))
audit_free(tsk);

Quoted 6 lines

tsk->exit_code = code; taskstats_exit(tsk, group_dead); --------(*)
exit_mm(tsk);
==
task_acct routine has to handle mm information (*).

Indeed. I missed that.
Thanks, Kame.

Signature · 8 lines

Kind regards,
Minchan Kim

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