[PATCH 3.3] memcg: free mem_cgroup by RCU to fix oops

[PATCH 3.3] memcg: free mem_cgroup by RCU to fix oops

[Hugh Dickins]

After fixing the GPF in mem_cgroup_lru_del_list(), three times one
machine running a similar load (moving and removing memcgs while swapping)
has oopsed in mem_cgroup_zone_nr_lru_pages(), when retrieving memcg zone
numbers for get_scan_count() for shrink_mem_cgroup_zone(): this is where a
struct mem_cgroup is first accessed after being chosen by mem_cgroup_iter().

Just what protects a struct mem_cgroup from being freed, in between
mem_cgroup_iter()'s css_get_next() and its css_tryget()?  css_tryget()
fails once css->refcnt is zero with CSS_REMOVED set in flags, yes: but
what if that memory is freed and reused for something else, which sets
"refcnt" non-zero?  Hmm, and scope for an indefinite freeze if refcnt
is left at zero but flags are cleared.

It's tempting to move the css_tryget() into css_get_next(), to make it
really "get" the css, but I don't think that actually solves anything:
the same difficulty in moving from css_id found to stable css remains.

But we already have rcu_read_lock() around the two, so it's easily
fixed if __mem_cgroup_free() just uses kfree_rcu() to free mem_cgroup.

However, a big struct mem_cgroup is allocated with vzalloc() instead
of kzalloc(), and we're not allowed to vfree() at interrupt time:
there doesn't appear to be a general vfree_rcu() to help with this,
so roll our own using schedule_work().  The compiler decently removes
vfree_work() and vfree_rcu() when the config doesn't need them.

Signed-off-by: Hugh Dickins <hughd@google.com>
---

I'm posting this a little prematurely to get eyes on it, since it's
more than a two-liner, but 3.3 time is running out.  If it is what's
needed to fix my oopses, I won't really be sure before Friday morning.
What's running now on the machine affected is using kfree_rcu(), but I
did hack it earlier to check that the vfree_rcu() alternative works.

 mm/memcontrol.c |   53 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 6 deletions(-)

--- 3.3-rc6+/mm/memcontrol.c	2012-03-05 22:03:45.940000832 -0800
+++ linux/mm/memcontrol.c	2012-03-07 20:00:21.142520811 -0800
@@ -230,10 +230,30 @@ struct mem_cgroup {
 	 * the counter to account for memory usage
 	 */
 	struct res_counter res;
-	/*
-	 * the counter to account for mem+swap usage.
-	 */
-	struct res_counter memsw;
+
+	union {
+		/*
+		 * the counter to account for mem+swap usage.
+		 */
+		struct res_counter memsw;
+
+		/*
+		 * rcu_freeing is used only when freeing struct mem_cgroup,
+		 * so put it into a union to avoid wasting more memory.
+		 * It must be disjoint from the css field.  It could be
+		 * in a union with the res field, but res plays a much
+		 * larger part in mem_cgroup life than memsw, and might
+		 * be of interest, even at time of free, when debugging.
+		 * So share rcu_head with the less interesting memsw.
+		 */
+		struct rcu_head rcu_freeing;
+		/*
+		 * But when using vfree(), that cannot be done at
+		 * interrupt time, so we must then queue the work.
+		 */
+		struct work_struct work_freeing;
+	};
+
 	/*
 	 * Per cgroup active and inactive list, similar to the
 	 * per zone LRU lists.
@@ -4780,6 +4800,27 @@ out_free:
 }
 
 /*
+ * Helpers for freeing a vzalloc()ed mem_cgroup by RCU,
+ * but in process context.  The work_freeing structure is overlaid
+ * on the rcu_freeing structure, which itself is overlaid on memsw.
+ */
+static void vfree_work(struct work_struct *work)
+{
+	struct mem_cgroup *memcg;
+
+	memcg = container_of(work, struct mem_cgroup, work_freeing);
+	vfree(memcg);
+}
+static void vfree_rcu(struct rcu_head *rcu_head)
+{
+	struct mem_cgroup *memcg;
+
+	memcg = container_of(rcu_head, struct mem_cgroup, rcu_freeing);
+	INIT_WORK(&memcg->work_freeing, vfree_work);
+	schedule_work(&memcg->work_freeing);
+}
+
+/*
  * At destroying mem_cgroup, references from swap_cgroup can remain.
  * (scanning all at force_empty is too costly...)
  *
@@ -4802,9 +4843,9 @@ static void __mem_cgroup_free(struct mem
 
 	free_percpu(memcg->stat);
 	if (sizeof(struct mem_cgroup) < PAGE_SIZE)
-		kfree(memcg);
+		kfree_rcu(memcg, rcu_freeing);
 	else
-		vfree(memcg);
+		call_rcu(&memcg->rcu_freeing, vfree_rcu);
 }
 
 static void mem_cgroup_get(struct mem_cgroup *memcg)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3.3] memcg: free mem_cgroup by RCU to fix oops

[KAMEZAWA Hiroyuki]

On Wed, 7 Mar 2012 22:01:50 -0800 (PST)
Hugh Dickins <hughd@google.com> wrote:

> After fixing the GPF in mem_cgroup_lru_del_list(), three times one
> machine running a similar load (moving and removing memcgs while swapping)
> has oopsed in mem_cgroup_zone_nr_lru_pages(), when retrieving memcg zone
> numbers for get_scan_count() for shrink_mem_cgroup_zone(): this is where a
> struct mem_cgroup is first accessed after being chosen by mem_cgroup_iter().
> 
> Just what protects a struct mem_cgroup from being freed, in between
> mem_cgroup_iter()'s css_get_next() and its css_tryget()?  css_tryget()
> fails once css->refcnt is zero with CSS_REMOVED set in flags, yes: but
> what if that memory is freed and reused for something else, which sets
> "refcnt" non-zero?  Hmm, and scope for an indefinite freeze if refcnt
> is left at zero but flags are cleared.
> 
> It's tempting to move the css_tryget() into css_get_next(), to make it
> really "get" the css, but I don't think that actually solves anything:
> the same difficulty in moving from css_id found to stable css remains.
> 
> But we already have rcu_read_lock() around the two, so it's easily
> fixed if __mem_cgroup_free() just uses kfree_rcu() to free mem_cgroup.
> 
> However, a big struct mem_cgroup is allocated with vzalloc() instead
> of kzalloc(), and we're not allowed to vfree() at interrupt time:
> there doesn't appear to be a general vfree_rcu() to help with this,
> so roll our own using schedule_work().  The compiler decently removes
> vfree_work() and vfree_rcu() when the config doesn't need them.
> 
> Signed-off-by: Hugh Dickins <hughd@google.com>

Thank you.
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3.3] memcg: free mem_cgroup by RCU to fix oops

[Andrew Morton]

On Wed, 7 Mar 2012 22:01:50 -0800 (PST)
Hugh Dickins <hughd@google.com> wrote:

> After fixing the GPF in mem_cgroup_lru_del_list(), three times one
> machine running a similar load (moving and removing memcgs while swapping)
> has oopsed in mem_cgroup_zone_nr_lru_pages(), when retrieving memcg zone
> numbers for get_scan_count() for shrink_mem_cgroup_zone(): this is where a
> struct mem_cgroup is first accessed after being chosen by mem_cgroup_iter().
> 
> Just what protects a struct mem_cgroup from being freed, in between
> mem_cgroup_iter()'s css_get_next() and its css_tryget()?  css_tryget()
> fails once css->refcnt is zero with CSS_REMOVED set in flags, yes: but
> what if that memory is freed and reused for something else, which sets
> "refcnt" non-zero?  Hmm, and scope for an indefinite freeze if refcnt
> is left at zero but flags are cleared.
> 
> It's tempting to move the css_tryget() into css_get_next(), to make it
> really "get" the css, but I don't think that actually solves anything:
> the same difficulty in moving from css_id found to stable css remains.
> 
> But we already have rcu_read_lock() around the two, so it's easily
> fixed if __mem_cgroup_free() just uses kfree_rcu() to free mem_cgroup.
> 
> However, a big struct mem_cgroup is allocated with vzalloc() instead
> of kzalloc(), and we're not allowed to vfree() at interrupt time:
> there doesn't appear to be a general vfree_rcu() to help with this,
> so roll our own using schedule_work().  The compiler decently removes
> vfree_work() and vfree_rcu() when the config doesn't need them.
> 
> ...
>
> @@ -4780,6 +4800,27 @@ out_free:
>  }
>  
>  /*
> + * Helpers for freeing a vzalloc()ed mem_cgroup by RCU,
> + * but in process context.  The work_freeing structure is overlaid
> + * on the rcu_freeing structure, which itself is overlaid on memsw.
> + */
> +static void vfree_work(struct work_struct *work)
> +{
> +	struct mem_cgroup *memcg;
> +
> +	memcg = container_of(work, struct mem_cgroup, work_freeing);
> +	vfree(memcg);
> +}
> +static void vfree_rcu(struct rcu_head *rcu_head)
> +{
> +	struct mem_cgroup *memcg;
> +
> +	memcg = container_of(rcu_head, struct mem_cgroup, rcu_freeing);
> +	INIT_WORK(&memcg->work_freeing, vfree_work);
> +	schedule_work(&memcg->work_freeing);
> +}
> +
> +/*
>   * At destroying mem_cgroup, references from swap_cgroup can remain.
>   * (scanning all at force_empty is too costly...)
>   *
> @@ -4802,9 +4843,9 @@ static void __mem_cgroup_free(struct mem
>  
>  	free_percpu(memcg->stat);
>  	if (sizeof(struct mem_cgroup) < PAGE_SIZE)
> -		kfree(memcg);
> +		kfree_rcu(memcg, rcu_freeing);
>  	else
> -		vfree(memcg);
> +		call_rcu(&memcg->rcu_freeing, vfree_rcu);
>  }
>  

It's fairly possible that a vfree_rcu() will later turn up in
vmalloc.c.  I guess that for now, it's OK to add a private version and
we can cut-n-paste it over when the need arises..

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3.3] memcg: free mem_cgroup by RCU to fix oops

[Hugh Dickins]

On Wed, 7 Mar 2012, Hugh Dickins wrote:
> 
> I'm posting this a little prematurely to get eyes on it, since it's
> more than a two-liner, but 3.3 time is running out.  If it is what's
> needed to fix my oopses, I won't really be sure before Friday morning.
> What's running now on the machine affected is using kfree_rcu(), but I
> did hack it earlier to check that the vfree_rcu() alternative works.

Yes, please do send that patch on to Linus for 3.3.

It did not get as much as the 36 hours of testing I had hoped for, only
25 hours so far.  12 hours while I was out yesterday got wasted by a
wireless driver interrupt spewing approximately one million messages:

iwl3945 0000:08:00.0: MAC is in deep sleep!. CSR_GP_CNTRL = 0xFFFFFFFF

which I've not suffered from before, and hope not again.  Having kdb
in, I did take a look what was going on with the memcg load when it was
interrupted: it appeared to be normal, and I've no reason to suppose that
my kfree_rcu() was in any way responsible for the wireless aberration.

Thanks,
Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3.3] memcg: free mem_cgroup by RCU to fix oops

[Stanislaw Gruszka]

On Fri, Mar 09, 2012 at 11:58:34AM -0800, Hugh Dickins wrote:
> On Wed, 7 Mar 2012, Hugh Dickins wrote:
> > 
> > I'm posting this a little prematurely to get eyes on it, since it's
> > more than a two-liner, but 3.3 time is running out.  If it is what's
> > needed to fix my oopses, I won't really be sure before Friday morning.
> > What's running now on the machine affected is using kfree_rcu(), but I
> > did hack it earlier to check that the vfree_rcu() alternative works.
> 
> Yes, please do send that patch on to Linus for 3.3.
> 
> It did not get as much as the 36 hours of testing I had hoped for, only
> 25 hours so far.  12 hours while I was out yesterday got wasted by a
> wireless driver interrupt spewing approximately one million messages:
> 
> iwl3945 0000:08:00.0: MAC is in deep sleep!. CSR_GP_CNTRL = 0xFFFFFFFF

I replaced that with WARN_ONCE
http://marc.info/?l=linux-wireless&m=132912863701997&w=2
(the patch is currently in net-next).

> which I've not suffered from before, and hope not again.  Having kdb
> in, I did take a look what was going on with the memcg load when it was
> interrupted: it appeared to be normal, and I've no reason to suppose that
> my kfree_rcu() was in any way responsible for the wireless aberration.

I don't know if is possible if test patch influence pci or mac80211 code
(we use rcu quite intensively in mac8021). Those "MAC is in deep sleep" 
usually mean that wireless device registers can not be read through pcie
bus - i.e. when pcie bridge is erroneously disabled like in report here:
http://marc.info/?l=linux-wireless&m=132577331132329&w=2

Note that wireless device is one of a few connected through pcie bridge
on most of the laptops, others external pcie devices like mmc are not
used frequently, hence breakage in pci code looks frequently like
breakage in wireless driver. Not sure if that was the case here though.

Stanislaw

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3.3] memcg: free mem_cgroup by RCU to fix oops

[Hugh Dickins]

On Mon, 12 Mar 2012, Stanislaw Gruszka wrote:
> On Fri, Mar 09, 2012 at 11:58:34AM -0800, Hugh Dickins wrote:
> > On Wed, 7 Mar 2012, Hugh Dickins wrote:
> > > 
> > > I'm posting this a little prematurely to get eyes on it, since it's
> > > more than a two-liner, but 3.3 time is running out.  If it is what's
> > > needed to fix my oopses, I won't really be sure before Friday morning.
> > > What's running now on the machine affected is using kfree_rcu(), but I
> > > did hack it earlier to check that the vfree_rcu() alternative works.
> > 
> > Yes, please do send that patch on to Linus for 3.3.
> > 
> > It did not get as much as the 36 hours of testing I had hoped for, only
> > 25 hours so far.  12 hours while I was out yesterday got wasted by a
> > wireless driver interrupt spewing approximately one million messages:
> > 
> > iwl3945 0000:08:00.0: MAC is in deep sleep!. CSR_GP_CNTRL = 0xFFFFFFFF
> 
> I replaced that with WARN_ONCE
> http://marc.info/?l=linux-wireless&m=132912863701997&w=2
> (the patch is currently in net-next).

That's very welcome, thank you.  One message will suit me better than
a million.  But I didn't mention that for every seven of those, there
was one slightly different message coming too:

iwl3945 0000:08:00.0: UNKNOWN (0xFFFFFFFF) 4294967295 ... (I got bored)

> 
> > which I've not suffered from before, and hope not again.  Having kdb
> > in, I did take a look what was going on with the memcg load when it was
> > interrupted: it appeared to be normal, and I've no reason to suppose that
> > my kfree_rcu() was in any way responsible for the wireless aberration.

I didn't see it again.  I rebooted and ran the test for 63 hours and
it went fine this time with no interference from the wifi.  I did have
the laptop differently positioned this time: maybe it was overheating
up against the wall, though that position gave no trouble in the past.

> 
> I don't know if is possible if test patch influence pci or mac80211 code
> (we use rcu quite intensively in mac8021).

It's very very unlikely that the patch I was testing had a significant
effect on RCU usage: the test ends up doing just one extra call_rcu every
minute.  There's a heavy swapping load running alongside, which shouldn't
be disturbing PCI config at all; but it's possible that a temporarily
failing memory allocation, or overheat, drove iwl3945 down a strange
path on this one occasion, and once there it couldn't recover.

Hugh

> Those "MAC is in deep sleep" 
> usually mean that wireless device registers can not be read through pcie
> bus - i.e. when pcie bridge is erroneously disabled like in report here:
> http://marc.info/?l=linux-wireless&m=132577331132329&w=2
> 
> Note that wireless device is one of a few connected through pcie bridge
> on most of the laptops, others external pcie devices like mmc are not
> used frequently, hence breakage in pci code looks frequently like
> breakage in wireless driver. Not sure if that was the case here though.
> 
> Stanislaw

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3.3] memcg: free mem_cgroup by RCU to fix oops

[Johannes Weiner]

On Wed, Mar 07, 2012 at 10:01:50PM -0800, Hugh Dickins wrote:
> After fixing the GPF in mem_cgroup_lru_del_list(), three times one
> machine running a similar load (moving and removing memcgs while swapping)
> has oopsed in mem_cgroup_zone_nr_lru_pages(), when retrieving memcg zone
> numbers for get_scan_count() for shrink_mem_cgroup_zone(): this is where a
> struct mem_cgroup is first accessed after being chosen by mem_cgroup_iter().
> 
> Just what protects a struct mem_cgroup from being freed, in between
> mem_cgroup_iter()'s css_get_next() and its css_tryget()?  css_tryget()
> fails once css->refcnt is zero with CSS_REMOVED set in flags, yes: but
> what if that memory is freed and reused for something else, which sets
> "refcnt" non-zero?  Hmm, and scope for an indefinite freeze if refcnt
> is left at zero but flags are cleared.
> 
> It's tempting to move the css_tryget() into css_get_next(), to make it
> really "get" the css, but I don't think that actually solves anything:
> the same difficulty in moving from css_id found to stable css remains.

I don't think so, either, as long as the css_id can be found after the
synchronize_rcu() between freezing the refcount and freeing the group.

> But we already have rcu_read_lock() around the two, so it's easily
> fixed if __mem_cgroup_free() just uses kfree_rcu() to free mem_cgroup.
> 
> However, a big struct mem_cgroup is allocated with vzalloc() instead
> of kzalloc(), and we're not allowed to vfree() at interrupt time:
> there doesn't appear to be a general vfree_rcu() to help with this,
> so roll our own using schedule_work().  The compiler decently removes
> vfree_work() and vfree_rcu() when the config doesn't need them.
> 
> Signed-off-by: Hugh Dickins <hughd@google.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>