Re: [Xen-devel] [PATCH v6 14/18] vmx: posted-interrupt handling when vCPU is blocked

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.09.15 at 10:50, <feng.wu@xxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Monday, September 07, 2015 7:48 PM
>> >>> On 25.08.15 at 03:57, <feng.wu@xxxxxxxxx> wrote:
>> > +static void pi_wakeup_interrupt(struct cpu_user_regs *regs)
>> > +{
>> > +    struct arch_vmx_struct *vmx, *tmp;
>> > +    struct vcpu *v;
>> > +    spinlock_t *lock = &this_cpu(pi_blocked_vcpu_lock);
>> > +    struct list_head *blocked_vcpus = &this_cpu(pi_blocked_vcpu);
>> 
>> At least on possibly performance relevant paths please avoid
>> multiple back-to-back uses of this_cpu() (use per_cpu() instead).
> 
> Thanks for the suggestion, would you mind share more information
> about why per_cpu() is preferable in this case? Thanks!

The compiler can't fold the smp_processor_id() uses inside multiple
uses of this_cpu().

>> > +    LIST_HEAD(list);
>> > +
>> > +    spin_lock(lock);
>> > +
>> > +    /*
>> > +     * XXX: The length of the list depends on how many vCPU is current
>> > +     * blocked on this specific pCPU. This may hurt the interrupt latency
>> > +     * if the list grows to too many entries.
>> > +     */
>> > +    list_for_each_entry_safe(vmx, tmp, blocked_vcpus,
>> pi_blocked_vcpu_list)
>> > +    {
>> > +        if ( pi_test_on(&vmx->pi_desc) )
>> > +        {
>> > +            list_del_init(&vmx->pi_blocked_vcpu_list);
>> 
>> Why not just list_del() (also further down in the second loop)?
> 
> What is the bad effect for list_del_init() here?

Double the writes on a performance critical path.

>> > +
>> > +            /*
>> > +             * We cannot call vcpu_unblock here, since it also needs
>> > +             * 'pi_blocked_vcpu_lock', we store the vCPUs with ON
>> > +             * set in another list and unblock them after we release
>> > +             * 'pi_blocked_vcpu_lock'.
>> > +             */
>> 
>> At least at this point in time this doesn't appear to be correct: I
>> cannot see how, after this patch, vcpu_unblock() would end up
>> acquiring the lock.
> 
> vcpu_unblock()
>     --> vcpu_wake()
>         --> arch_vcpu_wake_prepare()
>             --> spin_lock_irqsave(&per_cpu(pi_blocked_vcpu_lock,
>                    v->arch.hvm_vmx.pi_block_cpu), flags);

No - arch_vcpu_wake_prepare() gets introduced only in patch 16.

>> And without that (plus without anything ever
>> adding to pi_blocked_vcpu), this moving between two lists seems
>> rather odd; I think the splitting of the series into patches needs
>> to be re-thought unless (preferably) you can find a (reasonably
>> clean) way without using two lists in the first place.
> 
> Do you mean if using two lists here, I need re-split this patch-set?
> I wonder why the two lists affect the patchset splitting?

Because (see above) at this point in the series it makes no sense to
use two lists, as there's no locking issue to take care of.

Jan


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