[ovs-dev] [generic tci mask 8/8] nx-match: Implement support for arbitrary VLAN TCI masks.
Ben Pfaff
blp at nicira.com
Tue Nov 23 18:02:38 UTC 2010
On Mon, Nov 22, 2010 at 06:49:07PM -0800, Justin Pettit wrote:
> 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?
Right, thanks.
> > +/* 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.
Ditto.
> > @@ -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.
I guess I see maintaining this correspondence as trivial. And if it
gets worse, we just don't do it anymore.
> > 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?
Well, the old code was a special case, and the new code is general. The
other fields don't apply that kind of logic, so it seems cleaner not to
apply it here either.
> > + /* 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.
Good spotting, thanks. I fixed this up.
More information about the dev
mailing list