Re: [PATCH 17/24] sched/fair: Implement delayed dequeue

Re: [PATCH 17/24] sched/fair: Implement delayed dequeue

[Bert Karwatzki]

Since linux next-20240820 the following messages appears when booting:

[    T1] smp: Bringing up secondary CPUs ...
[    T1] smpboot: x86: Booting SMP configuration:
[    T1] .... node  #0, CPUs:        #2  #4  #6  #8 #10 #12 #14  #1
This is the line I'm concerend about:
[    T1] psi: inconsistent task state! task=61:cpuhp/3 cpu=0 psi_flags=4 clear=0 set=4
[    T1]   #3  #5  #7  #9 #11 #13 #15
[    T1] Spectre V2 : Update user space SMT mitigation: STIBP always-on
[    T1] smp: Brought up 1 node, 16 CPUs
[    T1] smpboot: Total of 16 processors activated (102216.16 BogoMIPS)

I bisected this to commit 152e11f6df29 ("sched/fair: Implement delayed dequeue").
Is this normal or is this something I should worry about?

Bert Karwatzki

[REGRESSION] Re: [PATCH 17/24] sched/fair: Implement delayed dequeue

[Chris Bainbridge]

On Fri, Aug 30, 2024 at 02:34:56PM +0200, Bert Karwatzki wrote:
> Since linux next-20240820 the following messages appears when booting:
> 
> [    T1] smp: Bringing up secondary CPUs ...
> [    T1] smpboot: x86: Booting SMP configuration:
> [    T1] .... node  #0, CPUs:        #2  #4  #6  #8 #10 #12 #14  #1
> This is the line I'm concerend about:
> [    T1] psi: inconsistent task state! task=61:cpuhp/3 cpu=0 psi_flags=4 clear=0 set=4
> [    T1]   #3  #5  #7  #9 #11 #13 #15
> [    T1] Spectre V2 : Update user space SMT mitigation: STIBP always-on
> [    T1] smp: Brought up 1 node, 16 CPUs
> [    T1] smpboot: Total of 16 processors activated (102216.16 BogoMIPS)
> 
> I bisected this to commit 152e11f6df29 ("sched/fair: Implement delayed dequeue").
> Is this normal or is this something I should worry about?
> 
> Bert Karwatzki

I am also getting a similar error on boot, and bisected it to the same commit:

[    0.342931] psi: inconsistent task state! task=15:rcu_tasks_trace cpu=0 psi_flags=4 clear=0 set=4

#regzbot introduced: 152e11f6df293e816a6a37c69757033cdc72667d

Re: [REGRESSION] Re: [PATCH 17/24] sched/fair: Implement delayed dequeue

[Linux regression tracking (Thorsten Leemhuis)]

On 22.09.24 17:45, Chris Bainbridge wrote:
> On Fri, Aug 30, 2024 at 02:34:56PM +0200, Bert Karwatzki wrote:
>> Since linux next-20240820 the following messages appears when booting:
>>
>> [    T1] smp: Bringing up secondary CPUs ...
>> [    T1] smpboot: x86: Booting SMP configuration:
>> [    T1] .... node  #0, CPUs:        #2  #4  #6  #8 #10 #12 #14  #1
>> This is the line I'm concerend about:
>> [    T1] psi: inconsistent task state! task=61:cpuhp/3 cpu=0 psi_flags=4 clear=0 set=4
>> [    T1]   #3  #5  #7  #9 #11 #13 #15
>> [    T1] Spectre V2 : Update user space SMT mitigation: STIBP always-on
>> [    T1] smp: Brought up 1 node, 16 CPUs
>> [    T1] smpboot: Total of 16 processors activated (102216.16 BogoMIPS)
>>
>> I bisected this to commit 152e11f6df29 ("sched/fair: Implement delayed dequeue").
>> Is this normal or is this something I should worry about?
>>
> I am also getting a similar error on boot, and bisected it to the same commit:
> 
> [    0.342931] psi: inconsistent task state! task=15:rcu_tasks_trace cpu=0 psi_flags=4 clear=0 set=4
> 
> #regzbot introduced: 152e11f6df293e816a6a37c69757033cdc72667d

thx for CCing the regressions list.

Paul also reported this earlier today:

https://lore.kernel.org/all/cd67fbcd-d659-4822-bb90-7e8fbb40a856@molgen.mpg.de/

Hannes [CCed] replied there and has some idea what might be wrong:

https://lore.kernel.org/all/20240922102047.GA437832@cmpxchg.org/

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

P.S.:

#regzbot dup:
https://lore.kernel.org/all/cd67fbcd-d659-4822-bb90-7e8fbb40a856@molgen.mpg.de/

#regzbot report:
https://lore.kernel.org/all/cd67fbcd-d659-4822-bb90-7e8fbb40a856@molgen.mpg.de/
#regzbot introduced: 152e11f6df293e816a6a37c69757033cdc72667d

Re: [REGRESSION] Re: [PATCH 17/24] sched/fair: Implement delayed dequeue

[Klaus Kudielka]

On Sun, 2024-09-22 at 16:45 +0100, Chris Bainbridge wrote:
> On Fri, Aug 30, 2024 at 02:34:56PM +0200, Bert Karwatzki wrote:
> > Since linux next-20240820 the following messages appears when booting:
> > 
> > [    T1] smp: Bringing up secondary CPUs ...
> > [    T1] smpboot: x86: Booting SMP configuration:
> > [    T1] .... node  #0, CPUs:        #2  #4  #6  #8 #10 #12 #14  #1
> > This is the line I'm concerend about:
> > [    T1] psi: inconsistent task state! task=61:cpuhp/3 cpu=0 psi_flags=4 clear=0 set=4
> > [    T1]   #3  #5  #7  #9 #11 #13 #15
> > [    T1] Spectre V2 : Update user space SMT mitigation: STIBP always-on
> > [    T1] smp: Brought up 1 node, 16 CPUs
> > [    T1] smpboot: Total of 16 processors activated (102216.16 BogoMIPS)
> > 
> > I bisected this to commit 152e11f6df29 ("sched/fair: Implement delayed dequeue").
> > Is this normal or is this something I should worry about?
> > 
> > Bert Karwatzki
> 
> I am also getting a similar error on boot, and bisected it to the same commit:
> 
> [    0.342931] psi: inconsistent task state! task=15:rcu_tasks_trace cpu=0 psi_flags=4 clear=0 set=4
> 
> #regzbot introduced: 152e11f6df293e816a6a37c69757033cdc72667d

Just another data point, while booting 6.12-rc1 on a Turris Omnia:

[    0.000000] Linux version 6.12.0-rc1 (XXX) (arm-linux-gnueabihf-gcc (Debian 14.2.0-1) 14.2.0, GNU ld (GNU Binutils for Debian) 2.43.1) #1 SMP Thu Oct  3 06:59:25 CEST 2024
[    0.000000] CPU: ARMv7 Processor [414fc091] revision 1 (ARMv7), cr=10c5387d
[    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
[    0.000000] OF: fdt: Machine model: Turris Omnia
...
[    0.000867] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
[    0.000876] psi: inconsistent task state! task=2:kthreadd cpu=0 psi_flags=4 clear=0 set=4

Re: [REGRESSION] Re: [PATCH 17/24] sched/fair: Implement delayed dequeue

[K Prateek Nayak]

Hello folks,

On 10/3/2024 11:01 AM, Klaus Kudielka wrote:
> On Sun, 2024-09-22 at 16:45 +0100, Chris Bainbridge wrote:
>> On Fri, Aug 30, 2024 at 02:34:56PM +0200, Bert Karwatzki wrote:
>>> Since linux next-20240820 the following messages appears when booting:
>>>
>>> [    T1] smp: Bringing up secondary CPUs ...
>>> [    T1] smpboot: x86: Booting SMP configuration:
>>> [    T1] .... node  #0, CPUs:        #2  #4  #6  #8 #10 #12 #14  #1
>>> This is the line I'm concerend about:
>>> [    T1] psi: inconsistent task state! task=61:cpuhp/3 cpu=0 psi_flags=4 clear=0 set=4
>>> [    T1]   #3  #5  #7  #9 #11 #13 #15
>>> [    T1] Spectre V2 : Update user space SMT mitigation: STIBP always-on
>>> [    T1] smp: Brought up 1 node, 16 CPUs
>>> [    T1] smpboot: Total of 16 processors activated (102216.16 BogoMIPS)
>>>
>>> I bisected this to commit 152e11f6df29 ("sched/fair: Implement delayed dequeue").
>>> Is this normal or is this something I should worry about?
>>>
>>> Bert Karwatzki
>>
>> I am also getting a similar error on boot, and bisected it to the same commit:
>>
>> [    0.342931] psi: inconsistent task state! task=15:rcu_tasks_trace cpu=0 psi_flags=4 clear=0 set=4
>>
>> #regzbot introduced: 152e11f6df293e816a6a37c69757033cdc72667d
> 
> Just another data point, while booting 6.12-rc1 on a Turris Omnia:
> 
> [    0.000000] Linux version 6.12.0-rc1 (XXX) (arm-linux-gnueabihf-gcc (Debian 14.2.0-1) 14.2.0, GNU ld (GNU Binutils for Debian) 2.43.1) #1 SMP Thu Oct  3 06:59:25 CEST 2024
> [    0.000000] CPU: ARMv7 Processor [414fc091] revision 1 (ARMv7), cr=10c5387d
> [    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
> [    0.000000] OF: fdt: Machine model: Turris Omnia
> ...
> [    0.000867] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
> [    0.000876] psi: inconsistent task state! task=2:kthreadd cpu=0 psi_flags=4 clear=0 set=4
> 

Not sure if someone took a stab at this but I haven't seen the "psi:
inconsistent task state" warning with the below diff. I'm not sure if my
approach is right which if why I'm pasting the diff before sending out
an official series. Any comments or testing is greatly appreciated.

The diff is based on:

     git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git sched/urgent

at commit d4ac164bde7a ("sched/eevdf: Fix wakeup-preempt by checking
cfs_rq->nr_running")

My approach was as follows:

o psi_dequeue() relied on psi_sched_switch() to set the PSI flags
   appropriately for a dequeued task. However, psi_sched_switch() used
   "!task_on_rq_queued(prev)" to judge if the prev task is blocked which
   is now untrue with DELAYED_DEQUEUE. Fix it by checking
   "p->se.sched_delayed" as well. I also added a matching check for
   ENQUEUE_DELAYED for psi_enqueue().

o With the above, the warning was put off for a few more seconds but it
   still appeared. I dumped all PSI flag transition along with
   "tsk->se.sched_delayed" to see what trips it and I saw the following
   state changes for the task that finally tripped it:

     psi: task state: task=18:rcu_preempt cpu=0 psi_flags=0 clear=0 set=0 delayed=1
     psi: task state: task=18:rcu_preempt cpu=128 psi_flags=0 clear=0 set=4 delayed=1
     psi: task state: task=18:rcu_preempt cpu=128 psi_flags=4 clear=0 set=4 delayed=0
     psi: inconsistent task state! task=18:rcu_preempt cpu=128 psi_flags=4 clear=0 set=4 delayed=0

   Note that cpu switched with "tsk->se.sched_delayed" still set which
   got me looking at the task migration path. The warning added below
   in "deactivate_task()" tripped without fail, just before the PSI
   warning was logged.

   To prevent migration of a delayed entity (XXX: Is it a good idea?)
   we do a "account_task_dequeue()" in the delayed dequeue case to
   remove the task from the "rq->cfs_list", thus removing it from the
   purview of the load balancer.

o With the above change, I only managed to trip the deactivate_task()
   WARN_ON() and immediately the PSI warning in the NUMA balancing path

     ------------[ cut here ]------------
     p->se.sched_delayed
     WARNING: CPU: 75 PID: 473 at kernel/sched/core.c:2075 deactivate_task+0xa6/0xc0
     Modules linked in: ...
     CPU: 75 UID: 0 PID: 473 Comm: migration/75 Not tainted 6.12.0-rc1-peterz-sched-urgent-psi-fix+ #32
     Hardware name: Dell Inc. PowerEdge R6525/024PW1, BIOS 2.7.3 03/30/2022
     Stopper: multi_cpu_stop+0x0/0x110 <- migrate_swap+0xd7/0x150
     RIP: 0010:deactivate_task+0xa6/0xc0
     Code: ...
     RSP: 0018:ffff9f210e12fdc0 EFLAGS: 00010086
     RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000027
     RDX: ffff90116ffa18c8 RSI: 0000000000000001 RDI: ffff90116ffa18c0
     RBP: ffff8fd2d8559ac0 R08: 0000000000000003 R09: 0000000000000000
     R10: 64656863732e6573 R11: 646579616c65645f R12: ffff90116ffb6500
     R13: ffff90116ffb6500 R14: 0000000000000004 R15: ffff8fd2f28f433c
     FS:  0000000000000000(0000) GS:ffff90116ff80000(0000) knlGS:0000000000000000
     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
     CR2: 000055748a34e000 CR3: 000000807974e004 CR4: 0000000000f70ef0
     PKRU: 55555554
     Call Trace:
      <TASK>
      ? __warn+0x88/0x130
      ? deactivate_task+0xa6/0xc0
      ? report_bug+0x18e/0x1a0
      ? prb_read_valid+0x1b/0x30
      ? handle_bug+0x5b/0xa0
      ? exc_invalid_op+0x18/0x70
      ? asm_exc_invalid_op+0x1a/0x20
      ? deactivate_task+0xa6/0xc0
      __migrate_swap_task.part.0+0xbe/0x180
      migrate_swap_stop+0x1b6/0x1f0
      multi_cpu_stop+0x6e/0x110
      ? __pfx_multi_cpu_stop+0x10/0x10
      cpu_stopper_thread+0x97/0x160
      ? __pfx_smpboot_thread_fn+0x10/0x10
      smpboot_thread_fn+0xdd/0x1d0
      kthread+0xd3/0x100
      ? __pfx_kthread+0x10/0x10
      ret_from_fork+0x34/0x50
      ? __pfx_kthread+0x10/0x10
      ret_from_fork_asm+0x1a/0x30
      </TASK>
     ---[ end trace 0000000000000000 ]---

   From some logging, I can say the "dst_task" is the one that is
   delayed but I could not go up the stack to find out how it is
   chosen for the swap. For this RFC, I just block the delayed
   entity in "__migrate_swap_task()" and set the "p->wake_cpu"
   to redirect the next wakeup to the appropriate NUMA node.

I haven't encountered any warnings with my machine going for a while now
but I haven't tested any fancy cgroups scenarios yet either; Mileage may
vary :)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 43e453ab7e20..b55b52b081ba 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2014,7 +2014,9 @@ void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
  
  	if (!(flags & ENQUEUE_RESTORE)) {
  		sched_info_enqueue(rq, p);
-		psi_enqueue(p, (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED));
+		/* Delayed tasks are considered dequeued by PSI tracking */
+		psi_enqueue(p, ((flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED)) ||
+			       (flags & ENQUEUE_DELAYED));
  	}
  
  	p->sched_class->enqueue_task(rq, p, flags);
@@ -2069,6 +2071,9 @@ void deactivate_task(struct rq *rq, struct task_struct *p, int flags)
  {
  	SCHED_WARN_ON(flags & DEQUEUE_SLEEP);
  
+	/* Delayed tasks should not be migrated */
+	SCHED_WARN_ON(p->se.sched_delayed);
+
  	WRITE_ONCE(p->on_rq, TASK_ON_RQ_MIGRATING);
  	ASSERT_EXCLUSIVE_WRITER(p->on_rq);
  
@@ -3298,9 +3303,21 @@ static void __migrate_swap_task(struct task_struct *p, int cpu)
  		struct rq_flags srf, drf;
  
  		src_rq = task_rq(p);
-		dst_rq = cpu_rq(cpu);
-
  		rq_pin_lock(src_rq, &srf);
+
+		if (p->se.sched_delayed) {
+			block_task(src_rq, p, DEQUEUE_DELAYED);
+			rq_unpin_lock(src_rq, &srf);
+
+			/*
+			 * Make it appear we last ran on the preferred
+			 * node. See the comment below.
+			 */
+			p->wake_cpu = cpu;
+			return;
+		}
+
+		dst_rq = cpu_rq(cpu);
  		rq_pin_lock(dst_rq, &drf);
  
  		deactivate_task(src_rq, p, 0);
@@ -6667,7 +6684,14 @@ static void __sched notrace __schedule(int sched_mode)
  
  		migrate_disable_switch(rq, prev);
  		psi_account_irqtime(rq, prev, next);
-		psi_sched_switch(prev, next, !task_on_rq_queued(prev));
+
+		/*
+		 * psi_task_switch() is responsible for clearing TSK_RUNNING
+		 * and TSK_IOWAIT which psi_dequeue() skips for a task going
+		 * to sleep (see comment there). Consider a delayed entity
+		 * as one that has gone to sleep for PSI accounting.
+		 */
+		psi_sched_switch(prev, next, !task_on_rq_queued(prev) || prev->se.sched_delayed);
  
  		trace_sched_switch(preempt, prev, next, prev_state);
  
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index ab497fafa7be..cf02d202ab0c 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3661,18 +3661,40 @@ static inline void update_scan_period(struct task_struct *p, int new_cpu)
  
  #endif /* CONFIG_NUMA_BALANCING */
  
+#ifdef CONFIG_SMP
+
+static void
+account_task_enqueue(struct cfs_rq *cfs_rq, struct sched_entity *se)
+{
+	struct rq *rq = rq_of(cfs_rq);
+
+	account_numa_enqueue(rq, task_of(se));
+	list_add(&se->group_node, &rq->cfs_tasks);
+}
+
+static void
+account_task_dequeue(struct cfs_rq *cfs_rq, struct sched_entity *se)
+{
+	account_numa_dequeue(rq_of(cfs_rq), task_of(se));
+	list_del_init(&se->group_node);
+}
+
+#else
+
+static void
+account_task_enqueue(struct cfs_rq *cfs_rq, struct sched_entity *se) { }
+static void
+account_task_dequeue(struct cfs_rq *cfs_rq, struct sched_entity *se) { }
+
+#endif
+
  static void
  account_entity_enqueue(struct cfs_rq *cfs_rq, struct sched_entity *se)
  {
  	update_load_add(&cfs_rq->load, se->load.weight);
-#ifdef CONFIG_SMP
-	if (entity_is_task(se)) {
-		struct rq *rq = rq_of(cfs_rq);
+	if (entity_is_task(se) && !se->sched_delayed)
+		account_task_enqueue(cfs_rq, se);
  
-		account_numa_enqueue(rq, task_of(se));
-		list_add(&se->group_node, &rq->cfs_tasks);
-	}
-#endif
  	cfs_rq->nr_running++;
  	if (se_is_idle(se))
  		cfs_rq->idle_nr_running++;
@@ -3682,12 +3704,11 @@ static void
  account_entity_dequeue(struct cfs_rq *cfs_rq, struct sched_entity *se)
  {
  	update_load_sub(&cfs_rq->load, se->load.weight);
-#ifdef CONFIG_SMP
-	if (entity_is_task(se)) {
-		account_numa_dequeue(rq_of(cfs_rq), task_of(se));
-		list_del_init(&se->group_node);
-	}
-#endif
+
+	/* Delayed tasks are already dequeued the first time */
+	if (entity_is_task(se) && !se->sched_delayed)
+		account_task_dequeue(cfs_rq, se);
+
  	cfs_rq->nr_running--;
  	if (se_is_idle(se))
  		cfs_rq->idle_nr_running--;
@@ -6943,6 +6964,10 @@ requeue_delayed_entity(struct sched_entity *se)
  
  	update_load_avg(cfs_rq, se, 0);
  	se->sched_delayed = 0;
+
+	if (entity_is_task(se))
+		account_task_enqueue(cfs_rq, se);
+
  }
  
  /*
@@ -7190,10 +7215,18 @@ static int dequeue_entities(struct rq *rq, struct sched_entity *se, int flags)
   */
  static bool dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
  {
-	if (!(p->se.sched_delayed && (task_on_rq_migrating(p) || (flags & DEQUEUE_SAVE))))
+	struct sched_entity *se = &p->se;
+
+	if (!(se->sched_delayed && (task_on_rq_migrating(p) || (flags & DEQUEUE_SAVE))))
  		util_est_dequeue(&rq->cfs, p);
  
-	if (dequeue_entities(rq, &p->se, flags) < 0) {
+	if (dequeue_entities(rq, se, flags) < 0) {
+		/*
+		 * Remove delayed entity from rq->cfs_tasks list
+		 * to prevent load balancer from migrating it
+		 * away.
+		 */
+		account_task_dequeue(cfs_rq_of(se), se);
  		util_est_update(&rq->cfs, p, DEQUEUE_SLEEP);
  		return false;
  	}
--

The above changes were arrived at by experimenting. If there are no
obvious objections, I'll send a clean series after some more testing.
Any and all comments are highly appreciated.
-- 
Thanks and Regards,
Prateek

Re: [REGRESSION] Re: [PATCH 17/24] sched/fair: Implement delayed dequeue

[Peter Zijlstra]

On Fri, Oct 04, 2024 at 04:40:08PM +0530, K Prateek Nayak wrote:
> Hello folks,
> 
> On 10/3/2024 11:01 AM, Klaus Kudielka wrote:
> > On Sun, 2024-09-22 at 16:45 +0100, Chris Bainbridge wrote:
> > > On Fri, Aug 30, 2024 at 02:34:56PM +0200, Bert Karwatzki wrote:
> > > > Since linux next-20240820 the following messages appears when booting:
> > > > 
> > > > [    T1] smp: Bringing up secondary CPUs ...
> > > > [    T1] smpboot: x86: Booting SMP configuration:
> > > > [    T1] .... node  #0, CPUs:        #2  #4  #6  #8 #10 #12 #14  #1
> > > > This is the line I'm concerend about:
> > > > [    T1] psi: inconsistent task state! task=61:cpuhp/3 cpu=0 psi_flags=4 clear=0 set=4
> > > > [    T1]   #3  #5  #7  #9 #11 #13 #15
> > > > [    T1] Spectre V2 : Update user space SMT mitigation: STIBP always-on
> > > > [    T1] smp: Brought up 1 node, 16 CPUs
> > > > [    T1] smpboot: Total of 16 processors activated (102216.16 BogoMIPS)
> > > > 
> > > > I bisected this to commit 152e11f6df29 ("sched/fair: Implement delayed dequeue").
> > > > Is this normal or is this something I should worry about?
> > > > 
> > > > Bert Karwatzki
> > > 
> > > I am also getting a similar error on boot, and bisected it to the same commit:
> > > 
> > > [    0.342931] psi: inconsistent task state! task=15:rcu_tasks_trace cpu=0 psi_flags=4 clear=0 set=4
> > > 
> > > #regzbot introduced: 152e11f6df293e816a6a37c69757033cdc72667d
> > 
> > Just another data point, while booting 6.12-rc1 on a Turris Omnia:
> > 
> > [    0.000000] Linux version 6.12.0-rc1 (XXX) (arm-linux-gnueabihf-gcc (Debian 14.2.0-1) 14.2.0, GNU ld (GNU Binutils for Debian) 2.43.1) #1 SMP Thu Oct  3 06:59:25 CEST 2024
> > [    0.000000] CPU: ARMv7 Processor [414fc091] revision 1 (ARMv7), cr=10c5387d
> > [    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
> > [    0.000000] OF: fdt: Machine model: Turris Omnia
> > ...
> > [    0.000867] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
> > [    0.000876] psi: inconsistent task state! task=2:kthreadd cpu=0 psi_flags=4 clear=0 set=4
> > 
> 
> Not sure if someone took a stab at this but I haven't seen the "psi:

I'm aware of the issue, but since it's just statistics and not
anything 'important', I've been spending my time on those crashing bugs.

> inconsistent task state" warning with the below diff. I'm not sure if my
> approach is right which if why I'm pasting the diff before sending out
> an official series. Any comments or testing is greatly appreciated.
> 
> The diff is based on:
> 
>     git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git sched/urgent
> 
> at commit d4ac164bde7a ("sched/eevdf: Fix wakeup-preempt by checking
> cfs_rq->nr_running")

Thanks, I just pushed all that out to tip/sched/urgent.

> My approach was as follows:
> 
> o psi_dequeue() relied on psi_sched_switch() to set the PSI flags
>   appropriately for a dequeued task. However, psi_sched_switch() used
>   "!task_on_rq_queued(prev)" to judge if the prev task is blocked which
>   is now untrue with DELAYED_DEQUEUE. Fix it by checking
>   "p->se.sched_delayed" as well. I also added a matching check for
>   ENQUEUE_DELAYED for psi_enqueue().

We already determine the whole sleep state earlier, the whole having
called block_task() is a clue, perhaps we should propagate that state
instead of trying to divinate it again.

> o With the above, the warning was put off for a few more seconds but it
>   still appeared. I dumped all PSI flag transition along with
>   "tsk->se.sched_delayed" to see what trips it and I saw the following
>   state changes for the task that finally tripped it:
> 
>     psi: task state: task=18:rcu_preempt cpu=0 psi_flags=0 clear=0 set=0 delayed=1
>     psi: task state: task=18:rcu_preempt cpu=128 psi_flags=0 clear=0 set=4 delayed=1
>     psi: task state: task=18:rcu_preempt cpu=128 psi_flags=4 clear=0 set=4 delayed=0
>     psi: inconsistent task state! task=18:rcu_preempt cpu=128 psi_flags=4 clear=0 set=4 delayed=0
> 
>   Note that cpu switched with "tsk->se.sched_delayed" still set which
>   got me looking at the task migration path. The warning added below
>   in "deactivate_task()" tripped without fail, just before the PSI
>   warning was logged.
> 
>   To prevent migration of a delayed entity (XXX: Is it a good idea?)

It is not. By migrating the entities they can get picked sooner and the
delayed thing gets removed sooner. Less 'hidden' weight.

>   we do a "account_task_dequeue()" in the delayed dequeue case to
>   remove the task from the "rq->cfs_list", thus removing it from the
>   purview of the load balancer.

Anyway, assuming PSI wants to preserve current semantics, does something
like the below work?

---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 43e453ab7e20..0d766fb9fbc4 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2012,7 +2012,7 @@ void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
 	if (!(flags & ENQUEUE_NOCLOCK))
 		update_rq_clock(rq);
 
-	if (!(flags & ENQUEUE_RESTORE)) {
+	if (!(flags & ENQUEUE_RESTORE) && !p->se.sched_delayed) {
 		sched_info_enqueue(rq, p);
 		psi_enqueue(p, (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED));
 	}
@@ -2039,7 +2039,7 @@ inline bool dequeue_task(struct rq *rq, struct task_struct *p, int flags)
 	if (!(flags & DEQUEUE_NOCLOCK))
 		update_rq_clock(rq);
 
-	if (!(flags & DEQUEUE_SAVE)) {
+	if (!(flags & DEQUEUE_SAVE) && !p->se.sched_delayed) {
 		sched_info_dequeue(rq, p);
 		psi_dequeue(p, flags & DEQUEUE_SLEEP);
 	}
@@ -6537,6 +6537,7 @@ static void __sched notrace __schedule(int sched_mode)
 	 * as a preemption by schedule_debug() and RCU.
 	 */
 	bool preempt = sched_mode > SM_NONE;
+	bool block = false;
 	unsigned long *switch_count;
 	unsigned long prev_state;
 	struct rq_flags rf;
@@ -6622,6 +6623,7 @@ static void __sched notrace __schedule(int sched_mode)
 			 * After this, schedule() must not care about p->state any more.
 			 */
 			block_task(rq, prev, flags);
+			block = true;
 		}
 		switch_count = &prev->nvcsw;
 	}
@@ -6667,7 +6669,7 @@ static void __sched notrace __schedule(int sched_mode)
 
 		migrate_disable_switch(rq, prev);
 		psi_account_irqtime(rq, prev, next);
-		psi_sched_switch(prev, next, !task_on_rq_queued(prev));
+		psi_sched_switch(prev, next, block);
 
 		trace_sched_switch(preempt, prev, next, prev_state);
 

Re: [REGRESSION] Re: [PATCH 17/24] sched/fair: Implement delayed dequeue

[Johannes Weiner]

On Fri, Oct 04, 2024 at 02:35:06PM +0200, Peter Zijlstra wrote:
> On Fri, Oct 04, 2024 at 04:40:08PM +0530, K Prateek Nayak wrote:
> > Hello folks,
> > 
> > On 10/3/2024 11:01 AM, Klaus Kudielka wrote:
> > > On Sun, 2024-09-22 at 16:45 +0100, Chris Bainbridge wrote:
> > > > On Fri, Aug 30, 2024 at 02:34:56PM +0200, Bert Karwatzki wrote:
> > > > > Since linux next-20240820 the following messages appears when booting:
> > > > > 
> > > > > [    T1] smp: Bringing up secondary CPUs ...
> > > > > [    T1] smpboot: x86: Booting SMP configuration:
> > > > > [    T1] .... node  #0, CPUs:        #2  #4  #6  #8 #10 #12 #14  #1
> > > > > This is the line I'm concerend about:
> > > > > [    T1] psi: inconsistent task state! task=61:cpuhp/3 cpu=0 psi_flags=4 clear=0 set=4
> > > > > [    T1]   #3  #5  #7  #9 #11 #13 #15
> > > > > [    T1] Spectre V2 : Update user space SMT mitigation: STIBP always-on
> > > > > [    T1] smp: Brought up 1 node, 16 CPUs
> > > > > [    T1] smpboot: Total of 16 processors activated (102216.16 BogoMIPS)
> > > > > 
> > > > > I bisected this to commit 152e11f6df29 ("sched/fair: Implement delayed dequeue").
> > > > > Is this normal or is this something I should worry about?
> > > > > 
> > > > > Bert Karwatzki
> > > > 
> > > > I am also getting a similar error on boot, and bisected it to the same commit:
> > > > 
> > > > [    0.342931] psi: inconsistent task state! task=15:rcu_tasks_trace cpu=0 psi_flags=4 clear=0 set=4
> > > > 
> > > > #regzbot introduced: 152e11f6df293e816a6a37c69757033cdc72667d
> > > 
> > > Just another data point, while booting 6.12-rc1 on a Turris Omnia:
> > > 
> > > [    0.000000] Linux version 6.12.0-rc1 (XXX) (arm-linux-gnueabihf-gcc (Debian 14.2.0-1) 14.2.0, GNU ld (GNU Binutils for Debian) 2.43.1) #1 SMP Thu Oct  3 06:59:25 CEST 2024
> > > [    0.000000] CPU: ARMv7 Processor [414fc091] revision 1 (ARMv7), cr=10c5387d
> > > [    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
> > > [    0.000000] OF: fdt: Machine model: Turris Omnia
> > > ...
> > > [    0.000867] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
> > > [    0.000876] psi: inconsistent task state! task=2:kthreadd cpu=0 psi_flags=4 clear=0 set=4
> > > 
> > 
> > Not sure if someone took a stab at this but I haven't seen the "psi:
> 
> I'm aware of the issue, but since it's just statistics and not
> anything 'important', I've been spending my time on those crashing bugs.
> 
> > inconsistent task state" warning with the below diff. I'm not sure if my
> > approach is right which if why I'm pasting the diff before sending out
> > an official series. Any comments or testing is greatly appreciated.

This fixes the bug for me.

> Anyway, assuming PSI wants to preserve current semantics, does something
> like the below work?

This doesn't. But it's a different corruption now:

[    2.298408] psi: inconsistent task state! task=24:cpuhp/1 cpu=1 psi_flags=10 clear=14 set=0

It's psi_sched_switch(.sleep=true) trying to clear the running state
and the queued state, but finds only the running state set.

I don't think it's an erroneous dequeue. __schedule() has that
block_task() dequeue before the switch, but the DEQUEUE_SLEEP makes it
a no-op and leaves the combined update to psi_sched_switch().

It looks instead it's missing an enqueue callback. This triggers:

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 020d58967d4e..09a251e3986d 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -922,6 +922,8 @@ void psi_task_switch(struct task_struct *prev, struct task_struct *next,
 	u64 now = cpu_clock(cpu);
 
 	if (next->pid) {
+		WARN_ON_ONCE(!(next->psi_flags & TSK_RUNNING));
+
 		psi_flags_change(next, 0, TSK_ONCPU);
 		/*
 		 * Set TSK_ONCPU on @next's cgroups. If @next shares any

> ---
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 43e453ab7e20..0d766fb9fbc4 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2012,7 +2012,7 @@ void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
>  	if (!(flags & ENQUEUE_NOCLOCK))
>  		update_rq_clock(rq);
>  
> -	if (!(flags & ENQUEUE_RESTORE)) {
> +	if (!(flags & ENQUEUE_RESTORE) && !p->se.sched_delayed) {
>  		sched_info_enqueue(rq, p);
>  		psi_enqueue(p, (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED));
>  	}

... we must be skipping here when we shouldn't.

I tried moving it past ->enqueue_task(), like we did for uclamp, to
get the enqueue when sched_delayed is cleared by the callback and task
is considered properly queued again. However, that results in yet
another problem:

[    4.624776] psi: inconsistent task state! task=161:systemd-ssh-gen cpu=1 psi_flags=15 clear=14 set=1

This is a psi_sched_switch() trying to clear running|queued and set
iowait. Task is already running|queued|iowait. It looks like we had a
genuine wakeup that was signaled with psi_enqueue(.wakeup=false) (so
it didn't clear the iowait).

> @@ -2039,7 +2039,7 @@ inline bool dequeue_task(struct rq *rq, struct task_struct *p, int flags)
>  	if (!(flags & DEQUEUE_NOCLOCK))
>  		update_rq_clock(rq);
>  
> -	if (!(flags & DEQUEUE_SAVE)) {
> +	if (!(flags & DEQUEUE_SAVE) && !p->se.sched_delayed) {
>  		sched_info_dequeue(rq, p);
>  		psi_dequeue(p, flags & DEQUEUE_SLEEP);
>  	}
> @@ -6537,6 +6537,7 @@ static void __sched notrace __schedule(int sched_mode)
>  	 * as a preemption by schedule_debug() and RCU.
>  	 */
>  	bool preempt = sched_mode > SM_NONE;
> +	bool block = false;
>  	unsigned long *switch_count;
>  	unsigned long prev_state;
>  	struct rq_flags rf;
> @@ -6622,6 +6623,7 @@ static void __sched notrace __schedule(int sched_mode)
>  			 * After this, schedule() must not care about p->state any more.
>  			 */
>  			block_task(rq, prev, flags);
> +			block = true;
>  		}
>  		switch_count = &prev->nvcsw;
>  	}
> @@ -6667,7 +6669,7 @@ static void __sched notrace __schedule(int sched_mode)
>  
>  		migrate_disable_switch(rq, prev);
>  		psi_account_irqtime(rq, prev, next);
> -		psi_sched_switch(prev, next, !task_on_rq_queued(prev));
> +		psi_sched_switch(prev, next, block);
>  
>  		trace_sched_switch(preempt, prev, next, prev_state);

Re: [REGRESSION] Re: [PATCH 17/24] sched/fair: Implement delayed dequeue

[K Prateek Nayak]

Hello Johannes, Peter,

On 10/4/2024 7:27 PM, Johannes Weiner wrote:
> On Fri, Oct 04, 2024 at 02:35:06PM +0200, Peter Zijlstra wrote:
>> On Fri, Oct 04, 2024 at 04:40:08PM +0530, K Prateek Nayak wrote:
>>> Hello folks,
>>>
>>> On 10/3/2024 11:01 AM, Klaus Kudielka wrote:
>>>> On Sun, 2024-09-22 at 16:45 +0100, Chris Bainbridge wrote:
>>>>> On Fri, Aug 30, 2024 at 02:34:56PM +0200, Bert Karwatzki wrote:
>>>>>> Since linux next-20240820 the following messages appears when booting:
>>>>>>
>>>>>> [    T1] smp: Bringing up secondary CPUs ...
>>>>>> [    T1] smpboot: x86: Booting SMP configuration:
>>>>>> [    T1] .... node  #0, CPUs:        #2  #4  #6  #8 #10 #12 #14  #1
>>>>>> This is the line I'm concerend about:
>>>>>> [    T1] psi: inconsistent task state! task=61:cpuhp/3 cpu=0 psi_flags=4 clear=0 set=4
>>>>>> [    T1]   #3  #5  #7  #9 #11 #13 #15
>>>>>> [    T1] Spectre V2 : Update user space SMT mitigation: STIBP always-on
>>>>>> [    T1] smp: Brought up 1 node, 16 CPUs
>>>>>> [    T1] smpboot: Total of 16 processors activated (102216.16 BogoMIPS)
>>>>>>
>>>>>> I bisected this to commit 152e11f6df29 ("sched/fair: Implement delayed dequeue").
>>>>>> Is this normal or is this something I should worry about?
>>>>>>
>>>>>> Bert Karwatzki
>>>>>
>>>>> I am also getting a similar error on boot, and bisected it to the same commit:
>>>>>
>>>>> [    0.342931] psi: inconsistent task state! task=15:rcu_tasks_trace cpu=0 psi_flags=4 clear=0 set=4
>>>>>
>>>>> #regzbot introduced: 152e11f6df293e816a6a37c69757033cdc72667d
>>>>
>>>> Just another data point, while booting 6.12-rc1 on a Turris Omnia:
>>>>
>>>> [    0.000000] Linux version 6.12.0-rc1 (XXX) (arm-linux-gnueabihf-gcc (Debian 14.2.0-1) 14.2.0, GNU ld (GNU Binutils for Debian) 2.43.1) #1 SMP Thu Oct  3 06:59:25 CEST 2024
>>>> [    0.000000] CPU: ARMv7 Processor [414fc091] revision 1 (ARMv7), cr=10c5387d
>>>> [    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
>>>> [    0.000000] OF: fdt: Machine model: Turris Omnia
>>>> ...
>>>> [    0.000867] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
>>>> [    0.000876] psi: inconsistent task state! task=2:kthreadd cpu=0 psi_flags=4 clear=0 set=4
>>>>
>>>
>>> Not sure if someone took a stab at this but I haven't seen the "psi:
>>
>> I'm aware of the issue, but since it's just statistics and not
>> anything 'important', I've been spending my time on those crashing bugs.
>>
>>> inconsistent task state" warning with the below diff. I'm not sure if my
>>> approach is right which if why I'm pasting the diff before sending out
>>> an official series. Any comments or testing is greatly appreciated.
> 
> This fixes the bug for me.

Thank you for testing :)

> 
>> Anyway, assuming PSI wants to preserve current semantics, does something
>> like the below work?
> 
> This doesn't. But it's a different corruption now:
> 
> [    2.298408] psi: inconsistent task state! task=24:cpuhp/1 cpu=1 psi_flags=10 clear=14 set=0

I hit the same log (clear 14, set 0) and I tried the below changes on
top of Peter's diff:

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 0d766fb9fbc4..9cf3d4359994 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2012,9 +2012,10 @@ void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
  	if (!(flags & ENQUEUE_NOCLOCK))
  		update_rq_clock(rq);
  
-	if (!(flags & ENQUEUE_RESTORE) && !p->se.sched_delayed) {
+	if (!(flags & ENQUEUE_RESTORE) && (!p->se.sched_delayed || (flags & ENQUEUE_DELAYED))) {
  		sched_info_enqueue(rq, p);
-		psi_enqueue(p, (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED));
+		psi_enqueue(p, ((flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED)) ||
+			       (flags & ENQUEUE_DELAYED));
  	}
  
  	p->sched_class->enqueue_task(rq, p, flags);
--

... but it just changes the warning to:

     psi: task underflow! cpu=65 t=0 tasks=[0 0 0 0] clear=1 set=4
     psi: task underflow! cpu=31 t=0 tasks=[0 0 1 0] clear=1 set=0

Doing a dump_stack(), I see it come from psi_enqueue() and
psi_ttwu_dequeue() and I see "clear=1" as the common theme. I've
stared at it for a while but I'm at a loss currently. If something
jumps out, I'll update here.

Thank you again both for taking a look.

-- 
Thanks and Regards,
Prateek

> 
> It's psi_sched_switch(.sleep=true) trying to clear the running state
> and the queued state, but finds only the running state set.
> 
> I don't think it's an erroneous dequeue. __schedule() has that
> block_task() dequeue before the switch, but the DEQUEUE_SLEEP makes it
> a no-op and leaves the combined update to psi_sched_switch().
> 
> It looks instead it's missing an enqueue callback. This triggers:
> 
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index 020d58967d4e..09a251e3986d 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -922,6 +922,8 @@ void psi_task_switch(struct task_struct *prev, struct task_struct *next,
>   	u64 now = cpu_clock(cpu);
>   
>   	if (next->pid) {
> +		WARN_ON_ONCE(!(next->psi_flags & TSK_RUNNING));
> +
>   		psi_flags_change(next, 0, TSK_ONCPU);
>   		/*
>   		 * Set TSK_ONCPU on @next's cgroups. If @next shares any
> 
>> ---
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 43e453ab7e20..0d766fb9fbc4 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -2012,7 +2012,7 @@ void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
>>   	if (!(flags & ENQUEUE_NOCLOCK))
>>   		update_rq_clock(rq);
>>   
>> -	if (!(flags & ENQUEUE_RESTORE)) {
>> +	if (!(flags & ENQUEUE_RESTORE) && !p->se.sched_delayed) {
>>   		sched_info_enqueue(rq, p);
>>   		psi_enqueue(p, (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED));
>>   	}
> 
> ... we must be skipping here when we shouldn't.
> 
> I tried moving it past ->enqueue_task(), like we did for uclamp, to
> get the enqueue when sched_delayed is cleared by the callback and task
> is considered properly queued again. However, that results in yet
> another problem:
> 
> [    4.624776] psi: inconsistent task state! task=161:systemd-ssh-gen cpu=1 psi_flags=15 clear=14 set=1
> 
> This is a psi_sched_switch() trying to clear running|queued and set
> iowait. Task is already running|queued|iowait. It looks like we had a
> genuine wakeup that was signaled with psi_enqueue(.wakeup=false) (so
> it didn't clear the iowait).
> 
>> @@ -2039,7 +2039,7 @@ inline bool dequeue_task(struct rq *rq, struct task_struct *p, int flags)
>>   	if (!(flags & DEQUEUE_NOCLOCK))
>>   		update_rq_clock(rq);
>>   
>> -	if (!(flags & DEQUEUE_SAVE)) {
>> +	if (!(flags & DEQUEUE_SAVE) && !p->se.sched_delayed) {
>>   		sched_info_dequeue(rq, p);
>>   		psi_dequeue(p, flags & DEQUEUE_SLEEP);
>>   	}
>> @@ -6537,6 +6537,7 @@ static void __sched notrace __schedule(int sched_mode)
>>   	 * as a preemption by schedule_debug() and RCU.
>>   	 */
>>   	bool preempt = sched_mode > SM_NONE;
>> +	bool block = false;
>>   	unsigned long *switch_count;
>>   	unsigned long prev_state;
>>   	struct rq_flags rf;
>> @@ -6622,6 +6623,7 @@ static void __sched notrace __schedule(int sched_mode)
>>   			 * After this, schedule() must not care about p->state any more.
>>   			 */
>>   			block_task(rq, prev, flags);
>> +			block = true;
>>   		}
>>   		switch_count = &prev->nvcsw;
>>   	}
>> @@ -6667,7 +6669,7 @@ static void __sched notrace __schedule(int sched_mode)
>>   
>>   		migrate_disable_switch(rq, prev);
>>   		psi_account_irqtime(rq, prev, next);
>> -		psi_sched_switch(prev, next, !task_on_rq_queued(prev));
>> +		psi_sched_switch(prev, next, block);
>>   
>>   		trace_sched_switch(preempt, prev, next, prev_state);

Re: [REGRESSION] Re: [PATCH 17/24] sched/fair: Implement delayed dequeue

[K Prateek Nayak]

Hello Peter,

On 10/4/2024 6:05 PM, Peter Zijlstra wrote:
> On Fri, Oct 04, 2024 at 04:40:08PM +0530, K Prateek Nayak wrote:
>> Hello folks,
>>
>> On 10/3/2024 11:01 AM, Klaus Kudielka wrote:
>>> On Sun, 2024-09-22 at 16:45 +0100, Chris Bainbridge wrote:
>>>> On Fri, Aug 30, 2024 at 02:34:56PM +0200, Bert Karwatzki wrote:
>>>>> Since linux next-20240820 the following messages appears when booting:
>>>>>
>>>>> [    T1] smp: Bringing up secondary CPUs ...
>>>>> [    T1] smpboot: x86: Booting SMP configuration:
>>>>> [    T1] .... node  #0, CPUs:        #2  #4  #6  #8 #10 #12 #14  #1
>>>>> This is the line I'm concerend about:
>>>>> [    T1] psi: inconsistent task state! task=61:cpuhp/3 cpu=0 psi_flags=4 clear=0 set=4
>>>>> [    T1]   #3  #5  #7  #9 #11 #13 #15
>>>>> [    T1] Spectre V2 : Update user space SMT mitigation: STIBP always-on
>>>>> [    T1] smp: Brought up 1 node, 16 CPUs
>>>>> [    T1] smpboot: Total of 16 processors activated (102216.16 BogoMIPS)
>>>>>
>>>>> I bisected this to commit 152e11f6df29 ("sched/fair: Implement delayed dequeue").
>>>>> Is this normal or is this something I should worry about?
>>>>>
>>>>> Bert Karwatzki
>>>>
>>>> I am also getting a similar error on boot, and bisected it to the same commit:
>>>>
>>>> [    0.342931] psi: inconsistent task state! task=15:rcu_tasks_trace cpu=0 psi_flags=4 clear=0 set=4
>>>>
>>>> #regzbot introduced: 152e11f6df293e816a6a37c69757033cdc72667d
>>>
>>> Just another data point, while booting 6.12-rc1 on a Turris Omnia:
>>>
>>> [    0.000000] Linux version 6.12.0-rc1 (XXX) (arm-linux-gnueabihf-gcc (Debian 14.2.0-1) 14.2.0, GNU ld (GNU Binutils for Debian) 2.43.1) #1 SMP Thu Oct  3 06:59:25 CEST 2024
>>> [    0.000000] CPU: ARMv7 Processor [414fc091] revision 1 (ARMv7), cr=10c5387d
>>> [    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
>>> [    0.000000] OF: fdt: Machine model: Turris Omnia
>>> ...
>>> [    0.000867] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
>>> [    0.000876] psi: inconsistent task state! task=2:kthreadd cpu=0 psi_flags=4 clear=0 set=4
>>>
>>
>> Not sure if someone took a stab at this but I haven't seen the "psi:
> 
> I'm aware of the issue, but since it's just statistics and not
> anything 'important', I've been spending my time on those crashing bugs.
> 
>> inconsistent task state" warning with the below diff. I'm not sure if my
>> approach is right which if why I'm pasting the diff before sending out
>> an official series. Any comments or testing is greatly appreciated.
>>
>> The diff is based on:
>>
>>      git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git sched/urgent
>>
>> at commit d4ac164bde7a ("sched/eevdf: Fix wakeup-preempt by checking
>> cfs_rq->nr_running")
> 
> Thanks, I just pushed all that out to tip/sched/urgent.
> 
>> My approach was as follows:
>>
>> o psi_dequeue() relied on psi_sched_switch() to set the PSI flags
>>    appropriately for a dequeued task. However, psi_sched_switch() used
>>    "!task_on_rq_queued(prev)" to judge if the prev task is blocked which
>>    is now untrue with DELAYED_DEQUEUE. Fix it by checking
>>    "p->se.sched_delayed" as well. I also added a matching check for
>>    ENQUEUE_DELAYED for psi_enqueue().
> 
> We already determine the whole sleep state earlier, the whole having
> called block_task() is a clue, perhaps we should propagate that state
> instead of trying to divinate it again.

Yup that makes sense!

> 
>> o With the above, the warning was put off for a few more seconds but it
>>    still appeared. I dumped all PSI flag transition along with
>>    "tsk->se.sched_delayed" to see what trips it and I saw the following
>>    state changes for the task that finally tripped it:
>>
>>      psi: task state: task=18:rcu_preempt cpu=0 psi_flags=0 clear=0 set=0 delayed=1
>>      psi: task state: task=18:rcu_preempt cpu=128 psi_flags=0 clear=0 set=4 delayed=1
>>      psi: task state: task=18:rcu_preempt cpu=128 psi_flags=4 clear=0 set=4 delayed=0
>>      psi: inconsistent task state! task=18:rcu_preempt cpu=128 psi_flags=4 clear=0 set=4 delayed=0
>>
>>    Note that cpu switched with "tsk->se.sched_delayed" still set which
>>    got me looking at the task migration path. The warning added below
>>    in "deactivate_task()" tripped without fail, just before the PSI
>>    warning was logged.
>>
>>    To prevent migration of a delayed entity (XXX: Is it a good idea?)
> 
> It is not. By migrating the entities they can get picked sooner and the
> delayed thing gets removed sooner. Less 'hidden' weight.

True that! I was thinking moving queued load could also potentially help
delayed entities being picked faster on the rq where they were delayed.
Both seem to help in one way or the other but I don't have any solid
data to conclusively say which might be better.

> 
>>    we do a "account_task_dequeue()" in the delayed dequeue case to
>>    remove the task from the "rq->cfs_list", thus removing it from the
>>    purview of the load balancer.
> 
> Anyway, assuming PSI wants to preserve current semantics, does something
> like the below work?

I've updated the details from my testing on the parallel thread by
Johannes.

tl;dr I still see PSI warnings, some more tinkering on top of your
changes altered the warning to "psi: task underflow!". So far, no
luck figuring out how that comes about.

Thank you for taking a look and for the quick patch!

-- 
Thanks and Regards,
Prateek

> 
> ---
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 43e453ab7e20..0d766fb9fbc4 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2012,7 +2012,7 @@ void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
>   	if (!(flags & ENQUEUE_NOCLOCK))
>   		update_rq_clock(rq);
>   
> -	if (!(flags & ENQUEUE_RESTORE)) {
> +	if (!(flags & ENQUEUE_RESTORE) && !p->se.sched_delayed) {
>   		sched_info_enqueue(rq, p);
>   		psi_enqueue(p, (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED));
>   	}
> @@ -2039,7 +2039,7 @@ inline bool dequeue_task(struct rq *rq, struct task_struct *p, int flags)
>   	if (!(flags & DEQUEUE_NOCLOCK))
>   		update_rq_clock(rq);
>   
> -	if (!(flags & DEQUEUE_SAVE)) {
> +	if (!(flags & DEQUEUE_SAVE) && !p->se.sched_delayed) {
>   		sched_info_dequeue(rq, p);
>   		psi_dequeue(p, flags & DEQUEUE_SLEEP);
>   	}
> @@ -6537,6 +6537,7 @@ static void __sched notrace __schedule(int sched_mode)
>   	 * as a preemption by schedule_debug() and RCU.
>   	 */
>   	bool preempt = sched_mode > SM_NONE;
> +	bool block = false;
>   	unsigned long *switch_count;
>   	unsigned long prev_state;
>   	struct rq_flags rf;
> @@ -6622,6 +6623,7 @@ static void __sched notrace __schedule(int sched_mode)
>   			 * After this, schedule() must not care about p->state any more.
>   			 */
>   			block_task(rq, prev, flags);
> +			block = true;
>   		}
>   		switch_count = &prev->nvcsw;
>   	}
> @@ -6667,7 +6669,7 @@ static void __sched notrace __schedule(int sched_mode)
>   
>   		migrate_disable_switch(rq, prev);
>   		psi_account_irqtime(rq, prev, next);
> -		psi_sched_switch(prev, next, !task_on_rq_queued(prev));
> +		psi_sched_switch(prev, next, block);
>   
>   		trace_sched_switch(preempt, prev, next, prev_state);
>   

Re: [REGRESSION] Re: [PATCH 17/24] sched/fair: Implement delayed dequeue

[K Prateek Nayak]

Hello Johannes, Peter,

On 10/4/2024 10:13 PM, K Prateek Nayak wrote:
> [..snip..]
>>> Anyway, assuming PSI wants to preserve current semantics, does something
>>> like the below work?
>>
>> This doesn't. But it's a different corruption now:
>>
>> [    2.298408] psi: inconsistent task state! task=24:cpuhp/1 cpu=1 psi_flags=10 clear=14 set=0
> 
> I hit the same log (clear 14, set 0) and I tried the below changes on
> top of Peter's diff:
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 0d766fb9fbc4..9cf3d4359994 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2012,9 +2012,10 @@ void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
>       if (!(flags & ENQUEUE_NOCLOCK))
>           update_rq_clock(rq);
> 
> -    if (!(flags & ENQUEUE_RESTORE) && !p->se.sched_delayed) {
> +    if (!(flags & ENQUEUE_RESTORE) && (!p->se.sched_delayed || (flags & ENQUEUE_DELAYED))) {
>           sched_info_enqueue(rq, p);
> -        psi_enqueue(p, (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED));
> +        psi_enqueue(p, ((flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED)) ||
> +                   (flags & ENQUEUE_DELAYED));
>       }
> 
>       p->sched_class->enqueue_task(rq, p, flags);
> -- 
> 
> ... but it just changes the warning to:
> 
>      psi: task underflow! cpu=65 t=0 tasks=[0 0 0 0] clear=1 set=4
>      psi: task underflow! cpu=31 t=0 tasks=[0 0 1 0] clear=1 set=0
> 
> Doing a dump_stack(), I see it come from psi_enqueue() and
> psi_ttwu_dequeue() and I see "clear=1" as the common theme. I've
> stared at it for a while but I'm at a loss currently. If something
> jumps out, I'll update here.

I could narrow down the crux of the matter to the fact that when a task
is delayed, and the delayed task is then migrated, the wakeup context
may not have any idea that the task was moved from its previous
runqueue. This is the same reason psi_enqueue() considers only ...

     (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED)

... as a wakeup. In case of a wakeup with migration, PSI forgoes
clearing the TSK_IOWAIT flag which seems to be the issue I encountered
in my splat previously.

With that said, the below diff, based on Peter's original approach
currently seems to work for me in the sense that I have not seen the
inconsistent state warning for a while now with my stress test.

Two key points of the approach are:

o It uses "p->migration_flags" to indicate a delayed entity has
   migrated to another runqueue and convey the same during psi_enqueue().

o It adds ENQUEUE_WAKEUP flag alongside ENQUEUE_DELAYED for
   enqueue_task() in ttwu_runnable() since psi_enqueue() needs to know of
   a wakeup without migration to clear the TSK_IOWAIT flag it would have
   set during psi_task_switch() for blocking task and going down the
   stack for enqueue_task_fair(), there seem to be no other observer of
   the ENQUEUE_WAKEUP flag other than psi_enqueue() in the requeue path.

If there are no obvious objections, I'll send a clean patch soon.
In the meantime, here is the diff:

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 43e453ab7e20..885801432e9a 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2009,12 +2009,19 @@ unsigned long get_wchan(struct task_struct *p)
  
  void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
  {
+	bool wakee_not_migrated = (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED);
+
  	if (!(flags & ENQUEUE_NOCLOCK))
  		update_rq_clock(rq);
  
  	if (!(flags & ENQUEUE_RESTORE)) {
  		sched_info_enqueue(rq, p);
-		psi_enqueue(p, (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED));
+
+		/* Notify PSI that the task was migrated in a delayed state before wakeup. */
+		if ((p->migration_flags & DELAYED_MIGRATED) && !task_on_rq_migrating(p)) {
+			wakee_not_migrated = false;
+			p->migration_flags &= ~DELAYED_MIGRATED;
+		}
  	}
  
  	p->sched_class->enqueue_task(rq, p, flags);
@@ -2023,6 +2030,8 @@ void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
  	 * ->sched_delayed.
  	 */
  	uclamp_rq_inc(rq, p);
+	if (!(flags & ENQUEUE_RESTORE))
+		psi_enqueue(p, wakee_not_migrated);
  
  	if (sched_core_enabled(rq))
  		sched_core_enqueue(rq, p);
@@ -2042,6 +2051,9 @@ inline bool dequeue_task(struct rq *rq, struct task_struct *p, int flags)
  	if (!(flags & DEQUEUE_SAVE)) {
  		sched_info_dequeue(rq, p);
  		psi_dequeue(p, flags & DEQUEUE_SLEEP);
+
+		if (p->se.sched_delayed && task_on_rq_migrating(p))
+			p->migration_flags |= DELAYED_MIGRATED;
  	}
  
  	/*
@@ -3733,7 +3745,7 @@ static int ttwu_runnable(struct task_struct *p, int wake_flags)
  	if (task_on_rq_queued(p)) {
  		update_rq_clock(rq);
  		if (p->se.sched_delayed)
-			enqueue_task(rq, p, ENQUEUE_NOCLOCK | ENQUEUE_DELAYED);
+			enqueue_task(rq, p, ENQUEUE_NOCLOCK | ENQUEUE_WAKEUP | ENQUEUE_DELAYED);
  		if (!task_on_cpu(rq, p)) {
  			/*
  			 * When on_rq && !on_cpu the task is preempted, see if
@@ -6537,6 +6549,7 @@ static void __sched notrace __schedule(int sched_mode)
  	 * as a preemption by schedule_debug() and RCU.
  	 */
  	bool preempt = sched_mode > SM_NONE;
+	bool block = false;
  	unsigned long *switch_count;
  	unsigned long prev_state;
  	struct rq_flags rf;
@@ -6622,6 +6635,7 @@ static void __sched notrace __schedule(int sched_mode)
  			 * After this, schedule() must not care about p->state any more.
  			 */
  			block_task(rq, prev, flags);
+			block = true;
  		}
  		switch_count = &prev->nvcsw;
  	}
@@ -6667,7 +6681,7 @@ static void __sched notrace __schedule(int sched_mode)
  
  		migrate_disable_switch(rq, prev);
  		psi_account_irqtime(rq, prev, next);
-		psi_sched_switch(prev, next, !task_on_rq_queued(prev));
+		psi_sched_switch(prev, next, block);
  
  		trace_sched_switch(preempt, prev, next, prev_state);
  
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b1c3588a8f00..2dc2c4cb4f5f 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1326,6 +1326,7 @@ static inline int cpu_of(struct rq *rq)
  }
  
  #define MDF_PUSH		0x01
+#define DELAYED_MIGRATED	0x02 /* Task was migrated when in DELAYED_DEQUEUE state */
  
  static inline bool is_migration_disabled(struct task_struct *p)
  {
diff --git a/kernel/sched/stats.h b/kernel/sched/stats.h
index 237780aa3c53..06a2c6d3ec1e 100644
--- a/kernel/sched/stats.h
+++ b/kernel/sched/stats.h
@@ -129,6 +129,13 @@ static inline void psi_enqueue(struct task_struct *p, bool wakeup)
  	if (static_branch_likely(&psi_disabled))
  		return;
  
+	/*
+	 * Delayed task is not ready to run yet!
+	 * Wait for a requeue before accounting.
+	 */
+	if (p->se.sched_delayed)
+		return;
+
  	if (p->in_memstall)
  		set |= TSK_MEMSTALL_RUNNING;
  
@@ -148,6 +155,9 @@ static inline void psi_dequeue(struct task_struct *p, bool sleep)
  	if (static_branch_likely(&psi_disabled))
  		return;
  
+	/* Delayed task can only be dequeued for migration. */
+	WARN_ON_ONCE(p->se.sched_delayed && sleep);
+
  	/*
  	 * A voluntary sleep is a dequeue followed by a task switch. To
  	 * avoid walking all ancestors twice, psi_task_switch() handles
--

Any and all suggestions are highly appreciated.

> 
> Thank you again both for taking a look.
> 

-- 
Thanks and Regards,
Prateek

Re: [REGRESSION] Re: [PATCH 17/24] sched/fair: Implement delayed dequeue

[K Prateek Nayak]

[-- Attachment #1: Type: text/plain, Size: 7659 bytes --]

Hello folks,

On 10/8/2024 9:08 PM, K Prateek Nayak wrote:
> Hello Johannes, Peter,
> 
> On 10/4/2024 10:13 PM, K Prateek Nayak wrote:
>> [..snip..]
>>>> Anyway, assuming PSI wants to preserve current semantics, does something
>>>> like the below work?
>>>
>>> This doesn't. But it's a different corruption now:
>>>
>>> [    2.298408] psi: inconsistent task state! task=24:cpuhp/1 cpu=1 psi_flags=10 clear=14 set=0
>>
>> I hit the same log (clear 14, set 0) and I tried the below changes on
>> top of Peter's diff:
>>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 0d766fb9fbc4..9cf3d4359994 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -2012,9 +2012,10 @@ void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
>>       if (!(flags & ENQUEUE_NOCLOCK))
>>           update_rq_clock(rq);
>>
>> -    if (!(flags & ENQUEUE_RESTORE) && !p->se.sched_delayed) {
>> +    if (!(flags & ENQUEUE_RESTORE) && (!p->se.sched_delayed || (flags & ENQUEUE_DELAYED))) {
>>           sched_info_enqueue(rq, p);
>> -        psi_enqueue(p, (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED));
>> +        psi_enqueue(p, ((flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED)) ||
>> +                   (flags & ENQUEUE_DELAYED));
>>       }
>>
>>       p->sched_class->enqueue_task(rq, p, flags);
>> -- 
>>
>> ... but it just changes the warning to:
>>
>>      psi: task underflow! cpu=65 t=0 tasks=[0 0 0 0] clear=1 set=4
>>      psi: task underflow! cpu=31 t=0 tasks=[0 0 1 0] clear=1 set=0
>>
>> Doing a dump_stack(), I see it come from psi_enqueue() and
>> psi_ttwu_dequeue() and I see "clear=1" as the common theme. I've
>> stared at it for a while but I'm at a loss currently. If something
>> jumps out, I'll update here.
> 
> I could narrow down the crux of the matter to the fact that when a task
> is delayed, and the delayed task is then migrated, the wakeup context
> may not have any idea that the task was moved from its previous
> runqueue. This is the same reason psi_enqueue() considers only ...
> 
>      (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED)
> 
> ... as a wakeup. In case of a wakeup with migration, PSI forgoes
> clearing the TSK_IOWAIT flag which seems to be the issue I encountered
> in my splat previously.
> 
> With that said, the below diff, based on Peter's original approach
> currently seems to work for me in the sense that I have not seen the
> inconsistent state warning for a while now with my stress test.
> 
> Two key points of the approach are:
> 
> o It uses "p->migration_flags" to indicate a delayed entity has
>    migrated to another runqueue and convey the same during psi_enqueue().
> 
> o It adds ENQUEUE_WAKEUP flag alongside ENQUEUE_DELAYED for
>    enqueue_task() in ttwu_runnable() since psi_enqueue() needs to know of
>    a wakeup without migration to clear the TSK_IOWAIT flag it would have
>    set during psi_task_switch() for blocking task and going down the
>    stack for enqueue_task_fair(), there seem to be no other observer of
>    the ENQUEUE_WAKEUP flag other than psi_enqueue() in the requeue path.
> 
> If there are no obvious objections, I'll send a clean patch soon.
> In the meantime, here is the diff:
> [..snip..]

That last diff was malformed! Sorry for the noise. Here is the corrected
diff:

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 43e453ab7e20..885801432e9a 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2009,12 +2009,19 @@ unsigned long get_wchan(struct task_struct *p)
  
  void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
  {
+	bool wakee_not_migrated = (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED);
+
  	if (!(flags & ENQUEUE_NOCLOCK))
  		update_rq_clock(rq);
  
  	if (!(flags & ENQUEUE_RESTORE)) {
  		sched_info_enqueue(rq, p);
-		psi_enqueue(p, (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED));
+
+		/* Notify PSI that the task was migrated in a delayed state before wakeup. */
+		if ((p->migration_flags & DELAYED_MIGRATED) && !task_on_rq_migrating(p)) {
+			wakee_not_migrated = false;
+			p->migration_flags &= ~DELAYED_MIGRATED;
+		}
  	}
  
  	p->sched_class->enqueue_task(rq, p, flags);
@@ -2023,6 +2030,8 @@ void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
  	 * ->sched_delayed.
  	 */
  	uclamp_rq_inc(rq, p);
+	if (!(flags & ENQUEUE_RESTORE))
+		psi_enqueue(p, wakee_not_migrated);
  
  	if (sched_core_enabled(rq))
  		sched_core_enqueue(rq, p);
@@ -2042,6 +2051,9 @@ inline bool dequeue_task(struct rq *rq, struct task_struct *p, int flags)
  	if (!(flags & DEQUEUE_SAVE)) {
  		sched_info_dequeue(rq, p);
  		psi_dequeue(p, flags & DEQUEUE_SLEEP);
+
+		if (p->se.sched_delayed && task_on_rq_migrating(p))
+			p->migration_flags |= DELAYED_MIGRATED;
  	}
  
  	/*
@@ -3733,7 +3745,7 @@ static int ttwu_runnable(struct task_struct *p, int wake_flags)
  	if (task_on_rq_queued(p)) {
  		update_rq_clock(rq);
  		if (p->se.sched_delayed)
-			enqueue_task(rq, p, ENQUEUE_NOCLOCK | ENQUEUE_DELAYED);
+			enqueue_task(rq, p, ENQUEUE_NOCLOCK | ENQUEUE_WAKEUP | ENQUEUE_DELAYED);
  		if (!task_on_cpu(rq, p)) {
  			/*
  			 * When on_rq && !on_cpu the task is preempted, see if
@@ -6537,6 +6549,7 @@ static void __sched notrace __schedule(int sched_mode)
  	 * as a preemption by schedule_debug() and RCU.
  	 */
  	bool preempt = sched_mode > SM_NONE;
+	bool block = false;
  	unsigned long *switch_count;
  	unsigned long prev_state;
  	struct rq_flags rf;
@@ -6622,6 +6635,7 @@ static void __sched notrace __schedule(int sched_mode)
  			 * After this, schedule() must not care about p->state any more.
  			 */
  			block_task(rq, prev, flags);
+			block = true;
  		}
  		switch_count = &prev->nvcsw;
  	}
@@ -6667,7 +6681,7 @@ static void __sched notrace __schedule(int sched_mode)
  
  		migrate_disable_switch(rq, prev);
  		psi_account_irqtime(rq, prev, next);
-		psi_sched_switch(prev, next, !task_on_rq_queued(prev));
+		psi_sched_switch(prev, next, block);
  
  		trace_sched_switch(preempt, prev, next, prev_state);
  
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b1c3588a8f00..2dc2c4cb4f5f 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1326,6 +1326,7 @@ static inline int cpu_of(struct rq *rq)
  }
  
  #define MDF_PUSH		0x01
+#define DELAYED_MIGRATED	0x02 /* Task was migrated when in DELAYED_DEQUEUE state */
  
  static inline bool is_migration_disabled(struct task_struct *p)
  {
diff --git a/kernel/sched/stats.h b/kernel/sched/stats.h
index 237780aa3c53..06a2c6d3ec1e 100644
--- a/kernel/sched/stats.h
+++ b/kernel/sched/stats.h
@@ -129,6 +129,13 @@ static inline void psi_enqueue(struct task_struct *p, bool wakeup)
  	if (static_branch_likely(&psi_disabled))
  		return;
  
+	/*
+	 * Delayed task is not ready to run yet!
+	 * Wait for a requeue before accounting.
+	 */
+	if (p->se.sched_delayed)
+		return;
+
  	if (p->in_memstall)
  		set |= TSK_MEMSTALL_RUNNING;
  
@@ -148,6 +155,9 @@ static inline void psi_dequeue(struct task_struct *p, bool sleep)
  	if (static_branch_likely(&psi_disabled))
  		return;
  
+	/* Delayed task can only be dequeued for migration. */
+	WARN_ON_ONCE(p->se.sched_delayed && sleep);
+
  	/*
  	 * A voluntary sleep is a dequeue followed by a task switch. To
  	 * avoid walking all ancestors twice, psi_task_switch() handles
--

Attaching an RFC patch in case I messed this up as well!

> -- 
> 
> Any and all suggestions are highly appreciated.
> 
>>
>> Thank you again both for taking a look.
>>
> 

-- 
Thanks and Regards,
Prateek

[-- Attachment #2: 0001-sched-psi-Fixup-PSI-accounting-with-DELAY_DEQUEUE.patch --]
[-- Type: text/plain, Size: 5869 bytes --]

From 2e15180e18b51e9a2bc0d7050e915a70d2673a06 Mon Sep 17 00:00:00 2001
From: K Prateek Nayak <kprateek.nayak@amd.com>
Date: Fri, 4 Oct 2024 15:24:35 +0000
Subject: [RFC PATCH] sched/psi: Fixup PSI accounting with DELAY_DEQUEUE

After the merge of DELAY_DEQUEUE, "psi: inconsistent task state: warning
were seen early into the boot. The crux of the matter is the fact that
when a task is delayed, and the delayed task is then migrated, the
wakeup context may not have any idea that the task was moved from its
previous runqueue. This is the same reason psi_enqueue() considers
only ...

     (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED)

... as a wakeup. In case of a wakeup with migration, PSI forgoes
clearing the TSK_IOWAIT flag which seems to be the issue I encountered
in my splat previously.

With that said, the below diff, based on Peter's original approach
currently seems to work for me in the sense that I have not seen the
inconsistent state warning for a while now with my stress test.

Two key points of the approach are:

o It uses "p->migration_flags" to indicate a delayed entity has
   migrated to another runqueue and convey the same during psi_enqueue().

o It adds ENQUEUE_WAKEUP flag alongside ENQUEUE_DELAYED for
   enqueue_task() in ttwu_runnable() since psi_enqueue() needs to know of
   a wakeup without migration to clear the TSK_IOWAIT flag it would have
   set during psi_task_switch() for blocking task and going down the
   stack for enqueue_task_fair(), there seem to be no other observer of
   the ENQUEUE_WAKEUP flag other than psi_enqueue() in the requeue path.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
 kernel/sched/core.c  | 20 +++++++++++++++++---
 kernel/sched/sched.h |  1 +
 kernel/sched/stats.h | 10 ++++++++++
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 43e453ab7e20..885801432e9a 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2009,12 +2009,19 @@ unsigned long get_wchan(struct task_struct *p)
 
 void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
 {
+	bool wakee_not_migrated = (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED);
+
 	if (!(flags & ENQUEUE_NOCLOCK))
 		update_rq_clock(rq);
 
 	if (!(flags & ENQUEUE_RESTORE)) {
 		sched_info_enqueue(rq, p);
-		psi_enqueue(p, (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED));
+
+		/* Notify PSI that the task was migrated in a delayed state before wakeup. */
+		if ((p->migration_flags & DELAYED_MIGRATED) && !task_on_rq_migrating(p)) {
+			wakee_not_migrated = false;
+			p->migration_flags &= ~DELAYED_MIGRATED;
+		}
 	}
 
 	p->sched_class->enqueue_task(rq, p, flags);
@@ -2023,6 +2030,8 @@ void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
 	 * ->sched_delayed.
 	 */
 	uclamp_rq_inc(rq, p);
+	if (!(flags & ENQUEUE_RESTORE))
+		psi_enqueue(p, wakee_not_migrated);
 
 	if (sched_core_enabled(rq))
 		sched_core_enqueue(rq, p);
@@ -2042,6 +2051,9 @@ inline bool dequeue_task(struct rq *rq, struct task_struct *p, int flags)
 	if (!(flags & DEQUEUE_SAVE)) {
 		sched_info_dequeue(rq, p);
 		psi_dequeue(p, flags & DEQUEUE_SLEEP);
+
+		if (p->se.sched_delayed && task_on_rq_migrating(p))
+			p->migration_flags |= DELAYED_MIGRATED;
 	}
 
 	/*
@@ -3733,7 +3745,7 @@ static int ttwu_runnable(struct task_struct *p, int wake_flags)
 	if (task_on_rq_queued(p)) {
 		update_rq_clock(rq);
 		if (p->se.sched_delayed)
-			enqueue_task(rq, p, ENQUEUE_NOCLOCK | ENQUEUE_DELAYED);
+			enqueue_task(rq, p, ENQUEUE_NOCLOCK | ENQUEUE_WAKEUP | ENQUEUE_DELAYED);
 		if (!task_on_cpu(rq, p)) {
 			/*
 			 * When on_rq && !on_cpu the task is preempted, see if
@@ -6537,6 +6549,7 @@ static void __sched notrace __schedule(int sched_mode)
 	 * as a preemption by schedule_debug() and RCU.
 	 */
 	bool preempt = sched_mode > SM_NONE;
+	bool block = false;
 	unsigned long *switch_count;
 	unsigned long prev_state;
 	struct rq_flags rf;
@@ -6622,6 +6635,7 @@ static void __sched notrace __schedule(int sched_mode)
 			 * After this, schedule() must not care about p->state any more.
 			 */
 			block_task(rq, prev, flags);
+			block = true;
 		}
 		switch_count = &prev->nvcsw;
 	}
@@ -6667,7 +6681,7 @@ static void __sched notrace __schedule(int sched_mode)
 
 		migrate_disable_switch(rq, prev);
 		psi_account_irqtime(rq, prev, next);
-		psi_sched_switch(prev, next, !task_on_rq_queued(prev));
+		psi_sched_switch(prev, next, block);
 
 		trace_sched_switch(preempt, prev, next, prev_state);
 
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b1c3588a8f00..2dc2c4cb4f5f 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1326,6 +1326,7 @@ static inline int cpu_of(struct rq *rq)
 }
 
 #define MDF_PUSH		0x01
+#define DELAYED_MIGRATED	0x02 /* Task was migrated when in DELAYED_DEQUEUE state */
 
 static inline bool is_migration_disabled(struct task_struct *p)
 {
diff --git a/kernel/sched/stats.h b/kernel/sched/stats.h
index 237780aa3c53..06a2c6d3ec1e 100644
--- a/kernel/sched/stats.h
+++ b/kernel/sched/stats.h
@@ -129,6 +129,13 @@ static inline void psi_enqueue(struct task_struct *p, bool wakeup)
 	if (static_branch_likely(&psi_disabled))
 		return;
 
+	/*
+	 * Delayed task is not ready to run yet!
+	 * Wait for a requeue before accounting.
+	 */
+	if (p->se.sched_delayed)
+		return;
+
 	if (p->in_memstall)
 		set |= TSK_MEMSTALL_RUNNING;
 
@@ -148,6 +155,9 @@ static inline void psi_dequeue(struct task_struct *p, bool sleep)
 	if (static_branch_likely(&psi_disabled))
 		return;
 
+	/* Delayed task can only be dequeued for migration. */
+	WARN_ON_ONCE(p->se.sched_delayed && sleep);
+
 	/*
 	 * A voluntary sleep is a dequeue followed by a task switch. To
 	 * avoid walking all ancestors twice, psi_task_switch() handles
-- 
2.34.1

Re: [REGRESSION] Re: [PATCH 17/24] sched/fair: Implement delayed dequeue

[Johannes Weiner]

On Tue, Oct 08, 2024 at 09:54:52PM +0530, K Prateek Nayak wrote:
> From 2e15180e18b51e9a2bc0d7050e915a70d2673a06 Mon Sep 17 00:00:00 2001
> From: K Prateek Nayak <kprateek.nayak@amd.com>
> Date: Fri, 4 Oct 2024 15:24:35 +0000
> Subject: [RFC PATCH] sched/psi: Fixup PSI accounting with DELAY_DEQUEUE
> 
> After the merge of DELAY_DEQUEUE, "psi: inconsistent task state: warning
> were seen early into the boot. The crux of the matter is the fact that
> when a task is delayed, and the delayed task is then migrated, the
> wakeup context may not have any idea that the task was moved from its
> previous runqueue. This is the same reason psi_enqueue() considers
> only ...
> 
>      (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED)
> 
> ... as a wakeup. In case of a wakeup with migration, PSI forgoes
> clearing the TSK_IOWAIT flag which seems to be the issue I encountered
> in my splat previously.
> 
> With that said, the below diff, based on Peter's original approach
> currently seems to work for me in the sense that I have not seen the
> inconsistent state warning for a while now with my stress test.
> 
> Two key points of the approach are:
> 
> o It uses "p->migration_flags" to indicate a delayed entity has
>    migrated to another runqueue and convey the same during psi_enqueue().
> 
> o It adds ENQUEUE_WAKEUP flag alongside ENQUEUE_DELAYED for
>    enqueue_task() in ttwu_runnable() since psi_enqueue() needs to know of
>    a wakeup without migration to clear the TSK_IOWAIT flag it would have
>    set during psi_task_switch() for blocking task and going down the
>    stack for enqueue_task_fair(), there seem to be no other observer of
>    the ENQUEUE_WAKEUP flag other than psi_enqueue() in the requeue path.
> 
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>

Tested-by: Johannes Weiner <hannes@cmpxchg.org>

It fixes the warning and bogus pressure values after stressing it for
an hour or so with tons of cpu contention and cgroup movements.

Re: [REGRESSION] Re: [PATCH 17/24] sched/fair: Implement delayed dequeue

[K Prateek Nayak]

Hello Johannes,

On 10/9/2024 11:37 PM, Johannes Weiner wrote:
> On Tue, Oct 08, 2024 at 09:54:52PM +0530, K Prateek Nayak wrote:
>>  From 2e15180e18b51e9a2bc0d7050e915a70d2673a06 Mon Sep 17 00:00:00 2001
>> From: K Prateek Nayak <kprateek.nayak@amd.com>
>> Date: Fri, 4 Oct 2024 15:24:35 +0000
>> Subject: [RFC PATCH] sched/psi: Fixup PSI accounting with DELAY_DEQUEUE
>>
>> After the merge of DELAY_DEQUEUE, "psi: inconsistent task state: warning
>> were seen early into the boot. The crux of the matter is the fact that
>> when a task is delayed, and the delayed task is then migrated, the
>> wakeup context may not have any idea that the task was moved from its
>> previous runqueue. This is the same reason psi_enqueue() considers
>> only ...
>>
>>       (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED)
>>
>> ... as a wakeup. In case of a wakeup with migration, PSI forgoes
>> clearing the TSK_IOWAIT flag which seems to be the issue I encountered
>> in my splat previously.
>>
>> With that said, the below diff, based on Peter's original approach
>> currently seems to work for me in the sense that I have not seen the
>> inconsistent state warning for a while now with my stress test.
>>
>> Two key points of the approach are:
>>
>> o It uses "p->migration_flags" to indicate a delayed entity has
>>     migrated to another runqueue and convey the same during psi_enqueue().
>>
>> o It adds ENQUEUE_WAKEUP flag alongside ENQUEUE_DELAYED for
>>     enqueue_task() in ttwu_runnable() since psi_enqueue() needs to know of
>>     a wakeup without migration to clear the TSK_IOWAIT flag it would have
>>     set during psi_task_switch() for blocking task and going down the
>>     stack for enqueue_task_fair(), there seem to be no other observer of
>>     the ENQUEUE_WAKEUP flag other than psi_enqueue() in the requeue path.
>>
>> Suggested-by: Peter Zijlstra <peterz@infradead.org>
>> Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
> 
> Tested-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> It fixes the warning and bogus pressure values after stressing it for
> an hour or so with tons of cpu contention and cgroup movements.

Thanks a ton for testing. I'll send a proper series soon for folks to
review.
-- 
Thanks and Regards,
Prateek