[ovs-dev] [PATCH v3 1/3] ofproto: Add reference count for Openflow groups.

Ryan Wilson 76511 wryan at vmware.com
Tue May 20 22:05:39 UTC 2014


Hey Andy,

>At a high level,  ofgroup struct current has rwlock that essentially
>solving
>the same problem as the ref count proposed in this patch. It would be
>better
>it seems confusing if we use both method together.
>
>Looking at the code, I'd think extending rwlock to cover xlate
>fastpath is the most
>straight forward approach.

The reason I use the reference count is because of the following situation:
- The main thread and xlate cache both have pointers to the group.
- The main thread deletes the group and frees memory.
- However, the xlate cache still has a pointer to the group and if a
handler is run prior to the revalidator clearing the xlate cache, a
handler could write to freed memory.

Thus, similar to netdev, I use a ref here, so the memory is not freed
until both the cache and main thread are done with the group. Again, like
netdev_mutex, the groups' rwlock is to serialize access to the group and
bucket stats. Plus the rwlock won't work once the group is freed.

Let me know if you have a cleaner suggestion to deal with this issue; I
agree, I would prefer to use just a lock and / or a ref count.

>I also noticed the lock annotation may be lacking around group
>functions. It would
>be nice to add them.

I'll add these annotations in my next patch.

Thanks for the review,

Ryan

>
>On Mon, May 19, 2014 at 6:20 AM, Ryan Wilson <wryan at nicira.com> wrote:
>> Signed-off-by: Ryan Wilson <wryan at nicira.com>
>>
>> ---
>> v2: Fixed bug with group stats all buckets, cleaned up ofgroup unref
>>code,
>>     added Andy Zhou's test for more complete test coverage
>> v3: Split group reference count, support for group and bucket stats
>>into 2
>> patches. Commit Andy Zhou's test patch separately.
>> ---
>>  ofproto/ofproto-dpif.h     |   16 ++++++++++++++++
>>  ofproto/ofproto-provider.h |    8 ++++++++
>>  ofproto/ofproto.c          |   32 +++++++++++++++++++++++++++-----
>>  3 files changed, 51 insertions(+), 5 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
>> index e49616e..345117a 100644
>> --- a/ofproto/ofproto-dpif.h
>> +++ b/ofproto/ofproto-dpif.h
>> @@ -232,6 +232,22 @@ BUILD_ASSERT_DECL(N_TABLES >= 2 && N_TABLES <=
>>255);
>>
>>  /* struct rule_dpif has struct rule as it's first member. */
>>  #define RULE_CAST(RULE) ((struct rule *)RULE)
>> +#define GROUP_CAST(GROUP) ((struct ofgroup *)GROUP)
>> +
>> +static inline struct group_dpif* group_dpif_ref(struct group_dpif
>>*group)
>> +{
>> +    if (group) {
>> +        ofproto_group_ref(GROUP_CAST(group));
>> +    }
>> +    return group;
>> +}
>> +
>> +static inline void group_dpif_unref(struct group_dpif *group)
>> +{
>> +    if (group) {
>> +        ofproto_group_unref(GROUP_CAST(group));
>> +    }
>> +}
>>
>>  static inline void rule_dpif_ref(struct rule_dpif *rule)
>>  {
>> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
>> index 141adec..209f6f9 100644
>> --- a/ofproto/ofproto-provider.h
>> +++ b/ofproto/ofproto-provider.h
>> @@ -488,6 +488,11 @@ struct ofgroup {
>>      uint32_t group_id;
>>      enum ofp11_group_type type; /* One of OFPGT_*. */
>>
>> +    /* Number of references.
>> +     * The classifier owns one reference.
>> +     * The xlate cache may also hold a reference. */
>> +    struct ovs_refcount ref_count;
>> +
>>      long long int created;      /* Creation time. */
>>      long long int modified;     /* Time of last modification. */
>>
>> @@ -502,6 +507,9 @@ bool ofproto_group_lookup(const struct ofproto
>>*ofproto, uint32_t group_id,
>>  void ofproto_group_release(struct ofgroup *group)
>>      OVS_RELEASES(group->rwlock);
>>
>> +void ofproto_group_ref(struct ofgroup *);
>> +void ofproto_group_unref(struct ofgroup *);
>> +
>>  /* ofproto class structure, to be defined by each ofproto
>>implementation.
>>   *
>>   *
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 9b26da7..b5a59f8 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -2647,6 +2647,31 @@ ofproto_rule_unref(struct rule *rule)
>>      }
>>  }
>>
>> +static void
>> +group_destroy_cb(struct ofgroup *group)
>> +{
>> +    group->ofproto->ofproto_class->group_destruct(group);
>> +    ofputil_bucket_list_destroy(&group->buckets);
>> +    ovs_rwlock_destroy(&group->rwlock);
>> +    group->ofproto->ofproto_class->group_dealloc(group);
>> +}
>> +
>> +void
>> +ofproto_group_ref(struct ofgroup *group)
>> +{
>> +    if (group) {
>> +        ovs_refcount_ref(&group->ref_count);
>> +    }
>> +}
>> +
>> +void
>> +ofproto_group_unref(struct ofgroup *group)
>> +{
>> +    if (group && ovs_refcount_unref(&group->ref_count) == 1) {
>> +        ovsrcu_postpone(group_destroy_cb, group);
>> +    }
>> +}
>> +
>>  static uint32_t get_provider_meter_id(const struct ofproto *,
>>                                        uint32_t of_meter_id);
>>
>> @@ -5622,6 +5647,7 @@ add_group(struct ofproto *ofproto, struct
>>ofputil_group_mod *gm)
>>      ofgroup->group_id = gm->group_id;
>>      ofgroup->type     = gm->type;
>>      ofgroup->created = ofgroup->modified = time_msec();
>> +    ovs_refcount_init(&ofgroup->ref_count);
>>
>>      list_move(&ofgroup->buckets, &gm->buckets);
>>      ofgroup->n_buckets = list_size(&ofgroup->buckets);
>> @@ -5752,12 +5778,8 @@ delete_group__(struct ofproto *ofproto, struct
>>ofgroup *ofgroup)
>>      /* No-one can find this group any more. */
>>      ofproto->n_groups[ofgroup->type]--;
>>      ovs_rwlock_unlock(&ofproto->groups_rwlock);
>> -
>> -    ofproto->ofproto_class->group_destruct(ofgroup);
>> -    ofputil_bucket_list_destroy(&ofgroup->buckets);
>>      ovs_rwlock_unlock(&ofgroup->rwlock);
>> -    ovs_rwlock_destroy(&ofgroup->rwlock);
>> -    ofproto->ofproto_class->group_dealloc(ofgroup);
>> +    ofproto_group_unref(ofgroup);
>>  }
>>
>>  /* Implements OFPGC_DELETE. */
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> 
>>https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman
>>/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=TfBS78Vw3dzttvXidhbff
>>g%3D%3D%0A&m=yJOSSP0gTPFhsJ2%2FdAJK7eyDBE8f7He9ElabH7ctSb4%3D%0A&s=c8eb05
>>9001930596833f500f75ed0cdf49802abfe3dac0d98729da1cdd7f905f
>_______________________________________________
>dev mailing list
>dev at openvswitch.org
>https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/
>listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=TfBS78Vw3dzttvXidhbffg%
>3D%3D%0A&m=yJOSSP0gTPFhsJ2%2FdAJK7eyDBE8f7He9ElabH7ctSb4%3D%0A&s=c8eb05900
>1930596833f500f75ed0cdf49802abfe3dac0d98729da1cdd7f905f




More information about the dev mailing list