Hi Drew, On Thu, Apr 06, 2017 at 02:02:12PM +0200, Andrew Jones wrote: > On Wed, Apr 05, 2017 at 10:20:17PM +0200, Radim Krčmář wrote: > > 2017-04-05 19:39+0200, Christoffer Dall: > > > On Wed, Apr 05, 2017 at 03:10:50PM +0200, Radim Krčmář wrote: > > >> 2017-04-04 18:41+0200, Andrew Jones: > > >> > On Tue, Apr 04, 2017 at 05:30:14PM +0200, Christoffer Dall wrote: > > >> >> On Fri, Mar 31, 2017 at 06:06:50PM +0200, Andrew Jones wrote: > > >> >> > From: Radim Krčmář <rkrcmar@xxxxxxxxxx> > > >> >> > > > >> >> > A first step in vcpu->requests encapsulation. > > >> >> > > >> >> Could we have a note here on why we need to access vcpu->requests using > > >> >> READ_ONCE now? > > >> > > > >> > Sure, maybe we should put the note as a comment above the read in > > >> > kvm_request_pending(). Something like > > >> > > > >> > /* > > >> > * vcpu->requests reads may appear in sequences that have strict > > >> > * data or control dependencies. Use READ_ONCE() to ensure the > > >> > * compiler does not do anything that breaks the required ordering. > > >> > */ > > >> > > > >> > Radim? > > >> > > >> Uses of vcpu->requests should already have barriers that take care of > > >> the ordering. I think the main reason for READ_ONCE() is to tell > > >> programmers that requests are special, but predictable. > > > > > > I don't know what to do with "special, but predictable", unfortunately. > > > In fact, I don't even think I know what you mean. > > > > With "special" to stand for the idea that vcpu->requests can change > > outside of the current execution thread. Letting the programmer assume > > additional guarantees makes the generated code and resulting behavior > > more predictable. > > > > >> READ_ONCE() is not necessary in any use I'm aware of, but there is no > > >> harm in telling the compiler that vcpu->requests are what we think they > > >> are ... > > > > > > Hmmm, I'm equally lost. > > > > vcpu->requests are volatile, so we need to assume that they can change > > at any moment when using them. > > > > I would prefer if vcpu->requests were of an atomic type and READ_ONCE() > > is about as close we can get without a major overhaul. > > > > >> > > >> /* > > >> * vcpu->requests are a lockless synchronization mechanism, where > > > > > > is the requests a synchronization mechanism? I think of it more as a > > > cross-thread communication protocol. > > > > Partly, synchronization is too restrictive and communication seems too > > generic, but probably still better. No idea how to shortly describe the > > part of vcpu->requests that prevents VM entry and that setting a request > > kicks VM out of guest mode. > > > > x86 uses KVM_REQ_MCLOCK_INPROGRESS for synchronization between cores and > > the use in this series looked very similar. > > > > >> * memory barriers are necessary for correct behavior, see > > >> * Documentation/virtual/kvm/vcpu-requests.rst. > > >> * > > >> * READ_ONCE() is not necessary for correctness, but simplifies > > >> * reasoning by constricting the generated code. > > >> */ > > >> > > >> I considered READ_ONCE() to be self-documenting. :) > > > > > > I realize that I'm probably unusually slow in this whole area, but using > > > READ_ONCE() where unnecessary doesn't help my reasoning, but makes me > > > wonder which part of this I didn't understand, so I don't seem to agree > > > with the statement that it simplifies reasoning. > > > > No, I think it is a matter of approach. When I see a READ_ONCE() > > without a comment, I think that the programmer was aware that this > > memory can change at any time and was defensive about it. > > > > I consider this use to simplify future development: > > We think now that READ_ONCE() is not needed, but vcpu->requests is still > > volatile and future changes in code might make READ_ONCE() necessary. > > Preemptively putting READ_ONCE() there saves us thinking or hard-to-find > > bugs. > > > > > Really, if there is no reason to use it, I don't think we should use it. > > > > I am leaning towards READ_ONCE() as the default for implicitly volatile > > memory, but commenting why we didn't have to use READ_ONCE() sounds good > > too. > > > > > To me, READ_ONCE() indicates that there's some flow in the code where > > > it's essential that the compiler doesn't generate multiple loads, but > > > that we only see a momentary single-read snapshot of the value, and this > > > doesn't seem to be the case. > > > > The compiler can also squash multiple reads together, which is more > > dangerous in this case as we would not notice a new requests. Avoiding > > READ_ONCE() requires a better knowledge of the compiler algorithms that > > prove which variable can be optimized. > > > > The difference is really minor and I agree that the comment is bad. > > The only comment I'm happy with is nothing, though ... even "READ_ONCE() > > is not necessary" is wrong as that might change without us noticing. > > FWIW, I first suggested using READ_ONCE() for the freshness argument, What is the 'freshness argument' ? > but then started to believe there were even more reasons after reading > the comment above it in include/linux/compiler.h. The last paragraph > of that comment says there are two major use cases for it. I think the > first one maps to the freshness argument. The second one is > > (2) Ensuring that the compiler does not fold, spindle, or otherwise > mutilate accesses that either do not require ordering or that > interact with an explicit memory barrier or atomic instruction that > provides the required ordering. > > Documentation/memory-barriers.txt seemed to agree with that, as it's > full of READ/WRITE_ONCE's, and statements saying they are not optional. > However, reading it closer (how many times have I tried to read it > closer?), I can't see any pattern where they're required that we need > to be concerned about. I think the future-proofing / freshness argument > still stands though, and I also like that it flags the variable as > "special". > > I think I actually prefer no comment now too, but the commit message > should get a sentence or two explaining why it got thrown in. > After reading that comment in detail again, I tend to be more open to the argument that READ_ONCE() actually makes it more clear what is going on. All I care about in this case, is that we're spending ages here trying to really understand the flow, and unless we document exactly what happens (including whether or not READ_ONCE is strictly required), someone is going to spend that same time all over again, and come to partial flawed conclusions again. So documenting the details, including which primites are absolutely required and which are not, is important IMHO, but doing so in the commit message is just fine. Thanks, -Christoffer
