On Tue, Oct 29, 2013 at 04:52:50PM +0900, Simon Horman wrote: > On Mon, Oct 28, 2013 at 11:47:32AM +0900, Simon Horman wrote: > > 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. > > I took a look into adding reference counts and I'm not entirely convinced > that it is necessary. As it stands the locking seem clean enough to me at > this time. Is there something in particular that you are worried about? > > To be clear, I'm quite happy to make this change. > But I'm not sure that it isn't just extra complexity at this time.
Feel free to leave it as-is, then, and I will consider the question more carefully for v8. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
