Looks good to me, but some comments below.
Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
> On Aug 5, 2015, at 9:59 PM, Ben Pfaff <b...@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 <b...@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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
