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 <b...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
