Re: [PATCH 9/9] cgroup: use cgroup->self.refcnt for cgroup refcnting

Re: [PATCH 9/9] cgroup: use cgroup->self.refcnt for cgroup refcnting

[Stephen Warren]

On 05/09/2014 03:13 PM, Tejun Heo wrote:
> Currently cgroup implements refcnting separately using atomic_t
> cgroup->refcnt.  The destruction paths of cgroup and css are rather
> complex and bear a lot of similiarities including the use of RCU and
> bouncing to a work item.
> 
> This patch makes cgroup use the refcnt of self css for refcnting
> instead of using its own.  This makes cgroup refcnting use css's
> percpu refcnt and share the destruction mechanism.
>...

In next-20150515, this patch causes silent boot failures on NVIDIA Tegra
ARM systems. There isn't even any earlyprintk. Reverting this one patch
solves this.

I was a bit surprised that cgroups would cause such an issue, but
looking at a normal kernel boot, I guess cgroup does get initialized
very early:

Uncompressing Linux... done, booting the kernel.
[    0.000000] Booting Linux on physical CPU 0x0
[    0.000000] Initializing cgroup subsys cpu
[    0.000000] Initializing cgroup subsys cpuacct

Anyway, I'll try to track down what the problem is, but if someone
familiar with this patch could give it a look, that'd be very useful,
since I'm not at all familiar with this code or feature.

Re: [PATCH 9/9] cgroup: use cgroup->self.refcnt for cgroup refcnting

[Stephen Warren]

On 05/15/2014 01:07 PM, Stephen Warren wrote:
> On 05/09/2014 03:13 PM, Tejun Heo wrote:
>> Currently cgroup implements refcnting separately using atomic_t
>> cgroup->refcnt.  The destruction paths of cgroup and css are rather
>> complex and bear a lot of similiarities including the use of RCU and
>> bouncing to a work item.
>>
>> This patch makes cgroup use the refcnt of self css for refcnting
>> instead of using its own.  This makes cgroup refcnting use css's
>> percpu refcnt and share the destruction mechanism.
>> ...
> 
> In next-20150515, this patch causes silent boot failures on NVIDIA Tegra
> ARM systems. There isn't even any earlyprintk. Reverting this one patch
> solves this.
> 
> I was a bit surprised that cgroups would cause such an issue, but
> looking at a normal kernel boot, I guess cgroup does get initialized
> very early:
> 
> Uncompressing Linux... done, booting the kernel.
> [    0.000000] Booting Linux on physical CPU 0x0
> [    0.000000] Initializing cgroup subsys cpu
> [    0.000000] Initializing cgroup subsys cpuacct
> 
> Anyway, I'll try to track down what the problem is, but if someone
> familiar with this patch could give it a look, that'd be very useful,
> since I'm not at all familiar with this code or feature.

It looks like this has something to do with the new code using percpu_
functions from within cgroup_init_early, yet that function being called
before init/main.c calls setup_per_cpu_areas() etc.? Unfortunately I
have to run to some meetings now, so won't look at this until tomorrow.

Re: [PATCH 9/9] cgroup: use cgroup->self.refcnt for cgroup refcnting

[Tejun Heo]

Hello, Stephen.

On Thu, May 15, 2014 at 03:56:51PM -0600, Stephen Warren wrote:
> On 05/15/2014 01:07 PM, Stephen Warren wrote:
> > On 05/09/2014 03:13 PM, Tejun Heo wrote:
> >> Currently cgroup implements refcnting separately using atomic_t
> >> cgroup->refcnt.  The destruction paths of cgroup and css are rather
> >> complex and bear a lot of similiarities including the use of RCU and
> >> bouncing to a work item.
> >>
> >> This patch makes cgroup use the refcnt of self css for refcnting
> >> instead of using its own.  This makes cgroup refcnting use css's
> >> percpu refcnt and share the destruction mechanism.
> >> ...
> > 
> > In next-20150515, this patch causes silent boot failures on NVIDIA Tegra
> > ARM systems. There isn't even any earlyprintk. Reverting this one patch
> > solves this.
> > 
> > I was a bit surprised that cgroups would cause such an issue, but
> > looking at a normal kernel boot, I guess cgroup does get initialized
> > very early:
> > 
> > Uncompressing Linux... done, booting the kernel.
> > [    0.000000] Booting Linux on physical CPU 0x0
> > [    0.000000] Initializing cgroup subsys cpu
> > [    0.000000] Initializing cgroup subsys cpuacct
> > 
> > Anyway, I'll try to track down what the problem is, but if someone
> > familiar with this patch could give it a look, that'd be very useful,
> > since I'm not at all familiar with this code or feature.
> 
> It looks like this has something to do with the new code using percpu_
> functions from within cgroup_init_early, yet that function being called
> before init/main.c calls setup_per_cpu_areas() etc.? Unfortunately I
> have to run to some meetings now, so won't look at this until tomorrow.

Yeap, I know where the issue is.  Will post a fix soon.

Thanks.

-- 
tejun

[PATCH cgroup/for-3.16] cgroup: introduce CSS_NO_REF and skip refcnting on normal root csses

[Tejun Heo]

From 49035d695d8f1c4bbe4e37480e5d06812818947c Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Date: Fri, 16 May 2014 11:40:33 -0400

9395a4500404 ("cgroup: enable refcnting for root csses") enabled
reference counting for root csses (cgroup_subsys_states) so that
cgroup's self csses can be used to manage the lifetime of the
containing cgroups.

Unfortunately, this change was incorrect.  cpu controller uses
early_init and starts using css reference counts on its root css from
then on.  percpu_ref can't initialized during early init and its
initialization is deferred till cgroup_init() time.  This means that
cpu was using percpu_ref which wasn't properly initialized.  Due to
the way percpu variables are laid out on x86, this didn't blow up
immediately on x86 but ended up incrementing and decrementing the
percpu variable at offset zero, whatever it may be; however, on other
archs, this caused fault and early boot failure.

As cgroup self csses still need working refcounting, we can't revert
9395a4500404.  This patch adds CSS_NO_REF which explicitly inhibits
reference counting on the css and sets it on all normal (non-self)
csses.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Reported-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Fixes: 9395a4500404 ("cgroup: enable refcnting for root csses")
---
Patch applied to cgroup/for-3.16.

Thanks.

 include/linux/cgroup.h | 11 ++++++++---
 kernel/cgroup.c        |  9 +++++++--
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 76dadd77..1737db0 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -77,6 +77,7 @@ struct cgroup_subsys_state {
 
 /* bits in struct cgroup_subsys_state flags field */
 enum {
+	CSS_NO_REF	= (1 << 0), /* no reference counting for this css */
 	CSS_ONLINE	= (1 << 1), /* between ->css_online() and ->css_offline() */
 };
 
@@ -88,7 +89,8 @@ enum {
  */
 static inline void css_get(struct cgroup_subsys_state *css)
 {
-	percpu_ref_get(&css->refcnt);
+	if (!(css->flags & CSS_NO_REF))
+		percpu_ref_get(&css->refcnt);
 }
 
 /**
@@ -103,7 +105,9 @@ static inline void css_get(struct cgroup_subsys_state *css)
  */
 static inline bool css_tryget_online(struct cgroup_subsys_state *css)
 {
-	return percpu_ref_tryget_live(&css->refcnt);
+	if (!(css->flags & CSS_NO_REF))
+		return percpu_ref_tryget_live(&css->refcnt);
+	return true;
 }
 
 /**
@@ -114,7 +118,8 @@ static inline bool css_tryget_online(struct cgroup_subsys_state *css)
  */
 static inline void css_put(struct cgroup_subsys_state *css)
 {
-	percpu_ref_put(&css->refcnt);
+	if (!(css->flags & CSS_NO_REF))
+		percpu_ref_put(&css->refcnt);
 }
 
 /* bits in struct cgroup flags field */
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index c01e8e8..ad15bb7 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4593,11 +4593,17 @@ static void __init cgroup_init_subsys(struct cgroup_subsys *ss, bool early)
 	/* We don't handle early failures gracefully */
 	BUG_ON(IS_ERR(css));
 	init_and_link_css(css, ss, &cgrp_dfl_root.cgrp);
+
+	/*
+	 * Root csses are never destroyed and we can't initialize
+	 * percpu_ref during early init.  Disable refcnting.
+	 */
+	css->flags |= CSS_NO_REF;
+
 	if (early) {
 		/* allocation can't be done safely during early init */
 		css->id = 1;
 	} else {
-		BUG_ON(percpu_ref_init(&css->refcnt, css_release));
 		css->id = cgroup_idr_alloc(&ss->css_idr, css, 1, 2, GFP_KERNEL);
 		BUG_ON(css->id < 0);
 	}
@@ -4684,7 +4690,6 @@ int __init cgroup_init(void)
 			struct cgroup_subsys_state *css =
 				init_css_set.subsys[ss->id];
 
-			BUG_ON(percpu_ref_init(&css->refcnt, css_release));
 			css->id = cgroup_idr_alloc(&ss->css_idr, css, 1, 2,
 						   GFP_KERNEL);
 			BUG_ON(css->id < 0);
-- 
1.9.0

Re: [PATCH cgroup/for-3.16] cgroup: introduce CSS_NO_REF and skip refcnting on normal root csses

[Stephen Warren]

On 05/16/2014 09:43 AM, Tejun Heo wrote:
> From 49035d695d8f1c4bbe4e37480e5d06812818947c Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Date: Fri, 16 May 2014 11:40:33 -0400
> 
> 9395a4500404 ("cgroup: enable refcnting for root csses") enabled
> reference counting for root csses (cgroup_subsys_states) so that
> cgroup's self csses can be used to manage the lifetime of the
> containing cgroups.
> 
> Unfortunately, this change was incorrect.  cpu controller uses
> early_init and starts using css reference counts on its root css from
> then on.  percpu_ref can't initialized during early init and its
> initialization is deferred till cgroup_init() time.  This means that
> cpu was using percpu_ref which wasn't properly initialized.  Due to
> the way percpu variables are laid out on x86, this didn't blow up
> immediately on x86 but ended up incrementing and decrementing the
> percpu variable at offset zero, whatever it may be; however, on other
> archs, this caused fault and early boot failure.
> 
> As cgroup self csses still need working refcounting, we can't revert
> 9395a4500404.  This patch adds CSS_NO_REF which explicitly inhibits
> reference counting on the css and sets it on all normal (non-self)
> csses.
> 
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Reported-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> Fixes: 9395a4500404 ("cgroup: enable refcnting for root csses")
> ---
> Patch applied to cgroup/for-3.16.

Unfortunately, this patch doesn't seem to solve the problem for me. I
still get a hang/crash very early during boot, before earlyprintk stars.

I tried this patch on top of both next-20150115, and next-20150516. For
both those kernels, reverting the original problematic patch does
resolve the problem.

I'll try to investigate more later today.

[PATCH v2 cgroup/for-3.16] cgroup: introduce CSS_NO_REF and skip refcnting on normal root csses

[Tejun Heo]

9395a4500404 ("cgroup: enable refcnting for root csses") enabled
reference counting for root csses (cgroup_subsys_states) so that
cgroup's self csses can be used to manage the lifetime of the
containing cgroups.

Unfortunately, this change was incorrect.  During early init,
cgrp_dfl_root self css refcnt is used.  percpu_ref can't initialized
during early init and its initialization is deferred till
cgroup_init() time.  This means that cpu was using percpu_ref which
wasn't properly initialized.  Due to the way percpu variables are laid
out on x86, this didn't blow up immediately on x86 but ended up
incrementing and decrementing the percpu variable at offset zero,
whatever it may be; however, on other archs, this caused fault and
early boot failure.

As cgroup self csses for root cgroups of non-dfl hierarchies need
working refcounting, we can't revert 9395a4500404.  This patch adds
CSS_NO_REF which explicitly inhibits reference counting on the css and
sets it on all normal (non-self) csses and cgroup_dfl_root self css.

v2: cgrp_dfl_root.self is the offending one.  Set the flag on it.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Reported-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Fixes: 9395a4500404 ("cgroup: enable refcnting for root csses")
---
Hello,

Can you try this one instead?

Thanks.

 include/linux/cgroup.h |   11 ++++++++---
 kernel/cgroup.c        |   11 +++++++++--
 2 files changed, 17 insertions(+), 5 deletions(-)

--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -77,6 +77,7 @@ struct cgroup_subsys_state {
 
 /* bits in struct cgroup_subsys_state flags field */
 enum {
+	CSS_NO_REF	= (1 << 0), /* no reference counting for this css */
 	CSS_ONLINE	= (1 << 1), /* between ->css_online() and ->css_offline() */
 };
 
@@ -88,7 +89,8 @@ enum {
  */
 static inline void css_get(struct cgroup_subsys_state *css)
 {
-	percpu_ref_get(&css->refcnt);
+	if (!(css->flags & CSS_NO_REF))
+		percpu_ref_get(&css->refcnt);
 }
 
 /**
@@ -103,7 +105,9 @@ static inline void css_get(struct cgroup
  */
 static inline bool css_tryget_online(struct cgroup_subsys_state *css)
 {
-	return percpu_ref_tryget_live(&css->refcnt);
+	if (!(css->flags & CSS_NO_REF))
+		return percpu_ref_tryget_live(&css->refcnt);
+	return true;
 }
 
 /**
@@ -114,7 +118,8 @@ static inline bool css_tryget_online(str
  */
 static inline void css_put(struct cgroup_subsys_state *css)
 {
-	percpu_ref_put(&css->refcnt);
+	if (!(css->flags & CSS_NO_REF))
+		percpu_ref_put(&css->refcnt);
 }
 
 /* bits in struct cgroup flags field */
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4593,11 +4593,17 @@ static void __init cgroup_init_subsys(st
 	/* We don't handle early failures gracefully */
 	BUG_ON(IS_ERR(css));
 	init_and_link_css(css, ss, &cgrp_dfl_root.cgrp);
+
+	/*
+	 * Root csses are never destroyed and we can't initialize
+	 * percpu_ref during early init.  Disable refcnting.
+	 */
+	css->flags |= CSS_NO_REF;
+
 	if (early) {
 		/* allocation can't be done safely during early init */
 		css->id = 1;
 	} else {
-		BUG_ON(percpu_ref_init(&css->refcnt, css_release));
 		css->id = cgroup_idr_alloc(&ss->css_idr, css, 1, 2, GFP_KERNEL);
 		BUG_ON(css->id < 0);
 	}
@@ -4636,6 +4642,8 @@ int __init cgroup_init_early(void)
 	int i;
 
 	init_cgroup_root(&cgrp_dfl_root, &opts);
+	cgrp_dfl_root.cgrp.self.flags |= CSS_NO_REF;
+
 	RCU_INIT_POINTER(init_task.cgroups, &init_css_set);
 
 	for_each_subsys(ss, i) {
@@ -4684,7 +4692,6 @@ int __init cgroup_init(void)
 			struct cgroup_subsys_state *css =
 				init_css_set.subsys[ss->id];
 
-			BUG_ON(percpu_ref_init(&css->refcnt, css_release));
 			css->id = cgroup_idr_alloc(&ss->css_idr, css, 1, 2,
 						   GFP_KERNEL);
 			BUG_ON(css->id < 0);

Re: [PATCH v2 cgroup/for-3.16] cgroup: introduce CSS_NO_REF and skip refcnting on normal root csses

[Stephen Warren]

On 05/16/2014 11:09 AM, Tejun Heo wrote:
> 9395a4500404 ("cgroup: enable refcnting for root csses") enabled
> reference counting for root csses (cgroup_subsys_states) so that
> cgroup's self csses can be used to manage the lifetime of the
> containing cgroups.
> 
> Unfortunately, this change was incorrect.  During early init,
> cgrp_dfl_root self css refcnt is used.  percpu_ref can't initialized
> during early init and its initialization is deferred till
> cgroup_init() time.  This means that cpu was using percpu_ref which
> wasn't properly initialized.  Due to the way percpu variables are laid
> out on x86, this didn't blow up immediately on x86 but ended up
> incrementing and decrementing the percpu variable at offset zero,
> whatever it may be; however, on other archs, this caused fault and
> early boot failure.
> 
> As cgroup self csses for root cgroups of non-dfl hierarchies need
> working refcounting, we can't revert 9395a4500404.  This patch adds
> CSS_NO_REF which explicitly inhibits reference counting on the css and
> sets it on all normal (non-self) csses and cgroup_dfl_root self css.
> 
> v2: cgrp_dfl_root.self is the offending one.  Set the flag on it.
> 
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Reported-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> Fixes: 9395a4500404 ("cgroup: enable refcnting for root csses")
> ---
> Hello,
> 
> Can you try this one instead?

Yes, this one works great, thanks.
Tested-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Re: [PATCH v2 cgroup/for-3.16] cgroup: introduce CSS_NO_REF and skip refcnting on normal root csses

[Tejun Heo]

On Fri, May 16, 2014 at 11:19:24AM -0600, Stephen Warren wrote:
> On 05/16/2014 11:09 AM, Tejun Heo wrote:
> > Can you try this one instead?
> 
> Yes, this one works great, thanks.
> Tested-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Thanks, I reverted the original one and applied the updated one w/
your Tested-by tag added.

-- 
tejun