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

["Radim Krčmář" <rkrcmar@redhat.com>]

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():

  /*
   *  The return value of kvm_request_pending() is implicitly volatile
   *  and must be protected from reordering by the caller.
   */
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm