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@xxxxxxxxxxxxxx> 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 _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers
