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

[Ben Pfaff]

On Wed, Jun 03, 2015 at 04:02:43PM +0900, Takashi Yamamoto wrote:
> hi,
> 
> On Wed, Jun 3, 2015 at 3:36 PM, Ben Pfaff <blp at nicira.com> wrote:
> > 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?
> 
> i think it's a good idea.
> in_group needs to be a counter if we allow recursion.

Thanks.

I posted a patch:
        http://openvswitch.org/pipermail/dev/2015-June/056048.html