Re: [PATCH v12 3/3] tracing: Centralize preemptirq tracepoints and unify their usage

From: Joel Fernandes <joelaf@google.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Joel Fernandes <joel@joelfernandes.org>,
	LKML <linux-kernel@vger.kernel.org>,
	"Cc: Android Kernel" <kernel-team@android.com>,
	Boqun Feng <boqun.feng@gmail.com>,
	Byungchul Park <byungchul.park@lge.com>,
	Ingo Molnar <mingo@redhat.com>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Paul McKenney <paulmck@linux.vnet.ibm.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Glexiner <tglx@linutronix.de>,
	Tom Zanussi <tom.zanussi@linux.intel.com>
Subject: Re: [PATCH v12 3/3] tracing: Centralize preemptirq tracepoints and unify their usage
Date: Tue, 7 Aug 2018 22:06:20 -0700	[thread overview]
Message-ID: <CAJWu+oruGocq8WG-A8OQ=1x8gxjURAja3hT98uB=yeoQ_QNh6g@mail.gmail.com> (raw)
In-Reply-To: <CAJWu+oqPUuF4rFOQ8=G3+ZxXJxiiNRBYE8C=zZ93A8qA+muHEg@mail.gmail.com>

On Tue, Aug 7, 2018 at 8:53 PM, Joel Fernandes <joelaf@google.com> wrote:
> On Tue, Aug 7, 2018 at 8:44 PM, Joel Fernandes <joelaf@google.com> wrote:
>> Hi Steve,
>>
>> On Tue, Aug 7, 2018 at 7:28 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> [...]
>>>> @@ -171,8 +174,7 @@ extern void syscall_unregfunc(void);
>>>>                       } while ((++it_func_ptr)->func);                \
>>>>               }                                                       \
>>>>                                                                       \
>>>> -             if (rcuidle)                                            \
>>>> -                     srcu_read_unlock_notrace(&tracepoint_srcu, idx);\
>>>> +             srcu_read_unlock_notrace(ss, idx);                      \
>>>
>>> Hmm, why do we have the two different srcu handles?
>>
>> Because if the memory operations happening on the normal SRCU handle
>> (during srcu_read_lock) is interrupted by NMI, then the other handle
>> (devoted to NMI) could be used instead and not bother the interrupted
>> handle. Does that makes sense?
>>
>> When I talked to Paul few months ago about SRCU from NMI context, he
>> mentioned the per-cpu memory operations during srcu_read_lock can be
>> NMI interrupted, that's why we added that warning.
>
> So I looked more closely, __srcu_read_lock on 2 different handles may
> still be doing a this_cpu_inc on the same location..
> (sp->sda->srcu_lock_count). :-(
>
> Paul any ideas on how to solve this?
>
> It does start to seem like a show stopper :-(

Steve, another thing we could do is drop "tracepoint: Make rcuidle
tracepoint callers use SRCU" and just call into irqsoff and preemptoff
tracer hooks directly.

The reason I added "tracepoint: Make rcuidle tracepoint callers use
SRCU" is because lockdep's performance dropped with existing
tracepoint code and SRCU improved that. But now that you're calling
directly into lockdep that shouldn't be an issue.

That should resolve your NMI issues and keep my macro and other clean
ups from the original patch. What do you think?

I am out in the morning, but I could write this up in the evening when
I get some time (unless you do it before me).

thanks,

 - Joel