| Message ID | 20170518204147.GU32729@ovn.org |
|---|---|
| State | Not Applicable |
| Headers | show |
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@vmware.com> wrote: >> >> >> >> On 4/4/17, 3:09 AM, "Lal, PrzemyslawX" <przemyslawx.lal@intel.com> wrote: >> >> On 04/04/2017 06:14, Darrell Ball wrote: >> >> > >> > On 4/3/17, 5:27 AM, "ovs-dev-bounces@openvswitch.org on behalf of Przemyslaw Lal" <ovs-dev-bounces@openvswitch.org on behalf of przemyslawx.lal@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@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@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@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@intel.com> > Signed-off-by: Ben Pfaff <blp@ovn.org> > Acked by: Darrell Ball <dlu998@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? -------------------------------------------------------------- Intel Research and Development Ireland Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
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@vmware.com> wrote: > >> > >> On 4/4/17, 3:09 AM, "Lal, PrzemyslawX" <przemyslawx.lal@intel.com> wrote: > >> On 04/04/2017 06:14, Darrell Ball wrote: > >> > > >> > On 4/3/17, 5:27 AM, "ovs-dev-bounces@openvswitch.org on behalf of Przemyslaw Lal" <ovs-dev-bounces@openvswitch.org on behalf of przemyslawx.lal@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@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@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@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@intel.com> > >Signed-off-by: Ben Pfaff <blp@ovn.org> > >Acked by: Darrell Ball <dlu998@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.
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;