Re: [RFC][PATCH v14 3/7] sched: Fix runtime accounting w/ split exec & sched contexts

Peter Zijlstra
2024-12-14T00:10:35Z

On Sat, Dec 14, 2024 at 12:37:40AM +0100, Peter Zijlstra wrote:
> On Mon, Nov 25, 2024 at 11:51:57AM -0800, John Stultz wrote:
>
>
>
> > -static s64 update_curr_se(struct rq *rq, struct sched_entity *curr)
> > +static s64 update_curr_se(struct rq *rq, struct sched_entity *se)
> > {
> > u64 now = rq_clock_task(rq);
> > s64 delta_exec;
> >
> > - delta_exec = now - curr->exec_start;
> > + delta_exec = now - se->exec_start;
> > if (unlikely(delta_exec <= 0))
> > return delta_exec;
> >
> > - curr->exec_start = now;
> > - curr->sum_exec_runtime += delta_exec;
> > + se->exec_start = now;
> > + if (entity_is_task(se)) {
> > + struct task_struct *running = rq->curr;
> > + /*
> > + * If se is a task, we account the time against the running
> > + * task, as w/ proxy-exec they may not be the same.
> > + */
> > + running->se.exec_start = now;
> > + running->se.sum_exec_runtime += delta_exec;
> > + } else {
> > + /* If not task, account the time against se */
> > + se->sum_exec_runtime += delta_exec;
> > + }
> >
> > if (schedstat_enabled()) {
> > struct sched_statistics *stats;
> >
> > - stats = __schedstats_from_se(curr);
> > + stats = __schedstats_from_se(se);
> > __schedstat_set(stats->exec_max,
> > max(delta_exec, stats->exec_max));
> > }
>
> Would it not be *much* clearer if we do it like:
>
> static s64 update_curr_se(struct rq *rq, struct sched_entity *donor,
> struct sched_entity *curr)
> {
> ...
> donor->exec_start = now;
> curr->exec_start = now;
> curr->sum_exec_runtime += delta_exec;
> ...
> }
>
> and update the callsites like so:
>
> update_curr_common()
> update_curr_se(rq, &donor->se, &rq->curr.se)
>
> update_curr()
> update_curr_se(rq, &curr->se, &curr->se);
>
>
> except, now I'm confused about the update_curr() case. That seems to
> always update the execution context, rather than the donor ?

Ah no, cfs_rq->curr is the donor.

I'll try again later; or risk keyboard face.. Zzzz