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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 9/19/18 6:12 PM, Gaëtan Rivet wrote:
> On Wed, Sep 19, 2018 at 04:50:57PM +0200, Gaëtan Rivet wrote:
>> 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.
> Sorry I forgot the mention it, but it seems that this could be done
> in fs_eth_dev_conf_apply() instead, which explains why I would consider
> using the dev-ops being unorthodox.

Yes, I've overlooked it on my review before submission.

>> 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?
>>
> If it was necessary to call this after the dev_start, I think it
> would be better to restrict the configuration to inactive sub-devices,
> in any case.

In theory, multicast addresses list is not listed in configuration items
retained across stop/start, so, could be wrong to set before start.
I hope it is just incomplete documentation in ethdev and we should
just fix it.

Thanks a lot for review,
Andrew.