[ovs-dev] [PATCH] netdev: Dynamic per-port Flow API.
Ilya Maximets
i.maximets at samsung.com
Mon May 13 09:21:45 UTC 2019
On 09.05.2019 1:59, Ophir Munk wrote:
> Hi Ilya,
>
>> -----Original Message-----
>> From: Ilya Maximets <i.maximets at samsung.com>
>> Sent: Wednesday, April 24, 2019 6:12 PM
>> To: Ophir Munk <ophirmu at mellanox.com>; ovs-dev at openvswitch.org
>> Cc: Ian Stokes <ian.stokes at intel.com>; Flavio Leitner <fbl at sysclose.org>;
>> Kevin Traynor <ktraynor at redhat.com>; Roni Bar Yanai
>> <roniba at mellanox.com>; Finn Christensen <fc at napatech.com>; Ben Pfaff
>> <blp at ovn.org>; Roi Dayan <roid at mellanox.com>; Simon Horman
>> <simon.horman at netronome.com>; Olga Shern <olgas at mellanox.com>;
>> Asaf Penso <asafp at mellanox.com>; Oz Shlomo <ozsh at mellanox.com>; Paul
>> Blakey <paulb at mellanox.com>
>> Subject: Re: [PATCH] netdev: Dynamic per-port Flow API.
>> ...
>>>> +static int
>>>> +netdev_assign_flow_api(struct netdev *netdev) {
>>>> + struct netdev_registered_flow_api *rfa;
>>>> +
>>>> + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
>>>> + if (!rfa->flow_api->init_flow_api(netdev)) {
>>>> + ovs_refcount_ref(&rfa->refcnt);
>>>> + ovsrcu_set(&netdev->flow_api, rfa->flow_api);
>>>> + VLOG_INFO("%s: Assigned flow API '%s'.",
>>>> + netdev_get_name(netdev), rfa->flow_api->type);
>>>> + return 0;
>>>> + }
>>>
>>> By this logic the first API which successfully passes init_flow_api() becomes
>> the flow_api for this netdev.
>>> Assuming there are 2 vport APIs: 1. system datapath vport API 2. netdev
>> datapath vport API (planned to be added soon).
>>> Assuming both vport APIs successfully pass init_flow_api(), then the first
>> one to be tested will always be chosen and the second one will never be
>> used.
>>> Are we sure that the system datapath vport will not always be chosen with
>> the current netdev_tc_init_flow_api() implementation.
>>> How can we distinguish between a vport created under different
>> datapaths?
>>> This issue must be resolved for this patch.
>>
>> netdev-vport that works in userspace datapath has no backing linux interface
>> --> get_ifindex failure --> no offloading API configured.
>
> I think the above suggestion will not solve the issue of how we can distinguish between a vport created under different datapaths?
> Relying on get_index will not work in case you have two vports (say both of type "vxlan"): one in kernel and another in userspace.
> In such a case: the kernel vxlan vport will be added first to linux with device name "vxlan_sys_4789".
> The same name is used by the userspace vport vxlan. Therefore,
>
> netdev-vport that works in userspace datapath will have the same name of the linux-vport which is already created in linux
> --> get_ifindex will unintentionally succeed --> netdev-vport will falsely select the kernel offloading API (as already described above: "By this logic the first API...").
>
> 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.
>
> 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.
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.
More information about the dev
mailing list