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