[ovs-dev] Can we trivially enable group chaining?

[Ben Pfaff]

We have the following comment in ofproto-dpif-xlate.c:

        /* Prevent nested translation of OpenFlow groups.
         *
         * OpenFlow allows this restriction.  We enforce this restriction only
         * because, with the current architecture, we would otherwise have to
         * take a possibly recursive read lock on the ofgroup rwlock, which is
         * unsafe given that POSIX allows taking a read lock to block if there
         * is a thread blocked on taking the write lock.  Other solutions
         * without this restriction are also possible, but seem unwarranted
         * given the current limited use of groups. */

But I think that the reasoning no longer holds.  At the time the comment
was written, each ofgroup had its own internal rwlock, which was held
for reading whenever the ofgroup was being translated.  Since then,
ofgroups have transitioned to using RCU for protection, and no rwlock is
held during translation.

I think that, therefore, we can trivially enable group chaining with a
commit like this (untested):

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 71b8bef..ce90b2a 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3302,33 +3302,9 @@ xlate_group_action__(struct xlate_ctx *ctx, struct group_dpif *group)
 }
 
 static bool
-xlate_group_resource_check(struct xlate_ctx *ctx)
-{
-    if (!xlate_resubmit_resource_check(ctx)) {
-        return false;
-    } else if (ctx->in_group) {
-        /* Prevent nested translation of OpenFlow groups.
-         *
-         * OpenFlow allows this restriction.  We enforce this restriction only
-         * because, with the current architecture, we would otherwise have to
-         * take a possibly recursive read lock on the ofgroup rwlock, which is
-         * unsafe given that POSIX allows taking a read lock to block if there
-         * is a thread blocked on taking the write lock.  Other solutions
-         * without this restriction are also possible, but seem unwarranted
-         * given the current limited use of groups. */
-        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
-
-        VLOG_ERR_RL(&rl, "cannot recursively translate OpenFlow group");
-        return false;
-    } else {
-        return true;
-    }
-}
-
-static bool
 xlate_group_action(struct xlate_ctx *ctx, uint32_t group_id)
 {
-    if (xlate_group_resource_check(ctx)) {
+    if (xlate_resubmit_resource_check(ctx)) {
         struct group_dpif *group;
         bool got_group;
 
Anyone know a reason this might be a bad idea?