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

[Andrii Anisov <andrii.anisov@xxxxxxxxx>]

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