[PATCH 1/2] sched/dlserver: flag to represent active status of dlserver

[PATCH 1/2] sched/dlserver: flag to represent active status of dlserver

[Vineeth Pillai (Google)]

dlserver can get dequeued during a dlserver pick_task due to the delayed
deueue feature and this can lead to issues with dlserver logic as it
still thinks that dlserver is on the runqueue. The dlserver throttling
and replenish logic gets confused and can lead to double enqueue of
dlserver.

Double enqueue of dlserver could happend due to couple of reasons:

Case 1
------

Delayed dequeue feature[1] can cause dlserver being stopped during a
pick initiated by dlserver:
  __pick_next_task
   pick_task_dl -> server_pick_task
    pick_task_fair
     pick_next_entity (if (sched_delayed))
      dequeue_entities
       dl_server_stop

server_pick_task goes ahead with update_curr_dl_se without knowing that
dlserver is dequeued and this confuses the logic and may lead to
unintended enqueue while the server is stopped.

Case 2
------
A race condition between a task dequeue on one cpu and same task's enqueue
on this cpu by a remote cpu while the lock is released causing dlserver
double enqueue.

One cpu would be in the schedule() and releasing RQ-lock:

current->state = TASK_INTERRUPTIBLE();
        schedule();
          deactivate_task()
            dl_stop_server();
          pick_next_task()
            pick_next_task_fair()
              sched_balance_newidle()
                rq_unlock(this_rq)

at which point another CPU can take our RQ-lock and do:

        try_to_wake_up()
          ttwu_queue()
            rq_lock()
            ...
            activate_task()
              dl_server_start() --> first enqueue
            wakeup_preempt() := check_preempt_wakeup_fair()
              update_curr()
                update_curr_task()
                  if (current->dl_server)
                    dl_server_update()
                      enqueue_dl_entity() --> second enqueue

This bug was not apparent as the enqueue in dl_server_start doesn't
usually happen because of the defer logic. But as a side effect of the
first case(dequeue during dlserver pick), dl_throttled and dl_yield will
be set and this causes the time accounting of dlserver to messup and
then leading to a enqueue in dl_server_start.

Have an explicit flag representing the status of dlserver to avoid the
confusion. This is set in dl_server_start and reset in dlserver_stop.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Vineeth Pillai (Google) <vineeth@bitbyteword.org>
---
 include/linux/sched.h   | 7 +++++++
 kernel/sched/deadline.c | 8 ++++++--
 kernel/sched/sched.h    | 5 +++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index d380bffee2ef..66b311fbd5d6 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -656,6 +656,12 @@ struct sched_dl_entity {
 	 * @dl_defer_armed tells if the deferrable server is waiting
 	 * for the replenishment timer to activate it.
 	 *
+	 * @dl_server_active tells if the dlserver is active(started).
+	 * dlserver is started on first cfs enqueue on an idle runqueue
+	 * and is stopped when a dequeue results in 0 cfs tasks on the
+	 * runqueue. In other words, dlserver is active only when cpu's
+	 * runqueue has atleast one cfs task.
+	 *
 	 * @dl_defer_running tells if the deferrable server is actually
 	 * running, skipping the defer phase.
 	 */
@@ -664,6 +670,7 @@ struct sched_dl_entity {
 	unsigned int			dl_non_contending : 1;
 	unsigned int			dl_overrun	  : 1;
 	unsigned int			dl_server         : 1;
+	unsigned int			dl_server_active  : 1;
 	unsigned int			dl_defer	  : 1;
 	unsigned int			dl_defer_armed	  : 1;
 	unsigned int			dl_defer_running  : 1;
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 33b4646f8b24..0abf14ac5ca7 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1667,6 +1667,7 @@ void dl_server_start(struct sched_dl_entity *dl_se)
 	if (!dl_se->dl_runtime)
 		return;
 
+	dl_se->dl_server_active = 1;
 	enqueue_dl_entity(dl_se, ENQUEUE_WAKEUP);
 	if (!dl_task(dl_se->rq->curr) || dl_entity_preempt(dl_se, &rq->curr->dl))
 		resched_curr(dl_se->rq);
@@ -1681,6 +1682,7 @@ void dl_server_stop(struct sched_dl_entity *dl_se)
 	hrtimer_try_to_cancel(&dl_se->dl_timer);
 	dl_se->dl_defer_armed = 0;
 	dl_se->dl_throttled = 0;
+	dl_se->dl_server_active = 0;
 }
 
 void dl_server_init(struct sched_dl_entity *dl_se, struct rq *rq,
@@ -2435,8 +2437,10 @@ static struct task_struct *__pick_task_dl(struct rq *rq)
 	if (dl_server(dl_se)) {
 		p = dl_se->server_pick_task(dl_se);
 		if (!p) {
-			dl_se->dl_yielded = 1;
-			update_curr_dl_se(rq, dl_se, 0);
+			if (dl_server_active(dl_se)) {
+				dl_se->dl_yielded = 1;
+				update_curr_dl_se(rq, dl_se, 0);
+			}
 			goto again;
 		}
 		rq->dl_server = dl_se;
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index aef716c41edb..65fa64845d9f 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -398,6 +398,11 @@ extern void __dl_server_attach_root(struct sched_dl_entity *dl_se, struct rq *rq
 extern int dl_server_apply_params(struct sched_dl_entity *dl_se,
 		    u64 runtime, u64 period, bool init);
 
+static inline bool dl_server_active(struct sched_dl_entity *dl_se)
+{
+	return dl_se->dl_server_active;
+}
+
 #ifdef CONFIG_CGROUP_SCHED
 
 extern struct list_head task_groups;
-- 
2.45.2

[PATCH 1/2] sched/dlserver: flag to represent active status of dlserver

[Vineeth Pillai (Google)]

dlserver can get dequeued during a dlserver pick_task due to the delayed
deueue feature and this can lead to issues with dlserver logic as it
still thinks that dlserver is on the runqueue. The dlserver throttling
and replenish logic gets confused and can lead to double enqueue of
dlserver.

Double enqueue of dlserver could happend due to couple of reasons:

Case 1
------

Delayed dequeue feature[1] can cause dlserver being stopped during a
pick initiated by dlserver:
  __pick_next_task
   pick_task_dl -> server_pick_task
    pick_task_fair
     pick_next_entity (if (sched_delayed))
      dequeue_entities
       dl_server_stop

server_pick_task goes ahead with update_curr_dl_se without knowing that
dlserver is dequeued and this confuses the logic and may lead to
unintended enqueue while the server is stopped.

Case 2
------
A race condition between a task dequeue on one cpu and same task's enqueue
on this cpu by a remote cpu while the lock is released causing dlserver
double enqueue.

One cpu would be in the schedule() and releasing RQ-lock:

current->state = TASK_INTERRUPTIBLE();
        schedule();
          deactivate_task()
            dl_stop_server();
          pick_next_task()
            pick_next_task_fair()
              sched_balance_newidle()
                rq_unlock(this_rq)

at which point another CPU can take our RQ-lock and do:

        try_to_wake_up()
          ttwu_queue()
            rq_lock()
            ...
            activate_task()
              dl_server_start() --> first enqueue
            wakeup_preempt() := check_preempt_wakeup_fair()
              update_curr()
                update_curr_task()
                  if (current->dl_server)
                    dl_server_update()
                      enqueue_dl_entity() --> second enqueue

This bug was not apparent as the enqueue in dl_server_start doesn't
usually happen because of the defer logic. But as a side effect of the
first case(dequeue during dlserver pick), dl_throttled and dl_yield will
be set and this causes the time accounting of dlserver to messup and
then leading to a enqueue in dl_server_start.

Have an explicit flag representing the status of dlserver to avoid the
confusion. This is set in dl_server_start and reset in dlserver_stop.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Vineeth Pillai (Google) <vineeth@bitbyteword.org>
---
 include/linux/sched.h   | 7 +++++++
 kernel/sched/deadline.c | 8 ++++++--
 kernel/sched/sched.h    | 5 +++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index d380bffee2ef..66b311fbd5d6 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -656,6 +656,12 @@ struct sched_dl_entity {
 	 * @dl_defer_armed tells if the deferrable server is waiting
 	 * for the replenishment timer to activate it.
 	 *
+	 * @dl_server_active tells if the dlserver is active(started).
+	 * dlserver is started on first cfs enqueue on an idle runqueue
+	 * and is stopped when a dequeue results in 0 cfs tasks on the
+	 * runqueue. In other words, dlserver is active only when cpu's
+	 * runqueue has atleast one cfs task.
+	 *
 	 * @dl_defer_running tells if the deferrable server is actually
 	 * running, skipping the defer phase.
 	 */
@@ -664,6 +670,7 @@ struct sched_dl_entity {
 	unsigned int			dl_non_contending : 1;
 	unsigned int			dl_overrun	  : 1;
 	unsigned int			dl_server         : 1;
+	unsigned int			dl_server_active  : 1;
 	unsigned int			dl_defer	  : 1;
 	unsigned int			dl_defer_armed	  : 1;
 	unsigned int			dl_defer_running  : 1;
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 33b4646f8b24..0abf14ac5ca7 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1667,6 +1667,7 @@ void dl_server_start(struct sched_dl_entity *dl_se)
 	if (!dl_se->dl_runtime)
 		return;
 
+	dl_se->dl_server_active = 1;
 	enqueue_dl_entity(dl_se, ENQUEUE_WAKEUP);
 	if (!dl_task(dl_se->rq->curr) || dl_entity_preempt(dl_se, &rq->curr->dl))
 		resched_curr(dl_se->rq);
@@ -1681,6 +1682,7 @@ void dl_server_stop(struct sched_dl_entity *dl_se)
 	hrtimer_try_to_cancel(&dl_se->dl_timer);
 	dl_se->dl_defer_armed = 0;
 	dl_se->dl_throttled = 0;
+	dl_se->dl_server_active = 0;
 }
 
 void dl_server_init(struct sched_dl_entity *dl_se, struct rq *rq,
@@ -2435,8 +2437,10 @@ static struct task_struct *__pick_task_dl(struct rq *rq)
 	if (dl_server(dl_se)) {
 		p = dl_se->server_pick_task(dl_se);
 		if (!p) {
-			dl_se->dl_yielded = 1;
-			update_curr_dl_se(rq, dl_se, 0);
+			if (dl_server_active(dl_se)) {
+				dl_se->dl_yielded = 1;
+				update_curr_dl_se(rq, dl_se, 0);
+			}
 			goto again;
 		}
 		rq->dl_server = dl_se;
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index aef716c41edb..65fa64845d9f 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -398,6 +398,11 @@ extern void __dl_server_attach_root(struct sched_dl_entity *dl_se, struct rq *rq
 extern int dl_server_apply_params(struct sched_dl_entity *dl_se,
 		    u64 runtime, u64 period, bool init);
 
+static inline bool dl_server_active(struct sched_dl_entity *dl_se)
+{
+	return dl_se->dl_server_active;
+}
+
 #ifdef CONFIG_CGROUP_SCHED
 
 extern struct list_head task_groups;
-- 
2.45.2

[PATCH 2/2] sched/dlserver: fix dlserver time accounting

[Vineeth Pillai (Google)]

dlserver time is accounted when:
- dlserver is active and the dlserver proxies the cfs task.
- dlserver is active but deferred and cfs task runs after being picked
  through the normal fair class pick.

dl_server_update is called in two places to make sure that both the
above times are accounted for. But it doesn't check if dlserver is
active or not. Now that we have this dl_server_active flag, we can
consolidate dl_server_update into one place and all we need to check is
whether dlserver is active or not. When dlserver is active there is only
two possible conditions:
- dlserver is deferred.
- cfs task is running on behalf of dlserver.

Signed-off-by: Vineeth Pillai (Google) <vineeth@bitbyteword.org>
---
 kernel/sched/fair.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 2d80aaac4413..f5329672815b 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1159,8 +1159,6 @@ static inline void update_curr_task(struct task_struct *p, s64 delta_exec)
 	trace_sched_stat_runtime(p, delta_exec);
 	account_group_exec_runtime(p, delta_exec);
 	cgroup_account_cputime(p, delta_exec);
-	if (p->dl_server)
-		dl_server_update(p->dl_server, delta_exec);
 }
 
 static inline bool did_preempt_short(struct cfs_rq *cfs_rq, struct sched_entity *curr)
@@ -1237,11 +1235,16 @@ static void update_curr(struct cfs_rq *cfs_rq)
 		update_curr_task(p, delta_exec);
 
 		/*
-		 * Any fair task that runs outside of fair_server should
-		 * account against fair_server such that it can account for
-		 * this time and possibly avoid running this period.
+		 * If the fair_server is active, we need to account for the
+		 * fair_server time whether or not the task is running on
+		 * behalf of fair_server or not:
+		 *  - If the task is running on behalf of fair_server, we need
+		 *    to limit its time based on the assigned runtime.
+		 *  - Fair task that runs outside of fair_server should account
+		 *    against fair_server such that it can account for this time
+		 *    and possibly avoid running this period.
 		 */
-		if (p->dl_server != &rq->fair_server)
+		if (dl_server_active(&rq->fair_server))
 			dl_server_update(&rq->fair_server, delta_exec);
 	}
 
-- 
2.45.2

Re: [PATCH 1/2] sched/dlserver: flag to represent active status of dlserver

[Marcel Ziswiler]

Thank you very much, Vineeth

On Thu, 2024-12-12 at 22:22 -0500, Vineeth Pillai (Google) wrote:
> dlserver can get dequeued during a dlserver pick_task due to the delayed
> deueue feature and this can lead to issues with dlserver logic as it
> still thinks that dlserver is on the runqueue. The dlserver throttling
> and replenish logic gets confused and can lead to double enqueue of
> dlserver.
> 
> Double enqueue of dlserver could happend due to couple of reasons:
> 
> Case 1
> ------
> 
> Delayed dequeue feature[1] can cause dlserver being stopped during a
> pick initiated by dlserver:
>   __pick_next_task
>    pick_task_dl -> server_pick_task
>     pick_task_fair
>      pick_next_entity (if (sched_delayed))
>       dequeue_entities
>        dl_server_stop
> 
> server_pick_task goes ahead with update_curr_dl_se without knowing that
> dlserver is dequeued and this confuses the logic and may lead to
> unintended enqueue while the server is stopped.
> 
> Case 2
> ------
> A race condition between a task dequeue on one cpu and same task's enqueue
> on this cpu by a remote cpu while the lock is released causing dlserver
> double enqueue.
> 
> One cpu would be in the schedule() and releasing RQ-lock:
> 
> current->state = TASK_INTERRUPTIBLE();
>         schedule();
>           deactivate_task()
>             dl_stop_server();
>           pick_next_task()
>             pick_next_task_fair()
>               sched_balance_newidle()
>                 rq_unlock(this_rq)
> 
> at which point another CPU can take our RQ-lock and do:
> 
>         try_to_wake_up()
>           ttwu_queue()
>             rq_lock()
>             ...
>             activate_task()
>               dl_server_start() --> first enqueue
>             wakeup_preempt() := check_preempt_wakeup_fair()
>               update_curr()
>                 update_curr_task()
>                   if (current->dl_server)
>                     dl_server_update()
>                       enqueue_dl_entity() --> second enqueue
> 
> This bug was not apparent as the enqueue in dl_server_start doesn't
> usually happen because of the defer logic. But as a side effect of the
> first case(dequeue during dlserver pick), dl_throttled and dl_yield will
> be set and this causes the time accounting of dlserver to messup and
> then leading to a enqueue in dl_server_start.
> 
> Have an explicit flag representing the status of dlserver to avoid the
> confusion. This is set in dl_server_start and reset in dlserver_stop.
> 
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Vineeth Pillai (Google) <vineeth@bitbyteword.org>

Tested-by: Marcel Ziswiler <marcel.ziswiler@codethink.co.uk> # ROCK 5B

> ---
>  include/linux/sched.h   | 7 +++++++
>  kernel/sched/deadline.c | 8 ++++++--
>  kernel/sched/sched.h    | 5 +++++
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d380bffee2ef..66b311fbd5d6 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -656,6 +656,12 @@ struct sched_dl_entity {
>  	 * @dl_defer_armed tells if the deferrable server is waiting
>  	 * for the replenishment timer to activate it.
>  	 *
> +	 * @dl_server_active tells if the dlserver is active(started).
> +	 * dlserver is started on first cfs enqueue on an idle runqueue
> +	 * and is stopped when a dequeue results in 0 cfs tasks on the
> +	 * runqueue. In other words, dlserver is active only when cpu's
> +	 * runqueue has atleast one cfs task.
> +	 *
>  	 * @dl_defer_running tells if the deferrable server is actually
>  	 * running, skipping the defer phase.
>  	 */
> @@ -664,6 +670,7 @@ struct sched_dl_entity {
>  	unsigned int			dl_non_contending : 1;
>  	unsigned int			dl_overrun	  : 1;
>  	unsigned int			dl_server         : 1;
> +	unsigned int			dl_server_active  : 1;
>  	unsigned int			dl_defer	  : 1;
>  	unsigned int			dl_defer_armed	  : 1;
>  	unsigned int			dl_defer_running  : 1;
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 33b4646f8b24..0abf14ac5ca7 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -1667,6 +1667,7 @@ void dl_server_start(struct sched_dl_entity *dl_se)
>  	if (!dl_se->dl_runtime)
>  		return;
>  
> +	dl_se->dl_server_active = 1;
>  	enqueue_dl_entity(dl_se, ENQUEUE_WAKEUP);
>  	if (!dl_task(dl_se->rq->curr) || dl_entity_preempt(dl_se, &rq->curr->dl))
>  		resched_curr(dl_se->rq);
> @@ -1681,6 +1682,7 @@ void dl_server_stop(struct sched_dl_entity *dl_se)
>  	hrtimer_try_to_cancel(&dl_se->dl_timer);
>  	dl_se->dl_defer_armed = 0;
>  	dl_se->dl_throttled = 0;
> +	dl_se->dl_server_active = 0;
>  }
>  
>  void dl_server_init(struct sched_dl_entity *dl_se, struct rq *rq,
> @@ -2435,8 +2437,10 @@ static struct task_struct *__pick_task_dl(struct rq *rq)
>  	if (dl_server(dl_se)) {
>  		p = dl_se->server_pick_task(dl_se);
>  		if (!p) {
> -			dl_se->dl_yielded = 1;
> -			update_curr_dl_se(rq, dl_se, 0);
> +			if (dl_server_active(dl_se)) {
> +				dl_se->dl_yielded = 1;
> +				update_curr_dl_se(rq, dl_se, 0);
> +			}
>  			goto again;
>  		}
>  		rq->dl_server = dl_se;
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index aef716c41edb..65fa64845d9f 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -398,6 +398,11 @@ extern void __dl_server_attach_root(struct sched_dl_entity *dl_se, struct rq *rq
>  extern int dl_server_apply_params(struct sched_dl_entity *dl_se,
>  		    u64 runtime, u64 period, bool init);
>  
> +static inline bool dl_server_active(struct sched_dl_entity *dl_se)
> +{
> +	return dl_se->dl_server_active;
> +}
> +
>  #ifdef CONFIG_CGROUP_SCHED
>  
>  extern struct list_head task_groups;

Cheers

Marcel

Re: [PATCH 2/2] sched/dlserver: fix dlserver time accounting

[Marcel Ziswiler]

Thank you very much, Vineeth

On Thu, 2024-12-12 at 22:22 -0500, Vineeth Pillai (Google) wrote:
> dlserver time is accounted when:
> - dlserver is active and the dlserver proxies the cfs task.
> - dlserver is active but deferred and cfs task runs after being picked
>   through the normal fair class pick.
> 
> dl_server_update is called in two places to make sure that both the
> above times are accounted for. But it doesn't check if dlserver is
> active or not. Now that we have this dl_server_active flag, we can
> consolidate dl_server_update into one place and all we need to check is
> whether dlserver is active or not. When dlserver is active there is only
> two possible conditions:
> - dlserver is deferred.
> - cfs task is running on behalf of dlserver.
> 
> Signed-off-by: Vineeth Pillai (Google) <vineeth@bitbyteword.org>

Tested-by: Marcel Ziswiler <marcel.ziswiler@codethink.co.uk> # ROCK 5B

> ---
>  kernel/sched/fair.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 2d80aaac4413..f5329672815b 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -1159,8 +1159,6 @@ static inline void update_curr_task(struct task_struct *p, s64 delta_exec)
>  	trace_sched_stat_runtime(p, delta_exec);
>  	account_group_exec_runtime(p, delta_exec);
>  	cgroup_account_cputime(p, delta_exec);
> -	if (p->dl_server)
> -		dl_server_update(p->dl_server, delta_exec);
>  }
>  
>  static inline bool did_preempt_short(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> @@ -1237,11 +1235,16 @@ static void update_curr(struct cfs_rq *cfs_rq)
>  		update_curr_task(p, delta_exec);
>  
>  		/*
> -		 * Any fair task that runs outside of fair_server should
> -		 * account against fair_server such that it can account for
> -		 * this time and possibly avoid running this period.
> +		 * If the fair_server is active, we need to account for the
> +		 * fair_server time whether or not the task is running on
> +		 * behalf of fair_server or not:
> +		 *  - If the task is running on behalf of fair_server, we need
> +		 *    to limit its time based on the assigned runtime.
> +		 *  - Fair task that runs outside of fair_server should account
> +		 *    against fair_server such that it can account for this time
> +		 *    and possibly avoid running this period.
>  		 */
> -		if (p->dl_server != &rq->fair_server)
> +		if (dl_server_active(&rq->fair_server))
>  			dl_server_update(&rq->fair_server, delta_exec);
>  	}

Cheers

Marcel

Re: [PATCH 1/2] sched/dlserver: flag to represent active status of dlserver

[Peter Zijlstra]

Thanks, I've invented a Fixes tag for them and stuck them in
queue/sched/urgent for the robot (although I don't expect many
complaints from it).

I'll graduate them to tip/sched/urgent if nothing comes up.

Re: [PATCH 1/2] sched/dlserver: flag to represent active status of dlserver

[Ilya Maximets]

On 12/13/24 13:51, Peter Zijlstra wrote:
> 
> 
> Thanks, I've invented a Fixes tag for them and stuck them in
> queue/sched/urgent for the robot (although I don't expect many
> complaints from it).
> 
> I'll graduate them to tip/sched/urgent if nothing comes up.

Thanks, Vineeth and Peter!

FWIW, I've been running the version of the patch from the discussion
thread for 50+ hours and didn't have any issues.

Now running these exact patches for the past 5 hours and didn't have
any issues so far as well.

So, even if a little late:

Tested-by: Ilya Maximets <i.maximets@ovn.org>

Re: [PATCH 1/2] sched/dlserver: flag to represent active status of dlserver

[Juri Lelli]

On 13/12/24 13:51, Peter Zijlstra wrote:
> 
> 
> Thanks, I've invented a Fixes tag for them and stuck them in
> queue/sched/urgent for the robot (although I don't expect many
> complaints from it).
> 
> I'll graduate them to tip/sched/urgent if nothing comes up.
> 

Thanks a lot Peter and Vineeth for working on this! The changes look
good to me so feel free to also add my

Acked-by: Juri Lelli <juri.lelli@redhat.com>

At our end, Tomas should be testing them as we speak.

Best,
Juri

[tip: sched/urgent] sched/dlserver: Fix dlserver time accounting

[tip-bot2 for Vineeth Pillai (Google)]

The following commit has been merged into the sched/urgent branch of tip:

Commit-ID:     c7f7e9c73178e0e342486fd31e7f363ef60e3f83
Gitweb:        https://git.kernel.org/tip/c7f7e9c73178e0e342486fd31e7f363ef60e3f83
Author:        Vineeth Pillai (Google) <vineeth@bitbyteword.org>
AuthorDate:    Thu, 12 Dec 2024 22:22:37 -05:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Fri, 13 Dec 2024 12:57:35 +01:00

sched/dlserver: Fix dlserver time accounting

dlserver time is accounted when:
 - dlserver is active and the dlserver proxies the cfs task.
 - dlserver is active but deferred and cfs task runs after being picked
   through the normal fair class pick.

dl_server_update is called in two places to make sure that both the
above times are accounted for. But it doesn't check if dlserver is
active or not. Now that we have this dl_server_active flag, we can
consolidate dl_server_update into one place and all we need to check is
whether dlserver is active or not. When dlserver is active there is only
two possible conditions:
 - dlserver is deferred.
 - cfs task is running on behalf of dlserver.

Fixes: a110a81c52a9 ("sched/deadline: Deferrable dl server")
Signed-off-by: "Vineeth Pillai (Google)" <vineeth@bitbyteword.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Marcel Ziswiler <marcel.ziswiler@codethink.co.uk> # ROCK 5B
Link: https://lore.kernel.org/r/20241213032244.877029-2-vineeth@bitbyteword.org
---
 kernel/sched/fair.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 97ee48c..53a4f78 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1159,8 +1159,6 @@ static inline void update_curr_task(struct task_struct *p, s64 delta_exec)
 	trace_sched_stat_runtime(p, delta_exec);
 	account_group_exec_runtime(p, delta_exec);
 	cgroup_account_cputime(p, delta_exec);
-	if (p->dl_server)
-		dl_server_update(p->dl_server, delta_exec);
 }
 
 static inline bool did_preempt_short(struct cfs_rq *cfs_rq, struct sched_entity *curr)
@@ -1237,11 +1235,16 @@ static void update_curr(struct cfs_rq *cfs_rq)
 		update_curr_task(p, delta_exec);
 
 		/*
-		 * Any fair task that runs outside of fair_server should
-		 * account against fair_server such that it can account for
-		 * this time and possibly avoid running this period.
+		 * If the fair_server is active, we need to account for the
+		 * fair_server time whether or not the task is running on
+		 * behalf of fair_server or not:
+		 *  - If the task is running on behalf of fair_server, we need
+		 *    to limit its time based on the assigned runtime.
+		 *  - Fair task that runs outside of fair_server should account
+		 *    against fair_server such that it can account for this time
+		 *    and possibly avoid running this period.
 		 */
-		if (p->dl_server != &rq->fair_server)
+		if (dl_server_active(&rq->fair_server))
 			dl_server_update(&rq->fair_server, delta_exec);
 	}
 

[tip: sched/urgent] sched/dlserver: Fix dlserver double enqueue

[tip-bot2 for Vineeth Pillai (Google)]

The following commit has been merged into the sched/urgent branch of tip:

Commit-ID:     b53127db1dbf7f1047cf35c10922d801dcd40324
Gitweb:        https://git.kernel.org/tip/b53127db1dbf7f1047cf35c10922d801dcd40324
Author:        Vineeth Pillai (Google) <vineeth@bitbyteword.org>
AuthorDate:    Thu, 12 Dec 2024 22:22:36 -05:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Fri, 13 Dec 2024 12:57:34 +01:00

sched/dlserver: Fix dlserver double enqueue

dlserver can get dequeued during a dlserver pick_task due to the delayed
deueue feature and this can lead to issues with dlserver logic as it
still thinks that dlserver is on the runqueue. The dlserver throttling
and replenish logic gets confused and can lead to double enqueue of
dlserver.

Double enqueue of dlserver could happend due to couple of reasons:

Case 1
------

Delayed dequeue feature[1] can cause dlserver being stopped during a
pick initiated by dlserver:
  __pick_next_task
   pick_task_dl -> server_pick_task
    pick_task_fair
     pick_next_entity (if (sched_delayed))
      dequeue_entities
       dl_server_stop

server_pick_task goes ahead with update_curr_dl_se without knowing that
dlserver is dequeued and this confuses the logic and may lead to
unintended enqueue while the server is stopped.

Case 2
------
A race condition between a task dequeue on one cpu and same task's enqueue
on this cpu by a remote cpu while the lock is released causing dlserver
double enqueue.

One cpu would be in the schedule() and releasing RQ-lock:

current->state = TASK_INTERRUPTIBLE();
        schedule();
          deactivate_task()
            dl_stop_server();
          pick_next_task()
            pick_next_task_fair()
              sched_balance_newidle()
                rq_unlock(this_rq)

at which point another CPU can take our RQ-lock and do:

        try_to_wake_up()
          ttwu_queue()
            rq_lock()
            ...
            activate_task()
              dl_server_start() --> first enqueue
            wakeup_preempt() := check_preempt_wakeup_fair()
              update_curr()
                update_curr_task()
                  if (current->dl_server)
                    dl_server_update()
                      enqueue_dl_entity() --> second enqueue

This bug was not apparent as the enqueue in dl_server_start doesn't
usually happen because of the defer logic. But as a side effect of the
first case(dequeue during dlserver pick), dl_throttled and dl_yield will
be set and this causes the time accounting of dlserver to messup and
then leading to a enqueue in dl_server_start.

Have an explicit flag representing the status of dlserver to avoid the
confusion. This is set in dl_server_start and reset in dlserver_stop.

Fixes: 63ba8422f876 ("sched/deadline: Introduce deadline servers")
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: "Vineeth Pillai (Google)" <vineeth@bitbyteword.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Marcel Ziswiler <marcel.ziswiler@codethink.co.uk> # ROCK 5B
Link: https://lkml.kernel.org/r/20241213032244.877029-1-vineeth@bitbyteword.org
---
 include/linux/sched.h   | 7 +++++++
 kernel/sched/deadline.c | 8 ++++++--
 kernel/sched/sched.h    | 5 +++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index d380bff..66b311f 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -656,6 +656,12 @@ struct sched_dl_entity {
 	 * @dl_defer_armed tells if the deferrable server is waiting
 	 * for the replenishment timer to activate it.
 	 *
+	 * @dl_server_active tells if the dlserver is active(started).
+	 * dlserver is started on first cfs enqueue on an idle runqueue
+	 * and is stopped when a dequeue results in 0 cfs tasks on the
+	 * runqueue. In other words, dlserver is active only when cpu's
+	 * runqueue has atleast one cfs task.
+	 *
 	 * @dl_defer_running tells if the deferrable server is actually
 	 * running, skipping the defer phase.
 	 */
@@ -664,6 +670,7 @@ struct sched_dl_entity {
 	unsigned int			dl_non_contending : 1;
 	unsigned int			dl_overrun	  : 1;
 	unsigned int			dl_server         : 1;
+	unsigned int			dl_server_active  : 1;
 	unsigned int			dl_defer	  : 1;
 	unsigned int			dl_defer_armed	  : 1;
 	unsigned int			dl_defer_running  : 1;
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index db47f33..d94f2ed 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1647,6 +1647,7 @@ void dl_server_start(struct sched_dl_entity *dl_se)
 	if (!dl_se->dl_runtime)
 		return;
 
+	dl_se->dl_server_active = 1;
 	enqueue_dl_entity(dl_se, ENQUEUE_WAKEUP);
 	if (!dl_task(dl_se->rq->curr) || dl_entity_preempt(dl_se, &rq->curr->dl))
 		resched_curr(dl_se->rq);
@@ -1661,6 +1662,7 @@ void dl_server_stop(struct sched_dl_entity *dl_se)
 	hrtimer_try_to_cancel(&dl_se->dl_timer);
 	dl_se->dl_defer_armed = 0;
 	dl_se->dl_throttled = 0;
+	dl_se->dl_server_active = 0;
 }
 
 void dl_server_init(struct sched_dl_entity *dl_se, struct rq *rq,
@@ -2421,8 +2423,10 @@ again:
 	if (dl_server(dl_se)) {
 		p = dl_se->server_pick_task(dl_se);
 		if (!p) {
-			dl_se->dl_yielded = 1;
-			update_curr_dl_se(rq, dl_se, 0);
+			if (dl_server_active(dl_se)) {
+				dl_se->dl_yielded = 1;
+				update_curr_dl_se(rq, dl_se, 0);
+			}
 			goto again;
 		}
 		rq->dl_server = dl_se;
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 1e494af..c5d67a4 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -398,6 +398,11 @@ extern void __dl_server_attach_root(struct sched_dl_entity *dl_se, struct rq *rq
 extern int dl_server_apply_params(struct sched_dl_entity *dl_se,
 		    u64 runtime, u64 period, bool init);
 
+static inline bool dl_server_active(struct sched_dl_entity *dl_se)
+{
+	return dl_se->dl_server_active;
+}
+
 #ifdef CONFIG_CGROUP_SCHED
 
 extern struct list_head task_groups;

Re: [PATCH 1/2] EXP sched/dlserver: flag to represent active status of dlserver

[Paul E. McKenney]

On Thu, Dec 12, 2024 at 10:22:36PM -0500, Vineeth Pillai (Google) wrote:
> dlserver can get dequeued during a dlserver pick_task due to the delayed
> deueue feature and this can lead to issues with dlserver logic as it
> still thinks that dlserver is on the runqueue. The dlserver throttling
> and replenish logic gets confused and can lead to double enqueue of
> dlserver.
> 
> Double enqueue of dlserver could happend due to couple of reasons:
> 
> Case 1
> ------
> 
> Delayed dequeue feature[1] can cause dlserver being stopped during a
> pick initiated by dlserver:
>   __pick_next_task
>    pick_task_dl -> server_pick_task
>     pick_task_fair
>      pick_next_entity (if (sched_delayed))
>       dequeue_entities
>        dl_server_stop
> 
> server_pick_task goes ahead with update_curr_dl_se without knowing that
> dlserver is dequeued and this confuses the logic and may lead to
> unintended enqueue while the server is stopped.
> 
> Case 2
> ------
> A race condition between a task dequeue on one cpu and same task's enqueue
> on this cpu by a remote cpu while the lock is released causing dlserver
> double enqueue.
> 
> One cpu would be in the schedule() and releasing RQ-lock:
> 
> current->state = TASK_INTERRUPTIBLE();
>         schedule();
>           deactivate_task()
>             dl_stop_server();
>           pick_next_task()
>             pick_next_task_fair()
>               sched_balance_newidle()
>                 rq_unlock(this_rq)
> 
> at which point another CPU can take our RQ-lock and do:
> 
>         try_to_wake_up()
>           ttwu_queue()
>             rq_lock()
>             ...
>             activate_task()
>               dl_server_start() --> first enqueue
>             wakeup_preempt() := check_preempt_wakeup_fair()
>               update_curr()
>                 update_curr_task()
>                   if (current->dl_server)
>                     dl_server_update()
>                       enqueue_dl_entity() --> second enqueue
> 
> This bug was not apparent as the enqueue in dl_server_start doesn't
> usually happen because of the defer logic. But as a side effect of the
> first case(dequeue during dlserver pick), dl_throttled and dl_yield will
> be set and this causes the time accounting of dlserver to messup and
> then leading to a enqueue in dl_server_start.
> 
> Have an explicit flag representing the status of dlserver to avoid the
> confusion. This is set in dl_server_start and reset in dlserver_stop.
> 
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Vineeth Pillai (Google) <vineeth@bitbyteword.org>

With both these patches, TREE03 ran without error for more than 20 hours
in a configuration that normally generates 50 enqueue_dl_entity() splats.
This provides at least 99% confidence of a reduction in splat rate of
at least an order of magnitude.  ;-)

For both:

Tested-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  include/linux/sched.h   | 7 +++++++
>  kernel/sched/deadline.c | 8 ++++++--
>  kernel/sched/sched.h    | 5 +++++
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d380bffee2ef..66b311fbd5d6 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -656,6 +656,12 @@ struct sched_dl_entity {
>  	 * @dl_defer_armed tells if the deferrable server is waiting
>  	 * for the replenishment timer to activate it.
>  	 *
> +	 * @dl_server_active tells if the dlserver is active(started).
> +	 * dlserver is started on first cfs enqueue on an idle runqueue
> +	 * and is stopped when a dequeue results in 0 cfs tasks on the
> +	 * runqueue. In other words, dlserver is active only when cpu's
> +	 * runqueue has atleast one cfs task.
> +	 *
>  	 * @dl_defer_running tells if the deferrable server is actually
>  	 * running, skipping the defer phase.
>  	 */
> @@ -664,6 +670,7 @@ struct sched_dl_entity {
>  	unsigned int			dl_non_contending : 1;
>  	unsigned int			dl_overrun	  : 1;
>  	unsigned int			dl_server         : 1;
> +	unsigned int			dl_server_active  : 1;
>  	unsigned int			dl_defer	  : 1;
>  	unsigned int			dl_defer_armed	  : 1;
>  	unsigned int			dl_defer_running  : 1;
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 33b4646f8b24..0abf14ac5ca7 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -1667,6 +1667,7 @@ void dl_server_start(struct sched_dl_entity *dl_se)
>  	if (!dl_se->dl_runtime)
>  		return;
>  
> +	dl_se->dl_server_active = 1;
>  	enqueue_dl_entity(dl_se, ENQUEUE_WAKEUP);
>  	if (!dl_task(dl_se->rq->curr) || dl_entity_preempt(dl_se, &rq->curr->dl))
>  		resched_curr(dl_se->rq);
> @@ -1681,6 +1682,7 @@ void dl_server_stop(struct sched_dl_entity *dl_se)
>  	hrtimer_try_to_cancel(&dl_se->dl_timer);
>  	dl_se->dl_defer_armed = 0;
>  	dl_se->dl_throttled = 0;
> +	dl_se->dl_server_active = 0;
>  }
>  
>  void dl_server_init(struct sched_dl_entity *dl_se, struct rq *rq,
> @@ -2435,8 +2437,10 @@ static struct task_struct *__pick_task_dl(struct rq *rq)
>  	if (dl_server(dl_se)) {
>  		p = dl_se->server_pick_task(dl_se);
>  		if (!p) {
> -			dl_se->dl_yielded = 1;
> -			update_curr_dl_se(rq, dl_se, 0);
> +			if (dl_server_active(dl_se)) {
> +				dl_se->dl_yielded = 1;
> +				update_curr_dl_se(rq, dl_se, 0);
> +			}
>  			goto again;
>  		}
>  		rq->dl_server = dl_se;
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index aef716c41edb..65fa64845d9f 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -398,6 +398,11 @@ extern void __dl_server_attach_root(struct sched_dl_entity *dl_se, struct rq *rq
>  extern int dl_server_apply_params(struct sched_dl_entity *dl_se,
>  		    u64 runtime, u64 period, bool init);
>  
> +static inline bool dl_server_active(struct sched_dl_entity *dl_se)
> +{
> +	return dl_se->dl_server_active;
> +}
> +
>  #ifdef CONFIG_CGROUP_SCHED
>  
>  extern struct list_head task_groups;
> -- 
> 2.45.2
>