Re: [dpdk-dev] [PATCH v2 2/2] net/failsafe: support multicast address list set

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

Hi,

Sorry about the delay on this, overall it looks ok;
I have an issue however, see inline.

On Mon, Sep 03, 2018 at 07:55:22AM +0100, Andrew Rybchenko wrote:
> From: Evgeny Im <Evgeny.Im@oktetlabs.com>
> 
> Signed-off-by: Evgeny Im <Evgeny.Im@oktetlabs.com>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  doc/guides/nics/features/failsafe.ini   |  1 +
>  doc/guides/rel_notes/release_18_11.rst  |  6 ++++
>  drivers/net/failsafe/failsafe.c         |  1 +
>  drivers/net/failsafe/failsafe_ether.c   | 17 +++++++++
>  drivers/net/failsafe/failsafe_ops.c     | 48 +++++++++++++++++++++++++
>  drivers/net/failsafe/failsafe_private.h |  2 ++
>  6 files changed, 75 insertions(+)
> 
> diff --git a/doc/guides/nics/features/failsafe.ini b/doc/guides/nics/features/failsafe.ini
> index 83cc99d19..39ee57965 100644
> --- a/doc/guides/nics/features/failsafe.ini
> +++ b/doc/guides/nics/features/failsafe.ini
> @@ -12,6 +12,7 @@ Jumbo frame          = Y
>  Promiscuous mode     = Y
>  Allmulticast mode    = Y
>  Unicast MAC filter   = Y
> +Multicast MAC filter = Y
>  VLAN filter          = Y
>  Flow control         = Y
>  Flow API             = Y
> diff --git a/doc/guides/rel_notes/release_18_11.rst b/doc/guides/rel_notes/release_18_11.rst
> index 24204e67b..54e0e4ee4 100644
> --- a/doc/guides/rel_notes/release_18_11.rst
> +++ b/doc/guides/rel_notes/release_18_11.rst
> @@ -54,6 +54,12 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =========================================================
>  
> +* **Updated failsafe driver.**
> +
> +  Updated the failsafe driver including the following changes:
> +
> +  * Support multicast MAC address set.
> +
>  
>  API Changes
>  -----------
> diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
> index 657919f93..c3999f026 100644
> --- a/drivers/net/failsafe/failsafe.c
> +++ b/drivers/net/failsafe/failsafe.c
> @@ -304,6 +304,7 @@ fs_rte_eth_free(const char *name)
>  	ret = pthread_mutex_destroy(&PRIV(dev)->hotplug_mutex);
>  	if (ret)
>  		ERROR("Error while destroying hotplug mutex");
> +	rte_free(PRIV(dev)->mcast_addrs);
>  	rte_free(PRIV(dev));
>  	rte_eth_dev_release_port(dev);
>  	return ret;
> diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
> index 5b5cb3b49..5078feabe 100644
> --- a/drivers/net/failsafe/failsafe_ether.c
> +++ b/drivers/net/failsafe/failsafe_ether.c
> @@ -424,6 +424,23 @@ failsafe_eth_dev_state_sync(struct rte_eth_dev *dev)
>  	ret = dev->dev_ops->dev_start(dev);
>  	if (ret)
>  		goto err_remove;
> +	/*
> +	 * Propagate multicast MAC addresses to sub-devices,
> +	 * if non zero number of addresses is set.
> +	 * The condition is required to avoid breakage of failsafe
> +	 * for sub-devices which do not support the operation
> +	 * if the feature is really not used.
> +	 */
> +	if (PRIV(dev)->nb_mcast_addr > 0) {
> +		ret = dev->dev_ops->set_mc_addr_list(dev,
> +						     PRIV(dev)->mcast_addrs,
> +						     PRIV(dev)->nb_mcast_addr);
> +		if (ret) {
> +			ERROR("Could not set list of multicast addresses to sub_device %d",
> +			      i);
> +			goto err_remove;
> +		}
> +	}

Using here the dev-ops instead of calling the rte_eth_* API as is done for the
other configuration items, is unorthodox and I believe could lead to
issues.

Why didn't you call rte_eth_dev_set_mc_addr_list on the new port only instead,
the same way it is done for the other configuration item?

Using the dev-ops, you are making the other sub-device re-apply the
same configuration periodically (in case of repeated hotplug error),
twice per sub-device upkeep cycle. This is unnecessary and seems to
foster instability for no clear benefit. Can you justify it?

-- 
Gaëtan Rivet
6WIND