xen/sched: Improvements to the {alloc,free}_domdata() interfaces

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>
Acked-by: Meng Xu <mengxu@cis.upenn.edu>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

xen/common/sched_arinc653.c

@@ -454,34 +454,6 @@ a653sched_free_vdata(const struct scheduler *ops, void *priv)
     update_schedule_vcpus(ops);
 }
 
-/**
- * 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
  *
@@ -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,
 

xen/common/sched_credit.c

@@ -1281,7 +1281,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);
@@ -1289,7 +1289,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
@@ -1301,8 +1301,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;
 

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);
+    if ( sdom )
+    {
+        unsigned long flags;
 
-    write_lock_irqsave(&prv->lock, flags);
+        kill_timer(&sdom->repl_timer);
 
-    list_del_init(&sdom->sdom_elem);
+        write_lock_irqsave(&prv->lock, flags);
+        list_del_init(&sdom->sdom_elem);
+        write_unlock_irqrestore(&prv->lock, flags);
 
-    write_unlock_irqrestore(&prv->lock, flags);
-
-    xfree(data);
+        xfree(sdom);
+    }
 }
 
 static void

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;
 
-    xfree(data);
+        spin_lock_irqsave(&prv->lock, flags);
+        list_del_init(&ndom->ndom_elem);
+        spin_unlock_irqrestore(&prv->lock, flags);
+
+        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;
 

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;
 

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

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;