Re: [RFC PATCH -tip] cpuacct: per-cgroup utime/stime statistics -v2

[KAMEZAWA Hiroyuki]

On Mon, 16 Mar 2009 10:07:54 +0530
Bharata B Rao <bharata@xxxxxxxxxxxxxxxxxx> wrote:

> > > +static void cpuacct_update_stats(struct task_struct *tsk,
> > > +		enum cpuacct_stat_index idx, cputime_t val)
> > > +{
> > > +	struct cpuacct *ca;
> > > +
> > > +	if (unlikely(!cpuacct_subsys.active))
> > > +		return;
> > > +
> > > +	ca = task_ca(tsk);
> > > +
> > > +	do {
> > > +		percpu_counter_add(&ca->cpustat[idx], val);
> > > +		ca = ca->parent;
> > > +	} while (ca);
> > > +}
> > > +
> > 
> > IIUC, to make sure accessing "ca" to be safe, we need some condition.
> > (task_lock() or some other.....
> 
> task_lock() protects tsk->cgroups->subsys[]. So can we hold task_lock()
> to protect this walk ?
If necessary (see below)

> But we do this cpuacct hierarchy walk for the
> current task here. So can a current task's ca or ca's parents disappear
> from under us ?

Ok, followings are correct, I think.
 1. All updates are called under preempt-disabled.
 2. If "ca" is pointed by task, "disappear" will not happen.
 3. if one of children is alive, the parent is alive.
 4. "attach" can happen and tsk may be moved to other cgroup,
     because we don't take task_lock.
     So, ca = task_ca() may not be correct one.

What we should explain here is How we consider "4", here.

> > > +	ca = task_ca(tsk);
> > > + ----------------------------------(*1)
> > > +	do {
> > > +		percpu_counter_add(&ca->cpustat[idx], val);
> > > +		ca = ca->parent;
> > > +	} while (ca);
        ----------------------------------(*2)

About accounting, we don't have any problem even if (*2) is different from (*1).
But, considering "ca" can be empty cgroup after (*1), in _theory_, we cannot
say "ca" is valid pointer after (*1).

Hmm, if necessary, adding rcu_read_lock() before (*1) is reasonable ?
(I say "if necessary" because I'm not sure.)

Thanks,
-Kame 






--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/