diff mbox series

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

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

Commit Message

Pier Luigi Ventre April 20, 2020, 2:57 p.m. UTC
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

Comments

0-day Robot April 20, 2020, 4 p.m. UTC | #1
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
Pier Luigi Ventre April 30, 2020, 3:34 p.m. UTC | #2
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 mbox series

Patch

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