Re: [PATCH v2 1/9] KVM: add kvm_request_pending

[Andrew Jones <drjones@redhat.com>]

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@redhat.com>
> >> >> > 
> >> >> > 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,
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.

Thanks,
drew