On Tue, Mar 19, 2013 at 12:20:51PM -0700, Jesse Gross wrote:
> On Tue, Mar 19, 2013 at 7:57 AM, Simon Horman <ho...@verge.net.au> wrote:
> > On Tue, Mar 19, 2013 at 10:35:40AM +0900, Simon Horman wrote:
> >> On Mon, Mar 18, 2013 at 08:54:49AM -0700, Jesse Gross wrote:
> >> > On Mon, Mar 18, 2013 at 12:37 AM, Simon Horman <ho...@verge.net.au>
> >> > wrote:
> >> > > * Update the order in which actions are committed and thus
> >> > > appear in the datapath such that MPLS actions appear after
> >> > > l3 and l4 (nw and port) actions.
> >> > >
> >> > > In the case where an mpls_push action is present it should ensure
> >> > > that l3 and l4 actions occur first, which seems logical as
> >> > > once a mpls_push has occur the frame will be MPLS rather
> >> > > than IPv4 or IPv6.
> >> > >
> >> > > In the case where there is an mpls_pop action is present this should
> >> > > not make any difference as the frame will have been MPLS to start
> >> > > with
> >> > > and thus not satisfy the pre-requisites for l3 or l4 actions.
> >> > >
> >> > > * Update commit_set_nw_action() to use the base ethertype when
> >> > > considering
> >> > > eligibility to commit l3 (nw) actions. This allows l3 actions to be
> >> > > applied so long as the frame was originally IPv4 or IPv6, even if
> >> > > an mpls_push action will be applied and thus flow indicates the
> >> > > frame will be MPLS.
> >> > >
> >> > > Signed-off-by: Simon Horman <ho...@verge.net.au>
> >> >
> >> > I think this is the right path although I'm not sure that it fully
> >> > handles L4 ports correctly if you push an MPLS label, output, and then
> >> > set a port number. We should ignore the last set but I think we will
> >> > attempt to do it anyways since we originally had an unlabeled packet.
> >> > Maybe we should sanitize the flow a little more when we push a label.
> >>
> >> I think this can be handled by adding some constraints in
> >> do_xlate_actions(). Something like this:
> >
> > Scratch that. I think your suggestion is sound.
>
> Hmm, thinking about this some more, I think your first version of this
> is actually more natural. For one thing, it avoids exposing
> implementation details of the action composition. For example, given
> these two action lists:
>
> * push_mpls, set_port, output
> * set_port, push_mpls, output
>
> The first version will produce different results (correctly) while the
> second one will be the same.
>
> The only thing is that I think we just want to make the action
> conditional on the EtherType, rather than jumping to 'out' since the
> latter will terminate future action processing.
Sure, how about this?
From: Simon Horman <ho...@verge.net.au>
mpls: Allow l3 and l4 actions to prior to a push_mpls action
* Update the order in which actions are committed and thus
appear in the datapath such that MPLS actions appear after
l3 and l4 (nw and port) actions.
In the case where an mpls_push action is present it should ensure
that l3 and l4 actions occur first, which seems logical as
once a mpls_push has occur the frame will be MPLS rather
than IPv4 or IPv6.
In the case where there is an mpls_pop action is present this should
not make any difference as the frame will have been MPLS to start with
and thus not satisfy the pre-requisites for l3 or l4 actions.
* Update commit_set_nw_action() to use the base ethertype when considering
eligibility to commit l3 (nw) actions. This allows l3 actions to be
applied so long as the frame was originally IPv4 or IPv6, even if
an mpls_push action will be applied and thus flow indicates the
frame will be MPLS.
* Invalidate the tp_src and tp_dst of a flow when committing
an mpls_push action to ensure that no subsequent set port actions
are added. An alternative approach would be to add the overhead
of a check of the dl_type of the base flow in commit_set_port_action().
Note that the checking of the dl_type of the base flow in
commit_set_nw_action() which prevents it from adding actions after an
mpls_push action.
Signed-off-by: Simon Horman <ho...@verge.net.au>
---
v2.22.draft
* Invalidate the tp_src and tp_dst of a flow when committing
an mpls_push. See details above.
v2.21
* First post
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 54e5b12..a0d43e4 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -2246,6 +2246,10 @@ commit_mpls_action(const struct flow *flow, struct flow
*base,
memset(mpls, 0, sizeof *mpls);
mpls->mpls_ethertype = flow->dl_type;
mpls->mpls_lse = flow->mpls_lse;
+
+ /* Ensure that a subsequent commit_set_port_action() action
+ * will not alter the packet as it is now MPLS and thus not IP. */
+ base->tp_src = base->tp_dst = 0;
} else {
struct ovs_key_mpls mpls_key;
@@ -2323,9 +2327,9 @@ commit_set_nw_action(const struct flow *flow, struct flow
*base,
return;
}
- if (flow->dl_type == htons(ETH_TYPE_IP)) {
+ if (base->dl_type == htons(ETH_TYPE_IP)) {
commit_set_ipv4_action(flow, base, odp_actions);
- } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) {
+ } else if (base->dl_type == htons(ETH_TYPE_IPV6)) {
commit_set_ipv6_action(flow, base, odp_actions);
}
}
@@ -2398,9 +2402,13 @@ commit_odp_actions(const struct flow *flow, struct flow
*base,
{
commit_set_ether_addr_action(flow, base, odp_actions);
commit_vlan_action(flow, base, odp_actions);
- commit_mpls_action(flow, base, odp_actions);
commit_set_nw_action(flow, base, odp_actions);
commit_set_port_action(flow, base, odp_actions);
+ /* Commiting MPLS actions should occur after committing nw and port
+ * actions. This is because committing MPLS actions may alter a packet so
+ * that it is no longer IP and thus nw and port actions are no longer
valid.
+ */
+ commit_mpls_action(flow, base, odp_actions);
commit_set_priority_action(flow, base, odp_actions);
commit_set_skb_mark_action(flow, base, odp_actions);
}
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
