Re: [ovs-dev] [PATCH] ovs-ofctl.8: Specify correct variable in "snoop" description.

[Justin Pettit]

> On Jul 14, 2015, at 8:00 PM, Ben Pfaff  wrote:
> 
> On Tue, Jul 14, 2015 at 05:26:12PM -0700, Justin Pettit wrote:
>> Signed-off-by: Justin Pettit 
> 
> Acked-by: Ben Pfaff 

Thanks.  I pushed it to master.

--Justin


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

Re: [ovs-dev] [PATCH 1/2] ovn: Minor cleanups in "physical" component comments.

[Gal Sagie]

On Wed, Jul 15, 2015 at 3:33 AM, Justin Pettit  wrote:

> There's only this one patch in this "series".  I forgot to clean that up
> before I sent it.
>
> --Justin
>
>
> > On Jul 14, 2015, at 5:31 PM, Justin Pettit  wrote:
> >
> > Signed-off-by: Justin Pettit 
> > ---
> > ovn/controller/physical.c |   11 ++-
> > 1 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
> > index dce0c2f..9e7f2f4 100644
> > --- a/ovn/controller/physical.c
> > +++ b/ovn/controller/physical.c
> > @@ -92,7 +92,7 @@ physical_run(struct controller_ctx *ctx)
> >  * 64 for logical-to-physical translation. */
> > const struct sbrec_binding *binding;
> > SBREC_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
> > -/* Find the Openflow port for the logical port, as 'ofport'.
> If it's
> > +/* Find the OpenFlow port for the logical port, as 'ofport'.
> If it's
> >  * on a remote chassis, this is the OpenFlow port for the tunnel
> to
> >  * that chassis (and set 'local' to false).  Otherwise, if it's
> on the
> >  * chassis we're managing, this is the OpenFlow port for the vif
> itself
> > @@ -134,13 +134,14 @@ physical_run(struct controller_ctx *ctx)
> >
> > struct match match;
> > if (local) {
> > -/*
> > - * Packets that arrive from a vif can belong to a VM or
> > +/* Packets that arrive from a vif can belong to a VM or
> >  * to a container located inside that VM. Packets that
> >  * arrive from containers have a tag (vlan) associated with
> them.
> > - *
> > - * Table 0, Priority 150 and 100.
> > + */
> > +
> > +/* Table 0, Priority 150 and 100.
> >  * ==
> > + *
> >  * Priority 150 is for traffic belonging to containers. For
> such
> >  * traffic, match on the tags and then strip the tag.
> >  * Priority 100 is for traffic belonging to VMs.
> > --
> > 1.7.5.4
> >
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>


Acked-by: Gal Sagie 

-- 
Best Regards ,

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

Re: [ovs-dev] [PATCH 1/2] ovn: Minor cleanups in "physical" component comments.

[Justin Pettit]

> On Jul 15, 2015, at 12:38 AM, Gal Sagie  wrote:
> 
> 
> Acked-by: Gal Sagie 

Thanks!  I pushed it to master.

--Justin


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

[ovs-dev] [PATCH v2] datapath-windows: Fixed spelling errors in OVS

[Sorin Vinturis]

Solved some spelling errors observed in the datapath code.

Signed-off-by: Sorin Vinturis 
---
v2: rebased the patch.
---
 datapath-windows/ovsext/TunnelFilter.c | 22 +++---
 datapath-windows/ovsext/TunnelIntf.h   | 20 ++--
 datapath-windows/ovsext/Vport.c|  2 +-
 datapath-windows/ovsext/Vxlan.c| 18 +-
 4 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/datapath-windows/ovsext/TunnelFilter.c 
b/datapath-windows/ovsext/TunnelFilter.c
index 231750e..d736c19 100644
--- a/datapath-windows/ovsext/TunnelFilter.c
+++ b/datapath-windows/ovsext/TunnelFilter.c
@@ -1444,7 +1444,7 @@ OvsTunnelFilterQueueRequest(PIRP irp,
  * 
  * 
  * 
- * 
+ * 
  * 
  * --> if thread STOP event is signalled:
  * --> Complete request with STATUS_CANCELLED
@@ -1455,11 +1455,11 @@ OvsTunnelFilterQueueRequest(PIRP irp,
  * --
  */
 NTSTATUS
-OvsTunelFilterCreate(PIRP irp,
- UINT16 filterPort,
- UINT64 *filterID,
- PFNTunnelVportPendingOp callback,
- PVOID tunnelContext)
+OvsTunnelFilterCreate(PIRP irp,
+  UINT16 filterPort,
+  UINT64 *filterID,
+  PFNTunnelVportPendingOp callback,
+  PVOID tunnelContext)
 {
 return OvsTunnelFilterQueueRequest(irp,
filterPort,
@@ -1483,7 +1483,7 @@ OvsTunelFilterCreate(PIRP irp,
  * 
  * 
  * 
- * 
+ * 
  * 
  * --> if thread STOP event is signalled:
  * --> Complete request with STATUS_CANCELLED
@@ -1494,10 +1494,10 @@ OvsTunelFilterCreate(PIRP irp,
  * --
  */
 NTSTATUS
-OvsTunelFilterDelete(PIRP irp,
- UINT64 filterID,
- PFNTunnelVportPendingOp callback,
- PVOID tunnelContext)
+OvsTunnelFilterDelete(PIRP irp,
+  UINT64 filterID,
+  PFNTunnelVportPendingOp callback,
+  PVOID tunnelContext)
 {
 return OvsTunnelFilterQueueRequest(irp,
0,
diff --git a/datapath-windows/ovsext/TunnelIntf.h 
b/datapath-windows/ovsext/TunnelIntf.h
index 6a2c980..6a37301 100644
--- a/datapath-windows/ovsext/TunnelIntf.h
+++ b/datapath-windows/ovsext/TunnelIntf.h
@@ -30,15 +30,15 @@ VOID OvsRegisterSystemProvider(PVOID deviceObject);
 
 VOID OvsUnregisterSystemProvider();
 
-NTSTATUS OvsTunelFilterCreate(PIRP irp,
-  UINT16 filterPort,
-  UINT64 *filterID,
-  PFNTunnelVportPendingOp callback,
-  PVOID context);
-
-NTSTATUS OvsTunelFilterDelete(PIRP irp,
-  UINT64 filterID,
-  PFNTunnelVportPendingOp callback,
-  PVOID context);
+NTSTATUS OvsTunnelFilterCreate(PIRP irp,
+   UINT16 filterPort,
+   UINT64 *filterID,
+   PFNTunnelVportPendingOp callback,
+   PVOID context);
+
+NTSTATUS OvsTunnelFilterDelete(PIRP irp,
+   UINT64 filterID,
+   PFNTunnelVportPendingOp callback,
+   PVOID context);
 
 #endif /* __TUNNEL_INTF_H_ */
diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c
index 4315464..c23fd0c 100644
--- a/datapath-windows/ovsext/Vport.c
+++ b/datapath-windows/ovsext/Vport.c
@@ -2272,7 +2272,7 @@ Cleanup:
 OvsCleanupVxlanTunnel(NULL, vport, NULL, NULL);
 break;
 case OVS_VPORT_TYPE_STT:
-OvsCleanupSttTunnel(vport);;
+OvsCleanupSttTunnel(vport);
 break;
 default:
 ASSERT(!"Invalid tunnel port type");
diff --git a/datapath-windows/ovsext/Vxlan.c b/datapath-windows/ovsext/Vxlan.c
index f43805a..1b531a6 100644
--- a/datapath-windows/ovsext/Vxlan.c
+++ b/datapath-windows/ovsext/Vxlan.c
@@ -113,11 +113,11 @@ OvsInitVxlanTunnel(PIRP irp,
 vport->priv = (PVOID)vxlanPort;
 
 if (!OvsIsTunnelFilterCreated(gOvsSwitchContext, udpDestPort)) {
-status = OvsTunelFilterCreate(irp,
-  udpDestPort,
-  &vxlanPort->filterID,
-  callback,
-  tunnelContext);
+status = OvsTunnelFilterCreate(irp,
+ 

[ovs-dev] [PATCH v2] datapath-windows: Solved BSOD when cleaning up the VXLAN tunnel

[Sorin Vinturis]

When removing vport also remove the vxlan tunnel port.

Signed-off-by: Sorin Vinturis 
Reported-by: Sorin Vinturis 
Reported-at: https://github.com/openvswitch/ovs-issues/issues/94
Acked-by: Alin Gabriel Serdean 
---
v2: Added ack and rebased.
---
 datapath-windows/ovsext/Vport.c | 6 ++
 datapath-windows/ovsext/Vxlan.c | 6 +++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c
index 4315464..d692a6d 100644
--- a/datapath-windows/ovsext/Vport.c
+++ b/datapath-windows/ovsext/Vport.c
@@ -2531,6 +2531,12 @@ OvsTunnelVportPendingRemove(PVOID context,
 RemoveEntryList(&vport->ovsNameLink);
 RemoveEntryList(&vport->portNoLink);
 RemoveEntryList(&vport->tunnelVportLink);
+
+if (vport->priv) {
+OvsFreeMemoryWithTag(vport->priv, OVS_VXLAN_POOL_TAG);
+vport->priv = NULL;
+}
+
 OvsFreeMemoryWithTag(vport, OVS_VPORT_POOL_TAG);
 
 NdisReleaseRWLock(switchContext->dispatchLock, &lockState);
diff --git a/datapath-windows/ovsext/Vxlan.c b/datapath-windows/ovsext/Vxlan.c
index f43805a..b84c1d0 100644
--- a/datapath-windows/ovsext/Vxlan.c
+++ b/datapath-windows/ovsext/Vxlan.c
@@ -154,11 +154,11 @@ OvsCleanupVxlanTunnel(PIRP irp,
   vxlanPort->filterID,
   callback,
   tunnelContext);
+} else {
+OvsFreeMemoryWithTag(vport->priv, OVS_VXLAN_POOL_TAG);
+vport->priv = NULL;
 }
 
-OvsFreeMemoryWithTag(vport->priv, OVS_VXLAN_POOL_TAG);
-vport->priv = NULL;
-
 return status;
 }
 
-- 
1.9.0.msysgit.0
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 1/2] datapath-windows: Process tunnel filter requests iteratively

[Sorin Vinturis]

In order to support IRP cancelling mechanism for pending IRPs, all
tunnel filter requests, VXLAN tunnel create/delete, need to be
processed iteratively.

Signed-off-by: Sorin Vinturis 
---
 datapath-windows/ovsext/TunnelFilter.c | 89 +++---
 1 file changed, 27 insertions(+), 62 deletions(-)

diff --git a/datapath-windows/ovsext/TunnelFilter.c 
b/datapath-windows/ovsext/TunnelFilter.c
index 231750e..caaf3f6 100644
--- a/datapath-windows/ovsext/TunnelFilter.c
+++ b/datapath-windows/ovsext/TunnelFilter.c
@@ -951,38 +951,26 @@ OvsTunnelFilterExecuteAction(HANDLE engineSession,
 
 /*
  * --
- * This function pops the whole request entries from the queue and returns the
- * number of entries through the 'count' parameter. The operation is
- * synchronized using request list spinlock.
+ * This function pops the head item from the requests list while holding
+ * the list's spinlock.
  * --
  */
-VOID
-OvsTunnelFilterRequestPopList(POVS_TUNFLT_REQUEST_LIST listRequests,
-  PLIST_ENTRY head,
-  UINT32 *count)
+POVS_TUNFLT_REQUEST
+OvsTunnelFilterRequestPop(POVS_TUNFLT_REQUEST_LIST listRequests)
 {
+POVS_TUNFLT_REQUEST request = NULL;
+
 NdisAcquireSpinLock(&listRequests->spinlock);
 
 if (!IsListEmpty(&listRequests->head)) {
-PLIST_ENTRY PrevEntry;
-PLIST_ENTRY NextEntry;
-
-NextEntry = listRequests->head.Flink;
-PrevEntry = listRequests->head.Blink;
-
-head->Flink = NextEntry;
-NextEntry->Blink = head;
-
-head->Blink = PrevEntry;
-PrevEntry->Flink = head;
-
-*count = listRequests->numEntries;
-
-InitializeListHead(&listRequests->head);
-listRequests->numEntries = 0;
+request = (POVS_TUNFLT_REQUEST)RemoveHeadList(&listRequests->head);
+InitializeListHead(&request->entry);
+listRequests->numEntries--;
 }
 
 NdisReleaseSpinLock(&listRequests->spinlock);
+
+return request;
 }
 
 /*
@@ -1052,16 +1040,11 @@ VOID
 OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT threadCtx)
 {
 POVS_TUNFLT_REQUEST request = NULL;
-PLIST_ENTRY link = NULL;
-PLIST_ENTRY next = NULL;
-LIST_ENTRY  head;
 NTSTATUSstatus = STATUS_SUCCESS;
-UINT32  count = 0;
 BOOLEAN inTransaction = FALSE;
-BOOLEAN error = TRUE;
+BOOLEAN error = FALSE;
 
-do
-{
+do {
 if (!InterlockedCompareExchange(
 (LONG volatile *)&threadCtx->listRequests.numEntries, 0, 0)) {
 OVS_LOG_INFO("Nothing to do... request list is empty.");
@@ -1076,27 +1059,23 @@ 
OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT threadCtx)
 }
 inTransaction = TRUE;
 
-InitializeListHead(&head);
-OvsTunnelFilterRequestPopList(&threadCtx->listRequests, &head, &count);
-
-LIST_FORALL_SAFE(&head, link, next) {
-request = CONTAINING_RECORD(link, OVS_TUNFLT_REQUEST, entry);
+while (NULL != (request = OvsTunnelFilterRequestPop(
+&threadCtx->listRequests))) {
 
 status = OvsTunnelFilterExecuteAction(threadCtx->engineSession,
   request);
+
+/* Complete the IRP last operation status. */
+OvsTunnelFilterCompleteRequest(request->irp,
+   request->callback,
+   request->context,
+   status);
+OvsFreeMemory(request);
+request = NULL;
+
 if (!NT_SUCCESS(status)) {
-RemoveEntryList(&request->entry);
-count--;
-
-/* Complete the IRP with the failure status. */
-OvsTunnelFilterCompleteRequest(request->irp,
-   request->callback,
-   request->context,
-   status);
-OvsFreeMemory(request);
-request = NULL;
-} else {
-error = FALSE;
+error = TRUE;
+break;
 }
 }
 
@@ -1107,7 +1086,7 @@ 
OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT threadCtx)
 }
 
 status = FwpmTransactionCommit(threadCtx->engineSession);
-if (!NT_SUCCESS(status)){
+if (!NT_SUCCESS(status)) {
 OVS_LOG_ERROR("Failed to commit transaction, status: %x.",
   status);
 break;
@@ -1121,20 +1100,6 @@ 
OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT threadCtx)
 OVS_LOG_ERROR("Failed to execute req

[ovs-dev] [PATCH 0/2] datapath-windows: IRP cancelling mechanism

[Sorin Vinturis]

This patch series adds support for IRP cancelling mechanism to the OVS 
extension.

Sorin Vinturis (2):
  [PATCH 1/2] datapath-windows: Process tunnel filter requests iteratively
  [PATCH 2/2] datapath-windows: Support for IRP cancelling mechanism
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 2/2] datapath-windows: Support for IRP cancelling mechanism

[Sorin Vinturis]

Under certain circumstances, we might need to cancel a pending IRP
that has been submitted and not yet responded. This might occur when
the request takes too long to complete or when the process which
initiated the request terminated, leaving the request outstanding.

This patch adds this missing piece by providing support for IRP
cancelling mechanism.

Signed-off-by: Sorin Vinturis 
Reported-by: Sorin Vinturis 
Reported-at: https://github.com/openvswitch/ovs-issues/issues/95
---
 datapath-windows/ovsext/Datapath.c |   4 ++
 datapath-windows/ovsext/TunnelFilter.c | 110 +++--
 datapath-windows/ovsext/TunnelIntf.h   |   3 +
 3 files changed, 112 insertions(+), 5 deletions(-)

diff --git a/datapath-windows/ovsext/Datapath.c 
b/datapath-windows/ovsext/Datapath.c
index 4af909c..0ec33bd 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -922,6 +922,10 @@ exit:
  * to be processed later, so we mark the IRP as pending and complete
  * it in another thread when the request is processed. */
 IoMarkIrpPending(irp);
+
+/* Set the Cancel routine for the pending IRP. */
+IoSetCancelRoutine(irp, OvsTunnelFilterCancelIrp);
+
 return status;
 }
 return OvsCompleteIrpRequest(irp, (ULONG_PTR)replyLen, status);
diff --git a/datapath-windows/ovsext/TunnelFilter.c 
b/datapath-windows/ovsext/TunnelFilter.c
index caaf3f6..2542f63 100644
--- a/datapath-windows/ovsext/TunnelFilter.c
+++ b/datapath-windows/ovsext/TunnelFilter.c
@@ -168,6 +168,8 @@ static VOID 
OvsTunnelFilterThreadStop(POVS_TUNFLT_THREAD_CONTEXT threadCtx,
   BOOLEAN signalEvent);
 static NTSTATUS OvsTunnelFilterThreadInit(POVS_TUNFLT_THREAD_CONTEXT 
threadCtx);
 static VOID OvsTunnelFilterThreadUninit(POVS_TUNFLT_THREAD_CONTEXT 
threadCtx);
+static VOID OvsTunnelFilterSetIrpContext(POVS_TUNFLT_REQUEST_LIST 
listRequests,
+ POVS_TUNFLT_REQUEST request);
 
 /*
  * Callout driver global variables
@@ -983,6 +985,7 @@ VOID
 OvsTunnelFilterRequestPush(POVS_TUNFLT_REQUEST_LIST listRequests,
POVS_TUNFLT_REQUEST request)
 {
+
 NdisAcquireSpinLock(&listRequests->spinlock);
 
 InsertTailList(&listRequests->head, &(request->entry));
@@ -1012,6 +1015,10 @@ OvsTunnelFilterThreadPush(POVS_TUNFLT_REQUEST request)
 &gTunnelThreadCtx[threadIndex].listRequests,
 request);
 
+OvsTunnelFilterSetIrpContext(
+&gTunnelThreadCtx[threadIndex].listRequests,
+request);
+
 KeSetEvent(&gTunnelThreadCtx[threadIndex].requestEvent,
IO_NO_INCREMENT,
FALSE);
@@ -1062,6 +1069,20 @@ 
OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT threadCtx)
 while (NULL != (request = OvsTunnelFilterRequestPop(
 &threadCtx->listRequests))) {
 
+if (request->irp) {
+if (!IoSetCancelRoutine(request->irp, NULL)) {
+/*
+ * The Cancel routine for the current IRP is running.
+ * Leave the IRP alone and go to the next one.
+ */
+continue;
+}
+}
+
+/*
+ * The Cancel routine cannot run now and cannot already have
+ * started to run.
+ */
 status = OvsTunnelFilterExecuteAction(threadCtx->engineSession,
   request);
 
@@ -1321,7 +1342,7 @@ OvsTunnelFilterThreadUninit(POVS_TUNFLT_THREAD_CONTEXT 
threadCtx)
  * --
  * This function creates a new tunnel filter request and push it to a thread
  * queue. If the thread stop event is signaled, the request is completed with
- * STATUS_CANCELLED without pushing it to any queue.
+ * STATUS_REQUEST_ABORTED without pushing it to any queue.
  * --
  */
 NTSTATUS
@@ -1347,7 +1368,7 @@ OvsTunnelFilterQueueRequest(PIRP irp,
   (LARGE_INTEGER *)&timeout)) {
 /* The stop event is signaled. Completed the IRP with
  * STATUS_CANCELLED. */
-status = STATUS_CANCELLED;
+status = STATUS_REQUEST_ABORTED;
 break;
 }
 
@@ -1396,8 +1417,8 @@ OvsTunnelFilterQueueRequest(PIRP irp,
 
 /*
  * --
- *  This function adds a new WFP filter for the received port and returns the
- *  ID of the created WFP filter.
+ * This function adds a new WFP filter for the received port and returns the
+ * ID of the created WFP filter.
  *
  *  Note:
  *  All necessary calls to the WFP filtering engine must be running at IRQL =
@@ -1436,7 +1457,7 @@ OvsTunelFilterCreate(PIRP irp,
 
 /*
  * ---

Re: [ovs-dev] [PATCH] ovn-nb: Add per-port IP addresses to routers.

[Gal Sagie]

The change looks good to me.

The question i have is how will public network be configured/connected to
the logical router in OVN?
(In neutron router can only be connected to one public network)
This layer also need to support SNAT/DNAT functionality between the public
network and the
virtual network connected to this router.

In order to implement this, at least in the API level, maybe we need to add
this information to the
Logical Router object, or be able to connect a special "Logical Switch"
like Russell's suggestion
in the provider networks talk, that can be attached to the router and
represent public networks

What do you guys have in mind?

*** Less important issue: 
There is an additional extension called extra_route, which means that
basically in OVN world
this topology can happen:
  VM1 -->  LS1 --> LR1 -->  LS  <-- LR2 <-- LS2 <-- VM2

And VM1 could find a route to VM2.
I am not sure if all the plugins support this, or if its really that use
full
*** end less important issue ***
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn-nb: Add per-port IP addresses to routers.

[Gal Sagie]

On Wed, Jul 15, 2015 at 1:08 AM, Ben Pfaff  wrote:

> A router needs one IP address per port, not just one for the whole router.
>
> Reported-by: Gal Sagie 
> Reported-at: http://openvswitch.org/pipermail/dev/2015-July/057242.html
> Signed-off-by: Ben Pfaff 
> ---
>  ovn/ovn-nb.ovsschema |  1 -
>  ovn/ovn-nb.xml   | 13 -
>  2 files changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
> index 8fe378a..40bba97 100644
> --- a/ovn/ovn-nb.ovsschema
> +++ b/ovn/ovn-nb.ovsschema
> @@ -65,7 +65,6 @@
> "refType": "weak"},
> "min": 0,
> "max": "unlimited"}},
> -"ip": {"type": "string"},
>  "default_gw": {"type": {"key": "string", "min": 0, "max":
> 1}},
>  "external_ids": {
>  "type": {"key": "string", "value": "string",
> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> index 266e6a4..59ec3d2 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -281,13 +281,6 @@
>table="Logical_Router_Port"/> or it will automatically be deleted.
>  
>
> -
> -  The logical router's own IP address.  The logical router uses this
> -  address for ICMP replies (e.g. network unreachable messages) and
> other
> -  traffic that it originates and responds to traffic destined to this
> -  address (e.g. ICMP echo requests).
> -
> -
>  
>IP address to use as default gateway, if any.
>  
> @@ -316,8 +309,10 @@
>  
>
>  
> -  The IP network and netmask of the network on the router port.  Used
> for
> -  routing.
> +  The IP address of the router and the netmask.  For example,
> +  192.168.0.1/24 indicates that the router's IP address
> is
> +  192.168.0.1 and that packets destined to 192.168.0.x
> should be
> +  routed to this port.
>  
>
>  
> --
> 2.1.3
>
>
Acked-by: Gal Sagie 


-- 
Best Regards ,

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

[ovs-dev] [PATCH] datapath-windows: Enable failure after restarting extension

[Sorin Vinturis]

If the extension was previously enabled and running, after issuing a
restart, stop+start, the extension fails to be enabled. This happens
because the extension's DeviceObject is not yet initialized before the
FilterAttach routine is called.

This patch addresses this issue.

Signed-off-by: Sorin Vinturis 
Reported-by: Sorin Vinturis 
Reported-at: https://github.com/openvswitch/ovs-issues/issues/96
---
This patch is for both master and 2.4 branch.
---
 datapath-windows/ovsext/TunnelFilter.c | 28 
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/datapath-windows/ovsext/TunnelFilter.c 
b/datapath-windows/ovsext/TunnelFilter.c
index 231750e..5466fe4 100644
--- a/datapath-windows/ovsext/TunnelFilter.c
+++ b/datapath-windows/ovsext/TunnelFilter.c
@@ -825,20 +825,24 @@ OvsInitTunnelFilter(PDRIVER_OBJECT driverObject, PVOID 
deviceObject)
 {
 NTSTATUS status = STATUS_SUCCESS;
 
-status = OvsSubscribeTunnelInitBfeStateChanges(driverObject, deviceObject);
-if (NT_SUCCESS(status)) {
-if (FWPM_SERVICE_RUNNING == FwpmBfeStateGet()) {
-status = OvsTunnelFilterInitialize(driverObject);
-if (!NT_SUCCESS(status)) {
-/* XXX: We need to decide what actions to take in case of
- * failure to initialize tunnel filter. */
-ASSERT(status == NDIS_STATUS_SUCCESS);
-OVS_LOG_ERROR(
-"Failed to initialize tunnel filter, status: %x.",
-status);
+if (deviceObject) {
+status = OvsSubscribeTunnelInitBfeStateChanges(driverObject, 
deviceObject);
+if (NT_SUCCESS(status)) {
+if (FWPM_SERVICE_RUNNING == FwpmBfeStateGet()) {
+status = OvsTunnelFilterInitialize(driverObject);
+if (!NT_SUCCESS(status)) {
+/* XXX: We need to decide what actions to take in case of
+ * failure to initialize tunnel filter. */
+ASSERT(status == NDIS_STATUS_SUCCESS);
+OVS_LOG_ERROR(
+"Failed to initialize tunnel filter, status: %x.",
+status);
+}
+OvsUnsubscribeTunnelInitBfeStateChanges();
 }
-OvsUnsubscribeTunnelInitBfeStateChanges();
 }
+} else {
+status = OvsTunnelFilterInitialize(driverObject);
 }
 
 return status;
-- 
1.9.0.msysgit.0
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC PATCH v2 7/7] dpif-netdev: Bug fix and performance improvement.

[Jarno Rajahalme]

> On Jul 14, 2015, at 22:38, Ben Pfaff  wrote:
> 
>> On Fri, Jul 10, 2015 at 10:35:25AM -0700, Jarno Rajahalme wrote:
>> MINIFLOW_FOR_EACH_IN_MAPS and NETDEV_FLOW_KEY_FOR_EACH_IN_MAPqS had a
>> bug where tunnel metadata values remaining after the processing of the
>> tnl_map was not advanced correctly.
> 
> Is this a bug introduced only by the previous commit then?

Yes it was,

  Jarno

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

Re: [ovs-dev] [PATCH] doc: Document proposed OVN Gateway HA design.

[Ben Pfaff]

On Thu, Jul 09, 2015 at 06:12:53PM -0700, Ethan Jackson wrote:
> High availability for gateways in network virtualization deployments
> is fairly difficult to get right.  There are a ton of options, most of
> which are too complicated or perform badly.  To help solve this
> problem, this patch proposes an HA design based on some of the lessons
> learned building similar systems.  The hope is that it can be used as
> a starting point for design discussions and an eventual
> implementation.
> 
> Signed-off-by: Ethan Jackson 

Thank you for writing this up!  

This had encoding "y", which made it challenging to apply ;-)

Can we put it in the ovn directory?

When a logical network contains a gateway, then both sides are part of
the logical network, and thus "logical space".  So while I agree with
the diagram at the very beginning that shows a gateway between an
external network and an OVN virtual network, I think it's a bit
misleading to say:

The OVN gateway is responsible for shuffling traffic between logical space
(governed by ovn-northd), and the legacy physical network.

since both sides of the gateway are in logical space.  I think it would
be more accurate to use some variant of "virtual" here, maybe:

The OVN gateway is responsible for shuffling traffic between VMs
(governed by ovn-northd), and the legacy physical network.

In the second paragraph, I am not sure why HA is critical to
performance:

An HA solution is both critical to the performance and manageability of the
system, and extremely difficult to get right.

The second paragraph of "Basic Architecture" starts:

Since the broader internet is managed outside of the OVN network
domain, all traffic between logical space and the WAN must travel
through this gateway.

Is that the reason?  The reasons that come to mind to me are different
(or maybe just more specific?).  First, the gateway is the machine that
has a connection to the external network of interest; it might be in a
remote location such as a branch office away from the bulk of the
hypervisors in an OVN deployment.  Second, supposing that in fact the
gateway isn't in that kind of remote location, we want to have a central
point of entry into the virtual part of an OVN network because otherwise
we don't know which of N hypervisors should bring the packet into the
virtual network.

Under "Naive active-backup", do you mean OpenFlow echo requests here
(a "hello" message is only sent at the very beginning of an OpenFlow
session, to negotiate the OpenFlow version):

ovn-northd monitors this
gateway via OpenFlow hello messages (or some equivalent),

Under "Controller Independent Active-backup", I am not sure that I buy
the argument here, because currently ovn-northd doesn't care about the
layout of the physical network.  The other argument rings true for me of
course:

This can significantly increase downtime in the event of a failover
as the (often already busy) ovn-northd controller has to recompute
state for the new leader.

Here are some spelling fixes as a patch.  This also replaces the fancy
Unicode U+2014 em dashes by the more common (in OVS, anyway) ASCII "--".

Thanks again for writing this!

diff --git a/OVN-GW-HA.md b/OVN-GW-HA.md
index ea598b2..e0d5c9f 100644
--- a/OVN-GW-HA.md
+++ b/OVN-GW-HA.md
@@ -30,8 +30,8 @@ The OVN gateway is responsible for shuffling traffic between 
logical space
 implementation, the gateway is a single x86 server, or hardware VTEP.  For most
 deployments, a single system has enough forwarding capacity to service the
 entire virtualized network, however, it introduces a single point of failure.
-If this system dies, the entire OVN deployment becomes unavailable.  To mitgate
-this risk, an HA solution is critical — by spreading responsibilty across
+If this system dies, the entire OVN deployment becomes unavailable.  To 
mitigate
+this risk, an HA solution is critical -- by spreading responsibility across
 multiple systems, no single server failure can take down the network.
 
 An HA solution is both critical to the performance and manageability of the
@@ -51,7 +51,7 @@ OVN controlled tunnel traffic, to raw physical network 
traffic.
 
 Since the broader internet is managed outside of the OVN network domain, all
 traffic between logical space and the WAN must travel through this gateway.
-This makes it a critical single point of failure — if the gateway dies,
+This makes it a critical single point of failure -- if the gateway dies,
 communication with the WAN ceases for all systems in logical space.
 
 To mitigate this risk, multiple gateways should be run in a "High Availability
@@ -128,15 +128,15 @@ absolute simplest way to achive this is what we'll call 
"naive-active-backup".
 Naive Active Backup HA Implementation
 ```
 
-In a naive active-bakup, one of the Gateways is choosen (arbitrarily) as a
+In a naive active-backup, one of the Gateways is choosen (arbitrarily) as a
 leader.  All logical route

Re: [ovs-dev] [RFC PATCH 0/6] Increase miniflow's capacity.

[Traynor, Kevin]

> From: Jarno Rajahalme [mailto:jrajaha...@nicira.com]
> Sent: Friday, July 10, 2015 6:38 PM
> To: Traynor, Kevin
> Cc: dev@openvswitch.org
> Subject: Re: [ovs-dev] [RFC PATCH 0/6] Increase miniflow's capacity.
> 
> 
> On Jul 10, 2015, at 9:38 AM, Traynor, Kevin  wrote:
> 
> 
> -Original Message-
> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Jarno Rajahalme
> Sent: Thursday, July 9, 2015 6:16 PM
> To: dev@openvswitch.org
> Subject: [ovs-dev] [RFC PATCH 0/6] Increase miniflow's capacity.
> 
> Upto now struct miniflow has been limited to 63 64-bit units.  This
> series increases this capacity to 128 64-bit units.  For presimed
> performance reasons the new miniflow uses one 64-bit map for tunnel
> metadata and another for the rest of the metadata and the fields
> extracted from packet headers.
> 
> Before making miniflow more complex, this series simplifies it a bit
> by always inlining the miniflow data and cleaning up the interface a
> bit.
> 
> All performance testing is yet to be done.  I would be thankful if
> someone verifies the performance impact on the DPDK datapath, if any.
> 
> Hi Jarno, I can run some tests on this early next week when I get access
> to a board.
> 
> 
> Kevin,
> 
> I just sent a v2 based on feedback from Daniele, so please test on that
> instead!
> 
>   Jarno

I've tested this multiple times for phy-phy with dpdk and a bi-directional flow.
It's showing an avg. of 16.39 mpps on head of master and 16.67 mpps with your
changes - so no need to be worried! 

I ran a few tests on the other dpdk ports and there was a slight drop of 100K 
pps
for vhost, but I think this is just test variance.

> 
> 
> 
> 
> Jarno Rajahalme (6):
>  tests: Check for core files before exiting.
>  meta-flow: Add a missing break statement.
>  lib: Always inline miniflows.
>  match: Single malloc minimatch.
>  flow: Eliminate miniflow_clone() and minimask_clone().
>  flow: Split miniflow's map.
> 
> lib/classifier-private.h |  122 
> lib/classifier.c |  149 --
> lib/dpif-netdev.c|  103 +-
> lib/flow.c   |  482 ++--
> --
> lib/flow.h   |  252 +---
> lib/match.c  |   39 ++--
> lib/match.h  |   12 +-
> lib/meta-flow.c  |2 +-
> lib/tnl-ports.c  |4 +-
> ofproto/ofproto.c|   10 +-
> tests/ofproto-macros.at  |6 +-
> tests/test-classifier.c  |  130 +++--
> 12 files changed, 716 insertions(+), 595 deletions(-)
> 
> --
> 1.7.10.4
> 
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev




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

Re: [ovs-dev] [PATCH] datapath-windows: Solved BSOD when cleaning up the VXLAN tunnel

[Ben Pfaff]

On Fri, Jul 10, 2015 at 08:26:51AM +, Sorin Vinturis wrote:
> When removing vport also remove the vxlan tunnel port.
> 
> Signed-off-by: Sorin Vinturis 
> Reported-by: Sorin Vinturis 
> Reported-at: https://github.com/openvswitch/ovs-issues/issues/94

Thanks, applied to master and branch-2.4.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath-windows: Fixed spelling errors in OVS

[Ben Pfaff]

Same here.

On Mon, Jul 13, 2015 at 11:44:28AM +, Alin Serdean wrote:
> Just one small nit think you need to rebase. Could not apply the patch on 
> master.
> 
> Alin.
> 
> -Mesaj original-
> De la: dev [mailto:dev-boun...@openvswitch.org] În numele Alin Serdean
> Trimis: Monday, July 13, 2015 2:19 PM
> Către: Sorin Vinturis; dev@openvswitch.org
> Subiect: Re: [ovs-dev] [PATCH] datapath-windows: Fixed spelling errors in OVS
> 
> Acked-by: Alin Gabriel Serdean 
> 
> Alin.
> 
> -Mesaj original-
> De la: dev [mailto:dev-boun...@openvswitch.org] În numele Sorin Vinturis
> Trimis: Friday, July 10, 2015 11:48 AM
> Către: dev@openvswitch.org
> Subiect: [ovs-dev] [PATCH] datapath-windows: Fixed spelling errors in OVS
> 
> Solved some spelling errors observed in the datapath code.
> 
> Signed-off-by: Sorin Vinturis 
> ___
> 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] datapath-windows: Fixed spelling errors in OVS

[Sorin Vinturis]

Hi Ben,

I sent a rebased v2 patch.

Thanks,
Sorin

-Original Message-
From: Ben Pfaff [mailto:b...@nicira.com] 
Sent: Wednesday, 15 July, 2015 18:55
To: Alin Serdean
Cc: Sorin Vinturis; dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows: Fixed spelling errors in OVS

Same here.

On Mon, Jul 13, 2015 at 11:44:28AM +, Alin Serdean wrote:
> Just one small nit think you need to rebase. Could not apply the patch on 
> master.
> 
> Alin.
> 
> -Mesaj original-
> De la: dev [mailto:dev-boun...@openvswitch.org] În numele Alin Serdean
> Trimis: Monday, July 13, 2015 2:19 PM
> Către: Sorin Vinturis; dev@openvswitch.org
> Subiect: Re: [ovs-dev] [PATCH] datapath-windows: Fixed spelling errors in OVS
> 
> Acked-by: Alin Gabriel Serdean 
> 
> Alin.
> 
> -Mesaj original-
> De la: dev [mailto:dev-boun...@openvswitch.org] În numele Sorin Vinturis
> Trimis: Friday, July 10, 2015 11:48 AM
> Către: dev@openvswitch.org
> Subiect: [ovs-dev] [PATCH] datapath-windows: Fixed spelling errors in OVS
> 
> Solved some spelling errors observed in the datapath code.
> 
> Signed-off-by: Sorin Vinturis 
> ___
> 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 v2] ofproto: Implement OF1.4 Set/Get asynchronous configuration messages.

[Ben Pfaff]

On Fri, Jul 10, 2015 at 05:42:50PM +0530, niti1...@gmail.com wrote:
> From: Niti 
> 
> This patch adds support for Openflow1.4 set/get asynchronous configuration
> messages. OpenVSwitch already supports set/get asynchronous configuration
> messages for Openflow1.3. In this patch OFPT_SET_ASYNC_CONFIG message
> allows the controllers to set the configuration for OFPT_ROLE_STATUS,
> OFPT_TABLE_STATUS and OFPT_REQUESTFORWARD in addition to the Openflow1.3
> messages. In a OFPT_SET_ASYNC, only the properties that shall be changed
> need to be included, properties that are omitted from the message are
> unchanged.
> 
> The OFPT_GET_ASYNC_CONFIG is used to query the asynchronous configuration
> of switch. In a OFPT_GET_ASYNC_REPLY message, all properties must be
> included.
> 
> According to Openflow1.4 the initial configuration shall be:
> 
>- In the “master” or “equal” role, enable all OFPT_PACKET_IN messages,
>  except those with reason OFPR_INVALID_TTL, enable all OFPT_PORT_STATUS
>  and OFPT_FLOW_REMOVED messages, and disable all OFPT_ROLE_STATUS,
>  OFPT_TABLE_STATUS and OFPT_REQUESTFORWARD messages.
> 
>- In the “slave” role, enable all OFPT_PORT_STATUS messages and disable
>  all OFPT_PACKET_IN, OFPT_FLOW_REMOVED, OFPT_ROLE_STATUS,
>  OFPT_TABLE_STATUS and OFPT_REQUESTFORWARD messages.
> 
> Signed-off-by: Niti Rohilla 

Thanks for the revision!

This commit causes many failures in the testsuite.  Please run "make
check" and figure out the problems.

This code combines decoding "set async config" and "get async config
reply" into a single function.  That's mostly OK--although it should be
mentioned in comments--but there is an important distinction that I
believe it misses.  For decoding "set async config", it is important to
return an error when an unknown attribute type is present, because the
switch can't implement an unknown type.  But for decoding "get async
config reply", there is no such problem (it just means that the switch
supports some feature that the controller doesn't understand, which is
not a problem) so the unknown attribute type should be ignored.  Please
implement this distinction.

In NEWS, s/asynchoronous/asynchronous/.

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

Re: [ovs-dev] [PATCH] datapath-windows: Fixed spelling errors in OVS

[Sorin Vinturis]

Rebased v2 patch: http://openvswitch.org/pipermail/dev/2015-July/057480.html


-Original Message-
From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Sorin Vinturis
Sent: Wednesday, 15 July, 2015 19:06
To: Ben Pfaff; Alin Serdean
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows: Fixed spelling errors in OVS

Hi Ben,

I sent a rebased v2 patch.

Thanks,
Sorin

-Original Message-
From: Ben Pfaff [mailto:b...@nicira.com] 
Sent: Wednesday, 15 July, 2015 18:55
To: Alin Serdean
Cc: Sorin Vinturis; dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows: Fixed spelling errors in OVS

Same here.

On Mon, Jul 13, 2015 at 11:44:28AM +, Alin Serdean wrote:
> Just one small nit think you need to rebase. Could not apply the patch on 
> master.
> 
> Alin.
> 
> -Mesaj original-
> De la: dev [mailto:dev-boun...@openvswitch.org] În numele Alin Serdean
> Trimis: Monday, July 13, 2015 2:19 PM
> Către: Sorin Vinturis; dev@openvswitch.org
> Subiect: Re: [ovs-dev] [PATCH] datapath-windows: Fixed spelling errors in OVS
> 
> Acked-by: Alin Gabriel Serdean 
> 
> Alin.
> 
> -Mesaj original-
> De la: dev [mailto:dev-boun...@openvswitch.org] În numele Sorin Vinturis
> Trimis: Friday, July 10, 2015 11:48 AM
> Către: dev@openvswitch.org
> Subiect: [ovs-dev] [PATCH] datapath-windows: Fixed spelling errors in OVS
> 
> Solved some spelling errors observed in the datapath code.
> 
> Signed-off-by: Sorin Vinturis 
> ___
> 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] [OVN] Add QoS to NB schema

[Ben Pfaff]

On Sun, Jul 12, 2015 at 09:21:41AM +0300, Gal Sagie wrote:
> Add QoS profile and QoS rule tables and explanations
> to the NB schema.
> Add specific bandwidth limit rule configurations
> in the document
> 
> Signed-off-by: Gal Sagie 

This is a lot of schema to support just a little bit of functionality!
I guess that must be because you foresee support for much more
sophisticated QoS profiles in the future.  Can you tell us a little
about those?  Otherwise it would make more sense to me to do this in a
simpler way.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] datapath-windows: Update VXLAN header information

[Ben Pfaff]

On Mon, Jul 13, 2015 at 04:59:26PM +, Alin Serdean wrote:
> Use tunnel key information on the IP header preceding the VXLAN header.
> 
> Signed-off-by: Alin Gabriel Serdean 
> ---
> v2: use separate variable to store the number of transmitted bytes

Thanks, I applied this to master.  Should I also apply it to branch-2.4?

Thanks,

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

Re: [ovs-dev] [PATCH] datapath-windows: update extension information

[Ben Pfaff]

On Mon, Jul 13, 2015 at 05:18:20PM +, Alin Serdean wrote:
> This patch sets additional information about the driver used by various
> applications.
> 
> Signed-off-by: Alin Gabriel Serdean 

Thanks!  I applied this to master.  Should I also apply it to
branch-2.4?

Thanks,

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

Re: [ovs-dev] [ovn-controller-vtep V3 1/6] ofproto-macros.at: Make check_logs() check all log files.

[Ben Pfaff]

On Mon, Jul 13, 2015 at 12:04:11PM -0700, Alex Wang wrote:
> As we have more daemons with OVN that can be tested using ovs autotest
> framework, it is convenient to extend check_logs() to check all log files
> (except the testsuite.log).
> 
> Signed-off-by: Alex Wang 
> 
> ---
> V2->V3:
> - adopt Ben's suggestion to check for all '*.log's except testsuite.log.
> - rebase to master.
> 
> PATCH->V2:
> - make check_logs() check all daemons' log files.

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

Re: [ovs-dev] [PATCH v2] datapath-windows: Update VXLAN header information

[Alin Serdean]

Sorry forgot to mention in the commit notes, yes please apply it on 2.4 as well.

Alin

-Mesaj original-
De la: Ben Pfaff [mailto:b...@nicira.com] 
Trimis: Wednesday, July 15, 2015 8:42 PM
Către: Alin Serdean
Cc: dev@openvswitch.org
Subiect: Re: [ovs-dev] [PATCH v2] datapath-windows: Update VXLAN header 
information

On Mon, Jul 13, 2015 at 04:59:26PM +, Alin Serdean wrote:
> Use tunnel key information on the IP header preceding the VXLAN header.
> 
> Signed-off-by: Alin Gabriel Serdean 
> ---
> v2: use separate variable to store the number of transmitted bytes

Thanks, I applied this to master.  Should I also apply it to branch-2.4?

Thanks,

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

Re: [ovs-dev] [PATCH] datapath-windows: update extension information

[Alin Serdean]

Sorry forgot to mention in the commit notes, yes please apply it on 2.4 as well.

Alin.

-Mesaj original-
De la: Ben Pfaff [mailto:b...@nicira.com] 
Trimis: Wednesday, July 15, 2015 8:46 PM
Către: Alin Serdean
Cc: dev@openvswitch.org
Subiect: Re: [ovs-dev] [PATCH] datapath-windows: update extension information

On Mon, Jul 13, 2015 at 05:18:20PM +, Alin Serdean wrote:
> This patch sets additional information about the driver used by 
> various applications.
> 
> Signed-off-by: Alin Gabriel Serdean 

Thanks!  I applied this to master.  Should I also apply it to branch-2.4?

Thanks,

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

Re: [ovs-dev] [ovn-controller-vtep V3 2/7] ovn-sb: Remove the "Gateway" table from the ovn-sb schema.

[Russell Bryant]

On 07/13/2015 11:22 PM, Alex Wang wrote:
> In a gateway like the VTEP L2 gateway, physical vlans belonging to
> the same logical network form a "logical switch".  Each logical switch
> has a dedicated tunnel key and will keep records of all MACs learned
> from the owned vlans.  So user can just send packet to a "logical
> switch" and the gateway will figure out the output port and vlan tag
> automatically.
> 
> Therefore, it is not really necessary to keep record of the vlan map
> for each gateway physical port in the OVN_Southbound database using
> "gateway_ports".
> 
> Thusly, this commit removes the "Gateway" table from the OVN_Southbound
> database.  In the "Chassis" table, the "gateway_ports" column is replaced
> by "logical_switches" column which maps the logical switch name in the
> gateway to a logical port name that exists in the OVN_Northbound database's
> "Logical_Port" table.
> 
> Signed-off-by: Alex Wang 

I think this makes sense to me.  I wonder if calling it
"logical_switches" could be confusing with the logical switch in OVN
Northbound.  Maybe "vtep_logical_switches" would clarify?

To make sure I understand correctly ...

1) ovn-controller-vtep will register itself in the Chassis table.  It
will populate the Chassis "logical_switches" with a list of the VTEP
switch's logical switches and associated auto-generated logical port names.

2) A management system will update OVN_Northbound with a logical port
with a special name that indicates that it's associated with a given
VTEP logical switch.

3) ovn-northd creates a Binding for this logical port just like a normal
port and assigns it a tunnel key.

4) ovn-controller-vtep will see this Binding and assign this tunnel key
to the logical switch via the VTEP schema.

Is this how the tunnel key is decided on to reach a given vtep logical
switch?  If so, where is #4 done?  or did I misunderstand the flow?

Also, what do you think about my proposal for using "type" and "options"
fields on logical ports in OVN_Northbound instead of a special port
name?  If we went this route, you don't need "logical_switches" to be a
smap, it can just be a list of logical switch names.  Creating a
connection in OVN northbound would be something like:

  Logical_Port
name: 
type: vtep
options:
   vtep_physical_switch: <...>
   vtep_logical_switch: <...>

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

[ovs-dev] [PATCH] datapath-windows: Fix broken break by distributing resource.h.

[Ben Pfaff]

Commit e19a0c6 (datapath-windows: update extension information) added
resource.h but did not distribute it.

Signed-off-by: Ben Pfaff 
---
I already applied this to master to fix the broken build.

 datapath-windows/automake.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/automake.mk b/datapath-windows/automake.mk
index a4f5a57..ed48c69 100644
--- a/datapath-windows/automake.mk
+++ b/datapath-windows/automake.mk
@@ -64,4 +64,5 @@ EXTRA_DIST += \
datapath-windows/ovsext/ovsext.vcxproj \
datapath-windows/ovsext/ovsext.vcxproj.user \
datapath-windows/ovsext/precomp.h \
-   datapath-windows/ovsext/precompsrc.c
+   datapath-windows/ovsext/precompsrc.c \
+   datapath-windows/ovsext/resource.h
-- 
2.1.3

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

[ovs-dev] [PATCH] Fix build on linux.

[Russell Bryant]

A recent commit added a new file but didn't add it to EXTRA_DIST.

Signed-off-by: Russell Bryant 
CC: Ben Pfaff 
CC: Alin Gabriel Serdean 
---
 datapath-windows/automake.mk | 1 +
 1 file changed, 1 insertion(+)

diff --git a/datapath-windows/automake.mk b/datapath-windows/automake.mk
index a4f5a57..b55d721 100644
--- a/datapath-windows/automake.mk
+++ b/datapath-windows/automake.mk
@@ -42,6 +42,7 @@ EXTRA_DIST += \
datapath-windows/ovsext/PacketIO.h \
datapath-windows/ovsext/PacketParser.c \
datapath-windows/ovsext/PacketParser.h \
+   datapath-windows/ovsext/resource.h \
datapath-windows/ovsext/Switch.c \
datapath-windows/ovsext/Switch.h \
datapath-windows/ovsext/Tunnel.c \
-- 
2.4.3

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

Re: [ovs-dev] [PATCH] datapath-windows: Fix broken break by distributing resource.h.

[Russell Bryant]

On 07/15/2015 02:28 PM, Ben Pfaff wrote:
> Commit e19a0c6 (datapath-windows: update extension information) added
> resource.h but did not distribute it.
> 
> Signed-off-by: Ben Pfaff 
> ---
> I already applied this to master to fix the broken build.
> 
>  datapath-windows/automake.mk | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/datapath-windows/automake.mk b/datapath-windows/automake.mk
> index a4f5a57..ed48c69 100644
> --- a/datapath-windows/automake.mk
> +++ b/datapath-windows/automake.mk
> @@ -64,4 +64,5 @@ EXTRA_DIST += \
>   datapath-windows/ovsext/ovsext.vcxproj \
>   datapath-windows/ovsext/ovsext.vcxproj.user \
>   datapath-windows/ovsext/precomp.h \
> - datapath-windows/ovsext/precompsrc.c
> + datapath-windows/ovsext/precompsrc.c \
> + datapath-windows/ovsext/resource.h
> 

Thanks!  Ignore my duplicate patch.  :-)

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

Re: [ovs-dev] [PATCH 1/2] flow: Factor out flag parsing and formatting routines.

[Ben Pfaff]

On Mon, Jul 13, 2015 at 02:53:56PM -0700, Jesse Gross wrote:
> There are several implementations of functions that parse/format
> flags and their binary representation. This factors them out into
> common routines. In addition to reducing code, it also makes things
> more consistent across different parts of OVS.
> 
> Signed-off-by: Jesse Gross 

Thanks, I like reducing code size!

This patch deletes a test and part of a test in ovs-ofctl.at.  Why?

In parse_flags(), s/preceed/preceded/:
*res_string = xasprintf("%s: %s must be preceed by '+' "

parse_flags() could use a function-level comment.

format_flags_masked() will output the empty string if flags == 0 && mask
== 0, and I think that parse_flags() will accept that too, but I don't
know if it's the most user-friendly form.  Don't know if that matters.

format_flags_masked() will output "0" if flags == 0 && mask == max_mask,
but parse_flags() won't accept "0".  It would be nice to resolve that.

Thanks,

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

Re: [ovs-dev] [PATCH 2/2] tunneling: Allow matching and setting tunnel 'OAM' flag.

[Ben Pfaff]

On Mon, Jul 13, 2015 at 02:53:57PM -0700, Jesse Gross wrote:
> Several encapsulation formats have the concept of an 'OAM' bit
> which typically is used with networking tracing tools to
> distinguish test packets from real traffic. OVS already internally
> has support for this, however, it doesn't do anything with it
> and it also isn't exposed for controllers to use. This enables
> support through OpenFlow.
> 
> There are several other tunnel flags which are consumed internally
> by OVS. It's not clear that it makes sense to use them externally
> so this does not expose those flags - although it should be easy
> to do so if necessary in the future.
> 
> Signed-off-by: Jesse Gross 

"git am" doesn't want to apply this for me:
fatal: patch fragment without header at line 439: @@ -1518,6 +1518,7 @@ 
OVS_VSWITCHD_START

I don't see what it's complaining about, though; maybe it's the
extremely long lines.  Either way, though, I can't apply it.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath-windows: Fix broken break by distributing resource.h.

[Jarno Rajahalme]

Me too :-)

Acked-by: Jarno Rajahalme 

> On Jul 15, 2015, at 11:32 AM, Russell Bryant  wrote:
> 
> On 07/15/2015 02:28 PM, Ben Pfaff wrote:
>> Commit e19a0c6 (datapath-windows: update extension information) added
>> resource.h but did not distribute it.
>> 
>> Signed-off-by: Ben Pfaff 
>> ---
>> I already applied this to master to fix the broken build.
>> 
>> datapath-windows/automake.mk | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/datapath-windows/automake.mk b/datapath-windows/automake.mk
>> index a4f5a57..ed48c69 100644
>> --- a/datapath-windows/automake.mk
>> +++ b/datapath-windows/automake.mk
>> @@ -64,4 +64,5 @@ EXTRA_DIST += \
>>  datapath-windows/ovsext/ovsext.vcxproj \
>>  datapath-windows/ovsext/ovsext.vcxproj.user \
>>  datapath-windows/ovsext/precomp.h \
>> -datapath-windows/ovsext/precompsrc.c
>> +datapath-windows/ovsext/precompsrc.c \
>> +datapath-windows/ovsext/resource.h
>> 
> 
> Thanks!  Ignore my duplicate patch.  :-)
> 
> -- 
> Russell Bryant
> ___
> 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] Fix build on linux.

[Ben Pfaff]

On Wed, Jul 15, 2015 at 02:30:57PM -0400, Russell Bryant wrote:
> A recent commit added a new file but didn't add it to EXTRA_DIST.
> 
> Signed-off-by: Russell Bryant 
> CC: Ben Pfaff 
> CC: Alin Gabriel Serdean 

Already fixed, sorry about that.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] tunneling: Allow matching and setting tunnel 'OAM' flag.

[Jesse Gross]

On Wed, Jul 15, 2015 at 11:38 AM, Ben Pfaff  wrote:
> On Mon, Jul 13, 2015 at 02:53:57PM -0700, Jesse Gross wrote:
>> Several encapsulation formats have the concept of an 'OAM' bit
>> which typically is used with networking tracing tools to
>> distinguish test packets from real traffic. OVS already internally
>> has support for this, however, it doesn't do anything with it
>> and it also isn't exposed for controllers to use. This enables
>> support through OpenFlow.
>>
>> There are several other tunnel flags which are consumed internally
>> by OVS. It's not clear that it makes sense to use them externally
>> so this does not expose those flags - although it should be easy
>> to do so if necessary in the future.
>>
>> Signed-off-by: Jesse Gross 
>
> "git am" doesn't want to apply this for me:
> fatal: patch fragment without header at line 439: @@ -1518,6 +1518,7 
> @@ OVS_VSWITCHD_START
>
> I don't see what it's complaining about, though; maybe it's the
> extremely long lines.  Either way, though, I can't apply it.

Hmm, that's weird, I was able to apply it back locally on my system.
In any case, I push this series to Github as an alternative:
https://github.com/jessegross/ovs.git oam

It's the same as the original version that I sent out, just rebased to
master. I haven't addressed your comments on the first patch yet.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] tunneling: Allow matching and setting tunnel 'OAM' flag.

[Ben Pfaff]

On Wed, Jul 15, 2015 at 12:04:54PM -0700, Jesse Gross wrote:
> On Wed, Jul 15, 2015 at 11:38 AM, Ben Pfaff  wrote:
> > On Mon, Jul 13, 2015 at 02:53:57PM -0700, Jesse Gross wrote:
> >> Several encapsulation formats have the concept of an 'OAM' bit
> >> which typically is used with networking tracing tools to
> >> distinguish test packets from real traffic. OVS already internally
> >> has support for this, however, it doesn't do anything with it
> >> and it also isn't exposed for controllers to use. This enables
> >> support through OpenFlow.
> >>
> >> There are several other tunnel flags which are consumed internally
> >> by OVS. It's not clear that it makes sense to use them externally
> >> so this does not expose those flags - although it should be easy
> >> to do so if necessary in the future.
> >>
> >> Signed-off-by: Jesse Gross 
> >
> > "git am" doesn't want to apply this for me:
> > fatal: patch fragment without header at line 439: @@ -1518,6 
> > +1518,7 @@ OVS_VSWITCHD_START
> >
> > I don't see what it's complaining about, though; maybe it's the
> > extremely long lines.  Either way, though, I can't apply it.
> 
> Hmm, that's weird, I was able to apply it back locally on my system.
> In any case, I push this series to Github as an alternative:
> https://github.com/jessegross/ovs.git oam
> 
> It's the same as the original version that I sent out, just rebased to
> master. I haven't addressed your comments on the first patch yet.

Thanks, got it.  I'll review patch 2 now.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [ovn-controller-vtep V3 2/7] ovn-sb: Remove the "Gateway" table from the ovn-sb schema.

[Alex Wang]

On Wed, Jul 15, 2015 at 11:22 AM, Russell Bryant  wrote:

> On 07/13/2015 11:22 PM, Alex Wang wrote:
> > In a gateway like the VTEP L2 gateway, physical vlans belonging to
> > the same logical network form a "logical switch".  Each logical switch
> > has a dedicated tunnel key and will keep records of all MACs learned
> > from the owned vlans.  So user can just send packet to a "logical
> > switch" and the gateway will figure out the output port and vlan tag
> > automatically.
> >
> > Therefore, it is not really necessary to keep record of the vlan map
> > for each gateway physical port in the OVN_Southbound database using
> > "gateway_ports".
> >
> > Thusly, this commit removes the "Gateway" table from the OVN_Southbound
> > database.  In the "Chassis" table, the "gateway_ports" column is replaced
> > by "logical_switches" column which maps the logical switch name in the
> > gateway to a logical port name that exists in the OVN_Northbound
> database's
> > "Logical_Port" table.
> >
> > Signed-off-by: Alex Wang 
>
> I think this makes sense to me.  I wonder if calling it
> "logical_switches" could be confusing with the logical switch in OVN
> Northbound.  Maybe "vtep_logical_switches" would clarify?
>
>

I'm not sure, if we may need the same map for non-vtep switch (e.g. dpdk
switch).  but I'm also fine with defining it more specific as suggested,



> To make sure I understand correctly ...
>
> 1) ovn-controller-vtep will register itself in the Chassis table.  It
> will populate the Chassis "logical_switches" with a list of the VTEP
> switch's logical switches and associated auto-generated logical port names.
>


Yes,



> 2) A management system will update OVN_Northbound with a logical port
> with a special name that indicates that it's associated with a given
> VTEP logical switch.
>
>
Yes


> 3) ovn-northd creates a Binding for this logical port just like a normal
> port and assigns it a tunnel key.
>
> 4) ovn-controller-vtep will see this Binding and assign this tunnel key
> to the logical switch via the VTEP schema.
>
> Is this how the tunnel key is decided on to reach a given vtep logical
> switch?  If so, where is #4 done?  or did I misunderstand the flow?
>


I think your understanding is correct,

And Ben's upcoming change will make northd assign a tunnel key per (ovn-
nb) logical switch.  And ovn-controller-vtep is responsible for updating the
tunnel key to the (vtep db) logical switch.



> Also, what do you think about my proposal for using "type" and "options"
> fields on logical ports in OVN_Northbound instead of a special port
> name?  If we went this route, you don't need "logical_switches" to be a
> smap, it can just be a list of logical switch names.  Creating a
> connection in OVN northbound would be something like:
>
>   Logical_Port
> name: 
> type: vtep
> options:
>vtep_physical_switch: <...>
>vtep_logical_switch: <...>
>
>

One thing I see missing is that: multiple vtep physical switches (supported
by
the same vtep db) could connect to the same vtep logical switch...   In
that
case, logical switch name can no longer uniquely identify a connection of
'a vtep physical switch to a vtep logical switch'...  That's why I use the
combination of pswitch_name+lswitch_name.

With you suggestion, first, the logical port name in ovn-nb can be
duplicated.
And if that is allowed, we need to pass the "options" down to ovn-sb
Binding table to uniquely binding the vtep lport to vtep chassis.


Thanks,
Alex Wang,



> --
> Russell Bryant
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [ovn-controller-vtep V3 2/7] ovn-sb: Remove the "Gateway" table from the ovn-sb schema.

[Russell Bryant]

On 07/15/2015 03:09 PM, Alex Wang wrote:
> Also, what do you think about my proposal for using "type" and "options"
> fields on logical ports in OVN_Northbound instead of a special port
> name?  If we went this route, you don't need "logical_switches" to be a
> smap, it can just be a list of logical switch names.  Creating a
> connection in OVN northbound would be something like:
> 
>   Logical_Port
> name: 
> type: vtep
> options:
>vtep_physical_switch: <...>
>vtep_logical_switch: <...>
> 
> 
> 
> One thing I see missing is that: multiple vtep physical switches 
> (supported by the same vtep db) could connect to the same vtep
> logical switch...   In that case, logical switch name can no longer
> uniquely identify a connection of 'a vtep physical switch to a vtep
> logical switch'...  That's why I use the combination of
> pswitch_name+lswitch_name.
> 
> With you suggestion, first, the logical port name in ovn-nb can be 
> duplicated. And if that is allowed, we need to pass the "options"
> down to ovn-sb Binding table to uniquely binding the vtep lport to
> vtep chassis.

Yes, I was thinking that this information would have to be added to the
Binding table in OVN_Southbound as well so that you still have the
pswitch+lswitch info, just in a little bit more structured way.

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

Re: [ovs-dev] [PATCH v2] datapath-windows: Update VXLAN header information

[Ben Pfaff]

Thanks, done.

On Wed, Jul 15, 2015 at 06:17:14PM +, Alin Serdean wrote:
> Sorry forgot to mention in the commit notes, yes please apply it on 2.4 as 
> well.
> 
> Alin
> 
> -Mesaj original-
> De la: Ben Pfaff [mailto:b...@nicira.com] 
> Trimis: Wednesday, July 15, 2015 8:42 PM
> Către: Alin Serdean
> Cc: dev@openvswitch.org
> Subiect: Re: [ovs-dev] [PATCH v2] datapath-windows: Update VXLAN header 
> information
> 
> On Mon, Jul 13, 2015 at 04:59:26PM +, Alin Serdean wrote:
> > Use tunnel key information on the IP header preceding the VXLAN header.
> > 
> > Signed-off-by: Alin Gabriel Serdean 
> > ---
> > v2: use separate variable to store the number of transmitted bytes
> 
> Thanks, I applied this to master.  Should I also apply it to branch-2.4?
> 
> Thanks,
> 
> Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] datapath-windows: update extension information

[Ben Pfaff]

Done, thanks!

On Wed, Jul 15, 2015 at 06:17:35PM +, Alin Serdean wrote:
> Sorry forgot to mention in the commit notes, yes please apply it on 2.4 as 
> well.
> 
> Alin.
> 
> -Mesaj original-
> De la: Ben Pfaff [mailto:b...@nicira.com] 
> Trimis: Wednesday, July 15, 2015 8:46 PM
> Către: Alin Serdean
> Cc: dev@openvswitch.org
> Subiect: Re: [ovs-dev] [PATCH] datapath-windows: update extension information
> 
> On Mon, Jul 13, 2015 at 05:18:20PM +, Alin Serdean wrote:
> > This patch sets additional information about the driver used by 
> > various applications.
> > 
> > Signed-off-by: Alin Gabriel Serdean 
> 
> Thanks!  I applied this to master.  Should I also apply it to branch-2.4?
> 
> Thanks,
> 
> Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] tunneling: Allow matching and setting tunnel 'OAM' flag.

[Ben Pfaff]

On Mon, Jul 13, 2015 at 02:53:57PM -0700, Jesse Gross wrote:
> Several encapsulation formats have the concept of an 'OAM' bit
> which typically is used with networking tracing tools to
> distinguish test packets from real traffic. OVS already internally
> has support for this, however, it doesn't do anything with it
> and it also isn't exposed for controllers to use. This enables
> support through OpenFlow.
> 
> There are several other tunnel flags which are consumed internally
> by OVS. It's not clear that it makes sense to use them externally
> so this does not expose those flags - although it should be easy
> to do so if necessary in the future.
> 
> Signed-off-by: Jesse Gross 

Is part of the mf_is_all_wild() change a bug fix that we should
backport?  At first glance it seems so.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] tunneling: Allow matching and setting tunnel 'OAM' flag.

[Ben Pfaff]

On Wed, Jul 15, 2015 at 12:37:08PM -0700, Ben Pfaff wrote:
> On Mon, Jul 13, 2015 at 02:53:57PM -0700, Jesse Gross wrote:
> > Several encapsulation formats have the concept of an 'OAM' bit
> > which typically is used with networking tracing tools to
> > distinguish test packets from real traffic. OVS already internally
> > has support for this, however, it doesn't do anything with it
> > and it also isn't exposed for controllers to use. This enables
> > support through OpenFlow.
> > 
> > There are several other tunnel flags which are consumed internally
> > by OVS. It's not clear that it makes sense to use them externally
> > so this does not expose those flags - although it should be easy
> > to do so if necessary in the future.
> > 
> > Signed-off-by: Jesse Gross 
> 
> Is part of the mf_is_all_wild() change a bug fix that we should
> backport?  At first glance it seems so.

Other than that:
Acked-by: Ben Pfaff 

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

Re: [ovs-dev] [ovn-controller-vtep V3 1/7] ofproto-macros.at: Make check_logs() check all daemons' log files.

[Ben Pfaff]

On Mon, Jul 13, 2015 at 08:22:37PM -0700, Alex Wang wrote:
> As we have more daemons with OVN that can be tested using ovs autotest
> framework, it is convenient to extend check_logs() to check the log files
> of all daemons.
> 
> Signed-off-by: Alex Wang 
> 
> ---
> V2->V3:
> - adopt Ben's suggestion to check for all '*.log's except testsuite.log.
> - rebase to master.
> 
> PATCH->V2:
> - make check_logs() check all daemons' log files.

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

Re: [ovs-dev] [ovn-controller-vtep V3 2/7] ovn-sb: Remove the "Gateway" table from the ovn-sb schema.

[Alex Wang]

On Wed, Jul 15, 2015 at 12:15 PM, Russell Bryant  wrote:

> On 07/15/2015 03:09 PM, Alex Wang wrote:
> > Also, what do you think about my proposal for using "type" and
> "options"
> > fields on logical ports in OVN_Northbound instead of a special port
> > name?  If we went this route, you don't need "logical_switches" to
> be a
> > smap, it can just be a list of logical switch names.  Creating a
> > connection in OVN northbound would be something like:
> >
> >   Logical_Port
> > name: 
> > type: vtep
> > options:
> >vtep_physical_switch: <...>
> >vtep_logical_switch: <...>
> >
> >
> >
> > One thing I see missing is that: multiple vtep physical switches
> > (supported by the same vtep db) could connect to the same vtep
> > logical switch...   In that case, logical switch name can no longer
> > uniquely identify a connection of 'a vtep physical switch to a vtep
> > logical switch'...  That's why I use the combination of
> > pswitch_name+lswitch_name.
> >
> > With you suggestion, first, the logical port name in ovn-nb can be
> > duplicated. And if that is allowed, we need to pass the "options"
> > down to ovn-sb Binding table to uniquely binding the vtep lport to
> > vtep chassis.
>
> Yes, I was thinking that this information would have to be added to the
> Binding table in OVN_Southbound as well so that you still have the
> pswitch+lswitch info, just in a little bit more structured way.
>
>
I see~, that sounds like more changes than just adding the ovn-controller-
vtep.  I can see this is more organized on OVN-NB.  But at the same time
I still like to have unique logical port name in OVN-SB binding.  Maybe we
could combine both?  With your OVN-NB schema change suggestion,
make ovn-northd create the logical_port name in binding table as
pswitch_name+vtep_logical_switch name to guarantee uniqueness?

Anyway, do not think I know all related parts well enough~  would like to
hear
more comments from you and Ben,

Thanks,
Alex Wang,



> --
> Russell Bryant
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [ovn-controller-vtep V3 2/7] ovn-sb: Remove the "Gateway" table from the ovn-sb schema.

[Ben Pfaff]

On Wed, Jul 15, 2015 at 12:09:21PM -0700, Alex Wang wrote:
> With you suggestion, first, the logical port name in ovn-nb can be
> duplicated.
> And if that is allowed, we need to pass the "options" down to ovn-sb
> Binding table to uniquely binding the vtep lport to vtep chassis.

I'm starting to wonder whether we need global uniqueness for logical
port names, or just uniqueness within a logical datapath.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [ovn-controller-vtep V3 2/7] ovn-sb: Remove the "Gateway" table from the ovn-sb schema.

[Ben Pfaff]

On Wed, Jul 15, 2015 at 02:22:58PM -0400, Russell Bryant wrote:
> On 07/13/2015 11:22 PM, Alex Wang wrote:
> > In a gateway like the VTEP L2 gateway, physical vlans belonging to
> > the same logical network form a "logical switch".  Each logical switch
> > has a dedicated tunnel key and will keep records of all MACs learned
> > from the owned vlans.  So user can just send packet to a "logical
> > switch" and the gateway will figure out the output port and vlan tag
> > automatically.
> > 
> > Therefore, it is not really necessary to keep record of the vlan map
> > for each gateway physical port in the OVN_Southbound database using
> > "gateway_ports".
> > 
> > Thusly, this commit removes the "Gateway" table from the OVN_Southbound
> > database.  In the "Chassis" table, the "gateway_ports" column is replaced
> > by "logical_switches" column which maps the logical switch name in the
> > gateway to a logical port name that exists in the OVN_Northbound database's
> > "Logical_Port" table.
> > 
> > Signed-off-by: Alex Wang 
> 
> I think this makes sense to me.  I wonder if calling it
> "logical_switches" could be confusing with the logical switch in OVN
> Northbound.  Maybe "vtep_logical_switches" would clarify?

Or "gateways" or "vtep_gateways".
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [ovn-controller-vtep V3 2/7] ovn-sb: Remove the "Gateway" table from the ovn-sb schema.

[Ben Pfaff]

On Mon, Jul 13, 2015 at 08:22:38PM -0700, Alex Wang wrote:
> In a gateway like the VTEP L2 gateway, physical vlans belonging to
> the same logical network form a "logical switch".  Each logical switch
> has a dedicated tunnel key and will keep records of all MACs learned
> from the owned vlans.  So user can just send packet to a "logical
> switch" and the gateway will figure out the output port and vlan tag
> automatically.
> 
> Therefore, it is not really necessary to keep record of the vlan map
> for each gateway physical port in the OVN_Southbound database using
> "gateway_ports".
> 
> Thusly, this commit removes the "Gateway" table from the OVN_Southbound
> database.  In the "Chassis" table, the "gateway_ports" column is replaced
> by "logical_switches" column which maps the logical switch name in the
> gateway to a logical port name that exists in the OVN_Northbound database's
> "Logical_Port" table.
> 
> Signed-off-by: Alex Wang 
> 
> ---
> ->V3:
> - Realize that the Gateway table is not needed.

I think that the documentation can be improved.

I ended up with this:


  
A gateway is a chassis that forwards traffic between the
OVN-managed part of a logical network and a physical VLAN, extending a
tunnel-based logical network into a physical network.  Gateways are
typically dedicated nodes that do not host VMs.
  

  
Maps from the name of a logical switch on the gateway to a logical port
name.  The logical port name must be unique; one way to do this is by
concatenating the chassis name and the logical switch name.  User needs
to create a same named logical port in the OVN_Northbound database's
Logical_Port table.
  


but I still have some issues with it.  First, I deleted this sentence
because I didn't understand it, but if you can explain then maybe
rephrasing it would be a better option:

Physical VLANs belonging to the one logical network form a
logical switch in the gateway.

Second, the documentation says that the "user" needs to create the
logical port.  I'm not sure who the user is in this case.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC PATCH v2 3/7] flow: Always inline miniflows.

[Jarno Rajahalme]

> On Jul 14, 2015, at 4:50 PM, Ben Pfaff  wrote:
> 
> On Fri, Jul 10, 2015 at 10:35:21AM -0700, Jarno Rajahalme wrote:
>> Now that performance critical code already inlines miniflows and
>> minimasks, we can simplify struct miniflow by always dynamically
>> allocating miniflows and minimasks to the correct size.  This changes
>> the struct minimatch to always contain pointers to its miniflow and
>> minimask.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> I like simplifying things so that two possibilities become one.  It's
> nice.
> 
> I see two possible ways to simplify this, each with its own tradeoffs:
> 
>   1. The way that you did it, which means that one must always
>  either dynamically allocate struct miniflow to a proper size
>  or ensure that there's enough space for the maximum size flow
>  immediately following struct miniflow.  
> 
>   2. Change struct miniflow to:
> 
>struct miniflow {
>uint64_t map;
>uint64_t *values;
>};
> 
>  With this strategy, there's no need to dynamically allocate
>  struct miniflow itself, so it can be directly embedded in
>  larger structures instead of via pointer.  The usual time cost
>  of accessing miniflow data is again one pointer dereference
>  (of 'values').
> 
>  This can be kind of nice because you're not fighting with the
>  compiler to put data in the right place.
> 
> Either way, the usual time cost of accessing miniflow data is one
> pointer dereference (in case #1, from whatever contains the miniflow; in
> case #2, dereference of 'values'), and the usual space cost of embedding
> a miniflow is one pointer (although in case #1 the pointer is in what
> contains the miniflow and in case #2 the pointer is in the miniflow
> itself).
> 
> Anyway, I wanted to make sure that you considered both of these
> solutions and decided that #1 was better.  If so:
> 

I agree that case #2 is syntactically nicer in the generic case, where one 
pointer dereference is always needed. However, we already embed miniflows to 
other data structures (flow keys and masks in classifier and userspace 
datapath), where avoid the dereference and hence make accessing the miniflow 
faster. With case #2 we’d need to define another “struct inline_miniflow” 
(basically the case #1) to avoid the dereference. IMO it is better to use the 
case #1 everywhere than defining two different miniflow structures.

  Jarno

> Acked-by: Ben Pfaff 

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

Re: [ovs-dev] [PATCH 2/2] tunneling: Allow matching and setting tunnel 'OAM' flag.

[Jesse Gross]

On Wed, Jul 15, 2015 at 12:37 PM, Ben Pfaff  wrote:
> On Mon, Jul 13, 2015 at 02:53:57PM -0700, Jesse Gross wrote:
>> Several encapsulation formats have the concept of an 'OAM' bit
>> which typically is used with networking tracing tools to
>> distinguish test packets from real traffic. OVS already internally
>> has support for this, however, it doesn't do anything with it
>> and it also isn't exposed for controllers to use. This enables
>> support through OpenFlow.
>>
>> There are several other tunnel flags which are consumed internally
>> by OVS. It's not clear that it makes sense to use them externally
>> so this does not expose those flags - although it should be easy
>> to do so if necessary in the future.
>>
>> Signed-off-by: Jesse Gross 
>
> Is part of the mf_is_all_wild() change a bug fix that we should
> backport?  At first glance it seems so.

The existing code is definitely conceptually wrong. However, I don't
believe that it can ever result in a triggerable bug. None of these
fields were publicly accessible before now so mf_is_all_wild() should
never be called for those fields.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [ovn-controller-vtep V3 2/7] ovn-sb: Remove the "Gateway" table from the ovn-sb schema.

[Russell Bryant]

On 07/15/2015 04:03 PM, Ben Pfaff wrote:
> On Wed, Jul 15, 2015 at 12:09:21PM -0700, Alex Wang wrote:
>> With you suggestion, first, the logical port name in ovn-nb can be
>> duplicated.
>> And if that is allowed, we need to pass the "options" down to ovn-sb
>> Binding table to uniquely binding the vtep lport to vtep chassis.
> 
> I'm starting to wonder whether we need global uniqueness for logical
> port names, or just uniqueness within a logical datapath.
> 

I think it has to be globally unique, right?  ovn-controller uses the
port name to match ports that appear on the local switch to a logical
port.  How would we match the exact logical port if it's not globally
unique?

Technically we could use the logcial port row's UUID, but I think that
makes integration more difficult.  It will break Neutron integration, at
least.  In OpenStack, Neutron has a UUID for its own logical port db
entry, and *that* UUID is what is used as the iface-id when a VM gets
plugged in to the switch.  OVN integration is pretty easy since we can
just specify Neutron's UUID as the OVN logical port name.

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

Re: [ovs-dev] [ovn-controller-vtep V3 2/7] ovn-sb: Remove the "Gateway" table from the ovn-sb schema.

[Russell Bryant]

On 07/15/2015 03:15 PM, Russell Bryant wrote:
> On 07/15/2015 03:09 PM, Alex Wang wrote:
>> Also, what do you think about my proposal for using "type" and "options"
>> fields on logical ports in OVN_Northbound instead of a special port
>> name?  If we went this route, you don't need "logical_switches" to be a
>> smap, it can just be a list of logical switch names.  Creating a
>> connection in OVN northbound would be something like:
>>
>>   Logical_Port
>> name: 
>> type: vtep
>> options:
>>vtep_physical_switch: <...>
>>vtep_logical_switch: <...>
>>
>>
>>
>> One thing I see missing is that: multiple vtep physical switches 
>> (supported by the same vtep db) could connect to the same vtep
>> logical switch...   In that case, logical switch name can no longer
>> uniquely identify a connection of 'a vtep physical switch to a vtep
>> logical switch'...  That's why I use the combination of
>> pswitch_name+lswitch_name.
>>
>> With you suggestion, first, the logical port name in ovn-nb can be 
>> duplicated. And if that is allowed, we need to pass the "options"
>> down to ovn-sb Binding table to uniquely binding the vtep lport to
>> vtep chassis.
> 
> Yes, I was thinking that this information would have to be added to the
> Binding table in OVN_Southbound as well so that you still have the
> pswitch+lswitch info, just in a little bit more structured way.
> 

Also, I've got some patches in progress that implements this.  It's a
part of a series I'm working on, but I could split it out separately if
you think it makes sense and want to rebase on them.

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

Re: [ovs-dev] [RFC PATCH 0/6] Increase miniflow's capacity.

[Jarno Rajahalme]

On Jul 15, 2015, at 08:53, Traynor, Kevin  wrote:

>> From: Jarno Rajahalme [mailto:jrajaha...@nicira.com]
>> Sent: Friday, July 10, 2015 6:38 PM
>> To: Traynor, Kevin
>> Cc: dev@openvswitch.org
>> Subject: Re: [ovs-dev] [RFC PATCH 0/6] Increase miniflow's capacity.
>> 
>> 
>> On Jul 10, 2015, at 9:38 AM, Traynor, Kevin  wrote:
>> 
>> 
>> -Original Message-
>> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Jarno Rajahalme
>> Sent: Thursday, July 9, 2015 6:16 PM
>> To: dev@openvswitch.org
>> Subject: [ovs-dev] [RFC PATCH 0/6] Increase miniflow's capacity.
>> 
>> Upto now struct miniflow has been limited to 63 64-bit units.  This
>> series increases this capacity to 128 64-bit units.  For presimed
>> performance reasons the new miniflow uses one 64-bit map for tunnel
>> metadata and another for the rest of the metadata and the fields
>> extracted from packet headers.
>> 
>> Before making miniflow more complex, this series simplifies it a bit
>> by always inlining the miniflow data and cleaning up the interface a
>> bit.
>> 
>> All performance testing is yet to be done.  I would be thankful if
>> someone verifies the performance impact on the DPDK datapath, if any.
>> 
>> Hi Jarno, I can run some tests on this early next week when I get access
>> to a board.
>> 
>> 
>> Kevin,
>> 
>> I just sent a v2 based on feedback from Daniele, so please test on that
>> instead!
>> 
>>  Jarno
> 
> I've tested this multiple times for phy-phy with dpdk and a bi-directional 
> flow.
> It's showing an avg. of 16.39 mpps on head of master and 16.67 mpps with your
> changes - so no need to be worried! 
> 
> I ran a few tests on the other dpdk ports and there was a slight drop of 100K 
> pps
> for vhost, but I think this is just test variance.
> 

Thanks for testing this out!

  Jarno

>> 
>> 
>> 
>> 
>> Jarno Rajahalme (6):
>> tests: Check for core files before exiting.
>> meta-flow: Add a missing break statement.
>> lib: Always inline miniflows.
>> match: Single malloc minimatch.
>> flow: Eliminate miniflow_clone() and minimask_clone().
>> flow: Split miniflow's map.
>> 
>> lib/classifier-private.h |  122 
>> lib/classifier.c |  149 --
>> lib/dpif-netdev.c|  103 +-
>> lib/flow.c   |  482 ++--
>> --
>> lib/flow.h   |  252 +---
>> lib/match.c  |   39 ++--
>> lib/match.h  |   12 +-
>> lib/meta-flow.c  |2 +-
>> lib/tnl-ports.c  |4 +-
>> ofproto/ofproto.c|   10 +-
>> tests/ofproto-macros.at  |6 +-
>> tests/test-classifier.c  |  130 +++--
>> 12 files changed, 716 insertions(+), 595 deletions(-)
>> 
>> --
>> 1.7.10.4
>> 
>> ___
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
> 
> 
> 
> 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] IS MSTP enabled on OVS 2.3.9

[Justin Pettit]

> On Jul 14, 2015, at 5:13 AM, ravali.bu...@wipro.com wrote:
> 
> Hi Team,
> 
> I am using ovs-master code Version 2.3.9 for my experiments.
> 
> Currently what I understood from ovs-vsctl  man page we are having commands 
> for the configuring  RSTP.
> 
> Similar way I want  to configure MSTP in OVS  but I have not  seen any 
> supported commands for configuration of MSTP from the interface perspective.
> 
> 
> 
> Can you please suggest me in bringing up with the design and implementation 
> of configuration for bridge and interface support for MSTP (ovs-vsctl) and 
> also datapath support.

MSTP isn't supported by OVS.  Are you asking how you could implement it 
yourself?  If so, I'd look at the code that implements RSTP for inspiration.

--Justin


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

Re: [ovs-dev] [PATCH 2/2] tunneling: Allow matching and setting tunnel 'OAM' flag.

[Ben Pfaff]

On Wed, Jul 15, 2015 at 01:18:26PM -0700, Jesse Gross wrote:
> On Wed, Jul 15, 2015 at 12:37 PM, Ben Pfaff  wrote:
> > On Mon, Jul 13, 2015 at 02:53:57PM -0700, Jesse Gross wrote:
> >> Several encapsulation formats have the concept of an 'OAM' bit
> >> which typically is used with networking tracing tools to
> >> distinguish test packets from real traffic. OVS already internally
> >> has support for this, however, it doesn't do anything with it
> >> and it also isn't exposed for controllers to use. This enables
> >> support through OpenFlow.
> >>
> >> There are several other tunnel flags which are consumed internally
> >> by OVS. It's not clear that it makes sense to use them externally
> >> so this does not expose those flags - although it should be easy
> >> to do so if necessary in the future.
> >>
> >> Signed-off-by: Jesse Gross 
> >
> > Is part of the mf_is_all_wild() change a bug fix that we should
> > backport?  At first glance it seems so.
> 
> The existing code is definitely conceptually wrong. However, I don't
> believe that it can ever result in a triggerable bug. None of these
> fields were publicly accessible before now so mf_is_all_wild() should
> never be called for those fields.

Oh, thanks for pointing that out.  Great, no change needed then.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC PATCH v2 3/7] flow: Always inline miniflows.

[Ben Pfaff]

On Wed, Jul 15, 2015 at 01:11:54PM -0700, Jarno Rajahalme wrote:
> 
> > On Jul 14, 2015, at 4:50 PM, Ben Pfaff  wrote:
> > 
> > On Fri, Jul 10, 2015 at 10:35:21AM -0700, Jarno Rajahalme wrote:
> >> Now that performance critical code already inlines miniflows and
> >> minimasks, we can simplify struct miniflow by always dynamically
> >> allocating miniflows and minimasks to the correct size.  This changes
> >> the struct minimatch to always contain pointers to its miniflow and
> >> minimask.
> >> 
> >> Signed-off-by: Jarno Rajahalme 
> > 
> > I like simplifying things so that two possibilities become one.  It's
> > nice.
> > 
> > I see two possible ways to simplify this, each with its own tradeoffs:
> > 
> >   1. The way that you did it, which means that one must always
> >  either dynamically allocate struct miniflow to a proper size
> >  or ensure that there's enough space for the maximum size flow
> >  immediately following struct miniflow.  
> > 
> >   2. Change struct miniflow to:
> > 
> >struct miniflow {
> >uint64_t map;
> >uint64_t *values;
> >};
> > 
> >  With this strategy, there's no need to dynamically allocate
> >  struct miniflow itself, so it can be directly embedded in
> >  larger structures instead of via pointer.  The usual time cost
> >  of accessing miniflow data is again one pointer dereference
> >  (of 'values').
> > 
> >  This can be kind of nice because you're not fighting with the
> >  compiler to put data in the right place.
> > 
> > Either way, the usual time cost of accessing miniflow data is one
> > pointer dereference (in case #1, from whatever contains the miniflow; in
> > case #2, dereference of 'values'), and the usual space cost of embedding
> > a miniflow is one pointer (although in case #1 the pointer is in what
> > contains the miniflow and in case #2 the pointer is in the miniflow
> > itself).
> > 
> > Anyway, I wanted to make sure that you considered both of these
> > solutions and decided that #1 was better.  If so:
> > 
> 
> I agree that case #2 is syntactically nicer in the generic case, where
> one pointer dereference is always needed. However, we already embed
> miniflows to other data structures (flow keys and masks in classifier
> and userspace datapath), where avoid the dereference and hence make
> accessing the miniflow faster. With case #2 we’d need to define
> another “struct inline_miniflow” (basically the case #1) to avoid the
> dereference. IMO it is better to use the case #1 everywhere than
> defining two different miniflow structures.

OK, I'm glad that you considered the possibility then.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [ovn-controller-vtep V3 2/7] ovn-sb: Remove the "Gateway" table from the ovn-sb schema.

[Ben Pfaff]

On Wed, Jul 15, 2015 at 04:23:14PM -0400, Russell Bryant wrote:
> On 07/15/2015 04:03 PM, Ben Pfaff wrote:
> > On Wed, Jul 15, 2015 at 12:09:21PM -0700, Alex Wang wrote:
> >> With you suggestion, first, the logical port name in ovn-nb can be
> >> duplicated.
> >> And if that is allowed, we need to pass the "options" down to ovn-sb
> >> Binding table to uniquely binding the vtep lport to vtep chassis.
> > 
> > I'm starting to wonder whether we need global uniqueness for logical
> > port names, or just uniqueness within a logical datapath.
> > 
> 
> I think it has to be globally unique, right?  ovn-controller uses the
> port name to match ports that appear on the local switch to a logical
> port.  How would we match the exact logical port if it's not globally
> unique?

We'd have to introduce some context to identify the logical datapath as
well.  It may not be worth it; it was an idle thought.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [v3 2/3] perf-counter: simplify the performance macro

[Andy Zhou]

Replace the original PERF_FUNCTION_BEGIN and PERF_FUNCTION_END
pair with a single PERF macro. This design is also more flexible,
removing the restriction of have only one measurement per function.

The next patch will make use of this macro.

Signed-off-by: Andy Zhou 
Acked-by: Ben Pfaff 

---
v1 -> v2:  Drop perf-counter.c clean up changes.
v2 -> v3:  No change.
---
 lib/perf-counter.h | 75 +-
 1 file changed, 29 insertions(+), 46 deletions(-)

diff --git a/lib/perf-counter.h b/lib/perf-counter.h
index 23365de..ca3aaba 100644
--- a/lib/perf-counter.h
+++ b/lib/perf-counter.h
@@ -44,54 +44,37 @@
  * Those are not fundamental limits, but only limited by current
  * implementation.
  *
- * Function instruction counter sample point Usage
- * 
+ * Usage:
+ * ===
  *
- * There are two macros provided:
+ * Adding performance counter is easy. Simply use the following macro to
+ * wrap around the expression you are interested in measuring.
  *
- * Macro 'PERF_FUNCTON_COUNT_BEGIN' needs to be inserted towards the
- * beginning of the function where local variables are declared.
+ * PERF(name, expr).
  *
- * Macro 'PERF_FUNCTON_COUNT_END' needs to appear in the same function,
- * some where below 'PERF_FUNCTION_COUNT_BEGIN', usually towards of
- * a function.
+ * The 'expr' is a set of C expressions you are interested in measuring.
+ * 'name' is the counter name.
  *
- * For example:
+ * For example, if we are interested in performance of perf_func():
  *
- *void my_func() {
- *  int some_local_variable;
- *
- *  PERF_FUNCTION_COUNT_BEGIN;
- *
- *  < implementation >
- *
- *  PERF_FUNCTION_COUNT_END
+ *int perf_func() {
+ *
  *}
  *
- * This will maintain the number of times 'my_func()' is called, total
- * number of instructions '' executed during all those calls.
+ *void func() {
+ *int rt;
  *
- * Currently there are two limitation:
- * 1). At most one pair can appear in the same variable scope.
- * 2). The Macros use function name as the counter name for display.
- * Thus, all functions in one annotation session are required to
- * have unique names.
+ *...
+ *PERF("perf_func", rt = perf_func());
  *
- * Note, there is no requirement for those macros to be balanced.
- * For example:
+ *return rt;
+ *}
  *
- *void my_func(int i){
  *
- *  PERF_FUNCTION_COUNT_BEGIN;
+ * This will maintain the number of times 'perf_func()' is called, total
+ * number of instructions '' plus function call overhead
+ * executed.
  *
- *  if (i == 300) {
- *  PERF_FUNCTION_COUNT_END;
- *  return;
- *  } else {
- *   
- *  }
- *}
- * will work just fine.
  */
 
 #if defined(__linux__) && defined(HAVE_LINUX_PERF_EVENT_H)
@@ -120,17 +103,17 @@ void perf_counter_accumulate(struct perf_counter *counter,
 char *perf_counters_to_string(void);
 
 /* User access macros. */
-#define PERF_FUNCTION_BEGIN \
-static struct perf_counter x__ = PERF_COUNTER_ONCE_INITIALIZER(__func__); \
-uint64_t start_count__ = perf_counter_read(&start_count__);   \
-
-#define PERF_FUNCTION_END \
-perf_counter_accumulate(&x__, start_count__);
-
+#define PERF(name, expr) \
+  { \
+  static struct perf_counter c = PERF_COUNTER_ONCE_INITIALIZER(name);\
+  uint64_t start_count = perf_counter_read(&start_count); \
+  \
+  expr;   \
+  \
+  perf_counter_accumulate(&c, start_count);   \
+  }
 #else
-
-#define PERF_FUNCTON_BEGIN
-#define PERF_FUNCTON_END
+#define PERF(name, expr)
 
 static inline void perf_counters_init(void) {}
 static inline void perf_counters_destroy(void) {}
-- 
1.9.1

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

[ovs-dev] [v3 3/3] ovsdb: Add per transaction commit instruction counter

[Andy Zhou]

Measure user space only instruction counters for commit each
transaction. This measurement is mainly useful for
tuning OVSDB internal implementation, such as how OVSDB scales over
number of client connections.

A simple usage example:

ovs-appctl -t ovsdb-server ovsdb-server/perf-counters-clear
ovs-vsctl add-port br0 p3
ovs-appctl -t ovsdb-server ovsdb-server/perf-counters-show
ovsdb_txn_commit2  906922 453461.0

The commands above show that the 'add-port br p3' command caused ovsdb
to execute 2 transactions. Each transaction takes 453461 instructions,
total 906922 instructions. The number will vary based on number of
ovsdb clients connected.

Signed-off-by: Andy Zhou 
Acked-by: Ben Pfaff 
---
 ovsdb/transaction.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
index 9e03963..83ddaff 100644
--- a/ovsdb/transaction.c
+++ b/ovsdb/transaction.c
@@ -27,6 +27,7 @@
 #include "ovsdb.h"
 #include "row.h"
 #include "table.h"
+#include "perf-counter.h"
 #include "uuid.h"
 
 struct ovsdb_txn {
@@ -747,8 +748,8 @@ check_index_uniqueness(struct ovsdb_txn *txn OVS_UNUSED,
 return NULL;
 }
 
-struct ovsdb_error *
-ovsdb_txn_commit(struct ovsdb_txn *txn, bool durable)
+static struct ovsdb_error *
+ovsdb_txn_commit_(struct ovsdb_txn *txn, bool durable)
 {
 struct ovsdb_replica *replica;
 struct ovsdb_error *error;
@@ -821,6 +822,15 @@ ovsdb_txn_commit(struct ovsdb_txn *txn, bool durable)
 return NULL;
 }
 
+struct ovsdb_error *
+ovsdb_txn_commit(struct ovsdb_txn *txn, bool durable)
+{
+   struct ovsdb_error *err;
+
+   PERF(__func__, err = ovsdb_txn_commit_(txn, durable));
+   return err;
+}
+
 void
 ovsdb_txn_for_each_change(const struct ovsdb_txn *txn,
   ovsdb_txn_row_cb_func *cb, void *aux)
-- 
1.9.1

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

[ovs-dev] [v3 1/3] perf-counter: initialize perf counter shash before use

[Andy Zhou]

Private variable perf_counters needs to be initialized before
use. Otherwise,  perf_counter_init() call will cause crashes.

Signed-off-by: Andy Zhou 
---
 lib/perf-counter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/perf-counter.c b/lib/perf-counter.c
index 8c859cc..62e2d76 100644
--- a/lib/perf-counter.c
+++ b/lib/perf-counter.c
@@ -31,7 +31,7 @@
 #include "shash.h"
 #include "util.h"
 
-static struct shash perf_counters;
+static struct shash perf_counters = SHASH_INITIALIZER(&perf_counters);
 static int fd__ = 0;
 
 uint64_t
-- 
1.9.1

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

Re: [ovs-dev] [ovn-controller-vtep V3 3/7] ovn-sbctl: Add ovn-sbctl.

[Ben Pfaff]

On Mon, Jul 13, 2015 at 08:22:39PM -0700, Alex Wang wrote:
> This commit adds ovn-sbctl to ovn family by using the db-ctl-base
> library.
> 
> As a side effect, we move the ovn-nbctl/ovn-sbctl related files
> into ovn/utilities.
> 
> Signed-off-by: Alex Wang 
> Acked-by: Ben Pfaff 
> 
> ---
> V2->V3:
> - rebase to base.
> - adopt Russell's review comments.
> - move ovn-nbctl/sbctl related files into ovn/utilities.
> 
> PATCH->V2:
> - change command add/del-ch to add/del-chassis.
> - refine the manual based on comments from Ben.

I get a test failure in a new test due to:
ovn-sbctl: unknown command 'add-chassis'; use --help for help

I think that the manual should say that it is really for advanced
debugging and troubleshooting and that it should not be used in normal
operation (I guess ovn-nbctl should say the same thing; I thought it did
before but I don't see it now).

If you fix those then:
Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [v2 2/2] ovsdb: Add per transaction commit instruction counter

[Andy Zhou]

When dropping perf-counter.c, I missed that It also contained a fix
that is necessary for the remaining patches for pass unit tests.

I will add it back to the patch set and post v3.  Sorry about this.

On Tue, Jul 14, 2015 at 5:06 PM, Ben Pfaff  wrote:
> On Tue, Jul 14, 2015 at 03:57:15PM -0700, Ben Pfaff wrote:
>> On Wed, Jul 08, 2015 at 02:17:57PM -0700, Andy Zhou wrote:
>> > Measure user space only instruction counters for commit each
>> > transaction. This measurement is mainly useful for
>> > tuning OVSDB internal implementation, such as how OVSDB scales over
>> > number of client connections.
>> >
>> > A simple usage example:
>> >
>> > ovs-appctl -t ovsdb-server ovsdb-server/perf-counters-clear
>> > ovs-vsctl add-port br0 p3
>> > ovs-appctl -t ovsdb-server ovsdb-server/perf-counters-show
>> > ovsdb_txn_commit2  906922 453461.0
>> >
>> > The commands above show that the 'add-port br p3' command caused ovsdb
>> > to execute 2 transactions. Each transaction takes 453461 instructions,
>> > total 906922 instructions. The number will vary based on number of
>> > ovsdb clients connected.
>> >
>> > Signed-off-by: Andy Zhou 
>> > Acked-by: Ben Pfaff 
>>
>> Acked-by: Ben Pfaff 
>
> Actually I have to take this back, because this commit causes tons of
> test failures for me:
>
>Subject: [openvswitch 2.4.90] testsuite: 103 104 105 106 107 108 118 
> 1275 1276 1277 1278 1279 1280 1281 1282 1283 1284 1285 1286 1287 1288 1289 
> 1293 1294 1295 1296 1297 1298 1299 1300 1301 1302 1303 1304 1305 1309 1310 
> 1311 1312 1313 1314 1315 1316 1317 1318 1320 1321 1322 1323 1324 1325 1326 
> 1327 1328 1329 1330 1331 1332 1333 1334 1335 1339 1340 1341 1342 1343 1344 
> 1345 1346 1347 1348 1349 1350 1351 1383 1384 1390 1394 1396 1450 1451 1452 
> 1454 1455 1457 1458 1468 1469 1470 1471 1472 1473 1474 1476 1477 1478 1479 
> 1480 1481 1482 1483 1484 1485 1486 1487 1488 1542 1543 1544 1545 1546 1547 
> 1548 1549 1550 1551 1552 1553 1554 1555 1556 1557 1558 1559 1560 1561 1562 
> 1563 1564 1565 1566 1567 1568 1569 1570 1571 1572 1573 1574 1575 1576 1577 
> 1580 1581 1582 1616 1617 1618 1619 1620 1621 1622 1623 1624 1625 1626 1627 
> 1628 1629 1630 1631 1632 1633 1634 1635 1636 1637 1638 1639 1640 1641 1642 
> 1643 1644 1645 1646 1647 1648 1649 failed
>
> The tests all seem to fail because of segfaults in ovsdb-server.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] flow: Factor out flag parsing and formatting routines.

[Jesse Gross]

On Wed, Jul 15, 2015 at 11:35 AM, Ben Pfaff  wrote:
> On Mon, Jul 13, 2015 at 02:53:56PM -0700, Jesse Gross wrote:
>> There are several implementations of functions that parse/format
>> flags and their binary representation. This factors them out into
>> common routines. In addition to reducing code, it also makes things
>> more consistent across different parts of OVS.
>>
>> Signed-off-by: Jesse Gross 
>
> Thanks, I like reducing code size!
>
> This patch deletes a test and part of a test in ovs-ofctl.at.  Why?

I'm not sure that these tests really make sense and arguably the
behavior there were checking wasn't even right in the first place. In
theory they are verifying that the correct OpenFlow version is
selected based on the fields provided (in this case the correct
version is "none") but it is being done using names that weren't
supposed to be public, though the old parsing code allowed them to
leak through. The new parsing code is enforcing the same invariants as
before but more carefully and now rejects these commands as a parse
error before it even gets to the OpenFlow layer that is supposed to be
exercised. The next patch makes this field valid anyways and verifies
the correct OpenFlow behavior, so it didn't seem like it made sense to
keep the test around.

> In parse_flags(), s/preceed/preceded/:
> *res_string = xasprintf("%s: %s must be preceed by '+' "

Fixed.

> parse_flags() could use a function-level comment.

That's a good idea. I added this:

/* Scans a string 's' of flags to determine their numerical value and
 * returns the number of characters parsed using 'bit_to_string' to
 * lookup flag names. Scanning continues until the character 'end' is
 * reached.
 *
 * In the event of a failure, a negative error code will be returned. In
 * addition, if 'res_string' is non-NULL then a descriptive string will
 * be returned incorporating the identifying string 'field_name'. This
 * error string must be freed by the caller.
 *
 * Upon success, the flag values will be stored in 'res_flags' and
 * optionally 'res_mask', if it is non-NULL (if it is NULL then any masks
 * present in the original string will be considered an error). The
 * caller may restrict the acceptable set of values through the mask
 * 'allowed'. */

> format_flags_masked() will output the empty string if flags == 0 && mask
> == 0, and I think that parse_flags() will accept that too, but I don't
> know if it's the most user-friendly form.  Don't know if that matters.

I agree, it's clearer and less ambiguous to output something in all
cases. I added this:

diff --git a/lib/flow.c b/lib/flow.c
index 0fc19d7..a13d832 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -843,6 +843,7 @@ format_flags(struct ds *ds, const char
*(*bit_to_string)(uint32_t),
 uint32_t bad = 0;

 if (!flags) {
+ds_put_char(ds, '0');
 return;
 }
 while (flags) {
@@ -875,12 +876,12 @@ format_flags_masked(struct ds *ds, const char *name,
 }

 if (mask == max_mask) {
-if (flags) {
-format_flags(ds, bit_to_string, flags, '|');
-} else {
-ds_put_char(ds, '0');
-}
+format_flags(ds, bit_to_string, flags, '|');
+return;
+}

+if (!mask) {
+ds_put_cstr(ds, "0/0");
 return;
 }

> format_flags_masked() will output "0" if flags == 0 && mask == max_mask,
> but parse_flags() won't accept "0".  It would be nice to resolve that.

I'm not sure if I understand correctly - parse_flags() should accept
both numbers as well as flag names either with or without masks. At
the top it scans for masked numeric values (which won't match in this
case) but in the second half of the function after it has looked for
masked flags, it tries again to look for both unmasked numbers and
names. I believe the unmasked numbers check should handle this
correctly.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC PATCH v2 2/7] meta-flow: Add a missing break statement.

[Jarno Rajahalme]

> On Jul 10, 2015, at 11:19 AM, Jesse Gross  wrote:
> 
> On Fri, Jul 10, 2015 at 10:35 AM, Jarno Rajahalme  
> wrote:
>> Signed-off-by: Jarno Rajahalme 
>> ---
>> lib/meta-flow.c |2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> This one seems like an obvious bugfix that can go in independent of
> the rest of the series.
> 
> Acked-by: Jesse Gross 

Thanks for the review,

Pushed to master,

  Jarno

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

Re: [ovs-dev] [RFC PATCH v2 3/7] flow: Always inline miniflows.

[Jarno Rajahalme]

Thanks for the review, pushed to master.

  Jarno

> On Jul 15, 2015, at 3:05 PM, Ben Pfaff  wrote:
> 
> On Wed, Jul 15, 2015 at 01:11:54PM -0700, Jarno Rajahalme wrote:
>> 
>>> On Jul 14, 2015, at 4:50 PM, Ben Pfaff  wrote:
>>> 
>>> On Fri, Jul 10, 2015 at 10:35:21AM -0700, Jarno Rajahalme wrote:
 Now that performance critical code already inlines miniflows and
 minimasks, we can simplify struct miniflow by always dynamically
 allocating miniflows and minimasks to the correct size.  This changes
 the struct minimatch to always contain pointers to its miniflow and
 minimask.
 
 Signed-off-by: Jarno Rajahalme 
>>> 
>>> I like simplifying things so that two possibilities become one.  It's
>>> nice.
>>> 
>>> I see two possible ways to simplify this, each with its own tradeoffs:
>>> 
>>>  1. The way that you did it, which means that one must always
>>> either dynamically allocate struct miniflow to a proper size
>>> or ensure that there's enough space for the maximum size flow
>>> immediately following struct miniflow.  
>>> 
>>>  2. Change struct miniflow to:
>>> 
>>>   struct miniflow {
>>>   uint64_t map;
>>>   uint64_t *values;
>>>   };
>>> 
>>> With this strategy, there's no need to dynamically allocate
>>> struct miniflow itself, so it can be directly embedded in
>>> larger structures instead of via pointer.  The usual time cost
>>> of accessing miniflow data is again one pointer dereference
>>> (of 'values').
>>> 
>>> This can be kind of nice because you're not fighting with the
>>> compiler to put data in the right place.
>>> 
>>> Either way, the usual time cost of accessing miniflow data is one
>>> pointer dereference (in case #1, from whatever contains the miniflow; in
>>> case #2, dereference of 'values'), and the usual space cost of embedding
>>> a miniflow is one pointer (although in case #1 the pointer is in what
>>> contains the miniflow and in case #2 the pointer is in the miniflow
>>> itself).
>>> 
>>> Anyway, I wanted to make sure that you considered both of these
>>> solutions and decided that #1 was better.  If so:
>>> 
>> 
>> I agree that case #2 is syntactically nicer in the generic case, where
>> one pointer dereference is always needed. However, we already embed
>> miniflows to other data structures (flow keys and masks in classifier
>> and userspace datapath), where avoid the dereference and hence make
>> accessing the miniflow faster. With case #2 we’d need to define
>> another “struct inline_miniflow” (basically the case #1) to avoid the
>> dereference. IMO it is better to use the case #1 everywhere than
>> defining two different miniflow structures.
> 
> OK, I'm glad that you considered the possibility then.

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

Re: [ovs-dev] [RFC PATCH v2 4/7] match: Single malloc minimatch.

[Jarno Rajahalme]

Pushed to master,

  Jarno

> On Jul 14, 2015, at 10:16 PM, Ben Pfaff  wrote:
> 
> On Fri, Jul 10, 2015 at 10:35:22AM -0700, Jarno Rajahalme wrote:
>> Allocate the miniflow and minimask in struct minimatch at once, so
>> that they are consecutive in memory.  This halves the number of
>> allocations, and allows smaller minimatches to share the same cache
>> line.
>> 
>> After this a minimatch has one heap allocation for all it's data.
>> Previously it had either none (when data was small enough to fit in
>> struct miniflow's inline buffer), or two (when the inline buffer was
>> insufficient).  Hopefully always having one performs almost the same
>> as none or two, in average.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> Acked-by: Ben Pfaff 

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

Re: [ovs-dev] [RFC PATCH v2 5/7] flow: Eliminate miniflow_clone() and minimask_clone().

[Jarno Rajahalme]

> On Jul 14, 2015, at 10:31 PM, Ben Pfaff  wrote:
> 
> On Fri, Jul 10, 2015 at 10:35:23AM -0700, Jarno Rajahalme wrote:
>> miniflow_clone() and minimask_clone() are no longer used, remove them
>> from the API.
>> 
>> Now that miniflow data is always inlined, it makes sense to rename
>> miniflow_clone_inline() miniflow_clone().
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> Acked-by: Ben Pfaff 

Thanks for the review!

Pushed to master,

  Jarno

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

Re: [ovs-dev] [ovn-controller-vtep V3 2/7] ovn-sb: Remove the "Gateway" table from the ovn-sb schema.

[Alex Wang]

On Wed, Jul 15, 2015 at 3:14 PM, Ben Pfaff  wrote:

> On Wed, Jul 15, 2015 at 04:23:14PM -0400, Russell Bryant wrote:
> > On 07/15/2015 04:03 PM, Ben Pfaff wrote:
> > > On Wed, Jul 15, 2015 at 12:09:21PM -0700, Alex Wang wrote:
> > >> With you suggestion, first, the logical port name in ovn-nb can be
> > >> duplicated.
> > >> And if that is allowed, we need to pass the "options" down to ovn-sb
> > >> Binding table to uniquely binding the vtep lport to vtep chassis.
> > >
> > > I'm starting to wonder whether we need global uniqueness for logical
> > > port names, or just uniqueness within a logical datapath.
> > >
> >
> > I think it has to be globally unique, right?  ovn-controller uses the
> > port name to match ports that appear on the local switch to a logical
> > port.  How would we match the exact logical port if it's not globally
> > unique?
>
> We'd have to introduce some context to identify the logical datapath as
> well.  It may not be worth it; it was an idle thought.
>


So, should we design that now?  or later?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] [OVN] Add QoS to NB schema

[Gal Sagie]

Yes, there are currently plans for marking (DSCP) and flow classification
profiles.
And other ideas like connections limiting.

On Wed, Jul 15, 2015 at 8:38 PM, Ben Pfaff  wrote:

> On Sun, Jul 12, 2015 at 09:21:41AM +0300, Gal Sagie wrote:
> > Add QoS profile and QoS rule tables and explanations
> > to the NB schema.
> > Add specific bandwidth limit rule configurations
> > in the document
> >
> > Signed-off-by: Gal Sagie 
>
> This is a lot of schema to support just a little bit of functionality!
> I guess that must be because you foresee support for much more
> sophisticated QoS profiles in the future.  Can you tell us a little
> about those?  Otherwise it would make more sense to me to do this in a
> simpler way.
>



-- 
Best Regards ,

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

Re: [ovs-dev] [RFC PATCH v2 5/7] flow: Eliminate miniflow_clone() and minimask_clone().

[Gurucharan Shetty]

One of the patches in this series breaks windows build because of zero
sized array.
https://ci.appveyor.com/project/blp/ovs/build/1.0.458

On Wed, Jul 15, 2015 at 4:01 PM, Jarno Rajahalme  wrote:
>
>> On Jul 14, 2015, at 10:31 PM, Ben Pfaff  wrote:
>>
>> On Fri, Jul 10, 2015 at 10:35:23AM -0700, Jarno Rajahalme wrote:
>>> miniflow_clone() and minimask_clone() are no longer used, remove them
>>> from the API.
>>>
>>> Now that miniflow data is always inlined, it makes sense to rename
>>> miniflow_clone_inline() miniflow_clone().
>>>
>>> Signed-off-by: Jarno Rajahalme 
>>
>> Acked-by: Ben Pfaff 
>
> Thanks for the review!
>
> Pushed to master,
>
>   Jarno
>
> ___
> 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] [RFC PATCH v2 6/7] flow: Split miniflow's map.

[Jarno Rajahalme]

> On Jul 14, 2015, at 10:37 PM, Ben Pfaff  wrote:
> 
> On Fri, Jul 10, 2015 at 10:35:24AM -0700, Jarno Rajahalme wrote:
>> Use two maps in miniflow to allow for expansion of struct flow past
>> 512 bytes.  We now have one map for tunnel related fields, and another
>> for the rest of the packet metadata and actual packet header fields.
>> This split has the benefit that for non-tunneled packets the overhead
>> should be minimal.
>> 
>> Some miniflow utilities now exist in two variants, new ones operating
>> over all the data, and the old ones operating only on a single 64-bit
>> map at a time.  The old ones require doubling of code but should
>> execute faster, so those are used in the datapath and classifier's
>> lookup path.
>> 
>> Signed-off-by: Jarno Rajahalme 
> 
> I get sparse warnings:
> 
>../lib/classifier-private.h:244:41: warning: shift too big (4294967258) 
> for type unsigned long long
>  CC   lib/flow.lo
>  CC   lib/learn.lo
>../lib/flow.c:461:9: warning: shift too big (98) for type unsigned long 
> long
>../lib/flow.c:461:9: warning: shift too big (4294967258) for type unsigned 
> long long
>../lib/flow.c:465:13: warning: shift too big (64) for type unsigned long 
> long
>../lib/flow.c:465:13: warning: shift too big (4294967262) for type 
> unsigned long long
> 

Is this on a 32-bit build? If so, did you get these on 64-bit build, too?

  Jarno

> miniflow_get_map_in_range() is pretty big, though it should optimize
> down a bit in particular cases.  Maybe you should mark it ALWAYS_INLINE
> to help the compiler with that.
> 
> If you don't mind, I'd like to carefully review a v3 with the above
> shifts fixed.

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

Re: [ovs-dev] [RFC PATCH v2 5/7] flow: Eliminate miniflow_clone() and minimask_clone().

[Jarno Rajahalme]

I can get on this tomorrow, if a more urgent fix is needed, then the three 
patches on the series I pushed should be reverted.

  Jarno


> On Jul 15, 2015, at 16:35, Gurucharan Shetty  wrote:
> 
> One of the patches in this series breaks windows build because of zero
> sized array.
> https://ci.appveyor.com/project/blp/ovs/build/1.0.458
> 
>> On Wed, Jul 15, 2015 at 4:01 PM, Jarno Rajahalme  
>> wrote:
>> 
>>> On Jul 14, 2015, at 10:31 PM, Ben Pfaff  wrote:
>>> 
>>> On Fri, Jul 10, 2015 at 10:35:23AM -0700, Jarno Rajahalme wrote:
 miniflow_clone() and minimask_clone() are no longer used, remove them
 from the API.
 
 Now that miniflow data is always inlined, it makes sense to rename
 miniflow_clone_inline() miniflow_clone().
 
 Signed-off-by: Jarno Rajahalme 
>>> 
>>> Acked-by: Ben Pfaff 
>> 
>> Thanks for the review!
>> 
>> Pushed to master,
>> 
>>  Jarno
>> 
>> ___
>> 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] [RFC PATCH v2 5/7] flow: Eliminate miniflow_clone() and minimask_clone().

[Gurucharan Shetty]

tomorrow is fine, thank Jarno!

On Wed, Jul 15, 2015 at 5:11 PM, Jarno Rajahalme  wrote:
> I can get on this tomorrow, if a more urgent fix is needed, then the three 
> patches on the series I pushed should be reverted.
>
>   Jarno
>
>
>> On Jul 15, 2015, at 16:35, Gurucharan Shetty  wrote:
>>
>> One of the patches in this series breaks windows build because of zero
>> sized array.
>> https://ci.appveyor.com/project/blp/ovs/build/1.0.458
>>
>>> On Wed, Jul 15, 2015 at 4:01 PM, Jarno Rajahalme  
>>> wrote:
>>>
 On Jul 14, 2015, at 10:31 PM, Ben Pfaff  wrote:

 On Fri, Jul 10, 2015 at 10:35:23AM -0700, Jarno Rajahalme wrote:
> miniflow_clone() and minimask_clone() are no longer used, remove them
> from the API.
>
> Now that miniflow data is always inlined, it makes sense to rename
> miniflow_clone_inline() miniflow_clone().
>
> Signed-off-by: Jarno Rajahalme 

 Acked-by: Ben Pfaff 
>>>
>>> Thanks for the review!
>>>
>>> Pushed to master,
>>>
>>>  Jarno
>>>
>>> ___
>>> 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] ovs-ofctl mod-table commands supporting OF1.4 Eviction and Vacancy-Events

[Ben Pfaff]

On Tue, Jul 14, 2015 at 11:39:17AM +0530, Saloni Jain wrote:
> Thanks for the reply.
> Please help me understand below points:
> As per openflow specification 1.4, Page 72 "The flag OFPTC_VACANCY_EVENTS 
> control vacancy events in that table (see 7.4.5). If this flag is set, the 
> switch must generate vacancy events for that table. If this flag is unset, 
> the switch must not generate those events. Parameters for vacancy events may 
> be specified using the OFPTMPT_VACANCY property."
> 
> This means that OFPTC_VACANCY_EVENTS is property for the table and not for 
> per-OpenFlow connection basis.
> 
> Regarding the events, then only those controller will get vacancy events 
> which have OFPT_TABLE_STATUS messages set on using "set-async configuration" 
> OFPT_SET_ASYNC.
> Page 121 of openflow 1.4 says that "The OFPT_SET_ASYNC message sets whether a 
> controller should receive a given asynchronous message that is generated by 
> the switch. Other OpenFlow features control whether a given message is 
> generated; for example, the OFPFF_SEND_FLOW_REM flag in each flow entry 
> controls whether the switch generates a OFPT_FLOW_REMOVED message when a flow 
> entry is removed. The asynchronous configuration acts as an additional 
> per-controller filter; for example it defines if a specific controller 
> receives those OFPT_FLOW_REMOVED messages."
> 
> So as per my understanding OFPTC_VACANCY_EVENTS set as config property will 
> generate vacancy_events using OFPT_TABLE_STATUS message type and reason 
> OFPTR_VACANCY_DOWN/OFPTR_VACANCY_UP for that table, but only those controller 
> will receive those events which have OFPT_TABLE_STATUS messages turned on via 
> OFPT_SET_ASYNC message.

OK, let's do it that way.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] ovs-ofctl mod-table commands supporting OF1.4 Eviction and Vacancy-Events

[Ben Pfaff]

On Tue, Jul 14, 2015 at 05:51:50PM +0530, Saloni Jain wrote:
> In addition to my previous response, please suggest if "vswitchd"
> should query and get the current stored table-configurartion
> using query_tables_desc() and then modify table-config properties
> according to user's request.

I don't understand the question.  Do you mean ovs-ofctl instead of
vswitchd?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [ovn-controller-vtep V3 1/7] ofproto-macros.at: Make check_logs() check all daemons' log files.

[Alex Wang]

Thx for the review, applied to master~

On Wed, Jul 15, 2015 at 12:42 PM, Ben Pfaff  wrote:

> On Mon, Jul 13, 2015 at 08:22:37PM -0700, Alex Wang wrote:
> > As we have more daemons with OVN that can be tested using ovs autotest
> > framework, it is convenient to extend check_logs() to check the log files
> > of all daemons.
> >
> > Signed-off-by: Alex Wang 
> >
> > ---
> > V2->V3:
> > - adopt Ben's suggestion to check for all '*.log's except testsuite.log.
> > - rebase to master.
> >
> > PATCH->V2:
> > - make check_logs() check all daemons' log files.
>
> Acked-by: Ben Pfaff 
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] flow: Factor out flag parsing and formatting routines.

[Joe Stringer]

On 15 July 2015 at 15:53, Jesse Gross  wrote:
> On Wed, Jul 15, 2015 at 11:35 AM, Ben Pfaff  wrote:
>> On Mon, Jul 13, 2015 at 02:53:56PM -0700, Jesse Gross wrote:
>>> There are several implementations of functions that parse/format
>>> flags and their binary representation. This factors them out into
>>> common routines. In addition to reducing code, it also makes things
>>> more consistent across different parts of OVS.
>>>
>>> Signed-off-by: Jesse Gross 
>>
>> Thanks, I like reducing code size!
>>
>> This patch deletes a test and part of a test in ovs-ofctl.at.  Why?
>
> I'm not sure that these tests really make sense and arguably the
> behavior there were checking wasn't even right in the first place. In
> theory they are verifying that the correct OpenFlow version is
> selected based on the fields provided (in this case the correct
> version is "none") but it is being done using names that weren't
> supposed to be public, though the old parsing code allowed them to
> leak through. The new parsing code is enforcing the same invariants as
> before but more carefully and now rejects these commands as a parse
> error before it even gets to the OpenFlow layer that is supposed to be
> exercised. The next patch makes this field valid anyways and verifies
> the correct OpenFlow behavior, so it didn't seem like it made sense to
> keep the test around.

Hmm, interesting. One of the bugs found by STACK recently also changes
the output of this test -- that bug was long-lived, in
parse_ofp_str__(). I plan to send the patches soonish. As I
understand, these tests are also meant to pick up on fields that
aren't supposed to be exposed, and make sure that ovs-ofctl doesn't
try to pass them on. That said, if the field will be allowed after the
next patch then I don't object.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] flow: Factor out flag parsing and formatting routines.

[Jesse Gross]

On Wed, Jul 15, 2015 at 5:31 PM, Joe Stringer  wrote:
> On 15 July 2015 at 15:53, Jesse Gross  wrote:
>> On Wed, Jul 15, 2015 at 11:35 AM, Ben Pfaff  wrote:
>>> On Mon, Jul 13, 2015 at 02:53:56PM -0700, Jesse Gross wrote:
 There are several implementations of functions that parse/format
 flags and their binary representation. This factors them out into
 common routines. In addition to reducing code, it also makes things
 more consistent across different parts of OVS.

 Signed-off-by: Jesse Gross 
>>>
>>> Thanks, I like reducing code size!
>>>
>>> This patch deletes a test and part of a test in ovs-ofctl.at.  Why?
>>
>> I'm not sure that these tests really make sense and arguably the
>> behavior there were checking wasn't even right in the first place. In
>> theory they are verifying that the correct OpenFlow version is
>> selected based on the fields provided (in this case the correct
>> version is "none") but it is being done using names that weren't
>> supposed to be public, though the old parsing code allowed them to
>> leak through. The new parsing code is enforcing the same invariants as
>> before but more carefully and now rejects these commands as a parse
>> error before it even gets to the OpenFlow layer that is supposed to be
>> exercised. The next patch makes this field valid anyways and verifies
>> the correct OpenFlow behavior, so it didn't seem like it made sense to
>> keep the test around.
>
> Hmm, interesting. One of the bugs found by STACK recently also changes
> the output of this test -- that bug was long-lived, in
> parse_ofp_str__(). I plan to send the patches soonish. As I
> understand, these tests are also meant to pick up on fields that
> aren't supposed to be exposed, and make sure that ovs-ofctl doesn't
> try to pass them on. That said, if the field will be allowed after the
> next patch then I don't object.

What was the actual bug that was discovered?

The problem here wasn't so much really that field should haven't been
exposed - that part was working correctly. The issue was that the
actual values should have been restricted by the parser. I can change
it so that the test remains in this patch but only tests tun_flags(0)
instead of key|csum. The next patch would then delete the test, since
at that point the field is allowed.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [RFC PATCH v2 3/7] flow: Always inline miniflows.

[Alin Serdean]

Hi Jarno,

This is breaking the windows build 
(https://ci.appveyor.com/project/blp/ovs/build/1.0.459):
lib/classifier.c(1355) : error C2229: struct '' has an illegal 
zero-sized array.

The problem is the following 
(https://msdn.microsoft.com/en-us/library/0scy7z2d.aspx ):
https://github.com/openvswitch/ovs/commit/8fd4792403254f868398a89fb5625830ee5a08eb#diff-b87c023d7e733f983131f231a672e71eR389

Could you please take a look into it?

Thanks,
Alin.

-Mesaj original-
De la: dev [mailto:dev-boun...@openvswitch.org] În numele Jarno Rajahalme
Trimis: Thursday, July 16, 2015 1:59 AM
Către: Ben Pfaff
Cc: dev@openvswitch.org
Subiect: Re: [ovs-dev] [RFC PATCH v2 3/7] flow: Always inline miniflows.

Thanks for the review, pushed to master.

  Jarno

> On Jul 15, 2015, at 3:05 PM, Ben Pfaff  wrote:
> 
> On Wed, Jul 15, 2015 at 01:11:54PM -0700, Jarno Rajahalme wrote:
>> 
>>> On Jul 14, 2015, at 4:50 PM, Ben Pfaff  wrote:
>>> 
>>> On Fri, Jul 10, 2015 at 10:35:21AM -0700, Jarno Rajahalme wrote:
 Now that performance critical code already inlines miniflows and 
 minimasks, we can simplify struct miniflow by always dynamically 
 allocating miniflows and minimasks to the correct size.  This 
 changes the struct minimatch to always contain pointers to its 
 miniflow and minimask.
 
 Signed-off-by: Jarno Rajahalme 
>>> 
>>> I like simplifying things so that two possibilities become one.  
>>> It's nice.
>>> 
>>> I see two possible ways to simplify this, each with its own tradeoffs:
>>> 
>>>  1. The way that you did it, which means that one must always
>>> either dynamically allocate struct miniflow to a proper size
>>> or ensure that there's enough space for the maximum size flow
>>> immediately following struct miniflow.  
>>> 
>>>  2. Change struct miniflow to:
>>> 
>>>   struct miniflow {
>>>   uint64_t map;
>>>   uint64_t *values;
>>>   };
>>> 
>>> With this strategy, there's no need to dynamically allocate
>>> struct miniflow itself, so it can be directly embedded in
>>> larger structures instead of via pointer.  The usual time cost
>>> of accessing miniflow data is again one pointer dereference
>>> (of 'values').
>>> 
>>> This can be kind of nice because you're not fighting with the
>>> compiler to put data in the right place.
>>> 
>>> Either way, the usual time cost of accessing miniflow data is one 
>>> pointer dereference (in case #1, from whatever contains the 
>>> miniflow; in case #2, dereference of 'values'), and the usual space 
>>> cost of embedding a miniflow is one pointer (although in case #1 the 
>>> pointer is in what contains the miniflow and in case #2 the pointer 
>>> is in the miniflow itself).
>>> 
>>> Anyway, I wanted to make sure that you considered both of these 
>>> solutions and decided that #1 was better.  If so:
>>> 
>> 
>> I agree that case #2 is syntactically nicer in the generic case, 
>> where one pointer dereference is always needed. However, we already 
>> embed miniflows to other data structures (flow keys and masks in 
>> classifier and userspace datapath), where avoid the dereference and 
>> hence make accessing the miniflow faster. With case #2 we’d need to 
>> define another “struct inline_miniflow” (basically the case #1) to 
>> avoid the dereference. IMO it is better to use the case #1 everywhere 
>> than defining two different miniflow structures.
> 
> OK, I'm glad that you considered the possibility then.

___
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 v4] datapath-windows: Output a packet to two or more VXLAN ports

[Alin Serdean]

If we have a flow rule in the following form:
 actions=strip_vlan,set_tunnel:0x3e9,15,16,17 (Where port 15, 16 and 17 are
 VXLAN tunnels with different tunnelling information)

A packet which will hit that specific flow will only be sent out encapsulated
with the first tunnelling information.

This patch saves the initial packet vport, source port and NIC index for
further use of the currently implemented pipeline and ignores the latter if it
is the last tunnelling port.

Signed-off-by: Alin Gabriel Serdean 
---
This patch should also be applied on 2.4
v4 Relax condition when saving ports
v3 Fix formatting
v2 Address comments
---
 datapath-windows/ovsext/Actions.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index c8de7c5..c824e71 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -975,6 +975,9 @@ OvsOutputBeforeSetAction(OvsForwardingContext *ovsFwdCtx)
ovsFwdCtx->tunnelTxNic != NULL || ovsFwdCtx->tunnelRxNic != NULL);
 
 /* Send the original packet out */
+UINT32 tempVportNo = ovsFwdCtx->srcVportNo;
+UINT32 tempSourcePortId = ovsFwdCtx->fwdDetail->SourcePortId;
+UINT32 tempNicIndex = ovsFwdCtx->fwdDetail->SourceNicIndex;
 status = OvsOutputForwardingCtx(ovsFwdCtx);
 ASSERT(ovsFwdCtx->curNbl == NULL);
 ASSERT(ovsFwdCtx->destPortsSizeOut == 0);
@@ -996,6 +999,9 @@ OvsOutputBeforeSetAction(OvsForwardingContext *ovsFwdCtx)
   
NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(newNbl),
   ovsFwdCtx->completionList,
   &ovsFwdCtx->layers, FALSE);
+ovsFwdCtx->srcVportNo = tempVportNo;
+ovsFwdCtx->fwdDetail->SourcePortId = tempSourcePortId;
+ovsFwdCtx->fwdDetail->SourceNicIndex = tempNicIndex;
 }
 
 return status;
-- 
1.9.5.msysgit.0
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] flow: Factor out flag parsing and formatting routines.

[Jesse Gross]

On Wed, Jul 15, 2015 at 5:41 PM, Jesse Gross  wrote:
> On Wed, Jul 15, 2015 at 5:31 PM, Joe Stringer  wrote:
>> On 15 July 2015 at 15:53, Jesse Gross  wrote:
>>> On Wed, Jul 15, 2015 at 11:35 AM, Ben Pfaff  wrote:
 On Mon, Jul 13, 2015 at 02:53:56PM -0700, Jesse Gross wrote:
> There are several implementations of functions that parse/format
> flags and their binary representation. This factors them out into
> common routines. In addition to reducing code, it also makes things
> more consistent across different parts of OVS.
>
> Signed-off-by: Jesse Gross 

 Thanks, I like reducing code size!

 This patch deletes a test and part of a test in ovs-ofctl.at.  Why?
>>>
>>> I'm not sure that these tests really make sense and arguably the
>>> behavior there were checking wasn't even right in the first place. In
>>> theory they are verifying that the correct OpenFlow version is
>>> selected based on the fields provided (in this case the correct
>>> version is "none") but it is being done using names that weren't
>>> supposed to be public, though the old parsing code allowed them to
>>> leak through. The new parsing code is enforcing the same invariants as
>>> before but more carefully and now rejects these commands as a parse
>>> error before it even gets to the OpenFlow layer that is supposed to be
>>> exercised. The next patch makes this field valid anyways and verifies
>>> the correct OpenFlow behavior, so it didn't seem like it made sense to
>>> keep the test around.
>>
>> Hmm, interesting. One of the bugs found by STACK recently also changes
>> the output of this test -- that bug was long-lived, in
>> parse_ofp_str__(). I plan to send the patches soonish. As I
>> understand, these tests are also meant to pick up on fields that
>> aren't supposed to be exposed, and make sure that ovs-ofctl doesn't
>> try to pass them on. That said, if the field will be allowed after the
>> next patch then I don't object.
>
> What was the actual bug that was discovered?
>
> The problem here wasn't so much really that field should haven't been
> exposed - that part was working correctly. The issue was that the
> actual values should have been restricted by the parser. I can change
> it so that the test remains in this patch but only tests tun_flags(0)
> instead of key|csum. The next patch would then delete the test, since
> at that point the field is allowed.

Actually, I realized that with the current version of the patch these
flags are still accepted and it's not until the next patch that we
clamp down on non-public values. Therefore, I restored the test to its
original form to make this a pure refactoring patch. The net result is
still the same after the next patch though.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] IS MSTP enabled on OVS 2.3.9

[ravali.burra]

Hi Justin

Thanks for reply.

Yes I want to implement the code of MSTP in OVS. Could you please help me in 
how to proceed further with the design and implementation of MSTP in OVS.

Thanks & Regards,
Ravali

-Original Message-
From: Justin Pettit [mailto:jpet...@nicira.com]
Sent: 16 July 2015 03:02
To: Ravali Burra (WT01 - Digital Marketing)
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] IS MSTP enabled on OVS 2.3.9


> On Jul 14, 2015, at 5:13 AM, ravali.bu...@wipro.com wrote:
>
> Hi Team,
>
> I am using ovs-master code Version 2.3.9 for my experiments.
>
> Currently what I understood from ovs-vsctl  man page we are having commands 
> for the configuring  RSTP.
>
> Similar way I want  to configure MSTP in OVS  but I have not  seen any 
> supported commands for configuration of MSTP from the interface perspective.
>
>
>
> Can you please suggest me in bringing up with the design and implementation 
> of configuration for bridge and interface support for MSTP (ovs-vsctl) and 
> also datapath support.

MSTP isn't supported by OVS.  Are you asking how you could implement it 
yourself?  If so, I'd look at the code that implements RSTP for inspiration.

--Justin


The information contained in this electronic message and any attachments to 
this message are intended for the exclusive use of the addressee(s) and may 
contain proprietary, confidential or privileged information. If you are not the 
intended recipient, you should not disseminate, distribute or copy this e-mail. 
Please notify the sender immediately and destroy all copies of this message and 
any attachments. WARNING: Computer viruses can be transmitted via email. The 
recipient should check this email and any attachments for the presence of 
viruses. The company accepts no liability for any damage caused by any virus 
transmitted by this email. www.wipro.com
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [ovn-controller-vtep V3 2/7] ovn-sb: Remove the "Gateway" table from the ovn-sb schema.

[Alex Wang]

On Wed, Jul 15, 2015 at 1:10 PM, Ben Pfaff  wrote:

> On Mon, Jul 13, 2015 at 08:22:38PM -0700, Alex Wang wrote:
> > In a gateway like the VTEP L2 gateway, physical vlans belonging to
> > the same logical network form a "logical switch".  Each logical switch
> > has a dedicated tunnel key and will keep records of all MACs learned
> > from the owned vlans.  So user can just send packet to a "logical
> > switch" and the gateway will figure out the output port and vlan tag
> > automatically.
> >
> > Therefore, it is not really necessary to keep record of the vlan map
> > for each gateway physical port in the OVN_Southbound database using
> > "gateway_ports".
> >
> > Thusly, this commit removes the "Gateway" table from the OVN_Southbound
> > database.  In the "Chassis" table, the "gateway_ports" column is replaced
> > by "logical_switches" column which maps the logical switch name in the
> > gateway to a logical port name that exists in the OVN_Northbound
> database's
> > "Logical_Port" table.
> >
> > Signed-off-by: Alex Wang 
> >
> > ---
> > ->V3:
> > - Realize that the Gateway table is not needed.
>
> I think that the documentation can be improved.
>
> I ended up with this:
>
> 
>   
> A gateway is a chassis that forwards traffic between the
> OVN-managed part of a logical network and a physical VLAN,
> extending a
> tunnel-based logical network into a physical network.  Gateways are
> typically dedicated nodes that do not host VMs.
>   
>
>   
> Maps from the name of a logical switch on the gateway to a logical
> port
> name.  The logical port name must be unique; one way to do this is
> by
> concatenating the chassis name and the logical switch name.  User
> needs
> to create a same named logical port in the OVN_Northbound
> database's
> Logical_Port table.
>   
> 
>
> but I still have some issues with it.  First, I deleted this sentence
> because I didn't understand it, but if you can explain then maybe
> rephrasing it would be a better option:
>
> Physical VLANs belonging to the one logical network form a
> logical switch in the gateway.


>
Second, the documentation says that the "user" needs to create the
> logical port.  I'm not sure who the user is in this case.
>

Here I'm trying to give a definition of what is a 'logical switch' in vtep,
to help
readers understand why the column is named 'logical_switches'.  I think I'll
change the column name to "vtep_logical_switches" and so maybe readers
could refer to the vtep schema manual.

For the second point, the 'user' should be the CMS~

how about this:

"""

Maps from the name of a vtep logical switch on the gateway to a logical port
name.  The logical port name must be unique; one way to do this is by
concatenating the chassis name and the logical switch name.  To include
one vtep logical switch to an OVN logical network, a same named logical
port should be created in the Logical_Port table and be attached to some
Logical_Switch in the OVN_Northbound database.

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

Re: [ovs-dev] [ovn-controller-vtep V3 2/7] ovn-sb: Remove the "Gateway" table from the ovn-sb schema.

[Alex Wang]

On Wed, Jul 15, 2015 at 1:33 PM, Russell Bryant  wrote:

> On 07/15/2015 03:15 PM, Russell Bryant wrote:
> > On 07/15/2015 03:09 PM, Alex Wang wrote:
> >> Also, what do you think about my proposal for using "type" and
> "options"
> >> fields on logical ports in OVN_Northbound instead of a special port
> >> name?  If we went this route, you don't need "logical_switches" to
> be a
> >> smap, it can just be a list of logical switch names.  Creating a
> >> connection in OVN northbound would be something like:
> >>
> >>   Logical_Port
> >> name: 
> >> type: vtep
> >> options:
> >>vtep_physical_switch: <...>
> >>vtep_logical_switch: <...>
> >>
> >>
> >>
> >> One thing I see missing is that: multiple vtep physical switches
> >> (supported by the same vtep db) could connect to the same vtep
> >> logical switch...   In that case, logical switch name can no longer
> >> uniquely identify a connection of 'a vtep physical switch to a vtep
> >> logical switch'...  That's why I use the combination of
> >> pswitch_name+lswitch_name.
> >>
> >> With you suggestion, first, the logical port name in ovn-nb can be
> >> duplicated. And if that is allowed, we need to pass the "options"
> >> down to ovn-sb Binding table to uniquely binding the vtep lport to
> >> vtep chassis.
> >
> > Yes, I was thinking that this information would have to be added to the
> > Binding table in OVN_Southbound as well so that you still have the
> > pswitch+lswitch info, just in a little bit more structured way.
> >
>
> Also, I've got some patches in progress that implements this.  It's a
> part of a series I'm working on, but I could split it out separately if
> you think it makes sense and want to rebase on them.
>
>

Hey Russell,

I'd like to see and try out your change.  At the same time, I do want to
have
my vtep controller code reviewed.  I think I'll first adopt your suggestion
and
rename the column to "vtep_logical_switches" and repost the series to have
it
reviewed.

With you change, I think I only need to change the binding module a bit to
adopt the new naming format.  The main logic (especially what is in pipeline
module) will remain intact~ ;D

Thanks,
Alex Wang,


> --
> Russell Bryant
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 1/4] ofp-parse: Fix typo in consistency check.

[Joe Stringer]

This check in parse_ofp_str__() attempted to detect inconsistencies
between matches and actions, or inconsistencies within the actions. In
this case, ofpacts_check() would effectively zero the "usable_protocols"
and return 0 (ie, OK). However, when checking the return parameter, it
checks the pointer rather than the value.

In practice, this seems to only come up for fields which are used
internally in OVS and not exposed for matching from the controller, like
tunnel flags or skb_priority.

Found by MIT STACK analyzer.

Signed-off-by: Joe Stringer 
---
 lib/ofp-parse.c| 2 +-
 tests/ovs-ofctl.at | 8 
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 9e88d6d..8cdda50 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -477,7 +477,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, 
char *string,
 
 err = ofpacts_check(ofpacts.data, ofpacts.size, &fm->match.flow,
 OFPP_MAX, fm->table_id, 255, usable_protocols);
-if (!err && !usable_protocols) {
+if (!err && !*usable_protocols) {
 err = OFPERR_OFPBAC_MATCH_INCONSISTENT;
 }
 if (err) {
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index 28bca14..9d6c571 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -127,9 +127,9 @@ do
 echo "### test case: '$1' should have usable protocols '$2'"
 if test "$2" = none; then
   AT_CHECK([ovs-ofctl parse-flow "$1,actions=drop"], [1],
-   [usable protocols: none
+   [dnl
 ],
-   [ovs-ofctl: no usable protocol
+   [ovs-ofctl: actions are invalid with specified match 
(OFPBAC_MATCH_INCONSISTENT)
 ])
 else
   AT_CHECK_UNQUOTED([ovs-ofctl parse-flow "$1,actions=drop" | sed 1q], [0],
@@ -309,7 +309,7 @@ 
tun_id=0x12345678/0x,tun_src=1.1.1.1,tun_dst=2.2.2.2,tun
 ]])
 
 AT_CHECK([ovs-ofctl parse-flows flows.txt
-], [1], [usable protocols: none
+], [1], [dnl
 ], [stderr])
 
 AT_CLEANUP
@@ -321,7 +321,7 @@ skb_priority=0x12341234,tcp,tp_src=123,actions=flood
 ]])
 
 AT_CHECK([ovs-ofctl parse-flows flows.txt
-], [1], [usable protocols: none
+], [1], [dnl
 ], [stderr])
 
 AT_CLEANUP
-- 
2.1.4

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

[ovs-dev] [PATCH 2/4] vtep-ctl: Remove extraneous NULL pointer check.

[Joe Stringer]

OVS will exit if the allocations in this function fail, so this check is
pointless.

Found by MIT STACK analyzer.

Signed-off-by: Joe Stringer 
---
 vtep/vtep-ctl.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/vtep/vtep-ctl.c b/vtep/vtep-ctl.c
index f065fc9..8dfd3ad 100644
--- a/vtep/vtep-ctl.c
+++ b/vtep/vtep-ctl.c
@@ -495,7 +495,7 @@ del_cached_port(struct vtep_ctl_context *vtepctl_ctx,
 free(port);
 }
 
-static struct vtep_ctl_pswitch *
+static void
 add_pswitch_to_cache(struct vtep_ctl_context *vtepctl_ctx,
  struct vteprec_physical_switch *ps_cfg)
 {
@@ -504,7 +504,6 @@ add_pswitch_to_cache(struct vtep_ctl_context *vtepctl_ctx,
 ps->name = xstrdup(ps_cfg->name);
 list_init(&ps->ports);
 shash_add(&vtepctl_ctx->pswitches, ps->name, ps);
-return ps;
 }
 
 static void
@@ -837,7 +836,6 @@ vtep_ctl_context_populate_cache(struct ctl_context *ctx)
 sset_init(&ports);
 for (i = 0; i < vtep_global->n_switches; i++) {
 struct vteprec_physical_switch *ps_cfg = vtep_global->switches[i];
-struct vtep_ctl_pswitch *ps;
 size_t j;
 
 if (!sset_add(&pswitches, ps_cfg->name)) {
@@ -845,10 +843,7 @@ vtep_ctl_context_populate_cache(struct ctl_context *ctx)
   ps_cfg->name);
 continue;
 }
-ps = add_pswitch_to_cache(vtepctl_ctx, ps_cfg);
-if (!ps) {
-continue;
-}
+add_pswitch_to_cache(vtepctl_ctx, ps_cfg);
 
 for (j = 0; j < ps_cfg->n_ports; j++) {
 struct vteprec_physical_port *port_cfg = ps_cfg->ports[j];
-- 
2.1.4

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

[ovs-dev] [PATCH 4/4] ovn: Fix extra token detection.

[Joe Stringer]

This code attempts to first check whether another error was detected for
the string it is parsing, then if it's not at the end of the tokens,
report an error. However, 'errorp' is always a valid pointer to a
'char *', so the first check in this statement always evaluates false.

Furthermore, this behaviour may be optimised out by modern compilers
due to the prior dereference in expr_parse(). Fix this to check the
actual value of *errorp.

Found by MIT STACK analyzer.

Signed-off-by: Joe Stringer 
---
 ovn/lib/expr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index c81c453..510a15e 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -1044,7 +1044,7 @@ expr_parse_string(const char *s, const struct shash 
*symtab, char **errorp)
 lexer_init(&lexer, s);
 lexer_get(&lexer);
 expr = expr_parse(&lexer, symtab, errorp);
-if (!errorp && lexer.token.type != LEX_T_END) {
+if (!*errorp && lexer.token.type != LEX_T_END) {
 *errorp = xstrdup("Extra tokens at end of input.");
 expr_destroy(expr);
 expr = NULL;
-- 
2.1.4

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

[ovs-dev] [PATCH 3/4] ovs-vsctl: Remove redundant checks.

[Joe Stringer]

In several places, "br" is dereferenced just before a check such as
"if (br ...)". These checks may be optimised out, and they provide no
additional safety, so this patch removes them.

Found by MIT STACK analyzer.

Signed-off-by: Joe Stringer 
---
 utilities/ovs-vsctl.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index 8d62d54..c71398e 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -740,9 +740,6 @@ vsctl_context_populate_cache(struct ctl_context *ctx)
 continue;
 }
 br = add_bridge_to_cache(vsctl_ctx, br_cfg, br_cfg->name, NULL, 0);
-if (!br) {
-continue;
-}
 
 for (j = 0; j < br_cfg->n_ports; j++) {
 struct ovsrec_port *port_cfg = br_cfg->ports[j];
@@ -2137,7 +2134,7 @@ cmd_add_aa_mapping(struct ctl_context *ctx)
 br = br->parent;
 }
 
-if (br && br->br_cfg) {
+if (br->br_cfg) {
 if (!br->br_cfg->auto_attach) {
 struct ovsrec_autoattach *aa = ovsrec_autoattach_insert(ctx->txn);
 ovsrec_bridge_set_auto_attach(br->br_cfg, aa);
@@ -2197,7 +2194,7 @@ cmd_del_aa_mapping(struct ctl_context *ctx)
 br = br->parent;
 }
 
-if (br && br->br_cfg && br->br_cfg->auto_attach &&
+if (br->br_cfg && br->br_cfg->auto_attach &&
 br->br_cfg->auto_attach->key_mappings &&
 br->br_cfg->auto_attach->value_mappings) {
 size_t i;
@@ -2248,7 +2245,7 @@ cmd_get_aa_mapping(struct ctl_context *ctx)
 
 verify_auto_attach(br->br_cfg);
 
-if (br && br->br_cfg && br->br_cfg->auto_attach &&
+if (br->br_cfg && br->br_cfg->auto_attach &&
 br->br_cfg->auto_attach->key_mappings &&
 br->br_cfg->auto_attach->value_mappings) {
 size_t i;
-- 
2.1.4

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

[ovs-dev] [PATCH 0/4] Fixes to undefined behaviour.

[Joe Stringer]

I was recently introduced to the MIT "STACK" code analyzer[1]. STACK analyses
the intermediate representation of LLVM-compiled code, and attempts to detect
cases where the compiler may optimise some code unexpectedly, usually due to
undefined behaviour. This series addresses the issues found by this tool,
outside the testsuite.

One example is the "anti-simplify" test. With code like the following:

void foo(int *x)
{
printf("%d\n", *x);
if (!x) {
printf("x is evaluated to false!\n");
}
}

The first printf() statement dereferences 'x'. At this point, either this call
succeeds, or the program will crash (Segmentation Fault). As such, subsequent
checks against 'x' must succeed. One could replace "if (!x)" with "if (false)"
and not change the behaviour of the program. The compiler may now entirely
remove the if block as an optimization. In practice, examples are likely to be
more complicated than this, but this kind of bug was found in a couple of
places within Open vSwitch.

In practice, to get up and running, the INSTALL and README files are fairly
clear. One needs to download LLVM and compile from scratch, so make sure you
have as much as 15GB free space. Once LLVM and STACK are compiled and set up
in your path, it's as easy as:

$ stack-build ./configure
$ stack-build make

This generates various *.ll and *.ll.out files within the build directory.
To analyze these files:

$ poptck
$ less pstack.txt

I didn't notice any difference to the compile time for the OVS codebase -
~3m30s with a single core. Analysis took just shy of 1m. The README has some
tips on interpreting the output. After pinpointing a particular problem and
applying a fix, I deleted the *.ll.out files, re-compiled and re-analysed the
codebase to verify that the fix has removed the problem.

[1]: https://github.com/xiw/stack

Joe Stringer (4):
  ofp-parse: Fix typo in consistency check.
  vtep-ctl: Remove extraneous NULL pointer check.
  ovs-vsctl: Remove redundant checks.
  ovn: Fix extra token detection.

 lib/ofp-parse.c   | 2 +-
 ovn/lib/expr.c| 2 +-
 tests/ovs-ofctl.at| 8 
 utilities/ovs-vsctl.c | 9 +++--
 vtep/vtep-ctl.c   | 9 ++---
 5 files changed, 11 insertions(+), 19 deletions(-)

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

Re: [ovs-dev] [PATCH 1/2] flow: Factor out flag parsing and formatting routines.

[Joe Stringer]

On 15 July 2015 at 17:41, Jesse Gross  wrote:
> On Wed, Jul 15, 2015 at 5:31 PM, Joe Stringer  wrote:
>> On 15 July 2015 at 15:53, Jesse Gross  wrote:
>>> On Wed, Jul 15, 2015 at 11:35 AM, Ben Pfaff  wrote:
 On Mon, Jul 13, 2015 at 02:53:56PM -0700, Jesse Gross wrote:
> There are several implementations of functions that parse/format
> flags and their binary representation. This factors them out into
> common routines. In addition to reducing code, it also makes things
> more consistent across different parts of OVS.
>
> Signed-off-by: Jesse Gross 

 Thanks, I like reducing code size!

 This patch deletes a test and part of a test in ovs-ofctl.at.  Why?
>>>
>>> I'm not sure that these tests really make sense and arguably the
>>> behavior there were checking wasn't even right in the first place. In
>>> theory they are verifying that the correct OpenFlow version is
>>> selected based on the fields provided (in this case the correct
>>> version is "none") but it is being done using names that weren't
>>> supposed to be public, though the old parsing code allowed them to
>>> leak through. The new parsing code is enforcing the same invariants as
>>> before but more carefully and now rejects these commands as a parse
>>> error before it even gets to the OpenFlow layer that is supposed to be
>>> exercised. The next patch makes this field valid anyways and verifies
>>> the correct OpenFlow behavior, so it didn't seem like it made sense to
>>> keep the test around.
>>
>> Hmm, interesting. One of the bugs found by STACK recently also changes
>> the output of this test -- that bug was long-lived, in
>> parse_ofp_str__(). I plan to send the patches soonish. As I
>> understand, these tests are also meant to pick up on fields that
>> aren't supposed to be exposed, and make sure that ovs-ofctl doesn't
>> try to pass them on. That said, if the field will be allowed after the
>> next patch then I don't object.
>
> What was the actual bug that was discovered?
>
> The problem here wasn't so much really that field should haven't been
> exposed - that part was working correctly. The issue was that the
> actual values should have been restricted by the parser. I can change
> it so that the test remains in this patch but only tests tun_flags(0)
> instead of key|csum. The next patch would then delete the test, since
> at that point the field is allowed.

I CC'd you on the patch:
http://openvswitch.org/pipermail/dev/2015-July/057560.html

As far as I understand, it's an invalid match which the parser should
have picked up on and returned OFPERR_OFPBAC_MATCH_INCONSISTENT.
Instead it parsed them and just said that no protocol can represent
it.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [ovn-controller-vtep V3 3/7] ovn-sbctl: Add ovn-sbctl.

[Alex Wang]

On Wed, Jul 15, 2015 at 3:48 PM, Ben Pfaff  wrote:

> On Mon, Jul 13, 2015 at 08:22:39PM -0700, Alex Wang wrote:
> > This commit adds ovn-sbctl to ovn family by using the db-ctl-base
> > library.
> >
> > As a side effect, we move the ovn-nbctl/ovn-sbctl related files
> > into ovn/utilities.
> >
> > Signed-off-by: Alex Wang 
> > Acked-by: Ben Pfaff 
> >
> > ---
> > V2->V3:
> > - rebase to base.
> > - adopt Russell's review comments.
> > - move ovn-nbctl/sbctl related files into ovn/utilities.
> >
> > PATCH->V2:
> > - change command add/del-ch to add/del-chassis.
> > - refine the manual based on comments from Ben.
>
> I get a test failure in a new test due to:
> ovn-sbctl: unknown command 'add-chassis'; use --help for help
>
>

I tried it on my 12.04 vm and another rhel7 vm...  Could not run into this
issue.




> I think that the manual should say that it is really for advanced
> debugging and troubleshooting and that it should not be used in normal
> operation (I guess ovn-nbctl should say the same thing; I thought it did
> before but I don't see it now).
>
>
Will make it explicit, thx


> If you fix those then:
> Acked-by: Ben Pfaff 
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] ovs-ofctl mod-table commands supporting OF1.4 Eviction and Vacancy-Events

[Saloni Jain]

Hi Ben,

According Open vSwitch Manual:
"ovs-vswitchd can perform all configured bridging and switching locally, or  it 
can be configured to communicate with one or more external Open‐Flow 
controllers.  The switch is typically configured to connect  to  a "primary  
controller"  that  takes  charge  of the bridge's flow table to implement a 
network policy.  In addition, the switch can be  configured to listen to 
connections from "service controllers".  Service controllers are typically used 
for occasional support and  maintenance,  e.g.  with ovs-ofctl."

So what I think that instead of "ovs-ofctl table-mod" to use an 
OFPMP_TABLE_DESC request to obtain the current configuration, then modify it 
according to the user's request, "ovs-vswitchd" should query and store the 
current table-configuration and modify it with user's request, as in this case 
both  "primary controller" and "service controller" will be taken into 
consideration.

If the query for the current configuration is sent from "ovs-ofctl", then only 
service controller will take the existing configuration into consideration and 
in that case table-mod message from other primary/open-flow controllers will 
overwrite/affect other properties that user didn't request. In other words, say 
for example there are 2 openflow controllers connected to switch, first 
controller through mod-table set Eviction for a table on switch and second 
controller sends the table-mod request for vacancy-events for the same table, 
in that case, the previously configured Eviction mechanism will get off and 
only vacancy events will be set for that table, which is not the desired action.

So I think it will be good that "ovs-vswitchd" should query and store the 
current table-configuration and modify it with controllers request
Please correct me if my understanding is wrong.

Thanks and Regards,
Saloni Jain
Tata Consultancy Services
Mailto: saloni.j...@tcs.com
Website: http://www.tcs.com

Experience certainty.   IT Services
Business Solutions
Consulting



-Ben Pfaff  wrote: -
To: Saloni Jain 
From: Ben Pfaff 
Date: 07/16/2015 05:50AM
Cc: dev@openvswitch.org, Deepankar Gupta , Partha 
Datta 
Subject: Re: [ovs-dev] ovs-ofctl mod-table commands supporting OF1.4 Eviction 
and Vacancy-Events

On Tue, Jul 14, 2015 at 05:51:50PM +0530, Saloni Jain wrote:
> In addition to my previous response, please suggest if "vswitchd"
> should query and get the current stored table-configurartion
> using query_tables_desc() and then modify table-config properties
> according to user's request.

I don't understand the question.  Do you mean ovs-ofctl instead of
vswitchd?
=-=-=
Notice: The information contained in this e-mail
message and/or attachments to it may contain 
confidential or privileged information. If you are 
not the intended recipient, any dissemination, use, 
review, distribution, printing or copying of the 
information contained in this e-mail message 
and/or attachments to it are strictly prohibited. If 
you have received this communication in error, 
please notify us by reply e-mail or telephone and 
immediately and permanently delete the message 
and any attachments. Thank you


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