[PATCH] tracing: Remove pointless memory barriers

[PATCH] tracing: Remove pointless memory barriers

[Nam Cao]

Memory barriers are useful to ensure memory accesses from one CPU appear in
the original order as seen by other CPUs.

Some smp_rmb() and smp_wmb() are used, but they are not ordering multiple
memory accesses.

Remove them.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
This is something I noticed while staring at RV code. And I couldn't
understand them. It seems RV code copied this from trace core, and it also
doesn't make sense to me there.

Memory barriers are useful if we have something like:

CPU1:              CPU2
write A            read B
write B            read A

From this code, if CPU2 reads the new B, then it "should" also read the new
A.

But CPU1 could reorder the writes, and CPU2 sees the new B, but still the
old A.

Memory barrier is useful here:

CPU1:              CPU2
write A            read B
smp_wb()           smp_rb()
write B            read A

Then if CPU2 sees the new B, and it will also see the new A.

However, the memory barriers I see in kernel/trace/ do not resemble the
above pattern. Therefore I think they are redundant.

Please let me know if there is an unobvious reason for them.

 kernel/trace/rv/rv.c | 6 ------
 kernel/trace/trace.c | 7 -------
 2 files changed, 13 deletions(-)

diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
index e4077500a91db..c04a49da43286 100644
--- a/kernel/trace/rv/rv.c
+++ b/kernel/trace/rv/rv.c
@@ -675,8 +675,6 @@ static bool __read_mostly monitoring_on;
  */
 bool rv_monitoring_on(void)
 {
-	/* Ensures that concurrent monitors read consistent monitoring_on */
-	smp_rmb();
 	return READ_ONCE(monitoring_on);
 }
 
@@ -696,8 +694,6 @@ static ssize_t monitoring_on_read_data(struct file *filp, char __user *user_buf,
 static void turn_monitoring_off(void)
 {
 	WRITE_ONCE(monitoring_on, false);
-	/* Ensures that concurrent monitors read consistent monitoring_on */
-	smp_wmb();
 }
 
 static void reset_all_monitors(void)
@@ -713,8 +709,6 @@ static void reset_all_monitors(void)
 static void turn_monitoring_on(void)
 {
 	WRITE_ONCE(monitoring_on, true);
-	/* Ensures that concurrent monitors read consistent monitoring_on */
-	smp_wmb();
 }
 
 static void turn_monitoring_on_with_reset(void)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 95ae7c4e58357..0dff4298fc0e5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -936,7 +936,6 @@ int tracing_is_enabled(void)
 	 * return the mirror variable of the state of the ring buffer.
 	 * It's a little racy, but we don't really care.
 	 */
-	smp_rmb();
 	return !global_trace.buffer_disabled;
 }
 
@@ -1107,8 +1106,6 @@ void tracer_tracing_on(struct trace_array *tr)
 	 * important to be fast than accurate.
 	 */
 	tr->buffer_disabled = 0;
-	/* Make the flag seen by readers */
-	smp_wmb();
 }
 
 /**
@@ -1640,8 +1637,6 @@ void tracer_tracing_off(struct trace_array *tr)
 	 * important to be fast than accurate.
 	 */
 	tr->buffer_disabled = 1;
-	/* Make the flag seen by readers */
-	smp_wmb();
 }
 
 /**
@@ -2710,8 +2705,6 @@ void trace_buffered_event_enable(void)
 
 static void enable_trace_buffered_event(void *data)
 {
-	/* Probably not needed, but do it anyway */
-	smp_rmb();
 	this_cpu_dec(trace_buffered_event_cnt);
 }
 
-- 
2.39.5

Re: [PATCH] tracing: Remove pointless memory barriers

[Steven Rostedt]

On Thu, 26 Jun 2025 17:19:40 +0200
Nam Cao <namcao@linutronix.de> wrote:


> However, the memory barriers I see in kernel/trace/ do not resemble the
> above pattern. Therefore I think they are redundant.

I'll focus my comments on the trace code as that's what I understand more.

> 
> Please let me know if there is an unobvious reason for them.

Sure!


>  static void turn_monitoring_on_with_reset(void)
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 95ae7c4e58357..0dff4298fc0e5 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -936,7 +936,6 @@ int tracing_is_enabled(void)
>  	 * return the mirror variable of the state of the ring buffer.
>  	 * It's a little racy, but we don't really care.
>  	 */
> -	smp_rmb();
>  	return !global_trace.buffer_disabled;
>  }
>  
> @@ -1107,8 +1106,6 @@ void tracer_tracing_on(struct trace_array *tr)
>  	 * important to be fast than accurate.
>  	 */
>  	tr->buffer_disabled = 0;
> -	/* Make the flag seen by readers */
> -	smp_wmb();
>  }
>  
>  /**
> @@ -1640,8 +1637,6 @@ void tracer_tracing_off(struct trace_array *tr)
>  	 * important to be fast than accurate.
>  	 */
>  	tr->buffer_disabled = 1;
> -	/* Make the flag seen by readers */
> -	smp_wmb();
>  }

The above three interact with each other. Without the barriers, the
tr->buffer_disabled = 0, can be set on one CPU, and the other CPU can think
the buffer is still enabled and do work that will end up doing nothing. Or
it can be set to 1, and the other CPU still sees it disabled and will not
do work when it can.

>  
>  /**
> @@ -2710,8 +2705,6 @@ void trace_buffered_event_enable(void)
>  
>  static void enable_trace_buffered_event(void *data)
>  {
> -	/* Probably not needed, but do it anyway */
> -	smp_rmb();

As the comment says, this one actually isn't needed, and yes, it can be
removed.

-- Steve

>  	this_cpu_dec(trace_buffered_event_cnt);
>  }
>  

Re: [PATCH] tracing: Remove pointless memory barriers

[Steven Rostedt]

On Thu, 26 Jun 2025 11:35:20 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> > @@ -2710,8 +2705,6 @@ void trace_buffered_event_enable(void)
> >  
> >  static void enable_trace_buffered_event(void *data)
> >  {
> > -	/* Probably not needed, but do it anyway */
> > -	smp_rmb();  
> 
> As the comment says, this one actually isn't needed, and yes, it can be
> removed.

One reason this isn't needed is because it's called via on_each_cpu() which
will trigger its own barriers.

-- Steve

Re: [PATCH] tracing: Remove pointless memory barriers

[Nam Cao]

On Thu, Jun 26, 2025 at 11:35:20AM -0400, Steven Rostedt wrote:
> On Thu, 26 Jun 2025 17:19:40 +0200
> Nam Cao <namcao@linutronix.de> wrote:
> >  static void turn_monitoring_on_with_reset(void)
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 95ae7c4e58357..0dff4298fc0e5 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -936,7 +936,6 @@ int tracing_is_enabled(void)
> >  	 * return the mirror variable of the state of the ring buffer.
> >  	 * It's a little racy, but we don't really care.
> >  	 */
> > -	smp_rmb();
> >  	return !global_trace.buffer_disabled;
> >  }
> >  
> > @@ -1107,8 +1106,6 @@ void tracer_tracing_on(struct trace_array *tr)
> >  	 * important to be fast than accurate.
> >  	 */
> >  	tr->buffer_disabled = 0;
> > -	/* Make the flag seen by readers */
> > -	smp_wmb();
> >  }
> >  
> >  /**
> > @@ -1640,8 +1637,6 @@ void tracer_tracing_off(struct trace_array *tr)
> >  	 * important to be fast than accurate.
> >  	 */
> >  	tr->buffer_disabled = 1;
> > -	/* Make the flag seen by readers */
> > -	smp_wmb();
> >  }
> 
> The above three interact with each other. Without the barriers, the
> tr->buffer_disabled = 0, can be set on one CPU, and the other CPU can think
> the buffer is still enabled and do work that will end up doing nothing. Or
> it can be set to 1, and the other CPU still sees it disabled and will not
> do work when it can.

(I'm not that experienced with memory barrier, so I may be writing nonsense
here)

I think you have it inverted? I assume you meant:

"Without the barriers, the tr->buffer_disabled = 1 can be set on one CPU,
and the other CPU can think the buffer is still enabled and do work that
will end up doing nothing."

Your scenario can still happen despite the memory barrier:

CPU1                          CPU2
                              smp_rb()
                              read buffer_disabled, see 0 --> let's do work!
buffer_disabled=1
smp_wb()
                              do work -> end up doing nothing

From my understanding, smp_wb()'s purpose is ensuring the ordering of one
write and another write, e.g.:
    write(a)
    smp_wb()
    write(b)

For our case, there is only a single write. Therefore I don't think
smp_wb() is useful.

Best regards,
Nam

Re: [PATCH] tracing: Remove pointless memory barriers

[Steven Rostedt]

On Thu, 26 Jun 2025 18:04:59 +0200
Nam Cao <namcao@linutronix.de> wrote:

> I think you have it inverted? I assume you meant:
> 
> "Without the barriers, the tr->buffer_disabled = 1 can be set on one CPU,
> and the other CPU can think the buffer is still enabled and do work that
> will end up doing nothing."
> 
> Your scenario can still happen despite the memory barrier:

Yes, but the point isn't really to prevent the race. It's more about making
the race window smaller.

When we disable it, if something is currently using it then it may or may
not get in. That's fine as this isn't critical.

But from my understanding, without the barriers, some architectures may
never see the update. That is, the write from one CPU may not get to memory
for a long time and new incoming readers will still see the old data. I'm
more concerned with new readers than ones that are currently racing with
the updates.

> 
> CPU1                          CPU2
>                               smp_rb()
>                               read buffer_disabled, see 0 --> let's do work!
> buffer_disabled=1
> smp_wb()
>                               do work -> end up doing nothing
> 
> >From my understanding, smp_wb()'s purpose is ensuring the ordering of one  
> write and another write, e.g.:
>     write(a)
>     smp_wb()
>     write(b)
> 
> For our case, there is only a single write. Therefore I don't think
> smp_wb() is useful.

Well, it does make it visible for other CPUs that do not have strong cache
coherency.

-- Steve

Re: [PATCH] tracing: Remove pointless memory barriers

[John Ogness]

Hi Steven,

On 2025-06-26, Steven Rostedt <rostedt@goodmis.org> wrote:
>> Your scenario can still happen despite the memory barrier:
>
> Yes, but the point isn't really to prevent the race. It's more about making
> the race window smaller.
>
> When we disable it, if something is currently using it then it may or may
> not get in. That's fine as this isn't critical.
>
> But from my understanding, without the barriers, some architectures may
> never see the update. That is, the write from one CPU may not get to memory
> for a long time and new incoming readers will still see the old data. I'm
> more concerned with new readers than ones that are currently racing with
> the updates.

Memory barriers do not affect visibility. They only affect ordering. And
ordering implies that there are at least 2 pieces of data involved. A
memory barrier has no meaning when you are only talking about 1 piece of
data (in this case @buffer_disabled).

For example, update_traceon_count() has an smp_rmb()/smp_wmb() pair to
make sure @count updates are ordered to be after @buffer_disabled
updates.

read(count)
smp_rmb()
read(buffer_disabled)

write(buffer_disabled)
smp_wmb()
write(count)

But what exactly are the memory barriers removed in this patch ordering?

John Ogness

Re: [PATCH] tracing: Remove pointless memory barriers

[Gabriele Monaco]

On Thu, 2025-06-26 at 19:47 +0206, John Ogness wrote:
> Hi Steven,
> 
> On 2025-06-26, Steven Rostedt <rostedt@goodmis.org> wrote:
> > > Your scenario can still happen despite the memory barrier:
> > 
> > Yes, but the point isn't really to prevent the race. It's more
> > about making
> > the race window smaller.
> > 
> > When we disable it, if something is currently using it then it may
> > or may
> > not get in. That's fine as this isn't critical.
> > 
> > But from my understanding, without the barriers, some architectures
> > may
> > never see the update. That is, the write from one CPU may not get
> > to memory
> > for a long time and new incoming readers will still see the old
> > data. I'm
> > more concerned with new readers than ones that are currently racing
> > with
> > the updates.
> 
> Memory barriers do not affect visibility. They only affect ordering.
> And
> ordering implies that there are at least 2 pieces of data involved. A
> memory barrier has no meaning when you are only talking about 1 piece
> of
> data (in this case @buffer_disabled).
> 
> For example, update_traceon_count() has an smp_rmb()/smp_wmb() pair
> to
> make sure @count updates are ordered to be after @buffer_disabled
> updates.
> 
> read(count)
> smp_rmb()
> read(buffer_disabled)
> 
> write(buffer_disabled)
> smp_wmb()
> write(count)
> 
> But what exactly are the memory barriers removed in this patch
> ordering?
> 

Hi all,

these statements made me curious: I always thought of memory barriers as a way
to order reads and writes to the same address across different CPUs (in other
words, for visibility).

For instance I'd do something like:

CPU 1             CPU2

write(x)
smp_mb()
                  <implicit paired barrier>
                  READ_ONCE(x)

Now, I get there isn't much we can do if reader and writer are racing, but, as
Steve said, I'm expecting the presence of barriers to make the racing window
smaller.

Am I misinterpreting the whole thing here? Are those barriers just ordering
reads with reads and writes with writes (hence useful only with multiple
variables)?

Thanks,
Gabriele

Re: [PATCH] tracing: Remove pointless memory barriers

[Nam Cao]

On Thu, Jul 03, 2025 at 10:05:59AM +0200, Gabriele Monaco wrote:
> Hi all,
Hi Gabriele,

> these statements made me curious: I always thought of memory barriers as a way
> to order reads and writes to the same address across different CPUs (in other
> words, for visibility).
> 
> For instance I'd do something like:
> 
> CPU 1             CPU2
> 
> write(x)
> smp_mb()
>                   <implicit paired barrier>
>                   READ_ONCE(x)
> 
> Now, I get there isn't much we can do if reader and writer are racing, but, as
> Steve said, I'm expecting the presence of barriers to make the racing window
> smaller.
> 
> Am I misinterpreting the whole thing here? Are those barriers just ordering
> reads with reads and writes with writes (hence useful only with multiple
> variables)?

From "WHAT ARE MEMORY BARRIERS?" section in
https://www.kernel.org/doc/Documentation/memory-barriers.txt

    "Memory barriers [...] impose a perceived partial ordering over the
    memory operations on either side of the barrier."

and also have a look at "WHAT MAY NOT BE ASSUMED ABOUT MEMORY BARRIERS?"
later on:

    "There is no guarantee that any of the memory accesses specified before
    a memory barrier will be _complete_ by the completion of a memory
    barrier instruction"

Or data memory barrier explanation from Arm
(https://developer.arm.com/documentation/den0042/0100/Memory-Ordering/Memory-barriers)

    "This instruction ensures that all memory accesses in program order
    before the barrier are observed in the system before any explicit
    memory accesses that appear in program order after the barrier. It does
    not affect the ordering of any other instructions executing on the
    processor, or of instruction fetches."

So yes, smp_rmb() is only useful inbetween reads, and smp_wmb() is only
userful inbetween writes.

Best regards,
Nam

Re: [PATCH] tracing: Remove pointless memory barriers

[David Laight]

On Thu, 26 Jun 2025 12:34:45 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 26 Jun 2025 18:04:59 +0200
> Nam Cao <namcao@linutronix.de> wrote:
> 
> > I think you have it inverted? I assume you meant:
> > 
> > "Without the barriers, the tr->buffer_disabled = 1 can be set on one CPU,
> > and the other CPU can think the buffer is still enabled and do work that
> > will end up doing nothing."
> > 
> > Your scenario can still happen despite the memory barrier:  
> 
> Yes, but the point isn't really to prevent the race. It's more about making
> the race window smaller.
> 
> When we disable it, if something is currently using it then it may or may
> not get in. That's fine as this isn't critical.
> 
> But from my understanding, without the barriers, some architectures may
> never see the update. That is, the write from one CPU may not get to memory
> for a long time and new incoming readers will still see the old data. I'm
> more concerned with new readers than ones that are currently racing with
> the updates.

I'm not an expert here, but I don't think the barriers necessarily do anything
to force writes out of the 'store buffer' (so the data gets into the cache
from where it will be snooped).

An implementation of 'wmb' might wait for the store buffer (or other scheme
for pending writes) to empty, but it only has to use a marker to ensure
ordering.

The actual writes of data to the data cache are also likely to happen
'in their own time' regardless of the code the cpu is executing (although
cache line reads from main memory (for reads) may take preference over
those for writes).

Thinks...
A plausible model is that write data is buffered on a cache-line basis
waiting for the old cache line to be read from memory.
While that is happening later writes can be written into other cache lines.

So a 'wmb' might just stall the cpu that executes it without having any
real effect on the timings of the memory updates seen by other cpu.

	David

 

Re: [PATCH] tracing: Remove pointless memory barriers

[Steven Rostedt]

On Tue, 8 Jul 2025 09:42:19 +0200
Nam Cao <namcao@linutronix.de> wrote:

> So yes, smp_rmb() is only useful inbetween reads, and smp_wmb() is
> only userful inbetween writes.

Hmm, I wonder if barriers isn't needed but atomic values are?

That is, it looks like rv_monitoring_on() is looking to read the
current state, where as turn_monitoring_on/off() changes the state.

Perhaps instead of barriers, it should use atomics?

 bool rv_monitoring_on(void)
 {
	return atomic_read(&monitoring_on);
 }
 
 static void turn_monitoring_off(void)
 {
	atomic_set(&monitoring_on, 0);
 }
 

Doesn't atomic make sure the values are seen when they are changed?

As this code is more about looking at state and not ordering, and I
think that's what atomics are about.

-- Steve

Re: [PATCH] tracing: Remove pointless memory barriers

[David Laight]

On Wed, 9 Jul 2025 11:08:27 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 8 Jul 2025 09:42:19 +0200
> Nam Cao <namcao@linutronix.de> wrote:
> 
> > So yes, smp_rmb() is only useful inbetween reads, and smp_wmb() is
> > only userful inbetween writes.  
> 
> Hmm, I wonder if barriers isn't needed but atomic values are?
> 
> That is, it looks like rv_monitoring_on() is looking to read the
> current state, where as turn_monitoring_on/off() changes the state.
> 
> Perhaps instead of barriers, it should use atomics?
> 
>  bool rv_monitoring_on(void)
>  {
> 	return atomic_read(&monitoring_on);
>  }
>  
>  static void turn_monitoring_off(void)
>  {
> 	atomic_set(&monitoring_on, 0);
>  }
>  
> 
> Doesn't atomic make sure the values are seen when they are changed?

No.
It normally just ensures the read/write aren't 'torn'.
Atomics are used for read-modify-writes to ensure two cpu don't
do read-read-modify-modify-write-write losing one of the changes.
(They can need special instructions for read and write - but normally don't.)
So here just the same as the volatile accesses READ_ONCE() and WRITE_ONCE().

	David


> 
> As this code is more about looking at state and not ordering, and I
> think that's what atomics are about.
> 
> -- Steve
> 

Re: [PATCH] tracing: Remove pointless memory barriers

[Steven Rostedt]

On Fri, 11 Jul 2025 09:29:46 +0100
David Laight <david.laight.linux@gmail.com> wrote:

> > Doesn't atomic make sure the values are seen when they are changed?  
> 
> No.
> It normally just ensures the read/write aren't 'torn'.
> Atomics are used for read-modify-writes to ensure two cpu don't
> do read-read-modify-modify-write-write losing one of the changes.
> (They can need special instructions for read and write - but normally don't.)
> So here just the same as the volatile accesses READ_ONCE() and WRITE_ONCE().

At first I was about to say "But wait! I rely on this to work in other
parts of my code", but then realized I use atomic_inc_return() and
similar that actually do make the update atomic across CPUs.

-- Steve

Re: [PATCH] tracing: Remove pointless memory barriers

[Steven Rostedt]

On Thu, 26 Jun 2025 18:04:59 +0200
Nam Cao <namcao@linutronix.de> wrote:

> > The above three interact with each other. Without the barriers, the
> > tr->buffer_disabled = 0, can be set on one CPU, and the other CPU can think
> > the buffer is still enabled and do work that will end up doing nothing. Or
> > it can be set to 1, and the other CPU still sees it disabled and will not
> > do work when it can.  
> 
> (I'm not that experienced with memory barrier, so I may be writing nonsense
> here)

So I did some git archeology and realized that the buffer_disabled is just
a cache to make irqsoff tracing faster. The commit even said it's not going
to be accurate.

OK, I'll just take this patch.

Thanks,

-- Steve