From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet Subject: Re: [PATCH 10/11] net/failsafe: fix sub-device ownership race Date: Wed, 9 May 2018 14:41:24 +0200 Message-ID: <20180509124123.un67tmsh75kxwrir@bidouze.vm.6wind.com> References: <20180509094337.26112-1-thomas@monjalon.net> <20180509094337.26112-11-thomas@monjalon.net> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org, Matan Azrad , stable@dpdk.org To: Thomas Monjalon Return-path: Received: from mail-wr0-f193.google.com (mail-wr0-f193.google.com [209.85.128.193]) by dpdk.org (Postfix) with ESMTP id 978A8A49B for ; Wed, 9 May 2018 14:41:39 +0200 (CEST) Received: by mail-wr0-f193.google.com with SMTP id v60-v6so35498561wrc.7 for ; Wed, 09 May 2018 05:41:39 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20180509094337.26112-11-thomas@monjalon.net> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hello Matan, Two nitpicks below: On Wed, May 09, 2018 at 11:43:36AM +0200, Thomas Monjalon wrote: > From: Matan Azrad > > There is time between the sub-device port probing by the sub-device PMD > to the sub-device port ownership taking by a fail-safe port. > > In this time, the port is available for the application usage. For > example, the port will be exposed to the applications which use > RTE_ETH_FOREACH_DEV iterator. > > Thus, ownership unaware applications may manage the port in this time > what may cause a lot of problematic behaviors in the fail-safe > sub-device initialization. > > Register to the ethdev NEW event to take the sub-device port ownership > before it becomes exposed to the application. > > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD") This fix is relying on the RTE_ETH_EVENT_NEW, an API that I think is not meant to be backported in the stable release that would be targetted by this commit id. I think this fix is useless without the rest of this series, so I don't know what is exactly planned about the rest (whether it is backported, and where), but I would only CC stable if this is planned, and only as soon as the relevant APIs are introduced. > Cc: stable@dpdk.org > > Signed-off-by: Matan Azrad > --- > drivers/net/failsafe/failsafe.c | 22 ++++++++++--- > drivers/net/failsafe/failsafe_eal.c | 58 +++++++++++++++++++++------------ > drivers/net/failsafe/failsafe_ether.c | 23 +++++++++++++ > drivers/net/failsafe/failsafe_private.h | 4 +++ > 4 files changed, 83 insertions(+), 24 deletions(-) > > diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c > index fc989c4f5..c9d128de3 100644 > --- a/drivers/net/failsafe/failsafe.c > +++ b/drivers/net/failsafe/failsafe.c > @@ -204,16 +204,25 @@ fs_eth_dev_create(struct rte_vdev_device *vdev) > } > snprintf(priv->my_owner.name, sizeof(priv->my_owner.name), > FAILSAFE_OWNER_NAME); > + DEBUG("Failsafe port %u owner info: %s_%016"PRIX64, dev->data->port_id, > + priv->my_owner.name, priv->my_owner.id); > + ret = rte_eth_dev_callback_register(RTE_ETH_ALL, RTE_ETH_EVENT_NEW, > + failsafe_eth_new_event_callback, > + dev); > + if (ret) { > + ERROR("Failed to register NEW callback"); > + goto free_args; > + } > ret = failsafe_eal_init(dev); > if (ret) > - goto free_args; > + goto unregister_new_callback; > ret = fs_mutex_init(priv); > if (ret) > - goto free_args; > + goto unregister_new_callback; > ret = failsafe_hotplug_alarm_install(dev); > if (ret) { > ERROR("Could not set up plug-in event detection"); > - goto free_args; > + goto unregister_new_callback; > } > mac = &dev->data->mac_addrs[0]; > if (mac_from_arg) { > @@ -226,7 +235,7 @@ fs_eth_dev_create(struct rte_vdev_device *vdev) > mac); > if (ret) { > ERROR("Failed to set default MAC address"); > - goto free_args; > + goto unregister_new_callback; > } > } > } else { > @@ -261,6 +270,9 @@ fs_eth_dev_create(struct rte_vdev_device *vdev) > }; > rte_eth_dev_probing_finish(dev); > return 0; > +unregister_new_callback: > + rte_eth_dev_callback_unregister(RTE_ETH_ALL, RTE_ETH_EVENT_NEW, > + failsafe_eth_new_event_callback, dev); > free_args: > failsafe_args_free(dev); > free_subs: > @@ -280,6 +292,8 @@ fs_rte_eth_free(const char *name) > dev = rte_eth_dev_allocated(name); > if (dev == NULL) > return -ENODEV; > + rte_eth_dev_callback_unregister(RTE_ETH_ALL, RTE_ETH_EVENT_NEW, > + failsafe_eth_new_event_callback, dev); > ret = failsafe_eal_uninit(dev); > if (ret) > ERROR("Error while uninitializing sub-EAL"); > diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c > index ce767703f..8f1b9d845 100644 > --- a/drivers/net/failsafe/failsafe_eal.c > +++ b/drivers/net/failsafe/failsafe_eal.c > @@ -10,7 +10,7 @@ > static int > fs_ethdev_portid_get(const char *name, uint16_t *port_id) > { > - uint16_t pid; > + uint32_t pid; I do not see why the port_id is made uint32_t? Is there a reason? Otherwise all seems fine. With the proper justification or with uin16_t pid, and with a second pass on the backport tagging, Acked-by: Gaetan Rivet Thanks, -- Gaëtan Rivet 6WIND