On Sat, Jul 29, 2017 at 11:10:55AM +0200, Peter Zijlstra wrote:
> On Thu, Jul 27, 2017 at 11:30:10AM -0400, Johannes Weiner wrote:
> > +static void domain_cpu_update(struct memdelay_domain *md, int cpu,
> > + int old, int new)
> > +{
> > + enum memdelay_domain_state state;
> > + struct memdelay_domain_cpu *mdc;
> > + unsigned long now, delta;
> > + unsigned long flags;
> > +
> > + mdc = per_cpu_ptr(md->mdcs, cpu);
> > + spin_lock_irqsave(&mdc->lock, flags);
>
> Afaict this is inside scheduler locks, this cannot be a spinlock. Also,
> do we really want to add more atomics there?
I think we should be able to get away without an additional lock and
rely on the rq lock instead. schedule, enqueue, dequeue already hold
it, memdelay_enter/leave could be added. I need to think about what to
do with try_to_wake_up in order to get the cpu move accounting inside
the locked section of ttwu_queue(), but that should be doable too.
> > + if (old) {
> > + WARN_ONCE(!mdc->tasks[old], "cpu=%d old=%d new=%d counter=%d\n",
> > + cpu, old, new, mdc->tasks[old]);
> > + mdc->tasks[old] -= 1;
> > + }
> > + if (new)
> > + mdc->tasks[new] += 1;
> > +
> > + /*
> > + * The domain is somewhat delayed when a number of tasks are
> > + * delayed but there are still others running the workload.
> > + *
> > + * The domain is fully delayed when all non-idle tasks on the
> > + * CPU are delayed, or when a delayed task is actively running
> > + * and preventing productive tasks from making headway.
> > + *
> > + * The state times then add up over all CPUs in the domain: if
> > + * the domain is fully blocked on one CPU and there is another
> > + * one running the workload, the domain is considered fully
> > + * blocked 50% of the time.
> > + */
> > + if (!mdc->tasks[MTS_DELAYED_ACTIVE] && !mdc->tasks[MTS_DELAYED])
> > + state = MDS_NONE;
> > + else if (mdc->tasks[MTS_WORKING])
> > + state = MDS_SOME;
> > + else
> > + state = MDS_FULL;
> > +
> > + if (mdc->state == state)
> > + goto unlock;
> > +
> > + now = ktime_to_ns(ktime_get());
>
> ktime_get_ns(), also no ktime in scheduler code.
Okay.
I actually don't need a time source that's comparable across CPUs
since accounting periods are always fully contained within one
CPU. From the comment docs, it sounds like cpu_clock() is what I want
to use there?
> > + /* Account domain state changes */
> > + rcu_read_lock();
> > + memcg = mem_cgroup_from_task(task);
> > + do {
> > + struct memdelay_domain *md;
> > +
> > + md = memcg_domain(memcg);
> > + md->aggregate += delay;
> > + domain_cpu_update(md, cpu, old, new);
> > + } while (memcg && (memcg = parent_mem_cgroup(memcg)));
> > + rcu_read_unlock();
>
> We are _NOT_ going to do a 3rd cgroup iteration for every task action.
I'll look into that.
Thanks
