| Message ID | 05D5A265-76C3-45B7-97A1-0F332767C457@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [ovs-dev,1/1] ofproto: Fix ofgroup ref_count in the group stats. | expand |
Bleep bloop. Greetings Pier Luigi Ventre, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Author Pier Luigi Ventre <pl.ventre@gmail.com> needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Pier Luigi Ventre <pl.ventre@gmail.com.> Lines checked: 345, Warnings: 1, Errors: 1 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
Can someone help with this ? Thanks Pier > On Apr 20, 2020, at 4:57 PM, Pier Luigi Ventre <pl.ventre@gmail.com> wrote: > > Hi everyone. > > This patch is a followup to this message sent in ovs-discuss: > https://mail.openvswitch.org/pipermail/ovs-discuss/2020-April/049897.html <https://mail.openvswitch.org/pipermail/ovs-discuss/2020-April/049897.html> > > This is the brief description of the change: > > The patch fixes ofgroup ref_count in the group stats. Previously, only the rules were counted. > However, ofgroups can reference other ofgroups. > > Updates tests that were failing: groups were not created in order. Adds new unit tests for reference count > > Signed-off-by: Pier Luigi Ventre <pl.ventre@gmail.com <mailto:pl.ventre@gmail.com>> > > — > > diff --git a/include/openvswitch/ofp-group.h b/include/openvswitch/ofp-group.h > index cd7af0ebf..e5dd3a9ec 100644 > --- a/include/openvswitch/ofp-group.h > +++ b/include/openvswitch/ofp-group.h > @@ -58,6 +58,9 @@ struct ofputil_bucket { > struct ofpact *ofpacts; /* Series of "struct ofpact"s. */ > size_t ofpacts_len; /* Length of ofpacts, in bytes. */ > > + bool has_groups; /* 'has_groups' is true if 'ofpacts' contains > + * an OFPACT_GROUP action .*/ > + > struct bucket_counter stats; > }; > > diff --git a/lib/ofp-group.c b/lib/ofp-group.c > index bf0f8af54..6cb904f79 100644 > --- a/lib/ofp-group.c > +++ b/lib/ofp-group.c > @@ -1375,6 +1375,10 @@ ofputil_pull_ofp11_buckets(struct ofpbuf *msg, size_t buckets_length, > > bucket->ofpacts = ofpbuf_steal_data(&ofpacts); > bucket->ofpacts_len = ofpacts.size; > + bucket->has_groups = > + (ofpact_find_type_flattened(bucket->ofpacts, OFPACT_GROUP, > + ofpact_end(bucket->ofpacts, bucket->ofpacts_len)) > + != NULL); > ovs_list_push_back(buckets, &bucket->list_node); > } > > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index afecb24cb..b0779996e 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -586,6 +586,7 @@ struct ofgroup { > struct ofputil_group_props props; > > struct rule_collection rules OVS_GUARDED; /* Referring rules. */ > + uint32_t rgroups OVS_GUARDED; /* Referring groups. */ > }; > > struct pkt_stats { > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 0fbd6c380..f31d4160e 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -3075,7 +3075,9 @@ ofproto_group_unref(struct ofgroup *group) > OVS_NO_THREAD_SAFETY_ANALYSIS > { > if (group && ovs_refcount_unref_relaxed(&group->ref_count) == 1) { > + /* There are no rules nor groups still referencing it. */ > ovs_assert(rule_collection_n(&group->rules) == 0); > + ovs_assert(group->rgroups == 0); > ovsrcu_postpone(group_destroy_cb, group); > } > } > @@ -7100,6 +7102,45 @@ group_remove_rule(struct ofgroup *group, struct rule *rule) > rule_collection_remove(&group->rules, rule); > } > > +static void > +group_add_rgroup(struct ofgroup *group) > +{ > + group->rgroups++; > +} > + > +static void > +group_remove_rgroup(struct ofgroup *group) > +{ > + group->rgroups--; > +} > + > +/* Iterates over the buckets and updates the rgroups count > + * in the referenced groups. */ > +static void > +update_group_references(struct ofproto *ofproto, struct ofgroup *ogroup, > + bool add) > +{ > + struct ofputil_bucket *bucket; > + LIST_FOR_EACH (bucket, list_node, &ogroup->buckets) { > + if (bucket->has_groups) { > + /* Iterate over the group actions and update the references */ > + const struct ofpact_group *a; > + OFPACT_FOR_EACH_TYPE_FLATTENED (a, GROUP, bucket->ofpacts, > + bucket->ofpacts_len) { > + struct ofgroup *group; > + group = ofproto_group_lookup(ofproto, a->group_id, > + OVS_VERSION_MAX, false); > + ovs_assert(group != NULL); > + if (add) { > + group_add_rgroup(group); > + } else { > + group_remove_rgroup(group); > + } > + } > + } > + } > +} > + > static void > append_group_stats(struct ofgroup *group, struct ovs_list *replies) > OVS_REQUIRES(ofproto_mutex) > @@ -7112,7 +7153,7 @@ append_group_stats(struct ofgroup *group, struct ovs_list *replies) > ogs.bucket_stats = xmalloc(group->n_buckets * sizeof *ogs.bucket_stats); > > /* Provider sets the packet and byte counts, we do the rest. */ > - ogs.ref_count = rule_collection_n(&group->rules); > + ogs.ref_count = rule_collection_n(&group->rules) + group->rgroups; > ogs.n_buckets = group->n_buckets; > > error = (ofproto->ofproto_class->group_get_stats > @@ -7349,6 +7390,7 @@ init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm, > &(*ofgroup)->props), > &gm->props); > rule_collection_init(&(*ofgroup)->rules); > + *CONST_CAST(uint32_t *, &((*ofgroup)->rgroups)) = 0; > > /* Make group visible from 'version'. */ > (*ofgroup)->versions = VERSIONS_INITIALIZER(version, > @@ -7391,6 +7433,8 @@ add_group_start(struct ofproto *ofproto, struct ofproto_group_mod *ogm) > cmap_insert(&ofproto->groups, &ogm->new_group->cmap_node, > hash_int(ogm->new_group->group_id, 0)); > ofproto->n_groups[ogm->new_group->type]++; > + /* Update group references. */ > + update_group_references(ofproto, ogm->new_group, true); > } > return error; > } > @@ -7551,6 +7595,11 @@ modify_group_start(struct ofproto *ofproto, struct ofproto_group_mod *ogm) > goto out; > } > > + /* Remove all the references from the groups. */ > + update_group_references(ofproto, old_group, false); > + /* Add back all the references without making the diff. */ > + update_group_references(ofproto, new_group, true); > + > /* The group creation time does not change during modification. */ > *CONST_CAST(long long int *, &(new_group->created)) = old_group->created; > *CONST_CAST(long long int *, &(new_group->modified)) = time_msec(); > @@ -7608,6 +7657,7 @@ delete_group_start(struct ofproto *ofproto, ovs_version_t version, > group->being_deleted = true; > > /* Mark all the referring groups for deletion. */ > + /* Why is needed ? */ > delete_flows_start__(ofproto, version, &group->rules); > group_collection_add(groups, group); > versions_set_remove_version(&group->versions, version); > @@ -7633,6 +7683,14 @@ delete_groups_start(struct ofproto *ofproto, struct ofproto_group_mod *ogm) > struct ofgroup *group; > > if (ogm->gm.group_id == OFPG_ALL) { > + /* Unless there is a way to do things in order. > + * To make sure there are no pending references. */ > + CMAP_FOR_EACH (group, cmap_node, &ofproto->groups) { > + if (versions_visible_in_version(&group->versions, ogm->version)) { > + /* Remove all the references for the other groups.*/ > + update_group_references(ofproto, group, false); > + } > + } > CMAP_FOR_EACH (group, cmap_node, &ofproto->groups) { > if (versions_visible_in_version(&group->versions, ogm->version)) { > delete_group_start(ofproto, ogm->version, &ogm->old_groups, > @@ -7640,8 +7698,11 @@ delete_groups_start(struct ofproto *ofproto, struct ofproto_group_mod *ogm) > } > } > } else { > - group = ofproto_group_lookup__(ofproto, ogm->gm.group_id, ogm->version); > + group = ofproto_group_lookup__(ofproto, ogm->gm.group_id, > + ogm->version); > if (group) { > + /* Remove all the references for the other groups.*/ > + update_group_references(ofproto, group, false); > delete_group_start(ofproto, ogm->version, &ogm->old_groups, group); > } > } > @@ -7708,16 +7769,24 @@ ofproto_group_mod_revert(struct ofproto *ofproto, > ovs_assert(group_collection_n(&ogm->old_groups) == 1); > /* Transfer rules back. */ > rule_collection_move(&old_group->rules, &new_group->rules); > + /* Remove the new references and restore the old ones. */ > + update_group_references(ofproto, new_group, false); > + update_group_references(ofproto, old_group, true); > } else { > old_group->being_deleted = false; > /* Revert rule deletion. */ > delete_flows_revert__(ofproto, &old_group->rules); > + /* Restore the old references. */ > + update_group_references(ofproto, old_group, true); > } > /* Restore visibility. */ > versions_set_remove_version(&old_group->versions, > OVS_VERSION_NOT_REMOVED); > } > + /* Revert new added groups. */ > if (new_group) { > + /* Remove the new references. */ > + update_group_references(ofproto, new_group, false); > /* Remove the new group immediately. It was never visible to > * lookups. */ > cmap_remove(&ofproto->groups, &new_group->cmap_node, > diff --git a/tests/nsh.at <http://nsh.at/> b/tests/nsh.at <http://nsh.at/> > index d5c772ff0..99d0a391c 100644 > --- a/tests/nsh.at <http://nsh.at/> > +++ b/tests/nsh.at <http://nsh.at/> > @@ -299,9 +299,9 @@ AT_DATA([flows.txt], [dnl > ]) > > AT_DATA([groups.txt], [dnl > - add group_id=100,type=indirect,bucket=actions=encap(nsh(md_type=1)),set_field:0x1234->nsh_spi,set_field:0x11223344->nsh_c1,group:200 > - add group_id=200,type=indirect,bucket=actions=encap(nsh(md_type=1)),set_field:0x5678->nsh_spi,set_field:0x55667788->nsh_c1,group:300 > add group_id=300,type=indirect,bucket=actions=encap(ethernet),set_field:11:22:33:44:55:66->dl_dst,3 > + add group_id=200,type=indirect,bucket=actions=encap(nsh(md_type=1)),set_field:0x5678->nsh_spi,set_field:0x55667788->nsh_c1,group:300 > + add group_id=100,type=indirect,bucket=actions=encap(nsh(md_type=1)),set_field:0x1234->nsh_spi,set_field:0x11223344->nsh_c1,group:200 > ]) > > AT_CHECK([ > diff --git a/tests/ofproto-dpif.at <http://ofproto-dpif.at/> b/tests/ofproto-dpif.at <http://ofproto-dpif.at/> > index d444cf084..335abf4b6 100644 > --- a/tests/ofproto-dpif.at <http://ofproto-dpif.at/> > +++ b/tests/ofproto-dpif.at <http://ofproto-dpif.at/> > @@ -318,8 +318,8 @@ AT_CLEANUP > AT_SETUP([ofproto-dpif - group chaining]) > OVS_VSWITCHD_START > add_of_ports br0 1 10 11 > -AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 'group_id=1234,type=all,bucket=set_field:192.168.3.90->ip_src,group:123,bucket=output:11']) > AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 'group_id=123,type=all,bucket=output:10']) > +AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 'group_id=1234,type=all,bucket=set_field:192.168.3.90->ip_src,group:123,bucket=output:11']) > AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 'ip actions=group:1234']) > AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=1,nw_tos=0,nw_ttl=128,icmp_type=8,icmp_code=0'], [0], [stdout]) > AT_CHECK([tail -1 stdout], [0], > diff --git a/tests/ofproto.at <http://ofproto.at/> b/tests/ofproto.at <http://ofproto.at/> > index 76a3be44d..8dbef9559 100644 > --- a/tests/ofproto.at <http://ofproto.at/> > +++ b/tests/ofproto.at <http://ofproto.at/> > @@ -6447,3 +6447,71 @@ verify_deleted > > OVS_VSWITCHD_STOP(["/<invalid/d"]) > AT_CLEANUP > + > +# Check the reference count of the groups using stats reply > +AT_SETUP([ofproto - flow ref_count]) > +OVS_VSWITCHD_START > +AT_DATA([groups.txt], [dnl > +group_id=1233,type=all,bucket=output:10 > +]) > +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn add-groups br0 groups.txt]) > +AT_DATA([flows.txt], [dnl > +tcp actions=group:1233 > +]) > +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn add-flows br0 flows.txt]) > +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn dump-flows br0 | ofctl_strip | sort], > +[0], [dnl > + tcp actions=group:1233 > +OFPST_FLOW reply (OF1.3): > +]) > +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn dump-group-stats br0], [0], [stdout]) > +AT_CHECK([strip_xids < stdout | sed 's/duration=[[0-9.]]*s/duration=?s/'], [0], [dnl > +OFPST_GROUP reply (OF1.3): > + group_id=1233,duration=?s,ref_count=1,packet_count=0,byte_count=0,bucket0:packet_count=0,byte_count=0 > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > +AT_SETUP([ofproto - group ref_count]) > +OVS_VSWITCHD_START > +AT_DATA([groups.txt], [dnl > +group_id=1233,type=indirect,bucket=output:10 > +group_id=1234,type=all,bucket=group:1233 > +]) > +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn add-groups br0 groups.txt]) > +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn dump-group-stats br0], [0], [stdout]) > +AT_CHECK([strip_xids < stdout | sed 's/duration=[[0-9.]]*s/duration=?s/' | sort], [0], [dnl > + group_id=1233,duration=?s,ref_count=1,packet_count=0,byte_count=0,bucket0:packet_count=0,byte_count=0 > + group_id=1234,duration=?s,ref_count=0,packet_count=0,byte_count=0,bucket0:packet_count=0,byte_count=0 > +OFPST_GROUP reply (OF1.3): > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > +AT_SETUP([ofproto - mixed ref_count]) > +OVS_VSWITCHD_START > +AT_DATA([groups.txt], [dnl > +group_id=1233,type=indirect,bucket=output:10 > +group_id=1234,type=all,bucket=group:1233 > +]) > +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn add-groups br0 groups.txt]) > +AT_DATA([flows.txt], [dnl > +tcp actions=group:1233 > +]) > +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn add-flows br0 flows.txt]) > +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn dump-flows br0 | ofctl_strip | sort], > +[0], [dnl > + tcp actions=group:1233 > +OFPST_FLOW reply (OF1.3): > +]) > +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn dump-group-stats br0], [0], [stdout]) > +AT_CHECK([strip_xids < stdout | sed 's/duration=[[0-9.]]*s/duration=?s/' | sort], [0], [dnl > + group_id=1233,duration=?s,ref_count=2,packet_count=0,byte_count=0,bucket0:packet_count=0,byte_count=0 > + group_id=1234,duration=?s,ref_count=0,packet_count=0,byte_count=0,bucket0:packet_count=0,byte_count=0 > +OFPST_GROUP reply (OF1.3): > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > > — > > Thanks > Pier
diff --git a/include/openvswitch/ofp-group.h b/include/openvswitch/ofp-group.h index cd7af0ebf..e5dd3a9ec 100644 --- a/include/openvswitch/ofp-group.h +++ b/include/openvswitch/ofp-group.h @@ -58,6 +58,9 @@ struct ofputil_bucket { struct ofpact *ofpacts; /* Series of "struct ofpact"s. */ size_t ofpacts_len; /* Length of ofpacts, in bytes. */ + bool has_groups; /* 'has_groups' is true if 'ofpacts' contains + * an OFPACT_GROUP action .*/ + struct bucket_counter stats; }; diff --git a/lib/ofp-group.c b/lib/ofp-group.c index bf0f8af54..6cb904f79 100644 --- a/lib/ofp-group.c +++ b/lib/ofp-group.c @@ -1375,6 +1375,10 @@ ofputil_pull_ofp11_buckets(struct ofpbuf *msg, size_t buckets_length, bucket->ofpacts = ofpbuf_steal_data(&ofpacts); bucket->ofpacts_len = ofpacts.size; + bucket->has_groups = + (ofpact_find_type_flattened(bucket->ofpacts, OFPACT_GROUP, + ofpact_end(bucket->ofpacts, bucket->ofpacts_len)) + != NULL); ovs_list_push_back(buckets, &bucket->list_node); } diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index afecb24cb..b0779996e 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -586,6 +586,7 @@ struct ofgroup { struct ofputil_group_props props; struct rule_collection rules OVS_GUARDED; /* Referring rules. */ + uint32_t rgroups OVS_GUARDED; /* Referring groups. */ }; struct pkt_stats { diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 0fbd6c380..f31d4160e 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -3075,7 +3075,9 @@ ofproto_group_unref(struct ofgroup *group) OVS_NO_THREAD_SAFETY_ANALYSIS { if (group && ovs_refcount_unref_relaxed(&group->ref_count) == 1) { + /* There are no rules nor groups still referencing it. */ ovs_assert(rule_collection_n(&group->rules) == 0); + ovs_assert(group->rgroups == 0); ovsrcu_postpone(group_destroy_cb, group); } } @@ -7100,6 +7102,45 @@ group_remove_rule(struct ofgroup *group, struct rule *rule) rule_collection_remove(&group->rules, rule); } +static void +group_add_rgroup(struct ofgroup *group) +{ + group->rgroups++; +} + +static void +group_remove_rgroup(struct ofgroup *group) +{ + group->rgroups--; +} + +/* Iterates over the buckets and updates the rgroups count + * in the referenced groups. */ +static void +update_group_references(struct ofproto *ofproto, struct ofgroup *ogroup, + bool add) +{ + struct ofputil_bucket *bucket; + LIST_FOR_EACH (bucket, list_node, &ogroup->buckets) { + if (bucket->has_groups) { + /* Iterate over the group actions and update the references */ + const struct ofpact_group *a; + OFPACT_FOR_EACH_TYPE_FLATTENED (a, GROUP, bucket->ofpacts, + bucket->ofpacts_len) { + struct ofgroup *group; + group = ofproto_group_lookup(ofproto, a->group_id, + OVS_VERSION_MAX, false); + ovs_assert(group != NULL); + if (add) { + group_add_rgroup(group); + } else { + group_remove_rgroup(group); + } + } + } + } +} + static void append_group_stats(struct ofgroup *group, struct ovs_list *replies) OVS_REQUIRES(ofproto_mutex) @@ -7112,7 +7153,7 @@ append_group_stats(struct ofgroup *group, struct ovs_list *replies) ogs.bucket_stats = xmalloc(group->n_buckets * sizeof *ogs.bucket_stats); /* Provider sets the packet and byte counts, we do the rest. */ - ogs.ref_count = rule_collection_n(&group->rules); + ogs.ref_count = rule_collection_n(&group->rules) + group->rgroups; ogs.n_buckets = group->n_buckets; error = (ofproto->ofproto_class->group_get_stats @@ -7349,6 +7390,7 @@ init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm, &(*ofgroup)->props), &gm->props); rule_collection_init(&(*ofgroup)->rules); + *CONST_CAST(uint32_t *, &((*ofgroup)->rgroups)) = 0; /* Make group visible from 'version'. */ (*ofgroup)->versions = VERSIONS_INITIALIZER(version, @@ -7391,6 +7433,8 @@ add_group_start(struct ofproto *ofproto, struct ofproto_group_mod *ogm) cmap_insert(&ofproto->groups, &ogm->new_group->cmap_node, hash_int(ogm->new_group->group_id, 0)); ofproto->n_groups[ogm->new_group->type]++; + /* Update group references. */ + update_group_references(ofproto, ogm->new_group, true); } return error; } @@ -7551,6 +7595,11 @@ modify_group_start(struct ofproto *ofproto, struct ofproto_group_mod *ogm) goto out; } + /* Remove all the references from the groups. */ + update_group_references(ofproto, old_group, false); + /* Add back all the references without making the diff. */ + update_group_references(ofproto, new_group, true); + /* The group creation time does not change during modification. */ *CONST_CAST(long long int *, &(new_group->created)) = old_group->created; *CONST_CAST(long long int *, &(new_group->modified)) = time_msec(); @@ -7608,6 +7657,7 @@ delete_group_start(struct ofproto *ofproto, ovs_version_t version, group->being_deleted = true; /* Mark all the referring groups for deletion. */ + /* Why is needed ? */ delete_flows_start__(ofproto, version, &group->rules); group_collection_add(groups, group); versions_set_remove_version(&group->versions, version); @@ -7633,6 +7683,14 @@ delete_groups_start(struct ofproto *ofproto, struct ofproto_group_mod *ogm) struct ofgroup *group; if (ogm->gm.group_id == OFPG_ALL) { + /* Unless there is a way to do things in order. + * To make sure there are no pending references. */ + CMAP_FOR_EACH (group, cmap_node, &ofproto->groups) { + if (versions_visible_in_version(&group->versions, ogm->version)) { + /* Remove all the references for the other groups.*/ + update_group_references(ofproto, group, false); + } + } CMAP_FOR_EACH (group, cmap_node, &ofproto->groups) { if (versions_visible_in_version(&group->versions, ogm->version)) { delete_group_start(ofproto, ogm->version, &ogm->old_groups, @@ -7640,8 +7698,11 @@ delete_groups_start(struct ofproto *ofproto, struct ofproto_group_mod *ogm) } } } else { - group = ofproto_group_lookup__(ofproto, ogm->gm.group_id, ogm->version); + group = ofproto_group_lookup__(ofproto, ogm->gm.group_id, + ogm->version); if (group) { + /* Remove all the references for the other groups.*/ + update_group_references(ofproto, group, false); delete_group_start(ofproto, ogm->version, &ogm->old_groups, group); } } @@ -7708,16 +7769,24 @@ ofproto_group_mod_revert(struct ofproto *ofproto, ovs_assert(group_collection_n(&ogm->old_groups) == 1); /* Transfer rules back. */ rule_collection_move(&old_group->rules, &new_group->rules); + /* Remove the new references and restore the old ones. */ + update_group_references(ofproto, new_group, false); + update_group_references(ofproto, old_group, true); } else { old_group->being_deleted = false; /* Revert rule deletion. */ delete_flows_revert__(ofproto, &old_group->rules); + /* Restore the old references. */ + update_group_references(ofproto, old_group, true); } /* Restore visibility. */ versions_set_remove_version(&old_group->versions, OVS_VERSION_NOT_REMOVED); } + /* Revert new added groups. */ if (new_group) { + /* Remove the new references. */ + update_group_references(ofproto, new_group, false); /* Remove the new group immediately. It was never visible to * lookups. */ cmap_remove(&ofproto->groups, &new_group->cmap_node, diff --git a/tests/nsh.at b/tests/nsh.at index d5c772ff0..99d0a391c 100644 --- a/tests/nsh.at +++ b/tests/nsh.at @@ -299,9 +299,9 @@ AT_DATA([flows.txt], [dnl ]) AT_DATA([groups.txt], [dnl - add group_id=100,type=indirect,bucket=actions=encap(nsh(md_type=1)),set_field:0x1234->nsh_spi,set_field:0x11223344->nsh_c1,group:200 - add group_id=200,type=indirect,bucket=actions=encap(nsh(md_type=1)),set_field:0x5678->nsh_spi,set_field:0x55667788->nsh_c1,group:300 add group_id=300,type=indirect,bucket=actions=encap(ethernet),set_field:11:22:33:44:55:66->dl_dst,3 + add group_id=200,type=indirect,bucket=actions=encap(nsh(md_type=1)),set_field:0x5678->nsh_spi,set_field:0x55667788->nsh_c1,group:300 + add group_id=100,type=indirect,bucket=actions=encap(nsh(md_type=1)),set_field:0x1234->nsh_spi,set_field:0x11223344->nsh_c1,group:200 ]) AT_CHECK([ diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index d444cf084..335abf4b6 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -318,8 +318,8 @@ AT_CLEANUP AT_SETUP([ofproto-dpif - group chaining]) OVS_VSWITCHD_START add_of_ports br0 1 10 11 -AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 'group_id=1234,type=all,bucket=set_field:192.168.3.90->ip_src,group:123,bucket=output:11']) AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 'group_id=123,type=all,bucket=output:10']) +AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 'group_id=1234,type=all,bucket=set_field:192.168.3.90->ip_src,group:123,bucket=output:11']) AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 'ip actions=group:1234']) AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=1,nw_tos=0,nw_ttl=128,icmp_type=8,icmp_code=0'], [0], [stdout]) AT_CHECK([tail -1 stdout], [0], diff --git a/tests/ofproto.at b/tests/ofproto.at index 76a3be44d..8dbef9559 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -6447,3 +6447,71 @@ verify_deleted OVS_VSWITCHD_STOP(["/<invalid/d"]) AT_CLEANUP + +# Check the reference count of the groups using stats reply +AT_SETUP([ofproto - flow ref_count]) +OVS_VSWITCHD_START +AT_DATA([groups.txt], [dnl +group_id=1233,type=all,bucket=output:10 +]) +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn add-groups br0 groups.txt]) +AT_DATA([flows.txt], [dnl +tcp actions=group:1233 +]) +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn add-flows br0 flows.txt]) +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn dump-flows br0 | ofctl_strip | sort], +[0], [dnl + tcp actions=group:1233 +OFPST_FLOW reply (OF1.3): +]) +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn dump-group-stats br0], [0], [stdout]) +AT_CHECK([strip_xids < stdout | sed 's/duration=[[0-9.]]*s/duration=?s/'], [0], [dnl +OFPST_GROUP reply (OF1.3): + group_id=1233,duration=?s,ref_count=1,packet_count=0,byte_count=0,bucket0:packet_count=0,byte_count=0 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + +AT_SETUP([ofproto - group ref_count]) +OVS_VSWITCHD_START +AT_DATA([groups.txt], [dnl +group_id=1233,type=indirect,bucket=output:10 +group_id=1234,type=all,bucket=group:1233 +]) +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn add-groups br0 groups.txt]) +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn dump-group-stats br0], [0], [stdout]) +AT_CHECK([strip_xids < stdout | sed 's/duration=[[0-9.]]*s/duration=?s/' | sort], [0], [dnl + group_id=1233,duration=?s,ref_count=1,packet_count=0,byte_count=0,bucket0:packet_count=0,byte_count=0 + group_id=1234,duration=?s,ref_count=0,packet_count=0,byte_count=0,bucket0:packet_count=0,byte_count=0 +OFPST_GROUP reply (OF1.3): +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + +AT_SETUP([ofproto - mixed ref_count]) +OVS_VSWITCHD_START +AT_DATA([groups.txt], [dnl +group_id=1233,type=indirect,bucket=output:10 +group_id=1234,type=all,bucket=group:1233 +]) +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn add-groups br0 groups.txt]) +AT_DATA([flows.txt], [dnl +tcp actions=group:1233 +]) +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn add-flows br0 flows.txt]) +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn dump-flows br0 | ofctl_strip | sort], +[0], [dnl + tcp actions=group:1233 +OFPST_FLOW reply (OF1.3): +]) +AT_CHECK([ovs-ofctl -O OpenFlow13 -vwarn dump-group-stats br0], [0], [stdout]) +AT_CHECK([strip_xids < stdout | sed 's/duration=[[0-9.]]*s/duration=?s/' | sort], [0], [dnl + group_id=1233,duration=?s,ref_count=2,packet_count=0,byte_count=0,bucket0:packet_count=0,byte_count=0 + group_id=1234,duration=?s,ref_count=0,packet_count=0,byte_count=0,bucket0:packet_count=0,byte_count=0 +OFPST_GROUP reply (OF1.3): +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP
Hi everyone. This patch is a followup to this message sent in ovs-discuss: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-April/049897.html <https://mail.openvswitch.org/pipermail/ovs-discuss/2020-April/049897.html> This is the brief description of the change: The patch fixes ofgroup ref_count in the group stats. Previously, only the rules were counted. However, ofgroups can reference other ofgroups. Updates tests that were failing: groups were not created in order. Adds new unit tests for reference count Signed-off-by: Pier Luigi Ventre <pl.ventre@gmail.com.> — — Thanks Pier