Messages in this thread | | | Date | Fri, 7 Apr 2017 13:55:15 -0400 | From | Steven Rostedt <> | Subject | Re: [PATCH 7/5] tracing: Make sure rcu_irq_enter() can work for trace_*_rcuidle() trace events |
| |
On Fri, 7 Apr 2017 17:49:17 +0000 (UTC) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> > Welcome to MACRO MAGIC!
Somebody is not wizardly happy.
> > > >> > >> as one argument to the __DO_TRACE() macro. To me it's a bit unexpected > >> coding-style wise. Am I the only one not comfortable with the proposed > >> syntax ? > > > > The entire TRACE_EVENT()/__DO_TRACE() is special. > > > > I thought about add yet another parameter, but as it doesn't change > > much, I figured this was good enough. We could beak it up if you like: > > > > #define RCU_IRQ_ENTER_CHECK \ > > if (WARN_ON_ONCE(rcu_irq_enter_disabled()) \ > > return; \ > > rcu_irq_enter_irqson(); > > > > [..] > > __DO_TRACE(&__tracepoint_##name, \ > > TP_PROTO(data_proto), \ > > TP_ARGS(data_args), \ > > TP_CONDITION(cond), \ > > PARAMS(RCU_IRQ_ENTER_CHECK), \ > > rcu_irq_exit_irqson()); \ > > > > > > Would that make you feel more comfortable? > > No, it's almost worse and adds still adds a return that apply within __DO_TRACE(), > but which is passed as an argument (code as macro argument), which I find really > unsettling.
/me finds it strangely enjoyable to make Mathieu unsettled.
> > I would prefer to add a new argument to __DO_TRACE, which we can call > "checkrcu", e.g.: > > #define __DO_TRACE(tp, proto, args, cond, checkrcu, prercu, postrcu) \
Grumble. I was trying to avoid making the patch more intrusive. But I do understand your concern.
> do { \ > struct tracepoint_func *it_func_ptr; \ > void *it_func; \ > void *__data; \ > \ > if (!((cond) && (checkrcu))) \ > return; \ > prercu; \ > rcu_read_lock_sched_notrace(); \ > it_func_ptr = rcu_dereference_sched((tp)->funcs); \ > if (it_func_ptr) { \ > do { \ > it_func = (it_func_ptr)->func; \ > __data = (it_func_ptr)->data; \ > ((void(*)(proto))(it_func))(args); \ > } while ((++it_func_ptr)->func); \ > } \ > rcu_read_unlock_sched_notrace(); \ > postrcu; \ > } while (0) > > And use it like this: > > #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), \ > !WARN_ON_ONCE(rcu_irq_enter_disabled()),\ > rcu_irq_enter_irqson(), \ > rcu_irq_exit_irqson()); \ > } > > This way we only pass evaluated expression (not code with "return" that > changes the flow) as arguments to __DO_TRACE, which makes it behave more > like a "sub-function", which is what we usually expect.
I understand what you are getting at, and I will concede your point. OK, I'll do it your way, but I still think you take all the fun out of it. ;-)
-- Steve
| |