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.
