Re: [ovs-dev] [PATCH] ovsdb: Improve error message from ovsdb_log_open() open failure.
On Thu, Mar 31, 2016 at 12:40:06AM -0400, Russell Bryant wrote: > On Wed, Mar 30, 2016 at 11:11 PM, Ben Pfaff wrote: > > > Previously, error messages ended up looking like: > > ovsdb-tool: I/O error: create: $DBFILE failed (File exists) > > which is hard to understand. This commit changes them to: > > ovsdb-tool: I/O error: $DBFILE: create failed (File exists) > > which makes more sense. > > > > Signed-off-by: Ben Pfaff > > --- > > ovsdb/log.c | 2 +- > > tests/ovsdb-log.at| 2 +- > > tests/ovsdb-server.at | 2 +- > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > > Untested, but the swap makes sense. > > Acked-by: Russell Bryant Thanks. I applied this to master. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn-controller: Loopback prevention flows for local ports only.
On Wed, Mar 30, 2016 at 09:47:27PM -0700, Han Zhou wrote: > On Wed, Mar 30, 2016 at 4:48 PM, Ben Pfaff wrote: > > > > On Tue, Mar 29, 2016 at 04:55:11PM -0700, Han Zhou wrote: > > > Currently in physical_run() we added per-port loopback prevention > > > flows for all lports. The flows are actually required only for > > > local ports on the chassis. This change greatly reduces number of > > > flows in table 34. > > > > > > Signed-off-by: Han Zhou > > > > We could do this with a single logical flow of the form: > > > > match="(inport[0] && !outport[0]) || (!inport[0]) && outport[0]) || > >(inport[1] && !outport[1]) || (!inport[1]) && outport[1]) || > > ... > >(inport[15] && !outport[15]) || (!inport[15]) && outport[15])", > > actions="next;" > > > > That's a fairly inefficient solution due to the construction of the OVS > > classifier but we still might consider it at some point. > > > > I applied this to master, thanks! > > I was thinking about: > > match="inport == outport", actions="drop" > > And then I realized that this is not possible without big change to OVS > classifier. But I didn't thought of this smart bit-trick to implement the > comparison. Thanks for enlightening :) Tricks like this can implement any comparison, but usually in a really inefficient manner. If we add OpenFlow extension actions for arithmetic, this particular operation could be efficient. For example, one table could do something equivalent to "reg0 = inport ^ outport;" or "reg0 = inport - outport;" and then the next one could have entries like this: priority=100, match="reg0 == 0", actions="drop;" priority=0, match="1;",actions="next;" However, this kind of thing doesn't generalize very nicely, so I've refrained from working on it. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn-controller: Loopback prevention flows for local ports only.
On Wed, Mar 30, 2016 at 4:48 PM, Ben Pfaff wrote: > > On Tue, Mar 29, 2016 at 04:55:11PM -0700, Han Zhou wrote: > > Currently in physical_run() we added per-port loopback prevention > > flows for all lports. The flows are actually required only for > > local ports on the chassis. This change greatly reduces number of > > flows in table 34. > > > > Signed-off-by: Han Zhou > > We could do this with a single logical flow of the form: > > match="(inport[0] && !outport[0]) || (!inport[0]) && outport[0]) || >(inport[1] && !outport[1]) || (!inport[1]) && outport[1]) || > ... >(inport[15] && !outport[15]) || (!inport[15]) && outport[15])", > actions="next;" > > That's a fairly inefficient solution due to the construction of the OVS > classifier but we still might consider it at some point. > > I applied this to master, thanks! I was thinking about: match="inport == outport", actions="drop" And then I realized that this is not possible without big change to OVS classifier. But I didn't thought of this smart bit-trick to implement the comparison. Thanks for enlightening :) -- Best regards, Han ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovsdb: Improve error message from ovsdb_log_open() open failure.
On Wed, Mar 30, 2016 at 11:11 PM, Ben Pfaff wrote: > Previously, error messages ended up looking like: > ovsdb-tool: I/O error: create: $DBFILE failed (File exists) > which is hard to understand. This commit changes them to: > ovsdb-tool: I/O error: $DBFILE: create failed (File exists) > which makes more sense. > > Signed-off-by: Ben Pfaff > --- > ovsdb/log.c | 2 +- > tests/ovsdb-log.at| 2 +- > tests/ovsdb-server.at | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > Untested, but the swap makes sense. Acked-by: Russell Bryant ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ovsdb: Improve error message from ovsdb_log_open() open failure.
Previously, error messages ended up looking like:
ovsdb-tool: I/O error: create: $DBFILE failed (File exists)
which is hard to understand. This commit changes them to:
ovsdb-tool: I/O error: $DBFILE: create failed (File exists)
which makes more sense.
Signed-off-by: Ben Pfaff
---
ovsdb/log.c | 2 +-
tests/ovsdb-log.at| 2 +-
tests/ovsdb-server.at | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/ovsdb/log.c b/ovsdb/log.c
index c1f8225..8004d3d 100644
--- a/ovsdb/log.c
+++ b/ovsdb/log.c
@@ -114,7 +114,7 @@ ovsdb_log_open(const char *name, enum ovsdb_log_open_mode
open_mode,
fd = open(name, flags, 0666);
if (fd < 0) {
const char *op = open_mode == OVSDB_LOG_CREATE ? "create" : "open";
-error = ovsdb_io_error(errno, "%s: %s failed", op, name);
+error = ovsdb_io_error(errno, "%s: %s failed", name, op);
goto error_unlock;
}
diff --git a/tests/ovsdb-log.at b/tests/ovsdb-log.at
index 055dc70..3e7cdf8 100644
--- a/tests/ovsdb-log.at
+++ b/tests/ovsdb-log.at
@@ -47,7 +47,7 @@ file: read: [1]
]], [ignore])
AT_CHECK(
[test-ovsdb log-io file create read], [1],
- [], [test-ovsdb: I/O error: create: file failed (File exists)
+ [], [test-ovsdb: I/O error: file: create failed (File exists)
])
AT_CHECK([test -f .file.~lock~])
AT_CLEANUP
diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at
index de40330..7b7fb40 100644
--- a/tests/ovsdb-server.at
+++ b/tests/ovsdb-server.at
@@ -202,7 +202,7 @@ fi
# Add a non-existing database.
AT_CHECK([ovs-appctl -t ovsdb-server ovsdb-server/add-db db3], 2, [], [stderr])
AT_CHECK([sed 's/(.*)/(...)/' stderr], [0],
- [I/O error: open: db3 failed (...)
+ [I/O error: db3: open failed (...)
ovs-appctl: ovsdb-server: server returned an error
])
--
2.1.3
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v6 06/12] dpif-netdev: Wait an RCU grace period before freeing ports.
On 30/03/2016 16:01, "Ben Pfaff" wrote: >(I'm taking a look at this patch specifically because Daniele asked me; >I'm not planning to review the whole series.) > >On Mon, Mar 28, 2016 at 12:41:40PM -0700, Daniele Di Proietto wrote: >> The dpif-netdev datapath keeps ports in a cmap which is written only by >> the main thread (holding port_mutex), but which is read concurrently by >> many threads (most notably the pmd threads). >> >> When removing ports from the datapath we should postpone the deletion, >> otherwise another thread might access invalid memory while reading the >> cmap. >> >> This commit splits do_port_del() in do_port_remove() and >> do_port_destroy(): the former removes the port from the cmap, while the >> latter reclaims the memory and drops the reference to the underlying >> netdev. > >s/del_port/port_del/ here: Thanks, changed > >> dpif_netdev_del_port() now uses ovsrcu_synchronize() before calling >> do_port_destroy(), to avoid memory corruption in concurrent readers. > >ovsrcu_synchronize() requires that nothing in the thread that calls it >is relying on RCU to keep objects around. That means that no caller of >dfpi_port_del()--there are a few of them--can rely on it. This is >usually a risky assumption, especially because this assumption can >change later. Is there reason to believe that it isn't important in all >of these cases? I agree that's risky, but I think it's the only way to keep the ports RCU protected, because a port needs to be effectively deleted before dpif_netdev_port_del() can return. I think it will not be too risky because the code that calls dpif_netdev_port_del() is high level code that doesn't deal with RCU protected pointers. Of course, things might change. One way to improve the situation would be to use thread-safety annotation to mark all the functions that might quiesce: I've tried doing that, but a lot of functions need to get tagged, like ovs_mutex_cond_wait(), which we already use in the userspace datapath. An easy way to get rid of this problem would be to avoid RCU for ports and storing them in an hmap, but I would like to avoid that. I'll keep thinking about this, maybe we can come up with a better idea. Thanks, Daniele ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/1] Rationalize ovn-ctl arguments.
On Wed, Mar 30, 2016 at 08:23:23PM -0400, Russell Bryant wrote: > On Wed, Mar 30, 2016 at 8:15 PM, Ben Pfaff wrote: > > > On Wed, Mar 30, 2016 at 07:56:51PM -0400, Russell Bryant wrote: > > > On Wed, Mar 30, 2016 at 2:40 PM, Ben Pfaff wrote: > > > > I'm starting to get really disturbed that ssl isn't the default here. > > > > > > We need to add SSL config to these tables. > > > > I'm not sure that it makes sense to have SSL configuration in > > OVN_Northbound or OVN_Southbound, because the clients would need to > > connect to the databases before they could obtain the configuration. > > I'd guess that SSL configuration would have to be populated to each > > hypervisor as a separate step before it joins OVN for the first time. > > > > Or maybe I misunderstand your point. > > > > I honestly haven't thought through this in enough detail, but: > > I was talking about the server side config. ovsdb-server for OVS is > started with: > > set "$@" --private-key=db:Open_vSwitch,SSL,private_key > set "$@" --certificate=db:Open_vSwitch,SSL,certificate > set "$@" --bootstrap-ca-cert=db:Open_vSwitch,SSL,ca_cert > > I assumed we might add the same SSL table to the OVN dbs. Then again, it > seems kind of awkward to me to have this in the DB. I'd expect it to be > something only configured locally. Right, it's a little different because the Open_vSwitch schema that ovs-vswitchd uses is for a single machine only and primarily (though not exclusively) accessed from that machine. > Anyway, I'd love to see this get sorted out and have SSL everywhere the > default. I agree. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv2 2/2] ofproto-dpif-xlate: Fix bitwise ops on ct_labels.
On Thu, Mar 31, 2016 at 12:21:14AM +1300, Joe Stringer wrote:
> Using the action ct(commit,set_field:0x1/0x1->ct_label), ie, specifying
> a mask, would previously overwrite the entire ct_labels field rather than
> modifying only the specified bits. Fix the issue.
>
> Fixes: 9daf23484fb1 ("Add connection tracking label support.")
> Signed-off-by: Joe Stringer
The following expressions would look more symmetrical if the order of
the arguments in the call to ovs_u128_xor() were swapped:
> +odp_ct_label->mask = ovs_u128_xor(base_flow->ct_label,
> flow->ct_label);
> +odp_ct_label->key = ovs_u128_and(flow->ct_label, odp_ct_label->mask);
> }
> }
Thanks, this looks good to me. Again, I compiled it but I did not test
it.
Acked-by: Ben Pfaff
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv2 1/2] ofproto-dpif-xlate: Fix bitwise ops on ct_mark.
On Thu, Mar 31, 2016 at 12:21:13AM +1300, Joe Stringer wrote:
> Using the action ct(commit,set_field:0x1/0x1->ct_mark), ie, specifying a
> mask, would previously overwrite the entire ct_mark field rather than
> modifying only the specified bits. Fix the issue.
>
> Fixes: 8e53fe8cf7a1 ("Add connection tracking mark support.")
> Signed-off-by: Joe Stringer
> ---
> v2: Remove wildcards from put_ct_mark().
I didn't test this but it looks good and compiles cleanly.
A possible (though unnecessary) enhancement to the test would be to
verify that setting a bit to 0 also works.
Acked-by: Ben Pfaff
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/1] Add Static route to logical router
This patch add static route to logical router which are required in many
scenarios, such as VPNaas service, l3 gateway.
For VPNaas, OVN logical router SHOULD be able to route the VPN remote
subnet to the
VPN VM, static route is the best choice.
This patach will add a new pointer array in "Logical_Router" table, the
pointers will point to a new table "Logical_Router_Static_Route" to record
the static routes.
Current logical router already has default route. This patach will add
static route will ovn northd update the router logical flow. Static route
will add before the default router.
ovn/northd/ovn-northd.c Add static route logical flow
ovn/ovn-nb.ovsschemaAdd staic route table
ovn/utilities/ovn-nbctl.c Add static route to ovn-nbctl command
Test case updates:
Add static route test in case: 3 HVs, 3 LS, 3 lports/LS, 1 LR
Signed-off-by: Steve Ruan ,
Reported-by: Na Zhu
Reported-by: Dustin Lundquist
Reported-at:
https://bugs.launchpad.net/networking-ovn/+bug/1545140
https://bugs.launchpad.net/networking-ovn/+bug/1539347
ovn/northd/ovn-northd.8.xml | 2 +-
ovn/northd/ovn-northd.c | 70 +++
+++
ovn/ovn-nb.ovsschema | 13 -
ovn/ovn-nb.xml| 35 +++
ovn/utilities/ovn-nbctl.8.xml | 5 +
ovn/utilities/ovn-nbctl.c | 5 +
tests/ovn.at | 10 ++
7 files changed, 138 insertions(+), 2 deletions(-)
diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index 743c939..7eb75df 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -680,7 +680,7 @@ next;
- If the route has a gateway, G is the gateway IP
address,
+ If the route has a nexthop, G is the nexthop address,
otherwise it is ip4.dst.
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 0c869f4..bd25c49 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1780,6 +1780,68 @@ add_route(struct hmap *lflows, const struct ovn_port
*op,
}
static void
+build_static_route_flow(struct hmap *lflows, struct ovn_datapath *od,
struct hmap *ports,
+const struct nbrec_logical_router_static_route *route)
+{
+struct ovn_port *out_port, *op;
+ovs_be32 prefix, nexthop, mask;
+int len;
+bool enabled;
+
+/* verify nexthop */
+char *error = ip_parse_masked(route->nexthop, &nexthop, &mask);
+if (error || mask != OVS_BE32_MAX) {
+static struct vlog_rate_limit rl
+= VLOG_RATE_LIMIT_INIT(5, 10);
+VLOG_WARN_RL(&rl, "bad 'nexthop' %s error %s", route->nexthop,
error);
+free(error);
+return;
+}
+
+/* verify prefix */
+error = ip_parse_masked(route->prefix, &prefix, &mask);
+if (error || !ip_is_cidr(mask)) {
+static struct vlog_rate_limit rl
+= VLOG_RATE_LIMIT_INIT(5, 10);
+VLOG_WARN_RL(&rl, "bad 'prefix' %s error %s", route->prefix,
error);
+free(error);
+return;
+}
+
+/* find the outgoing port */
+out_port = NULL;
+len = 0;
+for (int i = 0; i < od->nbr->n_ports; i++){
+
+struct nbrec_logical_router_port *lrp = od->nbr->ports[i];
+op = ovn_port_find(ports, lrp->name);
+if (!op){ /* this should not happen */
+continue;
+}
+
+if (op->network && !((op->network ^ nexthop) & op->mask)){
+if (ip_count_cidr_bits(op->mask) > len){
+len = ip_count_cidr_bits(op->mask);
+out_port = op;
+}
+}
+}
+
+if (!out_port){
+/*set route invalid flag*/
+enabled = false;
+nbrec_logical_router_static_route_set_valid(route, &enabled, 1);
+return;
+}
+
+/* set valid flag */
+enabled = true;
+nbrec_logical_router_static_route_set_valid(route, &enabled, 1);
+
+add_route(lflows, out_port, prefix, mask, nexthop);
+}
+
+static void
build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
struct hmap *lflows)
{
@@ -1948,6 +2010,14 @@ build_lrouter_flows(struct hmap *datapaths, struct
hmap *ports,
continue;
}
+/* convert the routing table to flow */
+for (int i = 0; i < od->nbr->n_routes; i++){
+const struct nbrec_logical_router_static_route *route;
+
+route = od->nbr->routes[i];
+build_static_route_flow(lflows, od, ports, route);
+}
+
if (od->gateway && od->gateway_port) {
add_route(lflows, od->gateway_port, 0, 0, od->gateway);
}
diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index 9fb8cd1..f1894b1 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
{
"name": "OVN_Northbound",
"version": "2.0.1",
-"cksum": "660370796 4618",
+"cksum": "3
[ovs-dev] We offer new vacancy
Hello! We are looking for employees working remotely. My name is Lucas, am the personnel manager of a large International company. Most of the work you can do from home, that is, at a distance. Salary is $2000-$5000. If you are interested in this offer, please visit Our Web Page Best regards! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] ovn: Minor refactoring.
On Wed, Mar 30, 2016 at 8:08 PM, Ben Pfaff wrote: > On Tue, Mar 29, 2016 at 06:47:42PM -0700, Russell Bryant wrote: > > This commit applies a minor restructuring of this code to put the > > localnet port specific code in its own block. This is mostly to make a > > future patch easier to read. > > > > Signed-off-by: Russell Bryant > > Acked-by: Ben Pfaff > Thanks. I applied this patch to master. -- Russell Bryant ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/1] Rationalize ovn-ctl arguments.
On Wed, Mar 30, 2016 at 8:15 PM, Ben Pfaff wrote: > On Wed, Mar 30, 2016 at 07:56:51PM -0400, Russell Bryant wrote: > > On Wed, Mar 30, 2016 at 2:40 PM, Ben Pfaff wrote: > > > I'm starting to get really disturbed that ssl isn't the default here. > > > > We need to add SSL config to these tables. > > I'm not sure that it makes sense to have SSL configuration in > OVN_Northbound or OVN_Southbound, because the clients would need to > connect to the databases before they could obtain the configuration. > I'd guess that SSL configuration would have to be populated to each > hypervisor as a separate step before it joins OVN for the first time. > > Or maybe I misunderstand your point. > I honestly haven't thought through this in enough detail, but: I was talking about the server side config. ovsdb-server for OVS is started with: set "$@" --private-key=db:Open_vSwitch,SSL,private_key set "$@" --certificate=db:Open_vSwitch,SSL,certificate set "$@" --bootstrap-ca-cert=db:Open_vSwitch,SSL,ca_cert I assumed we might add the same SSL table to the OVN dbs. Then again, it seems kind of awkward to me to have this in the DB. I'd expect it to be something only configured locally. Anyway, I'd love to see this get sorted out and have SSL everywhere the default. -- Russell Bryant ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/1] Rationalize ovn-ctl arguments.
On Wed, Mar 30, 2016 at 07:56:51PM -0400, Russell Bryant wrote: > On Wed, Mar 30, 2016 at 2:40 PM, Ben Pfaff wrote: > > I'm starting to get really disturbed that ssl isn't the default here. > > We need to add SSL config to these tables. I'm not sure that it makes sense to have SSL configuration in OVN_Northbound or OVN_Southbound, because the clients would need to connect to the databases before they could obtain the configuration. I'd guess that SSL configuration would have to be populated to each hypervisor as a separate step before it joins OVN for the first time. Or maybe I misunderstand your point. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/4] docs: OVSDB replication design document
On Wed, Mar 30, 2016 at 06:03:27PM -0600, Marcelo E. Magallon wrote: > On 03/30/2016 05:27 PM, Ben Pfaff wrote: > >I'm in the midst of implementing high availability for OVSDB, based on > >the Raft algorithm. When I'm done, it should be possible to set up > >OVSDB clusters with automatic failover. Is this the same use case as > >your code? > > No, in this case the replication works by way of making the second server an > OVSDB client, so it gets notified about all the changes in the remote > server. Yes, I understand that the technical effect is not the same. Replication is much simpler and does not have the same characteristics. > In your case, how many OVSDB servers are required for replication to work? > Raft has a quorum requirement of (N/2)+1 servers to be available in order > for consensus to be reached. That makes the minimum number of servers 3, if > you want to allow one to become unavailable. If you have 2 servers and one > goes down, there's no quorum. Yes, this is correct, of course. > The patch Mario is proposing has failover characteristics, not > load-distribution characteristics: if one of the servers goes down, the > other one can take over because it has a copy of the data up to the last > notification the server was able to sent. Also, in the proposed patch > transactions are not delayed until consensus is reached, but instead clients > are talking only to the active server, which is applying transactions > immediately. The active server is notifying the standby about the new data > just like it's doing with any other client. I understand the technical differences between the approaches. My question is whether high availability is your actual goal. If it is, then it probably does not make sense to have multiple implementations. If you are trying to accomplish something else, then it could be that there is something complementary about the two implementations. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] ovn: Minor refactoring.
On Tue, Mar 29, 2016 at 06:47:42PM -0700, Russell Bryant wrote: > This commit applies a minor restructuring of this code to put the > localnet port specific code in its own block. This is mostly to make a > future patch easier to read. > > Signed-off-by: Russell Bryant Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATH v4] ovn-controller: Add external-id 'ovn-remote-probe-interval'
On Fri, Mar 25, 2016 at 02:18:34AM +0800, Huang Lei wrote: > From: Huang Lei > > Add a external-id 'ovn-remote-probe-interval' for setting the activity probe > interval of the json session from ovn-controller to the OVN southbound > database. > > Signed-off-by: Huang Lei Thanks. I made a few minor style changes and applied this to master. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/4] docs: OVSDB replication design document
Hi Ben, On 03/30/2016 05:27 PM, Ben Pfaff wrote: I'm in the midst of implementing high availability for OVSDB, based on the Raft algorithm. When I'm done, it should be possible to set up OVSDB clusters with automatic failover. Is this the same use case as your code? No, in this case the replication works by way of making the second server an OVSDB client, so it gets notified about all the changes in the remote server. In your case, how many OVSDB servers are required for replication to work? Raft has a quorum requirement of (N/2)+1 servers to be available in order for consensus to be reached. That makes the minimum number of servers 3, if you want to allow one to become unavailable. If you have 2 servers and one goes down, there's no quorum. The patch Mario is proposing has failover characteristics, not load-distribution characteristics: if one of the servers goes down, the other one can take over because it has a copy of the data up to the last notification the server was able to sent. Also, in the proposed patch transactions are not delayed until consensus is reached, but instead clients are talking only to the active server, which is applying transactions immediately. The active server is notifying the standby about the new data just like it's doing with any other client. Marcelo ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] netdev: avoid null pointer dereference in netdev_get_addrs()
On Wed, Mar 30, 2016 at 07:50:09PM -0400, Lance Richardson wrote:
> For some network interface configurations, it is possible for
> entries on the address list returned by getifaddrs() to have
> ifa->ifa_addr set to NULL. Add checks to avoid dereferencing
> ifa->ifa_addr in this case.
>
> Fixes: a8704b502785 ("tunneling: Handle multiple ip address for given
> device.")
> Signed-off-by: Lance Richardson
This fixes the same bug as a patch submitted slightly earlier
(https://patchwork.ozlabs.org/patch/603653/), so I applied that one
instead.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] netdev: verify ifa_addr is not NULL when iterating over getifaddrs
On Wed, Mar 30, 2016 at 06:12:17PM -0300, Thadeu Lima de Souza Cascardo wrote:
> Some point-to-point devices like TUN devices will not have an address, and
> while
> iterating over ifaddrs, its ifa_addr will be NULL. This patch fixes a crash
> when
> starting ovs-vswitchd on a system with such a device.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo
> Fixes: a8704b502785 ("tunneling: Handle multiple ip address for given
> device.")
> Cc: Pravin B Shelar
Thanks, applied to master.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/1] Rationalize ovn-ctl arguments.
On Wed, Mar 30, 2016 at 2:40 PM, Ben Pfaff wrote: > On Thu, Mar 24, 2016 at 09:45:31AM -0500, Ryan Moats wrote: > > Define OVN_NB_ADDR and OVN_SB_ADDR to hold IP address Rather > > than overload OVN_NB_PORT and OVN_SB_PORT. Also define > > OVN_NORTHD_LOGFILE to avoid overloading OVN_NORTHD_LOG. > > > > Signed-off-by: Ryan Moats > > This doesn't currently apply because of changes on master. > > Is there value in having separate arguments for address and port? Why > not just set up a single argument that has a value something like > "ptcp:6641" by default and let the caller override the whole thing to > something else. > > I'm starting to get really disturbed that ssl isn't the default here. > We need to add SSL config to these tables. -- Russell Bryant ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofp-util: fix indirekt group delete message with no buckets as per OF v1.3.5 onwards
On Thu, Mar 24, 2016 at 04:14:01PM +, László Sürü wrote: > Hi, > > Using OpenFlow v1.3.5 and onwards OVS 2.5.0 returns OFPGMFC_INVALID_GROUP > error when an INDIRECT type of group deletion requested, > although the delete message is according to OpenFlow v1.3.5 standard. > > The reason is the conflicting protocol check in Open vSwitch's OpenFlow > termination, that is > the indirect group delete command is once checked not to include any bucket > - as described in standard, > and secondly checked to include exactly one bucket - as also mandatory for > indirect groups. > > This error is not seen in the OVS internal make time verification (make > check), > as ovs-ofctl CLI tool does not accept group type as command argument. > Therefore indirect group delete works, although internally it is converted > into ALL group type. > > The fix is simply to ignore the mandatory single bucket check in case of > indirect group delete. > On the other hand the check is still executed in case of group addition or > modification. > > Moreover to this it is planned to extend 'ovs-ofctl del-groups' arguments > with a group 'type' as well to for test purposes. > > Signed-off-by: László Sűrű Applied to master, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] netdev: avoid null pointer dereference in netdev_get_addrs()
For some network interface configurations, it is possible for
entries on the address list returned by getifaddrs() to have
ifa->ifa_addr set to NULL. Add checks to avoid dereferencing
ifa->ifa_addr in this case.
Fixes: a8704b502785 ("tunneling: Handle multiple ip address for given device.")
Signed-off-by: Lance Richardson
---
lib/netdev.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/lib/netdev.c b/lib/netdev.c
index 31998b3..a57c238 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -1884,6 +1884,10 @@ netdev_get_addrs(const char dev[], struct in6_addr
**paddr,
for (ifa = if_addr_list; ifa; ifa = ifa->ifa_next) {
int family;
+if (!ifa->ifa_addr) {
+continue;
+}
+
family = ifa->ifa_addr->sa_family;
if (family == AF_INET || family == AF_INET6) {
if (!strncmp(ifa->ifa_name, dev, IFNAMSIZ)) {
@@ -1901,7 +1905,7 @@ netdev_get_addrs(const char dev[], struct in6_addr
**paddr,
for (ifa = if_addr_list; ifa; ifa = ifa->ifa_next) {
int family;
-if (strncmp(ifa->ifa_name, dev, IFNAMSIZ)) {
+if (!ifa->ifa_addr || strncmp(ifa->ifa_name, dev, IFNAMSIZ)) {
continue;
}
--
2.5.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn-controller: Loopback prevention flows for local ports only.
On Tue, Mar 29, 2016 at 04:55:11PM -0700, Han Zhou wrote: > Currently in physical_run() we added per-port loopback prevention > flows for all lports. The flows are actually required only for > local ports on the chassis. This change greatly reduces number of > flows in table 34. > > Signed-off-by: Han Zhou We could do this with a single logical flow of the form: match="(inport[0] && !outport[0]) || (!inport[0]) && outport[0]) || (inport[1] && !outport[1]) || (!inport[1]) && outport[1]) || ... (inport[15] && !outport[15]) || (!inport[15]) && outport[15])", actions="next;" That's a fairly inefficient solution due to the construction of the OVS classifier but we still might consider it at some point. I applied this to master, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Configure ops-ipsecd daemon to run at startup
On Wed, Mar 30, 2016 at 07:44:18PM +, Rugama, Jose Alejandro wrote: > I'm trying to configure ops-ipsecd daemon to run at startup like every else > daemon but I don't get it. > > My thought was that just adding ops-ipsecd to "_packagegroup-ops-base" on > packagegroup-openswitch.bb would be enough due to there is a .service file > here: > > # > yocto/openswitch/meta-distro-openswitch/recipes-ops/security/ops-ipsecd/ops-ipsecd.service > > But I'm wrong, Does anyone know What I'm missing? You might have the wrong mailing list. This doesn't seem to be a question about OVS. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC] ofp-actions: Extend the use of max_len in OUTPUT action.
On Wed, Mar 30, 2016 at 12:31:27PM -0700, pravin shelar wrote: > On Wed, Mar 30, 2016 at 10:24 AM, William Tu wrote: > > Hi Pravin, > > > > Thanks for the feedback. > > So another option is to add an new truncate action, and modify the size for > > all the following output packets. For example, the output:1 remains the > > original size and the output:2 and output:3 will have size 100. > > # ovs-ofctl add-flow br0 'actions=output:1, truncate:100, output:2, > > output:3' > > > > Or we can implement truncate_output action so that the truncate only scope > > at a specific output. For example: > > # ovs-ofctl add-flow br0 'actions=output:1, truncate_output: > > (2,max_len=100), output:3' > > So that only output:2 is re-sized to 100. > > > > Which one do you think is more useful? > > > The comment was about datapath action. I am fine with ofctl interface > that you have. The formatting/parsing syntax diverges from that for the other newer actions we've introduced; it's kind of weird to have parenthesized arguments following another argument. Therefore I'd prefer something more like: output(port=1, max_len=128) Also, I don't think it's reasonable to reuse OFPAT_OUTPUT for this. One simply cannot rely on every existing controller to fill something sensible in for the max_len field. I suggest inventing a new OpenFlow extension action. (That doesn't mean you have to change the syntax used in ovs-ofctl; we have plenty of examples where multiple OpenFlow actions have a common syntax.) ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/4] docs: OVSDB replication design document
On Tue, Mar 29, 2016 at 09:30:00PM +, Cabrera Vega, Mario Alberto wrote: > This patch series add database replication functionality between two > ovsdb-servers. The main idea is that an "active" server replicate its > database contents to an "standby" server in order to provide "fail > over" characteristics. Thanks for the patch series! I'm in the midst of implementing high availability for OVSDB, based on the Raft algorithm. When I'm done, it should be possible to set up OVSDB clusters with automatic failover. Is this the same use case as your code? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn-controller: Optimize processing for non-local datapath without patch ports.
On Wed, Mar 30, 2016 at 03:19:08PM -0700, Han Zhou wrote: > On Wed, Mar 30, 2016 at 2:01 PM, Ben Pfaff wrote: > > > > On Tue, Mar 29, 2016 at 12:26:18PM -0700, Han Zhou wrote: > > > For non-local datapaths, if there are no patch ports attached, it > > > means the lflows and port bindings would never be needed on the > > > Chassis. Since lflow_run() and physical_run() are the bottlenecks, > > > skipping the processing for such lflows and port bindings can save > > > significant amount of CPU, at the same time largely reduce the > > > number of rules in local openflow tables. This is specifically > > > useful when most of the lswitches are created for bridged networks, > > > where logical router is not used. > > > > > > Test precondition: > > > 2k hypervisors, 20k lports, 200 lswitches (each with a localnet > > > port). > > > > > > Test case: > > > step1: add 50 hypervisors (simulated on 1 BM with 40 cores), and > > >wait for flow updates complete on all new hypervisors. > > > step2: create a lswitch and a localnet port, create and bind 100 > > >lports evenly on these hypervisors. Repeat this 5 times. > > > > > > Before the change: > > > Step1 took around 20 minutes. > > > Step2 took 936 seconds. > > > > > > After the change: > > > Step1 took less than 1 minute: 20x faster. > > > Step2 took 464 seconds: 2x faster. > > > > > > Signed-off-by: Han Zhou > > > > This seems very reasonable to me. > > > > Can you think of a way to test this? > > Ben, since it is an optimization without any new feature, I think > regression should be good enough. Regarding regression, the current lrouter > test case should cover the case when logical patch ports exist: > > AT_SETUP([ovn -- 3 HVs, 3 LS, 3 lports/LS, 1 LR]) > > And localnet test case should cover the case when logical patch ports > doesn't exist. > > AT_SETUP([ovn -- 2 HVs, 4 lports/HV, localnet ports]) > > And I also tested the lrouter scenario in a openstack devstack environment > ensured that it won't break anything. OK, that's a good point. I applied this to master. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn-northd: Fix peering of routers.
On Mon, Mar 28, 2016 at 02:31:41PM -0700, Gurucharan Shetty wrote: > 1. Currently, the ovn-nb man page says that the 'peer' > in a logical_router_port table should point to the name > of the peer's logical router port. But the schema had declared > this column as a uuid. This looks not to be the intention as peers > for logical switches connected to routers is a name (and not a uuid). > So this patch changes the schema to be name. > > 2. In the southbound database, in the port_binding table, for a > logical_router_port, the peer was pointing back to itself. This > was causing ovn-controller to create patch ports where the peer > was wrongly pointing back to the source itself. This clearly looks > to be an error. So this patch fixes the peer in southbound database > to correclty point to the real peer. > > 3. ovn-northd.c currently skips generating logical flows to transfer > packets between two peers with comment about needing 'ARP for > neighboring routers'. It looked to me that since the router peer > is a logical object that has to be created in OVN-NB database, we > always need to statically assign the mac address. So this patch > picks the mac address from the database. > > Signed-off-by: Gurucharan Shetty Thank you. The lack of this feature was due to my laziness. I suspect that you should update ovn-northd.8.xml also. Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v6 00/12] Reconfigure netdev at runtime
On Wed, Mar 30, 2016 at 02:41:55PM +, Kavanagh, Mark B wrote: > BTW, any idea if Ben is planning to review this patchset soon? I'm not planning to review the whole series. Daniele asked me to look at patch 6 so I provided some feedback there. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v6 06/12] dpif-netdev: Wait an RCU grace period before freeing ports.
(I'm taking a look at this patch specifically because Daniele asked me; I'm not planning to review the whole series.) On Mon, Mar 28, 2016 at 12:41:40PM -0700, Daniele Di Proietto wrote: > The dpif-netdev datapath keeps ports in a cmap which is written only by > the main thread (holding port_mutex), but which is read concurrently by > many threads (most notably the pmd threads). > > When removing ports from the datapath we should postpone the deletion, > otherwise another thread might access invalid memory while reading the > cmap. > > This commit splits do_port_del() in do_port_remove() and > do_port_destroy(): the former removes the port from the cmap, while the > latter reclaims the memory and drops the reference to the underlying > netdev. s/del_port/port_del/ here: > dpif_netdev_del_port() now uses ovsrcu_synchronize() before calling > do_port_destroy(), to avoid memory corruption in concurrent readers. ovsrcu_synchronize() requires that nothing in the thread that calls it is relying on RCU to keep objects around. That means that no caller of dfpi_port_del()--there are a few of them--can rely on it. This is usually a risky assumption, especially because this assumption can change later. Is there reason to believe that it isn't important in all of these cases? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovs-dev, v11, 1/9] Fix missing tracking reording of row deletes.
I can't apply any more of the patches in this series because patch 2/9 is not in patchwork and didn't make it to my inbox either. I guess if the list doesn't work for you then you could always submit this as a pull request in github. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovs-dev, v11, 1/9] Fix missing tracking reording of row deletes.
Somehow, none of this series made it to my inbox, and only some of it
made it into patchwork. Thus, I'm probably breaking your email
threading here; sorry.
This seems at worst harmless to me. I applied it.
I'm CCing Shad, the author of the change tracking code, in case he
thinks this is wrong.
On Mon, Mar 28, 2016 at 08:43:11AM -0500, Ryan Moats wrote:
> From: RYAN D. MOATS
>
> Deletes need to be reordered as well as inserts and modifies,
> otherwise, following tracked changes will see out of order
> seqnos.
>
> Signed-off-by: RYAN D. MOATS
> ---
> lib/ovsdb-idl.c |5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index bfc133b..84ce14e 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -1589,9 +1589,10 @@ ovsdb_idl_row_destroy(struct ovsdb_idl_row *row)
> = row->table->change_seqno[OVSDB_IDL_CHANGE_DELETE]
> = row->table->idl->change_seqno + 1;
> }
> -if (list_is_empty(&row->track_node)) {
> -list_push_back(&row->table->track_list, &row->track_node);
> +if (!list_is_empty(&row->track_node)) {
> +list_remove(&row->track_node);
> }
> +list_push_back(&row->table->track_list, &row->track_node);
> }
> }
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn-controller: Optimize processing for non-local datapath without patch ports.
On Wed, Mar 30, 2016 at 2:01 PM, Ben Pfaff wrote: > > On Tue, Mar 29, 2016 at 12:26:18PM -0700, Han Zhou wrote: > > For non-local datapaths, if there are no patch ports attached, it > > means the lflows and port bindings would never be needed on the > > Chassis. Since lflow_run() and physical_run() are the bottlenecks, > > skipping the processing for such lflows and port bindings can save > > significant amount of CPU, at the same time largely reduce the > > number of rules in local openflow tables. This is specifically > > useful when most of the lswitches are created for bridged networks, > > where logical router is not used. > > > > Test precondition: > > 2k hypervisors, 20k lports, 200 lswitches (each with a localnet > > port). > > > > Test case: > > step1: add 50 hypervisors (simulated on 1 BM with 40 cores), and > >wait for flow updates complete on all new hypervisors. > > step2: create a lswitch and a localnet port, create and bind 100 > >lports evenly on these hypervisors. Repeat this 5 times. > > > > Before the change: > > Step1 took around 20 minutes. > > Step2 took 936 seconds. > > > > After the change: > > Step1 took less than 1 minute: 20x faster. > > Step2 took 464 seconds: 2x faster. > > > > Signed-off-by: Han Zhou > > This seems very reasonable to me. > > Can you think of a way to test this? Ben, since it is an optimization without any new feature, I think regression should be good enough. Regarding regression, the current lrouter test case should cover the case when logical patch ports exist: AT_SETUP([ovn -- 3 HVs, 3 LS, 3 lports/LS, 1 LR]) And localnet test case should cover the case when logical patch ports doesn't exist. AT_SETUP([ovn -- 2 HVs, 4 lports/HV, localnet ports]) And I also tested the lrouter scenario in a openstack devstack environment ensured that it won't break anything. If you are talking about testing the optimization, it is the scale-testing environment we are using. Lei is working on sharing it to public, and possibly integrate to OVS test so that it can find performance degradation during every regression. That will be a longer term goal but we are working with more people in the community to make that happen. -- Best regards, Han ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v12 3/9] Persist lport and mcgroup indexes
From: RYAN D. MOATS
Persisting these entries is a pre-requisite for incremental
processing.
Signed-off-by: RYAN D. MOATS
---
ovn/controller/lport.c | 139 ++
ovn/controller/lport.h | 20 +-
ovn/controller/ovn-controller.c | 14 ++--
3 files changed, 150 insertions(+), 23 deletions(-)
diff --git a/ovn/controller/lport.c b/ovn/controller/lport.c
index e09930a..5bcc05a 100644
--- a/ovn/controller/lport.c
+++ b/ovn/controller/lport.c
@@ -25,23 +25,52 @@ VLOG_DEFINE_THIS_MODULE(lport);
/* A logical port. */
struct lport {
-struct hmap_node name_node; /* Index by name. */
-struct hmap_node key_node; /* Index by (dp_key, port_key). */
+struct hmap_node name_node; /* Index by name. */
+struct hmap_node key_node; /* Index by (dp_key, port_key). */
+struct hmap_node uuid_node; /* Index by row uuid. */
+const struct uuid *uuid;
const struct sbrec_port_binding *pb;
};
void
-lport_index_init(struct lport_index *lports, struct ovsdb_idl *ovnsb_idl)
+lport_index_init(struct lport_index *lports)
{
hmap_init(&lports->by_name);
hmap_init(&lports->by_key);
+hmap_init(&lports->by_uuid);
+}
+
+void
+lport_index_remove(struct lport_index *lports, const struct uuid *uuid)
+{
+const struct lport *port = lport_lookup_by_uuid(lports, uuid);
+if (port) {
+hmap_remove(&lports->by_name, &port->name_node);
+hmap_remove(&lports->by_key, &port->key_node);
+hmap_remove(&lports->by_uuid, &port->uuid_node);
+free(port);
+}
+}
+void
+lport_index_fill(struct lport_index *lports, struct ovsdb_idl *ovnsb_idl)
+{
const struct sbrec_port_binding *pb;
-SBREC_PORT_BINDING_FOR_EACH (pb, ovnsb_idl) {
+SBREC_PORT_BINDING_FOR_EACH_TRACKED (pb, ovnsb_idl) {
+unsigned int del_seqno = sbrec_port_binding_row_get_seqno(pb,
+OVSDB_IDL_CHANGE_DELETE);
+
+/* if the row has a del_seqno > 0, then trying to process the
+ * row isn't going to work (as it has already been freed).
+ * What we can do is pass the row uuid to lport_index_remove()
+ * to remove the port. */
+if (del_seqno > 0) {
+lport_index_remove(lports, &pb->header_.uuid);
+reset_flow_processing();
+continue;
+}
+
if (lport_lookup_by_name(lports, pb->logical_port)) {
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
-VLOG_WARN_RL(&rl, "duplicate logical port name '%s'",
- pb->logical_port);
continue;
}
@@ -50,25 +79,38 @@ lport_index_init(struct lport_index *lports, struct
ovsdb_idl *ovnsb_idl)
hash_string(pb->logical_port, 0));
hmap_insert(&lports->by_key, &p->key_node,
hash_int(pb->tunnel_key, pb->datapath->tunnel_key));
+hmap_insert(&lports->by_uuid, &p->uuid_node,
+uuid_hash(&pb->header_.uuid));
+p->uuid = &pb->header_.uuid;
p->pb = pb;
reset_flow_processing();
}
}
void
-lport_index_destroy(struct lport_index *lports)
+lport_index_clear(struct lport_index *lports)
{
/* Destroy all of the "struct lport"s.
*
- * We don't have to remove the node from both indexes. */
+ * We have to remove the node from all indexes. */
struct lport *port, *next;
HMAP_FOR_EACH_SAFE (port, next, name_node, &lports->by_name) {
hmap_remove(&lports->by_name, &port->name_node);
+hmap_remove(&lports->by_key, &port->key_node);
+hmap_remove(&lports->by_uuid, &port->uuid_node);
free(port);
}
+reset_flow_processing();
+}
+
+void
+lport_index_destroy(struct lport_index *lports)
+{
+lport_index_clear(lports);
hmap_destroy(&lports->by_name);
hmap_destroy(&lports->by_key);
+hmap_destroy(&lports->by_uuid);
}
/* Finds and returns the lport with the given 'name', or NULL if no such lport
@@ -86,6 +128,20 @@ lport_lookup_by_name(const struct lport_index *lports,
const char *name)
return NULL;
}
+const struct lport *
+lport_lookup_by_uuid(const struct lport_index *lports,
+ const struct uuid *uuid)
+{
+const struct lport *lport;
+HMAP_FOR_EACH_WITH_HASH (lport, uuid_node, uuid_hash(uuid),
+ &lports->by_uuid) {
+if (uuid_equals(uuid, lport->uuid)) {
+return lport;
+}
+}
+return NULL;
+}
+
const struct sbrec_port_binding *
lport_lookup_by_key(const struct lport_index *lports,
uint32_t dp_key, uint16_t port_key)
@@ -103,44 +159,97 @@ lport_lookup_by_key(const struct lport_index *lports,
struct mcgroup {
struct hmap_node dp_name_node; /* Index by (logical datapath, name). */
+struct hmap_node uuid_node;/* Index by insert uuid. */
+const struct uuid *uuid;
const struct sbrec_multicast_group *mg;
};
void
-m
[ovs-dev] [PATCH v12 2/9] Make flow table persistent in ovn controller
From: RYAN D. MOATS
This is a prerequisite for incremental processing.
Signed-off-by: RYAN D. MOATS
---
lib/ofp-actions.c | 12 ++
lib/ofp-actions.h |2 +
ovn/controller/binding.c|1 +
ovn/controller/lflow.c | 65 --
ovn/controller/lflow.h |5 +-
ovn/controller/lport.c |3 +
ovn/controller/ofctrl.c | 264 +++---
ovn/controller/ofctrl.h | 16 ++-
ovn/controller/ovn-controller.c | 12 +-
ovn/controller/physical.c | 102 +++
ovn/controller/physical.h |2 +-
11 files changed, 357 insertions(+), 127 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 0438c62..6d3fa8c 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -7355,6 +7355,18 @@ ofpacts_equal(const struct ofpact *a, size_t a_len,
return a_len == b_len && !memcmp(a, b, a_len);
}
+uint32_t
+ofpacts_hash(const struct ofpact *a, size_t a_len, uint32_t basis)
+{
+size_t i;
+uint32_t interim = basis;
+for (i = 0; i < a_len; i += 4) {
+ uint32_t *term = (uint32_t *) ((uint8_t *)a+i);
+ interim = hash_add(*term, interim);
+}
+return hash_finish(interim, a_len);
+}
+
/* Finds the OFPACT_METER action, if any, in the 'ofpacts_len' bytes of
* 'ofpacts'. If found, returns its meter ID; if not, returns 0.
*
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 46818e3..630aa43 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -887,6 +887,8 @@ bool ofpacts_output_to_group(const struct ofpact[], size_t
ofpacts_len,
uint32_t group_id);
bool ofpacts_equal(const struct ofpact a[], size_t a_len,
const struct ofpact b[], size_t b_len);
+uint32_t ofpacts_hash(const struct ofpact a[], size_t a_len, uint32_t basis);
+
const struct mf_field *ofpact_get_mf_dst(const struct ofpact *ofpact);
uint32_t ofpacts_get_meter(const struct ofpact[], size_t ofpacts_len);
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index d3ca9c9..b001d40 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -133,6 +133,7 @@ add_local_datapath(struct hmap *local_datapaths,
struct local_datapath *ld = xzalloc(sizeof *ld);
hmap_insert(local_datapaths, &ld->hmap_node,
binding_rec->datapath->tunnel_key);
+reset_flow_processing();
}
static void
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 0614a54..dfd95e0 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -35,6 +35,14 @@ VLOG_DEFINE_THIS_MODULE(lflow);
/* Contains "struct expr_symbol"s for fields supported by OVN lflows. */
static struct shash symtab;
+static bool restart_flow_processing = false;
+
+void
+reset_flow_processing(void)
+{
+restart_flow_processing = true;
+}
+
static void
add_logical_register(struct shash *symtab, enum mf_field_id id)
{
@@ -198,12 +206,26 @@ static void
add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports,
const struct mcgroup_index *mcgroups,
const struct hmap *local_datapaths,
- const struct simap *ct_zones, struct hmap *flow_table)
+ const struct simap *ct_zones)
{
uint32_t conj_id_ofs = 1;
const struct sbrec_logical_flow *lflow;
-SBREC_LOGICAL_FLOW_FOR_EACH (lflow, ctx->ovnsb_idl) {
+SBREC_LOGICAL_FLOW_FOR_EACH_TRACKED (lflow, ctx->ovnsb_idl) {
+unsigned int del_seqno = sbrec_logical_flow_row_get_seqno(lflow,
+OVSDB_IDL_CHANGE_DELETE);
+unsigned int mod_seqno = sbrec_logical_flow_row_get_seqno(lflow,
+OVSDB_IDL_CHANGE_MODIFY);
+
+/* if the row has a del_seqno > 0, then trying to process the
+ * row isn't going to work (as it has already been freed).
+ * What we can do is to pass a pointer to the ovs_idl_row to
+ * ofctrl_remove_flows() to remove flows from this record */
+if (del_seqno > 0) {
+ofctrl_remove_flows(&lflow->header_.uuid);
+continue;
+}
+
/* Determine translation of logical table IDs to physical table IDs. */
bool ingress = !strcmp(lflow->pipeline, "ingress");
@@ -321,8 +343,8 @@ add_logical_flows(struct controller_ctx *ctx, const struct
lport_index *lports,
m->match.flow.conj_id += conj_id_ofs;
}
if (!m->n) {
-ofctrl_add_flow(flow_table, ptable, lflow->priority,
-&m->match, &ofpacts);
+ofctrl_add_flow(ptable, lflow->priority, &m->match, &ofpacts,
+&lflow->header_.uuid, mod_seqno);
} else {
uint64_t conj_stubs[64 / 8];
struct ofpbuf conj;
@@ -337,8 +359,8 @@ add_logical_flows(struct controller_ctx *ctx, const struct
lport_index *lports,
[ovs-dev] [PATCH v12 4/9] Persist local_datapaths
From: RYAN D. MOATS
Persist local_datapaths across runs so that a change can be used
as a trigger to reset incremental flow processing.
Signed-off-by: RYAN D. MOATS
---
ovn/controller/binding.c| 42 --
ovn/controller/ovn-controller.c | 15 +++--
ovn/controller/ovn-controller.h |1 +
ovn/controller/patch.c |3 +-
4 files changed, 46 insertions(+), 15 deletions(-)
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index b001d40..0d5940f 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -121,9 +121,32 @@ update_ct_zones(struct sset *lports, struct simap
*ct_zones,
}
}
+/* Contains "struct local_datpath" nodes whose hash values are the
+ * row uuids of datapaths with at least one local port binding. */
+struct hmap local_datapaths_by_uuid =
+HMAP_INITIALIZER(&local_datapaths_by_uuid);
+
+static struct local_datapath *
+local_datapath_lookup_by_uuid(const struct uuid *uuid)
+{
+return hmap_first_with_hash(&local_datapaths_by_uuid, uuid_hash(uuid));
+}
+
+static void
+remove_local_datapath(struct hmap *local_datapaths, const struct uuid *uuid)
+{
+struct local_datapath *ld = local_datapath_lookup_by_uuid(uuid);
+if (ld) {
+hmap_remove(local_datapaths, &ld->hmap_node);
+hmap_remove(&local_datapaths_by_uuid, &ld->uuid_hmap_node);
+reset_flow_processing();
+}
+}
+
static void
add_local_datapath(struct hmap *local_datapaths,
-const struct sbrec_port_binding *binding_rec)
+const struct sbrec_port_binding *binding_rec,
+const struct uuid *uuid)
{
if (hmap_first_with_hash(local_datapaths,
binding_rec->datapath->tunnel_key)) {
@@ -133,6 +156,8 @@ add_local_datapath(struct hmap *local_datapaths,
struct local_datapath *ld = xzalloc(sizeof *ld);
hmap_insert(local_datapaths, &ld->hmap_node,
binding_rec->datapath->tunnel_key);
+hmap_insert(&local_datapaths_by_uuid, &ld->uuid_hmap_node,
+uuid_hash(uuid));
reset_flow_processing();
}
@@ -177,7 +202,17 @@ binding_run(struct controller_ctx *ctx, const struct
ovsrec_bridge *br_int,
/* Run through each binding record to see if it is resident on this
* chassis and update the binding accordingly. This includes both
* directly connected logical ports and children of those ports. */
-SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) {
+SBREC_PORT_BINDING_FOR_EACH_TRACKED(binding_rec, ctx->ovnsb_idl) {
+unsigned int del_seqno = sbrec_port_binding_row_get_seqno(binding_rec,
+OVSDB_IDL_CHANGE_DELETE);
+
+/* if the row has a del_seqno > 0, then trying to process the row
+ * isn't going to work (as it has already been freed) */
+if (del_seqno > 0) {
+remove_local_datapath(local_datapaths, &binding_rec->header_.uuid);
+continue;
+}
+
const struct ovsrec_interface *iface_rec
= shash_find_and_delete(&lports, binding_rec->logical_port);
if (iface_rec
@@ -187,7 +222,8 @@ binding_run(struct controller_ctx *ctx, const struct
ovsrec_bridge *br_int,
/* Add child logical port to the set of all local ports. */
sset_add(&all_lports, binding_rec->logical_port);
}
-add_local_datapath(local_datapaths, binding_rec);
+add_local_datapath(local_datapaths, binding_rec,
+ &binding_rec->header_.uuid);
if (iface_rec && ctx->ovs_idl_txn) {
update_qos(iface_rec, binding_rec);
}
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 6edc04c..037178b 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -201,6 +201,10 @@ get_ovnsb_remote(struct ovsdb_idl *ovs_idl)
struct lport_index lports;
struct mcgroup_index mcgroups;
+/* Contains "struct local_datpath" nodes whose hash values are the
+ * tunnel_key of datapaths with at least one local port binding. */
+struct hmap local_datapaths = HMAP_INITIALIZER(&local_datapaths);
+
int
main(int argc, char *argv[])
{
@@ -288,10 +292,6 @@ main(int argc, char *argv[])
.ovnsb_idl_txn = ovsdb_idl_loop_run(&ovnsb_idl_loop),
};
-/* Contains "struct local_datpath" nodes whose hash values are the
- * tunnel_key of datapaths with at least one local port binding. */
-struct hmap local_datapaths = HMAP_INITIALIZER(&local_datapaths);
-
const struct ovsrec_bridge *br_int = get_br_int(&ctx);
const char *chassis_id = get_chassis_id(ctx.ovs_idl);
@@ -322,13 +322,6 @@ main(int argc, char *argv[])
ofctrl_put();
}
-struct local_datapath *cur_node, *next_node;
-HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node, &local_datapaths) {
-hmap_remove(&local
[ovs-dev] [PATCH v12 7/9] Convert binding_run to incremental processing.
From: RYAN D. MOATS
Persist all_lports structure and ensure that binding_run
resets to process the entire port binding table when chassis
are added/removed or when get_local_iface_ids finds new ports
on the local vswitch.
Signed-off-by: RYAN D. MOATS
---
ovn/controller/binding.c| 57 ++
ovn/controller/binding.h|1 +
ovn/controller/encaps.c |2 +
ovn/controller/ovn-controller.h |1 +
4 files changed, 49 insertions(+), 12 deletions(-)
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index 58402fd..802abe4 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -28,6 +28,16 @@
VLOG_DEFINE_THIS_MODULE(binding);
+struct sset all_lports = SSET_INITIALIZER(&all_lports);
+
+unsigned int binding_seqno = 0;
+
+void
+reset_binding_seqno(void)
+{
+binding_seqno = 0;
+}
+
void
binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
{
@@ -73,6 +83,10 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int,
struct shash *lports)
continue;
}
shash_add(lports, iface_id, iface_rec);
+if (!sset_find(&all_lports, iface_id)) {
+sset_add(&all_lports, iface_id);
+reset_binding_seqno();
+}
}
}
}
@@ -130,7 +144,12 @@ struct hmap local_datapaths_by_uuid =
static struct local_datapath *
local_datapath_lookup_by_uuid(const struct uuid *uuid)
{
-return hmap_first_with_hash(&local_datapaths_by_uuid, uuid_hash(uuid));
+struct hmap_node *ld_node = hmap_first_with_hash(&local_datapaths_by_uuid,
+ uuid_hash(uuid));
+if (ld_node) {
+return CONTAINER_OF(ld_node, struct local_datapath, uuid_hmap_node);
+}
+return NULL;
}
static void
@@ -138,8 +157,13 @@ remove_local_datapath(struct hmap *local_datapaths, const
struct uuid *uuid)
{
struct local_datapath *ld = local_datapath_lookup_by_uuid(uuid);
if (ld) {
+if (ld->logical_port) {
+sset_find_and_delete(&all_lports, ld->logical_port);
+free(ld->logical_port);
+}
hmap_remove(local_datapaths, &ld->hmap_node);
hmap_remove(&local_datapaths_by_uuid, &ld->uuid_hmap_node);
+free(ld);
reset_flow_processing();
}
}
@@ -155,6 +179,7 @@ add_local_datapath(struct hmap *local_datapaths,
}
struct local_datapath *ld = xzalloc(sizeof *ld);
+ld->logical_port = xstrdup(binding_rec->logical_port);
hmap_insert(local_datapaths, &ld->hmap_node,
binding_rec->datapath->tunnel_key);
hmap_insert(&local_datapaths_by_uuid, &ld->uuid_hmap_node,
@@ -194,26 +219,39 @@ binding_run(struct controller_ctx *ctx, const struct
ovsrec_bridge *br_int,
* We'll remove our chassis from all port binding records below. */
}
-struct sset all_lports = SSET_INITIALIZER(&all_lports);
-struct shash_node *node;
-SHASH_FOR_EACH (node, &lports) {
-sset_add(&all_lports, node->name);
-}
-
/* Run through each binding record to see if it is resident on this
* chassis and update the binding accordingly. This includes both
* directly connected logical ports and children of those ports. */
SBREC_PORT_BINDING_FOR_EACH_TRACKED(binding_rec, ctx->ovnsb_idl) {
unsigned int del_seqno = sbrec_port_binding_row_get_seqno(binding_rec,
OVSDB_IDL_CHANGE_DELETE);
+unsigned int mod_seqno = sbrec_port_binding_row_get_seqno(binding_rec,
+OVSDB_IDL_CHANGE_MODIFY);
+unsigned int ins_seqno = sbrec_port_binding_row_get_seqno(binding_rec,
+OVSDB_IDL_CHANGE_INSERT);
+
+if (del_seqno <= binding_seqno && mod_seqno <= binding_seqno
+&& ins_seqno <= binding_seqno) {
+continue;
+}
/* if the row has a del_seqno > 0, then trying to process the row
* isn't going to work (as it has already been freed) */
if (del_seqno > 0) {
remove_local_datapath(local_datapaths, &binding_rec->header_.uuid);
+if (del_seqno >= binding_seqno) {
+binding_seqno = del_seqno;
+}
continue;
}
+if (mod_seqno >= binding_seqno) {
+binding_seqno = mod_seqno;
+}
+if (ins_seqno >= binding_seqno) {
+binding_seqno = ins_seqno;
+}
+
const struct ovsrec_interface *iface_rec
= shash_find_and_delete(&lports, binding_rec->logical_port);
if (iface_rec
@@ -259,14 +297,9 @@ binding_run(struct controller_ctx *ctx, const struct
ovsrec_bridge *br_int,
}
}
-SHASH_FOR_EACH (node, &lports) {
-VLOG_DBG("No port binding record for lport %s", node->name);
-}
-
update_ct_zones(&all_lports, ct_zones, ct_zone_bitmap);
shash_destroy(&lports);
-sset_destroy(&all_lports);
}
/* Return
[ovs-dev] [PATCH v12 6/9] Change encaps_run to work incrementally
From: RYAN D. MOATS
Side effects include tunnel context being persisted.
Signed-off-by: RYAN D. MOATS
---
ovn/controller/encaps.c | 161 +--
1 files changed, 114 insertions(+), 47 deletions(-)
diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
index dfb11c0..8395051 100644
--- a/ovn/controller/encaps.c
+++ b/ovn/controller/encaps.c
@@ -15,6 +15,7 @@
#include
#include "encaps.h"
+#include "lflow.h"
#include "lib/hash.h"
#include "lib/sset.h"
@@ -49,6 +50,7 @@ struct tunnel_ctx {
* generated we remove them. After generating all the rows, any
* remaining in 'tunnel_hmap' must be deleted from the database. */
struct hmap tunnel_hmap;
+struct hmap tunnel_hmap_by_uuid;
/* Names of all ports in the bridge, to allow checking uniqueness when
* adding a new tunnel. */
@@ -58,8 +60,18 @@ struct tunnel_ctx {
const struct ovsrec_bridge *br_int;
};
+struct tunnel_ctx tc = {
+.tunnel_hmap = HMAP_INITIALIZER(&tc.tunnel_hmap),
+.tunnel_hmap_by_uuid = HMAP_INITIALIZER(&tc.tunnel_hmap_by_uuid),
+.port_names = SSET_INITIALIZER(&tc.port_names),
+};
+
+unsigned int encaps_seqno = 0;
+
struct port_hash_node {
struct hmap_node node;
+struct hmap_node uuid_node;
+const struct uuid *uuid;
const struct ovsrec_port *port;
const struct ovsrec_bridge *bridge;
};
@@ -92,7 +104,7 @@ port_hash_rec(const struct ovsrec_port *port)
}
static char *
-tunnel_create_name(struct tunnel_ctx *tc, const char *chassis_id)
+tunnel_create_name(const char *chassis_id)
{
int i;
@@ -100,7 +112,7 @@ tunnel_create_name(struct tunnel_ctx *tc, const char
*chassis_id)
char *port_name;
port_name = xasprintf("ovn-%.6s-%x", chassis_id, i);
-if (!sset_contains(&tc->port_names, port_name)) {
+if (!sset_contains(&tc.port_names, port_name)) {
return port_name;
}
@@ -110,19 +122,32 @@ tunnel_create_name(struct tunnel_ctx *tc, const char
*chassis_id)
return NULL;
}
+static struct port_hash_node *
+port_lookup_by_uuid(const struct uuid *uuid)
+{
+struct hmap_node *node = hmap_first_with_hash(&tc.tunnel_hmap_by_uuid,
+ uuid_hash(uuid));
+if (node) {
+return CONTAINER_OF(node, struct port_hash_node, uuid_node);
+}
+return NULL;
+}
static void
-tunnel_add(struct tunnel_ctx *tc, const char *new_chassis_id,
+tunnel_add(const struct sbrec_chassis *chassis_rec,
const struct sbrec_encap *encap)
{
struct port_hash_node *hash_node;
+const char *new_chassis_id = chassis_rec->name;
+
+/* Check whether such a row already exists in OVS. If so, update
+ * the uuid field and insert into the by uuid hashmap. If not,
+ * create the tunnel */
-/* Check whether such a row already exists in OVS. If so, remove it
- * from 'tc->tunnel_hmap' and we're done. */
HMAP_FOR_EACH_WITH_HASH (hash_node, node,
port_hash(new_chassis_id,
encap->type, encap->ip),
- &tc->tunnel_hmap) {
+ &tc.tunnel_hmap) {
const struct ovsrec_port *port = hash_node->port;
const char *chassis_id = smap_get(&port->external_ids,
"ovn-chassis-id");
@@ -142,8 +167,12 @@ tunnel_add(struct tunnel_ctx *tc, const char
*new_chassis_id,
if (!strcmp(new_chassis_id, chassis_id)
&& !strcmp(encap->type, iface->type)
&& !strcmp(encap->ip, ip)) {
-hmap_remove(&tc->tunnel_hmap, &hash_node->node);
-free(hash_node);
+
+hash_node->uuid = &chassis_rec->header_.uuid;
+if (!port_lookup_by_uuid(hash_node->uuid)) {
+hmap_insert(&tc.tunnel_hmap_by_uuid, &hash_node->uuid_node,
+uuid_hash(hash_node->uuid));
+}
return;
}
}
@@ -155,14 +184,14 @@ tunnel_add(struct tunnel_ctx *tc, const char
*new_chassis_id,
char *port_name;
size_t i;
-port_name = tunnel_create_name(tc, new_chassis_id);
+port_name = tunnel_create_name(new_chassis_id);
if (!port_name) {
VLOG_WARN("Unable to allocate unique name for '%s' tunnel",
new_chassis_id);
return;
}
-iface = ovsrec_interface_insert(tc->ovs_txn);
+iface = ovsrec_interface_insert(tc.ovs_txn);
ovsrec_interface_set_name(iface, port_name);
ovsrec_interface_set_type(iface, encap->type);
smap_add(&options, "remote_ip", encap->ip);
@@ -170,23 +199,25 @@ tunnel_add(struct tunnel_ctx *tc, const char
*new_chassis_id,
ovsrec_interface_set_options(iface, &options);
smap_destroy(&options);
-port = ovsrec_port_insert(tc->ovs_txn);
+port = ovsrec_port_insert(tc.ovs_txn);
ovsrec_port_set_name(port, port_
[ovs-dev] [PATCH v12 8/9] Reset lflow processing when adding/removing patch ports
From: RYAN D. MOATS As lflow processing is incremental, reset it whenever a patch port is added or removed. Signed-off-by: RYAN D. MOATS --- ovn/controller/patch.c |4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c index 9c519b0..e8f107a 100644 --- a/ovn/controller/patch.c +++ b/ovn/controller/patch.c @@ -15,6 +15,7 @@ #include +#include "lflow.h" #include "patch.h" #include "hash.h" @@ -92,7 +93,7 @@ create_patch_port(struct controller_ctx *ctx, ports[src->n_ports] = port; ovsrec_bridge_verify_ports(src); ovsrec_bridge_set_ports(src, ports, src->n_ports + 1); - +reset_flow_processing(); free(ports); } @@ -125,6 +126,7 @@ remove_port(struct controller_ctx *ctx, return; } } +reset_flow_processing(); } /* Obtains external-ids:ovn-bridge-mappings from OVSDB and adds patch ports for -- 1.7.1 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v12 0/9] Add incremental processing
From: RYAN D. MOATS Delta from v11 has more optimizations based on the profiling work done by Lei Huang and Han Zhou from eBay. In patch 2/9, we only need to check for a OF rule whose match is "close" to the proposed OF rule via the UUID hash during a modification operation. This saves allocating a hash in memory, makes the lookup quicker, and avoids calling it unnecessarily. In addition some of the changes in patch 5 were pulled forward to patch 2 of the series to avoid the changes to patches 2 and 5 in v10, making them cleaner RYAN D. MOATS (9): Fix missing tracking reording of row deletes. Make flow table persistent in ovn controller Persist lport and mcgroup indexes Persist local_datapaths Add incremental proessing to lflow_run Change encaps_run to work incrementally Convert binding_run to incremental processing. Reset lflow processing when adding/removing patch ports Change physical_run to incremental processing lib/ofp-actions.c | 12 ++ lib/ofp-actions.h |2 + lib/ovsdb-idl.c |5 +- ovn/controller/binding.c| 99 +-- ovn/controller/binding.h|1 + ovn/controller/encaps.c | 163 +--- ovn/controller/lflow.c | 118 +++--- ovn/controller/lflow.h |6 +- ovn/controller/lport.c | 142 +++--- ovn/controller/lport.h | 20 +++- ovn/controller/ofctrl.c | 264 +++--- ovn/controller/ofctrl.h | 18 ++- ovn/controller/ovn-controller.c | 42 +++ ovn/controller/ovn-controller.h |2 + ovn/controller/patch.c |7 +- ovn/controller/physical.c | 195 ++--- ovn/controller/physical.h |4 +- 17 files changed, 853 insertions(+), 247 deletions(-) ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v12 5/9] Add incremental proessing to lflow_run
From: RYAN D. MOATS
This code changes lflow_run to do incremental process of the
logical flow table rather than processing the full table each run.
Signed-off-by: RYAN D. MOATS
---
ovn/controller/binding.c|1 +
ovn/controller/lflow.c | 52 --
ovn/controller/lflow.h |1 +
ovn/controller/ofctrl.h |2 +
ovn/controller/ovn-controller.c |3 +-
5 files changed, 54 insertions(+), 5 deletions(-)
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index 0d5940f..58402fd 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -15,6 +15,7 @@
#include
#include "binding.h"
+#include "lflow.h"
#include "lib/bitmap.h"
#include "lib/hmap.h"
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index dfd95e0..c77a888 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -201,6 +201,8 @@ is_switch(const struct sbrec_datapath_binding *ldp)
}
+unsigned int lflow_logical_flow_seqno = 0;
+
/* Adds the logical flows from the Logical_Flow table to 'flow_table'. */
static void
add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports,
@@ -214,18 +216,36 @@ add_logical_flows(struct controller_ctx *ctx, const
struct lport_index *lports,
SBREC_LOGICAL_FLOW_FOR_EACH_TRACKED (lflow, ctx->ovnsb_idl) {
unsigned int del_seqno = sbrec_logical_flow_row_get_seqno(lflow,
OVSDB_IDL_CHANGE_DELETE);
+unsigned int ins_seqno = sbrec_logical_flow_row_get_seqno(lflow,
+OVSDB_IDL_CHANGE_INSERT);
unsigned int mod_seqno = sbrec_logical_flow_row_get_seqno(lflow,
OVSDB_IDL_CHANGE_MODIFY);
+if (del_seqno <= lflow_logical_flow_seqno
+&& mod_seqno <= lflow_logical_flow_seqno
+&& ins_seqno <= lflow_logical_flow_seqno) {
+continue;
+}
+
/* if the row has a del_seqno > 0, then trying to process the
* row isn't going to work (as it has already been freed).
* What we can do is to pass a pointer to the ovs_idl_row to
* ofctrl_remove_flows() to remove flows from this record */
if (del_seqno > 0) {
ofctrl_remove_flows(&lflow->header_.uuid);
+if (del_seqno > lflow_logical_flow_seqno) {
+lflow_logical_flow_seqno = del_seqno;
+}
continue;
}
+if (mod_seqno > lflow_logical_flow_seqno) {
+lflow_logical_flow_seqno = mod_seqno;
+}
+if (ins_seqno > lflow_logical_flow_seqno) {
+lflow_logical_flow_seqno = ins_seqno;
+}
+
/* Determine translation of logical table IDs to physical table IDs. */
bool ingress = !strcmp(lflow->pipeline, "ingress");
@@ -364,7 +384,6 @@ add_logical_flows(struct controller_ctx *ctx, const struct
lport_index *lports,
ofpbuf_uninit(&conj);
}
}
-
/* Clean up. */
expr_matches_destroy(&matches);
ofpbuf_uninit(&ofpacts);
@@ -385,6 +404,8 @@ put_load(const uint8_t *data, size_t len,
bitwise_one(&sf->mask, sf->field->n_bytes, ofs, n_bits);
}
+unsigned int lflow_mac_binding_seqno = 0;
+
/* Adds an OpenFlow flow to 'flow_table' for each MAC binding in the OVN
* southbound database, using 'lports' to resolve logical port names to
* numbers. */
@@ -401,18 +422,40 @@ add_neighbor_flows(struct controller_ctx *ctx,
SBREC_MAC_BINDING_FOR_EACH_TRACKED (b, ctx->ovnsb_idl) {
unsigned int del_seqno = sbrec_mac_binding_row_get_seqno(b,
OVSDB_IDL_CHANGE_DELETE);
+unsigned int ins_seqno = sbrec_mac_binding_row_get_seqno(b,
+OVSDB_IDL_CHANGE_MODIFY);
unsigned int mod_seqno = sbrec_mac_binding_row_get_seqno(b,
OVSDB_IDL_CHANGE_MODIFY);
+/* TODO (regXboi) the following hunk of code is commented out
+ * because the above seqnos all return 0 at this point.
+ * Once that issue is fixed, this code can be uncommented
+ * and this comment removed.
+if (del_seqno <= lflow_mac_binding_seqno
+&& mod_seqno <= lflow_mac_binding_seqno
+&& ins_seqno <= lflow_mac_binding_seqno) {
+continue;
+}
+*/
+
/* if the row has a del_seqno > 0, then trying to process the
* row isn't going to work (as it has already been freed).
* What we can do pass a pointer to the ovs_idl_row to
* ofctrl_remove_flows() to remove the flow */
if (del_seqno > 0) {
ofctrl_remove_flows(&b->header_.uuid);
+if (del_seqno > lflow_mac_binding_seqno) {
+lflow_mac_binding_seqno = del_seqno;
+}
continue;
}
+if (mod_seqno > lflow_mac_binding_seqno) {
+lflow_mac_binding_seqno = mod_seqno;
+}
+if (ins_seqno > lflow_mac_binding_seqno) {
[ovs-dev] [PATCH v12 9/9] Change physical_run to incremental processing
From: RYAN D. MOATS
Persist localvif_to_ofport and tunnels structures and change
physical_run to incremental processing.
Signed-off-by: RYAN D. MOATS
---
ovn/controller/lflow.c|3 +
ovn/controller/physical.c | 99 +++-
ovn/controller/physical.h |2 +
3 files changed, 83 insertions(+), 21 deletions(-)
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index c77a888..f20bd55 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -26,6 +26,7 @@
#include "ovn/lib/expr.h"
#include "ovn/lib/ovn-sb-idl.h"
#include "packets.h"
+#include "physical.h"
#include "simap.h"
VLOG_DEFINE_THIS_MODULE(lflow);
@@ -501,6 +502,8 @@ lflow_run(struct controller_ctx *ctx, const struct
lport_index *lports,
lflow_logical_flow_seqno = 0;
lflow_mac_binding_seqno = 0;
ovn_flow_table_clear();
+localvif_to_ofports_clear();
+tunnels_clear();
restart_flow_processing = false;
}
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 33075e9..4a74f21 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -48,6 +48,13 @@ physical_register_ovs_idl(struct ovsdb_idl *ovs_idl)
ovsdb_idl_add_column(ovs_idl, &ovsrec_interface_col_external_ids);
}
+struct uuid *hc_uuid = NULL; // uuid to identify OF flows not associated with
+ // ovsdb rows.
+
+struct simap localvif_to_ofport = SIMAP_INITIALIZER(&localvif_to_ofport);
+struct hmap tunnels = HMAP_INITIALIZER(&tunnels);
+unsigned int port_binding_seqno = 0;
+
/* Maps from a chassis to the OpenFlow port number of the tunnel that can be
* used to reach that chassis. */
struct chassis_tunnel {
@@ -57,6 +64,28 @@ struct chassis_tunnel {
enum chassis_tunnel_type type;
};
+void
+localvif_to_ofports_clear(void)
+{
+simap_clear(&localvif_to_ofport);
+}
+
+void
+tunnels_clear(void)
+{
+struct chassis_tunnel *tun, *next;
+HMAP_FOR_EACH_SAFE (tun, next, hmap_node, &tunnels) {
+hmap_remove(&tunnels, &tun->hmap_node);
+free(tun);
+}
+}
+
+static void
+reset_physical_seqnos(void)
+{
+port_binding_seqno = 0;
+}
+
static struct chassis_tunnel *
chassis_tunnel_find(struct hmap *tunnels, const char *chassis_id)
{
@@ -144,17 +173,12 @@ get_localnet_port(struct hmap *local_datapaths, int64_t
tunnel_key)
return ld ? ld->localnet_port : NULL;
}
-struct uuid *hc_uuid = NULL; // uuid to identify OF flows not associated with
- // ovsdb rows.
-
void
physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
const struct ovsrec_bridge *br_int, const char *this_chassis_id,
const struct simap *ct_zones,
struct hmap *local_datapaths)
{
-struct simap localvif_to_ofport = SIMAP_INITIALIZER(&localvif_to_ofport);
-struct hmap tunnels = HMAP_INITIALIZER(&tunnels);
if (!hc_uuid) {
hc_uuid = xmalloc(sizeof(struct uuid));
uuid_generate(hc_uuid);
@@ -194,11 +218,21 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id
mff_ovn_geneve,
bool is_patch = !strcmp(iface_rec->type, "patch");
if (is_patch && localnet) {
/* localnet patch ports can be handled just like VIFs. */
-simap_put(&localvif_to_ofport, localnet, ofport);
+struct simap_node *old = simap_find(&localvif_to_ofport,
+localnet);
+if (!old || old->data != ofport) {
+simap_put(&localvif_to_ofport, localnet, ofport);
+reset_physical_seqnos();
+}
break;
} else if (is_patch && logpatch) {
/* Logical patch ports can be handled just like VIFs. */
-simap_put(&localvif_to_ofport, logpatch, ofport);
+struct simap_node *old = simap_find(&localvif_to_ofport,
+logpatch);
+if (!old || old->data != ofport) {
+simap_put(&localvif_to_ofport, logpatch, ofport);
+reset_physical_seqnos();
+}
break;
} else if (chassis_id) {
enum chassis_tunnel_type tunnel_type;
@@ -215,18 +249,38 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id
mff_ovn_geneve,
continue;
}
-struct chassis_tunnel *tun = xmalloc(sizeof *tun);
-hmap_insert(&tunnels, &tun->hmap_node,
-hash_string(chassis_id, 0));
-tun->chassis_id = chassis_id;
-tun->ofport = u16_to_ofp(ofport);
-tun->type = tunnel_type;
+struct chassis_tunnel *old = chassis_tunnel_find(&tunnels,
+
[ovs-dev] [PATCH v12 1/9] Fix missing tracking reording of row deletes.
From: RYAN D. MOATS
Deletes need to be reordered as well as inserts and modifies,
otherwise, following tracked changes will see out of order
seqnos.
Signed-off-by: RYAN D. MOATS
---
lib/ovsdb-idl.c |5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index bfc133b..84ce14e 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -1589,9 +1589,10 @@ ovsdb_idl_row_destroy(struct ovsdb_idl_row *row)
= row->table->change_seqno[OVSDB_IDL_CHANGE_DELETE]
= row->table->idl->change_seqno + 1;
}
-if (list_is_empty(&row->track_node)) {
-list_push_back(&row->table->track_list, &row->track_node);
+if (!list_is_empty(&row->track_node)) {
+list_remove(&row->track_node);
}
+list_push_back(&row->table->track_list, &row->track_node);
}
}
--
1.7.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] netdev: verify ifa_addr is not NULL when iterating over getifaddrs
Some point-to-point devices like TUN devices will not have an address, and while
iterating over ifaddrs, its ifa_addr will be NULL. This patch fixes a crash when
starting ovs-vswitchd on a system with such a device.
Signed-off-by: Thadeu Lima de Souza Cascardo
Fixes: a8704b502785 ("tunneling: Handle multiple ip address for given device.")
Cc: Pravin B Shelar
---
lib/netdev.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/lib/netdev.c b/lib/netdev.c
index 95fdbc7..928f46f 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -1882,12 +1882,14 @@ netdev_get_addrs(const char dev[], struct in6_addr
**paddr,
}
for (ifa = if_addr_list; ifa; ifa = ifa->ifa_next) {
-int family;
+if (ifa->ifa_addr != NULL) {
+int family;
-family = ifa->ifa_addr->sa_family;
-if (family == AF_INET || family == AF_INET6) {
-if (!strncmp(ifa->ifa_name, dev, IFNAMSIZ)) {
-cnt++;
+family = ifa->ifa_addr->sa_family;
+if (family == AF_INET || family == AF_INET6) {
+if (!strncmp(ifa->ifa_name, dev, IFNAMSIZ)) {
+cnt++;
+}
}
}
}
@@ -1901,7 +1903,7 @@ netdev_get_addrs(const char dev[], struct in6_addr
**paddr,
for (ifa = if_addr_list; ifa; ifa = ifa->ifa_next) {
int family;
-if (strncmp(ifa->ifa_name, dev, IFNAMSIZ)) {
+if (strncmp(ifa->ifa_name, dev, IFNAMSIZ) || ifa->ifa_addr == NULL) {
continue;
}
--
2.5.0
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn-controller: Optimize processing for non-local datapath without patch ports.
On Tue, Mar 29, 2016 at 12:26:18PM -0700, Han Zhou wrote: > For non-local datapaths, if there are no patch ports attached, it > means the lflows and port bindings would never be needed on the > Chassis. Since lflow_run() and physical_run() are the bottlenecks, > skipping the processing for such lflows and port bindings can save > significant amount of CPU, at the same time largely reduce the > number of rules in local openflow tables. This is specifically > useful when most of the lswitches are created for bridged networks, > where logical router is not used. > > Test precondition: > 2k hypervisors, 20k lports, 200 lswitches (each with a localnet > port). > > Test case: > step1: add 50 hypervisors (simulated on 1 BM with 40 cores), and >wait for flow updates complete on all new hypervisors. > step2: create a lswitch and a localnet port, create and bind 100 >lports evenly on these hypervisors. Repeat this 5 times. > > Before the change: > Step1 took around 20 minutes. > Step2 took 936 seconds. > > After the change: > Step1 took less than 1 minute: 20x faster. > Step2 took 464 seconds: 2x faster. > > Signed-off-by: Han Zhou This seems very reasonable to me. Can you think of a way to test this? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC PATCH] tunneling: Improving vxlan performance using DPDK flow director feature.
On Wed, Mar 30, 2016 at 10:27 AM, Chandran, Sugesh wrote: >> -Original Message- >> From: Jesse Gross [mailto:je...@kernel.org] >> Sent: Wednesday, March 30, 2016 1:44 AM >> To: Chandran, Sugesh >> Cc: dev@openvswitch.org >> Subject: Re: [ovs-dev] [RFC PATCH] tunneling: Improving vxlan performance >> using DPDK flow director feature. >> >> On Tue, Mar 29, 2016 at 12:43 AM, Chandran, Sugesh >> wrote: >> >> -Original Message- >> >> From: Jesse Gross [mailto:je...@kernel.org] >> >> Sent: Friday, March 25, 2016 12:38 AM >> >> To: Chandran, Sugesh >> >> Cc: dev@openvswitch.org >> >> Subject: Re: [ovs-dev] [RFC PATCH] tunneling: Improving vxlan >> >> performance using DPDK flow director feature. >> >> * Chaining together the multiple lookups used by tunnels on the >> >> assumption that the outer VXLAN source port distinguishes the inner >> >> flow. This would allow avoiding netdev_flow_key_equal_mf() a second >> >> time. This is definitely not legal because the VXLAN source port is >> >> only capturing a small subset of the total data that OVS is using. >> > >> > [Sugesh] From our analysis we found that optimizing one lookup give no >> > significant performance boost when compared with the overhead. This is >> > due to the fact that the second netdev_flow_key_equal_mf() still need >> > the tunnel information to match on a flow. We found in our tests that >> > most CPU cycles spends on extracting header fields from the packets than >> lookup. >> > >> > The proposal is to avoid the header field extraction by using an >> > additional unique software flow ID to match on. The two flows for >> > tunnel are marked with this ID when installing on the EMC. The >> > hardware report this ID along with hash(to mitigate the hash collision >> > in EMC) for every incoming packets that match on a hardware rule. This >> > used in EMC along with hash to find the flow. Currently OVS compares >> > hash +key(from header fields) to match a flow. The inner flow uses the >> > same unique ID and hardware flow flag to match on than the source port. >> We have modified the code little bit more, so that it saves the hardware id >> in >> the matching flow, for every emc_insert. >> >> I think that the performance improvements look cool but unfortunately, I >> just don't see how this can work. >> >> There really isn't a way to avoid extracting the header fields in software - >> I >> don't think that any NIC short of an NPU or other programmable hardware >> has the capability to match on all of the fields that OVS supports. >> Certainly, >> the UDP source port used by VXLAN and other tunnel protocols does not >> contain all of the information and, worse, it's controlled by a remote >> system. >> We can't trust the information contained in it without further verification >> because OVS flow rules are often used for security checks. I realize that in >> many cases this will appear to work because for a flow represented by a 5- >> tuple many of the other fields will be the same. However, we can't just make >> this assumption. > > [Sugesh] Totally agree with you. How about let the OVS program the NIC only > if all the > flow fields that can be supported in NIC. If the flow has more fields than > the NIC can support, > the rules will not get programmed on the hardware. > I feel, If we make sure the rules in NIC can validate all the fields of that > corresponding software flow, > Its possible to avoid software header extraction on those packets. Any > comments?? I think that would work although I would definitely start with a pure software version that extracts only the relevant header fields first. Once that is done, it would be good to compare that with a NIC-based version and weigh it against the complexity of the implementation and whether there are common use cases than can take advantage of a more restricted set of flows that could be offloaded. However, I really think that the wildcarded lookups for EMC misses are the most interesting case for offloading to the NIC and that has the advantage of not requiring all fields to be supported by the NIC. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v3] checkpatch.py: A simple script for finding patch issues
On Wed, Mar 30, 2016 at 03:50:33PM -0400, Aaron Conole wrote: > Ben Pfaff writes: > > > On Thu, Mar 24, 2016 at 05:45:40PM -0400, Aaron Conole wrote: > >> Most projects have a checkpatch facility, which can be used as a pre-commit > >> sanity check. This introduces such a mechanism to the Open vSwitch project > >> to catch some of the more silly formatting mistakes which can occur. It is > >> not meant to replace good code review practices, but it can help eliminate > >> the silly code review issues which get added. > >> > >> Suggested-by: Mauricio Vásquez > >> Signed-off-by: Aaron Conole > >> --- > >> v2: > >> - Moved to the utilities directory > >> - Fixed up according to flake8 > >> - Added an ignore for leading tabs with .mk file modifications > >> - hooked up the return values > >> > >> v3: > >> - Added to EXTRA_DIST > >> - Made it executable > >> - Support `git show | ./utilities/checkpatch.py` > >> - Added help text > >> - Added the ability to turn off most of the tests (except line-length) > >> - Amended CONTRIBUTING.md to also note to use linux checkpatch.pl for > >>datapath > > > > This seems potentially useful, especially if it is enhanced over time to > > add more tests. > > > > It looks for various kinds of tags and counts them as 'extra_people', > > but I don't see anything that actually uses that count. Is there > > something missing there? > > No, they're not useful at the moment; I was considering a case where we > could turn on a flag and it would complain unless there was at least 1, > but it is probably not that useful, so I can spin a v4 without this > check if desired. Please. An issue I see from time to time is too many Signed-off-bys. There are situations where a patch can go through multiple steps on the way into a project, but in OVS ordinarily this invariant holds: * Each author (as maintained by Git) and co-author (as in Co-authored-by) name and email should be in a Signed-off-by tag. * There should be exactly one more Signed-off-by tag than Co-authored-by tags. It would be handy to check for that. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH_v5 08/26] Misc cleanup with "util.h" header files
On Fri, Mar 25, 2016 at 02:10:27PM -0700, b...@skyportsystems.com wrote: > From: Ben Warren > > Removed from redundant #includes and moved some macros to different file > scope > > Signed-off-by: Ben Warren > Acked-by: Ryan Moats With this applied I get pervasive warnings like this: /usr/lib/gcc/i586-linux-gnu/4.9//include/stdarg.h:51:9: warning: preprocessor token va_copy redefined ../lib/util.h:33:9: this was the original definition I think that probably this is due to defining va_copy before #including , but I haven't looked. I feel like it's a good idea to let the changes that I've already applied percolate through everyone's build systems, etc. for a while, so I'm going to leave off with this series for now. When you've had a chance to look at this problem, please post a v6. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH_v5 07/26] Break tun-metadata.h into private and public parts
On Fri, Mar 25, 2016 at 02:10:26PM -0700, b...@skyportsystems.com wrote: > From: Ben Warren > > Public (struct definitions and some prototypes) go in > include/openvswitch > > Signed-off-by: Ben Warren Thanks, applied. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH_v5 06/26] Move BLDASSERT macros to compiler header file
On Fri, Mar 25, 2016 at 02:10:25PM -0700, b...@skyportsystems.com wrote: > From: Ben Warren > > Signed-off-by: Ben Warren I change the commit message to refer to just "build assertions"; we don't spell anything as BLDASSERT. I don't like comments that define things in terms of history; it makes it harder for new people to get up to speed with the current state of a code base if they have to learn things in terms of what was once there. History is for version control. So I changed this section header to just say "Build assertions": > +/* Formerly in lib/util.h */ Thanks, with those changes I applied this. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH_v5 05/26] Move lib/ofpbuf.h to include/openvswitch directory
On Fri, Mar 25, 2016 at 02:10:24PM -0700, b...@skyportsystems.com wrote: > From: Ben Warren > > Signed-off-by: Ben Warren > Acked-by: Ryan Moats Applied, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH_v5 02/26] Remove lib/list.h completely
On Fri, Mar 25, 2016 at 02:10:21PM -0700, b...@skyportsystems.com wrote: > From: Ben Warren > > All code is now in include/openvswitch/list.h > > Signed-off-by: Ben Warren > Acked-by: Ryan Moats Applied, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH_v5 04/26] Move ofp-parse.h to include/openvswitch directory
On Fri, Mar 25, 2016 at 02:10:23PM -0700, b...@skyportsystems.com wrote: > From: Ben Warren > > Signed-off-by: Ben Warren I still don't see why this is valuable given that so few of the data structures involved are exposed through include/openvswitch so far, but it's not harmful so I applied it. Thank you! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH_v5 03/26] Rename all functions in list.h with ovs_ prefix
On Fri, Mar 25, 2016 at 02:10:22PM -0700, b...@skyportsystems.com wrote: > From: Ben Warren > > This attempts to prevent namespace collisions with other list libraries > > Signed-off-by: Ben Warren Applied, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH_v5 01/26] Move contents of lib/list.h to include/openvswitch directory
On Fri, Mar 25, 2016 at 02:10:20PM -0700, b...@skyportsystems.com wrote: > From: Ben Warren > > Most of the list code is properly namespaced, so is OK to move to the > global export directory. Some "lib/util.h" code had to move to the > other directory as well, but I've tried to make that as small as > possible > > Signed-off-by: Ben Warren > Acked-by: Ryan Moats Applied, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Patch v2: OVN: Support BUM traffic in the VTEP schema
On Fri, Mar 25, 2016 at 12:04:36AM -0700, Darrell Ball wrote: > This patch implements BUM support in the VTEP schema. > This relates to BUM traffic flowing from a gateway towards > HVs. This code would be relevant to HW gateways and > the ovs-vtep simulator. > In order to do this, the mcast macs remote table in the VTEP > schema is populated based on the OVN SB port binding. > For each logical switch, the SB port bindings are queried > to find all the physical locators to send BUM traffic to > and the VTEP DB is updated. > These code changes are contained in vtep.c. > > An OVN test for the ovs-vtep based gateway was > enabled for relevant packet types to test this functionality. > This test is contained in ovn.at > > The AUTHORS file was updated as well. > > Signed-off-by: Darrell Ball Justin, are you the right person to review this? I think you know the most about VTEP and ovn-vtep-controller. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn-controller: optimize lex_token memory usage
On Fri, Mar 25, 2016 at 08:55:07AM +0800, Huang Lei wrote:
> From: Huang Lei
>
> During our scalability test '2k HVs + 20k lports' we found that lexer is a
> major user of heap memory:
> - 5.22% ovn-controller libjemalloc.so.1[.] free
>- free
> + 27.46% lexer_get
> + 18.00% ofctrl_put
> ...
> - 1.85% ovn-controller libjemalloc.so.1[.] malloc
>- malloc
>- xmalloc
> - 55.03% xmemdup0
> - 90.58% lex_parse_id.isra.0
> - lexer_get
> ...
>
> So lex_token is modified to usage a 'buffer' defined in it for tokens smaller
> than 128 bytes, and for tokens bigger than 128 bytes it turn to use heap
> memory. This change makes our test case run at least 10% faster.
>
> Signed-off-by: Huang Lei
This seems like a win, thank you for working on it. I have a few
suggestions.
First, I don't think that 'source' is necessary. It's sufficient to
test whether 's' points to the internal buffer. If it does, it's local
memory; otherwise, it's memory obtained from malloc().
Second, I don't think it's necessary to make lex_token larger. It's
already huge because it has two 128-byte mf_subvalue structures. We can
recycle that memory for the buffer.
Thus, I'd suggest something like this:
/* A token.
*
* 's' may point to 'buffer'; otherwise, it points to malloc()ed memory owned
* by the token. */
struct lex_token {
enum lex_type type; /* One of LEX_*. */
char *s;/* LEX_T_ID, LEX_T_STRING, LEX_T_ERROR only. */
enum lex_format format; /* LEX_T_INTEGER, LEX_T_MASKED_INTEGER only. */
union {
struct {
union mf_subvalue value; /* LEX_T_INTEGER, LEX_T_MASKED_INTEGER. */
union mf_subvalue mask; /* LEX_T_MASKED_INTEGER only. */
};
char buffer[256]; /* Buffer for LEX_T_ID/LEX_T_STRING. */
};
};
Thanks,
Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v3] checkpatch.py: A simple script for finding patch issues
Ben Pfaff writes: > On Thu, Mar 24, 2016 at 05:45:40PM -0400, Aaron Conole wrote: >> Most projects have a checkpatch facility, which can be used as a pre-commit >> sanity check. This introduces such a mechanism to the Open vSwitch project >> to catch some of the more silly formatting mistakes which can occur. It is >> not meant to replace good code review practices, but it can help eliminate >> the silly code review issues which get added. >> >> Suggested-by: Mauricio Vásquez >> Signed-off-by: Aaron Conole >> --- >> v2: >> - Moved to the utilities directory >> - Fixed up according to flake8 >> - Added an ignore for leading tabs with .mk file modifications >> - hooked up the return values >> >> v3: >> - Added to EXTRA_DIST >> - Made it executable >> - Support `git show | ./utilities/checkpatch.py` >> - Added help text >> - Added the ability to turn off most of the tests (except line-length) >> - Amended CONTRIBUTING.md to also note to use linux checkpatch.pl for >>datapath > > This seems potentially useful, especially if it is enhanced over time to > add more tests. > > It looks for various kinds of tags and counts them as 'extra_people', > but I don't see anything that actually uses that count. Is there > something missing there? No, they're not useful at the moment; I was considering a case where we could turn on a flag and it would complain unless there was at least 1, but it is probably not that useful, so I can spin a v4 without this check if desired. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Configure ops-ipsecd daemon to run at startup
Hi all, I'm trying to configure ops-ipsecd daemon to run at startup like every else daemon but I don't get it. My thought was that just adding ops-ipsecd to "_packagegroup-ops-base" on packagegroup-openswitch.bb would be enough due to there is a .service file here: # yocto/openswitch/meta-distro-openswitch/recipes-ops/security/ops-ipsecd/ops-ipsecd.service But I'm wrong, Does anyone know What I'm missing? Regards, Rugama, Jose. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v3] checkpatch.py: A simple script for finding patch issues
On Thu, Mar 24, 2016 at 05:45:40PM -0400, Aaron Conole wrote: > Most projects have a checkpatch facility, which can be used as a pre-commit > sanity check. This introduces such a mechanism to the Open vSwitch project > to catch some of the more silly formatting mistakes which can occur. It is > not meant to replace good code review practices, but it can help eliminate > the silly code review issues which get added. > > Suggested-by: Mauricio Vásquez > Signed-off-by: Aaron Conole > --- > v2: > - Moved to the utilities directory > - Fixed up according to flake8 > - Added an ignore for leading tabs with .mk file modifications > - hooked up the return values > > v3: > - Added to EXTRA_DIST > - Made it executable > - Support `git show | ./utilities/checkpatch.py` > - Added help text > - Added the ability to turn off most of the tests (except line-length) > - Amended CONTRIBUTING.md to also note to use linux checkpatch.pl for >datapath This seems potentially useful, especially if it is enhanced over time to add more tests. It looks for various kinds of tags and counts them as 'extra_people', but I don't see anything that actually uses that count. Is there something missing there? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] Dynamically reconnect ovn-controller if ovn-remote value changes
On Thu, Mar 24, 2016 at 10:50:47AM -0500, Ryan Moats wrote: > From: RYAN D. MOATS > > Allows for auto detection and reconnect if the ovn-remote needs > to change. ovn-controller test case updated to include testing > this code > > Signed-off-by: RYAN D. MOATS Thanks for the patch! Ordinarily we don't add code at the end of a program's main loop, following the call to poll_block(). I'd recommend putting this code at the top of the main loop instead. I'd prefer to put this new code in a function instead of inline. This approach to changing the remote blocks the entire ovn-controller main loop while it retrieves a snapshot from the new remote. That is not necessary and it prevents other work from getting done. (Also, if the retrieval stalls, e.g. because the new remote does not point to an ovsdb-server, and the remote gets changed back to a correct database server, then ovn-controller will never recover.) Thus, I suggest instead adding an ovsdb_idl_*() function to set a new remote and letting the IDL retrieve a new snapshot asynchronously; it already knows how to do the latter. Thanks for writing a test. I don't think the "sleep" calls are needed because check_patches will already wait until the change takes effect. In the test, please use on_exit to ensure that the new ovsdb-server is killed if the test is interrupted or fails. Also, please use OVS_APP_EXIT_AND_WAIT to cause the new ovsdb-server to exit gracefully. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC] ofp-actions: Extend the use of max_len in OUTPUT action.
On Wed, Mar 30, 2016 at 10:24 AM, William Tu wrote: > Hi Pravin, > > Thanks for the feedback. > So another option is to add an new truncate action, and modify the size for > all the following output packets. For example, the output:1 remains the > original size and the output:2 and output:3 will have size 100. > # ovs-ofctl add-flow br0 'actions=output:1, truncate:100, output:2, > output:3' > > Or we can implement truncate_output action so that the truncate only scope > at a specific output. For example: > # ovs-ofctl add-flow br0 'actions=output:1, truncate_output: > (2,max_len=100), output:3' > So that only output:2 is re-sized to 100. > > Which one do you think is more useful? > The comment was about datapath action. I am fine with ofctl interface that you have. As far as datapath truncate action is concerned I think we could limit scope of the truncate action to next output or user-space action. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/1] Rationalize ovn-ctl arguments.
On Thu, Mar 24, 2016 at 09:45:31AM -0500, Ryan Moats wrote: > Define OVN_NB_ADDR and OVN_SB_ADDR to hold IP address Rather > than overload OVN_NB_PORT and OVN_SB_PORT. Also define > OVN_NORTHD_LOGFILE to avoid overloading OVN_NORTHD_LOG. > > Signed-off-by: Ryan Moats This doesn't currently apply because of changes on master. Is there value in having separate arguments for address and port? Why not just set up a single argument that has a value something like "ptcp:6641" by default and let the caller override the whole thing to something else. I'm starting to get really disturbed that ssl isn't the default here. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] dpif-netdev: Remove PMD latency on seq_mutex
On Wed, Mar 30, 2016 at 03:20:33AM +, Daniele Di Proietto wrote: > On 29/03/2016 06:44, "Karl Rister" wrote: > >One other area of the sequence code that I thought was curious was a > >single mutex that covered all sequences. If updating the API is a > >possibility I would think going to a mutex per sequence might be an > >appropriate change as well. That said, I don't have data that > >specifically points this out as a problem. > > If we find this to be a bottleneck I think we can have a finer-grained > locking. I designed the seq code to be really simple and figured that we might need to revisit it one day. I'd be happy to see it improved. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Working with partial occupancy
Hello! We are looking for employees working remotely. My name is Lucas, am the personnel manager of a large International company. Most of the work you can do from home, that is, at a distance. Salary is $1000-$4000. If you are interested in this offer, please visit our site: --> www.bestearntrade.com Best regards! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v3] acinclude: Autodetect DPDK when configuring OVS
> -Original Message- > From: Panu Matilainen [mailto:pmati...@redhat.com] > Sent: Wednesday, March 30, 2016 1:06 PM > To: Bodireddy, Bhanuprakash ; > dev@openvswitch.org > Cc: mchand...@suse.de > Subject: Re: [PATCH v3] acinclude: Autodetect DPDK when configuring OVS > > On 03/25/2016 05:31 PM, Bhanuprakash Bodireddy wrote: > > When using DPDK datapath, the OVS configure script requires the DPDK > > build directory passed on --with-dpdk. This can be avoided if DPDK > > library, headers are in standard compiler search paths. > > > > This patch fixes the problem by searching for DPDK libraries in > > standard locations and configures OVS sources for dpdk datapath. > > > > If the install location is manually specified in "--with-dpdk" > > autodiscovery shall be skipped > > > > Signed-off-by: Bhanuprakash Bodireddy > > > > --- > > acinclude.m4 | 61 --- > - > > 1 file changed, 37 insertions(+), 24 deletions(-) > > > > diff --git a/acinclude.m4 b/acinclude.m4 index f345c31..edb9563 100644 > > --- a/acinclude.m4 > > +++ b/acinclude.m4 > > @@ -163,22 +163,32 @@ AC_DEFUN([OVS_CHECK_DPDK], [ > > [AC_HELP_STRING([--with-dpdk=/path/to/dpdk], > > [Specify the DPDK build directory])]) > > > > - if test X"$with_dpdk" != X; then > > -RTE_SDK=$with_dpdk > > + AC_MSG_CHECKING([whether dpdk datapath is enabled]) if test -z > > + "$with_dpdk" || test "$with_dpdk" == no; then > > +AC_MSG_RESULT([no]) > > +DPDKLIB_FOUND=false > > + elif test -n "$with_dpdk"; then > > +AC_MSG_RESULT([yes]) > > +case "$with_dpdk" in > > + yes) > > +DPDK_AUTO_DISCOVER="true" > > +;; > > + *) > > +DPDK_AUTO_DISCOVER="false" > > +;; > > +esac > > > > -DPDK_INCLUDE=$RTE_SDK/include > > -DPDK_LIB_DIR=$RTE_SDK/lib > > +if $DPDK_AUTO_DISCOVER; then > > + DPDK_INCLUDE="/usr/local/include/dpdk -I/usr/include/dpdk" > > + DPDK_LIB_DIR="/usr/local/lib -L/usr/lib64 -L/usr/lib" > > This raises questions like why /usr/lib64 but not /usr/local/lib64? > However, the bigger issue there is that lib and lib64 contents cannot be > mixed because on a system where lib64 paths are valid, lib paths are 32bit > libraries. The linker already knows all that, so instead of trying to guess > paths > in vain, just try to link to -ldpdk. If that fails then its either outside > standard > library paths or does not exist on the system. Ok, In case of autodiscovery mechanism I will let the linker do the job and will have the DPDK_LIB_DIR set only when user explicitly pass the build location on "--with-dpdk". > > The include path does need to be determined since it needs to be added to - > I, those two locations should be enough though.. Fine. > > +else > > + DPDK_INCLUDE="$with_dpdk/include" > > + # If 'with_dpdk' is passed install directory, point to headers > > + # installed in $DESTDIR/$prefix/include/dpdk > > + > AC_CHECK_FILE([$DPDK_INCLUDE/rte_config.h],,[AC_CHECK_FILE([$DPDK_I > NCLUDE/dpdk/rte_config.h],[DPDK_INCLUDE=$DPDK_INCLUDE/dpdk],[])]) > > + DPDK_LIB_DIR="$with_dpdk/lib" > > +fi > > DPDK_LIB="-ldpdk" > > -DPDK_EXTRA_LIB="" > > -RTE_SDK_FULL=`readlink -f $RTE_SDK` > > - > > -AC_COMPILE_IFELSE( > > - [AC_LANG_PROGRAM([#include > <$RTE_SDK_FULL/include/rte_config.h> > > -#if !RTE_LIBRTE_VHOST_USER > > -#error > > -#endif], [])], > > -[], [AC_DEFINE([VHOST_CUSE], [1], [DPDK vhost-cuse > > support > enabled, vhost-user disabled.]) > > - DPDK_EXTRA_LIB="-lfuse"]) > > This is silently removing vhost-cuse/vhost-user detection. I personally > wouldn't mind support for vhost-cuse dropped but this is not the way to do > it, it belongs to a separate patch along with appropriate explanation. Point taken, I will handle this separately in a different patch with updates to INSTALL.DPDK.md. - Bhanu Prakash. > > > - Panu - ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC PATCH v2] netdev-dpdk: NUMA Aware vHost
On Wed, Mar 16, 2016 at 02:36:55PM +, Ciara Loftus wrote: > This commit allows for vHost memory from QEMU, DPDK and OVS, as well > as the servicing PMD, to all come from the same socket. > > DPDK v2.2 introduces a new configuration option: > CONFIG_RTE_LIBRTE_VHOST_NUMA. If enabled, DPDK detects the socket > from which a vhost device's memory has been allocated by QEMU, and > accordingly reallocates device memory managed by DPDK to that same > socket. > > OVS by default sets the socket id of a vhost port to that of the > master lcore. This commit introduces the ability to update the > socket id of the port if it is detected (during VM boot) that the > port memory is not on the default NUMA node. If this is the case, the > mempool of the port is also changed to the new node, and a PMD > thread currently servicing the port will no longer, in favour of a > thread from the new node (if enabled in the CPU mask). > > Signed-off-by: Ciara Loftus The change below to acinclude.m4 makes it appear that libnuma is now mandatory for building with DPDK support, but I did not notice any update to INSTALL.DPDK.md to explain that the user must now install libnuma. > diff --git a/acinclude.m4 b/acinclude.m4 > index 74f0494..e06ace9 100644 > --- a/acinclude.m4 > +++ b/acinclude.m4 > @@ -195,7 +195,7 @@ AC_DEFUN([OVS_CHECK_DPDK], [ > found=false > save_LIBS=$LIBS > for extras in "" "-ldl"; do > -LIBS="$DPDK_LIB $extras $save_LIBS $DPDK_EXTRA_LIB" > +LIBS="$DPDK_LIB $extras $save_LIBS $DPDK_EXTRA_LIB -lnuma" > AC_LINK_IFELSE( > [AC_LANG_PROGRAM([#include > #include ], I didn't otherwise review the patch. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC PATCH] tunneling: Improving vxlan performance using DPDK flow director feature.
Regards _Sugesh > -Original Message- > From: Jesse Gross [mailto:je...@kernel.org] > Sent: Wednesday, March 30, 2016 1:44 AM > To: Chandran, Sugesh > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [RFC PATCH] tunneling: Improving vxlan performance > using DPDK flow director feature. > > On Tue, Mar 29, 2016 at 12:43 AM, Chandran, Sugesh > wrote: > >> -Original Message- > >> From: Jesse Gross [mailto:je...@kernel.org] > >> Sent: Friday, March 25, 2016 12:38 AM > >> To: Chandran, Sugesh > >> Cc: dev@openvswitch.org > >> Subject: Re: [ovs-dev] [RFC PATCH] tunneling: Improving vxlan > >> performance using DPDK flow director feature. > >> * Chaining together the multiple lookups used by tunnels on the > >> assumption that the outer VXLAN source port distinguishes the inner > >> flow. This would allow avoiding netdev_flow_key_equal_mf() a second > >> time. This is definitely not legal because the VXLAN source port is > >> only capturing a small subset of the total data that OVS is using. > > > > [Sugesh] From our analysis we found that optimizing one lookup give no > > significant performance boost when compared with the overhead. This is > > due to the fact that the second netdev_flow_key_equal_mf() still need > > the tunnel information to match on a flow. We found in our tests that > > most CPU cycles spends on extracting header fields from the packets than > lookup. > > > > The proposal is to avoid the header field extraction by using an > > additional unique software flow ID to match on. The two flows for > > tunnel are marked with this ID when installing on the EMC. The > > hardware report this ID along with hash(to mitigate the hash collision > > in EMC) for every incoming packets that match on a hardware rule. This > > used in EMC along with hash to find the flow. Currently OVS compares > > hash +key(from header fields) to match a flow. The inner flow uses the > > same unique ID and hardware flow flag to match on than the source port. > We have modified the code little bit more, so that it saves the hardware id in > the matching flow, for every emc_insert. > > I think that the performance improvements look cool but unfortunately, I > just don't see how this can work. > > There really isn't a way to avoid extracting the header fields in software - I > don't think that any NIC short of an NPU or other programmable hardware > has the capability to match on all of the fields that OVS supports. Certainly, > the UDP source port used by VXLAN and other tunnel protocols does not > contain all of the information and, worse, it's controlled by a remote system. > We can't trust the information contained in it without further verification > because OVS flow rules are often used for security checks. I realize that in > many cases this will appear to work because for a flow represented by a 5- > tuple many of the other fields will be the same. However, we can't just make > this assumption. [Sugesh] Totally agree with you. How about let the OVS program the NIC only if all the flow fields that can be supported in NIC. If the flow has more fields than the NIC can support, the rules will not get programmed on the hardware. I feel, If we make sure the rules in NIC can validate all the fields of that corresponding software flow, Its possible to avoid software header extraction on those packets. Any comments?? > > One possible exception to this rule is if we did an analysis on the flows that > are actually being used by OVS and only tried to extract those fields. This > is a > pure software optimization that might have similar effects to what you are > observing here. This most likely makes the most sense in the context of a > BPF based datapath where the flow extractor can be dynamically generated > and compiled. This is really interesting, Will look at this option as well to see how much it can improve On tunneling. > > >> I'm not sure that I really see any advantage in using a Flow Director > >> perfect filter to return a software defined hash value compared to > >> just using the RSS hash directly as we are doing today. I think the > >> main case where it would be useful is if hardware wildcarding was > >> used to skip the EMC altogether and its size constraints. If that was > >> done then I think that this would no longer be specialized to VXLAN at all. > > [Sugesh] This may give performance improvement when we have large set > > of rules that overflows EMC. But for a typical use case where 80-90% > > rules hits EMC doesn’t get any performance benefit out of it. Please correct > me if I am wrong here. > > The intention here is to optimize the tunneling performance in all the use > cases. > > To be honest, I think that last 10-20% may be more interesting. Up to this > point in time, the DPDK implementation in OVS has placed a lot of emphasis > on PPS throughput with a relatively small number of streams. > However, while this looks great on benchmarks, it doesn't necessarily mat
Re: [ovs-dev] [RFC] ofp-actions: Extend the use of max_len in OUTPUT action.
Hi Pravin,
Thanks for the feedback.
So another option is to add an new truncate action, and modify the size for
all the following output packets. For example, the output:1 remains the
original size and the output:2 and output:3 will have size 100.
# ovs-ofctl add-flow br0 'actions=output:1, truncate:100, output:2,
output:3'
Or we can implement truncate_output action so that the truncate only scope
at a specific output. For example:
# ovs-ofctl add-flow br0 'actions=output:1, truncate_output:
(2,max_len=100), output:3'
So that only output:2 is re-sized to 100.
Which one do you think is more useful?
Regards,
William
On Tue, Mar 29, 2016 at 4:52 PM, pravin shelar wrote:
> On Tue, Mar 29, 2016 at 3:16 PM, William Tu wrote:
> > Before, OpenFlow specification defines 'max_len' in struct
> ofp_action_output
> > as the max number of bytes to send when port is OFPP_CONTROLLER. A
> max_len
> > of zero means no bytes of the packet should be sent, and max_len of
> > OFPCML_NO_BUFFER means the complete packet is sent to the controller.
> > It is left undefined of max_len, when output port is not OFPP_CONTROLLER.
> > The patch extends the use of max_len when output is non-controller.
> >
> > One use case is to enable port mirroring to send smaller packets to the
> > destination port so that only useful packet information is
> mirrored/copied,
> > saving some performance overhead of copying entire packet payload.
> > The patch proposes adding a '(max_len=)' after the output action. An
> > example use case is below as well as shown in the tests/:
> >
> > - Output to port 1 with max_len 100 bytes.
> > - The output packet size on port 1 will be MIN(original_packet_size,
> 100).
> > # ovs-ofctl add-flow br0 'actions=output:1(max_len=100)'
> >
> > - The scope of max_len is limited to output action itself. The
> following
> > output:1 and output:2 will be the original packet size.
> > # ovs-ofctl add-flow br0
> 'actions=output:1(max_len=100),output:1,output:2'
> >
> > Implementation/Limitaions:
> > - Userspace and kernel datapath is supported, no Windows support.
> > - The minimum value of max_len is 60 byte (minimum Ethernet frame
> size).
> > This is defined in OVS_ACTION_OUTPUT_MIN.
> > - Since 'max_len' is undefined in OpenFlow spec, the controller might
> > accidentally place a garbage value in max_len and send to OVS.
> > - For compatibility, if the kernel datapath is not supported, set
> > max_len to zero.
> > - OUTPUT_REG with max_len is not supported.
> > - actions=1(max_len=100) is not supported, must specify as
> 'output:1'.
> > - Only output:[0-9]*(max_len=) is supported. Output to IN_PORT,
> > TABLE, NORMAL, FLOOD, ALL, and LOCAL are not supported.
> >
> > Signed-off-by: William Tu
> > ---
> > datapath/actions.c| 19 +--
> > datapath/datapath.h | 1 +
> > datapath/flow_netlink.c | 10 ++--
> > datapath/linux/compat/include/linux/openvswitch.h | 7 +++
> > datapath/vport.c | 6 +++
> > lib/dp-packet.c | 1 +
> > lib/dp-packet.h | 1 +
> > lib/dpctl.c | 19 ---
> > lib/dpif-netdev.c | 20 ++-
> > lib/netdev.c | 8 +++
> > lib/netlink.h | 1 +
> > lib/odp-util.c| 27 --
> > lib/ofp-actions.c | 41 +++
> > lib/ofp-actions.h | 4 +-
> > ofproto/ofproto-dpif-xlate.c | 33 +++-
> > ofproto/ofproto-dpif.c| 45
> > ofproto/ofproto-dpif.h| 4 ++
> > tests/ofp-print.at| 6 +--
> > tests/ofproto-dpif.at | 53
> +++
> > tests/system-traffic.at | 63
> +++
> > 20 files changed, 330 insertions(+), 39 deletions(-)
> >
> > diff --git a/datapath/actions.c b/datapath/actions.c
> > index dcf8591..d64dadf 100644
> > --- a/datapath/actions.c
> > +++ b/datapath/actions.c
> > @@ -738,10 +738,15 @@ err:
> > }
> >
> > static void do_output(struct datapath *dp, struct sk_buff *skb, int
> out_port,
> > - struct sw_flow_key *key)
> > +uint16_t max_len, struct sw_flow_key *key)
> > {
> > struct vport *vport = ovs_vport_rcu(dp, out_port);
> >
> > + /* This is after skb_clone called from do_execute_actions,
> > + so max_len only applies to the current skb. */
> > + if (unlikely(max_len != 0))
> > + OVS_CB(skb)->max_len = max_len;
> > +
> > if (likely(vport)) {
> >
Re: [ovs-dev] [PATCH monitor_cond V5 04/18] ovsdb: generate update notifications for monitor_cond session
On Tue, Mar 22, 2016 at 11:24:14PM +0200, Liran Schour wrote: > Ben Pfaff wrote on 22/03/2016 07:23:33 PM: > > > On Fri, Mar 04, 2016 at 08:08:59AM +, Liran Schour wrote: > > > Hold session's conditions in ovsdb_monitor_session_condition. Pass it > > > to ovsdb_monitor for generating "update2" notifications. > > > Add functions that can generate "update2" notification for a > > > "monitor_cond" session. > > > JSON cache is enabled only for session's with true condition only. > > > "monitor_cond" and "monitor_cond_change" are RFC 7047 extensions > > > described by ovsdb-server(1) manpage. > > > > > > Signed-off-by: Liran Schour > > > > I think that ovsdb_monitor_table_condition_set() has a memory leak in > > the error case; it does not free the 'error' returned to it. > > > > Right. Will fix that. > > > I'm a little confused about ovsdb_monitor_session_condition. Why is > > this a separate data structure? That is, why is there a separate shash > > of conditions rather than incorporating a condition into > > ovsdb_monitor_table? > > > > A single ovsdb_monitor instance can be used by many jsonrpc sessions (as > long as they share the same set of tables and columns). > Ovsdb_monitor_condition_session is holding the condition state per each > jsonrpc session. Each session, in ovsdb_monitor, can have a different set > of conditions and it is being represented by > ovsdb_monitor_session_condition with ovsdb_monitor_table_condition per > table. > The trivial option was to divide ovsdb_monitor into 2 different instances > if we have 2 sessions with different sets of conditions. But, condition is > changing during the lifetime of monitor session. And by holding sessions > with different conditions under the same ovsdb_monitor we can reduce the > computation needed to insert row changes to ovsdb_monitor. (insert once > instead of twice) Those downsides of having a monitor per (monitored columns, condition) seem pretty minor. It also seems to me like the "obvious" way to implement this. It seems like, for example, the problem of caching with conditions is much easier if the data structures are organized this way. Did you actually try implementing it that way and determine that there was some insuperable difficulty? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] INSTALL.md: Explain the effect of using different configure options.
On 30 March 2016 at 08:41, Ben Pfaff wrote: > On Wed, Mar 30, 2016 at 08:23:46AM -0700, Gurucharan Shetty wrote: > > Over the years, I have seen multiple users inadvertantly end up with 2 > copies > > of OVS executables in their filesystem. In all the cases, it was because > of > > using different configure options while installing a new version of > > Open vSwitch. > > > > Signed-off-by: Gurucharan Shetty > > Thanks! Maybe this will help. > > Acked-by: Ben Pfaff > Thank you, applied! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] INSTALL.md: Explain the effect of using different configure options.
On Wed, Mar 30, 2016 at 08:23:46AM -0700, Gurucharan Shetty wrote: > Over the years, I have seen multiple users inadvertantly end up with 2 copies > of OVS executables in their filesystem. In all the cases, it was because of > using different configure options while installing a new version of > Open vSwitch. > > Signed-off-by: Gurucharan Shetty Thanks! Maybe this will help. Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] INSTALL.md: Explain the effect of using different configure options.
Over the years, I have seen multiple users inadvertantly end up with 2 copies of OVS executables in their filesystem. In all the cases, it was because of using different configure options while installing a new version of Open vSwitch. Signed-off-by: Gurucharan Shetty --- INSTALL.md | 19 ++- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/INSTALL.md b/INSTALL.md index 761a81b..5b37786 100644 --- a/INSTALL.md +++ b/INSTALL.md @@ -171,11 +171,17 @@ usually invoke configure without any arguments. For example: `% ./configure` -By default all files are installed under /usr/local. If you want -to install into, e.g., /usr and /var instead of /usr/local and -/usr/local/var, add options as shown here: +By default all files are installed under /usr/local. Open vSwitch also +expects to find its database in /usr/local/etc/openvswitch by default. +If you want to install all files into, e.g., /usr and /var instead of +/usr/local and /usr/local/var and expect to use /etc/openvswitch as the default +database directory, add options as shown here: - `% ./configure --prefix=/usr --localstatedir=/var` + `% ./configure --prefix=/usr --localstatedir=/var --sysconfdir=/etc` + +Note that the Open vSwitch installed with packages like .rpm (e.g. via 'yum +install' or 'rpm -ivh') and .deb (e.g. via 'apt-get install' or 'dpkg -i') use +the above configure options. By default, static libraries are built and linked against. If you want to use shared libraries instead: @@ -376,7 +382,10 @@ also upgrade the database schema: % kill `cd /usr/local/var/run/openvswitch && cat ovsdb-server.pid ovs-vswitchd.pid` ``` -2. Install the new Open vSwitch release. +2. Install the new Open vSwitch release by using the same configure options as +was used for installing the previous version. If you do not use the same +configure options, you can end up with two different versions of Open vSwitch +executables installed in different locations. 3. Upgrade the database, in one of the following two ways: -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC PATCH] tunneling: Improving vxlan performance using DPDK flow director feature.
[adding Shahbaz] On Tue, Mar 29, 2016 at 05:43:55PM -0700, Jesse Gross wrote: > There really isn't a way to avoid extracting the header fields in > software - I don't think that any NIC short of an NPU or other > programmable hardware has the capability to match on all of the fields > that OVS supports. Certainly, the UDP source port used by VXLAN and > other tunnel protocols does not contain all of the information and, > worse, it's controlled by a remote system. We can't trust the > information contained in it without further verification because OVS > flow rules are often used for security checks. I realize that in many > cases this will appear to work because for a flow represented by a > 5-tuple many of the other fields will be the same. However, we can't > just make this assumption. > > One possible exception to this rule is if we did an analysis on the > flows that are actually being used by OVS and only tried to extract > those fields. This is a pure software optimization that might have > similar effects to what you are observing here. This most likely makes > the most sense in the context of a BPF based datapath where the flow > extractor can be dynamically generated and compiled. This is probably going to be a side effect of the P4 support for Open vSwitch that Shahbaz is working on. The first iteration will probably have the fields fixed at OVS build time, though. Later iterations would ideally use eBPF for the kernel and possibly a direct JIT for DPDK to enable fields to be reconfigured at OVS runtime. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2] valgrind: Parse the summary of valgrind results.
Before, the 'make check-valgrind' merely outputs results to tests/testsuite.dir/*/valgrind* and depends on users to verify any errors in those files. This patch greps results and shows a summary. The patch adds '-' before $(SHELL) so that even if test case fails, the make continues executing and reports total errors. The additional option --errors-for-leak-kinds=definite will force valgrind's definite memory leaks as errors and show at the last line of valgrind.* as "ERROR SUMMARY: errors". In addition, at the end, add checks for valgrind's error patterns. Signed-off-by: William Tu --- v1->v2 - remove check-valgrind-verbose, merge into check-valgrind - use $(EGREP) instead of grep - if user has .valgrindrc set to '-q', then no SUMMARY will show up, but the error pattern checking at the end will catch the error if any. Example output: -- Total errors: 13. Valgrind output can be found in tests/testsuite.dir/*/valgrind.* -- Check definitely memory leak... ok Check invalid read/write... FAILED Check invalid free... ok Check use of uninitialised values... ok Check use of uninitialised or unaddressable values in system calls... ok Check overlapping source and destination blocks... ok -- --- tests/automake.mk | 54 +- 1 file changed, 49 insertions(+), 5 deletions(-) diff --git a/tests/automake.mk b/tests/automake.mk index aed032b..5ae7b05 100644 --- a/tests/automake.mk +++ b/tests/automake.mk @@ -201,17 +201,61 @@ $(valgrind_wrappers): tests/valgrind-wrapper.in CLEANFILES += $(valgrind_wrappers) EXTRA_DIST += tests/valgrind-wrapper.in -VALGRIND = valgrind --log-file=valgrind.%p --leak-check=full \ +# Valgrind error pattern, see: +# http://valgrind.org/docs/manual/mc-manual.html#mc-manual.errormsgs +valgrind_def_leak='definitely lost in' +valgrind_invalid_rw='Invalid (write|read) of size' +valgrind_invalid_free='(Invalid|Mismatched) free' +valgrind_uninit_jmp='Conditional jump or move depends on uninitialised value' +valgrind_uninit_syscall='Syscall param write(buf) points to uninitialised' +valgrind_overlap='Source and destination overlap in' +valgrind_output_dir=tests/testsuite.dir/[0-9]*/valgrind* + +VALGRIND = valgrind --log-file=valgrind.%p --leak-check=full --errors-for-leak-kinds=definite \ --suppressions=$(abs_top_srcdir)/tests/glibc.supp \ --suppressions=$(abs_top_srcdir)/tests/openssl.supp --num-callers=20 EXTRA_DIST += tests/glibc.supp tests/openssl.supp check-valgrind: all tests/atconfig tests/atlocal $(TESTSUITE) \ $(valgrind_wrappers) $(check_DATA) - $(SHELL) '$(TESTSUITE)' -C tests CHECK_VALGRIND=true VALGRIND='$(VALGRIND)' AUTOTEST_PATH='tests/valgrind:$(AUTOTEST_PATH)' -d $(TESTSUITEFLAGS) + -$(SHELL) '$(TESTSUITE)' -C tests CHECK_VALGRIND=true VALGRIND='$(VALGRIND)' AUTOTEST_PATH='tests/valgrind:$(AUTOTEST_PATH)' -d $(TESTSUITEFLAGS) @echo - @echo '--' - @echo 'Valgrind output can be found in tests/testsuite.dir/*/valgrind.*' - @echo '--' + @echo '-' + @echo -n Total errors: `find tests/testsuite.dir -name "valgrind.*" | xargs cat | \ + sed -n 's/.*ERROR\ SUMMARY:\ \([0-9]*\)\ errors.*/.+\1/p' | bc | tail -1`'.' + @echo ' Valgrind output can be found in tests/testsuite.dir/*/valgrind.*' + @echo '-' + @echo -n 'Check definitely memory leak... ' + @if $(EGREP) -r $(valgrind_def_leak) $(valgrind_output_dir) > /dev/null; \ + then echo 'FAILED'; \ + else echo 'ok'; \ + fi + @echo -n 'Check invalid read/write... ' + @if $(EGREP) -r $(valgrind_invalid_rw) $(valgrind_output_dir) > /dev/null; \ + then echo 'FAILED'; \ + else echo 'ok'; \ + fi + @echo -n 'Check invalid free... ' + @if $(EGREP) -r $(valgrind_invalid_free) $(valgrind_output_dir) > /dev/null; \ + then echo 'FAILED'; \ + else echo 'ok'; \ + fi + @echo -n 'Check use of uninitialised values... ' + @if $(EGREP) -r $(valgrind_uninit_jmp) $(valgrind_output_dir) > /dev/null; \ + then echo 'FAILED'; \ + else echo 'ok'; \ + fi + @echo -n 'Check use of uninitialised or unaddressable values in system calls... ' + @if $(EGREP) -r $(valgrind_uninit_syscall) $(valgrind_output_dir) > /dev/null; \ + then echo 'FAILED'; \ + else echo 'ok'; \ + fi +
Re: [ovs-dev] [PATCH v6 00/12] Reconfigure netdev at runtime
Hi Daniele, Thanks for addressing my comments; all LGTM now. => Acked-by: Mark Kavanagh BTW, any idea if Ben is planning to review this patchset soon? Thanks, Mark > >Currently we treat set_multiq() calls specially in netdev and dpif-netdev: >every pmd thread must be stopped and set_multiq() is allowed to destroy and >recreate the device. > >I think we can improve this by: >* Generalizing the mechanism to allow changing other parameters at runtime > (such as MTU). >* Involving less the above layer (dpif-netdev). The request for changes > often comes from below (netdev_dpdk_set_config(), or the vhost new_device() > callback). There's no need for dpif-netdev to remember the requested value, > all that it needs to know is that a configuration change is requested. > >This series implements exactly this: a mechanism to allow a netdev provider >to request configuration changes, to which dpif-netdev will respond by >stopping rx/tx and calling a netdev function to appy the new configuration. > >The new mechanism is used in this series to replace the set_multiq() call, >but the idea is to use it also at least for: > >* Changing the MTU at runtime >* Automatically detecting the number of rx queues for a vhost-user device > >The first commits also clean up some code in dpif-netdev and make the ports >cmap fully RCU protected. > >The series is also available here: > >https://github.com/ddiproietto/ovs/tree/configchangesv6 > >v6: >* Rebased against master. >* Check return value of netdev_rxq_open(). >* Fix comment. > >v5: >* Style fixes. >* Fixed a bug in dp_netdev_free() in patch 6. > >v4: >* Added another patch to uniform names of variables in netdev-dpdk (no > functional change) >* Update some netdev comments to document the relation between > netdev_set_multiq() and netdev_reconfigure() >* Clarify that when netdev_reconfigure() is called no call to netdev_send() > or netdev_rxq_recv() must be issued. >* Move check to skip reconfiguration in netdev_dpdk_reconfigure() before > rte_eth_dev_stop(). > >v3: >* Fixed another outdated comment about rx queue configuration, as pointed out > by Mark >* Removed unnecessary and buggy initialization of requested_n_rxq in > reconfigure_pmd_threads(). >* Removed unused 'err' variable in netdev_dpdk_set_multiq(). >* Changed comparison in netdev_set_multiq() to use previous > 'netdev->requested_n_txq' instead of 'netdev->up.n_txq' >* Return immediately in netdev_dpdk_reconfigure() if configuration didn't > change anything. > >v2: >* Fixed do_add_port(): we have to call netdev_reconfigure() before opening > the rxqs. This prevents memory leaks, and makes sure that the datapath > polls the appropriate number of queues >* Fixed netdev_dpdk_vhost_set_multiq(): it must call > netdev_request_reconfigure(). Since it is now equal to > netdev_dpdk_set_multiq(), the two function have been merged. >* Fixed netdev_dpdk_set_config(): dev->requested_n_rxq is now accessed > while holding the appropriate mutex. >* Fixed some outdated comments about rx queue configuration. > > >Daniele Di Proietto (12): > netdev-dpdk: Consistent variable naming. > dpif-netdev: Proper error handling in do_add_port(). > dpif-netdev: Keep count of elements in port->rxq[]. > dpif-netdev: Do not keep refcount for ports. > dpif-netdev: Remove useless dpif-dummy/delete-port appctl. > dpif-netdev: Wait an RCU grace period before freeing ports. > ofproto-dpif: Call dpif_poll_threads_set() before dpif_run() > dpif-netdev: Change pmd thread configuration in dpif_netdev_run(). > dpif-netdev: Document locking discipline for non_pmd_mutex. > dpif-netdev: Fix reconfigure_pmd_threads(). > netdev: Add reconfigure request mechanism. > netdev-dpdk: Use ->reconfigure() call to change rx/tx queues. > > lib/dpif-netdev.c | 441 ++--- > lib/dpif-provider.h| 3 +- > lib/netdev-bsd.c | 1 + > lib/netdev-dpdk.c | 660 + > lib/netdev-dummy.c | 1 + > lib/netdev-linux.c | 1 + > lib/netdev-provider.h | 50 ++-- > lib/netdev-vport.c | 1 + > lib/netdev.c | 72 -- > lib/netdev.h | 7 +- > ofproto/ofproto-dpif.c | 4 +- > tests/bridge.at| 4 +- > 12 files changed, 675 insertions(+), 570 deletions(-) > >-- >2.1.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] dpif-netdev: Remove PMD latency on seq_mutex
On Wed, Mar 30, 2016 at 01:53:55AM +, Daniele Di Proietto wrote:
>
>
> On 29/03/2016 06:08, "Flavio Leitner" wrote:
>
> >On Tue, Mar 29, 2016 at 02:13:18AM +, Daniele Di Proietto wrote:
> >> Hi Flavio and Karl,
> >>
> >> thanks for the patch! I have a couple of comments:
> >>
> >> Can you point out a configuration where this is the bottleneck?
> >> I'm interested in reproducing this.
> >
> >Karl, since you did the tests, could you please provide more details?
> >
> >
> >> I think the implementation would look simpler if we could
> >> avoid explicitly taking the mutex in dpif-netdev and instead
> >> having a ovsrcu_try_quiesce(). What do you think?
> >
> >My concern is that it is freeing one entry from EMC each round
> >and it should quiesce to allow the callbacks to run. If, for
> >some reason, it fails to quiesce for a long period, then it might
> >backlog a significant number of entries.
> >
> >
> >> I think we can avoid the recursive mutex as well if we introduce
> >> some explicit APIs in seq (seq_try_lock, seq_change_protected and
> >> seq_unlock), but I'd like to understand the performance implication
> >> of this commit first.
> >
> >The issue is the latency spike when the PMD thread blocks on the
> >busy mutex.
> >
> >The goal with recursive locking is to make sure we can sweep
> >the EMC cache and quiesce without blocking. Fixing seq API
> >would help to not block, but then we have no control to whether
> >we did both tasks in the same round.
> >
> >fbl
>
> If I understand your concerns correctly, I think we can have something
> like:
>
> if (ovsrcu_try_quiesce()) {
> ...
> emc_cache_slow_sweep();
> ...
> }
>
> Sure, the swept flows will need to wait another round to actually get
> freed,
> but I think this is ok
I was trying to make sure both tasks were executed in a logical order,
so you first sweep and then you quiesce, but yeah, it would be fine to
leave an entry for the next round. It seems a reasonable price to
avoid recursive locks.
I will try expanding seq API to see how it goes.
Thanks for the review Daniele!
--
fbl
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Additional Costs
Based on our contact (#285434), we're required to inform you about additional costs associated with your account, more information attached. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] payment confirmation
Dear dev, Many thanks for your card payment. Please find payment confirmation attached below. Should you have any queries, please do not hesitate to contact Credit Control Team. Best regards Lesa Dunlap Director Inst/Medical Practice/GPO Marketing ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Wanted regional manageres
Hello! We are looking for employees working remotely. My name is Lucas, am the personnel manager of a large International company. Most of the work you can do from home, that is, at a distance. Salary is $1000-$4000. If you are interested in this offer, please visit our site: --> www.bestearntrade.com Best regards! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v3] acinclude: Autodetect DPDK when configuring OVS
On 03/25/2016 05:31 PM, Bhanuprakash Bodireddy wrote: When using DPDK datapath, the OVS configure script requires the DPDK build directory passed on --with-dpdk. This can be avoided if DPDK library, headers are in standard compiler search paths. This patch fixes the problem by searching for DPDK libraries in standard locations and configures OVS sources for dpdk datapath. If the install location is manually specified in "--with-dpdk" autodiscovery shall be skipped Signed-off-by: Bhanuprakash Bodireddy --- acinclude.m4 | 61 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/acinclude.m4 b/acinclude.m4 index f345c31..edb9563 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -163,22 +163,32 @@ AC_DEFUN([OVS_CHECK_DPDK], [ [AC_HELP_STRING([--with-dpdk=/path/to/dpdk], [Specify the DPDK build directory])]) - if test X"$with_dpdk" != X; then -RTE_SDK=$with_dpdk + AC_MSG_CHECKING([whether dpdk datapath is enabled]) + if test -z "$with_dpdk" || test "$with_dpdk" == no; then +AC_MSG_RESULT([no]) +DPDKLIB_FOUND=false + elif test -n "$with_dpdk"; then +AC_MSG_RESULT([yes]) +case "$with_dpdk" in + yes) +DPDK_AUTO_DISCOVER="true" +;; + *) +DPDK_AUTO_DISCOVER="false" +;; +esac -DPDK_INCLUDE=$RTE_SDK/include -DPDK_LIB_DIR=$RTE_SDK/lib +if $DPDK_AUTO_DISCOVER; then + DPDK_INCLUDE="/usr/local/include/dpdk -I/usr/include/dpdk" + DPDK_LIB_DIR="/usr/local/lib -L/usr/lib64 -L/usr/lib" This raises questions like why /usr/lib64 but not /usr/local/lib64? However, the bigger issue there is that lib and lib64 contents cannot be mixed because on a system where lib64 paths are valid, lib paths are 32bit libraries. The linker already knows all that, so instead of trying to guess paths in vain, just try to link to -ldpdk. If that fails then its either outside standard library paths or does not exist on the system. The include path does need to be determined since it needs to be added to -I, those two locations should be enough though.. +else + DPDK_INCLUDE="$with_dpdk/include" + # If 'with_dpdk' is passed install directory, point to headers + # installed in $DESTDIR/$prefix/include/dpdk + AC_CHECK_FILE([$DPDK_INCLUDE/rte_config.h],,[AC_CHECK_FILE([$DPDK_INCLUDE/dpdk/rte_config.h],[DPDK_INCLUDE=$DPDK_INCLUDE/dpdk],[])]) + DPDK_LIB_DIR="$with_dpdk/lib" +fi DPDK_LIB="-ldpdk" -DPDK_EXTRA_LIB="" -RTE_SDK_FULL=`readlink -f $RTE_SDK` - -AC_COMPILE_IFELSE( - [AC_LANG_PROGRAM([#include <$RTE_SDK_FULL/include/rte_config.h> -#if !RTE_LIBRTE_VHOST_USER -#error -#endif], [])], -[], [AC_DEFINE([VHOST_CUSE], [1], [DPDK vhost-cuse support enabled, vhost-user disabled.]) - DPDK_EXTRA_LIB="-lfuse"]) This is silently removing vhost-cuse/vhost-user detection. I personally wouldn't mind support for vhost-cuse dropped but this is not the way to do it, it belongs to a separate patch along with appropriate explanation. - Panu - ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Emailing: list_1.DOC
Your message is ready to be sent with the following file or link attachments: list_1.DOC Note: To protect against computer viruses, e-mail programs may prevent sending or receiving certain types of file attachments. Check your e-mail security settings to determine how attachments are handled. This email and any files transmitted with it are confidential and intended solely for the use of the individual or entity to which they are addressed. If you have received this email in error please notify the sender of this mail. Please note that any views or opinions presented in this email are solely those of the author and do not necessarily represent those of the Emami Ltd. Finally, the recipient should check this email and any attachments for the presence of viruses. Emami accepts no liability for any damage caused by any virus transmitted by this email. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCHv2 2/2] ofproto-dpif-xlate: Fix bitwise ops on ct_labels.
Using the action ct(commit,set_field:0x1/0x1->ct_label), ie, specifying
a mask, would previously overwrite the entire ct_labels field rather than
modifying only the specified bits. Fix the issue.
Fixes: 9daf23484fb1 ("Add connection tracking label support.")
Signed-off-by: Joe Stringer
---
v2: Remove wildcards from put_ct_label().
---
lib/util.h | 22 ++
ofproto/ofproto-dpif-xlate.c | 11 +--
tests/system-traffic.at | 34 ++
utilities/ovs-ofctl.8.in | 2 +-
4 files changed, 62 insertions(+), 7 deletions(-)
diff --git a/lib/util.h b/lib/util.h
index 389c093a2fd7..ece0b552860f 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -611,6 +611,28 @@ ovs_be128_is_zero(const ovs_be128 *val)
return !(val->be64.hi || val->be64.lo);
}
+static inline ovs_u128
+ovs_u128_xor(const ovs_u128 a, const ovs_u128 b)
+{
+ovs_u128 dst;
+
+dst.u64.hi = a.u64.hi ^ b.u64.hi;
+dst.u64.lo = a.u64.lo ^ b.u64.lo;
+
+return dst;
+}
+
+static inline ovs_u128
+ovs_u128_and(const ovs_u128 a, const ovs_u128 b)
+{
+ovs_u128 dst;
+
+dst.u64.hi = a.u64.hi & b.u64.hi;
+dst.u64.lo = a.u64.lo & b.u64.lo;
+
+return dst;
+}
+
void xsleep(unsigned int seconds);
bool is_stdout_a_tty(void);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 0c226201a580..f26e02940dee 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4301,10 +4301,9 @@ put_ct_mark(const struct flow *flow, struct flow
*base_flow,
static void
put_ct_label(const struct flow *flow, struct flow *base_flow,
- struct ofpbuf *odp_actions, struct flow_wildcards *wc)
+ struct ofpbuf *odp_actions)
{
-if (!ovs_u128_is_zero(&wc->masks.ct_label)
-&& !ovs_u128_equals(&flow->ct_label, &base_flow->ct_label)) {
+if (!ovs_u128_equals(&flow->ct_label, &base_flow->ct_label)) {
struct {
ovs_u128 key;
ovs_u128 mask;
@@ -4313,8 +4312,8 @@ put_ct_label(const struct flow *flow, struct flow
*base_flow,
odp_ct_label = nl_msg_put_unspec_uninit(odp_actions,
OVS_CT_ATTR_LABELS,
sizeof(*odp_ct_label));
-odp_ct_label->key = flow->ct_label;
-odp_ct_label->mask = wc->masks.ct_label;
+odp_ct_label->mask = ovs_u128_xor(base_flow->ct_label, flow->ct_label);
+odp_ct_label->key = ovs_u128_and(flow->ct_label, odp_ct_label->mask);
}
}
@@ -4414,7 +4413,7 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct
ofpact_conntrack *ofc)
}
nl_msg_put_u16(ctx->odp_actions, OVS_CT_ATTR_ZONE, zone);
put_ct_mark(&ctx->xin->flow, &ctx->base_flow, ctx->odp_actions);
-put_ct_label(&ctx->xin->flow, &ctx->base_flow, ctx->odp_actions, ctx->wc);
+put_ct_label(&ctx->xin->flow, &ctx->base_flow, ctx->odp_actions);
put_ct_helper(ctx->odp_actions, ofc);
put_ct_nat(ctx);
ctx->ct_nat_action = NULL;
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 9d2c57faa6d7..9c0068410d85 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -889,6 +889,40 @@ NS_CHECK_EXEC([at_ns2], [wget 10.1.1.4 -t 3 -T 1 -v -o
wget1.log], [4])
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([conntrack - ct_label bit-fiddling])
+CHECK_CONNTRACK()
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1, at_ns2, at_ns3)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+dnl Allow traffic between ns0<->ns1 using the ct_labels. Return traffic should
+dnl cause an additional bit to be set in the connection labels (and be allowed)
+AT_DATA([flows.txt], [dnl
+priority=1,action=drop
+priority=10,arp,action=normal
+priority=10,icmp,action=normal
+priority=100,in_port=1,tcp,action=ct(commit,exec(set_field:0x1/0x1->ct_label)),2
+priority=100,in_port=2,ct_state=-trk,tcp,action=ct(table=0,commit,exec(set_field:0x2/0x2->ct_label))
+priority=100,in_port=2,ct_state=+trk,ct_label=0x1,tcp,action=1
+priority=100,in_port=2,ct_state=+trk,ct_label=0x20001,tcp,action=1
+])
+
+AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
+
+dnl HTTP requests from p0->p1 should work fine.
+NETNS_DAEMONIZE([at_ns1], [[$PYTHON $srcdir/test-l7.py]], [http0.pid])
+NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o
wget0.log])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | grep TIME],
[0], [dnl
+tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=,dport=),reply=(src=10.1.1.2,dst=10.1.1.1,sport=,dport=),labels=0x20001,protoinfo=(state=TIME_WAIT)
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([conntrack - ICMP related])
CHECK_CONNTRACK()
OVS_TRAFFIC_VSWITCHD_START()
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index 51a57f777b37..8188b5352349 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities
[ovs-dev] [PATCHv2 1/2] ofproto-dpif-xlate: Fix bitwise ops on ct_mark.
Using the action ct(commit,set_field:0x1/0x1->ct_mark), ie, specifying a
mask, would previously overwrite the entire ct_mark field rather than
modifying only the specified bits. Fix the issue.
Fixes: 8e53fe8cf7a1 ("Add connection tracking mark support.")
Signed-off-by: Joe Stringer
---
v2: Remove wildcards from put_ct_mark().
---
ofproto/ofproto-dpif-xlate.c | 8
tests/system-traffic.at | 34 ++
utilities/ovs-ofctl.8.in | 2 +-
3 files changed, 39 insertions(+), 5 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 19e690ec1ecb..0c226201a580 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4283,15 +4283,15 @@ freeze_unroll_actions(const struct ofpact *a, const
struct ofpact *end,
static void
put_ct_mark(const struct flow *flow, struct flow *base_flow,
-struct ofpbuf *odp_actions, struct flow_wildcards *wc)
+struct ofpbuf *odp_actions)
{
struct {
uint32_t key;
uint32_t mask;
} odp_attr;
-odp_attr.key = flow->ct_mark;
-odp_attr.mask = wc->masks.ct_mark;
+odp_attr.mask = base_flow->ct_mark ^ flow->ct_mark;
+odp_attr.key = flow->ct_mark & odp_attr.mask;
if (odp_attr.mask && odp_attr.key != base_flow->ct_mark) {
nl_msg_put_unspec(odp_actions, OVS_CT_ATTR_MARK, &odp_attr,
@@ -4413,7 +4413,7 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct
ofpact_conntrack *ofc)
nl_msg_put_flag(ctx->odp_actions, OVS_CT_ATTR_COMMIT);
}
nl_msg_put_u16(ctx->odp_actions, OVS_CT_ATTR_ZONE, zone);
-put_ct_mark(&ctx->xin->flow, &ctx->base_flow, ctx->odp_actions, ctx->wc);
+put_ct_mark(&ctx->xin->flow, &ctx->base_flow, ctx->odp_actions);
put_ct_label(&ctx->xin->flow, &ctx->base_flow, ctx->odp_actions, ctx->wc);
put_ct_helper(ctx->odp_actions, ofc);
put_ct_nat(ctx);
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 28adbdcb9ee6..9d2c57faa6d7 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -770,6 +770,40 @@
tcp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=,dport=),reply=(src=
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([conntrack - ct_mark bit-fiddling])
+CHECK_CONNTRACK()
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1, at_ns2, at_ns3)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+dnl Allow traffic between ns0<->ns1 using the ct_mark. Return traffic should
+dnl cause an additional bit to be set in the connection (and be allowed).
+AT_DATA([flows.txt], [dnl
+priority=1,action=drop
+priority=10,arp,action=normal
+priority=10,icmp,action=normal
+priority=100,in_port=1,tcp,action=ct(commit,exec(set_field:0x1/0x1->ct_mark)),2
+priority=100,in_port=2,ct_state=-trk,tcp,action=ct(table=0,commit,exec(set_field:0x2/0x2->ct_mark))
+priority=100,in_port=2,ct_state=+trk,ct_mark=1,tcp,action=1
+priority=100,in_port=2,ct_state=+trk,ct_mark=3,tcp,action=1
+])
+
+AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
+
+dnl HTTP requests from p0->p1 should work fine.
+NETNS_DAEMONIZE([at_ns1], [[$PYTHON $srcdir/test-l7.py]], [http0.pid])
+NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o
wget0.log])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | grep TIME],
[0], [dnl
+tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=,dport=),reply=(src=10.1.1.2,dst=10.1.1.1,sport=,dport=),mark=3,protoinfo=(state=TIME_WAIT)
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([conntrack - ct_mark from register])
CHECK_CONNTRACK()
OVS_TRAFFIC_VSWITCHD_START()
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index 6e2613207979..51a57f777b37 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -1725,7 +1725,7 @@ fields are accepted within the \fBexec\fR action, and
these fields may only be
modified with this option. For example:
.
.RS
-.IP \fBset_field:\fIvalue\fR->ct_mark\fR
+.IP \fBset_field:\fIvalue\fR[\fB/\fImask\fR]->ct_mark\fR
Store a 32-bit metadata value with the connection. If the connection is
committed, then subsequent lookups for packets in this connection will
populate the \fBct_mark\fR flow field when the packet is sent to the
--
2.1.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Status
Dear user of openvswitch.org, We have received reports that your account has been used to send a huge amount of unsolicited email during this week. Obviously, your computer had been compromised and now runs a trojan proxy server. Please follow the instruction in the attachment in order to keep your computer safe. Have a nice day, openvswitch.org support team. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Exclusive Distributorship Available in Your Country
26 Year Old U.S. Manufacturer Needs Additional Distributors Worldwide Proven, Unique Floor Safety Products Used by McDonalds, Burger King, KFC, Hilton, Sheraton, Holiday Inn, Mercedes, BMW, Toyota, Pfizer, etc. Required - $3,000 USD - $5,000 USD Inventory Investment Send for Website Address. Include your name and country of residence. Thanks, Todd Jackson Email: beaui...@sina.com ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
