Messages in this thread | | | Date | Mon, 14 May 2018 12:05:33 +0100 | From | Mark Rutland <> | Subject | Re: [PATCH] perf/ring_buffer: ensure atomicity and order of updates |
| |
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.
> 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. ;)
> 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.
I'll see if I can come up with something which isn't hideous, or I might just pretend I never stumbled across this. :)
Thanks, Mark.
| |