Re: [dpdk-dev] [PATCH 2/5] net/mlx5: retrieve device index from Netlink

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Tue, Mar 13, 2018 at 01:50:36PM +0100, Nelio Laranjeiro wrote:
> This patch new file is not compiled yet, it starts a series necessary to
> fix some issues with VF devices.
> 
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>

The fact this new file is not compiled in at this point won't be obvious
when running git bisect and makes validation more difficult. I suggest to
simply merge it into the next patch.

A few more comments on this code below.

> ---
>  drivers/net/mlx5/mlx5_vf.c | 134 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 134 insertions(+)
>  create mode 100644 drivers/net/mlx5/mlx5_vf.c
> 
> diff --git a/drivers/net/mlx5/mlx5_vf.c b/drivers/net/mlx5/mlx5_vf.c

Regarding the name of this file, it looks to me like all the enclosed
functions could work equally well with non-VF devices. While this series is
targeted at VFs, internal functions are actually more versatile.

I suggest a more generic name, e.g. "mlx5_nl.c".

> new file mode 100644
> index 000000000..357407f56
> --- /dev/null
> +++ b/drivers/net/mlx5/mlx5_vf.c
> @@ -0,0 +1,134 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2018 6WIND S.A.
> + * Copyright 2018 Mellanox Technologies, Ltd.
> + */
> +
> +#include <linux/rtnetlink.h>
> +#include <linux/netlink.h>
> +
> +#include <rte_netlink.h>
> +
> +#include "mlx5.h"
> +#include "mlx5_utils.h"
> +
> +/* Data exchanged between Netlink library and PMD. */
> +struct mlx5_vf_iface {
> +	struct rte_eth_dev *dev; /**< Pointer to Ethernet device. */
> +	int iface_idx; /**< Network Interface index. */
> +};

Minor documentation nits here and elsewhere, you should remove the random
capitalization of "Interface", "Message", "Header" and so on in the middle
of sentences.

> +
> +/**
> + * Parse Netlink message to retrieve the interface index.
> + *
> + * @param nh
> + *   Pointer to Netlink Message Header.
> + * @param arg
> + *   PMD data register with this callback.
> + *
> + * @return
> + *   0 on success, -1 otherwise.

Looks like it only returns 0, never -1.

You should probably describe that ((struct mlx5_vf_iface)arg)->iface_idx is
left to -1 when its index can't be found.

> + */
> +static int
> +mlx5_vf_iface_idx_cb(struct nlmsghdr *nh, void *arg)
> +{
> +	struct mlx5_vf_iface *data = arg;
> +	struct rte_eth_dev *dev = data->dev;
> +	const struct ether_addr *mac = &dev->data->mac_addrs[0];
> +	struct ifinfomsg *iface;
> +	struct rtattr *attribute;
> +	int len;
> +
> +	/**
> +	 * Leave right away if the index does not match its initialised value.
> +	 * Interface index has already been found.
> +	 */
> +	if (data->iface_idx != -1)
> +		return 0;
> +	iface = NLMSG_DATA(nh);
> +	len = nh->nlmsg_len - NLMSG_LENGTH(sizeof(*iface));
> +	for (attribute = IFLA_RTA(iface);
> +	     RTA_OK(attribute, len);
> +	     attribute = RTA_NEXT(attribute, len)) {
> +		if (attribute->rta_type == IFLA_ADDRESS &&
> +		    !memcmp(RTA_DATA(attribute), mac, ETHER_ADDR_LEN)) {

Hmm, I'm not sure a matching MAC address is safe enough to determine it's
the right netdevice. Could be a bridge (br0) interface or any other virtual
thing. Although not great, you should probably rely on /sys as in
mlx5_get_ifname().

> +#ifndef NDEBUG
> +			struct ether_addr m;
> +
> +			memcpy(&m, RTA_DATA(attribute), ETHER_ADDR_LEN);
> +			DRV_LOG(DEBUG,
> +				"port %u interace %d MAC address"

interace => interface

> +				" %02X:%02X:%02X:%02X:%02X:%02X",
> +				dev->data->port_id,
> +				iface->ifi_index,
> +				m.addr_bytes[0], m.addr_bytes[1],
> +				m.addr_bytes[2], m.addr_bytes[3],
> +				m.addr_bytes[4], m.addr_bytes[5]);
> +#endif
> +			data->iface_idx = iface->ifi_index;
> +			return 0;
> +		}
> +	}
> +	data->iface_idx = -1;
> +	return 0;
> +}
> +
> +/**
> + * Retrieve interface Netlink interface index.
> + *
> + * @param dev
> + *   Pointer to Ethernet device.
> + *
> + * @return
> + *   Interface index, a negative errno value otherwise and rte_errno is set.
> + */
> +static int
> +mlx5_vf_iface_idx(struct rte_eth_dev *dev)
> +{
> +	struct nl_req {
> +		struct nlmsghdr hdr;
> +		struct rtgenmsg gen;
> +	} req = {
> +		.hdr = {
> +			.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtgenmsg)),
> +			.nlmsg_type = RTM_GETLINK,
> +			.nlmsg_flags = NLM_F_REQUEST | NLM_F_DUMP | NLM_F_ROOT,
> +		},
> +		.gen = {
> +			.rtgen_family =  AF_UNSPEC,

Extra space before AF_UNSPEC.

> +		},
> +	};
> +	struct mlx5_vf_iface data = {
> +		.dev = dev,
> +		.iface_idx = -1,
> +	};
> +	int fd;
> +	int ret;
> +
> +	fd = rte_nl_init(RTMGRP_LINK);
> +	if (fd < 0) {
> +		rte_errno = errno;
> +		goto error;
> +	}
> +	ret = rte_nl_send(fd, &req.hdr);
> +	if (ret == -1) {
> +		rte_errno = errno;
> +		goto error;
> +	}
> +	ret = rte_nl_recv(fd, mlx5_vf_iface_idx_cb, &data);
> +	if (ret == -1) {
> +		rte_errno = errno;
> +		goto error;
> +	}
> +	rte_nl_final(fd);
> +	if (data.iface_idx == -1) {
> +		rte_errno = EAGAIN;
> +		goto error;
> +	}
> +	return data.iface_idx;
> +error:
> +	if (fd >= 0)
> +		rte_nl_final(fd);
> +	DRV_LOG(DEBUG, "port %u cannot retrieve Netlink Interface index %s",
> +		dev->data->port_id, strerror(rte_errno));
> +	return -rte_errno;
> +}
> -- 
> 2.11.0
> 

Due to possible MAC addresses collisions, I suggest a simpler approach:
replacing all this code with a combination of mlx5_get_ifname() and
if_nametoindex(). Thoughts?

-- 
Adrien Mazarguil
6WIND