[v2,2/2] net/failsafe: support multicast address list set
Checks
Commit Message
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
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?
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.
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.
@@ -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
@@ -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
-----------
@@ -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;
@@ -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)
@@ -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,
};
@@ -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. */