[ovs-dev] [PATCH v5] netdev-dpdk: fix ifindex assignment for DPDK ports

Ben Pfaff blp at ovn.org
Sat May 20 23:36:53 UTC 2017


On Fri, May 19, 2017 at 10:19:10AM +0200, Przemyslaw Lal wrote:
> On 18/05/2017 22:41, Ben Pfaff wrote:
> 
> >On Thu, May 18, 2017 at 06:09:21PM +0000, Darrell Ball wrote:
> >>
> >>On 4/4/17, 5:47 PM, "Darrell Ball" <dball at vmware.com> wrote:
> >>
> >>     On 4/4/17, 3:09 AM, "Lal, PrzemyslawX" <przemyslawx.lal at intel.com> wrote:
> >>         On 04/04/2017 06:14, Darrell Ball wrote:
> >>         >
> >>         > On 4/3/17, 5:27 AM, "ovs-dev-bounces at openvswitch.org on behalf of Przemyslaw Lal" <ovs-dev-bounces at openvswitch.org on behalf of przemyslawx.lal at intel.com> wrote:
> >>         >
> >>         >      In current implementation port_id is used as an ifindex for all netdev-dpdk
> >>         >      interfaces.
> >>         >
> >>         >      For physical DPDK interfaces using port_id as ifindex causes that '0' is set as
> >>         >      ifindex for 'dpdk0' interface, '1' for 'dpdk1' and so on. For the DPDK vHost
> >>         >      interfaces ifindexes are not even assigned (0 is used by default) due to the
> >>         >      fact that vHost ports don't use port_id field from the DPDK library.
> >>         >
> >>         >      This causes multiple negative side-effects. First of all 0 is an invalid
> >>         >      ifindex value. The other issue is possible overlapping of 'dpdkX' interfaces
> >>         >      ifindex values with the ifindexes of kernel space interfaces which may cause
> >>         >      problems in any external tools that use those values. Neither 'dpdk0', nor any
> >>         >      DPDK vHost interfaces are visible in sFlow collector tools, as all interfaces
> >>         >      with ifindexes smaller than 1 are ignored.
> >>         >
> >>         >      Proposed solution to these issues is to calculate a hash of interface's name
> >>         >      and use calculated value as an ifindex. This way interfaces keep their
> >>         >      ifindexes during OVS-DPDK restarts, ports re-initialization events, etc., show
> >>         >      up in sFlow collectors and meet RFC 2863 specification regarding re-using
> >>         >      ifindex values by the same virtual interfaces and maximum ifindex value.
> >>         >
> >>         >      Signed-off-by: Przemyslaw Lal <przemyslawx.lal at intel.com>
> >>         >      ---
> >>         >       lib/netdev-dpdk.c | 6 +++++-
> >>         >       1 file changed, 5 insertions(+), 1 deletion(-)
> >>         >
> >>         >      diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> >>         >      index ddc651b..687b0a5 100644
> >>         >      --- a/lib/netdev-dpdk.c
> >>         >      +++ b/lib/netdev-dpdk.c
> >>         >      @@ -2215,7 +2215,11 @@ netdev_dpdk_get_ifindex(const struct netdev *netdev)
> >>         >           int ifindex;
> >>         >
> >>         >           ovs_mutex_lock(&dev->mutex);
> >>         >      -    ifindex = dev->port_id;
> >>         >      +    /* Calculate hash from the netdev name. Ensure that ifindex is a 24-bit
> >>         >      +     * postive integer to meet RFC 2863 recommendations.
> >>         >      +     */
> >>         >      +    uint32_t h = hash_string(netdev->name, 0);
> >>         >      +    ifindex = (int)(h % 0xfffffe + 1);
> >>         >
> >>         >
> >>         > If user configuration was supported, enforcing uniqueness would be the
> >>         > responsibility of the user.
> >>         This was already discussed on this mailing list and outcome was that while hash is not perfect, it is the best solution for now.
> >>         Also, please keep in mind that names of the physical DPDK devices and dpdkvhostuser interfaces are configurable, so user can still enforce
> >>         uniqueness.
> >>     I know uniqueness could be enforced by trial and error of name selection.
> >>     I saw the comment
> >>      “At some point, with vhost-pmd we will have port_ids also for vhost interfaces.
> >>        Maybe we can revisit this approach when that becomes available.”
> >>     If others are fine, then so am I.
> >>
> >>The uniqueness issue is understood and there is a workaround capability to
> >>address it.
> >>
> >>Let us just fold the patch in, since the patch has been out for long enough to
> >>receive feedback.
> >>
> >>Acked by: Darrell Ball <dlu998 at gmail.com>
> >Thanks Przemyslaw and Darrell.  I applied this to master.  I simplified
> >the code since the cast to int was a no-op (it does the same thing as
> >the conversion implied by the assignment:
> >
> >--8<--------------------------cut here-------------------------->8--
> >
> >From: Przemyslaw Lal <przemyslawx.lal at intel.com>
> >Date: Mon, 3 Apr 2017 13:27:47 +0100
> >Subject: [PATCH] netdev-dpdk: fix ifindex assignment for DPDK ports
> >
> >In current implementation port_id is used as an ifindex for all netdev-dpdk
> >interfaces.
> >
> >For physical DPDK interfaces using port_id as ifindex causes that '0' is set as
> >ifindex for 'dpdk0' interface, '1' for 'dpdk1' and so on. For the DPDK vHost
> >interfaces ifindexes are not even assigned (0 is used by default) due to the
> >fact that vHost ports don't use port_id field from the DPDK library.
> >
> >This causes multiple negative side-effects. First of all 0 is an invalid
> >ifindex value. The other issue is possible overlapping of 'dpdkX' interfaces
> >ifindex values with the ifindexes of kernel space interfaces which may cause
> >problems in any external tools that use those values. Neither 'dpdk0', nor any
> >DPDK vHost interfaces are visible in sFlow collector tools, as all interfaces
> >with ifindexes smaller than 1 are ignored.
> >
> >Proposed solution to these issues is to calculate a hash of interface's name
> >and use calculated value as an ifindex. This way interfaces keep their
> >ifindexes during OVS-DPDK restarts, ports re-initialization events, etc., show
> >up in sFlow collectors and meet RFC 2863 specification regarding re-using
> >ifindex values by the same virtual interfaces and maximum ifindex value.
> >
> >Signed-off-by: Przemyslaw Lal <przemyslawx.lal at intel.com>
> >Signed-off-by: Ben Pfaff <blp at ovn.org>
> >Acked by: Darrell Ball <dlu998 at gmail.com>
> >---
> >  lib/netdev-dpdk.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> >diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> >index 609b8da33537..61bfe3499fe3 100644
> >--- a/lib/netdev-dpdk.c
> >+++ b/lib/netdev-dpdk.c
> >@@ -1,5 +1,5 @@
> >  /*
> >- * Copyright (c) 2014, 2015, 2016 Nicira, Inc.
> >+ * Copyright (c) 2014, 2015, 2016, 2017 Nicira, Inc.
> >   *
> >   * Licensed under the Apache License, Version 2.0 (the "License");
> >   * you may not use this file except in compliance with the License.
> >@@ -2213,10 +2213,12 @@ static int
> >  netdev_dpdk_get_ifindex(const struct netdev *netdev)
> >  {
> >      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> >-    int ifindex;
> >      ovs_mutex_lock(&dev->mutex);
> >-    ifindex = dev->port_id;
> >+    /* Calculate hash from the netdev name. Ensure that ifindex is a 24-bit
> >+     * postive integer to meet RFC 2863 recommendations.
> >+     */
> >+    int ifindex = hash_string(netdev->name, 0) % 0xfffffe + 1;
> >      ovs_mutex_unlock(&dev->mutex);
> >      return ifindex;
> 
> Excellent, thank you Ben and Darell.
> 
> One minor question to Ben - do you plan to update the AUTHORS file at
> some point or am I supposed to create the relevant change on my own?

We encourage people to do it on their own but I also update it when I
notice it needs a change.

You've been added now.


More information about the dev mailing list