Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 8 Feb 2020 11:31:25 -0500 (EST)
From:   Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To:     "Joel Fernandes, Google" <joel@joelfernandes.org>
Cc:     linux-kernel <linux-kernel@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
        Ingo Molnar <mingo@redhat.com>,
        Richard Fontana <rfontana@redhat.com>,
        rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        paulmck <paulmck@kernel.org>,
        Josh Triplett <josh@joshtriplett.org>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Message-ID: <1997032737.615438.1581179485507.JavaMail.zimbra@efficios.com>
In-Reply-To: <20200207205656.61938-1-joel@joelfernandes.org>
References: <20200207205656.61938-1-joel@joelfernandes.org>
Subject: Re: [RFC 0/3] Revert SRCU from tracepoint infrastructure
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Thread-Topic: Revert SRCU from tracepoint infrastructure
Thread-Index: PMD/ty+mHKm7zkEqxhTVkMEkATXx4A==
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

----- On Feb 7, 2020, at 3:56 PM, Joel Fernandes, Google joel@joelfernandes.org 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..

I think the original patch re-enabling the rcu_irq_enter/exit_irqson() is a
tracepoint band-aid over what should actually been fixed within perf instead.

Perf should not do rcu_read_lock/unlock()/synchronize_rcu(), but rather use
tracepoint_synchronize_unregister() to match the read-side provided by
tracepoints.

If perf can then just rely on the underlying synchronization provided by each
instrumentation providers (tracepoint, kprobe, ...) and not explicitly add its own
unneeded synchronization on top (e.g. rcu_read_lock/unlock), then it should simplify
all this.

> 
> 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.

As Paul noted, SRCU is the exception to the "RCU watching".

> 
> 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.

Introducing SRCU was done to simplify handling of rcuidle, thus removing some
significant overhead that has been noticed due to use of rcu_irq_enter/exit_irqson().

There is another longer-term goal in adding SRCU-synchronized tracepoints: adding
the ability to create tracepoint probes which will be allowed to handle page
faults properly. This is very relevant for the syscall tracepoints reading the
system call parameters from user-space. Currently, we are stuck with sub par
hacks such as filling the missing data with zeroes. Usually nobody notices because
most syscall arguments are typically hot in the page cache, but it is still fragile.

I'd very much prefer see commits moving syscall tracepoints to use of SRCU
(without preempt disable around the tracepoint calls) rather than a commit removing
tracepoint SRCU use because of something that needs to be fixed within perf.

Thanks,

Mathieu


> 
> 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