[PATCH v3 1/2] cgroups: read-write lock CLONE_THREAD forking per threadgroup
in [Kernel]
|
Prev: [PATCH] ASoC: DaVinci: Added support for cpu clocking I2S
Next: wistron_btns: fix a memory leak in wb_module_init error path
From: KAMEZAWA Hiroyuki on 28 Jun 2010 03:20 On Fri, 25 Jun 2010 01:46:54 -0400 Ben Blum <bblum(a)andrew.cmu.edu> wrote: > Adds functionality to read/write lock CLONE_THREAD fork()ing per-threadgroup > > From: Ben Blum <bblum(a)andrew.cmu.edu> > > This patch adds an rwsem that lives in a threadgroup's signal_struct that's > taken for reading in the fork path, under CONFIG_CGROUPS. If another part of > the kernel later wants to use such a locking mechanism, the CONFIG_CGROUPS > ifdefs should be changed to a higher-up flag that CGROUPS and the other system > would both depend on. > > This is a pre-patch for cgroups-procs-write.patch. > Hmm, at adding a new lock, please describe its semantics. Following is my understanding, right ? Lock range: This rwsem is read-locked from cgroup_fork() to cgroup_post_fork(). Most of works for fork() are between cgroup_fork() and cgroup_post_for(). This means if sig->threadgroup_fork_lock is held, no new do_work() can make progress in this process groups. This rwsem is held only when CLONE_THREAD is in clone_flags. IOW, this rwsem is taken only at creating new thread. What we can do with this: By locking sig->threadgroup_fork_lock, a code can visit _all_ threads in a process group witout any races. So, if you want to implement an atomic operation against a process, taking this lock is an idea. For what: To implement an atomic process move in cgroup, we need this lock. Why this implemantation: Considering cgroup, threads in a cgroup can be under several different cgroup. So, we can't implement lock in cgroup-internal, we use signal struct. By the way, IMHO, hiding lock in cgroup_fork() and cgroup_post_fork() doesn't seem good idea. How about a code like this ? read_lock_thread_clone(current); cgroup_fork(); ..... cgroup_post_fork(); read_unlock_thrad_clone(current); We may have chances to move these lock to better position if cgroup is an only user. Thanks, -Kame -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ben Blum on 28 Jun 2010 11:50 On Mon, Jun 28, 2010 at 04:10:31PM +0900, KAMEZAWA Hiroyuki wrote: > On Fri, 25 Jun 2010 01:46:54 -0400 > Ben Blum <bblum(a)andrew.cmu.edu> wrote: > > > Adds functionality to read/write lock CLONE_THREAD fork()ing per-threadgroup > > > > From: Ben Blum <bblum(a)andrew.cmu.edu> > > > > This patch adds an rwsem that lives in a threadgroup's signal_struct that's > > taken for reading in the fork path, under CONFIG_CGROUPS. If another part of > > the kernel later wants to use such a locking mechanism, the CONFIG_CGROUPS > > ifdefs should be changed to a higher-up flag that CGROUPS and the other system > > would both depend on. > > > > This is a pre-patch for cgroups-procs-write.patch. > > > > Hmm, at adding a new lock, please describe its semantics. > Following is my understanding, right ? > > Lock range: > This rwsem is read-locked from cgroup_fork() to cgroup_post_fork(). > Most of works for fork() are between cgroup_fork() and cgroup_post_for(). > This means if sig->threadgroup_fork_lock is held, no new do_work() can > make progress in this process groups. This rwsem is held only when > CLONE_THREAD is in clone_flags. IOW, this rwsem is taken only at creating > new thread. > > What we can do with this: > By locking sig->threadgroup_fork_lock, a code can visit _all_ threads > in a process group witout any races. So, if you want to implement an > atomic operation against a process, taking this lock is an idea. > > For what: > To implement an atomic process move in cgroup, we need this lock. All good. Should a description like this go in Documentation/ somewhere, or above the declaration of the lock? > Why this implemantation: > Considering cgroup, threads in a cgroup can be under several different > cgroup. So, we can't implement lock in cgroup-internal, we use signal > struct. Not entirely, though that's an additional restriction... The reason it's in signal_struct: signal_struct is per-threadgroup and has exactly the lifetime rules we want. Putting the lock in task_struct and taking current->group_leader->signal... seems like it would also work, but introduces cacheline conflicts that weren't there before, since fork doesn't look at group_leader (but it does bump signal's count). > By the way, IMHO, hiding lock in cgroup_fork() and cgroup_post_fork() doesn't > seem good idea. How about a code like this ? > > read_lock_thread_clone(current); > cgroup_fork(); > ..... > cgroup_post_fork(); > read_unlock_thrad_clone(current); > > We may have chances to move these lock to better position if cgroup is > an only user. I didn't do that out of a desire to change fork.c as little as possible, but that does look better than what I've got. Those two functions should be in fork.c under #ifdef CONFIG_CGROUPS. > > Thanks, > -Kame Thanks, -- Ben -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ben Blum on 28 Jul 2010 04:40 On Mon, Jun 28, 2010 at 11:43:59AM -0400, Ben Blum wrote: > On Mon, Jun 28, 2010 at 04:10:31PM +0900, KAMEZAWA Hiroyuki wrote: > > By the way, IMHO, hiding lock in cgroup_fork() and cgroup_post_fork() doesn't > > seem good idea. How about a code like this ? > > > > read_lock_thread_clone(current); > > cgroup_fork(); > > ..... > > cgroup_post_fork(); > > read_unlock_thrad_clone(current); > > > > We may have chances to move these lock to better position if cgroup is > > an only user. > > I didn't do that out of a desire to change fork.c as little as possible, > but that does look better than what I've got. Those two functions should > be in fork.c under #ifdef CONFIG_CGROUPS. I'm looking at this now and am not sure where the best place to put these is: 1) Don't make new functions, just put: #ifdef CONFIG_CGROUPS if (clone_flags & CLONE_THREAD) down/up_read(...); #endif directly in copy_process() in fork.c. Simplest, but uglifies the code. 2) Make static helper functions in fork.c. Good, but not consistent with directly using the lock in write-side (attach_proc). 3) Define inline functions under #ifdef CONFIG_CGROUPS in sched.h, just under the declaration of the lock. Most robust, but I'm hesitant to add unneeded stuff to such a popular header file. Any opinions? -- Ben > > > > > Thanks, > > -Kame > > Thanks, > -- Ben > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: KAMEZAWA Hiroyuki on 28 Jul 2010 05:40 On Wed, 28 Jul 2010 04:29:53 -0400 Ben Blum <bblum(a)andrew.cmu.edu> wrote: > On Mon, Jun 28, 2010 at 11:43:59AM -0400, Ben Blum wrote: > > On Mon, Jun 28, 2010 at 04:10:31PM +0900, KAMEZAWA Hiroyuki wrote: > > > By the way, IMHO, hiding lock in cgroup_fork() and cgroup_post_fork() doesn't > > > seem good idea. How about a code like this ? > > > > > > read_lock_thread_clone(current); > > > cgroup_fork(); > > > ..... > > > cgroup_post_fork(); > > > read_unlock_thrad_clone(current); > > > > > > We may have chances to move these lock to better position if cgroup is > > > an only user. > > > > I didn't do that out of a desire to change fork.c as little as possible, > > but that does look better than what I've got. Those two functions should > > be in fork.c under #ifdef CONFIG_CGROUPS. > > I'm looking at this now and am not sure where the best place to put > these is: > > 1) Don't make new functions, just put: > > #ifdef CONFIG_CGROUPS > if (clone_flags & CLONE_THREAD) > down/up_read(...); > #endif > > directly in copy_process() in fork.c. Simplest, but uglifies the code. > > 2) Make static helper functions in fork.c. Good, but not consistent with > directly using the lock in write-side (attach_proc). > > 3) Define inline functions under #ifdef CONFIG_CGROUPS in sched.h, just > under the declaration of the lock. Most robust, but I'm hesitant to add > unneeded stuff to such a popular header file. > > Any opinions? > My point was simple. Because copy_process() is very important path, the new lock should be visible in copy_process() or kernek/fork.c. "If the lock is visible in copy_process(), the reader can notice it". Then, I offer you 2 options. rename cgroup_fork() and cgroup_post_fork() as cgroup_fork_lock() and cgroup_post_fork_unlock() Now, the lock is visible and the change is minimum. Or add the definition of lock/unlock to cgroup.h and include it from kernel/fork.c Thanks, -Kame -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ben Blum on 28 Jul 2010 11:50
On Wed, Jul 28, 2010 at 06:28:13PM +0900, KAMEZAWA Hiroyuki wrote: > On Wed, 28 Jul 2010 04:29:53 -0400 > Ben Blum <bblum(a)andrew.cmu.edu> wrote: > > > On Mon, Jun 28, 2010 at 11:43:59AM -0400, Ben Blum wrote: > > > On Mon, Jun 28, 2010 at 04:10:31PM +0900, KAMEZAWA Hiroyuki wrote: > > > > By the way, IMHO, hiding lock in cgroup_fork() and cgroup_post_fork() doesn't > > > > seem good idea. How about a code like this ? > > > > > > > > read_lock_thread_clone(current); > > > > cgroup_fork(); > > > > ..... > > > > cgroup_post_fork(); > > > > read_unlock_thrad_clone(current); > > > > > > > > We may have chances to move these lock to better position if cgroup is > > > > an only user. > > > > > > I didn't do that out of a desire to change fork.c as little as possible, > > > but that does look better than what I've got. Those two functions should > > > be in fork.c under #ifdef CONFIG_CGROUPS. > > > > I'm looking at this now and am not sure where the best place to put > > these is: > > > > 1) Don't make new functions, just put: > > > > #ifdef CONFIG_CGROUPS > > if (clone_flags & CLONE_THREAD) > > down/up_read(...); > > #endif > > > > directly in copy_process() in fork.c. Simplest, but uglifies the code. > > > > 2) Make static helper functions in fork.c. Good, but not consistent with > > directly using the lock in write-side (attach_proc). > > > > 3) Define inline functions under #ifdef CONFIG_CGROUPS in sched.h, just > > under the declaration of the lock. Most robust, but I'm hesitant to add > > unneeded stuff to such a popular header file. > > > > Any opinions? > > > > My point was simple. Because copy_process() is very important path, > the new lock should be visible in copy_process() or kernek/fork.c. > "If the lock is visible in copy_process(), the reader can notice it". > > Then, I offer you 2 options. > > rename cgroup_fork() and cgroup_post_fork() as > cgroup_fork_lock() and cgroup_post_fork_unlock() > > Now, the lock is visible and the change is minimum. > > Or > add the definition of lock/unlock to cgroup.h and include it > from kernel/fork.c > > Thanks, > -Kame I don't like either of these. Renaming to cgroup_fork_lock not only conveys the sense that a cgroup-specific lock is taken, but also hides the real purpose of these functions, which is to manipulate cgroup pointers. And it's not a cgroup-specific lock - only write-side is *currently* used by cgroups - so it shouldn't go in cgroup.h. -- Ben -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |