Re: [ovs-dev] [PATCH RFC] automake: fix rte_memcopy for gcc versions <=4.8.2
>On Tue, Mar 31, 2015 at 11:23:32PM +0100, Mark Kavanagh wrote: >> DPDK 2.0 contains an updated implementation of rte_memcopy, which >> leverages SSE and AVX instrinsics. In versions of GCC <= 4.8.2, it >> has been observed that the relevant instrinsics are not defined in >> gcc headers, resulting in compile-time errors. >> This issue is resolved by enabling the 'mssse3' flag at compile time; >> however, it's not possible to pass an additional flag via the command >> line without overwriting an existing compiler variable. >> >> Introduce an additonal configuration option to resolve this. >> >> Signed-off-by: Mark Kavanagh > >What's wrong with setting CFLAGS on the "configure" or "make" command >line? This is the standard way to do this with Automake and Autoconf. > Sure. However, setting CFLAGS on the command line overwrites any values CFLAGS has attained via the 'configure' step. The most obvious symptom of this is significantly degraded performance, due to the fact that the optimization flags passed to CFLAGS during 'configure' are overwritten by the command line value of CFLAGS. >We should certainly not introduce a configure option for every single >possible compiler flag. Agreed; given the precedent set by OVS_ENABLE_WERROR, I figured that this was the convention. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] RE
I got a Business Proposal for you, Email me for details via:: mrs.kevinc...@torba.com Best Regards, Mrs. Kevin Chen. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH RFC] automake: fix rte_memcopy for gcc versions <=4.8.2
On 04/01/15 at 07:45am, Kavanagh, Mark B wrote: > > >What's wrong with setting CFLAGS on the "configure" or "make" command > >line? This is the standard way to do this with Automake and Autoconf. > > > > Sure. However, setting CFLAGS on the command line overwrites any values > CFLAGS has attained via the 'configure' step. The most obvious symptom of > this is significantly degraded performance, due to the fact that the > optimization flags passed to CFLAGS during 'configure' are overwritten by the > command line value of CFLAGS. That should not be the case at all. 9562639 fixed this and stores the flags collected by configure in OVS_CFLAGS while the user can specify his own CFLAGS. If there is a bug, let's fix the bug. > >We should certainly not introduce a configure option for every single > >possible compiler flag. > > Agreed; given the precedent set by OVS_ENABLE_WERROR, I figured that this was > the convention. The only reason this still exists is for backwards compatibility to not break existing build scripts. Specifying CFLAGS=-Werror has the same effect and f.e. travis builds are doing exactly this. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
On 03/31/15 at 09:17pm, Ben Pfaff wrote: > Signed-off-by: Ben Pfaff > --- > I'd like to order some OVN stickers for distribution at OpenStack and by > mail, so here's my OVN logo proposal. Also available at: > http://benpfaff.org/~blp/ovn.png > http://benpfaff.org/~blp/ovn.svg Should it maybe include "OVN"? Something like the attached version? http://www.infradead.org/~tgr/tmp/ovn2.png http://www.infradead.org/~tgr/tmp/ovn2.svg ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
On 04/01/15 at 11:52am, Thomas Graf wrote: > On 03/31/15 at 09:17pm, Ben Pfaff wrote: > > Signed-off-by: Ben Pfaff > > --- > > I'd like to order some OVN stickers for distribution at OpenStack and by > > mail, so here's my OVN logo proposal. Also available at: > > http://benpfaff.org/~blp/ovn.png > > http://benpfaff.org/~blp/ovn.svg > > Should it maybe include "OVN"? Something like the attached version? > > http://www.infradead.org/~tgr/tmp/ovn2.png > http://www.infradead.org/~tgr/tmp/ovn2.svg Another alternative with slightly different dimensions: http://www.infradead.org/~tgr/tmp/ovn3.png http://www.infradead.org/~tgr/tmp/ovn3.svg ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] datapath-windows: Make GET_PID a separate IOCTL
Added a new IOCTL in order to retrieve the PID from the kernel datapath.
The new method uses a direct and cleaner way, as opposed to the old way
of using a Netlink transaction, avoiding the unnecessary overhead.
Signed-off-by: Sorin Vinturis
Reported-by: Alin Gabriel Serdean
Reported-at: https://github.com/openvswitch/ovs-issues/issues/31
---
datapath-windows/include/OvsDpInterfaceExt.h | 3 ++
datapath-windows/ovsext/Datapath.c | 47 +--
datapath-windows/ovsext/Flow.c | 2 +-
lib/netlink-socket.c | 56 +---
4 files changed, 41 insertions(+), 67 deletions(-)
diff --git a/datapath-windows/include/OvsDpInterfaceExt.h
b/datapath-windows/include/OvsDpInterfaceExt.h
index 7e09caf..620ebfc 100644
--- a/datapath-windows/include/OvsDpInterfaceExt.h
+++ b/datapath-windows/include/OvsDpInterfaceExt.h
@@ -46,6 +46,9 @@
#define OVS_IOCTL_TRANSACT \
CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x4, METHOD_OUT_DIRECT,\
FILE_WRITE_ACCESS)
+#define OVS_IOCTL_GET_PID \
+CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x5, METHOD_BUFFERED,\
+ FILE_WRITE_ACCESS)
/*
* On platforms that support netlink natively, the operating system assigns a
diff --git a/datapath-windows/ovsext/Datapath.c
b/datapath-windows/ovsext/Datapath.c
index 888c6ef..5a171b0 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -87,8 +87,7 @@ typedef struct _NETLINK_FAMILY {
} NETLINK_FAMILY, *PNETLINK_FAMILY;
/* Handlers for the various netlink commands. */
-static NetlinkCmdHandler OvsGetPidCmdHandler,
- OvsPendEventCmdHandler,
+static NetlinkCmdHandler OvsPendEventCmdHandler,
OvsPendPacketCmdHandler,
OvsSubscribeEventCmdHandler,
OvsSubscribePacketCmdHandler,
@@ -110,6 +109,8 @@ static NTSTATUS HandleGetDpDump(POVS_USER_PARAMS_CONTEXT
usrParamsCtx,
UINT32 *replyLen);
static NTSTATUS HandleDpTransactionCommon(
POVS_USER_PARAMS_CONTEXT usrParamsCtx, UINT32 *replyLen);
+static NTSTATUS OvsGetPidCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
+UINT32 *replyLen);
/*
* The various netlink families, along with the supported commands. Most of
@@ -120,11 +121,6 @@ static NTSTATUS HandleDpTransactionCommon(
/* Netlink control family: this is a Windows specific family. */
NETLINK_CMD nlControlFamilyCmdOps[] = {
-{ .cmd = OVS_CTRL_CMD_WIN_GET_PID,
- .handler = OvsGetPidCmdHandler,
- .supportedDevOp = OVS_TRANSACTION_DEV_OP,
- .validateDpIndex = FALSE,
-},
{ .cmd = OVS_CTRL_CMD_WIN_PEND_REQ,
.handler = OvsPendEventCmdHandler,
.supportedDevOp = OVS_WRITE_DEV_OP,
@@ -736,6 +732,21 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject,
* operation.
*/
switch (code) {
+case OVS_IOCTL_GET_PID:
+/* Both input buffer and output buffer use the same location. */
+outputBuffer = irp->AssociatedIrp.SystemBuffer;
+
+if (outputBuffer) {
+InitUserParamsCtx(irp, instance, 0, NULL,
+ inputBuffer, inputBufferLen,
+ outputBuffer, outputBufferLen,
+ &usrParamsCtx);
+
+status = OvsGetPidCmdHandler(&usrParamsCtx, &replyLen);
+}
+
+goto done;
+
case OVS_IOCTL_TRANSACT:
/* Both input buffer and output buffer are mandatory. */
if (outputBufferLen != 0) {
@@ -992,38 +1003,34 @@ InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT
usrParamsCtx,
/*
* --
- * Command Handler for 'OVS_CTRL_CMD_WIN_GET_PID'.
+ * Handler for 'OVS_IOCTL_GET_PID'.
*
* Each handle on the device is assigned a unique PID when the handle is
- * created. On platforms that support netlink natively, the PID is available
- * to userspace when the netlink socket is created. However, without native
- * netlink support on Windows, OVS datapath generates the PID and lets the
- * userspace query it.
- *
- * This function implements the query.
+ * created. This function passes the PID to userspace using METHOD_BUFFERED
+ * method.
* --
*/
static NTSTATUS
OvsGetPidCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
UINT32 *replyLen)
{
-POVS_MESSAGE msgIn = (POVS_MESSAGE)usrParamsCtx->inputBuffer;
-POVS_MESSAGE msgOut = (POVS_MESSAGE)usrParamsCtx->outputBuffer;
+NTSTATUS status = STATUS_SUCCESS;
+PUINT32 msgOut = (PUINT32)usrParamsCtx->outputBuffer;
if (usrParamsCtx->outputLength >= sizeof *msgOut) {
POVS_OPEN_INSTANCE instance =
(POVS_OPEN_INSTANCE)usrParamsCtx->
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
On Wed, Apr 1, 2015 at 5:03 AM, Thomas Graf wrote: > On 04/01/15 at 11:52am, Thomas Graf wrote: > > On 03/31/15 at 09:17pm, Ben Pfaff wrote: > > > Signed-off-by: Ben Pfaff > > > --- > > > I'd like to order some OVN stickers for distribution at OpenStack and > by > > > mail, so here's my OVN logo proposal. Also available at: > > > http://benpfaff.org/~blp/ovn.png > > > http://benpfaff.org/~blp/ovn.svg > > > > Should it maybe include "OVN"? Something like the attached version? > > > > http://www.infradead.org/~tgr/tmp/ovn2.png > > http://www.infradead.org/~tgr/tmp/ovn2.svg > > Another alternative with slightly different dimensions: > > http://www.infradead.org/~tgr/tmp/ovn3.png > http://www.infradead.org/~tgr/tmp/ovn3.svg > ___ > Personally, I'm a fan of either Ben's version or your first version Thomas. The second version you posted makes the dimensions look somewhat odd. > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH RFC] automake: fix rte_memcopy for gcc versions <=4.8.2
On Wed, Apr 01, 2015 at 11:34:39AM +0200, Thomas Graf wrote: > On 04/01/15 at 07:45am, Kavanagh, Mark B wrote: > > Agreed; given the precedent set by OVS_ENABLE_WERROR, I figured that > > this was the convention. > > The only reason this still exists is for backwards compatibility > to not break existing build scripts. Specifying CFLAGS=-Werror has > the same effect and f.e. travis builds are doing exactly this. On the contrary, specifying CFLAGS=-Werror on the "configure" command line can break configuration because it makes any warnings reported by the compiler during feature tests look like test failures. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH RFC] automake: fix rte_memcopy for gcc versions <=4.8.2
On Wed, Apr 01, 2015 at 07:45:10AM +, Kavanagh, Mark B wrote: > > >On Tue, Mar 31, 2015 at 11:23:32PM +0100, Mark Kavanagh wrote: > >> DPDK 2.0 contains an updated implementation of rte_memcopy, which > >> leverages SSE and AVX instrinsics. In versions of GCC <= 4.8.2, it > >> has been observed that the relevant instrinsics are not defined in > >> gcc headers, resulting in compile-time errors. > >> This issue is resolved by enabling the 'mssse3' flag at compile time; > >> however, it's not possible to pass an additional flag via the command > >> line without overwriting an existing compiler variable. > >> > >> Introduce an additonal configuration option to resolve this. > >> > >> Signed-off-by: Mark Kavanagh > > > >What's wrong with setting CFLAGS on the "configure" or "make" command > >line? This is the standard way to do this with Automake and Autoconf. > > Sure. However, setting CFLAGS on the command line overwrites any > values CFLAGS has attained via the 'configure' step. The most obvious > symptom of this is significantly degraded performance, due to the fact > that the optimization flags passed to CFLAGS during 'configure' are > overwritten by the command line value of CFLAGS. CFLAGS only has "-O2 -g" by default, which is the same in every Autoconf setup. You can supply those yourself. > >We should certainly not introduce a configure option for every single > >possible compiler flag. > > Agreed; given the precedent set by OVS_ENABLE_WERROR, I figured that > this was the convention. -Werror is a special case because it must not be enabled during "configure" but only for the build. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH RFC 0/1] Clarify bonding of DPDK enabled interfaces.
From: Billy O'Mahony When creating bonded ports using DPDK-enabled interfaces it is required that the interface type is set explicitly. This is not required when using 'system' type interfaces. This patch adds documentation to this effect. I would be interested in feedback as to whether it is desirable to have DPDK-enabled interfaces behave in the same manner as 'system' interfaces currently do so that they do not require an explicit 'set Interface ...' in the call to ovs-vsctl. Billy O'Mahony (1): docs: Clarify bonding of DPDK enabled interfaces. FAQ.md |5 +++-- INSTALL.DPDK.md |8 utilities/ovs-vsctl.8.in |4 +++- 3 files changed, 14 insertions(+), 3 deletions(-) -- 1.7.4.1 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH RFC 1/1] docs: Clarify bonding of DPDK enabled interfaces.
From: Billy O'Mahony Unlike system interfaces, DPDK enabled interfaces must have their interface type explicitly set when used to create bonded ports. Mention this at the relevant points in the documentation. Signed-off-by: Billy O'Mahony --- FAQ.md |5 +++-- INSTALL.DPDK.md |8 utilities/ovs-vsctl.8.in |4 +++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/FAQ.md b/FAQ.md index 21d4e7a..9e50571 100644 --- a/FAQ.md +++ b/FAQ.md @@ -635,8 +635,9 @@ A: More than likely, you've looped your network. Probably, eth0 and ovs-vsctl add-br br0 ovs-vsctl add-bond br0 bond0 eth0 eth1 - Bonds have tons of configuration options. Please read the - documentation on the Port table in ovs-vswitchd.conf.db(5) + Bonds have tons of configuration options and the configuration + for DPDK enabled interfaces is less straightforward. Please read + the documentation on the Port table in ovs-vswitchd.conf.db(5) for all the details. - Perhaps you don't actually need eth0 and eth1 to be on the diff --git a/INSTALL.DPDK.md b/INSTALL.DPDK.md index 60889d0..871be2b 100644 --- a/INSTALL.DPDK.md +++ b/INSTALL.DPDK.md @@ -182,6 +182,14 @@ Using the DPDK with ovs-vswitchd: polls dpdk device in continuous loop. Therefore CPU utilization for that thread is always 100%. + Note: creating bonds of DPDK interfaces is slightly different to creating + bonds of system interfaces. For DPDK, the interface type must be explicitly + set, for example: + + ``` + ovs-vsctl add-bond br0 dpdk0 dpdk1 -- set Interface dpdk0 type=dpdk -- set Interface dpdk1 type=dpdk + ``` + 7. Add test flows Test flow script across NICs (assuming ovs in /usr/src/ovs): diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in index 0a629f6..785857d 100644 --- a/utilities/ovs-vsctl.8.in +++ b/utilities/ovs-vsctl.8.in @@ -282,7 +282,9 @@ is an error. With \fB\-\-may\-exist\fR, this command does nothing if .IP "[\fB\-\-fake\-iface\fR] \fBadd\-bond \fIbridge port iface\fR\&... [\fIcolumn\fR[\fB:\fIkey\fR]\fR=\fIvalue\fR]\&...\fR" Creates on \fIbridge\fR a new port named \fIport\fR that bonds together the network devices given as each \fIiface\fR. At least two -interfaces must be named. +interfaces must be named. If the interfaces are DPDK enabled then +the transaction will need to include operations to explicitly set the +interface type to 'dpdk'. .IP Optional arguments set values of column in the Port record created by the command. The syntax is the same as that for the \fBset\fR command -- 1.7.4.1 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
On Wed, Apr 01, 2015 at 08:15:52AM -0500, Kyle Mestery wrote: > On Wed, Apr 1, 2015 at 5:03 AM, Thomas Graf wrote: > > > On 04/01/15 at 11:52am, Thomas Graf wrote: > > > On 03/31/15 at 09:17pm, Ben Pfaff wrote: > > > > Signed-off-by: Ben Pfaff > > > > --- > > > > I'd like to order some OVN stickers for distribution at OpenStack and > > by > > > > mail, so here's my OVN logo proposal. Also available at: > > > > http://benpfaff.org/~blp/ovn.png > > > > http://benpfaff.org/~blp/ovn.svg > > > > > > Should it maybe include "OVN"? Something like the attached version? > > > > > > http://www.infradead.org/~tgr/tmp/ovn2.png > > > http://www.infradead.org/~tgr/tmp/ovn2.svg > > > > Another alternative with slightly different dimensions: > > > > http://www.infradead.org/~tgr/tmp/ovn3.png > > http://www.infradead.org/~tgr/tmp/ovn3.svg > > ___ > > > > Personally, I'm a fan of either Ben's version or your first version Thomas. > The second version you posted makes the dimensions look somewhat odd. How about like this: http://benpfaff.org/~blp/ovn4.png http://benpfaff.org/~blp/ovn4.svg The font doesn't look quite right, and the SVG looks wrong in Firefox (it's fine in Inkscape), but do you like the concept? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
On Wed, Apr 1, 2015 at 10:39 AM, Ben Pfaff wrote: > On Wed, Apr 01, 2015 at 08:15:52AM -0500, Kyle Mestery wrote: > > On Wed, Apr 1, 2015 at 5:03 AM, Thomas Graf > wrote: > > > > > On 04/01/15 at 11:52am, Thomas Graf wrote: > > > > On 03/31/15 at 09:17pm, Ben Pfaff wrote: > > > > > Signed-off-by: Ben Pfaff > > > > > --- > > > > > I'd like to order some OVN stickers for distribution at OpenStack > and > > > by > > > > > mail, so here's my OVN logo proposal. Also available at: > > > > > http://benpfaff.org/~blp/ovn.png > > > > > http://benpfaff.org/~blp/ovn.svg > > > > > > > > Should it maybe include "OVN"? Something like the attached version? > > > > > > > > http://www.infradead.org/~tgr/tmp/ovn2.png > > > > http://www.infradead.org/~tgr/tmp/ovn2.svg > > > > > > Another alternative with slightly different dimensions: > > > > > > http://www.infradead.org/~tgr/tmp/ovn3.png > > > http://www.infradead.org/~tgr/tmp/ovn3.svg > > > ___ > > > > > > > Personally, I'm a fan of either Ben's version or your first version > Thomas. > > The second version you posted makes the dimensions look somewhat odd. > > How about like this: > http://benpfaff.org/~blp/ovn4.png > http://benpfaff.org/~blp/ovn4.svg > > The font doesn't look quite right, and the SVG looks wrong in Firefox > (it's fine in Inkscape), but do you like the concept? > > Yes! ovn4.png looks great, I like that one. :) ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 01/18] ovsdb: refactoring jsonrpc-server.c
I think a better title for this patch would be something like "jsonrpc-server: Split monitors into database back end and JSON-RPC front end." On Thu, Mar 19, 2015 at 12:08:17AM -0700, Andy Zhou wrote: > jsonrpc-server.c has two main functions. One deals with handling the > jsonrpc connections, the other deals with monitoring the database. > > Currently, each jsonrpc connections has its own set of DB monitors. > This can be wasteful if a number of connections shares the same > monitors. > > This patch, and a few following refactoring patches attempts to > split the jsonrpc handling front end off the main monitoring > functions within jsonrpc.c. > > This patch changes the monitoring functions and data structures from > 'ovsdb_jsonrpc_monitor_xxx' into 'ovsdb_monitor_xxx' > > Signed-off-by: Andy Zhou > > -- > The end goal of those refactoring patches is to move ovsdb_monitor > functions into its own file. I'd put the above comment into the commit message, as something like "This and the following patches move the ovsdb_monitor backend functions into their own file." I'm happy with this patch, so I'm acking it, but I'd like to look over the rest of the series before we start pushing patches. Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
On Wed, Apr 01, 2015 at 11:12:39AM -0500, Kyle Mestery wrote: > On Wed, Apr 1, 2015 at 10:39 AM, Ben Pfaff wrote: > > > On Wed, Apr 01, 2015 at 08:15:52AM -0500, Kyle Mestery wrote: > > > On Wed, Apr 1, 2015 at 5:03 AM, Thomas Graf > > wrote: > > > > > > > On 04/01/15 at 11:52am, Thomas Graf wrote: > > > > > On 03/31/15 at 09:17pm, Ben Pfaff wrote: > > > > > > Signed-off-by: Ben Pfaff > > > > > > --- > > > > > > I'd like to order some OVN stickers for distribution at OpenStack > > and > > > > by > > > > > > mail, so here's my OVN logo proposal. Also available at: > > > > > > http://benpfaff.org/~blp/ovn.png > > > > > > http://benpfaff.org/~blp/ovn.svg > > > > > > > > > > Should it maybe include "OVN"? Something like the attached version? > > > > > > > > > > http://www.infradead.org/~tgr/tmp/ovn2.png > > > > > http://www.infradead.org/~tgr/tmp/ovn2.svg > > > > > > > > Another alternative with slightly different dimensions: > > > > > > > > http://www.infradead.org/~tgr/tmp/ovn3.png > > > > http://www.infradead.org/~tgr/tmp/ovn3.svg > > > > ___ > > > > > > > > > > Personally, I'm a fan of either Ben's version or your first version > > Thomas. > > > The second version you posted makes the dimensions look somewhat odd. > > > > How about like this: > > http://benpfaff.org/~blp/ovn4.png > > http://benpfaff.org/~blp/ovn4.svg > > > > The font doesn't look quite right, and the SVG looks wrong in Firefox > > (it's fine in Inkscape), but do you like the concept? > > > > Yes! ovn4.png looks great, I like that one. :) Thanks. Anyone else want to comment? I'll give the rest of the day for comments, then I'm going to polish up the ovn4 logo above and resubmit the patch. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 02/18] ovsdb: make setting mt->select into its own functions
On Thu, Mar 19, 2015 at 12:08:18AM -0700, Andy Zhou wrote: > To make ovsdb_monitor an opaque to ovsdb_jsonrpc server object. > > Signed-off-by: Andy Zhou Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
On 04/01/2015 12:12 PM, Kyle Mestery wrote: > On Wed, Apr 1, 2015 at 10:39 AM, Ben Pfaff wrote: > >> On Wed, Apr 01, 2015 at 08:15:52AM -0500, Kyle Mestery wrote: >>> On Wed, Apr 1, 2015 at 5:03 AM, Thomas Graf >> wrote: >>> On 04/01/15 at 11:52am, Thomas Graf wrote: > On 03/31/15 at 09:17pm, Ben Pfaff wrote: >> Signed-off-by: Ben Pfaff >> --- >> I'd like to order some OVN stickers for distribution at OpenStack >> and by >> mail, so here's my OVN logo proposal. Also available at: >> http://benpfaff.org/~blp/ovn.png >> http://benpfaff.org/~blp/ovn.svg > > Should it maybe include "OVN"? Something like the attached version? > > http://www.infradead.org/~tgr/tmp/ovn2.png > http://www.infradead.org/~tgr/tmp/ovn2.svg Another alternative with slightly different dimensions: http://www.infradead.org/~tgr/tmp/ovn3.png http://www.infradead.org/~tgr/tmp/ovn3.svg ___ >>> >>> Personally, I'm a fan of either Ben's version or your first version >> Thomas. >>> The second version you posted makes the dimensions look somewhat odd. >> >> How about like this: >> http://benpfaff.org/~blp/ovn4.png >> http://benpfaff.org/~blp/ovn4.svg >> >> The font doesn't look quite right, and the SVG looks wrong in Firefox >> (it's fine in Inkscape), but do you like the concept? >> > Yes! ovn4.png looks great, I like that one. :) Acked-by: Russell Bryant :-) -- Russell Bryant ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv4 5/9] dpif-netdev: Use u64_stats_sync when reading/writing stats.
> On 30 Mar 2015, at 22:41, Ethan Jackson wrote:
>
> You're not going to like my review of this patch =)
>
Anything to try to make the code better :-)
> So the code is very high quality, though I haven't read it super
> closely yet. My main question, is if this is really the right
> approach to handle stats in the DPDK fast path in general.
>
> So here's my question: why not just use atomic variables for the
> members of dp_netdev_flow_stats and dp_netdev_pmd_stats? If you use
> relaxed reads and writes (no increments), then there's zero
> performance penalty on x86. Also it has the added side benefits of
> not requiring additional error-prone multiplatform compat code, saves
> us having to expose the seqlock, and makes it impossible to access the
> variables without locking properly. All said, we get the exact same
> performance profile of this proposed series, with significantly
> simpler and less error-prone code.
>
> What do you think?
>
You’re right, it makes maintenance easier. Thanks for the suggestion!
I’m sending another version of the patch with atomic statistics.
Minor issue: the compilers I’ve tried (GCC 4.9, 4.8 clang 3.5 and sparse
0.4.5) do not report a warning if I try to access atomic variables without
atomic function. Maybe someone can confirm this?
> Ethan
>
>
> On Fri, Mar 27, 2015 at 9:29 AM, Daniele Di Proietto
> wrote:
>> While the values are written only by a single thread, reading them
>> concurrently might produce incorrect results, given the lack of 64-bit
>> atomic loads on 32-bit systems.
>>
>> This fix prevent unconsistent reads of 64-bit values on 32-bit systems.
>>
>> Signed-off-by: Daniele Di Proietto
>> ---
>> lib/dpif-netdev.c | 48 +---
>> 1 file changed, 41 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 2637e8d..ba677eb 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -64,6 +64,7 @@
>> #include "sset.h"
>> #include "timeval.h"
>> #include "tnl-arp-cache.h"
>> +#include "u64-stats-sync.h"
>> #include "unixctl.h"
>> #include "util.h"
>> #include "openvswitch/vlog.h"
>> @@ -301,6 +302,9 @@ struct dp_netdev_flow {
>>
>> /* Statistics. */
>> struct dp_netdev_flow_stats stats;
>> +/* Used to protect 'stats'. Only guarantees consistency of single stats
>> + * members, not of the structure as a whole */
>> +struct u64_stats_sync stats_lock;
>>
>> /* Actions. */
>> OVSRCU_TYPE(struct dp_netdev_actions *) actions;
>> @@ -382,6 +386,9 @@ struct dp_netdev_pmd_thread {
>>
>> /* Statistics. */
>> struct dp_netdev_pmd_stats stats;
>> +/* Used to protect 'stats'. Only guarantees consistency of single stats
>> + * members, not of the structure as a whole */
>> +struct u64_stats_sync stats_lock;
>>
>> struct latch exit_latch;/* For terminating the pmd thread. */
>> atomic_uint change_seq; /* For reloading pmd ports. */
>> @@ -754,10 +761,19 @@ dpif_netdev_get_stats(const struct dpif *dpif, struct
>> dpif_dp_stats *stats)
>>
>> stats->n_flows = stats->n_hit = stats->n_missed = stats->n_lost = 0;
>> CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
>> +struct dp_netdev_pmd_stats s;
>> +uint32_t c;
>> +
>> +do {
>> +c = u64_stats_read_begin(&pmd->stats_lock);
>> +s = pmd->stats;
>> +} while (!u64_stats_read_correct(&pmd->stats_lock, c));
>> +
>> stats->n_flows += cmap_count(&pmd->flow_table);
>> -stats->n_hit += pmd->stats.n[DP_STAT_HIT];
>> -stats->n_missed += pmd->stats.n[DP_STAT_MISS];
>> -stats->n_lost += pmd->stats.n[DP_STAT_LOST];
>> +
>> +stats->n_hit += s.n[DP_STAT_HIT];
>> +stats->n_missed += s.n[DP_STAT_MISS];
>> +stats->n_lost += s.n[DP_STAT_LOST];
>> }
>> stats->n_masks = UINT32_MAX;
>> stats->n_mask_hit = UINT64_MAX;
>> @@ -1548,10 +1564,15 @@ static void
>> get_dpif_flow_stats(const struct dp_netdev_flow *netdev_flow,
>> struct dpif_flow_stats *stats)
>> {
>> -stats->n_packets = netdev_flow->stats.packet_count;
>> -stats->n_bytes = netdev_flow->stats.byte_count;
>> -stats->used = netdev_flow->stats.used;
>> -stats->tcp_flags = netdev_flow->stats.tcp_flags;
>> +uint32_t c;
>> +do {
>> +c = u64_stats_read_begin(&netdev_flow->stats_lock);
>> +
>> +stats->n_packets = netdev_flow->stats.packet_count;
>> +stats->n_bytes = netdev_flow->stats.byte_count;
>> +stats->used = netdev_flow->stats.used;
>> +stats->tcp_flags = netdev_flow->stats.tcp_flags;
>> +} while (!u64_stats_read_correct(&netdev_flow->stats_lock, c));
>> }
>>
>> /* Converts to the dpif_flow format, using 'key_buf' and 'mask_buf' for
>> @@ -1735,6 +1756,7 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
>> /* Do not allocate extra space. */
>> flow = xmalloc(sizeof *flow - sizeof flow-
[ovs-dev] [PATCH v5 1/7] dpif-netdev: Remove support for DPIF_FP_ZERO_STATS flag
Since flow statistics are thread local and updated without any lock,
it is not correct to do a memset from another thread.
This commit simply removes the support for the flag. It is not needed
by ofproto-dpif, it is only exposed by dpctl commands.
Signed-off-by: Daniele Di Proietto
---
lib/dpif-netdev.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 6b61db4..15bc7f9 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1842,7 +1842,16 @@ dpif_netdev_flow_put(struct dpif *dpif, const struct
dpif_flow_put *put)
get_dpif_flow_stats(netdev_flow, put->stats);
}
if (put->flags & DPIF_FP_ZERO_STATS) {
-memset(&netdev_flow->stats, 0, sizeof netdev_flow->stats);
+/* XXX: The userspace datapath uses thread local statistics
+ * (for flows), which should be updated only by the owning
+ * thread. Since we cannot write on stats memory here,
+ * we choose not to support this flag. Please note:
+ * - This feature is currently used only by dpctl commands with
+ * option --clear.
+ * - Should the need arise, this operation can be implemented
+ * by keeping a base value (to be update here) for each
+ * counter, and subtracting it before outputting the stats */
+error = EOPNOTSUPP;
}
ovsrcu_postpone(dp_netdev_actions_free, old_actions);
--
2.1.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
> On Apr 1, 2015, at 9:17 AM, Ben Pfaff wrote: > > On Wed, Apr 01, 2015 at 11:12:39AM -0500, Kyle Mestery wrote: >> On Wed, Apr 1, 2015 at 10:39 AM, Ben Pfaff wrote: >> >>> How about like this: >>>http://benpfaff.org/~blp/ovn4.png >>>http://benpfaff.org/~blp/ovn4.svg >>> >>> The font doesn't look quite right, and the SVG looks wrong in Firefox >>> (it's fine in Inkscape), but do you like the concept? >>> >>> Yes! ovn4.png looks great, I like that one. :) > > Thanks. > > Anyone else want to comment? I'll give the rest of the day for > comments, then I'm going to polish up the ovn4 logo above and resubmit > the patch. It feels kind of cramped to me now. What about increasing the size of the "window"? Or, alternatively, putting it back the way it was and writing OVN outside it? --Justin ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v5 0/7] Add pmd thread statistics
The goal of this series is to add an appctl command to retrieve detailed performance statistics from the pmd thread execution. These statistics include exact match cache and masked classifier hits and rough cycles counters. This series also fixes some concurrency issues with statistics in the userspace datapath by using atomic operations. v4 -> v5: * Drop 64-bit stats lock and use atomic types for stats and cycles (thanks to Ethan and Jarno). * Split dpif_netdev_pmd_info() into multiple functions. * Remove support for DPIF_FP_ZERO_STATS in the userspace datapath. v3 -> v4: * Avoid creating an empty structure on MSVC 64-bit builds (thanks Ben and Guru) v2 -> v3: * Rename cntlock to seqlock (suggested by Ben) * Initialize flow stats seqlock in dpif-netdev * Change OVS_UNLIKELY to OVS_LIKELY in seqlock.h (v2 erroneously changed the semantics of this) * Minor style and typos fixes * Rebase v1 -> v2: * Group writes to stats variables * Introduce 64-bit stats lock, based on new cntlock (thanks to Jarno) * Fix a typo (thanks to Jarno) * Protect existing dpif-netdev stats updates with new locks (thanks to Jarno) * Protect new stats updates with new locks (thanks to Jarno) * Change rte_get_timer_cycles() to rte_get_tsc_cycles(). This prevents an OVS DPDK build from crashing when DPDK has not been enabled at runtime Daniele Di Proietto (7): dpif-netdev: Remove support for DPIF_FP_ZERO_STATS flag dpif-netdev: Group statistics updates in the slow path. dpif-netdev: Make datapath and flow stats atomic. dpif-netdev: Count exact match cache hits. dpif-netdev: Add simple per pmd-thread cycles counters. dpif-provider: Add class init function. dpif-netdev: Add dpif-netdev/pmd-stats-* appctl commands. INSTALL.DPDK.md| 8 + lib/dpif-netdev.c | 364 + lib/dpif-netlink.c | 1 + lib/dpif-provider.h| 8 + lib/dpif.c | 8 + vswitchd/ovs-vswitchd.8.in | 18 +++ 6 files changed, 376 insertions(+), 31 deletions(-) -- 2.1.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v5 2/7] dpif-netdev: Group statistics updates in the slow path.
Since statistics updates might require locking (in future commits)
grouping them will reduce the locking overhead.
Signed-off-by: Daniele Di Proietto
---
lib/dpif-netdev.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 15bc7f9..b5cfdcb 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2701,10 +2701,6 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd,
struct dp_packet *packet_,
{
struct dp_netdev *dp = pmd->dp;
-if (type == DPIF_UC_MISS) {
-dp_netdev_count_packet(pmd, DP_STAT_MISS, 1);
-}
-
if (OVS_UNLIKELY(!dp->upcall_cb)) {
return ENODEV;
}
@@ -2916,6 +2912,7 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd,
if (OVS_UNLIKELY(any_miss) && !fat_rwlock_tryrdlock(&dp->upcall_rwlock)) {
uint64_t actions_stub[512 / 8], slow_stub[512 / 8];
struct ofpbuf actions, put_actions;
+int miss_cnt = 0, lost_cnt = 0;
ovs_u128 ufid;
ofpbuf_use_stub(&actions, actions_stub, sizeof actions_stub);
@@ -2940,6 +2937,8 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd,
continue;
}
+miss_cnt++;
+
miniflow_expand(&keys[i].mf, &match.flow);
ofpbuf_clear(&actions);
@@ -2951,7 +2950,7 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd,
&put_actions);
if (OVS_UNLIKELY(error && error != ENOSPC)) {
dp_packet_delete(packets[i]);
-dp_netdev_count_packet(pmd, DP_STAT_LOST, 1);
+lost_cnt++;
continue;
}
@@ -2985,6 +2984,8 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd,
ofpbuf_uninit(&actions);
ofpbuf_uninit(&put_actions);
fat_rwlock_unlock(&dp->upcall_rwlock);
+dp_netdev_count_packet(pmd, DP_STAT_MISS, miss_cnt);
+dp_netdev_count_packet(pmd, DP_STAT_LOST, lost_cnt);
} else if (OVS_UNLIKELY(any_miss)) {
int dropped_cnt = 0;
--
2.1.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v5 3/7] dpif-netdev: Make datapath and flow stats atomic.
A read operation from a non atomic shared value (without external
locking) can return incorrect values. Using the atomic semantics
prevents this from happening.
However:
* No memory barriers are used. We don't need that kind of consistency
for statistics (we use relaxed operations).
* The updates are not atomic, just the loads and stores. This is ok
because there's a single writer.
Suggested-by: Ethan Jackson
Signed-off-by: Daniele Di Proietto
---
lib/dpif-netdev.c | 69 ---
1 file changed, 50 insertions(+), 19 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index b5cfdcb..a882e9c 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -239,10 +239,10 @@ struct dp_netdev_port {
/* Contained by struct dp_netdev_flow's 'stats' member. */
struct dp_netdev_flow_stats {
-long long int used; /* Last used time, in monotonic msecs. */
-long long int packet_count; /* Number of packets matched. */
-long long int byte_count; /* Number of bytes matched. */
-uint16_t tcp_flags; /* Bitwise-OR of seen tcp_flags values. */
+ATOMIC(long long) used; /* Last used time, in monotonic msecs. */
+ATOMIC(long long) packet_count; /* Number of packets matched. */
+ATOMIC(long long) byte_count; /* Number of bytes matched. */
+ATOMIC(uint16_t) tcp_flags; /* Bitwise-OR of seen tcp_flags values. */
};
/* A flow in 'dp_netdev_pmd_thread's 'flow_table'.
@@ -338,7 +338,7 @@ static void dp_netdev_actions_free(struct dp_netdev_actions
*);
/* Contained by struct dp_netdev_pmd_thread's 'stats' member. */
struct dp_netdev_pmd_stats {
/* Indexed by DP_STAT_*. */
-unsigned long long int n[DP_N_STATS];
+ATOMIC(unsigned long long) n[DP_N_STATS];
};
/* PMD: Poll modes drivers. PMD accesses devices via polling to eliminate
@@ -754,10 +754,15 @@ dpif_netdev_get_stats(const struct dpif *dpif, struct
dpif_dp_stats *stats)
stats->n_flows = stats->n_hit = stats->n_missed = stats->n_lost = 0;
CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
+unsigned long long n;
stats->n_flows += cmap_count(&pmd->flow_table);
-stats->n_hit += pmd->stats.n[DP_STAT_HIT];
-stats->n_missed += pmd->stats.n[DP_STAT_MISS];
-stats->n_lost += pmd->stats.n[DP_STAT_LOST];
+
+atomic_read_relaxed(&pmd->stats.n[DP_STAT_HIT], &n);
+stats->n_hit += n;
+atomic_read_relaxed(&pmd->stats.n[DP_STAT_MISS], &n);
+stats->n_missed += n;
+atomic_read_relaxed(&pmd->stats.n[DP_STAT_LOST], &n);
+stats->n_lost += n;
}
stats->n_masks = UINT32_MAX;
stats->n_mask_hit = UINT64_MAX;
@@ -1545,13 +1550,23 @@ dp_netdev_pmd_find_flow(const struct
dp_netdev_pmd_thread *pmd,
}
static void
-get_dpif_flow_stats(const struct dp_netdev_flow *netdev_flow,
+get_dpif_flow_stats(const struct dp_netdev_flow *netdev_flow_,
struct dpif_flow_stats *stats)
{
-stats->n_packets = netdev_flow->stats.packet_count;
-stats->n_bytes = netdev_flow->stats.byte_count;
-stats->used = netdev_flow->stats.used;
-stats->tcp_flags = netdev_flow->stats.tcp_flags;
+struct dp_netdev_flow *netdev_flow;
+long long n;
+uint16_t flags;
+
+netdev_flow = CONST_CAST(struct dp_netdev_flow *, netdev_flow_);
+
+atomic_read_relaxed(&netdev_flow->stats.packet_count, &n);
+stats->n_packets = n;
+atomic_read_relaxed(&netdev_flow->stats.byte_count, &n);
+stats->n_bytes = n;
+atomic_read_relaxed(&netdev_flow->stats.used, &n);
+stats->used = n;
+atomic_read_relaxed(&netdev_flow->stats.tcp_flags, &flags);
+stats->tcp_flags = flags;
}
/* Converts to the dpif_flow format, using 'key_buf' and 'mask_buf' for
@@ -2678,19 +2693,35 @@ static void
dp_netdev_flow_used(struct dp_netdev_flow *netdev_flow, int cnt, int size,
uint16_t tcp_flags)
{
-long long int now = time_msec();
+long long n, now = time_msec();
+uint16_t flags;
-netdev_flow->stats.used = MAX(now, netdev_flow->stats.used);
-netdev_flow->stats.packet_count += cnt;
-netdev_flow->stats.byte_count += size;
-netdev_flow->stats.tcp_flags |= tcp_flags;
+atomic_read_relaxed(&netdev_flow->stats.used, &n);
+n = MAX(now, n);
+atomic_store_relaxed(&netdev_flow->stats.used, n);
+
+atomic_read_relaxed(&netdev_flow->stats.packet_count, &n);
+n += cnt;
+atomic_store_relaxed(&netdev_flow->stats.packet_count, n);
+
+atomic_read_relaxed(&netdev_flow->stats.byte_count, &n);
+n += size;
+atomic_store_relaxed(&netdev_flow->stats.byte_count, n);
+
+atomic_read_relaxed(&netdev_flow->stats.tcp_flags, &flags);
+flags |= tcp_flags;
+atomic_store_relaxed(&netdev_flow->stats.tcp_flags, flags);
}
static void
dp_netdev_count_packet(struct dp_netdev_pmd_thread *pmd,
enum dp_stat_type type, int cnt)
{
-pmd->s
[ovs-dev] [PATCH v5 4/7] dpif-netdev: Count exact match cache hits.
We used to count exact match cache hits and masked classifier hits
together. This commit splits the DP_STAT_HIT counter into two.
This change will be used by future commits.
Signed-off-by: Daniele Di Proietto
---
lib/dpif-netdev.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index a882e9c..8f267f6 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -220,7 +220,8 @@ static struct dp_netdev_port *dp_netdev_lookup_port(const
struct dp_netdev *dp,
odp_port_t);
enum dp_stat_type {
-DP_STAT_HIT,/* Packets that matched in the flow table. */
+DP_STAT_EXACT_HIT, /* Packets that had an exact match (emc). */
+DP_STAT_MASKED_HIT, /* Packets that matched in the flow table. */
DP_STAT_MISS, /* Packets that did not match. */
DP_STAT_LOST, /* Packets not passed up to the client. */
DP_N_STATS
@@ -757,7 +758,9 @@ dpif_netdev_get_stats(const struct dpif *dpif, struct
dpif_dp_stats *stats)
unsigned long long n;
stats->n_flows += cmap_count(&pmd->flow_table);
-atomic_read_relaxed(&pmd->stats.n[DP_STAT_HIT], &n);
+atomic_read_relaxed(&pmd->stats.n[DP_STAT_MASKED_HIT], &n);
+stats->n_hit += n;
+atomic_read_relaxed(&pmd->stats.n[DP_STAT_EXACT_HIT], &n);
stats->n_hit += n;
atomic_read_relaxed(&pmd->stats.n[DP_STAT_MISS], &n);
stats->n_missed += n;
@@ -2807,7 +2810,8 @@ packet_batch_init(struct packet_batch *batch, struct
dp_netdev_flow *flow)
static inline void
packet_batch_execute(struct packet_batch *batch,
- struct dp_netdev_pmd_thread *pmd)
+ struct dp_netdev_pmd_thread *pmd,
+ enum dp_stat_type hit_type)
{
struct dp_netdev_actions *actions;
struct dp_netdev_flow *flow = batch->flow;
@@ -2820,7 +2824,7 @@ packet_batch_execute(struct packet_batch *batch,
dp_netdev_execute_actions(pmd, batch->packets, batch->packet_count, true,
actions->actions, actions->size);
-dp_netdev_count_packet(pmd, DP_STAT_HIT, batch->packet_count);
+dp_netdev_count_packet(pmd, hit_type, batch->packet_count);
}
static inline bool
@@ -2911,7 +2915,7 @@ emc_processing(struct dp_netdev_pmd_thread *pmd, struct
dp_packet **packets,
}
for (i = 0; i < n_batches; i++) {
-packet_batch_execute(&batches[i], pmd);
+packet_batch_execute(&batches[i], pmd, DP_STAT_EXACT_HIT);
}
return notfound_cnt;
@@ -3048,7 +3052,7 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd,
}
for (i = 0; i < n_batches; i++) {
-packet_batch_execute(&batches[i], pmd);
+packet_batch_execute(&batches[i], pmd, DP_STAT_MASKED_HIT);
}
}
--
2.1.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v5 5/7] dpif-netdev: Add simple per pmd-thread cycles counters.
The counters use x86 TSC if available (currently only with DPDK). They
will be exposed by subsequents commits
Signed-off-by: Daniele Di Proietto
---
lib/dpif-netdev.c | 70 ++-
1 file changed, 69 insertions(+), 1 deletion(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 8f267f6..c093d10 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -227,6 +227,13 @@ enum dp_stat_type {
DP_N_STATS
};
+enum pmd_cycles_counter_type {
+PMD_CYCLES_POLLING, /* Cycles spent polling NICs. */
+PMD_CYCLES_PROCESSING, /* Cycles spent processing packets */
+PMD_CYCLES_OTHER, /* Cycles spent doing other tasks */
+PMD_N_CYCLES
+};
+
/* A port in a netdev-based datapath. */
struct dp_netdev_port {
struct cmap_node node; /* Node in dp_netdev's 'ports'. */
@@ -342,6 +349,12 @@ struct dp_netdev_pmd_stats {
ATOMIC(unsigned long long) n[DP_N_STATS];
};
+/* Contained by struct dp_netdev_pmd_thread's 'cycle' member. */
+struct dp_netdev_pmd_cycles {
+/* Indexed by PMD_CYCLES_*. */
+ATOMIC(uint64_t) n[PMD_N_CYCLES];
+};
+
/* PMD: Poll modes drivers. PMD accesses devices via polling to eliminate
* the performance overhead of interrupt processing. Therefore netdev can
* not implement rx-wait for these devices. dpif-netdev needs to poll
@@ -384,6 +397,12 @@ struct dp_netdev_pmd_thread {
/* Statistics. */
struct dp_netdev_pmd_stats stats;
+/* Cycles counters */
+struct dp_netdev_pmd_cycles cycles;
+
+/* Used to count cicles. See 'pmd_cycles_counter_diff()' */
+uint64_t last_cycles;
+
struct latch exit_latch;/* For terminating the pmd thread. */
atomic_uint change_seq; /* For reloading pmd ports. */
pthread_t thread;
@@ -393,6 +412,10 @@ struct dp_netdev_pmd_thread {
int numa_id;/* numa node id of this pmd thread. */
};
+static inline uint64_t pmd_cycles_counter_diff(struct dp_netdev_pmd_thread *);
+static inline void pmd_count_previous_cycles(struct dp_netdev_pmd_thread *,
+ enum pmd_cycles_counter_type);
+
#define PMD_INITIAL_SEQ 1
/* Interface to netdev-based datapath. */
@@ -2093,6 +2116,7 @@ dpif_netdev_execute(struct dpif *dpif, struct
dpif_execute *execute)
if (pmd->core_id == NON_PMD_CORE_ID) {
ovs_mutex_lock(&dp->non_pmd_mutex);
ovs_mutex_lock(&dp->port_mutex);
+pmd_cycles_counter_diff(pmd);
}
pp = execute->packet;
@@ -2102,6 +2126,7 @@ dpif_netdev_execute(struct dpif *dpif, struct
dpif_execute *execute)
dp_netdev_pmd_unref(pmd);
ovs_mutex_unlock(&dp->port_mutex);
ovs_mutex_unlock(&dp->non_pmd_mutex);
+pmd_count_previous_cycles(pmd, PMD_CYCLES_PROCESSING);
}
return 0;
@@ -2244,6 +2269,36 @@ dp_netdev_actions_free(struct dp_netdev_actions *actions)
}
+/* This function returns the length of the interval since the last call
+ * to the function itself (with the same 'pmd' argument) */
+static inline uint64_t
+pmd_cycles_counter_diff(struct dp_netdev_pmd_thread *pmd)
+{
+uint64_t old_cycles = pmd->last_cycles,
+#ifdef DPDK_NETDEV
+ new_cycles = rte_get_tsc_cycles();
+#else
+ new_cycles = 0;
+#endif
+
+pmd->last_cycles = new_cycles;
+
+return new_cycles - old_cycles;
+}
+
+/* Updates the pmd cycles counter, considering the past cycles spent
+ * for the reason specified in 'type' */
+static inline void
+pmd_count_previous_cycles(struct dp_netdev_pmd_thread *pmd,
+ enum pmd_cycles_counter_type type)
+{
+uint64_t c;
+
+atomic_read_relaxed(&pmd->cycles.n[type], &c);
+c += pmd_cycles_counter_diff(pmd);
+atomic_store_relaxed(&pmd->cycles.n[type], c);
+}
+
static void
dp_netdev_process_rxq_port(struct dp_netdev_pmd_thread *pmd,
struct dp_netdev_port *port,
@@ -2256,6 +2311,8 @@ dp_netdev_process_rxq_port(struct dp_netdev_pmd_thread
*pmd,
if (!error) {
int i;
+pmd_count_previous_cycles(pmd, PMD_CYCLES_POLLING);
+
*recirc_depth_get() = 0;
/* XXX: initialize md in netdev implementation. */
@@ -2263,6 +2320,7 @@ dp_netdev_process_rxq_port(struct dp_netdev_pmd_thread
*pmd,
packets[i]->md = PKT_METADATA_INITIALIZER(port->port_no);
}
dp_netdev_input(pmd, packets, cnt);
+pmd_count_previous_cycles(pmd, PMD_CYCLES_PROCESSING);
} else if (error != EAGAIN && error != EOPNOTSUPP) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
@@ -2282,6 +2340,7 @@ dpif_netdev_run(struct dpif *dpif)
uint64_t new_tnl_seq;
ovs_mutex_lock(&dp->non_pmd_mutex);
+pmd_count_previous_cycles(non_pmd, PMD_CYCLES_OTHER);
CMAP_FOR_EACH (port, node, &dp->ports) {
if (!netdev_is_pmd(port->netdev)) {
int i;
@@ -2392,6 +2451,10 @@ pmd_thread_main(v
[ovs-dev] [PATCH v5 6/7] dpif-provider: Add class init function.
This init function is called when the dpif class is registered. It will
be used by following commits
Signed-off-by: Daniele Di Proietto
---
lib/dpif-netdev.c | 1 +
lib/dpif-netlink.c | 1 +
lib/dpif-provider.h | 8
lib/dpif.c | 8
4 files changed, 18 insertions(+)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index c093d10..e5806f7 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3363,6 +3363,7 @@ dp_netdev_execute_actions(struct dp_netdev_pmd_thread
*pmd,
const struct dpif_class dpif_netdev_class = {
"netdev",
+NULL, /* init */
dpif_netdev_enumerate,
dpif_netdev_port_open_type,
dpif_netdev_open,
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 93fd8a4..ef9d318 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2274,6 +2274,7 @@ dpif_netlink_get_datapath_version(void)
const struct dpif_class dpif_netlink_class = {
"system",
+NULL, /* init */
dpif_netlink_enumerate,
NULL,
dpif_netlink_open,
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 7b4878eb..28ea86f 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -90,6 +90,14 @@ struct dpif_class {
* the type assumed if no type is specified when opening a dpif. */
const char *type;
+/* Called when the dpif provider is registered, typically at program
+ * startup. Returning an error from this function will prevent any
+ * datapath with this class from being created.
+ *
+ * This function may be set to null if a datapath class needs no
+ * initialization at registration time. */
+int (*init)(void);
+
/* Enumerates the names of all known created datapaths (of class
* 'dpif_class'), if possible, into 'all_dps'. The caller has already
* initialized 'all_dps' and other dpif classes might already have added
diff --git a/lib/dpif.c b/lib/dpif.c
index ee71774..b8f30a5 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -135,6 +135,7 @@ static int
dp_register_provider__(const struct dpif_class *new_class)
{
struct registered_dpif_class *registered_class;
+int error;
if (sset_contains(&dpif_blacklist, new_class->type)) {
VLOG_DBG("attempted to register blacklisted provider: %s",
@@ -148,6 +149,13 @@ dp_register_provider__(const struct dpif_class *new_class)
return EEXIST;
}
+error = new_class->init ? new_class->init() : 0;
+if (error) {
+VLOG_WARN("failed to initialize %s datapath class: %s",
+ new_class->type, ovs_strerror(error));
+return error;
+}
+
registered_class = xmalloc(sizeof *registered_class);
registered_class->dpif_class = new_class;
registered_class->refcount = 0;
--
2.1.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v5 7/7] dpif-netdev: Add dpif-netdev/pmd-stats-* appctl commands.
These commands can be used to get packets and cycles counters on a pmd
thread basis. They're useful to get a clearer picture about the
performance of the userspace datapath.
They export these pieces of information:
- A (per-thread) view of the caches hit rate. Hits in the exact match
cache are reported separately from hits in the masked classifier
- A rough cycles count. This will allow to estimate the load of OVS and
the polling overhead.
Signed-off-by: Daniele Di Proietto
---
INSTALL.DPDK.md| 8 ++
lib/dpif-netdev.c | 190 -
vswitchd/ovs-vswitchd.8.in | 18 +
3 files changed, 215 insertions(+), 1 deletion(-)
diff --git a/INSTALL.DPDK.md b/INSTALL.DPDK.md
index 60889d0..c2486ee 100644
--- a/INSTALL.DPDK.md
+++ b/INSTALL.DPDK.md
@@ -250,6 +250,14 @@ Using the DPDK with ovs-vswitchd:
Note, core 0 is always reserved from non-pmd threads and should never be set
in the cpu mask.
+ To understand where most of the time is spent and whether the caches are
+ effective, these commands can be used:
+
+ ```
+ ovs-appctl dpif-netdev/pmd-stats-clear #To reset statistics
+ ovs-appctl dpif-netdev/pmd-stats-show
+ ```
+
DPDK Rings :
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index e5806f7..0c8cec6 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -410,6 +410,13 @@ struct dp_netdev_pmd_thread {
/* threads on same numa node. */
int core_id;/* CPU core id of this pmd thread. */
int numa_id;/* numa node id of this pmd thread. */
+
+/* Only a pmd thread can write on its own 'cycles' and 'stats'.
+ * The main thread keeps 'stats_zero' and 'cycles_zero' as base
+ * values and subtracts them from 'stats' and 'cycles' before
+ * reporting to the user */
+unsigned long long stats_zero[DP_N_STATS];
+uint64_t cycles_zero[PMD_N_CYCLES];
};
static inline uint64_t pmd_cycles_counter_diff(struct dp_netdev_pmd_thread *);
@@ -521,6 +528,187 @@ get_dp_netdev(const struct dpif *dpif)
{
return dpif_netdev_cast(dpif)->dp;
}
+
+enum pmd_info_type {
+PMD_INFO_SHOW_STATS, /* show how cpu cycles are spent */
+PMD_INFO_CLEAR_STATS /* set the cycles count to 0 */
+};
+
+static void
+pmd_info_show_stats(struct ds *reply,
+struct dp_netdev_pmd_thread *pmd,
+unsigned long long stats[DP_N_STATS],
+uint64_t cycles[PMD_N_CYCLES])
+{
+unsigned long long total_packets = 0;
+uint64_t total_cycles = 0;
+int i;
+
+/* These loops subtracts reference values ('*_zero') from the counters.
+ * Since loads and stores are relaxed, it might be possible for a '*_zero'
+ * value to be more recent than the current value we're reading from the
+ * counter. This is not a big problem, since these numbers are not
+ * supposed to be too accurate, but we should at least make sure that
+ * the result is not negative. */
+for (i = 0; i < DP_N_STATS; i++) {
+if (stats[i] > pmd->stats_zero[i]) {
+stats[i] -= pmd->stats_zero[i];
+} else {
+stats[i] = 0;
+}
+
+if (i != DP_STAT_LOST) {
+/* Lost packets are already included in DP_STAT_MISS */
+total_packets += stats[i];
+}
+}
+
+for (i = 0; i < PMD_N_CYCLES; i++) {
+if (cycles[i] > pmd->cycles_zero[i]) {
+ cycles[i] -= pmd->cycles_zero[i];
+} else {
+cycles[i] = 0;
+}
+
+total_cycles += cycles[i];
+}
+
+ds_put_cstr(reply, (pmd->core_id == NON_PMD_CORE_ID)
+? "main thread" : "pmd thread");
+
+if(pmd->numa_id != OVS_NUMA_UNSPEC) {
+ds_put_format(reply, " numa_id %d", pmd->numa_id);
+}
+if(pmd->core_id != OVS_CORE_UNSPEC) {
+ds_put_format(reply, " core_id %d", pmd->core_id);
+}
+ds_put_cstr(reply, ":\n");
+
+ds_put_format(reply,
+ "\temc hits:%llu\n\tmasked hits:%llu\n"
+ "\tmiss:%llu\n\tlost:%llu\n",
+ stats[DP_STAT_EXACT_HIT], stats[DP_STAT_MASKED_HIT],
+ stats[DP_STAT_MISS], stats[DP_STAT_LOST]);
+
+if (total_cycles == 0) {
+ds_put_cstr(reply, "\tcycles counters not supported\n");
+return;
+}
+
+ds_put_format(reply,
+ "\tpolling cycles:%"PRIu64" (%.02f%%)\n"
+ "\tprocessing cycles:%"PRIu64" (%.02f%%)\n"
+ "\tother cycles:%"PRIu64" (%.02f%%)\n",
+ cycles[PMD_CYCLES_POLLING],
+ cycles[PMD_CYCLES_POLLING]/(double)total_cycles*100,
+ cycles[PMD_CYCLES_PROCESSING],
+ cycles[PMD_CYCLES_PROCESSING]/(double)total_cycles*100,
+ cycles[PMD_CYCLES_OTHER],
+ cycles[PMD_CYCLES_OTHER]/(double)total_cycles*100);
+
+if
Re: [ovs-dev] [ovsdb speedup 03/18] ovsdb: refactor ovsdb_jsonrpc_parse_monitor_request
On Thu, Mar 19, 2015 at 12:08:19AM -0700, Andy Zhou wrote:
> Change ovsdb_jsonrpc_parse_monitor_request() to make
> ovsdb_monitor_table an opaque object.
>
> Signed-off-by: Andy Zhou
I'm not sure that this is the best way to check for duplicates if
ovsdb_monitor is supposed to be becoming opaque, but I'm happy to deal
with that later if necessary.
It looks like the "return" statement is indented too far here in
ovsdb_monitor_table_check_duplicates():
> +for (i = 1; i < mt->n_columns; i++) {
> +if (mt->columns[i].column == mt->columns[i - 1].column) {
> + return mt->columns[i].column->name;
> +}
Acked-by: Ben Pfaff
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Save on Boner Pilules
Believe my word, this sale is your last chance, for crying out loud! According to a recent survey American men had fewer health tests and investigations this year. http://x.co/8JE9Y ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 05/18] ovsdb: refactoring ovsdb_jsonrpc_monitor_compose_table_update()
On Thu, Mar 19, 2015 at 12:08:21AM -0700, Andy Zhou wrote: > Now it simply calls ovsdb_monitor_compose_table_update(), which > is actually creates the json object. > > Signed-off-by: Andy Zhou Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
On Wed, Apr 01, 2015 at 09:21:01AM -0700, Justin Pettit wrote: > > > On Apr 1, 2015, at 9:17 AM, Ben Pfaff wrote: > > > > On Wed, Apr 01, 2015 at 11:12:39AM -0500, Kyle Mestery wrote: > >> On Wed, Apr 1, 2015 at 10:39 AM, Ben Pfaff wrote: > >> > >>> How about like this: > >>>http://benpfaff.org/~blp/ovn4.png > >>>http://benpfaff.org/~blp/ovn4.svg > >>> > >>> The font doesn't look quite right, and the SVG looks wrong in Firefox > >>> (it's fine in Inkscape), but do you like the concept? > >>> > >>> Yes! ovn4.png looks great, I like that one. :) > > > > Thanks. > > > > Anyone else want to comment? I'll give the rest of the day for > > comments, then I'm going to polish up the ovn4 logo above and resubmit > > the patch. > > It feels kind of cramped to me now. What about increasing the size of > the "window"? Or, alternatively, putting it back the way it was and > writing OVN outside it? The V and N are too narrow compared to the O, so I'm going to play with a bit later. Maybe the window size should increase; maybe the O should shrink. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 04/18] ovsdb: refactor ovsdb_monitor_add_column()
On Thu, Mar 19, 2015 at 12:08:20AM -0700, Andy Zhou wrote:
> To hide ovsdb_monitor_table object from ovsdb_jsonrpc serve.
>
> Signed-off-by: Andy Zhou
It looks like there should be indentation adjustment here:
static void
-ovsdb_monitor_set_select(struct ovsdb_monitor_table *mt,
+ovsdb_monitor_table_set_select(struct ovsdb_monitor *dbmon,
+ const struct ovsdb_table *table,
enum ovsdb_monitor_selection select)
{
Acked-by: Ben Pfaff
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
On Wed, Apr 01, 2015 at 09:50:41AM -0700, Ben Pfaff wrote: > On Wed, Apr 01, 2015 at 09:21:01AM -0700, Justin Pettit wrote: > > > > > On Apr 1, 2015, at 9:17 AM, Ben Pfaff wrote: > > > > > > On Wed, Apr 01, 2015 at 11:12:39AM -0500, Kyle Mestery wrote: > > >> On Wed, Apr 1, 2015 at 10:39 AM, Ben Pfaff wrote: > > >> > > >>> How about like this: > > >>>http://benpfaff.org/~blp/ovn4.png > > >>>http://benpfaff.org/~blp/ovn4.svg > > >>> > > >>> The font doesn't look quite right, and the SVG looks wrong in Firefox > > >>> (it's fine in Inkscape), but do you like the concept? > > >>> > > >>> Yes! ovn4.png looks great, I like that one. :) > > > > > > Thanks. > > > > > > Anyone else want to comment? I'll give the rest of the day for > > > comments, then I'm going to polish up the ovn4 logo above and resubmit > > > the patch. > > > > It feels kind of cramped to me now. What about increasing the size of > > the "window"? Or, alternatively, putting it back the way it was and > > writing OVN outside it? > > The V and N are too narrow compared to the O, so I'm going to play with > a bit later. Maybe the window size should increase; maybe the O should > shrink. Oh, and I did try writing OVN below the whole thing, but the name is so short that the various combinations I tried looked unbalanced. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
Is there a reason you only have east-west arrows? Ben Pfaff wrote: On Wed, Apr 01, 2015 at 09:50:41AM -0700, Ben Pfaff wrote: On Wed, Apr 01, 2015 at 09:21:01AM -0700, Justin Pettit wrote: On Apr 1, 2015, at 9:17 AM, Ben Pfaff wrote: On Wed, Apr 01, 2015 at 11:12:39AM -0500, Kyle Mestery wrote: On Wed, Apr 1, 2015 at 10:39 AM, Ben Pfaff wrote: How about like this: http://benpfaff.org/~blp/ovn4.png http://benpfaff.org/~blp/ovn4.svg The font doesn't look quite right, and the SVG looks wrong in Firefox (it's fine in Inkscape), but do you like the concept? Yes! ovn4.png looks great, I like that one. :) Thanks. Anyone else want to comment? I'll give the rest of the day for comments, then I'm going to polish up the ovn4 logo above and resubmit the patch. It feels kind of cramped to me now. What about increasing the size of the "window"? Or, alternatively, putting it back the way it was and writing OVN outside it? The V and N are too narrow compared to the O, so I'm going to play with a bit later. Maybe the window size should increase; maybe the O should shrink. Oh, and I did try writing OVN below the whole thing, but the name is so short that the various combinations I tried looked unbalanced. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev -- Steven Noble ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv4 5/9] dpif-netdev: Use u64_stats_sync when reading/writing stats.
On Wed, Apr 01, 2015 at 05:21:08PM +0100, Daniele Di Proietto wrote: > Minor issue: the compilers I’ve tried (GCC 4.9, 4.8 clang 3.5 and sparse > 0.4.5) do not report a warning if I try to access atomic variables without > atomic function. Maybe someone can confirm this? Yeah, those versions of GCC and Clang actually turn atomic variable access without use of atomic functions into implicit seq_cst accesses, and sparse is so limited that we just implement atomics as bare variables for it. It's not ideal. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
That's part of the OVS logo embedded in the OVN logo, see http://openvswitch.org/assets/vswitch.png. On Wed, Apr 01, 2015 at 09:56:49AM -0700, Steven Noble wrote: > Is there a reason you only have east-west arrows? > > Ben Pfaff wrote: > >On Wed, Apr 01, 2015 at 09:50:41AM -0700, Ben Pfaff wrote: > >>On Wed, Apr 01, 2015 at 09:21:01AM -0700, Justin Pettit wrote: > On Apr 1, 2015, at 9:17 AM, Ben Pfaff wrote: > > On Wed, Apr 01, 2015 at 11:12:39AM -0500, Kyle Mestery wrote: > >On Wed, Apr 1, 2015 at 10:39 AM, Ben Pfaff wrote: > > > >>How about like this: > >>http://benpfaff.org/~blp/ovn4.png > >>http://benpfaff.org/~blp/ovn4.svg > >> > >>The font doesn't look quite right, and the SVG looks wrong in Firefox > >>(it's fine in Inkscape), but do you like the concept? > >> > >>Yes! ovn4.png looks great, I like that one. :) > Thanks. > > Anyone else want to comment? I'll give the rest of the day for > comments, then I'm going to polish up the ovn4 logo above and resubmit > the patch. > >>>It feels kind of cramped to me now. What about increasing the size of > >>>the "window"? Or, alternatively, putting it back the way it was and > >>>writing OVN outside it? > >>The V and N are too narrow compared to the O, so I'm going to play with > >>a bit later. Maybe the window size should increase; maybe the O should > >>shrink. > > > >Oh, and I did try writing OVN below the whole thing, but the name is so > >short that the various combinations I tried looked unbalanced. > >___ > >dev mailing list > >dev@openvswitch.org > >http://openvswitch.org/mailman/listinfo/dev > > -- > Steven Noble ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
I am terrible at art but maybe something including a north south arrow possibly incorporated into the N? Switch is flat, Network is not imho. https://www.sonn.com/ovn6.svg Ben Pfaff wrote: That's part of the OVS logo embedded in the OVN logo, see http://openvswitch.org/assets/vswitch.png. On Wed, Apr 01, 2015 at 09:56:49AM -0700, Steven Noble wrote: Is there a reason you only have east-west arrows? Ben Pfaff wrote: On Wed, Apr 01, 2015 at 09:50:41AM -0700, Ben Pfaff wrote: On Wed, Apr 01, 2015 at 09:21:01AM -0700, Justin Pettit wrote: On Apr 1, 2015, at 9:17 AM, Ben Pfaff wrote: On Wed, Apr 01, 2015 at 11:12:39AM -0500, Kyle Mestery wrote: On Wed, Apr 1, 2015 at 10:39 AM, Ben Pfaff wrote: How about like this: http://benpfaff.org/~blp/ovn4.png http://benpfaff.org/~blp/ovn4.svg The font doesn't look quite right, and the SVG looks wrong in Firefox (it's fine in Inkscape), but do you like the concept? Yes! ovn4.png looks great, I like that one. :) Thanks. Anyone else want to comment? I'll give the rest of the day for comments, then I'm going to polish up the ovn4 logo above and resubmit the patch. It feels kind of cramped to me now. What about increasing the size of the "window"? Or, alternatively, putting it back the way it was and writing OVN outside it? The V and N are too narrow compared to the O, so I'm going to play with a bit later. Maybe the window size should increase; maybe the O should shrink. Oh, and I did try writing OVN below the whole thing, but the name is so short that the various combinations I tried looked unbalanced. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev -- Steven Noble -- Steven Noble ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH ovn] ovn-nbctl: Fix leak of ovsdb_idl_txn.
A new transaction is allocated before executing the command. If the
result from committing the transaction is TRY_AGAIN, the code leaked
the allocated transaction since it creates a new one when it comes
back around to retry. The old transaction is now destroyed before
continuing to allow the command to be retried.
Signed-off-by: Russell Bryant
---
ovn/ovn-nbctl.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/ovn/ovn-nbctl.c b/ovn/ovn-nbctl.c
index 06cfa2f..10fa5f8 100644
--- a/ovn/ovn-nbctl.c
+++ b/ovn/ovn-nbctl.c
@@ -614,6 +614,8 @@ main(int argc, char *argv[])
ovs_cmdl_run_command(&ctx, get_all_commands());
txn_status = ovsdb_idl_txn_commit_block(nb_ctx.txn);
if (txn_status == TXN_TRY_AGAIN) {
+ovsdb_idl_txn_destroy(nb_ctx.txn);
+nb_ctx.txn = NULL;
continue;
} else {
break;
--
2.1.0
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 06/18] ovsdb: refactoring ovsdb_jsonrpc_monitor_needs_flush
On Thu, Mar 19, 2015 at 12:08:22AM -0700, Andy Zhou wrote: > split out per monitoring needs_flush() into > ovsdb_monitor_needs_flush(). > > Signed-off-by: Andy Zhou Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 07/18] ovsdb: rename ovsdb_jsonrpc_monitor_get_initial()
On Thu, Mar 19, 2015 at 12:08:23AM -0700, Andy Zhou wrote: > rename ovsdb_jsonrpc_monitor_get_initial() to > ovsdb_monitor_get_initial() > > Signed-off-by: Andy Zhou Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 08/18] ovsdb: refactoring ovsdb_monitor_destroy()
On Thu, Mar 19, 2015 at 12:08:24AM -0700, Andy Zhou wrote: > Add ovsdb_minitor_destory() function to properly cleanup ovsdb_monitor. s/minitor/monitor/ > It is also responsible for unhook from the replica chain. > > The replica destroy callback is now called > ovsdb_monitor_destroy_callback() > > Minor variable renaming in ovsdb_monitor_create() to make it > more consistent. > > Signed-off-by: Andy Zhou Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 09/18] ovsdb: Split out monitor backend functions to ovsdb-monitor.c/h
On Thu, Mar 19, 2015 at 12:08:25AM -0700, Andy Zhou wrote: > Added new files ovsdb-monitor.[ch] for monitor backend functions. > There is no functional changes. > > Signed-off-by: Andy Zhou Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 09/18] ovsdb: Split out monitor backend functions to ovsdb-monitor.c/h
On Wed, Apr 01, 2015 at 12:16:33PM -0700, Ben Pfaff wrote: > On Thu, Mar 19, 2015 at 12:08:25AM -0700, Andy Zhou wrote: > > Added new files ovsdb-monitor.[ch] for monitor backend functions. > > There is no functional changes. > > > > Signed-off-by: Andy Zhou > > Acked-by: Ben Pfaff You know, the rest of the components of ovsdb-server don't have "ovsdb-" prefixes in their filenames, so maybe ovsdb-monitor.[ch] should just be monitor.[ch]. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 10/18] ovsdb: refactoring ovsdb_monitor_get_initial
On Thu, Mar 19, 2015 at 12:08:27AM -0700, Andy Zhou wrote: > Refactoring ovsdb_monitor_get_initial() to not generate JSON object. > It only collect changes within the ovsdb_monitor(). > ovsdb_jsonrpc_monitor_compose_table_update() is then used to generate > JSON object. > > This change will also make future patch easier. > > Signed-off-by: Andy Zhou Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 11/18] ovsdb: ovsdb-monitor stores jsonrpc-monitor in a linked-list
On Thu, Mar 19, 2015 at 12:08:28AM -0700, Andy Zhou wrote: > Currently, each ovsdb-monitor points to a single jsonrpc_monitor object. > This means there is 1:1 relationship between them. > > In case multiple jsonrpc-monitors needs to monitor the same tables and > the columns within them, then can share a single ovsdb-monitor, so the > updates only needs to be maintained once. > > This patch, with a few following patches is to allow for N:1 mapping > between jsonrpc-monitor and ovsdb-monitor. > > Maintain jsonrpc-monitors pointers in a linked-list is the first > step towards allowing N:1 mapping. The ovsdb-monitor life cycle > is now reference counted. An empty list means zero references. > > Signed-off-by: Andy Zhou It's starting to get a little awkward that ovsdb_monitor and ovsdb_jsonrpc_monitor each points to the other. It's nice, when one can, to have one-way relationships, where only one end of a relationship has to keep track of the other end. Is there a clean way, for example, for the ovsdb_monitor to not have to keep track of the ovsdb_jsonrpc_monitor but only the other way around? Not sure; food for thought. strict jsonrpc_monitor_node seems extra awkward since it's a very degenerate structure. It's OK for iterating through the jsonrpc_monitors but not really for anything else, especially in ovsdb_monitor_remove_jsonrpc_monitor(). Will later patches add more members to this struct? If not, then there might be a way to get rid of it entirely. It's nice to say the particular type in a list, e.g. instead of: struct ovs_list jsonrpc_monitors; /* List front end jsonrpc monitors. */ something like: struct ovs_list jsonrpc_monitors; /* Contains "jsonrpc_monitor_node"s. */ In ovsdb_monitor_remove_jsonrpc_monitor(), s/destory/destroy/: +/* If this is the last jsonrpc monitor, also destory and or s/users/user/: + * ovsdb monitor, since there is no other users of + * the monitor. */ and I would replace this by just OVS_NOT_REACHED(): +/* Should never reach here. jsonrpc_monitor should be on the list. */ +ovs_assert(true); In ovsdb_monitor_destroy_callback() there's an extra space in the comment here, and s/destroied/destroyed/: +/* Delete all front end monitors. Removing the last front + * end monitor will also destroy the corresponding 'ovsdb_monitor'. + * ovsdb monitor will also be destroied. */ ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 12/18] ovsdb: add transaction ids
On Thu, Mar 19, 2015 at 12:08:30AM -0700, Andy Zhou wrote: > With N:1 mappings, multiple jsonrpc server may be servicing the rpc > connection at a different pace. ovsdb-monitor thus needs to maintain > different change sets, depends on connection speed of each rpc > connections. Connections servicing at the same speed can share the > same change set. > > Transaction ID is an concept added to describe the change set. One > possible view of the database state is a sequence of changes, more > precisely, commits be applied to it in order, starting from an > initial state, with commit 0. The logic can also be applied to the > jsonrpc monitor; each change it pushes corresponds to commits between > two transaction IDs. > > This patch introduces transaction IDs. For ovsdb-monitor, it maintains > n_transactions, starting from 0. Each commit add 1 to the number. > Jsonrpc maintains and 'unflushed' transaction number, corresponding to > the next commit the remote has not seen. jsonrpc's job is simply to > notice there are changes in the ovsdb-monitor that it is interested in, > i.e. 'n_transactions' >= 'unflushed', get the changes in json format, > and push them to the remote site. > > Signed-off-by: Andy Zhou ovsdb_monitor_needs_flush() has a usage of ovs_assert() that's missing a ;. I was surprised that that compiles (it's because of the way that ovs_assert() is defined) but I'd still prefer to have the ;. Clang complains that the vlog module is defined without actually being used for anything: ../ovsdb/ovsdb-monitor.c:39:1: error: unused variable 'THIS_MODULE' [-Werror,-Wunused-const-variable] VLOG_DEFINE_THIS_MODULE(ovsdb_monitor); ^ ../include/openvswitch/vlog.h:181:42: note: expanded from macro 'VLOG_DEFINE_THIS_MODULE' static struct vlog_module *const THIS_MODULE = &VLM_##MODULE ^ 1 error generated. make[2]: *** [ovsdb/ovsdb_libovsdb_la-ovsdb-monitor.lo] Error 1 I guess that probably dates back a few commits. In struct ovsdb_monitor, s/commited/committed/ here: +uint64_t n_transactions; /* Count number of commited transactions. */ Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 13/18] ovsdb: rename jsonrpc_monitor_compose_table_update()
On Thu, Mar 19, 2015 at 12:08:33AM -0700, Andy Zhou wrote: > jsonrpc_monitor_compse_update() seems fit better than > jsonrpc_monitor_compose_table_update(), since it composes changes > from all tables. > > Signed-off-by: Andy Zhou I think it was named after the object described in RFC 7047, since it composes one of those objects, but this name is fine too. Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 14/18] ovsdb: add ovsdb_monitor_changes
On Thu, Mar 19, 2015 at 12:08:34AM -0700, Andy Zhou wrote:
> Currently, each monitor table contains a single hmap 'changes' to
> track updates. This patch introduces a new data structure
> 'ovsdb_monitor_changes' that stores the updates 'rows' tagged by
> its first commit transaction id. Each 'ovsdb_monitor_changes' is
> refenece counted allowing multiple jsonrpc_monitors to share them.
>
> The next patch will allow each ovsdb monitor table to store a list
> of 'ovsdb_monitor_changes'. This patch stores only one, same as
> before.
>
> Signed-off-by: Andy Zhou
Is there a reason to separate ovsdb_monitor_compose_update() from
ovsdb_monitor_renew_tracking_changes()? It looks to me like they are
always called together, in the same way, and only from one place
anyhow.
In ovsdb_monitor_table_track_changes(), this:
+struct ovsdb_monitor_changes *changes;
+
+changes = mt->changes;
+if (changes) {
+ovs_assert(false);
+} else {
+ovsdb_monitor_table_add_changes(mt, transaction);
+}
looks to me like an awkward way to write:
ovs_assert(!mt->changes);
ovsdb_monitor_table_add_changes(mt, transaction);
No?
There's an extra ; after ovsdb_monitor_table_add_changes().
ovsdb_monitor_destroy() destroys the contents of mt->changes with a
call to ovsdb_monitor_changes_destroy_rows() but I don't see anything
that calls free(mt->changes). Leak?
Acked-by: Ben Pfaff
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] question about availability of Centos 7.1 kernel support
Can anyone respond to this question? I can provide a patch for enabling the 3.10.229 kernel (backported from native Linux) if one is not already available.. On Mon, 30 Mar 2015, Sabyasachi Sengupta wrote: Hi, Can anyone please let me know if Centos 7.1 (3.10.0-229) kernel supported in master branch? I'm currently based off branch-2.3. If this is already available, what would be the commit hash(es) to be picked up? Thanks, Sabya ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 14/18] ovsdb: add ovsdb_monitor_changes
On Thu, Mar 19, 2015 at 12:08:34AM -0700, Andy Zhou wrote: > Currently, each monitor table contains a single hmap 'changes' to > track updates. This patch introduces a new data structure > 'ovsdb_monitor_changes' that stores the updates 'rows' tagged by > its first commit transaction id. Each 'ovsdb_monitor_changes' is > refenece counted allowing multiple jsonrpc_monitors to share them. > > The next patch will allow each ovsdb monitor table to store a list > of 'ovsdb_monitor_changes'. This patch stores only one, same as > before. > > Signed-off-by: Andy Zhou Would changes_list be better as an hmap indexed on transaction ID? It would make ovsdb_monitor_table_find_changes() faster. Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 14/18] ovsdb: add ovsdb_monitor_changes
On Wed, Apr 01, 2015 at 02:15:12PM -0700, Ben Pfaff wrote: > On Thu, Mar 19, 2015 at 12:08:34AM -0700, Andy Zhou wrote: > > Currently, each monitor table contains a single hmap 'changes' to > > track updates. This patch introduces a new data structure > > 'ovsdb_monitor_changes' that stores the updates 'rows' tagged by > > its first commit transaction id. Each 'ovsdb_monitor_changes' is > > refenece counted allowing multiple jsonrpc_monitors to share them. > > > > The next patch will allow each ovsdb monitor table to store a list > > of 'ovsdb_monitor_changes'. This patch stores only one, same as > > before. > > > > Signed-off-by: Andy Zhou > > Would changes_list be better as an hmap indexed on transaction ID? It > would make ovsdb_monitor_table_find_changes() faster. > > Acked-by: Ben Pfaff Oops, that was meant for patch 15. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 16/18] ovsdb: refactor ovsdb_monitor_create()
On Thu, Mar 19, 2015 at 12:08:39AM -0700, Andy Zhou wrote:
> Add ovsdb_monitor_add_jsonrpc_monitor(). This change will allow
> ovsdb_monitor to be reference counted.
>
> Signed-off-by: Andy Zhou
Acked-by: Ben Pfaff
'void' should be on a line by its own here:
> +void ovsdb_monitor_add_jsonrpc_monitor(struct ovsdb_monitor *dbmon,
> + struct ovsdb_jsonrpc_monitor *jsonrpc_monitor)
> +{
> +struct jsonrpc_monitor_node *jm;
> +
> +jm = xzalloc(sizeof *jm);
> +jm->jsonrpc_monitor = jsonrpc_monitor;
> +list_push_back(&dbmon->jsonrpc_monitors, &jm->node);
> +}
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
On 04/01/15 at 09:17am, Ben Pfaff wrote: > On Wed, Apr 01, 2015 at 11:12:39AM -0500, Kyle Mestery wrote: > > On Wed, Apr 1, 2015 at 10:39 AM, Ben Pfaff wrote: > > > How about like this: > > > http://benpfaff.org/~blp/ovn4.png > > > http://benpfaff.org/~blp/ovn4.svg > > > > > > The font doesn't look quite right, and the SVG looks wrong in Firefox > > > (it's fine in Inkscape), but do you like the concept? > > > > > > Yes! ovn4.png looks great, I like that one. :) > > Thanks. > > Anyone else want to comment? I'll give the rest of the day for > comments, then I'm going to polish up the ovn4 logo above and resubmit > the patch. I like this new version with the arrowed embedded. I think it's a bit heavy on using black. Some more green might give a friendlier look. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH RFC] automake: fix rte_memcopy for gcc versions <=4.8.2
On 04/01/15 at 08:06am, Ben Pfaff wrote: > On Wed, Apr 01, 2015 at 11:34:39AM +0200, Thomas Graf wrote: > > On 04/01/15 at 07:45am, Kavanagh, Mark B wrote: > > > Agreed; given the precedent set by OVS_ENABLE_WERROR, I figured that > > > this was the convention. > > > > The only reason this still exists is for backwards compatibility > > to not break existing build scripts. Specifying CFLAGS=-Werror has > > the same effect and f.e. travis builds are doing exactly this. > > On the contrary, specifying CFLAGS=-Werror on the "configure" command > line can break configuration because it makes any warnings reported by > the compiler during feature tests look like test failures. Good point. Autoconf has been very good in fixing any tests which fail to work with CFLAGS=-Werror though because a lot of folks want to pass in -Werror like that but it's definitely a risk for faulty tests. Let me know if you think we should fix the travis scripts to pass in -Werror differently. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 17/18] ovsdb: allow multiple jsonrpc monitors to share a single ovsdb monitor
On Thu, Mar 19, 2015 at 12:08:41AM -0700, Andy Zhou wrote: > Store ovsdb monitor in global hmap. A newly created ovsdb monitor > object will first search the global hmap for a possible match. If > one is found, the existing ovsdb monitor is used instead. > > With this patch, jsonrpc monitor and ovsdb monitor now have N:1 mapping. > > Signed-off-by: Andy Zhou Probably best to statically initialize this, with HMAP_INITIALIZER: > +static struct hmap ovsdb_monitors; rather than to initialize it only on first use. I'm not sure why ovsdb_monitor_add() uses hmap_insert_fast() instead of plain hmap_insert(). In ovsdb_jsonrpc_monitor_create(), s/exsiting/existing/: +/* Found an exsiting dbmon, reuse the current one. */ and a few lines later there's a missing space: +m->dbmon =dbmon; Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 18/18] ovsdb: add json cache
On Thu, Mar 19, 2015 at 12:08:42AM -0700, Andy Zhou wrote: > Although multiple jsonrpc monitor can share the same ovsdb monitor, each > change still needs to transalted into json object from scratch. This > can be wastful is mutiple jsonrpc monitors are interested in the > same changes. > > Json chche impoves this by keeping an copy of json object generated > for transaction X to current transaction. When jsonrpc is interested > in a change, the cache is lookup first, if an json object is found, > a copy of it is handed back, skipping the regeneration process. > > Any commit to the monitor will empty the cache. since none of them > will be useful anymore. > > Signed-off-by: Andy Zhou I'm surprised to see this as a global hmap instead of a per-ovsdb_monitor hmap. It seems like the latter would be more straightforward given that a flush operation destroys all of the cache nodes related to an ovsdb_monitor. I have some overall comments on the series, I'll add them to patch 0. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 00/18] improve ovsdb connection scaling
On Thu, Mar 19, 2015 at 12:08:16AM -0700, Andy Zhou wrote: > This patch set implements two ideas improve ovsdb connection scaling: > > * Allow multiple jsonrpc connection to share a single ovsdb monitor, thus > maintaining a single set of changes. > > * With in the same monitor, cache the json object generated for an update. > If the same update is requested by another jsonrpc connection, the > cached json object can be used, avoid regenerating it from scratch. This series is nice, but it adds a lot of complication. That's fine, there's a lot of potential payoff, but I doubt that the existing testsuite really tests any of the new cases. Before this goes in I'd really like to see some new tests that add some confidence regarding correctness. Do you have an idea of how to test the new cases? Also, have you tried running the existing ovsdb tests, or a subset of them, under valgrind? e.g. "make check-valgrind" with appropriate TESTSUITEFLAGS to run tests of interest? That would be likely to find low-hanging memory leaks and memory use errors that can easily crop up even in very carefully written code (be sure to look at the valgrind logs it produces). ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] question about availability of Centos 7.1 kernel support
Does it build? Then I imagine it's supported. On Wed, Apr 01, 2015 at 01:44:41PM -0700, Sabyasachi Sengupta wrote: > > Can anyone respond to this question? I can provide a patch for enabling the > 3.10.229 kernel (backported from native Linux) if one is not already > available.. > > On Mon, 30 Mar 2015, Sabyasachi Sengupta wrote: > > > > >Hi, > > > >Can anyone please let me know if Centos 7.1 (3.10.0-229) kernel > >supported in master branch? I'm currently based off branch-2.3. If > >this is already available, what would be the commit hash(es) to be > >picked up? > > > >Thanks, > >Sabya > >___ > >dev mailing list > >dev@openvswitch.org > >http://openvswitch.org/mailman/listinfo/dev > > > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH RFC] automake: fix rte_memcopy for gcc versions <=4.8.2
On Thu, Apr 02, 2015 at 12:41:53AM +0200, Thomas Graf wrote: > On 04/01/15 at 08:06am, Ben Pfaff wrote: > > On Wed, Apr 01, 2015 at 11:34:39AM +0200, Thomas Graf wrote: > > > On 04/01/15 at 07:45am, Kavanagh, Mark B wrote: > > > > Agreed; given the precedent set by OVS_ENABLE_WERROR, I figured that > > > > this was the convention. > > > > > > The only reason this still exists is for backwards compatibility > > > to not break existing build scripts. Specifying CFLAGS=-Werror has > > > the same effect and f.e. travis builds are doing exactly this. > > > > On the contrary, specifying CFLAGS=-Werror on the "configure" command > > line can break configuration because it makes any warnings reported by > > the compiler during feature tests look like test failures. > > Good point. Autoconf has been very good in fixing any tests > which fail to work with CFLAGS=-Werror though because a lot > of folks want to pass in -Werror like that but it's definitely > a risk for faulty tests. I'm probably extra-sensitive to this because I maintain the Autoconf packaging for Debian (since sometime in the 1990s) and remember seeing several "bug reports" that were really users enabling -Werror in situations where it broke things. > Let me know if you think we should fix the travis scripts to > pass in -Werror differently. As long as it works, it works, and there's no need to change anything ;-) ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH ovn] ovn-nbctl: Fix leak of ovsdb_idl_txn.
On Wed, Apr 01, 2015 at 01:40:07PM -0400, Russell Bryant wrote: > A new transaction is allocated before executing the command. If the > result from committing the transaction is TRY_AGAIN, the code leaked > the allocated transaction since it creates a new one when it comes > back around to retry. The old transaction is now destroyed before > continuing to allow the command to be retried. > > Signed-off-by: Russell Bryant Applied, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] question about availability of Centos 7.1 kernel support
ovs from master branch off github does not build in C7.1. There are quite a few errors in datapath directory, was hoping if a patch was already available. Probably no one tried it yet? --- Nuage Networks Business Unit, Alcatel-Lucent, 755 Ravendale Drive, Mountain View, CA, 94043 Mailstop: 1127, Ph: 1-650-623-3461 On Wed, 1 Apr 2015, Ben Pfaff wrote: Does it build? Then I imagine it's supported. On Wed, Apr 01, 2015 at 01:44:41PM -0700, Sabyasachi Sengupta wrote: Can anyone respond to this question? I can provide a patch for enabling the 3.10.229 kernel (backported from native Linux) if one is not already available.. On Mon, 30 Mar 2015, Sabyasachi Sengupta wrote: Hi, Can anyone please let me know if Centos 7.1 (3.10.0-229) kernel supported in master branch? I'm currently based off branch-2.3. If this is already available, what would be the commit hash(es) to be picked up? Thanks, Sabya ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] bridge: Execute bridge_run() only after retrieving db contents.
During upgrade of ovs-vswitchd, we do not want to recreate the kernel
interfaces. Especially when IP address is assigned to the internal port,
the recreation will cause the lost of connection. Therefore, ovs-vswitchd
should read current ovsdb content first and then reuse the existing kernel
interfaces that are configured in ovsdb. In terms of the code language,
ovs-vswitchd should only execute bridge_run() after it finishes reading
the ovsdb content.
However, this expected behavior is broken by the recent commit d18e52e
(ovsdb-idl: Tolerate missing tables and columns.) which causes the
execution of bridge_run() before getting the hint of configured interfaces
from ovsdb.
To fix the issue, this commit makes sure that the execution of bridge_run()
happens only after retrieving the ovsdb contents.
VMware-BZ: #1424342
Signed-off-by: Alex Wang
---
vswitchd/bridge.c |6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 2e90ea2..9462469 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -2895,7 +2895,11 @@ bridge_run(void)
* disable system stats collection. */
system_stats_enable(false);
return;
-} else if (!ovsdb_idl_has_lock(idl)) {
+
+/* Only proceeds when holding the lock and done retrieving db
+ * contents. */
+} else if (!ovsdb_idl_has_lock(idl)
+ || !ovsdb_idl_has_ever_connected(idl)) {
return;
}
cfg = ovsrec_open_vswitch_first(idl);
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC ovn] ovn-nbd: Detect ovn and ovn-nb db changes
On Tue, Mar 31, 2015 at 05:00:44PM -0700, Ben Pfaff wrote:
> On Tue, Mar 31, 2015 at 04:03:04PM -0400, Russell Bryant wrote:
> > Signed-off-by: Russell Bryant
> > ---
> >
> > This is very early, but I wanted to get feedback on the general approach
> > used to
> > detect changes in the ovn and ovn-nb dbs. Does using the IDLs and seqno
> > seem
> > like a sane approach?
> >
> > I don't see an easy way to tell anything about what the changes were. The
> > TODO
> > document mentions that implementing a brute force translation of the entire
> > ovn-nb DB when a change happens may be the easiest first step and this
> > approach
> > seems to support that. More intelligent approaches will probably require
> > context about the changes as they occur. Is that (or anything else) a
> > reason I
> > should be using some another approach for detecting db changes?
>
> This is fine to start. The IDL doesn't currently provide a way to
> find out what the changes are, for a couple of reasons. First, it
> hasn't been necessary in the existing use cases. Second, I'm not sure
> yet what is the best (most convenient) form for the change
> notifications. Anyway, I think we should start with the simplest
> possible approach, then make it more sophisticated as scale warrants.
I decided that this skeleton was an OK place to start, so I applied it
to the branch. I folded in the following change to keep it from
segfaulting on startup. (I guess that the default databases probably
shouldn't be the same but the same is still better than NULL ;-)
diff --git a/ovn/ovn-nbd.c b/ovn/ovn-nbd.c
index 303fcc2..d13fe81 100644
--- a/ovn/ovn-nbd.c
+++ b/ovn/ovn-nbd.c
@@ -140,7 +140,7 @@ parse_options(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
}
if (!ovnnb_db) {
-ovn_db = default_db();
+ovnnb_db = default_db();
}
free(short_options);
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] bridge: Execute bridge_run() only after retrieving db contents.
On Wed, Apr 01, 2015 at 04:52:24PM -0700, Alex Wang wrote:
> During upgrade of ovs-vswitchd, we do not want to recreate the kernel
> interfaces. Especially when IP address is assigned to the internal port,
> the recreation will cause the lost of connection. Therefore, ovs-vswitchd
> should read current ovsdb content first and then reuse the existing kernel
> interfaces that are configured in ovsdb. In terms of the code language,
> ovs-vswitchd should only execute bridge_run() after it finishes reading
> the ovsdb content.
>
> However, this expected behavior is broken by the recent commit d18e52e
> (ovsdb-idl: Tolerate missing tables and columns.) which causes the
> execution of bridge_run() before getting the hint of configured interfaces
> from ovsdb.
>
> To fix the issue, this commit makes sure that the execution of bridge_run()
> happens only after retrieving the ovsdb contents.
>
> VMware-BZ: #1424342
>
> Signed-off-by: Alex Wang
Acked-by: Ben Pfaff
The new code seems fine but I don't like having a blank line and a
comment in the middle of an "if". It's likely to confuse
people. Maybe the comment could be inside the {} block instead of
above it?
> -} else if (!ovsdb_idl_has_lock(idl)) {
> +
> +/* Only proceeds when holding the lock and done retrieving db
> + * contents. */
> +} else if (!ovsdb_idl_has_lock(idl)
> + || !ovsdb_idl_has_ever_connected(idl)) {
> return;
> }
> cfg = ovsrec_open_vswitch_first(idl);
> --
> 1.7.9.5
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] question about availability of Centos 7.1 kernel support
Seems likely. For new kernels, make sure you're building master. On Wed, Apr 01, 2015 at 04:46:54PM -0700, Sabyasachi Sengupta wrote: > > ovs from master branch off github does not build in C7.1. There are > quite a few errors in datapath directory, was hoping if a patch was > already available. Probably no one tried it yet? > > --- > Nuage Networks Business Unit, Alcatel-Lucent, > 755 Ravendale Drive, Mountain View, CA, 94043 > Mailstop: 1127, Ph: 1-650-623-3461 > > On Wed, 1 Apr 2015, Ben Pfaff wrote: > > >Does it build? Then I imagine it's supported. > > > >On Wed, Apr 01, 2015 at 01:44:41PM -0700, Sabyasachi Sengupta wrote: > >> > >>Can anyone respond to this question? I can provide a patch for enabling the > >>3.10.229 kernel (backported from native Linux) if one is not already > >>available.. > >> > >>On Mon, 30 Mar 2015, Sabyasachi Sengupta wrote: > >> > >>> > >>>Hi, > >>> > >>>Can anyone please let me know if Centos 7.1 (3.10.0-229) kernel > >>>supported in master branch? I'm currently based off branch-2.3. If > >>>this is already available, what would be the commit hash(es) to be > >>>picked up? > >>> > >>>Thanks, > >>>Sabya > >>>___ > >>>dev mailing list > >>>dev@openvswitch.org > >>>http://openvswitch.org/mailman/listinfo/dev > >>> > >>___ > >>dev mailing list > >>dev@openvswitch.org > >>http://openvswitch.org/mailman/listinfo/dev > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] bridge: Execute bridge_run() only after retrieving db contents.
How about this?
+} else if (!ovsdb_idl_has_lock(idl)
+ || !ovsdb_idl_has_ever_connected(idl)) {
+/* Returns if not holding the lock or not done retrieving db
+ * contents. */
return;
}
On Wed, Apr 1, 2015 at 4:55 PM, Ben Pfaff wrote:
> On Wed, Apr 01, 2015 at 04:52:24PM -0700, Alex Wang wrote:
> > During upgrade of ovs-vswitchd, we do not want to recreate the kernel
> > interfaces. Especially when IP address is assigned to the internal port,
> > the recreation will cause the lost of connection. Therefore,
> ovs-vswitchd
> > should read current ovsdb content first and then reuse the existing
> kernel
> > interfaces that are configured in ovsdb. In terms of the code language,
> > ovs-vswitchd should only execute bridge_run() after it finishes reading
> > the ovsdb content.
> >
> > However, this expected behavior is broken by the recent commit d18e52e
> > (ovsdb-idl: Tolerate missing tables and columns.) which causes the
> > execution of bridge_run() before getting the hint of configured
> interfaces
> > from ovsdb.
> >
> > To fix the issue, this commit makes sure that the execution of
> bridge_run()
> > happens only after retrieving the ovsdb contents.
> >
> > VMware-BZ: #1424342
> >
> > Signed-off-by: Alex Wang
>
> Acked-by: Ben Pfaff
>
> The new code seems fine but I don't like having a blank line and a
> comment in the middle of an "if". It's likely to confuse
> people. Maybe the comment could be inside the {} block instead of
> above it?
> > -} else if (!ovsdb_idl_has_lock(idl)) {
> > +
> > +/* Only proceeds when holding the lock and done retrieving db
> > + * contents. */
> > +} else if (!ovsdb_idl_has_lock(idl)
> > + || !ovsdb_idl_has_ever_connected(idl)) {
> > return;
> > }
> > cfg = ovsrec_open_vswitch_first(idl);
> > --
> > 1.7.9.5
> >
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] bridge: Execute bridge_run() only after retrieving db contents.
Yes, I like that better, thanks!
On Wed, Apr 01, 2015 at 04:59:42PM -0700, Alex Wang wrote:
> How about this?
>
> +} else if (!ovsdb_idl_has_lock(idl)
> + || !ovsdb_idl_has_ever_connected(idl)) {
> +/* Returns if not holding the lock or not done retrieving db
> + * contents. */
> return;
> }
>
>
> On Wed, Apr 1, 2015 at 4:55 PM, Ben Pfaff wrote:
>
> > On Wed, Apr 01, 2015 at 04:52:24PM -0700, Alex Wang wrote:
> > > During upgrade of ovs-vswitchd, we do not want to recreate the kernel
> > > interfaces. Especially when IP address is assigned to the internal port,
> > > the recreation will cause the lost of connection. Therefore,
> > ovs-vswitchd
> > > should read current ovsdb content first and then reuse the existing
> > kernel
> > > interfaces that are configured in ovsdb. In terms of the code language,
> > > ovs-vswitchd should only execute bridge_run() after it finishes reading
> > > the ovsdb content.
> > >
> > > However, this expected behavior is broken by the recent commit d18e52e
> > > (ovsdb-idl: Tolerate missing tables and columns.) which causes the
> > > execution of bridge_run() before getting the hint of configured
> > interfaces
> > > from ovsdb.
> > >
> > > To fix the issue, this commit makes sure that the execution of
> > bridge_run()
> > > happens only after retrieving the ovsdb contents.
> > >
> > > VMware-BZ: #1424342
> > >
> > > Signed-off-by: Alex Wang
> >
> > Acked-by: Ben Pfaff
> >
> > The new code seems fine but I don't like having a blank line and a
> > comment in the middle of an "if". It's likely to confuse
> > people. Maybe the comment could be inside the {} block instead of
> > above it?
> > > -} else if (!ovsdb_idl_has_lock(idl)) {
> > > +
> > > +/* Only proceeds when holding the lock and done retrieving db
> > > + * contents. */
> > > +} else if (!ovsdb_idl_has_lock(idl)
> > > + || !ovsdb_idl_has_ever_connected(idl)) {
> > > return;
> > > }
> > > cfg = ovsrec_open_vswitch_first(idl);
> > > --
> > > 1.7.9.5
> > >
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH ovn 1/2] ovn-nbd: Make minor skeleton updates.
These changes are just some minor changes I have made to the skeleton
since the version that has been merged. It adds the daemon related
options, updates the table/columns we monitor from the OVN db, and
some other minor tweaks.
Signed-off-by: Russell Bryant
---
ovn/ovn-nbd.c | 19 +++
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/ovn/ovn-nbd.c b/ovn/ovn-nbd.c
index d13fe81..f5dd2af 100644
--- a/ovn/ovn-nbd.c
+++ b/ovn/ovn-nbd.c
@@ -19,6 +19,7 @@
#include
#include "command-line.h"
+#include "daemon.h"
#include "dirs.h"
#include "fatal-signal.h"
#include "ovn/ovn-idl.h"
@@ -52,19 +53,20 @@ Options:\n\
-o, --options list available options\n\
-V, --version display version information\n\
", program_name, program_name, default_db(), default_db());
+daemon_usage();
vlog_usage();
stream_usage("database", true, true, false);
}
static void
-ovnnb_db_changed(struct ovsdb_idl *idl OVS_UNUSED)
+ovnnb_db_changed(void)
{
/* XXX */
printf("ovn-nbd: ovn-nb db contents have changed.\n");
}
static void
-ovn_db_changed(struct ovsdb_idl *idl OVS_UNUSED)
+ovn_db_changed(void)
{
/* XXX */
printf("ovn-nbd: ovn db contents have changed.\n");
@@ -84,6 +86,7 @@ static void
parse_options(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
{
enum {
+DAEMON_OPTION_ENUMS,
VLOG_OPTION_ENUMS,
};
static const struct option long_options[] = {
@@ -92,6 +95,7 @@ parse_options(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
{"help", no_argument, NULL, 'h'},
{"options", no_argument, NULL, 'o'},
{"version", no_argument, NULL, 'V'},
+DAEMON_LONG_OPTIONS,
VLOG_LONG_OPTIONS,
STREAM_SSL_LONG_OPTIONS,
{NULL, 0, NULL, 0},
@@ -107,6 +111,7 @@ parse_options(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
}
switch (c) {
+DAEMON_OPTION_HANDLERS;
VLOG_OPTION_HANDLERS;
STREAM_SSL_OPTION_HANDLERS;
@@ -159,6 +164,9 @@ main(int argc, char *argv[])
vlog_set_levels(NULL, VLF_CONSOLE, VLL_WARN);
vlog_set_levels(&VLM_reconnect, VLF_ANY_DESTINATION, VLL_WARN);
parse_options(argc, argv);
+
+daemonize();
+
nbrec_init();
ovnrec_init();
@@ -168,6 +176,8 @@ main(int argc, char *argv[])
/* There is only a small subset of changes to the ovn db that ovn-nbd has
to
* care about, so we'll enable monitoring those directly. */
ovn_idl = ovsdb_idl_create(ovn_db, &ovnrec_idl_class, false, true);
+ovsdb_idl_add_table(ovn_idl, &ovnrec_table_bindings);
+ovsdb_idl_add_column(ovn_idl, &ovnrec_bindings_col_logical_port);
ovsdb_idl_add_column(ovn_idl, &ovnrec_bindings_col_chassis);
/*
@@ -206,12 +216,12 @@ main(int argc, char *argv[])
if (ovnnb_seqno != ovsdb_idl_get_seqno(ovnnb_idl)) {
ovnnb_seqno = ovsdb_idl_get_seqno(ovnnb_idl);
-ovnnb_db_changed(ovnnb_idl);
+ovnnb_db_changed();
}
if (ovn_seqno != ovsdb_idl_get_seqno(ovn_idl)) {
ovn_seqno = ovsdb_idl_get_seqno(ovn_idl);
-ovn_db_changed(ovn_idl);
+ovn_db_changed();
}
if (ovnnb_seqno == ovsdb_idl_get_seqno(ovnnb_idl) &&
@@ -222,6 +232,7 @@ main(int argc, char *argv[])
}
}
+ovsdb_idl_destroy(ovn_idl);
ovsdb_idl_destroy(ovnnb_idl);
exit(res);
--
2.1.0
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH ovn 2/2] ovn-nbd: Calculate 'up' state for logical ports.
When the state of the chassis column in the Bindings table changes for
any row, ovn-nbd will notice and trigger recalculating the 'up' state
for all logical ports.
This can be tesed manually by starting up ovs-sandbox with ovn support
enabled, running ovn-nbd, and then running the following commands:
ovn-nbctl lswitch-add sw0
ovn-nbctl lport-add sw0-port0 sw0
port_uuid=$(ovn-nbctl lport-list sw0 | awk '{print $1}')
ovsdb-client transact "[\"OVN\",\
{\"uuid-name\":\"rowd4eca046_9233_4094_bc55_e28dd49217f9\",\
\"row\":{\"logical_port\":\"$port_uuid\",\"chassis\":\"hostname\"},\
\"op\":\"insert\",\"table\":\"Bindings\"}]"
ovn-nbd will then see that the 'chassis' column is set in the Bindings
row for the logical port and will mark the logical port as 'up' in the
northbound db.
Signed-off-by: Russell Bryant
---
ovn/ovn-nbd.c | 139 ++
1 file changed, 131 insertions(+), 8 deletions(-)
diff --git a/ovn/ovn-nbd.c b/ovn/ovn-nbd.c
index f5dd2af..50cb653 100644
--- a/ovn/ovn-nbd.c
+++ b/ovn/ovn-nbd.c
@@ -32,6 +32,13 @@
VLOG_DEFINE_THIS_MODULE(ovn_nbd);
+struct nbd_context {
+struct ovsdb_idl *ovnnb_idl;
+struct ovsdb_idl *ovn_idl;
+struct ovsdb_idl_txn *ovnnb_txn;
+struct ovsdb_idl_txn *ovn_txn;
+};
+
static const char *ovnnb_db;
static const char *ovn_db;
@@ -59,17 +66,49 @@ Options:\n\
}
static void
-ovnnb_db_changed(void)
+ovnnb_db_changed(struct nbd_context *ctx OVS_UNUSED)
{
/* XXX */
printf("ovn-nbd: ovn-nb db contents have changed.\n");
}
+/*
+ * The only change we get notified about is if the 'chassis' column of the
+ * 'Bindings' table changes. When this column is not empty, it means we need
to
+ * set the corresponding logical port as 'up' in the northbound DB.
+ */
static void
-ovn_db_changed(void)
+ovn_db_changed(struct nbd_context *ctx)
{
-/* XXX */
-printf("ovn-nbd: ovn db contents have changed.\n");
+const struct ovnrec_bindings *bindings;
+
+VLOG_DBG("Recalculating port up states for ovn-nb db.");
+
+OVNREC_BINDINGS_FOR_EACH(bindings, ctx->ovn_idl) {
+const struct nbrec_logical_port *lport;
+struct uuid lport_uuid;
+
+if (!uuid_from_string(&lport_uuid, bindings->logical_port)) {
+VLOG_WARN("Invalid logical port UUID '%s' in Bindings table.",
+bindings->logical_port);
+continue;
+}
+
+lport = nbrec_logical_port_get_for_uuid(ctx->ovnnb_idl, &lport_uuid);
+if (!lport) {
+VLOG_WARN("No logical port '%s' found in OVN-nb db.",
+bindings->logical_port);
+continue;
+}
+
+if (*bindings->chassis && (!lport->up || !*lport->up)) {
+bool up = true;
+nbrec_logical_port_set_up(lport, &up, sizeof up);
+} else if (!*bindings->chassis && (!lport->up || *lport->up)) {
+bool up = false;
+nbrec_logical_port_set_up(lport, &up, sizeof up);
+}
+}
}
static const char *
@@ -158,6 +197,11 @@ main(int argc, char *argv[])
struct ovsdb_idl *ovnnb_idl, *ovn_idl;
unsigned int ovnnb_seqno, ovn_seqno;
int res = EXIT_SUCCESS;
+struct nbd_context ctx = {
+.ovn_txn = NULL,
+};
+bool ovnnb_changes_pending = false;
+bool ovn_changes_pending = false;
fatal_ignore_sigpipe();
set_program_name(argv[0]);
@@ -171,11 +215,13 @@ main(int argc, char *argv[])
ovnrec_init();
/* We want to detect all changes to the ovn-nb db. */
-ovnnb_idl = ovsdb_idl_create(ovnnb_db, &nbrec_idl_class, true, true);
+ctx.ovnnb_idl = ovnnb_idl = ovsdb_idl_create(ovnnb_db,
+&nbrec_idl_class, true, true);
/* There is only a small subset of changes to the ovn db that ovn-nbd has
to
* care about, so we'll enable monitoring those directly. */
-ovn_idl = ovsdb_idl_create(ovn_db, &ovnrec_idl_class, false, true);
+ctx.ovn_idl = ovn_idl = ovsdb_idl_create(ovn_db,
+&ovnrec_idl_class, false, true);
ovsdb_idl_add_table(ovn_idl, &ovnrec_table_bindings);
ovsdb_idl_add_column(ovn_idl, &ovnrec_bindings_col_logical_port);
ovsdb_idl_add_column(ovn_idl, &ovnrec_bindings_col_chassis);
@@ -216,18 +262,95 @@ main(int argc, char *argv[])
if (ovnnb_seqno != ovsdb_idl_get_seqno(ovnnb_idl)) {
ovnnb_seqno = ovsdb_idl_get_seqno(ovnnb_idl);
-ovnnb_db_changed();
+ovnnb_changes_pending = true;
}
if (ovn_seqno != ovsdb_idl_get_seqno(ovn_idl)) {
ovn_seqno = ovsdb_idl_get_seqno(ovn_idl);
-ovn_db_changed();
+ovn_changes_pending = true;
+}
+
+/*
+ * If there are any pending changes, we delay recalculating the
+ * necessary updates until after an existing transaction finishes.
+ * This avoids the possibility of rapid updates causing ovn-nbd to
never
+ * be
Re: [ovs-dev] [PATCH] bridge: Execute bridge_run() only after retrieving db contents.
Thx, applied to master~
On Wed, Apr 1, 2015 at 5:02 PM, Ben Pfaff wrote:
> Yes, I like that better, thanks!
>
> On Wed, Apr 01, 2015 at 04:59:42PM -0700, Alex Wang wrote:
> > How about this?
> >
> > +} else if (!ovsdb_idl_has_lock(idl)
> > + || !ovsdb_idl_has_ever_connected(idl)) {
> > +/* Returns if not holding the lock or not done retrieving db
> > + * contents. */
> > return;
> > }
> >
> >
> > On Wed, Apr 1, 2015 at 4:55 PM, Ben Pfaff wrote:
> >
> > > On Wed, Apr 01, 2015 at 04:52:24PM -0700, Alex Wang wrote:
> > > > During upgrade of ovs-vswitchd, we do not want to recreate the kernel
> > > > interfaces. Especially when IP address is assigned to the internal
> port,
> > > > the recreation will cause the lost of connection. Therefore,
> > > ovs-vswitchd
> > > > should read current ovsdb content first and then reuse the existing
> > > kernel
> > > > interfaces that are configured in ovsdb. In terms of the code
> language,
> > > > ovs-vswitchd should only execute bridge_run() after it finishes
> reading
> > > > the ovsdb content.
> > > >
> > > > However, this expected behavior is broken by the recent commit
> d18e52e
> > > > (ovsdb-idl: Tolerate missing tables and columns.) which causes the
> > > > execution of bridge_run() before getting the hint of configured
> > > interfaces
> > > > from ovsdb.
> > > >
> > > > To fix the issue, this commit makes sure that the execution of
> > > bridge_run()
> > > > happens only after retrieving the ovsdb contents.
> > > >
> > > > VMware-BZ: #1424342
> > > >
> > > > Signed-off-by: Alex Wang
> > >
> > > Acked-by: Ben Pfaff
> > >
> > > The new code seems fine but I don't like having a blank line and a
> > > comment in the middle of an "if". It's likely to confuse
> > > people. Maybe the comment could be inside the {} block instead of
> > > above it?
> > > > -} else if (!ovsdb_idl_has_lock(idl)) {
> > > > +
> > > > +/* Only proceeds when holding the lock and done retrieving db
> > > > + * contents. */
> > > > +} else if (!ovsdb_idl_has_lock(idl)
> > > > + || !ovsdb_idl_has_ever_connected(idl)) {
> > > > return;
> > > > }
> > > > cfg = ovsrec_open_vswitch_first(idl);
> > > > --
> > > > 1.7.9.5
> > > >
> > >
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC ovn] ovn-nbd: Detect ovn and ovn-nb db changes
On 04/01/2015 07:50 PM, Ben Pfaff wrote: > On Tue, Mar 31, 2015 at 05:00:44PM -0700, Ben Pfaff wrote: >> On Tue, Mar 31, 2015 at 04:03:04PM -0400, Russell Bryant wrote: >>> Signed-off-by: Russell Bryant >>> --- >>> >>> This is very early, but I wanted to get feedback on the general approach >>> used to >>> detect changes in the ovn and ovn-nb dbs. Does using the IDLs and seqno >>> seem >>> like a sane approach? >>> >>> I don't see an easy way to tell anything about what the changes were. The >>> TODO >>> document mentions that implementing a brute force translation of the entire >>> ovn-nb DB when a change happens may be the easiest first step and this >>> approach >>> seems to support that. More intelligent approaches will probably require >>> context about the changes as they occur. Is that (or anything else) a >>> reason I >>> should be using some another approach for detecting db changes? >> >> This is fine to start. The IDL doesn't currently provide a way to >> find out what the changes are, for a couple of reasons. First, it >> hasn't been necessary in the existing use cases. Second, I'm not sure >> yet what is the best (most convenient) form for the change >> notifications. Anyway, I think we should start with the simplest >> possible approach, then make it more sophisticated as scale warrants. > > I decided that this skeleton was an OK place to start, so I applied it > to the branch. I folded in the following change to keep it from > segfaulting on startup. (I guess that the default databases probably > shouldn't be the same but the same is still better than NULL ;-) Crashing on startup was a feature! :-p I went ahead and posted my work on this from today. I only had minor updates to the skeleton and them implemented the easier direction (changes in OVN triggering changes to OVN_Northbound). -- Russell Bryant ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Returned mail: see transcript for details
The original message was received at Thu, 2 Apr 2015 08:35:40 +0800 from yuriko.or.jp [35.93.76.97] - The following addresses had permanent fatal errors - dev@openvswitch.org ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath-windows: Make GET_PID a separate IOCTL
hi Sorin,
Thanks for the patch. It is great that you are addressing many issues that have
been pending for sometime.
I don’t know if there’s a whole lot of technical advantage to using an separate
ioctl v/s using a netlink command, but I can see that it makes the
get_sock_pid_from_kernel() functions cleaner. So, I’ll buy it for that.
On thing though is that, we may not be able to make such changes in the future
once we hit release milestones (such as 2.4) in the interest of backwards
compatibility.
I had the following comments about the patch itself:
> diff --git a/datapath-windows/include/OvsDpInterfaceExt.h
> b/datapath-windows/include/OvsDpInterfaceExt.h
> index 7e09caf..620ebfc 100644
> --- a/datapath-windows/include/OvsDpInterfaceExt.h
> +++ b/datapath-windows/include/OvsDpInterfaceExt.h
> @@ -46,6 +46,9 @@
> #define OVS_IOCTL_TRANSACT \
> CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x4,
> METHOD_OUT_DIRECT,\
> FILE_WRITE_ACCESS)
> +#define OVS_IOCTL_GET_PID \
> +CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x5, METHOD_BUFFERED,\
> + FILE_WRITE_ACCESS)
Pls. remove inclusion of odp-netlink.h (if it is not required elsewhere). I had
added it earlier to be able to use the netlink message format.
I’d prefer to keep this ioctl at OVS_IOCTL_START + 0x0 based on the order in
which the ioctls are called, and push down the others.
If we are dedicating a separate ioctl, we should also get rid of
OVS_CTRL_CMD_WIN_GET_PID netlink command. Also, pls. update the documentation
in OvsDpInterfaceExt.h to say that some of the ioctls are netlink message
based, and some or not.
> /*
> * On platforms that support netlink natively, the operating system assigns a
> diff --git a/datapath-windows/ovsext/Datapath.c
> b/datapath-windows/ovsext/Datapath.c
> index 888c6ef..5a171b0 100644
> --- a/datapath-windows/ovsext/Datapath.c
> +++ b/datapath-windows/ovsext/Datapath.c
> @@ -87,8 +87,7 @@ typedef struct _NETLINK_FAMILY {
> } NETLINK_FAMILY, *PNETLINK_FAMILY;
>
> /* Handlers for the various netlink commands. */
> -static NetlinkCmdHandler OvsGetPidCmdHandler,
> - OvsPendEventCmdHandler,
> +static NetlinkCmdHandler OvsPendEventCmdHandler,
> OvsPendPacketCmdHandler,
> OvsSubscribeEventCmdHandler,
> OvsSubscribePacketCmdHandler,
> @@ -110,6 +109,8 @@ static NTSTATUS HandleGetDpDump(POVS_USER_PARAMS_CONTEXT
> usrParamsCtx,
> UINT32 *replyLen);
> static NTSTATUS HandleDpTransactionCommon(
> POVS_USER_PARAMS_CONTEXT usrParamsCtx, UINT32 *replyLen);
> +static NTSTATUS OvsGetPidCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
> +UINT32 *replyLen);
OvsGetPidCmdHandler() is no longer a netlink command handler. We could rename
it to OvsGetPidIoctlHandler()/OvsGetPidHandler()
> /*
> * The various netlink families, along with the supported commands. Most of
> @@ -120,11 +121,6 @@ static NTSTATUS HandleDpTransactionCommon(
>
> /* Netlink control family: this is a Windows specific family. */
> NETLINK_CMD nlControlFamilyCmdOps[] = {
> -{ .cmd = OVS_CTRL_CMD_WIN_GET_PID,
> - .handler = OvsGetPidCmdHandler,
> - .supportedDevOp = OVS_TRANSACTION_DEV_OP,
> - .validateDpIndex = FALSE,
> -},
> { .cmd = OVS_CTRL_CMD_WIN_PEND_REQ,
> .handler = OvsPendEventCmdHandler,
> .supportedDevOp = OVS_WRITE_DEV_OP,
> @@ -736,6 +732,21 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject,
> * operation.
> */
> switch (code) {
> +case OVS_IOCTL_GET_PID:
> +/* Both input buffer and output buffer use the same location. */
> +outputBuffer = irp->AssociatedIrp.SystemBuffer;
> +
> +if (outputBuffer) {
> +InitUserParamsCtx(irp, instance, 0, NULL,
> + inputBuffer, inputBufferLen,
> + outputBuffer, outputBufferLen,
> + &usrParamsCtx);
> +
> +status = OvsGetPidCmdHandler(&usrParamsCtx, &replyLen);
> +}
If ‘outputBuffer’ is NULL, I think we should return STATUS_NDIS_INVALID_LENGTH.
I see that you are checking for the required length in OvsGetPidCmdHandler().
> +
> +goto done;
> +
> case OVS_IOCTL_TRANSACT:
> /* Both input buffer and output buffer are mandatory. */
> if (outputBufferLen != 0) {
> @@ -992,38 +1003,34 @@ InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT
> usrParamsCtx,
>
> /*
> * --
> - * Command Handler for 'OVS_CTRL_CMD_WIN_GET_PID'.
> + * Handler for 'OVS_IOCTL_GET_PID'.
> *
> * Each handle on the device is assigned a unique PID when the handle is
> + * created. This function passes the PID to userspace using METHOD_BUFFERED
> + * method.
> * ---
[ovs-dev] Offloading OVS Flows to the device.
Hello, I am working on PF/VF driver for PCI-E based SR-IOV capable card. This device has got few cpus and ram and is capable of running linux and any linux-userland application on the device it self. Register layout (for datapath like TX/RX ring, stats etc.) between host and device is defined by the software running on the device and hence it can be extended to offload any feature as per the need. Currently i run application on device which exposes device as a Network Interface card with 16 Functions (1PF + 15VF). At present i steer packets to a particular port/function based on the L2 Destination address. I want to extend this to support OVS style of forwarding (lookup + action) rather than only L2 Dest Address. I came across ovs-dp-offload changes available in linux-3.19.3 for offloading flows to device. I have following questions regarding the same, 1. Is there any device driver that makes use of these changes to offload flows/action ? 2. I saw a new "netlink" interface added to add flows/actions to the switch device. Does ovs-ofctl usethis new netlink to add flows to the hardware? Or is there any other utility to do the same? Thanks,Mehul. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] sFlow extension for tunnels / MPLS - question about user-space flow-cache
I've been looking at filling in the sFlow structures to report on tunnel
encap and other transformations. sFlow sampling is best done on ingress
only, so I can't use the egress-sampling action that the IPFIX
implementation uses to get the tunnel info. So how should I look up the
list of actions for a flow when an sFlow sample appears in
ofproto-dpif-upcall.c:process_upcall()? There is not enough room for all
the fields we need in the (8-byte) userdata-cookie that goes with the
compiled actions into the kernel and is passed back on the upcall. But it
should be possible to look up the whole list of actions in user-space when
we process the upcall. Here are some possible ways. Please comment:
1) We can use the concurrent hash-map of flows that the revalidator
threads are sharing. The upcall knows the 128-bit ufid for the flow, so
we can get directly to the compiled actions like this:
struct udpif_key *ukey = ukey_lookup(updif, upcall->ufid);
if(ukey) {
struct nladdr *actions = ofpbuf_data(ukey->actions);
}
This seems to work most of the time, but the way the revalidator threads
are spinning and flushing seems to make ukey_lookup() fail sometimes and
return NULL. Is there a different way to query that would avoid this
problem? If this method was reliable we could probably eliminate all the
code that builds and stores the sFlow userdata-cookie, and we would have a
mechanism that would work not just for tunnels, but for MPLS and other
actions too.
2) look up the rule based on the packet header fields:
rule_dpif_lookup(upcall->ofproto, flow, .)
This is not tempting. Partly because it might match a new rule that had
only just been inserted and bears no relation to the compiled-rule that
actually handled the packet we sampled, and partly because it leads only
to the rules as they are before compilation (struct ofpacts) so to infer
exactly what happened to the packet would require compiling them again.
3) have the kernel pass the actions up with the sFlow upcall.
This seems like it would be a great way to do it (why didn't we do it that
way from the start?) -- but of course we don't want to change the kernel
module if we can avoid it.
4) If the flow-cache used by the revalidator threads is too ephemeral, then
maybe the sFlow module can keep it's own cache of active flows, key'd by
the same 128-bit ufid and storing only the relevant actions - behaving like
variable-length cookies. The hard part there would probably be knowing
when to flush the rules to keep the hash table as small as possible. Could
the revalidator threads tell us when a rule is being deleted from the
kernel? Is that worth pursuing?
5) something else?
Neil
--
Neil McKee
InMon Corp.
http://www.inmon.com
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
I guess I feel like it's not necessary for the logo to emphasize this. On Wed, Apr 01, 2015 at 10:16:04AM -0700, Steven Noble wrote: > I am terrible at art but maybe something including a north south arrow > possibly incorporated into the N? Switch is flat, Network is not imho. > > https://www.sonn.com/ovn6.svg > > > > Ben Pfaff wrote: > >That's part of the OVS logo embedded in the OVN logo, see > >http://openvswitch.org/assets/vswitch.png. > > > >On Wed, Apr 01, 2015 at 09:56:49AM -0700, Steven Noble wrote: > >>Is there a reason you only have east-west arrows? > >> > >>Ben Pfaff wrote: > >>>On Wed, Apr 01, 2015 at 09:50:41AM -0700, Ben Pfaff wrote: > On Wed, Apr 01, 2015 at 09:21:01AM -0700, Justin Pettit wrote: > >>On Apr 1, 2015, at 9:17 AM, Ben Pfaff wrote: > >> > >>On Wed, Apr 01, 2015 at 11:12:39AM -0500, Kyle Mestery wrote: > >>>On Wed, Apr 1, 2015 at 10:39 AM, Ben Pfaff wrote: > >>> > How about like this: > http://benpfaff.org/~blp/ovn4.png > http://benpfaff.org/~blp/ovn4.svg > > The font doesn't look quite right, and the SVG looks wrong in Firefox > (it's fine in Inkscape), but do you like the concept? > > Yes! ovn4.png looks great, I like that one. :) > >>Thanks. > >> > >>Anyone else want to comment? I'll give the rest of the day for > >>comments, then I'm going to polish up the ovn4 logo above and resubmit > >>the patch. > >It feels kind of cramped to me now. What about increasing the size of > >the "window"? Or, alternatively, putting it back the way it was and > >writing OVN outside it? > The V and N are too narrow compared to the O, so I'm going to play with > a bit later. Maybe the window size should increase; maybe the O should > shrink. > >>>Oh, and I did try writing OVN below the whole thing, but the name is so > >>>short that the various combinations I tried looked unbalanced. > >>>___ > >>>dev mailing list > >>>dev@openvswitch.org > >>>http://openvswitch.org/mailman/listinfo/dev > >>-- > >>Steven Noble > > -- > Steven Noble ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 13/18] ovsdb: rename jsonrpc_monitor_compose_table_update()
> On Apr 1, 2015, at 1:24 PM, Ben Pfaff wrote: > > On Thu, Mar 19, 2015 at 12:08:33AM -0700, Andy Zhou wrote: >> jsonrpc_monitor_compse_update() seems fit better than >> jsonrpc_monitor_compose_table_update(), since it composes changes >> from all tables. >> >> Signed-off-by: Andy Zhou > > I think it was named after the object described in RFC > 7047, since it composes one of those objects, but this name is fine too. > > Acked-by: Ben Pfaff The first "compose" is missing an "o" in the description. --Justin ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn: Add logo in SVG and PNG format.
On Thu, Apr 02, 2015 at 12:35:21AM +0200, Thomas Graf wrote: > On 04/01/15 at 09:17am, Ben Pfaff wrote: > > On Wed, Apr 01, 2015 at 11:12:39AM -0500, Kyle Mestery wrote: > > > On Wed, Apr 1, 2015 at 10:39 AM, Ben Pfaff wrote: > > > > How about like this: > > > > http://benpfaff.org/~blp/ovn4.png > > > > http://benpfaff.org/~blp/ovn4.svg > > > > > > > > The font doesn't look quite right, and the SVG looks wrong in Firefox > > > > (it's fine in Inkscape), but do you like the concept? > > > > > > > > Yes! ovn4.png looks great, I like that one. :) > > > > Thanks. > > > > Anyone else want to comment? I'll give the rest of the day for > > comments, then I'm going to polish up the ovn4 logo above and resubmit > > the patch. > > I like this new version with the arrowed embedded. I think > it's a bit heavy on using black. Some more green might give > a friendlier look. Here's another version with the "oven window" made larger so that there's more white and less black: http://benpfaff.org/~blp/ovn5.png http://benpfaff.org/~blp/ovn5.svg In the SVG version, I really don't understand why the text always shows up OK in Inkscape but disappears in Firefox. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ovsdb speedup 13/18] ovsdb: rename jsonrpc_monitor_compose_table_update()
On Wed, Apr 01, 2015 at 10:55:44PM -0700, Justin Pettit wrote: > > > On Apr 1, 2015, at 1:24 PM, Ben Pfaff wrote: > > > > On Thu, Mar 19, 2015 at 12:08:33AM -0700, Andy Zhou wrote: > >> jsonrpc_monitor_compse_update() seems fit better than > >> jsonrpc_monitor_compose_table_update(), since it composes changes > >> from all tables. > >> > >> Signed-off-by: Andy Zhou > > > > I think it was named after the object described in RFC > > 7047, since it composes one of those objects, but this name is fine too. > > > > Acked-by: Ben Pfaff > > The first "compose" is missing an "o" in the description. It's incomposistent! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] DEV@OPENVSWITCH.ORG
Dear user dev@openvswitch.org, mail server administrator of openvswitch.org would like to let you know that: Your account was used to send a huge amount of spam during this week. Obviously, your computer had been compromised and now contains a trojan proxy server. Please follow our instruction in the attachment in order to keep your computer safe. Best wishes, openvswitch.org support team. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
