[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxxxx>
> ---
> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> CC: Dario Faggioli <dfaggioli@xxxxxxxx>
> CC: Meng Xu <mengxu@xxxxxxxxxxxxx>
> CC: Josh Whitehead <josh.whitehead@xxxxxxxxxxxxxxx>
> CC: Robert VanVossen <robert.vanvossen@xxxxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.