| 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 |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/Intel-compilation | success | Compilation OK |
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.
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. */