[ovs-dev] [PATCH v6] ofproto: Add reference count for Openflow groups.

Andy Zhou azhou at nicira.com
Wed May 21 22:43:14 UTC 2014


On Wed, May 21, 2014 at 2:14 PM, Ryan Wilson <wryan at nicira.com> wrote:
> When adding support for OpenFlow group and bucket stats, a group entry is added
> to the xlate_cache. If the main thread removes the group from an ofproto, we
> need to guarantee that the group remains accessible to users of
> xlate_group_actions and the xlate_cache, as the xlate_cache will not be cleaned
> up until the corresponding datapath flows are revalidated.
>
> To make modify_group compatible with group reference counting, the group is
> re-created and then replaces the old group in ofproto's ofgroup hash map. Thus,
> the group is never altered while users of the xlate module hold a pointer to the
> group. This also eliminates the need for ofgroup's lock as all properties of
> ofgroup are read-only.
>
> Signed-off-by: Ryan Wilson <wryan at nicira.com>
>
> ---

Look good at high level.  Comments inline:


>  static inline void rule_dpif_ref(struct rule_dpif *rule)
>  {
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 141adec..e2af522 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -474,20 +474,22 @@ bool ofoperation_has_out_port(const struct ofoperation *, ofp_port_t out_port)
>   * With few exceptions, ofproto implementations may look at these fields but
>   * should not modify them. */
>  struct ofgroup {
> -    /* The rwlock is used to prevent groups from being deleted while child
> -     * threads are using them to xlate flows.  A read lock means the
> -     * group is currently being used.  A write lock means the group is
> -     * in the process of being deleted or updated.  Note that since
> -     * a read lock on the groups container is held while searching, and
> -     * a group is ever write locked only while holding a write lock
> -     * on the container, the user's of groups will never face a group
> -     * in the write locked state. */
> -    struct ovs_rwlock rwlock OVS_ACQ_AFTER(ofproto_mutex);
>      struct hmap_node hmap_node; /* In struct ofproto's "groups" hmap. */
Add OVS_GUARDED ?
>      struct ofproto *ofproto;    /* The ofproto that contains this group. */
>      uint32_t group_id;
>      enum ofp11_group_type type; /* One of OFPGT_*. */
>
> +    /* Number of references.
> +     *
> +     * This is needed to keep track of references to the group in the xlate
> +     * module.
> +     *
> +     * If the main thread removes the group from an ofproto, we need to
> +     * guarantee that the group remains accessible to users of
> +     * xlate_group_actions and the xlate_cache, as the xlate_cache will not be
> +     * cleaned up until the corresponding datapath flows are revalidated. */
> +    struct ovs_refcount ref_count;
How about move it right below hmap_node, it now protects all the fields below.
The comment above needs update about we don't change the fields once
constructed.

> +
>      long long int created;      /* Creation time. */
>      long long int modified;     /* Time of last modification. */
All fields above till ofproto (except ref_count) can be made const to
make sure they
are not being updated during runtime,init_group() can use CONST_CAST
to initialize them.
>
> @@ -496,11 +498,10 @@ struct ofgroup {
>  };
>
> +
> +void
> +ofproto_group_unref(struct ofgroup *group)
> +{
> +    if (group && ovs_refcount_unref(&group->ref_count) == 1) {
> +        ovsrcu_postpone(group_destroy_cb, group);
Why do we need ovsrcu_postpone? It seems we can just do the destroy in place.
> +    }
> +}
> +
>  static void
>  append_group_stats(struct ofgroup *group, struct list *replies)
> -    OVS_REQ_RDLOCK(group->rwlock)
>  {
>      struct ofputil_group_stats ogs;
>      struct ofproto *ofproto = group->ofproto;
> @@ -5476,15 +5494,15 @@ handle_group_request(struct ofconn *ofconn,
>      if (group_id == OFPG_ALL) {
>          ovs_rwlock_rdlock(&ofproto->groups_rwlock);
>          HMAP_FOR_EACH (group, hmap_node, &ofproto->groups) {
> -            ovs_rwlock_rdlock(&group->rwlock);
> +            ofproto_group_ref(group);
>              cb(group, &replies);
> -            ovs_rwlock_unlock(&group->rwlock);
> +            ofproto_group_unref(group);
Do we need ofproto_group_ref()/unref() here? Not sure why group would
disappear while holding the read lock.
>

>
> +static void
> +replace_group(struct ofproto *ofproto, struct ofgroup *ofgroup,
> +              struct ofgroup *new_ofgroup)
It can use a lock annotation for the function. Personally, I think
putting the logic
in modify_group() makes it more readable.
> +{
> +    hmap_remove(&ofproto->groups, &ofgroup->hmap_node);
> +    hmap_insert(&ofproto->groups, &new_ofgroup->hmap_node,
> +                hash_int(new_ofgroup->group_id, 0));
> +    if (ofgroup->type != new_ofgroup->type) {
> +        ofproto->n_groups[ofgroup->type]--;
> +        ofproto->n_groups[new_ofgroup->type]++;
> +    }
> +    ofproto_group_unref(ofgroup);
Will this cause a unref be called twice?
> +}
> +
>  /* Implements OFPFC_MODIFY.  Returns 0 on success or an OpenFlow error code on
>   * failure.
>   *
> + * Note that the group is re-created and then replaces the old group in
> + * ofproto's ofgroup hash map. Thus, the group is never altered while users of
> + * the xlate module hold a pointer to the group.
> + *
>   * 'ofconn' is used to retrieve the packet buffer specified in fm->buffer_id,
>   * if any. */
>  static enum ofperr
>  modify_group(struct ofproto *ofproto, struct ofputil_group_mod *gm)
> -
> -    error = ofproto->ofproto_class->group_modify(ofgroup, victim);
> -    if (!error) {
> -        ofputil_bucket_list_destroy(&victim->buckets);
> -        ofproto->n_groups[victim->type]--;
> -        ofproto->n_groups[ofgroup->type]++;
> -        ofgroup->modified = time_msec();
> +        ofproto_group_unref(new_ofgroup);
If we fall into this error path, should we _not_ unref the existing ofproto?
>      } else {
> -        ofputil_bucket_list_destroy(&ofgroup->buckets);
> +        /* The group creation time does not change during modification. */
> +        new_ofgroup->created = ofgroup->created;
>
Should we also update the modified time stamp here?
_______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list