Re: [ovs-dev] [PATCH] conntrack: remove the nat_action_info from the conn

[wenxu]

From: Michael Santana 
Date: 2021-08-09 23:17:06
To:  we...@ucloud.cn
Cc:  b...@ovn.org,Ilya Maximets 
,dlu...@gmail.com,d...@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] conntrack: remove the nat_action_info from the 
conn>nat_info->nat_action
>
>On Wed, Aug 4, 2021 at 11:20 PM  wrote:
>>
>> From: wenxu 
>>
>> Only the nat_action in the nat_action_info is used for conn
>> packet forward and other item such as min/max_ip/port only
>> used for the new conn operation. So the whole nat_ction_info
>> no need store in conn. This will also avoid unnecessary memory
>> allocation.
>This seems like a good change. There are like 20 or so instances of
>nat_info->nat_action compared to the 6 or so instances of
>min/max/ip/port that I could find.
>Seems you covered all the instances that needed to be changed which is
>good. The one thing that is not clear to me is why you changed certain
>variable declarations to const.


The nat_action_info pass to the nat_get_unique_tuple is const one, The functions
get the data from nat_action_info also should be const one. Or there are some 
compile
warnning.


>
>Otherwise LGTM
>>
>> Signed-off-by: wenxu 
>> ---
>>  lib/conntrack-private.h |   2 +-
>>  lib/conntrack.c | 101 
>> +---
>>  2 files changed, 53 insertions(+), 50 deletions(-)
>>
>> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
>> index 169ace5..dfdf4e6 100644
>> --- a/lib/conntrack-private.h
>> +++ b/lib/conntrack-private.h
>> @@ -98,7 +98,7 @@ struct conn {
>>  struct conn_key parent_key; /* Only used for orig_tuple support. */
>>  struct ovs_list exp_node;
>>  struct cmap_node cm_node;
>> -struct nat_action_info_t *nat_info;
>> +uint16_t nat_action;
>>  char *alg;
>>  struct conn *nat_conn; /* The NAT 'conn' context, if there is one. */
>>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index 551c206..33a1a92 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -111,7 +111,8 @@ static void *clean_thread_main(void *f_);
>>
>>  static bool
>>  nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
>> - struct conn *nat_conn);
>> + struct conn *nat_conn,
>> + const struct nat_action_info_t *nat_info);
>>
>>  static uint8_t
>>  reverse_icmp_type(uint8_t type);
>> @@ -724,7 +725,7 @@ handle_alg_ctl(struct conntrack *ct, const struct 
>> conn_lookup_ctx *ctx,
>>  static void
>>  pat_packet(struct dp_packet *pkt, const struct conn *conn)
>>  {
>> -if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
>> +if (conn->nat_action & NAT_ACTION_SRC) {
>>  if (conn->key.nw_proto == IPPROTO_TCP) {
>>  struct tcp_header *th = dp_packet_l4(pkt);
>>  packet_set_tcp_port(pkt, conn->rev_key.dst.port, th->tcp_dst);
>> @@ -732,7 +733,7 @@ pat_packet(struct dp_packet *pkt, const struct conn 
>> *conn)
>>  struct udp_header *uh = dp_packet_l4(pkt);
>>  packet_set_udp_port(pkt, conn->rev_key.dst.port, uh->udp_dst);
>>  }
>> -} else if (conn->nat_info->nat_action & NAT_ACTION_DST) {
>> +} else if (conn->nat_action & NAT_ACTION_DST) {
>>  if (conn->key.nw_proto == IPPROTO_TCP) {
>>  packet_set_tcp_port(pkt, conn->rev_key.dst.port,
>>  conn->rev_key.src.port);
>> @@ -746,7 +747,7 @@ pat_packet(struct dp_packet *pkt, const struct conn 
>> *conn)
>>  static void
>>  nat_packet(struct dp_packet *pkt, const struct conn *conn, bool related)
>>  {
>> -if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
>> +if (conn->nat_action & NAT_ACTION_SRC) {
>>  pkt->md.ct_state |= CS_SRC_NAT;
>>  if (conn->key.dl_type == htons(ETH_TYPE_IP)) {
>>  struct ip_header *nh = dp_packet_l3(pkt);
>> @@ -761,7 +762,7 @@ nat_packet(struct dp_packet *pkt, const struct conn 
>> *conn, bool related)
>>  if (!related) {
>>  pat_packet(pkt, conn);
>>  }
>> -} else if (conn->nat_info->nat_action & NAT_ACTION_DST) {
>> +} else if (conn->nat_action & NAT_ACTION_DST) {
>>  pkt->md.ct_state |= CS_DST_NAT;
>>  if (conn->key.dl_type == htons(ETH_TYPE_IP)) {
>>  struct ip_header *nh = dp_packet_l3(pkt);
>> @@ -782,7 +783,7 @@ nat_packet(struct dp_packet *pkt, const struct conn 
>> *conn, bool related)
>>  static void
>>  un_pat_packet(struct dp_packet *pkt, const struct conn *conn)
>>  {
>> -if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
>> +if (conn->nat_action & NAT_ACTION_SRC) {
>>  if (conn->key.nw_proto == IPPROTO_TCP) {
>>  struct tcp_header *th = dp_packet_l4(pkt);
>>  packet_set_tcp_port(pkt, th->tcp_src, conn->key.src.port);
>> @@ -790,7 +791,7 @@ un_pat_packet(struct dp_packet *pkt, const struct conn 
>> *conn)
>>  struct udp_header *uh = dp_packet_l4(pkt);
>>  packet_set_udp_port(pkt, 

Re: [ovs-dev] [PATCH RFC 0/1] use meson and ninja as a build system for ovs

[William Tu]

Hi Ilya,

Thanks for your feedback!

> Hi, Sergey and William.  Thanks for working on this.
>
> I think that it might be a good idea to move to a different build system
> that will not be that painful to run on Windows.  I'm not working on
> Windows parts, but it would be great to have a fast CI that can confirm
> that everything still working fine.

Yes, and avoiding the msys/mingw makes coding and debugging on windows
much easier.

>
> However, as I already said in the discussion on GitHub, it seems to be
> very hard to migrate our testsuite that heavily depends on autotest.
> And without migrating the testsuite we will, probably, have to maintain
> two different build systems to be able to run tests.  This, kind of,
> defeats the whole purpose of the change.

Why is it very hard?
I thought once we find a way to add tests to meson, it's just
"mechanically" copying all these scripts into a new test framework.
It's a lot of tedious work, but hopefully not a difficult or impossible task.

Or does current OVS autotests heavily depend on autotool/autoconf?
I see the test cases are pretty independent.
>
> Meson is a nice system in many aspects, but its support for tests is very
> limited.  IIUC, it can only run a single binary and check the error codes.
> Most of our tests starts several daemons and performs several fairly complex
> operations and checks.  I'm afraid that we will end up writing our own
> separate testing framework that will mimic autotest in order to be able to
> run our tests from meson.
>
> Did you think about this problem?  Do you have a solution in mind?

Thanks, we thought about it but I don't have a solution in mind at this moment.

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

Re: [ovs-dev] [PATCH] net: openvswitch: fix kernel-doc warnings in flow.c

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Sun,  8 Aug 2021 12:08:34 -0700 you wrote:
> Repair kernel-doc notation in a few places to make it conform to
> the expected format.
> 
> Fixes the following kernel-doc warnings:
> 
> flow.c:296: warning: This comment starts with '/**', but isn't a kernel-doc 
> comment. Refer Documentation/doc-guide/kernel-doc.rst
>  * Parse vlan tag from vlan header.
> flow.c:296: warning: missing initial short description on line:
>  * Parse vlan tag from vlan header.
> flow.c:537: warning: No description found for return value of 
> 'key_extract_l3l4'
> flow.c:769: warning: No description found for return value of 'key_extract'
> 
> [...]

Here is the summary with links:
  - net: openvswitch: fix kernel-doc warnings in flow.c
https://git.kernel.org/netdev/net/c/d6e712aa7e6a

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html


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

Re: [ovs-dev] [PATCH] netlink-socket: Log extack error messages in netlink transactions.

[Marcelo Ricardo Leitner]

Hi,

On Thu, Aug 05, 2021 at 11:55:51PM +0200, Paolo Valerio wrote:

Now that you put these two together:

> As an example, with this patch applied, the following generic message:
> 
> ovs|00239|netlink_socket|DBG|received NAK error=0 (Operation not supported)
 ^^^
> 
> becomes:
> 
> ovs|00239|netlink_socket|DBG|received NAK error=0 - Conntrack isn't enabled
 ^^^
(more on the error below)

Much better the msg!

...
> @@ -910,15 +916,17 @@ nl_sock_transact_multiple__(struct nl_sock *sock,
>  i = seq - base_seq;
>  txn = transactions[i];
>  
> +const char *err_msg = NULL;
>  /* Fill in the results for 'txn'. */
> -if (nl_msg_nlmsgerr(buf_txn->reply, >error)) {
> +if (nl_msg_nlmsgerr(buf_txn->reply, >error, _msg)) {
> +if (txn->error) {
> +VLOG_DBG_RL(, "received NAK error=%d - %s",
> +error,

Sounds like 'error' here should have been 'txn->error' ?

error here is the return of nl_sock_recv__(). It's something else.

> +err_msg ? err_msg : ovs_strerror(txn->error));
> +}
>  if (txn->reply) {
>  ofpbuf_clear(txn->reply);
>  }
> -if (txn->error) {
> -VLOG_DBG_RL(, "received NAK error=%d (%s)",
> -error, ovs_strerror(txn->error));
   ^

> -}
>  } else {
>  txn->error = 0;
>  if (txn->reply && txn != buf_txn) {

Thanks,
Marcelo

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

[ovs-dev] [PATCH ovn] ic: learn routes to LR only from corresponding transit switch

[Vladislav Odintsov]

This commit fixes an error where in case of LRs were connected
between different AZs with ovn-ic, their routes were leaking
from one LR attached to its transit switch to another LR
attached to another transit switch.

Testcase, which reproduces described problem is added as well.

Signed-off-by: Vladislav Odintsov 
---
This bugfix should have no conflicts to all branches,
so it's reasonable to apply it down to supported branches.
---
 ic/ovn-ic.c |  4 +++-
 tests/ovn-ic.at | 34 ++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index d69583956..8916ba542 100644
--- a/ic/ovn-ic.c
+++ b/ic/ovn-ic.c
@@ -1157,7 +1157,9 @@ sync_learned_route(struct ic_context *ctx,
 ovs_assert(ctx->ovnnb_txn);
 const struct icsbrec_route *isb_route;
 ICSBREC_ROUTE_FOR_EACH (isb_route, ctx->ovnisb_idl) {
-if (isb_route->availability_zone == az) {
+if (isb_route->availability_zone == az ||
+strcmp(isb_route->transit_switch,
+   ic_lr->isb_pb->transit_switch)) {
 continue;
 }
 struct in6_addr prefix, nexthop;
diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
index 2a4fba031..eb63d9450 100644
--- a/tests/ovn-ic.at
+++ b/tests/ovn-ic.at
@@ -315,6 +315,41 @@ OVS_WAIT_WHILE([ovn_as az2 ovn-nbctl lr-route-list lr2 | 
grep learned | grep 192
 # AZ1 shouldn't learn 10.11 any more.
 OVS_WAIT_WHILE([ovn_as az1 ovn-nbctl lr-route-list lr1 | grep learned | grep 
10.11])
 
+# Test routes don't leak from lr1 (ts1) to lr2 (ts2)
+ovn-ic-nbctl ts-add ts2
+for i in 1 2; do
+ovn_as az$i
+
+# Ensure route learning at AZ level
+ovn-nbctl set nb_global . options:ic-route-learn=true
+# Ensure route advertising at AZ level
+ovn-nbctl set nb_global . options:ic-route-adv=true
+# Drop blacklist
+ovn-nbctl remove nb_global . options ic-route-blacklist
+
+# Create LRP and connect to TS
+ovn-nbctl lr-add lr2$i
+ovn-nbctl lrp-add lr2$i lrp-lr2$i-ts2 aa:aa:aa:aa:aa:0$i 169.254.100.$i/24
+ovn-nbctl lsp-add ts2 lsp-ts2-lr2$i \
+-- lsp-set-addresses lsp-ts2-lr2$i router \
+-- lsp-set-type lsp-ts2-lr2$i router \
+-- lsp-set-options lsp-ts2-lr2$i router-port=lrp-lr2$i-ts2
+done
+
+# Create directly-connected route
+ovn_as az2 ovn-nbctl lrp-add lr2 lrp-lr2-ls2 aa:aa:aa:aa:bb:01 "192.168.0.1/24"
+
+# Test learned routes from lr2 didn't leak to lr22
+AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr1 |
+ grep learned | awk '{print $1, $2}' | sort], [0], [dnl
+10.11.2.0/24 169.254.100.2
+192.168.0.0/24 169.254.100.2
+])
+
+# Test there are no learned routes in lr21
+AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr21 | grep learned |
+ awk '{print $1, $2}'], [0], [])
+
 OVN_CLEANUP_IC([az1], [az2])
 
 AT_CLEANUP
-- 
2.30.0

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

Re: [ovs-dev] [PATCH RFC 0/1] use meson and ninja as a build system for ovs

[Ilya Maximets]

On 8/8/21 3:49 AM, Sergey Madaminov wrote:
> This RFC proposes to replace current, autotools-based, build system with 
> meson.
> It won't break the current build system process, which will allow for a 
> gradual transition.
> Furthermore, the new build system will natively work across multiple 
> operating systems (now it compiles on both Windows and Linux).
> As OVS build process relies on shell scripts to generate C source codes,
> ex: lib/dirs.c.in, we re-written them in Python to make them portable (as 
> awk/sed works differently on Windows platform).
> This does not introduce new dependencies (besides installing meson and ninja) 
> as Python is already being actively used to generate some of the files.
> 
> Multiple projects use meson as their build system where they replaced 
> autotools or make files.
> For example, QEMU, libvirt, and DPDK [1] already use it.
> It helps to speed up configuration and compilation process [2].
> Additionally, meson build files have a more centralized logic.
> Thus, it is not scattered across multiple files, e.g., configure.ac, 
> Makefile.am, and acinclude.m4.
> And meson.build files are easier to write, comprehend, and debug.
> So it will lower the entry barriers for the newcomers.
> 
> Currently, we are able to compile OvS on both Windows and Linux using Clang 
> and meson with ninja backend.
> While we have not yet implemented building several parts including AF_XPD, 
> AVX, and Documentation, all necessary binaries are generated.
> 
> Motivation
> **
> 
> Using autotools to build OvS on Linux works great.
> However, they are not natively available on Windows and require emulated 
> shell, ex: msys2 or mingw, to use them.
> This results in having additional dependency and significantly slows down 
> build process.
> We tested running "./boot.sh && ./configure && make" on Windows and it takes 
> >5min.
> Furthermore, it introduces an additional layer of indirection, which makes it 
> harder to debug build related issues.
> Having that additional layer not only slows down the build process but also 
> introduce additional dependencies, e.g., msys2 and mingw with potentially 
> their own limitations and issues.
> And this approach also requires a constant maintenance of build-aux/cccl 
> wrapper for cl.exe and link.exe.
> 
> Another downside of autotools is that it has a relatively high entry barrier.
> It is not straightforward how to read and write them.
> And debugging issues in a build process is quite painful.
> 
> Instead of trying to find a workaround, projects such as DPDK adopted a new 
> build system called meson written in Python.
> And it works natively on both Linux and Windows operating systems.
> As OvS and DPDK projects are closely related, we decided to explore that 
> option as well given the fact that OvS supports Clang on both platforms.
> Our experience showed that writing and reading meson.build files is quite 
> simple.
> And they also have a helpful document to ease transition from autotools [3].
> 
> Why Meson?
> **
> 
> As the main driver to transition to a new build system was to ease a build 
> process on Windows, there were several other candidates for a new build 
> system.
> 
> Bazel
> -
> 
> This is another emerging build system that gains traction.
> However, it seems that there could be potential issues of using Bazed on 
> Windows [4].
> And DPDK does not look forward to add Bazel support [5].
> 
> CMake
> -
> 
> It is well integrated with Visual Studio, which is a plus.
> Additionally, it is a quite mature build system with many features.
> However, CMake files are not particularly easy to read and write and it also 
> does things under the hood.
> It makes it potentially problematic to debug build process.
> 
> Visual Studio Solutions
> ---
> 
> While this sounds like a good approach at first, it would work only on 
> Windows.
> This means that it would require a separate effort to develop and maintain 
> such approach.
> 
> Given the above, I chose meson with ninja backend for the following reasons:
> 
> (1) Native runs on both Linux and Windows and does not introduce additional 
> dependency as it only requires Python [6], which is already required by OvS
> (2) Does not require a significant separate effort to maintain it for 
> different operating systems
> (3) Fast build process
> (4) DPDK uses meson and it is a closely replated project so it would make 
> sense to have the same build system
> 
> Usage Instruction
> *
> 
> On Linux:
> -
> 
> 1. Clone OvS with meson build system from this repository: 
> https://github.com/smadaminov/ovs-dpdk-meson
> 2. Enter the 'ovs' folder in the cloned repository
> 3. Run meson configuration step: `meson build`
>- To set Clang as a compiler: `CC=clang meson build`
> 4. Build OvS: `ninja -C build`
> 
> Additionally, you can clone current OvS master branch (as of this writing 
> commit hash: 

[ovs-dev] May we delay ovn-21.09?

[Mark Michelson]

Hi everyone,

If you've paid attention to ovn commits lately, you'll know that we've 
been putting in a lot of performance improvements. At Red Hat, this is 
because we've been engaging our scale team to try to help us to figure 
out bottlenecks in OVN and try to fix problem areas we find. As a happy 
coincidence, OVN developers from outside Red Hat have also been 
submitting fantastic optimizations lately.


Currently, ovn-21.09's soft freeze is scheduled for the end of this week 
(13 August). However, the Red Hat scale team has scheduled time through 
3 September for running large-scale tests. This effectively means that 
if we delayed until then, we could put even more optimizations into 
ovn-21.09 than we could if we were to soft-freeze now.


If we were to delay soft freeze until 3 September, then that would mean 
branching 2 weeks from then (17 September) and then releasing 2 weeks 
after that (1 October). This puts us releasing more-or-less one month 
later than the published schedule in the repository.


If this sort of delay would cause issues for developers, users, or 
organizations, please speak up. If anyone has an issue with delaying the 
release, we won't do it. After all, we published expected release dates 
and have already drifted on this one by a week. It's entirely possible 
people are relying on OVN 21.09 to be released then.


Thanks,
Mark Michelson

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

[ovs-dev] [PATCH 1/1] openflow: allow to configure ofp_desc fields on bridge other_config

[miguelborgesdefreitas]

From: Miguel Borges de Freitas 

Allow to configure all the missing ofp_desc fields (mfr_desc, hw_desc, sw_desc) 
on the bridge other_config.
Default values are still used (Nicira, Inc.; Open vSwitch; current ovs version) 
if no values are configured.

Rationale: SDN controllers such as ONOS make use of the information provided 
from the switch in the reply to OFPMP_DESC to load SDN controller drivers that 
define and control the pipeline (i.e. the sequence of tables and what match 
fields/actions are available in certain tables). Most of these drivers (if not 
all) are compatible with openvswitch. Some of them, for instance ofdpa-ovs are 
even designed to emulate physical pipelines (ofdpa) on top of the OVS dataplane.
It'd be nice that we could configure those fields on OVS so that the 
appropriate driver is loaded automatically when ovs attempts to connect to the 
controller, avoiding driver hot-reload which is prone to cause errors.

Signed-off-by: Miguel Borges de Freitas 
---
 ofproto/ofproto.c| 21 +
 ofproto/ofproto.h|  3 +++
 vswitchd/bridge.c| 30 +-
 vswitchd/vswitch.xml | 21 +
 4 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index bd6103b1c..d90342431 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -796,6 +796,27 @@ ofproto_set_threads(int n_handlers_, int n_revalidators_)
 n_handlers = MAX(n_handlers_, 0);
 }
 
+void
+ofproto_set_mfr_desc(struct ofproto *p, const char *mfr_desc)
+{
+free(p->mfr_desc);
+p->mfr_desc = nullable_xstrdup(mfr_desc);
+}
+
+void
+ofproto_set_hw_desc(struct ofproto *p, const char* hw_desc)
+{
+free(p->hw_desc);
+p->hw_desc = nullable_xstrdup(hw_desc);
+}
+
+void
+ofproto_set_sw_desc(struct ofproto *p, const char* sw_desc)
+{
+free(p->sw_desc);
+p->sw_desc = nullable_xstrdup(sw_desc);
+}
+
 void
 ofproto_set_dp_desc(struct ofproto *p, const char *dp_desc)
 {
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index b0262da2d..e8f12ce70 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -350,6 +350,9 @@ int ofproto_port_set_mcast_snooping(struct ofproto 
*ofproto, void *aux,
 void ofproto_set_threads(int n_handlers, int n_revalidators);
 void ofproto_type_set_config(const char *type,
  const struct smap *other_config);
+void ofproto_set_mfr_desc(struct ofproto*, const char *mfr_desc);
+void ofproto_set_hw_desc(struct ofproto*, const char *hw_desc);
+void ofproto_set_sw_desc(struct ofproto*, const char *sw_desc);
 void ofproto_set_dp_desc(struct ofproto *, const char *dp_desc);
 void ofproto_set_serial_desc(struct ofproto *p, const char *serial_desc);
 int ofproto_set_snoops(struct ofproto *, const struct sset *snoops);
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index cb7c5cb76..f05359687 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -287,8 +287,7 @@ static void bridge_configure_sflow(struct bridge *, int 
*sflow_bridge_number);
 static void bridge_configure_ipfix(struct bridge *);
 static void bridge_configure_spanning_tree(struct bridge *);
 static void bridge_configure_tables(struct bridge *);
-static void bridge_configure_dp_desc(struct bridge *);
-static void bridge_configure_serial_desc(struct bridge *);
+static void bridge_configure_ofp_desc(struct bridge *);
 static void bridge_configure_aa(struct bridge *);
 static void bridge_aa_refresh_queued(struct bridge *);
 static bool bridge_aa_need_refresh(struct bridge *);
@@ -942,8 +941,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch 
*ovs_cfg)
 bridge_configure_ipfix(br);
 bridge_configure_spanning_tree(br);
 bridge_configure_tables(br);
-bridge_configure_dp_desc(br);
-bridge_configure_serial_desc(br);
+bridge_configure_ofp_desc(br);
 bridge_configure_aa(br);
 }
 free(managers);
@@ -4123,17 +4121,23 @@ bridge_configure_tables(struct bridge *br)
 }
 
 static void
-bridge_configure_dp_desc(struct bridge *br)
-{
-ofproto_set_dp_desc(br->ofproto,
-smap_get(>cfg->other_config, "dp-desc"));
-}
-
-static void
-bridge_configure_serial_desc(struct bridge *br)
-{
+bridge_configure_ofp_desc(struct bridge *br)
+{
+// Manufacturer description
+ofproto_set_mfr_desc(br->ofproto,
+ smap_get(>cfg->other_config, "mfr-desc"));
+// Hardware description
+ofproto_set_hw_desc(br->ofproto,
+smap_get(>cfg->other_config, "hw-desc"));
+// Software description
+ofproto_set_sw_desc(br->ofproto,
+smap_get(>cfg->other_config, "sw-desc"));
+// Serial number
 ofproto_set_serial_desc(br->ofproto,
 smap_get(>cfg->other_config, "dp-sn"));
+// DP description
+ofproto_set_dp_desc(br->ofproto,
+smap_get(>cfg->other_config, "dp-desc"));
 }
 
 static struct aa_mapping *
diff --git 

Re: [ovs-dev] [PATCH] conntrack: remove the nat_action_info from the conn

[Michael Santana]

nat_info->nat_action

On Wed, Aug 4, 2021 at 11:20 PM  wrote:
>
> From: wenxu 
>
> Only the nat_action in the nat_action_info is used for conn
> packet forward and other item such as min/max_ip/port only
> used for the new conn operation. So the whole nat_ction_info
> no need store in conn. This will also avoid unnecessary memory
> allocation.
This seems like a good change. There are like 20 or so instances of
nat_info->nat_action compared to the 6 or so instances of
min/max/ip/port that I could find.
Seems you covered all the instances that needed to be changed which is
good. The one thing that is not clear to me is why you changed certain
variable declarations to const.

Otherwise LGTM
>
> Signed-off-by: wenxu 
> ---
>  lib/conntrack-private.h |   2 +-
>  lib/conntrack.c | 101 
> +---
>  2 files changed, 53 insertions(+), 50 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 169ace5..dfdf4e6 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -98,7 +98,7 @@ struct conn {
>  struct conn_key parent_key; /* Only used for orig_tuple support. */
>  struct ovs_list exp_node;
>  struct cmap_node cm_node;
> -struct nat_action_info_t *nat_info;
> +uint16_t nat_action;
>  char *alg;
>  struct conn *nat_conn; /* The NAT 'conn' context, if there is one. */
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 551c206..33a1a92 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -111,7 +111,8 @@ static void *clean_thread_main(void *f_);
>
>  static bool
>  nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
> - struct conn *nat_conn);
> + struct conn *nat_conn,
> + const struct nat_action_info_t *nat_info);
>
>  static uint8_t
>  reverse_icmp_type(uint8_t type);
> @@ -724,7 +725,7 @@ handle_alg_ctl(struct conntrack *ct, const struct 
> conn_lookup_ctx *ctx,
>  static void
>  pat_packet(struct dp_packet *pkt, const struct conn *conn)
>  {
> -if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
> +if (conn->nat_action & NAT_ACTION_SRC) {
>  if (conn->key.nw_proto == IPPROTO_TCP) {
>  struct tcp_header *th = dp_packet_l4(pkt);
>  packet_set_tcp_port(pkt, conn->rev_key.dst.port, th->tcp_dst);
> @@ -732,7 +733,7 @@ pat_packet(struct dp_packet *pkt, const struct conn *conn)
>  struct udp_header *uh = dp_packet_l4(pkt);
>  packet_set_udp_port(pkt, conn->rev_key.dst.port, uh->udp_dst);
>  }
> -} else if (conn->nat_info->nat_action & NAT_ACTION_DST) {
> +} else if (conn->nat_action & NAT_ACTION_DST) {
>  if (conn->key.nw_proto == IPPROTO_TCP) {
>  packet_set_tcp_port(pkt, conn->rev_key.dst.port,
>  conn->rev_key.src.port);
> @@ -746,7 +747,7 @@ pat_packet(struct dp_packet *pkt, const struct conn *conn)
>  static void
>  nat_packet(struct dp_packet *pkt, const struct conn *conn, bool related)
>  {
> -if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
> +if (conn->nat_action & NAT_ACTION_SRC) {
>  pkt->md.ct_state |= CS_SRC_NAT;
>  if (conn->key.dl_type == htons(ETH_TYPE_IP)) {
>  struct ip_header *nh = dp_packet_l3(pkt);
> @@ -761,7 +762,7 @@ nat_packet(struct dp_packet *pkt, const struct conn 
> *conn, bool related)
>  if (!related) {
>  pat_packet(pkt, conn);
>  }
> -} else if (conn->nat_info->nat_action & NAT_ACTION_DST) {
> +} else if (conn->nat_action & NAT_ACTION_DST) {
>  pkt->md.ct_state |= CS_DST_NAT;
>  if (conn->key.dl_type == htons(ETH_TYPE_IP)) {
>  struct ip_header *nh = dp_packet_l3(pkt);
> @@ -782,7 +783,7 @@ nat_packet(struct dp_packet *pkt, const struct conn 
> *conn, bool related)
>  static void
>  un_pat_packet(struct dp_packet *pkt, const struct conn *conn)
>  {
> -if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
> +if (conn->nat_action & NAT_ACTION_SRC) {
>  if (conn->key.nw_proto == IPPROTO_TCP) {
>  struct tcp_header *th = dp_packet_l4(pkt);
>  packet_set_tcp_port(pkt, th->tcp_src, conn->key.src.port);
> @@ -790,7 +791,7 @@ un_pat_packet(struct dp_packet *pkt, const struct conn 
> *conn)
>  struct udp_header *uh = dp_packet_l4(pkt);
>  packet_set_udp_port(pkt, uh->udp_src, conn->key.src.port);
>  }
> -} else if (conn->nat_info->nat_action & NAT_ACTION_DST) {
> +} else if (conn->nat_action & NAT_ACTION_DST) {
>  if (conn->key.nw_proto == IPPROTO_TCP) {
>  packet_set_tcp_port(pkt, conn->key.dst.port, conn->key.src.port);
>  } else if (conn->key.nw_proto == IPPROTO_UDP) {
> @@ -802,7 +803,7 @@ un_pat_packet(struct dp_packet *pkt, const struct conn 
> *conn)
>  static void
>  reverse_pat_packet(struct dp_packet *pkt, const struct conn *conn)
>  {
> 

Re: [ovs-dev] [PATCH] netdev-offload-dpdk: fix a crash in dump_flow_attr()

[Sriharsha Basavapatna via dev]

On Wed, Aug 4, 2021 at 6:07 PM Sriharsha Basavapatna
 wrote:
>
> In netdev_offload_dpdk_flow_create() when an offload request fails,
> dump_flow() is called to log a warning message. The 's_tnl' string
> in flow_patterns gets initialized in vport_to_rte_tunnel() conditionally
> via ds_put_format(). If it is not initialized, it crashes later in
> dump_flow_attr()->ds_clone()->memcpy() while dereferencing this string.
>
> Fix this by initializing s_tnl using ds_cstr(). Fix a similar
> issue with actions->s_tnl.
>
> Signed-off-by: Sriharsha Basavapatna 
> ---
>  lib/netdev-offload-dpdk.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index f6706ee0c..243e2c430 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -788,6 +788,7 @@ free_flow_patterns(struct flow_patterns *patterns)
>  free(CONST_CAST(void *, patterns->items[i].mask));
>  }
>  }
> +ds_destroy(>s_tnl);
>  free(patterns->items);
>  patterns->items = NULL;
>  patterns->cnt = 0;
> @@ -1334,6 +1335,7 @@ netdev_offload_dpdk_mark_rss(struct flow_patterns 
> *patterns,
>  struct rte_flow_error error;
>  struct rte_flow *flow;
>
> +ds_cstr(_tnl);
>  add_flow_mark_rss_actions(, flow_mark, netdev);
>
>  flow = netdev_offload_dpdk_flow_create(netdev, _attr, patterns,
> @@ -1814,6 +1816,7 @@ netdev_offload_dpdk_actions(struct netdev *netdev,
>  struct rte_flow_error error;
>  int ret;
>
> +ds_cstr(_tnl);
>  ret = parse_flow_actions(netdev, , nl_actions, actions_len);
>  if (ret) {
>  goto out;
> @@ -1838,6 +1841,7 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,
>  bool actions_offloaded = true;
>  struct rte_flow *flow;
>
> +ds_cstr(_tnl);
>  if (parse_flow_match(netdev, info->orig_in_port, , match)) {
>  VLOG_DBG_RL(, "%s: matches of ufid "UUID_FMT" are not supported",
>  netdev_get_name(netdev), UUID_ARGS((struct uuid *) 
> ufid));
> --
> 2.30.0.349.g30b29f044a
>

A gentle reminder on this fix. This crash was observed with the tunnel
offload feature.  The stack trace is below for reference.

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x7f61311f028c in __memmove_avx_unaligned_erms () from /lib64/libc.so.6
[Current thread is 1 (Thread 0x7f611700 (LWP 25514))]
(gdb) bt
#0  0x7f61311f028c in __memmove_avx_unaligned_erms () from /lib64/libc.so.6
#1  0x027c01cb in ds_clone (dst=0x7f611fffbee0, source=0x7f611fffc078)
at lib/dynamic-string.c:466
#2  0x02917189 in dump_flow_attr (s=0x7f611fffbec0,
s_extra=0x7f611fffbee0, attr=0x7f611fffbfe8, flow_patterns=0x7f611fffc050,
flow_actions=0x7f611fffbfa0) at lib/netdev-offload-dpdk.c:175
#3  0x02919884 in dump_flow (s=0x7f611fffbec0, s_extra=0x7f611fffbee0,
attr=0x7f611fffbfe8, flow_patterns=0x7f611fffc050,
flow_actions=0x7f611fffbfa0) at lib/netdev-offload-dpdk.c:627
#4  0x02919b74 in netdev_offload_dpdk_flow_create (netdev=0x23ffe8400,
attr=0x7f611fffbfe8, flow_patterns=0x7f611fffc050,
flow_actions=0x7f611fffbfa0, error=0x7f611fffbf80)
at lib/netdev-offload-dpdk.c:672
#5  0x0291cc60 in netdev_offload_dpdk_actions (netdev=0x23ffe8400,
patterns=0x7f611fffc050, nl_actions=0x7f60fc00b120, actions_len=8)
at lib/netdev-offload-dpdk.c:1821
#6  0x0291ce14 in netdev_offload_dpdk_add_flow (netdev=0x585d320,
match=0x7f60fc00b2a8, nl_actions=0x7f60fc00b120, actions_len=8,
ufid=0x7f60fc00a970, info=0x7f611fffc220) at lib/netdev-offload-dpdk.c:1847
#7  0x0291d304 in netdev_offload_dpdk_flow_put (netdev=0x585d320,
match=0x7f60fc00b2a8, actions=0x7f60fc00b120, actions_len=8,
ufid=0x7f60fc00a970, info=0x7f611fffc220, stats=0x0)
at lib/netdev-offload-dpdk.c:1963
#8  0x027f1d1e in netdev_flow_put (netdev=0x585d320,
match=0x7f60fc00b2a8, actions=0x7f60fc00b120, act_len=8,
ufid=0x7f60fc00a970, info=0x7f611fffc220, stats=0x0)
at lib/netdev-offload.c:251
#9  0x027a5a43 in dp_netdev_flow_offload_put (offload=0x7f60fc00b290)
at lib/dpif-netdev.c:2622
#10 0x027a5b70 in dp_netdev_flow_offload_main (data=0x0)
at lib/dpif-netdev.c:2671
#11 0x02877c0e in ovsthread_wrapper (aux_=0x57f90d0)
at lib/ovs-thread.c:383
#12 0x7f613299a2de in start_thread () from /lib64/libpthread.so.0
#13 0x7f613118e133 in clone () from /lib64/libc.so.6

Thanks,
-Harsha

-- 
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 

[ovs-dev] [PATCH v3] dpif-netdev: Forwarding optimization for flows with a simple match.

[Ilya Maximets]

There are cases where users might want simple forwarding or drop rules
for all packets received from a specific port, e.g ::

  "in_port=1,actions=2"
  "in_port=2,actions=IN_PORT"
  "in_port=3,vlan_tci=0x1234/0x1fff,actions=drop"
  "in_port=4,actions=push_vlan:0x8100,set_field:4196->vlan_vid,output:3"

There are also cases where complex OpenFlow rules can be simplified
down to datapath flows with very simple match criteria.

In theory, for very simple forwarding, OVS doesn't need to parse
packets at all in order to follow these rules.  "Simple match" lookup
optimization is intended to speed up packet forwarding in these cases.

Design:

Due to various implementation constraints userspace datapath has
following flow fields always in exact match (i.e. it's required to
match at least these fields of a packet even if the OF rule doesn't
need that):

  - recirc_id
  - in_port
  - packet_type
  - dl_type
  - vlan_tci (CFI + VID) - in most cases
  - nw_frag - for ip packets

Not all of these fields are related to packet itself.  We already
know the current 'recirc_id' and the 'in_port' before starting the
packet processing.  It also seems safe to assume that we're working
with Ethernet packets.  So, for the simple OF rule we need to match
only on 'dl_type', 'vlan_tci' and 'nw_frag'.

'in_port', 'dl_type', 'nw_frag' and 13 bits of 'vlan_tci' can be
combined in a single 64bit integer (mark) that can be used as a
hash in hash map.  We are using only VID and CFI form the 'vlan_tci',
flows that need to match on PCP will not qualify for the optimization.
Workaround for matching on non-existence of vlan updated to match on
CFI and VID only in order to qualify for the optimization.  CFI is
always set by OVS if vlan is present in a packet, so there is no need
to match on PCP in this case.  'nw_frag' takes 2 bits of PCP inside
the simple match mark.

New per-PMD flow table 'simple_match_table' introduced to store
simple match flows only.  'dp_netdev_flow_add' adds flow to the
usual 'flow_table' and to the 'simple_match_table' if the flow
meets following constraints:

  - 'recirc_id' in flow match is 0.
  - 'packet_type' in flow match is Ethernet.
  - Flow wildcards contains only minimal set of non-wildcarded fields
(listed above).

If the number of flows for current 'in_port' in a regular 'flow_table'
equals number of flows for current 'in_port' in a 'simple_match_table',
we may use simple match optimization, because all the flows we have
are simple match flows.  This means that we only need to parse
'dl_type', 'vlan_tci' and 'nw_frag' to perform packet matching.
Now we make the unique flow mark from the 'in_port', 'dl_type',
'nw_frag' and 'vlan_tci' and looking for it in the 'simple_match_table'.
On successful lookup we don't need to run full 'miniflow_extract()'.

Unsuccessful lookup technically means that we have no suitable flow
in the datapath and upcall will be required.  So, in this case EMC and
SMC lookups are disabled.  We may optimize this path in the future by
bypassing the dpcls lookup too.

Performance improvement of this solution on a 'simple match' flows
should be comparable with partial HW offloading, because it parses same
packet fields and uses similar flow lookup scheme.
However, unlike partial HW offloading, it works for all port types
including virtual ones.

Performance results when compared to EMC:

Test setup:

 virtio-user   OVSvirtio-user
  Testpmd1  >  pmd1  >  Testpmd2
  (txonly)   x<--  pmd2  < (mac swap)

Single stream of 64byte packets.  Actions:
  in_port=vhost0,actions=vhost1
  in_port=vhost1,actions=vhost0

Stats collected from pmd1 and pmd2, so there are 2 scenarios:
Virt-to-Virt   : Testpmd1 --> pmd1 --> Testpmd2.
Virt-to-NoCopy : Testpmd2 --> pmd2 --->x   Testpmd1.
Here the packet sent from pmd2 to Testpmd1 is always dropped, because
the virtqueue is full since Testpmd1 is in txonly mode and doesn't
receive any packets.  This should be closer to the performance of a
VM-to-Phy scenario.

Test performed on machine with Intel Xeon CPU E5-2690 v4 @ 2.60GHz.
Table below represents improvement in throughput when compared to EMC.

 ++++
 ||Default (-g -O2)| "-Ofast -march=native" |
 |   Scenario ++---++---+
 || GCC|   Clang   | GCC|   Clang   |
 +++---++---+
 | Virt-to-Virt   |+18.9%  |   +25.5%  |+10.8%  |   +16.7%  |
 | Virt-to-NoCopy |+24.3%  |   +33.7%  |+14.9%  |   +22.0%  |
 +++---++---+

For Phy-to-Phy case performance improvement should be even higher, but
it's not the main use-case for this functionality.  Performance
difference for the non-simple flows is within a margin of error.

Signed-off-by: Ilya Maximets 

[ovs-dev] [PATCH] tc: Set action flags for tunnel_key release

[Roi Dayan via dev]

From: Vlad Buslov 

The commit that enabled 'no_percpu' flag for compatible actions missed the
tunnel_key release action code. Add missing call to nl_msg_put_act_flags().

Fixes: 292d5bd9bb34 ("tc: Set 'no_percpu' flag for compatible actions")
Reported-by: Marcelo Ricardo Leitner 
Signed-off-by: Vlad Buslov 
Reviewed-by: Roi Dayan 
---
 lib/tc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/tc.c b/lib/tc.c
index 33287ea05e7a..38a1dfc0ebc8 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -2638,6 +2638,7 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct 
tc_flower *flower)
 if (!released && flower->tunnel) {
 act_offset = nl_msg_start_nested(request, act_index++);
 nl_msg_put_act_tunnel_key_release(request);
+nl_msg_put_act_flags(request);
 nl_msg_end_nested(request, act_offset);
 released = true;
 }
-- 
2.8.0

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