Messages in this thread | | | Date | Wed, 28 Sep 2011 12:23:13 -0300 | From | Glauber Costa <> | Subject | Re: [RFD 3/9] Display /proc/stat information per cgroup |
| |
On 09/27/2011 07:21 PM, Peter Zijlstra wrote: > On Tue, 2011-09-27 at 15:42 -0300, Glauber Costa wrote: >>>> +static inline void task_cgroup_account_field(struct task_struct *p, >>>> + cputime64_t tmp, int index) >>>> +{ >>>> + struct kernel_stat *kstat; >>>> + struct task_group *tg = task_group(p); >>>> + >>>> + do { >>>> + kstat = this_cpu_ptr(tg->cpustat); >>>> + kstat->cpustat[index] = cputime64_add(kstat->cpustat[index], >>>> + tmp); >>>> + tg = tg->parent; >>>> + } while (tg); >>>> +} >>> >>> What protects the walk (tg = tg->parent)? Could you please document it >> I think that the fact that the hierarchy only grows down, thus parent >> never changes (or am I wrong?) >> >> And since we run all this with preempt disabled and with the runqueue >> locked, we should have no problems. >> >> Do you agree? > > Right, so the tg can't be destroyed unless its empty, us finding this > task in it means its not empty, we require rq->lock or p->pi_lock to > move the task. > > However, afaict we don't actually have any of those locks. > > That said, it should be sufficient to wrap the whole thing in > rcu_read_lock(), accounting one tick funny because a cgroup move race > isn't the end of the world and its really no different than moving the > task a little later anyway. > > task_group() should complain about this if you compile a kernel with > CONFIG_PROVE_RCU.
Yeah, wrapping it around rcu_read_lock looks fine.
| |