On Wed, 2018-04-11 at 15:19 +0200, Mirela Simonovic wrote:
> Secondary pCPUs will be offlined on system suspend and hotplugged
> on resume. When offlining secondary CPUs all interrupts targeted
> to those CPUs will be routed to the boot CPU. The boot CPU
> is responsible for finalizing suspend procedure. All wake-up
> interrupts are therefore targeted to the boot CPU. 
>
There is no wake-up interrupt involved in the process of unpausing
domains during resume. So, I that that what you mean is "the interrups
that the vcpus receive once they're running again. And even in that
case, rather than "All", is it "The interrupts received until the first
time the vcpu goes through vcpu_migrate()", as vcpu_migrate() does call
sched_move_irq()?

Note that I'm not (trying to) being picky, I'm just trying to
undestand. :-)

> Existing code
> was missing the restoration of interrupts affinity after
> hotplugging a CPU.
>
Either use hot-unplug and hotplug, or offline and online. I think the
latter is better in this case.

>  This patch restores the IRQ affinity after
> a CPU is hotplugged.
> 
> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>

> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 343ab6306e..e3956019bc 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -724,6 +725,9 @@ void restore_vcpu_affinity(struct domain *d)
>          lock = vcpu_schedule_lock_irq(v);
>          v->processor = SCHED_OP(vcpu_scheduler(v), pick_cpu, v);
>          spin_unlock_irq(lock);
> +
> +        if ( affinity_was_broken )
> +            sched_move_irqs(v);
>
But I guess that, more than on whether or not the affinity was broken,
you are interested in whether or not v->processor changed.

In fact, for the vcpus that had 0 in v->processor at the beginning of
this function, and also have 0 in there now, there is no need to call
sched_move_irq(), is there?

Similarly, if the affinity of a vcpu has not been broken, but pick_cpu
end up selecting a different v->processor, you do want to call
sched_move_irq(), I think.

If I'm right, I think it would be better to do, at the beginning of the
for:

 unsigned int old_cpu = v->processor;

And here:

 if (old_cpu != v->processor)
   sched_move_irqs(v);

And I'd also add a comment (above the if()), briefly saying how this is
necessary to match/undo the call work of vcpu_move_nosched() in
cpu_disable_scheduler(). 

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/