From mboxrd@z Thu Jan 1 00:00:00 1970 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) 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=AH5CWNfdcEVASEdmIvXmXcjvsf+eAx976/aUDUeUlIc=; b=AegdrG2FUzc9SPFkIxF4nKzihZzZgsn3CQpQsM+7IR0OTS2jYbb9dFD2R03RbL166a nJo88bsAmpVfeEzthPj3yWDYIPs1lusbV6iTnFfvdFJ0jPOCL2GkUYxsSbS2iW/vIbuI hdLaXVI7QypB+hNZADPCmR4tcrfnpxtoVPiYV/wUGjzeUbS8OSHpBvR/Zjx3VLpmVwhl VSiJZoDaTU0GGZjoepXz76R/K4XoRB8YeW2K0QAHHSI2oeRPI+T07Bre5zDfeZ+9OElE +w0EZEq74YSDEPTkQRVbLN7s0u63H0ZQWDKLOcjkRNMD80USBERnP/1D7/nLFbnCH66B OZNQ== 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=AH5CWNfdcEVASEdmIvXmXcjvsf+eAx976/aUDUeUlIc=; b=WatzZJJ8LOk0N3IaikFvPU4yw8LEggs+6fooTTDdFR83VEz+XcVW1XSav15LNRMMOa HHZ0ggAGwFNq3U5E9eZmsJUkOIipvue5f/u+F8mpbKDXOprFQPpZ9NPUR879UF66cWDE fYMCqM3ZddWo9YSSZ8JyvYF+q1C0M8afP9sCS+ivycnYsQcuofdXlqDXYxD5oezYd8Vx 5ui4khdZbrXZBo8fPnifVCvVU/iooWeFBhTrAg98uAtu4t9n5G0zMBMXbYqzukC6Zd3y 494Bu/KTl5gnYSS4yZ+X07/kvYh+yxUwqgQQWU/d2i6OIlXPyvT5/hfrlM1H7qN1XBQ3 zuiA== X-Gm-Message-State: ALQs6tDa+e0WcabVZQieqm7H4I/7lLdS8bUXeKct9pAYIKvmOp1RAsud 6Z8EM5AtrRdbksfyVjqZr3DcDQ== X-Google-Smtp-Source: AB8JxZp/7y9xqehgOUKGbe+nLVKXOiQSCCke6w9bqGtlpa2JLo+a+tPbSGezcBEbB1dBHyQ5v3t12A== X-Received: by 2002:adf:90cd:: with SMTP id i71-v6mr31428124wri.136.1525869699222; Wed, 09 May 2018 05:41:39 -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 33-v6sm21019076wrs.5.2018.05.09.05.41.38 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 09 May 2018 05:41:38 -0700 (PDT) Date: Wed, 9 May 2018 14:41:24 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Thomas Monjalon Cc: dev@dpdk.org, Matan Azrad , stable@dpdk.org 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-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180509094337.26112-11-thomas@monjalon.net> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH 10/11] net/failsafe: fix sub-device ownership race 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, 09 May 2018 12:41:39 -0000 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