> 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(&dev->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. > + /* 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 & 0x0FFFFFFF)); 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. > ovs_mutex_unlock(&dev->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