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" <db...@vmware.com> wrote: > >> > >> On 4/4/17, 3:09 AM, "Lal, PrzemyslawX" <przemyslawx....@intel.com> > >> wrote: > >> On 04/04/2017 06:14, Darrell Ball wrote: > >> > > >> > On 4/3/17, 5:27 AM, "ovs-dev-boun...@openvswitch.org on behalf > >> of Przemyslaw Lal" <ovs-dev-boun...@openvswitch.org on behalf of > >> przemyslawx....@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....@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 <dlu...@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....@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....@intel.com> > >Signed-off-by: Ben Pfaff <b...@ovn.org> > >Acked by: Darrell Ball <dlu...@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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev