Re: [Xen-devel] [RFC 2/6] rangeset_destroy() refactoring

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrii Anisov [mailto:andrii.anisov@xxxxxxxxx]
> Sent: 16 February 2017 14:26
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; andrii_anisov@xxxxxxxx; Andrew
> Cooper <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> jbeulich@xxxxxxxx; konrad.wilk@xxxxxxxxxx; sstabellini@xxxxxxxxxx; Tim
> (Xen.org) <tim@xxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>
> Subject: Re: [RFC 2/6] rangeset_destroy() refactoring
> 
> Dear Paul,
> 
> >> It is still left a rangesets list functionality: rangeset_destroy()
> >> will remove itself from a list. If a spinlock is provided it will be
> >> held for list deletion operation. This would be reconsidered further.
> >>
> >
> > Maybe use the same scheme in patch #1 then and pass the lock, as well as
> the list_head, into rangeset_new().
> Initialized list head in patch#1 is provided by rangeset to user if
> user needs it to link to some list. So user should do the locking on
> tree insert if it needs. Here rangeset user can not acquire the tree
> head even if it has a rangeset itself, because `struct rangeset` is
> not exposed via header file. So rangeset_destroy() should take care of
> tree head remove and spinlock locking if needed.
> 
> I still have concerns why rangeset list keeping should be inside
> rangeset itself.

What use are rangesets if the implementation doesn't control the list/tree? How 
on earth would you implement an allocation function otherwise?

  Paul

> 
> Sincerely,
> Andrii Anisov.
> 
> 
> 2017-02-16 14:26 GMT+02:00 Paul Durrant <Paul.Durrant@xxxxxxxxxx>:
> >> -----Original Message-----
> >> From: Andrii Anisov [mailto:andrii.anisov@xxxxxxxxx]
> >> Sent: 16 February 2017 12:03
> >> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: andrii_anisov@xxxxxxxx; Andrew Cooper
> >> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> >> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> >> jbeulich@xxxxxxxx; konrad.wilk@xxxxxxxxxx; Paul Durrant
> >> <Paul.Durrant@xxxxxxxxxx>; sstabellini@xxxxxxxxxx; Tim (Xen.org)
> >> <tim@xxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>
> >> Subject: [RFC 2/6] rangeset_destroy() refactoring
> >>
> >> From: Andrii Anisov <andrii_anisov@xxxxxxxx>
> >>
> >> rangeset_destroy is made domain agnostic. The domain specific code
> >> is moved to common/domain.c:domain_rangeset_destroy().
> >>
> >> It is still left a rangesets list functionality: rangeset_destroy()
> >> will remove itself from a list. If a spinlock is provided it will be
> >> held for list deletion operation. This would be reconsidered further.
> >>
> >
> > Maybe use the same scheme in patch #1 then and pass the lock, as well as
> the list_head, into rangeset_new().
> >
> >   Paul
> >
> >> Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> >> ---
> >>  xen/arch/x86/hvm/ioreq.c   |  2 +-
> >>  xen/arch/x86/mm/p2m.c      |  2 +-
> >>  xen/common/domain.c        |  5 +++++
> >>  xen/common/rangeset.c      | 15 ++++++++-------
> >>  xen/include/xen/domain.h   |  3 +++
> >>  xen/include/xen/rangeset.h |  9 ++++++---
> >>  6 files changed, 24 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> >> index 6df191d..6ae5921 100644
> >> --- a/xen/arch/x86/hvm/ioreq.c
> >> +++ b/xen/arch/x86/hvm/ioreq.c
> >> @@ -496,7 +496,7 @@ static void
> hvm_ioreq_server_free_rangesets(struct
> >> hvm_ioreq_server *s,
> >>          return;
> >>
> >>      for ( i = 0; i < NR_IO_RANGE_TYPES; i++ )
> >> -        rangeset_destroy(s->range[i]);
> >> +        domain_rangeset_destroy(s->range[i], s->domain);
> >>  }
> >>
> >>  static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server
> *s,
> >> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> >> index 46301ad..d39c093 100644
> >> --- a/xen/arch/x86/mm/p2m.c
> >> +++ b/xen/arch/x86/mm/p2m.c
> >> @@ -141,7 +141,7 @@ static void p2m_teardown_hostp2m(struct domain
> >> *d)
> >>
> >>      if ( p2m )
> >>      {
> >> -        rangeset_destroy(p2m->logdirty_ranges);
> >> +        domain_rangeset_destroy(p2m->logdirty_ranges, d);
> >>          p2m_free_one(p2m);
> >>          d->arch.p2m = NULL;
> >>      }
> >> diff --git a/xen/common/domain.c b/xen/common/domain.c
> >> index 1b9bc3c..f03a032 100644
> >> --- a/xen/common/domain.c
> >> +++ b/xen/common/domain.c
> >> @@ -1553,6 +1553,11 @@ struct rangeset *domain_rangeset_new(struct
> >> domain *d, char *name,
> >>      return r;
> >>  }
> >>
> >> +void domain_rangeset_destroy(struct domain *d,
> >> +    struct rangeset *r)
> >> +{
> >> +    rangeset_destroy(r, &d->rangesets_lock);
> >> +}
> >>
> >>
> >>  /*
> >> diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
> >> index 478d232..1172950 100644
> >> --- a/xen/common/rangeset.c
> >> +++ b/xen/common/rangeset.c
> >> @@ -354,19 +354,20 @@ struct rangeset *rangeset_new(char *name,
> >> unsigned int flags,
> >>  }
> >>
> >>  void rangeset_destroy(
> >> -    struct rangeset *r)
> >> +    struct rangeset *r, spinlock_t *lock)
> >>  {
> >>      struct range *x;
> >>
> >>      if ( r == NULL )
> >>          return;
> >>
> >> -    if ( r->domain != NULL )
> >> -    {
> >> -        spin_lock(&r->domain->rangesets_lock);
> >> -        list_del(&r->rangeset_list);
> >> -        spin_unlock(&r->domain->rangesets_lock);
> >> -    }
> >> +    if ( lock )
> >> +        spin_lock(lock);
> >> +
> >> +    list_del(&r->rangeset_list);
> >> +
> >> +    if ( lock )
> >> +        spin_unlock(lock);
> >>
> >>      while ( (x = first_range(r)) != NULL )
> >>          destroy_range(r, x);
> >> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> >> index cd62e6e..3d9c652 100644
> >> --- a/xen/include/xen/domain.h
> >> +++ b/xen/include/xen/domain.h
> >> @@ -111,4 +111,7 @@ void vnuma_destroy(struct vnuma_info *vnuma);
> >>  struct rangeset *domain_rangeset_new(struct domain *d, char *name,
> >>                                       unsigned int flags);
> >>
> >> +void domain_rangeset_destroy(struct domain *d,
> >> +    struct rangeset *r);
> >> +
> >>  #endif /* __XEN_DOMAIN_H__ */
> >> diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
> >> index 395ba62..deed54d 100644
> >> --- a/xen/include/xen/rangeset.h
> >> +++ b/xen/include/xen/rangeset.h
> >> @@ -14,6 +14,7 @@
> >>
> >>  struct domain;
> >>  struct list_head;
> >> +struct spinlock;
> >>  struct rangeset;
> >>
> >>  /*
> >> @@ -37,11 +38,13 @@ 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
> >> + * Destroy a rangeset. Rangeset will take an action to remove itself from
> a
> >> + * list. If a spinlock is provided it will be held during list deletion
> >> + * operation.
> >> + * It is invalid to perform any operation on a rangeset @r
> >>   * after calling rangeset_destroy(r).
> >>   */
> >> -void rangeset_destroy(
> >> -    struct rangeset *r);
> >> +void rangeset_destroy(struct rangeset *r, struct spinlock *lock);
> >>
> >>  /*
> >>   * Set a limit on the number of ranges that may exist in set @r.
> >> --
> >> 2.7.4
> >
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel