From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Rybchenko Subject: Re: [PATCH v2 2/2] net/failsafe: support multicast address list set Date: Thu, 20 Sep 2018 14:46:16 +0300 Message-ID: References: <1535730814-24624-1-git-send-email-arybchenko@solarflare.com> <1535957722-17569-1-git-send-email-arybchenko@solarflare.com> <1535957722-17569-3-git-send-email-arybchenko@solarflare.com> <20180919145057.2nhxgalitzqvj5ha@bidouze.vm.6wind.com> <20180919151230.kl3jnlkbia2mgnsy@bidouze.vm.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Cc: , Evgeny Im , Ferruh Yigit , Thomas Monjalon To: =?UTF-8?Q?Ga=c3=abtan_Rivet?= Return-path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 0179E4CBD for ; Thu, 20 Sep 2018 13:47:07 +0200 (CEST) In-Reply-To: <20180919151230.kl3jnlkbia2mgnsy@bidouze.vm.6wind.com> Content-Language: en-GB List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >>> >>> Signed-off-by: Evgeny Im >>> Signed-off-by: Andrew Rybchenko >>> --- >>> 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.