Re: [ovs-dev] [RFC ovn 0/5] Facilitate external use of ovn-detrace
On 10/18/21 18:50, Timothy Redaelli wrote: On Mon, 18 Oct 2021 18:18:07 +0200 Adrian Moreno wrote: On 10/18/21 14:51, Dumitru Ceara wrote: On 10/14/21 6:41 PM, Adrian Moreno wrote: ovn-detrace is a very useful tool for debugging OVN issues. It's core logic (mapping openflow cookies / ports with OVN objects) can be used for a variety of troubleshooting tools. Therefore, it would be desirable to make use of such logic from an external python program. This could be done by creating a python library (similarly to what ovs provides) that is built and pushed to PyPi for other projects to consume. However, being the only python script that lives in OVN, this might be a bit of an overkill, so what this series proposes is an intermediate step that does not require that much extra maintenance and still alleviates the main obstacles one finds when trying to use ovn-detrace as a python module which are: - python expects module names to end in .py and use underscores instead of hyphens - internally, ovn-detrace prints directy to stdout, the output of the ovn-detrace information should be configurable - the version information is not easily available With this series, ovn-detrace is renamed to ovn_detrace.py and a symlink with the old name is created for backwards compatibility. As a result, a use can point her PYTHONPATH to ovn's installation path, run "import ovn_detrace", and make use of ovn-detrace's logic with, say, individual openflow cookies instead of ofproto/trace outputs. I know it's not the cleanest way to do it. I'd love to hear your opinion on the matter. Reviewing notes: - The first patch is a small fix I spotted when playing around with ovn-detrace - I have not tested the debian package thoroughly Hi Adrian, I briefly tried the RFC series out and I don't see any functional changes, so that works for me. I quickly glanced at the patches too and they also look OK to me. Looking forward to the v1. Regards, Dumitru Thanks Dumitru, If the general approach seems OK, I'll send v1. Timothy, do you foresee any issues on the packaging side? Hi, I sent you a couple of comments inline, but for packaging point of view I don't see any problem. Just copy the modifications you did in ovn-fedora.spec.in (symlink + adding the .py in %files) in our downstream spec file. Thanks Timothy. Do you know who can provide comments on the debian side? -- Adrián Moreno ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC ovn 3/5] ovn-detrace: rename ovn-detrace to ovn_detrace.py
On 10/18/21 18:48, Timothy Redaelli wrote:
On Thu, 14 Oct 2021 18:41:28 +0200
Adrian Moreno wrote:
For external python programs to be able to load ovn-detrace as a python
module easily, it should end in .py and should have underscores.
Move ovn-detrace to ovn-detrac.py and create a symlink with the old name
for backwards compatibility.
Signed-off-by: Adrian Moreno
---
debian/ovn-common.install | 2 +-
debian/ovn-common.postinst | 1 +
debian/ovn-common.postrm| 1 +
rhel/ovn-fedora.spec.in | 9 +
utilities/automake.mk | 14 --
utilities/{ovn-detrace.in => ovn_detrace.py.in} | 0
6 files changed, 24 insertions(+), 3 deletions(-)
rename utilities/{ovn-detrace.in => ovn_detrace.py.in} (100%)
diff --git a/debian/ovn-common.install b/debian/ovn-common.install
index 8e5915724..050d1c63a 100644
--- a/debian/ovn-common.install
+++ b/debian/ovn-common.install
@@ -4,7 +4,7 @@ usr/bin/ovn-sbctl
usr/bin/ovn-ic-nbctl
usr/bin/ovn-ic-sbctl
usr/bin/ovn-trace
-usr/bin/ovn-detrace
+usr/bin/ovn_detrace.py
usr/share/ovn/scripts/ovn-ctl
usr/share/ovn/scripts/ovndb-servers.ocf
usr/share/ovn/scripts/ovn-lib
diff --git a/debian/ovn-common.postinst b/debian/ovn-common.postinst
index 15f3c7577..dfddb1f08 100644
--- a/debian/ovn-common.postinst
+++ b/debian/ovn-common.postinst
@@ -9,6 +9,7 @@ case "$1" in
configure)
mkdir -p /usr/lib/ocf/resource.d/ovn
ln -sf /usr/share/ovn/scripts/ovndb-servers.ocf
/usr/lib/ocf/resource.d/ovn/ovndb-servers
+ln -sf /usr/bin/ovn_detrace.py /usr/bin/ovn-detrace
;;
abort-upgrade|abort-remove|abort-deconfigure)
;;
diff --git a/debian/ovn-common.postrm b/debian/ovn-common.postrm
index 9face726b..d607a66d5 100644
--- a/debian/ovn-common.postrm
+++ b/debian/ovn-common.postrm
@@ -8,6 +8,7 @@ set -e
case "$1" in
purge|remove)
rm -rf /usr/lib/ocf/resource.d/ovn
+rm -f /usr/bin/ovn-detrace
;;
upgrade|failed-upgrade|abort-install|abort-upgrade|disappear)
;;
diff --git a/rhel/ovn-fedora.spec.in b/rhel/ovn-fedora.spec.in
index 9c8647b5a..fc0992263 100644
--- a/rhel/ovn-fedora.spec.in
+++ b/rhel/ovn-fedora.spec.in
@@ -264,6 +264,12 @@ if [ $1 -eq 1 ] ; then
fi
fi
+%preun
+if [ $1 -eq 0 ] ; then
+# Package removal, not upgrade
+rm %{_bindir}/ovn-detrace
+fi
+
This is not necessary, since the symlink is installed in %files,
it's removed during uninstall by rpm without using %preun
Oh, right! Thanks.
%preun central
%if 0%{?systemd_preun:1}
%systemd_preun ovn-northd.service
@@ -318,6 +324,8 @@ fi
%endif
%post
+ln -sf ovn_detrace.py %{_bindir}/ovn-detrace
+
%if %{with libcapng}
if [ $1 -eq 1 ]; then
sed -i 's:^#OVN_USER_ID=:OVN_USER_ID=:' %{_sysconfdir}/sysconfig/ovn
@@ -462,6 +470,7 @@ fi
%{_bindir}/ovn-nbctl
%{_bindir}/ovn-sbctl
%{_bindir}/ovn-trace
+%{_bindir}/ovn_detrace.py
%{_bindir}/ovn-detrace
%{_bindir}/ovn-appctl
%{_bindir}/ovn-ic-nbctl
diff --git a/utilities/automake.mk b/utilities/automake.mk
index a03892f20..67b04cbff 100644
--- a/utilities/automake.mk
+++ b/utilities/automake.mk
@@ -20,7 +20,7 @@ MAN_ROOTS += \
bin_SCRIPTS += \
utilities/ovn-docker-overlay-driver \
utilities/ovn-docker-underlay-driver \
-utilities/ovn-detrace
+utilities/ovn_detrace.py
EXTRA_DIST += \
utilities/ovn-ctl \
@@ -34,7 +34,7 @@ EXTRA_DIST += \
utilities/ovn-ic-sbctl.8.xml \
utilities/ovn-appctl.8.xml \
utilities/ovn-trace.8.xml \
-utilities/ovn-detrace.in \
+utilities/ovn_detrace.py.in \
utilities/ovndb-servers.ocf \
utilities/checkpatch.py \
utilities/docker/Makefile \
@@ -60,6 +60,7 @@ CLEANFILES += \
utilities/ovn-trace.8 \
utilities/ovn-detrace.1 \
utilities/ovn-detrace \
+utilities/ovn_detrace.py \
utilities/ovn-appctl.8 \
utilities/ovn-appctl \
utilities/ovn-sim
@@ -105,4 +106,13 @@ bin_PROGRAMS += utilities/ovn-appctl
utilities_ovn_appctl_SOURCES = utilities/ovn-appctl.c
utilities_ovn_appctl_LDADD = lib/libovn.la $(OVSDB_LIBDIR)/libovsdb.la
$(OVS_LIBDIR)/libopenvswitch.la
+# ovn-detrace
+INSTALL_DATA_LOCAL += ovn-detrace-install
+ovn-detrace-install:
+ ln -sf ovn_detrace.py $(DESTDIR)$(bindir)/ovn-detrace
+
+UNINSTALL_LOCAL += ovn-detrace-uninstall
+ovn-detrace-uninstall:
+ rm -f $(DESTDIR)$(bindir)/ovn-detrace
+
include utilities/bugtool/automake.mk
diff --git a/utilities/ovn-detrace.in b/utilities/ovn_detrace.py.in
similarity index 100%
rename from utilities/ovn-detrace.in
rename to utilities/ovn_detrace.py.in
--
Adrián Moreno
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v16 3/8] dpif-offload-provider: Introduce dpif-offload-provider layer
On 10/18/2021 11:14 PM, Eelco Chaudron wrote:
On 18 Oct 2021, at 14:03, Chris Mi wrote:
Hi Eelco,
On 10/15/2021 9:42 PM, Eelco Chaudron wrote:
Small comments inline, and Ilya please take a look at the first comment/request.
//Eelco
On 12 Oct 2021, at 10:19, Chris Mi wrote:
Some offload actions require functionality that is not netdev
based, but dpif. For example, sFlow action requires to create
a psample netlink socket to receive the sampled packets from
TC or kernel driver.
Create dpif-offload-provider layer to support such actions.
Signed-off-by: Chris Mi
Reviewed-by: Eli Britstein
---
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 7e11b9697..ce20cdeb1 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -22,8 +22,9 @@
* exposed over OpenFlow as a single switch. Datapaths and the collections
of
* ports that they contain may be fixed or dynamic. */
-#include "openflow/openflow.h"
#include "dpif.h"
+#include "dpif-offload-provider.h"
+#include "openflow/openflow.h"
#include "util.h"
#ifdef __cplusplus
@@ -635,6 +636,11 @@ struct dpif_class {
* sufficient to store BOND_BUCKETS number of elements. */
int (*bond_stats_get)(struct dpif *dpif, uint32_t bond_id,
uint64_t *n_bytes);
+
+/* Some offload actions require functionality that is not netdev based,
+ * but dpif. Add dpif-offload-provider layer API to support such
+ * offload actions. */
+const struct dpif_offload_api *offload_api;
From previous revisions:
| EC> Here you add the provider directly into the dpif class. Not sure if this
is what Ilya had in mind. As in general, these get integrated into the
dpif/netdev, not the class. Ilya can you comment on/review this?
| CM> OK.
From my side, this looks wrong as there is a direct relation between dpif and
dpif-offload. I would assume you should be able to pick a specific one, or what
else would have stopped us from adding the
dpif_offload_sflow_recv_wait()/dpif_offload_sflow_recv() directly in the dpif.
I'll think about it. If I understand correctly, it should be something like
this, right?
struct dpif {
const struct dpif_class *dpif_class;
const struct dpif_offload_api *dpif_offload_api;
...
Yes, this is what I was referring to.
OK, got it.
Thanks,
Chris
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] netdev-linux: Ingress policing to use matchall if basic is not available.
Currently ingress policing uses the basic classifier to apply traffic
control filters if hardware offload is not enabled, else it uses
matchall. This change enables fallback onto the matchall classifier for
cases when the kernel is not built with basic support and hardware
offload is not in use. Basic is still selected first.
The system tests are modified to allow either basic or matchall
classification on the ingestion filter, and to allow either 1 or
10240 packets for the packet burst filter. 1 is accurate for kernel
5.14 and the most recent iproute2, however, 10240 is left for
compatibility with older kernels.
Signed-off-by: Mike Pattrick
---
lib/netdev-linux.c | 21 ++---
tests/system-offloads-traffic.at | 20 +---
2 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 97bd21be4..cb7ce1d2f 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -2776,8 +2776,7 @@ netdev_linux_set_policing(struct netdev *netdev_,
uint32_t kbits_rate,
error = tc_add_matchall_policer(netdev_, kbits_rate, kbits_burst,
kpkts_rate, kpkts_burst);
}
-ovs_mutex_unlock(>mutex);
-return error;
+goto out;
}
error = get_ifindex(netdev_, );
@@ -2803,6 +2802,12 @@ netdev_linux_set_policing(struct netdev *netdev_,
uint32_t kbits_rate,
error = tc_add_policer(netdev_, kbits_rate, kbits_burst,
kpkts_rate, kpkts_burst);
+if (error == ENOENT) {
+/* This error is returned when the basic classifier is missing.
+ * Fall back to the matchall classifier. */
+error = tc_add_matchall_policer(netdev_, kbits_rate, kbits_burst,
+kpkts_rate, kpkts_burst);
+}
if (error){
VLOG_WARN_RL(, "%s: adding policing action failed: %s",
netdev_name, ovs_strerror(error));
@@ -2810,12 +2815,14 @@ netdev_linux_set_policing(struct netdev *netdev_,
uint32_t kbits_rate,
}
}
-netdev->kbits_rate = kbits_rate;
-netdev->kbits_burst = kbits_burst;
-netdev->kpkts_rate = kpkts_rate;
-netdev->kpkts_burst = kpkts_burst;
-
out:
+if (!error) {
+netdev->kbits_rate = kbits_rate;
+netdev->kbits_burst = kbits_burst;
+netdev->kpkts_rate = kpkts_rate;
+netdev->kpkts_burst = kpkts_burst;
+}
+
if (!error || error == ENODEV) {
netdev->netdev_policing_error = error;
netdev->cache_valid |= VALID_POLICING;
diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
index be63057bb..80bc1dd5c 100644
--- a/tests/system-offloads-traffic.at
+++ b/tests/system-offloads-traffic.at
@@ -89,10 +89,8 @@ AT_CHECK([tc -o -s -d filter show dev ovs-p0 ingress |
[0],[dnl
rate 100Kbit burst 1280b
])
-AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | grep basic |
- sed -n 's/.*\(basic\).*/\1/; T; p; q'], [0], [dnl
-basic
-])
+AT_CHECK([tc -s -d filter show dev ovs-p0 ingress |
+ egrep "basic|matchall" > /dev/null], [0])
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
@@ -135,14 +133,13 @@ AT_CHECK([ovs-vsctl --columns=other_config list open],
[0], [dnl
other_config: {hw-offload="false"}
])
AT_CHECK([tc -o -s -d filter show dev ovs-p0 ingress |
- sed -n 's/.*\(pkts_rate [[0-9]]*[[a-zA-Z]]* pkts_burst
[[0-9]]*[[a-zA-Z]]*\).*/\1/; T; p; q'],
+ sed -n 's/.*\(pkts_rate [[0-9]]*[[a-zA-Z]]* pkts_burst
[[0-9]]*[[a-zA-Z]]*\).*/\1/; T; p; q' |
+ sed -e 's/10240/1/'],
[0],[dnl
-pkts_rate 10 pkts_burst 10240
+pkts_rate 10 pkts_burst 1
])
AT_CHECK([tc -s -d filter show dev ovs-p0 ingress |
- sed -n 's/.*\(basic\).*/\1/; T; p; q'], [0], [dnl
-basic
-])
+ egrep "basic|matchall" > /dev/null], [0])
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
@@ -160,9 +157,10 @@ AT_CHECK([ovs-vsctl --columns=other_config list open],
[0], [dnl
other_config: {hw-offload="true"}
])
AT_CHECK([tc -o -s -d filter show dev ovs-p0 ingress |
- sed -n 's/.*\(pkts_rate [[0-9]]*[[a-zA-Z]]* pkts_burst
[[0-9]]*[[a-zA-Z]]*\).*/\1/; T; p; q'],
+ sed -n 's/.*\(pkts_rate [[0-9]]*[[a-zA-Z]]* pkts_burst
[[0-9]]*[[a-zA-Z]]*\).*/\1/; T; p; q' |
+ sed -e 's/10240/1/'],
[0],[dnl
-pkts_rate 10 pkts_burst 10240
+pkts_rate 10 pkts_burst 1
])
AT_CHECK([tc -s -d filter show dev ovs-p0 ingress |
sed -n 's/.*\(matchall\).*/\1/; T; p; q'], [0], [dnl
--
2.30.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC ovn 0/5] Facilitate external use of ovn-detrace
On Mon, 18 Oct 2021 18:18:07 +0200 Adrian Moreno wrote: > > > On 10/18/21 14:51, Dumitru Ceara wrote: > > On 10/14/21 6:41 PM, Adrian Moreno wrote: > >> ovn-detrace is a very useful tool for debugging OVN issues. > >> > >> It's core logic (mapping openflow cookies / ports with OVN objects) can > >> be used for a variety of troubleshooting tools. Therefore, it would be > >> desirable to make use of such logic from an external python program. > >> > >> This could be done by creating a python library (similarly to what ovs > >> provides) that is built and pushed to PyPi for other projects to > >> consume. > >> > >> However, being the only python script that lives in OVN, this might be a > >> bit of an overkill, so what this series proposes is an intermediate step > >> that does not require that much extra maintenance and still alleviates > >> the main obstacles one finds when trying to use ovn-detrace as a python > >> module which are: > >> - python expects module names to end in .py and use underscores instead > >>of hyphens > >> - internally, ovn-detrace prints directy to stdout, the output of the > >>ovn-detrace information should be configurable > >> - the version information is not easily available > >> > >> With this series, ovn-detrace is renamed to ovn_detrace.py and a > >> symlink with the old name is created for backwards compatibility. As a > >> result, a use can point her PYTHONPATH to ovn's installation path, > >> run "import ovn_detrace", and make use of ovn-detrace's logic with, say, > >> individual openflow cookies instead of ofproto/trace outputs. > >> > >> I know it's not the cleanest way to do it. I'd love to hear your opinion > >> on the matter. > >> > >> Reviewing notes: > >> - The first patch is a small fix I spotted when playing around with > >> ovn-detrace > >> - I have not tested the debian package thoroughly > >> > > > > Hi Adrian, > > > > I briefly tried the RFC series out and I don't see any functional > > changes, so that works for me. > > > > I quickly glanced at the patches too and they also look OK to me. > > > > Looking forward to the v1. > > > > Regards, > > Dumitru > > > > Thanks Dumitru, > > If the general approach seems OK, I'll send v1. > > Timothy, do you foresee any issues on the packaging side? Hi, I sent you a couple of comments inline, but for packaging point of view I don't see any problem. Just copy the modifications you did in ovn-fedora.spec.in (symlink + adding the .py in %files) in our downstream spec file. > Thanks ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC ovn 3/5] ovn-detrace: rename ovn-detrace to ovn_detrace.py
On Thu, 14 Oct 2021 18:41:28 +0200
Adrian Moreno wrote:
> For external python programs to be able to load ovn-detrace as a python
> module easily, it should end in .py and should have underscores.
>
> Move ovn-detrace to ovn-detrac.py and create a symlink with the old name
> for backwards compatibility.
>
> Signed-off-by: Adrian Moreno
> ---
> debian/ovn-common.install | 2 +-
> debian/ovn-common.postinst | 1 +
> debian/ovn-common.postrm| 1 +
> rhel/ovn-fedora.spec.in | 9 +
> utilities/automake.mk | 14 --
> utilities/{ovn-detrace.in => ovn_detrace.py.in} | 0
> 6 files changed, 24 insertions(+), 3 deletions(-)
> rename utilities/{ovn-detrace.in => ovn_detrace.py.in} (100%)
>
> diff --git a/debian/ovn-common.install b/debian/ovn-common.install
> index 8e5915724..050d1c63a 100644
> --- a/debian/ovn-common.install
> +++ b/debian/ovn-common.install
> @@ -4,7 +4,7 @@ usr/bin/ovn-sbctl
> usr/bin/ovn-ic-nbctl
> usr/bin/ovn-ic-sbctl
> usr/bin/ovn-trace
> -usr/bin/ovn-detrace
> +usr/bin/ovn_detrace.py
> usr/share/ovn/scripts/ovn-ctl
> usr/share/ovn/scripts/ovndb-servers.ocf
> usr/share/ovn/scripts/ovn-lib
> diff --git a/debian/ovn-common.postinst b/debian/ovn-common.postinst
> index 15f3c7577..dfddb1f08 100644
> --- a/debian/ovn-common.postinst
> +++ b/debian/ovn-common.postinst
> @@ -9,6 +9,7 @@ case "$1" in
> configure)
> mkdir -p /usr/lib/ocf/resource.d/ovn
> ln -sf /usr/share/ovn/scripts/ovndb-servers.ocf
> /usr/lib/ocf/resource.d/ovn/ovndb-servers
> +ln -sf /usr/bin/ovn_detrace.py /usr/bin/ovn-detrace
> ;;
> abort-upgrade|abort-remove|abort-deconfigure)
> ;;
> diff --git a/debian/ovn-common.postrm b/debian/ovn-common.postrm
> index 9face726b..d607a66d5 100644
> --- a/debian/ovn-common.postrm
> +++ b/debian/ovn-common.postrm
> @@ -8,6 +8,7 @@ set -e
> case "$1" in
> purge|remove)
> rm -rf /usr/lib/ocf/resource.d/ovn
> +rm -f /usr/bin/ovn-detrace
> ;;
> upgrade|failed-upgrade|abort-install|abort-upgrade|disappear)
> ;;
> diff --git a/rhel/ovn-fedora.spec.in b/rhel/ovn-fedora.spec.in
> index 9c8647b5a..fc0992263 100644
> --- a/rhel/ovn-fedora.spec.in
> +++ b/rhel/ovn-fedora.spec.in
> @@ -264,6 +264,12 @@ if [ $1 -eq 1 ] ; then
> fi
> fi
>
> +%preun
> +if [ $1 -eq 0 ] ; then
> +# Package removal, not upgrade
> +rm %{_bindir}/ovn-detrace
> +fi
> +
This is not necessary, since the symlink is installed in %files,
it's removed during uninstall by rpm without using %preun
> %preun central
> %if 0%{?systemd_preun:1}
> %systemd_preun ovn-northd.service
> @@ -318,6 +324,8 @@ fi
> %endif
>
> %post
> +ln -sf ovn_detrace.py %{_bindir}/ovn-detrace
> +
> %if %{with libcapng}
> if [ $1 -eq 1 ]; then
> sed -i 's:^#OVN_USER_ID=:OVN_USER_ID=:' %{_sysconfdir}/sysconfig/ovn
> @@ -462,6 +470,7 @@ fi
> %{_bindir}/ovn-nbctl
> %{_bindir}/ovn-sbctl
> %{_bindir}/ovn-trace
> +%{_bindir}/ovn_detrace.py
> %{_bindir}/ovn-detrace
> %{_bindir}/ovn-appctl
> %{_bindir}/ovn-ic-nbctl
> diff --git a/utilities/automake.mk b/utilities/automake.mk
> index a03892f20..67b04cbff 100644
> --- a/utilities/automake.mk
> +++ b/utilities/automake.mk
> @@ -20,7 +20,7 @@ MAN_ROOTS += \
> bin_SCRIPTS += \
> utilities/ovn-docker-overlay-driver \
> utilities/ovn-docker-underlay-driver \
> -utilities/ovn-detrace
> +utilities/ovn_detrace.py
>
> EXTRA_DIST += \
> utilities/ovn-ctl \
> @@ -34,7 +34,7 @@ EXTRA_DIST += \
> utilities/ovn-ic-sbctl.8.xml \
> utilities/ovn-appctl.8.xml \
> utilities/ovn-trace.8.xml \
> -utilities/ovn-detrace.in \
> +utilities/ovn_detrace.py.in \
> utilities/ovndb-servers.ocf \
> utilities/checkpatch.py \
> utilities/docker/Makefile \
> @@ -60,6 +60,7 @@ CLEANFILES += \
> utilities/ovn-trace.8 \
> utilities/ovn-detrace.1 \
> utilities/ovn-detrace \
> +utilities/ovn_detrace.py \
> utilities/ovn-appctl.8 \
> utilities/ovn-appctl \
> utilities/ovn-sim
> @@ -105,4 +106,13 @@ bin_PROGRAMS += utilities/ovn-appctl
> utilities_ovn_appctl_SOURCES = utilities/ovn-appctl.c
> utilities_ovn_appctl_LDADD = lib/libovn.la $(OVSDB_LIBDIR)/libovsdb.la
> $(OVS_LIBDIR)/libopenvswitch.la
>
> +# ovn-detrace
> +INSTALL_DATA_LOCAL += ovn-detrace-install
> +ovn-detrace-install:
> + ln -sf ovn_detrace.py $(DESTDIR)$(bindir)/ovn-detrace
> +
> +UNINSTALL_LOCAL += ovn-detrace-uninstall
> +ovn-detrace-uninstall:
> + rm -f $(DESTDIR)$(bindir)/ovn-detrace
> +
> include utilities/bugtool/automake.mk
> diff --git a/utilities/ovn-detrace.in b/utilities/ovn_detrace.py.in
> similarity index 100%
> rename from utilities/ovn-detrace.in
> rename to utilities/ovn_detrace.py.in
___
dev mailing list
d...@openvswitch.org
Re: [ovs-dev] [RFC ovn 3/5] ovn-detrace: rename ovn-detrace to ovn_detrace.py
On Thu, 14 Oct 2021 18:41:28 +0200
Adrian Moreno wrote:
> For external python programs to be able to load ovn-detrace as a python
> module easily, it should end in .py and should have underscores.
>
> Move ovn-detrace to ovn-detrac.py and create a symlink with the old name
Nit: multiple typo ^ ^
> for backwards compatibility.
>
> Signed-off-by: Adrian Moreno
> ---
> debian/ovn-common.install | 2 +-
> debian/ovn-common.postinst | 1 +
> debian/ovn-common.postrm| 1 +
> rhel/ovn-fedora.spec.in | 9 +
> utilities/automake.mk | 14 --
> utilities/{ovn-detrace.in => ovn_detrace.py.in} | 0
> 6 files changed, 24 insertions(+), 3 deletions(-)
> rename utilities/{ovn-detrace.in => ovn_detrace.py.in} (100%)
>
> diff --git a/debian/ovn-common.install b/debian/ovn-common.install
> index 8e5915724..050d1c63a 100644
> --- a/debian/ovn-common.install
> +++ b/debian/ovn-common.install
> @@ -4,7 +4,7 @@ usr/bin/ovn-sbctl
> usr/bin/ovn-ic-nbctl
> usr/bin/ovn-ic-sbctl
> usr/bin/ovn-trace
> -usr/bin/ovn-detrace
> +usr/bin/ovn_detrace.py
> usr/share/ovn/scripts/ovn-ctl
> usr/share/ovn/scripts/ovndb-servers.ocf
> usr/share/ovn/scripts/ovn-lib
> diff --git a/debian/ovn-common.postinst b/debian/ovn-common.postinst
> index 15f3c7577..dfddb1f08 100644
> --- a/debian/ovn-common.postinst
> +++ b/debian/ovn-common.postinst
> @@ -9,6 +9,7 @@ case "$1" in
> configure)
> mkdir -p /usr/lib/ocf/resource.d/ovn
> ln -sf /usr/share/ovn/scripts/ovndb-servers.ocf
> /usr/lib/ocf/resource.d/ovn/ovndb-servers
> +ln -sf /usr/bin/ovn_detrace.py /usr/bin/ovn-detrace
> ;;
> abort-upgrade|abort-remove|abort-deconfigure)
> ;;
> diff --git a/debian/ovn-common.postrm b/debian/ovn-common.postrm
> index 9face726b..d607a66d5 100644
> --- a/debian/ovn-common.postrm
> +++ b/debian/ovn-common.postrm
> @@ -8,6 +8,7 @@ set -e
> case "$1" in
> purge|remove)
> rm -rf /usr/lib/ocf/resource.d/ovn
> +rm -f /usr/bin/ovn-detrace
> ;;
> upgrade|failed-upgrade|abort-install|abort-upgrade|disappear)
> ;;
> diff --git a/rhel/ovn-fedora.spec.in b/rhel/ovn-fedora.spec.in
> index 9c8647b5a..fc0992263 100644
> --- a/rhel/ovn-fedora.spec.in
> +++ b/rhel/ovn-fedora.spec.in
> @@ -264,6 +264,12 @@ if [ $1 -eq 1 ] ; then
> fi
> fi
>
> +%preun
> +if [ $1 -eq 0 ] ; then
> +# Package removal, not upgrade
> +rm %{_bindir}/ovn-detrace
> +fi
> +
> %preun central
> %if 0%{?systemd_preun:1}
> %systemd_preun ovn-northd.service
> @@ -318,6 +324,8 @@ fi
> %endif
>
> %post
> +ln -sf ovn_detrace.py %{_bindir}/ovn-detrace
> +
> %if %{with libcapng}
> if [ $1 -eq 1 ]; then
> sed -i 's:^#OVN_USER_ID=:OVN_USER_ID=:' %{_sysconfdir}/sysconfig/ovn
> @@ -462,6 +470,7 @@ fi
> %{_bindir}/ovn-nbctl
> %{_bindir}/ovn-sbctl
> %{_bindir}/ovn-trace
> +%{_bindir}/ovn_detrace.py
> %{_bindir}/ovn-detrace
> %{_bindir}/ovn-appctl
> %{_bindir}/ovn-ic-nbctl
> diff --git a/utilities/automake.mk b/utilities/automake.mk
> index a03892f20..67b04cbff 100644
> --- a/utilities/automake.mk
> +++ b/utilities/automake.mk
> @@ -20,7 +20,7 @@ MAN_ROOTS += \
> bin_SCRIPTS += \
> utilities/ovn-docker-overlay-driver \
> utilities/ovn-docker-underlay-driver \
> -utilities/ovn-detrace
> +utilities/ovn_detrace.py
>
> EXTRA_DIST += \
> utilities/ovn-ctl \
> @@ -34,7 +34,7 @@ EXTRA_DIST += \
> utilities/ovn-ic-sbctl.8.xml \
> utilities/ovn-appctl.8.xml \
> utilities/ovn-trace.8.xml \
> -utilities/ovn-detrace.in \
> +utilities/ovn_detrace.py.in \
> utilities/ovndb-servers.ocf \
> utilities/checkpatch.py \
> utilities/docker/Makefile \
> @@ -60,6 +60,7 @@ CLEANFILES += \
> utilities/ovn-trace.8 \
> utilities/ovn-detrace.1 \
> utilities/ovn-detrace \
> +utilities/ovn_detrace.py \
> utilities/ovn-appctl.8 \
> utilities/ovn-appctl \
> utilities/ovn-sim
> @@ -105,4 +106,13 @@ bin_PROGRAMS += utilities/ovn-appctl
> utilities_ovn_appctl_SOURCES = utilities/ovn-appctl.c
> utilities_ovn_appctl_LDADD = lib/libovn.la $(OVSDB_LIBDIR)/libovsdb.la
> $(OVS_LIBDIR)/libopenvswitch.la
>
> +# ovn-detrace
> +INSTALL_DATA_LOCAL += ovn-detrace-install
> +ovn-detrace-install:
> + ln -sf ovn_detrace.py $(DESTDIR)$(bindir)/ovn-detrace
> +
> +UNINSTALL_LOCAL += ovn-detrace-uninstall
> +ovn-detrace-uninstall:
> + rm -f $(DESTDIR)$(bindir)/ovn-detrace
> +
> include utilities/bugtool/automake.mk
> diff --git a/utilities/ovn-detrace.in b/utilities/ovn_detrace.py.in
> similarity index 100%
> rename from utilities/ovn-detrace.in
> rename to utilities/ovn_detrace.py.in
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Sync PMD ALB state with user commands.
Hi Kevin, > > > > I wonder if the logs in the above function be INFO instead of DBG ? > > Don't have a strong preference TBH. But if they were info, we need not > remove any test cases no ? > > > > The reason they are debug is so they won't pollute the logs when a rebalance > cannot occur Yup, agree, was just thinking if there is a way to not remove any testcases :) > > For example, if user sets ALB enabled and there is only one PMD. If it is at > 100% > loaded, this message would be printed every minute to say that a ALB will do > nothing. > > As nothing is changing and it can occur every minute I thought it was more > appropriate to keep it at debug level for when you need to check/debug the > operation of ALB. WDYT? Agreed :) Thanks! Acked-by: Sunil Pai G ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC ovn 0/5] Facilitate external use of ovn-detrace
On 10/18/21 14:51, Dumitru Ceara wrote: On 10/14/21 6:41 PM, Adrian Moreno wrote: ovn-detrace is a very useful tool for debugging OVN issues. It's core logic (mapping openflow cookies / ports with OVN objects) can be used for a variety of troubleshooting tools. Therefore, it would be desirable to make use of such logic from an external python program. This could be done by creating a python library (similarly to what ovs provides) that is built and pushed to PyPi for other projects to consume. However, being the only python script that lives in OVN, this might be a bit of an overkill, so what this series proposes is an intermediate step that does not require that much extra maintenance and still alleviates the main obstacles one finds when trying to use ovn-detrace as a python module which are: - python expects module names to end in .py and use underscores instead of hyphens - internally, ovn-detrace prints directy to stdout, the output of the ovn-detrace information should be configurable - the version information is not easily available With this series, ovn-detrace is renamed to ovn_detrace.py and a symlink with the old name is created for backwards compatibility. As a result, a use can point her PYTHONPATH to ovn's installation path, run "import ovn_detrace", and make use of ovn-detrace's logic with, say, individual openflow cookies instead of ofproto/trace outputs. I know it's not the cleanest way to do it. I'd love to hear your opinion on the matter. Reviewing notes: - The first patch is a small fix I spotted when playing around with ovn-detrace - I have not tested the debian package thoroughly Hi Adrian, I briefly tried the RFC series out and I don't see any functional changes, so that works for me. I quickly glanced at the patches too and they also look OK to me. Looking forward to the v1. Regards, Dumitru Thanks Dumitru, If the general approach seems OK, I'll send v1. Timothy, do you foresee any issues on the packaging side? Thanks -- Adrián Moreno ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs] lib: export calc_percentile function
Hi Aaron
We might for instance want to have some counters giving statistical data on
flow_mods added per ovn iteration.
So, for instance, we would be able to compare those statistics with
existing stopwatches indicating the time spent for those iterations, and
see whether a high time stopwatch is linked to a high value of the maximum
flow_mod
Coverage counters would not be sufficient. They provide very useful info
(average over a few seconds, minute, hour), but they do not provide (as far
as I know at least) this statistical view (max, percentile, ...) per
iteration - e.g. the maximum. Adding this statistical view to the coverage
counter does not seem to make too much sense for the existing coverage
counters, so I would not modify the coverage counters.
As I mentioned, we could make the stopwatch (slightly) more generic by
making two small modifications:
- Add an empty unit, so the stopwatch could cover all the "counts"
statistics, and not only the time related ones (as the existing
stopwatches).
- Add to the stopwatch printout the "Sum" of all items (and not only some
averages). This addition might be useful as well for the existing
stopwatches.
With those two minor changes, OVN could add its own statistics.
Do you think that such changes to the OVS stopwatch could be accepted
upstream?
Thanks
Xavier
On Thu, Oct 14, 2021 at 9:39 PM Aaron Conole wrote:
> Xavier Simonart writes:
>
> > Hi Aaron
> >
> > Thanks for looking into this.
> >
> > You are right when saying that OVN uses the stopwatch API just fine.
> > The goal of the proposed modification is to be able provide extra
> counters/statistics from OVN.
> > For instance, there was some interest about how many flows (or groups)
> are added (removed, updated, ...) by
> > ovn-controller.
> > In addition to the raw count, we'd like to provide some statistical view
> per iteration - hence the need of being able to
> > calculate things like percentiles (in addition to average, max, ...).
> >
> > The stopwatch API provides most of what would be needed, but would have
> required some changes:
> > - stopwatch always uses some "units" (msec, ...) while we would use it
> here to report non time-based statistics => we
> > would need to add some other "units". We could add an "empty" unit, to
> avoid having to require OVS changes
> > everytime we need different statistics (today flows, tomorrow something
> else).
> > - more important, stopwatch does not provide the total count we are
> looking for (i.e. the sum of all samples). We could
> > add a "Total" or a "Sum" to stopwatch, but this would have changed the
> stopwatch outputs for all existing counters. I
> > felt that this might be perceived as an API change or a compatibility
> issue.
> >
> > Hence I was proposing to only export the percentile related function, to
> avoid any change to OVS which might impact
> > backward compatibility.
> > But I would be happy to modify the stopwatch, adding the "Sum" and the
> empty unit. This would simplify OVN code as
> > well compared to what I was initially proposing.
> > Adding stopwatch_add_sample(...) would not change much in this case.
>
> I see - what you're looking for is some kind of generic statistics -
> would the coverage counters work instead? We do keep some flow
> statistics, so I guess it's useful to understand what you're looking
> to track. Maybe 'stopwatch' might not work perfectly, or maybe we can
> make some changes to make it seem more generic.
>
> > Thanks
> > Xavier
> >
> > On Wed, Oct 13, 2021 at 6:42 PM Aaron Conole wrote:
> >
> > Xavier Simonart writes:
> >
> > > export calc_percentile function (and percentile struct) so that
> > > it can be used in other libraries such as OVN.
> > >
> > > Signed-off-by: Xavier Simonart
> > > ---
> >
> > What is the intent to use this in other libraries? It would be nice to
> > understand why just running the existing stopwatch API doesn't work
> > (maybe you cannot start samples). AFAIK, OVN uses the stopwatch API
> > just fine?
> >
> > Perhaps it could make sense to expose the functionality in a different
> > fashion like:
> >
> >void stopwatch_add_sample(const char *, unsigned long long);
> >
> > This seems a useful API and doesn't expose all of the internal
> > information, but I don't know if it really is needed. Can you expand
> > why you need calc_percentile exposed?
> >
> > > lib/stopwatch.c | 24 +---
> > > lib/stopwatch.h | 26 ++
> > > 2 files changed, 27 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/lib/stopwatch.c b/lib/stopwatch.c
> > > index f5602163b..003c3a05f 100644
> > > --- a/lib/stopwatch.c
> > > +++ b/lib/stopwatch.c
> > > @@ -35,25 +35,6 @@ struct average {
> > > double alpha; /* Weight given to new samples */
> > > };
> > >
> > > -#define MARKERS 5
> > > -
> > > -/* Number of samples to collect before reporting P-square calculated
> > > - * percentile
> > > - */
> > >
Re: [ovs-dev] [PATCH v16 3/8] dpif-offload-provider: Introduce dpif-offload-provider layer
On 18 Oct 2021, at 14:03, Chris Mi wrote:
> Hi Eelco,
>
> On 10/15/2021 9:42 PM, Eelco Chaudron wrote:
>> Small comments inline, and Ilya please take a look at the first
>> comment/request.
>>
>> //Eelco
>>
>>
>> On 12 Oct 2021, at 10:19, Chris Mi wrote:
>>
>>> Some offload actions require functionality that is not netdev
>>> based, but dpif. For example, sFlow action requires to create
>>> a psample netlink socket to receive the sampled packets from
>>> TC or kernel driver.
>>>
>>> Create dpif-offload-provider layer to support such actions.
>>>
>>> Signed-off-by: Chris Mi
>>> Reviewed-by: Eli Britstein
>>> ---
>>
>>
>>> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
>>> index 7e11b9697..ce20cdeb1 100644
>>> --- a/lib/dpif-provider.h
>>> +++ b/lib/dpif-provider.h
>>> @@ -22,8 +22,9 @@
>>>* exposed over OpenFlow as a single switch. Datapaths and the
>>> collections of
>>>* ports that they contain may be fixed or dynamic. */
>>>
>>> -#include "openflow/openflow.h"
>>> #include "dpif.h"
>>> +#include "dpif-offload-provider.h"
>>> +#include "openflow/openflow.h"
>>> #include "util.h"
>>>
>>> #ifdef __cplusplus
>>> @@ -635,6 +636,11 @@ struct dpif_class {
>>>* sufficient to store BOND_BUCKETS number of elements. */
>>> int (*bond_stats_get)(struct dpif *dpif, uint32_t bond_id,
>>> uint64_t *n_bytes);
>>> +
>>> +/* Some offload actions require functionality that is not netdev based,
>>> + * but dpif. Add dpif-offload-provider layer API to support such
>>> + * offload actions. */
>>> +const struct dpif_offload_api *offload_api;
>> From previous revisions:
>>
>> | EC> Here you add the provider directly into the dpif class. Not sure if
>> this is what Ilya had in mind. As in general, these get integrated into the
>> dpif/netdev, not the class. Ilya can you comment on/review this?
>> | CM> OK.
>>
>> From my side, this looks wrong as there is a direct relation between dpif
>> and dpif-offload. I would assume you should be able to pick a specific one,
>> or what else would have stopped us from adding the
>> dpif_offload_sflow_recv_wait()/dpif_offload_sflow_recv() directly in the
>> dpif.
> I'll think about it. If I understand correctly, it should be something like
> this, right?
>
> struct dpif {
> const struct dpif_class *dpif_class;
> const struct dpif_offload_api *dpif_offload_api;
> ...
Yes, this is what I was referring to.
> Thanks,
> Chris
>> To me, it's also not clear how we would continue from here, are there any
>> plans to move all offload stuff to the offload provider? If so, in what time
>> frame?
>>
>>> };
>>>
>>> extern const struct dpif_class dpif_netlink_class;
>>> diff --git a/lib/dpif.c b/lib/dpif.c
>>> index 8c4aed47b..51cf5d666 100644
>>> --- a/lib/dpif.c
>>> +++ b/lib/dpif.c
>>> @@ -153,6 +153,15 @@ dp_register_provider__(const struct dpif_class
>>> *new_class)
>>> return error;
>>> }
>>>
>>> +if (new_class->offload_api && new_class->offload_api->init) {
>>> +error = new_class->offload_api->init();
>>> +if (error) {
>>> +VLOG_WARN("failed to initialize %s datapath class for offload:
>>> %s",
>> Please use a capital F for Failed.
>>
>>> + new_class->type, ovs_strerror(error));
>>> +return error;
>>> +}
>>> +}
>>> +
>>> registered_class = xmalloc(sizeof *registered_class);
>>> registered_class->dpif_class = new_class;
>>> registered_class->refcount = 0;
>>> @@ -183,6 +192,7 @@ static int
>>> dp_unregister_provider__(const char *type)
>>> {
>>> struct shash_node *node;
>>> +const struct dpif_class *dpif_class;
>>> struct registered_dpif_class *registered_class;
>>>
>>> node = shash_find(_classes, type);
>>> @@ -196,6 +206,11 @@ dp_unregister_provider__(const char *type)
>>> return EBUSY;
>>> }
>>>
>>> +dpif_class = registered_class->dpif_class;
>>> +if (dpif_class->offload_api && dpif_class->offload_api->destroy) {
>>> +dpif_class->offload_api->destroy();
>>> +}
>>> +
>>> shash_delete(_classes, node);
>>> free(registered_class);
>>>
>>> --
>>> 2.30.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Sync PMD ALB state with user commands.
On 18/10/2021 13:03, Pai G, Sunil wrote:
Hi Kevin,
Hi Sunil,
Thanks for reviewing.
Patch LGTM in general, just a query below.
+ * This function checks that some basic conditions needed for a
+rebalance to be
+ * effective are met. Such as Rxq scheduling assignment type, more than
+one
+ * PMD, more than 2 Rxqs on a PMD. If there was no reconfiguration
+change
+ * since the last check, it reuses the last result.
+ *
+ * It is not intended to be an inclusive check of every condition that
+may make
+ * a rebalance ineffective. It is done as a quick check so a full
+ * pmd_rebalance_dry_run() can be avoided when it is not needed.
+ */
+static bool
+pmd_reblance_dry_run_needed(struct dp_netdev *dp)
+OVS_REQUIRES(dp->port_mutex)
+{
+struct dp_netdev_pmd_thread *pmd;
+struct pmd_auto_lb *pmd_alb = >pmd_alb;
+unsigned int cnt = 0;
+bool multi_rxq = false;
+
+/* Check if there was no reconfiguration since last check. */
+if (!pmd_alb->recheck_config) {
+if (!pmd_alb->do_dry_run) {
+VLOG_DBG("PMD auto load balance nothing to do, "
+ "no configuration changes since last check.");
+return false;
+}
+return true;
+}
+pmd_alb->recheck_config = false;
+
+/* Check for incompatible assignment type. */
+if (dp->pmd_rxq_assign_type == SCHED_ROUNDROBIN) {
+VLOG_DBG("PMD auto load balance nothing to do, "
+ "pmd-rxq-assign=roundrobin assignment type configured.");
+return pmd_alb->do_dry_run = false;
+}
+
+/* Check that there is at least 2 non-isolated PMDs and
+ * one of them is polling more than one rxq. */
+CMAP_FOR_EACH (pmd, node, >poll_threads) {
+if (pmd->core_id == NON_PMD_CORE_ID || pmd->isolated) {
+continue;
+}
+
+if (hmap_count(>poll_list) > 1) {
+multi_rxq = true;
+}
+if (cnt && multi_rxq) {
+return pmd_alb->do_dry_run = true;
+}
+cnt++;
+}
+
+VLOG_DBG("PMD auto load balance nothing to do, "
+ "not enough non-isolated PMDs or RxQs.");
+return pmd_alb->do_dry_run = false; }
+
I wonder if the logs in the above function be INFO instead of DBG ?
Don't have a strong preference TBH. But if they were info, we need not remove
any test cases no ?
The reason they are debug is so they won't pollute the logs when a
rebalance cannot occur.
For example, if user sets ALB enabled and there is only one PMD. If it
is at 100% loaded, this message would be printed every minute to say
that a ALB will do nothing.
As nothing is changing and it can occur every minute I thought it was
more appropriate to keep it at debug level for when you need to
check/debug the operation of ALB. WDYT?
Kevin.
Thanks and regards,
Sunil
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC ovn 0/5] Facilitate external use of ovn-detrace
On 10/14/21 6:41 PM, Adrian Moreno wrote: > ovn-detrace is a very useful tool for debugging OVN issues. > > It's core logic (mapping openflow cookies / ports with OVN objects) can > be used for a variety of troubleshooting tools. Therefore, it would be > desirable to make use of such logic from an external python program. > > This could be done by creating a python library (similarly to what ovs > provides) that is built and pushed to PyPi for other projects to > consume. > > However, being the only python script that lives in OVN, this might be a > bit of an overkill, so what this series proposes is an intermediate step > that does not require that much extra maintenance and still alleviates > the main obstacles one finds when trying to use ovn-detrace as a python > module which are: > - python expects module names to end in .py and use underscores instead > of hyphens > - internally, ovn-detrace prints directy to stdout, the output of the > ovn-detrace information should be configurable > - the version information is not easily available > > With this series, ovn-detrace is renamed to ovn_detrace.py and a > symlink with the old name is created for backwards compatibility. As a > result, a use can point her PYTHONPATH to ovn's installation path, > run "import ovn_detrace", and make use of ovn-detrace's logic with, say, > individual openflow cookies instead of ofproto/trace outputs. > > I know it's not the cleanest way to do it. I'd love to hear your opinion > on the matter. > > Reviewing notes: > - The first patch is a small fix I spotted when playing around with > ovn-detrace > - I have not tested the debian package thoroughly > Hi Adrian, I briefly tried the RFC series out and I don't see any functional changes, so that works for me. I quickly glanced at the patches too and they also look OK to me. Looking forward to the v1. Regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 5/7] northd: Introduce struct northd_data
Bleep bloop. Greetings Mark Gray, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Comment with 'xxx' marker #436 FILE: northd/northd.c:14423: /* XXX Having to explicitly clean up macam here Lines checked: 810, 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
Re: [ovs-dev] [PATCH ovn 0/7] northd: Introduce incremental processing framework
On 18/10/2021 11:03, Mark Gray wrote:
> On 16/10/2021 02:21, Han Zhou wrote:
>> On Wed, Sep 29, 2021 at 10:23 AM Mark Gray wrote:
>>>
>>> Add the 'inc-proc-eng' framework to northd. This does *not*
>>> add any incremental processing at this stage but provides the
>>> framework to do so. Even in this base configuration, we see an
>>> advantage as northd no longer processes the databases if it has
>>> been woken only to handle, for example, a unixctl command. This
>>> can be seen below
>>>
>>> $ ovn-appctl -t ovn-northd stopwatch/reset
>>> $ for i in {1..10}; do ovn-appctl -t ovn-northd stopwatch/show
>>> /dev/null; done
>>> $ ovn-appctl -t ovn-northd stopwatch/show ovnnb_db_run
>>> Statistics for 'ovnnb_db_run'
>>> Total samples: 0
>>> Maximum: 0 msec
>>> Minimum: 0 msec
>>> 95th percentile: 0.00 msec
>>> Short term average: 0.00 msec
>>> Long term average: 0.00 msec
>>>
>>> Hopefully this starting point will allow others to discuss or contribute
>>> changes to incrementally process some aspects of northd. We an also
>>> decide if it is worth progressing with this in general.
>>>
>>> Thanks,
>>>
>>> Mark Gray (7):
>>> inc-proc-eng: Allow definition of engine_node with global scope
>>> northd: Introduce incremental processing for northd
>>> northd: Add n_nat_entries field to 'struct ovn_datapath'
>>> northd: Rename struct northd_context
>>
>> Hi Mark,
>>
>> Sorry for the slow response, but I encountered a problem when applying the
>> series at the 4th patch:
>>
>> Applying: northd: Rename struct northd_context
>> error: sha1 information is lacking or useless (northd/northd.c).
>> error: could not build fake ancestor
>>
>> Could you check on your side if it applies cleanly on the current main
>> branch?
>>
>
> Hi Han,
>
> It did need a rebase but I don't think that is the cause of your error.
> Your error looks like the patches were being applied before ovn-northd.c
> was renamed to northd.c? Regardless, I did the rebase and tested. The
> new version can be found at
> https://patchwork.ozlabs.org/project/ovn/list/?series=267610 :
This ^ version has a compile error in 2/7 which caused 0-day to fail.
This, below, is an updated series that fixes that:
https://patchwork.ozlabs.org/project/ovn/list/?series=267648
>
> $ git log --format=oneline -1
> 3a597e03cd411e8316b68e13f88f1b097ad82ba0 (HEAD -> main, upstream/main)
> controller: do not mark bfd and ipv6_pd msgs as local-only
>
> $ git checkout -b test
> Switched to a new branch 'test'
>
> $ git-pw series apply 267610
> Server version missing
> You should provide the server version in the URL configured via
> git-config or --server
> This will be required in git-pw 2.0
> Applying: inc-proc-eng: Allow definition of engine_node with global scope
> Applying: northd: Introduce incremental processing for northd
> Applying: northd: Add n_nat_entries field to 'struct ovn_datapath'
> Applying: northd: Rename struct northd_context
> Applying: northd: Introduce struct northd_data
> Applying: northd: Add lflow node
> Applying: en_lflow: Generate logical flows
>
>
>> Thanks,
>> Han
>>
>>> northd: Introduce struct northd_data
>>> northd: Add lflow node
>>> en_lflow: Generate logical flows
>>>
>>> controller/ovn-controller.c | 2 +-
>>> lib/inc-proc-eng.h | 24 ++-
>>> northd/automake.mk | 6 +
>>> northd/en-lflow.c | 54 ++
>>> northd/en-lflow.h | 16 ++
>>> northd/en-northd.c | 65 +++
>>> northd/en-northd.h | 21 +++
>>> northd/inc-proc-northd.c| 257 ++
>>> northd/inc-proc-northd.h| 15 ++
>>> northd/northd.c | 358 +++-
>>> northd/northd.h | 36 +++-
>>> northd/ovn-northd.c | 237 ++--
>>> 12 files changed, 807 insertions(+), 284 deletions(-)
>>> create mode 100644 northd/en-lflow.c
>>> create mode 100644 northd/en-lflow.h
>>> create mode 100644 northd/en-northd.c
>>> create mode 100644 northd/en-northd.h
>>> create mode 100644 northd/inc-proc-northd.c
>>> create mode 100644 northd/inc-proc-northd.h
>>>
>>> --
>>> 2.27.0
>>>
>>>
>>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v3 6/7] northd: Add lflow node
Add an additional node that initially does nothing. This serves as a
template for how to add a new node. This node is inserted after
the northd_node.
This node will be updated in a later commit to generate logical
flows for the SBDB.
Signed-off-by: Mark Gray
---
northd/automake.mk | 2 ++
northd/en-lflow.c| 42
northd/en-lflow.h| 16 +++
northd/inc-proc-northd.c | 5 -
northd/northd.c | 2 --
5 files changed, 64 insertions(+), 3 deletions(-)
create mode 100644 northd/en-lflow.c
create mode 100644 northd/en-lflow.h
diff --git a/northd/automake.mk b/northd/automake.mk
index f0c1fb11c83a..4862ec7b7ff3 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -6,6 +6,8 @@ northd_ovn_northd_SOURCES = \
northd/ovn-northd.c \
northd/en-northd.c \
northd/en-northd.h \
+ northd/en-lflow.c \
+ northd/en-lflow.h \
northd/inc-proc-northd.c \
northd/inc-proc-northd.h \
northd/ipam.c \
diff --git a/northd/en-lflow.c b/northd/en-lflow.c
new file mode 100644
index ..46072cb0162e
--- /dev/null
+++ b/northd/en-lflow.c
@@ -0,0 +1,42 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include
+
+#include
+#include
+#include
+
+#include "en-lflow.h"
+#include "en-northd.h"
+
+#include "lib/inc-proc-eng.h"
+#include "northd.h"
+#include "openvswitch/vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(en_lflow);
+
+void en_lflow_run(struct engine_node *node, void *data OVS_UNUSED)
+{
+engine_set_node_state(node, EN_UPDATED);
+}
+void *en_lflow_init(struct engine_node *node OVS_UNUSED,
+ struct engine_arg *arg OVS_UNUSED)
+{
+return NULL;
+}
+
+void en_lflow_cleanup(void *data OVS_UNUSED)
+{
+}
diff --git a/northd/en-lflow.h b/northd/en-lflow.h
new file mode 100644
index ..0e4d522ff3fe
--- /dev/null
+++ b/northd/en-lflow.h
@@ -0,0 +1,16 @@
+#ifndef EN_LFLOW_H
+#define EN_LFLOW_H 1
+
+#include
+
+#include
+#include
+#include
+
+#include "lib/inc-proc-eng.h"
+
+void en_lflow_run(struct engine_node *node, void *data);
+void *en_lflow_init(struct engine_node *node, struct engine_arg *arg);
+void en_lflow_cleanup(void *data);
+
+#endif /* EN_LFLOW_H */
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 572b8de6536a..519fc1d0cb46 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -25,6 +25,7 @@
#include "openvswitch/vlog.h"
#include "inc-proc-northd.h"
#include "en-northd.h"
+#include "en-lflow.h"
#include "util.h"
VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
@@ -140,6 +141,7 @@ enum sb_engine_node {
/* Define engine nodes for other nodes. They should be defined as static to
* avoid sparse errors. */
static ENGINE_NODE(northd, "northd");
+static ENGINE_NODE(lflow, "lflow");
void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
struct ovsdb_idl_loop *sb)
@@ -204,13 +206,14 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
engine_add_input(_northd, _sb_load_balancer, NULL);
engine_add_input(_northd, _sb_bfd, NULL);
engine_add_input(_northd, _sb_fdb, NULL);
+engine_add_input(_lflow, _northd, NULL);
struct engine_arg engine_arg = {
.nb_idl = nb->idl,
.sb_idl = sb->idl,
};
-engine_init(_northd, _arg);
+engine_init(_lflow, _arg);
}
void inc_proc_northd_run(struct northd_idl_context *ctx,
diff --git a/northd/northd.c b/northd/northd.c
index 6426fcbe1215..e1097e6b301a 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -12974,7 +12974,6 @@ struct lflows_thread_pool {
struct worker_pool *pool;
};
-
static void *
build_lflows_thread(void *arg)
{
@@ -14556,7 +14555,6 @@ ovnnb_db_run(struct northd_data *data,
cleanup_stale_fdp_entries(ctx, >datapaths);
bfd_cleanup_connections(ctx, >bfd_connections);
stopwatch_stop(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
-
}
/* Stores the list of chassis which references an ha_chassis_group.
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v3 5/7] northd: Introduce struct northd_data
'struct northd_data' is used to hold the output data from the
incremental processing node 'en_northd'. This will be used, in
a later commit, as the input data to 'en_lflow'. In order to achieve
this, we refactor in the following way:
* Introduce northd_init() which initializes this data.
* Introduce northd_destroy() which clears this data for a new iteration.
* Introduce northd_indices_create() which does a one-time creation of
indices for number of tables needed by northd.
* Remove 'ovn_internal_version' from the context passed to northd
as it can be determined within northd using ovn_get_internal_version.
* Remove 'use_parallel_build' from the context passed to northd
as it can be determined within northd using can_parallelize_hashes().
* Refactor northd.c to use 'struct northd_data' where applicable.
Signed-off-by: Mark Gray
---
northd/en-northd.c | 28 -
northd/en-northd.h | 4 +
northd/northd.c | 278
northd/northd.h | 28 -
northd/ovn-northd.c | 28 +
5 files changed, 204 insertions(+), 162 deletions(-)
diff --git a/northd/en-northd.c b/northd/en-northd.c
index 8dec51535af0..1b206521e152 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -20,26 +20,46 @@
#include "en-northd.h"
#include "lib/inc-proc-eng.h"
+#include "openvswitch/list.h" /* TODO This is needed for ovn-parallel-hmap.h.
+ * lib/ovn-parallel-hmap.h should be updated
+ * to include this dependency itself */
+#include "lib/ovn-parallel-hmap.h"
#include "northd.h"
+#include "lib/util.h"
#include "openvswitch/vlog.h"
VLOG_DEFINE_THIS_MODULE(en_northd);
-void en_northd_run(struct engine_node *node, void *data OVS_UNUSED)
+void en_northd_run(struct engine_node *node, void *data)
{
const struct engine_context *eng_ctx = engine_get_context();
struct northd_idl_context *ctx = eng_ctx->client_ctx;
-ovn_db_run(ctx);
+struct northd_data *northd_data = ((struct ed_type_northd *)data)->data;
+northd_destroy(northd_data);
+northd_init(northd_data);
+
+northd_run(ctx, northd_data);
engine_set_node_state(node, EN_UPDATED);
}
void *en_northd_init(struct engine_node *node OVS_UNUSED,
- struct engine_arg *arg OVS_UNUSED)
+ struct engine_arg *arg)
{
-return NULL;
+struct ed_type_northd *data = xmalloc(sizeof *data);
+data->data = xmalloc(sizeof *data->data);
+
+data->data->use_parallel_build = can_parallelize_hashes(false);
+northd_indices_create(data->data, arg->sb_idl);
+northd_init(data->data);
+
+return data;
}
void en_northd_cleanup(void *data OVS_UNUSED)
{
+struct northd_data *northd_data = ((struct ed_type_northd *)data)->data;
+
+northd_destroy(northd_data);
+free(northd_data);
}
diff --git a/northd/en-northd.h b/northd/en-northd.h
index 0e7f76245e69..da386298d821 100644
--- a/northd/en-northd.h
+++ b/northd/en-northd.h
@@ -9,6 +9,10 @@
#include "lib/inc-proc-eng.h"
+struct ed_type_northd {
+struct northd_data *data;
+};
+
void en_northd_run(struct engine_node *node OVS_UNUSED, void *data OVS_UNUSED);
void *en_northd_init(struct engine_node *node OVS_UNUSED,
struct engine_arg *arg);
diff --git a/northd/northd.c b/northd/northd.c
index d61368c1e406..6426fcbe1215 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -2950,6 +2950,7 @@ chassis_group_list_changed(
static void
sync_ha_chassis_group_for_sbpb(struct northd_idl_context *ctx,
+ struct northd_data *data,
const struct nbrec_ha_chassis_group *nb_ha_grp,
struct ovsdb_idl_index *sbrec_chassis_by_name,
const struct sbrec_port_binding *pb)
@@ -2957,7 +2958,7 @@ sync_ha_chassis_group_for_sbpb(struct northd_idl_context
*ctx,
bool new_sb_chassis_group = false;
const struct sbrec_ha_chassis_group *sb_ha_grp =
ha_chassis_group_lookup_by_name(
-ctx->sbrec_ha_chassis_grp_by_name, nb_ha_grp->name);
+data->sbrec_ha_chassis_grp_by_name, nb_ha_grp->name);
if (!sb_ha_grp) {
sb_ha_grp = sbrec_ha_chassis_group_insert(ctx->ovnsb_txn);
@@ -3003,6 +3004,7 @@ sync_ha_chassis_group_for_sbpb(struct northd_idl_context
*ctx,
static void
copy_gw_chassis_from_nbrp_to_sbpb(
struct northd_idl_context *ctx,
+struct northd_data *data,
struct ovsdb_idl_index *sbrec_chassis_by_name,
const struct nbrec_logical_router_port *lrp,
const struct sbrec_port_binding *port_binding)
@@ -3012,7 +3014,7 @@ copy_gw_chassis_from_nbrp_to_sbpb(
* for the distributed gateway router port. */
const struct sbrec_ha_chassis_group *sb_ha_chassis_group =
ha_chassis_group_lookup_by_name(
-ctx->sbrec_ha_chassis_grp_by_name, lrp->name);
+
[ovs-dev] [PATCH ovn v3 7/7] en_lflow: Generate logical flows
Generate logical flows using 'en_flow' incremental processing node.
This node uses output data from 'en_northd' in order to generate
logical flows.
Signed-off-by: Mark Gray
---
northd/en-lflow.c | 12
northd/northd.c | 6 +-
northd/northd.h | 1 +
3 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index 46072cb0162e..e24c00e039c3 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -23,12 +23,24 @@
#include "lib/inc-proc-eng.h"
#include "northd.h"
+#include "stopwatch.h"
+#include "lib/stopwatch-names.h"
+#include "timeval.h"
#include "openvswitch/vlog.h"
VLOG_DEFINE_THIS_MODULE(en_lflow);
void en_lflow_run(struct engine_node *node, void *data OVS_UNUSED)
{
+const struct engine_context *eng_ctx = engine_get_context();
+struct northd_idl_context *ctx = eng_ctx->client_ctx;
+
+struct ed_type_northd *northd_data = engine_get_input_data("northd", node);
+
+stopwatch_start(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
+build_lflows(ctx, northd_data->data);
+stopwatch_stop(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
+
engine_set_node_state(node, EN_UPDATED);
}
void *en_lflow_init(struct engine_node *node OVS_UNUSED,
diff --git a/northd/northd.c b/northd/northd.c
index e1097e6b301a..03f321ba63df 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -13327,8 +13327,7 @@ static bool reset_parallel = false;
/* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB database,
* constructing their contents based on the OVN_NB database. */
-static void
-build_lflows(struct northd_idl_context *ctx, struct northd_data *data)
+void build_lflows(struct northd_idl_context *ctx, struct northd_data *data)
{
struct hmap lflows;
@@ -14542,9 +14541,6 @@ ovnnb_db_run(struct northd_data *data,
build_meter_groups(ctx, >meter_groups);
build_bfd_table(ctx, >bfd_connections, >ports);
stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
-stopwatch_start(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
-build_lflows(ctx, data);
-stopwatch_stop(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
ovn_update_ipv6_prefix(>ports);
diff --git a/northd/northd.h b/northd/northd.h
index d4bc5cf16541..07ea6899984f 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -51,5 +51,6 @@ void northd_destroy(struct northd_data *data);
void northd_init(struct northd_data *data);
void northd_indices_create(struct northd_data *data,
struct ovsdb_idl *ovnsb_idl);
+void build_lflows(struct northd_idl_context *ctx, struct northd_data *data);
#endif /* NORTHD_H */
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v3 4/7] northd: Rename struct northd_context
In order to prepare for a subsequent commit, rename
'struct northd_context' to 'struct northd_idl_context'. In
subsequent commits, 'struct northd_idl_context' will then be
used, only, to hold the IDL context required by northd.
Signed-off-by: Mark Gray
---
northd/en-northd.c | 2 +-
northd/inc-proc-northd.c | 2 +-
northd/inc-proc-northd.h | 2 +-
northd/northd.c | 89
northd/northd.h | 4 +-
northd/ovn-northd.c | 10 ++---
6 files changed, 55 insertions(+), 54 deletions(-)
diff --git a/northd/en-northd.c b/northd/en-northd.c
index d310fa4dd31f..8dec51535af0 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -28,7 +28,7 @@ VLOG_DEFINE_THIS_MODULE(en_northd);
void en_northd_run(struct engine_node *node, void *data OVS_UNUSED)
{
const struct engine_context *eng_ctx = engine_get_context();
-struct northd_context *ctx = eng_ctx->client_ctx;
+struct northd_idl_context *ctx = eng_ctx->client_ctx;
ovn_db_run(ctx);
engine_set_node_state(node, EN_UPDATED);
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 85baeb07d3d9..572b8de6536a 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -213,7 +213,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
engine_init(_northd, _arg);
}
-void inc_proc_northd_run(struct northd_context *ctx,
+void inc_proc_northd_run(struct northd_idl_context *ctx,
bool recompute) {
engine_set_force_recompute(recompute);
engine_init_run();
diff --git a/northd/inc-proc-northd.h b/northd/inc-proc-northd.h
index 09cb8f3b3a80..6ee056dc14f5 100644
--- a/northd/inc-proc-northd.h
+++ b/northd/inc-proc-northd.h
@@ -8,7 +8,7 @@
void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
struct ovsdb_idl_loop *sb);
-void inc_proc_northd_run(struct northd_context *ctx,
+void inc_proc_northd_run(struct northd_idl_context *ctx,
bool recompute);
void inc_proc_northd_cleanup(void);
diff --git a/northd/northd.c b/northd/northd.c
index 9e84ee6310a9..d61368c1e406 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -1256,7 +1256,7 @@ ovn_datapath_update_external_ids(struct ovn_datapath *od)
}
static void
-join_datapaths(struct northd_context *ctx, struct hmap *datapaths,
+join_datapaths(struct northd_idl_context *ctx, struct hmap *datapaths,
struct ovs_list *sb_only, struct ovs_list *nb_only,
struct ovs_list *both, struct ovs_list *lr_list)
{
@@ -1367,7 +1367,7 @@ is_vxlan_mode(struct ovsdb_idl *ovnsb_idl)
}
static uint32_t
-get_ovn_max_dp_key_local(struct northd_context *ctx)
+get_ovn_max_dp_key_local(struct northd_idl_context *ctx)
{
if (is_vxlan_mode(ctx->ovnsb_idl)) {
/* OVN_MAX_DP_GLOBAL_NUM doesn't apply for vxlan mode. */
@@ -1377,7 +1377,7 @@ get_ovn_max_dp_key_local(struct northd_context *ctx)
}
static void
-ovn_datapath_allocate_key(struct northd_context *ctx,
+ovn_datapath_allocate_key(struct northd_idl_context *ctx,
struct hmap *datapaths, struct hmap *dp_tnlids,
struct ovn_datapath *od, uint32_t *hint)
{
@@ -1397,7 +1397,7 @@ ovn_datapath_allocate_key(struct northd_context *ctx,
}
static void
-ovn_datapath_assign_requested_tnl_id(struct northd_context *ctx,
+ovn_datapath_assign_requested_tnl_id(struct northd_idl_context *ctx,
struct hmap *dp_tnlids,
struct ovn_datapath *od)
{
@@ -1431,7 +1431,7 @@ ovn_datapath_assign_requested_tnl_id(struct
northd_context *ctx,
* Initializes 'datapaths' to contain a "struct ovn_datapath" for every logical
* switch and router. */
static void
-build_datapaths(struct northd_context *ctx, struct hmap *datapaths,
+build_datapaths(struct northd_idl_context *ctx, struct hmap *datapaths,
struct ovs_list *lr_list)
{
struct ovs_list sb_only, nb_only, both;
@@ -2402,7 +2402,7 @@ tag_alloc_create_new_tag(struct hmap *tag_alloc_table,
static void
-join_logical_ports(struct northd_context *ctx,
+join_logical_ports(struct northd_idl_context *ctx,
struct hmap *datapaths, struct hmap *ports,
struct hmap *chassis_qdisc_queues,
struct hmap *tag_alloc_table, struct ovs_list *sb_only,
@@ -2864,7 +2864,7 @@ sbpb_gw_chassis_needs_update(
}
static struct sbrec_ha_chassis *
-create_sb_ha_chassis(struct northd_context *ctx,
+create_sb_ha_chassis(struct northd_idl_context *ctx,
const struct sbrec_chassis *chassis,
const char *chassis_name, int priority)
{
@@ -2949,7 +2949,7 @@ chassis_group_list_changed(
}
static void
-sync_ha_chassis_group_for_sbpb(struct northd_context *ctx,
+sync_ha_chassis_group_for_sbpb(struct northd_idl_context *ctx,
const struct
[ovs-dev] [PATCH ovn v3 2/7] northd: Introduce incremental processing for northd
Initial implementation adds a single node (northd). This single
node executes the northd processing pipeline but does not do so
incrementally.
In order to develop incremental processing for northd, the code
will be organised with a .c/.h file for each I-P node following
the naming convention en-.c/.h. These files will
contain definition of the node data, the main node processing
functions and change handlers (if any). The purpose of these nodes
will be coordination of the nodes work and implemention of the
relevant interfaces to plugin to the I-P framework. The actual
work that will be executed by the node will be organised into
a companion file or files. Ideally this file will follow the
naming convention of the node: e.g. en-.c is
associated with .c.
Initial node topology sees the northd node dependent on all DB
nodes. This will evolve over time.
Co-authored-by: Numan Siddique
Signed-off-by: Numan Siddique
Signed-off-by: Mark Gray
---
lib/inc-proc-eng.h | 16 +++
northd/automake.mk | 4 +
northd/en-northd.c | 45 +++
northd/en-northd.h | 17 +++
northd/inc-proc-northd.c | 254 +++
northd/inc-proc-northd.h | 15 +++
northd/northd.c | 12 +-
northd/northd.h | 9 +-
northd/ovn-northd.c | 201 ---
9 files changed, 490 insertions(+), 83 deletions(-)
create mode 100644 northd/en-northd.c
create mode 100644 northd/en-northd.h
create mode 100644 northd/inc-proc-northd.c
create mode 100644 northd/inc-proc-northd.h
diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
index 1ccae559dff6..a3f5a7e64287 100644
--- a/lib/inc-proc-eng.h
+++ b/lib/inc-proc-eng.h
@@ -63,15 +63,22 @@
#define ENGINE_MAX_INPUT 256
#define ENGINE_MAX_OVSDB_INDEX 256
+#include
+#include
+
+#include "compiler.h"
+
struct engine_context {
struct ovsdb_idl_txn *ovs_idl_txn;
struct ovsdb_idl_txn *ovnsb_idl_txn;
+struct ovsdb_idl_txn *ovnnb_idl_txn;
void *client_ctx;
};
/* Arguments to be passed to the engine at engine_init(). */
struct engine_arg {
struct ovsdb_idl *sb_idl;
+struct ovsdb_idl *nb_idl;
struct ovsdb_idl *ovs_idl;
};
@@ -347,6 +354,11 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void *data
OVS_UNUSED) \
#define ENGINE_FUNC_SB(TBL_NAME) \
ENGINE_FUNC_OVSDB(sb, TBL_NAME)
+/* Macro to define member functions of an engine node which represents
+ * a table of OVN NB DB */
+#define ENGINE_FUNC_NB(TBL_NAME) \
+ENGINE_FUNC_OVSDB(nb, TBL_NAME)
+
/* Macro to define member functions of an engine node which represents
* a table of open_vswitch DB */
#define ENGINE_FUNC_OVS(TBL_NAME) \
@@ -360,6 +372,10 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void *data
OVS_UNUSED) \
#define ENGINE_NODE_SB(TBL_NAME, TBL_NAME_STR) \
ENGINE_NODE_OVSDB(sb, "SB", TBL_NAME, TBL_NAME_STR);
+/* Macro to define an engine node which represents a table of OVN NB DB */
+#define ENGINE_NODE_NB(TBL_NAME, TBL_NAME_STR) \
+ENGINE_NODE_OVSDB(nb, "NB", TBL_NAME, TBL_NAME_STR);
+
/* Macro to define an engine node which represents a table of open_vswitch
* DB */
#define ENGINE_NODE_OVS(TBL_NAME, TBL_NAME_STR) \
diff --git a/northd/automake.mk b/northd/automake.mk
index 35ad8c09d9ba..f0c1fb11c83a 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -4,6 +4,10 @@ northd_ovn_northd_SOURCES = \
northd/northd.c \
northd/northd.h \
northd/ovn-northd.c \
+ northd/en-northd.c \
+ northd/en-northd.h \
+ northd/inc-proc-northd.c \
+ northd/inc-proc-northd.h \
northd/ipam.c \
northd/ipam.h
northd_ovn_northd_LDADD = \
diff --git a/northd/en-northd.c b/northd/en-northd.c
new file mode 100644
index ..d310fa4dd31f
--- /dev/null
+++ b/northd/en-northd.c
@@ -0,0 +1,45 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include
+
+#include
+#include
+#include
+
+#include "en-northd.h"
+#include "lib/inc-proc-eng.h"
+#include "northd.h"
+#include "openvswitch/vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(en_northd);
+
+void en_northd_run(struct engine_node *node, void *data OVS_UNUSED)
+{
+const struct engine_context *eng_ctx = engine_get_context();
+struct northd_context *ctx = eng_ctx->client_ctx;
+ovn_db_run(ctx);
+
+engine_set_node_state(node, EN_UPDATED);
+
+}
+void *en_northd_init(struct engine_node *node OVS_UNUSED,
+ struct
[ovs-dev] [PATCH ovn v3 3/7] northd: Add n_nat_entries field to 'struct ovn_datapath'
destroy_nat_entries() iterates over nat_entries using 'n_nat'
as the number of NAT entries from the NB database. This behaviour can be
incorrect as it assumes that there are 'n_nat' 'nat_entries'. 'struct
ovn_datapath' should maintain a count of 'nat_entries' in 'struct
ovn_datapath' rather than read the value from NBDB IDL to properly
account for the number of 'nat_entries'.
This issue becomes appatent when using destroy_nat_entries()
as part of an incremental processing loop:
Consider an example in which we have completed iteration x, and
started iteration x+1. If we add a NAT entry to NBDB between iteration x
and iteration x+1, od->nbr->n_nat will contain the updated value of
n_nat as the northbound record will have been updated. However, if
we do not (re)initialized the nat entries in od, od->n_nat_entries
could be equal to the previous value. This can cause destroy_nat_entries()
to loop over the wrong number of 'nat_entries' causing unexpected
behaviour.
Signed-off-by: Mark Gray
---
northd/northd.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/northd/northd.c b/northd/northd.c
index 1321e26faa9d..9e84ee6310a9 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -603,6 +603,8 @@ struct ovn_datapath {
/* NAT entries configured on the router. */
struct ovn_nat *nat_entries;
+size_t n_nat_entries;
+
bool has_distributed_nat;
/* Set of nat external ips on the router. */
@@ -787,6 +789,7 @@ init_nat_entries(struct ovn_datapath *od)
od->has_distributed_nat = true;
}
}
+od->n_nat_entries = od->nbr->n_nat;
}
static void
@@ -800,7 +803,7 @@ destroy_nat_entries(struct ovn_datapath *od)
destroy_lport_addresses(>dnat_force_snat_addrs);
destroy_lport_addresses(>lb_force_snat_addrs);
-for (size_t i = 0; i < od->nbr->n_nat; i++) {
+for (size_t i = 0; i < od->n_nat_entries; i++) {
destroy_lport_addresses(>nat_entries[i].ext_addrs);
}
}
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v3 1/7] inc-proc-eng: Allow definition of engine_node with global scope
Refactor ENGINE_NODE() macro to not assign function pointers. This allows ENGINE_NODE() to be used outside functions, creating an engine_node with global scope (but can be statically defined within a file). This allows more flexibility in how the I-P engine can be used as engine nodes can be defined. Allow this is not explicitly required for I-P it does allow for a cleaner code base as the node definitions can be kept together outside any functions. Additional function pointers (e.g. is_valid(), clear_tracked_data()), may be assigned later, if required. Signed-off-by: Mark Gray --- controller/ovn-controller.c | 2 +- lib/inc-proc-eng.h | 8 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 4202f32ccaf6..48121ccce082 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -3219,7 +3219,7 @@ main(int argc, char *argv[]) stopwatch_create(BFD_RUN_STOPWATCH_NAME, SW_MS); /* Define inc-proc-engine nodes. */ -ENGINE_NODE_CUSTOM_WITH_CLEAR_TRACK_DATA(ct_zones, "ct_zones"); +ENGINE_NODE_CUSTOM_WITH_CLEAR_TRACK_DATA_IS_VALID(ct_zones, "ct_zones"); ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data"); ENGINE_NODE(non_vif_data, "non_vif_data"); ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h index 859b30a71c86..1ccae559dff6 100644 --- a/lib/inc-proc-eng.h +++ b/lib/inc-proc-eng.h @@ -295,19 +295,19 @@ void engine_ovsdb_node_add_index(struct engine_node *, const char *name, .init = en_##NAME##_init, \ .run = en_##NAME##_run, \ .cleanup = en_##NAME##_cleanup, \ -.is_valid = en_##NAME##_is_valid, \ +.is_valid = NULL, \ .clear_tracked_data = NULL, \ }; #define ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \ ENGINE_NODE_DEF(NAME, NAME_STR) -#define ENGINE_NODE_CUSTOM_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ +#define ENGINE_NODE_CUSTOM_WITH_CLEAR_TRACK_DATA_IS_VALID(NAME, NAME_STR) \ ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \ -en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; +en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; \ +en_##NAME.is_valid = en_##NAME##_is_valid; #define ENGINE_NODE(NAME, NAME_STR) \ -static bool (*en_##NAME##_is_valid)(struct engine_node *node) = NULL; \ ENGINE_NODE_DEF(NAME, NAME_STR) #define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ -- 2.27.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v3 0/7] northd: Introduce incremental processing framework
Add the 'inc-proc-eng' framework to northd. This does *not*
add any incremental processing at this stage but provides the
framework to do so. Even in this base configuration, we see an
advantage as northd no longer processes the databases if it has
been woken only to handle, for example, a unixctl command. This
can be seen below
$ ovn-appctl -t ovn-northd stopwatch/reset
$ for i in {1..10}; do ovn-appctl -t ovn-northd stopwatch/show >/dev/null; done
$ ovn-appctl -t ovn-northd stopwatch/show ovnnb_db_run
Statistics for 'ovnnb_db_run'
Total samples: 0
Maximum: 0 msec
Minimum: 0 msec
95th percentile: 0.00 msec
Short term average: 0.00 msec
Long term average: 0.00 msec
Hopefully this starting point will allow others to discuss or contribute
changes to incrementally process some aspects of northd. We an also
decide if it is worth progressing with this in general.
Thanks,
v2: Rebase
v3: Fix compile error in 2/7 picked up by 0-day robot
Mark Gray (7):
inc-proc-eng: Allow definition of engine_node with global scope
northd: Introduce incremental processing for northd
northd: Add n_nat_entries field to 'struct ovn_datapath'
northd: Rename struct northd_context
northd: Introduce struct northd_data
northd: Add lflow node
en_lflow: Generate logical flows
controller/ovn-controller.c | 2 +-
lib/inc-proc-eng.h | 24 ++-
northd/automake.mk | 6 +
northd/en-lflow.c | 54 ++
northd/en-lflow.h | 16 ++
northd/en-northd.c | 65 +++
northd/en-northd.h | 21 ++
northd/inc-proc-northd.c| 257 +
northd/inc-proc-northd.h| 15 ++
northd/northd.c | 368 +++-
northd/northd.h | 36 +++-
northd/ovn-northd.c | 237 +--
12 files changed, 812 insertions(+), 289 deletions(-)
create mode 100644 northd/en-lflow.c
create mode 100644 northd/en-lflow.h
create mode 100644 northd/en-northd.c
create mode 100644 northd/en-northd.h
create mode 100644 northd/inc-proc-northd.c
create mode 100644 northd/inc-proc-northd.h
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Sync PMD ALB state with user commands.
Hi Kevin,
Patch LGTM in general, just a query below.
> + * This function checks that some basic conditions needed for a
> +rebalance to be
> + * effective are met. Such as Rxq scheduling assignment type, more than
> +one
> + * PMD, more than 2 Rxqs on a PMD. If there was no reconfiguration
> +change
> + * since the last check, it reuses the last result.
> + *
> + * It is not intended to be an inclusive check of every condition that
> +may make
> + * a rebalance ineffective. It is done as a quick check so a full
> + * pmd_rebalance_dry_run() can be avoided when it is not needed.
> + */
> +static bool
> +pmd_reblance_dry_run_needed(struct dp_netdev *dp)
> +OVS_REQUIRES(dp->port_mutex)
> +{
> +struct dp_netdev_pmd_thread *pmd;
> +struct pmd_auto_lb *pmd_alb = >pmd_alb;
> +unsigned int cnt = 0;
> +bool multi_rxq = false;
> +
> +/* Check if there was no reconfiguration since last check. */
> +if (!pmd_alb->recheck_config) {
> +if (!pmd_alb->do_dry_run) {
> +VLOG_DBG("PMD auto load balance nothing to do, "
> + "no configuration changes since last check.");
> +return false;
> +}
> +return true;
> +}
> +pmd_alb->recheck_config = false;
> +
> +/* Check for incompatible assignment type. */
> +if (dp->pmd_rxq_assign_type == SCHED_ROUNDROBIN) {
> +VLOG_DBG("PMD auto load balance nothing to do, "
> + "pmd-rxq-assign=roundrobin assignment type configured.");
> +return pmd_alb->do_dry_run = false;
> +}
> +
> +/* Check that there is at least 2 non-isolated PMDs and
> + * one of them is polling more than one rxq. */
> +CMAP_FOR_EACH (pmd, node, >poll_threads) {
> +if (pmd->core_id == NON_PMD_CORE_ID || pmd->isolated) {
> +continue;
> +}
> +
> +if (hmap_count(>poll_list) > 1) {
> +multi_rxq = true;
> +}
> +if (cnt && multi_rxq) {
> +return pmd_alb->do_dry_run = true;
> +}
> +cnt++;
> +}
> +
> +VLOG_DBG("PMD auto load balance nothing to do, "
> + "not enough non-isolated PMDs or RxQs.");
> +return pmd_alb->do_dry_run = false; }
> +
I wonder if the logs in the above function be INFO instead of DBG ?
Don't have a strong preference TBH. But if they were info, we need not remove
any test cases no ?
Thanks and regards,
Sunil
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v16 3/8] dpif-offload-provider: Introduce dpif-offload-provider layer
Hi Eelco,
On 10/15/2021 9:42 PM, Eelco Chaudron wrote:
Small comments inline, and Ilya please take a look at the first comment/request.
//Eelco
On 12 Oct 2021, at 10:19, Chris Mi wrote:
Some offload actions require functionality that is not netdev
based, but dpif. For example, sFlow action requires to create
a psample netlink socket to receive the sampled packets from
TC or kernel driver.
Create dpif-offload-provider layer to support such actions.
Signed-off-by: Chris Mi
Reviewed-by: Eli Britstein
---
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 7e11b9697..ce20cdeb1 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -22,8 +22,9 @@
* exposed over OpenFlow as a single switch. Datapaths and the collections of
* ports that they contain may be fixed or dynamic. */
-#include "openflow/openflow.h"
#include "dpif.h"
+#include "dpif-offload-provider.h"
+#include "openflow/openflow.h"
#include "util.h"
#ifdef __cplusplus
@@ -635,6 +636,11 @@ struct dpif_class {
* sufficient to store BOND_BUCKETS number of elements. */
int (*bond_stats_get)(struct dpif *dpif, uint32_t bond_id,
uint64_t *n_bytes);
+
+/* Some offload actions require functionality that is not netdev based,
+ * but dpif. Add dpif-offload-provider layer API to support such
+ * offload actions. */
+const struct dpif_offload_api *offload_api;
From previous revisions:
| EC> Here you add the provider directly into the dpif class. Not sure if this
is what Ilya had in mind. As in general, these get integrated into the
dpif/netdev, not the class. Ilya can you comment on/review this?
| CM> OK.
From my side, this looks wrong as there is a direct relation between dpif and
dpif-offload. I would assume you should be able to pick a specific one, or what
else would have stopped us from adding the
dpif_offload_sflow_recv_wait()/dpif_offload_sflow_recv() directly in the dpif.
I'll think about it. If I understand correctly, it should be something
like this, right?
struct dpif {
const struct dpif_class *dpif_class;
const struct dpif_offload_api *dpif_offload_api;
...
Thanks,
Chris
To me, it's also not clear how we would continue from here, are there any plans
to move all offload stuff to the offload provider? If so, in what time frame?
};
extern const struct dpif_class dpif_netlink_class;
diff --git a/lib/dpif.c b/lib/dpif.c
index 8c4aed47b..51cf5d666 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -153,6 +153,15 @@ dp_register_provider__(const struct dpif_class *new_class)
return error;
}
+if (new_class->offload_api && new_class->offload_api->init) {
+error = new_class->offload_api->init();
+if (error) {
+VLOG_WARN("failed to initialize %s datapath class for offload: %s",
Please use a capital F for Failed.
+ new_class->type, ovs_strerror(error));
+return error;
+}
+}
+
registered_class = xmalloc(sizeof *registered_class);
registered_class->dpif_class = new_class;
registered_class->refcount = 0;
@@ -183,6 +192,7 @@ static int
dp_unregister_provider__(const char *type)
{
struct shash_node *node;
+const struct dpif_class *dpif_class;
struct registered_dpif_class *registered_class;
node = shash_find(_classes, type);
@@ -196,6 +206,11 @@ dp_unregister_provider__(const char *type)
return EBUSY;
}
+dpif_class = registered_class->dpif_class;
+if (dpif_class->offload_api && dpif_class->offload_api->destroy) {
+dpif_class->offload_api->destroy();
+}
+
shash_delete(_classes, node);
free(registered_class);
--
2.30.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 ovn 3/3] controller: add memory accounting for local_datapath
On 10/14/21 5:17 PM, Lorenzo Bianconi wrote: [...] > diff --git a/controller/local_data.h b/controller/local_data.h > index f6317e9ca..3defa9925 100644 > --- a/controller/local_data.h > +++ b/controller/local_data.h > @@ -154,5 +154,12 @@ bool get_chassis_tunnel_ofport(const struct hmap > *chassis_tunnels, > ofp_port_t *ofport); > > void chassis_tunnels_destroy(struct hmap *chassis_tunnels); > +void local_datapath_memory_usage(struct simap *usage); > +void > +add_local_datapath_external_port(struct local_datapath *ld, > + char *logical_port, const void *data); Probably best to change this to 'const char *logical_port'. When reviewing I was in doubt about whether we should free the shash_node's data when removing external_ports. Changing the type to const would make it explicit, as free works only on non-const pointers. > +void > +remove_local_datapath_external_port(struct local_datapath *ld, > +char *logical_port); Same here. Thanks! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 ovn 3/3] controller: add memory accounting for local_datapath
On 10/14/21 5:17 PM, Lorenzo Bianconi wrote:
> Similar to if-status-mgr, track memory allocated for local_datapath.
>
> Signed-off-by: Lorenzo Bianconi
> ---
> controller/binding.c| 6 ++--
> controller/local_data.c | 66 +
> controller/local_data.h | 7
> controller/ovn-controller.c | 1 +
> 4 files changed, 77 insertions(+), 3 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index c037b2352..329d0d12b 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -374,8 +374,8 @@ update_ld_external_ports(const struct sbrec_port_binding
> *binding_rec,
> struct local_datapath *ld = get_local_datapath(
> local_datapaths, binding_rec->datapath->tunnel_key);
> if (ld) {
> -shash_replace(>external_ports, binding_rec->logical_port,
> - binding_rec);
> +add_local_datapath_external_port(ld, binding_rec->logical_port,
> + binding_rec);
> }
> }
>
> @@ -1756,7 +1756,7 @@ remove_pb_from_local_datapath(const struct
> sbrec_port_binding *pb,
> ld->localnet_port = NULL;
> }
> } else if (!strcmp(pb->type, "external")) {
> -shash_find_and_delete(>external_ports, pb->logical_port);
> +remove_local_datapath_external_port(ld, pb->logical_port);
> }
> }
>
> diff --git a/controller/local_data.c b/controller/local_data.c
> index 6efed2de0..48e6958b7 100644
> --- a/controller/local_data.c
> +++ b/controller/local_data.c
> @@ -47,6 +47,30 @@ static struct tracked_datapath *tracked_datapath_create(
> enum en_tracked_resource_type tracked_type,
> struct hmap *tracked_datapaths);
>
> +static uint64_t local_datapath_usage;
> +
> +static void
> +local_datapath_peer_ports_mem_add(struct local_datapath *ld,
> + size_t n_peer_ports)
> +{
> +/* memory accounting - peer_ports. */
> +local_datapath_usage +=
> +(ld->n_allocated_peer_ports - n_peer_ports) * sizeof *ld->peer_ports;
> +}
> +
> +static void
> +local_datapath_ext_port_mem_update(struct local_datapath *ld, const char
> *name,
> + bool erase)
> +{
> +struct shash_node *node = shash_find(>external_ports, name);
> +
> +if (!node && !erase) { /* add a new element */
> +local_datapath_usage += (sizeof *node + strlen(name));
> +} else if (node && erase) { /* remove an element */
> +local_datapath_usage -= (sizeof *node + strlen(name));
> +}
> +}
> +
> struct local_datapath *
> get_local_datapath(const struct hmap *local_datapaths, uint32_t tunnel_key)
> {
> @@ -63,6 +87,9 @@ local_datapath_alloc(const struct sbrec_datapath_binding
> *dp)
> ld->datapath = dp;
> ld->is_switch = datapath_is_switch(dp);
> shash_init(>external_ports);
> +/* memory accounting - common part. */
> +local_datapath_usage += sizeof *ld;
> +
> return ld;
> }
>
> @@ -80,6 +107,16 @@ local_datapaths_destroy(struct hmap *local_datapaths)
> void
> local_datapath_destroy(struct local_datapath *ld)
> {
> +/* memory accounting. */
> +struct shash_node *node;
> +SHASH_FOR_EACH (node, >external_ports) {
> +local_datapath_usage -= strlen(node->name);
> +}
> +local_datapath_usage -= shash_count(>external_ports) * sizeof *node;
> +local_datapath_usage -= sizeof *ld;
> +local_datapath_usage -=
> +ld->n_allocated_peer_ports * sizeof *ld->peer_ports;
> +
> free(ld->peer_ports);
> shash_destroy(>external_ports);
> free(ld);
> @@ -175,10 +212,12 @@ add_local_datapath_peer_port(
> if (!present) {
> ld->n_peer_ports++;
> if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
> +size_t n_peer_ports = ld->n_allocated_peer_ports;
> ld->peer_ports =
> x2nrealloc(ld->peer_ports,
> >n_allocated_peer_ports,
> sizeof *ld->peer_ports);
> +local_datapath_peer_ports_mem_add(ld, n_peer_ports);
> }
> ld->peer_ports[ld->n_peer_ports - 1].local = pb;
> ld->peer_ports[ld->n_peer_ports - 1].remote = peer;
> @@ -204,10 +243,12 @@ add_local_datapath_peer_port(
>
> peer_ld->n_peer_ports++;
> if (peer_ld->n_peer_ports > peer_ld->n_allocated_peer_ports) {
> +size_t n_peer_ports = peer_ld->n_allocated_peer_ports;
> peer_ld->peer_ports =
> x2nrealloc(peer_ld->peer_ports,
> _ld->n_allocated_peer_ports,
> sizeof *peer_ld->peer_ports);
> +local_datapath_peer_ports_mem_add(peer_ld, n_peer_ports);
> }
> peer_ld->peer_ports[peer_ld->n_peer_ports - 1].local = peer;
> peer_ld->peer_ports[peer_ld->n_peer_ports - 1].remote = pb;
> @@ -248,6 +289,22 @@ remove_local_datapath_peer_port(const struct
> sbrec_port_binding *pb,
Re: [ovs-dev] [PATCH v4 ovn 2/3] controller: add memory accounting for if_status_mgr module
On 10/14/21 5:17 PM, Lorenzo Bianconi wrote: > Introduce memory accounting for data structures in ovn-controller > if_status_mgr module. > > Signed-off-by: Lorenzo Bianconi > --- Acked-by: Dumitru Ceara Thanks! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] dpif:Fix function pointer check for bond_add.
There was typo in function pointer check in
dpif_bond_add() before calling bond_add()
Fixes: 9df65060cf4c ("userspace: Avoid dp_hash recirculation for balance-tcp
bond mode.")
Signed-off-by: Somnath Chatterjee
---
lib/dpif.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/dpif.c b/lib/dpif.c
index 8c4aed4..294b1c4 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -2012,7 +2012,7 @@ dpif_meter_del(struct dpif *dpif, ofproto_meter_id
meter_id,
int
dpif_bond_add(struct dpif *dpif, uint32_t bond_id, odp_port_t *member_map)
{
-return dpif->dpif_class->bond_del
+return dpif->dpif_class->bond_add
? dpif->dpif_class->bond_add(dpif, bond_id, member_map)
: EOPNOTSUPP;
}
--
2.7.4
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 2/7] northd: Introduce incremental processing for northd
Bleep bloop. Greetings Mark Gray, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. build: libtool: link: gcc -std=gnu99 -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -o controller/ovn-controller controller/bfd.o controller/binding.o controller/chassis.o controller/encaps.o controller/ha-chassis.o controller/if-status.o controller/ip-mcast.o controller/lflow.o controller/lflow-cache.o controller/lport.o controller/ofctrl.o controller/ofctrl-seqno.o controller/pinctrl.o controller/patch.o controller/ovn-controller.o controller/physical.o controller/mac-learn.o controller/local_data.o lib/.libs/libovn.a /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib/.libs/libopenvswitch.a -lssl -lcrypto -lcap-ng -lpthread -lrt -lm -lunbound depbase=`echo controller-vtep/binding.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\ gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./ovn -I ./include -I ./lib -I ./lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT controller-vtep/binding.o -MD -MP -MF $depbase.Tpo -c -o controller-vtep/binding.o controller-vtep/binding.c &&\ mv -f $depbase.Tpo $depbase.Po depbase=`echo controller-vtep/gateway.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\ gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./ovn -I ./include -I ./lib -I ./lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT controller-vtep/gateway.o -MD -MP -MF $depbase.Tpo -c -o controller-vtep/gateway.o controller-vtep/gateway.c &&\ mv -f $depbase.Tpo $depbase.Po depbase=`echo controller-vtep/ovn-controller-vtep.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\ gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./ovn -I ./include -I ./lib -I ./lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT controller-vtep/ovn-controller-vtep.o -MD -MP -MF $depbase.Tpo -c -o controller-vtep/ovn-controller-vtep.o controller-vtep/ovn-controller-vtep.c &&\ mv -f $depbase.Tpo $depbase.Po depbase=`echo controller-vtep/vtep.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\ gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./ovn -I ./include -I ./lib -I ./lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include -I
Re: [ovs-dev] [PATCH ovn 0/7] northd: Introduce incremental processing framework
On 16/10/2021 02:21, Han Zhou wrote:
> On Wed, Sep 29, 2021 at 10:23 AM Mark Gray wrote:
>>
>> Add the 'inc-proc-eng' framework to northd. This does *not*
>> add any incremental processing at this stage but provides the
>> framework to do so. Even in this base configuration, we see an
>> advantage as northd no longer processes the databases if it has
>> been woken only to handle, for example, a unixctl command. This
>> can be seen below
>>
>> $ ovn-appctl -t ovn-northd stopwatch/reset
>> $ for i in {1..10}; do ovn-appctl -t ovn-northd stopwatch/show
>> /dev/null; done
>> $ ovn-appctl -t ovn-northd stopwatch/show ovnnb_db_run
>> Statistics for 'ovnnb_db_run'
>> Total samples: 0
>> Maximum: 0 msec
>> Minimum: 0 msec
>> 95th percentile: 0.00 msec
>> Short term average: 0.00 msec
>> Long term average: 0.00 msec
>>
>> Hopefully this starting point will allow others to discuss or contribute
>> changes to incrementally process some aspects of northd. We an also
>> decide if it is worth progressing with this in general.
>>
>> Thanks,
>>
>> Mark Gray (7):
>> inc-proc-eng: Allow definition of engine_node with global scope
>> northd: Introduce incremental processing for northd
>> northd: Add n_nat_entries field to 'struct ovn_datapath'
>> northd: Rename struct northd_context
>
> Hi Mark,
>
> Sorry for the slow response, but I encountered a problem when applying the
> series at the 4th patch:
>
> Applying: northd: Rename struct northd_context
> error: sha1 information is lacking or useless (northd/northd.c).
> error: could not build fake ancestor
>
> Could you check on your side if it applies cleanly on the current main
> branch?
>
Hi Han,
It did need a rebase but I don't think that is the cause of your error.
Your error looks like the patches were being applied before ovn-northd.c
was renamed to northd.c? Regardless, I did the rebase and tested. The
new version can be found at
https://patchwork.ozlabs.org/project/ovn/list/?series=267610 :
$ git log --format=oneline -1
3a597e03cd411e8316b68e13f88f1b097ad82ba0 (HEAD -> main, upstream/main)
controller: do not mark bfd and ipv6_pd msgs as local-only
$ git checkout -b test
Switched to a new branch 'test'
$ git-pw series apply 267610
Server version missing
You should provide the server version in the URL configured via
git-config or --server
This will be required in git-pw 2.0
Applying: inc-proc-eng: Allow definition of engine_node with global scope
Applying: northd: Introduce incremental processing for northd
Applying: northd: Add n_nat_entries field to 'struct ovn_datapath'
Applying: northd: Rename struct northd_context
Applying: northd: Introduce struct northd_data
Applying: northd: Add lflow node
Applying: en_lflow: Generate logical flows
> Thanks,
> Han
>
>> northd: Introduce struct northd_data
>> northd: Add lflow node
>> en_lflow: Generate logical flows
>>
>> controller/ovn-controller.c | 2 +-
>> lib/inc-proc-eng.h | 24 ++-
>> northd/automake.mk | 6 +
>> northd/en-lflow.c | 54 ++
>> northd/en-lflow.h | 16 ++
>> northd/en-northd.c | 65 +++
>> northd/en-northd.h | 21 +++
>> northd/inc-proc-northd.c| 257 ++
>> northd/inc-proc-northd.h| 15 ++
>> northd/northd.c | 358 +++-
>> northd/northd.h | 36 +++-
>> northd/ovn-northd.c | 237 ++--
>> 12 files changed, 807 insertions(+), 284 deletions(-)
>> create mode 100644 northd/en-lflow.c
>> create mode 100644 northd/en-lflow.h
>> create mode 100644 northd/en-northd.c
>> create mode 100644 northd/en-northd.h
>> create mode 100644 northd/inc-proc-northd.c
>> create mode 100644 northd/inc-proc-northd.h
>>
>> --
>> 2.27.0
>>
>>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 5/7] northd: Introduce struct northd_data
'struct northd_data' is used to hold the output data from the
incremental processing node 'en_northd'. This will be used, in
a later commit, as the input data to 'en_lflow'. In order to achieve
this, we refactor in the following way:
* Introduce northd_init() which initializes this data.
* Introduce northd_destroy() which clears this data for a new iteration.
* Introduce northd_indices_create() which does a one-time creation of
indices for number of tables needed by northd.
* Remove 'ovn_internal_version' from the context passed to northd
as it can be determined within northd using ovn_get_internal_version.
* Remove 'use_parallel_build' from the context passed to northd
as it can be determined within northd using can_parallelize_hashes().
* Refactor northd.c to use 'struct northd_data' where applicable.
Signed-off-by: Mark Gray
---
northd/en-northd.c | 28 -
northd/en-northd.h | 4 +
northd/northd.c | 278
northd/northd.h | 27 -
northd/ovn-northd.c | 28 +
5 files changed, 204 insertions(+), 161 deletions(-)
diff --git a/northd/en-northd.c b/northd/en-northd.c
index 8dec51535af0..1b206521e152 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -20,26 +20,46 @@
#include "en-northd.h"
#include "lib/inc-proc-eng.h"
+#include "openvswitch/list.h" /* TODO This is needed for ovn-parallel-hmap.h.
+ * lib/ovn-parallel-hmap.h should be updated
+ * to include this dependency itself */
+#include "lib/ovn-parallel-hmap.h"
#include "northd.h"
+#include "lib/util.h"
#include "openvswitch/vlog.h"
VLOG_DEFINE_THIS_MODULE(en_northd);
-void en_northd_run(struct engine_node *node, void *data OVS_UNUSED)
+void en_northd_run(struct engine_node *node, void *data)
{
const struct engine_context *eng_ctx = engine_get_context();
struct northd_idl_context *ctx = eng_ctx->client_ctx;
-ovn_db_run(ctx);
+struct northd_data *northd_data = ((struct ed_type_northd *)data)->data;
+northd_destroy(northd_data);
+northd_init(northd_data);
+
+northd_run(ctx, northd_data);
engine_set_node_state(node, EN_UPDATED);
}
void *en_northd_init(struct engine_node *node OVS_UNUSED,
- struct engine_arg *arg OVS_UNUSED)
+ struct engine_arg *arg)
{
-return NULL;
+struct ed_type_northd *data = xmalloc(sizeof *data);
+data->data = xmalloc(sizeof *data->data);
+
+data->data->use_parallel_build = can_parallelize_hashes(false);
+northd_indices_create(data->data, arg->sb_idl);
+northd_init(data->data);
+
+return data;
}
void en_northd_cleanup(void *data OVS_UNUSED)
{
+struct northd_data *northd_data = ((struct ed_type_northd *)data)->data;
+
+northd_destroy(northd_data);
+free(northd_data);
}
diff --git a/northd/en-northd.h b/northd/en-northd.h
index 0e7f76245e69..da386298d821 100644
--- a/northd/en-northd.h
+++ b/northd/en-northd.h
@@ -9,6 +9,10 @@
#include "lib/inc-proc-eng.h"
+struct ed_type_northd {
+struct northd_data *data;
+};
+
void en_northd_run(struct engine_node *node OVS_UNUSED, void *data OVS_UNUSED);
void *en_northd_init(struct engine_node *node OVS_UNUSED,
struct engine_arg *arg);
diff --git a/northd/northd.c b/northd/northd.c
index d61368c1e406..6426fcbe1215 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -2950,6 +2950,7 @@ chassis_group_list_changed(
static void
sync_ha_chassis_group_for_sbpb(struct northd_idl_context *ctx,
+ struct northd_data *data,
const struct nbrec_ha_chassis_group *nb_ha_grp,
struct ovsdb_idl_index *sbrec_chassis_by_name,
const struct sbrec_port_binding *pb)
@@ -2957,7 +2958,7 @@ sync_ha_chassis_group_for_sbpb(struct northd_idl_context
*ctx,
bool new_sb_chassis_group = false;
const struct sbrec_ha_chassis_group *sb_ha_grp =
ha_chassis_group_lookup_by_name(
-ctx->sbrec_ha_chassis_grp_by_name, nb_ha_grp->name);
+data->sbrec_ha_chassis_grp_by_name, nb_ha_grp->name);
if (!sb_ha_grp) {
sb_ha_grp = sbrec_ha_chassis_group_insert(ctx->ovnsb_txn);
@@ -3003,6 +3004,7 @@ sync_ha_chassis_group_for_sbpb(struct northd_idl_context
*ctx,
static void
copy_gw_chassis_from_nbrp_to_sbpb(
struct northd_idl_context *ctx,
+struct northd_data *data,
struct ovsdb_idl_index *sbrec_chassis_by_name,
const struct nbrec_logical_router_port *lrp,
const struct sbrec_port_binding *port_binding)
@@ -3012,7 +3014,7 @@ copy_gw_chassis_from_nbrp_to_sbpb(
* for the distributed gateway router port. */
const struct sbrec_ha_chassis_group *sb_ha_chassis_group =
ha_chassis_group_lookup_by_name(
-ctx->sbrec_ha_chassis_grp_by_name, lrp->name);
+
[ovs-dev] [PATCH ovn v2 6/7] northd: Add lflow node
Add an additional node that initially does nothing. This serves as a
template for how to add a new node. This node is inserted after
the northd_node.
This node will be updated in a later commit to generate logical
flows for the SBDB.
Signed-off-by: Mark Gray
---
northd/automake.mk | 2 ++
northd/en-lflow.c| 42
northd/en-lflow.h| 16 +++
northd/inc-proc-northd.c | 5 -
northd/northd.c | 2 --
5 files changed, 64 insertions(+), 3 deletions(-)
create mode 100644 northd/en-lflow.c
create mode 100644 northd/en-lflow.h
diff --git a/northd/automake.mk b/northd/automake.mk
index f0c1fb11c83a..4862ec7b7ff3 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -6,6 +6,8 @@ northd_ovn_northd_SOURCES = \
northd/ovn-northd.c \
northd/en-northd.c \
northd/en-northd.h \
+ northd/en-lflow.c \
+ northd/en-lflow.h \
northd/inc-proc-northd.c \
northd/inc-proc-northd.h \
northd/ipam.c \
diff --git a/northd/en-lflow.c b/northd/en-lflow.c
new file mode 100644
index ..46072cb0162e
--- /dev/null
+++ b/northd/en-lflow.c
@@ -0,0 +1,42 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include
+
+#include
+#include
+#include
+
+#include "en-lflow.h"
+#include "en-northd.h"
+
+#include "lib/inc-proc-eng.h"
+#include "northd.h"
+#include "openvswitch/vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(en_lflow);
+
+void en_lflow_run(struct engine_node *node, void *data OVS_UNUSED)
+{
+engine_set_node_state(node, EN_UPDATED);
+}
+void *en_lflow_init(struct engine_node *node OVS_UNUSED,
+ struct engine_arg *arg OVS_UNUSED)
+{
+return NULL;
+}
+
+void en_lflow_cleanup(void *data OVS_UNUSED)
+{
+}
diff --git a/northd/en-lflow.h b/northd/en-lflow.h
new file mode 100644
index ..0e4d522ff3fe
--- /dev/null
+++ b/northd/en-lflow.h
@@ -0,0 +1,16 @@
+#ifndef EN_LFLOW_H
+#define EN_LFLOW_H 1
+
+#include
+
+#include
+#include
+#include
+
+#include "lib/inc-proc-eng.h"
+
+void en_lflow_run(struct engine_node *node, void *data);
+void *en_lflow_init(struct engine_node *node, struct engine_arg *arg);
+void en_lflow_cleanup(void *data);
+
+#endif /* EN_LFLOW_H */
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 572b8de6536a..519fc1d0cb46 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -25,6 +25,7 @@
#include "openvswitch/vlog.h"
#include "inc-proc-northd.h"
#include "en-northd.h"
+#include "en-lflow.h"
#include "util.h"
VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
@@ -140,6 +141,7 @@ enum sb_engine_node {
/* Define engine nodes for other nodes. They should be defined as static to
* avoid sparse errors. */
static ENGINE_NODE(northd, "northd");
+static ENGINE_NODE(lflow, "lflow");
void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
struct ovsdb_idl_loop *sb)
@@ -204,13 +206,14 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
engine_add_input(_northd, _sb_load_balancer, NULL);
engine_add_input(_northd, _sb_bfd, NULL);
engine_add_input(_northd, _sb_fdb, NULL);
+engine_add_input(_lflow, _northd, NULL);
struct engine_arg engine_arg = {
.nb_idl = nb->idl,
.sb_idl = sb->idl,
};
-engine_init(_northd, _arg);
+engine_init(_lflow, _arg);
}
void inc_proc_northd_run(struct northd_idl_context *ctx,
diff --git a/northd/northd.c b/northd/northd.c
index 6426fcbe1215..e1097e6b301a 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -12974,7 +12974,6 @@ struct lflows_thread_pool {
struct worker_pool *pool;
};
-
static void *
build_lflows_thread(void *arg)
{
@@ -14556,7 +14555,6 @@ ovnnb_db_run(struct northd_data *data,
cleanup_stale_fdp_entries(ctx, >datapaths);
bfd_cleanup_connections(ctx, >bfd_connections);
stopwatch_stop(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
-
}
/* Stores the list of chassis which references an ha_chassis_group.
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 7/7] en_lflow: Generate logical flows
Generate logical flows using 'en_flow' incremental processing node.
This node uses output data from 'en_northd' in order to generate
logical flows.
Signed-off-by: Mark Gray
---
northd/en-lflow.c | 12
northd/northd.c | 6 +-
northd/northd.h | 1 +
3 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index 46072cb0162e..e24c00e039c3 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -23,12 +23,24 @@
#include "lib/inc-proc-eng.h"
#include "northd.h"
+#include "stopwatch.h"
+#include "lib/stopwatch-names.h"
+#include "timeval.h"
#include "openvswitch/vlog.h"
VLOG_DEFINE_THIS_MODULE(en_lflow);
void en_lflow_run(struct engine_node *node, void *data OVS_UNUSED)
{
+const struct engine_context *eng_ctx = engine_get_context();
+struct northd_idl_context *ctx = eng_ctx->client_ctx;
+
+struct ed_type_northd *northd_data = engine_get_input_data("northd", node);
+
+stopwatch_start(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
+build_lflows(ctx, northd_data->data);
+stopwatch_stop(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
+
engine_set_node_state(node, EN_UPDATED);
}
void *en_lflow_init(struct engine_node *node OVS_UNUSED,
diff --git a/northd/northd.c b/northd/northd.c
index e1097e6b301a..03f321ba63df 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -13327,8 +13327,7 @@ static bool reset_parallel = false;
/* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB database,
* constructing their contents based on the OVN_NB database. */
-static void
-build_lflows(struct northd_idl_context *ctx, struct northd_data *data)
+void build_lflows(struct northd_idl_context *ctx, struct northd_data *data)
{
struct hmap lflows;
@@ -14542,9 +14541,6 @@ ovnnb_db_run(struct northd_data *data,
build_meter_groups(ctx, >meter_groups);
build_bfd_table(ctx, >bfd_connections, >ports);
stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
-stopwatch_start(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
-build_lflows(ctx, data);
-stopwatch_stop(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
ovn_update_ipv6_prefix(>ports);
diff --git a/northd/northd.h b/northd/northd.h
index d4bc5cf16541..07ea6899984f 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -51,5 +51,6 @@ void northd_destroy(struct northd_data *data);
void northd_init(struct northd_data *data);
void northd_indices_create(struct northd_data *data,
struct ovsdb_idl *ovnsb_idl);
+void build_lflows(struct northd_idl_context *ctx, struct northd_data *data);
#endif /* NORTHD_H */
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 2/7] northd: Introduce incremental processing for northd
Initial implementation adds a single node (northd). This single
node executes the northd processing pipeline but does not do so
incrementally.
In order to develop incremental processing for northd, the code
will be organised with a .c/.h file for each I-P node following
the naming convention en-.c/.h. These files will
contain definition of the node data, the main node processing
functions and change handlers (if any). The purpose of these nodes
will be coordination of the nodes work and implemention of the
relevant interfaces to plugin to the I-P framework. The actual
work that will be executed by the node will be organised into
a companion file or files. Ideally this file will follow the
naming convention of the node: e.g. en-.c is
associated with .c.
Initial node topology sees the northd node dependent on all DB
nodes. This will evolve over time.
Co-authored-by: Numan Siddique
Signed-off-by: Numan Siddique
Signed-off-by: Mark Gray
---
lib/inc-proc-eng.h | 16 +++
northd/automake.mk | 4 +
northd/en-northd.c | 45 +++
northd/en-northd.h | 17 +++
northd/inc-proc-northd.c | 254 +++
northd/inc-proc-northd.h | 15 +++
northd/northd.c | 12 +-
northd/northd.h | 8 +-
northd/ovn-northd.c | 201 ---
9 files changed, 489 insertions(+), 83 deletions(-)
create mode 100644 northd/en-northd.c
create mode 100644 northd/en-northd.h
create mode 100644 northd/inc-proc-northd.c
create mode 100644 northd/inc-proc-northd.h
diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
index 1ccae559dff6..a3f5a7e64287 100644
--- a/lib/inc-proc-eng.h
+++ b/lib/inc-proc-eng.h
@@ -63,15 +63,22 @@
#define ENGINE_MAX_INPUT 256
#define ENGINE_MAX_OVSDB_INDEX 256
+#include
+#include
+
+#include "compiler.h"
+
struct engine_context {
struct ovsdb_idl_txn *ovs_idl_txn;
struct ovsdb_idl_txn *ovnsb_idl_txn;
+struct ovsdb_idl_txn *ovnnb_idl_txn;
void *client_ctx;
};
/* Arguments to be passed to the engine at engine_init(). */
struct engine_arg {
struct ovsdb_idl *sb_idl;
+struct ovsdb_idl *nb_idl;
struct ovsdb_idl *ovs_idl;
};
@@ -347,6 +354,11 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void *data
OVS_UNUSED) \
#define ENGINE_FUNC_SB(TBL_NAME) \
ENGINE_FUNC_OVSDB(sb, TBL_NAME)
+/* Macro to define member functions of an engine node which represents
+ * a table of OVN NB DB */
+#define ENGINE_FUNC_NB(TBL_NAME) \
+ENGINE_FUNC_OVSDB(nb, TBL_NAME)
+
/* Macro to define member functions of an engine node which represents
* a table of open_vswitch DB */
#define ENGINE_FUNC_OVS(TBL_NAME) \
@@ -360,6 +372,10 @@ static void en_##DB_NAME##_##TBL_NAME##_cleanup(void *data
OVS_UNUSED) \
#define ENGINE_NODE_SB(TBL_NAME, TBL_NAME_STR) \
ENGINE_NODE_OVSDB(sb, "SB", TBL_NAME, TBL_NAME_STR);
+/* Macro to define an engine node which represents a table of OVN NB DB */
+#define ENGINE_NODE_NB(TBL_NAME, TBL_NAME_STR) \
+ENGINE_NODE_OVSDB(nb, "NB", TBL_NAME, TBL_NAME_STR);
+
/* Macro to define an engine node which represents a table of open_vswitch
* DB */
#define ENGINE_NODE_OVS(TBL_NAME, TBL_NAME_STR) \
diff --git a/northd/automake.mk b/northd/automake.mk
index 35ad8c09d9ba..f0c1fb11c83a 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -4,6 +4,10 @@ northd_ovn_northd_SOURCES = \
northd/northd.c \
northd/northd.h \
northd/ovn-northd.c \
+ northd/en-northd.c \
+ northd/en-northd.h \
+ northd/inc-proc-northd.c \
+ northd/inc-proc-northd.h \
northd/ipam.c \
northd/ipam.h
northd_ovn_northd_LDADD = \
diff --git a/northd/en-northd.c b/northd/en-northd.c
new file mode 100644
index ..d310fa4dd31f
--- /dev/null
+++ b/northd/en-northd.c
@@ -0,0 +1,45 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include
+
+#include
+#include
+#include
+
+#include "en-northd.h"
+#include "lib/inc-proc-eng.h"
+#include "northd.h"
+#include "openvswitch/vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(en_northd);
+
+void en_northd_run(struct engine_node *node, void *data OVS_UNUSED)
+{
+const struct engine_context *eng_ctx = engine_get_context();
+struct northd_context *ctx = eng_ctx->client_ctx;
+ovn_db_run(ctx);
+
+engine_set_node_state(node, EN_UPDATED);
+
+}
+void *en_northd_init(struct engine_node *node OVS_UNUSED,
+ struct
[ovs-dev] [PATCH ovn v2 4/7] northd: Rename struct northd_context
In order to prepare for a subsequent commit, rename
'struct northd_context' to 'struct northd_idl_context'. In
subsequent commits, 'struct northd_idl_context' will then be
used, only, to hold the IDL context required by northd.
Signed-off-by: Mark Gray
---
northd/en-northd.c | 2 +-
northd/inc-proc-northd.c | 2 +-
northd/inc-proc-northd.h | 2 +-
northd/northd.c | 89
northd/northd.h | 4 +-
northd/ovn-northd.c | 10 ++---
6 files changed, 55 insertions(+), 54 deletions(-)
diff --git a/northd/en-northd.c b/northd/en-northd.c
index d310fa4dd31f..8dec51535af0 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -28,7 +28,7 @@ VLOG_DEFINE_THIS_MODULE(en_northd);
void en_northd_run(struct engine_node *node, void *data OVS_UNUSED)
{
const struct engine_context *eng_ctx = engine_get_context();
-struct northd_context *ctx = eng_ctx->client_ctx;
+struct northd_idl_context *ctx = eng_ctx->client_ctx;
ovn_db_run(ctx);
engine_set_node_state(node, EN_UPDATED);
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 85baeb07d3d9..572b8de6536a 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -213,7 +213,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
engine_init(_northd, _arg);
}
-void inc_proc_northd_run(struct northd_context *ctx,
+void inc_proc_northd_run(struct northd_idl_context *ctx,
bool recompute) {
engine_set_force_recompute(recompute);
engine_init_run();
diff --git a/northd/inc-proc-northd.h b/northd/inc-proc-northd.h
index 09cb8f3b3a80..6ee056dc14f5 100644
--- a/northd/inc-proc-northd.h
+++ b/northd/inc-proc-northd.h
@@ -8,7 +8,7 @@
void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
struct ovsdb_idl_loop *sb);
-void inc_proc_northd_run(struct northd_context *ctx,
+void inc_proc_northd_run(struct northd_idl_context *ctx,
bool recompute);
void inc_proc_northd_cleanup(void);
diff --git a/northd/northd.c b/northd/northd.c
index 9e84ee6310a9..d61368c1e406 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -1256,7 +1256,7 @@ ovn_datapath_update_external_ids(struct ovn_datapath *od)
}
static void
-join_datapaths(struct northd_context *ctx, struct hmap *datapaths,
+join_datapaths(struct northd_idl_context *ctx, struct hmap *datapaths,
struct ovs_list *sb_only, struct ovs_list *nb_only,
struct ovs_list *both, struct ovs_list *lr_list)
{
@@ -1367,7 +1367,7 @@ is_vxlan_mode(struct ovsdb_idl *ovnsb_idl)
}
static uint32_t
-get_ovn_max_dp_key_local(struct northd_context *ctx)
+get_ovn_max_dp_key_local(struct northd_idl_context *ctx)
{
if (is_vxlan_mode(ctx->ovnsb_idl)) {
/* OVN_MAX_DP_GLOBAL_NUM doesn't apply for vxlan mode. */
@@ -1377,7 +1377,7 @@ get_ovn_max_dp_key_local(struct northd_context *ctx)
}
static void
-ovn_datapath_allocate_key(struct northd_context *ctx,
+ovn_datapath_allocate_key(struct northd_idl_context *ctx,
struct hmap *datapaths, struct hmap *dp_tnlids,
struct ovn_datapath *od, uint32_t *hint)
{
@@ -1397,7 +1397,7 @@ ovn_datapath_allocate_key(struct northd_context *ctx,
}
static void
-ovn_datapath_assign_requested_tnl_id(struct northd_context *ctx,
+ovn_datapath_assign_requested_tnl_id(struct northd_idl_context *ctx,
struct hmap *dp_tnlids,
struct ovn_datapath *od)
{
@@ -1431,7 +1431,7 @@ ovn_datapath_assign_requested_tnl_id(struct
northd_context *ctx,
* Initializes 'datapaths' to contain a "struct ovn_datapath" for every logical
* switch and router. */
static void
-build_datapaths(struct northd_context *ctx, struct hmap *datapaths,
+build_datapaths(struct northd_idl_context *ctx, struct hmap *datapaths,
struct ovs_list *lr_list)
{
struct ovs_list sb_only, nb_only, both;
@@ -2402,7 +2402,7 @@ tag_alloc_create_new_tag(struct hmap *tag_alloc_table,
static void
-join_logical_ports(struct northd_context *ctx,
+join_logical_ports(struct northd_idl_context *ctx,
struct hmap *datapaths, struct hmap *ports,
struct hmap *chassis_qdisc_queues,
struct hmap *tag_alloc_table, struct ovs_list *sb_only,
@@ -2864,7 +2864,7 @@ sbpb_gw_chassis_needs_update(
}
static struct sbrec_ha_chassis *
-create_sb_ha_chassis(struct northd_context *ctx,
+create_sb_ha_chassis(struct northd_idl_context *ctx,
const struct sbrec_chassis *chassis,
const char *chassis_name, int priority)
{
@@ -2949,7 +2949,7 @@ chassis_group_list_changed(
}
static void
-sync_ha_chassis_group_for_sbpb(struct northd_context *ctx,
+sync_ha_chassis_group_for_sbpb(struct northd_idl_context *ctx,
const struct
[ovs-dev] [PATCH ovn v2 3/7] northd: Add n_nat_entries field to 'struct ovn_datapath'
destroy_nat_entries() iterates over nat_entries using 'n_nat'
as the number of NAT entries from the NB database. This behaviour can be
incorrect as it assumes that there are 'n_nat' 'nat_entries'. 'struct
ovn_datapath' should maintain a count of 'nat_entries' in 'struct
ovn_datapath' rather than read the value from NBDB IDL to properly
account for the number of 'nat_entries'.
This issue becomes appatent when using destroy_nat_entries()
as part of an incremental processing loop:
Consider an example in which we have completed iteration x, and
started iteration x+1. If we add a NAT entry to NBDB between iteration x
and iteration x+1, od->nbr->n_nat will contain the updated value of
n_nat as the northbound record will have been updated. However, if
we do not (re)initialized the nat entries in od, od->n_nat_entries
could be equal to the previous value. This can cause destroy_nat_entries()
to loop over the wrong number of 'nat_entries' causing unexpected
behaviour.
Signed-off-by: Mark Gray
---
northd/northd.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/northd/northd.c b/northd/northd.c
index 1321e26faa9d..9e84ee6310a9 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -603,6 +603,8 @@ struct ovn_datapath {
/* NAT entries configured on the router. */
struct ovn_nat *nat_entries;
+size_t n_nat_entries;
+
bool has_distributed_nat;
/* Set of nat external ips on the router. */
@@ -787,6 +789,7 @@ init_nat_entries(struct ovn_datapath *od)
od->has_distributed_nat = true;
}
}
+od->n_nat_entries = od->nbr->n_nat;
}
static void
@@ -800,7 +803,7 @@ destroy_nat_entries(struct ovn_datapath *od)
destroy_lport_addresses(>dnat_force_snat_addrs);
destroy_lport_addresses(>lb_force_snat_addrs);
-for (size_t i = 0; i < od->nbr->n_nat; i++) {
+for (size_t i = 0; i < od->n_nat_entries; i++) {
destroy_lport_addresses(>nat_entries[i].ext_addrs);
}
}
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 1/7] inc-proc-eng: Allow definition of engine_node with global scope
Refactor ENGINE_NODE() macro to not assign function pointers. This allows ENGINE_NODE() to be used outside functions, creating an engine_node with global scope (but can be statically defined within a file). This allows more flexibility in how the I-P engine can be used as engine nodes can be defined. Allow this is not explicitly required for I-P it does allow for a cleaner code base as the node definitions can be kept together outside any functions. Additional function pointers (e.g. is_valid(), clear_tracked_data()), may be assigned later, if required. Signed-off-by: Mark Gray --- controller/ovn-controller.c | 2 +- lib/inc-proc-eng.h | 8 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 4202f32ccaf6..48121ccce082 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -3219,7 +3219,7 @@ main(int argc, char *argv[]) stopwatch_create(BFD_RUN_STOPWATCH_NAME, SW_MS); /* Define inc-proc-engine nodes. */ -ENGINE_NODE_CUSTOM_WITH_CLEAR_TRACK_DATA(ct_zones, "ct_zones"); +ENGINE_NODE_CUSTOM_WITH_CLEAR_TRACK_DATA_IS_VALID(ct_zones, "ct_zones"); ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data"); ENGINE_NODE(non_vif_data, "non_vif_data"); ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h index 859b30a71c86..1ccae559dff6 100644 --- a/lib/inc-proc-eng.h +++ b/lib/inc-proc-eng.h @@ -295,19 +295,19 @@ void engine_ovsdb_node_add_index(struct engine_node *, const char *name, .init = en_##NAME##_init, \ .run = en_##NAME##_run, \ .cleanup = en_##NAME##_cleanup, \ -.is_valid = en_##NAME##_is_valid, \ +.is_valid = NULL, \ .clear_tracked_data = NULL, \ }; #define ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \ ENGINE_NODE_DEF(NAME, NAME_STR) -#define ENGINE_NODE_CUSTOM_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ +#define ENGINE_NODE_CUSTOM_WITH_CLEAR_TRACK_DATA_IS_VALID(NAME, NAME_STR) \ ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \ -en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; +en_##NAME.clear_tracked_data = en_##NAME##_clear_tracked_data; \ +en_##NAME.is_valid = en_##NAME##_is_valid; #define ENGINE_NODE(NAME, NAME_STR) \ -static bool (*en_##NAME##_is_valid)(struct engine_node *node) = NULL; \ ENGINE_NODE_DEF(NAME, NAME_STR) #define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \ -- 2.27.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v2 0/7] northd: Introduce incremental processing framework
Add the 'inc-proc-eng' framework to northd. This does *not*
add any incremental processing at this stage but provides the
framework to do so. Even in this base configuration, we see an
advantage as northd no longer processes the databases if it has
been woken only to handle, for example, a unixctl command. This
can be seen below
$ ovn-appctl -t ovn-northd stopwatch/reset
$ for i in {1..10}; do ovn-appctl -t ovn-northd stopwatch/show >/dev/null; done
$ ovn-appctl -t ovn-northd stopwatch/show ovnnb_db_run
Statistics for 'ovnnb_db_run'
Total samples: 0
Maximum: 0 msec
Minimum: 0 msec
95th percentile: 0.00 msec
Short term average: 0.00 msec
Long term average: 0.00 msec
Hopefully this starting point will allow others to discuss or contribute
changes to incrementally process some aspects of northd. We an also
decide if it is worth progressing with this in general.
Thanks,
v2: Rebase
Mark Gray (7):
inc-proc-eng: Allow definition of engine_node with global scope
northd: Introduce incremental processing for northd
northd: Add n_nat_entries field to 'struct ovn_datapath'
northd: Rename struct northd_context
northd: Introduce struct northd_data
northd: Add lflow node
en_lflow: Generate logical flows
controller/ovn-controller.c | 2 +-
lib/inc-proc-eng.h | 24 ++-
northd/automake.mk | 6 +
northd/en-lflow.c | 54 ++
northd/en-lflow.h | 16 ++
northd/en-northd.c | 65 +++
northd/en-northd.h | 21 ++
northd/inc-proc-northd.c| 257 +
northd/inc-proc-northd.h| 15 ++
northd/northd.c | 368 +++-
northd/northd.h | 36 +++-
northd/ovn-northd.c | 237 +--
12 files changed, 812 insertions(+), 289 deletions(-)
create mode 100644 northd/en-lflow.c
create mode 100644 northd/en-lflow.h
create mode 100644 northd/en-northd.c
create mode 100644 northd/en-northd.h
create mode 100644 northd/inc-proc-northd.c
create mode 100644 northd/inc-proc-northd.h
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
