[ovs-dev] [PATCH ovn v2] northd.c: Add flow to skip put_nd action if ip6.src or nd.sll is 0

[Ales Musil]

The ip6.src or nd.sll does not have to be always set.
According to rfc4861:

Source Address
 Either an address assigned to the interface from
 which this message is sent or (if Duplicate Address
 Detection is in progress [ADDRCONF]) the
 unspecified address.

Source link-layer address
 The link-layer address for the sender.  MUST NOT be
 included when the source IP address is the
 unspecified address.  Otherwise, on link layers
 that have addresses this option MUST be included in
 multicast solicitations and SHOULD be included in
 unicast solicitations.

Add rule that avoids adding MAC binding is either of those
is 0. This is continuation after discussion during review on
80187a803 (ovn-northd: Add flow to use eth.src if nd.tll is 0
in put_nd() action.)

Signed-off-by: Ales Musil 
---
 northd/northd.c | 3 +++
 northd/ovn-northd.8.xml | 6 ++
 tests/ovn-northd.at | 1 +
 3 files changed, 10 insertions(+)

diff --git a/northd/northd.c b/northd/northd.c
index 6997c280c..f1a1f9f50 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -11027,6 +11027,9 @@ build_neigh_learning_flows_for_lrouter(
   copp_meter_get(COPP_ARP, od->nbr->copp,
  meter_groups));
 
+ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95,
+  "nd_ns && (ip6.src == 0 || nd.sll == 0)", "next;");
+
 ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95,
   "nd_na && nd.tll == 0",
   "put_nd(inport, nd.target, eth.src); next;",
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 59c584710..5df74a410 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -2332,6 +2332,12 @@ next;
 to learn the neighbor.
   
 
+  
+A priority-95 flow with the match nd_ns 
+  (ip6.src == 0 || nd.sll == 0) and applies the action
+next;
+  
+
   
 A priority-90 flow with the match arp and
 applies the action
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 033b58b8c..7b43e7964 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -6751,6 +6751,7 @@ AT_CHECK([cat lrflows | grep -e lr_in_lookup_neighbor -e 
lr_in_learn_neighbor |
   table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_na), 
action=(put_nd(inport, nd.target, nd.tll); next;)
   table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_ns), 
action=(put_nd(inport, ip6.src, nd.sll); next;)
   table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_na && nd.tll == 
0), action=(put_nd(inport, nd.target, eth.src); next;)
+  table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_ns && (ip6.src == 
0 || nd.sll == 0)), action=(next;)
 ])
 
 AT_CLEANUP
-- 
2.35.3

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

[ovs-dev] [PATCH ovn] ovn-northd.at: Add OVN_FOR_EACH_NORTHD around LR NB Static_MAC_Binding table

[Ales Musil]

This is continuation after discussion during review on
80187a803 (ovn-northd: Add flow to use eth.src if nd.tll is 0
in put_nd() action.)

Signed-off-by: Ales Musil 
---
 tests/ovn-northd.at | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 033b58b8c..3f2d5d20a 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -6707,6 +6707,7 @@ AT_CHECK([ovn-sbctl dump-flows DR | grep -e lr_in_unsnat 
-e lr_out_snat -e lr_in
 
 AT_CLEANUP
 
+OVN_FOR_EACH_NORTHD([
 AT_SETUP([LR NB Static_MAC_Binding table])
 ovn_start
 
@@ -6730,6 +6731,7 @@ ovn-nbctl --may-exist static-mac-binding-add lr0-p0 
192.168.10.100 00:00:22:33:5
 wait_row_count Static_MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.100 
mac="00\:00\:22\:33\:55\:66"
 
 AT_CLEANUP
+])
 
 OVN_FOR_EACH_NORTHD([
 AT_SETUP([LR neighbor lookup and learning flows])
-- 
2.35.3

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

Re: [ovs-dev] [PATCH ovn] northd.c: Add flow to skip put_nd action if ip6.src or nd.sll is 0

[Ales Musil]

On Tue, Jun 28, 2022 at 4:41 PM Dumitru Ceara  wrote:

> On 6/27/22 11:56, Ales Musil wrote:
> > The ip6.src or nd.sll does not have to be always set.
> > According to rfc4861:
> >
> > Source Address
> >  Either an address assigned to the interface from
> >  which this message is sent or (if Duplicate Address
> >  Detection is in progress [ADDRCONF]) the
> >  unspecified address.
> >
> > Source link-layer address
> >  The link-layer address for the sender.  MUST NOT be
> >  included when the source IP address is the
> >  unspecified address.  Otherwise, on link layers
> >  that have addresses this option MUST be included in
> >  multicast solicitations and SHOULD be included in
> >  unicast solicitations.
> >
> > Add rule that avoids adding MAC binding is either of those
> > is 0. This is continuation after discussion during review on
> > 80187a803 (ovn-northd: Add flow to use eth.src if nd.tll is 0
> > in put_nd() action.)
> >
> > Signed-off-by: Ales Musil 
> > ---
>
> Hi Ales,
>
> Thanks for the follow up!
>
> >  northd/northd.c | 3 +++
> >  northd/ovn-northd.8.xml | 6 ++
> >  tests/ovn-northd.at | 3 +++
> >  3 files changed, 12 insertions(+)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 6997c280c..6634edb0f 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -11022,6 +11022,9 @@ build_neigh_learning_flows_for_lrouter(
> >  ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 100,
> >ds_cstr(match), "next;");
> >
> > +ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95,
> > +  "nd_ns && (ip6.src == 0 || nd.sll == 0)",
> "next;");
> > +
>
> Nit: I'd move this lower, I think the flows are grouped per IP family
> right now.
>

Moved in v2.


> >  ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90,
> >"arp", "put_arp(inport, arp.spa, arp.sha);
> next;",
> >copp_meter_get(COPP_ARP, od->nbr->copp,
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > index 59c584710..5df74a410 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> > @@ -2332,6 +2332,12 @@ next;
> >  to learn the neighbor.
> >
> >
> > +  
> > +A priority-95 flow with the match nd_ns 
> > +  (ip6.src == 0 || nd.sll == 0) and applies the action
> > +next;
> > +  
> > +
> >
> >  A priority-90 flow with the match arp and
> >  applies the action
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 033b58b8c..fe97bedad 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -6707,6 +6707,7 @@ AT_CHECK([ovn-sbctl dump-flows DR | grep -e
> lr_in_unsnat -e lr_out_snat -e lr_in
> >
> >  AT_CLEANUP
> >
> > +OVN_FOR_EACH_NORTHD([
>
> This is unrelated.
>

I've moved this change to a separate commit.


>
> >  AT_SETUP([LR NB Static_MAC_Binding table])
> >  ovn_start
> >
> > @@ -6730,6 +6731,7 @@ ovn-nbctl --may-exist static-mac-binding-add
> lr0-p0 192.168.10.100 00:00:22:33:5
> >  wait_row_count Static_MAC_Binding 1 logical_port=lr0-p0
> ip=192.168.10.100 mac="00\:00\:22\:33\:55\:66"
> >
> >  AT_CLEANUP
> > +])
>
> This too.
>

> >
> >  OVN_FOR_EACH_NORTHD([
> >  AT_SETUP([LR neighbor lookup and learning flows])
> > @@ -6751,6 +6753,7 @@ AT_CHECK([cat lrflows | grep -e
> lr_in_lookup_neighbor -e lr_in_learn_neighbor |
> >table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_na),
> action=(put_nd(inport, nd.target, nd.tll); next;)
> >table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_ns),
> action=(put_nd(inport, ip6.src, nd.sll); next;)
> >table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_na &&
> nd.tll == 0), action=(put_nd(inport, nd.target, eth.src); next;)
> > +  table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_ns &&
> (ip6.src == 0 || nd.sll == 0)), action=(next;)
> >  ])
> >
> >  AT_CLEANUP
>
> Thanks,
> Dumitru
>
>
Thanks,
Ales

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.comIM: amusil

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

Re: [ovs-dev] [PATCH 3/6] debian: Update packaging source from Debian/Ubuntu.

[0-day Robot]

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

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


checkpatch:
ERROR: Co-author Luca Boccassi  needs to sign off.
Lines checked: 3737, Warnings: 0, Errors: 1


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

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

[ovs-dev] [PATCH 1/6] debian: Archive debian packaging source.

[Frode Nordahl]

The packaging source in the OVS repository has drifted away from
what is currently in Debian and Ubuntu.  This state is problematic
because from time to time someone tries to build packages from the
upstream OVS debian package source and then expect that package to
work with up-/down-grades from-/to/ distro versions.

To support the on-going work to remove the out of tree OVS kernel
driver from the repository [0], an update to the debian packaging
is also required.  On the back of the discussion in [0] we agreed
that replacing the current version with what Debian and Ubuntu
is currently converging on would be preferable.

This commit is a first in a series to update the upstream OVS
debian packaging source to be up to date with what is currently
in Debian and Ubuntu.

0: https://mail.openvswitch.org/pipermail/ovs-dev/2022-June/394634.html

Signed-off-by: Frode Nordahl 
---
 .github/workflows/build-and-test.yml  |   3 -
 debian/.gitignore |  27 -
 debian/automake.mk|  82 --
 debian/compat |   1 -
 debian/control| 266 ---
 debian/control.modules.in |  20 -
 debian/copyright.in   | 711 --
 debian/dirs   |   2 -
 debian/dkms.conf.in   |  11 -
 debian/ifupdown.sh| 107 ---
 debian/libopenvswitch-dev.install |  19 -
 debian/libopenvswitch.install |   5 -
 debian/openvswitch-common.dirs|   1 -
 debian/openvswitch-common.docs|   0
 debian/openvswitch-common.install |  11 -
 debian/openvswitch-common.manpages|   7 -
 debian/openvswitch-datapath-dkms.postinst |  21 -
 debian/openvswitch-datapath-dkms.prerm|  15 -
 ...atapath-module-_KVERS_.postinst.modules.in |  27 -
 .../openvswitch-datapath-source.README.Debian |  31 -
 debian/openvswitch-datapath-source.copyright  |  15 -
 debian/openvswitch-datapath-source.dirs   |   1 -
 debian/openvswitch-datapath-source.install|   5 -
 debian/openvswitch-ipsec.dirs |   1 -
 debian/openvswitch-ipsec.init | 181 -
 debian/openvswitch-ipsec.install  |   1 -
 debian/openvswitch-pki.dirs   |   1 -
 debian/openvswitch-pki.postinst   |  41 -
 debian/openvswitch-pki.postrm |  43 --
 debian/openvswitch-switch.README.Debian   | 316 
 debian/openvswitch-switch.dirs|   2 -
 debian/openvswitch-switch.init| 147 
 debian/openvswitch-switch.install |  17 -
 debian/openvswitch-switch.links   |   2 -
 debian/openvswitch-switch.logrotate   |  16 -
 debian/openvswitch-switch.manpages|  12 -
 debian/openvswitch-switch.postinst|  60 --
 debian/openvswitch-switch.postrm  |  48 --
 debian/openvswitch-switch.template|   8 -
 debian/openvswitch-test.dirs  |   1 -
 debian/openvswitch-test.install   |   3 -
 debian/openvswitch-test.manpages  |   1 -
 .../openvswitch-testcontroller.README.Debian  |  12 -
 debian/openvswitch-testcontroller.default |  29 -
 debian/openvswitch-testcontroller.dirs|   1 -
 debian/openvswitch-testcontroller.init| 278 ---
 debian/openvswitch-testcontroller.install |   1 -
 debian/openvswitch-testcontroller.manpages|   1 -
 debian/openvswitch-testcontroller.postinst|  52 --
 debian/openvswitch-testcontroller.postrm  |  44 --
 debian/openvswitch-vtep.default   |   4 -
 debian/openvswitch-vtep.dirs  |   1 -
 debian/openvswitch-vtep.init  |  78 --
 debian/openvswitch-vtep.install   |   3 -
 debian/openvswitch-vtep.manpages  |   1 -
 debian/python3-openvswitch.dirs   |   2 -
 debian/python3-openvswitch.install|   1 -
 debian/rules  |  94 ---
 debian/rules.modules  |  39 -
 debian/source/format  |   1 -
 60 files changed, 2931 deletions(-)
 delete mode 100644 debian/.gitignore
 delete mode 100644 debian/automake.mk
 delete mode 100644 debian/compat
 delete mode 100644 debian/control
 delete mode 100644 debian/control.modules.in
 delete mode 100644 debian/copyright.in
 delete mode 100644 debian/dirs
 delete mode 100644 debian/dkms.conf.in
 delete mode 100755 debian/ifupdown.sh
 delete mode 100644 debian/libopenvswitch-dev.install
 delete mode 100644 debian/libopenvswitch.install
 delete mode 100644 debian/openvswitch-common.dirs
 delete mode 100644 debian/openvswitch-common.docs
 delete mode 100644 debian/openvswitch-common.install
 delete mode 100644 debian/openvswitch-common.manpages
 delete mode 100644 debian/openvswitch-datapath-dkms.postinst
 

[ovs-dev] [PATCH 6/6] debian: Fix build of python json C extension.

[Frode Nordahl]

Signed-off-by: Frode Nordahl 
---
 debian/rules | 26 ++
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/debian/rules b/debian/rules
index b9454fa79..62c4fff28 100755
--- a/debian/rules
+++ b/debian/rules
@@ -95,16 +95,26 @@ override_dh_install:
mv $(CURDIR)/debian/openvswitch-switch/usr/sbin/ovs-vswitchd \

$(CURDIR)/debian/openvswitch-switch/usr/lib/openvswitch-switch/ovs-vswitchd
set -e && for pyvers in $(PYTHONS); do \
-   cd python && python$$pyvers setup.py install 
--install-layout=deb \
-   --root $(CURDIR)/debian/python-openvswitch; cd ..; \
+   cd python && \
+   enable_shared=no \
+   CFLAGS=-I$(CURDIR)/debian/tmp/usr/include \
+   LDFLAGS=$(CURDIR)/debian/tmp/usr/lib/libopenvswitch.a \
+   python$$pyvers setup.py install --install-layout=deb \
+   --root $(CURDIR)/debian/python-openvswitch; \
+   cd ..; \
done
set -e && for pyvers in $(PYTHON3S); do \
-cd python && python$$pyvers setup.py install --install-layout=deb \
---root $(CURDIR)/debian/python3-openvswitch; cd ..; \
-mkdir -p 
$(CURDIR)/debian/openvswitch-test/usr/lib/python$$pyvers/dist-packages/ovstest; 
\
-install -v -D python/ovstest/*.py \
-
$(CURDIR)/debian/openvswitch-test/usr/lib/python$$pyvers/dist-packages/ovstest; 
\
-done
+   cd python && \
+   enable_shared=no \
+   CFLAGS=-I$(CURDIR)/debian/tmp/usr/include \
+   LDFLAGS=$(CURDIR)/debian/tmp/usr/lib/libopenvswitch.a \
+   python$$pyvers setup.py install --install-layout=deb \
+   --root $(CURDIR)/debian/python3-openvswitch; \
+   cd ..; \
+   mkdir -p 
$(CURDIR)/debian/openvswitch-test/usr/lib/python$$pyvers/dist-packages/ovstest; 
\
+   install -v -D python/ovstest/*.py \
+   
$(CURDIR)/debian/openvswitch-test/usr/lib/python$$pyvers/dist-packages/ovstest; 
\
+   done
 ifneq (,$(filter i386 amd64 ppc64el arm64, $(DEB_HOST_ARCH)))
install -v -D _dpdk/vswitchd/ovs-vswitchd \
 
$(CURDIR)/debian/openvswitch-switch-dpdk/usr/lib/openvswitch-switch-dpdk/ovs-vswitchd-dpdk
-- 
2.36.1

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

[ovs-dev] [PATCH 4/6] ci: Separate job for debs, ensure built pkg is tested.

[Frode Nordahl]

Use a separate GitHub Actions job for deb test so that we can
control base image for package test.

The CI deb package test code currently attempts to use `apt` to
install local packages.  That may not produce the expected result.

Explicitly install the local packages with `dpkg` after installing
dependencies from `apt` instead.

Also enable test installation of ipsec deb package.

Signed-off-by: Frode Nordahl 
---
 .ci/linux-build.sh   |  21 --
 .github/workflows/build-and-test.yml | 103 +++
 2 files changed, 106 insertions(+), 18 deletions(-)

diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 9963fb810..a8c437aaf 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -207,10 +207,23 @@ if [ "$DEB_PACKAGE" ]; then
 mk-build-deps --install --root-cmd sudo --remove debian/control
 dpkg-checkbuilddeps
 DEB_BUILD_OPTIONS='parallel=4 nocheck' fakeroot debian/rules binary
-# Not trying to install ipsec package as there are issues with system-wide
-# installed python3-openvswitch package and the pyenv used by Travis.
-packages=$(ls $(pwd)/../*.deb | grep -v ipsec)
-sudo apt install ${packages}
+packages=$(ls $(pwd)/../*.deb)
+deps=""
+for pkg in $packages; do
+_ifs=$IFS
+IFS=","
+for dep in $(dpkg-deb -f $pkg Depends); do
+dep_name=$(echo "$dep"|awk '{print$1}')
+# Don't install internal package inter-dependencies from apt
+echo $dep_name | grep -q openvswitch && continue
+deps+=" $dep_name"
+done
+IFS=$_ifs
+done
+# install package dependencies from apt
+echo $deps | xargs sudo apt -y install
+# install the locally built openvswitch packages
+sudo dpkg -i $packages
 exit 0
 fi
 
diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index ec34a9b0d..10ec01295 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -9,13 +9,10 @@ jobs:
 automake libtool gcc bc libjemalloc1 libjemalloc-dev\
 libssl-dev llvm-dev libelf-dev libnuma-dev libpcap-dev  \
 ninja-build selinux-policy-dev
-  deb_dependencies: |
-linux-headers-$(uname -r) build-essential fakeroot devscripts equivs
   AFXDP:   ${{ matrix.afxdp }}
   ASAN:${{ matrix.asan }}
   UBSAN:   ${{ matrix.ubsan }}
   CC:  ${{ matrix.compiler }}
-  DEB_PACKAGE: ${{ matrix.deb_package }}
   DPDK:${{ matrix.dpdk }}
   DPDK_SHARED: ${{ matrix.dpdk_shared }}
   KERNEL:  ${{ matrix.kernel }}
@@ -148,11 +145,7 @@ jobs:
 - name: update APT cache
   run:  sudo apt update || true
 - name: install common dependencies
-  if:   matrix.deb_package == ''
   run:  sudo apt install -y ${{ env.dependencies }}
-- name: install dependencies for debian packages
-  if:   matrix.deb_package != ''
-  run:  sudo apt install -y ${{ env.deb_dependencies }}
 - name: install libunbound libunwind
   if:   matrix.m32 == ''
   run:  sudo apt install -y libunbound-dev libunwind-dev
@@ -163,13 +156,6 @@ jobs:
 - name: build
   run:  ./.ci/linux-build.sh
 
-- name: upload deb packages
-  if:   matrix.deb_package != ''
-  uses: actions/upload-artifact@v2
-  with:
-name: deb-packages
-path: '/home/runner/work/ovs/*.deb'
-
 - name: copy logs on failure
   if: failure() || cancelled()
   run: |
@@ -224,3 +210,92 @@ jobs:
   with:
 name: logs-osx-clang---disable-ssl
 path: config.log
+
+  build-linux-deb:
+env:
+  deb_dependencies: |
+linux-headers-$(uname -r) build-essential fakeroot devscripts equivs
+  AFXDP:   ${{ matrix.afxdp }}
+  ASAN:${{ matrix.asan }}
+  UBSAN:   ${{ matrix.ubsan }}
+  CC:  ${{ matrix.compiler }}
+  DEB_PACKAGE: ${{ matrix.deb_package }}
+  DPDK:${{ matrix.dpdk }}
+  DPDK_SHARED: ${{ matrix.dpdk_shared }}
+  KERNEL:  ${{ matrix.kernel }}
+  KERNEL_LIST: ${{ matrix.kernel_list }}
+  LIBS:${{ matrix.libs }}
+  M32: ${{ matrix.m32 }}
+  OPTS:${{ matrix.opts }}
+  TESTSUITE:   ${{ matrix.testsuite }}
+
+name: linux ${{ join(matrix.*, ' ') }}
+runs-on: ubuntu-22.04
+timeout-minutes: 30
+
+strategy:
+  fail-fast: false
+  matrix:
+include:
+  - compiler: gcc
+deb_package:  deb
+
+steps:
+- name: checkout
+  uses: actions/checkout@v2
+
+- name: update PATH
+  run:  |
+echo "$HOME/bin">> $GITHUB_PATH
+echo "$HOME/.local/bin" >> $GITHUB_PATH
+
+- name: set up python
+  uses: actions/setup-python@v2
+  with:
+python-version: '3.9'
+
+- name: create ci signature file for the dpdk cache key
+  # This will collect most of DPDK related lines, 

[ovs-dev] [PATCH 2/6] checkpatch: Ignore line length and leading whitespace for debian/*.

[Frode Nordahl]

Signed-off-by: Frode Nordahl 
---
 utilities/checkpatch.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 8c02ac3ce..de2420e1f 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -194,12 +194,12 @@ skip_signoff_check = False
 #
 # Python isn't checked as flake8 performs these checks during build.
 line_length_ignore_list = re.compile(
-r'\.(am|at|etc|in|m4|mk|patch|py)$|debian/rules')
+r'\.(am|at|etc|in|m4|mk|patch|py)$|^debian/.*$')
 
 # Don't enforce a requirement that leading whitespace be all spaces on
 # files that include these characters in their name, since these kinds
 # of files need lines with leading tabs.
-leading_whitespace_ignore_list = re.compile(r'\.(mk|am|at)$|debian/rules')
+leading_whitespace_ignore_list = re.compile(r'\.(mk|am|at)$|^debian/.*$')
 
 
 def is_subtracted_line(line):
-- 
2.36.1

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

[ovs-dev] [PATCH 5/6] python: Allow building json C extension with static OVS library.

[Frode Nordahl]

Allow caller of setup.py to pass in libopenvswitch.a as an object
for linking through the use of LDFLAGS environment variable when
not building a shared openvswitch library.

To accomplish this set the `enable_shared` environment variable to
'no'.

Example:
LDFLAGS=lib/libopenvswitch.a enable_shared=no setup.py install

Signed-off-by: Frode Nordahl 
---
 python/setup.py | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/python/setup.py b/python/setup.py
index cfe01763f..b6e88e98e 100644
--- a/python/setup.py
+++ b/python/setup.py
@@ -10,6 +10,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import os
 import sys
 
 from distutils.command.build_ext import build_ext
@@ -63,6 +64,15 @@ class try_build_ext(build_ext):
 raise BuildFailed()
 
 
+# allow caller of setup.py to pass in libopenvswitch.a as an object for linking
+# through the use of LDFLAGS environment variable when not building a shared
+# openvswitch library.
+if 'enable_shared' in os.environ and os.environ['enable_shared'] == 'no':
+json_libraries = []
+else:
+json_libraries = ['openvswitch']
+
+
 setup_args = dict(
 name='ovs',
 description='Open vSwitch library',
@@ -85,7 +95,7 @@ setup_args = dict(
 'Programming Language :: Python :: 3.5',
 ],
 ext_modules=[setuptools.Extension("ovs._json", sources=["ovs/_json.c"],
-  libraries=['openvswitch'])],
+  libraries=json_libraries)],
 cmdclass={'build_ext': try_build_ext},
 install_requires=['sortedcontainers'],
 extras_require={':sys_platform == "win32"': ['pywin32 >= 1.0']},
-- 
2.36.1

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

Re: [ovs-dev] [v5 8/8] dpif-netlink: Offloading meter to tc police action

[Jianbo Liu via dev]

On Tue, 2022-06-28 at 13:18 +0200, Eelco Chaudron wrote:
> 
> 
> On 28 Jun 2022, at 10:52, Jianbo Liu wrote:
> 
> > On Tue, 2022-06-28 at 09:52 +0200, Eelco Chaudron wrote:
> > > 
> > > 
> > > On 27 Jun 2022, at 16:58, Jianbo Liu wrote:
> > > 
> > > > On Mon, 2022-06-27 at 11:32 +0200, Eelco Chaudron wrote:
> > > > > 
> > > > > 
> > > > > On 21 Jun 2022, at 10:22, Jianbo Liu wrote:
> > > > > 
> > > > > > On Mon, 2022-06-20 at 12:18 +0200, Eelco Chaudron wrote:
> > > > > > > On 27 May 2022, at 11:00, Jianbo Liu wrote:
> > > > > > > 
> > > > > > > > OVS meters are created in advance and openflow rules
> > > > > > > > refer
> > > > > > > > to
> > > > > > > > them
> > > > > > > > by
> > > > > > > > their unique ID. New tc_police API is used to offload
> > > > > > > > them.
> > > > > > > > By
> > > > > > > > calling
> > > > > > > > the API, police actions are created and meters are
> > > > > > > > mapped
> > > > > > > > to
> > > > > > > > them.
> > > > > > > > These actions then can be used in tc filter rules by
> > > > > > > > the
> > > > > > > > index.
> > > > > > > > 
> > > > > > > > Signed-off-by: Jianbo Liu 
> > > > > > > > ---
> > > > > > > >  NEWS |  2 ++
> > > > > > > >  lib/dpif-netlink.c   | 31
> > > > > > > > +---
> > > > > > > > -
> > > > > > > >  tests/system-offloads-traffic.at | 48
> > > > > > > > 
> > > > > > > >  3 files changed, 76 insertions(+), 5 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/NEWS b/NEWS
> > > > > > > > index eece0d0b2..dfd659d4e 100644
> > > > > > > > --- a/NEWS
> > > > > > > > +++ b/NEWS
> > > > > > > > @@ -29,6 +29,8 @@ Post-v2.17.0
> > > > > > > >     - Windows:
> > > > > > > >   * Conntrack support for TCPv6, UDPv6, ICMPv6,
> > > > > > > > FTPv6.
> > > > > > > >   * IPv6 Geneve tunnel support.
> > > > > > > > +   - Linux datapath:
> > > > > > > > + * Add offloading meter tc police.
> > > > > > > > 
> > > > > > > > 
> > > > > > > >  v2.17.0 - 17 Feb 2022
> > > > > > > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> > > > > > > > index 06e1e8ca0..0af9ee77e 100644
> > > > > > > > --- a/lib/dpif-netlink.c
> > > > > > > > +++ b/lib/dpif-netlink.c
> > > > > > > > @@ -4163,11 +4163,18 @@ static int
> > > > > > > >  dpif_netlink_meter_set(struct dpif *dpif_,
> > > > > > > > ofproto_meter_id
> > > > > > > > meter_id,
> > > > > > > >     struct ofputil_meter_config
> > > > > > > > *config)
> > > > > > > >  {
> > > > > > > > +    int err;
> > > > > > > > +
> > > > > > > >  if (probe_broken_meters(dpif_)) {
> > > > > > > >  return ENOMEM;
> > > > > > > >  }
> > > > > > > > 
> > > > > > > > -    return dpif_netlink_meter_set__(dpif_, meter_id,
> > > > > > > > config);
> > > > > > > > +    err = dpif_netlink_meter_set__(dpif_, meter_id,
> > > > > > > > config);
> > > > > > > > +    if (!err && netdev_is_flow_api_enabled()) {
> > > > > > > > +    meter_offload_set(meter_id, config);
> > > > > > > 
> > > > > > > Although currently we always return 0, we should still
> > > > > > > account
> > > > > > > for it
> > > > > > > to change in the future, so we should set err to the
> > > > > > > return
> > > > > > > value.
> > > > > > > 
> > > > > > 
> > > > > > If there is err from meter_offload_set, it will be passed
> > > > > > to
> > > > > > the
> > > > > > caller
> > > > > > of dpif_netlink_meter_set(). I don't agree with that,
> > > > > > because
> > > > > > the
> > > > > > caller thinks meter_set operation fail, but actually not.
> > > > > > Besides,
> > > > > > we
> > > > > > allow the case that dp meter_set success, but offloading
> > > > > > fails,
> > > > > > so
> > > > > > the
> > > > > > return the error of meter_offload_set seems unnecessary.
> > > > > 
> > > > > If this is the case, we should change the
> > > > > dpif_netlink_meter_set()
> > > > > API to return void.
> > > > > And add a comment to the function why it would not return an
> > > > > error:
> > > > > 
> > > > 
> > > > OK.
> > > > 
> > > > > --- a/lib/netdev-offload.c
> > > > > +++ b/lib/netdev-offload.c
> > > > > @@ -196,7 +196,7 @@ netdev_assign_flow_api(struct netdev
> > > > > *netdev)
> > > > >  return -1;
> > > > >  }
> > > > > 
> > > > > -int
> > > > > +void
> > > > >  meter_offload_set(ofproto_meter_id meter_id,
> > > > >    struct ofputil_meter_config *config)
> > > > >  {
> > > > > @@ -212,8 +212,8 @@ meter_offload_set(ofproto_meter_id
> > > > > meter_id,
> > > > >     }
> > > > >  }
> > > > >  }
> > > > > -
> > > > > -    return 0;
> > > > > +    /* Offload APIs could fail, for example, because the
> > > > > offload
> > > > > is
> > > > > not
> > > > > + * supported. This is fine, as the offload API should
> > > > > take
> > > > > care
> > > > > of this. */
> > > > >  }
> > > > > 
> > > > > 
> > > > > 
> > > > > > >  +    err = meter_offload_set(meter_id, config);
> > > > > > > 
> > > > > > > > +    }

Re: [ovs-dev] [v5 8/8] dpif-netlink: Offloading meter to tc police action

[Jianbo Liu via dev]

On Tue, 2022-06-28 at 13:15 +0200, Eelco Chaudron wrote:
> 
> 
> On 28 Jun 2022, at 10:28, Jianbo Liu wrote:
> 
> > On Tue, 2022-06-28 at 09:53 +0200, Eelco Chaudron wrote:
> > > 
> > > 
> > > On 27 Jun 2022, at 16:58, Jianbo Liu wrote:
> > > 
> > > > On Mon, 2022-06-27 at 11:32 +0200, Eelco Chaudron wrote:
> > > > > 
> > > > > 
> > > > > On 21 Jun 2022, at 10:22, Jianbo Liu wrote:
> > > > > 
> > > > > > On Mon, 2022-06-20 at 12:18 +0200, Eelco Chaudron wrote:
> > > > > > > On 27 May 2022, at 11:00, Jianbo Liu wrote:
> > > > > > > 
> > > > > > > > OVS meters are created in advance and openflow rules
> > > > > > > > refer
> > > > > > > > to
> > > > > > > > them
> > > > > > > > by
> > > > > > > > their unique ID. New tc_police API is used to offload
> > > > > > > > them.
> > > > > > > > By
> > > > > > > > calling
> > > > > > > > the API, police actions are created and meters are
> > > > > > > > mapped
> > > > > > > > to
> > > > > > > > them.
> > > > > > > > These actions then can be used in tc filter rules by
> > > > > > > > the
> > > > > > > > index.
> > > > > > > > 
> > > > > > > > Signed-off-by: Jianbo Liu 
> > > > > > > > ---
> > > > > > > >  NEWS |  2 ++
> > > > > > > >  lib/dpif-netlink.c   | 31
> > > > > > > > +---
> > > > > > > > -
> > > > > > > >  tests/system-offloads-traffic.at | 48
> > > > > > > > 
> > > > > > > >  3 files changed, 76 insertions(+), 5 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/NEWS b/NEWS
> > > > > > > > index eece0d0b2..dfd659d4e 100644
> > > > > > > > --- a/NEWS
> > > > > > > > +++ b/NEWS
> > > > > > > > @@ -29,6 +29,8 @@ Post-v2.17.0
> > > > > > > >     - Windows:
> > > > > > > >   * Conntrack support for TCPv6, UDPv6, ICMPv6,
> > > > > > > > FTPv6.
> > > > > > > >   * IPv6 Geneve tunnel support.
> > > > > > > > +   - Linux datapath:
> > > > > > > > + * Add offloading meter tc police.
> > > > > > > > 
> > > > > > > > 
> > > > > > > >  v2.17.0 - 17 Feb 2022
> > > > > > > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> > > > > > > > index 06e1e8ca0..0af9ee77e 100644
> > > > > > > > --- a/lib/dpif-netlink.c
> > > > > > > > +++ b/lib/dpif-netlink.c
> > > > > > > > @@ -4163,11 +4163,18 @@ static int
> > > > > > > >  dpif_netlink_meter_set(struct dpif *dpif_,
> > > > > > > > ofproto_meter_id
> > > > > > > > meter_id,
> > > > > > > >     struct ofputil_meter_config
> > > > > > > > *config)
> > > > > > > >  {
> > > > > > > > +    int err;
> > > > > > > > +
> > > > > > > >  if (probe_broken_meters(dpif_)) {
> > > > > > > >  return ENOMEM;
> > > > > > > >  }
> > > > > > > > 
> > > > > > > > -    return dpif_netlink_meter_set__(dpif_, meter_id,
> > > > > > > > config);
> > > > > > > > +    err = dpif_netlink_meter_set__(dpif_, meter_id,
> > > > > > > > config);
> > > > > > > > +    if (!err && netdev_is_flow_api_enabled()) {
> > > > > > > > +    meter_offload_set(meter_id, config);
> > > > > > > 
> > > > > > > Although currently we always return 0, we should still
> > > > > > > account
> > > > > > > for it
> > > > > > > to change in the future, so we should set err to the
> > > > > > > return
> > > > > > > value.
> > > > > > > 
> > > > > > 
> > > > > > If there is err from meter_offload_set, it will be passed
> > > > > > to
> > > > > > the
> > > > > > caller
> > > > > > of dpif_netlink_meter_set(). I don't agree with that,
> > > > > > because
> > > > > > the
> > > > > > caller thinks meter_set operation fail, but actually not.
> > > > > > Besides,
> > > > > > we
> > > > > > allow the case that dp meter_set success, but offloading
> > > > > > fails,
> > > > > > so
> > > > > > the
> > > > > > return the error of meter_offload_set seems unnecessary.
> > > > > 
> > > > > If this is the case, we should change the
> > > > > dpif_netlink_meter_set()
> > > > > API to return void.
> > > > > And add a comment to the function why it would not return an
> > > > > error:
> > > > > 
> > > > 
> > > > OK.
> > > > 
> > > > > --- a/lib/netdev-offload.c
> > > > > +++ b/lib/netdev-offload.c
> > > > > @@ -196,7 +196,7 @@ netdev_assign_flow_api(struct netdev
> > > > > *netdev)
> > > > >  return -1;
> > > > >  }
> > > > > 
> > > > > -int
> > > > > +void
> > > > >  meter_offload_set(ofproto_meter_id meter_id,
> > > > >    struct ofputil_meter_config *config)
> > > > >  {
> > > > > @@ -212,8 +212,8 @@ meter_offload_set(ofproto_meter_id
> > > > > meter_id,
> > > > >     }
> > > > >  }
> > > > >  }
> > > > > -
> > > > > -    return 0;
> > > > > +    /* Offload APIs could fail, for example, because the
> > > > > offload
> > > > > is
> > > > > not
> > > > > + * supported. This is fine, as the offload API should
> > > > > take
> > > > > care
> > > > > of this. */
> > > > >  }
> > > > > 
> > > > > 
> > > > > 
> > > > > > >  +    err = meter_offload_set(meter_id, config);
> > > > > > > 
> > > > > > > > +    }

Re: [ovs-dev] [PATCH ovn] patch.c: Avoid patch interface deletion & recreation during restart.

[Han Zhou]

On Tue, Jun 28, 2022 at 8:20 AM Dumitru Ceara  wrote:
>
> On 6/28/22 02:49, Han Zhou wrote:
> > When ovn-controller is restarted, it may need multiple iterations of
> > main loop before completely download all related data from SB DB,
> > especially when ovn-monitor-all=false, so after restart, before it sees
> > the related localnet ports from SB DB, it treats the related patch
> > ports on the chassis as not needed, and deleted them. Later when it
> > downloads thoses port-bindings it recreates them.  For a graceful
> > upgrade, we don't this to happen, because it would break the traffic.
> >
> > This is especially problematic at ovn-k8s setups because the external
> > side of the patch port is used to program some flows for external
> > network communication. When the patch ports are recreated, the OF port
> > number changes and those flows are broken. The CMS would detect and fix
> > the flows but it would result in even longer downtime.
> >
> > This patch postpone the deletion of the patch ports, with the assumption
> > that leaving the unused ports on the chassis for little longer is not
> > harmful.
> >
> > Signed-off-by: Han Zhou 
> > ---
> >  controller/patch.c  | 15 -
> >  tests/ovn-controller.at | 71 +
> >  2 files changed, 85 insertions(+), 1 deletion(-)
> >
> > diff --git a/controller/patch.c b/controller/patch.c
> > index ed831302c..9faae5879 100644
> > --- a/controller/patch.c
> > +++ b/controller/patch.c
> > @@ -307,11 +307,24 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
> >
> >  /* Now 'existing_ports' only still contains patch ports that exist
in the
> >   * database but shouldn't.  Delete them from the database. */
> > +
> > +/* Wait for some iterations before really deleting any patch
ports, because
> > + * with conditional monitoring it is possible that related SB data
is not
> > + * completely downloaded yet after last restart of ovn-controller.
> > + * Otherwise it may cause unncessary dataplane interruption during
> > + * restart/upgrade. */
> > +static int delete_patch_port_delay = 10;
>
> Hi Han,
>
> It's possible that ovn-controller wakes up 10 times in a row immediately
> due to local OVSDB changes and doesn't process any new SB updates in
> that time so 10 might not be enough in some cases.
>

Thanks Dumitru for the review!
In theory I agree with you 10 times is not 100% guaranteeing SB update
completed if other things are triggering the wakeups. However, in practice,
the purpose here is for the start/restart scenario. I think it is very
unlikely that local OVSDB is changing that frequently at the same time when
ovn-controller is being restarted. We have some similar logic (not ideal
for sure) at vif-plug.c:vif_plug_run() for similar reasons.

> Does it make sense to wait a given amount of time instead?  E.g., a few
> seconds?  Can we make this configurable somehow?  Maybe an
> ovn-controller command line argument to override the default?

Waiting for a given amount of time is also not ideal. It is possible that
when ovn-controller starts the SB IDL is not connected (due to server side
problems/ control plane network problems, etc.) so we don't know how long
it should wait at all.

I am ok with adding command line arguments to adjust, but I'd really want
to avoid that unless it is proved to be necessary. I'd rather use a bigger
hardcoded value to avoid another knob which is not easy to understand by
users - it should be something handled by the code itself.

Thanks,
Han

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

Re: [ovs-dev] [PATCH] python: Add Python bindings TODO file.

[Ilya Maximets]

On 6/27/22 17:27, Terry Wilson wrote:
> On Mon, Jun 27, 2022 at 5:44 AM Dumitru Ceara  wrote:
>>
>> For now include the IDL related TODO items as discussed at:
>> https://mail.openvswitch.org/pipermail/ovs-dev/2022-April/393516.html
>>
>> Signed-off-by: Dumitru Ceara 
>> ---
>>  python/TODO.rst| 34 ++
>>  python/automake.mk |  2 ++
>>  2 files changed, 36 insertions(+)
>>  create mode 100644 python/TODO.rst
> 
> Acked-by: Terry Wilson 

Applied.  Thanks!

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

Re: [ovs-dev] [PATCH v4] ovsdb-server: Log database transactions for user requested tables.

[Ilya Maximets]

On 6/24/22 11:55, Dumitru Ceara wrote:
> Add a new command, 'ovsdb-server/tlog-set DB:TABLE on|off', which
> allows the user to enable/disable transaction logging for specific
> databases and tables.
> 
> By default, logging is disabled.  Once enabled, logs are generated
> with level INFO and are also rate limited.
> 
> If used with care, this command can be useful in analyzing production
> deployment performance issues, allowing the user to pin point
> bottlenecks without the need to enable wider debug logs, e.g., jsonrpc.
> 
> A command to inspect the logging state is also added:
> 'ovsdb-server/tlog-list'.
> 
> Signed-off-by: Dumitru Ceara 
> ---
> A sample use case is an ovn-kubernetes scaled deployment in which
> we're interesting in reducing time to bring up PODs (represented by
> OVN logical switch ports).  In order to determine exactly where the
> bottleneck is when provisioning PODs (CMS/ovn-nbctl/client
> IDLs/ovsdb-server/ovn-controller/etc) we need timestamps of when
> operations happen at various places in the stack.
> 
> Without this patch the only option for tracking when transactions
> happen in the Northbound database is to enable jsonrpc debug logs in
> ovsdb-server.  This generates a rather large amount of data.
> 
> Instead, now, users would be able to just enable logging for the
> Logical_Switch_Port table getting more relevant and precise
> information.
> 
> V4:
> - Fixed usage string of ovsdb-server/tlog-list (it was hiding the
>   command).
> - addressed Ilya's comments:
>   - case insensitive on/off check
>   - ds_put_cstr()/_char() where applicable
>   - renamed some of the functions
>   - simplified log rate limit check
> V3:
> - rebased
> - addressed Mark Gray's comments
> V2:
> - rebased (fixed conflicts in NEWS).
> ---
>  NEWS|  3 ++
>  ovsdb/ovsdb-server.1.in |  7 
>  ovsdb/ovsdb-server.c| 87 +
>  ovsdb/row.c | 17 
>  ovsdb/row.h |  1 +
>  ovsdb/table.c   | 13 ++
>  ovsdb/table.h   |  4 ++
>  ovsdb/transaction.c | 43 
>  8 files changed, 175 insertions(+)
> 

Applied.  Thanks!

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

Re: [ovs-dev] [PATCH v2] ovsdb-idl: Get per-database memory usage statistics.

[Ilya Maximets]

On 6/24/22 09:48, Dumitru Ceara wrote:
> Clients might be connected to multiple databases (e.g., ovn-controller
> is connected to OVN_Southbound and Open_vSwitch databases) and the IDL
> memory statistics are more useful if they're not aggregated.
> 
> Signed-off-by: Dumitru Ceara 
> ---
> V2:
> - Addressed Ilya's comment and swapped database name and counter name.
> ---
>  lib/ovsdb-idl.c | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)

Applied.  Thanks!

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

Re: [ovs-dev] [PATCH] dpif-netdev-extract-avx512: Protect GCC builtin usage.

[Ilya Maximets]

On 6/21/22 17:52, Cian Ferriter wrote:
> __builtin_constant_p is only available in GCC and only versions >= 4.
> Use the same "#if __GNUC__ >= 4" check used in other parts of OVS for
> this builtin.
> 
> Signed-off-by: Cian Ferriter 
> ---
>  lib/dpif-netdev-extract-avx512.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/dpif-netdev-extract-avx512.c 
> b/lib/dpif-netdev-extract-avx512.c
> index 12271be17..f1919befd 100644
> --- a/lib/dpif-netdev-extract-avx512.c
> +++ b/lib/dpif-netdev-extract-avx512.c
> @@ -128,7 +128,11 @@ _mm512_maskz_permutexvar_epi8_selector(__mmask64 k_shuf, 
> __m512i v_shuf,
>  __m512i v512_zeros = _mm512_setzero_si512();
>  __m512i v_blk0;
>  #if HAVE_AVX512VBMI
> +#if __GNUC__ >= 4
>  if (__builtin_constant_p(use_vbmi) && use_vbmi) {
> +#else
> +if (use_vbmi) {
> +#endif
>  /* As different AVX512 ISA levels have different implementations,
>  * this specializes on the use_vbmi attribute passed in.
>  */

Thanks!  Applied and backported down to 2.16.

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

Re: [ovs-dev] [PATCH v2] ovs-tcpdump: Default to OVS_RUNDIR if present.

[Ilya Maximets]

On 6/20/22 22:08, Mike Pattrick wrote:
> Now ovs-tcpdump will check for an OVS_RUNDIR environment variable and
> if present, use it instead of the default RUNDIR. This is useful when
> used in conjunction with OVS_PAUSE_TEST while running the test suite.
> 
> Signed-off-by: Mike Pattrick 
> 
> --
> V2: Removed unnecessary conditional
> 
> Signed-off-by: Mike Pattrick 
> ---
>  utilities/ovs-tcpdump.in | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
> index b3dc65440..7fd26e405 100755
> --- a/utilities/ovs-tcpdump.in
> +++ b/utilities/ovs-tcpdump.in
> @@ -406,7 +406,8 @@ def py_which(executable):
>  
>  
>  def main():
> -db_sock = 'unix:@RUNDIR@/db.sock'
> +rundir = os.environ.get('OVS_RUNDIR', '@RUNDIR@')
> +db_sock = 'unix:%s' % os.path.join(rundir, "db.sock")
>  interface = None
>  tcpdargs = []
>  

Thanks!  Applied to master and branch-2.17.

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

Re: [ovs-dev] [PATCH v2] dpcls: add unlisted alias for subtable lookup command

[Ilya Maximets]

On 6/9/22 10:17, Eelco Chaudron wrote:
> 
> 
> On 7 Jun 2022, at 12:36, Harry van Haaren wrote:
> 
>> This patch adds the old name "subtable-lookup-prio-get" as an unlisted 
>> command,
>> to restore a consistency between OVS releases for testing scripts.
>>
>> Fixes 738c76a503f4 ("dpcls: Change info-get function to fetch dpcls usage 
>> stats.")

There is a missing colon here.
Also, the NEWS file update is missing.

I added both and applied the patch.

Best regards, Ilya Maximets.

>> Suggested-by: Eelco Chaudron 
>> Suggested-by: Ilya Maximets 
>> Signed-off-by: Harry van Haaren 
> 
> Thanks for the v2, it works as expected :)
> 
> Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH 1/1] ovsdb: Fix memory leak on error path in ovsdb_file_read__().

[Ilya Maximets]

On 6/17/22 18:19, Mike Pattrick wrote:
> On Mon, Jun 6, 2022 at 7:07 AM Yunjian Wang via dev
>  wrote:
>>
>> Found by Coverity.
>>
>> Fixes: 1b1d2e6daa56 ("ovsdb: Introduce experimental support for clustered 
>> databases.")
>> Signed-off-by: Yunjian Wang 
> 
> Looks correct to me!
> 
> Acked-by: Mike Pattrick 
> 

Thanks!  Applied and backported down to 2.13.

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

Re: [ovs-dev] [PATCH] odp-util: Ignore unknown attributes in parse_key_and_mask_to_match()

[Ilya Maximets]

On 5/31/22 16:34, Eelco Chaudron wrote:
> When processing netlink messages, we should ignore unknown OVS_KEY_ATTR
> as we can assume if newer attributes are present, they are backward
> compatible.
> 
> This patch also updates the existing comments to better explain what
> happens in the error cases. At this place in the code, we can not
> ignore the TOO_LITTLE/MUCH errors as some instances could have
> canceled processing leaving the returning match data incomplete.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2089331
> Signed-off-by: Eelco Chaudron 
> ---
>  lib/odp-util.c |   34 +-
>  1 file changed, 25 insertions(+), 9 deletions(-)

Thanks, Eelco, Roi and Michael!  Applied and backported down to 2.13.

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

Re: [ovs-dev] [PATCH v2 2/2] ofproto-dpif: avoid unneccesary backer revalidation

[Ilya Maximets]

On 6/8/22 19:14, Paolo Valerio wrote:
> lic121  writes:
> 
>> If lldp didn't change, we are not supposed to trigger backer
>> revalidation.
>> Without this patch, bridge_reconfigure() always trigger udpif
>> revalidator because of lldp.
>>
>> Signed-off-by: lic121 
>> Signed-off-by: Eelco Chaudron 
>> Co-authored-by: Eelco Chaudron 
>> ---
> 
> LGTM,
> 
> Acked-by: Paolo Valerio 

Thanks!  Applied to master and 2.17.

Best regards, Ilya Maximets.

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

Re: [ovs-dev] [PATCH v2 1/2] lldp: fix lldp memory leak

[Ilya Maximets]

On 6/8/22 19:14, Paolo Valerio wrote:
> lic121  writes:
> 
>> lldp_create() malloc memory for lldp->lldpd->g_hardware. lldp_unref
>> is supposed to free the memory regardless of hw->h_flags.
>>
>> Signed-off-by: lic121 
>> Acked-by: Eelco Chaudron 
>> ---
>>  lib/lldp/lldpd.c | 10 +++---
>>  1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/lldp/lldpd.c b/lib/lldp/lldpd.c
>> index 403f1f5..4bff7b0 100644
>> --- a/lib/lldp/lldpd.c
>> +++ b/lib/lldp/lldpd.c
>> @@ -140,13 +140,9 @@ lldpd_cleanup(struct lldpd *cfg)
>>  VLOG_DBG("cleanup all ports");
>>  
>>  LIST_FOR_EACH_SAFE (hw, h_entries, >g_hardware) {
>> -if (!hw->h_flags) {
>> -ovs_list_remove(>h_entries);
>> -lldpd_remote_cleanup(hw, NULL, true);
>> -lldpd_hardware_cleanup(cfg, hw);
>> -} else {
>> -lldpd_remote_cleanup(hw, NULL, false);
>> -}
>> +ovs_list_remove(>h_entries);
>> +lldpd_remote_cleanup(hw, NULL, true);
>> +lldpd_hardware_cleanup(cfg, hw);
>>  }
>>  
>>  VLOG_DBG("cleanup all chassis");
> 
> 
> Acked-by: Paolo Valerio 

Thanks!  Applied and backported down to 2.13.

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

Re: [ovs-dev] [PATCH v2] ipfix: trigger revalidation if ipfix options changes

[Ilya Maximets]

On 5/9/22 11:45, Eelco Chaudron wrote:
> 
> 
> On 30 Apr 2022, at 6:47, lic121 wrote:
> 
>> ipfix cfg creation/deleting triggers revalidation. But this does
>> not cover the case where ipfix options changes, which also suppose
>> to trigger revalidation.
>>
>> Fixes: a9f5ee1199e1 ("ofproto-dpif: Trigger revalidation when ipfix config 
>> set.")
>> Signed-off-by: lic121 
>> ---
> 
> Changes look good to me, basic testing is also passing.
> 
> Acked-by: Eelco Chaudron 

Thanks!  Applied and backported down to 2.13.

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

Re: [ovs-dev] [PATCH v5] dpif-netdev: Allow cross-NUMA polling on selected ports

[Kevin Traynor]

On 27/06/2022 05:21, Anurag Agarwal wrote:

Hello Kevin,


-Original Message-
From: Kevin Traynor 
Sent: Thursday, June 23, 2022 9:07 PM
To: Anurag Agarwal ; ovs-dev@openvswitch.org
Cc: lic...@chinatelecom.cn
Subject: Re: [ovs-dev] [PATCH v5] dpif-netdev: Allow cross-NUMA polling on
selected ports

Hi Anurag,

On 23/06/2022 11:18, Anurag Agarwal wrote:

From: Jan Scheurich

Today dpif-netdev considers PMD threads on a non-local NUMA node for
automatic assignment of the rxqs of a port only if there are no local,non-

isolated PMDs.


On typical servers with both physical ports on one NUMA node, this
often leaves the PMDs on the other NUMA node under-utilized, wasting CPU

resources.

The alternative, to manually pin the rxqs to PMDs on remote NUMA
nodes, also has drawbacks as it limits OVS' ability to auto load-balance the

rxqs.


This patch introduces a new interface configuration option to allow
ports to be automatically polled by PMDs on any NUMA node:

ovs-vsctl set interface  other_config:cross-numa-polling=true

The group assignment algorithm now has the ability to select lowest
loaded PMD on any NUMA, and not just the local NUMA on which the rxq
of the port resides

If this option is not present or set to false, legacy behaviour applies.

Co-authored-by: Anurag Agarwal
Signed-off-by: Jan Scheurich
Signed-off-by: Anurag Agarwal
---
Changes in v5:
- Addressed comments from
- First schedule rxqs that are not enabled for cross-numa scheduling
- Follow this with rxqs that are enabled for cross-numa scheduling



I don't think this is a correct fix for the issue reported. The root problem
reported is not really that rxqs with cross-numa=true are assigned first, but 
that
the pool or pmd resources is changing/overlapping during the assignments i.e. in
the reported case from a full pool to a fixed per-numa pool.

With the change you have now, you could have something like:
3 rxqs, (1 cross-numa) and 2 pmds.

cross-numa=true rxq load: 80%
per-numa rxq loads: 45%, 40%

rxq assignment rounds
1.
pmd0 = 45
pmd1 = 0

2.
pmd0 = 45
pmd1 = 40

3.
pmd0 = 45  = 40%
pmd1 = 40 + 80 = 120%

when clearly the best way is:
pmd0 = 80
pmd1 = 45 + 40



Could you help elaborate on this a bit more. Is PMD0 on NUMA0 and PMD1 on 
NUMA1? The two per-numa rxqs(45%, 40%) they belong to which NUMA?
Need some more details to understand this scenario.



pmd0 and pmd1 belong to NUMA 0. rxq 45% and rxq 40% belong to NUMA0.

To simplify above, I've shown 2 rxq and 2 pmd on NUMA0 but that could be 
replicated on other NUMAs as too.


rxq 80% cross-numa could belong to any NUMA.


So it's not about which ones gets assigned first, as is shown that can cause an
issue whichever one is assigned first. The problem is that the algorithm for
assignment is not designed for changing and overlapping ranges of pmds to
assign rxqs to.


Here is the comment from lic...@chinatelecom.cn:

It may be better to schedule non-cross-numa rxqs first, and then
cross-numa rxqs.
Otherwise, one numa may hold much more load because of later
scheduled non- cross-numa rxqs.


And here is the example shared:


Considering the following situation:

We have 2 numa nodes, each numa node has 1 pmd.
And we have 2 port(p0, p1), each port has 2 queues.
p0 is configured as cross_numa, p1 is not.

each queue's workload,

rxq   p0q0 p0q1 p1q0 p1q1
load  30   30   20   20

Based on your current implement, the assignment will be:

p0q0 -> numa0 (30)
p0q1 -> numa1 (30)
p1q0 -> numa0 (30+20=50)
p1q1 -> numa0 (50+20=70)

As the result, numa0 holds 70% workload but numa1 holds only 30%.
Because later assigned queues are numa affinity.





Yes, I understand the comment and it highlights a potential issue. My 
concern is that your solution only worksaround that issue, but causes 
other potential issues (like in the example i gave) because it does not 
fix the root cause.




To fix this, you would probably need to do all the assignments first as per v4 
and
then do another round of checking and possibly moving some cross-numa=true
rxqs. But that is further relying on estimates which you are making potentially
inaccurate. If you are writing something to "move" individual rxqs after initial
assignment, maybe it's better to rethink doing it in the ALB with the real loads
and not estimates.

It is worth noting again that while less flexible, if the rxq load was 
distributed on
an interface with RSS etc, some pinning of phy rxqs can allow cross-numa and
remove any inaccurate estimates from changing numa.

Do you mean pinning of phy rxqs can also be used alternatively to achieve 
cross-numa equivalent functionality although this is less flexible?



Yes, the user can pin rxqs to any NUMA, but OVS will not then reassign 
them. It will however, consider the load from them when placing other 
rxqs it can assign. I think the pros and cons were discussed in previous 
threads related to these patches.



If scheduling cross-numa queues along with per numa queues leads to inaccurate 

Re: [ovs-dev] [PATCH ovsdb v3 1/1] Add Local_Config schema

[Dumitru Ceara]

On 6/28/22 16:54, Terry Wilson wrote:
> The only way to configure settings on a remote (e.g. inactivity_probe)
> is via --remote=db:DB,table,row. There is no way to do this via the
> existing CLI options.
> 
> For a clustered DB with multiple servers listening on unique addresses
> there is no way to store these entries in the DB as the DB is shared.
> For example, three servers listening on  1.1.1.1, 1.1.1.2, and 1.1.1.3
> respectively would require a Manager/Connection row each, but then
> all three servers would try to listen on all three addresses.
> 
> It is possible for ovsdb-server to serve multiple databases. This
> means that we can have a local "config" database in addition to
> the main database we are servering (Open_vSwitch, OVN_Southbound, etc.)
> and this patch adds a Local_Config schema that currently just mirrors
> the Connection table and a Config table with a 'connections' row that
> stores each Connection.
> 
> Signed-off-by: Terry Wilson 
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara 

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

Re: [ovs-dev] [PATCH ovn v3 1/1] Allow arbitrary args to be passed to called binary

[Terry Wilson]

On Tue, Jun 28, 2022 at 4:39 AM Dumitru Ceara  wrote:
>
> On 6/27/22 18:00, Terry Wilson wrote:
> > It is common to pass ovn-ctl options via an /etc/sysconfig file.
> > It would be useful to be able to pass custom --remote options or
> > additional DBs to listen to via this file. This would give end
> > users the ability to have more fine-grained control without
> > having to modify ovn-ctl.
> >
> > Signed-off-by: Terry Wilson 
> > ---
>
> Hi Terry,
>
> For the change itself:
>
> Acked-by: Dumitru Ceara 

Thanks!

> While we can make it work for now with this new arbitrary args feature,
> for the actual goal, of passing a custom --remote option and additional
> DBs to listen to I think we need a follow up.  Currently, when starting
> OVN databases, if the database doesn't exist ovn-ctl takes care of
> creating it:
>
> https://github.com/ovn-org/ovn/blob/86684ad759b4f517b8f34fbf7ba8bb8ed77bb575/utilities/ovn-ctl#L246
> (raft)
>
> https://github.com/ovn-org/ovn/blob/86684ad759b4f517b8f34fbf7ba8bb8ed77bb575/utilities/ovn-ctl#L249
> (standalone)
>
> In the future, after [0] is accepted, can we add a patch that changes
> ovn-ctl to check if the local_config.ovsschema file is installed?  If it
> is and if DB_*_USE_REMOTE_IN_DB=yes it could then create the
> corresponding local_config database if not already found in $ovn_dbdir
> and automatically add the --remote and new db file args.
>
> What do you think?

Something like that sounds good to me. With this I was looking for
something that would not specifically require the ovs change, but
would still let me solve the problem if I needed to pass the
schema/create the db locally. And in general it's useful to be able to
pass through some options.

I hadn't originally planned to use Local_Config just because it
existed--I can imagine situations where you have a manually added
Connection or custom set things like inactivity_probe set up in the
main DB, and if ovn-ctl now notices you have a Local_Config schema, it
stops using the original table and you lose whatever was in the
original db. So I was thinking of an additional --use-local-config
kind of option to opt into using it.

> Thanks,
> Dumitru
>
> >  utilities/ovn-ctl   | 22 ++
> >  utilities/ovn-ctl.8.xml | 14 +-
> >  2 files changed, 35 insertions(+), 1 deletion(-)
> >
> > diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl
> > index 23d4d7f8c..93be9b84b 100755
> > --- a/utilities/ovn-ctl
> > +++ b/utilities/ovn-ctl
> > @@ -311,6 +311,10 @@ $cluster_remote_port
> >  set "$@" --sync-from=`cat $active_conf_file`
> >  fi
> >
> > +if test X"$extra_args" != X; then
> > +set "$@" $extra_args
> > +fi
> > +
> >  local run_ovsdb_in_bg="no"
> >  local process_id=
> >  if test X$detach = Xno && test $mode = cluster && test -z 
> > "$cluster_remote_addr" ; then
> > @@ -541,6 +545,10 @@ start_ic () {
> >
> >  set "$@" $OVN_IC_LOG $ovn_ic_params
> >
> > +if test X"$extra_args" != X; then
> > +set "$@" $extra_args
> > +fi
> > +
> >  OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_IC_PRIORITY" 
> > "$OVN_IC_WRAPPER" "$@"
> >  fi
> >  }
> > @@ -563,6 +571,10 @@ start_controller () {
> >
> >  [ "$OVN_USER" != "" ] && set "$@" --user "$OVN_USER"
> >
> > +if test X"$extra_args" != X; then
> > +set "$@" $extra_args
> > +fi
> > +
> >  OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_CONTROLLER_PRIORITY" 
> > "$OVN_CONTROLLER_WRAPPER" "$@"
> >  }
> >
> > @@ -590,6 +602,10 @@ start_controller_vtep () {
> >
> >  [ "$OVN_USER" != "" ] && set "$@" --user "$OVN_USER"
> >
> > +if test X"$extra_args" != X; then
> > +set "$@" $extra_args
> > +fi
> > +
> >  OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_CONTROLLER_PRIORITY" 
> > "$OVN_CONTROLLER_WRAPPER" "$@"
> >  }
> >
> > @@ -1106,8 +1122,10 @@ EOF
> >
> >  set_defaults
> >  command=
> > +extra_args=
> >  for arg
> >  do
> > +shift
> >  case $arg in
> >  -h | --help)
> >  usage
> > @@ -1130,6 +1148,10 @@ do
> >  type=bool
> >  set_option
> >  ;;
> > +--)
> > +extra_args=$@
> > +break
> > +;;
> >  -*)
> >  echo >&2 "$0: unknown option \"$arg\" (use --help for help)"
> >  exit 1
> > diff --git a/utilities/ovn-ctl.8.xml b/utilities/ovn-ctl.8.xml
> > index a1d39b22b..42d16fabc 100644
> > --- a/utilities/ovn-ctl.8.xml
> > +++ b/utilities/ovn-ctl.8.xml
> > @@ -4,7 +4,10 @@
> >  ovn-ctl -- Open Virtual Network northbound daemon lifecycle 
> > utility
> >
> >  Synopsis
> > -ovn-ctl [options] command
> > +
> > +  ovn-ctl [options] command
> > +  [--- extra_args]
> > +
> >
> >  Description
> >  This program is intended to be invoked internally by Open Virtual 
> > Network
> > @@ -156,6 +159,15 @@
> >  --db-nb-probe-interval-to-active=Time in 
> > 

Re: [ovs-dev] [PATCH ovn] patch.c: Avoid patch interface deletion & recreation during restart.

[Dumitru Ceara]

On 6/28/22 02:49, Han Zhou wrote:
> When ovn-controller is restarted, it may need multiple iterations of
> main loop before completely download all related data from SB DB,
> especially when ovn-monitor-all=false, so after restart, before it sees
> the related localnet ports from SB DB, it treats the related patch
> ports on the chassis as not needed, and deleted them. Later when it
> downloads thoses port-bindings it recreates them.  For a graceful
> upgrade, we don't this to happen, because it would break the traffic.
> 
> This is especially problematic at ovn-k8s setups because the external
> side of the patch port is used to program some flows for external
> network communication. When the patch ports are recreated, the OF port
> number changes and those flows are broken. The CMS would detect and fix
> the flows but it would result in even longer downtime.
> 
> This patch postpone the deletion of the patch ports, with the assumption
> that leaving the unused ports on the chassis for little longer is not
> harmful.
> 
> Signed-off-by: Han Zhou 
> ---
>  controller/patch.c  | 15 -
>  tests/ovn-controller.at | 71 +
>  2 files changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/controller/patch.c b/controller/patch.c
> index ed831302c..9faae5879 100644
> --- a/controller/patch.c
> +++ b/controller/patch.c
> @@ -307,11 +307,24 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
>  
>  /* Now 'existing_ports' only still contains patch ports that exist in the
>   * database but shouldn't.  Delete them from the database. */
> +
> +/* Wait for some iterations before really deleting any patch ports, 
> because
> + * with conditional monitoring it is possible that related SB data is not
> + * completely downloaded yet after last restart of ovn-controller.
> + * Otherwise it may cause unncessary dataplane interruption during
> + * restart/upgrade. */
> +static int delete_patch_port_delay = 10;

Hi Han,

It's possible that ovn-controller wakes up 10 times in a row immediately
due to local OVSDB changes and doesn't process any new SB updates in
that time so 10 might not be enough in some cases.

Does it make sense to wait a given amount of time instead?  E.g., a few
seconds?  Can we make this configurable somehow?  Maybe an
ovn-controller command line argument to override the default?

Thanks,
Dumitru


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

Re: [ovs-dev] [PATCH] tests: Add OVS-DPDK QoS unit tests

[Phelan, Michael]

> -Original Message-
> From: Stokes, Ian 
> Sent: Tuesday 28 June 2022 15:46
> To: Phelan, Michael ; d...@openvswitch.org
> Cc: acon...@redhat.com; i.maxim...@ovn.org; maxime.coque...@redhat.com
> Subject: RE: [PATCH] tests: Add OVS-DPDK QoS unit tests
> 
> 
> 
> > -Original Message-
> > From: Phelan, Michael 
> > Sent: Tuesday, June 21, 2022 11:19 AM
> > To: d...@openvswitch.org
> > Cc: Stokes, Ian ; acon...@redhat.com;
> > i.maxim...@ovn.org; maxime.coque...@redhat.com; Phelan, Michael
> > 
> > Subject: [PATCH] tests: Add OVS-DPDK QoS unit tests
> >
> > This adds 4 new unit tests to the 'check-dpdk' subsystem that will
> > test Quality of Service (QoS) functionality.
> >
> 
> Hi Michael,
> 
> This is quite similar to the rate limiting patch but there are a few comments 
> here
> that needs addressing.
> 
> 
> > Signed-off-by: Michael Phelan 
> > ---
> >  tests/system-dpdk.at | 141
> > +++
> >  1 file changed, 141 insertions(+)
> >
> > diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at index
> > 7d2715c4a..90e85e8a9 100644
> > --- a/tests/system-dpdk.at
> > +++ b/tests/system-dpdk.at
> > @@ -222,6 +222,147 @@ OVS_VSWITCHD_STOP("m4_join([],
> > [SYSTEM_DPDK_ALLOWED_LOGS], [  AT_CLEANUP  dnl
> > --
> > 
> >
> > +
> > +
> > +dnl
> > +-
> > +-
> > +dnl QoS create delete phy port
> > +AT_SETUP([OVS-DPDK - QoS create delete phy port])
> > +AT_KEYWORDS([dpdk])
> > +
> > +OVS_DPDK_PRE_PHY_SKIP()
> > +OVS_DPDK_START()
> > +
> > +dnl Add userspace bridge and attach it to OVS and add egress policer
> > +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> > datapath_type=netdev])
> > +AT_CHECK([ovs-vsctl add-port br10 phy0 -- set Interface phy0
> > +type=dpdk
> > options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
> > +OVS_WAIT_UNTIL([ovs-vsctl set port phy0 qos=@newqos -- --id=@newqos
> > create qos type=egress-policer other-config:cir=125 other-
> > config:cbs=2048])
> > +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show phy0], [], [stdout])
> > +sleep 2
> > +
> > +dnl Fail if policer could not be created AT_FAIL_IF([grep "Could not
> > +create rte meter for egress policer" ovs-
> > vswitchd.log], [], [stdout])
> > +
> > +dnl remove egress policer
> > +AT_CHECK([ovs-vsctl destroy QoS phy0 -- clear Port phy0 qos])
> > +
> > +dnl Clean up
> > +AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
> > +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
> > +\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostclient0:
> > No such file or directory@d
> 
> I don't see the need for this above, as it's a phy test there is no vhost 
> user port
> being created so this error should not occur.

You're right, that is there unnecessarily, I will remove it for the next 
version.

> 
> > +])")
> > +AT_CLEANUP
> > +dnl
> > +-
> > +-
> > +
> > +
> > +
> > +dnl
> > +-
> > +-
> > +dnl QoS create delete vport port
> > +AT_SETUP([OVS-DPDK - QoS create delete vport port])
> > +AT_KEYWORDS([dpdk])
> > +
> > +OVS_DPDK_PRE_CHECK()
> > +OVS_DPDK_START()
> > +
> > +dnl Add userspace bridge and attach it to OVS and add egress policer
> > +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> > datapath_type=netdev])
> > +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuserclient0 -- set
> > +Interface
> > dpdkvhostuserclient0 type=dpdkvhostuserclient options:vhost-server-
> > path=$OVS_RUNDIR/dpdkvhostclient0], [], [stdout], [stderr])
> > +OVS_WAIT_UNTIL([ovs-vsctl set port dpdkvhostuserclient0 qos=@newqos
> > -- --id=@newqos create qos type=egress-policer
> > other-config:cir=125 \
> > +  other-config:cbs=2048]) AT_CHECK([ovs-appctl -t
> > +ovs-vswitchd qos/show dpdkvhostuserclient0], [],
> > [stdout])
> > +sleep 2
> > +
> > +dnl Fail if policer could not be created
> 
> Would be better to specify here (and in the comments of the other tests) that 
> it
> is an egress policer that could not be created.

Agreed, I will update the comments for the next version.

Thanks,
Michael.

> 
> Thanks
> Ian
> 
> > +AT_FAIL_IF([grep "Could not create rte meter for egress policer" ovs-
> > vswitchd.log], [], [stdout])
> > +
> > +dnl remove egress policer
> > +AT_CHECK([ovs-vsctl destroy QoS dpdkvhostuserclient0 -- clear Port
> > dpdkvhostuserclient0 qos])
> > +
> > +dnl Parse log file
> > +AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created" ovs-
> > vswitchd.log], [], [stdout])
> > +AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in
> > 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
> > +AT_CHECK([grep "VHOST_CONFIG: $OVS_RUNDIR/dpdkvhostclient0:
> > reconnecting..." ovs-vswitchd.log], [], [stdout])
> > +

Re: [ovs-dev] [PATCH] tests: Add OVS-DPDK MTU unit tests.

[Phelan, Michael]

> -Original Message-
> From: Stokes, Ian 
> Sent: Tuesday 28 June 2022 15:09
> To: Phelan, Michael ; d...@openvswitch.org
> Cc: acon...@redhat.com; i.maxim...@ovn.org; maxime.coque...@redhat.com
> Subject: RE: [PATCH] tests: Add OVS-DPDK MTU unit tests.
> 
> 
> 
> > -Original Message-
> > From: Phelan, Michael 
> > Sent: Tuesday, June 21, 2022 11:21 AM
> > To: d...@openvswitch.org
> > Cc: Stokes, Ian ; acon...@redhat.com;
> > i.maxim...@ovn.org; maxime.coque...@redhat.com; Phelan, Michael
> > 
> > Subject: [PATCH] tests: Add OVS-DPDK MTU unit tests.
> >
> > This adds 8 new unit tests to the 'check-dpdk' subsystem that will
> > test Maximum Transmission Unit (MTU) functionality.
> >
> 
> Hi Michael,
> 
> Thanks for the patch, a few comments below.

Hey Ian,
Thanks for the review, I have left some responses inline below.
> 
> 
> > Signed-off-by: Michael Phelan 
> > ---
> >  tests/system-dpdk.at | 344
> > +++
> >  1 file changed, 344 insertions(+)
> >
> > diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at index
> > 7d2715c4a..6a2c5f79f 100644
> > --- a/tests/system-dpdk.at
> > +++ b/tests/system-dpdk.at
> > @@ -222,6 +222,350 @@ OVS_VSWITCHD_STOP("m4_join([],
> > [SYSTEM_DPDK_ALLOWED_LOGS], [  AT_CLEANUP  dnl
> > --
> > 
> >
> > +
> > +
> > +dnl
> > +-
> > +-
> > +dnl MTU increase phy port
> > +AT_SETUP([OVS-DPDK - MTU increase phy port])
> > +AT_KEYWORDS([dpdk])
> > +
> > +OVS_DPDK_PRE_PHY_SKIP()
> > +OVS_DPDK_START()
> > +
> > +dnl First set MTU to its default value and confirm that value, then
> > +increase
> > the MTU value and confirm the new value
> > +
> > +dnl Add userspace bridge and attach it to OVS with default MTU value
> > +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> > datapath_type=netdev])
> > +AT_CHECK([ovs-vsctl add-port br10 phy0 -- set Interface phy0
> > +type=dpdk
> > options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
> > +AT_CHECK([ovs-vsctl show], [], [stdout]) sleep 2
> > +
> > +dnl check defalt MTU value in DB and datapath AT_CHECK([ovs-vsctl get
> > +Interface phy0 mtu], [], [stdout]) AT_CHECK([egrep '1500' stdout],
> > +[], [stdout])
> 
> 
> I'm not sure is there a need to confirm the MTU in the DB here,  I'm thinking 
> is it
> enough to confirm the value in the datapath, the test itself is to confirm we 
> can
> set the MTU for the device, checking the DB MTU value doesn't confirm what's
> set on the device, only the value stored in the DB?
> 
> As such we could possibly remove the DB check unless there is a specific 
> reason
> to have it? @Ilya/Aaron, what are your thoughts?

Yep, I think that makes sense, no need for the extra check in that case. I will 
remove it in the next version.

> 
> > +
> > +AT_CHECK([ovs-appctl dpctl/show], [], [stdout]) AT_CHECK([egrep
> > +'mtu=1500,' stdout], [], [stdout])
> > +
> > +dnl increase MTU value and check in DB and datapath
> > +AT_CHECK(ovs-vsctl set Interface phy0 mtu_request=9000)
> > +AT_CHECK([ovs-vsctl get Interface phy0 mtu], [], [stdout])
> > +AT_CHECK([egrep '9000' stdout], [], [stdout])
> 
> So when we attempt to set the MTU for an ethdev there are 2 cases we should
> check for
> 
> 1. a device may not support MTU configuration, we should check for that when
> configuring ethdev ports as the test will be unable to increase/decrease MTU.
> 
> This warning is flagged specifically in the dpdk_eth_dev_port_config()
> 
> VLOG_WARN("Interface %s does not support MTU configuration, "
>   "max packet size supported is %"PRIu16".",
>   dev->up.name, conf_mtu);
> 
> 2. There could also be an error for one reason or another while setting he MTU
> for the ethdev device, we should check that this error occurs or not and fail 
> if it
> does occur. Again in dpdk_eth_dev_port_config()
> 
> VLOG_ERR("Interface %s MTU (%d) setup error: %s",
>  dev->up.name, dev->mtu, rte_strerror(-diag));
> 
> So it would be good to handle these cases for the ethdev (phy ports) unit 
> tests.

I might need to play around with triggering those warnings to see if the 
variables change across machines but other than that I think it will be good to 
include a check for each of them. I will add them in the next version also.

Thanks,
Michael.
> 
> Thanks
> Ian
> 
> > +
> > +AT_CHECK([ovs-appctl dpctl/show], [], [stdout]) AT_CHECK([egrep
> > +'mtu=9000,' stdout], [], [stdout])
> > +
> > +
> > +dnl Clean up
> > +AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
> > +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
> > +\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostclient0:
> > No such file or directory@d
> > +])")
> > +AT_CLEANUP
> > +dnl
> > +-
> > 

[ovs-dev] [PATCH ovsdb v3 1/1] Add Local_Config schema

[Terry Wilson]

The only way to configure settings on a remote (e.g. inactivity_probe)
is via --remote=db:DB,table,row. There is no way to do this via the
existing CLI options.

For a clustered DB with multiple servers listening on unique addresses
there is no way to store these entries in the DB as the DB is shared.
For example, three servers listening on  1.1.1.1, 1.1.1.2, and 1.1.1.3
respectively would require a Manager/Connection row each, but then
all three servers would try to listen on all three addresses.

It is possible for ovsdb-server to serve multiple databases. This
means that we can have a local "config" database in addition to
the main database we are servering (Open_vSwitch, OVN_Southbound, etc.)
and this patch adds a Local_Config schema that currently just mirrors
the Connection table and a Config table with a 'connections' row that
stores each Connection.

Signed-off-by: Terry Wilson 
---
 NEWS   |   5 +
 debian/openvswitch-common.manpages |   1 +
 ovsdb/.gitignore   |   2 +
 ovsdb/automake.mk  |  21 ++
 ovsdb/local-config.ovsschema   |  43 +
 ovsdb/local-config.xml | 296 +
 rhel/openvswitch-fedora.spec.in|   1 +
 tests/ovsdb-cluster.at |  42 +++-
 8 files changed, 404 insertions(+), 7 deletions(-)
 create mode 100644 ovsdb/local-config.ovsschema
 create mode 100644 ovsdb/local-config.xml

diff --git a/NEWS b/NEWS
index 9fe3f44f4..b9e6f0216 100644
--- a/NEWS
+++ b/NEWS
@@ -32,6 +32,11 @@ Post-v2.17.0
- DPDK:
  * OVS validated with DPDK 21.11.1.  It is recommended to use this version
until further releases.
+   - Local_Config schema added:
+ * Clustered ovsdb-servers listening on unique addresses can pass a second
+   DB created with the new Local_Config schema in order to configure their
+   Connections independent from each other. See the ovsdb.local-config.5
+   manpage for schema details.
 
 
 v2.17.0 - 17 Feb 2022
diff --git a/debian/openvswitch-common.manpages 
b/debian/openvswitch-common.manpages
index 95004122c..7c46a2acf 100644
--- a/debian/openvswitch-common.manpages
+++ b/debian/openvswitch-common.manpages
@@ -5,3 +5,4 @@ debian/tmp/usr/share/man/man8/ovs-appctl.8
 utilities/ovs-ofctl.8
 debian/tmp/usr/share/man/man8/ovs-parse-backtrace.8
 debian/tmp/usr/share/man/man8/ovs-pki.8
+ovsdb/ovsdb.local-config.5
diff --git a/ovsdb/.gitignore b/ovsdb/.gitignore
index fbcefafc6..a4f9d38f1 100644
--- a/ovsdb/.gitignore
+++ b/ovsdb/.gitignore
@@ -1,5 +1,7 @@
 /_server.ovsschema.inc
 /_server.ovsschema.stamp
+/local-config.ovsschema.stamp
+/ovsdb.local-config.5
 /ovsdb-client
 /ovsdb-client.1
 /ovsdb-doc
diff --git a/ovsdb/automake.mk b/ovsdb/automake.mk
index 62cc02686..3b3140102 100644
--- a/ovsdb/automake.mk
+++ b/ovsdb/automake.mk
@@ -148,4 +148,25 @@ ovsdb/ovsdb-server.5: \
$(srcdir)/ovsdb/_server.xml > $@.tmp && \
mv $@.tmp $@
 
+EXTRA_DIST += ovsdb/local-config.ovsschema
+pkgdata_DATA += ovsdb/local-config.ovsschema
+
+# Version checking for local-config.ovsschema.
+ALL_LOCAL += ovsdb/local-config.ovsschema.stamp
+ovsdb/local-config.ovsschema.stamp: ovsdb/local-config.ovsschema
+   $(srcdir)/build-aux/cksum-schema-check $? $@
+CLEANFILES += ovsdb/local-config.ovsschema.stamp
+
+# Local_Config schema documentation
+EXTRA_DIST += ovsdb/local-config.xml
+CLEANFILES += ovsdb/ovsdb.local-config.5
+man_MANS += ovsdb/ovsdb.local-config.5
+ovsdb/ovsdb.local-config.5: \
+   ovsdb/ovsdb-doc ovsdb/ ovsdb/local-config.xml 
ovsdb/local-config.ovsschema
+   $(AM_V_GEN)$(OVSDB_DOC) \
+   --version=$(VERSION) \
+   $(srcdir)/ovsdb/local-config.ovsschema \
+   $(srcdir)/ovsdb/local-config.xml > $@.tmp && \
+   mv $@.tmp $@
+
 EXTRA_DIST += ovsdb/TODO.rst
diff --git a/ovsdb/local-config.ovsschema b/ovsdb/local-config.ovsschema
new file mode 100644
index 0..bd86d0f4f
--- /dev/null
+++ b/ovsdb/local-config.ovsschema
@@ -0,0 +1,43 @@
+{
+"name": "Local_Config",
+"version": "1.0.0",
+"cksum": "2048726482 1858",
+"tables": {
+"Config": {
+"columns": {
+"connections": {
+"type": {"key": {"type": "uuid",
+ "refTable": "Connection"},
+ "min": 0,
+ "max": "unlimited"}}},
+"maxRows": 1,
+"isRoot": true},
+"Connection": {
+"columns": {
+"target": {"type": "string"},
+"max_backoff": {"type": {"key": {"type": "integer",
+ "minInteger": 1000},
+ "min": 0,
+ "max": 1}},
+"inactivity_probe": {"type": {"key": "integer",
+  "min": 0,
+

Re: [ovs-dev] [PATCH] tests: Add OVS-DPDK QoS unit tests

[Stokes, Ian]

> -Original Message-
> From: Phelan, Michael 
> Sent: Tuesday, June 21, 2022 11:19 AM
> To: d...@openvswitch.org
> Cc: Stokes, Ian ; acon...@redhat.com;
> i.maxim...@ovn.org; maxime.coque...@redhat.com; Phelan, Michael
> 
> Subject: [PATCH] tests: Add OVS-DPDK QoS unit tests
> 
> This adds 4 new unit tests to the 'check-dpdk' subsystem that will
> test Quality of Service (QoS) functionality.
> 

Hi Michael,

This is quite similar to the rate limiting patch but there are a few comments 
here that needs addressing.


> Signed-off-by: Michael Phelan 
> ---
>  tests/system-dpdk.at | 141
> +++
>  1 file changed, 141 insertions(+)
> 
> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
> index 7d2715c4a..90e85e8a9 100644
> --- a/tests/system-dpdk.at
> +++ b/tests/system-dpdk.at
> @@ -222,6 +222,147 @@ OVS_VSWITCHD_STOP("m4_join([],
> [SYSTEM_DPDK_ALLOWED_LOGS], [
>  AT_CLEANUP
>  dnl 
> --
> 
> +
> +
> +dnl 
> --
> +dnl QoS create delete phy port
> +AT_SETUP([OVS-DPDK - QoS create delete phy port])
> +AT_KEYWORDS([dpdk])
> +
> +OVS_DPDK_PRE_PHY_SKIP()
> +OVS_DPDK_START()
> +
> +dnl Add userspace bridge and attach it to OVS and add egress policer
> +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> datapath_type=netdev])
> +AT_CHECK([ovs-vsctl add-port br10 phy0 -- set Interface phy0 type=dpdk
> options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
> +OVS_WAIT_UNTIL([ovs-vsctl set port phy0 qos=@newqos -- --id=@newqos
> create qos type=egress-policer other-config:cir=125 other-
> config:cbs=2048])
> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show phy0], [], [stdout])
> +sleep 2
> +
> +dnl Fail if policer could not be created
> +AT_FAIL_IF([grep "Could not create rte meter for egress policer" ovs-
> vswitchd.log], [], [stdout])
> +
> +dnl remove egress policer
> +AT_CHECK([ovs-vsctl destroy QoS phy0 -- clear Port phy0 qos])
> +
> +dnl Clean up
> +AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
> +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
> +\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostclient0:
> No such file or directory@d

I don't see the need for this above, as it's a phy test there is no vhost user 
port being created so this error should not occur.

> +])")
> +AT_CLEANUP
> +dnl 
> --
> +
> +
> +
> +dnl 
> --
> +dnl QoS create delete vport port
> +AT_SETUP([OVS-DPDK - QoS create delete vport port])
> +AT_KEYWORDS([dpdk])
> +
> +OVS_DPDK_PRE_CHECK()
> +OVS_DPDK_START()
> +
> +dnl Add userspace bridge and attach it to OVS and add egress policer
> +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> datapath_type=netdev])
> +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuserclient0 -- set Interface
> dpdkvhostuserclient0 type=dpdkvhostuserclient options:vhost-server-
> path=$OVS_RUNDIR/dpdkvhostclient0], [], [stdout], [stderr])
> +OVS_WAIT_UNTIL([ovs-vsctl set port dpdkvhostuserclient0 qos=@newqos
> -- --id=@newqos create qos type=egress-policer other-config:cir=125 \
> +  other-config:cbs=2048])
> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [],
> [stdout])
> +sleep 2
> +
> +dnl Fail if policer could not be created

Would be better to specify here (and in the comments of the other tests) that 
it is an egress policer that could not be created.

Thanks
Ian

> +AT_FAIL_IF([grep "Could not create rte meter for egress policer" ovs-
> vswitchd.log], [], [stdout])
> +
> +dnl remove egress policer
> +AT_CHECK([ovs-vsctl destroy QoS dpdkvhostuserclient0 -- clear Port
> dpdkvhostuserclient0 qos])
> +
> +dnl Parse log file
> +AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created" ovs-
> vswitchd.log], [], [stdout])
> +AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in
> 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
> +AT_CHECK([grep "VHOST_CONFIG: $OVS_RUNDIR/dpdkvhostclient0:
> reconnecting..." ovs-vswitchd.log], [], [stdout])
> +
> +dnl Clean up
> +AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout],
> [stderr])
> +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
> +\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostclient0:
> No such file or directory@d
> +])")
> +AT_CLEANUP
> +dnl 
> --
> +
> +
> +
> +dnl 
> --
> +dnl QoS no cir
> +AT_SETUP([OVS-DPDK - QoS no cir])
> +AT_KEYWORDS([dpdk])
> +
> +OVS_DPDK_PRE_CHECK()
> +OVS_DPDK_START()
> +
> +dnl Add userspace bridge and attach it to OVS and add egress policer
> +AT_CHECK([ovs-vsctl add-br br10 -- 

Re: [ovs-dev] [PATCH v3] tests: Add ovs-dpdk rate limiting unit tests.

[Phelan, Michael]

> -Original Message-
> From: Stokes, Ian 
> Sent: Tuesday 28 June 2022 14:43
> To: Phelan, Michael ; d...@openvswitch.org
> Cc: acon...@redhat.com; i.maxim...@ovn.org;
> maxime.coque...@redhat.com; david.march...@redhat.com; Ryan, Seamus
> 
> Subject: RE: [PATCH v3] tests: Add ovs-dpdk rate limiting unit tests.
> 
> > This adds 4 new unit tests to the 'check-dpdk' subsystem that will
> > test rate limiting functionality.
> >
> > Signed-off-by: Seamus Ryan 
> > Signed-off-by: Michael Phelan 
> > Co-authored-by: Michael Phelan 
> >
> > ---
> > v3:
> >   - Removed NEWS entry.
> >   - Added check to catch error if policer fails to be created.
> >
> > v2:
> >   - Fixed dn1 typo and spacing issues.
> >   - Added check for removing burst and rate values.
> >   - Renamed tests to specify ingress policing.
> > ---
> > ---
> >  tests/system-dpdk.at | 164
> > +++
> >  1 file changed, 164 insertions(+)
> >
> > diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at index
> > 7d2715c4a..8151eee41 100644
> > --- a/tests/system-dpdk.at
> > +++ b/tests/system-dpdk.at
> > @@ -222,6 +222,170 @@ OVS_VSWITCHD_STOP("m4_join([],
> > [SYSTEM_DPDK_ALLOWED_LOGS], [  AT_CLEANUP  dnl
> > --
> > 
> >
> > +
> > +
> > +dnl
> > +-
> > +- dnl Ingress policing create delete phy port AT_SETUP([OVS-DPDK
> > +- Ingress policing create delete phy port])
> > +AT_KEYWORDS([dpdk])
> > +
> > +OVS_DPDK_PRE_PHY_SKIP()
> > +OVS_DPDK_START()
> > +
> > +dnl Add userspace bridge and attach it to OVS and add policer
> > +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> > datapath_type=netdev])
> > +AT_CHECK([ovs-vsctl add-port br10 phy0 -- set Interface phy0
> > +type=dpdk
> > options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
> > +AT_CHECK([ovs-vsctl set interface phy0 ingress_policing_rate=1
> > ingress_policing_burst=1000])
> > +AT_CHECK([ovs-vsctl show], [], [stdout]) sleep 2
> > +
> > +dnl Fail if policer could not be created AT_FAIL_IF([grep "Could not
> > +create rte meter for egress policer" ovs-
> > vswitchd.log], [], [stdout])
> 
> Hi Michael, this error will never actually occur as rate limiting is ingress, 
> no
> egress, as such the error message you should be checking for is
> 
> "Could not create rte meter for ingress policer"
> 
> It's probably worth specifying in the comment also that you are specifically
> checking for the ingress policer to fail here.

Hey Ian,
Thanks for pointing this out, I will change to ingress and update the comment 
in the next version.

Thanks,
Michael.
> 
> The same changes should be made to the rest of the tests in this patch.
> 
> Thanks
> Ian
> 
> > +
> > +dnl remove policer
> > +AT_CHECK([ovs-vsctl set interface phy0 ingress_policing_rate=0
> > ingress_policing_burst=0])
> > +
> > +dnl check policer was removed correctly AT_CHECK([ovs-vsctl list
> > +interface phy0], [], [stdout]) AT_CHECK([egrep
> > +'ingress_policing_burst: 0' stdout], [], [stdout])
> > +
> > +AT_CHECK([ovs-vsctl list interface phy0], [], [stdout])
> > +AT_CHECK([egrep 'ingress_policing_rate: 0' stdout], [], [stdout])
> > +
> > +dnl Clean up
> > +AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
> > +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
> > +\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostclient0:
> > No such file or directory@d
> > +])")
> > +AT_CLEANUP
> > +dnl
> > +-
> > +-
> > +
> > +
> > +
> > +dnl
> > +-
> > +- dnl Ingress policing create delete vport port
> > +AT_SETUP([OVS-DPDK - Ingress policing create delete vport port])
> > +AT_KEYWORDS([dpdk])
> > +
> > +OVS_DPDK_PRE_CHECK()
> > +OVS_DPDK_START()
> > +
> > +dnl Add userspace bridge and attach it to OVS and add policer
> > +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> > datapath_type=netdev])
> > +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuserclient0 -- set
> > +Interface
> > dpdkvhostuserclient0 type=dpdkvhostuserclient options:vhost-server-
> > path=$OVS_RUNDIR/dpdkvhostclient0], [], [stdout], [stderr])
> > +AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0
> > ingress_policing_rate=1 ingress_policing_burst=1000])
> > +AT_CHECK([ovs-vsctl show], [], [stdout]) sleep 2
> > +
> > +dnl Fail if policer could not be created AT_FAIL_IF([grep "Could not
> > +create rte meter for egress policer" ovs-
> > vswitchd.log], [], [stdout])
> > +
> > +dnl remove policer
> > +AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0
> > ingress_policing_rate=0 ingress_policing_burst=0])
> > +
> > +dnl check policer was removed correctly AT_CHECK([ovs-vsctl list
> > +interface dpdkvhostuserclient0], [], [stdout]) AT_CHECK([egrep
> > +'ingress_policing_burst: 0' stdout], [], 

Re: [ovs-dev] [PATCH ovn] northd.c: Add flow to skip put_nd action if ip6.src or nd.sll is 0

[Dumitru Ceara]

On 6/27/22 11:56, Ales Musil wrote:
> The ip6.src or nd.sll does not have to be always set.
> According to rfc4861:
> 
> Source Address
>  Either an address assigned to the interface from
>  which this message is sent or (if Duplicate Address
>  Detection is in progress [ADDRCONF]) the
>  unspecified address.
> 
> Source link-layer address
>  The link-layer address for the sender.  MUST NOT be
>  included when the source IP address is the
>  unspecified address.  Otherwise, on link layers
>  that have addresses this option MUST be included in
>  multicast solicitations and SHOULD be included in
>  unicast solicitations.
> 
> Add rule that avoids adding MAC binding is either of those
> is 0. This is continuation after discussion during review on
> 80187a803 (ovn-northd: Add flow to use eth.src if nd.tll is 0
> in put_nd() action.)
> 
> Signed-off-by: Ales Musil 
> ---

Hi Ales,

Thanks for the follow up!

>  northd/northd.c | 3 +++
>  northd/ovn-northd.8.xml | 6 ++
>  tests/ovn-northd.at | 3 +++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index 6997c280c..6634edb0f 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -11022,6 +11022,9 @@ build_neigh_learning_flows_for_lrouter(
>  ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 100,
>ds_cstr(match), "next;");
>  
> +ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95,
> +  "nd_ns && (ip6.src == 0 || nd.sll == 0)", "next;");
> +

Nit: I'd move this lower, I think the flows are grouped per IP family
right now.

>  ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90,
>"arp", "put_arp(inport, arp.spa, arp.sha); next;",
>copp_meter_get(COPP_ARP, od->nbr->copp,
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 59c584710..5df74a410 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -2332,6 +2332,12 @@ next;
>  to learn the neighbor.
>
>  
> +  
> +A priority-95 flow with the match nd_ns 
> +  (ip6.src == 0 || nd.sll == 0) and applies the action
> +next;
> +  
> +
>
>  A priority-90 flow with the match arp and
>  applies the action
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 033b58b8c..fe97bedad 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -6707,6 +6707,7 @@ AT_CHECK([ovn-sbctl dump-flows DR | grep -e 
> lr_in_unsnat -e lr_out_snat -e lr_in
>  
>  AT_CLEANUP
>  
> +OVN_FOR_EACH_NORTHD([

This is unrelated.

>  AT_SETUP([LR NB Static_MAC_Binding table])
>  ovn_start
>  
> @@ -6730,6 +6731,7 @@ ovn-nbctl --may-exist static-mac-binding-add lr0-p0 
> 192.168.10.100 00:00:22:33:5
>  wait_row_count Static_MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.100 
> mac="00\:00\:22\:33\:55\:66"
>  
>  AT_CLEANUP
> +])

This too.

>  
>  OVN_FOR_EACH_NORTHD([
>  AT_SETUP([LR neighbor lookup and learning flows])
> @@ -6751,6 +6753,7 @@ AT_CHECK([cat lrflows | grep -e lr_in_lookup_neighbor 
> -e lr_in_learn_neighbor |
>table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_na), 
> action=(put_nd(inport, nd.target, nd.tll); next;)
>table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_ns), 
> action=(put_nd(inport, ip6.src, nd.sll); next;)
>table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_na && nd.tll == 
> 0), action=(put_nd(inport, nd.target, eth.src); next;)
> +  table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_ns && (ip6.src 
> == 0 || nd.sll == 0)), action=(next;)
>  ])
>  
>  AT_CLEANUP

Thanks,
Dumitru

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

Re: [ovs-dev] [PATCH ovn] tests: fixed flaky test localnet connectivity with multiple requested-chassis

[Dumitru Ceara]

On 6/22/22 18:21, Xavier Simonart wrote:
> The test was failing randomly (on highly loaded systems) mainly because
> the MAC address of the migrator port in the main switch was migrating at
> unexpected times.
> In addition, a few undefined variables were fixed, and sleep was removed /
> replaced by ovs_wait actions.
> 
> Signed-off-by: Xavier Simonart 
> ---
>  tests/ovn.at | 22 --
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/ovn.at b/tests/ovn.at
> index bfaa41962..12d1f8667 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -14691,10 +14691,10 @@ wait_column "" Port_Binding 
> requested_additional_chassis logical_port=migrator
>  wait_for_ports_up
>  
>  # advertise location of ports through localnet port
> -send_garp hv1 migrator 00ff  $migrator_spa $migrator_tpa
> -send_garp hv1 first 0001  $first_spa $first_tpa
> -send_garp hv2 second 0002  $second_spa $second_tpa
> -send_garp hv3 third 0003  $third_spa $third_tpa
> +send_garp hv1 migrator 00ff  $migrator_tpa $migrator_tpa
> +send_garp hv1 first 0001  $first_spa $first_spa
> +send_garp hv2 second 0002  $second_spa $second_spa
> +send_garp hv3 third 0003  $third_spa $third_spa
>  reset_env
>  
>  # check that...
> @@ -14840,6 +14840,12 @@ echo $request >> hv3/third.expected
>  
>  check_packets
>  
> +# Wait for MAC address of migrator to be on hv1 related port in main switch.
> +# Hence the MAC will not migrate back unexpectedly later.
> +p1=$(as main ovs-ofctl show n1 | grep hv1_br-phys | awk '{print int($1)}')
> +p2=$(as main ovs-ofctl show n1 | grep hv2_br-phys | awk '{print int($1)}')
> +OVS_WAIT_UNTIL([test x`as main ovs-appctl fdb/show n1 | grep 
> 00:00:00:00:00:ff  | awk '{print $1}'` = x$p1])

Nit: extra whitespace before last pipe.

> +
>  # Complete migration: destination is bound
>  check ovn-nbctl lsp-set-options migrator requested-chassis=hv2
>  wait_column "$hv2_uuid" Port_Binding chassis logical_port=migrator
> @@ -14849,12 +14855,16 @@ wait_column "" Port_Binding 
> requested_additional_chassis logical_port=migrator
>  wait_for_ports_up
>  
>  check ovn-nbctl --wait=hv sync
> -sleep 1
> +OVS_WAIT_UNTIL([test `as hv2 ovs-vsctl get Interface migrator 
> external_ids:ovn-installed` = '"true"'])
>  
>  # advertise new location of the port through localnet port
> -send_garp hv2 migrator 00ff  $migrator_spa $migrator_tpa
> +send_garp hv2 migrator 00ff  $migrator_tpa $migrator_tpa
> +
>  reset_env
>  
> +# Wait for MAC address of migrator to be on hv2 port in main switch
> +OVS_WAIT_UNTIL([test x`as main ovs-appctl fdb/show n1 | grep 
> 00:00:00:00:00:ff  | awk '{print $1}'` = x$p2])

Nit: extra whitespace before last pipe.

I'm guessing both can be fixed at apply time, therefore:

Acked-by: Dumitru Ceara 

Thanks!

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

Re: [ovs-dev] [PATCH] tests: Add OVS-DPDK MTU unit tests.

[Stokes, Ian]

> -Original Message-
> From: Phelan, Michael 
> Sent: Tuesday, June 21, 2022 11:21 AM
> To: d...@openvswitch.org
> Cc: Stokes, Ian ; acon...@redhat.com;
> i.maxim...@ovn.org; maxime.coque...@redhat.com; Phelan, Michael
> 
> Subject: [PATCH] tests: Add OVS-DPDK MTU unit tests.
> 
> This adds 8 new unit tests to the 'check-dpdk' subsystem that will
> test Maximum Transmission Unit (MTU) functionality.
> 

Hi Michael,

Thanks for the patch, a few comments below.


> Signed-off-by: Michael Phelan 
> ---
>  tests/system-dpdk.at | 344
> +++
>  1 file changed, 344 insertions(+)
> 
> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
> index 7d2715c4a..6a2c5f79f 100644
> --- a/tests/system-dpdk.at
> +++ b/tests/system-dpdk.at
> @@ -222,6 +222,350 @@ OVS_VSWITCHD_STOP("m4_join([],
> [SYSTEM_DPDK_ALLOWED_LOGS], [
>  AT_CLEANUP
>  dnl 
> --
> 
> +
> +
> +dnl 
> --
> +dnl MTU increase phy port
> +AT_SETUP([OVS-DPDK - MTU increase phy port])
> +AT_KEYWORDS([dpdk])
> +
> +OVS_DPDK_PRE_PHY_SKIP()
> +OVS_DPDK_START()
> +
> +dnl First set MTU to its default value and confirm that value, then increase
> the MTU value and confirm the new value
> +
> +dnl Add userspace bridge and attach it to OVS with default MTU value
> +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> datapath_type=netdev])
> +AT_CHECK([ovs-vsctl add-port br10 phy0 -- set Interface phy0 type=dpdk
> options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
> +AT_CHECK([ovs-vsctl show], [], [stdout])
> +sleep 2
> +
> +dnl check defalt MTU value in DB and datapath
> +AT_CHECK([ovs-vsctl get Interface phy0 mtu], [], [stdout])
> +AT_CHECK([egrep '1500' stdout], [], [stdout])


I'm not sure is there a need to confirm the MTU in the DB here,  I'm thinking 
is it enough to confirm the value in the datapath, the test itself is to 
confirm we can set the MTU for the device, checking the DB MTU value doesn't 
confirm what's set on the device, only the value stored in the DB?

As such we could possibly remove the DB check unless there is a specific reason 
to have it? @Ilya/Aaron, what are your thoughts?

> +
> +AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
> +AT_CHECK([egrep 'mtu=1500,' stdout], [], [stdout])
> +
> +dnl increase MTU value and check in DB and datapath
> +AT_CHECK(ovs-vsctl set Interface phy0 mtu_request=9000)
> +AT_CHECK([ovs-vsctl get Interface phy0 mtu], [], [stdout])
> +AT_CHECK([egrep '9000' stdout], [], [stdout])

So when we attempt to set the MTU for an ethdev there are 2 cases we should 
check for

1. a device may not support MTU configuration, we should check for that when 
configuring ethdev ports as the test will be unable to increase/decrease MTU.

This warning is flagged specifically in the dpdk_eth_dev_port_config()

VLOG_WARN("Interface %s does not support MTU configuration, "
  "max packet size supported is %"PRIu16".",
  dev->up.name, conf_mtu);

2. There could also be an error for one reason or another while setting he MTU 
for the ethdev device, we should check that this error occurs or not and fail 
if it does occur. Again in dpdk_eth_dev_port_config()

VLOG_ERR("Interface %s MTU (%d) setup error: %s",
 dev->up.name, dev->mtu, rte_strerror(-diag));

So it would be good to handle these cases for the ethdev (phy ports) unit tests.

Thanks
Ian

> +
> +AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
> +AT_CHECK([egrep 'mtu=9000,' stdout], [], [stdout])
> +
> +
> +dnl Clean up
> +AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
> +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
> +\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostclient0:
> No such file or directory@d
> +])")
> +AT_CLEANUP
> +dnl 
> --
> +
> +
> +
> +dnl 
> --
> +dnl MTU decrease phy port
> +AT_SETUP([OVS-DPDK - MTU decrease phy port])
> +AT_KEYWORDS([dpdk])
> +
> +OVS_DPDK_PRE_PHY_SKIP()
> +OVS_DPDK_START()
> +
> +dnl First set an increased MTU value and confirm that value, then decrease
> the MTU value and confirm the new value
> +
> +dnl Add userspace bridge and attach it to OVS and modify MTU value
> +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> datapath_type=netdev])
> +AT_CHECK([ovs-vsctl add-port br10 phy0 -- set Interface phy0 type=dpdk
> options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
> +AT_CHECK(ovs-vsctl set Interface phy0 mtu_request=9000)
> +AT_CHECK([ovs-vsctl show], [], [stdout])
> +sleep 2
> +
> +dnl check MTU value in DB and datapath
> +AT_CHECK([ovs-vsctl get Interface phy0 mtu], [], [stdout])
> +AT_CHECK([egrep '9000' stdout], [], [stdout])
> +
> 

Re: [ovs-dev] [PATCH v3] tests: Add ovs-dpdk rate limiting unit tests.

[Stokes, Ian]

> This adds 4 new unit tests to the 'check-dpdk' subsystem that will
> test rate limiting functionality.
> 
> Signed-off-by: Seamus Ryan 
> Signed-off-by: Michael Phelan 
> Co-authored-by: Michael Phelan 
> 
> ---
> v3:
>   - Removed NEWS entry.
>   - Added check to catch error if policer fails to be created.
> 
> v2:
>   - Fixed dn1 typo and spacing issues.
>   - Added check for removing burst and rate values.
>   - Renamed tests to specify ingress policing.
> ---
> ---
>  tests/system-dpdk.at | 164
> +++
>  1 file changed, 164 insertions(+)
> 
> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
> index 7d2715c4a..8151eee41 100644
> --- a/tests/system-dpdk.at
> +++ b/tests/system-dpdk.at
> @@ -222,6 +222,170 @@ OVS_VSWITCHD_STOP("m4_join([],
> [SYSTEM_DPDK_ALLOWED_LOGS], [
>  AT_CLEANUP
>  dnl 
> --
> 
> +
> +
> +dnl 
> --
> +dnl Ingress policing create delete phy port
> +AT_SETUP([OVS-DPDK - Ingress policing create delete phy port])
> +AT_KEYWORDS([dpdk])
> +
> +OVS_DPDK_PRE_PHY_SKIP()
> +OVS_DPDK_START()
> +
> +dnl Add userspace bridge and attach it to OVS and add policer
> +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> datapath_type=netdev])
> +AT_CHECK([ovs-vsctl add-port br10 phy0 -- set Interface phy0 type=dpdk
> options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
> +AT_CHECK([ovs-vsctl set interface phy0 ingress_policing_rate=1
> ingress_policing_burst=1000])
> +AT_CHECK([ovs-vsctl show], [], [stdout])
> +sleep 2
> +
> +dnl Fail if policer could not be created
> +AT_FAIL_IF([grep "Could not create rte meter for egress policer" ovs-
> vswitchd.log], [], [stdout])

Hi Michael, this error will never actually occur as rate limiting is ingress, 
no egress, as such the error message you should be checking for is 

"Could not create rte meter for ingress policer"

It's probably worth specifying in the comment also that you are specifically 
checking for the ingress policer to fail here.

The same changes should be made to the rest of the tests in this patch.

Thanks
Ian

> +
> +dnl remove policer
> +AT_CHECK([ovs-vsctl set interface phy0 ingress_policing_rate=0
> ingress_policing_burst=0])
> +
> +dnl check policer was removed correctly
> +AT_CHECK([ovs-vsctl list interface phy0], [], [stdout])
> +AT_CHECK([egrep 'ingress_policing_burst: 0' stdout], [], [stdout])
> +
> +AT_CHECK([ovs-vsctl list interface phy0], [], [stdout])
> +AT_CHECK([egrep 'ingress_policing_rate: 0' stdout], [], [stdout])
> +
> +dnl Clean up
> +AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
> +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
> +\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostclient0:
> No such file or directory@d
> +])")
> +AT_CLEANUP
> +dnl 
> --
> +
> +
> +
> +dnl 
> --
> +dnl Ingress policing create delete vport port
> +AT_SETUP([OVS-DPDK - Ingress policing create delete vport port])
> +AT_KEYWORDS([dpdk])
> +
> +OVS_DPDK_PRE_CHECK()
> +OVS_DPDK_START()
> +
> +dnl Add userspace bridge and attach it to OVS and add policer
> +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> datapath_type=netdev])
> +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuserclient0 -- set Interface
> dpdkvhostuserclient0 type=dpdkvhostuserclient options:vhost-server-
> path=$OVS_RUNDIR/dpdkvhostclient0], [], [stdout], [stderr])
> +AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0
> ingress_policing_rate=1 ingress_policing_burst=1000])
> +AT_CHECK([ovs-vsctl show], [], [stdout])
> +sleep 2
> +
> +dnl Fail if policer could not be created
> +AT_FAIL_IF([grep "Could not create rte meter for egress policer" ovs-
> vswitchd.log], [], [stdout])
> +
> +dnl remove policer
> +AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0
> ingress_policing_rate=0 ingress_policing_burst=0])
> +
> +dnl check policer was removed correctly
> +AT_CHECK([ovs-vsctl list interface dpdkvhostuserclient0], [], [stdout])
> +AT_CHECK([egrep 'ingress_policing_burst: 0' stdout], [], [stdout])
> +
> +AT_CHECK([ovs-vsctl list interface dpdkvhostuserclient0], [], [stdout])
> +AT_CHECK([egrep 'ingress_policing_rate: 0' stdout], [], [stdout])
> +
> +dnl Parse log file
> +AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created" ovs-
> vswitchd.log], [], [stdout])
> +AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in
> 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
> +AT_CHECK([grep "VHOST_CONFIG: $OVS_RUNDIR/dpdkvhostclient0:
> reconnecting..." ovs-vswitchd.log], [], [stdout])
> +
> +dnl Clean up
> +AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout],
> [stderr])
> +OVS_VSWITCHD_STOP("m4_join([], 

Re: [ovs-dev] [PATCH] dpif-netdev: Refactor AVX512 runtime checks.

[Pai G, Sunil]

> -Original Message-
> From: Ilya Maximets 
> Sent: Tuesday, June 28, 2022 1:07 AM
> To: David Marchand ; d...@openvswitch.org
> Cc: i.maxim...@ovn.org; amu...@redhat.com; Van Haaren, Harry
> ; Ferriter, Cian ;
> Pai G, Sunil 
> Subject: Re: [PATCH] dpif-netdev: Refactor AVX512 runtime checks.
> 
> On 6/24/22 09:29, David Marchand wrote:
> > As described in the bugzilla below, cpu_has_isa code may be compiled
> > with some AVX512 instructions in it, because cpu.c is built as part of
> > the libopenvswitchavx512.
> > This is a problem when this function (supposed to probe for AVX512
> > instructions availability) is invoked from generic OVS code, on older
> > CPUs that don't support them.
> >
> > For the same reason, dpcls_subtable_avx512_gather_probe,
> > dp_netdev_input_outer_avx512_probe, mfex_avx512_probe and
> > mfex_avx512_vbmi_probe are potential runtime bombs and can't either be
> > built as part of libopenvswitchavx512.
> >
> > Move cpu.c to be part of the "normal" libopenvswitch.
> > And move other helpers in generic OVS code.
> >
> > Note:
> > - dpcls_subtable_avx512_gather_probe is split in two, because it also
> >   needs to do its own magic,
> > - while moving those helpers, prefer direct calls to cpu_has_isa and
> >   avoid cast to intermediate integer variables when a simple boolean
> >   is enough,
> >
> > Fixes: b366fa2f4947 ("dpif-netdev: Call cpuid for x86 isa
> > availability.")
> > Reported-at: https://bugzilla.redhat.com/2100393
> > Reported-by: Ales Musil 
> > Signed-off-by: David Marchand 
> > ---
> 
> Thanks, David and Ales.  The change looks good to me in general.
> 
> Cian, Sunil, could you, please, take a look?
> 
> The issue is causing failures in upstream OpenStack CI.
> We'll also need to backport this change to stable branches.
> 
> Best regards, Ilya Maximets.


The patch looks good to me. I'm happy to ack.
Acked-by: Sunil Pai G 



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

Re: [ovs-dev] [PATCH ovn v2] ovn-controller: fixed ovn-installed not always properly added.

[Dumitru Ceara]

On 6/27/22 10:52, Xavier Simonart wrote:
> OVN checks whether ovn-installed is already present (in OVS) before updating 
> it.
> This might cause ovn-installed related issues in the following case:
> - (1) ovn-installed is present
> - (2) we claim the interface
> - (3) we update ovs, removing ovn-installed and start installing flows
> - (4) (next loop), after flows installed, we check if ovn-installed is absent,
>   and if yes, we update OVS with ovn-installed.
> However, in step4, if OVS is still busy from step 3, ovn-installed is read as
> present; hence OVN controller does not update it and move to INSTALLED state.
> 
> Note that this does not happen with writing port up in SBDB because Port 
> status
> changes will hit I-P.
> 
> This patch will require updating the state machine description in the
> "controller: avoid recomputes triggered by SBDB Port_Binding updates." patch
> when it's merged, as it changes the if-status state-machine.
> 
> Signed-off-by: Xavier Simonart 
> ---

Hi Xavier,

I only have a couple tiny style related remarks below.  I think those
can be addressed at apply time if maintainers agree.  Therefore:

Acked-by: Dumitru Ceara 

Thanks,
Dumitru

>  controller/binding.c   | 35 --
>  controller/binding.h   |  6 ++
>  controller/if-status.c | 43 --
>  3 files changed, 80 insertions(+), 4 deletions(-)
> 
> diff --git a/controller/binding.c b/controller/binding.c
> index 2279570f9..157c381cf 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -643,6 +643,19 @@ local_binding_get_lport_ofport(const struct shash 
> *local_bindings,
>  u16_to_ofp(lbinding->iface->ofport[0]) : 0;
>  }
>  
> +bool
> +local_binding_is_ovn_installed(struct shash *local_bindings,
> +   const char *pb_name)
> +{
> +struct local_binding *lbinding =
> +local_binding_find(local_bindings, pb_name);
> +if (lbinding && lbinding->iface) {
> +return smap_get_bool(>iface->external_ids,
> + OVN_INSTALLED_EXT_ID, false);
> +}
> +return false;
> +}
> +
>  bool
>  local_binding_is_up(struct shash *local_bindings, const char *pb_name)
>  {
> @@ -715,6 +728,22 @@ local_binding_set_up(struct shash *local_bindings, const 
> char *pb_name,
>  }
>  }
>  
> +void
> +local_binding_remove_ovn_installed(struct shash *local_bindings,
> +   const char *pb_name, bool ovs_readonly)
> +{

Nit: we could avoid the shash lookup if we return if ovs_readonly ==
true here.

> +struct local_binding *lbinding =
> +local_binding_find(local_bindings, pb_name);
> +
> +if (!ovs_readonly && lbinding && lbinding->iface
> +&& smap_get_bool(>iface->external_ids,
> + OVN_INSTALLED_EXT_ID, false)) {
> +VLOG_INFO("Removing lport %s ovn-installed in OVS", pb_name);
> +ovsrec_interface_update_external_ids_delkey(lbinding->iface,
> +OVN_INSTALLED_EXT_ID);
> +}
> +}
> +
>  void
>  local_binding_set_down(struct shash *local_bindings, const char *pb_name,
> const struct sbrec_chassis *chassis_rec,
> @@ -1297,9 +1326,11 @@ consider_vif_lport_(const struct sbrec_port_binding 
> *pb,
>  const char *requested_chassis_option = smap_get(
>  >options, "requested-chassis");
>  VLOG_INFO_RL(,
> -"Not claiming lport %s, chassis %s requested-chassis %s",
> +"Not claiming lport %s, chassis %s requested-chassis %s "
> +"pb->chassis %s",
>  pb->logical_port, b_ctx_in->chassis_rec->name,
> -requested_chassis_option ? requested_chassis_option : "[]");
> +requested_chassis_option ? requested_chassis_option : "[]",
> +pb->chassis ? pb->chassis->name: "");
>  }
>  }
>  
> diff --git a/controller/binding.h b/controller/binding.h
> index 1fed06674..445bdd9f2 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -152,6 +152,12 @@ ofp_port_t local_binding_get_lport_ofport(const struct 
> shash *local_bindings,
>const char *pb_name);
>  
>  bool local_binding_is_up(struct shash *local_bindings, const char *pb_name);
> +bool local_binding_is_ovn_installed(struct shash *local_bindings,
> +const char *pb_name);
> +void local_binding_remove_ovn_installed(struct shash *local_bindings,
> +const char *pb_name,
> +bool ovs_readonly);
> +
>  bool local_binding_is_down(struct shash *local_bindings, const char 
> *pb_name);
>  void local_binding_set_up(struct shash *local_bindings, const char *pb_name,
>const struct sbrec_chassis *chassis_rec,
> diff --git 

Re: [ovs-dev] [PATCH ovn v3] controller: avoid recomputes triggered by SBDB Port_Binding updates.

[Dumitru Ceara]

On 6/22/22 12:23, Xavier Simonart wrote:
> When VIF ports are claimed on a chassis, SBDB Port_Binding table is updated.
> If the SBDB IDL is still is read-only ("in transaction") when such a update
> is required, the update is not possible and recompute is triggered through
> I+P failure.
> 
> This situation can happen:
> - after updating Port_Binding->chassis to SBDB for one port, in a following
>   iteration, ovn-controller handles Interface:external_ids:ovn-installed
>   (for the same port) while SBDB is still read-only.
> - after updating Port_Binding->chassis to SBDB for one port, in a following
>   iteration, ovn-controller updates Port_Binding->chassis for another port,
>   while SBDB is still read-only.
> 
> This patch prevent the recompute, by having the if-status module
> updating the Port_Binding chassis (if needed) when possible.
> This does not delay Port_Binding chassis update compared to before this patch.
> - With the patch, Port_Binding chassis will be updated as soon as SBDB is
> again writable, without recompute.
> - Without the patch, Port_Binding chassis was updated as soon as SBDB was
> again writable, through a recompute.
> 
> As part of this patch, ovn-installed will not be updated for additional 
> chassis;
> it will only be updated when the migration is completed.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2059253
> Signed-off-by: Xavier Simonart 
> 
> ---

Hi Xavier,

Thanks for the new revision!  It looks good to me.  I only have two very
minor comments below and I guess the maintainer applying the patch can
address that when pushing.   So, unless you need a new revision for
other reasons:

Acked-by: Dumitru Ceara 

Thanks,
Dumitru

> v2:  - handled Dumitru's comments.
>  - handled Han's comments, mainly ensure we moved out of CLAIMED state
>only after updating pb->chassis to guarentee physical flows are 
> installed
>when ovn-installed is updated in OVS.
>  - slighly reorganize the code to isolate 'notify_up = false' cases in
>claim_port (i.e. ports such as virtual ports), in the idea of making
>future patch preventing recomputes when virtual ports are claimed.
>  - updated test case to cause more race conditions.
>  - rebased on origin/main
>  - note that "additional chassis" as now supported by
>"Support LSP:options:requested-chassis as a list" might still cause
>recomputes.
>  - fixed missing flows when Port_Binding chassis was updated by mgr_update
>w/o any lflow recalculation.
> v3:  - handled Dumitru's comments on v2, mainly have runtime_data handler
>handling pb_claims when sb becomes writable (instead of a lflow 
> handler).
>  - fixed test as it was not checking recomputes on all hv, as well as a 
> flaky
>behavior.
>  - rebased on origin/main.
> ---
>  controller/binding.c| 154 +--
>  controller/binding.h|  15 +++-
>  controller/if-status.c  | 174 
>  controller/if-status.h  |  16 +++-
>  controller/ovn-controller.c |  72 ++-
>  tests/ovn-macros.at |  12 +++
>  tests/ovn.at| 147 +-
>  tests/perf-northd.at|  17 
>  8 files changed, 519 insertions(+), 88 deletions(-)
> 
> diff --git a/controller/binding.c b/controller/binding.c
> index 2279570f9..b21577f71 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -644,11 +644,17 @@ local_binding_get_lport_ofport(const struct shash 
> *local_bindings,
>  }
>  
>  bool
> -local_binding_is_up(struct shash *local_bindings, const char *pb_name)
> +local_binding_is_up(struct shash *local_bindings, const char *pb_name,
> +const struct sbrec_chassis *chassis_rec)
>  {
>  struct local_binding *lbinding =
>  local_binding_find(local_bindings, pb_name);
>  struct binding_lport *b_lport = 
> local_binding_get_primary_lport(lbinding);
> +
> +if (b_lport && b_lport->pb->chassis != chassis_rec) {
> +return false;
> +}
> +
>  if (lbinding && b_lport && lbinding->iface) {
>  if (b_lport->pb->n_up && !b_lport->pb->up[0]) {
>  return false;
> @@ -660,13 +666,23 @@ local_binding_is_up(struct shash *local_bindings, const 
> char *pb_name)
>  }
>  
>  bool
> -local_binding_is_down(struct shash *local_bindings, const char *pb_name)
> +local_binding_is_down(struct shash *local_bindings, const char *pb_name,
> +  const struct sbrec_chassis *chassis_rec)
>  {
>  struct local_binding *lbinding =
>  local_binding_find(local_bindings, pb_name);
>  
>  struct binding_lport *b_lport = 
> local_binding_get_primary_lport(lbinding);
>  
> +if (b_lport) {
> +if (b_lport->pb->chassis == chassis_rec) {
> +return false;
> +} else if (b_lport->pb->chassis) {
> +VLOG_DBG("lport %s already claimed by other 

Re: [ovs-dev] [v5 8/8] dpif-netlink: Offloading meter to tc police action

[Eelco Chaudron]

On 28 Jun 2022, at 10:52, Jianbo Liu wrote:

> On Tue, 2022-06-28 at 09:52 +0200, Eelco Chaudron wrote:
>>
>>
>> On 27 Jun 2022, at 16:58, Jianbo Liu wrote:
>>
>>> On Mon, 2022-06-27 at 11:32 +0200, Eelco Chaudron wrote:


 On 21 Jun 2022, at 10:22, Jianbo Liu wrote:

> On Mon, 2022-06-20 at 12:18 +0200, Eelco Chaudron wrote:
>> On 27 May 2022, at 11:00, Jianbo Liu wrote:
>>
>>> OVS meters are created in advance and openflow rules refer
>>> to
>>> them
>>> by
>>> their unique ID. New tc_police API is used to offload them.
>>> By
>>> calling
>>> the API, police actions are created and meters are mapped
>>> to
>>> them.
>>> These actions then can be used in tc filter rules by the
>>> index.
>>>
>>> Signed-off-by: Jianbo Liu 
>>> ---
>>>  NEWS |  2 ++
>>>  lib/dpif-netlink.c   | 31 +---
>>> -
>>>  tests/system-offloads-traffic.at | 48
>>> 
>>>  3 files changed, 76 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/NEWS b/NEWS
>>> index eece0d0b2..dfd659d4e 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -29,6 +29,8 @@ Post-v2.17.0
>>>     - Windows:
>>>   * Conntrack support for TCPv6, UDPv6, ICMPv6, FTPv6.
>>>   * IPv6 Geneve tunnel support.
>>> +   - Linux datapath:
>>> + * Add offloading meter tc police.
>>>
>>>
>>>  v2.17.0 - 17 Feb 2022
>>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>>> index 06e1e8ca0..0af9ee77e 100644
>>> --- a/lib/dpif-netlink.c
>>> +++ b/lib/dpif-netlink.c
>>> @@ -4163,11 +4163,18 @@ static int
>>>  dpif_netlink_meter_set(struct dpif *dpif_,
>>> ofproto_meter_id
>>> meter_id,
>>>     struct ofputil_meter_config
>>> *config)
>>>  {
>>> +    int err;
>>> +
>>>  if (probe_broken_meters(dpif_)) {
>>>  return ENOMEM;
>>>  }
>>>
>>> -    return dpif_netlink_meter_set__(dpif_, meter_id,
>>> config);
>>> +    err = dpif_netlink_meter_set__(dpif_, meter_id,
>>> config);
>>> +    if (!err && netdev_is_flow_api_enabled()) {
>>> +    meter_offload_set(meter_id, config);
>>
>> Although currently we always return 0, we should still
>> account
>> for it
>> to change in the future, so we should set err to the return
>> value.
>>
>
> If there is err from meter_offload_set, it will be passed to
> the
> caller
> of dpif_netlink_meter_set(). I don't agree with that, because
> the
> caller thinks meter_set operation fail, but actually not.
> Besides,
> we
> allow the case that dp meter_set success, but offloading fails,
> so
> the
> return the error of meter_offload_set seems unnecessary.

 If this is the case, we should change the
 dpif_netlink_meter_set()
 API to return void.
 And add a comment to the function why it would not return an
 error:

>>>
>>> OK.
>>>
 --- a/lib/netdev-offload.c
 +++ b/lib/netdev-offload.c
 @@ -196,7 +196,7 @@ netdev_assign_flow_api(struct netdev *netdev)
  return -1;
  }

 -int
 +void
  meter_offload_set(ofproto_meter_id meter_id,
    struct ofputil_meter_config *config)
  {
 @@ -212,8 +212,8 @@ meter_offload_set(ofproto_meter_id meter_id,
     }
  }
  }
 -
 -    return 0;
 +    /* Offload APIs could fail, for example, because the offload
 is
 not
 + * supported. This is fine, as the offload API should take
 care
 of this. */
  }



>>  +    err = meter_offload_set(meter_id, config);
>>
>>> +    }
>>> +

 

>>> diff --git a/tests/system-offloads-traffic.at
>>> b/tests/system-
>>> offloads-traffic.at
>>> index 80bc1dd5c..7ec75340f 100644
>>> --- a/tests/system-offloads-traffic.at
>>> +++ b/tests/system-offloads-traffic.at
>>> @@ -168,3 +168,51 @@ matchall
>>>  ])
>>>  OVS_TRAFFIC_VSWITCHD_STOP
>>>  AT_CLEANUP
>>> +
>>> +AT_SETUP([offloads - check if meter offloading ])
>>
>> Can we replace if with interface, as I keep on reading it as
>> "if".
>>
>
> Sure.
>
>>> +AT_KEYWORDS([meter])
>>> +AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
>>> +OVS_TRAFFIC_VSWITCHD_START()
>>> +
>>> +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-
>>> offload=true])
>>> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1
>>> pktps
>>> bands=type=drop rate=1'])
>>> +
>>> +ADD_NAMESPACES(at_ns0, at_ns1)
>>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24",
>>> "f0:00:00:01:01:01")
>>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24",
>>> "f0:00:00:01:01:02")
>>> +
>>> 

Re: [ovs-dev] [v5 8/8] dpif-netlink: Offloading meter to tc police action

[Eelco Chaudron]

On 28 Jun 2022, at 10:28, Jianbo Liu wrote:

> On Tue, 2022-06-28 at 09:53 +0200, Eelco Chaudron wrote:
>>
>>
>> On 27 Jun 2022, at 16:58, Jianbo Liu wrote:
>>
>>> On Mon, 2022-06-27 at 11:32 +0200, Eelco Chaudron wrote:


 On 21 Jun 2022, at 10:22, Jianbo Liu wrote:

> On Mon, 2022-06-20 at 12:18 +0200, Eelco Chaudron wrote:
>> On 27 May 2022, at 11:00, Jianbo Liu wrote:
>>
>>> OVS meters are created in advance and openflow rules refer
>>> to
>>> them
>>> by
>>> their unique ID. New tc_police API is used to offload them.
>>> By
>>> calling
>>> the API, police actions are created and meters are mapped
>>> to
>>> them.
>>> These actions then can be used in tc filter rules by the
>>> index.
>>>
>>> Signed-off-by: Jianbo Liu 
>>> ---
>>>  NEWS |  2 ++
>>>  lib/dpif-netlink.c   | 31 +---
>>> -
>>>  tests/system-offloads-traffic.at | 48
>>> 
>>>  3 files changed, 76 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/NEWS b/NEWS
>>> index eece0d0b2..dfd659d4e 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -29,6 +29,8 @@ Post-v2.17.0
>>>     - Windows:
>>>   * Conntrack support for TCPv6, UDPv6, ICMPv6, FTPv6.
>>>   * IPv6 Geneve tunnel support.
>>> +   - Linux datapath:
>>> + * Add offloading meter tc police.
>>>
>>>
>>>  v2.17.0 - 17 Feb 2022
>>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>>> index 06e1e8ca0..0af9ee77e 100644
>>> --- a/lib/dpif-netlink.c
>>> +++ b/lib/dpif-netlink.c
>>> @@ -4163,11 +4163,18 @@ static int
>>>  dpif_netlink_meter_set(struct dpif *dpif_,
>>> ofproto_meter_id
>>> meter_id,
>>>     struct ofputil_meter_config
>>> *config)
>>>  {
>>> +    int err;
>>> +
>>>  if (probe_broken_meters(dpif_)) {
>>>  return ENOMEM;
>>>  }
>>>
>>> -    return dpif_netlink_meter_set__(dpif_, meter_id,
>>> config);
>>> +    err = dpif_netlink_meter_set__(dpif_, meter_id,
>>> config);
>>> +    if (!err && netdev_is_flow_api_enabled()) {
>>> +    meter_offload_set(meter_id, config);
>>
>> Although currently we always return 0, we should still
>> account
>> for it
>> to change in the future, so we should set err to the return
>> value.
>>
>
> If there is err from meter_offload_set, it will be passed to
> the
> caller
> of dpif_netlink_meter_set(). I don't agree with that, because
> the
> caller thinks meter_set operation fail, but actually not.
> Besides,
> we
> allow the case that dp meter_set success, but offloading fails,
> so
> the
> return the error of meter_offload_set seems unnecessary.

 If this is the case, we should change the
 dpif_netlink_meter_set()
 API to return void.
 And add a comment to the function why it would not return an
 error:

>>>
>>> OK.
>>>
 --- a/lib/netdev-offload.c
 +++ b/lib/netdev-offload.c
 @@ -196,7 +196,7 @@ netdev_assign_flow_api(struct netdev *netdev)
  return -1;
  }

 -int
 +void
  meter_offload_set(ofproto_meter_id meter_id,
    struct ofputil_meter_config *config)
  {
 @@ -212,8 +212,8 @@ meter_offload_set(ofproto_meter_id meter_id,
     }
  }
  }
 -
 -    return 0;
 +    /* Offload APIs could fail, for example, because the offload
 is
 not
 + * supported. This is fine, as the offload API should take
 care
 of this. */
  }



>>  +    err = meter_offload_set(meter_id, config);
>>
>>> +    }
>>> +

 

>>> diff --git a/tests/system-offloads-traffic.at
>>> b/tests/system-
>>> offloads-traffic.at
>>> index 80bc1dd5c..7ec75340f 100644
>>> --- a/tests/system-offloads-traffic.at
>>> +++ b/tests/system-offloads-traffic.at
>>> @@ -168,3 +168,51 @@ matchall
>>>  ])
>>>  OVS_TRAFFIC_VSWITCHD_STOP
>>>  AT_CLEANUP
>>> +
>>> +AT_SETUP([offloads - check if meter offloading ])
>>
>> Can we replace if with interface, as I keep on reading it as
>> "if".
>>
>
> Sure.
>
>>> +AT_KEYWORDS([meter])
>>> +AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
>>> +OVS_TRAFFIC_VSWITCHD_START()
>>> +
>>> +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-
>>> offload=true])
>>> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1
>>> pktps
>>> bands=type=drop rate=1'])
>>> +
>>> +ADD_NAMESPACES(at_ns0, at_ns1)
>>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24",
>>> "f0:00:00:01:01:01")
>>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24",
>>> "f0:00:00:01:01:02")
>>> +
>>> 

Re: [ovs-dev] [PATCH v4] ofproto-dpif-xlate: No clone when tunnel push is last action

[Ilya Maximets]

On 6/3/22 17:31, Rosemarie O'Riorden wrote:
> When OVS sees a tunnel push with a nested list next, it will not
> clone the packet, as a clone is not needed. However, a clone action will
> still be created with the tunnel push encapulated inside. There is no
> need to create the clone action in this case, as extra parsing will need
> to be performed, which is less efficient.
> 
> Signed-off-by: Rosemarie O'Riorden 
> ---
> v2:
>  - Fixes tests that are affected by the change
> v3:
>  - Adds support for hardware offloading
> v4:
>  - Adds negative check in test
>  - Fixes logic in function native_tunnel_output
>  - Stylistic and organizational improvements
> 
>  lib/netdev-offload-dpdk.c | 38 +-
>  lib/netlink.c |  7 +++
>  lib/netlink.h |  1 +
>  ofproto/ofproto-dpif-xlate.c  | 22 ++--
>  tests/nsh.at  |  6 +--
>  tests/packet-type-aware.at| 24 -
>  tests/tunnel-push-pop-ipv6.at | 20 +++
>  tests/tunnel-push-pop.at  | 98 ---
>  8 files changed, 152 insertions(+), 64 deletions(-)
> 



> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 9ea21edc4..a908160a5 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3739,7 +3739,11 @@ native_tunnel_output(struct xlate_ctx *ctx, const 
> struct xport *xport,
>  size_t clone_ofs = 0;
>  size_t push_action_size;
>  
> -clone_ofs = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
> +if (!is_last_action) {
> +clone_ofs = nl_msg_start_nested(ctx->odp_actions,
> +OVS_ACTION_ATTR_CLONE);
> +}
> +size_t pre_patch_port_outp_size = ctx->odp_actions->size;

Hi, Rosemarie.  Thanks for addressing previous comments!
This version looks correct to me.  I also did some tests
with dummy offload and they seem to work fine.

The only thing I think we can improve is the variable name
here.  I think, it will be a bit cleaner if we will rename
and re-use the same variable for the offset in both cases.
Maybe something like this:

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 0c92ed922..c645c3470 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3736,14 +3736,12 @@ native_tunnel_output(struct xlate_ctx *ctx, const 
struct xport *xport,
   s_ip, tnl_params.is_ipv6,
   tnl_push_data.tnl_type);
 
-size_t clone_ofs = 0;
+size_t offset;
 size_t push_action_size;
 
-if (!is_last_action) {
-clone_ofs = nl_msg_start_nested(ctx->odp_actions,
-OVS_ACTION_ATTR_CLONE);
-}
-size_t pre_patch_port_outp_size = ctx->odp_actions->size;
+offset = is_last_action
+ ? ctx->odp_actions->size
+ : nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
 odp_put_tnl_push_action(ctx->odp_actions, _push_data);
 push_action_size = ctx->odp_actions->size;
 
---

What do you think?

If that looks good to you, I can just apply the diff above and
rename the variable in other places while applying the change.

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

Re: [ovs-dev] [PATCH ovn v3 1/1] Allow arbitrary args to be passed to called binary

[Dumitru Ceara]

On 6/27/22 18:00, Terry Wilson wrote:
> It is common to pass ovn-ctl options via an /etc/sysconfig file.
> It would be useful to be able to pass custom --remote options or
> additional DBs to listen to via this file. This would give end
> users the ability to have more fine-grained control without
> having to modify ovn-ctl.
> 
> Signed-off-by: Terry Wilson 
> ---

Hi Terry,

For the change itself:

Acked-by: Dumitru Ceara 

While we can make it work for now with this new arbitrary args feature,
for the actual goal, of passing a custom --remote option and additional
DBs to listen to I think we need a follow up.  Currently, when starting
OVN databases, if the database doesn't exist ovn-ctl takes care of
creating it:

https://github.com/ovn-org/ovn/blob/86684ad759b4f517b8f34fbf7ba8bb8ed77bb575/utilities/ovn-ctl#L246
(raft)

https://github.com/ovn-org/ovn/blob/86684ad759b4f517b8f34fbf7ba8bb8ed77bb575/utilities/ovn-ctl#L249
(standalone)

In the future, after [0] is accepted, can we add a patch that changes
ovn-ctl to check if the local_config.ovsschema file is installed?  If it
is and if DB_*_USE_REMOTE_IN_DB=yes it could then create the
corresponding local_config database if not already found in $ovn_dbdir
and automatically add the --remote and new db file args.

What do you think?

Thanks,
Dumitru

>  utilities/ovn-ctl   | 22 ++
>  utilities/ovn-ctl.8.xml | 14 +-
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl
> index 23d4d7f8c..93be9b84b 100755
> --- a/utilities/ovn-ctl
> +++ b/utilities/ovn-ctl
> @@ -311,6 +311,10 @@ $cluster_remote_port
>  set "$@" --sync-from=`cat $active_conf_file`
>  fi
>  
> +if test X"$extra_args" != X; then
> +set "$@" $extra_args
> +fi
> +
>  local run_ovsdb_in_bg="no"
>  local process_id=
>  if test X$detach = Xno && test $mode = cluster && test -z 
> "$cluster_remote_addr" ; then
> @@ -541,6 +545,10 @@ start_ic () {
>  
>  set "$@" $OVN_IC_LOG $ovn_ic_params
>  
> +if test X"$extra_args" != X; then
> +set "$@" $extra_args
> +fi
> +
>  OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_IC_PRIORITY" 
> "$OVN_IC_WRAPPER" "$@"
>  fi
>  }
> @@ -563,6 +571,10 @@ start_controller () {
>  
>  [ "$OVN_USER" != "" ] && set "$@" --user "$OVN_USER"
>  
> +if test X"$extra_args" != X; then
> +set "$@" $extra_args
> +fi
> +
>  OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_CONTROLLER_PRIORITY" 
> "$OVN_CONTROLLER_WRAPPER" "$@"
>  }
>  
> @@ -590,6 +602,10 @@ start_controller_vtep () {
>  
>  [ "$OVN_USER" != "" ] && set "$@" --user "$OVN_USER"
>  
> +if test X"$extra_args" != X; then
> +set "$@" $extra_args
> +fi
> +
>  OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_CONTROLLER_PRIORITY" 
> "$OVN_CONTROLLER_WRAPPER" "$@"
>  }
>  
> @@ -1106,8 +1122,10 @@ EOF
>  
>  set_defaults
>  command=
> +extra_args=
>  for arg
>  do
> +shift
>  case $arg in
>  -h | --help)
>  usage
> @@ -1130,6 +1148,10 @@ do
>  type=bool
>  set_option
>  ;;
> +--)
> +extra_args=$@
> +break
> +;;
>  -*)
>  echo >&2 "$0: unknown option \"$arg\" (use --help for help)"
>  exit 1
> diff --git a/utilities/ovn-ctl.8.xml b/utilities/ovn-ctl.8.xml
> index a1d39b22b..42d16fabc 100644
> --- a/utilities/ovn-ctl.8.xml
> +++ b/utilities/ovn-ctl.8.xml
> @@ -4,7 +4,10 @@
>  ovn-ctl -- Open Virtual Network northbound daemon lifecycle 
> utility
>  
>  Synopsis
> -ovn-ctl [options] command
> +
> +  ovn-ctl [options] command
> +  [--- extra_args]
> +
>  
>  Description
>  This program is intended to be invoked internally by Open Virtual 
> Network
> @@ -156,6 +159,15 @@
>  --db-nb-probe-interval-to-active=Time in 
> milliseconds
>  --db-sb-probe-interval-to-active=Time in 
> milliseconds
>  
> + Extra Options 
> +
> +  Any options after '--' will be passed on to the binary run by
> +  command with the exception of start_northd, which can have
> +  options specified in ovn-northd-db-params.conf. Any 
> extra_args
> +  passed to start_northd will be passed to the ovsdb-servers if
> +  --ovn-manage-ovsdb=yes
> +
> +
>  Configuration files
>  Following are the optional configuration files. If present, it should 
> be located in the etc dir
>  

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

Re: [ovs-dev] [PATCH ovsdb v2 1/1] Add Local_Config schema

[Dumitru Ceara]

On 6/27/22 17:22, Terry Wilson wrote:
> The only way to configure settings on a remote (e.g. inactivity_probe)
> is via --remote=db:DB,table,row. There is no way to do this via the
> existing CLI options.
> 
> For a clustered DB with multiple servers listening on unique addresses
> there is no way to store these entries in the DB as the DB is shared.
> For example, three servers listening on  1.1.1.1, 1.1.1.2, and 1.1.1.3
> respectively would require a Manager/Connection row each, but then
> all three servers would try to listen on all three addresses.
> 
> It is possible for ovsdb-server to serve multiple databases. This
> means that we can have a local "config" database in addition to
> the main database we are servering (Open_vSwitch, OVN_Southbound, etc.)
> and this patch adds a Local_Config schema that currently just mirrors
> the Connection table and a Config table with a 'connections' row that
> stores each Connection.
> 
> Signed-off-by: Terry Wilson 
> ---

Hi Terry,

Overall the change looks good to me, just a few small comments below.

>  NEWS   |   5 +
>  debian/openvswitch-common.manpages |   1 +
>  ovsdb/.gitignore   |   2 +
>  ovsdb/automake.mk  |  20 ++
>  ovsdb/local_config.ovsschema   |  43 +
>  ovsdb/local_config.xml | 281 +
>  rhel/openvswitch-fedora.spec.in|   1 +
>  tests/ovsdb-cluster.at |  42 -
>  8 files changed, 388 insertions(+), 7 deletions(-)
>  create mode 100644 ovsdb/local_config.ovsschema

Nit: At least in OVN, we use dashes instead of underscores for all the
schema file names.

>  create mode 100644 ovsdb/local_config.xml
> 
> diff --git a/NEWS b/NEWS
> index 9fe3f44f4..b9e6f0216 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -32,6 +32,11 @@ Post-v2.17.0
> - DPDK:
>   * OVS validated with DPDK 21.11.1.  It is recommended to use this 
> version
> until further releases.
> +   - Local_Config schema added:
> + * Clustered ovsdb-servers listening on unique addresses can pass a 
> second
> +   DB created with the new Local_Config schema in order to configure 
> their
> +   Connections independent from each other. See the ovsdb.local-config.5
> +   manpage for schema details.
>  
>  
>  v2.17.0 - 17 Feb 2022
> diff --git a/debian/openvswitch-common.manpages 
> b/debian/openvswitch-common.manpages
> index 95004122c..7c46a2acf 100644
> --- a/debian/openvswitch-common.manpages
> +++ b/debian/openvswitch-common.manpages
> @@ -5,3 +5,4 @@ debian/tmp/usr/share/man/man8/ovs-appctl.8
>  utilities/ovs-ofctl.8
>  debian/tmp/usr/share/man/man8/ovs-parse-backtrace.8
>  debian/tmp/usr/share/man/man8/ovs-pki.8
> +ovsdb/ovsdb.local-config.5
> diff --git a/ovsdb/.gitignore b/ovsdb/.gitignore
> index fbcefafc6..747f119a9 100644
> --- a/ovsdb/.gitignore
> +++ b/ovsdb/.gitignore
> @@ -1,5 +1,7 @@
>  /_server.ovsschema.inc
>  /_server.ovsschema.stamp
> +/local_config.ovsschema.stamp
> +/ovsdb.local-config.5
>  /ovsdb-client
>  /ovsdb-client.1
>  /ovsdb-doc
> diff --git a/ovsdb/automake.mk b/ovsdb/automake.mk
> index 62cc02686..c53675a87 100644
> --- a/ovsdb/automake.mk
> +++ b/ovsdb/automake.mk
> @@ -148,4 +148,24 @@ ovsdb/ovsdb-server.5: \
>   $(srcdir)/ovsdb/_server.xml > $@.tmp && \
>   mv $@.tmp $@
>  
> +EXTRA_DIST += ovsdb/local_config.ovsschema

It's probably useful to install the schema for users to find it in a
common place.  I think we can add:

pkgdata_DATA += ovsdb/local_config.ovsschema

Then "make install" will install it along with the other schema (vtep,
vswitch).

> +
> +# Version checking for local_config.ovsschema.
> +ALL_LOCAL += ovsdb/local_config.ovsschema.stamp
> +ovsdb/local_config.ovsschema.stamp: ovsdb/local_config.ovsschema
> + $(srcdir)/build-aux/cksum-schema-check $? $@
> +CLEANFILES += ovsdb/local_config.ovsschema.stamp
> +
> +# Local_Config schema documentation
> +EXTRA_DIST += ovsdb/local_config.xml
> +CLEANFILES += ovsdb/ovsdb.local-config.5
> +man_MANS += ovsdb/ovsdb.local-config.5
> +ovsdb/ovsdb.local-config.5: \
> + ovsdb/ovsdb-doc ovsdb/ ovsdb/local_config.xml 
> ovsdb/local_config.ovsschema
> + $(AM_V_GEN)$(OVSDB_DOC) \
> + --version=$(VERSION) \
> + $(srcdir)/ovsdb/local_config.ovsschema \
> + $(srcdir)/ovsdb/local_config.xml > $@.tmp && \
> + mv $@.tmp $@
> +
>  EXTRA_DIST += ovsdb/TODO.rst
> diff --git a/ovsdb/local_config.ovsschema b/ovsdb/local_config.ovsschema
> new file mode 100644
> index 0..bd86d0f4f
> --- /dev/null
> +++ b/ovsdb/local_config.ovsschema
> @@ -0,0 +1,43 @@
> +{
> +"name": "Local_Config",
> +"version": "1.0.0",
> +"cksum": "2048726482 1858",
> +"tables": {
> +"Config": {
> +"columns": {
> +"connections": {
> +"type": {"key": {"type": "uuid",
> + "refTable": "Connection"},
> +   

[ovs-dev] [PATCH v3 4/5] dpif-netdev: Add function pointer for dpif re-circulate.

[Kumar Amber]

The patch adds and re-uses the dpif set command to set the
function pointers to be used to switch between different inner
dpifs.

Signed-off-by: Kumar Amber 
Signed-off-by: Cian Ferriter 
Co-authored-by: Cian Ferriter 

---
v3:
- Add description  to the dpif recirc function.
- Fix use of return value to fall back to scalar dpif.
---
---
 lib/dpif-netdev-private-dpif.c   | 57 +++-
 lib/dpif-netdev-private-dpif.h   | 18 ++
 lib/dpif-netdev-private-thread.h |  3 ++
 lib/dpif-netdev.c| 19 +--
 4 files changed, 87 insertions(+), 10 deletions(-)

diff --git a/lib/dpif-netdev-private-dpif.c b/lib/dpif-netdev-private-dpif.c
index 2dc51270a..96bfd4824 100644
--- a/lib/dpif-netdev-private-dpif.c
+++ b/lib/dpif-netdev-private-dpif.c
@@ -27,6 +27,8 @@
 #include "util.h"
 
 VLOG_DEFINE_THIS_MODULE(dpif_netdev_impl);
+#define DPIF_NETDEV_IMPL_AVX512_CHECK (__x86_64__ && HAVE_AVX512F \
+&& HAVE_LD_AVX512_GOOD && __SSE4_2__)
 
 DEFINE_EXTERN_PER_THREAD_DATA(recirc_depth, 0);
 
@@ -39,18 +41,21 @@ enum dpif_netdev_impl_info_idx {
 static struct dpif_netdev_impl_info_t dpif_impls[] = {
 /* The default scalar C code implementation. */
 [DPIF_NETDEV_IMPL_SCALAR] = { .input_func = dp_netdev_input,
+  .recirc_func = dp_netdev_recirculate,
   .probe = NULL,
   .name = "dpif_scalar", },
 
 #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
 /* Only available on x86_64 bit builds with SSE 4.2 used for OVS core. */
 [DPIF_NETDEV_IMPL_AVX512] = { .input_func = dp_netdev_input_avx512,
+  .recirc_func = dp_netdev_input_avx512_recirc,
   .probe = dp_netdev_input_avx512_probe,
   .name = "dpif_avx512", },
 #endif
 };
 
 static dp_netdev_input_func default_dpif_func;
+static dp_netdev_recirc_func default_dpif_recirc_func;
 
 dp_netdev_input_func
 dp_netdev_impl_get_default(void)
@@ -61,7 +66,7 @@ dp_netdev_impl_get_default(void)
 int dpif_idx = DPIF_NETDEV_IMPL_SCALAR;
 
 /* Configure-time overriding to run test suite on all implementations. */
-#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
+#if DPIF_NETDEV_IMPL_AVX512_CHECK
 #ifdef DPIF_AVX512_DEFAULT
 dp_netdev_input_func_probe probe;
 
@@ -81,6 +86,35 @@ dp_netdev_impl_get_default(void)
 return default_dpif_func;
 }
 
+dp_netdev_recirc_func
+dp_netdev_recirc_impl_get_default(void)
+{
+/* For the first call, this will be NULL. Compute the compile time default.
+ */
+if (!default_dpif_recirc_func) {
+int dpif_idx = DPIF_NETDEV_IMPL_SCALAR;
+
+/* Configure-time overriding to run test suite on all implementations. */
+#if DPIF_NETDEV_IMPL_AVX512_CHECK
+#ifdef DPIF_AVX512_DEFAULT
+dp_netdev_input_func_probe probe;
+
+/* Check if the compiled default is compatible. */
+probe = dpif_impls[DPIF_NETDEV_IMPL_AVX512].probe;
+if (!probe || !probe()) {
+dpif_idx = DPIF_NETDEV_IMPL_AVX512;
+}
+#endif
+#endif
+
+VLOG_INFO("Default re-circulate DPIF implementation is %s.\n",
+  dpif_impls[dpif_idx].name);
+default_dpif_recirc_func = dpif_impls[dpif_idx].recirc_func;
+}
+
+return default_dpif_recirc_func;
+}
+
 void
 dp_netdev_impl_get(struct ds *reply, struct dp_netdev_pmd_thread **pmd_list,
size_t n)
@@ -116,10 +150,12 @@ dp_netdev_impl_get(struct ds *reply, struct 
dp_netdev_pmd_thread **pmd_list,
  * returns the function pointer to the one requested by "name".
  */
 static int32_t
-dp_netdev_impl_get_by_name(const char *name, dp_netdev_input_func *out_func)
+dp_netdev_impl_get_by_name(const char *name, dp_netdev_input_func *dpif_func,
+   dp_netdev_recirc_func *dpif_recirc_func)
 {
 ovs_assert(name);
-ovs_assert(out_func);
+ovs_assert(dpif_func);
+ovs_assert(dpif_recirc_func);
 
 uint32_t i;
 
@@ -129,11 +165,13 @@ dp_netdev_impl_get_by_name(const char *name, 
dp_netdev_input_func *out_func)
 if (dpif_impls[i].probe) {
 int probe_err = dpif_impls[i].probe();
 if (probe_err) {
-*out_func = NULL;
+*dpif_func = NULL;
+*dpif_recirc_func = NULL;
 return probe_err;
 }
 }
-*out_func = dpif_impls[i].input_func;
+*dpif_func = dpif_impls[i].input_func;
+*dpif_recirc_func = dpif_impls[i].recirc_func;
 return 0;
 }
 }
@@ -144,12 +182,15 @@ dp_netdev_impl_get_by_name(const char *name, 
dp_netdev_input_func *out_func)
 int32_t
 dp_netdev_impl_set_default_by_name(const char *name)
 {
-dp_netdev_input_func new_default;
+dp_netdev_input_func new_dpif_default;
+dp_netdev_recirc_func new_dpif_recirc_default;
 
-int32_t err = dp_netdev_impl_get_by_name(name, _default);
+int32_t err = dp_netdev_impl_get_by_name(name, _dpif_default,
+ 

[ovs-dev] [PATCH v3 5/5] dpif-netdev-avx512: Add inner packet handling to dpif.

[Kumar Amber]

This patch adds the necessary changes required to support
tunnel packet types in avx512 dpif.

Signed-off-by: Kumar Amber 
Signed-off-by: Cian Ferriter 
Co-authored-by: Cian Ferriter 

---
v3:
- Apply in_port optimization suggested by Harry.
---
---
 lib/dpif-netdev-avx512.c | 25 +++--
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
index ecf512651..8a1bf3ad6 100644
--- a/lib/dpif-netdev-avx512.c
+++ b/lib/dpif-netdev-avx512.c
@@ -80,7 +80,7 @@ dp_netdev_input_avx512_probe(void)
 static inline int32_t ALWAYS_INLINE
 dp_netdev_input_avx512__(struct dp_netdev_pmd_thread *pmd,
  struct dp_packet_batch *packets,
- bool md_is_valid OVS_UNUSED, odp_port_t in_port)
+ bool md_is_valid, odp_port_t in_port)
 {
 /* Allocate DPIF userdata. */
 if (OVS_UNLIKELY(!pmd->netdev_input_func_userdata)) {
@@ -92,6 +92,7 @@ dp_netdev_input_avx512__(struct dp_netdev_pmd_thread *pmd,
 struct netdev_flow_key *keys = ud->keys;
 struct netdev_flow_key **key_ptrs = ud->key_ptrs;
 struct pkt_flow_meta *pkt_meta = ud->pkt_meta;
+const uint32_t recirc_depth = *recirc_depth_get();
 
 /* The AVX512 DPIF implementation handles rules in a way that is optimized
  * for reducing data-movement between HWOL/EMC/SMC and DPCLS. This is
@@ -179,7 +180,9 @@ dp_netdev_input_avx512__(struct dp_netdev_pmd_thread *pmd,
 
 /* Get packet pointer from bitmask and packet md. */
 struct dp_packet *packet = packets->packets[i];
-pkt_metadata_init(>md, in_port);
+if (!md_is_valid) {
+pkt_metadata_init(>md, in_port);
+}
 
 struct dp_netdev_flow *f = NULL;
 struct netdev_flow_key *key = [i];
@@ -191,7 +194,7 @@ dp_netdev_input_avx512__(struct dp_netdev_pmd_thread *pmd,
 bool mfex_hit = !!(mf_mask & (UINT32_C(1) << i));
 
 /* Check for a partial hardware offload match. */
-if (hwol_enabled) {
+if (hwol_enabled && recirc_depth == 0) {
 if (OVS_UNLIKELY(dp_netdev_hw_flow(pmd, packet, ))) {
 /* Packet restoration failed and it was dropped, do not
  * continue processing. */
@@ -224,7 +227,9 @@ dp_netdev_input_avx512__(struct dp_netdev_pmd_thread *pmd,
 pkt_meta[i].tcp_flags = miniflow_get_tcp_flags(>mf);
 
 key->len = netdev_flow_key_size(miniflow_n_values(>mf));
-key->hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet, >mf);
+key->hash = (md_is_valid == false)
+? dpif_netdev_packet_get_rss_hash_orig_pkt(packet, >mf)
+: dpif_netdev_packet_get_rss_hash(packet, >mf);
 
 if (emc_enabled) {
 f = emc_lookup(>emc_cache, key);
@@ -262,7 +267,13 @@ dp_netdev_input_avx512__(struct dp_netdev_pmd_thread *pmd,
  * dpcls_rules[] array.
  */
 if (dpcls_key_idx > 0) {
-struct dpcls *cls = dp_netdev_pmd_lookup_dpcls(pmd, in_port);
+odp_port_t port_no;
+if (!md_is_valid) {
+port_no = in_port;
+} else {
+port_no = packets->packets[0]->md.in_port.odp_port;
+}
+struct dpcls *cls = dp_netdev_pmd_lookup_dpcls(pmd, port_no);
 if (OVS_UNLIKELY(!cls)) {
 return -1;
 }
@@ -318,7 +329,9 @@ dp_netdev_input_avx512__(struct dp_netdev_pmd_thread *pmd,
 
 /* At this point we don't return error anymore, so commit stats here. */
 uint32_t mfex_hit_cnt = __builtin_popcountll(mf_mask);
-pmd_perf_update_counter(>perf_stats, PMD_STAT_RECV, batch_size);
+pmd_perf_update_counter(>perf_stats,
+md_is_valid ? PMD_STAT_RECIRC : PMD_STAT_RECV,
+batch_size);
 pmd_perf_update_counter(>perf_stats, PMD_STAT_PHWOL_HIT, phwol_hits);
 pmd_perf_update_counter(>perf_stats, PMD_STAT_MFEX_OPT_HIT,
 mfex_hit_cnt);
-- 
2.25.1

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

[ovs-dev] [PATCH v3 3/5] dpif-netdev-avx512: Refactor avx512 dpif and create new APIs.

[Kumar Amber]

Create new APIs for the avx512 DPIF, enabling one baseline
common code to be specialized into DPIF implementations for
"outer" processing, and "recirc" processing.

Signed-off-by: Kumar Amber 
Signed-off-by: Cian Ferriter 
Co-authored-by: Cian Ferriter 

---
v3:
- Fix comments from Harry.
---
---
 lib/dpif-netdev-avx512.c   | 32 +++-
 lib/dpif-netdev-private-dpif.c |  4 ++--
 lib/dpif-netdev-private-dpif.h | 16 
 lib/dpif-netdev.c  |  5 ++---
 4 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
index 11d9a0005..ecf512651 100644
--- a/lib/dpif-netdev-avx512.c
+++ b/lib/dpif-netdev-avx512.c
@@ -59,8 +59,13 @@ struct dpif_userdata {
 struct pkt_flow_meta pkt_meta[NETDEV_MAX_BURST];
 };
 
+static int32_t
+dp_netdev_input_avx512__(struct dp_netdev_pmd_thread *pmd,
+ struct dp_packet_batch *packets,
+ bool md_is_valid, odp_port_t in_port);
+
 int32_t
-dp_netdev_input_outer_avx512_probe(void)
+dp_netdev_input_avx512_probe(void)
 {
 bool avx512f_available = cpu_has_isa(OVS_CPU_ISA_X86_AVX512F);
 bool bmi2_available = cpu_has_isa(OVS_CPU_ISA_X86_BMI2);
@@ -72,10 +77,10 @@ dp_netdev_input_outer_avx512_probe(void)
 return 0;
 }
 
-int32_t
-dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd,
- struct dp_packet_batch *packets,
- odp_port_t in_port)
+static inline int32_t ALWAYS_INLINE
+dp_netdev_input_avx512__(struct dp_netdev_pmd_thread *pmd,
+ struct dp_packet_batch *packets,
+ bool md_is_valid OVS_UNUSED, odp_port_t in_port)
 {
 /* Allocate DPIF userdata. */
 if (OVS_UNLIKELY(!pmd->netdev_input_func_userdata)) {
@@ -380,5 +385,22 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread 
*pmd,
 return 0;
 }
 
+int32_t
+dp_netdev_input_avx512(struct dp_netdev_pmd_thread *pmd,
+   struct dp_packet_batch *packets,
+   odp_port_t in_port)
+{
+int ret = dp_netdev_input_avx512__(pmd, packets, false, in_port);
+return ret;
+}
+
+int32_t
+dp_netdev_input_avx512_recirc(struct dp_netdev_pmd_thread *pmd,
+  struct dp_packet_batch *packets)
+{
+int ret = dp_netdev_input_avx512__(pmd, packets, true, 0);
+return ret;
+}
+
 #endif
 #endif
diff --git a/lib/dpif-netdev-private-dpif.c b/lib/dpif-netdev-private-dpif.c
index 6f8de9094..2dc51270a 100644
--- a/lib/dpif-netdev-private-dpif.c
+++ b/lib/dpif-netdev-private-dpif.c
@@ -44,8 +44,8 @@ static struct dpif_netdev_impl_info_t dpif_impls[] = {
 
 #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
 /* Only available on x86_64 bit builds with SSE 4.2 used for OVS core. */
-[DPIF_NETDEV_IMPL_AVX512] = { .input_func = dp_netdev_input_outer_avx512,
-  .probe = dp_netdev_input_outer_avx512_probe,
+[DPIF_NETDEV_IMPL_AVX512] = { .input_func = dp_netdev_input_avx512,
+  .probe = dp_netdev_input_avx512_probe,
   .name = "dpif_avx512", },
 #endif
 };
diff --git a/lib/dpif-netdev-private-dpif.h b/lib/dpif-netdev-private-dpif.h
index 15f1f36b3..37908de9a 100644
--- a/lib/dpif-netdev-private-dpif.h
+++ b/lib/dpif-netdev-private-dpif.h
@@ -74,11 +74,19 @@ dp_netdev_input(struct dp_netdev_pmd_thread *pmd,
 
 /* AVX512 enabled DPIF implementation and probe functions. */
 int32_t
-dp_netdev_input_outer_avx512_probe(void);
+dp_netdev_input_avx512_probe(void);
 
 int32_t
-dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd,
- struct dp_packet_batch *packets,
- odp_port_t in_port);
+dp_netdev_input_avx512(struct dp_netdev_pmd_thread *pmd,
+   struct dp_packet_batch *packets,
+   odp_port_t in_port);
+
+int32_t
+dp_netdev_input_avx512_recirc(struct dp_netdev_pmd_thread *pmd,
+  struct dp_packet_batch *packets);
+
+int32_t
+dp_netdev_recirculate(struct dp_netdev_pmd_thread *,
+  struct dp_packet_batch *);
 
 #endif /* netdev-private.h */
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index c4e47f715..ea95acde0 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -545,8 +545,6 @@ static void dp_netdev_execute_actions(struct 
dp_netdev_pmd_thread *pmd,
   const struct flow *flow,
   const struct nlattr *actions,
   size_t actions_len);
-static void dp_netdev_recirculate(struct dp_netdev_pmd_thread *,
-  struct dp_packet_batch *);
 
 static void dp_netdev_disable_upcall(struct dp_netdev *);
 static void dp_netdev_pmd_reload_done(struct dp_netdev_pmd_thread *pmd);
@@ -8493,11 +8491,12 @@ dp_netdev_input(struct dp_netdev_pmd_thread *pmd,
 return 0;
 }
 
-static void

[ovs-dev] [PATCH v3 1/5] dpif-netdev: Refactor per thread recirc data allocation.

[Kumar Amber]

The refactor allows us to use *recirc_depth_get() to obtain
the depth across ovs which was previously limited to only
dpif-netdev.c.

Signed-off-by: Kumar Amber 
Signed-off-by: Cian Ferriter 
Co-authored-by: Cian Ferriter 
Acked-by: Harry van Haaren 
---
 lib/dpif-netdev-private-dpif.c | 2 ++
 lib/dpif-netdev-private-dpif.h | 5 +
 lib/dpif-netdev.c  | 3 ---
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/dpif-netdev-private-dpif.c b/lib/dpif-netdev-private-dpif.c
index 84d4ec156..6f8de9094 100644
--- a/lib/dpif-netdev-private-dpif.c
+++ b/lib/dpif-netdev-private-dpif.c
@@ -28,6 +28,8 @@
 
 VLOG_DEFINE_THIS_MODULE(dpif_netdev_impl);
 
+DEFINE_EXTERN_PER_THREAD_DATA(recirc_depth, 0);
+
 enum dpif_netdev_impl_info_idx {
 DPIF_NETDEV_IMPL_SCALAR,
 DPIF_NETDEV_IMPL_AVX512
diff --git a/lib/dpif-netdev-private-dpif.h b/lib/dpif-netdev-private-dpif.h
index 0da639c55..15f1f36b3 100644
--- a/lib/dpif-netdev-private-dpif.h
+++ b/lib/dpif-netdev-private-dpif.h
@@ -18,6 +18,11 @@
 #define DPIF_NETDEV_PRIVATE_DPIF_H 1
 
 #include "openvswitch/types.h"
+#include "ovs-thread.h"
+
+#define MAX_RECIRC_DEPTH 6
+/* Use per thread recirc_depth to prevent recirculation loop. */
+DECLARE_EXTERN_PER_THREAD_DATA(uint32_t, recirc_depth);
 
 /* Forward declarations to avoid including files. */
 struct dp_netdev_pmd_thread;
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index ff57b3961..f65d9ee8c 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -98,9 +98,6 @@ VLOG_DEFINE_THIS_MODULE(dpif_netdev);
 #define MIN_TO_MSEC  6
 
 #define FLOW_DUMP_MAX_BATCH 50
-/* Use per thread recirc_depth to prevent recirculation loop. */
-#define MAX_RECIRC_DEPTH 6
-DEFINE_STATIC_PER_THREAD_DATA(uint32_t, recirc_depth, 0)
 
 /* Use instant packet send by default. */
 #define DEFAULT_TX_FLUSH_INTERVAL 0
-- 
2.25.1

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

[ovs-dev] [PATCH v3 2/5] dpif-netdev: Refactor hash function to own header.

[Kumar Amber]

The refactor allows us to use hash function accross
multiple files which was earlier restricted to
dpif-netdev.c only.

Signed-off-by: Kumar Amber 
Signed-off-by: Cian Ferriter 
Co-authored-by: Cian Ferriter 

---
v3:
- Fix minor comments from Harry.
---
---
 lib/dpif-netdev-private-dpcls.h | 23 +++
 lib/dpif-netdev.c   | 22 --
 2 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/lib/dpif-netdev-private-dpcls.h b/lib/dpif-netdev-private-dpcls.h
index 2a9279437..1b37ecb16 100644
--- a/lib/dpif-netdev-private-dpcls.h
+++ b/lib/dpif-netdev-private-dpcls.h
@@ -25,6 +25,7 @@
 
 #include "cmap.h"
 #include "openvswitch/thread.h"
+#include "dpif-netdev-private-dpif.h"
 
 #ifdef  __cplusplus
 extern "C" {
@@ -125,6 +126,28 @@ dpif_netdev_packet_get_rss_hash_orig_pkt(struct dp_packet 
*packet,
 return hash;
 }
 
+static inline uint32_t
+dpif_netdev_packet_get_rss_hash(struct dp_packet *packet,
+const struct miniflow *mf)
+{
+uint32_t hash, recirc_depth;
+
+if (OVS_LIKELY(dp_packet_rss_valid(packet))) {
+hash = dp_packet_get_rss_hash(packet);
+} else {
+hash = miniflow_hash_5tuple(mf, 0);
+dp_packet_set_rss_hash(packet, hash);
+}
+
+/* The RSS hash must account for the recirculation depth to avoid
+ * collisions in the exact match cache */
+recirc_depth = *recirc_depth_get_unsafe();
+if (OVS_UNLIKELY(recirc_depth)) {
+hash = hash_finish(hash, recirc_depth);
+}
+return hash;
+}
+
 /* Allow other implementations to call dpcls_lookup() for subtable search. */
 bool
 dpcls_lookup(struct dpcls *cls, const struct netdev_flow_key *keys[],
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index f65d9ee8c..c4e47f715 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -7807,28 +7807,6 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, 
struct dp_packet *packet_,
  actions, wc, put_actions, dp->upcall_aux);
 }
 
-static inline uint32_t
-dpif_netdev_packet_get_rss_hash(struct dp_packet *packet,
-const struct miniflow *mf)
-{
-uint32_t hash, recirc_depth;
-
-if (OVS_LIKELY(dp_packet_rss_valid(packet))) {
-hash = dp_packet_get_rss_hash(packet);
-} else {
-hash = miniflow_hash_5tuple(mf, 0);
-dp_packet_set_rss_hash(packet, hash);
-}
-
-/* The RSS hash must account for the recirculation depth to avoid
- * collisions in the exact match cache */
-recirc_depth = *recirc_depth_get_unsafe();
-if (OVS_UNLIKELY(recirc_depth)) {
-hash = hash_finish(hash, recirc_depth);
-}
-return hash;
-}
-
 struct packet_batch_per_flow {
 unsigned int byte_count;
 uint16_t tcp_flags;
-- 
2.25.1

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

[ovs-dev] [PATCH v3 0/5] DPIF AVX512 Recirculation

[Kumar Amber]

The patch adds support for recirculation of packets in AVX512
DPIF which would allow for processing tunneled packets.

---
v3:
* Fix comments from Harry.
V2:
* Split DPIF AVX512 into separate patchset.
---

Kumar Amber (5):
  dpif-netdev: Refactor per thread recirc data allocation.
  dpif-netdev: Refactor hash function to own header.
  dpif-netdev-avx512: Refactor avx512 dpif and create new APIs.
  dpif-netdev: Add function pointer for dpif re-circulate.
  dpif-netdev-avx512: Add inner packet handling to dpif.

 lib/dpif-netdev-avx512.c | 55 +++-
 lib/dpif-netdev-private-dpcls.h  | 23 
 lib/dpif-netdev-private-dpif.c   | 63 +++-
 lib/dpif-netdev-private-dpif.h   | 39 ++--
 lib/dpif-netdev-private-thread.h |  3 ++
 lib/dpif-netdev.c| 49 ++---
 6 files changed, 178 insertions(+), 54 deletions(-)

-- 
2.25.1

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

Re: [ovs-dev] [v5 8/8] dpif-netlink: Offloading meter to tc police action

[Jianbo Liu via dev]

On Tue, 2022-06-28 at 09:52 +0200, Eelco Chaudron wrote:
> 
> 
> On 27 Jun 2022, at 16:58, Jianbo Liu wrote:
> 
> > On Mon, 2022-06-27 at 11:32 +0200, Eelco Chaudron wrote:
> > > 
> > > 
> > > On 21 Jun 2022, at 10:22, Jianbo Liu wrote:
> > > 
> > > > On Mon, 2022-06-20 at 12:18 +0200, Eelco Chaudron wrote:
> > > > > On 27 May 2022, at 11:00, Jianbo Liu wrote:
> > > > > 
> > > > > > OVS meters are created in advance and openflow rules refer
> > > > > > to
> > > > > > them
> > > > > > by
> > > > > > their unique ID. New tc_police API is used to offload them.
> > > > > > By
> > > > > > calling
> > > > > > the API, police actions are created and meters are mapped
> > > > > > to
> > > > > > them.
> > > > > > These actions then can be used in tc filter rules by the
> > > > > > index.
> > > > > > 
> > > > > > Signed-off-by: Jianbo Liu 
> > > > > > ---
> > > > > >  NEWS |  2 ++
> > > > > >  lib/dpif-netlink.c   | 31 +---
> > > > > > -
> > > > > >  tests/system-offloads-traffic.at | 48
> > > > > > 
> > > > > >  3 files changed, 76 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/NEWS b/NEWS
> > > > > > index eece0d0b2..dfd659d4e 100644
> > > > > > --- a/NEWS
> > > > > > +++ b/NEWS
> > > > > > @@ -29,6 +29,8 @@ Post-v2.17.0
> > > > > >     - Windows:
> > > > > >   * Conntrack support for TCPv6, UDPv6, ICMPv6, FTPv6.
> > > > > >   * IPv6 Geneve tunnel support.
> > > > > > +   - Linux datapath:
> > > > > > + * Add offloading meter tc police.
> > > > > > 
> > > > > > 
> > > > > >  v2.17.0 - 17 Feb 2022
> > > > > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> > > > > > index 06e1e8ca0..0af9ee77e 100644
> > > > > > --- a/lib/dpif-netlink.c
> > > > > > +++ b/lib/dpif-netlink.c
> > > > > > @@ -4163,11 +4163,18 @@ static int
> > > > > >  dpif_netlink_meter_set(struct dpif *dpif_,
> > > > > > ofproto_meter_id
> > > > > > meter_id,
> > > > > >     struct ofputil_meter_config
> > > > > > *config)
> > > > > >  {
> > > > > > +    int err;
> > > > > > +
> > > > > >  if (probe_broken_meters(dpif_)) {
> > > > > >  return ENOMEM;
> > > > > >  }
> > > > > > 
> > > > > > -    return dpif_netlink_meter_set__(dpif_, meter_id,
> > > > > > config);
> > > > > > +    err = dpif_netlink_meter_set__(dpif_, meter_id,
> > > > > > config);
> > > > > > +    if (!err && netdev_is_flow_api_enabled()) {
> > > > > > +    meter_offload_set(meter_id, config);
> > > > > 
> > > > > Although currently we always return 0, we should still
> > > > > account
> > > > > for it
> > > > > to change in the future, so we should set err to the return
> > > > > value.
> > > > > 
> > > > 
> > > > If there is err from meter_offload_set, it will be passed to
> > > > the
> > > > caller
> > > > of dpif_netlink_meter_set(). I don't agree with that, because
> > > > the
> > > > caller thinks meter_set operation fail, but actually not.
> > > > Besides,
> > > > we
> > > > allow the case that dp meter_set success, but offloading fails,
> > > > so
> > > > the
> > > > return the error of meter_offload_set seems unnecessary.
> > > 
> > > If this is the case, we should change the
> > > dpif_netlink_meter_set()
> > > API to return void.
> > > And add a comment to the function why it would not return an
> > > error:
> > > 
> > 
> > OK.
> > 
> > > --- a/lib/netdev-offload.c
> > > +++ b/lib/netdev-offload.c
> > > @@ -196,7 +196,7 @@ netdev_assign_flow_api(struct netdev *netdev)
> > >  return -1;
> > >  }
> > > 
> > > -int
> > > +void
> > >  meter_offload_set(ofproto_meter_id meter_id,
> > >    struct ofputil_meter_config *config)
> > >  {
> > > @@ -212,8 +212,8 @@ meter_offload_set(ofproto_meter_id meter_id,
> > >     }
> > >  }
> > >  }
> > > -
> > > -    return 0;
> > > +    /* Offload APIs could fail, for example, because the offload
> > > is
> > > not
> > > + * supported. This is fine, as the offload API should take
> > > care
> > > of this. */
> > >  }
> > > 
> > > 
> > > 
> > > > >  +    err = meter_offload_set(meter_id, config);
> > > > > 
> > > > > > +    }
> > > > > > +
> > > 
> > > 
> > > 
> > > > > > diff --git a/tests/system-offloads-traffic.at
> > > > > > b/tests/system-
> > > > > > offloads-traffic.at
> > > > > > index 80bc1dd5c..7ec75340f 100644
> > > > > > --- a/tests/system-offloads-traffic.at
> > > > > > +++ b/tests/system-offloads-traffic.at
> > > > > > @@ -168,3 +168,51 @@ matchall
> > > > > >  ])
> > > > > >  OVS_TRAFFIC_VSWITCHD_STOP
> > > > > >  AT_CLEANUP
> > > > > > +
> > > > > > +AT_SETUP([offloads - check if meter offloading ])
> > > > > 
> > > > > Can we replace if with interface, as I keep on reading it as
> > > > > "if".
> > > > > 
> > > > 
> > > > Sure.
> > > > 
> > > > > > +AT_KEYWORDS([meter])
> > > > > > +AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
> > > > > > +OVS_TRAFFIC_VSWITCHD_START()
> > > > > > +
> > > > > > 

Re: [ovs-dev] [v5 8/8] dpif-netlink: Offloading meter to tc police action

[Jianbo Liu via dev]

On Tue, 2022-06-28 at 09:53 +0200, Eelco Chaudron wrote:
> 
> 
> On 27 Jun 2022, at 16:58, Jianbo Liu wrote:
> 
> > On Mon, 2022-06-27 at 11:32 +0200, Eelco Chaudron wrote:
> > > 
> > > 
> > > On 21 Jun 2022, at 10:22, Jianbo Liu wrote:
> > > 
> > > > On Mon, 2022-06-20 at 12:18 +0200, Eelco Chaudron wrote:
> > > > > On 27 May 2022, at 11:00, Jianbo Liu wrote:
> > > > > 
> > > > > > OVS meters are created in advance and openflow rules refer
> > > > > > to
> > > > > > them
> > > > > > by
> > > > > > their unique ID. New tc_police API is used to offload them.
> > > > > > By
> > > > > > calling
> > > > > > the API, police actions are created and meters are mapped
> > > > > > to
> > > > > > them.
> > > > > > These actions then can be used in tc filter rules by the
> > > > > > index.
> > > > > > 
> > > > > > Signed-off-by: Jianbo Liu 
> > > > > > ---
> > > > > >  NEWS |  2 ++
> > > > > >  lib/dpif-netlink.c   | 31 +---
> > > > > > -
> > > > > >  tests/system-offloads-traffic.at | 48
> > > > > > 
> > > > > >  3 files changed, 76 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/NEWS b/NEWS
> > > > > > index eece0d0b2..dfd659d4e 100644
> > > > > > --- a/NEWS
> > > > > > +++ b/NEWS
> > > > > > @@ -29,6 +29,8 @@ Post-v2.17.0
> > > > > >     - Windows:
> > > > > >   * Conntrack support for TCPv6, UDPv6, ICMPv6, FTPv6.
> > > > > >   * IPv6 Geneve tunnel support.
> > > > > > +   - Linux datapath:
> > > > > > + * Add offloading meter tc police.
> > > > > > 
> > > > > > 
> > > > > >  v2.17.0 - 17 Feb 2022
> > > > > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> > > > > > index 06e1e8ca0..0af9ee77e 100644
> > > > > > --- a/lib/dpif-netlink.c
> > > > > > +++ b/lib/dpif-netlink.c
> > > > > > @@ -4163,11 +4163,18 @@ static int
> > > > > >  dpif_netlink_meter_set(struct dpif *dpif_,
> > > > > > ofproto_meter_id
> > > > > > meter_id,
> > > > > >     struct ofputil_meter_config
> > > > > > *config)
> > > > > >  {
> > > > > > +    int err;
> > > > > > +
> > > > > >  if (probe_broken_meters(dpif_)) {
> > > > > >  return ENOMEM;
> > > > > >  }
> > > > > > 
> > > > > > -    return dpif_netlink_meter_set__(dpif_, meter_id,
> > > > > > config);
> > > > > > +    err = dpif_netlink_meter_set__(dpif_, meter_id,
> > > > > > config);
> > > > > > +    if (!err && netdev_is_flow_api_enabled()) {
> > > > > > +    meter_offload_set(meter_id, config);
> > > > > 
> > > > > Although currently we always return 0, we should still
> > > > > account
> > > > > for it
> > > > > to change in the future, so we should set err to the return
> > > > > value.
> > > > > 
> > > > 
> > > > If there is err from meter_offload_set, it will be passed to
> > > > the
> > > > caller
> > > > of dpif_netlink_meter_set(). I don't agree with that, because
> > > > the
> > > > caller thinks meter_set operation fail, but actually not.
> > > > Besides,
> > > > we
> > > > allow the case that dp meter_set success, but offloading fails,
> > > > so
> > > > the
> > > > return the error of meter_offload_set seems unnecessary.
> > > 
> > > If this is the case, we should change the
> > > dpif_netlink_meter_set()
> > > API to return void.
> > > And add a comment to the function why it would not return an
> > > error:
> > > 
> > 
> > OK.
> > 
> > > --- a/lib/netdev-offload.c
> > > +++ b/lib/netdev-offload.c
> > > @@ -196,7 +196,7 @@ netdev_assign_flow_api(struct netdev *netdev)
> > >  return -1;
> > >  }
> > > 
> > > -int
> > > +void
> > >  meter_offload_set(ofproto_meter_id meter_id,
> > >    struct ofputil_meter_config *config)
> > >  {
> > > @@ -212,8 +212,8 @@ meter_offload_set(ofproto_meter_id meter_id,
> > >     }
> > >  }
> > >  }
> > > -
> > > -    return 0;
> > > +    /* Offload APIs could fail, for example, because the offload
> > > is
> > > not
> > > + * supported. This is fine, as the offload API should take
> > > care
> > > of this. */
> > >  }
> > > 
> > > 
> > > 
> > > > >  +    err = meter_offload_set(meter_id, config);
> > > > > 
> > > > > > +    }
> > > > > > +
> > > 
> > > 
> > > 
> > > > > > diff --git a/tests/system-offloads-traffic.at
> > > > > > b/tests/system-
> > > > > > offloads-traffic.at
> > > > > > index 80bc1dd5c..7ec75340f 100644
> > > > > > --- a/tests/system-offloads-traffic.at
> > > > > > +++ b/tests/system-offloads-traffic.at
> > > > > > @@ -168,3 +168,51 @@ matchall
> > > > > >  ])
> > > > > >  OVS_TRAFFIC_VSWITCHD_STOP
> > > > > >  AT_CLEANUP
> > > > > > +
> > > > > > +AT_SETUP([offloads - check if meter offloading ])
> > > > > 
> > > > > Can we replace if with interface, as I keep on reading it as
> > > > > "if".
> > > > > 
> > > > 
> > > > Sure.
> > > > 
> > > > > > +AT_KEYWORDS([meter])
> > > > > > +AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
> > > > > > +OVS_TRAFFIC_VSWITCHD_START()
> > > > > > +
> > > > > > 

Re: [ovs-dev] [v5 8/8] dpif-netlink: Offloading meter to tc police action

[Eelco Chaudron]

On 27 Jun 2022, at 16:58, Jianbo Liu wrote:

> On Mon, 2022-06-27 at 11:32 +0200, Eelco Chaudron wrote:
>>
>>
>> On 21 Jun 2022, at 10:22, Jianbo Liu wrote:
>>
>>> On Mon, 2022-06-20 at 12:18 +0200, Eelco Chaudron wrote:
 On 27 May 2022, at 11:00, Jianbo Liu wrote:

> OVS meters are created in advance and openflow rules refer to
> them
> by
> their unique ID. New tc_police API is used to offload them. By
> calling
> the API, police actions are created and meters are mapped to
> them.
> These actions then can be used in tc filter rules by the index.
>
> Signed-off-by: Jianbo Liu 
> ---
>  NEWS |  2 ++
>  lib/dpif-netlink.c   | 31 +
>  tests/system-offloads-traffic.at | 48
> 
>  3 files changed, 76 insertions(+), 5 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index eece0d0b2..dfd659d4e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -29,6 +29,8 @@ Post-v2.17.0
>     - Windows:
>   * Conntrack support for TCPv6, UDPv6, ICMPv6, FTPv6.
>   * IPv6 Geneve tunnel support.
> +   - Linux datapath:
> + * Add offloading meter tc police.
>
>
>  v2.17.0 - 17 Feb 2022
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 06e1e8ca0..0af9ee77e 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -4163,11 +4163,18 @@ static int
>  dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id
> meter_id,
>     struct ofputil_meter_config *config)
>  {
> +    int err;
> +
>  if (probe_broken_meters(dpif_)) {
>  return ENOMEM;
>  }
>
> -    return dpif_netlink_meter_set__(dpif_, meter_id, config);
> +    err = dpif_netlink_meter_set__(dpif_, meter_id, config);
> +    if (!err && netdev_is_flow_api_enabled()) {
> +    meter_offload_set(meter_id, config);

 Although currently we always return 0, we should still account
 for it
 to change in the future, so we should set err to the return
 value.

>>>
>>> If there is err from meter_offload_set, it will be passed to the
>>> caller
>>> of dpif_netlink_meter_set(). I don't agree with that, because the
>>> caller thinks meter_set operation fail, but actually not. Besides,
>>> we
>>> allow the case that dp meter_set success, but offloading fails, so
>>> the
>>> return the error of meter_offload_set seems unnecessary.
>>
>> If this is the case, we should change the dpif_netlink_meter_set()
>> API to return void.
>> And add a comment to the function why it would not return an error:
>>
>
> OK.
>
>> --- a/lib/netdev-offload.c
>> +++ b/lib/netdev-offload.c
>> @@ -196,7 +196,7 @@ netdev_assign_flow_api(struct netdev *netdev)
>>  return -1;
>>  }
>>
>> -int
>> +void
>>  meter_offload_set(ofproto_meter_id meter_id,
>>    struct ofputil_meter_config *config)
>>  {
>> @@ -212,8 +212,8 @@ meter_offload_set(ofproto_meter_id meter_id,
>>     }
>>  }
>>  }
>> -
>> -    return 0;
>> +    /* Offload APIs could fail, for example, because the offload is
>> not
>> + * supported. This is fine, as the offload API should take care
>> of this. */
>>  }
>>
>>
>>
  +    err = meter_offload_set(meter_id, config);

> +    }
> +
>>
>> 
>>
> diff --git a/tests/system-offloads-traffic.at b/tests/system-
> offloads-traffic.at
> index 80bc1dd5c..7ec75340f 100644
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -168,3 +168,51 @@ matchall
>  ])
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_SETUP([offloads - check if meter offloading ])

 Can we replace if with interface, as I keep on reading it as
 "if".

>>>
>>> Sure.
>>>
> +AT_KEYWORDS([meter])
> +AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-
> offload=true])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps
> bands=type=drop rate=1'])
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02")
> +
> +NS_CHECK_EXEC([at_ns0], [ip neigh add 10.1.1.2 lladdr
> f0:00:00:01:01:02 dev p0])
> +NS_CHECK_EXEC([at_ns1], [ip neigh add 10.1.1.1 lladdr
> f0:00:00:01:01:01 dev p1])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0
> "actions=normal"])
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2 |
> FORMAT_PING], [0], [dnl
> +10 packets transmitted, 10 received, 0% packet loss, time 0ms
> +])
> +
> +NETNS_DAEMONIZE([at_ns1], [nc -u -l 5678 > /dev/null ],
> [nc0.pid])
> +
> 

Re: [ovs-dev] [v5 8/8] dpif-netlink: Offloading meter to tc police action

[Eelco Chaudron]

On 27 Jun 2022, at 16:58, Jianbo Liu wrote:

> On Mon, 2022-06-27 at 11:32 +0200, Eelco Chaudron wrote:
>>
>>
>> On 21 Jun 2022, at 10:22, Jianbo Liu wrote:
>>
>>> On Mon, 2022-06-20 at 12:18 +0200, Eelco Chaudron wrote:
 On 27 May 2022, at 11:00, Jianbo Liu wrote:

> OVS meters are created in advance and openflow rules refer to
> them
> by
> their unique ID. New tc_police API is used to offload them. By
> calling
> the API, police actions are created and meters are mapped to
> them.
> These actions then can be used in tc filter rules by the index.
>
> Signed-off-by: Jianbo Liu 
> ---
>  NEWS |  2 ++
>  lib/dpif-netlink.c   | 31 +
>  tests/system-offloads-traffic.at | 48
> 
>  3 files changed, 76 insertions(+), 5 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index eece0d0b2..dfd659d4e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -29,6 +29,8 @@ Post-v2.17.0
>     - Windows:
>   * Conntrack support for TCPv6, UDPv6, ICMPv6, FTPv6.
>   * IPv6 Geneve tunnel support.
> +   - Linux datapath:
> + * Add offloading meter tc police.
>
>
>  v2.17.0 - 17 Feb 2022
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 06e1e8ca0..0af9ee77e 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -4163,11 +4163,18 @@ static int
>  dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id
> meter_id,
>     struct ofputil_meter_config *config)
>  {
> +    int err;
> +
>  if (probe_broken_meters(dpif_)) {
>  return ENOMEM;
>  }
>
> -    return dpif_netlink_meter_set__(dpif_, meter_id, config);
> +    err = dpif_netlink_meter_set__(dpif_, meter_id, config);
> +    if (!err && netdev_is_flow_api_enabled()) {
> +    meter_offload_set(meter_id, config);

 Although currently we always return 0, we should still account
 for it
 to change in the future, so we should set err to the return
 value.

>>>
>>> If there is err from meter_offload_set, it will be passed to the
>>> caller
>>> of dpif_netlink_meter_set(). I don't agree with that, because the
>>> caller thinks meter_set operation fail, but actually not. Besides,
>>> we
>>> allow the case that dp meter_set success, but offloading fails, so
>>> the
>>> return the error of meter_offload_set seems unnecessary.
>>
>> If this is the case, we should change the dpif_netlink_meter_set()
>> API to return void.
>> And add a comment to the function why it would not return an error:
>>
>
> OK.
>
>> --- a/lib/netdev-offload.c
>> +++ b/lib/netdev-offload.c
>> @@ -196,7 +196,7 @@ netdev_assign_flow_api(struct netdev *netdev)
>>  return -1;
>>  }
>>
>> -int
>> +void
>>  meter_offload_set(ofproto_meter_id meter_id,
>>    struct ofputil_meter_config *config)
>>  {
>> @@ -212,8 +212,8 @@ meter_offload_set(ofproto_meter_id meter_id,
>>     }
>>  }
>>  }
>> -
>> -    return 0;
>> +    /* Offload APIs could fail, for example, because the offload is
>> not
>> + * supported. This is fine, as the offload API should take care
>> of this. */
>>  }
>>
>>
>>
  +    err = meter_offload_set(meter_id, config);

> +    }
> +
>>
>> 
>>
> diff --git a/tests/system-offloads-traffic.at b/tests/system-
> offloads-traffic.at
> index 80bc1dd5c..7ec75340f 100644
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -168,3 +168,51 @@ matchall
>  ])
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_SETUP([offloads - check if meter offloading ])

 Can we replace if with interface, as I keep on reading it as
 "if".

>>>
>>> Sure.
>>>
> +AT_KEYWORDS([meter])
> +AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-
> offload=true])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps
> bands=type=drop rate=1'])
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02")
> +
> +NS_CHECK_EXEC([at_ns0], [ip neigh add 10.1.1.2 lladdr
> f0:00:00:01:01:02 dev p0])
> +NS_CHECK_EXEC([at_ns1], [ip neigh add 10.1.1.1 lladdr
> f0:00:00:01:01:01 dev p1])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0
> "actions=normal"])
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2 |
> FORMAT_PING], [0], [dnl
> +10 packets transmitted, 10 received, 0% packet loss, time 0ms
> +])
> +
> +NETNS_DAEMONIZE([at_ns1], [nc -u -l 5678 > /dev/null ],
> [nc0.pid])
> +
> 

Re: [ovs-dev] [PATCH ovn v2 0/6] MAC binding aging mechanism

[Ales Musil]

Hi,

so I did the suggested test. Setup was HIV1 - ext0 and vm0, HIV2 ext1 and
vm1

The networks were connected as follow:
- vm0 and vm1 on the same switch
- logical router connected with the "internal" and "external" switch
- "external" switch connected to ext0 and ext1 through localnet

So the traffic was flowing:
vmX -- LR -- localnet -- extX

The iperf was running between vm0 - ext1 and vm1 - ext0.

I have removed the MAC binding for ext0 multiple times to see if it affects
the other traffic.
And it actually does not, which is great.

iperf output from vm0 - ext1:
[ ID] Interval   Transfer Bitrate Retr  Cwnd
[  5]   0.00-1.00   sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]   1.00-2.00   sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]   2.00-3.00   sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]   3.00-4.00   sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]   4.00-5.00   sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]   5.00-6.00   sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]   6.00-7.00   sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]   7.00-8.00   sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]   8.00-9.00   sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]   9.00-10.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  10.00-11.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  11.00-12.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  12.00-13.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  13.00-14.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  14.00-15.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  15.00-16.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  16.00-17.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  17.00-18.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  18.00-19.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  19.00-20.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  20.00-21.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  21.00-22.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  22.00-23.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  23.00-24.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  24.00-25.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  25.00-26.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  26.00-27.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  27.00-28.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  28.00-29.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  29.00-30.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  30.00-31.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  31.00-32.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  32.00-33.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  33.00-34.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  34.00-35.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  35.00-36.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  36.00-37.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  37.00-38.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  38.00-39.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  39.00-40.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  40.00-41.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  41.00-42.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  42.00-43.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  43.00-44.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  44.00-45.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  45.00-46.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  46.00-47.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  47.00-48.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  48.00-49.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  49.00-50.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  50.00-51.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  51.00-52.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  52.00-53.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  53.00-54.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  54.00-55.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  55.00-56.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  56.00-57.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  57.00-58.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

[  5]  58.00-59.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[  5]  59.00-60.00  sec  11.9 MBytes  99.6