[PATCH v2 0/3] xen/sched: fix cpu hotplug

[PATCH v2 0/3] xen/sched: fix cpu hotplug

[Juergen Gross]

A recent change in the hypervisor memory allocation framework led to
crashes when unplugging host cpus.

This was due to the (correct) assertion that allocating and freeing
memory is allowed with enabled interrupts only. As the main cpu unplug
operation is done in stop-machine context, this assertion triggers in
debug builds.

Correct that by pre-allocating all needed memory while interrupts are
still on, and free memory after interrupts are enabled again.

Changes in V2:
- addressed all comments

Juergen Gross (3):
  xen/sched: introduce cpupool_update_node_affinity()
  xen/sched: carve out memory allocation and freeing from
    schedule_cpu_rm()
  xen/sched: fix cpu hotplug

 xen/common/sched/core.c    | 204 +++++++++++++++++++++++--------------
 xen/common/sched/cpupool.c |  91 ++++++++++++-----
 xen/common/sched/private.h |  21 +++-
 xen/include/xen/sched.h    |   9 +-
 4 files changed, 225 insertions(+), 100 deletions(-)

-- 
2.35.3

[PATCH v2 1/3] xen/sched: introduce cpupool_update_node_affinity()

[Juergen Gross]

For updating the node affinities of all domains in a cpupool add a new
function cpupool_update_node_affinity().

In order to avoid multiple allocations of cpumasks carve out memory
allocation and freeing from domain_update_node_affinity() into new
helpers, which can be used by cpupool_update_node_affinity().

Modify domain_update_node_affinity() to take an additional parameter
for passing the allocated memory in and to allocate and free the memory
via the new helpers in case NULL was passed.

This will help later to pre-allocate the cpumasks in order to avoid
allocations in stop-machine context.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- move helpers to core.c (Jan Beulich)
- allocate/free memory in domain_update_node_aff() if NULL was passed
  in (Jan Beulich)
---
 xen/common/sched/core.c    | 54 ++++++++++++++++++++++++++------------
 xen/common/sched/cpupool.c | 39 +++++++++++++++------------
 xen/common/sched/private.h |  7 +++++
 xen/include/xen/sched.h    |  9 ++++++-
 4 files changed, 74 insertions(+), 35 deletions(-)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index ff1ddc7624..085a9dd335 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -1824,9 +1824,28 @@ int vcpu_affinity_domctl(struct domain *d, uint32_t cmd,
     return ret;
 }
 
-void domain_update_node_affinity(struct domain *d)
+bool update_node_aff_alloc(struct affinity_masks *affinity)
 {
-    cpumask_var_t dom_cpumask, dom_cpumask_soft;
+    if ( !alloc_cpumask_var(&affinity->hard) )
+        return false;
+    if ( !alloc_cpumask_var(&affinity->soft) )
+    {
+        free_cpumask_var(affinity->hard);
+        return false;
+    }
+
+    return true;
+}
+
+void update_node_aff_free(struct affinity_masks *affinity)
+{
+    free_cpumask_var(affinity->soft);
+    free_cpumask_var(affinity->hard);
+}
+
+void domain_update_node_aff(struct domain *d, struct affinity_masks *affinity)
+{
+    struct affinity_masks masks = { };
     cpumask_t *dom_affinity;
     const cpumask_t *online;
     struct sched_unit *unit;
@@ -1836,14 +1855,16 @@ void domain_update_node_affinity(struct domain *d)
     if ( !d->vcpu || !d->vcpu[0] )
         return;
 
-    if ( !zalloc_cpumask_var(&dom_cpumask) )
-        return;
-    if ( !zalloc_cpumask_var(&dom_cpumask_soft) )
+    if ( !affinity )
     {
-        free_cpumask_var(dom_cpumask);
-        return;
+        affinity = &masks;
+        if ( !update_node_aff_alloc(affinity) )
+            return;
     }
 
+    cpumask_clear(affinity->hard);
+    cpumask_clear(affinity->soft);
+
     online = cpupool_domain_master_cpumask(d);
 
     spin_lock(&d->node_affinity_lock);
@@ -1864,22 +1885,21 @@ void domain_update_node_affinity(struct domain *d)
          */
         for_each_sched_unit ( d, unit )
         {
-            cpumask_or(dom_cpumask, dom_cpumask, unit->cpu_hard_affinity);
-            cpumask_or(dom_cpumask_soft, dom_cpumask_soft,
-                       unit->cpu_soft_affinity);
+            cpumask_or(affinity->hard, affinity->hard, unit->cpu_hard_affinity);
+            cpumask_or(affinity->soft, affinity->soft, unit->cpu_soft_affinity);
         }
         /* Filter out non-online cpus */
-        cpumask_and(dom_cpumask, dom_cpumask, online);
-        ASSERT(!cpumask_empty(dom_cpumask));
+        cpumask_and(affinity->hard, affinity->hard, online);
+        ASSERT(!cpumask_empty(affinity->hard));
         /* And compute the intersection between hard, online and soft */
-        cpumask_and(dom_cpumask_soft, dom_cpumask_soft, dom_cpumask);
+        cpumask_and(affinity->soft, affinity->soft, affinity->hard);
 
         /*
          * If not empty, the intersection of hard, soft and online is the
          * narrowest set we want. If empty, we fall back to hard&online.
          */
-        dom_affinity = cpumask_empty(dom_cpumask_soft) ?
-                           dom_cpumask : dom_cpumask_soft;
+        dom_affinity = cpumask_empty(affinity->soft) ? affinity->hard
+                                                     : affinity->soft;
 
         nodes_clear(d->node_affinity);
         for_each_cpu ( cpu, dom_affinity )
@@ -1888,8 +1908,8 @@ void domain_update_node_affinity(struct domain *d)
 
     spin_unlock(&d->node_affinity_lock);
 
-    free_cpumask_var(dom_cpumask_soft);
-    free_cpumask_var(dom_cpumask);
+    if ( affinity == &masks )
+        update_node_aff_free(affinity);
 }
 
 typedef long ret_t;
diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index 2afe54f54d..58e082eb4c 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -410,6 +410,25 @@ int cpupool_move_domain(struct domain *d, struct cpupool *c)
     return ret;
 }
 
+/* Update affinities of all domains in a cpupool. */
+static void cpupool_update_node_affinity(const struct cpupool *c)
+{
+    struct affinity_masks masks;
+    struct domain *d;
+
+    if ( !update_node_aff_alloc(&masks) )
+        return;
+
+    rcu_read_lock(&domlist_read_lock);
+
+    for_each_domain_in_cpupool(d, c)
+        domain_update_node_aff(d, &masks);
+
+    rcu_read_unlock(&domlist_read_lock);
+
+    update_node_aff_free(&masks);
+}
+
 /*
  * assign a specific cpu to a cpupool
  * cpupool_lock must be held
@@ -417,7 +436,6 @@ int cpupool_move_domain(struct domain *d, struct cpupool *c)
 static int cpupool_assign_cpu_locked(struct cpupool *c, unsigned int cpu)
 {
     int ret;
-    struct domain *d;
     const cpumask_t *cpus;
 
     cpus = sched_get_opt_cpumask(c->gran, cpu);
@@ -442,12 +460,7 @@ static int cpupool_assign_cpu_locked(struct cpupool *c, unsigned int cpu)
 
     rcu_read_unlock(&sched_res_rculock);
 
-    rcu_read_lock(&domlist_read_lock);
-    for_each_domain_in_cpupool(d, c)
-    {
-        domain_update_node_affinity(d);
-    }
-    rcu_read_unlock(&domlist_read_lock);
+    cpupool_update_node_affinity(c);
 
     return 0;
 }
@@ -456,18 +469,14 @@ static int cpupool_unassign_cpu_finish(struct cpupool *c)
 {
     int cpu = cpupool_moving_cpu;
     const cpumask_t *cpus;
-    struct domain *d;
     int ret;
 
     if ( c != cpupool_cpu_moving )
         return -EADDRNOTAVAIL;
 
-    /*
-     * We need this for scanning the domain list, both in
-     * cpu_disable_scheduler(), and at the bottom of this function.
-     */
     rcu_read_lock(&domlist_read_lock);
     ret = cpu_disable_scheduler(cpu);
+    rcu_read_unlock(&domlist_read_lock);
 
     rcu_read_lock(&sched_res_rculock);
     cpus = get_sched_res(cpu)->cpus;
@@ -494,11 +503,7 @@ static int cpupool_unassign_cpu_finish(struct cpupool *c)
     }
     rcu_read_unlock(&sched_res_rculock);
 
-    for_each_domain_in_cpupool(d, c)
-    {
-        domain_update_node_affinity(d);
-    }
-    rcu_read_unlock(&domlist_read_lock);
+    cpupool_update_node_affinity(c);
 
     return ret;
 }
diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
index a870320146..38251b1f7b 100644
--- a/xen/common/sched/private.h
+++ b/xen/common/sched/private.h
@@ -593,6 +593,13 @@ affinity_balance_cpumask(const struct sched_unit *unit, int step,
         cpumask_copy(mask, unit->cpu_hard_affinity);
 }
 
+struct affinity_masks {
+    cpumask_var_t hard;
+    cpumask_var_t soft;
+};
+
+bool update_node_aff_alloc(struct affinity_masks *affinity);
+void update_node_aff_free(struct affinity_masks *affinity);
 void sched_rm_cpu(unsigned int cpu);
 const cpumask_t *sched_get_opt_cpumask(enum sched_gran opt, unsigned int cpu);
 void schedule_dump(struct cpupool *c);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index e2b3b6daa3..666264b8c3 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -663,8 +663,15 @@ static inline void get_knownalive_domain(struct domain *d)
     ASSERT(!(atomic_read(&d->refcnt) & DOMAIN_DESTROYED));
 }
 
+struct affinity_masks;
+
 int domain_set_node_affinity(struct domain *d, const nodemask_t *affinity);
-void domain_update_node_affinity(struct domain *d);
+void domain_update_node_aff(struct domain *d, struct affinity_masks *affinity);
+
+static inline void domain_update_node_affinity(struct domain *d)
+{
+    domain_update_node_aff(d, NULL);
+}
 
 /*
  * To be implemented by each architecture, sanity checking the configuration
-- 
2.35.3

[PATCH v2 2/3] xen/sched: carve out memory allocation and freeing from schedule_cpu_rm()

[Juergen Gross]

In order to prepare not allocating or freeing memory from
schedule_cpu_rm(), move this functionality to dedicated functions.

For now call those functions from schedule_cpu_rm().

No change of behavior expected.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- add const (Jan Beulich)
- use "unsigned int" for loop index (Jan Beulich)
- use xmalloc_flex_struct() (Jan Beulich)
- use XFREE() (Jan Beulich)
- hold rcu lock longer (Jan Beulich)
- add ASSERT() (Jan Beulich)

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/sched/core.c    | 133 +++++++++++++++++++++----------------
 xen/common/sched/private.h |   9 +++
 2 files changed, 86 insertions(+), 56 deletions(-)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 085a9dd335..d0b6513b6f 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -3237,6 +3237,65 @@ out:
     return ret;
 }
 
+static struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu)
+{
+    struct cpu_rm_data *data;
+    const struct sched_resource *sr;
+    unsigned int idx;
+
+    rcu_read_lock(&sched_res_rculock);
+
+    sr = get_sched_res(cpu);
+    data = xmalloc_flex_struct(struct cpu_rm_data, sr, sr->granularity - 1);
+    if ( !data )
+        goto out;
+
+    data->old_ops = sr->scheduler;
+    data->vpriv_old = idle_vcpu[cpu]->sched_unit->priv;
+    data->ppriv_old = sr->sched_priv;
+
+    for ( idx = 0; idx < sr->granularity - 1; idx++ )
+    {
+        data->sr[idx] = sched_alloc_res();
+        if ( data->sr[idx] )
+        {
+            data->sr[idx]->sched_unit_idle = sched_alloc_unit_mem();
+            if ( !data->sr[idx]->sched_unit_idle )
+            {
+                sched_res_free(&data->sr[idx]->rcu);
+                data->sr[idx] = NULL;
+            }
+        }
+        if ( !data->sr[idx] )
+        {
+            while ( idx > 0 )
+                sched_res_free(&data->sr[--idx]->rcu);
+            XFREE(data);
+            goto out;
+        }
+
+        data->sr[idx]->curr = data->sr[idx]->sched_unit_idle;
+        data->sr[idx]->scheduler = &sched_idle_ops;
+        data->sr[idx]->granularity = 1;
+
+        /* We want the lock not to change when replacing the resource. */
+        data->sr[idx]->schedule_lock = sr->schedule_lock;
+    }
+
+ out:
+    rcu_read_unlock(&sched_res_rculock);
+
+    return data;
+}
+
+static void schedule_cpu_rm_free(struct cpu_rm_data *mem, unsigned int cpu)
+{
+    sched_free_udata(mem->old_ops, mem->vpriv_old);
+    sched_free_pdata(mem->old_ops, mem->ppriv_old, cpu);
+
+    xfree(mem);
+}
+
 /*
  * Remove a pCPU from its cpupool. Its scheduler becomes &sched_idle_ops
  * (the idle scheduler).
@@ -3245,53 +3304,23 @@ out:
  */
 int schedule_cpu_rm(unsigned int cpu)
 {
-    void *ppriv_old, *vpriv_old;
-    struct sched_resource *sr, **sr_new = NULL;
+    struct sched_resource *sr;
+    struct cpu_rm_data *data;
     struct sched_unit *unit;
-    struct scheduler *old_ops;
     spinlock_t *old_lock;
     unsigned long flags;
-    int idx, ret = -ENOMEM;
+    int idx = 0;
     unsigned int cpu_iter;
 
+    data = schedule_cpu_rm_alloc(cpu);
+    if ( !data )
+        return -ENOMEM;
+
     rcu_read_lock(&sched_res_rculock);
 
     sr = get_sched_res(cpu);
-    old_ops = sr->scheduler;
 
-    if ( sr->granularity > 1 )
-    {
-        sr_new = xmalloc_array(struct sched_resource *, sr->granularity - 1);
-        if ( !sr_new )
-            goto out;
-        for ( idx = 0; idx < sr->granularity - 1; idx++ )
-        {
-            sr_new[idx] = sched_alloc_res();
-            if ( sr_new[idx] )
-            {
-                sr_new[idx]->sched_unit_idle = sched_alloc_unit_mem();
-                if ( !sr_new[idx]->sched_unit_idle )
-                {
-                    sched_res_free(&sr_new[idx]->rcu);
-                    sr_new[idx] = NULL;
-                }
-            }
-            if ( !sr_new[idx] )
-            {
-                for ( idx--; idx >= 0; idx-- )
-                    sched_res_free(&sr_new[idx]->rcu);
-                goto out;
-            }
-            sr_new[idx]->curr = sr_new[idx]->sched_unit_idle;
-            sr_new[idx]->scheduler = &sched_idle_ops;
-            sr_new[idx]->granularity = 1;
-
-            /* We want the lock not to change when replacing the resource. */
-            sr_new[idx]->schedule_lock = sr->schedule_lock;
-        }
-    }
-
-    ret = 0;
+    ASSERT(sr->granularity);
     ASSERT(sr->cpupool != NULL);
     ASSERT(cpumask_test_cpu(cpu, &cpupool_free_cpus));
     ASSERT(!cpumask_test_cpu(cpu, sr->cpupool->cpu_valid));
@@ -3299,10 +3328,6 @@ int schedule_cpu_rm(unsigned int cpu)
     /* See comment in schedule_cpu_add() regarding lock switching. */
     old_lock = pcpu_schedule_lock_irqsave(cpu, &flags);
 
-    vpriv_old = idle_vcpu[cpu]->sched_unit->priv;
-    ppriv_old = sr->sched_priv;
-
-    idx = 0;
     for_each_cpu ( cpu_iter, sr->cpus )
     {
         per_cpu(sched_res_idx, cpu_iter) = 0;
@@ -3316,27 +3341,27 @@ int schedule_cpu_rm(unsigned int cpu)
         else
         {
             /* Initialize unit. */
-            unit = sr_new[idx]->sched_unit_idle;
-            unit->res = sr_new[idx];
+            unit = data->sr[idx]->sched_unit_idle;
+            unit->res = data->sr[idx];
             unit->is_running = true;
             sched_unit_add_vcpu(unit, idle_vcpu[cpu_iter]);
             sched_domain_insert_unit(unit, idle_vcpu[cpu_iter]->domain);
 
             /* Adjust cpu masks of resources (old and new). */
             cpumask_clear_cpu(cpu_iter, sr->cpus);
-            cpumask_set_cpu(cpu_iter, sr_new[idx]->cpus);
+            cpumask_set_cpu(cpu_iter, data->sr[idx]->cpus);
             cpumask_set_cpu(cpu_iter, &sched_res_mask);
 
             /* Init timer. */
-            init_timer(&sr_new[idx]->s_timer, s_timer_fn, NULL, cpu_iter);
+            init_timer(&data->sr[idx]->s_timer, s_timer_fn, NULL, cpu_iter);
 
             /* Last resource initializations and insert resource pointer. */
-            sr_new[idx]->master_cpu = cpu_iter;
-            set_sched_res(cpu_iter, sr_new[idx]);
+            data->sr[idx]->master_cpu = cpu_iter;
+            set_sched_res(cpu_iter, data->sr[idx]);
 
             /* Last action: set the new lock pointer. */
             smp_mb();
-            sr_new[idx]->schedule_lock = &sched_free_cpu_lock;
+            data->sr[idx]->schedule_lock = &sched_free_cpu_lock;
 
             idx++;
         }
@@ -3352,16 +3377,12 @@ int schedule_cpu_rm(unsigned int cpu)
     /* _Not_ pcpu_schedule_unlock(): schedule_lock may have changed! */
     spin_unlock_irqrestore(old_lock, flags);
 
-    sched_deinit_pdata(old_ops, ppriv_old, cpu);
-
-    sched_free_udata(old_ops, vpriv_old);
-    sched_free_pdata(old_ops, ppriv_old, cpu);
+    sched_deinit_pdata(data->old_ops, data->ppriv_old, cpu);
 
-out:
     rcu_read_unlock(&sched_res_rculock);
-    xfree(sr_new);
+    schedule_cpu_rm_free(data, cpu);
 
-    return ret;
+    return 0;
 }
 
 struct scheduler *scheduler_get_default(void)
diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
index 38251b1f7b..601d639699 100644
--- a/xen/common/sched/private.h
+++ b/xen/common/sched/private.h
@@ -600,6 +600,15 @@ struct affinity_masks {
 
 bool update_node_aff_alloc(struct affinity_masks *affinity);
 void update_node_aff_free(struct affinity_masks *affinity);
+
+/* Memory allocation related data for schedule_cpu_rm(). */
+struct cpu_rm_data {
+    const struct scheduler *old_ops;
+    void *ppriv_old;
+    void *vpriv_old;
+    struct sched_resource *sr[];
+};
+
 void sched_rm_cpu(unsigned int cpu);
 const cpumask_t *sched_get_opt_cpumask(enum sched_gran opt, unsigned int cpu);
 void schedule_dump(struct cpupool *c);
-- 
2.35.3

[PATCH v2 3/3] xen/sched: fix cpu hotplug

[Juergen Gross]

Cpu cpu unplugging is calling schedule_cpu_rm() via stop_machine_run()
with interrupts disabled, thus any memory allocation or freeing must
be avoided.

Since commit 5047cd1d5dea ("xen/common: Use enhanced
ASSERT_ALLOC_CONTEXT in xmalloc()") this restriction is being enforced
via an assertion, which will now fail.

Before that commit cpu unplugging in normal configurations was working
just by chance as only the cpu performing schedule_cpu_rm() was doing
active work. With core scheduling enabled, however, failures could
result from memory allocations not being properly propagated to other
cpus' TLBs.

Fix this mess by allocating needed memory before entering
stop_machine_run() and freeing any memory only after having finished
stop_machine_run().

Fixes: 1ec410112cdd ("xen/sched: support differing granularity in schedule_cpu_[add/rm]()")
Reported-by: Gao Ruifeng <ruifeng.gao@intel.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- move affinity mask allocation into schedule_cpu_rm_alloc() (Jan Beulich)
---
 xen/common/sched/core.c    | 27 +++++++++++----
 xen/common/sched/cpupool.c | 68 +++++++++++++++++++++++++++++---------
 xen/common/sched/private.h |  5 ++-
 3 files changed, 78 insertions(+), 22 deletions(-)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index d0b6513b6f..ec09c8e5a9 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -3237,7 +3237,7 @@ out:
     return ret;
 }
 
-static struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu)
+struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu, bool aff_alloc)
 {
     struct cpu_rm_data *data;
     const struct sched_resource *sr;
@@ -3250,6 +3250,17 @@ static struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu)
     if ( !data )
         goto out;
 
+    if ( aff_alloc )
+    {
+        if ( !update_node_aff_alloc(&data->affinity) )
+        {
+            XFREE(data);
+            goto out;
+        }
+    }
+    else
+        memset(&data->affinity, 0, sizeof(data->affinity));
+
     data->old_ops = sr->scheduler;
     data->vpriv_old = idle_vcpu[cpu]->sched_unit->priv;
     data->ppriv_old = sr->sched_priv;
@@ -3270,6 +3281,7 @@ static struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu)
         {
             while ( idx > 0 )
                 sched_res_free(&data->sr[--idx]->rcu);
+            update_node_aff_free(&data->affinity);
             XFREE(data);
             goto out;
         }
@@ -3288,10 +3300,11 @@ static struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu)
     return data;
 }
 
-static void schedule_cpu_rm_free(struct cpu_rm_data *mem, unsigned int cpu)
+void schedule_cpu_rm_free(struct cpu_rm_data *mem, unsigned int cpu)
 {
     sched_free_udata(mem->old_ops, mem->vpriv_old);
     sched_free_pdata(mem->old_ops, mem->ppriv_old, cpu);
+    update_node_aff_free(&mem->affinity);
 
     xfree(mem);
 }
@@ -3302,17 +3315,18 @@ static void schedule_cpu_rm_free(struct cpu_rm_data *mem, unsigned int cpu)
  * The cpu is already marked as "free" and not valid any longer for its
  * cpupool.
  */
-int schedule_cpu_rm(unsigned int cpu)
+int schedule_cpu_rm(unsigned int cpu, struct cpu_rm_data *data)
 {
     struct sched_resource *sr;
-    struct cpu_rm_data *data;
     struct sched_unit *unit;
     spinlock_t *old_lock;
     unsigned long flags;
     int idx = 0;
     unsigned int cpu_iter;
+    bool freemem = !data;
 
-    data = schedule_cpu_rm_alloc(cpu);
+    if ( !data )
+        data = schedule_cpu_rm_alloc(cpu, false);
     if ( !data )
         return -ENOMEM;
 
@@ -3380,7 +3394,8 @@ int schedule_cpu_rm(unsigned int cpu)
     sched_deinit_pdata(data->old_ops, data->ppriv_old, cpu);
 
     rcu_read_unlock(&sched_res_rculock);
-    schedule_cpu_rm_free(data, cpu);
+    if ( freemem )
+        schedule_cpu_rm_free(data, cpu);
 
     return 0;
 }
diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index 58e082eb4c..2506861e4f 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -411,22 +411,28 @@ int cpupool_move_domain(struct domain *d, struct cpupool *c)
 }
 
 /* Update affinities of all domains in a cpupool. */
-static void cpupool_update_node_affinity(const struct cpupool *c)
+static void cpupool_update_node_affinity(const struct cpupool *c,
+                                         struct affinity_masks *masks)
 {
-    struct affinity_masks masks;
+    struct affinity_masks local_masks;
     struct domain *d;
 
-    if ( !update_node_aff_alloc(&masks) )
-        return;
+    if ( !masks )
+    {
+        if ( !update_node_aff_alloc(&local_masks) )
+            return;
+        masks = &local_masks;
+    }
 
     rcu_read_lock(&domlist_read_lock);
 
     for_each_domain_in_cpupool(d, c)
-        domain_update_node_aff(d, &masks);
+        domain_update_node_aff(d, masks);
 
     rcu_read_unlock(&domlist_read_lock);
 
-    update_node_aff_free(&masks);
+    if ( masks == &local_masks )
+        update_node_aff_free(masks);
 }
 
 /*
@@ -460,15 +466,17 @@ static int cpupool_assign_cpu_locked(struct cpupool *c, unsigned int cpu)
 
     rcu_read_unlock(&sched_res_rculock);
 
-    cpupool_update_node_affinity(c);
+    cpupool_update_node_affinity(c, NULL);
 
     return 0;
 }
 
-static int cpupool_unassign_cpu_finish(struct cpupool *c)
+static int cpupool_unassign_cpu_finish(struct cpupool *c,
+                                       struct cpu_rm_data *mem)
 {
     int cpu = cpupool_moving_cpu;
     const cpumask_t *cpus;
+    struct affinity_masks *masks = mem ? &mem->affinity : NULL;
     int ret;
 
     if ( c != cpupool_cpu_moving )
@@ -491,7 +499,7 @@ static int cpupool_unassign_cpu_finish(struct cpupool *c)
      */
     if ( !ret )
     {
-        ret = schedule_cpu_rm(cpu);
+        ret = schedule_cpu_rm(cpu, mem);
         if ( ret )
             cpumask_andnot(&cpupool_free_cpus, &cpupool_free_cpus, cpus);
         else
@@ -503,7 +511,7 @@ static int cpupool_unassign_cpu_finish(struct cpupool *c)
     }
     rcu_read_unlock(&sched_res_rculock);
 
-    cpupool_update_node_affinity(c);
+    cpupool_update_node_affinity(c, masks);
 
     return ret;
 }
@@ -567,7 +575,7 @@ static long cf_check cpupool_unassign_cpu_helper(void *info)
                       cpupool_cpu_moving->cpupool_id, cpupool_moving_cpu);
     spin_lock(&cpupool_lock);
 
-    ret = cpupool_unassign_cpu_finish(c);
+    ret = cpupool_unassign_cpu_finish(c, NULL);
 
     spin_unlock(&cpupool_lock);
     debugtrace_printk("cpupool_unassign_cpu ret=%ld\n", ret);
@@ -714,7 +722,7 @@ static int cpupool_cpu_add(unsigned int cpu)
  * This function is called in stop_machine context, so we can be sure no
  * non-idle vcpu is active on the system.
  */
-static void cpupool_cpu_remove(unsigned int cpu)
+static void cpupool_cpu_remove(unsigned int cpu, struct cpu_rm_data *mem)
 {
     int ret;
 
@@ -722,7 +730,7 @@ static void cpupool_cpu_remove(unsigned int cpu)
 
     if ( !cpumask_test_cpu(cpu, &cpupool_free_cpus) )
     {
-        ret = cpupool_unassign_cpu_finish(cpupool0);
+        ret = cpupool_unassign_cpu_finish(cpupool0, mem);
         BUG_ON(ret);
     }
     cpumask_clear_cpu(cpu, &cpupool_free_cpus);
@@ -788,7 +796,7 @@ static void cpupool_cpu_remove_forced(unsigned int cpu)
         {
             ret = cpupool_unassign_cpu_start(c, master_cpu);
             BUG_ON(ret);
-            ret = cpupool_unassign_cpu_finish(c);
+            ret = cpupool_unassign_cpu_finish(c, NULL);
             BUG_ON(ret);
         }
     }
@@ -1008,10 +1016,21 @@ static int cf_check cpu_callback(
 {
     unsigned int cpu = (unsigned long)hcpu;
     int rc = 0;
+    static struct cpu_rm_data *mem;
 
     switch ( action )
     {
     case CPU_DOWN_FAILED:
+        if ( system_state <= SYS_STATE_active )
+        {
+            if ( mem )
+            {
+                schedule_cpu_rm_free(mem, cpu);
+                mem = NULL;
+            }
+            rc = cpupool_cpu_add(cpu);
+        }
+        break;
     case CPU_ONLINE:
         if ( system_state <= SYS_STATE_active )
             rc = cpupool_cpu_add(cpu);
@@ -1019,12 +1038,31 @@ static int cf_check 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_prologue(cpu);
+            if ( !rc )
+            {
+                ASSERT(!mem);
+                mem = schedule_cpu_rm_alloc(cpu, true);
+                rc = mem ? 0 : -ENOMEM;
+            }
+        }
         break;
     case CPU_DYING:
         /* Suspend/Resume don't change assignments of cpus to cpupools. */
         if ( system_state <= SYS_STATE_active )
-            cpupool_cpu_remove(cpu);
+        {
+            ASSERT(mem);
+            cpupool_cpu_remove(cpu, mem);
+        }
+        break;
+    case CPU_DEAD:
+        if ( system_state <= SYS_STATE_active )
+        {
+            ASSERT(mem);
+            schedule_cpu_rm_free(mem, cpu);
+            mem = NULL;
+        }
         break;
     case CPU_RESUME_FAILED:
         cpupool_cpu_remove_forced(cpu);
diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
index 601d639699..cc7a6cb571 100644
--- a/xen/common/sched/private.h
+++ b/xen/common/sched/private.h
@@ -603,6 +603,7 @@ void update_node_aff_free(struct affinity_masks *affinity);
 
 /* Memory allocation related data for schedule_cpu_rm(). */
 struct cpu_rm_data {
+    struct affinity_masks affinity;
     const struct scheduler *old_ops;
     void *ppriv_old;
     void *vpriv_old;
@@ -617,7 +618,9 @@ struct scheduler *scheduler_alloc(unsigned int sched_id);
 void scheduler_free(struct scheduler *sched);
 int cpu_disable_scheduler(unsigned int cpu);
 int schedule_cpu_add(unsigned int cpu, struct cpupool *c);
-int schedule_cpu_rm(unsigned int cpu);
+struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu, bool aff_alloc);
+void schedule_cpu_rm_free(struct cpu_rm_data *mem, unsigned int cpu);
+int schedule_cpu_rm(unsigned int cpu, struct cpu_rm_data *mem);
 int sched_move_domain(struct domain *d, struct cpupool *c);
 struct cpupool *cpupool_get_by_id(unsigned int poolid);
 void cpupool_put(struct cpupool *pool);
-- 
2.35.3

Re: [PATCH v2 1/3] xen/sched: introduce cpupool_update_node_affinity()

[Jan Beulich]

On 15.08.2022 13:04, Juergen Gross wrote:
> For updating the node affinities of all domains in a cpupool add a new
> function cpupool_update_node_affinity().
> 
> In order to avoid multiple allocations of cpumasks carve out memory
> allocation and freeing from domain_update_node_affinity() into new
> helpers, which can be used by cpupool_update_node_affinity().
> 
> Modify domain_update_node_affinity() to take an additional parameter
> for passing the allocated memory in and to allocate and free the memory
> via the new helpers in case NULL was passed.
> 
> This will help later to pre-allocate the cpumasks in order to avoid
> allocations in stop-machine context.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with the observation that ...

> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -1824,9 +1824,28 @@ int vcpu_affinity_domctl(struct domain *d, uint32_t cmd,
>      return ret;
>  }
>  
> -void domain_update_node_affinity(struct domain *d)
> +bool update_node_aff_alloc(struct affinity_masks *affinity)
>  {
> -    cpumask_var_t dom_cpumask, dom_cpumask_soft;
> +    if ( !alloc_cpumask_var(&affinity->hard) )
> +        return false;
> +    if ( !alloc_cpumask_var(&affinity->soft) )
> +    {
> +        free_cpumask_var(affinity->hard);
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +void update_node_aff_free(struct affinity_masks *affinity)
> +{
> +    free_cpumask_var(affinity->soft);
> +    free_cpumask_var(affinity->hard);
> +}
> +
> +void domain_update_node_aff(struct domain *d, struct affinity_masks *affinity)
> +{
> +    struct affinity_masks masks = { };

... the initializer doesn't really look to be needed here, just like
you don't have one in cpupool_update_node_affinity(). The one thing
I'm not sure about is whether old gcc might mis-report a potentially
uninitialized variable with the initializer dropped ...

Jan

Re: [PATCH v2 2/3] xen/sched: carve out memory allocation and freeing from schedule_cpu_rm()

[Jan Beulich]

On 15.08.2022 13:04, Juergen Gross wrote:
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -3237,6 +3237,65 @@ out:
>      return ret;
>  }
>  
> +static struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu)
> +{
> +    struct cpu_rm_data *data;
> +    const struct sched_resource *sr;
> +    unsigned int idx;
> +
> +    rcu_read_lock(&sched_res_rculock);
> +
> +    sr = get_sched_res(cpu);
> +    data = xmalloc_flex_struct(struct cpu_rm_data, sr, sr->granularity - 1);
> +    if ( !data )
> +        goto out;
> +
> +    data->old_ops = sr->scheduler;
> +    data->vpriv_old = idle_vcpu[cpu]->sched_unit->priv;
> +    data->ppriv_old = sr->sched_priv;

Repeating a v1 comment:

"At least from an abstract perspective, doesn't reading fields from
 sr require the RCU lock to be held continuously (i.e. not dropping
 it at the end of this function and re-acquiring it in the caller)?"

Initially I thought you did respond to this in some way, but when
looking for a matching reply I couldn't find one.

Jan

Re: [PATCH v2 2/3] xen/sched: carve out memory allocation and freeing from schedule_cpu_rm()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1391 bytes --]

On 15.08.22 13:52, Jan Beulich wrote:
> On 15.08.2022 13:04, Juergen Gross wrote:
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -3237,6 +3237,65 @@ out:
>>       return ret;
>>   }
>>   
>> +static struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu)
>> +{
>> +    struct cpu_rm_data *data;
>> +    const struct sched_resource *sr;
>> +    unsigned int idx;
>> +
>> +    rcu_read_lock(&sched_res_rculock);
>> +
>> +    sr = get_sched_res(cpu);
>> +    data = xmalloc_flex_struct(struct cpu_rm_data, sr, sr->granularity - 1);
>> +    if ( !data )
>> +        goto out;
>> +
>> +    data->old_ops = sr->scheduler;
>> +    data->vpriv_old = idle_vcpu[cpu]->sched_unit->priv;
>> +    data->ppriv_old = sr->sched_priv;
> 
> Repeating a v1 comment:
> 
> "At least from an abstract perspective, doesn't reading fields from
>   sr require the RCU lock to be held continuously (i.e. not dropping
>   it at the end of this function and re-acquiring it in the caller)?"
> 
> Initially I thought you did respond to this in some way, but when
> looking for a matching reply I couldn't find one.

Oh, sorry.

The RCU lock is protecting only the sr, not any data pointers in the sr
are referencing. So it is fine to drop the RCU lock after reading some
of the fields from the sr and storing it in the cpu_rm_data memory.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 1/3] xen/sched: introduce cpupool_update_node_affinity()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2278 bytes --]

On 15.08.22 13:41, Jan Beulich wrote:
> On 15.08.2022 13:04, Juergen Gross wrote:
>> For updating the node affinities of all domains in a cpupool add a new
>> function cpupool_update_node_affinity().
>>
>> In order to avoid multiple allocations of cpumasks carve out memory
>> allocation and freeing from domain_update_node_affinity() into new
>> helpers, which can be used by cpupool_update_node_affinity().
>>
>> Modify domain_update_node_affinity() to take an additional parameter
>> for passing the allocated memory in and to allocate and free the memory
>> via the new helpers in case NULL was passed.
>>
>> This will help later to pre-allocate the cpumasks in order to avoid
>> allocations in stop-machine context.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with the observation that ...
> 
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -1824,9 +1824,28 @@ int vcpu_affinity_domctl(struct domain *d, uint32_t cmd,
>>       return ret;
>>   }
>>   
>> -void domain_update_node_affinity(struct domain *d)
>> +bool update_node_aff_alloc(struct affinity_masks *affinity)
>>   {
>> -    cpumask_var_t dom_cpumask, dom_cpumask_soft;
>> +    if ( !alloc_cpumask_var(&affinity->hard) )
>> +        return false;
>> +    if ( !alloc_cpumask_var(&affinity->soft) )
>> +    {
>> +        free_cpumask_var(affinity->hard);
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +void update_node_aff_free(struct affinity_masks *affinity)
>> +{
>> +    free_cpumask_var(affinity->soft);
>> +    free_cpumask_var(affinity->hard);
>> +}
>> +
>> +void domain_update_node_aff(struct domain *d, struct affinity_masks *affinity)
>> +{
>> +    struct affinity_masks masks = { };
> 
> ... the initializer doesn't really look to be needed here, just like
> you don't have one in cpupool_update_node_affinity(). The one thing
> I'm not sure about is whether old gcc might mis-report a potentially
> uninitialized variable with the initializer dropped ...

Hmm, yes, I think the initializer was needed only in V1.

I guess you could remove it while committing in case no respin of the
series is needed otherwise?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 2/3] xen/sched: carve out memory allocation and freeing from schedule_cpu_rm()

[Jan Beulich]

On 15.08.2022 13:55, Juergen Gross wrote:
> On 15.08.22 13:52, Jan Beulich wrote:
>> On 15.08.2022 13:04, Juergen Gross wrote:
>>> --- a/xen/common/sched/core.c
>>> +++ b/xen/common/sched/core.c
>>> @@ -3237,6 +3237,65 @@ out:
>>>       return ret;
>>>   }
>>>   
>>> +static struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu)
>>> +{
>>> +    struct cpu_rm_data *data;
>>> +    const struct sched_resource *sr;
>>> +    unsigned int idx;
>>> +
>>> +    rcu_read_lock(&sched_res_rculock);
>>> +
>>> +    sr = get_sched_res(cpu);
>>> +    data = xmalloc_flex_struct(struct cpu_rm_data, sr, sr->granularity - 1);
>>> +    if ( !data )
>>> +        goto out;
>>> +
>>> +    data->old_ops = sr->scheduler;
>>> +    data->vpriv_old = idle_vcpu[cpu]->sched_unit->priv;
>>> +    data->ppriv_old = sr->sched_priv;
>>
>> Repeating a v1 comment:
>>
>> "At least from an abstract perspective, doesn't reading fields from
>>   sr require the RCU lock to be held continuously (i.e. not dropping
>>   it at the end of this function and re-acquiring it in the caller)?"
>>
>> Initially I thought you did respond to this in some way, but when
>> looking for a matching reply I couldn't find one.
> 
> Oh, sorry.
> 
> The RCU lock is protecting only the sr, not any data pointers in the sr
> are referencing. So it is fine to drop the RCU lock after reading some
> of the fields from the sr and storing it in the cpu_rm_data memory.

Hmm, interesting. "Protecting only the sr" then means what exactly?
Just its allocation, but not its contents?

Plus it's not just the pointers - sr->granularity also would better not
increase in the meantime ... Quite likely there's a reason why that also
cannot happen, yet even then I think a brief code comment might be
helpful here.

Jan

Re: [PATCH v2 1/3] xen/sched: introduce cpupool_update_node_affinity()

[Jan Beulich]

On 15.08.2022 13:58, Juergen Gross wrote:
> On 15.08.22 13:41, Jan Beulich wrote:
>> On 15.08.2022 13:04, Juergen Gross wrote:
>>> For updating the node affinities of all domains in a cpupool add a new
>>> function cpupool_update_node_affinity().
>>>
>>> In order to avoid multiple allocations of cpumasks carve out memory
>>> allocation and freeing from domain_update_node_affinity() into new
>>> helpers, which can be used by cpupool_update_node_affinity().
>>>
>>> Modify domain_update_node_affinity() to take an additional parameter
>>> for passing the allocated memory in and to allocate and free the memory
>>> via the new helpers in case NULL was passed.
>>>
>>> This will help later to pre-allocate the cpumasks in order to avoid
>>> allocations in stop-machine context.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> with the observation that ...
>>
>>> --- a/xen/common/sched/core.c
>>> +++ b/xen/common/sched/core.c
>>> @@ -1824,9 +1824,28 @@ int vcpu_affinity_domctl(struct domain *d, uint32_t cmd,
>>>       return ret;
>>>   }
>>>   
>>> -void domain_update_node_affinity(struct domain *d)
>>> +bool update_node_aff_alloc(struct affinity_masks *affinity)
>>>   {
>>> -    cpumask_var_t dom_cpumask, dom_cpumask_soft;
>>> +    if ( !alloc_cpumask_var(&affinity->hard) )
>>> +        return false;
>>> +    if ( !alloc_cpumask_var(&affinity->soft) )
>>> +    {
>>> +        free_cpumask_var(affinity->hard);
>>> +        return false;
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>> +
>>> +void update_node_aff_free(struct affinity_masks *affinity)
>>> +{
>>> +    free_cpumask_var(affinity->soft);
>>> +    free_cpumask_var(affinity->hard);
>>> +}
>>> +
>>> +void domain_update_node_aff(struct domain *d, struct affinity_masks *affinity)
>>> +{
>>> +    struct affinity_masks masks = { };
>>
>> ... the initializer doesn't really look to be needed here, just like
>> you don't have one in cpupool_update_node_affinity(). The one thing
>> I'm not sure about is whether old gcc might mis-report a potentially
>> uninitialized variable with the initializer dropped ...
> 
> Hmm, yes, I think the initializer was needed only in V1.
> 
> I guess you could remove it while committing in case no respin of the
> series is needed otherwise?

Sure, I'll take a note.

Jan

Re: [PATCH v2 2/3] xen/sched: carve out memory allocation and freeing from schedule_cpu_rm()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2272 bytes --]

On 15.08.22 14:00, Jan Beulich wrote:
> On 15.08.2022 13:55, Juergen Gross wrote:
>> On 15.08.22 13:52, Jan Beulich wrote:
>>> On 15.08.2022 13:04, Juergen Gross wrote:
>>>> --- a/xen/common/sched/core.c
>>>> +++ b/xen/common/sched/core.c
>>>> @@ -3237,6 +3237,65 @@ out:
>>>>        return ret;
>>>>    }
>>>>    
>>>> +static struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu)
>>>> +{
>>>> +    struct cpu_rm_data *data;
>>>> +    const struct sched_resource *sr;
>>>> +    unsigned int idx;
>>>> +
>>>> +    rcu_read_lock(&sched_res_rculock);
>>>> +
>>>> +    sr = get_sched_res(cpu);
>>>> +    data = xmalloc_flex_struct(struct cpu_rm_data, sr, sr->granularity - 1);
>>>> +    if ( !data )
>>>> +        goto out;
>>>> +
>>>> +    data->old_ops = sr->scheduler;
>>>> +    data->vpriv_old = idle_vcpu[cpu]->sched_unit->priv;
>>>> +    data->ppriv_old = sr->sched_priv;
>>>
>>> Repeating a v1 comment:
>>>
>>> "At least from an abstract perspective, doesn't reading fields from
>>>    sr require the RCU lock to be held continuously (i.e. not dropping
>>>    it at the end of this function and re-acquiring it in the caller)?"
>>>
>>> Initially I thought you did respond to this in some way, but when
>>> looking for a matching reply I couldn't find one.
>>
>> Oh, sorry.
>>
>> The RCU lock is protecting only the sr, not any data pointers in the sr
>> are referencing. So it is fine to drop the RCU lock after reading some
>> of the fields from the sr and storing it in the cpu_rm_data memory.
> 
> Hmm, interesting. "Protecting only the sr" then means what exactly?
> Just its allocation, but not its contents?

Correct.

> Plus it's not just the pointers - sr->granularity also would better not
> increase in the meantime ... Quite likely there's a reason why that also
> cannot happen, yet even then I think a brief code comment might be
> helpful here.

Okay, will add something like:

"Between schedule_cpu_rm_alloc() and the real cpu removal action the relevant
  contents of struct sched_resource can't change, as the cpu in question is
  locked against any other movement to or from cpupools, and the data copied
  by schedule_cpu_rm_alloc() is cpupool specific."

Is that okay?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 3/3] xen/sched: fix cpu hotplug

[Jan Beulich]

On 15.08.2022 13:04, Juergen Gross wrote:
> Cpu cpu unplugging is calling schedule_cpu_rm() via stop_machine_run()
> with interrupts disabled, thus any memory allocation or freeing must
> be avoided.
> 
> Since commit 5047cd1d5dea ("xen/common: Use enhanced
> ASSERT_ALLOC_CONTEXT in xmalloc()") this restriction is being enforced
> via an assertion, which will now fail.
> 
> Before that commit cpu unplugging in normal configurations was working
> just by chance as only the cpu performing schedule_cpu_rm() was doing
> active work. With core scheduling enabled, however, failures could
> result from memory allocations not being properly propagated to other
> cpus' TLBs.
> 
> Fix this mess by allocating needed memory before entering
> stop_machine_run() and freeing any memory only after having finished
> stop_machine_run().
> 
> Fixes: 1ec410112cdd ("xen/sched: support differing granularity in schedule_cpu_[add/rm]()")
> Reported-by: Gao Ruifeng <ruifeng.gao@intel.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v2 2/3] xen/sched: carve out memory allocation and freeing from schedule_cpu_rm()

[Jan Beulich]

On 15.08.2022 14:16, Juergen Gross wrote:
> On 15.08.22 14:00, Jan Beulich wrote:
>> On 15.08.2022 13:55, Juergen Gross wrote:
>>> On 15.08.22 13:52, Jan Beulich wrote:
>>>> On 15.08.2022 13:04, Juergen Gross wrote:
>>>>> --- a/xen/common/sched/core.c
>>>>> +++ b/xen/common/sched/core.c
>>>>> @@ -3237,6 +3237,65 @@ out:
>>>>>        return ret;
>>>>>    }
>>>>>    
>>>>> +static struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu)
>>>>> +{
>>>>> +    struct cpu_rm_data *data;
>>>>> +    const struct sched_resource *sr;
>>>>> +    unsigned int idx;
>>>>> +
>>>>> +    rcu_read_lock(&sched_res_rculock);
>>>>> +
>>>>> +    sr = get_sched_res(cpu);
>>>>> +    data = xmalloc_flex_struct(struct cpu_rm_data, sr, sr->granularity - 1);
>>>>> +    if ( !data )
>>>>> +        goto out;
>>>>> +
>>>>> +    data->old_ops = sr->scheduler;
>>>>> +    data->vpriv_old = idle_vcpu[cpu]->sched_unit->priv;
>>>>> +    data->ppriv_old = sr->sched_priv;
>>>>
>>>> Repeating a v1 comment:
>>>>
>>>> "At least from an abstract perspective, doesn't reading fields from
>>>>    sr require the RCU lock to be held continuously (i.e. not dropping
>>>>    it at the end of this function and re-acquiring it in the caller)?"
>>>>
>>>> Initially I thought you did respond to this in some way, but when
>>>> looking for a matching reply I couldn't find one.
>>>
>>> Oh, sorry.
>>>
>>> The RCU lock is protecting only the sr, not any data pointers in the sr
>>> are referencing. So it is fine to drop the RCU lock after reading some
>>> of the fields from the sr and storing it in the cpu_rm_data memory.
>>
>> Hmm, interesting. "Protecting only the sr" then means what exactly?
>> Just its allocation, but not its contents?
> 
> Correct.
> 
>> Plus it's not just the pointers - sr->granularity also would better not
>> increase in the meantime ... Quite likely there's a reason why that also
>> cannot happen, yet even then I think a brief code comment might be
>> helpful here.
> 
> Okay, will add something like:
> 
> "Between schedule_cpu_rm_alloc() and the real cpu removal action the relevant
>   contents of struct sched_resource can't change, as the cpu in question is
>   locked against any other movement to or from cpupools, and the data copied
>   by schedule_cpu_rm_alloc() is cpupool specific."
> 
> Is that okay?

Well, I guess I need to leave this to the scheduler maintainers then. I
have to admit that it's not clear to me why all of sr->granularity,
sr->scheduler, or sr->sched_priv would be "cpupool specific". I may be
able to agree for sr->granularity, but the other two I thought was
scheduler data, not cpupool data. For sr->granularity in turn (but
perhaps also the other two fields) it's not obvious to me that pool
properties can't change in a racing manner.

Jan

Re: [PATCH v2 2/3] xen/sched: carve out memory allocation and freeing from schedule_cpu_rm()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 3529 bytes --]

On 15.08.22 14:34, Jan Beulich wrote:
> On 15.08.2022 14:16, Juergen Gross wrote:
>> On 15.08.22 14:00, Jan Beulich wrote:
>>> On 15.08.2022 13:55, Juergen Gross wrote:
>>>> On 15.08.22 13:52, Jan Beulich wrote:
>>>>> On 15.08.2022 13:04, Juergen Gross wrote:
>>>>>> --- a/xen/common/sched/core.c
>>>>>> +++ b/xen/common/sched/core.c
>>>>>> @@ -3237,6 +3237,65 @@ out:
>>>>>>         return ret;
>>>>>>     }
>>>>>>     
>>>>>> +static struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu)
>>>>>> +{
>>>>>> +    struct cpu_rm_data *data;
>>>>>> +    const struct sched_resource *sr;
>>>>>> +    unsigned int idx;
>>>>>> +
>>>>>> +    rcu_read_lock(&sched_res_rculock);
>>>>>> +
>>>>>> +    sr = get_sched_res(cpu);
>>>>>> +    data = xmalloc_flex_struct(struct cpu_rm_data, sr, sr->granularity - 1);
>>>>>> +    if ( !data )
>>>>>> +        goto out;
>>>>>> +
>>>>>> +    data->old_ops = sr->scheduler;
>>>>>> +    data->vpriv_old = idle_vcpu[cpu]->sched_unit->priv;
>>>>>> +    data->ppriv_old = sr->sched_priv;
>>>>>
>>>>> Repeating a v1 comment:
>>>>>
>>>>> "At least from an abstract perspective, doesn't reading fields from
>>>>>     sr require the RCU lock to be held continuously (i.e. not dropping
>>>>>     it at the end of this function and re-acquiring it in the caller)?"
>>>>>
>>>>> Initially I thought you did respond to this in some way, but when
>>>>> looking for a matching reply I couldn't find one.
>>>>
>>>> Oh, sorry.
>>>>
>>>> The RCU lock is protecting only the sr, not any data pointers in the sr
>>>> are referencing. So it is fine to drop the RCU lock after reading some
>>>> of the fields from the sr and storing it in the cpu_rm_data memory.
>>>
>>> Hmm, interesting. "Protecting only the sr" then means what exactly?
>>> Just its allocation, but not its contents?
>>
>> Correct.
>>
>>> Plus it's not just the pointers - sr->granularity also would better not
>>> increase in the meantime ... Quite likely there's a reason why that also
>>> cannot happen, yet even then I think a brief code comment might be
>>> helpful here.
>>
>> Okay, will add something like:
>>
>> "Between schedule_cpu_rm_alloc() and the real cpu removal action the relevant
>>    contents of struct sched_resource can't change, as the cpu in question is
>>    locked against any other movement to or from cpupools, and the data copied
>>    by schedule_cpu_rm_alloc() is cpupool specific."
>>
>> Is that okay?
> 
> Well, I guess I need to leave this to the scheduler maintainers then. I
> have to admit that it's not clear to me why all of sr->granularity,
> sr->scheduler, or sr->sched_priv would be "cpupool specific". I may be

sr->scheduler is the pointer to the scheduler ops array which is set when
a cpu is added to a cpupool (the scheduler is a cpupool property). The same
applies to sr->granularity: this value is per-cpupool, too. sr->sched_priv
is only changed when a cpu is added to or removed from a cpupool, as this
is the per-cpu data of a scheduler, which needs to stay when scheduling is
happening on the cpu, thus it is allowed to be removed only in case the
cpu is removed from or added to the cpupool.

> able to agree for sr->granularity, but the other two I thought was
> scheduler data, not cpupool data. For sr->granularity in turn (but
> perhaps also the other two fields) it's not obvious to me that pool
> properties can't change in a racing manner.

They can't. Otherwise the scheduler would explode.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 2/3] xen/sched: carve out memory allocation and freeing from schedule_cpu_rm()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 4180 bytes --]

On 15.08.22 14:47, Juergen Gross wrote:
> On 15.08.22 14:34, Jan Beulich wrote:
>> On 15.08.2022 14:16, Juergen Gross wrote:
>>> On 15.08.22 14:00, Jan Beulich wrote:
>>>> On 15.08.2022 13:55, Juergen Gross wrote:
>>>>> On 15.08.22 13:52, Jan Beulich wrote:
>>>>>> On 15.08.2022 13:04, Juergen Gross wrote:
>>>>>>> --- a/xen/common/sched/core.c
>>>>>>> +++ b/xen/common/sched/core.c
>>>>>>> @@ -3237,6 +3237,65 @@ out:
>>>>>>>         return ret;
>>>>>>>     }
>>>>>>> +static struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu)
>>>>>>> +{
>>>>>>> +    struct cpu_rm_data *data;
>>>>>>> +    const struct sched_resource *sr;
>>>>>>> +    unsigned int idx;
>>>>>>> +
>>>>>>> +    rcu_read_lock(&sched_res_rculock);
>>>>>>> +
>>>>>>> +    sr = get_sched_res(cpu);
>>>>>>> +    data = xmalloc_flex_struct(struct cpu_rm_data, sr, sr->granularity - 
>>>>>>> 1);
>>>>>>> +    if ( !data )
>>>>>>> +        goto out;
>>>>>>> +
>>>>>>> +    data->old_ops = sr->scheduler;
>>>>>>> +    data->vpriv_old = idle_vcpu[cpu]->sched_unit->priv;
>>>>>>> +    data->ppriv_old = sr->sched_priv;
>>>>>>
>>>>>> Repeating a v1 comment:
>>>>>>
>>>>>> "At least from an abstract perspective, doesn't reading fields from
>>>>>>     sr require the RCU lock to be held continuously (i.e. not dropping
>>>>>>     it at the end of this function and re-acquiring it in the caller)?"
>>>>>>
>>>>>> Initially I thought you did respond to this in some way, but when
>>>>>> looking for a matching reply I couldn't find one.
>>>>>
>>>>> Oh, sorry.
>>>>>
>>>>> The RCU lock is protecting only the sr, not any data pointers in the sr
>>>>> are referencing. So it is fine to drop the RCU lock after reading some
>>>>> of the fields from the sr and storing it in the cpu_rm_data memory.
>>>>
>>>> Hmm, interesting. "Protecting only the sr" then means what exactly?
>>>> Just its allocation, but not its contents?
>>>
>>> Correct.
>>>
>>>> Plus it's not just the pointers - sr->granularity also would better not
>>>> increase in the meantime ... Quite likely there's a reason why that also
>>>> cannot happen, yet even then I think a brief code comment might be
>>>> helpful here.
>>>
>>> Okay, will add something like:
>>>
>>> "Between schedule_cpu_rm_alloc() and the real cpu removal action the relevant
>>>    contents of struct sched_resource can't change, as the cpu in question is
>>>    locked against any other movement to or from cpupools, and the data copied
>>>    by schedule_cpu_rm_alloc() is cpupool specific."
>>>
>>> Is that okay?
>>
>> Well, I guess I need to leave this to the scheduler maintainers then. I
>> have to admit that it's not clear to me why all of sr->granularity,
>> sr->scheduler, or sr->sched_priv would be "cpupool specific". I may be
> 
> sr->scheduler is the pointer to the scheduler ops array which is set when
> a cpu is added to a cpupool (the scheduler is a cpupool property). The same
> applies to sr->granularity: this value is per-cpupool, too. sr->sched_priv
> is only changed when a cpu is added to or removed from a cpupool, as this
> is the per-cpu data of a scheduler, which needs to stay when scheduling is
> happening on the cpu, thus it is allowed to be removed only in case the
> cpu is removed from or added to the cpupool.
> 
>> able to agree for sr->granularity, but the other two I thought was
>> scheduler data, not cpupool data. For sr->granularity in turn (but
>> perhaps also the other two fields) it's not obvious to me that pool
>> properties can't change in a racing manner.
> 
> They can't. Otherwise the scheduler would explode.

BTW, I'll rework the comment a little bit to:

"Between schedule_cpu_rm_alloc() and the real cpu removal action the relevant
  contents of struct sched_resource can't change, as the cpu in question is
  locked against any other movement to or from cpupools, and the data copied
  by schedule_cpu_rm_alloc() is modified only in case the cpu in question is
  being moved from or to a cpupool."

Will resend the series tomorrow morning if nobody objects.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]