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

[Simon Horman]

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 <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.

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.