Re: [PATCH v2 4/4] VMX: fixup PI descritpor when cpu is offline

[George Dunlap <George.Dunlap@eu.citrix.com>]

On Tue, May 31, 2016 at 11:31 AM, Wu, Feng <feng.wu@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Friday, May 27, 2016 10:57 PM
>> To: Wu, Feng <feng.wu@intel.com>
>> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
>> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
>> devel@lists.xen.org; konrad.wilk@oracle.com; keir@xen.org
>> Subject: Re: [PATCH v2 4/4] VMX: fixup PI descritpor when cpu is offline
>>
>> >>> On 26.05.16 at 15:39, <feng.wu@intel.com> wrote:
>> > @@ -102,9 +103,10 @@ void vmx_pi_per_cpu_init(unsigned int cpu)
>> >  {
>> >      INIT_LIST_HEAD(&per_cpu(vmx_pi_blocking, cpu).list);
>> >      spin_lock_init(&per_cpu(vmx_pi_blocking, cpu).lock);
>> > +    per_cpu(vmx_pi_blocking, cpu).down = 0;
>>
>> This seems pointless - per-CPU data starts out all zero (and there
>> are various places already which rely on that).
>>
>> > @@ -122,10 +124,25 @@ static void vmx_vcpu_block(struct vcpu *v)
>> >           * new vCPU to the list.
>> >           */
>> >          spin_unlock_irqrestore(&v->arch.hvm_vmx.pi_hotplug_lock, flags);
>> > -        return;
>> > +        return 1;
>> >      }
>> >
>> >      spin_lock(pi_blocking_list_lock);
>> > +    if ( unlikely(per_cpu(vmx_pi_blocking, v->processor).down) )
>>
>> Is this something that can actually happen? vmx_pi_desc_fixup()
>> runs in stop-machine context, i.e. no CPU can actively be here (or
>> anywhere near the arch_vcpu_block() call sites).
>
> This is related to scheduler, maybe Dario can give some input about
> this. Dario?

Yeah, I was going to say just looking through this -- there's no way
that vmx_cpu_dead() will be called while there are vcpus *still
running* on that pcpu.  vcpus will be migrated to other pcpus before
that happens.  All of your changes around vmx_vcpu_block() are
unnecessary.

>> > +        new_lock = &per_cpu(vmx_pi_blocking, cpu).lock;
>>
>> DYM new_cpu here? In fact with ...
>>
>> > +        spin_lock(new_lock);
>>
>> ... this I can't see how you would have successfully tested this
>> new code path, as I can't see how this would end in other than
>> a deadlock (as you hold this very lock already).

Indeed, it's not very nice to send us complicated code that obviously
can't even run.

 -George

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