Re: [Xen-devel] [PATCH v3 6/6] VMX: Fixup PI descritpor when cpu is offline

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Thursday, September 1, 2016 4:49 PM
> To: Wu, Feng <feng.wu@xxxxxxxxx>
> Cc: andrew.cooper3@xxxxxxxxxx; dario.faggioli@xxxxxxxxxx;
> george.dunlap@xxxxxxxxxxxxx; Tian, Kevin <kevin.tian@xxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxx
> Subject: Re: [PATCH v3 6/6] VMX: Fixup PI descritpor when cpu is offline
> 
> >>> On 31.08.16 at 05:56, <feng.wu@xxxxxxxxx> wrote:
> > +void vmx_pi_desc_fixup(int cpu)
> 
> unsigned int
> 
> > +{
> > +    unsigned int new_cpu, dest;
> > +    unsigned long flags;
> > +    struct arch_vmx_struct *vmx, *tmp;
> > +    spinlock_t *new_lock, *old_lock = &per_cpu(vmx_pi_blocking, cpu).lock;
> > +    struct list_head *blocked_vcpus = &per_cpu(vmx_pi_blocking, cpu).list;
> > +
> > +    if ( !iommu_intpost )
> > +        return;
> > +
> > +    spin_lock_irqsave(old_lock, flags);
> > +
> > +    list_for_each_entry_safe(vmx, tmp, blocked_vcpus, pi_blocking.list)
> > +    {
> > +        /*
> > +         * We need to find an online cpu as the NDST of the PI descriptor, 
> > it
> > +         * doesn't matter whether it is within the cpupool of the domain or
> > +         * not. As long as it is online, the vCPU will be woken up once the
> > +         * notification event arrives.
> > +         */
> > +restart:
> 
> I'd prefer if you did this without label and goto, but in any case
> labels should be indented by at least one space. Yet ...
> 
> > +        new_cpu = cpumask_any(&cpu_online_map);
> > +        new_lock = &per_cpu(vmx_pi_blocking, new_cpu).lock;
> > +
> > +        spin_lock(new_lock);
> > +
> > +        /*
> > +         * If the new_cpu is not online, that means it became offline 
> > between
> > +         * we got 'new_cpu' and acquiring its lock above, we need to find
> > +         * another online cpu instead. Such as, this fucntion is being 
> > called
> > +         * on 'new_cpu' at the same time. Can this happen??
> > +         */
> > +        if ( !cpu_online(new_cpu) )
> > +        {
> > +            spin_unlock(new_lock);
> > +            goto restart;
> > +        }
> 
> ... I think this too has been discussed before: Is this case really
> possible? You're in the context of a CPU_DEAD or CPU_UP_CANCELED
> notification, which both get issued with cpu_add_remove_lock held.
> How can a second CPU go down in parallel?

Here is the call chain:

cpu_down() ->
        stop_machine_run() ->
                get_cpu_maps()  /* Try to hold the cpu_add_remove_lock */
                ......
                put_cpu_maps()  /* Release the lock */
        notifier_call_chain(..., CPU_DEAD, ...) ->
                vmx_vcpu_dead() ->
                        vmx_pi_desc_fixup()

Seems vmx_pi_desc_fixup() is not calling with holding cpu_add_remove_lock?
Or do I miss something? Thanks for further comments in advance!

Thanks,
Feng

> 
> Jan


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