[ovs-dev] [PATCH] ofp-util: For OF1.0, don't wildcard PCP field when 802.1Q header absent.

Jarno Rajahalme jrajahalme at nicira.com
Thu Aug 6 20:10:04 UTC 2015


Looks good to me, but some comments below.

Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

> On Aug 5, 2015, at 9:59 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> OpenFlow 1.0.1 says:
> 
>    The dl_vlan_pcp field must be ignored when the OFPFW_DL_VLAN wildcard
>    bit is set or when the dl_vlan value is set to OFP_VLAN_NONE.  Fields
>    that are ignored don’t need to be wildcarded and should be set to 0.
> 
> Previously, OVS wildcarded the PCP field when dl_vlan was OFP_VLAN_NONE,
> but this commit changes the behavior to that suggested above: the PCP
> field should not be wildcarded (and should be set to 0, but the code
> already did that).
> 

This feels highly counter-intuitive, but it works due to flow parser setting the PCP bits to zeroes when there is no vlan in the packet. However, this change will make matching a bit less efficient, as generally it is faster to wildcard bits than match them. Good to see that this was changed in OF 1.1.

> Found by OFTest.
> 
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> DESIGN.md          | 10 +++++-----
> lib/ofp-util.c     |  1 -
> tests/flowgen.pl   |  4 +---
> tests/ovs-ofctl.at | 10 ++++++----
> 4 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/DESIGN.md b/DESIGN.md
> index 58826d3..4afe3f8 100644
> --- a/DESIGN.md
> +++ b/DESIGN.md
> @@ -436,11 +436,11 @@ Each column is interpreted as follows.
>     NXM_OF_VLAN_TCI(_W), a mask of ffff is equivalent to
>     NXM_OF_VLAN_TCI.
> 
> -  - OF1.0 and OF1.1: wwww/x,yy/z means dl_vlan wwww, OFPFW_DL_VLAN
> -    x, dl_vlan_pcp yy, and OFPFW_DL_VLAN_PCP z.  ? means that the
> -    given nibble is ignored (and conventionally 0 for wwww or yy,
> -    conventionally 1 for x or z).  <none> means that the given match
> -    is not supported.
> +  - OF1.0 and OF1.1: wwww/x,yy/z means dl_vlan wwww, OFPFW_DL_VLAN x,
> +    dl_vlan_pcp yy, and OFPFW_DL_VLAN_PCP z.  ? means that the given
> +    bits are ignored (their conventional values are 0000/x,00/0 in
> +    OF1.0, 0000/x,00/1 in OF1.1; x is never ignored).  <none> means
> +    that the given match is not supported.
> 

It would be helpful if DESIGN.md reminded that OFPFW_* values here are flags that indicate if the given field should be wildcarded.

So, this comment could read:

  - OF1.0 and OF1.1: wwww/x,yy/z means dl_vlan wwww, OFPFW_DL_VLAN x,
    dl_vlan_pcp yy, and OFPFW_DL_VLAN_PCP z.  If OFPFW_DL_VLAN or
    OFPFW_DL_VLAN_PCP is 1, the corresponding field value is wildcarded,
    otherwise it is matched.  ? means that the given
    bits are ignored (their conventional values are 0000/x,00/0 in
    OF1.0, 0000/x,00/1 in OF1.1; x is never ignored).  <none> means
    that the given match is not supported.

>   - OF1.2: xxxx/yyyy,zz means OXM_OF_VLAN_VID_W with value xxxx and
>     mask yyyy, and OXM_OF_VLAN_PCP (which is not maskable) with
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index c333f28..06bd32e 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -344,7 +344,6 @@ ofputil_match_to_ofp10_match(const struct match *match,
>     } else if (match->wc.masks.vlan_tci & htons(VLAN_CFI)
>                && !(match->flow.vlan_tci & htons(VLAN_CFI))) {
>         ofmatch->dl_vlan = htons(OFP10_VLAN_NONE);
> -        ofpfw |= OFPFW10_DL_VLAN_PCP;
>     } else {
>         if (!(match->wc.masks.vlan_tci & htons(VLAN_VID_MASK))) {
>             ofpfw |= OFPFW10_DL_VLAN;
> diff --git a/tests/flowgen.pl b/tests/flowgen.pl
> index cdc275e..a0fc345 100755
> --- a/tests/flowgen.pl
> +++ b/tests/flowgen.pl
> @@ -1,6 +1,6 @@
> #! /usr/bin/perl
> 
> -# Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
> +# Copyright (c) 2009, 2010, 2011, 2012, 2015 Nicira, Inc.
> #
> # Licensed under the Apache License, Version 2.0 (the "License");
> # you may not use this file except in compliance with the License.
> @@ -115,8 +115,6 @@ sub output {
>     $packet .= pack_ethaddr($flow{DL_SRC});
>     if ($flow{DL_VLAN} != 0xffff) {
>         $packet .= pack('nn', 0x8100, $flow{DL_VLAN});
> -    } else {
> -        $wildcards |= 1 << 20;   # OFPFW10_DL_VLAN_PCP
>     }
>     my $len_ofs = length($packet);
>     $packet .= pack('n', 0) if $attrs{DL_HEADER} =~ /^802.2/;
> diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
> index c1e9ec4..ad08b31 100644
> --- a/tests/ovs-ofctl.at
> +++ b/tests/ovs-ofctl.at
> @@ -1140,14 +1140,16 @@ xxxxxxxx xxxxxxxx xxxx xxxx
> 002820fd xxxx xxxxxxxxxxxx xxxxxxxxxxxx 0123 04 xx xxxx xx xx xxxx dnl
> xxxxxxxx xxxxxxxx xxxx xxxx
> 
> +dnl dl_vlan_pcp doesn't make sense when 802.1Q is not present, so
> +dnl OVS ignores it and drops it on output.
> # vlan_tci=0x0000
> +#  1: 38 -> 28
> 003820fd xxxx xxxxxxxxxxxx xxxxxxxxxxxx ffff xx xx xxxx xx xx xxxx dnl
> xxxxxxxx xxxxxxxx xxxx xxxx
> 
> -dnl dl_vlan_pcp doesn't make sense when dl_vlan is "none", so
> +dnl dl_vlan_pcp doesn't make sense when 802.1Q is not present, so
> dnl OVS ignores it and drops it on output.
> # vlan_tci=0x0000
> -#  1: 28 -> 38
> # 20: 05 -> 00
> 002820fd xxxx xxxxxxxxxxxx xxxxxxxxxxxx ffff 05 xx xxxx xx xx xxxx dnl
> xxxxxxxx xxxxxxxx xxxx xxxx
> @@ -2374,7 +2376,7 @@ AT_CHECK([ovs-ofctl check-vlan 0000 ffff], [0], [dnl
> vlan_tci=0x0000 -> 0000/ffff
> NXM: NXM_OF_VLAN_TCI(0000) -> 0000/ffff
> OXM: OXM_OF_VLAN_VID(0000) -> 0000/1fff,--
> -OF1.0: ffff/0,00/1 -> 0000/ffff
> +OF1.0: ffff/0,00/0 -> 0000/ffff
> OF1.1: ffff/0,00/1 -> 0000/ffff
> ])
> 
> @@ -2419,7 +2421,7 @@ AT_CHECK([ovs-ofctl check-vlan 0000 f000], [0], [dnl
> vlan_tci=0x0000/0xf000 -> 0000/f000
> NXM: NXM_OF_VLAN_TCI_W(0000/f000) -> 0000/f000
> OXM: OXM_OF_VLAN_VID_W(0000/1000) -> 0000/1000,--
> -OF1.0: ffff/0,00/1 -> 0000/ffff
> +OF1.0: ffff/0,00/0 -> 0000/ffff
> OF1.1: ffff/0,00/1 -> 0000/ffff
> ])
> 
> -- 
> 2.1.3
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list