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

[Peter Zijlstra <peterz@infradead.org>]

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 ?