| Message ID | 20120904230834.GB11494@jtriplet-mobl1 |
|---|---|
| State | New |
| Headers | show |
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. > /* > * 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 Hmm, if there ever happens to be a governor that can be loaded as a module, and if it has a tracepoint, then it would require this too. But the first time someone tries that, it will panic with the above code ;-) -- Steve
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
On Tue, Sep 04, 2012 at 04:33:44PM -0700, Josh Triplett wrote: > 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. :) Not sure I see much difference in aesthetics between the three approaches, but am willing to switch over to a generally agreed-upon scheme. Thanx, Paul
On Tue, 2012-09-04 at 16:33 -0700, Josh Triplett wrote: > > > > +#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. The change itself is not complex. But what is already there is complex enough. I'm talking more about adding to: $ grep '#define' include/linux/tracepoint.h #define _LINUX_TRACEPOINT_H #define PARAMS(args...) args #define TP_PROTO(args...) args #define TP_ARGS(args...) args #define TP_CONDITION(args...) args #define __DO_TRACE(tp, proto, args, cond, prercu, postrcu) \ #define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \ #define DEFINE_TRACE_FN(name, reg, unreg) \ #define DEFINE_TRACE(name) \ #define EXPORT_TRACEPOINT_SYMBOL_GPL(name) \ #define EXPORT_TRACEPOINT_SYMBOL(name) \ #define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \ #define DEFINE_TRACE_FN(name, reg, unreg) #define DEFINE_TRACE(name) #define EXPORT_TRACEPOINT_SYMBOL_GPL(name) #define EXPORT_TRACEPOINT_SYMBOL(name) #define DECLARE_TRACE_NOARGS(name) \ #define DECLARE_TRACE(name, proto, args) \ #define DECLARE_TRACE_CONDITION(name, proto, args, cond) \ #define TRACE_EVENT_FLAGS(event, flag) #define DECLARE_EVENT_CLASS(name, proto, args, tstruct, assign, print) #define DEFINE_EVENT(template, name, proto, args) \ #define DEFINE_EVENT_PRINT(template, name, proto, args, print) \ #define DEFINE_EVENT_CONDITION(template, name, proto, \ #define TRACE_EVENT(name, proto, args, struct, assign, print) \ #define TRACE_EVENT_FN(name, proto, args, struct, \ #define TRACE_EVENT_CONDITION(name, proto, args, cond, \ #define TRACE_EVENT_FLAGS(event, flag) > > 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. :) We could add either. Probably keep the ugliness of tracepoints inside the tracepoint code than to start spreading it around to rcu. RCU has its own ugliness features ;-) Hence, I would be OK if you send me a patch that does what you proposed and removes the EXPORT from RCU. -- Steve
On Tue, Sep 04, 2012 at 07:46:42PM -0400, Steven Rostedt wrote: > On Tue, 2012-09-04 at 16:33 -0700, Josh Triplett wrote: > > > > > > +#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. > > The change itself is not complex. But what is already there is complex > enough. I'm talking more about adding to: > > $ grep '#define' include/linux/tracepoint.h > #define _LINUX_TRACEPOINT_H > #define PARAMS(args...) args > #define TP_PROTO(args...) args > #define TP_ARGS(args...) args > #define TP_CONDITION(args...) args > #define __DO_TRACE(tp, proto, args, cond, prercu, postrcu) \ > #define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \ > #define DEFINE_TRACE_FN(name, reg, unreg) \ > #define DEFINE_TRACE(name) \ > #define EXPORT_TRACEPOINT_SYMBOL_GPL(name) \ > #define EXPORT_TRACEPOINT_SYMBOL(name) \ > #define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \ > #define DEFINE_TRACE_FN(name, reg, unreg) > #define DEFINE_TRACE(name) > #define EXPORT_TRACEPOINT_SYMBOL_GPL(name) > #define EXPORT_TRACEPOINT_SYMBOL(name) > #define DECLARE_TRACE_NOARGS(name) \ > #define DECLARE_TRACE(name, proto, args) \ > #define DECLARE_TRACE_CONDITION(name, proto, args, cond) \ > #define TRACE_EVENT_FLAGS(event, flag) > #define DECLARE_EVENT_CLASS(name, proto, args, tstruct, assign, print) > #define DEFINE_EVENT(template, name, proto, args) \ > #define DEFINE_EVENT_PRINT(template, name, proto, args, print) \ > #define DEFINE_EVENT_CONDITION(template, name, proto, \ > #define TRACE_EVENT(name, proto, args, struct, assign, print) \ > #define TRACE_EVENT_FN(name, proto, args, struct, \ > #define TRACE_EVENT_CONDITION(name, proto, args, cond, \ > #define TRACE_EVENT_FLAGS(event, flag) Yeah, I've had the adventure of hand-tracing through most of those in the past myself. :) At least they have a fairly clear hierarchy, without particularly complex conditionals. Ah, for a language with decent code generation facilities. :) > > 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. :) > > We could add either. Probably keep the ugliness of tracepoints inside > the tracepoint code than to start spreading it around to rcu. RCU has > its own ugliness features ;-) > > Hence, I would be OK if you send me a patch that does what you proposed > and removes the EXPORT from RCU. To clarify: does what I proposed as in the macro change to avoid defining trace_*_rcuidle in modules, or makes the change to define panicy versions of the two functions trace_*_rcuidle needs? - Josh Triplett
On Tue, Sep 04, 2012 at 04:43:07PM -0700, Paul E. McKenney wrote: > On Tue, Sep 04, 2012 at 04:33:44PM -0700, Josh Triplett wrote: > > 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. :) > > Not sure I see much difference in aesthetics between the three approaches, > but am willing to switch over to a generally agreed-upon scheme. Steve, could I get an ack from you on the patch I sent? - Josh Triplett
On Thu, 2012-09-06 at 11:54 -0700, Josh Triplett wrote: > Not sure I see much difference in aesthetics between the three approaches, > > but am willing to switch over to a generally agreed-upon scheme. > > Steve, could I get an ack from you on the patch I sent? I acked it, but do you just want me to take the patch? I'm getting ready for another 3.7 push to tip. -- Steve
On Thu, Sep 06, 2012 at 03:54:04PM -0400, Steven Rostedt wrote: > On Thu, 2012-09-06 at 11:54 -0700, Josh Triplett wrote: > > Not sure I see much difference in aesthetics between the three approaches, > > > but am willing to switch over to a generally agreed-upon scheme. > > > > Steve, could I get an ack from you on the patch I sent? > > I acked it, but do you just want me to take the patch? I'm getting ready > for another 3.7 push to tip. Up to Paul. What would make it easiest to coordinate that patch and the corresponding bits in the RCU patch series? - Josh Triplett
On Thu, Sep 06, 2012 at 11:09:40PM -0700, Josh Triplett wrote: > On Thu, Sep 06, 2012 at 03:54:04PM -0400, Steven Rostedt wrote: > > On Thu, 2012-09-06 at 11:54 -0700, Josh Triplett wrote: > > > Not sure I see much difference in aesthetics between the three approaches, > > > > but am willing to switch over to a generally agreed-upon scheme. > > > > > > Steve, could I get an ack from you on the patch I sent? > > > > I acked it, but do you just want me to take the patch? I'm getting ready > > for another 3.7 push to tip. > > Up to Paul. What would make it easiest to coordinate that patch and the > corresponding bits in the RCU patch series? All I need to do is to eventually remove the exports, correct? If so, full speed ahead! (FYI, will be offline through Sunday, PDT.) Thanx, Paul
On Fri, Sep 07, 2012 at 07:24:41AM -0700, Paul E. McKenney wrote: > On Thu, Sep 06, 2012 at 11:09:40PM -0700, Josh Triplett wrote: > > On Thu, Sep 06, 2012 at 03:54:04PM -0400, Steven Rostedt wrote: > > > On Thu, 2012-09-06 at 11:54 -0700, Josh Triplett wrote: > > > > Not sure I see much difference in aesthetics between the three approaches, > > > > > but am willing to switch over to a generally agreed-upon scheme. > > > > > > > > Steve, could I get an ack from you on the patch I sent? > > > > > > I acked it, but do you just want me to take the patch? I'm getting ready > > > for another 3.7 push to tip. > > > > Up to Paul. What would make it easiest to coordinate that patch and the > > corresponding bits in the RCU patch series? > > All I need to do is to eventually remove the exports, correct? > If so, full speed ahead! Sounds like you could go ahead and remove the exports now, and just make sure Steve's push goes in before yours. - Josh Triplett
On Fri, 2012-09-07 at 07:47 -0700, Josh Triplett wrote: > On Fri, Sep 07, 2012 at 07:24:41AM -0700, Paul E. McKenney wrote: > > On Thu, Sep 06, 2012 at 11:09:40PM -0700, Josh Triplett wrote: > > > On Thu, Sep 06, 2012 at 03:54:04PM -0400, Steven Rostedt wrote: > > > > On Thu, 2012-09-06 at 11:54 -0700, Josh Triplett wrote: > > > > > Not sure I see much difference in aesthetics between the three approaches, > > > > > > but am willing to switch over to a generally agreed-upon scheme. > > > > > > > > > > Steve, could I get an ack from you on the patch I sent? > > > > > > > > I acked it, but do you just want me to take the patch? I'm getting ready > > > > for another 3.7 push to tip. > > > > > > Up to Paul. What would make it easiest to coordinate that patch and the > > > corresponding bits in the RCU patch series? > > > > All I need to do is to eventually remove the exports, correct? > > If so, full speed ahead! > > Sounds like you could go ahead and remove the exports now, and just make > sure Steve's push goes in before yours. > Is there any rush to do this? I just plan on pushing it for 3.7. Paul, you just push your changes through tip, right? Then we can just let Ingo know. I could even make the patch a separate branch, that Ingo can pull into the RCU branch too. -- Steve
On Fri, Sep 07, 2012 at 11:16:07AM -0400, Steven Rostedt wrote: > On Fri, 2012-09-07 at 07:47 -0700, Josh Triplett wrote: > > On Fri, Sep 07, 2012 at 07:24:41AM -0700, Paul E. McKenney wrote: > > > On Thu, Sep 06, 2012 at 11:09:40PM -0700, Josh Triplett wrote: > > > > On Thu, Sep 06, 2012 at 03:54:04PM -0400, Steven Rostedt wrote: > > > > > On Thu, 2012-09-06 at 11:54 -0700, Josh Triplett wrote: > > > > > > Not sure I see much difference in aesthetics between the three approaches, > > > > > > > but am willing to switch over to a generally agreed-upon scheme. > > > > > > > > > > > > Steve, could I get an ack from you on the patch I sent? > > > > > > > > > > I acked it, but do you just want me to take the patch? I'm getting ready > > > > > for another 3.7 push to tip. > > > > > > > > Up to Paul. What would make it easiest to coordinate that patch and the > > > > corresponding bits in the RCU patch series? > > > > > > All I need to do is to eventually remove the exports, correct? > > > If so, full speed ahead! > > > > Sounds like you could go ahead and remove the exports now, and just make > > sure Steve's push goes in before yours. > > > > Is there any rush to do this? I just plan on pushing it for 3.7. > > Paul, you just push your changes through tip, right? Then we can just > let Ingo know. I could even make the patch a separate branch, that Ingo > can pull into the RCU branch too. Yep! But we also need to worry about -next and -fengguang. How about if you push your change into 3.7 and I push mine into 3.8? Thanx, Paul
On Tue, 2012-09-11 at 18:07 -0700, Paul E. McKenney wrote: > > > Paul, you just push your changes through tip, right? Then we can just > > let Ingo know. I could even make the patch a separate branch, that Ingo > > can pull into the RCU branch too. > > Yep! But we also need to worry about -next and -fengguang. > > How about if you push your change into 3.7 and I push mine into 3.8? Bah, this is what Linus said not to do. Although he's more about not having this happen in a single merge window, but this isn't the "git way". There should be no problem in pushing the patch based off of Linus's tree and have it be pulled into two branches. This is what Linus said he wanted. The change will go in via one of the branches, whichever is pulled first. And as the change will have the same SHA1 in both branches, git will merge it nicely. This is what Linus said at kernel summit that he wants people to do. Note, this will also get into -next and -fengguang's tree as well, without issue. -- Steve
On Wed, Sep 12, 2012 at 10:13:30AM -0400, Steven Rostedt wrote: > On Tue, 2012-09-11 at 18:07 -0700, Paul E. McKenney wrote: > > > > > Paul, you just push your changes through tip, right? Then we can just > > > let Ingo know. I could even make the patch a separate branch, that Ingo > > > can pull into the RCU branch too. > > > > Yep! But we also need to worry about -next and -fengguang. > > > > How about if you push your change into 3.7 and I push mine into 3.8? > > Bah, this is what Linus said not to do. Although he's more about not > having this happen in a single merge window, but this isn't the "git > way". > > There should be no problem in pushing the patch based off of Linus's > tree and have it be pulled into two branches. This is what Linus said he > wanted. The change will go in via one of the branches, whichever is > pulled first. And as the change will have the same SHA1 in both > branches, git will merge it nicely. This is what Linus said at kernel > summit that he wants people to do. > > Note, this will also get into -next and -fengguang's tree as well, > without issue. Doesn't Fengguang pull branches individually? And won't that mean that as soon as I push my export-removal commit, that he will see build failures? Or are you suggesting that we both send both commits? Thanx, Paul
[ Added Linus to verify what I'm talking about ] On Wed, 2012-09-12 at 08:03 -0700, Paul E. McKenney wrote: > Doesn't Fengguang pull branches individually? And won't that mean > that as soon as I push my export-removal commit, that he will see > build failures? > > Or are you suggesting that we both send both commits? No, what I could do is to push the branch out to my repository on kernel.org (I'm currently running it under my test suite), and then both you and Ingo can pull from it. It is based off of v3.6-rc5 and only has Josh's change in it. This way, when Ingo pulls it into the perf/core branch, I can work against that, and you could either pull the same branch directly from me (as you need it to work too), or you could have Ingo pull it into the rcu branches in tip, and you could work against that. The trick is that the branch I push is against Linus's tree (something that we all should be working against) and contains only the one change. Then everyone that needs this change can just pull it directly, and git will sort it all out without any conflicts. This is the method that Linus was talking about at Kernel Summit. -- Steve
On Wed, Sep 12, 2012 at 11:18:40AM -0400, Steven Rostedt wrote: > [ Added Linus to verify what I'm talking about ] > > On Wed, 2012-09-12 at 08:03 -0700, Paul E. McKenney wrote: > > > Doesn't Fengguang pull branches individually? And won't that mean > > that as soon as I push my export-removal commit, that he will see > > build failures? > > > > Or are you suggesting that we both send both commits? > > No, what I could do is to push the branch out to my repository on > kernel.org (I'm currently running it under my test suite), and then both > you and Ingo can pull from it. > > It is based off of v3.6-rc5 and only has Josh's change in it. This way, > when Ingo pulls it into the perf/core branch, I can work against that, > and you could either pull the same branch directly from me (as you need > it to work too), or you could have Ingo pull it into the rcu branches in > tip, and you could work against that. > > The trick is that the branch I push is against Linus's tree (something > that we all should be working against) and contains only the one change. > Then everyone that needs this change can just pull it directly, and git > will sort it all out without any conflicts. This is the method that > Linus was talking about at Kernel Summit. Works for me! Thanx, Paul
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 + /* * 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) \ { \