diff mbox series

[v2,2/2] net/failsafe: support multicast address list set

Message ID 1535957722-17569-3-git-send-email-arybchenko@solarflare.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers show
Series net/failsafe: support multicast MAC address set | expand

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Andrew Rybchenko Sept. 3, 2018, 6:55 a.m. UTC
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(+)

Comments

Gaëtan Rivet Sept. 19, 2018, 2:50 p.m. UTC | #1
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 Sept. 19, 2018, 3:12 p.m. UTC | #2
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.

> 
> 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.
Andrew Rybchenko Sept. 20, 2018, 11:46 a.m. UTC | #3
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.
diff mbox series

Patch

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;
+		}
+	}
 	return 0;
 err_remove:
 	FOREACH_SUBDEV(sdev, i, dev)
diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index 24e91c931..082685bb1 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -952,6 +952,53 @@  fs_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 	return 0;
 }
 
+static int
+fs_set_mc_addr_list(struct rte_eth_dev *dev,
+		    struct ether_addr *mc_addr_set, uint32_t nb_mc_addr)
+{
+	struct sub_device *sdev;
+	uint8_t i;
+	int ret;
+
+	fs_lock(dev, 0);
+	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+		ret = rte_eth_dev_set_mc_addr_list(PORT_ID(sdev),
+						   mc_addr_set, nb_mc_addr);
+		if (ret != 0) {
+			ERROR("Operation rte_eth_dev_set_mc_addr_list failed for sub_device %d with error %d",
+			      i, ret);
+			goto rollback;
+		}
+	}
+	/* Do not reallocate/save if the method is called from sync */
+	if (mc_addr_set != PRIV(dev)->mcast_addrs) {
+		void *mcast_addrs = rte_realloc(PRIV(dev)->mcast_addrs,
+			nb_mc_addr * sizeof(PRIV(dev)->mcast_addrs[0]), 0);
+		if (mcast_addrs == NULL && nb_mc_addr > 0) {
+			ret = -ENOMEM;
+			goto rollback;
+		}
+		PRIV(dev)->mcast_addrs = mcast_addrs;
+		rte_memcpy(PRIV(dev)->mcast_addrs, mc_addr_set,
+			   nb_mc_addr * sizeof(PRIV(dev)->mcast_addrs[0]));
+	}
+	PRIV(dev)->nb_mcast_addr = nb_mc_addr;
+	fs_unlock(dev, 0);
+	return 0;
+
+rollback:
+	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+		int rc = rte_eth_dev_set_mc_addr_list(PORT_ID(sdev),
+			PRIV(dev)->mcast_addrs,	PRIV(dev)->nb_mcast_addr);
+		if (rc != 0) {
+			ERROR("Multicast MAC address list rollback for sub_device %d failed with error %d",
+			      i, rc);
+		}
+	}
+	fs_unlock(dev, 0);
+	return ret;
+}
+
 static int
 fs_rss_hash_update(struct rte_eth_dev *dev,
 			struct rte_eth_rss_conf *rss_conf)
@@ -1036,6 +1083,7 @@  const struct eth_dev_ops failsafe_ops = {
 	.mac_addr_remove = fs_mac_addr_remove,
 	.mac_addr_add = fs_mac_addr_add,
 	.mac_addr_set = fs_mac_addr_set,
+	.set_mc_addr_list = fs_set_mc_addr_list,
 	.rss_hash_update = fs_rss_hash_update,
 	.filter_ctrl = fs_filter_ctrl,
 };
diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
index 886af8616..abbe73e87 100644
--- a/drivers/net/failsafe/failsafe_private.h
+++ b/drivers/net/failsafe/failsafe_private.h
@@ -143,6 +143,8 @@  struct fs_priv {
 	uint32_t nb_mac_addr;
 	struct ether_addr mac_addrs[FAILSAFE_MAX_ETHADDR];
 	uint32_t mac_addr_pool[FAILSAFE_MAX_ETHADDR];
+	uint32_t nb_mcast_addr;
+	struct ether_addr *mcast_addrs;
 	/* current capabilities */
 	struct rte_eth_dev_info infos;
 	struct rte_eth_dev_owner my_owner; /* Unique owner. */