On Thu, Sep 05, 2013 at 03:24:20PM -0700, Jarno Rajahalme wrote:
>
> On Sep 4, 2013, at 10:19 PM, Simon Horman <ho...@verge.net.au> wrote:
>
> > Implementation note:
> >
> > All actions which modify a field are added to the action set
> > at the point where "set" actions should be added. In general
> > modifying a field many times is the same as only modifying it
> > the last time so the implementation simply adds all set actions to
> > the action set in the order they are specified. However, this breaks
> > down if two actions modify different portions of the same field.
> >
> > Some examples.
> >
> > 1. load acting a subfield
> > 2. mod_vlan_vid, mod_vlan_pcp
> >
> > If this is considered to be a problem one possible solution would be to
> > either disallow all set actions other than set_field in write_actions.
> > Another possible solution is prohibit problematic the actions listed above
> > in write actions.
>
> IMO two actions that modify different portions of the same field should not be
> a problem. If the subfields overlap, however, the result might be problematic.
I agree.
The overlapping sub-field case seems problematic indeed.
I think the implementation makes a reasonable attempt to
reconcile the set behaviour of the action set and the potential
for subfield overlap.
I am, however, very open to other ideas.
> BUNDLE conflicts with group semantics? Could still make sense for an
> INDIRECT group?
I'm not quite sure what you are asking but perhaps this answers your
question.
As it stands I believe that bundles are intended to be generic but
the only supported slave type at this time is an ofport. As such
I think it would be reasonable to treat BUNDLE as if it were
an OUTPUT action for the purpose of processing the action set.
I am also comfortable with disallowing BUNDLE within the action set.
> Strictly following the spec would likely require set_field only, so the above
> is
> an extension of the spec anyway.
I am not opposed to strictly following the spec.
And it would quite nicely eliminate the subfield problem.
But on the other hand it is nice to provide a richer feature set
if it can be done sanely. Especially as many of the set actions
are essentially aliases for each other.
> ...
> > +}
> > +
> > +/* True if an action is allowed in the action set.
> > + * False otherwise. */
> > +static bool
> > +ofpact_is_allowed_in_actions_set(const struct ofpact *a)
> > +{
> > + switch (a->type) {
> > + case OFPACT_GROUP:
> > + case OFPACT_DEC_TTL:
> > + case OFPACT_OUTPUT:
> > + case OFPACT_POP_MPLS:
> > + case OFPACT_PUSH_MPLS:
> > + case OFPACT_PUSH_VLAN:
> > + case OFPACT_REG_LOAD:
> > + case OFPACT_SET_ETH_DST:
> > + case OFPACT_SET_ETH_SRC:
> > + case OFPACT_SET_IPV4_DSCP:
> > + case OFPACT_SET_IPV4_DST:
> > + case OFPACT_SET_IPV4_SRC:
> > + case OFPACT_SET_L4_DST_PORT:
> > + case OFPACT_SET_L4_SRC_PORT:
> > + case OFPACT_SET_MPLS_TTL:
> > + case OFPACT_SET_QUEUE:
> > + case OFPACT_SET_TUNNEL:
> > + case OFPACT_SET_VLAN_PCP:
> > + case OFPACT_SET_VLAN_VID:
> > + case OFPACT_STRIP_VLAN:
> > + return true;
> > + case OFPACT_BUNDLE:
>
> It would seem to me that most of the actions in this "false" category might
> also make sense in a bucket. See comments below:
In general I made them false because they are outside the spec
and in particular, looking at the list from 1 to 11 in section "5.10 Action Set"
in the OpenFlow 1.3.2 specification, its unclear to me where many of the
actions I marked as 'false' should go.
> BUNDLE conflicts with group semantics? Could still make sense for an
> INDIRECT group?
>
> > + case OFPACT_CLEAR_ACTIONS:
>
> It is quite clear why instructions are not allowed, but it would be nice to
> group them together and accompany them with a comment.
Sure.
> As for the
> extension actions, maybe some of these would work in a bucket as well
> (OFPACT_NOTE, to give a trivial example, but maybe also
>
> > + case OFPACT_CONTROLLER:
>
> Why not?
It is not clear to how to order this action as per section "5.10 Action
Set". Would it work if it was considered to be the same as an
OFPACT_OUTPUT action for the purpose of constructing the action set from a
list of actions?
>
> > + case OFPACT_DEC_MPLS_TTL:
>
> Why not?
This I meant to make 'true'.
It can be added to the action set with OFPACT_DEC_TTL.
>
> > + case OFPACT_ENQUEUE:
>
> Why not?
I am unsure where it should go in the order described by "5.10 Action
Set". Perhaps, like OFPACT_CONTROLLER, it would make sense to consider it
as another form of OFPACT_OUTPUT. But this seems to conflict with the way
that qos is described in 5.10.
> > + case OFPACT_EXIT:
>
> Maybe you want ovs to exit is a specific bucket is selected ;-)
I am unsure where it should go in the order described by "5.10 Action
Set".
> > + case OFPACT_FIN_TIMEOUT:
>
> Goes with LEARN, I guess.
Possibly, but I'm unsure where learn should go in the order described by
"5.10 Action Set".
> > + case OFPACT_GOTO_TABLE:
Goto Table is an instruction.
> > + case OFPACT_LEARN:
Again, I'm unsure where it should go in the order described by "5.10 Action
Set".
>
> Why not?
>
> > + case OFPACT_METER:
Meter is an instruction.
> > + case OFPACT_MULTIPATH:
>
> Why not?
>
> > + case OFPACT_NOTE:
Again, I'm unsure where it should go in the order described by "5.10 Action
Set". But perhaps in this case the order is not particularly important.
I think, for example processing this first would be harmless.
> > + case OFPACT_OUTPUT_REG:
>
> Why not? A later table may set the output port to a specific register,
> that is then used by this at the end?
Again, I'm unsure where it should go in the order described by "5.10 Action
Set". But in this case perhaps it could be considered as equivalent
to an OFPACT_OUTPUT actino.
> > + case OFPACT_POP_QUEUE:
>
> I guess this would make no difference at the end.
Again, I'm unsure where it should go in the order described by "5.10 Action
Set". I guess the most logical place would be right after OFPACT_SET_QUEUE.
> > + case OFPACT_REG_MOVE:
Again, I'm unsure where it should go in the order described by "5.10 Action
Set". Perhaps a logical position would be either immediately before or
immediately after processing set actions.
> Why not?
>
> > + case OFPACT_RESUBMIT:
>
> This would conflict with group semantics.
>
> > + case OFPACT_SAMPLE:
>
> Why not?
Again, I'm unsure where it should go in the order described by "5.10 Action
Set".
>
> > + case OFPACT_STACK_POP:
>
> > + case OFPACT_STACK_PUSH:
>
> These could be useful in a bucket as well.
Again, I'm unsure where it should go in the order described by "5.10 Action
Set".
I guess that a possibility is to process OFPACT_STACK_POP with
the other pop actions. And to process OFPACT_STACK_PUSH with
the other push actions.
> > + case OFPACT_WRITE_METADATA:
> > + case OFPACT_WRITE_ACTIONS:
> > + return false;
> > + default:
> > + NOT_REACHED();
> > + }
> > +}
> > +
> >
> ...
> >
> > case OVSINST_OFPIT11_CLEAR_ACTIONS:
> > ofpact_put_CLEAR_ACTIONS(ofpacts);
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index e7cec14..359aa10 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -164,6 +164,10 @@ struct xlate_ctx {
> > odp_port_t sflow_odp_port; /* Output port for composing sFlow action.
> > */
> > uint16_t user_cookie_offset;/* Used for user_action_cookie fixup. */
> > bool exit; /* No further actions should be processed.
> > */
> > +
> > + struct list action_set; /* Action set.
> > + * An ordered list of struct ofpbuf
> > + * containing ofpacts to add to the
> > action. */
>
> There is quite a lot memory allocations and copying involved when gathering
> and using this list. Have you considered keeping (additional) read locks on
> the rules that have the buckets and using the buckets by reference while
> translating groups?
I had not considered that particular idea. In general my aim is to come
up with something sane to provide groups functionality. I believe it would
be best to defer optimisation. Especially in the area of multi-threading
where the code is changing at a rapid place and bottlenecks seem to
be as-yet unknown.
> Also, did you consider a strict set of all supported actions instead a list?
> Not
> saying it would be better, just asking.
Yes, I did consider creating a set, possibly based an hmap with the
action type as the key. I didn't code this idea up so it may well be
nicer. But I decided to take the approach in this patch at felt like
a simpler, though possibly less elegant, approach.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
