Re: [ovs-dev] [PATCH] readthedocs: Use dirhtml builder.

[Antonin Bas via dev]

LGTM
For what it’s worth, I can confirm that the following command generates what 
looks like the correct documentation structure:
sphinx-build -a -b dirhtml Documentation/ /tmp/foo


From: Ilya Maximets 
Date: Thursday, October 26, 2023 at 10:54 AM
To: ovs-dev@openvswitch.org 
Cc: Ilya Maximets , Antonin Bas , David 
Marchand 
Subject: [PATCH] readthedocs: Use dirhtml builder.
We used this builder before, but from the project configuration
on the website.  ReadTheDocs doesn't allow to change it there
anymore and it doesn't allow to see the full name of the previously
used builder (!!), so I failed to migrate it to the config file.

The result is that older link like:
  
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.openvswitch.org%2Fen%2Flatest%2Fhowto%2Fdpdk%2F=05%7C01%7Cabas%40vmware.com%7C850f232ba01d4a99968b08dbd64c999b%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C638339396708370094%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C=KbithcqCdrfxvdvLcjGzWKs6NEGPN3byzCFV1CCA%2BDA%3D=0
Now require .html:
  
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.openvswitch.org%2Fen%2Flatest%2Fhowto%2Fdpdk.html=05%7C01%7Cabas%40vmware.com%7C850f232ba01d4a99968b08dbd64c999b%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C638339396708526405%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C=FbNXZ4ESN8UcPYn%2BBY63PX6E5xS5UijNL%2F%2BxPPj5xNQ%3D=0

Fixing now by switching the builder back.

Fixes: e388bd73b70d ("readthedocs: Add the configuration file.")
Reported-by: Antonin Bas 
Reported-by: David Marchand 
Reported-at: 
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenvswitch%2Fovs-issues%2Fissues%2F310=05%7C01%7Cabas%40vmware.com%7C850f232ba01d4a99968b08dbd64c999b%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C638339396708526405%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C=w3%2BjSR%2FteyL0V%2BNnGrvjXi4MW7fpzrXFlsIDunzVPUU%3D=0
Signed-off-by: Ilya Maximets 
---

The version of the docs with the change applied can be
temporarily seen here:
   
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Figsilya-ovs.readthedocs.io%2Fen%2Flatest%2F=05%7C01%7Cabas%40vmware.com%7C850f232ba01d4a99968b08dbd64c999b%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C638339396708526405%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C=KxSIAxcb8LdvBe2PrWrT5%2BSKyJFrNEzaMJou7R4Op2s%3D=0

 .readthedocs.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.readthedocs.yaml b/.readthedocs.yaml
index e481e64f1..7d505150e 100644
--- a/.readthedocs.yaml
+++ b/.readthedocs.yaml
@@ -14,6 +14,7 @@ build:
 # Build documentation in the "Documentation/" directory with Sphinx.
 sphinx:
   configuration: Documentation/conf.py
+  builder: "dirhtml"

 # Build all formats: HTML, PDF, ePub.
 formats: all
--
2.41.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] tests: Use ping timeout instead of deadline.

[Ilya Maximets]

On 10/26/23 14:06, Ilya Maximets wrote:
> On 10/26/23 08:04, Frode Nordahl wrote:
>> On Wed, Oct 25, 2023 at 11:33 PM Ilya Maximets  wrote:
>>>
>>> On 10/25/23 11:45, Simon Horman wrote:
 On Sat, Oct 21, 2023 at 05:04:48PM +0200, Frode Nordahl wrote:
> Many system tests currently use ping with the combination of a
> low packet count (-c 3), short interval between sends (-i 0.3)
> and a _deadline_ of 2 seconds (-d 2).
>
> This combination of options may lead to a situation where more
> than count packets are sent however ping will stop when count
> packets are received. This results in a failed test due to how
> the result is checked, for example:
>
> ping6 -q -c 3 -i 0.3 -w 2 fc00::3 | FORMAT_PING
> @@ -1,2 +1,2 @@
> -3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +4 packets transmitted, 3 received, 25% packet loss, time 0ms
>
> To reiterate, in the above example there is no packet loss, but
> ping stops after _receiving_ 3 packets, not bothering with
> waiting for the response to the fourth packet it just sent out.
>
> If we look at the iputils ping manual for the -w deadline option
> we can read that this is expected behavior:
>
>> Specify a timeout, in seconds, before ping exits regardless of
>> how many packets have been sent or received. In this case ping
>> does not stop after count packet are sent, it waits either for
>> deadline expire or until count probes are answered or for some
>> error notification from network.
>
> To avoid these kinds of failures in checks where a response is
> expected, we replace ping -w with ping -W.
>
> We keep ping -w for checks where it is expected to NOT get a
> response.
>
> Signed-off-by: Frode Nordahl 

 Thanks Frode,

 TIL about -w and -W.
>>>
>>> I learned about -W as well. :)
>>>
>>> Thanks, Frode, for figuring out the cause of these failures!  I've seen
>>> them before, but didn't dig too deep to find a cause.  OVN also has them
>>> from time to time.
>>
>> yw, we run all the system tests in an automated fashion as part of the
>> openvswitch package regression testing, and we see them quite
>> frequently, most likely due to the load of the CI infrastructure.
>>
>>> Though I'm not sure if -W is the right choice.  Reading the description:
>>>
>>>-W timeout
>>>Time to wait for a response, in seconds. The
>>>option affects only timeout in absence of any
>>>responses, otherwise ping waits for two RTTs.
>>>Real number allowed with dot as a decimal
>>>separator (regardless locale setup). 0 means
>>>infinite timeout.
>>>
>>> And I don't really like the 'in absence of ANY responses' part of it.
>>>
>>> So, IIUC, if we send 3 packets, first gets replied and the other two
>>> are dropped somewhere, ping will ignore the timeout and will wait
>>> indefinitely.  Unfortunately, OVS gives the first packet a special
>>> treatment, so potential for this scenario to happen is rather high.
>>> This may break CI systems, getting them stuck testing one patch. And
>>> it doesn't seem like we can mix -w and -W, at least the behavior is
>>> not really defined in this case.
>>
>> It also says "otherwise ping waits for two RTTs.", so it will not wait
>> indefinitely. The documentation is a bit convoluted though so I went
>> to look so that we can be sure about what it will do.
>>
>> On arrival of the first packet, ping will gather various information
>> [0] which will be used to compute the RTT [1], which is used when
>> initializing the waittime [2][3].
>>
>> So it appears to me -W would cover the scenario laid out above, i.e.
>> if we get one reply quickly and the rest are lost, the computed RTT
>> would have a ping exit within a reasonable timeframe. Even if the
>> first response comes near the timeout value, the RTT would not be more
>> than 6 seconds for a -W of 3.
> 
> Looks like I misread the docs.  Thanks for digging this through!
> It does indeed look like it will not wait for too long.  I also
> tested this with the following set of OpenFlow rules that allows
> exactly one ICMP packet to go through and drops the others (the
> interval should be a bit higher for this to work, because it
> relies on revalidation to update the flows):
> 
>  table=0 priority=100 icmp 
> actions=learn(table=0,priority=110,eth_type=0x0800,nw_proto=1,NXM_OF_ETH_SRC),normal
>  table=0 priority=0 actions=normal
> 
> It does not wait indefinitely indeed.  However, I can't reproduce
> the RTT thing.  In my testing it waits for an extra interval (-i)
> instead.  But maybe it's because RTT is much lower than the interval.
> 
>>
>> 0: 
>> https://github.com/iputils/iputils/blob/0cc6da796b9a64113152c071088701cb95a72ae8/ping/ping.c#L1654-L1660
>> 1: 
>> 

Re: [ovs-dev] [PATCH ovn v2 10/18] northd: Refactor lflow management into a separate module.

[0-day Robot]

Bleep bloop.  Greetings Numan Siddique, 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 has non-spaces leading whitespace
WARNING: Line has trailing whitespace
#4506 FILE: northd/northd.c:15947:


Lines checked: 5244, 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

Re: [ovs-dev] [PATCH ovn v2 09/18] northd: Add a new node ls_lbacls.

[0-day Robot]

Bleep bloop.  Greetings Numan Siddique, 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:
ERROR: Inappropriate bracing around statement
#721 FILE: northd/en-ls-lb-acls.h:49:
HMAP_FOR_EACH (LS_LBACLS_REC, key_node, &(TABLE)->entries)

ERROR: Inappropriate bracing around statement
#770 FILE: northd/en-port-group.h:52:
HMAP_FOR_EACH (LS_PG, key_node, &(TABLE)->entries)

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


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

Re: [ovs-dev] [PATCH ovn v2 06/18] northd: Add a new engine 'lr-lb-nat-data' to manage routers' lb data.

[0-day Robot]

Bleep bloop.  Greetings Numan Siddique, 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:
ERROR: Inappropriate bracing around statement
#811 FILE: northd/en-lr-lb-nat-data.h:57:
HMAP_FOR_EACH (LR_LB_NAT_REC, key_node, &(TABLE)->entries)

ERROR: Inappropriate bracing around statement
#858 FILE: northd/en-lr-nat.h:93:
HMAP_FOR_EACH (LR_NAT_REC, key_node, &(TABLE)->entries)

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


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

Re: [ovs-dev] [PATCH ovn v2 04/18] northd: Move router ports SB PB options sync to sync_to_sb_pb node.

[0-day Robot]

Bleep bloop.  Greetings Numan Siddique, 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: Comment with 'xxx' marker
#171 FILE: northd/northd.c:4865:
/* XXX Why can't we enable always-redirect when redirect-type

Lines checked: 647, Warnings: 1, 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 ovn v2 18/18] northd: Add northd change handler for sync_to_sb_lb node.

[numans]

From: Numan Siddique 

Any changes to northd engine node due to load balancers
are now handled in 'sync_to_sb_lb' node to sync the changed
load balancers to SB load balancers.

The logic to sync the SB load balancers is changed a bit and it
now mimics the SB lflow sync.

Below are the scale testing results done with all the patches applied
in this series using ovn-heater.  The test ran the scenario  -
ocp-500-density-heavy.yml [1].

The resuts are:

---
Min (s) Median (s)  90%ile (s)  99%ile 
(s)  Max (s) Mean (s)Total (s)   Count   Failed
---
Iteration Total 0.1368831.1290161.192001
1.2041671.2127280.66501783.127099   125 0
Namespace.add_ports 0.0052160.0057360.007034
0.0154860.0189780.0062110.776373125 0
WorkerNode.bind_port0.0350300.0460820.052469
0.0582930.0603110.04597311.493259   250 0
WorkerNode.ping_port0.0050570.0067271.047692
1.0692531.0713360.26689666.724094   250 0
---

The results with the present main [2] are:

---
Min (s) Median (s)  90%ile (s)  99%ile 
(s)  Max (s) Mean (s)Total (s)   Count   Failed
---
Iteration Total 0.1354912.2238053.311270
3.3390783.3453461.729172216.146495  125 0
Namespace.add_ports 0.0053800.0057440.006819
0.0187730.0208000.0062920.786532125 0
WorkerNode.bind_port0.0341790.0460550.053488
0.0588010.0710430.04611711.529311   250 0
WorkerNode.ping_port0.0049560.0069523.086952
3.1917433.1928070.791544197.886026  250 0
---

[1] - 
https://github.com/ovn-org/ovn-heater/blob/main/test-scenarios/ocp-500-density-heavy.yml
[2] - 2a12cda890a7("controller, northd: Wait for cleanup before replying to 
exit")

Signed-off-by: Numan Siddique 
---
 northd/en-sync-sb.c  | 447 +--
 northd/inc-proc-northd.c |   1 +
 northd/lflow-mgr.c   | 196 ++---
 northd/lflow-mgr.h   |  23 +-
 northd/northd.c  | 238 -
 northd/northd.h  |   6 -
 tests/ovn-northd.at  | 102 +
 7 files changed, 586 insertions(+), 427 deletions(-)

diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
index 7a5d2f0ccd..1cb4a58718 100644
--- a/northd/en-sync-sb.c
+++ b/northd/en-sync-sb.c
@@ -30,6 +30,7 @@
 #include "lib/ovn-nb-idl.h"
 #include "lib/ovn-sb-idl.h"
 #include "lib/ovn-util.h"
+#include "lflow-mgr.h"
 #include "northd.h"
 
 #include "openvswitch/vlog.h"
@@ -53,6 +54,38 @@ static void build_port_group_address_set(const struct 
nbrec_port_group *,
  struct svec *ipv4_addrs,
  struct svec *ipv6_addrs);
 
+struct sb_lb_table;
+struct sb_lb_record;
+static void sb_lb_table_init(struct sb_lb_table *);
+static void sb_lb_table_clear(struct sb_lb_table *);
+static struct sb_lb_record *sb_lb_table_find(struct hmap *sb_lbs,
+ const struct uuid *);
+static void sb_lb_table_build_and_sync(struct sb_lb_table *,
+struct ovsdb_idl_txn *ovnsb_txn,
+const struct sbrec_load_balancer_table *,
+const struct sbrec_logical_dp_group_table *,
+struct hmap *lb_dps_map,
+struct ovn_datapaths *ls_datapaths,
+struct ovn_datapaths *lr_datapaths,
+struct chassis_features *);
+static void sync_sb_lb_record(struct sb_lb_record *,
+  const struct sbrec_load_balancer *,
+   

[ovs-dev] [PATCH ovn v2 13/18] northd: Handle lb changes in lflow engine.

[numans]

From: Numan Siddique 

Since northd tracked data has the changed lb data, northd
engine handler for lflow engine now handles the lb changes
incrementally.  All the lflows generated for each lb is
stored in the ovn_lb_datapaths->lflow_ref and this is used
similar to how we handle ovn_port changes.

Signed-off-by: Numan Siddique 
---
 northd/en-lflow.c   | 11 +++---
 northd/lflow-mgr.c  | 41 ++-
 northd/lflow-mgr.h  |  3 ++
 northd/northd.c | 95 +
 northd/northd.h | 28 +
 tests/ovn-northd.at | 31 ++-
 6 files changed, 186 insertions(+), 23 deletions(-)

diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index 6c61b2afcb..038aa90295 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -125,11 +125,6 @@ lflow_northd_handler(struct engine_node *node,
 return false;
 }
 
-/* Fall back to recompute if load balancers have changed. */
-if (northd_has_lbs_in_tracked_data(_data->trk_northd_changes)) {
-return false;
-}
-
 const struct engine_context *eng_ctx = engine_get_context();
 struct lflow_data *lflow_data = data;
 
@@ -142,6 +137,12 @@ lflow_northd_handler(struct engine_node *node,
 return false;
 }
 
+if (!lflow_handle_northd_lb_changes(eng_ctx->ovnsb_idl_txn,
+_data->trk_northd_changes.trk_lbs,
+_input, lflow_data->lflow_table)) {
+return false;
+}
+
 engine_set_node_state(node, EN_UPDATED);
 
 return true;
diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
index 08962e9172..d779e7e087 100644
--- a/northd/lflow-mgr.c
+++ b/northd/lflow-mgr.c
@@ -105,6 +105,10 @@ static void ovn_dp_group_add_with_reference(struct 
ovn_lflow *,
 size_t bitmap_len);
 
 static void unlink_lflows_from_datapath(struct lflow_ref *);
+static void unlink_lflows_from_all_datapaths(struct lflow_ref *,
+ size_t n_ls_datapaths,
+ size_t n_lr_datapaths);
+
 static void lflow_ref_sync_lflows_to_sb__(struct lflow_ref  *,
 struct lflow_table *,
 struct ovsdb_idl_txn *ovnsb_txn,
@@ -394,6 +398,15 @@ lflow_ref_clear_lflows(struct lflow_ref *lflow_ref)
 unlink_lflows_from_datapath(lflow_ref);
 }
 
+void
+lflow_ref_clear_lflows_for_all_dps(struct lflow_ref *lflow_ref,
+   size_t n_ls_datapaths,
+   size_t n_lr_datapaths)
+{
+unlink_lflows_from_all_datapaths(lflow_ref, n_ls_datapaths,
+ n_lr_datapaths);
+}
+
 void
 lflow_ref_clear_and_sync_lflows(struct lflow_ref *lflow_ref,
 struct lflow_table *lflow_table,
@@ -462,7 +475,9 @@ lflow_table_add_lflow(struct lflow_table *lflow_table,
 /*  lflow_ref_node for this lflow doesn't exist yet.  Add it. */
 struct lflow_ref_node *ref_node = xzalloc(sizeof *ref_node);
 ref_node->lflow = lflow;
-ref_node->dp_index = od->index;
+if (od) {
+ref_node->dp_index = od->index;
+}
 ovs_list_insert(_ref->lflows_ref_list,
 _node->ref_list_node);
 
@@ -1047,6 +1062,30 @@ unlink_lflows_from_datapath(struct lflow_ref *lflow_ref)
 }
 }
 
+static void
+unlink_lflows_from_all_datapaths(struct lflow_ref *lflow_ref,
+ size_t n_ls_datapaths,
+ size_t n_lr_datapaths)
+{
+struct lflow_ref_node *ref_node;
+struct ovn_lflow *lflow;
+LIST_FOR_EACH (ref_node, ref_list_node, _ref->lflows_ref_list) {
+size_t n_datapaths;
+size_t index;
+
+lflow = ref_node->lflow;
+if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH) {
+n_datapaths = n_ls_datapaths;
+} else {
+n_datapaths = n_lr_datapaths;
+}
+
+BITMAP_FOR_EACH_1 (index, n_datapaths, lflow->dpg_bitmap) {
+bitmap_set0(lflow->dpg_bitmap, index);
+}
+}
+}
+
 static void
 lflow_ref_sync_lflows_to_sb__(struct lflow_ref  *lflow_ref,
 struct lflow_table *lflow_table,
diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h
index 02b74aa131..c65cd70e71 100644
--- a/northd/lflow-mgr.h
+++ b/northd/lflow-mgr.h
@@ -54,6 +54,9 @@ struct lflow_ref *lflow_ref_alloc(const char *res_name);
 void lflow_ref_destroy(struct lflow_ref *);
 void lflow_ref_reset(struct lflow_ref *lflow_ref);
 void lflow_ref_clear_lflows(struct lflow_ref *);
+void lflow_ref_clear_lflows_for_all_dps(struct lflow_ref *,
+size_t n_ls_datapaths,
+size_t n_lr_datapaths);
 void lflow_ref_clear_and_sync_lflows(struct lflow_ref *,
 struct 

[ovs-dev] [PATCH ovn v2 16/18] northd: Add I-P for NB_Global and SB_Global.

[numans]

From: Numan Siddique 

A new engine node "global_config" is added which handles the changes
to NB_Global an SB_Global tables.  It also creates these rows if
not present.

Without the I-P, any changes to the options column of these tables
result in recompute of 'northd' and 'lflow' engine nodes.

Signed-off-by: Numan Siddique 
---
 northd/aging.c|  21 +-
 northd/automake.mk|   2 +
 northd/en-global-config.c | 588 ++
 northd/en-global-config.h |  65 +
 northd/en-lflow.c |  11 +-
 northd/en-northd.c|  52 ++--
 northd/en-northd.h|   2 +-
 northd/en-sync-sb.c   |  22 +-
 northd/inc-proc-northd.c  |  38 ++-
 northd/northd.c   | 230 +++
 northd/northd.h   |  24 +-
 tests/ovn-northd.at   | 256 +++--
 12 files changed, 1014 insertions(+), 297 deletions(-)
 create mode 100644 northd/en-global-config.c
 create mode 100644 northd/en-global-config.h

diff --git a/northd/aging.c b/northd/aging.c
index f626c72c8c..cf988b39c4 100644
--- a/northd/aging.c
+++ b/northd/aging.c
@@ -15,6 +15,7 @@
 
 #include 
 
+#include "en-global-config.h"
 #include "lib/inc-proc-eng.h"
 #include "lib/ovn-nb-idl.h"
 #include "lib/ovn-sb-idl.h"
@@ -100,15 +101,10 @@ aging_context_handle_timestamp(struct aging_context *ctx, 
int64_t timestamp)
 static uint32_t
 get_removal_limit(struct engine_node *node, const char *name)
 {
-const struct nbrec_nb_global_table *nb_global_table =
-EN_OVSDB_GET(engine_get_input("NB_nb_global", node));
-const struct nbrec_nb_global *nb =
-nbrec_nb_global_table_first(nb_global_table);
-if (!nb) {
-return 0;
-}
+struct ed_type_global_config *global_config =
+engine_get_input_data("global_config", node);
 
-return smap_get_uint(>options, name, 0);
+return smap_get_uint(_config->nb_options, name, 0);
 }
 
 /* MAC binding aging */
@@ -142,11 +138,14 @@ en_mac_binding_aging_run(struct engine_node *node, void 
*data OVS_UNUSED)
 {
 const struct engine_context *eng_ctx = engine_get_context();
 struct northd_data *northd_data = engine_get_input_data("northd", node);
+struct ed_type_global_config *global_config =
+engine_get_input_data("global_config", node);
+
 struct aging_waker *waker =
 engine_get_input_data("mac_binding_aging_waker", node);
 
 if (!eng_ctx->ovnsb_idl_txn ||
-!northd_data->features.mac_binding_timestamp ||
+!global_config->features.mac_binding_timestamp ||
 time_msec() < waker->next_wake_msec) {
 return;
 }
@@ -271,9 +270,11 @@ en_fdb_aging_run(struct engine_node *node, void *data 
OVS_UNUSED)
 const struct engine_context *eng_ctx = engine_get_context();
 struct northd_data *northd_data = engine_get_input_data("northd", node);
 struct aging_waker *waker = engine_get_input_data("fdb_aging_waker", node);
+struct ed_type_global_config *global_config =
+engine_get_input_data("global_config", node);
 
 if (!eng_ctx->ovnsb_idl_txn ||
-!northd_data->features.fdb_timestamp ||
+!global_config->features.fdb_timestamp ||
 time_msec() < waker->next_wake_msec) {
 return;
 }
diff --git a/northd/automake.mk b/northd/automake.mk
index 9707be7d8e..482270f5dd 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -8,6 +8,8 @@ northd_ovn_northd_SOURCES = \
northd/northd.c \
northd/northd.h \
northd/ovn-northd.c \
+   northd/en-global-config.c \
+   northd/en-global-config.h \
northd/en-northd.c \
northd/en-northd.h \
northd/en-lflow.c \
diff --git a/northd/en-global-config.c b/northd/en-global-config.c
new file mode 100644
index 00..0d218f2ab5
--- /dev/null
+++ b/northd/en-global-config.c
@@ -0,0 +1,588 @@
+/*
+ * 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 
+
+/* OVS includes */
+#include "openvswitch/vlog.h"
+
+/* OVN includes */
+#include "debug.h"
+#include "en-global-config.h"
+#include "include/ovn/features.h"
+#include "ipam.h"
+#include "lib/ovn-nb-idl.h"
+#include "lib/ovn-sb-idl.h"
+#include "northd.h"
+
+
+VLOG_DEFINE_THIS_MODULE(en_global_config);
+
+/* static function declarations. */
+static void northd_enable_all_features(struct ed_type_global_config *);
+static void build_chassis_features(const struct sbrec_chassis_table *,
+   

[ovs-dev] [PATCH ovn v2 15/18] northd: Add ls_lbacls handler for lflow engine node.

[numans]

From: Numan Siddique 

Signed-off-by: Numan Siddique 
---
 northd/en-lflow.c| 30 +
 northd/en-lflow.h|  1 +
 northd/en-ls-lb-acls.c   |  3 +++
 northd/en-ls-lb-acls.h   | 23 
 northd/inc-proc-northd.c |  2 +-
 northd/northd.c  | 54 +
 northd/northd.h  |  5 
 tests/ovn-northd.at  | 58 ++--
 8 files changed, 150 insertions(+), 26 deletions(-)

diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index 11f9e9..831b90da75 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -195,6 +195,36 @@ lflow_lr_lb_nat_data_handler(struct engine_node *node, 
void *data)
 return true;
 }
 
+bool
+lflow_ls_lbacls_handler(struct engine_node *node, void *data)
+{
+struct ed_type_ls_lbacls *ls_lbacls_data =
+engine_get_input_data("ls_lbacls", node);
+
+if (!ls_lbacls_data->tracked ||
+!hmapx_is_empty(_lbacls_data->tracked_data.deleted)) {
+return false;
+}
+
+const struct engine_context *eng_ctx = engine_get_context();
+struct lflow_data *lflow_data = data;
+
+struct lflow_input lflow_input;
+lflow_get_input_data(node, _input);
+
+if (!lflow_handle_ls_lbacls_changes(eng_ctx->ovnsb_idl_txn,
+_lbacls_data->tracked_data,
+_input,
+lflow_data->lflow_table)) {
+return false;
+}
+
+
+engine_set_node_state(node, EN_UPDATED);
+
+return true;
+}
+
 void *en_lflow_init(struct engine_node *node OVS_UNUSED,
  struct engine_arg *arg OVS_UNUSED)
 {
diff --git a/northd/en-lflow.h b/northd/en-lflow.h
index adbd8767c9..7f4fe43d55 100644
--- a/northd/en-lflow.h
+++ b/northd/en-lflow.h
@@ -21,5 +21,6 @@ void en_lflow_cleanup(void *data);
 bool lflow_northd_handler(struct engine_node *, void *data);
 bool lflow_port_group_handler(struct engine_node *, void *data);
 bool lflow_lr_lb_nat_data_handler(struct engine_node *, void *data);
+bool lflow_ls_lbacls_handler(struct engine_node *node, void *data);
 
 #endif /* EN_LFLOW_H */
diff --git a/northd/en-ls-lb-acls.c b/northd/en-ls-lb-acls.c
index 1ba7ecb3e2..643c8caab2 100644
--- a/northd/en-ls-lb-acls.c
+++ b/northd/en-ls-lb-acls.c
@@ -39,6 +39,7 @@
 #include "lib/ovn-sb-idl.h"
 #include "lib/ovn-util.h"
 #include "lib/stopwatch-names.h"
+#include "lflow-mgr.h"
 #include "northd.h"
 
 VLOG_DEFINE_THIS_MODULE(en_ls_lbacls);
@@ -356,6 +357,7 @@ ls_lbacls_record_create(struct ls_lbacls_table *table,
 struct ls_lbacls_record *ls_lbacls_rec = xzalloc(sizeof *ls_lbacls_rec);
 ls_lbacls_rec->od = od;
 ls_lbacls_record_init(ls_lbacls_rec, od, NULL, ls_pgs);
+ls_lbacls_rec->lflow_ref = lflow_ref_alloc(od->nbs->name);
 
 hmap_insert(>entries, _lbacls_rec->key_node,
 uuid_hash(_lbacls_rec->od->nbs->header_.uuid));
@@ -366,6 +368,7 @@ ls_lbacls_record_create(struct ls_lbacls_table *table,
 static void
 ls_lbacls_record_destroy(struct ls_lbacls_record *ls_lbacls_rec)
 {
+lflow_ref_destroy(ls_lbacls_rec->lflow_ref);
 free(ls_lbacls_rec);
 }
 
diff --git a/northd/en-ls-lb-acls.h b/northd/en-ls-lb-acls.h
index ccb75e40e8..02ddcb59b4 100644
--- a/northd/en-ls-lb-acls.h
+++ b/northd/en-ls-lb-acls.h
@@ -31,6 +31,8 @@
 #include "lib/ovn-util.h"
 #include "lib/stopwatch-names.h"
 
+struct lflow_ref;
+
 struct ls_lbacls_record {
 struct hmap_node key_node;
 
@@ -39,6 +41,27 @@ struct ls_lbacls_record {
 bool has_lb_vip;
 bool has_acls;
 uint64_t max_acl_tier;
+
+/* 'lflow_ref' is used to reference logical flows generated for
+ * this ls_lbacls record.
+ *
+ * This data is initialized and destroyed by the en_ls_lbacls node,
+ * but populated and used only by the en_lflow node. Ideally this data
+ * should be maintained as part of en_lflow's data.  However, it would
+ * be less efficient and more complex:
+ *
+ * 1. It would require an extra search (using the index) to find the
+ * lflows.
+ *
+ * 2. Building the index needs to be thread-safe, using either a global
+ * lock which is obviously less efficient, or hash-based lock array which
+ * is more complex.
+ *
+ * Adding the lflow_ref here is more straightforward. The drawback is that
+ * we need to keep in mind that this data belongs to en_lflow node, so
+ * never access it from any other nodes.
+ */
+struct lflow_ref *lflow_ref;
 };
 
 struct ls_lbacls_table {
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 955b5e2ed1..5c3d9faafa 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -236,11 +236,11 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
 engine_add_input(_lflow, _sb_multicast_group, NULL);
 engine_add_input(_lflow, _sb_igmp_group, NULL);
 engine_add_input(_lflow, _sb_logical_dp_group, 

[ovs-dev] [PATCH ovn v2 17/18] northd: Add a noop handler for northd SB mac binding.

[numans]

From: Numan Siddique 

Signed-off-by: Numan Siddique 
---
 northd/inc-proc-northd.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 99ac1079ec..cdc4927deb 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -185,7 +185,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
 engine_add_input(_northd, _sb_mirror, NULL);
 engine_add_input(_northd, _sb_meter, NULL);
 engine_add_input(_northd, _sb_datapath_binding, NULL);
-engine_add_input(_northd, _sb_mac_binding, NULL);
 engine_add_input(_northd, _sb_dns, NULL);
 engine_add_input(_northd, _sb_ha_chassis_group, NULL);
 engine_add_input(_northd, _sb_ip_multicast, NULL);
@@ -196,6 +195,14 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
 engine_add_input(_northd, _global_config,
  northd_global_config_handler);
 
+/* northd engine node uses the sb mac binding table to
+ * cleanup mac binding entries for deleted logical ports
+ * and datapaths. Any update to to SB mac binding doesn't
+ * change the northd engine node state or data.  Hence
+ * it is ok to add a noop_handler here. */
+engine_add_input(_northd, _sb_mac_binding,
+ engine_noop_handler);
+
 engine_add_input(_northd, _sb_port_binding,
  northd_sb_port_binding_handler);
 engine_add_input(_northd, _nb_logical_switch,
-- 
2.41.0

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

[ovs-dev] [PATCH ovn v2 14/18] northd: Add lr_lb_nat_data handler for lflow engine node.

[numans]

From: Numan Siddique 

Signed-off-by: Numan Siddique 
---
 northd/en-lflow.c  |  30 +++
 northd/en-lflow.h  |   1 +
 northd/en-lr-lb-nat-data.c |  41 -
 northd/en-lr-lb-nat-data.h |  26 +++
 northd/inc-proc-northd.c   |   5 +-
 northd/northd.c| 365 +
 northd/northd.h|  10 +
 tests/ovn-northd.at|  48 +++--
 8 files changed, 335 insertions(+), 191 deletions(-)

diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index 038aa90295..11f9e9 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -165,6 +165,36 @@ lflow_port_group_handler(struct engine_node *node, void 
*data OVS_UNUSED)
 return true;
 }
 
+bool
+lflow_lr_lb_nat_data_handler(struct engine_node *node, void *data)
+{
+struct ed_type_lr_lb_nat_data *lr_lb_nat_data =
+engine_get_input_data("lr_lb_nat_data", node);
+
+if (!lr_lb_nat_data->tracked
+|| lr_lb_nat_data->tracked_data.vip_nats_changed
+|| !hmapx_is_empty(_lb_nat_data->tracked_data.deleted)) {
+return false;
+}
+
+const struct engine_context *eng_ctx = engine_get_context();
+struct lflow_data *lflow_data = data;
+
+struct lflow_input lflow_input;
+lflow_get_input_data(node, _input);
+
+if (!lflow_handle_lr_lb_nat_data_changes(eng_ctx->ovnsb_idl_txn,
+ _lb_nat_data->tracked_data,
+ _input,
+ lflow_data->lflow_table)) {
+return false;
+}
+
+engine_set_node_state(node, EN_UPDATED);
+
+return true;
+}
+
 void *en_lflow_init(struct engine_node *node OVS_UNUSED,
  struct engine_arg *arg OVS_UNUSED)
 {
diff --git a/northd/en-lflow.h b/northd/en-lflow.h
index f7325c56b1..adbd8767c9 100644
--- a/northd/en-lflow.h
+++ b/northd/en-lflow.h
@@ -20,5 +20,6 @@ void *en_lflow_init(struct engine_node *node, struct 
engine_arg *arg);
 void en_lflow_cleanup(void *data);
 bool lflow_northd_handler(struct engine_node *, void *data);
 bool lflow_port_group_handler(struct engine_node *, void *data);
+bool lflow_lr_lb_nat_data_handler(struct engine_node *, void *data);
 
 #endif /* EN_LFLOW_H */
diff --git a/northd/en-lr-lb-nat-data.c b/northd/en-lr-lb-nat-data.c
index d816d2321d..f33ed5040d 100644
--- a/northd/en-lr-lb-nat-data.c
+++ b/northd/en-lr-lb-nat-data.c
@@ -39,6 +39,7 @@
 #include "lib/ovn-sb-idl.h"
 #include "lib/ovn-util.h"
 #include "lib/stopwatch-names.h"
+#include "lflow-mgr.h"
 #include "northd.h"
 
 VLOG_DEFINE_THIS_MODULE(en_lr_lb_nat_data);
@@ -81,7 +82,7 @@ static void remove_lrouter_lb_reachable_ips(struct 
lr_lb_nat_data_record *,
 enum lb_neighbor_responder_mode,
 const struct sset *lb_ips_v4,
 const struct sset *lb_ips_v6);
-static void lr_lb_nat_data_build_vip_nats(struct lr_lb_nat_data_record *);
+static bool lr_lb_nat_data_build_vip_nats(struct lr_lb_nat_data_record *);
 
 /* 'lr_lb_nat_data' engine node manages the NB logical router LB data.
  */
@@ -119,6 +120,7 @@ en_lr_lb_nat_data_clear_tracked_data(void *data_)
 }
 
 hmapx_clear(>tracked_data.crupdated);
+data->tracked_data.vip_nats_changed = false;
 data->tracked = false;
 }
 
@@ -186,7 +188,6 @@ lr_lb_nat_data_lb_data_handler(struct engine_node *node, 
void *data_)
 const struct lr_nat_record *lrnat_rec = lr_nat_table_find_by_index(
 input_data.lr_nats, od->index);
 ovs_assert(lrnat_rec);
-
 lr_lbnat_rec = lr_lb_nat_data_record_create(>lr_lbnats,
 lrnat_rec,
 input_data.lb_datapaths_map,
@@ -194,6 +195,10 @@ lr_lb_nat_data_lb_data_handler(struct engine_node *node, 
void *data_)
 
 /* Add the lr_lbnat_rec rec to the tracking data. */
 hmapx_add(>tracked_data.crupdated, lr_lbnat_rec);
+
+if (!sset_is_empty(_lbnat_rec->vip_nats)) {
+data->tracked_data.vip_nats_changed = true;
+}
 continue;
 }
 
@@ -302,7 +307,9 @@ lr_lb_nat_data_lb_data_handler(struct engine_node *node, 
void *data_)
  * vip nats and re-evaluate 'has_lb_vip'. */
 HMAPX_FOR_EACH (hmapx_node, >tracked_data.crupdated) {
 lr_lbnat_rec = hmapx_node->data;
-lr_lb_nat_data_build_vip_nats(lr_lbnat_rec);
+if (lr_lb_nat_data_build_vip_nats(lr_lbnat_rec)) {
+data->tracked_data.vip_nats_changed = true;
+}
 lr_lbnat_rec->has_lb_vip = od_has_lb_vip(lr_lbnat_rec->od);
 }
 
@@ -341,8 +348,13 @@ lr_lb_nat_data_lr_nat_handler(struct engine_node *node, 
void *data_)
 lrnat_rec,
 input_data.lb_datapaths_map,
   

[ovs-dev] [PATCH ovn v2 12/18] northd: Move ovn_lb_datapaths from lib to northd module.

[numans]

From: Numan Siddique 

Signed-off-by: Numan Siddique 
---
 lib/lb.c|  96 -
 lib/lb.h|  57 -
 northd/northd.c | 111 
 northd/northd.h |  28 
 4 files changed, 139 insertions(+), 153 deletions(-)

diff --git a/lib/lb.c b/lib/lb.c
index d0d562b6fb..991f20299d 100644
--- a/lib/lb.c
+++ b/lib/lb.c
@@ -1071,99 +1071,3 @@ remove_ips_from_lb_ip_set(struct ovn_lb_ip_set *lb_ips,
 }
 }
 }
-
-/* lb datapaths functions */
-struct  ovn_lb_datapaths *
-ovn_lb_datapaths_create(const struct ovn_northd_lb *lb, size_t n_ls_datapaths,
-size_t n_lr_datapaths)
-{
-struct ovn_lb_datapaths *lb_dps = xzalloc(sizeof *lb_dps);
-lb_dps->lb = lb;
-lb_dps->nb_ls_map = bitmap_allocate(n_ls_datapaths);
-lb_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths);
-
-return lb_dps;
-}
-
-struct ovn_lb_datapaths *
-ovn_lb_datapaths_find(const struct hmap *lb_dps_map,
-  const struct uuid *lb_uuid)
-{
-struct ovn_lb_datapaths *lb_dps;
-size_t hash = uuid_hash(lb_uuid);
-HMAP_FOR_EACH_WITH_HASH (lb_dps, hmap_node, hash, lb_dps_map) {
-if (uuid_equals(_dps->lb->nlb->header_.uuid, lb_uuid)) {
-return lb_dps;
-}
-}
-return NULL;
-}
-
-void
-ovn_lb_datapaths_destroy(struct ovn_lb_datapaths *lb_dps)
-{
-bitmap_free(lb_dps->nb_lr_map);
-bitmap_free(lb_dps->nb_ls_map);
-free(lb_dps);
-}
-
-void
-ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths *lb_dps, size_t n,
-struct ovn_datapath **ods)
-{
-for (size_t i = 0; i < n; i++) {
-if (!bitmap_is_set(lb_dps->nb_lr_map, ods[i]->index)) {
-bitmap_set1(lb_dps->nb_lr_map, ods[i]->index);
-lb_dps->n_nb_lr++;
-}
-}
-}
-
-void
-ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *lb_dps, size_t n,
-struct ovn_datapath **ods)
-{
-for (size_t i = 0; i < n; i++) {
-if (!bitmap_is_set(lb_dps->nb_ls_map, ods[i]->index)) {
-bitmap_set1(lb_dps->nb_ls_map, ods[i]->index);
-lb_dps->n_nb_ls++;
-}
-}
-}
-
-struct ovn_lb_group_datapaths *
-ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group,
-  size_t max_ls_datapaths,
-  size_t max_lr_datapaths)
-{
-struct ovn_lb_group_datapaths *lb_group_dps =
-xzalloc(sizeof *lb_group_dps);
-lb_group_dps->lb_group = lb_group;
-lb_group_dps->ls = xmalloc(max_ls_datapaths * sizeof *lb_group_dps->ls);
-lb_group_dps->lr = xmalloc(max_lr_datapaths * sizeof *lb_group_dps->lr);
-
-return lb_group_dps;
-}
-
-void
-ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths *lb_group_dps)
-{
-free(lb_group_dps->ls);
-free(lb_group_dps->lr);
-free(lb_group_dps);
-}
-
-struct ovn_lb_group_datapaths *
-ovn_lb_group_datapaths_find(const struct hmap *lb_group_dps_map,
-const struct uuid *lb_group_uuid)
-{
-struct ovn_lb_group_datapaths *lb_group_dps;
-size_t hash = uuid_hash(lb_group_uuid);
-
-HMAP_FOR_EACH_WITH_HASH (lb_group_dps, hmap_node, hash, lb_group_dps_map) {
-if (uuid_equals(_group_dps->lb_group->uuid, lb_group_uuid)) {
-return lb_group_dps;
-}
-}
-return NULL;
-}
diff --git a/lib/lb.h b/lib/lb.h
index b8e3c1e8fb..90ac39ee5a 100644
--- a/lib/lb.h
+++ b/lib/lb.h
@@ -167,63 +167,6 @@ void ovn_lb_group_reinit(
 const struct nbrec_load_balancer_group *,
 const struct hmap *lbs);
 
-struct ovn_lb_datapaths {
-struct hmap_node hmap_node;
-
-const struct ovn_northd_lb *lb;
-size_t n_nb_ls;
-unsigned long *nb_ls_map;
-
-size_t n_nb_lr;
-unsigned long *nb_lr_map;
-};
-
-struct ovn_lb_datapaths *ovn_lb_datapaths_create(const struct ovn_northd_lb *,
- size_t n_ls_datapaths,
- size_t n_lr_datapaths);
-struct ovn_lb_datapaths *ovn_lb_datapaths_find(const struct hmap *,
-   const struct uuid *);
-void ovn_lb_datapaths_destroy(struct ovn_lb_datapaths *);
-void ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths *, size_t n,
- struct ovn_datapath **);
-void ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *, size_t n,
- struct ovn_datapath **);
-
-struct ovn_lb_group_datapaths {
-struct hmap_node hmap_node;
-
-const struct ovn_lb_group *lb_group;
-
-/* Datapaths to which 'lb_group' is applied. */
-size_t n_ls;
-struct ovn_datapath **ls;
-size_t n_lr;
-struct ovn_datapath **lr;
-};
-
-struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_create(
-const struct ovn_lb_group *, size_t max_ls_datapaths,
-size_t max_lr_datapaths);
-
-void 

[ovs-dev] [PATCH ovn v2 09/18] northd: Add a new node ls_lbacls.

[numans]

From: Numan Siddique 

This new engine now maintains the load balancer and ACL data of a
logical switch which was earlier part of northd engine node data.
The main inputs to this engine are:
- northd node
- NB logical switch node
- Port group node

A record for each logical switch is maintained in the 'ls_lbacls'
hmap table and this record stores the below data which was earlier
part of 'struct ovn_datapath'.

- bool has_stateful_acl;
- bool has_lb_vip;
- bool has_acls;
- uint64_t max_acl_tier;

This engine node becomes an input to 'lflow' node.

Signed-off-by: Numan Siddique 
---
 lib/stopwatch-names.h  |   1 +
 northd/automake.mk |   2 +
 northd/en-lflow.c  |   4 +
 northd/en-lr-lb-nat-data.c |   8 +-
 northd/en-lr-lb-nat-data.h |   2 +
 northd/en-ls-lb-acls.c | 527 +
 northd/en-ls-lb-acls.h |  88 +++
 northd/en-port-group.h |   3 +
 northd/inc-proc-northd.c   |   9 +
 northd/northd.c| 271 +--
 northd/northd.h|   7 +-
 11 files changed, 776 insertions(+), 146 deletions(-)
 create mode 100644 northd/en-ls-lb-acls.c
 create mode 100644 northd/en-ls-lb-acls.h

diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
index 7d85acdaea..8b0018a593 100644
--- a/lib/stopwatch-names.h
+++ b/lib/stopwatch-names.h
@@ -34,5 +34,6 @@
 #define SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run"
 #define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run"
 #define LR_LB_NAT_DATA_RUN_STOPWATCH_NAME "lr_lb_nat_data"
+#define LS_LBACLS_RUN_STOPWATCH_NAME "lr_lb_acls"
 
 #endif
diff --git a/northd/automake.mk b/northd/automake.mk
index 4116c487df..4593654726 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -28,6 +28,8 @@ northd_ovn_northd_SOURCES = \
northd/en-lr-nat.h \
northd/en-lr-lb-nat-data.c \
northd/en-lr-lb-nat-data.h \
+   northd/en-ls-lb-acls.c \
+   northd/en-ls-lb-acls.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
index 229f4be1d0..648a477916 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -21,6 +21,7 @@
 #include "en-lflow.h"
 #include "en-lr-nat.h"
 #include "en-lr-lb-nat-data.h"
+#include "en-ls-lb-acls.h"
 #include "en-northd.h"
 #include "en-meters.h"
 
@@ -44,6 +45,8 @@ lflow_get_input_data(struct engine_node *node,
 engine_get_input_data("sync_meters", node);
 struct ed_type_lr_lb_nat_data *lr_lb_nat_data =
 engine_get_input_data("lr_lb_nat_data", node);
+struct ed_type_ls_lbacls *ls_lbacls_data =
+engine_get_input_data("ls_lbacls", node);
 
 lflow_input->nbrec_bfd_table =
 EN_OVSDB_GET(engine_get_input("NB_bfd", node));
@@ -67,6 +70,7 @@ lflow_get_input_data(struct engine_node *node,
 lflow_input->lr_ports = _data->lr_ports;
 lflow_input->ls_port_groups = _data->ls_port_groups;
 lflow_input->lr_lbnats = _lb_nat_data->lr_lbnats;
+lflow_input->ls_lbacls = _lbacls_data->ls_lbacls;
 lflow_input->meter_groups = _meters_data->meter_groups;
 lflow_input->lb_datapaths_map = _data->lb_datapaths_map;
 lflow_input->svc_monitor_map = _data->svc_monitor_map;
diff --git a/northd/en-lr-lb-nat-data.c b/northd/en-lr-lb-nat-data.c
index 19b638ce0b..d816d2321d 100644
--- a/northd/en-lr-lb-nat-data.c
+++ b/northd/en-lr-lb-nat-data.c
@@ -299,9 +299,11 @@ lr_lb_nat_data_lb_data_handler(struct engine_node *node, 
void *data_)
 if (!hmapx_is_empty(>tracked_data.crupdated)) {
 struct hmapx_node *hmapx_node;
 /* For all the modified lr_lb_nat_data records (re)build the
- * vip nats. */
+ * vip nats and re-evaluate 'has_lb_vip'. */
 HMAPX_FOR_EACH (hmapx_node, >tracked_data.crupdated) {
-lr_lb_nat_data_build_vip_nats(hmapx_node->data);
+lr_lbnat_rec = hmapx_node->data;
+lr_lb_nat_data_build_vip_nats(lr_lbnat_rec);
+lr_lbnat_rec->has_lb_vip = od_has_lb_vip(lr_lbnat_rec->od);
 }
 
 data->tracked = true;
@@ -523,6 +525,8 @@ lr_lb_nat_data_record_init(struct lr_lb_nat_data_record 
*lr_lbnat_rec,
 if (!nbr->n_nat) {
 lr_lb_nat_data_build_vip_nats(lr_lbnat_rec);
 }
+
+lr_lbnat_rec->has_lb_vip = od_has_lb_vip(lr_lbnat_rec->od);
 }
 
 static struct lr_lb_nat_data_input
diff --git a/northd/en-lr-lb-nat-data.h b/northd/en-lr-lb-nat-data.h
index ffe41cad73..ac21d28a57 100644
--- a/northd/en-lr-lb-nat-data.h
+++ b/northd/en-lr-lb-nat-data.h
@@ -39,6 +39,8 @@ struct lr_lb_nat_data_record {
 const struct ovn_datapath *od;
 const struct lr_nat_record *lrnat_rec;
 
+bool has_lb_vip;
+
 /* Load Balancer vIPs relevant for this datapath. */
 struct ovn_lb_ip_set *lb_ips;
 
diff --git a/northd/en-ls-lb-acls.c b/northd/en-ls-lb-acls.c
new file mode 100644
index 00..1ba7ecb3e2
--- /dev/null
+++ b/northd/en-ls-lb-acls.c
@@ -0,0 +1,527 @@
+/*
+ * 

[ovs-dev] [PATCH ovn v2 07/18] northd: Generate logical router's LB and NAT flows using lr_lbnat_data.

[numans]

From: Numan Siddique 

Previous commits added new engine nodes to store logical router's lb
and NAT data.  Make use of the data stored by these engine nodes
to generate logical flows related to router's LBs and NATs.

Signed-off-by: Numan Siddique 
---
 northd/en-lflow.c  |   3 -
 northd/en-lr-lb-nat-data.h |   4 +
 northd/inc-proc-northd.c   |   1 -
 northd/northd.c| 752 -
 northd/northd.h|   1 -
 5 files changed, 496 insertions(+), 265 deletions(-)

diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index 9cb0ead3f0..229f4be1d0 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -42,8 +42,6 @@ lflow_get_input_data(struct engine_node *node,
 engine_get_input_data("port_group", node);
 struct sync_meters_data *sync_meters_data =
 engine_get_input_data("sync_meters", node);
-struct ed_type_lr_nat_data *lr_nat_data =
-engine_get_input_data("lr_nat", node);
 struct ed_type_lr_lb_nat_data *lr_lb_nat_data =
 engine_get_input_data("lr_lb_nat_data", node);
 
@@ -68,7 +66,6 @@ lflow_get_input_data(struct engine_node *node,
 lflow_input->ls_ports = _data->ls_ports;
 lflow_input->lr_ports = _data->lr_ports;
 lflow_input->ls_port_groups = _data->ls_port_groups;
-lflow_input->lr_nats = _nat_data->lr_nats;
 lflow_input->lr_lbnats = _lb_nat_data->lr_lbnats;
 lflow_input->meter_groups = _meters_data->meter_groups;
 lflow_input->lb_datapaths_map = _data->lb_datapaths_map;
diff --git a/northd/en-lr-lb-nat-data.h b/northd/en-lr-lb-nat-data.h
index 9029aee339..ffe41cad73 100644
--- a/northd/en-lr-lb-nat-data.h
+++ b/northd/en-lr-lb-nat-data.h
@@ -56,6 +56,10 @@ struct lr_lb_nat_data_table {
 #define LR_LB_NAT_DATA_TABLE_FOR_EACH(LR_LB_NAT_REC, TABLE) \
 HMAP_FOR_EACH (LR_LB_NAT_REC, key_node, &(TABLE)->entries)
 
+#define LR_LB_NAT_DATA_TABLE_FOR_EACH_IN_P(LR_LB_NAT_REC, JOBID, TABLE) \
+HMAP_FOR_EACH_IN_PARALLEL (LR_LB_NAT_REC, key_node, JOBID, \
+   &(TABLE)->entries)
+
 struct lr_lb_nat_data_tracked_data {
 /* Created or updated logical router with LB data. */
 struct hmapx crupdated; /* Stores 'struct lr_lb_nat_data_record'. */
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 369a151fa3..84627070a8 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -228,7 +228,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
 engine_add_input(_lflow, _sb_igmp_group, NULL);
 engine_add_input(_lflow, _northd, lflow_northd_handler);
 engine_add_input(_lflow, _port_group, lflow_port_group_handler);
-engine_add_input(_lflow, _lr_nat, NULL);
 engine_add_input(_lflow, _lr_lb_nat_data, NULL);
 
 engine_add_input(_sync_to_sb_addr_set, _nb_address_set,
diff --git a/northd/northd.c b/northd/northd.c
index 24df14c0de..1877cbc7df 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -8854,18 +8854,14 @@ build_lrouter_groups(struct hmap *lr_ports, struct 
ovs_list *lr_list)
  */
 static void
 build_lswitch_rport_arp_req_self_orig_flow(struct ovn_port *op,
-   uint32_t priority,
-   struct ovn_datapath *od,
-   const struct lr_nat_table *lr_nats,
-   struct hmap *lflows)
+uint32_t priority,
+const struct ovn_datapath *od,
+const struct lr_nat_record *lrnat_rec,
+struct hmap *lflows)
 {
 struct ds eth_src = DS_EMPTY_INITIALIZER;
 struct ds match = DS_EMPTY_INITIALIZER;
 
-const struct lr_nat_record *lrnat_rec = lr_nat_table_find_by_index(
-lr_nats, op->od->index);
-ovs_assert(lrnat_rec);
-
 /* Self originated ARP requests/RARP/ND need to be flooded to the L2 domain
  * (except on router ports).  Determine that packets are self originated
  * by also matching on source MAC. Matching on ingress port is not
@@ -8952,7 +8948,8 @@ lrouter_port_ipv6_reachable(const struct ovn_port *op,
  */
 static void
 build_lswitch_rport_arp_req_flow(const char *ips,
-int addr_family, struct ovn_port *patch_op, struct ovn_datapath *od,
+int addr_family, struct ovn_port *patch_op,
+const struct ovn_datapath *od,
 uint32_t priority, struct hmap *lflows,
 const struct ovsdb_idl_row *stage_hint)
 {
@@ -8993,8 +8990,6 @@ static void
 build_lswitch_rport_arp_req_flows(struct ovn_port *op,
   struct ovn_datapath *sw_od,
   struct ovn_port *sw_op,
-  const struct lr_nat_table *lr_nats,
-  const struct lr_lb_nat_data_table *lr_lbnats,
   struct hmap *lflows,
 

[ovs-dev] [PATCH ovn v2 08/18] northd: Don't commit dhcp response flows in the conntrack.

[numans]

From: Numan Siddique 

This is not required.

Signed-off-by: Numan Siddique 
---
 northd/northd.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 1877cbc7df..c8a224d3cd 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -9223,9 +9223,7 @@ build_dhcpv4_options_flows(struct ovn_port *op,
 >nbsp->dhcpv4_options->options, "lease_time");
 ovs_assert(server_id && server_mac && lease_time);
 const char *dhcp_actions =
-(op->od->has_stateful_acl || op->od->has_lb_vip)
- ? REGBIT_ACL_VERDICT_ALLOW" = 1; ct_commit; next;"
- : REGBIT_ACL_VERDICT_ALLOW" = 1; next;";
+REGBIT_ACL_VERDICT_ALLOW" = 1; next;";
 ds_clear();
 ds_put_format(, "outport == %s && eth.src == %s "
   "&& ip4.src == %s && udp && udp.src == 67 "
@@ -9308,9 +9306,7 @@ build_dhcpv6_options_flows(struct ovn_port *op,
 ipv6_string_mapped(server_ip, );
 
 const char *dhcp6_actions =
-(op->od->has_stateful_acl || op->od->has_lb_vip)
-? REGBIT_ACL_VERDICT_ALLOW" = 1; ct_commit; next;"
-: REGBIT_ACL_VERDICT_ALLOW" = 1; next;";
+REGBIT_ACL_VERDICT_ALLOW" = 1; next;";
 ds_clear();
 ds_put_format(, "outport == %s && eth.src == %s "
   "&& ip6.src == %s && udp && udp.src == 547 "
-- 
2.41.0

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

[ovs-dev] [PATCH ovn v2 06/18] northd: Add a new engine 'lr-lb-nat-data' to manage routers' lb data.

[numans]

From: Numan Siddique 

This new engine now maintains the load balancer and NAT data of a
logical router which was earlier part of northd engine node data.
The main inputs to this engine are:
   - northd node
   - lr-nat node

A record for each logical router is maintained in the 'lr_lb_nats'
hmap table and this record
   - stores the lb related data
   - embeds the 'lr-nat' record.

This engine node becomes an input to 'lflow' node.

Signed-off-by: Numan Siddique 
---
 lib/stopwatch-names.h  |   1 +
 northd/automake.mk |   2 +
 northd/en-lflow.c  |   4 +
 northd/en-lr-lb-nat-data.c | 654 +
 northd/en-lr-lb-nat-data.h |  93 ++
 northd/en-lr-nat.h |   3 +
 northd/en-sync-sb.c|  50 +--
 northd/inc-proc-northd.c   |  13 +-
 northd/northd.c| 640 
 northd/northd.h| 137 +++-
 tests/ovn-northd.at| 110 +--
 11 files changed, 1212 insertions(+), 495 deletions(-)
 create mode 100644 northd/en-lr-lb-nat-data.c
 create mode 100644 northd/en-lr-lb-nat-data.h

diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
index 0a16da211e..7d85acdaea 100644
--- a/lib/stopwatch-names.h
+++ b/lib/stopwatch-names.h
@@ -33,5 +33,6 @@
 #define PORT_GROUP_RUN_STOPWATCH_NAME "port_group_run"
 #define SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run"
 #define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run"
+#define LR_LB_NAT_DATA_RUN_STOPWATCH_NAME "lr_lb_nat_data"
 
 #endif
diff --git a/northd/automake.mk b/northd/automake.mk
index ae367a2a8b..4116c487df 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -26,6 +26,8 @@ northd_ovn_northd_SOURCES = \
northd/en-lb-data.h \
northd/en-lr-nat.c \
northd/en-lr-nat.h \
+   northd/en-lr-lb-nat-data.c \
+   northd/en-lr-lb-nat-data.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
index 22f398d419..9cb0ead3f0 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -20,6 +20,7 @@
 
 #include "en-lflow.h"
 #include "en-lr-nat.h"
+#include "en-lr-lb-nat-data.h"
 #include "en-northd.h"
 #include "en-meters.h"
 
@@ -43,6 +44,8 @@ lflow_get_input_data(struct engine_node *node,
 engine_get_input_data("sync_meters", node);
 struct ed_type_lr_nat_data *lr_nat_data =
 engine_get_input_data("lr_nat", node);
+struct ed_type_lr_lb_nat_data *lr_lb_nat_data =
+engine_get_input_data("lr_lb_nat_data", node);
 
 lflow_input->nbrec_bfd_table =
 EN_OVSDB_GET(engine_get_input("NB_bfd", node));
@@ -66,6 +69,7 @@ lflow_get_input_data(struct engine_node *node,
 lflow_input->lr_ports = _data->lr_ports;
 lflow_input->ls_port_groups = _data->ls_port_groups;
 lflow_input->lr_nats = _nat_data->lr_nats;
+lflow_input->lr_lbnats = _lb_nat_data->lr_lbnats;
 lflow_input->meter_groups = _meters_data->meter_groups;
 lflow_input->lb_datapaths_map = _data->lb_datapaths_map;
 lflow_input->svc_monitor_map = _data->svc_monitor_map;
diff --git a/northd/en-lr-lb-nat-data.c b/northd/en-lr-lb-nat-data.c
new file mode 100644
index 00..19b638ce0b
--- /dev/null
+++ b/northd/en-lr-lb-nat-data.c
@@ -0,0 +1,654 @@
+/*
+ * Copyright (c) 2023, Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+
+#include 
+#include 
+#include 
+
+/* OVS includes */
+#include "include/openvswitch/hmap.h"
+#include "lib/bitmap.h"
+#include "lib/socket-util.h"
+#include "lib/uuidset.h"
+#include "openvswitch/util.h"
+#include "openvswitch/vlog.h"
+#include "stopwatch.h"
+
+/* OVN includes */
+#include "en-lb-data.h"
+#include "en-lr-lb-nat-data.h"
+#include "en-lr-nat.h"
+#include "lib/inc-proc-eng.h"
+#include "lib/lb.h"
+#include "lib/ovn-nb-idl.h"
+#include "lib/ovn-sb-idl.h"
+#include "lib/ovn-util.h"
+#include "lib/stopwatch-names.h"
+#include "northd.h"
+
+VLOG_DEFINE_THIS_MODULE(en_lr_lb_nat_data);
+
+/* Static function declarations. */
+static void lr_lb_nat_data_table_init(struct lr_lb_nat_data_table *);
+static void lr_lb_nat_data_table_clear(struct lr_lb_nat_data_table *);
+static void lr_lb_nat_data_table_destroy(struct lr_lb_nat_data_table *);
+static struct lr_lb_nat_data_record *lr_lb_nat_data_table_find_(
+const struct lr_lb_nat_data_table *, const struct nbrec_logical_router *);
+static struct lr_lb_nat_data_record 

[ovs-dev] [PATCH ovn v2 05/18] northd: Add a new engine 'lr-nat' to manage lr NAT data.

[numans]

From: Numan Siddique 

This new engine now maintains the NAT related data for each
logical router which was earlier maintained by the northd
engine node in the 'struct ovn_datapath'.  Main inputs to
this engine node are:
   - northd
   - NB logical router

A record for each logical router is maintained in the 'lr_nats'
hmap table and this record
  - stores the ovn_nat's

Handlers are also added to handle the changes to both these
inputs.  This engine node becomes an input to 'lflow' node.
This essentially decouples the lr NAT data from the northd
engine node.

Signed-off-by: Numan Siddique 
---
 lib/ovn-util.c   |   6 +-
 lib/ovn-util.h   |   2 +-
 lib/stopwatch-names.h|   1 +
 northd/automake.mk   |   2 +
 northd/en-lflow.c|   5 +
 northd/en-lr-nat.c   | 498 +
 northd/en-lr-nat.h   | 134 ++
 northd/en-sync-sb.c  |  11 +-
 northd/inc-proc-northd.c |   9 +
 northd/northd.c  | 514 ++-
 northd/northd.h  |  32 ++-
 tests/ovn-northd.at  |  18 ++
 12 files changed, 877 insertions(+), 355 deletions(-)
 create mode 100644 northd/en-lr-nat.c
 create mode 100644 northd/en-lr-nat.h

diff --git a/lib/ovn-util.c b/lib/ovn-util.c
index 33105202f2..05e635a6b4 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -395,7 +395,7 @@ extract_sbrec_binding_first_mac(const struct 
sbrec_port_binding *binding,
 }
 
 bool
-lport_addresses_is_empty(struct lport_addresses *laddrs)
+lport_addresses_is_empty(const struct lport_addresses *laddrs)
 {
 return !laddrs->n_ipv4_addrs && !laddrs->n_ipv6_addrs;
 }
@@ -405,6 +405,10 @@ destroy_lport_addresses(struct lport_addresses *laddrs)
 {
 free(laddrs->ipv4_addrs);
 free(laddrs->ipv6_addrs);
+laddrs->ipv4_addrs = NULL;
+laddrs->ipv6_addrs = NULL;
+laddrs->n_ipv4_addrs = 0;
+laddrs->n_ipv6_addrs = 0;
 }
 
 /* Returns a string of the IP address of 'laddrs' that overlaps with 'ip_s'.
diff --git a/lib/ovn-util.h b/lib/ovn-util.h
index bff50dbde9..5805415885 100644
--- a/lib/ovn-util.h
+++ b/lib/ovn-util.h
@@ -112,7 +112,7 @@ bool extract_sbrec_binding_first_mac(const struct 
sbrec_port_binding *binding,
 bool extract_lrp_networks__(char *mac, char **networks, size_t n_networks,
 struct lport_addresses *laddrs);
 
-bool lport_addresses_is_empty(struct lport_addresses *);
+bool lport_addresses_is_empty(const struct lport_addresses *);
 void destroy_lport_addresses(struct lport_addresses *);
 const char *find_lport_address(const struct lport_addresses *laddrs,
const char *ip_s);
diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
index 3452cc71cf..0a16da211e 100644
--- a/lib/stopwatch-names.h
+++ b/lib/stopwatch-names.h
@@ -32,5 +32,6 @@
 #define LFLOWS_TO_SB_STOPWATCH_NAME "lflows_to_sb"
 #define PORT_GROUP_RUN_STOPWATCH_NAME "port_group_run"
 #define SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run"
+#define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run"
 
 #endif
diff --git a/northd/automake.mk b/northd/automake.mk
index cf622fc3c9..ae367a2a8b 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -24,6 +24,8 @@ northd_ovn_northd_SOURCES = \
northd/en-sync-from-sb.h \
northd/en-lb-data.c \
northd/en-lb-data.h \
+   northd/en-lr-nat.c \
+   northd/en-lr-nat.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
index 96d03b7ada..22f398d419 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -19,6 +19,7 @@
 #include 
 
 #include "en-lflow.h"
+#include "en-lr-nat.h"
 #include "en-northd.h"
 #include "en-meters.h"
 
@@ -40,6 +41,9 @@ lflow_get_input_data(struct engine_node *node,
 engine_get_input_data("port_group", node);
 struct sync_meters_data *sync_meters_data =
 engine_get_input_data("sync_meters", node);
+struct ed_type_lr_nat_data *lr_nat_data =
+engine_get_input_data("lr_nat", node);
+
 lflow_input->nbrec_bfd_table =
 EN_OVSDB_GET(engine_get_input("NB_bfd", node));
 lflow_input->sbrec_bfd_table =
@@ -61,6 +65,7 @@ lflow_get_input_data(struct engine_node *node,
 lflow_input->ls_ports = _data->ls_ports;
 lflow_input->lr_ports = _data->lr_ports;
 lflow_input->ls_port_groups = _data->ls_port_groups;
+lflow_input->lr_nats = _nat_data->lr_nats;
 lflow_input->meter_groups = _meters_data->meter_groups;
 lflow_input->lb_datapaths_map = _data->lb_datapaths_map;
 lflow_input->svc_monitor_map = _data->svc_monitor_map;
diff --git a/northd/en-lr-nat.c b/northd/en-lr-nat.c
new file mode 100644
index 00..0d332d300b
--- /dev/null
+++ b/northd/en-lr-nat.c
@@ -0,0 +1,498 @@
+/*
+ * Copyright (c) 2023, Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You 

[ovs-dev] [PATCH ovn v2 04/18] northd: Move router ports SB PB options sync to sync_to_sb_pb node.

[numans]

From: Numan Siddique 

It also moves the logical router port IPv6 prefix delegation
updates to "sync-from-sb" engine node.

Signed-off-by: Numan Siddique 
---
 northd/en-northd.c  |   2 +-
 northd/en-sync-sb.c |   3 +-
 northd/northd.c | 283 ++--
 northd/northd.h |   6 +-
 tests/ovn-northd.at |  31 -
 5 files changed, 198 insertions(+), 127 deletions(-)

diff --git a/northd/en-northd.c b/northd/en-northd.c
index 96c2ce9f69..13e731cad9 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -189,7 +189,7 @@ northd_sb_port_binding_handler(struct engine_node *node,
 northd_get_input_data(node, _data);
 
 if (!northd_handle_sb_port_binding_changes(
-input_data.sbrec_port_binding_table, >ls_ports)) {
+input_data.sbrec_port_binding_table, >ls_ports, >lr_ports)) {
 return false;
 }
 
diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
index 2540fcfb97..a14c609acd 100644
--- a/northd/en-sync-sb.c
+++ b/northd/en-sync-sb.c
@@ -288,7 +288,8 @@ en_sync_to_sb_pb_run(struct engine_node *node, void *data 
OVS_UNUSED)
 const struct engine_context *eng_ctx = engine_get_context();
 struct northd_data *northd_data = engine_get_input_data("northd", node);
 
-sync_pbs(eng_ctx->ovnsb_idl_txn, _data->ls_ports);
+sync_pbs(eng_ctx->ovnsb_idl_txn, _data->ls_ports,
+ _data->lr_ports);
 engine_set_node_state(node, EN_UPDATED);
 }
 
diff --git a/northd/northd.c b/northd/northd.c
index 9ce1b2cb5a..c9c7045755 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3419,6 +3419,9 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
 {
 sbrec_port_binding_set_datapath(op->sb, op->od->sb);
 if (op->nbrp) {
+/* Note: SB port binding options for router ports are set in
+ * sync_pbs(). */
+
 /* If the router is for l3 gateway, it resides on a chassis
  * and its port type is "l3gateway". */
 const char *chassis_name = smap_get(>od->nbr->options, "chassis");
@@ -3430,15 +3433,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
 sbrec_port_binding_set_type(op->sb, "patch");
 }
 
-struct smap new;
-smap_init();
 if (is_cr_port(op)) {
 ovs_assert(sbrec_chassis_by_name);
 ovs_assert(sbrec_chassis_by_hostname);
 ovs_assert(sbrec_ha_chassis_grp_by_name);
 ovs_assert(active_ha_chassis_grps);
-const char *redirect_type = smap_get(>nbrp->options,
- "redirect-type");
 
 if (op->nbrp->ha_chassis_group) {
 if (op->nbrp->n_gateway_chassis) {
@@ -3480,49 +3479,8 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
 /* Delete the legacy gateway_chassis from the pb. */
 sbrec_port_binding_set_gateway_chassis(op->sb, NULL, 0);
 }
-smap_add(, "distributed-port", op->nbrp->name);
-
-bool always_redirect =
-!op->od->has_distributed_nat &&
-!l3dgw_port_has_associated_vtep_lports(op->l3dgw_port);
-
-if (redirect_type) {
-smap_add(, "redirect-type", redirect_type);
-/* XXX Why can't we enable always-redirect when redirect-type
- * is bridged? */
-if (!strcmp(redirect_type, "bridged")) {
-always_redirect = false;
-}
-}
-
-if (always_redirect) {
-smap_add(, "always-redirect", "true");
-}
-} else {
-if (op->peer) {
-smap_add(, "peer", op->peer->key);
-if (op->nbrp->ha_chassis_group ||
-op->nbrp->n_gateway_chassis) {
-char *redirect_name =
-ovn_chassis_redirect_name(op->nbrp->name);
-smap_add(, "chassis-redirect-port", redirect_name);
-free(redirect_name);
-}
-}
-if (chassis_name) {
-smap_add(, "l3gateway-chassis", chassis_name);
-}
-}
-
-const char *ipv6_pd_list = smap_get(>sb->options,
-"ipv6_ra_pd_list");
-if (ipv6_pd_list) {
-smap_add(, "ipv6_ra_pd_list", ipv6_pd_list);
 }
 
-sbrec_port_binding_set_options(op->sb, );
-smap_destroy();
-
 sbrec_port_binding_set_parent_port(op->sb, NULL);
 sbrec_port_binding_set_tag(op->sb, NULL, 0);
 
@@ -4752,12 +4710,14 @@ check_sb_lb_duplicates(const struct 
sbrec_load_balancer_table *table)
 return duplicates;
 }
 
-/* Syncs the SB port binding for the ovn_port 'op'.  Caller should make sure
- * that the OVN SB IDL txn is not NULL.  Presently it only syncs the nat
- * column of port binding corresponding to the 'op->nbsp' */
+/* Syncs the SB port binding for the 

[ovs-dev] [PATCH ovn v2 03/18] tests: Add a couple of tests in ovn-northd for I-P.

[numans]

From: Numan Siddique 

These tests cover scenarios for load balancers and NATs
and check for the 'northd' and 'lflow' engine node
recompute and compute stats.

Signed-off-by: Numan Siddique 
---
 tests/ovn-northd.at | 274 
 1 file changed, 274 insertions(+)

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 28c293473c..699f6cfdce 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -10893,3 +10893,277 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
 
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([Load balancer incremental processing with stateless ACLs])
+ovn_start
+
+check_engine_stats() {
+  node=$1
+  recompute=$2
+  compute=$3
+
+  echo "__file__:__line__: Checking engine stats for node $node : recompute - \
+$recompute : compute - $compute"
+
+  node_stat=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats $node)
+  # node_stat will be of this format :
+  # - Node: lflow - recompute: 3 - compute: 0 - abort: 0
+  node_recompute_ct=$(echo $node_stat | cut -d '-' -f2 | cut -d ':' -f2)
+  node_compute_ct=$(echo $node_stat | cut -d '-' -f3 | cut -d ':' -f2)
+
+  if [[ "$recompute" == "norecompute" ]]; then
+# node should not be recomputed
+echo "Expecting $node recompute count - $node_recompute_ct to be 0"
+check test "$node_recompute_ct" -eq "0"
+  else
+echo "Expecting $node recompute count - $node_recompute_ct not to be 0"
+check test "$node_recompute_ct" -ne "0"
+  fi
+
+  if [[ "$compute" == "nocompute" ]]; then
+# node should not be computed
+echo "Expecting $node compute count - $node_compute_ct to be 0"
+check test "$node_compute_ct" -eq "0"
+  else
+echo "Expecting $node compute count - $node_compute_ct not to be 0"
+check test "$node_compute_ct" -ne "0"
+  fi
+}
+
+# Test I-P for load balancers.
+# Presently ovn-northd handles I-P for NB LBs in northd_lb_data engine node
+# only.
+check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
+check ovn-nbctl --wait=sb lb-add lb1 10.0.0.10:80 10.0.0.3:80
+
+check ovn-nbctl ls-add sw0
+check ovn-nbctl acl-add sw0 from-lport 1 1 allow-stateless
+check ovn-nbctl --wait=sb acl-add sw0 to-lport 1 1 allow-stateless
+
+check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
+check ovn-nbctl --wait=sb ls-lb-add sw0 lb1
+check_engine_stats lb_data norecompute compute
+check_engine_stats northd norecompute compute
+check_engine_stats lflow recompute nocompute
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+# Clear the VIPs of lb1
+check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
+check ovn-nbctl --wait=sb clear load_balancer . vips
+check_engine_stats lb_data norecompute compute
+check_engine_stats northd norecompute compute
+check_engine_stats lflow recompute nocompute
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
+check ovn-nbctl --wait=sb lb-del lb1
+check_engine_stats lb_data norecompute compute
+check_engine_stats northd recompute nocompute
+check_engine_stats lflow recompute nocompute
+CHECK_NO_CHANGE_AFTER_RECOMPUTE
+
+AT_CLEANUP
+])
+
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([Logical router incremental processing for NAT])
+
+ovn_start
+
+net_add n1
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.11
+
+check_engine_stats() {
+  node=$1
+  recompute=$2
+  compute=$3
+
+  echo "__file__:__line__: Checking engine stats for node $node : recompute - \
+$recompute : compute - $compute"
+
+  node_stat=$(as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats $node)
+  # node_stat will be of this format :
+  # - Node: lflow - recompute: 3 - compute: 0 - abort: 0
+  node_recompute_ct=$(echo $node_stat | cut -d '-' -f2 | cut -d ':' -f2)
+  node_compute_ct=$(echo $node_stat | cut -d '-' -f3 | cut -d ':' -f2)
+
+  if [[ "$recompute" == "norecompute" ]]; then
+# node should not be recomputed
+echo "Expecting $node recompute count - $node_recompute_ct to be 0"
+check test "$node_recompute_ct" -eq "0"
+  else
+echo "Expecting $node recompute count - $node_recompute_ct not to be 0"
+check test "$node_recompute_ct" -ne "0"
+  fi
+
+  if [[ "$compute" == "nocompute" ]]; then
+# node should not be computed
+echo "Expecting $node compute count - $node_compute_ct to be 0"
+check test "$node_compute_ct" -eq "0"
+  else
+echo "Expecting $node compute count - $node_compute_ct not to be 0"
+check test "$node_compute_ct" -ne "0"
+  fi
+}
+
+ovn-sbctl chassis-add gw1 geneve 127.0.0.1
+
+check ovn-nbctl ls-add sw0
+check ovn-nbctl lsp-add sw0 sw0p1 -- lsp-set-addresses sw0p1 
"00:00:20:20:12:01 10.0.0.4"
+
+check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
+check ovn-nbctl --wait=sb lr-add lr0
+check_engine_stats northd recompute nocompute
+check_engine_stats lflow recompute nocompute
+
+# Adding a logical router port should result in recompute
+check as northd ovn-appctl -t NORTHD_TYPE 

[ovs-dev] [PATCH ovn v2 02/18] northd: Track ovn_datapaths in northd engine track data.

[numans]

From: Numan Siddique 

northd engine tracked data now also stores the logical switches
and logical routers that got updated due to the changed load balancers.

Eg 1.  For this command 'ovn-nbctl ls-lb-add sw0 lb1 -- lr-lb-add lr0
lb1', northd engine tracking data will store 'sw0' and 'lr0'.

Eg 2.  If load balancer lb1 is already associated with 'sw0' and 'lr0'
then for this command 'ovn-nbctl set load_balancer 
vips:10.0.0.10=20.0.0.20', northd engine tracking data will store
'sw0' and 'lr0'.

An upcoming commit will make use of this tracked data.

Signed-off-by: Numan Siddique 
---
 northd/northd.c | 34 +-
 northd/northd.h | 12 
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/northd/northd.c b/northd/northd.c
index df22a9c658..9ce1b2cb5a 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -5146,6 +5146,8 @@ destroy_northd_data_tracked_changes(struct northd_data 
*nd)
 struct northd_tracked_data *trk_changes = >trk_northd_changes;
 destroy_tracked_ovn_ports(_changes->trk_ovn_ports);
 destroy_tracked_lbs(_changes->trk_lbs);
+hmapx_clear(_changes->ls_with_changed_lbs.crupdated);
+hmapx_clear(_changes->lr_with_changed_lbs.crupdated);
 nd->change_tracked = false;
 }
 
@@ -5158,6 +5160,8 @@ init_northd_tracked_data(struct northd_data *nd)
 hmapx_init(_changes->trk_ovn_ports.deleted);
 hmapx_init(_changes->trk_lbs.crupdated);
 hmapx_init(_changes->trk_lbs.deleted);
+hmapx_init(_changes->ls_with_changed_lbs.crupdated);
+hmapx_init(_changes->lr_with_changed_lbs.crupdated);
 }
 
 static void
@@ -5169,6 +5173,8 @@ destroy_northd_tracked_data(struct northd_data *nd)
 hmapx_destroy(_changes->trk_ovn_ports.deleted);
 hmapx_destroy(_changes->trk_lbs.crupdated);
 hmapx_destroy(_changes->trk_lbs.deleted);
+hmapx_destroy(_changes->ls_with_changed_lbs.crupdated);
+hmapx_destroy(_changes->lr_with_changed_lbs.crupdated);
 }
 
 
@@ -5179,7 +5185,10 @@ northd_has_tracked_data(struct northd_tracked_data 
*trk_nd_changes)
 || !hmapx_is_empty(_nd_changes->trk_ovn_ports.updated)
 || !hmapx_is_empty(_nd_changes->trk_ovn_ports.deleted)
 || !hmapx_is_empty(_nd_changes->trk_lbs.crupdated)
-|| !hmapx_is_empty(_nd_changes->trk_lbs.deleted));
+|| !hmapx_is_empty(_nd_changes->trk_lbs.deleted)
+|| !hmapx_is_empty(_nd_changes->ls_with_changed_lbs.crupdated)
+|| !hmapx_is_empty(_nd_changes->lr_with_changed_lbs.crupdated)
+);
 }
 
 bool
@@ -5188,6 +5197,8 @@ northd_has_only_ports_in_tracked_data(
 {
 return (hmapx_is_empty(_nd_changes->trk_lbs.crupdated)
 && hmapx_is_empty(_nd_changes->trk_lbs.deleted)
+&& hmapx_is_empty(_nd_changes->ls_with_changed_lbs.crupdated)
+&& hmapx_is_empty(_nd_changes->lr_with_changed_lbs.crupdated)
 && (!hmapx_is_empty(_nd_changes->trk_ovn_ports.created)
 || !hmapx_is_empty(_nd_changes->trk_ovn_ports.updated)
 || !hmapx_is_empty(_nd_changes->trk_ovn_ports.deleted)));
@@ -5828,6 +5839,9 @@ northd_handle_lb_data_changes(struct tracked_lb_data 
*trk_lb_data,
lb_dps->nb_ls_map) {
 od = ls_datapaths->array[index];
 init_lb_for_datapath(od);
+
+/* Add the ls datapath to the northd tracked data. */
+hmapx_add(_changes->ls_with_changed_lbs.crupdated, od);
 }
 
 hmap_remove(lb_datapaths_map, _dps->hmap_node);
@@ -5909,6 +5923,9 @@ northd_handle_lb_data_changes(struct tracked_lb_data 
*trk_lb_data,
 
 /* Re-evaluate 'od->has_lb_vip' */
 init_lb_for_datapath(od);
+
+/* Add the ls datapath to the northd tracked data. */
+hmapx_add(_changes->ls_with_changed_lbs.crupdated, od);
 }
 
 LIST_FOR_EACH (codlb, list_node, _lb_data->crupdated_lr_lbs) {
@@ -5954,6 +5971,9 @@ northd_handle_lb_data_changes(struct tracked_lb_data 
*trk_lb_data,
 
 /* Re-evaluate 'od->has_lb_vip' */
 init_lb_for_datapath(od);
+
+/* Add the lr datapath to the northd tracked data. */
+hmapx_add(_changes->lr_with_changed_lbs.crupdated, od);
 }
 
 HMAP_FOR_EACH (clb, hmap_node, _lb_data->crupdated_lbs) {
@@ -5968,6 +5988,9 @@ northd_handle_lb_data_changes(struct tracked_lb_data 
*trk_lb_data,
 od = ls_datapaths->array[index];
 /* Re-evaluate 'od->has_lb_vip' */
 init_lb_for_datapath(od);
+
+/* Add the ls datapath to the northd tracked data. */
+hmapx_add(_changes->ls_with_changed_lbs.crupdated, od);
 }
 
 BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths),
@@ -5991,6 +6014,9 @@ northd_handle_lb_data_changes(struct tracked_lb_data 
*trk_lb_data,
 add_neigh_ips_to_lrouter(od, lb->neigh_mode,
  >inserted_vips_v4,
  >inserted_vips_v6);
+
+  

[ovs-dev] [PATCH ovn v2 01/18] northd: Refactor the northd change tracking.

[numans]

From: Numan Siddique 

northd engine tracking data now has the following tracking data
  - changed ovn_ports (right now only changed logical switch ports are
tracked.)
  - changed load balancers.

This separation becomes easier to add lflow handling for these
changes in lflow northd engine handler.  This patch doesn't
handle the load balancer changes in lflow handler.  It will
be handled in upcoming commits.

Signed-off-by: Numan Siddique 
---
 northd/en-lflow.c   |  11 +-
 northd/en-northd.c  |  13 +-
 northd/en-sync-sb.c |  10 +-
 northd/northd.c | 446 
 northd/northd.h |  63 ---
 tests/ovn-northd.at |  10 +-
 6 files changed, 313 insertions(+), 240 deletions(-)

diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index 2b84fef0ef..96d03b7ada 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -108,8 +108,8 @@ lflow_northd_handler(struct engine_node *node,
 return false;
 }
 
-/* Fall back to recompute if lb related data has changed. */
-if (northd_data->lb_changed) {
+/* Fall back to recompute if load balancers have changed. */
+if (northd_has_lbs_in_tracked_data(_data->trk_northd_changes)) {
 return false;
 }
 
@@ -119,13 +119,14 @@ lflow_northd_handler(struct engine_node *node,
 struct lflow_input lflow_input;
 lflow_get_input_data(node, _input);
 
-if (!lflow_handle_northd_ls_changes(eng_ctx->ovnsb_idl_txn,
-_data->tracked_ls_changes,
-_input, _data->lflows)) {
+if (!lflow_handle_northd_port_changes(eng_ctx->ovnsb_idl_txn,
+_data->trk_northd_changes.trk_ovn_ports,
+_input, _data->lflows)) {
 return false;
 }
 
 engine_set_node_state(node, EN_UPDATED);
+
 return true;
 }
 
diff --git a/northd/en-northd.c b/northd/en-northd.c
index aa0f20f0c2..96c2ce9f69 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -230,15 +230,16 @@ northd_lb_data_handler(struct engine_node *node, void 
*data)
>ls_datapaths,
>lr_datapaths,
>lb_datapaths_map,
-   >lb_group_datapaths_map)) {
+   >lb_group_datapaths_map,
+   >trk_northd_changes)) {
 return false;
 }
 
-/* Indicate the depedendant engine nodes that load balancer/group
- * related data has changed (including association to logical
- * switch/router). */
-nd->lb_changed = true;
-engine_set_node_state(node, EN_UPDATED);
+if (northd_has_lbs_in_tracked_data(>trk_northd_changes)) {
+nd->change_tracked = true;
+engine_set_node_state(node, EN_UPDATED);
+}
+
 return true;
 }
 
diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
index 2ec3bf54f8..2540fcfb97 100644
--- a/northd/en-sync-sb.c
+++ b/northd/en-sync-sb.c
@@ -236,7 +236,8 @@ sync_to_sb_lb_northd_handler(struct engine_node *node, void 
*data OVS_UNUSED)
 {
 struct northd_data *nd = engine_get_input_data("northd", node);
 
-if (!nd->change_tracked || nd->lb_changed) {
+if (!nd->change_tracked ||
+northd_has_lbs_in_tracked_data(>trk_northd_changes)) {
 /* Return false if no tracking data or if lbs changed. */
 return false;
 }
@@ -306,11 +307,14 @@ sync_to_sb_pb_northd_handler(struct engine_node *node, 
void *data OVS_UNUSED)
 }
 
 struct northd_data *nd = engine_get_input_data("northd", node);
-if (!nd->change_tracked) {
+if (!nd->change_tracked ||
+northd_has_lbs_in_tracked_data(>trk_northd_changes)) {
+/* Return false if no tracking data or if lbs changed. */
 return false;
 }
 
-if (!sync_pbs_for_northd_ls_changes(>tracked_ls_changes)) {
+if (!sync_pbs_for_northd_changed_ovn_ports(
+>trk_northd_changes.trk_ovn_ports)) {
 return false;
 }
 
diff --git a/northd/northd.c b/northd/northd.c
index f8b046d83e..df22a9c658 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -4894,21 +4894,20 @@ sync_pbs(struct ovsdb_idl_txn *ovnsb_idl_txn, struct 
hmap *ls_ports)
 }
 
 /* Sync the SB Port bindings for the added and updated logical switch ports
- *  of the tracked logical switches (from the northd engine node). */
+ * of the tracked northd engine data. */
 bool
-sync_pbs_for_northd_ls_changes(struct tracked_ls_changes *ls_changes)
+sync_pbs_for_northd_changed_ovn_ports( struct tracked_ovn_ports *trk_ovn_ports)
 {
-struct ls_change *ls_change;
-LIST_FOR_EACH (ls_change, list_node, _changes->updated) {
-struct ovn_port *op;
-
-LIST_FOR_EACH (op, list, _change->added_ports) {
-sync_pb_for_op(op);
-}
+struct hmapx_node *hmapx_node;
+struct ovn_port *op;
+HMAPX_FOR_EACH 

[ovs-dev] [PATCH ovn v2 00/18] northd lflow incremental processing

[numans]

From: Numan Siddique 

This patch series adds incremental processing in the lflow engine
node to handle changes to northd and other engine nodes.
Changed related to load balancers and NAT are mainly handled in
this patch series.

This patch series can also be found here - 
https://github.com/numansiddique/ovn/tree/northd_lflow_ip/v1

Prior to this patch series, most of the changes to northd engine
resulted in full recomputation of logical flows.  This series
aims to improve the performance of ovn-northd by adding the I-P
support.  In order to add this support, some of the northd engine
node data (from struct ovn_datapath) is split and moved over to
new engine nodes - mainly related to load balancers, NAT and ACLs.

Below are the scale testing results done with these patches applied
using ovn-heater.  The test ran the scenario  -
ocp-500-density-heavy.yml [1].

With all the lflow I-P patches applied, the resuts are:

---
Min (s) Median (s)  90%ile (s)  99%ile 
(s)  Max (s) Mean (s)Total (s)   Count   Failed
---
Iteration Total 0.1368831.1290161.192001
1.2041671.2127280.66501783.127099   125 0
Namespace.add_ports 0.0052160.0057360.007034
0.0154860.0189780.0062110.776373125 0
WorkerNode.bind_port0.0350300.0460820.052469
0.0582930.0603110.04597311.493259   250 0
WorkerNode.ping_port0.0050570.0067271.047692
1.0692531.0713360.26689666.724094   250 0
---

The results with the present main are:

---
Min (s) Median (s)  90%ile (s)  99%ile 
(s)  Max (s) Mean (s)Total (s)   Count   Failed
---
Iteration Total 0.1354912.2238053.311270
3.3390783.3453461.729172216.146495  125 0
Namespace.add_ports 0.0053800.0057440.006819
0.0187730.0208000.0062920.786532125 0
WorkerNode.bind_port0.0341790.0460550.053488
0.0588010.0710430.04611711.529311   250 0
WorkerNode.ping_port0.0049560.0069523.086952
3.1917433.1928070.791544197.886026  250 0
---

[1] - 
https://github.com/ovn-org/ovn-heater/blob/main/test-scenarios/ocp-500-density-heavy.yml

v1 -> v2

   * Now also maintaing array indexes for ls_lbacls, lr_nat and
 lr_lb_nat_data tables (similar to ovn_datapaths->array) to
 make the lookup effecient.  The same ovn_datapath->index
 is reused.

   * Made some signficant changes to 'struct lflow_ref' in lflow-mgr.c.
 In v2 we don't use objdep_mgr to maintain the resource to lflow
 references.  Instead we maintain the 'struct lflow' pointer.
 With this we don't need to maintain additional hmap of lflows.


Numan Siddique (18):
  northd: Refactor the northd change tracking.
  northd: Track ovn_datapaths in northd engine track data.
  tests: Add a couple of tests in ovn-northd for I-P.
  northd: Move router ports SB PB options sync to sync_to_sb_pb node.
  northd: Add a new engine 'lr-nat' to manage lr NAT data.
  northd: Add a new engine 'lr-lb-nat-data' to manage routers' lb data.
  northd:  Generate logical router's LB and NAT flows using
lr_lbnat_data.
  northd: Don't commit dhcp response flows in the conntrack.
  northd: Add a new node ls_lbacls.
  northd: Refactor lflow management into a separate module.
  northd: Use lflow_ref when adding all logical flows.
  northd:  Move ovn_lb_datapaths from lib to northd module.
  northd: Handle lb changes in lflow engine.
  northd: Add lr_lb_nat_data handler for lflow engine node.
  northd: Add ls_lbacls handler for lflow engine node.
  northd:  Add I-P for NB_Global and SB_Global.
  northd: Add a noop handler for northd SB mac binding.
  northd: Add northd change handler 

[ovs-dev] [PATCH] readthedocs: Use dirhtml builder.

[Ilya Maximets]

We used this builder before, but from the project configuration
on the website.  ReadTheDocs doesn't allow to change it there
anymore and it doesn't allow to see the full name of the previously
used builder (!!), so I failed to migrate it to the config file.

The result is that older link like:
  https://docs.openvswitch.org/en/latest/howto/dpdk/
Now require .html:
  https://docs.openvswitch.org/en/latest/howto/dpdk.html

Fixing now by switching the builder back.

Fixes: e388bd73b70d ("readthedocs: Add the configuration file.")
Reported-by: Antonin Bas 
Reported-by: David Marchand 
Reported-at: https://github.com/openvswitch/ovs-issues/issues/310
Signed-off-by: Ilya Maximets 
---

The version of the docs with the change applied can be
temporarily seen here:
   https://igsilya-ovs.readthedocs.io/en/latest/

 .readthedocs.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.readthedocs.yaml b/.readthedocs.yaml
index e481e64f1..7d505150e 100644
--- a/.readthedocs.yaml
+++ b/.readthedocs.yaml
@@ -14,6 +14,7 @@ build:
 # Build documentation in the "Documentation/" directory with Sphinx.
 sphinx:
   configuration: Documentation/conf.py
+  builder: "dirhtml"
 
 # Build all formats: HTML, PDF, ePub.
 formats: all
-- 
2.41.0

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

Re: [ovs-dev] [PATCH ovn] northd: Support CIDR-based MAC binding aging threshold.

[Han Zhou]

On Wed, Oct 25, 2023 at 11:15 AM Ilya Maximets  wrote:
>
> On 10/24/23 21:35, Han Zhou wrote:
> > Enhance MAC_Binding aging to allow CIDR-based threshold configurations.
> > This enables distinct threshold settings for different IP ranges,
> > applying the longest prefix matching for overlapping ranges.
> >
> > A common use case involves setting a default threshold for all IPs,
while
> > disabling aging for a specific range and potentially excluding a subset
> > within that range.
> >
> > Signed-off-by: Han Zhou 
> > ---
> >  northd/aging.c  | 297 +---
> >  northd/aging.h  |   3 +
> >  northd/northd.c |  11 +-
> >  ovn-nb.xml  |  63 +-
> >  tests/ovn.at|  60 ++
> >  5 files changed, 413 insertions(+), 21 deletions(-)
> >
> > diff --git a/northd/aging.c b/northd/aging.c
> > index f626c72c8ca3..e5868211a63b 100644
> > --- a/northd/aging.c
> > +++ b/northd/aging.c
> > @@ -47,12 +47,253 @@ aging_waker_schedule_next_wake(struct aging_waker
*waker, int64_t next_wake_ms)
> >  }
> >  }
> >
> > +struct threshold_entry {
> > +union {
> > +ovs_be32 ipv4;
> > +struct in6_addr ipv6;
> > +} prefix;
> > +bool is_v4;
>
> Hi, Han.  Thanks for the patch!
>
> Not a full review, but I wonder if it would be cleaner to replace
> all the structure members above with a single 'struct in6_addr prefix;'
> and store ipv4 addresses as ipv6-mapped ipv4.  This will allow to use
> a single array for storing the entries as well.  May save some lines
> of code.
>
> What do you think?

Thanks Ilya for the review. In fact using a common in6_addr in a single
array was my first attempt, but then I realized that if there are both IPv4
and IPv6 entries in the array, for each mac-binding checking, say if it is
IPv4, it may have to go through all the IPv6 entries unnecessarily. If
there are a huge amount of MAC-bindings to check, the time may be doubled.
Probably this doesn't have a big impact, but using two arrays didn't seem
too many lines of code, so I went with the current *safe* approach. Let me
know if you have a strong preference for the alternative, and I am happy to
change it.

>
> Also, this patch, probably, needs a NEWS entry.

I was wondering if this change is big enough to add one. Now I will add it
since you brought up :).

Thanks,
Han

>
> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] python: Remove duplicate UnixctlClient implementation.

[Simon Horman]

On Thu, Oct 26, 2023 at 02:10:44PM +0200, jm...@redhat.com wrote:
> From: Jakob Meng 
> 
> The unixctl implementation in Python has been split into three parts in
> the past. During this process the UnixctlClient was duplicated, in
> python/ovs/unixctl/client.py and python/ovs/unixctl/server.py. This
> patch removes the duplicate from the latter.
> 
> Fixes: 53cf9963ccc6 ("python: Break unixctl implementation into re...")
> Signed-off-by: Jakob Meng 

Thanks Jakob,

I agree that this class is duplicated,
and that the duplication was introduced by the cited commit.

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

Re: [ovs-dev] [ovn] pinctrl: unexpected behavior LB health check with windows server

[Dumitru Ceara]

On 10/26/23 14:56, Evgenii Kovalev wrote:
> 
> On 18.10.2023 19:49, Evgenii Kovalev wrote:
>> Hi All,
>>

Hi Evgenii,

>> We met with unexpected behavior of health check in load balancer with
>> a windows server as backend, for example:
>> ovn-controller --> win_srv SYN
>> ovn-controller <-- win_srv SYN_ACK
>> ovn-controller --> win_srv RST_ACK
>> ovn-controller <-- win_srv SYN_ACK TCP Retransmission
>> ovn-controller --> win_srv RST_ACK
>> ovn-controller <-- win_srv SYN_ACK TCP Retransmission
>> ovn-controller --> win_srv RST_ACK
>> ovn-controller <-- win_srv RST
>>
>> Dump where 172.20.2.2 is ovn-controller and 172.20.2.49 is windows
>> server backend:
>> 17:18:20.324464 IP 172.20.2.2.29636 > 172.20.2.49.7045: Flags [S], seq
>> 2289025863, win 65160, length 0
>> 17:18:20.324572 IP 172.20.2.49.7045 > 172.20.2.2.29636: Flags [S.],
>> seq 2447380613, ack 2289025864, win 8192, options [mss 1460], length 0
>> 17:18:20.325233 IP 172.20.2.2.29636 > 172.20.2.49.7045: Flags [R.],
>> seq 2, ack 1, win 65160, length 0
>> 17:18:23.336091 IP 172.20.2.49.7045 > 172.20.2.2.29636: Flags [S.],
>> seq 2447380613, ack 2289025864, win 8192, options [mss 1460], length 0
>> 17:18:23.336559 IP 172.20.2.2.29636 > 172.20.2.49.7045: Flags [R.],
>> seq 2, ack 1, win 65160, length 0
>> 17:18:29.335992 IP 172.20.2.49.7045 > 172.20.2.2.29636: Flags [S.],
>> seq 2447380613, ack 2289025864, win 8192, options [mss 1460], length 0
>> 17:18:29.336423 IP 172.20.2.2.29636 > 172.20.2.49.7045: Flags [R.],
>> seq 2, ack 1, win 65160, length 0
>> 17:18:41.335919 IP 172.20.2.49.7045 > 172.20.2.2.29636: Flags [R], seq
>> 2447380614, win 0, length 0
>>
>> For the linux it doesn't affected because linux just ignore RST_ACK
>> and didn't made tcp retransmission from client side, it looks like:
>> ovn-controller --> linux_srv SYN
>> ovn-controller <-- linux_srv SYN_ACK
>> ovn-controller --> linux_srv RST_ACK
>> The linux client didn't made TCP retransmission.
>>
>> The main issue:
>> The status in svc_mon flapping between online and offline because of
>> behavior with a windows server backend. Every change for status in
>> svc_mon trigger ovn-northd and increase CPU usage of ovn-northd to 100%.
>>
>> I checked it on windows server with simple http server as backend and
>> this is behavior reproduced and looks like all backends with windows
>> affected.
>>
>> I want make a patch to fix this behavior, but I don't know which
>> method is preferred:
>>
>> 1) Add a new boolean field in struct svc_mon. After the func
>> svc_monitor_run() called with init/online/offline cases set this field
>> in the func svc_monitor_send_health_check(). If the func
>> process_packet_in() called in ACTION_OPCODE_HANDLE_SVC_CHECK case we
>> change the boolean field and send RST_ACK to client if packet_in with
>> SYN_ACK flag or we make a choice based on the new boolean field,
>> change or not the svc_mon->state field if packet_in with RST flag.
>> The func where we make decision:
>> https://github.com/ovn-org/ovn/blob/main/controller/pinctrl.c#L7810-L7858
>>

Personally, I'd try to avoid another configuration knob if we can.

>> 2) Implement established tcp connection, for example:
>> ovn-controller --> win_srv SYN
>> ovn-controller <-- win_srv SYN_ACK
>> ovn-controller --> win_srv ACK
>> ovn-controller --> win_srv FIN_ACK
>> ovn-controller <-- win_srv FIN_ACK
>> ovn-controller --> win_srv ACK
>>

Sounds easy but I'm quite sure we'll end up with quite a lot of code
because of all the potential cases with TCP establish/close handshakes.

>> 3) Send RST instead of RST_ACK
>>

Does this work in all cases on Linux too?  If so, +1 from me.

>> It will be great if you advice which method is better or I need fix it
>> on another way.
>> Thanks.
>>
> 
> Gentle ping.
> I'll appreciate for your advice.
> 

Sorry for the delay.

Regards,
Dumitru

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

Re: [ovs-dev] [ovn] pinctrl: unexpected behavior LB health check with windows server

[Evgenii Kovalev]

On 18.10.2023 19:49, Evgenii Kovalev wrote:

Hi All,

We met with unexpected behavior of health check in load balancer with a 
windows server as backend, for example:

ovn-controller --> win_srv SYN
ovn-controller <-- win_srv SYN_ACK
ovn-controller --> win_srv RST_ACK
ovn-controller <-- win_srv SYN_ACK TCP Retransmission
ovn-controller --> win_srv RST_ACK
ovn-controller <-- win_srv SYN_ACK TCP Retransmission
ovn-controller --> win_srv RST_ACK
ovn-controller <-- win_srv RST

Dump where 172.20.2.2 is ovn-controller and 172.20.2.49 is windows 
server backend:
17:18:20.324464 IP 172.20.2.2.29636 > 172.20.2.49.7045: Flags [S], seq 
2289025863, win 65160, length 0
17:18:20.324572 IP 172.20.2.49.7045 > 172.20.2.2.29636: Flags [S.], seq 
2447380613, ack 2289025864, win 8192, options [mss 1460], length 0
17:18:20.325233 IP 172.20.2.2.29636 > 172.20.2.49.7045: Flags [R.], seq 
2, ack 1, win 65160, length 0
17:18:23.336091 IP 172.20.2.49.7045 > 172.20.2.2.29636: Flags [S.], seq 
2447380613, ack 2289025864, win 8192, options [mss 1460], length 0
17:18:23.336559 IP 172.20.2.2.29636 > 172.20.2.49.7045: Flags [R.], seq 
2, ack 1, win 65160, length 0
17:18:29.335992 IP 172.20.2.49.7045 > 172.20.2.2.29636: Flags [S.], seq 
2447380613, ack 2289025864, win 8192, options [mss 1460], length 0
17:18:29.336423 IP 172.20.2.2.29636 > 172.20.2.49.7045: Flags [R.], seq 
2, ack 1, win 65160, length 0
17:18:41.335919 IP 172.20.2.49.7045 > 172.20.2.2.29636: Flags [R], seq 
2447380614, win 0, length 0


For the linux it doesn't affected because linux just ignore RST_ACK and 
didn't made tcp retransmission from client side, it looks like:

ovn-controller --> linux_srv SYN
ovn-controller <-- linux_srv SYN_ACK
ovn-controller --> linux_srv RST_ACK
The linux client didn't made TCP retransmission.

The main issue:
The status in svc_mon flapping between online and offline because of 
behavior with a windows server backend. Every change for status in 
svc_mon trigger ovn-northd and increase CPU usage of ovn-northd to 100%.


I checked it on windows server with simple http server as backend and 
this is behavior reproduced and looks like all backends with windows 
affected.


I want make a patch to fix this behavior, but I don't know which method 
is preferred:


1) Add a new boolean field in struct svc_mon. After the func 
svc_monitor_run() called with init/online/offline cases set this field 
in the func svc_monitor_send_health_check(). If the func 
process_packet_in() called in ACTION_OPCODE_HANDLE_SVC_CHECK case we 
change the boolean field and send RST_ACK to client if packet_in with 
SYN_ACK flag or we make a choice based on the new boolean field, change 
or not the svc_mon->state field if packet_in with RST flag.

The func where we make decision:
https://github.com/ovn-org/ovn/blob/main/controller/pinctrl.c#L7810-L7858

2) Implement established tcp connection, for example:
ovn-controller --> win_srv SYN
ovn-controller <-- win_srv SYN_ACK
ovn-controller --> win_srv ACK
ovn-controller --> win_srv FIN_ACK
ovn-controller <-- win_srv FIN_ACK
ovn-controller --> win_srv ACK

3) Send RST instead of RST_ACK

It will be great if you advice which method is better or I need fix it 
on another way.

Thanks.



Gentle ping.
I'll appreciate for your advice.

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

Re: [ovs-dev] [PATCH v2] editorconfig: Remove [*] section and trim_trailing_whitespace.

[Jakob Meng]

On 26.10.23 14:21, Robin Jarry wrote:
> Jakob Meng, Oct 26, 2023 at 14:17:
>> On 26.10.23 13:52, Robin Jarry wrote:
>> > , Oct 26, 2023 at 13:07:
>> >> From: Jakob Meng 
>> >>
>> >> Wildcard sections [*] and [**] are unsafe because properties cannot be
>> >> applied safely to any filetype in general. For example, IDEs like
>> >> Visual Studio Code and KDevelop store configuration files in subfolders
>> >> like .vscode or .kdev4. Properties from wildcard sections also apply to
>> >> those files which is not safe in general.
>> >> Another example are patches created with 'git format-patch' which can
>> >> contain trailing whitespaces. When editing a patch, e.g. to fix a typo
>> >> in the title, trailing whitespaces should not be removed.
>> >>
>> >> Property trim_trailing_whitespace should not be defined at all because
>> >> it is interpreted differently by editors. Some wipe whitespaces from
>> >> the whole file, others remove them from edited lines only and a few
>> >> change their behavior between releases [0]. Limiting the property to a
>> >> subset of files like *.c/*.h will not mitigate the issue:
>> >>
>> >> Multiple definitions of a whitespace exist. Unicode considers a form
>> >> feed (0x0C) to be a whitespace [1]. QChar::isSpace() [2] from Qt
>> >> follows this definition, causing the Kate editor identify a form feed
>> >> as a trailing whitespace and removing it from sources [3]. This breaks
>> >> patches when editors remove form feeds and thus causing broken patches
>> >> which cannot be applied cleanly.
>> >>
>> >> Removing trim_trailing_whitespace will be a minor inconvienence, in
>> >> particular because utilities/checkpatch.py and thus 0-day Robot will
>> >> prevent trailing whitespaces for our definition of a whitespace.
>> >>
>> >> [0] 
>> >> https://github.com/KDE/ktexteditor/commit/94b328fc64e543d91930845d2a96ce08d3043295
>> >> [1] https://en.wikipedia.org/wiki/Whitespace_character
>> >> [2] 
>> >> https://github.com/qt/qtbase/blob/5628600a07295db6ed6683e97fafb0c45ddea505/src/corelib/text/qchar.h#L554
>> >> [3] 
>> >> https://github.com/KDE/ktexteditor/blob/10210ec1dd06afa1e7b19a4fff722a8a23719161/src/document/katedocument.cpp#L5643
>> >>
>> >> Fixes: 07f6d6a0cb51 ("Add editorconfig file.")
>> >>
>> >> Signed-off-by: Jakob Meng 
>> >> ---
>> >>  .editorconfig | 34 +-
>> >>  1 file changed, 29 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/.editorconfig b/.editorconfig
>> >> index 685c72750..aebcf3a72 100644
>> >> --- a/.editorconfig
>> >> +++ b/.editorconfig
>> >> @@ -2,47 +2,71 @@
>> >>  
>> >>  root = true
>> >>  
>> >> -[*]
>> >> -end_of_line = lf
>> >> -insert_final_newline = true
>> >> -trim_trailing_whitespace = true
>> >> -charset = utf-8
>> >
>> > Hi Jakob,
>> >
>> > I think you could keep these two options:
>> >
>> > end_of_line = lf
>> > charset = utf-8
>> >
>>
>> You cannot decide this in general for all possible filetypes across all 
>> possible dev platforms. Again, those properties in [*] would also apply to 
>> non-ovs-owned files inside the source tree, e.g. created by IDEs. Unsafe. 
>> Please don't.
>
> Ok fair enough.
>
>> > And probably adding insert_final_newline = true is not necessary. > 
>> > checkpatch should complain if the final newline is missing.
>>
>> I left it in .editorconfig because it is not causing trouble. But I can 
>> remove it, if you want.
>
> I don't think it is important you can leave it or remove it.
>
> However I just realized that you could simply copy the settings in the 
> [*.{c,h}] section as other sections will inherit from it unless they override 
> something.

Oh, nice! IIUC this is actually covered by the EditorConfig spec [0]:

"All found EditorConfig files are searched for sections with section names 
matching the given filename."

and

"Files are read top to bottom and the most recent rules found take precedence. 
If multiple EditorConfig files have matching sections, the rules from the 
closer EditorConfig file are read last, so pairs in closer files take 
precedence."

and

"For any pair, a value of unset removes the effect of that pair, even if it has 
been set before. For example, add indent_size = unset to undefine the 
indent_size pair (and use editor defaults)."

The latter would not make sense if you could not inherit properties from other 
sections.

[0] https://spec.editorconfig.org/

>
>> > Your patch should only remove insert_final_newline and > 
>> > trim_trailing_whitespace from the default section.
>>
>>
>> >
>> >> +# No wildcard sections [*] and [**] because properties cannot be
>> >> +# applied safely to any filetype in general.
>> >> +
>> >> +# Property trim_trailing_whitespace should not be defined at all
>> >> +# because it is interpreted differently by editors.
>> >>  
>> >>  [*.{c,h}]
>> >> +charset = utf-8
>> >> +end_of_line = lf
>> >>  indent_style = space
>> >>  indent_size = 4
>> >> +insert_final_newline = true
>> >>  max_line_length = 79
>> >>  
>> >>  

Re: [ovs-dev] [PATCH v2] editorconfig: Remove [*] section and trim_trailing_whitespace.

[Robin Jarry]

Jakob Meng, Oct 26, 2023 at 14:17:

On 26.10.23 13:52, Robin Jarry wrote:
> , Oct 26, 2023 at 13:07:
>> From: Jakob Meng 
>>
>> Wildcard sections [*] and [**] are unsafe because properties cannot be
>> applied safely to any filetype in general. For example, IDEs like
>> Visual Studio Code and KDevelop store configuration files in subfolders
>> like .vscode or .kdev4. Properties from wildcard sections also apply to
>> those files which is not safe in general.
>> Another example are patches created with 'git format-patch' which can
>> contain trailing whitespaces. When editing a patch, e.g. to fix a typo
>> in the title, trailing whitespaces should not be removed.
>>
>> Property trim_trailing_whitespace should not be defined at all because
>> it is interpreted differently by editors. Some wipe whitespaces from
>> the whole file, others remove them from edited lines only and a few
>> change their behavior between releases [0]. Limiting the property to a
>> subset of files like *.c/*.h will not mitigate the issue:
>>
>> Multiple definitions of a whitespace exist. Unicode considers a form
>> feed (0x0C) to be a whitespace [1]. QChar::isSpace() [2] from Qt
>> follows this definition, causing the Kate editor identify a form feed
>> as a trailing whitespace and removing it from sources [3]. This breaks
>> patches when editors remove form feeds and thus causing broken patches
>> which cannot be applied cleanly.
>>
>> Removing trim_trailing_whitespace will be a minor inconvienence, in
>> particular because utilities/checkpatch.py and thus 0-day Robot will
>> prevent trailing whitespaces for our definition of a whitespace.
>>
>> [0] 
https://github.com/KDE/ktexteditor/commit/94b328fc64e543d91930845d2a96ce08d3043295
>> [1] https://en.wikipedia.org/wiki/Whitespace_character
>> [2] 
https://github.com/qt/qtbase/blob/5628600a07295db6ed6683e97fafb0c45ddea505/src/corelib/text/qchar.h#L554
>> [3] 
https://github.com/KDE/ktexteditor/blob/10210ec1dd06afa1e7b19a4fff722a8a23719161/src/document/katedocument.cpp#L5643
>>
>> Fixes: 07f6d6a0cb51 ("Add editorconfig file.")
>>
>> Signed-off-by: Jakob Meng 
>> ---
>>  .editorconfig | 34 +-
>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/.editorconfig b/.editorconfig
>> index 685c72750..aebcf3a72 100644
>> --- a/.editorconfig
>> +++ b/.editorconfig
>> @@ -2,47 +2,71 @@
>>  
>>  root = true
>>  
>> -[*]
>> -end_of_line = lf
>> -insert_final_newline = true
>> -trim_trailing_whitespace = true
>> -charset = utf-8
>
> Hi Jakob,
>
> I think you could keep these two options:
>
> end_of_line = lf
> charset = utf-8
>

You cannot decide this in general for all possible filetypes across 
all possible dev platforms. Again, those properties in [*] would also 
apply to non-ovs-owned files inside the source tree, e.g. created by 
IDEs. Unsafe. Please don't.


Ok fair enough.

> And probably adding insert_final_newline = true is not necessary. 
> checkpatch should complain if the final newline is missing.


I left it in .editorconfig because it is not causing trouble. But 
I can remove it, if you want.


I don't think it is important you can leave it or remove it.

However I just realized that you could simply copy the settings in the 
[*.{c,h}] section as other sections will inherit from it unless they 
override something.


> Your patch should only remove insert_final_newline and 
> trim_trailing_whitespace from the default section.



>
>> +# No wildcard sections [*] and [**] because properties cannot be
>> +# applied safely to any filetype in general.
>> +
>> +# Property trim_trailing_whitespace should not be defined at all
>> +# because it is interpreted differently by editors.
>>  
>>  [*.{c,h}]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = space
>>  indent_size = 4
>> +insert_final_newline = true
>>  max_line_length = 79
>>  
>>  [include/linux/**.h]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = tab
>>  indent_size = tab
>> +insert_final_newline = true
>>  tab_width = 8
>>  
>>  [include/sparse/rte_*.h]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = tab
>> +insert_final_newline = true
>>  tab_width = 8
>>  
>>  [include/windows/getopt.h]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = tab
>>  indent_size = tab
>> +insert_final_newline = true
>>  tab_width = 8
>>  
>>  [include/windows/netinet/{icmp6,ip6}.h]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = tab
>>  indent_size = tab
>> +insert_final_newline = true
>>  tab_width = 8
>>  
>>  [lib/getopt_long.c]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = tab
>>  indent_size = tab
>> +insert_final_newline = true
>>  tab_width = 8
>>  
>>  [lib/sflow*.{c,h}]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = tab
>>  indent_size = tab
>> +insert_final_newline = true
>>  tab_width = 8
>>  
>>  [lib/strsep.c]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = tab
>>  indent_size = tab
>> 

Re: [ovs-dev] [PATCH] tests/system-traffic: Ensure no name resolution for tcpdump.

[Eelco Chaudron]

On 26 Oct 2023, at 14:18, Eelco Chaudron wrote:

> On 21 Oct 2023, at 1:22, Frode Nordahl wrote:
>
>> Depending on system configuration, executing tcpdump without the
>> -n parameter, may prolong the execution time for tcpdump while it
>> attempts name resolution.
>>
>> This delay may in turn lead to test failures due to contents of
>> tables to check being evicted.
>>
>> We recently started to see this problem with the
>> "conntrack -IPv6 ICMP6 Related with SNAT" test.
>>
>> For consistency, this patch adds the -n parameter to all tcpdump
>> calls in system-traffic.at.
>>
>> Signed-off-by: Frode Nordahl 
>
> This change looks good to me.
>
> Acked-by: Eelco Chaudron 

Looks like I’m too late it was already applied ;)

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

Re: [ovs-dev] [PATCH] tests/system-traffic: Ensure no name resolution for tcpdump.

[Eelco Chaudron]

On 21 Oct 2023, at 1:22, Frode Nordahl wrote:

> Depending on system configuration, executing tcpdump without the
> -n parameter, may prolong the execution time for tcpdump while it
> attempts name resolution.
>
> This delay may in turn lead to test failures due to contents of
> tables to check being evicted.
>
> We recently started to see this problem with the
> "conntrack -IPv6 ICMP6 Related with SNAT" test.
>
> For consistency, this patch adds the -n parameter to all tcpdump
> calls in system-traffic.at.
>
> Signed-off-by: Frode Nordahl 

This change looks good to me.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v2] editorconfig: Remove [*] section and trim_trailing_whitespace.

[Jakob Meng]

On 26.10.23 13:52, Robin Jarry wrote:
> , Oct 26, 2023 at 13:07:
>> From: Jakob Meng 
>>
>> Wildcard sections [*] and [**] are unsafe because properties cannot be
>> applied safely to any filetype in general. For example, IDEs like
>> Visual Studio Code and KDevelop store configuration files in subfolders
>> like .vscode or .kdev4. Properties from wildcard sections also apply to
>> those files which is not safe in general.
>> Another example are patches created with 'git format-patch' which can
>> contain trailing whitespaces. When editing a patch, e.g. to fix a typo
>> in the title, trailing whitespaces should not be removed.
>>
>> Property trim_trailing_whitespace should not be defined at all because
>> it is interpreted differently by editors. Some wipe whitespaces from
>> the whole file, others remove them from edited lines only and a few
>> change their behavior between releases [0]. Limiting the property to a
>> subset of files like *.c/*.h will not mitigate the issue:
>>
>> Multiple definitions of a whitespace exist. Unicode considers a form
>> feed (0x0C) to be a whitespace [1]. QChar::isSpace() [2] from Qt
>> follows this definition, causing the Kate editor identify a form feed
>> as a trailing whitespace and removing it from sources [3]. This breaks
>> patches when editors remove form feeds and thus causing broken patches
>> which cannot be applied cleanly.
>>
>> Removing trim_trailing_whitespace will be a minor inconvienence, in
>> particular because utilities/checkpatch.py and thus 0-day Robot will
>> prevent trailing whitespaces for our definition of a whitespace.
>>
>> [0] 
>> https://github.com/KDE/ktexteditor/commit/94b328fc64e543d91930845d2a96ce08d3043295
>> [1] https://en.wikipedia.org/wiki/Whitespace_character
>> [2] 
>> https://github.com/qt/qtbase/blob/5628600a07295db6ed6683e97fafb0c45ddea505/src/corelib/text/qchar.h#L554
>> [3] 
>> https://github.com/KDE/ktexteditor/blob/10210ec1dd06afa1e7b19a4fff722a8a23719161/src/document/katedocument.cpp#L5643
>>
>> Fixes: 07f6d6a0cb51 ("Add editorconfig file.")
>>
>> Signed-off-by: Jakob Meng 
>> ---
>>  .editorconfig | 34 +-
>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/.editorconfig b/.editorconfig
>> index 685c72750..aebcf3a72 100644
>> --- a/.editorconfig
>> +++ b/.editorconfig
>> @@ -2,47 +2,71 @@
>>  
>>  root = true
>>  
>> -[*]
>> -end_of_line = lf
>> -insert_final_newline = true
>> -trim_trailing_whitespace = true
>> -charset = utf-8
>
> Hi Jakob,
>
> I think you could keep these two options:
>
> end_of_line = lf
> charset = utf-8
>

You cannot decide this in general for all possible filetypes across all 
possible dev platforms. Again, those properties in [*] would also apply to 
non-ovs-owned files inside the source tree, e.g. created by IDEs. Unsafe. 
Please don't.

> And probably adding insert_final_newline = true is not necessary. checkpatch 
> should complain if the final newline is missing.

I left it in .editorconfig because it is not causing trouble. But I can remove 
it, if you want.

> Your patch should only remove insert_final_newline and 
> trim_trailing_whitespace from the default section.


>
>> +# No wildcard sections [*] and [**] because properties cannot be
>> +# applied safely to any filetype in general.
>> +
>> +# Property trim_trailing_whitespace should not be defined at all
>> +# because it is interpreted differently by editors.
>>  
>>  [*.{c,h}]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = space
>>  indent_size = 4
>> +insert_final_newline = true
>>  max_line_length = 79
>>  
>>  [include/linux/**.h]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = tab
>>  indent_size = tab
>> +insert_final_newline = true
>>  tab_width = 8
>>  
>>  [include/sparse/rte_*.h]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = tab
>> +insert_final_newline = true
>>  tab_width = 8
>>  
>>  [include/windows/getopt.h]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = tab
>>  indent_size = tab
>> +insert_final_newline = true
>>  tab_width = 8
>>  
>>  [include/windows/netinet/{icmp6,ip6}.h]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = tab
>>  indent_size = tab
>> +insert_final_newline = true
>>  tab_width = 8
>>  
>>  [lib/getopt_long.c]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = tab
>>  indent_size = tab
>> +insert_final_newline = true
>>  tab_width = 8
>>  
>>  [lib/sflow*.{c,h}]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = tab
>>  indent_size = tab
>> +insert_final_newline = true
>>  tab_width = 8
>>  
>>  [lib/strsep.c]
>> +charset = utf-8
>> +end_of_line = lf
>>  indent_style = tab
>>  indent_size = tab
>> +insert_final_newline = true
>>  tab_width = 8
>> -- 
>> 2.39.2
>
>
>

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

Re: [ovs-dev] [PATCH] tests: Use ping timeout instead of deadline.

[Eelco Chaudron]

On 21 Oct 2023, at 17:04, Frode Nordahl wrote:

> Many system tests currently use ping with the combination of a
> low packet count (-c 3), short interval between sends (-i 0.3)
> and a _deadline_ of 2 seconds (-d 2).
>
> This combination of options may lead to a situation where more
> than count packets are sent however ping will stop when count
> packets are received. This results in a failed test due to how
> the result is checked, for example:
>
> ping6 -q -c 3 -i 0.3 -w 2 fc00::3 | FORMAT_PING
> @@ -1,2 +1,2 @@
> -3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +4 packets transmitted, 3 received, 25% packet loss, time 0ms
>
> To reiterate, in the above example there is no packet loss, but
> ping stops after _receiving_ 3 packets, not bothering with
> waiting for the response to the fourth packet it just sent out.
>
> If we look at the iputils ping manual for the -w deadline option
> we can read that this is expected behavior:
>
>> Specify a timeout, in seconds, before ping exits regardless of
>> how many packets have been sent or received. In this case ping
>> does not stop after count packet are sent, it waits either for
>> deadline expire or until count probes are answered or for some
>> error notification from network.
>
> To avoid these kinds of failures in checks where a response is
> expected, we replace ping -w with ping -W.
>
> We keep ping -w for checks where it is expected to NOT get a
> response.
>

Thanks for the patch, the change looks good to me, and seems to pass the dp 
tests.

Acked-by: Eelco Chaudron 

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

[ovs-dev] [PATCH] python: Remove duplicate UnixctlClient implementation.

[jmeng]

From: Jakob Meng 

The unixctl implementation in Python has been split into three parts in
the past. During this process the UnixctlClient was duplicated, in
python/ovs/unixctl/client.py and python/ovs/unixctl/server.py. This
patch removes the duplicate from the latter.

Fixes: 53cf9963ccc6 ("python: Break unixctl implementation into re...")
Signed-off-by: Jakob Meng 
---
 python/ovs/unixctl/server.py | 44 
 1 file changed, 44 deletions(-)

diff --git a/python/ovs/unixctl/server.py b/python/ovs/unixctl/server.py
index 5f9b3e739..b9cb52fad 100644
--- a/python/ovs/unixctl/server.py
+++ b/python/ovs/unixctl/server.py
@@ -211,47 +211,3 @@ class UnixctlServer(object):
  version)
 
 return 0, UnixctlServer(listener)
-
-
-class UnixctlClient(object):
-def __init__(self, conn):
-assert isinstance(conn, ovs.jsonrpc.Connection)
-self._conn = conn
-
-def transact(self, command, argv):
-assert isinstance(command, str)
-assert isinstance(argv, list)
-for arg in argv:
-assert isinstance(arg, str)
-
-request = Message.create_request(command, argv)
-error, reply = self._conn.transact_block(request)
-
-if error:
-vlog.warn("error communicating with %s: %s"
-  % (self._conn.name, os.strerror(error)))
-return error, None, None
-
-if reply.error is not None:
-return 0, str(reply.error), None
-else:
-assert reply.result is not None
-return 0, None, str(reply.result)
-
-def close(self):
-self._conn.close()
-self.conn = None
-
-@staticmethod
-def create(path):
-assert isinstance(path, str)
-
-unix = "unix:%s" % ovs.util.abs_file_name(ovs.dirs.RUNDIR, path)
-error, stream = ovs.stream.Stream.open_block(
-ovs.stream.Stream.open(unix))
-
-if error:
-vlog.warn("failed to connect to %s" % path)
-return error, None
-
-return 0, UnixctlClient(ovs.jsonrpc.Connection(stream))
-- 
2.39.2

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

Re: [ovs-dev] [PATCH] tests: Use ping timeout instead of deadline.

[Ilya Maximets]

On 10/26/23 08:04, Frode Nordahl wrote:
> On Wed, Oct 25, 2023 at 11:33 PM Ilya Maximets  wrote:
>>
>> On 10/25/23 11:45, Simon Horman wrote:
>>> On Sat, Oct 21, 2023 at 05:04:48PM +0200, Frode Nordahl wrote:
 Many system tests currently use ping with the combination of a
 low packet count (-c 3), short interval between sends (-i 0.3)
 and a _deadline_ of 2 seconds (-d 2).

 This combination of options may lead to a situation where more
 than count packets are sent however ping will stop when count
 packets are received. This results in a failed test due to how
 the result is checked, for example:

 ping6 -q -c 3 -i 0.3 -w 2 fc00::3 | FORMAT_PING
 @@ -1,2 +1,2 @@
 -3 packets transmitted, 3 received, 0% packet loss, time 0ms
 +4 packets transmitted, 3 received, 25% packet loss, time 0ms

 To reiterate, in the above example there is no packet loss, but
 ping stops after _receiving_ 3 packets, not bothering with
 waiting for the response to the fourth packet it just sent out.

 If we look at the iputils ping manual for the -w deadline option
 we can read that this is expected behavior:

> Specify a timeout, in seconds, before ping exits regardless of
> how many packets have been sent or received. In this case ping
> does not stop after count packet are sent, it waits either for
> deadline expire or until count probes are answered or for some
> error notification from network.

 To avoid these kinds of failures in checks where a response is
 expected, we replace ping -w with ping -W.

 We keep ping -w for checks where it is expected to NOT get a
 response.

 Signed-off-by: Frode Nordahl 
>>>
>>> Thanks Frode,
>>>
>>> TIL about -w and -W.
>>
>> I learned about -W as well. :)
>>
>> Thanks, Frode, for figuring out the cause of these failures!  I've seen
>> them before, but didn't dig too deep to find a cause.  OVN also has them
>> from time to time.
> 
> yw, we run all the system tests in an automated fashion as part of the
> openvswitch package regression testing, and we see them quite
> frequently, most likely due to the load of the CI infrastructure.
> 
>> Though I'm not sure if -W is the right choice.  Reading the description:
>>
>>-W timeout
>>Time to wait for a response, in seconds. The
>>option affects only timeout in absence of any
>>responses, otherwise ping waits for two RTTs.
>>Real number allowed with dot as a decimal
>>separator (regardless locale setup). 0 means
>>infinite timeout.
>>
>> And I don't really like the 'in absence of ANY responses' part of it.
>>
>> So, IIUC, if we send 3 packets, first gets replied and the other two
>> are dropped somewhere, ping will ignore the timeout and will wait
>> indefinitely.  Unfortunately, OVS gives the first packet a special
>> treatment, so potential for this scenario to happen is rather high.
>> This may break CI systems, getting them stuck testing one patch. And
>> it doesn't seem like we can mix -w and -W, at least the behavior is
>> not really defined in this case.
> 
> It also says "otherwise ping waits for two RTTs.", so it will not wait
> indefinitely. The documentation is a bit convoluted though so I went
> to look so that we can be sure about what it will do.
> 
> On arrival of the first packet, ping will gather various information
> [0] which will be used to compute the RTT [1], which is used when
> initializing the waittime [2][3].
> 
> So it appears to me -W would cover the scenario laid out above, i.e.
> if we get one reply quickly and the rest are lost, the computed RTT
> would have a ping exit within a reasonable timeframe. Even if the
> first response comes near the timeout value, the RTT would not be more
> than 6 seconds for a -W of 3.

Looks like I misread the docs.  Thanks for digging this through!
It does indeed look like it will not wait for too long.  I also
tested this with the following set of OpenFlow rules that allows
exactly one ICMP packet to go through and drops the others (the
interval should be a bit higher for this to work, because it
relies on revalidation to update the flows):

 table=0 priority=100 icmp 
actions=learn(table=0,priority=110,eth_type=0x0800,nw_proto=1,NXM_OF_ETH_SRC),normal
 table=0 priority=0 actions=normal

It does not wait indefinitely indeed.  However, I can't reproduce
the RTT thing.  In my testing it waits for an extra interval (-i)
instead.  But maybe it's because RTT is much lower than the interval.

> 
> 0: 
> https://github.com/iputils/iputils/blob/0cc6da796b9a64113152c071088701cb95a72ae8/ping/ping.c#L1654-L1660
> 1: 
> https://github.com/iputils/iputils/blob/0cc6da796b9a64113152c071088701cb95a72ae8/ping/ping_common.c#L761
> 2: 
> https://github.com/iputils/iputils/blob/0cc6da796b9a64113152c071088701cb95a72ae8/ping/ping_common.c#L599
> 3: 
> 

Re: [ovs-dev] [PATCH v2] editorconfig: Remove [*] section and trim_trailing_whitespace.

[Robin Jarry]

, Oct 26, 2023 at 13:07:

From: Jakob Meng 

Wildcard sections [*] and [**] are unsafe because properties cannot be
applied safely to any filetype in general. For example, IDEs like
Visual Studio Code and KDevelop store configuration files in subfolders
like .vscode or .kdev4. Properties from wildcard sections also apply to
those files which is not safe in general.
Another example are patches created with 'git format-patch' which can
contain trailing whitespaces. When editing a patch, e.g. to fix a typo
in the title, trailing whitespaces should not be removed.

Property trim_trailing_whitespace should not be defined at all because
it is interpreted differently by editors. Some wipe whitespaces from
the whole file, others remove them from edited lines only and a few
change their behavior between releases [0]. Limiting the property to a
subset of files like *.c/*.h will not mitigate the issue:

Multiple definitions of a whitespace exist. Unicode considers a form
feed (0x0C) to be a whitespace [1]. QChar::isSpace() [2] from Qt
follows this definition, causing the Kate editor identify a form feed
as a trailing whitespace and removing it from sources [3]. This breaks
patches when editors remove form feeds and thus causing broken patches
which cannot be applied cleanly.

Removing trim_trailing_whitespace will be a minor inconvienence, in
particular because utilities/checkpatch.py and thus 0-day Robot will
prevent trailing whitespaces for our definition of a whitespace.

[0] 
https://github.com/KDE/ktexteditor/commit/94b328fc64e543d91930845d2a96ce08d3043295
[1] https://en.wikipedia.org/wiki/Whitespace_character
[2] 
https://github.com/qt/qtbase/blob/5628600a07295db6ed6683e97fafb0c45ddea505/src/corelib/text/qchar.h#L554
[3] 
https://github.com/KDE/ktexteditor/blob/10210ec1dd06afa1e7b19a4fff722a8a23719161/src/document/katedocument.cpp#L5643

Fixes: 07f6d6a0cb51 ("Add editorconfig file.")

Signed-off-by: Jakob Meng 
---
 .editorconfig | 34 +-
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/.editorconfig b/.editorconfig
index 685c72750..aebcf3a72 100644
--- a/.editorconfig
+++ b/.editorconfig
@@ -2,47 +2,71 @@
 
 root = true
 
-[*]

-end_of_line = lf
-insert_final_newline = true
-trim_trailing_whitespace = true
-charset = utf-8


Hi Jakob,

I think you could keep these two options:

end_of_line = lf
charset = utf-8

And probably adding insert_final_newline = true is not necessary. 
checkpatch should complain if the final newline is missing.


Your patch should only remove insert_final_newline and 
trim_trailing_whitespace from the default section.


What do you think?


+# No wildcard sections [*] and [**] because properties cannot be
+# applied safely to any filetype in general.
+
+# Property trim_trailing_whitespace should not be defined at all
+# because it is interpreted differently by editors.
 
 [*.{c,h}]

+charset = utf-8
+end_of_line = lf
 indent_style = space
 indent_size = 4
+insert_final_newline = true
 max_line_length = 79
 
 [include/linux/**.h]

+charset = utf-8
+end_of_line = lf
 indent_style = tab
 indent_size = tab
+insert_final_newline = true
 tab_width = 8
 
 [include/sparse/rte_*.h]

+charset = utf-8
+end_of_line = lf
 indent_style = tab
+insert_final_newline = true
 tab_width = 8
 
 [include/windows/getopt.h]

+charset = utf-8
+end_of_line = lf
 indent_style = tab
 indent_size = tab
+insert_final_newline = true
 tab_width = 8
 
 [include/windows/netinet/{icmp6,ip6}.h]

+charset = utf-8
+end_of_line = lf
 indent_style = tab
 indent_size = tab
+insert_final_newline = true
 tab_width = 8
 
 [lib/getopt_long.c]

+charset = utf-8
+end_of_line = lf
 indent_style = tab
 indent_size = tab
+insert_final_newline = true
 tab_width = 8
 
 [lib/sflow*.{c,h}]

+charset = utf-8
+end_of_line = lf
 indent_style = tab
 indent_size = tab
+insert_final_newline = true
 tab_width = 8
 
 [lib/strsep.c]

+charset = utf-8
+end_of_line = lf
 indent_style = tab
 indent_size = tab
+insert_final_newline = true
 tab_width = 8
--
2.39.2




--
Robin Jarry
Principal Software Engineer
Red Hat, Telco/NFV

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

Re: [ovs-dev] [RFC v3 1/4] Add global option for JSON output to ovs-appctl/dpctl.

[Jakob Meng]

On 25.10.23 11:37, jm...@redhat.com wrote:
> From: Jakob Meng 
>
> For monitoring systems such as Prometheus it would be beneficial if
> OVS and OVS-DPDK would expose statistics in a machine-readable format.
>
> This patch introduces support for different output formats to ovs-xxx
> tools. They gain a global option '-f,--format' which allows users to
> request JSON instead of plain-text for humans. An example call
> implemented in a later patch is 'ovs-appctl --format json dpif/show'.
>
> For that it is necessary to change the JSON-RPC API lib/unixctl.*
> which ovs-xxx tools use to communicate with ovs-vswitchd across unix
> sockets. It now allows to transport the requested output format
> besides the command and its args. This change has been implemented in
> a backward compatible way. One can use an updated client
> (ovs-appctl/dpctl) with an old server (ovs-vswitchd) and vice versa.
> Of course, JSON output only works when both sides have been updated.
>
> Previously, the command was copied to the 'method' parameter in
> JSON-RPC while the args were copied to the 'params' parameter. Without
> any other means to transport parameters via JSON-RPC left, the meaning
> of 'method' and 'params' had to be changed: 'method' will now be
> 'execute/v1' when an output format other than 'text' is requested. In
> that case, the first parameter of the JSON array 'params' will now be
> the designated command, the second one the output format and the rest
> will be command args.

Ilya brought up the question why I changed the meaning of 'method' and 'params' 
instead of adding the output format as an addition argument to the command 
arguments in 'params'. The server side would then interpret and filter out this 
argument before passing the remaining arguments to the command callbacks.

I decided against this approach because the code would get more involved, in 
particular we would have to implement option/argument parsing inside 
process_command() in lib/unixctl.c. (Besides, using getopt*() functions in a 
safe way would be difficult in general because their global state.)
The current implementation is based purely on JSON objects/arrays which are 
nicely supported by OVS with functions from lib/json.*.

Ilya also voiced concerns about the limited extensibility of the proposed API. 
To fix this, this patch series could be tweaked in the follow way:

(1.) Instead of passing the output format as second entry in the JSON array 
'params', turn the second entry into a JSON object (shash). The output format 
would be one entry in this JSON object, e.g. called 'format'.

The server (process_command() from lib/unixctl.c) will parse the content of 
this JSON object into known options. Clients would only add non-default options 
to this JSON object to keep compatibility with older servers. Options which are 
supported by the server but not transferred via this JSON object would be 
initialized with default values to keep compatibility with older clients. 
Unknown entries would cause the server to return an error.

For (2.) see below.

>
> unixctl_command_register() in lib/unixctl.* has been cloned as
> unixctl_command_register_fmt() in order to demonstrate the new API.
> The latter will be replaced with the former in a later patch. The
> new function gained an int argument called 'output_fmts' with which
> commands have to announce their support for output formats.
>
> Function type unixctl_cb_func in lib/unixctl.h has gained a new arg
> 'enum ovs_output_fmt fmt'. For now, it has been added as a comment
> only for the huge changes reason mentioned earlier. The output format
> which is passed via unix socket to ovs-vswitchd will be converted into
> a value of type 'enum ovs_output_fmt' in lib/unixctl.c and then passed
> to said 'fmt' arg of the choosen command handler (in a future patch).
> When a requested output format is not supported by a command,
> then process_command() in lib/unixctl.c will return an error.

(2.) Instead of adding 'enum ovs_output_fmt fmt' as a new arg to function type 
unixctl_cb_func, an indirection will be used:

The enum value will be put into a struct and the struct will be added to 
unixctl_cb_func. This would allow us to add new options easily without having 
to change all command callbacks again.

Wdyt?

> This patch does not yet pass the choosen output format to commands.
> Doing so requires changes to all unixctl_command_register() calls and
> all command callbacks. To improve readibility those changes have been
> split out into a follow up patch. Respectively, whenever an output
> format other than 'text' is choosen for ovs-xxx tools, they will fail.
> By default, the output format is plain-text as before.
>
> In popular tools like kubectl the option for output control is usually
> called '-o|--output' instead of '-f,--format'. But ovs-appctl already
> has an short option '-o' which prints the available ovs-appctl options
> ('--option'). The now choosen name also better alines with 

[ovs-dev] [PATCH ovn v3] controller: Don't artificially limit group and meter IDs to 16bit.

[Dumitru Ceara]

OVS actually supports way more.  Detect the real number of groups and
meters instead. To avoid preallocating huge bitmaps for the IDs,
switch to id-pool instead (as suggested by Ilya).

Reported-at: https://issues.redhat.com/browse/FDP-70
Suggested-by: Ilya Maximets 
Signed-off-by: Dumitru Ceara 
---
V3:
- Addressed Ilya's comments:
  - removed references to 'bitmap' in comments
  - fixed indentation & typos
  - moved "id-pool.h" include to C file
  - extended the OVS feature detection component to also check for the
max number of groups.
V2:
- Addressed Ilya's comment: fixed the way id_pool_create() is called.
- renamed max_size to max_id, it's more accurate.
---
 controller/ofctrl.c |  2 +
 controller/ovn-controller.c |  6 +--
 include/ovn/features.h  |  3 ++
 lib/extend-table.c  | 81 --
 lib/extend-table.h  | 13 --
 lib/features.c  | 88 ++---
 tests/test-ovn.c|  4 +-
 7 files changed, 140 insertions(+), 57 deletions(-)

diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 63b0aa975b..06f8b33232 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -677,11 +677,13 @@ run_S_CLEAR_FLOWS(void)
 /* Clear existing groups, to match the state of the switch. */
 if (groups) {
 ovn_extend_table_clear(groups, true);
+ovn_extend_table_reinit(groups, ovs_feature_max_select_groups_get());
 }
 
 /* Clear existing meters, to match the state of the switch. */
 if (meters) {
 ovn_extend_table_clear(meters, true);
+ovn_extend_table_reinit(meters, ovs_feature_max_meters_get());
 ofctrl_meter_bands_clear();
 }
 
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index da7d145ed3..7a8ca38286 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -3948,8 +3948,8 @@ en_lflow_output_init(struct engine_node *node OVS_UNUSED,
 {
 struct ed_type_lflow_output *data = xzalloc(sizeof *data);
 ovn_desired_flow_table_init(>flow_table);
-ovn_extend_table_init(>group_table);
-ovn_extend_table_init(>meter_table);
+ovn_extend_table_init(>group_table, "group-table", 0);
+ovn_extend_table_init(>meter_table, "meter-table", 0);
 objdep_mgr_init(>lflow_deps_mgr);
 lflow_conj_ids_init(>conj_ids);
 uuidset_init(>objs_processed);
@@ -5656,7 +5656,7 @@ main(int argc, char *argv[])
 engine_set_force_recompute(true);
 }
 
-if (br_int) {
+if (br_int && ovs_feature_set_discovered()) {
 ct_zones_data = engine_get_data(_ct_zones);
 if (ct_zones_data && ofctrl_run(br_int, ovs_table,
 _zones_data->pending)) {
diff --git a/include/ovn/features.h b/include/ovn/features.h
index 7c3004b271..2c47ab766e 100644
--- a/include/ovn/features.h
+++ b/include/ovn/features.h
@@ -49,5 +49,8 @@ void ovs_feature_support_destroy(void);
 bool ovs_feature_is_supported(enum ovs_feature_value feature);
 bool ovs_feature_support_run(const struct smap *ovs_capabilities,
  const char *br_name);
+bool ovs_feature_set_discovered(void);
+uint32_t ovs_feature_max_meters_get(void);
+uint32_t ovs_feature_max_select_groups_get(void);
 
 #endif
diff --git a/lib/extend-table.c b/lib/extend-table.c
index ebb1a054cb..ed975a511e 100644
--- a/lib/extend-table.c
+++ b/lib/extend-table.c
@@ -17,9 +17,9 @@
 #include 
 #include 
 
-#include "bitmap.h"
 #include "extend-table.h"
 #include "hash.h"
+#include "id-pool.h"
 #include "lib/uuid.h"
 #include "openvswitch/vlog.h"
 
@@ -30,13 +30,29 @@ ovn_extend_table_delete_desired(struct ovn_extend_table 
*table,
 struct ovn_extend_table_lflow_to_desired *l);
 
 void
-ovn_extend_table_init(struct ovn_extend_table *table)
+ovn_extend_table_init(struct ovn_extend_table *table, const char *table_name,
+  uint32_t max_id)
 {
-table->table_ids = bitmap_allocate(MAX_EXT_TABLE_ID);
-bitmap_set1(table->table_ids, 0); /* table id 0 is invalid. */
-hmap_init(>desired);
-hmap_init(>lflow_to_desired);
-hmap_init(>existing);
+*table = (struct ovn_extend_table) {
+.name = table_name,
+.max_id = max_id,
+/* Table id 0 is invalid, set id-pool base to 1. */
+.table_ids = id_pool_create(1, max_id),
+.desired = HMAP_INITIALIZER(>desired),
+.lflow_to_desired = HMAP_INITIALIZER(>lflow_to_desired),
+.existing = HMAP_INITIALIZER(>existing),
+};
+}
+
+void
+ovn_extend_table_reinit(struct ovn_extend_table *table, uint32_t max_id)
+{
+ovn_extend_table_clear(table, true);
+if (max_id != table->max_id) {
+table->max_id = max_id;
+id_pool_destroy(table->table_ids);
+table->table_ids = id_pool_create(1, max_id);
+}
 }
 
 static struct ovn_extend_table_info *
@@ -117,13 +133,13 @@ 

Re: [ovs-dev] [PATCH] editorconfig: Leave *.patch and *.diff files untouched.

[Jakob Meng]

On 25.10.23 20:50, Jakob Meng wrote:
> On 25.10.23 15:48, Mike Pattrick wrote:
>> On Wed, Oct 25, 2023 at 9:02 AM Robin Jarry  wrote:
>>> Eelco Chaudron, Oct 25, 2023 at 14:56:
 On 25 Oct 2023, at 13:52, jm...@redhat.com wrote:

> From: Jakob Meng 
>
> A patch created with 'git format-patch' can contain trailing spaces.
> When editing a patch, e.g. to fix a typo in the title, the trailing
> spaces should not be removed. This becomes tricky when editors like
> Kate identify a space followed by form feed as a trailing space and
> remove both. This will result in a broken patch which cannot be applied
> cleanly. Although this case should likely be considered a editor bug,
> keeping trailing spaces in a patch is the right thing to do and also
> helps mitigating these kinds of editor bugs.
>
> Fixes: 07f6d6a0cb51 ("Add editorconfig file.")
 This looks good to me, however as you mention this is more of an
 editor configuration issue. It looks like leaving out any default
 value will cause the editor to use its configured value.

 Acked-by: Eelco Chaudron 
>>> It seems OK as well. But another safer option would have been to move
>>> the trim_trailing_whitespace = true option in specific sections. Or
>>> completely remove it since it will be caught by checkpatch.
>> I think it also makes sense to remove trim_trailing_whitespace from
>> the default section, but the current patch makes sense as a standalone
>> change.
>>
>> Acked-by: Mike Pattrick 
> Thank you all for your feedback! You are right, I will change my patch ☺️
>
> We should completely remove the default section because we cannot set any 
> reasonable defaults for all possible filetypes. For example, IDEs tend to 
> write their own files to a subfolder like .vscode or .kdev4. A default 
> section would apply to files in there, too, which is not safe in general.
>
> We also should not use insert_final_newline and trim_trailing_whitespace 
> anywhere in .editorconfig! Editors interpret these lines differently, some 
> will wipe whitespaces across the whole file, others will only remove them 
> from lines being edited and others change their behavior between releases. 
> Limiting those options to a subset like *.c/*.h will not help: As written in 
> my other response, the definition of whitespace is ambiguous. Unicode 
> considers form feed to be a whitespace [0], causing some editors to wipe form 
> feeds when trim_trailing_whitespace is true which is not what we want in OVS. 
> As Robin mentioned, we already have a test for our definition of whitespaces 
> and thus we can avoid this whitespace mess by not using it in .editorconfig.
>
> [0] https://en.wikipedia.org/wiki/Whitespace_character
>

Updated patch is out 壟

https://patchwork.ozlabs.org/project/openvswitch/patch/20231026110729.21961-1-jm...@redhat.com/

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

[ovs-dev] [PATCH v2] editorconfig: Remove [*] section and trim_trailing_whitespace.

[jmeng]

From: Jakob Meng 

Wildcard sections [*] and [**] are unsafe because properties cannot be
applied safely to any filetype in general. For example, IDEs like
Visual Studio Code and KDevelop store configuration files in subfolders
like .vscode or .kdev4. Properties from wildcard sections also apply to
those files which is not safe in general.
Another example are patches created with 'git format-patch' which can
contain trailing whitespaces. When editing a patch, e.g. to fix a typo
in the title, trailing whitespaces should not be removed.

Property trim_trailing_whitespace should not be defined at all because
it is interpreted differently by editors. Some wipe whitespaces from
the whole file, others remove them from edited lines only and a few
change their behavior between releases [0]. Limiting the property to a
subset of files like *.c/*.h will not mitigate the issue:

Multiple definitions of a whitespace exist. Unicode considers a form
feed (0x0C) to be a whitespace [1]. QChar::isSpace() [2] from Qt
follows this definition, causing the Kate editor identify a form feed
as a trailing whitespace and removing it from sources [3]. This breaks
patches when editors remove form feeds and thus causing broken patches
which cannot be applied cleanly.

Removing trim_trailing_whitespace will be a minor inconvienence, in
particular because utilities/checkpatch.py and thus 0-day Robot will
prevent trailing whitespaces for our definition of a whitespace.

[0] 
https://github.com/KDE/ktexteditor/commit/94b328fc64e543d91930845d2a96ce08d3043295
[1] https://en.wikipedia.org/wiki/Whitespace_character
[2] 
https://github.com/qt/qtbase/blob/5628600a07295db6ed6683e97fafb0c45ddea505/src/corelib/text/qchar.h#L554
[3] 
https://github.com/KDE/ktexteditor/blob/10210ec1dd06afa1e7b19a4fff722a8a23719161/src/document/katedocument.cpp#L5643

Fixes: 07f6d6a0cb51 ("Add editorconfig file.")

Signed-off-by: Jakob Meng 
---
 .editorconfig | 34 +-
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/.editorconfig b/.editorconfig
index 685c72750..aebcf3a72 100644
--- a/.editorconfig
+++ b/.editorconfig
@@ -2,47 +2,71 @@
 
 root = true
 
-[*]
-end_of_line = lf
-insert_final_newline = true
-trim_trailing_whitespace = true
-charset = utf-8
+# No wildcard sections [*] and [**] because properties cannot be
+# applied safely to any filetype in general.
+
+# Property trim_trailing_whitespace should not be defined at all
+# because it is interpreted differently by editors.
 
 [*.{c,h}]
+charset = utf-8
+end_of_line = lf
 indent_style = space
 indent_size = 4
+insert_final_newline = true
 max_line_length = 79
 
 [include/linux/**.h]
+charset = utf-8
+end_of_line = lf
 indent_style = tab
 indent_size = tab
+insert_final_newline = true
 tab_width = 8
 
 [include/sparse/rte_*.h]
+charset = utf-8
+end_of_line = lf
 indent_style = tab
+insert_final_newline = true
 tab_width = 8
 
 [include/windows/getopt.h]
+charset = utf-8
+end_of_line = lf
 indent_style = tab
 indent_size = tab
+insert_final_newline = true
 tab_width = 8
 
 [include/windows/netinet/{icmp6,ip6}.h]
+charset = utf-8
+end_of_line = lf
 indent_style = tab
 indent_size = tab
+insert_final_newline = true
 tab_width = 8
 
 [lib/getopt_long.c]
+charset = utf-8
+end_of_line = lf
 indent_style = tab
 indent_size = tab
+insert_final_newline = true
 tab_width = 8
 
 [lib/sflow*.{c,h}]
+charset = utf-8
+end_of_line = lf
 indent_style = tab
 indent_size = tab
+insert_final_newline = true
 tab_width = 8
 
 [lib/strsep.c]
+charset = utf-8
+end_of_line = lf
 indent_style = tab
 indent_size = tab
+insert_final_newline = true
 tab_width = 8
-- 
2.39.2

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

Re: [ovs-dev] [PATCH v7 1/8] system-dpdk: Introduce helpers for testpmd.

[Frode Nordahl]

tor. 26. okt. 2023, 12:34 skrev David Marchand :

> On Thu, Oct 26, 2023 at 10:10 AM Frode Nordahl
>  wrote:
> >
> > On Mon, Oct 23, 2023 at 10:19 AM David Marchand
> >  wrote:
> > >
> > > Rather than copy/paste everywhere, introduce helpers to control
> > > testpmd runs.
> > > Rely on --stats-period (which outputs port stats every n seconds) so
> that
> > > testpmd keeps running without expecting any user input.
> > >
> > > Signed-off-by: David Marchand 
> > > Acked-by: Aaron Conole 
> > > Acked-by: Eelco Chaudron 
> > > ---
> > > Changes since v1:
> > > - fixed OVS_DPDK_START_TESTPMD passed arguments evaluation:: $@ -> $1,
> >
> > Thanks for working on this, is there a plan to backport this?
>
> I did not test extensively with earlier versions of DPDK, but I would
> expect it to work.
> I am all for getting those unit tests ran in previous branches.


For our immediate needs a backport to branch-3.2 would be sufficient, as we
see it in our regression testing with DPDK 22.11.3 and OVS 3.2.

>
> > It appears that there is currently an issue with dpdk-testpmd that
> > makes it exit immediately when run in non-interactive mode [0], while
> > this is an upstream DPDK bug, it does affect the system-dpdk testsuite
> > for released versions of OVS. I suspect this DPDK commit broke it [1].
> >
> > 0: https://bugs.launchpad.net/bugs/2040097
> > 1:
> https://github.com/DPDK/dpdk/commit/0fd1386c30c3ad9365d7fdd2829bf7cb2e1b9dff
>
> Yes [1] changed testpmd behavior.
> The issue seems related to the tap driver as I don't reproduce the
> early quit when using some other virtual driver.
> I opened an upstream bug: https://bugs.dpdk.org/show_bug.cgi?id=1305.
>

Thanks!

--
Frode Nordahl


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

Re: [ovs-dev] [PATCH v7 1/8] system-dpdk: Introduce helpers for testpmd.

[David Marchand]

On Thu, Oct 26, 2023 at 10:10 AM Frode Nordahl
 wrote:
>
> On Mon, Oct 23, 2023 at 10:19 AM David Marchand
>  wrote:
> >
> > Rather than copy/paste everywhere, introduce helpers to control
> > testpmd runs.
> > Rely on --stats-period (which outputs port stats every n seconds) so that
> > testpmd keeps running without expecting any user input.
> >
> > Signed-off-by: David Marchand 
> > Acked-by: Aaron Conole 
> > Acked-by: Eelco Chaudron 
> > ---
> > Changes since v1:
> > - fixed OVS_DPDK_START_TESTPMD passed arguments evaluation:: $@ -> $1,
>
> Thanks for working on this, is there a plan to backport this?

I did not test extensively with earlier versions of DPDK, but I would
expect it to work.
I am all for getting those unit tests ran in previous branches.


>
> It appears that there is currently an issue with dpdk-testpmd that
> makes it exit immediately when run in non-interactive mode [0], while
> this is an upstream DPDK bug, it does affect the system-dpdk testsuite
> for released versions of OVS. I suspect this DPDK commit broke it [1].
>
> 0: https://bugs.launchpad.net/bugs/2040097
> 1: 
> https://github.com/DPDK/dpdk/commit/0fd1386c30c3ad9365d7fdd2829bf7cb2e1b9dff

Yes [1] changed testpmd behavior.
The issue seems related to the tap driver as I don't reproduce the
early quit when using some other virtual driver.
I opened an upstream bug: https://bugs.dpdk.org/show_bug.cgi?id=1305.


-- 
David Marchand

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

Re: [ovs-dev] [PATCH v6 0/3] netdev: Sync and clean {get, set}_config() callbacks.

[Jakob Meng]

On 25.10.23 19:10, Ilya Maximets wrote:
> On 10/24/23 11:21, Kevin Traynor wrote:
>> Using correct email for Simon this time
>>
>> On 24/10/2023 10:19, Kevin Traynor wrote:
>>> On 23/10/2023 10:11, Jakob Meng wrote:
 On 20.10.23 12:02, Kevin Traynor wrote:
> On 13/10/2023 10:07, jm...@redhat.com wrote:
>> From: Jakob Meng 
>>
>> For better usability, the function pairs get_config() and
>> set_config() for netdevs should be symmetric: Options which are
>> accepted by set_config() should be returned by get_config() and the
>> latter should output valid options for set_config() only.
>>
>> This patch series moves key-value pairs which are no valid options
>> from get_config() to the get_status() callback. The documentation in
>> vswitchd/vswitch.xml for status columns as well as tests have been
>> updated accordingly.
>>
>> Compared to v4, the patch has been split into a series. Change requests
>> from Kevin Traynor have been incorporated, e.g. n_{r,t}xq will be
>> reported in dpkvhostclient status and tx-steering in the dpdk status
>> will be "unsupported" if the hw does not support steering traffic to
>> additional rxq.
>> The netdev dpdk classes no longer share a common get_config callback,
>> instead both the dpdk_class and the dpdk_vhost_client_class defines
>> their own callbacks. 
> Looks like you've lost the callback for the the vhost-user server ports.
> (I noticed that in the code, but I didn't do a full review, so replying here.)

With "vhost-user server ports" you meant dpdk_vhost_class?

The get_config callback for dpdk_vhost_class has been dropped because it does 
not have a set_config callback.

>
>> For dpdk_vhost_client_class both config options
>> vhost-server-path and tx-retries-max are returned which were missed in
>> the previous patch version.
>>
>> Jakob Meng (3):
>>      netdev-dpdk: Sync and clean {get,set}_config() callbacks.
>>      netdev-dummy: Sync and clean {get,set}_config() callbacks.
>>      netdev-afxdp: Sync and clean {get,set}_config() callbacks.
>>
>>     Documentation/intro/install/afxdp.rst |  12 +--
>>     Documentation/topics/dpdk/phy.rst |   4 +-
>>     lib/netdev-afxdp.c    |  21 +-
>>     lib/netdev-afxdp.h    |   1 +
>>     lib/netdev-dpdk.c | 104 
>> ++
>>     lib/netdev-dummy.c    |  19 -
>>     lib/netdev-linux-private.h    |   1 +
>>     lib/netdev-linux.c    |   4 +-
>>     tests/pmd.at  |  26 +++
>>     tests/system-dpdk.at  |  64 ++--
>>     vswitchd/vswitch.xml  |  25 ++-
>>     11 files changed, 193 insertions(+), 88 deletions(-)
>>
> Hi Jakob,
>
> The output looks good to me. Just a few minor code related comments on 
> the code.
>
> @previous reviewers/committers, if anyone else is intending to review 
> before Jakob respins for possibly the final version, please shout now!
>
> As it is user visible change, it's probably worth to put a note in the 
> NEWS so users can quickly spot if they notice a change.
>
> Best to mention the commands/output that changed ('ovs-appctl dpctl/show' 
> and 'ovs-vsctl get Interface  status') and say briefly 
> that you've aligned set_confg/get_config and updated status etc.
>
> Suggest to not to bother mentioning specific netdevs and just do in one 
> update, maybe in first patch?
>
> thanks,
> Kevin.
 Good idea. How about appending this to NEWS?

 Post-v3.2.0
 
      - OVSDB:
    * ...
      - ovs-appctl:
    * 'ovs-appctl dpctl/show' has been changed to print valid config 
 options
> It is an appctl section, no need to repeat.
>
      only: {configured,requested}_{rx,tx}_queues and 'rx_csum_offload' 
 have
      been dropped. Now, 'n_rxq' will be used for requested rx queues. 
 Tx
      queues cannot be changed by the user, hence 'requested_tx_queues' 
 has
      been dropped. 'lsc_interrupt_mode' has been renamed to option name
      'dpdk-lsc-interrupt'.
      Use 'ovs-vsctl get interface *** status' to query for values 
 which have
      previously been returned by 'ovs-appctl dpctl/show'. For example, 
 use
      'ovs-vsctl get interface *** status:n_txq' to get what was 
 previously
      returned by 'configured_tx_queues'.
       * 'ovs-appctl dpctl/show' will now print all available config 
 options like
     'dpdk-devargs', '{rx,tx}-flow-ctrl-{autoneg}', 'vhost-server-path' 
 and
     'tx-retries-max' if they are set.
> This doesn't seem to be entirely true.  The flow 

Re: [ovs-dev] [PATCH ovn] controller: fixed potential segfault when changing tunnel_key and deleting ls

[Xavier Simonart]

This patch fixes the segfault but might cause a memory leak. I'll send a v2
There is also an (unrelated) flaky test ("Check default openflow
flows") which will be fixed later in a different patch

Thanks
Xavier


On Mon, Oct 23, 2023 at 12:04 PM Xavier Simonart  wrote:
>
> When a tunnel_key for a datapath was changed, the local_datapaths hmap was 
> not properly updated
> as the old/initial entry was not removed.
> - If the datapath was not deleted at the same time, a new entry (for the same 
> dp) was created
>   in the local_datapaths as the previous entry was not found (wrong hash).
> - If the datapath was deleted at the same time, the former entry also 
> remained (as, again, the hash
>   was wrong). So, we kept a deleted (dp) entry in the hmap, and might crash 
> when we used it later.
>
> When tunnel_key is updated for an existing datapath, we now clean the 
> local_datapaths,
> removing bad entries (i.e entries for which the hash is not the tunnel_key).
>
> This issue was identified by flaky failures of test "ovn-ic -- route deletion 
> upon TS deletion".
>
> Backtrace:
> 0  0x00504a9a in hmap_first_with_hash (hmap=0x20f4d90, 
> hmap@entry=0x5d5634, hmap=0x20f4d90, hmap@entry=0x5d5634, hash=1956414673) at 
> ./include/openvswitch/hmap.h:360
> 1  smap_find__ (smap=smap@entry=0x20f4d90, key=key@entry=0x5d5634 
> "snat-ct-zone", key_len=key_len@entry=12, hash=1956414673) at lib/smap.c:421
> 2  0x00505073 in smap_get_node (key=0x5d5634 "snat-ct-zone", 
> smap=0x20f4d90) at lib/smap.c:217
> 3  smap_get_def (def=0x0, key=0x5d5634 "snat-ct-zone", smap=0x20f4d90) at 
> lib/smap.c:208
> 4  smap_get (smap=0x20f4d90, key=key@entry=0x5d5634 "snat-ct-zone") at 
> lib/smap.c:200
> 5  0x00419898 in get_common_nat_zone (ldp=0x20a8c40, ldp=0x20a8c40) 
> at controller/lflow.c:831
> 6  add_matches_to_flow_table (lflow=lflow@entry=0x21ddf90, 
> ldp=ldp@entry=0x20a8c40, matches=matches@entry=0x211bb50, 
> ptable=ptable@entry=10 '\n', output_ptable=output_ptable@entry=37 '%', 
> ovnacts=ovnacts@entry=0x7ffd19b99de0,
> ingress=true, l_ctx_in=0x7ffd19b9a300, l_ctx_out=0x7ffd19b9a2c0) at 
> controller/lflow.c:892
> 7  0x0041a29b in consider_logical_flow__ 
> (lflow=lflow@entry=0x21ddf90, dp=0x20eb720, 
> l_ctx_in=l_ctx_in@entry=0x7ffd19b9a300, 
> l_ctx_out=l_ctx_out@entry=0x7ffd19b9a2c0) at controller/lflow.c:1207
> 8  0x0041a587 in consider_logical_flow (lflow=lflow@entry=0x21ddf90, 
> is_recompute=is_recompute@entry=false, 
> l_ctx_in=l_ctx_in@entry=0x7ffd19b9a300, 
> l_ctx_out=l_ctx_out@entry=0x7ffd19b9a2c0) at controller/lflow.c:1276
> 9  0x0041e30f in lflow_handle_changed_flows 
> (l_ctx_in=l_ctx_in@entry=0x7ffd19b9a300, 
> l_ctx_out=l_ctx_out@entry=0x7ffd19b9a2c0) at controller/lflow.c:271
> 10 0x00439fc7 in lflow_output_sb_logical_flow_handler 
> (node=0x7ffd19ba5b70, data=) at 
> controller/ovn-controller.c:4045
> 11 0x004682fe in engine_compute (recompute_allowed=, 
> node=) at lib/inc-proc-eng.c:441
> 12 engine_run_node (recompute_allowed=true, node=0x7ffd19ba5b70) at 
> lib/inc-proc-eng.c:503
> 13 engine_run (recompute_allowed=recompute_allowed@entry=true) at 
> lib/inc-proc-eng.c:528
> 14 0x0040ade2 in main (argc=, argv=) at 
> controller/ovn-controller.c:5709
>
> Signed-off-by: Xavier Simonart 
> ---
>  controller/local_data.c | 11 ++
>  controller/local_data.h |  2 ++
>  controller/ovn-controller.c |  5 +
>  tests/ovn.at| 42 +
>  4 files changed, 60 insertions(+)
>
> diff --git a/controller/local_data.c b/controller/local_data.c
> index 3a7d3afeb..819dd8801 100644
> --- a/controller/local_data.c
> +++ b/controller/local_data.c
> @@ -56,6 +56,17 @@ static bool datapath_is_transit_switch(const struct 
> sbrec_datapath_binding *);
>
>  static uint64_t local_datapath_usage;
>
> +void
> +local_datapaths_clean(struct hmap *local_datapaths)
> +{
> +struct local_datapath *ld;
> +HMAP_FOR_EACH_SAFE (ld, hmap_node, local_datapaths) {
> +if (ld->datapath->tunnel_key != ld->hmap_node.hash) {
> +hmap_remove(local_datapaths, >hmap_node);
> +}
> +}
> +}
> +
>  struct local_datapath *
>  get_local_datapath(const struct hmap *local_datapaths, uint32_t tunnel_key)
>  {
> diff --git a/controller/local_data.h b/controller/local_data.h
> index f6d8f725f..5a4e9f4ca 100644
> --- a/controller/local_data.h
> +++ b/controller/local_data.h
> @@ -168,5 +168,7 @@ void remove_local_datapath_multichassis_port(struct 
> local_datapath *ld,
>   char *logical_port);
>  bool lb_is_local(const struct sbrec_load_balancer *sbrec_lb,
>   const struct hmap *local_datapaths);
> +void local_datapaths_clean(struct hmap *local_datapaths);
> +
>
>  #endif /* controller/local_data.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index da7d145ed..3f3c20e14 100644
> --- 

Re: [ovs-dev] [PATCH v7 1/8] system-dpdk: Introduce helpers for testpmd.

[Frode Nordahl]

On Mon, Oct 23, 2023 at 10:19 AM David Marchand
 wrote:
>
> Rather than copy/paste everywhere, introduce helpers to control
> testpmd runs.
> Rely on --stats-period (which outputs port stats every n seconds) so that
> testpmd keeps running without expecting any user input.
>
> Signed-off-by: David Marchand 
> Acked-by: Aaron Conole 
> Acked-by: Eelco Chaudron 
> ---
> Changes since v1:
> - fixed OVS_DPDK_START_TESTPMD passed arguments evaluation:: $@ -> $1,

Thanks for working on this, is there a plan to backport this?

It appears that there is currently an issue with dpdk-testpmd that
makes it exit immediately when run in non-interactive mode [0], while
this is an upstream DPDK bug, it does affect the system-dpdk testsuite
for released versions of OVS. I suspect this DPDK commit broke it [1].

0: https://bugs.launchpad.net/bugs/2040097
1: https://github.com/DPDK/dpdk/commit/0fd1386c30c3ad9365d7fdd2829bf7cb2e1b9dff

If there is no plan to backport this patch also fixes the problem:

diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
index 0f58e8574..26e4b5ed9 100644
--- a/tests/system-dpdk.at
+++ b/tests/system-dpdk.at
@@ -130,7 +130,7 @@ on_exit "pkill -f -x -9 'tail -f /dev/null'"
 tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat NUMA_NODE)" --no-pci\
--vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostuser0" \
--vdev="net_tap0,iface=tap0" --file-prefix page0 \
-   --single-file-segments -- -a
>$OVS_RUNDIR/testpmd-dpdkvhostuser0.log 2>&1 &
+   --single-file-segments -- -a --interactive
>$OVS_RUNDIR/testpmd-dpdkvhostuser0.log 2>&1 &

 OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log])
 OVS_WAIT_UNTIL([ip link show dev tap0 | grep -qw LOWER_UP])
@@ -205,7 +205,7 @@ on_exit "pkill -f -x -9 'tail -f /dev/null'"
 tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat NUMA_NODE)" --no-pci\
 
--vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostclient0,queues=2,server=1"
\
 --vdev="net_tap0,iface=tap0" --file-prefix page0 \
---single-file-segments -- -a --nb-cores 2 --rxq 2 --txq 2 \
+--single-file-segments -- -a --nb-cores 2 --rxq 2 --txq 2 --interactive \
 >$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &

 OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log])
@@ -683,7 +683,7 @@ on_exit "pkill -f -x -9 'tail -f /dev/null'"
 tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat NUMA_NODE)" --no-pci\
--vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostclient0,server=1"
\
--vdev="net_tap0,iface=tap0" --file-prefix page0 \
-   --single-file-segments -- -a
>$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &
+   --single-file-segments -- -a --interactive
>$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &

 OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log])

@@ -740,7 +740,7 @@ on_exit "pkill -f -x -9 'tail -f /dev/null'"
 tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat NUMA_NODE)" --no-pci\
--vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostclient0,server=1"
\
--vdev="net_tap0,iface=tap0" --file-prefix page0 \
-   --single-file-segments -- -a
>$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &
+   --single-file-segments -- -a --interactive
>$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &

 OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log])

@@ -874,7 +874,7 @@ on_exit "pkill -f -x -9 'tail -f /dev/null'"
 tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat NUMA_NODE)" --no-pci\
--vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostclient0,server=1"
\
--vdev="net_tap0,iface=tap0" --file-prefix page0 \
-   --single-file-segments -- -a
>$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &
+   --single-file-segments -- -a --interactive
>$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &

 OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log])

@@ -931,7 +931,7 @@ on_exit "pkill -f -x -9 'tail -f /dev/null'"
 tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat NUMA_NODE)" --no-pci\
--vdev="net_virtio_user,path=$OVS_RUNDIR/dpdkvhostclient0,server=1"
\
--vdev="net_tap0,iface=tap0" --file-prefix page0 \
-   --single-file-segments -- -a
>$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &
+   --single-file-segments -- -a --interactive
>$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &

 OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log])

-- 
2.40.1

-- 
Frode Nordahl

> ---
>  tests/system-dpdk-macros.at |  37 +
>  tests/system-dpdk.at| 103 +---
>  2 files changed, 61 insertions(+), 79 deletions(-)
>
> diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
> index 3920f08a5e..e1e91f2972 100644
> --- a/tests/system-dpdk-macros.at
> +++ b/tests/system-dpdk-macros.at
> @@ -79,3 

[ovs-dev] [PATCH v5] userspace: Support vxlan and geneve tso.

[Dexia Li via dev]

 For userspace datapath, this patch provides vxlan and geneve tunnel tso.
 Only support userspace vxlan or geneve tunnel, meanwhile support
 tunnel outter and inner csum offload. If netdev do not support offload
 features, there is a software fallback.If netdev do not support vxlan
 and geneve tso,packets will drop. Front-end devices can close offload
 features by ethtool also.

Signed-off-by: Dexia Li 
---
 lib/dp-packet.c |  41 +++-
 lib/dp-packet.h | 216 
 lib/dpif-netdev.c   |   4 +-
 lib/flow.c  |   2 +-
 lib/netdev-dpdk.c   |  88 ++--
 lib/netdev-dummy.c  |   2 +-
 lib/netdev-native-tnl.c | 106 ++--
 lib/netdev-provider.h   |   4 +
 lib/netdev.c|  35 +--
 lib/packets.c   |  12 +--
 lib/packets.h   |   6 +-
 tests/dpif-netdev.at|   4 +-
 12 files changed, 461 insertions(+), 59 deletions(-)

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index ed004c3b9..b5013da9f 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -543,16 +543,47 @@ dp_packet_compare_offsets(struct dp_packet *b1, struct 
dp_packet *b2,
 return true;
 }
 
+void
+dp_packet_tnl_outer_ol_send_prepare(struct dp_packet *p,
+uint64_t flags)
+{
+if (dp_packet_hwol_is_outer_ipv4_cksum(p)) {
+if (!(flags & NETDEV_TX_OFFLOAD_OUTER_IP_CKSUM)) {
+dp_packet_ip_set_header_csum(p, false);
+dp_packet_ol_set_ip_csum_good(p);
+dp_packet_hwol_reset_outer_ipv4_csum(p);
+}
+}
+
+if (!dp_packet_hwol_is_outer_UDP_cksum(p)) {
+return;
+}
+
+if (!(flags & NETDEV_TX_OFFLOAD_OUTER_UDP_CKSUM)) {
+packet_udp_complete_csum(p, false);
+dp_packet_ol_set_l4_csum_good(p);
+dp_packet_hwol_reset_outer_udp_csum(p);
+}
+}
+
 /* Checks if the packet 'p' is compatible with netdev_ol_flags 'flags'
  * and if not, updates the packet with the software fall back. */
 void
 dp_packet_ol_send_prepare(struct dp_packet *p, uint64_t flags)
 {
+bool tnl_inner = false;
+
+if (dp_packet_hwol_is_tunnel_geneve(p) ||
+dp_packet_hwol_is_tunnel_vxlan(p)) {
+dp_packet_tnl_outer_ol_send_prepare(p, flags);
+tnl_inner = true;
+}
+
 if (dp_packet_hwol_tx_ip_csum(p)) {
 if (dp_packet_ip_checksum_good(p)) {
 dp_packet_hwol_reset_tx_ip_csum(p);
 } else if (!(flags & NETDEV_TX_OFFLOAD_IPV4_CKSUM)) {
-dp_packet_ip_set_header_csum(p);
+dp_packet_ip_set_header_csum(p, tnl_inner);
 dp_packet_ol_set_ip_csum_good(p);
 dp_packet_hwol_reset_tx_ip_csum(p);
 }
@@ -562,24 +593,24 @@ dp_packet_ol_send_prepare(struct dp_packet *p, uint64_t 
flags)
 return;
 }
 
-if (dp_packet_l4_checksum_good(p)) {
+if (dp_packet_l4_checksum_good(p) && (!tnl_inner)) {
 dp_packet_hwol_reset_tx_l4_csum(p);
 return;
 }
 
 if (dp_packet_hwol_l4_is_tcp(p)
 && !(flags & NETDEV_TX_OFFLOAD_TCP_CKSUM)) {
-packet_tcp_complete_csum(p);
+packet_tcp_complete_csum(p, tnl_inner);
 dp_packet_ol_set_l4_csum_good(p);
 dp_packet_hwol_reset_tx_l4_csum(p);
 } else if (dp_packet_hwol_l4_is_udp(p)
&& !(flags & NETDEV_TX_OFFLOAD_UDP_CKSUM)) {
-packet_udp_complete_csum(p);
+packet_udp_complete_csum(p, tnl_inner);
 dp_packet_ol_set_l4_csum_good(p);
 dp_packet_hwol_reset_tx_l4_csum(p);
 } else if (!(flags & NETDEV_TX_OFFLOAD_SCTP_CKSUM)
&& dp_packet_hwol_l4_is_sctp(p)) {
-packet_sctp_complete_csum(p);
+packet_sctp_complete_csum(p, tnl_inner);
 dp_packet_ol_set_l4_csum_good(p);
 dp_packet_hwol_reset_tx_l4_csum(p);
 }
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 70ddf8aa4..80c7ab961 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -86,22 +86,47 @@ enum dp_packet_offload_mask {
 DEF_OL_FLAG(DP_PACKET_OL_TX_SCTP_CKSUM, RTE_MBUF_F_TX_SCTP_CKSUM, 0x800),
 /* Offload IP checksum. */
 DEF_OL_FLAG(DP_PACKET_OL_TX_IP_CKSUM, RTE_MBUF_F_TX_IP_CKSUM, 0x1000),
+/* Offload packet is tunnel GENEVE. */
+DEF_OL_FLAG(DP_PACKET_OL_TX_TUNNEL_GENEVE,
+RTE_MBUF_F_TX_TUNNEL_GENEVE, 0x2000),
+/* Offload packet is tunnel VXLAN. */
+DEF_OL_FLAG(DP_PACKET_OL_TX_TUNNEL_VXLAN,
+RTE_MBUF_F_TX_TUNNEL_VXLAN, 0x4000),
+/* Offload tunnel packet, out is ipv4 */
+DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_IPV4,
+RTE_MBUF_F_TX_OUTER_IPV4, 0x8000),
+/* Offload TUNNEL out ipv4 checksum */
+DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_IP_CKSUM,
+RTE_MBUF_F_TX_OUTER_IP_CKSUM, 0x1),
+/* Offload TUNNEL out udp checksum */
+DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_UDP_CKSUM,
+RTE_MBUF_F_TX_OUTER_UDP_CKSUM, 0x2),
+/* Offload tunnel packet, out is ipv6 */
+

Re: [ovs-dev] [PATCH v4] userspace: Support vxlan and geneve tso.

[0-day Robot]

References:  <20231026070359.676-1-dexia...@jaguarmicro.com>
 

Bleep bloop.  Greetings Dexia Li, 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:
ERROR: Inappropriate bracing around statement
#421 FILE: lib/dpif-netdev.c:7979:
if (type != DPIF_UC_MISS)

Lines checked: 898, Warnings: 0, Errors: 1


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 v4] userspace: Support vxlan and geneve tso.

[Dexia Li via dev]

 For userspace datapath, this patch provides vxlan and geneve tunnel tso.
 Only support userspace vxlan or geneve tunnel, meanwhile support
 tunnel outter and inner csum offload. If netdev do not support offload
 features, there is a software fallback.If netdev do not support vxlan
 and geneve tso,packets will drop. Front-end devices can close offload
 features by ethtool also.

Signed-off-by: Dexia Li 
---
 lib/dp-packet.c |  41 +++-
 lib/dp-packet.h | 216 
 lib/dpif-netdev.c   |   3 +-
 lib/flow.c  |   2 +-
 lib/netdev-dpdk.c   |  88 ++--
 lib/netdev-dummy.c  |   2 +-
 lib/netdev-native-tnl.c | 106 ++--
 lib/netdev-provider.h   |   4 +
 lib/netdev.c|  35 +--
 lib/packets.c   |  12 +--
 lib/packets.h   |   6 +-
 tests/dpif-netdev.at|   4 +-
 12 files changed, 460 insertions(+), 59 deletions(-)

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index ed004c3b9..b5013da9f 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -543,16 +543,47 @@ dp_packet_compare_offsets(struct dp_packet *b1, struct 
dp_packet *b2,
 return true;
 }
 
+void
+dp_packet_tnl_outer_ol_send_prepare(struct dp_packet *p,
+uint64_t flags)
+{
+if (dp_packet_hwol_is_outer_ipv4_cksum(p)) {
+if (!(flags & NETDEV_TX_OFFLOAD_OUTER_IP_CKSUM)) {
+dp_packet_ip_set_header_csum(p, false);
+dp_packet_ol_set_ip_csum_good(p);
+dp_packet_hwol_reset_outer_ipv4_csum(p);
+}
+}
+
+if (!dp_packet_hwol_is_outer_UDP_cksum(p)) {
+return;
+}
+
+if (!(flags & NETDEV_TX_OFFLOAD_OUTER_UDP_CKSUM)) {
+packet_udp_complete_csum(p, false);
+dp_packet_ol_set_l4_csum_good(p);
+dp_packet_hwol_reset_outer_udp_csum(p);
+}
+}
+
 /* Checks if the packet 'p' is compatible with netdev_ol_flags 'flags'
  * and if not, updates the packet with the software fall back. */
 void
 dp_packet_ol_send_prepare(struct dp_packet *p, uint64_t flags)
 {
+bool tnl_inner = false;
+
+if (dp_packet_hwol_is_tunnel_geneve(p) ||
+dp_packet_hwol_is_tunnel_vxlan(p)) {
+dp_packet_tnl_outer_ol_send_prepare(p, flags);
+tnl_inner = true;
+}
+
 if (dp_packet_hwol_tx_ip_csum(p)) {
 if (dp_packet_ip_checksum_good(p)) {
 dp_packet_hwol_reset_tx_ip_csum(p);
 } else if (!(flags & NETDEV_TX_OFFLOAD_IPV4_CKSUM)) {
-dp_packet_ip_set_header_csum(p);
+dp_packet_ip_set_header_csum(p, tnl_inner);
 dp_packet_ol_set_ip_csum_good(p);
 dp_packet_hwol_reset_tx_ip_csum(p);
 }
@@ -562,24 +593,24 @@ dp_packet_ol_send_prepare(struct dp_packet *p, uint64_t 
flags)
 return;
 }
 
-if (dp_packet_l4_checksum_good(p)) {
+if (dp_packet_l4_checksum_good(p) && (!tnl_inner)) {
 dp_packet_hwol_reset_tx_l4_csum(p);
 return;
 }
 
 if (dp_packet_hwol_l4_is_tcp(p)
 && !(flags & NETDEV_TX_OFFLOAD_TCP_CKSUM)) {
-packet_tcp_complete_csum(p);
+packet_tcp_complete_csum(p, tnl_inner);
 dp_packet_ol_set_l4_csum_good(p);
 dp_packet_hwol_reset_tx_l4_csum(p);
 } else if (dp_packet_hwol_l4_is_udp(p)
&& !(flags & NETDEV_TX_OFFLOAD_UDP_CKSUM)) {
-packet_udp_complete_csum(p);
+packet_udp_complete_csum(p, tnl_inner);
 dp_packet_ol_set_l4_csum_good(p);
 dp_packet_hwol_reset_tx_l4_csum(p);
 } else if (!(flags & NETDEV_TX_OFFLOAD_SCTP_CKSUM)
&& dp_packet_hwol_l4_is_sctp(p)) {
-packet_sctp_complete_csum(p);
+packet_sctp_complete_csum(p, tnl_inner);
 dp_packet_ol_set_l4_csum_good(p);
 dp_packet_hwol_reset_tx_l4_csum(p);
 }
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 70ddf8aa4..80c7ab961 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -86,22 +86,47 @@ enum dp_packet_offload_mask {
 DEF_OL_FLAG(DP_PACKET_OL_TX_SCTP_CKSUM, RTE_MBUF_F_TX_SCTP_CKSUM, 0x800),
 /* Offload IP checksum. */
 DEF_OL_FLAG(DP_PACKET_OL_TX_IP_CKSUM, RTE_MBUF_F_TX_IP_CKSUM, 0x1000),
+/* Offload packet is tunnel GENEVE. */
+DEF_OL_FLAG(DP_PACKET_OL_TX_TUNNEL_GENEVE,
+RTE_MBUF_F_TX_TUNNEL_GENEVE, 0x2000),
+/* Offload packet is tunnel VXLAN. */
+DEF_OL_FLAG(DP_PACKET_OL_TX_TUNNEL_VXLAN,
+RTE_MBUF_F_TX_TUNNEL_VXLAN, 0x4000),
+/* Offload tunnel packet, out is ipv4 */
+DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_IPV4,
+RTE_MBUF_F_TX_OUTER_IPV4, 0x8000),
+/* Offload TUNNEL out ipv4 checksum */
+DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_IP_CKSUM,
+RTE_MBUF_F_TX_OUTER_IP_CKSUM, 0x1),
+/* Offload TUNNEL out udp checksum */
+DEF_OL_FLAG(DP_PACKET_OL_TX_OUTER_UDP_CKSUM,
+RTE_MBUF_F_TX_OUTER_UDP_CKSUM, 0x2),
+/* Offload tunnel packet, out is ipv6 */
+

Re: [ovs-dev] [PATCH] tests: Use ping timeout instead of deadline.

[Frode Nordahl]

On Wed, Oct 25, 2023 at 11:33 PM Ilya Maximets  wrote:
>
> On 10/25/23 11:45, Simon Horman wrote:
> > On Sat, Oct 21, 2023 at 05:04:48PM +0200, Frode Nordahl wrote:
> >> Many system tests currently use ping with the combination of a
> >> low packet count (-c 3), short interval between sends (-i 0.3)
> >> and a _deadline_ of 2 seconds (-d 2).
> >>
> >> This combination of options may lead to a situation where more
> >> than count packets are sent however ping will stop when count
> >> packets are received. This results in a failed test due to how
> >> the result is checked, for example:
> >>
> >> ping6 -q -c 3 -i 0.3 -w 2 fc00::3 | FORMAT_PING
> >> @@ -1,2 +1,2 @@
> >> -3 packets transmitted, 3 received, 0% packet loss, time 0ms
> >> +4 packets transmitted, 3 received, 25% packet loss, time 0ms
> >>
> >> To reiterate, in the above example there is no packet loss, but
> >> ping stops after _receiving_ 3 packets, not bothering with
> >> waiting for the response to the fourth packet it just sent out.
> >>
> >> If we look at the iputils ping manual for the -w deadline option
> >> we can read that this is expected behavior:
> >>
> >>> Specify a timeout, in seconds, before ping exits regardless of
> >>> how many packets have been sent or received. In this case ping
> >>> does not stop after count packet are sent, it waits either for
> >>> deadline expire or until count probes are answered or for some
> >>> error notification from network.
> >>
> >> To avoid these kinds of failures in checks where a response is
> >> expected, we replace ping -w with ping -W.
> >>
> >> We keep ping -w for checks where it is expected to NOT get a
> >> response.
> >>
> >> Signed-off-by: Frode Nordahl 
> >
> > Thanks Frode,
> >
> > TIL about -w and -W.
>
> I learned about -W as well. :)
>
> Thanks, Frode, for figuring out the cause of these failures!  I've seen
> them before, but didn't dig too deep to find a cause.  OVN also has them
> from time to time.

yw, we run all the system tests in an automated fashion as part of the
openvswitch package regression testing, and we see them quite
frequently, most likely due to the load of the CI infrastructure.

> Though I'm not sure if -W is the right choice.  Reading the description:
>
>-W timeout
>Time to wait for a response, in seconds. The
>option affects only timeout in absence of any
>responses, otherwise ping waits for two RTTs.
>Real number allowed with dot as a decimal
>separator (regardless locale setup). 0 means
>infinite timeout.
>
> And I don't really like the 'in absence of ANY responses' part of it.
>
> So, IIUC, if we send 3 packets, first gets replied and the other two
> are dropped somewhere, ping will ignore the timeout and will wait
> indefinitely.  Unfortunately, OVS gives the first packet a special
> treatment, so potential for this scenario to happen is rather high.
> This may break CI systems, getting them stuck testing one patch. And
> it doesn't seem like we can mix -w and -W, at least the behavior is
> not really defined in this case.

It also says "otherwise ping waits for two RTTs.", so it will not wait
indefinitely. The documentation is a bit convoluted though so I went
to look so that we can be sure about what it will do.

On arrival of the first packet, ping will gather various information
[0] which will be used to compute the RTT [1], which is used when
initializing the waittime [2][3].

So it appears to me -W would cover the scenario laid out above, i.e.
if we get one reply quickly and the rest are lost, the computed RTT
would have a ping exit within a reasonable timeframe. Even if the
first response comes near the timeout value, the RTT would not be more
than 6 seconds for a -W of 3.

0: 
https://github.com/iputils/iputils/blob/0cc6da796b9a64113152c071088701cb95a72ae8/ping/ping.c#L1654-L1660
1: 
https://github.com/iputils/iputils/blob/0cc6da796b9a64113152c071088701cb95a72ae8/ping/ping_common.c#L761
2: 
https://github.com/iputils/iputils/blob/0cc6da796b9a64113152c071088701cb95a72ae8/ping/ping_common.c#L599
3: 
https://github.com/iputils/iputils/blob/0cc6da796b9a64113152c071088701cb95a72ae8/ping/ping_common.c#L263-L268

> Would be really nice to use fping instead that has simple and very
> straightforward arguments without side effects, but once again RHEL
> doesn't package it...
>
> Maybe we could use '$ timeout 2 ping6 -q -c 3 -i 0.3 fc00::3' instead?

That would also work, either option works for me, what would be your preference?

> Another option might be to slightly reduce the deadline, so the 4th
> packet will not be sent.  But that sounds fragile.

Agreed.

-- 
Frode Nordahl

> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev