date:20221213

[ovs-dev] [PATCH ovn v2 2/2] tests: Check the default drop directly from flows dump

2022-12-13 Thread Ales Musil

Instead of iterating over the logical flows in a loop
just check the number of flows compared to number of
defualt flows. In the later part directly filtere
the portion of the flowthat we are insterested in.
Reducing the duration ~17x.

Reported-at: https://bugzilla.redhat.com/2149851
Signed-off-by: Ales Musil 
---
v2: Address comment from Xavier.
---
 tests/ovn-northd.at | 75 +
 1 file changed, 56 insertions(+), 19 deletions(-)

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 71eda9ff4..6458cecef 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -8193,21 +8193,19 @@ OVN_FOR_EACH_NORTHD([
 AT_SETUP([Check default drop])
 AT_KEYWORDS([drop])
 
-# Check that there is an explicit drop lflow in all tables.
+ovn_start
+
+# Check that there is an explicit drop lflow in for spoecified DP and table.
 check_default_lflow() {
-dps=$(ovn-sbctl --bare  --columns=_uuid list Datapath_Binding | xargs)
-for dp in $dps; do
-for pipeline in ingress egress; do
-for table in $(ovn-sbctl --bare --columns=table_id find 
Logical_Flow logical_datapath="$dp" pipeline="$pipeline" | xargs | sort | 
uniq); do
-   echo "Checking if datapath $dp pipeline $pipeline table $table 
has a default action"
-   AT_CHECK([ovn-sbctl --columns=_uuid find Logical_Flow 
logical_datapath="$dp" pipeline="$pipeline" table_id=$table match="1" 
priority">="0 | wc -l | tr -d "\n\r" ], [0], [1], [ignore],
-   [echo "Datapath $dp pipeline $pipeline table $table does not 
have a default action"])
-done
-done
-done
-}
+dp=$1
+pipeline=$2
 
-ovn_start
+table_len=$(ovn-sbctl --bare --columns table find logical_flow 
logical_datapath=$dp pipeline=$pipeline | sort | uniq | wc -l)
+table_len_default=$(ovn-sbctl --bare --columns table find logical_flow 
logical_datapath=$dp pipeline=$pipeline match=1 | sort | uniq | wc -l)
+
+echo "Checking if datapath $dp pipeline $pipeline has default actions"
+AT_CHECK([test $table_len -eq $table_len_default], [0], [ignore], 
[ignore], [echo "Datapath $dp pipeline $pipeline is missing some default 
action"])
+}
 
 # Create LS + LR
 check ovn-nbctl --wait=sb \
@@ -8221,13 +8219,34 @@ check ovn-nbctl --wait=sb \
 -- lsp-add S1 p1 \
 -- lsp-set-addresses p1 "02:ac:10:01:00:0a 172.16.1.100"
 
-check_default_lflow
+ovn-sbctl dump-flows R1 > R1_flows
+ovn-sbctl dump-flows R1 | grep "match=(1)" > R1_default_flows
+ovn-sbctl dump-flows S1 > S1_flows
+ovn-sbctl dump-flows S1 | grep "match=(1)" > S1_default_flows
+
+AT_CAPTURE_FILE([R1_flows])
+AT_CAPTURE_FILE([R1_default_flows])
+AT_CAPTURE_FILE([S1_flows])
+AT_CAPTURE_FILE([S1_default_flows])
+
+lr_uuid=$(fetch_column datapath _uuid external_ids:name=R1)
+ls_uuid=$(fetch_column datapath _uuid external_ids:name=S1)
+
+check_default_lflow $lr_uuid ingress
+check_default_lflow $lr_uuid egress
+
+check_default_lflow $ls_uuid ingress
+check_default_lflow $ls_uuid egress
 
 # Add stateless ACL
 check ovn-nbctl --wait=sb \
 -- acl-add S1 from-lport 100 'inport=p1 && ip4' allow-stateless
 
-check_default_lflow
+AT_CHECK([ovn-sbctl dump-flows | grep "ls_in_acl" | grep "match=(1)"  | sed 
's/table=../table=??/' | sort], [0], [dnl
+  table=??(ls_in_acl  ), priority=0, match=(1), action=(next;)
+  table=??(ls_in_acl_after_lb ), priority=0, match=(1), action=(next;)
+  table=??(ls_in_acl_hint ), priority=0, match=(1), action=(next;)
+])
 
 check ovn-nbctl --wait=sb acl-del S1
 
@@ -8236,7 +8255,11 @@ check ovn-nbctl --wait=sb acl-del S1
 check ovn-nbctl --wait=sb \
 -- acl-add S1 from-lport 2 "udp" allow-related
 
-check_default_lflow
+AT_CHECK([ovn-sbctl dump-flows | grep "ls_in_acl" | grep "match=(1)"  | sed 
's/table=../table=??/' | sort], [0], [dnl
+  table=??(ls_in_acl  ), priority=0, match=(1), action=(next;)
+  table=??(ls_in_acl_after_lb ), priority=0, match=(1), action=(next;)
+  table=??(ls_in_acl_hint ), priority=0, match=(1), action=(next;)
+])
 
 check ovn-nbctl --wait=sb acl-del S1
 
@@ -8245,12 +8268,22 @@ check ovn-nbctl --wait=sb \
 -- lb-add lb "10.0.0.1" "10.0.0.2" \
 -- ls-lb-add S1 lb
 
-check_default_lflow
+AT_CHECK([ovn-sbctl dump-flows | grep "ls_in_acl" | grep "match=(1)"  | sed 
's/table=../table=??/' | sort], [0], [dnl
+  table=??(ls_in_acl  ), priority=0, match=(1), action=(next;)
+  table=??(ls_in_acl_after_lb ), priority=0, match=(1), action=(next;)
+  table=??(ls_in_acl_hint ), priority=0, match=(1), action=(next;)
+])
+
 
 # Check LB + stateless ACL
 check ovn-nbctl --wait=sb \
 -- acl-add S1 from-lport 100 'inport=p1 && ip4' allow-stateless
-check_default_lflow
+
+AT_CHECK([ovn-sbctl dump-flows | grep "ls_in_acl" | grep "match=(1)"  | sed 
's/table=../table=??/' | sort], [0], [dnl
+  table=??(ls_in_acl  ), priority=0,

[ovs-dev] [PATCH ovn v2 1/2] tests: Reduce duration of "northd-parallelization runtime" test

2022-12-13 Thread Ales Musil

By running single chained ovn-nbctl command the test
is ~5x faster.

Reported-at: https://bugzilla.redhat.com/2149851
Signed-off-by: Ales Musil 
---
v2: Rebase on top of main.
---
 tests/ovn-northd.at | 31 +--
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index ca4263eac..71eda9ff4 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -7431,12 +7431,21 @@ ovn_start
 
 add_switch_ports() {
 for port in $(seq $1 $2); do
-logical_switch_port=lsp${port}
-check ovn-nbctl lsp-add ls1 $logical_switch_port
-check ovn-nbctl lsp-set-addresses $logical_switch_port dynamic
+OVN_NBCTL(lsp-add ls1 lsp${port})
+OVN_NBCTL(lsp-set-addresses lsp${port} dynamic)
 done
+RUN_OVN_NBCTL()
 }
 
+delete_switch_ports() {
+for port in $(seq $1 $2); do
+OVN_NBCTL(lsp-del lsp${port})
+done
+RUN_OVN_NBCTL()
+}
+
+m4_define([DUMP_FLOWS_SORTED], [sed -e 's/arp.tpa == 
10.1.0.[[0-9]]\{1,3\}/arp.tpa == 10.1.0.??/;s/eth.dst == 
..:..:..:..:..:../??:??:??:??:??:??/' | sort])
+
 # Build some rather heavy config and modify number of threads in the middle
 check ovn-nbctl ls-add ls1
 check ovn-nbctl set Logical_Switch ls1 other_config:subnet=10.1.0.0/16
@@ -7464,27 +7473,21 @@ check ovn-nbctl --wait=sb sync
 # Run 3 times: one with parallelization enabled, one with disabled, and one 
while changing
 # Compare the flows produced by the three runs
 # Ignore IP/MAC addresses
-ovn-sbctl dump-flows | sed 's/arp.tpa == 10.1.0/arp.tpa == 10.1.0.??/' | 
sed 's/eth.dst == ..:..:..:..:..:../??:??:??:??:??:??/' |sort > flows1
+ovn-sbctl dump-flows | DUMP_FLOWS_SORTED > flows1
 
 # Restart with 1 thread
-for port in $(seq 1 250); do
-logical_switch_port=lsp${port}
-check ovn-nbctl lsp-del $logical_switch_port
-done
+delete_switch_ports 1 250
 add_switch_ports 1 250
 check ovn-nbctl --wait=sb sync
-ovn-sbctl dump-flows | sed 's/arp.tpa == 10.1.0/arp.tpa == 10.1.0.??/' | 
sed 's/eth.dst == ..:..:..:..:..:../??:??:??:??:??:??/' |sort > flows2
+ovn-sbctl dump-flows | DUMP_FLOWS_SORTED > flows2
 AT_CHECK([diff flows1 flows2])
 
 # Restart with with 8 threads
 check as northd ovn-appctl -t NORTHD_TYPE parallel-build/set-n-threads 8
-for port in $(seq 1 250); do
-logical_switch_port=lsp${port}
-check ovn-nbctl lsp-del $logical_switch_port
-done
+delete_switch_ports 1 250
 add_switch_ports 1 250
 check ovn-nbctl --wait=sb sync
-ovn-sbctl dump-flows | sed 's/arp.tpa == 10.1.0/arp.tpa == 10.1.0.??/' | 
sed 's/eth.dst == ..:..:..:..:..:../??:??:??:??:??:??/' |sort > flows3
+ovn-sbctl dump-flows | DUMP_FLOWS_SORTED > flows3
 AT_CHECK([diff flows1 flows3])
 
 AT_CLEANUP
-- 
2.38.1

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

Re: [ovs-dev] [OVN v19 1/3] OVN Remote Port Mirroring: Add new Schemas in NB

2022-12-13 Thread Numan Siddique

On Tue, Dec 13, 2022 at 3:33 PM Ihar Hrachyshka  wrote:
>
> Abhiram, these style violations should be fixed.

Hi Ihar,

These warnings are ok and can be ignored.  Perhaps we need to fix it
in the checkpatch to address these warnings.

Numan

>
> (I will review the actual code changes tomorrow.)
>
> On Tue, Dec 13, 2022 at 2:40 PM 0-day Robot  wrote:
> >
> > Bleep bloop.  Greetings Abhiram R N, I am a robot and I have tried out your 
> > patch.
> > Thanks for your contribution.
> >
> > I encountered some error that I wasn't expecting.  See the details below.
> >
> >
> > checkpatch:
> > WARNING: Line lacks whitespace around operator
> > #336 FILE: utilities/ovn-nbctl.c:276:
> >   mirror-add NAME TYPE INDEX FILTER IP\n\
> >
> > WARNING: Line lacks whitespace around operator
> > #345 FILE: utilities/ovn-nbctl.c:285:
> >   mirror-del [NAME] remove mirrors\n\
> >
> > WARNING: Line lacks whitespace around operator
> > #346 FILE: utilities/ovn-nbctl.c:286:
> >   mirror-list   print mirrors\n\
> >
> > WARNING: Line lacks whitespace around operator
> > #355 FILE: utilities/ovn-nbctl.c:328:
> >   lsp-attach-mirror PORT MIRROR   attach source PORT to MIRROR\n\
> >
> > WARNING: Line lacks whitespace around operator
> > #356 FILE: utilities/ovn-nbctl.c:329:
> >   lsp-detach-mirror PORT MIRROR   detach source PORT from MIRROR\n\
> >
> > Lines checked: 710, Warnings: 5, Errors: 0
> >
> >
> > Please check this out.  If you feel there has been an error, please email 
> > acon...@redhat.com
> >
> > Thanks,
> > 0-day Robot
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2] docs: Extend upgrade documentation.

2022-12-13 Thread Mark Michelson


On 12/9/22 09:21, Dumitru Ceara wrote:

On 12/6/22 16:34, Frode Nordahl wrote:

As uncovered during the OVSCON'22 open discussion forum on
upgrades, there are some challenges in upgrading from older
versions of OVN.

Document version requirements for performing a controller first
rolling upgrade.

Add a section about how to perform a fail-safe upgrade for
deployments that want to upgrade beyond a supported version span.

Reported-at: https://bugs.launchpad.net/bugs/1940043
Signed-off-by: Frode Nordahl 
---


Acked-by: Dumitru Ceara 

Thanks again for improving the upgrade documentation!  It looks good to
me but let's see what others think about it too.

Regards,
Dumitru



Acked-by: Mark Michelson 

Thanks a bunch, Frode!

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

Re: [ovs-dev] [OVN v19 1/3] OVN Remote Port Mirroring: Add new Schemas in NB

2022-12-13 Thread Ihar Hrachyshka

Abhiram, these style violations should be fixed.

(I will review the actual code changes tomorrow.)

On Tue, Dec 13, 2022 at 2:40 PM 0-day Robot  wrote:
>
> Bleep bloop.  Greetings Abhiram R N, I am a robot and I have tried out your 
> patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> checkpatch:
> WARNING: Line lacks whitespace around operator
> #336 FILE: utilities/ovn-nbctl.c:276:
>   mirror-add NAME TYPE INDEX FILTER IP\n\
>
> WARNING: Line lacks whitespace around operator
> #345 FILE: utilities/ovn-nbctl.c:285:
>   mirror-del [NAME] remove mirrors\n\
>
> WARNING: Line lacks whitespace around operator
> #346 FILE: utilities/ovn-nbctl.c:286:
>   mirror-list   print mirrors\n\
>
> WARNING: Line lacks whitespace around operator
> #355 FILE: utilities/ovn-nbctl.c:328:
>   lsp-attach-mirror PORT MIRROR   attach source PORT to MIRROR\n\
>
> WARNING: Line lacks whitespace around operator
> #356 FILE: utilities/ovn-nbctl.c:329:
>   lsp-detach-mirror PORT MIRROR   detach source PORT from MIRROR\n\
>
> Lines checked: 710, Warnings: 5, Errors: 0
>
>
> Please check this out.  If you feel there has been an error, please email 
> acon...@redhat.com
>
> Thanks,
> 0-day Robot
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

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

Re: [ovs-dev] [OVN v19 1/3] OVN Remote Port Mirroring: Add new Schemas in NB

2022-12-13 Thread 0-day Robot

Bleep bloop.  Greetings Abhiram R N, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

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


checkpatch:
WARNING: Line lacks whitespace around operator
#336 FILE: utilities/ovn-nbctl.c:276:
  mirror-add NAME TYPE INDEX FILTER IP\n\

WARNING: Line lacks whitespace around operator
#345 FILE: utilities/ovn-nbctl.c:285:
  mirror-del [NAME] remove mirrors\n\

WARNING: Line lacks whitespace around operator
#346 FILE: utilities/ovn-nbctl.c:286:
  mirror-list   print mirrors\n\

WARNING: Line lacks whitespace around operator
#355 FILE: utilities/ovn-nbctl.c:328:
  lsp-attach-mirror PORT MIRROR   attach source PORT to MIRROR\n\

WARNING: Line lacks whitespace around operator
#356 FILE: utilities/ovn-nbctl.c:329:
  lsp-detach-mirror PORT MIRROR   detach source PORT from MIRROR\n\

Lines checked: 710, Warnings: 5, Errors: 0


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

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

[ovs-dev] [OVN v19 3/3] OVN Remote Port Mirroring: controller changes to create ovs mirrors

2022-12-13 Thread Abhiram R N

Mirror creation just creates the mirror. The lsp-attach-mirror
triggers the sequence to create Mirror in OVS DB on compute node.
OVS already supports Port Mirroring.

Further added test cases in ovn.at to verify end to end
the functioning of Port Mirror and also verify bulk updates
to mirrors.

Note: This is targeted to mirror to destinations anywhere outside the
cluster where the analyser resides and it need not be an OVN node.

Example commands are as below:

Mirror creation
ovn-nbctl mirror-add mirror1 gre 0 from-lport 10.10.10.2

Attach a logical port to the mirror.
ovn-nbctl lsp-attach-mirror sw0-port1 mirror1

Detach a source from Mirror
ovn-nbctl lsp-detach-mirror sw0-port1 mirror1

Mirror deletion
ovn-nbctl mirror-del mirror1

Co-authored-by: Veda Barrenkala 
Signed-off-by: Veda Barrenkala 
Signed-off-by: Abhiram R N 
Acked-By: Ihar Hrachyshka 
---
 NEWS|   1 +
 controller/automake.mk  |   4 +-
 controller/mirror.c | 418 +
 controller/mirror.h |  33 +++
 controller/ovn-controller.c |   9 +
 tests/ovn.at| 514 
 6 files changed, 978 insertions(+), 1 deletion(-)
 create mode 100644 controller/mirror.c
 create mode 100644 controller/mirror.h

diff --git a/NEWS b/NEWS
index 7d1e49cf1..566d6731b 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,7 @@ OVN v22.12.0 - xx xxx 
 per-flow IPFIX sampling.
   - Add support for component templates within logical flows and load
 balancers.
+  - Added Support for Remote Port Mirroring.
 
 OVN v22.09.0 - 16 Sep 2022
 --
diff --git a/controller/automake.mk b/controller/automake.mk
index c2ab1bbe6..334672b4d 100644
--- a/controller/automake.mk
+++ b/controller/automake.mk
@@ -41,7 +41,9 @@ controller_ovn_controller_SOURCES = \
controller/ovsport.h \
controller/ovsport.c \
controller/vif-plug.h \
-   controller/vif-plug.c
+   controller/vif-plug.c \
+   controller/mirror.h \
+   controller/mirror.c
 
 controller_ovn_controller_LDADD = lib/libovn.la $(OVS_LIBDIR)/libopenvswitch.la
 man_MANS += controller/ovn-controller.8
diff --git a/controller/mirror.c b/controller/mirror.c
new file mode 100644
index 0..938ec3542
--- /dev/null
+++ b/controller/mirror.c
@@ -0,0 +1,418 @@
+/* Copyright (c) 2022 Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include 
+
+/* library headers */
+#include "lib/sset.h"
+#include "lib/util.h"
+
+/* OVS includes. */
+#include "lib/vswitch-idl.h"
+#include "include/openvswitch/shash.h"
+#include "openvswitch/vlog.h"
+
+/* OVN includes. */
+#include "binding.h"
+#include "lib/ovn-sb-idl.h"
+#include "mirror.h"
+
+VLOG_DEFINE_THIS_MODULE(port_mirror);
+
+struct ovn_mirror {
+char *name;
+const struct sbrec_mirror *sb_mirror;
+const struct ovsrec_mirror *ovs_mirror;
+struct ovs_list mirror_src_lports;
+struct ovs_list mirror_dst_lports;
+};
+
+struct mirror_lport {
+struct ovs_list list_node;
+
+struct local_binding *lbinding;
+};
+
+static struct ovn_mirror *ovn_mirror_create(char *mirror_name);
+static void ovn_mirror_add(struct shash *ovn_mirrors,
+   struct ovn_mirror *);
+static struct ovn_mirror *ovn_mirror_find(struct shash *ovn_mirrors,
+  const char *mirror_name);
+static void ovn_mirror_delete(struct ovn_mirror *);
+static void ovn_mirror_add_lport(struct ovn_mirror *, struct local_binding *);
+static void sync_ovn_mirror(struct ovn_mirror *, struct ovsdb_idl_txn *,
+const struct ovsrec_bridge *);
+
+static void create_ovs_mirror(struct ovn_mirror *, struct ovsdb_idl_txn *,
+  const struct ovsrec_bridge *);
+static void sync_ovs_mirror_ports(struct ovn_mirror *,
+  const struct ovsrec_bridge *);
+static void delete_ovs_mirror(struct ovn_mirror *,
+  const struct ovsrec_bridge *);
+static bool should_delete_ovs_mirror(struct ovn_mirror *);
+static void set_mirror_iface_options(struct ovsrec_interface *,
+ const struct sbrec_mirror *);
+
+static const struct ovsrec_port *get_iface_port(
+const struct ovsrec_interface *, const struct ovsrec_bridge *);
+
+
+void
+mirror_register_ovs_idl(struct ovsdb_idl *ovs_idl)
+{
+ovsdb_idl_add_column(ovs_idl,

[ovs-dev] [OVN v19 2/3] OVN Remote Port Mirroring: northd changes to sync NB and SB

2022-12-13 Thread Abhiram R N

Added the required schema SB and related xml changes.
Changes which syncs the NB port mirrors with SB port mirrors.
Also syncs mirror_rules column in Logical_Switch_Port table
of NB DB with corresponding mirror_rules column in
Port_Binding table of SB DB.
Further test added to check the NB and SB sync

Co-authored-by: Veda Barrenkala 
Signed-off-by: Veda Barrenkala 
Signed-off-by: Abhiram R N 
Acked-By: Ihar Hrachyshka 
---
 northd/en-northd.c   |   4 ++
 northd/inc-proc-northd.c |   4 ++
 northd/northd.c  | 133 +++
 northd/northd.h  |   2 +
 ovn-sb.ovsschema |  26 +++-
 ovn-sb.xml   |  50 +++
 tests/ovn-northd.at  | 105 +++
 utilities/ovn-sbctl.c|   4 ++
 8 files changed, 326 insertions(+), 2 deletions(-)

diff --git a/northd/en-northd.c b/northd/en-northd.c
index 9360c68e9..09fe8976a 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -80,6 +80,8 @@ void en_northd_run(struct engine_node *node, void *data)
 EN_OVSDB_GET(engine_get_input("NB_static_mac_binding", node));
 input_data.nbrec_chassis_template_var_table =
 EN_OVSDB_GET(engine_get_input("NB_chassis_template_var", node));
+input_data.nbrec_mirror_table =
+EN_OVSDB_GET(engine_get_input("NB_mirror", node));
 
 input_data.sbrec_sb_global_table =
 EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
@@ -113,6 +115,8 @@ void en_northd_run(struct engine_node *node, void *data)
 EN_OVSDB_GET(engine_get_input("SB_static_mac_binding", node));
 input_data.sbrec_chassis_template_var_table =
 EN_OVSDB_GET(engine_get_input("SB_chassis_template_var", node));
+input_data.sbrec_mirror_table =
+EN_OVSDB_GET(engine_get_input("SB_mirror", node));
 
 northd_run(_data, data,
eng_ctx->ovnnb_idl_txn,
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index ff3620d62..363e384bd 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -52,6 +52,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
 NB_NODE(acl, "acl") \
 NB_NODE(logical_router, "logical_router") \
 NB_NODE(qos, "qos") \
+NB_NODE(mirror, "mirror") \
 NB_NODE(meter, "meter") \
 NB_NODE(meter_band, "meter_band") \
 NB_NODE(logical_router_port, "logical_router_port") \
@@ -95,6 +96,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
 SB_NODE(logical_flow, "logical_flow") \
 SB_NODE(logical_dp_group, "logical_DP_group") \
 SB_NODE(multicast_group, "multicast_group") \
+SB_NODE(mirror, "mirror") \
 SB_NODE(meter, "meter") \
 SB_NODE(meter_band, "meter_band") \
 SB_NODE(datapath_binding, "datapath_binding") \
@@ -178,6 +180,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
 engine_add_input(_northd, _nb_acl, NULL);
 engine_add_input(_northd, _nb_logical_router, NULL);
 engine_add_input(_northd, _nb_qos, NULL);
+engine_add_input(_northd, _nb_mirror, NULL);
 engine_add_input(_northd, _nb_meter, NULL);
 engine_add_input(_northd, _nb_meter_band, NULL);
 engine_add_input(_northd, _nb_logical_router_port, NULL);
@@ -200,6 +203,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
 engine_add_input(_northd, _sb_encap, NULL);
 engine_add_input(_northd, _sb_port_group, NULL);
 engine_add_input(_northd, _sb_logical_dp_group, NULL);
+engine_add_input(_northd, _sb_mirror, NULL);
 engine_add_input(_northd, _sb_meter, NULL);
 engine_add_input(_northd, _sb_meter_band, NULL);
 engine_add_input(_northd, _sb_datapath_binding, NULL);
diff --git a/northd/northd.c b/northd/northd.c
index 7c48bb3b4..6ada6eb68 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3238,6 +3238,60 @@ ovn_port_update_sbrec_chassis(
 free(requested_chassis_sb);
 }
 
+static void
+check_and_do_sb_mirror_deletion(const struct ovn_port *op)
+{
+size_t i = 0;
+struct shash nb_mirror_rules = SHASH_INITIALIZER(_mirror_rules);
+
+for (i = 0; i < op->nbsp->n_mirror_rules; i++) {
+shash_add(_mirror_rules,
+  op->nbsp->mirror_rules[i]->name,
+  op->nbsp->mirror_rules[i]);
+}
+
+for (i = 0; i < op->sb->n_mirror_rules; i++) {
+if (!shash_find(_mirror_rules,
+op->sb->mirror_rules[i]->name)) {
+/* Delete from SB since its not present in NB*/
+sbrec_port_binding_update_mirror_rules_delvalue(op->sb,
+ op->sb->mirror_rules[i]);
+}
+}
+
+struct shash_node *node, *next;
+SHASH_FOR_EACH_SAFE (node, next, _mirror_rules) {
+shash_delete(_mirror_rules, node);
+}
+shash_destroy(_mirror_rules);
+}
+
+static void
+check_and_do_sb_mirror_addition(struct northd_input *input_data,
+const struct ovn_port *op)
+{
+for (size_t i = 0; i < op->nbsp->n_mirror_rules; i++) {
+const

[ovs-dev] [OVN v19 1/3] OVN Remote Port Mirroring: Add new Schemas in NB

2022-12-13 Thread Abhiram R N

In order to support Remote Port Mirroring
added the required schemas in NB and related xml.
Also, ovn-nbctl.c changes are added.
Futher added test cases to test nbctl commands.

Co-authored-by: Veda Barrenkala 
Signed-off-by: Veda Barrenkala 
Signed-off-by: Abhiram R N 
Acked-By: Ihar Hrachyshka 
---
 ovn-nb.ovsschema  |  25 ++-
 ovn-nb.xml|  55 +++
 tests/ovn-nbctl.at| 102 
 utilities/ovn-nbctl.8.xml |  51 ++
 utilities/ovn-nbctl.c | 334 ++
 5 files changed, 565 insertions(+), 2 deletions(-)

diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
index 6f9d38f47..4836a219f 100644
--- a/ovn-nb.ovsschema
+++ b/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
 "name": "OVN_Northbound",
-"version": "6.4.0",
-"cksum": "3512158873 32360",
+"version": "7.0.0",
+"cksum": "94023179 33468",
 "tables": {
 "NB_Global": {
 "columns": {
@@ -132,6 +132,11 @@
 "refType": "weak"},
  "min": 0,
  "max": 1}},
+"mirror_rules": {"type": {"key": {"type": "uuid",
+  "refTable": "Mirror",
+  "refType": "weak"},
+ "min": 0,
+ "max": "unlimited"}},
 "ha_chassis_group": {
 "type": {"key": {"type": "uuid",
  "refTable": "HA_Chassis_Group",
@@ -301,6 +306,22 @@
 "type": {"key": "string", "value": "string",
  "min": 0, "max": "unlimited"}}},
 "isRoot": false},
+"Mirror": {
+"columns": {
+"name": {"type": "string"},
+"filter": {"type": {"key": {"type": "string",
+"enum": ["set", ["from-lport",
+ "to-lport"]]}}},
+"sink":{"type": "string"},
+"type": {"type": {"key": {"type": "string",
+  "enum": ["set", ["gre",
+   "erspan"]]}}},
+"index": {"type": "integer"},
+"external_ids": {
+"type": {"key": "string", "value": "string",
+ "min": 0, "max": "unlimited"}}},
+"indexes": [["name"]],
+"isRoot": true},
 "Meter": {
 "columns": {
 "name": {"type": "string"},
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 0edc3da96..c2d05c6ef 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -1582,6 +1582,11 @@
   
 
 
+
+Mirror rules that apply to logical switch port which is the source.
+Please see the  table.
+
+
 
   References a row in the OVN Northbound database's
table.
@@ -2587,6 +2592,56 @@ or
 
   
 
+  
+
+  Each row in this table represents one Mirror that can be used for
+  port mirroring. These Mirrors are referenced by the
+   column in
+  the  table.
+
+
+
+  
+Represents the name of the mirror.
+  
+
+
+
+  
+The value of this field represents selection criteria of the mirror.
+Supported values for filter to-lport / from-lport
+to-lport - to mirror packets coming into logical port
+from-lport - to mirror packets going out of logical port.
+  
+
+
+
+  
+The value of this field represents the destination/sink of the mirror.
+The value it takes is an IP address of the sink port.
+  
+
+
+
+  
+The value of this field represents the type of the tunnel used for
+sending the mirrored packets. Supported Tunnel types gre and erspan
+  
+
+
+
+  
+The value of this field represents the tunnel ID. Depending on the
+tunnel type configured, GRE key value if type GRE and erspan_idx value
+if ERSPAN
+  
+
+
+
+  See External IDs at the beginning of this document.
+
+  
+
   
 
   Each row in this table represents a meter that can be used for QoS or
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 91d8d338e..8885ac9fc 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -435,6 +435,108 @@ AT_CHECK([ovn-nbctl meter-list], [0], [dnl
 
 dnl -
 
+OVN_NBCTL_TEST([ovn_nbctl_mirrors], [mirrors], [
+check ovn-nbctl mirror-add mirror1 gre 0 from-lport 10.10.10.1
+check ovn-nbctl mirror-add mirror2 erspan 1 to-lport 10.10.10.2
+check ovn-nbctl mirror-add mirror3 gre 2 to-lport 10.10.10.3
+check ovn-nbctl ls-add sw0
+check ovn-nbctl lsp-add sw0 sw0-port1
+check ovn-nbctl lsp-add sw0 sw0-port2
+check

[ovs-dev] [OVN v19 0/3] OVN Remote Port Mirroring

2022-12-13 Thread Abhiram R N

This patch set adds a new feature in OVN.
i.e Remote Port Mirroring
Already in OVS this support is present. We are leveraging that
and trying to add support in OVN.
So that from OVN APIs we can create/delete mirrors and
attach and detaches sources to them.

Mirror creation just creates the mirror. The lsp-attach-mirror
triggers the sequence to create Mirror in OVS DB on compute node.

Note: This is targeted to mirror to destinations anywhere outside the
cluster where the analyser resides and it need not be an OVN node.

For mirror filter currently we have decided to support 'to-lport' and
'from-lport' since we are observing issue for the 'both' case.
It will pursued with OVS team separately and tracked via a BZ and
after getting that fixed we can enchance the filter to support
'both' as well.

Abhiram R N (3):
  OVN Remote Port Mirroring: Add new Schemas in NB
  OVN Remote Port Mirroring: northd changes to sync NB and SB
  OVN Remote Port Mirroring: controller changes to create ovs mirrors

 NEWS|   1 +
 controller/automake.mk  |   4 +-
 controller/mirror.c | 418 +
 controller/mirror.h |  33 +++
 controller/ovn-controller.c |   9 +
 northd/en-northd.c  |   4 +
 northd/inc-proc-northd.c|   4 +
 northd/northd.c | 133 ++
 northd/northd.h |   2 +
 ovn-nb.ovsschema|  25 +-
 ovn-nb.xml  |  55 
 ovn-sb.ovsschema|  26 +-
 ovn-sb.xml  |  50 
 tests/ovn-nbctl.at  | 102 +++
 tests/ovn-northd.at | 105 
 tests/ovn.at| 514 
 utilities/ovn-nbctl.8.xml   |  51 
 utilities/ovn-nbctl.c   | 334 +++
 utilities/ovn-sbctl.c   |   4 +
 19 files changed, 1869 insertions(+), 5 deletions(-)
 create mode 100644 controller/mirror.c
 create mode 100644 controller/mirror.h

-- 
2.31.1

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

Re: [ovs-dev] [PATCH v5 ovn] controller: improve buffered packets management

2022-12-13 Thread Mark Michelson


Hi Lorenzo,

Acked-by: Mark Michelson 

I'm acking the patch, but it does need a rebase. If you post a rebase, 
I'll merge it.


On 11/28/22 11:12, Lorenzo Bianconi wrote:

Improve buffered packet management in ovn-controller using even
datapath and port keys as buffered_packets_map hashmap hash.
Improve run_buffered_binding avoiding unnecessary loops over
datapaths and port_bindings.

Signed-off-by: Lorenzo Bianconi 
---
Changes since v4:
- improve buffered_packets_map lookup looping over tracked mac_binding table
Changes since v3:
- fix ip stored in buffered_packets structure (must be unique for each
   buffered_packets_map map entry)
Changes since v2:
- improve hash function
Changes since v1:
- improve code readability
---
  controller/ovn-controller.c |   2 +
  controller/pinctrl.c| 130 +++-
  controller/pinctrl.h|   2 +
  tests/system-ovn.at | 127 +++
  4 files changed, 216 insertions(+), 45 deletions(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 0752a71ad..2e0a8952e 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -4304,6 +4304,8 @@ main(int argc, char *argv[])
  ovnsb_idl_loop.idl),
  sbrec_service_monitor_table_get(
  ovnsb_idl_loop.idl),
+sbrec_mac_binding_table_get(
+ovnsb_idl_loop.idl),
  sbrec_bfd_table_get(ovnsb_idl_loop.idl),
  br_int, chassis,
  _data->local_datapaths,
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index f44775c7e..e1a5adb1d 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -180,9 +180,8 @@ static struct pinctrl pinctrl;
  static void init_buffered_packets_map(void);
  static void destroy_buffered_packets_map(void);
  static void
-run_buffered_binding(struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
- struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
- const struct hmap *local_datapaths)
+run_buffered_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
+ const struct sbrec_mac_binding_table *mac_binding_table)
  OVS_REQUIRES(pinctrl_mutex);
  
  static void pinctrl_handle_put_mac_binding(const struct flow *md,

@@ -1430,6 +1429,9 @@ struct buffered_packets {
  struct in6_addr ip;
  struct eth_addr ea;
  
+uint64_t dp_key;

+uint64_t port_key;
+
  long long int timestamp;
  
  struct buffer_info data[BUFFER_QUEUE_DEPTH];

@@ -1555,20 +1557,40 @@ buffered_packets_map_gc(void)
  }
  }
  
+static uint32_t

+pinctrl_buffer_packet_hash(uint64_t dp_key, uint64_t port_key,
+   const struct in6_addr *addr)
+{
+uint32_t hash = 0;
+hash = hash_add64(hash, port_key);
+hash = hash_add64(hash, dp_key);
+hash = hash_bytes(addr, sizeof addr, hash);
+return hash_finish(hash, 16);
+}
+
  static struct buffered_packets *
-pinctrl_find_buffered_packets(const struct in6_addr *ip, uint32_t hash)
+pinctrl_find_buffered_packets(uint64_t dp_key, uint64_t port_key,
+  const struct in6_addr *ip, uint32_t hash)
  {
  struct buffered_packets *qp;
-
-HMAP_FOR_EACH_WITH_HASH (qp, hmap_node, hash,
- _packets_map) {
-if (IN6_ARE_ADDR_EQUAL(>ip, ip)) {
+HMAP_FOR_EACH_WITH_HASH (qp, hmap_node, hash, _packets_map) {
+if (qp->dp_key == dp_key && qp->port_key == port_key &&
+IN6_ARE_ADDR_EQUAL(>ip, ip)) {
  return qp;
  }
  }
  return NULL;
  }
  
+static struct buffered_packets *

+pinctrl_find_buffered_packets_with_hash(uint64_t dp_key, uint64_t port_key,
+const struct in6_addr *ip)
+{
+uint32_t hash = pinctrl_buffer_packet_hash(dp_key, port_key, ip);
+
+return pinctrl_find_buffered_packets(dp_key, port_key, ip, hash);
+}
+
  /* Called with in the pinctrl_handler thread context. */
  static int
  pinctrl_handle_buffered_packets(struct dp_packet *pkt_in,
@@ -1578,6 +1600,8 @@ pinctrl_handle_buffered_packets(struct dp_packet *pkt_in,
  struct buffered_packets *bp;
  struct dp_packet *clone;
  struct in6_addr addr;
+uint64_t dp_key = ntohll(md->flow.metadata);
+uint64_t oport_key = md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0];
  
  if (is_arp) {

  addr = in6_addr_mapped_ipv4(htonl(md->flow.regs[0]));
@@ -1586,8 +1610,8 @@ pinctrl_handle_buffered_packets(struct dp_packet *pkt_in,
  memcpy(, , sizeof addr);
  }
  
-uint32_t hash = hash_bytes(, sizeof addr, 0);

-bp = pinctrl_find_buffered_packets(, hash);
+uint32_t hash = pinctrl_buffer_packet_hash(dp_key, oport_key, );
+

Re: [ovs-dev] [PATCH v3 ovn] actions: introduce ct_commit_continue action

2022-12-13 Thread Lorenzo Bianconi

> On Tue, Dec 13, 2022 at 5:03 AM Dumitru Ceara  wrote:
> >
> > On 12/13/22 07:17, Han Zhou wrote:
> > > On Mon, Dec 12, 2022 at 8:52 AM Numan Siddique  wrote:
> > >>
> > >> On Thu, Dec 8, 2022 at 12:15 PM Lorenzo Bianconi
> > >>  wrote:
> > >>>
> > >>> In the current codebase ct_commit {} action clears ct_state metadata
> of
> > >>> the incoming packet. This behaviour introduces an issue if we need to
> > >>> check the connection tracking state in the subsequent pipeline stages,
> > >>> e.g. for hairpin traffic:
> > >>>
> > >>> table=14(ls_in_pre_hairpin  ), priority=100  , match=(ip && ct.trk),
> > > action=(reg0[6] = chk_lb_hairpin(); reg0[12] = chk_lb_hairpin_reply();
> > > next;)
> > >>>
> > >>> Fix the issue introducing ct_commit_continue action used to allow the
> ct
> > >>> packet to proceed in the pipeline instead of the original one.
> > >>>
> > >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2103086
> > >>> Signed-off-by: Lorenzo Bianconi 
> > >>
> > >> I've a few comments.
> > >>
> > >> Since the OVS action ct() supports specifying the recirculation table
> > >> number,  I think we can do something similar in OVN too
> > >> instead of just restricting to the next table.
> > >>
> > >> I'd suggest something similar on the v1 of this patch, but instead of
> > >> the next flag,  ovn-northd can specify the table number.
> > >>
> > >> Something like   - ct_commit {..., table=x}  where 'x' is a valid
> > >> table number in the stage where this action is used i.e restrict the
> > >> jump to the table within the
> > >> ingress or egress pipeline but not from ingress to egress or from
> > >> egress to ingress.
> > >>
> > >> ovn-northd can use the same feature flag approach used in this patch
> > >> to include this 'table' option.
> > >>
> > >> Also I'd suggest making use of this new version of ct_commit in all
> > >> the places and not just in the specific ones.
> > >>
> > >> Later if there is a requirement to skip a few stages after committing
> > >> the packet to conntrack we can do so without the need to add a new
> > >> action or modify ct_commit.
> > >>
> > >> @Dumitru Ceara @Mark Michelson  @Han Zhou  Do you have any thoughts ?
> > >> or objections ?
> >
> > Hi all,
> >
> > >
> > > Hi Numan, Lorenzo, I agree with Numan that adding the table=x in
> ct_commit
> > > action is more flexible, but I have a different concern. Either the
> > > ct_commit_continue implemented by this patch or the ct_commit
> {...,table=x}
> > > proposed by Numan would introduce an extra recirculation, which has a
> > > significant cost for data-plane. Since apply-after-lb is now frequently
> > > used for ACLs that implement egress network policies, it is better to
> avoid
> > > this penalty if possible. Another option to solve the problem is to move
> > > the ls_in_stateful stage after the hairpin stages, and keep the current
> > > ct_commit. Would that work?
> > >
> >
> > I think you're right, Han.  We seem to add quite a few extra
> recirculations.
> > For example, the datapath flows for a SYN packet hitting a load balancer
> > (dnats from 66.66.66.66:666 to 42.42.42.2:4242):
> >
> >
> recirc_id(0),in_port(5),eth(src=00:00:00:00:00:00/01:00:00:00:00:00,dst=00:00:00:00:01:00),eth_type(0x0800),ipv4(dst=66.66.66.66,proto=6,frag=no),tcp(dst=666),
> packets:1, bytes:66, used:1.180s, flags:.,
> actions:ct(zone=6,nat),recirc(0x50)
> >
> recirc_id(0x50),in_port(5),ct_state(+new-est-rel-rpl-inv+trk),ct_mark(0/0x1),eth(dst=00:00:00:00:01:00),eth_type(0x0800),ipv4(dst=66.66.66.66,proto=6,frag=no),tcp(dst=666),
> packets:0, bytes:0, used:never, actions:hash(l4(0)),recirc(0x54)
> >
> recirc_id(0x54),dp_hash(0x7/0xf),in_port(5),eth(),eth_type(0x0800),ipv4(frag=no),
> packets:0, bytes:0, used:never,
> actions:ct(commit,zone=6,mark=0x2/0x2,nat(dst=42.42.42.2:4242)),recirc(0x55)
> >
> recirc_id(0x55),in_port(5),ct_state(+new-est-rel-rpl-inv+trk),ct_mark(0/0x1),eth(src=00:00:00:00:00:02,dst=00:00:00:00:01:00),eth_type(0x0800),ipv4(src=
> 42.42.42.2/255.255.255.254,dst=42.42.42.2,proto=6,ttl=64,frag=no),
> packets:0, bytes:0, used:never,
> actions:ct(commit,zone=6,mark=0/0x1,nat(src)),set(eth(src=00:00:00:00:01:00,dst=00:00:00:00:00:01)),set(ipv4(ttl=63)),ct(zone=5,nat),recirc(0x51)
> >
> recirc_id(0x51),in_port(5),ct_state(+new-est-rel-rpl-inv+trk),ct_mark(0/0x1),eth(src=00:00:00:00:01:00,dst=00:00:00:00:00:00/01:00:00:00:00:00),eth_type(0x0800),ipv4(frag=no),
> packets:0, bytes:0, used:never,
> actions:ct(commit,zone=5,mark=0/0x1,nat(src)),4
> >
> > Now, with Lorenzo's patch:
> >
> >
> recirc_id(0),in_port(5),eth(src=00:00:00:00:00:00/01:00:00:00:00:00,dst=00:00:00:00:01:00),eth_type(0x0800),ipv4(dst=66.66.66.66,proto=6,frag=no),tcp(dst=666),
> packets:1, bytes:66, used:1.893s, flags:.,
> actions:ct(zone=6,nat),recirc(0x47)
> >
> recirc_id(0x47),in_port(5),ct_state(+new-est-rel-rpl-inv+trk),ct_mark(0/0x1),eth(dst=00:00:00:00:01:00),eth_type(0x0800),ipv4(dst=66.66.66.66,proto=6,frag=no),tcp(dst=666),
> packets:0, bytes:0,

Re: [ovs-dev] [PATCH v3 ovn] actions: introduce ct_commit_continue action

2022-12-13 Thread Han Zhou

On Tue, Dec 13, 2022 at 5:03 AM Dumitru Ceara  wrote:
>
> On 12/13/22 07:17, Han Zhou wrote:
> > On Mon, Dec 12, 2022 at 8:52 AM Numan Siddique  wrote:
> >>
> >> On Thu, Dec 8, 2022 at 12:15 PM Lorenzo Bianconi
> >>  wrote:
> >>>
> >>> In the current codebase ct_commit {} action clears ct_state metadata
of
> >>> the incoming packet. This behaviour introduces an issue if we need to
> >>> check the connection tracking state in the subsequent pipeline stages,
> >>> e.g. for hairpin traffic:
> >>>
> >>> table=14(ls_in_pre_hairpin  ), priority=100  , match=(ip && ct.trk),
> > action=(reg0[6] = chk_lb_hairpin(); reg0[12] = chk_lb_hairpin_reply();
> > next;)
> >>>
> >>> Fix the issue introducing ct_commit_continue action used to allow the
ct
> >>> packet to proceed in the pipeline instead of the original one.
> >>>
> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2103086
> >>> Signed-off-by: Lorenzo Bianconi 
> >>
> >> I've a few comments.
> >>
> >> Since the OVS action ct() supports specifying the recirculation table
> >> number,  I think we can do something similar in OVN too
> >> instead of just restricting to the next table.
> >>
> >> I'd suggest something similar on the v1 of this patch, but instead of
> >> the next flag,  ovn-northd can specify the table number.
> >>
> >> Something like   - ct_commit {..., table=x}  where 'x' is a valid
> >> table number in the stage where this action is used i.e restrict the
> >> jump to the table within the
> >> ingress or egress pipeline but not from ingress to egress or from
> >> egress to ingress.
> >>
> >> ovn-northd can use the same feature flag approach used in this patch
> >> to include this 'table' option.
> >>
> >> Also I'd suggest making use of this new version of ct_commit in all
> >> the places and not just in the specific ones.
> >>
> >> Later if there is a requirement to skip a few stages after committing
> >> the packet to conntrack we can do so without the need to add a new
> >> action or modify ct_commit.
> >>
> >> @Dumitru Ceara @Mark Michelson  @Han Zhou  Do you have any thoughts ?
> >> or objections ?
>
> Hi all,
>
> >
> > Hi Numan, Lorenzo, I agree with Numan that adding the table=x in
ct_commit
> > action is more flexible, but I have a different concern. Either the
> > ct_commit_continue implemented by this patch or the ct_commit
{...,table=x}
> > proposed by Numan would introduce an extra recirculation, which has a
> > significant cost for data-plane. Since apply-after-lb is now frequently
> > used for ACLs that implement egress network policies, it is better to
avoid
> > this penalty if possible. Another option to solve the problem is to move
> > the ls_in_stateful stage after the hairpin stages, and keep the current
> > ct_commit. Would that work?
> >
>
> I think you're right, Han.  We seem to add quite a few extra
recirculations.
> For example, the datapath flows for a SYN packet hitting a load balancer
> (dnats from 66.66.66.66:666 to 42.42.42.2:4242):
>
>
recirc_id(0),in_port(5),eth(src=00:00:00:00:00:00/01:00:00:00:00:00,dst=00:00:00:00:01:00),eth_type(0x0800),ipv4(dst=66.66.66.66,proto=6,frag=no),tcp(dst=666),
packets:1, bytes:66, used:1.180s, flags:.,
actions:ct(zone=6,nat),recirc(0x50)
>
recirc_id(0x50),in_port(5),ct_state(+new-est-rel-rpl-inv+trk),ct_mark(0/0x1),eth(dst=00:00:00:00:01:00),eth_type(0x0800),ipv4(dst=66.66.66.66,proto=6,frag=no),tcp(dst=666),
packets:0, bytes:0, used:never, actions:hash(l4(0)),recirc(0x54)
>
recirc_id(0x54),dp_hash(0x7/0xf),in_port(5),eth(),eth_type(0x0800),ipv4(frag=no),
packets:0, bytes:0, used:never,
actions:ct(commit,zone=6,mark=0x2/0x2,nat(dst=42.42.42.2:4242)),recirc(0x55)
>
recirc_id(0x55),in_port(5),ct_state(+new-est-rel-rpl-inv+trk),ct_mark(0/0x1),eth(src=00:00:00:00:00:02,dst=00:00:00:00:01:00),eth_type(0x0800),ipv4(src=
42.42.42.2/255.255.255.254,dst=42.42.42.2,proto=6,ttl=64,frag=no),
packets:0, bytes:0, used:never,
actions:ct(commit,zone=6,mark=0/0x1,nat(src)),set(eth(src=00:00:00:00:01:00,dst=00:00:00:00:00:01)),set(ipv4(ttl=63)),ct(zone=5,nat),recirc(0x51)
>
recirc_id(0x51),in_port(5),ct_state(+new-est-rel-rpl-inv+trk),ct_mark(0/0x1),eth(src=00:00:00:00:01:00,dst=00:00:00:00:00:00/01:00:00:00:00:00),eth_type(0x0800),ipv4(frag=no),
packets:0, bytes:0, used:never,
actions:ct(commit,zone=5,mark=0/0x1,nat(src)),4
>
> Now, with Lorenzo's patch:
>
>
recirc_id(0),in_port(5),eth(src=00:00:00:00:00:00/01:00:00:00:00:00,dst=00:00:00:00:01:00),eth_type(0x0800),ipv4(dst=66.66.66.66,proto=6,frag=no),tcp(dst=666),
packets:1, bytes:66, used:1.893s, flags:.,
actions:ct(zone=6,nat),recirc(0x47)
>
recirc_id(0x47),in_port(5),ct_state(+new-est-rel-rpl-inv+trk),ct_mark(0/0x1),eth(dst=00:00:00:00:01:00),eth_type(0x0800),ipv4(dst=66.66.66.66,proto=6,frag=no),tcp(dst=666),
packets:0, bytes:0, used:never, actions:hash(l4(0)),recirc(0x48)
>
recirc_id(0x48),dp_hash(0x1/0xf),in_port(5),eth(),eth_type(0x0800),ipv4(frag=no),
packets:0, bytes:0, used:never,

[ovs-dev] [PATCH v2] ovsdb-cs: Consider default conditions implicitly acked.

2022-12-13 Thread Dumitru Ceara

When initializing a monitor table the default monitor condition is
[True] which matches the behavior of the server (to send all rows of
that table).  There's no need to include this default condition in the
initial monitor request so we can consider it implicitly acked by the
server.

Reported-by: Numan Siddique 
Suggested-by: Ilya Maximets 
Signed-off-by: Dumitru Ceara 
---
 lib/ovsdb-cs.c   |   2 +-
 python/ovs/db/idl.py |   4 +-
 tests/ovsdb-idl.at   | 105 +--
 tests/test-ovsdb.c   |  38 
 tests/test-ovsdb.py  |  38 
 5 files changed, 134 insertions(+), 53 deletions(-)

diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
index a6fbd290c87d..0fca03d7231e 100644
--- a/lib/ovsdb-cs.c
+++ b/lib/ovsdb-cs.c
@@ -892,7 +892,7 @@ ovsdb_cs_db_get_table(struct ovsdb_cs_db *db, const char 
*table)
 
 t = xzalloc(sizeof *t);
 t->name = xstrdup(table);
-t->new_cond = json_array_create_1(json_boolean_create(true));
+t->ack_cond = json_array_create_1(json_boolean_create(true));
 hmap_insert(>tables, >hmap_node, hash);
 return t;
 }
diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index fe66402cff42..9fc2159b04a1 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -85,9 +85,9 @@ class Monitor(enum.IntEnum):
 
 class ConditionState(object):
 def __init__(self):
-self._ack_cond = None
+self._ack_cond = [True]
 self._req_cond = None
-self._new_cond = [True]
+self._new_cond = None
 
 def __iter__(self):
 return iter([self._new_cond, self._req_cond, self._ack_cond])
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index c2970984bae3..5a7e76eaa95b 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -576,9 +576,9 @@ OVSDB_CHECK_IDL([simple idl, conditional, false condition],
"b": true}}]']],
   [['condition simple []' \
 'condition simple [true]']],
-  [[000: change conditions
+  [[000: simple: change conditions
 001: empty
-002: change conditions
+002: simple: change conditions
 003: table simple: i=1 r=2 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] 
uuid=<1>
 004: done
 ]])
@@ -592,13 +592,40 @@ OVSDB_CHECK_IDL([simple idl, conditional, true condition],
"b": true}}]']],
   [['condition simple []' \
 'condition simple [true]']],
-  [[000: change conditions
+  [[000: simple: change conditions
 001: empty
-002: change conditions
+002: simple: change conditions
 003: table simple: i=1 r=2 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] 
uuid=<1>
 004: done
 ]])
 
+dnl This test ensures that the first explicitly set monitor condition
+dnl is sent to the server.
+OVSDB_CHECK_IDL([simple idl, conditional, wait for condition],
+  [],
+  [['["idltest",
+   {"op": "insert",
+   "table": "simple",
+   "row": {"i": 1,
+   "r": 2.0,
+   "b": true}}]' \
+ 'condition simple [true]' \
+ '^["idltest",
+   {"op": "insert",
+   "table": "simple",
+   "row": {"i": 2,
+   "r": 4.0,
+   "b": true}}]']],
+  [[000: empty
+001: {"error":null,"result":[{"uuid":["uuid","<0>"]}]}
+002: table simple: i=1 r=2 b=true s= u=<1> ia=[] ra=[] ba=[] sa=[] ua=[] 
uuid=<0>
+003: simple: conditions unchanged
+004: {"error":null,"result":[{"uuid":["uuid","<2>"]}]}
+005: table simple: i=1 r=2 b=true s= u=<1> ia=[] ra=[] ba=[] sa=[] ua=[] 
uuid=<0>
+005: table simple: i=2 r=4 b=true s= u=<1> ia=[] ra=[] ba=[] sa=[] ua=[] 
uuid=<2>
+006: done
+]])
+
 OVSDB_CHECK_IDL([simple idl, conditional, multiple clauses in condition],
   [['["idltest",
{"op": "insert",
@@ -613,9 +640,9 @@ OVSDB_CHECK_IDL([simple idl, conditional, multiple clauses 
in condition],
"b": true}}]']],
   [['condition simple []' \
 'condition simple [["i","==",1],["i","==",2]]']],
-  [[000: change conditions
+  [[000: simple: change conditions
 001: empty
-002: change conditions
+002: simple: change conditions
 003: table simple: i=1 r=2 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] 
uuid=<1>
 003: table simple: i=2 r=3 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] 
uuid=<2>
 004: done
@@ -630,9 +657,9 @@ OVSDB_CHECK_IDL([simple idl, conditional, modify as insert 
due to condition],
"b": true}}]']],
   [['condition simple []' \
 'condition simple [["i","==",1]]']],
-  [[000: change conditions
+  [[000: simple: change conditions
 001: empty
-002: change conditions
+002: simple: change conditions
 003: table simple: i=1 r=2 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] 
uuid=<1>
 004: done
 ]])
@@ -653,11 +680,11 @@ OVSDB_CHECK_IDL([simple idl, conditional, modify as 
delete due to condition],
"row": {"i": 2,
"r": 3.0,
"b": true}}]']],
-  [[000: change conditions
+  [[000: simple: change conditions
 001: empty
-002: change conditions
+002: simple: change conditions
 003: table simple: i=1 r=2 b=true s= u=<0> ia=[] ra=[]

Re: [ovs-dev] [PATCH] ovsdb-cs: Always send first explicitly set monitor condition.

2022-12-13 Thread Ilya Maximets

On 12/13/22 17:02, Dumitru Ceara wrote:
> On 12/13/22 14:56, Ilya Maximets wrote:
>> On 12/13/22 13:21, Dumitru Ceara wrote:
>>> On 12/13/22 11:57, Ilya Maximets wrote:
 On 12/12/22 20:42, Ilya Maximets wrote:
> On 12/8/22 16:21, Dumitru Ceara wrote:
>> Applications request specific monitor conditions via the
>> ovsdb_cs_set_condition() API.  This returns the condition sequence
>> number at which the client can assume that the server acknowledged
>> and applied the new monitor condition.
>>
>> A corner case is when the client's first request is to set the condition
>> to a value that's equivalent to the default one ("").  In such
>> cases, because it's requested explicitly by the client, we now
>> explicitly send the monitor_cond_change request to the server.  This
>> avoids cases in which the expected condition sequence number and the
>> acked condition sequence number get out of sync.
>
> Sorry, I think I'm lost again.  What exactly we're trying to fix here?
> [True] is a default condition if no other conditions are supplied for
> the table.  Why do we need to send it explicitly?
>
>>>
>>> The problem is that there's a discrepancy between the returned expected
>>> condition seqno and what happens if a condition is explicitly set to
>>> [True] before vs after the initial connection happened.
>>>
> At first I thought that newly created condition from 
> ovsdb_cs_db_get_table()
> has to be sent out, but...  if it is the first time we're setting
> conditions for that table, default condition should be [True], hence
> equal to the condition we're trying to set.
>
>
> The test case is a bit synthetic as it doesn't seem to test the issue
> that exists in OVN.  The fact that python IDL passes the test without
> changes also doesn't make a lot of sense to me.
>
>>>
>>> Actually it should test exactly what's happenning in OVN.  That is:
>>>
>>> - connect to database, no explicit monitor condition set
>>> - get initial contents
>>> - explicitly set monitor condition to [True] and get seqno S at
>>>   which the condition should be "acked".
>>> - expect that ovsdb_idl_get_condition_seqno() eventually reaches S
>>>
>>> The Python IDL behaves differently unfortunately, I'll give more details
>>> below.
>>>
> Could you provide some more data on what exactly is going on and what
> we're trying to fix?  With jsonrpc logs, if possible.

>>>
>>> For the C client, in the ovn-controller case, let's focus on 2 tables
>>> Chassis_Private and Chassis_Template_Var.  The difference between these
>>> 2 is that Chassis_Template_Var was recently added to the schema so
>>> ovn-controller only sets its monitor condition if
>>> sbrec_server_has_chassis_template_var_table() is true.  So that's only
>>> after the remote schema was received.
>>>
>>> Sorry for the wide table below but this is the best way I could find
>>> that should explain what's happening:
>>>
>>>   operation/eventreconnect-state
>>> ovsdb-cs-state  Chassis_Private-new_cond 
>>> Chassis_Template_Var-new_cond  cond-seqno   expected-cond-seqno
>>>   
>>> 
>>> 1.set_cond(Chassis_Private, [True])
>>>  BACKOFF   
>>> SERVER_SCHEMA_REQUESTED [True]  
>>> non-existent0 1
>>
>>
>> I think, this very first operation/event is wrong.
>> The condition change is never going to be sent, but the expected
>> sequence number is reported as 1.  The only reason this doesn't
>> cause any problems right away is that we're going to reconnect
>> here and drop the state, so the application will not rely on that
>> number in the end.  But it is still an incorrect return value.
>>
>> The change that I suggested should fix that fundamental issue.
>>
> 
> It should, yes.
> 
>> 
>>

 Should we do this instead:

 diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
 index a6fbd290c..0fca03d72 100644
 --- a/lib/ovsdb-cs.c
 +++ b/lib/ovsdb-cs.c
 @@ -892,7 +892,7 @@ ovsdb_cs_db_get_table(struct ovsdb_cs_db *db, const 
 char *table)
  
  t = xzalloc(sizeof *t);
  t->name = xstrdup(table);
 -t->new_cond = json_array_create_1(json_boolean_create(true));
 +t->ack_cond = json_array_create_1(json_boolean_create(true));
  hmap_insert(>tables, >hmap_node, hash);
  return t;
  }
 ---

 ?

 This breaks a few tests, but it seems to be a correct change because it
 is indeed a default condition that server already has for the table.

 What do you think?  Will that fix the OVN

Re: [ovs-dev] [PATCH] ovsdb-cs: Always send first explicitly set monitor condition.

2022-12-13 Thread Dumitru Ceara

On 12/13/22 14:56, Ilya Maximets wrote:
> On 12/13/22 13:21, Dumitru Ceara wrote:
>> On 12/13/22 11:57, Ilya Maximets wrote:
>>> On 12/12/22 20:42, Ilya Maximets wrote:
 On 12/8/22 16:21, Dumitru Ceara wrote:
> Applications request specific monitor conditions via the
> ovsdb_cs_set_condition() API.  This returns the condition sequence
> number at which the client can assume that the server acknowledged
> and applied the new monitor condition.
>
> A corner case is when the client's first request is to set the condition
> to a value that's equivalent to the default one ("").  In such
> cases, because it's requested explicitly by the client, we now
> explicitly send the monitor_cond_change request to the server.  This
> avoids cases in which the expected condition sequence number and the
> acked condition sequence number get out of sync.

 Sorry, I think I'm lost again.  What exactly we're trying to fix here?
 [True] is a default condition if no other conditions are supplied for
 the table.  Why do we need to send it explicitly?

>>
>> The problem is that there's a discrepancy between the returned expected
>> condition seqno and what happens if a condition is explicitly set to
>> [True] before vs after the initial connection happened.
>>
 At first I thought that newly created condition from 
 ovsdb_cs_db_get_table()
 has to be sent out, but...  if it is the first time we're setting
 conditions for that table, default condition should be [True], hence
 equal to the condition we're trying to set.


 The test case is a bit synthetic as it doesn't seem to test the issue
 that exists in OVN.  The fact that python IDL passes the test without
 changes also doesn't make a lot of sense to me.

>>
>> Actually it should test exactly what's happenning in OVN.  That is:
>>
>> - connect to database, no explicit monitor condition set
>> - get initial contents
>> - explicitly set monitor condition to [True] and get seqno S at
>>   which the condition should be "acked".
>> - expect that ovsdb_idl_get_condition_seqno() eventually reaches S
>>
>> The Python IDL behaves differently unfortunately, I'll give more details
>> below.
>>
 Could you provide some more data on what exactly is going on and what
 we're trying to fix?  With jsonrpc logs, if possible.
>>>
>>
>> For the C client, in the ovn-controller case, let's focus on 2 tables
>> Chassis_Private and Chassis_Template_Var.  The difference between these
>> 2 is that Chassis_Template_Var was recently added to the schema so
>> ovn-controller only sets its monitor condition if
>> sbrec_server_has_chassis_template_var_table() is true.  So that's only
>> after the remote schema was received.
>>
>> Sorry for the wide table below but this is the best way I could find
>> that should explain what's happening:
>>
>>   operation/eventreconnect-state
>> ovsdb-cs-state  Chassis_Private-new_cond 
>> Chassis_Template_Var-new_cond  cond-seqno   expected-cond-seqno
>>   
>> 
>> 1.set_cond(Chassis_Private, [True])
>>  BACKOFF   
>> SERVER_SCHEMA_REQUESTED [True]  
>> non-existent0 1
> 
> 
> I think, this very first operation/event is wrong.
> The condition change is never going to be sent, but the expected
> sequence number is reported as 1.  The only reason this doesn't
> cause any problems right away is that we're going to reconnect
> here and drop the state, so the application will not rely on that
> number in the end.  But it is still an incorrect return value.
> 
> The change that I suggested should fix that fundamental issue.
> 

It should, yes.

> 
> 
>>>
>>> Should we do this instead:
>>>
>>> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
>>> index a6fbd290c..0fca03d72 100644
>>> --- a/lib/ovsdb-cs.c
>>> +++ b/lib/ovsdb-cs.c
>>> @@ -892,7 +892,7 @@ ovsdb_cs_db_get_table(struct ovsdb_cs_db *db, const 
>>> char *table)
>>>  
>>>  t = xzalloc(sizeof *t);
>>>  t->name = xstrdup(table);
>>> -t->new_cond = json_array_create_1(json_boolean_create(true));
>>> +t->ack_cond = json_array_create_1(json_boolean_create(true));
>>>  hmap_insert(>tables, >hmap_node, hash);
>>>  return t;
>>>  }
>>> ---
>>>
>>> ?
>>>
>>> This breaks a few tests, but it seems to be a correct change because it
>>> is indeed a default condition that server already has for the table.
>>>
>>> What do you think?  Will that fix the OVN oproblem?
>>>
>>
>> That would fix the OVN problem I think.  I'm not sure how to rewrite the
>> "[track, simple idl, initially populated, strong references,

[ovs-dev] [PATCH v6 15/15] tests: Comment currently failing TC system-traffic tests.

2022-12-13 Thread Eelco Chaudron

The goal was to run 200 successful tc tests in a row. To do this the
following was run:

  for i in {1..200}; do make check-offloads || break; \
echo "ALL_200_OK: $i"; done;

Unfortunately, a bunch of test cases showed occasional failures.
For now, they are excluded from the test cases and need further
investigation. They are:

  802.1ad - vlan_limit
  conntrack - DNAT load balancing
  conntrack - DNAT load balancing with NC
  conntrack - ICMP related
  conntrack - ICMP related to original direction
  conntrack - ICMP related with NAT
  conntrack - IPv4 fragmentation with fragments specified
  conntrack - multiple namespaces, internal ports
  conntrack - zones from other field
  conntrack - zones from other field, more tests
  datapath - basic truncate action
  datapath - multiple mpls label pop
  datapath - truncate and output to gre tunnel
  datapath - truncate and output to gre tunnel by simulated packets

Some other test cases also fail due to what looks like problems
in the tc kernel conntrack implementation. For details see the
details in the system-offloads.at exclusion list definition.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |   43 +--
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 34de0136d..9ee6b96d6 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -61,20 +61,51 @@ m4_define([CHECK_CONNTRACK_TIMEOUT],
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
+# TC does not support moving ports to a different namespace than vswitchd's
+# namespace, so we need to disable this test.
 conntrack - multiple namespaces, internal ports
+
+# When moving through different zones, it can take up to ~8 seconds before
+# the conntrack state gets updated causing these tests to fail.
 conntrack - ct metadata, multiple zones
-conntrack - ICMP related
-conntrack - ICMP related to original direction
+conntrack - multiple zones, local
+conntrack - multi-stage pipeline, local
+
+# The kernel's tcf_ct_act() function does not seem to take care of any (QinQ)
+# VLAN headers causing commits to fail. However, if this is solved, we have to
+# make sure conntrack does not break the VLAN boundary, i.e., putting together
+# two packets with different CVLAN+SVLAN values.
 conntrack - IPv4 fragmentation + cvlan
-conntrack - IPv4 fragmentation with fragments specified
 conntrack - IPv6 fragmentation + cvlan
+
+# Fragmentation handling in ct zone 9 does not seem to work correctly.
+# When moving this test over to the default zone all works fine.
 conntrack - Fragmentation over vxlan
 conntrack - IPv6 Fragmentation over vxlan
-conntrack - multiple zones, local
-conntrack - multi-stage pipeline, local
+
+# Occasionaly we fail on the 'execute ct(commit) failed (Invalid argument) on
+# packet...' log message being present
+conntrack - zones from other field
+conntrack - zones from other field, more tests
+conntrack - multiple namespaces, internal ports
+conntrack - IPv4 fragmentation with fragments specified
+
+# Occasionaly we fail on the 'failed to flow_get/flow_del (No such file or 
directory)
+# ufid:..' log message being present.
+datapath - multiple mpls label pop
+datapath - basic truncate action
+conntrack - ICMP related
+conntrack - ICMP related to original direction
 conntrack - ICMP related with NAT
 conntrack - DNAT load balancing
-conntrack - DNAT load balancing with NC"
+conntrack - DNAT load balancing with NC
+802.1ad - vlan_limit
+
+# Occasionalt we fail with extreme high byte counters, i.e.
+# n_bytes=18446744073705804134
+datapath - truncate and output to gre tunnel by simulated packets
+datapath - truncate and output to gre tunnel
+"
 echo "$ovs_test_skip_list" | sed "s// /g"])
 
 m4_include([tests/system-traffic.at])

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

[ovs-dev] [PATCH v6 13/15] netdev-offload-tc: If the flow has not been used, report it as such.

2022-12-13 Thread Eelco Chaudron

If a tc flow was installed but has not yet been used, report it as such.

In addition, add a delay to the "IGMP - flood under normal action" test
case to make it work with many repetitions. This delay is also present
in other ICMP/IGMP tests.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 lib/tc.c |   14 +-
 tests/system-offloads.at |3 +--
 tests/system-traffic.at  |2 ++
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/lib/tc.c b/lib/tc.c
index a66dc432f..78b2fa797 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -1361,7 +1361,19 @@ get_user_hz(void)
 static void
 nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower)
 {
-uint64_t lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
+uint64_t lastused;
+
+/* On creation both tm->install and tm->lastuse are set to jiffies
+ * by the kernel. So if both values are the same, the flow has not been
+ * used yet.
+ *
+ * Note that tm->firstuse can not be used due to some kernel bug, i.e.,
+ * hardware offloaded flows do not update tm->firstuse. */
+if (tm->lastuse == tm->install) {
+lastused = 0;
+} else {
+lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
+}
 
 if (flower->lastused < lastused) {
 flower->lastused = lastused;
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 102e89a1f..9db68b2a0 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -76,8 +76,7 @@ conntrack - multiple zones, local
 conntrack - multi-stage pipeline, local
 conntrack - ICMP related with NAT
 conntrack - DNAT load balancing
-conntrack - DNAT load balancing with NC
-IGMP - flood under normal action"
+conntrack - DNAT load balancing with NC"
 echo "$ovs_test_skip_list" | sed "s// /g"])
 
 m4_include([tests/system-traffic.at])
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 07913a192..57ff83b51 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -7002,6 +7002,8 @@ f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 
49 45 65 eb 4a e0 dnl
 00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl
 00 00 00 00 > /dev/null])
 
+sleep 1
+
 AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep -e .*ipv4 | sort | dnl
   strip_stats | strip_used | strip_recirc | dnl
   sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'],

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

[ovs-dev] [PATCH v6 14/15] tests: Fix reading of OpenFlow byte counters in GRE test cases.

2022-12-13 Thread Eelco Chaudron

With some datapaths, read TC, it takes a bit longer to update the
OpenFlow statistics. Rather than adding an additional delay, try
to read the counters multiple times until we get the desired value.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |2 --
 tests/system-traffic.at  |   15 ++-
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 9db68b2a0..34de0136d 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -61,8 +61,6 @@ m4_define([CHECK_CONNTRACK_TIMEOUT],
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
-datapath - truncate and output to gre tunnel by simulated packets
-datapath - truncate and output to gre tunnel
 conntrack - multiple namespaces, internal ports
 conntrack - ct metadata, multiple zones
 conntrack - ICMP related
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 57ff83b51..ae21dace9 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -1660,9 +1660,8 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=2" | 
sed -n 's/.*\(n\_bytes=[
 n_bytes=242
 ])
 dnl After truncation = outer ETH(14) + outer IP(20) + GRE(4) + 100 = 138B
-AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
-n_bytes=138
-])
+OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed 
-n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p' | grep "n_bytes=138"],
+   [ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed 
-n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'])
 
 dnl check tunnel pop path, from at_ns0 to at_ns1
 dnl This 200-byte packet is simulated on behalf of ns_gre0
@@ -1697,9 +1696,8 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=2" | 
sed -n 's/.*\(n\_bytes=[
 n_bytes=242
 ])
 dnl After truncation = outer ETH(14) + outer IP(20) + GRE(4) + 100 = 138B
-AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
-n_bytes=138
-])
+OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed 
-n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p' | grep "n_bytes=138"],
+   [ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed 
-n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'])
 
 dnl check tunnel pop path, from at_ns0 to at_ns1
 dnl This 200-byte packet is simulated on behalf of ns_gre0
@@ -1707,9 +1705,8 @@ ovs-ofctl -O OpenFlow13 packet-out br-underlay "in_port=1 
packet=02908ca8a149faa
 
 dnl After truncation = 100 byte at loopback device p2(4)
 OVS_REVALIDATOR_PURGE()
-AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip], [0], [dnl
- n_packets=1, n_bytes=100, priority=1,ip,in_port=4 actions=drop
-])
+OVS_WAIT_UNTIL([ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip | 
grep "n_packets=1, n_bytes=100, priority=1,ip,in_port=4 actions=drop"],
+   [ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip])
 
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP

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

[ovs-dev] [PATCH v6 12/15] odp-util: Make odp_flow_key_from_flow__ nlattr order the same as the kernel.

2022-12-13 Thread Eelco Chaudron

Make the order of the Netlink attributes for odp_flow_key_from_flow__()
the same as the kernel will return them.

This will make sure the attributes displayed in the dpctl/dump-flows
output appear in the same order for all datapath.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 lib/odp-util.c|   21 +-
 tests/dpif-netdev.at  |   28 +++---
 tests/mcast-snooping.at   |4 +-
 tests/nsh.at  |   10 ++---
 tests/odp.at  |   83 +++--
 tests/ofproto-dpif.at |   30 +++
 tests/packet-type-aware.at|   22 +--
 tests/pmd.at  |2 -
 tests/system-offloads.at  |1 
 tests/tunnel-push-pop-ipv6.at |2 -
 tests/tunnel-push-pop.at  |2 -
 tests/tunnel.at   |2 -
 12 files changed, 101 insertions(+), 106 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 72e076e1c..9baca12d8 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -6197,6 +6197,11 @@ odp_flow_key_from_flow__(const struct odp_flow_key_parms 
*parms,
 const struct flow *mask = parms->mask;
 const struct flow *data = export_mask ? mask : flow;
 
+if (parms->support.recirc) {
+nl_msg_put_u32(buf, OVS_KEY_ATTR_RECIRC_ID, data->recirc_id);
+nl_msg_put_u32(buf, OVS_KEY_ATTR_DP_HASH, data->dp_hash);
+}
+
 nl_msg_put_u32(buf, OVS_KEY_ATTR_PRIORITY, data->skb_priority);
 
 if (flow_tnl_dst_is_set(>tunnel) ||
@@ -6205,6 +6210,12 @@ odp_flow_key_from_flow__(const struct odp_flow_key_parms 
*parms,
 parms->key_buf, NULL);
 }
 
+/* Add an ingress port attribute if this is a mask or 'in_port.odp_port'
+ * is not the magical value "ODPP_NONE". */
+if (export_mask || flow->in_port.odp_port != ODPP_NONE) {
+nl_msg_put_odp_port(buf, OVS_KEY_ATTR_IN_PORT, data->in_port.odp_port);
+}
+
 nl_msg_put_u32(buf, OVS_KEY_ATTR_SKB_MARK, data->pkt_mark);
 
 if (parms->support.ct_state) {
@@ -6248,16 +6259,6 @@ odp_flow_key_from_flow__(const struct odp_flow_key_parms 
*parms,
 ct->ipv6_proto = data->ct_nw_proto;
 }
 }
-if (parms->support.recirc) {
-nl_msg_put_u32(buf, OVS_KEY_ATTR_RECIRC_ID, data->recirc_id);
-nl_msg_put_u32(buf, OVS_KEY_ATTR_DP_HASH, data->dp_hash);
-}
-
-/* Add an ingress port attribute if this is a mask or 'in_port.odp_port'
- * is not the magical value "ODPP_NONE". */
-if (export_mask || flow->in_port.odp_port != ODPP_NONE) {
-nl_msg_put_odp_port(buf, OVS_KEY_ATTR_IN_PORT, data->in_port.odp_port);
-}
 
 nl_msg_put_be32(buf, OVS_KEY_ATTR_PACKET_TYPE, data->packet_type);
 
diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
index 6aff1eda7..04c82c109 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -72,13 +72,13 @@ ovs-appctl time/warp 5000
 AT_CHECK([ovs-appctl netdev-dummy/receive p1 
'in_port(1),eth(src=50:54:00:00:00:01,dst=50:54:00:00:02:00),eth_type(0x0800),ipv4(src=10.0.0.1,dst=10.0.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)'])
OVS_WAIT_UNTIL([grep "miss upcall" ovs-vswitchd.log])
AT_CHECK([grep -A 1 'miss upcall' ovs-vswitchd.log | tail -n 1], [0], [dnl
-skb_priority(0),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),recirc_id(0),dp_hash(0),in_port(1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:01,dst=50:54:00:00:02:00),eth_type(0x0800),ipv4(src=10.0.0.1,dst=10.0.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)
+recirc_id(0),dp_hash(0),skb_priority(0),in_port(1),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:01,dst=50:54:00:00:02:00),eth_type(0x0800),ipv4(src=10.0.0.1,dst=10.0.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)
 ])
 
 AT_CHECK([ovs-appctl netdev-dummy/receive p1 
'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:06:00),eth_type(0x0800),ipv4(src=10.0.0.5,dst=10.0.0.6,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)'
 --len 1024])
OVS_WAIT_UNTIL([test `grep -c "miss upcall" ovs-vswitchd.log` -ge 2])
AT_CHECK([grep -A 1 'miss upcall' ovs-vswitchd.log | tail -n 1], [0], [dnl
-skb_priority(0),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),recirc_id(0),dp_hash(0),in_port(1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:06:00),eth_type(0x0800),ipv4(src=10.0.0.5,dst=10.0.0.6,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)
+recirc_id(0),dp_hash(0),skb_priority(0),in_port(1),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:06:00),eth_type(0x0800),ipv4(src=10.0.0.5,dst=10.0.0.6,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)
 ])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
@@ -139,7 +139,7 @@ m4_define([DPIF_NETDEV_MISS_FLOW_INSTALL],
 
OVS_WAIT_UNTIL([grep "miss upcall"

[ovs-dev] [PATCH v6 11/15] test: Fix 'conntrack - Multiple ICMP traverse' for tc case.

2022-12-13 Thread Eelco Chaudron

tc does not include ethernet header length in packet byte count.
This fix will allow the packets that go trough tc to be 14 bytes less.

This difference in the TC implementation is already described in
tc-offload.rst.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |1 -
 tests/system-traffic.at  |2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index a94b4a64b..ef9c51309 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -77,7 +77,6 @@ conntrack - multi-stage pipeline, local
 conntrack - ICMP related with NAT
 conntrack - DNAT load balancing
 conntrack - DNAT load balancing with NC
-conntrack - Multiple ICMP traverse
 conntrack - can match and clear ct_state from outside OVS
 IGMP - flood under normal action"
 echo "$ovs_test_skip_list" | sed "s// /g"])
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 980b0bd70..07913a192 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -6929,7 +6929,7 @@ AT_CHECK([DPCTL_DUMP_CONNTRACK | FORMAT_CT(10.1.1)], [0], 
[dnl
 
icmp,orig=(src=10.1.1.1,dst=10.1.1.2,id=,type=8,code=0),reply=(src=10.1.1.2,dst=10.1.1.1,id=,type=0,code=0)
 ])
 
-AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | 
OFPROTO_CLEAR_DURATION_IDLE],
+AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | 
OFPROTO_CLEAR_DURATION_IDLE | sed 's/n_bytes=70,/n_bytes=84,/'],
  [0], [dnl
  cookie=0x0, duration=, table=2, n_packets=2, n_bytes=84, 
idle_age=, priority=10,ct_state=+new+trk,in_port=1 actions=drop
  cookie=0x0, duration=, table=2, n_packets=0, n_bytes=0, 
idle_age=, priority=10,ct_state=+est+trk actions=drop

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

[ovs-dev] [PATCH v6 10/15] test: tc does not support conntrack timeout, skip the related test.

2022-12-13 Thread Eelco Chaudron

The tc conntrack implementation does not support the timeout option.
The current implementation is silently ignoring the timeout option
by adding a general conntrack entry.

This patch will skip the related test by overriding the support macro.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 73a761316..a94b4a64b 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -50,6 +50,13 @@ m4_define([CHECK_CONNTRACK_ALG],
 ])
 
 
+# Conntrack timeout not supported for tc.
+m4_define([CHECK_CONNTRACK_TIMEOUT],
+[
+ AT_SKIP_IF([:])
+])
+
+
 # The list below are tests that will not pass for a "test environment" specific
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
@@ -65,7 +72,6 @@ conntrack - IPv4 fragmentation with fragments specified
 conntrack - IPv6 fragmentation + cvlan
 conntrack - Fragmentation over vxlan
 conntrack - IPv6 Fragmentation over vxlan
-conntrack - zone-based timeout policy
 conntrack - multiple zones, local
 conntrack - multi-stage pipeline, local
 conntrack - ICMP related with NAT

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

[ovs-dev] [PATCH v6 09/15] netdev-offload-tc: Conntrack ALGs are not supported with tc.

2022-12-13 Thread Eelco Chaudron

tc does not support conntrack ALGs. Even worse, with tc enabled, they
should not be used/configured at all. This is because even though TC
will ignore the rules with ALG configured, i.e., they will flow through
the kernel module, return traffic might flow through a tc conntrack
rule, and it will not invoke the ALG helper.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 Documentation/howto/tc-offload.rst |   11 +++
 lib/netdev-offload-tc.c|4 
 tests/system-offloads.at   |   28 
 3 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/Documentation/howto/tc-offload.rst 
b/Documentation/howto/tc-offload.rst
index f6482c8af..63687adc9 100644
--- a/Documentation/howto/tc-offload.rst
+++ b/Documentation/howto/tc-offload.rst
@@ -112,3 +112,14 @@ First flow packet not processed by meter
 Packets that are received by ovs-vswitchd through an upcall before the actual
 meter flow is installed, are not passing TC police action and therefore are
 not considered for policing.
+
+Conntrack Application Layer Gateways(ALG)
++
+
+TC does not support conntrack helpers, i.e., ALGs. TC will not offload flows if
+the ALG keyword is present within the ct() action. However, this will not allow
+ALGs to work within the datapath, as the return traffic without the ALG keyword
+might run through a TC rule, which internally will not call the conntrack
+helper required.
+
+So if ALG support is required, tc offload must be disabled.
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 915c45ed3..ba309c2b6 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1357,6 +1357,10 @@ parse_put_flow_ct_action(struct tc_flower *flower,
 action->ct.label_mask = ct_label->mask;
 }
 break;
+/* The following option we do not support in tc-ct, and should
+ * not be ignored for proper operation. */
+case OVS_CT_ATTR_HELPER:
+return EOPNOTSUPP;
 }
 }
 
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 9d1e80c8d..73a761316 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -30,6 +30,7 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START],
AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| 
uuidfilt])], [0], [$2])
 ])
 
+<<< current
 
 # We override the OVS_REVALIDATOR_PURGE macro, allowing a bit more time for the
 # tc-datapath entries to be installed.
@@ -42,6 +43,13 @@ m4_define([OVS_REVALIDATOR_PURGE],
 m4_define([DPCTL_DUMP_CONNTRACK], [sleep 3; ovs-appctl dpctl/dump-conntrack])
 
 
+# Conntrack ALGs are not supported for tc.
+m4_define([CHECK_CONNTRACK_ALG],
+[
+ AT_SKIP_IF([:])
+])
+
+
 # The list below are tests that will not pass for a "test environment" specific
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
@@ -60,27 +68,7 @@ conntrack - IPv6 Fragmentation over vxlan
 conntrack - zone-based timeout policy
 conntrack - multiple zones, local
 conntrack - multi-stage pipeline, local
-conntrack - FTP
-conntrack - FTP over IPv6
-conntrack - IPv6 FTP Passive
-conntrack - FTP with multiple expectations
-conntrack - TFTP
 conntrack - ICMP related with NAT
-conntrack - FTP SNAT prerecirc
-conntrack - FTP SNAT prerecirc seqadj
-conntrack - FTP SNAT postrecirc
-conntrack - FTP SNAT postrecirc seqadj
-conntrack - FTP SNAT orig tuple
-conntrack - FTP SNAT orig tuple seqadj
-conntrack - IPv4 FTP Passive with SNAT
-conntrack - IPv4 FTP Passive with DNAT
-conntrack - IPv4 FTP Passive with DNAT 2
-conntrack - IPv4 FTP Active with DNAT
-conntrack - IPv4 FTP Active with DNAT with reverse skew
-conntrack - IPv6 FTP with SNAT
-conntrack - IPv6 FTP Passive with SNAT
-conntrack - IPv6 FTP with SNAT - orig tuple
-conntrack - IPv4 TFTP with SNAT
 conntrack - DNAT load balancing
 conntrack - DNAT load balancing with NC
 conntrack - Multiple ICMP traverse

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

[ovs-dev] [PATCH v6 08/15] test: Flush datapath when changing rules on the fly.

2022-12-13 Thread Eelco Chaudron

Flush datapath flows as TC flows take some more time to be flushed out.
The flush speeds this up.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |2 --
 tests/system-traffic.at  |6 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 593dc1c7a..9d1e80c8d 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -48,8 +48,6 @@ m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
 datapath - truncate and output to gre tunnel by simulated packets
 datapath - truncate and output to gre tunnel
-conntrack - zones from other field
-conntrack - zones from other field, more tests
 conntrack - multiple namespaces, internal ports
 conntrack - ct metadata, multiple zones
 conntrack - ICMP related
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 2f6d8f13f..980b0bd70 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2694,6 +2694,9 @@ AT_CHECK([ovs-appctl dpctl/dump-flows --names 
filter=in_port=ovs-p0 dnl
 AT_CHECK([ovs-ofctl mod-flows br0 dnl
 'priority=100,ct_state=-trk,tcp,in_port="ovs-p0" 
actions=ct(table=0,zone=15)'])
 
+dnl Force flush flows as some datapaths (read TC) might take time to clear.
+AT_CHECK([ovs-appctl dpctl/del-flows])
+
 NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o 
wget0.log])
 
 AT_CHECK([ovs-appctl dpctl/dump-flows --names filter=in_port=ovs-p0 dnl
@@ -2742,6 +2745,9 @@ AT_CHECK([ovs-appctl dpctl/dump-flows --names 
filter=in_port=ovs-p0 dnl
 
 AT_CHECK([ovs-ofctl mod-flows br0 
'priority=100,ct_state=-trk,tcp,in_port="ovs-p0" 
actions=ct(table=0,zone=15,commit,exec(load:0x000f->NXM_NX_CT_LABEL[[0..31]]))'])
 
+dnl Force flush flows as some datapaths (read TC) might take time to clear.
+AT_CHECK([ovs-appctl dpctl/del-flows])
+
 NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o 
wget0.log])
 
 AT_CHECK([ovs-appctl dpctl/dump-flows --names filter=in_port=ovs-p0 dnl

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

[ovs-dev] [PATCH v6 07/15] test: Fix "conntrack - floating IP" test for TC.

2022-12-13 Thread Eelco Chaudron

This change fixes the "conntrack - floating" test for the TC
offload case. In this scenario, the connection might move to
CLOSE_WAIT, which would fail the test as it only accepts
TIME_WAIT. However, both indicate the connection was
established, so the test should pass.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |1 -
 tests/system-traffic.at  |   13 +++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 1aca41825..593dc1c7a 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -85,7 +85,6 @@ conntrack - IPv6 FTP with SNAT - orig tuple
 conntrack - IPv4 TFTP with SNAT
 conntrack - DNAT load balancing
 conntrack - DNAT load balancing with NC
-conntrack - floating IP
 conntrack - Multiple ICMP traverse
 conntrack - can match and clear ct_state from outside OVS
 IGMP - flood under normal action"
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 48545f57d..2f6d8f13f 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -6808,16 +6808,17 @@ AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
 
 dnl non-FIP case
 NS_CHECK_EXEC([at_ns1], [echo "foobar" |nc $NC_EOF_OPT 10.1.1.1 1234])
-OVS_WAIT_UNTIL([[ovs-appctl dpctl/dump-conntrack | sed -e 
's/port=[0-9]*/port=/g' -e 's/id=[0-9]*/id=/g' |
+OVS_WAIT_UNTIL([[ovs-appctl dpctl/dump-conntrack | sed -e 
's/port=[0-9]*/port=/g' -e 's/id=[0-9]*/id=/g' -e 
's/CLOSE_WAIT\|CLOSING/TIME_WAIT/g' |
 grep 
"tcp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=,dport=),reply=(src=10.1.1.1,dst=10.1.1.2,sport=,dport=),protoinfo=(state=TIME_WAIT)"
 ]])
 
-dnl Check that the full session ends as expected (i.e. TIME_WAIT). Otherwise it
-dnl means the datapath didn't process the ct_clear action. Ending in SYN_RECV
-dnl (OVS maps to ESTABLISHED) means the initial frame was committed, but not a
-dnl second time after the FIP translation (because ct_clear didn't occur).
+dnl Check that the full session ends as expected (i.e. TIME_WAIT, CLOSE_WAIT).
+dnl Otherwise it means the datapath didn't process the ct_clear action. Ending
+dnl in SYN_RECV (OVS maps to ESTABLISHED) means the initial frame was
+dnl committed, but not a second time after the FIP translation (because
+dnl ct_clear didn't occur).
 NS_CHECK_EXEC([at_ns1], [echo "foobar" |nc $NC_EOF_OPT 10.254.254.1 1234])
-OVS_WAIT_UNTIL([[ovs-appctl dpctl/dump-conntrack | sed -e 
's/port=[0-9]*/port=/g' -e 's/id=[0-9]*/id=/g' |
+OVS_WAIT_UNTIL([[ovs-appctl dpctl/dump-conntrack | sed -e 
's/port=[0-9]*/port=/g' -e 's/id=[0-9]*/id=/g'  -e 
's/CLOSE_WAIT\|CLOSING/TIME_WAIT/g' |
 grep 
"tcp,orig=(src=10.254.254.2,dst=10.1.1.1,sport=,dport=),reply=(src=10.1.1.1,dst=10.254.254.2,sport=,dport=),protoinfo=(state=TIME_WAIT)"
 ]])
 

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

[ovs-dev] [PATCH v6 06/15] tests: Add delay to dump-conntrack for tc test cases.

2022-12-13 Thread Eelco Chaudron

This patch adds a delay before dumping the conntrack table because with
tc it takes a bit longer before it gets synced.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-common-macros.at |3 +
 tests/system-offloads.at  |   25 +
 tests/system-traffic.at   |  198 +
 3 files changed, 107 insertions(+), 119 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index d95d79791..32b9ca0de 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -347,3 +347,6 @@ m4_define([OVS_CHECK_CT_CLEAR],
 # OVS_REVALIDATOR_PURGE()
 m4_define([OVS_REVALIDATOR_PURGE],
 [AT_CHECK([ovs-appctl revalidator/purge], [0])])
+
+# DPCTL_DUMP_CONNTRACK()
+m4_define([DPCTL_DUMP_CONNTRACK], [ovs-appctl dpctl/dump-conntrack])
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index d39997708..1aca41825 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -37,24 +37,20 @@ m4_define([OVS_REVALIDATOR_PURGE],
 [AT_CHECK([sleep 2; ovs-appctl revalidator/purge], [0])])
 
 
+# We override the DPCTL_DUMP_CONNTRACK macro, allowing a bit more time for the
+# tc-datapath conntrack entries to be installed/updated.
+m4_define([DPCTL_DUMP_CONNTRACK], [sleep 3; ovs-appctl dpctl/dump-conntrack])
+
+
 # The list below are tests that will not pass for a "test environment" specific
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
 datapath - truncate and output to gre tunnel by simulated packets
 datapath - truncate and output to gre tunnel
-conntrack - preserve registers
-conntrack - zones
-conntrack - zones from field
 conntrack - zones from other field
 conntrack - zones from other field, more tests
-conntrack - multiple zones
 conntrack - multiple namespaces, internal ports
-conntrack - ct_mark
-conntrack - ct_mark bit-fiddling
-conntrack - ct_mark from register
-conntrack - ct_label
-conntrack - ct_label bit-fiddling
 conntrack - ct metadata, multiple zones
 conntrack - ICMP related
 conntrack - ICMP related to original direction
@@ -64,8 +60,6 @@ conntrack - IPv6 fragmentation + cvlan
 conntrack - Fragmentation over vxlan
 conntrack - IPv6 Fragmentation over vxlan
 conntrack - zone-based timeout policy
-conntrack - IPv4 HTTP
-conntrack - IPv6 HTTP
 conntrack - multiple zones, local
 conntrack - multi-stage pipeline, local
 conntrack - FTP
@@ -73,14 +67,6 @@ conntrack - FTP over IPv6
 conntrack - IPv6 FTP Passive
 conntrack - FTP with multiple expectations
 conntrack - TFTP
-conntrack - simple SNAT
-conntrack - SNAT with port range
-conntrack - SNAT with port range with exhaustion
-conntrack - more complex SNAT
-conntrack - all-zero IP SNAT
-conntrack - simple DNAT
-conntrack - DNAT with additional SNAT
-conntrack - more complex DNAT
 conntrack - ICMP related with NAT
 conntrack - FTP SNAT prerecirc
 conntrack - FTP SNAT prerecirc seqadj
@@ -93,7 +79,6 @@ conntrack - IPv4 FTP Passive with DNAT
 conntrack - IPv4 FTP Passive with DNAT 2
 conntrack - IPv4 FTP Active with DNAT
 conntrack - IPv4 FTP Active with DNAT with reverse skew
-conntrack - IPv6 HTTP with DNAT
 conntrack - IPv6 FTP with SNAT
 conntrack - IPv6 FTP Passive with SNAT
 conntrack - IPv6 FTP with SNAT - orig tuple
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 1d0d0dfd5..48545f57d 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2215,7 +2215,7 @@ 
udp,vlan_tci=0x,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_src=10.
 dnl
 dnl Check that the directionality has been changed by force commit.
 dnl
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.2,"], 
[], [dnl
+AT_CHECK([DPCTL_DUMP_CONNTRACK | grep "orig=.src=10\.1\.1\.2,"], [], [dnl
 
udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2)
 ])
 
@@ -2223,7 +2223,7 @@ dnl OK, now send another packet from port 1 and see that 
it switches again
 AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 
packet=5054000a505400090800451c0011a4cd0a0101010a010102000100020008
 actions=resubmit(,0)"])
 OVS_REVALIDATOR_PURGE()
 
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.1,"], 
[], [dnl
+AT_CHECK([DPCTL_DUMP_CONNTRACK | grep "orig=.src=10\.1\.1\.1,"], [], [dnl
 
udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1)
 ])
 
@@ -2253,25 +2253,25 @@ AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
 dnl Test UDP from port 1
 AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 
packet=5054000a505400090800451c0011a4cd0a0101010a010102000100020008
 actions=resubmit(,0)"])
 
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.1,"], 
[], [dnl
+AT_CHECK([DPCTL_DUMP_CONNTRACK | grep "orig=.src=10\.1\.1\.1,"], [], [dnl

[ovs-dev] [PATCH v6 03/15] test: Do not use MPLS implicit null label in test cases.

2022-12-13 Thread Eelco Chaudron

TC flower does not allow the push of the implicit null labels (RFC3032).
Avoid the use of such labels in the MPLS test cases.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |2 --
 tests/system-traffic.at  |8 
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index f6dd931b7..fbe1dc99a 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -34,8 +34,6 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START],
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
-datapath - mpls actions
-datapath - multiple mpls label pop
 datapath - basic truncate action
 datapath - truncate and output to gre tunnel by simulated packets
 datapath - truncate and output to gre tunnel
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index e5403519f..cd3ad0f68 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -1167,8 +1167,8 @@ AT_CHECK([ovs-vsctl add-port br0 patch0])
 AT_CHECK([ovs-vsctl add-port br1 patch1])
 
 AT_DATA([flows.txt], [dnl
-table=0,priority=100,dl_type=0x0800 
actions=push_mpls:0x8847,set_mpls_label:3,resubmit(,1)
-table=0,priority=100,dl_type=0x8847,mpls_label=3 
actions=pop_mpls:0x0800,resubmit(,1)
+table=0,priority=100,dl_type=0x0800 
actions=push_mpls:0x8847,set_mpls_label:4,resubmit(,1)
+table=0,priority=100,dl_type=0x8847,mpls_label=4 
actions=pop_mpls:0x0800,resubmit(,1)
 table=0,priority=10 actions=resubmit(,1)
 table=1,priority=10 actions=normal
 ])
@@ -1204,10 +1204,10 @@ AT_CHECK([ovs-vsctl add-port br0 patch0])
 AT_CHECK([ovs-vsctl add-port br1 patch1])
 
 AT_DATA([flows.txt], [dnl
-table=0,priority=100,dl_type=0x0800 
actions=push_mpls:0x8847,set_mpls_label:3,push_mpls:0x8847,set_mpls_label:2,push_mpls:0x8847,set_mpls_label:1,resubmit(,3)
+table=0,priority=100,dl_type=0x0800 
actions=push_mpls:0x8847,set_mpls_label:4,push_mpls:0x8847,set_mpls_label:2,push_mpls:0x8847,set_mpls_label:1,resubmit(,3)
 table=0,priority=100,dl_type=0x8847,mpls_label=1 
actions=pop_mpls:0x8847,resubmit(,1)
 table=1,priority=100,dl_type=0x8847,mpls_label=2 
actions=pop_mpls:0x8847,resubmit(,2)
-table=2,priority=100,dl_type=0x8847,mpls_label=3 
actions=pop_mpls:0x0800,resubmit(,3)
+table=2,priority=100,dl_type=0x8847,mpls_label=4 
actions=pop_mpls:0x0800,resubmit(,3)
 table=0,priority=10 actions=resubmit(,3)
 table=3,priority=10 actions=normal
 ])

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

[ovs-dev] [PATCH v6 05/15] netdev-offload-tc: Fix tc conntrack force commit support.

2022-12-13 Thread Eelco Chaudron

tc was not setting the OVS_CT_ATTR_FORCE_COMMIT flag when a forced
commit was requested. This patch will fix this.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 lib/netdev-offload-tc.c  |   13 +++--
 tests/system-offloads.at |1 -
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index ce7f8ad97..915c45ed3 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -825,7 +825,11 @@ parse_tc_flower_to_actions__(struct tc_flower *flower, 
struct ofpbuf *buf,
 ct_offset = nl_msg_start_nested(buf, OVS_ACTION_ATTR_CT);
 
 if (action->ct.commit) {
-nl_msg_put_flag(buf, OVS_CT_ATTR_COMMIT);
+if (action->ct.force) {
+nl_msg_put_flag(buf, OVS_CT_ATTR_FORCE_COMMIT);
+} else {
+nl_msg_put_flag(buf, OVS_CT_ATTR_COMMIT);
+}
 }
 
 if (action->ct.zone) {
@@ -1309,7 +1313,12 @@ parse_put_flow_ct_action(struct tc_flower *flower,
 NL_ATTR_FOR_EACH_UNSAFE (ct_attr, ct_left, ct, ct_len) {
 switch (nl_attr_type(ct_attr)) {
 case OVS_CT_ATTR_COMMIT: {
-action->ct.commit = true;
+action->ct.commit = true;
+}
+break;
+case OVS_CT_ATTR_FORCE_COMMIT: {
+action->ct.commit = true;
+action->ct.force = true;
 }
 break;
 case OVS_CT_ATTR_ZONE: {
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 7b6deccf0..d39997708 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -43,7 +43,6 @@ m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
 datapath - truncate and output to gre tunnel by simulated packets
 datapath - truncate and output to gre tunnel
-conntrack - force commit
 conntrack - preserve registers
 conntrack - zones
 conntrack - zones from field

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

[ovs-dev] [PATCH v6 04/15] test: Add delay on revalidator flush for offload test cases.

2022-12-13 Thread Eelco Chaudron

The revalidator/purge commands in the system test cases sometimes
get called immediately after a partial test is completed. This
could cause the revalidator thread to log an error that it can
not find/delete a flow due to the slower flow installation nature
of TC.

This patch uses a macro to call the revalidator/purge command,
which can be overwritten when the system tests are run on a tc
enabled datapath.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-common-macros.at |4 
 tests/system-offloads.at  |8 +++-
 tests/system-traffic.at   |   38 +++---
 3 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 8b9f5c752..d95d79791 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -343,3 +343,7 @@ m4_define([OVS_CHECK_IPROUTE_ENCAP],
 # OVS_CHECK_CT_CLEAR()
 m4_define([OVS_CHECK_CT_CLEAR],
 [AT_SKIP_IF([! grep -q "Datapath supports ct_clear action" 
ovs-vswitchd.log])])
+
+# OVS_REVALIDATOR_PURGE()
+m4_define([OVS_REVALIDATOR_PURGE],
+[AT_CHECK([ovs-appctl revalidator/purge], [0])])
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index fbe1dc99a..7b6deccf0 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -30,11 +30,17 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START],
AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| 
uuidfilt])], [0], [$2])
 ])
 
+
+# We override the OVS_REVALIDATOR_PURGE macro, allowing a bit more time for the
+# tc-datapath entries to be installed.
+m4_define([OVS_REVALIDATOR_PURGE],
+[AT_CHECK([sleep 2; ovs-appctl revalidator/purge], [0])])
+
+
 # The list below are tests that will not pass for a "test environment" specific
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
-datapath - basic truncate action
 datapath - truncate and output to gre tunnel by simulated packets
 datapath - truncate and output to gre tunnel
 conntrack - force commit
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index cd3ad0f68..1d0d0dfd5 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -1517,12 +1517,12 @@ on_exit 'rm -f payload200.bin'
 NS_CHECK_EXEC([at_ns0], [nc $NC_EOF_OPT -u 10.1.1.2 1234 < payload200.bin])
 
 dnl packet with truncated size
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=3" |  sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
 n_bytes=100
 ])
 dnl packet with original size
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=5" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
 n_bytes=242
 ])
@@ -1539,7 +1539,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 NS_CHECK_EXEC([at_ns0], [nc $NC_EOF_OPT -u 10.1.1.2 1234 < payload200.bin])
 
 dnl 100 + 100 + 242 + min(65535,242) = 684
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=3" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
 n_bytes=684
 ])
@@ -1569,7 +1569,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 NS_CHECK_EXEC([at_ns0], [nc $NC_EOF_OPT -u 10.1.1.2 1234 < payload200.bin])
 
 dnl 100 + 100 + 242 + min(65535,242) = 684
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=3" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
 n_bytes=684
 ])
@@ -1653,7 +1653,7 @@ AT_CHECK([ovs-ofctl add-flows br-underlay 
flows-underlay.txt])
 
 dnl check tunnel push path, from at_ns1 to at_ns0
 NS_CHECK_EXEC([at_ns1], [nc $NC_EOF_OPT -u 10.1.1.1 1234 < payload200.bin])
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 
 dnl Before truncation = ETH(14) + IP(20) + UDP(8) + 200 = 242B
 AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=2" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
@@ -1669,7 +1669,7 @@ dnl This 200-byte packet is simulated on behalf of ns_gre0
 ovs-ofctl -O OpenFlow13 packet-out br-underlay "in_port=1 
packet=02908ca8a149faadfa25056008004500010a9e9d4000402f4084ac1f0101ac1f01646558e666c122e666c111080045e46f8e40004011b4760a0101010a010102e026162e00d016e6a366ebf904c74132c6fed42a9e9e46240b4d9fd13c9b47d9704a388e70a5e77db16934a6188dc01d86aa20007ace2cf9cdb111f208474b88ffc851c871f0e3fb4fff138c1d288d437efff487e2b86a9c99fbf4229a6485e133bcf3e16f6e345207fda0932d9eeb602740456fd077b4847d25481337bd716155cc245be129ccc11bf82b834767b3760b52fe913c0e24f31c0e1b27f88acf7bba6b985fb64ee2cd6fc6bba1a9c1f021e253e1728b046fd4d023307e3296361a37ea2617ebcb2537e0284a81050dd0ee
 actions=LOCAL"
 
 dnl After truncation = 100 byte at loopback device p2(4)
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 AT_CHECK([ovs-ofctl dump-flows br0 | grep

[ovs-dev] [PATCH v6 02/15] tests: Include working system-traffic tests into the system-offloads-testsuite.

2022-12-13 Thread Eelco Chaudron

Include and run the system-traffic.at tests as part of the system offload
testsuite. Exclude all the tests that will not run without any special
modifications.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/automake.mk  |1 
 tests/system-offloads-testsuite.at |1 
 tests/system-offloads.at   |  106 
 3 files changed, 108 insertions(+)
 create mode 100644 tests/system-offloads.at

diff --git a/tests/automake.mk b/tests/automake.mk
index d509cf935..12435d2c1 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -183,6 +183,7 @@ SYSTEM_TESTSUITE_AT = \
 
 SYSTEM_OFFLOADS_TESTSUITE_AT = \
tests/system-common-macros.at \
+   tests/system-offloads.at \
tests/system-offloads-traffic.at \
tests/system-offloads-testsuite.at
 
diff --git a/tests/system-offloads-testsuite.at 
b/tests/system-offloads-testsuite.at
index eb5d2d4b3..a2dfcbc94 100644
--- a/tests/system-offloads-testsuite.at
+++ b/tests/system-offloads-testsuite.at
@@ -23,3 +23,4 @@ m4_include([tests/system-common-macros.at])
 m4_include([tests/system-kmod-macros.at])
 
 m4_include([tests/system-offloads-traffic.at])
+m4_include([tests/system-offloads.at])
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
new file mode 100644
index 0..f6dd931b7
--- /dev/null
+++ b/tests/system-offloads.at
@@ -0,0 +1,106 @@
+AT_COPYRIGHT([Copyright (c) 2022 Red Hat, Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at:
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.])
+
+# The goal is to run as many as possible of the system-traffic tests with
+# OVS tc offload enabled. We do this by overriding the
+# OVS_TRAFFIC_VSWITCHD_START() with offloading enabled.
+m4_define([OVS_TRAFFIC_VSWITCHD_START],
+  [AT_CHECK([modprobe openvswitch])
+   on_exit 'modprobe -r openvswitch'
+   m4_foreach([mod], [[vport_geneve], [vport_gre], [vport_lisp], [vport_stt], 
[vport_vxlan]],
+  [modprobe -q mod || echo "Module mod not loaded."
+   on_exit 'modprobe -q -r mod'
+  ])
+   on_exit 'ovs-dpctl del-dp ovs-system'
+   on_exit 'ovs-appctl dpctl/flush-conntrack'
+   _OVS_VSWITCHD_START([], [-- set Open_vSwitch . other_config:hw-offload=true
+   $3])
+   dnl Add bridges, ports, etc.
+   AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| 
uuidfilt])], [0], [$2])
+])
+
+# The list below are tests that will not pass for a "test environment" specific
+# issue.
+m4_define([OVS_TEST_SKIP_LIST],
+[ovs_test_skip_list="
+datapath - mpls actions
+datapath - multiple mpls label pop
+datapath - basic truncate action
+datapath - truncate and output to gre tunnel by simulated packets
+datapath - truncate and output to gre tunnel
+conntrack - force commit
+conntrack - preserve registers
+conntrack - zones
+conntrack - zones from field
+conntrack - zones from other field
+conntrack - zones from other field, more tests
+conntrack - multiple zones
+conntrack - multiple namespaces, internal ports
+conntrack - ct_mark
+conntrack - ct_mark bit-fiddling
+conntrack - ct_mark from register
+conntrack - ct_label
+conntrack - ct_label bit-fiddling
+conntrack - ct metadata, multiple zones
+conntrack - ICMP related
+conntrack - ICMP related to original direction
+conntrack - IPv4 fragmentation + cvlan
+conntrack - IPv4 fragmentation with fragments specified
+conntrack - IPv6 fragmentation + cvlan
+conntrack - Fragmentation over vxlan
+conntrack - IPv6 Fragmentation over vxlan
+conntrack - zone-based timeout policy
+conntrack - IPv4 HTTP
+conntrack - IPv6 HTTP
+conntrack - multiple zones, local
+conntrack - multi-stage pipeline, local
+conntrack - FTP
+conntrack - FTP over IPv6
+conntrack - IPv6 FTP Passive
+conntrack - FTP with multiple expectations
+conntrack - TFTP
+conntrack - simple SNAT
+conntrack - SNAT with port range
+conntrack - SNAT with port range with exhaustion
+conntrack - more complex SNAT
+conntrack - all-zero IP SNAT
+conntrack - simple DNAT
+conntrack - DNAT with additional SNAT
+conntrack - more complex DNAT
+conntrack - ICMP related with NAT
+conntrack - FTP SNAT prerecirc
+conntrack - FTP SNAT prerecirc seqadj
+conntrack - FTP SNAT postrecirc
+conntrack - FTP SNAT postrecirc seqadj
+conntrack - FTP SNAT orig tuple
+conntrack - FTP SNAT orig tuple seqadj
+conntrack - IPv4 FTP Passive with SNAT
+conntrack - IPv4 FTP Passive with DNAT
+conntrack - IPv4 FTP Passive with DNAT 2
+conntrack - IPv4 FTP Active with DNAT
+conntrack - IPv4 FTP Active with DNAT with reverse skew
+conntrack -

[ovs-dev] [PATCH v6 00/15] tests: Add system-traffic.at tests to check-offloads.

2022-12-13 Thread Eelco Chaudron

This series makes it possible to include system-traffic.at tests into
"make check-offloads" tests.

The last patch of the series explains which tests are still not passing
and might need some more work.

I'll try to work on the remaining failing test cases or find someone
who can work on them.

v6:
  - Added ACKs from v5
  - Changed 'netdev-offload-tc: If the flow has not been used, report
it as such.' to also work on hardware offloaded flows.

v5:
  - Include all patches, v4 went out with missing two patches :(

v4:
  - Fix rename from system-traffic.at to sym-traffic.at in patch 11

v3:
  - Fixed missing MACRO's in patches 4, 6 and 10.

v2:
  - Fix commit message on last patch
  - Moved handling of system-traffic.at tests to a separate file
system-offloads.at
  - Re-based to the latest ovs master branch
  - Added Roi's ACKs

Eelco Chaudron (15):
  tests: Allow system-traffic tests to be skipped based on a list.
  tests: Include working system-traffic tests into the 
system-offloads-testsuite.
  test: Do not use MPLS implicit null label in test cases.
  test: Add delay on revalidator flush for offload test cases.
  netdev-offload-tc: Fix tc conntrack force commit support.
  tests: Add delay to dump-conntrack for tc test cases.
  test: Fix "conntrack - floating IP" test for TC.
  test: Flush datapath when changing rules on the fly.
  netdev-offload-tc: Conntrack ALGs are not supported with tc.
  test: tc does not support conntrack timeout, skip the related test.
  test: Fix 'conntrack - Multiple ICMP traverse' for tc case.
  odp-util: Make odp_flow_key_from_flow__ nlattr order the same as the 
kernel.
  netdev-offload-tc: If the flow has not been used, report it as such.
  tests: Fix reading of OpenFlow byte counters in GRE test cases.
  tests: Comment currently failing TC system-traffic tests.


 Documentation/howto/tc-offload.rst |  11 ++
 lib/netdev-offload-tc.c|  17 +-
 lib/odp-util.c |  21 ++-
 lib/tc.c   |  14 +-
 tests/automake.mk  |   1 +
 tests/dpif-netdev.at   |  28 +--
 tests/mcast-snooping.at|   4 +-
 tests/nsh.at   |  10 +-
 tests/odp.at   |  83 -
 tests/ofproto-dpif.at  |  30 +--
 tests/ofproto-macros.at|   5 +-
 tests/ovs-macros.at|   7 +
 tests/packet-type-aware.at |  22 +--
 tests/pmd.at   |   2 +-
 tests/system-common-macros.at  |   7 +
 tests/system-offloads-testsuite.at |   1 +
 tests/system-offloads.at   | 111 
 tests/system-traffic.at| 282 +++--
 tests/tunnel-push-pop-ipv6.at  |   2 +-
 tests/tunnel-push-pop.at   |   2 +-
 tests/tunnel.at|   2 +-
 21 files changed, 415 insertions(+), 247 deletions(-)
 create mode 100644 tests/system-offloads.at

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

[ovs-dev] [PATCH v6 01/15] tests: Allow system-traffic tests to be skipped based on a list.

2022-12-13 Thread Eelco Chaudron

When the test description is part of the OVS_TEST_SKIP_LIST
variable, the test is skipped.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/ofproto-macros.at |5 -
 tests/ovs-macros.at |7 +++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
index 676d55aa9..5c033f771 100644
--- a/tests/ofproto-macros.at
+++ b/tests/ofproto-macros.at
@@ -161,7 +161,10 @@ m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m'])
 # before starting ovs-vswitchd.
 #
 m4_define([_OVS_VSWITCHD_START],
-  [dnl Create database.
+  [dnl Check if test needs to be run.
+   OVS_SKIP_TEST_IF_REQUESTED()
+
+   dnl Create database.
touch .conf.db.~lock~
AT_CHECK([ovsdb-tool create conf.db 
$abs_top_srcdir/vswitchd/vswitch.ovsschema])
 
diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at
index 39fbfceeb..f3eff5c05 100644
--- a/tests/ovs-macros.at
+++ b/tests/ovs-macros.at
@@ -371,3 +371,10 @@ dnl Add a rule to always accept the traffic.
 m4_define([IPTABLES_ACCEPT],
   [AT_CHECK([iptables -I INPUT 1 -i $1 -j ACCEPT])
on_exit 'iptables -D INPUT 1 -i $1'])
+
+# OVS_TEST_SKIP_LIST()
+m4_define([OVS_TEST_SKIP_LIST], [ echo ""])
+
+# OVS_SKIP_TEST_IF_REQUESTED()
+m4_define([OVS_SKIP_TEST_IF_REQUESTED],
+[AT_SKIP_IF([OVS_TEST_SKIP_LIST() | grep -qx "$at_desc"])])

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

Re: [ovs-dev] [PATCH v5 13/15] netdev-offload-tc: If the flow has not been used, report it as such.

2022-12-13 Thread Eelco Chaudron




On 13 Dec 2022, at 16:03, Roi Dayan wrote:

> On 13/12/2022 16:27, Eelco Chaudron wrote:
>>
>>
>> On 12 Dec 2022, at 12:36, Roi Dayan wrote:
>>
>>> On 12/12/2022 10:06, Roi Dayan wrote:


 On 23/11/2022 13:19, Eelco Chaudron wrote:
> If a tc flow was installed but has not yet been used, report it as such.
>
> In addition, add a delay to the "IGMP - flood under normal action" test
> case to make it work with many repetitions. This delay is also present
> in other ICMP/IGMP tests.
>
> Signed-off-by: Eelco Chaudron 
> Acked-by: Roi Dayan 
> ---
>  lib/tc.c |8 +++-
>  tests/system-offloads.at |3 +--
>  tests/system-traffic.at  |2 ++
>  3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/lib/tc.c b/lib/tc.c
> index a66dc432f..934df2e5e 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -1361,7 +1361,13 @@ get_user_hz(void)
>  static void
>  nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower)
>  {
> -uint64_t lastused = time_msec() - (tm->lastuse * 1000 / 
> get_user_hz());
> +uint64_t lastused;
> +
> +if (tm->firstuse == 0) {
> +lastused = 0;
> +} else {
> +lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
> +}
>
>  if (flower->lastused < lastused) {
>  flower->lastused = lastused;
> diff --git a/tests/system-offloads.at b/tests/system-offloads.at
> index 102e89a1f..9db68b2a0 100644
> --- a/tests/system-offloads.at
> +++ b/tests/system-offloads.at
> @@ -76,8 +76,7 @@ conntrack - multiple zones, local
>  conntrack - multi-stage pipeline, local
>  conntrack - ICMP related with NAT
>  conntrack - DNAT load balancing
> -conntrack - DNAT load balancing with NC
> -IGMP - flood under normal action"
> +conntrack - DNAT load balancing with NC"
>  echo "$ovs_test_skip_list" | sed "s// /g"])
>
>  m4_include([tests/system-traffic.at])
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 07913a192..57ff83b51 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -7002,6 +7002,8 @@ f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 
> 02 d3 49 45 65 eb 4a e0 dnl
>  00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 
> 00 dnl
>  00 00 00 00 > /dev/null])
>
> +sleep 1
> +
>  AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep -e .*ipv4 | sort | 
> dnl
>strip_stats | strip_used | strip_recirc | dnl
>sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'],
>


 Hi Eelco,

 I missed but we found an issue with this commit.
 hw offloaded rules dont get their firtuse attr being updated.
 This cause tc hw offloaded rules to always report as used:never.
 We added a specific check for this to catch this.

 This looks like a kernel bug that .stats_update op in tc actions
 should have a wrapper like tcf_lastuse_update() in .act op to
 update firstuse if 0.
>>>
>>> or maybe to update the call to the offload driver to do it as the sw
>>> can't tell and doing it in .stats_update won't be real. it will be
>>> the first time dump stats was done.
>>
>> Hi Roi,
>>
>> Thanks for noticing this!
>>
>> I looked a bit at the kernel code and it looks like lastused is not 
>> initialized as 0, which is the main problem for OVS. However, I think the 
>> following modification to the patch should solve the problem:
>>
>>
>> -if (tm->firstuse == 0) {
>> +/* On creation both tm->install and tm->lastuse are set to jiffies
>> + * by the kernel. So if both values are the same, the flow has not been
>> + * used yet.
>> + *
>> + * Note that tm->firstuse can not be used due to some kernel bug, i.e.,
>> + * hardware offloaded flows do not update tm->firstuse. */
>> +if (tm->lastuse == tm->install) {
>>  lastused = 0;
>>  } else {
>>  lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
>>
>> Can you maybe test this with your HW, as I do not have a setup ready?
>>
>> Thanks,
>>
>> Eelco
>>
>>
>
> Hi,
>
> yes this is fine. I ran a checkup again with the original patch and
> saw the used:never issue. after this fixup it's ok again.
> so looks ok.
>
> Thanks,
> Roi

Ok, will send out a v6.

//Eelco

>>
 Will you send a fix for the kernel?

 Since we don't want to break working with current kernels I think
 we should avoid this patch for now and/or add a check to do this
 with newer kernels only.

 Thanks,
 Roi
>>

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

Re: [ovs-dev] [PATCH ovn 00/12] Fixes to multiple Unit and System Tests

2022-12-13 Thread Xavier Simonart

Hi Mark, Ales

Thanks for your review and feedback.
I'll send v2 for patch 4 and 7, and comments on patch 10.

Thanks
Xavier

On Mon, Dec 12, 2022 at 10:33 PM Mark Michelson  wrote:

> Hi Xavier. I did a review of this series.
>
> For patches 1, 2, 3, 5, 6, 8, 9, 11, and 12:
>
> Acked-by: Mark Michelson 
>
> For patches 4, 7, and 10 I have some comments.
>
> On 12/2/22 08:51, Xavier Simonart wrote:
> > Xavier Simonart (12):
> >tests: Fixed flaky system tests "ACL reject" and "ACL after lb -
> >  reject"
> >tests: Fixed flaky system-test ACL log_related
> >tests: Fixed system-test "load balancing affinity sessions"
> >tests: Fixed load balancing system-tests
> >tests: Fixed typo in macros
> >tests: Fixed flaky system-test "ECMP symmetric reply"
> >tests: Fixed flaky ACL fair Meters
> >tests: Fixed typo in "incremetal processing" test
> >tests: Removed macro reset_iface_pcap_file
> >tests: Fixed some tests failing on (very) slow systems
> >tests: Fixed "vtep: 3 HVs, 1 VIFs/HV, 1 GW, 1 LS"
> >tests: Fixed "IPv6 periodic RA" and "snat-ct-zone with common NAT
> >  zone"
> >
> >   tests/atlocal.in  |   3 +
> >   tests/network-functions.at|  18 +-
> >   tests/ovn-macros.at   |  21 --
> >   tests/ovn-northd.at   |   7 +-
> >   tests/ovn-performance.at  |   2 +-
> >   tests/ovn.at  |  54 ++--
> >   tests/system-common-macros.at |   2 +-
> >   tests/system-kmod-macros.at   |   1 -
> >   tests/system-ovn.at   | 456 +++---
> >   9 files changed, 317 insertions(+), 247 deletions(-)
> >
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5 13/15] netdev-offload-tc: If the flow has not been used, report it as such.

2022-12-13 Thread Roi Dayan via dev




On 13/12/2022 16:27, Eelco Chaudron wrote:
> 
> 
> On 12 Dec 2022, at 12:36, Roi Dayan wrote:
> 
>> On 12/12/2022 10:06, Roi Dayan wrote:
>>>
>>>
>>> On 23/11/2022 13:19, Eelco Chaudron wrote:
 If a tc flow was installed but has not yet been used, report it as such.

 In addition, add a delay to the "IGMP - flood under normal action" test
 case to make it work with many repetitions. This delay is also present
 in other ICMP/IGMP tests.

 Signed-off-by: Eelco Chaudron 
 Acked-by: Roi Dayan 
 ---
  lib/tc.c |8 +++-
  tests/system-offloads.at |3 +--
  tests/system-traffic.at  |2 ++
  3 files changed, 10 insertions(+), 3 deletions(-)

 diff --git a/lib/tc.c b/lib/tc.c
 index a66dc432f..934df2e5e 100644
 --- a/lib/tc.c
 +++ b/lib/tc.c
 @@ -1361,7 +1361,13 @@ get_user_hz(void)
  static void
  nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower)
  {
 -uint64_t lastused = time_msec() - (tm->lastuse * 1000 / 
 get_user_hz());
 +uint64_t lastused;
 +
 +if (tm->firstuse == 0) {
 +lastused = 0;
 +} else {
 +lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
 +}

  if (flower->lastused < lastused) {
  flower->lastused = lastused;
 diff --git a/tests/system-offloads.at b/tests/system-offloads.at
 index 102e89a1f..9db68b2a0 100644
 --- a/tests/system-offloads.at
 +++ b/tests/system-offloads.at
 @@ -76,8 +76,7 @@ conntrack - multiple zones, local
  conntrack - multi-stage pipeline, local
  conntrack - ICMP related with NAT
  conntrack - DNAT load balancing
 -conntrack - DNAT load balancing with NC
 -IGMP - flood under normal action"
 +conntrack - DNAT load balancing with NC"
  echo "$ovs_test_skip_list" | sed "s// /g"])

  m4_include([tests/system-traffic.at])
 diff --git a/tests/system-traffic.at b/tests/system-traffic.at
 index 07913a192..57ff83b51 100644
 --- a/tests/system-traffic.at
 +++ b/tests/system-traffic.at
 @@ -7002,6 +7002,8 @@ f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 
 02 d3 49 45 65 eb 4a e0 dnl
  00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 
 00 dnl
  00 00 00 00 > /dev/null])

 +sleep 1
 +
  AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep -e .*ipv4 | sort | 
 dnl
strip_stats | strip_used | strip_recirc | dnl
sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'],

>>>
>>>
>>> Hi Eelco,
>>>
>>> I missed but we found an issue with this commit.
>>> hw offloaded rules dont get their firtuse attr being updated.
>>> This cause tc hw offloaded rules to always report as used:never.
>>> We added a specific check for this to catch this.
>>>
>>> This looks like a kernel bug that .stats_update op in tc actions
>>> should have a wrapper like tcf_lastuse_update() in .act op to
>>> update firstuse if 0.
>>
>> or maybe to update the call to the offload driver to do it as the sw
>> can't tell and doing it in .stats_update won't be real. it will be
>> the first time dump stats was done.
> 
> Hi Roi,
> 
> Thanks for noticing this!
> 
> I looked a bit at the kernel code and it looks like lastused is not 
> initialized as 0, which is the main problem for OVS. However, I think the 
> following modification to the patch should solve the problem:
> 
> 
> -if (tm->firstuse == 0) {
> +/* On creation both tm->install and tm->lastuse are set to jiffies
> + * by the kernel. So if both values are the same, the flow has not been
> + * used yet.
> + *
> + * Note that tm->firstuse can not be used due to some kernel bug, i.e.,
> + * hardware offloaded flows do not update tm->firstuse. */
> +if (tm->lastuse == tm->install) {
>  lastused = 0;
>  } else {
>  lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
> 
> Can you maybe test this with your HW, as I do not have a setup ready?
> 
> Thanks,
> 
> Eelco
> 
> 

Hi,

yes this is fine. I ran a checkup again with the original patch and
saw the used:never issue. after this fixup it's ok again.
so looks ok.

Thanks,
Roi


> 
>>> Will you send a fix for the kernel?
>>>
>>> Since we don't want to break working with current kernels I think
>>> we should avoid this patch for now and/or add a check to do this
>>> with newer kernels only.
>>>
>>> Thanks,
>>> Roi
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] travis: Fix remaining trace of kernel build tests.

2022-12-13 Thread David Marchand

On Tue, Dec 13, 2022 at 2:34 PM Ilya Maximets  wrote:
>
> On 12/13/22 14:29, Aaron Conole wrote:
> > David Marchand  writes:
> >
> >> On Mon, Dec 12, 2022 at 5:14 PM Ilya Maximets  wrote:
> >>>
> >>> On 12/12/22 16:58, David Marchand wrote:
>  We stopped building OVS kernel module in the CI but forgot to update the
>  (unused?) Travis configuration.
> >>>
> >>> I'd say, we should just remove the yml.  Nobody seems to use it anyway.
> >>> What do you think?
> >>
> >> I don't mind dropping it, but just in case, I Cc'ed some ARM people.
> >
> > I am certainly in favor of dropping it.  The credits system they
> > implemented has made it unusable for most projects without some real $$
> > being spent.
>
> +1
>
> For the ARM CI, we should implement new ARM jobs in Cirrus CI.

Ok, I marked this patch as rejected and submitted a patch to drop Travis config.


-- 
David Marchand

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

[ovs-dev] [PATCH] travis: Drop support.

2022-12-13 Thread David Marchand

Following a change in the terms of use, free Travis credits are really too
low for a realistic usage by OVS contributors.
As a consequence, testing OVS with Travis has been abandonned by most
(if not all) contributors to the project.

Drop the Travis configuration from our repository and clean references in
the documentation.

Signed-off-by: David Marchand 
---
 .ci/linux-build.sh| 31 +-
 .ci/linux-prepare.sh  |  9 +--
 .travis.yml   | 57 ---
 .../contributing/submitting-patches.rst   |  7 +--
 Documentation/topics/testing.rst  | 40 -
 Makefile.am   |  1 -
 NEWS  |  2 +
 README.rst|  2 -
 8 files changed, 10 insertions(+), 139 deletions(-)
 delete mode 100644 .travis.yml

diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 4851096723..c06186ce1c 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -7,21 +7,6 @@ CFLAGS_FOR_OVS="-g -O2"
 SPARSE_FLAGS=""
 EXTRA_OPTS="--enable-Werror"
 
-on_exit() {
-if [ $? = 0 ]; then
-exit
-fi
-FILES_TO_PRINT="config.log"
-FILES_TO_PRINT="$FILES_TO_PRINT */_build/sub/tests/testsuite.log"
-
-for pr_file in $FILES_TO_PRINT; do
-cat "$pr_file" 2>/dev/null
-done
-}
-# We capture the error logs as artifacts in Github Actions, no need to dump
-# them via a EXIT handler.
-[ -n "$GITHUB_WORKFLOW" ] || trap on_exit EXIT
-
 function install_kernel()
 {
 if [[ "$1" =~ ^5.* ]]; then
@@ -98,19 +83,9 @@ function install_kernel()
 function install_dpdk()
 {
 local DPDK_VER=$1
-local VERSION_FILE="dpdk-dir/travis-dpdk-cache-version"
+local VERSION_FILE="dpdk-dir/cached-version"
 local DPDK_OPTS=""
-local DPDK_LIB=""
-
-if [ -z "$TRAVIS_ARCH" ] ||
-   [ "$TRAVIS_ARCH" == "amd64" ]; then
-DPDK_LIB=$(pwd)/dpdk-dir/build/lib/x86_64-linux-gnu
-elif [ "$TRAVIS_ARCH" == "aarch64" ]; then
-DPDK_LIB=$(pwd)/dpdk-dir/build/lib/aarch64-linux-gnu
-else
-echo "Target is unknown"
-exit 1
-fi
+local DPDK_LIB=$(pwd)/dpdk-dir/build/lib/x86_64-linux-gnu
 
 if [ "$DPDK_SHARED" ]; then
 EXTRA_OPTS="$EXTRA_OPTS --with-dpdk=shared"
@@ -245,7 +220,7 @@ elif [ "$M32" ]; then
 # Adding m32 flag directly to CC to avoid any posiible issues with API/ABI
 # difference on 'configure' and 'make' stages.
 export CC="$CC -m32"
-elif [ "$TRAVIS_ARCH" != "aarch64" ]; then
+else
 OPTS="--enable-sparse"
 if [ "$AFXDP" ]; then
 # netdev-afxdp uses memset for 64M for umem initialization.
diff --git a/.ci/linux-prepare.sh b/.ci/linux-prepare.sh
index 11d75a6d59..4bfa561f51 100755
--- a/.ci/linux-prepare.sh
+++ b/.ci/linux-prepare.sh
@@ -10,14 +10,11 @@ fi
 
 # Build and install sparse.
 #
-# Explicitly disable sparse support for llvm because some travis
-# environments claim to have LLVM (llvm-config exists and works) but
-# linking against it fails.
 # Disabling sqlite support because sindex build fails and we don't
 # really need this utility being installed.
 git clone git://git.kernel.org/pub/scm/devel/sparse/sparse.git
 cd sparse
-make -j4 HAVE_LLVM= HAVE_SQLITE= install
+make -j4 HAVE_SQLITE= install
 cd ..
 
 # Installing wheel separately because it may be needed to build some
@@ -45,7 +42,5 @@ fi
 # Install python test dependencies
 pip3 install -r python/test_requirements.txt
 
-# IPv6 is supported by kernel but disabled in TravisCI images:
-#   https://github.com/travis-ci/travis-ci/issues/8891
-# Enable it to avoid skipping of IPv6 related tests.
+# Make sure IPv6 is enabled to avoid skipping of IPv6 related tests.
 sudo sysctl -w net.ipv6.conf.all.disable_ipv6=0
diff --git a/.travis.yml b/.travis.yml
deleted file mode 100644
index c7aeede06e..00
--- a/.travis.yml
+++ /dev/null
@@ -1,57 +0,0 @@
-language: c
-
-os:
-  - linux
-
-cache:
-  directories:
-- dpdk-dir
-
-addons:
-  apt:
-packages:
-  - bc
-  - libssl-dev
-  - llvm-dev
-  - libjemalloc1
-  - libjemalloc-dev
-  - libnuma-dev
-  - libpcap-dev
-  - python3-pip
-  - python3-sphinx
-  - libelf-dev
-  - selinux-policy-dev
-  - libunbound-dev
-  - libunwind-dev
-  - python3-setuptools
-  - python3-wheel
-  - ninja-build
-
-before_install: ./.ci/${TRAVIS_OS_NAME}-prepare.sh
-
-before_script: export PATH=$PATH:$HOME/bin
-
-matrix:
-  include:
-- arch: arm64
-  compiler: gcc
-  env: TESTSUITE=1 DPDK=1
-- arch: arm64
-  compiler: gcc
-  env: KERNEL_LIST="5.5 4.19"
-- arch: arm64
-  compiler: gcc
-  env: KERNEL_LIST="4.9 3.16"
-- arch: arm64
-  compiler: gcc
-  env: DPDK_SHARED=1
-- arch: arm64
-  compiler: clang
-  env: OPTS="--disable-ssl"
-
-script: ./.ci/${TRAVIS_OS_NAME}-build.sh $OPTS
-
-notifications:
-  email:
-

Re: [ovs-dev] [PATCH ovn 07/12] tests: Fixed flaky ACL fair Meters

2022-12-13 Thread Xavier Simonart

Hi Mark

On Mon, Dec 12, 2022 at 10:33 PM Mark Michelson  wrote:

> On 12/2/22 08:51, Xavier Simonart wrote:
> > Tests was (unluckily) failing as grepping 10.0.0.11 (and expecting an IP)
> > and catching a UUID such as 103090811468
> >
> > Signed-off-by: Xavier Simonart 
> > ---
> >   tests/ovn-northd.at | 7 +++
> >   1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 78edad9ae..47c761927 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -2225,14 +2225,13 @@ check ovn-nbctl acl-add sw0 to-lport 1002
> 'outport == "sw0-p1" && ip4.src == 10.
> >   check ovn-nbctl acl-add sw0 to-lport 1002 'outport == "sw0-p1" &&
> ip4.src == 10.0.0.13' allow
> >   check ovn-nbctl acl-add pg0 to-lport 1002 'outport == "pg0" && ip4.src
> == 10.0.0.11' drop
> >
> > -acl1=$(ovn-nbctl --bare --column _uuid,match find acl | grep -B1
> '10.0.0.12' | head -1)
> > -acl2=$(ovn-nbctl --bare --column _uuid,match find acl | grep -B1
> '10.0.0.13' | head -1)
> > -acl3=$(ovn-nbctl --bare --column _uuid,match find acl | grep -B1
> '10.0.0.11' | head -1)
> > +acl1=$(ovn-nbctl --bare --column _uuid,match find acl | grep -B1
> '10\.0.0.12' | head -1)
> > +acl2=$(ovn-nbctl --bare --column _uuid,match find acl | grep -B1
> '10\.0.0.13' | head -1)
> > +acl3=$(ovn-nbctl --bare --column _uuid,match find acl | grep -B1
> '10\.0.0.11' | head -1)
>
> In a previous change where you needed to match on exact strings, you
> used `grep -F` . Why not do the same thing here?
>
> No good reason. I'll send a v2 with grep -F
Thanks
Xavier

> >   check ovn-nbctl set acl $acl1 log=true severity=alert meter=meter_me
> name=acl_one
> >   check ovn-nbctl set acl $acl2 log=true severity=info  meter=meter_me
> name=acl_two
> >   check ovn-nbctl set acl $acl3 log=true severity=info  meter=meter_me
> name=acl_three
> >   check ovn-nbctl --wait=sb sync
> > -
> >   check_row_count nb:meter 1
> >   check_column meter_me nb:meter name
> >
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn 04/12] tests: Fixed load balancing system-tests

2022-12-13 Thread Xavier Simonart

Hi Mark

On Mon, Dec 12, 2022 at 10:33 PM Mark Michelson  wrote:

> On 12/2/22 08:51, Xavier Simonart wrote:
> > Most load balancing system tests were randomly failing from time to time
> > as they were checking that, after 20 requests sent to load balancer, all
> > backends were at least reached once.
> > Statistically, this was failing from time to time.
> > Now, if after 10 requests we did not get the expected distribution, we
> > send 10 more requests. We do that around 30 times
> >
> > Signed-off-by: Xavier Simonart 
> > ---
> >   tests/system-ovn.at | 343 +---
> >   1 file changed, 196 insertions(+), 147 deletions(-)
> >
> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > index 7c56e8ef0..dbac646fc 100644
> > --- a/tests/system-ovn.at
> > +++ b/tests/system-ovn.at
> > @@ -1734,46 +1734,53 @@ OVS_START_L7([bar2], [http6])
> >   OVS_START_L7([bar3], [http6])
> >
> >   dnl Should work with the virtual IP fd03::1 address through NAT
> > -for i in `seq 1 20`; do
> > -echo Request $i
> > -NS_CHECK_EXEC([foo1], [wget http://[[fd03::1]] -t 5 -T 1
> --retry-connrefused -v -o wget$i.log || (ovs-ofctl -O OpenFlow13 dump-flows
> br-int && false)])
> > +j=0
> > +OVS_WAIT_FOR_OUTPUT([
> > +for i in `seq 1 10`; do
> > +NS_EXEC([foo1], [wget http://[[fd03::1]] -t 5 -T 1
> --retry-connrefused -v -o wget$i.log || (ovs-ofctl -O OpenFlow13 dump-flows
> br-int && false)])
> >   done
> > -
> > -dnl Each server should have at least one connection.
> > -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fd03::1) | grep
> -v fe80 | \
> > +j=$((j + 1))
> > +ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fd03::1) | grep -v fe80 | \
> >   sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl
> >
>  
> tcp,orig=(src=fd01::2,dst=fd03::1,sport=,dport=),reply=(src=fd02::2,dst=fd01::2,sport=,dport=),zone=,mark=2,protoinfo=(state=)
> >
>  
> tcp,orig=(src=fd01::2,dst=fd03::1,sport=,dport=),reply=(src=fd02::3,dst=fd01::2,sport=,dport=),zone=,mark=2,protoinfo=(state=)
> >
>  
> tcp,orig=(src=fd01::2,dst=fd03::1,sport=,dport=),reply=(src=fd02::4,dst=fd01::2,sport=,dport=),zone=,mark=2,protoinfo=(state=)
> >   ])
> >
> > +echo "Succeeded after $j iterations"
>
> I'm curious if these "succeeded after $j iterations" messages are
> helpful. First, there is no context for the messaages, so it's not clear
> what is succeeding. Second, since the message appears several times
> throughout the tests, it can cause confusion about which one is being
> printed, potentially. Third, since each "iteration" may consist of 10
> wgets, the definition of an "iteration" is a bit muddy.
>
> Honestly, I think these echos should be removed. And if those are
> removed, then j has no reason to exist, so the lines that set and
> increment j should be removed too.
>
> Agreed.
I'll send a v2 removing this debug info
Thanks
Xavier

> > +
> >   dnl Should work with the virtual IP fd03::3 address through NAT
> > -for i in `seq 1 20`; do
> > -echo Request $i
> > -NS_CHECK_EXEC([foo1], [wget http://[[fd03::3]] -t 5 -T 1
> --retry-connrefused -v -o wget$i.log])
> > +j=0
> > +OVS_WAIT_FOR_OUTPUT([
> > +for i in `seq 1 10`; do
> > +NS_EXEC([foo1], [wget http://[[fd03::3]] -t 5 -T 1
> --retry-connrefused -v -o wget$i.log])
> >   done
> > -
> > +j=$((j + 1))
> >   dnl Each server should have at least one connection.
> > -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fd03::3) | grep
> -v fe80 | \
> > +ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fd03::3) | grep -v fe80 | \
> >   sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl
> >
>  
> tcp,orig=(src=fd01::2,dst=fd03::3,sport=,dport=),reply=(src=fd02::2,dst=fd01::2,sport=,dport=),zone=,mark=2,protoinfo=(state=)
> >
>  
> tcp,orig=(src=fd01::2,dst=fd03::3,sport=,dport=),reply=(src=fd02::3,dst=fd01::2,sport=,dport=),zone=,mark=2,protoinfo=(state=)
> >
>  
> tcp,orig=(src=fd01::2,dst=fd03::3,sport=,dport=),reply=(src=fd02::4,dst=fd01::2,sport=,dport=),zone=,mark=2,protoinfo=(state=)
> >   ])
> >
> > +echo "Succeeded after $j iterations"
> > +
> > +j=0
> > +OVS_WAIT_FOR_OUTPUT([
> >   dnl Test load-balancing that includes L4 ports in NAT.
> > -for i in `seq 1 20`; do
> > -echo Request $i
> > -NS_CHECK_EXEC([foo1], [wget http://[[fd03::2]]:8000 -t 5 -T 1
> --retry-connrefused -v -o wget$i.log])
> > +for i in `seq 1 10`; do
> > +NS_EXEC([foo1], [wget http://[[fd03::2]]:8000 -t 5 -T 1
> --retry-connrefused -v -o wget$i.log])
> >   done
> > -
> > +j=$((j + 1))
> >   dnl Each server should have at least one connection.
> > -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fd03::2) | grep
> -v fe80 | \
> > +ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fd03::2) | grep -v fe80 | \
> >   sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl
> >
>  
> tcp,orig=(src=fd01::2,dst=fd03::2,sport=,dport=),reply=(src=fd02::2,dst=fd01::2,sport=,dport=),zone=,mark=2,protoinfo=(state=)
> >
>  
>

Re: [ovs-dev] [PATCH ovn 2/2] tests: Check the default drop directly from flows dump

2022-12-13 Thread Xavier Simonart

On Tue, Dec 13, 2022 at 3:01 PM Ales Musil  wrote:

>
>
> On Tue, Dec 13, 2022 at 2:55 PM Xavier Simonart 
> wrote:
>
>> Hi Ales
>>
>> Thanks for the patch and the nice speed-up
>>
>>
>> On Mon, Dec 12, 2022 at 4:57 PM Ales Musil  wrote:
>>
>>> Instead of iterating over the logical flows in a loop
>>> just check the flows directly filtering the portion the
>>> tests is interested in. Reducing the duration ~17x.
>>>
>>> Reported-at: https://bugzilla.redhat.com/2149851
>>> Signed-off-by: Ales Musil 
>>> ---
>>>  tests/ovn-northd.at | 115 
>>>  1 file changed, 95 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>>> index 71eda9ff4..554a9b63b 100644
>>> --- a/tests/ovn-northd.at
>>> +++ b/tests/ovn-northd.at
>>> @@ -8193,20 +8193,6 @@ OVN_FOR_EACH_NORTHD([
>>>  AT_SETUP([Check default drop])
>>>  AT_KEYWORDS([drop])
>>>
>>> -# Check that there is an explicit drop lflow in all tables.
>>> -check_default_lflow() {
>>> -dps=$(ovn-sbctl --bare  --columns=_uuid list Datapath_Binding |
>>> xargs)
>>> -for dp in $dps; do
>>> -for pipeline in ingress egress; do
>>> -for table in $(ovn-sbctl --bare --columns=table_id find
>>> Logical_Flow logical_datapath="$dp" pipeline="$pipeline" | xargs | sort |
>>> uniq); do
>>> -   echo "Checking if datapath $dp pipeline $pipeline table
>>> $table has a default action"
>>> -   AT_CHECK([ovn-sbctl --columns=_uuid find Logical_Flow
>>> logical_datapath="$dp" pipeline="$pipeline" table_id=$table match="1"
>>> priority">="0 | wc -l | tr -d "\n\r" ], [0], [1], [ignore],
>>> -   [echo "Datapath $dp pipeline $pipeline table $table does
>>> not have a default action"])
>>> -done
>>> -done
>>> -done
>>> -}
>>> -
>>>  ovn_start
>>>
>>>  # Create LS + LR
>>> @@ -8221,13 +8207,84 @@ check ovn-nbctl --wait=sb \
>>>  -- lsp-add S1 p1 \
>>>  -- lsp-set-addresses p1 "02:ac:10:01:00:0a 172.16.1.100"
>>>
>>> -check_default_lflow
>>> +AT_CHECK([ovn-sbctl dump-flows | grep "match=(1)"  | sed
>>> 's/table=../table=??/' | sort], [0], [dnl
>>> +  table=??(lr_in_admission), priority=0, match=(1),
>>> action=(drop;)
>>> +  table=??(lr_in_arp_request  ), priority=0, match=(1),
>>> action=(output;)
>>> +  table=??(lr_in_arp_resolve  ), priority=0, match=(1),
>>> action=(drop;)
>>> +  table=??(lr_in_chk_pkt_len  ), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_in_defrag   ), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_in_dnat ), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_in_ecmp_stateful), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_in_gw_redirect  ), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_in_ip_input ), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_in_ip_routing   ), priority=0, match=(1),
>>> action=(drop;)
>>> +  table=??(lr_in_ip_routing_ecmp), priority=0, match=(1),
>>> action=(drop;)
>>> +  table=??(lr_in_ip_routing_pre), priority=0, match=(1),
>>> action=(reg7 = 0; next;)
>>> +  table=??(lr_in_larger_pkts  ), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_in_lb_aff_check ), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_in_lb_aff_learn ), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_in_learn_neighbor), priority=0, match=(1),
>>> action=(drop;)
>>> +  table=??(lr_in_lookup_neighbor), priority=0, match=(1),
>>> action=(reg9[[2]] = 1; next;)
>>> +  table=??(lr_in_nd_ra_options), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_in_nd_ra_response), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_in_policy   ), priority=0, match=(1),
>>> action=(reg8[[0..15]] = 0; next;)
>>> +  table=??(lr_in_policy_ecmp  ), priority=0, match=(1),
>>> action=(drop;)
>>> +  table=??(lr_in_unsnat   ), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_out_chk_dnat_local), priority=0, match=(1),
>>> action=(reg9[[4]] = 0; next;)
>>> +  table=??(lr_out_delivery), priority=0, match=(1),
>>> action=(drop;)
>>> +  table=??(lr_out_egr_loop), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_out_post_snat   ), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_out_post_undnat ), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_out_snat), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(lr_out_undnat  ), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(ls_in_acl  ), priority=65535, match=(1),
>>> action=(next;)
>>> +  table=??(ls_in_acl_after_lb ), priority=0, match=(1),
>>> action=(next;)
>>> +  table=??(ls_in_acl_hint ), priority=65535, match=(1),
>>> action=(next;)
>>> +  table=??(ls_in_apply_port_sec), priority=0,

Re: [ovs-dev] [PATCH v5 13/15] netdev-offload-tc: If the flow has not been used, report it as such.

2022-12-13 Thread Eelco Chaudron




On 12 Dec 2022, at 12:36, Roi Dayan wrote:

> On 12/12/2022 10:06, Roi Dayan wrote:
>>
>>
>> On 23/11/2022 13:19, Eelco Chaudron wrote:
>>> If a tc flow was installed but has not yet been used, report it as such.
>>>
>>> In addition, add a delay to the "IGMP - flood under normal action" test
>>> case to make it work with many repetitions. This delay is also present
>>> in other ICMP/IGMP tests.
>>>
>>> Signed-off-by: Eelco Chaudron 
>>> Acked-by: Roi Dayan 
>>> ---
>>>  lib/tc.c |8 +++-
>>>  tests/system-offloads.at |3 +--
>>>  tests/system-traffic.at  |2 ++
>>>  3 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/lib/tc.c b/lib/tc.c
>>> index a66dc432f..934df2e5e 100644
>>> --- a/lib/tc.c
>>> +++ b/lib/tc.c
>>> @@ -1361,7 +1361,13 @@ get_user_hz(void)
>>>  static void
>>>  nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower)
>>>  {
>>> -uint64_t lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
>>> +uint64_t lastused;
>>> +
>>> +if (tm->firstuse == 0) {
>>> +lastused = 0;
>>> +} else {
>>> +lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
>>> +}
>>>
>>>  if (flower->lastused < lastused) {
>>>  flower->lastused = lastused;
>>> diff --git a/tests/system-offloads.at b/tests/system-offloads.at
>>> index 102e89a1f..9db68b2a0 100644
>>> --- a/tests/system-offloads.at
>>> +++ b/tests/system-offloads.at
>>> @@ -76,8 +76,7 @@ conntrack - multiple zones, local
>>>  conntrack - multi-stage pipeline, local
>>>  conntrack - ICMP related with NAT
>>>  conntrack - DNAT load balancing
>>> -conntrack - DNAT load balancing with NC
>>> -IGMP - flood under normal action"
>>> +conntrack - DNAT load balancing with NC"
>>>  echo "$ovs_test_skip_list" | sed "s// /g"])
>>>
>>>  m4_include([tests/system-traffic.at])
>>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>>> index 07913a192..57ff83b51 100644
>>> --- a/tests/system-traffic.at
>>> +++ b/tests/system-traffic.at
>>> @@ -7002,6 +7002,8 @@ f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 
>>> d3 49 45 65 eb 4a e0 dnl
>>>  00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 
>>> dnl
>>>  00 00 00 00 > /dev/null])
>>>
>>> +sleep 1
>>> +
>>>  AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep -e .*ipv4 | sort | dnl
>>>strip_stats | strip_used | strip_recirc | dnl
>>>sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'],
>>>
>>
>>
>> Hi Eelco,
>>
>> I missed but we found an issue with this commit.
>> hw offloaded rules dont get their firtuse attr being updated.
>> This cause tc hw offloaded rules to always report as used:never.
>> We added a specific check for this to catch this.
>>
>> This looks like a kernel bug that .stats_update op in tc actions
>> should have a wrapper like tcf_lastuse_update() in .act op to
>> update firstuse if 0.
>
> or maybe to update the call to the offload driver to do it as the sw
> can't tell and doing it in .stats_update won't be real. it will be
> the first time dump stats was done.

Hi Roi,

Thanks for noticing this!

I looked a bit at the kernel code and it looks like lastused is not initialized 
as 0, which is the main problem for OVS. However, I think the following 
modification to the patch should solve the problem:


-if (tm->firstuse == 0) {
+/* On creation both tm->install and tm->lastuse are set to jiffies
+ * by the kernel. So if both values are the same, the flow has not been
+ * used yet.
+ *
+ * Note that tm->firstuse can not be used due to some kernel bug, i.e.,
+ * hardware offloaded flows do not update tm->firstuse. */
+if (tm->lastuse == tm->install) {
 lastused = 0;
 } else {
 lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());

Can you maybe test this with your HW, as I do not have a setup ready?

Thanks,

Eelco



>> Will you send a fix for the kernel?
>>
>> Since we don't want to break working with current kernels I think
>> we should avoid this patch for now and/or add a check to do this
>> with newer kernels only.
>>
>> Thanks,
>> Roi

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

Re: [ovs-dev] [PATCH ovn 1/2] tests: Reduce duration of "northd-parallelization runtime" test

2022-12-13 Thread Xavier Simonart

Hi Ales

Thanks for the patch
Your new macro even fixes some bad parsing, so even better :-)

Looks good to me.

Thanks
Xavier

On Mon, Dec 12, 2022 at 4:57 PM Ales Musil  wrote:

> By running single chained ovn-nbctl command the test
> is ~5x faster.
>
> Reported-at: https://bugzilla.redhat.com/2149851
> Signed-off-by: Ales Musil 
> ---
>  tests/ovn-northd.at | 31 +--
>  1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index ca4263eac..71eda9ff4 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -7431,12 +7431,21 @@ ovn_start
>
>  add_switch_ports() {
>  for port in $(seq $1 $2); do
> -logical_switch_port=lsp${port}
> -check ovn-nbctl lsp-add ls1 $logical_switch_port
> -check ovn-nbctl lsp-set-addresses $logical_switch_port dynamic
> +OVN_NBCTL(lsp-add ls1 lsp${port})
> +OVN_NBCTL(lsp-set-addresses lsp${port} dynamic)
>  done
> +RUN_OVN_NBCTL()
>  }
>
> +delete_switch_ports() {
> +for port in $(seq $1 $2); do
> +OVN_NBCTL(lsp-del lsp${port})
> +done
> +RUN_OVN_NBCTL()
> +}
> +
> +m4_define([DUMP_FLOWS_SORTED], [sed -e 's/arp.tpa ==
> 10.1.0.[[0-9]]\{1,3\}/arp.tpa == 10.1.0.??/;s/eth.dst ==
> ..:..:..:..:..:../??:??:??:??:??:??/' | sort])
> +
>  # Build some rather heavy config and modify number of threads in the
> middle
>  check ovn-nbctl ls-add ls1
>  check ovn-nbctl set Logical_Switch ls1 other_config:subnet=10.1.0.0/16
> @@ -7464,27 +7473,21 @@ check ovn-nbctl --wait=sb sync
>  # Run 3 times: one with parallelization enabled, one with disabled, and
> one while changing
>  # Compare the flows produced by the three runs
>  # Ignore IP/MAC addresses
> -ovn-sbctl dump-flows | sed 's/arp.tpa == 10.1.0/arp.tpa ==
> 10.1.0.??/' | sed 's/eth.dst == ..:..:..:..:..:../??:??:??:??:??:??/' |sort
> > flows1
> +ovn-sbctl dump-flows | DUMP_FLOWS_SORTED > flows1
>
>  # Restart with 1 thread
> -for port in $(seq 1 250); do
> -logical_switch_port=lsp${port}
> -check ovn-nbctl lsp-del $logical_switch_port
> -done
> +delete_switch_ports 1 250
>  add_switch_ports 1 250
>  check ovn-nbctl --wait=sb sync
> -ovn-sbctl dump-flows | sed 's/arp.tpa == 10.1.0/arp.tpa ==
> 10.1.0.??/' | sed 's/eth.dst == ..:..:..:..:..:../??:??:??:??:??:??/' |sort
> > flows2
> +ovn-sbctl dump-flows | DUMP_FLOWS_SORTED > flows2
>  AT_CHECK([diff flows1 flows2])
>
>  # Restart with with 8 threads
>  check as northd ovn-appctl -t NORTHD_TYPE parallel-build/set-n-threads 8
> -for port in $(seq 1 250); do
> -logical_switch_port=lsp${port}
> -check ovn-nbctl lsp-del $logical_switch_port
> -done
> +delete_switch_ports 1 250
>  add_switch_ports 1 250
>  check ovn-nbctl --wait=sb sync
> -ovn-sbctl dump-flows | sed 's/arp.tpa == 10.1.0/arp.tpa ==
> 10.1.0.??/' | sed 's/eth.dst == ..:..:..:..:..:../??:??:??:??:??:??/' |sort
> > flows3
> +ovn-sbctl dump-flows | DUMP_FLOWS_SORTED > flows3
>  AT_CHECK([diff flows1 flows3])
>
>  AT_CLEANUP
> --
> 2.38.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn 2/2] tests: Check the default drop directly from flows dump

2022-12-13 Thread Ales Musil

On Tue, Dec 13, 2022 at 2:55 PM Xavier Simonart  wrote:

> Hi Ales
>
> Thanks for the patch and the nice speed-up
>
>
> On Mon, Dec 12, 2022 at 4:57 PM Ales Musil  wrote:
>
>> Instead of iterating over the logical flows in a loop
>> just check the flows directly filtering the portion the
>> tests is interested in. Reducing the duration ~17x.
>>
>> Reported-at: https://bugzilla.redhat.com/2149851
>> Signed-off-by: Ales Musil 
>> ---
>>  tests/ovn-northd.at | 115 
>>  1 file changed, 95 insertions(+), 20 deletions(-)
>>
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index 71eda9ff4..554a9b63b 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -8193,20 +8193,6 @@ OVN_FOR_EACH_NORTHD([
>>  AT_SETUP([Check default drop])
>>  AT_KEYWORDS([drop])
>>
>> -# Check that there is an explicit drop lflow in all tables.
>> -check_default_lflow() {
>> -dps=$(ovn-sbctl --bare  --columns=_uuid list Datapath_Binding |
>> xargs)
>> -for dp in $dps; do
>> -for pipeline in ingress egress; do
>> -for table in $(ovn-sbctl --bare --columns=table_id find
>> Logical_Flow logical_datapath="$dp" pipeline="$pipeline" | xargs | sort |
>> uniq); do
>> -   echo "Checking if datapath $dp pipeline $pipeline table
>> $table has a default action"
>> -   AT_CHECK([ovn-sbctl --columns=_uuid find Logical_Flow
>> logical_datapath="$dp" pipeline="$pipeline" table_id=$table match="1"
>> priority">="0 | wc -l | tr -d "\n\r" ], [0], [1], [ignore],
>> -   [echo "Datapath $dp pipeline $pipeline table $table does
>> not have a default action"])
>> -done
>> -done
>> -done
>> -}
>> -
>>  ovn_start
>>
>>  # Create LS + LR
>> @@ -8221,13 +8207,84 @@ check ovn-nbctl --wait=sb \
>>  -- lsp-add S1 p1 \
>>  -- lsp-set-addresses p1 "02:ac:10:01:00:0a 172.16.1.100"
>>
>> -check_default_lflow
>> +AT_CHECK([ovn-sbctl dump-flows | grep "match=(1)"  | sed
>> 's/table=../table=??/' | sort], [0], [dnl
>> +  table=??(lr_in_admission), priority=0, match=(1),
>> action=(drop;)
>> +  table=??(lr_in_arp_request  ), priority=0, match=(1),
>> action=(output;)
>> +  table=??(lr_in_arp_resolve  ), priority=0, match=(1),
>> action=(drop;)
>> +  table=??(lr_in_chk_pkt_len  ), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_in_defrag   ), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_in_dnat ), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_in_ecmp_stateful), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_in_gw_redirect  ), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_in_ip_input ), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_in_ip_routing   ), priority=0, match=(1),
>> action=(drop;)
>> +  table=??(lr_in_ip_routing_ecmp), priority=0, match=(1),
>> action=(drop;)
>> +  table=??(lr_in_ip_routing_pre), priority=0, match=(1),
>> action=(reg7 = 0; next;)
>> +  table=??(lr_in_larger_pkts  ), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_in_lb_aff_check ), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_in_lb_aff_learn ), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_in_learn_neighbor), priority=0, match=(1),
>> action=(drop;)
>> +  table=??(lr_in_lookup_neighbor), priority=0, match=(1),
>> action=(reg9[[2]] = 1; next;)
>> +  table=??(lr_in_nd_ra_options), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_in_nd_ra_response), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_in_policy   ), priority=0, match=(1),
>> action=(reg8[[0..15]] = 0; next;)
>> +  table=??(lr_in_policy_ecmp  ), priority=0, match=(1),
>> action=(drop;)
>> +  table=??(lr_in_unsnat   ), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_out_chk_dnat_local), priority=0, match=(1),
>> action=(reg9[[4]] = 0; next;)
>> +  table=??(lr_out_delivery), priority=0, match=(1),
>> action=(drop;)
>> +  table=??(lr_out_egr_loop), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_out_post_snat   ), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_out_post_undnat ), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_out_snat), priority=0, match=(1),
>> action=(next;)
>> +  table=??(lr_out_undnat  ), priority=0, match=(1),
>> action=(next;)
>> +  table=??(ls_in_acl  ), priority=65535, match=(1),
>> action=(next;)
>> +  table=??(ls_in_acl_after_lb ), priority=0, match=(1),
>> action=(next;)
>> +  table=??(ls_in_acl_hint ), priority=65535, match=(1),
>> action=(next;)
>> +  table=??(ls_in_apply_port_sec), priority=0, match=(1),
>> action=(next;)
>> +  table=??(ls_in_arp_rsp  ), priority=0, match=(1),
>> action=(next;)
>> +  table=??(ls_in_check_port_sec), priority=50   , match=(1),
>>

Re: [ovs-dev] [PATCH] ovsdb-cs: Always send first explicitly set monitor condition.

2022-12-13 Thread Ilya Maximets

On 12/13/22 13:21, Dumitru Ceara wrote:
> On 12/13/22 11:57, Ilya Maximets wrote:
>> On 12/12/22 20:42, Ilya Maximets wrote:
>>> On 12/8/22 16:21, Dumitru Ceara wrote:
 Applications request specific monitor conditions via the
 ovsdb_cs_set_condition() API.  This returns the condition sequence
 number at which the client can assume that the server acknowledged
 and applied the new monitor condition.

 A corner case is when the client's first request is to set the condition
 to a value that's equivalent to the default one ("").  In such
 cases, because it's requested explicitly by the client, we now
 explicitly send the monitor_cond_change request to the server.  This
 avoids cases in which the expected condition sequence number and the
 acked condition sequence number get out of sync.
>>>
>>> Sorry, I think I'm lost again.  What exactly we're trying to fix here?
>>> [True] is a default condition if no other conditions are supplied for
>>> the table.  Why do we need to send it explicitly?
>>>
> 
> The problem is that there's a discrepancy between the returned expected
> condition seqno and what happens if a condition is explicitly set to
> [True] before vs after the initial connection happened.
> 
>>> At first I thought that newly created condition from ovsdb_cs_db_get_table()
>>> has to be sent out, but...  if it is the first time we're setting
>>> conditions for that table, default condition should be [True], hence
>>> equal to the condition we're trying to set.
>>>
>>>
>>> The test case is a bit synthetic as it doesn't seem to test the issue
>>> that exists in OVN.  The fact that python IDL passes the test without
>>> changes also doesn't make a lot of sense to me.
>>>
> 
> Actually it should test exactly what's happenning in OVN.  That is:
> 
> - connect to database, no explicit monitor condition set
> - get initial contents
> - explicitly set monitor condition to [True] and get seqno S at
>   which the condition should be "acked".
> - expect that ovsdb_idl_get_condition_seqno() eventually reaches S
> 
> The Python IDL behaves differently unfortunately, I'll give more details
> below.
> 
>>> Could you provide some more data on what exactly is going on and what
>>> we're trying to fix?  With jsonrpc logs, if possible.
>>
> 
> For the C client, in the ovn-controller case, let's focus on 2 tables
> Chassis_Private and Chassis_Template_Var.  The difference between these
> 2 is that Chassis_Template_Var was recently added to the schema so
> ovn-controller only sets its monitor condition if
> sbrec_server_has_chassis_template_var_table() is true.  So that's only
> after the remote schema was received.
> 
> Sorry for the wide table below but this is the best way I could find
> that should explain what's happening:
> 
>   operation/eventreconnect-state
> ovsdb-cs-state  Chassis_Private-new_cond 
> Chassis_Template_Var-new_cond  cond-seqno   expected-cond-seqno
>   
> 
> 1.set_cond(Chassis_Private, [True])
>  BACKOFF   
> SERVER_SCHEMA_REQUESTED [True]  
> non-existent0 1


I think, this very first operation/event is wrong.
The condition change is never going to be sent, but the expected
sequence number is reported as 1.  The only reason this doesn't
cause any problems right away is that we're going to reconnect
here and drop the state, so the application will not rely on that
number in the end.  But it is still an incorrect return value.

The change that I suggested should fix that fundamental issue.



>>
>> Should we do this instead:
>>
>> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
>> index a6fbd290c..0fca03d72 100644
>> --- a/lib/ovsdb-cs.c
>> +++ b/lib/ovsdb-cs.c
>> @@ -892,7 +892,7 @@ ovsdb_cs_db_get_table(struct ovsdb_cs_db *db, const char 
>> *table)
>>  
>>  t = xzalloc(sizeof *t);
>>  t->name = xstrdup(table);
>> -t->new_cond = json_array_create_1(json_boolean_create(true));
>> +t->ack_cond = json_array_create_1(json_boolean_create(true));
>>  hmap_insert(>tables, >hmap_node, hash);
>>  return t;
>>  }
>> ---
>>
>> ?
>>
>> This breaks a few tests, but it seems to be a correct change because it
>> is indeed a default condition that server already has for the table.
>>
>> What do you think?  Will that fix the OVN oproblem?
>>
> 
> That would fix the OVN problem I think.  I'm not sure how to rewrite the
> "[track, simple idl, initially populated, strong references, conditional]"
> test however.

Something like this should fix it:

diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index 57ed0ac9d..1846cb521 100644
--- a/tests/ovsdb-idl.at
+++

[ovs-dev] Fwd: [PATCH ovn 2/2] tests: Check the default drop directly from flows dump

2022-12-13 Thread Xavier Simonart

Hi Ales

Thanks for the patch and the nice speed-up


On Mon, Dec 12, 2022 at 4:57 PM Ales Musil  wrote:

> Instead of iterating over the logical flows in a loop
> just check the flows directly filtering the portion the
> tests is interested in. Reducing the duration ~17x.
>
> Reported-at: https://bugzilla.redhat.com/2149851
> Signed-off-by: Ales Musil 
> ---
>  tests/ovn-northd.at | 115 
>  1 file changed, 95 insertions(+), 20 deletions(-)
>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 71eda9ff4..554a9b63b 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -8193,20 +8193,6 @@ OVN_FOR_EACH_NORTHD([
>  AT_SETUP([Check default drop])
>  AT_KEYWORDS([drop])
>
> -# Check that there is an explicit drop lflow in all tables.
> -check_default_lflow() {
> -dps=$(ovn-sbctl --bare  --columns=_uuid list Datapath_Binding | xargs)
> -for dp in $dps; do
> -for pipeline in ingress egress; do
> -for table in $(ovn-sbctl --bare --columns=table_id find
> Logical_Flow logical_datapath="$dp" pipeline="$pipeline" | xargs | sort |
> uniq); do
> -   echo "Checking if datapath $dp pipeline $pipeline table
> $table has a default action"
> -   AT_CHECK([ovn-sbctl --columns=_uuid find Logical_Flow
> logical_datapath="$dp" pipeline="$pipeline" table_id=$table match="1"
> priority">="0 | wc -l | tr -d "\n\r" ], [0], [1], [ignore],
> -   [echo "Datapath $dp pipeline $pipeline table $table does
> not have a default action"])
> -done
> -done
> -done
> -}
> -
>  ovn_start
>
>  # Create LS + LR
> @@ -8221,13 +8207,84 @@ check ovn-nbctl --wait=sb \
>  -- lsp-add S1 p1 \
>  -- lsp-set-addresses p1 "02:ac:10:01:00:0a 172.16.1.100"
>
> -check_default_lflow
> +AT_CHECK([ovn-sbctl dump-flows | grep "match=(1)"  | sed
> 's/table=../table=??/' | sort], [0], [dnl
> +  table=??(lr_in_admission), priority=0, match=(1), action=(drop;)
> +  table=??(lr_in_arp_request  ), priority=0, match=(1),
> action=(output;)
> +  table=??(lr_in_arp_resolve  ), priority=0, match=(1), action=(drop;)
> +  table=??(lr_in_chk_pkt_len  ), priority=0, match=(1), action=(next;)
> +  table=??(lr_in_defrag   ), priority=0, match=(1), action=(next;)
> +  table=??(lr_in_dnat ), priority=0, match=(1), action=(next;)
> +  table=??(lr_in_ecmp_stateful), priority=0, match=(1), action=(next;)
> +  table=??(lr_in_gw_redirect  ), priority=0, match=(1), action=(next;)
> +  table=??(lr_in_ip_input ), priority=0, match=(1), action=(next;)
> +  table=??(lr_in_ip_routing   ), priority=0, match=(1), action=(drop;)
> +  table=??(lr_in_ip_routing_ecmp), priority=0, match=(1),
> action=(drop;)
> +  table=??(lr_in_ip_routing_pre), priority=0, match=(1), action=(reg7
> = 0; next;)
> +  table=??(lr_in_larger_pkts  ), priority=0, match=(1), action=(next;)
> +  table=??(lr_in_lb_aff_check ), priority=0, match=(1), action=(next;)
> +  table=??(lr_in_lb_aff_learn ), priority=0, match=(1), action=(next;)
> +  table=??(lr_in_learn_neighbor), priority=0, match=(1),
> action=(drop;)
> +  table=??(lr_in_lookup_neighbor), priority=0, match=(1),
> action=(reg9[[2]] = 1; next;)
> +  table=??(lr_in_nd_ra_options), priority=0, match=(1), action=(next;)
> +  table=??(lr_in_nd_ra_response), priority=0, match=(1),
> action=(next;)
> +  table=??(lr_in_policy   ), priority=0, match=(1),
> action=(reg8[[0..15]] = 0; next;)
> +  table=??(lr_in_policy_ecmp  ), priority=0, match=(1), action=(drop;)
> +  table=??(lr_in_unsnat   ), priority=0, match=(1), action=(next;)
> +  table=??(lr_out_chk_dnat_local), priority=0, match=(1),
> action=(reg9[[4]] = 0; next;)
> +  table=??(lr_out_delivery), priority=0, match=(1), action=(drop;)
> +  table=??(lr_out_egr_loop), priority=0, match=(1), action=(next;)
> +  table=??(lr_out_post_snat   ), priority=0, match=(1), action=(next;)
> +  table=??(lr_out_post_undnat ), priority=0, match=(1), action=(next;)
> +  table=??(lr_out_snat), priority=0, match=(1), action=(next;)
> +  table=??(lr_out_undnat  ), priority=0, match=(1), action=(next;)
> +  table=??(ls_in_acl  ), priority=65535, match=(1), action=(next;)
> +  table=??(ls_in_acl_after_lb ), priority=0, match=(1), action=(next;)
> +  table=??(ls_in_acl_hint ), priority=65535, match=(1), action=(next;)
> +  table=??(ls_in_apply_port_sec), priority=0, match=(1),
> action=(next;)
> +  table=??(ls_in_arp_rsp  ), priority=0, match=(1), action=(next;)
> +  table=??(ls_in_check_port_sec), priority=50   , match=(1),
> action=(reg0[[15]] = check_in_port_sec(); next;)
> +  table=??(ls_in_dhcp_options ), priority=0, match=(1), action=(next;)
> +  table=??(ls_in_dhcp_response), priority=0, match=(1), action=(next;)
> +  table=??(ls_in_dns_lookup   ),

Re: [ovs-dev] [PATCH] travis: Fix remaining trace of kernel build tests.

2022-12-13 Thread Ilya Maximets

On 12/13/22 14:29, Aaron Conole wrote:
> David Marchand  writes:
> 
>> On Mon, Dec 12, 2022 at 5:14 PM Ilya Maximets  wrote:
>>>
>>> On 12/12/22 16:58, David Marchand wrote:
 We stopped building OVS kernel module in the CI but forgot to update the
 (unused?) Travis configuration.
>>>
>>> I'd say, we should just remove the yml.  Nobody seems to use it anyway.
>>> What do you think?
>>
>> I don't mind dropping it, but just in case, I Cc'ed some ARM people.
> 
> I am certainly in favor of dropping it.  The credits system they
> implemented has made it unusable for most projects without some real $$
> being spent.

+1

For the ARM CI, we should implement new ARM jobs in Cirrus CI.

> 
>>>

 Fixes: b6941ca7b8c9 ("ci: Stop building OVS kernel module.")
 Signed-off-by: David Marchand 
> 

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

Re: [ovs-dev] [PATCH] travis: Fix remaining trace of kernel build tests.

2022-12-13 Thread Aaron Conole

David Marchand  writes:

> On Mon, Dec 12, 2022 at 5:14 PM Ilya Maximets  wrote:
>>
>> On 12/12/22 16:58, David Marchand wrote:
>> > We stopped building OVS kernel module in the CI but forgot to update the
>> > (unused?) Travis configuration.
>>
>> I'd say, we should just remove the yml.  Nobody seems to use it anyway.
>> What do you think?
>
> I don't mind dropping it, but just in case, I Cc'ed some ARM people.

I am certainly in favor of dropping it.  The credits system they
implemented has made it unusable for most projects without some real $$
being spent.

>>
>> >
>> > Fixes: b6941ca7b8c9 ("ci: Stop building OVS kernel module.")
>> > Signed-off-by: David Marchand 

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

Re: [ovs-dev] [PATCH v3 ovn] actions: introduce ct_commit_continue action

2022-12-13 Thread Dumitru Ceara

On 12/13/22 07:17, Han Zhou wrote:
> On Mon, Dec 12, 2022 at 8:52 AM Numan Siddique  wrote:
>>
>> On Thu, Dec 8, 2022 at 12:15 PM Lorenzo Bianconi
>>  wrote:
>>>
>>> In the current codebase ct_commit {} action clears ct_state metadata of
>>> the incoming packet. This behaviour introduces an issue if we need to
>>> check the connection tracking state in the subsequent pipeline stages,
>>> e.g. for hairpin traffic:
>>>
>>> table=14(ls_in_pre_hairpin  ), priority=100  , match=(ip && ct.trk),
> action=(reg0[6] = chk_lb_hairpin(); reg0[12] = chk_lb_hairpin_reply();
> next;)
>>>
>>> Fix the issue introducing ct_commit_continue action used to allow the ct
>>> packet to proceed in the pipeline instead of the original one.
>>>
>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2103086
>>> Signed-off-by: Lorenzo Bianconi 
>>
>> I've a few comments.
>>
>> Since the OVS action ct() supports specifying the recirculation table
>> number,  I think we can do something similar in OVN too
>> instead of just restricting to the next table.
>>
>> I'd suggest something similar on the v1 of this patch, but instead of
>> the next flag,  ovn-northd can specify the table number.
>>
>> Something like   - ct_commit {..., table=x}  where 'x' is a valid
>> table number in the stage where this action is used i.e restrict the
>> jump to the table within the
>> ingress or egress pipeline but not from ingress to egress or from
>> egress to ingress.
>>
>> ovn-northd can use the same feature flag approach used in this patch
>> to include this 'table' option.
>>
>> Also I'd suggest making use of this new version of ct_commit in all
>> the places and not just in the specific ones.
>>
>> Later if there is a requirement to skip a few stages after committing
>> the packet to conntrack we can do so without the need to add a new
>> action or modify ct_commit.
>>
>> @Dumitru Ceara @Mark Michelson  @Han Zhou  Do you have any thoughts ?
>> or objections ?

Hi all,

> 
> Hi Numan, Lorenzo, I agree with Numan that adding the table=x in ct_commit
> action is more flexible, but I have a different concern. Either the
> ct_commit_continue implemented by this patch or the ct_commit {...,table=x}
> proposed by Numan would introduce an extra recirculation, which has a
> significant cost for data-plane. Since apply-after-lb is now frequently
> used for ACLs that implement egress network policies, it is better to avoid
> this penalty if possible. Another option to solve the problem is to move
> the ls_in_stateful stage after the hairpin stages, and keep the current
> ct_commit. Would that work?
> 

I think you're right, Han.  We seem to add quite a few extra recirculations.
For example, the datapath flows for a SYN packet hitting a load balancer
(dnats from 66.66.66.66:666 to 42.42.42.2:4242):

recirc_id(0),in_port(5),eth(src=00:00:00:00:00:00/01:00:00:00:00:00,dst=00:00:00:00:01:00),eth_type(0x0800),ipv4(dst=66.66.66.66,proto=6,frag=no),tcp(dst=666),
 packets:1, bytes:66, used:1.180s, flags:., actions:ct(zone=6,nat),recirc(0x50)
recirc_id(0x50),in_port(5),ct_state(+new-est-rel-rpl-inv+trk),ct_mark(0/0x1),eth(dst=00:00:00:00:01:00),eth_type(0x0800),ipv4(dst=66.66.66.66,proto=6,frag=no),tcp(dst=666),
 packets:0, bytes:0, used:never, actions:hash(l4(0)),recirc(0x54)
recirc_id(0x54),dp_hash(0x7/0xf),in_port(5),eth(),eth_type(0x0800),ipv4(frag=no),
 packets:0, bytes:0, used:never, 
actions:ct(commit,zone=6,mark=0x2/0x2,nat(dst=42.42.42.2:4242)),recirc(0x55)
recirc_id(0x55),in_port(5),ct_state(+new-est-rel-rpl-inv+trk),ct_mark(0/0x1),eth(src=00:00:00:00:00:02,dst=00:00:00:00:01:00),eth_type(0x0800),ipv4(src=42.42.42.2/255.255.255.254,dst=42.42.42.2,proto=6,ttl=64,frag=no),
 packets:0, bytes:0, used:never, 
actions:ct(commit,zone=6,mark=0/0x1,nat(src)),set(eth(src=00:00:00:00:01:00,dst=00:00:00:00:00:01)),set(ipv4(ttl=63)),ct(zone=5,nat),recirc(0x51)
recirc_id(0x51),in_port(5),ct_state(+new-est-rel-rpl-inv+trk),ct_mark(0/0x1),eth(src=00:00:00:00:01:00,dst=00:00:00:00:00:00/01:00:00:00:00:00),eth_type(0x0800),ipv4(frag=no),
 packets:0, bytes:0, used:never, actions:ct(commit,zone=5,mark=0/0x1,nat(src)),4

Now, with Lorenzo's patch:

recirc_id(0),in_port(5),eth(src=00:00:00:00:00:00/01:00:00:00:00:00,dst=00:00:00:00:01:00),eth_type(0x0800),ipv4(dst=66.66.66.66,proto=6,frag=no),tcp(dst=666),
 packets:1, bytes:66, used:1.893s, flags:., actions:ct(zone=6,nat),recirc(0x47)
recirc_id(0x47),in_port(5),ct_state(+new-est-rel-rpl-inv+trk),ct_mark(0/0x1),eth(dst=00:00:00:00:01:00),eth_type(0x0800),ipv4(dst=66.66.66.66,proto=6,frag=no),tcp(dst=666),
 packets:0, bytes:0, used:never, actions:hash(l4(0)),recirc(0x48)
recirc_id(0x48),dp_hash(0x1/0xf),in_port(5),eth(),eth_type(0x0800),ipv4(frag=no),
 packets:0, bytes:0, used:never, 
actions:ct(commit,zone=6,mark=0x2/0x2,nat(dst=42.42.42.2:4242)),recirc(0x49)
recirc_id(0x49),in_port(5),eth(),eth_type(0x0800),ipv4(frag=no), packets:0, 
bytes:0, used:never, actions:ct(commit,zone=6,mark=0/0x1,nat(src)),recirc(0x4a)

Re: [ovs-dev] [ovs-dev v7 1/3] ofproto-dpif-upcall: fix push_dp_ops

2022-12-13 Thread Eelco Chaudron



On 10 Dec 2022, at 1:37, Peng He wrote:

> Patch v5 has statistics issues.
>
> In order to solve this issue, we had a discussion.
>
> below is the quote of the email.
>
> ”
> After a second thought, I think maybe keeping INCONSISTENT just for the
> modify error is a better option.
>
> With current patch:
> 1.
> the modify error case:
> OPERATIONAL -> INCONSISTENT ->  EVICTING -> EVICTED
> 2.
> the delete error case:
> EVICTING -> EVICTED
>
> Change both to INCONSISTENT:
>
> the modify error case:
> did not change.
>
> the delete error case:
> EVICTING -> INCONSISTENT -> EVICTED?
>
> “
>
> And we agree to take the second solution.

I know, but going over the state meanings again, UKEY_EVICTING means the 
following:

 /* Ukey is in umap, datapath flow delete is queued. */

Which now no longer is the case, so should a new state not make more sense?

Any one else has some input on this??

> Eelco Chaudron  于2022年12月8日周四 18:54写道：
>
>>
>>
>> On 27 Nov 2022, at 8:28, Peng He wrote:
>>
>>> push_dp_ops only handles delete ops errors but ignores the modify
>>> ops results. It's better to handle all the dp operation errors in
>>> a consistent way.
>>>
>>> We observe in the production environment that sometimes a megaflow
>>> with wrong actions keep staying in datapath. The coverage command shows
>>> revalidators have dumped several times, however the correct
>>> actions are not set. This implies that the ukey's action does not
>>> equal to the meagaflow's, i.e. revalidators think the underlying
>>> megaflow's actions are correct however they are not.
>>>
>>> We also check the megaflow using the ofproto/trace command, and the
>>> actions are not matched with the ones in the actual magaflow. By
>>> performing a revalidator/purge command, the right actions are set.
>>>
>>> This patch prevents the inconsistency by considering modify failure
>>> in revalidators.
>>>
>>> To note, we cannot perform two state transitions and change ukey_state
>>> into UKEY_EVICTED directly here, because, if we do so, the
>>> sweep will remove the ukey alone and leave dp flow alive. Later, the
>>> dump will retrieve the dp flow and might even recover it. This will
>>> contribute the stats of this dp flow twice.
>>>
>>> Signed-off-by: Peng He 
>>> ---
>>>  ofproto/ofproto-dpif-upcall.c | 34 +++---
>>>  1 file changed, 23 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif-upcall.c
>> b/ofproto/ofproto-dpif-upcall.c
>>> index 7ad728adf..c2cefbeb8 100644
>>> --- a/ofproto/ofproto-dpif-upcall.c
>>> +++ b/ofproto/ofproto-dpif-upcall.c
>>> @@ -2416,26 +2416,30 @@ push_dp_ops(struct udpif *udpif, struct ukey_op
>> *ops, size_t n_ops)
>>>
>>>  for (i = 0; i < n_ops; i++) {
>>>  struct ukey_op *op = [i];
>>> -struct dpif_flow_stats *push, *stats, push_buf;
>>> -
>>> -stats = op->dop.flow_del.stats;
>>> -push = _buf;
>>> -
>>> -if (op->dop.type != DPIF_OP_FLOW_DEL) {
>>> -/* Only deleted flows need their stats pushed. */
>>> -continue;
>>> -}
>>>
>>>  if (op->dop.error) {
>>> -/* flow_del error, 'stats' is unusable. */
>>>  if (op->ukey) {
>>>  ovs_mutex_lock(>ukey->mutex);
>>> -transition_ukey(op->ukey, UKEY_EVICTED);
>>> +if (op->dop.type == DPIF_OP_FLOW_DEL) {
>>> +transition_ukey(op->ukey, UKEY_EVICTED);
>>> +} else {

I think we could use a comment here to make sure why we set it to evicting. 
Maybe just a reference to the comment in revalidator_sweep__() might be enough.

>>> +transition_ukey(op->ukey, UKEY_EVICTING);
>>> +}
>>>  ovs_mutex_unlock(>ukey->mutex);
>>>  }
>>>  continue;
>>>  }
>>>
>>> +if (op->dop.type != DPIF_OP_FLOW_DEL) {
>>> +/* Only deleted flows need their stats pushed. */
>>> +continue;
>>> +}
>>> +
>>> +struct dpif_flow_stats *push, *stats, push_buf;
>>> +
>>> +stats = op->dop.flow_del.stats;
>>> +push = _buf;
>>> +
>>>  if (op->ukey) {
>>>  ovs_mutex_lock(>ukey->mutex);
>>>  transition_ukey(op->ukey, UKEY_EVICTED);
>>> @@ -2848,6 +2852,14 @@ revalidator_sweep__(struct revalidator
>> *revalidator, bool purge)
>>>  continue;
>>>  }
>>>  ukey_state = ukey->state;
>>> +
>>> +if (ukey_state == UKEY_EVICTING) {
>>> +/* previous modify operation fails on this ukey and
>> ukey_state
>>> + * is set to UKEY_EVICTING, issue a delete operation on
>> this
>>> + * ukey.
>>> + */
>>> +delete_op_init(udpif, [n_ops++], ukey);

This would cause the later push_ukey_ops() to call ukey_delete() which will 
call transition_ukey(ukey, UKEY_DELETED).
Which I think will result in an invalid state

Re: [ovs-dev] [PATCH] ovsdb-cs: Always send first explicitly set monitor condition.

2022-12-13 Thread Dumitru Ceara

On 12/13/22 11:57, Ilya Maximets wrote:
> On 12/12/22 20:42, Ilya Maximets wrote:
>> On 12/8/22 16:21, Dumitru Ceara wrote:
>>> Applications request specific monitor conditions via the
>>> ovsdb_cs_set_condition() API.  This returns the condition sequence
>>> number at which the client can assume that the server acknowledged
>>> and applied the new monitor condition.
>>>
>>> A corner case is when the client's first request is to set the condition
>>> to a value that's equivalent to the default one ("").  In such
>>> cases, because it's requested explicitly by the client, we now
>>> explicitly send the monitor_cond_change request to the server.  This
>>> avoids cases in which the expected condition sequence number and the
>>> acked condition sequence number get out of sync.
>>
>> Sorry, I think I'm lost again.  What exactly we're trying to fix here?
>> [True] is a default condition if no other conditions are supplied for
>> the table.  Why do we need to send it explicitly?
>>

The problem is that there's a discrepancy between the returned expected
condition seqno and what happens if a condition is explicitly set to
[True] before vs after the initial connection happened.

>> At first I thought that newly created condition from ovsdb_cs_db_get_table()
>> has to be sent out, but...  if it is the first time we're setting
>> conditions for that table, default condition should be [True], hence
>> equal to the condition we're trying to set.
>>
>>
>> The test case is a bit synthetic as it doesn't seem to test the issue
>> that exists in OVN.  The fact that python IDL passes the test without
>> changes also doesn't make a lot of sense to me.
>>

Actually it should test exactly what's happenning in OVN.  That is:

- connect to database, no explicit monitor condition set
- get initial contents
- explicitly set monitor condition to [True] and get seqno S at
  which the condition should be "acked".
- expect that ovsdb_idl_get_condition_seqno() eventually reaches S

The Python IDL behaves differently unfortunately, I'll give more details
below.

>> Could you provide some more data on what exactly is going on and what
>> we're trying to fix?  With jsonrpc logs, if possible.
> 

For the C client, in the ovn-controller case, let's focus on 2 tables
Chassis_Private and Chassis_Template_Var.  The difference between these
2 is that Chassis_Template_Var was recently added to the schema so
ovn-controller only sets its monitor condition if
sbrec_server_has_chassis_template_var_table() is true.  So that's only
after the remote schema was received.

Sorry for the wide table below but this is the best way I could find
that should explain what's happening:

  operation/eventreconnect-state
ovsdb-cs-state  Chassis_Private-new_cond 
Chassis_Template_Var-new_cond  cond-seqno   expected-cond-seqno
  

1.set_cond(Chassis_Private, [True])
 BACKOFF   
SERVER_SCHEMA_REQUESTED [True]  
non-existent0 1
2.reconnect->CONNECTING
  - ovsdb_cs_db_sync_condition(OVN_Southbound) is called and
ack_all == true
  - this will move all ovsdb_cs table->req_cond to table->new_cond;
ovsdb_cs only knows of the existence of the Chassis_Private
table.
 CONNECTING
SERVER_SCHEMA_REQUESTED   nil   
non-existent0 1

# new event loop iteration
3.set_cond(Chassis_Private, [True])
 CONNECTING
SERVER_SCHEMA_REQUESTED   nil   
non-existent0 0
4.reconnect->CONNECTED
  - ovsdb_cs requests server schema
  jsonrpc|DBG|ssl:127.0.0.1:6642: send request, method="get_schema", 
params=["_Server"], id=7
 CONNECTED 
SERVER_SCHEMA_REQUESTED   nil   
non-existent0 0

# new event loop iteration
5.set_cond(Chassis_Private, [True])
 CONNECTED 
SERVER_SCHEMA_REQUESTED   nil   
non-existent0 0
6.ovsdb_cs receives server schema:
  jsonrpc|DBG|ssl:127.0.0.1:6642: received reply, 
result={...,"name":"_Server",...}
  - ovsdb_cs sends monitor_cond for the server db:
  jsonrpc|DBG|ssl:127.0.0.1:6642: send request, method="monitor_cond", 
params=["_Server",...]

Re: [ovs-dev] [PATCH v2] rhel: move conf.db to /var/lib/openvswitch, using symlinks

2022-12-13 Thread Ilya Maximets

On 12/5/22 15:36, Ilya Maximets wrote:
> On 12/4/22 09:23, Roi Dayan wrote:
>>
>>
>> On 30/11/2022 17:55, Ilya Maximets wrote:
>>> On 11/14/22 20:41, Timothy Redaelli wrote:
 conf.db is by default at /etc/openvswitch, but it should be at
 /var/lib/openvswitch like on Debian or like ovnnb_db.db and ovnsb_db.db.

 If conf.db already exists in /etc/openvswitch then it's moved to
 /var/lib/openvswitch.
 Symlinks are created for conf.db and .conf.db.~lock~ into /etc/openvswitch
 for backward compatibility.

 Reported-at: 
 https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2F1830857data=05%7C01%7Croid%40nvidia.com%7Cd69116141ff645fc2c7308dad2eb4612%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638054205222362304%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=%2BIcIVZBKrfhIpq%2B6r6I3QvjdZ9KvjLsrRSlvi9kFHzc%3Dreserved=0
 Reported-by: Yedidyah Bar David 
 Signed-off-by: Timothy Redaelli 
 ---
 v1 -> v2:
 - Use hugetlbfs group instead of openvswitch when the package is built
   with dpdk (as reported by Flavio)
 ---
  rhel/openvswitch-fedora.spec.in | 27 +++
  1 file changed, 23 insertions(+), 4 deletions(-)
>>>
>>> If that works for Fedora, then LGTM.  Applied.
>>>
>>> Thanks!
>>> Best regards, Ilya Maximets.
>>> ___
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-devdata=05%7C01%7Croid%40nvidia.com%7Cd69116141ff645fc2c7308dad2eb4612%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638054205222362304%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=fZZh4iYeUu%2BL2%2F%2FWTIgPNzpvfhpe%2F9MANkVPLmv57aY%3Dreserved=0
>>
>>
>> hi,
>>
>> This commit expose some kind of issue and cause openvswitch not
>> to start on clean systems.
>>
>> If old conf.db file didn't exists it creates an empty conf.db with
>> the touch command.
>> Empty conf.db cause ovsdb-server not to start.
>>
>> #  /usr/share/openvswitch/scripts/ovs-ctl start
>> ovsdb-tool: ovsdb error: /etc/openvswitch/conf.db: cannot identify file type
>> Starting ovsdb-server ovsdb-server: ovsdb error: /etc/openvswitch/conf.db: 
>> cannot identify file type
>>[FAILED]
>>
>> If I remove the conf.db file (can leave the symbolic link in /etc)
>> then ovs starts fine.
>> # rm /var/lib/openvswitch/conf.db
>> #  /usr/share/openvswitch/scripts/ovs-ctl start
>> /etc/openvswitch/conf.db does not exist ... (warning).
>> Creating empty database /etc/openvswitch/conf.db   [  OK  ]
>> Starting ovsdb-server  [  OK  ]
>> system ID not configured, please use --system-id ... failed!
>> Configuring Open vSwitch system IDs[  OK  ]
>> Starting ovs-vswitchd  [  OK  ]
>> Enabling remote OVSDB managers [  OK  ]
>>
>>
>> I'm not sure where it's better to fix this. either the spec here
>> not to create an empty file or in ovsdb/log.c to an accept empty conf.db,
>> or maybe even upgrade_db() in ovs-lib bash file to call create_db
>> even if conf.db exists but it's empty.
> 
> Thanks, Roi, for the report!
> I think, fixing the spec should be the right approach here.

Hi, Timothy.  Do you plan to work on the fix for this issue?

Otherwise we may just revert the change for now until the proper
fix is available.  Thoughts?

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

Re: [ovs-dev] [PATCH] ovsdb-cs: Always send first explicitly set monitor condition.

2022-12-13 Thread Ilya Maximets

On 12/12/22 20:42, Ilya Maximets wrote:
> On 12/8/22 16:21, Dumitru Ceara wrote:
>> Applications request specific monitor conditions via the
>> ovsdb_cs_set_condition() API.  This returns the condition sequence
>> number at which the client can assume that the server acknowledged
>> and applied the new monitor condition.
>>
>> A corner case is when the client's first request is to set the condition
>> to a value that's equivalent to the default one ("").  In such
>> cases, because it's requested explicitly by the client, we now
>> explicitly send the monitor_cond_change request to the server.  This
>> avoids cases in which the expected condition sequence number and the
>> acked condition sequence number get out of sync.
> 
> Sorry, I think I'm lost again.  What exactly we're trying to fix here?
> [True] is a default condition if no other conditions are supplied for
> the table.  Why do we need to send it explicitly?
> 
> At first I thought that newly created condition from ovsdb_cs_db_get_table()
> has to be sent out, but...  if it is the first time we're setting
> conditions for that table, default condition should be [True], hence
> equal to the condition we're trying to set.
> 
> 
> The test case is a bit synthetic as it doesn't seem to test the issue
> that exists in OVN.  The fact that python IDL passes the test without
> changes also doesn't make a lot of sense to me.
> 
> Could you provide some more data on what exactly is going on and what
> we're trying to fix?  With jsonrpc logs, if possible.


Should we do this instead:

diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
index a6fbd290c..0fca03d72 100644
--- a/lib/ovsdb-cs.c
+++ b/lib/ovsdb-cs.c
@@ -892,7 +892,7 @@ ovsdb_cs_db_get_table(struct ovsdb_cs_db *db, const char 
*table)
 
 t = xzalloc(sizeof *t);
 t->name = xstrdup(table);
-t->new_cond = json_array_create_1(json_boolean_create(true));
+t->ack_cond = json_array_create_1(json_boolean_create(true));
 hmap_insert(>tables, >hmap_node, hash);
 return t;
 }
---

?

This breaks a few tests, but it seems to be a correct change because it
is indeed a default condition that server already has for the table.

What do you think?  Will that fix the OVN oproblem?

> 
> Thanks.
> Best regards, Ilya Maximets.
> 
>>
>> Reported-by: Numan Siddique 
>> Signed-off-by: Dumitru Ceara 
>> ---
>>  lib/ovsdb-cs.c  |  1 +
>>  tests/ovsdb-idl.at  | 27 +++
>>  tests/test-ovsdb.c  | 28 
>>  tests/test-ovsdb.py | 20 +---
>>  4 files changed, 69 insertions(+), 7 deletions(-)

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

Re: [ovs-dev] [PATCH] [ovs-dev v2] dpctl: Add support to count upcall packets

2022-12-13 Thread 0-day Robot

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

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


checkpatch:
WARNING: Line has non-spaces leading whitespace
#84 FILE: lib/dpctl.c:745:
print_stat(dpctl_p, " upcall success:", s.tx_upcall_success);

WARNING: Line has non-spaces leading whitespace
#85 FILE: lib/dpctl.c:746:
print_stat(dpctl_p, " upcall fail:", s.tx_upcall_fail);

WARNING: Line has non-spaces leading whitespace
#98 FILE: lib/dpif-netlink.c:4689:
   .optional = true },

WARNING: Line has non-spaces leading whitespace
#129 FILE: lib/dpif-netlink.h:48:
   /* OVS_VPORT_ATTR_UPCALL_STATS. */

Lines checked: 162, Warnings: 4, Errors: 0


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

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

[ovs-dev] [PATCH] [ovs-dev v2] dpctl: Add support to count upcall packets

2022-12-13 Thread wangchuanlei

Add support to count upall packets, when kmod of openvswitch upcall to
count the number of packets for upcall succeed and failed, which is a
better way to see how many packets upcalled on every interfaces.

Signed-off-by: wangchuanlei 
---
ovs-kmod already support count statistic of interfaces, the link is
 below, and this commit is the part of userspace.

https://git.kernel.org/netdev/net-next/c/1933ea365aa7

note: this commit is compatible with old version of ovs-kmod, that is,
even the kernel is older, and do not support count statistic of
interfaces(do not have the code in upper link), this part of code is
still stable!

Here is modify based on code format check of robot

 include/linux/openvswitch.h  | 19 +++
 include/openvswitch/netdev.h |  3 +++
 lib/dpctl.c  |  2 ++
 lib/dpif-netlink.c   | 13 +
 lib/dpif-netlink.h   |  2 ++
 lib/netdev-linux.c   |  8 
 6 files changed, 47 insertions(+)

diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index 8bb5abdc8..ff2dc58c9 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -141,6 +141,11 @@ struct ovs_vport_stats {
__u64   tx_dropped; /* no space available in linux  */
 };
 
+struct ovs_vport_upcall_stats {
+   uint64_t   tx_success;  /* total packets upcall succeed */
+   uint64_t   tx_fail; /* total packets upcall failed  */
+};
+
 /* Allow last Netlink attribute to be unaligned */
 #define OVS_DP_F_UNALIGNED (1 << 0)
 
@@ -301,11 +306,25 @@ enum ovs_vport_attr {
OVS_VPORT_ATTR_PAD,
OVS_VPORT_ATTR_IFINDEX,
OVS_VPORT_ATTR_NETNSID,
+   OVS_VPORT_ATTR_UPCALL_STATS,
__OVS_VPORT_ATTR_MAX
 };
 
 #define OVS_VPORT_ATTR_MAX (__OVS_VPORT_ATTR_MAX - 1)
 
+/**
+* enum OVS_VPORT_UPCALL_ATTR -- attributes for %OVS_VPORT_UPCALL* commands
+* @OVS_VPORT_UPCALL_ATTR_SUCCESS: 64-bit upcall success packets.
+* @OVS_VPORT_UPCALL_ATTR_FAIL: 64-bit upcall fail packets.
+*/
+enum OVS_VPORT_UPCALL_ATTR {
+   OVS_VPORT_UPCALL_ATTR_SUCCESS,
+   OVS_VPORT_UPCALL_ATTR_FAIL,
+   __OVS_VPORT_UPCALL_ATTR_MAX,
+};
+
+#define OVS_VPORT_UPCALL_ATTR_MAX (__OVS_VPORT_UPCALL_ATTR_MAX - 1)
+
 enum {
OVS_VXLAN_EXT_UNSPEC,
OVS_VXLAN_EXT_GBP,
diff --git a/include/openvswitch/netdev.h b/include/openvswitch/netdev.h
index 0c10f7b48..ed1bf73dc 100644
--- a/include/openvswitch/netdev.h
+++ b/include/openvswitch/netdev.h
@@ -87,6 +87,9 @@ struct netdev_stats {
 uint64_t rx_oversize_errors;
 uint64_t rx_fragmented_errors;
 uint64_t rx_jabber_errors;
+
+uint64_t tx_upcall_success;
+uint64_t tx_upcall_fail;
 };
 
 /* Structure representation of custom statistics counter */
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 29041fa3e..956d1736d 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -742,6 +742,8 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p)
 dpctl_print(dpctl_p, "\n");
 
 print_stat(dpctl_p, "collisions:", s.collisions);
+   print_stat(dpctl_p, " upcall success:", s.tx_upcall_success);
+   print_stat(dpctl_p, " upcall fail:", s.tx_upcall_fail);
 dpctl_print(dpctl_p, "\n");
 
 print_stat(dpctl_p, "RX bytes:", s.rx_bytes);
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 026b0daa8..f4dca3796 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -4685,6 +4685,8 @@ dpif_netlink_vport_from_ofpbuf(struct dpif_netlink_vport 
*vport,
.optional = true },
 [OVS_VPORT_ATTR_OPTIONS] = { .type = NL_A_NESTED, .optional = true },
 [OVS_VPORT_ATTR_NETNSID] = { .type = NL_A_U32, .optional = true },
+[OVS_VPORT_ATTR_UPCALL_STATS] = { .type = NL_A_NESTED,
+  .optional = true },
 };
 
 dpif_netlink_vport_init(vport);
@@ -4716,6 +4718,17 @@ dpif_netlink_vport_from_ofpbuf(struct dpif_netlink_vport 
*vport,
 if (a[OVS_VPORT_ATTR_STATS]) {
 vport->stats = nl_attr_get(a[OVS_VPORT_ATTR_STATS]);
 }
+if (a[OVS_VPORT_ATTR_UPCALL_STATS]) {
+const struct nlattr *nla;
+size_t left;
+NL_NESTED_FOR_EACH (nla, left, a[OVS_VPORT_ATTR_UPCALL_STATS]) {
+if (nl_attr_type(nla) == OVS_VPORT_UPCALL_ATTR_SUCCESS) {
+vport->upcall_stats.tx_success = nl_attr_get_u64(nla);
+} else if (nl_attr_type(nla) == OVS_VPORT_UPCALL_ATTR_FAIL) {
+vport->upcall_stats.tx_fail = nl_attr_get_u64(nla);
+}
+}
+}
 if (a[OVS_VPORT_ATTR_OPTIONS]) {
 vport->options = nl_attr_get(a[OVS_VPORT_ATTR_OPTIONS]);
 vport->options_len = nl_attr_get_size(a[OVS_VPORT_ATTR_OPTIONS]);
diff --git a/lib/dpif-netlink.h b/lib/dpif-netlink.h
index 24294bc42..80e22882f 100644
--- a/lib/dpif-netlink.h
+++ b/lib/dpif-netlink.h
@@ -44,6 +44,8 @@ struct dpif_netlink_vport

Re: [ovs-dev] [PATCH 2/2] dpif-netdev/mfex: Add AVX512 NVGRE traffic profiles.

2022-12-13 Thread Eelco Chaudron

On 16 Sep 2022, at 12:12, Cian Ferriter wrote:

> A typical NVGRE encapsulated packet starts with the ETH/IP/GRE
> protocols.  Miniflow extract will parse just the ETH and IP headers. The
> GRE header will be processed later as part of the pop action. Add
> support for parsing the ETH/IP headers in this scenario.
>
> Signed-off-by: Cian Ferriter 
> ---
>  lib/dp-packet.h   | 58 +++
>  lib/dpif-netdev-extract-avx512.c  | 43 +--
>  lib/dpif-netdev-private-extract.c | 10 ++
>  lib/dpif-netdev-private-extract.h |  5 +++
>  4 files changed, 100 insertions(+), 16 deletions(-)
>
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index 55eeaab2c..230bbec27 100644

Only one small nit, maybe Ilya can change on commit if he finds no other issues.

Acked-by: Eelco Chaudron 

> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -1089,8 +1089,28 @@ dp_packet_l4_checksum_bad(const struct dp_packet *p)
>  DP_PACKET_OL_RX_L4_CKSUM_BAD;
>  }
>
> +static inline uint32_t ALWAYS_INLINE
> +dp_packet_calc_hash_ipv4(const uint8_t *pkt, const uint16_t l3_ofs,
> + uint32_t hash)
> +{
> +const void *ipv4_src = [l3_ofs + offsetof(struct ip_header, ip_src)];
> +const void *ipv4_dst = [l3_ofs + offsetof(struct ip_header, ip_dst)];
> +uint32_t ip_src, ip_dst;
> +
> +memcpy(_src, ipv4_src, sizeof ip_src);
> +memcpy(_dst, ipv4_dst, sizeof ip_dst);
> +
> +/* IPv4 Src and Dst. */
> +hash = hash_add(hash, ip_src);
> +hash = hash_add(hash, ip_dst);

Add cr/lf before comments.

> +/* IPv4 proto. */
> +hash = hash_add(hash, pkt[l3_ofs + offsetof(struct ip_header, 
> ip_proto)]);



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

Re: [ovs-dev] [PATCH 1/2] dpif-netdev/dpcls: Specialize 8, 1 and 5, 2 signatures.

2022-12-13 Thread Eelco Chaudron

On 16 Sep 2022, at 12:12, Cian Ferriter wrote:

> The subtable signatures being specialized here were found in an NVGRE
> tunnel scenario.
>
> Signed-off-by: Cian Ferriter 
> ---

Sorry for the late response, but I was rather busy.
Changes look good to me, and did basic testing using make check* with all 
avx512 stuff enabled.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH] dpctl: Add support to count upcall packets

2022-12-13 Thread 0-day Robot

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

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


checkpatch:
WARNING: Line is 80 characters long (recommended limit is 79)
#84 FILE: lib/dpctl.c:747:
print_stat(dpctl_p, "upcall success:", s.tx_upcall_success);

WARNING: Line is 82 characters long (recommended limit is 79)
#99 FILE: lib/dpif-netlink.c:4688:
[OVS_VPORT_ATTR_UPCALL_STATS] = { .type = NL_A_NESTED, .optional = true 
},

WARNING: Line is 82 characters long (recommended limit is 79)
#129 FILE: lib/dpif-netlink.h:47:
struct ovs_vport_upcall_stats upcall_stats; /* OVS_VPORT_ATTR_UPCALL_STATS. 
*/

Lines checked: 162, Warnings: 3, Errors: 0


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

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

[ovs-dev] [PATCH] dpctl: Add support to count upcall packets

2022-12-13 Thread wangchuanlei

Add support to count upall packets, when kmod of openvswitch upcall to
count the number of packets for upcall succeed and failed, which is a
better way to see how many packets upcalled on every interfaces.

Signed-off-by: wangchuanlei 
---

ovs-kmod already support count statistic of interfaces, the link is
below, and this commit is the part of userspace.

https://git.kernel.org/netdev/net-next/c/1933ea365aa7

note: this commit is compatible with old version of ovs-kmod, that is,
even the kernel is older, and do not support count statistic of
interfaces(do not have the code in upper link), this part of code is
 still stable!

 include/linux/openvswitch.h  | 19 +++
 include/openvswitch/netdev.h |  3 +++
 lib/dpctl.c  |  4 
 lib/dpif-netlink.c   | 12 
 lib/dpif-netlink.h   |  1 +
 lib/netdev-linux.c   |  8 
 6 files changed, 47 insertions(+)

diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index 8bb5abdc8..ff2dc58c9 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -141,6 +141,11 @@ struct ovs_vport_stats {
__u64   tx_dropped; /* no space available in linux  */
 };
 
+struct ovs_vport_upcall_stats {
+   uint64_t   tx_success;  /* total packets upcall succeed */
+   uint64_t   tx_fail; /* total packets upcall failed  */
+};
+
 /* Allow last Netlink attribute to be unaligned */
 #define OVS_DP_F_UNALIGNED (1 << 0)
 
@@ -301,11 +306,25 @@ enum ovs_vport_attr {
OVS_VPORT_ATTR_PAD,
OVS_VPORT_ATTR_IFINDEX,
OVS_VPORT_ATTR_NETNSID,
+   OVS_VPORT_ATTR_UPCALL_STATS,
__OVS_VPORT_ATTR_MAX
 };
 
 #define OVS_VPORT_ATTR_MAX (__OVS_VPORT_ATTR_MAX - 1)
 
+/**
+* enum OVS_VPORT_UPCALL_ATTR -- attributes for %OVS_VPORT_UPCALL* commands
+* @OVS_VPORT_UPCALL_ATTR_SUCCESS: 64-bit upcall success packets.
+* @OVS_VPORT_UPCALL_ATTR_FAIL: 64-bit upcall fail packets.
+*/
+enum OVS_VPORT_UPCALL_ATTR {
+   OVS_VPORT_UPCALL_ATTR_SUCCESS,
+   OVS_VPORT_UPCALL_ATTR_FAIL,
+   __OVS_VPORT_UPCALL_ATTR_MAX,
+};
+
+#define OVS_VPORT_UPCALL_ATTR_MAX (__OVS_VPORT_UPCALL_ATTR_MAX - 1)
+
 enum {
OVS_VXLAN_EXT_UNSPEC,
OVS_VXLAN_EXT_GBP,
diff --git a/include/openvswitch/netdev.h b/include/openvswitch/netdev.h
index 0c10f7b48..ed1bf73dc 100644
--- a/include/openvswitch/netdev.h
+++ b/include/openvswitch/netdev.h
@@ -87,6 +87,9 @@ struct netdev_stats {
 uint64_t rx_oversize_errors;
 uint64_t rx_fragmented_errors;
 uint64_t rx_jabber_errors;
+
+uint64_t tx_upcall_success;
+uint64_t tx_upcall_fail;
 };
 
 /* Structure representation of custom statistics counter */
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 29041fa3e..c8b195c89 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -744,6 +744,10 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p)
 print_stat(dpctl_p, "collisions:", s.collisions);
 dpctl_print(dpctl_p, "\n");
 
+print_stat(dpctl_p, "upcall success:", 
s.tx_upcall_success);
+print_stat(dpctl_p, " upcall fail:", s.tx_upcall_fail);
+dpctl_print(dpctl_p, "\n");
+
 print_stat(dpctl_p, "RX bytes:", s.rx_bytes);
 print_human_size(dpctl_p, s.rx_bytes);
 print_stat(dpctl_p, "  TX bytes:", s.tx_bytes);
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 026b0daa8..871ac8b5a 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -4685,6 +4685,7 @@ dpif_netlink_vport_from_ofpbuf(struct dpif_netlink_vport 
*vport,
.optional = true },
 [OVS_VPORT_ATTR_OPTIONS] = { .type = NL_A_NESTED, .optional = true },
 [OVS_VPORT_ATTR_NETNSID] = { .type = NL_A_U32, .optional = true },
+[OVS_VPORT_ATTR_UPCALL_STATS] = { .type = NL_A_NESTED, .optional = 
true },
 };
 
 dpif_netlink_vport_init(vport);
@@ -4716,6 +4717,17 @@ dpif_netlink_vport_from_ofpbuf(struct dpif_netlink_vport 
*vport,
 if (a[OVS_VPORT_ATTR_STATS]) {
 vport->stats = nl_attr_get(a[OVS_VPORT_ATTR_STATS]);
 }
+if (a[OVS_VPORT_ATTR_UPCALL_STATS]) {
+const struct nlattr *nla;
+size_t left;
+NL_NESTED_FOR_EACH (nla, left, a[OVS_VPORT_ATTR_UPCALL_STATS]) {
+if (nl_attr_type(nla) == OVS_VPORT_UPCALL_ATTR_SUCCESS) {
+vport->upcall_stats.tx_success = nl_attr_get_u64(nla);
+} else if (nl_attr_type(nla) == OVS_VPORT_UPCALL_ATTR_FAIL) {
+vport->upcall_stats.tx_fail = nl_attr_get_u64(nla);
+}
+}
+}
 if (a[OVS_VPORT_ATTR_OPTIONS]) {
 vport->options = nl_attr_get(a[OVS_VPORT_ATTR_OPTIONS]);
 vport->options_len = nl_attr_get_size(a[OVS_VPORT_ATTR_OPTIONS]);
diff --git a/lib/dpif-netlink.h b/lib/dpif-netlink.h
index 24294bc42..7826e34f5 100644
--- a/lib/dpif-netlink.h
+++ b/lib/dpif-netlink.h

58 matches

Mail list logo