On Wed, May 21, 2014 at 2:14 PM, Ryan Wilson <[email protected]> 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 <[email protected]>
>
> ---
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
> [email protected]
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
