Messages in this thread |  | | Date | Sun, 13 Aug 2017 12:44:21 -0700 | From | Tejun Heo <> | Subject | Re: [PATCH 3/3] cgroup: Implement cgroup2 basic CPU usage accounting |
| |
Hello, Waiman.
On Fri, Aug 11, 2017 at 04:19:07PM -0400, Waiman Long wrote: > > * CPU usage stats are collected and shown in "cgroup.stat" with "cpu." > > prefix. Total usage is collected from scheduling events. User/sys > > breakdown is sourced from tick sampling and adjusted to the usage > > using cputime_adjuts(). > > Typo: cputime_adjust()
Oops, will fix.
> > +static DEFINE_MUTEX(cgroup_stat_mutex); > > +static DEFINE_PER_CPU(raw_spinlock_t, cgroup_cpu_stat_lock); > > If the hierarchy is large enough and the stat data hasn't been read > recently, it may take a while to accumulate all the stat data even for > one cpu in cgroup_stat_flush_locked(). So I think it will make more > sense to use regular spinlock_t instead of raw_spinlock_t.
They need to be raw spinlocks because the accounting side may grab them while scheduling. If the accumulating latency becomes problematic, we can test for need_resched and spin_needbreak and break out as necessary. The iteration logic is built to allow that and an earlier revision actually did that but I wasn't sure whether it's actually necessary and removed it for simplicity.
If the latency ever becomes a problem, resurrecting that isn't difficult at all.
> > +static struct cgroup *cgroup_cpu_stat_next_updated(struct cgroup *pos, > > + struct cgroup *root, int cpu) > > This function is trying to unwind one cgrp from the updated_children and > updated_next linkage. It is somewhat like the opposite of > cgroup_cpu_stat_updated(). I just feel like its name isn't intuitive > enough to convey what it is doing. Maybe use name like > cgroup_cpu_stat_unlink_one() to match cgroup_cpu_stat_flush_one().
Hmm... the name comes from it being an iterator - most interators are named _next. But, yeah, the name doesn't signifiy that it unlinks as it goes along. I'll rename it to cgroup_cpu_stat_pop_updated().
Thanks.
-- tejun
|  |