Re: [Xen-devel] [PATCH v2 0/4] VMX: Properly handle pi descriptor and per-cpu blocking list

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

> -----Original Message-----
> From: Dario Faggioli [mailto:dario.faggioli@xxxxxxxxxx]
> Sent: Friday, May 27, 2016 1:21 AM
> To: Wu, Feng <feng.wu@xxxxxxxxx>; xen-devel@xxxxxxxxxxxxx
> Cc: keir@xxxxxxx; Tian, Kevin <kevin.tian@xxxxxxxxx>; jbeulich@xxxxxxxx;
> andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx;
> konrad.wilk@xxxxxxxxxx
> Subject: Re: [PATCH v2 0/4] VMX: Properly handle pi descriptor and per-cpu
> blocking list
> 
> Hi Feng,
> 
> On Thu, 2016-05-26 at 21:39 +0800, Feng Wu wrote:
> > Feng Wu (4):
> >   VMX: Properly handle pi when all the assigned devices are removed
> >   VMX: Cleanup PI per-cpu blocking list when vcpu is destroyed
> >   VMX: Assign the right value to 'NDST' field in a concern case
> >   VMX: fixup PI descritpor when cpu is offline
> >
> I need some time to carefully look at this series, and I don't have
> such amount of time right now. I'll try to look at it and send my
> comments either tomorrow or on Monday.

Thanks for your time!

> 
> However, allow me to just say that, from a quick glance, the various
> solutions does not look really convincing to me. Basically, you've
> added:
>  - a couple of flags (pi_blocking_cleaned_up, down)
>  - a new spinlock (pi_hotplug_lock)
> 
> And yet, neither the various changelogs, nor code comments explain much
> about what the lock serializes and protects, and/or what the flags
> represent ad how they should be used.
> 
> So, if you want try argumenting a bit on what was your line of
> reasoning when doing things this way, that would be helpful (at least
> to me).

'pi_hotplug_lock' is trying to protect the following scenario:
vmx_pi_blocking_cleanup() gets called either when the last assigned
device is detached or the vCPU is going to be destroyed, and at the
same time vmx_vcpu_block() is running. We need to make sure
after all the blocking vCPU is cleaned up, we should not add new
vCPU to the per-cpu blocking list. And that is why I introduce
' pi_blocking_cleaned_up' for each vCPU, which being set to
1 means that we cannot add it to the blocking list in vmx_vcpu_block().

For the other flag 'down', it is used for the following scenario:
When a pCPU is going away and meanwhile vmx_vcpu_block() is
called, we should not put the vCPU to a per-cpu blocking list, which
is going away. Another usage of 'down' is to control that the online
cpu we choose in vmx_pi_desc_fixup() is not going away as mentioned
in the comments in this function.

> 
> I'm also non-convinced that, in patch 4, the right thing to do is to
> just to pick-up a random online pCPU. At some point, during v1 review,
> I mentioned arch_move_irqs(), because it seemed to me the place from
> where you actually have the proper information available (i.e., where
> the vcpu is actually going, e.g. because the pCPU is on is going
> away!). It may well be the case that I'm wrong about it being suitable,
> and I'll look better at what you actually have implemented, but at a
> first glance, it looks cumbersome.
> 
> For instance, now arch_vcpu_block() returns a value and, as you say
> yourself in a comment, that is for (potentially) preventing a vcpu to
> block. So the behavior of schedule.c:vcpu_block(), now depends on your
> new flag per_cpu(vmx_pi_blocking, v->processor).down. Again, I'll look
> better, but this has few chances of being right (at least logically).

Like in vcpu_block(),it will check events before actually blocking the vcpu,
here we just introduce another case in which the vCPU cannot be blocked.
I don't know why you think this is problematic?

> 
> Finally, you're calling *per-vCPU* things foo_hotplug_bar, which is
> rather confusing, as it makes one thinking that they're related to
> *pCPU* hotplug.

The purpose is, we need to remove the vCPUs in the blocking list
of the pCPU which is about to go away. Do you think how should we
do for it?

Thanks,
Feng

> 
> Thanks and Regards,
> Dario
> --
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

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