From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 31A8C4F90 for ; Wed, 21 Mar 2018 10:11:58 +0100 (CET) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Mar 2018 02:11:57 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,339,1517904000"; d="scan'208";a="184679074" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.18.87]) by orsmga004.jf.intel.com with SMTP; 21 Mar 2018 02:11:54 -0700 Received: by (sSMTP sendmail emulation); Wed, 21 Mar 2018 09:11:53 +0000 Date: Wed, 21 Mar 2018 09:11:53 +0000 From: Bruce Richardson To: Arnon Warshavsky Cc: thomas@monjalon.net, anatoly.burakov@intel.com, wenzhuo.lu@intel.com, declan.doherty@intel.com, jerin.jacob@caviumnetworks.com, ferruh.yigit@intel.com, dev@dpdk.org Message-ID: <20180321091153.GA17868@bricha3-MOBL.ger.corp.intel.com> References: <1521582788-32364-1-git-send-email-arnon@qwilt.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1521582788-32364-1-git-send-email-arnon@qwilt.com> Organization: Intel Research and Development Ireland Ltd. User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH v3] eal: replace rte_panic instances to return an error value 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, 21 Mar 2018 09:11:59 -0000 On Tue, Mar 20, 2018 at 11:53:08PM +0200, Arnon Warshavsky wrote: > The purpose of this patch is to cleanup the library code > from paths that end up aborting the process, > and move to checking error values, in order to allow the running process > perform an orderly tear-down or other mitigation of the event. > > This patch modifies the majority of rte_panic calls under lib and drivers, > and replaces them with a new variation of rte_panic macro > that does not abort and returns an error value > that can be propagated up the call stack. > > - Focus was given to the dpdk initialization path > - Some of the panic calls within drivers were left in place where > the call is from within an interrupt or calls that are on the data path, > where there is no simple applicative route to propagate > the error to termination. > These should be handled by the driver maintainers. > I would like to define a device health state that can be monitored from > the side,and this will be an independent patch. > - No change took place in example and test files > - No change took place for debug assertions calling panic > - Some previously void functions calling panic where changed > to return a value,with callers modified accordingly. > > An additional independent patch to devtools/checkpatches.sh > will be submitted in order to prevent new additions of calls to rte_panic > under lib and drivers. > > Keep calm and don't panic. > > --- > V2: Fix typos in commit message > --- > V3: More commit message formatting > > Signed-off-by: Arnon Warshavsky > --- > drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c | 2 +- > drivers/crypto/dpaa_sec/dpaa_sec.c | 2 +- > drivers/net/bonding/rte_eth_bond_8023ad.c | 23 +++++--- > drivers/net/bonding/rte_eth_bond_8023ad_private.h | 2 +- > drivers/net/bonding/rte_eth_bond_api.c | 20 +++++-- > drivers/net/bonding/rte_eth_bond_pmd.c | 5 +- > drivers/net/bonding/rte_eth_bond_private.h | 2 +- > drivers/net/e1000/e1000_ethdev.h | 2 +- > drivers/net/e1000/igb_ethdev.c | 3 +- > drivers/net/e1000/igb_pf.c | 10 ++-- > drivers/net/ixgbe/ixgbe_ethdev.c | 3 +- > drivers/net/ixgbe/ixgbe_ethdev.h | 2 +- > drivers/net/ixgbe/ixgbe_pf.c | 6 +- > lib/librte_eal/bsdapp/eal/eal.c | 54 ++++++++++------- > lib/librte_eal/bsdapp/eal/eal_debug.c | 14 +++++ > lib/librte_eal/bsdapp/eal/eal_thread.c | 43 +++++++++----- > lib/librte_eal/common/eal_common_memzone.c | 2 +- > lib/librte_eal/common/eal_thread.h | 4 +- > lib/librte_eal/common/include/rte_debug.h | 20 +++++++ > lib/librte_eal/common/rte_malloc.c | 6 +- > lib/librte_eal/linuxapp/eal/eal.c | 72 ++++++++++++++--------- > lib/librte_eal/linuxapp/eal/eal_debug.c | 14 +++++ > lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 19 ++++-- > lib/librte_eal/linuxapp/eal/eal_interrupts.c | 16 +++-- > lib/librte_eal/linuxapp/eal/eal_thread.c | 43 +++++++++----- > lib/librte_eal/rte_eal_version.map | 1 + > lib/librte_ether/rte_ethdev.c | 10 +++- > lib/librte_eventdev/rte_eventdev_pmd_pci.h | 2 +- > lib/librte_eventdev/rte_eventdev_pmd_vdev.h | 6 +- > lib/librte_kni/rte_kni.c | 18 ++++-- > lib/librte_kni/rte_kni_fifo.h | 6 +- > 31 files changed, 291 insertions(+), 141 deletions(-) > > diff --git a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c > index 9a790dd..9fda47c 100644 > --- a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c > +++ b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c > @@ -2865,7 +2865,7 @@ struct rte_security_ops dpaa2_sec_security_ops = { > rte_socket_id()); > > if (cryptodev->data->dev_private == NULL) > - rte_panic("Cannot allocate memzone for private " > + rte_panic_return("Cannot allocate memzone for private " > "device data"); > } To echo Thomas' previous comment, I'm not sure I like the addition of this new "panic_return" macro. It seems an artificial hack just to have a 1:1 replacement for rte_panic. I think I'd rather an explicit log and then return call. /Bruce