[ovs-dev] [generic tci mask 8/8] nx-match: Implement support for arbitrary VLAN TCI masks.

Justin Pettit jpettit at nicira.com
Tue Nov 23 02:49:07 UTC 2010


On Nov 10, 2010, at 3:38 PM, Ben Pfaff wrote:

> +/* Modifies 'rule' so that the VLAN VID is wildcarded.  If the PCP is already
> + * wildcarded, then 'rule' will match a packet regardless of whether it has an
> + * 802.1Q header or not. */
> +void
> +cls_rule_set_any_vid(struct cls_rule *rule)
> +{
> +    if (rule->wc.vlan_tci_mask & htons(VLAN_PCP_MASK)) {
> +        rule->wc.vlan_tci_mask &= htons(VLAN_VID_MASK);
> +        rule->flow.vlan_tci &= htons(VLAN_VID_MASK);

Shouldn't these masks be inverted?

> +/* Modifies 'rule' so that the VLAN PCP is wildcarded.  If the VID is already
> + * wildcarded, then 'rule' will match a packet regardless of whether it has an
> + * 802.1Q header or not. */
> +void
> +cls_rule_set_any_pcp(struct cls_rule *rule)
> +{
> +    if (rule->wc.vlan_tci_mask & htons(VLAN_VID_MASK)) {
> +        rule->wc.vlan_tci_mask &= htons(VLAN_PCP_MASK);
> +        rule->flow.vlan_tci &= htons(VLAN_PCP_MASK);

Same here.

> @@ -99,7 +98,6 @@ typedef unsigned int OVS_BITWISE flow_wildcards_t;
> 
> /* Same values and meanings as corresponding OFPFW_* bits. */
> #define FWW_IN_PORT     ((OVS_FORCE flow_wildcards_t) (1 << 0))
> -#define FWW_DL_VLAN     ((OVS_FORCE flow_wildcards_t) (1 << 1))
> #define FWW_DL_SRC      ((OVS_FORCE flow_wildcards_t) (1 << 2))
> #define FWW_DL_DST      ((OVS_FORCE flow_wildcards_t) (1 << 3))
>                                               /* excluding the multicast bit */
> @@ -108,14 +106,14 @@ typedef unsigned int OVS_BITWISE flow_wildcards_t;
> #define FWW_TP_SRC      ((OVS_FORCE flow_wildcards_t) (1 << 6))
> #define FWW_TP_DST      ((OVS_FORCE flow_wildcards_t) (1 << 7))
> /* Same meanings as corresponding OFPFW_* bits, but differ in value. */
> -#define FWW_DL_VLAN_PCP ((OVS_FORCE flow_wildcards_t) (1 << 8))
> -#define FWW_NW_TOS      ((OVS_FORCE flow_wildcards_t) (1 << 9))
> +#define FWW_NW_TOS      ((OVS_FORCE flow_wildcards_t) (1 << 1))
> /* No OFPFW_* bits, but they do have corresponding OVSFW_* bits. */
> -#define FWW_TUN_ID      ((OVS_FORCE flow_wildcards_t) (1 << 10))
> +#define FWW_TUN_ID      ((OVS_FORCE flow_wildcards_t) (1 << 8))
> /* No corresponding OFPFW_* or OVSFW_* bits. */
> -#define FWW_ETH_MCAST   ((OVS_FORCE flow_wildcards_t) (1 << 11))
> +#define FWW_VLAN_TCI    ((OVS_FORCE flow_wildcards_t) (1 << 9)
> +#define FWW_ETH_MCAST   ((OVS_FORCE flow_wildcards_t) (1 << 10))
>                                                        /* multicast bit only */
> -#define FWW_ALL         ((OVS_FORCE flow_wildcards_t) (((1 << 12)) - 1))
> +#define FWW_ALL         ((OVS_FORCE flow_wildcards_t) (((1 << 11)) - 1))

I think trying to keep the OpenFlow wildcard bits somewhat in line with the OVS ones is a bit confusing.  I understand it saves bit manipulation for the ones that are in common, but I think this divergence is only going to get worse.

>     case NFI_NXM_OF_VLAN_TCI_W:
> -        return parse_tci(rule, get_unaligned_u16(value),
> -                         get_unaligned_u16(mask));
> +        if (wc->vlan_tci_mask) {
> +            return NXM_DUP_TYPE;
> +        } else {
> +            cls_rule_set_dl_tci_masked(rule, get_unaligned_u16(value),
> +                                       get_unaligned_u16(mask));
> +            return 0;
> +        }

In the previous version, a NXM_INVALID would be sent if an invalid mask/value combination was specified (e.g., an exact match with a nonzero TCI value with CFI=0).  Is there a reason not to do those sanity checks still?

> +    /* Translate VLANs. */
> +    vid = match->dl_vlan & htons(VLAN_VID_MASK);
> +    pcp = htons((match->dl_vlan_pcp << VLAN_PCP_SHIFT) & VLAN_PCP_MASK);
> +    switch (ofpfw & (OFPFW_DL_VLAN | OFPFW_DL_VLAN_PCP)) {
> ...
> +    case OFPFW_DL_VLAN_PCP:
> +        if (match->dl_vlan == htons(OFP_VLAN_NONE)) {
> +            /* Match only packets without 802.1Q header. */
> +            rule->flow.vlan_tci = htons(0);
> +            rule->wc.vlan_tci_mask = htons(0xffff);
> ...
> +    case 0:
> +        /* Specific VID and PCP. */
> +        rule->flow.vlan_tci = vid | pcp | htons(VLAN_CFI);
> +        rule->wc.vlan_tci_mask = htons(0xffff);
> +        break;
> +    }

Should you special-case the OFP_VLAN_NONE for case "0", so that it doesn't set the VLAN_CFI bit?  If not, it seems inconsistent with the definition in nicira-ext.h and the behavior for the OFPFW_DL_VLAN_PCP case.

--Justin






More information about the dev mailing list