Hi Andy,
I think your plan to remove ambiguity is a good one.
On Tue, Jul 30, 2013 at 09:09:35PM -0700, Andy Zhou wrote:
> Simon, Thanks for the review.
> The intention of my patch is to always export those attributes to remove
> the ambiguity associated with interpreting missing attributes. Glad to hear
> this patch would work well with recirculation.
>
>
> On Tue, Jul 30, 2013 at 8:54 PM, Simon Horman <ho...@verge.net.au> wrote:
>
> > On Tue, Jul 30, 2013 at 07:49:13PM -0700, Andy Zhou wrote:
> > > Handling of missing attributes in netlink can be tricky and turns out
> > > to be error prone. The value (savings in netlink bandwidth) does not
> > > seem to be significant enough to justify allowing them. This patch
> > > series make both kernel and userspace always export priority and
> > > skb_mark attribute. There will be follow on patches in the
> > > direction of making all attributes explicit.
> > >
> > > Signed-off-by: Andy Zhou <az...@nicira.com>
> > > ---
> > > lib/odp-util.c | 62
> > +++++++++++++++++++++++++++++++++++++++----------
> > > tests/ofproto-dpif.at | 18 +++++++-------
> > > 2 files changed, 59 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > > index 3c3063d..1f7db2f 100644
> > > --- a/lib/odp-util.c
> > > +++ b/lib/odp-util.c
> > > @@ -926,6 +926,42 @@ odp_mask_attr_is_exact(const struct nlattr *ma)
> > > return is_exact;
> > > }
> > >
> > > +static bool
> > > +ommit_format(enum ovs_key_attr attr, const struct nlattr *a,
> > > + const struct nlattr *ma)
> > > +{
> > > + switch (attr) {
> > > + case OVS_KEY_ATTR_PRIORITY:
> > > + case OVS_KEY_ATTR_SKB_MARK:
> > > + if (!nl_attr_get_u32(a)) {
> > > + if ((!ma) || !nl_attr_get_u32(ma)) {
> > > + return true; /* Omit output 0 (no mask) or 0/0 */
> > > + }
> > > + }
> > > + break;
> > > + case OVS_KEY_ATTR_UNSPEC:
> > > + case OVS_KEY_ATTR_ENCAP:
> > > + case OVS_KEY_ATTR_TUNNEL:
> > > + case OVS_KEY_ATTR_IN_PORT:
> > > + case OVS_KEY_ATTR_ETHERNET:
> > > + case OVS_KEY_ATTR_VLAN:
> > > + case OVS_KEY_ATTR_ETHERTYPE:
> > > + case OVS_KEY_ATTR_IPV4:
> > > + case OVS_KEY_ATTR_IPV6:
> > > + case OVS_KEY_ATTR_TCP:
> > > + case OVS_KEY_ATTR_UDP:
> > > + case OVS_KEY_ATTR_ICMP:
> > > + case OVS_KEY_ATTR_ICMPV6:
> > > + case OVS_KEY_ATTR_ARP:
> > > + case OVS_KEY_ATTR_ND:
> > > + case OVS_KEY_ATTR_MPLS:
> > > + case __OVS_KEY_ATTR_MAX:
> > > + default:
> > > + break;
> > > + }
> > > +
> > > + return false;
> > > +}
> > >
> > > static void
> > > format_odp_key_attr(const struct nlattr *a, const struct nlattr *ma,
> > > @@ -1345,13 +1381,13 @@ odp_flow_format(const struct nlattr *key, size_t
> > key_len,
> > > bool has_ethtype_key = false;
> > > const struct nlattr *ma = NULL;
> > > struct ofpbuf ofp;
> > > + bool first_key = true;
> > >
> > > ofpbuf_init(&ofp, 100);
> > > NL_ATTR_FOR_EACH (a, left, key, key_len) {
> > > - if (a != key) {
> > > - ds_put_char(ds, ',');
> > > - }
> > > - if (nl_attr_type(a) == OVS_KEY_ATTR_ETHERTYPE) {
> > > + enum ovs_key_attr attr = nl_attr_type(a);
> > > +
> > > + if ( attr == OVS_KEY_ATTR_ETHERTYPE) {
> > > has_ethtype_key = true;
> > > }
> > > if (mask && mask_len) {
> > > @@ -1360,8 +1396,14 @@ odp_flow_format(const struct nlattr *key, size_t
> > key_len,
> > > ma = generate_all_wildcard_mask(&ofp, a);
> > > }
> > > }
> > > - format_odp_key_attr(a, ma, ds);
> > > - ofpbuf_clear(&ofp);
> > > + if (!ommit_format(attr, a, ma)) {
> > > + if (!first_key) {
> > > + ds_put_char(ds, ',');
> > > + }
> > > + format_odp_key_attr(a, ma, ds);
> > > + first_key = false;
> > > + ofpbuf_clear(&ofp);
> > > + }
> > > }
> > > ofpbuf_uninit(&ofp);
> > >
> > > @@ -2323,17 +2365,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) {
> > > 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);
> >
> > I came across the skb_mark portion of this problem while re-basing
> > the recirculation patches. The approach that I took in
> > included in "Add packet recirculation" v14 is as follows:
> >
> >
> > if (flow->skb_mark || data->skb_mark) {
> > nl_msg_put_u32(buf, OVS_KEY_ATTR_SKB_MARK, data->skb_mark);
> > }
> >
> > This seemed to ensure that the mark was present in all cases
> > that it is needed by recirculation without adding it otherwise.
> > This avoided needing to update the test-suite. However, I think
> > your more general approach is cleaner, easier to understand,
> > and should work with recirculation: it suffers if skb_mark is not
> > present if needed but I don't believe the converse is true.
> >
> > Reviewed-by: Simon Horman <ho...@verge.net.au>
> >
> > > /* 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/ofproto-dpif.at b/tests/ofproto-dpif.at
> > > index 2728a28..3600fb6 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(src=50:54:00:00:00:05/00:00:00:00:00:00,dst=50:54:00:00:00:07/00:00:00:00:00:00),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/0x2),icmp(type=8/0,code=0/0),
> > packets:0, bytes:0, used:0.0s,
> > actions:userspace(pid=0,slow_path(controller))
> > >
> > -in_port(2),eth(src=50:54:00:00:00:07/00:00:00:00:00:00,dst=50:54:00:00:00:05/00:00:00:00:00:00),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/0x2),icmp(type=0/0,code=0/0),
> > packets:0, bytes:0, used:0.0s,
> > actions:userspace(pid=0,slow_path(controller))
> > >
> > +skb_priority(0),in_port(1),eth(src=50:54:00:00:00:05/00:00:00:00:00:00,dst=50:54:00:00:00:07/00:00:00:00:00:00),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/0x2),icmp(type=8/0,code=0/0),
> > packets:0, bytes:0, used:0.0s,
> > actions:userspace(pid=0,slow_path(controller))
> > >
> > +skb_priority(0),in_port(2),eth(src=50:54:00:00:00:07/00:00:00:00:00:00,dst=50:54:00:00:00:05/00:00:00:00:00:00),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/0x2),icmp(type=0/0,code=0/0),
> > 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(src=50:54:00:00:00:09/00:00:00:00:00:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),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/0x2),icmp(type=8/0,code=0/0),
> > packets:0, bytes:0, used:0.0s,
> > actions:userspace(pid=0,slow_path(controller))
> > >
> > +skb_priority(0),in_port(3),eth(src=50:54:00:00:00:09/00:00:00:00:00:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),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/0x2),icmp(type=8/0,code=0/0),
> > 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(src=50:54:00:00:00:05/00:00:00:00:00:00,dst=50:54:00:00:00:07/00:00:00:00:00:00),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/0x2),icmp(type=8/0,code=0/0),
> > packets:0, bytes:0, used:0.0s,
> > actions:userspace(pid=0,slow_path(controller))
> > >
> > -in_port(2),eth(src=50:54:00:00:00:07/00:00:00:00:00:00,dst=50:54:00:00:00:05/00:00:00:00:00:00),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/0x2),icmp(type=0/0,code=0/0),
> > packets:0, bytes:0, used:0.0s,
> > actions:userspace(pid=0,slow_path(controller))
> > >
> > +skb_priority(0),in_port(1),eth(src=50:54:00:00:00:05/00:00:00:00:00:00,dst=50:54:00:00:00:07/00:00:00:00:00:00),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/0x2),icmp(type=8/0,code=0/0),
> > packets:0, bytes:0, used:0.0s,
> > actions:userspace(pid=0,slow_path(controller))
> > >
> > +skb_priority(0),in_port(2),eth(src=50:54:00:00:00:07/00:00:00:00:00:00,dst=50:54:00:00:00:05/00:00:00:00:00:00),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/0x2),icmp(type=0/0,code=0/0),
> > 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(src=50:54:00:00:00:09/00:00:00:00:00:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),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/0x2),icmp(type=8/0,code=0/0),
> > packets:0, bytes:0, used:0.0s,
> > actions:userspace(pid=0,slow_path(controller))
> > >
> > +skb_priority(0),in_port(3),eth(src=50:54:00:00:00:09/00:00:00:00:00:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),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/0x2),icmp(type=8/0,code=0/0),
> > 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(src=50:54:00:00:00:09/00:00:00:00:00:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),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/0x2),icmp(type=8/0,code=0/0),
> > packets:0, bytes:0, used:0.0s,
> > actions:userspace(pid=0,slow_path(controller))
> > >
> > +skb_priority(0),in_port(3),eth(src=50:54:00:00:00:09/00:00:00:00:00:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),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/0x2),icmp(type=8/0,code=0/0),
> > packets:0, bytes:0, used:0.0s,
> > actions:userspace(pid=0,slow_path(controller))
> > > ])
> > >
> > > OVS_VSWITCHD_STOP
> > > @@ -2170,10 +2170,10 @@ dummy@ovs-dummy: hit:13 missed:2
> > > ])
> > >
> > > AT_CHECK([ovs-appctl dpif/dump-flows br0 | STRIP_USED], [0], [dnl
> > >
> > -in_port(100),eth(src=50:54:00:00:00:05/00:00:00:00:00:00,dst=50:54:00:00:00:07/00:00:00:00:00:00),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/0x2),icmp(type=8/0,code=0/0),
> > packets:9, bytes:540, used:0.0s, actions:101,3,2
> > >
> > +skb_priority(0),in_port(100),eth(src=50:54:00:00:00:05/00:00:00:00:00:00,dst=50:54:00:00:00:07/00:00:00:00:00:00),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/0x2),icmp(type=8/0,code=0/0),
> > 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(src=50:54:00:00:00:07/00:00:00:00:00:00,dst=50:54:00:00:00:05/00:00:00:00:00:00),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/0x2),icmp(type=8/0,code=0/0),
> > packets:4, bytes:240, used:0.0s, actions:100,2,3
> > >
> > +skb_priority(0),in_port(101),eth(src=50:54:00:00:00:07/00:00:00:00:00:00,dst=50:54:00:00:00:05/00:00:00:00:00:00),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/0x2),icmp(type=8/0,code=0/0),
> > packets:4, bytes:240, used:0.0s, actions:100,2,3
> > > ])
> > >
> > > AT_CHECK([ovs-ofctl dump-ports br0 pbr0], [0], [dnl
> > > --
> > > 1.7.9.5
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > http://openvswitch.org/mailman/listinfo/dev
> > >
> >
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
