Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1440767 > unrolled thread

[PATCH v4 0/3] sched,time: fix irq time accounting with nohz_idle

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/3] sched,time: fix irq time accounting with nohz_idle riel@redhat.com - 2016-07-11 19:00 +0200
    [PATCH 3/3] time: drop local_irq_save/restore from irqtime_account_irq riel@redhat.com - 2016-07-11 19:00 +0200
      Re: [PATCH 3/3] time: drop local_irq_save/restore from  irqtime_account_irq Paolo Bonzini <pbonzini@redhat.com> - 2016-07-11 19:10 +0200
    Re: [PATCH v4 0/3] sched,time: fix irq time accounting with nohz_idle Frederic Weisbecker <fweisbec@gmail.com> - 2016-07-12 14:20 +0200

#1440767 — [PATCH v4 0/3] sched,time: fix irq time accounting with nohz_idle

Currently irq time accounting only works in these cases:
1) purely ticke based accounting
2) nohz_full accounting, but only on housekeeping & nohz_full CPUs
3) architectures with native vtime accounting

On nohz_idle CPUs, which are probably the majority nowadays,
irq time accounting is currently broken. This leads to systems
reporting a dramatically lower amount of irq & softirq time than
is actually spent handling them, with all the time spent while the
system is in the idle task being accounted as idle.

This patch set seems to bring the amount of irq time reported by
top (and /proc/stat) roughly in line with that measured when I do
a "perf record -g -a" run to see what is using all that time.

The amount of irq time used, especially softirq, is shockingly high,
to the point of me thinking this patch set may be wrong, but the
numbers seem to match what perf is giving me...

These patches apply on top of Wanpeng Li's steal time patches.

CONFIG_IRQ_TIME_ACCOUNTING is now a config option that is available
as a separate choice from tick based / nohz_idle / nohz_full mode,
a suggested by Frederic Weisbecker.

Next up: look at the things that are using CPU time on an otherwise
idle system, and see if I can make those a little faster :)

v2: address Peterz's concerns, some more cleanups
v3: rewrite the code along Frederic's suggestions, now cputime_t
    is used everywhere
v4: greatly simplify the local_irq_save/restore optimisation, thanks
    to Paolo pointing out irqs are already blocked by the callers

[toc] | [next] | [standalone]

#1440768 — [PATCH 3/3] time: drop local_irq_save/restore from irqtime_account_irq

From: Rik van Riel <riel@redhat.com>

Paolo pointed out that irqs are already blocked when irqtime_account_irq
is called. That means there is no reason to call local_irq_save/restore
again.

Signed-off-by: Rik van Riel <riel@redhat.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
---
 kernel/sched/cputime.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index ca7e33cb0967..7b6fa4d7ad4c 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -49,15 +49,12 @@ DEFINE_PER_CPU(seqcount_t, irq_time_seq);
  */
 void irqtime_account_irq(struct task_struct *curr)
 {
-	unsigned long flags;
 	s64 delta;
 	int cpu;
 
 	if (!sched_clock_irqtime)
 		return;
 
-	local_irq_save(flags);
-
 	cpu = smp_processor_id();
 	delta = sched_clock_cpu(cpu) - __this_cpu_read(irq_start_time);
 	__this_cpu_add(irq_start_time, delta);
@@ -75,7 +72,6 @@ void irqtime_account_irq(struct task_struct *curr)
 		__this_cpu_add(cpu_softirq_time, delta);
 
 	irq_time_write_end();
-	local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(irqtime_account_irq);
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1440776 — Re: [PATCH 3/3] time: drop local_irq_save/restore from irqtime_account_irq

On 11/07/2016 18:53, riel@redhat.com wrote:
> From: Rik van Riel <riel@redhat.com>
> 
> Paolo pointed out that irqs are already blocked when irqtime_account_irq
> is called. That means there is no reason to call local_irq_save/restore
> again.
> 
> Signed-off-by: Rik van Riel <riel@redhat.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  kernel/sched/cputime.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index ca7e33cb0967..7b6fa4d7ad4c 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -49,15 +49,12 @@ DEFINE_PER_CPU(seqcount_t, irq_time_seq);
>   */
>  void irqtime_account_irq(struct task_struct *curr)
>  {
> -	unsigned long flags;
>  	s64 delta;
>  	int cpu;
>  
>  	if (!sched_clock_irqtime)
>  		return;
>  
> -	local_irq_save(flags);
> -
>  	cpu = smp_processor_id();
>  	delta = sched_clock_cpu(cpu) - __this_cpu_read(irq_start_time);
>  	__this_cpu_add(irq_start_time, delta);
> @@ -75,7 +72,6 @@ void irqtime_account_irq(struct task_struct *curr)
>  		__this_cpu_add(cpu_softirq_time, delta);
>  
>  	irq_time_write_end();
> -	local_irq_restore(flags);
>  }
>  EXPORT_SYMBOL_GPL(irqtime_account_irq);
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1441300

On Mon, Jul 11, 2016 at 12:53:54PM -0400, riel@redhat.com wrote:
> Currently irq time accounting only works in these cases:
> 1) purely ticke based accounting
> 2) nohz_full accounting, but only on housekeeping & nohz_full CPUs
> 3) architectures with native vtime accounting
> 
> On nohz_idle CPUs, which are probably the majority nowadays,
> irq time accounting is currently broken. This leads to systems
> reporting a dramatically lower amount of irq & softirq time than
> is actually spent handling them, with all the time spent while the
> system is in the idle task being accounted as idle.
> 
> This patch set seems to bring the amount of irq time reported by
> top (and /proc/stat) roughly in line with that measured when I do
> a "perf record -g -a" run to see what is using all that time.
> 
> The amount of irq time used, especially softirq, is shockingly high,
> to the point of me thinking this patch set may be wrong, but the
> numbers seem to match what perf is giving me...
> 
> These patches apply on top of Wanpeng Li's steal time patches.
> 
> CONFIG_IRQ_TIME_ACCOUNTING is now a config option that is available
> as a separate choice from tick based / nohz_idle / nohz_full mode,
> a suggested by Frederic Weisbecker.
> 
> Next up: look at the things that are using CPU time on an otherwise
> idle system, and see if I can make those a little faster :)
> 
> v2: address Peterz's concerns, some more cleanups
> v3: rewrite the code along Frederic's suggestions, now cputime_t
>     is used everywhere
> v4: greatly simplify the local_irq_save/restore optimisation, thanks
>     to Paolo pointing out irqs are already blocked by the callers
> 

Thanks Rick!

I'm applying the series with my patches and will do a pull request to
Ingo.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web