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