* [PATCH v2.39 0/7] MPLS actions and matches
@ 2013-09-09 7:20 Simon Horman
[not found] ` <1378711207-29890-1-git-send-email-horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
` (7 more replies)
0 siblings, 8 replies; 38+ messages in thread
From: Simon Horman @ 2013-09-09 7:20 UTC (permalink / raw)
To: dev, netdev
Cc: Ravi K, Isaku Yamahata, Jesse Gross, Pravin B Shelar, Joe Stringer
Hi,
This series implements MPLS actions and matches based on work by
Ravi K, Leo Alterman, Yamahata-san and Joe Stringer.
This series provides two changes
* Provide user-space support for the VLAN/MPLS tag insertion order
up to and including OpenFlow 1.2, and the different ordering
specified from OpenFlow 1.3. In a nutshell the datapath always
uses the OpenFlow 1.3 ordering, which is to always insert tags
immediately after the L2 header, regardless of the presence of other
tags. And ovs-vswtichd provides compatibility for the behaviour up
to OpenFlow 1.2, which is that MPLS tags should follow VLAN tags
if present.
* Adding basic MPLS action and match support to the kernel datapath
Differences between v2.39 and v2.38:
* Rebase for removal of vlan, checksum and skb->mark compat code
- This includes adding adding a new patch,
"[PATCH v2.39 6/7] datapath: Break out deacceleration portion of
vlan_push" to allow re-use of some existing code.
Differences between v2.38 and v2.37:
* Rebase for SCTP support
* Refactor validate_tp_port() to iterate over eth_types rather
than open-coding the loop. With the addition of SCTP this logic
is now used three times.
Differences between v2.37 and v2.36:
* Rebase
Differences between v2.36 and v2.35:
* Rebase
* Do not add set_ethertype() to datapath/actions.c.
As this patch has evolved this function had devolved into
to sets of functionality wrapped into a single function with
only one line of common code. Refactor things to simply
open-code setting the ether type in the two locations where
set_ethertype() was previously used. The aim here is to improve
readability.
* Update setting skb->ethertype after mpls push and pop.
- In the case of push_mpls it should be set unconditionally
as in v2.35 the behaviour of this function to always push
an MPLS LSE before any VLAN tags.
- In the case of mpls_pop eth_p_mpls(skb->protocol) is a better
test than skb->protocol != htons(ETH_P_8021Q) as it will give the
correct behaviour in the presence of other VLAN ethernet types,
for example 0x88a8 which is used by 802.1ad. Moreover, it seems
correct to update the ethernet type if it was previously set
according to the top-most MPLS LSE.
* Deaccelerate VLANs when pushing MPLS tags the
- Since v2.35 MPLS push will insert an MPLS LSE before any VLAN tags.
This means that if an accelerated tag is present it should be
deaccelerated to ensure it ends up in the correct position.
* Update skb->mac_len in push_mpls() so that it will be correct
when used by a subsequent call to pop_mpls().
As things stand I do not believe this is strictly necessary as
ovs-vswitchd will not send a pop MPLS action after a push MPLS action.
However, I have added this in order to code more defensively as I believe
that if such a sequence did occur it would be rather unobvious why
it didn't work.
* Do not add skb_cow_head() call in push_mpls().
It is unnecessary as there is a make_writable() call.
This change was also made in v2.30 but some how the
code regressed between then and v2.35.
Differences between v2.35 and v2.34:
* Add support for the tag ordering specified up until OpenFlow 1.2 and
the ordering specified from OpenFlow 1.3.
* Correct error in datapath patch's handling of GSO in the presence
of MPLS and absence of VLANs.
Patch overview:
* The first 5 patches of this series are new,
adding support for different tag ordering.
* The last patch is a revised version of the patch to add MPLS support
to the datapath. It has been updated to use OpenFlow 1.3 tag ordering
and resolve a GSO handling bug: both mentioned above. Its change log
includes a history of changes.
To aid review this series is available in git at:
git://github.com/horms/openvswitch.git devel/mpls-v2.38
Patch list and overall diffstat:
Joe Stringer (5):
odp: Only pass vlan_tci to commit_vlan_action()
odp: Allow VLAN actions after MPLS actions
ofp-actions: Add OFPUTIL_OFPAT13_PUSH_MPLS
ofp-actions: Add separate OpenFlow 1.3 action parser
lib: Push MPLS tags in the OpenFlow 1.3 ordering
Simon Horman (1):
datapath: Add basic MPLS support to kernel
datapath/Modules.mk | 1 +
datapath/actions.c | 121 +++++-
datapath/datapath.c | 259 +++++++++++--
datapath/datapath.h | 9 +
datapath/flow.c | 58 ++-
datapath/flow.h | 17 +-
datapath/linux/compat/gso.c | 50 ++-
datapath/linux/compat/gso.h | 39 ++
datapath/linux/compat/include/linux/netdevice.h | 12 -
datapath/linux/compat/netdevice.c | 28 --
datapath/mpls.h | 15 +
datapath/vport-lisp.c | 1 +
datapath/vport-netdev.c | 44 ++-
include/linux/openvswitch.h | 7 +-
lib/flow.c | 2 +-
lib/odp-util.c | 22 +-
lib/odp-util.h | 4 +-
lib/ofp-actions.c | 68 +++-
lib/ofp-parse.c | 1 +
lib/ofp-util.c | 3 +
lib/ofp-util.h | 1 +
lib/packets.c | 10 +-
lib/packets.h | 2 +-
ofproto/ofproto-dpif-xlate.c | 103 ++++--
ofproto/ofproto-dpif-xlate.h | 5 +
tests/ofproto-dpif.at | 446 +++++++++++++++++++++++
26 files changed, 1198 insertions(+), 130 deletions(-)
create mode 100644 datapath/mpls.h
--
1.7.10.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2.39 1/7] odp: Only pass vlan_tci to commit_vlan_action()
[not found] ` <1378711207-29890-1-git-send-email-horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
@ 2013-09-09 7:20 ` Simon Horman
2013-09-09 7:20 ` [PATCH v2.39 3/7] ofp-actions: Add OFPUTIL_OFPAT13_PUSH_MPLS Simon Horman
1 sibling, 0 replies; 38+ messages in thread
From: Simon Horman @ 2013-09-09 7:20 UTC (permalink / raw)
To: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA
Cc: Isaku Yamahata, Ravi K
From: Joe Stringer <joe-Q1GJJQv1iO6lP80pJB477g@public.gmane.org>
This allows for future patches to pass different tci values to
commit_vlan_action() without passing an entire flow structure.
Signed-off-by: Joe Stringer <joe-Q1GJJQv1iO6lP80pJB477g@public.gmane.org>
Signed-off-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
---
v2.36 - v2.39
* No change
v2.35
* First post
---
lib/odp-util.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index f20bd8a..11aa32f 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -3326,10 +3326,10 @@ commit_set_ether_addr_action(const struct flow *flow, struct flow *base,
}
static void
-commit_vlan_action(const struct flow *flow, struct flow *base,
+commit_vlan_action(ovs_be16 vlan_tci, struct flow *base,
struct ofpbuf *odp_actions, struct flow_wildcards *wc)
{
- if (base->vlan_tci == flow->vlan_tci) {
+ if (base->vlan_tci == vlan_tci) {
return;
}
@@ -3339,15 +3339,15 @@ commit_vlan_action(const struct flow *flow, struct flow *base,
nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_VLAN);
}
- if (flow->vlan_tci & htons(VLAN_CFI)) {
+ if (vlan_tci & htons(VLAN_CFI)) {
struct ovs_action_push_vlan vlan;
vlan.vlan_tpid = htons(ETH_TYPE_VLAN);
- vlan.vlan_tci = flow->vlan_tci;
+ vlan.vlan_tci = vlan_tci;
nl_msg_put_unspec(odp_actions, OVS_ACTION_ATTR_PUSH_VLAN,
&vlan, sizeof vlan);
}
- base->vlan_tci = flow->vlan_tci;
+ base->vlan_tci = vlan_tci;
}
static void
@@ -3567,7 +3567,7 @@ commit_odp_actions(const struct flow *flow, struct flow *base,
struct ofpbuf *odp_actions, struct flow_wildcards *wc)
{
commit_set_ether_addr_action(flow, base, odp_actions, wc);
- commit_vlan_action(flow, base, odp_actions, wc);
+ commit_vlan_action(flow->vlan_tci, base, odp_actions, wc);
commit_set_nw_action(flow, base, odp_actions, wc);
commit_set_port_action(flow, base, odp_actions, wc);
/* Committing MPLS actions should occur after committing nw and port
--
1.8.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2.39 2/7] odp: Allow VLAN actions after MPLS actions
2013-09-09 7:20 [PATCH v2.39 0/7] MPLS actions and matches Simon Horman
[not found] ` <1378711207-29890-1-git-send-email-horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
@ 2013-09-09 7:20 ` Simon Horman
2013-09-09 7:20 ` [PATCH v2.39 4/7] ofp-actions: Add separate OpenFlow 1.3 action parser Simon Horman
` (5 subsequent siblings)
7 siblings, 0 replies; 38+ messages in thread
From: Simon Horman @ 2013-09-09 7:20 UTC (permalink / raw)
To: dev, netdev
Cc: Ravi K, Isaku Yamahata, Jesse Gross, Pravin B Shelar, Joe Stringer
From: Joe Stringer <joe@wand.net.nz>
OpenFlow 1.2 and 1.3 differ on their handling of MPLS actions in the
presence of VLAN tags. To allow correct behaviour to be committed in
each situation, this patch adds a second round of VLAN tag action
handling to commit_odp_actions(), which occurs after MPLS actions. This
is implemented with a new field in 'struct xlate_in' called 'vlan_tci'.
When an push_mpls action is composed, the flow's current VLAN state is
stored into xin->vlan_tci, and flow->vlan_tci is set to 0 (pop_vlan). If
a VLAN tag is present, it is stripped; if not, then there is no change.
Any later modifications to the VLAN state is written to xin->vlan_tci.
When committing the actions, flow->vlan_tci is used before MPLS actions,
and xin->vlan_tci is used afterwards. This retains the current datapath
behaviour, but allows VLAN actions to be applied in a more flexible
manner.
Signed-off-by: Joe Stringer <joe@wand.net.nz>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
v2.38 - v2.39
* No change
v2.37
* Rebase
v2.36
* No change
v2.5
* First post
---
lib/odp-util.c | 10 ++-
lib/odp-util.h | 4 +-
ofproto/ofproto-dpif-xlate.c | 95 +++++++++++++++-----
ofproto/ofproto-dpif-xlate.h | 5 ++
tests/ofproto-dpif.at | 209 +++++++++++++++++++++++++++++++++++++++++++
5 files changed, 297 insertions(+), 26 deletions(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 11aa32f..6ed7aa3 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -3561,10 +3561,15 @@ commit_set_pkt_mark_action(const struct flow *flow, struct flow *base,
* key from 'base' into 'flow', and then changes 'base' the same way. Does not
* commit set_tunnel actions. Users should call commit_odp_tunnel_action()
* in addition to this function if needed. Sets fields in 'wc' that are
- * used as part of the action. */
+ * used as part of the action.
+ *
+ * VLAN actions may be committed twice; If vlan_tci in 'flow' differs from the
+ * one in 'base', then it is committed before MPLS actions. If 'final_vlan_tci'
+ * differs from 'flow->vlan_tci', it is committed afterwards. */
void
commit_odp_actions(const struct flow *flow, struct flow *base,
- struct ofpbuf *odp_actions, struct flow_wildcards *wc)
+ struct ofpbuf *odp_actions, struct flow_wildcards *wc,
+ ovs_be16 final_vlan_tci)
{
commit_set_ether_addr_action(flow, base, odp_actions, wc);
commit_vlan_action(flow->vlan_tci, base, odp_actions, wc);
@@ -3575,6 +3580,7 @@ commit_odp_actions(const struct flow *flow, struct flow *base,
* that it is no longer IP and thus nw and port actions are no longer valid.
*/
commit_mpls_action(flow, base, odp_actions, wc);
+ commit_vlan_action(final_vlan_tci, base, odp_actions, wc);
commit_set_priority_action(flow, base, odp_actions, wc);
commit_set_pkt_mark_action(flow, base, odp_actions, wc);
}
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 192cfa0..be68d4e 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -130,8 +130,8 @@ const char *odp_key_fitness_to_string(enum odp_key_fitness);
void commit_odp_tunnel_action(const struct flow *, struct flow *base,
struct ofpbuf *odp_actions);
void commit_odp_actions(const struct flow *, struct flow *base,
- struct ofpbuf *odp_actions,
- struct flow_wildcards *wc);
+ struct ofpbuf *odp_actions, struct flow_wildcards *wc,
+ ovs_be16 final_vlan_tci);
\f
/* ofproto-dpif interface.
*
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index e7cec14..2fa7785 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -974,10 +974,11 @@ static void
output_normal(struct xlate_ctx *ctx, const struct xbundle *out_xbundle,
uint16_t vlan)
{
- ovs_be16 *flow_tci = &ctx->xin->flow.vlan_tci;
+ ovs_be16 *flow_tci = &ctx->xin->vlan_tci;
uint16_t vid;
ovs_be16 tci, old_tci;
struct xport *xport;
+ bool flow_tci_equal_to_xin = (*flow_tci == ctx->xin->flow.vlan_tci);
vid = output_vlan_to_vid(out_xbundle, vlan);
if (list_is_empty(&out_xbundle->xports)) {
@@ -1008,9 +1009,15 @@ output_normal(struct xlate_ctx *ctx, const struct xbundle *out_xbundle,
}
}
*flow_tci = tci;
+ if (flow_tci_equal_to_xin) {
+ ctx->xin->flow.vlan_tci = tci;
+ }
compose_output_action(ctx, xport->ofp_port);
*flow_tci = old_tci;
+ if (flow_tci_equal_to_xin) {
+ ctx->xin->flow.vlan_tci = old_tci;
+ }
}
/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
@@ -1243,7 +1250,7 @@ xlate_normal(struct xlate_ctx *ctx)
/* Drop malformed frames. */
if (flow->dl_type == htons(ETH_TYPE_VLAN) &&
- !(flow->vlan_tci & htons(VLAN_CFI))) {
+ !(ctx->xin->vlan_tci & htons(VLAN_CFI))) {
if (ctx->xin->packet != NULL) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
VLOG_WARN_RL(&rl, "bridge %s: dropping packet with partial "
@@ -1267,7 +1274,7 @@ xlate_normal(struct xlate_ctx *ctx)
}
/* Check VLAN. */
- vid = vlan_tci_to_vid(flow->vlan_tci);
+ vid = vlan_tci_to_vid(ctx->xin->vlan_tci);
if (!input_vid_is_valid(vid, in_xbundle, ctx->xin->packet != NULL)) {
xlate_report(ctx, "disallowed VLAN VID for this input port, dropping");
return;
@@ -1525,7 +1532,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
const struct xport *xport = get_ofp_port(ctx->xbridge, ofp_port);
struct flow_wildcards *wc = &ctx->xout->wc;
struct flow *flow = &ctx->xin->flow;
- ovs_be16 flow_vlan_tci;
+ ovs_be16 flow_vlan_tci, xin_vlan_tci;
uint32_t flow_pkt_mark;
uint8_t flow_nw_tos;
odp_port_t out_port, odp_port;
@@ -1594,6 +1601,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
}
flow_vlan_tci = flow->vlan_tci;
+ xin_vlan_tci = ctx->xin->vlan_tci;
flow_pkt_mark = flow->pkt_mark;
flow_nw_tos = flow->nw_tos;
@@ -1633,18 +1641,19 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
}
vlandev_port = vsp_realdev_to_vlandev(ctx->xbridge->ofproto, ofp_port,
- flow->vlan_tci);
+ ctx->xin->vlan_tci);
if (vlandev_port == ofp_port) {
out_port = odp_port;
} else {
out_port = ofp_port_to_odp_port(ctx->xbridge, vlandev_port);
flow->vlan_tci = htons(0);
+ ctx->xin->vlan_tci = htons(0);
}
}
if (out_port != ODPP_NONE) {
- commit_odp_actions(flow, &ctx->base_flow,
- &ctx->xout->odp_actions, &ctx->xout->wc);
+ commit_odp_actions(flow, &ctx->base_flow, &ctx->xout->odp_actions,
+ &ctx->xout->wc, ctx->xin->vlan_tci);
nl_msg_put_odp_port(&ctx->xout->odp_actions, OVS_ACTION_ATTR_OUTPUT,
out_port);
@@ -1656,6 +1665,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
out:
/* Restore flow */
flow->vlan_tci = flow_vlan_tci;
+ ctx->xin->vlan_tci = xin_vlan_tci;
flow->pkt_mark = flow_pkt_mark;
flow->nw_tos = flow_nw_tos;
}
@@ -1802,7 +1812,8 @@ execute_controller_action(struct xlate_ctx *ctx, int len,
memset(&key.tunnel, 0, sizeof key.tunnel);
commit_odp_actions(&ctx->xin->flow, &ctx->base_flow,
- &ctx->xout->odp_actions, &ctx->xout->wc);
+ &ctx->xout->odp_actions, &ctx->xout->wc,
+ ctx->xin->vlan_tci);
odp_execute_actions(NULL, packet, &key, ctx->xout->odp_actions.data,
ctx->xout->odp_actions.size, NULL, NULL);
@@ -2135,8 +2146,8 @@ xlate_sample_action(struct xlate_ctx *ctx,
* the same percentage. */
uint32_t probability = (os->probability << 16) | os->probability;
- commit_odp_actions(&ctx->xin->flow, &ctx->base_flow,
- &ctx->xout->odp_actions, &ctx->xout->wc);
+ commit_odp_actions(&ctx->xin->flow, &ctx->base_flow, &ctx->xout->odp_actions,
+ &ctx->xout->wc, ctx->xin->vlan_tci);
compose_flow_sample_cookie(os->probability, os->collector_set_id,
os->obs_domain_id, os->obs_point_id, &cookie);
@@ -2165,11 +2176,23 @@ may_receive(const struct xport *xport, struct xlate_ctx *ctx)
}
static void
+vlan_tci_restore(struct xlate_in *xin, ovs_be16 *tci_ptr, ovs_be16 orig_tci)
+{
+ /* If MPLS actions were executed after MPLS, copy the final vlan_tci out
+ * and restore the intermediate VLAN state. */
+ if (xin->flow.vlan_tci != orig_tci && tci_ptr == &xin->vlan_tci) {
+ xin->vlan_tci = xin->flow.vlan_tci;
+ xin->flow.vlan_tci = orig_tci;
+ }
+}
+
+static void
do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
struct xlate_ctx *ctx)
{
struct flow_wildcards *wc = &ctx->xout->wc;
struct flow *flow = &ctx->xin->flow;
+ ovs_be16 *vlan_tci = &ctx->xin->flow.vlan_tci;
const struct ofpact *a;
OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
@@ -2180,6 +2203,15 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
break;
}
+ /* Update the final vlan state to be equal to the current state.
+ * - If 'vlan_tci' points to 'xin->flow->vlan_tci'. then additional
+ * VLAN actions will be applied before MPLS actions. 'xin->vlan_tci'
+ * is updated to reflect the final state of the flow.
+ * - If 'vlan_tci' already points to 'xin->vlan_tci', then additional
+ * VLAN actions will be applied after MPLS actions. 'xin->vlan_tci'
+ * is already equal to the current state. */
+ ctx->xin->vlan_tci = *vlan_tci;
+
switch (a->type) {
case OFPACT_OUTPUT:
xlate_output_action(ctx, ofpact_get_OUTPUT(a)->port,
@@ -2203,28 +2235,28 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
case OFPACT_SET_VLAN_VID:
wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
- flow->vlan_tci &= ~htons(VLAN_VID_MASK);
- flow->vlan_tci |= (htons(ofpact_get_SET_VLAN_VID(a)->vlan_vid)
- | htons(VLAN_CFI));
+ *vlan_tci &= ~htons(VLAN_VID_MASK);
+ *vlan_tci |= (htons(ofpact_get_SET_VLAN_VID(a)->vlan_vid)
+ | htons(VLAN_CFI));
break;
case OFPACT_SET_VLAN_PCP:
- wc->masks.vlan_tci |= htons(VLAN_PCP_MASK | VLAN_CFI);
- flow->vlan_tci &= ~htons(VLAN_PCP_MASK);
- flow->vlan_tci |=
+ wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
+ *vlan_tci &= ~htons(VLAN_PCP_MASK);
+ *vlan_tci |=
htons((ofpact_get_SET_VLAN_PCP(a)->vlan_pcp << VLAN_PCP_SHIFT)
| VLAN_CFI);
break;
case OFPACT_STRIP_VLAN:
memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
- flow->vlan_tci = htons(0);
+ *vlan_tci = htons(0);
break;
case OFPACT_PUSH_VLAN:
/* XXX 802.1AD(QinQ) */
memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
- flow->vlan_tci = htons(VLAN_CFI);
+ *vlan_tci = htons(VLAN_CFI);
break;
case OFPACT_SET_ETH_SRC:
@@ -2292,26 +2324,44 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
flow->skb_priority = ctx->orig_skb_priority;
break;
- case OFPACT_REG_MOVE:
+ case OFPACT_REG_MOVE: {
+ ovs_be16 orig_tci = flow->vlan_tci;
nxm_execute_reg_move(ofpact_get_REG_MOVE(a), flow, wc);
+ vlan_tci_restore(ctx->xin, vlan_tci, orig_tci);
break;
+ }
- case OFPACT_REG_LOAD:
+ case OFPACT_REG_LOAD: {
+ ovs_be16 orig_tci = flow->vlan_tci;
nxm_execute_reg_load(ofpact_get_REG_LOAD(a), flow);
+ vlan_tci_restore(ctx->xin, vlan_tci, orig_tci);
break;
+ }
- case OFPACT_STACK_PUSH:
+ case OFPACT_STACK_PUSH: {
+ ovs_be16 orig_tci = flow->vlan_tci;
+ flow->vlan_tci = *vlan_tci;
nxm_execute_stack_push(ofpact_get_STACK_PUSH(a), flow, wc,
&ctx->stack);
+ flow->vlan_tci = orig_tci;
break;
+ }
- case OFPACT_STACK_POP:
+ case OFPACT_STACK_POP: {
+ ovs_be16 orig_tci = flow->vlan_tci;
nxm_execute_stack_pop(ofpact_get_STACK_POP(a), flow, wc,
&ctx->stack);
+ vlan_tci_restore(ctx->xin, vlan_tci, orig_tci);
break;
+ }
case OFPACT_PUSH_MPLS:
compose_mpls_push_action(ctx, ofpact_get_PUSH_MPLS(a)->ethertype);
+
+ /* Save and pop any existing VLAN tags if running in OF1.2 mode. */
+ ctx->xin->vlan_tci = *vlan_tci;
+ flow->vlan_tci = htons(0);
+ vlan_tci = &ctx->xin->vlan_tci;
break;
case OFPACT_POP_MPLS:
@@ -2410,6 +2460,7 @@ xlate_in_init(struct xlate_in *xin, struct ofproto_dpif *ofproto,
{
xin->ofproto = ofproto;
xin->flow = *flow;
+ xin->vlan_tci = flow->vlan_tci;
xin->packet = packet;
xin->may_learn = packet != NULL;
xin->rule = rule;
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index a54a9e4..6ce3b31 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -60,6 +60,11 @@ struct xlate_in {
* this flow when actions change header fields. */
struct flow flow;
+ /* If MPLS and VLAN actions were both present in the translation, and VLAN
+ * actions should occur after the MPLS actions, then this field is used
+ * to store the final vlan_tci state. */
+ ovs_be16 vlan_tci;
+
/* The packet corresponding to 'flow', or a null pointer if we are
* revalidating without a packet to refer to. */
const struct ofpbuf *packet;
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index f67c3ab..605feeb 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -847,6 +847,215 @@ done
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - OF1.2 VLAN+MPLS handling])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy
+])
+ON_EXIT([kill `cat ovs-ofctl.pid`])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_DATA([flows.txt], [dnl
+cookie=0xa dl_src=40:44:44:44:54:50 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],push_vlan:0x8100,mod_vlan_vid:99,mod_vlan_pcp:1,controller
+cookie=0xa dl_src=40:44:44:44:54:51 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],push_vlan:0x8100,mod_vlan_vid:99,mod_vlan_pcp:1,controller
+cookie=0xa dl_src=40:44:44:44:54:52 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],push_vlan:0x8100,load:99->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,controller
+cookie=0xa dl_src=40:44:44:44:54:53 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],push_vlan:0x8100,load:99->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,controller
+cookie=0xa dl_src=40:44:44:44:54:54 actions=push_vlan:0x8100,mod_vlan_vid:99,mod_vlan_pcp:1,push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],controller
+cookie=0xa dl_src=40:44:44:44:54:55 actions=push_vlan:0x8100,mod_vlan_vid:99,mod_vlan_pcp:1,push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],controller
+cookie=0xa dl_src=40:44:44:44:54:56 actions=push_vlan:0x8100,load:99->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],controller
+cookie=0xa dl_src=40:44:44:44:54:57 actions=push_vlan:0x8100,load:99->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],controller
+])
+AT_CHECK([ovs-ofctl --protocols=OpenFlow12 add-flows br0 flows.txt])
+
+dnl Modified MPLS controller action.
+dnl In this test, we push the MPLS tag before pushing a VLAN tag, so we see
+dnl both of these in the final flow
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:50,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no)'
+done
+OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:50,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:50,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:50,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, the input packet in vlan-tagged, which should be stripped
+dnl before we push the MPLS and VLAN tags.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:51,dst=50:54:00:00:00:07),eth_type(0x8100),vlan(vid=88,pcp=7),encap(eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no))'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:51,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:51,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:51,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, we push the MPLS tag before pushing a VLAN tag, so we see
+dnl both of these in the final flow
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:52,dst=52:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no)'
+done
+OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit])
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:52,dl_dst=52:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:52,dl_dst=52:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:52,dl_dst=52:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, the input packet in vlan-tagged, which should be stripped
+dnl before we push the MPLS and VLAN tags.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:53,dst=50:54:00:00:00:07),eth_type(0x8100),vlan(vid=88,pcp=7),encap(eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no))'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:53,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:53,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:53,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, we push the VLAN tag before pushing a MPLS tag, but these
+dnl actions are reordered, so we see both of these in the final flow.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:54,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no)'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:54,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:54,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:54,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, the input packet in vlan-tagged, which should be stripped
+dnl before we push the MPLS and VLAN tags.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:55,dst=50:54:00:00:00:07),eth_type(0x8100),vlan(vid=88,pcp=7),encap(eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no))'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:55,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:55,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:55,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, we push the VLAN tag before pushing a MPLS tag, but these
+dnl actions are reordered, so we see both of these in the final flow.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:56,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no)'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:56,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:56,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:56,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, the input packet in vlan-tagged, which should be stripped
+dnl before we push the MPLS and VLAN tags.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:54:57,dst=50:54:00:00:00:07),eth_type(0x8100),vlan(vid=88,pcp=7),encap(eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no))'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:57,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:57,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:54:57,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=0,mpls_ttl=64,mpls_bos=1
+])
+
+AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
+AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:50 actions=push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],mod_vlan_vid:99,mod_vlan_pcp:1,CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:51 actions=push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],mod_vlan_vid:99,mod_vlan_pcp:1,CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:52 actions=push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],load:0x63->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:53 actions=push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],load:0x63->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:54 actions=mod_vlan_vid:99,mod_vlan_pcp:1,push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:55 actions=mod_vlan_vid:99,mod_vlan_pcp:1,push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:56 actions=load:0x63->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:54:57 actions=load:0x63->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],CONTROLLER:65535
+NXST_FLOW reply:
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - fragment handling])
OVS_VSWITCHD_START
ADD_OF_PORTS([br0], [1], [2], [3], [4], [5], [6], [90])
--
1.8.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2.39 3/7] ofp-actions: Add OFPUTIL_OFPAT13_PUSH_MPLS
[not found] ` <1378711207-29890-1-git-send-email-horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
2013-09-09 7:20 ` [PATCH v2.39 1/7] odp: Only pass vlan_tci to commit_vlan_action() Simon Horman
@ 2013-09-09 7:20 ` Simon Horman
1 sibling, 0 replies; 38+ messages in thread
From: Simon Horman @ 2013-09-09 7:20 UTC (permalink / raw)
To: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA
Cc: Isaku Yamahata, Ravi K
From: Joe Stringer <joe-Q1GJJQv1iO6lP80pJB477g@public.gmane.org>
This patch adds a new compatibility enum for use with MPLS, so that the
differing behaviour between OpenFlow 1.2 and 1.3 can be implemented in
ofproto-dpif-xlate.
Signed-off-by: Joe Stringer <joe-Q1GJJQv1iO6lP80pJB477g@public.gmane.org>
Signed-off-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
---
v2.36 - v2.39
* No change
v2.35
* First post
---
lib/ofp-actions.c | 5 ++++-
lib/ofp-parse.c | 1 +
lib/ofp-util.c | 3 +++
lib/ofp-util.h | 1 +
4 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 77aa69c..964ecd7 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -322,6 +322,7 @@ ofpact_from_nxast(const union ofp_action *a, enum ofputil_action_code code,
#define OFPAT10_ACTION(ENUM, STRUCT, NAME) case OFPUTIL_##ENUM:
#define OFPAT11_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) case OFPUTIL_##ENUM:
#include "ofp-util.def"
+ case OFPUTIL_OFPAT13_PUSH_MPLS:
NOT_REACHED();
case OFPUTIL_NXAST_RESUBMIT:
@@ -480,6 +481,7 @@ ofpact_from_openflow10(const union ofp_action *a, struct ofpbuf *out)
case OFPUTIL_ACTION_INVALID:
#define OFPAT11_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) case OFPUTIL_##ENUM:
#include "ofp-util.def"
+ case OFPUTIL_OFPAT13_PUSH_MPLS:
NOT_REACHED();
case OFPUTIL_OFPAT10_OUTPUT:
@@ -842,7 +844,8 @@ ofpact_from_openflow11(const union ofp_action *a, struct ofpbuf *out)
ofpact_put_DEC_MPLS_TTL(out);
break;
- case OFPUTIL_OFPAT11_PUSH_MPLS: {
+ case OFPUTIL_OFPAT11_PUSH_MPLS:
+ case OFPUTIL_OFPAT13_PUSH_MPLS: {
struct ofp11_action_push *oap = (struct ofp11_action_push *)a;
if (!eth_type_mpls(oap->ethertype)) {
return OFPERR_OFPBAC_BAD_ARGUMENT;
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index a61beb9..9aac20b 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -804,6 +804,7 @@ parse_named_action(enum ofputil_action_code code,
break;
case OFPUTIL_OFPAT11_PUSH_MPLS:
+ case OFPUTIL_OFPAT13_PUSH_MPLS:
case OFPUTIL_NXAST_PUSH_MPLS:
error = str_to_u16(arg, "push_mpls", ðertype);
if (!error) {
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 23c7136..87336e2 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -4768,6 +4768,9 @@ ofputil_put_action(enum ofputil_action_code code, struct ofpbuf *buf)
case OFPUTIL_ACTION_INVALID:
NOT_REACHED();
+ case OFPUTIL_OFPAT13_PUSH_MPLS:
+ return ofputil_put_OFPAT11_PUSH_MPLS(buf);
+
#define OFPAT10_ACTION(ENUM, STRUCT, NAME) \
case OFPUTIL_##ENUM: return ofputil_put_##ENUM(buf);
#define OFPAT11_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) \
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 0ca483c..3ca189d 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -787,6 +787,7 @@ enum OVS_PACKED_ENUM ofputil_action_code {
#define OFPAT11_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) OFPUTIL_##ENUM,
#define NXAST_ACTION(ENUM, STRUCT, EXTENSIBLE, NAME) OFPUTIL_##ENUM,
#include "ofp-util.def"
+ OFPUTIL_OFPAT13_PUSH_MPLS
};
/* The number of values of "enum ofputil_action_code". */
--
1.8.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2.39 4/7] ofp-actions: Add separate OpenFlow 1.3 action parser
2013-09-09 7:20 [PATCH v2.39 0/7] MPLS actions and matches Simon Horman
[not found] ` <1378711207-29890-1-git-send-email-horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
2013-09-09 7:20 ` [PATCH v2.39 2/7] odp: Allow VLAN actions after MPLS actions Simon Horman
@ 2013-09-09 7:20 ` Simon Horman
2013-09-09 7:20 ` [PATCH v2.39 5/7] lib: Push MPLS tags in the OpenFlow 1.3 ordering Simon Horman
` (4 subsequent siblings)
7 siblings, 0 replies; 38+ messages in thread
From: Simon Horman @ 2013-09-09 7:20 UTC (permalink / raw)
To: dev, netdev
Cc: Ravi K, Isaku Yamahata, Jesse Gross, Pravin B Shelar, Joe Stringer
From: Joe Stringer <joe@wand.net.nz>
This patch adds new ofpact_from_openflow13() and
ofpacts_from_openflow13() functions parallel to the existing ofpact
handling code. In the OpenFlow 1.3 version, push_mpls is handled
differently, but all other actions are handled by the existing code.
For push_mpls, ofpact_push_mpls.ofpact.compat is set to
OFPUTIL_OFPAT13_PUSH_MPLS, which allows correct VLAN+MPLS datapath
behaviour to be determined at odp translation time.
Signed-off-by: Joe Stringer <joe@wand.net.nz>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
v2.36 - v2.39
* No change
v2.35
* First post
---
lib/ofp-actions.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 60 insertions(+), 3 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 964ecd7..a890773 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -884,6 +884,40 @@ ofpacts_from_openflow11(const union ofp_action *in, size_t n_in,
return ofpacts_from_openflow(in, n_in, out, ofpact_from_openflow11);
}
\f
+static enum ofperr
+ofpact_from_openflow13(const union ofp_action *a, struct ofpbuf *out)
+{
+ enum ofputil_action_code code;
+ enum ofperr error;
+
+ error = decode_openflow11_action(a, &code);
+ if (error) {
+ return error;
+ }
+
+ if (code == OFPUTIL_OFPAT11_PUSH_MPLS) {
+ struct ofpact_push_mpls *oam;
+ struct ofp11_action_push *oap = (struct ofp11_action_push *)a;
+ if (!eth_type_mpls(oap->ethertype)) {
+ return OFPERR_OFPBAC_BAD_ARGUMENT;
+ }
+ oam = ofpact_put_PUSH_MPLS(out);
+ oam->ethertype = oap->ethertype;
+ oam->ofpact.compat = OFPUTIL_OFPAT13_PUSH_MPLS;
+ } else {
+ return ofpact_from_openflow11(a, out);
+ }
+
+ return error;
+}
+
+static enum ofperr
+ofpacts_from_openflow13(const union ofp_action *in, size_t n_in,
+ struct ofpbuf *out)
+{
+ return ofpacts_from_openflow(in, n_in, out, ofpact_from_openflow13);
+}
+\f
/* OpenFlow 1.1 instructions. */
#define DEFINE_INST(ENUM, STRUCT, EXTENSIBLE, NAME) \
@@ -1088,6 +1122,17 @@ get_actions_from_instruction(const struct ofp11_instruction *inst,
*n_actions = (ntohs(inst->len) - sizeof *inst) / OFP11_INSTRUCTION_ALIGN;
}
+static uint8_t
+get_version_from_ofpbuf(const struct ofpbuf *openflow)
+{
+ if (openflow && openflow->l2) {
+ struct ofp_header *oh = openflow->l2;
+ return oh->version;
+ }
+
+ return OFP10_VERSION;
+}
+
/* Attempts to convert 'actions_len' bytes of OpenFlow 1.1 actions from the
* front of 'openflow' into ofpacts. On success, replaces any existing content
* in 'ofpacts' by the converted ofpacts; on failure, clears 'ofpacts'.
@@ -1107,8 +1152,15 @@ ofpacts_pull_openflow11_actions(struct ofpbuf *openflow,
unsigned int actions_len,
struct ofpbuf *ofpacts)
{
- return ofpacts_pull_actions(openflow, actions_len, ofpacts,
- ofpacts_from_openflow11);
+ uint8_t version = get_version_from_ofpbuf(openflow);
+
+ if (version < OFP13_VERSION) {
+ return ofpacts_pull_actions(openflow, actions_len, ofpacts,
+ ofpacts_from_openflow11);
+ } else {
+ return ofpacts_pull_actions(openflow, actions_len, ofpacts,
+ ofpacts_from_openflow13);
+ }
}
enum ofperr
@@ -1160,10 +1212,15 @@ ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
if (insts[OVSINST_OFPIT11_APPLY_ACTIONS]) {
const union ofp_action *actions;
size_t n_actions;
+ uint8_t version = get_version_from_ofpbuf(openflow);
get_actions_from_instruction(insts[OVSINST_OFPIT11_APPLY_ACTIONS],
&actions, &n_actions);
- error = ofpacts_from_openflow11(actions, n_actions, ofpacts);
+ if (version < OFP13_VERSION) {
+ error = ofpacts_from_openflow11(actions, n_actions, ofpacts);
+ } else {
+ error = ofpacts_from_openflow13(actions, n_actions, ofpacts);
+ }
if (error) {
goto exit;
}
--
1.8.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2.39 5/7] lib: Push MPLS tags in the OpenFlow 1.3 ordering
2013-09-09 7:20 [PATCH v2.39 0/7] MPLS actions and matches Simon Horman
` (2 preceding siblings ...)
2013-09-09 7:20 ` [PATCH v2.39 4/7] ofp-actions: Add separate OpenFlow 1.3 action parser Simon Horman
@ 2013-09-09 7:20 ` Simon Horman
2013-09-09 7:20 ` [PATCH v2.39 6/7] datapath: Break out deacceleration portion of vlan_push Simon Horman
` (3 subsequent siblings)
7 siblings, 0 replies; 38+ messages in thread
From: Simon Horman @ 2013-09-09 7:20 UTC (permalink / raw)
To: dev, netdev
Cc: Ravi K, Isaku Yamahata, Jesse Gross, Pravin B Shelar, Joe Stringer
From: Joe Stringer <joe@wand.net.nz>
This patch modifies the push_mpls behaviour to follow the OpenFlow 1.3
specification in the presence of VLAN tagged packets. From the spec:
"Newly pushed tags should always be inserted as the outermost tag in the
outermost valid location for that tag. When a new VLAN tag is pushed, it
should be the outermost tag inserted, immediately after the Ethernet
header and before other tags. Likewise, when a new MPLS tag is pushed,
it should be the outermost tag inserted, immediately after the Ethernet
header and before other tags."
When the push_mpls action was inserted using OpenFlow 1.2, we implement
the previous behaviour by inserting VLAN actions around the MPLS action
in the odp translation; Pop VLAN tags before committing MPLS actions,
and push the expected VLAN tag afterwards. The trigger condition for
this is based on the ofpact->compat field.
Signed-off-by: Joe Stringer <joe@wand.net.nz>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
v2.36 - v2.39
* No change
v2.35
* First post
---
lib/flow.c | 2 +-
lib/packets.c | 10 +-
lib/packets.h | 2 +-
ofproto/ofproto-dpif-xlate.c | 10 +-
tests/ofproto-dpif.at | 237 +++++++++++++++++++++++++++++++++++++++++++
5 files changed, 253 insertions(+), 8 deletions(-)
diff --git a/lib/flow.c b/lib/flow.c
index 9ab1961..64a0e0c 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1063,7 +1063,7 @@ flow_compose(struct ofpbuf *b, const struct flow *flow)
}
if (eth_type_mpls(flow->dl_type)) {
- b->l2_5 = b->l3;
+ b->l2_5 = (char*)b->l2 + ETH_HEADER_LEN;
push_mpls(b, flow->dl_type, flow->mpls_lse);
}
}
diff --git a/lib/packets.c b/lib/packets.c
index d15c402..6637575 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -246,11 +246,11 @@ eth_mpls_depth(const struct ofpbuf *packet)
/* Set ethertype of the packet. */
void
-set_ethertype(struct ofpbuf *packet, ovs_be16 eth_type)
+set_ethertype(struct ofpbuf *packet, ovs_be16 eth_type, bool inner)
{
struct eth_header *eh = packet->data;
- if (eh->eth_type == htons(ETH_TYPE_VLAN)) {
+ if (inner && eh->eth_type == htons(ETH_TYPE_VLAN)) {
ovs_be16 *p;
p = ALIGNED_CAST(ovs_be16 *,
(char *)(packet->l2_5 ? packet->l2_5 : packet->l3) - 2);
@@ -358,8 +358,8 @@ push_mpls(struct ofpbuf *packet, ovs_be16 ethtype, ovs_be32 lse)
if (!is_mpls(packet)) {
/* Set ethtype and MPLS label stack entry. */
- set_ethertype(packet, ethtype);
- packet->l2_5 = packet->l3;
+ set_ethertype(packet, ethtype, false);
+ packet->l2_5 = (char*)packet->l2 + ETH_HEADER_LEN;
}
/* Push new MPLS shim header onto packet. */
@@ -380,7 +380,7 @@ pop_mpls(struct ofpbuf *packet, ovs_be16 ethtype)
size_t len;
mh = packet->l2_5;
len = (char*)packet->l2_5 - (char*)packet->l2;
- set_ethertype(packet, ethtype);
+ set_ethertype(packet, ethtype, true);
if (mh->mpls_lse & htonl(MPLS_BOS_MASK)) {
packet->l2_5 = NULL;
} else {
diff --git a/lib/packets.h b/lib/packets.h
index b776914..555a8f3 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -145,7 +145,7 @@ void eth_pop_vlan(struct ofpbuf *);
uint16_t eth_mpls_depth(const struct ofpbuf *packet);
-void set_ethertype(struct ofpbuf *packet, ovs_be16 eth_type);
+void set_ethertype(struct ofpbuf *packet, ovs_be16 eth_type, bool inner);
const char *eth_from_hex(const char *hex, struct ofpbuf **packetp);
void eth_format_masked(const uint8_t eth[ETH_ADDR_LEN],
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 2fa7785..c690db1 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2175,6 +2175,12 @@ may_receive(const struct xport *xport, struct xlate_ctx *ctx)
return true;
}
+static bool
+mpls_compat_behaviour(enum ofputil_action_code compat)
+{
+ return (compat != OFPUTIL_OFPAT13_PUSH_MPLS);
+}
+
static void
vlan_tci_restore(struct xlate_in *xin, ovs_be16 *tci_ptr, ovs_be16 orig_tci)
{
@@ -2360,7 +2366,9 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
/* Save and pop any existing VLAN tags if running in OF1.2 mode. */
ctx->xin->vlan_tci = *vlan_tci;
- flow->vlan_tci = htons(0);
+ if (mpls_compat_behaviour(a->compat)) {
+ flow->vlan_tci = htons(0);
+ }
vlan_tci = &ctx->xin->vlan_tci;
break;
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 605feeb..0a40e69 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -1056,6 +1056,243 @@ NXST_FLOW reply:
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - OF1.3+ VLAN+MPLS handling])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy
+])
+ON_EXIT([kill `cat ovs-ofctl.pid`])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_DATA([flows.txt], [dnl
+cookie=0xa dl_src=40:44:44:44:55:44 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],load:3->OXM_OF_MPLS_TC[[]],controller
+cookie=0xa dl_src=40:44:44:44:55:45 actions=push_vlan:0x8100,mod_vlan_vid:99,push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],load:3->OXM_OF_MPLS_TC[[]],controller
+cookie=0xa dl_src=40:44:44:44:55:46 actions=push_vlan:0x8100,mod_vlan_vid:99,push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],load:3->OXM_OF_MPLS_TC[[]],controller
+cookie=0xa dl_src=40:44:44:44:55:47 actions=push_vlan:0x8100,load:99->OXM_OF_VLAN_VID[[]],push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],load:3->OXM_OF_MPLS_TC[[]],controller
+cookie=0xa dl_src=40:44:44:44:55:48 actions=push_vlan:0x8100,load:99->OXM_OF_VLAN_VID[[]],push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],load:3->OXM_OF_MPLS_TC[[]],controller
+cookie=0xa dl_src=40:44:44:44:55:49 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],load:3->OXM_OF_MPLS_TC[[]],push_vlan:0x8100,mod_vlan_vid:99,controller
+cookie=0xa dl_src=40:44:44:44:55:50 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],load:3->OXM_OF_MPLS_TC[[]],push_vlan:0x8100,mod_vlan_vid:99,controller
+cookie=0xa dl_src=40:44:44:44:55:51 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],load:3->OXM_OF_MPLS_TC[[]],push_vlan:0x8100,load:99->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,controller
+cookie=0xa dl_src=40:44:44:44:55:52 actions=push_mpls:0x8847,load:10->OXM_OF_MPLS_LABEL[[]],load:3->OXM_OF_MPLS_TC[[]],push_vlan:0x8100,load:99->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,controller
+])
+AT_CHECK([ovs-ofctl --protocols=OpenFlow13 add-flows br0 flows.txt])
+
+dnl Modified MPLS controller action.
+dnl The input packet has a VLAN tag, but because we push an MPLS tag in
+dnl OF1.3 mode, we can no longer see it.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:55:44,dst=50:54:00:00:00:07),eth_type(0x8100),vlan(vid=88,pcp=7),encap(eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no))'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=40:44:44:44:55:44,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=40:44:44:44:55:44,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=40:44:44:44:55:44,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, we push a VLAN tag, then an MPLS tag in OF1.3 mode, so we
+dnl can only see the MPLS tag in the result.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:55:45,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no)'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=40:44:44:44:55:45,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=40:44:44:44:55:45,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=40:44:44:44:55:45,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, the input packet is vlan-tagged; we update this tag then
+dnl push an MPLS tag in OF1.3 mode. As such, we can only see the MPLS tag in
+dnl the result.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:55:46,dst=50:54:00:00:00:07),eth_type(0x8100),vlan(vid=88,pcp=7),encap(eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no))'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=40:44:44:44:55:46,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=40:44:44:44:55:46,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=40:44:44:44:55:46,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, we push a VLAN tag, then an MPLS tag in OF1.3 mode, so we
+dnl can only see the MPLS tag in the result.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:55:47,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no)'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=40:44:44:44:55:47,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=40:44:44:44:55:47,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=40:44:44:44:55:47,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, the input packet is vlan-tagged; we update this tag then
+dnl push an MPLS tag in OF1.3 mode. As such, we can only see the MPLS tag in
+dnl the result.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:55:48,dst=50:54:00:00:00:07),eth_type(0x8100),vlan(vid=88,pcp=7),encap(eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no))'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=40:44:44:44:55:48,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=40:44:44:44:55:48,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=64 in_port=1 (via action) data_len=64 (unbuffered)
+mpls,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=40:44:44:44:55:48,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, we push the MPLS tag before pushing a VLAN tag, so we see
+dnl both of these in the final flow.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:55:49,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no)'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=0,dl_src=40:44:44:44:55:49,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=0,dl_src=40:44:44:44:55:49,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=0,dl_src=40:44:44:44:55:49,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, the input packet in vlan-tagged, which should be stripped
+dnl before we push the MPLS and VLAN tags.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:55:50,dst=50:54:00:00:00:07),eth_type(0x8100),vlan(vid=88,pcp=7),encap(eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no))'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=0,dl_src=40:44:44:44:55:50,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=0,dl_src=40:44:44:44:55:50,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=0,dl_src=40:44:44:44:55:50,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, we push the MPLS tag before pushing a VLAN tag, so we see
+dnl both of these in the final flow.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:55:51,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no)'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:55:51,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:55:51,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:55:51,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+])
+
+dnl Modified MPLS controller action.
+dnl In this test, the input packet in vlan-tagged, which should be stripped
+dnl before we push the MPLS and VLAN tags.
+AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2> ofctl_monitor.log])
+
+for i in 1 2 3; do
+ ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=40:44:44:44:55:52,dst=50:54:00:00:00:07),eth_type(0x8100),vlan(vid=88,pcp=7),encap(eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no))'
+done
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+ovs-appctl -t ovs-ofctl exit
+
+AT_CHECK([cat ofctl_monitor.log], [0], [dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:55:52,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:55:52,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+dnl
+NXT_PACKET_IN (xid=0x0): cookie=0xa total_len=68 in_port=1 (via action) data_len=68 (unbuffered)
+mpls,metadata=0,in_port=0,dl_vlan=99,dl_vlan_pcp=1,dl_src=40:44:44:44:55:52,dl_dst=50:54:00:00:00:07,mpls_label=10,mpls_tc=3,mpls_ttl=64,mpls_bos=1
+])
+
+AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
+AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:55:44 actions=push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],load:0x3->OXM_OF_MPLS_TC[[]],CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:55:45 actions=mod_vlan_vid:99,push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],load:0x3->OXM_OF_MPLS_TC[[]],CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:55:46 actions=mod_vlan_vid:99,push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],load:0x3->OXM_OF_MPLS_TC[[]],CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:55:47 actions=load:0x63->OXM_OF_VLAN_VID[[]],push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],load:0x3->OXM_OF_MPLS_TC[[]],CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:55:48 actions=load:0x63->OXM_OF_VLAN_VID[[]],push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],load:0x3->OXM_OF_MPLS_TC[[]],CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:55:49 actions=push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],load:0x3->OXM_OF_MPLS_TC[[]],mod_vlan_vid:99,CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:55:50 actions=push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],load:0x3->OXM_OF_MPLS_TC[[]],mod_vlan_vid:99,CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:55:51 actions=push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],load:0x3->OXM_OF_MPLS_TC[[]],load:0x63->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,CONTROLLER:65535
+ cookie=0xa, n_packets=3, n_bytes=180, dl_src=40:44:44:44:55:52 actions=push_mpls:0x8847,load:0xa->OXM_OF_MPLS_LABEL[[]],load:0x3->OXM_OF_MPLS_TC[[]],load:0x63->OXM_OF_VLAN_VID[[]],mod_vlan_pcp:1,CONTROLLER:65535
+NXST_FLOW reply:
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - fragment handling])
OVS_VSWITCHD_START
ADD_OF_PORTS([br0], [1], [2], [3], [4], [5], [6], [90])
--
1.8.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2.39 6/7] datapath: Break out deacceleration portion of vlan_push
2013-09-09 7:20 [PATCH v2.39 0/7] MPLS actions and matches Simon Horman
` (3 preceding siblings ...)
2013-09-09 7:20 ` [PATCH v2.39 5/7] lib: Push MPLS tags in the OpenFlow 1.3 ordering Simon Horman
@ 2013-09-09 7:20 ` Simon Horman
2013-09-13 22:07 ` Jesse Gross
2013-09-09 7:20 ` [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel Simon Horman
` (2 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Simon Horman @ 2013-09-09 7:20 UTC (permalink / raw)
To: dev, netdev
Cc: Ravi K, Isaku Yamahata, Jesse Gross, Pravin B Shelar, Joe Stringer
Break out deacceleration portion of vlan_push into vlan_put
so that it may be re-used by mpls_push.
For both vlan_push and mpls_push if there is an accelerated VLAN tag
present then it should be deaccelerated, adding it to the data of
the skb, before the new tag is added.
Signed-off-by: Simon Horman <horms@verge.net.au>
---
v2.39
* First post
---
datapath/actions.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/datapath/actions.c b/datapath/actions.c
index 30ea1d2..6741d81 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -105,22 +105,29 @@ static int pop_vlan(struct sk_buff *skb)
return 0;
}
-static int push_vlan(struct sk_buff *skb, const struct ovs_action_push_vlan *vlan)
+/* push down current VLAN tag */
+static struct sk_buff *put_vlan(struct sk_buff *skb)
{
- if (unlikely(vlan_tx_tag_present(skb))) {
- u16 current_tag;
+ u16 current_tag = vlan_tx_tag_get(skb);
- /* push down current VLAN tag */
- current_tag = vlan_tx_tag_get(skb);
+ if (!__vlan_put_tag(skb, skb->vlan_proto, current_tag))
+ return ERR_PTR(-ENOMEM);
- if (!__vlan_put_tag(skb, skb->vlan_proto, current_tag))
- return -ENOMEM;
+ if (skb->ip_summed == CHECKSUM_COMPLETE)
+ skb->csum = csum_add(skb->csum, csum_partial(skb->data
+ + (2 * ETH_ALEN), VLAN_HLEN, 0));
- if (skb->ip_summed == CHECKSUM_COMPLETE)
- skb->csum = csum_add(skb->csum, csum_partial(skb->data
- + (2 * ETH_ALEN), VLAN_HLEN, 0));
+ return skb;
+}
+static int push_vlan(struct sk_buff *skb, const struct ovs_action_push_vlan *vlan)
+{
+ if (unlikely(vlan_tx_tag_present(skb))) {
+ skb = put_vlan(skb);
+ if (IS_ERR(skb))
+ return PTR_ERR(skb);
}
+
__vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
return 0;
}
--
1.8.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-09 7:20 [PATCH v2.39 0/7] MPLS actions and matches Simon Horman
` (4 preceding siblings ...)
2013-09-09 7:20 ` [PATCH v2.39 6/7] datapath: Break out deacceleration portion of vlan_push Simon Horman
@ 2013-09-09 7:20 ` Simon Horman
[not found] ` <1378711207-29890-8-git-send-email-horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
2013-09-17 18:38 ` Pravin Shelar
2013-09-09 7:25 ` [PATCH v2.39 0/7] MPLS actions and matches Simon Horman
2013-09-12 19:06 ` [ovs-dev] " Ben Pfaff
7 siblings, 2 replies; 38+ messages in thread
From: Simon Horman @ 2013-09-09 7:20 UTC (permalink / raw)
To: dev, netdev
Cc: Ravi K, Isaku Yamahata, Jesse Gross, Pravin B Shelar, Joe Stringer
Allow datapath to recognize and extract MPLS labels into flow keys
and execute actions which push, pop, and set labels on packets.
Based heavily on work by Leo Alterman, Ravi K, Isaku Yamahata and Joe Stringer.
Cc: Ravi K <rkerur@gmail.com>
Cc: Leo Alterman <lalterman@nicira.com>
Cc: Isaku Yamahata <yamahata@valinux.co.jp>
Cc: Joe Stringer <joe@wand.net.nz>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
v2.39
* Rebase for removal of vlan, checksum and skb->mark compat code
v2.38
* Rebase for SCTP support
* Refactor validate_tp_port() to iterate over eth_types rather
than open-coding the loop. With the addition of SCTP this logic
is now used three times.
v2.36 - v2.37
* Rebase
* Do not add set_ethertype() to datapath/actions.c.
As this patch has evolved this function had devolved into
to sets of functionality wrapped into a single function with
only one line of common code. Refactor things to simply
open-code setting the ether type in the two locations where
set_ethertype() was previously used. The aim here is to improve
readability.
* Update setting skb->ethertype after mpls push and pop.
- In the case of push_mpls it should be set unconditionally
as in v2.35 the behaviour of this function to always push
an MPLS LSE before any VLAN tags.
- In the case of mpls_pop eth_p_mpls(skb->protocol) is a better
test than skb->protocol != htons(ETH_P_8021Q) as it will give the
correct behaviour in the presence of other VLAN ethernet types,
for example 0x88a8 which is used by 802.1ad. Moreover, it seems
correct to update the ethernet type if it was previously set
according to the top-most MPLS LSE.
* Deaccelerate VLANs when pushing MPLS tags the
- Since v2.35 MPLS push will insert an MPLS LSE before any VLAN tags.
This means that if an accelerated tag is present it should be
deaccelerated to ensure it ends up in the correct position.
* Update skb->mac_len in push_mpls() so that it will be correct
when used by a subsequent call to pop_mpls().
As things stand I do not believe this is strictly necessary as
ovs-vswitchd will not send a pop MPLS action after a push MPLS action.
However, I have added this in order to code more defensively as I believe
that if such a sequence did occur it would be rather unobvious why
it didn't work.
* Do not add skb_cow_head() call in push_mpls().
It is unnecessary as there is a make_writable() call.
This change was also made in v2.30 but some how the
code regressed between then and v2.35.
v2.35
* Rebase
* Move MPLS constants to mpls.h
* Push MPLS tags after ethernet, before VLAN tags
- This is consistent with the OpenFlow 1.3 specification
- Compatibility with OpenFlow 1.2 and earlier versions
may be provided by ovs-vswitchd.
* Correct GSO behaviour in the presence of MPLS but absence of VLANs
v2.34
* Rebase for megaflow changes
v2.33
* Ensure that inner_protocol is always set to to the current
skb->protocol value in ovs_execute_actions(). This ensures
it is set to the correct value in the absence of a push_mpls action.
Also remove setting of inner_protocol in push_mpls() as
it duplicates the code now in ovs_execute_actions().
* Call __skb_gso_segment() instead of skb_gso_segment() from
rpl___skb_gso_segment() in the case that HAVE___SKB_GSO_SEGMENT is set.
This was a typo.
v2.32
* As suggested by Jesse Gross
- Use int instead of size_t in validate_and_copy_actions__().
- Fix crazy edit mess in pop_mpls() action comment
- Move eth_p_mpls() into mpls.h
- Refactor skb_gso_segment MPLS handling into rpl_skb_gso_segment
Address Jesse's comments regarding this code:
"Can we push this completely into the skb_gso_segment() compatibility
code? It's both nicer and may make the interactions with the vlan code
less confusing."
- Move GSO compatibility code into linux/compat/gso.*
- Set skb->protocol on mpls_push and mpls_pop in the presence
of an offloaded VLAN.
v2.31
* As suggested by Jesse Gross
- There is no need to make mac_header_end inline as it is not in a header file
- Remove dubious if (*skb_ethertype == ethertype) optimisation from
set_ethertype
- Only set skb->protocol in push_mpls() or pop_mpls() for non-VLAN packets
- Use MAX_ETH_TYPES instead of SAMPLE_ACTION_DEPTH for array size
of types in struct eth_types. This corrects a typo/thinko.
- Correct eth type tracking logic such that start isn't advanced
when entering a sample action, ensuring that all possibly types
are checked when verifying nested actions.
* Define HAVE_INNER_PROTOCOL based on kernel version.
inner_protocol has been merged into net-next and should appear in
v3.11 so there is no longer a need for a acinclude.m4 test to check for it.
* Add MPLS GSO compatibility code.
This is for use on kernels that do not have MPLS GSO support.
Thanks to Joe Stringer for his work on this.
v2.30
* As suggested by Jesse Gross
- Use skb_cow_head in push_mpls to ensure there is sufficient headroom for
skb_push
- Call make_writable with skb->mac_len instead of skb->mac_len + MPLS_HLEN
in push_mpls as only the first skb->mac_len bytes of existing packet data
are modified.
- Rename skb_mac_header_end as mac_header_end, this seems
to be a more appropriate name for a local function.
- Remove OVS_CSUM_COMPLETE code from set_ethertype().
Inside OVS the ethernet header is not covered by OVS_CSUM_COMPLETE.
- Use __skb_pull() instead of skb_pull() in pop_mpls()
- Decrement and decrement skb->mac_len when poping and pushing VLAN tags.
Previously mac_len was reset, but this would result in forgetting
the MPLS label stack.
- Remove spurious comment from before do_execute_actions().
- Move OVS_KEY_ATTR_MPLS attribute to its final, upstreamable, location.
- Correct ethertype check for OVS_ACTION_ATTR_POP_MPLS case in
validate_and_copy_actions() to check for MPLS ethertypes rather than
ETH_P_IP.
- Rewrite tracking of eth types used to verify actions in the presence
of sample actions. There is a large comment above struct eth_types
describing the new implementation.
v2.29
* Break include/ and lib/ portions of the patch out into a
separate patch "datapath: Add basic MPLS support to kernel"
* Update for new MPLS GSO scheme
- skb->protocol is set to the new ethertype of the packet
on MPLS push and pop
- When pushing the first MPLS LSE onto a previously non-MPLS
packet set skb->inner_protocol to the original ethertype.
- skb->inner_protocol may be used by the network stack
for GSO of the inner-packet.
* Drop const from ethertype parameter of set_ethertype.
This appears to be a legacy of this parameter being a pointer.
* Pass the ethertype patrameter of pop_mpls as a value rather
than a pointer.
v2.28
* Kernel Datapath changes as suggested by Jarno Rajahalme
+ Correct the logic introduced in v2.27 to set the network_header
to after the MPLS label stack in the case of an MPLS packet.
- Increment stack_len offset so that label stacks of depth greater
than two do not cause an infinite loop.
- Correct offset passed to check_header to include skb->mac len
v2.27
* Kernel Datapath changes as suggested by Jarno Rajahalme and Jesse Gross:
+ Previously the mac_len and network_header of an skb corresponded
to the end of the L2 header. To support GSO, just before transmission,
do_output, with the results as follows:
Input: non-MPLS skb: Output: network header and mac_len correspond
to the beginning of the L3 headers
Input: MPLS: Output: network header and mac_len correspond to the
end of the L2 headers.
This is somewhat confusing.
+ The new scheme is as follows:
- The mac_len always corresponds to the end of the L2 header.
- The network header always corresponds to the beginning of the
L3 header.
+ Note that in the case of MPLS output the end of the L2 headers and the
beginning of the L3 headers will differ.
* Remove unused declaration of skb_cb_mpls_stack()
v2.26
* Rebase on master
* Kernel Datapath changes as suggested by Jarno Rajahalme
- Use skb_network_header() instead of skb_mac_header() to locate
the ethertype to set in set_ethertype() as the latter will
be wrong in the presence of VLAN tags. This resolves
a regression introduced in v2.24.
- Enhance comment in do_output()
- do_execute_actions(): Do not alter mpls_stack_depth if
a MPLS push or pop action fail. This is achieved by altering
mpls_stack_depth at the end of push_mpls() and pop_mpls().
v2.25
* Rebase on master
* Pass big-endian value as the last argument of eth_types_set() in
validate_and_copy_actions__()
* Use revised GSO support as provided by the patch series
"[PATCH 0/2] Small Modifications to GSO to allow segmentation of MPLS"
- Set skb->mac_len to the length of the l2 header + MPLS stack length
- Update skb->network_header accordingly
- Set skb->encapsulated_features
v2.24
* Use skb_mac_header() in set_ethertype()
* Set skb->encapsulation in set_ethertype() to support MPLS GSO.
Also add a note about the other requirements for MPLS GSO.
MPLS GSO support will be posted as a patch net-next (Linux mainline)
"MPLS: Add limited GSO support"
* Do not add ETH_TYPE_MIN, it is no longer used
v2.23
* As suggested by Jesse Gross:
- Verify the current ethernet type when validating sample actions
both for the taken and not-taken path if the sample action.
- Document that the OVS_KEY_ATTR_MPLS attribute accepts a list of
struct ovs_key_mpls but that an implementation may restrict
the length it accepts.
- Restrict the array length of the OVS_KEY_ATTR_MPLS to one.
+ Don't add ovs_flow_verify_key_len as it was added to
handle attributes whose values are arrays but there are
no attributes with values that are arrays (of length greater than one).
v2.22
* As suggested by Jesse Gross:
- Fix sparse warning in validate_and_copy_actions()
I have no idea why sparse doesn't show this up this on my system.
- Remove call to skb_cow_head() from push_mpls() as it
is already covered by a call to make_writable()
- Check (key_type > OVS_KEY_ATTR_MAX) in ovs_flow_verify_key_len()
- Disallow set actions on l2.5+ data and MPLS push and pop actions
after an MPLS pop action as there is no verification that the packet
is actually of the new ethernet type. This may later be supported
using recirculation or by other means.
- Do not add spurious debuging message to ovs_flow_cmd_new_or_set()
v2.21
* As suggested by Jesse Gross:
- Verify that l3 and l4 actions always always occur prior to
a push_mpls action and use the network header pointer of an skb
to track the top of the MPLS stack. This avoids adding an l2_size
element to the skb callback.
v2.20
* As suggested by Jesse Gross:
- Do not add ovs_dp_ioctl_hook
+ This appears to be garbage from a rebase
- Do not add skb_cb_set_l2_size. Instead set OVS_CB(skb)->l2_size
in ovs_flow_extract().
- Do not free skb on error in push_mpls(), it is freed in the caller
- Call skb_reset_mac_len() in pop_mpls() and push_mpls()
- Update checksums in pop_mpls(), push_mpls() and set_mpls().
- Rename skb_cb_mpls_bos() as skb_cb_mpls_stack().
It returns the top not the bottom of the stack.
- Track the current eth_type in validate_and_copy_actions
which is initially the eth_type of the flow and may be modified
by push_mpls and pop_mpls actions. Use this to correctly validate
mpls_set actions. This is to allow mpls_set actions to be applied
to a non-MPLS frame after an mpls_push action (although ovs-vswitchd
doesn't currently do that).
Also:
+ Remove the check of the eth_type in set_mpls() as the new validation
scheme should ensure it cannot be incorrect.
+ Use the current eth_type to validate mpls_pop actions and remove
the eth_type check from pop_mpls().
- Move OVS_KEY_ATTR_MPLS to non-upstream group in ovs_key_lens
- Remove unnecessary memset of mpls_key in ovs_flow_to_nlattrs()
- Make a union of the mpls and ip elements of struct sw_flow_key.
Currently the code stops parsing after an MPLS header so it is
not possible for the ip and mpls elements to be used simultaneously
and some space can be saved by using a union.
- Allow an array of MPLS key attributes
+ Currently all but the first element is ignored
+ User-space needs to be updated to accept more than one element,
currently it will treat their presence as an error
- Do not update network header in ovs_flow_extract() for after parsing
the MPLS stack as it is never used because no l3+ processing
occurs on MPLS frames.
- Allow multiple MPLS entries in a match by allowing the OVS_KEY_ATTR_MPLS
to be an array of struct ovs_key_mpls with at least one entry.
Currently only one entry is used which is byte-for-byte compatible with
the previous scheme of having OVS_KEY_ATTR_MPLS as a struct
ovs_key_mpls.
* Make skb writable in pop_mpls(), push_mpls() and set_mpls().
v2.18 - v2.19
* No change
v2.17
* As suggested by Ben Pfaff
- Use consistent terminology for MPLS.
+ Consistently refer to the MPLS component of a packet as the
MPLS label stack and entries in the stack as MPLS label stack entries
(LSE). An MPLS label is a component of an MPLS label stack entry.
The other components are the traffic class (TC), time to live (TTL)
and bottom of stack (BoS) bit.
- Rename compose_.*mpls_ functions as execute_.*mpls_
v2.16
* No change
v2.15
* As suggested by Ben Pfaff
- Use OVS_ACTION_SET to set OVS_KEY_ATTR_MPLS instead of
OVS_ACTION_ATTR_SET_MPLS
v2.14
* Remove include/linux/openvswitch.h portion which added add
new key and action attributes. This
now present in "User-Space MPLS actions and matches"
which is now a dependency of this patch
v2.13
* As suggested by Jarno Rajahalme
- Rename mpls_bos element of ovs_skb_cb as l2_size as it is set and used
regardless of if an MPLS stack is present or not. Update the name of
helper functions and documentation accordingly.
- Ensure that skb_cb_mpls_bos() never returns NULL
* Correct endieness in eth_p_mpls()
v2.12
* Update skb and network header on MPLS extraction in ovs_flow_extract()
* Use NULL in skb_cb_mpls_bos()
* Add eth_p_mpls helper
v2.10 - v2.11
* No change
v2.9
* datapath: Always update the mpls bos if vlan_pop is successful
Regardless of the details of how a successful
vlan_pop is achieved, the mpls bos needs to be updated.
Without this fix it has been observed that the following
results in malformed packets
v2.8
* No change
v2.7
* Rebase
v2.6
* As suggested by Yamahata-san
- Do not guard against label == 0 for
OVS_ACTION_ATTR_SET_MPLS in validate_actions().
A label of 0 is valid
- Remove comment stupulating that if
the top_label element of struct sw_flow_key is 0 then
there is no MPLS label. An MPLS label of 0 is valid
and the correct check if ethertype is
ntohs(ETH_TYPE_MPLS) or ntohs(ETH_TYPE_MPLS_MCAST)
v2.4 - v2.5
* No change
v2.3
* s/mpls_stack/mpls_bos/
This is in keeping with the naming used in the OpenFlow 1.3 specification
v2.2
* Call skb_reset_mac_header() in skb_cb_set_mpls_stack()
eth_hdr(skb) is non-NULL when called in skb_cb_set_mpls_stack().
* Add a call to skb_cb_set_mpls_stack() in ovs_packet_cmd_execute().
I apologise that I have mislaid my notes on this but
it avoids a kernel panic. I can investigate again if necessary.
* Use struct ovs_action_push_mpls instead of
__be16 to decode OVS_ACTION_ATTR_PUSH_MPLS in validate_actions(). This is
consistent with the data format for the attribute.
* Indentation fix in skb_cb_mpls_stack(). [cosmetic]
v2.1
* Manual rebase
---
datapath/Modules.mk | 1 +
datapath/actions.c | 129 +++++++++++-
datapath/datapath.c | 259 +++++++++++++++++++++---
datapath/datapath.h | 9 +
datapath/flow.c | 58 +++++-
datapath/flow.h | 17 +-
datapath/linux/compat/gso.c | 50 ++++-
datapath/linux/compat/gso.h | 39 ++++
datapath/linux/compat/include/linux/netdevice.h | 12 --
datapath/linux/compat/netdevice.c | 28 ---
datapath/mpls.h | 15 ++
datapath/vport-lisp.c | 1 +
datapath/vport-netdev.c | 47 ++++-
include/linux/openvswitch.h | 7 +-
14 files changed, 585 insertions(+), 87 deletions(-)
create mode 100644 datapath/mpls.h
diff --git a/datapath/Modules.mk b/datapath/Modules.mk
index 7ddf79c..b54dc5b 100644
--- a/datapath/Modules.mk
+++ b/datapath/Modules.mk
@@ -22,6 +22,7 @@ openvswitch_headers = \
compat.h \
datapath.h \
flow.h \
+ mpls.h \
vlan.h \
vport.h \
vport-internal_dev.h \
diff --git a/datapath/actions.c b/datapath/actions.c
index 6741d81..2335014 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -35,6 +35,8 @@
#include <net/sctp/checksum.h>
#include "datapath.h"
+#include "gso.h"
+#include "mpls.h"
#include "vlan.h"
#include "vport.h"
@@ -71,7 +73,7 @@ static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
vlan_set_encap_proto(skb, vhdr);
skb->mac_header += VLAN_HLEN;
- skb_reset_mac_len(skb);
+ skb->mac_len -= VLAN_HLEN;
return 0;
}
@@ -113,6 +115,9 @@ static struct sk_buff *put_vlan(struct sk_buff *skb)
if (!__vlan_put_tag(skb, skb->vlan_proto, current_tag))
return ERR_PTR(-ENOMEM);
+ /* update mac_len for mac_header_end() */
+ skb->mac_len += VLAN_HLEN;
+
if (skb->ip_summed == CHECKSUM_COMPLETE)
skb->csum = csum_add(skb->csum, csum_partial(skb->data
+ (2 * ETH_ALEN), VLAN_HLEN, 0));
@@ -132,6 +137,110 @@ static int push_vlan(struct sk_buff *skb, const struct ovs_action_push_vlan *vla
return 0;
}
+/* The end of the mac header.
+ *
+ * For non-MPLS skbs this will correspond to the network header.
+ * For MPLS skbs it will be before the network_header as the MPLS
+ * label stack lies between the end of the mac header and the network
+ * header. That is, for MPLS skbs the end of the mac header
+ * is the top of the MPLS label stack.
+ */
+static unsigned char *mac_header_end(const struct sk_buff *skb)
+{
+ return skb_mac_header(skb) + skb->mac_len;
+}
+
+/* Push MPLS after the ethernet header. */
+static int push_mpls(struct sk_buff *skb,
+ const struct ovs_action_push_mpls *mpls)
+{
+ __be32 *new_mpls_lse;
+ struct ethhdr *hdr;
+ int err;
+
+ if (unlikely(vlan_tx_tag_present(skb))) {
+ skb = put_vlan(skb);
+ if (IS_ERR(skb))
+ return PTR_ERR(skb);
+
+ vlan_set_tci(skb, 0);
+ }
+
+ err = make_writable(skb, skb->mac_len);
+ if (unlikely(err))
+ return err;
+
+ /* update mac_len for subsequent pop_mpls actions. */
+ skb->mac_len += VLAN_HLEN;
+
+ skb_push(skb, MPLS_HLEN);
+ memmove(skb_mac_header(skb) - MPLS_HLEN, skb_mac_header(skb),
+ ETH_HLEN);
+ skb_reset_mac_header(skb);
+
+ new_mpls_lse = (__be32 *)(skb_mac_header(skb) + ETH_HLEN);
+ *new_mpls_lse = mpls->mpls_lse;
+
+ if (skb->ip_summed == CHECKSUM_COMPLETE)
+ skb->csum = csum_add(skb->csum, csum_partial(new_mpls_lse,
+ MPLS_HLEN, 0));
+
+ hdr = (struct ethhdr *)(skb_mac_header(skb));
+ hdr->h_proto = mpls->mpls_ethertype;
+ skb->protocol = mpls->mpls_ethertype;
+ return 0;
+}
+
+static int pop_mpls(struct sk_buff *skb, const __be16 ethertype)
+{
+ struct ethhdr *hdr;
+ int err;
+
+ err = make_writable(skb, skb->mac_len + MPLS_HLEN);
+ if (unlikely(err))
+ return err;
+
+ if (skb->ip_summed == CHECKSUM_COMPLETE)
+ skb->csum = csum_sub(skb->csum,
+ csum_partial(mac_header_end(skb),
+ MPLS_HLEN, 0));
+
+ memmove(skb_mac_header(skb) + MPLS_HLEN, skb_mac_header(skb),
+ skb->mac_len);
+
+ __skb_pull(skb, MPLS_HLEN);
+ skb_reset_mac_header(skb);
+
+ /* mac_header_end() is used to locate the ethertype
+ * field correctly in the presence of VLAN tags.
+ */
+ hdr = (struct ethhdr *)(mac_header_end(skb) - ETH_HLEN);
+ hdr->h_proto = ethertype;
+ if (eth_p_mpls(skb->protocol))
+ skb->protocol = ethertype;
+ return 0;
+}
+
+static int set_mpls(struct sk_buff *skb, const __be32 *mpls_lse)
+{
+ __be32 *stack = (__be32 *)mac_header_end(skb);
+ int err;
+
+ err = make_writable(skb, skb->mac_len + MPLS_HLEN);
+ if (unlikely(err))
+ return err;
+
+ if (skb->ip_summed == CHECKSUM_COMPLETE) {
+ __be32 diff[] = { ~(*stack), *mpls_lse };
+ skb->csum = ~csum_partial((char *)diff, sizeof(diff),
+ ~skb->csum);
+ }
+
+ *stack = *mpls_lse;
+
+ return 0;
+}
+
static int set_eth_addr(struct sk_buff *skb,
const struct ovs_key_ethernet *eth_key)
{
@@ -507,6 +616,9 @@ static int execute_set_action(struct sk_buff *skb,
case OVS_KEY_ATTR_SCTP:
err = set_sctp(skb, nla_data(nested_attr));
+
+ case OVS_KEY_ATTR_MPLS:
+ err = set_mpls(skb, nla_data(nested_attr));
break;
}
@@ -543,6 +655,14 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
output_userspace(dp, skb, a);
break;
+ case OVS_ACTION_ATTR_PUSH_MPLS:
+ err = push_mpls(skb, nla_data(a));
+ break;
+
+ case OVS_ACTION_ATTR_POP_MPLS:
+ err = pop_mpls(skb, nla_get_be16(a));
+ break;
+
case OVS_ACTION_ATTR_PUSH_VLAN:
err = push_vlan(skb, nla_data(a));
if (unlikely(err)) /* skb already freed. */
@@ -616,6 +736,13 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb)
goto out_loop;
}
+ /* Needed to initialise inner protocol on kernels older
+ * than v3.11 where skb->inner_protocol is not present
+ * and compatibility code uses the OVS_CB(skb) to store
+ * the inner protocol.
+ */
+ ovs_skb_set_inner_protocol(skb, skb->protocol);
+
OVS_CB(skb)->tun_key = NULL;
error = do_execute_actions(dp, skb, acts->actions,
acts->actions_len, false);
diff --git a/datapath/datapath.c b/datapath/datapath.c
index bb1e282..e1c6245 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -56,6 +56,8 @@
#include "datapath.h"
#include "flow.h"
+#include "gso.h"
+#include "mpls.h"
#include "vlan.h"
#include "vport-internal_dev.h"
#include "vport-netdev.h"
@@ -545,18 +547,132 @@ static inline void add_nested_action_end(struct sw_flow_actions *sfa, int st_off
a->nla_len = sfa->actions_len - st_offset;
}
-static int validate_and_copy_actions(const struct nlattr *attr,
+#define MAX_ETH_TYPES 16 /* Arbitrary Limit */
+
+/* struct eth_types - possible eth types
+ * @types: provides storage for the possible eth types.
+ * @start: is the index of the first entry of types which is possible.
+ * @end: is the index of the last entry of types which is possible.
+ * @cursor: is the index of the entry which should be updated if an action
+ * changes the eth type.
+ *
+ * Due to the sample action there may be multiple possible eth types.
+ * In order to correctly validate actions all possible types are tracked
+ * and verified. This is done using struct eth_types.
+ *
+ * Initially start, end and cursor should be 0, and the first element of
+ * types should be set to the eth type of the flow.
+ *
+ * When an action changes the eth type then the values of start and end are
+ * updated to the value of cursor. The new type is stored at types[cursor].
+ *
+ * When entering a sample action the start and cursor values are saved. The
+ * value of cursor is set to the value of end plus one.
+ *
+ * When leaving a sample action the start and cursor values are restored to
+ * their saved values.
+ *
+ * An example follows.
+ *
+ * actions: pop_mpls(A),sample(pop_mpls(B)),sample(pop_mpls(C)),pop_mpls(D)
+ *
+ * 0. Initial state:
+ * types = { original_eth_type }
+ * start = end = cursor = 0;
+ *
+ * 1. pop_mpls(A)
+ * a. Check types from start (0) to end (0) inclusive
+ * i.e. Check against original_eth_type
+ * b. Set start = end = cursor
+ * c. Set types[cursor] = A
+ * New state:
+ * types = { A }
+ * start = end = cursor = 0;
+ *
+ * 2. Enter first sample()
+ * a. Save start and cursor
+ * b. Set cursor = end + 1
+ * New state:
+ * types = { A }
+ * start = end = 0;
+ * cursor = 1;
+ *
+ * 3. pop_mpls(B)
+ * a. Check types from start (0) to end (0)
+ * i.e: Check against A
+ * b. Set start = end = cursor
+ * c. Set types[cursor] = B
+ * New state:
+ * types = { A, B }
+ * start = end = cursor = 1;
+ *
+ * 4. Leave first sample()
+ * a. Restore start and cursor to the values when entering 2.
+ * New state:
+ * types = { A, B }
+ * start = cursor = 0;
+ * end = 1;
+ *
+ * 5. Enter second sample()
+ * a. Save start and cursor
+ * b. Set cursor = end + 1
+ * New state:
+ * types = { A, B }
+ * start = 0;
+ * end = 1;
+ * cursor = 2;
+ *
+ * 6. pop_mpls(C)
+ * a. Check types from start (0) to end (1) inclusive
+ * i.e: Check against A and B
+ * b. Set start = end = cursor
+ * c. Set types[cursor] = C
+ * New state:
+ * types = { A, B, C }
+ * start = end = cursor = 2;
+ *
+ * 7. Leave second sample()
+ * a. Restore start and cursor to the values when entering 5.
+ * New state:
+ * types = { A, B, C }
+ * start = cursor = 0;
+ * end = 2;
+ *
+ * 8. pop_mpls(D)
+ * a. Check types from start (0) to end (2) inclusive
+ * i.e: Check against A, B and C
+ * b. Set start = end = cursor
+ * c. Set types[cursor] = D
+ * New state:
+ * types = { D } // Trailing entries of type are no longer used end = 0
+ * start = end = cursor = 0;
+ */
+struct eth_types {
+ int start, end, cursor;
+ __be16 types[MAX_ETH_TYPES];
+};
+
+static void eth_types_set(struct eth_types *types, __be16 type)
+{
+ types->start = types->end = types->cursor;
+ types->types[types->cursor] = type;
+}
+
+static int validate_and_copy_actions__(const struct nlattr *attr,
const struct sw_flow_key *key, int depth,
- struct sw_flow_actions **sfa);
+ struct sw_flow_actions **sfa,
+ struct eth_types *eth_types);
static int validate_and_copy_sample(const struct nlattr *attr,
const struct sw_flow_key *key, int depth,
- struct sw_flow_actions **sfa)
+ struct sw_flow_actions **sfa,
+ struct eth_types *eth_types)
{
const struct nlattr *attrs[OVS_SAMPLE_ATTR_MAX + 1];
const struct nlattr *probability, *actions;
const struct nlattr *a;
int rem, start, err, st_acts;
+ int saved_eth_types_start, saved_eth_types_cursor;
memset(attrs, 0, sizeof(attrs));
nla_for_each_nested(a, attr, rem) {
@@ -587,22 +703,39 @@ static int validate_and_copy_sample(const struct nlattr *attr,
if (st_acts < 0)
return st_acts;
- err = validate_and_copy_actions(actions, key, depth + 1, sfa);
+ /* Save and update eth_types cursor and start. Please see the
+ * comment for struct eth_types for a discussion of this.
+ */
+ saved_eth_types_start = eth_types->start;
+ saved_eth_types_cursor = eth_types->cursor;
+ eth_types->cursor = eth_types->end + 1;
+ if (eth_types->cursor == MAX_ETH_TYPES)
+ return -EINVAL;
+
+ err = validate_and_copy_actions__(actions, key, depth + 1, sfa,
+ eth_types);
if (err)
return err;
+ /* Restore eth_types cursor and start. Please see the
+ * comment for struct eth_types for a discussion of this.
+ */
+ eth_types->cursor = saved_eth_types_cursor;
+ eth_types->start = saved_eth_types_start;
+
add_nested_action_end(*sfa, st_acts);
add_nested_action_end(*sfa, start);
return 0;
}
-static int validate_tp_port(const struct sw_flow_key *flow_key)
+static int validate_tp_port__(const struct sw_flow_key *flow_key,
+ __be16 eth_type)
{
- if (flow_key->eth.type == htons(ETH_P_IP)) {
+ if (eth_type == htons(ETH_P_IP)) {
if (flow_key->ipv4.tp.src || flow_key->ipv4.tp.dst)
return 0;
- } else if (flow_key->eth.type == htons(ETH_P_IPV6)) {
+ } else if (eth_type == htons(ETH_P_IPV6)) {
if (flow_key->ipv6.tp.src || flow_key->ipv6.tp.dst)
return 0;
}
@@ -610,6 +743,21 @@ static int validate_tp_port(const struct sw_flow_key *flow_key)
return -EINVAL;
}
+static int validate_tp_port(const struct sw_flow_key *flow_key,
+ const struct eth_types *eth_types)
+{
+ int i;
+
+ for (i = eth_types->start; i < eth_types->end; i++) {
+ int ret = validate_tp_port__(flow_key, eth_types->types[i]);
+
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
static int validate_and_copy_set_tun(const struct nlattr *attr,
struct sw_flow_actions **sfa)
{
@@ -636,7 +784,7 @@ static int validate_and_copy_set_tun(const struct nlattr *attr,
static int validate_set(const struct nlattr *a,
const struct sw_flow_key *flow_key,
struct sw_flow_actions **sfa,
- bool *set_tun)
+ bool *set_tun, struct eth_types *eth_types)
{
const struct nlattr *ovs_key = nla_data(a);
int key_type = nla_type(ovs_key);
@@ -667,9 +815,12 @@ static int validate_set(const struct nlattr *a,
return err;
break;
- case OVS_KEY_ATTR_IPV4:
- if (flow_key->eth.type != htons(ETH_P_IP))
- return -EINVAL;
+ case OVS_KEY_ATTR_IPV4: {
+ int i;
+
+ for (i = eth_types->start; i <= eth_types->end; i++)
+ if (eth_types->types[i] != htons(ETH_P_IP))
+ return -EINVAL;
if (!flow_key->ip.proto)
return -EINVAL;
@@ -682,10 +833,14 @@ static int validate_set(const struct nlattr *a,
return -EINVAL;
break;
+ }
- case OVS_KEY_ATTR_IPV6:
- if (flow_key->eth.type != htons(ETH_P_IPV6))
- return -EINVAL;
+ case OVS_KEY_ATTR_IPV6: {
+ int i;
+
+ for (i = eth_types->start; i <= eth_types->end; i++)
+ if (eth_types->types[i] != htons(ETH_P_IPV6))
+ return -EINVAL;
if (!flow_key->ip.proto)
return -EINVAL;
@@ -701,24 +856,34 @@ static int validate_set(const struct nlattr *a,
return -EINVAL;
break;
+ }
case OVS_KEY_ATTR_TCP:
if (flow_key->ip.proto != IPPROTO_TCP)
return -EINVAL;
- return validate_tp_port(flow_key);
+ return validate_tp_port(flow_key, eth_types);
case OVS_KEY_ATTR_UDP:
if (flow_key->ip.proto != IPPROTO_UDP)
return -EINVAL;
- return validate_tp_port(flow_key);
+ return validate_tp_port(flow_key, eth_types);
+
+ case OVS_KEY_ATTR_MPLS: {
+ int i;
+
+ for (i = eth_types->start; i < eth_types->end; i++)
+ if (!eth_p_mpls(eth_types->types[i]))
+ return -EINVAL;
+ break;
+ }
case OVS_KEY_ATTR_SCTP:
if (flow_key->ip.proto != IPPROTO_SCTP)
return -EINVAL;
- return validate_tp_port(flow_key);
+ return validate_tp_port(flow_key, eth_types);
default:
return -EINVAL;
@@ -762,10 +927,10 @@ static int copy_action(const struct nlattr *from,
return 0;
}
-static int validate_and_copy_actions(const struct nlattr *attr,
- const struct sw_flow_key *key,
- int depth,
- struct sw_flow_actions **sfa)
+static int validate_and_copy_actions__(const struct nlattr *attr,
+ const struct sw_flow_key *key, int depth,
+ struct sw_flow_actions **sfa,
+ struct eth_types *eth_types)
{
const struct nlattr *a;
int rem, err;
@@ -778,6 +943,8 @@ static int validate_and_copy_actions(const struct nlattr *attr,
static const u32 action_lens[OVS_ACTION_ATTR_MAX + 1] = {
[OVS_ACTION_ATTR_OUTPUT] = sizeof(u32),
[OVS_ACTION_ATTR_USERSPACE] = (u32)-1,
+ [OVS_ACTION_ATTR_PUSH_MPLS] = sizeof(struct ovs_action_push_mpls),
+ [OVS_ACTION_ATTR_POP_MPLS] = sizeof(__be16),
[OVS_ACTION_ATTR_PUSH_VLAN] = sizeof(struct ovs_action_push_vlan),
[OVS_ACTION_ATTR_POP_VLAN] = 0,
[OVS_ACTION_ATTR_SET] = (u32)-1,
@@ -808,6 +975,33 @@ static int validate_and_copy_actions(const struct nlattr *attr,
return -EINVAL;
break;
+ case OVS_ACTION_ATTR_PUSH_MPLS: {
+ const struct ovs_action_push_mpls *mpls = nla_data(a);
+ if (!eth_p_mpls(mpls->mpls_ethertype))
+ return -EINVAL;
+ eth_types_set(eth_types, mpls->mpls_ethertype);
+ break;
+ }
+
+ case OVS_ACTION_ATTR_POP_MPLS: {
+ int i;
+
+ for (i = eth_types->start; i <= eth_types->end; i++)
+ if (!eth_p_mpls(eth_types->types[i]))
+ return -EINVAL;
+
+ /* Disallow subsequent L2.5+ set and mpls_pop actions
+ * as there is no check here to ensure that the new
+ * eth_type is valid and thus set actions could
+ * write off the end of the packet or otherwise
+ * corrupt it.
+ *
+ * Support for these actions is planned using packet
+ * recirculation.
+ */
+ eth_types_set(eth_types, htons(0));
+ break;
+ }
case OVS_ACTION_ATTR_POP_VLAN:
break;
@@ -821,13 +1015,14 @@ static int validate_and_copy_actions(const struct nlattr *attr,
break;
case OVS_ACTION_ATTR_SET:
- err = validate_set(a, key, sfa, &skip_copy);
+ err = validate_set(a, key, sfa, &skip_copy, eth_types);
if (err)
return err;
break;
case OVS_ACTION_ATTR_SAMPLE:
- err = validate_and_copy_sample(a, key, depth, sfa);
+ err = validate_and_copy_sample(a, key, depth, sfa,
+ eth_types);
if (err)
return err;
skip_copy = true;
@@ -849,6 +1044,20 @@ static int validate_and_copy_actions(const struct nlattr *attr,
return 0;
}
+static int validate_and_copy_actions(const struct nlattr *attr,
+ const struct sw_flow_key *key,
+ struct sw_flow_actions **sfa)
+{
+ struct eth_types eth_type = {
+ .start = 0,
+ .end = 0,
+ .cursor = 0,
+ .types = { key->eth.type, },
+ };
+
+ return validate_and_copy_actions__(attr, key, 0, sfa, ð_type);
+}
+
static void clear_stats(struct sw_flow *flow)
{
flow->used = 0;
@@ -912,7 +1121,7 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
if (IS_ERR(acts))
goto err_flow_free;
- err = validate_and_copy_actions(a[OVS_PACKET_ATTR_ACTIONS], &flow->key, 0, &acts);
+ err = validate_and_copy_actions(a[OVS_PACKET_ATTR_ACTIONS], &flow->key, &acts);
rcu_assign_pointer(flow->sf_acts, acts);
if (err)
goto err_flow_free;
@@ -1270,7 +1479,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
ovs_flow_key_mask(&masked_key, &key, &mask);
error = validate_and_copy_actions(a[OVS_FLOW_ATTR_ACTIONS],
- &masked_key, 0, &acts);
+ &masked_key, &acts);
if (error) {
OVS_NLERR("Flow actions may not be safe on all matching packets.\n");
goto err_kfree;
diff --git a/datapath/datapath.h b/datapath/datapath.h
index 5d50dd4..babae3b 100644
--- a/datapath/datapath.h
+++ b/datapath/datapath.h
@@ -36,6 +36,10 @@
#define SAMPLE_ACTION_DEPTH 3
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
+#define HAVE_INNER_PROTOCOL
+#endif
+
/**
* struct dp_stats_percpu - per-cpu packet processing statistics for a given
* datapath.
@@ -93,11 +97,16 @@ struct datapath {
* @pkt_key: The flow information extracted from the packet. Must be nonnull.
* @tun_key: Key for the tunnel that encapsulated this packet. NULL if the
* packet is not being tunneled.
+ * @inner_protocol: Provides a substitute for the skb->inner_protocol field on
+ * kernels before 3.11.
*/
struct ovs_skb_cb {
struct sw_flow *flow;
struct sw_flow_key *pkt_key;
struct ovs_key_ipv4_tunnel *tun_key;
+#ifndef HAVE_INNER_PROTOCOL
+ __be16 inner_protocol;
+#endif
};
#define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
diff --git a/datapath/flow.c b/datapath/flow.c
index 449e645..7b27720 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -44,6 +44,7 @@
#include <net/ipv6.h>
#include <net/ndisc.h>
+#include "mpls.h"
#include "vlan.h"
static struct kmem_cache *flow_cache;
@@ -140,7 +141,8 @@ static bool ovs_match_validate(const struct sw_flow_match *match,
| (1ULL << OVS_KEY_ATTR_ICMP)
| (1ULL << OVS_KEY_ATTR_ICMPV6)
| (1ULL << OVS_KEY_ATTR_ARP)
- | (1ULL << OVS_KEY_ATTR_ND));
+ | (1ULL << OVS_KEY_ATTR_ND)
+ | (1ULL << OVS_KEY_ATTR_MPLS));
/* Always allowed mask fields. */
mask_allowed |= ((1ULL << OVS_KEY_ATTR_TUNNEL)
@@ -155,6 +157,12 @@ static bool ovs_match_validate(const struct sw_flow_match *match,
mask_allowed |= 1ULL << OVS_KEY_ATTR_ARP;
}
+ if (eth_p_mpls(match->key->eth.type)) {
+ key_expected |= 1ULL << OVS_KEY_ATTR_MPLS;
+ if (match->mask && (match->mask->key.eth.type == htons(0xffff)))
+ mask_allowed |= 1ULL << OVS_KEY_ATTR_MPLS;
+ }
+
if (match->key->eth.type == htons(ETH_P_IP)) {
key_expected |= 1ULL << OVS_KEY_ATTR_IPV4;
if (match->mask && (match->mask->key.eth.type == htons(0xffff)))
@@ -879,6 +887,7 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key)
return -ENOMEM;
skb_reset_network_header(skb);
+ skb_reset_mac_len(skb);
__skb_push(skb, skb->data - skb_mac_header(skb));
/* Network layer. */
@@ -961,6 +970,33 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key)
memcpy(key->ipv4.arp.sha, arp->ar_sha, ETH_ALEN);
memcpy(key->ipv4.arp.tha, arp->ar_tha, ETH_ALEN);
}
+ } else if (eth_p_mpls(key->eth.type)) {
+ size_t stack_len = MPLS_HLEN;
+
+ /* In the presence of an MPLS label stack the end of the L2
+ * header and the beginning of the L3 header differ.
+ *
+ * Advance network_header to the beginning of the L3
+ * header. mac_len corresponds to the end of the L2 header.
+ */
+ while (1) {
+ __be32 lse;
+
+ error = check_header(skb, skb->mac_len + stack_len);
+ if (unlikely(error))
+ return 0;
+
+ memcpy(&lse, skb_network_header(skb), MPLS_HLEN);
+
+ if (stack_len == MPLS_HLEN)
+ memcpy(&key->mpls.top_lse, &lse, MPLS_HLEN);
+
+ skb_set_network_header(skb, skb->mac_len + stack_len);
+ if (lse & htonl(MPLS_BOS_MASK))
+ break;
+
+ stack_len += MPLS_HLEN;
+ }
} else if (key->eth.type == htons(ETH_P_IPV6)) {
int nh_len; /* IPv6 Header + Extensions */
@@ -1154,6 +1190,7 @@ const int ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
[OVS_KEY_ATTR_ARP] = sizeof(struct ovs_key_arp),
[OVS_KEY_ATTR_ND] = sizeof(struct ovs_key_nd),
[OVS_KEY_ATTR_TUNNEL] = -1,
+ [OVS_KEY_ATTR_MPLS] = sizeof(struct ovs_key_mpls),
};
static bool is_all_zero(const u8 *fp, size_t size)
@@ -1527,6 +1564,17 @@ static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
attrs &= ~(1ULL << OVS_KEY_ATTR_ARP);
}
+
+ if (attrs & (1ULL << OVS_KEY_ATTR_MPLS)) {
+ const struct ovs_key_mpls *mpls_key;
+
+ mpls_key = nla_data(a[OVS_KEY_ATTR_MPLS]);
+ SW_FLOW_KEY_PUT(match, mpls.top_lse,
+ mpls_key->mpls_lse, is_mask);
+
+ attrs &= ~(1ULL << OVS_KEY_ATTR_MPLS);
+ }
+
if (attrs & (1ULL << OVS_KEY_ATTR_TCP)) {
const struct ovs_key_tcp *tcp_key;
@@ -1890,6 +1938,14 @@ int ovs_flow_to_nlattrs(const struct sw_flow_key *swkey,
arp_key->arp_op = htons(output->ip.proto);
memcpy(arp_key->arp_sha, output->ipv4.arp.sha, ETH_ALEN);
memcpy(arp_key->arp_tha, output->ipv4.arp.tha, ETH_ALEN);
+ } else if (eth_p_mpls(swkey->eth.type)) {
+ struct ovs_key_mpls *mpls_key;
+
+ nla = nla_reserve(skb, OVS_KEY_ATTR_MPLS, sizeof(*mpls_key));
+ if (!nla)
+ goto nla_put_failure;
+ mpls_key = nla_data(nla);
+ mpls_key->mpls_lse = output->mpls.top_lse;
}
if ((swkey->eth.type == htons(ETH_P_IP) ||
diff --git a/datapath/flow.h b/datapath/flow.h
index 03eae03..9376802 100644
--- a/datapath/flow.h
+++ b/datapath/flow.h
@@ -87,12 +87,17 @@ struct sw_flow_key {
__be16 tci; /* 0 if no VLAN, VLAN_TAG_PRESENT set otherwise. */
__be16 type; /* Ethernet frame type. */
} eth;
- struct {
- u8 proto; /* IP protocol or lower 8 bits of ARP opcode. */
- u8 tos; /* IP ToS. */
- u8 ttl; /* IP TTL/hop limit. */
- u8 frag; /* One of OVS_FRAG_TYPE_*. */
- } ip;
+ union {
+ struct {
+ __be32 top_lse; /* top label stack entry */
+ } mpls;
+ struct {
+ u8 proto; /* IP protocol or lower 8 bits of ARP opcode. */
+ u8 tos; /* IP ToS. */
+ u8 ttl; /* IP TTL/hop limit. */
+ u8 frag; /* One of OVS_FRAG_TYPE_*. */
+ } ip;
+ };
union {
struct {
struct {
diff --git a/datapath/linux/compat/gso.c b/datapath/linux/compat/gso.c
index 30332a2..8e0e712 100644
--- a/datapath/linux/compat/gso.c
+++ b/datapath/linux/compat/gso.c
@@ -19,6 +19,7 @@
#include <linux/module.h>
#include <linux/if.h>
#include <linux/if_tunnel.h>
+#include <linux/if_vlan.h>
#include <linux/icmp.h>
#include <linux/in.h>
#include <linux/ip.h>
@@ -35,12 +36,20 @@
#include <net/xfrm.h>
#include "gso.h"
+#include "mpls.h"
+#include "vlan.h"
-static __be16 __skb_network_protocol(struct sk_buff *skb)
+#if LINUX_VERSION_CODE < KERNEL_VERSION(3,11,0)
+__be16 rpl_skb_network_protocol(struct sk_buff *skb)
{
__be16 type = skb->protocol;
+ __be16 inner_proto;
int vlan_depth = ETH_HLEN;
+ inner_proto = ovs_skb_get_inner_protocol(skb);
+ if (eth_p_mpls(skb->protocol) && !eth_p_mpls(inner_proto))
+ type = inner_proto;
+
while (type == htons(ETH_P_8021Q) || type == htons(ETH_P_8021AD)) {
struct vlan_hdr *vh;
@@ -55,6 +64,43 @@ static __be16 __skb_network_protocol(struct sk_buff *skb)
return type;
}
+struct sk_buff *rpl___skb_gso_segment(struct sk_buff *skb,
+ netdev_features_t features,
+ bool tx_path)
+{
+ struct sk_buff *skb_gso;
+ __be16 type = skb->protocol;
+
+ skb->protocol = skb_network_protocol(skb);
+
+ /* this hack needed to get regular skb_gso_segment() */
+#ifdef HAVE___SKB_GSO_SEGMENT
+#undef __skb_gso_segment
+ skb_gso = __skb_gso_segment(skb, features, tx_path);
+#else
+#undef skb_gso_segment
+ skb_gso = skb_gso_segment(skb, features);
+#endif
+
+ if (!skb_gso || IS_ERR(skb_gso))
+ return skb_gso;
+
+ skb = skb_gso;
+ while (skb) {
+ skb->protocol = type;
+ skb = skb->next;
+ }
+
+ return skb_gso;
+}
+
+struct sk_buff *rpl_skb_gso_segment(struct sk_buff *skb,
+ netdev_features_t features)
+{
+ return rpl___skb_gso_segment(skb, features, true);
+}
+#endif /* kernel version < 3.11.0 */
+
static struct sk_buff *tnl_skb_gso_segment(struct sk_buff *skb,
netdev_features_t features,
bool tx_path)
@@ -69,7 +115,7 @@ static struct sk_buff *tnl_skb_gso_segment(struct sk_buff *skb,
/* setup whole inner packet to get protocol. */
__skb_pull(skb, mac_offset);
- skb->protocol = __skb_network_protocol(skb);
+ skb->protocol = skb_network_protocol(skb);
/* setup l3 packet to gso, to get around segmentation bug on older kernel.*/
__skb_pull(skb, (pkt_hlen - mac_offset));
diff --git a/datapath/linux/compat/gso.h b/datapath/linux/compat/gso.h
index 44fd213..49ef8e6 100644
--- a/datapath/linux/compat/gso.h
+++ b/datapath/linux/compat/gso.h
@@ -1,6 +1,7 @@
#ifndef __LINUX_GSO_WRAPPER_H
#define __LINUX_GSO_WRAPPER_H
+#include <linux/netdevice.h>
#include <linux/skbuff.h>
#include <net/protocol.h>
@@ -69,4 +70,42 @@ static inline void skb_reset_inner_headers(struct sk_buff *skb)
#define ip_local_out rpl_ip_local_out
int ip_local_out(struct sk_buff *skb);
+
+#ifdef HAVE_INNER_PROTOCOL
+static inline void ovs_skb_set_inner_protocol(struct sk_buff *skb,
+ __be16 ethertype)
+{
+ skb->inner_protocol = ethertype;
+}
+
+static inline __be16 ovs_skb_get_inner_protocol(struct sk_buff *skb)
+{
+ return skb->inner_protocol;
+}
+#else
+static inline void ovs_skb_set_inner_protocol(struct sk_buff *skb,
+ __be16 ethertype) {
+ OVS_CB(skb)->inner_protocol = ethertype;
+}
+
+static inline __be16 ovs_skb_get_inner_protocol(struct sk_buff *skb)
+{
+ return OVS_CB(skb)->inner_protocol;
+}
+#endif
+
+#if LINUX_VERSION_CODE < KERNEL_VERSION(3,11,0)
+#define skb_network_protocol rpl_skb_network_protocol
+__be16 rpl_skb_network_protocol(struct sk_buff *skb);
+
+#define skb_gso_segment rpl_skb_gso_segment
+struct sk_buff *rpl_skb_gso_segment(struct sk_buff *skb,
+ netdev_features_t features);
+
+#define __skb_gso_segment rpl___skb_gso_segment
+struct sk_buff *rpl___skb_gso_segment(struct sk_buff *skb,
+ netdev_features_t features,
+ bool tx_path);
+#endif /* before 3.11 */
+
#endif
diff --git a/datapath/linux/compat/include/linux/netdevice.h b/datapath/linux/compat/include/linux/netdevice.h
index 4e2b7f5..47b0232 100644
--- a/datapath/linux/compat/include/linux/netdevice.h
+++ b/datapath/linux/compat/include/linux/netdevice.h
@@ -74,9 +74,6 @@ static inline struct net_device *dev_get_by_index_rcu(struct net *net, int ifind
#endif
#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,38)
-#define skb_gso_segment rpl_skb_gso_segment
-struct sk_buff *rpl_skb_gso_segment(struct sk_buff *skb, u32 features);
-
#define netif_skb_features rpl_netif_skb_features
u32 rpl_netif_skb_features(struct sk_buff *skb);
@@ -92,15 +89,6 @@ static inline int rpl_netif_needs_gso(struct sk_buff *skb, int features)
typedef u32 netdev_features_t;
#endif
-#ifndef HAVE___SKB_GSO_SEGMENT
-static inline struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
- netdev_features_t features,
- bool tx_path)
-{
- return skb_gso_segment(skb, features);
-}
-#endif
-
#if LINUX_VERSION_CODE < KERNEL_VERSION(3,9,0)
/* XEN dom0 networking assumes dev->master is bond device
diff --git a/datapath/linux/compat/netdevice.c b/datapath/linux/compat/netdevice.c
index 248066d..5f190b9 100644
--- a/datapath/linux/compat/netdevice.c
+++ b/datapath/linux/compat/netdevice.c
@@ -71,32 +71,4 @@ u32 rpl_netif_skb_features(struct sk_buff *skb)
return harmonize_features(skb, protocol, features);
}
}
-
-struct sk_buff *rpl_skb_gso_segment(struct sk_buff *skb, u32 features)
-{
- int vlan_depth = ETH_HLEN;
- __be16 type = skb->protocol;
- __be16 skb_proto;
- struct sk_buff *skb_gso;
-
- while (type == htons(ETH_P_8021Q)) {
- struct vlan_hdr *vh;
-
- if (unlikely(!pskb_may_pull(skb, vlan_depth + VLAN_HLEN)))
- return ERR_PTR(-EINVAL);
-
- vh = (struct vlan_hdr *)(skb->data + vlan_depth);
- type = vh->h_vlan_encapsulated_proto;
- vlan_depth += VLAN_HLEN;
- }
-
- /* this hack needed to get regular skb_gso_segment() */
-#undef skb_gso_segment
- skb_proto = skb->protocol;
- skb->protocol = type;
-
- skb_gso = skb_gso_segment(skb, features);
- skb->protocol = skb_proto;
- return skb_gso;
-}
#endif /* kernel version < 2.6.38 */
diff --git a/datapath/mpls.h b/datapath/mpls.h
new file mode 100644
index 0000000..7eab104
--- /dev/null
+++ b/datapath/mpls.h
@@ -0,0 +1,15 @@
+#ifndef MPLS_H
+#define MPLS_H 1
+
+#include <linux/if_ether.h>
+
+#define MPLS_BOS_MASK 0x00000100
+#define MPLS_HLEN 4
+
+static inline bool eth_p_mpls(__be16 eth_type)
+{
+ return eth_type == htons(ETH_P_MPLS_UC) ||
+ eth_type == htons(ETH_P_MPLS_MC);
+}
+
+#endif
diff --git a/datapath/vport-lisp.c b/datapath/vport-lisp.c
index 259cc2b..26e4385 100644
--- a/datapath/vport-lisp.c
+++ b/datapath/vport-lisp.c
@@ -34,6 +34,7 @@
#include <net/xfrm.h>
#include "datapath.h"
+#include "gso.h"
#include "vport.h"
/*
diff --git a/datapath/vport-netdev.c b/datapath/vport-netdev.c
index 215a47e..69f24d1 100644
--- a/datapath/vport-netdev.c
+++ b/datapath/vport-netdev.c
@@ -30,6 +30,8 @@
#include <net/llc.h>
#include "datapath.h"
+#include "gso.h"
+#include "mpls.h"
#include "vlan.h"
#include "vport-internal_dev.h"
#include "vport-netdev.h"
@@ -277,6 +279,8 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
struct netdev_vport *netdev_vport = netdev_vport_priv(vport);
int mtu = netdev_vport->dev->mtu;
int len;
+ __be16 inner_protocol;
+ bool vlan, mpls;
if (unlikely(packet_length(skb) > mtu && !skb_is_gso(skb))) {
net_warn_ratelimited("%s: dropped over-mtu packet: %d > %d\n",
@@ -287,8 +291,17 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
skb->dev = netdev_vport->dev;
- if (vlan_tx_tag_present(skb) && !dev_supports_vlan_tx(skb->dev)) {
- int features;
+ vlan = mpls = false;
+
+ inner_protocol = ovs_skb_get_inner_protocol(skb);
+ if (eth_p_mpls(skb->protocol) && !eth_p_mpls(inner_protocol))
+ mpls = true;
+
+ if (vlan_tx_tag_present(skb) && !dev_supports_vlan_tx(skb->dev))
+ vlan = true;
+
+ if (vlan || mpls) {
+ netdev_features_t features;
features = netif_skb_features(skb);
@@ -296,6 +309,20 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
features &= ~(NETIF_F_TSO | NETIF_F_TSO6 |
NETIF_F_UFO | NETIF_F_FSO);
+ /* As of v3.11 the kernel provides an mpls_features field in
+ * struct net_device which allows devices to advertise which
+ * features its supports for MPLS. This value defaults to
+ * NETIF_F_SG and as of v3.11.
+ *
+ * This compatibility code is intended for kernels older
+ * than v3.11 that do not support MPLS GSO and thus do not
+ * provide mpls_features. Thus this code uses NETIF_F_SG
+ * directly in place of mpls_features.
+ */
+
+ if (mpls)
+ features &= NETIF_F_SG;
+
if (netif_needs_gso(skb, features)) {
struct sk_buff *nskb;
@@ -319,10 +346,12 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
nskb = skb->next;
skb->next = NULL;
- skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
+ if (vlan)
+ skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
if (likely(skb)) {
len += skb->len;
- vlan_set_tci(skb, 0);
+ if (vlan)
+ vlan_set_tci(skb, 0);
dev_queue_xmit(skb);
}
@@ -333,10 +362,12 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
}
tag:
- skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
- if (unlikely(!skb))
- return 0;
- vlan_set_tci(skb, 0);
+ if (vlan) {
+ skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
+ if (unlikely(!skb))
+ return 0;
+ vlan_set_tci(skb, 0);
+ }
}
len = skb->len;
diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index 09c26b5..1ef98a8 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -283,14 +283,13 @@ enum ovs_key_attr {
OVS_KEY_ATTR_SKB_MARK, /* u32 skb mark */
OVS_KEY_ATTR_TUNNEL, /* Nested set of ovs_tunnel attributes */
OVS_KEY_ATTR_SCTP, /* struct ovs_key_sctp */
+ OVS_KEY_ATTR_MPLS, /* array of struct ovs_key_mpls.
+ * The implementation may restrict
+ * the accepted length of the array. */
#ifdef __KERNEL__
OVS_KEY_ATTR_IPV4_TUNNEL, /* struct ovs_key_ipv4_tunnel */
#endif
-
- OVS_KEY_ATTR_MPLS = 62, /* array of struct ovs_key_mpls.
- * The implementation may restrict
- * the accepted length of the array. */
__OVS_KEY_ATTR_MAX
};
--
1.8.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 0/7] MPLS actions and matches
2013-09-09 7:20 [PATCH v2.39 0/7] MPLS actions and matches Simon Horman
` (5 preceding siblings ...)
2013-09-09 7:20 ` [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel Simon Horman
@ 2013-09-09 7:25 ` Simon Horman
2013-09-12 19:06 ` [ovs-dev] " Ben Pfaff
7 siblings, 0 replies; 38+ messages in thread
From: Simon Horman @ 2013-09-09 7:25 UTC (permalink / raw)
To: dev, netdev
Cc: Ravi K, Isaku Yamahata, Jesse Gross, Pravin B Shelar, Joe Stringer
On Mon, Sep 09, 2013 at 04:20:00PM +0900, Simon Horman wrote:
> Hi,
>
> This series implements MPLS actions and matches based on work by
> Ravi K, Leo Alterman, Yamahata-san and Joe Stringer.
>
> This series provides two changes
>
> * Provide user-space support for the VLAN/MPLS tag insertion order
> up to and including OpenFlow 1.2, and the different ordering
> specified from OpenFlow 1.3. In a nutshell the datapath always
> uses the OpenFlow 1.3 ordering, which is to always insert tags
> immediately after the L2 header, regardless of the presence of other
> tags. And ovs-vswtichd provides compatibility for the behaviour up
> to OpenFlow 1.2, which is that MPLS tags should follow VLAN tags
> if present.
>
> * Adding basic MPLS action and match support to the kernel datapath
>
>
> Differences between v2.39 and v2.38:
>
> * Rebase for removal of vlan, checksum and skb->mark compat code
> - This includes adding adding a new patch,
> "[PATCH v2.39 6/7] datapath: Break out deacceleration portion of
> vlan_push" to allow re-use of some existing code.
>
>
> Differences between v2.38 and v2.37:
>
> * Rebase for SCTP support
> * Refactor validate_tp_port() to iterate over eth_types rather
> than open-coding the loop. With the addition of SCTP this logic
> is now used three times.
>
>
> Differences between v2.37 and v2.36:
>
> * Rebase
>
>
> Differences between v2.36 and v2.35:
>
> * Rebase
>
> * Do not add set_ethertype() to datapath/actions.c.
> As this patch has evolved this function had devolved into
> to sets of functionality wrapped into a single function with
> only one line of common code. Refactor things to simply
> open-code setting the ether type in the two locations where
> set_ethertype() was previously used. The aim here is to improve
> readability.
>
> * Update setting skb->ethertype after mpls push and pop.
> - In the case of push_mpls it should be set unconditionally
> as in v2.35 the behaviour of this function to always push
> an MPLS LSE before any VLAN tags.
> - In the case of mpls_pop eth_p_mpls(skb->protocol) is a better
> test than skb->protocol != htons(ETH_P_8021Q) as it will give the
> correct behaviour in the presence of other VLAN ethernet types,
> for example 0x88a8 which is used by 802.1ad. Moreover, it seems
> correct to update the ethernet type if it was previously set
> according to the top-most MPLS LSE.
>
> * Deaccelerate VLANs when pushing MPLS tags the
> - Since v2.35 MPLS push will insert an MPLS LSE before any VLAN tags.
> This means that if an accelerated tag is present it should be
> deaccelerated to ensure it ends up in the correct position.
>
> * Update skb->mac_len in push_mpls() so that it will be correct
> when used by a subsequent call to pop_mpls().
>
> As things stand I do not believe this is strictly necessary as
> ovs-vswitchd will not send a pop MPLS action after a push MPLS action.
> However, I have added this in order to code more defensively as I believe
> that if such a sequence did occur it would be rather unobvious why
> it didn't work.
>
> * Do not add skb_cow_head() call in push_mpls().
> It is unnecessary as there is a make_writable() call.
> This change was also made in v2.30 but some how the
> code regressed between then and v2.35.
>
>
> Differences between v2.35 and v2.34:
>
> * Add support for the tag ordering specified up until OpenFlow 1.2 and
> the ordering specified from OpenFlow 1.3.
>
> * Correct error in datapath patch's handling of GSO in the presence
> of MPLS and absence of VLANs.
I forgot to update the trailing portion of this cover letter.
It should read as follows:
Patch overview:
* The first 5 patches add support for different tag ordering
to user-space.
* The 6th patch breaks out kernel datapath some code to allow
it to be re-used by the last patch.
* The last patch is a revised version of the patch to add MPLS support
to the kernel datapath.
To aid review this series is available in git at:
git://github.com/horms/openvswitch.git devel/mpls-v2.39
Patch list and overall diffstat:
Joe Stringer (5):
odp: Only pass vlan_tci to commit_vlan_action()
odp: Allow VLAN actions after MPLS actions
ofp-actions: Add OFPUTIL_OFPAT13_PUSH_MPLS
ofp-actions: Add separate OpenFlow 1.3 action parser
lib: Push MPLS tags in the OpenFlow 1.3 ordering
Simon Horman (2):
datapath: Break out deacceleration portion of vlan_push
datapath: Add basic MPLS support to kernel
datapath/Modules.mk | 1 +
datapath/actions.c | 154 +++++++-
datapath/datapath.c | 259 ++++++++++++--
datapath/datapath.h | 9 +
datapath/flow.c | 58 ++-
datapath/flow.h | 17 +-
datapath/linux/compat/gso.c | 50 ++-
datapath/linux/compat/gso.h | 39 +++
datapath/linux/compat/include/linux/netdevice.h | 12 -
datapath/linux/compat/netdevice.c | 28 --
datapath/mpls.h | 15 +
datapath/vport-lisp.c | 1 +
datapath/vport-netdev.c | 47 ++-
include/linux/openvswitch.h | 7 +-
lib/flow.c | 2 +-
lib/odp-util.c | 22 +-
lib/odp-util.h | 4 +-
lib/ofp-actions.c | 68 +++-
lib/ofp-parse.c | 1 +
lib/ofp-util.c | 3 +
lib/ofp-util.h | 1 +
lib/packets.c | 10 +-
lib/packets.h | 2 +-
ofproto/ofproto-dpif-xlate.c | 103 ++++--
ofproto/ofproto-dpif-xlate.h | 5 +
tests/ofproto-dpif.at | 446 ++++++++++++++++++++++++
26 files changed, 1225 insertions(+), 139 deletions(-)
create mode 100644 datapath/mpls.h
--
1.8.4
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [ovs-dev] [PATCH v2.39 0/7] MPLS actions and matches
2013-09-09 7:20 [PATCH v2.39 0/7] MPLS actions and matches Simon Horman
` (6 preceding siblings ...)
2013-09-09 7:25 ` [PATCH v2.39 0/7] MPLS actions and matches Simon Horman
@ 2013-09-12 19:06 ` Ben Pfaff
[not found] ` <20130912190636.GL16044-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
7 siblings, 1 reply; 38+ messages in thread
From: Ben Pfaff @ 2013-09-12 19:06 UTC (permalink / raw)
To: Simon Horman, Jesse Gross; +Cc: dev, netdev, Isaku Yamahata, Ravi K
I've totally lost track of the status of this patch series. I assume it
needs Jesse's review. Jesse, if I'm wrong about that, let me know and
I'll take a pass at it.
Thanks,
Ben.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 0/7] MPLS actions and matches
[not found] ` <20130912225614.GB30229-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
@ 2013-09-12 22:54 ` Ben Pfaff
2013-09-13 22:15 ` [ovs-dev] " Jesse Gross
0 siblings, 1 reply; 38+ messages in thread
From: Ben Pfaff @ 2013-09-12 22:54 UTC (permalink / raw)
To: Simon Horman
Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA,
Ravi K, Isaku Yamahata
On Fri, Sep 13, 2013 at 07:56:14AM +0900, Simon Horman wrote:
> On Thu, Sep 12, 2013 at 12:06:36PM -0700, Ben Pfaff wrote:
> > I've totally lost track of the status of this patch series. I assume it
> > needs Jesse's review. Jesse, if I'm wrong about that, let me know and
> > I'll take a pass at it.
>
> My understanding is that you have looked over the approach
> taken for the non-datapath code and were happy with it in
> the context that it needed review from Jesse along with the
> datapath code.
>
> I believe it was a few revisions ago that you looked over
> the series but I don't believe the non-datapath code has changed
> in a meaningful way since then.
That sounds plausible, thanks for refreshing my memory.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 0/7] MPLS actions and matches
[not found] ` <20130912190636.GL16044-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
@ 2013-09-12 22:56 ` Simon Horman
[not found] ` <20130912225614.GB30229-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
0 siblings, 1 reply; 38+ messages in thread
From: Simon Horman @ 2013-09-12 22:56 UTC (permalink / raw)
To: Ben Pfaff
Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA,
Ravi K, Isaku Yamahata
On Thu, Sep 12, 2013 at 12:06:36PM -0700, Ben Pfaff wrote:
> I've totally lost track of the status of this patch series. I assume it
> needs Jesse's review. Jesse, if I'm wrong about that, let me know and
> I'll take a pass at it.
My understanding is that you have looked over the approach
taken for the non-datapath code and were happy with it in
the context that it needed review from Jesse along with the
datapath code.
I believe it was a few revisions ago that you looked over
the series but I don't believe the non-datapath code has changed
in a meaningful way since then.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 6/7] datapath: Break out deacceleration portion of vlan_push
2013-09-09 7:20 ` [PATCH v2.39 6/7] datapath: Break out deacceleration portion of vlan_push Simon Horman
@ 2013-09-13 22:07 ` Jesse Gross
[not found] ` <CAEP_g=_ZQ6hNpxoHm6t3N=PxA+3WTrvNegL514j0R3GDM5C92A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-09-19 14:56 ` Simon Horman
0 siblings, 2 replies; 38+ messages in thread
From: Jesse Gross @ 2013-09-13 22:07 UTC (permalink / raw)
To: Simon Horman
Cc: dev, netdev, Ravi K, Isaku Yamahata, Pravin B Shelar, Joe Stringer
On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@verge.net.au> wrote:
> diff --git a/datapath/actions.c b/datapath/actions.c
> index 30ea1d2..6741d81 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -105,22 +105,29 @@ static int pop_vlan(struct sk_buff *skb)
> return 0;
> }
>
> -static int push_vlan(struct sk_buff *skb, const struct ovs_action_push_vlan *vlan)
> +/* push down current VLAN tag */
> +static struct sk_buff *put_vlan(struct sk_buff *skb)
This never changes the skb, right? Can we simplify things and just
return an error code?
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [ovs-dev] [PATCH v2.39 0/7] MPLS actions and matches
2013-09-12 22:54 ` Ben Pfaff
@ 2013-09-13 22:15 ` Jesse Gross
2013-09-15 16:39 ` Simon Horman
0 siblings, 1 reply; 38+ messages in thread
From: Jesse Gross @ 2013-09-13 22:15 UTC (permalink / raw)
To: Ben Pfaff; +Cc: Simon Horman, dev, netdev, Isaku Yamahata, Ravi K
On Thu, Sep 12, 2013 at 3:54 PM, Ben Pfaff <blp@nicira.com> wrote:
> On Fri, Sep 13, 2013 at 07:56:14AM +0900, Simon Horman wrote:
>> On Thu, Sep 12, 2013 at 12:06:36PM -0700, Ben Pfaff wrote:
>> > I've totally lost track of the status of this patch series. I assume it
>> > needs Jesse's review. Jesse, if I'm wrong about that, let me know and
>> > I'll take a pass at it.
>>
>> My understanding is that you have looked over the approach
>> taken for the non-datapath code and were happy with it in
>> the context that it needed review from Jesse along with the
>> datapath code.
>>
>> I believe it was a few revisions ago that you looked over
>> the series but I don't believe the non-datapath code has changed
>> in a meaningful way since then.
>
> That sounds plausible, thanks for refreshing my memory.
I haven't really reviewed the userspace code but there is one thing in
particular that concerns me: mpls_depth in the flow structure. We
obviously can't be making flow-level decisions on information that the
kernel doesn't include in the flow and I think that it is mostly
vestigial at this point. However, at best the name seems misleading
and at worst could result in someone trying to use information that we
don't really have. Can we fix this somehow? Maybe using the BoS bit?
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 6/7] datapath: Break out deacceleration portion of vlan_push
[not found] ` <CAEP_g=_ZQ6hNpxoHm6t3N=PxA+3WTrvNegL514j0R3GDM5C92A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-09-15 16:32 ` Simon Horman
0 siblings, 0 replies; 38+ messages in thread
From: Simon Horman @ 2013-09-15 16:32 UTC (permalink / raw)
To: Jesse Gross; +Cc: dev-yBygre7rU0TnMu66kgdUjQ, Ravi K, netdev, Isaku Yamahata
On Fri, Sep 13, 2013 at 03:07:12PM -0700, Jesse Gross wrote:
> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
> > diff --git a/datapath/actions.c b/datapath/actions.c
> > index 30ea1d2..6741d81 100644
> > --- a/datapath/actions.c
> > +++ b/datapath/actions.c
> > @@ -105,22 +105,29 @@ static int pop_vlan(struct sk_buff *skb)
> > return 0;
> > }
> >
> > -static int push_vlan(struct sk_buff *skb, const struct ovs_action_push_vlan *vlan)
> > +/* push down current VLAN tag */
> > +static struct sk_buff *put_vlan(struct sk_buff *skb)
>
> This never changes the skb, right? Can we simplify things and just
> return an error code?
Yes, I think that should be possible.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [ovs-dev] [PATCH v2.39 0/7] MPLS actions and matches
2013-09-13 22:15 ` [ovs-dev] " Jesse Gross
@ 2013-09-15 16:39 ` Simon Horman
0 siblings, 0 replies; 38+ messages in thread
From: Simon Horman @ 2013-09-15 16:39 UTC (permalink / raw)
To: Jesse Gross; +Cc: Ben Pfaff, dev, netdev, Isaku Yamahata, Ravi K
On Fri, Sep 13, 2013 at 03:15:17PM -0700, Jesse Gross wrote:
> On Thu, Sep 12, 2013 at 3:54 PM, Ben Pfaff <blp@nicira.com> wrote:
> > On Fri, Sep 13, 2013 at 07:56:14AM +0900, Simon Horman wrote:
> >> On Thu, Sep 12, 2013 at 12:06:36PM -0700, Ben Pfaff wrote:
> >> > I've totally lost track of the status of this patch series. I assume it
> >> > needs Jesse's review. Jesse, if I'm wrong about that, let me know and
> >> > I'll take a pass at it.
> >>
> >> My understanding is that you have looked over the approach
> >> taken for the non-datapath code and were happy with it in
> >> the context that it needed review from Jesse along with the
> >> datapath code.
> >>
> >> I believe it was a few revisions ago that you looked over
> >> the series but I don't believe the non-datapath code has changed
> >> in a meaningful way since then.
> >
> > That sounds plausible, thanks for refreshing my memory.
>
> I haven't really reviewed the userspace code but there is one thing in
> particular that concerns me: mpls_depth in the flow structure. We
> obviously can't be making flow-level decisions on information that the
> kernel doesn't include in the flow and I think that it is mostly
> vestigial at this point. However, at best the name seems misleading
> and at worst could result in someone trying to use information that we
> don't really have. Can we fix this somehow? Maybe using the BoS bit?
I believe we discussed this a long time ago.
The recirculation series includes a patch to allow multiple levels of pull
and push and that patch removes the mpls_depth field from the flow
structure. The approach taken is to track changes in the mpls depth during
translation, without knowing what the original depth was, and to
recirculate when the depth changes by more than one. So in a nutshell my
strategy was to use recirculation to resolve this problem.
However, I do entirely agree with your concerns and I don't believe that we
need mpls_depth at all even at this stage. I will take a look at removing
it by tracking the depth during translation without a dependency on
recirculation.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
[not found] ` <1378711207-29890-8-git-send-email-horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
@ 2013-09-16 20:38 ` Jesse Gross
[not found] ` <CAEP_g=8FBQPZ=C6G39YdHRzG57m5MqfXSZFAX2S_KLHRwfzc2w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-09-19 15:57 ` Simon Horman
0 siblings, 2 replies; 38+ messages in thread
From: Jesse Gross @ 2013-09-16 20:38 UTC (permalink / raw)
To: Simon Horman, Ben Pfaff, Pravin Shelar
Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev, Ravi K, Isaku Yamahata
On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
> diff --git a/datapath/actions.c b/datapath/actions.c
> index 6741d81..2335014 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> +/* Push MPLS after the ethernet header. */
> +static int push_mpls(struct sk_buff *skb,
> + const struct ovs_action_push_mpls *mpls)
[...]
> + /* update mac_len for subsequent pop_mpls actions. */
> + skb->mac_len += VLAN_HLEN;
Is this supposed to be part of put_vlan()?
[...]
> + if (skb->ip_summed == CHECKSUM_COMPLETE)
> + skb->csum = csum_add(skb->csum, csum_partial(new_mpls_lse,
> + MPLS_HLEN, 0));
> +
> + hdr = (struct ethhdr *)(skb_mac_header(skb));
I think you could simplify this by using eth_hdr().
> @@ -616,6 +736,13 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb)
> goto out_loop;
> }
>
> + /* Needed to initialise inner protocol on kernels older
> + * than v3.11 where skb->inner_protocol is not present
> + * and compatibility code uses the OVS_CB(skb) to store
> + * the inner protocol.
> + */
> + ovs_skb_set_inner_protocol(skb, skb->protocol);
The comment makes it sound like this code should just be deleted when
upstreaming. However, I believe that we still need to initialize this
field, right? Is this the best place do it or should it be conditional
on adding a first MPLS header? (i.e. what happens if inner_protocol is
already set and the packet simply passes through OVS?)
> diff --git a/datapath/vport-netdev.c b/datapath/vport-netdev.c
> index 215a47e..69f24d1 100644
> --- a/datapath/vport-netdev.c
> +++ b/datapath/vport-netdev.c
> @@ -287,8 +291,17 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
> + inner_protocol = ovs_skb_get_inner_protocol(skb);
> + if (eth_p_mpls(skb->protocol) && !eth_p_mpls(inner_protocol))
> + mpls = true;
> +
> + if (vlan_tx_tag_present(skb) && !dev_supports_vlan_tx(skb->dev))
> + vlan = true;
A couple of thoughts on this:
- I'm not sure that the check on inner_protocol is right since I
don't think it will be set to an MPLS EtherType in any real situation.
I think you can just drop it.
- Can we simplify the loop by just inserting the vlan tag (if
necessary), calling skb_gso_segment(), and sending the packet? I think
it should be possible now that our skb_gso_segment() backport is more
robust and it would make things easier to read.
- Can we have some kind of version check for MPLS, similar to what we
do for VLAN, so that we don't call into this on kernels >= 3.11?
These are all pretty targeted comments though, as was the one in the
previous kernel patch so I am basically happy with this overall.
To move forward with this:
- Pravin, would you mind double checking the GSO compat code?
- Ben, do you want to take over for the userspace portions of the
series on the assumption that the above comments can be fixed fairly
easily?
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
[not found] ` <CAEP_g=8FBQPZ=C6G39YdHRzG57m5MqfXSZFAX2S_KLHRwfzc2w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-09-16 20:46 ` Ben Pfaff
2013-09-17 23:47 ` Simon Horman
0 siblings, 1 reply; 38+ messages in thread
From: Ben Pfaff @ 2013-09-16 20:46 UTC (permalink / raw)
To: Jesse Gross; +Cc: dev-yBygre7rU0TnMu66kgdUjQ, Ravi K, netdev, Isaku Yamahata
On Mon, Sep 16, 2013 at 03:38:21PM -0500, Jesse Gross wrote:
> - Ben, do you want to take over for the userspace portions of the
> series on the assumption that the above comments can be fixed fairly
> easily?
Yes, that's fine. Simon, I guess that you are looking at Jesse's
comments about mpls_depth from earlier?
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-09 7:20 ` [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel Simon Horman
[not found] ` <1378711207-29890-8-git-send-email-horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
@ 2013-09-17 18:38 ` Pravin Shelar
2013-09-18 22:07 ` Simon Horman
1 sibling, 1 reply; 38+ messages in thread
From: Pravin Shelar @ 2013-09-17 18:38 UTC (permalink / raw)
To: Simon Horman
Cc: dev, netdev, Ravi K, Isaku Yamahata, Jesse Gross, Joe Stringer
On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@verge.net.au> wrote:
> Allow datapath to recognize and extract MPLS labels into flow keys
> and execute actions which push, pop, and set labels on packets.
>
> Based heavily on work by Leo Alterman, Ravi K, Isaku Yamahata and Joe Stringer.
>
> Cc: Ravi K <rkerur@gmail.com>
> Cc: Leo Alterman <lalterman@nicira.com>
> Cc: Isaku Yamahata <yamahata@valinux.co.jp>
> Cc: Joe Stringer <joe@wand.net.nz>
> Signed-off-by: Simon Horman <horms@verge.net.au>
>
> ---
....
> diff --git a/datapath/datapath.h b/datapath/datapath.h
> index 5d50dd4..babae3b 100644
> --- a/datapath/datapath.h
> +++ b/datapath/datapath.h
> @@ -36,6 +36,10 @@
>
> #define SAMPLE_ACTION_DEPTH 3
>
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
> +#define HAVE_INNER_PROTOCOL
> +#endif
> +
> /**
> * struct dp_stats_percpu - per-cpu packet processing statistics for a given
> * datapath.
> @@ -93,11 +97,16 @@ struct datapath {
> * @pkt_key: The flow information extracted from the packet. Must be nonnull.
> * @tun_key: Key for the tunnel that encapsulated this packet. NULL if the
> * packet is not being tunneled.
> + * @inner_protocol: Provides a substitute for the skb->inner_protocol field on
> + * kernels before 3.11.
> */
> struct ovs_skb_cb {
> struct sw_flow *flow;
> struct sw_flow_key *pkt_key;
> struct ovs_key_ipv4_tunnel *tun_key;
> +#ifndef HAVE_INNER_PROTOCOL
> + __be16 inner_protocol;
> +#endif
> };
> #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
>
Can you move this to compat struct ovs_gso_cb {}
....
>
> +struct sk_buff *rpl___skb_gso_segment(struct sk_buff *skb,
> + netdev_features_t features,
> + bool tx_path)
> +{
> + struct sk_buff *skb_gso;
> + __be16 type = skb->protocol;
> +
> + skb->protocol = skb_network_protocol(skb);
> +
> + /* this hack needed to get regular skb_gso_segment() */
> +#ifdef HAVE___SKB_GSO_SEGMENT
> +#undef __skb_gso_segment
> + skb_gso = __skb_gso_segment(skb, features, tx_path);
> +#else
> +#undef skb_gso_segment
> + skb_gso = skb_gso_segment(skb, features);
> +#endif
> +
> + if (!skb_gso || IS_ERR(skb_gso))
> + return skb_gso;
> +
> + skb = skb_gso;
> + while (skb) {
> + skb->protocol = type;
> + skb = skb->next;
> + }
> +
Protocol set is required if there is MPLS header, which is rare case.
So I think we can skip this loop if there is no mpls.
> + return skb_gso;
> +}
> +
> +struct sk_buff *rpl_skb_gso_segment(struct sk_buff *skb,
.....
> +
> + if (vlan_tx_tag_present(skb) && !dev_supports_vlan_tx(skb->dev))
> + vlan = true;
> +
> + if (vlan || mpls) {
> + netdev_features_t features;
>
> features = netif_skb_features(skb);
>
> @@ -296,6 +309,20 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
> features &= ~(NETIF_F_TSO | NETIF_F_TSO6 |
> NETIF_F_UFO | NETIF_F_FSO);
>
> + /* As of v3.11 the kernel provides an mpls_features field in
> + * struct net_device which allows devices to advertise which
> + * features its supports for MPLS. This value defaults to
> + * NETIF_F_SG and as of v3.11.
> + *
> + * This compatibility code is intended for kernels older
> + * than v3.11 that do not support MPLS GSO and thus do not
> + * provide mpls_features. Thus this code uses NETIF_F_SG
> + * directly in place of mpls_features.
> + */
> +
> + if (mpls)
> + features &= NETIF_F_SG;
> +
> if (netif_needs_gso(skb, features)) {
> struct sk_buff *nskb;
>
> @@ -319,10 +346,12 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
> nskb = skb->next;
> skb->next = NULL;
>
> - skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
> + if (vlan)
> + skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
> if (likely(skb)) {
> len += skb->len;
> - vlan_set_tci(skb, 0);
> + if (vlan)
> + vlan_set_tci(skb, 0);
> dev_queue_xmit(skb);
> }
>
> @@ -333,10 +362,12 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
> }
>
> tag:
> - skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
> - if (unlikely(!skb))
> - return 0;
> - vlan_set_tci(skb, 0);
> + if (vlan) {
> + skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
> + if (unlikely(!skb))
> + return 0;
> + vlan_set_tci(skb, 0);
> + }
> }
>
I think we can simplify code by pushing vlan and then segmenting skb,
the way we do it for MPLS.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-16 20:46 ` Ben Pfaff
@ 2013-09-17 23:47 ` Simon Horman
2013-09-18 0:05 ` Ben Pfaff
0 siblings, 1 reply; 38+ messages in thread
From: Simon Horman @ 2013-09-17 23:47 UTC (permalink / raw)
To: Ben Pfaff
Cc: Jesse Gross, Pravin Shelar, dev, netdev, Ravi K, Isaku Yamahata,
Joe Stringer
On Mon, Sep 16, 2013 at 01:46:19PM -0700, Ben Pfaff wrote:
> On Mon, Sep 16, 2013 at 03:38:21PM -0500, Jesse Gross wrote:
> > - Ben, do you want to take over for the userspace portions of the
> > series on the assumption that the above comments can be fixed fairly
> > easily?
>
> Yes, that's fine. Simon, I guess that you are looking at Jesse's
> comments about mpls_depth from earlier?
Yes, I have a solution to that problem and I will post it after testing
further. I don't think it will affect the other user-space patches
in a material way. So I think you can review them as-is.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-17 23:47 ` Simon Horman
@ 2013-09-18 0:05 ` Ben Pfaff
0 siblings, 0 replies; 38+ messages in thread
From: Ben Pfaff @ 2013-09-18 0:05 UTC (permalink / raw)
To: Simon Horman
Cc: Jesse Gross, Pravin Shelar, dev, netdev, Ravi K, Isaku Yamahata,
Joe Stringer
On Tue, Sep 17, 2013 at 06:47:22PM -0500, Simon Horman wrote:
> On Mon, Sep 16, 2013 at 01:46:19PM -0700, Ben Pfaff wrote:
> > On Mon, Sep 16, 2013 at 03:38:21PM -0500, Jesse Gross wrote:
> > > - Ben, do you want to take over for the userspace portions of the
> > > series on the assumption that the above comments can be fixed fairly
> > > easily?
> >
> > Yes, that's fine. Simon, I guess that you are looking at Jesse's
> > comments about mpls_depth from earlier?
>
> Yes, I have a solution to that problem and I will post it after testing
> further. I don't think it will affect the other user-space patches
> in a material way. So I think you can review them as-is.
That sounds good, thanks.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-17 18:38 ` Pravin Shelar
@ 2013-09-18 22:07 ` Simon Horman
2013-09-19 17:31 ` Jesse Gross
[not found] ` <20130918220756.GA24919-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
0 siblings, 2 replies; 38+ messages in thread
From: Simon Horman @ 2013-09-18 22:07 UTC (permalink / raw)
To: Pravin Shelar
Cc: dev, netdev, Ravi K, Isaku Yamahata, Jesse Gross, Joe Stringer
On Tue, Sep 17, 2013 at 11:38:18AM -0700, Pravin Shelar wrote:
> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@verge.net.au> wrote:
> > Allow datapath to recognize and extract MPLS labels into flow keys
> > and execute actions which push, pop, and set labels on packets.
> >
> > Based heavily on work by Leo Alterman, Ravi K, Isaku Yamahata and Joe Stringer.
> >
> > Cc: Ravi K <rkerur@gmail.com>
> > Cc: Leo Alterman <lalterman@nicira.com>
> > Cc: Isaku Yamahata <yamahata@valinux.co.jp>
> > Cc: Joe Stringer <joe@wand.net.nz>
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> >
> > ---
> ....
> > diff --git a/datapath/datapath.h b/datapath/datapath.h
> > index 5d50dd4..babae3b 100644
> > --- a/datapath/datapath.h
> > +++ b/datapath/datapath.h
> > @@ -36,6 +36,10 @@
> >
> > #define SAMPLE_ACTION_DEPTH 3
> >
> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
> > +#define HAVE_INNER_PROTOCOL
> > +#endif
> > +
> > /**
> > * struct dp_stats_percpu - per-cpu packet processing statistics for a given
> > * datapath.
> > @@ -93,11 +97,16 @@ struct datapath {
> > * @pkt_key: The flow information extracted from the packet. Must be nonnull.
> > * @tun_key: Key for the tunnel that encapsulated this packet. NULL if the
> > * packet is not being tunneled.
> > + * @inner_protocol: Provides a substitute for the skb->inner_protocol field on
> > + * kernels before 3.11.
> > */
> > struct ovs_skb_cb {
> > struct sw_flow *flow;
> > struct sw_flow_key *pkt_key;
> > struct ovs_key_ipv4_tunnel *tun_key;
> > +#ifndef HAVE_INNER_PROTOCOL
> > + __be16 inner_protocol;
> > +#endif
> > };
> > #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
> >
> Can you move this to compat struct ovs_gso_cb {}
I think that you are correct and inner_protocol needs
to be in struct ovs_gso_cb so that it can
be accessed via skb_network_protocol() from
rpl___skb_gso_segment().
However I think it may also need to be present in struct ovs_cb
so that it can be set correctly.
Currently it is set unconditionally
in ovs_execute_actions() and Jesse has suggested setting
it conditionally in pop_mpls() (which is called by do_execute_actions()).
But regardless it seems to me that the field would need to be available
in struct ovs_cb.
>
> ....
> >
> > +struct sk_buff *rpl___skb_gso_segment(struct sk_buff *skb,
> > + netdev_features_t features,
> > + bool tx_path)
> > +{
> > + struct sk_buff *skb_gso;
> > + __be16 type = skb->protocol;
> > +
> > + skb->protocol = skb_network_protocol(skb);
> > +
> > + /* this hack needed to get regular skb_gso_segment() */
> > +#ifdef HAVE___SKB_GSO_SEGMENT
> > +#undef __skb_gso_segment
> > + skb_gso = __skb_gso_segment(skb, features, tx_path);
> > +#else
> > +#undef skb_gso_segment
> > + skb_gso = skb_gso_segment(skb, features);
> > +#endif
> > +
> > + if (!skb_gso || IS_ERR(skb_gso))
> > + return skb_gso;
> > +
> > + skb = skb_gso;
> > + while (skb) {
> > + skb->protocol = type;
> > + skb = skb->next;
> > + }
> > +
>
> Protocol set is required if there is MPLS header, which is rare case.
> So I think we can skip this loop if there is no mpls.
I'm not sure I see the benefit but I do agree with your analysis and
I'll make it so.
>
> > + return skb_gso;
> > +}
> > +
> > +struct sk_buff *rpl_skb_gso_segment(struct sk_buff *skb,
> .....
> > +
> > + if (vlan_tx_tag_present(skb) && !dev_supports_vlan_tx(skb->dev))
> > + vlan = true;
> > +
> > + if (vlan || mpls) {
> > + netdev_features_t features;
> >
> > features = netif_skb_features(skb);
> >
> > @@ -296,6 +309,20 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
> > features &= ~(NETIF_F_TSO | NETIF_F_TSO6 |
> > NETIF_F_UFO | NETIF_F_FSO);
> >
> > + /* As of v3.11 the kernel provides an mpls_features field in
> > + * struct net_device which allows devices to advertise which
> > + * features its supports for MPLS. This value defaults to
> > + * NETIF_F_SG and as of v3.11.
> > + *
> > + * This compatibility code is intended for kernels older
> > + * than v3.11 that do not support MPLS GSO and thus do not
> > + * provide mpls_features. Thus this code uses NETIF_F_SG
> > + * directly in place of mpls_features.
> > + */
> > +
> > + if (mpls)
> > + features &= NETIF_F_SG;
> > +
> > if (netif_needs_gso(skb, features)) {
> > struct sk_buff *nskb;
> >
> > @@ -319,10 +346,12 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
> > nskb = skb->next;
> > skb->next = NULL;
> >
> > - skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
> > + if (vlan)
> > + skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
> > if (likely(skb)) {
> > len += skb->len;
> > - vlan_set_tci(skb, 0);
> > + if (vlan)
> > + vlan_set_tci(skb, 0);
> > dev_queue_xmit(skb);
> > }
> >
> > @@ -333,10 +362,12 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
> > }
> >
> > tag:
> > - skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
> > - if (unlikely(!skb))
> > - return 0;
> > - vlan_set_tci(skb, 0);
> > + if (vlan) {
> > + skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
> > + if (unlikely(!skb))
> > + return 0;
> > + vlan_set_tci(skb, 0);
> > + }
> > }
> >
> I think we can simplify code by pushing vlan and then segmenting skb,
> the way we do it for MPLS.
Are you thinking of something like the following which applies
prior to the MPLS code.
[RFC] datapath: simplify VLAN segmentation
*** Do not apply: for informational purposes only
Push vlan tag onto packet before segmentation to simplify the code.
As suggested by Pravin Shelar.
Compile tested only.
Signed-off-by: Simon Horman <horms@verge.net.au>
---
datapath/vport-netdev.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/datapath/vport-netdev.c b/datapath/vport-netdev.c
index 215a47e..31680fd 100644
--- a/datapath/vport-netdev.c
+++ b/datapath/vport-netdev.c
@@ -296,6 +296,11 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
features &= ~(NETIF_F_TSO | NETIF_F_TSO6 |
NETIF_F_UFO | NETIF_F_FSO);
+ skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
+ if (unlikely(!skb))
+ return 0;
+ vlan_set_tci(skb, 0);
+
if (netif_needs_gso(skb, features)) {
struct sk_buff *nskb;
@@ -306,7 +311,7 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
goto drop;
skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY;
- goto tag;
+ goto xmit;
}
if (IS_ERR(nskb))
@@ -318,27 +323,16 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
do {
nskb = skb->next;
skb->next = NULL;
-
- skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
- if (likely(skb)) {
- len += skb->len;
- vlan_set_tci(skb, 0);
- dev_queue_xmit(skb);
- }
-
+ len += skb->len;
+ dev_queue_xmit(skb);
skb = nskb;
} while (skb);
return len;
}
-
-tag:
- skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
- if (unlikely(!skb))
- return 0;
- vlan_set_tci(skb, 0);
}
+xmit:
len = skb->len;
dev_queue_xmit(skb);
--
1.8.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 6/7] datapath: Break out deacceleration portion of vlan_push
2013-09-13 22:07 ` Jesse Gross
[not found] ` <CAEP_g=_ZQ6hNpxoHm6t3N=PxA+3WTrvNegL514j0R3GDM5C92A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-09-19 14:56 ` Simon Horman
1 sibling, 0 replies; 38+ messages in thread
From: Simon Horman @ 2013-09-19 14:56 UTC (permalink / raw)
To: Jesse Gross
Cc: dev, netdev, Ravi K, Isaku Yamahata, Pravin B Shelar, Joe Stringer
On Fri, Sep 13, 2013 at 03:07:12PM -0700, Jesse Gross wrote:
> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@verge.net.au> wrote:
> > diff --git a/datapath/actions.c b/datapath/actions.c
> > index 30ea1d2..6741d81 100644
> > --- a/datapath/actions.c
> > +++ b/datapath/actions.c
> > @@ -105,22 +105,29 @@ static int pop_vlan(struct sk_buff *skb)
> > return 0;
> > }
> >
> > -static int push_vlan(struct sk_buff *skb, const struct ovs_action_push_vlan *vlan)
> > +/* push down current VLAN tag */
> > +static struct sk_buff *put_vlan(struct sk_buff *skb)
>
> This never changes the skb, right? Can we simplify things and just
> return an error code?
Yes. I'm not sure what I was thinking when I chose not to do that
but I will change things around as you suggest.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-16 20:38 ` Jesse Gross
[not found] ` <CAEP_g=8FBQPZ=C6G39YdHRzG57m5MqfXSZFAX2S_KLHRwfzc2w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-09-19 15:57 ` Simon Horman
2013-09-19 17:21 ` Jesse Gross
1 sibling, 1 reply; 38+ messages in thread
From: Simon Horman @ 2013-09-19 15:57 UTC (permalink / raw)
To: Jesse Gross
Cc: Ben Pfaff, Pravin Shelar, dev, netdev, Ravi K, Isaku Yamahata,
Joe Stringer
On Mon, Sep 16, 2013 at 03:38:21PM -0500, Jesse Gross wrote:
> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@verge.net.au> wrote:
> > diff --git a/datapath/actions.c b/datapath/actions.c
> > index 6741d81..2335014 100644
> > --- a/datapath/actions.c
> > +++ b/datapath/actions.c
> > +/* Push MPLS after the ethernet header. */
> > +static int push_mpls(struct sk_buff *skb,
> > + const struct ovs_action_push_mpls *mpls)
> [...]
> > + /* update mac_len for subsequent pop_mpls actions. */
> > + skb->mac_len += VLAN_HLEN;
>
> Is this supposed to be part of put_vlan()?
Thanks, this does seem bogus.
I will remove it as mac_len is already incremented in put_vlan().
>
> [...]
>
> > + if (skb->ip_summed == CHECKSUM_COMPLETE)
> > + skb->csum = csum_add(skb->csum, csum_partial(new_mpls_lse,
> > + MPLS_HLEN, 0));
> > +
> > + hdr = (struct ethhdr *)(skb_mac_header(skb));
>
> I think you could simplify this by using eth_hdr().
Thanks, I have updated the code as follows:
hdr = eth_hdr(skb);
> > @@ -616,6 +736,13 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb)
> > goto out_loop;
> > }
> >
> > + /* Needed to initialise inner protocol on kernels older
> > + * than v3.11 where skb->inner_protocol is not present
> > + * and compatibility code uses the OVS_CB(skb) to store
> > + * the inner protocol.
> > + */
> > + ovs_skb_set_inner_protocol(skb, skb->protocol);
>
> The comment makes it sound like this code should just be deleted when
> upstreaming. However, I believe that we still need to initialize this
> field, right? Is this the best place do it or should it be conditional
> on adding a first MPLS header? (i.e. what happens if inner_protocol is
> already set and the packet simply passes through OVS?)
I believe there are several problems here.
The first one, which my comment was written around is that I think that if
inner_protocol is a field of struct sk_buff then we can rely on it already
being initialised. However, if we are using compatibility code, where
inner_protcol is called in the callback field of struct sk_buff then I
think that OVS needs to initialise it.
Perhaps a good solution to that problem is to add
an ovs_skb_reset_inner_protocol() call which sets
the inner_protocol unconditionally if it is not a field of struct sk_buff.
I believe this could be called from ovs_execute_actions().
A second problem is one that you raise which I had not considered
which is how to handle things if inner_protocol is already set.
I believe this should only occur in the case where inner_protocol
is a field of struct sk_buff and I think it would be most convenient
to set it conditionally in ovs_skb_reset_inner_protocol().
I think that if it is not set it should be zero but it should be
safe to check for values less than ETH_P_802_3_MIN.
In short, I am thinking of something like this:
#ifdef HAVE_INNER_PROTOCOL
static inline void ovs_skb_reset_inner_protocol(struct sk_buff *skb)
{
OVS_CB(skb)->inner_protocol = skb->protocol;
}
#else
static inline void ovs_skb_reset_inner_protocol(struct sk_buff *skb)
{
if (skb->inner_protocol < ETH_P_802_3_MIN)
skb->inner_protocol = skb->protocol;
}
#endif
Pravin has also raised a separate discussion on wheather
OVS_GSO_CB should be used in place of OVS_CB. I would like
to continue that discussion in the sub-thread where is started.
Though obviously the outcome of that discussion may affect the
exact implementation of a solution to the problem discussed above.
> > diff --git a/datapath/vport-netdev.c b/datapath/vport-netdev.c
> > index 215a47e..69f24d1 100644
> > --- a/datapath/vport-netdev.c
> > +++ b/datapath/vport-netdev.c
> > @@ -287,8 +291,17 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
> > + inner_protocol = ovs_skb_get_inner_protocol(skb);
> > + if (eth_p_mpls(skb->protocol) && !eth_p_mpls(inner_protocol))
> > + mpls = true;
> > +
> > + if (vlan_tx_tag_present(skb) && !dev_supports_vlan_tx(skb->dev))
> > + vlan = true;
>
> A couple of thoughts on this:
> - I'm not sure that the check on inner_protocol is right since I
> don't think it will be set to an MPLS EtherType in any real situation.
> I think you can just drop it.
Sure, that sounds reasonable.
> - Can we simplify the loop by just inserting the vlan tag (if
> necessary), calling skb_gso_segment(), and sending the packet? I think
> it should be possible now that our skb_gso_segment() backport is more
> robust and it would make things easier to read.
Pravin also raised that idea and yes I think it should be possible.
I have posted a prototype in the sub-thread where Pravin reviewed the code
(as I read that first).
> - Can we have some kind of version check for MPLS, similar to what we
> do for VLAN, so that we don't call into this on kernels >= 3.11?
Sure, I'll see about making that so.
> These are all pretty targeted comments though, as was the one in the
> previous kernel patch so I am basically happy with this overall.
>
> To move forward with this:
> - Pravin, would you mind double checking the GSO compat code?
> - Ben, do you want to take over for the userspace portions of the
> series on the assumption that the above comments can be fixed fairly
> easily?
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-19 15:57 ` Simon Horman
@ 2013-09-19 17:21 ` Jesse Gross
2013-09-22 5:34 ` Simon Horman
0 siblings, 1 reply; 38+ messages in thread
From: Jesse Gross @ 2013-09-19 17:21 UTC (permalink / raw)
To: Simon Horman
Cc: Ben Pfaff, Pravin Shelar, dev, netdev, Ravi K, Isaku Yamahata,
Joe Stringer
On Thu, Sep 19, 2013 at 10:57 AM, Simon Horman <horms@verge.net.au> wrote:
> On Mon, Sep 16, 2013 at 03:38:21PM -0500, Jesse Gross wrote:
>> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@verge.net.au> wrote:
>> > @@ -616,6 +736,13 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb)
>> > goto out_loop;
>> > }
>> >
>> > + /* Needed to initialise inner protocol on kernels older
>> > + * than v3.11 where skb->inner_protocol is not present
>> > + * and compatibility code uses the OVS_CB(skb) to store
>> > + * the inner protocol.
>> > + */
>> > + ovs_skb_set_inner_protocol(skb, skb->protocol);
>>
>> The comment makes it sound like this code should just be deleted when
>> upstreaming. However, I believe that we still need to initialize this
>> field, right? Is this the best place do it or should it be conditional
>> on adding a first MPLS header? (i.e. what happens if inner_protocol is
>> already set and the packet simply passes through OVS?)
>
> I believe there are several problems here.
>
> The first one, which my comment was written around is that I think that if
> inner_protocol is a field of struct sk_buff then we can rely on it already
> being initialised. However, if we are using compatibility code, where
> inner_protcol is called in the callback field of struct sk_buff then I
> think that OVS needs to initialise it.
I'm not sure that it's true that inner_protocol is already initialized
- I grepped the tree and the only assignment that I found is in
skbuff.c in __copy_skb_header().
> A second problem is one that you raise which I had not considered
> which is how to handle things if inner_protocol is already set.
>
> I believe this should only occur in the case where inner_protocol
> is a field of struct sk_buff and I think it would be most convenient
> to set it conditionally in ovs_skb_reset_inner_protocol().
> I think that if it is not set it should be zero but it should be
> safe to check for values less than ETH_P_802_3_MIN.
It's probably OK to check for values less than ETH_P_802_3_MIN but I'm
not sure that it's the most correct thing to do since skb->protocol
could contain these values (such as ETH_P_802_2). It's unlikely that
they will be GSO packets but it seems better to use the more strict
check against zero.
One other consideration in the OVS case - with recirculation we may
hit this code multiple times and the difference in behavior could be
surprising. However, on the other hand, we need to be careful because
skb->cb is not guaranteed to be initialized to zero.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-18 22:07 ` Simon Horman
@ 2013-09-19 17:31 ` Jesse Gross
2013-09-22 5:38 ` Simon Horman
[not found] ` <20130918220756.GA24919-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
1 sibling, 1 reply; 38+ messages in thread
From: Jesse Gross @ 2013-09-19 17:31 UTC (permalink / raw)
To: Simon Horman
Cc: Pravin Shelar, dev, netdev, Ravi K, Isaku Yamahata, Joe Stringer
On Wed, Sep 18, 2013 at 5:07 PM, Simon Horman <horms@verge.net.au> wrote:
> On Tue, Sep 17, 2013 at 11:38:18AM -0700, Pravin Shelar wrote:
>> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@verge.net.au> wrote:
>> > diff --git a/datapath/datapath.h b/datapath/datapath.h
>> > index 5d50dd4..babae3b 100644
>> > --- a/datapath/datapath.h
>> > +++ b/datapath/datapath.h
>> > @@ -36,6 +36,10 @@
>> >
>> > #define SAMPLE_ACTION_DEPTH 3
>> >
>> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
>> > +#define HAVE_INNER_PROTOCOL
>> > +#endif
>> > +
>> > /**
>> > * struct dp_stats_percpu - per-cpu packet processing statistics for a given
>> > * datapath.
>> > @@ -93,11 +97,16 @@ struct datapath {
>> > * @pkt_key: The flow information extracted from the packet. Must be nonnull.
>> > * @tun_key: Key for the tunnel that encapsulated this packet. NULL if the
>> > * packet is not being tunneled.
>> > + * @inner_protocol: Provides a substitute for the skb->inner_protocol field on
>> > + * kernels before 3.11.
>> > */
>> > struct ovs_skb_cb {
>> > struct sw_flow *flow;
>> > struct sw_flow_key *pkt_key;
>> > struct ovs_key_ipv4_tunnel *tun_key;
>> > +#ifndef HAVE_INNER_PROTOCOL
>> > + __be16 inner_protocol;
>> > +#endif
>> > };
>> > #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
>> >
>> Can you move this to compat struct ovs_gso_cb {}
>
> I think that you are correct and inner_protocol needs
> to be in struct ovs_gso_cb so that it can
> be accessed via skb_network_protocol() from
> rpl___skb_gso_segment().
>
> However I think it may also need to be present in struct ovs_cb
> so that it can be set correctly.
>
> Currently it is set unconditionally
> in ovs_execute_actions() and Jesse has suggested setting
> it conditionally in pop_mpls() (which is called by do_execute_actions()).
> But regardless it seems to me that the field would need to be available
> in struct ovs_cb.
Since the helper functions are also in the compat code, I think they
should have access to ovs_gso_cb.
>> I think we can simplify code by pushing vlan and then segmenting skb,
>> the way we do it for MPLS.
>
> Are you thinking of something like the following which applies
> prior to the MPLS code.
This is basically what I was thinking about. We might actually be able
to move all of this to compat code by having a replacement for
dev_queue_xmit() similar to what we have for ip_local_out() in the
tunnel code.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
[not found] ` <20130918220756.GA24919-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
@ 2013-09-19 20:45 ` Simon Horman
2013-09-20 1:31 ` [ovs-dev] " Pravin Shelar
0 siblings, 1 reply; 38+ messages in thread
From: Simon Horman @ 2013-09-19 20:45 UTC (permalink / raw)
To: Pravin Shelar; +Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev, Ravi K, Isaku Yamahata
On Wed, Sep 18, 2013 at 05:07:59PM -0500, Simon Horman wrote:
> On Tue, Sep 17, 2013 at 11:38:18AM -0700, Pravin Shelar wrote:
> > On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
> > > Allow datapath to recognize and extract MPLS labels into flow keys
> > > and execute actions which push, pop, and set labels on packets.
> > >
> > > Based heavily on work by Leo Alterman, Ravi K, Isaku Yamahata and Joe Stringer.
> > >
> > > Cc: Ravi K <rkerur-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > Cc: Leo Alterman <lalterman-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
> > > Cc: Isaku Yamahata <yamahata-jCdQPDEk3idL9jVzuh4AOg@public.gmane.org>
> > > Cc: Joe Stringer <joe-Q1GJJQv1iO6lP80pJB477g@public.gmane.org>
> > > Signed-off-by: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
> > >
> > > ---
> > ....
> > > diff --git a/datapath/datapath.h b/datapath/datapath.h
> > > index 5d50dd4..babae3b 100644
> > > --- a/datapath/datapath.h
> > > +++ b/datapath/datapath.h
> > > @@ -36,6 +36,10 @@
> > >
> > > #define SAMPLE_ACTION_DEPTH 3
> > >
> > > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
> > > +#define HAVE_INNER_PROTOCOL
> > > +#endif
> > > +
> > > /**
> > > * struct dp_stats_percpu - per-cpu packet processing statistics for a given
> > > * datapath.
> > > @@ -93,11 +97,16 @@ struct datapath {
> > > * @pkt_key: The flow information extracted from the packet. Must be nonnull.
> > > * @tun_key: Key for the tunnel that encapsulated this packet. NULL if the
> > > * packet is not being tunneled.
> > > + * @inner_protocol: Provides a substitute for the skb->inner_protocol field on
> > > + * kernels before 3.11.
> > > */
> > > struct ovs_skb_cb {
> > > struct sw_flow *flow;
> > > struct sw_flow_key *pkt_key;
> > > struct ovs_key_ipv4_tunnel *tun_key;
> > > +#ifndef HAVE_INNER_PROTOCOL
> > > + __be16 inner_protocol;
> > > +#endif
> > > };
> > > #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
> > >
> > Can you move this to compat struct ovs_gso_cb {}
>
> I think that you are correct and inner_protocol needs
> to be in struct ovs_gso_cb so that it can
> be accessed via skb_network_protocol() from
> rpl___skb_gso_segment().
>
> However I think it may also need to be present in struct ovs_cb
> so that it can be set correctly.
>
> Currently it is set unconditionally
> in ovs_execute_actions() and Jesse has suggested setting
> it conditionally in pop_mpls() (which is called by do_execute_actions()).
> But regardless it seems to me that the field would need to be available
> in struct ovs_cb.
Having reviewed the code once more I now notice that struct ovs_gso_cb
contains struct ovs_skb_cb dp_cb. Whereas my previous assumption was
that they were mutually exclusive.
With this in mind I think it should be safe to use ovs_gso_cb from
ovs_execute_actions() or do_execute_actions() but I would value
your opinion on that.
Conversely, if inner_protocol was left in struct ovs_skb_cb there should
be no problem with accessing it from GSO code as the code currently does.
So I am not sure that I see the value of moving it but I am happy to do
so if you think it is safe and it is your preferred option.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [ovs-dev] [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-19 20:45 ` Simon Horman
@ 2013-09-20 1:31 ` Pravin Shelar
2013-09-22 3:54 ` Simon Horman
0 siblings, 1 reply; 38+ messages in thread
From: Pravin Shelar @ 2013-09-20 1:31 UTC (permalink / raw)
To: Simon Horman; +Cc: dev, Ravi K, netdev, Isaku Yamahata
On Thu, Sep 19, 2013 at 1:45 PM, Simon Horman <horms@verge.net.au> wrote:
> On Wed, Sep 18, 2013 at 05:07:59PM -0500, Simon Horman wrote:
>> On Tue, Sep 17, 2013 at 11:38:18AM -0700, Pravin Shelar wrote:
>> > On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@verge.net.au> wrote:
>> > > Allow datapath to recognize and extract MPLS labels into flow keys
>> > > and execute actions which push, pop, and set labels on packets.
>> > >
>> > > Based heavily on work by Leo Alterman, Ravi K, Isaku Yamahata and Joe Stringer.
>> > >
>> > > Cc: Ravi K <rkerur@gmail.com>
>> > > Cc: Leo Alterman <lalterman@nicira.com>
>> > > Cc: Isaku Yamahata <yamahata@valinux.co.jp>
>> > > Cc: Joe Stringer <joe@wand.net.nz>
>> > > Signed-off-by: Simon Horman <horms@verge.net.au>
>> > >
>> > > ---
>> > ....
>> > > diff --git a/datapath/datapath.h b/datapath/datapath.h
>> > > index 5d50dd4..babae3b 100644
>> > > --- a/datapath/datapath.h
>> > > +++ b/datapath/datapath.h
>> > > @@ -36,6 +36,10 @@
>> > >
>> > > #define SAMPLE_ACTION_DEPTH 3
>> > >
>> > > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
>> > > +#define HAVE_INNER_PROTOCOL
>> > > +#endif
>> > > +
>> > > /**
>> > > * struct dp_stats_percpu - per-cpu packet processing statistics for a given
>> > > * datapath.
>> > > @@ -93,11 +97,16 @@ struct datapath {
>> > > * @pkt_key: The flow information extracted from the packet. Must be nonnull.
>> > > * @tun_key: Key for the tunnel that encapsulated this packet. NULL if the
>> > > * packet is not being tunneled.
>> > > + * @inner_protocol: Provides a substitute for the skb->inner_protocol field on
>> > > + * kernels before 3.11.
>> > > */
>> > > struct ovs_skb_cb {
>> > > struct sw_flow *flow;
>> > > struct sw_flow_key *pkt_key;
>> > > struct ovs_key_ipv4_tunnel *tun_key;
>> > > +#ifndef HAVE_INNER_PROTOCOL
>> > > + __be16 inner_protocol;
>> > > +#endif
>> > > };
>> > > #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
>> > >
>> > Can you move this to compat struct ovs_gso_cb {}
>>
>> I think that you are correct and inner_protocol needs
>> to be in struct ovs_gso_cb so that it can
>> be accessed via skb_network_protocol() from
>> rpl___skb_gso_segment().
>>
>> However I think it may also need to be present in struct ovs_cb
>> so that it can be set correctly.
>>
>> Currently it is set unconditionally
>> in ovs_execute_actions() and Jesse has suggested setting
>> it conditionally in pop_mpls() (which is called by do_execute_actions()).
>> But regardless it seems to me that the field would need to be available
>> in struct ovs_cb.
>
> Having reviewed the code once more I now notice that struct ovs_gso_cb
> contains struct ovs_skb_cb dp_cb. Whereas my previous assumption was
> that they were mutually exclusive.
>
> With this in mind I think it should be safe to use ovs_gso_cb from
> ovs_execute_actions() or do_execute_actions() but I would value
> your opinion on that.
>
> Conversely, if inner_protocol was left in struct ovs_skb_cb there should
> be no problem with accessing it from GSO code as the code currently does.
> So I am not sure that I see the value of moving it but I am happy to do
> so if you think it is safe and it is your preferred option.
right, access is not issue.
Value is the code in datapath.c, with compatibility, remains closer to
upstream ovs module. We have always tried to keep it that way.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [ovs-dev] [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-20 1:31 ` [ovs-dev] " Pravin Shelar
@ 2013-09-22 3:54 ` Simon Horman
0 siblings, 0 replies; 38+ messages in thread
From: Simon Horman @ 2013-09-22 3:54 UTC (permalink / raw)
To: Pravin Shelar; +Cc: dev, Ravi K, netdev, Isaku Yamahata
On Thu, Sep 19, 2013 at 06:31:14PM -0700, Pravin Shelar wrote:
> On Thu, Sep 19, 2013 at 1:45 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Wed, Sep 18, 2013 at 05:07:59PM -0500, Simon Horman wrote:
> >> On Tue, Sep 17, 2013 at 11:38:18AM -0700, Pravin Shelar wrote:
> >> > On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > > Allow datapath to recognize and extract MPLS labels into flow keys
> >> > > and execute actions which push, pop, and set labels on packets.
> >> > >
> >> > > Based heavily on work by Leo Alterman, Ravi K, Isaku Yamahata and Joe Stringer.
> >> > >
> >> > > Cc: Ravi K <rkerur@gmail.com>
> >> > > Cc: Leo Alterman <lalterman@nicira.com>
> >> > > Cc: Isaku Yamahata <yamahata@valinux.co.jp>
> >> > > Cc: Joe Stringer <joe@wand.net.nz>
> >> > > Signed-off-by: Simon Horman <horms@verge.net.au>
> >> > >
> >> > > ---
> >> > ....
> >> > > diff --git a/datapath/datapath.h b/datapath/datapath.h
> >> > > index 5d50dd4..babae3b 100644
> >> > > --- a/datapath/datapath.h
> >> > > +++ b/datapath/datapath.h
> >> > > @@ -36,6 +36,10 @@
> >> > >
> >> > > #define SAMPLE_ACTION_DEPTH 3
> >> > >
> >> > > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
> >> > > +#define HAVE_INNER_PROTOCOL
> >> > > +#endif
> >> > > +
> >> > > /**
> >> > > * struct dp_stats_percpu - per-cpu packet processing statistics for a given
> >> > > * datapath.
> >> > > @@ -93,11 +97,16 @@ struct datapath {
> >> > > * @pkt_key: The flow information extracted from the packet. Must be nonnull.
> >> > > * @tun_key: Key for the tunnel that encapsulated this packet. NULL if the
> >> > > * packet is not being tunneled.
> >> > > + * @inner_protocol: Provides a substitute for the skb->inner_protocol field on
> >> > > + * kernels before 3.11.
> >> > > */
> >> > > struct ovs_skb_cb {
> >> > > struct sw_flow *flow;
> >> > > struct sw_flow_key *pkt_key;
> >> > > struct ovs_key_ipv4_tunnel *tun_key;
> >> > > +#ifndef HAVE_INNER_PROTOCOL
> >> > > + __be16 inner_protocol;
> >> > > +#endif
> >> > > };
> >> > > #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
> >> > >
> >> > Can you move this to compat struct ovs_gso_cb {}
> >>
> >> I think that you are correct and inner_protocol needs
> >> to be in struct ovs_gso_cb so that it can
> >> be accessed via skb_network_protocol() from
> >> rpl___skb_gso_segment().
> >>
> >> However I think it may also need to be present in struct ovs_cb
> >> so that it can be set correctly.
> >>
> >> Currently it is set unconditionally
> >> in ovs_execute_actions() and Jesse has suggested setting
> >> it conditionally in pop_mpls() (which is called by do_execute_actions()).
> >> But regardless it seems to me that the field would need to be available
> >> in struct ovs_cb.
> >
> > Having reviewed the code once more I now notice that struct ovs_gso_cb
> > contains struct ovs_skb_cb dp_cb. Whereas my previous assumption was
> > that they were mutually exclusive.
> >
> > With this in mind I think it should be safe to use ovs_gso_cb from
> > ovs_execute_actions() or do_execute_actions() but I would value
> > your opinion on that.
> >
> > Conversely, if inner_protocol was left in struct ovs_skb_cb there should
> > be no problem with accessing it from GSO code as the code currently does.
> > So I am not sure that I see the value of moving it but I am happy to do
> > so if you think it is safe and it is your preferred option.
>
> right, access is not issue.
> Value is the code in datapath.c, with compatibility, remains closer to
> upstream ovs module. We have always tried to keep it that way.
Thanks, I understand.
I have moved inner_protocol as you suggest.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-19 17:21 ` Jesse Gross
@ 2013-09-22 5:34 ` Simon Horman
[not found] ` <20130922053156.GA4849-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
0 siblings, 1 reply; 38+ messages in thread
From: Simon Horman @ 2013-09-22 5:34 UTC (permalink / raw)
To: Jesse Gross
Cc: Ben Pfaff, Pravin Shelar, dev, netdev, Ravi K, Isaku Yamahata,
Joe Stringer
On Thu, Sep 19, 2013 at 12:21:33PM -0500, Jesse Gross wrote:
> On Thu, Sep 19, 2013 at 10:57 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Mon, Sep 16, 2013 at 03:38:21PM -0500, Jesse Gross wrote:
> >> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > @@ -616,6 +736,13 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb)
> >> > goto out_loop;
> >> > }
> >> >
> >> > + /* Needed to initialise inner protocol on kernels older
> >> > + * than v3.11 where skb->inner_protocol is not present
> >> > + * and compatibility code uses the OVS_CB(skb) to store
> >> > + * the inner protocol.
> >> > + */
> >> > + ovs_skb_set_inner_protocol(skb, skb->protocol);
> >>
> >> The comment makes it sound like this code should just be deleted when
> >> upstreaming. However, I believe that we still need to initialize this
> >> field, right? Is this the best place do it or should it be conditional
> >> on adding a first MPLS header? (i.e. what happens if inner_protocol is
> >> already set and the packet simply passes through OVS?)
> >
> > I believe there are several problems here.
> >
> > The first one, which my comment was written around is that I think that if
> > inner_protocol is a field of struct sk_buff then we can rely on it already
> > being initialised. However, if we are using compatibility code, where
> > inner_protcol is called in the callback field of struct sk_buff then I
> > think that OVS needs to initialise it.
>
> I'm not sure that it's true that inner_protocol is already initialized
> - I grepped the tree and the only assignment that I found is in
> skbuff.c in __copy_skb_header().
My assumption was that it would be initialised to zero,
primarily due to the behaviour of __alloc_skb_head().
Perhaps the core code should be fixed to make my assumption true?
> > A second problem is one that you raise which I had not considered
> > which is how to handle things if inner_protocol is already set.
> >
> > I believe this should only occur in the case where inner_protocol
> > is a field of struct sk_buff and I think it would be most convenient
> > to set it conditionally in ovs_skb_reset_inner_protocol().
> > I think that if it is not set it should be zero but it should be
> > safe to check for values less than ETH_P_802_3_MIN.
>
> It's probably OK to check for values less than ETH_P_802_3_MIN but I'm
> not sure that it's the most correct thing to do since skb->protocol
> could contain these values (such as ETH_P_802_2). It's unlikely that
> they will be GSO packets but it seems better to use the more strict
> check against zero.
Sure, a strict check against zero is fine my me.
> One other consideration in the OVS case - with recirculation we may
> hit this code multiple times and the difference in behavior could be
> surprising. However, on the other hand, we need to be careful because
> skb->cb is not guaranteed to be initialized to zero.
Thanks, that is also not something that I had considered.
I'm not sure, but I think that we can rely on skb->cb
not being clobbered between rounds of recirculation.
Or at the very least I think we could save and restore it
as necessary.
So I think if we could be careful to make sure that inner_protocol
is in a sane state the first time we see the skb but not
each time it is recirculated then I think things should work out.
In my current implementation of recirculation the datapath
side is driven ovs_dp_process_received_packet(). So by my reasoning
above I think it would make sense to reset the inner_protocol there
and in ovs_packet_cmd_execute() rather than in ovs_execute_actions()
which each of those functions call.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-19 17:31 ` Jesse Gross
@ 2013-09-22 5:38 ` Simon Horman
2013-09-23 19:47 ` Pravin Shelar
0 siblings, 1 reply; 38+ messages in thread
From: Simon Horman @ 2013-09-22 5:38 UTC (permalink / raw)
To: Jesse Gross
Cc: Pravin Shelar, dev, netdev, Ravi K, Isaku Yamahata, Joe Stringer
On Thu, Sep 19, 2013 at 12:31:51PM -0500, Jesse Gross wrote:
> On Wed, Sep 18, 2013 at 5:07 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Tue, Sep 17, 2013 at 11:38:18AM -0700, Pravin Shelar wrote:
> >> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > diff --git a/datapath/datapath.h b/datapath/datapath.h
> >> > index 5d50dd4..babae3b 100644
> >> > --- a/datapath/datapath.h
> >> > +++ b/datapath/datapath.h
> >> > @@ -36,6 +36,10 @@
> >> >
> >> > #define SAMPLE_ACTION_DEPTH 3
> >> >
> >> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
> >> > +#define HAVE_INNER_PROTOCOL
> >> > +#endif
> >> > +
> >> > /**
> >> > * struct dp_stats_percpu - per-cpu packet processing statistics for a given
> >> > * datapath.
> >> > @@ -93,11 +97,16 @@ struct datapath {
> >> > * @pkt_key: The flow information extracted from the packet. Must be nonnull.
> >> > * @tun_key: Key for the tunnel that encapsulated this packet. NULL if the
> >> > * packet is not being tunneled.
> >> > + * @inner_protocol: Provides a substitute for the skb->inner_protocol field on
> >> > + * kernels before 3.11.
> >> > */
> >> > struct ovs_skb_cb {
> >> > struct sw_flow *flow;
> >> > struct sw_flow_key *pkt_key;
> >> > struct ovs_key_ipv4_tunnel *tun_key;
> >> > +#ifndef HAVE_INNER_PROTOCOL
> >> > + __be16 inner_protocol;
> >> > +#endif
> >> > };
> >> > #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
> >> >
> >> Can you move this to compat struct ovs_gso_cb {}
> >
> > I think that you are correct and inner_protocol needs
> > to be in struct ovs_gso_cb so that it can
> > be accessed via skb_network_protocol() from
> > rpl___skb_gso_segment().
> >
> > However I think it may also need to be present in struct ovs_cb
> > so that it can be set correctly.
> >
> > Currently it is set unconditionally
> > in ovs_execute_actions() and Jesse has suggested setting
> > it conditionally in pop_mpls() (which is called by do_execute_actions()).
> > But regardless it seems to me that the field would need to be available
> > in struct ovs_cb.
>
> Since the helper functions are also in the compat code, I think they
> should have access to ovs_gso_cb.
Pravin also believes that is the case so I have moved inner_protocol
to struct ovs_gso_cb as he requested.
> >> I think we can simplify code by pushing vlan and then segmenting skb,
> >> the way we do it for MPLS.
> >
> > Are you thinking of something like the following which applies
> > prior to the MPLS code.
>
> This is basically what I was thinking about. We might actually be able
> to move all of this to compat code by having a replacement for
> dev_queue_xmit() similar to what we have for ip_local_out() in the
> tunnel code.
I would appreciate Pravin's opinion on this but it seems to me
that it should be possible.
The following applies on top of my previous proposed change
in this thread - simplification of segmentation - and before
the MPLS patch-set. Is this along the lines of what you were
thinking of?
From: Simon Horman <horms@verge.net.au>
datapath: Move segmentation compatibility code into a compatibility function
*** Do not apply: for informational purposes only
Move segmentation compatibility code out of netdev_send and into
rpl_dev_queue_xmit(), a compatibility function used in place
of dev_queue_xmit() as necessary.
As suggested by Jesse Gross.
Some minor though verbose implementation notes:
* This rpl_dev_queue_xmit() endeavours to return a valid error code or
zero on success as per dev_queue_xmit(). The exception is that when
dev_queue_xmit() is called in a loop only the status of the last call is
taken into account, thus ignoring any errors returned by previous calls.
This is derived from the previous calls to dev_queue_xmit() in a loop
where netdev_send() ignores the return value of dev_queue_xmit()
entirely.
* netdev_send() continues to ignore the value of dev_queue_xmit().
So the discussion of the return value of rpl_dev_queue_xmit()
above is has no bearing on run-time behaviour.
* The return value of netdev_send() may differ from the previous
implementation in the case where segmentation is performed before
calling the real dev_queue_xmit(). This is because previously in
this case netdev_send() would return the combined length of the
skbs resulting from segmentation. Whereas the current code
always returns the length of the original skb.
Compile tested only.
Signed-off-by: Simon Horman <horms@verge.net.au>
---
datapath/linux/compat/gso.c | 80 +++++++++++++++++++++++++++++++++++++++++++++
datapath/linux/compat/gso.h | 5 +++
datapath/vport-netdev.c | 78 ++-----------------------------------------
3 files changed, 87 insertions(+), 76 deletions(-)
diff --git a/datapath/linux/compat/gso.c b/datapath/linux/compat/gso.c
index 30332a2..d7e92fb 100644
--- a/datapath/linux/compat/gso.c
+++ b/datapath/linux/compat/gso.c
@@ -36,6 +36,86 @@
#include "gso.h"
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,37) && \
+ !defined(HAVE_VLAN_BUG_WORKAROUND)
+#include <linux/module.h>
+
+static int vlan_tso __read_mostly;
+module_param(vlan_tso, int, 0644);
+MODULE_PARM_DESC(vlan_tso, "Enable TSO for VLAN packets");
+#else
+#define vlan_tso true
+#endif
+
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,37)
+static bool dev_supports_vlan_tx(struct net_device *dev)
+{
+#if defined(HAVE_VLAN_BUG_WORKAROUND)
+ return dev->features & NETIF_F_HW_VLAN_TX;
+#else
+ /* Assume that the driver is buggy. */
+ return false;
+#endif
+}
+
+int rpl_dev_queue_xmit(struct sk_buff *skb)
+{
+#undef dev_queue_xmit
+ int err = -ENOMEM;
+
+ if (vlan_tx_tag_present(skb) && !dev_supports_vlan_tx(skb->dev)) {
+ int features;
+
+ features = netif_skb_features(skb);
+
+ if (!vlan_tso)
+ features &= ~(NETIF_F_TSO | NETIF_F_TSO6 |
+ NETIF_F_UFO | NETIF_F_FSO);
+
+ skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
+ if (unlikely(!skb))
+ return err;
+ vlan_set_tci(skb, 0);
+
+ if (netif_needs_gso(skb, features)) {
+ struct sk_buff *nskb;
+
+ nskb = skb_gso_segment(skb, features);
+ if (!nskb) {
+ if (unlikely(skb_cloned(skb) &&
+ pskb_expand_head(skb, 0, 0, GFP_ATOMIC)))
+ goto drop;
+
+ skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY;
+ goto xmit;
+ }
+
+ if (IS_ERR(nskb)) {
+ err = PTR_ERR(nskb);
+ goto drop;
+ }
+ consume_skb(skb);
+ skb = nskb;
+
+ do {
+ nskb = skb->next;
+ skb->next = NULL;
+ err = dev_queue_xmit(skb);
+ skb = nskb;
+ } while (skb);
+
+ return err;
+ }
+ }
+xmit:
+ return dev_queue_xmit(skb);
+
+drop:
+ kfree_skb(skb);
+ return err;
+}
+#endif /* kernel version < 3.6.37 */
+
static __be16 __skb_network_protocol(struct sk_buff *skb)
{
__be16 type = skb->protocol;
diff --git a/datapath/linux/compat/gso.h b/datapath/linux/compat/gso.h
index 44fd213..af1aaed 100644
--- a/datapath/linux/compat/gso.h
+++ b/datapath/linux/compat/gso.h
@@ -69,4 +69,9 @@ static inline void skb_reset_inner_headers(struct sk_buff *skb)
#define ip_local_out rpl_ip_local_out
int ip_local_out(struct sk_buff *skb);
+
+#if 1 // LINUX_VERSION_CODE < KERNEL_VERSION(2,6,37)
+#define dev_queue_xmit rpl_dev_queue_xmit
+#endif
+
#endif
diff --git a/datapath/vport-netdev.c b/datapath/vport-netdev.c
index 31680fd..4d934b5 100644
--- a/datapath/vport-netdev.c
+++ b/datapath/vport-netdev.c
@@ -34,17 +34,6 @@
#include "vport-internal_dev.h"
#include "vport-netdev.h"
-#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,37) && \
- !defined(HAVE_VLAN_BUG_WORKAROUND)
-#include <linux/module.h>
-
-static int vlan_tso __read_mostly;
-module_param(vlan_tso, int, 0644);
-MODULE_PARM_DESC(vlan_tso, "Enable TSO for VLAN packets");
-#else
-#define vlan_tso true
-#endif
-
static void netdev_port_receive(struct vport *vport, struct sk_buff *skb);
#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,39)
@@ -259,19 +248,6 @@ static unsigned int packet_length(const struct sk_buff *skb)
return length;
}
-static bool dev_supports_vlan_tx(struct net_device *dev)
-{
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,37)
- /* Software fallback means every device supports vlan_tci on TX. */
- return true;
-#elif defined(HAVE_VLAN_BUG_WORKAROUND)
- return dev->features & NETIF_F_HW_VLAN_TX;
-#else
- /* Assume that the driver is buggy. */
- return false;
-#endif
-}
-
static int netdev_send(struct vport *vport, struct sk_buff *skb)
{
struct netdev_vport *netdev_vport = netdev_vport_priv(vport);
@@ -282,65 +258,15 @@ static int netdev_send(struct vport *vport, struct sk_buff *skb)
net_warn_ratelimited("%s: dropped over-mtu packet: %d > %d\n",
netdev_vport->dev->name,
packet_length(skb), mtu);
- goto drop;
+ kfree_skb(skb);
+ return 0;
}
skb->dev = netdev_vport->dev;
-
- if (vlan_tx_tag_present(skb) && !dev_supports_vlan_tx(skb->dev)) {
- int features;
-
- features = netif_skb_features(skb);
-
- if (!vlan_tso)
- features &= ~(NETIF_F_TSO | NETIF_F_TSO6 |
- NETIF_F_UFO | NETIF_F_FSO);
-
- skb = __vlan_put_tag(skb, skb->vlan_proto, vlan_tx_tag_get(skb));
- if (unlikely(!skb))
- return 0;
- vlan_set_tci(skb, 0);
-
- if (netif_needs_gso(skb, features)) {
- struct sk_buff *nskb;
-
- nskb = skb_gso_segment(skb, features);
- if (!nskb) {
- if (unlikely(skb_cloned(skb) &&
- pskb_expand_head(skb, 0, 0, GFP_ATOMIC)))
- goto drop;
-
- skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY;
- goto xmit;
- }
-
- if (IS_ERR(nskb))
- goto drop;
- consume_skb(skb);
- skb = nskb;
-
- len = 0;
- do {
- nskb = skb->next;
- skb->next = NULL;
- len += skb->len;
- dev_queue_xmit(skb);
- skb = nskb;
- } while (skb);
-
- return len;
- }
- }
-
-xmit:
len = skb->len;
dev_queue_xmit(skb);
return len;
-
-drop:
- kfree_skb(skb);
- return 0;
}
/* Returns null if this device is not attached to a datapath. */
--
1.8.4
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-22 5:38 ` Simon Horman
@ 2013-09-23 19:47 ` Pravin Shelar
[not found] ` <CALnjE+o_bcNbU6kv2eUBaNS4tincV+BmrFyJtZZRTaMPMVv8gw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 38+ messages in thread
From: Pravin Shelar @ 2013-09-23 19:47 UTC (permalink / raw)
To: Simon Horman
Cc: Jesse Gross, dev, netdev, Ravi K, Isaku Yamahata, Joe Stringer
On Sat, Sep 21, 2013 at 10:38 PM, Simon Horman <horms@verge.net.au> wrote:
> On Thu, Sep 19, 2013 at 12:31:51PM -0500, Jesse Gross wrote:
>> On Wed, Sep 18, 2013 at 5:07 PM, Simon Horman <horms@verge.net.au> wrote:
>> > On Tue, Sep 17, 2013 at 11:38:18AM -0700, Pravin Shelar wrote:
>> >> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@verge.net.au> wrote:
>> >> > diff --git a/datapath/datapath.h b/datapath/datapath.h
>> >> > index 5d50dd4..babae3b 100644
>> >> > --- a/datapath/datapath.h
>> >> > +++ b/datapath/datapath.h
>> >> > @@ -36,6 +36,10 @@
>> >> >
>> >> > #define SAMPLE_ACTION_DEPTH 3
>> >> >
>> >> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
>> >> > +#define HAVE_INNER_PROTOCOL
>> >> > +#endif
>> >> > +
>> >> > /**
>> >> > * struct dp_stats_percpu - per-cpu packet processing statistics for a given
>> >> > * datapath.
>> >> > @@ -93,11 +97,16 @@ struct datapath {
>> >> > * @pkt_key: The flow information extracted from the packet. Must be nonnull.
>> >> > * @tun_key: Key for the tunnel that encapsulated this packet. NULL if the
>> >> > * packet is not being tunneled.
>> >> > + * @inner_protocol: Provides a substitute for the skb->inner_protocol field on
>> >> > + * kernels before 3.11.
>> >> > */
>> >> > struct ovs_skb_cb {
>> >> > struct sw_flow *flow;
>> >> > struct sw_flow_key *pkt_key;
>> >> > struct ovs_key_ipv4_tunnel *tun_key;
>> >> > +#ifndef HAVE_INNER_PROTOCOL
>> >> > + __be16 inner_protocol;
>> >> > +#endif
>> >> > };
>> >> > #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
>> >> >
>> >> Can you move this to compat struct ovs_gso_cb {}
>> >
>> > I think that you are correct and inner_protocol needs
>> > to be in struct ovs_gso_cb so that it can
>> > be accessed via skb_network_protocol() from
>> > rpl___skb_gso_segment().
>> >
>> > However I think it may also need to be present in struct ovs_cb
>> > so that it can be set correctly.
>> >
>> > Currently it is set unconditionally
>> > in ovs_execute_actions() and Jesse has suggested setting
>> > it conditionally in pop_mpls() (which is called by do_execute_actions()).
>> > But regardless it seems to me that the field would need to be available
>> > in struct ovs_cb.
>>
>> Since the helper functions are also in the compat code, I think they
>> should have access to ovs_gso_cb.
>
> Pravin also believes that is the case so I have moved inner_protocol
> to struct ovs_gso_cb as he requested.
>
>> >> I think we can simplify code by pushing vlan and then segmenting skb,
>> >> the way we do it for MPLS.
>> >
>> > Are you thinking of something like the following which applies
>> > prior to the MPLS code.
>>
>> This is basically what I was thinking about. We might actually be able
>> to move all of this to compat code by having a replacement for
>> dev_queue_xmit() similar to what we have for ip_local_out() in the
>> tunnel code.
>
> I would appreciate Pravin's opinion on this but it seems to me
> that it should be possible.
>
> The following applies on top of my previous proposed change
> in this thread - simplification of segmentation - and before
> the MPLS patch-set. Is this along the lines of what you were
> thinking of?
>
>
> From: Simon Horman <horms@verge.net.au>
>
> datapath: Move segmentation compatibility code into a compatibility function
>
> *** Do not apply: for informational purposes only
>
> Move segmentation compatibility code out of netdev_send and into
> rpl_dev_queue_xmit(), a compatibility function used in place
> of dev_queue_xmit() as necessary.
>
> As suggested by Jesse Gross.
>
> Some minor though verbose implementation notes:
>
> * This rpl_dev_queue_xmit() endeavours to return a valid error code or
> zero on success as per dev_queue_xmit(). The exception is that when
> dev_queue_xmit() is called in a loop only the status of the last call is
> taken into account, thus ignoring any errors returned by previous calls.
> This is derived from the previous calls to dev_queue_xmit() in a loop
> where netdev_send() ignores the return value of dev_queue_xmit()
> entirely.
>
> * netdev_send() continues to ignore the value of dev_queue_xmit().
> So the discussion of the return value of rpl_dev_queue_xmit()
> above is has no bearing on run-time behaviour.
>
> * The return value of netdev_send() may differ from the previous
> implementation in the case where segmentation is performed before
> calling the real dev_queue_xmit(). This is because previously in
> this case netdev_send() would return the combined length of the
> skbs resulting from segmentation. Whereas the current code
> always returns the length of the original skb.
This approach looks good to me.
>
> Compile tested only.
>
This patch does not work since vport-netdev does not include compat
gso header. after including gso.h it gives me compiler error.
Can you post combined patch with fixes?
It is better to move rpl_dev_queue_xmit definition from gso.h to
linux/netdevice.h.
Thanks.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
[not found] ` <20130922053156.GA4849-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
@ 2013-09-23 21:17 ` Jesse Gross
2013-09-24 1:32 ` Simon Horman
0 siblings, 1 reply; 38+ messages in thread
From: Jesse Gross @ 2013-09-23 21:17 UTC (permalink / raw)
To: Simon Horman; +Cc: dev-yBygre7rU0TnMu66kgdUjQ, Ravi K, netdev, Isaku Yamahata
On Sat, Sep 21, 2013 at 10:34 PM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
> On Thu, Sep 19, 2013 at 12:21:33PM -0500, Jesse Gross wrote:
>> On Thu, Sep 19, 2013 at 10:57 AM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
>> > On Mon, Sep 16, 2013 at 03:38:21PM -0500, Jesse Gross wrote:
>> >> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
>> >> > @@ -616,6 +736,13 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb)
>> >> > goto out_loop;
>> >> > }
>> >> >
>> >> > + /* Needed to initialise inner protocol on kernels older
>> >> > + * than v3.11 where skb->inner_protocol is not present
>> >> > + * and compatibility code uses the OVS_CB(skb) to store
>> >> > + * the inner protocol.
>> >> > + */
>> >> > + ovs_skb_set_inner_protocol(skb, skb->protocol);
>> >>
>> >> The comment makes it sound like this code should just be deleted when
>> >> upstreaming. However, I believe that we still need to initialize this
>> >> field, right? Is this the best place do it or should it be conditional
>> >> on adding a first MPLS header? (i.e. what happens if inner_protocol is
>> >> already set and the packet simply passes through OVS?)
>> >
>> > I believe there are several problems here.
>> >
>> > The first one, which my comment was written around is that I think that if
>> > inner_protocol is a field of struct sk_buff then we can rely on it already
>> > being initialised. However, if we are using compatibility code, where
>> > inner_protcol is called in the callback field of struct sk_buff then I
>> > think that OVS needs to initialise it.
>>
>> I'm not sure that it's true that inner_protocol is already initialized
>> - I grepped the tree and the only assignment that I found is in
>> skbuff.c in __copy_skb_header().
>
> My assumption was that it would be initialised to zero,
> primarily due to the behaviour of __alloc_skb_head().
> Perhaps the core code should be fixed to make my assumption true?
I misunderstood then - I think you can assume that it is initialized
to zero, I though you meant that it was initialized to a protocol
value. However, I then still have my original question - don't we need
to set it here in both cases since we're not just 'initializing' it
but actually setting a protocol?
>> One other consideration in the OVS case - with recirculation we may
>> hit this code multiple times and the difference in behavior could be
>> surprising. However, on the other hand, we need to be careful because
>> skb->cb is not guaranteed to be initialized to zero.
>
> Thanks, that is also not something that I had considered.
>
> I'm not sure, but I think that we can rely on skb->cb
> not being clobbered between rounds of recirculation.
> Or at the very least I think we could save and restore it
> as necessary.
Yes, it should be safe to assume this.
> So I think if we could be careful to make sure that inner_protocol
> is in a sane state the first time we see the skb but not
> each time it is recirculated then I think things should work out.
>
> In my current implementation of recirculation the datapath
> side is driven ovs_dp_process_received_packet(). So by my reasoning
> above I think it would make sense to reset the inner_protocol there
> and in ovs_packet_cmd_execute() rather than in ovs_execute_actions()
> which each of those functions call.
I think that would work, however, I wonder if it's the right place in
general, independent of this compatibility issue. I guess it still
seems like the ideal thing to do is to move this as close to where it
is necessary as possible, specifically in mpls_push(). Is there a
reason to not put it there (again, other than the out-of-tree
compatibility issues)?
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
[not found] ` <CALnjE+o_bcNbU6kv2eUBaNS4tincV+BmrFyJtZZRTaMPMVv8gw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-09-23 21:24 ` Jesse Gross
2013-09-24 1:33 ` Simon Horman
0 siblings, 1 reply; 38+ messages in thread
From: Jesse Gross @ 2013-09-23 21:24 UTC (permalink / raw)
To: Pravin Shelar; +Cc: dev-yBygre7rU0TnMu66kgdUjQ, Ravi K, netdev, Isaku Yamahata
On Mon, Sep 23, 2013 at 12:47 PM, Pravin Shelar <pshelar-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org> wrote:
> This patch does not work since vport-netdev does not include compat
> gso header. after including gso.h it gives me compiler error.
> Can you post combined patch with fixes?
I think it's probably because of this:
+#if 1 // LINUX_VERSION_CODE < KERNEL_VERSION(2,6,37)
+#define dev_queue_xmit rpl_dev_queue_xmit
+#endif
But otherwise, I agree the approach is much nicer than what is currently there.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-23 21:17 ` Jesse Gross
@ 2013-09-24 1:32 ` Simon Horman
2013-09-24 1:38 ` Jesse Gross
0 siblings, 1 reply; 38+ messages in thread
From: Simon Horman @ 2013-09-24 1:32 UTC (permalink / raw)
To: Jesse Gross
Cc: Ben Pfaff, Pravin Shelar, dev, netdev, Ravi K, Isaku Yamahata,
Joe Stringer
On Mon, Sep 23, 2013 at 02:17:50PM -0700, Jesse Gross wrote:
> On Sat, Sep 21, 2013 at 10:34 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Thu, Sep 19, 2013 at 12:21:33PM -0500, Jesse Gross wrote:
> >> On Thu, Sep 19, 2013 at 10:57 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > On Mon, Sep 16, 2013 at 03:38:21PM -0500, Jesse Gross wrote:
> >> >> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@verge.net.au> wrote:
> >> >> > @@ -616,6 +736,13 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb)
> >> >> > goto out_loop;
> >> >> > }
> >> >> >
> >> >> > + /* Needed to initialise inner protocol on kernels older
> >> >> > + * than v3.11 where skb->inner_protocol is not present
> >> >> > + * and compatibility code uses the OVS_CB(skb) to store
> >> >> > + * the inner protocol.
> >> >> > + */
> >> >> > + ovs_skb_set_inner_protocol(skb, skb->protocol);
> >> >>
> >> >> The comment makes it sound like this code should just be deleted when
> >> >> upstreaming. However, I believe that we still need to initialize this
> >> >> field, right? Is this the best place do it or should it be conditional
> >> >> on adding a first MPLS header? (i.e. what happens if inner_protocol is
> >> >> already set and the packet simply passes through OVS?)
> >> >
> >> > I believe there are several problems here.
> >> >
> >> > The first one, which my comment was written around is that I think that if
> >> > inner_protocol is a field of struct sk_buff then we can rely on it already
> >> > being initialised. However, if we are using compatibility code, where
> >> > inner_protcol is called in the callback field of struct sk_buff then I
> >> > think that OVS needs to initialise it.
> >>
> >> I'm not sure that it's true that inner_protocol is already initialized
> >> - I grepped the tree and the only assignment that I found is in
> >> skbuff.c in __copy_skb_header().
> >
> > My assumption was that it would be initialised to zero,
> > primarily due to the behaviour of __alloc_skb_head().
> > Perhaps the core code should be fixed to make my assumption true?
>
> I misunderstood then - I think you can assume that it is initialized
> to zero, I though you meant that it was initialized to a protocol
> value. However, I then still have my original question - don't we need
> to set it here in both cases since we're not just 'initializing' it
> but actually setting a protocol?
I believe that you are correct and it needs to be set in both cases.
> >> One other consideration in the OVS case - with recirculation we may
> >> hit this code multiple times and the difference in behavior could be
> >> surprising. However, on the other hand, we need to be careful because
> >> skb->cb is not guaranteed to be initialized to zero.
> >
> > Thanks, that is also not something that I had considered.
> >
> > I'm not sure, but I think that we can rely on skb->cb
> > not being clobbered between rounds of recirculation.
> > Or at the very least I think we could save and restore it
> > as necessary.
>
> Yes, it should be safe to assume this.
>
> > So I think if we could be careful to make sure that inner_protocol
> > is in a sane state the first time we see the skb but not
> > each time it is recirculated then I think things should work out.
> >
> > In my current implementation of recirculation the datapath
> > side is driven ovs_dp_process_received_packet(). So by my reasoning
> > above I think it would make sense to reset the inner_protocol there
> > and in ovs_packet_cmd_execute() rather than in ovs_execute_actions()
> > which each of those functions call.
>
> I think that would work, however, I wonder if it's the right place in
> general, independent of this compatibility issue. I guess it still
> seems like the ideal thing to do is to move this as close to where it
> is necessary as possible, specifically in mpls_push(). Is there a
> reason to not put it there (again, other than the out-of-tree
> compatibility issues)?
I agree that should work, out-of-tree compatibility issues aside.
Perhaps a solution is to have a conditional set_inner_protocol call inside
push_mpls, where the condition is that inner_protocol is zero.
And a reset_inner_protocol call earlier on, a call that sets inner_protocol
to zero only if the compatibility code is in use and thus it resides in
struct ovs_gso_cb. This call could be remove once the compatibility
code is no longer needed, that is once kernels older than 3.11 are no
longer supported.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-23 21:24 ` Jesse Gross
@ 2013-09-24 1:33 ` Simon Horman
0 siblings, 0 replies; 38+ messages in thread
From: Simon Horman @ 2013-09-24 1:33 UTC (permalink / raw)
To: Jesse Gross
Cc: Pravin Shelar, dev, netdev, Ravi K, Isaku Yamahata, Joe Stringer
On Mon, Sep 23, 2013 at 02:24:31PM -0700, Jesse Gross wrote:
> On Mon, Sep 23, 2013 at 12:47 PM, Pravin Shelar <pshelar@nicira.com> wrote:
> > This patch does not work since vport-netdev does not include compat
> > gso header. after including gso.h it gives me compiler error.
> > Can you post combined patch with fixes?
>
> I think it's probably because of this:
>
> +#if 1 // LINUX_VERSION_CODE < KERNEL_VERSION(2,6,37)
> +#define dev_queue_xmit rpl_dev_queue_xmit
> +#endif
>
> But otherwise, I agree the approach is much nicer than what is currently there.
Sorry for letting that slip through.
I'll post a more complete patch after doing some more testing.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-24 1:32 ` Simon Horman
@ 2013-09-24 1:38 ` Jesse Gross
2013-09-24 2:49 ` Simon Horman
0 siblings, 1 reply; 38+ messages in thread
From: Jesse Gross @ 2013-09-24 1:38 UTC (permalink / raw)
To: Simon Horman
Cc: Ben Pfaff, Pravin Shelar, dev, netdev, Ravi K, Isaku Yamahata,
Joe Stringer
On Mon, Sep 23, 2013 at 6:32 PM, Simon Horman <horms@verge.net.au> wrote:
> On Mon, Sep 23, 2013 at 02:17:50PM -0700, Jesse Gross wrote:
>> On Sat, Sep 21, 2013 at 10:34 PM, Simon Horman <horms@verge.net.au> wrote:
>> > On Thu, Sep 19, 2013 at 12:21:33PM -0500, Jesse Gross wrote:
>> >> On Thu, Sep 19, 2013 at 10:57 AM, Simon Horman <horms@verge.net.au> wrote:
>> >> > On Mon, Sep 16, 2013 at 03:38:21PM -0500, Jesse Gross wrote:
>> >> >> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@verge.net.au> wrote:
>> >> One other consideration in the OVS case - with recirculation we may
>> >> hit this code multiple times and the difference in behavior could be
>> >> surprising. However, on the other hand, we need to be careful because
>> >> skb->cb is not guaranteed to be initialized to zero.
>> >
>> > Thanks, that is also not something that I had considered.
>> >
>> > I'm not sure, but I think that we can rely on skb->cb
>> > not being clobbered between rounds of recirculation.
>> > Or at the very least I think we could save and restore it
>> > as necessary.
>>
>> Yes, it should be safe to assume this.
>>
>> > So I think if we could be careful to make sure that inner_protocol
>> > is in a sane state the first time we see the skb but not
>> > each time it is recirculated then I think things should work out.
>> >
>> > In my current implementation of recirculation the datapath
>> > side is driven ovs_dp_process_received_packet(). So by my reasoning
>> > above I think it would make sense to reset the inner_protocol there
>> > and in ovs_packet_cmd_execute() rather than in ovs_execute_actions()
>> > which each of those functions call.
>>
>> I think that would work, however, I wonder if it's the right place in
>> general, independent of this compatibility issue. I guess it still
>> seems like the ideal thing to do is to move this as close to where it
>> is necessary as possible, specifically in mpls_push(). Is there a
>> reason to not put it there (again, other than the out-of-tree
>> compatibility issues)?
>
> I agree that should work, out-of-tree compatibility issues aside.
>
> Perhaps a solution is to have a conditional set_inner_protocol call inside
> push_mpls, where the condition is that inner_protocol is zero.
> And a reset_inner_protocol call earlier on, a call that sets inner_protocol
> to zero only if the compatibility code is in use and thus it resides in
> struct ovs_gso_cb. This call could be remove once the compatibility
> code is no longer needed, that is once kernels older than 3.11 are no
> longer supported.
I agree that's probably the right solution.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel
2013-09-24 1:38 ` Jesse Gross
@ 2013-09-24 2:49 ` Simon Horman
0 siblings, 0 replies; 38+ messages in thread
From: Simon Horman @ 2013-09-24 2:49 UTC (permalink / raw)
To: Jesse Gross
Cc: Ben Pfaff, Pravin Shelar, dev, netdev, Ravi K, Isaku Yamahata,
Joe Stringer
On Mon, Sep 23, 2013 at 06:38:23PM -0700, Jesse Gross wrote:
> On Mon, Sep 23, 2013 at 6:32 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Mon, Sep 23, 2013 at 02:17:50PM -0700, Jesse Gross wrote:
> >> On Sat, Sep 21, 2013 at 10:34 PM, Simon Horman <horms@verge.net.au> wrote:
> >> > On Thu, Sep 19, 2013 at 12:21:33PM -0500, Jesse Gross wrote:
> >> >> On Thu, Sep 19, 2013 at 10:57 AM, Simon Horman <horms@verge.net.au> wrote:
> >> >> > On Mon, Sep 16, 2013 at 03:38:21PM -0500, Jesse Gross wrote:
> >> >> >> On Mon, Sep 9, 2013 at 12:20 AM, Simon Horman <horms@verge.net.au> wrote:
> >> >> One other consideration in the OVS case - with recirculation we may
> >> >> hit this code multiple times and the difference in behavior could be
> >> >> surprising. However, on the other hand, we need to be careful because
> >> >> skb->cb is not guaranteed to be initialized to zero.
> >> >
> >> > Thanks, that is also not something that I had considered.
> >> >
> >> > I'm not sure, but I think that we can rely on skb->cb
> >> > not being clobbered between rounds of recirculation.
> >> > Or at the very least I think we could save and restore it
> >> > as necessary.
> >>
> >> Yes, it should be safe to assume this.
> >>
> >> > So I think if we could be careful to make sure that inner_protocol
> >> > is in a sane state the first time we see the skb but not
> >> > each time it is recirculated then I think things should work out.
> >> >
> >> > In my current implementation of recirculation the datapath
> >> > side is driven ovs_dp_process_received_packet(). So by my reasoning
> >> > above I think it would make sense to reset the inner_protocol there
> >> > and in ovs_packet_cmd_execute() rather than in ovs_execute_actions()
> >> > which each of those functions call.
> >>
> >> I think that would work, however, I wonder if it's the right place in
> >> general, independent of this compatibility issue. I guess it still
> >> seems like the ideal thing to do is to move this as close to where it
> >> is necessary as possible, specifically in mpls_push(). Is there a
> >> reason to not put it there (again, other than the out-of-tree
> >> compatibility issues)?
> >
> > I agree that should work, out-of-tree compatibility issues aside.
> >
> > Perhaps a solution is to have a conditional set_inner_protocol call inside
> > push_mpls, where the condition is that inner_protocol is zero.
> > And a reset_inner_protocol call earlier on, a call that sets inner_protocol
> > to zero only if the compatibility code is in use and thus it resides in
> > struct ovs_gso_cb. This call could be remove once the compatibility
> > code is no longer needed, that is once kernels older than 3.11 are no
> > longer supported.
>
> I agree that's probably the right solution.
Thanks, I will see about making it so.
^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2013-09-24 2:49 UTC | newest]
Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-09 7:20 [PATCH v2.39 0/7] MPLS actions and matches Simon Horman
[not found] ` <1378711207-29890-1-git-send-email-horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
2013-09-09 7:20 ` [PATCH v2.39 1/7] odp: Only pass vlan_tci to commit_vlan_action() Simon Horman
2013-09-09 7:20 ` [PATCH v2.39 3/7] ofp-actions: Add OFPUTIL_OFPAT13_PUSH_MPLS Simon Horman
2013-09-09 7:20 ` [PATCH v2.39 2/7] odp: Allow VLAN actions after MPLS actions Simon Horman
2013-09-09 7:20 ` [PATCH v2.39 4/7] ofp-actions: Add separate OpenFlow 1.3 action parser Simon Horman
2013-09-09 7:20 ` [PATCH v2.39 5/7] lib: Push MPLS tags in the OpenFlow 1.3 ordering Simon Horman
2013-09-09 7:20 ` [PATCH v2.39 6/7] datapath: Break out deacceleration portion of vlan_push Simon Horman
2013-09-13 22:07 ` Jesse Gross
[not found] ` <CAEP_g=_ZQ6hNpxoHm6t3N=PxA+3WTrvNegL514j0R3GDM5C92A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-09-15 16:32 ` Simon Horman
2013-09-19 14:56 ` Simon Horman
2013-09-09 7:20 ` [PATCH v2.39 7/7] datapath: Add basic MPLS support to kernel Simon Horman
[not found] ` <1378711207-29890-8-git-send-email-horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
2013-09-16 20:38 ` Jesse Gross
[not found] ` <CAEP_g=8FBQPZ=C6G39YdHRzG57m5MqfXSZFAX2S_KLHRwfzc2w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-09-16 20:46 ` Ben Pfaff
2013-09-17 23:47 ` Simon Horman
2013-09-18 0:05 ` Ben Pfaff
2013-09-19 15:57 ` Simon Horman
2013-09-19 17:21 ` Jesse Gross
2013-09-22 5:34 ` Simon Horman
[not found] ` <20130922053156.GA4849-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
2013-09-23 21:17 ` Jesse Gross
2013-09-24 1:32 ` Simon Horman
2013-09-24 1:38 ` Jesse Gross
2013-09-24 2:49 ` Simon Horman
2013-09-17 18:38 ` Pravin Shelar
2013-09-18 22:07 ` Simon Horman
2013-09-19 17:31 ` Jesse Gross
2013-09-22 5:38 ` Simon Horman
2013-09-23 19:47 ` Pravin Shelar
[not found] ` <CALnjE+o_bcNbU6kv2eUBaNS4tincV+BmrFyJtZZRTaMPMVv8gw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-09-23 21:24 ` Jesse Gross
2013-09-24 1:33 ` Simon Horman
[not found] ` <20130918220756.GA24919-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
2013-09-19 20:45 ` Simon Horman
2013-09-20 1:31 ` [ovs-dev] " Pravin Shelar
2013-09-22 3:54 ` Simon Horman
2013-09-09 7:25 ` [PATCH v2.39 0/7] MPLS actions and matches Simon Horman
2013-09-12 19:06 ` [ovs-dev] " Ben Pfaff
[not found] ` <20130912190636.GL16044-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
2013-09-12 22:56 ` Simon Horman
[not found] ` <20130912225614.GB30229-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
2013-09-12 22:54 ` Ben Pfaff
2013-09-13 22:15 ` [ovs-dev] " Jesse Gross
2013-09-15 16:39 ` Simon Horman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.