Re: [Xen-devel] [PATCH 7/7] xen/arm: Restore IRQ affinity after hotplugging a CPU

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

Hi Dario,

On Thu, Apr 12, 2018 at 6:49 PM, Dario Faggioli <dfaggioli@xxxxxxxx> wrote:
> 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.
>>

Maybe I should have made the last sentence more understandable, like
"Interrupts that could wake-up the system from suspend to RAM state
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. :-)

No worries, my commit message should be more understandable and your
answer helps me identify what's unclear. I'll try to explain below.

This is about suspend/resume implementation for ARM that is based on
PSCI and targeted for embedded systems as well. Each guest could have
its own wake-up devices/interrupts (passthrough) that could trigger
the resume. So the wake-up interrupt in this context triggers the
resume. By 'all interrupts' I meant interrupts that are left enabled
by guests upon suspend (those interrupts could wake-up the system).
Here is the PSCI-based suspend to RAM design spec:
https://lists.xenproject.org/archives/html/xen-devel/2017-12/msg01574.html

This patch is required to ensure that interrupts are migrated back to
secondary pCPUs on resume if that is needed. When a vCPU is pinned to
a secondary pCPU the vcpu migration will not happen on resume (and may
never happen), so the interrupt will remain targeted to the boot CPU.
I'll try to fix the commit message to better explain this.

>
>> 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@xxxxxxxxxx>
>
>> 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().
>

I though affinity check was sufficient but your proposal is better and
I'll fix the patch accordingly. Thanks for the feedback!

Regards,
Mirela

> 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/

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel