[PATCH 0/2] Fix DEADLINE bandwidth accounting in root domain changes and hotplug

[PATCH 0/2] Fix DEADLINE bandwidth accounting in root domain changes and hotplug

[Juri Lelli]

Hello!

This patch series addresses two issues affecting DEADLINE bandwidth
accounting during non-destructive changes to root domains and hotplug
operations. The series is based on top of Waiman's "cgroup/cpuset:
Remove redundant rebuild_sched_domains_locked() calls" series [1] which
is now merged into cgroups/for-6.13 (this series is based on top of
that, commit c4c9cebe2fb9). The discussion that eventually led to these
two series can be found at [2].

Patch 01/02 deals with non-destructive root domain changes. Currently,
such operations do not correctly account for dl_server contributions, as
they are only considered on destructive changes (where runqueues are
reattached to new domains). This patch ensures that dl_servers’
bandwidth contributions are properly re-added to root domains that
remain unchanged, preventing accounting discrepancies.

Patch 02/02 deals with hotplug. For CPU hotplug events, DEADLINE
bandwidth needs verification to ensure adequate bandwidth remains after
a CPU goes offline. The current implementation overlooks this check,
potentially leading to overflow conditions. This patch modifies
dl_bw_manage() to correctly detect and handle overflow situations,
special casing dl_servers' bandwidth contibution.

Please go forth and test/review.

Series also available at

git@github.com:jlelli/linux.git upstream/dl-server-apply

Best,
Juri

[1] https://lore.kernel.org/lkml/20241110025023.664487-1-longman@redhat.com/
[2] https://lore.kernel.org/lkml/20241029225116.3998487-1-joel@joelfernandes.org/

Juri Lelli (2):
  sched/deadline: Restore dl_server bandwidth on non-destructive root
    domain changes
  sched/deadline: Correctly account for allocated bandwidth during
    hotplug

 include/linux/sched/deadline.h |  2 +-
 kernel/cgroup/cpuset.c         |  2 +-
 kernel/sched/core.c            |  2 +-
 kernel/sched/deadline.c        | 51 ++++++++++++++++++++++++----------
 kernel/sched/sched.h           |  2 +-
 kernel/sched/topology.c        | 10 ++++---
 6 files changed, 47 insertions(+), 22 deletions(-)

-- 
2.47.0

[PATCH 1/2] sched/deadline: Restore dl_server bandwidth on non-destructive root domain changes

[Juri Lelli]

When root domain non-destructive changes (e.g., only modifying one of
the existing root domains while the rest is not touched) happen we still
need to clear DEADLINE bandwidth accounting so that it's then properly
restore taking into account DEADLINE tasks associated to each cpuset
(associated to each root domain). After the introduction of dl_servers,
we fail to restore such servers contribution after non-destructive
changes (as they are only considered on destructive changes when
runqueues are attached to the new domains).

Fix this by making sure we iterate over the dl_server attached to
domains that have not been destroyed and add them bandwidth contribution
back correctly.

Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
---
 include/linux/sched/deadline.h |  2 +-
 kernel/cgroup/cpuset.c         |  2 +-
 kernel/sched/deadline.c        | 18 +++++++++++++-----
 kernel/sched/topology.c        | 10 ++++++----
 4 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/include/linux/sched/deadline.h b/include/linux/sched/deadline.h
index 3a912ab42bb5..82c966a55856 100644
--- a/include/linux/sched/deadline.h
+++ b/include/linux/sched/deadline.h
@@ -33,7 +33,7 @@ static inline bool dl_time_before(u64 a, u64 b)
 
 struct root_domain;
 extern void dl_add_task_root_domain(struct task_struct *p);
-extern void dl_clear_root_domain(struct root_domain *rd);
+extern void dl_clear_root_domain(struct root_domain *rd, bool restore);
 
 #endif /* CONFIG_SMP */
 
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index 142303abb055..4d3603a99db3 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -954,7 +954,7 @@ static void dl_rebuild_rd_accounting(void)
 	 * Clear default root domain DL accounting, it will be computed again
 	 * if a task belongs to it.
 	 */
-	dl_clear_root_domain(&def_root_domain);
+	dl_clear_root_domain(&def_root_domain, false);
 
 	cpuset_for_each_descendant_pre(cs, pos_css, &top_cpuset) {
 
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 9ce93d0bf452..e53208a50279 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -2968,13 +2968,21 @@ void dl_add_task_root_domain(struct task_struct *p)
 	task_rq_unlock(rq, p, &rf);
 }
 
-void dl_clear_root_domain(struct root_domain *rd)
+void dl_clear_root_domain(struct root_domain *rd, bool restore)
 {
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&rd->dl_bw.lock, flags);
+	guard(raw_spinlock_irqsave)(&rd->dl_bw.lock);
 	rd->dl_bw.total_bw = 0;
-	raw_spin_unlock_irqrestore(&rd->dl_bw.lock, flags);
+
+	if (restore) {
+		int i;
+
+		for_each_cpu(i, rd->span) {
+			struct sched_dl_entity *dl_se = &cpu_rq(i)->fair_server;
+
+			if (dl_server(dl_se))
+				rd->dl_bw.total_bw += dl_se->dl_bw;
+		}
+	}
 }
 
 #endif /* CONFIG_SMP */
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 9748a4c8d668..e9e7a7c43dd6 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -2721,12 +2721,14 @@ void partition_sched_domains_locked(int ndoms_new, cpumask_var_t doms_new[],
 
 				/*
 				 * This domain won't be destroyed and as such
-				 * its dl_bw->total_bw needs to be cleared.  It
-				 * will be recomputed in function
-				 * update_tasks_root_domain().
+				 * its dl_bw->total_bw needs to be cleared.
+				 * Tasks contribution will be then recomputed
+				 * in function dl_update_tasks_root_domain(),
+				 * dl_servers contribution in function
+				 * dl_restore_server_root_domain().
 				 */
 				rd = cpu_rq(cpumask_any(doms_cur[i]))->rd;
-				dl_clear_root_domain(rd);
+				dl_clear_root_domain(rd, true);
 				goto match1;
 			}
 		}
-- 
2.47.0

Re: [PATCH 1/2] sched/deadline: Restore dl_server bandwidth on non-destructive root domain changes

[Phil Auld]

Hi Juri,

On Wed, Nov 13, 2024 at 12:57:22PM +0000 Juri Lelli wrote:
> When root domain non-destructive changes (e.g., only modifying one of
> the existing root domains while the rest is not touched) happen we still
> need to clear DEADLINE bandwidth accounting so that it's then properly
> restore taking into account DEADLINE tasks associated to each cpuset

"restored, taking ..."  ?

> (associated to each root domain). After the introduction of dl_servers,
> we fail to restore such servers contribution after non-destructive
> changes (as they are only considered on destructive changes when
> runqueues are attached to the new domains).
> 
> Fix this by making sure we iterate over the dl_server attached to
> domains that have not been destroyed and add them bandwidth contribution
> back correctly.
> 
> Signed-off-by: Juri Lelli <juri.lelli@redhat.com>


Looks good to me. 


Reviewed-by: Phil Auld <pauld@redhat.com>


> ---
>  include/linux/sched/deadline.h |  2 +-
>  kernel/cgroup/cpuset.c         |  2 +-
>  kernel/sched/deadline.c        | 18 +++++++++++++-----
>  kernel/sched/topology.c        | 10 ++++++----
>  4 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/sched/deadline.h b/include/linux/sched/deadline.h
> index 3a912ab42bb5..82c966a55856 100644
> --- a/include/linux/sched/deadline.h
> +++ b/include/linux/sched/deadline.h
> @@ -33,7 +33,7 @@ static inline bool dl_time_before(u64 a, u64 b)
>  
>  struct root_domain;
>  extern void dl_add_task_root_domain(struct task_struct *p);
> -extern void dl_clear_root_domain(struct root_domain *rd);
> +extern void dl_clear_root_domain(struct root_domain *rd, bool restore);
>  
>  #endif /* CONFIG_SMP */
>  
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 142303abb055..4d3603a99db3 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -954,7 +954,7 @@ static void dl_rebuild_rd_accounting(void)
>  	 * Clear default root domain DL accounting, it will be computed again
>  	 * if a task belongs to it.
>  	 */
> -	dl_clear_root_domain(&def_root_domain);
> +	dl_clear_root_domain(&def_root_domain, false);
>  
>  	cpuset_for_each_descendant_pre(cs, pos_css, &top_cpuset) {
>  
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 9ce93d0bf452..e53208a50279 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -2968,13 +2968,21 @@ void dl_add_task_root_domain(struct task_struct *p)
>  	task_rq_unlock(rq, p, &rf);
>  }
>  
> -void dl_clear_root_domain(struct root_domain *rd)
> +void dl_clear_root_domain(struct root_domain *rd, bool restore)
>  {
> -	unsigned long flags;
> -
> -	raw_spin_lock_irqsave(&rd->dl_bw.lock, flags);
> +	guard(raw_spinlock_irqsave)(&rd->dl_bw.lock);
>  	rd->dl_bw.total_bw = 0;
> -	raw_spin_unlock_irqrestore(&rd->dl_bw.lock, flags);
> +
> +	if (restore) {
> +		int i;
> +
> +		for_each_cpu(i, rd->span) {
> +			struct sched_dl_entity *dl_se = &cpu_rq(i)->fair_server;
> +
> +			if (dl_server(dl_se))
> +				rd->dl_bw.total_bw += dl_se->dl_bw;
> +		}
> +	}
>  }
>  
>  #endif /* CONFIG_SMP */
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 9748a4c8d668..e9e7a7c43dd6 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -2721,12 +2721,14 @@ void partition_sched_domains_locked(int ndoms_new, cpumask_var_t doms_new[],
>  
>  				/*
>  				 * This domain won't be destroyed and as such
> -				 * its dl_bw->total_bw needs to be cleared.  It
> -				 * will be recomputed in function
> -				 * update_tasks_root_domain().
> +				 * its dl_bw->total_bw needs to be cleared.
> +				 * Tasks contribution will be then recomputed
> +				 * in function dl_update_tasks_root_domain(),
> +				 * dl_servers contribution in function
> +				 * dl_restore_server_root_domain().
>  				 */
>  				rd = cpu_rq(cpumask_any(doms_cur[i]))->rd;
> -				dl_clear_root_domain(rd);
> +				dl_clear_root_domain(rd, true);
>  				goto match1;
>  			}
>  		}
> -- 
> 2.47.0
> 
> 

-- 

Re: [PATCH 1/2] sched/deadline: Restore dl_server bandwidth on non-destructive root domain changes

[Juri Lelli]

Hi Phil,

On 13/11/24 08:43, Phil Auld wrote:
> Hi Juri,
> 
> On Wed, Nov 13, 2024 at 12:57:22PM +0000 Juri Lelli wrote:
> > When root domain non-destructive changes (e.g., only modifying one of
> > the existing root domains while the rest is not touched) happen we still
> > need to clear DEADLINE bandwidth accounting so that it's then properly
> > restore taking into account DEADLINE tasks associated to each cpuset
> 
> "restored, taking ..."  ?

Yep.

> > (associated to each root domain). After the introduction of dl_servers,
> > we fail to restore such servers contribution after non-destructive
> > changes (as they are only considered on destructive changes when
> > runqueues are attached to the new domains).
> > 
> > Fix this by making sure we iterate over the dl_server attached to
> > domains that have not been destroyed and add them bandwidth contribution
> > back correctly.
> > 
> > Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
> 
> 
> Looks good to me. 
> 
> 
> Reviewed-by: Phil Auld <pauld@redhat.com>

Thanks!
Juri

Re: [PATCH 1/2] sched/deadline: Restore dl_server bandwidth on non-destructive root domain changes

[Waiman Long]

On 11/13/24 7:57 AM, Juri Lelli wrote:
> When root domain non-destructive changes (e.g., only modifying one of
> the existing root domains while the rest is not touched) happen we still
> need to clear DEADLINE bandwidth accounting so that it's then properly
> restore taking into account DEADLINE tasks associated to each cpuset
> (associated to each root domain). After the introduction of dl_servers,
> we fail to restore such servers contribution after non-destructive
> changes (as they are only considered on destructive changes when
> runqueues are attached to the new domains).
>
> Fix this by making sure we iterate over the dl_server attached to
> domains that have not been destroyed and add them bandwidth contribution
> back correctly.
>
> Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
> ---
>   include/linux/sched/deadline.h |  2 +-
>   kernel/cgroup/cpuset.c         |  2 +-
>   kernel/sched/deadline.c        | 18 +++++++++++++-----
>   kernel/sched/topology.c        | 10 ++++++----
>   4 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/sched/deadline.h b/include/linux/sched/deadline.h
> index 3a912ab42bb5..82c966a55856 100644
> --- a/include/linux/sched/deadline.h
> +++ b/include/linux/sched/deadline.h
> @@ -33,7 +33,7 @@ static inline bool dl_time_before(u64 a, u64 b)
>   
>   struct root_domain;
>   extern void dl_add_task_root_domain(struct task_struct *p);
> -extern void dl_clear_root_domain(struct root_domain *rd);
> +extern void dl_clear_root_domain(struct root_domain *rd, bool restore);
>   
>   #endif /* CONFIG_SMP */
>   
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 142303abb055..4d3603a99db3 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -954,7 +954,7 @@ static void dl_rebuild_rd_accounting(void)
>   	 * Clear default root domain DL accounting, it will be computed again
>   	 * if a task belongs to it.
>   	 */
> -	dl_clear_root_domain(&def_root_domain);
> +	dl_clear_root_domain(&def_root_domain, false);
>   
>   	cpuset_for_each_descendant_pre(cs, pos_css, &top_cpuset) {
>   
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 9ce93d0bf452..e53208a50279 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -2968,13 +2968,21 @@ void dl_add_task_root_domain(struct task_struct *p)
>   	task_rq_unlock(rq, p, &rf);
>   }
>   
> -void dl_clear_root_domain(struct root_domain *rd)
> +void dl_clear_root_domain(struct root_domain *rd, bool restore)
>   {
> -	unsigned long flags;
> -
> -	raw_spin_lock_irqsave(&rd->dl_bw.lock, flags);
> +	guard(raw_spinlock_irqsave)(&rd->dl_bw.lock);
>   	rd->dl_bw.total_bw = 0;
> -	raw_spin_unlock_irqrestore(&rd->dl_bw.lock, flags);
> +
> +	if (restore) {
> +		int i;
> +
> +		for_each_cpu(i, rd->span) {
> +			struct sched_dl_entity *dl_se = &cpu_rq(i)->fair_server;
> +
> +			if (dl_server(dl_se))
> +				rd->dl_bw.total_bw += dl_se->dl_bw;
> +		}
> +	}
>   }
>   
>   #endif /* CONFIG_SMP */
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 9748a4c8d668..e9e7a7c43dd6 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -2721,12 +2721,14 @@ void partition_sched_domains_locked(int ndoms_new, cpumask_var_t doms_new[],
>   
>   				/*
>   				 * This domain won't be destroyed and as such
> -				 * its dl_bw->total_bw needs to be cleared.  It
> -				 * will be recomputed in function
> -				 * update_tasks_root_domain().
> +				 * its dl_bw->total_bw needs to be cleared.
> +				 * Tasks contribution will be then recomputed
> +				 * in function dl_update_tasks_root_domain(),
> +				 * dl_servers contribution in function
> +				 * dl_restore_server_root_domain().
>   				 */
>   				rd = cpu_rq(cpumask_any(doms_cur[i]))->rd;
> -				dl_clear_root_domain(rd);
> +				dl_clear_root_domain(rd, true);
>   				goto match1;
>   			}
>   		}

With my limited understanding of the deadline code, this change looks 
reasonable to me. dl_rebuild_rd_accounting() is a part of the cpuset 
code that is seldom touched. So I don't think this particular hunk will 
cause any merge conflict. So it can be carried in the tip tree.

Acked-by: Waiman Long <longman@redhat.com>

Re: [PATCH 1/2] sched/deadline: Restore dl_server bandwidth on non-destructive root domain changes

[Joel Fernandes]

> On Nov 13, 2024, at 9:57 PM, Juri Lelli <juri.lelli@redhat.com> wrote:
> 
> When root domain non-destructive changes (e.g., only modifying one of
> the existing root domains while the rest is not touched) happen we still
> need to clear DEADLINE bandwidth accounting so that it's then properly
> restore taking into account DEADLINE tasks associated to each cpuset
> (associated to each root domain). After the introduction of dl_servers,
> we fail to restore such servers contribution after non-destructive
> changes (as they are only considered on destructive changes when
> runqueues are attached to the new domains).
> 
> Fix this by making sure we iterate over the dl_server attached to
> domains that have not been destroyed and add them bandwidth contribution
> back correctly.
> 
> Signed-off-by: Juri Lelli <juri.lelli@redhat.com>

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Thanks,

 - Joel




> ---
> include/linux/sched/deadline.h |  2 +-
> kernel/cgroup/cpuset.c         |  2 +-
> kernel/sched/deadline.c        | 18 +++++++++++++-----
> kernel/sched/topology.c        | 10 ++++++----
> 4 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/sched/deadline.h b/include/linux/sched/deadline.h
> index 3a912ab42bb5..82c966a55856 100644
> --- a/include/linux/sched/deadline.h
> +++ b/include/linux/sched/deadline.h
> @@ -33,7 +33,7 @@ static inline bool dl_time_before(u64 a, u64 b)
> 
> struct root_domain;
> extern void dl_add_task_root_domain(struct task_struct *p);
> -extern void dl_clear_root_domain(struct root_domain *rd);
> +extern void dl_clear_root_domain(struct root_domain *rd, bool restore);
> 
> #endif /* CONFIG_SMP */
> 
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 142303abb055..4d3603a99db3 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -954,7 +954,7 @@ static void dl_rebuild_rd_accounting(void)
>     * Clear default root domain DL accounting, it will be computed again
>     * if a task belongs to it.
>     */
> -    dl_clear_root_domain(&def_root_domain);
> +    dl_clear_root_domain(&def_root_domain, false);
> 
>    cpuset_for_each_descendant_pre(cs, pos_css, &top_cpuset) {
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 9ce93d0bf452..e53208a50279 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -2968,13 +2968,21 @@ void dl_add_task_root_domain(struct task_struct *p)
>    task_rq_unlock(rq, p, &rf);
> }
> 
> -void dl_clear_root_domain(struct root_domain *rd)
> +void dl_clear_root_domain(struct root_domain *rd, bool restore)
> {
> -    unsigned long flags;
> -
> -    raw_spin_lock_irqsave(&rd->dl_bw.lock, flags);
> +    guard(raw_spinlock_irqsave)(&rd->dl_bw.lock);
>    rd->dl_bw.total_bw = 0;
> -    raw_spin_unlock_irqrestore(&rd->dl_bw.lock, flags);
> +
> +    if (restore) {
> +        int i;
> +
> +        for_each_cpu(i, rd->span) {
> +            struct sched_dl_entity *dl_se = &cpu_rq(i)->fair_server;
> +
> +            if (dl_server(dl_se))
> +                rd->dl_bw.total_bw += dl_se->dl_bw;
> +        }
> +    }
> }
> 
> #endif /* CONFIG_SMP */
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 9748a4c8d668..e9e7a7c43dd6 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -2721,12 +2721,14 @@ void partition_sched_domains_locked(int ndoms_new, cpumask_var_t doms_new[],
> 
>                /*
>                 * This domain won't be destroyed and as such
> -                 * its dl_bw->total_bw needs to be cleared.  It
> -                 * will be recomputed in function
> -                 * update_tasks_root_domain().
> +                 * its dl_bw->total_bw needs to be cleared.
> +                 * Tasks contribution will be then recomputed
> +                 * in function dl_update_tasks_root_domain(),
> +                 * dl_servers contribution in function
> +                 * dl_restore_server_root_domain().
>                 */
>                rd = cpu_rq(cpumask_any(doms_cur[i]))->rd;
> -                dl_clear_root_domain(rd);
> +                dl_clear_root_domain(rd, true);
>                goto match1;
>            }
>        }
> --
> 2.47.0
> 

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

[Juri Lelli]

For hotplug operations, DEADLINE needs to check that there is still enough
bandwidth left after removing the CPU that is going offline. We however
fail to do so currently.

Restore the correct behavior by restructuring dl_bw_manage() a bit, so
that overflow conditions (not enough bandwidth left) are properly
checked. Also account for dl_server bandwidth, i.e. discount such
bandwidht in the calculation since NORMAL tasks will be anyway moved
away from the CPU as a result of the hotplug operation.

Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
---
 kernel/sched/core.c     |  2 +-
 kernel/sched/deadline.c | 33 ++++++++++++++++++++++++---------
 kernel/sched/sched.h    |  2 +-
 3 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 43e453ab7e20..d1049e784510 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8057,7 +8057,7 @@ static void cpuset_cpu_active(void)
 static int cpuset_cpu_inactive(unsigned int cpu)
 {
 	if (!cpuhp_tasks_frozen) {
-		int ret = dl_bw_check_overflow(cpu);
+		int ret = dl_bw_deactivate(cpu);
 
 		if (ret)
 			return ret;
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index e53208a50279..609685c5df05 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -3467,29 +3467,31 @@ int dl_cpuset_cpumask_can_shrink(const struct cpumask *cur,
 }
 
 enum dl_bw_request {
-	dl_bw_req_check_overflow = 0,
+	dl_bw_req_deactivate = 0,
 	dl_bw_req_alloc,
 	dl_bw_req_free
 };
 
 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;
 
 	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.
@@ -3498,6 +3500,19 @@ 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;
+
+		overflow = __dl_overflow(dl_b, cap, fair_server_bw, 0);
+		break;
 	}
 
 	raw_spin_unlock_irqrestore(&dl_b->lock, flags);
@@ -3506,9 +3521,9 @@ static int dl_bw_manage(enum dl_bw_request req, int cpu, u64 dl_bw)
 	return overflow ? -EBUSY : 0;
 }
 
-int dl_bw_check_overflow(int cpu)
+int dl_bw_deactivate(int cpu)
 {
-	return dl_bw_manage(dl_bw_req_check_overflow, cpu, 0);
+	return dl_bw_manage(dl_bw_req_deactivate, cpu, 0);
 }
 
 int dl_bw_alloc(int cpu, u64 dl_bw)
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b1c3588a8f00..1fee840f1bab 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -362,7 +362,7 @@ extern void __getparam_dl(struct task_struct *p, struct sched_attr *attr);
 extern bool __checkparam_dl(const struct sched_attr *attr);
 extern bool dl_param_changed(struct task_struct *p, const struct sched_attr *attr);
 extern int  dl_cpuset_cpumask_can_shrink(const struct cpumask *cur, const struct cpumask *trial);
-extern int  dl_bw_check_overflow(int cpu);
+extern int  dl_bw_deactivate(int cpu);
 extern s64 dl_scaled_delta_exec(struct rq *rq, struct sched_dl_entity *dl_se, s64 delta_exec);
 /*
  * SCHED_DEADLINE supports servers (nested scheduling) with the following
-- 
2.47.0

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

[Phil Auld]

Hi Juri,

On Wed, Nov 13, 2024 at 12:57:23PM +0000 Juri Lelli wrote:
> For hotplug operations, DEADLINE needs to check that there is still enough
> bandwidth left after removing the CPU that is going offline. We however
> fail to do so currently.
> 
> Restore the correct behavior by restructuring dl_bw_manage() a bit, so
> that overflow conditions (not enough bandwidth left) are properly
> checked. Also account for dl_server bandwidth, i.e. discount such
> bandwidht in the calculation since NORMAL tasks will be anyway moved

"bandwidth"  :)


> away from the CPU as a result of the hotplug operation.
>

LGTM.

Reviewed-by: Phil Auld <pauld@redhat.com>


> Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
> ---
>  kernel/sched/core.c     |  2 +-
>  kernel/sched/deadline.c | 33 ++++++++++++++++++++++++---------
>  kernel/sched/sched.h    |  2 +-
>  3 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 43e453ab7e20..d1049e784510 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8057,7 +8057,7 @@ static void cpuset_cpu_active(void)
>  static int cpuset_cpu_inactive(unsigned int cpu)
>  {
>  	if (!cpuhp_tasks_frozen) {
> -		int ret = dl_bw_check_overflow(cpu);
> +		int ret = dl_bw_deactivate(cpu);
>  
>  		if (ret)
>  			return ret;
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index e53208a50279..609685c5df05 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -3467,29 +3467,31 @@ int dl_cpuset_cpumask_can_shrink(const struct cpumask *cur,
>  }
>  
>  enum dl_bw_request {
> -	dl_bw_req_check_overflow = 0,
> +	dl_bw_req_deactivate = 0,
>  	dl_bw_req_alloc,
>  	dl_bw_req_free
>  };
>  
>  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;
>  
>  	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.
> @@ -3498,6 +3500,19 @@ 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;
> +
> +		overflow = __dl_overflow(dl_b, cap, fair_server_bw, 0);
> +		break;
>  	}
>  
>  	raw_spin_unlock_irqrestore(&dl_b->lock, flags);
> @@ -3506,9 +3521,9 @@ static int dl_bw_manage(enum dl_bw_request req, int cpu, u64 dl_bw)
>  	return overflow ? -EBUSY : 0;
>  }
>  
> -int dl_bw_check_overflow(int cpu)
> +int dl_bw_deactivate(int cpu)
>  {
> -	return dl_bw_manage(dl_bw_req_check_overflow, cpu, 0);
> +	return dl_bw_manage(dl_bw_req_deactivate, cpu, 0);
>  }
>  
>  int dl_bw_alloc(int cpu, u64 dl_bw)
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index b1c3588a8f00..1fee840f1bab 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -362,7 +362,7 @@ extern void __getparam_dl(struct task_struct *p, struct sched_attr *attr);
>  extern bool __checkparam_dl(const struct sched_attr *attr);
>  extern bool dl_param_changed(struct task_struct *p, const struct sched_attr *attr);
>  extern int  dl_cpuset_cpumask_can_shrink(const struct cpumask *cur, const struct cpumask *trial);
> -extern int  dl_bw_check_overflow(int cpu);
> +extern int  dl_bw_deactivate(int cpu);
>  extern s64 dl_scaled_delta_exec(struct rq *rq, struct sched_dl_entity *dl_se, s64 delta_exec);
>  /*
>   * SCHED_DEADLINE supports servers (nested scheduling) with the following
> -- 
> 2.47.0
> 
> 

-- 

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

[Juri Lelli]

Hi Phil,

On 13/11/24 08:49, Phil Auld wrote:
> 
> Hi Juri,
> 
> On Wed, Nov 13, 2024 at 12:57:23PM +0000 Juri Lelli wrote:
> > For hotplug operations, DEADLINE needs to check that there is still enough
> > bandwidth left after removing the CPU that is going offline. We however
> > fail to do so currently.
> > 
> > Restore the correct behavior by restructuring dl_bw_manage() a bit, so
> > that overflow conditions (not enough bandwidth left) are properly
> > checked. Also account for dl_server bandwidth, i.e. discount such
> > bandwidht in the calculation since NORMAL tasks will be anyway moved
> 
> "bandwidth"  :)

Grrrr. :)

> 
> 
> > away from the CPU as a result of the hotplug operation.
> >
> 
> LGTM.
> 
> Reviewed-by: Phil Auld <pauld@redhat.com>

Thanks!
Juri

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

[Phil Auld]

On Wed, Nov 13, 2024 at 02:58:37PM +0000 Juri Lelli wrote:
> Hi Phil,
> 
> On 13/11/24 08:49, Phil Auld wrote:
> > 
> > Hi Juri,
> > 
> > On Wed, Nov 13, 2024 at 12:57:23PM +0000 Juri Lelli wrote:
> > > For hotplug operations, DEADLINE needs to check that there is still enough
> > > bandwidth left after removing the CPU that is going offline. We however
> > > fail to do so currently.
> > > 
> > > Restore the correct behavior by restructuring dl_bw_manage() a bit, so
> > > that overflow conditions (not enough bandwidth left) are properly
> > > checked. Also account for dl_server bandwidth, i.e. discount such
> > > bandwidht in the calculation since NORMAL tasks will be anyway moved
> > 
> > "bandwidth"  :)
> 
> Grrrr. :)
>

Yeah, those are just minor nits.   Maybe Peter can fix them on the
way by...


Cheers,
Phil

> > 
> > 
> > > away from the CPU as a result of the hotplug operation.
> > >
> > 
> > LGTM.
> > 
> > Reviewed-by: Phil Auld <pauld@redhat.com>
> 
> Thanks!
> Juri
> 

-- 

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

[Waiman Long]

On 11/13/24 7:57 AM, Juri Lelli wrote:
> For hotplug operations, DEADLINE needs to check that there is still enough
> bandwidth left after removing the CPU that is going offline. We however
> fail to do so currently.
>
> Restore the correct behavior by restructuring dl_bw_manage() a bit, so
> that overflow conditions (not enough bandwidth left) are properly
> checked. Also account for dl_server bandwidth, i.e. discount such
> bandwidht in the calculation since NORMAL tasks will be anyway moved
> away from the CPU as a result of the hotplug operation.
>
> Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
> ---
>   kernel/sched/core.c     |  2 +-
>   kernel/sched/deadline.c | 33 ++++++++++++++++++++++++---------
>   kernel/sched/sched.h    |  2 +-
>   3 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 43e453ab7e20..d1049e784510 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8057,7 +8057,7 @@ static void cpuset_cpu_active(void)
>   static int cpuset_cpu_inactive(unsigned int cpu)
>   {
>   	if (!cpuhp_tasks_frozen) {
> -		int ret = dl_bw_check_overflow(cpu);
> +		int ret = dl_bw_deactivate(cpu);
>   
>   		if (ret)
>   			return ret;
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index e53208a50279..609685c5df05 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -3467,29 +3467,31 @@ int dl_cpuset_cpumask_can_shrink(const struct cpumask *cur,
>   }
>   
>   enum dl_bw_request {
> -	dl_bw_req_check_overflow = 0,
> +	dl_bw_req_deactivate = 0,
>   	dl_bw_req_alloc,
>   	dl_bw_req_free
>   };
>   
>   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;
>   
>   	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.
> @@ -3498,6 +3500,19 @@ 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;
> +
> +		overflow = __dl_overflow(dl_b, cap, fair_server_bw, 0);
> +		break;

This part can still cause a failure in one of test cases in my cpuset 
partition test script. In this particular case, the CPU to be offlined 
is an isolated CPU with scheduling disabled. As a result, total_bw is 0 
and the __dl_overflow() test failed. Is there a way to skip the 
__dl_overflow() test for isolated CPUs? Can we use a null total_bw as a 
proxy for that?

Thanks,
Longman

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

[Juri Lelli]

On 13/11/24 11:06, Waiman Long wrote:

...

> This part can still cause a failure in one of test cases in my cpuset
> partition test script. In this particular case, the CPU to be offlined is an
> isolated CPU with scheduling disabled. As a result, total_bw is 0 and the
> __dl_overflow() test failed. Is there a way to skip the __dl_overflow() test
> for isolated CPUs? Can we use a null total_bw as a proxy for that?

Can you please share the repro script? Would like to check locally what
is going on.

Thanks!
Juri

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

[Waiman Long]

On 11/13/24 11:40 AM, Juri Lelli wrote:
> On 13/11/24 11:06, Waiman Long wrote:
>
> ...
>
>> This part can still cause a failure in one of test cases in my cpuset
>> partition test script. In this particular case, the CPU to be offlined is an
>> isolated CPU with scheduling disabled. As a result, total_bw is 0 and the
>> __dl_overflow() test failed. Is there a way to skip the __dl_overflow() test
>> for isolated CPUs? Can we use a null total_bw as a proxy for that?
> Can you please share the repro script? Would like to check locally what
> is going on.

Just run tools/testing/selftests/cgroup/test_cpuset_prs.sh.

Cheers,
Longman

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

[Waiman Long]

On 11/13/24 11:42 AM, Waiman Long wrote:
>
> On 11/13/24 11:40 AM, Juri Lelli wrote:
>> On 13/11/24 11:06, Waiman Long wrote:
>>
>> ...
>>
>>> This part can still cause a failure in one of test cases in my cpuset
>>> partition test script. In this particular case, the CPU to be 
>>> offlined is an
>>> isolated CPU with scheduling disabled. As a result, total_bw is 0 
>>> and the
>>> __dl_overflow() test failed. Is there a way to skip the 
>>> __dl_overflow() test
>>> for isolated CPUs? Can we use a null total_bw as a proxy for that?
>> Can you please share the repro script? Would like to check locally what
>> is going on.
>
> Just run tools/testing/selftests/cgroup/test_cpuset_prs.sh.

The failing test is

         # Remote partition offline tests
         " C0-3:S+ C1-3:S+ C2-3     .    X2-3   X2-3 X2-3:P2:O2=0 .   0 
A1:0-1,A2:1,A3:3 A1:P0,A3:P2 2-3"

You can remove all the previous lines in the TEST_MATRIX to get to 
failed test case immediately eliminating unnecessary noise in your testing.

Cheers,
Longman

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

[Juri Lelli]

On 13/11/24 11:50, Waiman Long wrote:
> 
> On 11/13/24 11:42 AM, Waiman Long wrote:
> > 
> > On 11/13/24 11:40 AM, Juri Lelli wrote:
> > > On 13/11/24 11:06, Waiman Long wrote:
> > > 
> > > ...
> > > 
> > > > This part can still cause a failure in one of test cases in my cpuset
> > > > partition test script. In this particular case, the CPU to be
> > > > offlined is an
> > > > isolated CPU with scheduling disabled. As a result, total_bw is
> > > > 0 and the
> > > > __dl_overflow() test failed. Is there a way to skip the
> > > > __dl_overflow() test
> > > > for isolated CPUs? Can we use a null total_bw as a proxy for that?
> > > Can you please share the repro script? Would like to check locally what
> > > is going on.
> > 
> > Just run tools/testing/selftests/cgroup/test_cpuset_prs.sh.
> 
> The failing test is
> 
>         # Remote partition offline tests
>         " C0-3:S+ C1-3:S+ C2-3     .    X2-3   X2-3 X2-3:P2:O2=0 .   0
> A1:0-1,A2:1,A3:3 A1:P0,A3:P2 2-3"
> 
> You can remove all the previous lines in the TEST_MATRIX to get to failed
> test case immediately eliminating unnecessary noise in your testing.

So, IIUC this test is doing the following

# echo +cpuset >cgroup/cgroup.subtree_control
# mkdir cgroup/A1
# echo 0-3 >cgroup/A1/cpuset.cpus
# echo +cpuset >cgroup/A1/cgroup.subtree_control
# mkdir cgroup/A1/A2
# echo 1-3 >cgroup/A1/A2/cpuset.cpus
# echo +cpuset >cgroup/A1/A2/cgroup.subtree_control
# mkdir cgroup/A1/A2/A3
# echo 2-3 >cgroup/A1/A2/A3/cpuset.cpus
# echo 2-3 >cgroup/A1/cpuset.cpus.exclusive
# echo 2-3 >cgroup/A1/A2/cpuset.cpus.exclusive
# echo 2-3 >cgroup/A1/A2/A3/cpuset.cpus.exclusive
# echo isolated >cgroup/A1/A2/A3/cpuset.cpus.partition

With the last command, we get to one root domain with span: 0-1,4-7 (in
my setup with 8 CPUs) and no root domain for 2,3, since they are
isolated.

The test then tries to hotplug CPU 2, but fails to do so and so the
reported error.

total_bw for CPU 2 and CPU 3 is indeed 0, and I guess we could special
case this as you suggest (nothing to really worry about if we don't have
DEADLINE tasks affined to these CPUs). But I would have expected the
fair server contribution to still show up in total_bw, so this is
something a need to check.

Thanks,
Juri

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

[Waiman Long]

On 11/13/24 1:11 PM, Juri Lelli wrote:
> On 13/11/24 11:50, Waiman Long wrote:
>> On 11/13/24 11:42 AM, Waiman Long wrote:
>>> On 11/13/24 11:40 AM, Juri Lelli wrote:
>>>> On 13/11/24 11:06, Waiman Long wrote:
>>>>
>>>> ...
>>>>
>>>>> This part can still cause a failure in one of test cases in my cpuset
>>>>> partition test script. In this particular case, the CPU to be
>>>>> offlined is an
>>>>> isolated CPU with scheduling disabled. As a result, total_bw is
>>>>> 0 and the
>>>>> __dl_overflow() test failed. Is there a way to skip the
>>>>> __dl_overflow() test
>>>>> for isolated CPUs? Can we use a null total_bw as a proxy for that?
>>>> Can you please share the repro script? Would like to check locally what
>>>> is going on.
>>> Just run tools/testing/selftests/cgroup/test_cpuset_prs.sh.
>> The failing test is
>>
>>          # Remote partition offline tests
>>          " C0-3:S+ C1-3:S+ C2-3     .    X2-3   X2-3 X2-3:P2:O2=0 .   0
>> A1:0-1,A2:1,A3:3 A1:P0,A3:P2 2-3"
>>
>> You can remove all the previous lines in the TEST_MATRIX to get to failed
>> test case immediately eliminating unnecessary noise in your testing.
> So, IIUC this test is doing the following
>
> # echo +cpuset >cgroup/cgroup.subtree_control
> # mkdir cgroup/A1
> # echo 0-3 >cgroup/A1/cpuset.cpus
> # echo +cpuset >cgroup/A1/cgroup.subtree_control
> # mkdir cgroup/A1/A2
> # echo 1-3 >cgroup/A1/A2/cpuset.cpus
> # echo +cpuset >cgroup/A1/A2/cgroup.subtree_control
> # mkdir cgroup/A1/A2/A3
> # echo 2-3 >cgroup/A1/A2/A3/cpuset.cpus
> # echo 2-3 >cgroup/A1/cpuset.cpus.exclusive
> # echo 2-3 >cgroup/A1/A2/cpuset.cpus.exclusive
> # echo 2-3 >cgroup/A1/A2/A3/cpuset.cpus.exclusive
> # echo isolated >cgroup/A1/A2/A3/cpuset.cpus.partition
>
> With the last command, we get to one root domain with span: 0-1,4-7 (in
> my setup with 8 CPUs) and no root domain for 2,3, since they are
> isolated.
>
> The test then tries to hotplug CPU 2, but fails to do so and so the
> reported error.
>
> total_bw for CPU 2 and CPU 3 is indeed 0, and I guess we could special
> case this as you suggest (nothing to really worry about if we don't have
> DEADLINE tasks affined to these CPUs). But I would have expected the
> fair server contribution to still show up in total_bw, so this is
> something a need to check.

Thanks for looking into this. So the test script does create a lot of 
different corner cases to test the correctness of the cpuset partition 
code. Hopefully that will help you to improve the DL code to better 
handle these corner cases.

Cheers,
Longman