[GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[Steven Rostedt]

Linus,

The recursion code in the internals of the ftrace ring buffer requires
using "preempt_disable_notrace". But it has been discovered that
preempt_disable() is being used by this_cpu_read() in some architectures
and trace_cpu_read() is part of the recursion protection of the ring buffer.
The only reason this did not crash was due to the recursion protection
in other parts of ftrace. But if there's a path that does some kind
of function tracing without that protection, it will crash the kernel.

Use the __this_cpu_*() version instead which does not add preempt_disable()
or other unexpected functions to the per cpu code. Preemption is already
disabled at these paths, so the __this_cpu*() version should be used
anyawy.

Please pull the latest trace-fixes-v4.0-rc4 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace-fixes-v4.0-rc4

Tag SHA1: 8c92b3f282f17acc4a17be91c7836e1442847270
Head SHA1: 9a22e2db723ae2c5eaf53efc40a1638620c1eb7a


Steven Rostedt (1):
      ring-buffer: Replace this_cpu_*() with __this_cpu_*()

----
 kernel/trace/ring_buffer.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)
---------------------------
commit 9a22e2db723ae2c5eaf53efc40a1638620c1eb7a
Author: Steven Rostedt <rostedt@goodmis.org>
Date:   Tue Mar 17 10:40:38 2015 -0400

    ring-buffer: Replace this_cpu_*() with __this_cpu_*()
    
    It has come to my attention that this_cpu_read/write are horrible on
    architectures other than x86. Worse yet, they actually disable
    preemption or interrupts! This caused some unexpected tracing results
    on ARM.
    
       101.356868: preempt_count_add <-ring_buffer_lock_reserve
       101.356870: preempt_count_sub <-ring_buffer_lock_reserve
    
    The ring_buffer_lock_reserve has recursion protection that requires
    accessing a per cpu variable. But since preempt_disable() is traced, it
    too got traced while accessing the variable that is suppose to prevent
    recursion like this.
    
    The generic version of this_cpu_read() and write() are:
    
     #define this_cpu_generic_read(pcp)					\
     ({	typeof(pcp) ret__;						\
    	preempt_disable();						\
    	ret__ = *this_cpu_ptr(&(pcp));					\
    	preempt_enable();						\
    	ret__;								\
     })
    
     #define this_cpu_generic_to_op(pcp, val, op)				\
     do {									\
    	unsigned long flags;						\
    	raw_local_irq_save(flags);					\
    	*__this_cpu_ptr(&(pcp)) op val;					\
    	raw_local_irq_restore(flags);					\
     } while (0)
    
    Which is unacceptable for locations that know they are within preempt
    disabled or interrupt disabled locations.
    
    Paul McKenney stated that __this_cpu_() versions produce much better code on
    other architectures than this_cpu_() does, if we know that the call is done in
    a preempt disabled location.
    
    I also changed the recursive_unlock() to use two local variables instead
    of accessing the per_cpu variable twice.
    
    Link: http://lkml.kernel.org/r/20150317114411.GE3589@linux.vnet.ibm.com
    Link: http://lkml.kernel.org/r/20150317104038.312e73d1@gandalf.local.home
    
    Cc: stable@vger.kernel.org
    Acked-by: Christoph Lameter <cl@linux.com>
    Reported-by: Uwe Kleine-KÃ=B6nig <u.kleine-koenig@pengutronix.de>
    Tested-by: Uwe Kleine-KÃ=B6nig <u.kleine-koenig@pengutronix.de>
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 5040d44fe5a3..922048a0f7ea 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -2679,7 +2679,7 @@ static DEFINE_PER_CPU(unsigned int, current_context);
 
 static __always_inline int trace_recursive_lock(void)
 {
-	unsigned int val = this_cpu_read(current_context);
+	unsigned int val = __this_cpu_read(current_context);
 	int bit;
 
 	if (in_interrupt()) {
@@ -2696,18 +2696,17 @@ static __always_inline int trace_recursive_lock(void)
 		return 1;
 
 	val |= (1 << bit);
-	this_cpu_write(current_context, val);
+	__this_cpu_write(current_context, val);
 
 	return 0;
 }
 
 static __always_inline void trace_recursive_unlock(void)
 {
-	unsigned int val = this_cpu_read(current_context);
+	unsigned int val = __this_cpu_read(current_context);
 
-	val--;
-	val &= this_cpu_read(current_context);
-	this_cpu_write(current_context, val);
+	val &= val & (val - 1);
+	__this_cpu_write(current_context, val);
 }
 
 #else

Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[Linus Torvalds]

Ugh.

I think this is bogus.

Why?

On Thu, Mar 19, 2015 at 3:02 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
>     The generic version of this_cpu_read() and write() are:
>
>      #define this_cpu_generic_read(pcp)                                 \
>      ({ typeof(pcp) ret__;                                              \
>         preempt_disable();                                              \
>         ret__ = *this_cpu_ptr(&(pcp));                                  \
>         preempt_enable();                                               \
>         ret__;                                                          \
>      })
>
>      #define this_cpu_generic_to_op(pcp, val, op)                               \
>      do {                                                                       \
>         unsigned long flags;                                            \
>         raw_local_irq_save(flags);                                      \
>         *__this_cpu_ptr(&(pcp)) op val;                                 \
>         raw_local_irq_restore(flags);                                   \
>      } while (0)

Let's just fix the generic versions of this_cpu_read/write.

Now, it is true that for the "op" versions of "this_cpu_xyz" we need
to disable preemption or interrupts or something for the generic case
(where "generic case" is weasel-wording for "the architecture is crap
and has horrible problems with any kinds of atomics, even just per-cpu
ones").

But that is *not* true for plain read/write. There is no point in
disabling preemption, because in the end, if preemption was enabled,
it happens on a random CPU anyway (and that may be fine - many
heuristics may not care *which* exact CPU it's about), and unlike the
"op" cases, the actual read or write is a single access, so there's no
reason to disable preemption/interrupts to make it "atomic" on that
random CPU.

If you pair a this_cpu_read with a this_cpu_write and expect them to
go to the same cpu, such a user obviously needs to disable preemption
etc for bigger reasons - but disabling preemption inside the operation
itself does absolutely nothing.

So is there really any reason to not make those simpler forms just do
something simpler like

   #define this_cpu_generic_read(pcp) \
      READ_ONCE(*this_cpu_ptr(&(pcp)))

  #define this_cpu_generic_read(pcp, val) \
      WRITE_ONCE(*this_cpu_ptr(&(pcp)), val)

instead?

Even if we end up being preempted in the middle, do we *care*? It's
going to one or the other CPU.

The only issue might be CPU hotplug in between (the previous CPU going
away), but I'm not sure even that matterts.

So I don't think the ring-buffer change is necessarily _wrong_, but if
this is a performance issue, why don't we just fix it up for the
generic case rather than for just one user?

Or is the hotplug issue a big deal? I thought we already had some
rcu-sched point for the cpu going away, so that even the hotplug case
should be ok.

                           Linus

Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[Steven Rostedt]

On Thu, 19 Mar 2015 15:16:25 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> So I don't think the ring-buffer change is necessarily _wrong_, but if
> this is a performance issue, why don't we just fix it up for the
> generic case rather than for just one user?

I totally agree with your analysis, but it's up to Christoph to come up
with an answer to your questions.

-- Steve

Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[Steven Rostedt]

On Thu, 19 Mar 2015 18:34:44 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 19 Mar 2015 15:16:25 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > So I don't think the ring-buffer change is necessarily _wrong_, but if
> > this is a performance issue, why don't we just fix it up for the
> > generic case rather than for just one user?
> 
> I totally agree with your analysis, but it's up to Christoph to come up
> with an answer to your questions.
> 

I will add that the ring buffer issue is not just a performance
problem. It is a correctness problem. The generic
preempt_disable/enable() functions can be traced by the function
tracer, where as the preempt_disable/enable_notrace() versions are not.

As tracing is very invasive, and can cause unnecessary recursions,
there are protection mechanisms to prevent something like that
happening. The issue that this patch addresses is that the recursion
protection is the code that happens to be causing the recursion!

 some_function()
   function_tracer()
     ring_buffer_reserve()
       trace_recursive_lock()
         this_cpu_read()
           preempt_disable()
             function_tracer()
               ring_buffer_reserve()
                 trace_recursion_lock()
                    (etc)

The reason this did not happen is that the function_tracer() also has
its own recursion protection that uses current->trace_recursion to
prevent that from happening. But if there was some function tracing
that did not check recursion and calls into the ring buffer, that could
crash the system.

-- Steve

Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[Christoph Lameter]

On Thu, 19 Mar 2015, Steven Rostedt wrote:

> On Thu, 19 Mar 2015 15:16:25 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > So I don't think the ring-buffer change is necessarily _wrong_, but if
> > this is a performance issue, why don't we just fix it up for the
> > generic case rather than for just one user?
>
> I totally agree with your analysis, but it's up to Christoph to come up
> with an answer to your questions.

Something beyond: Do not use this_cpu_* when preemption is already
off but use __this_cpu_*?

Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[Steven Rostedt]

On Tue, 19 May 2015 10:35:32 -0500 (CDT)
Christoph Lameter <cl@linux.com> wrote:

> On Thu, 19 Mar 2015, Steven Rostedt wrote:
> 
> > On Thu, 19 Mar 2015 15:16:25 -0700
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >
> > > So I don't think the ring-buffer change is necessarily _wrong_, but if
> > > this is a performance issue, why don't we just fix it up for the
> > > generic case rather than for just one user?
> >
> > I totally agree with your analysis, but it's up to Christoph to come up
> > with an answer to your questions.
> 
> Something beyond: Do not use this_cpu_* when preemption is already
> off but use __this_cpu_*?

I think the question was, why exactly does the generic this_cpu_read()
require disabling preemption? What breaks if it is not disabled?

-- Steve

Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[Christoph Lameter]

On Tue, 19 May 2015, Steven Rostedt wrote:

> On Tue, 19 May 2015 10:35:32 -0500 (CDT)
> Christoph Lameter <cl@linux.com> wrote:
>
> > On Thu, 19 Mar 2015, Steven Rostedt wrote:
> >
> > > On Thu, 19 Mar 2015 15:16:25 -0700
> > > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > >
> > > > So I don't think the ring-buffer change is necessarily _wrong_, but if
> > > > this is a performance issue, why don't we just fix it up for the
> > > > generic case rather than for just one user?
> > >
> > > I totally agree with your analysis, but it's up to Christoph to come up
> > > with an answer to your questions.
> >
> > Something beyond: Do not use this_cpu_* when preemption is already
> > off but use __this_cpu_*?
>
> I think the question was, why exactly does the generic this_cpu_read()
> require disabling preemption? What breaks if it is not disabled?

Ok I answered that before. There is a cache line population/eviction
issue (because it may happen on the wrong cache) but basically I think
its mostly there for symmetries sake. We could remove
this_cpu_read/write and let __this_cpu_read()/__this_cpu_write be used in non
preemptible contexts.

Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[Steven Rostedt]

Christoph,

Any comment on this?

-- Steve


On Thu, 19 Mar 2015 15:16:25 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> Ugh.
> 
> I think this is bogus.
> 
> Why?
> 
> On Thu, Mar 19, 2015 at 3:02 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> >     The generic version of this_cpu_read() and write() are:
> >
> >      #define this_cpu_generic_read(pcp)                                 \
> >      ({ typeof(pcp) ret__;                                              \
> >         preempt_disable();                                              \
> >         ret__ = *this_cpu_ptr(&(pcp));                                  \
> >         preempt_enable();                                               \
> >         ret__;                                                          \
> >      })
> >
> >      #define this_cpu_generic_to_op(pcp, val, op)                               \
> >      do {                                                                       \
> >         unsigned long flags;                                            \
> >         raw_local_irq_save(flags);                                      \
> >         *__this_cpu_ptr(&(pcp)) op val;                                 \
> >         raw_local_irq_restore(flags);                                   \
> >      } while (0)
> 
> Let's just fix the generic versions of this_cpu_read/write.
> 
> Now, it is true that for the "op" versions of "this_cpu_xyz" we need
> to disable preemption or interrupts or something for the generic case
> (where "generic case" is weasel-wording for "the architecture is crap
> and has horrible problems with any kinds of atomics, even just per-cpu
> ones").
> 
> But that is *not* true for plain read/write. There is no point in
> disabling preemption, because in the end, if preemption was enabled,
> it happens on a random CPU anyway (and that may be fine - many
> heuristics may not care *which* exact CPU it's about), and unlike the
> "op" cases, the actual read or write is a single access, so there's no
> reason to disable preemption/interrupts to make it "atomic" on that
> random CPU.
> 
> If you pair a this_cpu_read with a this_cpu_write and expect them to
> go to the same cpu, such a user obviously needs to disable preemption
> etc for bigger reasons - but disabling preemption inside the operation
> itself does absolutely nothing.
> 
> So is there really any reason to not make those simpler forms just do
> something simpler like
> 
>    #define this_cpu_generic_read(pcp) \
>       READ_ONCE(*this_cpu_ptr(&(pcp)))
> 
>   #define this_cpu_generic_read(pcp, val) \
>       WRITE_ONCE(*this_cpu_ptr(&(pcp)), val)
> 
> instead?
> 
> Even if we end up being preempted in the middle, do we *care*? It's
> going to one or the other CPU.
> 
> The only issue might be CPU hotplug in between (the previous CPU going
> away), but I'm not sure even that matterts.
> 
> So I don't think the ring-buffer change is necessarily _wrong_, but if
> this is a performance issue, why don't we just fix it up for the
> generic case rather than for just one user?
> 
> Or is the hotplug issue a big deal? I thought we already had some
> rcu-sched point for the cpu going away, so that even the hotplug case
> should be ok.
> 
>                            Linus

Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[Christoph Lameter]

On Mon, 23 Mar 2015, Steven Rostedt wrote:

> Any comment on this?

Yes we could do this but I am traveling a bit right now. Sorry not that
responsive.

It will not be strictly correct since a fetch or write could occur from a
different cpu and thereby kick out a cacheline. But that is minor I think.

The main issue is that this makes this_cpu_read/write different from other
this_cpu operations which guarantee that these are "per cpu atomic" in the
sense that they are either completely executed or not. But that is mostly
important for this_cpu_ ops that are RMV operations like this_cpu_inc().

Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[Christoph Lameter]

Here is a potential patch for this.

Subject: [percpu] make this_cpu_read/write operate like raw_cpu_read/write

As noted threre is not much of a point in disabling preemption or interrupts
for this_cpu_read/write since the processor may change afterwards anyways
if preemption is on. Thus we should be able to use the raw versions instead.

The most that could go amiss is that the guaranteed local read/writes now
become operaton that may rarely be done from a remote cpu and thus cause
cache line evictions.

Signed-off-by: Christoph Lameter <cl@linux.com>

Index: linux/include/asm-generic/percpu.h
===================================================================
--- linux.orig/include/asm-generic/percpu.h	2014-08-05 10:43:51.569894168 -0500
+++ linux/include/asm-generic/percpu.h	2015-03-24 14:18:07.655034405 -0500
@@ -105,15 +105,6 @@ do {									\
 	(__ret);							\
 })

-#define this_cpu_generic_read(pcp)					\
-({									\
-	typeof(pcp) __ret;						\
-	preempt_disable();						\
-	__ret = *this_cpu_ptr(&(pcp));					\
-	preempt_enable();						\
-	__ret;								\
-})
-
 #define this_cpu_generic_to_op(pcp, val, op)				\
 do {									\
 	unsigned long __flags;						\
@@ -292,32 +283,40 @@ do {									\
 	raw_cpu_generic_cmpxchg_double(pcp1, pcp2, oval1, oval2, nval1, nval2)
 #endif

+/*
+ * this_cpu_read/write is not per cpu atomic but that does not matter
+ * since the cpu may change afte the operation anyways. At most we
+ * kick out a per cpu area cacheline.
+ */
 #ifndef this_cpu_read_1
-#define this_cpu_read_1(pcp)		this_cpu_generic_read(pcp)
+#define this_cpu_read_1(pcp)		raw_cpu_read_1(pcp)
 #endif
 #ifndef this_cpu_read_2
-#define this_cpu_read_2(pcp)		this_cpu_generic_read(pcp)
+#define this_cpu_read_2(pcp)		raw_cpu_read_2(pcp)
 #endif
 #ifndef this_cpu_read_4
-#define this_cpu_read_4(pcp)		this_cpu_generic_read(pcp)
+#define this_cpu_read_4(pcp)		raw_cpu_read_4(pcp)
 #endif
 #ifndef this_cpu_read_8
-#define this_cpu_read_8(pcp)		this_cpu_generic_read(pcp)
+#define this_cpu_read_8(pcp)		raw_cpu_read_8(pcp)
 #endif

 #ifndef this_cpu_write_1
-#define this_cpu_write_1(pcp, val)	this_cpu_generic_to_op(pcp, val, =)
+#define this_cpu_write_1(pcp, val)	raw_cpu_write_1(pcp, val)
 #endif
 #ifndef this_cpu_write_2
-#define this_cpu_write_2(pcp, val)	this_cpu_generic_to_op(pcp, val, =)
+#define this_cpu_write_2(pcp, val)	raw_cpu_write_2(pcp, val)
 #endif
 #ifndef this_cpu_write_4
-#define this_cpu_write_4(pcp, val)	this_cpu_generic_to_op(pcp, val, =)
+#define this_cpu_write_4(pcp, val)	raw_cpu_write_4(pcp, val)
 #endif
 #ifndef this_cpu_write_8
-#define this_cpu_write_8(pcp, val)	this_cpu_generic_to_op(pcp, val, =)
+#define this_cpu_write_8(pcp, val)	raw_cpu_write_8(pcp, val)
 #endif

+/*
+ * this cpu rmv operations must be fully per cpu atomic.
+ */
 #ifndef this_cpu_add_1
 #define this_cpu_add_1(pcp, val)	this_cpu_generic_to_op(pcp, val, +=)
 #endif

Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[Linus Torvalds]

On Thu, Mar 19, 2015 at 3:16 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So I don't think the ring-buffer change is necessarily _wrong_, but if
> this is a performance issue, why don't we just fix it up for the
> generic case rather than for just one user?

This this_cpu_generic_read/this_cpu_generic_write() performance thing
seems to have dropped off everybody's radar.

Do people still care? Is it an issue?

                Linus

Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[Steven Rostedt]

On Mon, 18 May 2015 12:50:43 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Mar 19, 2015 at 3:16 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > So I don't think the ring-buffer change is necessarily _wrong_, but if
> > this is a performance issue, why don't we just fix it up for the
> > generic case rather than for just one user?
> 
> This this_cpu_generic_read/this_cpu_generic_write() performance thing
> seems to have dropped off everybody's radar.
> 
> Do people still care? Is it an issue?

Note, the ring buffer issues exasperated the problem because the
preempt_disable() itself was being traced (function tracer) and this
was for the code that was to detect recursion. Luckily, the function
tracer had its own recursion detection to prevent an infinite recursion
from happening and crashing the kernel, but the double call was
definitely being noticed.

Now, as for preempt_disable() being called for a simple per cpu read,
that is probably overkill. But I think Christoph is the one to answer
this.

-- Steve

Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[Uwe Kleine-König]

Hello Steven,

On Thu, Mar 19, 2015 at 06:02:16PM -0400, Steven Rostedt wrote:
>     Reported-by: Uwe Kleine-KÃ=B6nig <u.kleine-koenig@pengutronix.de>
>     Tested-by: Uwe Kleine-KÃ=B6nig <u.kleine-koenig@pengutronix.de>
Would you please care to type my name correctly? I just checked, the git
commit is wrong, too.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[Steven Rostedt]

On Fri, 20 Mar 2015 08:26:33 +0100
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> Hello Steven,
> 
> On Thu, Mar 19, 2015 at 06:02:16PM -0400, Steven Rostedt wrote:
> >     Reported-by: Uwe Kleine-KÃ=B6nig <u.kleine-koenig@pengutronix.de>
> >     Tested-by: Uwe Kleine-KÃ=B6nig <u.kleine-koenig@pengutronix.de>
> Would you please care to type my name correctly? I just checked, the git
> commit is wrong, too.
> 

Hmm, strange. I did a cut and paste from your email. There seems to
have been some character mapping issue.

I don't have a keyboard with the umlauts, so I can't type it. The best
I can do is "Koenig", or cut and paste, and cut and paste doesn't seem
to work for me.

-- Steve