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

[lic121]

If lldp didn't change, we are not supposed to trigger backer
revalidation.

Without this patch, bridge_reconfigure() always trigger udpif
revalidator because of lldp.

Signed-off-by: lic121 
---
 lib/ovs-lldp.c |  8 
 lib/ovs-lldp.h |  1 +
 ofproto/ofproto-dpif.c |  5 -
 tests/ofproto-dpif.at  | 33 +
 4 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
index 162311f..bfc69a3 100644
--- a/lib/ovs-lldp.c
+++ b/lib/ovs-lldp.c
@@ -738,6 +738,14 @@ lldp_put_packet(struct lldp *lldp, struct dp_packet 
*packet,
 ovs_mutex_unlock();
 }

+/* Is LLDP enabled?
+ */
+bool
+lldp_is_enabled(struct lldp *lldp)
+{
+return lldp ? lldp->enabled : false;
+}
+
 /* Configures the LLDP stack.
  */
 bool
diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
index 0e536e8..661ac4e 100644
--- a/lib/ovs-lldp.h
+++ b/lib/ovs-lldp.h
@@ -86,6 +86,7 @@ void lldp_run(struct lldpd *cfg);
 bool lldp_should_send_packet(struct lldp *cfg);
 bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
 bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
+bool lldp_is_enabled(struct lldp *lldp);
 void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
 void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
  const struct eth_addr eth_src);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 5737615..c3c1922 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2461,11 +2461,11 @@ set_lldp(struct ofport *ofport_,
 {
 struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
 struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
+bool old_enable = lldp_is_enabled(ofport->lldp);
 int error = 0;

 if (cfg) {
 if (!ofport->lldp) {
-ofproto->backer->need_revalidate = REV_RECONFIGURE;
 ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
 }

@@ -2477,6 +2477,9 @@ set_lldp(struct ofport *ofport_,
 } else if (ofport->lldp) {
 lldp_unref(ofport->lldp);
 ofport->lldp = NULL;
+}
+
+if (lldp_is_enabled(ofport->lldp) != old_enable) {
 ofproto->backer->need_revalidate = REV_RECONFIGURE;
 }

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 1660b08..1ba0aac 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -29,6 +29,39 @@ AT_CHECK([ovs-appctl revalidator/wait])
 OVS_VSWITCHD_STOP
 AT_CLEANUP

+AT_SETUP([ofproto-dpif - lldp revalidator event(REV_RECONFIGURE)])
+OVS_VSWITCHD_START(
+[add-port br0 p1 -- set interface p1 ofport_request=1 type=dummy]
+)
+dnl first revalidation triggered by add interface
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+1
+])
+
+dnl enable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=yes])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+2
+])
+
+dnl disable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=no])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+dnl remove lldp
+AT_CHECK([ovs-vsctl remove interface p1 lldp enable])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([ofproto-dpif - active-backup bonding (with primary)])

 dnl Create br0 with members p1, p2 and p7, creating bond0 with p1 and
--
1.8.3.1

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

[ovs-dev] [PATCH v2 1/2] ofproto-dpif: trigger revalidation when ipfix config set

[lic121]

Currently, ipfix conf creation/deletion don't trigger dpif backer
revalidation. This is not expected, as we need the revalidation
to commit ipfix into xlate. So that xlate can generate ipfix
actions.

This patch covers only new creation/deletion of ipfix config.
Will upload one more patch to cover ipfix option changes.

Signed-off-by: lic121 
---
 ofproto/ofproto-dpif.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index bc3df8e..5737615 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2333,6 +2333,12 @@ set_ipfix(
 dpif_ipfix_unref(di);
 ofproto->ipfix = NULL;
 }
+
+/* TODO: need to test ipfix option changes more than
+ * enable/disable */
+if (new_di || !ofproto->ipfix) {
+ofproto->backer->need_revalidate = REV_RECONFIGURE;
+}
 }

 return 0;
--
1.8.3.1

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

[ovs-dev] [PATCH v2 0/2] fix dpif backer revalidation

[lic121]

v2:
- add TODO comments to clear ipfix patch coverage
- test lldp enabling more than pointer
- add test cases for lldp

ovsdb change or netlink notification trigger bridge_reconfigure.
In bridge_reconfigure, backer->need_revalidate flag is set if backer
revalidation is needed.

This series fix two places where need_revalidate flag is not set
correctly.


lic121 (2):
  ofproto-dpif: trigger revalidation when ipfix config set
  ofproto-dpif: avoid unneccesary backer revalidation

 lib/ovs-lldp.c |  8 
 lib/ovs-lldp.h |  1 +
 ofproto/ofproto-dpif.c | 11 ++-
 tests/ofproto-dpif.at  | 33 +
 4 files changed, 52 insertions(+), 1 deletion(-)

--
1.8.3.1

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

[ovs-dev] fix dpif backer revalidation

[lic121]

v2:
- add TODO comments to clear ipfix patch coverage
- test lldp enabling more than pointer
- add test cases for lldp

ovsdb change or netlink notification trigger bridge_reconfigure.
In bridge_reconfigure, backer->need_revalidate flag is set if backer
revalidation is needed.

This series fix two places where need_revalidate flag is not set
correctly.


lic121 (2):
  ofproto-dpif: trigger revalidation when ipfix config set
  ofproto-dpif: avoid unneccesary backer revalidation

 lib/ovs-lldp.c |  8 
 lib/ovs-lldp.h |  1 +
 ofproto/ofproto-dpif.c | 11 ++-
 tests/ofproto-dpif.at  | 33 +
 4 files changed, 52 insertions(+), 1 deletion(-)

--
1.8.3.1

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

Re: [ovs-dev] RFC for adding P4 Support in OVS

[Limaye, Namrata]

Hi Frode,
 
Thanks for your feedback.
 
We are working to cleanup and package all Stratum code (P4Runtime and 
Openconfig support in C++) in the form of a submodule/library/RPM, so it can be 
used by OVS seamlessly. That will separate out the C and C++ code and OVS will 
contain only the C elements. Does that sound like the right direction? If so, 
please let us know what would be the preferred choice between 
submodule/library/RPM.
 
Thanks
Namrata

-Original Message-
From: Frode Nordahl  
Sent: Tuesday, January 25, 2022 5:37 AM
To: Limaye, Namrata 
Cc: ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] RFC for adding P4 Support in OVS

On Fri, Jan 21, 2022 at 12:52 AM Limaye, Namrata  
wrote:
>
> Hi,
>
> We are working on adding P4 support to OVS and we have recently open-sourced 
> the patches on the IPDK github  -
>https://github.com/ipdk-io/ovs/tree/ovs-with-p4
>
> The architecture document that explains the changes and the upcoming feature 
> list is here -
>
> https://github.com/ipdk-io/ovs/blob/ovs-with-p4/OVS_WITH_P4_ARCH.rst
>
> Here is the link for the user-guide document to run OVS on host - 
> https://github.com/ipdk-io/ipdk/blob/main/build/ovs-with-p4_howto
>
> We have also built a development container that ties in all necessary 
> pieces and runs OVS on container. It also brings up 2 Vagrant VMs and 
> switches traffic between them using a simple P4 and OVS - 
> https://github.com/ipdk-io/ipdk/blob/main/build/README.md
>
> Here is the link to the code-walk video that we did last November - 
> https://www.youtube.com/watch?v=vhRL5SQReQs
>
> We still have work to do to turn these patches into a minimal set that could 
> be upstreamed into OVS.  Once that is done, we would like to upstream these 
> patches to openvswitch. Please let us know your comments.

I took a quick look at this, which should not count as a full review, more 
commentary to help get the discussion going.

First I would like to thank you for your work on getting P4 support into Open 
vSwitch.  The P4 specifications mandate end user programmability for both 
control plane and data plane, so bringing this to OVS is no small feat.

I need to start with some history.   OpenFlow was conceived more than
a decade ago and has fueled the first leg of the SDN revolution. The protocol 
itself is very flexible and extensible, however any additions of match 
operators or actions also requires changes to the data path.
In this stage of the evolution changing the software data path is not 
sufficient as hardware offload is required to achieve the transfer speeds 
expected today (i.e. 100G+). The lead time of reaching consensus for new match 
operators, actions, hardware being designed and manufactured makes the OpenFlow 
extensibility impractical if not unusable.

Despite this OVS's OpenFlow implementation and the likes of OVN brings state of 
the art 100G+ networking to the masses today, and will continue to do so for 
several years.

What P4 promises is a framework and language which would allow reprogramming 
the hardware data plane behavior, targets also exist for outputting code for 
use in software data paths (p4-dpdk-target, eBPF etc).  There will of course 
still be a gap between what features the
P4 specification and language provides and what hardware can actually offload, 
but at least we have the tools to express what we want to do until the hardware 
catches up.

So to sum up, given we will depend on hardware offload in the future, OVS would 
benefit from P4 support to stay relevant for the next decade, and P4 would 
benefit from an OVS implementation to allow the experimentation and critical 
production usage required for it to develop.

As for the proposal itself the approach of importing large chunks of external 
code as sub-modules does feel a bit alien, I do however like this statement 
in``OVS_WITH_P4_ARCH.rst``: " Part of the development process is to prune away 
as much of the Stratum project as possible".

I wonder if we could bring more of the infrastructure required into native OVS 
code?

--
Frode Nordahl

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

Re: [ovs-dev] [PATCH ovn v2] Add LTS section to release documentation.

[Han Zhou]

On Thu, Jan 20, 2022 at 1:40 PM Mark Michelson  wrote:
>
> OVN LTS releases have a lot of ambiguity, so this is intended to codify
> LTS support and cadence.
>
> Signed-off-by: Mark Michelson 
> ---
>  Documentation/internals/release-process.rst | 28 +
>  1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/internals/release-process.rst
b/Documentation/internals/release-process.rst
> index f37c09e51..9db6e7494 100644
> --- a/Documentation/internals/release-process.rst
> +++ b/Documentation/internals/release-process.rst
> @@ -75,16 +75,24 @@ Scheduling`_ for the timing of each stage:
> 2019.10.2, and so on.  The process is the same for these additional
release
> as for a .0 release.
>
> -At most two release branches are formally maintained at any given time:
the
> -latest release and the latest release designed as LTS.  An LTS release
is one
> -that the OVN project has designated as being maintained for a longer
period of
> -time.  Currently, an LTS release is maintained until the next LTS is
chosen.
> -There is not currently a strict guideline on how often a new LTS release
is
> -chosen, but so far it has been about every 2 years.  That could change
based on
> -the current state of OVN development.  For example, we do not want to
designate
> -a new release as LTS that includes disruptive internal changes, as that
may
> -make it harder to support for a longer period of time.  Discussion about
> -choosing the next LTS release occurs on the OVS development mailing list.
> +Long-term Support Releases
> +--
> +
> +The OVN project will periodically designate a release as "long-term
support" or
> +LTS for short. An LTS release has the distinction of being maintained for
> +longer than a standard release.
> +
> +LTS releases will receive bug fixes until the point that another LTS is
> +released. At that point, the old LTS will receive an additional year of
> +critical and security fixes. Critical fixes are those that are required
to
> +ensure basic operation (e.g. memory leak fixes, crash fixes). Security
fixes
> +are those that address concerns about exploitable flaws in OVN and that
have a
> +corresponding CVE report.
> +
> +LTS releases are scheduled to be released once every two years. This
means
> +that any given LTS will receive bug fix support for two years, followed
by
> +one year of critical bug fixes and security fixes.
> +
>
>  Release Numbering
>  -
> --
> 2.31.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[ovs-dev] [PATCH ovn] ovn-northd: Enable change tracking for all SB tables.

[Dumitru Ceara]

I-P OVSDB nodes rely on change tracking to update their own states.
Such nodes are primary inputs to the northd incremental processing
engine and without proper update processing for Southbound tables,
northd might fail to timely reconcile the contents of the Southbound
database.

Fixes: 4597317f16d1 ("northd: Introduce incremental processing for northd")
Signed-off-by: Dumitru Ceara 
---
 northd/ovn-northd.c | 258 +---
 1 file changed, 2 insertions(+), 256 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 793135ede..80303503a 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -513,14 +513,6 @@ update_sequence_numbers(int64_t loop_start_time,
 }
 
 static void
-add_column_noalert(struct ovsdb_idl *idl,
-   const struct ovsdb_idl_column *column)
-{
-ovsdb_idl_add_column(idl, column);
-ovsdb_idl_omit_alert(idl, column);
-}
-
-static void
 usage(void)
 {
 printf("\
@@ -730,256 +722,10 @@ main(int argc, char *argv[])
 unixctl_command_register("nb-connection-status", "", 0, 0,
  ovn_conn_show, ovnnb_idl_loop.idl);
 
-/* We want to detect only selected changes to the ovn-sb db. */
+/* We want to detect all changes to the ovn-sb db. */
 struct ovsdb_idl_loop ovnsb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(
 ovsdb_idl_create(ovnsb_db, _idl_class, true, true));
-ovsdb_idl_add_table(ovnsb_idl_loop.idl, _table_sb_global);
-add_column_noalert(ovnsb_idl_loop.idl, _sb_global_col_nb_cfg);
-add_column_noalert(ovnsb_idl_loop.idl, _sb_global_col_options);
-add_column_noalert(ovnsb_idl_loop.idl, _sb_global_col_ipsec);
-ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
-   _sb_global_col_connections);
-
-ovsdb_idl_add_table(ovnsb_idl_loop.idl, _table_logical_flow);
-add_column_noalert(ovnsb_idl_loop.idl,
-   _logical_flow_col_logical_datapath);
-add_column_noalert(ovnsb_idl_loop.idl,
-   _logical_flow_col_logical_dp_group);
-add_column_noalert(ovnsb_idl_loop.idl, _logical_flow_col_pipeline);
-add_column_noalert(ovnsb_idl_loop.idl, _logical_flow_col_table_id);
-add_column_noalert(ovnsb_idl_loop.idl, _logical_flow_col_priority);
-add_column_noalert(ovnsb_idl_loop.idl, _logical_flow_col_match);
-add_column_noalert(ovnsb_idl_loop.idl, _logical_flow_col_actions);
-add_column_noalert(ovnsb_idl_loop.idl,
-   _logical_flow_col_controller_meter);
-ovsdb_idl_add_column(ovnsb_idl_loop.idl,
- _logical_flow_col_external_ids);
-
-ovsdb_idl_add_table(ovnsb_idl_loop.idl,
-_table_logical_dp_group);
-add_column_noalert(ovnsb_idl_loop.idl,
-   _logical_dp_group_col_datapaths);
-
-ovsdb_idl_add_table(ovnsb_idl_loop.idl, _table_multicast_group);
-add_column_noalert(ovnsb_idl_loop.idl,
-   _multicast_group_col_datapath);
-add_column_noalert(ovnsb_idl_loop.idl,
-   _multicast_group_col_tunnel_key);
-add_column_noalert(ovnsb_idl_loop.idl, _multicast_group_col_name);
-add_column_noalert(ovnsb_idl_loop.idl, _multicast_group_col_ports);
-
-ovsdb_idl_add_table(ovnsb_idl_loop.idl, _table_datapath_binding);
-add_column_noalert(ovnsb_idl_loop.idl,
-   _datapath_binding_col_tunnel_key);
-add_column_noalert(ovnsb_idl_loop.idl,
-   _datapath_binding_col_load_balancers);
-ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
-   _datapath_binding_col_external_ids);
-
-ovsdb_idl_add_table(ovnsb_idl_loop.idl, _table_port_binding);
-add_column_noalert(ovnsb_idl_loop.idl, _port_binding_col_datapath);
-add_column_noalert(ovnsb_idl_loop.idl,
-   _port_binding_col_logical_port);
-add_column_noalert(ovnsb_idl_loop.idl,
-   _port_binding_col_tunnel_key);
-add_column_noalert(ovnsb_idl_loop.idl,
-   _port_binding_col_parent_port);
-add_column_noalert(ovnsb_idl_loop.idl, _port_binding_col_tag);
-add_column_noalert(ovnsb_idl_loop.idl, _port_binding_col_type);
-ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
-   _port_binding_col_options);
-add_column_noalert(ovnsb_idl_loop.idl, _port_binding_col_mac);
-add_column_noalert(ovnsb_idl_loop.idl,
-   _port_binding_col_nat_addresses);
-ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
-   _port_binding_col_requested_chassis);
-ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
-   _port_binding_col_chassis);
-ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
-   _port_binding_col_gateway_chassis);
-ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
-   

Re: [ovs-dev] [PATCH ovn v2] Add LTS section to release documentation.

[Frode Nordahl]

On Thu, Jan 20, 2022 at 10:40 PM Mark Michelson  wrote:
>
> OVN LTS releases have a lot of ambiguity, so this is intended to codify
> LTS support and cadence.
>
> Signed-off-by: Mark Michelson 
> ---
>  Documentation/internals/release-process.rst | 28 +
>  1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/internals/release-process.rst 
> b/Documentation/internals/release-process.rst
> index f37c09e51..9db6e7494 100644
> --- a/Documentation/internals/release-process.rst
> +++ b/Documentation/internals/release-process.rst
> @@ -75,16 +75,24 @@ Scheduling`_ for the timing of each stage:
> 2019.10.2, and so on.  The process is the same for these additional 
> release
> as for a .0 release.
>
> -At most two release branches are formally maintained at any given time: the
> -latest release and the latest release designed as LTS.  An LTS release is one
> -that the OVN project has designated as being maintained for a longer period 
> of
> -time.  Currently, an LTS release is maintained until the next LTS is chosen.
> -There is not currently a strict guideline on how often a new LTS release is
> -chosen, but so far it has been about every 2 years.  That could change based 
> on
> -the current state of OVN development.  For example, we do not want to 
> designate
> -a new release as LTS that includes disruptive internal changes, as that may
> -make it harder to support for a longer period of time.  Discussion about
> -choosing the next LTS release occurs on the OVS development mailing list.
> +Long-term Support Releases
> +--
> +
> +The OVN project will periodically designate a release as "long-term support" 
> or
> +LTS for short. An LTS release has the distinction of being maintained for
> +longer than a standard release.
> +
> +LTS releases will receive bug fixes until the point that another LTS is
> +released. At that point, the old LTS will receive an additional year of
> +critical and security fixes. Critical fixes are those that are required to
> +ensure basic operation (e.g. memory leak fixes, crash fixes). Security fixes
> +are those that address concerns about exploitable flaws in OVN and that have 
> a
> +corresponding CVE report.
> +
> +LTS releases are scheduled to be released once every two years. This means
> +that any given LTS will receive bug fix support for two years, followed by
> +one year of critical bug fixes and security fixes.
> +
>
>  Release Numbering
>  -
> --
> 2.31.1

Acked-by: Frode Nordahl 

-- 
Frode Nordahl

>
> ___
> 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] acinclude: Detect avx512 vpopcntdq compiler support.

[Gregory Rose]

On 1/26/2022 7:45 PM, William Tu wrote:

Ubuntu Xenial 16.04 is using GCC 5.4 and it does not support
target "-mavx512vpopcntdq" and cuases error
   lib/dpif-netdev-lookup-avx512-gather.c:356:47:
   error: attribute(target("avx512vpopcntdq")) is unknown
GCC 7+ supports vpopcntdq:
https://gcc.gnu.org/gcc-7/changes.html
The patch detects vpopcntdq and disables AVX512 when not found.

Reported-by: Greg Rose 
Signed-off-by: William Tu 
---
  acinclude.m4 | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 5c971e98ce91..0c360fd1ef73 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -77,7 +77,7 @@ dnl Checks if compiler and binutils supports AVX512.
  AC_DEFUN([OVS_CHECK_AVX512], [
OVS_CHECK_BINUTILS_AVX512
OVS_CHECK_CC_OPTION(
-[-mavx512f], [ovs_have_cc_mavx512f=yes], [ovs_have_cc_mavx512f=no])
+[-mavx512f -mavx512vpopcntdq], [ovs_have_cc_mavx512f=yes], 
[ovs_have_cc_mavx512f=no])
AM_CONDITIONAL([HAVE_AVX512F], [test $ovs_have_cc_mavx512f = yes])
if test "$ovs_have_cc_mavx512f" = yes; then
  AC_DEFINE([HAVE_AVX512F], [1],


Tested-by: Greg Rose 
Reviewed-by: Greg Rose 

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

Re: [ovs-dev] [PATCH 2/3] ovsdb-cs: Clear last_id on reconnect if condition changes in-flight.

[Dumitru Ceara]

On 1/27/22 19:09, Han Zhou wrote:
> On Thu, Jan 27, 2022 at 3:29 AM Dumitru Ceara  wrote:
>>
>> On 1/26/22 01:22, Han Zhou wrote:
>>>
>>>
>>> On Tue, Jan 11, 2022 at 8:38 AM Dumitru Ceara >> > wrote:

 When reconnecting, if there are condition changes already sent to the
 server but not yet acked, reset the db's 'last-id', esentially clearing
 the local cache after reconnect.

 This is needed because the client cannot easily differentiate between
 the following cases:
 a. either the server already processed the requested monitor
condition change but the FSM was restarted before the
client was notified.  In this case the client should
clear its local cache because it's out of sync with the
monitor view on the server side.
 b. OR the server hasn't processed the requested monitor
condition change yet.

 Conditions changing at the same time with a reconnection happening are
 rare so the performance impact of this patch should be minimal.

 Also, the tests are updated to cover the fact that we cannot control
 which of the two scenarios ("a" and "b" above) are hit during the test.

 Reported-by: Maxime Coquelin >> >
 Signed-off-by: Dumitru Ceara >> >
>>>
>>> Thanks Dumitru. Could you tell more details about how this problem is
>>> reproduced? Or are there test case failures because of this?
>>
>> Hi Han,
>>
>> Thanks for looking into this.
>>
>> Yes, Maxime reported that the test "2241: simple idl, monitor_cond_since,
>> cluster disconnect - C - tcp" would occasionally fail without this patch.
>>
>>> I still didn't understand how the problem would occur. When a client
>>> reconnects to the server, it should trigger the jsonrpc session
>>> recreation on the server side, so the monitor data for the session
>>> should be recreated, and so there shouldn't be "out of sync" view on the
>>> server side. Did I misunderstand?
>>
>>
>> Going through the test logs we see (simplified and edited, skipping the
>> less relevant parts):
>>
>> 0. initial contents of the "simple" table on the server side:
>> "row": {"i": 1, "r": 1.0, "b": true}
>> "row": {"i": 2, "r": 1.0, "b": true}
>>
>> 1. the client connects for the first time, gets schema, monitors _Server
>>database.
>>
>> 2. the client requests a monitor_cond_since, id=3:
>> method="monitor_cond_since",
>> params=[
>>   "idltest",
>>   ["monid","idltest"],
>>
> {..."simple":[{"where":[false],"columns":["b","ba","i","ia","r","ra","s","sa","u","ua"]}],...},
>>   last_id=0 // last seen transaction id
>> ], id=3
>>
>> 3. the server doesn't need to send any updates, nothing matches "false"
>>and replies, id=3:
>> result=[
>>   false, // requested "last_id" not found in history, will cause the
>>  // client to flush in-memory contents
>>   "13220f46-f13f-4f30-ab19-0af58699e0aa", // last known transaction ID,
>>   // the client will store this
>>   // locally in last_id
>>   {}], id=3
>>
>> 4. the client changes its conditions and sends a monitor_cond_change,
>>id=5:
>> params=[
>>   ["monid","idltest"],["monid","idltest"],
>>   {"simple":[{"where":[["i","==",2]]}]}], id=5
>>
>> ---
>> Client side view:
>> - simple table: empty
>> - last_id: "13220f46-f13f-4f30-ab19-0af58699e0aa"
>> - acked-cond: "simple": false
>> - req-cond:   "simple": {"where":[["i","==",2]]}
>> - new-cond:   
>> ---
>>
>> 5. this new condition matches one row in table "simple" so the server
>>sends an update which includes that row:
>> method="update3",
>> params=[
>>   ["monid","idltest"],
>>   "13220f46-f13f-4f30-ab19-0af58699e0aa",
>>
> {"simple":{"fd500d06-8d0c-4149-9c69-2914538e07e5":{"insert":{"b":true,"r":1,"i":2
>> ]
>>
>> 6. the client inserts the row in its in-memory copy of the database,
>>last_id becomes "13220f46-f13f-4f30-ab19-0af58699e0aa".  However, the
>>condition change is not yet acked by the server!
>> ---
>> Client side view:
>> - simple table:
>>   - row: {"b":true,"r":1,"i":2}
>> - last_id: "13220f46-f13f-4f30-ab19-0af58699e0aa"
>> - acked-cond: "simple": false
>> - req-cond:   "simple": {"where":[["i","==",2]]}
>> - new-cond:   
>> ---
>>
>> 7. the client changes its conditions, doesn't send a monitor_cond_change
>>because there's already one in-flight, not yet acked.
>> ---
>> Client side view:
>> - simple table:
>>   - row: {"b":true,"r":1,"i":2}
>> - last_id: "13220f46-f13f-4f30-ab19-0af58699e0aa"
>> - acked-cond: "simple": false
>> - req-cond:   "simple": {"where":[["i","==",2]]}
>> - new-cond:   "simple": {"where":[["i","==",1]]}
>> ---
>>
>> 8. the connection goes down, ovsdb_cs_db_sync_condition() is called from
>>ovsdb_cs_restart_fsm():
>> ---
>> Client side view:
>> - simple table:
>>   - row: {"b":true,"r":1,"i":2}
>> - last_id: 

Re: [ovs-dev] [PATCH 2/3] ovsdb-cs: Clear last_id on reconnect if condition changes in-flight.

[Han Zhou]

On Thu, Jan 27, 2022 at 3:29 AM Dumitru Ceara  wrote:
>
> On 1/26/22 01:22, Han Zhou wrote:
> >
> >
> > On Tue, Jan 11, 2022 at 8:38 AM Dumitru Ceara  > > wrote:
> >>
> >> When reconnecting, if there are condition changes already sent to the
> >> server but not yet acked, reset the db's 'last-id', esentially clearing
> >> the local cache after reconnect.
> >>
> >> This is needed because the client cannot easily differentiate between
> >> the following cases:
> >> a. either the server already processed the requested monitor
> >>condition change but the FSM was restarted before the
> >>client was notified.  In this case the client should
> >>clear its local cache because it's out of sync with the
> >>monitor view on the server side.
> >> b. OR the server hasn't processed the requested monitor
> >>condition change yet.
> >>
> >> Conditions changing at the same time with a reconnection happening are
> >> rare so the performance impact of this patch should be minimal.
> >>
> >> Also, the tests are updated to cover the fact that we cannot control
> >> which of the two scenarios ("a" and "b" above) are hit during the test.
> >>
> >> Reported-by: Maxime Coquelin  > >
> >> Signed-off-by: Dumitru Ceara  > >
> >
> > Thanks Dumitru. Could you tell more details about how this problem is
> > reproduced? Or are there test case failures because of this?
>
> Hi Han,
>
> Thanks for looking into this.
>
> Yes, Maxime reported that the test "2241: simple idl, monitor_cond_since,
> cluster disconnect - C - tcp" would occasionally fail without this patch.
>
> > I still didn't understand how the problem would occur. When a client
> > reconnects to the server, it should trigger the jsonrpc session
> > recreation on the server side, so the monitor data for the session
> > should be recreated, and so there shouldn't be "out of sync" view on the
> > server side. Did I misunderstand?
>
>
> Going through the test logs we see (simplified and edited, skipping the
> less relevant parts):
>
> 0. initial contents of the "simple" table on the server side:
> "row": {"i": 1, "r": 1.0, "b": true}
> "row": {"i": 2, "r": 1.0, "b": true}
>
> 1. the client connects for the first time, gets schema, monitors _Server
>database.
>
> 2. the client requests a monitor_cond_since, id=3:
> method="monitor_cond_since",
> params=[
>   "idltest",
>   ["monid","idltest"],
>
{..."simple":[{"where":[false],"columns":["b","ba","i","ia","r","ra","s","sa","u","ua"]}],...},
>   last_id=0 // last seen transaction id
> ], id=3
>
> 3. the server doesn't need to send any updates, nothing matches "false"
>and replies, id=3:
> result=[
>   false, // requested "last_id" not found in history, will cause the
>  // client to flush in-memory contents
>   "13220f46-f13f-4f30-ab19-0af58699e0aa", // last known transaction ID,
>   // the client will store this
>   // locally in last_id
>   {}], id=3
>
> 4. the client changes its conditions and sends a monitor_cond_change,
>id=5:
> params=[
>   ["monid","idltest"],["monid","idltest"],
>   {"simple":[{"where":[["i","==",2]]}]}], id=5
>
> ---
> Client side view:
> - simple table: empty
> - last_id: "13220f46-f13f-4f30-ab19-0af58699e0aa"
> - acked-cond: "simple": false
> - req-cond:   "simple": {"where":[["i","==",2]]}
> - new-cond:   
> ---
>
> 5. this new condition matches one row in table "simple" so the server
>sends an update which includes that row:
> method="update3",
> params=[
>   ["monid","idltest"],
>   "13220f46-f13f-4f30-ab19-0af58699e0aa",
>
{"simple":{"fd500d06-8d0c-4149-9c69-2914538e07e5":{"insert":{"b":true,"r":1,"i":2
> ]
>
> 6. the client inserts the row in its in-memory copy of the database,
>last_id becomes "13220f46-f13f-4f30-ab19-0af58699e0aa".  However, the
>condition change is not yet acked by the server!
> ---
> Client side view:
> - simple table:
>   - row: {"b":true,"r":1,"i":2}
> - last_id: "13220f46-f13f-4f30-ab19-0af58699e0aa"
> - acked-cond: "simple": false
> - req-cond:   "simple": {"where":[["i","==",2]]}
> - new-cond:   
> ---
>
> 7. the client changes its conditions, doesn't send a monitor_cond_change
>because there's already one in-flight, not yet acked.
> ---
> Client side view:
> - simple table:
>   - row: {"b":true,"r":1,"i":2}
> - last_id: "13220f46-f13f-4f30-ab19-0af58699e0aa"
> - acked-cond: "simple": false
> - req-cond:   "simple": {"where":[["i","==",2]]}
> - new-cond:   "simple": {"where":[["i","==",1]]}
> ---
>
> 8. the connection goes down, ovsdb_cs_db_sync_condition() is called from
>ovsdb_cs_restart_fsm():
> ---
> Client side view:
> - simple table:
>   - row: {"b":true,"r":1,"i":2}
> - last_id: "13220f46-f13f-4f30-ab19-0af58699e0aa"
> - acked-cond: "simple": false
> - req-cond:   
> - new-cond:   "simple": {"where":[["i","==",1]]}
> ---
>
> 9. the client 

[ovs-dev] [PATCH ovn] controller: add ovn-set-local-ip option

[Vladislav Odintsov]

When transport node has multiple interfaces (vlans) and
ovn-encap-ip on different hosts need to be configured
from different VLANs source IP for encapsulated packet
can be not the same, which is expected by remote system.

Explicitely setting local_ip resolves such problem.

Signed-off-by: Vladislav Odintsov 
---
 controller/encaps.c | 37 +
 controller/ovn-controller.8.xml |  7 +++
 2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/controller/encaps.c b/controller/encaps.c
index 66e0cd8cd..3b0c92931 100644
--- a/controller/encaps.c
+++ b/controller/encaps.c
@@ -23,6 +23,7 @@
 #include "openvswitch/vlog.h"
 #include "lib/ovn-sb-idl.h"
 #include "ovn-controller.h"
+#include "smap.h"
 
 VLOG_DEFINE_THIS_MODULE(encaps);
 
@@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const struct 
sbrec_sb_global *sbg,
 smap_add(, "dst_port", dst_port);
 }
 
+const struct ovsrec_open_vswitch *cfg =
+ovsrec_open_vswitch_table_first(ovs_table);
+
+bool set_local_ip = false;
+if (cfg) {
+/* If the tos option is configured, get it */
+const char *encap_tos = smap_get_def(>external_ids,
+   "ovn-encap-tos", "none");
+
+if (encap_tos && strcmp(encap_tos, "none")) {
+smap_add(, "tos", encap_tos);
+}
+
+/* If ovn-set-local-ip option is configured, get it */
+set_local_ip = smap_get_bool(>external_ids, "ovn-set-local-ip",
+ false);
+}
+
 /* Add auth info if ipsec is enabled. */
 if (sbg->ipsec) {
+set_local_ip = true;
+smap_add(, "remote_name", new_chassis_id);
+}
+
+if (set_local_ip) {
 const struct sbrec_chassis *this_chassis = tc->this_chassis;
 const char *local_ip = NULL;
 
@@ -200,19 +224,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct 
sbrec_sb_global *sbg,
 if (local_ip) {
 smap_add(, "local_ip", local_ip);
 }
-smap_add(, "remote_name", new_chassis_id);
-}
-
-const struct ovsrec_open_vswitch *cfg =
-ovsrec_open_vswitch_table_first(ovs_table);
-/* If the tos option is configured, get it */
-if (cfg) {
-const char *encap_tos = smap_get_def(>external_ids,
-   "ovn-encap-tos", "none");
-
-if (encap_tos && strcmp(encap_tos, "none")) {
-smap_add(, "tos", encap_tos);
-}
 }
 
 /* If there's an existing chassis record that does not need any change,
diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
index e9708fe64..cc9a7d1c2 100644
--- a/controller/ovn-controller.8.xml
+++ b/controller/ovn-controller.8.xml
@@ -304,6 +304,13 @@
 of how many entries there are in the cache.  By default this is set to
 3 (30 seconds).
   
+  external_ids:ovn-set-local-ip
+  
+The boolean flag indicates if ovn-controller when create
+tunnel ports should set local_ip parameter.  Can be
+heplful to pin source outer IP for the tunnel when multiple interfaces
+are used on the host for overlay traffic.
+  
 
 
 
-- 
2.30.0

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

[ovs-dev] [PATCH ovn] ovn-northd: Don't log transaction failures on standby instances.

[Dumitru Ceara]

We still need to try to ovsdb_idl_loop_commit_and_wait() on instances
that are in standby mode.  That's because they need to try to take the
lock.  But if they're in standby they won't actually build a transaction
json and will return early because they don't own the SB lock.

That's reported as an error by ovsdb_idl_loop_commit_and_wait() but we
shouldn't act on it.

Also, to ensure that no DB changes are missed, ovsdb_idl_track_clear()
should be called only on active instances.  The standby or paused ones
will get the incremental updates when they become active.  Otherwise we
would be forced to perform a full recompute when the instance becomes
active.

Fixes: 953832eb17f3 ("ovn-northd: Don't trigger recompute for transactions that 
are in progress.")
Fixes: 4597317f16d1 ("northd: Introduce incremental processing for northd")
Reported-by: Ilya Maximets 
Signed-off-by: Dumitru Ceara 
---
 northd/ovn-northd.c | 35 ---
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 793135ede..4e1e51931 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -1089,6 +1089,26 @@ main(int argc, char *argv[])
 ovnnb_txn, ovnsb_txn,
 _idl_loop);
 }
+
+/* If there are any errors, we force a full recompute in order
+ * to ensure we handle all changes. */
+if (!ovsdb_idl_loop_commit_and_wait(_idl_loop)) {
+VLOG_INFO("OVNNB commit failed, "
+  "force recompute next time.");
+recompute = true;
+}
+
+if (!ovsdb_idl_loop_commit_and_wait(_idl_loop)) {
+VLOG_INFO("OVNSB commit failed, "
+  "force recompute next time.");
+recompute = true;
+}
+ovsdb_idl_track_clear(ovnnb_idl_loop.idl);
+ovsdb_idl_track_clear(ovnsb_idl_loop.idl);
+} else {
+/* Make sure we send any pending requests, e.g., lock. */
+ovsdb_idl_loop_commit_and_wait(_idl_loop);
+ovsdb_idl_loop_commit_and_wait(_idl_loop);
 }
 } else {
 /* ovn-northd is paused
@@ -1112,21 +1132,6 @@ main(int argc, char *argv[])
 ovsdb_idl_wait(ovnsb_idl_loop.idl);
 }
 
-/* If there are any errors, we force a full recompute in order to
-   ensure we handle all changes. */
-if (!ovsdb_idl_loop_commit_and_wait(_idl_loop)) {
-VLOG_INFO("OVNNB commit failed, force recompute next time.");
-recompute = true;
-}
-
-if (!ovsdb_idl_loop_commit_and_wait(_idl_loop)) {
-VLOG_INFO("OVNSB commit failed, force recompute next time.");
-recompute = true;
-}
-
-ovsdb_idl_track_clear(ovnnb_idl_loop.idl);
-ovsdb_idl_track_clear(ovnsb_idl_loop.idl);
-
 unixctl_server_run(unixctl);
 unixctl_server_wait(unixctl);
 memory_wait();
-- 
2.27.0

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

Re: [ovs-dev] [PATCH v1 14/18] python: introduce unit tests

[Adrian Moreno]

On 1/26/22 14:45, Ilya Maximets wrote:

On 1/26/22 14:33, Adrian Moreno wrote:



On 1/26/22 13:43, Ilya Maximets wrote:

On 1/26/22 13:34, Adrian Moreno wrote:

Hi,


diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4
index 772825a71..192147ff5 100644
--- a/m4/openvswitch.m4
+++ b/m4/openvswitch.m4
@@ -393,6 +393,18 @@ AC_DEFUN([OVS_CHECK_SPHINX],
   AC_ARG_VAR([SPHINXBUILD])
   AM_CONDITIONAL([HAVE_SPHINX], [test "$SPHINXBUILD" != none])])

+dnl Checks for pytest.
+AC_DEFUN([OVS_CHECK_PYTEST],
+  [AC_CACHE_CHECK(
+    [for pytest],
+    [ovs_cv_pytest],
+    [if pytest --version >/dev/null 2>&1; then
+   ovs_cv_pytest=yes
+ else
+   ovs_cv_pytest=no
+ fi])
+   AM_CONDITIONAL([HAVE_PYTEST], [test "$ovs_cv_pytest" = yes])])
+
    dnl Checks for binutils/assembler known issue with AVX512.
    dnl Due to backports, we probe assembling a reproducer instead of checking
    dnl binutils version string. More details, including ASM dumps and debug 
here:



Just FYI, In the new version, I'm changing this macro to check for all test 
dependencies.



Is pytest a build dependency or a test dependency?
If it's only for testing, the check should, probably,
be part of the tests/atlocal.in, which is executed
every time before starting the testsuite.






Hi Ilya, that's a good question. It's for testing but since I was
mimicking the flake8 tests, I have included them in the ALL_LOCAL
target. But I think it'd better if they are run as part of "make check",
right?


Yeah, flake8 is just a style check, while these are actual unit tests,
so should belong to a testsuite, I think.



Do you think it's better to add a new test as part of "testsuite.at" or create a 
new "python-testsuite.at" and a new target, e.g: "make check-python"?


A benefit of the former is that it'll be integrated into the CI for free.
A benefit from the latter is that it'll be faster since there is no need to 
build the entire project.





Best regards, Ilya Maximets.







--
Adrián Moreno

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

Re: [ovs-dev] [PATCH 02/10] netdev-offload-tc: Set the correct VLAN_VID and VLAN_PCP masks

[Eelco Chaudron]

On 27 Jan 2022, at 14:33, Eelco Chaudron wrote:

> This change will set the correct VID and PCP masks, as well as the
> ethernet type mask.
>
> Signed-off-by: Eelco Chaudron 
> ---
>  lib/tc.c |   10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/lib/tc.c b/lib/tc.c
> index 77305a105..5e6aa638e 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -568,16 +568,17 @@ nl_parse_flower_vlan(struct nlattr **attrs, struct 
> tc_flower *flower)
>
>  flower->key.encap_eth_type[0] =
>  nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ETH_TYPE]);
> +flower->mask.encap_eth_type[0] = 0x;
>

This needs fixing to avoid sparse to nag about it.

  +flower->mask.encap_eth_type[0] = CONSTANT_HTONS(0x);

>  if (attrs[TCA_FLOWER_KEY_VLAN_ID]) {
>  flower->key.vlan_id[0] =
>  nl_attr_get_u16(attrs[TCA_FLOWER_KEY_VLAN_ID]);
> -flower->mask.vlan_id[0] = 0x;
> +flower->mask.vlan_id[0] = VLAN_VID_MASK >> VLAN_VID_SHIFT;
>  }
>  if (attrs[TCA_FLOWER_KEY_VLAN_PRIO]) {
>  flower->key.vlan_prio[0] =
>  nl_attr_get_u8(attrs[TCA_FLOWER_KEY_VLAN_PRIO]);
> -flower->mask.vlan_prio[0] = 0xff;
> +flower->mask.vlan_prio[0] = VLAN_PCP_MASK >> VLAN_PCP_SHIFT;
>  }
>
>  if (!attrs[TCA_FLOWER_KEY_VLAN_ETH_TYPE]) {
> @@ -590,17 +591,18 @@ nl_parse_flower_vlan(struct nlattr **attrs, struct 
> tc_flower *flower)
>  }
>
>  flower->key.encap_eth_type[1] = flower->key.encap_eth_type[0];
> +flower->mask.encap_eth_type[1] = 0x;

  +flower->mask.encap_eth_type[1] = CONSTANT_HTONS(0x);

>  flower->key.encap_eth_type[0] = encap_ethtype;
>
>  if (attrs[TCA_FLOWER_KEY_CVLAN_ID]) {
>  flower->key.vlan_id[1] =
>  nl_attr_get_u16(attrs[TCA_FLOWER_KEY_CVLAN_ID]);
> -flower->mask.vlan_id[1] = 0x;
> +flower->mask.vlan_id[1] = VLAN_VID_MASK >> VLAN_VID_SHIFT;
>  }
>  if (attrs[TCA_FLOWER_KEY_CVLAN_PRIO]) {
>  flower->key.vlan_prio[1] =
>  nl_attr_get_u8(attrs[TCA_FLOWER_KEY_CVLAN_PRIO]);
> -flower->mask.vlan_prio[1] = 0xff;
> +flower->mask.vlan_prio[1] = VLAN_PCP_MASK >> VLAN_PCP_SHIFT;
>  }
>  }
>
>
> ___
> 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] dpif-netdev: Fix a race condition in deletion of offloaded flows

[Ilya Maximets]

On 1/27/22 13:09, Sriharsha Basavapatna wrote:
> On Thu, Jan 27, 2022 at 4:08 PM Ilya Maximets  wrote:
>>
>> On 1/27/22 07:42, Sriharsha Basavapatna via dev wrote:
>>> In dp_netdev_pmd_remove_flow() we schedule the deletion of an
>>> offloaded flow, if a mark has been assigned to the flow. But if
>>> this occurs in the window in which the offload thread completes
>>> offloading the flow and assigns a mark to the flow, then we miss
>>> deleting the flow. This problem has been observed while adding
>>> and deleting flows in a loop. To fix this, always enqueue flow
>>> deletion regardless of the flow->mark being set.
>>>
>>> Fixes: 241bad15d99a("dpif-netdev: associate flow with a mark id")
>>> Signed-off-by: Sriharsha Basavapatna 
>>> ---
>>>  lib/dpif-netdev.c | 4 +---
>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>> index e28e0b554..22c5f19a8 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -3029,9 +3029,7 @@ dp_netdev_pmd_remove_flow(struct dp_netdev_pmd_thread 
>>> *pmd,
>>>  dp_netdev_simple_match_remove(pmd, flow);
>>>  cmap_remove(>flow_table, node, dp_netdev_flow_hash(>ufid));
>>>  ccmap_dec(>n_flows, odp_to_u32(in_port));
>>> -if (flow->mark != INVALID_FLOW_MARK) {
>>> -queue_netdev_flow_del(pmd, flow);
>>> -}
>>> +queue_netdev_flow_del(pmd, flow);
>>>  flow->dead = true;
>>>
>>>  dp_netdev_flow_unref(flow);
>>>
>>
>> Thanks for the patch, but it looks like that it's not that simple...
>> A lot of tests are crashing in GHA and ASAN reports use-after-free
>> on flow disassociation if the dpif already destroyed:
>>   https://github.com/ovsrobot/ovs/actions/runs/1754866321
>>
>> I think that the problem is that offload thread holds the ref count
>> for PMD thread, but PMD thread structure doesn't hold the ref count
>> for the dp, because it doesn't expect that dp_netdev_pmd structure
>> will be used while PMD thread is destroyed.  I guess, we should fix
>> that.  OTOH, I'm not sure if offload thread actually needs a reference
>> to dp_netdev_pmd structure.  If I didn't miss something, it only
>> uses pmd pointer to access pmd->dp.  So, maybe, dp_offload_thread_item
>> should hold and ref the dp pointer instead?
>>
>> What do you think?  Gaetan?
>>
>> Another point is that queue_netdev_flow_del() should check for
>> netdev_is_flow_api_enabled() to avoid creation of offload threads
>> if offloading is disabled.  But that's good that we didn't have it,
>> as the refcount issue got exposed.  Otherwise it would be hard
>> to reproduce.
>>
>> Best regards, Ilya Maximets.
>>
>> Asan report below, for convenience:
>>
>> =
>> ==17076==ERROR: AddressSanitizer: heap-use-after-free on address 
>> 0x61603080 at pc 0x005e0e38 bp 0x7fe0594309f0 sp 0x7fe0594309e8
>> READ of size 8 at 0x61603080 thread T5 (hw_offload4)
>> #0 0x5e0e37 in mark_to_flow_disassociate 
>> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2556:62
>> #1 0x5dfaf1 in dp_offload_flow 
>> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2821:15
>> #2 0x5df8bd in dp_netdev_flow_offload_main 
>> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2884:17
>> #3 0x73a2fc in ovsthread_wrapper 
>> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/ovs-thread.c:422:12
>> #4 0x7fe0619506da in start_thread 
>> (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
>> #5 0x7fe060ecf71e in clone (/lib/x86_64-linux-gnu/libc.so.6+0x12171e)
>>
>> 0x61603080 is located 0 bytes inside of 576-byte region 
>> [0x61603080,0x616032c0)
>> freed by thread T0 here:
>> #0 0x49640d in free 
>> (/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/vswitchd/ovs-vswitchd+0x49640d)
>> #1 0x5d6652 in dp_netdev_free 
>> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:1980:5
>> #2 0x5f0722 in dp_netdev_unref 
>> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:1991:13
>> #3 0x5cf5e5 in dpif_netdev_close 
>> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2002:5
>> #4 0x5fc393 in dpif_uninit 
>> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif.c:1725:9
>> #5 0x5fc0c0 in dpif_close 
>> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif.c:457:9
>> #6 0x52a972 in close_dpif_backer 
>> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto-dpif.c:715:5
>> #7 0x517f08 in destruct 
>> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto-dpif.c:1860:5
>> #8 0x4f09e0 in ofproto_destroy 
>> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto.c:1728:5
>> #9 0x4c71e4 in bridge_destroy 
>> 

[ovs-dev] [PATCH 10/10] revalidator: Fix datapath statistics update.

[Eelco Chaudron]

Make sure to only update packet and byte counters when valid,
or else this could lead to "temporarily/occasionally"
out-of-sync flow counters.

push_dp_ops() will now handle updating the stats similar to
the way it's handled in revalidate_ukey().

Signed-off-by: Eelco Chaudron 
---
 ofproto/ofproto-dpif-upcall.c |8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 1c9c720f0..ca43ac083 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2432,8 +2432,12 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops, 
size_t n_ops)
 transition_ukey(op->ukey, UKEY_EVICTED);
 push->used = MAX(stats->used, op->ukey->stats.used);
 push->tcp_flags = stats->tcp_flags | op->ukey->stats.tcp_flags;
-push->n_packets = stats->n_packets - op->ukey->stats.n_packets;
-push->n_bytes = stats->n_bytes - op->ukey->stats.n_bytes;
+push->n_packets = (stats->n_packets > op->ukey->stats.n_packets
+   ? stats->n_packets - op->ukey->stats.n_packets
+   : 0);
+push->n_bytes = (stats->n_bytes > op->ukey->stats.n_bytes
+ ? stats->n_bytes - op->ukey->stats.n_bytes
+ : 0);
 ovs_mutex_unlock(>ukey->mutex);
 } else {
 push = stats;

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

[ovs-dev] [PATCH 09/10] netdev-offload-tc: stats should be captured on first action in the list

[Eelco Chaudron]

Statistics should be captured on the first action in a list
of actions, to do this we need to also capture the last update
time stamp from the first action.

Signed-off-by: Eelco Chaudron 
---
 lib/tc.c |  119 +-
 1 file changed, 86 insertions(+), 33 deletions(-)

diff --git a/lib/tc.c b/lib/tc.c
index b37dce22f..b430f1bad 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -994,12 +994,35 @@ nl_parse_flower_flags(struct nlattr **attrs, struct 
tc_flower *flower)
 flower->offloaded_state = nl_get_flower_offloaded_state(attrs);
 }
 
+static int
+get_user_hz(void)
+{
+static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
+static int user_hz = 100;
+
+if (ovsthread_once_start()) {
+user_hz = sysconf(_SC_CLK_TCK);
+ovsthread_once_done();
+}
+
+return user_hz;
+}
+
+static void
+nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower)
+{
+flower->lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
+}
+
 static const struct nl_policy pedit_policy[] = {
-[TCA_PEDIT_PARMS_EX] = { .type = NL_A_UNSPEC,
- .min_len = sizeof(struct tc_pedit),
- .optional = false, },
-[TCA_PEDIT_KEYS_EX]   = { .type = NL_A_NESTED,
-  .optional = false, },
+[TCA_PEDIT_TM] = { .type = NL_A_UNSPEC,
+   .min_len = sizeof(struct tcf_t),
+   .optional = false, },
+[TCA_PEDIT_PARMS_EX] = { .type = NL_A_UNSPEC,
+ .min_len = sizeof(struct tc_pedit),
+ .optional = false, },
+[TCA_PEDIT_KEYS_EX] = { .type = NL_A_NESTED,
+.optional = false, },
 };
 
 static int
@@ -1094,6 +1117,9 @@ nl_parse_act_pedit(struct nlattr *options, struct 
tc_flower *flower)
 action = >actions[flower->action_count++];
 action->type = TC_ACT_PEDIT;
 
+nl_parse_tcf(nl_attr_get_unspec(pe_attrs[TCA_PEDIT_TM],
+sizeof(struct tcf_t)),
+ flower);
 return 0;
 }
 
@@ -1101,6 +1127,9 @@ static const struct nl_policy tunnel_key_policy[] = {
 [TCA_TUNNEL_KEY_PARMS] = { .type = NL_A_UNSPEC,
.min_len = sizeof(struct tc_tunnel_key),
.optional = false, },
+[TCA_TUNNEL_KEY_TM] = { .type = NL_A_UNSPEC,
+.min_len = sizeof(struct tcf_t),
+.optional = false, },
 [TCA_TUNNEL_KEY_ENC_IPV4_SRC] = { .type = NL_A_U32, .optional = true, },
 [TCA_TUNNEL_KEY_ENC_IPV4_DST] = { .type = NL_A_U32, .optional = true, },
 [TCA_TUNNEL_KEY_ENC_IPV6_SRC] = { .type = NL_A_UNSPEC,
@@ -1275,6 +1304,10 @@ nl_parse_act_tunnel_key(struct nlattr *options, struct 
tc_flower *flower)
 tun->action, tun->t_action);
 return EINVAL;
 }
+
+nl_parse_tcf(nl_attr_get_unspec(tun_attrs[TCA_TUNNEL_KEY_TM],
+sizeof(struct tcf_t)),
+ flower);
 return 0;
 }
 
@@ -1287,26 +1320,6 @@ static const struct nl_policy gact_policy[] = {
   .optional = false, },
 };
 
-static int
-get_user_hz(void)
-{
-static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
-static int user_hz = 100;
-
-if (ovsthread_once_start()) {
-user_hz = sysconf(_SC_CLK_TCK);
-ovsthread_once_done();
-}
-
-return user_hz;
-}
-
-static void
-nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower)
-{
-flower->lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
-}
-
 static int
 nl_parse_act_gact(struct nlattr *options, struct tc_flower *flower)
 {
@@ -1394,18 +1407,21 @@ nl_parse_act_mirred(struct nlattr *options, struct 
tc_flower *flower)
 
 static const struct nl_policy ct_policy[] = {
 [TCA_CT_PARMS] = { .type = NL_A_UNSPEC,
-  .min_len = sizeof(struct tc_ct),
-  .optional = false, },
+   .min_len = sizeof(struct tc_ct),
+   .optional = false, },
+[TCA_CT_TM] = { .type = NL_A_UNSPEC,
+.min_len = sizeof(struct tcf_t),
+.optional = false, },
 [TCA_CT_ACTION] = { .type = NL_A_U16,
- .optional = true, },
+.optional = true, },
 [TCA_CT_ZONE] = { .type = NL_A_U16,
   .optional = true, },
 [TCA_CT_MARK] = { .type = NL_A_U32,
.optional = true, },
 [TCA_CT_MARK_MASK] = { .type = NL_A_U32,
-.optional = true, },
+   .optional = true, },
 [TCA_CT_LABELS] = { .type = NL_A_UNSPEC,
- .optional = true, },
+.optional = true, },
 [TCA_CT_LABELS_MASK] = { .type = 

[ovs-dev] [PATCH 08/10] netdev-offload-tc: Check for none offloadable ct_state flag combination

[Eelco Chaudron]

This patch checks for none offloadable ct_state match flag combinations.
If they exist tell the user about it once, and ignore the offload.

Signed-off-by: Eelco Chaudron 
---
 lib/netdev-offload-tc.c |   18 +++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 8135441c6..f3d1d3e61 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1478,14 +1478,14 @@ flower_match_to_tun_opt(struct tc_flower *flower, const 
struct flow_tnl *tnl,
 flower->mask.tunnel.metadata.present.len = tnl->metadata.present.len;
 }
 
-static void
+static int
 parse_match_ct_state_to_flower(struct tc_flower *flower, struct match *match)
 {
 const struct flow *key = >flow;
 struct flow *mask = >wc.masks;
 
 if (!ct_state_support) {
-return;
+return 0;
 }
 
 if ((ct_state_support & mask->ct_state) == mask->ct_state) {
@@ -1541,6 +1541,13 @@ parse_match_ct_state_to_flower(struct tc_flower *flower, 
struct match *match)
 flower->key.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
 flower->mask.ct_state &= ~(TCA_FLOWER_KEY_CT_FLAGS_NEW);
 }
+
+if (flower->key.ct_state &&
+!(flower->key.ct_state & TCA_FLOWER_KEY_CT_FLAGS_TRACKED)) {
+VLOG_INFO_ONCE("For ct_state match offload to work, you must "
+   "include +trk with any other flags set!");
+return EOPNOTSUPP;
+}
 }
 
 if (mask->ct_zone) {
@@ -1560,6 +1567,8 @@ parse_match_ct_state_to_flower(struct tc_flower *flower, 
struct match *match)
 flower->mask.ct_label = mask->ct_label;
 mask->ct_label = OVS_U128_ZERO;
 }
+
+return 0;
 }
 
 static int
@@ -1814,7 +1823,10 @@ netdev_tc_flow_put(struct netdev *netdev, struct match 
*match,
 }
 }
 
-parse_match_ct_state_to_flower(, match);
+err = parse_match_ct_state_to_flower(, match);
+if (err) {
+return err;
+}
 
 /* ignore exact match on skb_mark of 0. */
 if (mask->pkt_mark == UINT32_MAX && !key->pkt_mark) {

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

[ovs-dev] [PATCH 07/10] netdev-offload-tc: Fix IP and port ranges in flower returns.

[Eelco Chaudron]

When programming NAT rules OVS only sets the minimum value for a
single IP/port value. However, responses from flower will always
return min == max for single IP/port values. This is causing the
verification to fail as the request is different than the response.
To avoid this, we will update the response to match the request.

Signed-off-by: Eelco Chaudron 
---
 lib/tc.c |   12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/tc.c b/lib/tc.c
index b977811c1..b37dce22f 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -1486,7 +1486,9 @@ nl_parse_act_ct(struct nlattr *options, struct tc_flower 
*flower)
 if (ipv4_max) {
 ovs_be32 addr = nl_attr_get_be32(ipv4_max);
 
-action->ct.range.ipv4.max = addr;
+if (action->ct.range.ipv4.min != addr) {
+action->ct.range.ipv4.max = addr;
+}
 }
 } else if (ipv6_min) {
 action->ct.range.ip_family = AF_INET6;
@@ -1495,7 +1497,9 @@ nl_parse_act_ct(struct nlattr *options, struct tc_flower 
*flower)
 if (ipv6_max) {
 struct in6_addr addr = nl_attr_get_in6_addr(ipv6_max);
 
-action->ct.range.ipv6.max = addr;
+if (!ipv6_addr_equals(>ct.range.ipv6.min, )) {
+action->ct.range.ipv6.max = addr;
+}
 }
 }
 
@@ -1503,6 +1507,10 @@ nl_parse_act_ct(struct nlattr *options, struct tc_flower 
*flower)
 action->ct.range.port.min = nl_attr_get_be16(port_min);
 if (port_max) {
 action->ct.range.port.max = nl_attr_get_be16(port_max);
+if (action->ct.range.port.min ==
+action->ct.range.port.max) {
+action->ct.range.port.max = 0;
+}
 }
 }
 }

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

[ovs-dev] [PATCH 05/10] netdev-offload-tc: Always include conntrack information to tc

[Eelco Chaudron]

Regardless of the traffic type, if requested, the conntrack information
should be included to keep the datapath and tc rules in sync.

Signed-off-by: Eelco Chaudron 
---
 lib/tc.c |   10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/tc.c b/lib/tc.c
index 5e6aa638e..556596926 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -2903,13 +2903,13 @@ nl_msg_put_flower_options(struct ofpbuf *request, 
struct tc_flower *flower)
 FLOWER_PUT_MASKED_VALUE(icmp_code, TCA_FLOWER_KEY_ICMPV6_CODE);
 FLOWER_PUT_MASKED_VALUE(icmp_type, TCA_FLOWER_KEY_ICMPV6_TYPE);
 }
-
-FLOWER_PUT_MASKED_VALUE(ct_state, TCA_FLOWER_KEY_CT_STATE);
-FLOWER_PUT_MASKED_VALUE(ct_zone, TCA_FLOWER_KEY_CT_ZONE);
-FLOWER_PUT_MASKED_VALUE(ct_mark, TCA_FLOWER_KEY_CT_MARK);
-FLOWER_PUT_MASKED_VALUE(ct_label, TCA_FLOWER_KEY_CT_LABELS);
 }
 
+FLOWER_PUT_MASKED_VALUE(ct_state, TCA_FLOWER_KEY_CT_STATE);
+FLOWER_PUT_MASKED_VALUE(ct_zone, TCA_FLOWER_KEY_CT_ZONE);
+FLOWER_PUT_MASKED_VALUE(ct_mark, TCA_FLOWER_KEY_CT_MARK);
+FLOWER_PUT_MASKED_VALUE(ct_label, TCA_FLOWER_KEY_CT_LABELS);
+
 if (host_eth_type == ETH_P_IP) {
 FLOWER_PUT_MASKED_VALUE(ipv4.ipv4_src, TCA_FLOWER_KEY_IPV4_SRC);
 FLOWER_PUT_MASKED_VALUE(ipv4.ipv4_dst, TCA_FLOWER_KEY_IPV4_DST);

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

[ovs-dev] [PATCH 06/10] netdev-offload-tc: Fix use of ICMP values instead of masks defines.

[Eelco Chaudron]

Signed-off-by: Eelco Chaudron 
---
 lib/tc.c |   13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/lib/tc.c b/lib/tc.c
index 556596926..b977811c1 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -939,24 +939,21 @@ nl_parse_flower_ip(struct nlattr **attrs, struct 
tc_flower *flower) {
 key->icmp_code =
nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ICMPV4_CODE]);
 mask->icmp_code =
-nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ICMPV4_CODE]);
+nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ICMPV4_CODE_MASK]);
 }
 if (attrs[TCA_FLOWER_KEY_ICMPV4_TYPE_MASK]) {
-key->icmp_type =
-   nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ICMPV4_TYPE_MASK]);
+key->icmp_type = nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ICMPV4_TYPE]);
 mask->icmp_type =
 nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ICMPV4_TYPE_MASK]);
 }
 } else if (ip_proto == IPPROTO_ICMPV6) {
 if (attrs[TCA_FLOWER_KEY_ICMPV6_CODE_MASK]) {
-key->icmp_code =
-   nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ICMPV6_CODE]);
+key->icmp_code = nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ICMPV6_CODE]);
 mask->icmp_code =
- nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ICMPV6_CODE]);
+ nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ICMPV6_CODE_MASK]);
 }
 if (attrs[TCA_FLOWER_KEY_ICMPV6_TYPE_MASK]) {
-key->icmp_type =
-   nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ICMPV6_TYPE_MASK]);
+key->icmp_type = nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ICMPV6_TYPE]);
 mask->icmp_type =
 nl_attr_get_u8(attrs[TCA_FLOWER_KEY_ICMPV6_TYPE_MASK]);
 }

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

[ovs-dev] [PATCH 03/10] odp_util: Fix parse_key_and_mask_to_match() vlan parsing

[Eelco Chaudron]

The parse_key_and_mask_to_match() is a function to translate
a netlink formatted key/mask to match structure. And should
not consider any configuration setting when translating.

Signed-off-by: Eelco Chaudron 
---
 lib/odp-util.c |   41 -
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 0ff302856..99dd55807 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -7188,7 +7188,8 @@ parse_8021q_onward(const struct nlattr 
*attrs[OVS_KEY_ATTR_MAX + 1],
uint64_t present_attrs, int out_of_range_attr,
uint64_t expected_attrs, struct flow *flow,
const struct nlattr *key, size_t key_len,
-   const struct flow *src_flow, char **errorp)
+   const struct flow *src_flow, char **errorp,
+   bool ignore_vlan_limit)
 {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 bool is_mask = src_flow != flow;
@@ -7196,9 +7197,11 @@ parse_8021q_onward(const struct nlattr 
*attrs[OVS_KEY_ATTR_MAX + 1],
 const struct nlattr *encap;
 enum odp_key_fitness encap_fitness;
 enum odp_key_fitness fitness = ODP_FIT_ERROR;
+int vlan_limit;
 int encaps = 0;
 
-while (encaps < flow_vlan_limit &&
+vlan_limit = ignore_vlan_limit ? FLOW_MAX_VLAN_HEADERS : flow_vlan_limit;
+while (encaps < vlan_limit &&
(is_mask
 ? (src_flow->vlans[encaps].tci & htons(VLAN_CFI)) != 0
 : eth_type_vlan(flow->dl_type))) {
@@ -7281,7 +7284,7 @@ parse_8021q_onward(const struct nlattr 
*attrs[OVS_KEY_ATTR_MAX + 1],
 static enum odp_key_fitness
 odp_flow_key_to_flow__(const struct nlattr *key, size_t key_len,
struct flow *flow, const struct flow *src_flow,
-   char **errorp)
+   char **errorp, bool ignore_vlan_limit)
 {
 /* New "struct flow" fields that are visible to the datapath (including all
  * data fields) should be translated from equivalent datapath flow fields
@@ -7431,7 +7434,7 @@ odp_flow_key_to_flow__(const struct nlattr *key, size_t 
key_len,
 : eth_type_vlan(src_flow->dl_type)) {
 fitness = parse_8021q_onward(attrs, present_attrs, out_of_range_attr,
  expected_attrs, flow, key, key_len,
- src_flow, errorp);
+ src_flow, errorp, ignore_vlan_limit);
 } else {
 if (is_mask) {
 /* A missing VLAN mask means exact match on vlan_tci 0 (== no
@@ -7497,7 +7500,7 @@ enum odp_key_fitness
 odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
  struct flow *flow, char **errorp)
 {
-return odp_flow_key_to_flow__(key, key_len, flow, flow, errorp);
+return odp_flow_key_to_flow__(key, key_len, flow, flow, errorp, false);
 }
 
 /* Converts the 'mask_key_len' bytes of OVS_KEY_ATTR_* attributes in 'mask_key'
@@ -7509,14 +7512,16 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t 
key_len,
  * If 'errorp' is nonnull, this function uses it for detailed error reports: if
  * the return value is ODP_FIT_ERROR, it stores a malloc()'d error string in
  * '*errorp', otherwise NULL. */
-enum odp_key_fitness
-odp_flow_key_to_mask(const struct nlattr *mask_key, size_t mask_key_len,
- struct flow_wildcards *mask, const struct flow *src_flow,
- char **errorp)
+static enum odp_key_fitness
+odp_flow_key_to_mask__(const struct nlattr *mask_key, size_t mask_key_len,
+   struct flow_wildcards *mask,
+   const struct flow *src_flow,
+   char **errorp, bool ignore_vlan_limit)
 {
 if (mask_key_len) {
 return odp_flow_key_to_flow__(mask_key, mask_key_len,
-  >masks, src_flow, errorp);
+  >masks, src_flow, errorp,
+  ignore_vlan_limit);
 } else {
 if (errorp) {
 *errorp = NULL;
@@ -7530,6 +7535,15 @@ odp_flow_key_to_mask(const struct nlattr *mask_key, 
size_t mask_key_len,
 }
 }
 
+enum odp_key_fitness
+odp_flow_key_to_mask(const struct nlattr *mask_key, size_t mask_key_len,
+ struct flow_wildcards *mask,
+ const struct flow *src_flow, char **errorp)
+{
+return odp_flow_key_to_mask__(mask_key, mask_key_len, mask, src_flow,
+  errorp, false);
+}
+
 /* Converts the netlink formated key/mask to match.
  * Fails if odp_flow_key_from_key/mask and odp_flow_key_key/mask
  * disagree on the acceptable form of flow */
@@ -7540,7 +7554,8 @@ parse_key_and_mask_to_match(const struct nlattr *key, 
size_t key_len,
 {
 enum odp_key_fitness fitness;
 
-fitness = odp_flow_key_to_flow(key, key_len, >flow, NULL);
+fitness = 

[ovs-dev] [PATCH 04/10] netdev-offload-tc: Check for valid netdev ifindex in flow_put

[Eelco Chaudron]

Verify that the returned ifindex by netdev_get_ifindex() is valid.
This might not be the case in the ERSPAN port scenario, which can
not be offloaded.

Signed-off-by: Eelco Chaudron 
---
 lib/netdev-offload-tc.c |9 +
 1 file changed, 9 insertions(+)

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 9845e8d3f..8135441c6 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1842,6 +1842,15 @@ netdev_tc_flow_put(struct netdev *netdev, struct match 
*match,
 return ENODEV;
 }
 action->out.ifindex_out = netdev_get_ifindex(outdev);
+
+if (action->out.ifindex_out < 0) {
+VLOG_DBG_RL(,
+"Can't find ifindex for output port %s, error %d",
+netdev_get_name(outdev), action->out.ifindex_out);
+
+return -action->out.ifindex_out;
+}
+
 action->out.ingress = is_internal_port(netdev_get_type(outdev));
 action->type = TC_ACT_OUTPUT;
 flower.action_count++;

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

[ovs-dev] [PATCH 02/10] netdev-offload-tc: Set the correct VLAN_VID and VLAN_PCP masks

[Eelco Chaudron]

This change will set the correct VID and PCP masks, as well as the
ethernet type mask.

Signed-off-by: Eelco Chaudron 
---
 lib/tc.c |   10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/lib/tc.c b/lib/tc.c
index 77305a105..5e6aa638e 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -568,16 +568,17 @@ nl_parse_flower_vlan(struct nlattr **attrs, struct 
tc_flower *flower)
 
 flower->key.encap_eth_type[0] =
 nl_attr_get_be16(attrs[TCA_FLOWER_KEY_ETH_TYPE]);
+flower->mask.encap_eth_type[0] = 0x;
 
 if (attrs[TCA_FLOWER_KEY_VLAN_ID]) {
 flower->key.vlan_id[0] =
 nl_attr_get_u16(attrs[TCA_FLOWER_KEY_VLAN_ID]);
-flower->mask.vlan_id[0] = 0x;
+flower->mask.vlan_id[0] = VLAN_VID_MASK >> VLAN_VID_SHIFT;
 }
 if (attrs[TCA_FLOWER_KEY_VLAN_PRIO]) {
 flower->key.vlan_prio[0] =
 nl_attr_get_u8(attrs[TCA_FLOWER_KEY_VLAN_PRIO]);
-flower->mask.vlan_prio[0] = 0xff;
+flower->mask.vlan_prio[0] = VLAN_PCP_MASK >> VLAN_PCP_SHIFT;
 }
 
 if (!attrs[TCA_FLOWER_KEY_VLAN_ETH_TYPE]) {
@@ -590,17 +591,18 @@ nl_parse_flower_vlan(struct nlattr **attrs, struct 
tc_flower *flower)
 }
 
 flower->key.encap_eth_type[1] = flower->key.encap_eth_type[0];
+flower->mask.encap_eth_type[1] = 0x;
 flower->key.encap_eth_type[0] = encap_ethtype;
 
 if (attrs[TCA_FLOWER_KEY_CVLAN_ID]) {
 flower->key.vlan_id[1] =
 nl_attr_get_u16(attrs[TCA_FLOWER_KEY_CVLAN_ID]);
-flower->mask.vlan_id[1] = 0x;
+flower->mask.vlan_id[1] = VLAN_VID_MASK >> VLAN_VID_SHIFT;
 }
 if (attrs[TCA_FLOWER_KEY_CVLAN_PRIO]) {
 flower->key.vlan_prio[1] =
 nl_attr_get_u8(attrs[TCA_FLOWER_KEY_CVLAN_PRIO]);
-flower->mask.vlan_prio[1] = 0xff;
+flower->mask.vlan_prio[1] = VLAN_PCP_MASK >> VLAN_PCP_SHIFT;
 }
 }
 

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

[ovs-dev] [PATCH 01/10] netdev-offload-tc: Add debug logs on tc rule verify failures

[Eelco Chaudron]

This patch adds more detailed debug logs on tc verify failures to
ease debugging the actual cause after the fact.

Signed-off-by: Eelco Chaudron 
---
 lib/tc.c |   79 +-
 1 file changed, 73 insertions(+), 6 deletions(-)

diff --git a/lib/tc.c b/lib/tc.c
index 38a1dfc0e..77305a105 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -2980,12 +2980,78 @@ nl_msg_put_flower_options(struct ofpbuf *request, 
struct tc_flower *flower)
 return 0;
 }
 
+static void log_tc_flower_match(const char *msg,
+const struct tc_flower *a,
+const struct tc_flower *b)
+{
+uint8_t key_a[sizeof(struct tc_flower_key)];
+uint8_t key_b[sizeof(struct tc_flower_key)];
+struct ds s = DS_EMPTY_INITIALIZER;
+
+for (int i = 0; i < sizeof a->key; i++) {
+uint8_t mask_a = ((uint8_t *)>mask)[i];
+uint8_t mask_b = ((uint8_t *)>mask)[i];
+
+key_a[i] = ((uint8_t *)>key)[i] & mask_a;
+key_b[i] = ((uint8_t *)>key)[i] & mask_b;
+}
+ds_put_cstr(, "\nExpected Mask:\n");
+ds_put_hex(, >mask, sizeof a->mask);
+ds_put_cstr(, "\nReceived Mask:\n");
+ds_put_hex(, >mask, sizeof b->mask);
+ds_put_cstr(, "\nExpected Key:\n");
+ds_put_hex(, >key, sizeof a->key);
+ds_put_cstr(, "\nReceived Key:\n");
+ds_put_hex(, >key, sizeof b->key);
+ds_put_cstr(, "\nExpected Masked Key:\n");
+ds_put_hex(, key_a, sizeof key_a);
+ds_put_cstr(, "\nReceived Masked Key:\n");
+ds_put_hex(, key_b, sizeof key_b);
+
+if (a->action_count != b->action_count) {
+/* If action count is not equal, we print all actions to see which
+ * ones are missing. */
+const struct tc_action *action;
+int i;
+
+ds_put_cstr(, "\nExpected Actions:\n");
+for (i = 0, action = a->actions; i < a->action_count; i++, action++) {
+ds_put_cstr(, " - ");
+ds_put_hex(, action, sizeof *action);
+ds_put_cstr(, "\n");
+}
+ds_put_cstr(, "Received Actions:\n");
+for (i = 0, action = b->actions; i < b->action_count; i++, action++) {
+ds_put_cstr(, " - ");
+ds_put_hex(, action, sizeof *action);
+ds_put_cstr(, "\n");
+}
+} else {
+/* Only dump the delta in actions. */
+const struct tc_action *action_a = a->actions;
+const struct tc_action *action_b = b->actions;
+
+for (int i = 0; i < a->action_count; i++, action_a++, action_b++) {
+if (memcmp(action_a, action_b, sizeof *action_a)) {
+ds_put_format(,
+  "\nAction %d mismatch:\n - Expected Action: ",
+  i);
+ds_put_hex(, action_a, sizeof *action_a);
+ds_put_cstr(, "\n - Received Action: ");
+ds_put_hex(, action_b, sizeof *action_b);
+}
+}
+}
+VLOG_DBG_RL(_rl, "%s%s", msg, ds_cstr());
+ds_destroy();
+}
+
 static bool
 cmp_tc_flower_match_action(const struct tc_flower *a,
const struct tc_flower *b)
 {
 if (memcmp(>mask, >mask, sizeof a->mask)) {
-VLOG_DBG_RL(_rl, "tc flower compare failed mask compare");
+log_tc_flower_match("tc flower compare failed mask compare:", a, b);
 return false;
 }
 
@@ -2998,8 +3064,8 @@ cmp_tc_flower_match_action(const struct tc_flower *a,
 uint8_t key_b = ((uint8_t *)>key)[i] & mask;
 
 if (key_a != key_b) {
-VLOG_DBG_RL(_rl, "tc flower compare failed key compare at "
-"%d", i);
+log_tc_flower_match("tc flower compare failed masked key compare:",
+a, b);
 return false;
 }
 }
@@ -3009,14 +3075,15 @@ cmp_tc_flower_match_action(const struct tc_flower *a,
 const struct tc_action *action_b = b->actions;
 
 if (a->action_count != b->action_count) {
-VLOG_DBG_RL(_rl, "tc flower compare failed action length check");
+log_tc_flower_match("tc flower compare failed action length check",
+a, b);
 return false;
 }
 
 for (int i = 0; i < a->action_count; i++, action_a++, action_b++) {
 if (memcmp(action_a, action_b, sizeof *action_a)) {
-VLOG_DBG_RL(_rl, "tc flower compare failed action compare "
-"for %d", i);
+log_tc_flower_match("tc flower compare failed action compare",
+a, b);
 return false;
 }
 }

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

[ovs-dev] [PATCH 00/10] netdev-offload-tc: Fix various tc-offload related problems

[Eelco Chaudron]

This series fixes a bunch of TC offload-related issues found when
running the kernel data path self-test (make check-kernel) forcing
TC to be enabled. To do this manually I applied the following diff:

  
https://github.com/chaudron/ovs/commit/d7ff1060f371e2cbd4b2e8dee222a9eed55073c8

These changes are OVS-related fixes, however, I have still two
problems that look kernel to investigate.

I'm also planning to find a nice way to include the system-traffic.at
into "make check-offloads". However, this might take some time, and I
do not want to delay getting the actual fixes in.

The following branch has some raw changes to system-traffic.at to
make most of the tests work with TC enabled hardcoded:

  https://github.com/chaudron/ovs/tree/dev/tc_verify


Eelco Chaudron (10):
  netdev-offload-tc: Add debug logs on tc rule verify failures
  netdev-offload-tc: Set the correct VLAN_VID and VLAN_PCP masks
  odp_util: Fix parse_key_and_mask_to_match() vlan parsing
  netdev-offload-tc: Check for valid netdev ifindex in flow_put
  netdev-offload-tc: Always include conntrack information to tc
  netdev-offload-tc: Fix use of ICMP values instead of masks defines.
  netdev-offload-tc: Fix IP and port ranges in flower returns.
  netdev-offload-tc: Check for none offloadable ct_state flag combination
  netdev-offload-tc: stats should be captured on first action in the list
  revalidator: Fix datapath statistics update.


 lib/netdev-offload-tc.c   |  27 +++-
 lib/odp-util.c|  41 --
 lib/tc.c  | 243 ++
 ofproto/ofproto-dpif-upcall.c |   8 +-
 4 files changed, 243 insertions(+), 76 deletions(-)

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

[ovs-dev] [PATCH ovn v2] Copy external_ids from Logical_Router_Port to SB database This patch makes ovn-northd copy all string-string pairs in external_ids column of the Logical_Router_Port table in N

[Selvaraj Palaniyappan]

Signed-off-by: Selvaraj Palaniyappan 
---
 northd/northd.c |  1 +
 ovn-nb.xml  |  6 ++
 ovn-sb.xml  |  3 ++-
 tests/ovn-northd.at | 14 ++
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/northd/northd.c b/northd/northd.c
index fc7a64f99..090922ae2 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3240,6 +3240,7 @@ ovn_port_update_sbrec(struct northd_input *input_data,
 ds_destroy();
 
 struct smap ids = SMAP_INITIALIZER();
+smap_clone(, >nbrp->external_ids);
 sbrec_port_binding_set_external_ids(op->sb, );
 
 sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0);
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 6a6972856..293d25b32 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -2895,6 +2895,12 @@
 
   
 See External IDs at the beginning of this document.
+
+  The ovn-northd program copies all these pairs into the
+   column of the
+   table in 
+  database.
+
   
 
   
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 9ddacdf09..f7c41ccdc 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -3354,7 +3354,8 @@ tcp.flags = RST;
 
   The ovn-northd program populates this column with
   all entries into the  column of the
-   table of the
+   and
+   tables of the
database.
 
   
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 84e52e701..f9c5259f1 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -144,6 +144,20 @@ AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
 AT_CLEANUP
 ])
 
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([check external id propagation to SBDB])
+ovn_start
+
+ovn-nbctl lr-add ro
+ovn-nbctl lrp-add ro lrp0 00:00:00:00:00:01 192.168.1.1/24
+ovn-nbctl --wait=sb set logical_router_port lrp0 external_ids=test=123
+AT_CHECK([ovn-sbctl --columns=external_ids --bare find Port_Binding 
logical_port=lrp0],
+[0], [test=123
+])
+
+AT_CLEANUP
+])
+
 OVN_FOR_EACH_NORTHD([
 AT_SETUP([check IPv6 RA config propagation to SBDB])
 ovn_start
-- 
2.22.3

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

Re: [ovs-dev] [PATCH] dpif-netdev: Fix a race condition in deletion of offloaded flows

[Sriharsha Basavapatna via dev]

On Thu, Jan 27, 2022 at 4:08 PM Ilya Maximets  wrote:
>
> On 1/27/22 07:42, Sriharsha Basavapatna via dev wrote:
> > In dp_netdev_pmd_remove_flow() we schedule the deletion of an
> > offloaded flow, if a mark has been assigned to the flow. But if
> > this occurs in the window in which the offload thread completes
> > offloading the flow and assigns a mark to the flow, then we miss
> > deleting the flow. This problem has been observed while adding
> > and deleting flows in a loop. To fix this, always enqueue flow
> > deletion regardless of the flow->mark being set.
> >
> > Fixes: 241bad15d99a("dpif-netdev: associate flow with a mark id")
> > Signed-off-by: Sriharsha Basavapatna 
> > ---
> >  lib/dpif-netdev.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index e28e0b554..22c5f19a8 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -3029,9 +3029,7 @@ dp_netdev_pmd_remove_flow(struct dp_netdev_pmd_thread 
> > *pmd,
> >  dp_netdev_simple_match_remove(pmd, flow);
> >  cmap_remove(>flow_table, node, dp_netdev_flow_hash(>ufid));
> >  ccmap_dec(>n_flows, odp_to_u32(in_port));
> > -if (flow->mark != INVALID_FLOW_MARK) {
> > -queue_netdev_flow_del(pmd, flow);
> > -}
> > +queue_netdev_flow_del(pmd, flow);
> >  flow->dead = true;
> >
> >  dp_netdev_flow_unref(flow);
> >
>
> Thanks for the patch, but it looks like that it's not that simple...
> A lot of tests are crashing in GHA and ASAN reports use-after-free
> on flow disassociation if the dpif already destroyed:
>   https://github.com/ovsrobot/ovs/actions/runs/1754866321
>
> I think that the problem is that offload thread holds the ref count
> for PMD thread, but PMD thread structure doesn't hold the ref count
> for the dp, because it doesn't expect that dp_netdev_pmd structure
> will be used while PMD thread is destroyed.  I guess, we should fix
> that.  OTOH, I'm not sure if offload thread actually needs a reference
> to dp_netdev_pmd structure.  If I didn't miss something, it only
> uses pmd pointer to access pmd->dp.  So, maybe, dp_offload_thread_item
> should hold and ref the dp pointer instead?
>
> What do you think?  Gaetan?
>
> Another point is that queue_netdev_flow_del() should check for
> netdev_is_flow_api_enabled() to avoid creation of offload threads
> if offloading is disabled.  But that's good that we didn't have it,
> as the refcount issue got exposed.  Otherwise it would be hard
> to reproduce.
>
> Best regards, Ilya Maximets.
>
> Asan report below, for convenience:
>
> =
> ==17076==ERROR: AddressSanitizer: heap-use-after-free on address 
> 0x61603080 at pc 0x005e0e38 bp 0x7fe0594309f0 sp 0x7fe0594309e8
> READ of size 8 at 0x61603080 thread T5 (hw_offload4)
> #0 0x5e0e37 in mark_to_flow_disassociate 
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2556:62
> #1 0x5dfaf1 in dp_offload_flow 
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2821:15
> #2 0x5df8bd in dp_netdev_flow_offload_main 
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2884:17
> #3 0x73a2fc in ovsthread_wrapper 
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/ovs-thread.c:422:12
> #4 0x7fe0619506da in start_thread 
> (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
> #5 0x7fe060ecf71e in clone (/lib/x86_64-linux-gnu/libc.so.6+0x12171e)
>
> 0x61603080 is located 0 bytes inside of 576-byte region 
> [0x61603080,0x616032c0)
> freed by thread T0 here:
> #0 0x49640d in free 
> (/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/vswitchd/ovs-vswitchd+0x49640d)
> #1 0x5d6652 in dp_netdev_free 
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:1980:5
> #2 0x5f0722 in dp_netdev_unref 
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:1991:13
> #3 0x5cf5e5 in dpif_netdev_close 
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2002:5
> #4 0x5fc393 in dpif_uninit 
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif.c:1725:9
> #5 0x5fc0c0 in dpif_close 
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif.c:457:9
> #6 0x52a972 in close_dpif_backer 
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto-dpif.c:715:5
> #7 0x517f08 in destruct 
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto-dpif.c:1860:5
> #8 0x4f09e0 in ofproto_destroy 
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto.c:1728:5
> #9 0x4c71e4 in bridge_destroy 
> /home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../vswitchd/bridge.c:3608:9
> #10 0x4c6f4a in bridge_exit 
> 

[ovs-dev] [PATCH] dpif-netdev: add an option to assign pmd rxq to all numas

[Wan Junjie]

When assign a rxq to a pmd, the rxq will only be assigned to
the numa it belongs. An exception is that the numa has no non-iso
pmd there.

For example, we have one vm port (vhu) with all its rxqs in numa1,
while one phy port (p0) in numa0 with all its rxqs in numa 0.
we have four pmds in two numas.
See tables below, paras are listed as (port, queue, numa, core)
 p0  0 0 20
 p0  1 0 21
 p0  2 0 20
 p0  3 0 21
 vhu 0 1 40
 vhu 1 1 41
 vhu 2 1 40
 vhu 2 1 41
In this scenario, ovs-dpdk underperforms another setting that
all p0's queues pinned to four pmds using pmd-rxq-affinity. With
pmd-rxq-isolate=false, we can make vhu get polled on numa 1.
 p0  0 0 20
 p0  1 1 40
 p0  2 0 21
 p0  3 1 41
 vhu 0 1 40
 vhu 1 1 41
 vhu 2 1 40
 vhu 2 1 41
Then we found that with really high throughput, say pmd 40, 41
will reach 100% cycles busy, and pmd 20, 21 reach 70% cycles
busy. The unbalanced rxqs on these pmds became the bottle neck.

pmd-rxq-numa=false would ignore the rxq's numa attribute when
assign rxq to pmds. This could make full use of the cores from
different numas while we have limited core resources for pmds.

Signed-off-by: Wan Junjie 
---
 lib/dpif-netdev.c|  30 +---
 tests/pmd.at | 106 +++
 vswitchd/vswitch.xml |  19 +++-
 3 files changed, 148 insertions(+), 7 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index e28e0b554..6b54a3a4c 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -297,6 +297,7 @@ struct dp_netdev {
 struct ovs_mutex tx_qid_pool_mutex;
 /* Rxq to pmd assignment type. */
 enum sched_assignment_type pmd_rxq_assign_type;
+bool pmd_rxq_assign_numa;
 bool pmd_iso;
 
 /* Protects the access of the 'struct dp_netdev_pmd_thread'
@@ -1844,6 +1845,7 @@ create_dp_netdev(const char *name, const struct 
dpif_class *class,
 
 cmap_init(>poll_threads);
 dp->pmd_rxq_assign_type = SCHED_CYCLES;
+dp->pmd_rxq_assign_numa = true;
 
 ovs_mutex_init(>tx_qid_pool_mutex);
 /* We need 1 Tx queue for each possible core + 1 for non-PMD threads. */
@@ -4867,6 +4869,14 @@ dpif_netdev_set_config(struct dpif *dpif, const struct 
smap *other_config)
 dp_netdev_request_reconfigure(dp);
 }
 
+bool rxq_assign_numa = smap_get_bool(other_config, "pmd-rxq-numa", true);
+if (dp->pmd_rxq_assign_numa != rxq_assign_numa) {
+dp->pmd_rxq_assign_numa = rxq_assign_numa;
+VLOG_INFO("Rxq to PMD numa assignment changed to: \'%s\'.",
+rxq_assign_numa ? "numa enabled": "numa disabled");
+dp_netdev_request_reconfigure(dp);
+}
+
 struct pmd_auto_lb *pmd_alb = >pmd_alb;
 
 rebalance_intvl = smap_get_int(other_config, "pmd-auto-lb-rebal-interval",
@@ -5869,6 +5879,7 @@ static void
 sched_numa_list_schedule(struct sched_numa_list *numa_list,
  struct dp_netdev *dp,
  enum sched_assignment_type algo,
+ bool assign_numa,
  enum vlog_level level)
 OVS_REQ_RDLOCK(dp->port_rwlock)
 {
@@ -5980,14 +5991,19 @@ sched_numa_list_schedule(struct sched_numa_list 
*numa_list,
 numa_id = netdev_get_numa_id(rxq->port->netdev);
 numa = sched_numa_list_lookup(numa_list, numa_id);
 
-/* Check if numa has no PMDs or no non-isolated PMDs. */
-if (!numa || !sched_numa_noniso_pmd_count(numa)) {
+/* Check if numa has no PMDs or no non-isolated PMDs.
+   Or if we should ignore the pmd numa attribute */
+if (!numa || !sched_numa_noniso_pmd_count(numa) || !assign_numa) {
 /* Unable to use this numa to find a PMD. */
 numa = NULL;
-/* Find any numa with available PMDs. */
+/* If we ignore rxq pmd numa attribute and it's roundrobin, we
+  should do roundrobin, else find any numa with available PMDs. */
 for (int j = 0; j < n_numa; j++) {
 numa = sched_numa_list_next(numa_list, last_cross_numa);
 if (sched_numa_noniso_pmd_count(numa)) {
+if (!assign_numa) {
+last_cross_numa = numa;
+}
 break;
 }
 last_cross_numa = numa;
@@ -5996,7 +6012,7 @@ sched_numa_list_schedule(struct sched_numa_list 
*numa_list,
 }
 
 if (numa) {
-if (numa->numa_id != numa_id) {
+if (numa->numa_id != numa_id && assign_numa) {
 VLOG(level, "There's no available (non-isolated) pmd thread "
 "on numa node %d. Port \'%s\' rx queue %d will "
 "be assigned to a pmd on numa node %d. "
@@ -6036,9 +6052,10 @@ rxq_scheduling(struct dp_netdev *dp)
 {
 struct sched_numa_list numa_list;
 enum sched_assignment_type algo = dp->pmd_rxq_assign_type;
+bool assign_numa = dp->pmd_rxq_assign_numa;
 
 

Re: [ovs-dev] [PATCH ovn] ovn-parallel-hmap: Fix NUMA and core detection.

[Xavier Simonart]

Hi Mark

Thanks for fixing this. Nice catch...
Should the --dummy-numa option not also be added to northd documentation?
Does the --dummy-numa option have any other effect than setting the number
of threads? I mean does it have for instance any "numa" or pinning effect?
e.g. is there any difference between --dummy-numa=(0,0,0,0) and
dummy-numa=(0,1,0,1,0,1,0,1) ?
I do not think so, but I think it should be documented as --dummy-numa
might seem a complex option for specifying the number of threads.
Thanks
Xavier

On Thu, Jan 20, 2022 at 10:32 PM Mark Michelson  wrote:

> ovn-parallel-hmap attempted to determine the number of threads to
> allocate for its pool based on the NUMA and core count reported by OVS.
> The problem is that since ovn-northd never called ovs_numa_init(), OVS
> would always return OVS_NUMA_UNSPEC for the number of NUMAs, and
> OVS_CORE_UNSPEC for the number of cores. Parallelization still was
> operational because we would fall back to a CPU core count. On a single
> NUMA machine, this is identical to what was expected, so everything
> seemed fine.
>
> In 37ad427cfb78a9e59d7e7ef249b2f2b3ac68c65a, the --dumy-numa option was
> added to ovn-northd to allow for parallel operation on machines with
> fewer NUMAs and cores than the parallelization code otherwise would
> require. However, because of the missing call to ovs_numa_init(), this
> option did not function as intended.
>
> This commit adds a call to ovs_numa_init() when setting up the parallel
> hmap thread pool so that accurate NUMA and core counts are reported, and
> dummy NUMA and core counts can be used.
>
> The referenced bugzilla is not actually reporting this as an issue, but
> this particular fix will enable the functionality requested.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1975345
> Signed-off-by: Mark Michelson 
> ---
>  lib/ovn-parallel-hmap.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/ovn-parallel-hmap.c b/lib/ovn-parallel-hmap.c
> index 56ceed8e8..7edc4c0b6 100644
> --- a/lib/ovn-parallel-hmap.c
> +++ b/lib/ovn-parallel-hmap.c
> @@ -404,6 +404,7 @@ static void
>  setup_worker_pools(bool force) {
>  int cores, nodes;
>
> +ovs_numa_init();
>  nodes = ovs_numa_get_n_numas();
>  if (nodes == OVS_NUMA_UNSPEC || nodes <= 0) {
>  nodes = 1;
> --
> 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] [PATCH 2/3] ovsdb-cs: Clear last_id on reconnect if condition changes in-flight.

[Dumitru Ceara]

On 1/26/22 01:22, Han Zhou wrote:
> 
> 
> On Tue, Jan 11, 2022 at 8:38 AM Dumitru Ceara  > wrote:
>>
>> When reconnecting, if there are condition changes already sent to the
>> server but not yet acked, reset the db's 'last-id', esentially clearing
>> the local cache after reconnect.
>>
>> This is needed because the client cannot easily differentiate between
>> the following cases:
>> a. either the server already processed the requested monitor
>>    condition change but the FSM was restarted before the
>>    client was notified.  In this case the client should
>>    clear its local cache because it's out of sync with the
>>    monitor view on the server side.
>> b. OR the server hasn't processed the requested monitor
>>    condition change yet.
>>
>> Conditions changing at the same time with a reconnection happening are
>> rare so the performance impact of this patch should be minimal.
>>
>> Also, the tests are updated to cover the fact that we cannot control
>> which of the two scenarios ("a" and "b" above) are hit during the test.
>>
>> Reported-by: Maxime Coquelin  >
>> Signed-off-by: Dumitru Ceara  >
> 
> Thanks Dumitru. Could you tell more details about how this problem is
> reproduced? Or are there test case failures because of this?

Hi Han,

Thanks for looking into this.

Yes, Maxime reported that the test "2241: simple idl, monitor_cond_since,
cluster disconnect - C - tcp" would occasionally fail without this patch.

> I still didn't understand how the problem would occur. When a client
> reconnects to the server, it should trigger the jsonrpc session
> recreation on the server side, so the monitor data for the session
> should be recreated, and so there shouldn't be "out of sync" view on the
> server side. Did I misunderstand?


Going through the test logs we see (simplified and edited, skipping the
less relevant parts):

0. initial contents of the "simple" table on the server side:
"row": {"i": 1, "r": 1.0, "b": true}
"row": {"i": 2, "r": 1.0, "b": true}

1. the client connects for the first time, gets schema, monitors _Server
   database.

2. the client requests a monitor_cond_since, id=3:
method="monitor_cond_since",
params=[
  "idltest",
  ["monid","idltest"],
  
{..."simple":[{"where":[false],"columns":["b","ba","i","ia","r","ra","s","sa","u","ua"]}],...},
  last_id=0 // last seen transaction id
], id=3

3. the server doesn't need to send any updates, nothing matches "false"
   and replies, id=3:
result=[
  false, // requested "last_id" not found in history, will cause the
 // client to flush in-memory contents
  "13220f46-f13f-4f30-ab19-0af58699e0aa", // last known transaction ID,
  // the client will store this
  // locally in last_id
  {}], id=3

4. the client changes its conditions and sends a monitor_cond_change,
   id=5:
params=[
  ["monid","idltest"],["monid","idltest"],
  {"simple":[{"where":[["i","==",2]]}]}], id=5

---
Client side view:
- simple table: empty
- last_id: "13220f46-f13f-4f30-ab19-0af58699e0aa"
- acked-cond: "simple": false
- req-cond:   "simple": {"where":[["i","==",2]]}
- new-cond:   
---

5. this new condition matches one row in table "simple" so the server
   sends an update which includes that row:
method="update3",
params=[
  ["monid","idltest"],
  "13220f46-f13f-4f30-ab19-0af58699e0aa",
  
{"simple":{"fd500d06-8d0c-4149-9c69-2914538e07e5":{"insert":{"b":true,"r":1,"i":2
]

6. the client inserts the row in its in-memory copy of the database,
   last_id becomes "13220f46-f13f-4f30-ab19-0af58699e0aa".  However, the
   condition change is not yet acked by the server!
---
Client side view:
- simple table:
  - row: {"b":true,"r":1,"i":2}
- last_id: "13220f46-f13f-4f30-ab19-0af58699e0aa"
- acked-cond: "simple": false
- req-cond:   "simple": {"where":[["i","==",2]]}
- new-cond:   
---

7. the client changes its conditions, doesn't send a monitor_cond_change
   because there's already one in-flight, not yet acked.
---
Client side view:
- simple table:
  - row: {"b":true,"r":1,"i":2}
- last_id: "13220f46-f13f-4f30-ab19-0af58699e0aa"
- acked-cond: "simple": false
- req-cond:   "simple": {"where":[["i","==",2]]}
- new-cond:   "simple": {"where":[["i","==",1]]}
---

8. the connection goes down, ovsdb_cs_db_sync_condition() is called from
   ovsdb_cs_restart_fsm():
---
Client side view:
- simple table:
  - row: {"b":true,"r":1,"i":2}
- last_id: "13220f46-f13f-4f30-ab19-0af58699e0aa"
- acked-cond: "simple": false
- req-cond:   
- new-cond:   "simple": {"where":[["i","==",1]]}
---

9. the client reconnects, gets schema, monitors _Server database, a new
   monitor is setup on the server side.

10. the client requests a monitor_cond_since, id=9:
method="monitor_cond_since",
params=[
  "idltest",
  ["monid","idltest"],
  

Re: [ovs-dev] [PATCH] dpif-netdev: Fix a race condition in deletion of offloaded flows

[Ilya Maximets]

On 1/27/22 07:42, Sriharsha Basavapatna via dev wrote:
> In dp_netdev_pmd_remove_flow() we schedule the deletion of an
> offloaded flow, if a mark has been assigned to the flow. But if
> this occurs in the window in which the offload thread completes
> offloading the flow and assigns a mark to the flow, then we miss
> deleting the flow. This problem has been observed while adding
> and deleting flows in a loop. To fix this, always enqueue flow
> deletion regardless of the flow->mark being set.
> 
> Fixes: 241bad15d99a("dpif-netdev: associate flow with a mark id")
> Signed-off-by: Sriharsha Basavapatna 
> ---
>  lib/dpif-netdev.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index e28e0b554..22c5f19a8 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3029,9 +3029,7 @@ dp_netdev_pmd_remove_flow(struct dp_netdev_pmd_thread 
> *pmd,
>  dp_netdev_simple_match_remove(pmd, flow);
>  cmap_remove(>flow_table, node, dp_netdev_flow_hash(>ufid));
>  ccmap_dec(>n_flows, odp_to_u32(in_port));
> -if (flow->mark != INVALID_FLOW_MARK) {
> -queue_netdev_flow_del(pmd, flow);
> -}
> +queue_netdev_flow_del(pmd, flow);
>  flow->dead = true;
>  
>  dp_netdev_flow_unref(flow);
> 

Thanks for the patch, but it looks like that it's not that simple...
A lot of tests are crashing in GHA and ASAN reports use-after-free
on flow disassociation if the dpif already destroyed:
  https://github.com/ovsrobot/ovs/actions/runs/1754866321

I think that the problem is that offload thread holds the ref count
for PMD thread, but PMD thread structure doesn't hold the ref count
for the dp, because it doesn't expect that dp_netdev_pmd structure
will be used while PMD thread is destroyed.  I guess, we should fix
that.  OTOH, I'm not sure if offload thread actually needs a reference
to dp_netdev_pmd structure.  If I didn't miss something, it only
uses pmd pointer to access pmd->dp.  So, maybe, dp_offload_thread_item
should hold and ref the dp pointer instead?

What do you think?  Gaetan?

Another point is that queue_netdev_flow_del() should check for
netdev_is_flow_api_enabled() to avoid creation of offload threads
if offloading is disabled.  But that's good that we didn't have it,
as the refcount issue got exposed.  Otherwise it would be hard
to reproduce.

Best regards, Ilya Maximets.

Asan report below, for convenience:

=
==17076==ERROR: AddressSanitizer: heap-use-after-free on address 0x61603080 
at pc 0x005e0e38 bp 0x7fe0594309f0 sp 0x7fe0594309e8
READ of size 8 at 0x61603080 thread T5 (hw_offload4)
#0 0x5e0e37 in mark_to_flow_disassociate 
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2556:62
#1 0x5dfaf1 in dp_offload_flow 
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2821:15
#2 0x5df8bd in dp_netdev_flow_offload_main 
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2884:17
#3 0x73a2fc in ovsthread_wrapper 
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/ovs-thread.c:422:12
#4 0x7fe0619506da in start_thread 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
#5 0x7fe060ecf71e in clone (/lib/x86_64-linux-gnu/libc.so.6+0x12171e)

0x61603080 is located 0 bytes inside of 576-byte region 
[0x61603080,0x616032c0)
freed by thread T0 here:
#0 0x49640d in free 
(/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/vswitchd/ovs-vswitchd+0x49640d)
#1 0x5d6652 in dp_netdev_free 
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:1980:5
#2 0x5f0722 in dp_netdev_unref 
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:1991:13
#3 0x5cf5e5 in dpif_netdev_close 
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif-netdev.c:2002:5
#4 0x5fc393 in dpif_uninit 
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif.c:1725:9
#5 0x5fc0c0 in dpif_close 
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../lib/dpif.c:457:9
#6 0x52a972 in close_dpif_backer 
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto-dpif.c:715:5
#7 0x517f08 in destruct 
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto-dpif.c:1860:5
#8 0x4f09e0 in ofproto_destroy 
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../ofproto/ofproto.c:1728:5
#9 0x4c71e4 in bridge_destroy 
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../vswitchd/bridge.c:3608:9
#10 0x4c6f4a in bridge_exit 
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../vswitchd/bridge.c:553:9
#11 0x4e109a in main 
/home/runner/work/ovs/ovs/openvswitch-2.17.90/_build/sub/../../vswitchd/ovs-vswitchd.c:146:5
#12 0x7fe060dcfbf6 in 

Re: [ovs-dev] [BUG] dpcls_subtable_lookup_reprobe is not thread-safe.

[Ferriter, Cian]

> -Original Message-
> From: dev  On Behalf Of Ilya Maximets
> Sent: Friday 14 January 2022 11:44
> To: ovs-dev 
> Cc: i.maxim...@ovn.org
> Subject: [ovs-dev] [BUG] dpcls_subtable_lookup_reprobe is not thread-safe.
> 
> The function currently looks like this:
> 
> lib/dpif-netdev.c:
> 
> static uint32_t
> dpcls_subtable_lookup_reprobe(struct dpcls *cls)
> {
> struct pvector *pvec = >subtables;
> uint32_t subtables_changed = 0;
> struct dpcls_subtable *subtable = NULL;
> 
> PVECTOR_FOR_EACH (subtable, pvec) {
> uint32_t u0_bits = subtable->mf_bits_set_unit0;
> uint32_t u1_bits = subtable->mf_bits_set_unit1;
> void *old_func = subtable->lookup_func;
> subtable->lookup_func = dpcls_subtable_get_best_impl(u0_bits, 
> u1_bits);
> subtables_changed += (old_func != subtable->lookup_func);
> }
> pvector_publish(pvec);
> 
> return subtables_changed;
> }
> 
> The problem is that it only pretends to be thread-safe:
> 
> 1. PVECTOR_FOR_EACH() iterates over the *current* implementation of a priority
>vector.  Hence, the function is changing the pointers that are currently in
>use by a PMD thread.  A few possible consequences:
> 
>a. Pointer store may be not atomic.  In that case PMD thread may read
>   garbage and use it as a function pointer leading to a crash on some
>   platforms.
> 
>b. If PMD thread is currently re-sorting subtables, it already cloned the
>   current pvector implementation and will replace it with a new version.
>   Therefore modifications done here will be lost.
> 
> 2. At best, pvector_publish(pvec) is useless here, because there were no 
> pvector
>modifications.  At worst, it will publish changes that the other thread is
>working on at the same time.  That will likely break the vector.
> 
> 3. Even if there were actual pvector modifications and correct use of
>pvector_publish(), these calls should be done under the pmd->flow_mutex
>in order to be safe.
> 
> Best regards, Ilya Maximets.

Hi Ilya,

Thanks for finding and reporting this. I'm looking into the details and your 
suggestions.

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

Re: [ovs-dev] [PATCH] dpif-netdev: Fix a race condition in deletion of offloaded flows

[Sriharsha Basavapatna via dev]

On Thu, Jan 27, 2022 at 2:45 PM Gaëtan Rivet  wrote:
>
> On Thu, Jan 27, 2022, at 07:42, Sriharsha Basavapatna via dev wrote:
> > In dp_netdev_pmd_remove_flow() we schedule the deletion of an
> > offloaded flow, if a mark has been assigned to the flow. But if
> > this occurs in the window in which the offload thread completes
> > offloading the flow and assigns a mark to the flow, then we miss
> > deleting the flow. This problem has been observed while adding
> > and deleting flows in a loop. To fix this, always enqueue flow
> > deletion regardless of the flow->mark being set.
> >
> > Fixes: 241bad15d99a("dpif-netdev: associate flow with a mark id")
> > Signed-off-by: Sriharsha Basavapatna 
> > ---
> >  lib/dpif-netdev.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index e28e0b554..22c5f19a8 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -3029,9 +3029,7 @@ dp_netdev_pmd_remove_flow(struct
> > dp_netdev_pmd_thread *pmd,
> >  dp_netdev_simple_match_remove(pmd, flow);
> >  cmap_remove(>flow_table, node,
> > dp_netdev_flow_hash(>ufid));
> >  ccmap_dec(>n_flows, odp_to_u32(in_port));
> > -if (flow->mark != INVALID_FLOW_MARK) {
> > -queue_netdev_flow_del(pmd, flow);
> > -}
> > +queue_netdev_flow_del(pmd, flow);
>
> Hi Sriharsha,
>
> It makes sense, thanks for the patch.
> Additionally, the mark is being written in a thread and read in another but 
> is not atomic. Without fence, coherence might take time, making the described 
> issue more likely on some archs.

Right, good point.
>
> Acked-by: Gaetan Rivet 

Thanks Gaetan.
-Harsha
>
> --
> Gaetan Rivet

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dpif-netdev: Fix a race condition in deletion of offloaded flows

[Gaëtan Rivet]

On Thu, Jan 27, 2022, at 07:42, Sriharsha Basavapatna via dev wrote:
> In dp_netdev_pmd_remove_flow() we schedule the deletion of an
> offloaded flow, if a mark has been assigned to the flow. But if
> this occurs in the window in which the offload thread completes
> offloading the flow and assigns a mark to the flow, then we miss
> deleting the flow. This problem has been observed while adding
> and deleting flows in a loop. To fix this, always enqueue flow
> deletion regardless of the flow->mark being set.
>
> Fixes: 241bad15d99a("dpif-netdev: associate flow with a mark id")
> Signed-off-by: Sriharsha Basavapatna 
> ---
>  lib/dpif-netdev.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index e28e0b554..22c5f19a8 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3029,9 +3029,7 @@ dp_netdev_pmd_remove_flow(struct 
> dp_netdev_pmd_thread *pmd,
>  dp_netdev_simple_match_remove(pmd, flow);
>  cmap_remove(>flow_table, node, 
> dp_netdev_flow_hash(>ufid));
>  ccmap_dec(>n_flows, odp_to_u32(in_port));
> -if (flow->mark != INVALID_FLOW_MARK) {
> -queue_netdev_flow_del(pmd, flow);
> -}
> +queue_netdev_flow_del(pmd, flow);

Hi Sriharsha,

It makes sense, thanks for the patch.
Additionally, the mark is being written in a thread and read in another but is 
not atomic. Without fence, coherence might take time, making the described 
issue more likely on some archs.

Acked-by: Gaetan Rivet 

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

Re: [ovs-dev] [PATCH ovn] Copy external_ids from Logical_Router_Port to SB database

[0-day Robot]

References:  <20220127080929.181056-1-selvara...@nutanix.com>
 

Bleep bloop.  Greetings Selvaraj Palaniyappan, 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: Author Selvaraj Palaniyappan  needs to sign off.
Lines checked: 88, Warnings: 0, Errors: 1


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

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

[ovs-dev] [PATCH ovn] Copy external_ids from Logical_Router_Port to SB database

[Selvaraj Palaniyappan]

This patch makes ovn-northd copy all string-string pairs in
external_ids column of the Logical_Router_Port table in Northbound
database to the equivalent column of the Port_Binding table in
Southbound database.
---
 northd/northd.c |  1 +
 ovn-nb.xml  |  6 ++
 ovn-sb.xml  |  3 ++-
 tests/ovn-northd.at | 14 ++
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/northd/northd.c b/northd/northd.c
index fc7a64f99..090922ae2 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3240,6 +3240,7 @@ ovn_port_update_sbrec(struct northd_input *input_data,
 ds_destroy();
 
 struct smap ids = SMAP_INITIALIZER();
+smap_clone(, >nbrp->external_ids);
 sbrec_port_binding_set_external_ids(op->sb, );
 
 sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0);
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 6a6972856..293d25b32 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -2895,6 +2895,12 @@
 
   
 See External IDs at the beginning of this document.
+
+  The ovn-northd program copies all these pairs into the
+   column of the
+   table in 
+  database.
+
   
 
   
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 9ddacdf09..f7c41ccdc 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -3354,7 +3354,8 @@ tcp.flags = RST;
 
   The ovn-northd program populates this column with
   all entries into the  column of the
-   table of the
+   and
+   tables of the
database.
 
   
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 84e52e701..f9c5259f1 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -144,6 +144,20 @@ AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
 AT_CLEANUP
 ])
 
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([check external id propagation to SBDB])
+ovn_start
+
+ovn-nbctl lr-add ro
+ovn-nbctl lrp-add ro lrp0 00:00:00:00:00:01 192.168.1.1/24
+ovn-nbctl --wait=sb set logical_router_port lrp0 external_ids=test=123
+AT_CHECK([ovn-sbctl --columns=external_ids --bare find Port_Binding 
logical_port=lrp0],
+[0], [test=123
+])
+
+AT_CLEANUP
+])
+
 OVN_FOR_EACH_NORTHD([
 AT_SETUP([check IPv6 RA config propagation to SBDB])
 ovn_start
-- 
2.22.3

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