Re: [Xen-devel] [PATCH 7/7] xen/arm: Restore IRQ affinity after hotplugging a CPU
On Fri, 2018-04-13 at 12:11 +0200, Mirela Simonovic wrote: > Hi Dario, > Hi, > On Thu, Apr 12, 2018 at 6:49 PM, Dario Faggioli > wrote: > 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 > Ah, so you are talking about suspend/resume of _the_guest_! This was actually the bit of information that I was missing. :-) This was, probbly, partly due to me not being familiar with ARM, and to the fact that I haven't really looked at the rest of the series probably. Still, I think it would not hurt for the changelog to be a bit more explitic and clear. Regards, Dario -- <> (Raistlin Majere) - Dario Faggioli, Ph.D, http://about.me/dario.faggioli Software Engineer @ SUSE https://www.suse.com/ signature.asc Description: This is a digitally signed message part ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 7/7] xen/arm: Restore IRQ affinity after hotplugging a CPU
Hi Dario, On Thu, Apr 12, 2018 at 6:49 PM, Dario Faggioli 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 > >> 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 > -- > <> (Raistlin Majere) > - > Dario Faggioli, Ph.D, http://about.me/dario.faggioli > Software Engineer @ SUSE https://www.suse.com/ ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 7/7] xen/arm: Restore IRQ affinity after hotplugging a CPU
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 > 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 -- <> (Raistlin Majere) - Dario Faggioli, Ph.D, http://about.me/dario.faggioli Software Engineer @ SUSE https://www.suse.com/ signature.asc Description: This is a digitally signed message part ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 7/7] xen/arm: Restore IRQ affinity after hotplugging a CPU
Hi Stefano,
v->processor has to be updated before the sched_move_irqs is called. Since
updating v->processor is done after the 'if (v->affinity_broken)' check I
had to put sched_move_irqs below, where it is.
Thanks,
Mirela
On Thu, Apr 12, 2018 at 2:20 AM, Stefano Stabellini
wrote:
> On Wed, 11 Apr 2018, 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. Existing code
> > was missing the restoration of interrupts affinity after
> > hotplugging a CPU. This patch restores the IRQ affinity after
> > a CPU is hotplugged.
> >
> > Signed-off-by: Mirela Simonovic
> > ---
> > xen/common/schedule.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > 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
> > @@ -692,6 +692,7 @@ void restore_vcpu_affinity(struct domain *d)
> > for_each_vcpu ( d, v )
> > {
> > spinlock_t *lock;
> > +bool affinity_was_broken = v->affinity_broken;
> >
> > ASSERT(!vcpu_runnable(v));
> >
> > @@ -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);
> > }
> >
> > domain_update_node_affinity(d);
>
> I am no expert of this code, but it looks correct to me. You might want
> to move the setting of affinity_was_broken to inside the existing if (
> v->affinity_broken ) check.
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 7/7] xen/arm: Restore IRQ affinity after hotplugging a CPU
On Wed, 11 Apr 2018, 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. Existing code
> was missing the restoration of interrupts affinity after
> hotplugging a CPU. This patch restores the IRQ affinity after
> a CPU is hotplugged.
>
> Signed-off-by: Mirela Simonovic
> ---
> xen/common/schedule.c | 4
> 1 file changed, 4 insertions(+)
>
> 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
> @@ -692,6 +692,7 @@ void restore_vcpu_affinity(struct domain *d)
> for_each_vcpu ( d, v )
> {
> spinlock_t *lock;
> +bool affinity_was_broken = v->affinity_broken;
>
> ASSERT(!vcpu_runnable(v));
>
> @@ -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);
> }
>
> domain_update_node_affinity(d);
I am no expert of this code, but it looks correct to me. You might want
to move the setting of affinity_was_broken to inside the existing if (
v->affinity_broken ) check.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 7/7] xen/arm: Restore IRQ affinity after hotplugging a CPU
Hi,
Please CC relevant maintainers for the code you modify. You can use
scripts/get_maintainers.pl for that. I have CCed them for you this time.
Cheers,
On 11/04/18 14:19, 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. Existing code
was missing the restoration of interrupts affinity after
hotplugging a CPU. This patch restores the IRQ affinity after
a CPU is hotplugged.
Signed-off-by: Mirela Simonovic
---
xen/common/schedule.c | 4
1 file changed, 4 insertions(+)
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
@@ -692,6 +692,7 @@ void restore_vcpu_affinity(struct domain *d)
for_each_vcpu ( d, v )
{
spinlock_t *lock;
+bool affinity_was_broken = v->affinity_broken;
ASSERT(!vcpu_runnable(v));
@@ -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);
}
domain_update_node_affinity(d);
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 7/7] xen/arm: Restore IRQ affinity after hotplugging a CPU
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. Existing code
was missing the restoration of interrupts affinity after
hotplugging a CPU. This patch restores the IRQ affinity after
a CPU is hotplugged.
Signed-off-by: Mirela Simonovic
---
xen/common/schedule.c | 4
1 file changed, 4 insertions(+)
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
@@ -692,6 +692,7 @@ void restore_vcpu_affinity(struct domain *d)
for_each_vcpu ( d, v )
{
spinlock_t *lock;
+bool affinity_was_broken = v->affinity_broken;
ASSERT(!vcpu_runnable(v));
@@ -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);
}
domain_update_node_affinity(d);
--
2.13.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
