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