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.
I also noticed the lock annotation may be lacking around group
functions. It would
be nice to add them.
On Mon, May 19, 2014 at 6:20 AM, Ryan Wilson <wr...@nicira.com> wrote:
> Signed-off-by: Ryan Wilson <wr...@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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
