Re: [ovs-dev] [PATCH v5] netdev-dpdk: fix ifindex assignment for DPDK ports
On 18/05/2017 22:41, Ben Pfaff wrote: On Thu, May 18, 2017 at 06:09:21PM +, 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(>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 % 0xfe + 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 f
[ovs-dev] [PATCH v5] 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> --- 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(>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 % 0xfe + 1); ovs_mutex_unlock(>mutex); return ifindex; -- 1.9.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4] 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 RFC2863 specification regarding re-using ifindex values by the same virtual interfaces. Signed-off-by: Przemyslaw Lal <przemyslawx@intel.com> --- lib/netdev-dpdk.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ddc651b..646a2a2 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2215,7 +2215,12 @@ netdev_dpdk_get_ifindex(const struct netdev *netdev) int ifindex; ovs_mutex_lock(>mutex); -ifindex = dev->port_id; +/* Calculate hash from the netdev name. Ensure that ifindex is non-negative + * integer, XOR to improve collision rate after right shift and ensure that + * calculated value is 1 or larger. + */ +uint32_t h = hash_string(netdev->name, 0); +ifindex = (int)(((h >> 1) ^ (h & 0x7fff)) % 0x7ffe + 1); ovs_mutex_unlock(>mutex); return ifindex; -- 1.9.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3] 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 RFC2863 specification regarding re-using ifindex values by the same virtual interfaces. Signed-off-by: Przemyslaw Lal <przemyslawx@intel.com> --- lib/netdev-dpdk.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index ddc651b..d3b2184 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2216,6 +2216,12 @@ netdev_dpdk_get_ifindex(const struct netdev *netdev) ovs_mutex_lock(>mutex); ifindex = dev->port_id; +/* Calculate hash from the netdev name. Ensure that ifindex is non-negative + * integer, XOR to improve collision rate after right shift and ensure that + * calculated value is 1 or larger. + */ +uint32_t h = hash_string(netdev->name, 0); +ifindex = (int)(((h >> 1) ^ (h & 0x7fff)) % 0x7ffe + 1); ovs_mutex_unlock(>mutex); return ifindex; -- 1.9.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V2] 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 infindexes 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 RFC2863 specification regarding re-using ifindex values by the same virtual interfaces. Signed-off-by: Przemyslaw Lal <przemyslawx@intel.com> --- Thanks for the patch. I'm not the most experienced with sflow but after looking into it its clear the need for a change to ifindex is required for DPDK devices. lib/netdev-dpdk.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index de78ddd..ef99eb3 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2075,7 +2075,13 @@ netdev_dpdk_get_ifindex(const struct netdev *netdev) int ifindex; ovs_mutex_lock(>mutex); -ifindex = dev->port_id; Using a hash for the ifindex of dpdk devices will essentially make them non sequential. More of a question to the community, but is it ok if ifindexes are non-sequential? Wasn't sure of this myself. There's no requirement for ifIndexes to be sequential, even more - calculating hash using devices' names ensures that the same devices will have the same ifIndex after event of port re-initialization and OVS restart. Such behavior is also recommended by IETF RFC 2863 standard. Using hash was also suggested by the community during patch V1 review. +/* Calculate hash from the netdev name using hash_bytes() function. + * Because ifindex is declared as signed int in the kernel sources and + * OVS follows this implementation right shift is needed to set sign bit + * to 0 and then XOR to slightly improve collision rate. + */ +uint32_t h = hash_bytes(netdev->name, strlen(netdev->name), 0); +ifindex = (int)((h >> 1) ^ (h & 0x0FFF)); In the (unlikely) event of a hash collision, what would be the outcome? From what I can see the main use is for dpif_sflow_add_port(). I'm just wondering is there a need to ensure that no 2 DPDK ports have the same ifindex? From What I can see dpif_sflow_add_port() would still execute for the port. Only one of them will be accessible in the sFlow. However, as you mentioned, this is very unlikely - during my tests I've generated random 100 000 vHost-user port names using"vhu-XX" naming scheme where Xs are the first characters of UUID - I used Openstack port naming convention as an inspiration - and maximum number of collisions I observed was 5, but in the most of runs it was 0. Anyway, at the end of the day it shouldn't be a big issue since DPDK vHost and DPDK physical interfaces names can be changed by user. ovs_mutex_unlock(>mutex); return ifindex; -- 1.9.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH V2] 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 infindexes 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 RFC2863 specification regarding re-using ifindex values by the same virtual interfaces. Signed-off-by: Przemyslaw Lal <przemyslawx@intel.com> --- lib/netdev-dpdk.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index de78ddd..ef99eb3 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2075,7 +2075,13 @@ netdev_dpdk_get_ifindex(const struct netdev *netdev) int ifindex; ovs_mutex_lock(>mutex); -ifindex = dev->port_id; +/* Calculate hash from the netdev name using hash_bytes() function. + * Because ifindex is declared as signed int in the kernel sources and + * OVS follows this implementation right shift is needed to set sign bit + * to 0 and then XOR to slightly improve collision rate. + */ +uint32_t h = hash_bytes(netdev->name, strlen(netdev->name), 0); +ifindex = (int)((h >> 1) ^ (h & 0x0FFF)); ovs_mutex_unlock(>mutex); return ifindex; -- 1.9.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
