xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [Xen-devel] [PATCH v3 0/4] xen/sched: use new idle scheduler for free cpus
@ 2019-09-09  9:33 Juergen Gross
  2019-09-09  9:33 ` [Xen-devel] [PATCH v3 1/4] xen/sched: populate cpupool0 only after all cpus are up Juergen Gross
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Juergen Gross @ 2019-09-09  9:33 UTC (permalink / raw)
  To: xen-devel
  Cc: Juergen Gross, Tim Deegan, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
	Dario Faggioli, Julien Grall, Jan Beulich

These three patches have been carved out from my core scheduling series
as they are sufficiently independent to be applied even without the big
series.

Without this little series there are messages like the following to be
seen on the console when booting with smt=0:

(XEN) Adding cpu 1 to runqueue 0
(XEN) CPU 1 still not dead...
(XEN) CPU 1 still not dead...
(XEN) CPU 1 still not dead...
(XEN) CPU 1 still not dead...

By assigning cpus to Cpupool-0 only after all cpus are up and by not
using the more complicated credit2 scheduler for cpus not in any
cpupool this situation can simply no longer happen, as parking the not
to be started threads is done before.

Changes in V3:
- use ZERO_BLOCK_PTR (Patch 3)
- add patch 4

Changes in V2:
- commit messages updated
- renamed functions (Patch 2)

Juergen Gross (4):
  xen/sched: populate cpupool0 only after all cpus are up
  xen/sched: remove cpu from pool0 before removing it
  xen/sched: add minimalistic idle scheduler for free cpus
  xen/sched: switch to debugtrace in cpupool handling

 xen/common/cpupool.c       | 228 +++++++++++++++++++++++++++------------------
 xen/common/sched_credit.c  |   9 --
 xen/common/sched_null.c    |   7 --
 xen/common/schedule.c      | 172 +++++++++++++++++-----------------
 xen/include/xen/sched-if.h |   2 +
 5 files changed, 229 insertions(+), 189 deletions(-)

-- 
2.16.4


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Xen-devel] [PATCH v3 1/4] xen/sched: populate cpupool0 only after all cpus are up
  2019-09-09  9:33 [Xen-devel] [PATCH v3 0/4] xen/sched: use new idle scheduler for free cpus Juergen Gross
@ 2019-09-09  9:33 ` Juergen Gross
  2019-09-13 16:54   ` Dario Faggioli
  2019-09-09  9:33 ` [Xen-devel] [PATCH v3 2/4] xen/sched: remove cpu from pool0 before removing it Juergen Gross
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Juergen Gross @ 2019-09-09  9:33 UTC (permalink / raw)
  To: xen-devel; +Cc: Juergen Gross, Dario Faggioli

Simplify cpupool initialization by populating cpupool0 with cpus only
after all cpus are up. This avoids having to call the cpu notifier
directly for cpu 0.

With that in place there is no need to create cpupool0 earlier, so
do that just before assigning the cpus. Initialize free cpus with all
online cpus at that time in order to be able to add the cpu notifier
late, too.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V1: new patch
---
 xen/common/cpupool.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index f90e496eda..caea5bd8b3 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -762,18 +762,28 @@ static struct notifier_block cpu_nfb = {
     .notifier_call = cpu_callback
 };
 
-static int __init cpupool_presmp_init(void)
+static int __init cpupool_init(void)
 {
+    unsigned int cpu;
     int err;
-    void *cpu = (void *)(long)smp_processor_id();
+
     cpupool0 = cpupool_create(0, 0, &err);
     BUG_ON(cpupool0 == NULL);
     cpupool_put(cpupool0);
-    cpu_callback(&cpu_nfb, CPU_ONLINE, cpu);
     register_cpu_notifier(&cpu_nfb);
+
+    spin_lock(&cpupool_lock);
+
+    cpumask_copy(&cpupool_free_cpus, &cpu_online_map);
+
+    for_each_cpu ( cpu, &cpupool_free_cpus )
+        cpupool_assign_cpu_locked(cpupool0, cpu);
+
+    spin_unlock(&cpupool_lock);
+
     return 0;
 }
-presmp_initcall(cpupool_presmp_init);
+__initcall(cpupool_init);
 
 /*
  * Local variables:
-- 
2.16.4


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Xen-devel] [PATCH v3 2/4] xen/sched: remove cpu from pool0 before removing it
  2019-09-09  9:33 [Xen-devel] [PATCH v3 0/4] xen/sched: use new idle scheduler for free cpus Juergen Gross
  2019-09-09  9:33 ` [Xen-devel] [PATCH v3 1/4] xen/sched: populate cpupool0 only after all cpus are up Juergen Gross
@ 2019-09-09  9:33 ` Juergen Gross
  2019-09-13 17:27   ` Dario Faggioli
  2019-09-09  9:33 ` [Xen-devel] [PATCH v3 3/4] xen/sched: add minimalistic idle scheduler for free cpus Juergen Gross
  2019-09-09  9:33 ` [Xen-devel] [PATCH v3 4/4] xen/sched: switch to debugtrace in cpupool handling Juergen Gross
  3 siblings, 1 reply; 9+ messages in thread
From: Juergen Gross @ 2019-09-09  9:33 UTC (permalink / raw)
  To: xen-devel
  Cc: Juergen Gross, Tim Deegan, Stefano Stabellini, Wei Liu,
	Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
	Dario Faggioli, Julien Grall, Jan Beulich

Today a cpu which is removed from the system is taken directly from
Pool0 to the offline state. This will conflict with the new idle
scheduler, so remove it from Pool0 first. Additionally accept removing
a free cpu instead of requiring it to be in Pool0.

For the resume failed case we need to call the scheduler code for that
situation after the cpupool handling, so move the scheduler code into
a function and call it from cpupool_cpu_remove_forced() and remove the
CPU_RESUME_FAILED case from cpu_schedule_callback().

Note that we are calling now schedule_cpu_switch() in stop_machine
context so we need to switch from spinlock_irq to spinlock_irqsave.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2: rename cpupool_unassign_cpu_[epi|pro]logue() (Dario Faggioli)
---
 xen/common/cpupool.c       | 176 +++++++++++++++++++++++++++------------------
 xen/common/schedule.c      |  27 ++++---
 xen/include/xen/sched-if.h |   2 +
 3 files changed, 128 insertions(+), 77 deletions(-)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index caea5bd8b3..15e7004df4 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -282,22 +282,14 @@ static int cpupool_assign_cpu_locked(struct cpupool *c, unsigned int cpu)
     return 0;
 }
 
-static long cpupool_unassign_cpu_helper(void *info)
+static int cpupool_unassign_cpu_finish(struct cpupool *c)
 {
     int cpu = cpupool_moving_cpu;
-    struct cpupool *c = info;
     struct domain *d;
-    long ret;
-
-    cpupool_dprintk("cpupool_unassign_cpu(pool=%d,cpu=%d)\n",
-                    cpupool_cpu_moving->cpupool_id, cpu);
+    int ret;
 
-    spin_lock(&cpupool_lock);
     if ( c != cpupool_cpu_moving )
-    {
-        ret = -EADDRNOTAVAIL;
-        goto out;
-    }
+        return -EADDRNOTAVAIL;
 
     /*
      * We need this for scanning the domain list, both in
@@ -332,39 +324,19 @@ static long cpupool_unassign_cpu_helper(void *info)
         domain_update_node_affinity(d);
     }
     rcu_read_unlock(&domlist_read_lock);
-out:
-    spin_unlock(&cpupool_lock);
-    cpupool_dprintk("cpupool_unassign_cpu ret=%ld\n", ret);
+
     return ret;
 }
 
-/*
- * unassign a specific cpu from a cpupool
- * we must be sure not to run on the cpu to be unassigned! to achieve this
- * the main functionality is performed via continue_hypercall_on_cpu on a
- * specific cpu.
- * if the cpu to be removed is the last one of the cpupool no active domain
- * must be bound to the cpupool. dying domains are moved to cpupool0 as they
- * might be zombies.
- * possible failures:
- * - last cpu and still active domains in cpupool
- * - cpu just being unplugged
- */
-static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
+static int cpupool_unassign_cpu_start(struct cpupool *c, unsigned int cpu)
 {
-    int work_cpu;
     int ret;
     struct domain *d;
 
-    cpupool_dprintk("cpupool_unassign_cpu(pool=%d,cpu=%d)\n",
-                    c->cpupool_id, cpu);
-
     spin_lock(&cpupool_lock);
     ret = -EADDRNOTAVAIL;
     if ( (cpupool_moving_cpu != -1) && (cpu != cpupool_moving_cpu) )
         goto out;
-    if ( cpumask_test_cpu(cpu, &cpupool_locked_cpus) )
-        goto out;
 
     ret = 0;
     if ( !cpumask_test_cpu(cpu, c->cpu_valid) && (cpu != cpupool_moving_cpu) )
@@ -376,7 +348,7 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
         rcu_read_lock(&domlist_read_lock);
         for_each_domain_in_cpupool(d, c)
         {
-            if ( !d->is_dying )
+            if ( !d->is_dying && system_state == SYS_STATE_active )
             {
                 ret = -EBUSY;
                 break;
@@ -393,7 +365,57 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
     atomic_inc(&c->refcnt);
     cpupool_cpu_moving = c;
     cpumask_clear_cpu(cpu, c->cpu_valid);
+
+out:
+    spin_unlock(&cpupool_lock);
+
+    return ret;
+}
+
+static long cpupool_unassign_cpu_helper(void *info)
+{
+    struct cpupool *c = info;
+    long ret;
+
+    cpupool_dprintk("cpupool_unassign_cpu(pool=%d,cpu=%d)\n",
+                    cpupool_cpu_moving->cpupool_id, cpupool_moving_cpu);
+    spin_lock(&cpupool_lock);
+
+    ret = cpupool_unassign_cpu_finish(c);
+
     spin_unlock(&cpupool_lock);
+    cpupool_dprintk("cpupool_unassign_cpu ret=%ld\n", ret);
+
+    return ret;
+}
+
+/*
+ * unassign a specific cpu from a cpupool
+ * we must be sure not to run on the cpu to be unassigned! to achieve this
+ * the main functionality is performed via continue_hypercall_on_cpu on a
+ * specific cpu.
+ * if the cpu to be removed is the last one of the cpupool no active domain
+ * must be bound to the cpupool. dying domains are moved to cpupool0 as they
+ * might be zombies.
+ * possible failures:
+ * - last cpu and still active domains in cpupool
+ * - cpu just being unplugged
+ */
+static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
+{
+    int work_cpu;
+    int ret;
+
+    cpupool_dprintk("cpupool_unassign_cpu(pool=%d,cpu=%d)\n",
+                    c->cpupool_id, cpu);
+
+    ret = cpupool_unassign_cpu_start(c, cpu);
+    if ( ret )
+    {
+        cpupool_dprintk("cpupool_unassign_cpu(pool=%d,cpu=%d) ret %d\n",
+                        c->cpupool_id, cpu, ret);
+        return ret;
+    }
 
     work_cpu = smp_processor_id();
     if ( work_cpu == cpu )
@@ -403,12 +425,6 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
             work_cpu = cpumask_next(cpu, cpupool0->cpu_valid);
     }
     return continue_hypercall_on_cpu(work_cpu, cpupool_unassign_cpu_helper, c);
-
-out:
-    spin_unlock(&cpupool_lock);
-    cpupool_dprintk("cpupool_unassign_cpu(pool=%d,cpu=%d) ret %d\n",
-                    c->cpupool_id, cpu, ret);
-    return ret;
 }
 
 /*
@@ -492,30 +508,54 @@ static int cpupool_cpu_add(unsigned int cpu)
 }
 
 /*
- * Called to remove a CPU from a pool. The CPU is locked, to forbid removing
- * it from pool0. In fact, if we want to hot-unplug a CPU, it must belong to
- * pool0, or we fail.
+ * This function is called in stop_machine context, so we can be sure no
+ * non-idle vcpu is active on the system.
  */
-static int cpupool_cpu_remove(unsigned int cpu)
+static void cpupool_cpu_remove(unsigned int cpu)
 {
-    int ret = -ENODEV;
+    int ret;
 
-    spin_lock(&cpupool_lock);
+    ASSERT(is_idle_vcpu(current));
 
-    if ( cpumask_test_cpu(cpu, cpupool0->cpu_valid) )
+    if ( !cpumask_test_cpu(cpu, &cpupool_free_cpus) )
     {
-        /*
-         * If we are not suspending, we are hot-unplugging cpu, and that is
-         * allowed only for CPUs in pool0.
-         */
-        cpumask_clear_cpu(cpu, cpupool0->cpu_valid);
-        ret = 0;
+        ret = cpupool_unassign_cpu_finish(cpupool0);
+        BUG_ON(ret);
     }
+}
 
-    if ( !ret )
+/*
+ * Called before a CPU is being removed from the system.
+ * Removing a CPU is allowed for free CPUs or CPUs in Pool-0 (those are moved
+ * to free cpus actually before removing them).
+ * The CPU is locked, to forbid adding it again to another cpupool.
+ */
+static int cpupool_cpu_remove_prologue(unsigned int cpu)
+{
+    int ret = 0;
+
+    spin_lock(&cpupool_lock);
+
+    if ( cpumask_test_cpu(cpu, &cpupool_locked_cpus) )
+        ret = -EBUSY;
+    else
         cpumask_set_cpu(cpu, &cpupool_locked_cpus);
+
     spin_unlock(&cpupool_lock);
 
+    if ( ret )
+        return  ret;
+
+    if ( cpumask_test_cpu(cpu, cpupool0->cpu_valid) )
+    {
+        /* Cpupool0 is populated only after all cpus are up. */
+        ASSERT(system_state == SYS_STATE_active);
+
+        ret = cpupool_unassign_cpu_start(cpupool0, cpu);
+    }
+    else if ( !cpumask_test_cpu(cpu, &cpupool_free_cpus) )
+        ret = -ENODEV;
+
     return ret;
 }
 
@@ -523,13 +563,13 @@ static int cpupool_cpu_remove(unsigned int cpu)
  * Called during resume for all cpus which didn't come up again. The cpu must
  * be removed from the cpupool it is assigned to. In case a cpupool will be
  * left without cpu we move all domains of that cpupool to cpupool0.
+ * As we are called with all domains still frozen there is no need to take the
+ * cpupool lock here.
  */
 static void cpupool_cpu_remove_forced(unsigned int cpu)
 {
     struct cpupool **c;
-    struct domain *d;
-
-    spin_lock(&cpupool_lock);
+    int ret;
 
     if ( cpumask_test_cpu(cpu, &cpupool_free_cpus) )
         cpumask_clear_cpu(cpu, &cpupool_free_cpus);
@@ -539,19 +579,13 @@ static void cpupool_cpu_remove_forced(unsigned int cpu)
         {
             if ( cpumask_test_cpu(cpu, (*c)->cpu_valid) )
             {
-                cpumask_clear_cpu(cpu, (*c)->cpu_valid);
-                if ( cpumask_weight((*c)->cpu_valid) == 0 )
-                {
-                    if ( *c == cpupool0 )
-                        panic("No cpu left in cpupool0\n");
-                    for_each_domain_in_cpupool(d, *c)
-                        cpupool_move_domain_locked(d, cpupool0);
-                }
+                ret = cpupool_unassign_cpu(*c, cpu);
+                BUG_ON(ret);
             }
         }
     }
 
-    spin_unlock(&cpupool_lock);
+    sched_rm_cpu(cpu);
 }
 
 /*
@@ -619,7 +653,8 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
         if ( cpu >= nr_cpu_ids )
             goto addcpu_out;
         ret = -ENODEV;
-        if ( !cpumask_test_cpu(cpu, &cpupool_free_cpus) )
+        if ( !cpumask_test_cpu(cpu, &cpupool_free_cpus) ||
+             cpumask_test_cpu(cpu, &cpupool_locked_cpus) )
             goto addcpu_out;
         c = cpupool_find_by_id(op->cpupool_id);
         ret = -ENOENT;
@@ -746,7 +781,12 @@ static int cpu_callback(
     case CPU_DOWN_PREPARE:
         /* Suspend/Resume don't change assignments of cpus to cpupools. */
         if ( system_state <= SYS_STATE_active )
-            rc = cpupool_cpu_remove(cpu);
+            rc = cpupool_cpu_remove_prologue(cpu);
+        break;
+    case CPU_DYING:
+        /* Suspend/Resume don't change assignments of cpus to cpupools. */
+        if ( system_state <= SYS_STATE_active )
+            cpupool_cpu_remove(cpu);
         break;
     case CPU_RESUME_FAILED:
         cpupool_cpu_remove_forced(cpu);
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 7b71581756..93164c64f6 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1654,6 +1654,20 @@ static void cpu_schedule_down(unsigned int cpu)
     kill_timer(&sd->s_timer);
 }
 
+void sched_rm_cpu(unsigned int cpu)
+{
+    int rc;
+    struct schedule_data *sd = &per_cpu(schedule_data, cpu);
+    struct scheduler *sched = per_cpu(scheduler, cpu);
+
+    rcu_read_lock(&domlist_read_lock);
+    rc = cpu_disable_scheduler(cpu);
+    BUG_ON(rc);
+    rcu_read_unlock(&domlist_read_lock);
+    sched_deinit_pdata(sched, sd->sched_priv, cpu);
+    cpu_schedule_down(cpu);
+}
+
 static int cpu_schedule_callback(
     struct notifier_block *nfb, unsigned long action, void *hcpu)
 {
@@ -1709,16 +1723,10 @@ static int cpu_schedule_callback(
         rc = cpu_disable_scheduler_check(cpu);
         rcu_read_unlock(&domlist_read_lock);
         break;
-    case CPU_RESUME_FAILED:
     case CPU_DEAD:
         if ( system_state == SYS_STATE_suspend )
             break;
-        rcu_read_lock(&domlist_read_lock);
-        rc = cpu_disable_scheduler(cpu);
-        BUG_ON(rc);
-        rcu_read_unlock(&domlist_read_lock);
-        sched_deinit_pdata(sched, sd->sched_priv, cpu);
-        cpu_schedule_down(cpu);
+        sched_rm_cpu(cpu);
         break;
     case CPU_UP_CANCELED:
         if ( system_state != SYS_STATE_resume )
@@ -1841,6 +1849,7 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
     struct cpupool *old_pool = per_cpu(cpupool, cpu);
     struct schedule_data *sd = &per_cpu(schedule_data, cpu);
     spinlock_t *old_lock, *new_lock;
+    unsigned long flags;
 
     /*
      * pCPUs only move from a valid cpupool to free (i.e., out of any pool),
@@ -1895,7 +1904,7 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
      * that the lock itself changed, and retry acquiring the new one (which
      * will be the correct, remapped one, at that point).
      */
-    old_lock = pcpu_schedule_lock_irq(cpu);
+    old_lock = pcpu_schedule_lock_irqsave(cpu, &flags);
 
     vpriv_old = idle->sched_priv;
     ppriv_old = sd->sched_priv;
@@ -1913,7 +1922,7 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
     sd->schedule_lock = new_lock;
 
     /* _Not_ pcpu_schedule_unlock(): schedule_lock may have changed! */
-    spin_unlock_irq(old_lock);
+    spin_unlock_irqrestore(old_lock, flags);
 
     sched_do_tick_resume(new_ops, cpu);
 
diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
index d82ead586a..dc255b064b 100644
--- a/xen/include/xen/sched-if.h
+++ b/xen/include/xen/sched-if.h
@@ -437,4 +437,6 @@ affinity_balance_cpumask(const struct vcpu *v, int step, cpumask_t *mask)
         cpumask_copy(mask, v->cpu_hard_affinity);
 }
 
+void sched_rm_cpu(unsigned int cpu);
+
 #endif /* __XEN_SCHED_IF_H__ */
-- 
2.16.4


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Xen-devel] [PATCH v3 3/4] xen/sched: add minimalistic idle scheduler for free cpus
  2019-09-09  9:33 [Xen-devel] [PATCH v3 0/4] xen/sched: use new idle scheduler for free cpus Juergen Gross
  2019-09-09  9:33 ` [Xen-devel] [PATCH v3 1/4] xen/sched: populate cpupool0 only after all cpus are up Juergen Gross
  2019-09-09  9:33 ` [Xen-devel] [PATCH v3 2/4] xen/sched: remove cpu from pool0 before removing it Juergen Gross
@ 2019-09-09  9:33 ` Juergen Gross
  2019-09-09  9:33 ` [Xen-devel] [PATCH v3 4/4] xen/sched: switch to debugtrace in cpupool handling Juergen Gross
  3 siblings, 0 replies; 9+ messages in thread
From: Juergen Gross @ 2019-09-09  9:33 UTC (permalink / raw)
  To: xen-devel; +Cc: Juergen Gross, George Dunlap, Dario Faggioli

Instead of having a full blown scheduler running for the free cpus
add a very minimalistic scheduler for that purpose only ever scheduling
the related idle vcpu. This has the big advantage of not needing any
per-cpu, per-domain or per-scheduling unit data for free cpus and in
turn simplifying moving cpus to and from cpupools a lot.

Right now, CPUs that are not in any pool, still belong to Pool-0's
scheduler. This forces us to make, within the scheduler, extra effort
to avoid actually running vCPUs on those.

In the case of Credit1, this also cause issue to weights
(re)distribution, as the number of CPUs available to the scheduler is
wrong.

This is described in the changelog of commit e7191920261d ("xen:
credit2: never consider CPUs outside of our cpupool").

This new scheduler will just use a common lock for all free cpus.

As this new scheduler is not user selectable don't register it as an
official scheduler, but just include it in schedule.c.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Dario Faggioli <dfaggioli@suse.com>
---
V3:
- use ZERO_BLOCK_PTR instead of (void *)1 (Jan Beulich, Andrew Cooper)
---
 xen/common/sched_credit.c |   9 ---
 xen/common/sched_null.c   |   7 ---
 xen/common/schedule.c     | 153 +++++++++++++++++++++++-----------------------
 3 files changed, 75 insertions(+), 94 deletions(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 81dee5e472..70fe718127 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -617,15 +617,6 @@ csched_init_pdata(const struct scheduler *ops, void *pdata, int cpu)
 {
     unsigned long flags;
     struct csched_private *prv = CSCHED_PRIV(ops);
-    struct schedule_data *sd = &per_cpu(schedule_data, cpu);
-
-    /*
-     * This is called either during during boot, resume or hotplug, in
-     * case Credit1 is the scheduler chosen at boot. In such cases, the
-     * scheduler lock for cpu is already pointing to the default per-cpu
-     * spinlock, as Credit1 needs it, so there is no remapping to be done.
-     */
-    ASSERT(sd->schedule_lock == &sd->_lock && !spin_is_locked(&sd->_lock));
 
     spin_lock_irqsave(&prv->lock, flags);
     init_pdata(prv, pdata, cpu);
diff --git a/xen/common/sched_null.c b/xen/common/sched_null.c
index 26c6f0f129..6782ecda5c 100644
--- a/xen/common/sched_null.c
+++ b/xen/common/sched_null.c
@@ -167,17 +167,10 @@ static void init_pdata(struct null_private *prv, unsigned int cpu)
 static void null_init_pdata(const struct scheduler *ops, void *pdata, int cpu)
 {
     struct null_private *prv = null_priv(ops);
-    struct schedule_data *sd = &per_cpu(schedule_data, cpu);
 
     /* alloc_pdata is not implemented, so we want this to be NULL. */
     ASSERT(!pdata);
 
-    /*
-     * The scheduler lock points already to the default per-cpu spinlock,
-     * so there is no remapping to be done.
-     */
-    ASSERT(sd->schedule_lock == &sd->_lock && !spin_is_locked(&sd->_lock));
-
     init_pdata(prv, cpu);
 }
 
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 93164c64f6..fdeec10c3b 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -54,6 +54,10 @@ boolean_param("sched_smt_power_savings", sched_smt_power_savings);
  * */
 int sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
 integer_param("sched_ratelimit_us", sched_ratelimit_us);
+
+/* Common lock for free cpus. */
+static DEFINE_SPINLOCK(sched_free_cpu_lock);
+
 /* Various timer handlers. */
 static void s_timer_fn(void *unused);
 static void vcpu_periodic_timer_fn(void *data);
@@ -73,6 +77,58 @@ extern const struct scheduler *__start_schedulers_array[], *__end_schedulers_arr
 
 static struct scheduler __read_mostly ops;
 
+static spinlock_t *
+sched_idle_switch_sched(struct scheduler *new_ops, unsigned int cpu,
+                        void *pdata, void *vdata)
+{
+    idle_vcpu[cpu]->sched_priv = NULL;
+
+    return &sched_free_cpu_lock;
+}
+
+static int
+sched_idle_cpu_pick(const struct scheduler *ops, struct vcpu *v)
+{
+    return v->processor;
+}
+
+static void *
+sched_idle_alloc_vdata(const struct scheduler *ops, struct vcpu *v,
+                       void *dd)
+{
+    /* Any non-NULL pointer is fine here. */
+    return ZERO_BLOCK_PTR;
+}
+
+static void
+sched_idle_free_vdata(const struct scheduler *ops, void *priv)
+{
+}
+
+static struct task_slice sched_idle_schedule(
+    const struct scheduler *ops, s_time_t now,
+    bool tasklet_work_scheduled)
+{
+    const unsigned int cpu = smp_processor_id();
+    struct task_slice ret = { .time = -1 };
+
+    ret.task = idle_vcpu[cpu];
+    return ret;
+}
+
+static struct scheduler sched_idle_ops = {
+    .name           = "Idle Scheduler",
+    .opt_name       = "idle",
+    .sched_data     = NULL,
+
+    .pick_cpu       = sched_idle_cpu_pick,
+    .do_schedule    = sched_idle_schedule,
+
+    .alloc_vdata    = sched_idle_alloc_vdata,
+    .free_vdata     = sched_idle_free_vdata,
+    .switch_sched   = sched_idle_switch_sched,
+};
+
 static inline struct scheduler *dom_scheduler(const struct domain *d)
 {
     if ( likely(d->cpupool != NULL) )
@@ -1587,12 +1643,10 @@ static void poll_timer_fn(void *data)
 static int cpu_schedule_up(unsigned int cpu)
 {
     struct schedule_data *sd = &per_cpu(schedule_data, cpu);
-    void *sched_priv;
 
-    per_cpu(scheduler, cpu) = &ops;
+    per_cpu(scheduler, cpu) = &sched_idle_ops;
     spin_lock_init(&sd->_lock);
-    sd->schedule_lock = &sd->_lock;
-    sd->curr = idle_vcpu[cpu];
+    sd->schedule_lock = &sched_free_cpu_lock;
     init_timer(&sd->s_timer, s_timer_fn, NULL, cpu);
     atomic_set(&sd->urgent_count, 0);
 
@@ -1602,40 +1656,19 @@ static int cpu_schedule_up(unsigned int cpu)
 
     if ( idle_vcpu[cpu] == NULL )
         vcpu_create(idle_vcpu[0]->domain, cpu, cpu);
-    else
-    {
-        struct vcpu *idle = idle_vcpu[cpu];
-
-        /*
-         * During (ACPI?) suspend the idle vCPU for this pCPU is not freed,
-         * while its scheduler specific data (what is pointed by sched_priv)
-         * is. Also, at this stage of the resume path, we attach the pCPU
-         * to the default scheduler, no matter in what cpupool it was before
-         * suspend. To avoid inconsistency, let's allocate default scheduler
-         * data for the idle vCPU here. If the pCPU was in a different pool
-         * with a different scheduler, it is schedule_cpu_switch(), invoked
-         * later, that will set things up as appropriate.
-         */
-        ASSERT(idle->sched_priv == NULL);
 
-        idle->sched_priv = sched_alloc_vdata(&ops, idle,
-                                             idle->domain->sched_priv);
-        if ( idle->sched_priv == NULL )
-            return -ENOMEM;
-    }
     if ( idle_vcpu[cpu] == NULL )
         return -ENOMEM;
 
     /*
-     * We don't want to risk calling xfree() on an sd->sched_priv
-     * (e.g., inside free_pdata, from cpu_schedule_down() called
-     * during CPU_UP_CANCELLED) that contains an IS_ERR value.
+     * No need to allocate any scheduler data, as cpus coming online are
+     * free initially and the idle scheduler doesn't need any data areas
+     * allocated.
      */
-    sched_priv = sched_alloc_pdata(&ops, cpu);
-    if ( IS_ERR(sched_priv) )
-        return PTR_ERR(sched_priv);
 
-    sd->sched_priv = sched_priv;
+    sd->curr = idle_vcpu[cpu];
+
+    sd->sched_priv = NULL;
 
     return 0;
 }
@@ -1643,13 +1676,6 @@ static int cpu_schedule_up(unsigned int cpu)
 static void cpu_schedule_down(unsigned int cpu)
 {
     struct schedule_data *sd = &per_cpu(schedule_data, cpu);
-    struct scheduler *sched = per_cpu(scheduler, cpu);
-
-    sched_free_pdata(sched, sd->sched_priv, cpu);
-    sched_free_vdata(sched, idle_vcpu[cpu]->sched_priv);
-
-    idle_vcpu[cpu]->sched_priv = NULL;
-    sd->sched_priv = NULL;
 
     kill_timer(&sd->s_timer);
 }
@@ -1657,14 +1683,11 @@ static void cpu_schedule_down(unsigned int cpu)
 void sched_rm_cpu(unsigned int cpu)
 {
     int rc;
-    struct schedule_data *sd = &per_cpu(schedule_data, cpu);
-    struct scheduler *sched = per_cpu(scheduler, cpu);
 
     rcu_read_lock(&domlist_read_lock);
     rc = cpu_disable_scheduler(cpu);
     BUG_ON(rc);
     rcu_read_unlock(&domlist_read_lock);
-    sched_deinit_pdata(sched, sd->sched_priv, cpu);
     cpu_schedule_down(cpu);
 }
 
@@ -1672,8 +1695,6 @@ static int cpu_schedule_callback(
     struct notifier_block *nfb, unsigned long action, void *hcpu)
 {
     unsigned int cpu = (unsigned long)hcpu;
-    struct scheduler *sched = per_cpu(scheduler, cpu);
-    struct schedule_data *sd = &per_cpu(schedule_data, cpu);
     int rc = 0;
 
     /*
@@ -1681,39 +1702,25 @@ static int cpu_schedule_callback(
      * allocating and initializing the per-pCPU scheduler specific data,
      * as well as "registering" this pCPU to the scheduler (which may
      * involve modifying some scheduler wide data structures).
-     * This happens by calling the alloc_pdata and init_pdata hooks, in
-     * this order. A scheduler that does not need to allocate any per-pCPU
-     * data can avoid implementing alloc_pdata. init_pdata may, however, be
-     * necessary/useful in this case too (e.g., it can contain the "register
-     * the pCPU to the scheduler" part). alloc_pdata (if present) is called
-     * during CPU_UP_PREPARE. init_pdata (if present) is called during
-     * CPU_STARTING.
+     * As new pCPUs always start as "free" cpus with the minimal idle
+     * scheduler being in charge, we don't need any of that.
      *
      * On the other hand, at teardown, we need to reverse what has been done
-     * during initialization, and then free the per-pCPU specific data. This
-     * happens by calling the deinit_pdata and free_pdata hooks, in this
+     * during initialization, and then free the per-pCPU specific data. A
+     * pCPU brought down is not forced through "free" cpus, so here we need to
+     * use the appropriate hooks.
+     *
+     * This happens by calling the deinit_pdata and free_pdata hooks, in this
      * order. If no per-pCPU memory was allocated, there is no need to
      * provide an implementation of free_pdata. deinit_pdata may, however,
      * be necessary/useful in this case too (e.g., it can undo something done
      * on scheduler wide data structure during init_pdata). Both deinit_pdata
      * and free_pdata are called during CPU_DEAD.
      *
-     * If someting goes wrong during bringup, we go to CPU_UP_CANCELLED
-     * *before* having called init_pdata. In this case, as there is no
-     * initialization needing undoing, only free_pdata should be called.
-     * This means it is possible to call free_pdata just after alloc_pdata,
-     * without a init_pdata/deinit_pdata "cycle" in between the two.
-     *
-     * So, in summary, the usage pattern should look either
-     *  - alloc_pdata-->init_pdata-->deinit_pdata-->free_pdata, or
-     *  - alloc_pdata-->free_pdata.
+     * If someting goes wrong during bringup, we go to CPU_UP_CANCELLED.
      */
     switch ( action )
     {
-    case CPU_STARTING:
-        if ( system_state != SYS_STATE_resume )
-            sched_init_pdata(sched, sd->sched_priv, cpu);
-        break;
     case CPU_UP_PREPARE:
         if ( system_state != SYS_STATE_resume )
             rc = cpu_schedule_up(cpu);
@@ -1824,9 +1831,7 @@ void __init scheduler_init(void)
     idle_domain->max_vcpus = nr_cpu_ids;
     if ( vcpu_create(idle_domain, 0, 0) == NULL )
         BUG();
-    this_cpu(schedule_data).sched_priv = sched_alloc_pdata(&ops, 0);
-    BUG_ON(IS_ERR(this_cpu(schedule_data).sched_priv));
-    sched_init_pdata(&ops, this_cpu(schedule_data).sched_priv, 0);
+    this_cpu(schedule_data).curr = idle_vcpu[0];
 }
 
 /*
@@ -1834,18 +1839,14 @@ void __init scheduler_init(void)
  * cpupool, or subject it to the scheduler of a new cpupool.
  *
  * For the pCPUs that are removed from their cpupool, their scheduler becomes
- * &ops (the default scheduler, selected at boot, which also services the
- * default cpupool). However, as these pCPUs are not really part of any pool,
- * there won't be any scheduling event on them, not even from the default
- * scheduler. Basically, they will just sit idle until they are explicitly
- * added back to a cpupool.
+ * &sched_idle_ops (the idle scheduler).
  */
 int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
 {
     struct vcpu *idle;
     void *ppriv, *ppriv_old, *vpriv, *vpriv_old;
     struct scheduler *old_ops = per_cpu(scheduler, cpu);
-    struct scheduler *new_ops = (c == NULL) ? &ops : c->sched;
+    struct scheduler *new_ops = (c == NULL) ? &sched_idle_ops : c->sched;
     struct cpupool *old_pool = per_cpu(cpupool, cpu);
     struct schedule_data *sd = &per_cpu(schedule_data, cpu);
     spinlock_t *old_lock, *new_lock;
@@ -1865,9 +1866,6 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
     ASSERT((c == NULL && !cpumask_test_cpu(cpu, old_pool->cpu_valid)) ||
            (c != NULL && !cpumask_test_cpu(cpu, c->cpu_valid)));
 
-    if ( old_ops == new_ops )
-        goto out;
-
     /*
      * To setup the cpu for the new scheduler we need:
      *  - a valid instance of per-CPU scheduler specific data, as it is
@@ -1931,7 +1929,6 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c)
     sched_free_vdata(old_ops, vpriv_old);
     sched_free_pdata(old_ops, ppriv_old, cpu);
 
- out:
     per_cpu(cpupool, cpu) = c;
     /* When a cpu is added to a pool, trigger it to go pick up some work */
     if ( c != NULL )
-- 
2.16.4


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Xen-devel] [PATCH v3 4/4] xen/sched: switch to debugtrace in cpupool handling
  2019-09-09  9:33 [Xen-devel] [PATCH v3 0/4] xen/sched: use new idle scheduler for free cpus Juergen Gross
                   ` (2 preceding siblings ...)
  2019-09-09  9:33 ` [Xen-devel] [PATCH v3 3/4] xen/sched: add minimalistic idle scheduler for free cpus Juergen Gross
@ 2019-09-09  9:33 ` Juergen Gross
  2019-09-12  8:24   ` Dario Faggioli
  3 siblings, 1 reply; 9+ messages in thread
From: Juergen Gross @ 2019-09-09  9:33 UTC (permalink / raw)
  To: xen-devel; +Cc: Juergen Gross, Dario Faggioli

Instead of having a cpupool_dprintk() define just use debugtrace.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/cpupool.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 15e7004df4..179521e2dd 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -36,8 +36,6 @@ static DEFINE_SPINLOCK(cpupool_lock);
 
 DEFINE_PER_CPU(struct cpupool *, cpupool);
 
-#define cpupool_dprintk(x...) ((void)0)
-
 static struct cpupool *alloc_cpupool_struct(void)
 {
     struct cpupool *c = xzalloc(struct cpupool);
@@ -133,7 +131,7 @@ static struct cpupool *cpupool_create(
     /* One reference for caller, one reference for cpupool_destroy(). */
     atomic_set(&c->refcnt, 2);
 
-    cpupool_dprintk("cpupool_create(pool=%d,sched=%u)\n", poolid, sched_id);
+    debugtrace_printk("cpupool_create(pool=%d,sched=%u)\n", poolid, sched_id);
 
     spin_lock(&cpupool_lock);
 
@@ -175,8 +173,8 @@ static struct cpupool *cpupool_create(
 
     spin_unlock(&cpupool_lock);
 
-    cpupool_dprintk("Created cpupool %d with scheduler %s (%s)\n",
-                    c->cpupool_id, c->sched->name, c->sched->opt_name);
+    debugtrace_printk("Created cpupool %d with scheduler %s (%s)\n",
+                      c->cpupool_id, c->sched->name, c->sched->opt_name);
 
     *perr = 0;
     return c;
@@ -212,7 +210,7 @@ static int cpupool_destroy(struct cpupool *c)
 
     cpupool_put(c);
 
-    cpupool_dprintk("cpupool_destroy(pool=%d)\n", c->cpupool_id);
+    debugtrace_printk("cpupool_destroy(pool=%d)\n", c->cpupool_id);
     return 0;
 }
 
@@ -377,14 +375,14 @@ static long cpupool_unassign_cpu_helper(void *info)
     struct cpupool *c = info;
     long ret;
 
-    cpupool_dprintk("cpupool_unassign_cpu(pool=%d,cpu=%d)\n",
-                    cpupool_cpu_moving->cpupool_id, cpupool_moving_cpu);
+    debugtrace_printk("cpupool_unassign_cpu(pool=%d,cpu=%d)\n",
+                      cpupool_cpu_moving->cpupool_id, cpupool_moving_cpu);
     spin_lock(&cpupool_lock);
 
     ret = cpupool_unassign_cpu_finish(c);
 
     spin_unlock(&cpupool_lock);
-    cpupool_dprintk("cpupool_unassign_cpu ret=%ld\n", ret);
+    debugtrace_printk("cpupool_unassign_cpu ret=%ld\n", ret);
 
     return ret;
 }
@@ -406,14 +404,14 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
     int work_cpu;
     int ret;
 
-    cpupool_dprintk("cpupool_unassign_cpu(pool=%d,cpu=%d)\n",
-                    c->cpupool_id, cpu);
+    debugtrace_printk("cpupool_unassign_cpu(pool=%d,cpu=%d)\n",
+                      c->cpupool_id, cpu);
 
     ret = cpupool_unassign_cpu_start(c, cpu);
     if ( ret )
     {
-        cpupool_dprintk("cpupool_unassign_cpu(pool=%d,cpu=%d) ret %d\n",
-                        c->cpupool_id, cpu, ret);
+        debugtrace_printk("cpupool_unassign_cpu(pool=%d,cpu=%d) ret %d\n",
+                          c->cpupool_id, cpu, ret);
         return ret;
     }
 
@@ -455,8 +453,8 @@ int cpupool_add_domain(struct domain *d, int poolid)
         rc = 0;
     }
     spin_unlock(&cpupool_lock);
-    cpupool_dprintk("cpupool_add_domain(dom=%d,pool=%d) n_dom %d rc %d\n",
-                    d->domain_id, poolid, n_dom, rc);
+    debugtrace_printk("cpupool_add_domain(dom=%d,pool=%d) n_dom %d rc %d\n",
+                      d->domain_id, poolid, n_dom, rc);
     return rc;
 }
 
@@ -476,8 +474,8 @@ void cpupool_rm_domain(struct domain *d)
     n_dom = d->cpupool->n_dom;
     d->cpupool = NULL;
     spin_unlock(&cpupool_lock);
-    cpupool_dprintk("cpupool_rm_domain(dom=%d,pool=%d) n_dom %d\n",
-                    d->domain_id, cpupool_id, n_dom);
+    debugtrace_printk("cpupool_rm_domain(dom=%d,pool=%d) n_dom %d\n",
+                      d->domain_id, cpupool_id, n_dom);
     return;
 }
 
@@ -644,8 +642,8 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
         unsigned cpu;
 
         cpu = op->cpu;
-        cpupool_dprintk("cpupool_assign_cpu(pool=%d,cpu=%d)\n",
-                        op->cpupool_id, cpu);
+        debugtrace_printk("cpupool_assign_cpu(pool=%d,cpu=%d)\n",
+                          op->cpupool_id, cpu);
         spin_lock(&cpupool_lock);
         if ( cpu == XEN_SYSCTL_CPUPOOL_PAR_ANY )
             cpu = cpumask_first(&cpupool_free_cpus);
@@ -663,8 +661,8 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
         ret = cpupool_assign_cpu_locked(c, cpu);
     addcpu_out:
         spin_unlock(&cpupool_lock);
-        cpupool_dprintk("cpupool_assign_cpu(pool=%d,cpu=%d) ret %d\n",
-                        op->cpupool_id, cpu, ret);
+        debugtrace_printk("cpupool_assign_cpu(pool=%d,cpu=%d) ret %d\n",
+                          op->cpupool_id, cpu, ret);
     }
     break;
 
@@ -703,8 +701,8 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
             rcu_unlock_domain(d);
             break;
         }
-        cpupool_dprintk("cpupool move_domain(dom=%d)->pool=%d\n",
-                        d->domain_id, op->cpupool_id);
+        debugtrace_printk("cpupool move_domain(dom=%d)->pool=%d\n",
+                          d->domain_id, op->cpupool_id);
         ret = -ENOENT;
         spin_lock(&cpupool_lock);
 
@@ -713,8 +711,8 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
             ret = cpupool_move_domain_locked(d, c);
 
         spin_unlock(&cpupool_lock);
-        cpupool_dprintk("cpupool move_domain(dom=%d)->pool=%d ret %d\n",
-                        d->domain_id, op->cpupool_id, ret);
+        debugtrace_printk("cpupool move_domain(dom=%d)->pool=%d ret %d\n",
+                          d->domain_id, op->cpupool_id, ret);
         rcu_unlock_domain(d);
     }
     break;
-- 
2.16.4


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH v3 4/4] xen/sched: switch to debugtrace in cpupool handling
  2019-09-09  9:33 ` [Xen-devel] [PATCH v3 4/4] xen/sched: switch to debugtrace in cpupool handling Juergen Gross
@ 2019-09-12  8:24   ` Dario Faggioli
  0 siblings, 0 replies; 9+ messages in thread
From: Dario Faggioli @ 2019-09-12  8:24 UTC (permalink / raw)
  To: Juergen Gross, xen-devel


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

On Mon, 2019-09-09 at 11:33 +0200, Juergen Gross wrote:
> Instead of having a cpupool_dprintk() define just use debugtrace.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Acked-by: Dario Faggioli <dfaggioli@suse.com>

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


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH v3 1/4] xen/sched: populate cpupool0 only after all cpus are up
  2019-09-09  9:33 ` [Xen-devel] [PATCH v3 1/4] xen/sched: populate cpupool0 only after all cpus are up Juergen Gross
@ 2019-09-13 16:54   ` Dario Faggioli
  0 siblings, 0 replies; 9+ messages in thread
From: Dario Faggioli @ 2019-09-13 16:54 UTC (permalink / raw)
  To: Juergen Gross, xen-devel


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

On Mon, 2019-09-09 at 11:33 +0200, Juergen Gross wrote:
> Simplify cpupool initialization by populating cpupool0 with cpus only
> after all cpus are up. This avoids having to call the cpu notifier
> directly for cpu 0.
> 
> With that in place there is no need to create cpupool0 earlier, so
> do that just before assigning the cpus. Initialize free cpus with all
> online cpus at that time in order to be able to add the cpu notifier
> late, too.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

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


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH v3 2/4] xen/sched: remove cpu from pool0 before removing it
  2019-09-09  9:33 ` [Xen-devel] [PATCH v3 2/4] xen/sched: remove cpu from pool0 before removing it Juergen Gross
@ 2019-09-13 17:27   ` Dario Faggioli
  2019-09-14  5:04     ` Juergen Gross
  0 siblings, 1 reply; 9+ messages in thread
From: Dario Faggioli @ 2019-09-13 17:27 UTC (permalink / raw)
  To: Juergen Gross, xen-devel
  Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
	George Dunlap, Andrew Cooper, Tim Deegan, Julien Grall,
	Jan Beulich, Ian Jackson


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

On Mon, 2019-09-09 at 11:33 +0200, Juergen Gross wrote:
> Today a cpu which is removed from the system is taken directly from
> Pool0 to the offline state. This will conflict with the new idle
> scheduler, so remove it from Pool0 first. Additionally accept
> removing
> a free cpu instead of requiring it to be in Pool0.
> 
> For the resume failed case we need to call the scheduler code for
> that
> situation after the cpupool handling, so move the scheduler code into
> a function and call it from cpupool_cpu_remove_forced() and remove
> the
> CPU_RESUME_FAILED case from cpu_schedule_callback().
> 
> Note that we are calling now schedule_cpu_switch() in stop_machine
> context so we need to switch from spinlock_irq to spinlock_irqsave.
> 
So, I was looking at this patch, and while doing that, also trying it
out.

I've done the following:

# echo 0 > /sys/devices/system/xen_cpu/xen_cpu7/online

And CPU 7 went offline, and was listed among the free CPUs:

(XEN) Online Cpus: 0-6
(XEN) Free Cpus: 7
(XEN) Cpupool 0:
(XEN) Cpus: 0-6
(XEN) Scheduler: SMP Credit Scheduler rev2 (credit2)
(XEN) Active queues: 1
(XEN) 	default-weight     = 256
(XEN) Runqueue 0:
(XEN) 	ncpus              = 7
(XEN) 	cpus               = 0-6
(XEN) 	max_weight         = 256
(XEN) 	pick_bias          = 1
(XEN) 	instload           = 1
(XEN) 	aveload            = 3992 (~1%)
(XEN) 	idlers: 0000006f
(XEN) 	tickled: 00000000
(XEN) 	fully idle cores: 0000004f

Then, I did:

# echo 1 > /sys/devices/system/xen_cpu/xen_cpu7/online

And again it appear to have worked, i.e., the CPU is back online and in
Pool-0:

(XEN) Online Cpus: 0-7
(XEN) Cpupool 0:
(XEN) Cpus: 0-7
(XEN) Scheduler: SMP Credit Scheduler rev2 (credit2)
(XEN) Active queues: 1
(XEN) 	default-weight     = 256
(XEN) Runqueue 0:
(XEN) 	ncpus              = 8
(XEN) 	cpus               = 0-7
(XEN) 	max_weight         = 256
(XEN) 	pick_bias          = 1
(XEN) 	instload           = 2
(XEN) 	aveload            = 271474 (~103%)
(XEN) 	idlers: 000000af
(XEN) 	tickled: 00000000
(XEN) 	fully idle cores: 0000008f

Then I did:

# echo 0 > /sys/devices/system/xen_cpu/xen_cpu7/online

And, after that:

# xl cpupool-cpu-remove Pool-0 7

And the system hanged.

I don't have a working serial console on that testbox, unfortunately,
so I can't poke at debug keys, etc.

Is this anything that you've seen or that you can reproduce?

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


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Xen-devel] [PATCH v3 2/4] xen/sched: remove cpu from pool0 before removing it
  2019-09-13 17:27   ` Dario Faggioli
@ 2019-09-14  5:04     ` Juergen Gross
  0 siblings, 0 replies; 9+ messages in thread
From: Juergen Gross @ 2019-09-14  5:04 UTC (permalink / raw)
  To: Dario Faggioli, xen-devel
  Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
	George Dunlap, Andrew Cooper, Tim Deegan, Julien Grall,
	Jan Beulich, Ian Jackson

On 13.09.19 19:27, Dario Faggioli wrote:
> On Mon, 2019-09-09 at 11:33 +0200, Juergen Gross wrote:
>> Today a cpu which is removed from the system is taken directly from
>> Pool0 to the offline state. This will conflict with the new idle
>> scheduler, so remove it from Pool0 first. Additionally accept
>> removing
>> a free cpu instead of requiring it to be in Pool0.
>>
>> For the resume failed case we need to call the scheduler code for
>> that
>> situation after the cpupool handling, so move the scheduler code into
>> a function and call it from cpupool_cpu_remove_forced() and remove
>> the
>> CPU_RESUME_FAILED case from cpu_schedule_callback().
>>
>> Note that we are calling now schedule_cpu_switch() in stop_machine
>> context so we need to switch from spinlock_irq to spinlock_irqsave.
>>
> So, I was looking at this patch, and while doing that, also trying it
> out.
> 
> I've done the following:
> 
> # echo 0 > /sys/devices/system/xen_cpu/xen_cpu7/online
> 
> And CPU 7 went offline, and was listed among the free CPUs:
> 
> (XEN) Online Cpus: 0-6
> (XEN) Free Cpus: 7
> (XEN) Cpupool 0:
> (XEN) Cpus: 0-6
> (XEN) Scheduler: SMP Credit Scheduler rev2 (credit2)
> (XEN) Active queues: 1
> (XEN) 	default-weight     = 256
> (XEN) Runqueue 0:
> (XEN) 	ncpus              = 7
> (XEN) 	cpus               = 0-6
> (XEN) 	max_weight         = 256
> (XEN) 	pick_bias          = 1
> (XEN) 	instload           = 1
> (XEN) 	aveload            = 3992 (~1%)
> (XEN) 	idlers: 0000006f
> (XEN) 	tickled: 00000000
> (XEN) 	fully idle cores: 0000004f
> 
> Then, I did:
> 
> # echo 1 > /sys/devices/system/xen_cpu/xen_cpu7/online
> 
> And again it appear to have worked, i.e., the CPU is back online and in
> Pool-0:
> 
> (XEN) Online Cpus: 0-7
> (XEN) Cpupool 0:
> (XEN) Cpus: 0-7
> (XEN) Scheduler: SMP Credit Scheduler rev2 (credit2)
> (XEN) Active queues: 1
> (XEN) 	default-weight     = 256
> (XEN) Runqueue 0:
> (XEN) 	ncpus              = 8
> (XEN) 	cpus               = 0-7
> (XEN) 	max_weight         = 256
> (XEN) 	pick_bias          = 1
> (XEN) 	instload           = 2
> (XEN) 	aveload            = 271474 (~103%)
> (XEN) 	idlers: 000000af
> (XEN) 	tickled: 00000000
> (XEN) 	fully idle cores: 0000008f
> 
> Then I did:
> 
> # echo 0 > /sys/devices/system/xen_cpu/xen_cpu7/online
> 
> And, after that:
> 
> # xl cpupool-cpu-remove Pool-0 7
> 
> And the system hanged.
> 
> I don't have a working serial console on that testbox, unfortunately,
> so I can't poke at debug keys, etc.
> 
> Is this anything that you've seen or that you can reproduce?

I can reproduce it and already have found the bug.


Juergen


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-09-14  5:05 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-09  9:33 [Xen-devel] [PATCH v3 0/4] xen/sched: use new idle scheduler for free cpus Juergen Gross
2019-09-09  9:33 ` [Xen-devel] [PATCH v3 1/4] xen/sched: populate cpupool0 only after all cpus are up Juergen Gross
2019-09-13 16:54   ` Dario Faggioli
2019-09-09  9:33 ` [Xen-devel] [PATCH v3 2/4] xen/sched: remove cpu from pool0 before removing it Juergen Gross
2019-09-13 17:27   ` Dario Faggioli
2019-09-14  5:04     ` Juergen Gross
2019-09-09  9:33 ` [Xen-devel] [PATCH v3 3/4] xen/sched: add minimalistic idle scheduler for free cpus Juergen Gross
2019-09-09  9:33 ` [Xen-devel] [PATCH v3 4/4] xen/sched: switch to debugtrace in cpupool handling Juergen Gross
2019-09-12  8:24   ` Dario Faggioli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).