Re: [PATCH v2 1/9] KVM: add kvm_request_pending
2017-04-08 15:32-0400, Paolo Bonzini: > > > 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? "that" is the return value? kvm_request_pending() is 'inline static' so the compiler can prove that the function only returns vcpu->requests and that the surrounding code doesn't change it, so various optimizations are possible. Or the implicitly volatile bit? We know that vcpu->requests can change at any time without action of the execution thread, which makes it volatile, but we don't tell that to the compiler, hence implicit. READ_ONCE(vcpu->requests) is *(volatile unsigned long *)&vcpu->requests and makes it explicitly volatile. > > >* 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). > > You can play devil's advocate both ways and argue that READ_ONCE is > better, or that it is unnecessary hence worse. You can write good > comments in either case. That's how I read Radim's message. But > I think we all agree on keeping it in the end. Exactly, I am in favor of READ_ONCE() as I don't like to keep the compiler informed, just wanted to cover all options, because they are not that different ... minute details are the hardest to decide. :) ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
> > 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). You can play devil's advocate both ways and argue that READ_ONCE is better, or that it is unnecessary hence worse. You can write good comments in either case. That's how I read Radim's message. But I think we all agree on keeping it in the end. > 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? I agree. In any case, the memory barrier is the important part, but adding READ_ONCE is self-documenting and I prefer to have it. Paolo ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
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
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
On Fri, Apr 07, 2017 at 11:33:33PM +0800, Paolo Bonzini wrote:
>
>
> On 06/04/2017 23:08, Andrew Jones wrote:
> > My own made-up lingo to state that each time the variable is accessed it
> > must be loaded anew, taken care of by the volatile use in READ_ONCE. As
> > vcpu->requests can be written by other threads, then I prefer READ_ONCE
> > being used to read it, as it allows me to avoid spending energy convincing
> > myself that the compiler would have emitted a load at that point anyway.
>
> Also, READ_ONCE without a barrier is really fishy unless it's
>
> while (READ_ONCE(x) != 2) {
> ...
> }
>
> or similar, and WRITE_ONCE doesn't even have this exception. So
> annotating variables accessed by multiple threads with
> READ_ONCE/WRITE_ONCE is generally a good idea.
>
I'm sorry, I'm confused. You're saying that it's fishy to use
READ_ONCE() without also having a barrier, but does that imply that if
you have a barrier, you should also have READ_ONCE() ?
In any case, as I hope I've made clear, I'm perfectly fine with having
READ_ONCE(), as long as we make a best effort attempt at describing why
we added it, so even I can understand that later - in the code directly
or in the commit message.
Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
On 06/04/2017 23:08, Andrew Jones wrote:
> My own made-up lingo to state that each time the variable is accessed it
> must be loaded anew, taken care of by the volatile use in READ_ONCE. As
> vcpu->requests can be written by other threads, then I prefer READ_ONCE
> being used to read it, as it allows me to avoid spending energy convincing
> myself that the compiler would have emitted a load at that point anyway.
Also, READ_ONCE without a barrier is really fishy unless it's
while (READ_ONCE(x) != 2) {
...
}
or similar, and WRITE_ONCE doesn't even have this exception. So
annotating variables accessed by multiple threads with
READ_ONCE/WRITE_ONCE is generally a good idea.
Paolo
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
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
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
On Thu, Apr 06, 2017 at 04:37:51PM +0200, Christoffer Dall wrote: > > FWIW, I first suggested using READ_ONCE() for the freshness argument, > > What is the 'freshness argument' ? My own made-up lingo to state that each time the variable is accessed it must be loaded anew, taken care of by the volatile use in READ_ONCE. As vcpu->requests can be written by other threads, then I prefer READ_ONCE being used to read it, as it allows me to avoid spending energy convincing myself that the compiler would have emitted a load at that point anyway. The writes to vcpu->requests always go through bitops, so they're already also emitting fresh loads before doing their stores. Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
Hi Drew, On Thu, Apr 06, 2017 at 02:02:12PM +0200, Andrew Jones wrote: > 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: > > >> 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ář > > >> >> > > > >> >> > 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. > > > > With "special" to stand for the idea that vcpu->requests can change > > outside of the current execution thread. Letting the programmer assume > > additional guarantees makes the generated code and resulting behavior > > more predictable. > > > > >> 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 volatile, so we need to assume that they can change > > at any moment when using them. > > > > I would prefer if vcpu->requests were of an atomic type and READ_ONCE() > > is about as close we can get without a major overhaul. > > > > >> > > >> /* > > >> * 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. > > > > Partly, synchronization is too restrictive and communication seems too > > generic, but probably still better. No idea how to shortly describe the > > part of vcpu->requests that prevents VM entry and that setting a request > > kicks VM out of guest mode. > > > > 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 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. > > > > > Really, if there is no reason to use it, I don't think we should use it. > > > > I am leaning towards READ_ONCE() as the default for implicitly volatile > > memory, but commenting why we didn't have to use READ_ONCE() sounds good > > too. > > > > > 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. > > > > 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. > > FWIW, I first suggested using
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
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: > >> 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ář > >> >> > > >> >> > 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. > > With "special" to stand for the idea that vcpu->requests can change > outside of the current execution thread. Letting the programmer assume > additional guarantees makes the generated code and resulting behavior > more predictable. > > >> 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 volatile, so we need to assume that they can change > at any moment when using them. > > I would prefer if vcpu->requests were of an atomic type and READ_ONCE() > is about as close we can get without a major overhaul. > I finally see your point of conveying how things work using READ_ONCE(). If there's really no harm in letting the compiler read this as it wishes (within the boundaries already placed by our use of compiler and memory barriers), then I think we should just document that instead of relying on how people would interpret READ_ONCE, but it's up to you - I think I'm beginning to understand regardless. > >> > >> /* > >> * 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. > > Partly, synchronization is too restrictive and communication seems too > generic, but probably still better. No idea how to shortly describe the > part of vcpu->requests that prevents VM entry and that setting a request > kicks VM out of guest mode. heh, neither do I. > > 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. > > 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. > > Really, if there is no reason to use it, I don't think we should use it. > > I am leaning towards READ_ONCE() as the default for implicitly volatile > memory, but commenting why we didn't have to use READ_ONCE() sounds good > too. > Sure, I can live with both solutions :) > > 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
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
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: > >> 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ář > >> >> > > >> >> > 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. > > With "special" to stand for the idea that vcpu->requests can change > outside of the current execution thread. Letting the programmer assume > additional guarantees makes the generated code and resulting behavior > more predictable. > > >> 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 volatile, so we need to assume that they can change > at any moment when using them. > > I would prefer if vcpu->requests were of an atomic type and READ_ONCE() > is about as close we can get without a major overhaul. > > >> > >> /* > >> * 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. > > Partly, synchronization is too restrictive and communication seems too > generic, but probably still better. No idea how to shortly describe the > part of vcpu->requests that prevents VM entry and that setting a request > kicks VM out of guest mode. > > 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 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. > > > Really, if there is no reason to use it, I don't think we should use it. > > I am leaning towards READ_ONCE() as the default for implicitly volatile > memory, but commenting why we didn't have to use READ_ONCE() sounds good > too. > > > 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. > > 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. FWIW, I first suggested using READ_ONCE() for the freshness argument, but then started to believe there were even more reasons after reading the comment above it in include/linux/compiler.h. The last paragraph of that comment says there are two major use cases for it. I think the first one maps to the freshness argument. The
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
2017-04-05 19:39+0200, Christoffer Dall: > 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ář >> >> > >> >> > 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. With "special" to stand for the idea that vcpu->requests can change outside of the current execution thread. Letting the programmer assume additional guarantees makes the generated code and resulting behavior more predictable. >> 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 volatile, so we need to assume that they can change at any moment when using them. I would prefer if vcpu->requests were of an atomic type and READ_ONCE() is about as close we can get without a major overhaul. >> >> /* >> * 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. Partly, synchronization is too restrictive and communication seems too generic, but probably still better. No idea how to shortly describe the part of vcpu->requests that prevents VM entry and that setting a request kicks VM out of guest mode. 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 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. > Really, if there is no reason to use it, I don't think we should use it. I am leaning towards READ_ONCE() as the default for implicitly volatile memory, but commenting why we didn't have to use READ_ONCE() sounds good too. > 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. 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. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
On 05/04/2017 19:39, Christoffer Dall wrote: >> 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. I don't think it's special, but predictable. It's atomic, but with relaxed ordering. Paolo ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
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ář > >> > > >> > 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
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
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ář >> > >> > 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. 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 ... /* * vcpu->requests are a lockless synchronization mechanism, where * 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. :) ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
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ář > > > > 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? Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
On Fri, Mar 31, 2017 at 06:06:50PM +0200, Andrew Jones wrote:
> From: Radim Krčmář
>
> 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?
Thanks,
-Christoffer
>
> Signed-off-by: Radim Krčmář
> Signed-off-by: Andrew Jones
> ---
> arch/mips/kvm/trap_emul.c | 2 +-
> arch/powerpc/kvm/booke.c | 2 +-
> arch/powerpc/kvm/powerpc.c | 5 ++---
> arch/s390/kvm/kvm-s390.c | 2 +-
> arch/x86/kvm/x86.c | 4 ++--
> include/linux/kvm_host.h | 5 +
> 6 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/arch/mips/kvm/trap_emul.c b/arch/mips/kvm/trap_emul.c
> index b1fa53b252ea..9ac8b1d62643 100644
> --- a/arch/mips/kvm/trap_emul.c
> +++ b/arch/mips/kvm/trap_emul.c
> @@ -1029,7 +1029,7 @@ static void kvm_trap_emul_check_requests(struct
> kvm_vcpu *vcpu, int cpu,
> struct mm_struct *mm;
> int i;
>
> - if (likely(!vcpu->requests))
> + if (likely(!kvm_request_pending(vcpu)))
> return;
>
> if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 0514cbd4e533..65ed6595c9c2 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -682,7 +682,7 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)
>
> kvmppc_core_check_exceptions(vcpu);
>
> - if (vcpu->requests) {
> + if (kvm_request_pending(vcpu)) {
> /* Exception delivery raised request; start over */
> return 1;
> }
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 95c91a9de351..714674ea5be6 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -52,8 +52,7 @@ EXPORT_SYMBOL_GPL(kvmppc_pr_ops);
>
> int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
> {
> - return !!(v->arch.pending_exceptions) ||
> -v->requests;
> + return !!(v->arch.pending_exceptions) || kvm_request_pending(v);
> }
>
> int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
> @@ -105,7 +104,7 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>*/
> smp_mb();
>
> - if (vcpu->requests) {
> + if (kvm_request_pending(vcpu)) {
> /* Make sure we process requests preemptable */
> local_irq_enable();
> trace_kvm_check_requests(vcpu);
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index fd6cd05bb6a7..40ad6c8d082f 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2396,7 +2396,7 @@ static int kvm_s390_handle_requests(struct kvm_vcpu
> *vcpu)
> {
> retry:
> kvm_s390_vcpu_request_handled(vcpu);
> - if (!vcpu->requests)
> + if (!kvm_request_pending(vcpu))
> return 0;
> /*
>* We use MMU_RELOAD just to re-arm the ipte notifier for the
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1faf620a6fdc..9714bb230524 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6726,7 +6726,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>
> bool req_immediate_exit = false;
>
> - if (vcpu->requests) {
> + if (kvm_request_pending(vcpu)) {
> if (kvm_check_request(KVM_REQ_MMU_RELOAD, vcpu))
> kvm_mmu_unload(vcpu);
> if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))
> @@ -6890,7 +6890,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> kvm_x86_ops->sync_pir_to_irr(vcpu);
> }
>
> - if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests
> + if (vcpu->mode == EXITING_GUEST_MODE || kvm_request_pending(vcpu)
> || need_resched() || signal_pending(current)) {
> vcpu->mode = OUTSIDE_GUEST_MODE;
> smp_wmb();
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 2c14ad9809da..946bf0b3c43c 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1085,6 +1085,11 @@ static inline int kvm_ioeventfd(struct kvm *kvm,
> struct kvm_ioeventfd *args)
>
> #endif /* CONFIG_HAVE_KVM_EVENTFD */
>
> +static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
> +{
> + return READ_ONCE(vcpu->requests);
> +}
> +
> static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
> {
> /*
> --
> 2.9.3
>
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
