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

Groups > linux.kernel > #1485802 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/5] irqtime: Remove needless IRQs disablement on  kcpustat update Frederic Weisbecker <fweisbec@gmail.com> - 2016-09-18 15:50 +0200
    Re: [PATCH 2/5] irqtime: Remove needless IRQs disablement on  kcpustat update Peter Zijlstra <peterz@infradead.org> - 2016-09-18 18:00 +0200

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

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) <peterz@infradead.org>
> ---
> --- 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.

[toc] | [next] | [standalone]

#1485847

On Sun, Sep 18, 2016 at 03:47:43PM +0200, Frederic Weisbecker wrote:
> 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.

Yeah, just take it along with the rest of the patches.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web