[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <898571880.615459.1581179973560.JavaMail.zimbra@efficios.com>
Date: Sat, 8 Feb 2020 11:39:33 -0500 (EST)
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: "Joel Fernandes, Google" <joel@...lfernandes.org>
Cc: paulmck <paulmck@...nel.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Gustavo A. R. Silva" <gustavo@...eddedor.com>,
Ingo Molnar <mingo@...hat.com>,
Richard Fontana <rfontana@...hat.com>,
rostedt <rostedt@...dmis.org>,
Thomas Gleixner <tglx@...utronix.de>,
Josh Triplett <josh@...htriplett.org>,
Lai Jiangshan <jiangshanlai@...il.com>
Subject: Re: [RFC 0/3] Revert SRCU from tracepoint infrastructure
----- On Feb 7, 2020, at 4:43 PM, Joel Fernandes, Google joel@...lfernandes.org wrote:
> On Fri, Feb 07, 2020 at 01:24:50PM -0800, Paul E. McKenney wrote:
>> On Fri, Feb 07, 2020 at 03:56:53PM -0500, Joel Fernandes (Google) wrote:
>> > Hi,
>> > These patches remove SRCU usage from tracepoints. The reason for proposing the
>> > reverts is because the whole point of SRCU was to avoid having to call
>> > rcu_irq_enter_irqson(). However this was added back in 865e63b04e9b2 ("tracing:
>> > Add back in rcu_irq_enter/exit_irqson() for rcuidle tracepoints") because perf
>> > was breaking..
>> >
>> > Further it occurs to me that, by using SRCU for tracepoints, we forgot that RCU
>> > is not really watching the tracepoint callbacks. This means that anyone doing
>> > preempt_disable() in their tracepoint callback, and expecting RCU to listen to
>> > them is in for a big surprise. When RCU is not watching, it does not care about
>> > preempt-disable sections on CPUs as you can see in the forced-quiescent state
>> > loop.
>> >
>> > Since SRCU is not providing any benefit because of 865e63b04e9b2 anyway, let us
>> > revert SRCU tracepoint code to maintain the sanity of potential
>> > tracepoint callback registerers.
>>
>> For whatever it is worth, SRCU is the exception to the "RCU needs to
>> be watching" rule. You can have SRCU readers on idle CPUs, offline
>> CPUs, CPUs executing in userspace, whatever.
>
> Yes sure. My concern was that callbacks are still using regular RCU somewhere
> and RCU isn't watching. I believe BPF is using RCU that way (not sure). But
> could be other out-of-tree kernel modules etc.
Tracepoint users should issue "tracepoint_synchronize_unregister()" rather than
expect tracepoints to rely on RCU.
I cannot find a good reason for perf to issue a redundant rcu_read_lock/unlock from
a tracepoint probe already providing RCU or SRCU synchronization. Same goes for BPF.
If they _really_ need to do it, then they could implement their own idle probes
which do an additional rcu_irq_enter/exit_irqson(), but this work-around does not
belong in tracepoint.h.
If out-of-tree modules fail to use the API properly, the burden of getting fixed
is on their shoulders, as it has always been.
Thanks,
Mathieu
>
> thanks,
>
> - Joel
>
>>
>> Thanx, Paul
>>
>> > Joel Fernandes (Google) (3):
>> > Revert "tracepoint: Use __idx instead of idx in DO_TRACE macro to make
>> > it unique"
>> > Revert "tracing: Add back in rcu_irq_enter/exit_irqson() for rcuidle
>> > tracepoints"
>> > Revert "tracepoint: Make rcuidle tracepoint callers use SRCU"
>> >
>> > include/linux/tracepoint.h | 40 ++++++--------------------------------
>> > kernel/tracepoint.c | 10 +---------
>> > 2 files changed, 7 insertions(+), 43 deletions(-)
>> >
>> > --
>> > 2.25.0.341.g760bfbb309-goog
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
Powered by blists - more mailing lists