Re: [PATCH 5/5] net/mlx5: add a parameter for Netlink support in VF

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

On Tue, Mar 13, 2018 at 01:50:39PM +0100, Nelio Laranjeiro wrote:
> All Netlink request the PMD will do can also be done by a iproute2 command
> line interface, letting the operator configure the VF behavior without
> having to modify the application nor reaching PMD limits (e.g. MAC
> address number limit).
> 
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>

Some nits, please see below.

> ---
>  doc/guides/nics/mlx5.rst   | 13 ++++++++++++
>  drivers/net/mlx5/mlx5.c    |  7 ++++++
>  drivers/net/mlx5/mlx5.h    |  1 +
>  drivers/net/mlx5/mlx5_vf.c | 53 ++++++++++++++++++++++++++++++++++++++++++++--
>  4 files changed, 72 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
> index 46d26e4c8..54e6f327b 100644
> --- a/doc/guides/nics/mlx5.rst
> +++ b/doc/guides/nics/mlx5.rst
> @@ -135,6 +135,11 @@ Limitations
>  - Flows with a VXLAN Network Identifier equal (or ends to be equal)
>    to 0 are not supported.
>  - VXLAN TSO and checksum offloads are not supported on VM.
> +- VF: Flow rules matching a specific MAC address are only triggers if the MAC
> +  has been previously added by the application.

Rewording suggestion:

 Flow rules created on VF devices can only match traffic targeted at the
 configured MAC addresses (see ``rte_eth_dev_mac_addr_add()``).

> +  Adding more than MLX5_MAX_MAC_ADDRESSES MAC addresses is not supported in VF
> +  mode.  **Note** needs a Linux kernel v4.16 or higher or Mellanox OFED
> +  installed.

I think this last bit is kind of redundant since applications already can't
add more MAC addresses than what a PMD allows. It's not a new limitation.

>  
>  Statistics
>  ----------
> @@ -335,6 +340,14 @@ Run-time configuration
>  
>    Enabled by default.
>  
> +- ``vf_nl_en`` parameter [int]
> +
> +  A nonzero value enables Netlink requests from the VF to add/remove MAC
> +  addresses or/and enable/disable promiscuous/allmulticast on the Netdevice.
> +  Otherwise the according configuration must be run with Linux iproute2 tools.

You might add that it's a necessary prerequisite to receive the traffic in
question.

> +
> +  Enabled by default on VF.

=> VF devices.

> +
>  Prerequisites
>  -------------
>  
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index e966884bd..d1ca0837c 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -68,6 +68,9 @@
>  /* Device parameter to enable hardware Rx vector. */
>  #define MLX5_RX_VEC_EN "rx_vec_en"
>  
> +/* Activate Netlink support in VF mode. */
> +#define MLX5_VF_NL_EN "vf_nl_en"
> +
>  #ifndef HAVE_IBV_MLX5_MOD_MPW
>  #define MLX5DV_CONTEXT_FLAGS_MPW_ALLOWED (1 << 2)
>  #define MLX5DV_CONTEXT_FLAGS_ENHANCED_MPW (1 << 3)
> @@ -414,6 +417,8 @@ mlx5_args_check(const char *key, const char *val, void *opaque)
>  		config->tx_vec_en = !!tmp;
>  	} else if (strcmp(MLX5_RX_VEC_EN, key) == 0) {
>  		config->rx_vec_en = !!tmp;
> +	} else if (strcmp(MLX5_VF_NL_EN, key) == 0) {
> +		config->vf_nl_en = !!tmp;
>  	} else {
>  		DRV_LOG(WARNING, "%s: unknown parameter", key);
>  		rte_errno = EINVAL;
> @@ -445,6 +450,7 @@ mlx5_args(struct mlx5_dev_config *config, struct rte_devargs *devargs)
>  		MLX5_TXQ_MAX_INLINE_LEN,
>  		MLX5_TX_VEC_EN,
>  		MLX5_RX_VEC_EN,
> +		MLX5_VF_NL_EN,
>  		NULL,
>  	};
>  	struct rte_kvargs *kvlist;
> @@ -750,6 +756,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>  			.txq_inline = MLX5_ARG_UNSET,
>  			.txqs_inline = MLX5_ARG_UNSET,
>  			.inline_max_packet_sz = MLX5_ARG_UNSET,
> +			.vf_nl_en = 1,
>  		};
>  
>  		len = snprintf(name, sizeof(name), PCI_PRI_FMT,
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index 245235641..8c3e925af 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -88,6 +88,7 @@ struct mlx5_dev_config {
>  	unsigned int tx_vec_en:1; /* Tx vector is enabled. */
>  	unsigned int rx_vec_en:1; /* Rx vector is enabled. */
>  	unsigned int mpw_hdr_dseg:1; /* Enable DSEGs in the title WQEBB. */
> +	unsigned int vf_nl_en:1; /* Enable Netlink request in VF mode. */

request => requests

>  	unsigned int tso_max_payload_sz; /* Maximum TCP payload for TSO. */
>  	unsigned int ind_table_max_size; /* Maximum indirection table size. */
>  	int txq_inline; /* Maximum packet size for inlining. */
> diff --git a/drivers/net/mlx5/mlx5_vf.c b/drivers/net/mlx5/mlx5_vf.c
> index cf71e79d9..18b02365d 100644
> --- a/drivers/net/mlx5/mlx5_vf.c
> +++ b/drivers/net/mlx5/mlx5_vf.c
> @@ -361,7 +361,16 @@ mlx5_vf_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac)
>  	int i;
>  	int mac_index = MLX5_MAX_MAC_ADDRESSES;
>  	int ret;
> +	struct priv *priv = dev->data->dev_private;
>  
> +	if (!priv->config.vf_nl_en) {
> +		DRV_LOG(WARNING,
> +			"port %u Netlink requests are disabled, add the MAC"
> +			" though bridge command: bridge fdb add <mac> dev"
> +			" <device>",
> +			dev->data->port_id);

Log level should be INFO (even "PROTIP" if such a thing existed :)

Also I'm not sure it should be done for each call, printing it once during
startup should be enough.

> +		return 0;
> +	}
>  	ret = mlx5_vf_mac_addr_list(dev, &macs, &macs_n);
>  	if (ret)
>  		return ret;
> @@ -399,7 +408,16 @@ mlx5_vf_mac_addr_remove(struct rte_eth_dev *dev, struct ether_addr *mac)
>  	int i;
>  	int mac_index = MLX5_MAX_MAC_ADDRESSES;
>  	int ret;
> +	struct priv *priv = dev->data->dev_private;
>  
> +	if (!priv->config.vf_nl_en) {
> +		DRV_LOG(WARNING,
> +			"port %u Netlink requests are disabled, delete the MAC"
> +			" though bridge command: bridge fdb del <mac> dev"
> +			" <device>",
> +			dev->data->port_id);

Same here.

> +		return 0;
> +	}
>  	ret = mlx5_vf_mac_addr_list(dev, &macs, &macs_n);
>  	if (ret)
>  		return ret;
> @@ -432,7 +450,16 @@ mlx5_vf_mac_addr_flush(struct rte_eth_dev *dev)
>  {
>  	int i;
>  	const struct ether_addr mac_null = { .addr_bytes = { 0 }, };
> +	struct priv *priv = dev->data->dev_private;
>  
> +	if (!priv->config.vf_nl_en) {
> +		DRV_LOG(WARNING,
> +			"port %u Netlink requests are disabled, flush added"
> +			" MAC though bridge command: bridge fdb del <mac> dev"
> +			" <device>",
> +			dev->data->port_id);

Ditto.

> +		return 0;
> +	}
>  	/* Skip the default mac at index 0. */
>  	for (i = 1; i != MLX5_MAX_MAC_ADDRESSES; ++i) {
>  		struct ether_addr *m = &dev->data->mac_addrs[i];
> @@ -516,8 +543,19 @@ mlx5_vf_device_flags(struct rte_eth_dev *dev, uint32_t flags, int enable)
>  int
>  mlx5_vf_promisc(struct rte_eth_dev *dev, int enable)
>  {
> -	int ret = mlx5_vf_device_flags(dev, IFF_PROMISC, enable);
> +	int ret;
> +	struct priv *priv = dev->data->dev_private;
>  
> +	if (!priv->config.vf_nl_en) {
> +		DRV_LOG(WARNING,
> +			"port %u Netlink requests are disabled, %s promisc"
> +			" though ip link command: ip link set <device> promisc"
> +			" %s",
> +			dev->data->port_id, enable ? "enable" : "disable",
> +			enable ? "on" : "off");

Ditto.

> +		return 0;
> +	}
> +	ret = mlx5_vf_device_flags(dev, IFF_PROMISC, enable);
>  	if (ret)
>  		DRV_LOG(DEBUG,
>  			"port %u cannot %s promisc mode: Netlink error %s",
> @@ -540,8 +578,19 @@ mlx5_vf_promisc(struct rte_eth_dev *dev, int enable)
>  int
>  mlx5_vf_allmulti(struct rte_eth_dev *dev, int enable)
>  {
> -	int ret = mlx5_vf_device_flags(dev, IFF_ALLMULTI, enable);
> +	int ret;
> +	struct priv *priv = dev->data->dev_private;
>  
> +	if (!priv->config.vf_nl_en) {
> +		DRV_LOG(WARNING,
> +			"port %u Netlink requests are disabled, %s allmulti"
> +			" though ip link command: ip link set <device>"
> +			" allmulticast %s",
> +			dev->data->port_id, enable ? "enable" : "disable",
> +			enable ? "on" : "off");

Ditto.

> +		return 0;
> +	}
> +	ret = mlx5_vf_device_flags(dev, IFF_ALLMULTI, enable);
>  	if (ret)
>  		DRV_LOG(DEBUG,
>  			"port %u cannot %s allmulti mode: Netlink error %s",
> -- 
> 2.11.0
> 

-- 
Adrien Mazarguil
6WIND