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

[Ben Pfaff]

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.

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?

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.

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.