Re: [ovs-dev] [PATCH] timeval: expose unreasonably long poll counter

[Eelco Chaudron]

On 30 Aug 2023, at 19:50, Aaron Conole wrote:

> Martin Kennelly observes that even though this data is available to
> humans via the journal/log files, these aren't exactly easy for a
> developer to make any kind of behavioral inferences.  This kind of
> log and counter would be useful when checking on system health to
> let us know that an Open vSwitch component is noticing some kind of
> system level hiccup.
>
> Add a new coverage counter to track information on these events, and
> let a developer or system engineer know how long these events have
> occurred with some historical context.
>
> Reported-at: 
> https://lists.linuxfoundation.org/pipermail/ovs-discuss/2023-June/052523.html
> Reported-by: Martin Kennelly 
> Signed-off-by: Aaron Conole 

Thanks for adding this Aaron! I think it makes sense to be able to see this has 
happened even if the log rotates, etc.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v3] conntrack: Remove nat_conn introducing key directionality.

[Frode Nordahl]

On Wed, Aug 30, 2023 at 9:30 PM Paolo Valerio  wrote:
>
> From: hepeng 
>
> The patch avoids the extra allocation for nat_conn.
> Currently, when doing NAT, the userspace conntrack will use an extra
> conn for the two directions in a flow. However, each conn has actually
> the two keys for both orig and rev directions. This patch introduces a
> key_node[CT_DIRS] member as per Aaron's suggestion in the conn which
> consists of a key, direction, and a cmap_node for hash lookup so
> addressing the feedback received by the original patch [0].
>
> [0] 
> https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/
>
> Signed-off-by: Peng He 
> Co-authored-by: Paolo Valerio 
> Signed-off-by: Paolo Valerio 

Thanks alot for working on this, should we perhaps reference the
original bug report, i.e:
Reported-by: Michael Plato 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2022-September/052065.html

We have a reproducer for the issue and we no longer see it occurring
with this patch.
Tested-by: Frode Nordahl 

Is there a plan for a backport, we have users on OVS 2.17 that would
be interested in having this fixed, and AFAICT this patch does not
cleanly backport.

-- 
Frode Nordahl

> ---
> v3:
>   - resolved a potentially UB with offsetof() and integer constant
> expression (Ilya)
>   - int to bool assignment (Ilya)
>   - check the direction early in conntrack_dump_next() to avoid
> unneeded operations (Ilya)
>   - unrelated change added that turns the branch:
> if (!conn_lookup()) { return true; } else { return false; }
> into return !conn_lookup() (Ilya)
>   - cosmetic/coding style changes (Ilya)
>
> v2:
>   - use enum value instead of bool (Aaron).
>   - s/conn_for_expectation/conn_for_exp/ in process_ftp_ctl_v6()
> to avoid long line.
>   - removed CT_CONN_TYPE_* reference in two comments.
> ---
>  lib/conntrack-private.h |   19 +-
>  lib/conntrack-tp.c  |6 +
>  lib/conntrack.c |  366 
> +++
>  3 files changed, 164 insertions(+), 227 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index bb326868e..3fd5fccd3 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -49,6 +49,12 @@ struct ct_endpoint {
>   * hashing in ct_endpoint_hash_add(). */
>  BUILD_ASSERT_DECL(sizeof(struct ct_endpoint) == sizeof(union ct_addr) + 4);
>
> +enum key_dir {
> +CT_DIR_FWD = 0,
> +CT_DIR_REV,
> +CT_DIRS,
> +};
> +
>  /* Changes to this structure need to be reflected in conn_key_hash()
>   * and conn_key_cmp(). */
>  struct conn_key {
> @@ -112,20 +118,18 @@ enum ct_timeout {
>
>  #define N_EXP_LISTS 100
>
> -enum OVS_PACKED_ENUM ct_conn_type {
> -CT_CONN_TYPE_DEFAULT,
> -CT_CONN_TYPE_UN_NAT,
> +struct conn_key_node {
> +enum key_dir dir;
> +struct conn_key key;
> +struct cmap_node cm_node;
>  };
>
>  struct conn {
>  /* Immutable data. */
> -struct conn_key key;
> -struct conn_key rev_key;
> +struct conn_key_node key_node[CT_DIRS];
>  struct conn_key parent_key; /* Only used for orig_tuple support. */
> -struct cmap_node cm_node;
>  uint16_t nat_action;
>  char *alg;
> -struct conn *nat_conn; /* The NAT 'conn' context, if there is one. */
>  atomic_flag reclaimed; /* False during the lifetime of the connection,
>  * True as soon as a thread has started freeing
>  * its memory. */
> @@ -150,7 +154,6 @@ struct conn {
>
>  /* Immutable data. */
>  bool alg_related; /* True if alg data connection. */
> -enum ct_conn_type conn_type;
>
>  uint32_t tp_id; /* Timeout policy ID. */
>  };
> diff --git a/lib/conntrack-tp.c b/lib/conntrack-tp.c
> index 89cb2704a..2149fdc73 100644
> --- a/lib/conntrack-tp.c
> +++ b/lib/conntrack-tp.c
> @@ -253,7 +253,8 @@ conn_update_expiration(struct conntrack *ct, struct conn 
> *conn,
>  }
>  VLOG_DBG_RL(&rl, "Update timeout %s zone=%u with policy id=%d "
>  "val=%u sec.",
> -ct_timeout_str[tm], conn->key.zone, conn->tp_id, val);
> +ct_timeout_str[tm], conn->key_node[CT_DIR_FWD].key.zone,
> +conn->tp_id, val);
>
>  atomic_store_relaxed(&conn->expiration, now + val * 1000);
>  }
> @@ -273,7 +274,8 @@ conn_init_expiration(struct conntrack *ct, struct conn 
> *conn,
>  }
>
>  VLOG_DBG_RL(&rl, "Init timeout %s zone=%u with policy id=%d val=%u sec.",
> -ct_timeout_str[tm], conn->key.zone, conn->tp_id, val);
> +ct_timeout_str[tm], conn->key_node[CT_DIR_FWD].key.zone,
> +conn->tp_id, val);
>
>  conn->expiration = now + val * 1000;
>  }
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 5f1176d33..47a443fba 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -103,7 +103,7 @@ static enum ct_update_res conn_update(struct conntrack 
> *ct

[ovs-dev] [PATCH ovn] ovs: Bump submodule to branch-3.2

[Ales Musil]

Bump submodule to branch-3.2 mainly for:
759a29dc2d97 ("backtrace: Extend the backtrace functionality.")
1d78a3f3164a ("netdev-dpdk: Disable net/tap Tx L4 checksum offloads.")

Unfortunately we cannot use the tag because we need
the DPDK offload fix, which was merged later on.

At the same time fix three issues caused by the bump.

Remove our custom 'struct sctp_chunk_header' that was added
to packets.h as part of:
501f665a5a4b ("conntrack: Extract l4 information for SCTP.")

Adjust "daemonize_start" to accept two parameters
and set the second to false, as we don't need access
to DPDK devices directly at this moment. This was introduced
by commit:
07cf5810de8d ("dpdk: Allow retaining CAP_SYS_RAWIO privileges.")

Adjust the path for OvS python helpers, introduced by:
bb0dd1135ba9 ("python: Rename build related code to ovs_build_helpers.")

Reported-at: https://bugzilla.redhat.com/2164058
Signed-off-by: Ales Musil 
---
 Makefile.am   |  2 +-
 build-aux/sodepends.py|  2 +-
 build-aux/soexpand.py |  2 +-
 build-aux/xml2nroff   | 14 +++---
 controller-vtep/ovn-controller-vtep.c |  2 +-
 controller/ovn-controller.c   |  2 +-
 controller/pinctrl.c  | 10 +-
 ic/ovn-ic.c   |  2 +-
 lib/ovn-util.h|  7 ---
 northd/ovn-northd.c   |  2 +-
 ovs   |  2 +-
 utilities/ovn-dbctl.c |  2 +-
 utilities/ovn-trace.c |  2 +-
 13 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 27182c7bc..b58d4a501 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -430,7 +430,7 @@ endif
 CLEANFILES += flake8-check
 
 include $(srcdir)/manpages.mk
-$(srcdir)/manpages.mk: $(MAN_ROOTS) build-aux/sodepends.py 
$(OVS_SRCDIR)/python/build/soutil.py
+$(srcdir)/manpages.mk: $(MAN_ROOTS) build-aux/sodepends.py 
$(OVS_SRCDIR)/python/ovs_build_helpers/soutil.py

@PYTHONPATH=$(OVS_SRCDIR)/python$(psep)$$PYTHONPATH$(psep)$(srcdir)/python 
$(PYTHON3) $(srcdir)/build-aux/sodepends.py -I. -Isrcdir,$(srcdir) 
-IOVS_MANDIR,$(OVS_MANDIR) $(MAN_ROOTS) >$(@F).tmp
@if cmp -s $(@F).tmp $@; then \
  touch $@; \
diff --git a/build-aux/sodepends.py b/build-aux/sodepends.py
index 7b1f9c840..c04a5c681 100755
--- a/build-aux/sodepends.py
+++ b/build-aux/sodepends.py
@@ -14,7 +14,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from build import soutil
+from ovs_build_helpers import soutil
 import sys
 import getopt
 import os
diff --git a/build-aux/soexpand.py b/build-aux/soexpand.py
index 00adcf47a..f9ab80963 100755
--- a/build-aux/soexpand.py
+++ b/build-aux/soexpand.py
@@ -14,7 +14,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from build import soutil
+from ovs_build_helpers import soutil
 import sys
 
 
diff --git a/build-aux/xml2nroff b/build-aux/xml2nroff
index 9e781a396..dd9577204 100755
--- a/build-aux/xml2nroff
+++ b/build-aux/xml2nroff
@@ -18,7 +18,7 @@ import getopt
 import sys
 import xml.dom.minidom
 
-import build.nroff
+from ovs_build_helpers import nroff
 
 argv0 = sys.argv[0]
 
@@ -94,12 +94,12 @@ def manpage_to_nroff(xml_file, subst, include_path, 
version=None):
 .  PP
 .  I "\\$1"
 ..
-''' % (build.nroff.text_to_nroff(program),
-   build.nroff.text_to_nroff(section),
-   build.nroff.text_to_nroff(title),
-   build.nroff.text_to_nroff(version))
+''' % (nroff.text_to_nroff(program),
+   nroff.text_to_nroff(section),
+   nroff.text_to_nroff(title),
+   nroff.text_to_nroff(version))
 
-s += build.nroff.block_xml_to_nroff(doc.childNodes) + "\n"
+s += nroff.block_xml_to_nroff(doc.childNodes) + "\n"
 
 return s
 
@@ -145,7 +145,7 @@ if __name__ == "__main__":
 
 try:
 s = manpage_to_nroff(args[0], subst, include_path, version)
-except build.nroff.error.Error as e:
+except nroff.error.Error as e:
 sys.stderr.write("%s: %s\n" % (argv0, e.msg))
 sys.exit(1)
 for line in s.splitlines():
diff --git a/controller-vtep/ovn-controller-vtep.c 
b/controller-vtep/ovn-controller-vtep.c
index 5f017d87d..23b368179 100644
--- a/controller-vtep/ovn-controller-vtep.c
+++ b/controller-vtep/ovn-controller-vtep.c
@@ -116,7 +116,7 @@ main(int argc, char *argv[])
 parse_options(argc, argv);
 fatal_ignore_sigpipe();
 
-daemonize_start(false);
+daemonize_start(false, false);
 
 char *abs_unixctl_path = get_abs_unix_ctl_path(NULL);
 retval = unixctl_server_create(abs_unixctl_path, &unixctl);
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 1e49f423f..64d21b39e 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -4988,7 +4988,7 @@ main(int argc, char *argv[])
 char *ovs_remote = parse_options(argc, arg

[ovs-dev] [dpdk-latest] sparse: Fix build with DPDK v23.11-rc1.

[David Marchand]

After DPDK started relying on compiler intrinsics in rte_cycles.h,
sparse raises the following warning:

libtool: compile:  env REAL_CC=gcc "CHECK=sparse -Wsparse-error
-I ../../include/sparse -I ../../include -m64
-I /usr/local/include -I /usr/include/x86_64-linux-gnu
" cgcc -target=x86_64 -target=host_os_specs -D__MMX__=1
-D__SSE2_MATH__=1 -D__SSE_MATH__=1 -D__SSE2__=1 -D__SSE__=1
-DHAVE_CONFIG_H -I. -I../.. -I ../../include -I ./include
-I ../../lib -I ./lib -Wstrict-prototypes -Wall -Wextra
-Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security
-Wswitch-enum -Wunused-parameter -Wbad-function-cast
-Wcast-align -Wstrict-prototypes -Wold-style-definition
-Wmissing-prototypes -Wmissing-field-initializers
-fno-strict-aliasing -Wswitch-bool -Wlogical-not-parentheses
-Wsizeof-array-argument -Wbool-compare -Wshift-negative-value
-Wduplicated-cond -Wshadow -Wmultistatement-macros
-Wcast-align=strict -mssse3 -include rte_config.h -mrtm
-I/home/runner/work/ovs/ovs/dpdk-dir/include -Werror
-D_FILE_OFFSET_BITS=64 -g -O2 -MT lib/netdev-dpdk.lo -MD -MP
-MF lib/.deps/netdev-dpdk.Tpo -c ../../lib/netdev-dpdk.c
-o lib/netdev-dpdk.o
../../lib/netdev-dpdk.c: note: in included file (through
/usr/lib/gcc/x86_64-linux-gnu/9//include/x86intrin.h,
/home/runner/work/ovs/ovs/dpdk-dir/include/rte_cycles.h,
/home/runner/work/ovs/ovs/dpdk-dir/include/rte_spinlock.h, ...):
/usr/lib/gcc/x86_64-linux-gnu/9//include/ia32intrin.h:114:10: error:
undefined identifier '__builtin_ia32_rdtsc'

Provide an empty implementation of __builtin_ia32_rdtsc() builtin.

Signed-off-by: David Marchand 
---
Note: I am sending this early, but please wait before merging this in
dpdk-latest as v23.11-rc1 is far from being ready.
I am expecting more changes in EAL headers and I'll update this patch
if hitting more issues.

---
 include/sparse/automake.mk  |  1 +
 include/sparse/ia32intrin.h | 23 +++
 2 files changed, 24 insertions(+)
 create mode 100644 include/sparse/ia32intrin.h

diff --git a/include/sparse/automake.mk b/include/sparse/automake.mk
index e966371192..c1229870bb 100644
--- a/include/sparse/automake.mk
+++ b/include/sparse/automake.mk
@@ -4,6 +4,7 @@ noinst_HEADERS += \
 include/sparse/arpa/inet.h \
 include/sparse/bits/floatn.h \
 include/sparse/assert.h \
+include/sparse/ia32intrin.h \
 include/sparse/math.h \
 include/sparse/numa.h \
 include/sparse/netinet/in.h \
diff --git a/include/sparse/ia32intrin.h b/include/sparse/ia32intrin.h
new file mode 100644
index 00..5045bf38d9
--- /dev/null
+++ b/include/sparse/ia32intrin.h
@@ -0,0 +1,23 @@
+/* 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.
+ */
+
+#ifndef __CHECKER__
+#error "Use this header only with sparse.  It is not a correct implementation."
+#endif
+
+#define __builtin_ia32_rdtsc() (unsigned long long) 0
+
+/* Get actual  definitions for us to annotate and build on. */
+#include_next 
-- 
2.41.0

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

Re: [ovs-dev] [PATCH v4 ovn] controller: make garp_max_timeout configurable

[Ales Musil]

On Wed, Aug 30, 2023 at 7:24 PM Lorenzo Bianconi
 wrote:
>
> When using VLAN backed networks and OVN routers leveraging the
> 'ovn-chassis-mac-mappings' option for east-west traffic, the eth.src field is
> replaced by the chassis mac address in order to not expose the router mac
> address from different nodes and confuse the TOR switch. However doing so
> the TOR switch is not able to learn the port/mac bindings for routed E/W
> traffic and it is force to always flood it. Fix this issue adding the
> capability to configure a given timeout for garp sent by ovn-controller
> and not disable it after the exponential backoff in order to keep
> refreshing the entries in TOR swtich fdb table.
> More into about the issue can be found here [0].
>
> [0] 
> https://mail.openvswitch.org/pipermail/ovs-discuss/2020-September/050678.html
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2087779
>
> Signed-off-by: Lorenzo Bianconi 
> ---
> Changes since v3:
> - simplify code logic
>
> Changes since v2:
> - cap exponential backoff timeout to garp_max_timeout
>
> Changes since v1:
> - add uni-test
> - add documentation
> ---
>  controller/ovn-controller.8.xml | 11 +
>  controller/ovn-controller.c |  4 +-
>  controller/pinctrl.c| 73 +++--
>  controller/pinctrl.h|  4 +-
>  tests/ovn.at| 16 
>  5 files changed, 93 insertions(+), 15 deletions(-)
>
> diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
> index 0883d8da9..7b4100592 100644
> --- a/controller/ovn-controller.8.xml
> +++ b/controller/ovn-controller.8.xml
> @@ -365,6 +365,17 @@
>  heplful to pin source outer IP for the tunnel when multiple 
> interfaces
>  are used on the host for overlay traffic.
>
> +  external_ids:garp-max-timeout-sec
> +  
> +When used, this configuration value specifies the maximum timeout
> +(in seconds) between two consecutive GARP packets sent by
> +ovn-controller.
> +ovn-controller by default sends just 4 GARP packets
> +with an exponential backoff timeout.
> +Setting external_ids:garp-max-timeout-sec allows to
> +cap for the exponential backoff used by ovn-controller
> +to send GARPs packets.
> +  
>  
>
>  
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 1e49f423f..90bb80d56 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -5787,7 +5787,9 @@ main(int argc, char *argv[])
>  &runtime_data->local_datapaths,
>  &runtime_data->active_tunnels,
>  
> &runtime_data->local_active_ports_ipv6_pd,
> -&runtime_data->local_active_ports_ras);
> +&runtime_data->local_active_ports_ras,
> +ovsrec_open_vswitch_table_get(
> +ovs_idl_loop.idl));
>  stopwatch_stop(PINCTRL_RUN_STOPWATCH_NAME,
> time_msec());
>  mirror_run(ovs_idl_txn,
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index bed90fe0b..c280d2256 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -166,6 +166,10 @@ static struct ovs_mutex pinctrl_mutex = 
> OVS_MUTEX_INITIALIZER;
>  static struct seq *pinctrl_handler_seq;
>  static struct seq *pinctrl_main_seq;
>
> +#define GARP_RARP_DEF_MAX_TIMEOUT16000
> +static long long int garp_rarp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT;
> +static bool garp_rarp_continuous;
> +
>  static void *pinctrl_handler(void *arg);
>
>  struct pinctrl {
> @@ -227,7 +231,8 @@ static void send_garp_rarp_prepare(
>  const struct ovsrec_bridge *,
>  const struct sbrec_chassis *,
>  const struct hmap *local_datapaths,
> -const struct sset *active_tunnels)
> +const struct sset *active_tunnels,
> +const struct ovsrec_open_vswitch_table *ovs_table)
>  OVS_REQUIRES(pinctrl_mutex);
>  static void send_garp_rarp_run(struct rconn *swconn,
> long long int *send_garp_rarp_time)
> @@ -3492,7 +3497,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>  const struct hmap *local_datapaths,
>  const struct sset *active_tunnels,
>  const struct shash *local_active_ports_ipv6_pd,
> -const struct shash *local_active_ports_ras)
> +const struct shash *local_active_ports_ras,
> +const struct ovsrec_open_vswitch_table *ovs_table)
>  {
>  ovs_mutex_lock(&pinctrl_mutex);
>  run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
> @@ -3503,7 +3509,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>  send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath,
>   

Re: [ovs-dev] [PATCH ovn v2] northd: Add incremental processing for NB port groups.

[Ales Musil]

On Wed, Aug 30, 2023 at 3:50 PM Dumitru Ceara  wrote:

> It's similar to the processing we do for address sets.  There's a bit
> more mechanics involved due to the fact that we need to split NB port
> groups per datapath.
>
> We currently only partially implement incremental processing of
> port_group changes in the lflow node.  That is, we deal with the case
> when the sets of "switches per port group" doesn't change.  In that
> specific case ACL lflows don't need to be reprocessed.
>
> In a synthetic benchmark that created (in this order):
> - 500 switches
> - 2000 port groups
> - 4 ACLs per port group
> - 1 ports distributed equally between the switches and port groups
>
> we measured the following ovn-northd CPU usage:
>
>   +-+++
>   | Incremental processing? | --wait=sb? | northd avg cpu (%) |
>   +-+++
>   |   N | Y  |84.2|
>   +-+++
>   |   Y | Y  |41.5|
>   +-+++
>   |   N | N  |93.2|
>   +-+++
>   |   Y | N  |53.6|
>   +-+++
>
> where '--wait=sb' set to 'Y'  means the benchmark was waiting for the
> port and port group operations to be propagated to the Southbound DB
> before continuing to the next operation.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2228162
> Signed-off-by: Dumitru Ceara 
> ---
> V2:
> - First 4 patches of the series merged, only respun the 5th one.
> - Addressed Han's comments:
>   - reversed semantics of ls_port_groups_sets_unchanged
>   - changed "struct port_group_to_ls_table" to "struct port_group_ls_table"
>   - changed "struct port_group_to_ls" to "struct port_group_ls_record"
>   - fixed bug due to missing recompute whenever a per-LS port group (but
> not
> the last one) was deleted
>   - handled SB port group creation in the I-P handler (normally that should
> not happen but if the SB is manually changed we now reconcile it
> without trigerring a recompute)
>   - fixed up some comment typos
>   - changed error checks in ls_port_group_record_clear() into assert()
> calls.
>   - updated unit test to include missing scenarios
>   - added CHECK_NO_CHANGE_AFTER_RECOMPUTE calls to unit test
>   - added port group check to _DUMP_DB_TABLES
> - Addressed Ales' comments:
>   - use xmalloc instead of xzalloc followed by full initialization of the
> structure
> - Other changes:
>   - unified I-P handler processing for port_group_ls_record
> creation/deletion
> ---
>  northd/en-lflow.c|  17 ++
>  northd/en-lflow.h|   1 +
>  northd/en-port-group.c   | 452 ++-
>  northd/en-port-group.h   |  36 +++-
>  northd/inc-proc-northd.c |  13 +-
>  northd/ovn-northd.c  |   4 +
>  tests/ovn-northd.at  | 371 
>  7 files changed, 832 insertions(+), 62 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index c6c0d4ebbf..153bf0f57a 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -122,6 +122,23 @@ lflow_northd_handler(struct engine_node *node,
>  return true;
>  }
>
> +bool
> +lflow_port_group_handler(struct engine_node *node, void *data OVS_UNUSED)
> +{
> +struct port_group_data *pg_data =
> +engine_get_input_data("port_group", node);
> +
> +/* If the set of switches per port group didn't change then there's no
> + * need to reprocess lflows.  Otherwise, there might be a need to
> + * add/delete port-group ACLs to/from switches. */
> +if (pg_data->ls_port_groups_sets_changed) {
> +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 5e3fbc25e3..5417b2faff 100644
> --- a/northd/en-lflow.h
> +++ b/northd/en-lflow.h
> @@ -13,5 +13,6 @@ void en_lflow_run(struct engine_node *node, void *data);
>  void *en_lflow_init(struct engine_node *node, struct engine_arg *arg);
>  void en_lflow_cleanup(void *data);
>  bool lflow_northd_handler(struct engine_node *, void *data);
> +bool lflow_port_group_handler(struct engine_node *, void *data);
>
>  #endif /* EN_LFLOW_H */
> diff --git a/northd/en-port-group.c b/northd/en-port-group.c
> index 2c36410246..0de9dc5f67 100644
> --- a/northd/en-port-group.c
> +++ b/northd/en-port-group.c
> @@ -33,15 +33,43 @@ static struct ls_port_group *ls_port_group_create(
>  static void ls_port_group_destroy(struct ls_port_group_table *,

Re: [ovs-dev] [PATCH v3] conntrack: Remove nat_conn introducing key directionality.

[Ilya Maximets]

On 8/31/23 09:15, Frode Nordahl wrote:
> On Wed, Aug 30, 2023 at 9:30 PM Paolo Valerio  wrote:
>>
>> From: hepeng 
>>
>> The patch avoids the extra allocation for nat_conn.
>> Currently, when doing NAT, the userspace conntrack will use an extra
>> conn for the two directions in a flow. However, each conn has actually
>> the two keys for both orig and rev directions. This patch introduces a
>> key_node[CT_DIRS] member as per Aaron's suggestion in the conn which
>> consists of a key, direction, and a cmap_node for hash lookup so
>> addressing the feedback received by the original patch [0].
>>
>> [0] 
>> https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/
>>
>> Signed-off-by: Peng He 
>> Co-authored-by: Paolo Valerio 
>> Signed-off-by: Paolo Valerio 
> 
> Thanks alot for working on this, should we perhaps reference the
> original bug report, i.e:
> Reported-by: Michael Plato 
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-discuss/2022-September/052065.html

Can be added while applying, I think.  It also may be worth adding
a sentence about fixing the assertion to the commit message.

> 
> We have a reproducer for the issue and we no longer see it occurring
> with this patch.
> Tested-by: Frode Nordahl 

Thanks!

> 
> Is there a plan for a backport, we have users on OVS 2.17 that would
> be interested in having this fixed, and AFAICT this patch does not
> cleanly backport.

Yes, the plan was to get this one reviewed, accepted and backported
down to 3.0.  3.1 and 3.0 seems to require only minor rebase (no support
for SCTP and parent key dumps).  Then post a backport patch for 2.17 to
get it reviewed, since it will have some extra modifications.

Aaron, I suppose your Ack from v2 still mostly relevant, but could
you please take another look at v3?

From my side the code looks fine:

Acked-by: Ilya Maximets 

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

[ovs-dev] [PATCH] dpdk: expose cpu usage stats on telemetry socket

[Robin Jarry]

Since DPDK 23.03, it is possible to register a callback to report lcore
TSC cycles usage. Reuse the busy/idle cycles gathering in dpif-netdev
and expose them to the DPDK telemetry socket.

Upon dpdk_attach_thread, record the mapping between the DPDK lcore_id
and the dpif-netdev core_id. Reuse that mapping in the lcore usage
callback to invoke dpif_netdev_get_pmd_cycles.

Here is an example output:

  ~# ovs-appctl dpif-netdev/pmd-stats-show | grep -e ^pmd -e cycles:
  pmd thread numa_id 0 core_id 8:
idle cycles: 2720796781680 (100.00%)
processing cycles: 3566020 (0.00%)
  pmd thread numa_id 0 core_id 9:
idle cycles: 2718974371440 (100.00%)
processing cycles: 3136840 (0.00%)
  pmd thread numa_id 0 core_id 72:
  pmd thread numa_id 0 core_id 73:

  ~# echo /eal/lcore/usage | dpdk-telemetry.py | jq
  {
"/eal/lcore/usage": {
  "lcore_ids": [
3,
5,
11,
15
  ],
  "total_cycles": [
2725722342740,
2725722347480,
2723899464040,
2725722354980
  ],
  "busy_cycles": [
3566020,
3566020,
3136840,
3566020
  ]
}
  }

Link: https://git.dpdk.org/dpdk/commit/?id=9ab1804922ba583b0b16
Cc: David Marchand 
Cc: Kevin Traynor 
Signed-off-by: Robin Jarry 
---
 lib/dpdk-stub.c   |  5 +++
 lib/dpdk.c| 95 ++-
 lib/dpdk.h|  5 +++
 lib/dpif-netdev.c | 38 +++
 4 files changed, 142 insertions(+), 1 deletion(-)

diff --git a/lib/dpdk-stub.c b/lib/dpdk-stub.c
index 58ebf6cb62cd..02fb561bea7b 100644
--- a/lib/dpdk-stub.c
+++ b/lib/dpdk-stub.c
@@ -49,6 +49,11 @@ dpdk_detach_thread(void)
 {
 }
 
+void
+dpdk_register_core_usage_callback(dpdk_core_usage_cb *cb OVS_UNUSED)
+{
+}
+
 bool
 dpdk_available(void)
 {
diff --git a/lib/dpdk.c b/lib/dpdk.c
index d76d53f8f16c..31871300f719 100644
--- a/lib/dpdk.c
+++ b/lib/dpdk.c
@@ -23,6 +23,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -310,6 +311,10 @@ malloc_dump_stats_wrapper(FILE *stream)
 rte_malloc_dump_stats(stream, NULL);
 }
 
+#ifdef ALLOW_EXPERIMENTAL_API
+static int dpdk_get_lcore_cycles(unsigned int, struct rte_lcore_usage *);
+#endif
+
 static bool
 dpdk_init__(const struct smap *ovs_other_config)
 {
@@ -440,6 +445,10 @@ dpdk_init__(const struct smap *ovs_other_config)
 /* We are called from the main thread here */
 RTE_PER_LCORE(_lcore_id) = NON_PMD_CORE_ID;
 
+#ifdef ALLOW_EXPERIMENTAL_API
+rte_lcore_register_usage_cb(dpdk_get_lcore_cycles);
+#endif
+
 /* Finally, register the dpdk classes */
 netdev_dpdk_register(ovs_other_config);
 netdev_register_flow_api_provider(&netdev_offload_dpdk);
@@ -490,9 +499,52 @@ dpdk_available(void)
 return initialized;
 }
 
+struct lcore_id_map {
+unsigned int lcore_id;
+unsigned int pmd_core_id;
+};
+
+/* Protects against changes to 'lcore_id_maps'. */
+struct ovs_mutex lcore_id_maps_mutex = OVS_MUTEX_INITIALIZER;
+
+/* Contains all 'struct lcore_id_map's. */
+static struct shash lcore_id_maps OVS_GUARDED_BY(lcore_id_maps_mutex)
+= SHASH_INITIALIZER(&lcore_id_maps);
+
+static void
+lcore_id_to_str(char *buf, size_t len, unsigned int lcore_id)
+{
+int n;
+
+n = snprintf(buf, len, "%u", lcore_id);
+if (n < 0) {
+VLOG_WARN("Failed to format lcore_id: %s", ovs_strerror(errno));
+n = 0;
+}
+buf[n] = '\0';
+}
+
+static void
+lcore_id_map_update(unsigned int lcore_id, unsigned int cpu, bool add)
+{
+char buf[128];
+
+lcore_id_to_str(buf, sizeof buf, lcore_id);
+
+ovs_mutex_lock(&lcore_id_maps_mutex);
+if (add) {
+shash_replace(&lcore_id_maps, buf, (void *) (uintptr_t) cpu);
+} else {
+shash_find_and_delete(&lcore_id_maps, buf);
+}
+ovs_mutex_unlock(&lcore_id_maps_mutex);
+}
+
 bool
 dpdk_attach_thread(unsigned cpu)
 {
+unsigned int lcore_id;
+
 /* NON_PMD_CORE_ID is reserved for use by non pmd threads. */
 ovs_assert(cpu != NON_PMD_CORE_ID);
 
@@ -506,7 +558,9 @@ dpdk_attach_thread(unsigned cpu)
 return false;
 }
 
-VLOG_INFO("PMD thread uses DPDK lcore %u.", rte_lcore_id());
+lcore_id = rte_lcore_id();
+lcore_id_map_update(lcore_id, cpu, true);
+VLOG_INFO("PMD thread uses DPDK lcore %u.", lcore_id);
 return true;
 }
 
@@ -516,10 +570,49 @@ dpdk_detach_thread(void)
 unsigned int lcore_id;
 
 lcore_id = rte_lcore_id();
+lcore_id_map_update(lcore_id, 0, false);
+
 rte_thread_unregister();
 VLOG_INFO("PMD thread released DPDK lcore %u.", lcore_id);
 }
 
+static dpdk_core_usage_cb_t *core_usage_cb;
+
+void
+dpdk_register_core_usage_callback(dpdk_core_usage_cb_t *cb)
+{
+core_usage_cb = cb;
+}
+
+#ifdef ALLOW_EXPERIMENTAL_API
+static int
+dpdk_get_lcore_cycles(unsigned int lcore_id, struct rte_lcore_usage *usage)
+{
+struct shash_node *node;
+unsigned int core_id;
+char buf[128];
+
+if (!core_usage_cb) {
+return

Re: [ovs-dev] [PATCH] dpdk: expose cpu usage stats on telemetry socket

[Robin Jarry]

Robin Jarry, Aug 31, 2023 at 15:11:
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 70b953ae6dd3..ebf43a0f62e4 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1427,6 +1427,41 @@ dpif_netdev_pmd_rebalance(struct unixctl_conn *conn, 
> int argc,
>  ds_destroy(&reply);
>  }
>  
> +static void
> +dpif_netdev_get_pmd_cycles(unsigned int core_id,
> +uint64_t *busy_cycles, uint64_t *total_cycles)
> +{
> +struct dp_netdev_pmd_thread **pmd_list = NULL;
> +uint64_t stats[PMD_N_STATS];
> +struct dp_netdev *dp;
> +size_t num_pmds;
> +
> +ovs_mutex_lock(&dp_netdev_mutex);
> +
> +if (shash_count(&dp_netdevs) != 1) {
> +goto out;
> +}
> +
> +dp = shash_first(&dp_netdevs)->data;
> +sorted_poll_thread_list(dp, &pmd_list, &num_pmds);
> +
> +for (size_t i = 0; i < num_pmds; i++) {
> +struct dp_netdev_pmd_thread *pmd = pmd_list[i];
> +
> +if (pmd->core_id == core_id) {
> +continue;
> +}

This logic is reversed. Too bad for the last minute cleanup/rework.

Before sending a v2, I'll wait to see if there are other things to
change.

By the way, this patch is targeted for the dpdk-latest branch. I forgot
to change the subject prefix. I'll do that for v2 as well.

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

Re: [ovs-dev] OVN: Move website maintenance scripts to ovn-org

[Dumitru Ceara]

On 8/30/23 18:42, Mark Michelson wrote:
> Hi everyone,
> 
> With the recent security releases of OVN, I pulled the trigger on
> https://github.com/ovn-org/ovn-website/pull/56 , which changed the OVN
> website to have its content generated by scripts and templated markdown.
> As a result, I was able to update the OVN website for the security
> releases with about 2 minutes of work.
> 
> The scripts and templates currently live in my github repo here:
> https://github.com/putnopvut/ovn-website-scripts .
> 
> I'm proposing that this repo should be in the ovn-org organization. This
> would allow for easier community contribution to the OVN website. What
> are your thoughts?

I didn't review the repo contents closely but the proposal makes
complete sense to me.

+1

Regards,
Dumitru

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

Re: [ovs-dev] [PATCH v2] ovsdb: raft: Support pre-vote mechanism to deal with disruptive server.

[Ilya Maximets]

On 8/16/23 14:52, Simon Horman wrote:
> On Tue, Jul 18, 2023 at 02:38:26AM -0700, Han Zhou wrote:
>> When a server becomes unstable due to system overloading or intermittent
>> partitioning, it may miss some heartbeats and then starts election with
>> a new term, which would disrupt the otherwise healthy cluster formed by
>> the rest of the healthy nodes.  Such situation may exist for a long time
>> until the "flapping" server is shutdown or recovered completely, which
>> can severely impact the availability of the cluster. The pre-vote
>> mechanism introduced in the raft paper section 9.6 can prevent such
>> problems. This patch implements the pre-vote mechanism.
>>
>> Note: during the upgrade, since the old version doesn't recognize the
>> new optional field in the vote rpc (and the ovsdb_parse_finish validates
>> that all fields in the jsonrpc are parsed), an error log may be noticed
>> on old nodes if an upgraded node happens to become candidate first and
>> vote for itself, and the vote request will be discarded. If this happens
>> before enough nodes complete the upgrade, the vote from the upgraded
>> node may not reach the quorum. This results in re-election, and any old
>> nodes should be able to vote and get elected as leader. So, in unlucky
>> cases there can be more leader elections happening during the upgrade.
>>
>> Signed-off-by: Han Zhou 
> 
> Reviewed-by: Simon Horman 
> 

Thanks, Han and Simon!

I wrapped a few long lines in the test and applied the change.

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

Re: [ovs-dev] [PATCH ovn v6 00/16] northd: I-P for load balancer and lb groups

[Mark Michelson]

I had a look at patches 1-8. I have no additional notes beyond what Han 
and Ales have already mentioned.


For patches 1-8:

Acked-by: Mark Michelson 

On 8/18/23 04:56, num...@ovn.org wrote:

From: Numan Siddique 

This patch series adds the support to handle load balancer and
load balancer group changes incrementally in the "northd" engine
node and "lflow" engine node.  Changes to logical switches and router's load
balancer and load balancer group columns are also handled incrementally
provided other columns do not change.

V4 of this series did not include LB I-P handling in the lflow engine
node.  V5 adds 6 more patches to handle the LB changes in the lflow
engine node.  V6 added 2 additional patches to handle router NAT I-P
handling.

This patch series can be divided into 3 parts

Part 1.  Patches 1 to 8  (LB I-P only in northd)
Part 2.  Patches 9 to 14 (LB I-P in lflow engine too)
Part 3.  Patches 15 and 16  (LR NAT I-P handling in both northd and
lflow)

Submitting all these patches as one series.  These patches can also be
found here - https://github.com/numansiddique/ovn/tree/northd_ip_lb_ip_v6

If there are any conflicts,  request the reviewers to use this branch
for reviewing.

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 these patches applied (with load balancer I-P handling in both
northd and lflow engine nodes) the resuts (Result 1) are:

---
 Min (s) Median (s)  90%ile (s)  99%ile 
(s)  Max (s) Mean (s)Total (s)   Count   Failed
---
Iteration Total 0.1356511.1305271.179357
1.2014102.1802030.67460684.325717   125 0
Namespace.add_ports 0.0052180.0056780.006457
0.0189360.0208120.0061820.772796125 0
WorkerNode.bind_port0.0336310.0432870.051171
0.0582230.0628190.04383910.959757   250 0
WorkerNode.ping_port0.0054600.0067911.041434
1.0648071.0699570.27435268.587878   250 0
---


With only the first 8 patches applied (with load balancer I-P handling
only in northd engine node) the results (Result 2) are:


---
 Min (s) Median (s)  90%ile (s)  99%ile 
(s)  Max (s) Mean (s)Total (s)   Count   Failed
---
Iteration Total 0.1329292.1571033.314847
3.3315614.3786261.581889197.736147  125 0
Namespace.add_ports 0.0052170.0057600.006565
0.0133480.0210140.0061060.763214125 0
WorkerNode.bind_port0.0352050.0454580.052278
0.0598040.0639410.04565211.413122   250 0
WorkerNode.ping_port0.0050750.0068143.088548
3.1925774.2420260.726453181.613284  250 0
---


The results with the present main (Result 3) are:

---
 Min (s) Median (s)  90%ile (s)  99%ile 
(s)  Max (s) Mean (s)Total (s)   Count   Failed
---
Iteration Total 4.3772606.4869627.502040
8.3225878.3347016.559002819.875306  125 0
Namespace.add_ports 0.0051120.0054840.005953
0.0091530.0114520.0056620.707752125 0
WorkerNode.bind_port0.0353600.0427320.049152
0.0536980.0566350.04321510.803700   250 0
WorkerNode.ping_port0.005338   

Re: [ovs-dev] [PATCH v3] conntrack: Remove nat_conn introducing key directionality.

[Aaron Conole]

Ilya Maximets  writes:

> On 8/31/23 09:15, Frode Nordahl wrote:
>> On Wed, Aug 30, 2023 at 9:30 PM Paolo Valerio  wrote:
>>>
>>> From: hepeng 
>>>
>>> The patch avoids the extra allocation for nat_conn.
>>> Currently, when doing NAT, the userspace conntrack will use an extra
>>> conn for the two directions in a flow. However, each conn has actually
>>> the two keys for both orig and rev directions. This patch introduces a
>>> key_node[CT_DIRS] member as per Aaron's suggestion in the conn which
>>> consists of a key, direction, and a cmap_node for hash lookup so
>>> addressing the feedback received by the original patch [0].
>>>
>>> [0]
>>> https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/
>>>
>>> Signed-off-by: Peng He 
>>> Co-authored-by: Paolo Valerio 
>>> Signed-off-by: Paolo Valerio 
>> 
>> Thanks alot for working on this, should we perhaps reference the
>> original bug report, i.e:
>> Reported-by: Michael Plato 
>> Reported-at: 
>> https://mail.openvswitch.org/pipermail/ovs-discuss/2022-September/052065.html
>
> Can be added while applying, I think.  It also may be worth adding
> a sentence about fixing the assertion to the commit message.

Done.

>> 
>> We have a reproducer for the issue and we no longer see it occurring
>> with this patch.
>> Tested-by: Frode Nordahl 
>
> Thanks!

Thanks everyone!  I've applied and backported down to branch-3.0, and
will work on the backport to branch-2.17.

>> 
>> Is there a plan for a backport, we have users on OVS 2.17 that would
>> be interested in having this fixed, and AFAICT this patch does not
>> cleanly backport.
>
> Yes, the plan was to get this one reviewed, accepted and backported
> down to 3.0.  3.1 and 3.0 seems to require only minor rebase (no support
> for SCTP and parent key dumps).  Then post a backport patch for 2.17 to
> get it reviewed, since it will have some extra modifications.
>
> Aaron, I suppose your Ack from v2 still mostly relevant, but could
> you please take another look at v3?
>
> From my side the code looks fine:
>
> Acked-by: Ilya Maximets 
>
> Best regards, Ilya Maximets.

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

Re: [ovs-dev] [PATCH v3] conntrack: Add a test for IPv4 UDP zero checksum

[Aaron Conole]

Eelco Chaudron  writes:

> On 30 Aug 2023, at 19:56, Aaron Conole wrote:
>
>> In the past, during some conntrack testing a bug was uncovered in a DPDK
>> PMD which didn't support an IPv4 packet with a zero checksum value.
>> In order to show that the connection tracking code in userspace
>> supports IPv4 UDP with a zero checksum, add a test case to enforce
>> this behavior
>>
>> Reported-at: http://mails.dpdk.org/archives/dev/2021-January/198528.html
>> Reported-by: Paolo Valerio 
>> Signed-off-by: Aaron Conole 
>> ---
>>  v1->v2: - Addressed most of the comments by Flavio & William Tu.
>>  - Added the 0x checksum case
>>  - Added a bad checksum case
>>  - For the single instance of "sleep 1," this needs to be retained
>>due to the negative test case (we have no WAIT_UNTIL to rely on)
>>but there are other instances of sleep 1 throughout the suite,
>>so I guess it should be okay.
>
> Hi Aaron,
>
> I think the patch looks good, however, can we just not wait for the
> drop actions to appear, and if it does then do the check for the empty
> conntrack table? Or am I missing something?

Most likely we can.  I'll update the test to WAIT for the drop
condition.

> Cheers,
>
> Eelco
>
>
>>  v2->v3: - Add a check for the ofproto dropped packet stats
>>  - Fix whitespace issue
>>  - Restricted the dump-conntrack checks to keep the possibility of
>>UDP queries poisoning the test case lower.
>>
>>  tests/system-traffic.at | 67 +
>>  1 file changed, 67 insertions(+)
>>
>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>> index 808c492a22..65c44c4350 100644
>> --- a/tests/system-traffic.at
>> +++ b/tests/system-traffic.at
>> @@ -3813,6 +3813,73 @@ OVS_WAIT_UNTIL([ovs-pcap p0.pcap | grep -q 
>> "f0010101f0010102080045c00045
>>  OVS_TRAFFIC_VSWITCHD_STOP
>>  AT_CLEANUP
>>
>> +AT_SETUP([conntrack - IPv4 UDP zero checksum])
>> +dnl Checks sending zero checksum packets for udp over ipv4
>> +CHECK_CONNTRACK()
>> +OVS_TRAFFIC_VSWITCHD_START()
>> +OVS_CHECK_CT_CLEAR()
>> +
>> +ADD_NAMESPACES(at_ns0, at_ns1)
>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01")
>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02")
>> +
>> +dnl Setup conntrack flows
>> +AT_DATA([flows.txt], [dnl
>> +table=0,priority=10  ip,udp,ct_state=-trk action=ct(zone=1,table=1)
>> +table=0,priority=0   action=drop
>> +table=1,priority=10  ct_state=-est+trk+new,ip,ct_zone=1,in_port=1 
>> action=ct(commit,table=2)
>> +table=1,priority=10  ct_state=+est-new+trk,ct_zone=1,in_port=1 
>> action=resubmit(,2)
>> +table=1,priority=0   action=drop
>> +table=2,priority=10  ct_state=+trk+new,in_port=1 action=2
>> +table=2,priority=10  ct_state=+trk+est action=2
>> +])
>> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
>> +
>> +dnl sending udp pkt with  checksum
>> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 f0 00 00 01 01 02 
>> f0 00 00 01 01 01 08 00 45 00 00 28 00 01 00 00 40 11 64 c0 0a 01 01 01 0a 
>> 01 01 02 04 d2 04 d2 00 14 00 00 aa aa aa aa aa aa aa aa aa aa aa aa > 
>> /dev/null])
>> +
>> +OVS_WAIT_UNTIL([ovs-appctl dpctl/dump-conntrack | grep "udp"])
>> +
>> +dnl ensure CT picked up the packet
>> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1)], [0], [dnl
>> +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=,dport=),reply=(src=10.1.1.2,dst=10.1.1.1,sport=,dport=)
>> +])
>> +
>> +dnl clear CT tuples
>> +AT_CHECK([ovs-appctl dpctl/flush-conntrack])
>> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "udp"], [1])
>> +
>> +dnl send UDP with  checksum
>> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 f0 00 00 01 01 02 
>> f0 00 00 01 01 01 08 00 45 00 00 40 00 01 00 00 40 11 64 a8 0a 01 01 01 0a 
>> 01 01 02 04 d2 04 d2 00 2c ff ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 df ed > 
>> /dev/null])
>> +OVS_WAIT_UNTIL([ovs-appctl dpctl/dump-conntrack | grep "udp"])
>> +
>> +dnl ensure CT picked up the packet
>> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1)], [0], [dnl
>> +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=,dport=),reply=(src=10.1.1.2,dst=10.1.1.1,sport=,dport=)
>> +])
>> +
>> +dnl clear CT tuples
>> +AT_CHECK([ovs-appctl dpctl/flush-conntrack])
>> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "udp"], [1])
>> +
>> +dnl sending udp pkt with bad checksum
>> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 f0 00 00 01 01 02 
>> f0 00 00 01 01 01 08 00 45 00 00 28 00 01 00 00 40 11 64 c0 0a 01 01 01 0a 
>> 01 01 02 04 d2 04 d2 00 14 11 11 aa aa aa aa aa aa aa aa aa aa aa aa > 
>> /dev/null])
>> +
>> +dnl We call a sleep here to ensure that we let the system get through any
>> +dnl needed upcalls.  Once the sleep is expired, we can check the drop 
>> counter
>> +dnl in table 1 for our bad csum packet, and ensure that there are no entries
>> +dnl in 

Re: [ovs-dev] [PATCH v4 ovn] controller: make garp_max_timeout configurable

[Mark Michelson]

Thank you Lorenzo and Ales. I added an item to the NEWS file about this 
and pushed the change to main.


On 8/31/23 03:34, Ales Musil wrote:

On Wed, Aug 30, 2023 at 7:24 PM Lorenzo Bianconi
 wrote:


When using VLAN backed networks and OVN routers leveraging the
'ovn-chassis-mac-mappings' option for east-west traffic, the eth.src field is
replaced by the chassis mac address in order to not expose the router mac
address from different nodes and confuse the TOR switch. However doing so
the TOR switch is not able to learn the port/mac bindings for routed E/W
traffic and it is force to always flood it. Fix this issue adding the
capability to configure a given timeout for garp sent by ovn-controller
and not disable it after the exponential backoff in order to keep
refreshing the entries in TOR swtich fdb table.
More into about the issue can be found here [0].

[0] 
https://mail.openvswitch.org/pipermail/ovs-discuss/2020-September/050678.html
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2087779

Signed-off-by: Lorenzo Bianconi 
---
Changes since v3:
- simplify code logic

Changes since v2:
- cap exponential backoff timeout to garp_max_timeout

Changes since v1:
- add uni-test
- add documentation
---
  controller/ovn-controller.8.xml | 11 +
  controller/ovn-controller.c |  4 +-
  controller/pinctrl.c| 73 +++--
  controller/pinctrl.h|  4 +-
  tests/ovn.at| 16 
  5 files changed, 93 insertions(+), 15 deletions(-)

diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
index 0883d8da9..7b4100592 100644
--- a/controller/ovn-controller.8.xml
+++ b/controller/ovn-controller.8.xml
@@ -365,6 +365,17 @@
  heplful to pin source outer IP for the tunnel when multiple interfaces
  are used on the host for overlay traffic.

+  external_ids:garp-max-timeout-sec
+  
+When used, this configuration value specifies the maximum timeout
+(in seconds) between two consecutive GARP packets sent by
+ovn-controller.
+ovn-controller by default sends just 4 GARP packets
+with an exponential backoff timeout.
+Setting external_ids:garp-max-timeout-sec allows to
+cap for the exponential backoff used by ovn-controller
+to send GARPs packets.
+  
  

  
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 1e49f423f..90bb80d56 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -5787,7 +5787,9 @@ main(int argc, char *argv[])
  &runtime_data->local_datapaths,
  &runtime_data->active_tunnels,
  &runtime_data->local_active_ports_ipv6_pd,
-&runtime_data->local_active_ports_ras);
+&runtime_data->local_active_ports_ras,
+ovsrec_open_vswitch_table_get(
+ovs_idl_loop.idl));
  stopwatch_stop(PINCTRL_RUN_STOPWATCH_NAME,
 time_msec());
  mirror_run(ovs_idl_txn,
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index bed90fe0b..c280d2256 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -166,6 +166,10 @@ static struct ovs_mutex pinctrl_mutex = 
OVS_MUTEX_INITIALIZER;
  static struct seq *pinctrl_handler_seq;
  static struct seq *pinctrl_main_seq;

+#define GARP_RARP_DEF_MAX_TIMEOUT16000
+static long long int garp_rarp_max_timeout = GARP_RARP_DEF_MAX_TIMEOUT;
+static bool garp_rarp_continuous;
+
  static void *pinctrl_handler(void *arg);

  struct pinctrl {
@@ -227,7 +231,8 @@ static void send_garp_rarp_prepare(
  const struct ovsrec_bridge *,
  const struct sbrec_chassis *,
  const struct hmap *local_datapaths,
-const struct sset *active_tunnels)
+const struct sset *active_tunnels,
+const struct ovsrec_open_vswitch_table *ovs_table)
  OVS_REQUIRES(pinctrl_mutex);
  static void send_garp_rarp_run(struct rconn *swconn,
 long long int *send_garp_rarp_time)
@@ -3492,7 +3497,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
  const struct hmap *local_datapaths,
  const struct sset *active_tunnels,
  const struct shash *local_active_ports_ipv6_pd,
-const struct shash *local_active_ports_ras)
+const struct shash *local_active_ports_ras,
+const struct ovsrec_open_vswitch_table *ovs_table)
  {
  ovs_mutex_lock(&pinctrl_mutex);
  run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
@@ -3503,7 +3509,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
  send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath,
 sbrec_port_bin

Re: [ovs-dev] [PATCH ovn v2] northd: Add incremental processing for NB port groups.

[Mark Michelson]

Thanks Ales and Dumitru. I pushed this change to main.

On 8/31/23 05:42, Ales Musil wrote:



On Wed, Aug 30, 2023 at 3:50 PM Dumitru Ceara > wrote:


It's similar to the processing we do for address sets.  There's a bit
more mechanics involved due to the fact that we need to split NB port
groups per datapath.

We currently only partially implement incremental processing of
port_group changes in the lflow node.  That is, we deal with the case
when the sets of "switches per port group" doesn't change.  In that
specific case ACL lflows don't need to be reprocessed.

In a synthetic benchmark that created (in this order):
- 500 switches
- 2000 port groups
- 4 ACLs per port group
- 1 ports distributed equally between the switches and port groups

we measured the following ovn-northd CPU usage:

   +-+++
   | Incremental processing? | --wait=sb? | northd avg cpu (%) |
   +-+++
   |           N             |     Y      |        84.2        |
   +-+++
   |           Y             |     Y      |        41.5        |
   +-+++
   |           N             |     N      |        93.2        |
   +-+++
   |           Y             |     N      |        53.6        |
   +-+++

where '--wait=sb' set to 'Y'  means the benchmark was waiting for the
port and port group operations to be propagated to the Southbound DB
before continuing to the next operation.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2228162

Signed-off-by: Dumitru Ceara mailto:dce...@redhat.com>>
---
V2:
- First 4 patches of the series merged, only respun the 5th one.
- Addressed Han's comments:
   - reversed semantics of ls_port_groups_sets_unchanged
   - changed "struct port_group_to_ls_table" to "struct
port_group_ls_table"
   - changed "struct port_group_to_ls" to "struct port_group_ls_record"
   - fixed bug due to missing recompute whenever a per-LS port group
(but
     not
     the last one) was deleted
   - handled SB port group creation in the I-P handler (normally
that should
     not happen but if the SB is manually changed we now reconcile it
     without trigerring a recompute)
   - fixed up some comment typos
   - changed error checks in ls_port_group_record_clear() into assert()
     calls.
   - updated unit test to include missing scenarios
   - added CHECK_NO_CHANGE_AFTER_RECOMPUTE calls to unit test
   - added port group check to _DUMP_DB_TABLES
- Addressed Ales' comments:
   - use xmalloc instead of xzalloc followed by full initialization
of the
     structure
- Other changes:
   - unified I-P handler processing for port_group_ls_record
     creation/deletion
---
  northd/en-lflow.c        |  17 ++
  northd/en-lflow.h        |   1 +
  northd/en-port-group.c   | 452 ++-
  northd/en-port-group.h   |  36 +++-
  northd/inc-proc-northd.c |  13 +-
  northd/ovn-northd.c      |   4 +
  tests/ovn-northd.at       | 371

  7 files changed, 832 insertions(+), 62 deletions(-)

diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index c6c0d4ebbf..153bf0f57a 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -122,6 +122,23 @@ lflow_northd_handler(struct engine_node *node,
      return true;
  }

+bool
+lflow_port_group_handler(struct engine_node *node, void *data
OVS_UNUSED)
+{
+    struct port_group_data *pg_data =
+        engine_get_input_data("port_group", node);
+
+    /* If the set of switches per port group didn't change then
there's no
+     * need to reprocess lflows.  Otherwise, there might be a need to
+     * add/delete port-group ACLs to/from switches. */
+    if (pg_data->ls_port_groups_sets_changed) {
+        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 5e3fbc25e3..5417b2faff 100644
--- a/northd/en-lflow.h
+++ b/northd/en-lflow.h
@@ -13,5 +13,6 @@ void en_lflow_run(struct engine_node *node, void
*data);
  void *en_lflow_init(struct engine_node *node, struct engine_arg *arg);
  void

Re: [ovs-dev] [PATCH ovn] northd.c: Incremental processing for first/last switch port change.

[Mark Michelson]

Hi Han, thanks for the change.

Acked-by: Mark Michelson 

On 8/17/23 12:14, Han Zhou wrote:

On Thu, Aug 17, 2023 at 2:13 AM Ales Musil  wrote:




On Wed, Aug 16, 2023 at 2:56 AM Han Zhou  wrote:


Commit 1eb09838 fixed an incremental processing problem by falling back
to recompute. The problem was handling VIF changes for a LS that
has router ports when:
- adding the first switch port to the LS, or
- deleting the last switch port from the LS
because there are router port related lflows that depends on the
condition whether there are only router ports on the LS or not.

Since such scenario is common, this patch further improves it to avoid
falling back to recompute.

Signed-off-by: Han Zhou 



Hi Han,

there is one small nit down below.


---
  northd/northd.c | 71 +
  northd/northd.h |  1 +
  tests/ovn-northd.at |  6 ++--
  3 files changed, 45 insertions(+), 33 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 9a12a94ae25d..a87298a026de 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -5120,21 +5120,8 @@ northd_handle_ls_changes(struct ovsdb_idl_txn

*ovnsb_idl_txn,

  op->visited = false;
  }

-/* Check if the logical switch has only router ports before

this change.

- * If so, fall back to recompute.
- * lflow engine node while building the lflows checks if the

logical switch

- * has any router ports and depending on that it adds different

flows.

- * See build_lswitch_rport_arp_req_flow() for more details.
- * Note: We can definitely handle this scenario incrementally

in the

- * northd engine node and fall back to recompute in lflow

engine node

- * and even handle this incrementally in lflow node.  Until we

do that

- * resort to full recompute of northd node.
- */
-bool only_rports = (od->n_router_ports
-&& (od->n_router_ports ==

hmap_count(&od->ports)));

-if (only_rports) {
-goto fail;
-}
+ls_change->had_only_router_ports = (od->n_router_ports
+&& (od->n_router_ports == hmap_count(&od->ports)));

  /* Compare the individual ports in the old and new Logical

Switches */

  for (size_t j = 0; j < changed_ls->n_ports; ++j) {
@@ -5217,22 +5204,6 @@ northd_handle_ls_changes(struct ovsdb_idl_txn

*ovnsb_idl_txn,

  }
  }

-/* Check if the logical switch has only router ports after this

change.

- * If so, fall back to recompute.
- * lflow engine node while building the lflows checks if the

logical switch

- * has any router ports and depending on that it adds different

flows.

- * See build_lswitch_rport_arp_req_flow() for more details.
- * Note: We can definitely handle this scenario incrementally

in the

- * northd engine node and fall back to recompute in lflow

engine node

- * and even handle this incrementally in lflow node.  Until we

do that

- * resort to full recompute of northd node.
- */
-only_rports = (od->n_router_ports
-   && (od->n_router_ports ==

hmap_count(&od->ports)));

-if (only_rports) {
-goto fail_clean_deleted;
-}
-
  if (!ovs_list_is_empty(&ls_change->added_ports) ||
  !ovs_list_is_empty(&ls_change->updated_ports) ||
  !ovs_list_is_empty(&ls_change->deleted_ports)) {
@@ -16550,6 +16521,44 @@ bool lflow_handle_northd_ls_changes(struct

ovsdb_idl_txn *ovnsb_txn,

lfrn->lflow);
  }
  }
+
+bool ls_has_only_router_ports = (ls_change->od->n_router_ports

&&

+ (ls_change->od->n_router_ports

==

+

  hmap_count(&ls_change->od->ports)));

+
+if (ls_change->had_only_router_ports !=

ls_has_only_router_ports) {

+/* There are lflows related to router ports that depends on

whether

+ * there are switch ports on the logical switch (see
+ * build_lswitch_rport_arp_req_flow() for more details).

Since this

+ * dependency changed, we need to regenerate lflows for

each router

+ * port on this logical switch. */
+for (size_t i = 0; i < ls_change->od->n_router_ports; i++) {
+op = ls_change->od->router_ports[i];
+
+/* Delete old lflows. */
+if (!delete_lflow_for_lsp(op, "affected router",
+

  lflow_input->sbrec_logical_flow_table,

+  lflows)) {
+return false;
+}
+
+/* Generate new lflows. */
+struct ds match = DS_EMPTY_INITIALIZER;
+struct ds actions = DS_EMPTY_INITIALIZER;
+build_lswitch_and_lrouter_iterate_by_lsp(op,
+lflow_input->ls_

Re: [ovs-dev] [PATCH v3 ovn] northd: support binding remote ports in ovn-northd

[Mark Michelson]

Thanks Lorenzo,

Acked-by: Mark Michelson 

There is one small issue, but it can be fixed when this is merged. See 
below.


On 8/25/23 12:34, Lorenzo Bianconi wrote:

ovn supports creating remote logical ports. An user
can set requested-chassis option for a logical switch port
to the remote chassis and ovn-northd can bind it to that chassis.
This is required for OVN IC in ovn-k8s. Right now ovn-k8s
ovnkube-controller after creating a remote logical port, sets the
chassis column of the corresponding port binding in SB DB to the
remote chassis. This process can be implemented in ovn-northd.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2217930
Signed-off-by: Lorenzo Bianconi 
---
Changes since v2:
- address missing logica cases
- add more unit tests

Changes since v1:
- add NEWS entry
- do not remove ovn-ic code to bind sb to gw chassis
- simplify codebase
---
  NEWS|  2 ++
  ic/ovn-ic.c |  5 
  northd/northd.c | 28 +++
  tests/ovn-ic.at |  2 ++
  tests/ovn-northd.at | 67 +
  tests/ovn.at| 27 ++
  6 files changed, 131 insertions(+)

diff --git a/NEWS b/NEWS
index 93d4bedcd..d7580d9bd 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,8 @@ Post v23.06.0
  Existing sessions might get re-hashed to a different ECMP path when
  OVN detects the algorithm support in the datapath during an upgrade
  or restart of ovn-controller.
+  - Introduce support for binding remote ports in ovn-northd if the CMS sets
+requested-chassis option for a remote logical switch port.
  
  OVN v23.06.0 - 01 Jun 2023

  --
diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index db7e86bc1..2a8383523 100644
--- a/ic/ovn-ic.c
+++ b/ic/ovn-ic.c
@@ -651,6 +651,11 @@ sync_remote_port(struct ic_context *ctx,
  /* Sync tunnel key from ISB to NB */
  sync_lsp_tnl_key(lsp, isb_pb->tunnel_key);
  
+/* Skip port binding if it is already requested by the CMS. */

+if (smap_get(&lsp->options, "requested-chassis")) {
+return;
+}
+
  /* Sync gateway from ISB to SB */
  if (isb_pb->gateway[0]) {
  if (!sb_pb->chassis || strcmp(sb_pb->chassis->name, isb_pb->gateway)) 
{
diff --git a/northd/northd.c b/northd/northd.c
index a01f59efb..e6b37e66a 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3579,6 +3579,28 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
  }
  }
  
+if (lsp_is_remote(op->nbsp)) {

+/* ovn-northd is supposed to set port_binding for remote ports
+ * if requested chassis is marked as remote. */
+if (op->sb->requested_chassis &&
+smap_get_bool(&op->sb->requested_chassis->other_config,
+  "is-remote", false)) {
+sbrec_port_binding_set_chassis(op->sb,
+   op->sb->requested_chassis);
+smap_add(&options, "is-remote-nb-binded", "true");


The name of this option should be "is-remote-nb-bound" instead of 
"is-remote-nb-binded."



+} else if (smap_get_bool(&op->sb->options,
+ "is-remote-nb-binded", false)) {
+sbrec_port_binding_set_chassis(op->sb, NULL);
+smap_add(&options, "is-remote-nb-binded", "false");
+}
+} else if (op->sb->chassis &&
+   smap_get_bool(&op->sb->chassis->other_config,
+ "is-remote", false)) {
+/* Its not a remote port but if the chassis is set and if its a
+ * remote chassis then clear it. */
+sbrec_port_binding_set_chassis(op->sb, NULL);
+}
+
  sbrec_port_binding_set_options(op->sb, &options);
  smap_destroy(&options);
  if (ovn_is_known_nb_lsp_type(op->nbsp->type)) {
@@ -3616,6 +3638,12 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
  sbrec_port_binding_set_ha_chassis_group(op->sb, NULL);
  }
  } else {
+if (op->sb->chassis &&
+smap_get_bool(&op->sb->chassis->other_config,
+  "is-remote", false)) {
+sbrec_port_binding_set_chassis(op->sb, NULL);
+}
+
  const char *chassis = NULL;
  if (op->peer && op->peer->od && op->peer->od->nbr) {
  chassis = smap_get(&op->peer->od->nbr->options, "chassis");
diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
index 09eac860c..66368e7c9 100644
--- a/tests/ovn-ic.at
+++ b/tests/ovn-ic.at
@@ -336,6 +336,7 @@ ovn-nbctl lsp-set-addresses lsp-ts1-lr1 router
  ovn-nbctl lsp-set-type lsp-ts1-lr1 router
  ovn-nbctl --wait=hv lsp-set-options lsp-ts1-lr1 router-port=lrp-lr1-ts1
  
+ovn_as az2 ovn-nbctl lsp-set-options lsp-ts1-lr1 reque

Re: [ovs-dev] [PATCH ovn] ovs: Bump submodule to branch-3.2

[Mark Michelson]

Thanks for this Ales.

Acked-by: Mark Michelson 

Which branches is this a candidate for? Main only?

On 8/31/23 03:27, Ales Musil wrote:

Bump submodule to branch-3.2 mainly for:
759a29dc2d97 ("backtrace: Extend the backtrace functionality.")
1d78a3f3164a ("netdev-dpdk: Disable net/tap Tx L4 checksum offloads.")

Unfortunately we cannot use the tag because we need
the DPDK offload fix, which was merged later on.

At the same time fix three issues caused by the bump.

Remove our custom 'struct sctp_chunk_header' that was added
to packets.h as part of:
501f665a5a4b ("conntrack: Extract l4 information for SCTP.")

Adjust "daemonize_start" to accept two parameters
and set the second to false, as we don't need access
to DPDK devices directly at this moment. This was introduced
by commit:
07cf5810de8d ("dpdk: Allow retaining CAP_SYS_RAWIO privileges.")

Adjust the path for OvS python helpers, introduced by:
bb0dd1135ba9 ("python: Rename build related code to ovs_build_helpers.")

Reported-at: https://bugzilla.redhat.com/2164058
Signed-off-by: Ales Musil 
---
  Makefile.am   |  2 +-
  build-aux/sodepends.py|  2 +-
  build-aux/soexpand.py |  2 +-
  build-aux/xml2nroff   | 14 +++---
  controller-vtep/ovn-controller-vtep.c |  2 +-
  controller/ovn-controller.c   |  2 +-
  controller/pinctrl.c  | 10 +-
  ic/ovn-ic.c   |  2 +-
  lib/ovn-util.h|  7 ---
  northd/ovn-northd.c   |  2 +-
  ovs   |  2 +-
  utilities/ovn-dbctl.c |  2 +-
  utilities/ovn-trace.c |  2 +-
  13 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 27182c7bc..b58d4a501 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -430,7 +430,7 @@ endif
  CLEANFILES += flake8-check
  
  include $(srcdir)/manpages.mk

-$(srcdir)/manpages.mk: $(MAN_ROOTS) build-aux/sodepends.py 
$(OVS_SRCDIR)/python/build/soutil.py
+$(srcdir)/manpages.mk: $(MAN_ROOTS) build-aux/sodepends.py 
$(OVS_SRCDIR)/python/ovs_build_helpers/soutil.py
@PYTHONPATH=$(OVS_SRCDIR)/python$(psep)$$PYTHONPATH$(psep)$(srcdir)/python 
$(PYTHON3) $(srcdir)/build-aux/sodepends.py -I. -Isrcdir,$(srcdir) 
-IOVS_MANDIR,$(OVS_MANDIR) $(MAN_ROOTS) >$(@F).tmp
@if cmp -s $(@F).tmp $@; then \
  touch $@; \
diff --git a/build-aux/sodepends.py b/build-aux/sodepends.py
index 7b1f9c840..c04a5c681 100755
--- a/build-aux/sodepends.py
+++ b/build-aux/sodepends.py
@@ -14,7 +14,7 @@
  # See the License for the specific language governing permissions and
  # limitations under the License.
  
-from build import soutil

+from ovs_build_helpers import soutil
  import sys
  import getopt
  import os
diff --git a/build-aux/soexpand.py b/build-aux/soexpand.py
index 00adcf47a..f9ab80963 100755
--- a/build-aux/soexpand.py
+++ b/build-aux/soexpand.py
@@ -14,7 +14,7 @@
  # See the License for the specific language governing permissions and
  # limitations under the License.
  
-from build import soutil

+from ovs_build_helpers import soutil
  import sys
  
  
diff --git a/build-aux/xml2nroff b/build-aux/xml2nroff

index 9e781a396..dd9577204 100755
--- a/build-aux/xml2nroff
+++ b/build-aux/xml2nroff
@@ -18,7 +18,7 @@ import getopt
  import sys
  import xml.dom.minidom
  
-import build.nroff

+from ovs_build_helpers import nroff
  
  argv0 = sys.argv[0]
  
@@ -94,12 +94,12 @@ def manpage_to_nroff(xml_file, subst, include_path, version=None):

  .  PP
  .  I "\\$1"
  ..
-''' % (build.nroff.text_to_nroff(program),
-   build.nroff.text_to_nroff(section),
-   build.nroff.text_to_nroff(title),
-   build.nroff.text_to_nroff(version))
+''' % (nroff.text_to_nroff(program),
+   nroff.text_to_nroff(section),
+   nroff.text_to_nroff(title),
+   nroff.text_to_nroff(version))
  
-s += build.nroff.block_xml_to_nroff(doc.childNodes) + "\n"

+s += nroff.block_xml_to_nroff(doc.childNodes) + "\n"
  
  return s
  
@@ -145,7 +145,7 @@ if __name__ == "__main__":
  
  try:

  s = manpage_to_nroff(args[0], subst, include_path, version)
-except build.nroff.error.Error as e:
+except nroff.error.Error as e:
  sys.stderr.write("%s: %s\n" % (argv0, e.msg))
  sys.exit(1)
  for line in s.splitlines():
diff --git a/controller-vtep/ovn-controller-vtep.c 
b/controller-vtep/ovn-controller-vtep.c
index 5f017d87d..23b368179 100644
--- a/controller-vtep/ovn-controller-vtep.c
+++ b/controller-vtep/ovn-controller-vtep.c
@@ -116,7 +116,7 @@ main(int argc, char *argv[])
  parse_options(argc, argv);
  fatal_ignore_sigpipe();
  
-daemonize_start(false);

+daemonize_start(false, false);
  
  char *abs_unixctl_path = get_abs_unix_ctl_path(NULL);

  retval = unixctl_server_create(abs_unixctl_path, &unixctl);
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.

Re: [ovs-dev] [PATCH ovn v6 05/16] northd: Handle load balancer changes for a logical switch.

[Han Zhou]

On Fri, Aug 18, 2023 at 1:58 AM  wrote:
>
> From: Numan Siddique 
>
> 'lb_data' engine node now also handles logical switch changes.
> Its data maintains ls to lb related information. i.e if a
> logical switch sw0 has lb1, lb2 and lb3 associated then
> it stores this info in its data.  And when a new load balancer
> lb4 is associated to it, it stores this information in its
> tracked data so that 'northd' engine node can handle it
> accordingly.  Tracked data will have information like:
>   changed ls -> {sw0 : {associated_lbs: [lb4]}
>
> In the 'northd' engne node, an additional handler for 'lb_data'
> is added after the 'nbrec_logical_switch' changes.  With this patch
> we now have 2 handlers for 'lb_data' in 'northd' engine node.
>
> 
> engine_add_input(&en_northd, &en_lb_data, northd_lb_data_handler_pre_od);
> engine_add_input(&en_northd, &en_nb_logical_switch,
>  northd_nb_logical_switch_handler);
> engine_add_input(&en_northd, &en_lb_data, northd_lb_data_handler_post_od);
> 
>
> The first handler 'northd_lb_data_handler_pre_od' is called before the
> 'northd_nb_logical_switch_handler' handler and it just creates or
> deletes the lb_datapaths hmap for the tracked lbs.
>
> The second handler 'northd_lb_data_handler_post_od' is called after
> the 'northd_nb_logical_switch_handler' handler and it updates the
> ovn_lb_datapaths's 'nb_ls_map' bitmap.
>
> Eg.  If the lb_data has the below tracked data:
>
> tracked_data = {'crupdated_lbs': [lb1, lb2],
> 'deleted_lbs': [lb3],
> 'crupdated_lb_groups': [lbg1, lbg2],
> 'crupdated_ls_lbs': [{ls: sw0, assoc_lbs: [lb1],
>  {ls: sw1, assoc_lbs: [lb1, lb2]}
>
> then the first handler northd_lb_data_handler_pre_od(), creates the
> ovn_lb_datapaths object for lb1 and lb2 and deletes lb3 from
> the ovn_lb_datapaths hmap.  Similarly for the created or updated lb
> groups lbg1 and lbg2.
>
> The second handler northd_lb_data_handler_post_od() updates the
> nb_ls_bitmap of lb1 for sw0 and sw1 and nb_ls_bitmap of lb2 for sw1.
>
> This second handler is added mainly so that the actual logical switch
> handler northd_nb_logical_switch_handler() has done modifying the
> 'od' struct.
>

Thanks for explaining the pre_od and post_od approach here! If we do want
to go with this approach I'd suggest adding comments also in the code so
that people can understand the code easily without having to look at the
commit history.
However, I am concerned with the this approach. Here are the reasons:

1. The I-P engine implementation didn't consider such usage that add
dependency between two nodes twice, although it didn't explicitly call this
out in the documentation. If this is considered valid and necessary use
case, we should call this out in the I-P engine documentation so that
people modifying I-P engine implementation is aware of this and won't break
it, and of course we should carefully evaluate the side-effects that can be
caused by this.

2. For the current use case I think it will lead to dangling pointer in
this _pre_od handling, because a LS may have been deleted from IDL, while
this function still tries to access od->nbs, e.g. in
init_lb_for_datapaths(). Because of this, we should handle LB data only
after handling LS changes.

3. From the explanation above I can see what is done by pre_od and post_od
but still don't understand why can't we do both steps after handling LS
changes. Could you explain why? And is it possible to move both steps to
post_od?

Please find some more comments below.

> Signed-off-by: Numan Siddique 
> ---
>  lib/lb.c |   5 +-
>  northd/en-lb-data.c  | 176 +++
>  northd/en-lb-data.h  |  17 
>  northd/en-lflow.c|   6 ++
>  northd/en-northd.c   |  40 +++--
>  northd/en-northd.h   |   3 +-
>  northd/inc-proc-northd.c |   5 +-
>  northd/northd.c  | 104 +++
>  northd/northd.h  |  15 ++--
>  tests/ovn-northd.at  |  56 +
>  10 files changed, 380 insertions(+), 47 deletions(-)
>
> diff --git a/lib/lb.c b/lib/lb.c
> index 6fd67e2218..e6c9dc2be2 100644
> --- a/lib/lb.c
> +++ b/lib/lb.c
> @@ -1088,7 +1088,10 @@ 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++) {
> -bitmap_set1(lb_dps->nb_ls_map, ods[i]->index);
> +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++;
> +}
>  }
>  }
>
> diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
> index 23f2cb1021..e0c4db1422 100644
> --- a/northd/en-lb-data.c
> +++ b/northd/en-lb-data.c
> @@ -39,6 +39,14 @@ static void lb_data_destroy(struct ed_type_lb_data *);
>  static void build_lbs(const struct nbrec_load_balancer_table *,
>  

Re: [ovs-dev] [PATCH ovn v6 05/16] northd: Handle load balancer changes for a logical switch.

[Han Zhou]

On Thu, Aug 31, 2023 at 7:14 PM Han Zhou  wrote:
>
>
>
> On Fri, Aug 18, 2023 at 1:58 AM  wrote:
> >
> > From: Numan Siddique 
> >
> > 'lb_data' engine node now also handles logical switch changes.
> > Its data maintains ls to lb related information. i.e if a
> > logical switch sw0 has lb1, lb2 and lb3 associated then
> > it stores this info in its data.  And when a new load balancer
> > lb4 is associated to it, it stores this information in its
> > tracked data so that 'northd' engine node can handle it
> > accordingly.  Tracked data will have information like:
> >   changed ls -> {sw0 : {associated_lbs: [lb4]}
> >
> > In the 'northd' engne node, an additional handler for 'lb_data'
> > is added after the 'nbrec_logical_switch' changes.  With this patch
> > we now have 2 handlers for 'lb_data' in 'northd' engine node.
> >
> > 
> > engine_add_input(&en_northd, &en_lb_data,
northd_lb_data_handler_pre_od);
> > engine_add_input(&en_northd, &en_nb_logical_switch,
> >  northd_nb_logical_switch_handler);
> > engine_add_input(&en_northd, &en_lb_data,
northd_lb_data_handler_post_od);
> > 
> >
> > The first handler 'northd_lb_data_handler_pre_od' is called before the
> > 'northd_nb_logical_switch_handler' handler and it just creates or
> > deletes the lb_datapaths hmap for the tracked lbs.
> >
> > The second handler 'northd_lb_data_handler_post_od' is called after
> > the 'northd_nb_logical_switch_handler' handler and it updates the
> > ovn_lb_datapaths's 'nb_ls_map' bitmap.
> >
> > Eg.  If the lb_data has the below tracked data:
> >
> > tracked_data = {'crupdated_lbs': [lb1, lb2],
> > 'deleted_lbs': [lb3],
> > 'crupdated_lb_groups': [lbg1, lbg2],
> > 'crupdated_ls_lbs': [{ls: sw0, assoc_lbs: [lb1],
> >  {ls: sw1, assoc_lbs: [lb1, lb2]}
> >
> > then the first handler northd_lb_data_handler_pre_od(), creates the
> > ovn_lb_datapaths object for lb1 and lb2 and deletes lb3 from
> > the ovn_lb_datapaths hmap.  Similarly for the created or updated lb
> > groups lbg1 and lbg2.
> >
> > The second handler northd_lb_data_handler_post_od() updates the
> > nb_ls_bitmap of lb1 for sw0 and sw1 and nb_ls_bitmap of lb2 for sw1.
> >
> > This second handler is added mainly so that the actual logical switch
> > handler northd_nb_logical_switch_handler() has done modifying the
> > 'od' struct.
> >
>
> Thanks for explaining the pre_od and post_od approach here! If we do want
to go with this approach I'd suggest adding comments also in the code so
that people can understand the code easily without having to look at the
commit history.
> However, I am concerned with the this approach. Here are the reasons:
>
> 1. The I-P engine implementation didn't consider such usage that add
dependency between two nodes twice, although it didn't explicitly call this
out in the documentation. If this is considered valid and necessary use
case, we should call this out in the I-P engine documentation so that
people modifying I-P engine implementation is aware of this and won't break
it, and of course we should carefully evaluate the side-effects that can be
caused by this.
>
> 2. For the current use case I think it will lead to dangling pointer in
this _pre_od handling, because a LS may have been deleted from IDL, while
this function still tries to access od->nbs, e.g. in
init_lb_for_datapaths(). Because of this, we should handle LB data only
after handling LS changes.
>
> 3. From the explanation above I can see what is done by pre_od and
post_od but still don't understand why can't we do both steps after
handling LS changes. Could you explain why? And is it possible to move both
steps to post_od?
>
> Please find some more comments below.
>
> > Signed-off-by: Numan Siddique 
> > ---
> >  lib/lb.c |   5 +-
> >  northd/en-lb-data.c  | 176 +++
> >  northd/en-lb-data.h  |  17 
> >  northd/en-lflow.c|   6 ++
> >  northd/en-northd.c   |  40 +++--
> >  northd/en-northd.h   |   3 +-
> >  northd/inc-proc-northd.c |   5 +-
> >  northd/northd.c  | 104 +++
> >  northd/northd.h  |  15 ++--
> >  tests/ovn-northd.at  |  56 +
> >  10 files changed, 380 insertions(+), 47 deletions(-)
> >
> > diff --git a/lib/lb.c b/lib/lb.c
> > index 6fd67e2218..e6c9dc2be2 100644
> > --- a/lib/lb.c
> > +++ b/lib/lb.c
> > @@ -1088,7 +1088,10 @@ 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++) {
> > -bitmap_set1(lb_dps->nb_ls_map, ods[i]->index);
> > +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++;
> > +}
> >  }
> >  }
> >
> > diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
> > index 23f2cb1

[ovs-dev] [PATCH v7] ofproto-dpif-xlate: Don't reinstall removed XC_LEARN rule

[Mike Pattrick]

When the a revalidator thread is updating statistics for an XC_LEARN
xcache entry in xlate_push_stats_entry it uses ofproto_flow_mod_learn.
The revalidator will update stats for rules even if they are in a
removed state or marked as invisible. However, ofproto_flow_mod_learn
will detect if a flow has been removed and re-add it in that case. This
can result in an old learn action replacing the new learn action that
had replaced it in the first place.

This change adds a new last_used parameter to ofproto_flow_mod_learn
allowing the caller to provide a timestamp that will be fed into the
learned rule's modified time. The provided timestamp should be the time
of the last packet activity. If last_used is not set then the current
time is used, as is the current behaviour.

This change also adds a check when replacing a learned rule to favour the
newest rule.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2213892
Signed-off-by: Mike Pattrick 
---
 v2: Added additional checks if rule is removed
 v3: v2 patch was corrupted in transit
 v4: Added check against dpif flow stats
 v5: Fixed typos, updated commit message
 Changed timestamps to use datapath timestamp more consistently
 v6: Added a unit test, changed some variable names and comments,
 zero is no longer an acceptable value
 v7: Simplified unit test, updated comments.

---
 ofproto/ofproto-dpif-xlate-cache.c |   2 +-
 ofproto/ofproto-dpif-xlate.c   |  10 ++-
 ofproto/ofproto-dpif.c |   2 +-
 ofproto/ofproto-provider.h |   6 +-
 ofproto/ofproto.c  |  60 +++--
 tests/learn.at | 132 +
 6 files changed, 198 insertions(+), 14 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate-cache.c 
b/ofproto/ofproto-dpif-xlate-cache.c
index 9224ee2e6..2e1fcb3a6 100644
--- a/ofproto/ofproto-dpif-xlate-cache.c
+++ b/ofproto/ofproto-dpif-xlate-cache.c
@@ -125,7 +125,7 @@ xlate_push_stats_entry(struct xc_entry *entry,
 case XC_LEARN: {
 enum ofperr error;
 error = ofproto_flow_mod_learn(entry->learn.ofm, true,
-   entry->learn.limit, NULL);
+   entry->learn.limit, NULL, stats->used);
 if (error) {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 VLOG_WARN_RL(&rl, "xcache LEARN action execution failed.");
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 47ea0f47e..d608a5f25 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5700,8 +5700,16 @@ xlate_learn_action(struct xlate_ctx *ctx, const struct 
ofpact_learn *learn)
 if (!error) {
 bool success = true;
 if (ctx->xin->allow_side_effects) {
+long long int last_used;
+
+if (ctx->xin->resubmit_stats) {
+last_used = ctx->xin->resubmit_stats->used;
+} else {
+last_used = time_msec();
+}
 error = ofproto_flow_mod_learn(ofm, ctx->xin->xcache != NULL,
-   learn->limit, &success);
+   learn->limit, &success,
+   last_used);
 } else if (learn->limit) {
 if (!ofm->temp_rule
 || ofm->temp_rule->state != RULE_INSERTED) {
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index e22ca757a..ba5706f6a 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4880,7 +4880,7 @@ packet_xlate(struct ofproto *ofproto_, struct 
ofproto_packet_out *opo)
 if (entry->type == XC_LEARN) {
 struct ofproto_flow_mod *ofm = entry->learn.ofm;

-error = ofproto_flow_mod_learn_refresh(ofm);
+error = ofproto_flow_mod_learn_refresh(ofm, time_msec());
 if (error) {
 goto error_out;
 }
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 143ded690..9f7b8b6e8 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -2027,9 +2027,11 @@ enum ofperr ofproto_flow_mod_init_for_learn(struct 
ofproto *,
 struct ofproto_flow_mod *)
 OVS_EXCLUDED(ofproto_mutex);
 enum ofperr ofproto_flow_mod_learn(struct ofproto_flow_mod *, bool keep_ref,
-   unsigned limit, bool *below_limit)
+   unsigned limit, bool *below_limit,
+   long long int last_used)
 OVS_EXCLUDED(ofproto_mutex);
-enum ofperr ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod *ofm);
+enum ofperr ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod *ofm,
+   long long int last_used);
 enum ofperr ofproto_flow_mod_learn_st

Re: [ovs-dev] [PATCH ovn] ovs: Bump submodule to branch-3.2

[Ales Musil]

On Thu, Aug 31, 2023 at 10:19 PM Mark Michelson  wrote:
>
> Thanks for this Ales.
>
> Acked-by: Mark Michelson 
>
> Which branches is this a candidate for? Main only?

Hi Mark,

thank you for the review. For now I would say main only and we can
discuss how far back it might be useful. We need custom backport
patches anyway for every branch except 23.06.

Thanks,
Ales

>
> On 8/31/23 03:27, Ales Musil wrote:
> > Bump submodule to branch-3.2 mainly for:
> > 759a29dc2d97 ("backtrace: Extend the backtrace functionality.")
> > 1d78a3f3164a ("netdev-dpdk: Disable net/tap Tx L4 checksum offloads.")
> >
> > Unfortunately we cannot use the tag because we need
> > the DPDK offload fix, which was merged later on.
> >
> > At the same time fix three issues caused by the bump.
> >
> > Remove our custom 'struct sctp_chunk_header' that was added
> > to packets.h as part of:
> > 501f665a5a4b ("conntrack: Extract l4 information for SCTP.")
> >
> > Adjust "daemonize_start" to accept two parameters
> > and set the second to false, as we don't need access
> > to DPDK devices directly at this moment. This was introduced
> > by commit:
> > 07cf5810de8d ("dpdk: Allow retaining CAP_SYS_RAWIO privileges.")
> >
> > Adjust the path for OvS python helpers, introduced by:
> > bb0dd1135ba9 ("python: Rename build related code to ovs_build_helpers.")
> >
> > Reported-at: https://bugzilla.redhat.com/2164058
> > Signed-off-by: Ales Musil 
> > ---
> >   Makefile.am   |  2 +-
> >   build-aux/sodepends.py|  2 +-
> >   build-aux/soexpand.py |  2 +-
> >   build-aux/xml2nroff   | 14 +++---
> >   controller-vtep/ovn-controller-vtep.c |  2 +-
> >   controller/ovn-controller.c   |  2 +-
> >   controller/pinctrl.c  | 10 +-
> >   ic/ovn-ic.c   |  2 +-
> >   lib/ovn-util.h|  7 ---
> >   northd/ovn-northd.c   |  2 +-
> >   ovs   |  2 +-
> >   utilities/ovn-dbctl.c |  2 +-
> >   utilities/ovn-trace.c |  2 +-
> >   13 files changed, 22 insertions(+), 29 deletions(-)
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 27182c7bc..b58d4a501 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -430,7 +430,7 @@ endif
> >   CLEANFILES += flake8-check
> >
> >   include $(srcdir)/manpages.mk
> > -$(srcdir)/manpages.mk: $(MAN_ROOTS) build-aux/sodepends.py 
> > $(OVS_SRCDIR)/python/build/soutil.py
> > +$(srcdir)/manpages.mk: $(MAN_ROOTS) build-aux/sodepends.py 
> > $(OVS_SRCDIR)/python/ovs_build_helpers/soutil.py
> >   
> > @PYTHONPATH=$(OVS_SRCDIR)/python$(psep)$$PYTHONPATH$(psep)$(srcdir)/python 
> > $(PYTHON3) $(srcdir)/build-aux/sodepends.py -I. -Isrcdir,$(srcdir) 
> > -IOVS_MANDIR,$(OVS_MANDIR) $(MAN_ROOTS) >$(@F).tmp
> >   @if cmp -s $(@F).tmp $@; then \
> > touch $@; \
> > diff --git a/build-aux/sodepends.py b/build-aux/sodepends.py
> > index 7b1f9c840..c04a5c681 100755
> > --- a/build-aux/sodepends.py
> > +++ b/build-aux/sodepends.py
> > @@ -14,7 +14,7 @@
> >   # See the License for the specific language governing permissions and
> >   # limitations under the License.
> >
> > -from build import soutil
> > +from ovs_build_helpers import soutil
> >   import sys
> >   import getopt
> >   import os
> > diff --git a/build-aux/soexpand.py b/build-aux/soexpand.py
> > index 00adcf47a..f9ab80963 100755
> > --- a/build-aux/soexpand.py
> > +++ b/build-aux/soexpand.py
> > @@ -14,7 +14,7 @@
> >   # See the License for the specific language governing permissions and
> >   # limitations under the License.
> >
> > -from build import soutil
> > +from ovs_build_helpers import soutil
> >   import sys
> >
> >
> > diff --git a/build-aux/xml2nroff b/build-aux/xml2nroff
> > index 9e781a396..dd9577204 100755
> > --- a/build-aux/xml2nroff
> > +++ b/build-aux/xml2nroff
> > @@ -18,7 +18,7 @@ import getopt
> >   import sys
> >   import xml.dom.minidom
> >
> > -import build.nroff
> > +from ovs_build_helpers import nroff
> >
> >   argv0 = sys.argv[0]
> >
> > @@ -94,12 +94,12 @@ def manpage_to_nroff(xml_file, subst, include_path, 
> > version=None):
> >   .  PP
> >   .  I "\\$1"
> >   ..
> > -''' % (build.nroff.text_to_nroff(program),
> > -   build.nroff.text_to_nroff(section),
> > -   build.nroff.text_to_nroff(title),
> > -   build.nroff.text_to_nroff(version))
> > +''' % (nroff.text_to_nroff(program),
> > +   nroff.text_to_nroff(section),
> > +   nroff.text_to_nroff(title),
> > +   nroff.text_to_nroff(version))
> >
> > -s += build.nroff.block_xml_to_nroff(doc.childNodes) + "\n"
> > +s += nroff.block_xml_to_nroff(doc.childNodes) + "\n"
> >
> >   return s
> >
> > @@ -145,7 +145,7 @@ if __name__ == "__main__":
> >
> >   try:
> >   s = manpage_to_nroff(args[0], subst, include_path, version)
> > -except build.nroff.error.Error as e:
> > +except nroff.error.Error as e:
> 

Re: [ovs-dev] [PATCH ovn v6 06/16] northd: Handle load balancer group changes for a logical switch.

[Han Zhou]

On Fri, Aug 18, 2023 at 1:58 AM  wrote:
>
> From: Numan Siddique 
>
> For every a given load balancer group 'A', northd engine data maintains
> a bitmap of datapaths associated to this lb group.  So when lb group 'A'
> gets associated to a logical switch 's1', the bitmap index of 's1' is set
> in its bitmap.
>
> In order to handle the load balancer group changes incrementally for a
> logical switch, we need to set and clear the bitmap bits accordingly.
> And this patch does it.
>
> Signed-off-by: Numan Siddique 

Thanks Numan. Some of my comments/questions to the previous patch also
apply to this patch because the logic is similar. Otherwise looks good to
me.

Regards,
Han

> ---
>  northd/en-lb-data.c | 102 
>  northd/en-lb-data.h |   4 ++
>  northd/en-northd.c  |   9 +++-
>  northd/northd.c |  70 --
>  northd/northd.h |   5 ++-
>  tests/ovn-northd.at |  30 +
>  6 files changed, 169 insertions(+), 51 deletions(-)
>
> diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
> index e0c4db1422..8619b4cc14 100644
> --- a/northd/en-lb-data.c
> +++ b/northd/en-lb-data.c
> @@ -63,6 +63,7 @@ static struct crupdated_lb_group *
>  static void add_deleted_lb_group_to_tracked_data(
>  struct ovn_lb_group *, struct tracked_lb_data *);
>  static bool is_ls_lbs_changed(const struct nbrec_logical_switch *nbs);
> +static bool is_ls_lbgrps_changed(const struct nbrec_logical_switch *nbs);
>
>  /* 'lb_data' engine node manages the NB load balancers and load balancer
>   * groups.  For each NB LB, it creates 'struct ovn_northd_lb' and
> @@ -271,12 +272,15 @@ lb_data_logical_switch_handler(struct engine_node
*node, void *data)
>  destroy_od_lb_data(od_lb_data);
>  }
>  } else {
> -if (!is_ls_lbs_changed(nbs)) {
> +bool ls_lbs_changed = is_ls_lbs_changed(nbs);
> +bool ls_lbgrps_changed = is_ls_lbgrps_changed(nbs);
> +if (!ls_lbs_changed && !ls_lbgrps_changed) {
>  continue;
>  }
>  struct crupdated_od_lb_data *codlb = xzalloc(sizeof *codlb);
>  codlb->od_uuid = nbs->header_.uuid;
>  uuidset_init(&codlb->assoc_lbs);
> +uuidset_init(&codlb->assoc_lbgrps);
>
>  struct od_lb_data *od_lb_data =
>  find_od_lb_data(&lb_data->ls_lb_map, &nbs->header_.uuid);
> @@ -285,38 +289,66 @@ lb_data_logical_switch_handler(struct engine_node
*node, void *data)
>  &nbs->header_.uuid);
>  }
>
> -struct uuidset *pre_lb_uuids = od_lb_data->lbs;
> -od_lb_data->lbs = xzalloc(sizeof *od_lb_data->lbs);
> -uuidset_init(od_lb_data->lbs);
> -
> -for (size_t i = 0; i < nbs->n_load_balancer; i++) {
> -const struct uuid *lb_uuid =
> -&nbs->load_balancer[i]->header_.uuid;
> -uuidset_insert(od_lb_data->lbs, lb_uuid);
> +if (ls_lbs_changed) {
> +struct uuidset *pre_lb_uuids = od_lb_data->lbs;
> +od_lb_data->lbs = xzalloc(sizeof *od_lb_data->lbs);
> +uuidset_init(od_lb_data->lbs);
> +
> +for (size_t i = 0; i < nbs->n_load_balancer; i++) {
> +const struct uuid *lb_uuid =
> +&nbs->load_balancer[i]->header_.uuid;
> +uuidset_insert(od_lb_data->lbs, lb_uuid);
> +
> +struct uuidset_node *unode =
uuidset_find(pre_lb_uuids,
> +lb_uuid);
> +
> +if (!unode || (nbrec_load_balancer_row_get_seqno(
> +nbs->load_balancer[i],
> +OVSDB_IDL_CHANGE_MODIFY) > 0)) {
> +/* Add this lb to the tracked data. */
> +uuidset_insert(&codlb->assoc_lbs, lb_uuid);
> +changed = true;
> +}
> +
> +if (unode) {
> +uuidset_delete(pre_lb_uuids, unode);
> +}
> +}
> +if (!uuidset_is_empty(pre_lb_uuids)) {
> +trk_lb_data->has_dissassoc_lbs_from_od = true;
> +changed = true;
> +}
>
> -struct uuidset_node *unode = uuidset_find(pre_lb_uuids,
> -  lb_uuid);
> +uuidset_destroy(pre_lb_uuids);
> +free(pre_lb_uuids);
> +}
>
> -if (!unode || (nbrec_load_balancer_row_get_seqno(
> -nbs->load_balancer[i], OVSDB_IDL_CHANGE_MODIFY)
> 0)) {
> -/* Add this lb to the tracked data. */
> -uuidset_insert(&codlb->assoc_lbs, lb_uuid);
> -changed = tru