memory cgroup: update root memory cgroup when node is onlined
Subsystems (7)
PATCH 1/1 memory cgroup: update root memory cgroup when node is onlined
root_mem_cgroup->info.nodeinfo is initialized when the system boots. But NODE_DATA(nid) is null if the node is not onlined, so root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone contains an invalid pointer. If we use numactl to bind a program to the node after onlining the node and its memory, it will cause the kernel panicked:
[ 63.413436] BUG: unable to handle kernel NULL pointer dereference at 0000000000000f60 [ 63.414161] IP: [<ffffffff811870b9>] __mod_zone_page_state+0x9/0x60 [ 63.414161] PGD 0 [ 63.414161] Oops: 0000 [#1] SMP [ 63.414161] Modules linked in: acpi_memhotplug binfmt_misc dm_mirror dm_region_hash dm_log dm_mod ppdev sg microcode pcspkr virtio_console virtio_balloon snd_intel8x0 snd_ac9 7_codec ac97_bus snd_seq snd_seq_device snd_pcm snd_timer snd soundcore snd_page_alloc e1000 i2c_piix4 i2c_core floppy parport_pc parport sr_mod cdrom virtio_blk pata_acpi ata_g eneric ata_piix libata scsi_mod [ 63.414161] CPU 2 [ 63.414161] Pid: 1219, comm: numactl Not tainted 3.6.0-rc5+ #180 Bochs Bochs ... [ 63.414161] Process numactl (pid: 1219, threadinfo ffff880039abc000, task ffff8800383c4ce0) [ 63.414161] Stack: [ 63.414161] ffff880039abdaf8 ffffffff8117390f ffff880039abdaf8 000000008167c601 [ 63.414161] ffffffff81174162 ffff88003a480f00 0000000000000001 ffff8800395e0000 [ 63.414161] ffff88003dbd0e80 0000000000000282 ffff880039abdb48 ffffffff81174181 [ 63.414161] Call Trace: [ 63.414161] [<ffffffff8117390f>] __pagevec_lru_add_fn+0xdf/0x140 [ 63.414161] [<ffffffff81174162>] ? pagevec_lru_move_fn+0x92/0x100 [ 63.414161] [<ffffffff81174181>] pagevec_lru_move_fn+0xb1/0x100 [ 63.414161] [<ffffffff81173830>] ? lru_add_page_tail+0x1b0/0x1b0 [ 63.414161] [<ffffffff811dff71>] ? exec_mmap+0x121/0x230 [ 63.414161] [<ffffffff811741ec>] __pagevec_lru_add+0x1c/0x30 [ 63.414161] [<ffffffff81174383>] lru_add_drain_cpu+0xa3/0x130 [ 63.414161] [<ffffffff8117443f>] lru_add_drain+0x2f/0x40 [ 63.414161] [<ffffffff81196da9>] exit_mmap+0x69/0x160 [ 63.414161] [<ffffffff810db115>] ? lock_release_holdtime+0x35/0x1a0 [ 63.414161] [<ffffffff81069c37>] mmput+0x77/0x100 [ 63.414161] [<ffffffff811dffc0>] exec_mmap+0x170/0x230 [ 63.414161] [<ffffffff811e0152>] flush_old_exec+0xd2/0x140 [ 63.414161] [<ffffffff812343ea>] load_elf_binary+0x32a/0xe70 [ 63.414161] [<ffffffff810d700d>] ? trace_hardirqs_off+0xd/0x10 [ 63.414161] [<ffffffff810b231f>] ? local_clock+0x6f/0x80 [ 63.414161] [<ffffffff810db115>] ? lock_release_holdtime+0x35/0x1a0 [ 63.414161] [<ffffffff810dd813>] ? __lock_release+0x133/0x1a0 [ 63.414161] [<ffffffff811e22e7>] ? search_binary_handler+0x1a7/0x4a0 [ 63.414161] [<ffffffff811e22f3>] search_binary_handler+0x1b3/0x4a0 [ 63.414161] [<ffffffff811e2194>] ? search_binary_handler+0x54/0x4a0 [ 63.414161] [<ffffffff812340c0>] ? set_brk+0xe0/0xe0 [ 63.414161] [<ffffffff811e284f>] do_execve_common+0x26f/0x320 [ 63.414161] [<ffffffff811bde33>] ? kmem_cache_alloc+0x113/0x220 [ 63.414161] [<ffffffff811e298a>] do_execve+0x3a/0x40 [ 63.414161] [<ffffffff8102061a>] sys_execve+0x4a/0x80 [ 63.414161] [<ffffffff81686c6c>] stub_execve+0x6c/0xc0 [ 63.414161] Code: ff 03 00 00 48 c1 e7 0b 48 c1 e2 07 48 29 d7 48 03 3c c5 c0 27 d2 81 e8 a6 fe ff ff c9 c3 0f 1f 40 00 55 48 89 e5 0f 1f 44 00 00 <48> 8b 4f 60 89 f6 48 8d 44 31 40 65 44 8a 40 02 45 0f be c0 41
The reason is that we don't update root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone when onlining the node, and we try to access it.
--- include/linux/memcontrol.h | 7 +++++++ mm/memcontrol.c | 14 ++++++++++++++ mm/memory_hotplug.c | 2 ++ 3 files changed, 23 insertions(+), 0 deletions(-)
include/linux/memcontrol.h +7 -0
@@ -182,6 +182,9 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order, unsigned long *total_scanned);void mem_cgroup_count_vm_event (struct mm_struct *mm,enum vm_event_item idx); + +void update_root_mem_cgroup (int nid); +#ifdef CONFIG_TRANSPARENT_HUGEPAGE void mem_cgroup_split_huge_fixup (struct page *head);#endif
@@ -374,6 +377,10 @@ static inline void mem_cgroup_replace_page_cache(struct page *oldpage, struct page *newpage) { } + +static inline void update_root_mem_cgroup (int nid) +{ +}#endif /* CONFIG_MEMCG */ #if !defined(CONFIG_MEMCG ) || !defined(CONFIG_DEBUG_VM )
mm/memcontrol.c +14 -0
@@ -3427,6 +3427,20 @@ void mem_cgroup_replace_page_cache(struct page *oldpage,__mem_cgroup_commit_charge (memcg, newpage,1 , type, true); } +/* NODE_DATA(nid) is changed */ +void update_root_mem_cgroup (int nid) +{ +struct mem_cgroup_per_node *pn; +struct mem_cgroup_per_zone *mz; +int zone; + + pn = root_mem_cgroup->info.nodeinfo[nid]; +for (zone =0 ; zone <MAX_NR_ZONES ; zone++) { + mz = &pn->zoneinfo[zone]; +lruvec_init (&mz->lruvec, &NODE_DATA (nid)->node_zones[zone]); + } +} +#ifdef CONFIG_DEBUG_VM static struct page_cgroup *lookup_page_cgroup_used (struct page *page) {
mm/memory_hotplug.c +2 -0
@@ -555,6 +555,8 @@ static pg_data_t __ref *hotadd_new_pgdat(int nid, u64 start)/* we can use NODE_DATA(nid) from here */ +update_root_mem_cgroup (nid); +/* init node's zones as empty zones, we don't have any present pages.*/ free_area_init_node (nid, zones_size, start_pfn, zholes_size);
Signature · 1 line
1.7.1
Quoted 52 lines · Wen Congyang
root_mem_cgroup->info.nodeinfois initialized when the system boots. ButNODE_DATA(nid)is null if the node is not onlined, so root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone contains an invalid pointer. If we use numactl to bind a program to the node after onlining the node and its memory, it will cause the kernel panicked:[ 63.413436] BUG: unable to handle kernel NULL pointer dereference at 0000000000000f60 [ 63.414161] IP: [<ffffffff811870b9>] __mod_zone_page_state+0x9/0x60 [ 63.414161] PGD 0 [ 63.414161] Oops: 0000 [#1] SMP [ 63.414161] Modules linked in: acpi_memhotplug binfmt_misc dm_mirror dm_region_hash dm_log dm_mod ppdev sg microcode pcspkr virtio_console virtio_balloon snd_intel8x0 snd_ac9 7_codec ac97_bus snd_seq snd_seq_device snd_pcm snd_timer snd soundcore snd_page_alloc e1000 i2c_piix4 i2c_core floppy parport_pc parport sr_mod cdrom virtio_blk pata_acpi ata_g eneric ata_piix libata scsi_mod [ 63.414161] CPU 2 [ 63.414161] Pid: 1219, comm: numactl Not tainted 3.6.0-rc5+ #180 Bochs Bochs ... [ 63.414161] Process numactl (pid: 1219, threadinfo ffff880039abc000, task ffff8800383c4ce0) [ 63.414161] Stack: [ 63.414161] ffff880039abdaf8 ffffffff8117390f ffff880039abdaf8 000000008167c601 [ 63.414161] ffffffff81174162 ffff88003a480f00 0000000000000001 ffff8800395e0000 [ 63.414161] ffff88003dbd0e80 0000000000000282 ffff880039abdb48 ffffffff81174181 [ 63.414161] Call Trace: [ 63.414161] [<ffffffff8117390f>] __pagevec_lru_add_fn+0xdf/0x140 [ 63.414161] [<ffffffff81174162>] ? pagevec_lru_move_fn+0x92/0x100 [ 63.414161] [<ffffffff81174181>] pagevec_lru_move_fn+0xb1/0x100 [ 63.414161] [<ffffffff81173830>] ? lru_add_page_tail+0x1b0/0x1b0 [ 63.414161] [<ffffffff811dff71>] ? exec_mmap+0x121/0x230 [ 63.414161] [<ffffffff811741ec>] __pagevec_lru_add+0x1c/0x30 [ 63.414161] [<ffffffff81174383>] lru_add_drain_cpu+0xa3/0x130 [ 63.414161] [<ffffffff8117443f>] lru_add_drain+0x2f/0x40 [ 63.414161] [<ffffffff81196da9>] exit_mmap+0x69/0x160 [ 63.414161] [<ffffffff810db115>] ? lock_release_holdtime+0x35/0x1a0 [ 63.414161] [<ffffffff81069c37>] mmput+0x77/0x100 [ 63.414161] [<ffffffff811dffc0>] exec_mmap+0x170/0x230 [ 63.414161] [<ffffffff811e0152>] flush_old_exec+0xd2/0x140 [ 63.414161] [<ffffffff812343ea>] load_elf_binary+0x32a/0xe70 [ 63.414161] [<ffffffff810d700d>] ? trace_hardirqs_off+0xd/0x10 [ 63.414161] [<ffffffff810b231f>] ? local_clock+0x6f/0x80 [ 63.414161] [<ffffffff810db115>] ? lock_release_holdtime+0x35/0x1a0 [ 63.414161] [<ffffffff810dd813>] ? __lock_release+0x133/0x1a0 [ 63.414161] [<ffffffff811e22e7>] ? search_binary_handler+0x1a7/0x4a0 [ 63.414161] [<ffffffff811e22f3>] search_binary_handler+0x1b3/0x4a0 [ 63.414161] [<ffffffff811e2194>] ? search_binary_handler+0x54/0x4a0 [ 63.414161] [<ffffffff812340c0>] ? set_brk+0xe0/0xe0 [ 63.414161] [<ffffffff811e284f>] do_execve_common+0x26f/0x320 [ 63.414161] [<ffffffff811bde33>] ? kmem_cache_alloc+0x113/0x220 [ 63.414161] [<ffffffff811e298a>] do_execve+0x3a/0x40 [ 63.414161] [<ffffffff8102061a>] sys_execve+0x4a/0x80 [ 63.414161] [<ffffffff81686c6c>] stub_execve+0x6c/0xc0 [ 63.414161] Code: ff 03 00 00 48 c1 e7 0b 48 c1 e2 07 48 29 d7 48 03 3c c5 c0 27 d2 81 e8 a6 fe ff ff c9 c3 0f 1f 40 00 55 48 89 e5 0f 1f 44 00 00 <48> 8b 4f 60 89 f6 48 8d 44 31 40 65 44 8a 40 02 45 0f be c0 41
Quoted 5 lines
The reason is that we don't update root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone when onlining the node, and we try to access it.
Signed-off-by Wen Congyang <[email protected]>Reported-by Tang Chen <[email protected]>
Thank you !!!
But, I think, all memcg's lruvec should be updated.
I guess you'll see panic again if you put tasks under memcg and allocates memory on a new node.
Could you dig more ?
Thanks,
-Kame
Quoted 70 lines
--- include/linux/memcontrol.h | 7 +++++++ mm/memcontrol.c | 14 ++++++++++++++ mm/memory_hotplug.c | 2 ++ 3 files changed, 23 insertions(+), 0 deletions(-)include/linux/memcontrol.h +7
-0@@ -182,6 +182,9 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order, unsigned long *total_scanned);void mem_cgroup_count_vm_event (struct mm_struct *mm,enum vm_event_item idx); + +void update_root_mem_cgroup (int nid); +#ifdef CONFIG_TRANSPARENT_HUGEPAGE void mem_cgroup_split_huge_fixup (struct page *head);#endif @@ -374,6 +377,10 @@ static inline void mem_cgroup_replace_page_cache(struct page *oldpage, struct page *newpage) { } + +static inline void update_root_mem_cgroup (int nid) +{ +}#endif /* CONFIG_MEMCG */ #if !defined(CONFIG_MEMCG ) || !defined(CONFIG_DEBUG_VM )mm/memcontrol.c +14
-0@@ -3427,6 +3427,20 @@ void mem_cgroup_replace_page_cache(struct page *oldpage,__mem_cgroup_commit_charge (memcg, newpage,1 , type, true); } +/* NODE_DATA(nid) is changed */ +void update_root_mem_cgroup (int nid) +{ +struct mem_cgroup_per_node *pn; +struct mem_cgroup_per_zone *mz; +int zone; + + pn = root_mem_cgroup->info.nodeinfo[nid]; +for (zone =0 ; zone <MAX_NR_ZONES ; zone++) { + mz = &pn->zoneinfo[zone]; +lruvec_init (&mz->lruvec, &NODE_DATA (nid)->node_zones[zone]); + } +} +#ifdef CONFIG_DEBUG_VM static struct page_cgroup *lookup_page_cgroup_used (struct page *page) {mm/memory_hotplug.c +2
-0@@ -555,6 +555,8 @@ static pg_data_t __ref *hotadd_new_pgdat(int nid, u64 start)/* we can use NODE_DATA(nid) from here */ +update_root_mem_cgroup (nid); +/* init node's zones as empty zones, we don't have any present pages.*/ free_area_init_node (nid, zones_size, start_pfn, zholes_size);
At 09/13/2012 06:06 PM, Kamezawa Hiroyuki Wrote:
Quoted 73 lines
Quoted 65 lines · Wen Congyang
root_mem_cgroup->info.nodeinfois initialized when the system boots. ButNODE_DATA(nid)is null if the node is not onlined, so root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone contains an invalid pointer. If we use numactl to bind a program to the node after onlining the node and its memory, it will cause the kernel panicked:[ 63.413436] BUG: unable to handle kernel NULL pointer dereference at 0000000000000f60 [ 63.414161] IP: [<ffffffff811870b9>] __mod_zone_page_state+0x9/0x60 [ 63.414161] PGD 0 [ 63.414161] Oops: 0000 [#1] SMP [ 63.414161] Modules linked in: acpi_memhotplug binfmt_misc
dm_mirrordm_region_hashdm_logdm_modppdev sg microcode pcspkrvirtio_consolevirtio_balloonsnd_intel8x0snd_ac97_codecac97_bussnd_seqsnd_seq_devicesnd_pcmsnd_timersnd soundcoresnd_page_alloce1000i2c_piix4i2c_corefloppyparport_pcparportsr_modcdromvirtio_blkpata_acpiata_g
enericata_piixlibatascsi_mod[ 63.414161] CPU 2 [ 63.414161] Pid: 1219, comm: numactl Not tainted 3.6.0-rc5+ #180 Bochs Bochs ... [ 63.414161] Process numactl (pid: 1219, threadinfo ffff880039abc000, task ffff8800383c4ce0) [ 63.414161] Stack: [ 63.414161] ffff880039abdaf8 ffffffff8117390f ffff880039abdaf8 000000008167c601 [ 63.414161] ffffffff81174162 ffff88003a480f00 0000000000000001 ffff8800395e0000 [ 63.414161] ffff88003dbd0e80 0000000000000282 ffff880039abdb48 ffffffff81174181 [ 63.414161] Call Trace: [ 63.414161] [<ffffffff8117390f>] __pagevec_lru_add_fn+0xdf/0x140 [ 63.414161] [<ffffffff81174162>] ? pagevec_lru_move_fn+0x92/0x100 [ 63.414161] [<ffffffff81174181>] pagevec_lru_move_fn+0xb1/0x100 [ 63.414161] [<ffffffff81173830>] ? lru_add_page_tail+0x1b0/0x1b0 [ 63.414161] [<ffffffff811dff71>] ? exec_mmap+0x121/0x230 [ 63.414161] [<ffffffff811741ec>] __pagevec_lru_add+0x1c/0x30 [ 63.414161] [<ffffffff81174383>] lru_add_drain_cpu+0xa3/0x130 [ 63.414161] [<ffffffff8117443f>] lru_add_drain+0x2f/0x40 [ 63.414161] [<ffffffff81196da9>] exit_mmap+0x69/0x160 [ 63.414161] [<ffffffff810db115>] ? lock_release_holdtime+0x35/0x1a0 [ 63.414161] [<ffffffff81069c37>] mmput+0x77/0x100 [ 63.414161] [<ffffffff811dffc0>] exec_mmap+0x170/0x230 [ 63.414161] [<ffffffff811e0152>] flush_old_exec+0xd2/0x140 [ 63.414161] [<ffffffff812343ea>] load_elf_binary+0x32a/0xe70 [ 63.414161] [<ffffffff810d700d>] ? trace_hardirqs_off+0xd/0x10 [ 63.414161] [<ffffffff810b231f>] ? local_clock+0x6f/0x80 [ 63.414161] [<ffffffff810db115>] ? lock_release_holdtime+0x35/0x1a0 [ 63.414161] [<ffffffff810dd813>] ? __lock_release+0x133/0x1a0 [ 63.414161] [<ffffffff811e22e7>] ? search_binary_handler+0x1a7/0x4a0 [ 63.414161] [<ffffffff811e22f3>] search_binary_handler+0x1b3/0x4a0 [ 63.414161] [<ffffffff811e2194>] ? search_binary_handler+0x54/0x4a0 [ 63.414161] [<ffffffff812340c0>] ? set_brk+0xe0/0xe0 [ 63.414161] [<ffffffff811e284f>] do_execve_common+0x26f/0x320 [ 63.414161] [<ffffffff811bde33>] ? kmem_cache_alloc+0x113/0x220 [ 63.414161] [<ffffffff811e298a>] do_execve+0x3a/0x40 [ 63.414161] [<ffffffff8102061a>] sys_execve+0x4a/0x80 [ 63.414161] [<ffffffff81686c6c>] stub_execve+0x6c/0xc0 [ 63.414161] Code: ff 03 00 00 48 c1 e7 0b 48 c1 e2 07 48 29 d7 4803 3c c5 c0 27 d2 81 e8 a6 fe ff ff c9 c3 0f 1f 40 00 55 48 89 e5 0f 1f 44 00 00 <48> 8b 4f 60 89 f6 48 8d 44 31 40 65 44 8a 40 02 45 0f be c0 41
Quoted 6 lines
The reason is that we don't update root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone when onlining the node, and we try to access it.
Signed-off-by Wen Congyang <[email protected]>Reported-by Tang Chen <[email protected]>
Quoted 7 lines
Thank you !!!
But, I think, all memcg's lruvec should be updated.
I guess you'll see panic again if you put tasks under memcg and allocates memory on a new node.Could you dig more ?
OK, I will do it.
Thanks
Wen Congyang
Quoted 73 lines
Thanks,
-KameQuoted 69 lines
--- include/linux/memcontrol.h | 7 +++++++ mm/memcontrol.c | 14 ++++++++++++++ mm/memory_hotplug.c | 2 ++ 3 files changed, 23 insertions(+), 0 deletions(-)include/linux/memcontrol.h +3
-0@@ -182,6 +182,9 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order, unsigned long *total_scanned);void mem_cgroup_count_vm_event (struct mm_struct *mm,enum vm_event_item idx); + +void update_root_mem_cgroup (int nid); +#ifdef CONFIG_TRANSPARENT_HUGEPAGE void mem_cgroup_split_huge_fixup(struct page *head); #endifinclude/linux/memcontrol.h +4
-0@@ -374,6 +377,10 @@ static inline void mem_cgroup_replace_page_cache(struct page *oldpage, struct page *newpage) { } + +static inline void update_root_mem_cgroup (int nid) +{ +}#endif /* CONFIG_MEMCG */ #if !defined(CONFIG_MEMCG) || !defined(CONFIG_DEBUG_VM)mm/memcontrol.c +14
-0@@ -3427,6 +3427,20 @@ void mem_cgroup_replace_page_cache(struct page *oldpage,__mem_cgroup_commit_charge (memcg, newpage,1 , type, true); } +/* NODE_DATA(nid) is changed */ +void update_root_mem_cgroup (int nid) +{ +struct mem_cgroup_per_node *pn; +struct mem_cgroup_per_zone *mz; +int zone; + + pn = root_mem_cgroup->info.nodeinfo[nid]; +for (zone =0 ; zone <MAX_NR_ZONES ; zone++) { + mz = &pn->zoneinfo[zone]; +lruvec_init (&mz->lruvec, &NODE_DATA (nid)->node_zones[zone]); + } +} +#ifdef CONFIG_DEBUG_VM static struct page_cgroup *lookup_page_cgroup_used (struct page *page){mm/memory_hotplug.c +0
-0@@ -555,6 +555,8 @@ static pg_data_t __ref *hotadd_new_pgdat(int nid, u64 start)/* we can use NODE_DATA(nid) from here */
Quoted 10 lines
Quoted 7 lines
mm/memory_hotplug.c +2
-0+update_root_mem_cgroup (nid); +/* init node's zones as empty zones, we don't have any present pages.*/
free_area_init_node(nid, zones_size, start_pfn, zholes_size);
--
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>
Johannes Weiner
maintainerHi,
Quoted 6 lines · Wen Congyang
root_mem_cgroup->info.nodeinfois initialized when the system boots. ButNODE_DATA(nid)is null if the node is not onlined, so root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone contains an invalid pointer. If we use numactl to bind a program to the node after onlining the node and its memory, it will cause the kernel panicked:
Is there any chance we could get rid of the zone backpointer in lruvec again instead? Adding new nodes is a rare event and so updating every single memcg in the system might be just borderline crazy. But can't we just go back to passing the zone along with the lruvec down vmscan.c paths? I agree it's ugly to pass both, given their relationship. But I don't think the backpointer is any cleaner but in addition less robust.
That being said, the crashing code in particular makes me wonder:
static __always_inline void add_page_to_lru_list(struct page *page,
struct lruvec *lruvec, enum lru_list lru)
{
int nr_pages = hpage_nr_pages(page);
mem_cgroup_update_lru_size(lruvec, lru, nr_pages);
list_add(&page->lru, &lruvec->lists[lru]);
__mod_zone_page_state(lruvec_zone(lruvec), NR_LRU_BASE + lru, nr_pages);
}Why did we ever pass zone in here and then felt the need to replace it with lruvec->zone in fa9add6 "mm/memcg: apply add/del_page to lruvec"? A page does not roam between zones, its zone is a static property that can be retrieved with page_zone().
Johannes
Quoted 121 lines
[ 63.413436] BUG: unable to handle kernel NULL pointer dereference at 0000000000000f60 [ 63.414161] IP: [<ffffffff811870b9>] __mod_zone_page_state+0x9/0x60 [ 63.414161] PGD 0 [ 63.414161] Oops: 0000 [#1] SMP [ 63.414161] Modules linked in: acpi_memhotplug binfmt_misc dm_mirror dm_region_hash dm_log dm_mod ppdev sg microcode pcspkr virtio_console virtio_balloon snd_intel8x0 snd_ac9 7_codec ac97_bus snd_seq snd_seq_device snd_pcm snd_timer snd soundcore snd_page_alloc e1000 i2c_piix4 i2c_core floppy parport_pc parport sr_mod cdrom virtio_blk pata_acpi ata_g eneric ata_piix libata scsi_mod [ 63.414161] CPU 2 [ 63.414161] Pid: 1219, comm: numactl Not tainted 3.6.0-rc5+ #180 Bochs Bochs ... [ 63.414161] Process numactl (pid: 1219, threadinfo ffff880039abc000, task ffff8800383c4ce0) [ 63.414161] Stack: [ 63.414161] ffff880039abdaf8 ffffffff8117390f ffff880039abdaf8 000000008167c601 [ 63.414161] ffffffff81174162 ffff88003a480f00 0000000000000001 ffff8800395e0000 [ 63.414161] ffff88003dbd0e80 0000000000000282 ffff880039abdb48 ffffffff81174181 [ 63.414161] Call Trace: [ 63.414161] [<ffffffff8117390f>] __pagevec_lru_add_fn+0xdf/0x140 [ 63.414161] [<ffffffff81174162>] ? pagevec_lru_move_fn+0x92/0x100 [ 63.414161] [<ffffffff81174181>] pagevec_lru_move_fn+0xb1/0x100 [ 63.414161] [<ffffffff81173830>] ? lru_add_page_tail+0x1b0/0x1b0 [ 63.414161] [<ffffffff811dff71>] ? exec_mmap+0x121/0x230 [ 63.414161] [<ffffffff811741ec>] __pagevec_lru_add+0x1c/0x30 [ 63.414161] [<ffffffff81174383>] lru_add_drain_cpu+0xa3/0x130 [ 63.414161] [<ffffffff8117443f>] lru_add_drain+0x2f/0x40 [ 63.414161] [<ffffffff81196da9>] exit_mmap+0x69/0x160 [ 63.414161] [<ffffffff810db115>] ? lock_release_holdtime+0x35/0x1a0 [ 63.414161] [<ffffffff81069c37>] mmput+0x77/0x100 [ 63.414161] [<ffffffff811dffc0>] exec_mmap+0x170/0x230 [ 63.414161] [<ffffffff811e0152>] flush_old_exec+0xd2/0x140 [ 63.414161] [<ffffffff812343ea>] load_elf_binary+0x32a/0xe70 [ 63.414161] [<ffffffff810d700d>] ? trace_hardirqs_off+0xd/0x10 [ 63.414161] [<ffffffff810b231f>] ? local_clock+0x6f/0x80 [ 63.414161] [<ffffffff810db115>] ? lock_release_holdtime+0x35/0x1a0 [ 63.414161] [<ffffffff810dd813>] ? __lock_release+0x133/0x1a0 [ 63.414161] [<ffffffff811e22e7>] ? search_binary_handler+0x1a7/0x4a0 [ 63.414161] [<ffffffff811e22f3>] search_binary_handler+0x1b3/0x4a0 [ 63.414161] [<ffffffff811e2194>] ? search_binary_handler+0x54/0x4a0 [ 63.414161] [<ffffffff812340c0>] ? set_brk+0xe0/0xe0 [ 63.414161] [<ffffffff811e284f>] do_execve_common+0x26f/0x320 [ 63.414161] [<ffffffff811bde33>] ? kmem_cache_alloc+0x113/0x220 [ 63.414161] [<ffffffff811e298a>] do_execve+0x3a/0x40 [ 63.414161] [<ffffffff8102061a>] sys_execve+0x4a/0x80 [ 63.414161] [<ffffffff81686c6c>] stub_execve+0x6c/0xc0 [ 63.414161] Code: ff 03 00 00 48 c1 e7 0b 48 c1 e2 07 48 29 d7 48 03 3c c5 c0 27 d2 81 e8 a6 fe ff ff c9 c3 0f 1f 40 00 55 48 89 e5 0f 1f 44 00 00 <48> 8b 4f 60 89 f6 48 8d 44 31 40 65 44 8a 40 02 45 0f be c0 41The reason is that we don't update root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone when onlining the node, and we try to access it.
Signed-off-by Wen Congyang <[email protected]>Reported-by Tang Chen <[email protected]>--- include/linux/memcontrol.h | 7 +++++++ mm/memcontrol.c | 14 ++++++++++++++ mm/memory_hotplug.c | 2 ++ 3 files changed, 23 insertions(+), 0 deletions(-)include/linux/memcontrol.h +7
-0@@ -182,6 +182,9 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order, unsigned long *total_scanned);void mem_cgroup_count_vm_event (struct mm_struct *mm,enum vm_event_item idx); + +void update_root_mem_cgroup (int nid); +#ifdef CONFIG_TRANSPARENT_HUGEPAGE void mem_cgroup_split_huge_fixup (struct page *head);#endif @@ -374,6 +377,10 @@ static inline void mem_cgroup_replace_page_cache(struct page *oldpage, struct page *newpage) { } + +static inline void update_root_mem_cgroup (int nid) +{ +}#endif /* CONFIG_MEMCG */ #if !defined(CONFIG_MEMCG ) || !defined(CONFIG_DEBUG_VM )mm/memcontrol.c +14
-0@@ -3427,6 +3427,20 @@ void mem_cgroup_replace_page_cache(struct page *oldpage,__mem_cgroup_commit_charge (memcg, newpage,1 , type, true); } +/* NODE_DATA(nid) is changed */ +void update_root_mem_cgroup (int nid) +{ +struct mem_cgroup_per_node *pn; +struct mem_cgroup_per_zone *mz; +int zone; + + pn = root_mem_cgroup->info.nodeinfo[nid]; +for (zone =0 ; zone <MAX_NR_ZONES ; zone++) { + mz = &pn->zoneinfo[zone]; +lruvec_init (&mz->lruvec, &NODE_DATA (nid)->node_zones[zone]); + } +} +#ifdef CONFIG_DEBUG_VM static struct page_cgroup *lookup_page_cgroup_used (struct page *page) {mm/memory_hotplug.c +2
-0@@ -555,6 +555,8 @@ static pg_data_t __ref *hotadd_new_pgdat(int nid, u64 start)/* we can use NODE_DATA(nid) from here */ +update_root_mem_cgroup (nid); +/* init node's zones as empty zones, we don't have any present pages.*/ free_area_init_node (nid, zones_size, start_pfn, zholes_size);Signature · 1 line
1.7.1
Quoted 10 lines · Johannes Weiner
Quoted 6 lines · Wen Congyang
root_mem_cgroup->info.nodeinfois initialized when the system boots. ButNODE_DATA(nid)is null if the node is not onlined, so root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone contains an invalid pointer. If we use numactl to bind a program to the node after onlining the node and its memory, it will cause the kernel panicked:Is there any chance we could get rid of the zone backpointer in lruvec again instead?
It could be done, but it would make me sad :(
Quoted 2 lines
Adding new nodes is a rare event and so updating every single memcg in the system might be just borderline crazy.
Not horribly crazy, but rather ugly, yes.
Quoted 5 lines
But can't
we just go back to passing the zone along with the lruvec downvmscan.cpaths? I agree it's ugly to pass both, given their relationship. But I don't think the backpointer is any cleaner but in addition less robust.
It's like how we use vma->mm: we could change everywhere to pass mm with vma, but it looks cleaner and cuts down on long arglists to have mm in vma. From past experience, one of the things I worried about was adding extra args to the reclaim stack.
Quoted 12 lines
That being said, the crashing code in particular makes me wonder:
static
__always_inlinevoid add_page_to_lru_list(struct page *page,
struct lruvec *lruvec, enumlru_listlru){ int nr_pages = hpage_nr_pages(page); mem_cgroup_update_lru_size(lruvec, lru, nr_pages); list_add(&page->lru, &lruvec->lists[lru]); __mod_zone_page_state(lruvec_zone(lruvec), NR_LRU_BASE + lru, nr_pages); }
Quoted 4 lines
Why did we ever pass zone in here and then felt the need to replace it with
lruvec->zonein fa9add6 "mm/memcg: apply add/del_page to lruvec"? A page does not roam between zones, its zone is a static property that can be retrieved withpage_zone().
Just as in vmscan.c, we have the lruvec to hand, and that's what we mainly want to operate upon, but there is also some need for zone.
(Both Konstantin and I were looking towards the day when we move the lru_lock into the lruvec, removing more dependence on "zone". Pretty much the only reason that hasn't happened yet, is that we have not found time to make a performance case convincingly - but that's another topic.)
Yes, page_zone(page) is a static property of the page, but it's not necessarily cheap to evaluate: depends on how complex the memory model and the spare page flags space, doesn't it? We both preferred to derive zone from lruvec where convenient.
How do you feel about this patch, and does it work for you guys?
You'd be right if you guessed that I started out without the mem_cgroup_zone_lruvec part of it, but oops in get_scan_count told me that's needed too.
Description to be filled in later: would it be needed for -stable, or is onlining already broken in other ways that you're now fixing up?
---
include/linux/mmzone.h | 2 - mm/memcontrol.c | 40 ++++++++++++++++++++++++++++++++------- mm/mmzone.c | 6 ----- mm/page_alloc.c | 2 - 4 files changed, 36 insertions(+), 14 deletions(-)
include/linux/mmzone.h +1 -1
@@ -744,7 +744,7 @@ extern int init_currently_empty_zone(strunsigned long size, enum memmap_context context); -extern void lruvec_init (struct lruvec *lruvec,struct zone *zone); +extern void lruvec_init (struct lruvec *lruvec);static inline struct zone *lruvec_zone (struct lruvec *lruvec) {
mm/memcontrol.c +33 -7
@@ -1061,12 +1061,25 @@ struct lruvec *mem_cgroup_zone_lruvec(st struct mem_cgroup *memcg) {struct mem_cgroup_per_zone *mz; +struct lruvec *lruvec; -if (mem_cgroup_disabled ()) -return &zone->lruvec; +if (mem_cgroup_disabled ()) { + lruvec = &zone->lruvec; + goto out; + } mz =mem_cgroup_zoneinfo (memcg,zone_to_nid (zone),zone_idx (zone)); -return &mz->lruvec; + lruvec = &mz->lruvec; +out: +/* +* Since a node can be onlined after the mem_cgroup was created, +* we have to be prepared to initialize lruvec->zone here. +*/ +if (unlikely (lruvec->zone != zone)) { +VM_BUG_ON (lruvec->zone); + lruvec->zone = zone; + } +return lruvec; }/*
@@ -1093,9 +1106,12 @@ struct lruvec *mem_cgroup_page_lruvec(ststruct mem_cgroup_per_zone *mz;struct mem_cgroup *memcg;struct page_cgroup *pc; +struct lruvec *lruvec; -if (mem_cgroup_disabled ()) -return &zone->lruvec; +if (mem_cgroup_disabled ()) { + lruvec = &zone->lruvec; + goto out; + } pc =lookup_page_cgroup (page); memcg = pc->mem_cgroup;
@@ -1113,7 +1129,17 @@ struct lruvec *mem_cgroup_page_lruvec(st pc->mem_cgroup = memcg = root_mem_cgroup; mz =page_cgroup_zoneinfo (memcg, page); -return &mz->lruvec; + lruvec = &mz->lruvec; +out: +/* +* Since a node can be onlined after the mem_cgroup was created, +* we have to be prepared to initialize lruvec->zone here. +*/ +if (unlikely (lruvec->zone != zone)) { +VM_BUG_ON (lruvec->zone); + lruvec->zone = zone; + } +return lruvec; }/**
@@ -4742,7 +4768,7 @@ static int alloc_mem_cgroup_per_zone_inffor (zone =0 ; zone <MAX_NR_ZONES ; zone++) { mz = &pn->zoneinfo[zone]; -lruvec_init (&mz->lruvec, &NODE_DATA (node)->node_zones[zone]); +lruvec_init (&mz->lruvec); mz->usage_in_excess =0 ; mz->on_tree = false; mz->memcg = memcg;
mm/mmzone.c +1 -5
@@ -87,7 +87,7 @@ int memmap_valid_within(unsigned long pf }#endif /* CONFIG_ARCH_HAS_HOLES_MEMORYMODEL */ -void lruvec_init (struct lruvec *lruvec,struct zone *zone) +void lruvec_init (struct lruvec *lruvec) {enum lru_list lru;
@@ -95,8 +95,4 @@ void lruvec_init(struct lruvec *lruvec,for_each_lru (lru)INIT_LIST_HEAD (&lruvec->lists[lru]); - -#ifdef CONFIG_MEMCG - lruvec->zone = zone; -#endif }
mm/page_alloc.c +1 -1
@@ -4456,7 +4456,7 @@ static void __paginginit free_area_init_ zone->zone_pgdat = pgdat;zone_pcp_init (zone); -lruvec_init (&zone->lruvec, zone); +lruvec_init (&zone->lruvec);if (!size)continue ;
--
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>
At 09/14/2012 09:36 AM, Hugh Dickins Wrote:
Quoted 60 lines · Johannes Weiner
Quoted 10 lines · Wen Congyang
Quoted 6 lines
root_mem_cgroup->info.nodeinfois initialized when the system boots. ButNODE_DATA(nid)is null if the node is not onlined, so root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone contains an invalid pointer. If we use numactl to bind a program to the node after onlining the node and its memory, it will cause the kernel panicked:Is there any chance we could get rid of the zone backpointer in lruvec again instead?
It could be done, but it would make me sad :(
Quoted 2 lines
Adding new nodes is a rare event and so updating every single memcg in the system might be just borderline crazy.
Not horribly crazy, but rather ugly, yes.
Quoted 5 lines
But can't
we just go back to passing the zone along with the lruvec downvmscan.cpaths? I agree it's ugly to pass both, given their relationship. But I don't think the backpointer is any cleaner but in addition less robust.It's like how we use
vma->mm: we could change everywhere to pass mm with vma, but it looks cleaner and cuts down on long arglists to have mm in vma.Quoted 1 line
From past experience, one of the things I worried about was adding extra
args to the reclaim stack.
Quoted 12 lines
That being said, the crashing code in particular makes me wonder:
static
__always_inlinevoid add_page_to_lru_list(struct page *page,
struct lruvec *lruvec, enumlru_listlru){ int nr_pages = hpage_nr_pages(page); mem_cgroup_update_lru_size(lruvec, lru, nr_pages); list_add(&page->lru, &lruvec->lists[lru]); __mod_zone_page_state(lruvec_zone(lruvec), NR_LRU_BASE + lru, nr_pages); }Quoted 4 lines
Why did we ever pass zone in here and then felt the need to replace it with
lruvec->zonein fa9add6 "mm/memcg: apply add/del_page to lruvec"? A page does not roam between zones, its zone is a static property that can be retrieved withpage_zone().Just as in
vmscan.c, we have the lruvec to hand, and that's what we mainly want to operate upon, but there is also some need for zone.(Both Konstantin and I were looking towards the day when we move the
lru_lockinto the lruvec, removing more dependence on "zone". Pretty much the only reason that hasn't happened yet, is that we have not found time to make a performance case convincingly - but that's another topic.)Yes,
page_zone(page)is a static property of the page, but it's not necessarily cheap to evaluate: depends on how complex the memory model and the spare page flags space, doesn't it? We both preferred to derive zone from lruvec where convenient.
Quoted 8 lines
How do you feel about this patch, and does it work for you guys?
You'd be right if you guessed that I started out without the
mem_cgroup_zone_lruvecpart of it, but oops inget_scan_counttold me that's needed too.Description to be filled in later: would it be needed for -stable, or is onlining already broken in other ways that you're now fixing up?
We will test your patch, please wait some time.
Quoted 71 lines
Reported-by Tang Chen <[email protected]>Signed-off-by Hugh Dickins <[email protected]>---
include/linux/mmzone.h | 2 - mm/memcontrol.c | 40 ++++++++++++++++++++++++++++++++------- mm/mmzone.c | 6 ----- mm/page_alloc.c | 2 - 4 files changed, 36 insertions(+), 14 deletions(-)include/linux/mmzone.h +1
-1@@ -744,7 +744,7 @@ extern int init_currently_empty_zone(strunsigned long size, enum memmap_context context); -extern void lruvec_init (struct lruvec *lruvec,struct zone *zone); +extern void lruvec_init (struct lruvec *lruvec);static inline struct zone *lruvec_zone (struct lruvec *lruvec) {mm/memcontrol.c +21
-5@@ -1061,12 +1061,25 @@ struct lruvec *mem_cgroup_zone_lruvec(st struct mem_cgroup *memcg) {struct mem_cgroup_per_zone *mz; +struct lruvec *lruvec; -if (mem_cgroup_disabled ()) -return &zone->lruvec; +if (mem_cgroup_disabled ()) { + lruvec = &zone->lruvec; + goto out; + } mz =mem_cgroup_zoneinfo (memcg,zone_to_nid (zone),zone_idx (zone)); -return &mz->lruvec; + lruvec = &mz->lruvec; +out: +/* +* Since a node can be onlined after the mem_cgroup was created, +* we have to be prepared to initialize lruvec->zone here. +*/ +if (unlikely (lruvec->zone != zone)) { +VM_BUG_ON (lruvec->zone); + lruvec->zone = zone; + } +return lruvec; }/* @@ -1093,9 +1106,12 @@ struct lruvec *mem_cgroup_page_lruvec(ststruct mem_cgroup_per_zone *mz;struct mem_cgroup *memcg;struct page_cgroup *pc; +struct lruvec *lruvec; -if (mem_cgroup_disabled ()) -return &zone->lruvec; +if (mem_cgroup_disabled ()) { + lruvec = &zone->lruvec; + goto out; + } pc =lookup_page_cgroup (page); memcg = pc->mem_cgroup;@@ -1113,7 +1129,17 @@ struct lruvec *mem_cgroup_page_lruvec(st pc->mem_cgroup = memcg = root_mem_cgroup;
mm/memcontrol.c struct lruvec *mem_cgroup_page_lruvec(st · line 1139
Quoted 10 lines
mm/memcontrol.c +8
-1mz =page_cgroup_zoneinfo (memcg, page); -return &mz->lruvec; + lruvec = &mz->lruvec; +out: +/* +* Since a node can be onlined after the mem_cgroup was created, +* we have to be prepared to initialize lruvec->zone here. +*/ +if (unlikely (lruvec->zone != zone)) { +VM_BUG_ON (lruvec->zone);
According your comment, lruver->zone != zone if a node is onlined. So it is not a bug, and VM_BUG_ON() should not be used here.
Thanks
Wen Congyang
Quoted 39 lines
mm/memcontrol.c +4
-1+ lruvec->zone = zone; + } +return lruvec; }/** @@ -4742,7 +4768,7 @@ static int alloc_mem_cgroup_per_zone_inffor (zone =0 ; zone <MAX_NR_ZONES ; zone++) { mz = &pn->zoneinfo[zone]; -lruvec_init (&mz->lruvec, &NODE_DATA (node)->node_zones[zone]); +lruvec_init (&mz->lruvec); mz->usage_in_excess =0 ; mz->on_tree = false; mz->memcg = memcg;mm/mmzone.c +1
-5@@ -87,7 +87,7 @@ int memmap_valid_within(unsigned long pf }#endif /* CONFIG_ARCH_HAS_HOLES_MEMORYMODEL */ -void lruvec_init (struct lruvec *lruvec,struct zone *zone) +void lruvec_init (struct lruvec *lruvec) {enum lru_list lru;@@ -95,8 +95,4 @@ void lruvec_init(struct lruvec *lruvec,for_each_lru (lru)INIT_LIST_HEAD (&lruvec->lists[lru]); - -#ifdef CONFIG_MEMCG - lruvec->zone = zone; -#endif }
mm/page_alloc.c static void __paginginit free_area_init_ · line 4462
Quoted 8 lines
mm/page_alloc.c +1
-1@@ -4456,7 +4456,7 @@ static void __paginginit free_area_init_ zone->zone_pgdat = pgdat;zone_pcp_init (zone); -lruvec_init (&zone->lruvec, zone); +lruvec_init (&zone->lruvec);if (!size)continue ;
--
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>
Quoted 9 lines · Hugh Dickins
Quoted 8 lines · Johannes Weiner
Quoted 6 lines · Wen Congyang
root_mem_cgroup->info.nodeinfois initialized when the system boots. ButNODE_DATA(nid)is null if the node is not onlined, so root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone contains an invalid pointer. If we use numactl to bind a program to the node after onlining the node and its memory, it will cause the kernel panicked:
Quoted 4 lines
Quoted 2 lines
Is there any chance we could get rid of the zone backpointer in lruvec again instead?
It could be done, but it would make me sad :(
We would not want that!
Quoted 10 lines
Quoted 5 lines
But can't
we just go back to passing the zone along with the lruvec downvmscan.cpaths? I agree it's ugly to pass both, given their relationship. But I don't think the backpointer is any cleaner but in addition less robust.It's like how we use
vma->mm: we could change everywhere to pass mm with vma, but it looks cleaner and cuts down on long arglists to have mm in vma.Quoted 1 line
From past experience, one of the things I worried about was adding extra
args to the reclaim stack.
Ok, you certainly have a point.
Quoted 35 lines
Quoted 11 lines
That being said, the crashing code in particular makes me wonder:
static
__always_inlinevoid add_page_to_lru_list(struct page *page,
struct lruvec *lruvec, enumlru_listlru){ int nr_pages = hpage_nr_pages(page); mem_cgroup_update_lru_size(lruvec, lru, nr_pages); list_add(&page->lru, &lruvec->lists[lru]); __mod_zone_page_state(lruvec_zone(lruvec), NR_LRU_BASE + lru, nr_pages); }Quoted 4 lines
Why did we ever pass zone in here and then felt the need to replace it with
lruvec->zonein fa9add6 "mm/memcg: apply add/del_page to lruvec"? A page does not roam between zones, its zone is a static property that can be retrieved withpage_zone().Just as in
vmscan.c, we have the lruvec to hand, and that's what we mainly want to operate upon, but there is also some need for zone.(Both Konstantin and I were looking towards the day when we move the
lru_lockinto the lruvec, removing more dependence on "zone". Pretty much the only reason that hasn't happened yet, is that we have not found time to make a performance case convincingly - but that's another topic.)Yes,
page_zone(page)is a static property of the page, but it's not necessarily cheap to evaluate: depends on how complex the memory model and the spare page flags space, doesn't it? We both preferred to derive zone from lruvec where convenient.How do you feel about this patch, and does it work for you guys?
You'd be right if you guessed that I started out without the
mem_cgroup_zone_lruvecpart of it, but oops inget_scan_counttold me that's needed too.
Quoted 5 lines
Description to be filled in later: would it be needed for -stable, or is onlining already broken in other ways that you're now fixing up?
Reported-by Tang Chen <[email protected]>Signed-off-by Hugh Dickins <[email protected]>
This looks good to me, thanks.
Quoted 9 lines · Hugh Dickins
Quoted 8 lines · Johannes Weiner
Quoted 6 lines · Wen Congyang
root_mem_cgroup->info.nodeinfois initialized when the system boots. ButNODE_DATA(nid)is null if the node is not onlined, so root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone contains an invalid pointer. If we use numactl to bind a program to the node after onlining the node and its memory, it will cause the kernel panicked:
Quoted 4 lines
Quoted 2 lines
Is there any chance we could get rid of the zone backpointer in lruvec again instead?
It could be done, but it would make me sad :(
Me too
Quoted 96 lines
Quoted 3 lines
Adding new nodes is a rare event and so updating every single memcg in the system might be just borderline crazy.
Not horribly crazy, but rather ugly, yes.
Quoted 5 lines
But can't
we just go back to passing the zone along with the lruvec downvmscan.cpaths? I agree it's ugly to pass both, given their relationship. But I don't think the backpointer is any cleaner but in addition less robust.It's like how we use
vma->mm: we could change everywhere to pass mm with vma, but it looks cleaner and cuts down on long arglists to have mm in vma.
From past experience, one of the things I worried about was adding extra
args to the reclaim stack.Quoted 12 lines
That being said, the crashing code in particular makes me wonder:
static
__always_inlinevoid add_page_to_lru_list(struct page *page,
struct lruvec *lruvec, enumlru_listlru){ int nr_pages = hpage_nr_pages(page); mem_cgroup_update_lru_size(lruvec, lru, nr_pages); list_add(&page->lru,&lruvec->lists[lru]); __mod_zone_page_state(lruvec_zone(lruvec), NR_LRU_BASE + lru, nr_pages); }Quoted 4 lines
Why did we ever pass zone in here and then felt the need to replace it with
lruvec->zonein fa9add6 "mm/memcg: apply add/del_page to lruvec"? A page does not roam between zones, its zone is a static property that can be retrieved withpage_zone().Just as in
vmscan.c, we have the lruvec to hand, and that's what we mainly want to operate upon, but there is also some need for zone.(Both Konstantin and I were looking towards the day when we move the
lru_lockinto the lruvec, removing more dependence on "zone". Pretty much the only reason that hasn't happened yet, is that we have not found time to make a performance case convincingly - but that's another topic.)Yes,
page_zone(page)is a static property of the page, but it's not necessarily cheap to evaluate: depends on how complex the memory model and the spare page flags space, doesn't it? We both preferred to derive zone from lruvec where convenient.How do you feel about this patch, and does it work for you guys?
You'd be right if you guessed that I started out without the
mem_cgroup_zone_lruvecpart of it, but oops inget_scan_counttold me that's needed too.Description to be filled in later: would it be needed for -stable, or is onlining already broken in other ways that you're now fixing up?
Reported-by Tang Chen<[email protected]>Signed-off-by Hugh Dickins<[email protected]>---
include/linux/mmzone.h | 2 - mm/memcontrol.c | 40 ++++++++++++++++++++++++++++++++------- mm/mmzone.c | 6 ----- mm/page_alloc.c | 2 - 4 files changed, 36 insertions(+), 14 deletions(-)include/linux/mmzone.h +1
-1@@ -744,7 +744,7 @@ extern int init_currently_empty_zone(strunsigned long size, enum memmap_context context); -extern void lruvec_init (struct lruvec *lruvec,struct zone *zone); +extern void lruvec_init (struct lruvec *lruvec);static inline struct zone *lruvec_zone (struct lruvec *lruvec) {mm/memcontrol.c +7
-3@@ -1061,12 +1061,25 @@ struct lruvec *mem_cgroup_zone_lruvec(st struct mem_cgroup *memcg) {struct mem_cgroup_per_zone *mz; +struct lruvec *lruvec; -if (mem_cgroup_disabled ()) -return &zone->lruvec; +if (mem_cgroup_disabled ()) { + lruvec =&zone->lruvec; + goto out; + } mz =mem_cgroup_zoneinfo (memcg,zone_to_nid (zone),zone_idx (zone)); -return &mz->lruvec; + lruvec =&mz->lruvec; +out:
mm/memcontrol.c struct lruvec *mem_cgroup_zone_lruvec(st · line 1083
Quoted 10 lines
mm/memcontrol.c +9
-0+/* +* Since a node can be onlined after the mem_cgroup was created, +* we have to be prepared to initialize lruvec->zone here. +*/ +if (unlikely (lruvec->zone != zone)) { +VM_BUG_ON (lruvec->zone); + lruvec->zone = zone; + } +return lruvec; }
Whoaaa, this makes me dizzy. I prefer hook in register_one_node(). BTW It would be nice to add notifier-chains into memory-hotplug.
Quoted 76 lines
mm/memcontrol.c +17
-4/* @@ -1093,9 +1106,12 @@ struct lruvec *mem_cgroup_page_lruvec(ststruct mem_cgroup_per_zone *mz;struct mem_cgroup *memcg;struct page_cgroup *pc; +struct lruvec *lruvec; -if (mem_cgroup_disabled ()) -return &zone->lruvec; +if (mem_cgroup_disabled ()) { + lruvec =&zone->lruvec; + goto out; + } pc =lookup_page_cgroup (page); memcg = pc->mem_cgroup;@@ -1113,7 +1129,17 @@ struct lruvec *mem_cgroup_page_lruvec(st pc->mem_cgroup = memcg = root_mem_cgroup; mz =page_cgroup_zoneinfo (memcg, page); -return &mz->lruvec; + lruvec =&mz->lruvec; +out: +/* +* Since a node can be onlined after the mem_cgroup was created, +* we have to be prepared to initialize lruvec->zone here. +*/ +if (unlikely (lruvec->zone != zone)) { +VM_BUG_ON (lruvec->zone); + lruvec->zone = zone; + } +return lruvec; }/** @@ -4742,7 +4768,7 @@ static int alloc_mem_cgroup_per_zone_inffor (zone =0 ; zone<MAX_NR_ZONES ; zone++) { mz =&pn->zoneinfo[zone]; -lruvec_init (&mz->lruvec,&NODE_DATA (node)->node_zones[zone]); +lruvec_init (&mz->lruvec); mz->usage_in_excess =0 ; mz->on_tree = false; mz->memcg = memcg;mm/mmzone.c +1
-5@@ -87,7 +87,7 @@ int memmap_valid_within(unsigned long pf }#endif /* CONFIG_ARCH_HAS_HOLES_MEMORYMODEL */ -void lruvec_init (struct lruvec *lruvec,struct zone *zone) +void lruvec_init (struct lruvec *lruvec) {enum lru_list lru;@@ -95,8 +95,4 @@ void lruvec_init(struct lruvec *lruvec,for_each_lru (lru)INIT_LIST_HEAD (&lruvec->lists[lru]); - -#ifdef CONFIG_MEMCG - lruvec->zone = zone; -#endif }mm/page_alloc.c +1
-1@@ -4456,7 +4456,7 @@ static void __paginginit free_area_init_ zone->zone_pgdat = pgdat;zone_pcp_init (zone); -lruvec_init (&zone->lruvec, zone); +lruvec_init (&zone->lruvec);if (!size)continue ;
At 09/14/2012 09:36 AM, Hugh Dickins Wrote:
Quoted 105 lines · Johannes Weiner
Quoted 10 lines · Wen Congyang
Quoted 6 lines
root_mem_cgroup->info.nodeinfois initialized when the system boots. ButNODE_DATA(nid)is null if the node is not onlined, so root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone contains an invalid pointer. If we use numactl to bind a program to the node after onlining the node and its memory, it will cause the kernel panicked:Is there any chance we could get rid of the zone backpointer in lruvec again instead?
It could be done, but it would make me sad :(
Quoted 2 lines
Adding new nodes is a rare event and so updating every single memcg in the system might be just borderline crazy.
Not horribly crazy, but rather ugly, yes.
Quoted 5 lines
But can't
we just go back to passing the zone along with the lruvec downvmscan.cpaths? I agree it's ugly to pass both, given their relationship. But I don't think the backpointer is any cleaner but in addition less robust.It's like how we use
vma->mm: we could change everywhere to pass mm with vma, but it looks cleaner and cuts down on long arglists to have mm in vma.Quoted 1 line
From past experience, one of the things I worried about was adding extra
args to the reclaim stack.
Quoted 12 lines
That being said, the crashing code in particular makes me wonder:
static
__always_inlinevoid add_page_to_lru_list(struct page *page,
struct lruvec *lruvec, enumlru_listlru){ int nr_pages = hpage_nr_pages(page); mem_cgroup_update_lru_size(lruvec, lru, nr_pages); list_add(&page->lru, &lruvec->lists[lru]); __mod_zone_page_state(lruvec_zone(lruvec), NR_LRU_BASE + lru, nr_pages); }Quoted 4 lines
Why did we ever pass zone in here and then felt the need to replace it with
lruvec->zonein fa9add6 "mm/memcg: apply add/del_page to lruvec"? A page does not roam between zones, its zone is a static property that can be retrieved withpage_zone().Just as in
vmscan.c, we have the lruvec to hand, and that's what we mainly want to operate upon, but there is also some need for zone.(Both Konstantin and I were looking towards the day when we move the
lru_lockinto the lruvec, removing more dependence on "zone". Pretty much the only reason that hasn't happened yet, is that we have not found time to make a performance case convincingly - but that's another topic.)Yes,
page_zone(page)is a static property of the page, but it's not necessarily cheap to evaluate: depends on how complex the memory model and the spare page flags space, doesn't it? We both preferred to derive zone from lruvec where convenient.How do you feel about this patch, and does it work for you guys?
You'd be right if you guessed that I started out without the
mem_cgroup_zone_lruvecpart of it, but oops inget_scan_counttold me that's needed too.Description to be filled in later: would it be needed for -stable, or is onlining already broken in other ways that you're now fixing up?
Reported-by Tang Chen <[email protected]>Signed-off-by Hugh Dickins <[email protected]>---
include/linux/mmzone.h | 2 - mm/memcontrol.c | 40 ++++++++++++++++++++++++++++++++------- mm/mmzone.c | 6 ----- mm/page_alloc.c | 2 - 4 files changed, 36 insertions(+), 14 deletions(-)include/linux/mmzone.h +1
-1@@ -744,7 +744,7 @@ extern int init_currently_empty_zone(strunsigned long size, enum memmap_context context); -extern void lruvec_init (struct lruvec *lruvec,struct zone *zone); +extern void lruvec_init (struct lruvec *lruvec);static inline struct zone *lruvec_zone (struct lruvec *lruvec) {mm/memcontrol.c +5
-2@@ -1061,12 +1061,25 @@ struct lruvec *mem_cgroup_zone_lruvec(st struct mem_cgroup *memcg) {struct mem_cgroup_per_zone *mz; +struct lruvec *lruvec; -if (mem_cgroup_disabled ()) -return &zone->lruvec; +if (mem_cgroup_disabled ()) { + lruvec = &zone->lruvec; + goto out; + }
mm/memcontrol.c struct lruvec *mem_cgroup_zone_lruvec(st · line 1079
Quoted 10 lines
mm/memcontrol.c +8
-1mz =mem_cgroup_zoneinfo (memcg,zone_to_nid (zone),zone_idx (zone)); -return &mz->lruvec; + lruvec = &mz->lruvec; +out: +/* +* Since a node can be onlined after the mem_cgroup was created, +* we have to be prepared to initialize lruvec->zone here. +*/ +if (unlikely (lruvec->zone != zone)) { +VM_BUG_ON (lruvec->zone);
If node is offlined and onlined again, lruvec->zone is not NULL, and not equal to zone, this line will cause kernel panicked.
Quoted 24 lines
mm/memcontrol.c +8
-2+ lruvec->zone = zone; + } +return lruvec; }/* @@ -1093,9 +1106,12 @@ struct lruvec *mem_cgroup_page_lruvec(ststruct mem_cgroup_per_zone *mz;struct mem_cgroup *memcg;struct page_cgroup *pc; +struct lruvec *lruvec; -if (mem_cgroup_disabled ()) -return &zone->lruvec; +if (mem_cgroup_disabled ()) { + lruvec = &zone->lruvec; + goto out; + } pc =lookup_page_cgroup (page); memcg = pc->mem_cgroup;@@ -1113,7 +1129,17 @@ struct lruvec *mem_cgroup_page_lruvec(st pc->mem_cgroup = memcg = root_mem_cgroup;
mm/memcontrol.c struct lruvec *mem_cgroup_page_lruvec(st · line 1139
Quoted 10 lines
mm/memcontrol.c +8
-1mz =page_cgroup_zoneinfo (memcg, page); -return &mz->lruvec; + lruvec = &mz->lruvec; +out: +/* +* Since a node can be onlined after the mem_cgroup was created, +* we have to be prepared to initialize lruvec->zone here. +*/ +if (unlikely (lruvec->zone != zone)) { +VM_BUG_ON (lruvec->zone);
I apply your patch, and remove VM_BUG_ON(). I don't find any error in my test now.
Thanks
Wen Congyang
Quoted 39 lines
mm/memcontrol.c +4
-1+ lruvec->zone = zone; + } +return lruvec; }/** @@ -4742,7 +4768,7 @@ static int alloc_mem_cgroup_per_zone_inffor (zone =0 ; zone <MAX_NR_ZONES ; zone++) { mz = &pn->zoneinfo[zone]; -lruvec_init (&mz->lruvec, &NODE_DATA (node)->node_zones[zone]); +lruvec_init (&mz->lruvec); mz->usage_in_excess =0 ; mz->on_tree = false; mz->memcg = memcg;mm/mmzone.c +1
-5@@ -87,7 +87,7 @@ int memmap_valid_within(unsigned long pf }#endif /* CONFIG_ARCH_HAS_HOLES_MEMORYMODEL */ -void lruvec_init (struct lruvec *lruvec,struct zone *zone) +void lruvec_init (struct lruvec *lruvec) {enum lru_list lru;@@ -95,8 +95,4 @@ void lruvec_init(struct lruvec *lruvec,for_each_lru (lru)INIT_LIST_HEAD (&lruvec->lists[lru]); - -#ifdef CONFIG_MEMCG - lruvec->zone = zone; -#endif }
mm/page_alloc.c static void __paginginit free_area_init_ · line 4462
Quoted 8 lines
mm/page_alloc.c +1
-1@@ -4456,7 +4456,7 @@ static void __paginginit free_area_init_ zone->zone_pgdat = pgdat;zone_pcp_init (zone); -lruvec_init (&zone->lruvec, zone); +lruvec_init (&zone->lruvec);if (!size)continue ;
--
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>
At 09/14/2012 09:36 AM, Hugh Dickins Wrote:
Quoted 66 lines · Johannes Weiner
Quoted 10 lines · Wen Congyang
Quoted 6 lines
root_mem_cgroup->info.nodeinfois initialized when the system boots. ButNODE_DATA(nid)is null if the node is not onlined, so root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone contains an invalid pointer. If we use numactl to bind a program to the node after onlining the node and its memory, it will cause the kernel panicked:Is there any chance we could get rid of the zone backpointer in lruvec again instead?
It could be done, but it would make me sad :(
Quoted 2 lines
Adding new nodes is a rare event and so updating every single memcg in the system might be just borderline crazy.
Not horribly crazy, but rather ugly, yes.
Quoted 5 lines
But can't
we just go back to passing the zone along with the lruvec downvmscan.cpaths? I agree it's ugly to pass both, given their relationship. But I don't think the backpointer is any cleaner but in addition less robust.It's like how we use
vma->mm: we could change everywhere to pass mm with vma, but it looks cleaner and cuts down on long arglists to have mm in vma.Quoted 1 line
From past experience, one of the things I worried about was adding extra
args to the reclaim stack.
Quoted 12 lines
That being said, the crashing code in particular makes me wonder:
static
__always_inlinevoid add_page_to_lru_list(struct page *page,
struct lruvec *lruvec, enumlru_listlru){ int nr_pages = hpage_nr_pages(page); mem_cgroup_update_lru_size(lruvec, lru, nr_pages); list_add(&page->lru, &lruvec->lists[lru]); __mod_zone_page_state(lruvec_zone(lruvec), NR_LRU_BASE + lru, nr_pages); }Quoted 4 lines
Why did we ever pass zone in here and then felt the need to replace it with
lruvec->zonein fa9add6 "mm/memcg: apply add/del_page to lruvec"? A page does not roam between zones, its zone is a static property that can be retrieved withpage_zone().Just as in
vmscan.c, we have the lruvec to hand, and that's what we mainly want to operate upon, but there is also some need for zone.(Both Konstantin and I were looking towards the day when we move the
lru_lockinto the lruvec, removing more dependence on "zone". Pretty much the only reason that hasn't happened yet, is that we have not found time to make a performance case convincingly - but that's another topic.)Yes,
page_zone(page)is a static property of the page, but it's not necessarily cheap to evaluate: depends on how complex the memory model and the spare page flags space, doesn't it? We both preferred to derive zone from lruvec where convenient.How do you feel about this patch, and does it work for you guys?
You'd be right if you guessed that I started out without the
mem_cgroup_zone_lruvecpart of it, but oops inget_scan_counttold me that's needed too.
Quoted 5 lines
Description to be filled in later: would it be needed for -stable, or is onlining already broken in other ways that you're now fixing up?
Reported-by Tang Chen <[email protected]>Signed-off-by Hugh Dickins <[email protected]>
Hi, all:
What about the status of this patch?
Thanks
Wen Congyang
Quoted 117 lines
---
include/linux/mmzone.h | 2 - mm/memcontrol.c | 40 ++++++++++++++++++++++++++++++++------- mm/mmzone.c | 6 ----- mm/page_alloc.c | 2 - 4 files changed, 36 insertions(+), 14 deletions(-)include/linux/mmzone.h +1
-1@@ -744,7 +744,7 @@ extern int init_currently_empty_zone(strunsigned long size, enum memmap_context context); -extern void lruvec_init (struct lruvec *lruvec,struct zone *zone); +extern void lruvec_init (struct lruvec *lruvec);static inline struct zone *lruvec_zone (struct lruvec *lruvec) {mm/memcontrol.c +33
-7@@ -1061,12 +1061,25 @@ struct lruvec *mem_cgroup_zone_lruvec(st struct mem_cgroup *memcg) {struct mem_cgroup_per_zone *mz; +struct lruvec *lruvec; -if (mem_cgroup_disabled ()) -return &zone->lruvec; +if (mem_cgroup_disabled ()) { + lruvec = &zone->lruvec; + goto out; + } mz =mem_cgroup_zoneinfo (memcg,zone_to_nid (zone),zone_idx (zone)); -return &mz->lruvec; + lruvec = &mz->lruvec; +out: +/* +* Since a node can be onlined after the mem_cgroup was created, +* we have to be prepared to initialize lruvec->zone here. +*/ +if (unlikely (lruvec->zone != zone)) { +VM_BUG_ON (lruvec->zone); + lruvec->zone = zone; + } +return lruvec; }/* @@ -1093,9 +1106,12 @@ struct lruvec *mem_cgroup_page_lruvec(ststruct mem_cgroup_per_zone *mz;struct mem_cgroup *memcg;struct page_cgroup *pc; +struct lruvec *lruvec; -if (mem_cgroup_disabled ()) -return &zone->lruvec; +if (mem_cgroup_disabled ()) { + lruvec = &zone->lruvec; + goto out; + } pc =lookup_page_cgroup (page); memcg = pc->mem_cgroup;@@ -1113,7 +1129,17 @@ struct lruvec *mem_cgroup_page_lruvec(st pc->mem_cgroup = memcg = root_mem_cgroup; mz =page_cgroup_zoneinfo (memcg, page); -return &mz->lruvec; + lruvec = &mz->lruvec; +out: +/* +* Since a node can be onlined after the mem_cgroup was created, +* we have to be prepared to initialize lruvec->zone here. +*/ +if (unlikely (lruvec->zone != zone)) { +VM_BUG_ON (lruvec->zone); + lruvec->zone = zone; + } +return lruvec; }/** @@ -4742,7 +4768,7 @@ static int alloc_mem_cgroup_per_zone_inffor (zone =0 ; zone <MAX_NR_ZONES ; zone++) { mz = &pn->zoneinfo[zone]; -lruvec_init (&mz->lruvec, &NODE_DATA (node)->node_zones[zone]); +lruvec_init (&mz->lruvec); mz->usage_in_excess =0 ; mz->on_tree = false; mz->memcg = memcg;mm/mmzone.c +1
-5@@ -87,7 +87,7 @@ int memmap_valid_within(unsigned long pf }#endif /* CONFIG_ARCH_HAS_HOLES_MEMORYMODEL */ -void lruvec_init (struct lruvec *lruvec,struct zone *zone) +void lruvec_init (struct lruvec *lruvec) {enum lru_list lru;@@ -95,8 +95,4 @@ void lruvec_init(struct lruvec *lruvec,for_each_lru (lru)INIT_LIST_HEAD (&lruvec->lists[lru]); - -#ifdef CONFIG_MEMCG - lruvec->zone = zone; -#endif }
mm/page_alloc.c static void __paginginit free_area_init_ · line 4462
Quoted 8 lines
mm/page_alloc.c +1
-1@@ -4456,7 +4456,7 @@ static void __paginginit free_area_init_ zone->zone_pgdat = pgdat;zone_pcp_init (zone); -lruvec_init (&zone->lruvec, zone); +lruvec_init (&zone->lruvec);if (!size)continue ;
--
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>
Quoted 11 lines · Wen Congyang
At 09/14/2012 09:36 AM, Hugh Dickins Wrote:
Quoted 6 lines
Description to be filled in later: would it be needed for -stable, or is onlining already broken in other ways that you're now fixing up?
Reported-by Tang Chen <[email protected]>Signed-off-by Hugh Dickins <[email protected]>Hi, all:
What about the status of this patch?
Sorry I'm being so unresponsive at the moment (or, as usual).
When I sent the fixed version afterwards (minus mistaken VM_BUG_ON, plus safer mem_cgroup_force_empty_list), I expected you or Konstantin to respond with a patch to fix it as you preferred (at offline/online); so this was on hold until we could compare and decide between them.
In the meantime, I assume, we've all come to feel that this way is simple, and probably the best way for now; or at least good enough, and we all have better things to do than play with alternatives.
I'll write up the description of the fixed version, and post it for 3.7, including the Acks from Hannes and KAMEZAWA (assuming they carry forward to the second version) - but probably not today or tomorrow.
But please help me: I still don't know if it's needed for -stable. We introduced the bug in 3.5, but I see lots of memory hotplug fixes coming by from you and others, so I do not know if this lruvec->zone fix is useful by itself in 3.5 and 3.6, or not - please tell me.
Thanks,
Hugh
Quoted 122 lines
Thanks
Wen CongyangQuoted 118 lines
---
include/linux/mmzone.h | 2 - mm/memcontrol.c | 40 ++++++++++++++++++++++++++++++++------- mm/mmzone.c | 6 ----- mm/page_alloc.c | 2 - 4 files changed, 36 insertions(+), 14 deletions(-)include/linux/mmzone.h +1
-1@@ -744,7 +744,7 @@ extern int init_currently_empty_zone(strunsigned long size, enum memmap_context context); -extern void lruvec_init (struct lruvec *lruvec,struct zone *zone); +extern void lruvec_init (struct lruvec *lruvec);static inline struct zone *lruvec_zone (struct lruvec *lruvec) {mm/memcontrol.c +33
-7@@ -1061,12 +1061,25 @@ struct lruvec *mem_cgroup_zone_lruvec(st struct mem_cgroup *memcg) {struct mem_cgroup_per_zone *mz; +struct lruvec *lruvec; -if (mem_cgroup_disabled ()) -return &zone->lruvec; +if (mem_cgroup_disabled ()) { + lruvec = &zone->lruvec; + goto out; + } mz =mem_cgroup_zoneinfo (memcg,zone_to_nid (zone),zone_idx (zone)); -return &mz->lruvec; + lruvec = &mz->lruvec; +out: +/* +* Since a node can be onlined after the mem_cgroup was created, +* we have to be prepared to initialize lruvec->zone here. +*/ +if (unlikely (lruvec->zone != zone)) { +VM_BUG_ON (lruvec->zone); + lruvec->zone = zone; + } +return lruvec; }/* @@ -1093,9 +1106,12 @@ struct lruvec *mem_cgroup_page_lruvec(ststruct mem_cgroup_per_zone *mz;struct mem_cgroup *memcg;struct page_cgroup *pc; +struct lruvec *lruvec; -if (mem_cgroup_disabled ()) -return &zone->lruvec; +if (mem_cgroup_disabled ()) { + lruvec = &zone->lruvec; + goto out; + } pc =lookup_page_cgroup (page); memcg = pc->mem_cgroup;@@ -1113,7 +1129,17 @@ struct lruvec *mem_cgroup_page_lruvec(st pc->mem_cgroup = memcg = root_mem_cgroup; mz =page_cgroup_zoneinfo (memcg, page); -return &mz->lruvec; + lruvec = &mz->lruvec; +out: +/* +* Since a node can be onlined after the mem_cgroup was created, +* we have to be prepared to initialize lruvec->zone here. +*/ +if (unlikely (lruvec->zone != zone)) { +VM_BUG_ON (lruvec->zone); + lruvec->zone = zone; + } +return lruvec; }/** @@ -4742,7 +4768,7 @@ static int alloc_mem_cgroup_per_zone_inffor (zone =0 ; zone <MAX_NR_ZONES ; zone++) { mz = &pn->zoneinfo[zone]; -lruvec_init (&mz->lruvec, &NODE_DATA (node)->node_zones[zone]); +lruvec_init (&mz->lruvec); mz->usage_in_excess =0 ; mz->on_tree = false; mz->memcg = memcg;mm/mmzone.c +1
-5@@ -87,7 +87,7 @@ int memmap_valid_within(unsigned long pf }#endif /* CONFIG_ARCH_HAS_HOLES_MEMORYMODEL */ -void lruvec_init (struct lruvec *lruvec,struct zone *zone) +void lruvec_init (struct lruvec *lruvec) {enum lru_list lru;@@ -95,8 +95,4 @@ void lruvec_init(struct lruvec *lruvec,for_each_lru (lru)INIT_LIST_HEAD (&lruvec->lists[lru]); - -#ifdef CONFIG_MEMCG - lruvec->zone = zone; -#endif }mm/page_alloc.c +0
-0@@ -4456,7 +4456,7 @@ static void __paginginit free_area_init_ zone->zone_pgdat = pgdat;
Quoted 9 lines
mm/page_alloc.c static void __paginginit free_area_init_ · line 4462
Quoted 7 lines
mm/page_alloc.c +1
-1zone_pcp_init (zone); -lruvec_init (&zone->lruvec, zone); +lruvec_init (&zone->lruvec);if (!size)continue ;
--
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>
Johannes Weiner
maintainerQuoted 20 lines · Hugh Dickins
Quoted 11 lines · Wen Congyang
At 09/14/2012 09:36 AM, Hugh Dickins Wrote:
Quoted 6 lines
Description to be filled in later: would it be needed for -stable, or is onlining already broken in other ways that you're now fixing up?
Reported-by Tang Chen <[email protected]>Signed-off-by Hugh Dickins <[email protected]>Hi, all:
What about the status of this patch?
Sorry I'm being so unresponsive at the moment (or, as usual).
When I sent the fixed version afterwards (minus mistaken
VM_BUG_ON, plus safermem_cgroup_force_empty_list), I expected you or Konstantin to respond with a patch to fix it as you preferred (at offline/online); so this was on hold until we could compare and decide between them.
Quoted 7 lines
In the meantime, I assume, we've all come to feel that this way is simple, and probably the best way for now; or at least good enough, and we all have better things to do than play with alternatives.
I'll write up the description of the fixed version, and post it for 3.7, including the Acks from Hannes and KAMEZAWA (assuming they carry forward to the second version) - but probably not today or tomorrow.
Mine does, thanks for asking :-)
PATCH 1/1 memcg: fix hotplugged memory zone oops
When MEMCG is configured on (even when it's disabled by boot option), when adding or removing a page to/from its lru list, the zone pointer used for stats updates is nowadays taken from the struct lruvec. (On many configurations, calculating zone from page is slower.)
But we have no code to update all the lruvecs (per zone, per memcg) when a memory node is hotadded. Here's an extract from the oops which results when running numactl to bind a program to a newly onlined node:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000f60 IP: [<ffffffff811870b9>] __mod_zone_page_state+0x9/0x60 PGD 0 Oops: 0000 [#1] SMP CPU 2 Pid: 1219, comm: numactl Not tainted 3.6.0-rc5+ #180 Bochs Bochs Process numactl (pid: 1219, threadinfo ffff880039abc000, task ffff8800383c4ce0) Stack: ffff880039abdaf8 ffffffff8117390f ffff880039abdaf8 000000008167c601 ffffffff81174162 ffff88003a480f00 0000000000000001 ffff8800395e0000 ffff88003dbd0e80 0000000000000282 ffff880039abdb48 ffffffff81174181 Call Trace: [<ffffffff8117390f>] __pagevec_lru_add_fn+0xdf/0x140 [<ffffffff81174181>] pagevec_lru_move_fn+0xb1/0x100 [<ffffffff811741ec>] __pagevec_lru_add+0x1c/0x30 [<ffffffff81174383>] lru_add_drain_cpu+0xa3/0x130 [<ffffffff8117443f>] lru_add_drain+0x2f/0x40 ...
The natural solution might be to use a memcg callback whenever memory is hotadded; but that solution has not been scoped out, and it happens that we do have an easy location at which to update lruvec->zone. The lruvec pointer is discovered either by mem_cgroup_zone_lruvec() or by mem_cgroup_page_lruvec(), and both of those do know the right zone.
So check and set lruvec->zone in those; and remove the inadequate attempt to set lruvec->zone from lruvec_init(), which is called before NODE_DATA(node) has been allocated in such cases.
Ah, there was one exceptionr. For no particularly good reason, mem_cgroup_force_empty_list() has its own code for deciding lruvec. Change it to use the standard mem_cgroup_zone_lruvec() and mem_cgroup_get_lru_size() too. In fact it was already safe against such an oops (the lru lists in danger could only be empty), but we're better proofed against future changes this way.
---
I've marked this for stable (3.6) since we introduced the problem in 3.5 (now closed to stable); but I have no idea if this is the only fix needed to get memory hotadd working with memcg in 3.6, and received no answer when I enquired twice before.
include/linux/mmzone.h | 2 - mm/memcontrol.c | 46 +++++++++++++++++++++++++++++---------- mm/mmzone.c | 6 ----- mm/page_alloc.c | 2 - 4 files changed, 38 insertions(+), 18 deletions(-)
include/linux/mmzone.h +1 -1
@@ -752,7 +752,7 @@ extern int init_currently_empty_zone(strunsigned long size, enum memmap_context context); -extern void lruvec_init (struct lruvec *lruvec,struct zone *zone); +extern void lruvec_init (struct lruvec *lruvec);static inline struct zone *lruvec_zone (struct lruvec *lruvec) {
mm/memcontrol.c +35 -11
87 lines not on this page · raw message
mm/mmzone.c +1 -5
@@ -87,7 +87,7 @@ int memmap_valid_within(unsigned long pf }#endif /* CONFIG_ARCH_HAS_HOLES_MEMORYMODEL */ -void lruvec_init (struct lruvec *lruvec,struct zone *zone) +void lruvec_init (struct lruvec *lruvec) {enum lru_list lru;
@@ -95,8 +95,4 @@ void lruvec_init(struct lruvec *lruvec,for_each_lru (lru)INIT_LIST_HEAD (&lruvec->lists[lru]); - -#ifdef CONFIG_MEMCG - lruvec->zone = zone; -#endif }
mm/page_alloc.c +1 -1
@@ -4505,7 +4505,7 @@ static void __paginginit free_area_init_ zone->zone_pgdat = pgdat;zone_pcp_init (zone); -lruvec_init (&zone->lruvec, zone); +lruvec_init (&zone->lruvec);if (!size)continue ;
Re: [PATCH] memcg: fix hotplugged memory zone oops
Quoted 45 lines · Hugh Dickins
When MEMCG is configured on (even when it's disabled by boot option), when adding or removing a page to/from its lru list, the zone pointer used for stats updates is nowadays taken from the struct lruvec. (On many configurations, calculating zone from page is slower.)
But we have no code to update all the lruvecs (per zone, per memcg) when a memory node is hotadded. Here's an extract from the oops which results when running numactl to bind a program to a newly onlined node:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000f60 IP: [<ffffffff811870b9>] __mod_zone_page_state+0x9/0x60 PGD 0 Oops: 0000 [#1] SMP CPU 2 Pid: 1219, comm: numactl Not tainted 3.6.0-rc5+ #180 Bochs Bochs Process numactl (pid: 1219, threadinfo ffff880039abc000, task ffff8800383c4ce0) Stack: ffff880039abdaf8 ffffffff8117390f ffff880039abdaf8 000000008167c601 ffffffff81174162 ffff88003a480f00 0000000000000001 ffff8800395e0000 ffff88003dbd0e80 0000000000000282 ffff880039abdb48 ffffffff81174181 Call Trace: [<ffffffff8117390f>] __pagevec_lru_add_fn+0xdf/0x140 [<ffffffff81174181>] pagevec_lru_move_fn+0xb1/0x100 [<ffffffff811741ec>] __pagevec_lru_add+0x1c/0x30 [<ffffffff81174383>] lru_add_drain_cpu+0xa3/0x130 [<ffffffff8117443f>] lru_add_drain+0x2f/0x40 ...The natural solution might be to use a memcg callback whenever memory is hotadded; but that solution has not been scoped out, and it happens that we do have an easy location at which to update
lruvec->zone. The lruvec pointer is discovered either bymem_cgroup_zone_lruvec()or bymem_cgroup_page_lruvec(), and both of those do know the right zone.So check and set
lruvec->zonein those; and remove the inadequate attempt to setlruvec->zonefromlruvec_init(), which is called beforeNODE_DATA(node)has been allocated in such cases.Ah, there was one exceptionr. For no particularly good reason,
mem_cgroup_force_empty_list()has its own code for deciding lruvec. Change it to use the standardmem_cgroup_zone_lruvec()andmem_cgroup_get_lru_size()too. In fact it was already safe against such an oops (the lru lists in danger could only be empty), but we're better proofed against future changes this way.
Quoted 7 lines
Reported-by Tang Chen <[email protected]>Signed-off-by Hugh Dickins <[email protected]>Acked-by Johannes Weiner <[email protected]>Cc Konstantin Khlebnikov <[email protected]>Cc Wen Congyang <[email protected]>
OK, it adds "an overhead" also when there is no hotadd going on but this is just one additional mem access&cmp&je so it shouldn't be noticable (lruvec->zone is used most of the time later so it not a pointless load).
It is also easier to backport for stable. But is there any reason to fix it later properly in the hotadd hook?
Anyway
Thanks!
Quoted 148 lines
---
I've marked this for stable (3.6) since we introduced the problem in 3.5 (now closed to stable); but I have no idea if this is the only fix needed to get memory hotadd working with memcg in 3.6, and received no answer when I enquired twice before.include/linux/mmzone.h | 2 - mm/memcontrol.c | 46 +++++++++++++++++++++++++++++---------- mm/mmzone.c | 6 ----- mm/page_alloc.c | 2 - 4 files changed, 38 insertions(+), 18 deletions(-)include/linux/mmzone.h +1
-1@@ -752,7 +752,7 @@ extern int init_currently_empty_zone(strunsigned long size, enum memmap_context context); -extern void lruvec_init (struct lruvec *lruvec,struct zone *zone); +extern void lruvec_init (struct lruvec *lruvec);static inline struct zone *lruvec_zone (struct lruvec *lruvec) {mm/memcontrol.c +35
-11@@ -1055,12 +1055,24 @@ struct lruvec *mem_cgroup_zone_lruvec(st struct mem_cgroup *memcg) {struct mem_cgroup_per_zone *mz; +struct lruvec *lruvec; -if (mem_cgroup_disabled ()) -return &zone->lruvec; +if (mem_cgroup_disabled ()) { + lruvec = &zone->lruvec; + goto out; + } mz =mem_cgroup_zoneinfo (memcg,zone_to_nid (zone),zone_idx (zone)); -return &mz->lruvec; + lruvec = &mz->lruvec; +out: +/* +* Since a node can be onlined after the mem_cgroup was created, +* we have to be prepared to initialize lruvec->zone here; +* and if offlined then reonlined, we need to reinitialize it. +*/ +if (unlikely (lruvec->zone != zone)) + lruvec->zone = zone; +return lruvec; }/* @@ -1087,9 +1099,12 @@ struct lruvec *mem_cgroup_page_lruvec(ststruct mem_cgroup_per_zone *mz;struct mem_cgroup *memcg;struct page_cgroup *pc; +struct lruvec *lruvec; -if (mem_cgroup_disabled ()) -return &zone->lruvec; +if (mem_cgroup_disabled ()) { + lruvec = &zone->lruvec; + goto out; + } pc =lookup_page_cgroup (page); memcg = pc->mem_cgroup;@@ -1107,7 +1122,16 @@ struct lruvec *mem_cgroup_page_lruvec(st pc->mem_cgroup = memcg = root_mem_cgroup; mz =page_cgroup_zoneinfo (memcg, page); -return &mz->lruvec; + lruvec = &mz->lruvec; +out: +/* +* Since a node can be onlined after the mem_cgroup was created, +* we have to be prepared to initialize lruvec->zone here; +* and if offlined then reonlined, we need to reinitialize it. +*/ +if (unlikely (lruvec->zone != zone)) + lruvec->zone = zone; +return lruvec; }/** @@ -3688,17 +3712,17 @@ unsigned long mem_cgroup_soft_limit_reclstatic bool mem_cgroup_force_empty_list (struct mem_cgroup *memcg,int node,int zid,enum lru_list lru) { -struct mem_cgroup_per_zone *mz; +struct lruvec *lruvec;unsigned long flags, loop;struct list_head *list;struct page *busy;struct zone *zone; zone = &NODE_DATA (node)->node_zones[zid]; - mz =mem_cgroup_zoneinfo (memcg, node, zid); - list = &mz->lruvec.lists[lru]; + lruvec =mem_cgroup_zone_lruvec (zone, memcg); + list = &lruvec->lists[lru]; - loop = mz->lru_size[lru]; + loop =mem_cgroup_get_lru_size (lruvec, lru);/* give some margin against EBUSY etc...*/ loop +=256 ; busy =NULL ;@@ -4736,7 +4760,7 @@ static int alloc_mem_cgroup_per_zone_inffor (zone =0 ; zone <MAX_NR_ZONES ; zone++) { mz = &pn->zoneinfo[zone]; -lruvec_init (&mz->lruvec, &NODE_DATA (node)->node_zones[zone]); +lruvec_init (&mz->lruvec); mz->usage_in_excess =0 ; mz->on_tree = false; mz->memcg = memcg;mm/mmzone.c +1
-5@@ -87,7 +87,7 @@ int memmap_valid_within(unsigned long pf }#endif /* CONFIG_ARCH_HAS_HOLES_MEMORYMODEL */ -void lruvec_init (struct lruvec *lruvec,struct zone *zone) +void lruvec_init (struct lruvec *lruvec) {enum lru_list lru;@@ -95,8 +95,4 @@ void lruvec_init(struct lruvec *lruvec,for_each_lru (lru)INIT_LIST_HEAD (&lruvec->lists[lru]); - -#ifdef CONFIG_MEMCG - lruvec->zone = zone; -#endif }mm/page_alloc.c +1
-1@@ -4505,7 +4505,7 @@ static void __paginginit free_area_init_ zone->zone_pgdat = pgdat;zone_pcp_init (zone); -lruvec_init (&zone->lruvec, zone); +lruvec_init (&zone->lruvec);if (!size)continue ;
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH] memcg: fix hotplugged memory zone oops
Quoted 1 line · Michal Hocko
On Thu 01-11-12 18:28:02, Hugh Dickins wrote:
[...]
And I forgot to mention that the following hunk will clash with "memcg: Simplify mem_cgroup_force_empty_list error handling" which is in linux-next already (via Tejun's tree).
Would it be easier to split the patch into the real fix and the hunk bellow? That one doesn't have to go into stable anyway and we would save some merging conflicts. The updated fix on top of -mm tree is bellow for your convinience.
Quoted 18 lines
Quoted 18 lines
/**
(quoted diff) +3
-3@@ -3688,17 +3712,17 @@ unsigned long mem_cgroup_soft_limit_recl static bool mem_cgroup_force_empty_list(struct mem_cgroup *memcg, int node, int zid, enum lru_list lru) { - struct mem_cgroup_per_zone *mz; + struct lruvec *lruvec; unsigned long flags, loop; struct list_head *list; struct page *busy; struct zone *zone; zone = &NODE_DATA(node)->node_zones[zid]; - mz = mem_cgroup_zoneinfo(memcg, node, zid); - list = &mz->lruvec.lists[lru]; + lruvec = mem_cgroup_zone_lruvec(zone, memcg); + list = &lruvec->lists[lru];
Quoted 5 lines
Review unsigned long mem_cgroup_soft_limit_recl · line 3728
Quoted 5 lines
(quoted diff) +1
-1- loop = mz->lru_size[lru]; + loop = mem_cgroup_get_lru_size(lruvec, lru); /* give some margin against EBUSY etc...*/ loop += 256; busy = NULL;
---
From 0e55c305a050502a4b2f5167efed58bb6585520b Mon Sep 17 00:00:00 2001
From: Hugh Dickins <[email protected]>
Date: Fri, 2 Nov 2012 11:47:48 +0100
Subject: [PATCH] memcg: use mem_cgroup_zone_lruvec in
mem_cgroup_force_empty_list
For no particularly good reason, mem_cgroup_force_empty_list() has its own code for deciding lruvec. Change it to use the standard mem_cgroup_zone_lruvec() and mem_cgroup_get_lru_size() too. In fact it was already safe against oops after memory hotadd (see "memcg: fix hotplugged memory zone oops" for more details) when lruvec is not initialized yet (the lru lists in danger could only be empty), but we're better proofed against future changes this way.
--- mm/memcontrol.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
mm/memcontrol.c +3 -3
@@ -3710,15 +3710,15 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order,static void mem_cgroup_force_empty_list (struct mem_cgroup *memcg,int node,int zid,enum lru_list lru) { -struct mem_cgroup_per_zone *mz; +struct lruvec *lruvec;unsigned long flags;struct list_head *list;struct page *busy;struct zone *zone; zone = &NODE_DATA (node)->node_zones[zid]; - mz =mem_cgroup_zoneinfo (memcg, node, zid); - list = &mz->lruvec.lists[lru]; + lruvec =mem_cgroup_zone_lruvec (zone, memcg); + list = &lruvec->lists[lru]; busy =NULL ; do {
Signature · 11 lines
1.7.10.4 -- Michal Hocko SUSE Labs -- 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>
Re: [PATCH] memcg: fix hotplugged memory zone oops
Quoted 7 lines · Michal Hocko
Quoted 1 line · Michal Hocko
On Thu 01-11-12 18:28:02, Hugh Dickins wrote:
[...]
And I forgot to mention that the following hunk will clash with "memcg: Simplify
mem_cgroup_force_empty_listerror handling" which is in linux-next already (via Tejun's tree).
Oh, via Tejun's tree. Right, when I checked mmotm there was no problem.
Quoted 4 lines
Would it be easier to split the patch into the real fix and the hunk bellow? That one doesn't have to go into stable anyway and we would save some merging conflicts. The updated fix on top of -mm tree is bellow for your convinience.
I'd prefer to leave it as one patch, so even the "future proof" part of the fix goes into 3.7 and stable. But your point is that you have already seen the future, and it forks in a slightly different direction!
Well, I don't want to be obstructive, but it doesn't look difficult to resolve. Perhaps if I hold off on splitting them, and see if akpm barks at me or not :)
Hugh
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
/**
mm/memcontrol.c +3
-3@@ -3688,17 +3712,17 @@ unsigned long mem_cgroup_soft_limit_reclstatic bool mem_cgroup_force_empty_list (struct mem_cgroup *memcg,int node,int zid,enum lru_list lru) { -struct mem_cgroup_per_zone *mz; +struct lruvec *lruvec;unsigned long flags, loop;struct list_head *list;struct page *busy;struct zone *zone; zone = &NODE_DATA (node)->node_zones[zid]; - mz =mem_cgroup_zoneinfo (memcg, node, zid); - list = &mz->lruvec.lists[lru]; + lruvec =mem_cgroup_zone_lruvec (zone, memcg); + list = &lruvec->lists[lru];
Quoted 5 lines
Quoted 5 lines
Review unsigned long mem_cgroup_soft_limit_recl · line 3728
Quoted 5 lines
(quoted diff) +1
-1- loop = mz->lru_size[lru]; + loop = mem_cgroup_get_lru_size(lruvec, lru); /* give some margin against EBUSY etc...*/ loop += 256; busy = NULL;
--
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>
Re: [PATCH] memcg: fix hotplugged memory zone oops
Quoted 10 lines · Hugh Dickins
Quoted 7 lines · Michal Hocko
Quoted 1 line · Michal Hocko
On Thu 01-11-12 18:28:02, Hugh Dickins wrote:
[...]
And I forgot to mention that the following hunk will clash with "memcg: Simplify
mem_cgroup_force_empty_listerror handling" which is in linux-next already (via Tejun's tree).Oh, via Tejun's tree. Right, when I checked mmotm there was no problem.
Yeah, whole that thing goes through Tejun's tree because there are many follow up clean ups depending on that change.
Quoted 11 lines
Quoted 4 lines
Would it be easier to split the patch into the real fix and the hunk bellow? That one doesn't have to go into stable anyway and we would save some merging conflicts. The updated fix on top of -mm tree is bellow for your convinience.
I'd prefer to leave it as one patch, so even the "future proof" part of the fix goes into 3.7 and stable. But your point is that you have already seen the future, and it forks in a slightly different direction!
Well, I don't want to be obstructive, but it doesn't look difficult to resolve.
True.
Quoted 28 lines
Perhaps if I hold off on splitting them, and see if akpm barks at me or not :)
Hugh
Quoted 23 lines
Quoted 23 lines
Quoted 23 lines
/**
mm/memcontrol.c +4
-4@@ -3688,17 +3712,17 @@ unsigned long mem_cgroup_soft_limit_reclstatic bool mem_cgroup_force_empty_list (struct mem_cgroup *memcg,int node,int zid,enum lru_list lru) { -struct mem_cgroup_per_zone *mz; +struct lruvec *lruvec;unsigned long flags, loop;struct list_head *list;struct page *busy;struct zone *zone; zone = &NODE_DATA (node)->node_zones[zid]; - mz =mem_cgroup_zoneinfo (memcg, node, zid); - list = &mz->lruvec.lists[lru]; + lruvec =mem_cgroup_zone_lruvec (zone, memcg); + list = &lruvec->lists[lru]; - loop = mz->lru_size[lru]; + loop =mem_cgroup_get_lru_size (lruvec, lru);/* give some margin against EBUSY etc...*/ loop +=256 ; busy =NULL ;
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH] memcg: fix hotplugged memory zone oops
In reply to Michal HockoQuoted 5 lines · Michal Hocko
OK, it adds "an overhead" also when there is no hotadd going on but this is just one additional mem access&cmp&je so it shouldn't be noticable (
lruvec->zoneis used most of the time later so it not a pointless load).
I think so too.
Quoted 1 line
It is also easier to backport for stable.
Yes.
Quoted 1 line
But is there any reason to fix it later properly in the hotadd hook?
I don't know. Not everybody liked it fixed this way: it's not unreasonable to see this as a quick hack rather than the right fix.
I was expecting objectors to post alternative hotadd hook patches, then we could compare and decide. That didn't happen; but we can certainly look to taking out these lines later if something we agree is better comes along. Not high on anyone's agenda, I think.
Quoted 3 lines
Anyway
Acked-by Michal Hocko <[email protected]>
Thanks,
Hugh
--
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
Acked-byQuoted 66 lines · Hugh Dickins
Quoted 10 lines · Johannes Weiner
Quoted 6 lines · Wen Congyang
root_mem_cgroup->info.nodeinfois initialized when the system boots. ButNODE_DATA(nid)is null if the node is not onlined, so root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone contains an invalid pointer. If we use numactl to bind a program to the node after onlining the node and its memory, it will cause the kernel panicked:Is there any chance we could get rid of the zone backpointer in lruvec again instead?
It could be done, but it would make me sad :(
Quoted 2 lines
Adding new nodes is a rare event and so updating every single memcg in the system might be just borderline crazy.
Not horribly crazy, but rather ugly, yes.
Quoted 5 lines
But can't
we just go back to passing the zone along with the lruvec downvmscan.cpaths? I agree it's ugly to pass both, given their relationship. But I don't think the backpointer is any cleaner but in addition less robust.It's like how we use
vma->mm: we could change everywhere to pass mm with vma, but it looks cleaner and cuts down on long arglists to have mm in vma.
From past experience, one of the things I worried about was adding extra
args to the reclaim stack.Quoted 12 lines
That being said, the crashing code in particular makes me wonder:
static
__always_inlinevoid add_page_to_lru_list(struct page *page,
struct lruvec *lruvec, enumlru_listlru){ int nr_pages = hpage_nr_pages(page); mem_cgroup_update_lru_size(lruvec, lru, nr_pages); list_add(&page->lru, &lruvec->lists[lru]); __mod_zone_page_state(lruvec_zone(lruvec), NR_LRU_BASE + lru, nr_pages); }Quoted 4 lines
Why did we ever pass zone in here and then felt the need to replace it with
lruvec->zonein fa9add6 "mm/memcg: apply add/del_page to lruvec"? A page does not roam between zones, its zone is a static property that can be retrieved withpage_zone().Just as in
vmscan.c, we have the lruvec to hand, and that's what we mainly want to operate upon, but there is also some need for zone.(Both Konstantin and I were looking towards the day when we move the
lru_lockinto the lruvec, removing more dependence on "zone". Pretty much the only reason that hasn't happened yet, is that we have not found time to make a performance case convincingly - but that's another topic.)Yes,
page_zone(page)is a static property of the page, but it's not necessarily cheap to evaluate: depends on how complex the memory model and the spare page flags space, doesn't it? We both preferred to derive zone from lruvec where convenient.How do you feel about this patch, and does it work for you guys?
You'd be right if you guessed that I started out without the
mem_cgroup_zone_lruvecpart of it, but oops inget_scan_counttold me that's needed too.
Quoted 5 lines
Description to be filled in later: would it be needed for -stable, or is onlining already broken in other ways that you're now fixing up?
Reported-by Tang Chen <[email protected]>Signed-off-by Hugh Dickins <[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>
At 09/14/2012 04:59 AM, Johannes Weiner Wrote:
Quoted 29 lines
Hi,
Quoted 6 lines · Wen Congyang
root_mem_cgroup->info.nodeinfois initialized when the system boots. ButNODE_DATA(nid)is null if the node is not onlined, so root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone contains an invalid pointer. If we use numactl to bind a program to the node after onlining the node and its memory, it will cause the kernel panicked:Is there any chance we could get rid of the zone backpointer in lruvec again instead? Adding new nodes is a rare event and so updating every single memcg in the system might be just borderline crazy. But can't we just go back to passing the zone along with the lruvec down
vmscan.cpaths? I agree it's ugly to pass both, given their relationship. But I don't think the backpointer is any cleaner but in addition less robust.That being said, the crashing code in particular makes me wonder:
static
__always_inlinevoid add_page_to_lru_list(struct page *page,
struct lruvec *lruvec, enumlru_listlru){ int nr_pages = hpage_nr_pages(page); mem_cgroup_update_lru_size(lruvec, lru, nr_pages); list_add(&page->lru, &lruvec->lists[lru]); __mod_zone_page_state(lruvec_zone(lruvec), NR_LRU_BASE + lru, nr_pages); }
Quoted 4 lines
Why did we ever pass zone in here and then felt the need to replace it with
lruvec->zonein fa9add6 "mm/memcg: apply add/del_page to lruvec"? A page does not roam between zones, its zone is a static property that can be retrieved withpage_zone().
Hmm, if we don't update lruvec_zone(lruvec) when a node is onlined, lruver_zone(lruvec) contains a invalid pointer, so we should check all place that accesses lruvec_zone(lruvec). We store zone in lruvec but we can't access it. And we can't avoid to access it in the furture. So I think it is better to update it when the node is onlined.
Thanks
Wen Congyang
Quoted 117 lines
Johannes
Quoted 114 lines
[ 63.413436] BUG: unable to handle kernel NULL pointer dereference at 0000000000000f60 [ 63.414161] IP: [<ffffffff811870b9>] __mod_zone_page_state+0x9/0x60 [ 63.414161] PGD 0 [ 63.414161] Oops: 0000 [#1] SMP [ 63.414161] Modules linked in: acpi_memhotplug binfmt_misc dm_mirror dm_region_hash dm_log dm_mod ppdev sg microcode pcspkr virtio_console virtio_balloon snd_intel8x0 snd_ac9 7_codec ac97_bus snd_seq snd_seq_device snd_pcm snd_timer snd soundcore snd_page_alloc e1000 i2c_piix4 i2c_core floppy parport_pc parport sr_mod cdrom virtio_blk pata_acpi ata_g eneric ata_piix libata scsi_mod [ 63.414161] CPU 2 [ 63.414161] Pid: 1219, comm: numactl Not tainted 3.6.0-rc5+ #180 Bochs Bochs ... [ 63.414161] Process numactl (pid: 1219, threadinfo ffff880039abc000, task ffff8800383c4ce0) [ 63.414161] Stack: [ 63.414161] ffff880039abdaf8 ffffffff8117390f ffff880039abdaf8 000000008167c601 [ 63.414161] ffffffff81174162 ffff88003a480f00 0000000000000001 ffff8800395e0000 [ 63.414161] ffff88003dbd0e80 0000000000000282 ffff880039abdb48 ffffffff81174181 [ 63.414161] Call Trace: [ 63.414161] [<ffffffff8117390f>] __pagevec_lru_add_fn+0xdf/0x140 [ 63.414161] [<ffffffff81174162>] ? pagevec_lru_move_fn+0x92/0x100 [ 63.414161] [<ffffffff81174181>] pagevec_lru_move_fn+0xb1/0x100 [ 63.414161] [<ffffffff81173830>] ? lru_add_page_tail+0x1b0/0x1b0 [ 63.414161] [<ffffffff811dff71>] ? exec_mmap+0x121/0x230 [ 63.414161] [<ffffffff811741ec>] __pagevec_lru_add+0x1c/0x30 [ 63.414161] [<ffffffff81174383>] lru_add_drain_cpu+0xa3/0x130 [ 63.414161] [<ffffffff8117443f>] lru_add_drain+0x2f/0x40 [ 63.414161] [<ffffffff81196da9>] exit_mmap+0x69/0x160 [ 63.414161] [<ffffffff810db115>] ? lock_release_holdtime+0x35/0x1a0 [ 63.414161] [<ffffffff81069c37>] mmput+0x77/0x100 [ 63.414161] [<ffffffff811dffc0>] exec_mmap+0x170/0x230 [ 63.414161] [<ffffffff811e0152>] flush_old_exec+0xd2/0x140 [ 63.414161] [<ffffffff812343ea>] load_elf_binary+0x32a/0xe70 [ 63.414161] [<ffffffff810d700d>] ? trace_hardirqs_off+0xd/0x10 [ 63.414161] [<ffffffff810b231f>] ? local_clock+0x6f/0x80 [ 63.414161] [<ffffffff810db115>] ? lock_release_holdtime+0x35/0x1a0 [ 63.414161] [<ffffffff810dd813>] ? __lock_release+0x133/0x1a0 [ 63.414161] [<ffffffff811e22e7>] ? search_binary_handler+0x1a7/0x4a0 [ 63.414161] [<ffffffff811e22f3>] search_binary_handler+0x1b3/0x4a0 [ 63.414161] [<ffffffff811e2194>] ? search_binary_handler+0x54/0x4a0 [ 63.414161] [<ffffffff812340c0>] ? set_brk+0xe0/0xe0 [ 63.414161] [<ffffffff811e284f>] do_execve_common+0x26f/0x320 [ 63.414161] [<ffffffff811bde33>] ? kmem_cache_alloc+0x113/0x220 [ 63.414161] [<ffffffff811e298a>] do_execve+0x3a/0x40 [ 63.414161] [<ffffffff8102061a>] sys_execve+0x4a/0x80 [ 63.414161] [<ffffffff81686c6c>] stub_execve+0x6c/0xc0 [ 63.414161] Code: ff 03 00 00 48 c1 e7 0b 48 c1 e2 07 48 29 d7 48 03 3c c5 c0 27 d2 81 e8 a6 fe ff ff c9 c3 0f 1f 40 00 55 48 89 e5 0f 1f 44 00 00 <48> 8b 4f 60 89 f6 48 8d 44 31 40 65 44 8a 40 02 45 0f be c0 41The reason is that we don't update root_mem_cgroup->info.nodeinfo[nid]->zoneinfo[zone].lruvec.zone when onlining the node, and we try to access it.
Signed-off-by Wen Congyang <[email protected]>Reported-by Tang Chen <[email protected]>--- include/linux/memcontrol.h | 7 +++++++ mm/memcontrol.c | 14 ++++++++++++++ mm/memory_hotplug.c | 2 ++ 3 files changed, 23 insertions(+), 0 deletions(-)include/linux/memcontrol.h +7
-0@@ -182,6 +182,9 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order, unsigned long *total_scanned);void mem_cgroup_count_vm_event (struct mm_struct *mm,enum vm_event_item idx); + +void update_root_mem_cgroup (int nid); +#ifdef CONFIG_TRANSPARENT_HUGEPAGE void mem_cgroup_split_huge_fixup (struct page *head);#endif @@ -374,6 +377,10 @@ static inline void mem_cgroup_replace_page_cache(struct page *oldpage, struct page *newpage) { } + +static inline void update_root_mem_cgroup (int nid) +{ +}#endif /* CONFIG_MEMCG */ #if !defined(CONFIG_MEMCG ) || !defined(CONFIG_DEBUG_VM )mm/memcontrol.c +14
-0@@ -3427,6 +3427,20 @@ void mem_cgroup_replace_page_cache(struct page *oldpage,__mem_cgroup_commit_charge (memcg, newpage,1 , type, true); } +/* NODE_DATA(nid) is changed */ +void update_root_mem_cgroup (int nid) +{ +struct mem_cgroup_per_node *pn; +struct mem_cgroup_per_zone *mz; +int zone; + + pn = root_mem_cgroup->info.nodeinfo[nid]; +for (zone =0 ; zone <MAX_NR_ZONES ; zone++) { + mz = &pn->zoneinfo[zone]; +lruvec_init (&mz->lruvec, &NODE_DATA (nid)->node_zones[zone]); + } +} +#ifdef CONFIG_DEBUG_VM static struct page_cgroup *lookup_page_cgroup_used (struct page *page) {mm/memory_hotplug.c +0
-0@@ -555,6 +555,8 @@ static pg_data_t __ref *hotadd_new_pgdat(int nid, u64 start)/* we can use NODE_DATA(nid) from here */
Quoted 8 lines
Quoted 7 lines
mm/memory_hotplug.c +2
-0+update_root_mem_cgroup (nid); +/* init node's zones as empty zones, we don't have any present pages.*/ free_area_init_node (nid, zones_size, start_pfn, zholes_size);Signature · 1 line
1.7.1
--
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>
Quoted 7 lines · Wen Congyang
Hmm, if we don't update
lruvec_zone(lruvec)when a node is onlined,lruver_zone(lruvec)contains a invalid pointer, so we should check all place that accesseslruvec_zone(lruvec). We store zone in lruvec but we can't access it. And we can't avoid to access it in the furture. So I think it is better to update it when the node is onlined.
Thanks for your feedback (-), and Johannes (+), and Konstantin (-):
though your remarks above appear to be in reply to Johannes, I took them as applying to my patch.
I think it's okay to update lruvec->zone in mem_cgroup_zone_lruvec() and mem_cgroup_page_lruvec(), rather than in each place we call lruvec_zone(lruvec), because the lruvec addressed is not pulled out of nowhere: it has been supplied by either mem_cgroup_zone_lruvec() or mem_cgroup_page_lruvec().
But your comment did prompt me to go back and check on that, and I did find one other place which makes its own determination of the
lruvec: mem_cgroup_force_empty_list(). Now, that happens to be safe at present because of its list_empty() check, but it's easy to imagine a future patch which would accidentally make it go wrong: so in the updated version of my lruvec->zone patch below, I've changed the assignments at the head of mem_cgroup_force_empty_list() to use the standard mem_cgroup_zone_lruvec() to determine lruvec.
But I've made another change (in both places) too - though if you've already been testing the earlier patch, this will not be any worse:
I've removed the VM_BUG_ONs, after realizing that hotremove of memory node followed by hotadd of memory node would need to update lruvec->zone even though it's currently non-zero - unless hotremove goes through updating all memcgs in the same way as you propose that hotadd does.
I'm not sure how I feel about my patch: really, I think I'd like to see the "update all memcgs when memory node is onlined and offlined" patch that you and Konstantin prefer (but I'd prefer one of you to write), before we make up our minds which way to go.
If it's not too complicated, yes, I may well prefer it myself too; but I worry that it may turn out to be hard to get the serialization against mem cgroup creation and destruction right.
Konstantin remarks that it would be nice to have notifier chains in memory hotplug: I believe that's already there, and, for example, KSM is one user of hotplug_memory_notifier().
I've seen you comment elsewhere that memory hotremove is not working with memcg, and I wonder if you've found it far away from working, or nearly there. That might determine which patch to choose: if it's nearly working, then you may need to update all memcgs anyway to get it fully working, in which case it would probably be right to handle lruvec->zone that way too. But if it's hard to get working, but hotadd nearly right, then my patch may be the right choice for now.
I've still not written the patch comment: it was when I started to write that, that I realized the hotadd after hotremove issue.
---
include/linux/mmzone.h | 2 - mm/memcontrol.c | 46 +++++++++++++++++++++++++++++---------- mm/mmzone.c | 6 ----- mm/page_alloc.c | 2 - 4 files changed, 38 insertions(+), 18 deletions(-)
include/linux/mmzone.h +1 -1
@@ -744,7 +744,7 @@ extern int init_currently_empty_zone(strunsigned long size, enum memmap_context context); -extern void lruvec_init (struct lruvec *lruvec,struct zone *zone); +extern void lruvec_init (struct lruvec *lruvec);static inline struct zone *lruvec_zone (struct lruvec *lruvec) {
mm/memcontrol.c +35 -11
87 lines not on this page · raw message
mm/mmzone.c +1 -5
@@ -87,7 +87,7 @@ int memmap_valid_within(unsigned long pf }#endif /* CONFIG_ARCH_HAS_HOLES_MEMORYMODEL */ -void lruvec_init (struct lruvec *lruvec,struct zone *zone) +void lruvec_init (struct lruvec *lruvec) {enum lru_list lru;
@@ -95,8 +95,4 @@ void lruvec_init(struct lruvec *lruvec,for_each_lru (lru)INIT_LIST_HEAD (&lruvec->lists[lru]); - -#ifdef CONFIG_MEMCG - lruvec->zone = zone; -#endif }
mm/page_alloc.c +1 -1
@@ -4456,7 +4456,7 @@ static void __paginginit free_area_init_ zone->zone_pgdat = pgdat;zone_pcp_init (zone); -lruvec_init (&zone->lruvec, zone); +lruvec_init (&zone->lruvec);if (!size)continue ;
--
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>