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
