-----Original Message-----
From: Ilya Maximets <i.maxim...@samsung.com>
Sent: Monday, May 13, 2019 3:33 PM
To: Ophir Munk <ophi...@mellanox.com>; ovs-dev@openvswitch.org
Cc: Ian Stokes <ian.sto...@intel.com>; Flavio Leitner <f...@sysclose.org>;
Kevin Traynor <ktray...@redhat.com>; Roni Bar Yanai
<ron...@mellanox.com>; Finn Christensen <f...@napatech.com>; Ben Pfaff
<b...@ovn.org>; Simon Horman <simon.hor...@netronome.com>; Olga
Shern <ol...@mellanox.com>; Asaf Penso <as...@mellanox.com>; Oz
Shlomo <o...@mellanox.com>; Majd Dibbiny <m...@mellanox.com>
Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
On 13.05.2019 14:41, Ophir Munk wrote:
Hi Ilya,
-----Original Message-----
From: Ilya Maximets <i.maxim...@samsung.com>
Sent: Monday, May 13, 2019 12:22 PM
To: Ophir Munk <ophi...@mellanox.com>; ovs-dev@openvswitch.org
Cc: Ian Stokes <ian.sto...@intel.com>; Flavio Leitner
<f...@sysclose.org>; Kevin Traynor <ktray...@redhat.com>; Roni Bar
Yanai <ron...@mellanox.com>; Finn Christensen <f...@napatech.com>;
Ben
Pfaff <b...@ovn.org>; Simon Horman <simon.hor...@netronome.com>;
Olga
Shern <ol...@mellanox.com>; Asaf Penso <as...@mellanox.com>; Oz
Shlomo <o...@mellanox.com>
Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
On 09.05.2019 1:59, Ophir Munk wrote:
Hi Ilya,
...
I am recreating this scenario in my setup.
I see. Yes, you're right. And I think that this case could be
reproduced on current master without any patches. So, it's a bug that we
need to fix.
Otherwise userspace datapath will try to offload its flows to the
unrelated system interfaces. For now we could just forbid offloading
to vports in dpif- netdev. I'll prepare the patch. This fix also should be
backported.
We need a way to enable offloading of vports in dpif-netdev. So if you
can address It with your new patch - that would be appreciated.
I'm suggesting disabling because it'll be easy to backport to older branches.
On master, after applying dynamic flow API patch-set, we'll be able to use
patch that I suggested in previous mail to properly handle this situation and
enable vport offloading.
This patch series is important but one of its main goals is to
enable different
offloads for different vports under Kernel/userspace.
Can you please advise how this goal can be achieved?
It looks like there is no way to distinguish system and userspace
vports by looking only on netdev. We should check with dpif type.
Agreed. But then looking at your sample patch below you are basing
your decision on the existence of system port (and not on dpif type).
Right now 'ifindex' is used for checking, so this is equally racy.
I think it is risky because
you are dependent on the order of operations: Creation of the system port
versus checking for system port existence.
Which was first (under different scenarios)?
Actually, port creation and checking will always happen in the same thread
context, so creation and checking will be serialized. Kernel should guarantee
the port existence. No races here.
What do you say about the following patch:
diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c int
netdev_ports_insert(struct netdev *netdev, const struct dpif_class
*dpif_class,
struct dpif_port *dpif_port) @@ -547,8 +555,9 @@
netdev_ports_insert(struct netdev *netdev, const struct dpif_class
*dpif_class,
netdev_ports_hash(dpif_port->port_no, dpif_class));
hmap_insert(&ifindex_to_port, &data->ifindex_node, ifindex);
ovs_mutex_unlock(&netdev_hmap_mutex);
- netdev_init_flow_api(netdev);
+ netdev_init_flow_api(netdev, dpif_class->type); /* Pass class
+ type "netdev" or "syste" */
return 0;
}
It's not a full solution. It is just a hint how to pass the dpif type ("netdev"
or
"system") when initializing the flow api.
We can use the dpif type when initializing vport offload.
This is, actually, the first thing I tried. However, this requires exposing the
internals of 'dpif-provider', which is bad from the point of code architecture.
For the below patch code, I missed actual port requesting from the datapath.
Following incremental needed:
---
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index
82943c102..1c88a91ed 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -124,13 +124,28 @@ netdev_vport_class_get_dpif_port(const struct
netdev_class *class) bool netdev_vport_has_system_port(const struct
netdev *netdev) {
- bool found;
+ bool found = false;
+ const char *name;
+ const char *type = "system";
struct sset names = SSET_INITIALIZER(&names);
ovs_assert(is_vport_class(netdev_get_class(netdev)));
- dp_enumerate_names("system", &names);
- found = sset_contains(&names, netdev_get_name(netdev));
+ dp_enumerate_names(type, &names);
+ SSET_FOR_EACH (name, &names) {
+ struct dpif *dpifp = NULL;
+
+ if (dpif_open(name, type, &dpifp)) {
+ continue;
+ }
+
+ found = dpif_port_exists(dpifp, netdev_get_name(netdev));
+
+ dpif_close(dpifp);
+ if (found) {
+ break;
+ }
+ }
sset_destroy(&names);
return found;
---
Something like this (not tested):
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 01e900461..b7b0616ec 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -22,6 +22,7 @@
#include "dpif-netdev.h"
#include "netdev-offload-provider.h"
#include "netdev-provider.h"
+#include "netdev-vport.h"
#include "openvswitch/match.h"
#include "openvswitch/vlog.h"
#include "packets.h"
@@ -752,6 +753,13 @@ netdev_offload_dpdk_flow_del(struct netdev
*netdev, const ovs_u128 *ufid, static int
netdev_offload_dpdk_init_flow_api(struct netdev *netdev) {
+ if (netdev_vport_is_vport_class(netdev->netdev_class)
+ && netdev_vport_has_system_port(netdev)) {
+ VLOG_DBG("%s: vport has backing system interface. Skipping.",
+ netdev_get_name(netdev));
+ return EOPNOTSUPP;
+ }
+
return netdev_dpdk_flow_api_supported(netdev) ? 0 : EOPNOTSUPP;
}
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index
498aae369..e4d121ed7 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -31,6 +31,7 @@
#include "netdev-linux.h"
#include "netdev-offload-provider.h"
#include "netdev-provider.h"
+#include "netdev-vport.h"
#include "netlink.h"
#include "netlink-socket.h"
#include "odp-netlink.h"
@@ -1560,6 +1561,13 @@ netdev_tc_init_flow_api(struct netdev
*netdev)
int ifindex;
int error;
+ if (netdev_vport_is_vport_class(netdev->netdev_class)
+ && !netdev_vport_has_system_port(netdev)) {
+ VLOG_DBG("%s: vport has no backing system interface. Skipping.",
+ netdev_get_name(netdev));
+ return EOPNOTSUPP;
+ }
+
ifindex = netdev_get_ifindex(netdev);
if (ifindex < 0) {
VLOG_INFO("init: failed to get ifindex for %s: %s", diff
--git a/lib/netdev- vport.c b/lib/netdev-vport.c index
92a256af1..82943c102 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -44,6 +44,7 @@
#include "simap.h"
#include "smap.h"
#include "socket-util.h"
+#include "sset.h"
#include "unaligned.h"
#include "unixctl.h"
#include "openvswitch/vlog.h"
@@ -120,6 +121,21 @@ netdev_vport_class_get_dpif_port(const struct
netdev_class *class)
return is_vport_class(class) ?
vport_class_cast(class)->dpif_port : NULL; }
+bool
+netdev_vport_has_system_port(const struct netdev *netdev) {
+ bool found;
+ struct sset names = SSET_INITIALIZER(&names);
+
+ ovs_assert(is_vport_class(netdev_get_class(netdev)));
+
+ dp_enumerate_names("system", &names);
+ found = sset_contains(&names, netdev_get_name(netdev));
+ sset_destroy(&names);
+
+ return found;
+}
+
const char *
netdev_vport_get_dpif_port(const struct netdev *netdev,
char namebuf[], size_t bufsize) diff
--git a/lib/netdev-vport.h b/lib/netdev-vport.h index
9d756a265..4be1b92ec 100644
--- a/lib/netdev-vport.h
+++ b/lib/netdev-vport.h
@@ -40,6 +40,7 @@ void netdev_vport_inc_tx(const struct netdev *,
const struct dpif_flow_stats *);
bool netdev_vport_is_vport_class(const struct netdev_class *);
+bool netdev_vport_has_system_port(const struct netdev *);
const char *netdev_vport_class_get_dpif_port(const struct
netdev_class *);
#ifndef _WIN32
---
I'll format this as a proper patch and send along with patch for
enabling offloading for ports without correct ifindex.
Best regards, Ilya Maximets.
Best regards,
Ophir
