linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Josh Triplett <josh@joshtriplett.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: paulmck@linux.vnet.ibm.com, linux-kernel@vger.kernel.org,
	mingo@elte.hu, laijs@cn.fujitsu.com, dipankar@in.ibm.com,
	akpm@linux-foundation.org, mathieu.desnoyers@polymtl.ca,
	niv@us.ibm.com, tglx@linutronix.de, peterz@infradead.org,
	Valdis.Kletnieks@vt.edu, dhowells@redhat.com,
	eric.dumazet@gmail.com, darren@dvhart.com, fweisbec@gmail.com,
	sbw@mit.edu, patches@linaro.org,
	"Paul E. McKenney" <paul.mckenney@linaro.org>
Subject: Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
Date: Tue, 4 Sep 2012 16:33:44 -0700	[thread overview]
Message-ID: <20120904233344.GB11855@jtriplet-mobl1> (raw)
In-Reply-To: <1346801031.27919.39.camel@gandalf.local.home>

On Tue, Sep 04, 2012 at 07:23:51PM -0400, Steven Rostedt wrote:
> On Tue, 2012-09-04 at 16:08 -0700, Josh Triplett wrote:
> > On Tue, Sep 04, 2012 at 06:51:22PM -0400, Steven Rostedt wrote:
> > > On Tue, 2012-09-04 at 15:33 -0700, Paul E. McKenney wrote:
> > > > On Fri, Aug 31, 2012 at 11:00:52AM -0700, Josh Triplett wrote:
> > > > > On Thu, Aug 30, 2012 at 11:56:17AM -0700, Paul E. McKenney wrote:
> > > > > > From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> > > > > > 
> > > > > > There is a need to use RCU from interrupt context, but either before
> > > > > > rcu_irq_enter() is called or after rcu_irq_exit() is called.  If the
> > > > > > interrupt occurs from idle, then lockdep-RCU will complain about such
> > > > > > uses, as they appear to be illegal uses of RCU from the idle loop.
> > > > > > In other environments, RCU_NONIDLE() could be used to properly protect
> > > > > > the use of RCU, but RCU_NONIDLE() currently cannot be invoked except
> > > > > > from process context.
> > > > > > 
> > > > > > This commit therefore modifies RCU_NONIDLE() to permit its use more
> > > > > > globally.
> > > > > > 
> > > > > > Reported-by: Steven Rostedt <rostedt@goodmis.org>
> > > > > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > 
> > > > > Something seems wrong about this.  The addition of EXPORT_SYMBOL_GPL
> > > > > suggests that such interrupt handlers might live in modules.  In what
> > > > > situation might a module interrupt handler get called from the idle
> > > > > loop, before rcu_irq_enter or after rcu_irq_exit, and need to know that
> > > > > when using RCU?
> > > > 
> > > > Drivers can be in modules, in which case their interrupt handlers will
> > > > also be in the corresponding module.  I do agree that in most cases,
> > > > the irq_enter() and irq_exit() hooks would be invoked by non-module code,
> > > > but I do believe that I had to add those exports due to build failures.
> > > > 
> > > > Steven will let me know if I am confused on this point.
> > > > 
> > > 
> > > You're not confused, the situation is confusing :-/
> > > 
> > > Because some trace events happen inside the idle loop after rcu has
> > > "shutdown", we needed to create "trace_foo_rcuidle()" handlers that can
> > > handle this condition. That is, for every trace_foo() static inline
> > > (used at the tracepoint location), there exists a static inline
> > > trace_foo_rcuidle(), that looks something like this:
> > > 
> > > static inline void trace_##name##_rcuidle(proto) {
> > > 	if (static_key_false(&__tracepoint_##name.key)) { 
> > > 		rcu_idle_exit();
> > > 		__DO_TRACE();
> > > 		rcu_idle_enter();
> > > 	}
> > > }
> > > 
> > > Although these calls are never used by module code, because they are
> > > static inlines, they are still defined for all tracepoints, kernel
> > > tracepoints as well as module tracepoints. And thus, need the export :-(
> > 
> > Fair enough.
> > 
> > What about having the tracepoint code generation detect when building as
> > part of a module via defined(MODULE), and omit the unused _rcuidle
> > versions in those cases?  That would avoid the need to export those
> > functions at all.  Strawman patch (not tested):
> > 
> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > index 802de56..41e1ef2 100644
> > --- a/include/linux/tracepoint.h
> > +++ b/include/linux/tracepoint.h
> > @@ -136,6 +136,22 @@ static inline void tracepoint_synchronize_unregister(void)
> >  		postrcu;						\
> >  	} while (0)
> >  
> > +#ifdef MODULE
> > +#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args) \
> > +	static inline void trace_##name##_rcuidle(proto)		\
> > +	{								\
> > +		if (static_key_false(&__tracepoint_##name.key))		\
> > +			__DO_TRACE(&__tracepoint_##name,		\
> > +				TP_PROTO(data_proto),			\
> > +				TP_ARGS(data_args),			\
> > +				TP_CONDITION(cond),			\
> > +				rcu_idle_exit(),			\
> > +				rcu_idle_enter());			\
> > +	}
> > +#else
> > +#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args)
> > +#endif
> > +
> 
> Egad! More macros on top of macros! Yeah I know I'm the most guilty of
> this, but it just seems to add one more indirection that I would like to
> avoid.

This doesn't seem to add a significant amount of complexity; it forwards
exactly the same parameters to the helper macro, and just moves the one
function definition there and makes it conditional.  This still seems
more preferable than exporting functions just so they won't get called.

> >  /*
> >   * Make sure the alignment of the structure in the __tracepoints section will
> >   * not add unwanted padding between the beginning of the section and the
> > @@ -151,16 +167,7 @@ static inline void tracepoint_synchronize_unregister(void)
> >  				TP_ARGS(data_args),			\
> >  				TP_CONDITION(cond),,);			\
> >  	}								\
> > -	static inline void trace_##name##_rcuidle(proto)		\
> > -	{								\
> > -		if (static_key_false(&__tracepoint_##name.key))		\
> > -			__DO_TRACE(&__tracepoint_##name,		\
> > -				TP_PROTO(data_proto),			\
> > -				TP_ARGS(data_args),			\
> > -				TP_CONDITION(cond),			\
> > -				rcu_idle_exit(),			\
> > -				rcu_idle_enter());			\
> > -	}								\
> > +	__DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args) \
> >  	static inline int						\
> >  	register_trace_##name(void (*probe)(data_proto), void *data)	\
> >  	{								\
> > 
> > 
> > If that doesn't work out, please consider adding an explicit comment
> > saying why you exported the functions.
> > 
> 
> Or, we could also add in include/linux/rcupdate.h:
> 
> #ifdef MODULE
> static inline void rcu_idle_enter(void) {
> 	panic("Don't call me from modules");
> }
> static inline void rcu_idle_exit(void) {
> 	panic("Don't call me from modules");
> }
> #else
> extern void rcu_idle_enter(void);
> extern void rcu_idle_exit(void);
> #endif

I could live with that; it seems preferable to unnecessary exports,
though it still seems much uglier to me than the conditional definition
of trace_*_rcuidle. :)

- Josh Triplett

  reply	other threads:[~2012-09-04 23:33 UTC|newest]

Thread overview: 86+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-08-30 18:56 [PATCH tip/core/rcu 0/15] Miscellaneous fixes Paul E. McKenney
2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
2012-08-30 18:56   ` [PATCH tip/core/rcu 02/15] rcu: Pull TINY_RCU dyntick-idle tracing into non-idle region Paul E. McKenney
2012-08-31 16:53     ` Josh Triplett
2012-08-30 18:56   ` [PATCH tip/core/rcu 03/15] rcu: Properly initialize ->boost_tasks on CPU offline Paul E. McKenney
2012-08-31 17:56     ` Josh Triplett
2012-09-06 14:40     ` Peter Zijlstra
2012-09-06 20:58       ` Paul E. McKenney
2012-08-30 18:56   ` [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context Paul E. McKenney
2012-08-31 18:00     ` Josh Triplett
2012-09-04 22:33       ` Paul E. McKenney
2012-09-04 22:48         ` Josh Triplett
2012-09-04 22:51         ` Steven Rostedt
2012-09-04 23:08           ` Josh Triplett
2012-09-04 23:23             ` Steven Rostedt
2012-09-04 23:33               ` Josh Triplett [this message]
2012-09-04 23:43                 ` Paul E. McKenney
2012-09-06 18:54                   ` Josh Triplett
2012-09-06 19:54                     ` Steven Rostedt
2012-09-07  6:09                       ` Josh Triplett
2012-09-07 14:24                         ` Paul E. McKenney
2012-09-07 14:47                           ` Josh Triplett
2012-09-07 15:16                             ` Steven Rostedt
2012-09-12  1:07                               ` Paul E. McKenney
2012-09-12 14:13                                 ` Steven Rostedt
2012-09-12 15:03                                   ` Paul E. McKenney
2012-09-12 15:18                                     ` Steven Rostedt
2012-09-12 16:57                                       ` Paul E. McKenney
2012-09-04 23:46                 ` Steven Rostedt
2012-09-05  0:42                   ` Josh Triplett
2012-09-05  6:23                   ` [PATCH] trace: Don't declare trace_*_rcuidle functions in modules Josh Triplett
2012-09-05 14:26                     ` Mathieu Desnoyers
2012-09-05 16:36                     ` Paul E. McKenney
2012-09-06 19:49                     ` Steven Rostedt
2012-09-14  6:07                     ` [tip:core/rcu] trace: Don' t " tip-bot for Josh Triplett
2012-09-04 23:14           ` [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context Paul E. McKenney
2012-08-30 18:56   ` [PATCH tip/core/rcu 05/15] rcu: Improve boost selection when moving tasks to root rcu_node Paul E. McKenney
2012-08-31 18:09     ` Josh Triplett
2012-08-30 18:56   ` [PATCH tip/core/rcu 06/15] rcu: Make offline-CPU checking allow for indefinite delays Paul E. McKenney
2012-08-31 18:12     ` Josh Triplett
2012-08-30 18:56   ` [PATCH tip/core/rcu 07/15] rcu: Fix obsolete rcu_initiate_boost() header comment Paul E. McKenney
2012-08-31 18:13     ` Josh Triplett
2012-08-30 18:56   ` [PATCH tip/core/rcu 08/15] rcu: Apply for_each_rcu_flavor() to increment_cpu_stall_ticks() Paul E. McKenney
2012-08-31 18:15     ` Josh Triplett
2012-09-04 22:44       ` Paul E. McKenney
2012-08-30 18:56   ` [PATCH tip/core/rcu 09/15] rcu: Avoid rcu_print_detail_task_stall_rnp() segfault Paul E. McKenney
2012-08-31 18:19     ` Josh Triplett
2012-09-04 22:46       ` Paul E. McKenney
2012-09-04 22:55         ` Josh Triplett
2012-08-30 18:56   ` [PATCH tip/core/rcu 10/15] rcu: Protect rcu_node accesses during CPU stall warnings Paul E. McKenney
2012-08-31 18:23     ` Josh Triplett
2012-09-04 22:51       ` Paul E. McKenney
2012-09-06 14:51     ` Peter Zijlstra
2012-09-06 21:01       ` Paul E. McKenney
2012-08-30 18:56   ` [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU " Paul E. McKenney
2012-08-31 18:24     ` Josh Triplett
2012-09-06 14:56     ` Peter Zijlstra
2012-09-06 15:07       ` Steven Rostedt
2012-09-06 15:19         ` Peter Zijlstra
2012-09-06 21:03           ` Paul E. McKenney
2012-09-06 21:41             ` Steven Rostedt
2012-09-06 21:58               ` Paul E. McKenney
2012-09-06 22:05                 ` Steven Rostedt
2012-09-06 22:22                   ` Paul E. McKenney
2012-09-07  7:00                     ` Peter Zijlstra
2012-09-07 14:42                       ` Steven Rostedt
2012-08-30 18:56   ` [PATCH tip/core/rcu 12/15] rcu: Remove redundant memory barrier from __call_rcu() Paul E. McKenney
2012-08-31 18:30     ` Josh Triplett
2012-08-31 18:40       ` Josh Triplett
2012-08-30 18:56   ` [PATCH tip/core/rcu 13/15] rcu: Move TINY_PREEMPT_RCU away from raw_local_irq_save() Paul E. McKenney
2012-08-31 18:34     ` Josh Triplett
2012-09-04 23:03       ` Paul E. McKenney
2012-08-30 18:56   ` [PATCH tip/core/rcu 14/15] time: RCU permitted to stop idle entry via softirq Paul E. McKenney
2012-08-31 18:51     ` Josh Triplett
2012-09-06 15:12     ` Peter Zijlstra
2012-09-06 21:35       ` Paul E. McKenney
2012-09-06 21:57         ` Steven Rostedt
2012-09-06 22:11           ` Paul E. McKenney
2012-08-30 18:56   ` [PATCH tip/core/rcu 15/15] kmemleak: Replace list_for_each_continue_rcu with new interface Paul E. McKenney
2012-08-31 18:55     ` Josh Triplett
2012-09-04 23:41       ` Paul E. McKenney
2012-08-31 16:49   ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Josh Triplett
2012-09-04 22:36     ` Paul E. McKenney
2012-09-06 14:38   ` Peter Zijlstra
2012-09-06 20:51     ` Paul E. McKenney
2012-09-07  6:54       ` Peter Zijlstra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120904233344.GB11855@jtriplet-mobl1 \
    --to=josh@joshtriplett.org \
    --cc=Valdis.Kletnieks@vt.edu \
    --cc=akpm@linux-foundation.org \
    --cc=darren@dvhart.com \
    --cc=dhowells@redhat.com \
    --cc=dipankar@in.ibm.com \
    --cc=eric.dumazet@gmail.com \
    --cc=fweisbec@gmail.com \
    --cc=laijs@cn.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mathieu.desnoyers@polymtl.ca \
    --cc=mingo@elte.hu \
    --cc=niv@us.ibm.com \
    --cc=patches@linaro.org \
    --cc=paul.mckenney@linaro.org \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=sbw@mit.edu \
    --cc=tglx@linutronix.de \
    --subject='Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox