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

Dan Carpenter
2024-12-06T10:43:32Z

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) {

regards,
dan carpenter

> + /*
> + * Leaving at least one CPU for DEADLINE tasks seems a
> + * wise thing to do.
> + */
> + if (dl_bw_cpus(cpu))
> + overflow = __dl_overflow(dl_b, cap, fair_server_bw, 0);
> + else
> + overflow = 1;
> + }
> +
> + break;
> }