Re: [Xen-devel] [PATCH v10 6/7] vmx: VT-d posted-interrupt core logic handling

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, January 20, 2016 4:35 PM
> To: Wu, Feng <feng.wu@xxxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Dario Faggioli
> <dario.faggioli@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxxxxx>;
> Tian, Kevin <kevin.tian@xxxxxxxxx>; xen-devel@xxxxxxxxxxxxx; Keir Fraser
> <keir@xxxxxxx>
> Subject: RE: [PATCH v10 6/7] vmx: VT-d posted-interrupt core logic handling
> 
> >>> On 20.01.16 at 08:49, <feng.wu@xxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Monday, January 18, 2016 11:14 PM
> >> >>> On 03.12.15 at 09:35, <feng.wu@xxxxxxxxx> wrote:
> >> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >> > @@ -83,7 +83,131 @@ static int vmx_msr_write_intercept(unsigned int
> >> msr, uint64_t msr_content);
> >> > +    ASSERT(v->arch.hvm_vmx.pi_block_cpu == NR_CPUS);
> >> > +
> >> > +    /*
> >> > +     * The vCPU is blocking, we need to add it to one of the per pCPU
> lists.
> >> > +     * We save v->processor to v->arch.hvm_vmx.pi_block_cpu and use
> it
> >> for
> >> > +     * the per-CPU list, we also save it to posted-interrupt descriptor
> and
> >> > +     * make it as the destination of the wake-up notification event.
> >> > +     */
> >> > +    v->arch.hvm_vmx.pi_block_cpu = v->processor;
> >> > +
> >> > +    spin_lock_irqsave(&per_cpu(pi_blocked_vcpu_lock,
> >> > +                      v->arch.hvm_vmx.pi_block_cpu), flags);
> >> > +    list_add_tail(&v->arch.hvm_vmx.pi_blocked_vcpu_list,
> >> > +                  &per_cpu(pi_blocked_vcpu, v-
> >arch.hvm_vmx.pi_block_cpu));
> >> > +    spin_unlock_irqrestore(&per_cpu(pi_blocked_vcpu_lock,
> >> > +                           v->arch.hvm_vmx.pi_block_cpu), flags);
> >> > +
> >> > +    ASSERT(!pi_test_sn(pi_desc));
> >> > +
> >> > +    /*
> >> > +     * We don't need to set the 'NDST' field, since it should point to
> >> > +     * the same pCPU as v->processor.
> >> > +     */
> >> > +
> >> > +    write_atomic(&pi_desc->nv, pi_wakeup_vector);
> >>
> >> Stray blank line between comment and corresponding code.
> >>
> >> Also the ASSERT() is rather more disconnected from the write
> >> than seems desirable: Wouldn't it be better to cmpxchg() the
> >> whole 32-bit value, validating that SN is clear in the result?
> >
> > Not quite understand this. The control field is 64 bits, do you
> > mean cmpxchg() the whole 64-bit value like follows:
> >
> > +        do {
> > +            old.control = new.control = pi_desc->control;
> > +            new.nv = pi_wakeup_vector;
> > +        } while ( cmpxchg(&pi_desc->control, old.control, new.control)
> > +                  != old.control );
> >
> > This a somewhat like the implementation in the earlier versions,
> > why do you want to change it back?
> 
> Did you read what I've said? I'm worried about the disconnect of
> assertion and update: You really care about SN being clear
> _after_ the update aiui. 

No, the ASSERT has no connection with the update. the SN bit should
be clear before updating pi_desc->nv, adding ASSERT here is just kind
of protection code.

> And since NDST doesn't change, a 32-bit
> CMPXCHG would seem to suffice.
> 
> >> > +    pi_block_cpu = v->arch.hvm_vmx.pi_block_cpu;
> >> > +    if ( pi_block_cpu == NR_CPUS )
> >> > +        return;
> >>
> >> Are this condition and the one in the immediately preceding if()
> >> connected in any way?
> >
> > I am not sure I understand this correctly. Which one is
> > " the one in the immediately preceding if()"?
> 
> If you hadn't ripped out too much of the context, it would be a
> matter of pointing you back up. Now I have to re-quote the full
> code:
> 
> +    if ( pi_desc->nv != posted_intr_vector )
> +        write_atomic(&pi_desc->nv, posted_intr_vector);
> +
> +    /* the vCPU is not on any blocking list. */
> +    pi_block_cpu = v->arch.hvm_vmx.pi_block_cpu;
> +    if ( pi_block_cpu == NR_CPUS )
> +        return;
> 
> These are the two if()-s the question is about.
> 
> >> I.e. could the latter perhaps become an
> >> ASSERT() by connecting the two blocks suitably? I'm simply
> >> trying to limit the number of different cases to consider mentally
> >> when looking at this code ...
> >
> > If we get a true from above check, it means the vcpu was removed by
> > pi_wakeup_interrupt(), then we just need to return. If we get a false,
> > we will acquire the spinlock as the following code does, there are two
> > scenarios:
> > - pi_wakeup_interrupt() acquires the spinlock before us, then when
> > we get the lock, 'v->arch.hvm_vmx.pi_block_cpu' has already been
> > set to NR_CPUS by pi_wakeup_interrupt(), we need do nothing.
> > - We get the spinlock before pi_wakeup_interrupt(), this time we need
> > to remove the vCPU from the list and set 'v->arch.hvm_vmx.pi_block_cpu'
> > to NR_CPUS.
> 
> This is all only about the second condition, but the question really is
> whether one being true or false may imply the result of other. In
> which case this would better be ASSERT()ed than handled by two
> conditionals.

Thanks for the explanation. I don't think there are any connections
Between " if ( pi_desc->nv != posted_intr_vector )" and
" if ( pi_block_cpu == NR_CPUS )". 

> 
> >> > @@ -106,10 +230,17 @@ static int vmx_vcpu_initialise(struct vcpu *v)
> >> >
> >> >      spin_lock_init(&v->arch.hvm_vmx.vmcs_lock);
> >> >
> >> > +    INIT_LIST_HEAD(&v->arch.hvm_vmx.pi_blocked_vcpu_list);
> >> > +
> >> > +    v->arch.hvm_vmx.pi_block_cpu = NR_CPUS;
> >> > +
> >> >      v->arch.schedule_tail    = vmx_do_resume;
> >> >      v->arch.ctxt_switch_from = vmx_ctxt_switch_from;
> >> >      v->arch.ctxt_switch_to   = vmx_ctxt_switch_to;
> >> >
> >> > +    if ( iommu_intpost )
> >> > +        v->arch.vcpu_block = vmx_vcpu_block;
> >>
> >> Considering the locking question above - couldn't this be done only
> >> when a device gets added, and the pointer zapped upon removal
> >> of the last device, at once saving the conditional inside the hook?
> >
> > That is another solution, but I think it make simple thing difficult,
> > I feel the current logic is simple and clear. Btw, this hook is not
> > a critical path, another conditional in it should not a big deal.
> 
> Then you didn't understand: The question isn't this path, but the
> path where the hook gets called if non-NULL (and hence the
> possibility to avoid such needless calls).

I understand you mean the overhead happens when the hooks
is called. My point is the hook is not called in a critical path, so I doubt
whether it worth doing so to make the logic complex.

> 
> >> > --- a/xen/common/schedule.c
> >> > +++ b/xen/common/schedule.c
> >> > @@ -802,6 +802,8 @@ void vcpu_block(void)
> >> >
> >> >      set_bit(_VPF_blocked, &v->pause_flags);
> >> >
> >> > +    arch_vcpu_block(v);
> >> > +
> >> >      /* Check for events /after/ blocking: avoids wakeup waiting race. */
> >> >      if ( local_events_need_delivery() )
> >> >      {
> >>
> >> ISTR some previous discussion on this placement - wasn't it
> >> determined that putting it in the else part here would be
> >> sufficient for your needs and better in terms of overhead?
> >> Or is there some race possible if moved past the invocation
> >> of local_events_need_delivery()?
> >
> > The discussion happened several month ago, it is really hard to recall
> > every details, I have to look for it from my index. It is really bad to
> > last for so long for the review.
> 
> I understand the frustration on your end, but I can only justify
> it by the frustration at my end resulting from everyone dumping
> out their patch sets with only very few people helping to deal
> with that flood. Plus, after previous iterations, I expected the
> review to be more involved than it turned out in the end (i.e.
> you've done a good job in simplifying things after the feedback
> you had received - thanks for that).
> 
> > We need to call arch_vcpu_block() before local_events_need_delivery(),
> > since VT-d hardware can issue notification event when we are in
> > arch_vcpu_block(), it that happens, 'ON' bit is set, then we will check
> > it in local_events_need_delivery(). If we do arch_vcpu_block() in the
> > else part, we cannot handle this. This is one reason I can recall, I am
> > not sure whether there are other concerns since it has been really
> > a long time. The current implementation is fully discussed with scheduler
> > maintainers.
> 
> Okay, this all makes sense. It's just that I don't see how the ON
> bit would get checked by local_events_need_delivery(). But that
> may in turn be because, with the long turnaround, I've lost some
> of the details of the rest of this series.

Here is how ON is checked:
local_events_need_delivery() -->
        hvm_local_events_need_delivery() -->
                hvm_vcpu_has_pending_irq() -->
                        vlapic_has_pending_irq() -->
                                vlapic_find_highest_irr() -->
                                        vmx_sync_pir_to_irr() -->
                                                pi_test_and_clear_on()

> 
> >> > --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> >> > +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> >>
> >> > +    unsigned int         pi_block_cpu;
> >>
> >> Looking at all the use sites I wonder whether it wouldn't make for
> >> easier to read code if you instead stored the lock to acquire. This
> >> would then perhaps also prepare a road for balancing lists growing
> >> too large (i.e. rather than having one list per CPU, you could
> >> then have a variable number of lists, depending on current needs,
> >> and [kind of] freely select one of them).
> >
> > Since this patch is still experimental, can we put the "length of the list"
> > issue in the next stage when we turn it to default on?
> 
> I in no way meant to say that the list length issue needs to be
> dealt with right away. All I was saying was that storing a lock
> pointer here instead of a CPU number may make that easier (and
> might therefore be a good idea to do right away).

Do you mean storing a pointer to ' pi_blocked_vcpu_lock ' here?
Are you strong with this method. I will think about it a bit more to
see whether it affects current implementation a lot.

Thanks,
Feng

> 
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel