|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |