From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by dpdk.org (Postfix) with ESMTP id 0B7C94C93 for ; Wed, 19 Sep 2018 17:12:48 +0200 (CEST) Received: by mail-wr1-f67.google.com with SMTP id v17-v6so6194977wrr.9 for ; Wed, 19 Sep 2018 08:12:48 -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=eidxUEfdhHU/eTN1/k6pTvoQ+oiBnoUx086azsmd5bg=; b=IBZ8KVE48oRGzfr4mol+4Tr4tWSGJYijN6rsAhTa2SRHy8wLTqxZK7uS0b1NMuyVyP 8vFHLRfeTnEtg0K4vuk10u7tugNTjKHHJHI/KKV5KEul6G9jBWDhQRuJDT+KK+wqG3PW ybzqa2wTJFYbmsXn/SFIxx9t8tMiVTQhpplajP1l4vj/zUNvejkbk/whcecZuwx763Wv jrk0qvvmGFts/kPa3J+4SVi2FyCp/klg9V+S27gD4E9LCDbKPUyrs3HgdJUadRz7g15A NksSRA/yvc8A/+meHKNreFY5qoU5Mbz3AsrhMIkLF2sp83Ra1hf9/3s3gbZSanV06Ksc IOtg== 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=eidxUEfdhHU/eTN1/k6pTvoQ+oiBnoUx086azsmd5bg=; b=PKBpKJJ+acer8b4Hj4LEPMOPtxa3xTpZuS+ueoRDGjE3PWg/YQpHhGMhpQe8snijGM OuezRvboNzQzvQ54+3h/S9RTdoTrDl6aypa6OvObpcXzVylSprNrdvQnKijoizjX3ioQ LHRZ+M06/kz+d1de7ACxStGrFhdKjZ90OuHOiG7dNmBbmEdL7amKEJx2/WxqcYnMrqWJ iHIxE+BHAye47vX1OhfQj1ti+oPOC6wtytVDQQAkKF3CREKXuHaC+48F75NE9ysFq3vw ynTDPHmveFbUJUDQjKxQhaS+a2RrZQa+xE6nIDhWdbk0lDEgZxwiAmFZNLHyMnrK3Hcf 74ww== X-Gm-Message-State: APzg51BfhquzzRsnWZ+gwUfTIh5Tzz+6HEKvx7uHkIjxZCDvjyiA17XJ k1oLiLykPvW50uvLd3VEzmOncw== X-Google-Smtp-Source: ANB0VdbGtOEK/N6aMLjFH0M9qS/rHNJCVVXkxvCPvSlyVyfio/yRzE+F3qCHXcTJD3agvnfddx+7aA== X-Received: by 2002:adf:93c2:: with SMTP id 60-v6mr26641497wrp.81.1537369968504; Wed, 19 Sep 2018 08:12:48 -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 h10-v6sm7072966wmf.44.2018.09.19.08.12.47 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 19 Sep 2018 08:12:47 -0700 (PDT) Date: Wed, 19 Sep 2018 17:12:30 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Andrew Rybchenko Cc: dev@dpdk.org, Evgeny Im Message-ID: <20180919151230.kl3jnlkbia2mgnsy@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> <20180919145057.2nhxgalitzqvj5ha@bidouze.vm.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180919145057.2nhxgalitzqvj5ha@bidouze.vm.6wind.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 15:12:49 -0000 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. > > 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. -- Gaëtan Rivet 6WIND