[ovs-dev] [OpenFlow 0.9 1/7] ofproto: Match VLAN PCP and rewrite ToS bits (OpenFlow 0.9)
Ben Pfaff
blp at nicira.com
Thu Nov 19 17:25:07 UTC 2009
Looks good, some comments below:
Justin Pettit <jpettit at nicira.com> writes:
> @@ -252,6 +252,30 @@ static struct sk_buff *set_nw_addr(struct sk_buff *skb,
> return skb;
> }
>
> +static struct sk_buff *set_nw_tos(struct sk_buff *skb,
> + struct odp_flow_key *key,
> + const struct odp_action_nw_tos *a,
> + gfp_t gfp)
> +{
> + if (key->dl_type != htons(ETH_P_IP))
> + return skb;
> +
> + skb = make_writable(skb, 0, gfp);
> + if (skb) {
> + struct iphdr *nh = ip_hdr(skb);
> + u8 *f = &nh->tos;
> + u8 old = *f;
> +
> + /* We only set the lower 6 bits. */
> + u8 new = (a->nw_tos & 0x3f) | (nh->tos & 0xc0);
> +
> + update_csum(&nh->check, skb, htons((uint16_t)old),
> + htons((uint16_t)new), 0);
The casts to uint16_t can be omitted because the htons parameter
is that type anyhow. (If you want to keep the casts for some
reason then the type should be u16 since this is kernel code.)
Does update_csum() work for 8-bit quantities like this? It
expects 16-bit quantities but the math is tricky and some things
like this actually work. I always have to think through it
again, and usually I end up writing down little equations on
paper and so on.
> + *f = new;
> + }
> + return skb;
> +}
> +
> static struct sk_buff *
> set_tp_port(struct sk_buff *skb, struct odp_flow_key *key,
> const struct odp_action_tp_port *a,
> @@ -424,6 +448,10 @@ int execute_actions(struct datapath *dp, struct sk_buff *skb,
> skb = set_nw_addr(skb, key, &a->nw_addr, gfp);
> break;
>
> + case ODPAT_SET_NW_TOS:
> + skb = set_nw_tos(skb, key, &a->nw_tos, gfp);
> + break;
I think it would be better to just pass a->nw_tos.nw_tos instead
of the address. In most of the other cases we pass an address
because the function handles two types of actions (set source,
set dest) but I can't think of anything analogous for nw_tos.
> case ODPAT_SET_TP_SRC:
> case ODPAT_SET_TP_DST:
> skb = set_tp_port(skb, key, &a->tp_port, gfp);
> diff --git a/datapath/flow.c b/datapath/flow.c
> index b2de023..f470f3d 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -231,6 +231,7 @@ int flow_extract(struct sk_buff *skb, u16 in_port, struct odp_flow_key *key)
> struct vlan_hdr *vh = (struct vlan_hdr*)(skb->data + nh_ofs);
> key->dl_type = vh->h_vlan_encapsulated_proto;
> key->dl_vlan = vh->h_vlan_TCI & htons(VLAN_VID_MASK);
> + key->dl_vlan_pcp = (ntohs(vh->h_vlan_TCI) & 0xe000) >> 13;
We have VLAN_PCP_MASK and VLAN_PCP_SHIFT for this in datapath.h.
> nh_ofs += sizeof(struct vlan_hdr);
> }
> memcpy(key->dl_src, eth->h_source, ETH_ALEN);
> diff --git a/include/openflow/openflow.h b/include/openflow/openflow.h
> index 1f18370..c45dbeb 100644
> --- a/include/openflow/openflow.h
> +++ b/include/openflow/openflow.h
> @@ -46,7 +46,7 @@
> /* The most significant bit being set in the version field indicates an
> * experimental OpenFlow version.
> */
> -#define OFP_VERSION 0x97
> +#define OFP_VERSION 0x98
>
> #define OFP_MAX_TABLE_NAME_LEN 32
> #define OFP_MAX_PORT_NAME_LEN 16
> @@ -313,6 +313,7 @@ enum ofp_action_type {
> OFPAT_SET_DL_DST, /* Ethernet destination address. */
> OFPAT_SET_NW_SRC, /* IP source address. */
> OFPAT_SET_NW_DST, /* IP destination address. */
> + OFPAT_SET_NW_TOS, /* IP ToS/DSCP field (6 bits). */
This really got added in the *middle*? Doesn't anyone think
about really basic compatibility issues?
> OFPAT_SET_TP_SRC, /* TCP/UDP source port. */
> OFPAT_SET_TP_DST, /* TCP/UDP destination port. */
> OFPAT_VENDOR = 0xffff
> @@ -208,9 +209,10 @@ struct odp_flowvec {
> #define ODPAT_SET_DL_DST 7 /* Ethernet destination address. */
> #define ODPAT_SET_NW_SRC 8 /* IP source address. */
> #define ODPAT_SET_NW_DST 9 /* IP destination address. */
> -#define ODPAT_SET_TP_SRC 10 /* TCP/UDP source port. */
> -#define ODPAT_SET_TP_DST 11 /* TCP/UDP destination port. */
> -#define ODPAT_N_ACTIONS 12
> +#define ODPAT_SET_NW_TOS 10 /* IP ToS/DSCP field (6 bits). */
> +#define ODPAT_SET_TP_SRC 11 /* TCP/UDP source port. */
> +#define ODPAT_SET_TP_DST 12 /* TCP/UDP destination port. */
> +#define ODPAT_N_ACTIONS 13
Let's add it at the end in the kernel, at least.
> struct odp_action_output {
> __u16 type; /* ODPAT_OUTPUT. */
> @@ -262,6 +264,14 @@ struct odp_action_nw_addr {
> __be32 nw_addr; /* IP address. */
> };
>
> +struct odp_action_nw_tos {
> + __u16 type; /* ODPAT_SET_NW_TOS. */
> + __u8 nw_tos; /* IP ToS/DSCP field (6 bits). */
> + __u8 reserved1;
> + __u16 reserved2;
> + __u16 reserved3;
> +};
> +
> /* Action structure for ODPAT_SET_TP_SRC/DST. */
> struct odp_action_tp_port {
> __u16 type; /* ODPAT_SET_TP_SRC/DST. */
> @@ -279,6 +289,7 @@ union odp_action {
> struct odp_action_vlan_pcp vlan_pcp;
> struct odp_action_dl_addr dl_addr;
> struct odp_action_nw_addr nw_addr;
> + struct odp_action_nw_tos nw_tos;
> struct odp_action_tp_port tp_port;
> };
>
> diff --git a/lib/classifier.h b/lib/classifier.h
> index 8b095e9..b11aaa2 100644
> --- a/lib/classifier.h
> +++ b/lib/classifier.h
> @@ -46,6 +46,7 @@
> /* ----------------- ----------- -------- */ \
> CLS_FIELD(OFPFW_IN_PORT, in_port, IN_PORT) \
> CLS_FIELD(OFPFW_DL_VLAN, dl_vlan, DL_VLAN) \
> + CLS_FIELD(OFPFW_DL_VLAN_PCP, dl_vlan_pcp, DL_VLAN_PCP) \
> CLS_FIELD(OFPFW_DL_SRC, dl_src, DL_SRC) \
> CLS_FIELD(OFPFW_DL_DST, dl_dst, DL_DST) \
> CLS_FIELD(OFPFW_DL_TYPE, dl_type, DL_TYPE) \
Have you carefully thought out the placement here? What is the
likelihood that someone will wildcard vlan_pcp? If it is highly
likely, it should go toward the end; if it is highly unlikely, it
should be toward the beginning. My guess is that it is one of
the most likely to be wildcarded (who really cares about the
exact vlan_pcp?) and so I would put it at the end.
> @@ -1162,6 +1163,23 @@ dp_netdev_set_nw_addr(struct ofpbuf *packet, flow_t *key,
> }
>
> static void
> +dp_netdev_set_nw_tos(struct ofpbuf *packet, flow_t *key,
> + const struct odp_action_nw_tos *a)
> +{
> + if (key->dl_type == htons(ETH_TYPE_IP)) {
> + struct ip_header *nh = packet->l3;
> + uint8_t *field = &nh->ip_tos;
> +
> + /* We only set the lower 6 bits. */
> + uint8_t new = (a->nw_tos & 0x3f) | (nh->ip_tos & 0xc0);
> +
> + nh->ip_csum = recalc_csum16(nh->ip_csum, htons((uint16_t)*field),
> + htons((uint16_t)a->nw_tos));
Don't need the casts here either.
> + *field = new;
> + }
> +}
> +
> +static void
> dp_netdev_set_tp_port(struct ofpbuf *packet, flow_t *key,
> const struct odp_action_tp_port *a)
> {
> @@ -1284,6 +1302,10 @@ dp_netdev_execute_actions(struct dp_netdev *dp,
> dp_netdev_set_nw_addr(packet, key, &a->nw_addr);
> break;
>
> + case ODPAT_SET_NW_TOS:
> + dp_netdev_set_nw_tos(packet, key, &a->nw_tos);
> + break;
I'd consider changing the third argument to be a->nw_tos.nw_tos
here too.
> case ODPAT_SET_TP_SRC:
> case ODPAT_SET_TP_DST:
> dp_netdev_set_tp_port(packet, key, &a->tp_port);
> diff --git a/lib/flow.c b/lib/flow.c
> index 7d4a1bd..04ff3f8 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -139,6 +139,7 @@ flow_extract(struct ofpbuf *packet, uint16_t in_port, flow_t *flow)
> if (vh) {
> flow->dl_type = vh->vlan_next_type;
> flow->dl_vlan = vh->vlan_tci & htons(VLAN_VID_MASK);
> + flow->dl_vlan_pcp = (ntohs(vh->vlan_tci) & 0xe000) >> 13;
I'd add a VLAN_PCP_SHIFT to packets.h and use it here (along with
VLAN_PCP_MASK). Or we could add inline functions for extracting
these bits to packets.h.
More information about the dev
mailing list