From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by dpdk.org (Postfix) with ESMTP id 5EDBF326C for ; Wed, 19 Sep 2018 16:51:16 +0200 (CEST) Received: by mail-wr1-f68.google.com with SMTP id g33-v6so6143262wrd.1 for ; Wed, 19 Sep 2018 07:51:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=c9v3E22N9GmbK77WXhVO9RGWDaDzZoNbWIXKJc684pk=; b=Phk9Ed+o8UgB5YtTRRJcpP8iZp0tIjV/pQdoFvjTdA+ktBvKvtTxxQE8mS0AuXrvFT d4avU8HOjmh88ioR3ZRazQocmTtROuLLDlm6E7QZA5kN6uTqlqzVW3eeLDCpZw193Rus zbDYzoxyBfWjJ4FbCkRlg2jIUVt9VSA3+vErFrFKXR7NydTV7AitwgafHvHk49TOk9f6 SznECaEGVXhDAIAQvG9o9enwyquIg+kN8AuJhVHaIacC6LvGbP6G/kHzJ5Pcv5z3vlxg WYKQqIgezCYz6ZmIAqg2cQ8d4xgyKQydOAnYELUp6VyUxekFhLhId4IspRuI+EBUct2T QaIw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=c9v3E22N9GmbK77WXhVO9RGWDaDzZoNbWIXKJc684pk=; b=YFacyo8FPw/A6QTWPwqX+dGjIrPs+C8AvgPMYgkDOqpeBzSJ4QTNRXHwT5DttyONuY CsNLCSh3e0zoAHWfUQZ+AnbBvGeVY8s4lm1rvJGStvi58pcaW5kfjX7aFaU+T1mONA6s xbe9KtrC+ugsMrtCIlpkGT8m4kvl4nBSg2U590W7sju6TC0w2pNSQqIVlzwbzIq1AODu XCBqDFOT02YDWFCT2JpVuDLi9k0RGKegFeBhIjSd/y9nRAa/uMYEaCScgFoQW8c03fxY pJ5zFWigwmzGzt/zVGOcBK05A4SWIfBsv4jcjeQhN3EaF9SLjxehWXD40CwrtXFRfbBZ ijBQ== X-Gm-Message-State: APzg51DHHW9nc0bkVWIE2sN/80c7+dnfolIgMvh1JxTPK2JxnoG3lEUC ze6mhbca3jz3WBNT3AtI9WlcOA== X-Google-Smtp-Source: ANB0VdZMWC0aSaAR6K8l/t9rpHIxchCmaXKA2+hfYZ0d787SFe6UkyvS7Z1XseyvYvts94JNCyBwVQ== X-Received: by 2002:a5d:428a:: with SMTP id k10-v6mr30436211wrq.225.1537368675583; Wed, 19 Sep 2018 07:51:15 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 132-v6sm5694971wmd.13.2018.09.19.07.51.14 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 19 Sep 2018 07:51:14 -0700 (PDT) Date: Wed, 19 Sep 2018 16:50:57 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Andrew Rybchenko Cc: dev@dpdk.org, Evgeny Im Message-ID: <20180919145057.2nhxgalitzqvj5ha@bidouze.vm.6wind.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1535957722-17569-3-git-send-email-arybchenko@solarflare.com> User-Agent: NeoMutt/20170113 (1.7.2) 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: Wed, 19 Sep 2018 14:51:16 -0000 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. 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 6WIND