From mboxrd@z Thu Jan 1 00:00:00 1970 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) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us3.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 3CB54980084; Thu, 20 Sep 2018 11:47:06 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Thu, 20 Sep 2018 12:47:00 +0100 To: =?UTF-8?Q?Ga=c3=abtan_Rivet?= CC: , Evgeny Im , Ferruh Yigit , Thomas Monjalon 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> From: Andrew Rybchenko Message-ID: Date: Thu, 20 Sep 2018 14:46:16 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: <20180919151230.kl3jnlkbia2mgnsy@bidouze.vm.6wind.com> Content-Language: en-GB X-Originating-IP: [91.220.146.112] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24106.003 X-TM-AS-Result: No-18.793000-8.000000-10 X-TMASE-MatchedRID: EMyCvCfVN1Gjk2p62FWROya1MaKuob8PC/ExpXrHizxjPrlNB+gMq72+ Pt89anuuxSDIrpWQxBG/pfzL3ymmCG94Ipa1otxooprTEHvewABaNaxZBRbNWmecrqZc3vabpRr MkaeNIWMWZdx9eq2z0f0GGkeErTDamEmUvLx0ItgjRwcsjqWGAjFcf92WG8u/d71AOvz4tNwVN8 HXIE+U8pBfcxBP5xWexiJX9rHQsRrMc5yqMOfs1hWCVBr+Ay9806VWvQpsdffvmIIuT2MkFfbC7 4s+hpdXO3jRCVojzWbu13W3DuiJ9AAt/Fsc5wFGF6z9HGHKwNsdQSyPGb6mCAsIL6sgb35lhCYQ 0GRRnBPAUdSEDHdAUD9Vgel7ZLgqQw3ojRObXh3Yd2+/8wYTde/PojcyF5jzArMcoUfMuwmVQ2b E9K2Fgp4wl819njyx4YS6FyG8vyjyUQNiagGSs9sfxZpQv2qMm/y00tE9Sta/wz3p7pLVvSxZV2 XdhwOw7fNKgmEEsE1qlnm6jwp3Dzhzk989ZlO2hDK4kXfgEbocDDLReGt4PfmUDxpFogQXo8WMk QWv6iWhMIDkR/KfwCIQ5mZ5SqHPASN+zO7NTmu6oqI4mKr0b8En4optfuHOd5lqN2dDwN1CTKzw +LfOd3c4CjpNg7sNqPmCHkt84tnGFZtyof2PLsKGFAYIA+LvlExlQIQeRG0= X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--18.793000-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24106.003 X-MDID: 1537444027-EBJSsF7cslNf Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v2 2/2] net/failsafe: support multicast address list set X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 Sep 2018 11:47:08 -0000 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.