Re: [PATCH] ring-buffer: use READ_ONCE() to read cpu_buffer->commit_page in concurrent environment

Re: [PATCH] ring-buffer: use READ_ONCE() to read cpu_buffer->commit_page in concurrent environment

[linke]

Hi Steven, sorry for the late reply.

> 
> Now the reason for the above READ_ONCE() is because the variables *are*
> going to be used again. We do *not* want the compiler to play any games
> with that.
> 

I don't think it is because the variables are going to be used again. 
Compiler optimizations barely do bad things in single thread programs. It
is because cpu_buffer->commit_page may change concurrently and should be
accessed atomically.

	/* Make sure commit page didn't change */
	curr_commit_page = READ_ONCE(cpu_buffer->commit_page);
	curr_commit_ts = READ_ONCE(curr_commit_page->page->time_stamp);

	/* If the commit page changed, then there's more data */
	if (curr_commit_page != commit_page ||
	    curr_commit_ts != commit_ts)
		return 0;

This code read cpu_buffer->commit_page and time_stamp again to check
whether commit page changed. It shows that cpu_buffer->commit_page and 
time_stamp may be changed by other threads.

        commit_page = cpu_buffer->commit_page;
        commit_ts = commit_page->page->time_stamp;

So the commit_page and time_stamp above is read while other threads may
change it. I think it is a data race if it is not atomic. Thus it is 
necessary to use READ_ONCE() here.

Re: [PATCH] ring-buffer: use READ_ONCE() to read cpu_buffer->commit_page in concurrent environment

[linke]

> So basically you are worried about read-tearing?
> 
> That wasn't mentioned in the change log.

Yes. Sorry for making this confused, I am not very familiar with this and
still learning.

> Funny part is, if the above timestamp read did a tear, then this would
> definitely not match, and would return the correct value. That is, the
> buffer is not empty because the only way for this to get corrupted is if
> something is in the process of writing to it.

I agree with you here.

	commit = rb_page_commit(commit_page);

But if commit_page above is the result of a torn read, the commit field
read by rb_page_commit() may not represent a valid value. 

In this case, READ_ONCE() is only needed for the commit_page.

Re: [PATCH] ring-buffer: use READ_ONCE() to read cpu_buffer->commit_page in concurrent environment

[Steven Rostedt]

On Fri, 1 Mar 2024 13:37:18 +0800
linke <lilinke99@qq.com> wrote:

> > So basically you are worried about read-tearing?
> > 
> > That wasn't mentioned in the change log.  
> 
> Yes. Sorry for making this confused, I am not very familiar with this and
> still learning.

No problem. We all have to learn this anyway.

> 
> > Funny part is, if the above timestamp read did a tear, then this would
> > definitely not match, and would return the correct value. That is, the
> > buffer is not empty because the only way for this to get corrupted is if
> > something is in the process of writing to it.  
> 
> I agree with you here.
> 
> 	commit = rb_page_commit(commit_page);
> 
> But if commit_page above is the result of a torn read, the commit field
> read by rb_page_commit() may not represent a valid value. 

But commit_page is a word length, and I will argue that any compiler that
tears "long" words is broken. ;-)

> 
> In this case, READ_ONCE() is only needed for the commit_page.

But we can at least keep the READ_ONCE() on the commit_page just because it
is used in the next instruction.

-- Steve

Re: [PATCH] ring-buffer: use READ_ONCE() to read cpu_buffer->commit_page in concurrent environment

[Mathieu Desnoyers]

On 2024-03-01 10:49, Steven Rostedt wrote:
> On Fri, 1 Mar 2024 13:37:18 +0800
> linke <lilinke99@qq.com> wrote:
> 
>>> So basically you are worried about read-tearing?
>>>
>>> That wasn't mentioned in the change log.
>>
>> Yes. Sorry for making this confused, I am not very familiar with this and
>> still learning.
> 
> No problem. We all have to learn this anyway.
> 
>>
>>> Funny part is, if the above timestamp read did a tear, then this would
>>> definitely not match, and would return the correct value. That is, the
>>> buffer is not empty because the only way for this to get corrupted is if
>>> something is in the process of writing to it.
>>
>> I agree with you here.
>>
>> 	commit = rb_page_commit(commit_page);
>>
>> But if commit_page above is the result of a torn read, the commit field
>> read by rb_page_commit() may not represent a valid value.
> 
> But commit_page is a word length, and I will argue that any compiler that
> tears "long" words is broken. ;-)

[ For those tuning in, we are discussing ring_buffer_iter_empty()
   "commit_page = cpu_buffer->commit_page;" racy load. ]

I counter-argue that real-world compilers *are* broken based on your
personal definition, but we have to deal with them, as documented
in Documentation/memory-barriers.txt (see below).

What is the added overhead of using a READ_ONCE() there ? Why are
we wasting effort trying to guess the compiler behavior if the
real-world performance impact is insignificant ?

Quote from memory-barrier.txt explaining the purpose of {READ,WRITE}_ONCE():

"(*) For aligned memory locations whose size allows them to be accessed
      with a single memory-reference instruction, prevents "load tearing"
      and "store tearing," in which a single large access is replaced by
      multiple smaller accesses."

I agree that {READ,WRITE}_ONCE() are really not needed at initialization,
when there are demonstrably no concurrent accesses to the data

But trying to eliminate {READ,WRITE}_ONCE() on concurrently accessed fields
just adds complexity, prevents static analyzers to properly understand the
code and report issues, and just obfuscates the code.

Thanks,

Mathieu

> 
>>
>> In this case, READ_ONCE() is only needed for the commit_page.
> 
> But we can at least keep the READ_ONCE() on the commit_page just because it
> is used in the next instruction.
> 
> -- Steve

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [PATCH] ring-buffer: use READ_ONCE() to read cpu_buffer->commit_page in concurrent environment

[Steven Rostedt]

On Fri, 1 Mar 2024 11:37:54 -0500
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> On 2024-03-01 10:49, Steven Rostedt wrote:
> > On Fri, 1 Mar 2024 13:37:18 +0800
> > linke <lilinke99@qq.com> wrote:
> >   
> >>> So basically you are worried about read-tearing?
> >>>
> >>> That wasn't mentioned in the change log.  
> >>
> >> Yes. Sorry for making this confused, I am not very familiar with this and
> >> still learning.  
> > 
> > No problem. We all have to learn this anyway.
> >   
> >>  
> >>> Funny part is, if the above timestamp read did a tear, then this would
> >>> definitely not match, and would return the correct value. That is, the
> >>> buffer is not empty because the only way for this to get corrupted is if
> >>> something is in the process of writing to it.  
> >>
> >> I agree with you here.
> >>
> >> 	commit = rb_page_commit(commit_page);
> >>
> >> But if commit_page above is the result of a torn read, the commit field
> >> read by rb_page_commit() may not represent a valid value.  
> > 
> > But commit_page is a word length, and I will argue that any compiler that
> > tears "long" words is broken. ;-)  
> 
> [ For those tuning in, we are discussing ring_buffer_iter_empty()
>    "commit_page = cpu_buffer->commit_page;" racy load. ]
> 
> I counter-argue that real-world compilers *are* broken based on your
> personal definition, but we have to deal with them, as documented
> in Documentation/memory-barriers.txt (see below).
> 
> What is the added overhead of using a READ_ONCE() there ? Why are
> we wasting effort trying to guess the compiler behavior if the
> real-world performance impact is insignificant ?
> 
> Quote from memory-barrier.txt explaining the purpose of {READ,WRITE}_ONCE():
> 
> "(*) For aligned memory locations whose size allows them to be accessed
>       with a single memory-reference instruction, prevents "load tearing"
>       and "store tearing," in which a single large access is replaced by
>       multiple smaller accesses."
> 
> I agree that {READ,WRITE}_ONCE() are really not needed at initialization,
> when there are demonstrably no concurrent accesses to the data
> 
> But trying to eliminate {READ,WRITE}_ONCE() on concurrently accessed fields
> just adds complexity, prevents static analyzers to properly understand the
> code and report issues, and just obfuscates the code.
> 
> Thanks,
> 
> Mathieu
> 
> >   
> >>
> >> In this case, READ_ONCE() is only needed for the commit_page.  
> > 
> > But we can at least keep the READ_ONCE() on the commit_page just because it
> > is used in the next instruction.
> 

And here I did state that READ_ONCE() does have another use case. So
there's no argument about adding it here.

-- Steve

Re: [PATCH] ring-buffer: use READ_ONCE() to read cpu_buffer->commit_page in concurrent environment

[linke]

Hi Steven, sorry for the late reply.

> 
> Now the reason for the above READ_ONCE() is because the variables *are*
> going to be used again. We do *not* want the compiler to play any games
> with that.
> 

I don't think it is because the variables are going to be used again. 
Compiler optimizations barely do bad things in single thread programs. It
is because cpu_buffer->commit_page may change concurrently and should be
accessed atomically.

	/* Make sure commit page didn't change */
	curr_commit_page = READ_ONCE(cpu_buffer->commit_page);
	curr_commit_ts = READ_ONCE(curr_commit_page->page->time_stamp);

	/* If the commit page changed, then there's more data */
	if (curr_commit_page != commit_page ||
	    curr_commit_ts != commit_ts)
		return 0;

This code read cpu_buffer->commit_page and time_stamp again to check
whether commit page changed. It shows that cpu_buffer->commit_page and 
time_stamp may be changed by other threads.

        commit_page = cpu_buffer->commit_page;
        commit_ts = commit_page->page->time_stamp;

So the commit_page and time_stamp above is read while other threads may
change it. I think it is a data race if it is not atomic. Thus it is 
necessary to use READ_ONCE() here.

Re: [PATCH] ring-buffer: use READ_ONCE() to read cpu_buffer->commit_page in concurrent environment

[Steven Rostedt]

On Thu, 29 Feb 2024 20:32:26 +0800
linke <lilinke99@qq.com> wrote:

> Hi Steven, sorry for the late reply.
> 
> > 
> > Now the reason for the above READ_ONCE() is because the variables *are*
> > going to be used again. We do *not* want the compiler to play any games
> > with that.
> >   
> 
> I don't think it is because the variables are going to be used again. 
> Compiler optimizations barely do bad things in single thread programs. It
> is because cpu_buffer->commit_page may change concurrently and should be
> accessed atomically.

So basically you are worried about read-tearing?

That wasn't mentioned in the change log.

> 
> 	/* Make sure commit page didn't change */
> 	curr_commit_page = READ_ONCE(cpu_buffer->commit_page);
> 	curr_commit_ts = READ_ONCE(curr_commit_page->page->time_stamp);
> 
> 	/* If the commit page changed, then there's more data */
> 	if (curr_commit_page != commit_page ||
> 	    curr_commit_ts != commit_ts)
> 		return 0;
> 
> This code read cpu_buffer->commit_page and time_stamp again to check
> whether commit page changed. It shows that cpu_buffer->commit_page and 
> time_stamp may be changed by other threads.
> 
>         commit_page = cpu_buffer->commit_page;
>         commit_ts = commit_page->page->time_stamp;
> 
> So the commit_page and time_stamp above is read while other threads may
> change it. I think it is a data race if it is not atomic. Thus it is 
> necessary to use READ_ONCE() here.

Funny part is, if the above timestamp read did a tear, then this would
definitely not match, and would return the correct value. That is, the
buffer is not empty because the only way for this to get corrupted is if
something is in the process of writing to it.

Now we could add a comment stating this.

So, I don't even think the reading of the commit_page is needed (it's long
size so it should not tear, and if it does, I consider that more a bug in
the compiler).

Please explain why READ_ONCE() is needed, and what exactly is it "fixing".
That is, what breaks if it's not there?

-- Steve

[PATCH] ring-buffer: use READ_ONCE() to read cpu_buffer->commit_page in concurrent environment

[linke li]

In function ring_buffer_iter_empty(), cpu_buffer->commit_page and
curr_commit_page->page->time_stamp is read using READ_ONCE() in 
line 4354, 4355

4354    curr_commit_page = READ_ONCE(cpu_buffer->commit_page);
4355    curr_commit_ts = READ_ONCE(curr_commit_page->page->time_stamp);

while they are read directly in line 4340, 4341

4340    commit_page = cpu_buffer->commit_page;
4341    commit_ts = commit_page->page->time_stamp;

There is patch similar to this. commit c1c0ce31b242 ("r8169: fix the KCSAN reported data-race in rtl_tx() while reading tp->cur_tx")
This patch find two read of same variable while one is protected, another
is not. And READ_ONCE() is added to protect.

Signed-off-by: linke li <lilinke99@qq.com>
---
 kernel/trace/ring_buffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 0699027b4f4c..eb3fa629b837 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -4337,8 +4337,8 @@ int ring_buffer_iter_empty(struct ring_buffer_iter *iter)
 	cpu_buffer = iter->cpu_buffer;
 	reader = cpu_buffer->reader_page;
 	head_page = cpu_buffer->head_page;
-	commit_page = cpu_buffer->commit_page;
-	commit_ts = commit_page->page->time_stamp;
+	commit_page = READ_ONCE(cpu_buffer->commit_page);
+	commit_ts = READ_ONCE(commit_page->page->time_stamp);
 
 	/*
 	 * When the writer goes across pages, it issues a cmpxchg which
-- 
2.39.3 (Apple Git-145)

Re: [PATCH] ring-buffer: use READ_ONCE() to read cpu_buffer->commit_page in concurrent environment

[Steven Rostedt]

On Sun, 25 Feb 2024 11:05:06 +0800
linke li <lilinke99@qq.com> wrote:

> In function ring_buffer_iter_empty(), cpu_buffer->commit_page and
> curr_commit_page->page->time_stamp is read using READ_ONCE() in 
> line 4354, 4355
> 
> 4354    curr_commit_page = READ_ONCE(cpu_buffer->commit_page);
> 4355    curr_commit_ts = READ_ONCE(curr_commit_page->page->time_stamp);
> 
> while they are read directly in line 4340, 4341
> 
> 4340    commit_page = cpu_buffer->commit_page;
> 4341    commit_ts = commit_page->page->time_stamp;

Just because it's used in one place does not mean it's required in
another.

> 
> There is patch similar to this. commit c1c0ce31b242 ("r8169: fix the KCSAN reported data-race in rtl_tx() while reading tp->cur_tx")
> This patch find two read of same variable while one is protected, another
> is not. And READ_ONCE() is added to protect.
> 

Here's the entire code:

        cpu_buffer = iter->cpu_buffer;
        reader = cpu_buffer->reader_page;
        head_page = cpu_buffer->head_page;
        commit_page = cpu_buffer->commit_page;
        commit_ts = commit_page->page->time_stamp;

        /*
         * When the writer goes across pages, it issues a cmpxchg which
         * is a mb(), which will synchronize with the rmb here.
         * (see rb_tail_page_update())
         */
        smp_rmb();

The above smp_rmb() is a full read barrier. The commit_page and
timestamp are not going to be read again after this.

        commit = rb_page_commit(commit_page);
        /* We want to make sure that the commit page doesn't change */
        smp_rmb();

        /* Make sure commit page didn't change */
        curr_commit_page = READ_ONCE(cpu_buffer->commit_page);
        curr_commit_ts = READ_ONCE(curr_commit_page->page->time_stamp);

Now the reason for the above READ_ONCE() is because the variables *are*
going to be used again. We do *not* want the compiler to play any games
with that.

Thus, the first read of commit_page and time_stamp are read properly as
the compiler will not do anything that can hurt us beyond that
smp_rmb(). The second time we read those variables, we are using them
in the below code.


        /* If the commit page changed, then there's more data */
        if (curr_commit_page != commit_page ||
            curr_commit_ts != commit_ts)
                return 0;

        /* Still racy, as it may return a false positive, but that's OK */
        return ((iter->head_page == commit_page && iter->head >= commit) ||
                (iter->head_page == reader && commit_page == head_page &&
                 head_page->read == commit &&
                 iter->head == rb_page_commit(cpu_buffer->reader_page)));
}

*But* looking at this deeper, the commit_page may need a READ_ONCE()
but not for the reason you suggested.

        commit_page = cpu_buffer->commit_page;
        commit_ts = commit_page->page->time_stamp;

The commit_page above *is* used again, and we want commit_ts to be part
of the commit_page that was originally read and not a second reading.

So, I think for the commit_page we do need a READ_ONCE() but that's
because it is referenced again just below it and we don't want the
compiler to read the memory location again for the second reference.

-- Steve

Re: [PATCH] ring-buffer: use READ_ONCE() to read cpu_buffer->commit_page in concurrent environment

[Steven Rostedt]

On Sun, 25 Feb 2024 15:03:02 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> *But* looking at this deeper, the commit_page may need a READ_ONCE()
> but not for the reason you suggested.
> 
>         commit_page = cpu_buffer->commit_page;
>         commit_ts = commit_page->page->time_stamp;
> 
> The commit_page above *is* used again, and we want commit_ts to be part
> of the commit_page that was originally read and not a second reading.
> 
> So, I think for the commit_page we do need a READ_ONCE() but that's
> because it is referenced again just below it and we don't want the
> compiler to read the memory location again for the second reference.

Care to send a v2 patch that just adds READ_ONCE() for the commit_page, and
change the change log stating that it is to fix the possibility that the
time_stamp may come from a different page.

-- Steve