[ovs-dev] [PATCH 2/3] odp-util: Always export the priority and skb_mark netline attributes.

Andy Zhou azhou at nicira.com
Mon Aug 5 19:42:49 UTC 2013


Ben's original commit message gives more background to the netlink protocol
changes we have been making. It would be nice to keep them as well.


On Mon, Aug 5, 2013 at 12:34 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Mon, Aug 05, 2013 at 11:22:49AM -0700, Jesse Gross wrote:
> > This information is factually correct but it's not really related to
> > the change in this commit.
> >
> > The problem here is very simple: the current protocol allows a default
> > value of zero if either mark or priority is not specified (this
> > actually is part of the ABI). When userspace serializes either the
> > value or mask it looks at the value and omits the netlink attribute if
> > it is zero. This is a bug because an exact match on zero turns into a
> > wildcard of the field.
> >
> > These two fields (plus input port and EtherType) are special because
> > they can be omitted whereas most other values are required to be fully
> > specified. These protocol variations tend to cause bugs (as above)
> > when we evolve the protocol because an exception that makes sense in
> > one context might not be logical in another. Since the default value
> > for mark and priority are merely shorthands, we can push the protocol
> > in a more consistent direction by ignoring the shortcut and always
> > serializing the values.
>
> Thanks, like this?
>
> --8<--------------------------cut here-------------------------->8--
>
> From: Andy Zhou <azhou at nicira.com>
> Date: Sat, 3 Aug 2013 12:23:15 -0700
> Subject: [PATCH] odp-util: Always export the priority and skb_mark netlink
>  attributes.
>
> The current Netlink protocol allows a default value of zero if either mark
> or priority is not specified (this is part of the ABI).  Until now, when
> userspace serializes either the value or mask, it looked at the value and
> omitted the netlink attribute if it is zero.  This is a bug because an
> exact match on zero turns into a wildcard of the field.
>
> These two fields (plus input port and EtherType) are special because they
> can be omitted whereas most other values are required to be fully
> specified.  These protocol variations tend to cause bugs (as above) when we
> evolve the protocol because an exception that makes sense in one context
> might not be logical in another.  Since the default value for mark and
> priority are merely shorthands, we can push the protocol in a more
> consistent direction by ignoring the shortcut and always serializing the
> values.  This is what this commits does.
>
> Signed-off-by: Andy Zhou <azhou at nicira.com>
> [blp at nicira.com added Jesse's text to the commit message]
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  lib/odp-util.c        |    8 ++------
>  tests/odp.at          |   33 +++++++++++++++++++--------------
>  tests/ofproto-dpif.at |   18 +++++++++---------
>  3 files changed, 30 insertions(+), 29 deletions(-)
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index cc83fa5..78d5a1b 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -2355,17 +2355,13 @@ odp_flow_key_from_flow__(struct ofpbuf *buf, const
> struct flow *data,
>       * treat 'data' as a mask. */
>      is_mask = (data != flow);
>
> -    if (flow->skb_priority) {
> -        nl_msg_put_u32(buf, OVS_KEY_ATTR_PRIORITY, data->skb_priority);
> -    }
> +    nl_msg_put_u32(buf, OVS_KEY_ATTR_PRIORITY, data->skb_priority);
>
>      if (flow->tunnel.ip_dst || is_mask) {
>          tun_key_to_attr(buf, &data->tunnel);
>      }
>
> -    if (flow->skb_mark) {
> -        nl_msg_put_u32(buf, OVS_KEY_ATTR_SKB_MARK, data->skb_mark);
> -    }
> +    nl_msg_put_u32(buf, OVS_KEY_ATTR_SKB_MARK, data->skb_mark);
>
>      /* Add an ingress port attribute if this is a mask or 'odp_in_port'
>       * is not the magical value "ODPP_NONE". */
> diff --git a/tests/odp.at b/tests/odp.at
> index b0aca6c..469e120 100644
> --- a/tests/odp.at
> +++ b/tests/odp.at
> @@ -24,7 +24,7 @@
> in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x86dd),ipv
>
>  in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x86dd),ipv6(src=::1,dst=::2,label=0,proto=58,tclass=0,hlimit=128,frag=no),icmpv6(type=136,code=0),nd(target=::3,tll=00:0a:0b:0c:0d:0e)
>
>  in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x86dd),ipv6(src=::1,dst=::2,label=0,proto=58,tclass=0,hlimit=128,frag=no),icmpv6(type=136,code=0),nd(target=::3,sll=00:05:06:07:08:09,tll=00:0a:0b:0c:0d:0e)
>
>  in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x0806),arp(sip=1.2.3.4,tip=5.6.7.8,op=1,sha=00:0f:10:11:12:13,tha=00:14:15:16:17:18)
>
> -skb_mark(0x1234),in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x86dd),ipv6(src=::1,dst=::2,label=0,proto=58,tclass=0,hlimit=128,frag=no),icmpv6(type=136,code=0),nd(target=::3,sll=00:05:06:07:08:09,tll=00:0a:0b:0c:0d:0e)
>
> +in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x86dd),ipv6(src=::1,dst=::2,label=0,proto=58,tclass=0,hlimit=128,frag=no),icmpv6(type=136,code=0),nd(target=::3,sll=00:05:06:07:08:09,tll=00:0a:0b:0c:0d:0e)
>
>  in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x8847),mpls(label=100,tc=3,ttl=64,bos=1)
>
>  in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x8847),mpls(label=100,tc=7,ttl=100,bos=1)
>
>  in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x8847),mpls(label=100,tc=7,ttl=100,bos=0)
> @@ -33,48 +33,53 @@
> in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x8848),mpl
>  ])
>
>  (echo '# Valid forms without tun_id or VLAN header.'
> - cat odp-base.txt
> + set 's/^/skb_priority(0),skb_mark(0),/' odp-base.txt
> +
> + set '
> +s/^/skb_priority(0),skb_mark(0),/
> +' odp-base.txt
> +
>
>   echo
>   echo '# Valid forms with tunnel header.'
> - sed
> 's/^/tunnel(tun_id=0x7f10354,src=10.10.10.10,dst=20.20.20.20,tos=0x0,ttl=64,flags(csum,key)),/'
> odp-base.txt
> + sed
> 's/^/skb_priority(0),tunnel(tun_id=0x7f10354,src=10.10.10.10,dst=20.20.20.20,tos=0x0,ttl=64,flags(csum,key)),skb_mark(0x1234),/'
> odp-base.txt
>
>   echo
>   echo '# Valid forms with VLAN header.'
> - sed 's/\(eth([[^)]]*)\),*/\1,eth_type(0x8100),vlan(vid=99,pcp=7),encap(/
> + sed 's/^/skb_priority(0),skb_mark(0),/
> +s/\(eth([[^)]]*)\),*/\1,eth_type(0x8100),vlan(vid=99,pcp=7),encap(/
>  s/$/)/' odp-base.txt
>
>   echo
>   echo '# Valid forms with MPLS header.'
> - sed
> 's/\(eth([[^)]]*),?\)/\1,eth_type(0x8847),mpls(label=100,tc=7,ttl=64,bos=1)/'
> odp-base.txt
> + sed 's/^/skb_priority(0),skb_mark(0),/
> +s/\(eth([[^)]]*),?\)/\1,eth_type(0x8847),mpls(label=100,tc=7,ttl=64,bos=1)/'
> odp-base.txt
>
>   echo
>   echo '# Valid forms with MPLS multicast header.'
> - sed
> 's/\(eth([[^)]]*),?\)/\1,eth_type(0x8848),mpls(label=100,tc=7,ttl=64,bos=1)/'
> odp-base.txt
> -
> - echo
> - echo '# Valid forms with QoS priority.'
> - sed 's/^/skb_priority(0x1234),/' odp-base.txt
> + sed 's/^/skb_priority(0),skb_mark(0),/
> +s/\(eth([[^)]]*),?\)/\1,eth_type(0x8848),mpls(label=100,tc=7,ttl=64,bos=1)/'
> odp-base.txt
>
>   echo
>   echo '# Valid forms with tunnel and VLAN headers.'
> - sed
> 's/^/tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,tos=0x8,ttl=128,flags(key)),/
> + sed
> 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,tos=0x8,ttl=128,flags(key)),skb_mark(0),/
>  s/\(eth([[^)]]*)\),*/\1,eth_type(0x8100),vlan(vid=99,pcp=7),encap(/
>  s/$/)/' odp-base.txt
>
>   echo
>   echo '# Valid forms with QOS priority, tunnel, and VLAN headers.'
> - sed
> 's/^/skb_priority(0x1234),tunnel(tun_id=0xfedcba9876543210,src=10.10.10.10,dst=20.20.20.20,tos=0x8,ttl=64,flags(key)),/
> + sed
> 's/^/skb_priority(0x1234),tunnel(tun_id=0xfedcba9876543210,src=10.10.10.10,dst=20.20.20.20,tos=0x8,ttl=64,flags(key)),skb_mark(0),/
>  s/\(eth([[^)]]*)\),*/\1,eth_type(0x8100),vlan(vid=99,pcp=7),encap(/
>  s/$/)/' odp-base.txt
>
>   echo
>   echo '# Valid forms with IP first fragment.'
> -sed -n 's/,frag=no),/,frag=first),/p' odp-base.txt
> +sed 's/^/skb_priority(0),skb_mark(0),/' odp-base.txt | sed -n
> 's/,frag=no),/,frag=first),/p'
>
>   echo
>   echo '# Valid forms with IP later fragment.'
> -sed -n 's/,frag=no),.*/,frag=later)/p' odp-base.txt) > odp.txt
> +sed 's/^/skb_priority(0),skb_mark(0),/' odp-base.txt | sed -n
> 's/,frag=no),.*/,frag=later)/p'
> +) > odp.txt
>  AT_CAPTURE_FILE([odp.txt])
>  AT_CHECK_UNQUOTED([test-odp parse-keys < odp.txt], [0], [`cat odp.txt`
>  ])
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 2c14af6..46e1dea 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -2088,12 +2088,12 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p2
> 'in_port(2),eth(src=50:54:00:00:00:
>  AT_CHECK([ovs-appctl netdev-dummy/receive p3
> 'in_port(3),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
>
>  AT_CHECK([ovs-appctl dpif/dump-flows br0 | sort | STRIP_USED], [0], [dnl
> -in_port(1),eth_type(0x0800),ipv4(src=
> 192.168.0.1/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:0, bytes:0, used:0.0s,
> actions:userspace(pid=0,slow_path(controller))
> -in_port(2),eth_type(0x0800),ipv4(src=
> 192.168.0.2/0.0.0.0,dst=192.168.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:0, bytes:0, used:0.0s,
> actions:userspace(pid=0,slow_path(controller))
> +skb_priority(0),in_port(1),eth_type(0x0800),ipv4(src=
> 192.168.0.1/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:0, bytes:0, used:0.0s,
> actions:userspace(pid=0,slow_path(controller))
> +skb_priority(0),in_port(2),eth_type(0x0800),ipv4(src=
> 192.168.0.2/0.0.0.0,dst=192.168.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:0, bytes:0, used:0.0s,
> actions:userspace(pid=0,slow_path(controller))
>  ])
>
>  AT_CHECK([ovs-appctl dpif/dump-flows br1 | sort | STRIP_USED], [0], [dnl
> -in_port(3),eth_type(0x0800),ipv4(src=
> 10.0.0.2/0.0.0.0,dst=10.0.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:0, bytes:0, used:0.0s,
> actions:userspace(pid=0,slow_path(controller))
> +skb_priority(0),in_port(3),eth_type(0x0800),ipv4(src=
> 10.0.0.2/0.0.0.0,dst=10.0.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:0, bytes:0, used:0.0s,
> actions:userspace(pid=0,slow_path(controller))
>  ])
>
>  OVS_VSWITCHD_STOP
> @@ -2110,12 +2110,12 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p2
> 'in_port(2),eth(src=50:54:00:00:00:
>  AT_CHECK([ovs-appctl netdev-dummy/receive p3
> 'in_port(3),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
>
>  AT_CHECK([ovs-appctl dpif/dump-flows br0 | sort | STRIP_USED], [0], [dnl
> -in_port(1),eth_type(0x0800),ipv4(src=
> 192.168.0.1/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:0, bytes:0, used:0.0s,
> actions:userspace(pid=0,slow_path(controller))
> -in_port(2),eth_type(0x0800),ipv4(src=
> 192.168.0.2/0.0.0.0,dst=192.168.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:0, bytes:0, used:0.0s,
> actions:userspace(pid=0,slow_path(controller))
> +skb_priority(0),in_port(1),eth_type(0x0800),ipv4(src=
> 192.168.0.1/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:0, bytes:0, used:0.0s,
> actions:userspace(pid=0,slow_path(controller))
> +skb_priority(0),in_port(2),eth_type(0x0800),ipv4(src=
> 192.168.0.2/0.0.0.0,dst=192.168.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:0, bytes:0, used:0.0s,
> actions:userspace(pid=0,slow_path(controller))
>  ])
>
>  AT_CHECK([ovs-appctl dpif/dump-flows br1 | sort | STRIP_USED], [0], [dnl
> -in_port(3),eth_type(0x0800),ipv4(src=
> 10.0.0.2/0.0.0.0,dst=10.0.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:0, bytes:0, used:0.0s,
> actions:userspace(pid=0,slow_path(controller))
> +skb_priority(0),in_port(3),eth_type(0x0800),ipv4(src=
> 10.0.0.2/0.0.0.0,dst=10.0.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:0, bytes:0, used:0.0s,
> actions:userspace(pid=0,slow_path(controller))
>  ])
>
>  AT_CHECK([ovs-appctl dpif/del-flows br0])
> @@ -2123,7 +2123,7 @@ AT_CHECK([ovs-appctl dpif/dump-flows br0 | sort |
> STRIP_USED], [0], [dnl
>  ])
>
>  AT_CHECK([ovs-appctl dpif/dump-flows br1 | sort | STRIP_USED], [0], [dnl
> -in_port(3),eth_type(0x0800),ipv4(src=
> 10.0.0.2/0.0.0.0,dst=10.0.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:0, bytes:0, used:0.0s,
> actions:userspace(pid=0,slow_path(controller))
> +skb_priority(0),in_port(3),eth_type(0x0800),ipv4(src=
> 10.0.0.2/0.0.0.0,dst=10.0.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:0, bytes:0, used:0.0s,
> actions:userspace(pid=0,slow_path(controller))
>  ])
>
>  OVS_VSWITCHD_STOP
> @@ -2170,10 +2170,10 @@ dummy at ovs-dummy: hit:13 missed:2
>  ])
>
>  AT_CHECK([ovs-appctl dpif/dump-flows br0 | STRIP_USED], [0], [dnl
> -in_port(100),eth_type(0x0800),ipv4(src=
> 192.168.0.1/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:9, bytes:540, used:0.0s, actions:101,3,2
> +skb_priority(0),in_port(100),eth_type(0x0800),ipv4(src=
> 192.168.0.1/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:9, bytes:540, used:0.0s, actions:101,3,2
>  ]),
>  AT_CHECK([ovs-appctl dpif/dump-flows br1 | STRIP_USED], [0], [dnl
> -in_port(101),eth_type(0x0800),ipv4(src=
> 192.168.0.2/0.0.0.0,dst=192.168.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:4, bytes:240, used:0.0s, actions:100,2,3
> +skb_priority(0),in_port(101),eth_type(0x0800),ipv4(src=
> 192.168.0.2/0.0.0.0,dst=192.168.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),
> packets:4, bytes:240, used:0.0s, actions:100,2,3
>  ])
>
>  AT_CHECK([ovs-ofctl dump-ports br0 pbr0], [0], [dnl
> --
> 1.7.10.4
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130805/c9a793cd/attachment-0003.html>


More information about the dev mailing list