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

[Christoffer Dall <cdall@linaro.org>]

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.

> 
> 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 a lockless synchronization mechanism, where

is the requests a synchronization mechanism?  I think of it more as a
cross-thread communication protocol.

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

Really, if there is no reason to use it, I don't think we should use it.
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.

Thanks,
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm