[ovs-dev] [PATCH] datapath: Simplify ODPAT_SET_DL_TCI action.
Ben Pfaff
blp at nicira.com
Tue Oct 12 18:40:16 UTC 2010
There's no need to have a mask in this action, because both parts of the
TCI are part of the flow structure.
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
So far, this is oompile-tested only, so there could be some really
stupid things in it.
datapath/actions.c | 3 +--
datapath/datapath.c | 8 +-------
include/openvswitch/datapath-protocol.h | 8 +++-----
lib/dpif-netdev.c | 26 ++++++++------------------
lib/odp-util.c | 24 ++++--------------------
lib/ofp-util.c | 18 ++++++++++++++++++
ofproto/ofproto-sflow.c | 10 ++++------
ofproto/ofproto.c | 9 ++++-----
vswitchd/bridge.c | 2 +-
9 files changed, 44 insertions(+), 64 deletions(-)
diff --git a/datapath/actions.c b/datapath/actions.c
index 5365278..5904c83 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -74,7 +74,6 @@ static struct sk_buff *modify_vlan_tci(struct datapath *dp, struct sk_buff *skb,
const struct odp_flow_key *key,
const union odp_action *a, int n_actions)
{
- __be16 mask = a->dl_tci.mask;
__be16 tci = a->dl_tci.tci;
skb = make_writable(skb, VLAN_HLEN);
@@ -92,7 +91,7 @@ static struct sk_buff *modify_vlan_tci(struct datapath *dp, struct sk_buff *skb,
vh = vlan_eth_hdr(skb);
old_tci = vh->h_vlan_TCI;
- vh->h_vlan_TCI = (vh->h_vlan_TCI & ~mask) | tci;
+ vh->h_vlan_TCI = tci;
if (OVS_CB(skb)->ip_summed == OVS_CSUM_COMPLETE) {
__be16 diff[] = { ~old_tci, vh->h_vlan_TCI };
diff --git a/datapath/datapath.c b/datapath/datapath.c
index 9eeb28c..b41110d 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -904,7 +904,6 @@ static int validate_actions(const struct sw_flow_actions *actions)
for (i = 0; i < actions->n_actions; i++) {
const union odp_action *a = &actions->actions[i];
- __be16 mask;
switch (a->type) {
case ODPAT_CONTROLLER:
@@ -928,12 +927,7 @@ static int validate_actions(const struct sw_flow_actions *actions)
break;
case ODPAT_SET_DL_TCI:
- mask = a->dl_tci.mask;
- if (mask != htons(VLAN_VID_MASK) &&
- mask != htons(VLAN_PCP_MASK) &&
- mask != htons(VLAN_VID_MASK | VLAN_PCP_MASK))
- return -EINVAL;
- if (a->dl_tci.tci & ~mask)
+ if (a->dl_tci.tci & htons(VLAN_CFI_MASK))
return -EINVAL;
break;
diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h
index 1aa8066..b0e9dfb 100644
--- a/include/openvswitch/datapath-protocol.h
+++ b/include/openvswitch/datapath-protocol.h
@@ -258,7 +258,7 @@ struct odp_flowvec {
/* Action types. */
#define ODPAT_OUTPUT 0 /* Output to switch port. */
#define ODPAT_CONTROLLER 2 /* Send copy to controller. */
-#define ODPAT_SET_DL_TCI 3 /* Set the 802.1q VLAN VID and/or PCP. */
+#define ODPAT_SET_DL_TCI 3 /* Set the 802.1q TCI value. */
#define ODPAT_STRIP_VLAN 5 /* Strip the 802.1q header. */
#define ODPAT_SET_DL_SRC 6 /* Ethernet source address. */
#define ODPAT_SET_DL_DST 7 /* Ethernet destination address. */
@@ -295,10 +295,8 @@ struct odp_action_tunnel {
/* Action structure for ODPAT_SET_DL_TCI. */
struct odp_action_dl_tci {
uint16_t type; /* ODPAT_SET_DL_TCI. */
- ovs_be16 tci; /* New TCI. Bits not in mask must be zero. */
- ovs_be16 mask; /* 0x0fff to set VID, 0xe000 to set PCP,
- * or 0xefff to set both. */
- uint16_t reserved;
+ ovs_be16 tci; /* New TCI. CFI bit must be zero. */
+ uint32_t reserved;
};
/* Action structure for ODPAT_SET_DL_SRC/DST. */
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index aa4fdf8..3df6598 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -667,12 +667,7 @@ dpif_netdev_validate_actions(const union odp_action *actions, int n_actions,
case ODPAT_SET_DL_TCI:
*mutates = true;
- if (a->dl_tci.mask != htons(VLAN_VID_MASK)
- && a->dl_tci.mask != htons(VLAN_PCP_MASK)
- && a->dl_tci.mask != htons(VLAN_VID_MASK | VLAN_PCP_MASK)) {
- return EINVAL;
- }
- if (a->dl_tci.tci & ~a->dl_tci.mask){
+ if (a->dl_tci.tci & htons(VLAN_CFI)) {
return EINVAL;
}
break;
@@ -1011,16 +1006,12 @@ dp_netdev_wait(void)
}
-/* Modify the TCI field of 'packet'. If a VLAN tag is not present, one is
- * added with the TCI field set to 'a->tci'. If a VLAN tag is present, then
- * 'a->mask' bits are cleared before 'a->tci' is logically OR'd into the TCI
- * field.
- *
- * Note that the function does not ensure that 'a->tci' does not affect bits
- * outside of 'a->mask'.
+/* Modify the TCI field of 'packet'. If a VLAN tag is present, its TCI field
+ * is replaced by 'tci'. If a VLAN tag is not present, one is added with the
+ * TCI field set to 'tci'.
*/
static void
-dp_netdev_set_dl_tci(struct ofpbuf *packet, const struct odp_action_dl_tci *a)
+dp_netdev_set_dl_tci(struct ofpbuf *packet, uint16_t tci)
{
struct vlan_eth_header *veh;
struct eth_header *eh;
@@ -1028,16 +1019,15 @@ dp_netdev_set_dl_tci(struct ofpbuf *packet, const struct odp_action_dl_tci *a)
eh = packet->l2;
if (packet->size >= sizeof(struct vlan_eth_header)
&& eh->eth_type == htons(ETH_TYPE_VLAN)) {
- /* Clear 'mask' bits, but maintain other TCI bits. */
veh = packet->l2;
- veh->veth_tci = (veh->veth_tci & ~a->mask) | a->tci;
+ veh->veth_tci = tci;
} else {
/* Insert new 802.1Q header. */
struct vlan_eth_header tmp;
memcpy(tmp.veth_dst, eh->eth_dst, ETH_ADDR_LEN);
memcpy(tmp.veth_src, eh->eth_src, ETH_ADDR_LEN);
tmp.veth_type = htons(ETH_TYPE_VLAN);
- tmp.veth_tci = htons(a->tci);
+ tmp.veth_tci = tci;
tmp.veth_next_type = eh->eth_type;
veh = ofpbuf_push_uninit(packet, VLAN_HEADER_LEN);
@@ -1235,7 +1225,7 @@ dp_netdev_execute_actions(struct dp_netdev *dp,
break;
case ODPAT_SET_DL_TCI:
- dp_netdev_set_dl_tci(packet, &a->dl_tci);
+ dp_netdev_set_dl_tci(packet, a->dl_tci.tci);
break;
case ODPAT_STRIP_VLAN:
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 130d55d..511ec3a 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -71,26 +71,10 @@ format_odp_action(struct ds *ds, const union odp_action *a)
case ODPAT_SET_TUNNEL:
ds_put_format(ds, "set_tunnel(0x%08"PRIx32")", ntohl(a->tunnel.tun_id));
break;
- case ODPAT_SET_DL_TCI: {
- int vid = vlan_tci_to_vid(a->dl_tci.tci);
- int pcp = vlan_tci_to_pcp(a->dl_tci.tci);
-
- switch (ntohs(a->dl_tci.mask)) {
- case VLAN_VID_MASK:
- ds_put_format(ds, "set_tci(vlan=%d)", vid);
- break;
- case VLAN_PCP_MASK:
- ds_put_format(ds, "set_tci(pcp=%d)", pcp);
- break;
- case VLAN_VID_MASK | VLAN_PCP_MASK:
- ds_put_format(ds, "set_tci(vlan=%d,pcp=%d)", vid, pcp);
- break;
- default:
- ds_put_format(ds, "set_tci(tci=%04"PRIx16",mask=%04"PRIx16")",
- ntohs(a->dl_tci.tci), ntohs(a->dl_tci.mask));
- break;
- }
- }
+ case ODPAT_SET_DL_TCI:
+ ds_put_format(ds, "set_tci(vid=%"PRIu16",pcp=%d)",
+ vlan_tci_to_vid(a->dl_tci.tci),
+ vlan_tci_to_pcp(a->dl_tci.tci));
break;
case ODPAT_STRIP_VLAN:
ds_put_format(ds, "strip_vlan");
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index d8d3ced..99e5943 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -587,7 +587,25 @@ check_action(const union ofp_action *a, unsigned int len, int max_ports)
return check_output_port(ntohs(a->output.port), max_ports);
case OFPAT_SET_VLAN_VID:
+ error = check_action_exact_len(a, len, 8);
+ if (error) {
+ return error;
+ }
+ if (a->vlan_vid.vlan_vid & ~htons(0xfff)) {
+ return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_ARGUMENT);
+ }
+ return 0;
+
case OFPAT_SET_VLAN_PCP:
+ error = check_action_exact_len(a, len, 8);
+ if (error) {
+ return error;
+ }
+ if (a->vlan_vid.vlan_vid & ~7) {
+ return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_ARGUMENT);
+ }
+ return 0;
+
case OFPAT_STRIP_VLAN:
case OFPAT_SET_NW_SRC:
case OFPAT_SET_NW_DST:
diff --git a/ofproto/ofproto-sflow.c b/ofproto/ofproto-sflow.c
index de9bf89..784e552 100644
--- a/ofproto/ofproto-sflow.c
+++ b/ofproto/ofproto-sflow.c
@@ -567,6 +567,7 @@ ofproto_sflow_received(struct ofproto_sflow *os, struct odp_msg *msg)
n_outputs = 0;
for (i = 0; i < n_actions; i++) {
const union odp_action *a = &actions[i];
+ uint16_t tci;
switch (a->type) {
case ODPAT_OUTPUT:
@@ -575,12 +576,9 @@ ofproto_sflow_received(struct ofproto_sflow *os, struct odp_msg *msg)
break;
case ODPAT_SET_DL_TCI:
- if (a->dl_tci.mask & htons(VLAN_VID_MASK)) {
- switchElem.flowType.sw.dst_vlan = vlan_tci_to_vid(a->dl_tci.tci);
- }
- if (a->dl_tci.mask & htons(VLAN_PCP_MASK)) {
- switchElem.flowType.sw.dst_priority = vlan_tci_to_pcp(a->dl_tci.tci);
- }
+ tci = a->dl_tci.tci;
+ switchElem.flowType.sw.dst_vlan = vlan_tci_to_vid(tci);
+ switchElem.flowType.sw.dst_priority = vlan_tci_to_pcp(tci);
break;
default:
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index e25ce28..b4e647f 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2758,16 +2758,15 @@ do_xlate_actions(const union ofp_action *in, size_t n_in,
case OFPAT_SET_VLAN_VID:
oa = odp_actions_add(ctx->out, ODPAT_SET_DL_TCI);
- oa->dl_tci.tci = ia->vlan_vid.vlan_vid & htons(VLAN_VID_MASK);
- oa->dl_tci.mask = htons(VLAN_VID_MASK);
+ oa->dl_tci.tci = ia->vlan_vid.vlan_vid;
+ oa->dl_tci.tci |= htons(ctx->flow.dl_vlan_pcp << VLAN_PCP_SHIFT);
ctx->flow.dl_vlan = ia->vlan_vid.vlan_vid;
break;
case OFPAT_SET_VLAN_PCP:
oa = odp_actions_add(ctx->out, ODPAT_SET_DL_TCI);
- oa->dl_tci.tci = htons((ia->vlan_pcp.vlan_pcp << VLAN_PCP_SHIFT)
- & VLAN_PCP_MASK);
- oa->dl_tci.mask = htons(VLAN_PCP_MASK);
+ oa->dl_tci.tci = htons(ia->vlan_pcp.vlan_pcp << VLAN_PCP_SHIFT);
+ oa->dl_tci.tci |= ctx->flow.dl_vlan;
ctx->flow.dl_vlan_pcp = ia->vlan_pcp.vlan_pcp;
break;
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 74ac87e..c67bca4 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -2314,7 +2314,7 @@ compose_actions(struct bridge *br, const struct flow *flow, uint16_t vlan,
} else {
a = odp_actions_add(actions, ODPAT_SET_DL_TCI);
a->dl_tci.tci = htons(p->vlan & VLAN_VID_MASK);
- a->dl_tci.mask = htons(VLAN_VID_MASK);
+ a->dl_tci.tci |= htons(flow->dl_vlan_pcp << VLAN_PCP_SHIFT);
}
cur_vlan = p->vlan;
}
--
1.7.1
More information about the dev
mailing list