Re: [ovs-dev] [patch v1] conntrack: Fix ICMPV4 error data L4 length check.

[Vishal Deep Ajmera]

Thanks Darrell. Patch looks ok to me.

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

Re: [ovs-dev] [PATCH 2.9 2.8 2.7] tests: fix output indentation in tunnel.at

[Ben Pfaff]

On Thu, Aug 29, 2019 at 12:36:08AM -0300, Flavio Leitner via dev wrote:
> The commit b1356b50aa6a ("tnl-neigh: Use outgoing ofproto version.")
> uses the output indentation from newer OvS versions which include the
> commit 7be29a47576d ("ofproto-dpif: Remove tabs from output.").
> 
> Fixes: b1356b50aa6a ("tnl-neigh: Use outgoing ofproto version.")
> Signed-off-by: Flavio Leitner 

Applied to 2.9, 2.8, 2.7, thanks!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2.9 2.8 2.7] tests: fix output indentation in tunnel.at

[0-day Robot]

Bleep bloop.  Greetings Flavio Leitner via dev, 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.


build:
-e 's,[@]PYTHON[@],/bin/python2,g' \
-e 's,[@]RUNDIR[@],/usr/local/var/run/openvswitch,g' \
-e 's,[@]VERSION[@],2.12.90,g' \
-e 's,[@]localstatedir[@],/usr/local/var,g' \
-e 's,[@]pkgdatadir[@],/usr/local/share/openvswitch,g' \
-e 's,[@]sysconfdir[@],/usr/local/etc,g' \
-e 's,[@]bindir[@],/usr/local/bin,g' \
-e 's,[@]sbindir[@],/usr/local/sbin,g' \
-e 
's,[@]abs_builddir[@],/var/lib/jenkins/jobs/upstream_build_from_pw/workspace,g' 
\
-e 
's,[@]abs_top_srcdir[@],/var/lib/jenkins/jobs/upstream_build_from_pw/workspace,g'
 \
  > rhel/ovn-fedora.spec.tmp
mv rhel/ovn-fedora.spec.tmp rhel/ovn-fedora.spec
(printf '\043 Generated automatically -- do not modify!-*- 
buffer-read-only: t -*-\n' && sed -e 's,[@]VERSION[@],2.12.90,g') \
< ./xenserver/openvswitch-xen.spec.in > openvswitch-xen.spec.tmp || 
exit 1; \
if cmp -s openvswitch-xen.spec.tmp xenserver/openvswitch-xen.spec; then touch 
xenserver/openvswitch-xen.spec; rm openvswitch-xen.spec.tmp; else mv 
openvswitch-xen.spec.tmp xenserver/openvswitch-xen.spec; fi
make[3]: Entering directory 
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/datapath'
make[3]: Leaving directory 
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/datapath'
tests/tunnel.at
See above for files that use tabs for indentation.
Please use spaces instead.
make[2]: *** [check-tabs] Error 1
make[2]: Leaving directory 
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory 
`/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make: *** [all] Error 2


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

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

Re: [ovs-dev] [PATCH] tnl-neigh: Use outgoing ofproto version.

[Flavio Leitner via dev]

On Wed, Aug 28, 2019 at 11:29:54AM -0700, Ben Pfaff wrote:
> On Tue, Aug 13, 2019 at 01:34:04PM -0300, Flavio Leitner via dev wrote:
> > When a packet needs to be encapsulated in userspace, the endpoint
> > address needs to be resolved to fill in the headers. If it is not,
> > then currently OvS sends either a Neighbor Solicitation (IPv6)
> > or an ARP Query (IPv4) to resolve it.
> 
> Thanks, I applied this to master and backported it as far as it would go.

Hi Ben,

Sorry, but 2.9, 2.8 and 2.7 have a different output indentation which
breaks the testsuite.

I sent out a patch to fix those branches:
https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/362166.html

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

[ovs-dev] [PATCH 2.9 2.8 2.7] tests: fix output indentation in tunnel.at

[Flavio Leitner via dev]

The commit b1356b50aa6a ("tnl-neigh: Use outgoing ofproto version.")
uses the output indentation from newer OvS versions which include the
commit 7be29a47576d ("ofproto-dpif: Remove tabs from output.").

Fixes: b1356b50aa6a ("tnl-neigh: Use outgoing ofproto version.")
Signed-off-by: Flavio Leitner 
---
 tests/tunnel.at | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/tunnel.at b/tests/tunnel.at
index fd9fe04b6..38f7f48d8 100644
--- a/tests/tunnel.at
+++ b/tests/tunnel.at
@@ -408,13 +408,13 @@ AT_CHECK([ovs-vsctl add-port int-br v1 -- set Interface 
v1 type=vxlan \
 
 AT_CHECK([ovs-appctl dpif/show], [0], [dnl
 dummy@ovs-dummy: hit:0 missed:0
-  br0:
-br0 65534/100: (dummy-internal)
-p0 1/1: (dummy)
-  int-br:
-int-br 65534/2: (dummy-internal)
-v1 2/4789: (vxlan: key=123, remote_ip=172.31.1.2)
-v2 3/3: (dummy-internal)
+   br0:
+   br0 65534/100: (dummy-internal)
+   p0 1/1: (dummy)
+   int-br:
+   int-br 65534/2: (dummy-internal)
+   v1 2/4789: (vxlan: key=123, remote_ip=172.31.1.2)
+   v2 3/3: (dummy-internal)
 ])
 
 dnl First setup dummy interface IP address, then add the route
-- 
2.20.1

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

Re: [ovs-dev] [PATCH v4] ovsdb-tool: Convert clustered db to standalone db.

[Ginwala, Aliasgar via dev]

Thanks Ben:
Sent v5 https://patchwork.ozlabs.org/patch/1154959/ to address the same. PTAL.

On 8/28/19, 9:53 AM, "Ben Pfaff"  wrote:

On Mon, Aug 26, 2019 at 04:04:21PM -0700, amgin...@gmail.com wrote:
> From: Aliasgar Ginwala 
> 
> Add support in ovsdb-tool for migrating clustered dbs to standalone dbs.
> E.g. usage to migrate nb/sb db to standalone db from raft:
> ovsdb-tool cluster-to-standalone ovnnb_db.db ovnnb_db_cluster.db
> 
> Signed-off-by: Aliasgar Ginwala 

Please also update the ovsdb-tool manpage.

Thanks,

Ben.


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

[ovs-dev] [PATCH v5] ovsdb-tool: Convert clustered db to standalone db.

[amginwal]

From: Aliasgar Ginwala 

Add support in ovsdb-tool for migrating clustered dbs to standalone dbs.
E.g. usage to migrate nb/sb db to standalone db from raft:
ovsdb-tool cluster-to-standalone ovnnb_db.db ovnnb_db_cluster.db

Signed-off-by: Aliasgar Ginwala 
---
 Documentation/ref/ovsdb.7.rst |   3 +
 ovsdb/ovsdb-tool.1.in |   8 +++
 ovsdb/ovsdb-tool.c| 110 +-
 3 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst
index cd1c63d64..b12d8066c 100644
--- a/Documentation/ref/ovsdb.7.rst
+++ b/Documentation/ref/ovsdb.7.rst
@@ -514,6 +514,9 @@ standalone database from the contents of a running 
clustered database.
 When the cluster is down and cannot be revived, ``ovsdb-client backup`` will
 not work.
 
+Use ``ovsdb-tool cluster-to-standalone`` to convert clustered database to
+standalone database when the cluster is down and cannot be revived.
+
 Upgrading or Downgrading a Database
 ---
 
diff --git a/ovsdb/ovsdb-tool.1.in b/ovsdb/ovsdb-tool.1.in
index ec85e14c4..8c7962ab3 100644
--- a/ovsdb/ovsdb-tool.1.in
+++ b/ovsdb/ovsdb-tool.1.in
@@ -147,6 +147,14 @@ avoid this possibility, specify \fB\-\-cid=\fIuuid\fR, 
where
 \fIuuid\fR is the cluster ID of the cluster to join, as printed by
 \fBovsdb\-tool get\-cid\fR.
 .
+.SS "Database Migration Commands"
+This commands will convert cluster database to standalone database.
+.
+.IP "\fBcluster-to-standalone \fR[\fIdb\fR [\fIdb\fR]]"
+Use this command to convert to standalone database from clustered database
+when the cluster is down and cannot be revived. It creates new standalone
+\fIdb\fR file from the given cluster \fIdb\fR file.
+.
 .SS "Version Management Commands"
 .so ovsdb/ovsdb-schemas.man
 .PP
diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
index 438f97590..8039078a3 100644
--- a/ovsdb/ovsdb-tool.c
+++ b/ovsdb/ovsdb-tool.c
@@ -173,6 +173,9 @@ usage(void)
"  compare-versions A OP B  compare OVSDB schema version numbers\n"
"  query [DB] TRNS execute read-only transaction on DB\n"
"  transact [DB] TRNS  execute read/write transaction on DB\n"
+   "  cluster-to-standalone [DB [DB]]Convert clustered DB to\n"
+   "standalone DB when cluster is down and cannot be\n"
+"revived\n"
"  [-m]... show-log [DB]   print DB's log entries\n"
"The default DB is %s.\n"
"The default SCHEMA is %s.\n",
@@ -942,6 +945,64 @@ print_raft_record(const struct raft_record *r,
 }
 }
 
+static void
+write_raft_header_to_file(const struct json *data,
+  struct ovsdb_log *db_log_data)
+{
+if (!data || json_array(data)->n != 2) {
+   ovs_fatal(0, "***Invalid data***\n");
+}
+
+struct json *schema_json = json_array(data)->elems[0];
+if (schema_json->type != JSON_NULL) {
+struct ovsdb_schema *schema;
+check_ovsdb_error(ovsdb_schema_from_json(schema_json, &schema));
+ovsdb_schema_destroy(schema);
+check_ovsdb_error(ovsdb_log_write_and_free(db_log_data, schema_json));
+}
+
+struct json *data_json = json_array(data)->elems[1];
+if (!data_json || data_json->type != JSON_OBJECT) {
+ovs_fatal(0, "***invalid data***\n");
+}
+if (data_json->type != JSON_NULL) {
+check_ovsdb_error(ovsdb_log_write_and_free(db_log_data, data_json));
+}
+}
+
+static void
+write_raft_header(const struct raft_header *h, struct ovsdb_log *db_log_data)
+{
+if (h->snap_index) {
+write_raft_header_to_file(h->snap.data, db_log_data);
+}
+}
+
+static void
+write_raft_record_to_file(const struct json *data,
+  struct ovsdb_log *db_log_data)
+{
+if (json_array(data)->n != 2) {
+ovs_fatal(0, "***invalid data***\n");
+}
+
+struct json *data_json = json_array(data)->elems[1];
+if (data_json->type != JSON_NULL) {
+check_ovsdb_error(ovsdb_log_write_and_free(db_log_data, data_json));
+}
+}
+
+static void
+write_raft_record(const struct raft_record *r, struct ovsdb_log *db_log_data)
+{
+if (r->type == RAFT_REC_ENTRY) {
+if (!r->entry.data) {
+return;
+}
+write_raft_record_to_file(r->entry.data, db_log_data);
+}
+}
+
 static void
 do_show_log_cluster(struct ovsdb_log *log)
 {
@@ -1511,6 +1572,51 @@ do_compare_versions(struct ovs_cmdl_context *ctx)
 exit(result ? 0 : 2);
 }
 
+static void
+do_convert_to_standalone(struct ovsdb_log *log, struct ovsdb_log *db_log_data)
+{
+for (unsigned int i = 0; ; i++) {
+struct json *json;
+check_ovsdb_error(ovsdb_log_read(log, &json));
+if (!json) {
+break;
+}
+
+if (i == 0) {
+struct raft_header h;
+check_ovsdb_error(raft_header_from_json(&h, json));
+write_raft_header(&h, db_log_data);
+

[ovs-dev] [PATCHi v5] ovsdb-tool: Convert clustered db to standalone db.

[amginwal]

From: Aliasgar Ginwala 

Add support in ovsdb-tool for migrating clustered dbs to standalone dbs.
E.g. usage to migrate nb/sb db to standalone db from raft:
ovsdb-tool cluster-to-standalone ovnnb_db.db ovnnb_db_cluster.db

Signed-off-by: Aliasgar Ginwala 
---
 Documentation/ref/ovsdb.7.rst |   3 +
 ovsdb/ovsdb-tool.1.in |   8 +++
 ovsdb/ovsdb-tool.c| 110 +-
 3 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst
index cd1c63d64..b12d8066c 100644
--- a/Documentation/ref/ovsdb.7.rst
+++ b/Documentation/ref/ovsdb.7.rst
@@ -514,6 +514,9 @@ standalone database from the contents of a running 
clustered database.
 When the cluster is down and cannot be revived, ``ovsdb-client backup`` will
 not work.
 
+Use ``ovsdb-tool cluster-to-standalone`` to convert clustered database to
+standalone database when the cluster is down and cannot be revived.
+
 Upgrading or Downgrading a Database
 ---
 
diff --git a/ovsdb/ovsdb-tool.1.in b/ovsdb/ovsdb-tool.1.in
index ec85e14c4..8c7962ab3 100644
--- a/ovsdb/ovsdb-tool.1.in
+++ b/ovsdb/ovsdb-tool.1.in
@@ -147,6 +147,14 @@ avoid this possibility, specify \fB\-\-cid=\fIuuid\fR, 
where
 \fIuuid\fR is the cluster ID of the cluster to join, as printed by
 \fBovsdb\-tool get\-cid\fR.
 .
+.SS "Database Migration Commands"
+This commands will convert cluster database to standalone database.
+.
+.IP "\fBcluster-to-standalone \fR[\fIdb\fR [\fIdb\fR]]"
+Use this command to convert to standalone database from clustered database
+when the cluster is down and cannot be revived. It creates new standalone
+\fIdb\fR file from the given cluster \fIdb\fR file.
+.
 .SS "Version Management Commands"
 .so ovsdb/ovsdb-schemas.man
 .PP
diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
index 438f97590..8039078a3 100644
--- a/ovsdb/ovsdb-tool.c
+++ b/ovsdb/ovsdb-tool.c
@@ -173,6 +173,9 @@ usage(void)
"  compare-versions A OP B  compare OVSDB schema version numbers\n"
"  query [DB] TRNS execute read-only transaction on DB\n"
"  transact [DB] TRNS  execute read/write transaction on DB\n"
+   "  cluster-to-standalone [DB [DB]]Convert clustered DB to\n"
+   "standalone DB when cluster is down and cannot be\n"
+"revived\n"
"  [-m]... show-log [DB]   print DB's log entries\n"
"The default DB is %s.\n"
"The default SCHEMA is %s.\n",
@@ -942,6 +945,64 @@ print_raft_record(const struct raft_record *r,
 }
 }
 
+static void
+write_raft_header_to_file(const struct json *data,
+  struct ovsdb_log *db_log_data)
+{
+if (!data || json_array(data)->n != 2) {
+   ovs_fatal(0, "***Invalid data***\n");
+}
+
+struct json *schema_json = json_array(data)->elems[0];
+if (schema_json->type != JSON_NULL) {
+struct ovsdb_schema *schema;
+check_ovsdb_error(ovsdb_schema_from_json(schema_json, &schema));
+ovsdb_schema_destroy(schema);
+check_ovsdb_error(ovsdb_log_write_and_free(db_log_data, schema_json));
+}
+
+struct json *data_json = json_array(data)->elems[1];
+if (!data_json || data_json->type != JSON_OBJECT) {
+ovs_fatal(0, "***invalid data***\n");
+}
+if (data_json->type != JSON_NULL) {
+check_ovsdb_error(ovsdb_log_write_and_free(db_log_data, data_json));
+}
+}
+
+static void
+write_raft_header(const struct raft_header *h, struct ovsdb_log *db_log_data)
+{
+if (h->snap_index) {
+write_raft_header_to_file(h->snap.data, db_log_data);
+}
+}
+
+static void
+write_raft_record_to_file(const struct json *data,
+  struct ovsdb_log *db_log_data)
+{
+if (json_array(data)->n != 2) {
+ovs_fatal(0, "***invalid data***\n");
+}
+
+struct json *data_json = json_array(data)->elems[1];
+if (data_json->type != JSON_NULL) {
+check_ovsdb_error(ovsdb_log_write_and_free(db_log_data, data_json));
+}
+}
+
+static void
+write_raft_record(const struct raft_record *r, struct ovsdb_log *db_log_data)
+{
+if (r->type == RAFT_REC_ENTRY) {
+if (!r->entry.data) {
+return;
+}
+write_raft_record_to_file(r->entry.data, db_log_data);
+}
+}
+
 static void
 do_show_log_cluster(struct ovsdb_log *log)
 {
@@ -1511,6 +1572,51 @@ do_compare_versions(struct ovs_cmdl_context *ctx)
 exit(result ? 0 : 2);
 }
 
+static void
+do_convert_to_standalone(struct ovsdb_log *log, struct ovsdb_log *db_log_data)
+{
+for (unsigned int i = 0; ; i++) {
+struct json *json;
+check_ovsdb_error(ovsdb_log_read(log, &json));
+if (!json) {
+break;
+}
+
+if (i == 0) {
+struct raft_header h;
+check_ovsdb_error(raft_header_from_json(&h, json));
+write_raft_header(&h, db_log_data);
+

Re: [ovs-dev] [PATCH v3] datapath: compat: Backports bugfixes for nf_conncount

[Ben Pfaff]

On Wed, Aug 28, 2019 at 04:48:52PM -0700, Yi-Hung Wei wrote:
> On Wed, Aug 28, 2019 at 4:07 PM Ben Pfaff  wrote:
> >
> > On Wed, Aug 07, 2019 at 03:25:33PM -0700, Yifeng Sun wrote:
> > > This patch backports several critical bug fixes related to
> > > locking and data consistency in nf_conncount code.
> > >
> > > This backport is based on the following upstream net-next upstream 
> > > commits.
> > > a007232 ("netfilter: nf_conncount: fix argument order to find_next_bit")
> > > c80f10b ("netfilter: nf_conncount: speculative garbage collection on 
> > > empty lists")
> > > 2f971a8 ("netfilter: nf_conncount: move all list iterations under 
> > > spinlock")
> > > df4a902 ("netfilter: nf_conncount: merge lookup and add functions")
> > > e8cfb37 ("netfilter: nf_conncount: restart search when nodes have been 
> > > erased")
> > > f7fcc98 ("netfilter: nf_conncount: split gc in two phases")
> > > 4cd273b ("netfilter: nf_conncount: don't skip eviction when age is 
> > > negative")
> > > c78e781 ("netfilter: nf_conncount: replace CONNCOUNT_LOCK_SLOTS with 
> > > CONNCOUNT_SLOTS")
> > > d4e7df1 ("netfilter: nf_conncount: use rb_link_node_rcu() instead of 
> > > rb_link_node()")
> > > 53ca0f2 ("netfilter: nf_conncount: remove wrong condition check routine")
> > > 3c5cdb1 ("netfilter: nf_conncount: fix unexpected permanent node of 
> > > list.")
> > > 31568ec ("netfilter: nf_conncount: fix list_del corruption in conn_free")
> > > fd3e71a ("netfilter: nf_conncount: use spin_lock_bh instead of spin_lock")
> > >
> > > This patch adds additional compat code so that it can build on
> > > all supported kernel versions.
> >
> > I think that our most common approach is to use one OVS commit to
> > backport one Linux kernel commit.  This commit combines many Linux
> > kernel commits.  Is that an intentional change in this case?
> 
> Hi Ben,
> 
> Yes, we are intended to pull in all of the bug fixes in this case.
> The rationale is as following.
> 
> For the commits in ovs kernel module, we usually backport one upstream
> net-next commit to one OVS commit.  We need this fine granularity
> backports because a single OVS kernel module changes can affect OVS
> behavior.   For the other type of kernel backports (mainly in
> ./datapath/linux/compat/ ), we try to backport the required missing
> features for ovs kernel module in the older kernel.  The goal is to
> keep the older kernel in sync with the newer kernel on the required
> features, and we may not need much detailed information per upstream
> patch.  In this case, it would be easier to pull in multiple patches
> at once.
> 
> Some existing examples are,
> c387d8177f20 ("compat: Add ipv6 GRE and IPV6 Tunneling")
> 744964326f6c ("datapath: compat: Backports nf_conncount")

Thanks a lot.

I applied this to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] Open vSwitch 2.12 release ready?

[Ben Pfaff]

Hello, everyone.

I believe that we're up-to-date with patches posted against branch-2.12.
Does anyone know of anything that should hold up release?

(Travis is currently doing a build, but the previous one did pass.)

Thanks,

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

Re: [ovs-dev] [PATCH 1/2] datapath: Properly set L4 keys on "later" IP fragments

[Gregory Rose]

On 8/28/2019 5:17 PM, Justin Pettit wrote:

On Aug 28, 2019, at 4:50 PM, Greg Rose  wrote:

Upstream commit:
commit ad06a566e118e57b852cab5933dbbbaebb141de3
Author: Greg Rose 
Date:   Tue Aug 27 07:58:09 2019 -0700

openvswitch: Properly set L4 keys on "later" IP fragments

When IP fragments are reassembled before being sent to conntrack, the
key from the last fragment is used.  Unless there are reordering
issues, the last fragment received will not contain the L4 ports, so the
key for the reassembled datagram won't contain them.  This patch updates
the key once we have a reassembled datagram.

The handle_fragments() function works on L3 headers so we pull the L3/L4
flow key update code from key_extract into a new function
'key_extract_l3l4'.  Then we add a another new function
ovs_flow_key_update_l3l4() and export it so that it is accessible by
handle_fragments() for conntrack packet reassembly.

Co-authored-by: Justin Pettit 
Signed-off-by: Greg Rose 
Acked-by: Pravin B Shelar 
Signed-off-by: David S. Miller 

Cc: Justin Pettit 
Signed-off-by: Greg Rose 

Thanks, Greg.  I was able to cleanly apply this to master and branch-2.12.  
Would you be able to provide backports to older OVS versions?  I think fragment 
reassembly was introduce in OVS 2.5.

Thanks!


I can do that.

- Greg


--Justin




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

Re: [ovs-dev] [PATCH 1/2] datapath: Properly set L4 keys on "later" IP fragments

[Justin Pettit]

> On Aug 28, 2019, at 4:50 PM, Greg Rose  wrote:
> 
> Upstream commit:
>commit ad06a566e118e57b852cab5933dbbbaebb141de3
>Author: Greg Rose 
>Date:   Tue Aug 27 07:58:09 2019 -0700
> 
>openvswitch: Properly set L4 keys on "later" IP fragments
> 
>When IP fragments are reassembled before being sent to conntrack, the
>key from the last fragment is used.  Unless there are reordering
>issues, the last fragment received will not contain the L4 ports, so the
>key for the reassembled datagram won't contain them.  This patch updates
>the key once we have a reassembled datagram.
> 
>The handle_fragments() function works on L3 headers so we pull the L3/L4
>flow key update code from key_extract into a new function
>'key_extract_l3l4'.  Then we add a another new function
>ovs_flow_key_update_l3l4() and export it so that it is accessible by
>handle_fragments() for conntrack packet reassembly.
> 
>Co-authored-by: Justin Pettit 
>Signed-off-by: Greg Rose 
>Acked-by: Pravin B Shelar 
>Signed-off-by: David S. Miller 
> 
> Cc: Justin Pettit 
> Signed-off-by: Greg Rose 

Thanks, Greg.  I was able to cleanly apply this to master and branch-2.12.  
Would you be able to provide backports to older OVS versions?  I think fragment 
reassembly was introduce in OVS 2.5.

Thanks!

--Justin


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

Re: [ovs-dev] [PATCH 1/2] datapath: Properly set L4 keys on "later" IP fragments

[Justin Pettit]

> On Aug 28, 2019, at 5:05 PM, Yi-Hung Wei  wrote:
> 
> Thanks for the backport.
> 
> Acked-by: Yi-Hung Wei 

Thanks, Yi-Hung.  I already pushed the backports.

--Justin


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

Re: [ovs-dev] [PATCH 2/2] datapath: Clear the L4 portion of the key for "later" fragments

[Yi-Hung Wei]

On Wed, Aug 28, 2019 at 4:51 PM Greg Rose  wrote:
>
> From: Justin Pettit 
>
> Upstream commit:
> commit 0754b4e8cdf3eec6e4122e79af26ed9bab20f8f8
> Author: Justin Pettit 
> Date:   Tue Aug 27 07:58:10 2019 -0700
>
> openvswitch: Clear the L4 portion of the key for "later" fragments.
>
> Only the first fragment in a datagram contains the L4 headers.  When the
> Open vSwitch module parses a packet, it always sets the IP protocol
> field in the key, but can only set the L4 fields on the first fragment.
> The original behavior would not clear the L4 portion of the key, so
> garbage values would be sent in the key for "later" fragments.  This
> patch clears the L4 fields in that circumstance to prevent sending those
> garbage values as part of the upcall.
>
> Signed-off-by: Justin Pettit 
> Acked-by: Pravin B Shelar 
> Signed-off-by: David S. Miller 
>
> Cc: Justin Pettit 
> Signed-off-by: Greg Rose 
> ---

LGTM.

Acked-by: Yi-Hung Wei 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] datapath: Properly set L4 keys on "later" IP fragments

[Yi-Hung Wei]

On Wed, Aug 28, 2019 at 4:50 PM Greg Rose  wrote:
>
> Upstream commit:
> commit ad06a566e118e57b852cab5933dbbbaebb141de3
> Author: Greg Rose 
> Date:   Tue Aug 27 07:58:09 2019 -0700
>
> openvswitch: Properly set L4 keys on "later" IP fragments
>
> When IP fragments are reassembled before being sent to conntrack, the
> key from the last fragment is used.  Unless there are reordering
> issues, the last fragment received will not contain the L4 ports, so the
> key for the reassembled datagram won't contain them.  This patch updates
> the key once we have a reassembled datagram.
>
> The handle_fragments() function works on L3 headers so we pull the L3/L4
> flow key update code from key_extract into a new function
> 'key_extract_l3l4'.  Then we add a another new function
> ovs_flow_key_update_l3l4() and export it so that it is accessible by
> handle_fragments() for conntrack packet reassembly.
>
> Co-authored-by: Justin Pettit 
> Signed-off-by: Greg Rose 
> Acked-by: Pravin B Shelar 
> Signed-off-by: David S. Miller 
>
> Cc: Justin Pettit 
> Signed-off-by: Greg Rose 
> ---

Thanks for the backport.

Acked-by: Yi-Hung Wei 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] datapath: Clear the L4 portion of the key for "later" fragments

[0-day Robot]

Bleep bloop.  Greetings Greg Rose, 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 Justin Pettit  needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Greg Rose 
Lines checked: 57, Warnings: 1, 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 2/2] datapath: Clear the L4 portion of the key for "later" fragments

[Greg Rose]

From: Justin Pettit 

Upstream commit:
commit 0754b4e8cdf3eec6e4122e79af26ed9bab20f8f8
Author: Justin Pettit 
Date:   Tue Aug 27 07:58:10 2019 -0700

openvswitch: Clear the L4 portion of the key for "later" fragments.

Only the first fragment in a datagram contains the L4 headers.  When the
Open vSwitch module parses a packet, it always sets the IP protocol
field in the key, but can only set the L4 fields on the first fragment.
The original behavior would not clear the L4 portion of the key, so
garbage values would be sent in the key for "later" fragments.  This
patch clears the L4 fields in that circumstance to prevent sending those
garbage values as part of the upcall.

Signed-off-by: Justin Pettit 
Acked-by: Pravin B Shelar 
Signed-off-by: David S. Miller 

Cc: Justin Pettit 
Signed-off-by: Greg Rose 
---
 datapath/flow.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/datapath/flow.c b/datapath/flow.c
index dc93581..46e2bac 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -577,6 +577,7 @@ static int key_extract_l3l4(struct sk_buff *skb, struct 
sw_flow_key *key)
offset = nh->frag_off & htons(IP_OFFSET);
if (offset) {
key->ip.frag = OVS_FRAG_TYPE_LATER;
+   memset(&key->tp, 0, sizeof(key->tp));
return 0;
}
 #ifdef HAVE_SKB_GSO_UDP
@@ -699,8 +700,10 @@ static int key_extract_l3l4(struct sk_buff *skb, struct 
sw_flow_key *key)
return error;
}
 
-   if (key->ip.frag == OVS_FRAG_TYPE_LATER)
+   if (key->ip.frag == OVS_FRAG_TYPE_LATER) {
+   memset(&key->tp, 0, sizeof(key->tp));
return 0;
+   }
 #ifdef HAVE_SKB_GSO_UDP
if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
key->ip.frag = OVS_FRAG_TYPE_FIRST;
-- 
1.8.3.1

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

[ovs-dev] [PATCH 1/2] datapath: Properly set L4 keys on "later" IP fragments

[Greg Rose]

Upstream commit:
commit ad06a566e118e57b852cab5933dbbbaebb141de3
Author: Greg Rose 
Date:   Tue Aug 27 07:58:09 2019 -0700

openvswitch: Properly set L4 keys on "later" IP fragments

When IP fragments are reassembled before being sent to conntrack, the
key from the last fragment is used.  Unless there are reordering
issues, the last fragment received will not contain the L4 ports, so the
key for the reassembled datagram won't contain them.  This patch updates
the key once we have a reassembled datagram.

The handle_fragments() function works on L3 headers so we pull the L3/L4
flow key update code from key_extract into a new function
'key_extract_l3l4'.  Then we add a another new function
ovs_flow_key_update_l3l4() and export it so that it is accessible by
handle_fragments() for conntrack packet reassembly.

Co-authored-by: Justin Pettit 
Signed-off-by: Greg Rose 
Acked-by: Pravin B Shelar 
Signed-off-by: David S. Miller 

Cc: Justin Pettit 
Signed-off-by: Greg Rose 
---
 datapath/conntrack.c |   5 ++
 datapath/flow.c  | 157 +--
 datapath/flow.h  |   1 +
 3 files changed, 95 insertions(+), 68 deletions(-)

diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index 292febb..4c96d90 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -570,6 +570,11 @@ static int handle_fragments(struct net *net, struct 
sw_flow_key *key,
return -EPFNOSUPPORT;
}
 
+   /* The key extracted from the fragment that completed this datagram
+* likely didn't have an L4 header, so regenerate it.
+*/
+   ovs_flow_key_update_l3l4(skb, key);
+
key->ip.frag = OVS_FRAG_TYPE_NONE;
skb_clear_hash(skb);
skb->ignore_df = 1;
diff --git a/datapath/flow.c b/datapath/flow.c
index 618c25e..dc93581 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -541,79 +541,14 @@ static int parse_nsh(struct sk_buff *skb, struct 
sw_flow_key *key)
 }
 
 /**
- * key_extract - extracts a flow key from an Ethernet frame.
+ * key_extract_l3l4 - extracts L3/L4 header information.
  * @skb: sk_buff that contains the frame, with skb->data pointing to the
- * Ethernet header
+ *   L3 header
  * @key: output flow key
- *
- * The caller must ensure that skb->len >= ETH_HLEN.
- *
- * Returns 0 if successful, otherwise a negative errno value.
- *
- * Initializes @skb header fields as follows:
- *
- *- skb->mac_header: the L2 header.
- *
- *- skb->network_header: just past the L2 header, or just past the
- *  VLAN header, to the first byte of the L2 payload.
- *
- *- skb->transport_header: If key->eth.type is ETH_P_IP or ETH_P_IPV6
- *  on output, then just past the IP header, if one is present and
- *  of a correct length, otherwise the same as skb->network_header.
- *  For other key->eth.type values it is left untouched.
- *
- *- skb->protocol: the type of the data starting at skb->network_header.
- *  Equals to key->eth.type.
  */
-static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
+static int key_extract_l3l4(struct sk_buff *skb, struct sw_flow_key *key)
 {
int error;
-   struct ethhdr *eth;
-
-   /* Flags are always used as part of stats */
-   key->tp.flags = 0;
-
-   skb_reset_mac_header(skb);
-
-   /* Link layer. */
-   clear_vlan(key);
-   if (ovs_key_mac_proto(key) == MAC_PROTO_NONE) {
-   if (unlikely(eth_type_vlan(skb->protocol)))
-   return -EINVAL;
-
-   skb_reset_network_header(skb);
-   key->eth.type = skb->protocol;
-   } else {
-   eth = eth_hdr(skb);
-   ether_addr_copy(key->eth.src, eth->h_source);
-   ether_addr_copy(key->eth.dst, eth->h_dest);
-
-   __skb_pull(skb, 2 * ETH_ALEN);
-   /* We are going to push all headers that we pull, so no need to
-* update skb->csum here.
-*/
-
-   if (unlikely(parse_vlan(skb, key)))
-   return -ENOMEM;
-
-   key->eth.type = parse_ethertype(skb);
-   if (unlikely(key->eth.type == htons(0)))
-   return -ENOMEM;
-
-   /* Multiple tagged packets need to retain TPID to satisfy
-* skb_vlan_pop(), which will later shift the ethertype into
-* skb->protocol.
-*/
-   if (key->eth.cvlan.tci & htons(VLAN_CFI_MASK))
-   skb->protocol = key->eth.cvlan.tpid;
-   else
-   skb->protocol = key->eth.type;
-
-   skb_reset_network_header(skb);
-   __skb_push(skb, skb->data - skb_mac_header(skb));
-   }
-
-   skb_reset_mac_len(skb);
 
/* Network layer. */
if (key->eth.type == htons(ETH_P_IP)) {
@@ -814,6 +749,92 @@ static int key_extract(stru

Re: [ovs-dev] [PATCH v3] datapath: compat: Backports bugfixes for nf_conncount

[Yi-Hung Wei]

On Wed, Aug 28, 2019 at 4:07 PM Ben Pfaff  wrote:
>
> On Wed, Aug 07, 2019 at 03:25:33PM -0700, Yifeng Sun wrote:
> > This patch backports several critical bug fixes related to
> > locking and data consistency in nf_conncount code.
> >
> > This backport is based on the following upstream net-next upstream commits.
> > a007232 ("netfilter: nf_conncount: fix argument order to find_next_bit")
> > c80f10b ("netfilter: nf_conncount: speculative garbage collection on empty 
> > lists")
> > 2f971a8 ("netfilter: nf_conncount: move all list iterations under spinlock")
> > df4a902 ("netfilter: nf_conncount: merge lookup and add functions")
> > e8cfb37 ("netfilter: nf_conncount: restart search when nodes have been 
> > erased")
> > f7fcc98 ("netfilter: nf_conncount: split gc in two phases")
> > 4cd273b ("netfilter: nf_conncount: don't skip eviction when age is 
> > negative")
> > c78e781 ("netfilter: nf_conncount: replace CONNCOUNT_LOCK_SLOTS with 
> > CONNCOUNT_SLOTS")
> > d4e7df1 ("netfilter: nf_conncount: use rb_link_node_rcu() instead of 
> > rb_link_node()")
> > 53ca0f2 ("netfilter: nf_conncount: remove wrong condition check routine")
> > 3c5cdb1 ("netfilter: nf_conncount: fix unexpected permanent node of list.")
> > 31568ec ("netfilter: nf_conncount: fix list_del corruption in conn_free")
> > fd3e71a ("netfilter: nf_conncount: use spin_lock_bh instead of spin_lock")
> >
> > This patch adds additional compat code so that it can build on
> > all supported kernel versions.
>
> I think that our most common approach is to use one OVS commit to
> backport one Linux kernel commit.  This commit combines many Linux
> kernel commits.  Is that an intentional change in this case?

Hi Ben,

Yes, we are intended to pull in all of the bug fixes in this case.
The rationale is as following.

For the commits in ovs kernel module, we usually backport one upstream
net-next commit to one OVS commit.  We need this fine granularity
backports because a single OVS kernel module changes can affect OVS
behavior.   For the other type of kernel backports (mainly in
./datapath/linux/compat/ ), we try to backport the required missing
features for ovs kernel module in the older kernel.  The goal is to
keep the older kernel in sync with the newer kernel on the required
features, and we may not need much detailed information per upstream
patch.  In this case, it would be easier to pull in multiple patches
at once.

Some existing examples are,
c387d8177f20 ("compat: Add ipv6 GRE and IPV6 Tunneling")
744964326f6c ("datapath: compat: Backports nf_conncount")

Thanks,

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

Re: [ovs-dev] [PATCH v5 8/9] datapath: Add support for conntrack timeout policy

[0-day Robot]

Bleep bloop.  Greetings Yi-Hung Wei, 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 Dan Carpenter  needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Yi-Hung Wei 
Lines checked: 286, Warnings: 1, Errors: 1


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

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

Re: [ovs-dev] [PATCH] utilities: Improve ovs-dpctl-top flow parsing

[Ben Pfaff]

On Mon, Aug 12, 2019 at 06:52:27PM +0200, Jaime Caamaño Ruiz wrote:
> * check that expected bytes and packets stats are correctly read from
>   every flow.
> * check that the expected elements are read for every field type
>   aggregation.
> 
> Signed-off-by: Jaime Caamaño Ruiz 

Thanks!  I applied this to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] datapath: compat: Backports bugfixes for nf_conncount

[Ben Pfaff]

On Wed, Aug 07, 2019 at 03:25:33PM -0700, Yifeng Sun wrote:
> This patch backports several critical bug fixes related to
> locking and data consistency in nf_conncount code.
> 
> This backport is based on the following upstream net-next upstream commits.
> a007232 ("netfilter: nf_conncount: fix argument order to find_next_bit")
> c80f10b ("netfilter: nf_conncount: speculative garbage collection on empty 
> lists")
> 2f971a8 ("netfilter: nf_conncount: move all list iterations under spinlock")
> df4a902 ("netfilter: nf_conncount: merge lookup and add functions")
> e8cfb37 ("netfilter: nf_conncount: restart search when nodes have been 
> erased")
> f7fcc98 ("netfilter: nf_conncount: split gc in two phases")
> 4cd273b ("netfilter: nf_conncount: don't skip eviction when age is negative")
> c78e781 ("netfilter: nf_conncount: replace CONNCOUNT_LOCK_SLOTS with 
> CONNCOUNT_SLOTS")
> d4e7df1 ("netfilter: nf_conncount: use rb_link_node_rcu() instead of 
> rb_link_node()")
> 53ca0f2 ("netfilter: nf_conncount: remove wrong condition check routine")
> 3c5cdb1 ("netfilter: nf_conncount: fix unexpected permanent node of list.")
> 31568ec ("netfilter: nf_conncount: fix list_del corruption in conn_free")
> fd3e71a ("netfilter: nf_conncount: use spin_lock_bh instead of spin_lock")
> 
> This patch adds additional compat code so that it can build on
> all supported kernel versions.

I think that our most common approach is to use one OVS commit to
backport one Linux kernel commit.  This commit combines many Linux
kernel commits.  Is that an intentional change in this case?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2.12] OVN: Repair memory leak for OVN controller events.

[Ben Pfaff]

On Thu, Aug 15, 2019 at 02:12:18PM -0400, Mark Michelson wrote:
> Controller event action is leaking its genopts. This corrects the error.

Needs a Signed-off-by.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1] doc: Added OVS Nicira Extension document

[Ben Pfaff]

Thanks for working on this!  There's a lot here!  More comments below.

On Mon, Aug 12, 2019 at 12:00:26PM -0700, Ashish Varma wrote:
> OVS supports Nicira Extensions as various vendor messages or as vendor
> types in stats or multipart messages. Added a document to describe the
> Nicira extension as currently supported by OVS.

The commit message should not be indented.

> Signed-off-by: Ashish Varma 

This document is a bit unusual because it is written in the form of a
FAQ, but it isn't part of the FAQ.  I'd suggest either making it part of
the FAQ or writing it in the usual form for a document; the latter is
probably a bit more natural.

I have a slight terminology change request here.  We've often referred
to OVS's extensions to OpenFlow as "Nicira extensions".  That made more
sense a long time ago when OVS was really quite a bit more related to
Nicira than it is today. Today, Open vSwitch is very multi-vendor and
multi-company, and, in counterpoint, Nicira was absorbed into VMware
several years ago.  To me, this means that we should mostly stop talking
about "Nicira extensions" and instead talk about "Open vSwitch
extensions".  The only place where we really need to refer to Nicira is
a sentence or paragraph to explain why Nicira is sometimes mentioned
elsewhere (and why the constants start with NX).

> +Q: What are the Nicira vendor messages in OpenFlow and OVS?
> +
> +A: OpenFlow since version 1.0 allows 'vendor objects' to be added in the

The term "objects" here isn't a familiar one.  I think that "extensions"
would be a better term.

> +protocol and OVS has used this as 'Nicira extensions' in the
> +implementation. It has been used to add additional functionality for
> +the desired features not present in the standard OpenFlow protocol.
> +
> +There are two types of vendor or experimenter message types in OpenFlow:
> +
> +
> +1. **OFPT_VENDOR (In OpenFlow 1.0) or
> +   OFPT_EXPERIMENTER (In OpenFlow 1.1+)**
> +
> +This is a vendor message type with value the value of 4
> +in the OpenFlow header 'type' field. After the header of this message,
> +there is a vendor id field which identifies the vendor. This is followed
> +by a subtype field which defines the vendor specific message types.
> +Currently, following vendor ids are defined:

I think it would be better to simply refer to the header file here, to
prevent it from getting out of date.  It's worth mentioning NX_VENDOR_ID
and saying that it's the value to use for Open vSwitch extensions.

> +OVS uses the Nicira vendor id 0x2320 for all the vendor extension
> +messages. To see a list of all the Nicira vendor message subytes, we
> +can refer to 'ovs/lib/ofp-msgs.inc' file which gets auto generated after
> +compiling the ovs code. 

Wouldn't it be easier to look at ofp-msgs.h?  

> +Q: What is NXM?

I think that ovs-fields(7) provides all this information.  If people are
having trouble finding it, it might be worth just referring to that manpage.

Thanks,

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

[ovs-dev] [PATCH v5 9/9] ofproto-dpif-xlate: Translate timeout policy in ct action

[Yi-Hung Wei]

This patch derives the timeout policy based on ct zone from the
internal data structure that we maintain on dpif layer.

It also adds a system traffic test to verify the zone-based conntrack
timeout feature.  The test uses ovs-vsctl commands to configure
the customized ICMP and UDP timeout on zone 5 to a shorter period.
It then injects ICMP and UDP traffic to conntrack, and checks if the
corresponding conntrack entry expires after the predefined timeout.

Signed-off-by: Yi-Hung Wei 

ofproto-dpif: Checks if datapath supports OVS_CT_ATTR_TIMEOUT

This patch checks whether datapath supports OVS_CT_ATTR_TIMEOUT. With this
check, ofproto-dpif-xlate can use this information to decide whether to
translate the ct timeout policy.

Signed-off-by: Yi-Hung Wei 
---
 NEWS |  1 +
 lib/ct-dpif.c| 11 +
 lib/ct-dpif.h|  3 ++
 lib/dpif-netdev.c|  1 +
 lib/dpif-netlink.c   | 12 +
 lib/dpif-provider.h  | 10 +
 ofproto/ofproto-dpif-xlate.c | 23 ++
 ofproto/ofproto-dpif.c   | 96 
 ofproto/ofproto-dpif.h   | 12 -
 tests/system-kmod-macros.at  | 20 +
 tests/system-traffic.at  | 66 +++
 tests/system-userspace-macros.at | 19 
 12 files changed, 272 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index c5caa13d6374..9f7fbb852e08 100644
--- a/NEWS
+++ b/NEWS
@@ -69,6 +69,7 @@ v2.12.0 - xx xxx 
- Linux datapath:
  * Support for the kernel versions 4.19.x and 4.20.x.
  * Support for the kernel version 5.0.x.
+ * Add support for conntrack zone-based timeout policy.
- 'ovs-dpctl dump-flows' is no longer suitable for dumping offloaded flows.
  'ovs-appctl dpctl/dump-flows' should be used instead.
- Add L2 GRE tunnel over IPv6 support.
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index be59f34ff995..2181c83157ad 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -862,3 +862,14 @@ ct_dpif_timeout_policy_dump_done(struct dpif *dpif, void 
*state)
 ? dpif->dpif_class->ct_timeout_policy_dump_done(dpif, state)
 : EOPNOTSUPP);
 }
+
+int
+ct_dpif_get_timeout_policy_name(struct dpif *dpif, uint32_t tp_id,
+uint16_t dl_type, uint8_t nw_proto,
+struct ds *tp_name, bool *is_generic)
+{
+return (dpif->dpif_class->ct_get_timeout_policy_name
+? dpif->dpif_class->ct_get_timeout_policy_name(
+dpif, tp_id, dl_type, nw_proto, tp_name, is_generic)
+: EOPNOTSUPP);
+}
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index aabd6962f2c0..b95e6ac762ab 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -318,5 +318,8 @@ int ct_dpif_timeout_policy_dump_start(struct dpif *dpif, 
void **statep);
 int ct_dpif_timeout_policy_dump_next(struct dpif *dpif, void *state,
  struct ct_dpif_timeout_policy *tp);
 int ct_dpif_timeout_policy_dump_done(struct dpif *dpif, void *state);
+int ct_dpif_get_timeout_policy_name(struct dpif *dpif, uint32_t tp_id,
+uint16_t dl_type, uint8_t nw_proto,
+struct ds *tp_name, bool *is_generic);
 
 #endif /* CT_DPIF_H */
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 7240a3e6f3c8..36637052e598 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -7539,6 +7539,7 @@ const struct dpif_class dpif_netdev_class = {
 NULL,   /* ct_timeout_policy_dump_start */
 NULL,   /* ct_timeout_policy_dump_next */
 NULL,   /* ct_timeout_policy_dump_done */
+NULL,   /* ct_get_timeout_policy_name */
 dpif_netdev_ipf_set_enabled,
 dpif_netdev_ipf_set_min_frag,
 dpif_netdev_ipf_set_max_nfrags,
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 85827cd65503..8f1c6d1ffde7 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -3071,6 +3071,17 @@ dpif_netlink_format_tp_name(uint32_t id, uint16_t l3num, 
uint8_t l4num,
 ovs_assert(tp_name->length < CTNL_TIMEOUT_NAME_MAX);
 }
 
+static int
+dpif_netlink_ct_get_timeout_policy_name(struct dpif *dpif OVS_UNUSED,
+uint32_t tp_id, uint16_t dl_type, uint8_t nw_proto, struct ds *tp_name,
+bool *is_generic)
+{
+dpif_netlink_format_tp_name(tp_id,
+dl_type == ETH_TYPE_IP ? AF_INET : AF_INET6, nw_proto, tp_name);
+*is_generic = false;
+return 0;
+}
+
 #define CT_DPIF_NL_TP_TCP_MAPPINGS  \
 CT_DPIF_NL_TP_MAPPING(TCP, TCP, SYN_SENT, SYN_SENT) \
 CT_DPIF_NL_TP_MAPPING(TCP, TCP, SYN_RECV, SYN_RECV) \
@@ -3905,6 +3916,7 @@ const struct dpif_class dpif_netlink_class = {
 dpif_netlink_ct_timeout_policy_dump_start,
 dpif_netlink_ct_timeout_policy_dump_next,
 dpif_netlink_ct_timeout_policy_dump_done,
+dpif_n

[ovs-dev] [PATCH v5 7/9] datapath: compat: Backport nf_conntrack_timeout support

[Yi-Hung Wei]

This patch brings in nf_ct_timeout_put() and nf_ct_set_timeout()
when it is not available in the kernel.

Three symbols are created in acinclude.m4.

* HAVE_NF_CT_SET_TIMEOUT is used to determine if upstream net-next commit
717700d183d65 ("netfilter: Export nf_ct_{set,destroy}_timeout()") is
availabe.  If it is defined, the kernel should have all the
nf_conntrack_timeout support that OVS needs.

* HAVE_NF_CT_TIMEOUT is used to check if upstream net-next commit
6c1fd7dc489d9 ("netfilter: cttimeout: decouple timeout policy from
nfnetlink_cttimeout object") is there.  If it is not defined, we
will use the old ctnl_timeout interface rather than the nf_ct_timeout
interface that is introduced in this commit.

* HAVE_NF_CT_TIMEOUT_FIND_GET_HOOK_NET is used to check if upstream
commit 19576c9478682 ("netfilter: cttimeout: add netns support") is
there, so that we pass different arguement based on whether the kernel
has netns support.

Signed-off-by: Yi-Hung Wei 
---
 acinclude.m4   |   7 ++
 datapath/linux/Modules.mk  |   2 +
 .../include/net/netfilter/nf_conntrack_timeout.h   |  34 +++
 datapath/linux/compat/nf_conntrack_timeout.c   | 102 +
 4 files changed, 145 insertions(+)
 create mode 100644 
datapath/linux/compat/include/net/netfilter/nf_conntrack_timeout.h
 create mode 100644 datapath/linux/compat/nf_conntrack_timeout.c

diff --git a/acinclude.m4 b/acinclude.m4
index 116ffcf9096d..61fe4faa006a 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -714,6 +714,13 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_seqadj.h], 
[nf_ct_seq_adjust])
   OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_count.h], 
[nf_conncount_gc_list],
   [OVS_DEFINE([HAVE_UPSTREAM_NF_CONNCOUNT])])
+  OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_timeout.h], 
[nf_ct_set_timeout])
+  OVS_GREP_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_timeout.h], 
[struct nf_ct_timeout],
+  [OVS_DEFINE([HAVE_NF_CT_TIMEOUT])])
+  OVS_FIND_PARAM_IFELSE([$KSRC/include/net/netfilter/nf_conntrack_timeout.h],
+[\(*nf_ct_timeout_find_get_hook\)], [net],
+[OVS_DEFINE([HAVE_NF_CT_TIMEOUT_FIND_GET_HOOK_NET])])
+
 
   OVS_GREP_IFELSE([$KSRC/include/linux/random.h], [prandom_u32])
   OVS_GREP_IFELSE([$KSRC/include/linux/random.h], [prandom_u32_max])
diff --git a/datapath/linux/Modules.mk b/datapath/linux/Modules.mk
index cbb29f1c69d0..f93097b8e0e5 100644
--- a/datapath/linux/Modules.mk
+++ b/datapath/linux/Modules.mk
@@ -21,6 +21,7 @@ openvswitch_sources += \
linux/compat/nf_conntrack_core.c \
linux/compat/nf_conntrack_proto.c \
linux/compat/nf_conntrack_reasm.c \
+   linux/compat/nf_conntrack_timeout.c \
linux/compat/reciprocal_div.c \
linux/compat/skbuff-openvswitch.c \
linux/compat/socket.c \
@@ -108,6 +109,7 @@ openvswitch_headers += \
linux/compat/include/net/netfilter/nf_conntrack_helper.h \
linux/compat/include/net/netfilter/nf_conntrack_labels.h \
linux/compat/include/net/netfilter/nf_conntrack_seqadj.h \
+   linux/compat/include/net/netfilter/nf_conntrack_timeout.h \
linux/compat/include/net/netfilter/nf_conntrack_zones.h \
linux/compat/include/net/netfilter/nf_nat.h \
linux/compat/include/net/netfilter/ipv6/nf_defrag_ipv6.h \
diff --git a/datapath/linux/compat/include/net/netfilter/nf_conntrack_timeout.h 
b/datapath/linux/compat/include/net/netfilter/nf_conntrack_timeout.h
new file mode 100644
index ..134e72b8363e
--- /dev/null
+++ b/datapath/linux/compat/include/net/netfilter/nf_conntrack_timeout.h
@@ -0,0 +1,34 @@
+#ifndef _NF_CONNTRACK_TIMEOUT_WRAPPER_H
+#define _NF_CONNTRACK_TIMEOUT_WRAPPER_H
+
+#include_next 
+
+#ifndef HAVE_NF_CT_SET_TIMEOUT
+
+#ifndef HAVE_NF_CT_TIMEOUT
+#define nf_ct_timeout ctnl_timeout
+#endif
+
+#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
+int rpl_nf_ct_set_timeout(struct net *net, struct nf_conn *ct, u8 l3num, u8 
l4num,
+ const char *timeout_name);
+void rpl_nf_ct_destroy_timeout(struct nf_conn *ct);
+#else
+static inline int rpl_nf_ct_set_timeout(struct net *net, struct nf_conn *ct,
+   u8 l3num, u8 l4num,
+   const char *timeout_name)
+{
+   return -EOPNOTSUPP;
+}
+
+static inline void rpl_nf_ct_destroy_timeout(struct nf_conn *ct)
+{
+   return;
+}
+#endif /* CONFIG_NF_CONNTRACK_TIMEOUT */
+
+#define nf_ct_set_timeout rpl_nf_ct_set_timeout
+#define nf_ct_destroy_timeout rpl_nf_ct_destroy_timeout
+
+#endif /* HAVE_NF_CT_SET_TIMEOUT */
+#endif /* _NF_CONNTRACK_TIMEOUT_WRAPPER_H */
diff --git a/datapath/linux/compat/nf_conntrack_timeout.c 
b/datapath/linux/compat/nf_conntrack_timeout.c
new file mode 100644
index ..c02baff5771b
--- /dev/null
+++ b/datapath/linux/compat/nf

[ovs-dev] [PATCH v5 8/9] datapath: Add support for conntrack timeout policy

[Yi-Hung Wei]

This patch adds support for specifying a timeout policy for a
connection in connection tracking system in kernel datapath.
The timeout policy will be attached to a connection when the
connection is committed to conntrack.

This patch introduces a new odp field OVS_CT_ATTR_TIMEOUT in the
ct action that specifies the timeout policy in the datapath.
In the following patch, during the upcall process, the vswitchd will use
the ct_zone to look up the corresponding timeout policy and fill
OVS_CT_ATTR_TIMEOUT if it is available.

The datapath code is from the following two net-next upstream commits.

Upstream commit:
commit 06bd2bdf19d2f3d22731625e1a47fa1dff5ac407
Author: Yi-Hung Wei 
Date:   Tue Mar 26 11:31:14 2019 -0700

openvswitch: Add timeout support to ct action

Add support for fine-grain timeout support to conntrack action.
The new OVS_CT_ATTR_TIMEOUT attribute of the conntrack action
specifies a timeout to be associated with this connection.
If no timeout is specified, it acts as is, that is the default
timeout for the connection will be automatically applied.

Example usage:
$ nfct timeout add timeout_1 inet tcp syn_sent 100 established 200
$ ovs-ofctl add-flow br0 
in_port=1,ip,tcp,action=ct(commit,timeout=timeout_1)

CC: Pravin Shelar 
CC: Pablo Neira Ayuso 
Signed-off-by: Yi-Hung Wei 
Acked-by: Pravin B Shelar 
Signed-off-by: David S. Miller 

commit 6d670497e01803b486aa72cc1a718401ab986896
Author: Dan Carpenter 
Date:   Tue Apr 2 09:53:14 2019 +0300

openvswitch: use after free in __ovs_ct_free_action()

We free "ct_info->ct" and then use it on the next line when we pass it
to nf_ct_destroy_timeout().  This patch swaps the order to avoid the use
after free.

Fixes: 06bd2bdf19d2 ("openvswitch: Add timeout support to ct action")
Signed-off-by: Dan Carpenter 
Acked-by: Yi-Hung Wei 
Signed-off-by: David S. Miller 

Signed-off-by: Yi-Hung Wei 
---
 datapath/conntrack.c  | 30 ++-
 datapath/linux/compat/include/linux/openvswitch.h |  4 +++
 lib/dpif-netdev.c |  4 +++
 lib/odp-util.c| 29 +++---
 tests/odp.at  |  1 +
 5 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index 292febb3c83e..f85d0a2572f6 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -86,6 +87,7 @@ struct ovs_conntrack_info {
u32 eventmask;  /* Mask of 1 << IPCT_*. */
struct md_mark mark;
struct md_labels labels;
+   char timeout[CTNL_TIMEOUT_NAME_MAX];
 #ifdef CONFIG_NF_NAT_NEEDED
struct nf_nat_range2 range;  /* Only present for SRC NAT and DST NAT. */
 #endif
@@ -1519,6 +1521,8 @@ static const struct ovs_ct_len_tbl 
ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
 #endif
[OVS_CT_ATTR_EVENTMASK] = { .minlen = sizeof(u32),
.maxlen = sizeof(u32) },
+   [OVS_CT_ATTR_TIMEOUT] = { .minlen = 1,
+ .maxlen = CTNL_TIMEOUT_NAME_MAX },
 };
 
 static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
@@ -1604,6 +1608,15 @@ static int parse_ct(const struct nlattr *attr, struct 
ovs_conntrack_info *info,
info->have_eventmask = true;
info->eventmask = nla_get_u32(a);
break;
+#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
+   case OVS_CT_ATTR_TIMEOUT:
+   memcpy(info->timeout, nla_data(a), nla_len(a));
+   if (!memchr(info->timeout, '\0', nla_len(a))) {
+   OVS_NLERR(log, "Invalid conntrack helper");
+   return -EINVAL;
+   }
+   break;
+#endif
 
default:
OVS_NLERR(log, "Unknown conntrack attr (%d)",
@@ -1685,6 +1698,14 @@ int ovs_ct_copy_action(struct net *net, const struct 
nlattr *attr,
OVS_NLERR(log, "Failed to allocate conntrack template");
return -ENOMEM;
}
+
+   if (ct_info.timeout[0]) {
+   if (nf_ct_set_timeout(net, ct_info.ct, family, key->ip.proto,
+ ct_info.timeout))
+   pr_info_ratelimited("Failed to associated timeout "
+   "policy `%s'\n", ct_info.timeout);
+   }
+
if (helper) {
err = ovs_ct_add_helper(&ct_info, helper, key, log);
if (err)
@@ -1809,6 +1830,10 @@ int ovs_ct_action_to_attr(const struct 
ovs_conntrack_info *ct_info,
if (ct_info->have_eventmask &&
nla_put_u32(skb, OVS_CT_ATTR_EVENTMASK, ct_info->eventmask))

[ovs-dev] [PATCH v5 6/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

[Yi-Hung Wei]

This patch consumes the CT_Zone and CT_Timeout_Policy tables, maintains
the zone-based configuration in the vswitchd.  Whenever there is a
database change, vswitchd will read the datapath, CT_Zone, and
CT_Timeout_Policy tables from ovsdb, builds an internal snapshot of the
database configuration in bridge.c, and pushes down the change into
ofproto and dpif layer.

If a new zone-based timeout policy is added, it updates the zone to
timeout policy mapping in the per datapath type datapath structure in
dpif-backer, and pushes down the timeout policy into the datapath via
dpif interface.

If a timeout policy is no longer used, for kernel datapath, vswitchd
may not be able to remove it from datapath immediately since
datapath flows can still reference the to-be-deleted timeout policies.
Thus, we keep an timeout policy kill list, that vswitchd will go
back to the list periodically and try to kill the unused timeout policies.

Signed-off-by: Yi-Hung Wei 
---
 ofproto/ofproto-dpif.c | 294 +
 ofproto/ofproto-dpif.h |  10 ++
 ofproto/ofproto-provider.h |  10 ++
 ofproto/ofproto.c  |  26 
 ofproto/ofproto.h  |   5 +
 vswitchd/bridge.c  | 199 ++
 6 files changed, 544 insertions(+)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 751535249e21..4b4c4d722645 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -156,6 +156,31 @@ struct ofport_dpif {
 size_t n_qdscp;
 };
 
+struct ct_timeout_policy {
+int ref_count;  /* The number of ct zones that use this
+ * timeout policy. */
+uint32_t tp_id; /* Timeout policy id in the datapath. */
+struct simap tp;/* A map from timeout policy attribute to
+ * timeout value. */
+struct hmap_node node;  /* Element in struct dpif_backer's "ct_tps"
+ * cmap. */
+struct ovs_list list_node;  /* Element in struct dpif_backer's
+ * "ct_tp_kill_list" list. */
+};
+
+/* Periodically try to purge deleted timeout policies from the datapath. Retry
+ * may be necessary if the kernel datapath has a non-zero datapath flow
+ * reference count for the timeout policy. */
+#define TIMEOUT_POLICY_CLEANUP_INTERVAL (2) /* 20 seconds. */
+static long long int timeout_policy_cleanup_timer = LLONG_MIN;
+
+struct ct_zone {
+uint16_t zone_id;
+struct ct_timeout_policy *ct_tp;
+struct cmap_node node;  /* Element in struct dpif_backer's
+ * "ct_zones" cmap. */
+};
+
 static odp_port_t ofp_port_to_odp_port(const struct ofproto_dpif *,
ofp_port_t);
 
@@ -196,6 +221,9 @@ static struct hmap all_ofproto_dpifs_by_uuid =
 
 static bool ofproto_use_tnl_push_pop = true;
 static void ofproto_unixctl_init(void);
+static void ct_zone_config_init(struct dpif_backer *backer);
+static void ct_zone_config_uninit(struct dpif_backer *backer);
+static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
 
 static inline struct ofproto_dpif *
 ofproto_dpif_cast(const struct ofproto *ofproto)
@@ -488,6 +516,7 @@ type_run(const char *type)
 }
 
 process_dpif_port_changes(backer);
+ct_zone_timeout_policy_sweep(backer);
 
 return 0;
 }
@@ -683,6 +712,7 @@ close_dpif_backer(struct dpif_backer *backer, bool del)
 }
 dpif_close(backer->dpif);
 id_pool_destroy(backer->meter_ids);
+ct_zone_config_uninit(backer);
 free(backer);
 }
 
@@ -811,6 +841,8 @@ open_dpif_backer(const char *type, struct dpif_backer 
**backerp)
 backer->meter_ids = NULL;
 }
 
+ct_zone_config_init(backer);
+
 /* Make a pristine snapshot of 'support' into 'boottime_support'.
  * 'boottime_support' can be checked to prevent 'support' to be changed
  * beyond the datapath capabilities. In case 'support' is changed by
@@ -5086,6 +5118,266 @@ ct_flush(const struct ofproto *ofproto_, const uint16_t 
*zone)
 ct_dpif_flush(ofproto->backer->dpif, zone, NULL);
 }
 
+static struct ct_timeout_policy *
+ct_timeout_policy_lookup(const struct hmap *ct_tps, struct simap *tp)
+{
+struct ct_timeout_policy *ct_tp;
+
+HMAP_FOR_EACH_WITH_HASH (ct_tp, node, simap_hash(tp), ct_tps) {
+if (simap_equal(&ct_tp->tp, tp)) {
+return ct_tp;
+}
+}
+return NULL;
+}
+
+static struct ct_timeout_policy *
+ct_timeout_policy_alloc__(void)
+{
+struct ct_timeout_policy *ct_tp = xzalloc(sizeof *ct_tp);
+simap_init(&ct_tp->tp);
+return ct_tp;
+}
+
+static struct ct_timeout_policy *
+ct_timeout_policy_alloc(struct simap *tp, struct id_pool *tp_ids)
+{
+struct simap_node *node;
+
+struct ct_timeout_policy *ct_tp = ct_timeout_policy_alloc__();
+SIMAP_FOR_EACH (node, tp) {
+simap_put(&ct_tp->tp, node->name, node->data);
+}
+
+if (!

[ovs-dev] [PATCH v5 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support

[Yi-Hung Wei]

This patch first defines the dpif interface for a datapath to support
adding, deleting, getting and dumping conntrack timeout policy.
The timeout policy is identified by a 4 bytes unsigned integer in
datapath, and it currently support timeout for TCP, UDP, and ICMP
protocols.

Moreover, this patch provides the implementation for Linux kernel
datapath in dpif-netlink.

In Linux kernel, the timeout policy is maintained per L3/L4 protocol,
and it is identified by 32 bytes null terminated string.  On the other
hand, in vswitchd, the timeout policy is a generic one that consists of
all the supported L4 protocols.  Therefore, one of the main task in
dpif-netlink is to break down the generic timeout policy into 6
sub policies (ipv4 tcp, udp, icmp, and ipv6 tcp, udp, icmp),
and push down the configuration using the netlink API in
netlink-conntrack.c.

This patch also adds missing symbols in the windows datapath so
that the build on windows can pass.

Appveyor CI:
* https://ci.appveyor.com/project/YiHungWei/ovs/builds/26387754

Signed-off-by: Yi-Hung Wei 
Acked-by: Alin Gabriel Serdean 
---
 Documentation/faq/releases.rst |   3 +-
 datapath-windows/include/OvsDpInterfaceCtExt.h | 114 +
 datapath-windows/ovsext/Netlink/NetlinkProto.h |   8 +-
 include/windows/automake.mk|   1 +
 .../windows/linux/netfilter/nfnetlink_cttimeout.h  |   0
 lib/ct-dpif.c  | 102 +
 lib/ct-dpif.h  |  56 +++
 lib/dpif-netdev.c  |   6 +
 lib/dpif-netlink.c | 480 -
 lib/dpif-netlink.h |   1 -
 lib/dpif-provider.h|  44 ++
 lib/netlink-conntrack.c| 301 +
 lib/netlink-conntrack.h|  27 +-
 lib/netlink-protocol.h |   8 +-
 14 files changed, 1142 insertions(+), 9 deletions(-)
 create mode 100644 include/windows/linux/netfilter/nfnetlink_cttimeout.h

diff --git a/Documentation/faq/releases.rst b/Documentation/faq/releases.rst
index 8daa23bb2d0c..0b7eaab1b143 100644
--- a/Documentation/faq/releases.rst
+++ b/Documentation/faq/releases.rst
@@ -110,8 +110,9 @@ Q: Are all features available with all datapaths?
 == == == = ===
 Connection tracking 4.3YES  YES  YES
 Conntrack Fragment Reass.   4.3YES  YES  YES
+Conntrack Timeout Policies  5.2YES  NO   NO
+Conntrack Zone Limit4.18   YES  NO   YES
 NAT 4.6YES  YES  YES
-Conntrack zone limit4.18   YES  NO   YES
 Tunnel - LISP   NO YES  NO   NO
 Tunnel - STTNO YES  NO   YES
 Tunnel - GRE3.11   YES  YES  YES
diff --git a/datapath-windows/include/OvsDpInterfaceCtExt.h 
b/datapath-windows/include/OvsDpInterfaceCtExt.h
index 3b947782e90c..3379f0a256fa 100644
--- a/datapath-windows/include/OvsDpInterfaceCtExt.h
+++ b/datapath-windows/include/OvsDpInterfaceCtExt.h
@@ -421,4 +421,118 @@ struct nf_ct_tcp_flags {
 UINT8 mask;
 };
 
+/* File: nfnetlink_cttimeout.h. XXX: the following are not implemented */
+enum ctnl_timeout_msg_types {
+IPCTNL_MSG_TIMEOUT_NEW,
+IPCTNL_MSG_TIMEOUT_GET,
+IPCTNL_MSG_TIMEOUT_DELETE,
+IPCTNL_MSG_TIMEOUT_DEFAULT_SET,
+IPCTNL_MSG_TIMEOUT_DEFAULT_GET,
+
+IPCTNL_MSG_TIMEOUT_MAX
+};
+
+enum ctattr_timeout {
+CTA_TIMEOUT_UNSPEC,
+CTA_TIMEOUT_NAME,
+CTA_TIMEOUT_L3PROTO,
+CTA_TIMEOUT_L4PROTO,
+CTA_TIMEOUT_DATA,
+CTA_TIMEOUT_USE,
+__CTA_TIMEOUT_MAX
+};
+#define CTA_TIMEOUT_MAX (__CTA_TIMEOUT_MAX - 1)
+
+enum ctattr_timeout_generic {
+CTA_TIMEOUT_GENERIC_UNSPEC,
+CTA_TIMEOUT_GENERIC_TIMEOUT,
+__CTA_TIMEOUT_GENERIC_MAX
+};
+#define CTA_TIMEOUT_GENERIC_MAX (__CTA_TIMEOUT_GENERIC_MAX - 1)
+
+enum ctattr_timeout_tcp {
+CTA_TIMEOUT_TCP_UNSPEC,
+CTA_TIMEOUT_TCP_SYN_SENT,
+CTA_TIMEOUT_TCP_SYN_RECV,
+CTA_TIMEOUT_TCP_ESTABLISHED,
+CTA_TIMEOUT_TCP_FIN_WAIT,
+CTA_TIMEOUT_TCP_CLOSE_WAIT,
+CTA_TIMEOUT_TCP_LAST_ACK,
+CTA_TIMEOUT_TCP_TIME_WAIT,
+CTA_TIMEOUT_TCP_CLOSE,
+CTA_TIMEOUT_TCP_SYN_SENT2,
+CTA_TIMEOUT_TCP_RETRANS,
+CTA_TIMEOUT_TCP_UNACK,
+__CTA_TIMEOUT_TCP_MAX
+};
+#define CTA_TIMEOUT_TCP_MAX (__CTA_TIMEOUT_TCP_MAX - 1)
+
+enum ctattr_timeout_udp {
+CTA_TIMEOUT_UDP_UNSPEC,
+CTA_TIMEOUT_UDP_UNREPLIED,
+CTA_TIMEOUT_UDP_REPLIED,
+__CTA_TIMEOUT_UDP_MAX
+};
+#define CTA_TIMEOUT_UDP_MAX (__CTA_TIMEOUT_UDP_MAX - 1)
+
+enum ctattr_timeout_udplite {
+CTA_TIMEOUT_UDPLITE_UNSPEC,
+CTA

[ovs-dev] [PATCH v5 5/9] simap: Add utility function to help compare two simaps.

[Yi-Hung Wei]

From: Ben Pfaff 

Signed-off-by: Ben Pfaff 
---
 lib/simap.c | 15 ++-
 lib/simap.h |  1 +
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/lib/simap.c b/lib/simap.c
index d634f8ed9eea..f404ece67703 100644
--- a/lib/simap.c
+++ b/lib/simap.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011, 2012, 2017 Nicira, Inc.
+ * Copyright (c) 2009, 2010, 2011, 2012, 2017, 2019 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -242,6 +242,19 @@ simap_equal(const struct simap *a, const struct simap *b)
 return true;
 }
 
+uint32_t
+simap_hash(const struct simap *simap)
+{
+uint32_t hash = 0;
+
+const struct simap_node *node;
+SIMAP_FOR_EACH (node, simap) {
+hash ^= hash_int(node->data,
+ hash_name(node->name, strlen(node->name)));
+}
+return hash;
+}
+
 static size_t
 hash_name(const char *name, size_t length)
 {
diff --git a/lib/simap.h b/lib/simap.h
index 5b4a2f39dca3..5e646e660782 100644
--- a/lib/simap.h
+++ b/lib/simap.h
@@ -70,6 +70,7 @@ bool simap_find_and_delete(struct simap *, const char *);
 
 const struct simap_node **simap_sort(const struct simap *);
 bool simap_equal(const struct simap *, const struct simap *);
+uint32_t simap_hash(const struct simap *);
 
 #ifdef  __cplusplus
 }
-- 
2.7.4

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

[ovs-dev] [PATCH v5 3/9] ct-dpif: Export ct_dpif_format_ipproto()

[Yi-Hung Wei]

This function will be useful for following patches.

Signed-off-by: Yi-Hung Wei 
Acked-by: Justin Pettit 
---
 lib/ct-dpif.c | 3 +--
 lib/ct-dpif.h | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index 5d8a75d3a63f..6ea7feb0ee35 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -31,7 +31,6 @@ struct flags {
 const char *name;
 };
 
-static void ct_dpif_format_ipproto(struct ds *, uint16_t ipproto);
 static void ct_dpif_format_counters(struct ds *,
 const struct ct_dpif_counters *);
 static void ct_dpif_format_timestamp(struct ds *,
@@ -315,7 +314,7 @@ ct_dpif_format_entry(const struct ct_dpif_entry *entry, 
struct ds *ds,
 }
 }
 
-static void
+void
 ct_dpif_format_ipproto(struct ds *ds, uint16_t ipproto)
 {
 const char *name;
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index 14178bb7c3f0..2f4906817946 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -250,6 +250,7 @@ int ct_dpif_ipf_dump_done(struct dpif *dpif, void *);
 void ct_dpif_entry_uninit(struct ct_dpif_entry *);
 void ct_dpif_format_entry(const struct ct_dpif_entry *, struct ds *,
   bool verbose, bool print_stats);
+void ct_dpif_format_ipproto(struct ds *ds, uint16_t ipproto);
 void ct_dpif_format_tuple(struct ds *, const struct ct_dpif_tuple *);
 uint8_t ct_dpif_coalesce_tcp_state(uint8_t state);
 void ct_dpif_format_tcp_stat(struct ds *, int, int);
-- 
2.7.4

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

[ovs-dev] [PATCH v5 2/9] ovs-vsctl: Add conntrack zone commands.

[Yi-Hung Wei]

From: William Tu 

The patch adds commands creating/deleting/listing conntrack zone
timeout policies:
  $ ovs-vsctl {add,del,list}-zone-tp dp zone=zone_id ...

Signed-off-by: William Tu 
---
 tests/ovs-vsctl.at   |  34 +++-
 utilities/ovs-vsctl.8.in |  26 ++
 utilities/ovs-vsctl.c| 202 ++-
 3 files changed, 256 insertions(+), 6 deletions(-)

diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index 46fa3c5b1a33..df15fb6901a0 100644
--- a/tests/ovs-vsctl.at
+++ b/tests/ovs-vsctl.at
@@ -805,6 +805,20 @@ AT_CHECK(
   [RUN_OVS_VSCTL([--if-exists remove netflow x targets '"1.2.3.4:567"'])])
 AT_CHECK(
   [RUN_OVS_VSCTL([--if-exists clear netflow x targets])])
+
+AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 -- set 
Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1 
icmp_reply=2])])
+AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-tp netdev zone=1 icmp_first=1 
icmp_reply=2])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout 
Policies: icmp_first=1 icmp_reply=2
+])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 
icmp_reply=3])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout 
Policies: icmp_first=1 icmp_reply=2
+Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3
+])
+AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])])
+AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=1])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout 
Policies: icmp_first=2 icmp_reply=3
+])
 OVS_VSCTL_CLEANUP
 AT_CLEANUP
 
@@ -890,10 +904,10 @@ AT_CHECK([RUN_OVS_VSCTL([set bridge br0 flood_vlans=-1])],
 AT_CHECK([RUN_OVS_VSCTL([set bridge br0 flood_vlans=4096])],
   [1], [], [ovs-vsctl: constraint violation: 4096 is not in the valid range 0 
to 4095 (inclusive)
 ])
-AT_CHECK([RUN_OVS_VSCTL([set c br1 'connection-mode=xyz'])],
+AT_CHECK([RUN_OVS_VSCTL([set controller br1 'connection-mode=xyz'])],
   [1], [], [[ovs-vsctl: constraint violation: xyz is not one of the allowed 
values ([in-band, out-of-band])
 ]])
-AT_CHECK([RUN_OVS_VSCTL([set c br1 connection-mode:x=y])],
+AT_CHECK([RUN_OVS_VSCTL([set controller br1 connection-mode:x=y])],
   [1], [], [ovs-vsctl: cannot specify key to set for non-map column 
connection_mode
 ])
 AT_CHECK([RUN_OVS_VSCTL([add bridge br1 datapath_id x y])],
@@ -929,6 +943,22 @@ AT_CHECK([RUN_OVS_VSCTL([remove bridge br1 flood-vlans 
true])],
 AT_CHECK([RUN_OVS_VSCTL([clear bridge br1 name])],
   [1], [], [ovs-vsctl: cannot modify read-only column name in table Bridge
 ])
+
+AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 -- set 
Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx zone=1 icmp_first=1 
icmp_reply=2])],
+  [1], [], [ovs-vsctl: datapath netdevxx does not exist
+])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 
icmp_reply=3])])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 
icmp_reply=3])],
+  [1], [], [ovs-vsctl: zone id 2 already exists
+])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout 
Policies: icmp_first=2 icmp_reply=3
+])
+AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=11])],
+  [1], [], [ovs-vsctl: zone id 11 does not exist
+])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout 
Policies: icmp_first=2 icmp_reply=3
+])
 OVS_VSCTL_CLEANUP
 AT_CLEANUP
 
diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
index 7c09df79bd29..5b9883ae1c3d 100644
--- a/utilities/ovs-vsctl.8.in
+++ b/utilities/ovs-vsctl.8.in
@@ -353,6 +353,32 @@ list.
 Prints the name of the bridge that contains \fIiface\fR on standard
 output.
 .
+.SS "Conntrack Zone Commands"
+These commands query and modify datapath CT zones and Timeout Policies.
+.
+.IP "[\fB\-\-may\-exist\fR] \fBadd\-zone\-tp \fIdatapath \fBzone=\fIzone_id 
\fIpolicies\fR"
+Creates a conntrack zone timeout policy with \fIzone_id\fR in
+\fIdatapath\fR.  The \fIpolicies\fR consist of \fIkey\fB=\fIvalue\fR
+pairs, separated by spaces.  For example, \fBicmp_first=30
+icmp_reply=60\fR specifies a 30-second timeout policy for the first ICMP
+packet and a 60-second policy for ICMP reply packet.  See the
+\fBCT_Timeout_Policy\fR table in \fBovs-vswitchd.conf.db\fR(5) for the
+supported keys.
+.IP
+Without \fB\-\-may\-exist\fR, attempting to add a \fIzone_id\fR that
+already exists is an error.  With \fB\-\-may\-exist\fR,
+this command does nothing if \fIzone_id\fR is already created\fR.
+.
+.IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-tp \fIdatapath \fBzone=\fIzone_id\fR"
+Delete the timeout policy associated with \fIzone_id\fR from \fIdatapath\fR.
+.IP
+Without \fB\-\-if\-exists\fR, attempting to delete a zone that
+does not exist is an error.  With \fB\-\-if\-exists\fR, attempting to
+delete a zone that does not exist has no effect.
+.
+.IP "\fBlis

[ovs-dev] [PATCH v5 1/9] ovs-vswitchd: Add Datapath, CT_Zone, and CT_Zone_Policy tables.

[Yi-Hung Wei]

From: Justin Pettit 

Signed-off-by: Justin Pettit 
Signed-off-by: Yi-Hung Wei 
Co-authored-by: Yi-Hung Wei 
---
 vswitchd/vswitch.ovsschema |  51 -
 vswitchd/vswitch.xml   | 275 +
 2 files changed, 277 insertions(+), 49 deletions(-)

diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
index f7c6eb8983cd..c0a2242ad345 100644
--- a/vswitchd/vswitch.ovsschema
+++ b/vswitchd/vswitch.ovsschema
@@ -1,9 +1,14 @@
 {"name": "Open_vSwitch",
- "version": "8.0.0",
- "cksum": "3962141869 23978",
+ "version": "8.1.0",
+ "cksum": "1635647160 26090",
  "tables": {
"Open_vSwitch": {
  "columns": {
+   "datapaths": {
+ "type": {"key": {"type": "string"},
+  "value": {"type": "uuid",
+"refTable": "Datapath"},
+  "min": 0, "max": "unlimited"}},
"bridges": {
  "type": {"key": {"type": "uuid",
   "refTable": "Bridge"},
@@ -629,6 +634,48 @@
   "min": 0, "max": "unlimited"},
  "ephemeral": true}},
  "indexes": [["target"]]},
+   "Datapath": {
+ "columns": {
+   "datapath_version": {
+ "type": "string"},
+   "ct_zones": {
+ "type": {"key": {"type": "integer",
+  "minInteger": 0,
+  "maxInteger": 65535},
+  "value": {"type": "uuid",
+"refTable": "CT_Zone"},
+  "min": 0, "max": "unlimited"}},
+   "external_ids": {
+ "type": {"key": "string", "value": "string",
+  "min": 0, "max": "unlimited",
+   "CT_Zone": {
+ "columns": {
+   "timeout_policy": {
+ "type": {"key": {"type": "uuid",
+  "refTable": "CT_Timeout_Policy"},
+  "min": 0, "max": 1}},
+   "external_ids": {
+ "type": {"key": "string", "value": "string",
+  "min": 0, "max": "unlimited",
+   "CT_Timeout_Policy": {
+ "columns": {
+   "timeouts": {
+ "type": {"key": {"type" : "string",
+  "enum": ["set", ["tcp_syn_sent", "tcp_syn_recv",
+   "tcp_established", "tcp_fin_wait",
+   "tcp_close_wait", "tcp_last_ack",
+   "tcp_time_wait", "tcp_close",
+   "tcp_syn_sent2", "tcp_retransmit",
+   "tcp_unack", "udp_first",
+   "udp_single", "udp_multiple",
+   "icmp_first", "icmp_reply"]]},
+  "value": {"type" : "integer",
+"minInteger" : 0,
+"maxInteger" : 4294967295},
+  "min": 0, "max": "unlimited"}},
+   "external_ids": {
+ "type": {"key": "string", "value": "string",
+  "min": 0, "max": "unlimited",
"SSL": {
  "columns": {
"private_key": {
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 9a743c05b4bf..08586110db51 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -52,6 +52,13 @@
 one record in the  table.
 
 
+  
+Map of datapath types to datapaths.  The
+ column of the 
+table is used as a key for this map.  The value points to a row in
+the  table.
+  
+
   
 Set of bridges managed by the daemon.
   
@@ -1217,53 +1224,11 @@
   
 
   
-
-  Reports the version number of the Open vSwitch datapath in use.
-  This allows management software to detect and report discrepancies
-  between Open vSwitch userspace and datapath versions.  (The  column in the  reports the Open vSwitch userspace version.)
-  The version reported depends on the datapath in use:
-
-
-
-  
-When the kernel module included in the Open vSwitch source tree is
-used, this column reports the Open vSwitch version from which the
-module was taken.
-  
-
-  
-When the kernel module that is part of the upstream Linux kernel is
-used, this column reports .
-  
-
-  
-When the datapath is built into the ovs-vswitchd
-binary, this column reports .  A
-built-in datapath is by definition the same version as the rest of
-the Open VSwitch userspace.
-  
-
-  
-Other datapaths (such as the Hyper-V kernel datapath) currently
-report .
-  
-
-
-
-  A version discrepancy between ovs-vswitchd and the
-  datapath in use is not normally cause for alarm.  The Open vSwitch
-  kernel datapaths for Linux and Hyper-

[ovs-dev] [PATCH v5 0/9] Support zone-based conntrack timeout policy

[Yi-Hung Wei]

This patch series enables zone-based conntrack timeout policy support in OVS.
Timeout policy is a set of timeout attributes that can be associated with a
connection when it is committed.  Then, the connection tracking system will
expire a connection based on its connection state.  For example, one use
case would be to extend the timeout of TCP connection in the established
state to avoid re-connect overhead. Other use case may be to shorten the
connection timeout so that the system can reclaim resources faster.
The idea of zone-based conntrack timeout policy is to group connections
with similar characteristics in a conntrack zone, and assign timeout policy
to the conntrack zone.  In this way, all the connections in that zone will share
the same timeout policy.

For zone-based timeout policy configuration, the association of conntrack
zone and conntrack timeout policy is defined per datapath in vswitchd ovsdb
schema.  User can program the database through ovs-vsctl or using ovsdb
protocol directly.  Once the zone-based timeout policy configuration is
in the database, vswitchd will read those configuration and organize it
in internal datapath structure, and push the timeout policy into datapath.
Currently, only the kernel datapath supports customized timeout policy.

When a packet is committed to connection tracking system, during flow
translation in ofproto-dpif-xlate, vswitchd will lookup the internal
data structure to figure out which timeout policy to associate with
the connection.  If timeout policy is not specified to the committed
zone, it defaults to the timeout policy in the default zone (zone 0).
If the timeout policy is not specified in the default zone, it defaults
to the system default timeouts.

Here are some more details about each patch
* p01: Introduce ovsdb schema for ct timeout policy.
* p02: ovs-vsctl commands to configure zone-based ct timeout policy.
* p03: Expose a utility functions.
* p04: dpif interface along with dpif-netlink implementation to support
   ct timeout policy.
* p05: Consume ct timeout policy configuration from ovsdb server,
   keep it in internal data structure, and push configuration to
   datapath.
* p06: Add utility function to help compare two simaps.
* p07-08: Kernel datapath support for the new ct action attribute.
* p09: Translate timeout policy in ofproto-dpif-xlate and system traffic test.

v4->v5:
* Fold in Darrell's suggestion on patch 4 and patch 6 in v4 review.
* Rebase to master.

v3->v4:
* ofproto-dpif
- Probe datapath for timeout policy support.
- With the probing information only translate timeout policy when
  the datapath is supported.
- Resolve the old kernel compatibility issue reported by Darrell.
* system-traffic
- Simplify the testing script (diff from Darrell).
* Address various code changes as in the mailing list discussion.

v2->v3
* ovsdb schema
- Fold in changes from Justin.
- Make ct timeout policy key to be in a pre-defined set.
* ovs-vsctl
- Bug fixes.
* ct-dpif
- Fold in diff suggestion from Justin.
* bridge, ofproto-dpif
- Restruct the ofproto and dpif layer support for zone based timeout
  policy.
* system traffic test
- Fix bug reported by Darrell.
* Address review comments from Justin and Darrell.

v1->v2

* ovs-vsctl
- Remove add-dp,del-dp,list-dp ovs-vsctl commands.
- Add --may-exist and --if-exists to ovs-vsctl add-zone-tp command.
- Improve ovs-vsctl test.
* ct-dpif, dpif-netlink
- Remove support to change default timeout policy in the datapath.
- Squash ct-dpif and dpif-netlink layer implementation altogether.
- Address review comments from William.
* ofproto-dpif
- Remove changes from datapath-config module to ofproto-dpif layer.
- Maintain zone-based timeout policy in dpif-backer since this is
  per datapath type configuration. This will not break the OVS
  hierarchy as Ilya suggested.
- Allocate timeout policy id using id_pool instead of idl_seqno
  as Darrell suggested.
- Add a timeout policy sweep function that clean up unnecessary
  timeout policy regularly in the datapath.
* ofproto-dpif-xlate
- Only translate ct timeout policy if it is a ct commit action
  in kernel datapath.
* system-traffic test
- Update system traffic test with low level ovs-vsctl command.
- Make system traffic test to be datapath type agnostic.
- Improve system traffic test as Darrell suggested.
* Rebase to master

Ben Pfaff (1):
  simap: Add utility function to help compare two simaps.

Justin Pettit (1):
  ovs-vswitchd: Add Datapath, CT_Zone, and CT_Zone_Policy tables.

William Tu (1):
  ovs-vsctl: Add conntrack zone commands.

Yi-Hung Wei (6):
  ct-dpif: Export ct_dpif_format_ipproto()
  ct-dpif, dpif-netlink: Add conntrack timeout policy support
  ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables
  datapath: compat: Backport nf_conntrack_timeout support
  datapath: Add support for conntrack timeout pol

Re: [ovs-dev] [PATCH v2] flow: miniflow_extract metadata branchless optimization

[Ben Pfaff]

This fails to apply to current master.

Whereas most of the time I'd be comfortable with reviewing 'flow'
patches myself, this one is closed related to the dpif-netdev code and
I'd prefer to have someone who understands that code well, as well as
the tradeoffs between performance and maintainability, review it.  Ilya
(added to the To line) is a good choice.

On Thu, Aug 22, 2019 at 06:09:16PM +0800, Yanqin Wei wrote:
> "miniflow_extract" is a branch heavy implementation for packet header and
> metadata parsing. There is a lot of meta data handling for all traffic.
> But this should not be applicable for packets from interface.
> This patch adds a layer of inline encapsulation to miniflow_extract and
> introduces constant "md_valid" input parameter as a branch condition.
> The new branch will be removed by the compiler at compile time. Two
> instances of miniflow_extract with different branches will be generated.
> 
> This patch is tested on an arm64 platform. It improves more than 3.5%
> performance in P2P forwarding cases.
> 
> Reviewed-by: Gavin Hu 
> Signed-off-by: Yanqin Wei 
> ---
>  lib/dpif-netdev.c |  13 +++---
>  lib/flow.c| 116 
> --
>  lib/flow.h|   2 +
>  3 files changed, 79 insertions(+), 52 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index d0a1c58..6686b14 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -6508,12 +6508,15 @@ dfc_processing(struct dp_netdev_pmd_thread *pmd,
>  }
>  }
>  
> -miniflow_extract(packet, &key->mf);
> +if (!md_is_valid) {
> +miniflow_extract_firstpass(packet, &key->mf);
> +key->hash =
> +dpif_netdev_packet_get_rss_hash_orig_pkt(packet, &key->mf);
> +} else {
> +miniflow_extract(packet, &key->mf);
> +key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf);
> +}
>  key->len = 0; /* Not computed yet. */
> -key->hash =
> -(md_is_valid == false)
> -? dpif_netdev_packet_get_rss_hash_orig_pkt(packet, &key->mf)
> -: dpif_netdev_packet_get_rss_hash(packet, &key->mf);
>  
>  /* If EMC is disabled skip emc_lookup */
>  flow = (cur_min != 0) ? emc_lookup(&cache->emc_cache, key) : NULL;
> diff --git a/lib/flow.c b/lib/flow.c
> index e54fd2e..e5b554b 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -707,7 +707,8 @@ ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr *nh, 
> size_t size)
>  }
>  
>  /* Initializes 'dst' from 'packet' and 'md', taking the packet type into
> - * account.  'dst' must have enough space for FLOW_U64S * 8 bytes.
> + * account.  'dst' must have enough space for FLOW_U64S * 8 bytes. Metadata
> + * initialization should be bypassed if "md_valid" is false.
>   *
>   * Initializes the layer offsets as follows:
>   *
> @@ -732,8 +733,9 @@ ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr *nh, 
> size_t size)
>   *  present and the packet has at least the content used for the fields
>   *  of interest for the flow, otherwise UINT16_MAX.
>   */
> -void
> -miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
> +static inline ALWAYS_INLINE void
> +miniflow_extract__(struct dp_packet *packet, struct miniflow *dst,
> +const bool md_valid)
>  {
>  /* Add code to this function (or its callees) to extract new fields. */
>  BUILD_ASSERT_DECL(FLOW_WC_SEQ == 41);
> @@ -752,54 +754,60 @@ miniflow_extract(struct dp_packet *packet, struct 
> miniflow *dst)
>  ovs_be16 ct_tp_src = 0, ct_tp_dst = 0;
>  
>  /* Metadata. */
> -if (flow_tnl_dst_is_set(&md->tunnel)) {
> -miniflow_push_words(mf, tunnel, &md->tunnel,
> -offsetof(struct flow_tnl, metadata) /
> -sizeof(uint64_t));
> -
> -if (!(md->tunnel.flags & FLOW_TNL_F_UDPIF)) {
> -if (md->tunnel.metadata.present.map) {
> -miniflow_push_words(mf, tunnel.metadata, 
> &md->tunnel.metadata,
> -sizeof md->tunnel.metadata /
> -sizeof(uint64_t));
> -}
> -} else {
> -if (md->tunnel.metadata.present.len) {
> -miniflow_push_words(mf, tunnel.metadata.present,
> -&md->tunnel.metadata.present, 1);
> -miniflow_push_words(mf, tunnel.metadata.opts.gnv,
> -md->tunnel.metadata.opts.gnv,
> -
> DIV_ROUND_UP(md->tunnel.metadata.present.len,
> - sizeof(uint64_t)));
> +if (md_valid) {
> +if (flow_tnl_dst_is_set(&md->tunnel)) {
> +miniflow_push_words(mf, tunnel, &md->tunnel,
> +offsetof(struct flow_tnl, metadata) /
> +  

Re: [ovs-dev] [PATCH v2] flow: fix incorrect padding length checking and combine branch in ipv6_sanity_check

[Ben Pfaff]

On Thu, Aug 22, 2019 at 06:09:34PM +0800, Yanqin Wei wrote:
> The padding length is (packet size - ipv6 header length - ipv6 plen).  This
> patch fixes incorrect ipv6 size checking and improves it by combining branch.
> 
> Reviewed-by: Gavin Hu 
> Signed-off-by: Yanqin Wei 
> ---
>  lib/flow.c | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/flow.c b/lib/flow.c
> index e5b554b..1b21f51 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -688,18 +688,16 @@ ipv4_get_nw_frag(const struct ip_header *nh)
>  static inline bool
>  ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr *nh, size_t size)
>  {
> -uint16_t plen;
> +int pad_len;
>  
>  if (OVS_UNLIKELY(size < sizeof *nh)) {
>  return false;
>  }
>  
> -plen = ntohs(nh->ip6_plen);
> -if (OVS_UNLIKELY(plen + IPV6_HEADER_LEN > size)) {
> -return false;
> -}
> +pad_len = size - IPV6_HEADER_LEN - ntohs(nh->ip6_plen);

The types in the above calculation worry me.  Writing the type of each
term:

int = size_t - int - uint16_t

The most likely type of the right side of the expression is size_t.
Assigning this to an 'int' might do "the right thing" for negative
values, but it's risky--especially since size_t and int might be
different widths.  I think it would be safer to cast the first and third
terms to int, e.g.:

pad_len = (int) size - IPV6_HEADER_LEN - (int) ntohs(nh->ip6_plen);

>  /* Jumbo Payload option not supported yet. */
> -if (OVS_UNLIKELY(size - plen > UINT8_MAX)) {
> +if (OVS_UNLIKELY(pad_len < 0 || pad_len > UINT8_MAX)) {
>  return false;
>  }

Thanks,

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

Re: [ovs-dev] [PATCH] flow: Reduce metadata connection state branches in miniflow_extract

[Ben Pfaff]

On Tue, Aug 27, 2019 at 01:21:08PM -0500, Malvika Gupta wrote:
> This patch merges two separate if-else branches for metadata connection state
> into one if-else branch to improve performance. It gives an average 
> performance
> improvement of ~3% on arm platforms and ~4.5% on x86 platforms.
> 
> Signed-off-by: Malvika Gupta 
> Reviewed-by: Yanqin Wei 
> Reviewed-by: Gavin Hu 

That's an incredible impact.

Thanks, I applied this to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 ] flow: save "vlan_hdrs" memset for untagged traffic

[Ben Pfaff]

On Thu, Aug 22, 2019 at 06:09:50PM +0800, Yanqin Wei wrote:
> For untagged traffic, it is unnecessary to clear vlan_hdrs as it costs 32B
> memset. So the patch improves it by postponing to clear vlan_hdrs until
> ethtype check. It can benefit both untagged and single-tagged traffic. From
> testing, it does not impact performance of dual-tagged traffic.
> 
> Reviewed-by: Gavin Hu 
> Signed-off-by: Yanqin Wei 

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

Re: [ovs-dev] Hypervisor down during upgrade OVS 2.10.x to 2.10.y

[Jin, Liang via dev]

Hi,
We upgrade the OVS recently from one version 2.10 to another version 2.10.  on 
some HV upgrade, the HV is down when running force reload kernel.
In the ovs-ctl log, kill ovs-vswitch is failed, but the script is still going 
to reload the modules.
```
ovsdb-server is running with pid 2431
ovs-vswitchd is running with pid 2507
Thu Aug 22 23:13:49 UTC 2019:stop
2019-08-22T23:13:50Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
2019-08-22T23:13:51Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
* Exiting ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507) with SIGKILL
* Killing ovs-vswitchd (2507) failed
* Exiting ovsdb-server (2431)
Thu Aug 22 23:14:58 UTC 2019:load-kmod
Thu Aug 22 23:14:58 UTC 2019:start --system-id=random --no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Starting ovsdb-server
* Configuring Open vSwitch system IDs
* ovs-vswitchd is already running
* Enabling remote OVSDB managers
ovsdb-server is running with pid 3860447
ovs-vswitchd is running with pid 2507
ovsdb-server is running with pid 3860447
ovs-vswitchd is running with pid 2507
Thu Aug 22 23:15:09 UTC 2019:load-kmod
Thu Aug 22 23:15:09 UTC 2019:force-reload-kmod --system-id=random 
--no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Detected internal interfaces: br-int
Thu Aug 22 23:37:08 UTC 2019:stop
2019-08-22T23:37:09Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
2019-08-22T23:37:10Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
* Exiting ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507) with SIGKILL
* Killing ovs-vswitchd (2507) failed
* Exiting ovsdb-server (3860447)
Thu Aug 22 23:40:42 UTC 2019:load-kmod
* Inserting openvswitch module
Thu Aug 22 23:40:42 UTC 2019:start --system-id=random --no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Starting ovsdb-server
* Configuring Open vSwitch system IDs
* Starting ovs-vswitchd
* Enabling remote OVSDB managers
ovsdb-server is running with pid 2399
ovs-vswitchd is running with pid 2440
ovsdb-server is running with pid 2399
ovs-vswitchd is running with pid 2440
Thu Aug 22 23:46:18 UTC 2019:load-kmod
Thu Aug 22 23:46:18 UTC 2019:force-reload-kmod --system-id=random 
--no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Detected internal interfaces: br-int br0
* Saving flows
* Exiting ovsdb-server (2399)
* Starting ovsdb-server
* Configuring Open vSwitch system IDs
* Flush old conntrack entries
* Exiting ovs-vswitchd (2440)
* Saving interface configuration
* Removing datapath: system@ovs-system
* Removing openvswitch module
rmmod: ERROR: Module vxlan is in use by: i40e
* Forcing removal of vxlan module
* Inserting openvswitch module
* Starting ovs-vswitchd
* Restoring saved flows
* Enabling remote OVSDB managers
* Restoring interface configuration
```

But in kern.log, we see the log as below, the process could not exit because 
waiting br0 release,  and then, the ovs-ctl try to `kill term` and `kill -9` 
the process, it does not work, because kernel is in infinity loop.  Then, 
ovs-ctl try to save the flows, when save flow, core dump happened in kernel. 
Then HV is down until restart it.
```
Aug 22 16:13:45 slx11c-9gjm kernel: [21177057.998961] device br0 left 
promiscuous mode
Aug 22 16:13:55 slx11c-9gjm kernel: [21177068.044859] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:05 slx11c-9gjm kernel: [21177078.068429] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:15 slx11c-9gjm kernel: [21177088.312001] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:25 slx11c-9gjm kernel: [21177098.359556] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:35 slx11c-9gjm kernel: [21177108.603058] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:45 slx11c-9gjm kernel: [21177118.658643] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:55 slx11c-9gjm kernel: [21177128.678211] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:15:05 slx11c-9gjm kernel: [21177138.721732] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:15:15 slx11c-9gjm kernel: [21177148.817317] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:15:25 slx11c-9gjm kernel: [21177158.828853] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:15:35 slx11c-9gjm kernel: [21177168.860459] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 

Re: [ovs-dev] Hypervisor down during upgrade OVS 2.10.x to 2.10.y

[Jin, Liang via dev]

Hi,
We upgrade the OVS recently from one version 2.10 to another version 2.10.  on 
some HV upgrade, the HV is down when running force reload kernel.
In the ovs-ctl log, kill ovs-vswitch is failed, but the script is still going 
to reload the modules.
```
ovsdb-server is running with pid 2431
ovs-vswitchd is running with pid 2507
Thu Aug 22 23:13:49 UTC 2019:stop
2019-08-22T23:13:50Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
2019-08-22T23:13:51Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
* Exiting ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507) with SIGKILL
* Killing ovs-vswitchd (2507) failed
* Exiting ovsdb-server (2431)
Thu Aug 22 23:14:58 UTC 2019:load-kmod
Thu Aug 22 23:14:58 UTC 2019:start --system-id=random --no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Starting ovsdb-server
* Configuring Open vSwitch system IDs
* ovs-vswitchd is already running
* Enabling remote OVSDB managers
ovsdb-server is running with pid 3860447
ovs-vswitchd is running with pid 2507
ovsdb-server is running with pid 3860447
ovs-vswitchd is running with pid 2507
Thu Aug 22 23:15:09 UTC 2019:load-kmod
Thu Aug 22 23:15:09 UTC 2019:force-reload-kmod --system-id=random 
--no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Detected internal interfaces: br-int
Thu Aug 22 23:37:08 UTC 2019:stop
2019-08-22T23:37:09Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
2019-08-22T23:37:10Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
* Exiting ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507) with SIGKILL
* Killing ovs-vswitchd (2507) failed
* Exiting ovsdb-server (3860447)
Thu Aug 22 23:40:42 UTC 2019:load-kmod
* Inserting openvswitch module
Thu Aug 22 23:40:42 UTC 2019:start --system-id=random --no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Starting ovsdb-server
* Configuring Open vSwitch system IDs
* Starting ovs-vswitchd
* Enabling remote OVSDB managers
ovsdb-server is running with pid 2399
ovs-vswitchd is running with pid 2440
ovsdb-server is running with pid 2399
ovs-vswitchd is running with pid 2440
Thu Aug 22 23:46:18 UTC 2019:load-kmod
Thu Aug 22 23:46:18 UTC 2019:force-reload-kmod --system-id=random 
--no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Detected internal interfaces: br-int br0
* Saving flows
* Exiting ovsdb-server (2399)
* Starting ovsdb-server
* Configuring Open vSwitch system IDs
* Flush old conntrack entries
* Exiting ovs-vswitchd (2440)
* Saving interface configuration
* Removing datapath: system@ovs-system
* Removing openvswitch module
rmmod: ERROR: Module vxlan is in use by: i40e
* Forcing removal of vxlan module
* Inserting openvswitch module
* Starting ovs-vswitchd
* Restoring saved flows
* Enabling remote OVSDB managers
* Restoring interface configuration
```

But in kern.log, we see the log as below, the process could not exit because 
waiting br0 release,  and then, the ovs-ctl try to `kill term` and `kill -9` 
the process, it does not work, because kernel is in infinity loop.  Then, 
ovs-ctl try to save the flows, when save flow, core dump happened in kernel. 
Then HV is down until restart it.
```
Aug 22 16:13:45 slx11c-9gjm kernel: [21177057.998961] device br0 left 
promiscuous mode
Aug 22 16:13:55 slx11c-9gjm kernel: [21177068.044859] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:05 slx11c-9gjm kernel: [21177078.068429] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:15 slx11c-9gjm kernel: [21177088.312001] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:25 slx11c-9gjm kernel: [21177098.359556] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:35 slx11c-9gjm kernel: [21177108.603058] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:45 slx11c-9gjm kernel: [21177118.658643] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:55 slx11c-9gjm kernel: [21177128.678211] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:15:05 slx11c-9gjm kernel: [21177138.721732] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:15:15 slx11c-9gjm kernel: [21177148.817317] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:15:25 slx11c-9gjm kernel: [21177158.828853] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:15:35 slx11c-9gjm kernel: [21177168.860459] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 2

[ovs-dev] Hypervisor down during upgrade OVS 2.10.x to 2.10.y

[Jin, Liang via dev]

Hi,
We upgrade the OVS recently from one version 2.10 to another version 2.10.  on 
some HV upgrade, the HV is down when running force reload kernel.
In the ovs-ctl log, kill ovs-vswitch is failed, but the script is still going 
to reload the modules.
```
ovsdb-server is running with pid 2431
ovs-vswitchd is running with pid 2507
Thu Aug 22 23:13:49 UTC 2019:stop
2019-08-22T23:13:50Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
2019-08-22T23:13:51Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
* Exiting ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507) with SIGKILL
* Killing ovs-vswitchd (2507) failed
* Exiting ovsdb-server (2431)
Thu Aug 22 23:14:58 UTC 2019:load-kmod
Thu Aug 22 23:14:58 UTC 2019:start --system-id=random --no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Starting ovsdb-server
* Configuring Open vSwitch system IDs
* ovs-vswitchd is already running
* Enabling remote OVSDB managers
ovsdb-server is running with pid 3860447
ovs-vswitchd is running with pid 2507
ovsdb-server is running with pid 3860447
ovs-vswitchd is running with pid 2507
Thu Aug 22 23:15:09 UTC 2019:load-kmod
Thu Aug 22 23:15:09 UTC 2019:force-reload-kmod --system-id=random 
--no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Detected internal interfaces: br-int
Thu Aug 22 23:37:08 UTC 2019:stop
2019-08-22T23:37:09Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
2019-08-22T23:37:10Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
* Exiting ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507) with SIGKILL
* Killing ovs-vswitchd (2507) failed
* Exiting ovsdb-server (3860447)
Thu Aug 22 23:40:42 UTC 2019:load-kmod
* Inserting openvswitch module
Thu Aug 22 23:40:42 UTC 2019:start --system-id=random --no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Starting ovsdb-server
* Configuring Open vSwitch system IDs
* Starting ovs-vswitchd
* Enabling remote OVSDB managers
ovsdb-server is running with pid 2399
ovs-vswitchd is running with pid 2440
ovsdb-server is running with pid 2399
ovs-vswitchd is running with pid 2440
Thu Aug 22 23:46:18 UTC 2019:load-kmod
Thu Aug 22 23:46:18 UTC 2019:force-reload-kmod --system-id=random 
--no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Detected internal interfaces: br-int br0
* Saving flows
* Exiting ovsdb-server (2399)
* Starting ovsdb-server
* Configuring Open vSwitch system IDs
* Flush old conntrack entries
* Exiting ovs-vswitchd (2440)
* Saving interface configuration
* Removing datapath: system@ovs-system
* Removing openvswitch module
rmmod: ERROR: Module vxlan is in use by: i40e
* Forcing removal of vxlan module
* Inserting openvswitch module
* Starting ovs-vswitchd
* Restoring saved flows
* Enabling remote OVSDB managers
* Restoring interface configuration
```

But in kern.log, we see the log as below, the process could not exit because 
waiting br0 release,  and then, the ovs-ctl try to `kill term` and `kill -9` 
the process, it does not work, because kernel is in infinity loop.  Then, 
ovs-ctl try to save the flows, when save flow, core dump happened in kernel. 
Then HV is down until restart it.
```
Aug 22 16:13:45 slx11c-9gjm kernel: [21177057.998961] device br0 left 
promiscuous mode
Aug 22 16:13:55 slx11c-9gjm kernel: [21177068.044859] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:05 slx11c-9gjm kernel: [21177078.068429] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:15 slx11c-9gjm kernel: [21177088.312001] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:25 slx11c-9gjm kernel: [21177098.359556] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:35 slx11c-9gjm kernel: [21177108.603058] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:45 slx11c-9gjm kernel: [21177118.658643] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:55 slx11c-9gjm kernel: [21177128.678211] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:15:05 slx11c-9gjm kernel: [21177138.721732] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:15:15 slx11c-9gjm kernel: [21177148.817317] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:15:25 slx11c-9gjm kernel: [21177158.828853] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:15:35 slx11c-9gjm kernel: [21177168.860459] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 

Re: [ovs-dev] Hypervisor down during upgrade OVS 2.10.x to 2.10.y

[Jin, Liang via dev]

From: "Jin, Liang" 
Date: Wednesday, August 28, 2019 at 3:58 AM
To: "ovs-dev@openvswitch.org" 
Cc: "Zhou, Han" , "Murthy, Sudheendra(sumurthy)" 

Subject: Hypervisor down during upgrade OVS 2.10.x to 2.10.y

Hi,
We upgrade the OVS recently from one version 2.10 to another version 2.10.  on 
some HV upgrade, the HV is down when running force reload kernel.
In the ovs-ctl log, kill ovs-vswitch is failed, but the script is still going 
to reload the modules.
```
ovsdb-server is running with pid 2431
ovs-vswitchd is running with pid 2507
Thu Aug 22 23:13:49 UTC 2019:stop
2019-08-22T23:13:50Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
2019-08-22T23:13:51Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
* Exiting ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507) with SIGKILL
* Killing ovs-vswitchd (2507) failed
* Exiting ovsdb-server (2431)
Thu Aug 22 23:14:58 UTC 2019:load-kmod
Thu Aug 22 23:14:58 UTC 2019:start --system-id=random --no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Starting ovsdb-server
* Configuring Open vSwitch system IDs
* ovs-vswitchd is already running
* Enabling remote OVSDB managers
ovsdb-server is running with pid 3860447
ovs-vswitchd is running with pid 2507
ovsdb-server is running with pid 3860447
ovs-vswitchd is running with pid 2507
Thu Aug 22 23:15:09 UTC 2019:load-kmod
Thu Aug 22 23:15:09 UTC 2019:force-reload-kmod --system-id=random 
--no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Detected internal interfaces: br-int
Thu Aug 22 23:37:08 UTC 2019:stop
2019-08-22T23:37:09Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
2019-08-22T23:37:10Z|1|fatal_signal|WARN|terminating with signal 14 (Alarm 
clock)
Alarm clock
* Exiting ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507)
* Killing ovs-vswitchd (2507) with SIGKILL
* Killing ovs-vswitchd (2507) failed
* Exiting ovsdb-server (3860447)
Thu Aug 22 23:40:42 UTC 2019:load-kmod
* Inserting openvswitch module
Thu Aug 22 23:40:42 UTC 2019:start --system-id=random --no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Starting ovsdb-server
* Configuring Open vSwitch system IDs
* Starting ovs-vswitchd
* Enabling remote OVSDB managers
ovsdb-server is running with pid 2399
ovs-vswitchd is running with pid 2440
ovsdb-server is running with pid 2399
ovs-vswitchd is running with pid 2440
Thu Aug 22 23:46:18 UTC 2019:load-kmod
Thu Aug 22 23:46:18 UTC 2019:force-reload-kmod --system-id=random 
--no-full-hostname
/usr/share/openvswitch/scripts/ovs-ctl: unknown option "--no-full-hostname" 
(use --help for help)
* Detected internal interfaces: br-int br0
* Saving flows
* Exiting ovsdb-server (2399)
* Starting ovsdb-server
* Configuring Open vSwitch system IDs
* Flush old conntrack entries
* Exiting ovs-vswitchd (2440)
* Saving interface configuration
* Removing datapath: system@ovs-system
* Removing openvswitch module
rmmod: ERROR: Module vxlan is in use by: i40e
* Forcing removal of vxlan module
* Inserting openvswitch module
* Starting ovs-vswitchd
* Restoring saved flows
* Enabling remote OVSDB managers
* Restoring interface configuration
```

But in kern.log, we see the log as below, the process could not exit because 
waiting br0 release,  and then, the ovs-ctl try to `kill term` and `kill -9` 
the process, it does not work, because kernel is in infinity loop.  Then, 
ovs-ctl try to save the flows, when save flow, core dump happened in kernel. 
Then HV is down until restart it.
```
Aug 22 16:13:45 slx11c-9gjm kernel: [21177057.998961] device br0 left 
promiscuous mode
Aug 22 16:13:55 slx11c-9gjm kernel: [21177068.044859] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:05 slx11c-9gjm kernel: [21177078.068429] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:15 slx11c-9gjm kernel: [21177088.312001] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:25 slx11c-9gjm kernel: [21177098.359556] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:35 slx11c-9gjm kernel: [21177108.603058] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:45 slx11c-9gjm kernel: [21177118.658643] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:14:55 slx11c-9gjm kernel: [21177128.678211] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:15:05 slx11c-9gjm kernel: [21177138.721732] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:15:15 slx11c-9gjm kernel: [21177148.817317] unregister_netdevice: 
waiting for br0 to become free. Usage count = 1
Aug 22 16:15:25 slx11c-9gjm kernel: [21177158.

Re: [ovs-dev] [PATCH v4 ovn 0/4] External OVS source support and separate run dir for OVN

[Mark Michelson]

Acked-by: Mark Michelson 

On 8/28/19 12:38 PM, nusid...@redhat.com wrote:

From: Numan Siddique 

This patch series adds support for building OVN from external OVS
sources.

The first patch adds the support to compile OVN from external OVS sources.
The following configuration options are added when configuring OVN
   * --with-ovs-source (mandatory)
   * --with-ovs-build (optional)

Patch 2 adds support to run OVN services using separate
directores
   - Default run time dir - /usr/local/var/run/ovm
   - Default log dir - /usr/loca/var/log/ovn
   - Default db dir - /usr/loca/etc/ovn

Patch 3 fixes "make rpm-fedora" which is presently broken

Patch 4 runs OVN services as openvswitch user for rhel when rpms are
used.

v3 -> v4
===
  * Rebased to latest master to resolve merge conflict in p2

v2 -> v3
===
  * Added the support to provide the ovs source and build dirs as
relative paths as suggested By Ben in the irc meeting.
  * Dropped patch 5 from the series which was added in v2.
Patch 5 deleted the python subdirectory in the ovn repo. But that
patch is failing in travis CI. It will be submitted separately
once I get the chance to work on it and the fix the issue.


v1 -> v2

  * Addressed the review comments.
  * Swapped the patch 1 and 2 as it was easier to address Mark's comment
on OVS_RUNDIR/OVN_RUNDIR
  * In patch 2, renamed m4/openvswitch.m4 to m4/ovn.m4 and renamed few of
the macros to OVS_* to OVN_*.

  * Combined the patch 1 and 2 in this series which were submitted
separately earlier.

Numan Siddique (4):
   Build OVN using external OVS directory
   Add support for using OVN specific rundirs
   Fix "make rpm-fedora"
   rhel: Run ovn services with the 'openvswitch' user

  .travis/linux-build.sh|  17 +-
  .travis/osx-build.sh  |  13 +-
  Documentation/intro/install/fedora.rst|  13 +-
  Documentation/intro/install/general.rst   |  63 ++--
  Makefile.am   |  30 +-
  TODO_SPLIT.rst|   2 +
  acinclude.m4  |  43 +++
  configure.ac  |  63 ++--
  controller-vtep/automake.mk   |   2 +-
  controller/ovn-controller.c   |   4 +-
  include/ovn/version.h.in  |  28 ++
  lib/.gitignore|   1 +
  lib/automake.mk   |  21 +-
  lib/ovn-dirs.c.in | 112 +++
  lib/ovn-dirs.h|  35 ++
  lib/ovn-util.c|  24 +-
  lib/ovn-util.h|   1 +
  lib/ovsdb_automake.mk |   7 +-
  m4/{openvswitch.m4 => ovn.m4} |  60 ++--
  northd/ovn-northd.c   |   9 +-
  rhel/automake.mk  |   5 +-
  rhel/etc_logrotate.d_ovn  |  22 ++
  rhel/ovn-fedora.spec.in   |  91 --
  ...systemd_system_ovn-controller-vtep.service |  15 +-
  ..._lib_systemd_system_ovn-controller.service |   9 +-
  .../usr_lib_systemd_system_ovn-northd.service |  15 +-
  ...are_ovn_scripts_systemd_sysconfig.template |  13 +
  tests/automake.mk |   6 +-
  tests/ofproto-macros.at   |   4 +-
  tests/ovn-controller-vtep.at  |  12 +-
  tests/ovn-nbctl.at|   6 +-
  tests/ovn-sbctl.at|  20 +-
  tests/ovn.at  | 158 -
  tests/ovs-macros.at   |   1 +
  tests/ovsdb-macros.at |   2 +-
  tutorial/automake.mk  |   2 +-
  tutorial/ovs-sandbox  | 309 +-
  utilities/automake.mk |   5 +
  utilities/ovn-ctl |  86 +++--
  utilities/ovn-ctl.8.xml   |  12 +-
  utilities/ovn-lib.in  | 204 
  41 files changed, 1085 insertions(+), 460 deletions(-)
  create mode 100644 include/ovn/version.h.in
  create mode 100644 lib/ovn-dirs.c.in
  create mode 100644 lib/ovn-dirs.h
  rename m4/{openvswitch.m4 => ovn.m4} (94%)
  create mode 100644 rhel/etc_logrotate.d_ovn
  create mode 100644 rhel/usr_share_ovn_scripts_systemd_sysconfig.template
  create mode 100644 utilities/ovn-lib.in



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

Re: [ovs-dev] [PATCH v3] userspace: Enable non-bridge port as tunnel endpoint.

[Darrell Ball]

Thanks for the patch

How about writing a system test ?

Darrell

On Wed, Aug 28, 2019 at 10:50 AM Yifeng Sun  wrote:

> For userspace datapath, currently only the bridge itself, the LOCAL port,
> can be the tunnel endpoint to encap/decap tunnel packets.  This patch
> enables non-bridge port as tunnel endpoint.  One use case is for users to
> create a bridge and a vtep port as tap, and configure underlay IP at vtep
> port as the tunnel endpoint.
>
> This patch causes failure for test "ptap - L3 over patch port". This is
> because this test is already using non-bridge port gre1 as tunnel endpoint.
> In this test, an extra flow is added to support this, as shown below:
>   ovs-ofctl add-flow br1 in_port=p1,actions=output=gre1
>
> It later generates a datapath flow which matches an extra eth field:
>   - recirc_id(0),...,eth_type(0x0800),...
>   + recirc_id(0),...,eth(dst=1e:2c:e9:2a:66:9e),eth_type(0x0800),...
>
> With this patch, the above flow is no longer needed.
>
> Signed-off-by: William Tu 
> Co-authored-by: William Tu 
> Signed-off-by: Yifeng Sun 
> ---
> v1->v2: Fixed an error pointed out by Ben.
> v2->v3: Fixed a test failure, thanks Ben for review and testing!
>  ofproto/ofproto-dpif-xlate.c | 56
> +++-
>  tests/packet-type-aware.at   |  1 -
>  tests/tunnel-push-pop.at | 55
> +++
>  3 files changed, 100 insertions(+), 12 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 02a2a4535542..290924634f36 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3410,6 +3410,19 @@ tnl_route_lookup_flow(const struct xlate_ctx *ctx,
>  }
>  }
>  }
> +
> +/* If tunnel IP isn't configured on bridges, then we search all
> ports. */
> +HMAP_FOR_EACH (xbridge, hmap_node, &ctx->xcfg->xbridges) {
> +struct xport *port;
> +
> +HMAP_FOR_EACH (port, ofp_node, &xbridge->xports) {
> +if (!strncmp(netdev_get_name(port->netdev),
> + out_dev, IFNAMSIZ)) {
> +*out_port = port;
> +return 0;
> +}
> +}
> +}
>  return -ENOENT;
>  }
>
> @@ -3972,6 +3985,16 @@ is_nd_dst_correct(const struct flow *flow, const
> struct in6_addr *ipv6_addr)
>  IN6_ARE_ADDR_EQUAL(&flow->ipv6_dst, ipv6_addr);
>  }
>
> +static bool
> +is_neighbor_reply_matched(const struct flow *flow, struct in6_addr
> *ip_addr)
> +{
> +return ((IN6_IS_ADDR_V4MAPPED(ip_addr) &&
> + flow->dl_type == htons(ETH_TYPE_ARP) &&
> + in6_addr_get_mapped_ipv4(ip_addr) == flow->nw_dst) ||
> +(!IN6_IS_ADDR_V4MAPPED(ip_addr) &&
> +  is_nd_dst_correct(flow, ip_addr)));
> +}
> +
>  /* Function verifies if the ARP reply or Neighbor Advertisement
> represented by
>   * 'flow' addresses the 'xbridge' of 'ctx'. Returns true if the ARP TA or
>   * neighbor discovery destination is in the list of configured IP
> addresses of
> @@ -3986,11 +4009,7 @@ is_neighbor_reply_correct(const struct xlate_ctx
> *ctx, const struct flow *flow)
>  /* Verify if 'nw_dst' of ARP or 'ipv6_dst' of ICMPV6 is in the list.
> */
>  for (i = 0; xbridge_addr && i < xbridge_addr->n_addr; i++) {
>  struct in6_addr *ip_addr = &xbridge_addr->addr[i];
> -if ((IN6_IS_ADDR_V4MAPPED(ip_addr) &&
> - flow->dl_type == htons(ETH_TYPE_ARP) &&
> - in6_addr_get_mapped_ipv4(ip_addr) == flow->nw_dst) ||
> -(!IN6_IS_ADDR_V4MAPPED(ip_addr) &&
> -  is_nd_dst_correct(flow, ip_addr))) {
> +if (is_neighbor_reply_matched(flow, ip_addr)) {
>  /* Found a match. */
>  ret = true;
>  break;
> @@ -3998,20 +4017,35 @@ is_neighbor_reply_correct(const struct xlate_ctx
> *ctx, const struct flow *flow)
>  }
>
>  xbridge_addr_unref(xbridge_addr);
> +
> +/* If not found in bridge's IPs, search in its ports. */
> +if (!ret) {
> +struct in6_addr *ip_addr, *mask;
> +struct xport *port;
> +int error, n_in6;
> +
> +HMAP_FOR_EACH (port, ofp_node, &ctx->xbridge->xports) {
> +error = netdev_get_addr_list(port->netdev, &ip_addr,
> + &mask, &n_in6);
> +if (!error && is_neighbor_reply_matched(flow, ip_addr)) {
> +/* Found a match. */
> +ret = true;
> +break;
> +}
> +}
> +}
>  return ret;
>  }
>
>  static bool
> -terminate_native_tunnel(struct xlate_ctx *ctx, ofp_port_t ofp_port,
> -struct flow *flow, struct flow_wildcards *wc,
> -odp_port_t *tnl_port)
> +terminate_native_tunnel(struct xlate_ctx *ctx, struct flow *flow,
> +struct flow_wildcards *wc, odp_port_t *tnl_port)
>  {
>  *tnl_port = ODPP_NONE;
>
>  /* XXX: Write bett

Re: [ovs-dev] [PATCH branch-2.12] pinctrl: Fix DNS packet parsing

[Ben Pfaff]

On Fri, Aug 16, 2019 at 04:35:52PM +0200, Dumitru Ceara wrote:
> Please backport this to 2.11, 2.10, 2.9 at least.

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

Re: [ovs-dev] [PATCH v2.12] OVN: fix memory leak in build_pre_lb

[Ben Pfaff]

On Mon, Aug 26, 2019 at 11:19:32AM +0200, Lorenzo Bianconi wrote:
> Fix memory leak of ip_address string in build_pre_lb routine if we
> install logical flows for empty_lb controller event
> 
> Fixes: f49b17a6cbe3 ("OVN: use trigger_event action to report 'empty_lb_rule' 
> events")
> Signed-off-by: Lorenzo Bianconi 

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

Re: [ovs-dev] [PATCH 1/2] ovs-thread: Make struct spin lock cache aligned.

[Ben Pfaff]

On Mon, Aug 26, 2019 at 04:00:31PM -0700, William Tu wrote:
> Make the spin lock struct 64-byte aligned to avoid false sharing issue.
> 
> Signed-off-by: William Tu 

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

Re: [ovs-dev] [PATCH] tnl-neigh: Use outgoing ofproto version.

[Flavio Leitner via dev]

On Wed, Aug 28, 2019 at 11:29:54AM -0700, Ben Pfaff wrote:
> On Tue, Aug 13, 2019 at 01:34:04PM -0300, Flavio Leitner via dev wrote:
> > When a packet needs to be encapsulated in userspace, the endpoint
> > address needs to be resolved to fill in the headers. If it is not,
> > then currently OvS sends either a Neighbor Solicitation (IPv6)
> > or an ARP Query (IPv4) to resolve it.
> 
> Thanks, I applied this to master and backported it as far as it would go.

Thanks a lot Ben!
fbl
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] userspace: Enable non-bridge port as tunnel endpoint.

[Ben Pfaff]

On Wed, Aug 28, 2019 at 10:49:59AM -0700, Yifeng Sun wrote:
> For userspace datapath, currently only the bridge itself, the LOCAL port,
> can be the tunnel endpoint to encap/decap tunnel packets.  This patch
> enables non-bridge port as tunnel endpoint.  One use case is for users to
> create a bridge and a vtep port as tap, and configure underlay IP at vtep
> port as the tunnel endpoint.
> 
> This patch causes failure for test "ptap - L3 over patch port". This is
> because this test is already using non-bridge port gre1 as tunnel endpoint.
> In this test, an extra flow is added to support this, as shown below:
>   ovs-ofctl add-flow br1 in_port=p1,actions=output=gre1
> 
> It later generates a datapath flow which matches an extra eth field:
>   - recirc_id(0),...,eth_type(0x0800),...
>   + recirc_id(0),...,eth(dst=1e:2c:e9:2a:66:9e),eth_type(0x0800),...
> 
> With this patch, the above flow is no longer needed.
> 
> Signed-off-by: William Tu 
> Co-authored-by: William Tu 
> Signed-off-by: Yifeng Sun 

Thanks for the update.

I'm getting a different failure now:

../../tests/packet-type-aware.at:703: ovs-ofctl -OOpenFlow13 dump-flows br0 | 
ofctl_strip | grep actions
../../tests/packet-type-aware.at:708: ovs-ofctl -OOpenFlow13 dump-flows br1 | 
ofctl_strip | grep actions
--- -   2019-08-28 11:42:26.385258842 -0700
+++ /home/blp/nicira/ovs/_build/tests/testsuite.dir/at-groups/2787/stdout   
2019-08-28 11:42:26.380439131 -0700
@@ -1,2 +1 @@
- reset_counts in_port=20 actions=output:100
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] tnl-neigh: Use outgoing ofproto version.

[Ben Pfaff]

On Tue, Aug 13, 2019 at 01:34:04PM -0300, Flavio Leitner via dev wrote:
> When a packet needs to be encapsulated in userspace, the endpoint
> address needs to be resolved to fill in the headers. If it is not,
> then currently OvS sends either a Neighbor Solicitation (IPv6)
> or an ARP Query (IPv4) to resolve it.

Thanks, I applied this to master and backported it as far as it would go.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] rhel: Add case for RHEL 7.5 major version to kmod manage script

[Guru Shetty]

On Wed, 28 Aug 2019 at 10:42, Gregory Rose  wrote:

>
>
> On 8/28/2019 10:16 AM, Gregory Rose wrote:
> >
> >
> > On 8/28/2019 10:07 AM, Guru Shetty wrote:
> >> I applied this to master and 2.12. I got a conflict for 2.11. If you
> >> need this for 2.11, can you please rebase?
> >
> > Yes, it needs to go back to 2.10 where we introduce the %posttrans
> > scriptlet in rhel/openvswitch-kmod-fedora.spec.in
>
> I sent a patch for branch 2.11.  After looking more closely there is no
> need for the patch in 2.10.
>

Done. Applied to 2.11. Thanks for rebase!


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

[ovs-dev] [PATCH] ovn: Prevent erroneous duplicate IP address

[Mark Michelson]

When using dynamic address assignment for logical switches, OVN reserves
the first address in the subnet for the attached router port to use.

In commit 488d153ee87841c042af05bc0eb8b5481aaa98cf, the IPAM code was
modified to add assigned router port addresses to IPAM. The use case for
this was when a switch was joined to multiple routers, and all router
addresses were dynamically assigned.

However, that commit also made it so that when a router rightly claimed
the first address in the subnet, ovn-northd would issue a warning about
a duplicate IP address being set. This change fixes the issue by adding
a special case so that we don't add the router's IP address to IPAM if
it is the first address in the subnet. This prevents the warning message
from appearing.

Signed-off-by: Mark Michelson 
Acked-by: Numan Siddique 
Acked-by: Han ZHou 
---
This is a backport of a patch from the ovn repo.
---
 ovn/northd/ovn-northd.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 2abf1f897..7827830b6 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1194,7 +1194,14 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct 
ovn_port *op)
 
 for (size_t i = 0; i < lrp_networks.n_ipv4_addrs; i++) {
 uint32_t ip = ntohl(lrp_networks.ipv4_addrs[i].addr);
-ipam_insert_ip(op->peer->od, ip);
+/* If the router has the first IP address of the subnet, don't add
+ * it to IPAM. We already added this when we initialized IPAM for
+ * the datapath. This will just result in an erroneous message
+ * about a duplicate IP address.
+ */
+if (ip != op->peer->od->ipam_info.start_ipv4) {
+ipam_insert_ip(op->peer->od, ip);
+}
 }
 
 destroy_lport_addresses(&lrp_networks);
-- 
2.14.5

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

Re: [ovs-dev] [PATCH branch-2.11] rhel: Add case for RHEL 7.5 major version to kmod manage script

[0-day Robot]

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

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


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Gurucharan Shetty 
Lines checked: 35, Warnings: 1, Errors: 0


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

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

Re: [ovs-dev] [PATCHv2 ovn] Prevent erroneous duplicate IP address messages.

[Mark Michelson]

I pushed this to master.

On 8/8/19 5:00 PM, Mark Michelson wrote:

When using dynamic address assignment for logical switches, OVN reserves
the first address in the subnet for the attached router port to use.

In commit 488d153ee87841c042af05bc0eb8b5481aaa98cf, the IPAM code was
modified to add assigned router port addresses to IPAM. The use case for
this was when a switch was joined to multiple routers, and all router
addresses were dynamically assigned.

However, that commit also made it so that when a router rightly claimed
the first address in the subnet, ovn-northd would issue a warning about
a duplicate IP address being set. This change fixes the issue by adding
a special case so that we don't add the router's IP address to IPAM if
it is the first address in the subnet. This prevents the warning message
from appearing.

Signed-off-by: Mark Michelson 
Acked-by: Numan Siddique 
Acked-by: Han ZHou 
---
v1->v2: Added "ovn" to subject line
---
  northd/ovn-northd.c | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index e6953a405..f27fb70e4 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -1194,7 +1194,14 @@ ipam_add_port_addresses(struct ovn_datapath *od, struct 
ovn_port *op)
  
  for (size_t i = 0; i < lrp_networks.n_ipv4_addrs; i++) {

  uint32_t ip = ntohl(lrp_networks.ipv4_addrs[i].addr);
-ipam_insert_ip(op->peer->od, ip);
+/* If the router has the first IP address of the subnet, don't add
+ * it to IPAM. We already added this when we initialized IPAM for
+ * the datapath. This will just result in an erroneous message
+ * about a duplicate IP address.
+ */
+if (ip != op->peer->od->ipam_info.start_ipv4) {
+ipam_insert_ip(op->peer->od, ip);
+}
  }
  
  destroy_lport_addresses(&lrp_networks);




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

[ovs-dev] [PATCH v3] userspace: Enable non-bridge port as tunnel endpoint.

[Yifeng Sun]

For userspace datapath, currently only the bridge itself, the LOCAL port,
can be the tunnel endpoint to encap/decap tunnel packets.  This patch
enables non-bridge port as tunnel endpoint.  One use case is for users to
create a bridge and a vtep port as tap, and configure underlay IP at vtep
port as the tunnel endpoint.

This patch causes failure for test "ptap - L3 over patch port". This is
because this test is already using non-bridge port gre1 as tunnel endpoint.
In this test, an extra flow is added to support this, as shown below:
  ovs-ofctl add-flow br1 in_port=p1,actions=output=gre1

It later generates a datapath flow which matches an extra eth field:
  - recirc_id(0),...,eth_type(0x0800),...
  + recirc_id(0),...,eth(dst=1e:2c:e9:2a:66:9e),eth_type(0x0800),...

With this patch, the above flow is no longer needed.

Signed-off-by: William Tu 
Co-authored-by: William Tu 
Signed-off-by: Yifeng Sun 
---
v1->v2: Fixed an error pointed out by Ben.
v2->v3: Fixed a test failure, thanks Ben for review and testing!
 ofproto/ofproto-dpif-xlate.c | 56 +++-
 tests/packet-type-aware.at   |  1 -
 tests/tunnel-push-pop.at | 55 +++
 3 files changed, 100 insertions(+), 12 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 02a2a4535542..290924634f36 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3410,6 +3410,19 @@ tnl_route_lookup_flow(const struct xlate_ctx *ctx,
 }
 }
 }
+
+/* If tunnel IP isn't configured on bridges, then we search all ports. */
+HMAP_FOR_EACH (xbridge, hmap_node, &ctx->xcfg->xbridges) {
+struct xport *port;
+
+HMAP_FOR_EACH (port, ofp_node, &xbridge->xports) {
+if (!strncmp(netdev_get_name(port->netdev),
+ out_dev, IFNAMSIZ)) {
+*out_port = port;
+return 0;
+}
+}
+}
 return -ENOENT;
 }
 
@@ -3972,6 +3985,16 @@ is_nd_dst_correct(const struct flow *flow, const struct 
in6_addr *ipv6_addr)
 IN6_ARE_ADDR_EQUAL(&flow->ipv6_dst, ipv6_addr);
 }
 
+static bool
+is_neighbor_reply_matched(const struct flow *flow, struct in6_addr *ip_addr)
+{
+return ((IN6_IS_ADDR_V4MAPPED(ip_addr) &&
+ flow->dl_type == htons(ETH_TYPE_ARP) &&
+ in6_addr_get_mapped_ipv4(ip_addr) == flow->nw_dst) ||
+(!IN6_IS_ADDR_V4MAPPED(ip_addr) &&
+  is_nd_dst_correct(flow, ip_addr)));
+}
+
 /* Function verifies if the ARP reply or Neighbor Advertisement represented by
  * 'flow' addresses the 'xbridge' of 'ctx'. Returns true if the ARP TA or
  * neighbor discovery destination is in the list of configured IP addresses of
@@ -3986,11 +4009,7 @@ is_neighbor_reply_correct(const struct xlate_ctx *ctx, 
const struct flow *flow)
 /* Verify if 'nw_dst' of ARP or 'ipv6_dst' of ICMPV6 is in the list. */
 for (i = 0; xbridge_addr && i < xbridge_addr->n_addr; i++) {
 struct in6_addr *ip_addr = &xbridge_addr->addr[i];
-if ((IN6_IS_ADDR_V4MAPPED(ip_addr) &&
- flow->dl_type == htons(ETH_TYPE_ARP) &&
- in6_addr_get_mapped_ipv4(ip_addr) == flow->nw_dst) ||
-(!IN6_IS_ADDR_V4MAPPED(ip_addr) &&
-  is_nd_dst_correct(flow, ip_addr))) {
+if (is_neighbor_reply_matched(flow, ip_addr)) {
 /* Found a match. */
 ret = true;
 break;
@@ -3998,20 +4017,35 @@ is_neighbor_reply_correct(const struct xlate_ctx *ctx, 
const struct flow *flow)
 }
 
 xbridge_addr_unref(xbridge_addr);
+
+/* If not found in bridge's IPs, search in its ports. */
+if (!ret) {
+struct in6_addr *ip_addr, *mask;
+struct xport *port;
+int error, n_in6;
+
+HMAP_FOR_EACH (port, ofp_node, &ctx->xbridge->xports) {
+error = netdev_get_addr_list(port->netdev, &ip_addr,
+ &mask, &n_in6);
+if (!error && is_neighbor_reply_matched(flow, ip_addr)) {
+/* Found a match. */
+ret = true;
+break;
+}
+}
+}
 return ret;
 }
 
 static bool
-terminate_native_tunnel(struct xlate_ctx *ctx, ofp_port_t ofp_port,
-struct flow *flow, struct flow_wildcards *wc,
-odp_port_t *tnl_port)
+terminate_native_tunnel(struct xlate_ctx *ctx, struct flow *flow,
+struct flow_wildcards *wc, odp_port_t *tnl_port)
 {
 *tnl_port = ODPP_NONE;
 
 /* XXX: Write better Filter for tunnel port. We can use in_port
  * in tunnel-port flow to avoid these checks completely. */
-if (ofp_port == OFPP_LOCAL &&
-ovs_native_tunneling_is_on(ctx->xbridge->ofproto)) {
+if (ovs_native_tunneling_is_on(ctx->xbridge->ofproto)) {
 *tnl_port = tnl_port_map_lookup(flow, wc);
 
 /* If no tunnel port

Re: [ovs-dev] [PATCH] rhel: Add case for RHEL 7.5 major version to kmod manage script

[Gregory Rose]

On 8/28/2019 10:16 AM, Gregory Rose wrote:



On 8/28/2019 10:07 AM, Guru Shetty wrote:
I applied this to master and 2.12. I got a conflict for 2.11. If you 
need this for 2.11, can you please rebase?


Yes, it needs to go back to 2.10 where we introduce the %posttrans 
scriptlet in rhel/openvswitch-kmod-fedora.spec.in


I sent a patch for branch 2.11.  After looking more closely there is no 
need for the patch in 2.10.


Thanks,

- Greg

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

[ovs-dev] [PATCH branch-2.11] rhel: Add case for RHEL 7.5 major version to kmod manage script

[Greg Rose]

A Centos 7.5 kernel with an unencountered set of minor build numbers
caused an upgrade bug.  Adding the case for the rhel 7.5 kmod management
script fixes the problem.

Signed-off-by: Greg Rose 
Reviewed-by: Yifeng Sun 
Signed-off-by: Gurucharan Shetty 
---
 rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh | 5 +
 1 file changed, 5 insertions(+)

diff --git a/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh 
b/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
index b5c4615..245f349 100644
--- a/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
+++ b/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
@@ -72,6 +72,11 @@ if [ "$mainline_major" = "3" ] && [ "$mainline_minor" = "10" 
]; then
 comp_ver=11
 ver_offset=4
 installed_ver="$minor_rev"
+elif [ "$major_rev" = "862" ]; then
+#echo "rhel75"
+comp_ver=11
+ver_offset=4
+installed_ver="$minor_rev"
 fi
 elif [ "$mainline_major" = "4" ] && [ "$mainline_minor" = "4" ]; then
 if [ "$mainline_patch" -ge "73" ]; then
-- 
1.8.3.1

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

Re: [ovs-dev] [PATCH] Docs: Add DPDK AF_XDP PMD use case.

[William Tu]

On Tue, Aug 27, 2019 at 10:38 AM Stokes, Ian  wrote:
>
>
>
> On 8/22/2019 3:43 PM, William Tu wrote:
> > On Thu, Aug 22, 2019 at 1:33 AM David Marchand
> >  wrote:
> >>
> >> On Thu, Aug 22, 2019 at 10:30 AM Stokes, Ian  wrote:
> >>>
> >>>
> >>> On 8/22/2019 7:00 AM, David Marchand wrote:
>  On Thu, Aug 22, 2019 at 12:49 AM William Tu  wrote:
> > Add command lines for using DPDK's AF_XDP PMD driver.
> >
> > Signed-off-by: William Tu 
> > ---
> >Documentation/intro/install/afxdp.rst | 36 
> > +++
> >1 file changed, 36 insertions(+)
> >
> > diff --git a/Documentation/intro/install/afxdp.rst 
> > b/Documentation/intro/install/afxdp.rst
> > index 820e9d993d8f..c3e03b40f3b5 100644
> > --- a/Documentation/intro/install/afxdp.rst
> > +++ b/Documentation/intro/install/afxdp.rst
> > @@ -426,6 +426,42 @@ In the namespace, run drop or bounce back the 
> > packet::
> >  ip netns exec at_ns0 ./xdp_rxq_info --dev p0 --action XDP_TX
> >
> >
> > +Using DPDK's AF_XDP PMD driver
> > +--
> > +Another way to use AF_XDP is to enable DPDK and use DPDK's AF_XDP
> > +driver. First, enable the AF_XDP PMD config at DPDK's
> > +config/common_base file by::
> > +
> > +  CONFIG_RTE_LIBRTE_PMD_AF_XDP=y
> > +
> > +and recompile the DPDK source. Then, compile OVS with DPDK::
>  The AF_XDP pmd went in dpdk 19.05.
> 
>  OVS master uses dpdk 18.11.2.
>  - 
>  https://github.com/openvswitch/ovs/commit/446a2a7ede1f377687e8c0d848d63eb5e9988cfa
>  is required to build against 19.05,
>  - 
>  https://github.com/openvswitch/ovs/commit/75e5e39e5468cb9f15114f7d8b367d39bc26756c
>  is required to build against 19.08,
> 
>  If you want to have a note in this doc in the master branch, you must
>  properly explain this, or users will be disappointed and complain on
>  the ml :-)
> >>>
> >>>
> >>> +1, also I think we shouldn't reference commits in the docs that
> >>> reference the dpdk-latest branch (I was burned by this before in commit
> >>> messages myself), the reason is I intend to rebase dpdk-latest on master
> >>> after the 2.12 release and as such the commit  IDs will change due to
> >>> the rebase.
> >>>
> >>> I'm not so sure we should even add documentation to master regarding the
> >>> AF_XDP PMD until master moves to 19.11? Possibly the 'dpdk-latest' might
> >>> be a better candidate for this doc change?
> >>
> >> Yes, this was my thought too.
> >>
> >
> > Hi David, Ian, and Eelco,
> >
> > Thanks for your comments.
> > Do you suggest me to resubmit the patch to dpdk-latest now?
> > Or do you mean wait until master moves to dpdk 19.11, and then
> > submit to master?
>
> For me I'd be happy to see it re-submitted when master supports 19.11.
> dpdk-latest to date has been more about compilation enabling against
> latest DPDK target (with basic existing/supported feature validation
> carried out obv). But we haven't really used it for documentation
> changes like this so far. Instead we've made the doc changes in master
> once the new feature is enabled and available on master.
>
Hi Ian,
Thank you, I will wait for master support 19.11
William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] rhel: Add case for RHEL 7.5 major version to kmod manage script

[Gregory Rose]

On 8/28/2019 10:07 AM, Guru Shetty wrote:
I applied this to master and 2.12. I got a conflict for 2.11. If you 
need this for 2.11, can you please rebase?


Yes, it needs to go back to 2.10 where we introduce the %posttrans 
scriptlet in rhel/openvswitch-kmod-fedora.spec.in


I'll send rebased patches for both of those branches.

Thanks!

- Greg



On Tue, 27 Aug 2019 at 14:12, Yifeng Sun > wrote:


Reviewed-by: Yifeng Sun mailto:pkusunyif...@gmail.com>>

On Tue, Aug 27, 2019 at 2:06 PM Greg Rose mailto:gvrose8...@gmail.com>> wrote:
>
> A Centos 7.5 kernel with an unencountered set of minor build numbers
> caused an upgrade bug.  Adding the case for the rhel 7.5 kmod
management
> script fixes the problem.
>
> Signed-off-by: Greg Rose mailto:gvrose8...@gmail.com>>
> ---
>  rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git
a/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
b/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
> index 2cd8e5c..51756ec 100644
> --- a/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
> +++ b/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
> @@ -85,6 +85,11 @@ if [ "$mainline_major" = "3" ] && [
"$mainline_minor" = "10" ]; then
>          comp_ver=11
>          ver_offset=4
>          installed_ver="$minor_rev"
> +    elif [ "$major_rev" = "862" ]; then
> +#        echo "rhel75"
> +        comp_ver=11
> +        ver_offset=4
> +        installed_ver="$minor_rev"
>      elif [ "$major_rev" = "957" ]; then
>  #        echo "rhel76"
>          comp_ver=10
> --
> 1.8.3.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



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

Re: [ovs-dev] [PATCH] rhel: Add case for RHEL 7.5 major version to kmod manage script

[Guru Shetty]

I applied this to master and 2.12. I got a conflict for 2.11. If you need
this for 2.11, can you please rebase?

On Tue, 27 Aug 2019 at 14:12, Yifeng Sun  wrote:

> Reviewed-by: Yifeng Sun 
>
> On Tue, Aug 27, 2019 at 2:06 PM Greg Rose  wrote:
> >
> > A Centos 7.5 kernel with an unencountered set of minor build numbers
> > caused an upgrade bug.  Adding the case for the rhel 7.5 kmod management
> > script fixes the problem.
> >
> > Signed-off-by: Greg Rose 
> > ---
> >  rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh | 5 +
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
> b/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
> > index 2cd8e5c..51756ec 100644
> > --- a/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
> > +++ b/rhel/usr_share_openvswitch_scripts_ovs-kmod-manage.sh
> > @@ -85,6 +85,11 @@ if [ "$mainline_major" = "3" ] && [ "$mainline_minor"
> = "10" ]; then
> >  comp_ver=11
> >  ver_offset=4
> >  installed_ver="$minor_rev"
> > +elif [ "$major_rev" = "862" ]; then
> > +#echo "rhel75"
> > +comp_ver=11
> > +ver_offset=4
> > +installed_ver="$minor_rev"
> >  elif [ "$major_rev" = "957" ]; then
> >  #echo "rhel76"
> >  comp_ver=10
> > --
> > 1.8.3.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
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 ovn 3/4] Fix "make rpm-fedora"

[0-day Robot]

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

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


checkpatch:
WARNING: Line is 108 characters long (recommended limit is 79)
#284 FILE: rhel/usr_lib_systemd_system_ovn-controller-vtep.service:22:
#   Environment="OVN_DB=unix:/usr/local/var/run/ovn/db.sock" 
"VTEP_DB=unix:/usr/local/var/run/ovn/vtep.sock"

WARNING: Line is 133 characters long (recommended limit is 79)
#342 FILE: rhel/usr_lib_systemd_system_ovn-northd.service:10:
#   
Environment="OVN_NORTHD_OPTS=--db-nb-sock=/usr/local/var/run/ovn/ovnnb_db.sock 
--db-sb-sock=/usr/local/var/run/ovn/ovnsb_db.sock"

WARNING: Line is 121 characters long (recommended limit is 79)
#347 FILE: rhel/usr_lib_systemd_system_ovn-northd.service:14:
#   OVN_NORTHD_OPTS="--db-nb-sock=/usr/local/var/run/ovn/ovnnb_db.sock 
--db-sb-sock=/usr/local/var/run/ovn/ovnsb_db.sock"

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


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

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

Re: [ovs-dev] [PATCH v4 ovn 2/4] Add support for using OVN specific rundirs

[0-day Robot]

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

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


checkpatch:
WARNING: Line is 99 characters long (recommended limit is 79)
#2123 FILE: utilities/ovn-ctl:340:
OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_NORTHD_PRIORITY" 
"$OVN_NORTHD_WRAPPER" "$@"

WARNING: Line is 103 characters long (recommended limit is 79)
#2132 FILE: utilities/ovn-ctl:362:
OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_CONTROLLER_PRIORITY" 
"$OVN_CONTROLLER_WRAPPER" "$@"

WARNING: Line is 103 characters long (recommended limit is 79)
#2141 FILE: utilities/ovn-ctl:389:
OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_CONTROLLER_PRIORITY" 
"$OVN_CONTROLLER_WRAPPER" "$@"

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


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

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

Re: [ovs-dev] [PATCH v4 ovn 1/4] Build OVN using external OVS directory

[0-day Robot]

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

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


checkpatch:
WARNING: Line is 81 characters long (recommended limit is 79)
#143 FILE: .travis/osx-build.sh:31:
export DISTCHECK_CONFIGURE_FLAGS="$EXTRA_OPTS 
--with-ovs-source=$PWD/ovs_src"

WARNING: Line is 97 characters long (recommended limit is 79)
#159 FILE: Documentation/intro/install/general.rst:46:
Please see the Open vSwitch documentation 
(https://docs.openvswitch.org/en/latest/intro/install/)

WARNING: Line is 85 characters long (recommended limit is 79)
#204 FILE: Documentation/intro/install/general.rst:292:
$./configure --with-ovs-source=/home/foo/ovs/ 
--with-ovs-build=/home/foo/ovs/_gcc

WARNING: Line is 96 characters long (recommended limit is 79)
#1401 FILE: tutorial/ovs-sandbox:342:

PATH=$ovsbuilddir/ovsdb:$ovsbuilddir/vswitchd:$ovsbuilddir/utilities:$ovsbuilddir/vtep:$PATH

WARNING: Line is 98 characters long (recommended limit is 79)
#1402 FILE: tutorial/ovs-sandbox:343:

PATH=$builddir/controller:$builddir/controller-vtep:$builddir/northd:$builddir/utilities:$PATH

WARNING: Line is 87 characters long (recommended limit is 79)
#1591 FILE: tutorial/ovs-sandbox:476:
run ovsdb-tool create-cluster ${db}1.db "$schema" 
unix:${db}1.raft;

WARNING: Line is 102 characters long (recommended limit is 79)
#1593 FILE: tutorial/ovs-sandbox:478:
run ovsdb-tool join-cluster $db$i.db $schema_name 
unix:$db$i.raft unix:${db}1.raft

WARNING: Line is 106 characters long (recommended limit is 79)
#1653 FILE: tutorial/ovs-sandbox:525:
ovn-nbctl set-ssl $sandbox/ovnnb-privkey.pem  $sandbox/ovnnb-cert.pem 
$sandbox/pki/switchca/cacert.pem

WARNING: Line is 106 characters long (recommended limit is 79)
#1655 FILE: tutorial/ovs-sandbox:527:
ovn-sbctl set-ssl $sandbox/ovnsb-privkey.pem  $sandbox/ovnsb-cert.pem 
$sandbox/pki/switchca/cacert.pem

WARNING: Line is 120 characters long (recommended limit is 79)
#1682 FILE: tutorial/ovs-sandbox:534:
OVN_CTRLR_PKI="-p $sandbox/chassis-1-privkey.pem -c 
$sandbox/chassis-1-cert.pem -C $sandbox/pki/switchca/cacert.pem"

Lines checked: 1724, Warnings: 10, Errors: 0


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

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

Re: [ovs-dev] [PATCH v4] ovsdb-tool: Convert clustered db to standalone db.

[Ben Pfaff]

On Mon, Aug 26, 2019 at 04:04:21PM -0700, amgin...@gmail.com wrote:
> From: Aliasgar Ginwala 
> 
> Add support in ovsdb-tool for migrating clustered dbs to standalone dbs.
> E.g. usage to migrate nb/sb db to standalone db from raft:
> ovsdb-tool cluster-to-standalone ovnnb_db.db ovnnb_db_cluster.db
> 
> Signed-off-by: Aliasgar Ginwala 

Please also update the ovsdb-tool manpage.

Thanks,

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

[ovs-dev] [PATCH v4 ovn 4/4] rhel: Run ovn services with the 'openvswitch' user

[nusiddiq]

From: Numan Siddique 

This patch could have created a new user 'ovn' for ovn services instead
of using 'openvswitch' user. But this would require some amount of work and
proper testing since the new user 'ovn' should be part of 'openvswitch'
group (to access /var/run/openvswitch/db.sock.). If ovs is compiled with dpdk,
then it may get tricky (as ovs-vswitchd is run as user - openvswitch:hugetlbfs).
We can support a new user for 'ovn' services in the future.

Recently the commit [1] in ovs repo added support to run ovn services with the
'openvswitch' user, but this commit was not applied to ovn repo as we had
already created a new OVN repo. During the OVS/OVN formal split, we missed
out on applying the patch [1]. This patch takes some code from [1].

[1] - 94e1e8be3187 ("rhel: run ovn with the same user as ovs").

Signed-off-by: Numan Siddique 
---
 rhel/automake.mk|  3 ++-
 rhel/ovn-fedora.spec.in | 13 +
 ...r_lib_systemd_system_ovn-controller-vtep.service |  2 ++
 rhel/usr_lib_systemd_system_ovn-controller.service  |  2 ++
 rhel/usr_lib_systemd_system_ovn-northd.service  |  5 -
 ...usr_share_ovn_scripts_systemd_sysconfig.template | 13 +
 utilities/ovn-ctl   | 12 
 7 files changed, 48 insertions(+), 2 deletions(-)
 create mode 100644 rhel/usr_share_ovn_scripts_systemd_sysconfig.template

diff --git a/rhel/automake.mk b/rhel/automake.mk
index 39e216b01..a46e6579b 100644
--- a/rhel/automake.mk
+++ b/rhel/automake.mk
@@ -15,7 +15,8 @@ EXTRA_DIST += \
rhel/usr_lib_systemd_system_ovn-controller-vtep.service \
rhel/usr_lib_systemd_system_ovn-northd.service \
rhel/usr_lib_firewalld_services_ovn-central-firewall-service.xml \
-   rhel/usr_lib_firewalld_services_ovn-host-firewall-service.xml
+   rhel/usr_lib_firewalld_services_ovn-host-firewall-service.xml \
+   rhel/usr_share_ovn_scripts_systemd_sysconfig.template
 
 update_rhel_spec = \
   $(AM_V_GEN)($(ro_shell) && sed -e 's,[@]VERSION[@],$(VERSION),g') \
diff --git a/rhel/ovn-fedora.spec.in b/rhel/ovn-fedora.spec.in
index cbca87511..14035de9a 100644
--- a/rhel/ovn-fedora.spec.in
+++ b/rhel/ovn-fedora.spec.in
@@ -186,6 +186,10 @@ make %{?_smp_mflags}
 rm -rf $RPM_BUILD_ROOT
 make install DESTDIR=$RPM_BUILD_ROOT
 
+install -p -D -m 0644 \
+rhel/usr_share_ovn_scripts_systemd_sysconfig.template \
+$RPM_BUILD_ROOT/%{_sysconfdir}/sysconfig/ovn
+
 for service in ovn-controller ovn-controller-vtep ovn-northd; do
 install -p -D -m 0644 \
 rhel/usr_lib_systemd_system_${service}.service \
@@ -319,6 +323,14 @@ fi
 fi
 %endif
 
+%post
+%if %{with libcapng}
+if [ $1 -eq 1 ]; then
+sed -i 's:^#OVN_USER_ID=:OVN_USER_ID=:' %{_sysconfdir}/sysconfig/ovn
+sed -i 's:\(.*su\).*:\1 ovn ovn:' %{_sysconfdir}/logrotate.d/ovn
+fi
+%endif
+
 %post central
 %if 0%{?systemd_post:1}
 %systemd_post ovn-northd.service
@@ -413,6 +425,7 @@ if [ $1 -eq 1 ]; then
 fi
 
 %files
+%config(noreplace) %{_sysconfdir}/sysconfig/ovn
 %{_bindir}/ovn-nbctl
 %{_bindir}/ovn-sbctl
 %{_bindir}/ovn-trace
diff --git a/rhel/usr_lib_systemd_system_ovn-controller-vtep.service 
b/rhel/usr_lib_systemd_system_ovn-controller-vtep.service
index 832849488..09ad0612c 100644
--- a/rhel/usr_lib_systemd_system_ovn-controller-vtep.service
+++ b/rhel/usr_lib_systemd_system_ovn-controller-vtep.service
@@ -38,10 +38,12 @@ Restart=on-failure
 Environment=OVS_RUNDIR=%t/openvswitch
 Environment=OVN_RUNDIR=%t/ovn
 Environment=OVN_DB=unix:%t/ovn/ovnsb_db.sock
+EnvironmentFile=-/etc/sysconfig/ovn
 Environment=VTEP_DB=unix:%t/openvswitch/db.sock
 EnvironmentFile=-/etc/sysconfig/ovn-controller-vtep
 ExecStart=/usr/bin/ovn-controller-vtep -vconsole:emer -vsyslog:err -vfile:info 
\
   --log-file=/var/log/ovn/ovn-controller-vtep.log \
+  --ovn-user=${OVN_USER_ID} \
   --no-chdir --pidfile=${OVN_RUNDIR}/ovn-controller-vtep.pid \
   --ovnsb-db=${OVN_DB} --vtep-db=${VTEP_DB}
 
diff --git a/rhel/usr_lib_systemd_system_ovn-controller.service 
b/rhel/usr_lib_systemd_system_ovn-controller.service
index 6c8f33a27..15d0ac853 100644
--- a/rhel/usr_lib_systemd_system_ovn-controller.service
+++ b/rhel/usr_lib_systemd_system_ovn-controller.service
@@ -24,8 +24,10 @@ Type=forking
 PIDFile=/var/run/ovn/ovn-controller.pid
 Restart=on-failure
 Environment=OVN_RUNDIR=%t/ovn OVS_RUNDIR=%t/openvswitch
+EnvironmentFile=-/etc/sysconfig/ovn
 EnvironmentFile=-/etc/sysconfig/ovn-controller
 ExecStart=/usr/share/ovn/scripts/ovn-ctl --no-monitor \
+   --ovn-user=${OVN_USER_ID} \
   start_controller $OVN_CONTROLLER_OPTS
 ExecStop=/usr/share/ovn/scripts/ovn-ctl stop_controller
 
diff --git a/rhel/usr_lib_systemd_system_ovn-northd.service 
b/rhel/usr_lib_systemd_system_ovn-northd.service
index 82c23cee4..d281f861c 100644
--- a/rhel/usr_lib_systemd_system_ovn-northd.service
+++ b/rhel/us

[ovs-dev] [PATCH v4 ovn 3/4] Fix "make rpm-fedora"

[nusiddiq]

From: Numan Siddique 

"make rpm-fedora" is broken and this patch fixes it. Previous patch
in this series supported building OVN from external OVS sources.

Before running "make rpm-fedora", it is expected that the developer has run
"make dist" in the OVS source folder to generate the 
openvswitch-%{version}.tar.gz.
This tar file is copied to rpmbuild/SOURCES. The rpm spec file extracts this tar
file (using %autosetup in prep step) and compiles it before compiling OVN.

Signed-off-by: Numan Siddique 
---
 Documentation/intro/install/fedora.rst| 13 +++-
 Documentation/intro/install/general.rst   |  2 +
 rhel/automake.mk  |  2 +
 rhel/etc_logrotate.d_ovn  | 22 ++
 rhel/ovn-fedora.spec.in   | 78 +--
 ...systemd_system_ovn-controller-vtep.service | 13 ++--
 ..._lib_systemd_system_ovn-controller.service |  7 +-
 .../usr_lib_systemd_system_ovn-northd.service | 12 ++-
 utilities/ovn-ctl |  3 +-
 9 files changed, 107 insertions(+), 45 deletions(-)
 create mode 100644 rhel/etc_logrotate.d_ovn

diff --git a/Documentation/intro/install/fedora.rst 
b/Documentation/intro/install/fedora.rst
index c8ea6ec01..6e5f11a02 100644
--- a/Documentation/intro/install/fedora.rst
+++ b/Documentation/intro/install/fedora.rst
@@ -96,8 +96,15 @@ Building
 OVN RPMs
 ~~~
 
-To build OVN RPMs, execute the following from the directory
-in which `./configure` was executed:
+To build OVN RPMs, first generate openvswitch source tarball in
+your openvwitch source directory by running
+
+::
+
+$make dist
+
+And then execute the following in the OVN source directory
+(in which `./configure` was executed):
 
 ::
 
@@ -108,7 +115,7 @@ This will create the RPMs `ovn`, `ovn-central`, `ovn-host`, 
`ovn-vtep`,
 ``ovn-host-debuginfo`` and ```ovn-vtep-debuginfo```.
 
 
-You can also have the above commands automatically run the Open vSwitch unit
+You can also have the above commands automatically run the OVN unit
 tests.  This can take several minutes.
 
 ::
diff --git a/Documentation/intro/install/general.rst 
b/Documentation/intro/install/general.rst
index 9afd7f799..4936540fb 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -291,6 +291,8 @@ Example usage::
 $./boot.sh
 $./configure --with-ovs-source=/home/foo/ovs/ 
--with-ovs-build=/home/foo/ovs/_gcc
 
+It is expected to configure both Open vSwitch and OVN with the same prefix.
+
 .. _general-building:
 
 Building
diff --git a/rhel/automake.mk b/rhel/automake.mk
index be7c275a7..39e216b01 100644
--- a/rhel/automake.mk
+++ b/rhel/automake.mk
@@ -8,6 +8,7 @@
 EXTRA_DIST += \
rhel/README.RHEL.rst \
rhel/automake.mk \
+   rhel/etc_logrotate.d_ovn \
rhel/ovn-fedora.spec \
rhel/ovn-fedora.spec.in \
rhel/usr_lib_systemd_system_ovn-controller.service \
@@ -27,6 +28,7 @@ RPMBUILD_OPT ?= --without check
 rpm-fedora: dist $(srcdir)/rhel/ovn-fedora.spec
${MKDIR_P} ${RPMBUILD_TOP}/SOURCES
cp ${DIST_ARCHIVES} ${RPMBUILD_TOP}/SOURCES
+   cp $(ovs_builddir)/openvswitch-$(VERSION).tar.gz ${RPMBUILD_TOP}/SOURCES
rpmbuild ${RPMBUILD_OPT} \
  -D "_topdir ${RPMBUILD_TOP}" \
  -ba $(srcdir)/rhel/ovn-fedora.spec
diff --git a/rhel/etc_logrotate.d_ovn b/rhel/etc_logrotate.d_ovn
new file mode 100644
index 0..a351ec303
--- /dev/null
+++ b/rhel/etc_logrotate.d_ovn
@@ -0,0 +1,22 @@
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# Copying and distribution of this file, with or without modification,
+# are permitted in any medium without royalty provided the copyright
+# notice and this notice are preserved.  This file is offered as-is,
+# without warranty of any kind.
+
+/var/log/ovn/*.log {
+su root root
+daily
+compress
+sharedscripts
+missingok
+postrotate
+# Tell OVN daemons to reopen their log files
+if [ -d /var/run/ovn ]; then
+for ctl in /var/run/ovn/*.ctl; do
+ovs-appctl -t "$ctl" vlog/reopen 2>/dev/null || :
+done
+fi
+endscript
+}
diff --git a/rhel/ovn-fedora.spec.in b/rhel/ovn-fedora.spec.in
index 2234e949f..cbca87511 100644
--- a/rhel/ovn-fedora.spec.in
+++ b/rhel/ovn-fedora.spec.in
@@ -1,6 +1,6 @@
 # Spec file for Open Virtual Network (OVN).
 
-# Copyright (C) 2018 Red Hat, Inc.
+# Copyright (C) 2018,2019 Red Hat, Inc.
 #
 # Copying and distribution of this file, with or without modification,
 # are permitted in any medium without royalty provided the copyright
@@ -48,11 +48,15 @@ Version: @VERSION@
 Obsoletes: openvswitch-ovn-common < %{?epoch:%{epoch}:}%{version}-%{release}
 Provides: openvswitch-ovn-common = %{?epoch:%{epoch}:}%{version}-%{release}
 
+%define ovsver %{version}
+%define ovsdir openvswitch-%{ovsver}
+
 # Nearly all of openvswitch is ASL 2.0.  The bugtool is LGPLv2+, and the
 # lib/sflow*.[ch] files are 

[ovs-dev] [PATCH v4 ovn 2/4] Add support for using OVN specific rundirs

[nusiddiq]

From: Numan Siddique 

Until now, OVN uses the openvswitch rundirs (rundir, logdir, etcdir).
The commit [1] changed the package name from openvswitch to ovn, but
it didn't take into the account the effects of it. When "make install"
is run ovn-ctl utility is copied to /usr/local/share/ovn/scripts folder.
ovn-ctl depends on 'ovs-lib' and it is not present in this scripts foler.
Because of which we cannot start OVN services using ovn-ctl.

This patch addresses all these issues. It changes the rundir to
ovn specific ones. (i.e /usr/local/var/run/ovn, /usr/local/var/log/ovn,
/usr/local/etc/ovn with default configuration).

[1] - 7795e0e28dce("Change the package name from openvswitch to ovn in 
AC_INIT()")

Tested:by: Dumitru Ceara 
Signed-off-by: Numan Siddique 
---
 Documentation/intro/install/general.rst |  30 ++--
 Makefile.am |   6 +-
 TODO_SPLIT.rst  |   2 +
 configure.ac|  34 ++--
 controller/ovn-controller.c |   4 +-
 lib/.gitignore  |   1 +
 lib/automake.mk |  21 ++-
 lib/ovn-dirs.c.in   | 112 +
 lib/ovn-dirs.h  |  35 
 lib/ovn-util.c  |  24 ++-
 lib/ovn-util.h  |   1 +
 m4/{openvswitch.m4 => ovn.m4}   |  60 +++
 northd/ovn-northd.c |   9 +-
 tests/ovs-macros.at |   1 +
 tutorial/ovs-sandbox|   1 +
 utilities/automake.mk   |   5 +
 utilities/ovn-ctl   |  71 +
 utilities/ovn-ctl.8.xml |  12 +-
 utilities/ovn-lib.in| 204 
 19 files changed, 520 insertions(+), 113 deletions(-)
 create mode 100644 lib/ovn-dirs.c.in
 create mode 100644 lib/ovn-dirs.h
 rename m4/{openvswitch.m4 => ovn.m4} (94%)
 create mode 100644 utilities/ovn-lib.in

diff --git a/Documentation/intro/install/general.rst 
b/Documentation/intro/install/general.rst
index 01d545da2..9afd7f799 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -159,17 +159,17 @@ For example::
 If you have built Open vSwitch in a separate directory, then you
 need to provide that path in the option - --with-ovs-build.
 
-By default all files are installed under ``/usr/local``. OVN and Open vSwitch
-also expects to find its database in ``/usr/local/etc/openvswitch`` by default.
+By default all files are installed under ``/usr/local``. OVN expects to find
+its database in ``/usr/local/etc/ovn`` by default.
 If you want to install all files into, e.g., ``/usr`` and ``/var`` instead of
-``/usr/local`` and ``/usr/local/var`` and expect to use ``/etc/openvswitch`` as
+``/usr/local`` and ``/usr/local/var`` and expect to use ``/etc/ovn`` as
 the default database directory, add options as shown here::
 
 $ ./configure --prefix=/usr --localstatedir=/var --sysconfdir=/etc
 
 .. note::
 
-  Open vSwitch and OVN installed with packages like .rpm (e.g. via
+  OVN installed with packages like .rpm (e.g. via
   ``yum install`` or ``rpm -ivh``) and .deb (e.g. via
   ``apt-get install`` or ``dpkg -i``) use the above configure options.
 
@@ -338,9 +338,13 @@ and stopping ovn-northd, ovn-controller and ovsdb-servers. 
After installation,
 the daemons can be started by using the ovn-ctl utility.  This will take care
 to setup initial conditions, and start the daemons in the correct order.
 The ovn-ctl utility is located in '$(pkgdatadir)/scripts', and defaults to
-'/usr/local/share/openvswitch/scripts'.  An example after install might be::
+'/usr/local/share/ovn/scripts'.  ovn-ctl utility requires the 'ovs-lib'
+helper shell script which is present in '/usr/local/share/openvswitch/scripts'.
+So invoking ovn-ctl as "./ovn-ctl" will fail.
 
-$ export PATH=$PATH:/usr/local/share/openvswitch/scripts
+An example after install might be::
+
+$ export PATH=$PATH:/usr/local/share/ovn/scripts
 $ ovn-ctl start_northd
 $ ovn-ctl start_controller
 
@@ -350,7 +354,7 @@ Starting OVN Central services
 OVN central services includes ovn-northd, Northbound and
 Southbound ovsdb-server.
 
-$ export PATH=$PATH:/usr/local/share/openvswitch/scripts
+$ export PATH=$PATH:/usr/local/share/ovn/scripts
 $ ovn-ctl start_northd
 
 Refer to ovn-ctl(8) for more information and the supported options.
@@ -360,23 +364,23 @@ Before starting ovn-northd you need to start OVN 
Northbound and Southbound
 ovsdb-servers. Before ovsdb-servers can be started,
 configure the Northbound and Southbound databases::
 
-   $ mkdir -p /usr/local/etc/openvswitch
-   $ ovsdb-tool create /usr/local/etc/openvswitch/ovnnb_db.db \
+   $ mkdir -p /usr/local/etc/ovn
+   $ ovsdb-tool create /usr/local/etc/ovn/ovnnb_db.db \
  ovn-nb.ovsschema
-   $ ovsdb-tool create /usr/local/etc/openvswitch/ovnsb_db.db \
+   $ ov

[ovs-dev] [PATCH v4 ovn 1/4] Build OVN using external OVS directory

[nusiddiq]

From: Numan Siddique 

With this patch we have to configure OVN to refer to external OVS source/build
directory instead of the ovs subtree.

The new configuration options added are:
 * --with-ovs-source=/path/to/ovs/source/dir
 * --with-ovs-build=/path/to/ovs/build/dir

The path to these directories can also be a relative path.

Before configuring OVN, user should configure and compile OVS. If the user has
configured OVS on a different directory than the source dir, then 
'with-ovs-build'
should be specified.

If ovs-build dir is not defined, then ovs-source is used.

An upcoming patch will delete the ovs subtree.

Example usage:
  $ # Clone OVS repo
  $cd /home/foo/ovs
  $./boot.sh
  $mkdir _gcc
  $cd _gcc && ../configure && cd ..
  $make -C _gcc

  $ # Clone OVN repo
  $cd /home/foo/ovn
  $./boot.sh
  $./configure --with-ovs-source=/home/foo/ovs/ 
--with-ovs-build=/home/foo/ovs/_gcc
  $make

The test files ovn-controller-vtep.at, ovn-nbctl.at and ovn-sbctl.at needed to 
be modified
because of this commit [1] in the openvswitch repo.

This patch also updates the tutorial/ovs-sandbox to use OVS binaries from the 
OVS build
folder.

[1] - 
https://github.com/openvswitch/ovs/commit/29004db273985088cdb60097bdfd4a6bc6a966d1

Acked-by: Lucas Alvares Gomes 
Signed-off-by: Numan Siddique 
Tested-by: Lorenzo Bianconi 
---
 .travis/linux-build.sh  |  17 +-
 .travis/osx-build.sh|  13 +-
 Documentation/intro/install/general.rst |  31 ++-
 Makefile.am |  24 +-
 acinclude.m4|  43 
 configure.ac|  29 +--
 controller-vtep/automake.mk |   2 +-
 include/ovn/version.h.in|  28 +++
 lib/ovsdb_automake.mk   |   7 +-
 tests/automake.mk   |   6 +-
 tests/ofproto-macros.at |   4 +-
 tests/ovn-controller-vtep.at|  12 +-
 tests/ovn-nbctl.at  |   6 +-
 tests/ovn-sbctl.at  |  20 +-
 tests/ovn.at| 158 ++--
 tests/ovsdb-macros.at   |   2 +-
 tutorial/automake.mk|   2 +-
 tutorial/ovs-sandbox| 308 
 18 files changed, 411 insertions(+), 301 deletions(-)
 create mode 100644 include/ovn/version.h.in

diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh
index a20474345..37a6844ab 100755
--- a/.travis/linux-build.sh
+++ b/.travis/linux-build.sh
@@ -10,7 +10,18 @@ TARGET="x86_64-native-linuxapp-gcc"
 
 function configure_ovs()
 {
+git clone https://github.com/openvswitch/ovs.git ovs_src
+pushd ovs_src
 ./boot.sh && ./configure $* || { cat config.log; exit 1; }
+make -j4
+popd
+}
+
+function configure_ovn()
+{
+configure_ovs $*
+./boot.sh && ./configure --with-ovs-source=$PWD/ovs_src $* || \
+{ cat config.log; exit 1; }
 }
 
 OPTS="$EXTRA_OPTS $*"
@@ -28,16 +39,16 @@ fi
 if [ "$TESTSUITE" ]; then
 # 'distcheck' will reconfigure with required options.
 # Now we only need to prepare the Makefile without sparse-wrapped CC.
-configure_ovs
+configure_ovn
 
-export DISTCHECK_CONFIGURE_FLAGS="$OPTS"
+export DISTCHECK_CONFIGURE_FLAGS="$OPTS --with-ovs-source=$PWD/ovs_src"
 if ! make distcheck -j4 TESTSUITEFLAGS="-j4 -k ovn" RECHECK=yes; then
 # testsuite.log is necessary for debugging.
 cat */_build/tests/testsuite.log
 exit 1
 fi
 else
-configure_ovs $OPTS
+configure_ovn $OPTS
 make selinux-policy
 
 make -j4
diff --git a/.travis/osx-build.sh b/.travis/osx-build.sh
index f11d7b9af..1d6ac54af 100755
--- a/.travis/osx-build.sh
+++ b/.travis/osx-build.sh
@@ -7,10 +7,20 @@ EXTRA_OPTS=""
 
 function configure_ovs()
 {
+git clone https://github.com/openvswitch/ovs.git ovs_src
+pushd ovs_src
 ./boot.sh && ./configure $*
+make -j4
+popd
 }
 
-configure_ovs $EXTRA_OPTS $*
+function configure_ovn()
+{
+configure_ovs $*
+./boot.sh && ./configure $* --with-ovs-source=$PWD/ovs_src
+}
+
+configure_ovn $EXTRA_OPTS $*
 
 if [ "$CC" = "clang" ]; then
 make CFLAGS="$CFLAGS -Wno-error=unused-command-line-argument"
@@ -18,6 +28,7 @@ else
 make CFLAGS="$CFLAGS $BUILD_ENV"
 fi
 if [ "$TESTSUITE" ] && [ "$CC" != "clang" ]; then
+export DISTCHECK_CONFIGURE_FLAGS="$EXTRA_OPTS 
--with-ovs-source=$PWD/ovs_src"
 if ! make distcheck RECHECK=yes; then
 # testsuite.log is necessary for debugging.
 cat */_build/tests/testsuite.log
diff --git a/Documentation/intro/install/general.rst 
b/Documentation/intro/install/general.rst
index 1d5323f76..01d545da2 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -42,9 +42,9 @@ out.  This is the right branch for general development.
 
 As of now there are no official OVN releases.
 
-Although building OVN, also builds OVS, it is recommended to clone
-and build

[ovs-dev] [PATCH v4 ovn 0/4] External OVS source support and separate run dir for OVN

[nusiddiq]

From: Numan Siddique 

This patch series adds support for building OVN from external OVS
sources.

The first patch adds the support to compile OVN from external OVS sources.
The following configuration options are added when configuring OVN
  * --with-ovs-source (mandatory)
  * --with-ovs-build (optional)

Patch 2 adds support to run OVN services using separate
directores 
  - Default run time dir - /usr/local/var/run/ovm
  - Default log dir - /usr/loca/var/log/ovn
  - Default db dir - /usr/loca/etc/ovn

Patch 3 fixes "make rpm-fedora" which is presently broken

Patch 4 runs OVN services as openvswitch user for rhel when rpms are
used.

v3 -> v4
===
 * Rebased to latest master to resolve merge conflict in p2

v2 -> v3
===
 * Added the support to provide the ovs source and build dirs as
   relative paths as suggested By Ben in the irc meeting.
 * Dropped patch 5 from the series which was added in v2. 
   Patch 5 deleted the python subdirectory in the ovn repo. But that
   patch is failing in travis CI. It will be submitted separately
   once I get the chance to work on it and the fix the issue.


v1 -> v2

 * Addressed the review comments.
 * Swapped the patch 1 and 2 as it was easier to address Mark's comment
   on OVS_RUNDIR/OVN_RUNDIR
 * In patch 2, renamed m4/openvswitch.m4 to m4/ovn.m4 and renamed few of
   the macros to OVS_* to OVN_*.

 * Combined the patch 1 and 2 in this series which were submitted
   separately earlier.

Numan Siddique (4):
  Build OVN using external OVS directory
  Add support for using OVN specific rundirs
  Fix "make rpm-fedora"
  rhel: Run ovn services with the 'openvswitch' user

 .travis/linux-build.sh|  17 +-
 .travis/osx-build.sh  |  13 +-
 Documentation/intro/install/fedora.rst|  13 +-
 Documentation/intro/install/general.rst   |  63 ++--
 Makefile.am   |  30 +-
 TODO_SPLIT.rst|   2 +
 acinclude.m4  |  43 +++
 configure.ac  |  63 ++--
 controller-vtep/automake.mk   |   2 +-
 controller/ovn-controller.c   |   4 +-
 include/ovn/version.h.in  |  28 ++
 lib/.gitignore|   1 +
 lib/automake.mk   |  21 +-
 lib/ovn-dirs.c.in | 112 +++
 lib/ovn-dirs.h|  35 ++
 lib/ovn-util.c|  24 +-
 lib/ovn-util.h|   1 +
 lib/ovsdb_automake.mk |   7 +-
 m4/{openvswitch.m4 => ovn.m4} |  60 ++--
 northd/ovn-northd.c   |   9 +-
 rhel/automake.mk  |   5 +-
 rhel/etc_logrotate.d_ovn  |  22 ++
 rhel/ovn-fedora.spec.in   |  91 --
 ...systemd_system_ovn-controller-vtep.service |  15 +-
 ..._lib_systemd_system_ovn-controller.service |   9 +-
 .../usr_lib_systemd_system_ovn-northd.service |  15 +-
 ...are_ovn_scripts_systemd_sysconfig.template |  13 +
 tests/automake.mk |   6 +-
 tests/ofproto-macros.at   |   4 +-
 tests/ovn-controller-vtep.at  |  12 +-
 tests/ovn-nbctl.at|   6 +-
 tests/ovn-sbctl.at|  20 +-
 tests/ovn.at  | 158 -
 tests/ovs-macros.at   |   1 +
 tests/ovsdb-macros.at |   2 +-
 tutorial/automake.mk  |   2 +-
 tutorial/ovs-sandbox  | 309 +-
 utilities/automake.mk |   5 +
 utilities/ovn-ctl |  86 +++--
 utilities/ovn-ctl.8.xml   |  12 +-
 utilities/ovn-lib.in  | 204 
 41 files changed, 1085 insertions(+), 460 deletions(-)
 create mode 100644 include/ovn/version.h.in
 create mode 100644 lib/ovn-dirs.c.in
 create mode 100644 lib/ovn-dirs.h
 rename m4/{openvswitch.m4 => ovn.m4} (94%)
 create mode 100644 rhel/etc_logrotate.d_ovn
 create mode 100644 rhel/usr_share_ovn_scripts_systemd_sysconfig.template
 create mode 100644 utilities/ovn-lib.in

-- 
2.21.0

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

Re: [ovs-dev] [patch v1] conntrack: Fix ICMPV4 error data L4 length check.

[Darrell Ball]

On Wed, Aug 28, 2019 at 1:43 AM Vishal Deep Ajmera <
vishal.deep.ajm...@ericsson.com> wrote:

> That is interesting
>
> i just tried applying on top of tree and I see that the git applies some
> changes (2 lines)
>
> in extract_l4_icmp6() rather the intended extract_l4_icmp() as in the
> patch I sent out.
>
> My guess is that the surrounding lines are identical in the 2 functions
> and I had other
>
> patches in the same branch shifting the patch downward, hence git applied
> the changes
>
> to extract_l4_icmp6() rather than extract_l4_icmp()
>
>
>
> I'll make the changes on a clean branch and resend.
>
>
>
> Thanks. I applied this patch and looks ok to me.
>

Thanks for confirming


>
>
> JTBC, the 8 byte ICMP error data L4 length restriction is only for V4.
>
> ICMP6 does not have this restriction; see
> https://tools.ietf.org/html/rfc4443
>
>
>
> In my opinion, we should limit the check to < 8 bytes even in case of
> ICMPv6 as that is all
>
> is required from the TCP header to extract port numbers and aligns it with
> ICMPv4.
>
> Specially because RFC is not mandating minimum size for L4 header in case
> of ICMPv6.
>

For V6, the ICMP error L4 length can even be zero, legitimately, with a
large extension header
presence, theoretically; practically, these are almost certainly crafted
packets. The existing
check for ICMP6 is mostly permissive but reasonable.

The ICMP6 sanity check can be enhanced. For the most part, it can be made
more strict while
handling the above corner case perfectly.

I don't think we should mess with enhancing ICMP6 as part of this bug fix
for ICMPv4.
Thats why this patch leaves ICMP6 unmodified.


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

Re: [ovs-dev] Regarding TSO using AF_PACKET in OVS

[Ben Pfaff]

On Wed, Aug 28, 2019 at 04:40:41PM +0530, Ramana Reddy wrote:
> Hi Ben, Justin, Jesse and All,
> 
> Hope everyone doing great.

For what it's worth, I'm not the kernel module maintainer and I don't
really have an answer to this question.  I hope that someone else pops
up to answer it.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v8 3/3 ovn] OVN: Vlan backed DVR N-S, redirect packet via localnet port

[Numan Siddique]

On Wed, Aug 28, 2019 at 7:27 AM Ankur Sharma 
wrote:

> Background:
> With c0974331b7a19a87ab8f1f2cec8fbe366af92fa2, we have added
> support for E-W workflow for vlan backed DVRs.
>
> This series enables N-S workflow for vlan backed DVRs.
>
> Key difference between E-W and N-S traffic flow is that
> N-S flow requires a gateway chassis. A gateway chassis
> will be respondible for following:
> a. Doing Network Address Translation (NAT).
> b. Becoming entry and exit point for North->South
>and South->North traffic respectively.
>
> OVN by default always uses overlay encapsulation to redirect
> the packet to gateway chassis. This series will enable
> the redirection to gateway chassis in the absence of encapsulation.
>
> This patch:
> Achieves the vlan backed redirection by doing following:
>
> Sender Side:
> 
> a. For a remote port of type "chassisredirect" and if it
>has redirect type as "vlan", then do not add tunnel
>based redirection flow in table=32.
>
> b. In table=33, add a flow with priority=100, that would do following:
>i. Change the metadata to that of gateway logical switch
>   (i.e logical switch attached to gateway logical router port).
>   ii. Change REG15 to point to localnet port of gateway logical switch.
>  iii. send to packet to table=15.
>
> c. In Table=65, packet will hit the existing priority=150 flow to send
>the packet to physical bridge, while attaching vlan header and
>changing source mac to chassis mac.
>
> Receiver Side:
> --
> a. No changes needed
>
> OVERALL PACKET FLOW:
> Sender Side:
> ---
> a. logical flow in lr_in_gw_redirect stage will ensure that
>outport of the packet is chassisredirect port.
>For example:
>table=12(lr_in_gw_redirect  ), priority=50   , match=(outport ==
> "router-to-underlay"), action=(outport = "cr-router-to-underlay"; next;)
>
> b. After ingress pipeline, packet will enter the table=32, followed by
> table=33
>
> c. Table=33, will send the packet to table=65.
>
> d. Table=65, will send the packet to uplink bridge
>with destination mac of chassisredirect port and vlan
>id of peer logical switch.
>
> Receiver Side:
> -
> a. Packet is received by the pipeline of peer logical switch.
> b. Since destination mac is that of router port, hence packet will
>enter the logical router pipeline.
> c. Now, packet will go through regular logical router pipeline
>(both ingress and egress).
>
> One caveat with the approach is that ttl will be decremented twice,
> since the packets are going through logical router ingress pipeline
> twice (once on sender chassis and again on gateway chassis).
>
> No changes needed for the reverse path.
>
> Signed-off-by: Ankur Sharma 
> ---
>

Hi Ankur,

I applied this patch to master with some changes in the commit message -
corrected the commit id and
typo - table 15 to table 65 and below changes

**
diff --git a/controller/physical.c b/controller/physical.c
index 427be18f8..c818646f0 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -228,29 +228,28 @@ get_zone_ids(const struct sbrec_port_binding *binding,
 }

 static void
-put_remote_port_redirect_vlan(const struct
-  sbrec_port_binding *binding,
-  const struct hmap *local_datapaths,
-  struct local_datapath *ld,
-  struct match *match,
-  struct ofpbuf *ofpacts_p,
-  struct ovn_desired_flow_table *flow_table)
+put_remote_port_redirect_bridged(const struct
+ sbrec_port_binding *binding,
+ const struct hmap *local_datapaths,
+ struct local_datapath *ld,
+ struct match *match,
+ struct ofpbuf *ofpacts_p,
+ struct ovn_desired_flow_table *flow_table)
 {
-struct eth_addr binding_mac;
-uint32_t ls_dp_key = 0;
-
 if (strcmp(binding->type, "chassisredirect")) {
-/* VLAN based redirect is only supported for chassisredirect
+/* bridged based redirect is only supported for chassisredirect
  * type remote ports. */
 return;
 }

+struct eth_addr binding_mac;
 bool  is_valid_mac = extract_sbrec_binding_first_mac(binding,
  &binding_mac);
 if (!is_valid_mac) {
 return;
 }

+uint32_t ls_dp_key = 0;
 for (int i = 0; i < ld->n_peer_ports; i++) {
 const struct sbrec_port_binding *sport_binding =
ld->peer_ports[i];
 const char *sport_peer_name = smap_get(&sport_binding->options,
@@ -1011,8 +1010,9 @@ consider_port_binding(struct ovsdb_idl_index
*sbrec_port_binding_

Re: [ovs-dev] [PATCH v8 2/3 ovn] OVN: Vlan backed DVR N-S, avoid get_arp on non redirect chassis.

[Numan Siddique]

On Wed, Aug 28, 2019 at 7:27 AM Ankur Sharma 
wrote:

> Background:
> With c0974331b7a19a87ab8f1f2cec8fbe366af92fa2, we have added
> support for E-W workflow for vlan backed DVRs.
>
> This series enables N-S workflow for vlan backed DVRs.
>
> Key difference between E-W and N-S traffic flow is that
> N-S flow requires a gateway chassis. A gateway chassis
> will be respondible for following:
> a. Doing Network Address Translation (NAT).
> b. Becoming entry and exit point for North->South
>and South->North traffic respectively.
>
> OVN by default always uses overlay encapsulation to redirect
> the packet to gateway chassis. This series will enable
> the redirection to gateway chassis in the absence of encapsulation.
>
> This patch:
> a. Make sure that ARP request for endpoint behind the gateway
>router port is sent from gateway chassis only and not from
>host(compute) chassis.
>
> b. This is achieved by adding a new logical flow in
>lr_in_arp_resolve at priority=50.
>
> c. This flow run on non gateway chassis and sets the destination
>mac to router port mac, if outport is a gateway chassis attached
>router port and redirect-type is set as "vlan".
>Example logical flow:
>table=9 (lr_in_arp_resolve  ), priority=50   , match=(outport ==
> "router-to-underlay" && !is_chassis_resident("cr-router-to-underlay")),
> action=(eth.dst = 00:00:01:01:02:04; next;)
>
> d. This change is needed because other wise for non resolved ARPs,
>we will end up doing get_arp in host chassis. Doing so will
>have following issues:
>i. We want all the interation with North bound endpoints via
>   gateway chassis only, doing so on host chassis will violate
>   that.
>
>   ii. With get_arp, ovn-controller will generate the ARP using router
>   port's mac as source mac, which will lead us to the same issue,
>   where router port mac will be going through continous mac moves
>   in physical network. Worst, it would affect the redirection,
>   since it uses router port mac as destination mac.
>
> Signed-off-by: Ankur Sharma 
>

Hi Ankur,

I applied this patch with some commit correction in the commit message and
below changes

**
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 0daf3271a..78246506c 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -3636,16 +3636,6 @@ lsp_is_external(const struct
nbrec_logical_switch_port *nbsp)
 return !strcmp(nbsp->type, "external");
 }

-/* Returns true if lrp has either gateway chassis or ha chassis group
- * attached to it. */
-static bool
-lrp_has_gateway(const struct nbrec_logical_router_port *nbrp)
-{
-return (nbrp->n_gateway_chassis ||
-(nbrp->ha_chassis_group &&
nbrp->ha_chassis_group->n_ha_chassis))
-? true : false;
-}
-
 static bool
 build_dhcpv4_action(struct ovn_port *op, ovs_be32 offer_ip,
 struct ds *options_action, struct ds *response_action,
@@ -7754,7 +7744,7 @@ build_lrouter_flows(struct hmap *datapaths, struct
hmap *ports,
 }
 }

-if (!op->derived && lrp_has_gateway(op->nbrp)) {
+if (!op->derived && op->od->l3redirect_port) {
 const char *redirect_type = smap_get(&op->nbrp->options,
  "redirect-type");
 if (redirect_type && !strcasecmp(redirect_type,
"bridged")) {
***

Let me know if you think this isn't fine.

Thanks
Numan


> ---
>  northd/ovn-northd.8.xml | 12 
>  northd/ovn-northd.c | 32 
>  2 files changed, 44 insertions(+)
>
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index d45bb15..442e899 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -2239,6 +2239,18 @@ next;
>get_nd(outport, xxreg0); next;.
>  
>
> +
> +  
> +
> +  For logical router port with redirect-chassis and redirect-type
> +  being set as bridged, a priority-50 flow will match
> +  outport == "ROUTER_PORT" and !is_chassis_resident
> +  ("cr-ROUTER_PORT") has actions eth.dst =
> E;
> +  next;, where E is the ethernet address of the
> +  logical router port.
> +
> +  
> +
>  
>
>  Ingress Table 9: Check packet length
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 0a7f181..0daf327 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -3636,6 +3636,16 @@ lsp_is_external(const struct
> nbrec_logical_switch_port *nbsp)
>  return !strcmp(nbsp->type, "external");
>  }
>
> +/* Returns true if lrp has either gateway chassis or ha chassis group
> + * attached to it. */
> +static bool
> +lrp_has_gateway(const struct nbrec_logical_router_port *nbrp)
> +{
> +return (nbrp->n_gateway_chassis ||
> +(nbrp->ha_chassis_group &&
> nbrp->ha_chassis_group->n_ha_chassis))
> +? tru

Re: [ovs-dev] [PATCH v8 1/3 ovn] OVN: Vlan backed DVR N-S, redirect-type option

[Numan Siddique]

On Wed, Aug 28, 2019 at 7:26 AM Ankur Sharma 
wrote:

> Background:
> With c0974331b7a19a87ab8f1f2cec8fbe366af92fa2, we have added
> support for E-W workflow for vlan backed DVRs.
>
> This series enables N-S workflow for vlan backed DVRs.
>
> Key difference between E-W and N-S traffic flow is that
> N-S flow requires a gateway chassis. A gateway chassis
> will be respondible for following:
> a. Doing Network Address Translation (NAT).
> b. Becoming entry and exit point for North->South
>and South->North traffic respectively.
>
> OVN by default always uses overlay encapsulation to redirect
> the packet to gateway chassis. This series will enable
> the redirection to gateway chassis in the absence of encapsulation.
>
> This patch:
> a. Add a new key-value in options of a router port.
> b. This new config key will be used by ovn-controller
>to determine if a redirected packet will go out of
>tunnel port or localnet port.
> c. key is "redirect-type" and it takes "overlay" and
>"bridged" as values.
> d. Added ovn-nbctl command to set and get redirect-type
>option on a router port.
> e. This new configuration is added because bridged or overlay
>based forwarding is considered to be a logical switch property,
>hence for a router configuration has to be done at the router port
>level.
> f. Restored the function ovsdb_datum_to_smap, which helps in ensuring
>that we do not overwrite existing options, while adding a new
>key-value pair to it. This function exists in 2.8.5, i am not
>able to figure out so far, which release/why it was removed.
>I do not see a harm in adding it back.
>

Hi Ankur, in the commit message, I deleted (f) and corrected the commit id
(which
was incorrect) and applied this patch to master with the below changes


diff --git a/ovn-nb.xml b/ovn-nb.xml
index aab369aa6..b99a808b8 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -1959,7 +1959,7 @@
 
   This options dictates if a packet redirected to
   gateway chassis will be overlay encapsulated
-  or go as a regular vlan packet.
+  or go as a regular packet via the localnet port.
 

 
@@ -1982,8 +1982,8 @@
 

 
-  BRIDGED option will ensure that redirected packet goes out as
vlan
-  tagged via the localnet port.
+  BRIDGED option will ensure that redirected packet goes out
+  via the localnet port tagged with vlan (if configured).
 

 
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index eebd36961..42033d589 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -958,12 +958,11 @@ uuid=`ovn-sbctl --columns=_uuid --bare find
Port_Binding logical_port=cr-R1-S1`
 echo "CR-LRP UUID is: " $uuid

 ovn-nbctl lrp-set-redirect-type R1-S1 bridged
-AT_CHECK([ovn-sbctl get Port_Binding ${uuid} options:redirect-type], [0],
[bridged
+OVS_WAIT_UNTIL([ovn-sbctl get Port_Binding ${uuid} options:redirect-type],
[0], [bridged
 ])

 ovn-nbctl lrp-set-redirect-type R1-S1 overlay
-AT_CHECK([ovn-sbctl get Port_Binding ${uuid} options:redirect-type], [0],
[overlay
+OVS_WAIT_UNTIL([ovn-sbctl get Port_Binding ${uuid} options:redirect-type],
[0], [overlay
 ])

-
 AT_CLEANUP
***

Thanks
Numan


>
> Signed-off-by: Ankur Sharma 
> ---
>  northd/ovn-northd.c   |  6 +
>  ovn-nb.xml| 43 ++
>  tests/ovn-nbctl.at| 25 
>  tests/ovn-northd.at   | 31 
>  utilities/ovn-nbctl.c | 65
> +++
>  5 files changed, 170 insertions(+)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index ca128c9..0a7f181 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -2514,6 +2514,9 @@ ovn_port_update_sbrec(struct northd_context *ctx,
>  if (op->derived) {
>  const char *redirect_chassis = smap_get(&op->nbrp->options,
>  "redirect-chassis");
> +const char *redirect_type = smap_get(&op->nbrp->options,
> + "redirect-type");
> +
>  int n_gw_options_set = 0;
>  if (op->nbrp->ha_chassis_group) {
>  n_gw_options_set++;
> @@ -2605,6 +2608,9 @@ ovn_port_update_sbrec(struct northd_context *ctx,
>  sbrec_port_binding_set_gateway_chassis(op->sb, NULL, 0);
>  }
>  smap_add(&new, "distributed-port", op->nbrp->name);
> +if (redirect_type) {
> +smap_add(&new, "redirect-type", redirect_type);
> +}
>  } else {
>  if (op->peer) {
>  smap_add(&new, "peer", op->peer->key);
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 1f8d751..aab369a 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -1954,6 +1954,49 @@
>issues.
>  
>  

Re: [ovs-dev] [PATCH] tnl-neigh: Use outgoing ofproto version.

[Flavio Leitner via dev]

Hi Ben and Ilya,

This patch has two reviews, so any chance for you to take a look
soon as well?

Thanks in advance!
fbl

On Tue, Aug 13, 2019 at 01:34:04PM -0300, Flavio Leitner via dev wrote:
> When a packet needs to be encapsulated in userspace, the endpoint
> address needs to be resolved to fill in the headers. If it is not,
> then currently OvS sends either a Neighbor Solicitation (IPv6)
> or an ARP Query (IPv4) to resolve it.
> 
> The problem is that the NS/ARP packet will go through the flow
> rules in the new bridge, but inheriting the ofproto table version
> from the original packet to be encapsulated. When those versions
> don't match, the result is unexpected because no flow rules might
> be visible, which would cause the default table rule to be used
> to drop the packet. Or only part of the flow rules would be visible
> and so on.
> 
> Since the NS/ARP packet is created by OvS and will be injected in
> the outgoing bridge, use the corresponding ofproto version instead.
> 
> Signed-off-by: Flavio Leitner 
> ---
>  ofproto/ofproto-dpif-xlate.c |  4 +--
>  tests/tunnel.at  | 62 
>  2 files changed, 64 insertions(+), 2 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 28a7fdd84..5a8a46370 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3414,6 +3414,7 @@ compose_table_xlate(struct xlate_ctx *ctx, const struct 
> xport *out_dev,
>  struct dp_packet *packet)
>  {
>  struct xbridge *xbridge = out_dev->xbridge;
> +ovs_version_t version = 
> ofproto_dpif_get_tables_version(xbridge->ofproto);
>  struct ofpact_output output;
>  struct flow flow;
>  
> @@ -3423,8 +3424,7 @@ compose_table_xlate(struct xlate_ctx *ctx, const struct 
> xport *out_dev,
>  output.port = OFPP_TABLE;
>  output.max_len = 0;
>  
> -return ofproto_dpif_execute_actions__(xbridge->ofproto,
> -  ctx->xin->tables_version, &flow,
> +return ofproto_dpif_execute_actions__(xbridge->ofproto, version, &flow,
>NULL, &output.ofpact, sizeof 
> output,
>ctx->depth, ctx->resubmits, 
> packet);
>  }
> diff --git a/tests/tunnel.at b/tests/tunnel.at
> index fc6f87936..faffb4149 100644
> --- a/tests/tunnel.at
> +++ b/tests/tunnel.at
> @@ -394,6 +394,68 @@ AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>  
> +AT_SETUP([tunnel - table version])
> +dnl check if changes in the egress bridge flow table affects
> +dnl discovering the link layer address of tunnel endpoints.
> +OVS_VSWITCHD_START([add-port br0 p0 -- set Interface p0 type=dummy 
> ofport_request=1 other-config:hwaddr=aa:55:aa:55:00:00])
> +AT_CHECK([ovs-vsctl add-br int-br -- set bridge int-br datapath_type=dummy], 
> [0])
> +AT_CHECK([ovs-vsctl add-port int-br v1 -- set Interface v1 type=vxlan \
> +   options:remote_ip=172.31.1.2 options:key=123 \
> +   ofport_request=2 \
> + -- add-port int-br v2 -- set Interface v2 type=internal \
> +   ofport_request=3 \
> +   ], [0])
> +
> +AT_CHECK([ovs-appctl dpif/show], [0], [dnl
> +dummy@ovs-dummy: hit:0 missed:0
> +  br0:
> +br0 65534/100: (dummy-internal)
> +p0 1/1: (dummy)
> +  int-br:
> +int-br 65534/2: (dummy-internal)
> +v1 2/4789: (vxlan: key=123, remote_ip=172.31.1.2)
> +v2 3/3: (dummy-internal)
> +])
> +
> +dnl First setup dummy interface IP address, then add the route
> +dnl so that tnl-port table can get valid IP address for the device.
> +AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 172.31.1.1/24], [0], [OK
> +])
> +AT_CHECK([ovs-appctl ovs/route/add 172.31.1.0/24 br0], [0], [OK
> +])
> +
> +dnl change the flow table to bump the internal table version
> +AT_CHECK([ovs-ofctl add-flow int-br action=normal])
> +AT_CHECK([ovs-ofctl add-flow br0 action=normal])
> +AT_CHECK([ovs-ofctl del-flows br0])
> +AT_CHECK([ovs-ofctl add-flow br0 action=normal])
> +AT_CHECK([ovs-ofctl del-flows br0])
> +AT_CHECK([ovs-ofctl add-flow br0 action=normal])
> +AT_CHECK([ovs-ofctl del-flows br0])
> +AT_CHECK([ovs-ofctl add-flow br0 action=normal])
> +AT_CHECK([ovs-ofctl del-flows br0])
> +AT_CHECK([ovs-ofctl add-flow br0 action=normal])
> +AT_CHECK([ovs-ofctl del-flows br0])
> +AT_CHECK([ovs-ofctl add-flow br0 action=normal])
> +AT_CHECK([ovs-ofctl del-flows br0])
> +AT_CHECK([ovs-ofctl add-flow br0 action=normal])
> +AT_CHECK([ovs-ofctl del-flows br0])
> +AT_CHECK([ovs-ofctl add-flow br0 action=normal])
> +
> +dnl Check Neighbour discovery.
> +AT_CHECK([ovs-vsctl -- set Interface p0 options:pcap=p0.pcap])
> +
> +AT_CHECK([ovs-appctl netdev-dummy/receive int-br 
> 'in_port(2),eth(src=aa:55:aa:55:00:00,dst=f8:bc:12:ff:ff:ff),eth_type(0x0800),ipv4(src=1.1.3.92,dst=1.1.3.88,proto=1,tos=0,ttl=64,fr

Re: [ovs-dev] [PATCH v7] Detailed packet drop statistics per dpdk and vhostuser ports

[0-day Robot]

Bleep bloop.  Greetings Sriram Vatala via dev, I am a robot and I have tried 
out your patch.
Thanks for your contribution.

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


checkpatch:
WARNING: Line is 138 characters long (recommended limit is 79)
#351 FILE: utilities/bugtool/plugins/network-status/openvswitch.xml:37:
/usr/share/openvswitch/scripts/ovs-bugtool-get-iface-stats

Lines checked: 392, Warnings: 1, Errors: 0


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

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

[ovs-dev] [PATCH v7] Detailed packet drop statistics per dpdk and vhostuser ports

[Sriram Vatala via dev]

OVS may be unable to transmit packets for multiple reasons and
today there is a single counter to track packets dropped due to
any of those reasons. The most common reason is that a VM is
unable to read packets fast enough causing the vhostuser port
transmit queue on the OVS side to become full. This manifests
as a problem with VNFs not receiving all packets. Having a
separate drop counter to track packets dropped because the
transmit queue is full will clearly indicate that the problem
is on the VM side and not in OVS. Similarly maintaining separate
counters for all possible drops helps in indicating sensible
cause for packet drops.

This patch adds custom stats counters to track packets dropped
at port level and these counters are displayed along with
other stats in "ovs-vsctl get interface  statistics"
command. The detailed stats will be available for both dpdk and
vhostuser ports.

Signed-off-by: Sriram Vatala 
---
 lib/netdev-dpdk.c  | 99 +++---
 utilities/bugtool/automake.mk  |  3 +-
 utilities/bugtool/ovs-bugtool-get-iface-stats  | 25 ++
 .../bugtool/plugins/network-status/openvswitch.xml |  1 +
 vswitchd/vswitch.xml   | 24 ++
 5 files changed, 139 insertions(+), 13 deletions(-)
 create mode 100755 utilities/bugtool/ovs-bugtool-get-iface-stats

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index bc20d68..a679c5b 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -413,8 +413,17 @@ struct netdev_dpdk {
 
 PADDED_MEMBERS(CACHE_LINE_SIZE,
 struct netdev_stats stats;
-/* Custom stat for retries when unable to transmit. */
+/* Custom device stats */
+/* No. of retries when unable to transmit. */
 uint64_t tx_retries;
+/* Pkts dropped when unable to transmit; Probably Tx queue is full */
+uint64_t tx_failure_drops;
+/* Pkts len greater than max dev MTU */
+uint64_t tx_mtu_exceeded_drops;
+/* Pkt drops in egress policier processing */
+uint64_t tx_qos_drops;
+/* Pkts drops in ingress policier processing */
+uint64_t rx_qos_drops;
 /* Protects stats */
 rte_spinlock_t stats_lock;
 /* 4 pad bytes here. */
@@ -2171,6 +2180,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
 struct ingress_policer *policer = netdev_dpdk_get_ingress_policer(dev);
 uint16_t nb_rx = 0;
 uint16_t dropped = 0;
+uint16_t qos_drops = 0;
 int qid = rxq->queue_id * VIRTIO_QNUM + VIRTIO_TXQ;
 int vid = netdev_dpdk_get_vid(dev);
 
@@ -2202,11 +2212,13 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
 (struct rte_mbuf **) batch->packets,
 nb_rx, true);
 dropped -= nb_rx;
+qos_drops = dropped;
 }
 
 rte_spinlock_lock(&dev->stats_lock);
 netdev_dpdk_vhost_update_rx_counters(&dev->stats, batch->packets,
  nb_rx, dropped);
+dev->rx_qos_drops += qos_drops;
 rte_spinlock_unlock(&dev->stats_lock);
 
 batch->count = nb_rx;
@@ -2232,6 +2244,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct 
dp_packet_batch *batch,
 struct ingress_policer *policer = netdev_dpdk_get_ingress_policer(dev);
 int nb_rx;
 int dropped = 0;
+int qos_drops = 0;
 
 if (OVS_UNLIKELY(!(dev->flags & NETDEV_UP))) {
 return EAGAIN;
@@ -2250,12 +2263,14 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct 
dp_packet_batch *batch,
 (struct rte_mbuf **) batch->packets,
 nb_rx, true);
 dropped -= nb_rx;
+qos_drops = dropped;
 }
 
 /* Update stats to reflect dropped packets */
 if (OVS_UNLIKELY(dropped)) {
 rte_spinlock_lock(&dev->stats_lock);
 dev->stats.rx_dropped += dropped;
+dev->rx_qos_drops += qos_drops;
 rte_spinlock_unlock(&dev->stats_lock);
 }
 
@@ -2339,6 +2354,9 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
 struct rte_mbuf **cur_pkts = (struct rte_mbuf **) pkts;
 unsigned int total_pkts = cnt;
 unsigned int dropped = 0;
+unsigned int tx_failure;
+unsigned int mtu_drops;
+unsigned int qos_drops;
 int i, retries = 0;
 int max_retries = VHOST_ENQ_RETRY_MIN;
 int vid = netdev_dpdk_get_vid(dev);
@@ -2356,9 +2374,12 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
 rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
 
 cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);
+mtu_drops = total_pkts - cnt;
+qos_drops = cnt;
 /* Check has QoS has been configured for the netdev */
 cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true);
-dropped = total_pkts - cnt;
+qos_drops -= cnt;
+dropped = qos_drops + mtu_drops;
 
 do {
 int vhost_qid = qid * VIRTIO_QNUM + VIRTIO_RXQ;
@@ -2383,12 +2

Re: [ovs-dev] [PATCH v4 ovn 2/2] OVN: northd: add rate limiting support for SB controller events

[0-day Robot]

Bleep bloop.  Greetings Lorenzo Bianconi, 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.


git-am:
fatal: sha1 information is lacking or useless (tests/ovn.at).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 OVN: northd: add rate limiting support for SB controller 
events
The copy of the patch that failed is found in:
   
/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/OVN/.git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


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

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

Re: [ovs-dev] [PATCH v2 ovn 3/3] ovn-controller: Minimize SB DB port_binding lookups.

[Numan Siddique]

On Fri, Aug 23, 2019 at 4:55 PM Dumitru Ceara  wrote:

> Instead of storing only peer_ports in struct local_datapath, store both
> local-remote mappings for patch ports. Also, it's useful to directly
> store sbrec_port_binding pointers for all datapath ports as we avoid
> doing costly sbrec_port_binding index lookups by port name.
>
> Signed-off-by: Dumitru Ceara 
>

This patch needs a rebase.

Thanks
Numan


> ---
>  controller/binding.c|   19 ---
>  controller/ovn-controller.h |   11 -
>  controller/physical.c   |4 ++-
>  controller/pinctrl.c|   53
> +++
>  4 files changed, 41 insertions(+), 46 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 47d4fea..242163d 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -160,13 +160,24 @@ add_local_datapath__(struct ovsdb_idl_index
> *sbrec_datapath_binding_by_key,
>   peer->datapath, false,
>   depth + 1, local_datapaths);
>  ld->n_peer_ports++;
> -ld->peer_ports = xrealloc(ld->peer_ports,
> -  ld->n_peer_ports *
> -  sizeof *ld->peer_ports);
> -ld->peer_ports[ld->n_peer_ports - 1] = peer;
> +if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
> +ld->peer_ports =
> +x2nrealloc(ld->peer_ports,
> +   &ld->n_allocated_peer_ports,
> +   sizeof *ld->peer_ports);
> +}
> +ld->peer_ports[ld->n_peer_ports - 1].local = pb;
> +ld->peer_ports[ld->n_peer_ports - 1].remote = peer;
>  }
>  }
>  }
> +
> +ld->n_ports++;
> +if (ld->n_ports > ld->n_allocated_ports) {
> +ld->ports = x2nrealloc(ld->ports, &ld->n_allocated_ports,
> +   sizeof *ld->ports);
> +}
> +ld->ports[ld->n_ports - 1] = pb;
>  }
>  sbrec_port_binding_index_destroy_row(target);
>  }
> diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h
> index 41feec3..86b300e 100644
> --- a/controller/ovn-controller.h
> +++ b/controller/ovn-controller.h
> @@ -60,8 +60,17 @@ struct local_datapath {
>   * hypervisor. */
>  bool has_local_l3gateway;
>
> -const struct sbrec_port_binding **peer_ports;
> +const struct sbrec_port_binding **ports;
> +size_t n_ports;
> +size_t n_allocated_ports;
> +
> +struct {
> +const struct sbrec_port_binding *local;
> +const struct sbrec_port_binding *remote;
> +} *peer_ports;
> +
>  size_t n_peer_ports;
> +size_t n_allocated_peer_ports;
>  };
>
>  struct local_datapath *get_local_datapath(const struct hmap *,
> diff --git a/controller/physical.c b/controller/physical.c
> index a05962b..49bc1a4 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -266,11 +266,13 @@ put_replace_router_port_mac_flows(const struct
>  }
>
>  for (int i = 0; i < ld->n_peer_ports; i++) {
> -const struct sbrec_port_binding *rport_binding =
> ld->peer_ports[i];
> +const struct sbrec_port_binding *rport_binding;
>  struct eth_addr router_port_mac;
>  struct match match;
>  struct ofpact_mac *replace_mac;
>
> +rport_binding = ld->peer_ports[i].remote;
> +
>  /* Table 65, priority 150.
>   * ===
>   *
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 21ed75f..a8ff0bb 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -174,8 +174,7 @@ static struct pinctrl pinctrl;
>  static void init_buffered_packets_map(void);
>  static void destroy_buffered_packets_map(void);
>  static void
> -run_buffered_binding(struct ovsdb_idl_index
> *sbrec_port_binding_by_datapath,
> - struct ovsdb_idl_index
> *sbrec_mac_binding_by_lport_ip,
> +run_buffered_binding(struct ovsdb_idl_index
> *sbrec_mac_binding_by_lport_ip,
>   const struct hmap *local_datapaths)
>  OVS_REQUIRES(pinctrl_mutex);
>
> @@ -239,10 +238,7 @@ static void wait_controller_event(struct
> ovsdb_idl_txn *ovnsb_idl_txn);
>  static void init_ipv6_ras(void);
>  static void destroy_ipv6_ras(void);
>  static void ipv6_ra_wait(long long int send_ipv6_ra_time);
> -static void prepare_ipv6_ras(
> -struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
> -struct ovsdb_idl_index *sbrec_port_binding_by_name,
> -const struct hmap *local_datapaths)
> +static void prepare_ipv6_ras(const struct hmap *local_datapaths)
>  OVS_REQUIRES(pinctrl_mutex);
>  static void send_ipv6_ras(struct rconn *swconn,
>long long int

Re: [ovs-dev] [PATCH v2 ovn 2/3] ovn-controller: Optimize update of ct-zones external-ids.

[Numan Siddique]

On Fri, Aug 23, 2019 at 4:54 PM Dumitru Ceara  wrote:

> commit_ct_zones() is called at every ovn-controller iteration but updates
> to
> ct-zones don't happen at every iteration. Avoid cloning the
> br-int->external_ids map unless an update is needed.
>
> Signed-off-by: Dumitru Ceara 
> ---
>  controller/ovn-controller.c |   53
> +--
>  1 file changed, 45 insertions(+), 8 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 86f29ac..1825c1f 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -531,10 +531,9 @@ static void
>  commit_ct_zones(const struct ovsrec_bridge *br_int,
>  struct shash *pending_ct_zones)
>  {
> -struct smap new_ids;
> -smap_clone(&new_ids, &br_int->external_ids);
> +struct smap ct_add_ids = SMAP_INITIALIZER(&ct_add_ids);
> +struct sset ct_del_ids = SSET_INITIALIZER(&ct_del_ids);
>
> -bool updated = false;
>  struct shash_node *iter;
>  SHASH_FOR_EACH(iter, pending_ct_zones) {
>  struct ct_zone_pending_entry *ctzpe = iter->data;
> @@ -550,22 +549,60 @@ commit_ct_zones(const struct ovsrec_bridge *br_int,
>  char *user_str = xasprintf("ct-zone-%s", iter->name);
>  if (ctzpe->add) {
>  char *zone_str = xasprintf("%"PRId32, ctzpe->zone);
> -smap_replace(&new_ids, user_str, zone_str);
> +struct smap_node *node =
> +smap_get_node(&br_int->external_ids, user_str);
> +if (!node || strcmp(node->value, zone_str)) {
> +smap_add_nocopy(&ct_add_ids, user_str, zone_str);
> +user_str = NULL;
> +zone_str = NULL;
> +}
>  free(zone_str);
>  } else {
> -smap_remove(&new_ids, user_str);
> +if (smap_get(&br_int->external_ids, user_str)) {
> +sset_add(&ct_del_ids, user_str);
> +}
>  }
>  free(user_str);
>
>  ctzpe->state = CT_ZONE_DB_SENT;
> -updated = true;
>  }
>
> -if (updated) {
> +/* Update the bridge external IDs only if really needed (i.e., we must
> + * add a value or delete one). Rebuilding the external IDs map at
> + * every run is a costly operation when having lots of ct_zones.
> + */
> +if (!smap_is_empty(&ct_add_ids) || !sset_is_empty(&ct_del_ids)) {
> +struct smap new_ids = SMAP_INITIALIZER(&new_ids);
> +
> +struct smap_node *id;
> +SMAP_FOR_EACH (id, &br_int->external_ids) {
> +if (sset_find_and_delete(&ct_del_ids, id->key)) {
> +continue;
> +}
> +
> +if (smap_get(&ct_add_ids, id->key)) {
>

What if the value of this key is changed ?

Suppose we have "a = 1" in external_ids  and the ct_add_ids has "a = 2".
Looks like in this case, the external_ids column in the db will not be
updated
with "a = 2".

I am not sure though if this can actually happen.

Thanks
Numan

+continue;
> +}
> +
> +smap_add(&new_ids, id->key, id->value);
> +}
> +
> +struct smap_node *next_id;
> +SMAP_FOR_EACH_SAFE (id, next_id, &ct_add_ids) {
> +smap_replace(&new_ids, id->key, id->value);
> +smap_remove_node(&ct_add_ids, id);
> +}
> +
>  ovsrec_bridge_verify_external_ids(br_int);
>  ovsrec_bridge_set_external_ids(br_int, &new_ids);
> +smap_destroy(&new_ids);
>  }
> -smap_destroy(&new_ids);
> +
> +ovs_assert(smap_is_empty(&ct_add_ids));
> +ovs_assert(sset_is_empty(&ct_del_ids));
> +
> +smap_destroy(&ct_add_ids);
> +sset_destroy(&ct_del_ids);
>  }
>
>  static void
>
> ___
> 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 v2 ovn 1/3] ofctrl: Avoid recomputing match hash in ofctrl_dup_flow().

[Numan Siddique]

On Fri, Aug 23, 2019 at 4:54 PM Dumitru Ceara  wrote:

> Signed-off-by: Dumitru Ceara 
>

Thanks. I applied this patch to master.

Numan


> ---
>  controller/ofctrl.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 8928205..3131baf 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -743,7 +743,7 @@ ofctrl_dup_flow(struct ovn_flow *src)
>  dst->ofpacts = xmemdup(src->ofpacts, src->ofpacts_len);
>  dst->ofpacts_len = src->ofpacts_len;
>  dst->sb_uuid = src->sb_uuid;
> -dst->match_hmap_node.hash = ovn_flow_match_hash(dst);
> +dst->match_hmap_node.hash = src->match_hmap_node.hash;
>  dst->uuid_hindex_node.hash = uuid_hash(&src->sb_uuid);
>  return dst;
>  }
>
> ___
> 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

[ovs-dev] [PATCH v4 ovn 2/2] OVN: northd: add rate limiting support for SB controller events

[Lorenzo Bianconi]

Introduce the capability to associate a meter to each controller event
type in order to not overload the pinctrl thread under heavy load.
Each event type relies on a meter with a defined name:
- empty_lb_backends: event-elb

Acked-by: Mark Michelson 
Signed-off-by: Lorenzo Bianconi 
---
 northd/ovn-northd.8.xml |  7 +++--
 northd/ovn-northd.c | 68 +
 ovn-nb.xml  |  8 +
 tests/ovn.at|  2 +-
 4 files changed, 63 insertions(+), 22 deletions(-)

diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index f58338880..d9e8380b1 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -304,7 +304,8 @@
   If controller_event has been enabled and load balancing rules with
   empty backends have been added in OVN_Northbound, a 130 flow
   is added to trigger ovn-controller events whenever the chassis receives a
-  packet for that particular VIP
+  packet for that particular VIP. If event-elb meter has been
+  previously created, it will be associated to the empty_lb logical flow
 
 
 Ingress Table 5: Pre-stateful
@@ -1766,7 +1767,9 @@ icmp6 {
 balancing rules for a Gateway router or Router with gateway port
 in OVN_Northbound database that does not have configured
 backends, a priority-130 flow is added to trigger ovn-controller events
-whenever the chassis receives a packet for that particular VIP
+whenever the chassis receives a packet for that particular VIP.
+If event-elb meter has been previously created, it will be
+associated to the empty_lb logical flow
   
 
   
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index d6ac8159e..265984339 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -4005,14 +4005,18 @@ static void
 build_empty_lb_event_flow(struct ovn_datapath *od, struct hmap *lflows,
   struct smap_node *node, char *ip_address,
   struct nbrec_load_balancer *lb, uint16_t port,
-  int addr_family, int pl)
+  int addr_family, int pl, struct shash *meter_groups)
 {
 if (!controller_event_en || node->value[0]) {
 return;
 }
 
 struct ds match = DS_EMPTY_INITIALIZER;
-char *action;
+char *meter = "", *action;
+
+if (meter_groups && shash_find(meter_groups, "event-elb")) {
+meter = "event-elb";
+}
 
 if (addr_family == AF_INET) {
 ds_put_format(&match, "ip4.dst == %s && %s",
@@ -4026,18 +4030,20 @@ build_empty_lb_event_flow(struct ovn_datapath *od, 
struct hmap *lflows,
   port);
 }
 action = xasprintf("trigger_event(event = \"%s\", "
-   "vip = \"%s\", protocol = \"%s\", "
-   "load_balancer = \"" UUID_FMT "\");",
-   event_to_string(OVN_EVENT_EMPTY_LB_BACKENDS),
-   node->key, lb->protocol,
-   UUID_ARGS(&lb->header_.uuid));
+   "meter = \"%s\", vip = \"%s\", "
+   "protocol = \"%s\", "
+   "load_balancer = \"" UUID_FMT "\");",
+   event_to_string(OVN_EVENT_EMPTY_LB_BACKENDS),
+   meter, node->key, lb->protocol,
+   UUID_ARGS(&lb->header_.uuid));
 ovn_lflow_add(lflows, od, pl, 130, ds_cstr(&match), action);
 ds_destroy(&match);
 free(action);
 }
 
 static void
-build_pre_lb(struct ovn_datapath *od, struct hmap *lflows)
+build_pre_lb(struct ovn_datapath *od, struct hmap *lflows,
+ struct shash *meter_groups)
 {
 /* Do not send ND packets to conntrack */
 ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_LB, 110,
@@ -4074,7 +4080,8 @@ build_pre_lb(struct ovn_datapath *od, struct hmap *lflows)
 }
 
 build_empty_lb_event_flow(od, lflows, node, ip_address, lb,
-  port, addr_family, S_SWITCH_IN_PRE_LB);
+  port, addr_family, S_SWITCH_IN_PRE_LB,
+  meter_groups);
 
 free(ip_address);
 
@@ -4894,7 +4901,8 @@ build_lrouter_groups(struct hmap *ports, struct ovs_list 
*lr_list)
 static void
 build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
 struct hmap *port_groups, struct hmap *lflows,
-struct hmap *mcgroups, struct hmap *igmp_groups)
+struct hmap *mcgroups, struct hmap *igmp_groups,
+struct shash *meter_groups)
 {
 /* This flow table structure is documented in ovn-northd(8), so please
  * update ovn-northd.8.xml if you change anything. */
@@ -4911,7 +4919,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap 
*ports,
 }
 
 build_pre_acls(od, lflows);
-build_pre_lb(od, lflows);
+build_pre_lb(od, lflows, meter_groups);
 build_pr

[ovs-dev] [PATCH v4 ovn 1/2] OVN: add meter support to trigger_event action

[Lorenzo Bianconi]

Introduce meter support to trigger_event action in order to not
overload pinctrl thread under heavy load

Signed-off-by: Lorenzo Bianconi 
---
 include/ovn/actions.h |  1 +
 lib/actions.c | 43 +--
 ovn-sb.xml|  5 -
 tests/ovn.at  |  4 
 4 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 0ca06537c..145f27f25 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -327,6 +327,7 @@ struct ovnact_controller_event {
 int event_type;   /* controller event type */
 struct ovnact_gen_option *options;
 size_t n_options;
+char *meter;
 };
 
 /* OVNACT_BIND_VPORT. */
diff --git a/lib/actions.c b/lib/actions.c
index 08c589ab3..6a5907e1b 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -1273,6 +1273,9 @@ format_TRIGGER_EVENT(const struct ovnact_controller_event 
*event,
 {
 ds_put_format(s, "trigger_event(event = \"%s\"",
   event_to_string(event->event_type));
+if (event->meter) {
+ds_put_format(s, ", meter = \"%s\"", event->meter);
+}
 for (const struct ovnact_gen_option *o = event->options;
  o < &event->options[event->n_options]; o++) {
 ds_put_cstr(s, ", ");
@@ -1420,10 +1423,21 @@ encode_TRIGGER_EVENT(const struct 
ovnact_controller_event *event,
  const struct ovnact_encode_params *ep OVS_UNUSED,
  struct ofpbuf *ofpacts)
 {
+uint32_t meter_id = NX_CTLR_NO_METER;
 size_t oc_offset;
 
+if (event->meter) {
+meter_id = ovn_extend_table_assign_id(ep->meter_table, event->meter,
+  ep->lflow_uuid);
+if (meter_id == EXT_TABLE_ID_INVALID) {
+VLOG_WARN("Unable to assign id for trigger meter: %s",
+  event->meter);
+return;
+}
+}
+
 oc_offset = encode_start_controller_op(ACTION_OPCODE_EVENT, false,
-   NX_CTLR_NO_METER, ofpacts);
+   meter_id, ofpacts);
 ovs_be32 ofs = htonl(event->event_type);
 ofpbuf_put(ofpacts, &ofs, sizeof ofs);
 
@@ -1738,11 +1752,27 @@ parse_trigger_event(struct action_context *ctx,
  sizeof *event->options);
 }
 
-struct ovnact_gen_option *o = &event->options[event->n_options++];
-memset(o, 0, sizeof *o);
-parse_gen_opt(ctx, o,
-  &ctx->pp->controller_event_opts->event_opts[event_type],
-  event_to_string(event_type));
+if (lexer_match_id(ctx->lexer, "meter")) {
+if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
+return;
+}
+/* If multiple meters are given, use the most recent. */
+if (ctx->lexer->token.type == LEX_T_STRING &&
+strlen(ctx->lexer->token.s)) {
+free(event->meter);
+event->meter = xstrdup(ctx->lexer->token.s);
+} else if (ctx->lexer->token.type != LEX_T_STRING) {
+lexer_syntax_error(ctx->lexer, "expecting string");
+return;
+}
+lexer_get(ctx->lexer);
+} else {
+struct ovnact_gen_option *o = &event->options[event->n_options++];
+memset(o, 0, sizeof *o);
+parse_gen_opt(ctx, o,
+&ctx->pp->controller_event_opts->event_opts[event_type],
+event_to_string(event_type));
+}
 if (ctx->lexer->error) {
 return;
 }
@@ -1763,6 +1793,7 @@ static void
 ovnact_controller_event_free(struct ovnact_controller_event *event)
 {
 free_gen_options(event->options, event->n_options);
+free(event->meter);
 }
 
 static void
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 02691bbd3..477e7bc7a 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -1996,7 +1996,9 @@ tcp.flags = RST;
   
 This action is used to allow ovs-vswitchd to report CMS related
 events writing them in  table.
-Supported event:
+It is possible to associate a meter to a each event in order to
+not overload pinctrl thread under heavy load; each meter is
+identified though a defined naming convention. Supported events:
   
 
   
@@ -2007,6 +2009,7 @@ tcp.flags = RST;
 no configured backend destinations. For this event, the event
 info includes the load balancer VIP, the load balancer UUID,
 and the transport protocol.
+Associated meter: event-elb
   
 
   
diff --git a/tests/ovn.at b/tests/ovn.at
index 86078c400..014566d50 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1353,6 +1353,10 @@ tcp_reset { };
 trigger_event(event = "empty_lb_backends", vip = "10.0.0.1:80", protoco

[ovs-dev] [PATCH v4 ovn 0/2] Add rate limiting support for controller_events

[Lorenzo Bianconi]

Introduce the capability to associate a meter to each controller event
type in order to not overload the pinctrl thread under heavy load.
For each event type we need to define a naming convention for the related
meter:
- empty_lb_backends: event-elb

Changes since v3:
- rebase on top of 'add empty_lb controller_event for logical router pipeline'
  https://patchwork.ozlabs.org/cover/1153918/
- update documentation
- added a unit test for the meter option in the "action parsing"
Changes since v2:
- fold patch 'Repair memory leak for OVN controller events'
- fix memory leak of meter string
Changes since v1:
- fix subject

Lorenzo Bianconi (2):
  OVN: add meter support to trigger_event action
  OVN: northd: add rate limiting support for SB controller events

 include/ovn/actions.h   |  1 +
 lib/actions.c   | 43 ++
 northd/ovn-northd.8.xml |  7 +++--
 northd/ovn-northd.c | 68 +
 ovn-nb.xml  |  8 +
 ovn-sb.xml  |  5 ++-
 tests/ovn.at|  6 +++-
 7 files changed, 109 insertions(+), 29 deletions(-)

-- 
2.21.0

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

Re: [ovs-dev] [PATCH v1] ofproto: Fix OVS crash when packets hit Openflow rules with certain combinations of nested actions

[Ilya Maximets]

On 28.08.2019 13:24, Anil Kumar wrote:
> OVS crashes when a packet sent with action set to OFPP_TABLE hits Openflow
> rules with conntrack and learn actions.
> 
> For example:
> The crash can be triggered by installing the following Open flow rules and
> sending packet with action set to OFPP_TABLE
> 
> 1. ovs-ofctl -OOpenflow13 add-flow br-int "table=0, priority=50, \
>ct_state=-trk,ip, in_port=10 actions=ct(table=0)"
> 
> 2. ovs-ofctl -OOpenflow13 add-flow br-int "table=0, priority=50, \
>ct_state=+trk,ip, in_port=10 actions=ct(commit),resubmit(,1)"
> 
> 3. ovs-ofctl -OOpenflow13 add-flow br-int "table=1 \
>
> actions=learn(table=2,NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_OF_IN_PORT[]->NXM_NX_REG0[0..15],
>  \
>output:NXM_NX_REG0[0..15]),resubmit(,2)"
> 
> 4. Send a packet with output as OFPP_TABLE
>ovs-ofctl -OOpenflow13 packet-out br-int 'in_port=10 \
>
> packet=505400071011080045284006f97cc0a80001c0a800020008000a50022e7d,
>  \
> actions=TABLE'
> 
> The processing code path results in the same thread context attempting to
> acquire a mutex that it already holds. Since the mutex is of error checking
> type this situation is considered fatal and OVS aborts. The crash isn’t
> limited to only the above combination of actions
> 
> Signed-off-by: Anil Kumar 
> ---

Hi. Thanks for the patch.
But I cannot reproduce the issue on current master using commands you provided.
Could you, please, provide also a stack trace of the asserted thread?
This should make it easier to understand the issue.

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

Re: [ovs-dev] [PATCH v6] Detailed packet drop statistics per dpdk and vhostuser ports

[Sriram Vatala via dev]

Hi All,

The patch " netdev-dpdk: Refactor vhost custom stats for extensibility" by
Ilya is merged to master. So I will rebase and send updated patch to avoid
any git conflicts.

Thanks & Regards,
Sriram.

-Original Message-
From: Sriram Vatala  
Sent: 26 August 2019 18:29
To: ovs-dev@openvswitch.org; i.maxim...@samsung.com
Cc: b...@ovn.org; ian.sto...@intel.com; Sriram Vatala

Subject: [PATCH v6] Detailed packet drop statistics per dpdk and vhostuser
ports

OVS may be unable to transmit packets for multiple reasons and today there
is a single counter to track packets dropped due to any of those reasons.
The most common reason is that a VM is unable to read packets fast enough
causing the vhostuser port transmit queue on the OVS side to become full.
This manifests as a problem with VNFs not receiving all packets. Having a
separate drop counter to track packets dropped because the transmit queue is
full will clearly indicate that the problem is on the VM side and not in
OVS. Similarly maintaining separate counters for all possible drops helps in
indicating sensible cause for packet drops.

This patch adds custom stats counters to track packets dropped at port level
and these counters are displayed along with other stats in "ovs-vsctl get
interface  statistics"
command. The detailed stats will be available for both dpdk and vhostuser
ports.

Signed-off-by: Sriram Vatala 
---
 lib/netdev-dpdk.c  | 120
++---
 utilities/bugtool/automake.mk  |   3 +-
 utilities/bugtool/ovs-bugtool-get-iface-stats  |  25 +
 .../bugtool/plugins/network-status/openvswitch.xml |   1 +
 vswitchd/vswitch.xml   |  24 +
 5 files changed, 157 insertions(+), 16 deletions(-)  create mode 100755
utilities/bugtool/ovs-bugtool-get-iface-stats

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 4805783..6685f32
100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -447,8 +447,14 @@ struct netdev_dpdk {
 
 PADDED_MEMBERS(CACHE_LINE_SIZE,
 struct netdev_stats stats;
-/* Custom stat for retries when unable to transmit. */
+/* Counters for Custom device stats */
+/* No. of retries when unable to transmit. */
 uint64_t tx_retries;
+/* Pkts left untransmitted in Tx buffers. Probably Tx Que is full
*/
+uint64_t tx_failure_drops;
+uint64_t tx_mtu_exceeded_drops;
+uint64_t tx_qos_drops;
+uint64_t rx_qos_drops;
 /* Protects stats */
 rte_spinlock_t stats_lock;
 /* 4 pad bytes here. */
@@ -2205,6 +2211,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
 struct ingress_policer *policer = netdev_dpdk_get_ingress_policer(dev);
 uint16_t nb_rx = 0;
 uint16_t dropped = 0;
+uint16_t qos_drops = 0;
 int qid = rxq->queue_id * VIRTIO_QNUM + VIRTIO_TXQ;
 int vid = netdev_dpdk_get_vid(dev);
 
@@ -2236,11 +2243,13 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
 (struct rte_mbuf **) batch->packets,
 nb_rx, true);
 dropped -= nb_rx;
+qos_drops = dropped;
 }
 
 rte_spinlock_lock(&dev->stats_lock);
 netdev_dpdk_vhost_update_rx_counters(&dev->stats, batch->packets,
  nb_rx, dropped);
+dev->rx_qos_drops += qos_drops;
 rte_spinlock_unlock(&dev->stats_lock);
 
 batch->count = nb_rx;
@@ -2266,6 +2275,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct
dp_packet_batch *batch,
 struct ingress_policer *policer = netdev_dpdk_get_ingress_policer(dev);
 int nb_rx;
 int dropped = 0;
+int qos_drops = 0;
 
 if (OVS_UNLIKELY(!(dev->flags & NETDEV_UP))) {
 return EAGAIN;
@@ -2284,12 +2294,14 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct
dp_packet_batch *batch,
 (struct rte_mbuf **) batch->packets,
 nb_rx, true);
 dropped -= nb_rx;
+qos_drops = dropped;
 }
 
 /* Update stats to reflect dropped packets */
 if (OVS_UNLIKELY(dropped)) {
 rte_spinlock_lock(&dev->stats_lock);
 dev->stats.rx_dropped += dropped;
+dev->rx_qos_drops += qos_drops;
 rte_spinlock_unlock(&dev->stats_lock);
 }
 
@@ -2373,6 +2385,9 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int
qid,
 struct rte_mbuf **cur_pkts = (struct rte_mbuf **) pkts;
 unsigned int total_pkts = cnt;
 unsigned int dropped = 0;
+unsigned int tx_failure;
+unsigned int mtu_drops;
+unsigned int qos_drops;
 int i, retries = 0;
 int max_retries = VHOST_ENQ_RETRY_MIN;
 int vid = netdev_dpdk_get_vid(dev); @@ -2390,9 +2405,12 @@
__netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
 rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
 
 cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);
+mtu_drops = total_pkts - 

[ovs-dev] Regarding TSO using AF_PACKET in OVS

[Ramana Reddy]

Hi Ben, Justin, Jesse and All,

Hope everyone doing great.

During my work, I create a socket using AF_PACKET with virtio_net_hdr and
filled all the fields in the virtio_net_hdr
and the flag to VIRTIO_NET_GSO_TCPV4 to enable TSO using af_packet.

vnet->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;

The code is not working when I am trying to send large packets over OVS
VXLAN tunnel. It's dropping the large packets and
the application is resending the MTU sized packets. The code is working
fine in non ovs case (directly sending without ovs).

I understood that UDP tunneling offloading (VXLAN) not happening because of
nic may not support this offloading feature,
but when I send iperf (AF_INET) traffic, though the offloading is not
happening, but the large packets are fragmented and the
VXLAN tunneling sending the fragmented packets.  Why are we seeing this
different behavior?

What is the expected behavior in AF_PACKET in OVS? Does OVS support
AF_PACKET offloading mechanism?
If not, how we can avoid the retransmission of the packets. What are things
to be done so that the kernel fragments
large packets and send them out without dropping ( in case if offloading
feature not available)?

Looking forward to your reply.

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

[ovs-dev] [PATCH v1] ofproto: Fix OVS crash when packets hit Openflow rules with certain combinations of nested actions

[Anil Kumar via dev]

OVS crashes when a packet sent with action set to OFPP_TABLE hits Openflow
rules with conntrack and learn actions.

For example:
The crash can be triggered by installing the following Open flow rules and
sending packet with action set to OFPP_TABLE

1. ovs-ofctl -OOpenflow13 add-flow br-int "table=0, priority=50, \
   ct_state=-trk,ip, in_port=10 actions=ct(table=0)"

2. ovs-ofctl -OOpenflow13 add-flow br-int "table=0, priority=50, \
   ct_state=+trk,ip, in_port=10 actions=ct(commit),resubmit(,1)"

3. ovs-ofctl -OOpenflow13 add-flow br-int "table=1 \
   
actions=learn(table=2,NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_OF_IN_PORT[]->NXM_NX_REG0[0..15],
 \
   output:NXM_NX_REG0[0..15]),resubmit(,2)"

4. Send a packet with output as OFPP_TABLE
   ovs-ofctl -OOpenflow13 packet-out br-int 'in_port=10 \
   
packet=505400071011080045284006f97cc0a80001c0a800020008000a50022e7d,
 \
actions=TABLE'

The processing code path results in the same thread context attempting to
acquire a mutex that it already holds. Since the mutex is of error checking
type this situation is considered fatal and OVS aborts. The crash isn’t
limited to only the above combination of actions

Signed-off-by: Anil Kumar 
---
 ofproto/ofproto.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 12758a3..ff7d90b 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -302,7 +302,7 @@ static size_t n_ofproto_classes;
 static size_t allocated_ofproto_classes;
 
 /* Global lock that protects all flow table operations. */
-struct ovs_mutex ofproto_mutex = OVS_MUTEX_INITIALIZER;
+struct ovs_mutex ofproto_mutex;
 
 unsigned ofproto_flow_limit = OFPROTO_FLOW_LIMIT_DEFAULT;
 unsigned ofproto_max_idle = OFPROTO_MAX_IDLE_DEFAULT;
@@ -337,6 +337,8 @@ ofproto_init(const struct shash *iface_hints)
 struct shash_node *node;
 size_t i;
 
+ovs_mutex_init_recursive(&ofproto_mutex);
+
 ofproto_class_register(&ofproto_dpif_class);
 
 /* Make a local copy, since we don't own 'iface_hints' elements. */
-- 
2.7.4

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

Re: [ovs-dev] [patch v1] conntrack: Fix ICMPV4 error data L4 length check.

[Vishal Deep Ajmera]

That is interesting
i just tried applying on top of tree and I see that the git applies some 
changes (2 lines)
in extract_l4_icmp6() rather the intended extract_l4_icmp() as in the patch I 
sent out.
My guess is that the surrounding lines are identical in the 2 functions and I 
had other
patches in the same branch shifting the patch downward, hence git applied the 
changes
to extract_l4_icmp6() rather than extract_l4_icmp()

I'll make the changes on a clean branch and resend.

Thanks. I applied this patch and looks ok to me.

JTBC, the 8 byte ICMP error data L4 length restriction is only for V4.
ICMP6 does not have this restriction; see https://tools.ietf.org/html/rfc4443

In my opinion, we should limit the check to < 8 bytes even in case of ICMPv6 as 
that is all
is required from the TCP header to extract port numbers and aligns it with 
ICMPv4.
Specially because RFC is not mandating minimum size for L4 header in case of 
ICMPv6.

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

[ovs-dev] [PATCH V3] packets: Fix using outdated RSS hash after MPLS decapsulation.

[Nitin Katiyar]

When a packet is received, the RSS hash is calculated if it is not
already available. The Exact Match Cache (EMC) entry is then looked up
using this RSS hash.

When a MPLS encapsulated packet is received, the MPLS header is popped
and the packet is recirculated. Since the RSS hash has not been
invalidated here, the EMC lookup for all decapsulated packets will hit
the same entry even though these packets will have different tuple
values. This degrades performance severely as different inner packets
from the same MPLS tunnel would hit the same EMC entry.

This patch invalidates RSS hash (by resetting offload flags) after MPLS
header is popped.

Signed-off-by: Nitin Katiyar 
---
 lib/packets.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/lib/packets.c b/lib/packets.c
index ab0b1a3..12053df 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -411,6 +411,10 @@ pop_mpls(struct dp_packet *packet, ovs_be16 ethtype)
 /* Shift the l2 header forward. */
 memmove((char*)dp_packet_data(packet) + MPLS_HLEN, 
dp_packet_data(packet), len);
 dp_packet_resize_l2_5(packet, -MPLS_HLEN);
+
+/* Invalidate offload flags as they are not valid after
+ * decapsulation of MPLS header. */
+dp_packet_reset_offload(packet);
 }
 }
 
-- 
1.9.1

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

Re: [ovs-dev] [PATCH] packets: Fix using outdated RSS hash after MPLS decapsulation.

[Ilya Maximets]

On 28.08.2019 11:27, Nitin Katiyar wrote:
> 
> 
>> -Original Message-
>> From: Ilya Maximets [mailto:i.maxim...@samsung.com]
>> Sent: Wednesday, August 28, 2019 12:44 PM
>> To: Nitin Katiyar ; ovs-dev@openvswitch.org
>> Cc: Stokes, Ian 
>> Subject: Re: [ovs-dev] [PATCH] packets: Fix using outdated RSS hash after
>> MPLS decapsulation.
>>
>> On 28.08.2019 10:03, Nitin Katiyar wrote:
>>>
>>>
 -Original Message-
 From: Ilya Maximets [mailto:i.maxim...@samsung.com]
 Sent: Tuesday, August 27, 2019 5:37 PM
 To: Nitin Katiyar ;
 ovs-dev@openvswitch.org
 Cc: Stokes, Ian 
 Subject: Re: [ovs-dev] [PATCH] packets: Fix using outdated RSS hash
 after MPLS decapsulation.

 Hi.

 Since this is not a first version of a patch, the subject prefix
 should be [PATCH v2]. For the next version, please, use [PATCH v3].
 This way it's much easier to recognize/track patches in a list or mailbox.
>>> Hi,
>>> Since the title got of the patch got changed so I didn't make it V2. I 
>>> wasn't
>> sure to use V2 as with new title it becomes new thread. Anyway, I will be
>> sending the new patch with V3.
>>>

 Hint: It's generally speeds up review process if relevant people are
   in CC list while sending the patch. Not everyone subscribed for
   all the messages in a list.
>>> Thanks for suggestion. I will follow it in future.
>>>

 The code looks good in general.
 Few comments inline.

 Best regards, Ilya Maximets.

 On 16.08.2019 21:54, Nitin Katiyar wrote:
> When a packet is received, the RSS hash is calculated if it is not
> already available. The Exact Match Cache (EMC) entry is then looked
> up using this RSS hash.
>
> When a MPLS encapsulated packet is received, the MPLS header is
> popped and the packet is recirculated. Since the RSS hash has not
> been invalidated here, the EMC lookup will hit the same entry as
> that before recirculation. This degrades performance severely.

 Above sentences are not correct. It's stated that the collision
 happens with the previous pass of the same packet, but it happens
 with different packets from the same MPLS tunnel. This should be fixed.
>>> It will actually collide with the entry of packet before de-capsulation. I 
>>> have
>> rephrased it as follows:
>>>
>>> When a MPLS encapsulated packet is received, the MPLS header is
>> popped
>>> and the packet is recirculated. Since the RSS hash has not been
>>> invalidated here, the EMC lookup for decapsulated packets will hit the
>>> same entry as that before recirculation (i.e. entry of the packet with
>>> MPLS encapsulation). This degrades performance severely as different
>>> inner packets from the same MPLS tunnel would hit the same EMC entry.
>>>
>>> Let me know if it is more clear now.
>>
>> But dpif_netdev_packet_get_rss_hash() mixes the recirculation depth into
>> RSS hash on each packet pass through the datapath. So, after recirculation 
>> RSS
>> hash should be different from the hash of encapsulated packet. and where
>> should be no collisions.
>> I already wrote about this in my review to v1 and you said: "If there are
>> multiple flows are sent across same tunnel (with same MPLS label) then they
>> will all lead to same hash as external tuple and recirculation id remains 
>> same
>> for them."  That makes sense, but it's opposite to what you're saying now. 
>> I'm
>> confused.
> Ah okay, I misinterpret your previous comment. You are correct. I have 
> modified it as below:
> When a MPLS encapsulated packet is received, the MPLS header is popped
> and the packet is recirculated. Since the RSS hash has not been
> invalidated here, the EMC lookup for all decapsulated packets will hit
> the same entry even though these packets will have different tuple
> values. This degrades performance severely as different inner packets
> from the same MPLS tunnel would hit the same EMC entry.

This version looks good to me.

> 
> Thanks for correcting this.
> 
> Regards,
> Nitin
>>
>> Best regards, Ilya Maximets.
>>
>>>
>>> Thanks,
>>> Nitin

>
> This patch invalidates RSS hash (by resetting offload flags) after
> MPLS header is popped.
>
> Signed-off-by: Nitin Katiyar 
> ---
>  lib/packets.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/lib/packets.c b/lib/packets.c index ab0b1a3..db3f6d0
> 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
> @@ -404,6 +404,10 @@ pop_mpls(struct dp_packet *packet, ovs_be16
 ethtype)
>  struct mpls_hdr *mh = dp_packet_l2_5(packet);
>  size_t len = packet->l2_5_ofs;
>
> +/* Invalidate offload flags as they are not valid after
> + * decapsulation of MPLS header. */
> +dp_packet_reset_offload(packet);

 Maybe it's better to move this code down to 'dp_pack

Re: [ovs-dev] [PATCH V2] dpif-netdev: Do RCU synchronization at fixed interval in PMD main loop.

[Nitin Katiyar]

Hi,
Please provide your feedback.

Regards,
Nitin

> -Original Message-
> From: Nitin Katiyar
> Sent: Thursday, August 22, 2019 10:24 PM
> To: ovs-dev@openvswitch.org
> Cc: Nitin Katiyar ; Anju Thomas
> 
> Subject: [PATCH V2] dpif-netdev: Do RCU synchronization at fixed interval in
> PMD main loop.
> 
> Each PMD updates the global sequence number for RCU synchronization
> purpose with other OVS threads. This is done at every 1025th iteration in
> PMD main loop.
> 
> If the PMD thread is responsible for polling large number of queues that are
> carrying traffic, it spends a lot of time processing packets and this results 
> in
> significant delay in performing the housekeeping activities.
> 
> If the OVS main thread is waiting to synchronize with the PMD threads and if
> those threads delay performing housekeeping activities for more than 3 sec
> then LACP processing will be impacted and it will lead to LACP flaps. 
> Similarly,
> other controls protocols run by OVS main thread are impacted.
> 
> For e.g. a PMD thread polling 200 ports/queues with average of 1600
> processing cycles per packet with batch size of 32 may take 1024
> (200 * 1600 * 32) cycles per iteration. In system with 2.0 GHz CPU it means
> more than 5 ms per iteration. So, for 1024 iterations to complete it would be
> more than 5 seconds.
> 
> This gets worse when there are PMD threads which are less loaded.
> It reduces possibility of getting mutex lock in ovsrcu_try_quiesce() by 
> heavily
> loaded PMD and next attempt to quiesce would be after 1024 iterations.
> 
> With this patch, PMD RCU synchronization will be performed after fixed
> interval instead after a fixed number of iterations. This will ensure that 
> even if
> the packet processing load is high the RCU synchronization will not be delayed
> long.
> 
> Co-authored-by: Anju Thomas 
> Signed-off-by: Anju Thomas 
> Signed-off-by: Nitin Katiyar 
> ---
>  lib/dpif-netdev.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index d0a1c58..63b6cb9
> 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -228,6 +228,9 @@ struct dfc_cache {
>   * and used during rxq to pmd assignment. */  #define
> PMD_RXQ_INTERVAL_MAX 6
> 
> +/* Time in microseconds to try RCU quiescing. */ #define
> +PMD_RCU_QUIESCE_INTERVAL 1LL
> +
>  struct dpcls {
>  struct cmap_node node;  /* Within dp_netdev_pmd_thread.classifiers
> */
>  odp_port_t in_port;
> @@ -751,6 +754,9 @@ struct dp_netdev_pmd_thread {
> 
>  /* Set to true if the pmd thread needs to be reloaded. */
>  bool need_reload;
> +
> +/* Next time when PMD should try RCU quiescing. */
> +long long next_rcu_quiesce;
>  };
> 
>  /* Interface to netdev-based datapath. */ @@ -5486,6 +5492,8 @@ reload:
>  pmd->intrvl_tsc_prev = 0;
>  atomic_store_relaxed(&pmd->intrvl_cycles, 0);
>  cycles_counter_update(s);
> +
> +pmd->next_rcu_quiesce = pmd->ctx.now +
> PMD_RCU_QUIESCE_INTERVAL;
>  /* Protect pmd stats from external clearing while polling. */
>  ovs_mutex_lock(&pmd->perf_stats.stats_mutex);
>  for (;;) {
> @@ -5493,6 +5501,17 @@ reload:
> 
>  pmd_perf_start_iteration(s);
> 
> +/* Do RCU synchronization at fixed interval instead of doing it
> + * at fixed number of iterations. This ensures that synchronization
> + * would not be delayed long even at high load of packet
> + * processing. */
> +if (pmd->ctx.now > pmd->next_rcu_quiesce) {
> +if (!ovsrcu_try_quiesce()) {
> +pmd->next_rcu_quiesce =
> +pmd->ctx.now + PMD_RCU_QUIESCE_INTERVAL;
> +}
> +}
> +
>  for (i = 0; i < poll_cnt; i++) {
> 
>  if (!poll_list[i].rxq_enabled) { @@ -5527,6 +5546,8 @@ reload:
>  dp_netdev_pmd_try_optimize(pmd, poll_list, poll_cnt);
>  if (!ovsrcu_try_quiesce()) {
>  emc_cache_slow_sweep(&((pmd->flow_cache).emc_cache));
> +pmd->next_rcu_quiesce =
> +pmd->ctx.now + PMD_RCU_QUIESCE_INTERVAL;
>  }
> 
>  for (i = 0; i < poll_cnt; i++) { @@ -5986,6 +6007,7 @@
> dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd, struct
> dp_netdev *dp,
>  pmd->ctx.last_rxq = NULL;
>  pmd_thread_ctx_time_update(pmd);
>  pmd->next_optimization = pmd->ctx.now +
> DPCLS_OPTIMIZATION_INTERVAL;
> +pmd->next_rcu_quiesce = pmd->ctx.now +
> PMD_RCU_QUIESCE_INTERVAL;
>  pmd->rxq_next_cycle_store = pmd->ctx.now + PMD_RXQ_INTERVAL_LEN;
>  hmap_init(&pmd->poll_list);
>  hmap_init(&pmd->tx_ports);
> --
> 1.9.1

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

Re: [ovs-dev] [PATCH] packets: Fix using outdated RSS hash after MPLS decapsulation.

[Nitin Katiyar]

> -Original Message-
> From: Ilya Maximets [mailto:i.maxim...@samsung.com]
> Sent: Wednesday, August 28, 2019 12:44 PM
> To: Nitin Katiyar ; ovs-dev@openvswitch.org
> Cc: Stokes, Ian 
> Subject: Re: [ovs-dev] [PATCH] packets: Fix using outdated RSS hash after
> MPLS decapsulation.
> 
> On 28.08.2019 10:03, Nitin Katiyar wrote:
> >
> >
> >> -Original Message-
> >> From: Ilya Maximets [mailto:i.maxim...@samsung.com]
> >> Sent: Tuesday, August 27, 2019 5:37 PM
> >> To: Nitin Katiyar ;
> >> ovs-dev@openvswitch.org
> >> Cc: Stokes, Ian 
> >> Subject: Re: [ovs-dev] [PATCH] packets: Fix using outdated RSS hash
> >> after MPLS decapsulation.
> >>
> >> Hi.
> >>
> >> Since this is not a first version of a patch, the subject prefix
> >> should be [PATCH v2]. For the next version, please, use [PATCH v3].
> >> This way it's much easier to recognize/track patches in a list or mailbox.
> > Hi,
> > Since the title got of the patch got changed so I didn't make it V2. I 
> > wasn't
> sure to use V2 as with new title it becomes new thread. Anyway, I will be
> sending the new patch with V3.
> >
> >>
> >> Hint: It's generally speeds up review process if relevant people are
> >>   in CC list while sending the patch. Not everyone subscribed for
> >>   all the messages in a list.
> > Thanks for suggestion. I will follow it in future.
> >
> >>
> >> The code looks good in general.
> >> Few comments inline.
> >>
> >> Best regards, Ilya Maximets.
> >>
> >> On 16.08.2019 21:54, Nitin Katiyar wrote:
> >>> When a packet is received, the RSS hash is calculated if it is not
> >>> already available. The Exact Match Cache (EMC) entry is then looked
> >>> up using this RSS hash.
> >>>
> >>> When a MPLS encapsulated packet is received, the MPLS header is
> >>> popped and the packet is recirculated. Since the RSS hash has not
> >>> been invalidated here, the EMC lookup will hit the same entry as
> >>> that before recirculation. This degrades performance severely.
> >>
> >> Above sentences are not correct. It's stated that the collision
> >> happens with the previous pass of the same packet, but it happens
> >> with different packets from the same MPLS tunnel. This should be fixed.
> > It will actually collide with the entry of packet before de-capsulation. I 
> > have
> rephrased it as follows:
> >
> > When a MPLS encapsulated packet is received, the MPLS header is
> popped
> > and the packet is recirculated. Since the RSS hash has not been
> > invalidated here, the EMC lookup for decapsulated packets will hit the
> > same entry as that before recirculation (i.e. entry of the packet with
> > MPLS encapsulation). This degrades performance severely as different
> > inner packets from the same MPLS tunnel would hit the same EMC entry.
> >
> > Let me know if it is more clear now.
> 
> But dpif_netdev_packet_get_rss_hash() mixes the recirculation depth into
> RSS hash on each packet pass through the datapath. So, after recirculation RSS
> hash should be different from the hash of encapsulated packet. and where
> should be no collisions.
> I already wrote about this in my review to v1 and you said: "If there are
> multiple flows are sent across same tunnel (with same MPLS label) then they
> will all lead to same hash as external tuple and recirculation id remains same
> for them."  That makes sense, but it's opposite to what you're saying now. I'm
> confused.
Ah okay, I misinterpret your previous comment. You are correct. I have modified 
it as below:
When a MPLS encapsulated packet is received, the MPLS header is popped
and the packet is recirculated. Since the RSS hash has not been
invalidated here, the EMC lookup for all decapsulated packets will hit
the same entry even though these packets will have different tuple
values. This degrades performance severely as different inner packets
from the same MPLS tunnel would hit the same EMC entry.

Thanks for correcting this.

Regards,
Nitin
> 
> Best regards, Ilya Maximets.
> 
> >
> > Thanks,
> > Nitin
> >>
> >>>
> >>> This patch invalidates RSS hash (by resetting offload flags) after
> >>> MPLS header is popped.
> >>>
> >>> Signed-off-by: Nitin Katiyar 
> >>> ---
> >>>  lib/packets.c | 4 
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/lib/packets.c b/lib/packets.c index ab0b1a3..db3f6d0
> >>> 100644
> >>> --- a/lib/packets.c
> >>> +++ b/lib/packets.c
> >>> @@ -404,6 +404,10 @@ pop_mpls(struct dp_packet *packet, ovs_be16
> >> ethtype)
> >>>  struct mpls_hdr *mh = dp_packet_l2_5(packet);
> >>>  size_t len = packet->l2_5_ofs;
> >>>
> >>> +/* Invalidate offload flags as they are not valid after
> >>> + * decapsulation of MPLS header. */
> >>> +dp_packet_reset_offload(packet);
> >>
> >> Maybe it's better to move this code down to 'dp_packet_resize_l2_5'
> >> at the end of this function to have similar changes closer?
> >>
> > Sure, I will move it to end  of function.
> >>> 

Re: [ovs-dev] vswitchd crashed when revalidate flows in ovs 2.8.2

[ychen]

(gdb) p/x seq_mutex
$1 = {
  lock = {
__data = {
  __lock = 0x2, 
  __count = 0x0, 
  __owner = 0x0, > owner is already 0, but still abort
  __nusers = 0x0, 
  __kind = 0x2, 
  __spins = 0x0, 
  __elision = 0x0, 
  __list = {
__prev = 0x0, 
__next = 0x0
  }
}, 
__size = {0x2, 0x0 , 0x2, 0x0 }, 
__align = 0x2
  }, 
  where = 0x7f835b0e5520
}





At 2019-08-26 19:51:20, "ychen"  wrote:

Hi, 
   has any one see the following backtrace?


   Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `ovs-vswitchd unix:/var/run/openvswitch/db.sock 
-vconsole:emer -vsyslog:err -vfi'.
Program terminated with signal SIGABRT, Aborted.
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51  ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
[Current thread is 1 (Thread 0x7f82d6ffd700 (LWP 10089))]
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x7f835a2b042a in __GI_abort () at abort.c:89
#2  0x7f835a2a7e67 in __assert_fail_base (fmt=, 
assertion=assertion@entry=0x7f835ab39df2 "mutex->__data.__owner == 0", 
file=file@entry=0x7f835ab39dd5 "../nptl/pthread_mutex_lock.c", 
line=line@entry=81, 
function=function@entry=0x7f835ab39f60 <__PRETTY_FUNCTION__.8475> 
"__pthread_mutex_lock") at assert.c:92
#3  0x7f835a2a7f12 in __GI___assert_fail 
(assertion=assertion@entry=0x7f835ab39df2 "mutex->__data.__owner == 0", 
file=file@entry=0x7f835ab39dd5 "../nptl/pthread_mutex_lock.c", 
line=line@entry=81, 
function=function@entry=0x7f835ab39f60 <__PRETTY_FUNCTION__.8475> 
"__pthread_mutex_lock") at assert.c:101
#4  0x7f835ab30d50 in __GI___pthread_mutex_lock 
(mutex=mutex@entry=0x7f835b3935e0 ) at 
../nptl/pthread_mutex_lock.c:81
#5  0x7f835b064218 in ovs_mutex_lock_at (l_=l_@entry=0x7f835b3935e0 
, where=where@entry=0x7f835b1052cb "lib/seq.c:141")
at lib/ovs-thread.c:76
#6  0x7f835b0841d7 in seq_change (seq=0x55982c7b5630) at lib/seq.c:141
#7  0x7f835b062d06 in ovsrcu_quiesce () at lib/ovs-rcu.c:152
#8  0x7f835b5f7058 in revalidator_sweep__ 
(revalidator=revalidator@entry=0x55982c7bb178, purge=purge@entry=false)
at ofproto/ofproto-dpif-upcall.c:2549
#9  0x7f835b5f9b80 in revalidator_sweep (revalidator=0x55982c7bb178) at 
ofproto/ofproto-dpif-upcall.c:2556
#10 udpif_revalidator (arg=0x55982c7bb178) at ofproto/ofproto-dpif-upcall.c:913
#11 0x7f835b0641d7 in ovsthread_wrapper (aux_=) at 
lib/ovs-thread.c:348
#12 0x7f835ab2e4a4 in start_thread (arg=0x7f82d6ffd700) at 
pthread_create.c:456
#13 0x7f835a364d0f in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:97


we haven't find the rule how to reproduce it, but it seems crashes frequently 
about one time a day
  kernel version: 4.9.0-3-openstack-amd64
  ovs version:2.8.2  








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

Re: [ovs-dev] [PATCH] packets: Fix using outdated RSS hash after MPLS decapsulation.

[Ilya Maximets]

On 28.08.2019 10:03, Nitin Katiyar wrote:
> 
> 
>> -Original Message-
>> From: Ilya Maximets [mailto:i.maxim...@samsung.com]
>> Sent: Tuesday, August 27, 2019 5:37 PM
>> To: Nitin Katiyar ; ovs-dev@openvswitch.org
>> Cc: Stokes, Ian 
>> Subject: Re: [ovs-dev] [PATCH] packets: Fix using outdated RSS hash after
>> MPLS decapsulation.
>>
>> Hi.
>>
>> Since this is not a first version of a patch, the subject prefix should be 
>> [PATCH
>> v2]. For the next version, please, use [PATCH v3].
>> This way it's much easier to recognize/track patches in a list or mailbox.
> Hi,
> Since the title got of the patch got changed so I didn't make it V2. I wasn't 
> sure to use V2 as with new title it becomes new thread. Anyway, I will be 
> sending the new patch with V3.
> 
>>
>> Hint: It's generally speeds up review process if relevant people are
>>   in CC list while sending the patch. Not everyone subscribed for
>>   all the messages in a list.
> Thanks for suggestion. I will follow it in future.
> 
>>
>> The code looks good in general.
>> Few comments inline.
>>
>> Best regards, Ilya Maximets.
>>
>> On 16.08.2019 21:54, Nitin Katiyar wrote:
>>> When a packet is received, the RSS hash is calculated if it is not
>>> already available. The Exact Match Cache (EMC) entry is then looked up
>>> using this RSS hash.
>>>
>>> When a MPLS encapsulated packet is received, the MPLS header is popped
>>> and the packet is recirculated. Since the RSS hash has not been
>>> invalidated here, the EMC lookup will hit the same entry as that
>>> before recirculation. This degrades performance severely.
>>
>> Above sentences are not correct. It's stated that the collision happens with
>> the previous pass of the same packet, but it happens with different packets
>> from the same MPLS tunnel. This should be fixed.
> It will actually collide with the entry of packet before de-capsulation. I 
> have rephrased it as follows:
> 
> When a MPLS encapsulated packet is received, the MPLS header is popped
> and the packet is recirculated. Since the RSS hash has not been
> invalidated here, the EMC lookup for decapsulated packets will hit the
> same entry as that before recirculation (i.e. entry of the packet with
> MPLS encapsulation). This degrades performance severely as different
> inner packets from the same MPLS tunnel would hit the same EMC entry.
> 
> Let me know if it is more clear now.

But dpif_netdev_packet_get_rss_hash() mixes the recirculation depth into
RSS hash on each packet pass through the datapath. So, after recirculation
RSS hash should be different from the hash of encapsulated packet. and where
should be no collisions.
I already wrote about this in my review to v1 and you said: "If there are
multiple flows are sent across same tunnel (with same MPLS label) then they
will all lead to same hash as external tuple and recirculation id remains
same for them."  That makes sense, but it's opposite to what you're saying
now. I'm confused.

Best regards, Ilya Maximets.

> 
> Thanks,
> Nitin
>>
>>>
>>> This patch invalidates RSS hash (by resetting offload flags) after
>>> MPLS header is popped.
>>>
>>> Signed-off-by: Nitin Katiyar 
>>> ---
>>>  lib/packets.c | 4 
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/lib/packets.c b/lib/packets.c index ab0b1a3..db3f6d0
>>> 100644
>>> --- a/lib/packets.c
>>> +++ b/lib/packets.c
>>> @@ -404,6 +404,10 @@ pop_mpls(struct dp_packet *packet, ovs_be16
>> ethtype)
>>>  struct mpls_hdr *mh = dp_packet_l2_5(packet);
>>>  size_t len = packet->l2_5_ofs;
>>>
>>> +/* Invalidate offload flags as they are not valid after
>>> + * decapsulation of MPLS header. */
>>> +dp_packet_reset_offload(packet);
>>
>> Maybe it's better to move this code down to 'dp_packet_resize_l2_5' at the
>> end of this function to have similar changes closer?
>>
> Sure, I will move it to end  of function.
>>> +
>>>  set_ethertype(packet, ethtype);
>>>  if (get_16aligned_be32(&mh->mpls_lse) & htonl(MPLS_BOS_MASK)) {
>>>  dp_packet_set_l2_5(packet, NULL);
>>>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] packets: Fix using outdated RSS hash after MPLS decapsulation.

[Nitin Katiyar]

> -Original Message-
> From: Ilya Maximets [mailto:i.maxim...@samsung.com]
> Sent: Tuesday, August 27, 2019 5:37 PM
> To: Nitin Katiyar ; ovs-dev@openvswitch.org
> Cc: Stokes, Ian 
> Subject: Re: [ovs-dev] [PATCH] packets: Fix using outdated RSS hash after
> MPLS decapsulation.
> 
> Hi.
> 
> Since this is not a first version of a patch, the subject prefix should be 
> [PATCH
> v2]. For the next version, please, use [PATCH v3].
> This way it's much easier to recognize/track patches in a list or mailbox.
Hi,
Since the title got of the patch got changed so I didn't make it V2. I wasn't 
sure to use V2 as with new title it becomes new thread. Anyway, I will be 
sending the new patch with V3.

> 
> Hint: It's generally speeds up review process if relevant people are
>   in CC list while sending the patch. Not everyone subscribed for
>   all the messages in a list.
Thanks for suggestion. I will follow it in future.

> 
> The code looks good in general.
> Few comments inline.
> 
> Best regards, Ilya Maximets.
> 
> On 16.08.2019 21:54, Nitin Katiyar wrote:
> > When a packet is received, the RSS hash is calculated if it is not
> > already available. The Exact Match Cache (EMC) entry is then looked up
> > using this RSS hash.
> >
> > When a MPLS encapsulated packet is received, the MPLS header is popped
> > and the packet is recirculated. Since the RSS hash has not been
> > invalidated here, the EMC lookup will hit the same entry as that
> > before recirculation. This degrades performance severely.
> 
> Above sentences are not correct. It's stated that the collision happens with
> the previous pass of the same packet, but it happens with different packets
> from the same MPLS tunnel. This should be fixed.
It will actually collide with the entry of packet before de-capsulation. I have 
rephrased it as follows:

When a MPLS encapsulated packet is received, the MPLS header is popped
and the packet is recirculated. Since the RSS hash has not been
invalidated here, the EMC lookup for decapsulated packets will hit the
same entry as that before recirculation (i.e. entry of the packet with
MPLS encapsulation). This degrades performance severely as different
inner packets from the same MPLS tunnel would hit the same EMC entry.

Let me know if it is more clear now.

Thanks,
Nitin
> 
> >
> > This patch invalidates RSS hash (by resetting offload flags) after
> > MPLS header is popped.
> >
> > Signed-off-by: Nitin Katiyar 
> > ---
> >  lib/packets.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/lib/packets.c b/lib/packets.c index ab0b1a3..db3f6d0
> > 100644
> > --- a/lib/packets.c
> > +++ b/lib/packets.c
> > @@ -404,6 +404,10 @@ pop_mpls(struct dp_packet *packet, ovs_be16
> ethtype)
> >  struct mpls_hdr *mh = dp_packet_l2_5(packet);
> >  size_t len = packet->l2_5_ofs;
> >
> > +/* Invalidate offload flags as they are not valid after
> > + * decapsulation of MPLS header. */
> > +dp_packet_reset_offload(packet);
> 
> Maybe it's better to move this code down to 'dp_packet_resize_l2_5' at the
> end of this function to have similar changes closer?
> 
Sure, I will move it to end  of function.
> > +
> >  set_ethertype(packet, ethtype);
> >  if (get_16aligned_be32(&mh->mpls_lse) & htonl(MPLS_BOS_MASK)) {
> >  dp_packet_set_l2_5(packet, NULL);
> >
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev