Re: [RFC 1/6] rangeset_new() refactoring

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Andrii Anisov [mailto:andrii.anisov@gmail.com]
> Sent: 16 February 2017 12:03
> To: xen-devel@lists.xenproject.org
> Cc: andrii_anisov@epam.com; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
> jbeulich@suse.com; konrad.wilk@oracle.com; Paul Durrant
> <Paul.Durrant@citrix.com>; sstabellini@kernel.org; Tim (Xen.org)
> <tim@xen.org>; Wei Liu <wei.liu2@citrix.com>
> Subject: [RFC 1/6] rangeset_new() refactoring
> 
> From: Andrii Anisov <andrii_anisov@epam.com>
> 
> rangeset_new is made domain agnostic. The domain specific code
> is moved to common/domain.c:domain_rangeset_new().
> 
> It is still left a rangesets list functionality: rangeset_new() is
> returning its list head if requested. This would be reconsidered
> further.
> 
> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> ---
>  xen/arch/x86/domain.c      |  2 +-
>  xen/arch/x86/hvm/ioreq.c   |  4 ++--
>  xen/arch/x86/mm/p2m.c      |  4 ++--
>  xen/arch/x86/setup.c       |  4 ++--
>  xen/common/domain.c        | 22 ++++++++++++++++++++--
>  xen/common/rangeset.c      | 12 ++++--------
>  xen/include/xen/domain.h   |  3 +++
>  xen/include/xen/rangeset.h | 19 +++++++++++--------
>  8 files changed, 45 insertions(+), 25 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index eae643f..93f18d7 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -630,7 +630,7 @@ int arch_domain_create(struct domain *d, unsigned
> int domcr_flags,
>          d->arch.x86_model  = boot_cpu_data.x86_model;
> 
>          d->arch.ioport_caps =
> -            rangeset_new(d, "I/O Ports", RANGESETF_prettyprint_hex);
> +            domain_rangeset_new(d, "I/O Ports", RANGESETF_prettyprint_hex);
>          rc = -ENOMEM;
>          if ( d->arch.ioport_caps == NULL )
>              goto fail;
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index 88071ab..6df191d 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -520,8 +520,8 @@ static int hvm_ioreq_server_alloc_rangesets(struct
> hvm_ioreq_server *s,
>          if ( rc )
>              goto fail;
> 
> -        s->range[i] = rangeset_new(s->domain, name,
> -                                   RANGESETF_prettyprint_hex);
> +        s->range[i] = domain_rangeset_new(s->domain, name,
> +                                          RANGESETF_prettyprint_hex);
> 
>          xfree(name);
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 6a45185..46301ad 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -122,8 +122,8 @@ static int p2m_init_hostp2m(struct domain *d)
> 
>      if ( p2m )
>      {
> -        p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
> -                                            RANGESETF_prettyprint_hex);
> +        p2m->logdirty_ranges = domain_rangeset_new(d, "log-dirty",
> +                                                   RANGESETF_prettyprint_hex);
>          if ( p2m->logdirty_ranges )
>          {
>              d->arch.p2m = p2m;
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index b130671..69a961c 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1419,8 +1419,8 @@ void __init noreturn __start_xen(unsigned long
> mbi_p)
>      /* Low mappings were only needed for some BIOS table parsing. */
>      zap_low_mappings();
> 
> -    mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
> -                                  RANGESETF_prettyprint_hex);
> +    mmio_ro_ranges = rangeset_new("r/o mmio ranges",
> +                                  RANGESETF_prettyprint_hex, NULL);
> 
>      init_apic_mappings();
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 3abaca9..1b9bc3c 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -329,8 +329,8 @@ struct domain *domain_create(domid_t domid,
> unsigned int domcr_flags,
>      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);
> +    d->iomem_caps = domain_rangeset_new(d, "I/O Memory",
> RANGESETF_prettyprint_hex);
> +    d->irq_caps   = domain_rangeset_new(d, "Interrupts", 0);
>      if ( (d->iomem_caps == NULL) || (d->irq_caps == NULL) )
>          goto fail;
> 
> @@ -1537,6 +1537,24 @@ int continue_hypercall_on_cpu(
>      return 0;
>  }
> 
> +struct rangeset *domain_rangeset_new(struct domain *d, char *name,
> +                                     unsigned int flags)
> +{
> +    struct list_head *head;
> +    struct rangeset *r = rangeset_new(name, flags, &head);
> +
> +    if ( d != NULL )

Shouldn't d == NULL be a hard error now, in which you could pass d->rangesets directly into rangeset_new() (under lock).

  Paul

> +    {
> +        spin_lock(&d->rangesets_lock);
> +        list_add(head, &d->rangesets);
> +        spin_unlock(&d->rangesets_lock);
> +    }
> +
> +    return r;
> +}
> +
> +
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
> index 6c6293c..478d232 100644
> --- a/xen/common/rangeset.c
> +++ b/xen/common/rangeset.c
> @@ -322,8 +322,8 @@ bool_t rangeset_is_empty(
>      return ((r == NULL) || list_empty(&r->range_list));
>  }
> 
> -struct rangeset *rangeset_new(
> -    struct domain *d, char *name, unsigned int flags)
> +struct rangeset *rangeset_new(char *name, unsigned int flags,
> +                              struct list_head **head)
>  {
>      struct rangeset *r;
> 
> @@ -347,12 +347,8 @@ struct rangeset *rangeset_new(
>          snprintf(r->name, sizeof(r->name), "(no name)");
>      }
> 
> -    if ( (r->domain = d) != NULL )
> -    {
> -        spin_lock(&d->rangesets_lock);
> -        list_add(&r->rangeset_list, &d->rangesets);
> -        spin_unlock(&d->rangesets_lock);
> -    }
> +    if (head)
> +        *head = &r->rangeset_list;
> 
>      return r;
>  }
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index bce0ea1..cd62e6e 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -108,4 +108,7 @@ struct vnuma_info {
> 
>  void vnuma_destroy(struct vnuma_info *vnuma);
> 
> +struct rangeset *domain_rangeset_new(struct domain *d, char *name,
> +                                     unsigned int flags);
> +
>  #endif /* __XEN_DOMAIN_H__ */
> diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
> index aa64082..395ba62 100644
> --- a/xen/include/xen/rangeset.h
> +++ b/xen/include/xen/rangeset.h
> @@ -13,6 +13,7 @@
>  #include <xen/types.h>
> 
>  struct domain;
> +struct list_head;
>  struct rangeset;
> 
>  /*
> @@ -28,15 +29,17 @@ void rangeset_domain_destroy(
>      struct domain *d);
> 
>  /*
> - * Create/destroy a rangeset. Optionally attach to specified domain @d for
> - * auto-destruction when the domain dies. A name may be specified, for
> use
> - * in debug pretty-printing, and various RANGESETF flags (defined below).
> - *
> - * It is invalid to perform any operation on a rangeset @r after calling
> - * rangeset_destroy(r).
> + * Create a rangeset. Optionally attach to a specified list @head.
> + * A name may be specified, for use in debug pretty-printing, and various
> + * RANGESETF flags (defined below).
> + */
> +struct rangeset *rangeset_new(char *name, unsigned int flags,
> +                              struct list_head **head);
> +
> +/*
> + * Destroy a rangeset. It is invalid to perform any operation on a rangeset
> @r
> + * after calling rangeset_destroy(r).
>   */
> -struct rangeset *rangeset_new(
> -    struct domain *d, char *name, unsigned int flags);
>  void rangeset_destroy(
>      struct rangeset *r);
> 
> --
> 2.7.4


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