[PATCH 0/1] sched/fair: Update blocked averages on tick

[PATCH 0/1] sched/fair: Update blocked averages on tick

[Pierre Gondois]

The patch below was tested on a Pixel6 based on v6.8. The patch was
then ported to v6.12-rc2. The energy consumption of the Pixel6 is:
- measured using the phone's energy counters
- estimated using util signals, cluster frequency and the Energy Model

An rt-app workload runs during 20s, 5 iterations. 'count' tasks are created
with a utilization of 'util'%. Task's period is 16ms.

Measured energy:
+-------------+------+-------+-----------+------------+------------+
| granularity | util | count | base      | with patch | ratio      |
+-------------+------+-------+-----------+------------+------------+
| 0           | 5    | 8     |  5703.759 |  5624.371  | - 1.391    |
| 0           | 5    | 16    | 15021.425 | 14540.839  | - 3.199    |
| 0           | 5    | 24    | 30018.226 | 27046.899  | - 9.898    |
| 0           | 10   | 8     | 12843.869 | 12346.898  | - 3.869    |
| 0           | 10   | 16    | 45925.643 | 43081.072  | - 6.193    |
| 2           | 5    | 8     |  6288.044 |  6174.609  | - 1.803    |
| 2           | 5    | 16    | 15657.632 | 15037.596  | - 3.959    |
| 2           | 5    | 24    | 31483.032 | 27205.665  | -13.58     |
| 2           | 10   | 8     | 14003.121 | 12675.037  | - 9.484    |
| 2           | 10   | 16    | 45005.382 | 42088.515  | - 6.481    |
+-------------+------+-------+-----------+------------+------------+

Computed energy:
+-------------+------+-------+-----------+------------+---------+
| granularity | util | count | base      | with patch | ratio   |
+-------------+------+-------+-----------+------------+---------+
| 0           | 5    | 8     | 2.5764e10 | 2.4581e10  | - 4.591 |
| 0           | 5    | 16    | 7.0705e10 | 6.2580e10  | -11.490 |
| 0           | 5    | 24    | 1.5034e11 | 1.1889e11  | -20.916 |
| 0           | 10   | 8     | 4.7312e10 | 4.1573e10  | -12.131 |
| 0           | 10   | 16    | 1.7860e11 | 1.4748e11  | -17.423 |
| 2           | 5    | 8     | 3.0811e10 | 2.8950e10  | - 6.040 |
| 2           | 5    | 16    | 8.3871e10 | 7.1320e10  | -14.963 |
| 2           | 5    | 24    | 1.3414e11 | 1.2771e11  | - 4.788 |
| 2           | 10   | 8     | 5.8014e10 | 4.5861e10  | -20.949 |
| 2           | 10   | 16    | 1.8283e11 | 1.6708e11  | - 8.613 |
+-------------+------+-------+-----------+------------+---------+

Pierre Gondois (1):
  sched/fair: Update blocked averages on tick

 kernel/sched/fair.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH 1/1] sched/fair: Update blocked averages on tick

[Pierre Gondois]

The Energy Aware Scheduler (EAS) relies on CPU/tasks utilization
signals. On an idle CPU, the blocked load is updated during
load balancing.

sd->balance_interval increases with the number of CPUs in the domain.
On an Arm DynamIQ system, sched domains containing CPUs with the same
capacity do not exist. On a Pixel6 with 4 little, 2 mid, 2 big CPUs:
- sd->min_interval = 8
- sd->min_interval = 16

The balance interval is doubled if the system is balanced, meaning
that a balanced system will likely update blocked load every 16ms.

The find_energy_efficient_cpu() function might thus relies on outdated
util signals to place tasks, leading to bad energy placement.

Update blocked load on sched tick if:
- the rq is idle
- the load balancer will not be triggered.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 kernel/sched/fair.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 225b31aaee55..2f03bd10ac7a 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9841,15 +9841,12 @@ static unsigned long task_h_load(struct task_struct *p)
 }
 #endif
 
-static void sched_balance_update_blocked_averages(int cpu)
+static void update_blocked_averages(struct rq *rq)
 {
 	bool decayed = false, done = true;
-	struct rq *rq = cpu_rq(cpu);
-	struct rq_flags rf;
 
-	rq_lock_irqsave(rq, &rf);
-	update_blocked_load_tick(rq);
 	update_rq_clock(rq);
+	update_blocked_load_tick(rq);
 
 	decayed |= __update_blocked_others(rq, &done);
 	decayed |= __update_blocked_fair(rq, &done);
@@ -9857,6 +9854,18 @@ static void sched_balance_update_blocked_averages(int cpu)
 	update_blocked_load_status(rq, !done);
 	if (decayed)
 		cpufreq_update_util(rq, 0);
+}
+
+static void sched_balance_update_blocked_averages(int cpu)
+{
+	struct rq *rq = cpu_rq(cpu);
+	struct cfs_rq *cfs_rq;
+	struct rq_flags rf;
+
+	cfs_rq = &rq->cfs;
+
+	rq_lock_irqsave(rq, &rf);
+	update_blocked_averages(rq);
 	rq_unlock_irqrestore(rq, &rf);
 }
 
@@ -12877,6 +12886,8 @@ void sched_balance_trigger(struct rq *rq)
 
 	if (time_after_eq(jiffies, rq->next_balance))
 		raise_softirq(SCHED_SOFTIRQ);
+	else if (idle_cpu(rq->cpu))
+		update_blocked_averages(rq);
 
 	nohz_balancer_kick(rq);
 }
-- 
2.25.1

Re: [PATCH 0/1] sched/fair: Update blocked averages on tick

[Pierre Gondois]

On 10/11/24 14:32, Pierre Gondois wrote:
> The patch below was tested on a Pixel6 based on v6.8. The patch was
> then ported to v6.12-rc2. The energy consumption of the Pixel6 is:
> - measured using the phone's energy counters
> - estimated using util signals, cluster frequency and the Energy Model
> 
> An rt-app workload runs during 20s, 5 iterations. 'count' tasks are created
> with a utilization of 'util'%. Task's period is 16ms.
> 
> Measured energy:
> +-------------+------+-------+-----------+------------+------------+
> | granularity | util | count | base      | with patch | ratio      |
> +-------------+------+-------+-----------+------------+------------+
> | 0           | 5    | 8     |  5703.759 |  5624.371  | - 1.391    |
> | 0           | 5    | 16    | 15021.425 | 14540.839  | - 3.199    |
> | 0           | 5    | 24    | 30018.226 | 27046.899  | - 9.898    |
> | 0           | 10   | 8     | 12843.869 | 12346.898  | - 3.869    |
> | 0           | 10   | 16    | 45925.643 | 43081.072  | - 6.193    |

I forgot to trimm a configuration off the results. Please ignore the
lines with a 'granularity=2'.

8>
> | 2           | 5    | 8     |  6288.044 |  6174.609  | - 1.803    |
> | 2           | 5    | 16    | 15657.632 | 15037.596  | - 3.959    |
> | 2           | 5    | 24    | 31483.032 | 27205.665  | -13.58     |
> | 2           | 10   | 8     | 14003.121 | 12675.037  | - 9.484    |
> | 2           | 10   | 16    | 45005.382 | 42088.515  | - 6.481    |
<8
> +-------------+------+-------+-----------+------------+------------+
> 
> Computed energy:
> +-------------+------+-------+-----------+------------+---------+
> | granularity | util | count | base      | with patch | ratio   |
> +-------------+------+-------+-----------+------------+---------+
> | 0           | 5    | 8     | 2.5764e10 | 2.4581e10  | - 4.591 |
> | 0           | 5    | 16    | 7.0705e10 | 6.2580e10  | -11.490 |
> | 0           | 5    | 24    | 1.5034e11 | 1.1889e11  | -20.916 |
> | 0           | 10   | 8     | 4.7312e10 | 4.1573e10  | -12.131 |
> | 0           | 10   | 16    | 1.7860e11 | 1.4748e11  | -17.423 |

8>
> | 2           | 5    | 8     | 3.0811e10 | 2.8950e10  | - 6.040 |
> | 2           | 5    | 16    | 8.3871e10 | 7.1320e10  | -14.963 |
> | 2           | 5    | 24    | 1.3414e11 | 1.2771e11  | - 4.788 |
> | 2           | 10   | 8     | 5.8014e10 | 4.5861e10  | -20.949 |
> | 2           | 10   | 16    | 1.8283e11 | 1.6708e11  | - 8.613 |
<8
> +-------------+------+-------+-----------+------------+---------+
> 
> Pierre Gondois (1):
>    sched/fair: Update blocked averages on tick
> 
>   kernel/sched/fair.c | 21 ++++++++++++++++-----
>   1 file changed, 16 insertions(+), 5 deletions(-)
> 

Re: [PATCH 1/1] sched/fair: Update blocked averages on tick

[kernel test robot]

Hello,

kernel test robot noticed "WARNING:at_kernel/sched/sched.h:#update_rq_clock" on:

commit: 8250cf6004de715b0abbbd5bf837d5ac869f1641 ("[PATCH 1/1] sched/fair: Update blocked averages on tick")
url: https://github.com/intel-lab-lkp/linux/commits/Pierre-Gondois/sched-fair-Update-blocked-averages-on-tick/20241011-203402
base: https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git 7266f0a6d3bb73f42ea06656d3cc48c7d0386f71
patch link: https://lore.kernel.org/all/20241011123222.1282936-2-pierre.gondois@arm.com/
patch subject: [PATCH 1/1] sched/fair: Update blocked averages on tick

in testcase: boot

config: x86_64-randconfig-006-20241013
compiler: clang-18
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G

(please refer to attached dmesg/kmsg for entire log/backtrace)


+-------------------------------------------------------------+------------+------------+
|                                                             | 7266f0a6d3 | 8250cf6004 |
+-------------------------------------------------------------+------------+------------+
| boot_successes                                              | 18         | 0          |
| boot_failures                                               | 0          | 21         |
| WARNING:at_kernel/sched/sched.h:#update_rq_clock            | 0          | 21         |
| RIP:update_rq_clock                                         | 0          | 21         |
| RIP:default_idle                                            | 0          | 21         |
| WARNING:at_kernel/sched/sched.h:#_update_idle_rq_clock_pelt | 0          | 21         |
| RIP:_update_idle_rq_clock_pelt                              | 0          | 21         |
| WARNING:at_kernel/sched/sched.h:#update_other_load_avgs     | 0          | 21         |
| RIP:update_other_load_avgs                                  | 0          | 21         |
| WARNING:at_kernel/sched/sched.h:#update_blocked_averages    | 0          | 21         |
| RIP:update_blocked_averages                                 | 0          | 21         |
+-------------------------------------------------------------+------------+------------+


If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202410141238.aa194a45-lkp@intel.com


[    2.078083][    C1] ------------[ cut here ]------------
[ 2.079049][ C1] WARNING: CPU: 1 PID: 0 at kernel/sched/sched.h:1496 update_rq_clock (kernel/sched/sched.h:1496) 
[    2.080428][    C1] Modules linked in:
[    2.081070][    C1] CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Not tainted 6.12.0-rc2-00014-g8250cf6004de #1 dfe5ae60b9ab8d21e49756916bb824f564f72e7b
[    2.081391][    C1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 2.081391][ C1] RIP: 0010:update_rq_clock (kernel/sched/sched.h:1496) 
[ 2.081391][ C1] Code: 00 00 48 89 d8 48 c1 e8 03 42 80 3c 28 00 74 08 48 89 df e8 c7 38 41 00 4c 01 3b 48 83 c4 18 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b e9 52 fd ff ff 48 89 df e8 fe 38 01 00 eb e0 48 c7 c1 3c 21
All code
========
   0:	00 00                	add    %al,(%rax)
   2:	48 89 d8             	mov    %rbx,%rax
   5:	48 c1 e8 03          	shr    $0x3,%rax
   9:	42 80 3c 28 00       	cmpb   $0x0,(%rax,%r13,1)
   e:	74 08                	je     0x18
  10:	48 89 df             	mov    %rbx,%rdi
  13:	e8 c7 38 41 00       	call   0x4138df
  18:	4c 01 3b             	add    %r15,(%rbx)
  1b:	48 83 c4 18          	add    $0x18,%rsp
  1f:	5b                   	pop    %rbx
  20:	41 5c                	pop    %r12
  22:	41 5d                	pop    %r13
  24:	41 5e                	pop    %r14
  26:	41 5f                	pop    %r15
  28:	5d                   	pop    %rbp
  29:	c3                   	ret
  2a:*	0f 0b                	ud2		<-- trapping instruction
  2c:	e9 52 fd ff ff       	jmp    0xfffffffffffffd83
  31:	48 89 df             	mov    %rbx,%rdi
  34:	e8 fe 38 01 00       	call   0x13937
  39:	eb e0                	jmp    0x1b
  3b:	48                   	rex.W
  3c:	c7                   	.byte 0xc7
  3d:	c1                   	.byte 0xc1
  3e:	3c 21                	cmp    $0x21,%al

Code starting with the faulting instruction
===========================================
   0:	0f 0b                	ud2
   2:	e9 52 fd ff ff       	jmp    0xfffffffffffffd59
   7:	48 89 df             	mov    %rbx,%rdi
   a:	e8 fe 38 01 00       	call   0x1390d
   f:	eb e0                	jmp    0xfffffffffffffff1
  11:	48                   	rex.W
  12:	c7                   	.byte 0xc7
  13:	c1                   	.byte 0xc1
  14:	3c 21                	cmp    $0x21,%al
[    2.081391][    C1] RSP: 0000:ffffc900001e8df0 EFLAGS: 00010046
[    2.081391][    C1] RAX: 0000000000000000 RBX: ffff8883aeb396c0 RCX: dffffc0000000000
[    2.081391][    C1] RDX: 0000000000000000 RSI: 00000000ffffffff RDI: ffff8883aeb396d8
[    2.081391][    C1] RBP: ffffc900001e8e30 R08: ffffffffb6af076f R09: 1ffffffff6d5e0ed
[    2.081391][    C1] R10: dffffc0000000000 R11: fffffbfff6d5e0ee R12: dffffc0000000000
[    2.081391][    C1] R13: dffffc0000000000 R14: ffff8883aeb3a158 R15: ffff8883aeb3a088
[    2.081391][    C1] FS:  0000000000000000(0000) GS:ffff8883aeb00000(0000) knlGS:0000000000000000
[    2.081391][    C1] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    2.081391][    C1] CR2: 0000000000000000 CR3: 0000000182a4c000 CR4: 00000000000406f0
[    2.081391][    C1] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    2.081391][    C1] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[    2.081391][    C1] Call Trace:
[    2.081391][    C1]  <IRQ>
[ 2.081391][ C1] ? show_regs (arch/x86/kernel/dumpstack.c:479) 
[ 2.081391][ C1] ? __warn (kernel/panic.c:748) 
[ 2.081391][ C1] ? update_rq_clock (kernel/sched/sched.h:1496) 
[ 2.081391][ C1] ? report_bug (lib/bug.c:?) 
[ 2.081391][ C1] ? update_rq_clock (kernel/sched/sched.h:1496) 
[ 2.081391][ C1] ? handle_bug (arch/x86/kernel/traps.c:285) 
[ 2.081391][ C1] ? exc_invalid_op (arch/x86/kernel/traps.c:309) 
[ 2.081391][ C1] ? asm_exc_invalid_op (arch/x86/include/asm/idtentry.h:621) 
[ 2.081391][ C1] ? update_rq_clock (kernel/sched/sched.h:1496) 
[ 2.081391][ C1] update_blocked_averages (kernel/sched/fair.c:9720 kernel/sched/fair.c:9845) 
[ 2.081391][ C1] sched_balance_trigger (kernel/sched/fair.c:12887) 
[ 2.081391][ C1] sched_tick (kernel/sched/core.c:5617) 
[ 2.081391][ C1] update_process_times (kernel/time/timer.c:2526) 
[ 2.081391][ C1] tick_periodic (kernel/time/tick-common.c:102) 
[ 2.081391][ C1] tick_handle_periodic (kernel/time/tick-common.c:120) 
[ 2.081391][ C1] __sysvec_apic_timer_interrupt (arch/x86/include/asm/jump_label.h:27 include/linux/jump_label.h:207 arch/x86/include/asm/trace/irq_vectors.h:41 arch/x86/kernel/apic/apic.c:1044) 
[ 2.081391][ C1] sysvec_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:1037) 
[    2.081391][    C1]  </IRQ>
[    2.081391][    C1]  <TASK>
[ 2.081391][ C1] asm_sysvec_apic_timer_interrupt (arch/x86/include/asm/idtentry.h:702) 
[ 2.081391][ C1] RIP: 0010:default_idle (arch/x86/include/asm/irqflags.h:37 arch/x86/include/asm/irqflags.h:92 arch/x86/kernel/process.c:743) 
[ 2.081391][ C1] Code: 00 4d 29 c8 4c 01 c7 4c 29 c2 e9 72 ff ff ff cc cc cc cc 90 90 90 b8 0c 67 40 a5 eb 07 0f 00 2d 9f 89 47 00 55 48 89 e5 fb f4 <fa> 5d c3 66 0f 1f 44 00 00 90 90 90 b8 0c 67 40 a5 55 48 89 e5 e8
All code
========
   0:	00 4d 29             	add    %cl,0x29(%rbp)
   3:	c8 4c 01 c7          	enter  $0x14c,$0xc7
   7:	4c 29 c2             	sub    %r8,%rdx
   a:	e9 72 ff ff ff       	jmp    0xffffffffffffff81
   f:	cc                   	int3
  10:	cc                   	int3
  11:	cc                   	int3
  12:	cc                   	int3
  13:	90                   	nop
  14:	90                   	nop
  15:	90                   	nop
  16:	b8 0c 67 40 a5       	mov    $0xa540670c,%eax
  1b:	eb 07                	jmp    0x24
  1d:	0f 00 2d 9f 89 47 00 	verw   0x47899f(%rip)        # 0x4789c3
  24:	55                   	push   %rbp
  25:	48 89 e5             	mov    %rsp,%rbp
  28:	fb                   	sti
  29:	f4                   	hlt
  2a:*	fa                   	cli		<-- trapping instruction
  2b:	5d                   	pop    %rbp
  2c:	c3                   	ret
  2d:	66 0f 1f 44 00 00    	nopw   0x0(%rax,%rax,1)
  33:	90                   	nop
  34:	90                   	nop
  35:	90                   	nop
  36:	b8 0c 67 40 a5       	mov    $0xa540670c,%eax
  3b:	55                   	push   %rbp
  3c:	48 89 e5             	mov    %rsp,%rbp
  3f:	e8                   	.byte 0xe8

Code starting with the faulting instruction
===========================================
   0:	fa                   	cli
   1:	5d                   	pop    %rbp
   2:	c3                   	ret
   3:	66 0f 1f 44 00 00    	nopw   0x0(%rax,%rax,1)
   9:	90                   	nop
   a:	90                   	nop
   b:	90                   	nop
   c:	b8 0c 67 40 a5       	mov    $0xa540670c,%eax
  11:	55                   	push   %rbp
  12:	48 89 e5             	mov    %rsp,%rbp
  15:	e8                   	.byte 0xe8


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20241014/202410141238.aa194a45-lkp@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/1] sched/fair: Update blocked averages on tick

[Vincent Guittot]

On Fri, 11 Oct 2024 at 14:32, Pierre Gondois <pierre.gondois@arm.com> wrote:
>
> The Energy Aware Scheduler (EAS) relies on CPU/tasks utilization
> signals. On an idle CPU, the blocked load is updated during
> load balancing.
>
> sd->balance_interval increases with the number of CPUs in the domain.
> On an Arm DynamIQ system, sched domains containing CPUs with the same
> capacity do not exist. On a Pixel6 with 4 little, 2 mid, 2 big CPUs:
> - sd->min_interval = 8
> - sd->min_interval = 16
>
> The balance interval is doubled if the system is balanced, meaning
> that a balanced system will likely update blocked load every 16ms.

The real max boundary is LOAD_AVG_PERIOD that is used to update
nohz.next_blocked. This is the max between 2 updates of blocked load.
The other ones are opportunistics updates when a normal load balance
is triggered.

>
> The find_energy_efficient_cpu() function might thus relies on outdated
> util signals to place tasks, leading to bad energy placement.

Moving from 8ms to 16 ms is what makes the difference for you ?

The LOAD_AVG_PERIOD mas period has been used as a default value but if
it's too long, we could consider changing the max period between 2
updates

>
> Update blocked load on sched tick if:
> - the rq is idle
> - the load balancer will not be triggered.
>
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>  kernel/sched/fair.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 225b31aaee55..2f03bd10ac7a 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9841,15 +9841,12 @@ static unsigned long task_h_load(struct task_struct *p)
>  }
>  #endif
>
> -static void sched_balance_update_blocked_averages(int cpu)
> +static void update_blocked_averages(struct rq *rq)
>  {
>         bool decayed = false, done = true;
> -       struct rq *rq = cpu_rq(cpu);
> -       struct rq_flags rf;
>
> -       rq_lock_irqsave(rq, &rf);
> -       update_blocked_load_tick(rq);
>         update_rq_clock(rq);
> +       update_blocked_load_tick(rq);
>
>         decayed |= __update_blocked_others(rq, &done);
>         decayed |= __update_blocked_fair(rq, &done);
> @@ -9857,6 +9854,18 @@ static void sched_balance_update_blocked_averages(int cpu)
>         update_blocked_load_status(rq, !done);
>         if (decayed)
>                 cpufreq_update_util(rq, 0);
> +}
> +
> +static void sched_balance_update_blocked_averages(int cpu)
> +{
> +       struct rq *rq = cpu_rq(cpu);
> +       struct cfs_rq *cfs_rq;
> +       struct rq_flags rf;
> +
> +       cfs_rq = &rq->cfs;
> +
> +       rq_lock_irqsave(rq, &rf);
> +       update_blocked_averages(rq);
>         rq_unlock_irqrestore(rq, &rf);
>  }
>
> @@ -12877,6 +12886,8 @@ void sched_balance_trigger(struct rq *rq)
>
>         if (time_after_eq(jiffies, rq->next_balance))
>                 raise_softirq(SCHED_SOFTIRQ);
> +       else if (idle_cpu(rq->cpu))
> +               update_blocked_averages(rq);

would be good to explain why you don't need rq lock here

There is no rate limit so we can do this every tick (possibly  1ms)
when staying in shallowest state

So it's looks better to update the period between 2 update of blocked
load instead of adding a new path

>
>         nohz_balancer_kick(rq);
>  }
> --
> 2.25.1
>

Re: [PATCH 1/1] sched/fair: Update blocked averages on tick

[Pierre Gondois]

Hello Vincent,

On 10/15/24 14:44, Vincent Guittot wrote:
> On Fri, 11 Oct 2024 at 14:32, Pierre Gondois <pierre.gondois@arm.com> wrote:
>>
>> The Energy Aware Scheduler (EAS) relies on CPU/tasks utilization
>> signals. On an idle CPU, the blocked load is updated during
>> load balancing.
>>
>> sd->balance_interval increases with the number of CPUs in the domain.
>> On an Arm DynamIQ system, sched domains containing CPUs with the same
>> capacity do not exist. On a Pixel6 with 4 little, 2 mid, 2 big CPUs:
>> - sd->min_interval = 8
>> - sd->min_interval = 16
>>
>> The balance interval is doubled if the system is balanced, meaning
>> that a balanced system will likely update blocked load every 16ms.
> 
> The real max boundary is LOAD_AVG_PERIOD that is used to update
> nohz.next_blocked. This is the max between 2 updates of blocked load.
> The other ones are opportunistics updates when a normal load balance
> is triggered.

I wanted to mean that on an idle CPU with ticks still on, the cfs_rq load
is only updated through this path:
sched_balance_trigger() {
   if (time_after_eq(jiffies, rq->next_balance))
     raise_softirq(SCHED_SOFTIRQ);
}

...

sched_balance_softirq()
\-sched_balance_update_blocked_averages()

If the next_balance happens every 16ms, this means feec() might operate
its task placement using an (up to) 16ms old util signal. The CPU might
thus look busier than what it actually is.

> 
>>
>> The find_energy_efficient_cpu() function might thus relies on outdated
>> util signals to place tasks, leading to bad energy placement.
> 
> Moving from 8ms to 16 ms is what makes the difference for you ?

With this patch, the cfs_rq signal of an idle CPU is updated every tick,
so every 4ms.

> 
> The LOAD_AVG_PERIOD mas period has been used as a default value but if
> it's too long, we could consider changing the max period between 2
> updates
> 
>>
>> Update blocked load on sched tick if:
>> - the rq is idle
>> - the load balancer will not be triggered.
>>
>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
>> ---
>>   kernel/sched/fair.c | 21 ++++++++++++++++-----
>>   1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 225b31aaee55..2f03bd10ac7a 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -9841,15 +9841,12 @@ static unsigned long task_h_load(struct task_struct *p)
>>   }
>>   #endif
>>
>> -static void sched_balance_update_blocked_averages(int cpu)
>> +static void update_blocked_averages(struct rq *rq)
>>   {
>>          bool decayed = false, done = true;
>> -       struct rq *rq = cpu_rq(cpu);
>> -       struct rq_flags rf;
>>
>> -       rq_lock_irqsave(rq, &rf);
>> -       update_blocked_load_tick(rq);
>>          update_rq_clock(rq);
>> +       update_blocked_load_tick(rq);
>>
>>          decayed |= __update_blocked_others(rq, &done);
>>          decayed |= __update_blocked_fair(rq, &done);
>> @@ -9857,6 +9854,18 @@ static void sched_balance_update_blocked_averages(int cpu)
>>          update_blocked_load_status(rq, !done);
>>          if (decayed)
>>                  cpufreq_update_util(rq, 0);
>> +}
>> +
>> +static void sched_balance_update_blocked_averages(int cpu)
>> +{
>> +       struct rq *rq = cpu_rq(cpu);
>> +       struct cfs_rq *cfs_rq;
>> +       struct rq_flags rf;
>> +
>> +       cfs_rq = &rq->cfs;
>> +
>> +       rq_lock_irqsave(rq, &rf);
>> +       update_blocked_averages(rq);
>>          rq_unlock_irqrestore(rq, &rf);
>>   }
>>
>> @@ -12877,6 +12886,8 @@ void sched_balance_trigger(struct rq *rq)
>>
>>          if (time_after_eq(jiffies, rq->next_balance))
>>                  raise_softirq(SCHED_SOFTIRQ);
>> +       else if (idle_cpu(rq->cpu))
>> +               update_blocked_averages(rq);
> 
> would be good to explain why you don't need rq lock here

This is a mistake, the lock is indeed required.

> 
> There is no rate limit so we can do this every tick (possibly  1ms)
> when staying in shallowest state

I'm not sure we understood each other as this patch should no be related
to NOHZ CPUs. So please let me know if I used a wrong path as you said,
or if a rate limit would be needed.

> 
> So it's looks better to update the period between 2 update of blocked
> load instead of adding a new path
> 
>>
>>          nohz_balancer_kick(rq);
>>   }
>> --
>> 2.25.1
>>

Regards,
Pierre

Re: [PATCH 1/1] sched/fair: Update blocked averages on tick

[Vincent Guittot]

On Mon, 21 Oct 2024 at 11:47, Pierre Gondois <pierre.gondois@arm.com> wrote:
>
> Hello Vincent,
>
> On 10/15/24 14:44, Vincent Guittot wrote:
> > On Fri, 11 Oct 2024 at 14:32, Pierre Gondois <pierre.gondois@arm.com> wrote:
> >>
> >> The Energy Aware Scheduler (EAS) relies on CPU/tasks utilization
> >> signals. On an idle CPU, the blocked load is updated during
> >> load balancing.
> >>
> >> sd->balance_interval increases with the number of CPUs in the domain.
> >> On an Arm DynamIQ system, sched domains containing CPUs with the same
> >> capacity do not exist. On a Pixel6 with 4 little, 2 mid, 2 big CPUs:
> >> - sd->min_interval = 8
> >> - sd->min_interval = 16
> >>
> >> The balance interval is doubled if the system is balanced, meaning
> >> that a balanced system will likely update blocked load every 16ms.
> >
> > The real max boundary is LOAD_AVG_PERIOD that is used to update
> > nohz.next_blocked. This is the max between 2 updates of blocked load.
> > The other ones are opportunistics updates when a normal load balance
> > is triggered.
>
> I wanted to mean that on an idle CPU with ticks still on, the cfs_rq load
> is only updated through this path:
> sched_balance_trigger() {
>    if (time_after_eq(jiffies, rq->next_balance))
>      raise_softirq(SCHED_SOFTIRQ);
> }
>
> ...
>
> sched_balance_softirq()
> \-sched_balance_update_blocked_averages()
>
> If the next_balance happens every 16ms, this means feec() might operate
> its task placement using an (up to) 16ms old util signal. The CPU might

This is true for all idle CPUs and not only the local one with tick
still firing when idle

> thus look busier than what it actually is.

yes and it can be up to 32 ms because the real max limit between 2
updates is currently set to LOAD_AVG_PERIOD.  You can probably find a
unitary test on a board that takes advantage of some "random" update
during idle ticks but you will still have some old values on the
system and you don't have any control of their max period.

You could also reduce min_interval to a lower value but this will not
take care of other idle cpus

>
> >
> >>
> >> The find_energy_efficient_cpu() function might thus relies on outdated
> >> util signals to place tasks, leading to bad energy placement.
> >
> > Moving from 8ms to 16 ms is what makes the difference for you ?
>
> With this patch, the cfs_rq signal of an idle CPU is updated every tick,
> so every 4ms.

If the CPU is kept in the shallowest idle state then it is not
expected to stay for a long time which also means not that old
outdated value.

And what if the tick is 1ms or 10ms ?

>
> >
> > The LOAD_AVG_PERIOD mas period has been used as a default value but if
> > it's too long, we could consider changing the max period between 2
> > updates
> >
> >>
> >> Update blocked load on sched tick if:
> >> - the rq is idle
> >> - the load balancer will not be triggered.
> >>
> >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> >> ---
> >>   kernel/sched/fair.c | 21 ++++++++++++++++-----
> >>   1 file changed, 16 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >> index 225b31aaee55..2f03bd10ac7a 100644
> >> --- a/kernel/sched/fair.c
> >> +++ b/kernel/sched/fair.c
> >> @@ -9841,15 +9841,12 @@ static unsigned long task_h_load(struct task_struct *p)
> >>   }
> >>   #endif
> >>
> >> -static void sched_balance_update_blocked_averages(int cpu)
> >> +static void update_blocked_averages(struct rq *rq)
> >>   {
> >>          bool decayed = false, done = true;
> >> -       struct rq *rq = cpu_rq(cpu);
> >> -       struct rq_flags rf;
> >>
> >> -       rq_lock_irqsave(rq, &rf);
> >> -       update_blocked_load_tick(rq);
> >>          update_rq_clock(rq);
> >> +       update_blocked_load_tick(rq);
> >>
> >>          decayed |= __update_blocked_others(rq, &done);
> >>          decayed |= __update_blocked_fair(rq, &done);
> >> @@ -9857,6 +9854,18 @@ static void sched_balance_update_blocked_averages(int cpu)
> >>          update_blocked_load_status(rq, !done);
> >>          if (decayed)
> >>                  cpufreq_update_util(rq, 0);
> >> +}
> >> +
> >> +static void sched_balance_update_blocked_averages(int cpu)
> >> +{
> >> +       struct rq *rq = cpu_rq(cpu);
> >> +       struct cfs_rq *cfs_rq;
> >> +       struct rq_flags rf;
> >> +
> >> +       cfs_rq = &rq->cfs;
> >> +
> >> +       rq_lock_irqsave(rq, &rf);
> >> +       update_blocked_averages(rq);
> >>          rq_unlock_irqrestore(rq, &rf);
> >>   }
> >>
> >> @@ -12877,6 +12886,8 @@ void sched_balance_trigger(struct rq *rq)
> >>
> >>          if (time_after_eq(jiffies, rq->next_balance))
> >>                  raise_softirq(SCHED_SOFTIRQ);
> >> +       else if (idle_cpu(rq->cpu))
> >> +               update_blocked_averages(rq);
> >
> > would be good to explain why you don't need rq lock here
>
> This is a mistake, the lock is indeed required.

update_blocked_averages() can take time as we go through all cgroups
and interrupts are still disabled here whereas they are not during
softirq. Some already complained that running
update_blocked_averages() can impact the system latency that is why we
moved the update out of schedler path for the newly idle case for
example.

IMO, you should keep the update in the softirq and take advantage if
this tick to update other idle CPUs which don't have idle tick.

>
> >
> > There is no rate limit so we can do this every tick (possibly  1ms)
> > when staying in shallowest state
>
> I'm not sure we understood each other as this patch should no be related
> to NOHZ CPUs. So please let me know if I used a wrong path as you said,
> or if a rate limit would be needed.

If your problem is about too old pelt value for idle CPUs because the
load balance interval and the next_block period are too high then you
should fix it for all idle CPUs with the  next_block period probably

>
> >
> > So it's looks better to update the period between 2 update of blocked
> > load instead of adding a new path
> >
> >>
> >>          nohz_balancer_kick(rq);
> >>   }
> >> --
> >> 2.25.1
> >>
>
> Regards,
> Pierre