From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754223AbcIBOx5 (ORCPT ); Fri, 2 Sep 2016 10:53:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35552 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753986AbcIBOxy (ORCPT ); Fri, 2 Sep 2016 10:53:54 -0400 Subject: Re: [PATCH 2/5] irqtime: Remove needless IRQs disablement on kcpustat update To: Frederic Weisbecker , LKML References: <1472824985-22947-1-git-send-email-fweisbec@gmail.com> <1472824985-22947-3-git-send-email-fweisbec@gmail.com> Cc: Peter Zijlstra , Wanpeng Li , Eric Dumazet , Ingo Molnar , Mike Galbraith , Rik van Riel From: Paolo Bonzini Message-ID: <6c9a4f26-0b0a-ccd1-7125-b15ceed2f2c0@redhat.com> Date: Fri, 2 Sep 2016 16:53:47 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1472824985-22947-3-git-send-email-fweisbec@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Fri, 02 Sep 2016 14:53:54 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Cc: Paolo Bonzini > Cc: Wanpeng Li > Cc: Mike Galbraith > Cc: Peter Zijlstra > Cc: Ingo Molnar > Cc: Eric Dumazet > Signed-off-by: Frederic Weisbecker > --- > 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; > } > >