Re: [ovs-dev] [OVN Patch v8 3/3] northd: Restore parallel build with dp_groups

[Anton Ivanov]

I need to have a look.

I use the ovn-heater end-to-end test, that was showing a substantial 
improvement.


What are you running this on?

A.

On 30/09/2021 00:56, Han Zhou wrote:



On Wed, Sep 15, 2021 at 5:45 AM > wrote:

>
> From: Anton Ivanov >

>
> Restore parallel build with dp groups using rwlock instead
> of per row locking as an underlying mechanism.
>
> This provides improvement ~ 10% end-to-end on ovn-heater
> under virutalization despite awakening some qemu gremlin
> which makes qemu climb to silly CPU usage. The gain on
> bare metal is likely to be higher.
>
Hi Anton,

I am trying to see the benefit of parallel_build, but encountered 
unexpected performance result when running the perf tests with command:

 make check-perf TESTSUITEFLAGS="--rebuild"

It shows significantly worse performance than without parallel_build. 
For dp_group = no cases, it is better, but still ~30% slower than 
without parallel_build. I have 24 cores, but each thread is not 
consuming much CPU except the main thread. I also tried hardcode the 
number of thread to just 4, which end up with slightly better results, 
but still far behind "without parallel_build".


             no parallel                                   | parallel  
(24 pool threads)                  | parallel with (4 pool threads)

                                   |                                   |
    1: ovn-northd basic scale test -- 200 Hypervisors, 200 |    1: 
ovn-northd basic scale test -- 200 Hypervisors, 200 |    1: ovn-northd 
basic scale test -- 200 Hypervisors, 200
    ---                                                    |    ---   
                                                 |    ---
    Maximum (NB in msec): 1058                             |   
 Maximum (NB in msec): 4269                             |    Maximum 
(NB in msec): 4097
    Average (NB in msec): 836.941167                       |   
 Average (NB in msec): 3697.253931                      |    Average 
(NB in msec): 3498.311525
    Maximum (SB in msec): 30                               |   
 Maximum (SB in msec): 30                               |    Maximum 
(SB in msec): 28
    Average (SB in msec): 25.934011                        |   
 Average (SB in msec): 26.001840                        |    Average 
(SB in msec): 25.685091
    Maximum (northd-loop in msec): 1204                    |   
 Maximum (northd-loop in msec): 4379                    |    Maximum 
(northd-loop in msec): 4251
    Average (northd-loop in msec): 1005.330078             |   
 Average (northd-loop in msec): 4233.871504             |    Average 
(northd-loop in msec): 4022.774208
                                                           |           
                                                |
    2: ovn-northd basic scale test -- 200 Hypervisors, 200 |    2: 
ovn-northd basic scale test -- 200 Hypervisors, 200 |    2: ovn-northd 
basic scale test -- 200 Hypervisors, 200
    ---                                                    |    ---   
                                                 |    ---
    Maximum (NB in msec): 1124                             |   
 Maximum (NB in msec): 1480                             |    Maximum 
(NB in msec): 1331
    Average (NB in msec): 892.403405                       |   
 Average (NB in msec): 1206.189287                      |    Average 
(NB in msec): 1089.378455
    Maximum (SB in msec): 29                               |   
 Maximum (SB in msec): 31                               |    Maximum 
(SB in msec): 30
    Average (SB in msec): 26.922632                        |   
 Average (SB in msec): 26.636706                        |    Average 
(SB in msec): 25.657484
    Maximum (northd-loop in msec): 1275                    |   
 Maximum (northd-loop in msec): 1639                    |    Maximum 
(northd-loop in msec): 1495
    Average (northd-loop in msec): 1074.917873             |   
 Average (northd-loop in msec): 1458.152327             |    Average 
(northd-loop in msec): 1301.057201
                                                           |           
                                                |
    5: ovn-northd basic scale test -- 500 Hypervisors, 50 L|    5: 
ovn-northd basic scale test -- 500 Hypervisors, 50 L|    5: ovn-northd 
basic scale test -- 500 Hypervisors, 50
    ---                                                    |    ---   
                                                 |    ---
    Maximum (NB in msec): 768                              |   
 Maximum (NB in msec): 3086                             |    Maximum 
(NB in msec): 2876
    Average (NB in msec): 614.491938                       |   
 Average (NB in msec): 2681.688365                      |    Average 
(NB in msec): 2531.255444
    Maximum (SB in msec): 18                               |   
 Maximum (SB in msec): 17                               |    Maximum 
(SB 

Re: [ovs-dev] [PATCH] datapath-windows:adjust Offset when processing packet in POP_VLAN action

[Wilson Peng]

Alin,

Thanks for your comments, I have sent out the updated patch, please help check.
https://patchwork.ozlabs.org/project/openvswitch/patch/20210930045626.9250-1-pweis...@vmware.com/

Regards
Wilson

在 2021/9/29 23:21，“Alin-Gabriel Serdean” 写入:

Indeed. Thanks for pointing that out.

To be honest that looks like a bug.
We should return an error if the it is not a valid VLAN frame.

-Original Message-
From: Wilson Peng  
Sent: Wednesday, September 29, 2021 5:52 PM
To: Alin-Gabriel Serdean ; ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows:adjust Offset when 
processing packet in POP_VLAN action

Alin,

Original code diff is to avoid the case below(it will return 
NDIS_STATUS_SUCCESS before RtlMoveMemory in function OvsPopFieldInPacketBuf).

Regards
Wilson

In function OvsPopFieldInPacketBuf(..)
 if (!bufferData) {
EthHdr *ethHdr = (EthHdr *)bufferStart;
/* If the frame is not VLAN make it a no op */
if (ethHdr->Type != ETH_TYPE_802_1PQ_NBO) {
return NDIS_STATUS_SUCCESS;>  may exit here.
}
}

  RtlMoveMemory(bufferStart + shiftLength, bufferStart, shiftOffset);
  NdisAdvanceNetBufferDataStart(curNb, shiftLength, FALSE, NULL);

在 2021/9/29 21:55，“dev 代表 Alin-Gabriel 
Serdean” 写入:

From: aserd...@ovn.org

Thank you for raising awareness about this issue. You are right the
offsets are not being updated when a pop vlan action is hit, they are
updated only on pop_mpls.

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenvswitch%2Fovs%2Fblob%2Fmaster%2Fdatapath-windows%2Fovsext%2FActions.c%23L1173-L1174data=04%7C01%7Cpweisong%40vmware.com%7Ca65191ed9abd47b21a8608d9835cddfe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637685257162775283%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=oonWmImrDVicJ%2BBdGaYvmNDiUsueMXwOOjSn9MuFmBE%3Dreserved=0
Can you please update the offsets in the corresponding pop vlan
function. I.e.:
---
 datapath-windows/ovsext/Actions.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index e130c2f96..34aa5c5e4 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1141,11 +1141,21 @@ OvsPopVlanInPktBuf(OvsForwardingContext 
*ovsFwdCtx)
  * Declare a dummy vlanTag structure since we need to compute the 
size
  * of shiftLength. The NDIS one is a unionized structure.
  */
+NDIS_STATUS status;
+OVS_PACKET_HDR_INFO* layers = >layers;
 NDIS_PACKET_8021Q_INFO vlanTag = {0};
 UINT32 shiftLength = sizeof(vlanTag.TagHeader);
 UINT32 shiftOffset = sizeof(DL_EUI48) + sizeof(DL_EUI48);

-return OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength, 
NULL);
+status = OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, 
shiftLength,
+NULL);
+
+if (status == NDIS_STATUS_SUCCESS) {
+layers->l3Offset -= (UINT16) shiftLength;
+layers->l4Offset -= (UINT16) shiftLength;
+}
+
+return status;
 }


-- 
2.32.0

___
dev mailing list
d...@openvswitch.org

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-devdata=04%7C01%7Cpweisong%40vmware.com%7Ca65191ed9abd47b21a8608d9835cddfe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637685257162775283%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=29gFWHNVRPTixEkYAQeD6HevknOxi6%2BYerYFAUM3FTI%3Dreserved=0



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 1/1] datapath-windows:adjust Offset when processing packet in POP_VLAN action

[Wilson Peng]

From: wilsonpeng 

In one typical setup, on the Windows VM running OVS Windows Kernel, a Geneva
packet with 8021.q VLAN tag is received. Then it may do POP_VLAN action
processing in Actions.c, if the packet does not have Ieee8021QNetBufferListInfo
in the oob of the packet, it will be processed by function OvsPopVlanInPktBuf().
In the function it will go on remove VLAN header present in the nbl, but related
layers is never readjusted for the offset value at this moment. As a result, it
will cause function OvsValidateIPChecksum drop the packet.

Reported-at:https://github.com/openvswitch/ovs-issues/issues/225
Signed-off-by: wilsonpeng 
---
 datapath-windows/ovsext/Actions.c | 18 +++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index e130c2f96..59e70a5b5 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1112,9 +1112,9 @@ OvsPopFieldInPacketBuf(OvsForwardingContext *ovsFwdCtx,
  * should split the function and refactor. */
 if (!bufferData) {
 EthHdr *ethHdr = (EthHdr *)bufferStart;
-/* If the frame is not VLAN make it a no op */
 if (ethHdr->Type != ETH_TYPE_802_1PQ_NBO) {
-return NDIS_STATUS_SUCCESS;
+OVS_LOG_ERROR("Invalid ethHdr type %u, nbl %p", ethHdr->Type, 
ovsFwdCtx->curNbl);
+return NDIS_STATUS_INVALID_PACKET;
 }
 }
 RtlMoveMemory(bufferStart + shiftLength, bufferStart, shiftOffset);
@@ -1137,6 +1137,9 @@ OvsPopFieldInPacketBuf(OvsForwardingContext *ovsFwdCtx,
 static __inline NDIS_STATUS
 OvsPopVlanInPktBuf(OvsForwardingContext *ovsFwdCtx)
 {
+NDIS_STATUS status;
+OVS_PACKET_HDR_INFO* layers = >layers;
+
 /*
  * Declare a dummy vlanTag structure since we need to compute the size
  * of shiftLength. The NDIS one is a unionized structure.
@@ -1145,7 +1148,15 @@ OvsPopVlanInPktBuf(OvsForwardingContext *ovsFwdCtx)
 UINT32 shiftLength = sizeof(vlanTag.TagHeader);
 UINT32 shiftOffset = sizeof(DL_EUI48) + sizeof(DL_EUI48);
 
-return OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength, NULL);
+status = OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength,
+   NULL);
+
+if (status == NDIS_STATUS_SUCCESS) {
+layers->l3Offset -= (UINT16) shiftLength;
+layers->l4Offset -= (UINT16) shiftLength;
+}
+
+return status;
 }
 
 
@@ -2100,6 +2111,7 @@ OvsDoExecuteActions(POVS_SWITCH_CONTEXT switchContext,
  */
 status = OvsPopVlanInPktBuf();
 if (status != NDIS_STATUS_SUCCESS) {
+OVS_LOG_ERROR("OVS-pop vlan action failed status = %lu", 
status);
 dropReason = L"OVS-pop vlan action failed";
 goto dropit;
 }
-- 
2.17.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 0/6] Add support for ovs metering with tc offload

[Jianbo Liu via dev]

On Wed, 2021-09-29 at 13:37 +0200, Simon Horman wrote:
> On Thu, Sep 02, 2021 at 01:18:17PM +0300, Roi Dayan via dev wrote:
> > Hi,
> > 
> > This series is adding support for tc offloading of ovs metering.
> > The first 3 patches add lib support.
> > 4th patch adding tc police actions to reflect ovs metering.
> > 5th patch cleaning existing tc police actions on start to avoid
> > conflicts.
> > last patch is offloacing tc rules with the police actions.
> > 
> > Thanks,
> > Roi
> 
> Hi Roi,
> 
> I apologise for not seeing this earlier.
> 
> As I believe you are aware Corigine have been working on adding
> support for
> offloading TC actions independent of flows to the kernel. This is
> intended
> as a mechanism for TC offload of meters, by allowing police action
> instances to be shared between flows - meters can be used by
> different
> flows.
> 
> Am I correct in thinking that this patchset would be complementary to
> such support in TC?

Yes, it is. We are paying attention to your patchset for kernel. And it
could be glad to hear your comments on this patchset.

Thanks!
Jianbo

> 
> Kind regards,
> Simon

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v6] Encap & Decap actions for MPLS packet type.

[0-day Robot]

Bleep bloop.  Greetings Martin Varghese, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#332 FILE: lib/odp-util.c:2616:
 eth_type=0x%"SCNx16")%n", , , , , _type, )) {

Lines checked: 1277, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v6] Encap & Decap actions for MPLS packet type.

[Martin Varghese]

From: Martin Varghese 

The encap & decap actions are extended to support MPLS packet type.
Encap & decap actions adds and removes MPLS header at start of the
packet.

The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS
header between ethernet header and the IP header. Though this behaviour
is fine for L3 VPN where an IP packet is encapsulated inside a MPLS
tunnel, it does not suffice the L2 VPN requirements. In L2 VPN the
ethernet packets must be encapsulated inside MPLS tunnel.

In this change the encap & decap actions are extended to support MPLS
packet type. The encap & decap adds and removes MPLS header at the
start of packet as depicted below.

Encapsulation:

Actions - encap(mpls(ether_type=0x8847)),encap(ethernet)

Incoming packet -> | ETH | IP | Payload |

1 Actions -  encap(mpls(ether_type=0x8847)) [Datapath action - ADD_MPLS:0x8847]

Outgoing packet -> | MPLS | ETH | Payload|

2 Actions - encap(ethernet) [ Datapath action - push_eth ]

Outgoing packet -> | ETH | MPLS | ETH | Payload|

Decapsulation:

Incoming packet -> | ETH | MPLS | ETH | IP | Payload |

Actions - decap(),decap(packet_type(ns=0,type=0)

1 Actions -  decap() [Datapath action - pop_eth)

Outgoing packet -> | MPLS | ETH | IP | Payload|

2 Actions - decap(packet_type(ns=0,type=0) [Datapath action - POP_MPLS:0x6558]

Outgoing packet -> | ETH  | IP | Payload|

Signed-off-by: Martin Varghese 
---
Changes in v2:
   - Fixed the compilation error reported by bot.

Changes in v3:
   - Adapted the changes to allign with the kernel implementaion

Changes in v4:
   - Fixed the compilation error reported by bot.
   - Added SLOW_ACTION support for no datapath support.

Changes in v5:
   -  Code styling fixed.
   -  Given reference for packet_type field in documentation.
   -  Modified code to do recirc only for the last label.
   -  Cleaed l2,l3,l4 fields with encap.
   -  Fixed existing encap & decap tests to do set eth to outer header
   -  Added tests to verify Encap & Decap with legacy Push & Pop.
   -  Added xlate tests.
   -  Added seperate tests to inspect packet after encap & decap.
   -  Added tests for multiple mpls test cases.

Changes in v6:
   - Styling and variable naming fixed.
   - Tests added for slow path support.
   - Added comments in code.
   - Added odp test for add_mpls action.
   - Documentation added in ovs-actions.7.rst
 
 Documentation/ref/ovs-actions.7.rst   |  16 +-
 NEWS  |   2 +-
 .../linux/compat/include/linux/openvswitch.h  |  33 +-
 include/openvswitch/ofp-ed-props.h|  18 ++
 lib/dpif-netdev.c |   1 +
 lib/dpif.c|   1 +
 lib/odp-execute.c |  12 +
 lib/odp-util.c|  72 -
 lib/ofp-actions.c |   5 +
 lib/ofp-ed-props.c|  91 ++
 lib/packets.c |  45 +++
 lib/packets.h |   2 +
 ofproto/ofproto-dpif-ipfix.c  |   1 +
 ofproto/ofproto-dpif-sflow.c  |   1 +
 ofproto/ofproto-dpif-xlate.c  |  78 +
 ofproto/ofproto-dpif.c|  38 +++
 ofproto/ofproto-dpif.h|   4 +-
 tests/mpls-xlate.at   |  57 
 tests/odp.at  |   1 +
 tests/system-traffic.at   | 304 ++
 20 files changed, 765 insertions(+), 17 deletions(-)

diff --git a/Documentation/ref/ovs-actions.7.rst 
b/Documentation/ref/ovs-actions.7.rst
index 7224896df..d3b75a602 100644
--- a/Documentation/ref/ovs-actions.7.rst
+++ b/Documentation/ref/ovs-actions.7.rst
@@ -212,12 +212,13 @@ Unsupported packet type
   is not an L3 packet, and ``encap(nsh)`` raises this error if the current
   packet is not Ethernet, IPv4, IPv6, or NSH.
 
+  The ``decap`` action is supported only for packet types ethernet, NSH and
+  MPLS. Openvswitch raises this error for other packet types.
   When a ``decap`` action decapsulates a packet, Open vSwitch raises this error
   if it does not support the type of inner packet.  ``decap`` of an Ethernet
   header raises this error if a VLAN header is present, ``decap`` of a NSH
   packet raises this error if the NSH inner packet is not Ethernet, IPv4, IPv6,
-  or NSH, and ``decap`` of other types of packets is unsupported and also
-  raises this error.
+  or NSH.
 
   This error terminates packet processing, retaining any previous side effects
   (e.g. output actions).  When this error arises within the execution of a
@@ -995,6 +996,7 @@ The ``encap`` action
   | ``encap(nsh([md_type=``\ *md_type*\
  ``], [tlv(``\ *class*,\ *type*,\ *value*\ ``)]...))``
   | ``encap(ethernet)``
+  | ``encap(mpls(ether_type=``\ *ether_type*\)
 
 The ``encap`` action encapsulates a packet with a specified header.  It has

Re: [ovs-dev] [OVN Patch v8 3/3] northd: Restore parallel build with dp_groups

[Han Zhou]

On Wed, Sep 15, 2021 at 5:45 AM  wrote:
>
> From: Anton Ivanov 
>
> Restore parallel build with dp groups using rwlock instead
> of per row locking as an underlying mechanism.
>
> This provides improvement ~ 10% end-to-end on ovn-heater
> under virutalization despite awakening some qemu gremlin
> which makes qemu climb to silly CPU usage. The gain on
> bare metal is likely to be higher.
>
Hi Anton,

I am trying to see the benefit of parallel_build, but encountered
unexpected performance result when running the perf tests with command:
 make check-perf TESTSUITEFLAGS="--rebuild"

It shows significantly worse performance than without parallel_build. For
dp_group = no cases, it is better, but still ~30% slower than without
parallel_build. I have 24 cores, but each thread is not consuming much CPU
except the main thread. I also tried hardcode the number of thread to just
4, which end up with slightly better results, but still far behind "without
parallel_build".

 no parallel   |
parallel  (24 pool threads)  |parallel with (4 pool
threads)
   |
|
1: ovn-northd basic scale test -- 200 Hypervisors, 200 |1:
ovn-northd basic scale test -- 200 Hypervisors, 200 |1: ovn-northd
basic scale test -- 200 Hypervisors, 200
---|---
   |---
Maximum (NB in msec): 1058 |Maximum (NB
in msec): 4269 |Maximum (NB in msec): 4097

Average (NB in msec): 836.941167   |Average (NB
in msec): 3697.253931  |Average (NB in msec):
3498.311525
Maximum (SB in msec): 30   |Maximum (SB
in msec): 30   |Maximum (SB in msec): 28

Average (SB in msec): 25.934011|Average (SB
in msec): 26.001840|Average (SB in msec):
25.685091
Maximum (northd-loop in msec): 1204|Maximum
(northd-loop in msec): 4379|Maximum (northd-loop in
msec): 4251
Average (northd-loop in msec): 1005.330078 |Average
(northd-loop in msec): 4233.871504 |Average (northd-loop in
msec): 4022.774208
   |
|
2: ovn-northd basic scale test -- 200 Hypervisors, 200 |2:
ovn-northd basic scale test -- 200 Hypervisors, 200 |2: ovn-northd
basic scale test -- 200 Hypervisors, 200
---|---
   |---
Maximum (NB in msec): 1124 |Maximum (NB
in msec): 1480 |Maximum (NB in msec): 1331

Average (NB in msec): 892.403405   |Average (NB
in msec): 1206.189287  |Average (NB in msec):
1089.378455
Maximum (SB in msec): 29   |Maximum (SB
in msec): 31   |Maximum (SB in msec): 30

Average (SB in msec): 26.922632|Average (SB
in msec): 26.636706|Average (SB in msec):
25.657484
Maximum (northd-loop in msec): 1275|Maximum
(northd-loop in msec): 1639|Maximum (northd-loop in
msec): 1495
Average (northd-loop in msec): 1074.917873 |Average
(northd-loop in msec): 1458.152327 |Average (northd-loop in
msec): 1301.057201
   |
|
5: ovn-northd basic scale test -- 500 Hypervisors, 50 L|5:
ovn-northd basic scale test -- 500 Hypervisors, 50 L|5: ovn-northd
basic scale test -- 500 Hypervisors, 50
---|---
   |---
Maximum (NB in msec): 768  |Maximum (NB
in msec): 3086 |Maximum (NB in msec): 2876

Average (NB in msec): 614.491938   |Average (NB
in msec): 2681.688365  |Average (NB in msec):
2531.255444
Maximum (SB in msec): 18   |Maximum (SB
in msec): 17   |Maximum (SB in msec): 18

Average (SB in msec): 16.347526|Average (SB
in msec): 15.955263|Average (SB in msec):
16.278075
Maximum (northd-loop in msec): 889 |Maximum
(northd-loop in msec): 3247|Maximum (northd-loop in
msec): 

[ovs-dev] [PATCH] Documentation: Change the address in userspace-tunneling.rst

[Paolo Valerio]

Fixes: b438493e1b03 ("doc: Add DPDK to userspace tunneling guide")
Signed-off-by: Paolo Valerio 
---
 Documentation/howto/userspace-tunneling.rst |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/howto/userspace-tunneling.rst 
b/Documentation/howto/userspace-tunneling.rst
index 4e23b2e0c..e96b79985 100644
--- a/Documentation/howto/userspace-tunneling.rst
+++ b/Documentation/howto/userspace-tunneling.rst
@@ -175,7 +175,7 @@ If the tunnel route is missing, adding it now::
 
 $ ovs-appctl ovs/route/add 172.168.1.1/24 br-phy
 
-Repeat these steps if necessary for `host2`, but using ``192.168.1.1`` and
+Repeat these steps if necessary for `host2`, but using ``192.168.1.2`` and
 ``172.168.1.2`` for the VM and tunnel interface IP addresses, respectively.
 
 Testing

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn 2/7] northd: Introduce incremental processing for northd

[0-day Robot]

Bleep bloop.  Greetings Mark Gray, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


build:
libtool: link: gcc -std=gnu99 -Wstrict-prototypes -Wall -Wextra 
-Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum 
-Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes 
-Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers 
-fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -o 
controller/ovn-controller controller/bfd.o controller/binding.o 
controller/chassis.o controller/encaps.o controller/ha-chassis.o 
controller/if-status.o controller/ip-mcast.o controller/lflow.o 
controller/lflow-cache.o controller/lport.o controller/ofctrl.o 
controller/ofctrl-seqno.o controller/pinctrl.o controller/patch.o 
controller/ovn-controller.o controller/physical.o controller/mac-learn.o 
controller/local_data.o  lib/.libs/libovn.a 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib/.libs/libopenvswitch.a
 -lssl -lcrypto -lcap-ng -lpthread -lrt -lm -lunbound
depbase=`echo controller-vtep/binding.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
gcc -std=gnu99 -DHAVE_CONFIG_H -I.   -I ./include  -I ./include -I ./ovn -I 
./include -I ./lib -I ./lib -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include
 -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include
 -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib
 -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib
 -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR
-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat 
-Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast 
-Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes 
-Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror  -g 
-O2 -MT controller-vtep/binding.o -MD -MP -MF $depbase.Tpo -c -o 
controller-vtep/binding.o controller-vtep/binding.c &&\
mv -f $depbase.Tpo $depbase.Po
depbase=`echo controller-vtep/gateway.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
gcc -std=gnu99 -DHAVE_CONFIG_H -I.   -I ./include  -I ./include -I ./ovn -I 
./include -I ./lib -I ./lib -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include
 -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include
 -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib
 -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib
 -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR
-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat 
-Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast 
-Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes 
-Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror  -g 
-O2 -MT controller-vtep/gateway.o -MD -MP -MF $depbase.Tpo -c -o 
controller-vtep/gateway.o controller-vtep/gateway.c &&\
mv -f $depbase.Tpo $depbase.Po
depbase=`echo controller-vtep/ovn-controller-vtep.o | sed 
's|[^/]*$|.deps/&|;s|\.o$||'`;\
gcc -std=gnu99 -DHAVE_CONFIG_H -I.   -I ./include  -I ./include -I ./ovn -I 
./include -I ./lib -I ./lib -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include
 -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include
 -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib
 -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib
 -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR
-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat 
-Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast 
-Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes 
-Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror  -g 
-O2 -MT controller-vtep/ovn-controller-vtep.o -MD -MP -MF $depbase.Tpo -c -o 
controller-vtep/ovn-controller-vtep.o controller-vtep/ovn-controller-vtep.c &&\
mv -f $depbase.Tpo $depbase.Po
depbase=`echo controller-vtep/vtep.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
gcc -std=gnu99 -DHAVE_CONFIG_H -I.   -I ./include  -I ./include -I ./ovn -I 
./include -I ./lib -I ./lib -I 
/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include
 -I 

Re: [ovs-dev] [PATCH ovn v4 7/7] northd: Add functionality to runtime node

[Mark Gray]

On 16/09/2021 08:08, Han Zhou wrote:
> On Fri, Sep 3, 2021 at 5:22 AM Mark Gray  wrote:
>>
>> Update the runtime node to initialize and destroy some common data
>> used by multiple functions in northd.
> 
> Hi Mark,
> 
> It looks good overall as an initiate step to employ the inc-proc engine for
> ovn-northd. It is great that it achieves the no-input-change-no-compute
> outcome with this series.
> However, for the en-runtime node it doesn't look right. I understand it is
> only for demonstration purposes and is supposed to evolve, but it may look
> misleading in how would we expand it further for I-P:
> 
> 1) I think the I-P engine should focus on data dependency. Each node should
> define its data (output data computed by the node's run()/change-handlers).
> The en-runtime node defines its own data as lr_list, datapaths and ports,
> but the run() function merely initiates empty data structures that are
> later filled by en-northd, which is wrong. Instead, en-runtime should have
> its own data computed according to its input (all those NB/SB  tables), and
> then en-northd should use en-runtime data  in a read-only fashion. It is
> better not adding this node if we don't have this data dependency defined.
> 
> 2) en-northd run() calls ovn_db_run(), which accesses the inputs (DB
> tables) using the northd_context instead of the data from its input nodes.
> If we really want to use the engine for incremental processing, then I
> think the first step is to make sure we never access data through any
> global context in engine functions, and only use input data from the engine
> nodes it depends on. The engine context should only contain idl_txn for
> writing to DBs (even this could be improved so that we don't have any
> global ctx in engines at all, but at least that's how we are living with in
> ovn-controller). Otherwise it would be hard to ensure correctness of I-P.
> For example, what if in ovn_db_run() it depends on some data from the
> context that is not in any of the input nodes, and now if that data
> changes, the ovn_db_run() won't be triggered any more because engine inputs
> not changed, and this would be a bug.

Thanks for this feedback. I have posted a new series. I split it
differently into two nodes - en_northd, and en_flow. en_northd does
everything but generates the lflows and, crucially (as you pointed out),
only depends on the output data from en_northd.
> 
> Thanks,
> Han
> 
>>
>> Signed-off-by: Mark Gray 
>> ---
>>  northd/en-northd.c  |  9 -
>>  northd/en-runtime.c | 30 --
>>  northd/en-runtime.h |  8 
>>  northd/northd.c | 15 +--
>>  northd/northd.h |  5 -
>>  5 files changed, 53 insertions(+), 14 deletions(-)
>>
>> diff --git a/northd/en-northd.c b/northd/en-northd.c
>> index d310fa4dd31f..2a3250f3d57a 100644
>> --- a/northd/en-northd.c
>> +++ b/northd/en-northd.c
>> @@ -19,6 +19,7 @@
>>  #include 
>>
>>  #include "en-northd.h"
>> +#include "en-runtime.h"
>>  #include "lib/inc-proc-eng.h"
>>  #include "northd.h"
>>  #include "openvswitch/vlog.h"
>> @@ -29,7 +30,13 @@ void en_northd_run(struct engine_node *node, void
> *data OVS_UNUSED)
>>  {
>>  const struct engine_context *eng_ctx = engine_get_context();
>>  struct northd_context *ctx = eng_ctx->client_ctx;
>> -ovn_db_run(ctx);
>> +
>> +struct ed_type_runtime *runtime_data =
>> + engine_get_input_data("runtime", node);
>> +
>> +ovn_db_run(ctx, _data->lr_list,
>> +_data->datapaths,
>> +_data->ports);
>>
>>  engine_set_node_state(node, EN_UPDATED);
>>
>> diff --git a/northd/en-runtime.c b/northd/en-runtime.c
>> index aac01cd0351f..b8e5766823bf 100644
>> --- a/northd/en-runtime.c
>> +++ b/northd/en-runtime.c
>> @@ -19,7 +19,9 @@
>>  #include 
>>
>>  #include "en-runtime.h"
>> +#include "openvswitch/hmap.h"
>>  #include "lib/inc-proc-eng.h"
>> +#include "openvswitch/list.h"
>>  #include "northd.h"
>>  #include "openvswitch/vlog.h"
>>
>> @@ -27,14 +29,38 @@ VLOG_DEFINE_THIS_MODULE(en_runtime);
>>
>>  void en_runtime_run(struct engine_node *node, void *data OVS_UNUSED)
>>  {
>> +struct ed_type_runtime *runtime_data = data;
>> +
>> +struct ovs_list *lr_list = _data->lr_list;
>> +struct hmap *datapaths = _data->datapaths;
>> +struct hmap *ports = _data->ports;
>> +
>> +destroy_datapaths_and_ports(datapaths, ports, lr_list);
>> +
>> +ovs_list_init(lr_list);
>> +hmap_init(datapaths);
>> +hmap_init(ports);
>> +
>>  engine_set_node_state(node, EN_UPDATED);
>>  }
>>  void *en_runtime_init(struct engine_node *node OVS_UNUSED,
>>   struct engine_arg *arg OVS_UNUSED)
>>  {
>> -return NULL;
>> +struct ed_type_runtime *data = xzalloc(sizeof *data);
>> +ovs_list_init(>lr_list);
>> +hmap_init(>datapaths);
>> +hmap_init(>ports);
>> +
>> +return data;
>>  }
>>
>> -void en_runtime_cleanup(void *data OVS_UNUSED)
>> +void 

Re: [ovs-dev] [PATCH ovn v4 5/7] northd: Add n_nat_entries field to 'struct ovn_datapath'

[Mark Gray]

On 15/09/2021 18:58, Mark Michelson wrote:
> On 9/14/21 1:27 PM, Mark Gray wrote:
>> On 13/09/2021 21:41, Mark Michelson wrote:
>>> Hi Mark,
>>>
>>> Of all the patches in this series, this is the one I am least sure about.
>> Thanks for looking over these.
>>
>>>
>>> It seems simple enough, but it's not clear to me
>>>
>>> 1) Why would the number of nat entries on an ovn_datapath differ from
>>> what's in the database? Since this series doesn't introduce any "real"
>>> incremental processing, we should be recreating the ovn_datapath on
>>> every iteration. And on each iteration, we should have a fresh view of
>>> the database, so why would the values differ?
>>> 2) Why is this only an issue in destroy_nat_entries()? There are several
>>> other places in the code where we iterate over the number of nat entries
>>> using od->nbr->n_nat. Why are those not affected?
>>> 3) How does this patch address that issue? The patch just sets the
>>> ovn_datapath's n_nat_entries to be the same as the database's count,
>>> which according to the commit message, is not always accurate.
>>>
>>> Can you provide some clarity here? Maybe tell of a situation where
>>> od->nbr->n_nat is incorrect?
>>
>> You are absolutely correct as the details in the commit message are not
>> sufficient to understand. In fact, I had a hard time remembering the
>> reason myself!
>>
>> IIRC, on each iteration, we need to call en_runtime_run(). This needs to
>> initialize the runtime data .. but .. it also needs to destroy the
>> runtime data from the previous run. This may not be the best way to do
>> it, but the I-P framework doesn't have a convenient way to clear data
>> between runs.
>>
>> Consider an example in which we have completed iteration x, and started
>> iteration x+1. If we add a NAT entry between iteration x and iteration
>> x+1, od->nbr->n_nat will contain the updated value of n_nat as the
>> northbound record will have been updated. However, as we have not yet
>> (re)initialized the nat entries in od, od->n_nat_entries will be equal
>> to the previous value. These two values could then be different which
>> makes this loop invalid. Does this make sense? If so, shall I update the
>> commit message? How would you like me to proceed?
> 
> OK, this makes a lot more sense now. It makes sense that the issue 
> specifically would be seen when destroying datapaths and not at any 
> other point.
> 
> Here is my suggestion for how to proceed:
> 1) Update the commit message to give an explanation similar to what 
> you've given here.

I posted a new series and added the comment.
> 2) Correct the finding that I just found right now and that I mention 
> down below.

I forgot to do this, I will fix in v2. Also, I am surprised I didn't see
this as I though I compiled with --enable-Werror.
> 
>>
>>>
>>> On 9/3/21 8:21 AM, Mark Gray wrote:
 destroy_nat_entries() iterates over nat_entries using the
 number of NAT entries from the NB database. When using I-P,
 this behaviour is incorrect as the number of NAT entries in
 the NBDB may differ from that stored in 'struct ovn_datapath'
 causing unexpected behaviour.

 Signed-off-by: Mark Gray 
 ---
northd/northd.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)

 diff --git a/northd/northd.c b/northd/northd.c
 index 724baa3994d5..829c4479f14b 100644
 --- a/northd/northd.c
 +++ b/northd/northd.c
 @@ -605,6 +605,8 @@ struct ovn_datapath {

/* NAT entries configured on the router. */
struct ovn_nat *nat_entries;
 +size_t n_nat_entries;
 +
> 
> n_nat_entries is declared and never used in this function.
> 
bool has_distributed_nat;

/* Set of nat external ips on the router. */
 @@ -789,6 +791,7 @@ init_nat_entries(struct ovn_datapath *od)
od->has_distributed_nat = true;
}
}
 +od->n_nat_entries = od->nbr->n_nat;
}

static void
 @@ -802,7 +805,7 @@ destroy_nat_entries(struct ovn_datapath *od)
destroy_lport_addresses(>dnat_force_snat_addrs);
destroy_lport_addresses(>lb_force_snat_addrs);

 -for (size_t i = 0; i < od->nbr->n_nat; i++) {
 +for (size_t i = 0; i < od->n_nat_entries; i++) {
destroy_lport_addresses(>nat_entries[i].ext_addrs);
}
}

>>>
>>
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn 5/7] northd: Introduce struct northd_data

[Mark Gray]

'struct northd_data' is used to hold the output data from the
incremental processing node 'en_northd'. This will be used, in
a later commit, as the input data to 'en_lflow'. In order to achieve
this, we refactor in the following way:

* Introduce northd_init() which initializes this data.
* Introduce northd_destroy() which clears this data for a new iteration.
* Introduce northd_indices_create() which does a one-time creation of
  indices for number of tables needed by northd.
* Remove 'ovn_internal_version' from the context passed to northd
  as it can be determined within northd using ovn_get_internal_version.
* Remove 'use_parallel_build' from the context passed to northd
  as it can be determined within northd using can_parallelize_hashes().
* Refactor northd.c to use 'struct northd_data' where applicable.

Signed-off-by: Mark Gray 
---
 northd/en-northd.c  |  28 -
 northd/en-northd.h  |   4 +
 northd/northd.c | 274 
 northd/northd.h |  27 -
 northd/ovn-northd.c |  28 +
 5 files changed, 202 insertions(+), 159 deletions(-)

diff --git a/northd/en-northd.c b/northd/en-northd.c
index 8dec51535af0..1b206521e152 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -20,26 +20,46 @@
 
 #include "en-northd.h"
 #include "lib/inc-proc-eng.h"
+#include "openvswitch/list.h" /* TODO This is needed for ovn-parallel-hmap.h.
+   * lib/ovn-parallel-hmap.h should be updated
+   * to include this dependency itself */
+#include "lib/ovn-parallel-hmap.h"
 #include "northd.h"
+#include "lib/util.h"
 #include "openvswitch/vlog.h"
 
 VLOG_DEFINE_THIS_MODULE(en_northd);
 
-void en_northd_run(struct engine_node *node, void *data OVS_UNUSED)
+void en_northd_run(struct engine_node *node, void *data)
 {
 const struct engine_context *eng_ctx = engine_get_context();
 struct northd_idl_context *ctx = eng_ctx->client_ctx;
-ovn_db_run(ctx);
+struct northd_data *northd_data = ((struct ed_type_northd *)data)->data;
 
+northd_destroy(northd_data);
+northd_init(northd_data);
+
+northd_run(ctx, northd_data);
 engine_set_node_state(node, EN_UPDATED);
 
 }
 void *en_northd_init(struct engine_node *node OVS_UNUSED,
- struct engine_arg *arg OVS_UNUSED)
+ struct engine_arg *arg)
 {
-return NULL;
+struct ed_type_northd *data = xmalloc(sizeof *data);
+data->data = xmalloc(sizeof *data->data);
+
+data->data->use_parallel_build = can_parallelize_hashes(false);
+northd_indices_create(data->data, arg->sb_idl);
+northd_init(data->data);
+
+return data;
 }
 
 void en_northd_cleanup(void *data OVS_UNUSED)
 {
+struct northd_data *northd_data = ((struct ed_type_northd *)data)->data;
+
+northd_destroy(northd_data);
+free(northd_data);
 }
diff --git a/northd/en-northd.h b/northd/en-northd.h
index 0e7f76245e69..da386298d821 100644
--- a/northd/en-northd.h
+++ b/northd/en-northd.h
@@ -9,6 +9,10 @@
 
 #include "lib/inc-proc-eng.h"
 
+struct ed_type_northd {
+struct northd_data *data;
+};
+
 void en_northd_run(struct engine_node *node OVS_UNUSED, void *data OVS_UNUSED);
 void *en_northd_init(struct engine_node *node OVS_UNUSED,
  struct engine_arg *arg);
diff --git a/northd/northd.c b/northd/northd.c
index 7c5948504a9b..d236e0361885 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -2853,6 +2853,7 @@ chassis_group_list_changed(
 
 static void
 sync_ha_chassis_group_for_sbpb(struct northd_idl_context *ctx,
+   struct northd_data *data,
const struct nbrec_ha_chassis_group *nb_ha_grp,
struct ovsdb_idl_index *sbrec_chassis_by_name,
const struct sbrec_port_binding *pb)
@@ -2860,7 +2861,7 @@ sync_ha_chassis_group_for_sbpb(struct northd_idl_context 
*ctx,
 bool new_sb_chassis_group = false;
 const struct sbrec_ha_chassis_group *sb_ha_grp =
 ha_chassis_group_lookup_by_name(
-ctx->sbrec_ha_chassis_grp_by_name, nb_ha_grp->name);
+data->sbrec_ha_chassis_grp_by_name, nb_ha_grp->name);
 
 if (!sb_ha_grp) {
 sb_ha_grp = sbrec_ha_chassis_group_insert(ctx->ovnsb_txn);
@@ -2906,6 +2907,7 @@ sync_ha_chassis_group_for_sbpb(struct northd_idl_context 
*ctx,
 static void
 copy_gw_chassis_from_nbrp_to_sbpb(
 struct northd_idl_context *ctx,
+struct northd_data *data,
 struct ovsdb_idl_index *sbrec_chassis_by_name,
 const struct nbrec_logical_router_port *lrp,
 const struct sbrec_port_binding *port_binding)
@@ -2915,7 +2917,7 @@ copy_gw_chassis_from_nbrp_to_sbpb(
  * for the distributed gateway router port. */
 const struct sbrec_ha_chassis_group *sb_ha_chassis_group =
 ha_chassis_group_lookup_by_name(
-ctx->sbrec_ha_chassis_grp_by_name, lrp->name);
+

[ovs-dev] [PATCH ovn 7/7] en_lflow: Generate logical flows

[Mark Gray]

Generate logical flows using 'en_flow' incremental processing node.
This node uses output data from 'en_northd' in order to generate
logical flows.

Signed-off-by: Mark Gray 
---
 northd/en-lflow.c | 12 
 northd/northd.c   |  6 +-
 northd/northd.h   |  1 +
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index 46072cb0162e..e24c00e039c3 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -23,12 +23,24 @@
 
 #include "lib/inc-proc-eng.h"
 #include "northd.h"
+#include "stopwatch.h"
+#include "lib/stopwatch-names.h"
+#include "timeval.h"
 #include "openvswitch/vlog.h"
 
 VLOG_DEFINE_THIS_MODULE(en_lflow);
 
 void en_lflow_run(struct engine_node *node, void *data OVS_UNUSED)
 {
+const struct engine_context *eng_ctx = engine_get_context();
+struct northd_idl_context *ctx = eng_ctx->client_ctx;
+
+struct ed_type_northd *northd_data = engine_get_input_data("northd", node);
+
+stopwatch_start(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
+build_lflows(ctx, northd_data->data);
+stopwatch_stop(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
+
 engine_set_node_state(node, EN_UPDATED);
 }
 void *en_lflow_init(struct engine_node *node OVS_UNUSED,
diff --git a/northd/northd.c b/northd/northd.c
index 6699b8a3ffc1..94620d243dec 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -13209,8 +13209,7 @@ static bool reset_parallel = false;
 
 /* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB database,
  * constructing their contents based on the OVN_NB database. */
-static void
-build_lflows(struct northd_idl_context *ctx, struct northd_data *data)
+void build_lflows(struct northd_idl_context *ctx, struct northd_data *data)
 {
 struct hmap lflows;
 
@@ -14396,9 +14395,6 @@ ovnnb_db_run(struct northd_data *data,
 build_meter_groups(ctx, >meter_groups);
 build_bfd_table(ctx, >bfd_connections, >ports);
 stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
-stopwatch_start(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
-build_lflows(ctx, data);
-stopwatch_stop(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
 stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
 ovn_update_ipv6_prefix(>ports);
 
diff --git a/northd/northd.h b/northd/northd.h
index d4bc5cf16541..07ea6899984f 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -51,5 +51,6 @@ void northd_destroy(struct northd_data *data);
 void northd_init(struct northd_data *data);
 void northd_indices_create(struct northd_data *data,
struct ovsdb_idl *ovnsb_idl);
+void build_lflows(struct northd_idl_context *ctx, struct northd_data *data);
 
 #endif /* NORTHD_H */
-- 
2.27.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn 6/7] northd: Add lflow node

[Mark Gray]

Add an additional node that initially does nothing. This serves as a
template for how to add a new node. This node is inserted after
the northd_node.

This node will be updated in a later commit to generate logical
flows for the SBDB.

Signed-off-by: Mark Gray 
---
 northd/automake.mk   |  2 ++
 northd/en-lflow.c| 42 
 northd/en-lflow.h| 16 +++
 northd/inc-proc-northd.c |  5 -
 northd/northd.c  |  2 --
 5 files changed, 64 insertions(+), 3 deletions(-)
 create mode 100644 northd/en-lflow.c
 create mode 100644 northd/en-lflow.h

diff --git a/northd/automake.mk b/northd/automake.mk
index f0c1fb11c83a..4862ec7b7ff3 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -6,6 +6,8 @@ northd_ovn_northd_SOURCES = \
northd/ovn-northd.c \
northd/en-northd.c \
northd/en-northd.h \
+   northd/en-lflow.c \
+   northd/en-lflow.h \
northd/inc-proc-northd.c \
northd/inc-proc-northd.h \
northd/ipam.c \
diff --git a/northd/en-lflow.c b/northd/en-lflow.c
new file mode 100644
index ..46072cb0162e
--- /dev/null
+++ b/northd/en-lflow.c
@@ -0,0 +1,42 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+
+#include 
+#include 
+#include 
+
+#include "en-lflow.h"
+#include "en-northd.h"
+
+#include "lib/inc-proc-eng.h"
+#include "northd.h"
+#include "openvswitch/vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(en_lflow);
+
+void en_lflow_run(struct engine_node *node, void *data OVS_UNUSED)
+{
+engine_set_node_state(node, EN_UPDATED);
+}
+void *en_lflow_init(struct engine_node *node OVS_UNUSED,
+ struct engine_arg *arg OVS_UNUSED)
+{
+return NULL;
+}
+
+void en_lflow_cleanup(void *data OVS_UNUSED)
+{
+}
diff --git a/northd/en-lflow.h b/northd/en-lflow.h
new file mode 100644
index ..0e4d522ff3fe
--- /dev/null
+++ b/northd/en-lflow.h
@@ -0,0 +1,16 @@
+#ifndef EN_LFLOW_H
+#define EN_LFLOW_H 1
+
+#include 
+
+#include 
+#include 
+#include 
+
+#include "lib/inc-proc-eng.h"
+
+void en_lflow_run(struct engine_node *node, void *data);
+void *en_lflow_init(struct engine_node *node, struct engine_arg *arg);
+void en_lflow_cleanup(void *data);
+
+#endif /* EN_LFLOW_H */
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 572b8de6536a..519fc1d0cb46 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -25,6 +25,7 @@
 #include "openvswitch/vlog.h"
 #include "inc-proc-northd.h"
 #include "en-northd.h"
+#include "en-lflow.h"
 #include "util.h"
 
 VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
@@ -140,6 +141,7 @@ enum sb_engine_node {
 /* Define engine nodes for other nodes. They should be defined as static to
  * avoid sparse errors. */
 static ENGINE_NODE(northd, "northd");
+static ENGINE_NODE(lflow, "lflow");
 
 void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
   struct ovsdb_idl_loop *sb)
@@ -204,13 +206,14 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
 engine_add_input(_northd, _sb_load_balancer, NULL);
 engine_add_input(_northd, _sb_bfd, NULL);
 engine_add_input(_northd, _sb_fdb, NULL);
+engine_add_input(_lflow, _northd, NULL);
 
 struct engine_arg engine_arg = {
 .nb_idl = nb->idl,
 .sb_idl = sb->idl,
 };
 
-engine_init(_northd, _arg);
+engine_init(_lflow, _arg);
 }
 
 void inc_proc_northd_run(struct northd_idl_context *ctx,
diff --git a/northd/northd.c b/northd/northd.c
index d236e0361885..6699b8a3ffc1 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -12881,7 +12881,6 @@ struct lflows_thread_pool {
 struct worker_pool *pool;
 };
 
-
 static void *
 build_lflows_thread(void *arg)
 {
@@ -14410,7 +14409,6 @@ ovnnb_db_run(struct northd_data *data,
 cleanup_stale_fdp_entries(ctx, >datapaths);
 bfd_cleanup_connections(ctx, >bfd_connections);
 stopwatch_stop(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
-
 }
 
 /* Stores the list of chassis which references an ha_chassis_group.
-- 
2.27.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn 4/7] northd: Rename struct northd_context

[Mark Gray]

In order to prepare for a subsequent commit, rename
'struct northd_context' to 'struct northd_idl_context'. In
subsequent commits, 'struct northd_idl_context' will then be
used, only, to hold the IDL context required by northd.

Signed-off-by: Mark Gray 
---
 northd/en-northd.c   |  2 +-
 northd/inc-proc-northd.c |  2 +-
 northd/inc-proc-northd.h |  2 +-
 northd/northd.c  | 83 
 northd/northd.h  |  4 +-
 northd/ovn-northd.c  | 10 ++---
 6 files changed, 52 insertions(+), 51 deletions(-)

diff --git a/northd/en-northd.c b/northd/en-northd.c
index d310fa4dd31f..8dec51535af0 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -28,7 +28,7 @@ VLOG_DEFINE_THIS_MODULE(en_northd);
 void en_northd_run(struct engine_node *node, void *data OVS_UNUSED)
 {
 const struct engine_context *eng_ctx = engine_get_context();
-struct northd_context *ctx = eng_ctx->client_ctx;
+struct northd_idl_context *ctx = eng_ctx->client_ctx;
 ovn_db_run(ctx);
 
 engine_set_node_state(node, EN_UPDATED);
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 85baeb07d3d9..572b8de6536a 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -213,7 +213,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
 engine_init(_northd, _arg);
 }
 
-void inc_proc_northd_run(struct northd_context *ctx,
+void inc_proc_northd_run(struct northd_idl_context *ctx,
  bool recompute) {
 engine_set_force_recompute(recompute);
 engine_init_run();
diff --git a/northd/inc-proc-northd.h b/northd/inc-proc-northd.h
index 09cb8f3b3a80..6ee056dc14f5 100644
--- a/northd/inc-proc-northd.h
+++ b/northd/inc-proc-northd.h
@@ -8,7 +8,7 @@
 
 void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
   struct ovsdb_idl_loop *sb);
-void inc_proc_northd_run(struct northd_context *ctx,
+void inc_proc_northd_run(struct northd_idl_context *ctx,
  bool recompute);
 void inc_proc_northd_cleanup(void);
 
diff --git a/northd/northd.c b/northd/northd.c
index b22c29f90de5..7c5948504a9b 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -1167,7 +1167,7 @@ ovn_datapath_update_external_ids(struct ovn_datapath *od)
 }
 
 static void
-join_datapaths(struct northd_context *ctx, struct hmap *datapaths,
+join_datapaths(struct northd_idl_context *ctx, struct hmap *datapaths,
struct ovs_list *sb_only, struct ovs_list *nb_only,
struct ovs_list *both, struct ovs_list *lr_list)
 {
@@ -1278,7 +1278,7 @@ is_vxlan_mode(struct ovsdb_idl *ovnsb_idl)
 }
 
 static uint32_t
-get_ovn_max_dp_key_local(struct northd_context *ctx)
+get_ovn_max_dp_key_local(struct northd_idl_context *ctx)
 {
 if (is_vxlan_mode(ctx->ovnsb_idl)) {
 /* OVN_MAX_DP_GLOBAL_NUM doesn't apply for vxlan mode. */
@@ -1288,7 +1288,7 @@ get_ovn_max_dp_key_local(struct northd_context *ctx)
 }
 
 static void
-ovn_datapath_allocate_key(struct northd_context *ctx,
+ovn_datapath_allocate_key(struct northd_idl_context *ctx,
   struct hmap *datapaths, struct hmap *dp_tnlids,
   struct ovn_datapath *od, uint32_t *hint)
 {
@@ -1334,7 +1334,7 @@ ovn_datapath_assign_requested_tnl_id(struct hmap 
*dp_tnlids,
  * Initializes 'datapaths' to contain a "struct ovn_datapath" for every logical
  * switch and router. */
 static void
-build_datapaths(struct northd_context *ctx, struct hmap *datapaths,
+build_datapaths(struct northd_idl_context *ctx, struct hmap *datapaths,
 struct ovs_list *lr_list)
 {
 struct ovs_list sb_only, nb_only, both;
@@ -2305,7 +2305,7 @@ tag_alloc_create_new_tag(struct hmap *tag_alloc_table,
 
 
 static void
-join_logical_ports(struct northd_context *ctx,
+join_logical_ports(struct northd_idl_context *ctx,
struct hmap *datapaths, struct hmap *ports,
struct hmap *chassis_qdisc_queues,
struct hmap *tag_alloc_table, struct ovs_list *sb_only,
@@ -2767,7 +2767,7 @@ sbpb_gw_chassis_needs_update(
 }
 
 static struct sbrec_ha_chassis *
-create_sb_ha_chassis(struct northd_context *ctx,
+create_sb_ha_chassis(struct northd_idl_context *ctx,
  const struct sbrec_chassis *chassis,
  const char *chassis_name, int priority)
 {
@@ -2852,7 +2852,7 @@ chassis_group_list_changed(
 }
 
 static void
-sync_ha_chassis_group_for_sbpb(struct northd_context *ctx,
+sync_ha_chassis_group_for_sbpb(struct northd_idl_context *ctx,
const struct nbrec_ha_chassis_group *nb_ha_grp,
struct ovsdb_idl_index *sbrec_chassis_by_name,
const struct sbrec_port_binding *pb)
@@ -2905,7 +2905,7 @@ sync_ha_chassis_group_for_sbpb(struct northd_context *ctx,
  */
 static void
 copy_gw_chassis_from_nbrp_to_sbpb(
-struct northd_context *ctx,
+struct 

[ovs-dev] [PATCH ovn 2/7] northd: Introduce incremental processing for northd

[Mark Gray]

Initial implementation adds a single node (northd). This single
node executes the northd processing pipeline but does not do so
incrementally.

In order to develop incremental processing for northd, the code
will be organised with a .c/.h file for each I-P node following
the naming convention en-.c/.h. These files will
contain definition of the node data, the main node processing
functions and change handlers (if any). The purpose of these nodes
will be coordination of the nodes work and implemention of the
relevant interfaces to plugin to the I-P framework. The actual
work that will be executed by the node will be organised into
a companion file or files. Ideally this file will follow the
naming convention of the node: e.g. en-.c is
associated with .c.

Initial node topology sees the northd node dependent on all DB
nodes. This will evolve over time.

Co-authored-by: Numan Siddique 
Signed-off-by: Numan Siddique 
Signed-off-by: Mark Gray 
---
 lib/inc-proc-eng.h   |  16 +++
 northd/automake.mk   |   4 +
 northd/en-northd.c   |  45 +++
 northd/en-northd.h   |  17 +++
 northd/inc-proc-northd.c | 254 +++
 northd/inc-proc-northd.h |  15 +++
 northd/northd.c  |  12 +-
 northd/northd.h  |   8 +-
 northd/ovn-northd.c  | 201 ---
 9 files changed, 489 insertions(+), 83 deletions(-)
 create mode 100644 northd/en-northd.c
 create mode 100644 northd/en-northd.h
 create mode 100644 northd/inc-proc-northd.c
 create mode 100644 northd/inc-proc-northd.h

diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
index 1ccae559dff6..a3f5a7e64287 100644
--- a/lib/inc-proc-eng.h
+++ b/lib/inc-proc-eng.h
@@ -63,15 +63,22 @@
 #define ENGINE_MAX_INPUT 256
 #define ENGINE_MAX_OVSDB_INDEX 256
 
+#include 
+#include 
+
+#include "compiler.h"
+
 struct engine_context {
 struct ovsdb_idl_txn *ovs_idl_txn;
 struct ovsdb_idl_txn *ovnsb_idl_txn;
+struct ovsdb_idl_txn *ovnnb_idl_txn;
 void *client_ctx;
 };
 
 /* Arguments to be passed to the engine at engine_init(). */
 struct engine_arg {
 struct ovsdb_idl *sb_idl;
+struct ovsdb_idl *nb_idl;
 struct ovsdb_idl *ovs_idl;
 };
 
@@ -347,6 +354,11 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void *data 
OVS_UNUSED) \
 #define ENGINE_FUNC_SB(TBL_NAME) \
 ENGINE_FUNC_OVSDB(sb, TBL_NAME)
 
+/* Macro to define member functions of an engine node which represents
+ * a table of OVN NB DB */
+#define ENGINE_FUNC_NB(TBL_NAME) \
+ENGINE_FUNC_OVSDB(nb, TBL_NAME)
+
 /* Macro to define member functions of an engine node which represents
  * a table of open_vswitch DB */
 #define ENGINE_FUNC_OVS(TBL_NAME) \
@@ -360,6 +372,10 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void *data 
OVS_UNUSED) \
 #define ENGINE_NODE_SB(TBL_NAME, TBL_NAME_STR) \
 ENGINE_NODE_OVSDB(sb, "SB", TBL_NAME, TBL_NAME_STR);
 
+/* Macro to define an engine node which represents a table of OVN NB DB */
+#define ENGINE_NODE_NB(TBL_NAME, TBL_NAME_STR) \
+ENGINE_NODE_OVSDB(nb, "NB", TBL_NAME, TBL_NAME_STR);
+
 /* Macro to define an engine node which represents a table of open_vswitch
  * DB */
 #define ENGINE_NODE_OVS(TBL_NAME, TBL_NAME_STR) \
diff --git a/northd/automake.mk b/northd/automake.mk
index 35ad8c09d9ba..f0c1fb11c83a 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -4,6 +4,10 @@ northd_ovn_northd_SOURCES = \
northd/northd.c \
northd/northd.h \
northd/ovn-northd.c \
+   northd/en-northd.c \
+   northd/en-northd.h \
+   northd/inc-proc-northd.c \
+   northd/inc-proc-northd.h \
northd/ipam.c \
northd/ipam.h
 northd_ovn_northd_LDADD = \
diff --git a/northd/en-northd.c b/northd/en-northd.c
new file mode 100644
index ..d310fa4dd31f
--- /dev/null
+++ b/northd/en-northd.c
@@ -0,0 +1,45 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+
+#include 
+#include 
+#include 
+
+#include "en-northd.h"
+#include "lib/inc-proc-eng.h"
+#include "northd.h"
+#include "openvswitch/vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(en_northd);
+
+void en_northd_run(struct engine_node *node, void *data OVS_UNUSED)
+{
+const struct engine_context *eng_ctx = engine_get_context();
+struct northd_context *ctx = eng_ctx->client_ctx;
+ovn_db_run(ctx);
+
+engine_set_node_state(node, EN_UPDATED);
+
+}
+void *en_northd_init(struct engine_node *node OVS_UNUSED,
+ struct 

[ovs-dev] [PATCH ovn 3/7] northd: Add n_nat_entries field to 'struct ovn_datapath'

[Mark Gray]

destroy_nat_entries() iterates over nat_entries using 'n_nat'
as the number of NAT entries from the NB database. This behaviour can be
incorrect as it assumes that there are 'n_nat' 'nat_entries'. 'struct
ovn_datapath' should maintain a count of 'nat_entries' in 'struct
ovn_datapath' rather than read the value from NBDB IDL to properly
account for the number of 'nat_entries'.

This issue becomes appatent when using destroy_nat_entries()
as part of an incremental processing loop:

Consider an example in which we have completed iteration x, and
started iteration x+1. If we add a NAT entry to NBDB between iteration x
and iteration x+1, od->nbr->n_nat will contain the updated value of
n_nat as the northbound record will have been updated. However, if
we do not (re)initialized the nat entries in od, od->n_nat_entries
could be equal to the previous value. This can cause destroy_nat_entries()
to loop over the wrong number of 'nat_entries' causing unexpected
behaviour.

Signed-off-by: Mark Gray 
---
 northd/northd.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/northd/northd.c b/northd/northd.c
index c895eeb68b6d..b22c29f90de5 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -603,6 +603,8 @@ struct ovn_datapath {
 
 /* NAT entries configured on the router. */
 struct ovn_nat *nat_entries;
+size_t n_nat_entries;
+
 bool has_distributed_nat;
 
 /* Set of nat external ips on the router. */
@@ -787,6 +789,7 @@ init_nat_entries(struct ovn_datapath *od)
 od->has_distributed_nat = true;
 }
 }
+od->n_nat_entries = od->nbr->n_nat;
 }
 
 static void
@@ -800,7 +803,7 @@ destroy_nat_entries(struct ovn_datapath *od)
 destroy_lport_addresses(>dnat_force_snat_addrs);
 destroy_lport_addresses(>lb_force_snat_addrs);
 
-for (size_t i = 0; i < od->nbr->n_nat; i++) {
+for (size_t i = 0; i < od->n_nat_entries; i++) {
 destroy_lport_addresses(>nat_entries[i].ext_addrs);
 }
 }
-- 
2.27.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn 1/7] inc-proc-eng: Allow definition of engine_node with global scope

[Mark Gray]

Refactor ENGINE_NODE() macro to not assign function pointers. This
allows ENGINE_NODE() to be used outside functions, creating an
engine_node with global scope (but can be statically defined within a file).
This allows more flexibility in how the I-P engine can be used as
engine nodes can be defined. Allow this is not explicitly required for
I-P it does allow for a cleaner code base as the node definitions can
be kept together outside any functions.

Additional function pointers (e.g. is_valid(), clear_tracked_data()),
may be assigned later, if required.

Signed-off-by: Mark Gray 
---
 controller/ovn-controller.c | 2 +-
 lib/inc-proc-eng.h  | 8 
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index a719beb0e615..1071966c321d 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -3218,7 +3218,7 @@ main(int argc, char *argv[])
 stopwatch_create(BFD_RUN_STOPWATCH_NAME, SW_MS);
 
 /* Define inc-proc-engine nodes. */
-ENGINE_NODE_CUSTOM_WITH_CLEAR_TRACK_DATA(ct_zones, "ct_zones");
+ENGINE_NODE_CUSTOM_WITH_CLEAR_TRACK_DATA_IS_VALID(ct_zones, "ct_zones");
 ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data");
 ENGINE_NODE(non_vif_data, "non_vif_data");
 ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve");
diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
index 859b30a71c86..1ccae559dff6 100644
--- a/lib/inc-proc-eng.h
+++ b/lib/inc-proc-eng.h
@@ -295,19 +295,19 @@ void engine_ovsdb_node_add_index(struct engine_node *, 
const char *name,
 .init = en_##NAME##_init, \
 .run = en_##NAME##_run, \
 .cleanup = en_##NAME##_cleanup, \
-.is_valid = en_##NAME##_is_valid, \
+.is_valid = NULL, \
 .clear_tracked_data = NULL, \
 };
 
 #define ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \
 ENGINE_NODE_DEF(NAME, NAME_STR)
 
-#define ENGINE_NODE_CUSTOM_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \
+#define ENGINE_NODE_CUSTOM_WITH_CLEAR_TRACK_DATA_IS_VALID(NAME, NAME_STR) \
 ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \
-en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data;
+en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; \
+en_##NAME.is_valid = en_##NAME##_is_valid;
 
 #define ENGINE_NODE(NAME, NAME_STR) \
-static bool (*en_##NAME##_is_valid)(struct engine_node *node) = NULL; \
 ENGINE_NODE_DEF(NAME, NAME_STR)
 
 #define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \
-- 
2.27.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn 0/7] northd: Introduce incremental processing framework

[Mark Gray]

Add the 'inc-proc-eng' framework to northd. This does *not*
add any incremental processing at this stage but provides the
framework to do so. Even in this base configuration, we see an
advantage as northd no longer processes the databases if it has
been woken only to handle, for example, a unixctl command. This
can be seen below

$ ovn-appctl -t ovn-northd stopwatch/reset
$ for i in {1..10}; do ovn-appctl -t ovn-northd stopwatch/show >/dev/null; done
$ ovn-appctl -t ovn-northd stopwatch/show ovnnb_db_run
Statistics for 'ovnnb_db_run'
  Total samples: 0
  Maximum: 0 msec
  Minimum: 0 msec
  95th percentile: 0.00 msec
  Short term average: 0.00 msec
  Long term average: 0.00 msec

Hopefully this starting point will allow others to discuss or contribute
changes to incrementally process some aspects of northd. We an also
decide if it is worth progressing with this in general.

Thanks,

Mark Gray (7):
  inc-proc-eng: Allow definition of engine_node with global scope
  northd: Introduce incremental processing for northd
  northd: Add n_nat_entries field to 'struct ovn_datapath'
  northd: Rename struct northd_context
  northd: Introduce struct northd_data
  northd: Add lflow node
  en_lflow: Generate logical flows

 controller/ovn-controller.c |   2 +-
 lib/inc-proc-eng.h  |  24 ++-
 northd/automake.mk  |   6 +
 northd/en-lflow.c   |  54 ++
 northd/en-lflow.h   |  16 ++
 northd/en-northd.c  |  65 +++
 northd/en-northd.h  |  21 +++
 northd/inc-proc-northd.c| 257 ++
 northd/inc-proc-northd.h|  15 ++
 northd/northd.c | 358 +++-
 northd/northd.h |  36 +++-
 northd/ovn-northd.c | 237 ++--
 12 files changed, 807 insertions(+), 284 deletions(-)
 create mode 100644 northd/en-lflow.c
 create mode 100644 northd/en-lflow.h
 create mode 100644 northd/en-northd.c
 create mode 100644 northd/en-northd.h
 create mode 100644 northd/inc-proc-northd.c
 create mode 100644 northd/inc-proc-northd.h

-- 
2.27.0


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows:adjust Offset when processing packet in POP_VLAN action

[Alin-Gabriel Serdean]

Indeed. Thanks for pointing that out.

To be honest that looks like a bug.
We should return an error if the it is not a valid VLAN frame.

-Original Message-
From: Wilson Peng  
Sent: Wednesday, September 29, 2021 5:52 PM
To: Alin-Gabriel Serdean ; ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] datapath-windows:adjust Offset when processing 
packet in POP_VLAN action

Alin,

Original code diff is to avoid the case below(it will return 
NDIS_STATUS_SUCCESS before RtlMoveMemory in function OvsPopFieldInPacketBuf).

Regards
Wilson

In function OvsPopFieldInPacketBuf(..)
 if (!bufferData) {
EthHdr *ethHdr = (EthHdr *)bufferStart;
/* If the frame is not VLAN make it a no op */
if (ethHdr->Type != ETH_TYPE_802_1PQ_NBO) {
return NDIS_STATUS_SUCCESS;>  may exit here.
}
}

  RtlMoveMemory(bufferStart + shiftLength, bufferStart, shiftOffset);
  NdisAdvanceNetBufferDataStart(curNb, shiftLength, FALSE, NULL);

在 2021/9/29 21:55，“dev 代表 Alin-Gabriel 
Serdean” 写入:

From: aserd...@ovn.org

Thank you for raising awareness about this issue. You are right the
offsets are not being updated when a pop vlan action is hit, they are
updated only on pop_mpls.

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenvswitch%2Fovs%2Fblob%2Fmaster%2Fdatapath-windows%2Fovsext%2FActions.c%23L1173-L1174data=04%7C01%7Cpweisong%40vmware.com%7C97a24c5e1c5642cde59d08d98350cbed%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637685205333861058%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=J4d0vndzCmjpdacwp5KBVntxzNsWepJkSxqvEPRTbCQ%3Dreserved=0
Can you please update the offsets in the corresponding pop vlan
function. I.e.:
---
 datapath-windows/ovsext/Actions.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index e130c2f96..34aa5c5e4 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1141,11 +1141,21 @@ OvsPopVlanInPktBuf(OvsForwardingContext *ovsFwdCtx)
  * Declare a dummy vlanTag structure since we need to compute the size
  * of shiftLength. The NDIS one is a unionized structure.
  */
+NDIS_STATUS status;
+OVS_PACKET_HDR_INFO* layers = >layers;
 NDIS_PACKET_8021Q_INFO vlanTag = {0};
 UINT32 shiftLength = sizeof(vlanTag.TagHeader);
 UINT32 shiftOffset = sizeof(DL_EUI48) + sizeof(DL_EUI48);

-return OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength, 
NULL);
+status = OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength,
+NULL);
+
+if (status == NDIS_STATUS_SUCCESS) {
+layers->l3Offset -= (UINT16) shiftLength;
+layers->l4Offset -= (UINT16) shiftLength;
+}
+
+return status;
 }


-- 
2.32.0

___
dev mailing list
d...@openvswitch.org

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-devdata=04%7C01%7Cpweisong%40vmware.com%7C97a24c5e1c5642cde59d08d98350cbed%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637685205333861058%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=cYplthGDj6Y3JKU33UWegukyXDh7ZehNv6wXVqfWD6s%3Dreserved=0


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath-windows:adjust Offset when processing packet in POP_VLAN action

[Wilson Peng]

Alin,

Original code diff is to avoid the case below(it will return 
NDIS_STATUS_SUCCESS before 
RtlMoveMemory in function OvsPopFieldInPacketBuf).

Regards
Wilson

In function OvsPopFieldInPacketBuf(..)
 if (!bufferData) {
EthHdr *ethHdr = (EthHdr *)bufferStart;
/* If the frame is not VLAN make it a no op */
if (ethHdr->Type != ETH_TYPE_802_1PQ_NBO) {
return NDIS_STATUS_SUCCESS;>  may exit here.
}
}

  RtlMoveMemory(bufferStart + shiftLength, bufferStart, shiftOffset);
  NdisAdvanceNetBufferDataStart(curNb, shiftLength, FALSE, NULL);

在 2021/9/29 21:55，“dev 代表 Alin-Gabriel 
Serdean” 写入:

From: aserd...@ovn.org

Thank you for raising awareness about this issue. You are right the
offsets are not being updated when a pop vlan action is hit, they are
updated only on pop_mpls.

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenvswitch%2Fovs%2Fblob%2Fmaster%2Fdatapath-windows%2Fovsext%2FActions.c%23L1173-L1174data=04%7C01%7Cpweisong%40vmware.com%7C97a24c5e1c5642cde59d08d98350cbed%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637685205333861058%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=J4d0vndzCmjpdacwp5KBVntxzNsWepJkSxqvEPRTbCQ%3Dreserved=0
Can you please update the offsets in the corresponding pop vlan
function. I.e.:
---
 datapath-windows/ovsext/Actions.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index e130c2f96..34aa5c5e4 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1141,11 +1141,21 @@ OvsPopVlanInPktBuf(OvsForwardingContext *ovsFwdCtx)
  * Declare a dummy vlanTag structure since we need to compute the size
  * of shiftLength. The NDIS one is a unionized structure.
  */
+NDIS_STATUS status;
+OVS_PACKET_HDR_INFO* layers = >layers;
 NDIS_PACKET_8021Q_INFO vlanTag = {0};
 UINT32 shiftLength = sizeof(vlanTag.TagHeader);
 UINT32 shiftOffset = sizeof(DL_EUI48) + sizeof(DL_EUI48);

-return OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength, 
NULL);
+status = OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength,
+NULL);
+
+if (status == NDIS_STATUS_SUCCESS) {
+layers->l3Offset -= (UINT16) shiftLength;
+layers->l4Offset -= (UINT16) shiftLength;
+}
+
+return status;
 }


-- 
2.32.0

___
dev mailing list
d...@openvswitch.org

https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-devdata=04%7C01%7Cpweisong%40vmware.com%7C97a24c5e1c5642cde59d08d98350cbed%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637685205333861058%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=cYplthGDj6Y3JKU33UWegukyXDh7ZehNv6wXVqfWD6s%3Dreserved=0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] netdev-vport : Fix userspace tunnel ioctl(SIOCGIFINDEX) info logs.

[lin huang]

From: linhuang 

Userspace tunnel doesn't have a valid device in the kernel. So
get_ifindex() function (ioctl) always get error during
adding a port, deleting a port or updating a port status.

The info log is
"2021-08-29T09:17:39.830Z|00059|netdev_linux|INFO|ioctl(SIOCGIFINDEX)
on vxlan_sys_4789 device failed: No such device"

If there are a lot of userspace tunnel ports on a bridge, the
iface_refresh_netdev_status() function will spend a lot of time.

So ignore userspace tunnel port ioctl(SIOCGIFINDEX) operation, just
return ifindex=0.

Signed-off-by: linhuang 
---
 lib/netdev-vport.c | 3 ++-
 vswitchd/bridge.c  | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 499c0291c..90e21f213 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -1151,8 +1151,9 @@ netdev_vport_get_ifindex(const struct netdev *netdev_)
 {
 char buf[NETDEV_VPORT_NAME_BUFSIZE];
 const char *name = netdev_vport_get_dpif_port(netdev_, buf, sizeof(buf));
+const char *type = netdev_get_dpif_type(netdev_);

-return linux_get_ifindex(name);
+return (strncmp(type, "netdev", 6)) ? linux_get_ifindex(name) : 0;
 }

 #define NETDEV_VPORT_GET_IFINDEX netdev_vport_get_ifindex
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index cb7c5cb76..1325f80c0 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -2052,6 +2052,8 @@ iface_do_create(const struct bridge *br,
 goto error;
 }

+netdev_set_dpif_type(netdev, br->type);
+
 error = iface_set_netdev_config(iface_cfg, netdev, errp);
 if (error) {
 goto error;
--
2.12.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] conntrack: support default timeout policy get/set cmd for netdev datapath

[wenxu]

From: wenxu 

Now, the default timeout policy for netdev datapath is hard codeing. In
some case show or modify is needed.
Add command for get/set default timeout policy. Using like this:

ovs-appctl dpctl/ct-get-default-timeout-policy [dp]
ovs-appctl dpctl/ct-set-default-timeout-policy [dp] policies

Signed-off-by: wenxu 
---
 lib/conntrack-tp.c |  9 ++
 lib/conntrack-tp.h |  2 ++
 lib/ct-dpif.c  | 64 +
 lib/ct-dpif.h  |  8 ++
 lib/dpctl.c| 84 ++
 5 files changed, 167 insertions(+)

diff --git a/lib/conntrack-tp.c b/lib/conntrack-tp.c
index a586d3a..726b854 100644
--- a/lib/conntrack-tp.c
+++ b/lib/conntrack-tp.c
@@ -230,6 +230,15 @@ tm_to_ct_dpif_tp(enum ct_timeout tm)
 return CT_DPIF_TP_ATTR_MAX;
 }
 
+void
+timeout_policy_dump(const struct ct_dpif_timeout_policy *tp, struct ds *ds)
+{
+for (unsigned i = 0; i < N_CT_TM; i++) {
+ds_put_format(ds, "\n\t%s = %"PRIu32, ct_timeout_str[i],
+  tp->attrs[tm_to_ct_dpif_tp(i)]);
+}
+}
+
 static void
 conn_update_expiration__(struct conntrack *ct, struct conn *conn,
  enum ct_timeout tm, long long now,
diff --git a/lib/conntrack-tp.h b/lib/conntrack-tp.h
index 4d411d1..bd22242 100644
--- a/lib/conntrack-tp.h
+++ b/lib/conntrack-tp.h
@@ -22,6 +22,8 @@ enum ct_timeout;
 void timeout_policy_init(struct conntrack *ct);
 int timeout_policy_update(struct conntrack *ct, struct timeout_policy *tp);
 int timeout_policy_delete(struct conntrack *ct, uint32_t tp_id);
+void timeout_policy_dump(const struct ct_dpif_timeout_policy *tp,
+ struct ds *ds);
 struct timeout_policy *timeout_policy_get(struct conntrack *ct, int32_t tp_id);
 void conn_init_expiration(struct conntrack *ct, struct conn *conn,
   enum ct_timeout tm, long long now);
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index cfc2315..ccc4caa 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -20,6 +20,8 @@
 #include 
 
 #include "ct-dpif.h"
+#include "conntrack-private.h"
+#include "conntrack-tp.h"
 #include "openvswitch/ofp-parse.h"
 #include "openvswitch/vlog.h"
 
@@ -180,6 +182,25 @@ ct_dpif_get_tcp_seq_chk(struct dpif *dpif, bool *enabled)
 }
 
 int
+ct_dpif_set_default_timeout_policy(struct dpif *dpif,
+   const struct ct_dpif_timeout_policy *tp)
+{
+return (dpif->dpif_class->ct_set_timeout_policy
+? dpif->dpif_class->ct_set_timeout_policy(dpif, tp)
+: EOPNOTSUPP);
+}
+
+
+int
+ct_dpif_get_default_timeout_policy(struct dpif *dpif,
+   struct ct_dpif_timeout_policy *tp)
+{
+return (dpif->dpif_class->ct_get_timeout_policy
+? dpif->dpif_class->ct_get_timeout_policy(dpif, DEFAULT_TP_ID, tp)
+: EOPNOTSUPP);
+}
+
+int
 ct_dpif_set_limits(struct dpif *dpif, const uint32_t *default_limit,
const struct ovs_list *zone_limits)
 {
@@ -710,6 +731,42 @@ ct_dpif_free_zone_limits(struct ovs_list *zone_limits)
 }
 }
 
+
+/* Parses a specification of a timeout policy from 's' into '*tp'
+ * .  Returns true on success.  Otherwise, returns false and
+ * and puts the error message in 'ds'. */
+bool
+ct_dpif_parse_timeout_policy_tuple(const char *s, struct ds *ds,
+   struct ct_dpif_timeout_policy *tp)
+{
+char *pos, *key, *value, *copy, *err;
+
+pos = copy = xstrdup(s);
+while (ofputil_parse_key_value(, , )) {
+uint32_t tmp;
+
+if (!*value) {
+ds_put_format(ds, "field %s missing value", key);
+goto error;
+}
+
+err = str_to_u32(value, );
+if (err) {
+  free(err);
+  goto error_with_msg;
+}
+
+ct_dpif_set_timeout_policy_attr_by_name(tp, key, tmp);
+}
+free(copy);
+return true;
+
+error_with_msg:
+ds_put_format(ds, "failed to parse field %s", key);
+error:
+free(copy);
+return false;
+}
 /* Parses a specification of a conntrack zone limit from 's' into '*pzone'
  * and '*plimit'.  Returns true on success.  Otherwise, returns false and
  * and puts the error message in 'ds'. */
@@ -792,6 +849,13 @@ static const char *const ct_dpif_tp_attr_string[] = {
 #undef CT_DPIF_TP_ICMP_ATTR
 };
 
+void
+ct_dpif_format_timeout_policy(const struct ct_dpif_timeout_policy *tp,
+  struct ds *ds)
+{
+timeout_policy_dump(tp, ds);
+}
+
 static bool
 ct_dpif_set_timeout_policy_attr(struct ct_dpif_timeout_policy *tp,
 uint32_t attr, uint32_t value)
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index b59cba9..9f09ee0 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -292,6 +292,12 @@ int ct_dpif_set_limits(struct dpif *dpif, const uint32_t 
*default_limit,
 int ct_dpif_get_limits(struct dpif *dpif, uint32_t *default_limit,
const struct ovs_list *, 

Re: [ovs-dev] [PATCH] datapath-windows:adjust Offset when processing packet in POP_VLAN action

[Alin-Gabriel Serdean]

From: aserd...@ovn.org

Thank you for raising awareness about this issue. You are right the
offsets are not being updated when a pop vlan action is hit, they are
updated only on pop_mpls.
https://github.com/openvswitch/ovs/blob/master/datapath-windows/ovsext/Actions.c#L1173-L1174
Can you please update the offsets in the corresponding pop vlan
function. I.e.:
---
 datapath-windows/ovsext/Actions.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/datapath-windows/ovsext/Actions.c 
b/datapath-windows/ovsext/Actions.c
index e130c2f96..34aa5c5e4 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -1141,11 +1141,21 @@ OvsPopVlanInPktBuf(OvsForwardingContext *ovsFwdCtx)
  * Declare a dummy vlanTag structure since we need to compute the size
  * of shiftLength. The NDIS one is a unionized structure.
  */
+NDIS_STATUS status;
+OVS_PACKET_HDR_INFO* layers = >layers;
 NDIS_PACKET_8021Q_INFO vlanTag = {0};
 UINT32 shiftLength = sizeof(vlanTag.TagHeader);
 UINT32 shiftOffset = sizeof(DL_EUI48) + sizeof(DL_EUI48);
 
-return OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength, NULL);
+status = OvsPopFieldInPacketBuf(ovsFwdCtx, shiftOffset, shiftLength,
+NULL);
+
+if (status == NDIS_STATUS_SUCCESS) {
+layers->l3Offset -= (UINT16) shiftLength;
+layers->l4Offset -= (UINT16) shiftLength;
+}
+
+return status;
 }
 
 
-- 
2.32.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] python: replace pyOpenSSL with ssl

[Terry Wilson]

I'll take a look this morning and run it through the neutron ml2/ovn
SSL-related tests.

Terry

On Fri, Sep 10, 2021 at 9:34 AM Timothy Redaelli  wrote:
>
> Currently, pyOpenSSL is half-deprecated upstream and so it's removed on
> some distributions (for example on CentOS Stream 9,
> https://issues.redhat.com/browse/CS-336), but since OVS only
> supports Python 3 it's possible to replace pyOpenSSL with "import ssl"
> included in base Python 3.
>
> Stream recv and send had to be splitted as _recv and _send, since SSLError
> is a subclass of socket.error and so it was not possible to except for
> SSLWantReadError and SSLWantWriteError in recv and send of SSLStream.
>
> Reported-by: Timothy Redaelli 
> Reported-at: https://bugzilla.redhat.com/1988429
> Signed-off-by: Timothy Redaelli 
> ---
>  .ci/linux-prepare.sh |  2 +-
>  .cirrus.yml  |  2 +-
>  .travis.yml  |  1 -
>  python/ovs/poller.py |  6 ++--
>  python/ovs/stream.py | 75 +++-
>  tests/ovsdb-idl.at   |  2 +-
>  6 files changed, 46 insertions(+), 42 deletions(-)
>
> diff --git a/.ci/linux-prepare.sh b/.ci/linux-prepare.sh
> index c55125cf7..b9b499bad 100755
> --- a/.ci/linux-prepare.sh
> +++ b/.ci/linux-prepare.sh
> @@ -21,7 +21,7 @@ make -j4 HAVE_LLVM= HAVE_SQLITE= install
>  cd ..
>
>  pip3 install --disable-pip-version-check --user \
> -flake8 hacking sphinx pyOpenSSL wheel setuptools
> +flake8 hacking sphinx wheel setuptools
>  pip3 install --user --upgrade docutils
>  pip3 install --user  'meson==0.47.1'
>
> diff --git a/.cirrus.yml b/.cirrus.yml
> index 358f2ba25..bb206f35f 100644
> --- a/.cirrus.yml
> +++ b/.cirrus.yml
> @@ -9,7 +9,7 @@ freebsd_build_task:
>
>env:
>  DEPENDENCIES: automake libtool gmake gcc wget openssl python3
> -PY_DEPS:  sphinx|openssl
> +PY_DEPS:  sphinx
>  matrix:
>COMPILER: gcc
>COMPILER: clang
> diff --git a/.travis.yml b/.travis.yml
> index 51d051108..c7aeede06 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -17,7 +17,6 @@ addons:
>- libjemalloc-dev
>- libnuma-dev
>- libpcap-dev
> -  - python3-openssl
>- python3-pip
>- python3-sphinx
>- libelf-dev
> diff --git a/python/ovs/poller.py b/python/ovs/poller.py
> index 3624ec865..157719c3a 100644
> --- a/python/ovs/poller.py
> +++ b/python/ovs/poller.py
> @@ -26,9 +26,9 @@ if sys.platform == "win32":
>  import ovs.winutils as winutils
>
>  try:
> -from OpenSSL import SSL
> +import ssl
>  except ImportError:
> -SSL = None
> +ssl = None
>
>  try:
>  from eventlet import patcher as eventlet_patcher
> @@ -73,7 +73,7 @@ class _SelectSelect(object):
>  def register(self, fd, events):
>  if isinstance(fd, socket.socket):
>  fd = fd.fileno()
> -if SSL and isinstance(fd, SSL.Connection):
> +if ssl and isinstance(fd, ssl.SSLSocket):
>  fd = fd.fileno()
>
>  if sys.platform != 'win32':
> diff --git a/python/ovs/stream.py b/python/ovs/stream.py
> index f5a520862..cd74b46be 100644
> --- a/python/ovs/stream.py
> +++ b/python/ovs/stream.py
> @@ -22,9 +22,9 @@ import ovs.socket_util
>  import ovs.vlog
>
>  try:
> -from OpenSSL import SSL
> +import ssl
>  except ImportError:
> -SSL = None
> +ssl = None
>
>  if sys.platform == 'win32':
>  import ovs.winutils as winutils
> @@ -322,6 +322,12 @@ class Stream(object):
>  The recv function will not block waiting for data to arrive.  If no
>  data have been received, it returns (errno.EAGAIN, "") 
> immediately."""
>
> +try:
> +return self._recv(n)
> +except socket.error as e:
> +return (ovs.socket_util.get_exception_errno(e), "")
> +
> +def _recv(self, n):
>  retval = self.connect()
>  if retval != 0:
>  return (retval, "")
> @@ -331,10 +337,7 @@ class Stream(object):
>  if sys.platform == 'win32' and self.socket is None:
>  return self.__recv_windows(n)
>
> -try:
> -return (0, self.socket.recv(n))
> -except socket.error as e:
> -return (ovs.socket_util.get_exception_errno(e), "")
> +return (0, self.socket.recv(n))
>
>  def __recv_windows(self, n):
>  if self._read_pending:
> @@ -396,6 +399,12 @@ class Stream(object):
>  Will not block.  If no bytes can be immediately accepted for
>  transmission, returns -errno.EAGAIN immediately."""
>
> +try:
> +return self._send(buf)
> +except socket.error as e:
> +return -ovs.socket_util.get_exception_errno(e)
> +
> +def _send(self, buf):
>  retval = self.connect()
>  if retval != 0:
>  return -retval
> @@ -409,10 +418,7 @@ class Stream(object):
>  if sys.platform == 'win32' and self.socket is None:
>  return self.__send_windows(buf)
>
> -try:
> -return 

Re: [ovs-dev] [PATCH net-next v6] net: openvswitch: IPv6: Add IPv6 extension header support

[Jakub Kicinski]

On Wed, 29 Sep 2021 08:19:05 +0200 Nicolas Dichtel wrote:
> > /* Insert a kernel only KEY_ATTR */
> > #define OVS_KEY_ATTR_TUNNEL_INFO__OVS_KEY_ATTR_MAX
> > #undef OVS_KEY_ATTR_MAX
> > #define OVS_KEY_ATTR_MAX__OVS_KEY_ATTR_MAX  
> Following the other thread [1], this will break if a new app runs over an old
> kernel.

Good point.

> Why not simply expose this attribute to userspace and throw an error if a
> userspace app uses it?

Does it matter if it's exposed or not? Either way the parsing policy
for attrs coming from user space should have a reject for the value.
(I say that not having looked at the code, so maybe I shouldn't...)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] system-offloads-traffic.at: Add sFlow offload test cases

[Simon Horman]

On Tue, Sep 28, 2021 at 02:41:17PM +0800, Chris Mi wrote:
> Hi Simon,
> 
> On 9/24/2021 3:03 PM, Simon Horman wrote:
> > On Thu, Sep 16, 2021 at 11:38:52AM +0300, Chris Mi wrote:
> > > Add two sFlow offload test caes:
> > > 
> > >3: sflow offloads with sampling=1 - ping between two ports - offloads 
> > > enabled ok
> > >4: sflow offloads with sampling=2 - ping between two ports - offloads 
> > > enabled ok
> > > 
> > > Signed-off-by: Chris Mi
> > Thanks Chris,
> > 
> > These tests look good to me. But could we arrange things such
> > that they are skipped if the system doesn't support this offload.
> Above 2 new tests don't have special requirement. They take test case #2 as
> example and extend it.
> You know the sample offload series is being reviewed in another email
> thread. Maybe I should add this
> patch to the end of that series. There are two reasons I didn't add it:
>  1. I don't want to increase the version number too big.
>  2. It is easier to review and revise this patch alone.
> 
> So how about I add this patch to the sample offload series in next version?
> Then we don't need to check
> if the system support it or not. Because if it fails in the future, we must
> fix it.

Thanks, I think that is reasonable.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 0/6] Add support for ovs metering with tc offload

[Simon Horman]

On Thu, Sep 02, 2021 at 01:18:17PM +0300, Roi Dayan via dev wrote:
> Hi,
> 
> This series is adding support for tc offloading of ovs metering.
> The first 3 patches add lib support.
> 4th patch adding tc police actions to reflect ovs metering.
> 5th patch cleaning existing tc police actions on start to avoid conflicts.
> last patch is offloacing tc rules with the police actions.
> 
> Thanks,
> Roi

Hi Roi,

I apologise for not seeing this earlier.

As I believe you are aware Corigine have been working on adding support for
offloading TC actions independent of flows to the kernel. This is intended
as a mechanism for TC offload of meters, by allowing police action
instances to be shared between flows - meters can be used by different
flows.

Am I correct in thinking that this patchset would be complementary to
such support in TC?

Kind regards,
Simon
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] nbctl: allow multiple bfd sessions with same nexthop and different outports

[Surya Seetharaman]

Hi all,

Tested this PR on an OCP OVN-K cluster and it works as expected. I created
two pods on different nodes having the same nexthop and OVN created two BFD
sessions one from each of the GW routers on those nodes towards the
nexthop/dst_ip.

===

sh-4.4# ovn-nbctl lr-route-list GR_ci-ln-xwgizmt-f76d1-f8gv9-worker-b-vshc4
IPv4 Routes
  10.128.8.1210.0.128.4 src-ip
rtoe-GR_ci-ln-xwgizmt-f76d1-f8gv9-worker-b-vshc4 ecmp-symmetric-reply bfd
10.128.0.0/16100.64.0.1 dst-ip
0.0.0.0/010.0.128.1 dst-ip
rtoe-GR_ci-ln-xwgizmt-f76d1-f8gv9-worker-b-vshc4
sh-4.4# ovn-nbctl lr-route-list GR_ci-ln-xwgizmt-f76d1-f8gv9-worker-c-rrq85
IPv4 Routes
 10.128.10.1310.0.128.4 src-ip
rtoe-GR_ci-ln-xwgizmt-f76d1-f8gv9-worker-c-rrq85 ecmp-symmetric-reply bfd
10.128.0.0/16100.64.0.1 dst-ip
0.0.0.0/010.0.128.1 dst-ip
rtoe-GR_ci-ln-xwgizmt-f76d1-f8gv9-worker-c-rrq85
sh-4.4# ovn-nbctl list bfd
_uuid   : a646da00-f201-4048-923b-82bac426c91a
detect_mult : []
dst_ip  : "10.0.128.4"
external_ids: {}
logical_port: rtoe-GR_ci-ln-xwgizmt-f76d1-f8gv9-worker-b-vshc4
min_rx  : []
min_tx  : []
options : {}
status  : down

_uuid   : 00c14ced-6c0c-4b9e-8772-4eff9411bad6
detect_mult : []
dst_ip  : "10.0.128.4"
external_ids: {}
logical_port: rtoe-GR_ci-ln-xwgizmt-f76d1-f8gv9-worker-c-rrq85
min_rx  : []
min_tx  : []
options : {}
status  : down

Would be good if we could move this patch along.

Thanks,
Surya.


On Fri, Sep 24, 2021 at 3:54 PM Lorenzo Bianconi <
lorenzo.bianc...@redhat.com> wrote:

> Allow CMS to create multiple bfd sessions with the same nexthop but
> different outports:
>
> ovn-nbctl --bfd --policy=src-ip --ecmp-symmetric-reply lr-route-add
> GR_ovn-worker 10.244.0.5/32 172.18.0.4 rtoe-GR_ovn-worker
> ovn-nbctl --bfd --policy=src-ip --ecmp-symmetric-reply lr-route-add
> GR_ovn-worker2 10.244.2.5/32 172.18.0.4 rtoe-GR_ovn-worker2
>
> https://bugzilla.redhat.com/show_bug.cgi?id=2007549
> Signed-off-by: Lorenzo Bianconi 
> ---
>  tests/ovn-northd.at   | 12 ++--
>  utilities/ovn-nbctl.c | 31 +++
>  2 files changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 5de554455..c5400d806 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -3011,7 +3011,7 @@ AT_KEYWORDS([northd-bfd])
>  ovn_start
>
>  check ovn-nbctl --wait=sb lr-add r0
> -for i in $(seq 1 5); do
> +for i in $(seq 1 7); do
>  check ovn-nbctl --wait=sb lrp-add r0 r0-sw$i 00:00:00:00:00:0$i
> 192.168.$i.1/24
>  check ovn-nbctl --wait=sb ls-add sw$i
>  check ovn-nbctl --wait=sb lsp-add sw$i sw$i-r0
> @@ -3052,12 +3052,20 @@ check ovn-nbctl --bfd lr-route-add r0 240.0.0.0/8
> 192.168.5.2 r0-sw5
>  wait_column down bfd status logical_port=r0-sw5
>  AT_CHECK([ovn-nbctl lr-route-list r0 | grep 192.168.5.2 | grep -q
> bfd],[0])
>
> +check ovn-nbctl --bfd --policy=src-ip lr-route-add r0 192.168.6.1/32
> 192.168.10.10 r0-sw6
> +wait_column down bfd status logical_port=r0-sw6
> +AT_CHECK([ovn-nbctl lr-route-list r0 | grep 192.168.6.1 | grep -q
> bfd],[0])
> +
> +check ovn-nbctl --bfd --policy=src-ip lr-route-add r0 192.168.7.1/32
> 192.168.10.10 r0-sw7
> +wait_column down bfd status logical_port=r0-sw7
> +AT_CHECK([ovn-nbctl lr-route-list r0 | grep 192.168.7.1 | grep -q
> bfd],[0])
> +
>  route_uuid=$(fetch_column nb:logical_router_static_route _uuid ip_prefix="
> 100.0.0.0/8")
>  check ovn-nbctl clear logical_router_static_route $route_uuid bfd
>  wait_column admin_down bfd status logical_port=r0-sw1
>
>  ovn-nbctl destroy bfd $uuid
> -wait_row_count bfd 3
> +wait_row_count bfd 5
>
>  AT_CLEANUP
>  ])
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 10217dcd5..e34bb65f7 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -4121,6 +4121,15 @@ nbctl_lr_route_add(struct ctl_context *ctx)
>  }
>  }
>
> +if (ctx->argc == 5) {
> +/* validate output port. */
> +error = lrp_by_name_or_uuid(ctx, ctx->argv[4], true, _lrp);
> +if (error) {
> +ctx->error = error;
> +goto cleanup;
> +}
> +}
> +
>  struct shash_node *bfd = shash_find(>options, "--bfd");
>  const struct nbrec_bfd *nb_bt = NULL;
>  if (bfd) {
> @@ -4136,20 +4145,18 @@ nbctl_lr_route_add(struct ctl_context *ctx)
>  } else {
>  const struct nbrec_bfd *iter;
>  NBREC_BFD_FOR_EACH (iter, ctx->idl) {
> -if (!strcmp(iter->dst_ip, next_hop)) {
> -nb_bt = iter;
> -break;
> +/* match endpoint ip. */
> +if 

Re: [ovs-dev] [PATCH v2 0/6] Add support for ovs metering with tc offload

[Jianbo Liu via dev]

On Fri, 2021-09-24 at 15:51 +0200, Ilya Maximets wrote:
> On 9/22/21 09:25, Roi Dayan wrote:
> > 
> > 
> > On 2021-09-02 3:59 PM, Roi Dayan wrote:
> > > Hi,
> > > 
> > > This series is adding support for tc offloading of ovs metering.
> > > The first 3 patches add lib support.
> > > 4th patch adding tc police actions to reflect ovs metering.
> > > 5th patch cleaning existing tc police actions on start to avoid
> > > conflicts.
> > > last patch is offloacing tc rules with the police actions.
> > > 
> > > Thanks,
> > > Roi
> > > 
> > > 
> > > Notes:
> > >  v2
> > >  - Move tc police parse call from last patch to 2nd patch.
> > >    2nd patch is adding the parse lib func and add the call to
> > > it in that patch.
> > >    Last patch is the put of tc police act.
> > >    In 2nd patch also add empty switch case for tc police act
> > > put as the impl.
> > >    is done in the last patch when support for put is being
> > > added.
> > > 
> > > 
> > > 
> > > Jianbo Liu (6):
> > >    netdev-linux: Refactor put police action netlink message
> > >    tc: Add support parsing tc police action
> > >    netdev-linux: Add functions to manipulate tc police action
> > >    dpif-netlink: Offloading meter to tc police action
> > >    dpif-netlink: Cleanup police actions with reserved indexes on
> > > startup
> > >    netdev-offload-tc: Offloading rules with police actions
> > > 
> > >   lib/dpif-netlink.c  | 259
> > > ++-
> > >   lib/netdev-linux.c  | 350
> > > ++--
> > >   lib/netdev-linux.h  |   9 ++
> > >   lib/netdev-offload-tc.c |  11 ++
> > >   lib/netdev-offload.h    |   4 +
> > >   lib/tc.c    | 103 ++
> > >   lib/tc.h    |  11 ++
> > >   7 files changed, 702 insertions(+), 45 deletions(-)
> > > 
> > 
> > 
> > Hi,
> > 
> > Just pinging about this series. adding support for metering for tc.
> > Would like to get some initial comments/thoughts.
> 
> Hi, Roi.  Thanks for working on this!
> I didn't look closely at the code, but I have one design comment.

Thanks for your time to take a look at the patches.

> 
> In theory it should be possible to use netdev-oddload-tc from the
> userspace datapath (dpif-netdev).  E.g. we may want to use it for
> afxdp ports with skip_sw policy.
> 
> I could be wrong, but at the quick glance it seems that some of
> the important parts of meter offloading for tc are implemented
> inside the lib/dpif-netlink and can not be re-used by the userspace
> datapath and will need to be duplicated.  If that's true, I think,
> we need to move them to a lower netdev-offload-tc layer, if possible.
> 
> You  may also want to look at old patch set for meter offloading
> in userspace datapath:
>  
> https://patchwork.ozlabs.org/project/openvswitch/list/?series=150532
> I'm not saying that has a perfect API or that it can smoothly use
> netdev-offload-tc, it definitely needs some changes.  But it may
> give some idea on what userspace datapath needs and what we need
> to do in order to came up with a common dpif API that can be used
> by both datapath implementations, and a common netdev-offload API
> that also can be used from both of them.

It's a good idea to have a common API for both datapaths. This patchset
has been there for long time. I wonder why it was not merged, what did
it miss? Could you please give me more guides before I continue to work
on it?

Thanks!
Jianbo

> 
> Best regards, Ilya Maximets.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v6 13/12] ovn-controller: Remove unnecessary log call

[Frode Nordahl]

A INFO level log call used during development was left behind in
the en_plug_provider_lookup_run function.  Remove it.
---
 controller/ovn-controller.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index f5516a879..7ec377f03 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -2995,7 +2995,6 @@ static void
 en_plug_provider_lookup_run(struct engine_node *node,
 void *data OVS_UNUSED)
 {
-VLOG_INFO("en_plug_provider_lookup_run");
 if (!plug_provider_has_providers()) {
 engine_set_node_state(node, EN_UNCHANGED);
 return;
-- 
2.32.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH net-next v6] net: openvswitch: IPv6: Add IPv6 extension header support

[Nicolas Dichtel]

Le 29/09/2021 à 02:48, Jakub Kicinski a écrit :
> On Tue, 28 Sep 2021 12:47:27 -0700 Toms Atteka wrote:
>> diff --git a/include/uapi/linux/openvswitch.h 
>> b/include/uapi/linux/openvswitch.h
>> index a87b44cd5590..dc6eb5f6399f 100644
>> --- a/include/uapi/linux/openvswitch.h
>> +++ b/include/uapi/linux/openvswitch.h
>> @@ -346,6 +346,13 @@ enum ovs_key_attr {
>>  #ifdef __KERNEL__
>>  OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
>>  #endif
>> +
>> +#ifndef __KERNEL__
> 
> #else
> 
>> +PADDING,  /* Padding so kernel and non kernel field count would match */
> 
> The name PADDING seems rather risky, collisions will be likely.
> OVS_KEY_ATTR_PADDING maybe?
> 
> But maybe we don't need to define this special value and bake it into
> the uAPI, why can't we add something like this to the kernel header
> (i.e. include/linux/openvswitch.h):
> 
> /* Insert a kernel only KEY_ATTR */
> #define OVS_KEY_ATTR_TUNNEL_INFO  __OVS_KEY_ATTR_MAX
> #undef OVS_KEY_ATTR_MAX
> #define OVS_KEY_ATTR_MAX  __OVS_KEY_ATTR_MAX
Following the other thread [1], this will break if a new app runs over an old
kernel.
Why not simply expose this attribute to userspace and throw an error if a
userspace app uses it?

[1]
https://lore.kernel.org/lkml/CAASuNyUWoZ1wToEUYbdehux=yVnWQ=sukdyrkqfrd-72dol...@mail.gmail.com/

> 
>> +#endif
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev