[ovs-dev,1/1] ofproto: Fix ofgroup ref_count in the group stats.

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

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

On 4/20/20 16:57, Pier Luigi Ventre 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.>

Hi Pier,

I was looking through old patches in our patchwork instance and
found this one.  Sorry for not looking at it earlier.

It does look like an issue in our implementation for reference
counters indeed.

There are few issues with the implementation though.  The code
doesn't check if the group referenced in the bucket exists
while adding a new group.  This will skew the counting down the
road when the group will be added later.
Also, there is no mechanism to protect us from removing the
group that is still referenced by some other group.  OVS will
just assert instead.  That is not good.

One way to fix these issues is to actually take real reference
and forbid adding groups that reference non-existent groups
or removing groups that are still referenced.  I'm not sure
though how that align with the OpenFlow spec and that will
for sure impose extra restrictions on the order of operations
for existing users.

Another option is to find a way to count things while keeping
in mind that some groups may not actually exist.

Best regards, Ilya Maximets.