Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756650AbZAOCCD (ORCPT ); Wed, 14 Jan 2009 21:02:03 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752682AbZAOCBw (ORCPT ); Wed, 14 Jan 2009 21:01:52 -0500 Received: from fgwmail5.fujitsu.co.jp ([192.51.44.35]:58744 "EHLO fgwmail5.fujitsu.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752006AbZAOCBv (ORCPT ); Wed, 14 Jan 2009 21:01:51 -0500 Date: Thu, 15 Jan 2009 11:00:44 +0900 From: KAMEZAWA Hiroyuki To: Daisuke Nishimura Cc: "LKML" , "linux-mm" , "Andrew Morton" , "Balbir Singh" , "Pavel Emelyanov" , "Li Zefan" , "Paul Menage" Subject: Re: [RFC][PATCH 5/4] memcg: don't call res_counter_uncharge when obsolete Message-Id: <20090115110044.3a863af8.kamezawa.hiroyu@jp.fujitsu.com> In-Reply-To: <20090115100330.37d89d3d.nishimura@mxp.nes.nec.co.jp> References: <20090113184533.6ffd2af9.nishimura@mxp.nes.nec.co.jp> <20090114175121.275ecd59.nishimura@mxp.nes.nec.co.jp> <7602a77a9fc6b1e8757468048fde749a.squirrel@webmail-b.css.fujitsu.com> <20090115100330.37d89d3d.nishimura@mxp.nes.nec.co.jp> Organization: FUJITSU Co. LTD. X-Mailer: Sylpheed 2.5.0 (GTK+ 2.10.14; i686-pc-mingw32) Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-2022-JP Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8570 Lines: 227 On Thu, 15 Jan 2009 10:03:30 +0900 Daisuke Nishimura wrote: > On Wed, 14 Jan 2009 22:43:05 +0900 (JST), "KAMEZAWA Hiroyuki" wrote: > > Daisuke Nishimura さんは書きました： > > > This is a new one. Please review. > > > > > > === > > > From: Daisuke Nishimura > > > > > > mem_cgroup_get ensures that the memcg that has been got can be accessed > > > even after the directory has been removed, but it doesn't ensure that > > > parents > > > of it can be accessed: parents might have been freed already by rmdir. > > > > > > This causes a bug in case of use_hierarchy==1, because > > > res_counter_uncharge > > > climb up the tree. > > > > > > Check if the memcg is obsolete, and don't call res_counter_uncharge when > > > obsole. > > > > > Hmm, did you see panic ? > I saw 2 types of bugs, A: spinlock lockup and B: general protection fault. > (described below) > > Those bugs happened in case of (use_hierarchy && do_swap_account), > and didn't happen (at leaset I haven't seen) in case of > (!use_hierarchy && do_swap_account) nor (use_hierarchy && !do_swap_account). > And, they didn't happen with this patch applied all through the last night. > > A: spinlock lockup > === > BUG: spinlock lockup on CPU#1, mmapstress10/27706, ffff880 > 3a41ef0a0 > Pid: 27706, comm: mmapstress10 Not tainted 2.6.28-git12-7c > 99bf20 #1 > Call Trace: > [] _raw_spin_lock+0xfb/0x122 > [] _spin_lock+0x4e/0x5f > [] res_counter_uncharge+0x2a/0x70 > [] res_counter_uncharge+0x2a/0x70 > [] swap_info_get+0x6a/0xa3 > [] mem_cgroup_uncharge_swap+0x2a/0x35 > [] swap_entry_free+0x8f/0x93 > [] swap_free+0x19/0x28 > [] delete_from_swap_cache+0x36/0x43 > [] free_swap_and_cache+0xb1/0xeb > [] unmap_vmas+0x57f/0x837 > [] exit_mmap+0xa5/0x11c > [] mmput+0x41/0x9f > [] exit_mm+0x102/0x10d > [] do_exit+0x1a2/0x73e > [] __dequeue_signal+0x15/0x11c > [] do_group_exit+0x73/0xa5 > [] get_signal_to_deliver+0x34f/0x3a1 > [] do_notify_resume+0x8c/0x7a5 > [] lock_hrtimer_base+0x1b/0x3c > [] getnstimeofday+0x57/0xb6 > [] ktime_get_ts+0x22/0x4b > [] ktime_get+0xc/0x41 > [] hrtimer_nanosleep+0xa5/0xf1 > [] hrtimer_wakeup+0x0/0x22 > [] sysret_signal+0x7c/0xcb > === > > This context has hold swap_lock already, so other contexts trying to hold > swap_lock also get spinlock lockup bug. > > B: general protection fault > === > general protection fault: 0000 [#1] SMP > last sysfs file: /sys/devices/system/cpu/cpu15/cache/index1/shared_cpu_map > CPU 3 > Modules linked in: ipt_REJECT xt_tcpudp iptable_filter ip_tables x_tables bridge stp ipv6 > autofs4 hidp rfcomm l2cap bluetooth sunrpc dm_mirror dm_region_hash dm_log dm_multipath dm > _mod sbs sbshc battery ac lp sg rtc_cmos rtc_core ide_cd_mod parport_pc rtc_lib parport se > rio_raw cdrom acpi_memhotplug button e1000 i2c_i801 i2c_core shpchp pcspkr ata_piix libata > megaraid_mbox megaraid_mm sd_mod scsi_mod ext3 jbd ehci_hcd ohci_hcd uhci_hcd [last unloa > ded: microcode] > Pid: 8051, comm: bash Not tainted 2.6.29-rc1-0ed85935 #1 > RIP: 0010:[] [] _raw_spin_trylock+0x0/0x39 > RSP: 0000:ffff8800bb995e00 EFLAGS: 00010092 > RAX: ffff88010b54a620 RBX: 0097040900455377 RCX: 0000000000000000 > RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0097040900455377 > RBP: 009704090045538f R08: 0000000000000002 R09: 0000000000000001 > R10: ffffe2000c861640 R11: ffffffff8026f9b5 R12: 0000000000000296 > R13: 0000000000001000 R14: ffff8801003cf080 R15: 00007fa79a932028 > FS: 00007fa79a9316f0(0000) GS:ffff8803af7d7a80(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b > CR2: 00007fa79a932028 CR3: 00000000cc8e0000 CR4: 00000000000006e0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 > Process bash (pid: 8051, threadinfo ffff8800bb994000, task ffff88010b54a620) > Stack: > ffffffff804d8f1e ffffffff8026f9b5 0097040900455377 0097040900455357 > ffffffff8026f9b5 0000000000000282 ffff88010e1a2000 ffffe2000c861640 > ffff8801094f9000 0000000000000000 ffffffff802b7371 00000001ed3e8025 > Call Trace: > [] ? _spin_lock+0x35/0x5f > [] ? res_counter_uncharge+0x2a/0x70 > [] ? res_counter_uncharge+0x2a/0x70 > [] ? mem_cgroup_commit_charge_swapin+0x74/0x8a > [] ? handle_mm_fault+0x5e3/0x750 > [] ? do_page_fault+0x3b2/0x73e > [] ? page_fault+0x1f/0x30 > Code: 31 c0 e8 5f 4a ed ff 48 c7 c7 da df 5c 80 31 c0 e8 51 4a ed ff c7 05 cc d5 33 00 01 > 00 00 00 c7 05 62 c7 de 00 00 00 00 00 58 c3 <0f> b7 07 38 e0 8d 88 00 01 00 00 75 05 f0 6 > 6 0f b1 0f 0f 94 c1 > RIP [] _raw_spin_trylock+0x0/0x39 > RSP > ---[ end trace 1ecf768aff114688 ]--- > === > > > > To handle the problem "parent may be obsolete", > > > > call mem_cgroup_get(parent) at create() > > call mem_cgroup_put(parent) at freeing memcg. > > (regardless of use_hierarchy.) > > > > is clearer way to go, I think. > > > > I wonder whether there is mis-accounting problem or not.. > > > > So, adding css_tryget() around problematic code can be a fix. > > -- > > mem = swap_cgroup_record(); > > if (css_tryget(&mem->css)) { > > res_counter_uncharge(&mem->memsw, PAZE_SIZE); > > css_put(&mem->css) > > } > > -- > > I like css_tryget() rather than mem_cgroup_obsolete(). > I agree. > The updated version is attached. > > > Thanks, > Daisuke nishimura. > > > To be honest, I'd like to remove memcg special stuff when I can. > > > > Thanks, > > -Kame > > > === > From: Daisuke Nishimura > > mem_cgroup_get ensures that the memcg that has been got can be accessed > even after the directory has been removed, but it doesn't ensure that parents > of it can be accessed: parents might have been freed already by rmdir. > > This causes a bug in case of use_hierarchy==1, because res_counter_uncharge > climb up the tree. > > Check if the memcg is obsolete by css_tryget, and don't call > res_counter_uncharge when obsole. > > Signed-off-by: Daisuke Nishimura seems nice loock. > --- > mm/memcontrol.c | 15 ++++++++++++--- > 1 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index fb62b43..4e3b100 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -1182,7 +1182,10 @@ int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm, > /* avoid double counting */ > mem = swap_cgroup_record(ent, NULL); > if (mem) { > - res_counter_uncharge(&mem->memsw, PAGE_SIZE); > + if (!css_tryget(&mem->css)) { > + res_counter_uncharge(&mem->memsw, PAGE_SIZE); > + css_put(&mem->css); > + } > mem_cgroup_put(mem); > } > } I think css_tryget() returns "ture" at success.... So, == if (mem && css_tryget(&mem->css)) res_counter.... is correct. -Kame > @@ -1252,7 +1255,10 @@ void mem_cgroup_commit_charge_swapin(struct page *page, struct mem_cgroup *ptr) > struct mem_cgroup *memcg; > memcg = swap_cgroup_record(ent, NULL); > if (memcg) { > - res_counter_uncharge(&memcg->memsw, PAGE_SIZE); > + if (!css_tryget(&memcg->css)) { > + res_counter_uncharge(&memcg->memsw, PAGE_SIZE); > + css_put(&memcg->css); > + } > mem_cgroup_put(memcg); > } > > @@ -1397,7 +1403,10 @@ void mem_cgroup_uncharge_swap(swp_entry_t ent) > > memcg = swap_cgroup_record(ent, NULL); > if (memcg) { > - res_counter_uncharge(&memcg->memsw, PAGE_SIZE); > + if (!css_tryget(&memcg->css)) { > + res_counter_uncharge(&memcg->memsw, PAGE_SIZE); > + css_put(&memcg->css); > + } > mem_cgroup_put(memcg); > } > } > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/