[PATCH 0/2] xen: some scheduler related simplifications

[PATCH 0/2] xen: some scheduler related simplifications

[Juergen Gross]

While working on the scheduler I needed to modify some bits which are
worth to be considered as a stand-alone improvement IMO.

Juergen Gross (2):
  xen: introduce a cpumask with all bits set
  xen/sched: don't disable scheduler on cpus during suspend

 xen/arch/x86/io_apic.c    |   4 +-
 xen/common/cpu.c          |   4 ++
 xen/common/cpupool.c      |   2 +-
 xen/common/schedule.c     | 163 +++++++++++++---------------------------------
 xen/include/xen/cpumask.h |   2 +
 5 files changed, 53 insertions(+), 122 deletions(-)

-- 
2.16.4


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

[PATCH 1/2] xen: introduce a cpumask with all bits set

[Juergen Gross]

There are several places in Xen allocating a cpumask on the stack and
setting all bits in it just to use it as an initial mask for allowing
all cpus.

Save the stack space and omit the need for runtime initialization by
defining a globally accessible cpumask_all variable.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/io_apic.c    |  4 +---
 xen/common/cpu.c          |  4 ++++
 xen/common/schedule.c     | 17 ++++-------------
 xen/include/xen/cpumask.h |  2 ++
 4 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index daa5e9e5ff..a5344ed727 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1881,7 +1881,6 @@ static void __init check_timer(void)
     int apic1, pin1, apic2, pin2;
     int vector, ret;
     unsigned long flags;
-    cpumask_t mask_all;
 
     local_irq_save(flags);
 
@@ -1892,8 +1891,7 @@ static void __init check_timer(void)
     vector = IRQ0_VECTOR;
     clear_irq_vector(0);
 
-    cpumask_setall(&mask_all);
-    if ((ret = bind_irq_vector(0, vector, &mask_all)))
+    if ((ret = bind_irq_vector(0, vector, &cpumask_all)))
         printk(KERN_ERR"..IRQ0 is not set correctly with ioapic!!!, err:%d\n", ret);
     
     irq_desc[0].status &= ~IRQ_DISABLED;
diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index 653a56b840..836c62f97f 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -11,6 +11,10 @@ unsigned int __read_mostly nr_cpumask_bits
     = BITS_TO_LONGS(NR_CPUS) * BITS_PER_LONG;
 #endif
 
+const cpumask_t cpumask_all = {
+    .bits[0 ... (BITS_TO_LONGS(NR_CPUS) - 1)] = ~0UL
+};
+
 /*
  * cpu_bit_bitmap[] is a special, "compressed" data structure that
  * represents all NR_CPUS bits binary values of 1<<nr.
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index fd587622f4..60755a631e 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -256,9 +256,6 @@ static void sched_spin_unlock_double(spinlock_t *lock1, spinlock_t *lock2,
 int sched_init_vcpu(struct vcpu *v, unsigned int processor)
 {
     struct domain *d = v->domain;
-    cpumask_t allcpus;
-
-    cpumask_setall(&allcpus);
 
     v->processor = processor;
 
@@ -280,9 +277,9 @@ int sched_init_vcpu(struct vcpu *v, unsigned int processor)
      * domain-0 VCPUs, are pinned onto their respective physical CPUs.
      */
     if ( is_idle_domain(d) || d->is_pinned )
-        sched_set_affinity(v, cpumask_of(processor), &allcpus);
+        sched_set_affinity(v, cpumask_of(processor), &cpumask_all);
     else
-        sched_set_affinity(v, &allcpus, &allcpus);
+        sched_set_affinity(v, &cpumask_all, &cpumask_all);
 
     /* Idle VCPUs are scheduled immediately, so don't put them in runqueue. */
     if ( is_idle_domain(d) )
@@ -361,7 +358,6 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
     for_each_vcpu ( d, v )
     {
         spinlock_t *lock;
-        cpumask_t allcpus;
 
         vcpudata = v->sched_priv;
 
@@ -369,11 +365,9 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
         migrate_timer(&v->singleshot_timer, new_p);
         migrate_timer(&v->poll_timer, new_p);
 
-        cpumask_setall(&allcpus);
-
         lock = vcpu_schedule_lock_irq(v);
 
-        sched_set_affinity(v, &allcpus, &allcpus);
+        sched_set_affinity(v, &cpumask_all, &cpumask_all);
 
         v->processor = new_p;
         /*
@@ -812,8 +806,6 @@ int cpu_disable_scheduler(unsigned int cpu)
             if ( cpumask_empty(&online_affinity) &&
                  cpumask_test_cpu(cpu, v->cpu_hard_affinity) )
             {
-                cpumask_t allcpus;
-
                 if ( v->affinity_broken )
                 {
                     /* The vcpu is temporarily pinned, can't move it. */
@@ -831,8 +823,7 @@ int cpu_disable_scheduler(unsigned int cpu)
                 else
                     printk(XENLOG_DEBUG "Breaking affinity for %pv\n", v);
 
-                cpumask_setall(&allcpus);
-                sched_set_affinity(v, &allcpus, NULL);
+                sched_set_affinity(v, &cpumask_all, NULL);
             }
 
             if ( v->processor != cpu )
diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h
index b4cc92a4f5..5a43438988 100644
--- a/xen/include/xen/cpumask.h
+++ b/xen/include/xen/cpumask.h
@@ -75,6 +75,8 @@
 
 typedef struct cpumask{ DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
 
+extern const cpumask_t cpumask_all;
+
 extern unsigned int nr_cpu_ids;
 
 #if NR_CPUS > 4 * BITS_PER_LONG
-- 
2.16.4


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

Re: [PATCH 1/2] xen: introduce a cpumask with all bits set

[Andrew Cooper]

On 14/03/2019 09:59, Juergen Gross wrote:
> There are several places in Xen allocating a cpumask on the stack and
> setting all bits in it just to use it as an initial mask for allowing
> all cpus.
>
> Save the stack space and omit the need for runtime initialization by
> defining a globally accessible cpumask_all variable.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one minor
style suggestions which can be folded on commit.

> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index daa5e9e5ff..a5344ed727 100644
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -1892,8 +1891,7 @@ static void __init check_timer(void)
>      vector = IRQ0_VECTOR;
>      clear_irq_vector(0);
>  
> -    cpumask_setall(&mask_all);
> -    if ((ret = bind_irq_vector(0, vector, &mask_all)))
> +    if ((ret = bind_irq_vector(0, vector, &cpumask_all)))

Spaces, seeing as we're modifying the line.

~Andrew

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

Re: [PATCH 1/2] xen: introduce a cpumask with all bits set

[Juergen Gross]

On 14/03/2019 11:04, Andrew Cooper wrote:
> On 14/03/2019 09:59, Juergen Gross wrote:
>> There are several places in Xen allocating a cpumask on the stack and
>> setting all bits in it just to use it as an initial mask for allowing
>> all cpus.
>>
>> Save the stack space and omit the need for runtime initialization by
>> defining a globally accessible cpumask_all variable.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one minor
> style suggestions which can be folded on commit.
> 
>> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
>> index daa5e9e5ff..a5344ed727 100644
>> --- a/xen/arch/x86/io_apic.c
>> +++ b/xen/arch/x86/io_apic.c
>> @@ -1892,8 +1891,7 @@ static void __init check_timer(void)
>>      vector = IRQ0_VECTOR;
>>      clear_irq_vector(0);
>>  
>> -    cpumask_setall(&mask_all);
>> -    if ((ret = bind_irq_vector(0, vector, &mask_all)))
>> +    if ((ret = bind_irq_vector(0, vector, &cpumask_all)))
> 
> Spaces, seeing as we're modifying the line.

You are aware this file originates from the Linux kernel?

I really don't mind either way, I just wanted to mention it...


Juergen

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

Re: [PATCH 1/2] xen: introduce a cpumask with all bits set

[George Dunlap]

On 3/14/19 9:59 AM, Juergen Gross wrote:
> There are several places in Xen allocating a cpumask on the stack and
> setting all bits in it just to use it as an initial mask for allowing
> all cpus.
> 
> Save the stack space and omit the need for runtime initialization by
> defining a globally accessible cpumask_all variable.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

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

Re: [PATCH 1/2] xen: introduce a cpumask with all bits set

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 849 bytes --]

On Thu, 2019-03-14 at 10:30 +0000, George Dunlap wrote:
> On 3/14/19 9:59 AM, Juergen Gross wrote:
> > There are several places in Xen allocating a cpumask on the stack
> > and
> > setting all bits in it just to use it as an initial mask for
> > allowing
> > all cpus.
> > 
> > Save the stack space and omit the need for runtime initialization
> > by
> > defining a globally accessible cpumask_all variable.
> > 
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Acked-by: George Dunlap <george.dunlap@citrix.com>
> 
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

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/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 1/2] xen: introduce a cpumask with all bits set

[Jan Beulich]

>>> On 14.03.19 at 10:59, <jgross@suse.com> wrote:
> @@ -1892,8 +1891,7 @@ static void __init check_timer(void)
>      vector = IRQ0_VECTOR;
>      clear_irq_vector(0);
>  
> -    cpumask_setall(&mask_all);
> -    if ((ret = bind_irq_vector(0, vector, &mask_all)))
> +    if ((ret = bind_irq_vector(0, vector, &cpumask_all)))

Since you replace the users of cpumask_setall(), wouldn't it be
better to also remove that function from the header?

Jan



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

Re: [PATCH 1/2] xen: introduce a cpumask with all bits set

[Juergen Gross]

On 14/03/2019 14:12, Jan Beulich wrote:
>>>> On 14.03.19 at 10:59, <jgross@suse.com> wrote:
>> @@ -1892,8 +1891,7 @@ static void __init check_timer(void)
>>      vector = IRQ0_VECTOR;
>>      clear_irq_vector(0);
>>  
>> -    cpumask_setall(&mask_all);
>> -    if ((ret = bind_irq_vector(0, vector, &mask_all)))
>> +    if ((ret = bind_irq_vector(0, vector, &cpumask_all)))
> 
> Since you replace the users of cpumask_setall(), wouldn't it be
> better to also remove that function from the header?

I don't replace all of them. There are still some users left where
replacing with cpumask_all would be less optimal.


Juergen

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

Re: [PATCH 1/2] xen: introduce a cpumask with all bits set

[Jan Beulich]

>>> On 14.03.19 at 10:59, <jgross@suse.com> wrote:
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -1881,7 +1881,6 @@ static void __init check_timer(void)
>      int apic1, pin1, apic2, pin2;
>      int vector, ret;
>      unsigned long flags;
> -    cpumask_t mask_all;
>  
>      local_irq_save(flags);
>  
> @@ -1892,8 +1891,7 @@ static void __init check_timer(void)
>      vector = IRQ0_VECTOR;
>      clear_irq_vector(0);
>  
> -    cpumask_setall(&mask_all);
> -    if ((ret = bind_irq_vector(0, vector, &mask_all)))
> +    if ((ret = bind_irq_vector(0, vector, &cpumask_all)))
>          printk(KERN_ERR"..IRQ0 is not set correctly with ioapic!!!, err:%d\n", ret);

I actually question this use: check_timer() gets called before
any APs get onlined, so cpumask_of(0) should be equally fine
here. But the uses in the scheduler perhaps still warrant the
new object, and the code here can as well be cleaned up later.

Jan



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

[PATCH 2/2] xen/sched: don't disable scheduler on cpus during suspend

[Juergen Gross]

Today there is special handling in cpu_disable_scheduler() for suspend
by forcing all vcpus to the boot cpu. In fact there is no need for that
as during resume the vcpus are put on the correct cpus again.

So we can just omit the call of cpu_disable_scheduler() when offlining
a cpu due to suspend and on resuming we can omit taking the schedule
lock for selecting the new processor.

In restore_vcpu_affinity() we should be careful when applying affinity
as the cpu might not have come back to life. This in turn enables us
to even support affinity_broken across suspend/resume.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/cpupool.c  |   2 +-
 xen/common/schedule.c | 146 ++++++++++++++------------------------------------
 2 files changed, 42 insertions(+), 106 deletions(-)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index e89bb67e71..7b5ce18426 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -313,7 +313,7 @@ static long cpupool_unassign_cpu_helper(void *info)
      * cpu_disable_scheduler(), and at the bottom of this function.
      */
     rcu_read_lock(&domlist_read_lock);
-    ret = cpu_disable_scheduler(cpu);
+    ret = (system_state == SYS_STATE_suspend) ? 0 : cpu_disable_scheduler(cpu);
     cpumask_set_cpu(cpu, &cpupool_free_cpus);
 
     /*
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 60755a631e..1ab4f182ce 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -560,33 +560,6 @@ static void vcpu_move_locked(struct vcpu *v, unsigned int new_cpu)
         v->processor = new_cpu;
 }
 
-/*
- * Move a vcpu from its current processor to a target new processor,
- * without asking the scheduler to do any placement. This is intended
- * for being called from special contexts, where things are quiet
- * enough that no contention is supposed to happen (i.e., during
- * shutdown or software suspend, like ACPI S3).
- */
-static void vcpu_move_nosched(struct vcpu *v, unsigned int new_cpu)
-{
-    unsigned long flags;
-    spinlock_t *lock, *new_lock;
-
-    ASSERT(system_state == SYS_STATE_suspend);
-    ASSERT(!vcpu_runnable(v) && (atomic_read(&v->pause_count) ||
-                                 atomic_read(&v->domain->pause_count)));
-
-    lock = per_cpu(schedule_data, v->processor).schedule_lock;
-    new_lock = per_cpu(schedule_data, new_cpu).schedule_lock;
-
-    sched_spin_lock_double(lock, new_lock, &flags);
-    ASSERT(new_cpu != v->processor);
-    vcpu_move_locked(v, new_cpu);
-    sched_spin_unlock_double(lock, new_lock, flags);
-
-    sched_move_irqs(v);
-}
-
 /*
  * Initiating migration
  *
@@ -735,31 +708,36 @@ void restore_vcpu_affinity(struct domain *d)
 
         ASSERT(!vcpu_runnable(v));
 
-        lock = vcpu_schedule_lock_irq(v);
-
-        if ( v->affinity_broken )
-        {
-            sched_set_affinity(v, v->cpu_hard_affinity_saved, NULL);
-            v->affinity_broken = 0;
-
-        }
-
         /*
-         * During suspend (in cpu_disable_scheduler()), we moved every vCPU
-         * to BSP (which, as of now, is pCPU 0), as a temporary measure to
-         * allow the nonboot processors to have their data structure freed
-         * and go to sleep. But nothing guardantees that the BSP is a valid
-         * pCPU for a particular domain.
+         * Re-assign the initial processor as after resume we have no
+         * guarantee the old processor has come back to life again.
          *
          * Therefore, here, before actually unpausing the domains, we should
          * set v->processor of each of their vCPUs to something that will
          * make sense for the scheduler of the cpupool in which they are in.
          */
         cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_hard_affinity,
-                    cpupool_domain_cpumask(v->domain));
-        v->processor = cpumask_any(cpumask_scratch_cpu(cpu));
+                    cpupool_domain_cpumask(d));
+        if ( cpumask_empty(cpumask_scratch_cpu(cpu)) )
+        {
+            if ( v->affinity_broken )
+            {
+                sched_set_affinity(v, v->cpu_hard_affinity_saved, NULL);
+                v->affinity_broken = 0;
+                cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_hard_affinity,
+                            cpupool_domain_cpumask(d));
+            }
 
-        spin_unlock_irq(lock);
+            if ( cpumask_empty(cpumask_scratch_cpu(cpu)) )
+            {
+                printk(XENLOG_DEBUG "Breaking affinity for %pv\n", v);
+                sched_set_affinity(v, &cpumask_all, NULL);
+                cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_hard_affinity,
+                            cpupool_domain_cpumask(d));
+            }
+        }
+
+        v->processor = cpumask_any(cpumask_scratch_cpu(cpu));
 
         lock = vcpu_schedule_lock_irq(v);
         v->processor = SCHED_OP(vcpu_scheduler(v), pick_cpu, v);
@@ -782,7 +760,6 @@ int cpu_disable_scheduler(unsigned int cpu)
     struct vcpu *v;
     struct cpupool *c;
     cpumask_t online_affinity;
-    unsigned int new_cpu;
     int ret = 0;
 
     c = per_cpu(cpupool, cpu);
@@ -814,14 +791,7 @@ int cpu_disable_scheduler(unsigned int cpu)
                     break;
                 }
 
-                if (system_state == SYS_STATE_suspend)
-                {
-                    cpumask_copy(v->cpu_hard_affinity_saved,
-                                 v->cpu_hard_affinity);
-                    v->affinity_broken = 1;
-                }
-                else
-                    printk(XENLOG_DEBUG "Breaking affinity for %pv\n", v);
+                printk(XENLOG_DEBUG "Breaking affinity for %pv\n", v);
 
                 sched_set_affinity(v, &cpumask_all, NULL);
             }
@@ -833,60 +803,26 @@ int cpu_disable_scheduler(unsigned int cpu)
                 continue;
             }
 
-            /* If it is on this cpu, we must send it away. */
-            if ( unlikely(system_state == SYS_STATE_suspend) )
-            {
-                vcpu_schedule_unlock_irqrestore(lock, flags, v);
-
-                /*
-                 * If we are doing a shutdown/suspend, it is not necessary to
-                 * ask the scheduler to chime in. In fact:
-                 *  * there is no reason for it: the end result we are after
-                 *    is just 'all the vcpus on the boot pcpu, and no vcpu
-                 *    anywhere else', so let's just go for it;
-                 *  * it's wrong, for cpupools with only non-boot pcpus, as
-                 *    the scheduler would always fail to send the vcpus away
-                 *    from the last online (non boot) pcpu!
-                 *
-                 * Therefore, in the shutdown/suspend case, we just pick up
-                 * one (still) online pcpu. Note that, at this stage, all
-                 * domains (including dom0) have been paused already, so we
-                 * do not expect any vcpu activity at all.
-                 */
-                cpumask_andnot(&online_affinity, &cpu_online_map,
-                               cpumask_of(cpu));
-                BUG_ON(cpumask_empty(&online_affinity));
-                /*
-                 * As boot cpu is, usually, pcpu #0, using cpumask_first()
-                 * will make us converge quicker.
-                 */
-                new_cpu = cpumask_first(&online_affinity);
-                vcpu_move_nosched(v, new_cpu);
-            }
-            else
-            {
-                /*
-                 * OTOH, if the system is still live, and we are here because
-                 * we are doing some cpupool manipulations:
-                 *  * we want to call the scheduler, and let it re-evaluation
-                 *    the placement of the vcpu, taking into account the new
-                 *    cpupool configuration;
-                 *  * the scheduler will always fine a suitable solution, or
-                 *    things would have failed before getting in here.
-                 */
-                vcpu_migrate_start(v);
-                vcpu_schedule_unlock_irqrestore(lock, flags, v);
+            /* If it is on this cpu, we must send it away.
+             * We are doing some cpupool manipulations:
+             *  * we want to call the scheduler, and let it re-evaluation
+             *    the placement of the vcpu, taking into account the new
+             *    cpupool configuration;
+             *  * the scheduler will always find a suitable solution, or
+             *    things would have failed before getting in here.
+             */
+            vcpu_migrate_start(v);
+            vcpu_schedule_unlock_irqrestore(lock, flags, v);
 
-                vcpu_migrate_finish(v);
+            vcpu_migrate_finish(v);
 
-                /*
-                 * The only caveat, in this case, is that if a vcpu active in
-                 * the hypervisor isn't migratable. In this case, the caller
-                 * should try again after releasing and reaquiring all locks.
-                 */
-                if ( v->processor == cpu )
-                    ret = -EAGAIN;
-            }
+            /*
+             * The only caveat, in this case, is that if a vcpu active in
+             * the hypervisor isn't migratable. In this case, the caller
+             * should try again after releasing and reaquiring all locks.
+             */
+            if ( v->processor == cpu )
+                ret = -EAGAIN;
         }
     }
 
-- 
2.16.4


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

Re: [PATCH 2/2] xen/sched: don't disable scheduler on cpus during suspend

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 4751 bytes --]

On Thu, 2019-03-14 at 10:59 +0100, Juergen Gross wrote:
> Today there is special handling in cpu_disable_scheduler() for
> suspend
> by forcing all vcpus to the boot cpu. In fact there is no need for
> that
> as during resume the vcpus are put on the correct cpus again.
> 
So, basically, you're saying that moving the vcpus to BSP / CPU0,
before suspending is not necessary, right?

I thought it was, because going into suspend involves things like
cpu_schedule_down(), which does, among other things,
SCHED_OP(free_pdata) for freeing the scheduler's data structs, and
hence we did not want vcpus "around".

> So we can just omit the call of cpu_disable_scheduler() when
> offlining
> a cpu due to suspend and on resuming we can omit taking the schedule
> lock for selecting the new processor.
> 
Well, in theory, we should hold the lock when for using
'cpumask_scratch_cpu(cpu)'. Not sure this is an actual problem in this
case, but still...

> In restore_vcpu_affinity() we should be careful when applying
> affinity
> as the cpu might not have come back to life. 
>
But restore_vcpu_affinity() is done, in a loop, for all vcpus of all
domains, in thaw_domains(), which in turn comes after
enable_nonboot_cpus(), which I think is supposed to bring every CPU
back up.... What am I missing?

> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
> index e89bb67e71..7b5ce18426 100644
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -313,7 +313,7 @@ static long cpupool_unassign_cpu_helper(void
> *info)
>       * cpu_disable_scheduler(), and at the bottom of this function.
>       */
>      rcu_read_lock(&domlist_read_lock);
> -    ret = cpu_disable_scheduler(cpu);
> +    ret = (system_state == SYS_STATE_suspend) ? 0 :
> cpu_disable_scheduler(cpu);
>
Mmm... How can this function be called with state = STATE_suspend ?

> @@ -735,31 +708,36 @@ void restore_vcpu_affinity(struct domain *d)
>  
>          ASSERT(!vcpu_runnable(v));
>  
> -        lock = vcpu_schedule_lock_irq(v);
> -
> -        if ( v->affinity_broken )
> -        {
> -            sched_set_affinity(v, v->cpu_hard_affinity_saved, NULL);
> -            v->affinity_broken = 0;
> -
> -        }
> -
>          /*
> -         * During suspend (in cpu_disable_scheduler()), we moved
> every vCPU
> -         * to BSP (which, as of now, is pCPU 0), as a temporary
> measure to
> -         * allow the nonboot processors to have their data structure
> freed
> -         * and go to sleep. But nothing guardantees that the BSP is
> a valid
> -         * pCPU for a particular domain.
> +         * Re-assign the initial processor as after resume we have
> no
> +         * guarantee the old processor has come back to life again.
>           *
>           * Therefore, here, before actually unpausing the domains,
> we should
>           * set v->processor of each of their vCPUs to something that
> will
>           * make sense for the scheduler of the cpupool in which they
> are in.
>           */
>          cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_hard_affinity,
> -                    cpupool_domain_cpumask(v->domain));
> -        v->processor = cpumask_any(cpumask_scratch_cpu(cpu));
> +                    cpupool_domain_cpumask(d));
> +        if ( cpumask_empty(cpumask_scratch_cpu(cpu)) )
> +        {
> +            if ( v->affinity_broken )
> +            {
> +                sched_set_affinity(v, v->cpu_hard_affinity_saved,
> NULL);
> +                v->affinity_broken = 0;
> +                cpumask_and(cpumask_scratch_cpu(cpu), v-
> >cpu_hard_affinity,
> +                            cpupool_domain_cpumask(d));
> +            }
>  
I'm not sure I follow this; I'll think deeper, but, in the meanwhile...

> -        spin_unlock_irq(lock);
> +            if ( cpumask_empty(cpumask_scratch_cpu(cpu)) )
> +            {
> +                printk(XENLOG_DEBUG "Breaking affinity for %pv\n",
> v);
> +                sched_set_affinity(v, &cpumask_all, NULL);
> +                cpumask_and(cpumask_scratch_cpu(cpu), v-
> >cpu_hard_affinity,
> +                            cpupool_domain_cpumask(d));
>
... what does it mean we're breaking the affinity while trying to
restore it?

When/where is this vcpu getting its real affinity back in place then?

It might well be me, but I feel like there is something I'm missing
about how this patch is supposed to work.

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/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 2/2] xen/sched: don't disable scheduler on cpus during suspend

[Juergen Gross]

On 16/03/2019 04:01, Dario Faggioli wrote:
> On Thu, 2019-03-14 at 10:59 +0100, Juergen Gross wrote:
>> Today there is special handling in cpu_disable_scheduler() for
>> suspend
>> by forcing all vcpus to the boot cpu. In fact there is no need for
>> that
>> as during resume the vcpus are put on the correct cpus again.
>>
> So, basically, you're saying that moving the vcpus to BSP / CPU0,
> before suspending is not necessary, right?

Right.

> I thought it was, because going into suspend involves things like
> cpu_schedule_down(), which does, among other things,
> SCHED_OP(free_pdata) for freeing the scheduler's data structs, and
> hence we did not want vcpus "around".

Suspend is done from a tasklet with all domains "frozen". So there is no
active vcpu other than the idle ones in the system, meaning that there
is no vcpu in any runqueue.

> 
>> So we can just omit the call of cpu_disable_scheduler() when
>> offlining
>> a cpu due to suspend and on resuming we can omit taking the schedule
>> lock for selecting the new processor.
>>
> Well, in theory, we should hold the lock when for using
> 'cpumask_scratch_cpu(cpu)'. Not sure this is an actual problem in this
> case, but still...

I think there is no problem as there is no scheduling activity.

>> In restore_vcpu_affinity() we should be careful when applying
>> affinity
>> as the cpu might not have come back to life. 
>>
> But restore_vcpu_affinity() is done, in a loop, for all vcpus of all
> domains, in thaw_domains(), which in turn comes after
> enable_nonboot_cpus(), which I think is supposed to bring every CPU
> back up.... What am I missing?

There is no guarantee all cpus will be up after suspend. Some might
fail coming up.

> 
>> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
>> index e89bb67e71..7b5ce18426 100644
>> --- a/xen/common/cpupool.c
>> +++ b/xen/common/cpupool.c
>> @@ -313,7 +313,7 @@ static long cpupool_unassign_cpu_helper(void
>> *info)
>>       * cpu_disable_scheduler(), and at the bottom of this function.
>>       */
>>      rcu_read_lock(&domlist_read_lock);
>> -    ret = cpu_disable_scheduler(cpu);
>> +    ret = (system_state == SYS_STATE_suspend) ? 0 :
>> cpu_disable_scheduler(cpu);
>>
> Mmm... How can this function be called with state = STATE_suspend ?

Meh, my bad. I masked the wrong call of cpu_disable_scheduler(). Should
have been that from __cpu_disable().

> 
>> @@ -735,31 +708,36 @@ void restore_vcpu_affinity(struct domain *d)
>>  
>>          ASSERT(!vcpu_runnable(v));
>>  
>> -        lock = vcpu_schedule_lock_irq(v);
>> -
>> -        if ( v->affinity_broken )
>> -        {
>> -            sched_set_affinity(v, v->cpu_hard_affinity_saved, NULL);
>> -            v->affinity_broken = 0;
>> -
>> -        }
>> -
>>          /*
>> -         * During suspend (in cpu_disable_scheduler()), we moved
>> every vCPU
>> -         * to BSP (which, as of now, is pCPU 0), as a temporary
>> measure to
>> -         * allow the nonboot processors to have their data structure
>> freed
>> -         * and go to sleep. But nothing guardantees that the BSP is
>> a valid
>> -         * pCPU for a particular domain.
>> +         * Re-assign the initial processor as after resume we have
>> no
>> +         * guarantee the old processor has come back to life again.
>>           *
>>           * Therefore, here, before actually unpausing the domains,
>> we should
>>           * set v->processor of each of their vCPUs to something that
>> will
>>           * make sense for the scheduler of the cpupool in which they
>> are in.
>>           */
>>          cpumask_and(cpumask_scratch_cpu(cpu), v->cpu_hard_affinity,
>> -                    cpupool_domain_cpumask(v->domain));
>> -        v->processor = cpumask_any(cpumask_scratch_cpu(cpu));
>> +                    cpupool_domain_cpumask(d));
>> +        if ( cpumask_empty(cpumask_scratch_cpu(cpu)) )
>> +        {
>> +            if ( v->affinity_broken )
>> +            {
>> +                sched_set_affinity(v, v->cpu_hard_affinity_saved,
>> NULL);
>> +                v->affinity_broken = 0;
>> +                cpumask_and(cpumask_scratch_cpu(cpu), v-
>>> cpu_hard_affinity,
>> +                            cpupool_domain_cpumask(d));
>> +            }
>>  
> I'm not sure I follow this; I'll think deeper, but, in the meanwhile...
> 
>> -        spin_unlock_irq(lock);
>> +            if ( cpumask_empty(cpumask_scratch_cpu(cpu)) )
>> +            {
>> +                printk(XENLOG_DEBUG "Breaking affinity for %pv\n",
>> v);
>> +                sched_set_affinity(v, &cpumask_all, NULL);
>> +                cpumask_and(cpumask_scratch_cpu(cpu), v-
>>> cpu_hard_affinity,
>> +                            cpupool_domain_cpumask(d));
>>
> ... what does it mean we're breaking the affinity while trying to
> restore it?
> 
> When/where is this vcpu getting its real affinity back in place then?

This is only happening in case a cpu didn't come up again. There is
no way to restore the real affinity in this case.


Juergen

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

Re: [PATCH 2/2] xen/sched: don't disable scheduler on cpus during suspend

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2635 bytes --]

On Sat, 2019-03-16 at 14:14 +0100, Juergen Gross wrote:
> On 16/03/2019 04:01, Dario Faggioli wrote:

> > > So we can just omit the call of cpu_disable_scheduler() when
> > > offlining
> > > a cpu due to suspend and on resuming we can omit taking the
> > > schedule
> > > lock for selecting the new processor.
> > > 
> > Well, in theory, we should hold the lock when for using
> > 'cpumask_scratch_cpu(cpu)'. Not sure this is an actual problem in
> > this
> > case, but still...
> 
> I think there is no problem as there is no scheduling activity.
> 
I concur. I'm wondering whether it's worth adding a brief comment about
it. If it were me doing this, I most likely would.

> > > In restore_vcpu_affinity() we should be careful when applying
> > > affinity
> > > as the cpu might not have come back to life. 
> > > 
> > But restore_vcpu_affinity() is done, in a loop, for all vcpus of
> > all
> > domains, in thaw_domains(), which in turn comes after
> > enable_nonboot_cpus(), which I think is supposed to bring every CPU
> > back up.... What am I missing?
> 
> There is no guarantee all cpus will be up after suspend. Some might
> fail coming up.
> 
Ah, ok, that's the case we're trying to address. I didn't get it in the
first place.

And this, in fact, was what I was missing, and the reason why I was
finding some aspects of the patch a little weird. :-P

So, can we state this a bit better in the changelog, and also in a
comment? I'd like the reason why we break the affinity in a function
called "repair_affinity" to be more obvious. :-P

> > > diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
> > > index e89bb67e71..7b5ce18426 100644
> > > --- a/xen/common/cpupool.c
> > > +++ b/xen/common/cpupool.c
> > > @@ -313,7 +313,7 @@ static long cpupool_unassign_cpu_helper(void
> > > *info)
> > >       * cpu_disable_scheduler(), and at the bottom of this
> > > function.
> > >       */
> > >      rcu_read_lock(&domlist_read_lock);
> > > -    ret = cpu_disable_scheduler(cpu);
> > > +    ret = (system_state == SYS_STATE_suspend) ? 0 :
> > > cpu_disable_scheduler(cpu);
> > > 
> > Mmm... How can this function be called with state = STATE_suspend ?
> 
> Meh, my bad. I masked the wrong call of cpu_disable_scheduler().
> Should
> have been that from __cpu_disable().
> 
Yes, that should be the one.

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/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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