Re: [RFC][PATCH 1/8] cgroup: introduce cancel_attach()

[Daisuke Nishimura <d-nishimura@mtf.biglobe.ne.jp>]

Thank you for your comments.

On Thu, 15 Oct 2009 00:37:23 -0700
Paul Menage <menage@google.com> wrote:
> On Mon, Oct 12, 2009 at 9:50 PM, Daisuke Nishimura
> <nishimura@mxp.nes.nec.co.jp> wrote:
> > A int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> > A {
> > A  A  A  A int retval = 0;
> > - A  A  A  struct cgroup_subsys *ss;
> > + A  A  A  struct cgroup_subsys *ss, *fail = NULL;
> 
> Maybe give this a more descriptive name, such as "failed_subsys" ?
> 
will fix.

> > @@ -1553,8 +1553,10 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
> > A  A  A  A for_each_subsys(root, ss) {
> > A  A  A  A  A  A  A  A if (ss->can_attach) {
> > A  A  A  A  A  A  A  A  A  A  A  A retval = ss->can_attach(ss, cgrp, tsk, false);
> > - A  A  A  A  A  A  A  A  A  A  A  if (retval)
> > - A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  return retval;
> > + A  A  A  A  A  A  A  A  A  A  A  if (retval) {
> > + A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  fail = ss;
> 
> Comment here on why you set "fail" ?
> > +out:
> > + A  A  A  if (retval)
> > + A  A  A  A  A  A  A  for_each_subsys(root, ss) {
> > + A  A  A  A  A  A  A  A  A  A  A  if (ss == fail)
> 
> Comment here?
> 
"fail" is used to call ->cancel_attach() only for subsystems
->can_attach() of which have succeeded.

> The problem with this API is that the subsystem doesn't know how long
> it needs to hold on to the potential rollback state for. The design
> for transactional cgroup attachment that I sent out over a year ago
> presented a more robust API, but I've not had time to work on it. So
> maybe this patch could be a stop-gap.
> 
I digged my mail box and read your RFC, and I agree those APIs would be
more desirable.
In 2-8 of this patch-set, I do similar things as you proposed in the RFC.
I use ->can_attach() as prepare_attach_sleep(), ->attach() as commit_attach(),
and ->cancel_attach() as abort_attach_sleep(), so I think it wouldn't be so
difficult to adapt my interfaces to new API when it comes.
The reason why I introduced ->cancel_attach() is I need to cancel(do rollback)
the charge reservation against @to cgroup, which I've reserved in ->can_attach(),
and this patch is enough for my purpose for now.

Anyway, I'll add more comments.


Thanks,
Daisuke Nishimura.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>