Re: [ovs-dev] [PATCH 2/3] odp-util: Always export the priority and skb_mark netline attributes.
On Mon, Aug 05, 2013 at 12:46:43PM -0700, Jesse Gross wrote: > On Mon, Aug 5, 2013 at 12:34 PM, Ben Pfaff 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? > > Looks good, thanks. I applied this to master and branch-1.11. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/3] odp-util: Always export the priority and skb_mark netline attributes.
Those problems are different and not related to this change so it is
confusing to have it here.
On Mon, Aug 5, 2013 at 12:42 PM, Andy Zhou wrote:
> 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 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
>> 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
>> [b...@nicira.com added Jesse's text to the commit message]
>> Signed-off-by: Ben Pfaff
>> ---
>> 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,
Re: [ovs-dev] [PATCH 2/3] odp-util: Always export the priority and skb_mark netline attributes.
On Mon, Aug 5, 2013 at 12:34 PM, Ben Pfaff 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? Looks good, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/3] odp-util: Always export the priority and skb_mark netline attributes.
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 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
> 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
> [b...@nicira.com added Jesse's text to the commit message]
> Signed-off-by: Ben Pfaff
> ---
> 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
Re: [ovs-dev] [PATCH 2/3] odp-util: Always export the priority and skb_mark netline attributes.
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
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
[b...@nicira.com added Jesse's text to the commit message]
Signed-off-by: Ben Pfaff
---
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 @
Re: [ovs-dev] [PATCH 2/3] odp-util: Always export the priority and skb_mark netline attributes.
On Mon, Aug 5, 2013 at 11:06 AM, Ben Pfaff wrote: > After some consultation with Andy, this is the commit message I ended > up with. Jesse, Andy, is this correct? > > --8<--cut here-->8-- > > From: Andy Zhou > Date: Sat, 3 Aug 2013 12:23:15 -0700 > Subject: [PATCH] odp-util: Always export the priority and skb_mark netlink > attributes. > > Some of the kernel flows have "opaque" values used in special situations: > > - When no in_port is available, the kernel internally uses > USHRT_MAX to represent this, but this should not be visible to > userspace, that is, it should be possible for the kernel to switch > to using some other value without affecting userspace-visible > behavior. > > - When no skb_mark is specified, the kernel uses zero. (It is easy to > believe that zero is fixed by the kernel ABI, not opaque, but there > is some desire to keep it opaque.) > > - skb_priority, like skb_mark. > > If a flow specified via Netlink contained a mask without a value, then the > kernel would default to matching against the opaque "default" value for that > field. For example, a match against in_port that specified no value but a > mask of 0xf would, in the current kernel implementation, specify a match > against USHRT_MAX with a mask of 0xf. If the kernel accepted that value and > mask as-is, and then the user tried to add another flow with, say, a value > of 1 and mask of 1, that would overlap with the first flow, causing the > kernel to reject it. > > There are multiple ways that the kernel could solve this problem: > > - Reject flows that do not include a value, but only a mask, for a > given field. This is what the kernel does for most fields, > including skb_priority and skb_mask. > > - Always expand all masks for a given field to exact-match, so that > there can be no unexpected overlap among flows. The kernel does > this for in_port. > > We are somewhat uncertain, however, what the final handling of skb_mark and > skb_priority should be. This commit allows us some flexibility in deferring > the decision on kernel handling by making userspace always specify both a > value and a mask for the field. > > Signed-off-by: Andy Zhou > [b...@nicira.com rewrote the commit message] > Signed-off-by: Ben Pfaff 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. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/3] odp-util: Always export the priority and skb_mark netline attributes.
After some consultation with Andy, this is the commit message I ended
up with. Jesse, Andy, is this correct?
--8<--cut here-->8--
From: Andy Zhou
Date: Sat, 3 Aug 2013 12:23:15 -0700
Subject: [PATCH] odp-util: Always export the priority and skb_mark netlink
attributes.
Some of the kernel flows have "opaque" values used in special situations:
- When no in_port is available, the kernel internally uses
USHRT_MAX to represent this, but this should not be visible to
userspace, that is, it should be possible for the kernel to switch
to using some other value without affecting userspace-visible
behavior.
- When no skb_mark is specified, the kernel uses zero. (It is easy to
believe that zero is fixed by the kernel ABI, not opaque, but there
is some desire to keep it opaque.)
- skb_priority, like skb_mark.
If a flow specified via Netlink contained a mask without a value, then the
kernel would default to matching against the opaque "default" value for that
field. For example, a match against in_port that specified no value but a
mask of 0xf would, in the current kernel implementation, specify a match
against USHRT_MAX with a mask of 0xf. If the kernel accepted that value and
mask as-is, and then the user tried to add another flow with, say, a value
of 1 and mask of 1, that would overlap with the first flow, causing the
kernel to reject it.
There are multiple ways that the kernel could solve this problem:
- Reject flows that do not include a value, but only a mask, for a
given field. This is what the kernel does for most fields,
including skb_priority and skb_mask.
- Always expand all masks for a given field to exact-match, so that
there can be no unexpected overlap among flows. The kernel does
this for in_port.
We are somewhat uncertain, however, what the final handling of skb_mark and
skb_priority should be. This commit allows us some flexibility in deferring
the decision on kernel handling by making userspace always specify both a
value and a mask for the field.
Signed-off-by: Andy Zhou
[b...@nicira.com rewrote the commit message]
Signed-off-by: Ben Pfaff
---
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_ty
Re: [ovs-dev] [PATCH 2/3] odp-util: Always export the priority and skb_mark netline attributes.
It actually does fix a bug because when the value is zero we don't serialize the attributes for the value or mask. This is OK for the value because zero is the default. However, just because the value is zero doesn't mean that the mask can be omitted (and therefore wildcarded). Longer term, the goal here is to reduce the possibility for these types of bugs by removing exceptions to our normal processing rules. On Sat, Aug 3, 2013 at 2:57 PM, Andy Zhou wrote: > Jesse, please feel free to correct me if I am wrong. > > As far as I know, this is part of the plan on improving how user space and > kernel are treating missing attributes. > > Missing attribute has to be interpreted to be some default value, and the > value is usually opaque to the other side of the protocol. When a mask is > applied to the field, The opaque value may be matched unintentionally. In > general, we feel that having opaque values will make the netlink protocol > error prone. The saving in netlink bandwidth is not worth the trouble. > > For backward compatibility reasons, some fields, such as in_port and > eth_type, we will continue to allow them to be missing, Priority and > skb_mark, on the hand, can be made explicit and still be backward > compatible, > > Although not fix any bug in particular, this is part the clean up effort. > Kernel side of change is already in. > > > On Sat, Aug 3, 2013 at 2:22 PM, Ben Pfaff wrote: >> >> The commit message doesn't say why. Why? > > > > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/3] odp-util: Always export the priority and skb_mark netline attributes.
Just for completeness, We are dealing with eth_type and in_port by forcing exact match int the kernel. On Sat, Aug 3, 2013 at 2:57 PM, Andy Zhou wrote: > Jesse, please feel free to correct me if I am wrong. > > As far as I know, this is part of the plan on improving how user space and > kernel are treating missing attributes. > > Missing attribute has to be interpreted to be some default value, and the > value is usually opaque to the other side of the protocol. When a mask is > applied to the field, The opaque value may be matched unintentionally. In > general, we feel that having opaque values will make the netlink protocol > error prone. The saving in netlink bandwidth is not worth the trouble. > > For backward compatibility reasons, some fields, such as in_port and > eth_type, we will continue to allow them to be missing, Priority and > skb_mark, on the hand, can be made explicit and still be backward > compatible, > > Although not fix any bug in particular, this is part the clean up effort. > Kernel side of change is already in. > > > On Sat, Aug 3, 2013 at 2:22 PM, Ben Pfaff wrote: > >> The commit message doesn't say why. Why? >> > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/3] odp-util: Always export the priority and skb_mark netline attributes.
Jesse, please feel free to correct me if I am wrong. As far as I know, this is part of the plan on improving how user space and kernel are treating missing attributes. Missing attribute has to be interpreted to be some default value, and the value is usually opaque to the other side of the protocol. When a mask is applied to the field, The opaque value may be matched unintentionally. In general, we feel that having opaque values will make the netlink protocol error prone. The saving in netlink bandwidth is not worth the trouble. For backward compatibility reasons, some fields, such as in_port and eth_type, we will continue to allow them to be missing, Priority and skb_mark, on the hand, can be made explicit and still be backward compatible, Although not fix any bug in particular, this is part the clean up effort. Kernel side of change is already in. On Sat, Aug 3, 2013 at 2:22 PM, Ben Pfaff wrote: > The commit message doesn't say why. Why? > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/3] odp-util: Always export the priority and skb_mark netline attributes.
The commit message doesn't say why. Why? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
