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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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.

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).

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).

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.

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)

Attachment: signature.asc
Description: This is a digitally signed message part

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