All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] Assorted improvements to domain creation
@ 2018-02-28 14:14 Andrew Cooper
  2018-02-28 14:14 ` [PATCH 1/6] xen/domain: Reduce the quantity of initialisation for system domains Andrew Cooper
                   ` (5 more replies)
  0 siblings, 6 replies; 26+ messages in thread
From: Andrew Cooper @ 2018-02-28 14:14 UTC (permalink / raw)
  To: Xen-devel; +Cc: 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

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

* [PATCH 1/6] xen/domain: Reduce the quantity of initialisation for system domains
  2018-02-28 14:14 [PATCH 0/6] Assorted improvements to domain creation Andrew Cooper
@ 2018-02-28 14:14 ` Andrew Cooper
  2018-02-28 15:22   ` George Dunlap
  2018-03-01 10:10   ` Dario Faggioli
  2018-02-28 14:14 ` [PATCH 2/6] xen/credit2: Move repl_timer into struct csched2_dom Andrew Cooper
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 26+ messages in thread
From: Andrew Cooper @ 2018-02-28 14:14 UTC (permalink / raw)
  To: Xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Tim Deegan, Dario Faggioli, Julien Grall, Jan Beulich,
	Roger Pau Monné

 * 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

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

* [PATCH 2/6] xen/credit2: Move repl_timer into struct csched2_dom
  2018-02-28 14:14 [PATCH 0/6] Assorted improvements to domain creation Andrew Cooper
  2018-02-28 14:14 ` [PATCH 1/6] xen/domain: Reduce the quantity of initialisation for system domains Andrew Cooper
@ 2018-02-28 14:14 ` Andrew Cooper
  2018-02-28 15:26   ` George Dunlap
  2018-02-28 14:14 ` [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces Andrew Cooper
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 26+ messages in thread
From: Andrew Cooper @ 2018-02-28 14:14 UTC (permalink / raw)
  To: Xen-devel; +Cc: George Dunlap, Andrew Cooper, Dario Faggioli

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

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

* [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces
  2018-02-28 14:14 [PATCH 0/6] Assorted improvements to domain creation Andrew Cooper
  2018-02-28 14:14 ` [PATCH 1/6] xen/domain: Reduce the quantity of initialisation for system domains Andrew Cooper
  2018-02-28 14:14 ` [PATCH 2/6] xen/credit2: Move repl_timer into struct csched2_dom Andrew Cooper
@ 2018-02-28 14:14 ` Andrew Cooper
  2018-02-28 15:40   ` George Dunlap
                     ` (3 more replies)
  2018-02-28 14:14 ` [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces Andrew Cooper
                   ` (2 subsequent siblings)
  5 siblings, 4 replies; 26+ messages in thread
From: Andrew Cooper @ 2018-02-28 14:14 UTC (permalink / raw)
  To: Xen-devel
  Cc: George Dunlap, Andrew Cooper, Robert VanVossen, Dario Faggioli,
	Josh Whitehead, Meng Xu

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

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

* [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces
  2018-02-28 14:14 [PATCH 0/6] Assorted improvements to domain creation Andrew Cooper
                   ` (2 preceding siblings ...)
  2018-02-28 14:14 ` [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces Andrew Cooper
@ 2018-02-28 14:14 ` Andrew Cooper
  2018-02-28 16:22   ` George Dunlap
                     ` (2 more replies)
  2018-02-28 14:14 ` [PATCH 5/6] xen/domain: Call sched_destroy_domain() in the domain_create() error path Andrew Cooper
  2018-02-28 14:14 ` [PATCH 6/6] xen/domain: Use IS_ERR_OR_NULL() when checking the return value of domain_create() Andrew Cooper
  5 siblings, 3 replies; 26+ messages in thread
From: Andrew Cooper @ 2018-02-28 14:14 UTC (permalink / raw)
  To: Xen-devel
  Cc: George Dunlap, Andrew Cooper, Robert VanVossen, Dario Faggioli,
	Josh Whitehead, Meng Xu

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

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

* [PATCH 5/6] xen/domain: Call sched_destroy_domain() in the domain_create() error path
  2018-02-28 14:14 [PATCH 0/6] Assorted improvements to domain creation Andrew Cooper
                   ` (3 preceding siblings ...)
  2018-02-28 14:14 ` [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces Andrew Cooper
@ 2018-02-28 14:14 ` Andrew Cooper
  2018-02-28 16:24   ` George Dunlap
                     ` (2 more replies)
  2018-02-28 14:14 ` [PATCH 6/6] xen/domain: Use IS_ERR_OR_NULL() when checking the return value of domain_create() Andrew Cooper
  5 siblings, 3 replies; 26+ messages in thread
From: Andrew Cooper @ 2018-02-28 14:14 UTC (permalink / raw)
  To: Xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Tim Deegan, Dario Faggioli, Jan Beulich, Roger Pau Monné

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

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

* [PATCH 6/6] xen/domain: Use IS_ERR_OR_NULL() when checking the return value of domain_create()
  2018-02-28 14:14 [PATCH 0/6] Assorted improvements to domain creation Andrew Cooper
                   ` (4 preceding siblings ...)
  2018-02-28 14:14 ` [PATCH 5/6] xen/domain: Call sched_destroy_domain() in the domain_create() error path Andrew Cooper
@ 2018-02-28 14:14 ` Andrew Cooper
  2018-02-28 16:36   ` George Dunlap
  2018-03-07 19:12   ` [PATCH v2 6/6] xen/domain: Added debug safety in the domain_create() failure path Andrew Cooper
  5 siblings, 2 replies; 26+ messages in thread
From: Andrew Cooper @ 2018-02-28 14:14 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, Jan Beulich

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

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

* Re: [PATCH 1/6] xen/domain: Reduce the quantity of initialisation for system domains
  2018-02-28 14:14 ` [PATCH 1/6] xen/domain: Reduce the quantity of initialisation for system domains Andrew Cooper
@ 2018-02-28 15:22   ` George Dunlap
  2018-03-01 10:10   ` Dario Faggioli
  1 sibling, 0 replies; 26+ messages in thread
From: George Dunlap @ 2018-02-28 15:22 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan,
	Dario Faggioli, Julien Grall, Jan Beulich, Roger Pau Monné

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

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

* Re: [PATCH 2/6] xen/credit2: Move repl_timer into struct csched2_dom
  2018-02-28 14:14 ` [PATCH 2/6] xen/credit2: Move repl_timer into struct csched2_dom Andrew Cooper
@ 2018-02-28 15:26   ` George Dunlap
  2018-03-01 10:12     ` Dario Faggioli
  0 siblings, 1 reply; 26+ messages in thread
From: George Dunlap @ 2018-02-28 15:26 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel; +Cc: George Dunlap, Dario Faggioli

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

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

* Re: [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces
  2018-02-28 14:14 ` [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces Andrew Cooper
@ 2018-02-28 15:40   ` George Dunlap
  2018-02-28 16:31   ` Meng Xu
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 26+ messages in thread
From: George Dunlap @ 2018-02-28 15:40 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: George Dunlap, Josh Whitehead, Meng Xu, Robert VanVossen, Dario Faggioli

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

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

* Re: [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces
  2018-02-28 14:14 ` [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces Andrew Cooper
@ 2018-02-28 16:22   ` George Dunlap
  2018-02-28 16:33     ` Andrew Cooper
  2018-02-28 16:34   ` Meng Xu
  2018-03-01 11:00   ` Dario Faggioli
  2 siblings, 1 reply; 26+ messages in thread
From: George Dunlap @ 2018-02-28 16:22 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: George Dunlap, Josh Whitehead, Meng Xu, Robert VanVossen, Dario Faggioli

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

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

* Re: [PATCH 5/6] xen/domain: Call sched_destroy_domain() in the domain_create() error path
  2018-02-28 14:14 ` [PATCH 5/6] xen/domain: Call sched_destroy_domain() in the domain_create() error path Andrew Cooper
@ 2018-02-28 16:24   ` George Dunlap
  2018-03-01 13:25   ` Dario Faggioli
  2018-03-01 13:27   ` Dario Faggioli
  2 siblings, 0 replies; 26+ messages in thread
From: George Dunlap @ 2018-02-28 16:24 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan,
	Dario Faggioli, Jan Beulich, Roger Pau Monné

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

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

* Re: [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces
  2018-02-28 14:14 ` [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces Andrew Cooper
  2018-02-28 15:40   ` George Dunlap
@ 2018-02-28 16:31   ` Meng Xu
  2018-03-01 10:39   ` Dario Faggioli
  2018-03-05 18:20   ` Andrew Cooper
  3 siblings, 0 replies; 26+ messages in thread
From: Meng Xu @ 2018-02-28 16:31 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: George Dunlap, Dario Faggioli, Josh Whitehead, Robert VanVossen,
	Xen-devel

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

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

* Re: [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces
  2018-02-28 16:22   ` George Dunlap
@ 2018-02-28 16:33     ` Andrew Cooper
  2018-03-01 11:08       ` Dario Faggioli
  0 siblings, 1 reply; 26+ messages in thread
From: Andrew Cooper @ 2018-02-28 16:33 UTC (permalink / raw)
  To: George Dunlap, Xen-devel
  Cc: George Dunlap, Josh Whitehead, Meng Xu, Robert VanVossen, Dario Faggioli

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

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

* Re: [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces
  2018-02-28 14:14 ` [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces Andrew Cooper
  2018-02-28 16:22   ` George Dunlap
@ 2018-02-28 16:34   ` Meng Xu
  2018-03-01 11:00   ` Dario Faggioli
  2 siblings, 0 replies; 26+ messages in thread
From: Meng Xu @ 2018-02-28 16:34 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: George Dunlap, Dario Faggioli, Josh Whitehead, Robert VanVossen,
	Xen-devel

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

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

* Re: [PATCH 6/6] xen/domain: Use IS_ERR_OR_NULL() when checking the return value of domain_create()
  2018-02-28 14:14 ` [PATCH 6/6] xen/domain: Use IS_ERR_OR_NULL() when checking the return value of domain_create() Andrew Cooper
@ 2018-02-28 16:36   ` George Dunlap
  2018-03-07 19:12   ` [PATCH v2 6/6] xen/domain: Added debug safety in the domain_create() failure path Andrew Cooper
  1 sibling, 0 replies; 26+ messages in thread
From: George Dunlap @ 2018-02-28 16:36 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Julien Grall, Stefano Stabellini, Jan Beulich, Xen-devel

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

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

* Re: [PATCH 1/6] xen/domain: Reduce the quantity of initialisation for system domains
  2018-02-28 14:14 ` [PATCH 1/6] xen/domain: Reduce the quantity of initialisation for system domains Andrew Cooper
  2018-02-28 15:22   ` George Dunlap
@ 2018-03-01 10:10   ` Dario Faggioli
  1 sibling, 0 replies; 26+ messages in thread
From: Dario Faggioli @ 2018-03-01 10:10 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan,
	Julien Grall, Jan Beulich, Roger Pau Monné


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

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

* Re: [PATCH 2/6] xen/credit2: Move repl_timer into struct csched2_dom
  2018-02-28 15:26   ` George Dunlap
@ 2018-03-01 10:12     ` Dario Faggioli
  0 siblings, 0 replies; 26+ messages in thread
From: Dario Faggioli @ 2018-03-01 10:12 UTC (permalink / raw)
  To: George Dunlap, Andrew Cooper, Xen-devel; +Cc: George Dunlap


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

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

* Re: [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces
  2018-02-28 14:14 ` [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces Andrew Cooper
  2018-02-28 15:40   ` George Dunlap
  2018-02-28 16:31   ` Meng Xu
@ 2018-03-01 10:39   ` Dario Faggioli
  2018-03-05 18:20   ` Andrew Cooper
  3 siblings, 0 replies; 26+ messages in thread
From: Dario Faggioli @ 2018-03-01 10:39 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: George Dunlap, Josh Whitehead, Meng Xu, Robert VanVossen


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

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

* Re: [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces
  2018-02-28 14:14 ` [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces Andrew Cooper
  2018-02-28 16:22   ` George Dunlap
  2018-02-28 16:34   ` Meng Xu
@ 2018-03-01 11:00   ` Dario Faggioli
  2 siblings, 0 replies; 26+ messages in thread
From: Dario Faggioli @ 2018-03-01 11:00 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: George Dunlap, Josh Whitehead, Meng Xu, Robert VanVossen


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

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

* Re: [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces
  2018-02-28 16:33     ` Andrew Cooper
@ 2018-03-01 11:08       ` Dario Faggioli
  0 siblings, 0 replies; 26+ messages in thread
From: Dario Faggioli @ 2018-03-01 11:08 UTC (permalink / raw)
  To: Andrew Cooper, George Dunlap, Xen-devel
  Cc: George Dunlap, Josh Whitehead, Meng Xu, Robert VanVossen


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

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

* Re: [PATCH 5/6] xen/domain: Call sched_destroy_domain() in the domain_create() error path
  2018-02-28 14:14 ` [PATCH 5/6] xen/domain: Call sched_destroy_domain() in the domain_create() error path Andrew Cooper
  2018-02-28 16:24   ` George Dunlap
@ 2018-03-01 13:25   ` Dario Faggioli
  2018-03-01 13:27   ` Dario Faggioli
  2 siblings, 0 replies; 26+ messages in thread
From: Dario Faggioli @ 2018-03-01 13:25 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan,
	Jan Beulich, Roger Pau Monné


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

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

* Re: [PATCH 5/6] xen/domain: Call sched_destroy_domain() in the domain_create() error path
  2018-02-28 14:14 ` [PATCH 5/6] xen/domain: Call sched_destroy_domain() in the domain_create() error path Andrew Cooper
  2018-02-28 16:24   ` George Dunlap
  2018-03-01 13:25   ` Dario Faggioli
@ 2018-03-01 13:27   ` Dario Faggioli
  2 siblings, 0 replies; 26+ messages in thread
From: Dario Faggioli @ 2018-03-01 13:27 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan,
	Jan Beulich, Roger Pau Monné


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

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

* Re: [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces
  2018-02-28 14:14 ` [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces Andrew Cooper
                     ` (2 preceding siblings ...)
  2018-03-01 10:39   ` Dario Faggioli
@ 2018-03-05 18:20   ` Andrew Cooper
  3 siblings, 0 replies; 26+ messages in thread
From: Andrew Cooper @ 2018-03-05 18:20 UTC (permalink / raw)
  To: Xen-devel; +Cc: Josh Whitehead, Robert VanVossen

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

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

* [PATCH v2 6/6] xen/domain: Added debug safety in the domain_create() failure path
  2018-02-28 14:14 ` [PATCH 6/6] xen/domain: Use IS_ERR_OR_NULL() when checking the return value of domain_create() Andrew Cooper
  2018-02-28 16:36   ` George Dunlap
@ 2018-03-07 19:12   ` Andrew Cooper
  2018-03-08  9:04     ` Jan Beulich
  1 sibling, 1 reply; 26+ messages in thread
From: Andrew Cooper @ 2018-03-07 19:12 UTC (permalink / raw)
  To: Xen-devel
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
	Tim Deegan, Jan Beulich

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

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

* Re: [PATCH v2 6/6] xen/domain: Added debug safety in the domain_create() failure path
  2018-03-07 19:12   ` [PATCH v2 6/6] xen/domain: Added debug safety in the domain_create() failure path Andrew Cooper
@ 2018-03-08  9:04     ` Jan Beulich
  0 siblings, 0 replies; 26+ messages in thread
From: Jan Beulich @ 2018-03-08  9:04 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan, Xen-devel

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

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

end of thread, other threads:[~2018-03-08  9:04 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-28 14:14 [PATCH 0/6] Assorted improvements to domain creation Andrew Cooper
2018-02-28 14:14 ` [PATCH 1/6] xen/domain: Reduce the quantity of initialisation for system domains Andrew Cooper
2018-02-28 15:22   ` George Dunlap
2018-03-01 10:10   ` Dario Faggioli
2018-02-28 14:14 ` [PATCH 2/6] xen/credit2: Move repl_timer into struct csched2_dom Andrew Cooper
2018-02-28 15:26   ` George Dunlap
2018-03-01 10:12     ` Dario Faggioli
2018-02-28 14:14 ` [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces Andrew Cooper
2018-02-28 15:40   ` George Dunlap
2018-02-28 16:31   ` Meng Xu
2018-03-01 10:39   ` Dario Faggioli
2018-03-05 18:20   ` Andrew Cooper
2018-02-28 14:14 ` [PATCH 4/6] xen/sched: Remove {init, destroy}_domain() interfaces Andrew Cooper
2018-02-28 16:22   ` George Dunlap
2018-02-28 16:33     ` Andrew Cooper
2018-03-01 11:08       ` Dario Faggioli
2018-02-28 16:34   ` Meng Xu
2018-03-01 11:00   ` Dario Faggioli
2018-02-28 14:14 ` [PATCH 5/6] xen/domain: Call sched_destroy_domain() in the domain_create() error path Andrew Cooper
2018-02-28 16:24   ` George Dunlap
2018-03-01 13:25   ` Dario Faggioli
2018-03-01 13:27   ` Dario Faggioli
2018-02-28 14:14 ` [PATCH 6/6] xen/domain: Use IS_ERR_OR_NULL() when checking the return value of domain_create() Andrew Cooper
2018-02-28 16:36   ` George Dunlap
2018-03-07 19:12   ` [PATCH v2 6/6] xen/domain: Added debug safety in the domain_create() failure path Andrew Cooper
2018-03-08  9:04     ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.