[ovs-dev,RESEND,ovs-dev] netdev-offload-tc: Add checking when assigning flow api
diff mbox series

Message ID 1579434043-28916-1-git-send-email-xiangxia.m.yue@gmail.com
State New
Headers show
Series
  • [ovs-dev,RESEND,ovs-dev] netdev-offload-tc: Add checking when assigning flow api
Related show

Commit Message

Tonghao Zhang Jan. 19, 2020, 11:40 a.m. UTC
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

netdev_assign_flow_api will try to init the netdev,
if success, the netdev will use the offload api.
If we init the type of netdev is dpdk, using the tc offload
api (netdev_tc_init_flow_api, which may be called firstly.),
the err log always is showing up. This patch adds a additional
check, and can void the err log.

"failed adding ingress qdisc required for offloading: No such device"

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 lib/netdev-linux.c      | 21 +++++++++++++++++++++
 lib/netdev-linux.h      |  1 +
 lib/netdev-offload-tc.c |  4 ++++
 3 files changed, 26 insertions(+)

Comments

Ilya Maximets Jan. 20, 2020, 8:59 a.m. UTC | #1
On 19.01.2020 12:40, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> netdev_assign_flow_api will try to init the netdev,
> if success, the netdev will use the offload api.
> If we init the type of netdev is dpdk, using the tc offload
> api (netdev_tc_init_flow_api, which may be called firstly.),
> the err log always is showing up. This patch adds a additional
> check, and can void the err log.
> 
> "failed adding ingress qdisc required for offloading: No such device"

I don't think that it's a good solution to restrict netdev-offload-tc
to netdev-linux ports only for two reasons:

1. netdev-vport offloading works via tc and this patch seems to break
   this functionality.

2. netdev-dpdk ports in theory could be offloaded via netdev-offload-tc
   in case of bifurcated drivers (Mellanox NICs).

The root cause of your issue is that netdev-dpdk reports fake ifindex
for ports that doesn't have one.  This is actually in issue, because
it's possible that reported ifindex will be valid for some other system
network interface and OVS will try to pass unrelated configuration to it.

IMHO, netdev-dpdk should not report fake ifindexes in the first place.
This will eliminate your issue since 'no ifindex' logged in INFO level.
However, I didn't check if this is fully safe and which parts will be
broken.  At least you'll need a couple of patches from this patch set
to make offloading work without ifindex:
https://patchwork.ozlabs.org/project/openvswitch/list/?series=146209

Best regards, Ilya Maximets.
Tonghao Zhang Jan. 20, 2020, 11:33 a.m. UTC | #2
On Mon, Jan 20, 2020 at 4:59 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 19.01.2020 12:40, xiangxia.m.yue@gmail.com wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > netdev_assign_flow_api will try to init the netdev,
> > if success, the netdev will use the offload api.
> > If we init the type of netdev is dpdk, using the tc offload
> > api (netdev_tc_init_flow_api, which may be called firstly.),
> > the err log always is showing up. This patch adds a additional
> > check, and can void the err log.
> >
> > "failed adding ingress qdisc required for offloading: No such device"
>
> I don't think that it's a good solution to restrict netdev-offload-tc
> to netdev-linux ports only for two reasons:
>
> 1. netdev-vport offloading works via tc and this patch seems to break
>    this functionality.
>
> 2. netdev-dpdk ports in theory could be offloaded via netdev-offload-tc
>    in case of bifurcated drivers (Mellanox NICs).
>
> The root cause of your issue is that netdev-dpdk reports fake ifindex
> for ports that doesn't have one.  This is actually in issue, because
> it's possible that reported ifindex will be valid for some other system
> network interface and OVS will try to pass unrelated configuration to it.
>
> IMHO, netdev-dpdk should not report fake ifindexes in the first place.
> This will eliminate your issue since 'no ifindex' logged in INFO level.
> However, I didn't check if this is fully safe and which parts will be
> broken.  At least you'll need a couple of patches from this patch set
> to make offloading work without ifindex:
> https://patchwork.ozlabs.org/project/openvswitch/list/?series=146209
That is better solution for me, thanks for explaining it to me.
> Best regards, Ilya Maximets.

Patch
diff mbox series

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index f8e59ba..bd83e64 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -3317,6 +3317,27 @@  const struct netdev_class netdev_afxdp_class = {
 #endif
 
 
+static bool
+is_linux_class(const struct netdev_class *class)
+{
+    if (class->destruct == netdev_linux_destruct) {
+        return true;
+    }
+#ifdef HAVE_AF_XDP
+    if (class->destruct == netdev_afxdp_destruct) {
+        return true;
+    }
+#endif
+
+    return false;
+}
+
+bool
+netdev_linux_flow_api_supported(const struct netdev *netdev)
+{
+    return is_linux_class(netdev->netdev_class);
+}
+
 #define CODEL_N_QUEUES 0x0000
 
 /* In sufficiently new kernel headers these are defined as enums in
diff --git a/lib/netdev-linux.h b/lib/netdev-linux.h
index e1e30f8..43cc725 100644
--- a/lib/netdev-linux.h
+++ b/lib/netdev-linux.h
@@ -28,5 +28,6 @@  struct netdev;
 int netdev_linux_ethtool_set_flag(struct netdev *netdev, uint32_t flag,
                                   const char *flag_name, bool enable);
 int linux_get_ifindex(const char *netdev_name);
+bool netdev_linux_flow_api_supported(const struct netdev *netdev);
 
 #endif /* netdev-linux.h */
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 1adbb32..80bbf2d 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1643,6 +1643,10 @@  netdev_tc_init_flow_api(struct netdev *netdev)
     int ifindex;
     int error;
 
+    if (!netdev_linux_flow_api_supported(netdev)) {
+        return EOPNOTSUPP;
+    }
+
     ifindex = netdev_get_ifindex(netdev);
     if (ifindex < 0) {
         VLOG_INFO("init: failed to get ifindex for %s: %s",