[PATCH 0/6] Assorted improvements to domain creation

[PATCH 0/6] Assorted improvements to domain creation

[Andrew Cooper]

I started trying to do something unrelated to domain creation, and found this
rats nest.  See individual patches for details.

Andrew Cooper (6):
  xen/domain: Reduce the quantity of initialisation for system domains
  xen/credit2: Move repl_timer into struct csched2_dom
  xen/sched: Improvements to the {alloc,free}_domdata() interfaces
  xen/sched: Remove {init,destroy}_domain() interfaces
  xen/domain: Call sched_destroy_domain() in the domain_create() error path
  xen/domain: Use IS_ERR_OR_NULL() when checking the return value of domain_create()

 xen/arch/arm/mm.c           |  6 ++---
 xen/arch/arm/setup.c        |  2 +-
 xen/arch/x86/mm.c           |  6 ++---
 xen/arch/x86/setup.c        |  2 +-
 xen/common/domain.c         | 66 ++++++++++++++++++++++-----------------------
 xen/common/domctl.c         |  2 +-
 xen/common/sched_arinc653.c | 34 -----------------------
 xen/common/sched_credit.c   | 30 ++-------------------
 xen/common/sched_credit2.c  | 66 +++++++++++----------------------------------
 xen/common/sched_null.c     | 39 +++++++--------------------
 xen/common/sched_rt.c       | 39 +++++++--------------------
 xen/common/schedule.c       | 40 ++++++++++++++++++---------
 xen/include/xen/sched-if.h  | 30 ++++++++++++++++++---
 13 files changed, 132 insertions(+), 230 deletions(-)

-- 
2.1.4


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

[PATCH 1/6] xen/domain: Reduce the quantity of initialisation for system domains

[Andrew Cooper]

 * System domains don't need watchdog initialisation or iomem/irq rangesets,
   and will not plausibly be a xenstore or hardware domain.
 * The idle domain doesn't need scheduler initialisation (and in particular,
   removing this path allows for substantial scheduler cleanup), and isn't
   liable to ever need late_hwdom_init().

Move all of these initialisations pass the DOMCRF_dummy early exit, and into
non-idle paths.  rangeset_domain_initialise() remains because it makes no
allocations, but does initialise a linked list and spinlock.  The poolid
parameter can be dropped as sched_init_domain()'s parameter is now
unconditionally 0.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Dario Faggioli <dfaggioli@suse.com>
---
 xen/common/domain.c | 63 +++++++++++++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 33 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index e1c003d..e0b024c 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -268,7 +268,6 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
     enum { INIT_xsm = 1u<<0, INIT_watchdog = 1u<<1, INIT_rangeset = 1u<<2,
            INIT_evtchn = 1u<<3, INIT_gnttab = 1u<<4, INIT_arch = 1u<<5 };
     int err, init_status = 0;
-    int poolid = CPUPOOLID_NONE;
 
     if ( (d = alloc_domain_struct()) == NULL )
         return ERR_PTR(-ENOMEM);
@@ -283,9 +282,6 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
         goto fail;
     init_status |= INIT_xsm;
 
-    watchdog_domain_init(d);
-    init_status |= INIT_watchdog;
-
     atomic_set(&d->refcnt, 1);
     spin_lock_init_prof(d, domain_lock);
     spin_lock_init_prof(d, page_alloc_lock);
@@ -313,35 +309,38 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
     else
         d->guest_type = guest_type_pv;
 
-    if ( domid == 0 || domid == hardware_domid )
-    {
-        if ( hardware_domid < 0 || hardware_domid >= DOMID_FIRST_RESERVED )
-            panic("The value of hardware_dom must be a valid domain ID");
-        d->is_pinned = opt_dom0_vcpus_pin;
-        d->disable_migrate = 1;
-        old_hwdom = hardware_domain;
-        hardware_domain = d;
-    }
-
-    if ( domcr_flags & DOMCRF_xs_domain )
-    {
-        d->is_xenstore = 1;
-        d->disable_migrate = 1;
-    }
-
     rangeset_domain_initialise(d);
     init_status |= INIT_rangeset;
 
-    d->iomem_caps = rangeset_new(d, "I/O Memory", RANGESETF_prettyprint_hex);
-    d->irq_caps   = rangeset_new(d, "Interrupts", 0);
-    if ( (d->iomem_caps == NULL) || (d->irq_caps == NULL) )
-        goto fail;
-
     if ( domcr_flags & DOMCRF_dummy )
         return d;
 
     if ( !is_idle_domain(d) )
     {
+        watchdog_domain_init(d);
+        init_status |= INIT_watchdog;
+
+        if ( domid == 0 || domid == hardware_domid )
+        {
+            if ( hardware_domid < 0 || hardware_domid >= DOMID_FIRST_RESERVED )
+                panic("The value of hardware_dom must be a valid domain ID");
+            d->is_pinned = opt_dom0_vcpus_pin;
+            d->disable_migrate = 1;
+            old_hwdom = hardware_domain;
+            hardware_domain = d;
+        }
+
+        if ( domcr_flags & DOMCRF_xs_domain )
+        {
+            d->is_xenstore = 1;
+            d->disable_migrate = 1;
+        }
+
+        d->iomem_caps = rangeset_new(d, "I/O Memory", RANGESETF_prettyprint_hex);
+        d->irq_caps   = rangeset_new(d, "Interrupts", 0);
+        if ( !d->iomem_caps || !d->irq_caps )
+            goto fail;
+
         if ( (err = xsm_domain_create(XSM_HOOK, d, ssidref)) != 0 )
             goto fail;
 
@@ -366,8 +365,6 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
             goto fail;
         init_status |= INIT_gnttab;
 
-        poolid = 0;
-
         err = -ENOMEM;
 
         d->pbuf = xzalloc_array(char, DOMAIN_PBUF_SIZE);
@@ -379,14 +376,14 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
         goto fail;
     init_status |= INIT_arch;
 
-    if ( (err = sched_init_domain(d, poolid)) != 0 )
-        goto fail;
-
-    if ( (err = late_hwdom_init(d)) != 0 )
-        goto fail;
-
     if ( !is_idle_domain(d) )
     {
+        if ( (err = sched_init_domain(d, 0)) != 0 )
+            goto fail;
+
+        if ( (err = late_hwdom_init(d)) != 0 )
+            goto fail;
+
         spin_lock(&domlist_update_lock);
         pd = &domain_list; /* NB. domain_list maintained in order of domid. */
         for ( pd = &domain_list; *pd != NULL; pd = &(*pd)->next_in_list )
-- 
2.1.4


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

Re: [PATCH 1/6] xen/domain: Reduce the quantity of initialisation for system domains

[George Dunlap]

On 02/28/2018 02:14 PM, Andrew Cooper wrote:
>  * System domains don't need watchdog initialisation or iomem/irq rangesets,
>    and will not plausibly be a xenstore or hardware domain.
>  * The idle domain doesn't need scheduler initialisation (and in particular,
>    removing this path allows for substantial scheduler cleanup), and isn't
>    liable to ever need late_hwdom_init().
> 
> Move all of these initialisations pass the DOMCRF_dummy early exit, and into
> non-idle paths.  rangeset_domain_initialise() remains because it makes no
> allocations, but does initialise a linked list and spinlock.  The poolid
> parameter can be dropped as sched_init_domain()'s parameter is now
> unconditionally 0.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Looks good:

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

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

Re: [PATCH 1/6] xen/domain: Reduce the quantity of initialisation for system domains

[Dario Faggioli]

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

On Wed, 2018-02-28 at 14:14 +0000, Andrew Cooper wrote:
>  * System domains don't need watchdog initialisation or iomem/irq
> rangesets,
>    and will not plausibly be a xenstore or hardware domain.
>  * The idle domain doesn't need scheduler initialisation (and in
> particular,
>    removing this path allows for substantial scheduler cleanup), and
> isn't
>    liable to ever need late_hwdom_init().
> 
> Move all of these initialisations pass the DOMCRF_dummy early exit,
> and into
> non-idle paths.  rangeset_domain_initialise() remains because it
> makes no
> allocations, but does initialise a linked list and spinlock.  The
> poolid
> parameter can be dropped as sched_init_domain()'s parameter is now
> unconditionally 0.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

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

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

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

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

[PATCH 2/6] xen/credit2: Move repl_timer into struct csched2_dom

[Andrew Cooper]

For exactly the same reason as 418ae6021d.  Having a separate allocation is
unnecessary and wasteful.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Dario Faggioli <dfaggioli@suse.com>
---
 xen/common/sched_credit2.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index ee9768e..b094b3c 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -547,7 +547,7 @@ struct csched2_dom {
     s_time_t tot_budget;        /* Total amount of budget                     */
     s_time_t budget;            /* Currently available budget                 */
 
-    struct timer *repl_timer;   /* Timer for periodic replenishment of budget */
+    struct timer repl_timer;    /* Timer for periodic replenishment of budget */
     s_time_t next_repl;         /* Time at which next replenishment occurs    */
     struct list_head parked_vcpus; /* List of CPUs waiting for budget         */
 
@@ -1974,7 +1974,7 @@ static void replenish_domain_budget(void* data)
     unpark_parked_vcpus(sdom->dom->cpupool->sched, &parked);
 
  out:
-    set_timer(sdom->repl_timer, sdom->next_repl);
+    set_timer(&sdom->repl_timer, sdom->next_repl);
 }
 
 #ifndef NDEBUG
@@ -2874,7 +2874,7 @@ csched2_dom_cntl(
                  */
                 sdom->budget = sdom->tot_budget;
                 sdom->next_repl = NOW() + CSCHED2_BDGT_REPL_PERIOD;
-                set_timer(sdom->repl_timer, sdom->next_repl);
+                set_timer(&sdom->repl_timer, sdom->next_repl);
 
                 /*
                  * Now, let's enable budget accounting for all the vCPUs.
@@ -2933,7 +2933,7 @@ csched2_dom_cntl(
         {
             LIST_HEAD(parked);
 
-            stop_timer(sdom->repl_timer);
+            stop_timer(&sdom->repl_timer);
 
             /* Disable budget accounting for all the vCPUs. */
             for_each_vcpu ( d, v )
@@ -3014,13 +3014,6 @@ csched2_alloc_domdata(const struct scheduler *ops, struct domain *dom)
     if ( sdom == NULL )
         return NULL;
 
-    sdom->repl_timer = xzalloc(struct timer);
-    if ( sdom->repl_timer == NULL )
-    {
-        xfree(sdom);
-        return NULL;
-    }
-
     /* Initialize credit, cap and weight */
     INIT_LIST_HEAD(&sdom->sdom_elem);
     sdom->dom = dom;
@@ -3028,7 +3021,7 @@ csched2_alloc_domdata(const struct scheduler *ops, struct domain *dom)
     sdom->cap = 0U;
     sdom->nr_vcpus = 0;
 
-    init_timer(sdom->repl_timer, replenish_domain_budget, sdom,
+    init_timer(&sdom->repl_timer, replenish_domain_budget, sdom,
                cpumask_any(cpupool_domain_cpumask(dom)));
     spin_lock_init(&sdom->budget_lock);
     INIT_LIST_HEAD(&sdom->parked_vcpus);
@@ -3066,7 +3059,7 @@ csched2_free_domdata(const struct scheduler *ops, void *data)
     struct csched2_dom *sdom = data;
     struct csched2_private *prv = csched2_priv(ops);
 
-    kill_timer(sdom->repl_timer);
+    kill_timer(&sdom->repl_timer);
 
     write_lock_irqsave(&prv->lock, flags);
 
@@ -3074,7 +3067,6 @@ csched2_free_domdata(const struct scheduler *ops, void *data)
 
     write_unlock_irqrestore(&prv->lock, flags);
 
-    xfree(sdom->repl_timer);
     xfree(data);
 }
 
-- 
2.1.4


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

Re: [PATCH 2/6] xen/credit2: Move repl_timer into struct csched2_dom

[George Dunlap]

On 02/28/2018 02:14 PM, Andrew Cooper wrote:
> For exactly the same reason as 418ae6021d.  Having a separate allocation is
> unnecessary and wasteful.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

Re: [PATCH 2/6] xen/credit2: Move repl_timer into struct csched2_dom

[Dario Faggioli]

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

On Wed, 2018-02-28 at 15:26 +0000, George Dunlap wrote:
> On 02/28/2018 02:14 PM, Andrew Cooper wrote:
> > For exactly the same reason as 418ae6021d.  Having a separate
> > allocation is
> > unnecessary and wasteful.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

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

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

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

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

[PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces

[Andrew Cooper]

The main purpose of this change is for the subsequent cleanup it enables, but
it stands on its own merits.

In principle, these hooks are optional, but the SCHED_OP() default aliases a
memory allocation failure, which causes arinc653 to play the dangerous game of
passing its priv pointer back, and remembering not to actually free it.

Redefine alloc_domdata to use ERR_PTR() for errors, NULL for nothing, and
non-NULL for a real allocation, which allows the hook to become properly
optional.  Redefine free_domdata to be idempotent.

For arinc653, this means the dummy hooks can be dropped entirely.  For the
other schedulers, this means returning ERR_PTR(-ENOMEM) instead of NULL for
memory allocation failures, and modifying the free hooks to cope with a NULL
pointer.  While making the alterations, drop some spurious casts to void *.

Introduce and use proper wrappers for sched_{alloc,free}_domdata().  These are
strictly better than SCHED_OP(), as the source code is visible to
grep/cscope/tags, the generated code is better, and there can be proper
per-hook defaults and checks.

Callers of the alloc hooks are switched to using IS_ERR(), rather than
checking for NULL.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Dario Faggioli <dfaggioli@suse.com>
CC: Meng Xu <mengxu@cis.upenn.edu>
CC: Josh Whitehead <josh.whitehead@dornerworks.com>
CC: Robert VanVossen <robert.vanvossen@dornerworks.com>
---
 xen/common/sched_arinc653.c | 31 -------------------------------
 xen/common/sched_credit.c   |  8 ++++----
 xen/common/sched_credit2.c  | 24 +++++++++++++-----------
 xen/common/sched_null.c     | 22 +++++++++++++---------
 xen/common/sched_rt.c       | 21 +++++++++++++--------
 xen/common/schedule.c       | 12 ++++++------
 xen/include/xen/sched-if.h  | 27 ++++++++++++++++++++++++++-
 7 files changed, 75 insertions(+), 70 deletions(-)

diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c
index 982845b..17e765d 100644
--- a/xen/common/sched_arinc653.c
+++ b/xen/common/sched_arinc653.c
@@ -455,34 +455,6 @@ a653sched_free_vdata(const struct scheduler *ops, void *priv)
 }
 
 /**
- * This function allocates scheduler-specific data for a domain
- *
- * We do not actually make use of any per-domain data but the hypervisor
- * expects a non-NULL return value
- *
- * @param ops       Pointer to this instance of the scheduler structure
- *
- * @return          Pointer to the allocated data
- */
-static void *
-a653sched_alloc_domdata(const struct scheduler *ops, struct domain *dom)
-{
-    /* return a non-NULL value to keep schedule.c happy */
-    return SCHED_PRIV(ops);
-}
-
-/**
- * This function frees scheduler-specific data for a domain
- *
- * @param ops       Pointer to this instance of the scheduler structure
- */
-static void
-a653sched_free_domdata(const struct scheduler *ops, void *data)
-{
-    /* nop */
-}
-
-/**
  * Xen scheduler callback function to sleep a VCPU
  *
  * @param ops       Pointer to this instance of the scheduler structure
@@ -740,9 +712,6 @@ static const struct scheduler sched_arinc653_def = {
     .free_vdata     = a653sched_free_vdata,
     .alloc_vdata    = a653sched_alloc_vdata,
 
-    .free_domdata   = a653sched_free_domdata,
-    .alloc_domdata  = a653sched_alloc_domdata,
-
     .init_domain    = NULL,
     .destroy_domain = NULL,
 
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index f8212db..e2133df 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1282,7 +1282,7 @@ csched_alloc_domdata(const struct scheduler *ops, struct domain *dom)
 
     sdom = xzalloc(struct csched_dom);
     if ( sdom == NULL )
-        return NULL;
+        return ERR_PTR(-ENOMEM);
 
     /* Initialize credit and weight */
     INIT_LIST_HEAD(&sdom->active_vcpu);
@@ -1290,7 +1290,7 @@ csched_alloc_domdata(const struct scheduler *ops, struct domain *dom)
     sdom->dom = dom;
     sdom->weight = CSCHED_DEFAULT_WEIGHT;
 
-    return (void *)sdom;
+    return sdom;
 }
 
 static int
@@ -1302,8 +1302,8 @@ csched_dom_init(const struct scheduler *ops, struct domain *dom)
         return 0;
 
     sdom = csched_alloc_domdata(ops, dom);
-    if ( sdom == NULL )
-        return -ENOMEM;
+    if ( IS_ERR(sdom) )
+        return PTR_ERR(sdom);
 
     dom->sched_priv = sdom;
 
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index b094b3c..29a24d6 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -3012,7 +3012,7 @@ csched2_alloc_domdata(const struct scheduler *ops, struct domain *dom)
 
     sdom = xzalloc(struct csched2_dom);
     if ( sdom == NULL )
-        return NULL;
+        return ERR_PTR(-ENOMEM);
 
     /* Initialize credit, cap and weight */
     INIT_LIST_HEAD(&sdom->sdom_elem);
@@ -3032,7 +3032,7 @@ csched2_alloc_domdata(const struct scheduler *ops, struct domain *dom)
 
     write_unlock_irqrestore(&prv->lock, flags);
 
-    return (void *)sdom;
+    return sdom;
 }
 
 static int
@@ -3044,8 +3044,8 @@ csched2_dom_init(const struct scheduler *ops, struct domain *dom)
         return 0;
 
     sdom = csched2_alloc_domdata(ops, dom);
-    if ( sdom == NULL )
-        return -ENOMEM;
+    if ( IS_ERR(sdom) )
+        return PTR_ERR(sdom);
 
     dom->sched_priv = sdom;
 
@@ -3055,19 +3055,21 @@ csched2_dom_init(const struct scheduler *ops, struct domain *dom)
 static void
 csched2_free_domdata(const struct scheduler *ops, void *data)
 {
-    unsigned long flags;
     struct csched2_dom *sdom = data;
     struct csched2_private *prv = csched2_priv(ops);
 
-    kill_timer(&sdom->repl_timer);
-
-    write_lock_irqsave(&prv->lock, flags);
+    if ( sdom )
+    {
+        unsigned long flags;
 
-    list_del_init(&sdom->sdom_elem);
+        kill_timer(&sdom->repl_timer);
 
-    write_unlock_irqrestore(&prv->lock, flags);
+        write_lock_irqsave(&prv->lock, flags);
+        list_del_init(&sdom->sdom_elem);
+        write_unlock_irqrestore(&prv->lock, flags);
 
-    xfree(data);
+        xfree(sdom);
+    }
 }
 
 static void
diff --git a/xen/common/sched_null.c b/xen/common/sched_null.c
index b4a24ba..4dd405b 100644
--- a/xen/common/sched_null.c
+++ b/xen/common/sched_null.c
@@ -231,7 +231,7 @@ static void * null_alloc_domdata(const struct scheduler *ops,
 
     ndom = xzalloc(struct null_dom);
     if ( ndom == NULL )
-        return NULL;
+        return ERR_PTR(-ENOMEM);
 
     ndom->dom = d;
 
@@ -239,20 +239,24 @@ static void * null_alloc_domdata(const struct scheduler *ops,
     list_add_tail(&ndom->ndom_elem, &null_priv(ops)->ndom);
     spin_unlock_irqrestore(&prv->lock, flags);
 
-    return (void*)ndom;
+    return ndom;
 }
 
 static void null_free_domdata(const struct scheduler *ops, void *data)
 {
-    unsigned long flags;
     struct null_dom *ndom = data;
     struct null_private *prv = null_priv(ops);
 
-    spin_lock_irqsave(&prv->lock, flags);
-    list_del_init(&ndom->ndom_elem);
-    spin_unlock_irqrestore(&prv->lock, flags);
+    if ( ndom )
+    {
+        unsigned long flags;
+
+        spin_lock_irqsave(&prv->lock, flags);
+        list_del_init(&ndom->ndom_elem);
+        spin_unlock_irqrestore(&prv->lock, flags);
 
-    xfree(data);
+        xfree(ndom);
+    }
 }
 
 static int null_dom_init(const struct scheduler *ops, struct domain *d)
@@ -263,8 +267,8 @@ static int null_dom_init(const struct scheduler *ops, struct domain *d)
         return 0;
 
     ndom = null_alloc_domdata(ops, d);
-    if ( ndom == NULL )
-        return -ENOMEM;
+    if ( IS_ERR(ndom) )
+        return PTR_ERR(ndom);
 
     d->sched_priv = ndom;
 
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index a202802..e4ff5c1 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -820,7 +820,7 @@ rt_alloc_domdata(const struct scheduler *ops, struct domain *dom)
 
     sdom = xzalloc(struct rt_dom);
     if ( sdom == NULL )
-        return NULL;
+        return ERR_PTR(-ENOMEM);
 
     INIT_LIST_HEAD(&sdom->sdom_elem);
     sdom->dom = dom;
@@ -836,14 +836,19 @@ rt_alloc_domdata(const struct scheduler *ops, struct domain *dom)
 static void
 rt_free_domdata(const struct scheduler *ops, void *data)
 {
-    unsigned long flags;
     struct rt_dom *sdom = data;
     struct rt_private *prv = rt_priv(ops);
 
-    spin_lock_irqsave(&prv->lock, flags);
-    list_del_init(&sdom->sdom_elem);
-    spin_unlock_irqrestore(&prv->lock, flags);
-    xfree(data);
+    if ( sdom )
+    {
+        unsigned long flags;
+
+        spin_lock_irqsave(&prv->lock, flags);
+        list_del_init(&sdom->sdom_elem);
+        spin_unlock_irqrestore(&prv->lock, flags);
+
+        xfree(sdom);
+    }
 }
 
 static int
@@ -856,8 +861,8 @@ rt_dom_init(const struct scheduler *ops, struct domain *dom)
         return 0;
 
     sdom = rt_alloc_domdata(ops, dom);
-    if ( sdom == NULL )
-        return -ENOMEM;
+    if ( IS_ERR(sdom) )
+        return PTR_ERR(sdom);
 
     dom->sched_priv = sdom;
 
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index b788426..08a31b6 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -318,14 +318,14 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
             return -EBUSY;
     }
 
-    domdata = SCHED_OP(c->sched, alloc_domdata, d);
-    if ( domdata == NULL )
-        return -ENOMEM;
+    domdata = sched_alloc_domdata(c->sched, d);
+    if ( IS_ERR(domdata) )
+        return PTR_ERR(domdata);
 
     vcpu_priv = xzalloc_array(void *, d->max_vcpus);
     if ( vcpu_priv == NULL )
     {
-        SCHED_OP(c->sched, free_domdata, domdata);
+        sched_free_domdata(c->sched, domdata);
         return -ENOMEM;
     }
 
@@ -337,7 +337,7 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
             for_each_vcpu ( d, v )
                 xfree(vcpu_priv[v->vcpu_id]);
             xfree(vcpu_priv);
-            SCHED_OP(c->sched, free_domdata, domdata);
+            sched_free_domdata(c->sched, domdata);
             return -ENOMEM;
         }
     }
@@ -393,7 +393,7 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
 
     domain_unpause(d);
 
-    SCHED_OP(old_ops, free_domdata, old_domdata);
+    sched_free_domdata(old_ops, old_domdata);
 
     xfree(vcpu_priv);
 
diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
index c4a4935..56e7d0c 100644
--- a/xen/include/xen/sched-if.h
+++ b/xen/include/xen/sched-if.h
@@ -146,8 +146,11 @@ struct scheduler {
     void *       (*alloc_pdata)    (const struct scheduler *, int);
     void         (*init_pdata)     (const struct scheduler *, void *, int);
     void         (*deinit_pdata)   (const struct scheduler *, void *, int);
-    void         (*free_domdata)   (const struct scheduler *, void *);
+
+    /* Returns ERR_PTR(-err) for error, NULL for 'nothing needed'. */
     void *       (*alloc_domdata)  (const struct scheduler *, struct domain *);
+    /* Idempotent. */
+    void         (*free_domdata)   (const struct scheduler *, void *);
 
     void         (*switch_sched)   (struct scheduler *, unsigned int,
                                     void *, void *);
@@ -181,6 +184,28 @@ struct scheduler {
     void         (*tick_resume)     (const struct scheduler *, unsigned int);
 };
 
+static inline void *sched_alloc_domdata(const struct scheduler *s,
+                                        struct domain *d)
+{
+    if ( s->alloc_domdata )
+        return s->alloc_domdata(s, d);
+    else
+        return NULL;
+}
+
+static inline void sched_free_domdata(const struct scheduler *s,
+                                      void *data)
+{
+    if ( s->free_domdata )
+        s->free_domdata(s, data);
+    else
+        /*
+         * Check that if there isn't a free_domdata hook, we haven't got any
+         * data we're expected to deal with.
+         */
+        ASSERT(!data);
+}
+
 #define REGISTER_SCHEDULER(x) static const struct scheduler *x##_entry \
   __used_section(".data.schedulers") = &x;
 
-- 
2.1.4


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

Re: [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces

[George Dunlap]

On 02/28/2018 02:14 PM, Andrew Cooper wrote:
> The main purpose of this change is for the subsequent cleanup it enables, but
> it stands on its own merits.
> 
> In principle, these hooks are optional, but the SCHED_OP() default aliases a
> memory allocation failure, which causes arinc653 to play the dangerous game of
> passing its priv pointer back, and remembering not to actually free it.
> 
> Redefine alloc_domdata to use ERR_PTR() for errors, NULL for nothing, and
> non-NULL for a real allocation, which allows the hook to become properly
> optional.  Redefine free_domdata to be idempotent.
> 
> For arinc653, this means the dummy hooks can be dropped entirely.  For the
> other schedulers, this means returning ERR_PTR(-ENOMEM) instead of NULL for
> memory allocation failures, and modifying the free hooks to cope with a NULL
> pointer.  While making the alterations, drop some spurious casts to void *.
> 
> Introduce and use proper wrappers for sched_{alloc,free}_domdata().  These are
> strictly better than SCHED_OP(), as the source code is visible to
> grep/cscope/tags, the generated code is better, and there can be proper
> per-hook defaults and checks.
> 
> Callers of the alloc hooks are switched to using IS_ERR(), rather than
> checking for NULL.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

Re: [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces

[Meng Xu]

On Wed, Feb 28, 2018 at 9:14 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> The main purpose of this change is for the subsequent cleanup it enables, but
> it stands on its own merits.
>
> In principle, these hooks are optional, but the SCHED_OP() default aliases a
> memory allocation failure, which causes arinc653 to play the dangerous game of
> passing its priv pointer back, and remembering not to actually free it.
>
> Redefine alloc_domdata to use ERR_PTR() for errors, NULL for nothing, and
> non-NULL for a real allocation, which allows the hook to become properly
> optional.  Redefine free_domdata to be idempotent.
>
> For arinc653, this means the dummy hooks can be dropped entirely.  For the
> other schedulers, this means returning ERR_PTR(-ENOMEM) instead of NULL for
> memory allocation failures, and modifying the free hooks to cope with a NULL
> pointer.  While making the alterations, drop some spurious casts to void *.
>
> Introduce and use proper wrappers for sched_{alloc,free}_domdata().  These are
> strictly better than SCHED_OP(), as the source code is visible to
> grep/cscope/tags, the generated code is better, and there can be proper
> per-hook defaults and checks.
>
> Callers of the alloc hooks are switched to using IS_ERR(), rather than
> checking for NULL.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---


Acked-by: Meng Xu <mengxu@cis.upenn.edu>


Thanks,


Meng

-----------
Meng Xu
Ph.D. Candidate in Computer and Information Science
University of Pennsylvania
http://www.cis.upenn.edu/~mengxu/

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

Re: [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces

[Dario Faggioli]

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

On Wed, 2018-02-28 at 14:14 +0000, Andrew Cooper wrote:
> The main purpose of this change is for the subsequent cleanup it
> enables, but
> it stands on its own merits.
> 
> In principle, these hooks are optional, but the SCHED_OP() default
> aliases a
> memory allocation failure, which causes arinc653 to play the
> dangerous game of
> passing its priv pointer back, and remembering not to actually free
> it.
> 
> Redefine alloc_domdata to use ERR_PTR() for errors, NULL for nothing,
> and
> non-NULL for a real allocation, which allows the hook to become
> properly
> optional.  Redefine free_domdata to be idempotent.
> 
> For arinc653, this means the dummy hooks can be dropped
> entirely.  For the
> other schedulers, this means returning ERR_PTR(-ENOMEM) instead of
> NULL for
> memory allocation failures, and modifying the free hooks to cope with
> a NULL
> pointer.  While making the alterations, drop some spurious casts to
> void *.
> 
> Introduce and use proper wrappers for
> sched_{alloc,free}_domdata().  These are
> strictly better than SCHED_OP(), as the source code is visible to
> grep/cscope/tags, the generated code is better, and there can be
> proper
> per-hook defaults and checks.
> 
> Callers of the alloc hooks are switched to using IS_ERR(), rather
> than
> checking for NULL.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

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

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

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

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

Re: [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces

[Andrew Cooper]

Ping Dornerworks.

Any comment (on this and the next patch), or are you happy for these
trivial changes to fall under general scheduler maintainership?

~Andrew

On 28/02/18 14:14, Andrew Cooper wrote:
> The main purpose of this change is for the subsequent cleanup it enables, but
> it stands on its own merits.
>
> In principle, these hooks are optional, but the SCHED_OP() default aliases a
> memory allocation failure, which causes arinc653 to play the dangerous game of
> passing its priv pointer back, and remembering not to actually free it.
>
> Redefine alloc_domdata to use ERR_PTR() for errors, NULL for nothing, and
> non-NULL for a real allocation, which allows the hook to become properly
> optional.  Redefine free_domdata to be idempotent.
>
> For arinc653, this means the dummy hooks can be dropped entirely.  For the
> other schedulers, this means returning ERR_PTR(-ENOMEM) instead of NULL for
> memory allocation failures, and modifying the free hooks to cope with a NULL
> pointer.  While making the alterations, drop some spurious casts to void *.
>
> Introduce and use proper wrappers for sched_{alloc,free}_domdata().  These are
> strictly better than SCHED_OP(), as the source code is visible to
> grep/cscope/tags, the generated code is better, and there can be proper
> per-hook defaults and checks.
>
> Callers of the alloc hooks are switched to using IS_ERR(), rather than
> checking for NULL.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Dario Faggioli <dfaggioli@suse.com>
> CC: Meng Xu <mengxu@cis.upenn.edu>
> CC: Josh Whitehead <josh.whitehead@dornerworks.com>
> CC: Robert VanVossen <robert.vanvossen@dornerworks.com>
> ---
>  xen/common/sched_arinc653.c | 31 -------------------------------
>  xen/common/sched_credit.c   |  8 ++++----
>  xen/common/sched_credit2.c  | 24 +++++++++++++-----------
>  xen/common/sched_null.c     | 22 +++++++++++++---------
>  xen/common/sched_rt.c       | 21 +++++++++++++--------
>  xen/common/schedule.c       | 12 ++++++------
>  xen/include/xen/sched-if.h  | 27 ++++++++++++++++++++++++++-
>  7 files changed, 75 insertions(+), 70 deletions(-)
>
> diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c
> index 982845b..17e765d 100644
> --- a/xen/common/sched_arinc653.c
> +++ b/xen/common/sched_arinc653.c
> @@ -455,34 +455,6 @@ a653sched_free_vdata(const struct scheduler *ops, void *priv)
>  }
>  
>  /**
> - * This function allocates scheduler-specific data for a domain
> - *
> - * We do not actually make use of any per-domain data but the hypervisor
> - * expects a non-NULL return value
> - *
> - * @param ops       Pointer to this instance of the scheduler structure
> - *
> - * @return          Pointer to the allocated data
> - */
> -static void *
> -a653sched_alloc_domdata(const struct scheduler *ops, struct domain *dom)
> -{
> -    /* return a non-NULL value to keep schedule.c happy */
> -    return SCHED_PRIV(ops);
> -}
> -
> -/**
> - * This function frees scheduler-specific data for a domain
> - *
> - * @param ops       Pointer to this instance of the scheduler structure
> - */
> -static void
> -a653sched_free_domdata(const struct scheduler *ops, void *data)
> -{
> -    /* nop */
> -}
> -
> -/**
>   * Xen scheduler callback function to sleep a VCPU
>   *
>   * @param ops       Pointer to this instance of the scheduler structure
> @@ -740,9 +712,6 @@ static const struct scheduler sched_arinc653_def = {
>      .free_vdata     = a653sched_free_vdata,
>      .alloc_vdata    = a653sched_alloc_vdata,
>  
> -    .free_domdata   = a653sched_free_domdata,
> -    .alloc_domdata  = a653sched_alloc_domdata,
> -
>      .init_domain    = NULL,
>      .destroy_domain = NULL,
>  
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index f8212db..e2133df 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -1282,7 +1282,7 @@ csched_alloc_domdata(const struct scheduler *ops, struct domain *dom)
>  
>      sdom = xzalloc(struct csched_dom);
>      if ( sdom == NULL )
> -        return NULL;
> +        return ERR_PTR(-ENOMEM);
>  
>      /* Initialize credit and weight */
>      INIT_LIST_HEAD(&sdom->active_vcpu);
> @@ -1290,7 +1290,7 @@ csched_alloc_domdata(const struct scheduler *ops, struct domain *dom)
>      sdom->dom = dom;
>      sdom->weight = CSCHED_DEFAULT_WEIGHT;
>  
> -    return (void *)sdom;
> +    return sdom;
>  }
>  
>  static int
> @@ -1302,8 +1302,8 @@ csched_dom_init(const struct scheduler *ops, struct domain *dom)
>          return 0;
>  
>      sdom = csched_alloc_domdata(ops, dom);
> -    if ( sdom == NULL )
> -        return -ENOMEM;
> +    if ( IS_ERR(sdom) )
> +        return PTR_ERR(sdom);
>  
>      dom->sched_priv = sdom;
>  
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index b094b3c..29a24d6 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -3012,7 +3012,7 @@ csched2_alloc_domdata(const struct scheduler *ops, struct domain *dom)
>  
>      sdom = xzalloc(struct csched2_dom);
>      if ( sdom == NULL )
> -        return NULL;
> +        return ERR_PTR(-ENOMEM);
>  
>      /* Initialize credit, cap and weight */
>      INIT_LIST_HEAD(&sdom->sdom_elem);
> @@ -3032,7 +3032,7 @@ csched2_alloc_domdata(const struct scheduler *ops, struct domain *dom)
>  
>      write_unlock_irqrestore(&prv->lock, flags);
>  
> -    return (void *)sdom;
> +    return sdom;
>  }
>  
>  static int
> @@ -3044,8 +3044,8 @@ csched2_dom_init(const struct scheduler *ops, struct domain *dom)
>          return 0;
>  
>      sdom = csched2_alloc_domdata(ops, dom);
> -    if ( sdom == NULL )
> -        return -ENOMEM;
> +    if ( IS_ERR(sdom) )
> +        return PTR_ERR(sdom);
>  
>      dom->sched_priv = sdom;
>  
> @@ -3055,19 +3055,21 @@ csched2_dom_init(const struct scheduler *ops, struct domain *dom)
>  static void
>  csched2_free_domdata(const struct scheduler *ops, void *data)
>  {
> -    unsigned long flags;
>      struct csched2_dom *sdom = data;
>      struct csched2_private *prv = csched2_priv(ops);
>  
> -    kill_timer(&sdom->repl_timer);
> -
> -    write_lock_irqsave(&prv->lock, flags);
> +    if ( sdom )
> +    {
> +        unsigned long flags;
>  
> -    list_del_init(&sdom->sdom_elem);
> +        kill_timer(&sdom->repl_timer);
>  
> -    write_unlock_irqrestore(&prv->lock, flags);
> +        write_lock_irqsave(&prv->lock, flags);
> +        list_del_init(&sdom->sdom_elem);
> +        write_unlock_irqrestore(&prv->lock, flags);
>  
> -    xfree(data);
> +        xfree(sdom);
> +    }
>  }
>  
>  static void
> diff --git a/xen/common/sched_null.c b/xen/common/sched_null.c
> index b4a24ba..4dd405b 100644
> --- a/xen/common/sched_null.c
> +++ b/xen/common/sched_null.c
> @@ -231,7 +231,7 @@ static void * null_alloc_domdata(const struct scheduler *ops,
>  
>      ndom = xzalloc(struct null_dom);
>      if ( ndom == NULL )
> -        return NULL;
> +        return ERR_PTR(-ENOMEM);
>  
>      ndom->dom = d;
>  
> @@ -239,20 +239,24 @@ static void * null_alloc_domdata(const struct scheduler *ops,
>      list_add_tail(&ndom->ndom_elem, &null_priv(ops)->ndom);
>      spin_unlock_irqrestore(&prv->lock, flags);
>  
> -    return (void*)ndom;
> +    return ndom;
>  }
>  
>  static void null_free_domdata(const struct scheduler *ops, void *data)
>  {
> -    unsigned long flags;
>      struct null_dom *ndom = data;
>      struct null_private *prv = null_priv(ops);
>  
> -    spin_lock_irqsave(&prv->lock, flags);
> -    list_del_init(&ndom->ndom_elem);
> -    spin_unlock_irqrestore(&prv->lock, flags);
> +    if ( ndom )
> +    {
> +        unsigned long flags;
> +
> +        spin_lock_irqsave(&prv->lock, flags);
> +        list_del_init(&ndom->ndom_elem);
> +        spin_unlock_irqrestore(&prv->lock, flags);
>  
> -    xfree(data);
> +        xfree(ndom);
> +    }
>  }
>  
>  static int null_dom_init(const struct scheduler *ops, struct domain *d)
> @@ -263,8 +267,8 @@ static int null_dom_init(const struct scheduler *ops, struct domain *d)
>          return 0;
>  
>      ndom = null_alloc_domdata(ops, d);
> -    if ( ndom == NULL )
> -        return -ENOMEM;
> +    if ( IS_ERR(ndom) )
> +        return PTR_ERR(ndom);
>  
>      d->sched_priv = ndom;
>  
> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> index a202802..e4ff5c1 100644
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -820,7 +820,7 @@ rt_alloc_domdata(const struct scheduler *ops, struct domain *dom)
>  
>      sdom = xzalloc(struct rt_dom);
>      if ( sdom == NULL )
> -        return NULL;
> +        return ERR_PTR(-ENOMEM);
>  
>      INIT_LIST_HEAD(&sdom->sdom_elem);
>      sdom->dom = dom;
> @@ -836,14 +836,19 @@ rt_alloc_domdata(const struct scheduler *ops, struct domain *dom)
>  static void
>  rt_free_domdata(const struct scheduler *ops, void *data)
>  {
> -    unsigned long flags;
>      struct rt_dom *sdom = data;
>      struct rt_private *prv = rt_priv(ops);
>  
> -    spin_lock_irqsave(&prv->lock, flags);
> -    list_del_init(&sdom->sdom_elem);
> -    spin_unlock_irqrestore(&prv->lock, flags);
> -    xfree(data);
> +    if ( sdom )
> +    {
> +        unsigned long flags;
> +
> +        spin_lock_irqsave(&prv->lock, flags);
> +        list_del_init(&sdom->sdom_elem);
> +        spin_unlock_irqrestore(&prv->lock, flags);
> +
> +        xfree(sdom);
> +    }
>  }
>  
>  static int
> @@ -856,8 +861,8 @@ rt_dom_init(const struct scheduler *ops, struct domain *dom)
>          return 0;
>  
>      sdom = rt_alloc_domdata(ops, dom);
> -    if ( sdom == NULL )
> -        return -ENOMEM;
> +    if ( IS_ERR(sdom) )
> +        return PTR_ERR(sdom);
>  
>      dom->sched_priv = sdom;
>  
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index b788426..08a31b6 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -318,14 +318,14 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
>              return -EBUSY;
>      }
>  
> -    domdata = SCHED_OP(c->sched, alloc_domdata, d);
> -    if ( domdata == NULL )
> -        return -ENOMEM;
> +    domdata = sched_alloc_domdata(c->sched, d);
> +    if ( IS_ERR(domdata) )
> +        return PTR_ERR(domdata);
>  
>      vcpu_priv = xzalloc_array(void *, d->max_vcpus);
>      if ( vcpu_priv == NULL )
>      {
> -        SCHED_OP(c->sched, free_domdata, domdata);
> +        sched_free_domdata(c->sched, domdata);
>          return -ENOMEM;
>      }
>  
> @@ -337,7 +337,7 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
>              for_each_vcpu ( d, v )
>                  xfree(vcpu_priv[v->vcpu_id]);
>              xfree(vcpu_priv);
> -            SCHED_OP(c->sched, free_domdata, domdata);
> +            sched_free_domdata(c->sched, domdata);
>              return -ENOMEM;
>          }
>      }
> @@ -393,7 +393,7 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
>  
>      domain_unpause(d);
>  
> -    SCHED_OP(old_ops, free_domdata, old_domdata);
> +    sched_free_domdata(old_ops, old_domdata);
>  
>      xfree(vcpu_priv);
>  
> diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
> index c4a4935..56e7d0c 100644
> --- a/xen/include/xen/sched-if.h
> +++ b/xen/include/xen/sched-if.h
> @@ -146,8 +146,11 @@ struct scheduler {
>      void *       (*alloc_pdata)    (const struct scheduler *, int);
>      void         (*init_pdata)     (const struct scheduler *, void *, int);
>      void         (*deinit_pdata)   (const struct scheduler *, void *, int);
> -    void         (*free_domdata)   (const struct scheduler *, void *);
> +
> +    /* Returns ERR_PTR(-err) for error, NULL for 'nothing needed'. */
>      void *       (*alloc_domdata)  (const struct scheduler *, struct domain *);
> +    /* Idempotent. */
> +    void         (*free_domdata)   (const struct scheduler *, void *);
>  
>      void         (*switch_sched)   (struct scheduler *, unsigned int,
>                                      void *, void *);
> @@ -181,6 +184,28 @@ struct scheduler {
>      void         (*tick_resume)     (const struct scheduler *, unsigned int);
>  };
>  
> +static inline void *sched_alloc_domdata(const struct scheduler *s,
> +                                        struct domain *d)
> +{
> +    if ( s->alloc_domdata )
> +        return s->alloc_domdata(s, d);
> +    else
> +        return NULL;
> +}
> +
> +static inline void sched_free_domdata(const struct scheduler *s,
> +                                      void *data)
> +{
> +    if ( s->free_domdata )
> +        s->free_domdata(s, data);
> +    else
> +        /*
> +         * Check that if there isn't a free_domdata hook, we haven't got any
> +         * data we're expected to deal with.
> +         */
> +        ASSERT(!data);
> +}
> +
>  #define REGISTER_SCHEDULER(x) static const struct scheduler *x##_entry \
>    __used_section(".data.schedulers") = &x;
>  


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

[PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces

[Andrew Cooper]

These hooks have one single caller (sched_{init,destroy}_domain()
respectively) and are all identical (when implemented).

Previous changes have ensured that only real domains reach these functions, so
ASSERT() that system domains are not seen. Call sched_{alloc,free}_domdata()
directly, and handle d->sched_priv directly.

The net diffstat is:
  add/remove: 0/8 grow/shrink: 1/7 up/down: 7/-335 (-328)
  function                                     old     new   delta
  sched_destroy_domain                         130     137      +7
  sched_init_domain                            138     137      -1
  rt_dom_destroy                                 6       -      -6
  null_dom_destroy                               6       -      -6
  csched_dom_destroy                             9       -      -9
  csched2_dom_destroy                            9       -      -9
  sched_rtds_def                               264     248     -16
  sched_null_def                               264     248     -16
  sched_credit_def                             264     248     -16
  sched_credit2_def                            264     248     -16
  sched_arinc653_def                           264     248     -16
  ops                                          264     248     -16
  rt_dom_init                                   52       -     -52
  null_dom_init                                 52       -     -52
  csched_dom_init                               52       -     -52
  csched2_dom_init                              52       -     -52

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Dario Faggioli <dfaggioli@suse.com>
CC: Meng Xu <mengxu@cis.upenn.edu>
CC: Josh Whitehead <josh.whitehead@dornerworks.com>
CC: Robert VanVossen <robert.vanvossen@dornerworks.com>
---
 xen/common/sched_arinc653.c |  3 ---
 xen/common/sched_credit.c   | 26 --------------------------
 xen/common/sched_credit2.c  | 28 ----------------------------
 xen/common/sched_null.c     | 23 -----------------------
 xen/common/sched_rt.c       | 26 --------------------------
 xen/common/schedule.c       | 16 ++++++++++++++--
 xen/include/xen/sched-if.h  |  3 ---
 7 files changed, 14 insertions(+), 111 deletions(-)

diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c
index 17e765d..a4c6d00 100644
--- a/xen/common/sched_arinc653.c
+++ b/xen/common/sched_arinc653.c
@@ -712,9 +712,6 @@ static const struct scheduler sched_arinc653_def = {
     .free_vdata     = a653sched_free_vdata,
     .alloc_vdata    = a653sched_alloc_vdata,
 
-    .init_domain    = NULL,
-    .destroy_domain = NULL,
-
     .insert_vcpu    = NULL,
     .remove_vcpu    = NULL,
 
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index e2133df..0178ff5 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1293,35 +1293,12 @@ csched_alloc_domdata(const struct scheduler *ops, struct domain *dom)
     return sdom;
 }
 
-static int
-csched_dom_init(const struct scheduler *ops, struct domain *dom)
-{
-    struct csched_dom *sdom;
-
-    if ( is_idle_domain(dom) )
-        return 0;
-
-    sdom = csched_alloc_domdata(ops, dom);
-    if ( IS_ERR(sdom) )
-        return PTR_ERR(sdom);
-
-    dom->sched_priv = sdom;
-
-    return 0;
-}
-
 static void
 csched_free_domdata(const struct scheduler *ops, void *data)
 {
     xfree(data);
 }
 
-static void
-csched_dom_destroy(const struct scheduler *ops, struct domain *dom)
-{
-    csched_free_domdata(ops, CSCHED_DOM(dom));
-}
-
 /*
  * This is a O(n) optimized sort of the runq.
  *
@@ -2259,9 +2236,6 @@ static const struct scheduler sched_credit_def = {
     .sched_id       = XEN_SCHEDULER_CREDIT,
     .sched_data     = NULL,
 
-    .init_domain    = csched_dom_init,
-    .destroy_domain = csched_dom_destroy,
-
     .insert_vcpu    = csched_vcpu_insert,
     .remove_vcpu    = csched_vcpu_remove,
 
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 29a24d6..5a635e8 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -3035,23 +3035,6 @@ csched2_alloc_domdata(const struct scheduler *ops, struct domain *dom)
     return sdom;
 }
 
-static int
-csched2_dom_init(const struct scheduler *ops, struct domain *dom)
-{
-    struct csched2_dom *sdom;
-
-    if ( is_idle_domain(dom) )
-        return 0;
-
-    sdom = csched2_alloc_domdata(ops, dom);
-    if ( IS_ERR(sdom) )
-        return PTR_ERR(sdom);
-
-    dom->sched_priv = sdom;
-
-    return 0;
-}
-
 static void
 csched2_free_domdata(const struct scheduler *ops, void *data)
 {
@@ -3073,14 +3056,6 @@ csched2_free_domdata(const struct scheduler *ops, void *data)
 }
 
 static void
-csched2_dom_destroy(const struct scheduler *ops, struct domain *dom)
-{
-    ASSERT(csched2_dom(dom)->nr_vcpus == 0);
-
-    csched2_free_domdata(ops, csched2_dom(dom));
-}
-
-static void
 csched2_vcpu_insert(const struct scheduler *ops, struct vcpu *vc)
 {
     struct csched2_vcpu *svc = vc->sched_priv;
@@ -4016,9 +3991,6 @@ static const struct scheduler sched_credit2_def = {
     .sched_id       = XEN_SCHEDULER_CREDIT2,
     .sched_data     = NULL,
 
-    .init_domain    = csched2_dom_init,
-    .destroy_domain = csched2_dom_destroy,
-
     .insert_vcpu    = csched2_vcpu_insert,
     .remove_vcpu    = csched2_vcpu_remove,
 
diff --git a/xen/common/sched_null.c b/xen/common/sched_null.c
index 4dd405b..58e306a 100644
--- a/xen/common/sched_null.c
+++ b/xen/common/sched_null.c
@@ -259,26 +259,6 @@ static void null_free_domdata(const struct scheduler *ops, void *data)
     }
 }
 
-static int null_dom_init(const struct scheduler *ops, struct domain *d)
-{
-    struct null_dom *ndom;
-
-    if ( is_idle_domain(d) )
-        return 0;
-
-    ndom = null_alloc_domdata(ops, d);
-    if ( IS_ERR(ndom) )
-        return PTR_ERR(ndom);
-
-    d->sched_priv = ndom;
-
-    return 0;
-}
-static void null_dom_destroy(const struct scheduler *ops, struct domain *d)
-{
-    null_free_domdata(ops, null_dom(d));
-}
-
 /*
  * vCPU to pCPU assignment and placement. This _only_ happens:
  *  - on insert,
@@ -923,9 +903,6 @@ const struct scheduler sched_null_def = {
     .alloc_domdata  = null_alloc_domdata,
     .free_domdata   = null_free_domdata,
 
-    .init_domain    = null_dom_init,
-    .destroy_domain = null_dom_destroy,
-
     .insert_vcpu    = null_vcpu_insert,
     .remove_vcpu    = null_vcpu_remove,
 
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index e4ff5c1..1c72ea8 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -851,30 +851,6 @@ rt_free_domdata(const struct scheduler *ops, void *data)
     }
 }
 
-static int
-rt_dom_init(const struct scheduler *ops, struct domain *dom)
-{
-    struct rt_dom *sdom;
-
-    /* IDLE Domain does not link on rt_private */
-    if ( is_idle_domain(dom) )
-        return 0;
-
-    sdom = rt_alloc_domdata(ops, dom);
-    if ( IS_ERR(sdom) )
-        return PTR_ERR(sdom);
-
-    dom->sched_priv = sdom;
-
-    return 0;
-}
-
-static void
-rt_dom_destroy(const struct scheduler *ops, struct domain *dom)
-{
-    rt_free_domdata(ops, rt_dom(dom));
-}
-
 static void *
 rt_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd)
 {
@@ -1585,8 +1561,6 @@ static const struct scheduler sched_rtds_def = {
     .deinit_pdata   = rt_deinit_pdata,
     .alloc_domdata  = rt_alloc_domdata,
     .free_domdata   = rt_free_domdata,
-    .init_domain    = rt_dom_init,
-    .destroy_domain = rt_dom_destroy,
     .alloc_vdata    = rt_alloc_vdata,
     .free_vdata     = rt_free_vdata,
     .insert_vcpu    = rt_vcpu_insert,
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 08a31b6..5f596f0 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -413,25 +413,37 @@ void sched_destroy_vcpu(struct vcpu *v)
 
 int sched_init_domain(struct domain *d, int poolid)
 {
+    void *sdom;
     int ret;
 
     ASSERT(d->cpupool == NULL);
+    ASSERT(d->domain_id < DOMID_FIRST_RESERVED);
 
     if ( (ret = cpupool_add_domain(d, poolid)) )
         return ret;
 
     SCHED_STAT_CRANK(dom_init);
     TRACE_1D(TRC_SCHED_DOM_ADD, d->domain_id);
-    return SCHED_OP(dom_scheduler(d), init_domain, d);
+
+    sdom = sched_alloc_domdata(dom_scheduler(d), d);
+    if ( IS_ERR(sdom) )
+        return PTR_ERR(sdom);
+
+    d->sched_priv = sdom;
+
+    return 0;
 }
 
 void sched_destroy_domain(struct domain *d)
 {
     ASSERT(d->cpupool != NULL || is_idle_domain(d));
+    ASSERT(d->domain_id < DOMID_FIRST_RESERVED);
 
     SCHED_STAT_CRANK(dom_destroy);
     TRACE_1D(TRC_SCHED_DOM_REM, d->domain_id);
-    SCHED_OP(dom_scheduler(d), destroy_domain, d);
+
+    sched_free_domdata(dom_scheduler(d), d->sched_priv);
+    d->sched_priv = NULL;
 
     cpupool_rm_domain(d);
 }
diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
index 56e7d0c..4895242 100644
--- a/xen/include/xen/sched-if.h
+++ b/xen/include/xen/sched-if.h
@@ -155,9 +155,6 @@ struct scheduler {
     void         (*switch_sched)   (struct scheduler *, unsigned int,
                                     void *, void *);
 
-    int          (*init_domain)    (const struct scheduler *, struct domain *);
-    void         (*destroy_domain) (const struct scheduler *, struct domain *);
-
     /* Activate / deactivate vcpus in a cpu pool */
     void         (*insert_vcpu)    (const struct scheduler *, struct vcpu *);
     void         (*remove_vcpu)    (const struct scheduler *, struct vcpu *);
-- 
2.1.4


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

Re: [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces

[George Dunlap]

On 02/28/2018 02:14 PM, Andrew Cooper wrote:
> These hooks have one single caller (sched_{init,destroy}_domain()
> respectively) and are all identical (when implemented).
> 
> Previous changes have ensured that only real domains reach these functions, so
> ASSERT() that system domains are not seen. Call sched_{alloc,free}_domdata()
> directly, and handle d->sched_priv directly.
> 
> The net diffstat is:
>   add/remove: 0/8 grow/shrink: 1/7 up/down: 7/-335 (-328)
>   function                                     old     new   delta
>   sched_destroy_domain                         130     137      +7
>   sched_init_domain                            138     137      -1
>   rt_dom_destroy                                 6       -      -6
>   null_dom_destroy                               6       -      -6
>   csched_dom_destroy                             9       -      -9
>   csched2_dom_destroy                            9       -      -9
>   sched_rtds_def                               264     248     -16
>   sched_null_def                               264     248     -16
>   sched_credit_def                             264     248     -16
>   sched_credit2_def                            264     248     -16
>   sched_arinc653_def                           264     248     -16
>   ops                                          264     248     -16
>   rt_dom_init                                   52       -     -52
>   null_dom_init                                 52       -     -52
>   csched_dom_init                               52       -     -52
>   csched2_dom_init                              52       -     -52
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I kind of feel like there was a reason for the init / alloc difference;
but as you say, at the moment all the schedulers are basically
identical.  In the unlikely event that we need separate callbacks, we
can introduce them at such time as we have a need for them.

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

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

Re: [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces

[Andrew Cooper]

On 28/02/18 16:22, George Dunlap wrote:
> On 02/28/2018 02:14 PM, Andrew Cooper wrote:
>> These hooks have one single caller (sched_{init,destroy}_domain()
>> respectively) and are all identical (when implemented).
>>
>> Previous changes have ensured that only real domains reach these functions, so
>> ASSERT() that system domains are not seen. Call sched_{alloc,free}_domdata()
>> directly, and handle d->sched_priv directly.
>>
>> The net diffstat is:
>>   add/remove: 0/8 grow/shrink: 1/7 up/down: 7/-335 (-328)
>>   function                                     old     new   delta
>>   sched_destroy_domain                         130     137      +7
>>   sched_init_domain                            138     137      -1
>>   rt_dom_destroy                                 6       -      -6
>>   null_dom_destroy                               6       -      -6
>>   csched_dom_destroy                             9       -      -9
>>   csched2_dom_destroy                            9       -      -9
>>   sched_rtds_def                               264     248     -16
>>   sched_null_def                               264     248     -16
>>   sched_credit_def                             264     248     -16
>>   sched_credit2_def                            264     248     -16
>>   sched_arinc653_def                           264     248     -16
>>   ops                                          264     248     -16
>>   rt_dom_init                                   52       -     -52
>>   null_dom_init                                 52       -     -52
>>   csched_dom_init                               52       -     -52
>>   csched2_dom_init                              52       -     -52
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> I kind of feel like there was a reason for the init / alloc difference;
> but as you say, at the moment all the schedulers are basically
> identical.  In the unlikely event that we need separate callbacks, we
> can introduce them at such time as we have a need for them.
>
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>

It looks like it was the cpupool work (c/s 78be3dbb, 2010) which split
alloc/free domdata() out of init/destroy domain() and made the latter
effectively redundant.

~Andrew

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

Re: [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces

[Dario Faggioli]

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

On Wed, 2018-02-28 at 16:33 +0000, Andrew Cooper wrote:
> On 28/02/18 16:22, George Dunlap wrote:
> > 
> > I kind of feel like there was a reason for the init / alloc
> > difference;
> > but as you say, at the moment all the schedulers are basically
> > identical.  In the unlikely event that we need separate callbacks,
> > we
> > can introduce them at such time as we have a need for them.
> > 
> > Reviewed-by: George Dunlap <george.dunlap@citrix.com>
> 
> It looks like it was the cpupool work (c/s 78be3dbb, 2010) which
> split
> alloc/free domdata() out of init/destroy domain() and made the latter
> effectively redundant.
> 
Yes. As far as I can see/tell (I wasn't here), that commit introduced
alloc_domdata because there was the need to allocate the per-scheduler
domain data, without immediately assigning it to sched_priv.

I guess something like this that Andrew is doing could have been done
at that point. It just was not. :-)

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

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

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

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

Re: [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces

[Meng Xu]

On Wed, Feb 28, 2018 at 9:14 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> These hooks have one single caller (sched_{init,destroy}_domain()
> respectively) and are all identical (when implemented).
>
> Previous changes have ensured that only real domains reach these functions, so
> ASSERT() that system domains are not seen. Call sched_{alloc,free}_domdata()
> directly, and handle d->sched_priv directly.
>
> The net diffstat is:
>   add/remove: 0/8 grow/shrink: 1/7 up/down: 7/-335 (-328)
>   function                                     old     new   delta
>   sched_destroy_domain                         130     137      +7
>   sched_init_domain                            138     137      -1
>   rt_dom_destroy                                 6       -      -6
>   null_dom_destroy                               6       -      -6
>   csched_dom_destroy                             9       -      -9
>   csched2_dom_destroy                            9       -      -9
>   sched_rtds_def                               264     248     -16
>   sched_null_def                               264     248     -16
>   sched_credit_def                             264     248     -16
>   sched_credit2_def                            264     248     -16
>   sched_arinc653_def                           264     248     -16
>   ops                                          264     248     -16
>   rt_dom_init                                   52       -     -52
>   null_dom_init                                 52       -     -52
>   csched_dom_init                               52       -     -52
>   csched2_dom_init                              52       -     -52
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---


Acked-by: Meng Xu <mengxu@cis.upenn.edu>


Thanks,

Meng

-----------
Meng Xu
Ph.D. Candidate in Computer and Information Science
University of Pennsylvania
http://www.cis.upenn.edu/~mengxu/

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

Re: [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces

[Dario Faggioli]

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

On Wed, 2018-02-28 at 14:14 +0000, Andrew Cooper wrote:
> These hooks have one single caller (sched_{init,destroy}_domain()
> respectively) and are all identical (when implemented).
> 
> Previous changes have ensured that only real domains reach these
> functions, so
> ASSERT() that system domains are not seen. Call
> sched_{alloc,free}_domdata()
> directly, and handle d->sched_priv directly.
> 
> The net diffstat is:
>   add/remove: 0/8 grow/shrink: 1/7 up/down: 7/-335 (-328)
>   function                                     old     new   delta
>   sched_destroy_domain                         130     137      +7
>   sched_init_domain                            138     137      -1
>   rt_dom_destroy                                 6       -      -6
>   null_dom_destroy                               6       -      -6
>   csched_dom_destroy                             9       -      -9
>   csched2_dom_destroy                            9       -      -9
>   sched_rtds_def                               264     248     -16
>   sched_null_def                               264     248     -16
>   sched_credit_def                             264     248     -16
>   sched_credit2_def                            264     248     -16
>   sched_arinc653_def                           264     248     -16
>   ops                                          264     248     -16
>   rt_dom_init                                   52       -     -52
>   null_dom_init                                 52       -     -52
>   csched_dom_init                               52       -     -52
>   csched2_dom_init                              52       -     -52
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

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

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

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

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

[PATCH 5/6] xen/domain: Call sched_destroy_domain() in the domain_create() error path

[Andrew Cooper]

If domain_create() fails, complete_domain_destroy() doesn't get called,
meaning that sched_destroy_domain() is missed.  In practice, this can only
fail because of exceptional late_hwdom_init() issues at the moment.

Make sched_destroy_domain() idempotent, and call it in the fail path.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Dario Faggioli <dfaggioli@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/common/domain.c   |  3 +++
 xen/common/schedule.c | 14 ++++++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index e0b024c..3cefe76 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -404,6 +404,9 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
         hardware_domain = old_hwdom;
     atomic_set(&d->refcnt, DOMAIN_DESTROYED);
     xfree(d->pbuf);
+
+    sched_destroy_domain(d);
+
     if ( init_status & INIT_arch )
         arch_domain_destroy(d);
     if ( init_status & INIT_gnttab )
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 5f596f0..64524f4 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -436,16 +436,18 @@ int sched_init_domain(struct domain *d, int poolid)
 
 void sched_destroy_domain(struct domain *d)
 {
-    ASSERT(d->cpupool != NULL || is_idle_domain(d));
     ASSERT(d->domain_id < DOMID_FIRST_RESERVED);
 
-    SCHED_STAT_CRANK(dom_destroy);
-    TRACE_1D(TRC_SCHED_DOM_REM, d->domain_id);
+    if ( d->cpupool )
+    {
+        SCHED_STAT_CRANK(dom_destroy);
+        TRACE_1D(TRC_SCHED_DOM_REM, d->domain_id);
 
-    sched_free_domdata(dom_scheduler(d), d->sched_priv);
-    d->sched_priv = NULL;
+        sched_free_domdata(dom_scheduler(d), d->sched_priv);
+        d->sched_priv = NULL;
 
-    cpupool_rm_domain(d);
+        cpupool_rm_domain(d);
+    }
 }
 
 void vcpu_sleep_nosync(struct vcpu *v)
-- 
2.1.4


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

Re: [PATCH 5/6] xen/domain: Call sched_destroy_domain() in the domain_create() error path

[George Dunlap]

On 02/28/2018 02:14 PM, Andrew Cooper wrote:
> If domain_create() fails, complete_domain_destroy() doesn't get called,
> meaning that sched_destroy_domain() is missed.  In practice, this can only
> fail because of exceptional late_hwdom_init() issues at the moment.
> 
> Make sched_destroy_domain() idempotent, and call it in the fail path.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

Re: [PATCH 5/6] xen/domain: Call sched_destroy_domain() in the domain_create() error path

[Dario Faggioli]

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

On Wed, 2018-02-28 at 14:14 +0000, Andrew Cooper wrote:
> If domain_create() fails, complete_domain_destroy() doesn't get
> called,
> meaning that sched_destroy_domain() is missed.  In practice, this can
> only
> fail because of exceptional late_hwdom_init() issues at the moment.
> 
> Make sched_destroy_domain() idempotent, and call it in the fail path.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

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

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

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

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

Re: [PATCH 5/6] xen/domain: Call sched_destroy_domain() in the domain_create() error path

[Dario Faggioli]

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

On Wed, 2018-02-28 at 14:14 +0000, Andrew Cooper wrote:
> If domain_create() fails, complete_domain_destroy() doesn't get
> called,
> meaning that sched_destroy_domain() is missed.  In practice, this can
> only
> fail because of exceptional late_hwdom_init() issues at the moment.
> 
> Make sched_destroy_domain() idempotent, and call it in the fail path.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

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

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

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

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

[PATCH 6/6] xen/domain: Use IS_ERR_OR_NULL() when checking the return value of domain_create()

[Andrew Cooper]

This means that hitting the fail path with with err set to 0 isn't considered
as success by the callers.  All current codepaths look fine.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c     | 6 +++---
 xen/arch/arm/setup.c  | 2 +-
 xen/arch/x86/mm.c     | 6 +++---
 xen/arch/x86/setup.c  | 2 +-
 xen/common/domctl.c   | 2 +-
 xen/common/schedule.c | 2 +-
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 3c328e2..16a08cf 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -521,7 +521,7 @@ void __init arch_init_memory(void)
      * their domain field set to dom_xen.
      */
     dom_xen = domain_create(DOMID_XEN, DOMCRF_dummy, 0, NULL);
-    BUG_ON(IS_ERR(dom_xen));
+    BUG_ON(IS_ERR_OR_NULL(dom_xen));
 
     /*
      * Initialise our DOMID_IO domain.
@@ -529,14 +529,14 @@ void __init arch_init_memory(void)
      * array. Mappings occur at the priv of the caller.
      */
     dom_io = domain_create(DOMID_IO, DOMCRF_dummy, 0, NULL);
-    BUG_ON(IS_ERR(dom_io));
+    BUG_ON(IS_ERR_OR_NULL(dom_io));
 
     /*
      * Initialise our COW domain.
      * This domain owns sharable pages.
      */
     dom_cow = domain_create(DOMID_COW, DOMCRF_dummy, 0, NULL);
-    BUG_ON(IS_ERR(dom_cow));
+    BUG_ON(IS_ERR_OR_NULL(dom_cow));
 }
 
 static inline lpae_t pte_of_xenaddr(vaddr_t va)
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 032a6a8..6646074 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -857,7 +857,7 @@ void __init start_xen(unsigned long boot_phys_offset,
     config.nr_spis = gic_number_lines() - 32;
 
     dom0 = domain_create(0, 0, 0, &config);
-    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
+    if ( IS_ERR_OR_NULL(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
             panic("Error creating domain 0");
 
     dom0->is_privileged = 1;
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index e1f089b..c44b781 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -272,7 +272,7 @@ void __init arch_init_memory(void)
      * (but be [partly] controlled by Dom0 nevertheless).
      */
     dom_xen = domain_create(DOMID_XEN, DOMCRF_dummy, 0, NULL);
-    BUG_ON(IS_ERR(dom_xen));
+    BUG_ON(IS_ERR_OR_NULL(dom_xen));
     INIT_LIST_HEAD(&dom_xen->arch.pdev_list);
 
     /*
@@ -281,14 +281,14 @@ void __init arch_init_memory(void)
      * array. Mappings occur at the priv of the caller.
      */
     dom_io = domain_create(DOMID_IO, DOMCRF_dummy, 0, NULL);
-    BUG_ON(IS_ERR(dom_io));
+    BUG_ON(IS_ERR_OR_NULL(dom_io));
 
     /*
      * Initialise our COW domain.
      * This domain owns sharable pages.
      */
     dom_cow = domain_create(DOMID_COW, DOMCRF_dummy, 0, NULL);
-    BUG_ON(IS_ERR(dom_cow));
+    BUG_ON(IS_ERR_OR_NULL(dom_cow));
 
     /*
      * First 1MB of RAM is historically marked as I/O.  If we booted PVH,
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index ac530ec..92428da 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1640,7 +1640,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     /* Create initial domain 0. */
     dom0 = domain_create(get_initial_domain_id(), domcr_flags, 0, &config);
-    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
+    if ( IS_ERR_OR_NULL(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
         panic("Error creating domain 0");
 
     if ( !pv_shim )
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 50f7422..18dcb2d 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -547,7 +547,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 
         d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
                           &op->u.createdomain.config);
-        if ( IS_ERR(d) )
+        if ( IS_ERR_OR_NULL(d) )
         {
             ret = PTR_ERR(d);
             d = NULL;
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 64524f4..9c629a5 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1735,7 +1735,7 @@ void __init scheduler_init(void)
     }
 
     idle_domain = domain_create(DOMID_IDLE, 0, 0, NULL);
-    BUG_ON(IS_ERR(idle_domain));
+    BUG_ON(IS_ERR_OR_NULL(idle_domain));
     idle_domain->vcpu = idle_vcpu;
     idle_domain->max_vcpus = nr_cpu_ids;
     if ( alloc_vcpu(idle_domain, 0, 0) == NULL )
-- 
2.1.4


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

Re: [PATCH 6/6] xen/domain: Use IS_ERR_OR_NULL() when checking the return value of domain_create()

[George Dunlap]

On Wed, Feb 28, 2018 at 2:14 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> This means that hitting the fail path with with err set to 0 isn't considered
> as success by the callers.  All current codepaths look fine.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/mm.c     | 6 +++---
>  xen/arch/arm/setup.c  | 2 +-
>  xen/arch/x86/mm.c     | 6 +++---
>  xen/arch/x86/setup.c  | 2 +-
>  xen/common/domctl.c   | 2 +-
>  xen/common/schedule.c | 2 +-
>  6 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 3c328e2..16a08cf 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -521,7 +521,7 @@ void __init arch_init_memory(void)
>       * their domain field set to dom_xen.
>       */
>      dom_xen = domain_create(DOMID_XEN, DOMCRF_dummy, 0, NULL);
> -    BUG_ON(IS_ERR(dom_xen));
> +    BUG_ON(IS_ERR_OR_NULL(dom_xen));
>
>      /*
>       * Initialise our DOMID_IO domain.
> @@ -529,14 +529,14 @@ void __init arch_init_memory(void)
>       * array. Mappings occur at the priv of the caller.
>       */
>      dom_io = domain_create(DOMID_IO, DOMCRF_dummy, 0, NULL);
> -    BUG_ON(IS_ERR(dom_io));
> +    BUG_ON(IS_ERR_OR_NULL(dom_io));
>
>      /*
>       * Initialise our COW domain.
>       * This domain owns sharable pages.
>       */
>      dom_cow = domain_create(DOMID_COW, DOMCRF_dummy, 0, NULL);
> -    BUG_ON(IS_ERR(dom_cow));
> +    BUG_ON(IS_ERR_OR_NULL(dom_cow));
>  }
>
>  static inline lpae_t pte_of_xenaddr(vaddr_t va)
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 032a6a8..6646074 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -857,7 +857,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>      config.nr_spis = gic_number_lines() - 32;
>
>      dom0 = domain_create(0, 0, 0, &config);
> -    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> +    if ( IS_ERR_OR_NULL(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
>              panic("Error creating domain 0");
>
>      dom0->is_privileged = 1;
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index e1f089b..c44b781 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -272,7 +272,7 @@ void __init arch_init_memory(void)
>       * (but be [partly] controlled by Dom0 nevertheless).
>       */
>      dom_xen = domain_create(DOMID_XEN, DOMCRF_dummy, 0, NULL);
> -    BUG_ON(IS_ERR(dom_xen));
> +    BUG_ON(IS_ERR_OR_NULL(dom_xen));
>      INIT_LIST_HEAD(&dom_xen->arch.pdev_list);
>
>      /*
> @@ -281,14 +281,14 @@ void __init arch_init_memory(void)
>       * array. Mappings occur at the priv of the caller.
>       */
>      dom_io = domain_create(DOMID_IO, DOMCRF_dummy, 0, NULL);
> -    BUG_ON(IS_ERR(dom_io));
> +    BUG_ON(IS_ERR_OR_NULL(dom_io));
>
>      /*
>       * Initialise our COW domain.
>       * This domain owns sharable pages.
>       */
>      dom_cow = domain_create(DOMID_COW, DOMCRF_dummy, 0, NULL);
> -    BUG_ON(IS_ERR(dom_cow));
> +    BUG_ON(IS_ERR_OR_NULL(dom_cow));
>
>      /*
>       * First 1MB of RAM is historically marked as I/O.  If we booted PVH,
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index ac530ec..92428da 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1640,7 +1640,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>
>      /* Create initial domain 0. */
>      dom0 = domain_create(get_initial_domain_id(), domcr_flags, 0, &config);
> -    if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> +    if ( IS_ERR_OR_NULL(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
>          panic("Error creating domain 0");
>
>      if ( !pv_shim )
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 50f7422..18dcb2d 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -547,7 +547,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>
>          d = domain_create(dom, domcr_flags, op->u.createdomain.ssidref,
>                            &op->u.createdomain.config);
> -        if ( IS_ERR(d) )
> +        if ( IS_ERR_OR_NULL(d) )
>          {
>              ret = PTR_ERR(d);
>              d = NULL;

This will fail to crash if domain_create() returns NULL, but it will
still return 0 from the hypercall (since AFAICT PTR_ERR(NULL) will be
just '0').  Is that what we want?

Given that anywhere else this returns NULL will cause a BUG() or a
panic, I almost feel like a BUG_ON(d == NULL) would be more
appropriate.

Everything else looks good to me.

 -George

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

[PATCH v2 6/6] xen/domain: Added debug safety in the domain_create() failure path

[Andrew Cooper]

Hitting the fail path with err = 0 causes callers to dereference a NULL
pointer, as 0 fails an IS_ERR() check.

All of the paths appear to be fine, but leave some logic to help catch stray
misuses.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>

v2:
 * Completely different implementation
---
 xen/common/domain.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 3cefe76..fd054db 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -399,6 +399,9 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
     return d;
 
  fail:
+    ASSERT(err < 0);      /* Sanity check paths leading here. */
+    err = err ?: -EINVAL; /* Release build safety. */
+
     d->is_dying = DOMDYING_dead;
     if ( hardware_domain == d )
         hardware_domain = old_hwdom;
-- 
2.1.4


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

Re: [PATCH v2 6/6] xen/domain: Added debug safety in the domain_create() failure path

[Jan Beulich]

>>> On 07.03.18 at 20:12, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -399,6 +399,9 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
>      return d;
>  
>   fail:
> +    ASSERT(err < 0);      /* Sanity check paths leading here. */
> +    err = err ?: -EINVAL; /* Release build safety. */

Fundamentally I'm fine with this, but could you use a much less
frequently used (and hence easier to identify) error code here?
Like EILSEQ, ECHILD, or ENOTEMPTY (and I'm open to the use of
other obscure ones)? We don't know what the cause of the error
was, so what exact error code to report is pretty meaningless to
the caller anyway.

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

Jan


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