Messages in this thread | | | Date | Wed, 5 Oct 2011 16:43:20 +0400 | From | Glauber Costa <> | Subject | Re: [PATCH 04/10] Display /proc/stat information per cgroup |
| |
On 10/05/2011 04:38 PM, Peter Zijlstra wrote: > On Wed, 2011-10-05 at 16:10 +0400, Glauber Costa wrote: >> On 10/05/2011 12:56 PM, Peter Zijlstra wrote: >>> On Sun, 2011-10-02 at 23:21 +0400, Glauber Costa wrote: >>>> +struct kernel_stat *task_group_kstat(struct task_struct *p) >>>> +{ >>>> + struct task_group *tg; >>>> + struct kernel_stat *kstat; >>>> + >>>> + rcu_read_lock(); >>>> + tg = task_group(p); >>>> + kstat = tg->cpustat; >>>> + rcu_read_unlock(); >>>> + return kstat; >>>> +} >>> >>> Who keeps tg alive and kicking while you poke at its (cpustat) member? >> >> * All calls to this function currently pass current as a parameter >> (Okay, maybe it is too generic and we should pass nothing at all, and >> grab current within it) >> * rcu_read_lock() guarantees that current will exist during this call, >> and task_group won't change. (right?) > > The thing I worry about is: > > A (pid n) B > > kstat = task_group_kstat() > echo n> /cgroup/something-else/pid > rmdir /cgroup/group-that-had-A > <timer interrupt> > RCU complete > <softirq> > kfree(tg) etc.. > > kstat->foo++;<-- *BOOM* > > > The only way to avoid someone moving you around is by holding some > cgroup lock, task->alloc_lock, task->pi_lock or the rq->lock where task > runs. Alternatively keep rcu_read_lock() around the entire kstat usage. > > Well, if I understand it correctly, we'd have to hold the lock around the entire kstat usge as well, right?
Otherwise it can just explode. So rcu does seem the way to go. I do, however, see the problem you are describing.
Maybe we can remove the rcu_read_lock() call and replace with a rcu validation. Then patch all callers. (Including of course the current users of kstat_cpu() all over). What do you think?
| |