[ovs-dev,02/13] ofproto: Lockless group lookups.

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index fee11aa..39b4bc9 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1502,7 +1502,8 @@  group_is_alive(const struct xlate_ctx *ctx, uint32_t group_id, int depth)
 {
     struct group_dpif *group;
 
-    if (group_dpif_lookup(ctx->xbridge->ofproto, group_id, &group)) {
+    group = group_dpif_lookup(ctx->xbridge->ofproto, group_id);
+    if (group) {
         struct ofputil_bucket *bucket;
 
         bucket = group_first_live_bucket(ctx, group, depth);
@@ -1541,7 +1542,7 @@  group_first_live_bucket(const struct xlate_ctx *ctx,
     struct ofputil_bucket *bucket;
     const struct ovs_list *buckets;
 
-    group_dpif_get_buckets(group, &buckets);
+    buckets = group_dpif_get_buckets(group);
     LIST_FOR_EACH (bucket, list_node, buckets) {
         if (bucket_is_alive(ctx, bucket, depth)) {
             return bucket;
@@ -1562,7 +1563,7 @@  group_best_live_bucket(const struct xlate_ctx *ctx,
     struct ofputil_bucket *bucket;
     const struct ovs_list *buckets;
 
-    group_dpif_get_buckets(group, &buckets);
+    buckets = group_dpif_get_buckets(group);
     LIST_FOR_EACH (bucket, list_node, buckets) {
         if (bucket_is_alive(ctx, bucket, 0)) {
             uint32_t score =
@@ -3448,8 +3449,7 @@  xlate_all_group(struct xlate_ctx *ctx, struct group_dpif *group)
     struct ofputil_bucket *bucket;
     const struct ovs_list *buckets;
 
-    group_dpif_get_buckets(group, &buckets);
-
+    buckets = group_dpif_get_buckets(group);
     LIST_FOR_EACH (bucket, list_node, buckets) {
         xlate_group_bucket(ctx, bucket);
     }
@@ -3605,14 +3605,13 @@  xlate_group_action(struct xlate_ctx *ctx, uint32_t group_id)
 {
     if (xlate_resubmit_resource_check(ctx)) {
         struct group_dpif *group;
-        bool got_group;
 
-        got_group = group_dpif_lookup(ctx->xbridge->ofproto, group_id, &group);
-        if (got_group) {
-            xlate_group_action__(ctx, group);
-        } else {
+        group = group_dpif_lookup(ctx->xbridge->ofproto, group_id);
+        if (!group) {
+            /* XXX: Should set ctx->error ? */
             return true;
         }
+        xlate_group_action__(ctx, group);
     }
 
     return false;
@@ -4772,6 +4771,9 @@  do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
         case OFPACT_GROUP:
             if (xlate_group_action(ctx, ofpact_get_GROUP(a)->group_id)) {
                 /* Group could not be found. */
+
+                /* XXX: Terminates action list translation, but does not
+                 * terminate the pipeline. */
                 return;
             }
             break;
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 57f3eff..ab26f38 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4264,7 +4264,7 @@  group_construct_stats(struct group_dpif *group)
     group->packet_count = 0;
     group->byte_count = 0;
 
-    group_dpif_get_buckets(group, &buckets);
+    buckets = group_dpif_get_buckets(group);
     LIST_FOR_EACH (bucket, list_node, buckets) {
         bucket->stats.packet_count = 0;
         bucket->stats.byte_count = 0;
@@ -4285,7 +4285,7 @@  group_dpif_credit_stats(struct group_dpif *group,
     } else { /* Credit to all buckets */
         const struct ovs_list *buckets;
 
-        group_dpif_get_buckets(group, &buckets);
+        buckets = group_dpif_get_buckets(group);
         LIST_FOR_EACH (bucket, list_node, buckets) {
             bucket->stats.packet_count += stats->n_packets;
             bucket->stats.byte_count += stats->n_bytes;
@@ -4335,7 +4335,7 @@  group_get_stats(const struct ofgroup *group_, struct ofputil_group_stats *ogs)
     ogs->packet_count = group->packet_count;
     ogs->byte_count = group->byte_count;
 
-    group_dpif_get_buckets(group, &buckets);
+    buckets = group_dpif_get_buckets(group);
     bucket_stats = ogs->bucket_stats;
     LIST_FOR_EACH (bucket, list_node, buckets) {
         bucket_stats->packet_count = bucket->stats.packet_count;
@@ -4351,24 +4351,17 @@  group_get_stats(const struct ofgroup *group_, struct ofputil_group_stats *ogs)
  *
  * Make sure to call group_dpif_unref() after no longer needing to maintain
  * a reference to the group. */
-bool
-group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id,
-                  struct group_dpif **group)
+struct group_dpif *
+group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id)
 {
-    struct ofgroup *ofgroup;
-    bool found;
-
-    found = ofproto_group_lookup(&ofproto->up, group_id, &ofgroup);
-    *group = found ?  group_dpif_cast(ofgroup) : NULL;
-
-    return found;
+    struct ofgroup *ofgroup = ofproto_group_lookup(&ofproto->up, group_id);
+    return ofgroup ? group_dpif_cast(ofgroup) : NULL;
 }
 
-void
-group_dpif_get_buckets(const struct group_dpif *group,
-                       const struct ovs_list **buckets)
+const struct ovs_list *
+group_dpif_get_buckets(const struct group_dpif *group)
 {
-    *buckets = &group->up.buckets;
+    return &group->up.buckets;
 }
 
 enum ofp11_group_type
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index ff07ba7..18a90b2 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -136,11 +136,10 @@  void rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_timeout,
 void group_dpif_credit_stats(struct group_dpif *,
                              struct ofputil_bucket *,
                              const struct dpif_flow_stats *);
-bool group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id,
-                       struct group_dpif **group);
+struct group_dpif *group_dpif_lookup(struct ofproto_dpif *ofproto,
+                                     uint32_t group_id);
+const struct ovs_list *group_dpif_get_buckets(const struct group_dpif *group);
 
-void group_dpif_get_buckets(const struct group_dpif *group,
-                            const struct ovs_list **buckets);
 enum ofp11_group_type group_dpif_get_type(const struct group_dpif *group);
 const char *group_dpif_get_selection_method(const struct group_dpif *group);
 uint64_t group_dpif_get_selection_method_param(const struct group_dpif *group);
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 5ceabe5..e2bc6f3 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -125,7 +125,7 @@  struct ofproto {
 
     /* Groups. */
     struct ovs_rwlock groups_rwlock;
-    struct hmap groups OVS_GUARDED;   /* Contains "struct ofgroup"s. */
+    struct cmap groups;               /* Contains "struct ofgroup"s. */
     uint32_t n_groups[4] OVS_GUARDED; /* # of existing groups of each type. */
     struct ofputil_group_features ogf;
 };
@@ -490,12 +490,12 @@  void ofproto_rule_reduce_timeouts(struct rule *rule, uint16_t idle_timeout,
                                   uint16_t hard_timeout)
     OVS_EXCLUDED(ofproto_mutex);
 
-/* A group within a "struct ofproto".
+/* A group within a "struct ofproto", RCU-protected.
  *
  * With few exceptions, ofproto implementations may look at these fields but
  * should not modify them. */
 struct ofgroup {
-    struct hmap_node hmap_node OVS_GUARDED; /* In ofproto's "groups" hmap. */
+    struct cmap_node cmap_node; /* In ofproto's "groups" cmap. */
 
     /* Number of references.
      *
@@ -517,16 +517,18 @@  struct ofgroup {
     const long long int created;      /* Creation time. */
     const long long int modified;     /* Time of last modification. */
 
+    /* const ?? */
     struct ovs_list buckets;        /* Contains "struct ofputil_bucket"s. */
     const uint32_t n_buckets;
 
     const struct ofputil_group_props props;
 };
 
-bool ofproto_group_lookup(const struct ofproto *ofproto, uint32_t group_id,
-                          struct ofgroup **group);
+struct ofgroup *ofproto_group_lookup(const struct ofproto *ofproto,
+                                     uint32_t group_id);
 
 void ofproto_group_ref(struct ofgroup *);
+bool ofproto_group_try_ref(struct ofgroup *);
 void ofproto_group_unref(struct ofgroup *);
 
 void ofproto_group_delete_all(struct ofproto *);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index b4876b5..fb4993f 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -293,12 +293,7 @@  static void send_buffered_packet(const struct flow_mod_requester *,
                                  uint32_t buffer_id, struct rule *)
     OVS_REQUIRES(ofproto_mutex);
 
-static bool ofproto_group_exists__(const struct ofproto *ofproto,
-                                   uint32_t group_id)
-    OVS_REQ_RDLOCK(ofproto->groups_rwlock);
-static bool ofproto_group_exists(const struct ofproto *ofproto,
-                                 uint32_t group_id)
-    OVS_EXCLUDED(ofproto->groups_rwlock);
+static bool ofproto_group_exists(const struct ofproto *, uint32_t group_id);
 static enum ofperr add_group(struct ofproto *,
                              const struct ofputil_group_mod *);
 static void handle_openflow(struct ofconn *, const struct ofpbuf *);
@@ -564,7 +559,7 @@  ofproto_create(const char *datapath_name, const char *datapath_type,
     guarded_list_init(&ofproto->rule_executes);
     ofproto->min_mtu = INT_MAX;
     ovs_rwlock_init(&ofproto->groups_rwlock);
-    hmap_init(&ofproto->groups);
+    cmap_init(&ofproto->groups);
     ovs_mutex_unlock(&ofproto_mutex);
     ofproto->ogf.types = 0xf;
     ofproto->ogf.capabilities = OFPGFC_CHAINING | OFPGFC_SELECT_LIVENESS |
@@ -1591,7 +1586,7 @@  ofproto_destroy__(struct ofproto *ofproto)
 
     guarded_list_destroy(&ofproto->rule_executes);
     ovs_rwlock_destroy(&ofproto->groups_rwlock);
-    hmap_destroy(&ofproto->groups);
+    cmap_destroy(&ofproto->groups);
 
     hmap_remove(&all_ofprotos, &ofproto->hmap_node);
     free(ofproto->name);
@@ -2947,13 +2942,28 @@  ofproto_group_ref(struct ofgroup *group)
     }
 }
 
+bool
+ofproto_group_try_ref(struct ofgroup *group)
+{
+    if (group) {
+        return ovs_refcount_try_ref_rcu(&group->ref_count);
+    }
+    return false;
+}
+
+static void
+group_destroy_cb(struct ofgroup *group)
+{
+    group->ofproto->ofproto_class->group_destruct(group);
+    ofputil_bucket_list_destroy(&group->buckets);
+    group->ofproto->ofproto_class->group_dealloc(group);
+}
+
 void
 ofproto_group_unref(struct ofgroup *group)
 {
-    if (group && ovs_refcount_unref(&group->ref_count) == 1) {
-        group->ofproto->ofproto_class->group_destruct(group);
-        ofputil_bucket_list_destroy(&group->buckets);
-        group->ofproto->ofproto_class->group_dealloc(group);
+    if (group && ovs_refcount_unref_relaxed(&group->ref_count) == 1) {
+        ovsrcu_postpone(group_destroy_cb, group);
     }
 }
 
@@ -6153,68 +6163,51 @@  handle_meter_request(struct ofconn *ofconn, const struct ofp_header *request,
     return 0;
 }
 
-static bool
-ofproto_group_lookup__(const struct ofproto *ofproto, uint32_t group_id,
-                       struct ofgroup **group)
-    OVS_REQ_RDLOCK(ofproto->groups_rwlock)
+/* Returned group is RCU protected. */
+static struct ofgroup *
+ofproto_group_lookup__(const struct ofproto *ofproto, uint32_t group_id)
 {
-    HMAP_FOR_EACH_IN_BUCKET (*group, hmap_node,
-                             hash_int(group_id, 0), &ofproto->groups) {
-        if ((*group)->group_id == group_id) {
-            return true;
+    struct ofgroup *group;
+
+    CMAP_FOR_EACH_WITH_HASH (group, cmap_node, hash_int(group_id, 0),
+                             &ofproto->groups) {
+        if (group->group_id == group_id) {
+            return group;
         }
     }
 
-    return false;
+    return NULL;
 }
 
 /* If the group exists, this function increments the groups's reference count.
  *
  * Make sure to call ofproto_group_unref() after no longer needing to maintain
  * a reference to the group. */
-bool
-ofproto_group_lookup(const struct ofproto *ofproto, uint32_t group_id,
-                     struct ofgroup **group)
+struct ofgroup *
+ofproto_group_lookup(const struct ofproto *ofproto, uint32_t group_id)
 {
-    bool found;
-
-    ovs_rwlock_rdlock(&ofproto->groups_rwlock);
-    found = ofproto_group_lookup__(ofproto, group_id, group);
-    if (found) {
-        ofproto_group_ref(*group);
-    }
-    ovs_rwlock_unlock(&ofproto->groups_rwlock);
-    return found;
-}
-
-static bool
-ofproto_group_exists__(const struct ofproto *ofproto, uint32_t group_id)
-    OVS_REQ_RDLOCK(ofproto->groups_rwlock)
-{
-    struct ofgroup *grp;
+    struct ofgroup *group;
 
-    HMAP_FOR_EACH_IN_BUCKET (grp, hmap_node,
-                             hash_int(group_id, 0), &ofproto->groups) {
-        if (grp->group_id == group_id) {
-            return true;
-        }
+    group = ofproto_group_lookup__(ofproto, group_id);
+    if (group) {
+        /* Not holding a lock, so it is possible that another thread releases
+         * the last reference just before we manage to get one. */
+        return ofproto_group_try_ref(group) ? group : NULL;
     }
-    return false;
+    return NULL;
 }
 
+/* Caller should hold 'ofproto->groups_rwlock' if it is important that the
+ * group is not removed by someone else. */
 static bool
 ofproto_group_exists(const struct ofproto *ofproto, uint32_t group_id)
-    OVS_EXCLUDED(ofproto->groups_rwlock)
 {
-    bool exists;
-
-    ovs_rwlock_rdlock(&ofproto->groups_rwlock);
-    exists = ofproto_group_exists__(ofproto, group_id);
-    ovs_rwlock_unlock(&ofproto->groups_rwlock);
-
-    return exists;
+    return ofproto_group_lookup__(ofproto, group_id) != NULL;
 }
 
+/* XXX: This is potentially very expensive for large flow tables, and may be
+ * called in a loop.  Maybe explicitly maintain the count of rules referring to
+ * the group instead?. */
 static uint32_t
 group_get_ref_count(struct ofgroup *group)
     OVS_EXCLUDED(ofproto_mutex)
@@ -6283,15 +6276,16 @@  handle_group_request(struct ofconn *ofconn,
 
     ofpmp_init(&replies, request);
     if (group_id == OFPG_ALL) {
+        /* Must exclude modifications to guarantee iterating all groups. */
         ovs_rwlock_rdlock(&ofproto->groups_rwlock);
-        HMAP_FOR_EACH (group, hmap_node, &ofproto->groups) {
+        CMAP_FOR_EACH (group, cmap_node, &ofproto->groups) {
             cb(group, &replies);
         }
         ovs_rwlock_unlock(&ofproto->groups_rwlock);
     } else {
-        if (ofproto_group_lookup(ofproto, group_id, &group)) {
+        group = ofproto_group_lookup__(ofproto, group_id);
+        if (group) {
             cb(group, &replies);
-            ofproto_group_unref(group);
         }
     }
     ofconn_send_replies(ofconn, &replies);
@@ -6496,8 +6490,8 @@  add_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm)
         return error;
     }
 
-    /* We wrlock as late as possible to minimize the time we jam any other
-     * threads: No visible state changes before acquiring the lock. */
+    /* We take the mutex as late as possible to minimize the time we jam any
+     * other threads: No visible state changes before acquiring the lock. */
     ovs_rwlock_wrlock(&ofproto->groups_rwlock);
 
     if (ofproto->n_groups[gm->type] >= ofproto->ogf.max_groups[gm->type]) {
@@ -6505,27 +6499,22 @@  add_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm)
         goto unlock_out;
     }
 
-    if (ofproto_group_exists__(ofproto, gm->group_id)) {
+    if (ofproto_group_exists(ofproto, gm->group_id)) {
         error = OFPERR_OFPGMFC_GROUP_EXISTS;
         goto unlock_out;
     }
 
-    if (!error) {
-        /* Insert new group. */
-        hmap_insert(&ofproto->groups, &ofgroup->hmap_node,
-                    hash_int(ofgroup->group_id, 0));
-        ofproto->n_groups[ofgroup->type]++;
+    /* Insert new group. */
+    cmap_insert(&ofproto->groups, &ofgroup->cmap_node,
+                hash_int(ofgroup->group_id, 0));
+    ofproto->n_groups[ofgroup->type]++;
 
-        ovs_rwlock_unlock(&ofproto->groups_rwlock);
-        return error;
-    }
+    ovs_rwlock_unlock(&ofproto->groups_rwlock);
+    return 0;
 
  unlock_out:
     ovs_rwlock_unlock(&ofproto->groups_rwlock);
-    ofproto->ofproto_class->group_destruct(ofgroup);
-    ofputil_bucket_list_destroy(&ofgroup->buckets);
-    ofproto->ofproto_class->group_dealloc(ofgroup);
-
+    group_destroy_cb(ofgroup);
     return error;
 }
 
@@ -6646,7 +6635,8 @@  modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm)
     retiring = new_ofgroup;
 
     ovs_rwlock_wrlock(&ofproto->groups_rwlock);
-    if (!ofproto_group_lookup__(ofproto, gm->group_id, &ofgroup)) {
+    ofgroup = ofproto_group_lookup__(ofproto, gm->group_id);
+    if (!ofgroup) {
         error = OFPERR_OFPGMFC_UNKNOWN_GROUP;
         goto out;
     }
@@ -6674,6 +6664,8 @@  modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm)
     *CONST_CAST(long long int *, &(new_ofgroup->created)) = ofgroup->created;
     *CONST_CAST(long long int *, &(new_ofgroup->modified)) = time_msec();
 
+    /* XXX: OK to lose old group's stats? */
+
     error = ofproto->ofproto_class->group_modify(new_ofgroup);
     if (error) {
         goto out;
@@ -6681,9 +6673,9 @@  modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm)
 
     retiring = ofgroup;
     /* Replace ofgroup in ofproto's groups hash map with new_ofgroup. */
-    hmap_remove(&ofproto->groups, &ofgroup->hmap_node);
-    hmap_insert(&ofproto->groups, &new_ofgroup->hmap_node,
-                hash_int(new_ofgroup->group_id, 0));
+    cmap_replace(&ofproto->groups, &ofgroup->cmap_node,
+                 &new_ofgroup->cmap_node, hash_int(ofgroup->group_id, 0));
+
     if (ofgroup->type != new_ofgroup->type) {
         ofproto->n_groups[ofgroup->type]--;
         ofproto->n_groups[new_ofgroup->type]++;
@@ -6700,13 +6692,12 @@  out:
 static enum ofperr
 add_or_modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm)
 {
-    struct ofgroup *ofgroup;
     enum ofperr error;
     bool exists;
 
-    ovs_rwlock_rdlock(&ofproto->groups_rwlock);
-    exists = ofproto_group_lookup__(ofproto, gm->group_id, &ofgroup);
-    ovs_rwlock_unlock(&ofproto->groups_rwlock);
+    exists = ofproto_group_exists(ofproto, gm->group_id);
+
+    /* XXX: Race: Should hold groups_rwlock here. */
 
     if (!exists) {
         error = add_group(ofproto, gm);
@@ -6731,8 +6722,10 @@  delete_group__(struct ofproto *ofproto, struct ofgroup *ofgroup)
     ofm.fm.table_id = OFPTT_ALL;
     handle_flow_mod__(ofproto, &ofm, NULL);
 
-    hmap_remove(&ofproto->groups, &ofgroup->hmap_node);
-    /* No-one can find this group any more. */
+    cmap_remove(&ofproto->groups, &ofgroup->cmap_node,
+                hash_int(ofgroup->group_id, 0));
+    /* No-one can find this group any more, but current users may continue to
+     * use it. */
     ofproto->n_groups[ofgroup->type]--;
     ovs_rwlock_unlock(&ofproto->groups_rwlock);
     ofproto_group_unref(ofgroup);
@@ -6747,18 +6740,18 @@  delete_group(struct ofproto *ofproto, uint32_t group_id)
     ovs_rwlock_wrlock(&ofproto->groups_rwlock);
     if (group_id == OFPG_ALL) {
         for (;;) {
-            struct hmap_node *node = hmap_first(&ofproto->groups);
+            struct cmap_node *node = cmap_first(&ofproto->groups);
             if (!node) {
                 break;
             }
-            ofgroup = CONTAINER_OF(node, struct ofgroup, hmap_node);
-            delete_group__(ofproto, ofgroup);
+            ofgroup = CONTAINER_OF(node, struct ofgroup, cmap_node);
+            delete_group__(ofproto, ofgroup); /* Releases the mutex. */
             /* Lock for each node separately, so that we will not jam the
              * other threads for too long time. */
             ovs_rwlock_wrlock(&ofproto->groups_rwlock);
         }
     } else {
-        HMAP_FOR_EACH_IN_BUCKET (ofgroup, hmap_node,
+        CMAP_FOR_EACH_WITH_HASH (ofgroup, cmap_node,
                                  hash_int(group_id, 0), &ofproto->groups) {
             if (ofgroup->group_id == group_id) {
                 delete_group__(ofproto, ofgroup);
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 4abd335..e5fb54c 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -347,10 +347,10 @@  group_id=1235,type=all,bucket=actions=output:10
 ])
 AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn add-groups br0 groups.txt])
 AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn dump-groups br0 ], [0], [stdout])
-AT_CHECK([strip_xids < stdout], [0], [dnl
-OFPST_GROUP_DESC reply (OF1.1):
+AT_CHECK([strip_xids < stdout | sort], [0], [dnl
  group_id=1234,type=all,bucket=actions=output:10
  group_id=1235,type=all,bucket=actions=output:10
+OFPST_GROUP_DESC reply (OF1.1):
 ])
 AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn del-groups br0 group_id=1234])
 AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn dump-groups br0], [0], [stdout])
@@ -437,17 +437,17 @@  OFPST_GROUP_DESC reply (OF1.5):
 ])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn del-groups br0 group_id=1234])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout])
-AT_CHECK([strip_xids < stdout], [0], [dnl
-OFPST_GROUP_DESC reply (OF1.5):
- group_id=1235,type=all,bucket=bucket_id:2,actions=output:12,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11
+AT_CHECK([strip_xids < stdout | sort], [0], [dnl
  group_id=1233,type=select,selection_method=hash,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11
+ group_id=1235,type=all,bucket=bucket_id:2,actions=output:12,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11
+OFPST_GROUP_DESC reply (OF1.5):
 ])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn del-groups br0 group_id=1234])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout])
-AT_CHECK([strip_xids < stdout], [0], [dnl
-OFPST_GROUP_DESC reply (OF1.5):
- group_id=1235,type=all,bucket=bucket_id:2,actions=output:12,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11
+AT_CHECK([strip_xids < stdout | sort], [0], [dnl
  group_id=1233,type=select,selection_method=hash,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11
+ group_id=1235,type=all,bucket=bucket_id:2,actions=output:12,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11
+OFPST_GROUP_DESC reply (OF1.5):
 ])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn del-groups br0], [0])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout])