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