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@xxxxxxxxxx> > >> > > >> > 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
