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

[Christoffer Dall <christoffer.dall@linaro.org>]

On Fri, Apr 07, 2017 at 03:15:37PM +0200, Radim Krčmář wrote:
> 2017-04-06 16:25+0200, Christoffer Dall:
> > 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:
> >> 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 think it means that you have to read it exactly once at the exact flow
> > in the code where it's placed.
> 
> The compiler can still reorder surrounding non-volatile code, but
> reading exactly once is the subset of meaning that READ_ONCE() should
> have.  Not assigning it any more meaning sounds good.
> 
> >> 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.
> >> 
> > 
> > I'm always a bit sceptical about such reasoning as I think without a
> > complete understanding of what needs to change when doing changes, we're
> > likely to get it wrong anyway.
> 
> I think we cannot achieve and maintain a complete understanding, so
> getting things wrong is just a matter of time.
> 
> It is almost impossible to break ordering of vcpu->requests, though.
> 
> >> > 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.
> > 
> > Isn't that covered by the memory barriers that imply compiler barriers
> > that we (will) have between checking the mode and the requests variable?
> 
> It is, asm volatile ("" ::: "memory") is enough.
> 
> The minimal conditions that would require explicit barrier:
>  1) not having vcpu->mode(), because it cannot work without memory
>     barriers
>  2) the instruction that disables interrupts doesn't have "memory"
>     constraint  (the smp_rmb in between is not necessary here)
> 
> And of course, there would have to be no functions that would contain a
> compiler barrier or their bodies remained unknown in between disabling
> interrupts and checking requests ...
> 
> >> 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.
> > 
> > "READ_ONCE() is not necessary" while actually using READ_ONCE() is a
> > terrible comment because it makes readers just doubt the correctness of
> > the code.
> > 
> > Regardless of whether or not we end up using READ_ONCE(), I think we
> > should document exactly what the requirements are for accessing this
> > variable at this time, i.e. any assumption about preceding barriers or
> > other flows of events that we rely on.
> 
> Makes sense.  My pitch at the documentation after dropping READ_ONCE():

I'm confused again, I thought you wanted to keep READ_ONCE().

> 
>   /*
>    *  The return value of kvm_request_pending() is implicitly volatile

why is that, actually?

>    *  and must be protected from reordering by the caller.
>    */

Can we be specific about what that means?  (e.g. must be preceded by a
full smp_mb() - or whatever the case is).

Perhaps we should just let Drew respin at this point, in case he's
confident about the right path, and then pick up from there?

Thanks,
-Christoffer