| Message ID | 1580954375-5030-1-git-send-email-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: apic: reuse smp_wmb() in kvm_make_request() | expand |
linmiaohe <linmiaohe@huawei.com> writes: > From: Miaohe Lin <linmiaohe@huawei.com> > > There is already an smp_mb() barrier in kvm_make_request(). We reuse it > here. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > arch/x86/kvm/lapic.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index eafc631d305c..ea871206a370 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1080,9 +1080,12 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, > result = 1; > /* assumes that there are only KVM_APIC_INIT/SIPI */ > apic->pending_events = (1UL << KVM_APIC_INIT); > - /* make sure pending_events is visible before sending > - * the request */ > - smp_wmb(); > + /* > + * Make sure pending_events is visible before sending > + * the request. > + * There is already an smp_wmb() in kvm_make_request(), > + * we reuse that barrier here. > + */ Let me suggest an alternative wording, "kvm_make_request() provides smp_wmb() so pending_events changes are guaranteed to be visible" But there is nothing wrong with yours, it's just longer than it could be :-) Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > kvm_make_request(KVM_REQ_EVENT, vcpu); > kvm_vcpu_kick(vcpu); > }
On Thu, Feb 06, 2020 at 11:47:02AM +0100, Vitaly Kuznetsov wrote: > linmiaohe <linmiaohe@huawei.com> writes: > > > From: Miaohe Lin <linmiaohe@huawei.com> > > > > There is already an smp_mb() barrier in kvm_make_request(). We reuse it > > here. > > > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > > --- > > arch/x86/kvm/lapic.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index eafc631d305c..ea871206a370 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -1080,9 +1080,12 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, > > result = 1; > > /* assumes that there are only KVM_APIC_INIT/SIPI */ > > apic->pending_events = (1UL << KVM_APIC_INIT); > > - /* make sure pending_events is visible before sending > > - * the request */ > > - smp_wmb(); > > + /* > > + * Make sure pending_events is visible before sending > > + * the request. > > + * There is already an smp_wmb() in kvm_make_request(), > > + * we reuse that barrier here. > > + */ > > Let me suggest an alternative wording, > > "kvm_make_request() provides smp_wmb() so pending_events changes are > guaranteed to be visible" > > But there is nothing wrong with yours, it's just longer than it could be > :-) I usually lean in favor of more comments, but in thise case I'd vote to drop the comment altogether. There are lots of places that rely on the smp_wmb() in kvm_make_request() without a comment, e.g. the cases for APIC_DM_STARTUP and APIC_DM_REMRD in this same switch, kvm_inject_nmi(), etc... One might wonder what makes INIT special. And on the flip side, APIC_DM_STARTUP is a good example of when a smp_wmb()/smp_rmb() is needed and commented correctly (though calling out the exactly location of the other half would be helpful). > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > > kvm_make_request(KVM_REQ_EVENT, vcpu); > > kvm_vcpu_kick(vcpu); > > } > > -- > Vitaly >
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index eafc631d305c..ea871206a370 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -1080,9 +1080,12 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, result = 1; /* assumes that there are only KVM_APIC_INIT/SIPI */ apic->pending_events = (1UL << KVM_APIC_INIT); - /* make sure pending_events is visible before sending - * the request */ - smp_wmb(); + /* + * Make sure pending_events is visible before sending + * the request. + * There is already an smp_wmb() in kvm_make_request(), + * we reuse that barrier here. + */ kvm_make_request(KVM_REQ_EVENT, vcpu); kvm_vcpu_kick(vcpu); }