From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934513AbdDGRz2 (ORCPT ); Fri, 7 Apr 2017 13:55:28 -0400 Received: from mail.kernel.org ([198.145.29.136]:50032 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752848AbdDGRzT (ORCPT ); Fri, 7 Apr 2017 13:55:19 -0400 Date: Fri, 7 Apr 2017 13:55:15 -0400 From: Steven Rostedt To: Mathieu Desnoyers Cc: linux-kernel , Ingo Molnar , Andrew Morton , "Paul E. McKenney" Subject: Re: [PATCH 7/5] tracing: Make sure rcu_irq_enter() can work for trace_*_rcuidle() trace events Message-ID: <20170407135515.6e212a1b@gandalf.local.home> In-Reply-To: <1679331943.4538.1491587357083.JavaMail.zimbra@efficios.com> References: <20170407140106.051135969@goodmis.org> <20170407130615.2309b96d@gandalf.local.home> <1475342880.4473.1491585545139.JavaMail.zimbra@efficios.com> <20170407132613.4a9fa430@gandalf.local.home> <1679331943.4538.1491587357083.JavaMail.zimbra@efficios.com> X-Mailer: Claws Mail 3.14.0 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 7 Apr 2017 17:49:17 +0000 (UTC) Mathieu Desnoyers 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