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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241214000951.GA16123@noisy.programming.kicks-ass.net>
Date: Sat, 14 Dec 2024 01:09:51 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: John Stultz <jstultz@...gle.com>
Cc: LKML <linux-kernel@...r.kernel.org>, Joel Fernandes <joelaf@...gle.com>,
	Qais Yousef <qyousef@...alina.io>, Ingo Molnar <mingo@...hat.com>,
	Juri Lelli <juri.lelli@...hat.com>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>,
	Valentin Schneider <vschneid@...hat.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ben Segall <bsegall@...gle.com>,
	Zimuzo Ezeozue <zezeozue@...gle.com>, Mel Gorman <mgorman@...e.de>,
	Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>,
	Boqun Feng <boqun.feng@...il.com>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Metin Kaya <Metin.Kaya@....com>,
	Xuewen Yan <xuewen.yan94@...il.com>,
	K Prateek Nayak <kprateek.nayak@....com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Daniel Lezcano <daniel.lezcano@...aro.org>, kernel-team@...roid.com
Subject: Re: [RFC][PATCH v14 3/7] sched: Fix runtime accounting w/ split exec
 & sched contexts

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