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

[Juri Lelli <juri.lelli@redhat.com>]

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