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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 2 Sep 2016 16:53:47 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Frederic Weisbecker <fweisbec@...il.com>,
        LKML <linux-kernel@...r.kernel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Wanpeng Li <wanpeng.li@...mail.com>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Ingo Molnar <mingo@...nel.org>, Mike Galbraith <efault@....de>,
        Rik van Riel <riel@...hat.com>
Subject: Re: [PATCH 2/5] irqtime: Remove needless IRQs disablement on kcpustat
 update



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@...hat.com>
> Cc: Paolo Bonzini <pbonzini@...hat.com>
> Cc: Wanpeng Li <wanpeng.li@...mail.com>
> Cc: Mike Galbraith <efault@....de>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Eric Dumazet <eric.dumazet@...il.com>
> Signed-off-by: Frederic Weisbecker <fweisbec@...il.com>
> ---
>  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;
>  }
>  
>