diff mbox

[RFC,2/6] rangeset_destroy() refactoring

Message ID 1487246610-8298-3-git-send-email-andrii.anisov@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrii Anisov Feb. 16, 2017, 12:03 p.m. UTC
From: Andrii Anisov <andrii_anisov@epam.com>

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.

Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
---
 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(-)

Comments

Paul Durrant Feb. 16, 2017, 12:26 p.m. UTC | #1
> -----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 2/6] rangeset_destroy() refactoring
> 
> From: Andrii Anisov <andrii_anisov@epam.com>
> 
> 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@epam.com>
> ---
>  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
Andrii Anisov Feb. 16, 2017, 2:26 p.m. UTC | #2
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.

Sincerely,
Andrii Anisov.


2017-02-16 14:26 GMT+02:00 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 2/6] rangeset_destroy() refactoring
>>
>> From: Andrii Anisov <andrii_anisov@epam.com>
>>
>> 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@epam.com>
>> ---
>>  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
>
Paul Durrant Feb. 16, 2017, 2:29 p.m. UTC | #3
> -----Original Message-----

> From: Andrii Anisov [mailto:andrii.anisov@gmail.com]

> Sent: 16 February 2017 14:26

> To: Paul Durrant <Paul.Durrant@citrix.com>

> Cc: xen-devel@lists.xenproject.org; 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; sstabellini@kernel.org; Tim

> (Xen.org) <tim@xen.org>; Wei Liu <wei.liu2@citrix.com>

> 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@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 2/6] rangeset_destroy() refactoring

> >>

> >> From: Andrii Anisov <andrii_anisov@epam.com>

> >>

> >> 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@epam.com>

> >> ---

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

> >
Andrii Anisov Feb. 16, 2017, 4:22 p.m. UTC | #4
> What use are rangesets if the implementation doesn't control the list/tree? How on earth would you implement an allocation function otherwise?
Just to be on the same page, my understanding of the rangesets is as following:

 - Currently the `struct rangeset` is a list of `ranges`. This list
head is a `range_list` of `struct rangeset`. Currently `range_list`
manipulations are not protected by any locks. IMO this is the core
functionality of the rangeset.

 - Also there is another list head `rangeset_list` inside `struct
rangeset`. It is used to link a rangeset to an external list of
rangesets. This is protected by spinlocks now. IMO this functionality
is odd to the rangeset itself.

Sincerely,
Andrii Anisov.
Paul Durrant Feb. 16, 2017, 4:37 p.m. UTC | #5
> -----Original Message-----

> From: Andrii Anisov [mailto:andrii.anisov@gmail.com]

> Sent: 16 February 2017 16:22

> To: Paul Durrant <Paul.Durrant@citrix.com>

> Cc: xen-devel@lists.xenproject.org; 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; sstabellini@kernel.org; Tim

> (Xen.org) <tim@xen.org>; Wei Liu <wei.liu2@citrix.com>

> Subject: Re: [RFC 2/6] rangeset_destroy() refactoring

> 

> > What use are rangesets if the implementation doesn't control the list/tree?

> How on earth would you implement an allocation function otherwise?

> Just to be on the same page, my understanding of the rangesets is as

> following:

> 

>  - Currently the `struct rangeset` is a list of `ranges`. This list

> head is a `range_list` of `struct rangeset`. Currently `range_list`

> manipulations are not protected by any locks. IMO this is the core

> functionality of the rangeset.

> 

>  - Also there is another list head `rangeset_list` inside `struct

> rangeset`. It is used to link a rangeset to an external list of

> rangesets. This is protected by spinlocks now. IMO this functionality

> is odd to the rangeset itself.


Ok. Thanks for the clarification. Yes, that second list_head does not strictly belong inside the rangeset structure itself. I guess it could live in a 'domain_rangeset' wrapper structure.

  Paul

> 

> Sincerely,

> Andrii Anisov.
diff mbox

Patch

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.