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...@oktetlabs.com>
Signed-off-by: Evgeny Im <evgeny...@oktetlabs.com>
Signed-off-by: Andrew Rybchenko <arybche...@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.
