Re: [ovs-dev] [PATCH v2] datapath: Add support for kernel 3.14.
On 04/09/2014 01:10 AM, Pritesh Kothari wrote: Signed-off-by: Pritesh Kothari --- v2: Use OVS_GREP_IFELSE instead of upstream kernel versions as Thomas suggested. --- FAQ |2 +- NEWS |2 +- acinclude.m4 |8 ++-- datapath/actions.c|2 +- datapath/linux/Modules.mk |1 + datapath/linux/compat/include/linux/etherdevice.h |2 ++ datapath/linux/compat/include/linux/if_tunnel.h |4 ++-- datapath/linux/compat/include/linux/random.h | 10 ++ datapath/linux/compat/include/linux/skbuff.h |3 +++ datapath/linux/compat/vxlan.c |2 +- datapath/vport-lisp.c |2 +- datapath/vport.c | 12 ++-- datapath/vport.h |2 +- 13 files changed, 36 insertions(+), 16 deletions(-) create mode 100644 datapath/linux/compat/include/linux/random.h LGTM now Acked-by: Thomas Graf ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 08/10] openvswithch.h: Clarify use of key attributes.
On 04/09/2014 01:38 AM, Jarno Rajahalme wrote: Key attributes relating to actual packet headers are ignored for OVS_PACKET_CMD_EXECUTE as the header key attributes are retrieved from the packet itself. Signed-off-by: Jarno Rajahalme LGTM Acked-by: Thomas Graf ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 09/10] datapath: Allow masks for set actions.
On 04/09/2014 01:38 AM, Jarno Rajahalme wrote:
Masked set actions allow more megaflow wildcarding. All other key
types than the tunnel key that can be set, can now be set with a mask.
It is not clear wether masked set is useful for skb_priority.
However, we already use the LSB of pkt_mark for IPSec in tunnels, so
it might be useful to be able to set individual bits on pkt_mark.
This looks great!
+ if (mask) {
+ ether_addr_copy_masked(eth_hdr(skb)->h_source, key->eth_src,
+ mask->eth_src);
+ ether_addr_copy_masked(eth_hdr(skb)->h_dest, key->eth_dst,
+ mask->eth_dst);
+ } else {
+ ether_addr_copy(eth_hdr(skb)->h_source, key->eth_src);
+ ether_addr_copy(eth_hdr(skb)->h_dest, key->eth_dst);
+ }
Quite a few of these branches are introduced in the fast path. Might
be worth checking 'perf stat' quickly to see if this generates a lot
of branch misses for either the wildcards enabled or the wildcards
disabled case. It might in fact be faster to always assume a mask if
the compiler cannot predict this correctly.
@@ -1281,13 +1294,17 @@ static int validate_set(const struct nlattr *a,
{
const struct nlattr *ovs_key = nla_data(a);
int key_type = nla_type(ovs_key);
+ bool have_mask;
/* There can be only one key in a action */
if (nla_total_size(nla_len(ovs_key)) != nla_len(a))
return -EINVAL;
+ have_mask = (ovs_key_lens[key_type] * 2 == nla_len(ovs_key));
+
key_type is not validated at this point and might be > OVS_KEY_ATTR_MAX
and thus access ovs_key_lens[] out of bound.
if (key_type > OVS_KEY_ATTR_MAX ||
(ovs_key_lens[key_type] != nla_len(ovs_key) &&
+(!have_mask || !validate_masked(nla_data(ovs_key), nla_len(ovs_key)))
&&
ovs_key_lens[key_type] != -1))
return -EINVAL;
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/4] datapath: Add flow mask cache.
On 04/08/2014 12:00 AM, Pravin wrote:
From: Pravin Shelar
On every packet OVS needs to lookup flow-table with every mask.
the packet flow-key is first masked with mask in the list and
then the masked key is looked up in flow-table. Therefore number
of masks can affect packet processing performance.
Following patch adds mask pointer to mask cache from last
pakcet lookup in same flow. Index of mask is stored in
this cache. This cache is indexed by 5 tuple hash (skb rxhash).
Signed-off-by: Pravin B Shelar
---
datapath/datapath.c |3 +-
datapath/flow_table.c | 80 -
datapath/flow_table.h | 11 +--
3 files changed, 83 insertions(+), 11 deletions(-)
diff --git a/datapath/datapath.c b/datapath/datapath.c
index f4e415e..c5059e1 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -262,7 +262,8 @@ void ovs_dp_process_received_packet(struct vport *p, struct
sk_buff *skb)
}
/* Look up flow. */
- flow = ovs_flow_tbl_lookup_stats(&dp->table, &key, &n_mask_hit);
+ flow = ovs_flow_tbl_lookup_stats(&dp->table, &key, skb_get_rxhash(skb),
+&n_mask_hit);
if (unlikely(!flow)) {
struct dp_upcall_info upcall;
diff --git a/datapath/flow_table.c b/datapath/flow_table.c
index 75c1b82..f12dd81 100644
--- a/datapath/flow_table.c
+++ b/datapath/flow_table.c
@@ -49,6 +49,10 @@
#define TBL_MIN_BUCKETS 1024
#define REHASH_INTERVAL (10 * 60 * HZ)
+#define MC_HASH_SHIFT 8
+#define MC_HASH_ENTRIES(1u << MC_HASH_SHIFT)
+#define MC_HASH_SEGS ((sizeof(uint32_t) * 8) / MC_HASH_SHIFT)
A comment describing this cache with some ascii art and stuff
would help tremendously. Took me 30 minutes to figure it out ;-)
+struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *tbl,
+ const struct sw_flow_key *key,
+ u32 skb_hash,
+ u32 *n_mask_hit)
+{
+ struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
+ struct mask_cache_entry *entries, *ce, *del;
+ struct sw_flow *flow;
+ u32 hash = skb_hash;
+ int seg;
+
+ del = NULL;
+ entries = this_cpu_ptr(tbl->mask_cache);
+
+ for (seg = 0; seg < MC_HASH_SEGS; seg++) {
+ int index;
+
+ index = hash & (MC_HASH_ENTRIES - 1);
+ ce = &entries[index];
+
+ if (ce->skb_hash == skb_hash) {
I believe skb_get_hash() can be 0 and that would result in mistaking any
empty slot to be a cached entry and we would only look at the first flow
mask.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/4] datapath: Add flow mask cache.
On 04/08/2014 12:00 AM, Pravin wrote:
+struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *tbl,
+ const struct sw_flow_key *key,
+ u32 skb_hash,
+ u32 *n_mask_hit)
+{
+ struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
+ struct mask_cache_entry *entries, *ce, *del;
+ struct sw_flow *flow;
+ u32 hash = skb_hash;
+ int seg;
+
+ del = NULL;
+ entries = this_cpu_ptr(tbl->mask_cache);
+
+ for (seg = 0; seg < MC_HASH_SEGS; seg++) {
+ int index;
+
+ index = hash & (MC_HASH_ENTRIES - 1);
+ ce = &entries[index];
+
+ if (ce->skb_hash == skb_hash) {
+ struct sw_flow_mask *mask;
+ int i;
+
+ i = 0;
+ list_for_each_entry_rcu(mask, &tbl->mask_list, list) {
+ if (ce->mask_index == i++) {
+ flow = masked_flow_lookup(ti, key,
mask);
+ if (flow) /* Found */
+ return flow;
+
+ ce->skb_hash = 0;
One more comment, perhaps break out of the loop as it should be
impossible to hit another cache entry at this point?
Setting del = ce before breaking out would allow to cache the flow
mask that applies again right away, ce is guarantee to be unused
at this point.
+ }
+ }
+ }
+
+ if (!del || (del->skb_hash && !ce->skb_hash)) {
+ del = ce;
+ }
+
+ hash >>= MC_HASH_SHIFT;
+ }
+ flow = flow_lookup(tbl, ti, key, n_mask_hit);
+
+ if (flow) {
+ del->skb_hash = skb_hash;
+ del->mask_index = (*n_mask_hit - 1);
+ }
+ return flow;
+}
+
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [openflow-discuss] Quick experimenting with OVS on OpenWrt
2014-04-08 18:51 GMT-04:00 Nicholas Bastin : > There are two variants of this device - one with an AR9132 (boring white > case), and one with a QCA9558 (blue spaceship-looking thing). The AR9132 > version uses a separate switch ASIC (AR8316 - I have one of these), so your > max throughput through that device in any software forwarding (regardless of > CPU speed) will be a gigabit. The 400Mhz ARM in that box is going to limit > you even further, so 450Mbits seems a bit suspect. > We have five of the TL-WR1043ND v1 boring white boxes. They have Realtek RTL8366RB[0] switch IC if a photo[1] on the openwrt wiki is to be believed. I don't have my screwdrivers with me to open one up and confirm, not that my professor would be okay with it ;) Also, the AR9132 is mips not arm if $(uname -m) is to be believed. Do you have any suggestions of how to confirm that the 450Mbit/s is not a false result? I am still a bit disbelieving. I did set-fail-mode secure and confirmed that no traffic passed with empty flow tables. [0] http://realtek.info/pdf/rtl8366_8369_datasheet_1-1.pdf [1] http://wiki.openwrt.org/_media/inbox/tl-wr1043nd_v14.jpg?cache= Cheers, -- Alison Chan chan7...@kettering.edu SMS: +1 909 278 7753 Fax: +1 206 203 2793 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Is your child's financial future secure?
*Life Insurance policy can last a lifetime, as long as premiums are paid. Coverage doubles during the year your child is 18, with no increase in premium. For children 14 days to 14 years old. Gerber Life ?? Continuing the long Gerber tradition of helping parents care for their children since 1967. You are receiving this email because you registered online to receive information. If you no longer wish to receive email from Gerber Life Insurance Company, click here. Links: 1. http://domain.com/ungerber.php 1. http://domain.com/gerber.php?em=%recipient% 1. http://domain.com/gerber.php?em=%recipient% 1. http://domain.com/gerber.php?em=%recipient% You may receive a Child ID =SAFETY= Card without applying for insurance by writing to us: Gerber Life Insurance Company, 445 State Street, Fremont, MI 49412. The retail value of this item is in accord with your state??s requirements. Policy Form Series GPP-08 Copyright ?? 2013 Gerber Life Insurance Company 1311 Mamaroneck Avenue White Plains, NY 10605 Call toll-free 1-800-503-4484 EMK-4269___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] bridge: improve vlan mode related error messages when adding port
Inform about fallback to trunk mode and convert errors to warnings
when we are not failing.
Signed-off-by: Thomas Graf
---
vswitchd/bridge.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 24b3602..c6095b2 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -839,14 +839,15 @@ port_configure(struct port *port)
s.vlan_mode = PORT_VLAN_NATIVE_UNTAGGED;
} else {
/* This "can't happen" because ovsdb-server should prevent it. */
-VLOG_ERR("unknown VLAN mode %s", cfg->vlan_mode);
+VLOG_WARN("port %s: unknown VLAN mode %s, falling back to trunk
mode",
+ port->name, cfg->vlan_mode);
s.vlan_mode = PORT_VLAN_TRUNK;
}
} else {
if (s.vlan >= 0) {
s.vlan_mode = PORT_VLAN_ACCESS;
if (cfg->n_trunks) {
-VLOG_ERR("port %s: ignoring trunks in favor of implicit vlan",
+VLOG_WARN("port %s: ignoring trunks in favor of implicit vlan",
port->name);
}
} else {
--
1.8.3.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovs-dev.py: Update repository location.
Does this mean we send in pull requests from github and no more patches on mailing lists? Regards, Pritesh On Apr 8, 2014, at 5:04 PM, Ben Pfaff wrote: > Applied to master, thanks! > > On Tue, Apr 08, 2014 at 04:46:19PM -0700, Justin Pettit wrote: >> Acked-by: Justin Pettit >> >> >> >> On April 8, 2014 at 4:43:09 PM, Ben Pfaff (b...@nicira.com) wrote: >>> The Open vSwitch repository has moved to Github. >>> >>> CC: Ethan Jackson >>> Signed-off-by: Ben Pfaff >>> --- >>> utilities/ovs-dev.py | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/utilities/ovs-dev.py b/utilities/ovs-dev.py >>> index 3f32e58..f73899b 100755 >>> --- a/utilities/ovs-dev.py >>> +++ b/utilities/ovs-dev.py >>> @@ -1,5 +1,5 @@ >>> #!/usr/bin/python >>> -# Copyright (c) 2013 Nicira, Inc. >>> +# Copyright (c) 2013, 2014 Nicira, Inc. >>> # >>> # Licensed under the Apache License, Version 2.0 (the "License"); >>> # you may not use this file except in compliance with the License. >>> @@ -275,7 +275,7 @@ Basic Configuration: >>> libssl-dev gdb linux-headers-`uname -r` >>> >>> # Next clone the Open vSwitch source. >>> - git clone git://git.openvswitch.org/openvswitch %(ovs)s >>> + git clone https://github.com/openvswitch/ovs.git %(ovs)s >>> >>> # Setup environment variables. >>> `%(v)s env` >>> -- >>> 1.7.10.4 >>> >>> ___ >>> dev mailing list >>> dev@openvswitch.org >>> http://openvswitch.org/mailman/listinfo/dev >>> >> > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] datapath: Orphan frags in skb_zerocopy and handle errors
This is the ported version of commit 36d5fe6a with the same name from net-next.
Apart from the small datapath.c changes it adjust the compat layer files as
well. This is the original commit message:
"skb_zerocopy can copy elements of the frags array between skbs, but it doesn't
orphan them. Also, it doesn't handle errors, so this patch takes care of that
as well, and modify the callers accordingly. skb_tx_error() is also added to
the callers so they will signal the failed delivery towards the creator of the
skb."
Signed-off-by: Zoltan Kiss
---
diff --git a/datapath/datapath.c b/datapath/datapath.c
index 25edd7d..5e5aa71 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -485,7 +485,9 @@ static int queue_userspace_packet(struct datapath *dp,
struct sk_buff *skb,
}
nla->nla_len = nla_attr_size(skb->len);
- skb_zerocopy(user_skb, skb, skb->len, hlen);
+ err = skb_zerocopy(user_skb, skb, skb->len, hlen);
+ if (err)
+ goto out;
/* Pad OVS_PACKET_ATTR_PACKET if linear copy was performed */
if (!(dp->user_features & OVS_DP_F_UNALIGNED)) {
@@ -499,6 +501,8 @@ static int queue_userspace_packet(struct datapath *dp,
struct sk_buff *skb,
err = genlmsg_unicast(ovs_dp_get_net(dp), user_skb,
upcall_info->portid);
out:
+ if (err)
+ skb_tx_error(skb);
kfree_skb(nskb);
return err;
}
diff --git a/datapath/linux/compat/include/linux/skbuff.h
b/datapath/linux/compat/include/linux/skbuff.h
index de0c56a..c709aca 100644
--- a/datapath/linux/compat/include/linux/skbuff.h
+++ b/datapath/linux/compat/include/linux/skbuff.h
@@ -260,7 +260,7 @@ static inline __u32 skb_get_rxhash(struct sk_buff *skb)
#if LINUX_VERSION_CODE < KERNEL_VERSION(3,14,0)
unsigned int skb_zerocopy_headlen(const struct sk_buff *from);
-void skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len,
+int skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len,
int hlen);
#endif
diff --git a/datapath/linux/compat/skbuff-openvswitch.c
b/datapath/linux/compat/skbuff-openvswitch.c
index ddd7bc8..96c27d0 100644
--- a/datapath/linux/compat/skbuff-openvswitch.c
+++ b/datapath/linux/compat/skbuff-openvswitch.c
@@ -61,25 +61,31 @@ skb_zerocopy_headlen(const struct sk_buff *from)
*
* The `hlen` as calculated by skb_zerocopy_headlen() specifies the
* headroom in the `to` buffer.
+ *
+ * Return value:
+ * 0: everything is OK
+ * -ENOMEM: couldn't orphan frags of @from due to lack of memory
+ * -EFAULT: skb_copy_bits() found some problem with skb geometry
*/
-void
-skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen)
+int
+skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len, int hlen)
{
int i, j = 0;
int plen = 0; /* length of skb->head fragment */
+ int ret;
struct page *page;
unsigned int offset;
BUG_ON(!head_frag(from) && !hlen);
/* dont bother with small payloads */
- if (len <= skb_tailroom(to)) {
- skb_copy_bits(from, 0, skb_put(to, len), len);
- return;
- }
+ if (len <= skb_tailroom(to))
+ return skb_copy_bits(from, 0, skb_put(to, len), len);
if (hlen) {
- skb_copy_bits(from, 0, skb_put(to, hlen), hlen);
+ ret = skb_copy_bits(from, 0, skb_put(to, hlen), hlen);
+ if (unlikely(ret))
+ return ret;
len -= hlen;
} else {
plen = min_t(int, skb_headlen(from), len);
@@ -97,6 +103,11 @@ skb_zerocopy(struct sk_buff *to, const struct sk_buff
*from, int len, int hlen)
to->len += len + plen;
to->data_len += len + plen;
+ if (unlikely(skb_orphan_frags(from, GFP_ATOMIC))) {
+ skb_tx_error(from);
+ return -ENOMEM;
+ }
+
for (i = 0; i < skb_shinfo(from)->nr_frags; i++) {
if (!len)
break;
@@ -107,5 +118,7 @@ skb_zerocopy(struct sk_buff *to, const struct sk_buff
*from, int len, int hlen)
j++;
}
skb_shinfo(to)->nr_frags = j;
+
+ return 0;
}
#endif
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Orphan frags in skb_zerocopy and handle errors
I don't know if Kyle posted this already, but I needed it anyway, so I
sent in my version.
Zoli
On 09/04/14 17:20, Zoltan Kiss wrote:
This is the ported version of commit 36d5fe6a with the same name from net-next.
Apart from the small datapath.c changes it adjust the compat layer files as
well. This is the original commit message:
"skb_zerocopy can copy elements of the frags array between skbs, but it doesn't
orphan them. Also, it doesn't handle errors, so this patch takes care of that
as well, and modify the callers accordingly. skb_tx_error() is also added to
the callers so they will signal the failed delivery towards the creator of the
skb."
Signed-off-by: Zoltan Kiss
---
diff --git a/datapath/datapath.c b/datapath/datapath.c
index 25edd7d..5e5aa71 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -485,7 +485,9 @@ static int queue_userspace_packet(struct datapath *dp,
struct sk_buff *skb,
}
nla->nla_len = nla_attr_size(skb->len);
- skb_zerocopy(user_skb, skb, skb->len, hlen);
+ err = skb_zerocopy(user_skb, skb, skb->len, hlen);
+ if (err)
+ goto out;
/* Pad OVS_PACKET_ATTR_PACKET if linear copy was performed */
if (!(dp->user_features & OVS_DP_F_UNALIGNED)) {
@@ -499,6 +501,8 @@ static int queue_userspace_packet(struct datapath *dp,
struct sk_buff *skb,
err = genlmsg_unicast(ovs_dp_get_net(dp), user_skb,
upcall_info->portid);
out:
+ if (err)
+ skb_tx_error(skb);
kfree_skb(nskb);
return err;
}
diff --git a/datapath/linux/compat/include/linux/skbuff.h
b/datapath/linux/compat/include/linux/skbuff.h
index de0c56a..c709aca 100644
--- a/datapath/linux/compat/include/linux/skbuff.h
+++ b/datapath/linux/compat/include/linux/skbuff.h
@@ -260,7 +260,7 @@ static inline __u32 skb_get_rxhash(struct sk_buff *skb)
#if LINUX_VERSION_CODE < KERNEL_VERSION(3,14,0)
unsigned int skb_zerocopy_headlen(const struct sk_buff *from);
-void skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len,
+int skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len,
int hlen);
#endif
diff --git a/datapath/linux/compat/skbuff-openvswitch.c
b/datapath/linux/compat/skbuff-openvswitch.c
index ddd7bc8..96c27d0 100644
--- a/datapath/linux/compat/skbuff-openvswitch.c
+++ b/datapath/linux/compat/skbuff-openvswitch.c
@@ -61,25 +61,31 @@ skb_zerocopy_headlen(const struct sk_buff *from)
*
*The `hlen` as calculated by skb_zerocopy_headlen() specifies the
*headroom in the `to` buffer.
+ *
+ * Return value:
+ * 0: everything is OK
+ * -ENOMEM: couldn't orphan frags of @from due to lack of memory
+ * -EFAULT: skb_copy_bits() found some problem with skb geometry
*/
-void
-skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen)
+int
+skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len, int hlen)
{
int i, j = 0;
int plen = 0; /* length of skb->head fragment */
+ int ret;
struct page *page;
unsigned int offset;
BUG_ON(!head_frag(from) && !hlen);
/* dont bother with small payloads */
- if (len <= skb_tailroom(to)) {
- skb_copy_bits(from, 0, skb_put(to, len), len);
- return;
- }
+ if (len <= skb_tailroom(to))
+ return skb_copy_bits(from, 0, skb_put(to, len), len);
if (hlen) {
- skb_copy_bits(from, 0, skb_put(to, hlen), hlen);
+ ret = skb_copy_bits(from, 0, skb_put(to, hlen), hlen);
+ if (unlikely(ret))
+ return ret;
len -= hlen;
} else {
plen = min_t(int, skb_headlen(from), len);
@@ -97,6 +103,11 @@ skb_zerocopy(struct sk_buff *to, const struct sk_buff
*from, int len, int hlen)
to->len += len + plen;
to->data_len += len + plen;
+ if (unlikely(skb_orphan_frags(from, GFP_ATOMIC))) {
+ skb_tx_error(from);
+ return -ENOMEM;
+ }
+
for (i = 0; i < skb_shinfo(from)->nr_frags; i++) {
if (!len)
break;
@@ -107,5 +118,7 @@ skb_zerocopy(struct sk_buff *to, const struct sk_buff
*from, int len, int hlen)
j++;
}
skb_shinfo(to)->nr_frags = j;
+
+ return 0;
}
#endif
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovs-dev.py: Update repository location.
On Wed, Apr 09, 2014 at 03:57:45PM +, Pritesh Kothari (pritkoth) wrote: > Does this mean we send in pull requests from github and no more > patches on mailing lists? It isn't clear yet how pull requests fit in with the Open vSwitch workflow. If people start sending them, then we'll start reviewing them, as an experiment. If they work well, we'll encourage people who like them to use them. Otherwise, we'll ask people to just use the mailing lists. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovs-dev.py: Update repository location.
On Wed, Apr 09, 2014 at 09:37:24AM -0700, Ben Pfaff wrote: > On Wed, Apr 09, 2014 at 03:57:45PM +, Pritesh Kothari (pritkoth) wrote: > > Does this mean we send in pull requests from github and no more > > patches on mailing lists? > > It isn't clear yet how pull requests fit in with the Open vSwitch > workflow. If people start sending them, then we'll start reviewing > them, as an experiment. If they work well, we'll encourage people who > like them to use them. Otherwise, we'll ask people to just use the > mailing lists. In fact, I see that we have one pull request already: https://github.com/openvswitch/ovs/pull/1 It's in an area that Guru usually looks at, so I assigned it to him. I guess that github probably notified you already, Guru, but if not will you take a look at this? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] bridge: improve vlan mode related error messages when adding port
On Wed, Apr 09, 2014 at 05:19:17PM +0200, Thomas Graf wrote: > Inform about fallback to trunk mode and convert errors to warnings > when we are not failing. > > Signed-off-by: Thomas Graf I'm pretty sure that the first one, as the comment says, really can't happen. Have you been able to trigger it somehow? The second change is OK by me. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] bridge: improve vlan mode related error messages when adding port
On Wed, Apr 09, 2014 at 09:52:03AM -0700, Ben Pfaff wrote: > On Wed, Apr 09, 2014 at 05:19:17PM +0200, Thomas Graf wrote: > > Inform about fallback to trunk mode and convert errors to warnings > > when we are not failing. > > > > Signed-off-by: Thomas Graf > > I'm pretty sure that the first one, as the comment says, really can't > happen. Have you been able to trigger it somehow? > > The second change is OK by me. On second thought, I'm happy enough with these as warnings, so I applied this after touching up line lengths. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: Remove the flow_dumper thread.
On Wed, Apr 09, 2014 at 04:36:50PM +1200, Joe Stringer wrote:
> Thanks Alex.
>
> There is also one minor fix to be rolled in:
>
> @@ -1403,8 +1416,8 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key
> *ukey,
> compose_slow_path(udpif, &xout, odp_in_port, &xout_actions);
> }
>
> -if (actions_len != xout_actions.size
> -|| memcmp(xout_actions.data, actions, actions_len)) {
> +if (actions_len != ofpbuf_size(&xout_actions)
> +|| memcmp(ofpbuf_data(&xout_actions), actions, actions_len)) {
> goto exit;
> }
>
> I will wait for further review. Let me know if I should repost.
Repost with the fix and I'll review it.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] recirc: preserve packet metadata fields for recirculation
On Tue, Apr 08, 2014 at 01:37:04PM -0700, Andy Zhou wrote: > Packet metadata fields may be modified when execute post recirculation > actions. Current implementation only preserves recirc_id and dp_hash, > but not other fields. With this patch, a copy of metadata is supplied > instead of the original metadata for recirculation to preserve > the original value. I think I understand the change here. Previously, if the actions executed during recirculation changed metadata fields, then any actions after the recirculation returns would see those new values. Now, all metadata are saved and restored across a recirculation. Right? Data fields are already saved and restored across recirculation, both before and after this patch, correct? Needs a sign-off. I guess I would prefix the first line as "dpif-netdev:" not "recirc:". If I'm right about the above: Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] dpif-netdev: Use existing flow for computing dp hash
Needs a sign-off. Does this visibly change any behavior? If not: Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 01/10] lib/ofp-actions: Silently discard set ip ecn/ttl actions on OpenFlow10.
On Tue, Apr 08, 2014 at 04:38:43PM -0700, Jarno Rajahalme wrote: > It is better to not abort(). > > Signed-off-by: Jarno Rajahalme Please backport as far as necessary. Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 02/10] ofproto: Fix wildcard masking with nw_tos.
On Tue, Apr 08, 2014 at 04:38:44PM -0700, Jarno Rajahalme wrote: > Later patches rely on the DSCP and ECN masks to be properly set when > the fields are read. Also, avoid reading nw_tos if tunnel's inner > packet is not IP. > > Signed-off-by: Jarno Rajahalme I guess that these don't really cause much in the way of real bugs, so backporting might not be worth it? Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 03/10] Tests: Fix ofproto/trace and expose megaflows when having a set action.
On Tue, Apr 08, 2014 at 04:38:45PM -0700, Jarno Rajahalme wrote: > ofproto/trace incorrectly reported the megaflow based on the modified > flow, rather than the original flow key. Now the original flow key is > stored before any modifications and is used for reporting the megaflow. > > Also, flow reporting is suppressed only for resubmit flows, so that > the final flow will be printed if it is different from the incoming > flow key. > > Test for the megaflow key and mask with flows having set actions. > This helps in verifying the correctness of operation with masked set > actions in the following patches. > > Signed-off-by: Jarno Rajahalme This one failed to apply for me, but the commit message makes sense and the change is straightforward. Thanks Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 04/10] ofproto/xlate: Fix set field unwildcarding.
On Tue, Apr 08, 2014 at 04:38:46PM -0700, Jarno Rajahalme wrote: > If the field does not exist, nothing is set. However, we must > unwildcard the bits we used to make the decision, and we need not > unwildcard the field and it's prerequisities, if nothing is set. > > Signed-off-by: Jarno Rajahalme This one might need a backport for correctness? I suspect that we should treat MFF_MPLS_BOS the same as the other MPLS fields. Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] recirc: preserve packet metadata fields for recirculation
On Wed, Apr 9, 2014 at 10:11 AM, Ben Pfaff wrote: > On Tue, Apr 08, 2014 at 01:37:04PM -0700, Andy Zhou wrote: >> Packet metadata fields may be modified when execute post recirculation >> actions. Current implementation only preserves recirc_id and dp_hash, >> but not other fields. With this patch, a copy of metadata is supplied >> instead of the original metadata for recirculation to preserve >> the original value. > > I think I understand the change here. Previously, if the actions > executed during recirculation changed metadata fields, then any > actions after the recirculation returns would see those new values. > Now, all metadata are saved and restored across a recirculation. > Right? Correct. I will change the commit message to use your description above since it is clearer. > > Data fields are already saved and restored across recirculation, both > before and after this patch, correct? Yes, A flow will be constructed at the beginning of each recirculation, similar to a new packets received off the wire. The current flow starting the recirc action will continue to be used after recirculation. > > Needs a sign-off. > > I guess I would prefix the first line as "dpif-netdev:" not "recirc:". > Will fix both. > If I'm right about the above: > Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] dpif-netdev: Use existing flow for computing dp hash
On Wed, Apr 9, 2014 at 10:12 AM, Ben Pfaff wrote: > Needs a sign-off. > will fix > Does this visibly change any behavior? If not: No. > Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovs-dev.py: Update repository location.
On Wed, Apr 9, 2014 at 9:48 AM, Ben Pfaff wrote: > On Wed, Apr 09, 2014 at 09:37:24AM -0700, Ben Pfaff wrote: >> On Wed, Apr 09, 2014 at 03:57:45PM +, Pritesh Kothari (pritkoth) wrote: >> > Does this mean we send in pull requests from github and no more >> > patches on mailing lists? >> >> It isn't clear yet how pull requests fit in with the Open vSwitch >> workflow. If people start sending them, then we'll start reviewing >> them, as an experiment. If they work well, we'll encourage people who >> like them to use them. Otherwise, we'll ask people to just use the >> mailing lists. > > In fact, I see that we have one pull request already: > https://github.com/openvswitch/ovs/pull/1 > > It's in an area that Guru usually looks at, so I assigned it to him. > I guess that github probably notified you already, Guru, but if not > will you take a look at this? The change is fine. The contributor hasn't added a Signed-off-by line. I can drop a comment there. But, how do we synchronize comments between there and this list. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 05/10] lib/odp: Masked set action execution and printing.
On Tue, Apr 08, 2014 at 04:38:47PM -0700, Jarno Rajahalme wrote:
> Masked set actions add a mask, immediately following the netlink
> attribute data, within the netlink attribute itself. Thus the key
> attribute size for a masked set action is exactly double of the
> non-masked set action.
>
> Signed-off-by: Jarno Rajahalme
This adds a new restriction to key attributes: they must have a fixed
length (because otherwise one cannot tell whether the length is
doubled). In fact, MPLS is already variable length, as documented in
:
OVS_KEY_ATTR_MPLS = 62, /* array of struct ovs_key_mpls.
* The implementation may restrict
* the accepted length of the array. */
So I'd suggest some other way to distinguish masked set actions.
In general the code looks good but it has more repetition than I
usually like. Is there a good way to avoid some of that? Maybe some
pattern like this (not tested):
void
memcpy_masked(void *dst_, const void *src_, const void *mask_, size_t size)
{
uint8_t *dst = dst_;
const uint8_t *src = src_;
const uint8_t *mask = mask_;
size_t i;
if (mask) {
for (i = 0; i < size; i++) {
dst[i] = src[i] | (dst[i] & ~mask[i]);
}
} else {
memcpy(dst, src, size);
}
}
#define copy_masked(dst, src, mask) \
((void) sizeof((dst) == (src)), \
(void) sizeof((dst) == (mask)),\
memcpy_masked(dst, src, mask))
#define get_mask(a, type) \
(nl_attr_get_size(a) >= 2 * sizeof(type)\
? (const type *) nl_attr_get(a) + 1\
: NULL)
...
switch (type) {
case OVS_KEY_ATTR_PRIORITY:
copy_masked(&md->skb_priority,
nl_attr_get_u32(a), get_mask(a, uint32_t));
break;
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovs-dev.py: Update repository location.
On Wed, Apr 09, 2014 at 10:39:31AM -0700, Gurucharan Shetty wrote: > On Wed, Apr 9, 2014 at 9:48 AM, Ben Pfaff wrote: > > On Wed, Apr 09, 2014 at 09:37:24AM -0700, Ben Pfaff wrote: > >> On Wed, Apr 09, 2014 at 03:57:45PM +, Pritesh Kothari (pritkoth) wrote: > >> > Does this mean we send in pull requests from github and no more > >> > patches on mailing lists? > >> > >> It isn't clear yet how pull requests fit in with the Open vSwitch > >> workflow. If people start sending them, then we'll start reviewing > >> them, as an experiment. If they work well, we'll encourage people who > >> like them to use them. Otherwise, we'll ask people to just use the > >> mailing lists. > > > > In fact, I see that we have one pull request already: > > https://github.com/openvswitch/ovs/pull/1 > > > > It's in an area that Guru usually looks at, so I assigned it to him. > > I guess that github probably notified you already, Guru, but if not > > will you take a look at this? > > The change is fine. > The contributor hasn't added a Signed-off-by line. I can drop a > comment there. But, how do we synchronize comments between there and > this list. I'm not sure. One possibility: create a Github user for ovs-dev, with dev@openvswitch.org as its email address, and configure that account to get all notifications for the ovs repository. Do you think that is worth a try? (It has one fairly obvious downside that, nevertheless, I don't want to point out publicly.) ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 06/10] ofproto: Probe for masked set action support.
On Tue, Apr 08, 2014 at 04:38:48PM -0700, Jarno Rajahalme wrote: > Signed-off-by: Jarno Rajahalme Does the masked set action cause EINVAL or ERANGE on older kernels. Comments say EINVAL in two places, ERANGE in one, and the code actually looks for EINVAL. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 03/10] Tests: Fix ofproto/trace and expose megaflows when having a set action.
Pushed, I hope it applies now! Jarno On Apr 9, 2014, at 10:22 AM, Ben Pfaff wrote: > On Tue, Apr 08, 2014 at 04:38:45PM -0700, Jarno Rajahalme wrote: >> ofproto/trace incorrectly reported the megaflow based on the modified >> flow, rather than the original flow key. Now the original flow key is >> stored before any modifications and is used for reporting the megaflow. >> >> Also, flow reporting is suppressed only for resubmit flows, so that >> the final flow will be printed if it is different from the incoming >> flow key. >> >> Test for the megaflow key and mask with flows having set actions. >> This helps in verifying the correctness of operation with masked set >> actions in the following patches. >> >> Signed-off-by: Jarno Rajahalme > > This one failed to apply for me, but the commit message makes sense > and the change is straightforward. Thanks > > Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 02/10] ofproto: Fix wildcard masking with nw_tos.
On Apr 9, 2014, at 10:19 AM, Ben Pfaff wrote: > On Tue, Apr 08, 2014 at 04:38:44PM -0700, Jarno Rajahalme wrote: >> Later patches rely on the DSCP and ECN masks to be properly set when >> the fields are read. Also, avoid reading nw_tos if tunnel's inner >> packet is not IP. >> >> Signed-off-by: Jarno Rajahalme > > I guess that these don't really cause much in the way of real bugs, so > backporting might not be worth it? > I agree. Pushed to master, Jarno > Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 01/10] lib/ofp-actions: Silently discard set ip ecn/ttl actions on OpenFlow10.
Pushed to master and branch-2.1, Thanks, Jarno On Apr 9, 2014, at 10:14 AM, Ben Pfaff wrote: > On Tue, Apr 08, 2014 at 04:38:43PM -0700, Jarno Rajahalme wrote: >> It is better to not abort(). >> >> Signed-off-by: Jarno Rajahalme > > Please backport as far as necessary. > > Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] recirc: preserve packet metadata fields for recirculation
Pushed. Thanks. On Wed, Apr 9, 2014 at 10:34 AM, Andy Zhou wrote: > On Wed, Apr 9, 2014 at 10:11 AM, Ben Pfaff wrote: >> On Tue, Apr 08, 2014 at 01:37:04PM -0700, Andy Zhou wrote: >>> Packet metadata fields may be modified when execute post recirculation >>> actions. Current implementation only preserves recirc_id and dp_hash, >>> but not other fields. With this patch, a copy of metadata is supplied >>> instead of the original metadata for recirculation to preserve >>> the original value. >> >> I think I understand the change here. Previously, if the actions >> executed during recirculation changed metadata fields, then any >> actions after the recirculation returns would see those new values. >> Now, all metadata are saved and restored across a recirculation. >> Right? > > Correct. I will change the commit message to use your description above > since it is clearer. > >> >> Data fields are already saved and restored across recirculation, both >> before and after this patch, correct? > Yes, A flow will be constructed at the beginning of each > recirculation, similar to > a new packets received off the wire. The current flow starting the recirc > action > will continue to be used after recirculation. >> >> Needs a sign-off. >> >> I guess I would prefix the first line as "dpif-netdev:" not "recirc:". >> > Will fix both. > >> If I'm right about the above: >> Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] dpif-netdev: Use existing flow for computing dp hash
Pushed. Thanks. On Wed, Apr 9, 2014 at 10:35 AM, Andy Zhou wrote: > On Wed, Apr 9, 2014 at 10:12 AM, Ben Pfaff wrote: >> Needs a sign-off. >> > will fix >> Does this visibly change any behavior? If not: > No. >> Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 04/10] ofproto/xlate: Fix set field unwildcarding.
Thanks for the review, pushed to master and branch-2.1 Jarno On Apr 9, 2014, at 10:25 AM, Ben Pfaff wrote: > On Tue, Apr 08, 2014 at 04:38:46PM -0700, Jarno Rajahalme wrote: >> If the field does not exist, nothing is set. However, we must >> unwildcard the bits we used to make the decision, and we need not >> unwildcard the field and it's prerequisities, if nothing is set. >> >> Signed-off-by: Jarno Rajahalme > > This one might need a backport for correctness? > > I suspect that we should treat MFF_MPLS_BOS the same as the other MPLS > fields. > MFF_MPLS_BOS is not writeable, so it need not be considered for SET FIELD. > Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 05/10] lib/odp: Masked set action execution and printing.
On Apr 9, 2014, at 10:51 AM, Ben Pfaff wrote:
> On Tue, Apr 08, 2014 at 04:38:47PM -0700, Jarno Rajahalme wrote:
>> Masked set actions add a mask, immediately following the netlink
>> attribute data, within the netlink attribute itself. Thus the key
>> attribute size for a masked set action is exactly double of the
>> non-masked set action.
>>
>> Signed-off-by: Jarno Rajahalme
>
> This adds a new restriction to key attributes: they must have a fixed
> length (because otherwise one cannot tell whether the length is
> doubled). In fact, MPLS is already variable length, as documented in
> :
>
> OVS_KEY_ATTR_MPLS = 62, /* array of struct ovs_key_mpls.
>* The implementation may restrict
>* the accepted length of the array. */
>
I totally missed this, maybe due to the current userspace datapath setting only
on lee, and linux kernel datapath not yet supporting MPLS.
> So I'd suggest some other way to distinguish masked set actions.
>
The obvious alternative is to use a second attribute of the same type and
length within the nested attributes of the set action. It is currently limited
to one. The additional structure will be somewhat repetitive, and we’ll have to
enforce that the key type and length are the same for both. To be more explicit
we could set the MSB of the type, but then we might also want to support mask
before the value, etc. That may be a bit too much flexibility, though.
Thoughts?
> In general the code looks good but it has more repetition than I
> usually like. Is there a good way to avoid some of that? Maybe some
> pattern like this (not tested):
>
Will consider this for the revision.
Jarno
> void
> memcpy_masked(void *dst_, const void *src_, const void *mask_, size_t size)
> {
>uint8_t *dst = dst_;
>const uint8_t *src = src_;
>const uint8_t *mask = mask_;
>size_t i;
>
>if (mask) {
>for (i = 0; i < size; i++) {
>dst[i] = src[i] | (dst[i] & ~mask[i]);
>}
>} else {
>memcpy(dst, src, size);
>}
> }
>
> #define copy_masked(dst, src, mask) \
>((void) sizeof((dst) == (src)), \
> (void) sizeof((dst) == (mask)),\
> memcpy_masked(dst, src, mask))
> #define get_mask(a, type) \
>(nl_attr_get_size(a) >= 2 * sizeof(type)\
> ? (const type *) nl_attr_get(a) + 1\
> : NULL)
>
> ...
>switch (type) {
>case OVS_KEY_ATTR_PRIORITY:
>copy_masked(&md->skb_priority,
>nl_attr_get_u32(a), get_mask(a, uint32_t));
>break;
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 07/10] lib/odp: Use masked set actions.
On Tue, Apr 08, 2014 at 04:38:49PM -0700, Jarno Rajahalme wrote:
> Signed-off-by: Jarno Rajahalme
Consider this code from commit_set_ether_addr_action():
bool have_mask_src = !eth_addr_is_zero(wc->masks.dl_src);
bool have_mask_dst = !eth_addr_is_zero(wc->masks.dl_dst);
...
memcpy(base->dl_src, flow->dl_src, ETH_ADDR_LEN);
memcpy(base->dl_dst, flow->dl_dst, ETH_ADDR_LEN);
/* Do not use mask if fully masked. */
if (use_masked && !(have_mask_src && have_mask_dst)) {
memcpy(mask.eth_src, wc->masks.dl_src, ETH_ADDR_LEN);
eth_addr_bitand(base->dl_src, wc->masks.dl_src, key.eth_src);
memcpy(mask.eth_dst, wc->masks.dl_dst, ETH_ADDR_LEN);
eth_addr_bitand(base->dl_dst, wc->masks.dl_dst, key.eth_dst);
commit_masked_set_action(odp_actions, OVS_KEY_ATTR_ETHERNET, &key,
&mask, sizeof key);
} else {
memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
memcpy(key.eth_src, base->dl_src, ETH_ADDR_LEN);
memcpy(key.eth_dst, base->dl_dst, ETH_ADDR_LEN);
commit_set_action(odp_actions, OVS_KEY_ATTR_ETHERNET, &key,
sizeof key);
}
First, !(have_mask_src && have_mask_dst) is stricter than it would
have to be, right? It sets all the bits in the Ethernet addresses if
at least one bit in dl_src needs to be set and at least one bit in
dl_dst needs to be set. It could, instead, set all the bits if *all*
the bits in dl_src and dl_dst need to be set. Now, I'm not sure that
it's worth doing that, since it's an extra check, but I do want to
make sure that I understand the logic.
Second, 'base' gets used a lot in the "if" statement. I guess that's
OK, because the previous memcpys mean that it has the same contents as
'flow' for dl_src and dl_dst, but 'base' is used only in special
situations (where the previous value of a field matters) so it stuck
out, and it took me some time to realize that it was in fact the same
as 'flow'. So, I'd prefer to see 'flow' used, where it can be, in
place of 'base'. I feel the same way about some other cases later,
e.g. commit_set_ipv4_action().
Finally, commit_masked_set_action() is used here and elsewhere in kind
of an awkward pattern, where the caller has to do masking of the key
against the mask. I think that commit_masked_set_action() could do
the masking as it copies the key, and then all of the callers would be
simpler. (If you don't take my advice on that, then I think that
commit_set_priority_action() and commit_set_pkt_mark_action() forgot
to do the masking.)
There is a doubled ';' in commit_set_ipv4_action():
+key.ipv4_src = base->nw_src & wc->masks.nw_src;;
The beginning of commit_set_ipv4_action() first asserts:
!((a ^ b) & ~c)
and then checks whether
(a ^ b) & c
for several instances of 'a', 'b', and 'c'. But if the assertions are
true, then I believe that the checks can be reduced to just
a != b
because we already know at that the bits not in 'c' are the same. (Am
I missing something?)
It seems to me that IPV6_MASKED could be written as a function. At
least, the uses of the macro arguments should be (parenthesized).
In commit_set_ipv6_action(), is the following mask check needed? I
doubt that it is much faster than the check that follows it, and if
that later checks passes (does not "return;") then we know that some
bits in the mask must be set. Same question for the early check in
commit_set_arp_action(), commit_set_port_action(), ...
/* Wildcard bits are set when we have either read or set the corresponding
* values. Nothing to be done if no bits are set. */
if (!(ipv6_mask_is_any(&wc->masks.ipv6_src) ||
ipv6_mask_is_any(&wc->masks.ipv6_dst) ||
wc->masks.ipv6_label || wc->masks.nw_tos || wc->masks.nw_ttl)) {
return;
}
commit_set_arp_action() may always use masked "set" operations,
regardless of 'use_masked', because ARP set operations are always
slow-pathed (see the "return SLOW_ACTION;" at the end of the
function).
Thanks,
Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 06/10] ofproto: Probe for masked set action support.
On Apr 9, 2014, at 11:03 AM, Ben Pfaff wrote: > On Tue, Apr 08, 2014 at 04:38:48PM -0700, Jarno Rajahalme wrote: >> Signed-off-by: Jarno Rajahalme > > Does the masked set action cause EINVAL or ERANGE on older kernels. > Comments say EINVAL in two places, ERANGE in one, and the code > actually looks for EINVAL. It would probably be cleaner to use masked actions only when probe succeeds, and keep the old behavior if any error value is returned. It seems I picked up the default case from an existing probe for the userspace action probe for fully considering the desired behavior in this case. Jarno ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 04/10] ofproto/xlate: Fix set field unwildcarding.
On Wed, Apr 09, 2014 at 01:11:40PM -0700, Jarno Rajahalme wrote: > On Apr 9, 2014, at 10:25 AM, Ben Pfaff wrote: > > > On Tue, Apr 08, 2014 at 04:38:46PM -0700, Jarno Rajahalme wrote: > >> If the field does not exist, nothing is set. However, we must > >> unwildcard the bits we used to make the decision, and we need not > >> unwildcard the field and it's prerequisities, if nothing is set. > >> > >> Signed-off-by: Jarno Rajahalme > > > > This one might need a backport for correctness? > > > > I suspect that we should treat MFF_MPLS_BOS the same as the other MPLS > > fields. > > > > MFF_MPLS_BOS is not writeable, so it need not be considered for SET FIELD. I see, thank you. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 05/10] lib/odp: Masked set action execution and printing.
On Wed, Apr 09, 2014 at 01:24:07PM -0700, Jarno Rajahalme wrote: > > On Apr 9, 2014, at 10:51 AM, Ben Pfaff wrote: > > > On Tue, Apr 08, 2014 at 04:38:47PM -0700, Jarno Rajahalme wrote: > >> Masked set actions add a mask, immediately following the netlink > >> attribute data, within the netlink attribute itself. Thus the key > >> attribute size for a masked set action is exactly double of the > >> non-masked set action. > >> > >> Signed-off-by: Jarno Rajahalme > > > > This adds a new restriction to key attributes: they must have a fixed > > length (because otherwise one cannot tell whether the length is > > doubled). In fact, MPLS is already variable length, as documented in > > : > > > > OVS_KEY_ATTR_MPLS = 62, /* array of struct ovs_key_mpls. > > * The implementation may restrict > > * the accepted length of the array. */ > > > > I totally missed this, maybe due to the current userspace datapath setting > only on lee, and linux kernel datapath not yet supporting MPLS. > > > So I'd suggest some other way to distinguish masked set actions. > > The obvious alternative is to use a second attribute of the same > type and length within the nested attributes of the set action. It > is currently limited to one. The additional structure will be > somewhat repetitive, and we?ll have to enforce that the key type and > length are the same for both. To be more explicit we could set the > MSB of the type, but then we might also want to support mask before > the value, etc. That may be a bit too much flexibility, though. > > Thoughts? One alternative would be to introduce OVS_ACTION_ATTR_SET_MASKED, would use the format you proposed in this patch except that the mask would always be present. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 08/10] openvswithch.h: Clarify use of key attributes.
On Tue, Apr 08, 2014 at 04:38:50PM -0700, Jarno Rajahalme wrote: > Key attributes relating to actual packet headers are ignored for > OVS_PACKET_CMD_EXECUTE as the header key attributes are retrieved > from the packet itself. > > Signed-off-by: Jarno Rajahalme There is a typo in the subject. - * flow key against the kernel's. + * flow key against the kernel's. When used with %OVS_PACKET_CMD_EXECUTE, + * other than metadata key fields (e.g., priority, skb mark) are ignored as + * they are parsed from the packet instead. I think that the new text actually adds to the confusion because it is not obvious to the reader whether the parenthesized examples are "metadata key fields" or "other than metadata key fields". Perhaps: When used with %OVS_PACKET_CMD_EXECUTE, only metadata key fields (e.g. priority, skb mark) are honored. All the fields that can be are instead parsed from the packet. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 09/10] datapath: Allow masks for set actions.
On Tue, Apr 08, 2014 at 04:38:51PM -0700, Jarno Rajahalme wrote: > Masked set actions allow more megaflow wildcarding. All other key > types than the tunnel key that can be set, can now be set with a mask. > > It is not clear wether masked set is useful for skb_priority. > However, we already use the LSB of pkt_mark for IPSec in tunnels, so > it might be useful to be able to set individual bits on pkt_mark. > > Signed-off-by: Jarno Rajahalme I'm skipping this datapath patch. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 10/10] Clarify tunnel wildcarding.
On Tue, Apr 08, 2014 at 04:38:52PM -0700, Jarno Rajahalme wrote: > It would seem that we should set the 'tun_dst' in 'wc' when calling > tnl_port_should_receive(), as it is reading that flow field. > > However, tnl_port_should_receive() returns true, if the flow has > tunnel metadata. If there is no tunnel metadata, then there is > nothing to mask, so we do not set the 'ip_dst' field in the 'wc' if > this test fails, even though we used that field to determine the > non-presence of the tunnel metadata. > > Datapath flow matching ensures that a key that does not include tunnel > metadata cannot match a tunneled packet. > > Signed-off-by: Jarno Rajahalme Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 07/10] lib/odp: Use masked set actions.
Ben,
Thanks for your review. Some comments below,
Jarno
On Apr 9, 2014, at 1:19 PM, Ben Pfaff wrote:
> On Tue, Apr 08, 2014 at 04:38:49PM -0700, Jarno Rajahalme wrote:
>> Signed-off-by: Jarno Rajahalme
>
> Consider this code from commit_set_ether_addr_action():
>
>bool have_mask_src = !eth_addr_is_zero(wc->masks.dl_src);
>bool have_mask_dst = !eth_addr_is_zero(wc->masks.dl_dst);
>
> ...
>
>memcpy(base->dl_src, flow->dl_src, ETH_ADDR_LEN);
>memcpy(base->dl_dst, flow->dl_dst, ETH_ADDR_LEN);
>
>/* Do not use mask if fully masked. */
>if (use_masked && !(have_mask_src && have_mask_dst)) {
>memcpy(mask.eth_src, wc->masks.dl_src, ETH_ADDR_LEN);
>eth_addr_bitand(base->dl_src, wc->masks.dl_src, key.eth_src);
>memcpy(mask.eth_dst, wc->masks.dl_dst, ETH_ADDR_LEN);
>eth_addr_bitand(base->dl_dst, wc->masks.dl_dst, key.eth_dst);
>
>commit_masked_set_action(odp_actions, OVS_KEY_ATTR_ETHERNET, &key,
> &mask, sizeof key);
>} else {
>memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
>memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
>
>memcpy(key.eth_src, base->dl_src, ETH_ADDR_LEN);
>memcpy(key.eth_dst, base->dl_dst, ETH_ADDR_LEN);
>
>commit_set_action(odp_actions, OVS_KEY_ATTR_ETHERNET, &key,
> sizeof key);
>}
>
> First, !(have_mask_src && have_mask_dst) is stricter than it would
> have to be, right? It sets all the bits in the Ethernet addresses if
> at least one bit in dl_src needs to be set and at least one bit in
> dl_dst needs to be set. It could, instead, set all the bits if *all*
> the bits in dl_src and dl_dst need to be set. Now, I'm not sure that
> it's worth doing that, since it's an extra check, but I do want to
> make sure that I understand the logic.
You are right, my intention was to “not use mask if fully masked”. The thought
here is that if ethernet addresses are set, it is most likely that both are
fully set at the same time, and it would be slightly faster in the datapath to
not have to bother about the mask. We could, however, let the datapath to worry
about this, as it is validating the actions. Indeed, it could be faster for the
datapath to implement field-spcific set actions internally for the typical
cases.
>
> Second, 'base' gets used a lot in the "if" statement. I guess that's
> OK, because the previous memcpys mean that it has the same contents as
> 'flow' for dl_src and dl_dst, but 'base' is used only in special
> situations (where the previous value of a field matters) so it stuck
> out, and it took me some time to realize that it was in fact the same
> as 'flow'. So, I'd prefer to see 'flow' used, where it can be, in
> place of 'base'. I feel the same way about some other cases later,
> e.g. commit_set_ipv4_action().
Will change.
>
> Finally, commit_masked_set_action() is used here and elsewhere in kind
> of an awkward pattern, where the caller has to do masking of the key
> against the mask. I think that commit_masked_set_action() could do
> the masking as it copies the key, and then all of the callers would be
> simpler. (If you don't take my advice on that, then I think that
> commit_set_priority_action() and commit_set_pkt_mark_action() forgot
> to do the masking.)
>
True, it will be simpler and cleaner.
> There is a doubled ';' in commit_set_ipv4_action():
> +key.ipv4_src = base->nw_src & wc->masks.nw_src;;
>
> The beginning of commit_set_ipv4_action() first asserts:
>!((a ^ b) & ~c)
> and then checks whether
>(a ^ b) & c
> for several instances of 'a', 'b', and 'c'. But if the assertions are
> true, then I believe that the checks can be reduced to just
>a != b
> because we already know at that the bits not in 'c' are the same. (Am
> I missing something?)
>
I had intended to remove the assertions. However, those have been valuable in
catching cases where flow values have been modified without setting the mask
bits. One way of keeping them around for testing purposes would be to piggyback
on the FLOW_WC_SEQ, in effect enabling the asserts every time FLOW_WC_SEQ is
bumped… other ideas?
> It seems to me that IPV6_MASKED could be written as a function. At
> least, the uses of the macro arguments should be (parenthesized).
>
> In commit_set_ipv6_action(), is the following mask check needed? I
> doubt that it is much faster than the check that follows it, and if
> that later checks passes (does not "return;") then we know that some
> bits in the mask must be set. Same question for the early check in
> commit_set_arp_action(), commit_set_port_action(), ...
>
>/* Wildcard bits are set when we have either read or set the corresponding
> * values. Nothing to be done if no bits are set. */
>if (!(ipv6_mask_is_any(&wc->masks.ipv6_src) ||
> ipv6_mask_is_any(&wc->masks.ipv6_dst) ||
> wc->masks.ipv6
Re: [ovs-dev] [PATCH 05/10] lib/odp: Masked set action execution and printing.
On Apr 9, 2014, at 1:26 PM, Ben Pfaff wrote: > On Wed, Apr 09, 2014 at 01:24:07PM -0700, Jarno Rajahalme wrote: >> >> On Apr 9, 2014, at 10:51 AM, Ben Pfaff wrote: >> >>> On Tue, Apr 08, 2014 at 04:38:47PM -0700, Jarno Rajahalme wrote: Masked set actions add a mask, immediately following the netlink attribute data, within the netlink attribute itself. Thus the key attribute size for a masked set action is exactly double of the non-masked set action. Signed-off-by: Jarno Rajahalme >>> >>> This adds a new restriction to key attributes: they must have a fixed >>> length (because otherwise one cannot tell whether the length is >>> doubled). In fact, MPLS is already variable length, as documented in >>> : >>> >>> OVS_KEY_ATTR_MPLS = 62, /* array of struct ovs_key_mpls. >>> * The implementation may restrict >>> * the accepted length of the array. */ >>> >> >> I totally missed this, maybe due to the current userspace datapath setting >> only on lee, and linux kernel datapath not yet supporting MPLS. >> >>> So I'd suggest some other way to distinguish masked set actions. >> >> The obvious alternative is to use a second attribute of the same >> type and length within the nested attributes of the set action. It >> is currently limited to one. The additional structure will be >> somewhat repetitive, and we?ll have to enforce that the key type and >> length are the same for both. To be more explicit we could set the >> MSB of the type, but then we might also want to support mask before >> the value, etc. That may be a bit too much flexibility, though. >> >> Thoughts? > > One alternative would be to introduce OVS_ACTION_ATTR_SET_MASKED, > would use the format you proposed in this patch except that the mask > would always be present. Thanks, I like this, as this will also make feature checking more explicit. Jarno ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 05/10] lib/odp: Masked set action execution and printing.
On Wed, Apr 9, 2014 at 1:51 PM, Jarno Rajahalme wrote: > > On Apr 9, 2014, at 1:26 PM, Ben Pfaff wrote: > > On Wed, Apr 09, 2014 at 01:24:07PM -0700, Jarno Rajahalme wrote: > > > On Apr 9, 2014, at 10:51 AM, Ben Pfaff wrote: > > On Tue, Apr 08, 2014 at 04:38:47PM -0700, Jarno Rajahalme wrote: > > Masked set actions add a mask, immediately following the netlink > attribute data, within the netlink attribute itself. Thus the key > attribute size for a masked set action is exactly double of the > non-masked set action. > > Signed-off-by: Jarno Rajahalme > > > This adds a new restriction to key attributes: they must have a fixed > length (because otherwise one cannot tell whether the length is > doubled). In fact, MPLS is already variable length, as documented in > : > > OVS_KEY_ATTR_MPLS = 62, /* array of struct ovs_key_mpls. > * The implementation may restrict > * the accepted length of the array. */ > > > I totally missed this, maybe due to the current userspace datapath setting > only on lee, and linux kernel datapath not yet supporting MPLS. > > So I'd suggest some other way to distinguish masked set actions. > > > The obvious alternative is to use a second attribute of the same > type and length within the nested attributes of the set action. It > is currently limited to one. The additional structure will be > somewhat repetitive, and we?ll have to enforce that the key type and > length are the same for both. To be more explicit we could set the > MSB of the type, but then we might also want to support mask before > the value, etc. That may be a bit too much flexibility, though. > > Thoughts? > > > One alternative would be to introduce OVS_ACTION_ATTR_SET_MASKED, > would use the format you proposed in this patch except that the mask > would always be present. > > > Thanks, I like this, as this will also make feature checking more explicit. I think that's a reasonable thing to do as well. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] bridge: improve vlan mode related error messages when adding port
On 04/09/2014 06:57 PM, Ben Pfaff wrote: On Wed, Apr 09, 2014 at 09:52:03AM -0700, Ben Pfaff wrote: On Wed, Apr 09, 2014 at 05:19:17PM +0200, Thomas Graf wrote: Inform about fallback to trunk mode and convert errors to warnings when we are not failing. Signed-off-by: Thomas Graf I'm pretty sure that the first one, as the comment says, really can't happen. Have you been able to trigger it somehow? The second change is OK by me. On second thought, I'm happy enough with these as warnings, so I applied this after touching up line lengths. I haven't triggered them myself. I came across these while working on a proposal for ovs-vsctl status. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 07/10] lib/odp: Use masked set actions.
On Wed, Apr 09, 2014 at 01:46:48PM -0700, Jarno Rajahalme wrote: > > There is a doubled ';' in commit_set_ipv4_action(): > > +key.ipv4_src = base->nw_src & wc->masks.nw_src;; > > > > The beginning of commit_set_ipv4_action() first asserts: > >!((a ^ b) & ~c) > > and then checks whether > >(a ^ b) & c > > for several instances of 'a', 'b', and 'c'. But if the assertions are > > true, then I believe that the checks can be reduced to just > >a != b > > because we already know at that the bits not in 'c' are the same. (Am > > I missing something?) > > > > I had intended to remove the assertions. However, those have been > valuable in catching cases where flow values have been modified > without setting the mask bits. One way of keeping them around for > testing purposes would be to piggyback on the FLOW_WC_SEQ, in effect > enabling the asserts every time FLOW_WC_SEQ is bumped? other ideas? I don't really object to the assertions, more to the masking in the check given that we already know (either because of the assertion, or because we assume that our code is correct) that the mask bits are the same. In other words, I'd just write a != b instead of (a ^ b) & c. A lot of the functions already do a != b so I'm curious why commit_set_ipv4_action() and maybe a few other cases are different. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: Remove the flow_dumper thread.
OK, sure.
On 10 April 2014 04:59, Ben Pfaff wrote:
> On Wed, Apr 09, 2014 at 04:36:50PM +1200, Joe Stringer wrote:
> > Thanks Alex.
> >
> > There is also one minor fix to be rolled in:
> >
> > @@ -1403,8 +1416,8 @@ revalidate_ukey(struct udpif *udpif, struct
> udpif_key
> > *ukey,
> > compose_slow_path(udpif, &xout, odp_in_port, &xout_actions);
> > }
> >
> > -if (actions_len != xout_actions.size
> > -|| memcmp(xout_actions.data, actions, actions_len)) {
> > +if (actions_len != ofpbuf_size(&xout_actions)
> > +|| memcmp(ofpbuf_data(&xout_actions), actions, actions_len)) {
> > goto exit;
> > }
> >
> > I will wait for further review. Let me know if I should repost.
>
> Repost with the fix and I'll review it.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Orphan frags in skb_zerocopy and handle errors
On Wed, Apr 9, 2014 at 9:20 AM, Zoltan Kiss wrote: > This is the ported version of commit 36d5fe6a with the same name from > net-next. > Apart from the small datapath.c changes it adjust the compat layer files as > well. This is the original commit message: > > "skb_zerocopy can copy elements of the frags array between skbs, but it > doesn't > orphan them. Also, it doesn't handle errors, so this patch takes care of that > as well, and modify the callers accordingly. skb_tx_error() is also added to > the callers so they will signal the failed delivery towards the creator of the > skb." > > Signed-off-by: Zoltan Kiss I think this makes sense as an independent bugfix outside of the 3.14 patch but can you also provide a backport for skb_tx_error()? I think otherwise it won't compile on kernels before 3.8. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 4/8] ofproto-dpif: Make bfd and cfm notify ofproto-dpif when the status changes.
On Fri, Apr 04, 2014 at 03:00:39PM -0700, Alex Wang wrote: > This commit adds call-back function to the tunnel monitoring protocal > bfd/cfm to allow the session to notify the ofproto-dpif whenever its > status changes. This helps eliminate the overhead of updating unchanged > session status to the database. > > Signed-off-by: Alex Wang I prefer to avoid callbacks, when I can. Is it possible to avoid them here? One possible way would be to add a sequence number to cfm and bfd objects; another way is to add a "changed" flag to cfm and bfd flags and provide an interface to read it and reset it. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 4/8] ofproto-dpif: Make bfd and cfm notify ofproto-dpif when the status changes.
On Wed, Apr 9, 2014 at 4:00 PM, Ben Pfaff wrote: > On Fri, Apr 04, 2014 at 03:00:39PM -0700, Alex Wang wrote: > > This commit adds call-back function to the tunnel monitoring protocal > > bfd/cfm to allow the session to notify the ofproto-dpif whenever its > > status changes. This helps eliminate the overhead of updating unchanged > > session status to the database. > > > > Signed-off-by: Alex Wang > > I prefer to avoid callbacks, when I can. Is it possible to avoid them > here? One possible way would be to add a sequence number to cfm and > bfd objects; another way is to add a "changed" flag to cfm and bfd > flags and provide an interface to read it and reset it. > Yes, I think a boolean flag is good enough. I'll change accordingly, ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 5/8] bridge: Remove the 'Instant' stats.
On Fri, Apr 04, 2014 at 03:00:40PM -0700, Alex Wang wrote:
> This commit removes the "Instant" stats related logic in bridge.c
> by moving the logic into iface_refresh_ofproto_status() and using
> the global sequence number to check whether or not to call it.
>
> Also, the 'netdev''s 'change_seq' is used to skip the update if the
> interface's netdev status is not changed since last update.
>
> Signed-off-by: Alex Wang
The commit message is too low-level, in my opinion, because it doesn't
explain the user-visible effects. Does this mean, for example, that
the user will see slower database updates? I'm not really sure, since
it seems to talk more about moving code around.
There's a comment in iface_refresh_netdev_status() that I don't quite
understand:
> +if (iface->change_seq == netdev_get_change_seq(iface->netdev)) {
> +return;
> +}
> +
> +/* Should not worry any race. Since only main thread can change the
> + * 'netdev''s change_seq. */
> +iface->change_seq = netdev_get_change_seq(iface->netdev);
It might be true (I did not check) that currently only actions taken
by the main thread can cause netdev change sequence numbers to change.
But that's not a requirement. One can imagine, for example, some type
of netdev for which the best implementation is to have a thread
specialized for waiting for status changes and, when they occur,
incrementing the netdev's change sequence number.
But I don't think that this code even cares whether that's the case:
it only actually reads the netdev's status *after* it reads the
sequence number, which means that any change after this code should,
at worst, get picked up on the next call to the function. So I don't
think there's a race here at all, even if other threads can change the
change_seq.
I also don't quite follow this comment in
iface_refresh_ofproto_status(), because it talks about a change to
status but actually checks for an error fetching status:
+smap_init(&smap);
+error = ofproto_port_get_bfd_status(iface->port->bridge->ofproto,
+iface->ofp_port, &smap);
+/* No need to do the following work if there is no change to status. */
+if (error < 0) {
+return;
+}
+ovsrec_interface_set_bfd_status(iface->cfg, &smap);
+smap_destroy(&smap);
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCHv8 2/2] ofproto-dpif: Remove the flow_dumper thread.
From: Ethan Jackson
Previously, we had a separate flow_dumper thread that fetched flows from
the datapath to distribute to revalidator threads. This patch takes the
logic for dumping and pushes it into the revalidator threads, resulting
in simpler code with similar performance to the current code.
One thread, the "leader", is responsible for beginning and ending each
flow dump, maintaining the flow_limit, and checking whether the
revalidator threads need to exit. All revalidator threads dump,
revalidate, delete datapath flows and garbage collect ukeys.
Co-authored-by: Joe Stringer
Signed-off-by: Joe Stringer
---
v8: Rebase.
v7: Add back logic (present in master) that deletes all flows older than
100ms if we are currently exceeding the flow limit.
Rebase.
v6: Shift ukeys hmaps from revalidators into udpif.
Documentation tidyups.
v5: Handle ukey creation race.
Style fixes.
v4: Rebase.
v3: First post.
---
ofproto/ofproto-dpif-upcall.c | 632 +++--
1 file changed, 300 insertions(+), 332 deletions(-)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 938cfde..064d939 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -67,34 +67,27 @@ struct handler {
'mutex'. */
};
-/* A thread that processes each kernel flow handed to it by the flow_dumper
- * thread, updates OpenFlow statistics, and updates or removes the kernel flow
- * as necessary. */
+/* A thread that processes datapath flows, updates OpenFlow statistics, and
+ * updates or removes them if necessary. */
struct revalidator {
struct udpif *udpif; /* Parent udpif. */
char *name;/* Thread name. */
pthread_t thread; /* Thread ID. */
-struct hmap ukeys; /* Datapath flow keys. */
-
-uint64_t dump_seq;
-
-struct ovs_mutex mutex;/* Mutex guarding the following. */
-pthread_cond_t wake_cond;
-struct list udumps OVS_GUARDED;/* Unprocessed udumps. */
-size_t n_udumps OVS_GUARDED; /* Number of unprocessed udumps. */
+struct hmap *ukeys;/* Points into udpif->ukeys for this
+ revalidator. Used for GC phase. */
};
/* An upcall handler for ofproto_dpif.
*
* udpif has two logically separate pieces:
*
- *- A "dispatcher" thread that reads upcalls from the kernel and dispatches
- * them to one of several "handler" threads (see struct handler).
+ *- Miss handling threads led by a "dispatcher" thread that reads upcalls
+ * from the kernel and dispatches them to one of several "handler" threads
+ * (see struct handler).
*
- *- A "flow_dumper" thread that reads the kernel flow table and dispatches
- * flows to one of several "revalidator" threads (see struct
- * revalidator). */
+ *- Revalidation threads which read the datapath flow table and maintains
+ * them. */
struct udpif {
struct list list_node; /* In all_udpifs list. */
@@ -104,7 +97,6 @@ struct udpif {
uint32_t secret; /* Random seed for upcall hash. */
pthread_t dispatcher; /* Dispatcher thread ID. */
-pthread_t flow_dumper; /* Flow dumper thread ID. */
struct handler *handlers; /* Upcall handlers. */
size_t n_handlers;
@@ -112,14 +104,24 @@ struct udpif {
struct revalidator *revalidators; /* Flow revalidators. */
size_t n_revalidators;
-uint64_t last_reval_seq; /* 'reval_seq' at last revalidation. */
-struct seq *reval_seq; /* Incremented to force revalidation. */
-
-struct seq *dump_seq; /* Increments each dump iteration. */
-
struct latch exit_latch; /* Tells child threads to exit. */
+/* Revalidation. */
+struct seq *reval_seq; /* Incremented to force revalidation. */
+bool need_revalidate; /* As indicated by 'reval_seq'. */
+bool reval_exit; /* Set by leader on 'exit_latch. */
+pthread_barrier_t reval_barrier; /* Barrier used by revalidators. */
+struct dpif_flow_dump dump;/* DPIF flow dump state. */
long long int dump_duration; /* Duration of the last flow dump. */
+struct seq *dump_seq; /* Increments each dump iteration. */
+
+/* During the flow dump phase, revalidators insert into these with a random
+ * distribution. During the garbage collection phase, each revalidator
+ * takes care of garbage collecting one of these hmaps. */
+struct {
+struct ovs_mutex mutex;/* Guards the following. */
+struct hmap hmap OVS_GUARDED; /* Datapath flow keys. */
+} *ukeys;
/* Datapath flow statistics. */
unsigned int max_n_flows;
@@ -155,40 +157,28 @@ struct upcall {
/* 'udpif_k
[ovs-dev] [PATCHv8 1/2] tests/ofproto-dpif: Use vlog to test dpif behaviour.
From: Joe Stringer Previously we made heavy use of the 'ovs-appctl dpif/dump-flows' command to verify that flows were being installed into the datapath correctly. However this is sensitive to timing, particularly when the behaviour of revalidator threads is modified. A common race condition involves flows being revalidated and deleted before the test script gets a chance to connect to ovs-vswitchd and dump the datapath flows. This patch reworks the tests to make use of the vlog facility for dpif, checking for flow installation and flow dump messages. Most tests are converted to check for flow installation, which should reduce these race conditions significantly. For tests that require packet counts, these will continue to check for flow_dump messages. Race conditions for these should be reduced but not eliminated. Signed-off-by: Joe Stringer --- This patch was previously posted independently, but is now a prerequisite for "ofproto-dpif: Remove the flow_dumper thread". v2: Also modify ofproto-dpif, active-backup bonding Rebase --- tests/ofproto-dpif.at | 229 + 1 file changed, 137 insertions(+), 92 deletions(-) diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 4058cf4..93add7f 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -11,6 +11,15 @@ m4_define([STRIP_XOUT], [[sed ' s/packets:[0-9]*/packets:0/ s/bytes:[0-9]*/bytes:0/ ' | sort]]) +m4_define([FILTER_FLOW_INSTALL], [[ +grep ' put' | sed ' +s/.*put\[create\]\[modify\] // +' | sort | uniq]]) +m4_define([FILTER_FLOW_DUMP], [[ +grep 'flow_dump ' | sed ' +s/.*flow_dump // +s/used:[0-9]*\.[0-9]*/used:0.0/ +' | sort | uniq]]) AT_SETUP([ofproto-dpif - dummy interface]) # Create br0 with interfaces p1 and p7 @@ -25,6 +34,7 @@ OVS_VSWITCHD_START( fail-mode=secure -- \ add-port br1 p2 -- set interface p2 type=dummy options:stream=unix:$OVS_RUNDIR/p0.sock ofport_request=2 -- \ add-port br1 p8 -- set interface p8 ofport_request=8 type=dummy --]) +AT_CHECK([ovs-appctl vlog/set dpif:dbg]) AT_CHECK([ovs-ofctl add-flow br0 action=normal]) AT_CHECK([ovs-ofctl add-flow br1 action=normal]) @@ -35,14 +45,11 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p8 'in_port(8),eth(src=50:54:00:00:00: ovs-appctl time/warp 100 sleep 1 # wait for forwarders process packets -AT_CHECK([ovs-appctl dpif/dump-flows br0 | STRIP_XOUT], [0], [dnl -skb_priority(0),in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.3/0.0.0.0,dst=10.0.0.4/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), packets:0, bytes:0, used:never, actions: -skb_priority(0),in_port(7),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),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:never, actions: -]) - -AT_CHECK([ovs-appctl dpif/dump-flows br1 | STRIP_XOUT], [0], [dnl -skb_priority(0),in_port(2),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),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:never, actions: -skb_priority(0),in_port(8),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.3/0.0.0.0,dst=10.0.0.4/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), packets:0, bytes:0, used:never, actions: +AT_CHECK([cat ovs-vswitchd.log | FILTER_FLOW_INSTALL | STRIP_XOUT], [0], [dnl +skb_priority(0),skb_mark(0/0),in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.3/0.0.0.0,dst=10.0.0.4/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),icmp(type=8/0,code=0/0), actions: +skb_priority(0),skb_mark(0/0),in_port(2),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),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),icmp(type=8/0,code=0/0), actions: +skb_priority(0),skb_mark(0/0),in_port(7),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),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),icmp(type=8/0,code=0/0), actions: +skb_priority(0),skb_mark(0/0),in_port(8),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.3/0.0.0.0,dst=10.0.0.4/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),icmp(type=8/0,code=0/0), actions: ]) OVS_VSWITCHD_STOP AT_CLEANUP @@ -63,6 +70,7 @@ OVS_VSWITCHD_START( add-port br1 p3 -- set interface p3 type=dummy options:stream=unix:$OVS_RUNDIR/p1.sock ofport_request=3 -- \ add-port br1 p4 -- set interface p4 type=dummy options:stream=unix:$OVS_RUNDIR/p2.sock ofport_request=4 -- \ add-port br1 p8 -- set interface p8 ofport_request=8 type=dummy --]) +AT_CHECK([ovs-appctl vlog/set dpif:dbg]) AT_CHECK([ovs-ofctl add-flow br0 action=normal]) AT_CHECK([ovs-ofctl add-flow br1 action=normal]) @@ -81,13 +89,13 @@ AT_CHECK([ovs-appctl n
Re: [ovs-dev] [PATCH 6/8] bridge: Add configuration to disable the interface stats/status update to OVSDB.
On Fri, Apr 04, 2014 at 03:00:41PM -0700, Alex Wang wrote: > This commit adds a new configuration "disable-stats-update" in > "other_config" of Open_Vswitch table. When set, it allows user > to disable the statistics update to OVSDB. This can greatly reduce > the cpu consumption when statistics is not used. > > Signed-off-by: Alex Wang I don't like "negative" names, because they often lead to double negatives (e.g. "if (!disable_stats_update)") that can be hard to understand, especially when one reads quickly. Could we use a name like "enable-stats-update" instead? Actually, is the "update" part meaningful? One might simply say "enable-stats". I would expand the documentation to mention more specifically which stats are disabled. It might also be nice to update the description of each stats column to say that it is controlled by this new setting. When stats are disabled, the database might contain old stats that will quickly grow stale, which is not user friendly. It would be nice to clear the stats columns to avoid that. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 7/8] bridge: Add command to force statistics update when the 'disable-stats-update' is set.
On Fri, Apr 04, 2014 at 03:00:42PM -0700, Alex Wang wrote: > This command adds a new appctl sub-command "stats/require_update", which > allows users to force statistics update to the database when the > 'disable-stats-update' is set. > > Signed-off-by: Alex Wang Do we expect this to actually get used? I don't like the idea of keeping arbitrarily stale stats in the database, if we can avoid it. I think that all of the stats are available from other sources (e.g. OpenFlow), so anyone who really wants to poll the up-to-date stats can do it that way. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 8/8] tests/ovs-vswitchd: Add a test for the 'stats/require-update'.
On Fri, Apr 04, 2014 at 03:00:43PM -0700, Alex Wang wrote: > This commit adds a unit test for the 'ovs-appctl stats/require-update'. > > Signed-off-by: Alex Wang If we really need this command at all, then I would probably fold this patch in with the patch that adds the command. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/8] netdev: Add 'change_seq' back to netdev.
On Fri, Apr 04, 2014 at 03:00:36PM -0700, Alex Wang wrote: > This commit can be seen as a partial revert of commit > da4a619179d (netdev: Globally track port status changes) > by adding the 'change_seq' to 'struct netdev'. > > Signed-off-by: Alex Wang Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/8] netdev-dpdk: Signal the change of etheraddr and mtu.
On Fri, Apr 04, 2014 at 03:00:37PM -0700, Alex Wang wrote: > This commit makes the netdev-dpdk module signal the change of > etheraddr and mtu by changing the global sequence number and > incrementing its 'change_seq'. > > Signed-off-by: Alex Wang Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 3/8] ofproto: Use netdev's 'change_seq' to reduce port modification check.
On Fri, Apr 04, 2014 at 03:00:38PM -0700, Alex Wang wrote: > This commit uses the 'change_seq' in 'struct netdev' to determine > whether to update the 'ofport'. This helps eliminate unnecessary > update. > > Signed-off-by: Alex Wang Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 5/8] bridge: Remove the 'Instant' stats.
Thx for the review Ben, please see my reply inline
On Wed, Apr 9, 2014 at 4:13 PM, Ben Pfaff wrote:
> On Fri, Apr 04, 2014 at 03:00:40PM -0700, Alex Wang wrote:
> > This commit removes the "Instant" stats related logic in bridge.c
> > by moving the logic into iface_refresh_ofproto_status() and using
> > the global sequence number to check whether or not to call it.
> >
> > Also, the 'netdev''s 'change_seq' is used to skip the update if the
> > interface's netdev status is not changed since last update.
> >
> > Signed-off-by: Alex Wang
>
> The commit message is too low-level, in my opinion, because it doesn't
> explain the user-visible effects. Does this mean, for example, that
> the user will see slower database updates? I'm not really sure, since
> it seems to talk more about moving code around.
>
Thanks for pointing out it,
It will not cause a slower update. And I will fix the message to explain
the
user-visible effects.
> There's a comment in iface_refresh_netdev_status() that I don't quite
> understand:
>
> > +if (iface->change_seq == netdev_get_change_seq(iface->netdev)) {
> > +return;
> > +}
> > +
> > +/* Should not worry any race. Since only main thread can change the
> > + * 'netdev''s change_seq. */
> > +iface->change_seq = netdev_get_change_seq(iface->netdev);
>
> It might be true (I did not check) that currently only actions taken
> by the main thread can cause netdev change sequence numbers to change.
> But that's not a requirement. One can imagine, for example, some type
> of netdev for which the best implementation is to have a thread
> specialized for waiting for status changes and, when they occur,
> incrementing the netdev's change sequence number.
>
> But I don't think that this code even cares whether that's the case:
> it only actually reads the netdev's status *after* it reads the
> sequence number, which means that any change after this code should,
> at worst, get picked up on the next call to the function. So I don't
> think there's a race here at all, even if other threads can change the
> change_seq.
>
Thx for the explanation, I agree, the comment is invalid. Will remove it.
> I also don't quite follow this comment in
> iface_refresh_ofproto_status(), because it talks about a change to
> status but actually checks for an error fetching status:
>
>
I'll change the variable name.
> +smap_init(&smap);
> +error = ofproto_port_get_bfd_status(iface->port->bridge->ofproto,
> +iface->ofp_port, &smap);
> +/* No need to do the following work if there is no change to status.
> */
> +if (error < 0) {
> +return;
> +}
> +ovsrec_interface_set_bfd_status(iface->cfg, &smap);
> +smap_destroy(&smap);
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2.56] datapath: Add basic MPLS support to kernel
On Tue, Apr 01, 2014 at 12:46:20PM +0900, Simon Horman wrote: > Hi Jesse, Hi Pravin, Hi All, > > This patchset makes use of a whitelist to only allow mpls_push actions to > be applied to packets with an ethertype where the tag order is well defined > and implemented. This avoids the problem of where an MPLS LSE should be > added in relation to a VLAN or similar tag. > > The list of allowed ethertypes is currently: > > - ETH_P_IP (0x0800) > - ETH_P_ARP (0x0806) > - ETH_P_RARP (0x0835) > - ETH_P_IPV6 (0x86DD) > - ETH_P_MPLS_UC (0x8847) > - ETH_P_MPLS_MC (0x8847) Hi, a gentle nudge on this one. I'd be most appreciative of some feedback. > I have updated this patch since v2.55 so that: > > * The MPLS ethertypes are included in the whitelist of ethertypes > that allow mpls_push actions. > * It once again applies on top of the master branch > > > Simon Horman (1): > datapath: Add basic MPLS support to kernel > > OPENFLOW-1.1+ | 4 - > datapath/Modules.mk | 1 + > datapath/actions.c | 119 +- > datapath/datapath.c | 4 +- > datapath/flow.c | 29 +++ > datapath/flow.h | 17 +- > datapath/flow_netlink.c | 298 > ++-- > datapath/flow_netlink.h | 2 +- > datapath/linux/compat/gso.c | 70 +- > datapath/linux/compat/gso.h | 41 +++- > datapath/linux/compat/include/linux/netdevice.h | 6 +- > datapath/linux/compat/netdevice.c | 10 +- > datapath/mpls.h | 15 ++ > include/linux/openvswitch.h | 9 +- > 14 files changed, 568 insertions(+), 57 deletions(-) > create mode 100644 datapath/mpls.h > > -- > 1.8.4 > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 5/8] bridge: Remove the 'Instant' stats.
On Wed, Apr 09, 2014 at 04:31:46PM -0700, Alex Wang wrote:
> On Wed, Apr 9, 2014 at 4:13 PM, Ben Pfaff wrote:
> > I also don't quite follow this comment in
> > iface_refresh_ofproto_status(), because it talks about a change to
> > status but actually checks for an error fetching status:
> >
> >
>
> I'll change the variable name.
But it's not the variable name that's the problem. I guess that
you're thinking about how ofproto_port_get_bfd_status() returns ENOENT
if there's no BFD configured on that port? But in that case we'd
still like to fill in the BFD status (as an empty map) because BFD
might have been configured previously and we don't want to leave stale
status information there.
>
>
> > +smap_init(&smap);
> > +error = ofproto_port_get_bfd_status(iface->port->bridge->ofproto,
> > +iface->ofp_port, &smap);
> > +/* No need to do the following work if there is no change to status.
> > */
> > +if (error < 0) {
> > +return;
> > +}
> > +ovsrec_interface_set_bfd_status(iface->cfg, &smap);
> > +smap_destroy(&smap);
> >
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH/RFC] ofproto-dpif-upcall: Filter translated mask using flow
On Sat, Mar 29, 2014 at 04:01:35PM -0700, Simon Horman wrote: > On Thu, Mar 20, 2014 at 09:37:14AM -0700, Ben Pfaff wrote: > > On Thu, Mar 20, 2014 at 05:34:23PM +0900, Simon Horman wrote: > > > When revalidating a flow convert xout.wc.masks, the mask calculated by > > > translation, to a key that relates to the flow present in the datapath. > > > Only > > > mask elements that relate to the flow will be included in the resulting > > > key. > > > This has the effect of filtering the mask based on 'flow'. > > > > > > In particular this filters out mpls_lse mask bits set on non-MPLS flows. > > > > > > This avoids flows being premeturely evicted from the datapath > > > because their calculated mask is too specific. > > > > I agree that that is a good thing to do, which has been a real problem > > in some cases, but I don't yet understand why there would be mpls_lse > > mask bits set on non-MPLS flows. Can you explain why (when?) that > > happens? > > Hi Ben, > > I'm not sure if there are other cases but one that I have isolated > is if a non-MPLS packet becomes an MPLS packet through a push MPLS action. > > In this case the mpls_lse mask bits are all set to 1 in flow_push_mpls() > during translation. This is in contrast to a mask produced based on the > original (non-MPLS flow) by odp_flow_key_to_mask(), where the mpls_lse bits > are all 0. > > And my analysis is that in the case of revalidate_ukey() the mask > comparison fails because of this miss-match, causing unnecessary eviction > of facets from the datapath. Hi Ben, ping. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv8 1/2] tests/ofproto-dpif: Use vlog to test dpif behaviour.
On Thu, Apr 10, 2014 at 11:14:07AM +1200, Joe Stringer wrote: > From: Joe Stringer > > Previously we made heavy use of the 'ovs-appctl dpif/dump-flows' command > to verify that flows were being installed into the datapath correctly. > However this is sensitive to timing, particularly when the behaviour of > revalidator threads is modified. A common race condition involves flows > being revalidated and deleted before the test script gets a chance to > connect to ovs-vswitchd and dump the datapath flows. > > This patch reworks the tests to make use of the vlog facility for dpif, > checking for flow installation and flow dump messages. Most tests are > converted to check for flow installation, which should reduce these race > conditions significantly. For tests that require packet counts, these > will continue to check for flow_dump messages. Race conditions for these > should be reduced but not eliminated. > > Signed-off-by: Joe Stringer Applied, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 8/8] tests/ovs-vswitchd: Add a test for the 'stats/require-update'.
On Wed, Apr 9, 2014 at 4:23 PM, Ben Pfaff wrote: > On Fri, Apr 04, 2014 at 03:00:43PM -0700, Alex Wang wrote: > > This commit adds a unit test for the 'ovs-appctl stats/require-update'. > > > > Signed-off-by: Alex Wang > > If we really need this command at all, then I would probably fold this > patch in with the patch that adds the command. > Thx, I'll combine the two if we need this command. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] [RFC] flow: Do not clear L3+ fields of flow in flow_push_mpls()
On Tue, Apr 01, 2014 at 12:21:03PM +0900, Simon Horman wrote:
> On Thu, Mar 20, 2014 at 10:05:44AM -0700, Ben Pfaff wrote:
> > On Fri, Mar 14, 2014 at 04:19:52PM +0900, Simon Horman wrote:
> > > When creating a flow in the datapath as the result of an upcall
> > > the match itself is the match supplied in the upcall while
> > > the mask of the match, if supplied, is generated based on the
> > > flow and mask composed during action translation.
> > >
> > > In the case of, for example a UDP packet, the match will include
> > > of L2, L3 and L4 fields. However, if the flow is cleared in
> > > flow_push_mpls() then the mask that is synthesised from it will
> > > not include L3 and L4 fields. This seems incorrect and the kernel
> > > datapath complains about this mismatch.
> > >
> > > Signed-off-by: Simon Horman
> >
> > The goal of clearing the fields is to ensure that later flow tables
> > can't match on fields that aren't visible anymore. That's important for
> > accurate OpenFlow implementation, so I'd rather not change it. On the
> > other hand, I see the point you're making, but I don't immediately
> > understand why it happens that way. After all, I can change fields with
> > OpenFlow actions and the datapath flows work out OK, why doesn't this
> > work out OK too? Do you understand the reason?
>
> Hi Ben,
>
> I have taken a closer look at this and I think that I now understand the
> problem. I would like to propose the following:
Hi Ben,
I'd be most grateful if you could take another moment to look over this.
>
>
> From: Simon Horman
>
> odp-util: Do not set port mask of non-IP packets
>
> In the case that an flow for an IP packet has an mpls_push action applied
> the L3 and L4 portions of the flow will be cleared in flow_push_mpls().
>
> Without this change commit_set_port_action() will set the tp_src and tp_dst
> mask for the flow to all-ones because the base and flow port values no
> longer match. Even though there will be no corresponding set action for the
> ports; because the flow is no longer IP.
>
> In this case where nw_proto is not part of the match this manifests
> in a problem because the userspace datapath rejects flows whose masks
> have non-zero values for tp_src or dp_dst if the nw_proto mask is
> not all-ones.
>
> This patch resolves this problem by having commit_set_port_action() return
> without doing anything if flow->nw_proto is zero. The same logic is present
> in commit_set_nw_action().
>
> Signed-off-by: Simon Horman
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 956fef1..b40f9bc 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -3778,6 +3778,11 @@ static void
> commit_set_port_action(const struct flow *flow, struct flow *base,
> struct ofpbuf *odp_actions, struct flow_wildcards *wc)
> {
> +/* Check if 'flow' really has an L3 header. */
> +if (!flow->nw_proto) {
> +return;
> +}
> +
> if (!is_ip_any(base) || (!base->tp_src && !base->tp_dst)) {
> return;
> }
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/2] ofproto-dpif: implement group and group bucket stats
Fix a bug in Openflow group implementation that neither group stats
nor per bucket stats are properly updated.
Reported-by: Marco Canini
Signed-off-by: Andy Zhou
---
AUTHORS |1 +
ofproto/ofproto-dpif-xlate.c | 22 --
ofproto/ofproto-dpif-xlate.h |2 +-
ofproto/ofproto-dpif.c | 39 +++
ofproto/ofproto-dpif.h |3 +++
5 files changed, 60 insertions(+), 7 deletions(-)
diff --git a/AUTHORS b/AUTHORS
index 78640b8..e83af71 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -212,6 +212,7 @@ Len Gao l...@vmware.com
Logan Rosen logatron...@gmail.com
Luca Falavigna dktrkr...@debian.org
Luiz Henrique Ozaki luiz.oz...@gmail.com
+Marco Caninimarco.can...@acm.org
Marco d'Itrim...@linux.it
Maxime Brun m.b...@alphalink.fr
Michael A. Collins mike.a.coll...@ark-net.org
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 7b6e9f7..5dd4f93 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -765,20 +765,21 @@ odp_port_is_alive(const struct xlate_ctx *ctx, ofp_port_t
ofp_port)
static const struct ofputil_bucket *
group_first_live_bucket(const struct xlate_ctx *, const struct group_dpif *,
-int depth);
+int depth, int *index);
static bool
group_is_alive(const struct xlate_ctx *ctx, uint32_t group_id, int depth)
{
struct group_dpif *group;
bool hit;
+int unused;
hit = group_dpif_lookup(ctx->xbridge->ofproto, group_id, &group);
if (!hit) {
return false;
}
-hit = group_first_live_bucket(ctx, group, depth) != NULL;
+hit = group_first_live_bucket(ctx, group, depth, &unused) != NULL;
group_dpif_release(group);
return hit;
@@ -807,16 +808,19 @@ bucket_is_alive(const struct xlate_ctx *ctx,
static const struct ofputil_bucket *
group_first_live_bucket(const struct xlate_ctx *ctx,
-const struct group_dpif *group, int depth)
+const struct group_dpif *group, int depth, int *index)
{
struct ofputil_bucket *bucket;
const struct list *buckets;
+int i = 0;
group_dpif_get_buckets(group, &buckets);
LIST_FOR_EACH (bucket, list_node, buckets) {
if (bucket_is_alive(ctx, bucket, depth)) {
+*index = i;
return bucket;
}
+i++;
}
return NULL;
@@ -825,7 +829,7 @@ group_first_live_bucket(const struct xlate_ctx *ctx,
static const struct ofputil_bucket *
group_best_live_bucket(const struct xlate_ctx *ctx,
const struct group_dpif *group,
- uint32_t basis)
+ uint32_t basis, int *index)
{
const struct ofputil_bucket *best_bucket = NULL;
uint32_t best_score = 0;
@@ -841,6 +845,7 @@ group_best_live_bucket(const struct xlate_ctx *ctx,
if (score >= best_score) {
best_bucket = bucket;
best_score = score;
+*index = i;
}
}
i++;
@@ -2022,16 +2027,19 @@ xlate_all_group(struct xlate_ctx *ctx, struct
group_dpif *group)
* just before applying the all or indirect group. */
ctx->xin->flow = old_flow;
}
+group_dpif_credit_stats(group, -1, ctx->xin->resubmit_stats);
}
static void
xlate_ff_group(struct xlate_ctx *ctx, struct group_dpif *group)
{
const struct ofputil_bucket *bucket;
+int index;
-bucket = group_first_live_bucket(ctx, group, 0);
+bucket = group_first_live_bucket(ctx, group, 0, &index);
if (bucket) {
xlate_group_bucket(ctx, bucket);
+group_dpif_credit_stats(group, index, ctx->xin->resubmit_stats);
}
}
@@ -2041,12 +2049,14 @@ xlate_select_group(struct xlate_ctx *ctx, struct
group_dpif *group)
struct flow_wildcards *wc = &ctx->xout->wc;
const struct ofputil_bucket *bucket;
uint32_t basis;
+int index;
basis = hash_mac(ctx->xin->flow.dl_dst, 0, 0);
-bucket = group_best_live_bucket(ctx, group, basis);
+bucket = group_best_live_bucket(ctx, group, basis, &index);
if (bucket) {
memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
xlate_group_bucket(ctx, bucket);
+group_dpif_credit_stats(group, index, ctx->xin->resubmit_stats);
}
}
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index 8b53e10..bb3bad0 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -122,7 +122,7 @@ struct xlate_in {
void (*report_hook)(struct xlate_in *, const char *s, int recurse);
/* If nonnull, flow translation credits the specified statistics to each
- * rule reached through a resubmit or OFPP_TABLE action.
+ * rule reached through a resubmit, OFPP_TABLE or group action.
*
* This is normally
[ovs-dev] [PATCH 2/2] tests: add tests for group stats
Signed-off-by: Andy Zhou --- tests/ofproto-dpif.at | 44 1 file changed, 44 insertions(+) diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 064407a..769df2d 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -369,6 +369,50 @@ AT_CHECK([tail -1 stdout], [0], OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif - group stats single bucket]) +OVS_VSWITCHD_START +ADD_OF_PORTS([br0], [1], [10], [11]) +AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 'group_id=1234,type=select,bucket=output:10,weight=2000,bucket=output:11,weight=0']) +AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 'ip actions=write_actions(group:1234)']) +( +for i in `seq 0 2`; +do + pkt="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:01:00),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 netdev-dummy/receive p1 $pkt]) +done +) +ovs-appctl time/warp 100 +sleep 1 # wait for forwarders process packets +AT_CHECK([ovs-ofctl -O OpenFlow12 -vwarn dump-group-stats br0], [0], [stdout]) +AT_CHECK([STRIP_XIDS stdout | sort], [0], [dnl + group_id=1234,ref_count=0,packet_count=3,byte_count=180,bucket0:packet_count=3,byte_count=180,bucket1:packet_count=0,byte_count=0 +OFPST_GROUP reply (OF1.2): +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + +AT_SETUP([ofproto-dpif - group stats all buckets]) +OVS_VSWITCHD_START +ADD_OF_PORTS([br0], [1], [10], [11]) +AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 'group_id=1234,type=all,bucket=output:10,bucket=output:11']) +AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 'ip actions=write_actions(group:1234)']) +( +for i in `seq 0 2`; +do + pkt="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:01:00),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 netdev-dummy/receive p1 $pkt]) +done +) +ovs-appctl time/warp 100 +sleep 1 # wait for forwarders process packets +AT_CHECK([ovs-ofctl -O OpenFlow12 -vwarn dump-group-stats br0], [0], [stdout]) +AT_CHECK([STRIP_XIDS stdout | sort], [0], [dnl + group_id=1234,ref_count=0,packet_count=3,byte_count=180,bucket0:packet_count=3,byte_count=180,bucket1:packet_count=3,byte_count=180 +OFPST_GROUP reply (OF1.2): +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([ofproto-dpif - registers]) OVS_VSWITCHD_START ADD_OF_PORTS([br0], [20], [21], [22], [33], [90]) -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] ofproto-dpif: implement group and group bucket stats
Forgot to add marco.can...@acm.org to CC
On Wed, Apr 9, 2014 at 4:43 PM, Andy Zhou wrote:
> Fix a bug in Openflow group implementation that neither group stats
> nor per bucket stats are properly updated.
>
> Reported-by: Marco Canini
> Signed-off-by: Andy Zhou
> ---
> AUTHORS |1 +
> ofproto/ofproto-dpif-xlate.c | 22 --
> ofproto/ofproto-dpif-xlate.h |2 +-
> ofproto/ofproto-dpif.c | 39 +++
> ofproto/ofproto-dpif.h |3 +++
> 5 files changed, 60 insertions(+), 7 deletions(-)
>
> diff --git a/AUTHORS b/AUTHORS
> index 78640b8..e83af71 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -212,6 +212,7 @@ Len Gao l...@vmware.com
> Logan Rosen logatron...@gmail.com
> Luca Falavigna dktrkr...@debian.org
> Luiz Henrique Ozaki luiz.oz...@gmail.com
> +Marco Caninimarco.can...@acm.org
> Marco d'Itrim...@linux.it
> Maxime Brun m.b...@alphalink.fr
> Michael A. Collins mike.a.coll...@ark-net.org
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 7b6e9f7..5dd4f93 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -765,20 +765,21 @@ odp_port_is_alive(const struct xlate_ctx *ctx,
> ofp_port_t ofp_port)
>
> static const struct ofputil_bucket *
> group_first_live_bucket(const struct xlate_ctx *, const struct group_dpif *,
> -int depth);
> +int depth, int *index);
>
> static bool
> group_is_alive(const struct xlate_ctx *ctx, uint32_t group_id, int depth)
> {
> struct group_dpif *group;
> bool hit;
> +int unused;
>
> hit = group_dpif_lookup(ctx->xbridge->ofproto, group_id, &group);
> if (!hit) {
> return false;
> }
>
> -hit = group_first_live_bucket(ctx, group, depth) != NULL;
> +hit = group_first_live_bucket(ctx, group, depth, &unused) != NULL;
>
> group_dpif_release(group);
> return hit;
> @@ -807,16 +808,19 @@ bucket_is_alive(const struct xlate_ctx *ctx,
>
> static const struct ofputil_bucket *
> group_first_live_bucket(const struct xlate_ctx *ctx,
> -const struct group_dpif *group, int depth)
> +const struct group_dpif *group, int depth, int
> *index)
> {
> struct ofputil_bucket *bucket;
> const struct list *buckets;
> +int i = 0;
>
> group_dpif_get_buckets(group, &buckets);
> LIST_FOR_EACH (bucket, list_node, buckets) {
> if (bucket_is_alive(ctx, bucket, depth)) {
> +*index = i;
> return bucket;
> }
> +i++;
> }
>
> return NULL;
> @@ -825,7 +829,7 @@ group_first_live_bucket(const struct xlate_ctx *ctx,
> static const struct ofputil_bucket *
> group_best_live_bucket(const struct xlate_ctx *ctx,
> const struct group_dpif *group,
> - uint32_t basis)
> + uint32_t basis, int *index)
> {
> const struct ofputil_bucket *best_bucket = NULL;
> uint32_t best_score = 0;
> @@ -841,6 +845,7 @@ group_best_live_bucket(const struct xlate_ctx *ctx,
> if (score >= best_score) {
> best_bucket = bucket;
> best_score = score;
> +*index = i;
> }
> }
> i++;
> @@ -2022,16 +2027,19 @@ xlate_all_group(struct xlate_ctx *ctx, struct
> group_dpif *group)
> * just before applying the all or indirect group. */
> ctx->xin->flow = old_flow;
> }
> +group_dpif_credit_stats(group, -1, ctx->xin->resubmit_stats);
> }
>
> static void
> xlate_ff_group(struct xlate_ctx *ctx, struct group_dpif *group)
> {
> const struct ofputil_bucket *bucket;
> +int index;
>
> -bucket = group_first_live_bucket(ctx, group, 0);
> +bucket = group_first_live_bucket(ctx, group, 0, &index);
> if (bucket) {
> xlate_group_bucket(ctx, bucket);
> +group_dpif_credit_stats(group, index, ctx->xin->resubmit_stats);
> }
> }
>
> @@ -2041,12 +2049,14 @@ xlate_select_group(struct xlate_ctx *ctx, struct
> group_dpif *group)
> struct flow_wildcards *wc = &ctx->xout->wc;
> const struct ofputil_bucket *bucket;
> uint32_t basis;
> +int index;
>
> basis = hash_mac(ctx->xin->flow.dl_dst, 0, 0);
> -bucket = group_best_live_bucket(ctx, group, basis);
> +bucket = group_best_live_bucket(ctx, group, basis, &index);
> if (bucket) {
> memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
> xlate_group_bucket(ctx, bucket);
> +group_dpif_credit_stats(group, index, ctx->xin->resubmit_stats);
> }
> }
>
> diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
> index 8b53e10..bb3bad0 100644
> --- a/ofproto/ofproto-dpif-xlate.h
> +++ b/ofproto/ofproto-dpif-xlate.h
> @@ -122,7 +122,
[ovs-dev] FW: Retro decorationa lighting fixture
Dear Friends, This is the Mars here. I would like to introduce the hot sales of Retro lighting fixture for you. If you have any plan and interest, please let me know. Hear from your business and reply soon Please see the photo as follow: Best Regards Mars International Business Department Guangzhou PLighting Technology Co.,Limited. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] learn.at: Fix a shell syntax
Fix a botch in commit 7f12bdcc.
("learn.at: Fix a race in "self-modifying flow with idle_timeout" test")
Signed-off-by: YAMAMOTO Takashi
---
tests/learn.at | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/learn.at b/tests/learn.at
index 42dd363..7245295 100644
--- a/tests/learn.at
+++ b/tests/learn.at
@@ -352,7 +352,7 @@ for i in `seq 1 10`; do
# (i % 3 == 0) below is somehow arbitrary but chosen to ensure
# that we update the flow's 'used' frequently enough to prevent
# idle_timeout.
-if [[ $i -eq 1 -or $((i % 3)) -eq 0 ]]; then
+if [[ $i -eq 1 -o $((i % 3)) -eq 0 ]]; then
sleep 1
fi
done
--
1.8.3.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] learn.at: Fix a shell syntax
Looks good, thanks!
On Apr 9, 2014 5:42 PM, "YAMAMOTO Takashi" wrote:
> Fix a botch in commit 7f12bdcc.
> ("learn.at: Fix a race in "self-modifying flow with idle_timeout" test")
>
> Signed-off-by: YAMAMOTO Takashi
> ---
> tests/learn.at | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/learn.at b/tests/learn.at
> index 42dd363..7245295 100644
> --- a/tests/learn.at
> +++ b/tests/learn.at
> @@ -352,7 +352,7 @@ for i in `seq 1 10`; do
> # (i % 3 == 0) below is somehow arbitrary but chosen to ensure
> # that we update the flow's 'used' frequently enough to prevent
> # idle_timeout.
> -if [[ $i -eq 1 -or $((i % 3)) -eq 0 ]]; then
> +if [[ $i -eq 1 -o $((i % 3)) -eq 0 ]]; then
> sleep 1
> fi
> done
> --
> 1.8.3.1
>
> ___
> 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 1/8] netdev: Add 'change_seq' back to netdev.
It had occurred to me at the time, that this was a possible middle ground
between making netdev change_seq completely global or retaining this
per-netdev seq as well as having a global seq. When I looked at perf when
many ports are configured, a majority of the cost seemed to be just in just
iterating through the ports to check the seq.
Do you have a sense of the impact this has?
On 5 April 2014 11:00, Alex Wang wrote:
> This commit can be seen as a partial revert of commit
> da4a619179d (netdev: Globally track port status changes)
> by adding the 'change_seq' to 'struct netdev'.
>
> Signed-off-by: Alex Wang
> ---
> lib/netdev-bsd.c |5 +
> lib/netdev-dpdk.c |1 +
> lib/netdev-dummy.c|2 ++
> lib/netdev-linux.c|1 +
> lib/netdev-provider.h | 17 +
> lib/netdev-vport.c|3 +++
> lib/netdev.c |8
> lib/netdev.h |1 +
> 8 files changed, 38 insertions(+)
>
> diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
> index 9952aef..affd08a 100644
> --- a/lib/netdev-bsd.c
> +++ b/lib/netdev-bsd.c
> @@ -217,6 +217,7 @@ netdev_bsd_cache_cb(const struct rtbsd_change *change,
> if (is_netdev_bsd_class(netdev_class)) {
> dev = netdev_bsd_cast(base_dev);
> dev->cache_valid = 0;
> +netdev_change_seq_changed(base_dev);
> seq_change(connectivity_seq_get());
> }
> netdev_close(base_dev);
> @@ -236,6 +237,7 @@ netdev_bsd_cache_cb(const struct rtbsd_change *change,
> dev = netdev_bsd_cast(netdev);
> dev->cache_valid = 0;
> seq_change(connectivity_seq_get());
> +netdev_change_seq_changed(netdev);
> netdev_close(netdev);
> }
> shash_destroy(&device_shash);
> @@ -774,6 +776,7 @@ netdev_bsd_set_etheraddr(struct netdev *netdev_,
> if (!error) {
> netdev->cache_valid |= VALID_ETHERADDR;
> memcpy(netdev->etheraddr, mac, ETH_ADDR_LEN);
> +netdev_change_seq_changed(netdev_);
> seq_change(connectivity_seq_get());
> }
> }
> @@ -1228,6 +1231,7 @@ netdev_bsd_set_in4(struct netdev *netdev_, struct
> in_addr addr,
> netdev->netmask = mask;
> }
> }
> +netdev_change_seq_changed(netdev_);
> seq_change(connectivity_seq_get());
> }
> ovs_mutex_unlock(&netdev->mutex);
> @@ -1526,6 +1530,7 @@ netdev_bsd_update_flags(struct netdev *netdev_, enum
> netdev_flags off,
> new_flags = (old_flags & ~nd_to_iff_flags(off)) |
> nd_to_iff_flags(on);
> if (new_flags != old_flags) {
> error = set_flags(netdev_get_kernel_name(netdev_), new_flags);
> +netdev_change_seq_changed(netdev_);
> seq_change(connectivity_seq_get());
> }
> }
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 4b36f52..1ab57cc 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -318,6 +318,7 @@ check_link_status(struct netdev_dpdk *dev)
> rte_eth_link_get_nowait(dev->port_id, &link);
>
> if (dev->link.link_status != link.link_status) {
> +netdev_change_seq_changed(&dev->up);
> seq_change(connectivity_seq_get());
>
> dev->link_reset_cnt++;
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> index 995e923..359fc06 100644
> --- a/lib/netdev-dummy.c
> +++ b/lib/netdev-dummy.c
> @@ -873,6 +873,7 @@ netdev_dummy_set_etheraddr(struct netdev *netdev,
> ovs_mutex_lock(&dev->mutex);
> if (!eth_addr_equals(dev->hwaddr, mac)) {
> memcpy(dev->hwaddr, mac, ETH_ADDR_LEN);
> +netdev_change_seq_changed(netdev);
> seq_change(connectivity_seq_get());
> }
> ovs_mutex_unlock(&dev->mutex);
> @@ -968,6 +969,7 @@ netdev_dummy_update_flags__(struct netdev_dummy
> *netdev,
> netdev->flags |= on;
> netdev->flags &= ~off;
> if (*old_flagsp != netdev->flags) {
> +netdev_change_seq_changed(&netdev->up);
> seq_change(connectivity_seq_get());
> }
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index f9634b6..2f61936 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -616,6 +616,7 @@ netdev_linux_changed(struct netdev_linux *dev,
> unsigned int ifi_flags, unsigned int mask)
> OVS_REQUIRES(dev->mutex)
> {
> +netdev_change_seq_changed(&dev->up);
> seq_change(connectivity_seq_get());
>
> if ((dev->ifi_flags ^ ifi_flags) & IFF_RUNNING) {
> diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
> index f233c0c..84b62fb 100644
> --- a/lib/netdev-provider.h
> +++ b/lib/netdev-provider.h
> @@ -38,6 +38,14 @@ struct netdev {
> const struct netdev_class *netdev_class; /* Functions to control
> this device. */
>
> +/* A sequence number which indicates changes in one of 'netdev'
Re: [ovs-dev] [PATCH 1/8] netdev: Add 'change_seq' back to netdev.
Hey Joe,
On Wed, Apr 9, 2014 at 6:16 PM, Joe Stringer wrote:
> It had occurred to me at the time, that this was a possible middle ground
> between making netdev change_seq completely global or retaining this
> per-netdev seq as well as having a global seq. When I looked at perf when
> many ports are configured, a majority of the cost seemed to be just in just
> iterating through the ports to check the seq.
>
> Do you have a sense of the impact this has?
>
>
I haven't tested about it. And I'd like to measure it.
I only concerned and tested the benefit of updating to database.
Let me update the thread after testing the configuration perf and latency.
> On 5 April 2014 11:00, Alex Wang wrote:
>
>> This commit can be seen as a partial revert of commit
>> da4a619179d (netdev: Globally track port status changes)
>> by adding the 'change_seq' to 'struct netdev'.
>>
>> Signed-off-by: Alex Wang
>> ---
>> lib/netdev-bsd.c |5 +
>> lib/netdev-dpdk.c |1 +
>> lib/netdev-dummy.c|2 ++
>> lib/netdev-linux.c|1 +
>> lib/netdev-provider.h | 17 +
>> lib/netdev-vport.c|3 +++
>> lib/netdev.c |8
>> lib/netdev.h |1 +
>> 8 files changed, 38 insertions(+)
>>
>> diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
>> index 9952aef..affd08a 100644
>> --- a/lib/netdev-bsd.c
>> +++ b/lib/netdev-bsd.c
>> @@ -217,6 +217,7 @@ netdev_bsd_cache_cb(const struct rtbsd_change *change,
>> if (is_netdev_bsd_class(netdev_class)) {
>> dev = netdev_bsd_cast(base_dev);
>> dev->cache_valid = 0;
>> +netdev_change_seq_changed(base_dev);
>> seq_change(connectivity_seq_get());
>> }
>> netdev_close(base_dev);
>> @@ -236,6 +237,7 @@ netdev_bsd_cache_cb(const struct rtbsd_change *change,
>> dev = netdev_bsd_cast(netdev);
>> dev->cache_valid = 0;
>> seq_change(connectivity_seq_get());
>> +netdev_change_seq_changed(netdev);
>> netdev_close(netdev);
>> }
>> shash_destroy(&device_shash);
>> @@ -774,6 +776,7 @@ netdev_bsd_set_etheraddr(struct netdev *netdev_,
>> if (!error) {
>> netdev->cache_valid |= VALID_ETHERADDR;
>> memcpy(netdev->etheraddr, mac, ETH_ADDR_LEN);
>> +netdev_change_seq_changed(netdev_);
>> seq_change(connectivity_seq_get());
>> }
>> }
>> @@ -1228,6 +1231,7 @@ netdev_bsd_set_in4(struct netdev *netdev_, struct
>> in_addr addr,
>> netdev->netmask = mask;
>> }
>> }
>> +netdev_change_seq_changed(netdev_);
>> seq_change(connectivity_seq_get());
>> }
>> ovs_mutex_unlock(&netdev->mutex);
>> @@ -1526,6 +1530,7 @@ netdev_bsd_update_flags(struct netdev *netdev_,
>> enum netdev_flags off,
>> new_flags = (old_flags & ~nd_to_iff_flags(off)) |
>> nd_to_iff_flags(on);
>> if (new_flags != old_flags) {
>> error = set_flags(netdev_get_kernel_name(netdev_),
>> new_flags);
>> +netdev_change_seq_changed(netdev_);
>> seq_change(connectivity_seq_get());
>> }
>> }
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 4b36f52..1ab57cc 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -318,6 +318,7 @@ check_link_status(struct netdev_dpdk *dev)
>> rte_eth_link_get_nowait(dev->port_id, &link);
>>
>> if (dev->link.link_status != link.link_status) {
>> +netdev_change_seq_changed(&dev->up);
>> seq_change(connectivity_seq_get());
>>
>> dev->link_reset_cnt++;
>> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
>> index 995e923..359fc06 100644
>> --- a/lib/netdev-dummy.c
>> +++ b/lib/netdev-dummy.c
>> @@ -873,6 +873,7 @@ netdev_dummy_set_etheraddr(struct netdev *netdev,
>> ovs_mutex_lock(&dev->mutex);
>> if (!eth_addr_equals(dev->hwaddr, mac)) {
>> memcpy(dev->hwaddr, mac, ETH_ADDR_LEN);
>> +netdev_change_seq_changed(netdev);
>> seq_change(connectivity_seq_get());
>> }
>> ovs_mutex_unlock(&dev->mutex);
>> @@ -968,6 +969,7 @@ netdev_dummy_update_flags__(struct netdev_dummy
>> *netdev,
>> netdev->flags |= on;
>> netdev->flags &= ~off;
>> if (*old_flagsp != netdev->flags) {
>> +netdev_change_seq_changed(&netdev->up);
>> seq_change(connectivity_seq_get());
>> }
>>
>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
>> index f9634b6..2f61936 100644
>> --- a/lib/netdev-linux.c
>> +++ b/lib/netdev-linux.c
>> @@ -616,6 +616,7 @@ netdev_linux_changed(struct netdev_linux *dev,
>> unsigned int ifi_flags, unsigned int mask)
>> OVS_REQUIRES(dev->mutex)
>> {
>> +netdev_change_seq_changed(&dev->up);
>> seq_change(connectivity_seq_get());
>>
>> if ((dev->ifi_flags ^ ifi_flags) & IFF_RUNNING) {
>> dif
Re: [ovs-dev] [PATCHv8 1/2] tests/ofproto-dpif: Use vlog to test dpif behaviour.
> @@ -3702,6 +3710,7 @@ AT_SETUP([ofproto-dpif - ovs-appctl dpif/dump-flows - > MPLS actions that result i can you update test titles? YAMAMOTO Takashi ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCHv9] ofproto-dpif: Remove the flow_dumper thread.
From: Ethan Jackson
Previously, we had a separate flow_dumper thread that fetched flows from
the datapath to distribute to revalidator threads. This patch takes the
logic for dumping and pushes it into the revalidator threads, resulting
in simpler code with similar performance to the current code.
One thread, the "leader", is responsible for beginning and ending each
flow dump, maintaining the flow_limit, and checking whether the
revalidator threads need to exit. All revalidator threads dump,
revalidate, delete datapath flows and garbage collect ukeys.
Co-authored-by: Joe Stringer
Signed-off-by: Joe Stringer
---
v9: Update testsuite for also printing actions on flow_dump.
v8: Rebase.
v7: Add back logic (present in master) that deletes all flows older than
100ms if we are currently exceeding the flow limit.
Rebase.
v6: Shift ukeys hmaps from revalidators into udpif.
Documentation tidyups.
v5: Handle ukey creation race.
Style fixes.
v4: Rebase.
v3: First post.
---
ofproto/ofproto-dpif-upcall.c | 632 +++--
tests/ofproto-dpif.at |8 +-
2 files changed, 304 insertions(+), 336 deletions(-)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 938cfde..064d939 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -67,34 +67,27 @@ struct handler {
'mutex'. */
};
-/* A thread that processes each kernel flow handed to it by the flow_dumper
- * thread, updates OpenFlow statistics, and updates or removes the kernel flow
- * as necessary. */
+/* A thread that processes datapath flows, updates OpenFlow statistics, and
+ * updates or removes them if necessary. */
struct revalidator {
struct udpif *udpif; /* Parent udpif. */
char *name;/* Thread name. */
pthread_t thread; /* Thread ID. */
-struct hmap ukeys; /* Datapath flow keys. */
-
-uint64_t dump_seq;
-
-struct ovs_mutex mutex;/* Mutex guarding the following. */
-pthread_cond_t wake_cond;
-struct list udumps OVS_GUARDED;/* Unprocessed udumps. */
-size_t n_udumps OVS_GUARDED; /* Number of unprocessed udumps. */
+struct hmap *ukeys;/* Points into udpif->ukeys for this
+ revalidator. Used for GC phase. */
};
/* An upcall handler for ofproto_dpif.
*
* udpif has two logically separate pieces:
*
- *- A "dispatcher" thread that reads upcalls from the kernel and dispatches
- * them to one of several "handler" threads (see struct handler).
+ *- Miss handling threads led by a "dispatcher" thread that reads upcalls
+ * from the kernel and dispatches them to one of several "handler" threads
+ * (see struct handler).
*
- *- A "flow_dumper" thread that reads the kernel flow table and dispatches
- * flows to one of several "revalidator" threads (see struct
- * revalidator). */
+ *- Revalidation threads which read the datapath flow table and maintains
+ * them. */
struct udpif {
struct list list_node; /* In all_udpifs list. */
@@ -104,7 +97,6 @@ struct udpif {
uint32_t secret; /* Random seed for upcall hash. */
pthread_t dispatcher; /* Dispatcher thread ID. */
-pthread_t flow_dumper; /* Flow dumper thread ID. */
struct handler *handlers; /* Upcall handlers. */
size_t n_handlers;
@@ -112,14 +104,24 @@ struct udpif {
struct revalidator *revalidators; /* Flow revalidators. */
size_t n_revalidators;
-uint64_t last_reval_seq; /* 'reval_seq' at last revalidation. */
-struct seq *reval_seq; /* Incremented to force revalidation. */
-
-struct seq *dump_seq; /* Increments each dump iteration. */
-
struct latch exit_latch; /* Tells child threads to exit. */
+/* Revalidation. */
+struct seq *reval_seq; /* Incremented to force revalidation. */
+bool need_revalidate; /* As indicated by 'reval_seq'. */
+bool reval_exit; /* Set by leader on 'exit_latch. */
+pthread_barrier_t reval_barrier; /* Barrier used by revalidators. */
+struct dpif_flow_dump dump;/* DPIF flow dump state. */
long long int dump_duration; /* Duration of the last flow dump. */
+struct seq *dump_seq; /* Increments each dump iteration. */
+
+/* During the flow dump phase, revalidators insert into these with a random
+ * distribution. During the garbage collection phase, each revalidator
+ * takes care of garbage collecting one of these hmaps. */
+struct {
+struct ovs_mutex mutex;/* Guards the following. */
+struct hmap hmap OVS_GUARDED; /* Datapath flow keys. */
+} *ukeys;
/* Datapat
[ovs-dev] [PATCH] SubmittingPatches: Rename to CONTRIBUTING.
From: Joe Stringer This makes the GitHub interface aware of the contribution guidelines, so it will be displayed to contributors when they submit issues or pull requests. Signed-off-by: Joe Stringer --- CONTRIBUTING | 211 + SubmittingPatches | 211 - 2 files changed, 211 insertions(+), 211 deletions(-) create mode 100644 CONTRIBUTING delete mode 100644 SubmittingPatches diff --git a/CONTRIBUTING b/CONTRIBUTING new file mode 100644 index 000..d755186 --- /dev/null +++ b/CONTRIBUTING @@ -0,0 +1,211 @@ +How to Submit Patches for Open vSwitch +== + +Send changes to Open vSwitch as patches to dev@openvswitch.org. +One patch per email, please. More details are included below. + +If you are using Git, then "git format-patch" takes care of most of +the mechanics described below for you. + +Before You Start + + +Before you send patches at all, make sure that each patch makes sense. +In particular: + +- A given patch should not break anything, even if later + patches fix the problems that it causes. The source tree + should still build and work after each patch is applied. + (This enables "git bisect" to work best.) + +- A patch should make one logical change. Don't make + multiple, logically unconnected changes to disparate + subsystems in a single patch. + +- A patch that adds or removes user-visible features should + also update the appropriate user documentation or manpages. + +Testing is also important: + +- A patch that adds or deletes files should be tested with + "make distcheck" before submission. + +- A patch that modifies Linux kernel code should be at least + build-tested on various Linux kernel versions before + submission. I suggest versions 2.6.32 and whatever + the current latest release version is at the time. + +- A patch that modifies the ofproto or vswitchd code should be + tested in at least simple cases before submission. + +- A patch that modifies xenserver code should be tested on + XenServer before submission. + +Email Subject +- + +The subject line of your email should be in the following format: +[PATCH /] : + +- [PATCH /] indicates that this is the nth of a series + of m patches. It helps reviewers to read patches in the + correct order. You may omit this prefix if you are sending + only one patch. + +- : indicates the area of the Open vSwitch to which the + change applies (often the name of a source file or a + directory). You may omit it if the change crosses multiple + distinct pieces of code. + +- briefly describes the change. + +The subject, minus the [PATCH /] prefix, becomes the first line +of the commit's change log message. + +Description +--- + +The body of the email should start with a more thorough description of +the change. This becomes the body of the commit message, following +the subject. There is no need to duplicate the summary given in the +subject. + +Please limit lines in the description to 79 characters in width. + +The description should include: + +- The rationale for the change. + +- Design description and rationale (but this might be better + added as code comments). + +- Testing that you performed (or testing that should be done + but you could not for whatever reason). + +There is no need to describe what the patch actually changed, if the +reader can see it for himself. + +If the patch refers to a commit already in the Open vSwitch +repository, please include both the commit number and the subject of +the patch, e.g. 'commit 632d136c (vswitch: Remove restriction on +datapath names.)'. + +If you, the person sending the patch, did not write the patch +yourself, then the very first line of the body should take the form +"From: ", followed by a blank line. This +will automatically cause the named author to be credited with +authorship in the repository. If others contributed to the patch, but +are not the main authors, then please credit them as part of the +description (e.g. "Thanks to Bob J. User for reporting this bug."). + +Please sign off on the patch as a submitter, and be sure to have the +author(s) sign off for patches that you did not author. + +Simply include your name and email address as the last line of the commit +message before any comments (and author too, if that is not you): + +Signed-off-by: Author Name +Signed-off-by: Submitter Name + +By doing this, you are agreeing to the Developer's Certificate of Origin +(see below for more details). + +Developer's Certificate of Origin +- + +To help track the author of a patch as well as
[ovs-dev] [PATCH 3/3] ofproto-dpif.at: Fix races in dpif/dump-flows test
Signed-off-by: YAMAMOTO Takashi --- tests/ofproto-dpif.at | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 2537d30..dbf68b0 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -3678,14 +3678,15 @@ AT_CLEANUP AT_SETUP([ofproto-dpif - ovs-appctl dpif/dump-flows]) OVS_VSWITCHD_START([add-br br1 -- \ -set bridge br1 datapath-type=dummy fail-mode=secure]) +set bridge br1 datapath-type=dummy fail-mode=secure -- \ +set Open_vSwitch . other_config:max-idle=1]) ADD_OF_PORTS([br0], [1], [2]) ADD_OF_PORTS([br1], [3]) AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) AT_CHECK([ovs-appctl netdev-dummy/receive p2 'in_port(2),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0)']) 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)']) - +sleep 1 AT_CHECK([ovs-appctl dpif/dump-flows br0 | sort | STRIP_USED], [0], [dnl 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:never, actions:drop 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:never, actions:drop -- 1.8.3.1 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/3] ofproto-dpif.at: Use OVS_APP_EXIT_AND_WAIT where appropriate
Leftover of commit 0c473314.
("ofproto-dpif.at: Wait for the monitor's pidfile disappears where necessary")
Signed-off-by: YAMAMOTO Takashi
---
tests/ofproto-dpif.at | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 3018a84..2537d30 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -2135,7 +2135,7 @@ for i in 1 2 3; do
ovs-appctl netdev-dummy/receive p1
'in_port(1),eth(src=40:44:44:44:54:55,dst=50:54:00:00:00:07),eth_type(0x8100),vlan(vid=88,pcp=7),encap(eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no))'
done
OVS_WAIT_UNTIL([test `grep OFPT_PACKET_IN ofctl_monitor.log | wc -l` -ge 3])
-ovs-appctl -t ovs-ofctl exit
+OVS_APP_EXIT_AND_WAIT([ovs-ofctl])
AT_CHECK([cat ofctl_monitor.log | ofctl_strip], [0], [dnl
OFPT_PACKET_IN (OF1.2): total_len=64 in_port=1 (via action) data_len=64
(unbuffered)
--
1.8.3.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/3] ofproto-dpif.at: Fix some races
These races were exposed on my environment by commit c2a77f33.
("tests/ofproto-dpif: Use vlog to test dpif behaviour.")
Signed-off-by: YAMAMOTO Takashi
---
tests/ofproto-dpif.at | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 93add7f..3018a84 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -89,6 +89,7 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p7
'in_port(7),eth(src=50:54:00:00:00:
AT_CHECK([ovs-appctl netdev-dummy/receive p7
'in_port(7),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0e),eth_type(0x0800),ipv4(src=10.0.0.6,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
ovs-appctl time/warp 100
ovs-appctl time/warp 100
+sleep 1
AT_CHECK([cat ovs-vswitchd.log | grep 'in_port([[348]])' | FILTER_FLOW_INSTALL
| STRIP_XOUT], [0], [dnl
skb_priority(0),skb_mark(0/0),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/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),icmp(type=8/0,code=0/0),
actions:
skb_priority(0),skb_mark(0/0),in_port(3),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.3/0.0.0.0,dst=10.0.0.4/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),icmp(type=8/0,code=0/0),
actions:
@@ -3883,6 +3884,7 @@ table=0 in_port=1 actions=output(2)
])
AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
AT_CHECK([ovs-appctl netdev-dummy/receive p1
'in_port(1),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)'])
+sleep 1
AT_CHECK([ovs-appctl netdev-dummy/receive p1
'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
sleep 1
AT_CHECK([cat ovs-vswitchd.log | FILTER_FLOW_INSTALL | STRIP_XOUT], [0], [dnl
@@ -3956,7 +3958,9 @@ table=0 in_port=1,icmp,icmp_type=8 actions=output(2)
])
AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
AT_CHECK([ovs-appctl netdev-dummy/receive p1
'in_port(1),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)'])
+sleep 1
AT_CHECK([ovs-appctl netdev-dummy/receive p1
'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
+sleep 1
AT_CHECK([cat ovs-vswitchd.log | FILTER_FLOW_INSTALL | STRIP_XOUT], [0], [dnl
skb_priority(0),skb_mark(0/0),in_port(1),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/0xff,tos=0/0,ttl=64/0,frag=no/0xff),icmp(type=8/0xff,code=0/0),
actions:
])
@@ -4102,7 +4106,7 @@ ovs-appctl time/stop
ovs-appctl time/warp 5000
AT_CHECK([ovs-appctl netdev-dummy/receive p7
'in_port(1),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 netdev-dummy/receive p7
'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
-
+sleep 1
AT_CHECK([cat ovs-vswitchd.log | FILTER_FLOW_INSTALL | STRIP_XOUT], [0], [dnl
skb_priority(0),skb_mark(0/0),in_port(7),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),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),icmp(type=8/0,code=0/0),
actions:
skb_priority(0),skb_mark(0/0),in_port(7),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4/0.0.0.0,dst=10.0.0.3/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff),icmp(type=8/0,code=0/0),
actions:
@@ -4182,6 +4186,7 @@ in_port=1 actions=output:2
])
AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
AT_CHECK([ovs-appctl netdev-dummy/receive p1
'in_port(1),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)'])
+sleep 1
AT_CHECK([ovs-appctl netdev-dummy/receive p1
'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
sleep 1
AT_CHECK([cat ovs-vswitchd.log | FILTER_FLOW_INSTALL | STRIP_XOUT], [0], [dnl
@@ -4297,8 +4302,10 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
dnl ECN bits are always copied out, but don't use 0x3 (CE), since that
dnl will cause the packet to be dropped.
AT_CHECK([ovs-appctl netdev-dummy/receive p1
'in_port(1),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=0xfd,ttl=128,frag=no),icmp(type=8,code=0)'])
+sleep 1
AT_CHECK([ovs-appctl netdev-dummy/receive p1
'in_port
[ovs-dev] [PATCH] ofproto-dpif.at: Update MPLS test names.
From: Joe Stringer Signed-off-by: Joe Stringer --- tests/ofproto-dpif.at |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 93add7f..b12b4fe 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -3706,7 +3706,7 @@ skb_priority(0),skb_mark(0/0),in_port(p3),eth(src=50:54:00:00:00:09/00:00:00:00: OVS_VSWITCHD_STOP AT_CLEANUP -AT_SETUP([ofproto-dpif - ovs-appctl dpif/dump-flows - MPLS actions that result in a userspace action]) +AT_SETUP([ofproto-dpif - MPLS actions that result in a userspace action]) OVS_VSWITCHD_START([dnl add-port br0 p1 -- set Interface p1 type=dummy ]) @@ -3743,7 +3743,7 @@ OVS_VSWITCHD_STOP AT_CLEANUP -AT_SETUP([ofproto-dpif - ovs-appctl dpif/dump-flows - MPLS actions that result in a drop]) +AT_SETUP([ofproto-dpif - MPLS actions that result in a drop]) OVS_VSWITCHD_START([dnl add-port br0 p1 -- set Interface p1 type=dummy ]) -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCHv2] SubmittingPatches: Rename to CONTRIBUTING.
From: Joe Stringer This makes the GitHub interface aware of the contribution guidelines, so it will be displayed to contributors when they submit issues or pull requests. Signed-off-by: Joe Stringer --- v2: Update Makefile.am and OPENFLOW-1.1+. --- CONTRIBUTING | 211 + Makefile.am |2 +- OPENFLOW-1.1+ |2 +- SubmittingPatches | 211 - 4 files changed, 213 insertions(+), 213 deletions(-) create mode 100644 CONTRIBUTING delete mode 100644 SubmittingPatches diff --git a/CONTRIBUTING b/CONTRIBUTING new file mode 100644 index 000..d755186 --- /dev/null +++ b/CONTRIBUTING @@ -0,0 +1,211 @@ +How to Submit Patches for Open vSwitch +== + +Send changes to Open vSwitch as patches to dev@openvswitch.org. +One patch per email, please. More details are included below. + +If you are using Git, then "git format-patch" takes care of most of +the mechanics described below for you. + +Before You Start + + +Before you send patches at all, make sure that each patch makes sense. +In particular: + +- A given patch should not break anything, even if later + patches fix the problems that it causes. The source tree + should still build and work after each patch is applied. + (This enables "git bisect" to work best.) + +- A patch should make one logical change. Don't make + multiple, logically unconnected changes to disparate + subsystems in a single patch. + +- A patch that adds or removes user-visible features should + also update the appropriate user documentation or manpages. + +Testing is also important: + +- A patch that adds or deletes files should be tested with + "make distcheck" before submission. + +- A patch that modifies Linux kernel code should be at least + build-tested on various Linux kernel versions before + submission. I suggest versions 2.6.32 and whatever + the current latest release version is at the time. + +- A patch that modifies the ofproto or vswitchd code should be + tested in at least simple cases before submission. + +- A patch that modifies xenserver code should be tested on + XenServer before submission. + +Email Subject +- + +The subject line of your email should be in the following format: +[PATCH /] : + +- [PATCH /] indicates that this is the nth of a series + of m patches. It helps reviewers to read patches in the + correct order. You may omit this prefix if you are sending + only one patch. + +- : indicates the area of the Open vSwitch to which the + change applies (often the name of a source file or a + directory). You may omit it if the change crosses multiple + distinct pieces of code. + +- briefly describes the change. + +The subject, minus the [PATCH /] prefix, becomes the first line +of the commit's change log message. + +Description +--- + +The body of the email should start with a more thorough description of +the change. This becomes the body of the commit message, following +the subject. There is no need to duplicate the summary given in the +subject. + +Please limit lines in the description to 79 characters in width. + +The description should include: + +- The rationale for the change. + +- Design description and rationale (but this might be better + added as code comments). + +- Testing that you performed (or testing that should be done + but you could not for whatever reason). + +There is no need to describe what the patch actually changed, if the +reader can see it for himself. + +If the patch refers to a commit already in the Open vSwitch +repository, please include both the commit number and the subject of +the patch, e.g. 'commit 632d136c (vswitch: Remove restriction on +datapath names.)'. + +If you, the person sending the patch, did not write the patch +yourself, then the very first line of the body should take the form +"From: ", followed by a blank line. This +will automatically cause the named author to be credited with +authorship in the repository. If others contributed to the patch, but +are not the main authors, then please credit them as part of the +description (e.g. "Thanks to Bob J. User for reporting this bug."). + +Please sign off on the patch as a submitter, and be sure to have the +author(s) sign off for patches that you did not author. + +Simply include your name and email address as the last line of the commit +message before any comments (and author too, if that is not you): + +Signed-off-by: Author Name +Signed-off-by: Submitter Name + +By doing this, you are agreeing to the Developer's Certificate of Origin +(see below for more details). + +Developer's Ce
Re: [ovs-dev] [PATCHv8 1/2] tests/ofproto-dpif: Use vlog to test dpif behaviour.
Woops, I posted a patch to do so here: http://openvswitch.org/pipermail/dev/2014-April/038737.html On 10 April 2014 16:24, YAMAMOTO Takashi wrote: > > @@ -3702,6 +3710,7 @@ AT_SETUP([ofproto-dpif - ovs-appctl > dpif/dump-flows - MPLS actions that result i > > can you update test titles? > > YAMAMOTO Takashi > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] dpif-netdev: Move hash function out of the recirc action, into its own action
Currently recirculation action can optionally compute hash. This patch
adds a hash action that is independent of the recirc action, which
no longer computes hash. For megaflow bond with recirc, the output
to a bond port action will look like:
hash(hash_l4(0)), recric()
Obviously, when a recirculation application that does not depend on
hash value can just use the recirc action alone.
Signed-off-by: Andy Zhou
---
include/linux/openvswitch.h | 21 +
lib/dpif-netdev.c| 30 +-
lib/dpif.c |1 +
lib/odp-execute.c|1 +
lib/odp-util.c | 26 +-
ofproto/ofproto-dpif-xlate.c | 19 ---
6 files changed, 57 insertions(+), 41 deletions(-)
diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index a88f6f1..11a4969 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -537,26 +537,22 @@ struct ovs_action_push_vlan {
/* Data path hash algorithm for computing Datapath hash.
*
- * The Algorithm type only specifies the fields in a flow
+ * The algorithm type only specifies the fields in a flow
* will be used as part of the hash. Each datapath is free
* to use its own hash algorithm. The hash value will be
* opaque to the user space daemon.
*/
-enum ovs_recirc_hash_alg {
- OVS_RECIRC_HASH_ALG_NONE,
- OVS_RECIRC_HASH_ALG_L4,
+enum ovs_hash_alg {
+ OVS_HASH_ALG_L4,
};
/*
- * struct ovs_action_recirc - %OVS_ACTION_ATTR_RECIRC action argument.
- * @recirc_id: The Recirculation label, Zero is invalid.
+ * struct ovs_action_hash - %OVS_ACTION_ATTR_HASH action argument.
* @hash_alg: Algorithm used to compute hash prior to recirculation.
- * @hash_bias: bias used for computing hash. used to compute hash prior to
- * recirculation.
+ * @hash_bias: bias used for computing hash.
*/
-struct ovs_action_recirc {
- uint32_t hash_alg; /* One of ovs_dp_hash_alg. */
+struct ovs_action_hash {
+ uint32_t hash_alg; /* One of ovs_hash_alg. */
uint32_t hash_bias;
- uint32_t recirc_id;/* Recirculation label. */
};
/**
@@ -599,7 +595,8 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_SAMPLE, /* Nested OVS_SAMPLE_ATTR_*. */
OVS_ACTION_ATTR_PUSH_MPLS,/* struct ovs_action_push_mpls. */
OVS_ACTION_ATTR_POP_MPLS, /* __be16 ethertype. */
- OVS_ACTION_ATTR_RECIRC, /* struct ovs_action_recirc. */
+ OVS_ACTION_ATTR_RECIRC, /* u32 recirc_id. */
+ OVS_ACTION_ATTR_HASH, /* struct ovs_action_hash. */
__OVS_ACTION_ATTR_MAX
};
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 9ad9386..75cfd9e 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2138,25 +2138,29 @@ dp_execute_cb(void *aux_, struct ofpbuf *packet,
break;
}
+case OVS_ACTION_ATTR_HASH: {
+const struct ovs_action_hash *hash_act;
+hash_act = nl_attr_get(a);
+if (hash_act->hash_alg == OVS_HASH_ALG_L4) {
+uint32_t l4_hash;
+
+l4_hash = flow_hash_symmetric_l4(aux->key, hash_act->hash_bias);
+if (!l4_hash) {
+l4_hash = 1; /* 0 is not valid */
+}
+
+md->dp_hash = l4_hash;
+}
+break;
+}
+
case OVS_ACTION_ATTR_RECIRC:
if (*depth < MAX_RECIRC_DEPTH) {
struct pkt_metadata recirc_md = *md;
struct ofpbuf *recirc_packet;
-const struct ovs_action_recirc *act;
recirc_packet = may_steal ? packet : ofpbuf_clone(packet);
-
-act = nl_attr_get(a);
-recirc_md.recirc_id = act->recirc_id;
-recirc_md.dp_hash = 0;
-
-if (act->hash_alg == OVS_RECIRC_HASH_ALG_L4) {
-recirc_md.dp_hash = flow_hash_symmetric_l4(aux->key,
- act->hash_bias);
-if (!recirc_md.dp_hash) {
-recirc_md.dp_hash = 1; /* 0 is not valid */
-}
-}
+recirc_md.recirc_id = nl_attr_get_u32(a);
(*depth)++;
dp_netdev_input(aux->dp, recirc_packet, &recirc_md);
diff --git a/lib/dpif.c b/lib/dpif.c
index a8bd674..41b8eb7 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1134,6 +1134,7 @@ dpif_execute_helper_cb(void *aux_, struct ofpbuf *packet,
case OVS_ACTION_ATTR_SAMPLE:
case OVS_ACTION_ATTR_UNSPEC:
case OVS_ACTION_ATTR_RECIRC:
+case OVS_ACTION_ATTR_HASH:
case __OVS_ACTION_ATTR_MAX:
OVS_NOT_REACHED();
}
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 37e44e3..12ad679 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -208,6 +208,7 @@ odp_execute_actions__(void *dp, struct ofpbuf *packet, bool
steal,
case OVS_ACTION_ATTR_OUTPUT:
case OVS_ACTION_ATTR_USERSPACE:
case OVS_ACTION_A
Re: [ovs-dev] [PATCH] ofproto-dpif.at: Update MPLS test names.
> From: Joe Stringer thanks. Acked-by: YAMAMOTO Takashi > > Signed-off-by: Joe Stringer > --- > tests/ofproto-dpif.at |4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 93add7f..b12b4fe 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -3706,7 +3706,7 @@ > skb_priority(0),skb_mark(0/0),in_port(p3),eth(src=50:54:00:00:00:09/00:00:00:00: > OVS_VSWITCHD_STOP > AT_CLEANUP > > -AT_SETUP([ofproto-dpif - ovs-appctl dpif/dump-flows - MPLS actions that > result in a userspace action]) > +AT_SETUP([ofproto-dpif - MPLS actions that result in a userspace action]) > OVS_VSWITCHD_START([dnl > add-port br0 p1 -- set Interface p1 type=dummy > ]) > @@ -3743,7 +3743,7 @@ OVS_VSWITCHD_STOP > AT_CLEANUP > > > -AT_SETUP([ofproto-dpif - ovs-appctl dpif/dump-flows - MPLS actions that > result in a drop]) > +AT_SETUP([ofproto-dpif - MPLS actions that result in a drop]) > OVS_VSWITCHD_START([dnl > add-port br0 p1 -- set Interface p1 type=dummy > ]) > -- > 1.7.10.4 > > ___ > 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] ofproto-dpif.at: Update MPLS test names.
Thanks. I am not a committer, could you push this for me? On 10 Apr 2014 17:32, "YAMAMOTO Takashi" wrote: > > From: Joe Stringer > > thanks. > > Acked-by: YAMAMOTO Takashi > > > > > Signed-off-by: Joe Stringer > > --- > > tests/ofproto-dpif.at |4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > > index 93add7f..b12b4fe 100644 > > --- a/tests/ofproto-dpif.at > > +++ b/tests/ofproto-dpif.at > > @@ -3706,7 +3706,7 @@ > skb_priority(0),skb_mark(0/0),in_port(p3),eth(src=50:54:00:00:00:09/00:00:00:00: > > OVS_VSWITCHD_STOP > > AT_CLEANUP > > > > -AT_SETUP([ofproto-dpif - ovs-appctl dpif/dump-flows - MPLS actions that > result in a userspace action]) > > +AT_SETUP([ofproto-dpif - MPLS actions that result in a userspace > action]) > > OVS_VSWITCHD_START([dnl > > add-port br0 p1 -- set Interface p1 type=dummy > > ]) > > @@ -3743,7 +3743,7 @@ OVS_VSWITCHD_STOP > > AT_CLEANUP > > > > > > -AT_SETUP([ofproto-dpif - ovs-appctl dpif/dump-flows - MPLS actions that > result in a drop]) > > +AT_SETUP([ofproto-dpif - MPLS actions that result in a drop]) > > OVS_VSWITCHD_START([dnl > > add-port br0 p1 -- set Interface p1 type=dummy > > ]) > > -- > > 1.7.10.4 > > > > ___ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
