| Message ID | 20211009021236.4122790-22-seanjc@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: Halt-polling and x86 APICv overhaul | expand |
On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > Move the WARN sanity checks out of the PI descriptor update loop so as > not to spam the kernel log if the condition is violated and the update > takes multiple attempts due to another writer. This also eliminates a > few extra uops from the retry path. > > Technically not checking every attempt could mean KVM will now fail to > WARN in a scenario that would have failed before, but any such failure > would be inherently racy as some other agent (CPU or device) would have > to concurrent modify the PI descriptor. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/vmx/posted_intr.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c > index 351666c41bbc..67cbe6ab8f66 100644 > --- a/arch/x86/kvm/vmx/posted_intr.c > +++ b/arch/x86/kvm/vmx/posted_intr.c > @@ -100,10 +100,11 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) > struct pi_desc old, new; > unsigned int dest; > > + WARN(pi_desc->nv != POSTED_INTR_WAKEUP_VECTOR, > + "Wakeup handler not enabled while the vCPU was blocking"); > + > do { > old.control = new.control = pi_desc->control; > - WARN(old.nv != POSTED_INTR_WAKEUP_VECTOR, > - "Wakeup handler not enabled while the VCPU is blocked\n"); > > dest = cpu_physical_id(vcpu->cpu); > > @@ -161,13 +162,12 @@ int pi_pre_block(struct kvm_vcpu *vcpu) > spin_unlock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu)); > } > > + WARN(pi_desc->sn == 1, > + "Posted Interrupt Suppress Notification set before blocking"); > + > do { > old.control = new.control = pi_desc->control; > > - WARN((pi_desc->sn == 1), > - "Warning: SN field of posted-interrupts " > - "is set before blocking\n"); > - > /* > * Since vCPU can be preempted during this process, > * vcpu->cpu could be different with pre_pcpu, we Don't know for sure if this is desired. I'll would just use WARN_ON_ONCE instead if the warning spams the log. If there is a race I would rather want to catch it even if rare. Best regards, Maxim Levitsky
On Thu, Oct 28, 2021, Maxim Levitsky wrote: > On Fri, 2021-10-08 at 19:12 -0700, Sean Christopherson wrote: > > Move the WARN sanity checks out of the PI descriptor update loop so as > > not to spam the kernel log if the condition is violated and the update > > takes multiple attempts due to another writer. This also eliminates a > > few extra uops from the retry path. > > > > Technically not checking every attempt could mean KVM will now fail to > > WARN in a scenario that would have failed before, but any such failure > > would be inherently racy as some other agent (CPU or device) would have > > to concurrent modify the PI descriptor. ... > Don't know for sure if this is desired. I'll would just use WARN_ON_ONCE instead > if the warning spams the log. > > If there is a race I would rather want to catch it even if rare. Paolo had similar concerns[*]. I copied the most relevant part of the discussion below, let me know if you object to the outcome. Thanks for the reviews! [*] https://lore.kernel.org/all/YXllGfrjPX1pVUx6@google.com/T/#u On Wed, Oct 27, 2021 at 8:38 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > On 27/10/21 17:28, Sean Christopherson wrote: > > On Wed, Oct 27, 2021, Paolo Bonzini wrote: > > > On 27/10/21 16:41, Sean Christopherson wrote: > > > > The other thing I don't like about having the WARN in the loop is that it suggests > > > > that something other than the vCPU can modify the NDST and SN fields, which is > > > > wrong and confusing (for me). > > > > > > Yeah, I can agree with that. Can you add it in a comment above the cmpxchg > > > loop, it can be as simple as > > > > > > /* The processor can set ON concurrently. */ > > > > > > when you respin patch 21 and the rest of the series? > > > > I can definitely add a comment, but I think that comment is incorrect. > > It's completely backwards indeed. I first had "the hardware" and then > shut down my brain for a second to replace it. > > > So something like this? > > > > /* ON can be set concurrently by a different vCPU or by hardware. */ > > Yes, of course.
diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c index 351666c41bbc..67cbe6ab8f66 100644 --- a/arch/x86/kvm/vmx/posted_intr.c +++ b/arch/x86/kvm/vmx/posted_intr.c @@ -100,10 +100,11 @@ static void __pi_post_block(struct kvm_vcpu *vcpu) struct pi_desc old, new; unsigned int dest; + WARN(pi_desc->nv != POSTED_INTR_WAKEUP_VECTOR, + "Wakeup handler not enabled while the vCPU was blocking"); + do { old.control = new.control = pi_desc->control; - WARN(old.nv != POSTED_INTR_WAKEUP_VECTOR, - "Wakeup handler not enabled while the VCPU is blocked\n"); dest = cpu_physical_id(vcpu->cpu); @@ -161,13 +162,12 @@ int pi_pre_block(struct kvm_vcpu *vcpu) spin_unlock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu)); } + WARN(pi_desc->sn == 1, + "Posted Interrupt Suppress Notification set before blocking"); + do { old.control = new.control = pi_desc->control; - WARN((pi_desc->sn == 1), - "Warning: SN field of posted-interrupts " - "is set before blocking\n"); - /* * Since vCPU can be preempted during this process, * vcpu->cpu could be different with pre_pcpu, we
Move the WARN sanity checks out of the PI descriptor update loop so as not to spam the kernel log if the condition is violated and the update takes multiple attempts due to another writer. This also eliminates a few extra uops from the retry path. Technically not checking every attempt could mean KVM will now fail to WARN in a scenario that would have failed before, but any such failure would be inherently racy as some other agent (CPU or device) would have to concurrent modify the PI descriptor. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/vmx/posted_intr.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)