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.
Strictly following the spec would likely require set_field only, so the above is
an extension of the spec anyway.
...
> +}
> +
> +/* 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:
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. 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?
> + case OFPACT_DEC_MPLS_TTL:
Why not?
> + case OFPACT_ENQUEUE:
Why not?
> + case OFPACT_EXIT:
Maybe you want ovs to exit is a specific bucket is selected ;-)
> + case OFPACT_FIN_TIMEOUT:
Goes with LEARN, I guess.
> + case OFPACT_GOTO_TABLE:
> + case OFPACT_LEARN:
Why not?
> + case OFPACT_METER:
> + case OFPACT_MULTIPATH:
Why not?
> + case OFPACT_NOTE:
See above :-)
> + 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?
> + case OFPACT_POP_QUEUE:
I guess this would make no difference at the end.
> + case OFPACT_REG_MOVE:
Why not?
> + case OFPACT_RESUBMIT:
This would conflict with group semantics.
> + case OFPACT_SAMPLE:
Why not?
> + case OFPACT_STACK_POP:
> + case OFPACT_STACK_PUSH:
These could be useful in a bucket as well.
> + 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?
Also, did you consider a strict set of all supported actions instead a list? Not
saying it would be better, just asking.
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
