From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Hocko Subject: Re: [RFC] memcg: fix race between css_offline and async charge (was: Re: Possible regression with cgroups in 3.11) Date: Wed, 13 Nov 2013 16:13:39 +0100 Message-ID: <20131113151339.GC22131@dhcp22.suse.cz> References: <5277932C.40400@huawei.com> <5278B3F1.9040502@huawei.com> <20131107235301.GB1092@cmpxchg.org> <20131111153148.GC14497@dhcp22.suse.cz> <20131112145824.GC6049@dhcp22.suse.cz> <20131113033840.GC19394@mtj.dyndns.org> <20131113110108.GA22131@dhcp22.suse.cz> <20131113132337.GB22131@dhcp22.suse.cz> <20131113145427.GG707@cmpxchg.org> Mime-Version: 1.0 Return-path: Content-Disposition: inline In-Reply-To: <20131113145427.GG707-druUgvl0LCNAfugRpC6u6w@public.gmane.org> Sender: cgroups-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Johannes Weiner Cc: Tejun Heo , Li Zefan , Markus Blank-Burian , Steven Rostedt , Hugh Dickins , David Rientjes , Ying Han , Greg Thelen , Michel Lespinasse , cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org On Wed 13-11-13 09:54:27, Johannes Weiner wrote: > On Wed, Nov 13, 2013 at 02:23:37PM +0100, Michal Hocko wrote: > > Johannes, what do you think about something like this? > > I have just compile tested it. > > --- > > >From 73042adc905847bfe401ae12073d1c479db8fdab Mon Sep 17 00:00:00 2001 > > From: Michal Hocko > > Date: Wed, 13 Nov 2013 13:53:54 +0100 > > Subject: [PATCH] memcg: fix race between css_offline and async charge > > > > As correctly pointed out by Johannes, charges done on behalf of a group > > where the current task is not its member (swap accounting currently) are > > racy wrt. memcg offlining (mem_cgroup_css_offline). > > > > This might lead to a charge leak as follows: > > > > rcu_read_lock() > > css_tryget() > > rcu_read_unlock() > > disable tryget() > > call_rcu() > > offline_css() > > reparent_charges() > > res_counter_charge() > > css_put() > > css_free() > > pc->mem_cgroup = deadcg > > add page to lru > > > > If a group has a parent then the parent's res_counter would have a > > charge which doesn't have any corresponding page on any reachable LRUs > > under its hierarchy and so it won't be able to free/reparent its own > > charges when going away and end up looping in reparent_charges for ever. > > > > This patch fixes the issue by introducing memcg->offline flag which is > > set when memcg is offlined (and the memcg is not reachable anymore). > > > > The only async charger we have currently (swapin accounting path) checks > > the offline status after successful charge and uncharges and falls back > > to charge the current task if the group is offline now. > > > > Spotted-by: Johannes Weiner > > Signed-off-by: Michal Hocko > > Ultimately, we want to have the tryget and the res_counter charge in > the same rcu readlock region because cgroup already provides rcu > protection. We need a quick fix until then. > > So I'm not sure why you are sending more patches, I already provided a > one-liner change that should take care of this and you didn't say why > it wouldn't work. I've completely forgot about that one. Sorry about that! Yes, that one will work as well (it would be sufficient to call mem_cgroup_reparent_charges only if there are any charges left). Both approaches are hacks so I do not care either way. -- Michal Hocko SUSE Labs