OpenVZ Forum - RDF feed
type: channel
items: [PATCH] cgroup: Remove RCU from task-&gt;cgroups
items: Re: [PATCH] cgroup: Remove RCU from task-&gt;cgroups
items: [PATCH] cgroup: Convert synchronize_rcu to call_rcu in cgroup_attach_task
items: Re: [PATCH] cgroup: Convert synchronize_rcu to call_rcu in cgroup_attach_task
items: [PATCH 1/2] cgroup: Set CGRP_RELEASABLE when adding to a cgroup
items: [PATCH 2/2] cgroup: Remove call to synchronize_rcu in cgroup_attach_task
items: Re: [PATCH 1/2] cgroup: Set CGRP_RELEASABLE when adding to a cgroup
items: Re: [PATCH 2/2] cgroup: Remove call to synchronize_rcu in cgroup_attach_task

[PATCH] cgroup: Remove RCU from task-&gt;cgroups
type: item
description: The synchronize_rcu call in cgroup_attach_task can be very expensive. All fastpath accesses to task->cgroups already use task_lock() or cgroup_lock() to protect against updates, and only the CGROUP_DEBUG files have RCU read-side critical sections. This patch replaces rcu_read_lock() with task_lock(current) around the debug file acceses to current->cgroups and removes the synchronize_rcu call in cgroup_attach_task. Signed-off-by: Colin Cross <ccross@android.com> --- kernel/cgroup.c | 22 ++++++++-------------- 1 files changed, 8 insertions(+), 14 deletions(-) diff --git a/kernel/cgroup.c b/kernel/cgroup.c index 66a416b..4a40183 100644 --- a/kernel/cgroup.c +++ b/kernel/cgroup.c @@ -725,14 +725,11 @@ static struct cgroup *task_cgroup_from_root(struct task_struct *task, * cgroup_attach_task(), which overwrites one tasks cgroup pointer with * another. It does so using cgroup_mutex, however there are * several performance critical places that need to reference - * task->cgroup without the expense of grabbing a system global + * task->cgroups without the expense of grabbing a system global * mutex. Therefore except as noted below, when dereferencing or, as - * in cgroup_attach_task(), modifying a task'ss cgroup pointer we use + * in cgroup_attach_task(), modifying a task's cgroups pointer we use * task_lock(), which acts on a spinlock (task->alloc_lock) already in * the task_struct routinely used for such matters. - * - * P.S. One more locking exception. RCU is used to guard the - * update of a tasks cgroup pointer by cgroup_attach_task() */ /** @@ -1786,7 +1783,7 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk) retval = -ESRCH; goto out; } - rcu_assign_pointer(tsk->cgroups, newcg); + tsk->cgroups = newcg; task_unlock(tsk); /* Update the css_set linked lists if we're using them */ @@ -1802,7 +1799,6 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk) ss->attach(ss, cgrp, oldcgrp, tsk, false); } set_bit(CGRP_RELEASABLE, &oldcgrp->f…
creator: Colin Cross
date: 2010-11-21T02:00:24-00:00

Re: [PATCH] cgroup: Remove RCU from task-&gt;cgroups
type: item
description: On Sat, Nov 20, 2010 at 6:00 PM, Colin Cross <ccross@android.com> wrote: > The synchronize_rcu call in cgroup_attach_task can be very > expensive. All fastpath accesses to task->cgroups already > use task_lock() or cgroup_lock() to protect against updates, > and only the CGROUP_DEBUG files have RCU read-side critical > sections. > > This patch replaces rcu_read_lock() with task_lock(current) > around the debug file acceses to current->cgroups and removes > the synchronize_rcu call in cgroup_attach_task. > > Signed-off-by: Colin Cross <ccross@android.com> > --- > kernel/cgroup.c | 22 ++++++++-------------- > 1 files changed, 8 insertions(+), 14 deletions(-) > This patch isn't correct, there's an rcu_dereference I missed inside task_group(), and that's the important one. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containe rs
creator: Colin Cross
date: 2010-11-21T23:02:28-00:00

[PATCH] cgroup: Convert synchronize_rcu to call_rcu in cgroup_attach_task
type: item
description: The synchronize_rcu call in cgroup_attach_task can be very expensive. All fastpath accesses to task->cgroups that expect task->cgroups not to change already use task_lock() or cgroup_lock() to protect against updates, and, in cgroup.c, only the CGROUP_DEBUG files have RCU read-side critical sections. sched.c uses RCU read-side-critical sections on task->cgroups, but only to ensure that a dereference of task->cgroups does not become invalid, not that it doesn't change. This patch adds a function put_css_set_rcu, which delays the put until after a grace period has elapsed. This ensures that any RCU read-side critical sections that dereferenced task->cgroups in sched.c have completed before the css_set is deleted. The synchronize_rcu()/put_css_set() combo in cgroup_attach_task() can then be replaced with put_css_set_rcu(). Also converts the CGROUP_DEBUG files that access current->cgroups to use task_lock(current) instead of rcu_read_lock(). Signed-off-by: Colin Cross <ccross@android.com> --- This version fixes the problems with the previous patch by keeping the use of RCU in cgroup_attach_task, but allowing cgroup_attach_task to return immediately by deferring the final put_css_reg to an rcu callback. include/linux/cgroup.h | 4 +++ kernel/cgroup.c | 58 ++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 50 insertions(+), 12 deletions(-) diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h index ed4ba11..fd26218 100644 --- a/include/linux/cgroup.h +++ b/include/linux/cgroup.h @@ -287,6 +287,10 @@ struct css_set { /* For RCU-protected deletion */ struct rcu_head rcu_head; + + /* For RCU-delayed puts */ + atomic_t delayed_put_count; + struct work_struct delayed_put_work; }; /* diff --git a/kernel/cgroup.c b/kernel/cgroup.c index 66a416b..c7348e7 100644 --- a/kernel/cgroup.c +++ b/kernel/cgroup.c @@ -298,7 +298,8 @@ static int cgroup_init_idr(struct cgroup_subsys *ss, /* css_set_lock protects the list of css_set objects, and the * chain of tasks off e…
creator: Colin Cross
date: 2010-11-22T04:06:07-00:00

Re: [PATCH] cgroup: Convert synchronize_rcu to call_rcu in cgroup_attach_task
type: item
description: On Tue, Nov 23, 2010 at 12:14 AM, Li Zefan <lizf@cn.fujitsu.com> wrote: > 12:06, Colin Cross wrote: >> The synchronize_rcu call in cgroup_attach_task can be very >> expensive. All fastpath accesses to task->cgroups that expect >> task->cgroups not to change already use task_lock() or >> cgroup_lock() to protect against updates, and, in cgroup.c, >> only the CGROUP_DEBUG files have RCU read-side critical >> sections. >> >> sched.c uses RCU read-side-critical sections on task->cgroups, >> but only to ensure that a dereference of task->cgroups does >> not become invalid, not that it doesn't change. >> > > Other cgroup subsystems also use rcu_read_lock to access task->cgroups, > for example net_cls cgroup and device cgroup. I believe the same comment applies as sched.c, I'll update the commit message. > I don't think the performance of task attaching is so critically > important that we have to use call_rcu() instead of synchronize_rcu()? On my desktop, moving a task between cgroups averages 100 ms, and on an Tegra2 SMP ARM platform it takes 20 ms. Moving a task with many threads can take hundreds of milliseconds or more. With this patch it takes 50 microseconds to move one task, a 400x improvement. >> This patch adds a function put_css_set_rcu, which delays the >> put until after a grace period has elapsed. This ensures that >> any RCU read-side critical sections that dereferenced >> task->cgroups in sched.c have completed before the css_set is >> deleted. The synchronize_rcu()/put_css_set() combo in >> cgroup_attach_task() can then be replaced with >> put_css_set_rcu(). >> > >> Also converts the CGROUP_DEBUG files that access >> current->cgroups to use task_lock(current) instead of >> rcu_read_lock(). >> > > What for? What do we gain from doing this for those debug > interfaces? Left over from the previous patch that incorrectly dropped RCU completely. I'll put the rcu_read_locks back. >> Signed-off-by: Colin Cross <ccross@android.com> >> >> --- >> >> This version fix…
creator: Colin Cross
date: 2010-11-23T08:58:39-00:00

Re: [PATCH] cgroup: Convert synchronize_rcu to call_rcu in cgroup_attach_task
type: item
description: On Tue, Nov 23, 2010 at 12:58 AM, Colin Cross <ccross@android.com> wrote: > On Tue, Nov 23, 2010 at 12:14 AM, Li Zefan <lizf@cn.fujitsu.com> wrote: >> 12:06, Colin Cross wrote: >>> The synchronize_rcu call in cgroup_attach_task can be very >>> expensive. All fastpath accesses to task->cgroups that expect >>> task->cgroups not to change already use task_lock() or >>> cgroup_lock() to protect against updates, and, in cgroup.c, >>> only the CGROUP_DEBUG files have RCU read-side critical >>> sections. >>> >>> sched.c uses RCU read-side-critical sections on task->cgroups, >>> but only to ensure that a dereference of task->cgroups does >>> not become invalid, not that it doesn't change. >>> >> >> Other cgroup subsystems also use rcu_read_lock to access task->cgroups, >> for example net_cls cgroup and device cgroup. > I believe the same comment applies as sched.c, I'll update the commit message. > >> I don't think the performance of task attaching is so critically >> important that we have to use call_rcu() instead of synchronize_rcu()? > On my desktop, moving a task between cgroups averages 100 ms, and on > an Tegra2 SMP ARM platform it takes 20 ms. Moving a task with many > threads can take hundreds of milliseconds or more. With this patch it > takes 50 microseconds to move one task, a 400x improvement. > >>> This patch adds a function put_css_set_rcu, which delays the >>> put until after a grace period has elapsed. This ensures that >>> any RCU read-side critical sections that dereferenced >>> task->cgroups in sched.c have completed before the css_set is >>> deleted. The synchronize_rcu()/put_css_set() combo in >>> cgroup_attach_task() can then be replaced with >>> put_css_set_rcu(). >>> >> >>> Also converts the CGROUP_DEBUG files that access >>> current->cgroups to use task_lock(current) instead of >>> rcu_read_lock(). >>> >> >> What for? What do we gain from doing this for those debug >> interfaces? > Left over from the previous patch that incorrectly dropped RCU > co…
creator: Colin Cross
date: 2010-11-23T20:22:45-00:00

Re: [PATCH] cgroup: Convert synchronize_rcu to call_rcu in cgroup_attach_task
type: item
description: On Tue, Nov 23, 2010 at 6:06 PM, Li Zefan <lizf@cn.fujitsu.com> wrote: > Paul Menage wrote: >> On Sun, Nov 21, 2010 at 8:06 PM, Colin Cross <ccross@android.com> wrote: >>> The synchronize_rcu call in cgroup_attach_task can be very >>> expensive. All fastpath accesses to task->cgroups that expect >>> task->cgroups not to change already use task_lock() or >>> cgroup_lock() to protect against updates, and, in cgroup.c, >>> only the CGROUP_DEBUG files have RCU read-side critical >>> sections. >> >> I definitely agree with the goal of using lighter-weight >> synchronization than the current synchronize_rcu() call. However, >> there are definitely some subtleties to worry about in this code. >> >> One of the reasons originally for the current synchronization was to >> avoid the case of calling subsystem destroy() callbacks while there >> could still be threads with RCU references to the subsystem state. The >> fact that synchronize_rcu() was called within a cgroup_mutex critical >> section meant that an rmdir (or any other significant cgrooup >> management action) couldn't possibly start until any RCU read sections >> were done. >> >> I suspect that when we moved a lot of the cgroup teardown code from >> cgroup_rmdir() to cgroup_diput() (which also has a synchronize_rcu() >> call in it) this restriction could have been eased, but I think I left >> it as it was mostly out of paranoia that I was missing/forgetting some >> crucial reason for keeping it in place. >> >> I'd suggest trying the following approach, which I suspect is similar >> to what you were suggesting in your last email >> >> 1) make find_existing_css_set ignore css_set objects with a zero refcount >> 2) change __put_css_set to be simply >> >> if (atomic_dec_and_test(&cg->refcount)) { >> call_rcu(&cg->rcu_head, free_css_set_rcu); >> } > > If we do this, it's not anymore safe to use get_css_set(), which just > increments the refcount without checking if it's zero. I used an alternate approach, removing the css…
creator: Colin Cross
date: 2010-11-24T02:10:58-00:00

[PATCH 1/2] cgroup: Set CGRP_RELEASABLE when adding to a cgroup
type: item
description: Changes the meaning of CGRP_RELEASABLE to be set on any cgroup that has ever had a task or cgroup in it, or had css_get called on it. The bit is set in cgroup_attach_task, cgroup_create, and __css_get. It is not necessary to set the bit in cgroup_fork, as the task is either in the root cgroup, in which can never be released, or the task it was forked from already set the bit in croup_attach_task. Signed-off-by: Colin Cross <ccross@android.com> --- include/linux/cgroup.h | 12 +-------- kernel/cgroup.c | 54 ++++++++++++++++++++--------------------------- 2 files changed, 25 insertions(+), 41 deletions(-) diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h index ed4ba11..9e13078 100644 --- a/include/linux/cgroup.h +++ b/include/linux/cgroup.h @@ -84,12 +84,6 @@ enum { CSS_REMOVED, /* This CSS is dead */ }; -/* Caller must verify that the css is not for root cgroup */ -static inline void __css_get(struct cgroup_subsys_state *css, int count) -{ - atomic_add(count, &css->refcnt); -} - /* * Call css_get() to hold a reference on the css; it can be used * for a reference obtained via: @@ -97,6 +91,7 @@ static inline void __css_get(struct cgroup_subsys_state *css, int count) * - task->cgroups for a locked task */ +extern void __css_get(struct cgroup_subsys_state *css, int count); static inline void css_get(struct cgroup_subsys_state *css) { /* We don't need to reference count the root state */ @@ -143,10 +138,7 @@ static inline void css_put(struct cgroup_subsys_state *css) enum { /* Control Group is dead */ CGRP_REMOVED, - /* - * Control Group has previously had a child cgroup or a task, - * but no longer (only if CGRP_NOTIFY_ON_RELEASE is set) - */ + /* Control Group has ever had a child cgroup or a task */ CGRP_RELEASABLE, /* Control Group requires release notifications to userspace */ CGRP_NOTIFY_ON_RELEASE, diff --git a/kernel/cgroup.c b/kernel/cgroup.c index 66a416b..34e855e 100644 --- a/kernel/cgroup.c +++ b/kernel/cgroup.c @@ -338,7 +338,15 @@ static void free…
creator: Colin Cross
date: 2010-11-24T05:37:03-00:00

[PATCH 2/2] cgroup: Remove call to synchronize_rcu in cgroup_attach_task
type: item
description: synchronize_rcu can be very expensive, averaging 100 ms in some cases. In cgroup_attach_task, it is used to prevent a task->cgroups pointer dereferenced in an RCU read side critical section from being invalidated, by delaying the call to put_css_set until after an RCU grace period. To avoid the call to synchronize_rcu, make the put_css_set call rcu-safe by moving the deletion of the css_set links into free_css_set_work, scheduled by the rcu callback free_css_set_rcu. The decrement of the cgroup refcount is no longer synchronous with the call to put_css_set, which can result in the cgroup refcount staying positive after the last call to cgroup_attach_task returns. To allow the cgroup to be deleted with cgroup_rmdir synchronously after cgroup_attach_task, have rmdir check the refcount of all associated css_sets. If cgroup_rmdir is called on a cgroup for which the css_sets all have refcount zero but the cgroup refcount is nonzero, reuse the rmdir waitqueue to block the rmdir until free_css_set_work is called. Signed-off-by: Colin Cross <ccross@android.com> --- include/linux/cgroup.h | 1 + kernel/cgroup.c | 120 +++++++++++++++++++++++++++++------------------- 2 files changed, 74 insertions(+), 47 deletions(-) diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h index 9e13078..49fdff0 100644 --- a/include/linux/cgroup.h +++ b/include/linux/cgroup.h @@ -279,6 +279,7 @@ struct css_set { /* For RCU-protected deletion */ struct rcu_head rcu_head; + struct work_struct work; }; /* diff --git a/kernel/cgroup.c b/kernel/cgroup.c index 34e855e..e752c83 100644 --- a/kernel/cgroup.c +++ b/kernel/cgroup.c @@ -267,6 +267,33 @@ static void cgroup_release_agent(struct work_struct *work); static DECLARE_WORK(release_agent_work, cgroup_release_agent); static void check_for_release(struct cgroup *cgrp); +/* + * A queue for waiters to do rmdir() cgroup. A tasks will sleep when + * cgroup->count == 0 && list_empty(&cgroup->children) && subsys has some + * reference to css->refcnt. I…
creator: Colin Cross
date: 2010-11-24T05:37:04-00:00

Re: [PATCH 1/2] cgroup: Set CGRP_RELEASABLE when adding to a cgroup
type: item
description: On Wed, Nov 24, 2010 at 3:54 PM, Paul Menage <menage@google.com> wrote: > On Tue, Nov 23, 2010 at 9:37 PM, Colin Cross <ccross@android.com> wrote: >> @@ -364,12 +372,8 @@ static void __put_css_set(struct css_set *cg, int taskexit) >> struct cgroup *cgrp = link->cgrp; >> list_del(&link->cg_link_list); >> list_del(&link->cgrp_link_list); >> - if (atomic_dec_and_test(&cgrp->count) && >> - notify_on_release(cgrp)) { >> - if (taskexit) >> - set_bit(CGRP_RELEASABLE, &cgrp->flags); >> + if (atomic_dec_and_test(&cgrp->count)) >> check_for_release(cgrp); >> - } > > We seem to have lost some notify_on_release() checks - maybe move that > to check_for_release()? check_for_release immediately calls cgroup_is_releasable, which checks for the same bit as notify_on_release. There's no need for CGRP_RELEASABLE to depend on notify_on_release, or to check notify_on_release before calling check_for_release. >> /* Caller must verify that the css is not for root cgroup */ >> +void __css_get(struct cgroup_subsys_state *css, int count) >> +{ >> + atomic_add(count, &css->refcnt); >> + set_bit(CGRP_RELEASABLE, &css->cgroup->flags); >> +} > > Is css_get() the right place to be putting this? It's not clear to me > why a subsystem taking a refcount on a cgroup's state should render it > releasable when it drops that refcount. I matched the existing behavior, __css_put sets CGRP_RELEASABLE when refcnt goes to 0. > Should we maybe clear the CGRP_RELEASABLE flag right before doing the > userspace callback? Actually, I think CGRP_RELEASABLE can be dropped entirely. check_for_release is only called from __css_put, cgroup_rmdir, and __put_css_set (or free_css_set_work after my second patch). Those all imply that __css_get, get_css_set, or cgroup_create have been previously called, which are the functions that set CGRP_RELEASABLE. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containe r…
creator: Colin Cross
date: 2010-11-25T00:11:34-00:00

Re: [PATCH 1/2] cgroup: Set CGRP_RELEASABLE when adding to a cgroup
type: item
description: On Wed, Nov 24, 2010 at 4:11 PM, Colin Cross <ccross@android.com> wrote: > On Wed, Nov 24, 2010 at 3:54 PM, Paul Menage <menage@google.com> wrote: >> On Tue, Nov 23, 2010 at 9:37 PM, Colin Cross <ccross@android.com> wrote: >>> @@ -364,12 +372,8 @@ static void __put_css_set(struct css_set *cg, int taskexit) >>> struct cgroup *cgrp = link->cgrp; >>> list_del(&link->cg_link_list); >>> list_del(&link->cgrp_link_list); >>> - if (atomic_dec_and_test(&cgrp->count) && >>> - notify_on_release(cgrp)) { >>> - if (taskexit) >>> - set_bit(CGRP_RELEASABLE, &cgrp->flags); >>> + if (atomic_dec_and_test(&cgrp->count)) >>> check_for_release(cgrp); >>> - } >> >> We seem to have lost some notify_on_release() checks - maybe move that >> to check_for_release()? > check_for_release immediately calls cgroup_is_releasable, which checks > for the same bit as notify_on_release. There's no need for > CGRP_RELEASABLE to depend on notify_on_release, or to check > notify_on_release before calling check_for_release. > >>> /* Caller must verify that the css is not for root cgroup */ >>> +void __css_get(struct cgroup_subsys_state *css, int count) >>> +{ >>> + atomic_add(count, &css->refcnt); >>> + set_bit(CGRP_RELEASABLE, &css->cgroup->flags); >>> +} >> >> Is css_get() the right place to be putting this? It's not clear to me >> why a subsystem taking a refcount on a cgroup's state should render it >> releasable when it drops that refcount. > I matched the existing behavior, __css_put sets CGRP_RELEASABLE when > refcnt goes to 0. > >> Should we maybe clear the CGRP_RELEASABLE flag right before doing the >> userspace callback? > Actually, I think CGRP_RELEASABLE can be dropped entirely. > check_for_release is only called from __css_put, cgroup_rmdir, and > __put_css_set (or free_css_set_work after my second patch). Those all > imply that __css_get, get_css_set, or cgroup_create have been > previously called, which are the functions that set CGRP_RELEASABLE. Nevermind, that's not true - get_css_set doe…
creator: Colin Cross
date: 2010-11-25T00:18:59-00:00

Re: [PATCH 1/2] cgroup: Set CGRP_RELEASABLE when adding to a cgroup
type: item
description: On Wed, Nov 24, 2010 at 4:21 PM, Paul Menage <menage@google.com> wrote: > On Wed, Nov 24, 2010 at 4:11 PM, Colin Cross <ccross@android.com> wrote: >>> >>> We seem to have lost some notify_on_release() checks - maybe move that >>> to check_for_release()? >> check_for_release immediately calls cgroup_is_releasable, which checks >> for the same bit as notify_on_release. There's no need for >> CGRP_RELEASABLE to depend on notify_on_release, or to check >> notify_on_release before calling check_for_release. > > OK. > >> I matched the existing behavior, __css_put sets CGRP_RELEASABLE when >> refcnt goes to 0. >> > > Ah, we do appear to have had that behaviour for a while. I don't > remember the justification for it at this point :-) > >> check_for_release is only called from __css_put, cgroup_rmdir, and >> __put_css_set (or free_css_set_work after my second patch). Those all >> imply that __css_get, get_css_set, or cgroup_create have been >> previously called, which are the functions that set CGRP_RELEASABLE. > > Not in one case - if we create a new cgroup and try to move a thread > into it, but the thread is exiting as we move it, we'll call > put_css_set() on the new css_set, which will drop the refcount on the > target cgroup back to 0. We wouldn't want the auto-release > notification to kick in in that situation, I think. Clearing the CGRP_RELEASABLE bit any time after the tests in check_for_release introduces a race if __css_get is called between the check and clearing the bit - the cgroup will have an entry, but the bit will not be set. Without additional locking in __css_get, I don't see any way to safely clear CGRP_RELEASABLE. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containe rs
creator: Colin Cross
date: 2010-12-03T03:07:12-00:00

Re: [PATCH 1/2] cgroup: Set CGRP_RELEASABLE when adding to a cgroup
type: item
description: On Thu, Dec 16, 2010 at 4:54 PM, Paul Menage <menage@google.com> wrote: > On Thu, Dec 2, 2010 at 7:07 PM, Colin Cross <ccross@android.com> wrote: >>> Not in one case - if we create a new cgroup and try to move a thread >>> into it, but the thread is exiting as we move it, we'll call >>> put_css_set() on the new css_set, which will drop the refcount on the >>> target cgroup back to 0. We wouldn't want the auto-release >>> notification to kick in in that situation, I think. >> >> Clearing the CGRP_RELEASABLE bit any time after the tests in >> check_for_release introduces a race if __css_get is called between the >> check and clearing the bit - the cgroup will have an entry, but the >> bit will not be set. Without additional locking in __css_get, I don't >> see any way to safely clear CGRP_RELEASABLE. > > I don't quite follow your argument here. Are you saying that the > problem is that you could end up spawning a release agent for a cgroup > that was no longer releasable since it now had a process in it again? > If so, then I don't think that's a problem - spurious release agent > invocations for non-empty cgroups will always happen occasionally due > to races between the kernel and userspace. But a failed move of a task > into a previously-empty cgroup shouldn't trigger the agent. No, if you add a new process to the group while check_for_release, the bit could get set by the add for the new process, then cleared by the concurrently running check_for_release. The release agent would be spawned with a process in the group, which is fine, but when RELEASABLE bit would be clear. When the new process was removed, check_for_release would not call the release agent at all. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containe rs
creator: Colin Cross
date: 2010-12-17T01:12:42-00:00

Re: [PATCH 2/2] cgroup: Remove call to synchronize_rcu in cgroup_attach_task
type: item
description: On 11/23/2010 09:37 PM, Colin Cross wrote: > synchronize_rcu can be very expensive, averaging 100 ms in > some cases. In cgroup_attach_task, it is used to prevent > a task->cgroups pointer dereferenced in an RCU read side > critical section from being invalidated, by delaying the > call to put_css_set until after an RCU grace period. > > To avoid the call to synchronize_rcu, make the put_css_set > call rcu-safe by moving the deletion of the css_set links > into free_css_set_work, scheduled by the rcu callback > free_css_set_rcu. > > The decrement of the cgroup refcount is no longer > synchronous with the call to put_css_set, which can result > in the cgroup refcount staying positive after the last call > to cgroup_attach_task returns. To allow the cgroup to be > deleted with cgroup_rmdir synchronously after > cgroup_attach_task, have rmdir check the refcount of all > associated css_sets. If cgroup_rmdir is called on a cgroup > for which the css_sets all have refcount zero but the > cgroup refcount is nonzero, reuse the rmdir waitqueue to > block the rmdir until free_css_set_work is called. > > Signed-off-by: Colin Cross <ccross@android.com> > --- > include/linux/cgroup.h | 1 + > kernel/cgroup.c | 120 +++++++++++++++++++++++++++++------------------- > 2 files changed, 74 insertions(+), 47 deletions(-) > > diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h > index 9e13078..49fdff0 100644 > --- a/include/linux/cgroup.h > +++ b/include/linux/cgroup.h > @@ -279,6 +279,7 @@ struct css_set { > > /* For RCU-protected deletion */ > struct rcu_head rcu_head; > + struct work_struct work; > }; > > /* > diff --git a/kernel/cgroup.c b/kernel/cgroup.c > index 34e855e..e752c83 100644 > --- a/kernel/cgroup.c > +++ b/kernel/cgroup.c > @@ -267,6 +267,33 @@ static void cgroup_release_agent(struct work_struct *work); > static DECLARE_WORK(release_agent_work, cgroup_release_agent); > static void check_for_release(struct cgroup *cgrp); > > +/* > + * A queue for waiters to do rmd…
creator: Bryan Huntsman
date: 2011-01-28T01:17:26-00:00

Re: [PATCH 1/2] cgroup: Set CGRP_RELEASABLE when adding to a cgroup
type: item
description: On 11/23/2010 09:37 PM, Colin Cross wrote: > Changes the meaning of CGRP_RELEASABLE to be set on any cgroup > that has ever had a task or cgroup in it, or had css_get called > on it. The bit is set in cgroup_attach_task, cgroup_create, > and __css_get. It is not necessary to set the bit in > cgroup_fork, as the task is either in the root cgroup, in > which can never be released, or the task it was forked from > already set the bit in croup_attach_task. > > Signed-off-by: Colin Cross <ccross@android.com> > --- > include/linux/cgroup.h | 12 +-------- > kernel/cgroup.c | 54 ++++++++++++++++++++--------------------------- > 2 files changed, 25 insertions(+), 41 deletions(-) > > diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h > index ed4ba11..9e13078 100644 > --- a/include/linux/cgroup.h > +++ b/include/linux/cgroup.h > @@ -84,12 +84,6 @@ enum { > CSS_REMOVED, /* This CSS is dead */ > }; > > -/* Caller must verify that the css is not for root cgroup */ > -static inline void __css_get(struct cgroup_subsys_state *css, int count) > -{ > - atomic_add(count, &css->refcnt); > -} > - > /* > * Call css_get() to hold a reference on the css; it can be used > * for a reference obtained via: > @@ -97,6 +91,7 @@ static inline void __css_get(struct cgroup_subsys_state *css, int count) > * - task->cgroups for a locked task > */ > > +extern void __css_get(struct cgroup_subsys_state *css, int count); > static inline void css_get(struct cgroup_subsys_state *css) > { > /* We don't need to reference count the root state */ > @@ -143,10 +138,7 @@ static inline void css_put(struct cgroup_subsys_state *css) > enum { > /* Control Group is dead */ > CGRP_REMOVED, > - /* > - * Control Group has previously had a child cgroup or a task, > - * but no longer (only if CGRP_NOTIFY_ON_RELEASE is set) > - */ > + /* Control Group has ever had a child cgroup or a task */ > CGRP_RELEASABLE, > /* Control Group requires release notifications to userspace */ > CGRP_NOTIFY_ON_RELEASE, > diff --git …
creator: Bryan Huntsman
date: 2011-01-28T01:17:32-00:00

Re: [PATCH 1/2] cgroup: Set CGRP_RELEASABLE when adding to a cgroup
type: item
description: On 1/27/2011 5:30 PM, Paul Menage wrote: > On Thu, Jan 27, 2011 at 5:17 PM, Bryan Huntsman<bryanh@codeaurora.org> wrote: >> >> Tested-by: Mike Bohan<mbohan@codeaurora.org> >> >> I'm responding on Mike's behalf and adding him to this thread. This >> patch improves launch time of a test app from ~700ms to ~250ms on MSM, >> with much lower variance across tests. We also see UI latency >> improvements, but have not quantified the gains. >> > > Is this attached to the wrong patch? I'd thought that it was the other > patch (removing the rcu_synchronize()) that's the performance booster. > This one is more about preserving the semantics of the notification > API. You are correct. "[PATCH 2/2] cgroup: Remove call to synchronize_rcu in cgroup_attach_task" improved the performance. To be more correct, I tested this patch (eg. "cgroup: Set CGRP_RELEASABLE when adding to a cgroup") to the degree that it didn't appear to cause any stability or functional regressions when performing the simple benchmark procedure described above. I did also test "[PATCH 2/2] cgroup: Remove call to synchronize_rcu in cgroup_attach_task" independently of this patch to verify that it alone improved the performance. Thanks, Mike -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containe rs
creator: Michael Bohan
date: 2011-01-28T01:48:01-00:00