Re: [PATCH] perf/ring_buffer: ensure atomicity and order of updates

[Peter Zijlstra]

On Mon, May 14, 2018 at 12:05:33PM +0100, Mark Rutland wrote:
> On Fri, May 11, 2018 at 06:22:29PM +0200, Peter Zijlstra wrote:
> > On Fri, May 11, 2018 at 11:59:32AM +0100, Mark Rutland wrote:
> > > READ_ONCE() and WRITE_ONCE() "helpfully" make a silent fallback to a
> > > memcpy in this case, so we're broken today, regardless of this change.
> > > 
> > > I suspect that in practice we get single-copy-atomicity for the 32-bit
> > > halves, and sessions likely produce less than 4GiB of ringbuffer data,
> > > so failures would be rare.
> > 
> > This should not be a problem because of the 32bit adress space limit,
> > which would necessarily limit us to the low word.
> 
> For the wrapped values, yes.
> 
> I thought that the head and tail values were meant to be free-running,
> but I can't see where I got that idea from now that I've gone digging
> again.

They are indeed free running.

> > Also note that in perf_output_put_handle(), where we write ->data_head,
> > the store is from an 'unsigned long'. So on 32bit that will result in a
> > zero high word. Similarly, in __perf_output_begin() we read ->data_tail
> > into an unsigned long, which will discard the high word.
> 
> Ah, that's a fair point. So it's just compat userspace that this is
> potentially borked for. ;)

Right.. #$$#@ compat. Hurmph.. not sure how to go about fixing that
there.

> > So userspace should always read (head) a zero high word, irrespective of
> > a split store (2x32bit), and the kernel will disregard the high word on
> > reading (tail), irrespective of what userspace put there.
> > 
> > This is all a bit subtle, and could probably use a comment, but it ought
> > to work..
> 
> It would be nice to guarantee that we don't lose 32-bit atomicity by
> virtue of {READ,WRITE}_ONCE() falling back to memcpy in this case, so
> maybe we should wrap this in some helpers.

Our __READ_ONCE_SIZE / __write_once_size include case 8 unconditionally.
So we'll always issue a volatile u64 load/store and let the compiler
figure out how to do that -- typically 2 load/stores I would imagine.