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.
*/
