Re: [PATCH 2/5] irqtime: Remove needless IRQs disablement on kcpustat update

[Paolo Bonzini]

On 02/09/2016 16:03, Frederic Weisbecker wrote:
> The callers of the functions performing irqtime kcpustat updates have
> IRQS disabled, no need to disable them again.

They do, but perhaps this should be annotated through some sparse magic.
 It's starting to be hairy, with the requirement spanning many separate
files.

Something like

#define __irq_disabled __must_hold(IRQ)

together with __acquire and __release annotations in
include/linux/irqflags.h would do.  I'm not sure how to handle
local_irq_save/local_irq_restore, but I guess sparse would be fine with

((void)({
   raw_local_irq_save(flags);
   if (flags) __acquire(IRQ);
}))

and

((void)({
   if (flags) __release(IRQ);
   raw_local_irq_restore(flags);
}))

since below that it's assembly.

Starting from irqtime_account_hi_update, irqtime_account_si_update and
irqtime_account_irq you'd get quite a few functions annotated.

Paolo

> Cc: Rik van Riel <riel@xxxxxxxxxx>
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
> Cc: Mike Galbraith <efault@xxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Eric Dumazet <eric.dumazet@xxxxxxxxx>
> Signed-off-by: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> ---
>  kernel/sched/cputime.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index f111076..d4d12a9 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -78,30 +78,26 @@ EXPORT_SYMBOL_GPL(irqtime_account_irq);
>  static cputime_t irqtime_account_hi_update(cputime_t maxtime)
>  {
>  	u64 *cpustat = kcpustat_this_cpu->cpustat;
> -	unsigned long flags;
>  	cputime_t irq_cputime;
>  
> -	local_irq_save(flags);
>  	irq_cputime = nsecs_to_cputime64(__this_cpu_read(cpu_hardirq_time)) -
>  		      cpustat[CPUTIME_IRQ];
>  	irq_cputime = min(irq_cputime, maxtime);
>  	cpustat[CPUTIME_IRQ] += irq_cputime;
> -	local_irq_restore(flags);
> +
>  	return irq_cputime;
>  }
>  
>  static cputime_t irqtime_account_si_update(cputime_t maxtime)
>  {
>  	u64 *cpustat = kcpustat_this_cpu->cpustat;
> -	unsigned long flags;
>  	cputime_t softirq_cputime;
>  
> -	local_irq_save(flags);
>  	softirq_cputime = nsecs_to_cputime64(__this_cpu_read(cpu_softirq_time)) -
>  			  cpustat[CPUTIME_SOFTIRQ];
>  	softirq_cputime = min(softirq_cputime, maxtime);
>  	cpustat[CPUTIME_SOFTIRQ] += softirq_cputime;
> -	local_irq_restore(flags);
> +
>  	return softirq_cputime;
>  }
>  
>