[ovs-dev] [port-renumbering 1/8] ofproto-dpif: Improve comment.

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 ofproto/ofproto-dpif.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ff77903..bfa9a9a 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -436,7 +436,7 @@ struct dpif_backer {
 struct timer next_expiration;
 
 struct ovs_rwlock odp_to_ofport_lock;
-struct hmap odp_to_ofport_map OVS_GUARDED; /* ODP port to ofport map. */
+struct hmap odp_to_ofport_map OVS_GUARDED; /* Contains "struct ofport"s. */
 
 struct simap tnl_backers;  /* Set of dpif ports backing tunnels. */
 
-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [port-renumbering 7/8] bridge: Reconfigure in single pass.

[Ben Pfaff]

This greatly simplifies the reconfiguration code, making it much easier
to understand and modify.  The old multi-pass configuration had the
property that it didn't delay block packet processing as much, but that's
not much of a worry anymore now that latency critical activities have
been moved outside the main thread.

Signed-off-by: Ben Pfaff 
---
 vswitchd/bridge.c |  632 +
 1 file changed, 202 insertions(+), 430 deletions(-)

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 0b0e4d7..581f87c 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -63,33 +63,20 @@ VLOG_DEFINE_THIS_MODULE(bridge);
 
 COVERAGE_DEFINE(bridge_reconfigure);
 
-/* Configuration of an uninstantiated iface. */
-struct if_cfg {
-struct hmap_node hmap_node; /* Node in bridge's if_cfg_todo. */
-const struct ovsrec_interface *cfg; /* Interface record. */
-const struct ovsrec_port *parent;   /* Parent port record. */
-ofp_port_t ofport;  /* Requested OpenFlow port number. */
-};
-
-/* OpenFlow port slated for removal from ofproto. */
-struct ofpp_garbage {
-struct list list_node;  /* Node in bridge's ofpp_garbage. */
-ofp_port_t ofp_port;/* Port to be deleted. */
-};
-
 struct iface {
-/* These members are always valid. */
+/* These members are always valid.
+ *
+ * They are immutable: they never change between iface_create() and
+ * iface_destroy(). */
 struct list port_elem;  /* Element in struct port's "ifaces" list. */
 struct hmap_node name_node; /* In struct bridge's "iface_by_name" hmap. */
+struct hmap_node ofp_port_node; /* In struct bridge's "ifaces" hmap. */
 struct port *port;  /* Containing port. */
 char *name; /* Host network device name. */
-
-/* These members are valid only after bridge_reconfigure() causes them to
- * be initialized. */
-struct hmap_node ofp_port_node; /* In struct bridge's "ifaces" hmap. */
-ofp_port_t ofp_port;/* OpenFlow port number, */
-/* OFPP_NONE if unknown. */
 struct netdev *netdev;  /* Network device. */
+ofp_port_t ofp_port;/* OpenFlow port number. */
+
+/* These members are valid only within bridge_reconfigure(). */
 const char *type;   /* Usually same as cfg->type. */
 const struct ovsrec_interface *cfg;
 };
@@ -130,13 +117,12 @@ struct bridge {
 struct hmap ifaces; /* "struct iface"s indexed by ofp_port. */
 struct hmap iface_by_name;  /* "struct iface"s indexed by name. */
 
-struct list ofpp_garbage;   /* "struct ofpp_garbage" slated for removal. */
-struct hmap if_cfg_todo;/* "struct if_cfg"s slated for creation.
-   Indexed on 'cfg->name'. */
-
 /* Port mirroring. */
 struct hmap mirrors;/* "struct mirror" indexed by UUID. */
 
+/* Used during reconfiguration. */
+struct shash wanted_ports;
+
 /* Synthetic local port if necessary. */
 struct ovsrec_port synth_local_port;
 struct ovsrec_interface synth_local_iface;
@@ -183,10 +169,8 @@ static long long int iface_stats_timer = LLONG_MIN;
  * This allows the rest of the code to catch up on important things like
  * forwarding packets. */
 #define OFP_PORT_ACTION_WINDOW 10
-static bool reconfiguring = false;
 
 static void add_del_bridges(const struct ovsrec_open_vswitch *);
-static void bridge_update_ofprotos(void);
 static void bridge_create(const struct ovsrec_bridge *);
 static void bridge_destroy(struct bridge *);
 static struct bridge *bridge_lookup(const char *name);
@@ -194,9 +178,8 @@ static unixctl_cb_func bridge_unixctl_dump_flows;
 static unixctl_cb_func bridge_unixctl_reconnect;
 static size_t bridge_get_controllers(const struct bridge *br,
  struct ovsrec_controller ***controllersp);
-static void bridge_add_del_ports(struct bridge *,
- const unsigned long int *splinter_vlans);
-static void bridge_refresh_ofp_port(struct bridge *);
+static void bridge_del_ports(struct bridge *,
+ const struct shash *wanted_ports);
 static void bridge_configure_flow_miss_model(const char *opt);
 static void bridge_configure_datapath_id(struct bridge *);
 static void bridge_configure_netflow(struct bridge *);
@@ -216,9 +199,6 @@ static void bridge_pick_local_hw_addr(struct bridge *,
 static uint64_t bridge_pick_datapath_id(struct bridge *,
 const uint8_t bridge_ea[ETH_ADDR_LEN],
 struct iface *hw_addr_iface);
-static void bridge_queue_if_cfg(struct bridge *,
-const struct ovsrec_interface *,
-const struct ovsrec_port *);
 static uint64_t dpid_from_hash(const void *, size_t nbytes);
 static bool bridge_has_bond_fake_iface(const struc

[ovs-dev] [port-renumbering 3/8] ofproto: Make ofproto_enumerate_types() match its comment.

[Ben Pfaff]

The comment says that it clears the passed-in sset, but it didn't.  The bug
didn't actually affect any of the existing callers, which all passed in an
empty sset.

Signed-off-by: Ben Pfaff 
---
 ofproto/ofproto.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 3a60328..1a1113f 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -432,6 +432,7 @@ ofproto_enumerate_types(struct sset *types)
 {
 size_t i;
 
+sset_clear(types);
 for (i = 0; i < n_ofproto_classes; i++) {
 ofproto_classes[i]->enumerate_types(types);
 }
-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [port-renumbering 2/8] ofproto-provider: Add comment on struct ofport.

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 ofproto/ofproto-provider.h |2 ++
 1 file changed, 2 insertions(+)

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 54d97f1..600b92b 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -162,6 +162,8 @@ struct ofport *ofproto_get_port(const struct ofproto *, 
ofp_port_t ofp_port);
 
 /* An OpenFlow port within a "struct ofproto".
  *
+ * The port's name is netdev_get_name(port->netdev).
+ *
  * With few exceptions, ofproto implementations may look at these fields but
  * should not modify them. */
 struct ofport {
-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [port-renumbering 8/8] bridge: Support changing port numbers.

[Ben Pfaff]

Feature #21293.
Requested-by: Anuprem Chalvadi 
Signed-off-by: Ben Pfaff 
---
 NEWS |3 ++
 vswitchd/bridge.c|   93 --
 vswitchd/vswitch.xml |   21 +++-
 3 files changed, 99 insertions(+), 18 deletions(-)

diff --git a/NEWS b/NEWS
index 8556083..ce968f2 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,9 @@ Post-v2.0.0
  * OVS limits the OpenFlow port numbers it assigns to port 32767 and
below, leaving port numbers above that range free for assignment
by the controller.
+ * ovs-vswitchd now honors changes to the "ofport_request" column
+   in the Interface table by changing the port's OpenFlow port
+   number.
- ovs-vswitchd.conf.db.5 man page will contain graphviz/dot
  diagram only if graphviz package was installed at the build time.
- Support for Linux kernels up to 3.11
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 581f87c..131b800 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -75,6 +75,7 @@ struct iface {
 char *name; /* Host network device name. */
 struct netdev *netdev;  /* Network device. */
 ofp_port_t ofp_port;/* OpenFlow port number. */
+ofp_port_t requested_ofp_port; /* Port number requested previously. */
 
 /* These members are valid only within bridge_reconfigure(). */
 const char *type;   /* Usually same as cfg->type. */
@@ -247,6 +248,8 @@ static void iface_refresh_cfm_stats(struct iface *);
 static void iface_refresh_stats(struct iface *);
 static void iface_refresh_status(struct iface *);
 static bool iface_is_synthetic(const struct iface *);
+static ofp_port_t iface_get_requested_ofp_port(
+const struct ovsrec_interface *);
 static ofp_port_t iface_pick_ofport(const struct ovsrec_interface *);
 
 /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
@@ -636,6 +639,7 @@ bridge_delete_ports(struct bridge *br)
 n = allocated = 0;
 
 OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) {
+ofp_port_t requested_ofp_port;
 struct iface *iface;
 
 iface = iface_lookup(br, ofproto_port.name);
@@ -657,6 +661,43 @@ bridge_delete_ports(struct bridge *br)
 goto delete;
 }
 
+/* If the requested OpenFlow port for 'iface' changed, and it's not
+ * already the correct port, then we might want to temporarily delete
+ * this interface, so we can add it back again with the new OpenFlow
+ * port number. */
+requested_ofp_port = iface_get_requested_ofp_port(iface->cfg);
+if (requested_ofp_port != iface->requested_ofp_port &&
+requested_ofp_port != OFPP_NONE &&
+requested_ofp_port != iface->ofp_port) {
+ofp_port_t victim_request;
+struct iface *victim;
+
+/* Check for an existing OpenFlow port currently occupying
+ * 'iface''s requested port number.  If there isn't one, then
+ * delete this port.  Otherwise we need to consider further. */
+victim = iface_from_ofp_port(br, requested_ofp_port);
+if (!victim) {
+goto delete;
+}
+
+/* 'victim' is a port currently using 'iface''s requested port
+ * number.  Unless 'victim' specifically requested that port
+ * number, too, then we can delete both 'iface' and 'victim'
+ * temporarily.  (We'll add both of them back again later with new
+ * OpenFlow port numbers.)
+ *
+ * If 'victim' did request port number 'requested_ofp_port', just
+ * like 'iface', then that's a configuration inconsistency that we
+ * can't resolve.  We might as well let it keep its current port
+ * number. */
+victim_request = iface_get_requested_ofp_port(victim->cfg);
+if (victim_request != requested_ofp_port) {
+del = add_ofp_port(victim->ofp_port, del, &n, &allocated);
+iface_destroy(victim);
+goto delete;
+}
+}
+
 continue;
 
 delete:
@@ -671,7 +712,8 @@ bridge_delete_ports(struct bridge *br)
 }
 
 static void
-bridge_add_ports(struct bridge *br, const struct shash *wanted_ports)
+bridge_add_ports__(struct bridge *br, const struct shash *wanted_ports,
+   bool with_requested_port)
 {
 struct shash_node *port_node;
 
@@ -681,16 +723,33 @@ bridge_add_ports(struct bridge *br, const struct shash 
*wanted_ports)
 
 for (i = 0; i < port_cfg->n_interfaces; i++) {
 const struct ovsrec_interface *iface_cfg = port_cfg->interfaces[i];
-struct iface *iface = iface_lookup(br, iface_cfg->name);
+ofp_port_t requested_ofp_port;
+
+requested_ofp_port = iface_get_requested_ofp_port(iface_cfg);
+if ((requested_ofp_port != OFPP_NONE) == with_requested_port) {
+ 

[ovs-dev] [port-renumbering 5/8] netdev: Log a warning when netdev_set_config() fails.

[Ben Pfaff]

This allows its callers to avoid duplicating the code.

Signed-off-by: Ben Pfaff 
---
 lib/netdev.c  |   10 --
 utilities/ovs-dpctl.c |2 --
 vswitchd/bridge.c |9 +
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/lib/netdev.c b/lib/netdev.c
index 5ed6062..159ca51 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -386,8 +386,14 @@ netdev_set_config(struct netdev *netdev, const struct smap 
*args)
 {
 if (netdev->netdev_class->set_config) {
 const struct smap no_args = SMAP_INITIALIZER(&no_args);
-return netdev->netdev_class->set_config(netdev,
-args ? args : &no_args);
+int error;
+
+error = netdev->netdev_class->set_config(netdev,
+ args ? args : &no_args);
+if (error) {
+VLOG_WARN("%s: could not set configuration (%s)",
+  netdev_get_name(netdev), ovs_strerror(error));
+}
 } else if (args && !smap_is_empty(args)) {
 VLOG_WARN("%s: arguments provided to device that is not configurable",
   netdev_get_name(netdev));
diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
index 29f1151..bd99834 100644
--- a/utilities/ovs-dpctl.c
+++ b/utilities/ovs-dpctl.c
@@ -350,7 +350,6 @@ dpctl_add_if(int argc OVS_UNUSED, char *argv[])
 
 error = netdev_set_config(netdev, &args);
 if (error) {
-ovs_error(error, "%s: failed to configure network device", name);
 goto next;
 }
 
@@ -459,7 +458,6 @@ dpctl_set_if(int argc, char *argv[])
 error = netdev_set_config(netdev, &args);
 smap_destroy(&args);
 if (error) {
-ovs_error(error, "%s: failed to configure network device", name);
 goto next;
 }
 
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 2b11c5b..0b0e4d7 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1343,14 +1343,7 @@ static int
 iface_set_netdev_config(const struct ovsrec_interface *iface_cfg,
 struct netdev *netdev)
 {
-int error;
-
-error = netdev_set_config(netdev, &iface_cfg->options);
-if (error) {
-VLOG_WARN("could not configure network device %s (%s)",
-  iface_cfg->name, ovs_strerror(error));
-}
-return error;
+return netdev_set_config(netdev, &iface_cfg->options);
 }
 
 /* This function determines whether 'ofproto_port', which is attached to
-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [port-renumbering 4/8] ofproto: Fix alphabetical order in list of structs.

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 ofproto/ofproto.h |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index d734eab..6197278 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -36,12 +36,12 @@ struct bfd_cfg;
 struct cfm_settings;
 struct cls_rule;
 struct netdev;
-struct ofproto;
+struct netdev_stats;
 struct ofport;
+struct ofproto;
 struct shash;
 struct simap;
 struct smap;
-struct netdev_stats;
 
 struct ofproto_controller_info {
 bool is_connected;
-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [port-renumbering 6/8] lacp: Make lacp negotiation test hard-code aggregation keys.

[Ben Pfaff]

The lacp implementation takes the aggregation key from the key or portid
of the first slave to be added to the lacp object.  When multiple slaves
are added initially to a lacp object (the most common case), which is the
"first" is arbitrary.  Until now, it seems that the "first" was actually
predictable enough for the tests to (unknowingly) rely on it, but an
upcoming commit will break that.  This commit fixes the test by forcing
a particular aggregation key for each lacp object.

Signed-off-by: Ben Pfaff 
---
 tests/lacp.at |   12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tests/lacp.at b/tests/lacp.at
index ffc8996..ede7c8f 100644
--- a/tests/lacp.at
+++ b/tests/lacp.at
@@ -126,8 +126,10 @@ OVS_VSWITCHD_START(
   [add-bond br0 bond0 p0 p1 bond_mode=balance-tcp lacp=active \
 other-config:lacp-time=fast \
 other-config:bond-rebalance-interval=0 -- \
-   set interface p0 type=patch options:peer=p2 ofport_request=1 -- \
-   set interface p1 type=patch options:peer=p3 ofport_request=2 -- \
+   set interface p0 type=patch options:peer=p2 ofport_request=1 \
+other-config:lacp-aggregation-key=2 -- \
+   set interface p1 type=patch options:peer=p3 ofport_request=2 \
+other-config:lacp-aggregation-key=2 -- \
add-br br1 -- \
set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \
@@ -135,8 +137,10 @@ OVS_VSWITCHD_START(
add-bond br1 bond1 p2 p3 bond_mode=balance-tcp lacp=active \
 other-config:lacp-time=fast \
 other-config:bond-rebalance-interval=0 -- \
-   set interface p2 type=patch options:peer=p0 ofport_request=3 -- \
-   set interface p3 type=patch options:peer=p1 ofport_request=4 --])
+   set interface p2 type=patch options:peer=p0 ofport_request=3 \
+other-config:lacp-aggregation-key=4 -- \
+   set interface p3 type=patch options:peer=p1 ofport_request=4 \
+other-config:lacp-aggregation-key=4 --])
 
 AT_CHECK([ovs-appctl netdev-dummy/set-admin-state up], 0, [OK
 ])
-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC 1/4] compiler: Add OVS_CONSTRUCTOR to mark functions as init functions

[Helmut Schaa]

On Wed, Dec 11, 2013 at 12:10 AM, Gurucharan Shetty  wrote:
> On Tue, Dec 10, 2013 at 5:49 AM, Helmut Schaa
>  wrote:
>> Functions marked with OVS_CONSTRUCTOR are called unconditionally
>> before main.
>>
>> Signed-off-by: Helmut Schaa 
>> ---
>>
>> This works with gcc (tested), should work with clang (untested)
>> but does not work with MSVS.
>>
>> Could anyone using MSVC try if the solution described at
>> http://stackoverflow.com/questions/1113409/attribute-constructor-equivalent-in-vc
>> is suitable?
>>
>> Is MSVC even supported in OVS?
>
> There is work going on to port OVS to windows and MSVC is the compiler
> for that work.
> I did try out the solution given in the stackoverflow link given. It
> does compile and the initialize() function gets called before main().

Great, I'll update the patch accordingly after some more comments ...
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] util: Remove unused count_1bits_8

[Jarno Rajahalme]

On Dec 10, 2013, at 5:44 PM, Ben Pfaff  wrote:

> On Wed, Dec 11, 2013 at 10:42:02AM +0900, Simon Horman wrote:
>> On Tue, Dec 10, 2013 at 05:40:00PM -0800, Ben Pfaff wrote:
>>> On Wed, Dec 11, 2013 at 10:34:14AM +0900, Simon Horman wrote:
 On Tue, Dec 10, 2013 at 05:07:14PM -0800, Ben Pfaff wrote:
> On Wed, Dec 11, 2013 at 10:01:12AM +0900, Simon Horman wrote:
>> Remove count_1bits_8() from util.c.
>> It appears to be unused since
>> c3cc4d2dd2658238 ("util: Better count_1bits().")
>> 
>> Cc: Jarno Rajahalme 
>> Signed-off-by: Simon Horman 
> 
> It's used from inline functions defined in util.h.
 
 Ok, I guess not under all circumstances as this seemed to work for me.
 
 I'll look into this more closely but I was seeing
 a warning about it being undeclared from sparse.
>>> 
>>> I posted a fix for that warning, and Jarno already reviewed it, but I
>>> hadn't pushed it because I'm working hard on a high-priority issue.
>>> I'll do that in a minute since it's causing you trouble too.
>> 
>> Thanks, sorry for the noise, I can easily imagine higher-priority issues.
> 
> Warnings are really annoying, especially since I normally compile with
> -Werror, so of course it's fine to propose fixes.  Thank you.


I just realized that I failed to exclude the definition of count_1bits_8 for 
64-bit builds. It is only used with 32-bit code now. I’ll propose a patch in a 
moment.

  Jarno

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCHv4 3/4] ofproto-dpif: Only run bundles when lacp or bonds are enabled

[Joe Stringer]

When dealing with a large number of ports, bundle_run() and
bundle_wait() add significant unnecessary processing to the main run
loop, even when there are no bonds and lacp is not configured. This
patch skips such execution if it is unneeded, reducing CPU usage from
about 25% to about 20% in a test environment of 5000 internal ports and
50 tunnel ports running bfd.

Signed-off-by: Joe Stringer 
---
v4: Rebase
---
 ofproto/ofproto-dpif.c |   21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index a4334cc..3235891 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -488,6 +488,7 @@ struct ofproto_dpif {
 struct hmap bundles;/* Contains "struct ofbundle"s. */
 struct mac_learning *ml;
 bool has_bonded_bundles;
+bool lacp_enabled;
 struct mbridge *mbridge;
 
 /* Facets. */
@@ -1254,6 +1255,7 @@ construct(struct ofproto *ofproto_)
 ofproto->ml = mac_learning_create(MAC_ENTRY_DEFAULT_IDLE_TIME);
 ofproto->mbridge = mbridge_create();
 ofproto->has_bonded_bundles = false;
+ofproto->lacp_enabled = false;
 ovs_mutex_init(&ofproto->stats_mutex);
 ovs_mutex_init(&ofproto->vsp_mutex);
 
@@ -1477,7 +1479,6 @@ static int
 run(struct ofproto *ofproto_)
 {
 struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
-struct ofbundle *bundle;
 uint64_t new_seq;
 int error;
 
@@ -1521,8 +1522,12 @@ run(struct ofproto *ofproto_)
 
 ofproto->change_seq = new_seq;
 }
-HMAP_FOR_EACH (bundle, hmap_node, &ofproto->bundles) {
-bundle_run(bundle);
+if (ofproto->lacp_enabled || ofproto->has_bonded_bundles) {
+struct ofbundle *bundle;
+
+HMAP_FOR_EACH (bundle, hmap_node, &ofproto->bundles) {
+bundle_run(bundle);
+}
 }
 
 stp_run(ofproto);
@@ -1562,7 +1567,6 @@ static void
 wait(struct ofproto *ofproto_)
 {
 struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
-struct ofbundle *bundle;
 
 if (ofproto_get_flow_restore_wait()) {
 return;
@@ -1574,8 +1578,12 @@ wait(struct ofproto *ofproto_)
 if (ofproto->ipfix) {
 dpif_ipfix_wait(ofproto->ipfix);
 }
-HMAP_FOR_EACH (bundle, hmap_node, &ofproto->bundles) {
-bundle_wait(bundle);
+if (ofproto->lacp_enabled || ofproto->has_bonded_bundles) {
+struct ofbundle *bundle;
+
+HMAP_FOR_EACH (bundle, hmap_node, &ofproto->bundles) {
+bundle_wait(bundle);
+}
 }
 if (ofproto->netflow) {
 netflow_wait(ofproto->netflow);
@@ -2477,6 +2485,7 @@ bundle_set(struct ofproto *ofproto_, void *aux,
 
 /* LACP. */
 if (s->lacp) {
+ofproto->lacp_enabled = true;
 if (!bundle->lacp) {
 ofproto->backer->need_revalidate = REV_RECONFIGURE;
 bundle->lacp = lacp_create();
-- 
1.7.9.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCHv4 4/4] bridge: Only store instant_stats on device changes

[Joe Stringer]

Previously, we iterated through all interfaces in instant_stats_run(),
grabbing up-to-date information about device status. After assembling
all of this information for all interfaces, we would determine whether
anything changed and only send an update to ovsdb-server if something
changed.

This patch uses the new global netdev_seq to determine whether there
have been any changes before polling all interfaces, which provides
further CPU usage savings. In a test environment of 5000 internal ports
and 50 tunnel ports with bfd, this reduces CPU usage to around 5%. When
ports change status more often than every 100ms, CPU usage will increase
to previous rates.

Signed-off-by: Joe Stringer 
---
v4: Rebase
---
 vswitchd/bridge.c |   10 ++
 1 file changed, 10 insertions(+)

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 6da5e93..4c35ebb 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -170,6 +170,9 @@ static struct ovsdb_idl_txn *daemonize_txn;
 /* Most recently processed IDL sequence number. */
 static unsigned int idl_seqno;
 
+/* Track changes to port connectivity. */
+static uint64_t connectivity_seqno = LLONG_MIN;
+
 /* Each time this timer expires, the bridge fetches interface and mirror
  * statistics and pushes them into the database. */
 #define IFACE_STATS_INTERVAL (5 * 1000) /* In milliseconds. */
@@ -2254,12 +2257,19 @@ instant_stats_run(void)
 
 if (!instant_txn) {
 struct bridge *br;
+uint64_t seq;
 
 if (time_msec() < instant_next_txn) {
 return;
 }
 instant_next_txn = time_msec() + INSTANT_INTERVAL_MSEC;
 
+seq = connectivity_seq_read();
+if (seq == connectivity_seqno) {
+return;
+}
+connectivity_seqno = seq;
+
 instant_txn = ovsdb_idl_txn_create(idl);
 HMAP_FOR_EACH (br, node, &all_bridges) {
 struct iface *iface;
-- 
1.7.9.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCHv4 1/4] netdev: Globally track port status changes

[Joe Stringer]

Previously, we tracked status changes for ofports on a per-device basis.
Each time in the main thread's loop, we would loop through all ofports
and manually check whether the status has changed for corresponding
devices.

This patch shifts change_seq above the netdevice layer, with one atomic
variable tracking status change for all ports. In the average case where
ports are not constantly going up or down, this allows us to check
change_seq once per loop and not poll any ports. In the worst case,
execution is expected to be similar to how it is currently.

As change_seq is no longer tracked per-device, it doesn't make sense to
cache this status in each ofport struct. As such, we shift this into the
ofproto struct.

In a test environment of 5000 internal ports and 50 tunnel ports with
bfd, this reduces CPU usage from about 45% to about 35%.

Signed-off-by: Joe Stringer 
---
v4: Rebase
---
 lib/automake.mk|2 ++
 lib/connectivity.c |   62 
 lib/connectivity.h |   46 
 lib/netdev-bsd.c   |   36 +
 lib/netdev-dummy.c |   33 +++
 lib/netdev-linux.c |   23 ++--
 lib/netdev-provider.h  |   24 -
 lib/netdev-vport.c |   30 +++--
 lib/netdev.c   |   14 ++
 lib/netdev.h   |2 --
 ofproto/bond.c |7 +++--
 ofproto/ofproto-provider.h |2 +-
 ofproto/ofproto.c  |   53 +
 ofproto/tunnel.c   |7 ++---
 vswitchd/bridge.c  |4 +++
 15 files changed, 175 insertions(+), 170 deletions(-)
 create mode 100644 lib/connectivity.c
 create mode 100644 lib/connectivity.h

diff --git a/lib/automake.mk b/lib/automake.mk
index fadc4be..4d741f0 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -29,6 +29,8 @@ lib_libopenvswitch_a_SOURCES = \
lib/command-line.c \
lib/command-line.h \
lib/compiler.h \
+   lib/connectivity.c \
+   lib/connectivity.h \
lib/coverage.c \
lib/coverage.h \
lib/crc32c.c \
diff --git a/lib/connectivity.c b/lib/connectivity.c
new file mode 100644
index 000..08e5a97
--- /dev/null
+++ b/lib/connectivity.c
@@ -0,0 +1,62 @@
+/*
+ * Copyright (c) 2013 Nicira, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include "connectivity.h"
+#include "ovs-thread.h"
+#include "seq.h"
+
+static struct seq *connectivity_seq;
+
+uint64_t
+connectivity_seq_read(void)
+{
+return seq_read(connectivity_seq);
+}
+
+void
+connectivity_seq_wait(uint64_t seq)
+{
+seq_wait(connectivity_seq, seq);
+}
+
+void
+connectivity_seq_notify(void)
+{
+seq_change(connectivity_seq);
+}
+
+void
+connectivity_seq_init(void)
+{
+static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
+
+if (ovsthread_once_start(&once)) {
+connectivity_seq = seq_create();
+ovsthread_once_done(&once);
+}
+}
+
+void
+connectivity_seq_destroy(void)
+{
+static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
+
+if (ovsthread_once_start(&once)) {
+seq_destroy(connectivity_seq);
+ovsthread_once_done(&once);
+}
+}
diff --git a/lib/connectivity.h b/lib/connectivity.h
new file mode 100644
index 000..c2af6c6
--- /dev/null
+++ b/lib/connectivity.h
@@ -0,0 +1,46 @@
+/*
+ * Copyright (c) 2013 Nicira, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CONNECTIVITY_H
+#define CONNECTIVITY_H 1
+
+#include 
+
+/* Returns a sequence number which indicates changes in connectivity. The
+ * returned sequence number tracks all connectivity changes globally, and will
+ * change whenever a netdev's flags, features, ethernet address or carrier
+ * changes, or whenever a port's bfd, cfm, lacp or stp status changes. It may
+ * change for othe

[ovs-dev] [PATCHv4 0/4] Tunnel Scalability

[Joe Stringer]

Essentially the same patchset as previously posted, three minor changes:-
* First patch merged ("bridge: Refresh STP statistics separately from status")
* Fixed test failures in Patch #2
* Rebase against master

A full description can be found in the previous posting:
https://www.mail-archive.com/dev@openvswitch.org/msg25594.html

Joe Stringer (4):
  netdev: Globally track port status changes
  ofproto-dpif: Don't poll ports when nothing changes
  ofproto-dpif: Only run bundles when lacp or bonds are enabled
  bridge: Only store instant_stats on device changes

 lib/automake.mk|2 ++
 lib/bfd.c  |6 -
 lib/cfm.c  |   13 ++
 lib/connectivity.c |   62 
 lib/connectivity.h |   46 
 lib/lacp.c |7 +
 lib/netdev-bsd.c   |   36 +
 lib/netdev-dummy.c |   33 +++
 lib/netdev-linux.c |   23 ++--
 lib/netdev-provider.h  |   24 -
 lib/netdev-vport.c |   30 +++--
 lib/netdev.c   |   14 ++
 lib/netdev.h   |2 --
 lib/stp.c  |4 +++
 ofproto/bond.c |7 +++--
 ofproto/ofproto-dpif.c |   37 +++---
 ofproto/ofproto-provider.h |2 +-
 ofproto/ofproto.c  |   53 +
 ofproto/tunnel.c   |7 ++---
 tests/test-stp.c   |5 
 vswitchd/bridge.c  |   14 ++
 21 files changed, 247 insertions(+), 180 deletions(-)
 create mode 100644 lib/connectivity.c
 create mode 100644 lib/connectivity.h

-- 
1.7.9.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCHv4 2/4] ofproto-dpif: Don't poll ports when nothing changes

[Joe Stringer]

Currently, as part of ofproto-dpif run() processing, we loop through all
ports and poll corresponding devices for changes in carrier, cfm and bfd
status. This allows us to determine how it may affect bundles. For the
average case where devices are not going up or down constantly, this is
a large amount of unnecessary processing.

This patch gets the bfd, cfm, lacp and stp modules to update the global
netdev change_seq when changes in port/device status occur. We can then
use this global change_seq to check if anything has changed before
looping through the ports in ofproto-dpif. In a test environment of 5000
internal ports and 50 tunnel ports with bfd, this reduces CPU usage from
about 35% to about 25%.

Signed-off-by: Joe Stringer 
---
v4: Ensure that bfd_forwarding__() is called each time through bfd_run().
---
 lib/bfd.c  |6 +-
 lib/cfm.c  |   13 +
 lib/lacp.c |7 +++
 lib/stp.c  |4 
 ofproto/ofproto-dpif.c |   16 +---
 tests/test-stp.c   |5 +
 6 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/lib/bfd.c b/lib/bfd.c
index 1df5acd..2482d37 100644
--- a/lib/bfd.c
+++ b/lib/bfd.c
@@ -21,6 +21,7 @@
 #include 
 
 #include "byte-order.h"
+#include "connectivity.h"
 #include "csum.h"
 #include "dpif.h"
 #include "dynamic-string.h"
@@ -505,8 +506,8 @@ bfd_run(struct bfd *bfd) OVS_EXCLUDED(mutex)
 
 if (bfd->state > STATE_DOWN && now >= bfd->detect_time) {
 bfd_set_state(bfd, STATE_DOWN, DIAG_EXPIRED);
-bfd_forwarding__(bfd);
 }
+bfd_forwarding__(bfd);
 
 /* Decay may only happen when state is STATE_UP, bfd->decay_min_rx is
  * configured, and decay_detect_time is reached. */
@@ -851,6 +852,7 @@ bfd_forwarding__(struct bfd *bfd) OVS_REQUIRES(mutex)
 && bfd->rmt_diag != DIAG_RCPATH_DOWN;
 if (bfd->last_forwarding != last_forwarding) {
 bfd->flap_count++;
+connectivity_seq_notify();
 }
 return bfd->last_forwarding;
 }
@@ -1052,6 +1054,8 @@ bfd_set_state(struct bfd *bfd, enum state state, enum 
diag diag)
 if (bfd->state == STATE_UP && bfd->decay_min_rx) {
 bfd_decay_update(bfd);
 }
+
+connectivity_seq_notify();
 }
 }
 
diff --git a/lib/cfm.c b/lib/cfm.c
index 9c65b34..5d27c6f 100644
--- a/lib/cfm.c
+++ b/lib/cfm.c
@@ -22,6 +22,7 @@
 #include 
 
 #include "byte-order.h"
+#include "connectivity.h"
 #include "dynamic-string.h"
 #include "flow.h"
 #include "hash.h"
@@ -396,6 +397,7 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
 long long int interval = cfm_fault_interval(cfm);
 struct remote_mp *rmp, *rmp_next;
 bool old_cfm_fault = cfm->fault;
+bool old_rmp_opup = cfm->remote_opup;
 bool demand_override;
 bool rmp_set_opup = false;
 bool rmp_set_opdown = false;
@@ -420,6 +422,7 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
 cfm->health = 0;
 } else {
 int exp_ccm_recvd;
+int old_health = cfm->health;
 
 rmp = CONTAINER_OF(hmap_first(&cfm->remote_mps),
struct remote_mp, node);
@@ -434,6 +437,10 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
 cfm->health = MIN(cfm->health, 100);
 rmp->num_health_ccm = 0;
 ovs_assert(cfm->health >= 0 && cfm->health <= 100);
+
+if (cfm->health != old_health) {
+connectivity_seq_notify();
+}
 }
 cfm->health_interval = 0;
 }
@@ -476,6 +483,10 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
 cfm->remote_opup = true;
 }
 
+if (old_rmp_opup != cfm->remote_opup) {
+connectivity_seq_notify();
+}
+
 if (hmap_is_empty(&cfm->remote_mps)) {
 cfm->fault |= CFM_FAULT_RECV;
 }
@@ -497,6 +508,8 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
 if (old_cfm_fault == false || cfm->fault == false) {
 cfm->flap_count++;
 }
+
+connectivity_seq_notify();
 }
 
 cfm->booted = true;
diff --git a/lib/lacp.c b/lib/lacp.c
index fce65b3..bb6e940 100644
--- a/lib/lacp.c
+++ b/lib/lacp.c
@@ -18,6 +18,7 @@
 
 #include 
 
+#include "connectivity.h"
 #include "dynamic-string.h"
 #include "hash.h"
 #include "hmap.h"
@@ -509,11 +510,16 @@ lacp_run(struct lacp *lacp, lacp_send_pdu *send_pdu) 
OVS_EXCLUDED(mutex)
 ovs_mutex_lock(&mutex);
 HMAP_FOR_EACH (slave, node, &lacp->slaves) {
 if (timer_expired(&slave->rx)) {
+enum slave_status old_status = slave->status;
+
 if (slave->status == LACP_CURRENT) {
 slave_set_expired(slave);
 } else if (slave->status == LACP_EXPIRED) {
 slave_set_defaulted(slave);
 }
+if (slave->status != old_sta

Re: [ovs-dev] [PATCH RFC] netdev-linux: Read packet auxdata to obtain vlan_tid

[Ben Pfaff]

On Wed, Dec 11, 2013 at 11:24:14AM +0900, Simon Horman wrote:
> If VLAN acceleration is used when the kernel receives a packet
> then the outer-most VLAN tag will not be present in the packet
> when it is received by netdev-linux. Rather, it will be present
> in auxdata.
> 
> This patch uses recvmsg() instead of recv() to read auxdata for
> each packet and if the vlan_tid is set then it is added to the packet.
> 
> Adding the vlan_tid to the packet involves copying most of the packet
> and may be rather expensive. There is ample scope to avoid this by
> passing the vlan_tid back to the caller separately to the packet itself
> or providing access headroom in the packet. This would most likely
> involve updating the netdev-class API.
> 
> Signed-off-by: Simon Horman 

Thanks for doing this.

I think that we should change netdev_class to pass an ofpbuf into
rx_recv.  Then we can just ofpbuf_reserve() VLAN_HEADER_LEN bytes
before doing the receive, and just use eth_push_vlan() to insert the
VLAN header.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [Windows thread 2]

[Alin Serdean]

Hey,

After the include_next from string.h(wrapper) will be solved we will be faced 
with another scenario.

Unfortunately on MSVC we don't have inline we have __inline(see 
http://msdn.microsoft.com/en-us/library/cx3b23a3.aspx for more information) so 
it is either autoconf magic again :-) or an include that is used by all the 
header files.

In our port I defined the following in config.h (brutal way of doing it ;-) ).
#define ffs __lzcnt
#define inline __inline
#define strtok_r strtok_s
#define __func__ __FUNCTION__
#define u_int8_t uint8_t
#define u_int16_t uint16_t
#define u_int32_t uint32_t
#define u_int64_t uint64_t

Kind Regards,
Alin.

From: dev-boun...@openvswitch.org [dev-boun...@openvswitch.org] on behalf of 
Ben Pfaff [b...@nicira.com]
Sent: Wednesday, December 11, 2013 7:31 PM
To: Simon Horman
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH RFC] netdev-linux: Read packet auxdata to obtain 
vlan_tid

On Wed, Dec 11, 2013 at 11:24:14AM +0900, Simon Horman wrote:
> If VLAN acceleration is used when the kernel receives a packet
> then the outer-most VLAN tag will not be present in the packet
> when it is received by netdev-linux. Rather, it will be present
> in auxdata.
>
> This patch uses recvmsg() instead of recv() to read auxdata for
> each packet and if the vlan_tid is set then it is added to the packet.
>
> Adding the vlan_tid to the packet involves copying most of the packet
> and may be rather expensive. There is ample scope to avoid this by
> passing the vlan_tid back to the caller separately to the packet itself
> or providing access headroom in the packet. This would most likely
> involve updating the netdev-class API.
>
> Signed-off-by: Simon Horman 

Thanks for doing this.

I think that we should change netdev_class to pass an ofpbuf into
rx_recv.  Then we can just ofpbuf_reserve() VLAN_HEADER_LEN bytes
before doing the receive, and just use eth_push_vlan() to insert the
VLAN header.
___
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] [Windows thread 3]

[Alin Serdean]

Hey,


The following is a quick patch for secure pseudorandom number generator on 
windows. I split the functionality with a brutal ifdef macro. Feedback on the 
code and suggestions for a nicer implementation is appreciated :).

diff --git a/lib/entropy.c b/lib/entropy.c
index 02f56e0..ec9d95c 100644
--- a/lib/entropy.c
+++ b/lib/entropy.c
@@ -20,6 +20,9 @@
 #include 
 #include 
 #include 
+#ifdef _WIN32
+#include 
+#endif

 #include "socket-util.h"
 #include "vlog.h"
@@ -33,6 +36,7 @@ static const char urandom[] = "/dev/urandom";
 int
 get_entropy(void *buffer, size_t n)
 {
+#ifndef _WIN32
 size_t bytes_read;
 int error;
 int fd;
@@ -49,6 +53,20 @@ get_entropy(void *buffer, size_t n)
 if (error) {
 VLOG_ERR("%s: read error (%s)", urandom, ovs_retval_to_string(error));
 }
+#else
+ int error = 1;
+ HCRYPTPROV   crypt_prov = 0;
+ CryptAcquireContext(&crypt_prov, NULL, NULL, PROV_RSA_FULL, 0);
+
+ if (CryptGenRandom(crypt_prov, n, buffer)) {
+ error = 0;
+ }
+
+ if (error) {
+ VLOG_ERR("CryptGenRandom: read error (%s)", urandom, 
ovs_retval_to_string(error));
+ }
+ CryptReleaseContext(crypt_prov, 0);
+#endif
 return error;
 }


Kind Regards,

Alin.

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [Windows thread 3]

[Ben Pfaff]

On Wed, Dec 11, 2013 at 06:02:58PM +, Alin Serdean wrote:
> The following is a quick patch for secure pseudorandom number
> generator on windows. I split the functionality with a brutal ifdef
> macro. Feedback on the code and suggestions for a nicer
> implementation is appreciated :).

I'm fine with an #ifdef in this case.

The VLOG_ERR message "CryptGenRandom: read error (/dev/urandom)"
doesn't make any sense.  If you'll fix that, and supply some
reasonable errno value instead of just 1, then I'll apply this.

Thanks,

Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [port-renumbering 6/8] lacp: Make lacp negotiation test hard-code aggregation keys.

[Jarno Rajahalme]

I bumped to this yesterday and had the exact same patch ready to be sent, so:

Acked-by: Jarno Rajahalme 


On Dec 10, 2013, at 11:20 PM, Ben Pfaff  wrote:

> The lacp implementation takes the aggregation key from the key or portid
> of the first slave to be added to the lacp object.  When multiple slaves
> are added initially to a lacp object (the most common case), which is the
> "first" is arbitrary.  Until now, it seems that the "first" was actually
> predictable enough for the tests to (unknowingly) rely on it, but an
> upcoming commit will break that.  This commit fixes the test by forcing
> a particular aggregation key for each lacp object.
> 
> Signed-off-by: Ben Pfaff 
> ---
> tests/lacp.at |   12 
> 1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/lacp.at b/tests/lacp.at
> index ffc8996..ede7c8f 100644
> --- a/tests/lacp.at
> +++ b/tests/lacp.at
> @@ -126,8 +126,10 @@ OVS_VSWITCHD_START(
>   [add-bond br0 bond0 p0 p1 bond_mode=balance-tcp lacp=active \
> other-config:lacp-time=fast \
> other-config:bond-rebalance-interval=0 -- \
> -   set interface p0 type=patch options:peer=p2 ofport_request=1 -- \
> -   set interface p1 type=patch options:peer=p3 ofport_request=2 -- \
> +   set interface p0 type=patch options:peer=p2 ofport_request=1 \
> +other-config:lacp-aggregation-key=2 -- \
> +   set interface p1 type=patch options:peer=p3 ofport_request=2 \
> +other-config:lacp-aggregation-key=2 -- \
>add-br br1 -- \
>set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
>set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \
> @@ -135,8 +137,10 @@ OVS_VSWITCHD_START(
>add-bond br1 bond1 p2 p3 bond_mode=balance-tcp lacp=active \
> other-config:lacp-time=fast \
> other-config:bond-rebalance-interval=0 -- \
> -   set interface p2 type=patch options:peer=p0 ofport_request=3 -- \
> -   set interface p3 type=patch options:peer=p1 ofport_request=4 --])
> +   set interface p2 type=patch options:peer=p0 ofport_request=3 \
> +other-config:lacp-aggregation-key=4 -- \
> +   set interface p3 type=patch options:peer=p1 ofport_request=4 \
> +other-config:lacp-aggregation-key=4 --])
> 
> AT_CHECK([ovs-appctl netdev-dummy/set-admin-state up], 0, [OK
> ])
> -- 
> 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] coverage: Fix build when linker sections not supported.

[Gurucharan Shetty]

On Sat, Dec 7, 2013 at 8:17 AM, Ben Pfaff  wrote:
> With this change, OVS builds and runs fine without linker section support.
>
> CC: Linda Sun 
> CC: Saurabh Shah 
> Signed-off-by: Ben Pfaff 
Looks good to me.
> ---
>  lib/coverage.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/coverage.c b/lib/coverage.c
> index aae9937..c7a0028 100644
> --- a/lib/coverage.c
> +++ b/lib/coverage.c
> @@ -48,7 +48,7 @@ extern struct coverage_counter *__stop_coverage[];
>  }   \
>  extern struct coverage_counter counter_##COUNTER;   \
>  struct coverage_counter counter_##COUNTER   \
> -= { #COUNTER, COUNTER##_count, 0 };
> += { #COUNTER, COUNTER##_count, 0, 0, {0}, {0} };
>  #include "coverage.def"
>  #undef COVERAGE_COUNTER
>
> --
> 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] [port-renumbering 1/8] ofproto-dpif: Improve comment.

[Jarno Rajahalme]

Acked-by: Jarno Rajahalme 

On Dec 10, 2013, at 11:19 PM, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
> ---
> ofproto/ofproto-dpif.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index ff77903..bfa9a9a 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -436,7 +436,7 @@ struct dpif_backer {
> struct timer next_expiration;
> 
> struct ovs_rwlock odp_to_ofport_lock;
> -struct hmap odp_to_ofport_map OVS_GUARDED; /* ODP port to ofport map. */
> +struct hmap odp_to_ofport_map OVS_GUARDED; /* Contains "struct ofport"s. 
> */
> 
> struct simap tnl_backers;  /* Set of dpif ports backing tunnels. */
> 
> -- 
> 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] [port-renumbering 2/8] ofproto-provider: Add comment on struct ofport.

[Jarno Rajahalme]

Acked-by: Jarno Rajahalme 

On Dec 10, 2013, at 11:19 PM, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
> ---
> ofproto/ofproto-provider.h |2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 54d97f1..600b92b 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -162,6 +162,8 @@ struct ofport *ofproto_get_port(const struct ofproto *, 
> ofp_port_t ofp_port);
> 
> /* An OpenFlow port within a "struct ofproto".
>  *
> + * The port's name is netdev_get_name(port->netdev).
> + *
>  * With few exceptions, ofproto implementations may look at these fields but
>  * should not modify them. */
> struct ofport {
> -- 
> 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] [port-renumbering 3/8] ofproto: Make ofproto_enumerate_types() match its comment.

[Jarno Rajahalme]

Acked-by: Jarno Rajahalme 

On Dec 10, 2013, at 11:19 PM, Ben Pfaff  wrote:

> The comment says that it clears the passed-in sset, but it didn't.  The bug
> didn't actually affect any of the existing callers, which all passed in an
> empty sset.
> 
> Signed-off-by: Ben Pfaff 
> ---
> ofproto/ofproto.c |1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 3a60328..1a1113f 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -432,6 +432,7 @@ ofproto_enumerate_types(struct sset *types)
> {
> size_t i;
> 
> +sset_clear(types);
> for (i = 0; i < n_ofproto_classes; i++) {
> ofproto_classes[i]->enumerate_types(types);
> }
> -- 
> 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] [port-renumbering 4/8] ofproto: Fix alphabetical order in list of structs.

[Jarno Rajahalme]

Acked-by: Jarno Rajahalme 

On Dec 10, 2013, at 11:20 PM, Ben Pfaff  wrote:

> Signed-off-by: Ben Pfaff 
> ---
> ofproto/ofproto.h |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index d734eab..6197278 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -36,12 +36,12 @@ struct bfd_cfg;
> struct cfm_settings;
> struct cls_rule;
> struct netdev;
> -struct ofproto;
> +struct netdev_stats;
> struct ofport;
> +struct ofproto;
> struct shash;
> struct simap;
> struct smap;
> -struct netdev_stats;
> 
> struct ofproto_controller_info {
> bool is_connected;
> -- 
> 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] [port-renumbering 5/8] netdev: Log a warning when netdev_set_config() fails.

[Jarno Rajahalme]

On Dec 10, 2013, at 11:20 PM, Ben Pfaff  wrote:

> This allows its callers to avoid duplicating the code.
> 
> Signed-off-by: Ben Pfaff 
> ---
> lib/netdev.c  |   10 --
> utilities/ovs-dpctl.c |2 --
> vswitchd/bridge.c |9 +
> 3 files changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 5ed6062..159ca51 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -386,8 +386,14 @@ netdev_set_config(struct netdev *netdev, const struct 
> smap *args)
> {
> if (netdev->netdev_class->set_config) {
> const struct smap no_args = SMAP_INITIALIZER(&no_args);
> -return netdev->netdev_class->set_config(netdev,
> -args ? args : &no_args);
> +int error;
> +
> +error = netdev->netdev_class->set_config(netdev,
> + args ? args : &no_args);
> +if (error) {
> +VLOG_WARN("%s: could not set configuration (%s)",
> +  netdev_get_name(netdev), ovs_strerror(error));
> +}

Wouldn’t you still want to return the error? The callers seem to expect it.

Otherwise:

Acked-by: Jarno Rajahalme 

> } else if (args && !smap_is_empty(args)) {
> VLOG_WARN("%s: arguments provided to device that is not configurable",
>   netdev_get_name(netdev));
> diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
> index 29f1151..bd99834 100644
> --- a/utilities/ovs-dpctl.c
> +++ b/utilities/ovs-dpctl.c
> @@ -350,7 +350,6 @@ dpctl_add_if(int argc OVS_UNUSED, char *argv[])
> 
> error = netdev_set_config(netdev, &args);
> if (error) {
> -ovs_error(error, "%s: failed to configure network device", name);
> goto next;
> }
> 
> @@ -459,7 +458,6 @@ dpctl_set_if(int argc, char *argv[])
> error = netdev_set_config(netdev, &args);
> smap_destroy(&args);
> if (error) {
> -ovs_error(error, "%s: failed to configure network device", name);
> goto next;
> }
> 
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 2b11c5b..0b0e4d7 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -1343,14 +1343,7 @@ static int
> iface_set_netdev_config(const struct ovsrec_interface *iface_cfg,
> struct netdev *netdev)
> {
> -int error;
> -
> -error = netdev_set_config(netdev, &iface_cfg->options);
> -if (error) {
> -VLOG_WARN("could not configure network device %s (%s)",
> -  iface_cfg->name, ovs_strerror(error));
> -}
> -return error;
> +return netdev_set_config(netdev, &iface_cfg->options);
> }
> 
> /* This function determines whether 'ofproto_port', which is attached to
> -- 
> 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] [Windows thread 2]

[Gurucharan Shetty]

On Wed, Dec 11, 2013 at 9:43 AM, Alin Serdean
 wrote:
> Hey,
>
> After the include_next from string.h(wrapper) will be solved we will be faced 
> with another scenario.
>
> Unfortunately on MSVC we don't have inline we have __inline(see 
> http://msdn.microsoft.com/en-us/library/cx3b23a3.aspx for more information) 
> so it is either autoconf magic again :-) or an include that is used by all 
> the header files.
>
> In our port I defined the following in config.h (brutal way of doing it ;-) ).
> #define ffs __lzcnt
> #define inline __inline
> #define strtok_r strtok_s
> #define __func__ __FUNCTION__
> #define u_int8_t uint8_t
> #define u_int16_t uint16_t
> #define u_int32_t uint32_t
> #define u_int64_t uint64_t

I had sent a patch couple of weeks back to solve a similar issue. (I
abandoned the patch for a different reason.)
http://openvswitch.org/pipermail/dev/2013-November/034001.html

I wonder for this particular case, we can include the #defines inside
the windefs.h

Thanks,
Guru

>
> Kind Regards,
> Alin.
> 
> From: dev-boun...@openvswitch.org [dev-boun...@openvswitch.org] on behalf of 
> Ben Pfaff [b...@nicira.com]
> Sent: Wednesday, December 11, 2013 7:31 PM
> To: Simon Horman
> Cc: dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH RFC] netdev-linux: Read packet auxdata to 
> obtain vlan_tid
>
> On Wed, Dec 11, 2013 at 11:24:14AM +0900, Simon Horman wrote:
>> If VLAN acceleration is used when the kernel receives a packet
>> then the outer-most VLAN tag will not be present in the packet
>> when it is received by netdev-linux. Rather, it will be present
>> in auxdata.
>>
>> This patch uses recvmsg() instead of recv() to read auxdata for
>> each packet and if the vlan_tid is set then it is added to the packet.
>>
>> Adding the vlan_tid to the packet involves copying most of the packet
>> and may be rather expensive. There is ample scope to avoid this by
>> passing the vlan_tid back to the caller separately to the packet itself
>> or providing access headroom in the packet. This would most likely
>> involve updating the netdev-class API.
>>
>> Signed-off-by: Simon Horman 
>
> Thanks for doing this.
>
> I think that we should change netdev_class to pass an ofpbuf into
> rx_recv.  Then we can just ofpbuf_reserve() VLAN_HEADER_LEN bytes
> before doing the receive, and just use eth_push_vlan() to insert the
> VLAN header.
> ___
> 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

Re: [ovs-dev] [PATCH v6 1/2] dpif-netdev: Properly create exact match masks.

[Ben Pfaff]

On Mon, Dec 09, 2013 at 06:35:57PM -0800, Jarno Rajahalme wrote:
> Normally OVS userspace supplies a mask along with a flow key for each
> new data path flow that should be created.  OVS also provides an
> option to disable the kernel wildcarding, in which case the flows are
> created without a mask.  When kernel wildcarding is disabled, the
> datapath should use exact match, i.e. not wildcard any bits in the
> flow key.  Currently, what happens with the userspace datapath instead
> is that a datapath flow with mostly empty mask is created (i.e., most
> fields are wildcarded), as the current code does not examine the given
> mask key length to find out that the mask key is actually empty.  This
> results in the same datapath flow matching on packets of multiple
> different flows, wrong actions being processed, and stats being
> incorrect.
> 
> This patch refactors userspace datapath code to explicitly initialize
> a suitable exact match mask when a flow put without a mask is
> executed.
> 
> Signed-off-by: Jarno Rajahalme 
> 
> v6: - Unwildcard all fields whose prerequisities are met, add test case
>   to prevent future regressions.
> 
> v5: - Split dpif_netdev_flow_mask_from_nlattrs() to
>   dpif_netdev_flow_from_nlattrs() and dpif_netdev_mask_from_nlattrs()
>   and make the latter to require non-zero mask_key_len.
> - Make dp_netdev_flow_add() to initialize an exact match mask if no
>   wildcards are given.  This reflects the kernel datapath behavior.

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v6 2/2] Classifier: Track address prefixes.

[Ben Pfaff]

On Mon, Dec 09, 2013 at 06:35:58PM -0800, Jarno Rajahalme wrote:
> Add a prefix tree (trie) structure for tracking the used address
> space, enabling skipping classifier tables containing longer masks
> than necessary for an address field value in a packet header being
> classified.  This enables less unwildcarding for datapath flows in
> parts of the address space without host routes.
> 
> Trie lookup is interwoven to the staged lookup, so that a trie is
> searched only when the configured trie field becomes relevant
> for the lookup.  The trie lookup results are retained so that each
> trie is checked at most once for each classifier lookup.
> 
> This implementation tracks the number of rules at each address prefix
> for the whole classifier.  More aggressive table skipping would be
> possible by maintaining lists of tables that have prefixes at the
> lengths encountered on tree traversal, or by maintaining separate
> tries for subsets of rules separated by metadata fields.
> 
> Prefix tracking is configured via OVSDB.  A new column "prefixes" is
> added to the database table "Flow_Table".  "prefixes" is a set of
> string values listing the field names for which prefix lookup should
> be used.
> 
> As of now, the fields for which prefix lookup can be enabled are:
> - tun_id, tun_src, tun_dst
> - nw_src, nw_dst (or aliases ip_src and ip_dst)
> - ipv6_src, ipv6_dst
> 
> There is a maximum number of fields that can be enabled for any one
> flow table.  Currently this limit is 3.
> 
> Examples:
> 
> ovs-vsctl set Bridge br0 flow_tables:0=@N1 -- \
>  --id=@N1 create Flow_Table name=table0
> ovs-vsctl set Bridge br0 flow_tables:1=@N1 -- \
>  --id=@N1 create Flow_Table name=table1
> 
> ovs-vsctl set Flow_Table table0 prefixes=ip_dst,ip_src
> ovs-vsctl set Flow_Table table1 prefixes=[]
> 
> Signed-off-by: Jarno Rajahalme 
> 
> v6: Address Ben's review comments.
> 
> v5:
> - Refactoring and cleaning up based on Ben's comments.
> - Ben's updated commentary on lib/classifier.h.
> - Explain the prefix offset numbering used in comments.
> - Get more of a prefix from the next word if necessary.
> - Refactor to eliminate duplicated code.
> - trie_remove(): Also prune parent nodes if possible.
> - schema: Changed "fieldspec" smap to "prefixes" set.
> - removed support for metadata field, as it has no datapath equivalent.
> - Added NEWS.
> 
> v4:
> 
> - Remove easily perishable comments from classifier.h
> - Make mf_field.flow_u32ofs -1 for registers, making registers not
>   supported as prefix lookup fields.  Registers are held in host byte
>   order, whereas current implementation only works for network byte
>   order.
> - Use 'u32ofs' instead of 'be32ofs' to make the above more clear.
> - Check for prefix lookup field compatibility at
>   classifier_set_prefix_fields() and remove redundant checks from
>   minimask_get_prefix_len(), trie_insert(), and trie_remove().
> - Simplify minimask_get_prefix_len(), separate out
>   minimatch_get_prefix().
> - Make trie_insert() and trie_remove() take CIDR mask length as a
>   parameter instead of finding the mask length again.  Call
>   trie_insert() or trie_remove() only with a non-zero (and positive)
>   prefix length.
> - Make trie_init() destroy existing trie (if any) and accept NULL
>   field, allowing classifier_set_prefix_fields() to be simplified.
> - Detect duplicate prefix fields on set-up.
> - Add "fieldspec" documentation to vswitch.xml
> - Add IPv6 trie testing, verifying that crossing 32-bit boundaries in
>   masks works.

Acked-by: Ben Pfaff 

Here are some changes you may consider, but none of them is more than
cosmetic.

diff --git a/lib/classifier.c b/lib/classifier.c
index e9a13d0..8b5c36a 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -198,9 +198,7 @@ classifier_destroy(struct classifier *cls)
 int i;
 
 for (i = 0; i < cls->n_tries; i++) {
-if (cls->tries[i].root) {
-trie_destroy(cls->tries[i].root);
-}
+trie_destroy(cls->tries[i].root);
 }
 
 HMAP_FOR_EACH_SAFE (subtable, next_subtable, hmap_node,
@@ -235,7 +233,7 @@ classifier_set_prefix_fields(struct classifier *cls,
 const struct mf_field *field = mf_from_id(trie_fields[i]);
 if (field->flow_be32ofs < 0 || field->n_bits % 32) {
 /* Incompatible field.  This is the only place where we
- * Enforce these requirements, but the rest of the trie code
+ * enforce these requirements, but the rest of the trie code
  * depends on the flow_be32ofs to be non-negative and the
  * field length to be a multiple of 32 bits. */
 continue;
@@ -268,7 +266,7 @@ trie_init(struct classifier *cls, int trie_idx,
 struct cls_trie *trie = &cls->tries[trie_idx];
 struct cls_subtable *subtable;
 
-if (trie_idx < cls->n_tries && trie->root) {
+if (trie_idx < cls->n_tries) {
 trie_destroy(trie->root);
 }
 trie->root = NULL;

Re: [ovs-dev] [PATCH] coverage: Fix build when linker sections not supported.

[Ben Pfaff]

On Wed, Dec 11, 2013 at 10:17:40AM -0800, Gurucharan Shetty wrote:
> On Sat, Dec 7, 2013 at 8:17 AM, Ben Pfaff  wrote:
> > With this change, OVS builds and runs fine without linker section support.
> >
> > CC: Linda Sun 
> > CC: Saurabh Shah 
> > Signed-off-by: Ben Pfaff 
> Looks good to me.

Applied, thanks.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCHv4 1/4] netdev: Globally track port status changes

[Ethan Jackson]

Couple of comments.

Do we need a whole new module for the connectivity seq stuff?
Couldn't we just do with an extern global struct seq *?  That said,
it's not exactly clear where we'd put it, so perhaps a separate module
is the way to go.  I don't feel strongly about it.

Along those same lines, if we need a whole separate module, could all
this be implemented with a single function:

struct seq *connectivity_seq_get().

This function would create the seq if it doesn't exist, and return it.
 With this approach we don't have to deal with all the pass through
boilerplate logic.

Could you move the comments for the connectivity seq stuff from the
header file into the C file?  It's not entirely clear to me why that's
our convention, but the rest of the code does it that way, so may as
well here to.

Ethan

On Wed, Dec 11, 2013 at 9:10 AM, Joe Stringer  wrote:
> Previously, we tracked status changes for ofports on a per-device basis.
> Each time in the main thread's loop, we would loop through all ofports
> and manually check whether the status has changed for corresponding
> devices.
>
> This patch shifts change_seq above the netdevice layer, with one atomic
> variable tracking status change for all ports. In the average case where
> ports are not constantly going up or down, this allows us to check
> change_seq once per loop and not poll any ports. In the worst case,
> execution is expected to be similar to how it is currently.
>
> As change_seq is no longer tracked per-device, it doesn't make sense to
> cache this status in each ofport struct. As such, we shift this into the
> ofproto struct.
>
> In a test environment of 5000 internal ports and 50 tunnel ports with
> bfd, this reduces CPU usage from about 45% to about 35%.
>
> Signed-off-by: Joe Stringer 
> ---
> v4: Rebase
> ---
>  lib/automake.mk|2 ++
>  lib/connectivity.c |   62 
> 
>  lib/connectivity.h |   46 
>  lib/netdev-bsd.c   |   36 +
>  lib/netdev-dummy.c |   33 +++
>  lib/netdev-linux.c |   23 ++--
>  lib/netdev-provider.h  |   24 -
>  lib/netdev-vport.c |   30 +++--
>  lib/netdev.c   |   14 ++
>  lib/netdev.h   |2 --
>  ofproto/bond.c |7 +++--
>  ofproto/ofproto-provider.h |2 +-
>  ofproto/ofproto.c  |   53 +
>  ofproto/tunnel.c   |7 ++---
>  vswitchd/bridge.c  |4 +++
>  15 files changed, 175 insertions(+), 170 deletions(-)
>  create mode 100644 lib/connectivity.c
>  create mode 100644 lib/connectivity.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index fadc4be..4d741f0 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -29,6 +29,8 @@ lib_libopenvswitch_a_SOURCES = \
> lib/command-line.c \
> lib/command-line.h \
> lib/compiler.h \
> +   lib/connectivity.c \
> +   lib/connectivity.h \
> lib/coverage.c \
> lib/coverage.h \
> lib/crc32c.c \
> diff --git a/lib/connectivity.c b/lib/connectivity.c
> new file mode 100644
> index 000..08e5a97
> --- /dev/null
> +++ b/lib/connectivity.c
> @@ -0,0 +1,62 @@
> +/*
> + * Copyright (c) 2013 Nicira, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include 
> +#include "connectivity.h"
> +#include "ovs-thread.h"
> +#include "seq.h"
> +
> +static struct seq *connectivity_seq;
> +
> +uint64_t
> +connectivity_seq_read(void)
> +{
> +return seq_read(connectivity_seq);
> +}
> +
> +void
> +connectivity_seq_wait(uint64_t seq)
> +{
> +seq_wait(connectivity_seq, seq);
> +}
> +
> +void
> +connectivity_seq_notify(void)
> +{
> +seq_change(connectivity_seq);
> +}
> +
> +void
> +connectivity_seq_init(void)
> +{
> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +
> +if (ovsthread_once_start(&once)) {
> +connectivity_seq = seq_create();
> +ovsthread_once_done(&once);
> +}
> +}
> +
> +void
> +connectivity_seq_destroy(void)
> +{
> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +
> +if (ovsthread_once_start(&once)) {
> +seq_destroy(connectivity_seq);
> +ovsthread_once_done(&once);
> +}
> +}
> diff --git a/lib/connectivity.h b/lib/connectivity.h

Re: [ovs-dev] [port-renumbering 5/8] netdev: Log a warning when netdev_set_config() fails.

[Ben Pfaff]

On Wed, Dec 11, 2013 at 10:24:50AM -0800, Jarno Rajahalme wrote:
> 
> On Dec 10, 2013, at 11:20 PM, Ben Pfaff  wrote:
> 
> > This allows its callers to avoid duplicating the code.
> > 
> > Signed-off-by: Ben Pfaff 
> > ---
> > lib/netdev.c  |   10 --
> > utilities/ovs-dpctl.c |2 --
> > vswitchd/bridge.c |9 +
> > 3 files changed, 9 insertions(+), 12 deletions(-)
> > 
> > diff --git a/lib/netdev.c b/lib/netdev.c
> > index 5ed6062..159ca51 100644
> > --- a/lib/netdev.c
> > +++ b/lib/netdev.c
> > @@ -386,8 +386,14 @@ netdev_set_config(struct netdev *netdev, const struct 
> > smap *args)
> > {
> > if (netdev->netdev_class->set_config) {
> > const struct smap no_args = SMAP_INITIALIZER(&no_args);
> > -return netdev->netdev_class->set_config(netdev,
> > -args ? args : &no_args);
> > +int error;
> > +
> > +error = netdev->netdev_class->set_config(netdev,
> > + args ? args : &no_args);
> > +if (error) {
> > +VLOG_WARN("%s: could not set configuration (%s)",
> > +  netdev_get_name(netdev), ovs_strerror(error));
> > +}
> 
> Wouldn?t you still want to return the error? The callers seem to expect it.

Yes, good point, sorry.  I added a "return error;" here.

> Otherwise:
> 
> Acked-by: Jarno Rajahalme 

Thanks!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] datapath: Fix sparse warning.

[Jarno Rajahalme]

Make the new compat function skb_flow_get_ports() static to silence a
sparse warning.

Signed-off-by: Jarno Rajahalme 
---
 datapath/linux/compat/flow_dissector.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/datapath/linux/compat/flow_dissector.c 
b/datapath/linux/compat/flow_dissector.c
index 7a0d09b..a8a2e52 100644
--- a/datapath/linux/compat/flow_dissector.c
+++ b/datapath/linux/compat/flow_dissector.c
@@ -46,7 +46,7 @@ static void iph_to_flow_copy_addrs(struct flow_keys *flow, 
const struct iphdr *i
memcpy(&flow->src, &iph->saddr, sizeof(flow->src) + sizeof(flow->dst));
 }
 
-__be32 skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto)
+static __be32 skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 
ip_proto)
 {
int poff = proto_ports_offset(ip_proto);
 
-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] lib/util: Only define count_1bits_8 when needed.

[Jarno Rajahalme]

util.h declares this when needed, make sure the definition is compiled
in only in that case.

Signed-off-by: Jarno Rajahalme 
---
 lib/util.c |2 +-
 lib/util.h |   18 --
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/lib/util.c b/lib/util.c
index 13d41a7..000504c 100644
--- a/lib/util.c
+++ b/lib/util.c
@@ -901,7 +901,7 @@ raw_clz64(uint64_t n)
 }
 #endif
 
-#if !(__GNUC__ >= 4 && defined(__corei7))
+#if NEED_COUNT_1BITS_8
 #define INIT1(X)\
 X) & (1 << 0)) != 0) +  \
  (((X) & (1 << 1)) != 0) +  \
diff --git a/lib/util.h b/lib/util.h
index 8d810c2..264cdca 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -371,8 +371,7 @@ log_2_ceil(uint64_t n)
 return log_2_floor(n) + !is_pow2(n);
 }
 
-extern const uint8_t count_1bits_8[256];
-
+#if INTPTR_MAX == INT32_MAX
 /* Returns the number of 1-bits in 'x', between 0 and 32 inclusive. */
 static inline unsigned int
 count_1bits_32(uint32_t x)
@@ -381,6 +380,9 @@ count_1bits_32(uint32_t x)
 /* __builtin_popcount() is fast only when supported by the CPU. */
 return __builtin_popcount(x);
 #else
+#define NEED_COUNT_1BITS_8 1
+extern const uint8_t count_1bits_8[256];
+
 /* This portable implementation is the fastest one we know of for 32 bits,
  * and faster than GCC __builtin_popcount(). */
 return (count_1bits_8[x & 0xff] +
@@ -394,7 +396,13 @@ count_1bits_32(uint32_t x)
 static inline unsigned int
 count_1bits(uint64_t x)
 {
-if (sizeof(void *) == 8) { /* 64-bit CPU */
+return count_1bits_32(x) + count_1bits_32(x >> 32);
+}
+#else
+/* Returns the number of 1-bits in 'x', between 0 and 64 inclusive. */
+static inline unsigned int
+count_1bits(uint64_t x)
+{
 #if __GNUC__ >= 4 && defined(__corei7)
 /* __builtin_popcountll() is fast only when supported by the CPU. */
 return __builtin_popcountll(x);
@@ -410,10 +418,8 @@ count_1bits(uint64_t x)
 x = (x + (x >> 4)) & h0F;  /* Count of each 8 bits in-place. */
 return (x * h01) >> 56;/* Sum of all bytes. */
 #endif
-} else { /* 32-bit CPU */
-return count_1bits_32(x) + count_1bits_32(x >> 32);
-}
 }
+#endif
 
 /* Returns the rightmost 1-bit in 'x' (e.g. 01011000 => 1000), or 0 if 'x'
  * is 0. */
-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath: Fix sparse warning.

[Pravin Shelar]

On Wed, Dec 11, 2013 at 10:51 AM, Jarno Rajahalme  wrote:
> Make the new compat function skb_flow_get_ports() static to silence a
> sparse warning.
>
> Signed-off-by: Jarno Rajahalme 
> ---
>  datapath/linux/compat/flow_dissector.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/datapath/linux/compat/flow_dissector.c 
> b/datapath/linux/compat/flow_dissector.c
> index 7a0d09b..a8a2e52 100644
> --- a/datapath/linux/compat/flow_dissector.c
> +++ b/datapath/linux/compat/flow_dissector.c
> @@ -46,7 +46,7 @@ static void iph_to_flow_copy_addrs(struct flow_keys *flow, 
> const struct iphdr *i
> memcpy(&flow->src, &iph->saddr, sizeof(flow->src) + 
> sizeof(flow->dst));
>  }
>
> -__be32 skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto)
> +static __be32 skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 
> ip_proto)
>  {
> int poff = proto_ports_offset(ip_proto);
>
> --

Looks good.

Acked-by: Pravin B Shelar 

> 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] [PATCHv4 1/4] netdev: Globally track port status changes

[Joe Stringer]

On 11 December 2013 10:44, Ethan Jackson  wrote:
> Couple of comments.
>
> Do we need a whole new module for the connectivity seq stuff?
> Couldn't we just do with an extern global struct seq *?  That said,
> it's not exactly clear where we'd put it, so perhaps a separate module
> is the way to go.  I don't feel strongly about it.
>
> Along those same lines, if we need a whole separate module, could all
> this be implemented with a single function:
>
> struct seq *connectivity_seq_get().
>
> This function would create the seq if it doesn't exist, and return it.
>  With this approach we don't have to deal with all the pass through
> boilerplate logic.

This seems sensible. I had it in mind that if we wish to split up this
single seq for each protocol later, we may wish to collect them in a
separate module like this. However I don't think that's a strong
reason to leave it where it is. One alternative is to place it in
seq.[ch]. What do you think?

> Could you move the comments for the connectivity seq stuff from the
> header file into the C file?  It's not entirely clear to me why that's
> our convention, but the rest of the code does it that way, so may as
> well here to.

Sure, I'll do that.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v6 1/2] dpif-netdev: Properly create exact match masks.

[Jarno Rajahalme]

Pushed to master,

  Jarno

On Dec 11, 2013, at 10:36 AM, Ben Pfaff  wrote:

> On Mon, Dec 09, 2013 at 06:35:57PM -0800, Jarno Rajahalme wrote:
>> Normally OVS userspace supplies a mask along with a flow key for each
>> new data path flow that should be created.  OVS also provides an
>> option to disable the kernel wildcarding, in which case the flows are
>> created without a mask.  When kernel wildcarding is disabled, the
>> datapath should use exact match, i.e. not wildcard any bits in the
>> flow key.  Currently, what happens with the userspace datapath instead
>> is that a datapath flow with mostly empty mask is created (i.e., most
>> fields are wildcarded), as the current code does not examine the given
>> mask key length to find out that the mask key is actually empty.  This
>> results in the same datapath flow matching on packets of multiple
>> different flows, wrong actions being processed, and stats being
>> incorrect.
>> 
>> This patch refactors userspace datapath code to explicitly initialize
>> a suitable exact match mask when a flow put without a mask is
>> executed.
>> 
>> Signed-off-by: Jarno Rajahalme 
>> 
>> v6: - Unwildcard all fields whose prerequisities are met, add test case
>>  to prevent future regressions.
>> 
>> v5: - Split dpif_netdev_flow_mask_from_nlattrs() to
>>  dpif_netdev_flow_from_nlattrs() and dpif_netdev_mask_from_nlattrs()
>>  and make the latter to require non-zero mask_key_len.
>>- Make dp_netdev_flow_add() to initialize an exact match mask if no
>>  wildcards are given.  This reflects the kernel datapath behavior.
> 
> Acked-by: Ben Pfaff 

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v6 2/2] Classifier: Track address prefixes.

[Jarno Rajahalme]

Thanks for the review Ben!

Pushed to master with the changes proposed,

  Jarno

On Dec 11, 2013, at 10:38 AM, Ben Pfaff  wrote:

> On Mon, Dec 09, 2013 at 06:35:58PM -0800, Jarno Rajahalme wrote:
>> Add a prefix tree (trie) structure for tracking the used address
>> space, enabling skipping classifier tables containing longer masks
>> than necessary for an address field value in a packet header being
>> classified.  This enables less unwildcarding for datapath flows in
>> parts of the address space without host routes.
>> 
>> Trie lookup is interwoven to the staged lookup, so that a trie is
>> searched only when the configured trie field becomes relevant
>> for the lookup.  The trie lookup results are retained so that each
>> trie is checked at most once for each classifier lookup.
>> 
>> This implementation tracks the number of rules at each address prefix
>> for the whole classifier.  More aggressive table skipping would be
>> possible by maintaining lists of tables that have prefixes at the
>> lengths encountered on tree traversal, or by maintaining separate
>> tries for subsets of rules separated by metadata fields.
>> 
>> Prefix tracking is configured via OVSDB.  A new column "prefixes" is
>> added to the database table "Flow_Table".  "prefixes" is a set of
>> string values listing the field names for which prefix lookup should
>> be used.
>> 
>> As of now, the fields for which prefix lookup can be enabled are:
>> - tun_id, tun_src, tun_dst
>> - nw_src, nw_dst (or aliases ip_src and ip_dst)
>> - ipv6_src, ipv6_dst
>> 
>> There is a maximum number of fields that can be enabled for any one
>> flow table.  Currently this limit is 3.
>> 
>> Examples:
>> 
>> ovs-vsctl set Bridge br0 flow_tables:0=@N1 -- \
>> --id=@N1 create Flow_Table name=table0
>> ovs-vsctl set Bridge br0 flow_tables:1=@N1 -- \
>> --id=@N1 create Flow_Table name=table1
>> 
>> ovs-vsctl set Flow_Table table0 prefixes=ip_dst,ip_src
>> ovs-vsctl set Flow_Table table1 prefixes=[]
>> 
>> Signed-off-by: Jarno Rajahalme 
>> 
>> v6: Address Ben's review comments.
>> 
>> v5:
>> - Refactoring and cleaning up based on Ben's comments.
>> - Ben's updated commentary on lib/classifier.h.
>> - Explain the prefix offset numbering used in comments.
>> - Get more of a prefix from the next word if necessary.
>> - Refactor to eliminate duplicated code.
>> - trie_remove(): Also prune parent nodes if possible.
>> - schema: Changed "fieldspec" smap to "prefixes" set.
>> - removed support for metadata field, as it has no datapath equivalent.
>> - Added NEWS.
>> 
>> v4:
>> 
>> - Remove easily perishable comments from classifier.h
>> - Make mf_field.flow_u32ofs -1 for registers, making registers not
>>  supported as prefix lookup fields.  Registers are held in host byte
>>  order, whereas current implementation only works for network byte
>>  order.
>> - Use 'u32ofs' instead of 'be32ofs' to make the above more clear.
>> - Check for prefix lookup field compatibility at
>>  classifier_set_prefix_fields() and remove redundant checks from
>>  minimask_get_prefix_len(), trie_insert(), and trie_remove().
>> - Simplify minimask_get_prefix_len(), separate out
>>  minimatch_get_prefix().
>> - Make trie_insert() and trie_remove() take CIDR mask length as a
>>  parameter instead of finding the mask length again.  Call
>>  trie_insert() or trie_remove() only with a non-zero (and positive)
>>  prefix length.
>> - Make trie_init() destroy existing trie (if any) and accept NULL
>>  field, allowing classifier_set_prefix_fields() to be simplified.
>> - Detect duplicate prefix fields on set-up.
>> - Add "fieldspec" documentation to vswitch.xml
>> - Add IPv6 trie testing, verifying that crossing 32-bit boundaries in
>>  masks works.
> 
> Acked-by: Ben Pfaff 
> 
> Here are some changes you may consider, but none of them is more than
> cosmetic.
> 
> diff --git a/lib/classifier.c b/lib/classifier.c
> index e9a13d0..8b5c36a 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -198,9 +198,7 @@ classifier_destroy(struct classifier *cls)
> int i;
> 
> for (i = 0; i < cls->n_tries; i++) {
> -if (cls->tries[i].root) {
> -trie_destroy(cls->tries[i].root);
> -}
> +trie_destroy(cls->tries[i].root);
> }
> 
> HMAP_FOR_EACH_SAFE (subtable, next_subtable, hmap_node,
> @@ -235,7 +233,7 @@ classifier_set_prefix_fields(struct classifier *cls,
> const struct mf_field *field = mf_from_id(trie_fields[i]);
> if (field->flow_be32ofs < 0 || field->n_bits % 32) {
> /* Incompatible field.  This is the only place where we
> - * Enforce these requirements, but the rest of the trie code
> + * enforce these requirements, but the rest of the trie code
>  * depends on the flow_be32ofs to be non-negative and the
>  * field length to be a multiple of 32 bits. */
> continue;
> @@ -268,7 +266,7 @@ trie_init(struct classifier *cls, int tri

[ovs-dev] [PATCH] lib/jhash.c: Fix comment.

[Jarno Rajahalme]

Signed-off-by: Jarno Rajahalme 
---
 lib/jhash.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/jhash.c b/lib/jhash.c
index c08c368..c59b51b 100644
--- a/lib/jhash.c
+++ b/lib/jhash.c
@@ -91,7 +91,7 @@ jhash_words(const uint32_t *p, size_t n, uint32_t basis)
 
 /* Returns the Jenkins hash of the 'n' bytes at 'p', starting from 'basis'.
  *
- * Use jhash_bytes() instead, unless you're computing a hash function whose
+ * Use hash_bytes() instead, unless you're computing a hash function whose
  * value is exposed "on the wire" so we don't want to change it. */
 uint32_t
 jhash_bytes(const void *p_, size_t n, uint32_t basis)
-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [port-renumbering 6/8] lacp: Make lacp negotiation test hard-code aggregation keys.

[Ben Pfaff]

Great minds think alike, I guess.

I applied patches 1 through 6 to master.

On Wed, Dec 11, 2013 at 10:15:25AM -0800, Jarno Rajahalme wrote:
> I bumped to this yesterday and had the exact same patch ready to be sent, so:
> 
> Acked-by: Jarno Rajahalme 
> 
> 
> On Dec 10, 2013, at 11:20 PM, Ben Pfaff  wrote:
> 
> > The lacp implementation takes the aggregation key from the key or portid
> > of the first slave to be added to the lacp object.  When multiple slaves
> > are added initially to a lacp object (the most common case), which is the
> > "first" is arbitrary.  Until now, it seems that the "first" was actually
> > predictable enough for the tests to (unknowingly) rely on it, but an
> > upcoming commit will break that.  This commit fixes the test by forcing
> > a particular aggregation key for each lacp object.
> > 
> > Signed-off-by: Ben Pfaff 
> > ---
> > tests/lacp.at |   12 
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tests/lacp.at b/tests/lacp.at
> > index ffc8996..ede7c8f 100644
> > --- a/tests/lacp.at
> > +++ b/tests/lacp.at
> > @@ -126,8 +126,10 @@ OVS_VSWITCHD_START(
> >   [add-bond br0 bond0 p0 p1 bond_mode=balance-tcp lacp=active \
> > other-config:lacp-time=fast \
> > other-config:bond-rebalance-interval=0 -- \
> > -   set interface p0 type=patch options:peer=p2 ofport_request=1 -- \
> > -   set interface p1 type=patch options:peer=p3 ofport_request=2 -- \
> > +   set interface p0 type=patch options:peer=p2 ofport_request=1 \
> > +other-config:lacp-aggregation-key=2 -- \
> > +   set interface p1 type=patch options:peer=p3 ofport_request=2 \
> > +other-config:lacp-aggregation-key=2 -- \
> >add-br br1 -- \
> >set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
> >set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \
> > @@ -135,8 +137,10 @@ OVS_VSWITCHD_START(
> >add-bond br1 bond1 p2 p3 bond_mode=balance-tcp lacp=active \
> > other-config:lacp-time=fast \
> > other-config:bond-rebalance-interval=0 -- \
> > -   set interface p2 type=patch options:peer=p0 ofport_request=3 -- \
> > -   set interface p3 type=patch options:peer=p1 ofport_request=4 --])
> > +   set interface p2 type=patch options:peer=p0 ofport_request=3 \
> > +other-config:lacp-aggregation-key=4 -- \
> > +   set interface p3 type=patch options:peer=p1 ofport_request=4 \
> > +other-config:lacp-aggregation-key=4 --])
> > 
> > AT_CHECK([ovs-appctl netdev-dummy/set-admin-state up], 0, [OK
> > ])
> > -- 
> > 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] lib/jhash.c: Fix comment.

[Ben Pfaff]

On Wed, Dec 11, 2013 at 11:14:06AM -0800, Jarno Rajahalme wrote:
> 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] lib/jhash.c: Fix comment.

[Jarno Rajahalme]

Pushed, 

  Jarno

On Dec 11, 2013, at 11:14 AM, Ben Pfaff  wrote:

> On Wed, Dec 11, 2013 at 11:14:06AM -0800, Jarno Rajahalme wrote:
>> 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] datapath: Fix sparse warning.

[Jarno Rajahalme]

Pushed to master,

  Jarno

On Dec 11, 2013, at 10:53 AM, Pravin Shelar  wrote:

> On Wed, Dec 11, 2013 at 10:51 AM, Jarno Rajahalme  
> wrote:
>> Make the new compat function skb_flow_get_ports() static to silence a
>> sparse warning.
>> 
>> Signed-off-by: Jarno Rajahalme 
>> ---
>> datapath/linux/compat/flow_dissector.c |2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/datapath/linux/compat/flow_dissector.c 
>> b/datapath/linux/compat/flow_dissector.c
>> index 7a0d09b..a8a2e52 100644
>> --- a/datapath/linux/compat/flow_dissector.c
>> +++ b/datapath/linux/compat/flow_dissector.c
>> @@ -46,7 +46,7 @@ static void iph_to_flow_copy_addrs(struct flow_keys *flow, 
>> const struct iphdr *i
>>memcpy(&flow->src, &iph->saddr, sizeof(flow->src) + 
>> sizeof(flow->dst));
>> }
>> 
>> -__be32 skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto)
>> +static __be32 skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 
>> ip_proto)
>> {
>>int poff = proto_ports_offset(ip_proto);
>> 
>> --
> 
> Looks good.
> 
> Acked-by: Pravin B Shelar 
> 
>> 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] lib/util: Only define count_1bits_8 when needed.

[Ben Pfaff]

On Wed, Dec 11, 2013 at 10:54:00AM -0800, Jarno Rajahalme wrote:
> util.h declares this when needed, make sure the definition is compiled
> in only in that case.
> 
> Signed-off-by: Jarno Rajahalme 

With this, I still get:
../lib/util.c:921:15: warning: symbol 'count_1bits_8' was not declared. 
Should it be static?
because the previous declaration of count_1bits_8[] is not in scope at
the point of definition in util.c.

How about something like this?  (Not tested outside of 32-bit without
__POPCNT__.)

diff --git a/lib/util.c b/lib/util.c
index 13d41a7..000504c 100644
--- a/lib/util.c
+++ b/lib/util.c
@@ -901,7 +901,7 @@ raw_clz64(uint64_t n)
 }
 #endif
 
-#if !(__GNUC__ >= 4 && defined(__corei7))
+#if NEED_COUNT_1BITS_8
 #define INIT1(X)\
 X) & (1 << 0)) != 0) +  \
  (((X) & (1 << 1)) != 0) +  \
diff --git a/lib/util.h b/lib/util.h
index 8d810c2..b158c2f 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -371,49 +371,55 @@ log_2_ceil(uint64_t n)
 return log_2_floor(n) + !is_pow2(n);
 }
 
-extern const uint8_t count_1bits_8[256];
-
-/* Returns the number of 1-bits in 'x', between 0 and 32 inclusive. */
+/* unsigned int count_1bits(uint64_t x):
+ *
+ * Returns the number of 1-bits in 'x', between 0 and 64 inclusive. */
+#if UINTPTR_MAX == UINT64_MAX
+static inline unsigned int
+count_1bits(uint64_t x)
+{
+#if __GNUC__ >= 4 && __POPCNT__
+return __builtin_popcountll(x);
+#else
+/* This portable implementation is the fastest one we know of for 64
+ * bits, and about 3x faster than GCC 4.7 __builtin_popcountll(). */
+const uint64_t h55 = UINT64_C(0x);
+const uint64_t h33 = UINT64_C(0x);
+const uint64_t h0F = UINT64_C(0x0F0F0F0F0F0F0F0F);
+const uint64_t h01 = UINT64_C(0x0101010101010101);
+x -= (x >> 1) & h55;   /* Count of each 2 bits in-place. */
+x = (x & h33) + ((x >> 2) & h33);  /* Count of each 4 bits in-place. */
+x = (x + (x >> 4)) & h0F;  /* Count of each 8 bits in-place. */
+return (x * h01) >> 56;/* Sum of all bytes. */
+#endif
+}
+#else  /* not 64-bit */
+#if __GNUC__ >= 4 && __POPCNT__
 static inline unsigned int
 count_1bits_32(uint32_t x)
 {
-#if __GNUC__ >= 4 && defined(__corei7)
-/* __builtin_popcount() is fast only when supported by the CPU. */
 return __builtin_popcount(x);
+}
 #else
+#define NEED_COUNT_1BITS_8 1
+extern const uint8_t count_1bits_8[256];
+static inline unsigned int
+count_1bits_32(uint32_t x)
+{
 /* This portable implementation is the fastest one we know of for 32 bits,
  * and faster than GCC __builtin_popcount(). */
 return (count_1bits_8[x & 0xff] +
 count_1bits_8[(x >> 8) & 0xff] +
 count_1bits_8[(x >> 16) & 0xff] +
 count_1bits_8[x >> 24]);
-#endif
 }
-
-/* Returns the number of 1-bits in 'x', between 0 and 64 inclusive. */
+#endif
 static inline unsigned int
 count_1bits(uint64_t x)
 {
-if (sizeof(void *) == 8) { /* 64-bit CPU */
-#if __GNUC__ >= 4 && defined(__corei7)
-/* __builtin_popcountll() is fast only when supported by the CPU. */
-return __builtin_popcountll(x);
-#else
-/* This portable implementation is the fastest one we know of for 64
- * bits, and about 3x faster than GCC 4.7 __builtin_popcountll(). */
-const uint64_t h55 = UINT64_C(0x);
-const uint64_t h33 = UINT64_C(0x);
-const uint64_t h0F = UINT64_C(0x0F0F0F0F0F0F0F0F);
-const uint64_t h01 = UINT64_C(0x0101010101010101);
-x -= (x >> 1) & h55;   /* Count of each 2 bits in-place. */
-x = (x & h33) + ((x >> 2) & h33);  /* Count of each 4 bits in-place. */
-x = (x + (x >> 4)) & h0F;  /* Count of each 8 bits in-place. */
-return (x * h01) >> 56;/* Sum of all bytes. */
-#endif
-} else { /* 32-bit CPU */
-return count_1bits_32(x) + count_1bits_32(x >> 32);
-}
+return count_1bits_32(x) + count_1bits_32(x >> 32);
 }
+#endif
 
 /* Returns the rightmost 1-bit in 'x' (e.g. 01011000 => 1000), or 0 if 'x'
  * is 0. */
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ofproto-dpif: Ignore non-packet field masks during flow revalidation

[Andy Zhou]

Commit bcd2633a5be(ofproto-dpif: Store relevant fields for wildcarding
in facet) implements flow revalidation by comparing the newly looked
up flow mask with that of the existing facet.

The non-packet fields, such as register masks, are always cleared
by xlate_actions in the masks stored within facets, but they are not
cleared in the newly looked up flow masks, causing otherwise valid
flows to be declared as invalid flows and be removed from the datapath.

This patch provides a fix. I was able to verify the fix on a system set
up by Ying Chen where the bug can be reproduced.

Bug #21680
Reported by: Ying Chen 

Signed-off-by: Andy Zhou 
---
 lib/flow.c   |9 +
 lib/flow.h   |2 ++
 ofproto/ofproto-dpif-xlate.c |3 +--
 ofproto/ofproto-dpif.c   |3 +++
 4 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/lib/flow.c b/lib/flow.c
index 0dfd01f..31a07f8 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -630,6 +630,15 @@ flow_wildcards_init_catchall(struct flow_wildcards *wc)
 memset(&wc->masks, 0, sizeof wc->masks);
 }
 
+/* Clear the metadata and register wildcard masks. They are not packet
+ * header fields. */
+void
+flow_wildcards_clear_non_packet_fields(struct flow_wildcards *wc)
+{
+memset(&wc->masks.metadata, 0, sizeof wc->masks.metadata);
+memset(&wc->masks.regs, 0, sizeof wc->masks.regs);
+}
+
 /* Returns true if 'wc' matches every packet, false if 'wc' fixes any bits or
  * fields. */
 bool
diff --git a/lib/flow.h b/lib/flow.h
index 5c0007d..44d96ad 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -284,6 +284,8 @@ struct flow_wildcards {
 
 void flow_wildcards_init_catchall(struct flow_wildcards *);
 
+void flow_wildcards_clear_non_packet_fields(struct flow_wildcards *);
+
 bool flow_wildcards_is_catchall(const struct flow_wildcards *);
 
 void flow_wildcards_set_reg_mask(struct flow_wildcards *,
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index b0f61ca..aec17c0 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3203,8 +3203,7 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out 
*xout)
 
 /* Clear the metadata and register wildcard masks, because we won't
  * use non-header fields as part of the cache. */
-memset(&wc->masks.metadata, 0, sizeof wc->masks.metadata);
-memset(&wc->masks.regs, 0, sizeof wc->masks.regs);
+flow_wildcards_clear_non_packet_fields(wc);
 
 out:
 rule_actions_unref(actions);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ff77903..8d9a9bd 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4219,6 +4219,9 @@ facet_revalidate(struct facet *facet)
 xlate_in_init(&xin, ofproto, &facet->flow, new_rule, 0, NULL);
 xlate_actions(&xin, &xout);
 flow_wildcards_or(&xout.wc, &xout.wc, &wc);
+/* Make sure non packet fields are not masked. If not cleared, the memcmp()
+ * below may fail, causing otherwise valid facet to be removed. */
+flow_wildcards_clear_non_packet_fields(&xout.wc);
 
 /* A facet's slow path reason should only change under dramatic
  * circumstances.  Rather than try to update everything, it's simpler to
-- 
1.7.9.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ofproto-dpif: Ignore non-packet field masks during flow revalidation

[Justin Pettit]

On Dec 11, 2013, at 11:37 AM, Andy Zhou  wrote:

> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index ff77903..8d9a9bd 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4219,6 +4219,9 @@ facet_revalidate(struct facet *facet)
> xlate_in_init(&xin, ofproto, &facet->flow, new_rule, 0, NULL);
> xlate_actions(&xin, &xout);
> flow_wildcards_or(&xout.wc, &xout.wc, &wc);
> +/* Make sure non packet fields are not masked. If not cleared, the 
> memcmp()

Nitpicking, but can you make it "non-packet".

> + * below may fail, causing otherwise valid facet to be removed. */

Can you add an "an" before "otherwise"?

Acked-by: Justin Pettit 

--Justin


___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [Windows thread 3]

[Saurabh Shah]

Hey,


The following is a quick patch for secure pseudorandom number generator on 
windows. I split the functionality with a brutal ifdef macro. Feedback on the 
code and suggestions for a nicer implementation is appreciated :).

diff --git a/lib/entropy.c b/lib/entropy.c
index 02f56e0..ec9d95c 100644
--- a/lib/entropy.c
+++ b/lib/entropy.c
@@ -20,6 +20,9 @@
#include 
#include 
#include 
+#ifdef _WIN32
+#include 
+#endif

#include "socket-util.h"
#include "vlog.h"
@@ -33,6 +36,7 @@ static const char urandom[] = "/dev/urandom";
int
get_entropy(void *buffer, size_t n)
{
+#ifndef _WIN32
 size_t bytes_read;
 int error;
 int fd;
@@ -49,6 +53,20 @@ get_entropy(void *buffer, size_t n)
 if (error) {
 VLOG_ERR("%s: read error (%s)", urandom, ovs_retval_to_string(error));
 }
+#else
+ int error = 1;
+ HCRYPTPROV   crypt_prov = 0;
+ CryptAcquireContext(&crypt_prov, NULL, NULL, PROV_RSA_FULL, 0);
+

Microsoft documentation suggests using CRYPT_VERIFYCONTEXT. Although, I haven't 
tested to see what sort of an impact this will have.

http://msdn.microsoft.com/en-us/library/windows/desktop/aa379886(v=vs.85).aspx
For performance reasons, we recommend that you set the pszContainer parameter 
to NULL and the dwFlags parameter to CRYPT_VERIFYCONTEXT in all situations 
where you do not require a persisted key. In particular, consider setting the 
pszContainer parameter to NULL and the dwFlags parameter to CRYPT_VERIFYCONTEXT 
for the following scenarios:

+ if (CryptGenRandom(crypt_prov, n, buffer)) {
+ error = 0;
+ }
+
+ if (error) {
+ VLOG_ERR("CryptGenRandom: read error (%s)", urandom, 
ovs_retval_to_string(error));
+ }

How about doing instead -

int error = 0;
If (! CryptGetRandom(crypt_prov, n, buffer)) {
error = GetLastError();
 VLOG_ERR("CryptGenRandom: read error (%s)", urandom, 
ovs_retval_to_string(error));
}

+ CryptReleaseContext(crypt_prov, 0);
+#endif
 return error;
}


Kind Regards,

Alin.

___
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=pEkjsHfytvHEWufeZPpgqSOJMdMjuZPbesVsNhCUc0E%3D%0A&m=KlUcJXE7spv5Cm%2FmexYFbql6rLI%2BJfpjXWgtb05Lero%3D%0A&s=ccb6c3872370fa5ee60d07509dba3d0a07ebc526274c18553e02ab434de8bcdb

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC 1/4] compiler: Add OVS_CONSTRUCTOR to mark functions as init functions

[Ben Pfaff]

On Tue, Dec 10, 2013 at 02:49:27PM +0100, Helmut Schaa wrote:
> Functions marked with OVS_CONSTRUCTOR are called unconditionally
> before main.
> 
> Signed-off-by: Helmut Schaa 
> ---
> 
> This works with gcc (tested), should work with clang (untested)
> but does not work with MSVS.
> 
> Could anyone using MSVC try if the solution described at
> http://stackoverflow.com/questions/1113409/attribute-constructor-equivalent-in-vc
> is suitable?

Guru already answered your actual question, but I'll comment that I
like the solution there better than the one here.  In other words, I
how the macro there is used.

> Is MSVC even supported in OVS?

No, but some contributors are working on it.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC 2/4] vlog: Use OVS_CONSTRUCTOR for vlog initialization

[Ben Pfaff]

On Tue, Dec 10, 2013 at 02:49:28PM +0100, Helmut Schaa wrote:
> This allows to get rid of some special segment handling to allow
> distributed registering of vlog modules.
> 
> Instead use a global list and vlog module constructor functions to
> build up the list. That means vlog modules reside within the
> compilation unit they are defined in and can be iterated upon
> by using the global list vlog_modules.
> 
> Signed-off-by: Helmut Schaa 

I'm happy with this once we get the OVS_CONSTRUCTOR definition sorted
out.

Thanks,

Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC 3/4] coverage: Use OVS_CONSTRUCTOR to initialize the coverage counter array

[Ben Pfaff]

On Tue, Dec 10, 2013 at 02:49:29PM +0100, Helmut Schaa wrote:
> Use a global array that gets initialized by constructor functions
> per coverage definition.
> 
> Signed-off-by: Helmut Schaa 
> ---
> 
> The realloc is not the nicest thing to do but looks reasonable as initial
> approach. Using a list would cause more intrusive changes to the coverage
> implementation.

Could you refactor the constructor code as a call to a helper
function, please?  (That will allow you to make the coverage_counters
definitions static within coverage.c, I think.)

I suggest using x2nrealloc instead of xrealloc for this.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC 4/4] Remove unused USE_LINKER_SECTIONS

[Ben Pfaff]

On Tue, Dec 10, 2013 at 02:49:30PM +0100, Helmut Schaa wrote:
> 
> Signed-off-by: Helmut Schaa 

You can also remove OVS_LINK2_IFELSE.

Thanks,

Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC 1/4] compiler: Add OVS_CONSTRUCTOR to mark functions as init functions

[Ben Pfaff]

On Wed, Dec 11, 2013 at 12:58 PM, Ben Pfaff  wrote:

> On Tue, Dec 10, 2013 at 02:49:27PM +0100, Helmut Schaa wrote:
> > Functions marked with OVS_CONSTRUCTOR are called unconditionally
> > before main.
> >
> > Signed-off-by: Helmut Schaa 
> > ---
> >
> > This works with gcc (tested), should work with clang (untested)
> > but does not work with MSVS.
> >
> > Could anyone using MSVC try if the solution described at
> >
> http://stackoverflow.com/questions/1113409/attribute-constructor-equivalent-in-vc
> > is suitable?
>
> Guru already answered your actual question, but I'll comment that I
> like the solution there better than the one here.  In other words, I
>

...like...


> how the macro there is used.
>
> > Is MSVC even supported in OVS?
>
> No, but some contributors are working on it.
>



-- 
"I don't normally do acked-by's.  I think it's my way of avoiding
getting blamed when it all blows up."   Andrew Morton
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [port-renumbering 7/8] bridge: Reconfigure in single pass.

[Jarno Rajahalme]

On Dec 10, 2013, at 11:20 PM, Ben Pfaff  wrote:

> This greatly simplifies the reconfiguration code, making it much easier
> to understand and modify.  The old multi-pass configuration had the
> property that it didn't delay block packet processing as much, but that's
> not much of a worry anymore now that latency critical activities have
> been moved outside the main thread.
> 

You’ll find two minor comments below, but otherwise seems good to me. I have no 
history with this code, though, so with that caveat:

Acked-by: Jarno Rajahalme 

…
> +static void
> +bridge_delete_ports(struct bridge *br)

IMO this function should be renamed, as it only deletes ports that should not 
exists, are of wrong type, or cannot be successfully configured. How about 
“bridge_conf_or_del_ports” etc.?

> +{
> +struct ofproto_port ofproto_port;
> +struct ofproto_port_dump dump;
> 
> -/* Remove ports from any datapath with the same name as 'br'.  If we
> - * don't do this, creating 'br''s ofproto will fail because a port 
> with
> - * the same name as its local port already exists. */
> -HMAP_FOR_EACH (br2, node, &all_bridges) {
> -struct ofproto_port ofproto_port;
> +ofp_port_t *del;
> +size_t n, allocated;
> +size_t i;
> +
…

> @@ -2824,44 +2625,23 @@ bridge_get_controllers(const struct bridge *br,
> }
> 
> static void
> -bridge_queue_if_cfg(struct bridge *br,
> -const struct ovsrec_interface *cfg,
> -const struct ovsrec_port *parent)
> +bridge_collect_wanted_ports(struct bridge *br,
> +const unsigned long int *splinter_vlans,
> +struct shash *wanted_ports)
> {
> -struct if_cfg *if_cfg = xmalloc(sizeof *if_cfg);
> -
> -if_cfg->cfg = cfg;
> -if_cfg->parent = parent;
> -if_cfg->ofport = iface_pick_ofport(cfg);
> -hmap_insert(&br->if_cfg_todo, &if_cfg->hmap_node,
> -hash_string(if_cfg->cfg->name, 0));
> -}
> -
> -/* Deletes "struct port"s and "struct iface"s under 'br' which aren't
> - * consistent with 'br->cfg'.  Updates 'br->if_cfg_queue' with interfaces 
> which
> - * 'br' needs to complete its configuration. */
> -static void
> -bridge_add_del_ports(struct bridge *br,
> - const unsigned long int *splinter_vlans)
> -{
> -struct shash_node *port_node;
> -struct port *port, *next;
> -struct shash new_ports;
> size_t i;
> +

Extra trailing whitespace on this line.

> +shash_init(wanted_ports);
> 
> -ovs_assert(hmap_is_empty(&br->if_cfg_todo));
> -
...

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] lib: Determine cpu core count with /proc/cpuinfo

[Ed Maste]

On 5 December 2013 18:42, Joe Stringer  wrote:
> On systems that provide /proc/cpuinfo similar to Linux on x86, this
> should allow us to choose a better default value for the number of
> upcall handler threads---in particular, it avoids counting hyper-thread
> cores. If /proc/cpuinfo cannot be parsed for any reason, fall back to
> using sysconf().
...
> +if (!stream) {
> +VLOG_WARN("%s: open failed (%s)", file_name, ovs_strerror(errno));

The warning causes much of the testsuite to fail on systems that don't
have /proc/cpuinfo.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] lib: Determine cpu core count with /proc/cpuinfo

[Joe Stringer]

Hmm. Is it enough set a lower log level? /proc/cpuinfo parsing isn't
strictly required.

On 11 December 2013 13:26, Ed Maste  wrote:
> On 5 December 2013 18:42, Joe Stringer  wrote:
>> On systems that provide /proc/cpuinfo similar to Linux on x86, this
>> should allow us to choose a better default value for the number of
>> upcall handler threads---in particular, it avoids counting hyper-thread
>> cores. If /proc/cpuinfo cannot be parsed for any reason, fall back to
>> using sysconf().
> ...
>> +if (!stream) {
>> +VLOG_WARN("%s: open failed (%s)", file_name, ovs_strerror(errno));
>
> The warning causes much of the testsuite to fail on systems that don't
> have /proc/cpuinfo.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ofproto-dpif: Ignore non-packet field masks during flow revalidation

[Andy Zhou]

Thanks for the review, pushed to master and branch-2.0 with fixes suggested.


On Wed, Dec 11, 2013 at 11:48 AM, Justin Pettit  wrote:

> On Dec 11, 2013, at 11:37 AM, Andy Zhou  wrote:
>
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index ff77903..8d9a9bd 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -4219,6 +4219,9 @@ facet_revalidate(struct facet *facet)
> > xlate_in_init(&xin, ofproto, &facet->flow, new_rule, 0, NULL);
> > xlate_actions(&xin, &xout);
> > flow_wildcards_or(&xout.wc, &xout.wc, &wc);
> > +/* Make sure non packet fields are not masked. If not cleared, the
> memcmp()
>
> Nitpicking, but can you make it "non-packet".
>
> > + * below may fail, causing otherwise valid facet to be removed. */
>
> Can you add an "an" before "otherwise"?
>
> Acked-by: Justin Pettit 
>
> --Justin
>
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ovs-thread: Reduce logging level for cpuinfo parsing

[Joe Stringer]

This was causing test failures on non-Linux platforms.

Reported-by: Ed Maste 
Signed-off-by: Joe Stringer 
---
 lib/ovs-thread.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
index 1a633cf..dcaf7ff 100644
--- a/lib/ovs-thread.c
+++ b/lib/ovs-thread.c
@@ -326,7 +326,7 @@ parse_cpuinfo(long int *n_cores)
 
 stream = fopen(file_name, "r");
 if (!stream) {
-VLOG_WARN("%s: open failed (%s)", file_name, ovs_strerror(errno));
+VLOG_DBG("%s: open failed (%s)", file_name, ovs_strerror(errno));
 return;
 }
 
-- 
1.7.9.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] lib: Determine cpu core count with /proc/cpuinfo

[Joe Stringer]

I'm pretty sure that we only care about this warning if we are
specifically investigating how we calculate the number of threads to
use.

I've sent a patch below to reduce this to debug logging:-
http://openvswitch.org/pipermail/dev/2013-December/034899.html
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovs-thread: Reduce logging level for cpuinfo parsing

[Ethan Jackson]

Merged thanks.

On Wed, Dec 11, 2013 at 1:46 PM, Joe Stringer  wrote:
> This was causing test failures on non-Linux platforms.
>
> Reported-by: Ed Maste 
> Signed-off-by: Joe Stringer 
> ---
>  lib/ovs-thread.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
> index 1a633cf..dcaf7ff 100644
> --- a/lib/ovs-thread.c
> +++ b/lib/ovs-thread.c
> @@ -326,7 +326,7 @@ parse_cpuinfo(long int *n_cores)
>
>  stream = fopen(file_name, "r");
>  if (!stream) {
> -VLOG_WARN("%s: open failed (%s)", file_name, ovs_strerror(errno));
> +VLOG_DBG("%s: open failed (%s)", file_name, ovs_strerror(errno));
>  return;
>  }
>
> --
> 1.7.9.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [port-renumbering 6/8] lacp: Make lacp negotiation test hard-code aggregation keys.

[Ethan Jackson]

FWIW I'm fine with this as well.

Ethan

On Wed, Dec 11, 2013 at 11:13 AM, Ben Pfaff  wrote:
> Great minds think alike, I guess.
>
> I applied patches 1 through 6 to master.
>
> On Wed, Dec 11, 2013 at 10:15:25AM -0800, Jarno Rajahalme wrote:
>> I bumped to this yesterday and had the exact same patch ready to be sent, so:
>>
>> Acked-by: Jarno Rajahalme 
>>
>>
>> On Dec 10, 2013, at 11:20 PM, Ben Pfaff  wrote:
>>
>> > The lacp implementation takes the aggregation key from the key or portid
>> > of the first slave to be added to the lacp object.  When multiple slaves
>> > are added initially to a lacp object (the most common case), which is the
>> > "first" is arbitrary.  Until now, it seems that the "first" was actually
>> > predictable enough for the tests to (unknowingly) rely on it, but an
>> > upcoming commit will break that.  This commit fixes the test by forcing
>> > a particular aggregation key for each lacp object.
>> >
>> > Signed-off-by: Ben Pfaff 
>> > ---
>> > tests/lacp.at |   12 
>> > 1 file changed, 8 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/tests/lacp.at b/tests/lacp.at
>> > index ffc8996..ede7c8f 100644
>> > --- a/tests/lacp.at
>> > +++ b/tests/lacp.at
>> > @@ -126,8 +126,10 @@ OVS_VSWITCHD_START(
>> >   [add-bond br0 bond0 p0 p1 bond_mode=balance-tcp lacp=active \
>> > other-config:lacp-time=fast \
>> > other-config:bond-rebalance-interval=0 -- \
>> > -   set interface p0 type=patch options:peer=p2 ofport_request=1 -- \
>> > -   set interface p1 type=patch options:peer=p3 ofport_request=2 -- \
>> > +   set interface p0 type=patch options:peer=p2 ofport_request=1 \
>> > +other-config:lacp-aggregation-key=2 -- \
>> > +   set interface p1 type=patch options:peer=p3 ofport_request=2 \
>> > +other-config:lacp-aggregation-key=2 -- \
>> >add-br br1 -- \
>> >set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
>> >set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \
>> > @@ -135,8 +137,10 @@ OVS_VSWITCHD_START(
>> >add-bond br1 bond1 p2 p3 bond_mode=balance-tcp lacp=active \
>> > other-config:lacp-time=fast \
>> > other-config:bond-rebalance-interval=0 -- \
>> > -   set interface p2 type=patch options:peer=p0 ofport_request=3 -- \
>> > -   set interface p3 type=patch options:peer=p1 ofport_request=4 --])
>> > +   set interface p2 type=patch options:peer=p0 ofport_request=3 \
>> > +other-config:lacp-aggregation-key=4 -- \
>> > +   set interface p3 type=patch options:peer=p1 ofport_request=4 \
>> > +other-config:lacp-aggregation-key=4 --])
>> >
>> > AT_CHECK([ovs-appctl netdev-dummy/set-admin-state up], 0, [OK
>> > ])
>> > --
>> > 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

Re: [ovs-dev] [port-renumbering 7/8] bridge: Reconfigure in single pass.

[Jarno Rajahalme]

Ben,

I did some additional testing and accidentally found out a problem with this 
patch:

While testing the code I accidentally requested a change in the interface type 
(to dummy), like this:

# ovs-vsctl add-br br0
# ovs-vsctl add-port br0 p1 -- set Interface p1 type=internal
# ovs-vsctl -- set Interface p1 type=dummy

Running OVS without dummy enabled seg faults:

ovsdb-server: /usr/local/var/run/openvswitch/ovsdb-server.pid: already running 
as pid 57673, aborting
2013-12-11T22:11:12Z|1|reconnect|INFO|unix:/usr/local/var/run/openvswitch/db.sock:
 connecting...
2013-12-11T22:11:12Z|2|reconnect|INFO|unix:/usr/local/var/run/openvswitch/db.sock:
 connected
2013-12-11T22:11:12Z|3|bridge|INFO|ovs-vswitchd (Open vSwitch) 2.0.90
2013-12-11T22:11:26Z|4|memory|INFO|2172 kB peak resident set size after 
14.0 seconds
2013-12-11T22:11:26Z|5|bridge|INFO|bridge br0: added interface br0 on port 
65534
2013-12-11T22:11:26Z|6|bridge|INFO|bridge br0: using datapath ID 
3e6b1980eb47
2013-12-11T22:11:26Z|7|connmgr|INFO|br0: added service controller 
"punix:/usr/local/var/run/openvswitch/br0.mgmt"
2013-12-11T22:11:37Z|8|bridge|INFO|bridge br0: added interface p1 on port 1
2013-12-11T22:11:46Z|9|netdev|WARN|could not create netdev p1 of unknown 
type dummy
2013-12-11T22:11:46Z|00010|bridge|WARN|could not open network device p1 
(Address family not supported by protocol)
./start_ovs.sh: line 6:  6961 Segmentation fault  ovs-vswitchd --pidfile 
-vinfo


This does not happen on master, so this patch is at fault.

  Jarno

On Dec 10, 2013, at 11:20 PM, Ben Pfaff  wrote:

> This greatly simplifies the reconfiguration code, making it much easier
> to understand and modify.  The old multi-pass configuration had the
> property that it didn't delay block packet processing as much, but that's
> not much of a worry anymore now that latency critical activities have
> been moved outside the main thread.
> 
> Signed-off-by: Ben Pfaff 
> ---
> vswitchd/bridge.c |  632 +
> 1 file changed, 202 insertions(+), 430 deletions(-)
> 
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 0b0e4d7..581f87c 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -63,33 +63,20 @@ VLOG_DEFINE_THIS_MODULE(bridge);
> 
> COVERAGE_DEFINE(bridge_reconfigure);
> 
> -/* Configuration of an uninstantiated iface. */
> -struct if_cfg {
> -struct hmap_node hmap_node; /* Node in bridge's if_cfg_todo. */
> -const struct ovsrec_interface *cfg; /* Interface record. */
> -const struct ovsrec_port *parent;   /* Parent port record. */
> -ofp_port_t ofport;  /* Requested OpenFlow port number. */
> -};
> -
> -/* OpenFlow port slated for removal from ofproto. */
> -struct ofpp_garbage {
> -struct list list_node;  /* Node in bridge's ofpp_garbage. */
> -ofp_port_t ofp_port;/* Port to be deleted. */
> -};
> -
> struct iface {
> -/* These members are always valid. */
> +/* These members are always valid.
> + *
> + * They are immutable: they never change between iface_create() and
> + * iface_destroy(). */
> struct list port_elem;  /* Element in struct port's "ifaces" list. */
> struct hmap_node name_node; /* In struct bridge's "iface_by_name" hmap. */
> +struct hmap_node ofp_port_node; /* In struct bridge's "ifaces" hmap. */
> struct port *port;  /* Containing port. */
> char *name; /* Host network device name. */
> -
> -/* These members are valid only after bridge_reconfigure() causes them to
> - * be initialized. */
> -struct hmap_node ofp_port_node; /* In struct bridge's "ifaces" hmap. */
> -ofp_port_t ofp_port;/* OpenFlow port number, */
> -/* OFPP_NONE if unknown. */
> struct netdev *netdev;  /* Network device. */
> +ofp_port_t ofp_port;/* OpenFlow port number. */
> +
> +/* These members are valid only within bridge_reconfigure(). */
> const char *type;   /* Usually same as cfg->type. */
> const struct ovsrec_interface *cfg;
> };
> @@ -130,13 +117,12 @@ struct bridge {
> struct hmap ifaces; /* "struct iface"s indexed by ofp_port. */
> struct hmap iface_by_name;  /* "struct iface"s indexed by name. */
> 
> -struct list ofpp_garbage;   /* "struct ofpp_garbage" slated for removal. 
> */
> -struct hmap if_cfg_todo;/* "struct if_cfg"s slated for creation.
> -   Indexed on 'cfg->name'. */
> -
> /* Port mirroring. */
> struct hmap mirrors;/* "struct mirror" indexed by UUID. */
> 
> +/* Used during reconfiguration. */
> +struct shash wanted_ports;
> +
> /* Synthetic local port if necessary. */
> struct ovsrec_port synth_local_port;
> struct ovsrec_interface synth_local_iface;
> @@ -183,10 +169,8 @@ static long long int iface_stats_timer = LLONG_MIN;
>  * 

Re: [ovs-dev] [port-renumbering 8/8] bridge: Support changing port numbers.

[Jarno Rajahalme]

On Dec 10, 2013, at 11:20 PM, Ben Pfaff  wrote:

> Feature #21293.
> Requested-by: Anuprem Chalvadi 
> Signed-off-by: Ben Pfaff 
> ---
> NEWS |3 ++
> vswitchd/bridge.c|   93 --
> vswitchd/vswitch.xml |   21 +++-
> 3 files changed, 99 insertions(+), 18 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 8556083..ce968f2 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -14,6 +14,9 @@ Post-v2.0.0
>  * OVS limits the OpenFlow port numbers it assigns to port 32767 and
>below, leaving port numbers above that range free for assignment
>by the controller.
> + * ovs-vswitchd now honors changes to the "ofport_request" column
> +   in the Interface table by changing the port's OpenFlow port
> +   number.
>- ovs-vswitchd.conf.db.5 man page will contain graphviz/dot
>  diagram only if graphviz package was installed at the build time.
>- Support for Linux kernels up to 3.11
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 581f87c..131b800 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -75,6 +75,7 @@ struct iface {
> char *name; /* Host network device name. */
> struct netdev *netdev;  /* Network device. */
> ofp_port_t ofp_port;/* OpenFlow port number. */
> +ofp_port_t requested_ofp_port; /* Port number requested previously. */
> 
> /* These members are valid only within bridge_reconfigure(). */
> const char *type;   /* Usually same as cfg->type. */
> @@ -247,6 +248,8 @@ static void iface_refresh_cfm_stats(struct iface *);
> static void iface_refresh_stats(struct iface *);
> static void iface_refresh_status(struct iface *);
> static bool iface_is_synthetic(const struct iface *);
> +static ofp_port_t iface_get_requested_ofp_port(
> +const struct ovsrec_interface *);
> static ofp_port_t iface_pick_ofport(const struct ovsrec_interface *);
> 
> /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
> @@ -636,6 +639,7 @@ bridge_delete_ports(struct bridge *br)
> n = allocated = 0;
> 
> OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) {
> +ofp_port_t requested_ofp_port;
> struct iface *iface;
> 
> iface = iface_lookup(br, ofproto_port.name);
> @@ -657,6 +661,43 @@ bridge_delete_ports(struct bridge *br)
> goto delete;
> }
> 
> +/* If the requested OpenFlow port for 'iface' changed, and it's not
> + * already the correct port, then we might want to temporarily delete
> + * this interface, so we can add it back again with the new OpenFlow
> + * port number. */
> +requested_ofp_port = iface_get_requested_ofp_port(iface->cfg);
> +if (requested_ofp_port != iface->requested_ofp_port &&
> +requested_ofp_port != OFPP_NONE &&
> +requested_ofp_port != iface->ofp_port) {

It would seem that this test should fail if the face requested a port number 
that it did not get (for any reason), and is still requesting the same port 
number, but the offending other port was deleted and the (originally) requested 
port number would now be available. However, when testing this, this seems to 
work:

- create ports p1, p2
- request port number 10 for p1 -> p1 changes to 10
- request port number 10 for p2 -> p2 does not change
- request port number 11 for p1 -> p1 changes to 11 AND p2 changes to 10

Is it so that the requested port number is not stored unless it is also gotten?

 Maybe this is intended behavior to avoid surprisingly changing port numbers, 
even if the change would be towards the wanted configuration?

> +ofp_port_t victim_request;
> +struct iface *victim;
> +
> +/* Check for an existing OpenFlow port currently occupying
> + * 'iface''s requested port number.  If there isn't one, then
> + * delete this port.  Otherwise we need to consider further. */
> +victim = iface_from_ofp_port(br, requested_ofp_port);
> +if (!victim) {
> +goto delete;
> +}
> +
> +/* 'victim' is a port currently using 'iface''s requested port
> + * number.  Unless 'victim' specifically requested that port
> + * number, too, then we can delete both 'iface' and 'victim'
> + * temporarily.  (We'll add both of them back again later with 
> new
> + * OpenFlow port numbers.)
> + *
> + * If 'victim' did request port number 'requested_ofp_port', just
> + * like 'iface', then that's a configuration inconsistency that 
> we
> + * can't resolve.  We might as well let it keep its current port
> + * number. */
> +victim_request = iface_get_requested_ofp_port(victim->cfg);
> +if (victim_request != requested_ofp_port) {
> +del = add_ofp_port(victim->ofp_port, del, &n, &allocate

Re: [ovs-dev] [PATCH] ofproto-dpif: Ignore non-packet field masks during flow revalidation

[Justin Pettit]

Did you determine whether it should be backported to branch-1.11?

--Justin


On Dec 11, 2013, at 1:34 PM, Andy Zhou  wrote:

> Thanks for the review, pushed to master and branch-2.0 with fixes suggested.
> 
> 
> On Wed, Dec 11, 2013 at 11:48 AM, Justin Pettit  wrote:
> On Dec 11, 2013, at 11:37 AM, Andy Zhou  wrote:
> 
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index ff77903..8d9a9bd 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -4219,6 +4219,9 @@ facet_revalidate(struct facet *facet)
> > xlate_in_init(&xin, ofproto, &facet->flow, new_rule, 0, NULL);
> > xlate_actions(&xin, &xout);
> > flow_wildcards_or(&xout.wc, &xout.wc, &wc);
> > +/* Make sure non packet fields are not masked. If not cleared, the 
> > memcmp()
> 
> Nitpicking, but can you make it "non-packet".
> 
> > + * below may fail, causing otherwise valid facet to be removed. */
> 
> Can you add an "an" before "otherwise"?
> 
> Acked-by: Justin Pettit 
> 
> --Justin
> 
> 
> 

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [port-renumbering 7/8] bridge: Reconfigure in single pass.

[Ethan Jackson]

I'm happy with this once the segfault is fixed.

Ethan

On Tue, Dec 10, 2013 at 11:20 PM, Ben Pfaff  wrote:
> This greatly simplifies the reconfiguration code, making it much easier
> to understand and modify.  The old multi-pass configuration had the
> property that it didn't delay block packet processing as much, but that's
> not much of a worry anymore now that latency critical activities have
> been moved outside the main thread.
>
> Signed-off-by: Ben Pfaff 
> ---
>  vswitchd/bridge.c |  632 
> +
>  1 file changed, 202 insertions(+), 430 deletions(-)
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 0b0e4d7..581f87c 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -63,33 +63,20 @@ VLOG_DEFINE_THIS_MODULE(bridge);
>
>  COVERAGE_DEFINE(bridge_reconfigure);
>
> -/* Configuration of an uninstantiated iface. */
> -struct if_cfg {
> -struct hmap_node hmap_node; /* Node in bridge's if_cfg_todo. */
> -const struct ovsrec_interface *cfg; /* Interface record. */
> -const struct ovsrec_port *parent;   /* Parent port record. */
> -ofp_port_t ofport;  /* Requested OpenFlow port number. */
> -};
> -
> -/* OpenFlow port slated for removal from ofproto. */
> -struct ofpp_garbage {
> -struct list list_node;  /* Node in bridge's ofpp_garbage. */
> -ofp_port_t ofp_port;/* Port to be deleted. */
> -};
> -
>  struct iface {
> -/* These members are always valid. */
> +/* These members are always valid.
> + *
> + * They are immutable: they never change between iface_create() and
> + * iface_destroy(). */
>  struct list port_elem;  /* Element in struct port's "ifaces" list. */
>  struct hmap_node name_node; /* In struct bridge's "iface_by_name" hmap. 
> */
> +struct hmap_node ofp_port_node; /* In struct bridge's "ifaces" hmap. */
>  struct port *port;  /* Containing port. */
>  char *name; /* Host network device name. */
> -
> -/* These members are valid only after bridge_reconfigure() causes them to
> - * be initialized. */
> -struct hmap_node ofp_port_node; /* In struct bridge's "ifaces" hmap. */
> -ofp_port_t ofp_port;/* OpenFlow port number, */
> -/* OFPP_NONE if unknown. */
>  struct netdev *netdev;  /* Network device. */
> +ofp_port_t ofp_port;/* OpenFlow port number. */
> +
> +/* These members are valid only within bridge_reconfigure(). */
>  const char *type;   /* Usually same as cfg->type. */
>  const struct ovsrec_interface *cfg;
>  };
> @@ -130,13 +117,12 @@ struct bridge {
>  struct hmap ifaces; /* "struct iface"s indexed by ofp_port. */
>  struct hmap iface_by_name;  /* "struct iface"s indexed by name. */
>
> -struct list ofpp_garbage;   /* "struct ofpp_garbage" slated for removal. 
> */
> -struct hmap if_cfg_todo;/* "struct if_cfg"s slated for creation.
> -   Indexed on 'cfg->name'. */
> -
>  /* Port mirroring. */
>  struct hmap mirrors;/* "struct mirror" indexed by UUID. */
>
> +/* Used during reconfiguration. */
> +struct shash wanted_ports;
> +
>  /* Synthetic local port if necessary. */
>  struct ovsrec_port synth_local_port;
>  struct ovsrec_interface synth_local_iface;
> @@ -183,10 +169,8 @@ static long long int iface_stats_timer = LLONG_MIN;
>   * This allows the rest of the code to catch up on important things like
>   * forwarding packets. */
>  #define OFP_PORT_ACTION_WINDOW 10
> -static bool reconfiguring = false;
>
>  static void add_del_bridges(const struct ovsrec_open_vswitch *);
> -static void bridge_update_ofprotos(void);
>  static void bridge_create(const struct ovsrec_bridge *);
>  static void bridge_destroy(struct bridge *);
>  static struct bridge *bridge_lookup(const char *name);
> @@ -194,9 +178,8 @@ static unixctl_cb_func bridge_unixctl_dump_flows;
>  static unixctl_cb_func bridge_unixctl_reconnect;
>  static size_t bridge_get_controllers(const struct bridge *br,
>   struct ovsrec_controller 
> ***controllersp);
> -static void bridge_add_del_ports(struct bridge *,
> - const unsigned long int *splinter_vlans);
> -static void bridge_refresh_ofp_port(struct bridge *);
> +static void bridge_del_ports(struct bridge *,
> + const struct shash *wanted_ports);
>  static void bridge_configure_flow_miss_model(const char *opt);
>  static void bridge_configure_datapath_id(struct bridge *);
>  static void bridge_configure_netflow(struct bridge *);
> @@ -216,9 +199,6 @@ static void bridge_pick_local_hw_addr(struct bridge *,
>  static uint64_t bridge_pick_datapath_id(struct bridge *,
>  const uint8_t 
> bridge_ea[ETH_ADDR_LEN],
>  struct i

Re: [ovs-dev] [PATCH] ofproto-dpif: Ignore non-packet field masks during flow revalidation

[Andy Zhou]

It looks to me this needs to be back ported. But I would like to make sure
the same bug exists on branch-1.11 before applying. What do you think?


On Wed, Dec 11, 2013 at 2:23 PM, Justin Pettit  wrote:

> Did you determine whether it should be backported to branch-1.11?
>
> --Justin
>
>
> On Dec 11, 2013, at 1:34 PM, Andy Zhou  wrote:
>
> > Thanks for the review, pushed to master and branch-2.0 with fixes
> suggested.
> >
> >
> > On Wed, Dec 11, 2013 at 11:48 AM, Justin Pettit 
> wrote:
> > On Dec 11, 2013, at 11:37 AM, Andy Zhou  wrote:
> >
> > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > > index ff77903..8d9a9bd 100644
> > > --- a/ofproto/ofproto-dpif.c
> > > +++ b/ofproto/ofproto-dpif.c
> > > @@ -4219,6 +4219,9 @@ facet_revalidate(struct facet *facet)
> > > xlate_in_init(&xin, ofproto, &facet->flow, new_rule, 0, NULL);
> > > xlate_actions(&xin, &xout);
> > > flow_wildcards_or(&xout.wc, &xout.wc, &wc);
> > > +/* Make sure non packet fields are not masked. If not cleared,
> the memcmp()
> >
> > Nitpicking, but can you make it "non-packet".
> >
> > > + * below may fail, causing otherwise valid facet to be removed. */
> >
> > Can you add an "an" before "otherwise"?
> >
> > Acked-by: Justin Pettit 
> >
> > --Justin
> >
> >
> >
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ofproto-dpif: Ignore non-packet field masks during flow revalidation

[Justin Pettit]

As we discussed off-line, I think you can reproduce this pretty easily with a 
fairly simple flow table.

--Justin


On Dec 11, 2013, at 2:43 PM, Andy Zhou  wrote:

> It looks to me this needs to be back ported. But I would like to make sure 
> the same bug exists on branch-1.11 before applying. What do you think? 
> 
> 
> On Wed, Dec 11, 2013 at 2:23 PM, Justin Pettit  wrote:
> Did you determine whether it should be backported to branch-1.11?
> 
> --Justin
> 
> 
> On Dec 11, 2013, at 1:34 PM, Andy Zhou  wrote:
> 
> > Thanks for the review, pushed to master and branch-2.0 with fixes suggested.
> >
> >
> > On Wed, Dec 11, 2013 at 11:48 AM, Justin Pettit  wrote:
> > On Dec 11, 2013, at 11:37 AM, Andy Zhou  wrote:
> >
> > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > > index ff77903..8d9a9bd 100644
> > > --- a/ofproto/ofproto-dpif.c
> > > +++ b/ofproto/ofproto-dpif.c
> > > @@ -4219,6 +4219,9 @@ facet_revalidate(struct facet *facet)
> > > xlate_in_init(&xin, ofproto, &facet->flow, new_rule, 0, NULL);
> > > xlate_actions(&xin, &xout);
> > > flow_wildcards_or(&xout.wc, &xout.wc, &wc);
> > > +/* Make sure non packet fields are not masked. If not cleared, the 
> > > memcmp()
> >
> > Nitpicking, but can you make it "non-packet".
> >
> > > + * below may fail, causing otherwise valid facet to be removed. */
> >
> > Can you add an "an" before "otherwise"?
> >
> > Acked-by: Justin Pettit 
> >
> > --Justin
> >
> >
> >
> 
> 

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 2/2] string: Use workaround for #include_next when it is not available.

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 configure.ac  |3 +++
 lib/automake.mk   |2 +-
 lib/{string.h => string.h.in} |4 ++--
 3 files changed, 6 insertions(+), 3 deletions(-)
 rename lib/{string.h => string.h.in} (93%)

diff --git a/configure.ac b/configure.ac
index 167cc71..04becad 100644
--- a/configure.ac
+++ b/configure.ac
@@ -92,6 +92,9 @@ OVS_CHECK_ATOMIC_ALWAYS_LOCK_FREE(8)
 OVS_CHECK_POSIX_AIO
 OVS_CHECK_PTHREAD_SET_NAME
 
+OVS_CHECK_INCLUDE_NEXT([string.h])
+AC_CONFIG_FILES([lib/string.h])
+
 OVS_ENABLE_OPTION([-Wall])
 OVS_ENABLE_OPTION([-Wextra])
 OVS_ENABLE_OPTION([-Wno-sign-compare])
diff --git a/lib/automake.mk b/lib/automake.mk
index fadc4be..fef8212 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -192,7 +192,6 @@ lib_libopenvswitch_a_SOURCES = \
lib/stream.c \
lib/stream.h \
lib/string.c \
-   lib/string.h \
lib/svec.c \
lib/svec.h \
lib/table.c \
@@ -230,6 +229,7 @@ lib_libopenvswitch_a_SOURCES = \
lib/vswitch-idl.h \
lib/vtep-idl.c \
lib/vtep-idl.h
+EXTRA_DIST += lib/string.h.in
 
 nodist_lib_libopenvswitch_a_SOURCES = \
lib/dirs.c
diff --git a/lib/string.h b/lib/string.h.in
similarity index 93%
rename from lib/string.h
rename to lib/string.h.in
index 2b7b454..6e2c2ba 100644
--- a/lib/string.h
+++ b/lib/string.h.in
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2011 Nicira, Inc.
+ * Copyright (c) 2009, 2011, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -17,7 +17,7 @@
 #ifndef STRING_WRAPPER_H
 #define STRING_WRAPPER_H 1
 
-#include_next 
+#@INCLUDE_NEXT@ @NEXT_STRING_H@
 
 /* Glibc 2.7 has a bug in strtok_r when compiling with optimization that can
  * cause segfaults if the delimiters argument is a compile-time constant that
-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 1/2] configure: Add macros to check for #include_next and add a workaround.

[Ben Pfaff]

This will be used for the #include_next in string.h in the following
commit.

Signed-off-by: Ben Pfaff 
---
 NOTICE|5 +-
 build-aux/.gitignore  |2 +
 debian/copyright.in   |   10 ++-
 m4/absolute-header.m4 |  102 ++
 m4/include_next.m4|  223 +
 m4/openvswitch.m4 |5 ++
 6 files changed, 345 insertions(+), 2 deletions(-)
 create mode 100644 m4/absolute-header.m4
 create mode 100644 m4/include_next.m4

diff --git a/NOTICE b/NOTICE
index dafd25f..7a3d660 100644
--- a/NOTICE
+++ b/NOTICE
@@ -2,7 +2,7 @@ This file is included in compliance with the Apache 2.0 license,
 available at http://www.apache.org/licenses/LICENSE-2.0.html
 
 Open vSwitch
-Copyright (c) 2007, 2008, 2009, 2010, 2011 Nicira, Inc.
+Copyright (c) 2007, 2008, 2009, 2010, 2011, 2013 Nicira, Inc.
 
 Open vSwitch BSD port
 Copyright (c) 2011 Gaetano Catalli
@@ -19,3 +19,6 @@ Illinois at Urbana-Champaign.
 
 lib/ovs.tmac includes troff macros written by Eric S. Raymond
 and Werner Lemberg.
+
+m4/include_next.m4 and m4/absolute-header.m4
+Copyright (C) 2006-2013 Free Software Foundation, Inc.
diff --git a/build-aux/.gitignore b/build-aux/.gitignore
index 999eae2..3cb4071 100644
--- a/build-aux/.gitignore
+++ b/build-aux/.gitignore
@@ -1,4 +1,6 @@
 /compile
+/config.guess
+/config.sub
 /depcomp
 /install-sh
 /missing
diff --git a/debian/copyright.in b/debian/copyright.in
index 986f7a1..0676387 100644
--- a/debian/copyright.in
+++ b/debian/copyright.in
@@ -8,7 +8,7 @@ Upstream Authors (from AUTHORS):
 
 Upstream Copyright Holders:
 
-   Copyright (c) 2007, 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+   Copyright (c) 2007, 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
Copyright (c) 2010 Jean Tourrilhes - HP-Labs.
Copyright (c) 2008,2009,2010 Citrix Systems, Inc.
and authors listed above.
@@ -182,6 +182,14 @@ License:
 .\" Copyright (C) 2007, 2009, 2011 Free Software Foundation, Inc.
 .\" You may freely use, modify and/or distribute this file.
 
+* m4/absolute-header.m4, by Derek Price, and m4/include_next.m4, by
+  Paul Eggert and Derek Price bear the following notices:
+
+Copyright (C) 2006-2013 Free Software Foundation, Inc.
+This file is free software; the Free Software Foundation
+gives unlimited permission to copy and/or distribute it,
+with or without modifications, as long as this notice is preserved.
+
 * All other components of this package are licensed under
   The Apache License Version 2.0.
 
diff --git a/m4/absolute-header.m4 b/m4/absolute-header.m4
new file mode 100644
index 000..89ff5be
--- /dev/null
+++ b/m4/absolute-header.m4
@@ -0,0 +1,102 @@
+# absolute-header.m4 serial 16
+dnl Copyright (C) 2006-2013 Free Software Foundation, Inc.
+dnl This file is free software; the Free Software Foundation
+dnl gives unlimited permission to copy and/or distribute it,
+dnl with or without modifications, as long as this notice is preserved.
+
+dnl From Derek Price.
+
+# gl_ABSOLUTE_HEADER(HEADER1 HEADER2 ...)
+# ---
+# Find the absolute name of a header file, testing first if the header exists.
+# If the header were sys/inttypes.h, this macro would define
+# ABSOLUTE_SYS_INTTYPES_H to the '""' quoted absolute name of sys/inttypes.h
+# in config.h
+# (e.g. '#define ABSOLUTE_SYS_INTTYPES_H "///usr/include/sys/inttypes.h"').
+# The three "///" are to pacify Sun C 5.8, which otherwise would say
+# "warning: #include of /usr/include/... may be non-portable".
+# Use '""', not '<>', so that the /// cannot be confused with a C99 comment.
+# Note: This macro assumes that the header file is not empty after
+# preprocessing, i.e. it does not only define preprocessor macros but also
+# provides some type/enum definitions or function/variable declarations.
+AC_DEFUN([gl_ABSOLUTE_HEADER],
+[AC_REQUIRE([AC_CANONICAL_HOST])
+AC_LANG_PREPROC_REQUIRE()dnl
+dnl FIXME: gl_absolute_header and ac_header_exists must be used unquoted
+dnl until we can assume autoconf 2.64 or newer.
+m4_foreach_w([gl_HEADER_NAME], [$1],
+  [AS_VAR_PUSHDEF([gl_absolute_header],
+  [gl_cv_absolute_]m4_defn([gl_HEADER_NAME]))dnl
+  AC_CACHE_CHECK([absolute name of <]m4_defn([gl_HEADER_NAME])[>],
+m4_defn([gl_absolute_header]),
+[AS_VAR_PUSHDEF([ac_header_exists],
+[ac_cv_header_]m4_defn([gl_HEADER_NAME]))dnl
+AC_CHECK_HEADERS_ONCE(m4_defn([gl_HEADER_NAME]))dnl
+if test AS_VAR_GET(ac_header_exists) = yes; then
+  gl_ABSOLUTE_HEADER_ONE(m4_defn([gl_HEADER_NAME]))
+fi
+AS_VAR_POPDEF([ac_header_exists])dnl
+])dnl
+  AC_DEFINE_UNQUOTED(AS_TR_CPP([ABSOLUTE_]m4_defn([gl_HEADER_NAME])),
+ ["AS_VAR_GET(gl_absolute_header)"],
+ [Define this to an absolute name of 
<]m4_defn([gl_HEADER_NAME])[>.])
+  AS_VAR_POPDEF([gl_absolute_header])dnl
+])dnl
+])# gl_ABSOLUTE_HEADER
+
+# gl_ABSOLUTE_HEADER_ONE(HEAD

Re: [ovs-dev] [PATCH] lib: Bypass include_next preprocessor directives on Windows platform

[Ben Pfaff]

On Mon, Dec 09, 2013 at 08:34:25PM +, Alin Serdean wrote:
> I would leave it up too you. An easier solution would be to put in
> the Vstudio includes first(in the case for cccl) because there is no
> need for the string.h wrapper.

I sent out a two-patch series, will you try it out?
http://openvswitch.org/pipermail/dev/2013-December/034910.html
http://openvswitch.org/pipermail/dev/2013-December/034909.html
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [port-renumbering 7/8] bridge: Reconfigure in single pass.

[Ben Pfaff]

On Wed, Dec 11, 2013 at 02:17:13PM -0800, Jarno Rajahalme wrote:
> While testing the code I accidentally requested a change in the interface 
> type (to dummy), like this:
> 
> # ovs-vsctl add-br br0
> # ovs-vsctl add-port br0 p1 -- set Interface p1 type=internal
> # ovs-vsctl -- set Interface p1 type=dummy
> 
> Running OVS without dummy enabled seg faults:

I couldn't reproduce this, can you try to get me a backtrace or run
under valgrind?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] lib/util: Only define count_1bits_8 when needed.

[Jarno Rajahalme]

Ben,

I like this, I’ll post a new patch ASAP,

  Jarno

On Dec 11, 2013, at 11:33 AM, Ben Pfaff  wrote:

> On Wed, Dec 11, 2013 at 10:54:00AM -0800, Jarno Rajahalme wrote:
>> util.h declares this when needed, make sure the definition is compiled
>> in only in that case.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> With this, I still get:
>../lib/util.c:921:15: warning: symbol 'count_1bits_8' was not 
> declared. Should it be static?
> because the previous declaration of count_1bits_8[] is not in scope at
> the point of definition in util.c.
> 
> How about something like this?  (Not tested outside of 32-bit without
> __POPCNT__.)
> 
> diff --git a/lib/util.c b/lib/util.c
> index 13d41a7..000504c 100644
> --- a/lib/util.c
> +++ b/lib/util.c
> @@ -901,7 +901,7 @@ raw_clz64(uint64_t n)
> }
> #endif
> 
> -#if !(__GNUC__ >= 4 && defined(__corei7))
> +#if NEED_COUNT_1BITS_8
> #define INIT1(X)\
> X) & (1 << 0)) != 0) +  \
>  (((X) & (1 << 1)) != 0) +  \
> diff --git a/lib/util.h b/lib/util.h
> index 8d810c2..b158c2f 100644
> --- a/lib/util.h
> +++ b/lib/util.h
> @@ -371,49 +371,55 @@ log_2_ceil(uint64_t n)
> return log_2_floor(n) + !is_pow2(n);
> }
> 
> -extern const uint8_t count_1bits_8[256];
> -
> -/* Returns the number of 1-bits in 'x', between 0 and 32 inclusive. */
> +/* unsigned int count_1bits(uint64_t x):
> + *
> + * Returns the number of 1-bits in 'x', between 0 and 64 inclusive. */
> +#if UINTPTR_MAX == UINT64_MAX
> +static inline unsigned int
> +count_1bits(uint64_t x)
> +{
> +#if __GNUC__ >= 4 && __POPCNT__
> +return __builtin_popcountll(x);
> +#else
> +/* This portable implementation is the fastest one we know of for 64
> + * bits, and about 3x faster than GCC 4.7 __builtin_popcountll(). */
> +const uint64_t h55 = UINT64_C(0x);
> +const uint64_t h33 = UINT64_C(0x);
> +const uint64_t h0F = UINT64_C(0x0F0F0F0F0F0F0F0F);
> +const uint64_t h01 = UINT64_C(0x0101010101010101);
> +x -= (x >> 1) & h55;   /* Count of each 2 bits in-place. */
> +x = (x & h33) + ((x >> 2) & h33);  /* Count of each 4 bits in-place. */
> +x = (x + (x >> 4)) & h0F;  /* Count of each 8 bits in-place. */
> +return (x * h01) >> 56;/* Sum of all bytes. */
> +#endif
> +}
> +#else  /* not 64-bit */
> +#if __GNUC__ >= 4 && __POPCNT__
> static inline unsigned int
> count_1bits_32(uint32_t x)
> {
> -#if __GNUC__ >= 4 && defined(__corei7)
> -/* __builtin_popcount() is fast only when supported by the CPU. */
> return __builtin_popcount(x);
> +}
> #else
> +#define NEED_COUNT_1BITS_8 1
> +extern const uint8_t count_1bits_8[256];
> +static inline unsigned int
> +count_1bits_32(uint32_t x)
> +{
> /* This portable implementation is the fastest one we know of for 32 bits,
>  * and faster than GCC __builtin_popcount(). */
> return (count_1bits_8[x & 0xff] +
> count_1bits_8[(x >> 8) & 0xff] +
> count_1bits_8[(x >> 16) & 0xff] +
> count_1bits_8[x >> 24]);
> -#endif
> }
> -
> -/* Returns the number of 1-bits in 'x', between 0 and 64 inclusive. */
> +#endif
> static inline unsigned int
> count_1bits(uint64_t x)
> {
> -if (sizeof(void *) == 8) { /* 64-bit CPU */
> -#if __GNUC__ >= 4 && defined(__corei7)
> -/* __builtin_popcountll() is fast only when supported by the CPU. */
> -return __builtin_popcountll(x);
> -#else
> -/* This portable implementation is the fastest one we know of for 64
> - * bits, and about 3x faster than GCC 4.7 __builtin_popcountll(). */
> -const uint64_t h55 = UINT64_C(0x);
> -const uint64_t h33 = UINT64_C(0x);
> -const uint64_t h0F = UINT64_C(0x0F0F0F0F0F0F0F0F);
> -const uint64_t h01 = UINT64_C(0x0101010101010101);
> -x -= (x >> 1) & h55;   /* Count of each 2 bits in-place. 
> */
> -x = (x & h33) + ((x >> 2) & h33);  /* Count of each 4 bits in-place. 
> */
> -x = (x + (x >> 4)) & h0F;  /* Count of each 8 bits in-place. 
> */
> -return (x * h01) >> 56;/* Sum of all bytes. */
> -#endif
> -} else { /* 32-bit CPU */
> -return count_1bits_32(x) + count_1bits_32(x >> 32);
> -}
> +return count_1bits_32(x) + count_1bits_32(x >> 32);
> }
> +#endif
> 
> /* Returns the rightmost 1-bit in 'x' (e.g. 01011000 => 1000), or 0 if 'x'
>  * is 0. */

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [port-renumbering 7/8] bridge: Reconfigure in single pass.

[Ben Pfaff]

On Wed, Dec 11, 2013 at 01:27:25PM -0800, Jarno Rajahalme wrote:
> 
> On Dec 10, 2013, at 11:20 PM, Ben Pfaff  wrote:
> 
> > This greatly simplifies the reconfiguration code, making it much easier
> > to understand and modify.  The old multi-pass configuration had the
> > property that it didn't delay block packet processing as much, but that's
> > not much of a worry anymore now that latency critical activities have
> > been moved outside the main thread.
> > 
> 
> You?ll find two minor comments below, but otherwise seems good to
> me. I have no history with this code, though, so with that caveat:
> 
> Acked-by: Jarno Rajahalme 

This commit is really unpolished because I posted it when I was really
tired.  Sorry about that.

I folded in the following, which helps I think.  There is probably
still room for better function naming, at the very least.

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index fcf7efb..ed16860 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -178,8 +178,16 @@ static unixctl_cb_func bridge_unixctl_dump_flows;
 static unixctl_cb_func bridge_unixctl_reconnect;
 static size_t bridge_get_controllers(const struct bridge *br,
  struct ovsrec_controller ***controllersp);
+static void bridge_collect_wanted_ports(struct bridge *,
+const unsigned long *splinter_vlans,
+struct shash *wanted_ports);
+static void bridge_delete_ofprotos(void);
+static void bridge_delete_or_reconfigure_ports(struct bridge *);
 static void bridge_del_ports(struct bridge *,
  const struct shash *wanted_ports);
+static void bridge_add_ports(struct bridge *,
+ const struct shash *wanted_ports);
+
 static void bridge_configure_flow_miss_model(const char *opt);
 static void bridge_configure_datapath_id(struct bridge *);
 static void bridge_configure_netflow(struct bridge *);
@@ -466,14 +474,6 @@ collect_in_band_managers(const struct ovsrec_open_vswitch 
*ovs_cfg,
 *n_managersp = n_managers;
 }
 
-static void bridge_collect_wanted_ports(struct bridge *,
-const unsigned long *splinter_vlans,
-struct shash *wanted_ports);
-static void bridge_delete_ofprotos(void);
-static void bridge_delete_ports(struct bridge *);
-static void bridge_add_ports(struct bridge *,
- const struct shash *wanted_ports);
-
 static void
 bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
 {
@@ -508,17 +508,30 @@ bridge_reconfigure(const struct ovsrec_open_vswitch 
*ovs_cfg)
 }
 free(splinter_vlans);
 
-/* Delete datapaths and ports that are no longer configured.  Attempt to
- * reconfigure existing ports to their desired configurations, or delete
- * them if not possible. */
+/* Start pushing configuration changes down to the ofproto layer:
+ *
+ *   - Delete ofprotos that are no longer configured.
+ *
+ *   - Delete ports that are no longer configured.
+ *
+ *   - Reconfigure existing ports to their desired configurations, or
+ * delete them if not possible.
+ *
+ * We have to do all the deletions before we can do any additions, because
+ * the ports to be added might require resources that will be freed up by
+ * deletions (they might especially overlap in name). */
 bridge_delete_ofprotos();
 HMAP_FOR_EACH (br, node, &all_bridges) {
 if (br->ofproto) {
-bridge_delete_ports(br);
+bridge_delete_or_reconfigure_ports(br);
 }
 }
 
-/* Add new ofprotos and ports. */
+/* Finish pushing configuration changes to the ofproto layer:
+ *
+ * - Create ofprotos that are missing.
+ *
+ * - Add ports that are missing. */
 HMAP_FOR_EACH_SAFE (br, next, node, &all_bridges) {
 if (!br->ofproto) {
 int error;
@@ -623,11 +636,14 @@ add_ofp_port(ofp_port_t port, ofp_port_t *ports, size_t 
*n, size_t *allocated)
 }
 
 static void
-bridge_delete_ports(struct bridge *br)
+bridge_delete_or_reconfigure_ports(struct bridge *br)
 {
 struct ofproto_port ofproto_port;
 struct ofproto_port_dump dump;
 
+/* List of "ofp_port"s to delete.  We make a list instead of deleting them
+ * right away because ofproto implementations aren't necessarily able to
+ * iterate through a changing list of ports in an entirely robust way. */
 ofp_port_t *del;
 size_t n, allocated;
 size_t i;
@@ -654,9 +670,12 @@ bridge_delete_ports(struct bridge *br)
 
 if (strcmp(ofproto_port.type, iface->type)
 || netdev_set_config(iface->netdev, &iface->cfg->options)) {
+/* The interface is the wrong type or can't be configured.
+ * Delete it. */
 goto delete;
 }
 
+/* Keep it. */
 continue;
 
 delete:
@@ -2630,7 +2

[ovs-dev] [PATCH] lib/util: More portable use of builtin popcnt.

[Jarno Rajahalme]

- Use the GCC predefined macro __POPCNT__ to detect the availability
  of fast __builtin_popcnt function.
- Use portable preprocessor macros to detect 64-bit build.
- Only define the 32-bit parts when needed and declare the
  count_1bits_8 at file scope to silence a warning.

This time I have tested all code paths to make sure no warnigns are
generated.

Signed-off-by: Jarno Rajahalme 
---
 lib/util.c |2 +-
 lib/util.h |   62 +---
 2 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/lib/util.c b/lib/util.c
index 13d41a7..000504c 100644
--- a/lib/util.c
+++ b/lib/util.c
@@ -901,7 +901,7 @@ raw_clz64(uint64_t n)
 }
 #endif
 
-#if !(__GNUC__ >= 4 && defined(__corei7))
+#if NEED_COUNT_1BITS_8
 #define INIT1(X)\
 X) & (1 << 0)) != 0) +  \
  (((X) & (1 << 1)) != 0) +  \
diff --git a/lib/util.h b/lib/util.h
index 8d810c2..0327ab0 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -371,49 +371,55 @@ log_2_ceil(uint64_t n)
 return log_2_floor(n) + !is_pow2(n);
 }
 
-extern const uint8_t count_1bits_8[256];
-
-/* Returns the number of 1-bits in 'x', between 0 and 32 inclusive. */
+/* unsigned int count_1bits(uint64_t x):
+ *
+ * Returns the number of 1-bits in 'x', between 0 and 64 inclusive. */
+#if UINTPTR_MAX == UINT64_MAX
+static inline unsigned int
+count_1bits(uint64_t x)
+{
+#if __GNUC__ >= 4 && __POPCNT__
+return __builtin_popcountll(x);
+#else
+/* This portable implementation is the fastest one we know of for 64
+ * bits, and about 3x faster than GCC 4.7 __builtin_popcountll(). */
+const uint64_t h55 = UINT64_C(0x);
+const uint64_t h33 = UINT64_C(0x);
+const uint64_t h0F = UINT64_C(0x0F0F0F0F0F0F0F0F);
+const uint64_t h01 = UINT64_C(0x0101010101010101);
+x -= (x >> 1) & h55;   /* Count of each 2 bits in-place. */
+x = (x & h33) + ((x >> 2) & h33);  /* Count of each 4 bits in-place. */
+x = (x + (x >> 4)) & h0F;  /* Count of each 8 bits in-place. */
+return (x * h01) >> 56;/* Sum of all bytes. */
+#endif
+}
+#else /* Not 64-bit. */
+#if __GNUC__ >= 4 && __POPCNT__
 static inline unsigned int
-count_1bits_32(uint32_t x)
+count_1bits_32__(uint32_t x)
 {
-#if __GNUC__ >= 4 && defined(__corei7)
-/* __builtin_popcount() is fast only when supported by the CPU. */
 return __builtin_popcount(x);
+}
 #else
+#define NEED_COUNT_1BITS_8 1
+extern const uint8_t count_1bits_8[256];
+static inline unsigned int
+count_1bits_32__(uint32_t x)
+{
 /* This portable implementation is the fastest one we know of for 32 bits,
  * and faster than GCC __builtin_popcount(). */
 return (count_1bits_8[x & 0xff] +
 count_1bits_8[(x >> 8) & 0xff] +
 count_1bits_8[(x >> 16) & 0xff] +
 count_1bits_8[x >> 24]);
-#endif
 }
-
-/* Returns the number of 1-bits in 'x', between 0 and 64 inclusive. */
+#endif
 static inline unsigned int
 count_1bits(uint64_t x)
 {
-if (sizeof(void *) == 8) { /* 64-bit CPU */
-#if __GNUC__ >= 4 && defined(__corei7)
-/* __builtin_popcountll() is fast only when supported by the CPU. */
-return __builtin_popcountll(x);
-#else
-/* This portable implementation is the fastest one we know of for 64
- * bits, and about 3x faster than GCC 4.7 __builtin_popcountll(). */
-const uint64_t h55 = UINT64_C(0x);
-const uint64_t h33 = UINT64_C(0x);
-const uint64_t h0F = UINT64_C(0x0F0F0F0F0F0F0F0F);
-const uint64_t h01 = UINT64_C(0x0101010101010101);
-x -= (x >> 1) & h55;   /* Count of each 2 bits in-place. */
-x = (x & h33) + ((x >> 2) & h33);  /* Count of each 4 bits in-place. */
-x = (x + (x >> 4)) & h0F;  /* Count of each 8 bits in-place. */
-return (x * h01) >> 56;/* Sum of all bytes. */
-#endif
-} else { /* 32-bit CPU */
-return count_1bits_32(x) + count_1bits_32(x >> 32);
-}
+return count_1bits_32__(x) + count_1bits_32__(x >> 32);
 }
+#endif
 
 /* Returns the rightmost 1-bit in 'x' (e.g. 01011000 => 1000), or 0 if 'x'
  * is 0. */
-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] lib: Determine cpu core count with /proc/cpuinfo

[Joe Stringer]

Thanks Ed, it should be fixed on master now.

On 11 December 2013 13:47, Joe Stringer  wrote:
> I'm pretty sure that we only care about this warning if we are
> specifically investigating how we calculate the number of threads to
> use.
>
> I've sent a patch below to reduce this to debug logging:-
> http://openvswitch.org/pipermail/dev/2013-December/034899.html
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [port-renumbering 7/8] bridge: Reconfigure in single pass.

[Jarno Rajahalme]

On Dec 11, 2013, at 3:14 PM, Ben Pfaff  wrote:

> On Wed, Dec 11, 2013 at 02:17:13PM -0800, Jarno Rajahalme wrote:
>> While testing the code I accidentally requested a change in the interface 
>> type (to dummy), like this:
>> 
>> # ovs-vsctl add-br br0
>> # ovs-vsctl add-port br0 p1 -- set Interface p1 type=internal
>> # ovs-vsctl -- set Interface p1 type=dummy
>> 
>> Running OVS without dummy enabled seg faults:
> 
> I couldn't reproduce this, can you try to get me a backtrace or run
> under valgrind?


 I also upped logging to dbg:

2013-12-12T00:03:27Z|00309|netlink_socket|DBG|nl_sock_transact_multiple__ 
(Success): nl(len:40, type=196(ovs_vport), flags=9[REQUEST][ECHO], seq=35, 
pid=33664,genl(cmd=3,version=1)
2013-12-12T00:03:27Z|00310|netlink_socket|DBG|nl_sock_recv__ (Success): 
nl(len:124, type=196(ovs_vport), flags=0, seq=35, 
pid=33664,genl(cmd=1,version=1)
2013-12-12T00:03:27Z|00311|dpif|DBG|system@ovs-system: device br0 is on port 1
2013-12-12T00:03:27Z|00312|netlink_socket|DBG|nl_sock_transact_multiple__ 
(Success): nl(len:40, type=196(ovs_vport), flags=9[REQUEST][ECHO], seq=36, 
pid=33664,genl(cmd=3,version=1)
2013-12-12T00:03:27Z|00313|netlink_socket|DBG|nl_sock_recv__ (Success): 
nl(len:124, type=196(ovs_vport), flags=0, seq=36, 
pid=33664,genl(cmd=1,version=1)
2013-12-12T00:03:27Z|00314|dpif|DBG|system@ovs-system: device p1 is on port 2
2013-12-12T00:03:27Z|00315|netlink_socket|DBG|nl_sock_transact_multiple__ 
(Success): nl(len:32, type=196(ovs_vport), flags=9[REQUEST][ECHO], seq=37, 
pid=33664,genl(cmd=2,version=1)
2013-12-12T00:03:27Z|00316|netlink_socket|DBG|nl_sock_recv__ (Success): 
nl(len:124, type=196(ovs_vport), flags=0, seq=37, 
pid=33664,genl(cmd=2,version=1)
2013-12-12T00:03:27Z|00317|dpif|DBG|system@ovs-system: port_del(2)
2013-12-12T00:03:27Z|00318|netlink_socket|DBG|nl_sock_transact_multiple__ 
(Success): nl(len:40, type=196(ovs_vport), flags=9[REQUEST][ECHO], seq=38, 
pid=33664,genl(cmd=3,version=1)
2013-12-12T00:03:27Z|00319|netlink_socket|DBG|nl_sock_recv__ (Success): 
nl(len:60, type=2(error), flags=0, seq=38, pid=33664 error(-19(No such device), 
in-reply-to(nl(len:40, type=196(ovs_vport), flags=9[REQUEST][ECHO], seq=38, 
pid=33664))
2013-12-12T00:03:27Z|00320|netlink_socket|DBG|received NAK error=0 (No such 
device)
2013-12-12T00:03:27Z|00321|dpif|DBG|system@ovs-system: failed to query port p1: 
No such device
2013-12-12T00:03:27Z|00322|netlink_socket|DBG|nl_sock_transact_multiple__ 
(Success): nl(len:40, type=196(ovs_vport), flags=9[REQUEST][ECHO], seq=39, 
pid=33664,genl(cmd=3,version=1)
2013-12-12T00:03:27Z|00323|netlink_socket|DBG|nl_sock_recv__ (Success): 
nl(len:60, type=2(error), flags=0, seq=39, pid=33664 error(-19(No such device), 
in-reply-to(nl(len:40, type=196(ovs_vport), flags=9[REQUEST][ECHO], seq=39, 
pid=33664))
2013-12-12T00:03:27Z|00324|netlink_socket|DBG|received NAK error=0 (No such 
device)
2013-12-12T00:03:27Z|00325|netdev|WARN|could not create netdev p1 of unknown 
type dummy
2013-12-12T00:03:27Z|00326|bridge|WARN|could not open network device p1 
(Address family not supported by protocol)
==33664== Invalid read of size 2
==33664==at 0x40A1C2: bridge_reconfigure (bridge.c:1568)
==33664==by 0x406BAC: bridge_run (bridge.c:2289)
==33664==by 0x40E498: main (ovs-vswitchd.c:118)
==33664==  Address 0x48 is not stack'd, malloc'd or (recently) free'd
==33664== 
==33664== 
==33664== Process terminating with default action of signal 11 (SIGSEGV)
==33664==  Access not within mapped region at address 0x48
==33664==at 0x40A1C2: bridge_reconfigure (bridge.c:1568)
==33664==by 0x406BAC: bridge_run (bridge.c:2289)
==33664==by 0x40E498: main (ovs-vswitchd.c:118)
==33664==  If you believe this happened as a result of a stack
==33664==  overflow in your program's main thread (unlikely but
==33664==  possible), you can try to increase the size of the
==33664==  main thread stack using the --main-stacksize= flag.
==33664==  The main thread stack size used in this run was 8388608.
==33664== 
==33664== HEAP SUMMARY:
==33664== in use at exit: 125,133 bytes in 858 blocks
==33664==   total heap usage: 14,036 allocs, 13,178 frees, 4,882,832 bytes 
allocated
==33664== 
==33664== LEAK SUMMARY:
==33664==definitely lost: 40 bytes in 1 blocks
==33664==indirectly lost: 0 bytes in 0 blocks
==33664==  possibly lost: 7,792 bytes in 18 blocks
==33664==still reachable: 117,301 bytes in 839 blocks
==33664== suppressed: 0 bytes in 0 blocks
==33664== Rerun with --leak-check=full to see details of leaked memory
==33664== 
==33664== For counts of detected and suppressed errors, rerun with: -v
==33664== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 4 from 4)
./start_ovs.sh: line 6: 33664 Killed  valgrind ovs-vswitchd 
--pidfile -vdbg

The offending line is:

if (iface->ofp_port == OFPP_LOCAL) {

Seems like iface is NULL (if offset of odp_port == 0x48) in this case.

  Jarno

__

[ovs-dev] [PATCH] ovs-thread: Break recursion for coverage counters.

[Gurucharan Shetty]

For systems that do not use linker sections and also do not
have either HAVE_THREAD_LOCAL or HAVE___THREAD (ex: windows
using MSVC), a COVERAGE_INC() calls xmalloc which inturn calls
COVERAGE_INC() creating a recursion that causes a stack overflow.

This commit breaks the recursion by calling malloc() instead of
xmalloc()

Signed-off-by: Gurucharan Shetty 
---
 lib/ovs-thread.h |5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index 7f3195d..8de0f27 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -313,7 +313,10 @@ void xpthread_join(pthread_t, void **);
 if (!value) {   \
 static const NAME##_type initial_value = __VA_ARGS__;   \
 \
-value = xmalloc(sizeof *value); \
+value = malloc(sizeof *value);  \
+if (value == NULL) {\
+out_of_memory();\
+}   \
 *value = initial_value; \
 xpthread_setspecific(NAME##_key, value);\
 }   \
-- 
1.7.9.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH RFC] netdev-linux: Read packet auxdata to obtain vlan_tid

[Simon Horman]

On Wed, Dec 11, 2013 at 09:31:49AM -0800, Ben Pfaff wrote:
> On Wed, Dec 11, 2013 at 11:24:14AM +0900, Simon Horman wrote:
> > If VLAN acceleration is used when the kernel receives a packet
> > then the outer-most VLAN tag will not be present in the packet
> > when it is received by netdev-linux. Rather, it will be present
> > in auxdata.
> > 
> > This patch uses recvmsg() instead of recv() to read auxdata for
> > each packet and if the vlan_tid is set then it is added to the packet.
> > 
> > Adding the vlan_tid to the packet involves copying most of the packet
> > and may be rather expensive. There is ample scope to avoid this by
> > passing the vlan_tid back to the caller separately to the packet itself
> > or providing access headroom in the packet. This would most likely
> > involve updating the netdev-class API.
> > 
> > Signed-off-by: Simon Horman 
> 
> Thanks for doing this.
> 
> I think that we should change netdev_class to pass an ofpbuf into
> rx_recv.  Then we can just ofpbuf_reserve() VLAN_HEADER_LEN bytes
> before doing the receive, and just use eth_push_vlan() to insert the
> VLAN header.

Thanks, I was entirely unsure about how you would want the API changed.
I'll see about implementing your idea.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH RFC] ofproto: Allow multiple array elements in PORT_DESC request body

[Simon Horman]

Signed-off-by: Simon Horman 

---

This change is also needed for several other stats request messages.
I have posted this as an RFC before proceeding with updating
the decoders for other stats request messages.
---
 lib/ofp-msgs.h|  4 ++--
 lib/ofp-print.c   | 23 +++
 lib/ofp-util.c| 54 +++---
 lib/ofp-util.h|  4 ++--
 ofproto/ofproto.c | 38 --
 tests/ofproto.at  | 36 
 6 files changed, 126 insertions(+), 33 deletions(-)

diff --git a/lib/ofp-msgs.h b/lib/ofp-msgs.h
index 26fd6a3..ea4b37d 100644
--- a/lib/ofp-msgs.h
+++ b/lib/ofp-msgs.h
@@ -281,9 +281,9 @@ enum ofpraw {
 /* OFPST 1.3 (3): struct ofp13_table_stats[]. */
 OFPRAW_OFPST13_TABLE_REPLY,
 
-/* OFPST 1.0 (4): struct ofp10_port_stats_request. */
+/* OFPST 1.0 (4): struct ofp10_port_stats_request[]. */
 OFPRAW_OFPST10_PORT_REQUEST,
-/* OFPST 1.1+ (4): struct ofp11_port_stats_request. */
+/* OFPST 1.1+ (4): struct ofp11_port_stats_request[]. */
 OFPRAW_OFPST11_PORT_REQUEST,
 
 /* OFPST 1.0 (4): struct ofp10_port_stats[]. */
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 13705d0..ffeaf12 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -1515,16 +1515,23 @@ static void
 ofp_print_ofpst_port_request(struct ds *string, const struct ofp_header *oh)
 {
 ofp_port_t ofp10_port;
-enum ofperr error;
+struct ofpbuf b;
 
-error = ofputil_decode_port_stats_request(oh, &ofp10_port);
-if (error) {
-ofp_print_error(string, error);
-return;
-}
+ofpbuf_use_const(&b, oh, ntohs(oh->length));
+for (;;) {
+int error;
 
-ds_put_cstr(string, " port_no=");
-ofputil_format_port(ofp10_port, string);
+error = ofputil_decode_port_stats_request(&b, &ofp10_port);
+if (error) {
+if (error != EOF) {
+ofp_print_error(string, error);
+}
+return;
+}
+
+ds_put_cstr(string, " port_no=");
+ofputil_format_port(ofp10_port, string);
+}
 }
 
 static void
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 7fc4c7c..a7c3769 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -5739,23 +5739,58 @@ ofputil_decode_port_stats(struct ofputil_port_stats 
*ps, struct ofpbuf *msg)
 return OFPERR_OFPBRC_BAD_LEN;
 }
 
-/* Parse a port status request message into a 16 bit OpenFlow 1.0
+/* Converts an OFPST_PORT_DESC 'msg' into a 16 bit OpenFlow 1.0
  * port number and stores the latter in '*ofp10_port'.
- * Returns 0 if successful, otherwise an OFPERR_* number. */
-enum ofperr
-ofputil_decode_port_stats_request(const struct ofp_header *request,
+ *
+ * Multiple OFPST_PORT_DESC requests can be packed into a single
+ * OpenFlow message.  Calling this function multiple times for a single 'msg'
+ * iterates through the replies.  The caller must initially leave 'msg''s layer
+ * pointers null and not modify them between calls.
+ *
+ * Returns 0 if successful, EOF if no replies were left in this 'msg',
+ * otherwise an OFPERR_* number. */
+int
+ofputil_decode_port_stats_request(struct ofpbuf *msg,
   ofp_port_t *ofp10_port)
 {
-switch ((enum ofp_version)request->version) {
+const struct ofp_header *oh;
+enum ofperr error;
+enum ofpraw raw;
+
+error = (msg->l2
+ ? ofpraw_decode(&raw, msg->l2)
+ : ofpraw_pull(&raw, msg));
+if (error) {
+return error;
+}
+oh = msg->l2;
+
+if (!msg->size) {
+return EOF;
+}
+
+switch ((enum ofp_version)oh->version) {
 case OFP13_VERSION:
 case OFP12_VERSION:
 case OFP11_VERSION: {
-const struct ofp11_port_stats_request *psr11 = ofpmsg_body(request);
+const struct ofp11_port_stats_request *psr11;
+
+psr11 = ofpbuf_try_pull(msg, sizeof *psr11);
+if (!psr11) {
+goto trailing_garbage;
+}
+
 return ofputil_port_from_ofp11(psr11->port_no, ofp10_port);
 }
 
 case OFP10_VERSION: {
-const struct ofp10_port_stats_request *psr10 = ofpmsg_body(request);
+const struct ofp10_port_stats_request *psr10;
+
+psr10 = ofpbuf_try_pull(msg, sizeof *psr10);
+if (!psr10) {
+goto trailing_garbage;
+}
+
 *ofp10_port = u16_to_ofp(ntohs(psr10->port_no));
 return 0;
 }
@@ -5763,6 +5798,11 @@ ofputil_decode_port_stats_request(const struct 
ofp_header *request,
 default:
 NOT_REACHED();
 }
+
+trailing_garbage:
+VLOG_WARN_RL(&bad_ofmsg_rl, "OFPST_PORT_DESC request has "
+ "%"PRIuSIZE" leftover bytes at end", msg->size);
+return OFPERR_OFPBRC_BAD_LEN;
 }
 
 /* Frees all of the "struct ofputil_bucket"s in the 'buckets' list. */
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index fef85e0..726cd3e 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -918,8 +918,8

[ovs-dev] [PATCH RFC] ofproto: Allow multiple array elements in PORT_DESC request body

[Simon Horman]

Signed-off-by: Simon Horman 

---

This change is also needed for several other stats request messages.
I have posted this as an RFC before proceeding with updating
the decoders for other stats request messages.
---
 lib/ofp-msgs.h|  4 ++--
 lib/ofp-print.c   | 23 +++
 lib/ofp-util.c| 54 +++---
 lib/ofp-util.h|  4 ++--
 ofproto/ofproto.c | 38 --
 tests/ofproto.at  | 36 
 6 files changed, 126 insertions(+), 33 deletions(-)

diff --git a/lib/ofp-msgs.h b/lib/ofp-msgs.h
index 26fd6a3..ea4b37d 100644
--- a/lib/ofp-msgs.h
+++ b/lib/ofp-msgs.h
@@ -281,9 +281,9 @@ enum ofpraw {
 /* OFPST 1.3 (3): struct ofp13_table_stats[]. */
 OFPRAW_OFPST13_TABLE_REPLY,
 
-/* OFPST 1.0 (4): struct ofp10_port_stats_request. */
+/* OFPST 1.0 (4): struct ofp10_port_stats_request[]. */
 OFPRAW_OFPST10_PORT_REQUEST,
-/* OFPST 1.1+ (4): struct ofp11_port_stats_request. */
+/* OFPST 1.1+ (4): struct ofp11_port_stats_request[]. */
 OFPRAW_OFPST11_PORT_REQUEST,
 
 /* OFPST 1.0 (4): struct ofp10_port_stats[]. */
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 13705d0..ffeaf12 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -1515,16 +1515,23 @@ static void
 ofp_print_ofpst_port_request(struct ds *string, const struct ofp_header *oh)
 {
 ofp_port_t ofp10_port;
-enum ofperr error;
+struct ofpbuf b;
 
-error = ofputil_decode_port_stats_request(oh, &ofp10_port);
-if (error) {
-ofp_print_error(string, error);
-return;
-}
+ofpbuf_use_const(&b, oh, ntohs(oh->length));
+for (;;) {
+int error;
 
-ds_put_cstr(string, " port_no=");
-ofputil_format_port(ofp10_port, string);
+error = ofputil_decode_port_stats_request(&b, &ofp10_port);
+if (error) {
+if (error != EOF) {
+ofp_print_error(string, error);
+}
+return;
+}
+
+ds_put_cstr(string, " port_no=");
+ofputil_format_port(ofp10_port, string);
+}
 }
 
 static void
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 7fc4c7c..a7c3769 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -5739,23 +5739,58 @@ ofputil_decode_port_stats(struct ofputil_port_stats 
*ps, struct ofpbuf *msg)
 return OFPERR_OFPBRC_BAD_LEN;
 }
 
-/* Parse a port status request message into a 16 bit OpenFlow 1.0
+/* Converts an OFPST_PORT_DESC 'msg' into a 16 bit OpenFlow 1.0
  * port number and stores the latter in '*ofp10_port'.
- * Returns 0 if successful, otherwise an OFPERR_* number. */
-enum ofperr
-ofputil_decode_port_stats_request(const struct ofp_header *request,
+ *
+ * Multiple OFPST_PORT_DESC requests can be packed into a single
+ * OpenFlow message.  Calling this function multiple times for a single 'msg'
+ * iterates through the replies.  The caller must initially leave 'msg''s layer
+ * pointers null and not modify them between calls.
+ *
+ * Returns 0 if successful, EOF if no replies were left in this 'msg',
+ * otherwise an OFPERR_* number. */
+int
+ofputil_decode_port_stats_request(struct ofpbuf *msg,
   ofp_port_t *ofp10_port)
 {
-switch ((enum ofp_version)request->version) {
+const struct ofp_header *oh;
+enum ofperr error;
+enum ofpraw raw;
+
+error = (msg->l2
+ ? ofpraw_decode(&raw, msg->l2)
+ : ofpraw_pull(&raw, msg));
+if (error) {
+return error;
+}
+oh = msg->l2;
+
+if (!msg->size) {
+return EOF;
+}
+
+switch ((enum ofp_version)oh->version) {
 case OFP13_VERSION:
 case OFP12_VERSION:
 case OFP11_VERSION: {
-const struct ofp11_port_stats_request *psr11 = ofpmsg_body(request);
+const struct ofp11_port_stats_request *psr11;
+
+psr11 = ofpbuf_try_pull(msg, sizeof *psr11);
+if (!psr11) {
+goto trailing_garbage;
+}
+
 return ofputil_port_from_ofp11(psr11->port_no, ofp10_port);
 }
 
 case OFP10_VERSION: {
-const struct ofp10_port_stats_request *psr10 = ofpmsg_body(request);
+const struct ofp10_port_stats_request *psr10;
+
+psr10 = ofpbuf_try_pull(msg, sizeof *psr10);
+if (!psr10) {
+goto trailing_garbage;
+}
+
 *ofp10_port = u16_to_ofp(ntohs(psr10->port_no));
 return 0;
 }
@@ -5763,6 +5798,11 @@ ofputil_decode_port_stats_request(const struct 
ofp_header *request,
 default:
 NOT_REACHED();
 }
+
+trailing_garbage:
+VLOG_WARN_RL(&bad_ofmsg_rl, "OFPST_PORT_DESC request has "
+ "%"PRIuSIZE" leftover bytes at end", msg->size);
+return OFPERR_OFPBRC_BAD_LEN;
 }
 
 /* Frees all of the "struct ofputil_bucket"s in the 'buckets' list. */
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index fef85e0..726cd3e 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -918,8 +918,8

Re: [ovs-dev] [PATCH RFC] ofproto: Allow multiple array elements in PORT_DESC request body

[Ben Pfaff]

On Thu, Dec 12, 2013 at 09:36:03AM +0900, Simon Horman wrote:
> Signed-off-by: Simon Horman 
> 
> ---
> 
> This change is also needed for several other stats request messages.
> I have posted this as an RFC before proceeding with updating

What's the rationale for the change?  As far as I can tell, OpenFlow
only allows a single port in an OFPST_PORT_REQUEST.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [port-renumbering 7/8] bridge: Reconfigure in single pass.

[Ben Pfaff]

On Wed, Dec 11, 2013 at 04:09:57PM -0800, Jarno Rajahalme wrote:
> 
> On Dec 11, 2013, at 3:14 PM, Ben Pfaff  wrote:
> 
> > On Wed, Dec 11, 2013 at 02:17:13PM -0800, Jarno Rajahalme wrote:
> >> While testing the code I accidentally requested a change in the interface 
> >> type (to dummy), like this:
> >> 
> >> # ovs-vsctl add-br br0
> >> # ovs-vsctl add-port br0 p1 -- set Interface p1 type=internal
> >> # ovs-vsctl -- set Interface p1 type=dummy
> >> 
> >> Running OVS without dummy enabled seg faults:
> > 
> > I couldn't reproduce this, can you try to get me a backtrace or run
> > under valgrind?
> 

[...]

> ==33664== Invalid read of size 2
> ==33664==at 0x40A1C2: bridge_reconfigure (bridge.c:1568)
> ==33664==by 0x406BAC: bridge_run (bridge.c:2289)
> ==33664==by 0x40E498: main (ovs-vswitchd.c:118)
> ==33664==  Address 0x48 is not stack'd, malloc'd or (recently) free'd

[...]

> The offending line is:
> 
> if (iface->ofp_port == OFPP_LOCAL) {
> 
> Seems like iface is NULL (if offset of odp_port == 0x48) in this case.

Thanks.

I figured out how to reproduce it.

I folded this in and it fixes the problem for me.  For you too?

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index ed16860..5dd7752 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -653,6 +653,7 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
 
 OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) {
 struct iface *iface;
+struct port *port;
 
 iface = iface_lookup(br, ofproto_port.name);
 if (!iface) {
@@ -679,7 +680,11 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
 continue;
 
 delete:
+port = iface->port;
 iface_destroy(iface);
+if (list_is_empty(&port->ifaces)) {
+port_destroy(port);
+}
 del = add_ofp_port(ofproto_port.ofp_port, del, &n, &allocated);
 }
 
@@ -3102,7 +3107,12 @@ port_del_ifaces(struct port *port)
 /* Get rid of deleted interfaces. */
 LIST_FOR_EACH_SAFE (iface, next, port_elem, &port->ifaces) {
 if (!sset_contains(&new_ifaces, iface->name)) {
+struct port *port = iface->port;
+
 iface_destroy(iface);
+if (list_is_empty(&port->ifaces)) {
+port_destroy(port);
+}
 }
 }
 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCHv4 2/4] ofproto-dpif: Don't poll ports when nothing changes

[Ethan Jackson]

Acked-by: Ethan Jackson 

Could you fold my acked by into the commit message so when you resend
rebased on the new version of the first patch, I know not to review it
again?

Thanks

On Wed, Dec 11, 2013 at 9:10 AM, Joe Stringer  wrote:
> Currently, as part of ofproto-dpif run() processing, we loop through all
> ports and poll corresponding devices for changes in carrier, cfm and bfd
> status. This allows us to determine how it may affect bundles. For the
> average case where devices are not going up or down constantly, this is
> a large amount of unnecessary processing.
>
> This patch gets the bfd, cfm, lacp and stp modules to update the global
> netdev change_seq when changes in port/device status occur. We can then
> use this global change_seq to check if anything has changed before
> looping through the ports in ofproto-dpif. In a test environment of 5000
> internal ports and 50 tunnel ports with bfd, this reduces CPU usage from
> about 35% to about 25%.
>
> Signed-off-by: Joe Stringer 
> ---
> v4: Ensure that bfd_forwarding__() is called each time through bfd_run().
> ---
>  lib/bfd.c  |6 +-
>  lib/cfm.c  |   13 +
>  lib/lacp.c |7 +++
>  lib/stp.c  |4 
>  ofproto/ofproto-dpif.c |   16 +---
>  tests/test-stp.c   |5 +
>  6 files changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/lib/bfd.c b/lib/bfd.c
> index 1df5acd..2482d37 100644
> --- a/lib/bfd.c
> +++ b/lib/bfd.c
> @@ -21,6 +21,7 @@
>  #include 
>
>  #include "byte-order.h"
> +#include "connectivity.h"
>  #include "csum.h"
>  #include "dpif.h"
>  #include "dynamic-string.h"
> @@ -505,8 +506,8 @@ bfd_run(struct bfd *bfd) OVS_EXCLUDED(mutex)
>
>  if (bfd->state > STATE_DOWN && now >= bfd->detect_time) {
>  bfd_set_state(bfd, STATE_DOWN, DIAG_EXPIRED);
> -bfd_forwarding__(bfd);
>  }
> +bfd_forwarding__(bfd);
>
>  /* Decay may only happen when state is STATE_UP, bfd->decay_min_rx is
>   * configured, and decay_detect_time is reached. */
> @@ -851,6 +852,7 @@ bfd_forwarding__(struct bfd *bfd) OVS_REQUIRES(mutex)
>  && bfd->rmt_diag != DIAG_RCPATH_DOWN;
>  if (bfd->last_forwarding != last_forwarding) {
>  bfd->flap_count++;
> +connectivity_seq_notify();
>  }
>  return bfd->last_forwarding;
>  }
> @@ -1052,6 +1054,8 @@ bfd_set_state(struct bfd *bfd, enum state state, enum 
> diag diag)
>  if (bfd->state == STATE_UP && bfd->decay_min_rx) {
>  bfd_decay_update(bfd);
>  }
> +
> +connectivity_seq_notify();
>  }
>  }
>
> diff --git a/lib/cfm.c b/lib/cfm.c
> index 9c65b34..5d27c6f 100644
> --- a/lib/cfm.c
> +++ b/lib/cfm.c
> @@ -22,6 +22,7 @@
>  #include 
>
>  #include "byte-order.h"
> +#include "connectivity.h"
>  #include "dynamic-string.h"
>  #include "flow.h"
>  #include "hash.h"
> @@ -396,6 +397,7 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
>  long long int interval = cfm_fault_interval(cfm);
>  struct remote_mp *rmp, *rmp_next;
>  bool old_cfm_fault = cfm->fault;
> +bool old_rmp_opup = cfm->remote_opup;
>  bool demand_override;
>  bool rmp_set_opup = false;
>  bool rmp_set_opdown = false;
> @@ -420,6 +422,7 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
>  cfm->health = 0;
>  } else {
>  int exp_ccm_recvd;
> +int old_health = cfm->health;
>
>  rmp = CONTAINER_OF(hmap_first(&cfm->remote_mps),
> struct remote_mp, node);
> @@ -434,6 +437,10 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
>  cfm->health = MIN(cfm->health, 100);
>  rmp->num_health_ccm = 0;
>  ovs_assert(cfm->health >= 0 && cfm->health <= 100);
> +
> +if (cfm->health != old_health) {
> +connectivity_seq_notify();
> +}
>  }
>  cfm->health_interval = 0;
>  }
> @@ -476,6 +483,10 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
>  cfm->remote_opup = true;
>  }
>
> +if (old_rmp_opup != cfm->remote_opup) {
> +connectivity_seq_notify();
> +}
> +
>  if (hmap_is_empty(&cfm->remote_mps)) {
>  cfm->fault |= CFM_FAULT_RECV;
>  }
> @@ -497,6 +508,8 @@ cfm_run(struct cfm *cfm) OVS_EXCLUDED(mutex)
>  if (old_cfm_fault == false || cfm->fault == false) {
>  cfm->flap_count++;
>  }
> +
> +connectivity_seq_notify();
>  }
>
>  cfm->booted = true;
> diff --git a/lib/lacp.c b/lib/lacp.c
> index fce65b3..bb6e940 100644
> --- a/lib/lacp.c
> +++ b/lib/lacp.c
> @@ -18,6 +18,7 @@
>
>  #include 
>
> +#include "connectivity.h"
>  #include "dynamic-string.h"
>  #include "hash.h"
>  #include "hmap.h"
> @@ -509,11 +510,16 @@ lacp_run(struct lac

Re: [ovs-dev] [PATCHv4 3/4] ofproto-dpif: Only run bundles when lacp or bonds are enabled

[Ethan Jackson]

Acked-by: Ethan Jackson 


On Wed, Dec 11, 2013 at 9:10 AM, Joe Stringer  wrote:
> When dealing with a large number of ports, bundle_run() and
> bundle_wait() add significant unnecessary processing to the main run
> loop, even when there are no bonds and lacp is not configured. This
> patch skips such execution if it is unneeded, reducing CPU usage from
> about 25% to about 20% in a test environment of 5000 internal ports and
> 50 tunnel ports running bfd.
>
> Signed-off-by: Joe Stringer 
> ---
> v4: Rebase
> ---
>  ofproto/ofproto-dpif.c |   21 +++--
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index a4334cc..3235891 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -488,6 +488,7 @@ struct ofproto_dpif {
>  struct hmap bundles;/* Contains "struct ofbundle"s. */
>  struct mac_learning *ml;
>  bool has_bonded_bundles;
> +bool lacp_enabled;
>  struct mbridge *mbridge;
>
>  /* Facets. */
> @@ -1254,6 +1255,7 @@ construct(struct ofproto *ofproto_)
>  ofproto->ml = mac_learning_create(MAC_ENTRY_DEFAULT_IDLE_TIME);
>  ofproto->mbridge = mbridge_create();
>  ofproto->has_bonded_bundles = false;
> +ofproto->lacp_enabled = false;
>  ovs_mutex_init(&ofproto->stats_mutex);
>  ovs_mutex_init(&ofproto->vsp_mutex);
>
> @@ -1477,7 +1479,6 @@ static int
>  run(struct ofproto *ofproto_)
>  {
>  struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> -struct ofbundle *bundle;
>  uint64_t new_seq;
>  int error;
>
> @@ -1521,8 +1522,12 @@ run(struct ofproto *ofproto_)
>
>  ofproto->change_seq = new_seq;
>  }
> -HMAP_FOR_EACH (bundle, hmap_node, &ofproto->bundles) {
> -bundle_run(bundle);
> +if (ofproto->lacp_enabled || ofproto->has_bonded_bundles) {
> +struct ofbundle *bundle;
> +
> +HMAP_FOR_EACH (bundle, hmap_node, &ofproto->bundles) {
> +bundle_run(bundle);
> +}
>  }
>
>  stp_run(ofproto);
> @@ -1562,7 +1567,6 @@ static void
>  wait(struct ofproto *ofproto_)
>  {
>  struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> -struct ofbundle *bundle;
>
>  if (ofproto_get_flow_restore_wait()) {
>  return;
> @@ -1574,8 +1578,12 @@ wait(struct ofproto *ofproto_)
>  if (ofproto->ipfix) {
>  dpif_ipfix_wait(ofproto->ipfix);
>  }
> -HMAP_FOR_EACH (bundle, hmap_node, &ofproto->bundles) {
> -bundle_wait(bundle);
> +if (ofproto->lacp_enabled || ofproto->has_bonded_bundles) {
> +struct ofbundle *bundle;
> +
> +HMAP_FOR_EACH (bundle, hmap_node, &ofproto->bundles) {
> +bundle_wait(bundle);
> +}
>  }
>  if (ofproto->netflow) {
>  netflow_wait(ofproto->netflow);
> @@ -2477,6 +2485,7 @@ bundle_set(struct ofproto *ofproto_, void *aux,
>
>  /* LACP. */
>  if (s->lacp) {
> +ofproto->lacp_enabled = true;
>  if (!bundle->lacp) {
>  ofproto->backer->need_revalidate = REV_RECONFIGURE;
>  bundle->lacp = lacp_create();
> --
> 1.7.9.5
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCHv4 4/4] bridge: Only store instant_stats on device changes

[Ethan Jackson]

Acked-by: Ethan Jackson 

The first line of the commit message needs a period.

On Wed, Dec 11, 2013 at 9:10 AM, Joe Stringer  wrote:
> Previously, we iterated through all interfaces in instant_stats_run(),
> grabbing up-to-date information about device status. After assembling
> all of this information for all interfaces, we would determine whether
> anything changed and only send an update to ovsdb-server if something
> changed.
>
> This patch uses the new global netdev_seq to determine whether there
> have been any changes before polling all interfaces, which provides
> further CPU usage savings. In a test environment of 5000 internal ports
> and 50 tunnel ports with bfd, this reduces CPU usage to around 5%. When
> ports change status more often than every 100ms, CPU usage will increase
> to previous rates.
>
> Signed-off-by: Joe Stringer 
> ---
> v4: Rebase
> ---
>  vswitchd/bridge.c |   10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 6da5e93..4c35ebb 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -170,6 +170,9 @@ static struct ovsdb_idl_txn *daemonize_txn;
>  /* Most recently processed IDL sequence number. */
>  static unsigned int idl_seqno;
>
> +/* Track changes to port connectivity. */
> +static uint64_t connectivity_seqno = LLONG_MIN;
> +
>  /* Each time this timer expires, the bridge fetches interface and mirror
>   * statistics and pushes them into the database. */
>  #define IFACE_STATS_INTERVAL (5 * 1000) /* In milliseconds. */
> @@ -2254,12 +2257,19 @@ instant_stats_run(void)
>
>  if (!instant_txn) {
>  struct bridge *br;
> +uint64_t seq;
>
>  if (time_msec() < instant_next_txn) {
>  return;
>  }
>  instant_next_txn = time_msec() + INSTANT_INTERVAL_MSEC;
>
> +seq = connectivity_seq_read();
> +if (seq == connectivity_seqno) {
> +return;
> +}
> +connectivity_seqno = seq;
> +
>  instant_txn = ovsdb_idl_txn_create(idl);
>  HMAP_FOR_EACH (br, node, &all_bridges) {
>  struct iface *iface;
> --
> 1.7.9.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCHv4 1/4] netdev: Globally track port status changes

[Ethan Jackson]

I've reviewed this entire series except for the new version of the
first patch.  Ben, I'll be traveling all day tomorrow, so once Joe
sends the new version of this patch, would you please merge the series
for me?

Thanks,
Ethan

On Wed, Dec 11, 2013 at 10:58 AM, Joe Stringer  wrote:
> On 11 December 2013 10:44, Ethan Jackson  wrote:
>> Couple of comments.
>>
>> Do we need a whole new module for the connectivity seq stuff?
>> Couldn't we just do with an extern global struct seq *?  That said,
>> it's not exactly clear where we'd put it, so perhaps a separate module
>> is the way to go.  I don't feel strongly about it.
>>
>> Along those same lines, if we need a whole separate module, could all
>> this be implemented with a single function:
>>
>> struct seq *connectivity_seq_get().
>>
>> This function would create the seq if it doesn't exist, and return it.
>>  With this approach we don't have to deal with all the pass through
>> boilerplate logic.
>
> This seems sensible. I had it in mind that if we wish to split up this
> single seq for each protocol later, we may wish to collect them in a
> separate module like this. However I don't think that's a strong
> reason to leave it where it is. One alternative is to place it in
> seq.[ch]. What do you think?
>
>> Could you move the comments for the connectivity seq stuff from the
>> header file into the C file?  It's not entirely clear to me why that's
>> our convention, but the rest of the code does it that way, so may as
>> well here to.
>
> Sure, I'll do that.
> ___
> 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] [threaded-put 14/21] ofproto-dpif-xlate: Add a mechanism to skip wildcard calculation.

[Ethan Jackson]

Btw to whoever reviews this, this version of the patch fails to
initialize skip_wildcards in xlate_in_init().

Ethan

On Sun, Dec 8, 2013 at 6:45 PM, Ethan Jackson  wrote:
> As time goes on and the classifier becomes more complicated, calculate
> the wildcard mask will get more and more expensive.  This patch adds a
> mechanism to xlate_actions() allowing callers to disable wildcard
> calculation when it isn't really necessary.  Used in future patches.
>
> Signed-off-by: Ethan Jackson 
> ---
>  ofproto/ofproto-dpif-xlate.c | 10 ++
>  ofproto/ofproto-dpif-xlate.h |  6 ++
>  2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index da03f06..58ec484 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1870,9 +1870,10 @@ xlate_table_action(struct xlate_ctx *ctx,
> ofp_port_t in_port, uint8_t table_id, bool may_packet_in)
>  {
>  if (xlate_resubmit_resource_check(ctx)) {
> -struct rule_dpif *rule;
>  ofp_port_t old_in_port = ctx->xin->flow.in_port.ofp_port;
> +bool skip_wildcards = ctx->xin->skip_wildcards;
>  uint8_t old_table_id = ctx->table_id;
> +struct rule_dpif *rule;
>
>  ctx->table_id = table_id;
>
> @@ -1880,8 +1881,8 @@ xlate_table_action(struct xlate_ctx *ctx,
>   * original input port (otherwise OFPP_NORMAL and OFPP_IN_PORT will
>   * have surprising behavior). */
>  ctx->xin->flow.in_port.ofp_port = in_port;
> -rule_dpif_lookup_in_table(ctx->xbridge->ofproto,
> -  &ctx->xin->flow, &ctx->xout->wc,
> +rule_dpif_lookup_in_table(ctx->xbridge->ofproto, &ctx->xin->flow,
> +  !skip_wildcards ? &ctx->xout->wc : NULL,
>table_id, &rule);
>  ctx->xin->flow.in_port.ofp_port = old_in_port;
>
> @@ -3077,7 +3078,8 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out 
> *xout)
>  ctx.mpls_depth_delta = 0;
>
>  if (!xin->ofpacts && !ctx.rule) {
> -rule_dpif_lookup(ctx.xbridge->ofproto, flow, wc, &rule);
> +rule_dpif_lookup(ctx.xbridge->ofproto, flow,
> + !xin->skip_wildcards ? wc : NULL, &rule);
>  if (ctx.xin->resubmit_stats) {
>  rule_dpif_credit_stats(rule, ctx.xin->resubmit_stats);
>  }
> diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
> index f4ef1e1..68076ca 100644
> --- a/ofproto/ofproto-dpif-xlate.h
> +++ b/ofproto/ofproto-dpif-xlate.h
> @@ -73,6 +73,12 @@ struct xlate_in {
>   * not if we are just revalidating. */
>  bool may_learn;
>
> +/* If the caller of xlate_actions() doesn't need the flow_wildcards
> + * contained in struct xlate_out.  'skip_wildcards' can be set to true
> + * disabling the expensive wildcard computation.  When true, 'wc' in 
> struct
> + * xlate_out is undefined and should not be read. */
> +bool skip_wildcards;
> +
>  /* The rule initiating translation or NULL. If both 'rule' and 'ofpacts'
>   * are NULL, xlate_actions() will do the initial rule lookup itself. */
>  struct rule_dpif *rule;
> --
> 1.8.1.2
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH RFC] ofproto: Allow multiple array elements in PORT_DESC request body

[Simon Horman]

On Wed, Dec 11, 2013 at 05:34:28PM -0800, Ben Pfaff wrote:
> On Thu, Dec 12, 2013 at 09:36:03AM +0900, Simon Horman wrote:
> > Signed-off-by: Simon Horman 
> > 
> > ---
> > 
> > This change is also needed for several other stats request messages.
> > I have posted this as an RFC before proceeding with updating
> 
> What's the rationale for the change?  As far as I can tell, OpenFlow
> only allows a single port in an OFPST_PORT_REQUEST.

Indeed, I was confused!

I have been looking at implementing multi-part requests and for some reason
I assumed they were based on requests with multiple array elements. I now
see that is not true in the general case..
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovs-thread: Break recursion for coverage counters.

[Saurabh Shah]

Looks good to me. Thanks for fixing this!

From: Gurucharan Shetty mailto:shet...@nicira.com>>
Date: Wednesday, December 11, 2013 4:11 PM
To: "dev@openvswitch.org" 
mailto:dev@openvswitch.org>>
Subject: [ovs-dev] [PATCH] ovs-thread: Break recursion for coverage counters.

For systems that do not use linker sections and also do not
have either HAVE_THREAD_LOCAL or HAVE___THREAD (ex: windows
using MSVC), a COVERAGE_INC() calls xmalloc which inturn calls
COVERAGE_INC() creating a recursion that causes a stack overflow.

This commit breaks the recursion by calling malloc() instead of
xmalloc()

Signed-off-by: Gurucharan Shetty mailto:gshe...@nicira.com>>
---
lib/ovs-thread.h |5 -
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index 7f3195d..8de0f27 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -313,7 +313,10 @@ void xpthread_join(pthread_t, void **);
 if (!value) {   \
 static const NAME##_type initial_value = __VA_ARGS__;   \
 \
-value = xmalloc(sizeof *value); \
+value = malloc(sizeof *value);  \
+if (value == NULL) {\
+out_of_memory();\
+}   \
 *value = initial_value; \
 xpthread_setspecific(NAME##_key, value);\
 }   \
--
1.7.9.5

___
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=pEkjsHfytvHEWufeZPpgqSOJMdMjuZPbesVsNhCUc0E%3D%0A&m=HChkh6ne5OJCVvYw0WP5wywcMq64u64%2FmhcsvoD8Rqc%3D%0A&s=be73dc6736c4f0d1a84ec8b61ec9fa1f4b5aba836968fb445c923f1a42c7b92e

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] lib/util: More portable use of builtin popcnt.

[Simon Horman]

On Wed, Dec 11, 2013 at 03:41:19PM -0800, Jarno Rajahalme wrote:
> - Use the GCC predefined macro __POPCNT__ to detect the availability
>   of fast __builtin_popcnt function.
> - Use portable preprocessor macros to detect 64-bit build.
> - Only define the 32-bit parts when needed and declare the
>   count_1bits_8 at file scope to silence a warning.
> 
> This time I have tested all code paths to make sure no warnigns are
> generated.
> 
> Signed-off-by: Jarno Rajahalme 

No objections here.

Reviewed-by: Simon Horman 

> ---
>  lib/util.c |2 +-
>  lib/util.h |   62 
> +---
>  2 files changed, 35 insertions(+), 29 deletions(-)
> 
> diff --git a/lib/util.c b/lib/util.c
> index 13d41a7..000504c 100644
> --- a/lib/util.c
> +++ b/lib/util.c
> @@ -901,7 +901,7 @@ raw_clz64(uint64_t n)
>  }
>  #endif
>  
> -#if !(__GNUC__ >= 4 && defined(__corei7))
> +#if NEED_COUNT_1BITS_8
>  #define INIT1(X)\
>  X) & (1 << 0)) != 0) +  \
>   (((X) & (1 << 1)) != 0) +  \
> diff --git a/lib/util.h b/lib/util.h
> index 8d810c2..0327ab0 100644
> --- a/lib/util.h
> +++ b/lib/util.h
> @@ -371,49 +371,55 @@ log_2_ceil(uint64_t n)
>  return log_2_floor(n) + !is_pow2(n);
>  }
>  
> -extern const uint8_t count_1bits_8[256];
> -
> -/* Returns the number of 1-bits in 'x', between 0 and 32 inclusive. */
> +/* unsigned int count_1bits(uint64_t x):
> + *
> + * Returns the number of 1-bits in 'x', between 0 and 64 inclusive. */
> +#if UINTPTR_MAX == UINT64_MAX
> +static inline unsigned int
> +count_1bits(uint64_t x)
> +{
> +#if __GNUC__ >= 4 && __POPCNT__
> +return __builtin_popcountll(x);
> +#else
> +/* This portable implementation is the fastest one we know of for 64
> + * bits, and about 3x faster than GCC 4.7 __builtin_popcountll(). */
> +const uint64_t h55 = UINT64_C(0x);
> +const uint64_t h33 = UINT64_C(0x);
> +const uint64_t h0F = UINT64_C(0x0F0F0F0F0F0F0F0F);
> +const uint64_t h01 = UINT64_C(0x0101010101010101);
> +x -= (x >> 1) & h55;   /* Count of each 2 bits in-place. */
> +x = (x & h33) + ((x >> 2) & h33);  /* Count of each 4 bits in-place. */
> +x = (x + (x >> 4)) & h0F;  /* Count of each 8 bits in-place. */
> +return (x * h01) >> 56;/* Sum of all bytes. */
> +#endif
> +}
> +#else /* Not 64-bit. */
> +#if __GNUC__ >= 4 && __POPCNT__
>  static inline unsigned int
> -count_1bits_32(uint32_t x)
> +count_1bits_32__(uint32_t x)
>  {
> -#if __GNUC__ >= 4 && defined(__corei7)
> -/* __builtin_popcount() is fast only when supported by the CPU. */
>  return __builtin_popcount(x);
> +}
>  #else
> +#define NEED_COUNT_1BITS_8 1
> +extern const uint8_t count_1bits_8[256];
> +static inline unsigned int
> +count_1bits_32__(uint32_t x)
> +{
>  /* This portable implementation is the fastest one we know of for 32 
> bits,
>   * and faster than GCC __builtin_popcount(). */
>  return (count_1bits_8[x & 0xff] +
>  count_1bits_8[(x >> 8) & 0xff] +
>  count_1bits_8[(x >> 16) & 0xff] +
>  count_1bits_8[x >> 24]);
> -#endif
>  }
> -
> -/* Returns the number of 1-bits in 'x', between 0 and 64 inclusive. */
> +#endif
>  static inline unsigned int
>  count_1bits(uint64_t x)
>  {
> -if (sizeof(void *) == 8) { /* 64-bit CPU */
> -#if __GNUC__ >= 4 && defined(__corei7)
> -/* __builtin_popcountll() is fast only when supported by the CPU. */
> -return __builtin_popcountll(x);
> -#else
> -/* This portable implementation is the fastest one we know of for 64
> - * bits, and about 3x faster than GCC 4.7 __builtin_popcountll(). */
> -const uint64_t h55 = UINT64_C(0x);
> -const uint64_t h33 = UINT64_C(0x);
> -const uint64_t h0F = UINT64_C(0x0F0F0F0F0F0F0F0F);
> -const uint64_t h01 = UINT64_C(0x0101010101010101);
> -x -= (x >> 1) & h55;   /* Count of each 2 bits in-place. 
> */
> -x = (x & h33) + ((x >> 2) & h33);  /* Count of each 4 bits in-place. 
> */
> -x = (x + (x >> 4)) & h0F;  /* Count of each 8 bits in-place. 
> */
> -return (x * h01) >> 56;/* Sum of all bytes. */
> -#endif
> -} else { /* 32-bit CPU */
> -return count_1bits_32(x) + count_1bits_32(x >> 32);
> -}
> +return count_1bits_32__(x) + count_1bits_32__(x >> 32);
>  }
> +#endif
>  
>  /* Returns the rightmost 1-bit in 'x' (e.g. 01011000 => 1000), or 0 if 
> 'x'
>   * is 0. */
> -- 
> 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] [port-renumbering 7/8] bridge: Reconfigure in single pass.

[Ben Pfaff]

On Wed, Dec 11, 2013 at 05:55:35PM -0800, Ben Pfaff wrote:
> On Wed, Dec 11, 2013 at 04:09:57PM -0800, Jarno Rajahalme wrote:
> > 
> > On Dec 11, 2013, at 3:14 PM, Ben Pfaff  wrote:
> > 
> > > On Wed, Dec 11, 2013 at 02:17:13PM -0800, Jarno Rajahalme wrote:
> > >> While testing the code I accidentally requested a change in the 
> > >> interface type (to dummy), like this:
> > >> 
> > >> # ovs-vsctl add-br br0
> > >> # ovs-vsctl add-port br0 p1 -- set Interface p1 type=internal
> > >> # ovs-vsctl -- set Interface p1 type=dummy
> > >> 
> > >> Running OVS without dummy enabled seg faults:
> > > 
> > > I couldn't reproduce this, can you try to get me a backtrace or run
> > > under valgrind?
> > 
> 
> [...]
> 
> > ==33664== Invalid read of size 2
> > ==33664==at 0x40A1C2: bridge_reconfigure (bridge.c:1568)
> > ==33664==by 0x406BAC: bridge_run (bridge.c:2289)
> > ==33664==by 0x40E498: main (ovs-vswitchd.c:118)
> > ==33664==  Address 0x48 is not stack'd, malloc'd or (recently) free'd
> 
> [...]
> 
> > The offending line is:
> > 
> > if (iface->ofp_port == OFPP_LOCAL) {
> > 
> > Seems like iface is NULL (if offset of odp_port == 0x48) in this case.
> 
> Thanks.
> 
> I figured out how to reproduce it.
> 
> I folded this in and it fixes the problem for me.  For you too?
> 
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index ed16860..5dd7752 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -653,6 +653,7 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
>  
>  OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) {
>  struct iface *iface;
> +struct port *port;
>  
>  iface = iface_lookup(br, ofproto_port.name);
>  if (!iface) {
> @@ -679,7 +680,11 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
>  continue;
>  
>  delete:
> +port = iface->port;
>  iface_destroy(iface);
> +if (list_is_empty(&port->ifaces)) {
> +port_destroy(port);
> +}
>  del = add_ofp_port(ofproto_port.ofp_port, del, &n, &allocated);
>  }
>  
> @@ -3102,7 +3107,12 @@ port_del_ifaces(struct port *port)
>  /* Get rid of deleted interfaces. */
>  LIST_FOR_EACH_SAFE (iface, next, port_elem, &port->ifaces) {
>  if (!sset_contains(&new_ifaces, iface->name)) {
> +struct port *port = iface->port;
> +
>  iface_destroy(iface);
> +if (list_is_empty(&port->ifaces)) {
> +port_destroy(port);
> +}
>  }
>  }
>  

But introduced a new segfault because 'iface' could be null in the first
case.  I folded in the following additional patch as a combined fix and
cleanup.

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 8267636..37f5812 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -657,7 +657,6 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
 OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) {
 ofp_port_t requested_ofp_port;
 struct iface *iface;
-struct port *port;
 
 iface = iface_lookup(br, ofproto_port.name);
 if (!iface) {
@@ -721,11 +720,7 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
 continue;
 
 delete:
-port = iface->port;
 iface_destroy(iface);
-if (list_is_empty(&port->ifaces)) {
-port_destroy(port);
-}
 del = add_ofp_port(ofproto_port.ofp_port, del, &n, &allocated);
 }
 
@@ -3131,6 +3126,8 @@ bridge_configure_dp_desc(struct bridge *br)
 
 /* Port functions. */
 
+static void iface_destroy__(struct iface *);
+
 static struct port *
 port_create(struct bridge *br, const struct ovsrec_port *cfg)
 {
@@ -3167,12 +3164,7 @@ port_del_ifaces(struct port *port)
 /* Get rid of deleted interfaces. */
 LIST_FOR_EACH_SAFE (iface, next, port_elem, &port->ifaces) {
 if (!sset_contains(&new_ifaces, iface->name)) {
-struct port *port = iface->port;
-
 iface_destroy(iface);
-if (list_is_empty(&port->ifaces)) {
-port_destroy(port);
-}
 }
 }
 
@@ -3191,7 +3183,7 @@ port_destroy(struct port *port)
 }
 
 LIST_FOR_EACH_SAFE (iface, next, port_elem, &port->ifaces) {
-iface_destroy(iface);
+iface_destroy__(iface);
 }
 
 hmap_remove(&br->ports, &port->hmap_node);
@@ -3413,7 +3405,7 @@ iface_get_type(const struct ovsrec_interface *iface,
 }
 
 static void
-iface_destroy(struct iface *iface)
+iface_destroy__(struct iface *iface)
 {
 if (iface) {
 struct port *port = iface->port;
@@ -3437,6 +3429,19 @@ iface_destroy(struct iface *iface)
 }
 }
 
+static void
+iface_destroy(struct iface *iface)
+{
+if (iface) {
+struct port *port = iface->port;
+
+iface_destroy__(iface);
+if (list_is_empty(&port->ifaces)) {
+port_destroy(port);
+}
+}
+}
+
 static struct iface *

Re: [ovs-dev] [port-renumbering 8/8] bridge: Support changing port numbers.

[Ben Pfaff]

On Wed, Dec 11, 2013 at 02:17:56PM -0800, Jarno Rajahalme wrote:
> 
> On Dec 10, 2013, at 11:20 PM, Ben Pfaff  wrote:
> 
> > Feature #21293.
> > Requested-by: Anuprem Chalvadi 
> > Signed-off-by: Ben Pfaff 
> > ---
> > NEWS |3 ++
> > vswitchd/bridge.c|   93 
> > --
> > vswitchd/vswitch.xml |   21 +++-
> > 3 files changed, 99 insertions(+), 18 deletions(-)
> > 
> > diff --git a/NEWS b/NEWS
> > index 8556083..ce968f2 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -14,6 +14,9 @@ Post-v2.0.0
> >  * OVS limits the OpenFlow port numbers it assigns to port 32767 and
> >below, leaving port numbers above that range free for assignment
> >by the controller.
> > + * ovs-vswitchd now honors changes to the "ofport_request" column
> > +   in the Interface table by changing the port's OpenFlow port
> > +   number.
> >- ovs-vswitchd.conf.db.5 man page will contain graphviz/dot
> >  diagram only if graphviz package was installed at the build time.
> >- Support for Linux kernels up to 3.11
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 581f87c..131b800 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -75,6 +75,7 @@ struct iface {
> > char *name; /* Host network device name. */
> > struct netdev *netdev;  /* Network device. */
> > ofp_port_t ofp_port;/* OpenFlow port number. */
> > +ofp_port_t requested_ofp_port; /* Port number requested previously. */
> > 
> > /* These members are valid only within bridge_reconfigure(). */
> > const char *type;   /* Usually same as cfg->type. */
> > @@ -247,6 +248,8 @@ static void iface_refresh_cfm_stats(struct iface *);
> > static void iface_refresh_stats(struct iface *);
> > static void iface_refresh_status(struct iface *);
> > static bool iface_is_synthetic(const struct iface *);
> > +static ofp_port_t iface_get_requested_ofp_port(
> > +const struct ovsrec_interface *);
> > static ofp_port_t iface_pick_ofport(const struct ovsrec_interface *);
> > 
> > /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
> > @@ -636,6 +639,7 @@ bridge_delete_ports(struct bridge *br)
> > n = allocated = 0;
> > 
> > OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) {
> > +ofp_port_t requested_ofp_port;
> > struct iface *iface;
> > 
> > iface = iface_lookup(br, ofproto_port.name);
> > @@ -657,6 +661,43 @@ bridge_delete_ports(struct bridge *br)
> > goto delete;
> > }
> > 
> > +/* If the requested OpenFlow port for 'iface' changed, and it's not
> > + * already the correct port, then we might want to temporarily 
> > delete
> > + * this interface, so we can add it back again with the new 
> > OpenFlow
> > + * port number. */
> > +requested_ofp_port = iface_get_requested_ofp_port(iface->cfg);
> > +if (requested_ofp_port != iface->requested_ofp_port &&
> > +requested_ofp_port != OFPP_NONE &&
> > +requested_ofp_port != iface->ofp_port) {
> 
> It would seem that this test should fail if the face requested a port
> number that it did not get (for any reason), and is still requesting
> the same port number, but the offending other port was deleted and the
> (originally) requested port number would now be available. However,
> when testing this, this seems to work:
> 
> - create ports p1, p2
> - request port number 10 for p1 -> p1 changes to 10
> - request port number 10 for p2 -> p2 does not change
> - request port number 11 for p1 -> p1 changes to 11 AND p2 changes to 10
> 
> Is it so that the requested port number is not stored unless it is
> also gotten?
> 
>  Maybe this is intended behavior to avoid surprisingly changing port
>  numbers, even if the change would be towards the wanted
>  configuration?

My intent here was actually to avoid churn in some theoretical ofproto
implementation which limits (or entirely fixes) the port numbers that
certain ports can have, by only trying a given ofport_request once until
it changes.  But that may not be a real problem (I don't know of any
ofprotos that actually work that way), whereas the problem you describe
is one that I would like to avoid in reality.

I folded this in:

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 37f5812..f3b03af 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -75,7 +75,6 @@ struct iface {
 char *name; /* Host network device name. */
 struct netdev *netdev;  /* Network device. */
 ofp_port_t ofp_port;/* OpenFlow port number. */
-ofp_port_t requested_ofp_port; /* Port number requested previously. */
 
 /* These members are valid only within bridge_reconfigure(). */
 const char *type;   /* Usually same as cfg->type. */
@@ -684,7 +683,7 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
  * this interfac