* Re: [PATCH] KVM: apic: reuse smp_wmb() in kvm_make_request()
@ 2020-02-07 2:59 linmiaohe
0 siblings, 0 replies; 4+ messages in thread
From: linmiaohe @ 2020-02-07 2:59 UTC (permalink / raw)
To: Sean Christopherson, Vitaly Kuznetsov
Cc: kvm, linux-kernel, x86, pbonzini, rkrcmar, wanpengli, jmattson,
joro, tglx, mingo, bp, hpa
Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 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.
>> > + /*
>> > + * 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
>> :-)
Thanks for your alternative wording. It looks much better.
>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).
Yeh, I think the comment should be dropped too. :)
Thanks to both for review! I would send v2.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] KVM: apic: reuse smp_wmb() in kvm_make_request()
@ 2020-02-06 1:59 linmiaohe
2020-02-06 10:47 ` Vitaly Kuznetsov
0 siblings, 1 reply; 4+ messages in thread
From: linmiaohe @ 2020-02-06 1:59 UTC (permalink / raw)
To: pbonzini, rkrcmar, sean.j.christopherson, vkuznets, wanpengli,
jmattson, joro, tglx, mingo, bp, hpa
Cc: linmiaohe, kvm, linux-kernel, x86
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.
+ */
kvm_make_request(KVM_REQ_EVENT, vcpu);
kvm_vcpu_kick(vcpu);
}
--
2.19.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: apic: reuse smp_wmb() in kvm_make_request()
2020-02-06 1:59 linmiaohe
@ 2020-02-06 10:47 ` Vitaly Kuznetsov
2020-02-06 15:05 ` Sean Christopherson
0 siblings, 1 reply; 4+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-06 10:47 UTC (permalink / raw)
To: linmiaohe
Cc: kvm, linux-kernel, x86, pbonzini, rkrcmar, sean.j.christopherson,
wanpengli, jmattson, joro, tglx, mingo, bp, hpa
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);
> }
--
Vitaly
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: apic: reuse smp_wmb() in kvm_make_request()
2020-02-06 10:47 ` Vitaly Kuznetsov
@ 2020-02-06 15:05 ` Sean Christopherson
0 siblings, 0 replies; 4+ messages in thread
From: Sean Christopherson @ 2020-02-06 15:05 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: linmiaohe, kvm, linux-kernel, x86, pbonzini, rkrcmar, wanpengli,
jmattson, joro, tglx, mingo, bp, hpa
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-02-07 2:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-07 2:59 [PATCH] KVM: apic: reuse smp_wmb() in kvm_make_request() linmiaohe
-- strict thread matches above, loose matches on Subject: below --
2020-02-06 1:59 linmiaohe
2020-02-06 10:47 ` Vitaly Kuznetsov
2020-02-06 15:05 ` Sean Christopherson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).