[ovs-dev] [PATCH v7 2/3] Translation of indirect and all groups

Simon Horman horms at verge.net.au
Mon Oct 28 02:47:32 UTC 2013


On Mon, Oct 21, 2013 at 02:46:15PM -0700, Ben Pfaff wrote:
> On Tue, Oct 15, 2013 at 05:17:49PM +0900, Simon Horman wrote:
> > Allow translation of indirect and all groups.  Also allow insertion of
> > indirect and all groups by changing the maximum permitted number in the
> > groups table from 0 to OFPG_MAX.
> > 
> > Implementation note:
> > 
> > After translating the actions for each bucket ctx->flow is reset to its
> > state prior to translation of the buckets actions. This is equivalent to
> > cloning the bucket before applying actions. This is my interpretation of the
> > OpenFlow 1.3.2 specification section 5.6.1 Group Types, which includes the
> > following text. I believe there is room for other interpretations.
> > 
> > * On all groups: "The packet is effectively cloned for each bucket; one
> >   packet is processed for each bucket of the group."
> > * On indirect groups: "This group type is effectively identical to an
> >   all group with one bucket."
> > 
> > Signed-off-by: Simon Horman <horms at verge.net.au>
> 
> This patch treats OFPAT_GROUP in Apply-Actions differently from
> OFPAT_GROUP in Write-Actions.  The words of the standard don't make
> clear whether that is intended.  I filed EXT-408, at
> https://rs.opennetworking.org/bugs/browse/EXT-408, to get
> clarification.  That tracker is private to ONF members, so here's the
> text of what I filed:
> 
>     Section 5.6 "Group Table" in 1.4 (emphasis mine) implies that an
>     action bucket contains an action set:
> 
>     > action buckets: an ordered list of action buckets, where each action
>     > bucket contains a *set of actions* to execute and associated
>     > parameters.
> 
>     Section 5.10 "Action Set" in 1.4 (emphasis mine), through its
>     explanation of how to process a bucket, implies that there might be
>     some ambiguity about whether it's an action set or an action list:
> 
>     > If an action set contains a group action, the actions in the
>     > appropriate action bucket of the group are applied *in the order
>     > specified below*.
> 
>     and:
> 
>     > 10. group.  If a group action is specified, apply the actions of the
>     > relevant group bucket(s) in the order specified by this list.
> 
>     Section 5.11 "Action List" in 1.4 doesn't say one way or another
>     whether the action bucket is processed as a set or a list:
> 
>     > If the list contains group actions, a copy of the packet in its
>     > current state is processed by the relevant group buckets.
> 
>     I've recently received a submission to Open vSwitch that treats action
>     buckets as actions sets when a group action appears in the action set
>     (as required by the plain words of 5.10 "Action Set") and as action
>     lists when a group action appears in an Apply-Actions instruction.  I
>     am unsure about the latter behavior, because the standard appears to
>     say nothing convincing one way or another.  I would prefer consistent
>     behavior, i.e. to treat the bucket as an action set in both cases.
> 
>     The OVS patch is at:
>     http://openvswitch.org/pipermail/dev/2013-October/032833.html
> 
> In the meantime, I'd prefer to treat OFPAT_GROUP as an action set in
> both cases, for consistency.  Would you mind modifying the patch to do
> that?  It should simplify it, a little.

Sure. Actually that was what earlier postings of this patch did.

> xlate_group_action() does only one of the resource checks that
> xlate_table_action() does.  Would you please factor the resource
> checks out of xlate_table_action(), into a new function, and then use
> it in both places?

Sure, I have now done that and it seems to be quite clean.

> This patch, and the previous, make me think that we need to have
> reference counts on groups for the same reason we have them on rules.
> Making rules lockable didn't work out so well, because we want to
> separate modifying a rule from freeing it, in particular to make sure
> that a rule doesn't get freed while we're looking at it.  Probably the
> same holds for groups.

Yes, I expect so.
I'll take a look at adding reference counts.

> Please consider folding in these trivial fixes:
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 5fda8ff..d4038be 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1780,7 +1780,7 @@ static void
>  xlate_group_bucket_set(struct xlate_ctx *ctx,
>                         const struct ofputil_bucket *bucket)
>  {
> -    uint64_t action_list_stub[1024 / 64];
> +    uint64_t action_list_stub[1024 / 8];
>      struct ofpbuf action_list, action_set;
>  
>      ofpbuf_use_const(&action_set, bucket->ofpacts, bucket->ofpacts_len);
> @@ -1814,7 +1814,7 @@ xlate_all_group(struct xlate_ctx *ctx, struct group_dpif *group)
>  
>      group_dpif_get_buckets(group, &buckets);
>  
> -    LIST_FOR_EACH(bucket, list_node, buckets) {
> +    LIST_FOR_EACH (bucket, list_node, buckets) {
>          xlate_group_bucket(ctx, bucket);
>          /* Roll back flow to previous state.
>           * This is equivalent to cloning the packet for each bucket.
> 

Thanks, I have added those.



More information about the dev mailing list