From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id DD7801B1DE for ; Fri, 19 Jan 2018 17:19:32 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Jan 2018 08:19:31 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,381,1511856000"; d="scan'208";a="11542876" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.48]) ([10.237.220.48]) by fmsmga007.fm.intel.com with ESMTP; 19 Jan 2018 08:19:28 -0800 To: Matan Azrad , Adrien Mazarguil , Gaetan Rivet Cc: Thomas Monjalon , "dev@dpdk.org" , Andrew Rybchenko , "Ananyev, Konstantin" , Alejandro Lucero , Jerin Jacob , Hemant Agrawal , Shahaf Shuler , Adrien Mazarguil , Olivier MATZ , "Zhang, Helin" References: <1516220357-13013-1-git-send-email-matan@mellanox.com> <1516274834-19755-1-git-send-email-matan@mellanox.com> <1516274834-19755-5-git-send-email-matan@mellanox.com> <03662190-8477-b218-21f7-ecfb806b5997@intel.com> From: Ferruh Yigit Message-ID: Date: Fri, 19 Jan 2018 16:19:27 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v6 4/6] ethdev: adjust APIs removal error report 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: Fri, 19 Jan 2018 16:19:33 -0000 On 1/18/2018 6:10 PM, Matan Azrad wrote: > Hi Ferruh > > From: Ferruh Yigit, Thursday, January 18, 2018 7:31 PM >> On 1/18/2018 11:27 AM, Matan Azrad wrote: >>> rte_eth_dev_is_removed API was added to detect a device removal >>> synchronously. >>> >>> When a device removal occurs during control command execution, many >>> different errors can be reported to the user. >>> >>> Adjust all ethdev APIs error reports to return -EIO in case of device >>> removal using rte_eth_dev_is_removed API. >>> >>> Signed-off-by: Matan Azrad >>> Acked-by: Thomas Monjalon >>> --- >>> lib/librte_ether/rte_ethdev.c | 192 >>> +++++++++++++++++++++++++++--------------- >>> lib/librte_ether/rte_ethdev.h | 51 ++++++++++- >>> 2 files changed, 170 insertions(+), 73 deletions(-) >>> >>> diff --git a/lib/librte_ether/rte_ethdev.c >>> b/lib/librte_ether/rte_ethdev.c index c93cec1..7044159 100644 >>> --- a/lib/librte_ether/rte_ethdev.c >>> +++ b/lib/librte_ether/rte_ethdev.c >>> @@ -338,6 +338,16 @@ struct rte_eth_dev * >>> return -ENODEV; >>> } >>> >>> +static int >>> +eth_err(uint16_t port_id, int ret) >>> +{ >>> + if (ret == 0) >>> + return 0; >>> + if (rte_eth_dev_is_removed(port_id)) >>> + return -EIO; >>> + return ret; >>> +} >>> + >>> /* attach the new device, then store port_id of the device */ int >>> rte_eth_dev_attach(const char *devargs, uint16_t *port_id) @@ -492,7 >>> +502,8 @@ struct rte_eth_dev * >>> return 0; >>> } >>> >>> - return dev->dev_ops->rx_queue_start(dev, rx_queue_id); >>> + return eth_err(port_id, dev->dev_ops->rx_queue_start(dev, >>> + rx_queue_id)); >>> >>> } >> >> This patch updates *all* ethdev public APIs to add if device is removed >> check? > > Yes. > >> And each check goes to ethdev is_removed() dev_ops to ask if dev is >> removed. > Probably, if the REMOVED state setted in will not call device is_remove. > >> These must be better way of doing this, am I missing something. > > Suggest. With a silly analogy, this is like a blind person asking each time if he is dead before talking to other person. At first glance I can think of a kind of watchdog timer can be implemented in ethdev layer. It provides periodic checks and if device is dead it calls the registered user callback function. This method presented as synchronous method but not triggered from side where event happens, I mean not triggered from PMD but from application. So does application doing polling continuously if device is dead? Or if application is relying this patch to add a check in each API, what happens if device removed during data processing, will app rely on asynchronous method? I am including a few consumers of the ethdev to the mail thread, clearly I am not very supportive of this patch, but specially taking release is being close to the account, if there is no objection than me I will take as consensus to get the patch in. > > This code will replace similar code in each PMD. > >> I definitely would like to see more comments for this patch. >> >> Another question is what happens if device removed while or before >> dev_ops called? There is no synchronizations in drivers for removal, right? >> > > Yes. You right, the device removal can be changed a moment after the call. > Actually the caller suspected in removal before call it(and want to validate it) - so it makes sense. > From this reason the check in ethdev APIs is called generally in error flows. > > >> <...>