linux-kernel - Re: [PATCH v2 2/2] sched/deadline: Correctly account for allocated bandwidth during hotplug

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] [day] [month] [year] [list]

Message-ID: <Z1b8mGctDusOZPA8@jlelli-thinkpadt14gen4.remote.csb>
Date: Mon, 9 Dec 2024 14:20:08 +0000
From: Juri Lelli <juri.lelli@...hat.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Waiman Long <longman@...hat.com>, Tejun Heo <tj@...nel.org>,
	Johannes Weiner <hannes@...xchg.org>,
	Michal Koutny <mkoutny@...e.com>, Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
	Valentin Schneider <vschneid@...hat.com>,
	Phil Auld <pauld@...hat.com>, Qais Yousef <qyousef@...alina.io>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	"Joel Fernandes (Google)" <joel@...lfernandes.org>,
	Suleiman Souhlal <suleiman@...gle.com>,
	Aashish Sharma <shraash@...gle.com>,
	Shin Kawamura <kawasin@...gle.com>,
	Vineeth Remanan Pillai <vineeth@...byteword.org>,
	linux-kernel@...r.kernel.org, cgroups@...r.kernel.org
Subject: Re: [PATCH v2 2/2] sched/deadline: Correctly account for allocated
 bandwidth during hotplug

Hi,

On 06/12/24 13:43, Dan Carpenter wrote:
> On Thu, Nov 14, 2024 at 02:28:10PM +0000, Juri Lelli wrote:
> >  static int dl_bw_manage(enum dl_bw_request req, int cpu, u64 dl_bw)
> >  {
> > -	unsigned long flags;
> > +	unsigned long flags, cap;
> >  	struct dl_bw *dl_b;
> >  	bool overflow = 0;
> > +	u64 fair_server_bw = 0;
>         ^^^^^^^^^^^^^^^^^^
> This is a u64.
> 
> >  
> >  	rcu_read_lock_sched();
> >  	dl_b = dl_bw_of(cpu);
> >  	raw_spin_lock_irqsave(&dl_b->lock, flags);
> >  
> > -	if (req == dl_bw_req_free) {
> > +	cap = dl_bw_capacity(cpu);
> > +	switch (req) {
> > +	case dl_bw_req_free:
> >  		__dl_sub(dl_b, dl_bw, dl_bw_cpus(cpu));
> > -	} else {
> > -		unsigned long cap = dl_bw_capacity(cpu);
> > -
> > +		break;
> > +	case dl_bw_req_alloc:
> >  		overflow = __dl_overflow(dl_b, cap, 0, dl_bw);
> >  
> > -		if (req == dl_bw_req_alloc && !overflow) {
> > +		if (!overflow) {
> >  			/*
> >  			 * We reserve space in the destination
> >  			 * root_domain, as we can't fail after this point.
> > @@ -3501,6 +3503,34 @@ static int dl_bw_manage(enum dl_bw_request req, int cpu, u64 dl_bw)
> >  			 */
> >  			__dl_add(dl_b, dl_bw, dl_bw_cpus(cpu));
> >  		}
> > +		break;
> > +	case dl_bw_req_deactivate:
> > +		/*
> > +		 * cpu is going offline and NORMAL tasks will be moved away
> > +		 * from it. We can thus discount dl_server bandwidth
> > +		 * contribution as it won't need to be servicing tasks after
> > +		 * the cpu is off.
> > +		 */
> > +		if (cpu_rq(cpu)->fair_server.dl_server)
> > +			fair_server_bw = cpu_rq(cpu)->fair_server.dl_bw;
> > +
> > +		/*
> > +		 * Not much to check if no DEADLINE bandwidth is present.
> > +		 * dl_servers we can discount, as tasks will be moved out the
> > +		 * offlined CPUs anyway.
> > +		 */
> > +		if (dl_b->total_bw - fair_server_bw > 0) {
>                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Since this subtraction is unsigned the condition is equivalent to:
> 
>         if (dl_b->total_bw != fair_server_bw)
> 
> but it feels like maybe it was intended to be:
> 
>         if (dl_b->total_bw > fair_server_bw) {

I actually believe they are equivalent for this case, as if there is a
dl_server total_bw is either equal or bigger than fair_server_bw, so
checking for it to be different than fair_server_bw should still be OK
(even though confusing maybe).

Thanks,
Juri