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

[Stokes, Ian]

> 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.lal at 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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev