From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934988AbcIRNrw (ORCPT ); Sun, 18 Sep 2016 09:47:52 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:34189 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759425AbcIRNrq (ORCPT ); Sun, 18 Sep 2016 09:47:46 -0400 Date: Sun, 18 Sep 2016 15:47:43 +0200 From: Frederic Weisbecker To: Peter Zijlstra Cc: LKML , Paolo Bonzini , Wanpeng Li , Eric Dumazet , Ingo Molnar , Mike Galbraith , Rik van Riel Subject: Re: [PATCH 2/5] irqtime: Remove needless IRQs disablement on kcpustat update Message-ID: <20160918134742.GB5909@lerouge> References: <1472824985-22947-1-git-send-email-fweisbec@gmail.com> <1472824985-22947-3-git-send-email-fweisbec@gmail.com> <20160907075913.GR10153@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160907075913.GR10153@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Sep 07, 2016 at 09:59:13AM +0200, Peter Zijlstra wrote: > On Fri, Sep 02, 2016 at 04:03:02PM +0200, Frederic Weisbecker wrote: > > > 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; > > > + lockdep_assert_irqs_disabled(); > > > - 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; > > > + lockdep_assert_irqs_disabled(); > > > - 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; > > } > > --- > Subject: locking/lockdep: Provide IRQ state assertion helpers > > Provide a cheap alternative to: > > WARN_ON(!irqs_disabled()); > > This patch provides: > > lockdep_assert_irqs_disabled(); > lockdep_assert_softirqs_disabled(); > > Which compile away for CONFIG_LOCKDEP=n kernels. > > Signed-off-by: Peter Zijlstra (Intel) > --- > --- a/include/linux/lockdep.h > +++ b/include/linux/lockdep.h > @@ -526,6 +526,23 @@ static inline void print_irqtrace_events > } > #endif > > +#if defined(CONFIG_LOCKDEP) && defined(CONFIG_TRACE_IRQFLAGS) > + > +#define lockdep_assert_irqs_disabled() do { \ > + WARN_ON(debug_locks && current->hardirqs_enabled); \ > + } while (0) > + > +#define lockdep_assert_softirqs_disabled() do { \ > + WARN_ON(debug_locks && current->softirqs_enabled); \ > + } while (0) > + > +#else > + > +#define lockdep_assert_irqs_disabled() do { } while (0) > +#define lockdep_assert_softirqs_disabled() do { } while (0) > + > +#endif > + > /* > * For trivial one-depth nesting of a lock-class, the following > * global define can be used. (Subsystems with multiple levels I love that! I've seen so many cases where I wanted this runtime check without the overhead of it on production kernels. I can take that patch, now since this is lockdep code and my series is scheduler's code that depend on it, how can we manage the dependency between the two branches? Perhaps I can add the lockdep patch in the series, there will likely be no wicked conflicts agains potential changes in the lockdep tree. Another alternative is to use WARN_ON_(!irqs_disabled()) on my series, then on the lockdep branch we can convert all the potential users including the current one.. The lockdep branch would then depend on the others. That way looks better as I can think of several sites to convert. Thanks.