date:20221125

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

2022-11-25 Thread kernel test robot

Hi wangchuanlei,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on net/master linus/master v6.1-rc6 next-20221125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:
https://github.com/intel-lab-lkp/linux/commits/wangchuanlei/openvswitch-Add-support-to-count-upcall-packets/20221123-172156
patch link:
https://lore.kernel.org/r/20221123091843.3414856-1-wangchuanlei%40inspur.com
patch subject: [PATCH] [openvswitch v4] openvswitch: Add support to count 
upcall packets
config: x86_64-randconfig-a015
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# 
https://github.com/intel-lab-lkp/linux/commit/2a933adf45aad43350c7074e0f9a6a12e7f41986
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review 
wangchuanlei/openvswitch-Add-support-to-count-upcall-packets/20221123-172156
git checkout 2a933adf45aad43350c7074e0f9a6a12e7f41986
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot 

All errors (new ones prefixed by >>):

   In file included from :
>> ./usr/include/linux/openvswitch.h:130:9: error: unknown type name 'uint64_t'
 130 | uint64_t   upcall_success;  /* total packets upcalls 
succeed */
 | ^~~~
   ./usr/include/linux/openvswitch.h:131:9: error: unknown type name 'uint64_t'
 131 | uint64_t   upcall_fail; /* total packets upcalls 
failed  */
 | ^~~~

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp
#
# Automatically generated file; DO NOT EDIT.
# Linux/x86_64 6.1.0-rc5 Kernel Configuration
#
CONFIG_CC_VERSION_TEXT="gcc-11 (Debian 11.3.0-8) 11.3.0"
CONFIG_CC_IS_GCC=y
CONFIG_GCC_VERSION=110300
CONFIG_CLANG_VERSION=0
CONFIG_AS_IS_GNU=y
CONFIG_AS_VERSION=23900
CONFIG_LD_IS_BFD=y
CONFIG_LD_VERSION=23900
CONFIG_LLD_VERSION=0
CONFIG_CC_CAN_LINK=y
CONFIG_CC_CAN_LINK_STATIC=y
CONFIG_CC_HAS_ASM_GOTO_OUTPUT=y
CONFIG_CC_HAS_ASM_INLINE=y
CONFIG_CC_HAS_NO_PROFILE_FN_ATTR=y
CONFIG_PAHOLE_VERSION=123
CONFIG_CONSTRUCTORS=y
CONFIG_IRQ_WORK=y
CONFIG_BUILDTIME_TABLE_SORT=y
CONFIG_THREAD_INFO_IN_TASK=y

#
# General setup
#
CONFIG_INIT_ENV_ARG_LIMIT=32
# CONFIG_COMPILE_TEST is not set
# CONFIG_WERROR is not set
CONFIG_UAPI_HEADER_TEST=y
CONFIG_LOCALVERSION=""
CONFIG_LOCALVERSION_AUTO=y
CONFIG_BUILD_SALT=""
CONFIG_HAVE_KERNEL_GZIP=y
CONFIG_HAVE_KERNEL_BZIP2=y
CONFIG_HAVE_KERNEL_LZMA=y
CONFIG_HAVE_KERNEL_XZ=y
CONFIG_HAVE_KERNEL_LZO=y
CONFIG_HAVE_KERNEL_LZ4=y
CONFIG_HAVE_KERNEL_ZSTD=y
# CONFIG_KERNEL_GZIP is not set
# CONFIG_KERNEL_BZIP2 is not set
CONFIG_KERNEL_LZMA=y
# CONFIG_KERNEL_XZ is not set
# CONFIG_KERNEL_LZO is not set
# CONFIG_KERNEL_LZ4 is not set
# CONFIG_KERNEL_ZSTD is not set
CONFIG_DEFAULT_INIT=""
CONFIG_DEFAULT_HOSTNAME="(none)"
CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
# CONFIG_POSIX_MQUEUE is not set
CONFIG_WATCH_QUEUE=y
# CONFIG_CROSS_MEMORY_ATTACH is not set
CONFIG_USELIB=y
# CONFIG_AUDIT is not set
CONFIG_HAVE_ARCH_AUDITSYSCALL=y

#
# IRQ subsystem
#
CONFIG_GENERIC_IRQ_PROBE=y
CONFIG_GENERIC_IRQ_SHOW=y
CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=y
CONFIG_GENERIC_PENDING_IRQ=y
CONFIG_GENERIC_IRQ_MIGRATION=y
CONFIG_GENERIC_IRQ_INJECTION=y
CONFIG_HARDIRQS_SW_RESEND=y
CONFIG_IRQ_DOMAIN=y
CONFIG_IRQ_SIM=y
CONFIG_IRQ_DOMAIN_HIERARCHY=y
CONFIG_GENERIC_IRQ_MATRIX_ALLOCATOR=y
CONFIG_GENERIC_IRQ_RESERVATION_MODE=y
CONFIG_IRQ_FORCED_THREADING=y
CONFIG_SPARSE_IRQ=y
CONFIG_GENERIC_IRQ_DEBUGFS=y
# end of IRQ subsystem

CONFIG_CLOCKSOURCE_WATCHDOG=y
CONFIG_ARCH_CLOCKSOURCE_INIT=y
CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE=y
CONFIG_GENERIC_TIME_VSYSCALL=y
CONFIG_GENERIC_CLOCKEVENTS=y
CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=y
CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST=y
CONFIG_GENERIC_CMOS_UPDATE=y
CONFIG_HAVE_POSIX_CPU_TIMERS_TASK_WORK=y
CONFIG_POSIX_CPU_TIMERS_TASK_WORK=y
CONFIG_CONTEXT_TRACKING=y
CONFIG_CONTEXT_TRACKING_IDLE=y

#
# Timers subsystem
#
CONFIG_TICK_ONESHOT=y
CONFIG_NO_HZ_COMMON=y
# CONFIG_HZ_PERIODIC is not set
# CONFIG_NO_HZ_IDLE is not set
CONFIG_NO_HZ_FULL=y
CONFIG_CONTEXT_TRACKING_USER=y
# CONFIG_CONTEXT_TRACKING_USER_FORCE is not set
# CONFIG_NO_HZ is not set
CONFIG_HIGH_RES_TIMERS=y
CONFIG_CLOCKSOURCE_WATCHDOG_MAX_SKEW_US=100
# end of Timers subsystem

CONFIG_BPF=y
CONFIG_HAVE_EBPF_JIT=y
CONFIG_ARCH_WANT_DEFAULT_BPF_JIT=y

#
# BPF subsystem
#
CONFIG_BPF_SYSCALL=y
# CONFIG_BPF_JIT is not set
# CONFIG_BPF_UNPRIV_DEFAULT_OFF is not set
CONFIG_USERMODE_DRIVER=y
# CONFIG_BPF_PRELOAD is not set
# end of BPF subsystem

CONFIG_PREEM

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

2022-11-25 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
#418 FILE: utilities/ovn-nbctl.c:275:
  mirror-add NAME TYPE INDEX FILTER IP\n\

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

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

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

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

Lines checked: 837, 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 v16 3/3] OVN Remote Port Mirroring: controller changes to create ovs mirrors

2022-11-25 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 
---
 NEWS|   1 +
 controller/automake.mk  |   4 +-
 controller/mirror.c | 402 
 controller/mirror.h |  33 +++
 controller/ovn-controller.c |  54 ++--
 tests/ovn.at| 514 
 6 files changed, 985 insertions(+), 23 deletions(-)
 create mode 100644 controller/mirror.c
 create mode 100644 controller/mirror.h

diff --git a/NEWS b/NEWS
index 6c4573b50..dbffcac0f 100644
--- a/NEWS
+++ b/NEWS
@@ -27,6 +27,7 @@ OVN v22.09.0 - 16 Sep 2022
 any of LR's LRP IP, there is no need to create SNAT entry.  Now such
 traffic destined to LRP IP is not dropped.
   - Bump python version required for building OVN to 3.6.
+  - Added Support for Remote Port Mirroring.
 
 OVN v22.06.0 - 03 Jun 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..b947aeb5f
--- /dev/null
+++ b/controller/mirror.c
@@ -0,0 +1,402 @@
+/* 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 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, &ovsrec_bridge_col_mirrors);
+
+ovsdb_idl_add_table(ovs_idl, &ovsrec_table_mirror);
+ovsdb_

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

2022-11-25 Thread Abhiram R N

Changes which syncs the NB port mirrors with SB port mirrors.
Also 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 
---
 northd/en-northd.c   |   4 +
 northd/inc-proc-northd.c |   4 +
 northd/northd.c  | 172 +++
 northd/northd.h  |   2 +
 tests/ovn-northd.at  | 102 +++
 5 files changed, 284 insertions(+)

diff --git a/northd/en-northd.c b/northd/en-northd.c
index 93891b0b7..66ecc6573 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -78,6 +78,8 @@ void en_northd_run(struct engine_node *node, void *data)
 EN_OVSDB_GET(engine_get_input("NB_acl", node));
 input_data.nbrec_static_mac_binding_table =
 EN_OVSDB_GET(engine_get_input("NB_static_mac_binding", 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));
@@ -109,6 +111,8 @@ void en_northd_run(struct engine_node *node, void *data)
 EN_OVSDB_GET(engine_get_input("SB_chassis_private", node));
 input_data.sbrec_static_mac_binding_table =
 EN_OVSDB_GET(engine_get_input("SB_static_mac_binding", node));
+input_data.sbrec_mirror_table =
+EN_OVSDB_GET(engine_get_input("SB_mirror", node));
 
 northd_run(&input_data, data,
eng_ctx->ovnnb_idl_txn,
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 73f230b2c..7b7b250f3 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") \
@@ -94,6 +95,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") \
@@ -176,6 +178,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
 engine_add_input(&en_northd, &en_nb_acl, NULL);
 engine_add_input(&en_northd, &en_nb_logical_router, NULL);
 engine_add_input(&en_northd, &en_nb_qos, NULL);
+engine_add_input(&en_northd, &en_nb_mirror, NULL);
 engine_add_input(&en_northd, &en_nb_meter, NULL);
 engine_add_input(&en_northd, &en_nb_meter_band, NULL);
 engine_add_input(&en_northd, &en_nb_logical_router_port, NULL);
@@ -197,6 +200,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
 engine_add_input(&en_northd, &en_sb_encap, NULL);
 engine_add_input(&en_northd, &en_sb_port_group, NULL);
 engine_add_input(&en_northd, &en_sb_logical_dp_group, NULL);
+engine_add_input(&en_northd, &en_sb_mirror, NULL);
 engine_add_input(&en_northd, &en_sb_meter, NULL);
 engine_add_input(&en_northd, &en_sb_meter_band, NULL);
 engine_add_input(&en_northd, &en_sb_datapath_binding, NULL);
diff --git a/northd/northd.c b/northd/northd.c
index 040f46e1a..16739983c 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3239,6 +3239,89 @@ ovn_port_update_sbrec_chassis(
 free(requested_chassis_sb);
 }
 
+static void
+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 struct sbrec_mirror *sb_mirror;
+SBREC_MIRROR_TABLE_FOR_EACH (sb_mirror,
+ input_data->sbrec_mirror_table) {
+if (!strcmp(sb_mirror->name,
+op->nbsp->mirror_rules[i]->name)) {
+/* Add the value to SB */
+sbrec_port_binding_update_mirror_rules_addvalue(op->sb,
+sb_mirror);
+}
+}
+}
+}
+
+static void
+sbrec_port_binding_update_mirror_rules(struct northd_input *input_data,
+   const struct ovn_port *op)
+{
+size_t i = 0;
+if (op->sb->n_mirror_rules > op->nbsp->n_mirror_rules) {
+/* Needs deletion in SB */
+struct shash nb_mirror_rules = SHASH_INITIALIZER(&nb_mirror_rules);
+for (i = 0; i < op->nbsp->n_mirror_rules; i++) {
+shash_add(&nb_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(&nb_mirror_rules,
+   op->sb->mirror_rules[i]->name)) {
+

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

2022-11-25 Thread Abhiram R N

In order to support Remote Port Mirroring
added the required schemas in NB and SB.
Also, nbctl.c and sbctl.c changes are added.
Futher added test case to test nbctl commands.

Co-authored-by: Veda Barrenkala 
Signed-off-by: Veda Barrenkala 
Signed-off-by: Abhiram R N 
---
 ovn-nb.ovsschema  |  31 +++-
 ovn-nb.xml|  63 
 ovn-sb.ovsschema  |  26 ++-
 ovn-sb.xml|  50 ++
 tests/ovn-nbctl.at| 120 ++
 utilities/ovn-nbctl.c | 357 ++
 utilities/ovn-sbctl.c |   4 +
 7 files changed, 647 insertions(+), 4 deletions(-)

diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
index 174364c8b..01de55222 100644
--- a/ovn-nb.ovsschema
+++ b/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
 "name": "OVN_Northbound",
-"version": "6.3.0",
-"cksum": "4042813038 31869",
+"version": "6.4.0",
+"cksum": "589874483 33352",
 "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,28 @@
 "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",
+ "both"]]}}},
+"sink":{"type": "string"},
+"type": {"type": {"key": {"type": "string",
+"enum": ["set", ["gre",
+ "erspan"]]}}},
+"index": {"type": "integer"},
+"src": {"type": {"key": {"type": "uuid",
+   "refTable": "Logical_Switch_Port",
+   "refType": "weak"},
+   "min": 0,
+   "max": "unlimited"}},
+"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 7f207a413..671692b49 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.
@@ -2527,6 +2532,64 @@
 
   
 
+  
+
+  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 / both
+to-lport - to mirror packets coming into logical port
+from-lport - to mirror packets going out of logical port
+both - to mirror packets coming into and 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
+  
+
+
+
+  
+The value of this field represents a list of source ports for the
+mirror. Please see the  table.
+  
+
+
+
+  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

Re: [ovs-dev] [PATCH] ofp-msgs: Fix comment typo.

2022-11-25 Thread Adrian Moreno




On 5/24/22 15:04, mit...@outlook.com wrote:

From: Lin Huang 

Fix comment typo.

Signed-off-by: Lin Huang 
---
  lib/ofp-msgs.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ofp-msgs.c b/lib/ofp-msgs.c
index 93aa81297..fdb898064 100644
--- a/lib/ofp-msgs.c
+++ b/lib/ofp-msgs.c
@@ -148,7 +148,7 @@ struct raw_instance {
  /* Information about a particular 'enum ofpraw'. */
  struct raw_info {
  /* All possible instantiations of this OFPRAW_* into OpenFlow headers. */
-struct raw_instance *instances; /* min_version - max_version + 1 elems. */
+struct raw_instance *instances; /* max_version - min_version + 1 elems. */
  uint8_t min_version;
  uint8_t max_version;
  



Good catch!
Acked-by: Adrian Moreno 

--
Adrián Moreno

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

Re: [ovs-dev] [v5] odp-execute: Add ISA implementation of set_masked IPv6 action

2022-11-25 Thread Ilya Maximets

On 11/25/22 17:23, Emma Finn wrote:
> This commit adds support for the AVX512 implementation of the
> ipv6_set_addrs action as well as an AVX512 implementation of
> updating the L4 checksums.
> 
> Signed-off-by: Emma Finn 
> 
> ---
> v5:
>   - Fixed load for ip6 src and dst mask for checksum check.
> v4:
>   - Reworked and moved check for checksum outside loop.
>   - Code cleanup based on review from Eelco.
> v3:
>   - Added a runtime check for AVX512 vbmi.
> v2:
>   - Added check for availbility of s6_addr32 field of struct in6_addr.
>   - Fixed network headers for freebsd builds.
> ---
> ---
>  lib/odp-execute-avx512.c  | 204 ++
>  lib/odp-execute-private.c |  17 
>  lib/odp-execute-private.h |   1 +
>  3 files changed, 222 insertions(+)

Hi, Emma.  Thanks for the patch!
I didn't review the actual AVX512 code, but I have a couple of
questions and nits inline.

Best regards, Ilya Maximets.

> 
> diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
> index 6c7713251..df0b31ffd 100644
> --- a/lib/odp-execute-avx512.c
> +++ b/lib/odp-execute-avx512.c
> @@ -20,6 +20,9 @@
>  
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
>  
>  #include "csum.h"
>  #include "dp-packet.h"
> @@ -75,6 +78,26 @@ BUILD_ASSERT_DECL(offsetof(struct ovs_key_ipv4, ipv4_tos) +
>MEMBER_SIZEOF(struct ovs_key_ipv4, ipv4_tos) ==
>offsetof(struct ovs_key_ipv4, ipv4_ttl));
>  
> +BUILD_ASSERT_DECL(offsetof(struct ovs_key_ipv6, ipv6_src) +
> +  MEMBER_SIZEOF(struct ovs_key_ipv6, ipv6_src) ==
> +  offsetof(struct ovs_key_ipv6, ipv6_dst));
> +
> +BUILD_ASSERT_DECL(offsetof(struct ovs_key_ipv6, ipv6_dst) +
> +  MEMBER_SIZEOF(struct ovs_key_ipv6, ipv6_dst) ==
> +  offsetof(struct ovs_key_ipv6, ipv6_label));
> +
> +BUILD_ASSERT_DECL(offsetof(struct ovs_key_ipv6, ipv6_label) +
> +  MEMBER_SIZEOF(struct ovs_key_ipv6, ipv6_label) ==
> +  offsetof(struct ovs_key_ipv6, ipv6_proto));
> +
> +BUILD_ASSERT_DECL(offsetof(struct ovs_key_ipv6, ipv6_proto) +
> +  MEMBER_SIZEOF(struct ovs_key_ipv6, ipv6_proto) ==
> +  offsetof(struct ovs_key_ipv6, ipv6_tclass));
> +
> +BUILD_ASSERT_DECL(offsetof(struct ovs_key_ipv6, ipv6_tclass) +
> +  MEMBER_SIZEOF(struct ovs_key_ipv6, ipv6_tclass) ==
> +  offsetof(struct ovs_key_ipv6, ipv6_hlimit));
> +
>  /* Array of callback functions, one for each masked operation. */
>  odp_execute_action_cb impl_set_masked_funcs[__OVS_KEY_ATTR_MAX];
>  
> @@ -483,6 +506,180 @@ action_avx512_ipv4_set_addrs(struct dp_packet_batch 
> *batch,
>  }
>  }
>  
> +#if HAVE_AVX512VBMI
> +static inline uint16_t ALWAYS_INLINE
> +__attribute__((__target__("avx512vbmi")))
> +avx512_ipv6_sum_header(__m512i ip6_header)
> +{
> +__m256i v_zeros = _mm256_setzero_si256();
> +__m512i v_shuf_src_dst = _mm512_setr_epi64(0x01, 0x02, 0x03, 0x04,
> +   0xFF, 0xFF, 0xFF, 0xFF);
> +
> +/* Shuffle ip6 src and dst to beginning of register. */
> +__m512i v_ip6_hdr_shuf = _mm512_permutexvar_epi64(v_shuf_src_dst,
> +  ip6_header);
> +
> +/* Extract ip6 src and dst into smaller 256-bit wide register. */
> +__m256i v_ip6_src_dst = _mm512_extracti64x4_epi64(v_ip6_hdr_shuf, 0);
> +
> +/* These two shuffle masks, v_swap16a and v_swap16b, are to shuffle the
> + * src and dst fields and add padding after each 16-bit value for the
> + * following carry over addition. */
> +__m256i v_swap16a = _mm256_setr_epi16(0x0100, 0x, 0x0302, 0x,
> +  0x0504, 0x, 0x0706, 0x,
> +  0x0100, 0x, 0x0302, 0x,
> +  0x0504, 0x, 0x0706, 0x);
> +__m256i v_swap16b = _mm256_setr_epi16(0x0908, 0x, 0x0B0A, 0x,
> +  0x0D0C, 0x, 0x0F0E, 0x,
> +  0x0908, 0x, 0x0B0A, 0x,
> +  0x0D0C, 0x, 0x0F0E, 0x);
> +__m256i v_shuf_old1 = _mm256_shuffle_epi8(v_ip6_src_dst, v_swap16a);
> +__m256i v_shuf_old2 = _mm256_shuffle_epi8(v_ip6_src_dst, v_swap16b);
> +
> +/* Add each part of the old and new headers together. */
> +__m256i v_delta = _mm256_add_epi32(v_shuf_old1, v_shuf_old2);
> +
> +/* Perform horizontal add to go from 8x32-bits to 2x32-bits. */
> +v_delta = _mm256_hadd_epi32(v_delta, v_zeros);
> +v_delta = _mm256_hadd_epi32(v_delta, v_zeros);
> +
> +/* Shuffle 32-bit value from 3rd lane into first lane for final
> + * horizontal add. */
> +__m256i v_swap32a = _mm256_setr_epi32(0x0, 0x4, 0xF, 0xF,
> +  0xF, 0xF, 0xF, 0xF);
> +
> +

Re: [ovs-dev] [PATCH v1] ovs-ctl: Allow inclusion of hugepages in coredumps

2022-11-25 Thread Adrian Moreno


Hi Mike,

Sorry it took that long to review this patch.

On 3/25/22 23:17, Mike Pattrick wrote:

Add new option --dump-hugepages option in ovs-ctl to enable the addition
of hugepages in the core dump filter.

Signed-off-by: Mike Pattrick 
---
  NEWS |  4 
  utilities/ovs-ctl.in | 15 +++
  2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/NEWS b/NEWS
index 8fa57836a..7af60dce3 100644
--- a/NEWS
+++ b/NEWS
@@ -3,6 +3,10 @@ Post-v2.17.0
 - OVSDB:
   * 'relay' service model now supports transaction history, i.e. honors the
 'last-txn-id' field in 'monitor_cond_since' requests from clients.
+   - ovs-ctl:
+ * New option '--dump-hugepages' to include hugepages in core dumps. This
+   can assist with postmortem analysis involving DPDK, but may also produce
+   significantly larger core dump files.
  


I'm afraid this part needs rebasing.

  
  v2.17.0 - 17 Feb 2022

diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
index e6e07f476..8f900314b 100644
--- a/utilities/ovs-ctl.in
+++ b/utilities/ovs-ctl.in
@@ -103,8 +103,13 @@ set_system_ids () {
  action "Configuring Open vSwitch system IDs" "$@" $extra_ids
  }
  
-check_force_cores () {

-if test X"$FORCE_COREFILES" = Xyes; then
+check_core_config () {
+if test X"$DUMP_HUGEPAGES" = Xyes; then
+echo 0x3f > /proc/self/coredump_filter
+if test X"$FORCE_COREFILES" = Xyes; then
+ulimit -c unlimited
+fi
+elif test X"$FORCE_COREFILES" = Xyes; then
  ulimit -c 67108864
  fi
  }
@@ -116,7 +121,7 @@ del_transient_ports () {
  }
  
  do_start_ovsdb () {

-check_force_cores
+check_core_config
  
  if daemon_is_running ovsdb-server; then

  log_success_msg "ovsdb-server is already running"
@@ -193,7 +198,7 @@ add_managers () {
  }
  
  do_start_forwarding () {

-check_force_cores
+check_core_config
  
  insert_mod_if_required || return 1
  
@@ -330,6 +335,7 @@ set_defaults () {
  
  DAEMON_CWD=/

  FORCE_COREFILES=yes
+DUMP_HUGEPAGES=no
  MLOCKALL=yes
  SELF_CONFINEMENT=yes
  MONITOR=yes
@@ -419,6 +425,7 @@ Other important options for "start", "restart" and 
"force-reload-kmod":
  Less important options for "start", "restart" and "force-reload-kmod":
--daemon-cwd=DIR   set working dir for OVS daemons (default: 
$DAEMON_CWD)
--no-force-corefiles   do not force on core dumps for OVS daemons
+  --dump-hugepages   include hugepages in coredumps
--no-mlockall  do not lock all of ovs-vswitchd into memory
--ovsdb-server-priority=NICE   set ovsdb-server's niceness (default: 
$OVSDB_SERVER_PRIORITY)
--ovsdb-server-options=OPTIONS additional options for ovsdb-server 
(example: '-vconsole:dbg -vfile:dbg')



Tested locally and verified that with the option hugepages appear in coredumps.
Apart from the need to rebase the NEWS, the patch looks good to me.

Acked-by: Adrian Moreno 

--
Adrián Moreno

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

Re: [ovs-dev] [PATCH/RFC net-next] tc: allow drivers to accept gact with PIPE when offloading

2022-11-25 Thread Jamal Hadi Salim

Hi Tianyu,

On Thu, Nov 24, 2022 at 10:10 PM Tianyu Yuan 
wrote:

> On Fri, Nov 25, 2022 at 10:21 AM  Jamal Hadi Salim 
> wrote:
>
> >
> > I am not sure if the mlx5 changes will work since  they both seem to be
> calling
> > mlx5e_tc_act_get() which expects the act->id to exist in tc_acts_xxx
> tables,
> > meaning mlx5e_tc_act_get() will always return you NULL  and that check is
> > hit before you check for ACT_PIPE.
>

[..]

>
> Thanks Jamal for your review.
>
> About mlx5e_tc_act_get(), I'll later add PIPE action in tc_acts_nic so
> that mlx5e_tc_act_get() will return the right
> act_id.
>

Just noticed Vlad posted a patch for it ;->
I only looked at that change because i have that hardware and wanted to try
it out.
You are Ccing all the driver stakeholders hopefully they can double check.

> In driver we choose just ignore this gact with ACT_PIPE, so after parsing
> the filter(rule) from kernel, the remaining
> actions are just like what they used to be without changes in this patch.
> So the flow could be processed as before.
>
>
So in the simple case it is as if no action was specified?

> The connection between POLICE and ACT_PIPE may exist in userspace (e.g.
> ovs), we could put a gact (PIPE) at the
> beginning place in each tc filter. We will also have an OVS patch for this
> propose.
>
>
makes sense.

> I'm not very clear with your last case, but in expectation, the once the
> traffic is offloaded in h/w tc datapath, the
> stats will be updated by the flower stats from hardware. And when the
> traffic is using s/w tc datapath, the stats are
> from software.
>

My initial thought was you want to cover two scenarios:
1) pipe followed by police.
2) pipe alone

In both cases pipe serves as a placeholder for stats counters. That these
counters
come from hardware and will occasionally be updated to tc by the driver.
i.e if i get/dump the filter or pipe action stats i can see the hw count.
Am i correct? If the
answer is yes, then would i see the hw stats and not the sw stat updates?
Maybe if you have a filter dump in your test environment that you can show
it will
help satisfy my curiosity.

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

[ovs-dev] [v5] odp-execute: Add ISA implementation of set_masked IPv6 action

2022-11-25 Thread Emma Finn

This commit adds support for the AVX512 implementation of the
ipv6_set_addrs action as well as an AVX512 implementation of
updating the L4 checksums.

Signed-off-by: Emma Finn 

---
v5:
  - Fixed load for ip6 src and dst mask for checksum check.
v4:
  - Reworked and moved check for checksum outside loop.
  - Code cleanup based on review from Eelco.
v3:
  - Added a runtime check for AVX512 vbmi.
v2:
  - Added check for availbility of s6_addr32 field of struct in6_addr.
  - Fixed network headers for freebsd builds.
---
---
 lib/odp-execute-avx512.c  | 204 ++
 lib/odp-execute-private.c |  17 
 lib/odp-execute-private.h |   1 +
 3 files changed, 222 insertions(+)

diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
index 6c7713251..df0b31ffd 100644
--- a/lib/odp-execute-avx512.c
+++ b/lib/odp-execute-avx512.c
@@ -20,6 +20,9 @@
 
 #include 
 #include 
+#include 
+#include 
+#include 
 
 #include "csum.h"
 #include "dp-packet.h"
@@ -75,6 +78,26 @@ BUILD_ASSERT_DECL(offsetof(struct ovs_key_ipv4, ipv4_tos) +
   MEMBER_SIZEOF(struct ovs_key_ipv4, ipv4_tos) ==
   offsetof(struct ovs_key_ipv4, ipv4_ttl));
 
+BUILD_ASSERT_DECL(offsetof(struct ovs_key_ipv6, ipv6_src) +
+  MEMBER_SIZEOF(struct ovs_key_ipv6, ipv6_src) ==
+  offsetof(struct ovs_key_ipv6, ipv6_dst));
+
+BUILD_ASSERT_DECL(offsetof(struct ovs_key_ipv6, ipv6_dst) +
+  MEMBER_SIZEOF(struct ovs_key_ipv6, ipv6_dst) ==
+  offsetof(struct ovs_key_ipv6, ipv6_label));
+
+BUILD_ASSERT_DECL(offsetof(struct ovs_key_ipv6, ipv6_label) +
+  MEMBER_SIZEOF(struct ovs_key_ipv6, ipv6_label) ==
+  offsetof(struct ovs_key_ipv6, ipv6_proto));
+
+BUILD_ASSERT_DECL(offsetof(struct ovs_key_ipv6, ipv6_proto) +
+  MEMBER_SIZEOF(struct ovs_key_ipv6, ipv6_proto) ==
+  offsetof(struct ovs_key_ipv6, ipv6_tclass));
+
+BUILD_ASSERT_DECL(offsetof(struct ovs_key_ipv6, ipv6_tclass) +
+  MEMBER_SIZEOF(struct ovs_key_ipv6, ipv6_tclass) ==
+  offsetof(struct ovs_key_ipv6, ipv6_hlimit));
+
 /* Array of callback functions, one for each masked operation. */
 odp_execute_action_cb impl_set_masked_funcs[__OVS_KEY_ATTR_MAX];
 
@@ -483,6 +506,180 @@ action_avx512_ipv4_set_addrs(struct dp_packet_batch 
*batch,
 }
 }
 
+#if HAVE_AVX512VBMI
+static inline uint16_t ALWAYS_INLINE
+__attribute__((__target__("avx512vbmi")))
+avx512_ipv6_sum_header(__m512i ip6_header)
+{
+__m256i v_zeros = _mm256_setzero_si256();
+__m512i v_shuf_src_dst = _mm512_setr_epi64(0x01, 0x02, 0x03, 0x04,
+   0xFF, 0xFF, 0xFF, 0xFF);
+
+/* Shuffle ip6 src and dst to beginning of register. */
+__m512i v_ip6_hdr_shuf = _mm512_permutexvar_epi64(v_shuf_src_dst,
+  ip6_header);
+
+/* Extract ip6 src and dst into smaller 256-bit wide register. */
+__m256i v_ip6_src_dst = _mm512_extracti64x4_epi64(v_ip6_hdr_shuf, 0);
+
+/* These two shuffle masks, v_swap16a and v_swap16b, are to shuffle the
+ * src and dst fields and add padding after each 16-bit value for the
+ * following carry over addition. */
+__m256i v_swap16a = _mm256_setr_epi16(0x0100, 0x, 0x0302, 0x,
+  0x0504, 0x, 0x0706, 0x,
+  0x0100, 0x, 0x0302, 0x,
+  0x0504, 0x, 0x0706, 0x);
+__m256i v_swap16b = _mm256_setr_epi16(0x0908, 0x, 0x0B0A, 0x,
+  0x0D0C, 0x, 0x0F0E, 0x,
+  0x0908, 0x, 0x0B0A, 0x,
+  0x0D0C, 0x, 0x0F0E, 0x);
+__m256i v_shuf_old1 = _mm256_shuffle_epi8(v_ip6_src_dst, v_swap16a);
+__m256i v_shuf_old2 = _mm256_shuffle_epi8(v_ip6_src_dst, v_swap16b);
+
+/* Add each part of the old and new headers together. */
+__m256i v_delta = _mm256_add_epi32(v_shuf_old1, v_shuf_old2);
+
+/* Perform horizontal add to go from 8x32-bits to 2x32-bits. */
+v_delta = _mm256_hadd_epi32(v_delta, v_zeros);
+v_delta = _mm256_hadd_epi32(v_delta, v_zeros);
+
+/* Shuffle 32-bit value from 3rd lane into first lane for final
+ * horizontal add. */
+__m256i v_swap32a = _mm256_setr_epi32(0x0, 0x4, 0xF, 0xF,
+  0xF, 0xF, 0xF, 0xF);
+
+v_delta = _mm256_permutexvar_epi32(v_swap32a, v_delta);
+v_delta = _mm256_hadd_epi32(v_delta, v_zeros);
+v_delta = _mm256_hadd_epi16(v_delta, v_zeros);
+
+/* Extract delta value. */
+return _mm256_extract_epi16(v_delta, 0);
+}
+
+static inline uint16_t ALWAYS_INLINE
+__attribute__((__target__("avx512vbmi")))
+avx512_ipv6_addr_csum_delta(__m512i old_header, __m512i new_header)
+{
+uint16_t old_

Re: [ovs-dev] [RFC net-next 1/6] openvswitch: exclude kernel flow key from upcalls

2022-11-25 Thread Ilya Maximets

On 11/25/22 16:29, Adrian Moreno wrote:
> 
> 
> On 11/23/22 22:22, Ilya Maximets wrote:
>> On 11/22/22 15:03, Aaron Conole wrote:
>>> When processing upcall commands, two groups of data are available to
>>> userspace for processing: the actual packet data and the kernel
>>> sw flow key data.  The inclusion of the flow key allows the userspace
>>> avoid running through the dissection again.
>>>
>>> However, the userspace can choose to ignore the flow key data, as is
>>> the case in some ovs-vswitchd upcall processing.  For these messages,
>>> having the flow key data merely adds additional data to the upcall
>>> pipeline without any actual gain.  Userspace simply throws the data
>>> away anyway.
>>
>> Hi, Aaron.  While it's true that OVS in userpsace is re-parsing the
>> packet from scratch and using the newly parsed key for the OpenFlow
>> translation, the kernel-porvided key is still used in a few important
>> places.  Mainly for the compatibility checking.  The use is described
>> here in more details:
>>    https://docs.kernel.org/networking/openvswitch.html#flow-key-compatibility
>>
>> We need to compare the key generated in userspace with the key
>> generated by the kernel to know if it's safe to install the new flow
>> to the kernel, i.e. if the kernel and OVS userpsace are parsing the
>> packet in the same way.
>>
> 
> Hi Ilya,
> 
> Do we need to do that for every packet?
> Could we send a bitmask of supported fields to userspace at feature
> negotiation and let OVS slowpath flows that it knows the kernel won't
> be able to handle properly?

It's not that simple, because supported fields in a packet depend
on previous fields in that same packet.  For example, parsing TCP
header is generally supported, but it won't be parsed for IPv6
fragments (even the first one), number of vlan headers will affect
the parsing as we do not parse deeper than 2 vlan headers, etc.
So, I'm afraid we have to have a per-packet information, unless we
can somehow probe all the possible valid combinations of packet
headers.

> 
> 
>> On the other hand, OVS today doesn't check the data, it only checks
>> which fields are present.  So, if we can generate and pass the bitmap
>> of fields present in the key or something similar without sending the
>> full key, that might still save some CPU cycles and memory in the
>> socket buffer while preserving the ability to check for forward and
>> backward compatibility.  What do you think?
>>
>>
>> The rest of the patch set seems useful even without patch #1 though.
>>
>> Nit: This patch #1 should probably be merged with the patch #6 and be
>> at the end of a patch set, so the selftest and the main code are updated
>> at the same time.
>>
>> Best regards, Ilya Maximets.
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
> 
> Thanks

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

Re: [ovs-dev] [PATCH] dpif-netdev: Use unmasked key when adding datapath flows.

2022-11-25 Thread Adrian Moreno




On 10/18/22 18:42, Eelco Chaudron wrote:

The datapath supports installing wider flows, and OVS relies on
this behavior. For example if ipv4(src=1.1.1.1/192.0.0.0,
dst=1.1.1.2/192.0.0.0) exists, a wider flow (smaller mask) of
ipv4(src=192.1.1.1/128.0.0.0,dst=192.1.1.2/128.0.0.0) is allowed
to be added.

However, if we try to add a wildcard rule, the installation fails:

# ovs-appctl dpctl/add-flow system@myDP "in_port(1),eth_type(0x0800), \
   ipv4(src=1.1.1.1/192.0.0.0,dst=1.1.1.2/192.0.0.0,frag=no)" 2
# ovs-appctl dpctl/add-flow system@myDP "in_port(1),eth_type(0x0800), \
   ipv4(src=192.1.1.1/0.0.0.0,dst=49.1.1.2/0.0.0.0,frag=no)" 2
ovs-vswitchd: updating flow table (File exists)

The reason is that the key used to determine if the flow is already
present in the system uses the original key ANDed with the mask.
This results in the IP address not being part of the (miniflow) key,
i.e., being substituted with an all-zero value. When doing the actual
lookup, this results in the key wrongfully matching the first flow,
and therefore the flow does not get installed. The solution is to use
the unmasked key for the existence check, the same way this is handled
in the userspace datapath.

Signed-off-by: Eelco Chaudron 
---
  lib/dpif-netdev.c|   33 +
  tests/dpif-netdev.at |   14 ++
  2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index a45b46014..daa00aa2f 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3321,6 +3321,28 @@ netdev_flow_key_init_masked(struct netdev_flow_key *dst,
  (dst_u64 - miniflow_get_values(&dst->mf)) * 8);
  }
  
+/* Initializes 'dst' as a copy of 'flow'. */


nit: s/dst/key/


+static inline void
+netdev_flow_key_init(struct netdev_flow_key *key,
+ const struct flow *flow)
+{
+uint64_t *dst = miniflow_values(&key->mf);
+uint32_t hash = 0;
+uint64_t value;
+
+miniflow_map_init(&key->mf, flow);
+miniflow_init(&key->mf, flow);
+
+size_t n = dst - miniflow_get_values(&key->mf);
+
+FLOW_FOR_EACH_IN_MAPS (value, flow, key->mf.map) {
+hash = hash_add64(hash, value);
+}
+
+key->hash = hash_finish(hash, n * 8);
+key->len = netdev_flow_key_size(n);
+}
+
  static inline void
  emc_change_entry(struct emc_entry *ce, struct dp_netdev_flow *flow,
   const struct netdev_flow_key *key)
@@ -4195,7 +4217,7 @@ static int
  dpif_netdev_flow_put(struct dpif *dpif, const struct dpif_flow_put *put)
  {
  struct dp_netdev *dp = get_dp_netdev(dpif);
-struct netdev_flow_key key, mask;
+struct netdev_flow_key key;
  struct dp_netdev_pmd_thread *pmd;
  struct match match;
  ovs_u128 ufid;
@@ -4244,9 +4266,12 @@ dpif_netdev_flow_put(struct dpif *dpif, const struct 
dpif_flow_put *put)
  
  /* Must produce a netdev_flow_key for lookup.

   * Use the same method as employed to create the key when adding
- * the flow to the dplcs to make sure they match. */
-netdev_flow_mask_init(&mask, &match);
-netdev_flow_key_init_masked(&key, &match.flow, &mask);
+ * the flow to the dplcs to make sure they match.
+ * We need to put in the unmasked key as flow_put_on_pmd() will first try
+ * to see if an entry exists doing a packet type lookup. As masked-out
+ * fields are interpreted as zeros, they could falsely match a wider IP
+ * address mask. Installation of the flow will use the match variable. */
+netdev_flow_key_init(&key, &match.flow);
  
  if (put->pmd_id == PMD_ID_NULL) {

  if (cmap_count(&dp->poll_threads) == 0) {
diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
index 3179e1645..32054c52e 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -636,6 +636,20 @@ OVS_VSWITCHD_STOP(["/flow: in_port is not an exact match/d
  /failed to put/d"])
  AT_CLEANUP
  
+AT_SETUP([dpif-netdev - check dpctl/add-flow wider ip match])

+OVS_VSWITCHD_START(
+  [add-port br0 p1 \
+   -- set interface p1 type=dummy options:pstream=punix:$OVS_RUNDIR/p0.sock \
+   -- set bridge br0 datapath-type=dummy])
+
+AT_CHECK([ovs-appctl revalidator/pause])
+AT_CHECK([ovs-appctl dpctl/add-flow 
"in_port(1),eth_type(0x0800),ipv4(src=0.0.0.0/192.0.0.0,dst=0.0.0.0/192.0.0.0,frag=no)" 
"3"])
+AT_CHECK([ovs-appctl dpctl/add-flow 
"in_port(1),eth_type(0x0800),ipv4(src=192.1.1.1/0.0.0.0,dst=49.1.1.1/0.0.0.0,frag=no)" 
"3"])
+AT_CHECK([ovs-appctl revalidator/resume])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
  # SEND_UDP_PKTS([p_name], [p_ofport])
  #
  # Sends 128 packets to port 'p_name' with different UDP destination ports.

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



--
Adrián Moreno

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

Re: [ovs-dev] [v4] odp-execute: Add ISA implementation of set_masked IPv6 action

2022-11-25 Thread Eelco Chaudron




On 25 Nov 2022, at 16:20, Finn, Emma wrote:

>> -Original Message-
>> From: Eelco Chaudron 
>> Sent: Friday 25 November 2022 12:26
>> To: Finn, Emma 
>> Cc: d...@openvswitch.org; Van Haaren, Harry
>> 
>> Subject: Re: [v4] odp-execute: Add ISA implementation of set_masked IPv6
>> action
>>
>>
>>
>> On 24 Nov 2022, at 10:30, Emma Finn wrote:
>>
>>> This commit adds support for the AVX512 implementation of the
>>> ipv6_set_addrs action as well as an AVX512 implementation of updating
>>> the L4 checksums.
>>>
>>> Signed-off-by: Emma Finn 
>>
>> Thanks Emma for the v4, I have one question and a couple of style issues. To
>> speed things up I just provide the diff for the style issues.
>>
>> I was not able to do any actual testing, as my system did not have the
>> avx512vbmi extension :(
>>
>> Cheers,
>>
>> Eelco
>>
>>> ---
>>
>> Style issues diff:
>>
>> diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c index
>> 82ff7e647..f798d6708 100644
>> --- a/lib/odp-execute-avx512.c
>> +++ b/lib/odp-execute-avx512.c
>> @@ -20,9 +20,9 @@
>>
>>  #include 
>>  #include 
>> -#include 
>>  #include 
>>  #include 
>> +#include 
>>
> For FreeBSD, network headers must be included in a certain order.
> See this commit for details:
> b2befd5bb2db ("sparse: Add guards to prevent FreeBSD-incompatible 
> #include order.")
> So these need to stay in this order.

Oops, yes I did not test with FreeBSD (or windows), I need to figure out how to 
do that in the future ;)

>>  #include "csum.h"
>>  #include "dp-packet.h"
>> @@ -547,8 +547,8 @@ avx512_ipv6_sum_header(__m512i ip6_header)
>>   * horizontal add. */
>>  __m256i v_swap32a = _mm256_setr_epi32(0x0, 0x4, 0xF, 0xF,
>>0xF, 0xF, 0xF, 0xF);
>> -v_delta = _mm256_permutexvar_epi32(v_swap32a, v_delta);
>>
>> +v_delta = _mm256_permutexvar_epi32(v_swap32a, v_delta);
>>  v_delta = _mm256_hadd_epi32(v_delta, v_zeros);
>>  v_delta = _mm256_hadd_epi16(v_delta, v_zeros);
>>
>> @@ -562,7 +562,7 @@ avx512_ipv6_addr_csum_delta(__m512i old_header,
>> __m512i new_header)  {
>>  uint16_t old_delta = avx512_ipv6_sum_header(old_header);
>>  uint16_t new_delta = avx512_ipv6_sum_header(new_header);
>> -uint32_t csum_delta = (uint16_t)~old_delta + new_delta;
>> +uint32_t csum_delta = ((uint16_t) ~old_delta) + new_delta;
>>
>>  return  ~csum_finish(csum_delta);
>>  }
>> @@ -606,15 +606,15 @@ action_avx512_ipv6_set_addrs(struct
>> dp_packet_batch *batch,
>>  __m512i v_key_shuf = _mm512_permutexvar_epi8(v_shuffle, v_key);
>>  __m512i v_mask_shuf = _mm512_permutexvar_epi8(v_shuffle, v_mask);
>>
>> -
>>  /* Set the v_zero register to all zero's. */
>>  const __m128i v_zeros = _mm_setzero_si128();
>> +
>>  /* Set the v_all_ones register to all one's. */
>>  const __m128i v_all_ones = _mm_cmpeq_epi16(v_zeros, v_zeros);
>>
>> -/* Load ip6 src and dst respectively into 128-bit wide registers. */
>> +/* Load ip6 src and dst masks respectively into 128-bit wide
>> + registers. */
>>  __m128i v_src = _mm_loadu_si128((void *) mask);
>> -__m128i v_dst = _mm_maskz_loadu_epi64(0xC,(void *) mask);
>> +__m128i v_dst = _mm_maskz_loadu_epi64(0xC, (void *) mask);
>>
>>  /* Perform a bitwise OR between src and dst registers. */
>>  __m128i v_or = _mm_or_si128(v_src, v_dst);
>>
>>> v4:
>>>   - Reworked and moved check for checksum outside loop.
>>>   - Code cleanup based on review from Eelco.
>>> v3:
>>>   - Added a runtime check for AVX512 vbmi.
>>> v2:
>>>   - Added check for availbility of s6_addr32 field of struct in6_addr.
>>>   - Fixed network headers for freebsd builds.
>>>
>>
>> 
>>
>>> +/* Load ip6 src and dst respectively into 128-bit wide registers. */
>>> +__m128i v_src = _mm_loadu_si128((void *) mask);
>>> +__m128i v_dst = _mm_maskz_loadu_epi64(0xC,(void *) mask);
>>
>> Guess it might be me, but I do not understand how
>> _mm_maskz_loadu_epi64() will load the dst from the mask.
>> Looking at the intrinsics guide it will only read the first two 64-bit 
>> values, but
>> mask points to src?
>>
>> Should we not just do the following here?
>>
>> +__m128i v_src = _mm_loadu_si128((void *) &mask->ipv6_src);
>> +__m128i v_dst = _mm_loadu_si128((void *) &mask->ipv6_dst);
>>
>
> Yes, good catch. The _maskz_load_ isn't correctly pointing to the dst values 
> from the mask.
>
> I will make all the changes above (except the include header comment) and 
> send v5 shortly.

Thanks, take your time and enjoy the weekend!

> 

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

Re: [ovs-dev] [PATCH ovn v5 0/2] Allow related traffic for LB

2022-11-25 Thread Numan Siddique

On Fri, Nov 25, 2022 at 4:53 AM Dumitru Ceara  wrote:
>
> On 11/22/22 16:03, Ales Musil wrote:
> > The related traffic wasn't correctly forwarded
> > through the LB, the main issue was that the
> > traffic was not NATted. This series allows
> > the NAT to be applied and the traffic should
> > arrive with correct addresses.
> > ---
> > v2: Add e2e test case.
> > v3: Rebase on top of main.
> > Address comments from Mark.
> > v4: Rebase on top of main.
> > v5: Add feature flag for backward compatibility.
>
> As discussed offline, we don't expect to backport this feature.  So v4
> should be reviewed instead.
>

Even if we are not backporting, shouldn't having the feature flag help
in upgrade scenarios ?
Particularly when CMS upgrades OVN central components first ?

Just thinking out loud to avoid any upgrade issues in the future.

Numan

> Thanks,
> Dumitru
>
> ___
> 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] ovn-util: Bump OVN internal minor version for new stages.

2022-11-25 Thread Numan Siddique

On Fri, Nov 25, 2022 at 6:18 AM Dumitru Ceara  wrote:
>
> A few logical pipeline stages have been added.  The internal minor
> version must be bumped every time that happens.
>
> Fixes: d3926b433e44 ("northd: rely on new actions for lb affinity")
> Signed-off-by: Dumitru Ceara 

Acked-by: Numan Siddique 

Numan

> ---
>  lib/ovn-util.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index 868472ace0..36ca96dbc4 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -817,7 +817,7 @@ ip_address_and_port_from_lb_key(const char *key, char 
> **ip_address,
>   *
>   * This value is also used to handle some backward compatibility during
>   * upgrading. It should never decrease or rewind. */
> -#define OVN_INTERNAL_MINOR_VER 5
> +#define OVN_INTERNAL_MINOR_VER 6
>
>  /* Returns the OVN version. The caller must free the returned value. */
>  char *
> --
> 2.31.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] [RFC net-next 1/6] openvswitch: exclude kernel flow key from upcalls

2022-11-25 Thread Adrian Moreno




On 11/23/22 22:22, Ilya Maximets wrote:

On 11/22/22 15:03, Aaron Conole wrote:

When processing upcall commands, two groups of data are available to
userspace for processing: the actual packet data and the kernel
sw flow key data.  The inclusion of the flow key allows the userspace
avoid running through the dissection again.

However, the userspace can choose to ignore the flow key data, as is
the case in some ovs-vswitchd upcall processing.  For these messages,
having the flow key data merely adds additional data to the upcall
pipeline without any actual gain.  Userspace simply throws the data
away anyway.


Hi, Aaron.  While it's true that OVS in userpsace is re-parsing the
packet from scratch and using the newly parsed key for the OpenFlow
translation, the kernel-porvided key is still used in a few important
places.  Mainly for the compatibility checking.  The use is described
here in more details:
   https://docs.kernel.org/networking/openvswitch.html#flow-key-compatibility

We need to compare the key generated in userspace with the key
generated by the kernel to know if it's safe to install the new flow
to the kernel, i.e. if the kernel and OVS userpsace are parsing the
packet in the same way.



Hi Ilya,

Do we need to do that for every packet?
Could we send a bitmask of supported fields to userspace at feature negotiation 
and let OVS slowpath flows that it knows the kernel won't be able to handle 
properly?




On the other hand, OVS today doesn't check the data, it only checks
which fields are present.  So, if we can generate and pass the bitmap
of fields present in the key or something similar without sending the
full key, that might still save some CPU cycles and memory in the
socket buffer while preserving the ability to check for forward and
backward compatibility.  What do you think?


The rest of the patch set seems useful even without patch #1 though.

Nit: This patch #1 should probably be merged with the patch #6 and be
at the end of a patch set, so the selftest and the main code are updated
at the same time.

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



Thanks
--
Adrián Moreno

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

Re: [ovs-dev] [v4] odp-execute: Add ISA implementation of set_masked IPv6 action

2022-11-25 Thread Finn, Emma




> -Original Message-
> From: Eelco Chaudron 
> Sent: Friday 25 November 2022 12:26
> To: Finn, Emma 
> Cc: d...@openvswitch.org; Van Haaren, Harry
> 
> Subject: Re: [v4] odp-execute: Add ISA implementation of set_masked IPv6
> action
> 
> 
> 
> On 24 Nov 2022, at 10:30, Emma Finn wrote:
> 
> > This commit adds support for the AVX512 implementation of the
> > ipv6_set_addrs action as well as an AVX512 implementation of updating
> > the L4 checksums.
> >
> > Signed-off-by: Emma Finn 
> 
> Thanks Emma for the v4, I have one question and a couple of style issues. To
> speed things up I just provide the diff for the style issues.
> 
> I was not able to do any actual testing, as my system did not have the
> avx512vbmi extension :(
> 
> Cheers,
> 
> Eelco
> 
> > ---
> 
> Style issues diff:
> 
> diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c index
> 82ff7e647..f798d6708 100644
> --- a/lib/odp-execute-avx512.c
> +++ b/lib/odp-execute-avx512.c
> @@ -20,9 +20,9 @@
> 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
> +#include 
> 
For FreeBSD, network headers must be included in a certain order. 
See this commit for details:
b2befd5bb2db ("sparse: Add guards to prevent FreeBSD-incompatible #include 
order.")
So these need to stay in this order. 

>  #include "csum.h"
>  #include "dp-packet.h"
> @@ -547,8 +547,8 @@ avx512_ipv6_sum_header(__m512i ip6_header)
>   * horizontal add. */
>  __m256i v_swap32a = _mm256_setr_epi32(0x0, 0x4, 0xF, 0xF,
>0xF, 0xF, 0xF, 0xF);
> -v_delta = _mm256_permutexvar_epi32(v_swap32a, v_delta);
> 
> +v_delta = _mm256_permutexvar_epi32(v_swap32a, v_delta);
>  v_delta = _mm256_hadd_epi32(v_delta, v_zeros);
>  v_delta = _mm256_hadd_epi16(v_delta, v_zeros);
> 
> @@ -562,7 +562,7 @@ avx512_ipv6_addr_csum_delta(__m512i old_header,
> __m512i new_header)  {
>  uint16_t old_delta = avx512_ipv6_sum_header(old_header);
>  uint16_t new_delta = avx512_ipv6_sum_header(new_header);
> -uint32_t csum_delta = (uint16_t)~old_delta + new_delta;
> +uint32_t csum_delta = ((uint16_t) ~old_delta) + new_delta;
> 
>  return  ~csum_finish(csum_delta);
>  }
> @@ -606,15 +606,15 @@ action_avx512_ipv6_set_addrs(struct
> dp_packet_batch *batch,
>  __m512i v_key_shuf = _mm512_permutexvar_epi8(v_shuffle, v_key);
>  __m512i v_mask_shuf = _mm512_permutexvar_epi8(v_shuffle, v_mask);
> 
> -
>  /* Set the v_zero register to all zero's. */
>  const __m128i v_zeros = _mm_setzero_si128();
> +
>  /* Set the v_all_ones register to all one's. */
>  const __m128i v_all_ones = _mm_cmpeq_epi16(v_zeros, v_zeros);
> 
> -/* Load ip6 src and dst respectively into 128-bit wide registers. */
> +/* Load ip6 src and dst masks respectively into 128-bit wide
> + registers. */
>  __m128i v_src = _mm_loadu_si128((void *) mask);
> -__m128i v_dst = _mm_maskz_loadu_epi64(0xC,(void *) mask);
> +__m128i v_dst = _mm_maskz_loadu_epi64(0xC, (void *) mask);
> 
>  /* Perform a bitwise OR between src and dst registers. */
>  __m128i v_or = _mm_or_si128(v_src, v_dst);
> 
> > v4:
> >   - Reworked and moved check for checksum outside loop.
> >   - Code cleanup based on review from Eelco.
> > v3:
> >   - Added a runtime check for AVX512 vbmi.
> > v2:
> >   - Added check for availbility of s6_addr32 field of struct in6_addr.
> >   - Fixed network headers for freebsd builds.
> >
> 
> 
> 
> > +/* Load ip6 src and dst respectively into 128-bit wide registers. */
> > +__m128i v_src = _mm_loadu_si128((void *) mask);
> > +__m128i v_dst = _mm_maskz_loadu_epi64(0xC,(void *) mask);
> 
> Guess it might be me, but I do not understand how
> _mm_maskz_loadu_epi64() will load the dst from the mask.
> Looking at the intrinsics guide it will only read the first two 64-bit 
> values, but
> mask points to src?
> 
> Should we not just do the following here?
> 
> +__m128i v_src = _mm_loadu_si128((void *) &mask->ipv6_src);
> +__m128i v_dst = _mm_loadu_si128((void *) &mask->ipv6_dst);
> 

Yes, good catch. The _maskz_load_ isn't correctly pointing to the dst values 
from the mask. 
 
I will make all the changes above (except the include header comment) and send 
v5 shortly. 


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

Re: [ovs-dev] Patch "openvswitch: Fix Frame-size larger than 1024 bytes warning" not correct.

2022-11-25 Thread Ilya Maximets

On 11/15/22 17:16, Eelco Chaudron wrote:
> Hi Pravin,
> 
> It looks like a previous fix you made, 190aa3e77880 ("openvswitch: Fix 
> Frame-size larger than 1024 bytes warning."), is breaking stuff. With this 
> change, the actual flow lookup, ovs_flow_tbl_lookup(), is done using a masked 
> key, where it should be an unmasked key. This is maybe more clear if you take 
> a look at the diff for the ufid addition, 74ed7ab9264c ("openvswitch: Add 
> support for unique flow IDs.").
> 
> Just reverting the change gets rid of the problem, but it will re-introduce 
> the larger stack size. It looks like we either have it on the stack or 
> dynamically allocate it each time. Let me know if you have any other clever 
> fix ;)

I'd say that dynamic allocation should be fine.
We do alloc other things in the same function, and
I don't immediately see another simple way to fix
the problem without heavily re-working the logic.

My 2c.
Best regards, Ilya Maximets.

> 
> We found this after debugging some customer-specific issue. More details are 
> in the following OVS patch, 
> https://patchwork.ozlabs.org/project/openvswitch/list/?series=328315
> 
> Cheers,
> 
> Eelco
> 
> 
> FYI the working revers:
> 
> 
>Revert "openvswitch: Fix Frame-size larger than 1024 bytes warning."
> 
> This reverts commit 190aa3e77880a05332ea1ccb382a51285d57adb5.
> 
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 861dfb8daf4a..660d5fdd9b28 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -948,6 +948,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct 
> genl_info *info)
> struct sw_flow_mask mask;
> struct sk_buff *reply;
> struct datapath *dp;
> +   struct sw_flow_key key;
> struct sw_flow_actions *acts;
> struct sw_flow_match match;
> u32 ufid_flags = ovs_nla_get_ufid_flags(a[OVS_FLOW_ATTR_UFID_FLAGS]);
> @@ -975,24 +976,20 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct 
> genl_info *info)
> }
> 
> /* Extract key. */
> -   ovs_match_init(&match, &new_flow->key, false, &mask);
> +   ovs_match_init(&match, &key, true, &mask);
> error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
>   a[OVS_FLOW_ATTR_MASK], log);
> if (error)
> goto err_kfree_flow;
> 
> +   ovs_flow_mask_key(&new_flow->key, &key, true, &mask);
> +
> /* Extract flow identifier. */
> error = ovs_nla_get_identifier(&new_flow->id, a[OVS_FLOW_ATTR_UFID],
> -  &new_flow->key, log);
> +  &key, log);
> if (error)
> goto err_kfree_flow;
> 
> -   /* unmasked key is needed to match when ufid is not used. */
> -   if (ovs_identifier_is_key(&new_flow->id))
> -   match.key = new_flow->id.unmasked_key;
> -
> -   ovs_flow_mask_key(&new_flow->key, &new_flow->key, true, &mask);
> -
> /* Validate actions. */
> error = ovs_nla_copy_actions(net, a[OVS_FLOW_ATTR_ACTIONS],
>  &new_flow->key, &acts, log);
> @@ -1019,7 +1016,7 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct 
> genl_info *info)
> if (ovs_identifier_is_ufid(&new_flow->id))
> flow = ovs_flow_tbl_lookup_ufid(&dp->table, &new_flow->id);
> if (!flow)
> -   flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->key);
> +   flow = ovs_flow_tbl_lookup(&dp->table, &key);
> if (likely(!flow)) {
> rcu_assign_pointer(new_flow->sf_acts, acts);
> 

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

Re: [ovs-dev] [PATCH/RFC net-next] tc: allow drivers to accept gact with PIPE when offloading

2022-11-25 Thread Eelco Chaudron




On 25 Nov 2022, at 15:19, Marcelo Leitner wrote:

> On Fri, Nov 25, 2022 at 03:10:37AM +, Tianyu Yuan wrote:
>> On Fri, Nov 25, 2022 at 10:21 AM  Jamal Hadi Salim  wrote:
>>
>>>
>>> I am not sure if the mlx5 changes will work since  they both seem to be 
>>> calling
>>> mlx5e_tc_act_get() which expects the act->id to exist in tc_acts_xxx tables,
>>> meaning mlx5e_tc_act_get() will always return you NULL  and that check is
>>> hit before you check for ACT_PIPE.
>>>
>>> Something not obvious to me:
>>> Would all these drivers now be able to handle ACT_PIPE transparently as if
>>> no action is specified? Cant see the obvious connection to POLICE by just
>>> staring at the patch - is there and ACT_PIPE first then a POLICE?
>>>  Another question:
>>> If the ACT_PIPE count is not being updated in s/w - is there a h/w 
>>> equivalent
>>> stat being updated?
>>>
>>> cheers,
>>> jamal
>>>
>> Thanks Jamal for your review.
>>
>> About mlx5e_tc_act_get(), I'll later add PIPE action in tc_acts_nic so that 
>> mlx5e_tc_act_get() will return the right
>> act_id.
>>
>> In driver we choose just ignore this gact with ACT_PIPE, so after parsing 
>> the filter(rule) from kernel, the remaining
>> actions are just like what they used to be without changes in this patch. So 
>> the flow could be processed as before.
>>
>> The connection between POLICE and ACT_PIPE may exist in userspace (e.g. 
>> ovs), we could put a gact (PIPE) at the
>> beginning place in each tc filter. We will also have an OVS patch for this 
>> propose.
>>
>> I'm not very clear with your last case, but in expectation, the once the 
>> traffic is offloaded in h/w tc datapath, the
>> stats will be updated by the flower stats from hardware. And when the 
>> traffic is using s/w tc datapath, the stats are
>> from software.
>
> I'm still confused here. Take, for example cxgb4 driver below. It will
> simply ignore this action AFAICT. This is good because it will still
> offload whatever vswitchd would be offloading but then, I don't see
> how the stats will be right in the end. I think the hw stats will be
> zeroed, no? (this is already considering the per action stats change
> that Oz is working on, see [ RFC  net-next v2 0/2] net: flow_offload:
> add support for per action hw stats)
>
> I think the drivers have to reject the action if they don't support
> it, and vswitchd will have to probe for proper support when starting.

I guess OVS userspace needs a simple way to determine which approach to use, 
i.e. if the kernel has this patch series applied. Or else it would not be easy 
to migrate userspace to use this approach.

> Other than this, patch seems good.
>
> Thanks,
> Marcelo
>
>>
>> B.R.
>> Tianyu
>>
>>>
>>> On Tue, Nov 22, 2022 at 6:21 AM Simon Horman
>>>  wrote:

 From: Tianyu Yuan 

 Support gact with PIPE action when setting up gact in TC.
 This PIPE gact could come first in each tc filter to update the
 filter(flow) stats.

 The stats for each actons in a filter are updated by the flower stats
 from HW(via netdev drivers) in kernel TC rather than drivers.

 In each netdev driver, we don't have to process this gact, but only to
 ignore it to make sure the whole rule can be offloaded.

 Background:

 This is a proposed solution to a problem with a miss-match between TC
 police action instances - which may be shared between flows - and
 OpenFlow meter actions - the action is per flow, while the underlying
 meter may be shared. The key problem being that the police action
 statistics are shared between flows, and this does not match the
 requirement of OpenFlow for per-flow statistics.

 Ref: [ovs-dev] [PATCH] tests: fix reference output for meter offload stats

 https://mail.openvswitch.org/pipermail/ovs-dev/2022-
>>> October/398363.htm
 l

 Signed-off-by: Tianyu Yuan 
 Signed-off-by: Simon Horman 
 ---
  drivers/net/dsa/ocelot/felix_vsc9959.c | 5 +
  drivers/net/dsa/sja1105/sja1105_flower.c   | 5 +
  drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c   | 5 +
  drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c | 6 ++
  drivers/net/ethernet/intel/ice/ice_tc_lib.c| 5 +
  drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c   | 5 +
  drivers/net/ethernet/marvell/prestera/prestera_flower.c| 5 +
  drivers/net/ethernet/mediatek/mtk_ppe_offload.c| 5 +
  drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c| 6 ++
  drivers/net/ethernet/mellanox/mlx5/core/en_tc.c| 5 +
  drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c  | 5 +
  drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c   | 4 
  drivers/net/ethernet/mscc/ocelot_flower.c  | 5 +
  drivers/net/ethernet/netronome/nfp/flower/action.c | 5

Re: [ovs-dev] [PATCH/RFC net-next] tc: allow drivers to accept gact with PIPE when offloading

2022-11-25 Thread Marcelo Leitner

On Fri, Nov 25, 2022 at 03:10:37AM +, Tianyu Yuan wrote:
> On Fri, Nov 25, 2022 at 10:21 AM  Jamal Hadi Salim  wrote:
>
> >
> > I am not sure if the mlx5 changes will work since  they both seem to be 
> > calling
> > mlx5e_tc_act_get() which expects the act->id to exist in tc_acts_xxx tables,
> > meaning mlx5e_tc_act_get() will always return you NULL  and that check is
> > hit before you check for ACT_PIPE.
> >
> > Something not obvious to me:
> > Would all these drivers now be able to handle ACT_PIPE transparently as if
> > no action is specified? Cant see the obvious connection to POLICE by just
> > staring at the patch - is there and ACT_PIPE first then a POLICE?
> >  Another question:
> > If the ACT_PIPE count is not being updated in s/w - is there a h/w 
> > equivalent
> > stat being updated?
> >
> > cheers,
> > jamal
> >
> Thanks Jamal for your review.
>
> About mlx5e_tc_act_get(), I'll later add PIPE action in tc_acts_nic so that 
> mlx5e_tc_act_get() will return the right
> act_id.
>
> In driver we choose just ignore this gact with ACT_PIPE, so after parsing the 
> filter(rule) from kernel, the remaining
> actions are just like what they used to be without changes in this patch. So 
> the flow could be processed as before.
>
> The connection between POLICE and ACT_PIPE may exist in userspace (e.g. ovs), 
> we could put a gact (PIPE) at the
> beginning place in each tc filter. We will also have an OVS patch for this 
> propose.
>
> I'm not very clear with your last case, but in expectation, the once the 
> traffic is offloaded in h/w tc datapath, the
> stats will be updated by the flower stats from hardware. And when the traffic 
> is using s/w tc datapath, the stats are
> from software.

I'm still confused here. Take, for example cxgb4 driver below. It will
simply ignore this action AFAICT. This is good because it will still
offload whatever vswitchd would be offloading but then, I don't see
how the stats will be right in the end. I think the hw stats will be
zeroed, no? (this is already considering the per action stats change
that Oz is working on, see [ RFC  net-next v2 0/2] net: flow_offload:
add support for per action hw stats)

I think the drivers have to reject the action if they don't support
it, and vswitchd will have to probe for proper support when starting.

Other than this, patch seems good.

Thanks,
Marcelo

>
> B.R.
> Tianyu
>
> >
> > On Tue, Nov 22, 2022 at 6:21 AM Simon Horman
> >  wrote:
> > >
> > > From: Tianyu Yuan 
> > >
> > > Support gact with PIPE action when setting up gact in TC.
> > > This PIPE gact could come first in each tc filter to update the
> > > filter(flow) stats.
> > >
> > > The stats for each actons in a filter are updated by the flower stats
> > > from HW(via netdev drivers) in kernel TC rather than drivers.
> > >
> > > In each netdev driver, we don't have to process this gact, but only to
> > > ignore it to make sure the whole rule can be offloaded.
> > >
> > > Background:
> > >
> > > This is a proposed solution to a problem with a miss-match between TC
> > > police action instances - which may be shared between flows - and
> > > OpenFlow meter actions - the action is per flow, while the underlying
> > > meter may be shared. The key problem being that the police action
> > > statistics are shared between flows, and this does not match the
> > > requirement of OpenFlow for per-flow statistics.
> > >
> > > Ref: [ovs-dev] [PATCH] tests: fix reference output for meter offload stats
> > >
> > > https://mail.openvswitch.org/pipermail/ovs-dev/2022-
> > October/398363.htm
> > > l
> > >
> > > Signed-off-by: Tianyu Yuan 
> > > Signed-off-by: Simon Horman 
> > > ---
> > >  drivers/net/dsa/ocelot/felix_vsc9959.c | 5 +
> > >  drivers/net/dsa/sja1105/sja1105_flower.c   | 5 +
> > >  drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c   | 5 +
> > >  drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c | 6 ++
> > >  drivers/net/ethernet/intel/ice/ice_tc_lib.c| 5 +
> > >  drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c   | 5 +
> > >  drivers/net/ethernet/marvell/prestera/prestera_flower.c| 5 +
> > >  drivers/net/ethernet/mediatek/mtk_ppe_offload.c| 5 +
> > >  drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c| 6 ++
> > >  drivers/net/ethernet/mellanox/mlx5/core/en_tc.c| 5 +
> > >  drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c  | 5 +
> > >  drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c   | 4 
> > >  drivers/net/ethernet/mscc/ocelot_flower.c  | 5 +
> > >  drivers/net/ethernet/netronome/nfp/flower/action.c | 5 +
> > >  drivers/net/ethernet/qlogic/qede/qede_filter.c | 5 +
> > >
> >   | 5 +
> > >  drivers/net/ethernet/ti/cpsw_priv.c| 5 +
> > >  net/sched/act_gact.c

Re: [ovs-dev] [PATCH ovn v2] controller: Fixed ovs/ovn(features) connection lost when running more than 120 seconds

2022-11-25 Thread Dumitru Ceara

On 11/24/22 15:24, Ales Musil wrote:
> On Thu, Nov 24, 2022 at 3:03 PM Xavier Simonart  wrote:
> 
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2144084
>>
>> Signed-off-by: Xavier Simonart 
>>
>> ---
>> v2: - Based on Dumitru's feedback:
>>   - Reduce test case length by removing uggly sleep
>>   - Add an explicit probe for the feature connection
>> - Rebased on main
>> ---
>>  lib/features.c | 22 +++---
>>  tests/ovn.at   | 31 +++
>>  2 files changed, 46 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/features.c b/lib/features.c
>> index f15ec42bb..462b99818 100644
>> --- a/lib/features.c
>> +++ b/lib/features.c
>> @@ -26,10 +26,13 @@
>>  #include "openvswitch/rconn.h"
>>  #include "openvswitch/ofp-msgs.h"
>>  #include "openvswitch/ofp-meter.h"
>> +#include "openvswitch/ofp-util.h"
>>  #include "ovn/features.h"
>>
>>  VLOG_DEFINE_THIS_MODULE(features);
>>
>> +#define FEATURES_DEFAULT_PROBE_INTERVAL_SEC 5
>> +
>>  struct ovs_feature {
>>  enum ovs_feature_value value;
>>  const char *name;
>> @@ -74,7 +77,8 @@ static void
>>  ovs_feature_rconn_setup(const char *br_name)
>>  {
>>  if (!swconn) {
>> -swconn = rconn_create(5, 0, DSCP_DEFAULT, 1 << OFP15_VERSION);
>> +swconn = rconn_create(FEATURES_DEFAULT_PROBE_INTERVAL_SEC, 0,
>> +  DSCP_DEFAULT, 1 << OFP15_VERSION);
>>  }
>>
>>  if (!rconn_is_connected(swconn)) {
>> @@ -85,11 +89,14 @@ ovs_feature_rconn_setup(const char *br_name)
>>  }
>>  free(target);
>>  }
>> +rconn_set_probe_interval(swconn, FEATURES_DEFAULT_PROBE_INTERVAL_SEC);
>>  }
>>
>>  static bool
>>  ovs_feature_get_openflow_cap(const char *br_name)
>>  {
>> +struct ofpbuf *msg;
>> +
>>  if (!br_name) {
>>  return false;
>>  }
>> @@ -102,15 +109,14 @@ ovs_feature_get_openflow_cap(const char *br_name)
>>  }
>>
>>  /* send new requests just after reconnect. */
>> -if (conn_seq_no == rconn_get_connection_seqno(swconn)) {
>> -return false;
>> +if (conn_seq_no != rconn_get_connection_seqno(swconn)) {
>> +/* dump datapath meter capabilities. */
>> +msg = ofpraw_alloc(OFPRAW_OFPST13_METER_FEATURES_REQUEST,
>> +   rconn_get_version(swconn), 0);
>> +rconn_send(swconn, msg, NULL);
>>  }
>>
>>  bool ret = false;
>> -/* dump datapath meter capabilities. */
>> -struct ofpbuf *msg =
>> ofpraw_alloc(OFPRAW_OFPST13_METER_FEATURES_REQUEST,
>> -  rconn_get_version(swconn), 0);
>> -rconn_send(swconn, msg, NULL);
>>  for (int i = 0; i < 50; i++) {
>>  msg = rconn_recv(swconn);
>>  if (!msg) {
>> @@ -137,6 +143,8 @@ ovs_feature_get_openflow_cap(const char *br_name)
>>  }
>>  }
>>  conn_seq_no = rconn_get_connection_seqno(swconn);
>> +} else if (type == OFPTYPE_ECHO_REQUEST) {
>> +rconn_send(swconn, ofputil_encode_echo_reply(oh), NULL);
>>  }
>>  ofpbuf_delete(msg);
>>  }
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 9d52b1677..0ef536509 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -33450,3 +33450,34 @@ check_drops
>>  OVN_CLEANUP([hv1],[hv2])
>>  AT_CLEANUP
>>  ])
>> +
>> +OVN_FOR_EACH_NORTHD([
>> +AT_SETUP([feature inactivity probe])
>> +ovn_start
>> +net_add n1
>> +
>> +sim_add hv1
>> +as hv1
>> +check ovs-vsctl add-br br-phys
>> +ovn_attach n1 br-phys 192.168.0.1
>> +
>> +dnl Ensure that there are at least 3 openflow connections.
>> +OVS_WAIT_UNTIL([test "$(grep -c 'negotiated OpenFlow version'
>> hv1/ovs-vswitchd.log)" -eq "3"])
>> +
>> +dnl "Wait" 3 times 60 seconds and ensure ovn-controller writes to the
>> +dnl openflow connections in the meantime.  This should allow ovs-vswitchd
>> +dnl to probe the openflow connections at least twice.
>> +
>> +as hv1 ovs-appctl time/warp 6
>> +check ovn-nbctl --wait=hv sync
>> +
>> +as hv1 ovs-appctl time/warp 6
>> +check ovn-nbctl --wait=hv sync
>> +
>> +as hv1 ovs-appctl time/warp 6
>> +check ovn-nbctl --wait=hv sync
>> +
>> +AT_CHECK([test -z "`grep disconnecting hv1/ovs-vswitchd.log`"])
>> +OVN_CLEANUP([hv1])
>> +AT_CLEANUP
>> +])
>> --
>> 2.31.1
>>
>>
> Looks good to me, thanks.
> 
> Acked-by: Ales Musil 
> 

Thanks Ales and Xavier!

Applied and backported down to 22.03.

Thanks,
Dumitru

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

[ovs-dev] [PATCH] tc: allow gact pipe action offload

2022-11-25 Thread Vlad Buslov via dev

Flow action infrastructure and mlx5 only.

Signed-off-by: Vlad Buslov 
---
 .../net/ethernet/mellanox/mlx5/core/Makefile  |  3 +-
 .../mellanox/mlx5/core/en/tc/act/act.c|  2 ++
 .../mellanox/mlx5/core/en/tc/act/act.h|  1 +
 .../mellanox/mlx5/core/en/tc/act/pipe.c   | 28 +++
 net/sched/act_gact.c  |  7 +++--
 5 files changed, 37 insertions(+), 4 deletions(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/pipe.c

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile 
b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
index a22c32aabf11..566a03e80cf8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
@@ -55,7 +55,8 @@ mlx5_core-$(CONFIG_MLX5_CLS_ACT) += en/tc/act/act.o 
en/tc/act/drop.o en/tc/a
en/tc/act/vlan.o 
en/tc/act/vlan_mangle.o en/tc/act/mpls.o \
en/tc/act/mirred.o 
en/tc/act/mirred_nic.o \
en/tc/act/ct.o en/tc/act/sample.o 
en/tc/act/ptype.o \
-   en/tc/act/redirect_ingress.o 
en/tc/act/police.o
+   en/tc/act/redirect_ingress.o 
en/tc/act/police.o \
+   en/tc/act/pipe.o
 
 ifneq ($(CONFIG_MLX5_TC_CT),)
mlx5_core-y  += en/tc_ct.o en/tc/ct_fs_dmfs.o
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.c 
b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.c
index 3337241cfd84..e8fcc18c7074 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.c
@@ -28,6 +28,7 @@ static struct mlx5e_tc_act *tc_acts_fdb[NUM_FLOW_ACTIONS] = {
[FLOW_ACTION_CT] = &mlx5e_tc_act_ct,
[FLOW_ACTION_MPLS_PUSH] = &mlx5e_tc_act_mpls_push,
[FLOW_ACTION_MPLS_POP] = &mlx5e_tc_act_mpls_pop,
+   [FLOW_ACTION_PIPE] = &mlx5e_tc_act_pipe,
[FLOW_ACTION_VLAN_PUSH_ETH] = &mlx5e_tc_act_vlan,
[FLOW_ACTION_VLAN_POP_ETH] = &mlx5e_tc_act_vlan,
 };
@@ -42,6 +43,7 @@ static struct mlx5e_tc_act *tc_acts_nic[NUM_FLOW_ACTIONS] = {
[FLOW_ACTION_CSUM] = &mlx5e_tc_act_csum,
[FLOW_ACTION_MARK] = &mlx5e_tc_act_mark,
[FLOW_ACTION_CT] = &mlx5e_tc_act_ct,
+   [FLOW_ACTION_PIPE] = &mlx5e_tc_act_pipe,
 };
 
 /**
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.h 
b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.h
index e1570ff056ae..dd863e84a925 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/act.h
@@ -87,6 +87,7 @@ extern struct mlx5e_tc_act mlx5e_tc_act_sample;
 extern struct mlx5e_tc_act mlx5e_tc_act_ptype;
 extern struct mlx5e_tc_act mlx5e_tc_act_redirect_ingress;
 extern struct mlx5e_tc_act mlx5e_tc_act_police;
+extern struct mlx5e_tc_act mlx5e_tc_act_pipe;
 
 struct mlx5e_tc_act *
 mlx5e_tc_act_get(enum flow_action_id act_id,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/pipe.c 
b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/pipe.c
new file mode 100644
index ..75207b57bec2
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc/act/pipe.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+// Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+
+#include "act.h"
+#include "en/tc_priv.h"
+
+static bool
+tc_act_can_offload_pipe(struct mlx5e_tc_act_parse_state *parse_state,
+   const struct flow_action_entry *act,
+   int act_index,
+   struct mlx5_flow_attr *attr)
+{
+   return true;
+}
+
+static int
+tc_act_parse_pipe(struct mlx5e_tc_act_parse_state *parse_state,
+ const struct flow_action_entry *act,
+ struct mlx5e_priv *priv,
+ struct mlx5_flow_attr *attr)
+{
+   return 0;
+}
+
+struct mlx5e_tc_act mlx5e_tc_act_pipe = {
+   .can_offload = tc_act_can_offload_pipe,
+   .parse_action = tc_act_parse_pipe,
+};
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 62d682b96b88..82d1371e251e 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -250,15 +250,14 @@ static int tcf_gact_offload_act_setup(struct tc_action 
*act, void *entry_data,
} else if (is_tcf_gact_goto_chain(act)) {
entry->id = FLOW_ACTION_GOTO;
entry->chain_index = tcf_gact_goto_chain_index(act);
+   } else if (is_tcf_gact_pipe(act)) {
+   entry->id = FLOW_ACTION_PIPE;
} else if (is_tcf_gact_continue(act)) {
NL_SET_ERR_MSG_MOD(extack, "Offload of \"continue\" 
action is not supported");
return -EOPNOTSUPP;
} else if (is_tc

Re: [ovs-dev] [PATCH] Handlers: Allow to explicitly set the number of handlers

2022-11-25 Thread Adrian Moreno


On 10/25/22 21:18, Michael Santana wrote:

Currently n-handler-threads is ignored when using per-cpu mode. There
are some small situations where manually setting the number of handlers
is necessary. 


Hi Michael,
This is not a full review, I just want to understand the use-case.
Could you please give an example of when this is needed?

Thanks
--
Adrián Moreno


For this reason allow manually setting the number of
handlers via n-handler-threads

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2106570
Signed-off-by: Michael Santana 
---
  lib/dpif-netlink.c| 20 +++-
  lib/dpif-provider.h   |  2 +-
  lib/dpif.c|  4 ++--
  lib/dpif.h|  2 +-
  ofproto/ofproto-dpif-upcall.c |  3 +++
  ofproto/ofproto-dpif.c|  5 +++--
  6 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index a620a6ec5..641c9ff99 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -264,7 +264,8 @@ static void dpif_netlink_handler_uninit(struct dpif_handler 
*handler);
  static int dpif_netlink_refresh_handlers_vport_dispatch(struct dpif_netlink *,
  uint32_t n_handlers);
  static void destroy_all_channels(struct dpif_netlink *);
-static int dpif_netlink_refresh_handlers_cpu_dispatch(struct dpif_netlink *);
+static int dpif_netlink_refresh_handlers_cpu_dispatch(struct dpif_netlink *,
+  uint32_t n_handlers);
  static void destroy_all_handlers(struct dpif_netlink *);
  
  static void dpif_netlink_vport_to_ofpbuf(const struct dpif_netlink_vport *,

@@ -2586,15 +2587,14 @@ dpif_netlink_calculate_n_handlers(void)
  }
  
  static int

-dpif_netlink_refresh_handlers_cpu_dispatch(struct dpif_netlink *dpif)
+dpif_netlink_refresh_handlers_cpu_dispatch(struct dpif_netlink *dpif,
+   uint32_t n_handlers)
  OVS_REQ_WRLOCK(dpif->upcall_lock)
  {
  int handler_id;
  int error = 0;
-uint32_t n_handlers;
  uint32_t *upcall_pids;
  
-n_handlers = dpif_netlink_calculate_n_handlers();

  if (dpif->n_handlers != n_handlers) {
  VLOG_DBG("Dispatch mode(per-cpu): initializing %d handlers",
 n_handlers);
@@ -2770,7 +2770,8 @@ dpif_netlink_recv_set_vport_dispatch(struct dpif_netlink 
*dpif, bool enable)
  }
  
  static int

-dpif_netlink_recv_set_cpu_dispatch(struct dpif_netlink *dpif, bool enable)
+dpif_netlink_recv_set_cpu_dispatch(struct dpif_netlink *dpif, bool enable,
+   uint32_t handler)
  OVS_REQ_WRLOCK(dpif->upcall_lock)
  {
  if ((dpif->handlers != NULL) == enable) {
@@ -2779,19 +2780,19 @@ dpif_netlink_recv_set_cpu_dispatch(struct dpif_netlink 
*dpif, bool enable)
  destroy_all_handlers(dpif);
  return 0;
  } else {
-return dpif_netlink_refresh_handlers_cpu_dispatch(dpif);
+return dpif_netlink_refresh_handlers_cpu_dispatch(dpif, handler);
  }
  }
  
  static int

-dpif_netlink_recv_set(struct dpif *dpif_, bool enable)
+dpif_netlink_recv_set(struct dpif *dpif_, bool enable, uint32_t handlers)
  {
  struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
  int error;
  
  fat_rwlock_wrlock(&dpif->upcall_lock);

  if (dpif_netlink_upcall_per_cpu(dpif)) {
-error = dpif_netlink_recv_set_cpu_dispatch(dpif, enable);
+error = dpif_netlink_recv_set_cpu_dispatch(dpif, enable, handlers);
  } else {
  error = dpif_netlink_recv_set_vport_dispatch(dpif, enable);
  }
@@ -2817,7 +2818,8 @@ dpif_netlink_handlers_set(struct dpif *dpif_, uint32_t 
n_handlers)
  fat_rwlock_wrlock(&dpif->upcall_lock);
  if (dpif->handlers) {
  if (dpif_netlink_upcall_per_cpu(dpif)) {
-error = dpif_netlink_refresh_handlers_cpu_dispatch(dpif);
+error = dpif_netlink_refresh_handlers_cpu_dispatch(dpif,
+   n_handlers);
  } else {
  error = dpif_netlink_refresh_handlers_vport_dispatch(dpif,
   n_handlers);
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 12477a24f..91be767a8 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -342,7 +342,7 @@ struct dpif_class {
   * Turning packet receive off and then back on is allowed to change 
Netlink
   * PID assignments (see ->port_get_pid()).  The client is responsible for
   * updating flows as necessary if it does this. */
-int (*recv_set)(struct dpif *dpif, bool enable);
+int (*recv_set)(struct dpif *dpif, bool enable, uint32_t n_handlers);
  
  /* Attempts to refresh the poll loops and Netlink sockets used for handling

   * upcalls when the number of upcall handlers (upcall receiving thread) is
diff --git a/lib/dpif.c b/lib/dpif.c
index 40f5fe446..6

Re: [ovs-dev] [PATCH/RFC net-next] tc: allow drivers to accept gact with PIPE when offloading

2022-11-25 Thread Vlad Buslov via dev

On Fri 25 Nov 2022 at 03:10, Tianyu Yuan  wrote:
> On Fri, Nov 25, 2022 at 10:21 AM  Jamal Hadi Salim  wrote:
>
>> 
>> I am not sure if the mlx5 changes will work since  they both seem to be 
>> calling
>> mlx5e_tc_act_get() which expects the act->id to exist in tc_acts_xxx tables,
>> meaning mlx5e_tc_act_get() will always return you NULL  and that check is
>> hit before you check for ACT_PIPE.
>> 
>> Something not obvious to me:
>> Would all these drivers now be able to handle ACT_PIPE transparently as if
>> no action is specified? Cant see the obvious connection to POLICE by just
>> staring at the patch - is there and ACT_PIPE first then a POLICE?
>>  Another question:
>> If the ACT_PIPE count is not being updated in s/w - is there a h/w equivalent
>> stat being updated?
>> 
>> cheers,
>> jamal
>> 
> Thanks Jamal for your review.
>
> About mlx5e_tc_act_get(), I'll later add PIPE action in tc_acts_nic so that 
> mlx5e_tc_act_get() will return the right
> act_id.

Sorry for the late response. Jamal is correct and ACT_PIPE should indeed
be properly handled in mlx5 by extending action array with correct type.
You also need to extend tc_acts_fdb besides tc_acts_nic since "fdb" is
responsible for actions in switchdev mode. I'll followup with a PoC
patch that works on our hardware.

>
> In driver we choose just ignore this gact with ACT_PIPE, so after parsing the 
> filter(rule) from kernel, the remaining
> actions are just like what they used to be without changes in this patch. So 
> the flow could be processed as before.
>
> The connection between POLICE and ACT_PIPE may exist in userspace (e.g. ovs), 
> we could put a gact (PIPE) at the
> beginning place in each tc filter. We will also have an OVS patch for this 
> propose.
>
> I'm not very clear with your last case, but in expectation, the once the 
> traffic is offloaded in h/w tc datapath, the
> stats will be updated by the flower stats from hardware. And when the traffic 
> is using s/w tc datapath, the stats are
> from software.
>
> B.R.
> Tianyu
>
>> 
>> On Tue, Nov 22, 2022 at 6:21 AM Simon Horman
>>  wrote:
>> >
>> > From: Tianyu Yuan 
>> >
>> > Support gact with PIPE action when setting up gact in TC.
>> > This PIPE gact could come first in each tc filter to update the
>> > filter(flow) stats.
>> >
>> > The stats for each actons in a filter are updated by the flower stats
>> > from HW(via netdev drivers) in kernel TC rather than drivers.
>> >
>> > In each netdev driver, we don't have to process this gact, but only to
>> > ignore it to make sure the whole rule can be offloaded.
>> >
>> > Background:
>> >
>> > This is a proposed solution to a problem with a miss-match between TC
>> > police action instances - which may be shared between flows - and
>> > OpenFlow meter actions - the action is per flow, while the underlying
>> > meter may be shared. The key problem being that the police action
>> > statistics are shared between flows, and this does not match the
>> > requirement of OpenFlow for per-flow statistics.
>> >
>> > Ref: [ovs-dev] [PATCH] tests: fix reference output for meter offload stats
>> >
>> > https://mail.openvswitch.org/pipermail/ovs-dev/2022-
>> October/398363.htm
>> > l
>> >
>> > Signed-off-by: Tianyu Yuan 
>> > Signed-off-by: Simon Horman 
>> > ---
>> >  drivers/net/dsa/ocelot/felix_vsc9959.c | 5 +
>> >  drivers/net/dsa/sja1105/sja1105_flower.c   | 5 +
>> >  drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c   | 5 +
>> >  drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-flower.c | 6 ++
>> >  drivers/net/ethernet/intel/ice/ice_tc_lib.c| 5 +
>> >  drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c   | 5 +
>> >  drivers/net/ethernet/marvell/prestera/prestera_flower.c| 5 +
>> >  drivers/net/ethernet/mediatek/mtk_ppe_offload.c| 5 +
>> >  drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c| 6 ++
>> >  drivers/net/ethernet/mellanox/mlx5/core/en_tc.c| 5 +
>> >  drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c  | 5 +
>> >  drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c   | 4 
>> >  drivers/net/ethernet/mscc/ocelot_flower.c  | 5 +
>> >  drivers/net/ethernet/netronome/nfp/flower/action.c | 5 +
>> >  drivers/net/ethernet/qlogic/qede/qede_filter.c | 5 +
>> >
>>   | 5 +
>> >  drivers/net/ethernet/ti/cpsw_priv.c| 5 +
>> >  net/sched/act_gact.c   | 7 ---
>> >  18 files changed, 90 insertions(+), 3 deletions(-)
>> >
>> >
>> > diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c
>> > b/drivers/net/dsa/ocelot/felix_vsc9959.c
>> > index b0ae8d6156f6..e54eb8e28386 100644
>> > --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
>> > +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
>> > @@ -2217,6 +2217,11 @@ static int vsc9959_psfp_filter_add(struct ocelot

[ovs-dev] [PATCH] ovsdb: Count weak reference objects.

2022-11-25 Thread Ilya Maximets

OVSDB creates a separate object for each weak reference in order to
track them and there could be a significant amount of these objects
in the database.

We also had problems with number of these objects growing out of
bounds recently.  So, adding them to a memory report seems to be
a good thing.

Counting them globally to cover all the copied instances in transactions
and the transaction history (even though there should be none).
It's also hard to count them per-database, because weak references
are stored on destination rows and can be destroyed either while
destroying the destination row or while removing the reference from
the source row.  Also, not all the involved functions have direct
access to the database object.  So, there is no single clear place
where counters should be updated.

Signed-off-by: Ilya Maximets 
---
 ovsdb/ovsdb.c   | 4 
 ovsdb/ovsdb.h   | 4 
 ovsdb/row.c | 5 -
 ovsdb/transaction.c | 2 ++
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c
index 1c011fab0..11786f376 100644
--- a/ovsdb/ovsdb.c
+++ b/ovsdb/ovsdb.c
@@ -43,6 +43,8 @@
 #include "openvswitch/vlog.h"
 VLOG_DEFINE_THIS_MODULE(ovsdb);
 
+size_t n_weak_refs = 0;
+
 struct ovsdb_schema *
 ovsdb_schema_create(const char *name, const char *version, const char *cksum)
 {
@@ -546,6 +548,8 @@ ovsdb_get_memory_usage(const struct ovsdb *db, struct simap 
*usage)
 if (db->storage) {
 ovsdb_storage_get_memory_usage(db->storage, usage);
 }
+
+simap_put(usage, "n-weak-refs", n_weak_refs);
 }
 
 struct ovsdb_table *
diff --git a/ovsdb/ovsdb.h b/ovsdb/ovsdb.h
index d05e7c64a..13d8bf407 100644
--- a/ovsdb/ovsdb.h
+++ b/ovsdb/ovsdb.h
@@ -125,6 +125,10 @@ struct ovsdb {
 struct ovsdb_compaction_state *snap_state;
 };
 
+/* Total number of 'weak reference' objects in all databases
+ * and transactions. */
+extern size_t n_weak_refs;
+
 struct ovsdb *ovsdb_create(struct ovsdb_schema *, struct ovsdb_storage *);
 void ovsdb_destroy(struct ovsdb *);
 
diff --git a/ovsdb/row.c b/ovsdb/row.c
index 3f0bb8acf..d7bfbdd36 100644
--- a/ovsdb/row.c
+++ b/ovsdb/row.c
@@ -21,8 +21,9 @@
 
 #include "openvswitch/dynamic-string.h"
 #include "openvswitch/json.h"
-#include "ovsdb-error.h"
 #include "openvswitch/shash.h"
+#include "ovsdb-error.h"
+#include "ovsdb.h"
 #include "sort.h"
 #include "table.h"
 #include "util.h"
@@ -78,6 +79,7 @@ ovsdb_weak_ref_clone(struct ovsdb_weak_ref *src)
 ovsdb_type_clone(&weak->type, &src->type);
 weak->column_idx = src->column_idx;
 weak->by_key = src->by_key;
+n_weak_refs++;
 return weak;
 }
 
@@ -130,6 +132,7 @@ ovsdb_weak_ref_destroy(struct ovsdb_weak_ref *weak)
 }
 ovsdb_type_destroy(&weak->type);
 free(weak);
+n_weak_refs--;
 }
 
 struct ovsdb_row *
diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
index 5d7c70a51..03541af85 100644
--- a/ovsdb/transaction.c
+++ b/ovsdb/transaction.c
@@ -613,6 +613,8 @@ add_weak_ref(const struct ovsdb_row *src, const struct 
ovsdb_row *dst_,
 weak->column_idx = column->index;
 hmap_node_nullify(&weak->dst_node);
 ovs_list_push_back(ref_list, &weak->src_node);
+
+n_weak_refs++;
 }
 
 static void
-- 
2.38.1

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

Re: [ovs-dev] [RFC PATCH ovn] northd, controller: Commit flows dropped by ACLs in a separate CT zone

2022-11-25 Thread Adrian Moreno




On 11/21/22 14:56, Abhiram Sangana wrote:

Hi Adrian,

I apologise for the delay in replying back.


On 14 Nov 2022, at 13:05, Adrian Moreno  wrote:

Hi,

On 10/20/22 15:49, Abhiram Sangana wrote:

Hi Dumitru,
Thanks for reviewing the patch.

On 19 Oct 2022, at 14:09, Dumitru Ceara  wrote:

Hi Abhiram,

Thanks for the patch!  I only skimmed the changes so this is not a full
review but more of a discussion starter.

On 10/18/22 17:33, Abhiram Sangana wrote:

To identify connections dropped by ACLs, users can enable logging for ACLs
but this approach does not scale. ACL logging uses "controller" action
which causes a significant spike in the CPU usage of ovs-vswitchd (and
ovn-controller to a lesser extent) even with metering enabled (observed
65% ovs-vswitchd CPU usage for logging 1000 packets per second). Another
approach is to use drop sampling (patch by Adrian Moreno currently in
review) but we might miss specific connections of interest with this
approach.

This patch commits connections dropped by ACLs to the connection tracking
table with a specific ACL label that was introduced in 0e0228be (
northd: Add ACL label). The dropped connections are committed in a separate
CT zone so that they can be managed independently.



I'm not sure I understand how the CMS can manage this.  How is this
better than sampling?  Committed connections are going to time out at
some point (30 sec by default for udp/icmp with the kernel datapath).
So the CMS will have to continuously monitor the contents of the
conntrack zone?  Aren't we just moving the CPU load somewhere else with
this?  Even so, there's a chance an entry is missed.

Linux nf_conntrack module supports sending connection tracking events
to userspace via ctnetlink ("net.netfilter.nf_conntrack_events" kernel
parameter). So, CMS can parse the stream of new connection events from
conntrack and log the packets based on CT label.


Isn't this datapath-specific? this won't be available for the netdev datapath, 
right?


Yes, this approach is datapath specific - I haven’t checked how to send 
connection tracking events for netdev datapath.



An issue with sampling is that if there are a large number of packets
for a particular connection(s), packets of other connections might not
get sampled and we miss information about these connections. With the
conntrack approach, we get a single event for each connection (until
they time out), so there is lesser load on the CMS/collector and lesser
likelihood of missing connections.


I don't see why sampling would inherently miss packets. The current RFC for ACL 
sampling (different from the generic drop sampling one) allows the user to 
specify a probability per ACL so 100% of packets can be sampled in connection 
establishment drops while we sample N% of accepted traffic having a lot of 
flexibility in the inevitable performance vs accuracy tradeoff.

I'd like to better understand what are the limitations of the current approach 
to see if it can be improved in any way.


I haven’t experimented with flow-based IPFIX sampling but I noticed high CPU 
usage of ovs-vswitchd while trying to export Netflow records (which I think is 
similar to 100% sampling) with large number of connections in OVN bridge. I was 
expecting a similar cost with respect to upcalls if we use 100% sampling rate 
for drop ACLs when there are multiple connection establishment packets in the 
bridge.



I have also found that per-bridge sampling at 100% not very usable. Besides, by 
default both ingress and egress traffic is sampled so that's pretty much 
generating 2 IPFIX messages per packet.


For ACL-rejection sampling, however, it shouldn't be that much. Besides, we can 
always make use of the flow cache to avoid keeping hold of the handler threads. 
I plan to run some performance benchmarking of this.




Thanks,
Abhiram


Thanks,
--
Adrián




Each logical port is assigned a new zone for committing dropped flows.
The zone is loaded into register MFF_LOG_ACL_DROP_ZONE.

A new lflow action "ct_commit_drop" is introduced that commits flows
to connection tracking table in a zone identified by
MFF_LOG_ACL_DROP_ZONE register.

An ACL with "drop" action and non-empty label is translated to "ct_commit_drop"
instead of silently dropping the packet.

Signed-off-by: Abhiram Sangana 
---
controller/ovn-controller.c  | 23 ++---
controller/physical.c| 32 +--
include/ovn/actions.h|  1 +
include/ovn/logical-fields.h |  1 +
lib/actions.c| 50 
lib/ovn-util.c   |  4 +--
lib/ovn-util.h   |  2 +-
northd/northd.c  | 14 --
northd/ovn-northd.8.xml  | 14 --
ovn-sb.xml   | 17 
ovs  |  2 +-


This shouldn't need to change the OVS submodule.


My bad. Will fix this.
Thanks,
Abhiram Sangana

Thanks,
Dumitru


utilities/ovn-nbctl.c|  7 ++---
12 files changed, 151

Re: [ovs-dev] [v4] odp-execute: Add ISA implementation of set_masked IPv6 action

2022-11-25 Thread Eelco Chaudron




On 24 Nov 2022, at 10:30, Emma Finn wrote:

> This commit adds support for the AVX512 implementation of the
> ipv6_set_addrs action as well as an AVX512 implementation of
> updating the L4 checksums.
>
> Signed-off-by: Emma Finn 

Thanks Emma for the v4, I have one question and a couple of style issues. To 
speed things up I just provide the diff for the style issues.

I was not able to do any actual testing, as my system did not have the 
avx512vbmi extension :(

Cheers,

Eelco

> ---

Style issues diff:

diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
index 82ff7e647..f798d6708 100644
--- a/lib/odp-execute-avx512.c
+++ b/lib/odp-execute-avx512.c
@@ -20,9 +20,9 @@

 #include 
 #include 
-#include 
 #include 
 #include 
+#include 

 #include "csum.h"
 #include "dp-packet.h"
@@ -547,8 +547,8 @@ avx512_ipv6_sum_header(__m512i ip6_header)
  * horizontal add. */
 __m256i v_swap32a = _mm256_setr_epi32(0x0, 0x4, 0xF, 0xF,
   0xF, 0xF, 0xF, 0xF);
-v_delta = _mm256_permutexvar_epi32(v_swap32a, v_delta);

+v_delta = _mm256_permutexvar_epi32(v_swap32a, v_delta);
 v_delta = _mm256_hadd_epi32(v_delta, v_zeros);
 v_delta = _mm256_hadd_epi16(v_delta, v_zeros);

@@ -562,7 +562,7 @@ avx512_ipv6_addr_csum_delta(__m512i old_header, __m512i 
new_header)
 {
 uint16_t old_delta = avx512_ipv6_sum_header(old_header);
 uint16_t new_delta = avx512_ipv6_sum_header(new_header);
-uint32_t csum_delta = (uint16_t)~old_delta + new_delta;
+uint32_t csum_delta = ((uint16_t) ~old_delta) + new_delta;

 return  ~csum_finish(csum_delta);
 }
@@ -606,15 +606,15 @@ action_avx512_ipv6_set_addrs(struct dp_packet_batch 
*batch,
 __m512i v_key_shuf = _mm512_permutexvar_epi8(v_shuffle, v_key);
 __m512i v_mask_shuf = _mm512_permutexvar_epi8(v_shuffle, v_mask);

-
 /* Set the v_zero register to all zero's. */
 const __m128i v_zeros = _mm_setzero_si128();
+
 /* Set the v_all_ones register to all one's. */
 const __m128i v_all_ones = _mm_cmpeq_epi16(v_zeros, v_zeros);

-/* Load ip6 src and dst respectively into 128-bit wide registers. */
+/* Load ip6 src and dst masks respectively into 128-bit wide registers. */
 __m128i v_src = _mm_loadu_si128((void *) mask);
-__m128i v_dst = _mm_maskz_loadu_epi64(0xC,(void *) mask);
+__m128i v_dst = _mm_maskz_loadu_epi64(0xC, (void *) mask);

 /* Perform a bitwise OR between src and dst registers. */
 __m128i v_or = _mm_or_si128(v_src, v_dst);

> v4:
>   - Reworked and moved check for checksum outside loop.
>   - Code cleanup based on review from Eelco.
> v3:
>   - Added a runtime check for AVX512 vbmi.
> v2:
>   - Added check for availbility of s6_addr32 field of struct in6_addr.
>   - Fixed network headers for freebsd builds.
>



> +/* Load ip6 src and dst respectively into 128-bit wide registers. */
> +__m128i v_src = _mm_loadu_si128((void *) mask);
> +__m128i v_dst = _mm_maskz_loadu_epi64(0xC,(void *) mask);

Guess it might be me, but I do not understand how _mm_maskz_loadu_epi64() will 
load the dst from the mask.
Looking at the intrinsics guide it will only read the first two 64-bit values, 
but mask points to src?

Should we not just do the following here?

+__m128i v_src = _mm_loadu_si128((void *) &mask->ipv6_src);
+__m128i v_dst = _mm_loadu_si128((void *) &mask->ipv6_dst);

> +
> +/* Perform a bitwise OR between src and dst registers. */
> +__m128i v_or = _mm_or_si128(v_src, v_dst);
> +
> +/* Will return true if any bit has been set in v_or, else it will return
> + * false. */
> +bool do_checksum = !_mm_test_all_zeros(v_or, v_all_ones);
> +
> +DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
> +struct ovs_16aligned_ip6_hdr *nh = dp_packet_l3(packet);
> +
> +/* Load the 40 bytes of the IPv6 header. */
> +__m512i v_packet = _mm512_maskz_loadu_epi64(0x1F, (void *) nh);
> +
> +/* AND the v_pkt_mask to the packet data (v_packet). */
> +__m512i v_pkt_masked = _mm512_andnot_si512(v_mask_shuf, v_packet);
> +
> +/* OR the new addresses (v_key_shuf) with the masked packet addresses
> + * (v_pkt_masked). */
> +__m512i v_new_hdr = _mm512_or_si512(v_key_shuf, v_pkt_masked);
> +
> +/* If ip6_src or ip6_dst has been modified, L4 checksum needs to
> + * be updated. */
> +if (do_checksum) {
> +uint8_t proto = nh->ip6_nxt;
> +uint16_t delta_checksum = avx512_ipv6_addr_csum_delta(v_packet,
> +  v_new_hdr);
> +
> +if (proto == IPPROTO_UDP) {
> +struct udp_header *uh = dp_packet_l4(packet);
> +
> +if (uh->udp_csum) {
> +uint16_t old_udp_checksum = ~uh->udp_csum;
> +uint32_t udp_checksum = old_udp_checksum + 
> delta_checksum;
> +
> +udp_checksum = csum_finish(ud

Re: [ovs-dev] [PATCH ovn] CI: Update GH actions version

2022-11-25 Thread Dumitru Ceara

On 11/25/22 12:51, Ales Musil wrote:
> On Fri, Nov 25, 2022 at 12:41 PM Dumitru Ceara  wrote:
> 
>> On 11/23/22 16:19, Ales Musil wrote:
>>> As stated by the Github CI:
>>> "Node.js 12 actions are deprecated. For more information see:
>>> https://github.blog/changelog/2022-09-22-github-actions-all-
>>> actions-will-begin-running-on-node16-instead-of-node12/.
>>> Please update the following actions to use Node.js 16:
>>> actions/checkout@v2, actions/setup-python@v2"
>>>
>>> Signed-off-by: Ales Musil 
>>> ---
>>>  .github/workflows/test.yml | 16 
>>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>>
>> Thanks for the patch, Ales!
>>
>> It seems we need to update actions/upload-artifact@v2 to
>> actions/upload-artifact@v3 as well.
>>
>> Also, can you please update the actions in
>> .github/workflos/ovn-kubernetes.yml too?
>>
>> Thanks,
>> Dumitru
>>
>>
> Interesting GH didn't report the upload-artifacts, but it is updated in v2.
> 

For the "Build and Test" workflow we only upload artifacts on failure or
job cancellation.  I'm guessing that's why you missed them.

Thanks,
Dumitru

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

Re: [ovs-dev] [PATCH ovn] CI: Update GH actions version

2022-11-25 Thread Ales Musil

On Fri, Nov 25, 2022 at 12:41 PM Dumitru Ceara  wrote:

> On 11/23/22 16:19, Ales Musil wrote:
> > As stated by the Github CI:
> > "Node.js 12 actions are deprecated. For more information see:
> > https://github.blog/changelog/2022-09-22-github-actions-all-
> > actions-will-begin-running-on-node16-instead-of-node12/.
> > Please update the following actions to use Node.js 16:
> > actions/checkout@v2, actions/setup-python@v2"
> >
> > Signed-off-by: Ales Musil 
> > ---
> >  .github/workflows/test.yml | 16 
> >  1 file changed, 8 insertions(+), 8 deletions(-)
>
>
> Thanks for the patch, Ales!
>
> It seems we need to update actions/upload-artifact@v2 to
> actions/upload-artifact@v3 as well.
>
> Also, can you please update the actions in
> .github/workflos/ovn-kubernetes.yml too?
>
> Thanks,
> Dumitru
>
>
Interesting GH didn't report the upload-artifacts, but it is updated in v2.

Thanks,
Ales

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.comIM: amusil

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

[ovs-dev] [PATCH ovn v2] CI: Update GH actions version

2022-11-25 Thread Ales Musil

As stated by the Github CI:
"Node.js 12 actions are deprecated. For more information see:
https://github.blog/changelog/2022-09-22-github-actions-all-
actions-will-begin-running-on-node16-instead-of-node12/.
Please update the following actions to use Node.js 16:
actions/checkout@v2, actions/setup-python@v2"

Signed-off-by: Ales Musil 
---
v2: Address comments from Dumitru.
---
 .github/workflows/ovn-kubernetes.yml | 14 +++---
 .github/workflows/test.yml   | 24 
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/.github/workflows/ovn-kubernetes.yml 
b/.github/workflows/ovn-kubernetes.yml
index ba6b291ff..344937e53 100644
--- a/.github/workflows/ovn-kubernetes.yml
+++ b/.github/workflows/ovn-kubernetes.yml
@@ -30,7 +30,7 @@ jobs:
 runs-on: ubuntu-20.04
 steps:
 - name: Check out ovn
-  uses: actions/checkout@v2
+  uses: actions/checkout@v3
   with:
 submodules: recursive
 
@@ -42,7 +42,7 @@ jobs:
 mkdir /tmp/_output
 docker save ovn-daemonset-f:dev > /tmp/_output/image.tar
 
-- uses: actions/upload-artifact@v2
+- uses: actions/upload-artifact@v3
   with:
 name: test-image
 path: /tmp/_output/image.tar
@@ -86,13 +86,13 @@ jobs:
   run: sudo eatmydata apt-get remove --auto-remove -y aspnetcore-* 
dotnet-* libmono-* mono-* msbuild php-* php7* ghc-* zulu-*
 
 - name: Set up Go
-  uses: actions/setup-go@v2
+  uses: actions/setup-go@v3
   with:
 go-version: ${{ env.GO_VERSION }}
   id: go
 
 - name: Check out ovn-kubernetes
-  uses: actions/checkout@v2
+  uses: actions/checkout@v3
   with:
   path: src/github.com/ovn-org/ovn-kubernetes
   repository: ovn-org/ovn-kubernetes
@@ -109,7 +109,7 @@ jobs:
   run: |
 sudo ufw disable
 
-- uses: actions/download-artifact@v2
+- uses: actions/download-artifact@v3
   with:
 name: test-image
 
@@ -130,7 +130,7 @@ jobs:
 
 - name: Upload Junit Reports
   if: always()
-  uses: actions/upload-artifact@v2
+  uses: actions/upload-artifact@v3
   with:
 name: kind-junit-${{ env.JOB_NAME }}-${{ github.run_id }}
 path: 'src/github.com/ovn-org/ovn-kubernetes/test/_artifacts/*.xml'
@@ -144,7 +144,7 @@ jobs:
 
 - name: Upload logs
   if: always()
-  uses: actions/upload-artifact@v2
+  uses: actions/upload-artifact@v3
   with:
 name: kind-logs-${{ env.JOB_NAME }}-${{ github.run_id }}
 path: /tmp/kind/logs
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 88c48dd2c..f7ad08a57 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -58,20 +58,20 @@ jobs:
 steps:
 - name: checkout
   if: github.event_name == 'push' || github.event_name == 'pull_request'
-  uses: actions/checkout@v2
+  uses: actions/checkout@v3
   with:
 submodules: recursive
 
 # For weekly runs, don't update submodules
 - name: checkout without submodule
   if: github.event_name == 'schedule'
-  uses: actions/checkout@v2
+  uses: actions/checkout@v3
 
 # Weekly runs test using OVS master instead of the
 # submodule.
 - name: checkout OVS master
   if: github.event_name == 'schedule'
-  uses: actions/checkout@v2
+  uses: actions/checkout@v3
   with:
 repository: 'openvswitch/ovs'
 path: 'ovs'
@@ -100,7 +100,7 @@ jobs:
 echo "$HOME/.local/bin" >> $GITHUB_PATH
 
 - name: set up python
-  uses: actions/setup-python@v2
+  uses: actions/setup-python@v4
   with:
 python-version: '3.x'
 
@@ -113,9 +113,9 @@ jobs:
 - name: copy logs on failure
   if: failure() || cancelled()
   run: |
-# upload-artifact@v2 throws exceptions if it tries to upload socket
+# upload-artifact@v3 throws exceptions if it tries to upload socket
 # files and we could have some socket files in testsuite.dir.
-# Also, upload-artifact@v2 doesn't work well enough with wildcards.
+# Also, upload-artifact@v3 doesn't work well enough with wildcards.
 # So, we're just archiving everything here to avoid any issues.
 mkdir logs
 cp config.log ./logs/
@@ -127,7 +127,7 @@ jobs:
 
 - name: upload logs on failure
   if: failure() || cancelled()
-  uses: actions/upload-artifact@v2
+  uses: actions/upload-artifact@v3
   with:
 name: logs-linux-${{ join(matrix.cfg.*, '-') }}
 path: logs.tgz
@@ -146,18 +146,18 @@ jobs:
 steps:
 - name: checkout
   if: github.event_name == 'push' || github.event_name == 'pull_request'
-  uses: actions/checkout@v2
+  uses: actions/checkout@v3
   with:
 submodules: recursive
 # For weekly runs, don't update submodules
 - name: checkout without submodule
   if: github.event_name == 'schedule'
-  uses: actions/checkout@v2
+

Re: [ovs-dev] [PATCH ovn] CI: Update GH actions version

2022-11-25 Thread Dumitru Ceara

On 11/23/22 16:19, Ales Musil wrote:
> As stated by the Github CI:
> "Node.js 12 actions are deprecated. For more information see:
> https://github.blog/changelog/2022-09-22-github-actions-all-
> actions-will-begin-running-on-node16-instead-of-node12/.
> Please update the following actions to use Node.js 16:
> actions/checkout@v2, actions/setup-python@v2"
> 
> Signed-off-by: Ales Musil 
> ---
>  .github/workflows/test.yml | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)


Thanks for the patch, Ales!

It seems we need to update actions/upload-artifact@v2 to
actions/upload-artifact@v3 as well.

Also, can you please update the actions in
.github/workflos/ovn-kubernetes.yml too?

Thanks,
Dumitru

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

[ovs-dev] [PATCH ovn] ovn-util: Bump OVN internal minor version for new stages.

2022-11-25 Thread Dumitru Ceara

A few logical pipeline stages have been added.  The internal minor
version must be bumped every time that happens.

Fixes: d3926b433e44 ("northd: rely on new actions for lb affinity")
Signed-off-by: Dumitru Ceara 
---
 lib/ovn-util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ovn-util.c b/lib/ovn-util.c
index 868472ace0..36ca96dbc4 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -817,7 +817,7 @@ ip_address_and_port_from_lb_key(const char *key, char 
**ip_address,
  *
  * This value is also used to handle some backward compatibility during
  * upgrading. It should never decrease or rewind. */
-#define OVN_INTERNAL_MINOR_VER 5
+#define OVN_INTERNAL_MINOR_VER 6
 
 /* Returns the OVN version. The caller must free the returned value. */
 char *
-- 
2.31.1

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

Re: [ovs-dev] [PATCH ovn v4 2/2] northd: Allow related traffic through LB

2022-11-25 Thread Dumitru Ceara

On 11/22/22 10:29, Ales Musil wrote:
> In order to allow related traffic use the
> new action ct_commit_nat, which ensures that
> the traffic is commited and NATted. In combination
> with match on ct.rel it allows the related traffic
> to go through with correct NAT being applied.
> 
> Reported-at: https://bugzilla.redhat.com/2126083
> Signed-off-by: Ales Musil 
> ---
> v2: Add e2e test case.
> v3: Rebase on current main.
> Address comments from Mark.
> ---
>  northd/northd.c |  29 --
>  northd/ovn-northd.8.xml |  29 --
>  tests/ovn-northd.at | 210 +---
>  tests/ovn.at|  10 +-
>  tests/system-ovn.at | 135 ++
>  5 files changed, 292 insertions(+), 121 deletions(-)
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index 00ff8f933..9adfd4abb 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -6707,7 +6707,8 @@ build_acls(struct ovn_datapath *od, const struct 
> chassis_features *features,
>  /* Ingress and Egress ACL Table (Priority 65535).
>   *
>   * Allow traffic that is related to an existing conntrack entry that
> - * has not been marked for deletion (ct_mark.blocked).
> + * has not been marked for deletion (ct_mark.blocked). At the same
> + * time apply NAT on this traffic.
>   *
>   * This is enforced at a higher priority than ACLs can be defined.
>   *
> @@ -6720,9 +6721,9 @@ build_acls(struct ovn_datapath *od, const struct 
> chassis_features *features,
>use_ct_inv_match ? " && !ct.inv" : "",
>ct_blocked_match);
>  ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX - 3,
> -  ds_cstr(&match), "next;");
> +  ds_cstr(&match), "ct_commit_nat;");
>  ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX - 3,
> -  ds_cstr(&match), "next;");
> +  ds_cstr(&match), "ct_commit_nat;");
>  
>  /* Ingress and Egress ACL Table (Priority 65532).
>   *
> @@ -10249,16 +10250,16 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip 
> *lb_vip,
>  int prio = 110;
>  if (lb_vip->vip_port) {
>  prio = 120;
> -new_match = xasprintf("ct.new && %s && %s && "
> +new_match = xasprintf("ct.new && !ct.rel && %s && %s && "
>REG_ORIG_TP_DPORT_ROUTER" == %d",
>ds_cstr(match), lb->proto, lb_vip->vip_port);
> -est_match = xasprintf("ct.est && %s && %s && "
> +est_match = xasprintf("ct.est && !ct.rel && %s && %s && "
>REG_ORIG_TP_DPORT_ROUTER" == %d && %s == 1",
>ds_cstr(match), lb->proto, lb_vip->vip_port,
>ct_natted);

Can't we just skip the !ct.rel match? I think these flows will always
have lower priority than the ones matching on ct.rel.

>  } else {
> -new_match = xasprintf("ct.new && %s", ds_cstr(match));
> -est_match = xasprintf("ct.est && %s && %s == 1",
> +new_match = xasprintf("ct.new && !ct.rel && %s", ds_cstr(match));
> +est_match = xasprintf("ct.est && !ct.rel && %s && %s == 1",
>ds_cstr(match), ct_natted);
>  }
>  

Thanks,
Dumitru

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

Re: [ovs-dev] [PATCH ovn v4 1/2] actions: Add new action called ct_commit_nat

2022-11-25 Thread Dumitru Ceara

On 11/22/22 10:29, Ales Musil wrote:
> Add action called ct_commit_nat, that performs
> NAT while committing the connection. This is
> useful for related traffic on which we need
> to perform NAT, mainly ICMP. We need to
> commit due to design decision of OvS[0]:
> 
> "Connections identified as rel are separate from
> the originating connection and must be committed separately."
> 
> [0] http://www.openvswitch.org/support/dist-docs/ovs-fields.7.txt
> 
> Reported-at: https://bugzilla.redhat.com/2126083
> Acked-by: Mark Michelson 
> Signed-off-by: Ales Musil 
> ---
> v3: Rebase on current main.
> ---

Hi Ales,

The change looks mostly OK to me.  I just have a few comments below.

>  include/ovn/actions.h |  3 +++
>  lib/actions.c | 42 +-
>  ovn-sb.xml| 12 
>  tests/ovn.at  |  5 +
>  utilities/ovn-trace.c | 34 ++
>  5 files changed, 95 insertions(+), 1 deletion(-)
> 
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index fdb6ab08b..d1776d684 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -74,6 +74,7 @@ struct ovn_extend_table;
>  OVNACT(CT_LB_MARK,ovnact_ct_lb)   \
>  OVNACT(SELECT,ovnact_select)  \
>  OVNACT(CT_CLEAR,  ovnact_null)\
> +OVNACT(CT_COMMIT_NAT, ovnact_ct_nat)  \
>  OVNACT(CLONE, ovnact_nest)\
>  OVNACT(ARP,   ovnact_nest)\
>  OVNACT(ICMP4, ovnact_nest)\
> @@ -277,6 +278,8 @@ struct ovnact_ct_nat {

Can you please add OVNACT_CT_COMMIT_NAT to the comment just before
'struct ovnact_ct_nat {'?

> uint16_t port_hi;
>  } port_range;
>  
> +bool commit;/* Explicit commit action. */
> +
>  uint8_t ltable; /* Logical table ID of next table. */
>  };
>  
> diff --git a/lib/actions.c b/lib/actions.c
> index b59f364bf..fb688eeb1 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -920,6 +920,7 @@ parse_ct_nat(struct action_context *ctx, const char *name,
>  return;
>  }
>  cn->ltable = ctx->pp->cur_ltable + 1;
> +cn->commit = false;
>  
>  if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
>  if (ctx->lexer->token.type != LEX_T_INTEGER
> @@ -929,9 +930,11 @@ parse_ct_nat(struct action_context *ctx, const char 
> *name,
>  return;
>  }
>  if (ctx->lexer->token.format == LEX_F_IPV4) {
> +cn->commit = true;
>  cn->family = AF_INET;
>  cn->ipv4 = ctx->lexer->token.value.ipv4;
>  } else if (ctx->lexer->token.format == LEX_F_IPV6) {
> +cn->commit = true;
>  cn->family = AF_INET6;
>  cn->ipv6 = ctx->lexer->token.value.ipv6;
>  }
> @@ -1004,6 +1007,24 @@ parse_CT_SNAT_IN_CZONE(struct action_context *ctx)
>   ovnact_put_CT_SNAT_IN_CZONE(ctx->ovnacts));
>  }
>  
> +static void
> +parse_CT_COMMIT_NAT(struct action_context *ctx)
> +{
> +add_prerequisite(ctx, "ip");
> +
> +if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
> +lexer_error(ctx->lexer,
> +"\"ct_commit_related\" action not allowed in last 
> table.");
> +return;
> +}
> +
> +struct ovnact_ct_nat *cn = ovnact_put_CT_COMMIT_NAT(ctx->ovnacts);
> +cn->commit = true;
> +cn->ltable = ctx->pp->cur_ltable + 1;
> +cn->family = AF_UNSPEC;
> +cn->port_range.exists = false;
> +}
> +
>  static void
>  format_ct_nat(const struct ovnact_ct_nat *cn, const char *name, struct ds *s)
>  {
> @@ -1053,6 +1074,12 @@ format_CT_SNAT_IN_CZONE(const struct ovnact_ct_nat 
> *cn, struct ds *s)
>  format_ct_nat(cn, "ct_snat_in_czone", s);
>  }
>  
> +static void
> +format_CT_COMMIT_NAT(const struct ovnact_ct_nat *cn OVS_UNUSED, struct ds *s)
> +{
> +ds_put_cstr(s, "ct_commit_nat;");
> +}
> +
>  static void
>  encode_ct_nat(const struct ovnact_ct_nat *cn,
>const struct ovnact_encode_params *ep,
> @@ -1104,7 +1131,7 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
>  
>  ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
>  ct = ofpacts->header;
> -if (cn->family == AF_INET || cn->family == AF_INET6) {
> +if (cn->commit) {
>  ct->flags |= NX_CT_F_COMMIT;
>  }
>  ofpact_finish(ofpacts, &ct->ofpact);
> @@ -1143,6 +1170,17 @@ encode_CT_SNAT_IN_CZONE(const struct ovnact_ct_nat *cn,
>  encode_ct_nat(cn, ep, true, ep->common_nat_ct_zone, ofpacts);
>  }
>  
> +static void
> +encode_CT_COMMIT_NAT(const struct ovnact_ct_nat *cn,
> + const struct ovnact_encode_params *ep,
> + struct ofpbuf *ofpacts)
> +{
> +enum mf_field_id zone = ep->is_switch
> +? MFF_LOG_CT_ZONE
> +: MFF_LOG_DNAT_ZONE;
> +encode_ct_nat(cn, ep, false, zone, ofpacts);

Re: [ovs-dev] [OVN v15] OVN - Add Support for Remote Port Mirroring

2022-11-25 Thread Abhiram R N

Hi Numan,

Thanks for your review.

On Fri, Nov 25, 2022 at 8:47 AM Numan Siddique  wrote:

> On Thu, Nov 24, 2022 at 9:14 AM Abhiram R N  wrote:
> >
> > 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.
> >
> > 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 
> > ---
> > v14 --> v15: Addressed comments of Ihar from v14 in function
> >  ovn_port_mirror_handle_lport (mirror.c)
> >  i.e. to have both mirror_delete AND mirror_create
> >  regardless of which case it is:
> >  n_mirror_rules == shash_count(), or <, or >.
> >
> > Ihar,
> >  Regarding the recomputes you asked I think we have to wait
> > for Numan's comments.
> >
>
>
> Hi Abhiram,
>
> Thanks for addressing the review comments and for your patience.
>
> I've a few comments about this patch.
>
> 1. This patch seems to be very big and it can be split into a couple of
> small
>patches. I'd suggest you split into following patches
>- Patch 1 can add the new schemas for port mirroring in both NB and SB
>  and ovn-nbctl related changes.
>- Patch 2 - only ovn-northd changes which syncs the NB port mirrors to
>  SB port mirrrors.
>- Patch 3 - ovn-controller changes which create ovs mirrors.
>
>  This would make reviewing the patch series easier. I'm sorry, I should've
>  asked you to split earlier.
>
Sure. I will split and send the next one. No issues.

>
>
> 2. The engine_node 'en_port_mirror' added in this patch is no way related
> to
>flow output and it just syncs and creates OVS mirrors.
>The mirror.c added in this patch is way too complicated and confusing.
> I'm
>afraid it will be difficult to maintain it and enhance it.
>
>Since it only syncs the SB mirrors to OVS mirrors, it seems to me
>we can avoid the engine changes completely.
>Instead I think we can do the mirror syncs similar to patch.c,
> chassis.c,
>and encaps.c.
>
>Please take a look at those files.
>In the mirror.c we can have the below functions
>   - mirror_init()
>   - mirror_destroy()
>   - mirror_run() -> This function should first iterate over the SB
> mirrors,
> OVS mirrors and local bindings and then create, update or delete
> the
> ovs mirrors.
>ovn-controller should call mirror_run() just like it calls
> patch_run(), encaps_run() etc.
>
>The only drop back of this approach is that there is no incremental
> handling.
>Whenever ovn-controller loop wakes up, it will call mirror_run().
>I think this is ok to start with.  If there are scale concerns, we can
>add engine support for the mirrors.  This can be a future task.
>
>To start with and to make the implementation cleaner and less
> complicated
>I'd suggest this.
>
Okay sure.

>
>I'm sorry that I didn't provide these comments in my earlier reviews.
>Earlier I thought it should be straightforward to add this feature using
>the I-P engine.
>
> I did a quick implementation on top of your patch.  It can be found here
>   - https://github.com/numansiddique/ovn/commits/port_mirror_local_hack
>   -
> https://github.com/numansiddique/ovn/commit/615d28d4146964a5f6c7b7d58c9c947b363b0a7b
>
> I'd suggest you refer to this and use this.  Feel free to have your own
> implementation if you prefer.
>
Agreed, without the engine it's simpler. I took your patch and updated my
repo and tested it.
Only 1 issue I found.
Update of a single field of NB mirror was not getting propagated to OVS
mirror like below for example.
check ovn-nbctl set mirror . type=erspan

So I have modified ONLY below code by adding an 'else' part and
used the  function check_and_update_interface_table from old code to update
it.

Also retained function 'create_and_set_options' since create_ovs_mirror and
'check_and_update_interface_table' both need to set options now.
It's working as expected with the change below.

if (m->sb_mirror && !m->ovs_mirror) {
create_ovs_mirror(m, ovs_idl_txn, br_int);
} else {
/* Since mirror already exists in OVS, look for updates */
check_and_update_interface_table(m->sb_mirror, m->ovs_mirror);
}

Apart from that I didn't find any issue as of now.

I will do a little bit more testing and re-submit the patch set

Thanks & Regards,
Abhiram R N


> Thanks
> Numan
>

Re: [ovs-dev] [PATCH ovn v2] northd: Improve the LB affinity code

2022-11-25 Thread 0-day Robot

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

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


checkpatch:
ERROR: Co-author Dumitru Ceara  needs to sign off.
Lines checked: 716, Warnings: 0, Errors: 1


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

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

Re: [ovs-dev] [PATCH ovn v5 0/2] Allow related traffic for LB

2022-11-25 Thread Dumitru Ceara

On 11/22/22 16:03, Ales Musil wrote:
> The related traffic wasn't correctly forwarded
> through the LB, the main issue was that the
> traffic was not NATted. This series allows
> the NAT to be applied and the traffic should
> arrive with correct addresses.
> ---
> v2: Add e2e test case.
> v3: Rebase on top of main.
> Address comments from Mark.
> v4: Rebase on top of main.
> v5: Add feature flag for backward compatibility.

As discussed offline, we don't expect to backport this feature.  So v4
should be reviewed instead.

Thanks,
Dumitru

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

[ovs-dev] [PATCH ovn v2] northd: Improve the LB affinity code

2022-11-25 Thread Ales Musil

Improve the affinity code to reuse ds buffers as much as possible
without constantly repeating some parts. Add ct.new for the LB flows
so it is clear that the commit happens only when we have a new
connection.

Co-authored-by: Dumitru Ceara 
Signed-off-by: Ales Musil 
---
v2: Rebase on top of main.
Address comments and suggestions from Dumitru.
---
 northd/northd.c | 430 
 northd/ovn-northd.8.xml |  15 +-
 tests/ovn-northd.at |  59 +-
 tests/system-ovn.at |   8 +-
 4 files changed, 369 insertions(+), 143 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 040f46e1a..b1578589d 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -243,7 +243,6 @@ enum ovn_stage {
 #define REGBIT_DST_NAT_IP_LOCAL "reg9[4]"
 #define REGBIT_KNOWN_ECMP_NH"reg9[5]"
 #define REGBIT_KNOWN_LB_SESSION "reg9[6]"
-#define REG
 
 /* Register to store the eth address associated to a router port for packets
  * received in S_ROUTER_IN_ADMISSION.
@@ -6948,102 +6947,137 @@ build_lb_rules_pre_stateful(struct hmap *lflows, 
struct ovn_northd_lb *lb,
 }
 }
 
-/* Builds the logical flows related to load balancer affinity in:
- * - Ingress Table 11: Load balancing affinity check
- * - Ingress Table 12: LB
- * - Ingress Table 13: Load balancing affinity learn
+/* Builds the logical router flows related to load balancer affinity.
+ * For a LB configured with 'vip=V:VP' and backends 'B1:BP1,B2:BP2' and
+ * affinity timeout set to T, it generates the following logical flows:
+ * - load balancing affinity check:
+ *   table=lr_in_lb_aff_check, priority=100
+ *  match=(new_lb_match)
+ *  action=(REGBIT_KNOWN_LB_SESSION = chk_lb_aff(); next;)
+ *
+ * - load balancing:
+ *   table=lr_in_dnat, priority=150
+ *  match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4
+ * && REG_LB_AFF_BACKEND_IP4 == B1 && REG_LB_AFF_MATCH_PORT == BP1)
+ *  action=(REG_NEXT_HOP_IPV4 = V; lb_action;
+ *  ct_lb_mark(backends=B1:BP1);)
+ *   table=lr_in_dnat, priority=150
+ *  match=(REGBIT_KNOWN_LB_SESSION == 1 && ct.new && ip4
+ * && REG_LB_AFF_BACKEND_IP4 == B2 && REG_LB_AFF_MATCH_PORT == BP2)
+ *  action=(REG_NEXT_HOP_IPV4 = V; lb_action;
+ *  ct_lb_mark(backends=B2:BP2);)
+ *
+ * - load balancing affinity learn:
+ *   table=lr_in_lb_aff_learn, priority=100
+ *  match=(REGBIT_KNOWN_LB_SESSION == 0
+ * && ct.new && ip4
+ * && REG_ORIG_DIP_IPV4 == V && ip4.dst == B1 && tcp.dst == BP1)
+ *  action=(commit_lb_aff(vip = "V:VP", backend = "B1:BP1",
+ *proto = tcp, timeout = T));
+ *   table=lr_in_lb_aff_learn, priority=100
+ *  match=(REGBIT_KNOWN_LB_SESSION == 0
+ * && ct.new && ip4
+ * && REG_ORIG_DIP_IPV4 == V && ip4.dst == B2 && tcp.dst == BP2)
+ *  action=(commit_lb_aff(vip = "V:VP", backend = "B2:BP2",
+ *proto = tcp, timeout = T));
+ *
  */
 static void
-build_lb_affinity_flows(struct hmap *lflows, struct ovn_northd_lb *lb,
-struct ovn_lb_vip *lb_vip, char *check_lb_match,
-char *lb_action, struct ovn_datapath **dplist,
-int n_dplist, bool router_pipeline)
+build_lb_affinity_lr_flows(struct hmap *lflows, struct ovn_northd_lb *lb,
+   struct ovn_lb_vip *lb_vip, char *new_lb_match,
+   char *lb_action, struct ovn_datapath **dplist,
+   int n_dplist)
 {
 if (!lb->affinity_timeout) {
 return;
 }
 
-enum ovn_stage stage0 = router_pipeline ?
-S_ROUTER_IN_LB_AFF_CHECK : S_SWITCH_IN_LB_AFF_CHECK;
+static char *aff_check = REGBIT_KNOWN_LB_SESSION" = chk_lb_aff(); next;";
 struct ovn_lflow *lflow_ref_aff_check = NULL;
 /* Check if we have already a enstablished connection for this
  * tuple and we are in affinity timeslot. */
 uint32_t hash_aff_check = ovn_logical_flow_hash(
-ovn_stage_get_table(stage0), ovn_stage_get_pipeline(stage0), 100,
-check_lb_match, REGBIT_KNOWN_LB_SESSION" = chk_lb_aff(); next;");
+ovn_stage_get_table(S_ROUTER_IN_LB_AFF_CHECK),
+ovn_stage_get_pipeline(S_ROUTER_IN_LB_AFF_CHECK), 100,
+new_lb_match, aff_check);
 
 for (size_t i = 0; i < n_dplist; i++) {
 if (!ovn_dp_group_add_with_reference(lflow_ref_aff_check, dplist[i])) {
 lflow_ref_aff_check = ovn_lflow_add_at_with_hash(
-lflows, dplist[i], stage0, 100, check_lb_match,
-REGBIT_KNOWN_LB_SESSION" = chk_lb_aff(); next;",
-NULL, NULL, &lb->nlb->header_,
+lflows, dplist[i], S_ROUTER_IN_LB_AFF_CHECK, 100,
+new_lb_match, aff_check, NULL, NULL, &lb->nlb->header_,
 OVS_SOURCE_LOCATOR, hash_aff_check);
 }
 }
 
+struct ds aff_action = DS_

38 matches

Mail list logo