[dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Arnon Warshavsky]

The purpose of this patch series 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 teardown or other mitigation of the event.

This patch modifies the majority of rte_panic calls
under lib and drivers, and replaces them with a log message
and an error return code according to context,
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 temination.
  These should be handled by the driver maintainers..
- local void functions with no api were changed to retrun a value
  where needed
- No change took place in example and test files
- No change took place for debug assertions calling panic
- A new function was added to devtools/checkpatches.sh
  in order to prevent new additions of calls to rte_panic
  under lib and drivers.

Keep calm and don't panic

---

v2:
- reformat error messages so that literal string are in the same line
- fix typo in commit message
- add new return code to doxigen of rte_memzone_free()

v3:
- submit  all 13 patches changed and unchanged in the same patchset

v4:
- remove 2 patches that are no more relevant
- fix split literal string in error message
- change return value -1 to enum
- split value and success code in a static function

v5:
- reword commit messages
- revert thread related instances back to panicing
- handle file descriptors with state to reset after eal init failure
  in case re initialization takes place

v6:
- Use pmd log macro rather than rte_log



Arnon Warshavsky (11):
  crypto/dpaa: replace rte_panic instances in crypto/dpaa driver
  bond: replace rte_panic instances in bonding driver
  e1000: replace rte_panic instances in e1000 driver
  ixgbe: replace rte_panic instances in ixgbe driver
  eal: replace rte_panic instances in eventdev
  kni: replace rte_panic instances in kni
  eal: replace rte_panic instances in hugepage_info
  eal: replace rte_panic instances in interrupts thread
  eal: replace rte_panic instances in ethdev
  eal: replace rte_panic instances in init sequence
  devtools: prevent new instances of rte_panic and rte_exit

 devtools/checkpatches.sh                          | 95 +++++++++++++++++++++-
 drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c       |  8 +-
 drivers/crypto/dpaa_sec/dpaa_sec.c                |  8 +-
 drivers/net/bonding/rte_eth_bond_8023ad.c         | 29 ++++---
 drivers/net/bonding/rte_eth_bond_8023ad_private.h |  2 +-
 drivers/net/bonding/rte_eth_bond_api.c            | 22 +++--
 drivers/net/bonding/rte_eth_bond_pmd.c            |  9 ++-
 drivers/net/bonding/rte_eth_bond_private.h        |  2 +-
 drivers/net/e1000/e1000_ethdev.h                  |  2 +-
 drivers/net/e1000/igb_ethdev.c                    |  4 +-
 drivers/net/e1000/igb_pf.c                        | 15 ++--
 drivers/net/ixgbe/ixgbe_ethdev.c                  |  6 +-
 drivers/net/ixgbe/ixgbe_ethdev.h                  |  2 +-
 drivers/net/ixgbe/ixgbe_pf.c                      | 15 ++--
 lib/librte_eal/bsdapp/eal/eal.c                   | 70 +++++++++++-----
 lib/librte_eal/linuxapp/eal/eal.c                 | 97 +++++++++++++++--------
 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c   | 37 ++++++---
 lib/librte_eal/linuxapp/eal/eal_interrupts.c      | 25 ++++--
 lib/librte_ether/rte_ethdev.c                     | 42 +++++++---
 lib/librte_ether/rte_ethdev.h                     |  4 +-
 lib/librte_eventdev/rte_eventdev_pmd_pci.h        |  8 +-
 lib/librte_eventdev/rte_eventdev_pmd_vdev.h       |  8 +-
 lib/librte_kni/rte_kni.c                          | 18 +++--
 lib/librte_kni/rte_kni_fifo.h                     | 11 ++-
 24 files changed, 396 insertions(+), 143 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH v6 01/11] crypto/dpaa: replace rte_panic instances in crypto/dpaa driver

[Arnon Warshavsky]

replace panic calls with log and return value.

Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
---
 drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c | 8 +++++---
 drivers/crypto/dpaa_sec/dpaa_sec.c          | 8 +++++---
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
index 58cbce8..75c50a5 100644
--- a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
+++ b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
@@ -2883,9 +2883,11 @@ struct rte_security_ops dpaa2_sec_security_ops = {
 					RTE_CACHE_LINE_SIZE,
 					rte_socket_id());
 
-		if (cryptodev->data->dev_private == NULL)
-			rte_panic("Cannot allocate memzone for private "
-				  "device data");
+		if (cryptodev->data->dev_private == NULL) {
+			RTE_LOG(ERR, PMD, "%s() Cannot allocate memzone for private device data",
+			__func__);
+			return -ENOMEM;
+		}
 	}
 
 	dpaa2_dev->cryptodev = cryptodev;
diff --git a/drivers/crypto/dpaa_sec/dpaa_sec.c b/drivers/crypto/dpaa_sec/dpaa_sec.c
index e456fd5..025f844 100644
--- a/drivers/crypto/dpaa_sec/dpaa_sec.c
+++ b/drivers/crypto/dpaa_sec/dpaa_sec.c
@@ -2384,9 +2384,11 @@ struct rte_security_ops dpaa_sec_security_ops = {
 					RTE_CACHE_LINE_SIZE,
 					rte_socket_id());
 
-		if (cryptodev->data->dev_private == NULL)
-			rte_panic("Cannot allocate memzone for private "
-					"device data");
+		if (cryptodev->data->dev_private == NULL) {
+			RTE_LOG(ERR, PMD, "%s() Cannot allocate memzone for private device data",
+			__func__);
+			return -ENOMEM;
+		}
 	}
 
 	dpaa_dev->crypto_dev = cryptodev;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v6 01/11] crypto/dpaa: replace rte_panic instances in crypto/dpaa driver

[Stephen Hemminger]

On Tue, 24 Apr 2018 09:41:53 +0300
Arnon Warshavsky <arnon@qwilt.com> wrote:

> +		if (cryptodev->data->dev_private == NULL) {
> +			RTE_LOG(ERR, PMD, "%s() Cannot allocate memzone for private device data",
> +			__func__);

dpaa2_sec is already doing private log type, via DPAA2_SEC_LOG macro.

You should go through your patch and make sure there are as few direct calls
to RTE_LOG as possible.

Re: [dpdk-dev] [PATCH v6 01/11] crypto/dpaa: replace rte_panic instances in crypto/dpaa driver

[Arnon Warshavsky]

>
>
> > +             if (cryptodev->data->dev_private == NULL) {
> > +                     RTE_LOG(ERR, PMD, "%s() Cannot allocate memzone
> for private device data",
> > +                     __func__);
>
> dpaa2_sec is already doing private log type, via DPAA2_SEC_LOG macro.
>
> You should go through your patch and make sure there are as few direct
> calls
> to RTE_LOG as possible.
>

Thanks Stephen,
missed that one which seems to be declared but not yet used.Will replace

I may be missing something here.
While seeing the point in a short common form such as PMD_DRV_LOG,
why won't dpaa use the same syntax rather than adding another syntax
variant which does pretty much the same?

[dpdk-dev] [PATCH v6 02/11] bond: replace rte_panic instances in bonding driver

[Arnon Warshavsky]

replace panic calls with log and return value.
Local functions to this file,
changing from void to int are non-abi-breaking

Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
---
 drivers/net/bonding/rte_eth_bond_8023ad.c         | 29 ++++++++++++++---------
 drivers/net/bonding/rte_eth_bond_8023ad_private.h |  2 +-
 drivers/net/bonding/rte_eth_bond_api.c            | 22 ++++++++++++-----
 drivers/net/bonding/rte_eth_bond_pmd.c            |  9 ++++---
 drivers/net/bonding/rte_eth_bond_private.h        |  2 +-
 5 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index c452318..2fb6cad 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -893,7 +893,7 @@
 			bond_mode_8023ad_periodic_cb, arg);
 }
 
-void
+int
 bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 				uint16_t slave_id)
 {
@@ -939,7 +939,7 @@
 	timer_cancel(&port->warning_timer);
 
 	if (port->mbuf_pool != NULL)
-		return;
+		return 0;
 
 	RTE_ASSERT(port->rx_ring == NULL);
 	RTE_ASSERT(port->tx_ring == NULL);
@@ -968,8 +968,9 @@
 	/* Any memory allocation failure in initialization is critical because
 	 * resources can't be free, so reinitialization is impossible. */
 	if (port->mbuf_pool == NULL) {
-		rte_panic("Slave %u: Failed to create memory pool '%s': %s\n",
-			slave_id, mem_name, rte_strerror(rte_errno));
+		RTE_LOG(ERR, PMD, "%s() Slave %u: Failed to create memory pool '%s': %s\n",
+			__func__, slave_id, mem_name, rte_strerror(rte_errno));
+		return -1;
 	}
 
 	snprintf(mem_name, RTE_DIM(mem_name), "slave_%u_rx", slave_id);
@@ -977,8 +978,9 @@
 			rte_align32pow2(BOND_MODE_8023AX_SLAVE_RX_PKTS), socket_id, 0);
 
 	if (port->rx_ring == NULL) {
-		rte_panic("Slave %u: Failed to create rx ring '%s': %s\n", slave_id,
-			mem_name, rte_strerror(rte_errno));
+		RTE_LOG(ERR, PMD, "%s() Slave %u: Failed to create rx ring '%s': %s\n",
+			__func__, slave_id, mem_name, rte_strerror(rte_errno));
+		return -1;
 	}
 
 	/* TX ring is at least one pkt longer to make room for marker packet. */
@@ -987,9 +989,12 @@
 			rte_align32pow2(BOND_MODE_8023AX_SLAVE_TX_PKTS + 1), socket_id, 0);
 
 	if (port->tx_ring == NULL) {
-		rte_panic("Slave %u: Failed to create tx ring '%s': %s\n", slave_id,
-			mem_name, rte_strerror(rte_errno));
+		RTE_LOG(ERR, PMD, "%s() Slave %u: Fail to create tx ring '%s': %s\n",
+			__func__, slave_id, mem_name, rte_strerror(rte_errno));
+		return -1;
 	}
+
+	return 0;
 }
 
 int
@@ -1143,9 +1148,11 @@
 	struct bond_dev_private *internals = bond_dev->data->dev_private;
 	uint8_t i;
 
-	for (i = 0; i < internals->active_slave_count; i++)
-		bond_mode_8023ad_activate_slave(bond_dev,
-				internals->active_slaves[i]);
+	for (i = 0; i < internals->active_slave_count; i++) {
+		if (!bond_mode_8023ad_activate_slave(bond_dev,
+						internals->active_slaves[i]))
+			return -1;
+	}
 
 	return 0;
 }
diff --git a/drivers/net/bonding/rte_eth_bond_8023ad_private.h b/drivers/net/bonding/rte_eth_bond_8023ad_private.h
index 0f490a5..96a42f2 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad_private.h
+++ b/drivers/net/bonding/rte_eth_bond_8023ad_private.h
@@ -263,7 +263,7 @@ struct mode8023ad_private {
  * @return
  *  0 on success, negative value otherwise.
  */
-void
+int
 bond_mode_8023ad_activate_slave(struct rte_eth_dev *dev, uint16_t port_id);
 
 /**
diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index aa89425..657fd74 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -69,14 +69,15 @@
 	return 0;
 }
 
-void
+int
 activate_slave(struct rte_eth_dev *eth_dev, uint16_t port_id)
 {
 	struct bond_dev_private *internals = eth_dev->data->dev_private;
 	uint8_t active_count = internals->active_slave_count;
 
 	if (internals->mode == BONDING_MODE_8023AD)
-		bond_mode_8023ad_activate_slave(eth_dev, port_id);
+		if (bond_mode_8023ad_activate_slave(eth_dev, port_id) != 0)
+			return -1;
 
 	if (internals->mode == BONDING_MODE_TLB
 			|| internals->mode == BONDING_MODE_ALB) {
@@ -94,6 +95,8 @@
 		bond_tlb_activate_slave(internals);
 	if (internals->mode == BONDING_MODE_ALB)
 		bond_mode_alb_client_list_upd(eth_dev);
+
+	return 0;
 }
 
 void
@@ -357,10 +360,17 @@
 				bond_ethdev_primary_set(internals,
 							slave_port_id);
 
-			if (find_slave_by_id(internals->active_slaves,
-					     internals->active_slave_count,
-					     slave_port_id) == internals->active_slave_count)
-				activate_slave(bonded_eth_dev, slave_port_id);
+			int rc =
+				find_slave_by_id(internals->active_slaves,
+					internals->active_slave_count,
+					slave_port_id);
+
+			if (rc == internals->active_slave_count) {
+				int rc = activate_slave(bonded_eth_dev,
+							slave_port_id);
+				if (rc != 0)
+					return -1;
+			}
 		}
 	}
 
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 2805c71..2d9052d 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1741,8 +1741,10 @@ struct bwg_slave {
 		/* Any memory allocation failure in initialization is critical because
 		 * resources can't be free, so reinitialization is impossible. */
 		if (port->slow_pool == NULL) {
-			rte_panic("Slave %u: Failed to create memory pool '%s': %s\n",
-				slave_id, mem_name, rte_strerror(rte_errno));
+			RTE_LOG(ERR, PMD, "%s() Slave %u: Failed to create memory pool '%s': %s\n",
+				__func__, slave_id,
+				mem_name, rte_strerror(rte_errno));
+			return -1;
 		}
 	}
 
@@ -2673,7 +2675,8 @@ struct bwg_slave {
 			mac_address_slaves_update(bonded_eth_dev);
 		}
 
-		activate_slave(bonded_eth_dev, port_id);
+		if (activate_slave(bonded_eth_dev, port_id) != 0)
+			return -1;
 
 		/* If user has defined the primary port then default to using it */
 		if (internals->user_defined_primary_port &&
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 94eca88..d99d42c 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -187,7 +187,7 @@ struct bond_dev_private {
 void
 deactivate_slave(struct rte_eth_dev *eth_dev, uint16_t port_id);
 
-void
+int
 activate_slave(struct rte_eth_dev *eth_dev, uint16_t port_id);
 
 void
-- 
1.8.3.1

[dpdk-dev] [PATCH v6 03/11] e1000: replace rte_panic instances in e1000 driver

[Arnon Warshavsky]

replace panic calls with log and return value.
Local function to this file,
changing from void to int is non-abi-breaking

Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
---

v6:
 - replaced rte_log with pmd log macro

 drivers/net/e1000/e1000_ethdev.h |  2 +-
 drivers/net/e1000/igb_ethdev.c   |  4 +++-
 drivers/net/e1000/igb_pf.c       | 15 +++++++++------
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/e1000/e1000_ethdev.h b/drivers/net/e1000/e1000_ethdev.h
index 6354b89..2e527de 100644
--- a/drivers/net/e1000/e1000_ethdev.h
+++ b/drivers/net/e1000/e1000_ethdev.h
@@ -411,7 +411,7 @@ int eth_igb_rss_hash_conf_get(struct rte_eth_dev *dev,
 /*
  * misc function prototypes
  */
-void igb_pf_host_init(struct rte_eth_dev *eth_dev);
+int igb_pf_host_init(struct rte_eth_dev *eth_dev);
 
 void igb_pf_mbx_process(struct rte_eth_dev *eth_dev);
 
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index 9b808a9..67a32a2 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -833,7 +833,9 @@ static int igb_flex_filter_uninit(struct rte_eth_dev *eth_dev)
 	}
 
 	/* initialize PF if max_vfs not zero */
-	igb_pf_host_init(eth_dev);
+	error = igb_pf_host_init(eth_dev);
+	if (error != 0)
+		goto err_late;
 
 	ctrl_ext = E1000_READ_REG(hw, E1000_CTRL_EXT);
 	/* Set PF Reset Done bit so PF/VF Mail Ops can work */
diff --git a/drivers/net/e1000/igb_pf.c b/drivers/net/e1000/igb_pf.c
index b9f2e53..6e511a9 100644
--- a/drivers/net/e1000/igb_pf.c
+++ b/drivers/net/e1000/igb_pf.c
@@ -63,7 +63,7 @@ int igb_vf_perm_addr_gen(struct rte_eth_dev *dev, uint16_t vf_num)
 	return 0;
 }
 
-void igb_pf_host_init(struct rte_eth_dev *eth_dev)
+int igb_pf_host_init(struct rte_eth_dev *eth_dev)
 {
 	struct e1000_vf_info **vfinfo =
 		E1000_DEV_PRIVATE_TO_P_VFDATA(eth_dev->data->dev_private);
@@ -74,7 +74,7 @@ void igb_pf_host_init(struct rte_eth_dev *eth_dev)
 
 	RTE_ETH_DEV_SRIOV(eth_dev).active = 0;
 	if (0 == (vf_num = dev_num_vf(eth_dev)))
-		return;
+		return 0;
 
 	if (hw->mac.type == e1000_i350)
 		nb_queue = 1;
@@ -82,11 +82,14 @@ void igb_pf_host_init(struct rte_eth_dev *eth_dev)
 		/* per datasheet, it should be 2, but 1 seems correct */
 		nb_queue = 1;
 	else
-		return;
+		return 0;
 
 	*vfinfo = rte_zmalloc("vf_info", sizeof(struct e1000_vf_info) * vf_num, 0);
-	if (*vfinfo == NULL)
-		rte_panic("Cannot allocate memory for private VF data\n");
+	if (*vfinfo == NULL) {
+		PMD_DRV_LOG(CRIT, "%s(): Cannot allocate memory for private VF data\n",
+			__func__);
+		return -ENOMEM;
+	}
 
 	RTE_ETH_DEV_SRIOV(eth_dev).active = ETH_8_POOLS;
 	RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool = nb_queue;
@@ -98,7 +101,7 @@ void igb_pf_host_init(struct rte_eth_dev *eth_dev)
 	/* set mb interrupt mask */
 	igb_mb_intr_setup(eth_dev);
 
-	return;
+	return 0;
 }
 
 void igb_pf_host_uninit(struct rte_eth_dev *dev)
-- 
1.8.3.1

[dpdk-dev] [PATCH v6 04/11] ixgbe: replace rte_panic instances in ixgbe driver

[Arnon Warshavsky]

replace panic calls with log and return value.
Local function to this file,
changing from void to int is non-abi-breaking

Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
---

v6:
 - replaced rte_log with pmd log macro

 drivers/net/ixgbe/ixgbe_ethdev.c |  6 ++++--
 drivers/net/ixgbe/ixgbe_ethdev.h |  2 +-
 drivers/net/ixgbe/ixgbe_pf.c     | 15 ++++++++++-----
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index a5e2fc0..fb95cc7 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -1061,7 +1061,7 @@ struct rte_ixgbe_xstats_name_off {
 		IXGBE_DEV_PRIVATE_TO_BW_CONF(eth_dev->data->dev_private);
 	uint32_t ctrl_ext;
 	uint16_t csum;
-	int diag, i;
+	int diag, i, error;
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -1224,7 +1224,9 @@ struct rte_ixgbe_xstats_name_off {
 	memset(hwstrip, 0, sizeof(*hwstrip));
 
 	/* initialize PF if max_vfs not zero */
-	ixgbe_pf_host_init(eth_dev);
+	error = ixgbe_pf_host_init(eth_dev);
+	if (error != 0)
+		return error;
 
 	ctrl_ext = IXGBE_READ_REG(hw, IXGBE_CTRL_EXT);
 	/* let hardware know driver is loaded */
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index 6550777..8bb41ec 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -661,7 +661,7 @@ int ixgbe_fdir_filter_program(struct rte_eth_dev *dev,
 
 void ixgbe_vlan_hw_strip_config(struct rte_eth_dev *dev);
 
-void ixgbe_pf_host_init(struct rte_eth_dev *eth_dev);
+int ixgbe_pf_host_init(struct rte_eth_dev *eth_dev);
 
 void ixgbe_pf_host_uninit(struct rte_eth_dev *eth_dev);
 
diff --git a/drivers/net/ixgbe/ixgbe_pf.c b/drivers/net/ixgbe/ixgbe_pf.c
index 4e61310..81a9910 100644
--- a/drivers/net/ixgbe/ixgbe_pf.c
+++ b/drivers/net/ixgbe/ixgbe_pf.c
@@ -66,7 +66,7 @@ int ixgbe_vf_perm_addr_gen(struct rte_eth_dev *dev, uint16_t vf_num)
 	return 0;
 }
 
-void ixgbe_pf_host_init(struct rte_eth_dev *eth_dev)
+int ixgbe_pf_host_init(struct rte_eth_dev *eth_dev)
 {
 	struct ixgbe_vf_info **vfinfo =
 		IXGBE_DEV_PRIVATE_TO_P_VFDATA(eth_dev->data->dev_private);
@@ -84,11 +84,14 @@ void ixgbe_pf_host_init(struct rte_eth_dev *eth_dev)
 	RTE_ETH_DEV_SRIOV(eth_dev).active = 0;
 	vf_num = dev_num_vf(eth_dev);
 	if (vf_num == 0)
-		return;
+		return 0;
 
 	*vfinfo = rte_zmalloc("vf_info", sizeof(struct ixgbe_vf_info) * vf_num, 0);
-	if (*vfinfo == NULL)
-		rte_panic("Cannot allocate memory for private VF data\n");
+	if (*vfinfo == NULL) {
+		PMD_DRV_LOG(ERR, "%s() Cannot allocate memory for private VF data\n",
+				__func__);
+		return -ENOMEM;
+	}
 
 	memset(mirror_info, 0, sizeof(struct ixgbe_mirror_info));
 	memset(uta_info, 0, sizeof(struct ixgbe_uta_info));
@@ -116,6 +119,8 @@ void ixgbe_pf_host_init(struct rte_eth_dev *eth_dev)
 
 	/* set mb interrupt mask */
 	ixgbe_mb_intr_setup(eth_dev);
+
+	return 0;
 }
 
 void ixgbe_pf_host_uninit(struct rte_eth_dev *eth_dev)
@@ -203,7 +208,7 @@ int ixgbe_pf_host_configure(struct rte_eth_dev *eth_dev)
 
 	vf_num = dev_num_vf(eth_dev);
 	if (vf_num == 0)
-		return -1;
+		return -ENOMEM;
 
 	/* enable VMDq and set the default pool for PF */
 	vtctl = IXGBE_READ_REG(hw, IXGBE_VT_CTL);
-- 
1.8.3.1

[dpdk-dev] [PATCH v6 05/11] eal: replace rte_panic instances in eventdev

[Arnon Warshavsky]

replace panic calls with log and return value.

Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
---
 lib/librte_eventdev/rte_eventdev_pmd_pci.h  | 8 +++++---
 lib/librte_eventdev/rte_eventdev_pmd_vdev.h | 8 +++++---
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/lib/librte_eventdev/rte_eventdev_pmd_pci.h b/lib/librte_eventdev/rte_eventdev_pmd_pci.h
index 8fb6138..6e08705 100644
--- a/lib/librte_eventdev/rte_eventdev_pmd_pci.h
+++ b/lib/librte_eventdev/rte_eventdev_pmd_pci.h
@@ -66,9 +66,11 @@
 						RTE_CACHE_LINE_SIZE,
 						rte_socket_id());
 
-		if (eventdev->data->dev_private == NULL)
-			rte_panic("Cannot allocate memzone for private "
-					"device data");
+		if (eventdev->data->dev_private == NULL) {
+			RTE_LOG(CRIT, EAL, "%s(): Cannot allocate memzone for private device data",
+				__func__);
+			return -ENOMEM;
+		}
 	}
 
 	eventdev->dev = &pci_dev->device;
diff --git a/lib/librte_eventdev/rte_eventdev_pmd_vdev.h b/lib/librte_eventdev/rte_eventdev_pmd_vdev.h
index 8c64a06..b7c08fa 100644
--- a/lib/librte_eventdev/rte_eventdev_pmd_vdev.h
+++ b/lib/librte_eventdev/rte_eventdev_pmd_vdev.h
@@ -61,9 +61,11 @@
 						RTE_CACHE_LINE_SIZE,
 						socket_id);
 
-		if (eventdev->data->dev_private == NULL)
-			rte_panic("Cannot allocate memzone for private device"
-					" data");
+		if (eventdev->data->dev_private == NULL) {
+			RTE_LOG(CRIT, EAL, "%s(): Cannot allocate memzone for private device data",
+				__func__);
+			return NULL;
+		}
 	}
 
 	return eventdev;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v6 05/11] eal: replace rte_panic instances in eventdev

[Stephen Hemminger]

On Tue, 24 Apr 2018 09:41:57 +0300
Arnon Warshavsky <arnon@qwilt.com> wrote:

> diff --git a/lib/librte_eventdev/rte_eventdev_pmd_pci.h b/lib/librte_eventdev/rte_eventdev_pmd_pci.h
> index 8fb6138..6e08705 100644
> --- a/lib/librte_eventdev/rte_eventdev_pmd_pci.h
> +++ b/lib/librte_eventdev/rte_eventdev_pmd_pci.h
> @@ -66,9 +66,11 @@
>  						RTE_CACHE_LINE_SIZE,
>  						rte_socket_id());
>  
> -		if (eventdev->data->dev_private == NULL)
> -			rte_panic("Cannot allocate memzone for private "
> -					"device data");
> +		if (eventdev->data->dev_private == NULL) {
> +			RTE_LOG(CRIT, EAL, "%s(): Cannot allocate memzone for private device data",
> +				__func__);
> +			return -ENOMEM;
> +		}
>  	

Off topic, why is the probe function inline in a .h file.
This is wasteful, and not at all performance critical. it should be in the .c file.

Re: [dpdk-dev] [PATCH v6 05/11] eal: replace rte_panic instances in eventdev

[Stephen Hemminger]

On Tue, 24 Apr 2018 09:41:57 +0300
Arnon Warshavsky <arnon@qwilt.com> wrote:

> +		if (eventdev->data->dev_private == NULL) {
> +			RTE_LOG(CRIT, EAL, "%s(): Cannot allocate memzone for private device data",
> +				__func__);
> +			return -ENOMEM;
> +		}

In eventdev, use RTE_EDEV_LOG_ERR for this.

Re: [dpdk-dev] [PATCH v6 05/11] eal: replace rte_panic instances in eventdev

[Arnon Warshavsky]

Thanks. Same as with dpaa

On Tue, Apr 24, 2018 at 6:06 PM, Stephen Hemminger <
stephen@networkplumber.org> wrote:

> On Tue, 24 Apr 2018 09:41:57 +0300
> Arnon Warshavsky <arnon@qwilt.com> wrote:
>
> > +             if (eventdev->data->dev_private == NULL) {
> > +                     RTE_LOG(CRIT, EAL, "%s(): Cannot allocate memzone
> for private device data",
> > +                             __func__);
> > +                     return -ENOMEM;
> > +             }
>
> In eventdev, use RTE_EDEV_LOG_ERR for this.
>



-- 

*Arnon Warshavsky*
*Qwilt | work: +972-72-2221634 | mobile: +972-50-8583058 | arnon@qwilt.com
<arnon@qwilt.com>*

[dpdk-dev] [PATCH v6 06/11] kni: replace rte_panic instances in kni

[Arnon Warshavsky]

replace panic calls with log and return value.
Local function to this file,
changing from void to int is non-abi-breaking

Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
---
 lib/librte_kni/rte_kni.c      | 18 ++++++++++++------
 lib/librte_kni/rte_kni_fifo.h | 11 ++++++++---
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
index 8a8f6c1..4dac407 100644
--- a/lib/librte_kni/rte_kni.c
+++ b/lib/librte_kni/rte_kni.c
@@ -353,37 +353,43 @@ struct rte_kni *
 	/* TX RING */
 	mz = slot->m_tx_q;
 	ctx->tx_q = mz->addr;
-	kni_fifo_init(ctx->tx_q, KNI_FIFO_COUNT_MAX);
+	if (kni_fifo_init(ctx->tx_q, KNI_FIFO_COUNT_MAX))
+		return NULL;
 	dev_info.tx_phys = mz->phys_addr;
 
 	/* RX RING */
 	mz = slot->m_rx_q;
 	ctx->rx_q = mz->addr;
-	kni_fifo_init(ctx->rx_q, KNI_FIFO_COUNT_MAX);
+	if (kni_fifo_init(ctx->rx_q, KNI_FIFO_COUNT_MAX))
+		return NULL;
 	dev_info.rx_phys = mz->phys_addr;
 
 	/* ALLOC RING */
 	mz = slot->m_alloc_q;
 	ctx->alloc_q = mz->addr;
-	kni_fifo_init(ctx->alloc_q, KNI_FIFO_COUNT_MAX);
+	if (kni_fifo_init(ctx->alloc_q, KNI_FIFO_COUNT_MAX))
+		return NULL;
 	dev_info.alloc_phys = mz->phys_addr;
 
 	/* FREE RING */
 	mz = slot->m_free_q;
 	ctx->free_q = mz->addr;
-	kni_fifo_init(ctx->free_q, KNI_FIFO_COUNT_MAX);
+	if (kni_fifo_init(ctx->free_q, KNI_FIFO_COUNT_MAX))
+		return NULL;
 	dev_info.free_phys = mz->phys_addr;
 
 	/* Request RING */
 	mz = slot->m_req_q;
 	ctx->req_q = mz->addr;
-	kni_fifo_init(ctx->req_q, KNI_FIFO_COUNT_MAX);
+	if (kni_fifo_init(ctx->req_q, KNI_FIFO_COUNT_MAX))
+		return NULL;
 	dev_info.req_phys = mz->phys_addr;
 
 	/* Response RING */
 	mz = slot->m_resp_q;
 	ctx->resp_q = mz->addr;
-	kni_fifo_init(ctx->resp_q, KNI_FIFO_COUNT_MAX);
+	if (kni_fifo_init(ctx->resp_q, KNI_FIFO_COUNT_MAX))
+		return NULL;
 	dev_info.resp_phys = mz->phys_addr;
 
 	/* Req/Resp sync mem area */
diff --git a/lib/librte_kni/rte_kni_fifo.h b/lib/librte_kni/rte_kni_fifo.h
index ac26a8c..5052015 100644
--- a/lib/librte_kni/rte_kni_fifo.h
+++ b/lib/librte_kni/rte_kni_fifo.h
@@ -7,17 +7,22 @@
 /**
  * Initializes the kni fifo structure
  */
-static void
+static int
 kni_fifo_init(struct rte_kni_fifo *fifo, unsigned size)
 {
 	/* Ensure size is power of 2 */
-	if (size & (size - 1))
-		rte_panic("KNI fifo size must be power of 2\n");
+	if (size & (size - 1)) {
+		RTE_LOG(CRIT, EAL, "%s(): KNI fifo size must be power of 2\n",
+				__func__);
+		return -1;
+	}
 
 	fifo->write = 0;
 	fifo->read = 0;
 	fifo->len = size;
 	fifo->elem_size = sizeof(void *);
+
+	return 0;
 }
 
 /**
-- 
1.8.3.1

[dpdk-dev] [PATCH v6 07/11] eal: replace rte_panic instances in hugepage_info

[Arnon Warshavsky]

replace panic calls with log and return value.

Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
---
 lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 37 +++++++++++++++++--------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
index db5aabd..797b8fa 100644
--- a/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
+++ b/lib/librte_eal/linuxapp/eal/eal_hugepage_info.c
@@ -145,8 +145,8 @@
 	return num_pages;
 }
 
-static uint64_t
-get_default_hp_size(void)
+static int
+get_default_hp_size(uint64_t *result)
 {
 	const char proc_meminfo[] = "/proc/meminfo";
 	const char str_hugepagesz[] = "Hugepagesize:";
@@ -155,8 +155,11 @@
 	unsigned long long size = 0;
 
 	FILE *fd = fopen(proc_meminfo, "r");
-	if (fd == NULL)
-		rte_panic("Cannot open %s\n", proc_meminfo);
+	if (fd == NULL) {
+		RTE_LOG(CRIT, EAL, "%s(): Cannot open %s\n",
+				__func__, proc_meminfo);
+		return -1;
+	}
 	while(fgets(buffer, sizeof(buffer), fd)){
 		if (strncmp(buffer, str_hugepagesz, hugepagesz_len) == 0){
 			size = rte_str_to_size(&buffer[hugepagesz_len]);
@@ -164,9 +167,13 @@
 		}
 	}
 	fclose(fd);
-	if (size == 0)
-		rte_panic("Cannot get default hugepage size from %s\n", proc_meminfo);
-	return size;
+	if (size == 0) {
+		RTE_LOG(CRIT, EAL, "%s(): Cannot get default hugepage size from %s\n",
+						 __func__, proc_meminfo);
+		return -1;
+	}
+	*result = size;
+	return 0;
 }
 
 static int
@@ -191,11 +198,19 @@
 	int retval = -1;
 
 	FILE *fd = fopen(proc_mounts, "r");
-	if (fd == NULL)
-		rte_panic("Cannot open %s\n", proc_mounts);
+	if (fd == NULL) {
+		RTE_LOG(CRIT, EAL, "%s(): Cannot open %s\n",
+				__func__, proc_mounts);
+		return -ENOENT;
+	}
 
-	if (default_size == 0)
-		default_size = get_default_hp_size();
+	if (default_size == 0) {
+		retval = get_default_hp_size(&default_size);
+		if (retval) {
+			fclose(fd);
+			return retval;
+		}
+	}
 
 	while (fgets(buf, sizeof(buf), fd)){
 		if (rte_strsplit(buf, sizeof(buf), splitstr, _FIELDNAME_MAX,
-- 
1.8.3.1

[dpdk-dev] [PATCH v6 08/11] eal: replace rte_panic instances in interrupts thread

[Arnon Warshavsky]

replace panic calls with log and return value.
Thread function removes the noreturn attribute.

Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
---
 lib/librte_eal/linuxapp/eal/eal_interrupts.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index 58e9328..77e6f2a 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -785,7 +785,7 @@ struct rte_intr_source {
  * @return
  *  never return;
  */
-static __attribute__((noreturn)) void *
+static void *
 eal_intr_thread_main(__rte_unused void *arg)
 {
 	struct epoll_event ev;
@@ -803,8 +803,11 @@ static __attribute__((noreturn)) void *
 
 		/* create epoll fd */
 		int pfd = epoll_create(1);
-		if (pfd < 0)
-			rte_panic("Cannot create epoll instance\n");
+		if (pfd < 0) {
+			RTE_LOG(CRIT, EAL, "%s(): Cannot create epoll instance\n",
+					__func__);
+			return NULL;
+		}
 
 		pipe_event.data.fd = intr_pipe.readfd;
 		/**
@@ -813,8 +816,11 @@ static __attribute__((noreturn)) void *
 		 */
 		if (epoll_ctl(pfd, EPOLL_CTL_ADD, intr_pipe.readfd,
 						&pipe_event) < 0) {
-			rte_panic("Error adding fd to %d epoll_ctl, %s\n",
+			RTE_LOG(CRIT, EAL, "%s(): Error adding fd to %d "
+					"epoll_ctl, %s\n",
+					__func__,
 					intr_pipe.readfd, strerror(errno));
+			return NULL;
 		}
 		numfds++;
 
@@ -831,9 +837,12 @@ static __attribute__((noreturn)) void *
 			 * into wait list.
 			 */
 			if (epoll_ctl(pfd, EPOLL_CTL_ADD,
-					src->intr_handle.fd, &ev) < 0){
-				rte_panic("Error adding fd %d epoll_ctl, %s\n",
-					src->intr_handle.fd, strerror(errno));
+					src->intr_handle.fd, &ev) < 0) {
+				RTE_LOG(CRIT, EAL, "%s(): Error adding fd %d epoll_ctl, %s\n",
+					__func__,
+					src->intr_handle.fd,
+					strerror(errno));
+				return NULL;
 			}
 			else
 				numfds++;
@@ -848,6 +857,8 @@ static __attribute__((noreturn)) void *
 		 */
 		close(pfd);
 	}
+
+	return NULL;
 }
 
 int
-- 
1.8.3.1

[dpdk-dev] [PATCH v6 09/11] eal: replace rte_panic instances in ethdev

[Arnon Warshavsky]

Local function to this file,
changing from void to int is non-abi-breaking

Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
---
 lib/librte_ether/rte_ethdev.c | 42 ++++++++++++++++++++++++++++++------------
 lib/librte_ether/rte_ethdev.h |  4 +++-
 2 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 7821a88..4ffdc54 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -194,7 +194,7 @@ enum {
 	return port_id;
 }
 
-static void
+static int
 rte_eth_dev_shared_data_prepare(void)
 {
 	const unsigned flags = 0;
@@ -210,8 +210,12 @@ enum {
 					rte_socket_id(), flags);
 		} else
 			mz = rte_memzone_lookup(MZ_RTE_ETH_DEV_DATA);
-		if (mz == NULL)
-			rte_panic("Cannot allocate ethdev shared data\n");
+		if (mz == NULL) {
+			rte_spinlock_unlock(&rte_eth_shared_data_lock);
+			RTE_LOG(CRIT, EAL, "%s(): Cannot allocate ethdev shared data\n",
+					__func__);
+			return -1;
+		}
 
 		rte_eth_dev_shared_data = mz->addr;
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
@@ -224,6 +228,8 @@ enum {
 	}
 
 	rte_spinlock_unlock(&rte_eth_shared_data_lock);
+
+	return 0;
 }
 
 struct rte_eth_dev *
@@ -274,7 +280,8 @@ struct rte_eth_dev *
 	uint16_t port_id;
 	struct rte_eth_dev *eth_dev = NULL;
 
-	rte_eth_dev_shared_data_prepare();
+	if (rte_eth_dev_shared_data_prepare() != 0)
+		return NULL;
 
 	/* Synchronize port creation between primary and secondary threads. */
 	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
@@ -317,7 +324,8 @@ struct rte_eth_dev *
 	uint16_t i;
 	struct rte_eth_dev *eth_dev = NULL;
 
-	rte_eth_dev_shared_data_prepare();
+	if (rte_eth_dev_shared_data_prepare() != 0)
+		return NULL;
 
 	/* Synchronize port attachment to primary port creation and release. */
 	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
@@ -345,7 +353,8 @@ struct rte_eth_dev *
 	if (eth_dev == NULL)
 		return -EINVAL;
 
-	rte_eth_dev_shared_data_prepare();
+	if (rte_eth_dev_shared_data_prepare() != 0)
+		return -1;
 
 	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
 
@@ -399,7 +408,8 @@ struct rte_eth_dev *
 int __rte_experimental
 rte_eth_dev_owner_new(uint64_t *owner_id)
 {
-	rte_eth_dev_shared_data_prepare();
+	if (rte_eth_dev_shared_data_prepare() != 0)
+		return -1;
 
 	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
 
@@ -450,7 +460,8 @@ struct rte_eth_dev *
 {
 	int ret;
 
-	rte_eth_dev_shared_data_prepare();
+	if (rte_eth_dev_shared_data_prepare() != 0)
+		return -1;
 
 	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
 
@@ -467,7 +478,8 @@ struct rte_eth_dev *
 			{.id = RTE_ETH_DEV_NO_OWNER, .name = ""};
 	int ret;
 
-	rte_eth_dev_shared_data_prepare();
+	if (rte_eth_dev_shared_data_prepare() != 0)
+		return -1;
 
 	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
 
@@ -477,12 +489,15 @@ struct rte_eth_dev *
 	return ret;
 }
 
-void __rte_experimental
+int __rte_experimental
 rte_eth_dev_owner_delete(const uint64_t owner_id)
 {
 	uint16_t port_id;
+	int error;
 
-	rte_eth_dev_shared_data_prepare();
+	error = rte_eth_dev_shared_data_prepare();
+	if (error != 0)
+		return error;
 
 	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
 
@@ -495,6 +510,8 @@ struct rte_eth_dev *
 	}
 
 	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
+
+	return 0;
 }
 
 int __rte_experimental
@@ -502,7 +519,8 @@ struct rte_eth_dev *
 {
 	int ret = 0;
 
-	rte_eth_dev_shared_data_prepare();
+	if (rte_eth_dev_shared_data_prepare() != 0)
+		return -1;
 
 	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
 
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index b9eb8ae..46e5947 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1354,8 +1354,10 @@ int __rte_experimental rte_eth_dev_owner_unset(const uint16_t port_id,
  *
  * @param	owner_id
  *  The owner identifier.
+ *  @return
+ *  0 on success, negative errno value on error.
  */
-void __rte_experimental rte_eth_dev_owner_delete(const uint64_t owner_id);
+int __rte_experimental rte_eth_dev_owner_delete(const uint64_t owner_id);
 
 /**
  * @warning
-- 
1.8.3.1

[dpdk-dev] [PATCH v6 10/11] eal: replace rte_panic instances in init sequence

[Arnon Warshavsky]

Change some local functions return type from void to int.
This change does not break ABI as the functions are internal.
Panic thrown from threads was not handled in this patch

Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
---
 lib/librte_eal/bsdapp/eal/eal.c   | 70 ++++++++++++++++++++--------
 lib/librte_eal/linuxapp/eal/eal.c | 97 ++++++++++++++++++++++++++-------------
 2 files changed, 115 insertions(+), 52 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index d996190..a3c3b37 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -151,7 +151,7 @@ enum rte_iova_mode
  * We also don't lock the whole file, so that in future we can use read-locks
  * on other parts, e.g. memzones, to detect if there are running secondary
  * processes. */
-static void
+static int
 rte_eal_config_create(void)
 {
 	void *rte_mem_cfg_addr;
@@ -160,60 +160,81 @@ enum rte_iova_mode
 	const char *pathname = eal_runtime_config_path();
 
 	if (internal_config.no_shconf)
-		return;
+		return 0;
 
 	if (mem_cfg_fd < 0){
 		mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0660);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(CRIT, EAL, "%s(): Cannot open '%s' for rte_mem_config\n",
+					__func__, pathname);
+			return -1;
+		}
 	}
 
 	retval = ftruncate(mem_cfg_fd, sizeof(*rte_config.mem_config));
 	if (retval < 0){
 		close(mem_cfg_fd);
-		rte_panic("Cannot resize '%s' for rte_mem_config\n", pathname);
+		mem_cfg_fd = -1;
+		RTE_LOG(CRIT, EAL, "%s(): Cannot resize '%s' for rte_mem_config\n",
+				__func__, pathname);
+		return -1;
 	}
 
 	retval = fcntl(mem_cfg_fd, F_SETLK, &wr_lock);
 	if (retval < 0){
 		close(mem_cfg_fd);
-		rte_exit(EXIT_FAILURE, "Cannot create lock on '%s'. Is another primary "
-				"process running?\n", pathname);
+		mem_cfg_fd = -1;
+		RTE_LOG(CRIT, EAL, "%s(): Cannot create lock on '%s'. Is another primary process running?\n",
+				__func__, pathname);
+		return -1;
 	}
 
 	rte_mem_cfg_addr = mmap(NULL, sizeof(*rte_config.mem_config),
 				PROT_READ | PROT_WRITE, MAP_SHARED, mem_cfg_fd, 0);
 
 	if (rte_mem_cfg_addr == MAP_FAILED){
-		rte_panic("Cannot mmap memory for rte_config\n");
+		RTE_LOG(CRIT, EAL, "%s(): Cannot mmap memory for rte_config\n",
+				__func__);
+		return -1;
 	}
 	memcpy(rte_mem_cfg_addr, &early_mem_config, sizeof(early_mem_config));
 	rte_config.mem_config = rte_mem_cfg_addr;
+
+	return 0;
 }
 
 /* attach to an existing shared memory config */
-static void
+static int
 rte_eal_config_attach(void)
 {
 	void *rte_mem_cfg_addr;
 	const char *pathname = eal_runtime_config_path();
 
 	if (internal_config.no_shconf)
-		return;
+		return 0;
 
 	if (mem_cfg_fd < 0){
 		mem_cfg_fd = open(pathname, O_RDWR);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(CRIT, EAL, "%s(): Cannot open '%s' for rte_mem_config\n",
+					__func__, pathname);
+			return -1;
+		}
 	}
 
 	rte_mem_cfg_addr = mmap(NULL, sizeof(*rte_config.mem_config),
 				PROT_READ | PROT_WRITE, MAP_SHARED, mem_cfg_fd, 0);
 	close(mem_cfg_fd);
-	if (rte_mem_cfg_addr == MAP_FAILED)
-		rte_panic("Cannot mmap memory for rte_config\n");
+	if (rte_mem_cfg_addr == MAP_FAILED) {
+		mem_cfg_fd = -1;
+		RTE_LOG(CRIT, EAL, "%s(): Cannot mmap memory for rte_config\n",
+				__func__);
+		return -1;
+	}
 
 	rte_config.mem_config = rte_mem_cfg_addr;
+
+	return 0;
 }
 
 /* Detect if we are a primary or a secondary process */
@@ -237,23 +258,29 @@ enum rte_proc_type_t
 }
 
 /* Sets up rte_config structure with the pointer to shared memory config.*/
-static void
+static int
 rte_config_init(void)
 {
 	rte_config.process_type = internal_config.process_type;
 
 	switch (rte_config.process_type){
 	case RTE_PROC_PRIMARY:
-		rte_eal_config_create();
+		if (rte_eal_config_create())
+			return -1;
 		break;
 	case RTE_PROC_SECONDARY:
-		rte_eal_config_attach();
+		if (rte_eal_config_attach())
+			return -1;
 		rte_eal_mcfg_wait_complete(rte_config.mem_config);
 		break;
 	case RTE_PROC_AUTO:
 	case RTE_PROC_INVALID:
-		rte_panic("Invalid process type\n");
+	default:
+		RTE_LOG(CRIT, EAL, "%s(): Invalid process type %d\n",
+				__func__, rte_config.process_type);
+		return -1;
 	}
+	return 0;
 }
 
 /* display usage */
@@ -595,7 +622,11 @@ static void rte_eal_init_alert(const char *msg)
 
 	rte_srand(rte_rdtsc());
 
-	rte_config_init();
+	if (rte_config_init() != 0) {
+		rte_eal_init_alert("Failed to init configuration");
+		rte_errno = EFAULT;
+		return -1;
+	}
 
 	if (rte_mp_channel_init() < 0) {
 		rte_eal_init_alert("failed to init mp channel\n");
@@ -668,6 +699,7 @@ static void rte_eal_init_alert(const char *msg)
 		 */
 		if (pipe(lcore_config[i].pipe_master2slave) < 0)
 			rte_panic("Cannot create pipe\n");
+
 		if (pipe(lcore_config[i].pipe_slave2master) < 0)
 			rte_panic("Cannot create pipe\n");
 
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 21afa73..1efcc9f 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -160,7 +160,7 @@ enum rte_iova_mode
  * We also don't lock the whole file, so that in future we can use read-locks
  * on other parts, e.g. memzones, to detect if there are running secondary
  * processes. */
-static void
+static int
 rte_eal_config_create(void)
 {
 	void *rte_mem_cfg_addr;
@@ -169,7 +169,7 @@ enum rte_iova_mode
 	const char *pathname = eal_runtime_config_path();
 
 	if (internal_config.no_shconf)
-		return;
+		return 0;
 
 	/* map the config before hugepage address so that we don't waste a page */
 	if (internal_config.base_virtaddr != 0)
@@ -179,30 +179,40 @@ enum rte_iova_mode
 	else
 		rte_mem_cfg_addr = NULL;
 
-	if (mem_cfg_fd < 0){
+	if (mem_cfg_fd < 0) {
 		mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0660);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(CRIT, EAL, "%s(): Cannot open '%s' for rte_mem_config\n",
+				__func__, pathname);
+			return -1;
+		}
 	}
 
 	retval = ftruncate(mem_cfg_fd, sizeof(*rte_config.mem_config));
-	if (retval < 0){
+	if (retval < 0) {
 		close(mem_cfg_fd);
-		rte_panic("Cannot resize '%s' for rte_mem_config\n", pathname);
+		mem_cfg_fd = -1;
+		RTE_LOG(CRIT, EAL, "%s(): Cannot resize '%s' for rte_mem_config\n",
+				__func__, pathname);
+		return -1;
 	}
 
 	retval = fcntl(mem_cfg_fd, F_SETLK, &wr_lock);
-	if (retval < 0){
+	if (retval < 0) {
 		close(mem_cfg_fd);
-		rte_exit(EXIT_FAILURE, "Cannot create lock on '%s'. Is another primary "
-				"process running?\n", pathname);
+		mem_cfg_fd = -1;
+		RTE_LOG(CRIT, EAL, "%s(): Cannot create lock on '%s'.Is another primary process running?\n",
+			__func__, pathname);
+		return -1;
 	}
 
 	rte_mem_cfg_addr = mmap(rte_mem_cfg_addr, sizeof(*rte_config.mem_config),
 				PROT_READ | PROT_WRITE, MAP_SHARED, mem_cfg_fd, 0);
 
-	if (rte_mem_cfg_addr == MAP_FAILED){
-		rte_panic("Cannot mmap memory for rte_config\n");
+	if (rte_mem_cfg_addr == MAP_FAILED) {
+		RTE_LOG(CRIT, EAL, "%s(): Cannot mmap memory for rte_config\n",
+			__func__);
+		return -1;
 	}
 	memcpy(rte_mem_cfg_addr, &early_mem_config, sizeof(early_mem_config));
 	rte_config.mem_config = rte_mem_cfg_addr;
@@ -211,10 +221,11 @@ enum rte_iova_mode
 	 * processes could later map the config into this exact location */
 	rte_config.mem_config->mem_cfg_addr = (uintptr_t) rte_mem_cfg_addr;
 
+	return 0;
 }
 
 /* attach to an existing shared memory config */
-static void
+static int
 rte_eal_config_attach(void)
 {
 	struct rte_mem_config *mem_config;
@@ -222,33 +233,41 @@ enum rte_iova_mode
 	const char *pathname = eal_runtime_config_path();
 
 	if (internal_config.no_shconf)
-		return;
+		return 0;
 
-	if (mem_cfg_fd < 0){
+	if (mem_cfg_fd < 0) {
 		mem_cfg_fd = open(pathname, O_RDWR);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(CRIT, EAL, "%s(): Cannot open '%s' for rte_mem_config\n",
+						__func__, pathname);
+			return -1;
+		}
 	}
 
 	/* map it as read-only first */
 	mem_config = (struct rte_mem_config *) mmap(NULL, sizeof(*mem_config),
 			PROT_READ, MAP_SHARED, mem_cfg_fd, 0);
-	if (mem_config == MAP_FAILED)
-		rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
-			  errno, strerror(errno));
+	if (mem_config == MAP_FAILED) {
+		mem_cfg_fd = -1;
+		RTE_LOG(CRIT, EAL, "%s(): Cannot mmap memory for rte_config! error %i (%s)\n",
+				__func__, errno, strerror(errno));
+		return -1;
+	}
 
 	rte_config.mem_config = mem_config;
+
+	return 0;
 }
 
 /* reattach the shared config at exact memory location primary process has it */
-static void
+static int
 rte_eal_config_reattach(void)
 {
 	struct rte_mem_config *mem_config;
 	void *rte_mem_cfg_addr;
 
 	if (internal_config.no_shconf)
-		return;
+		return 0;
 
 	/* save the address primary process has mapped shared config to */
 	rte_mem_cfg_addr = (void *) (uintptr_t) rte_config.mem_config->mem_cfg_addr;
@@ -263,16 +282,18 @@ enum rte_iova_mode
 	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
 		if (mem_config != MAP_FAILED)
 			/* errno is stale, don't use */
-			rte_panic("Cannot mmap memory for rte_config at [%p], got [%p]"
-				  " - please use '--base-virtaddr' option\n",
-				  rte_mem_cfg_addr, mem_config);
+			RTE_LOG(CRIT, EAL, "%s(): Cannot mmap memory for rte_config at [%p], got [%p] - please use '--base-virtaddr' option\n",
+					__func__, rte_mem_cfg_addr, mem_config);
 		else
-			rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
-				  errno, strerror(errno));
+			RTE_LOG(CRIT, EAL, "%s(): Cannot mmap memory for rte_config! error %i (%s)\n",
+					__func__, errno, strerror(errno));
+		return -1;
 	}
 	close(mem_cfg_fd);
 
 	rte_config.mem_config = mem_config;
+
+	return 0;
 }
 
 /* Detect if we are a primary or a secondary process */
@@ -296,24 +317,32 @@ enum rte_proc_type_t
 }
 
 /* Sets up rte_config structure with the pointer to shared memory config.*/
-static void
+static int
 rte_config_init(void)
 {
 	rte_config.process_type = internal_config.process_type;
 
 	switch (rte_config.process_type){
 	case RTE_PROC_PRIMARY:
-		rte_eal_config_create();
+		if (rte_eal_config_create())
+			return -1;
 		break;
 	case RTE_PROC_SECONDARY:
-		rte_eal_config_attach();
+		if (rte_eal_config_attach())
+			return -1;
 		rte_eal_mcfg_wait_complete(rte_config.mem_config);
-		rte_eal_config_reattach();
+		if (rte_eal_config_reattach())
+			return -1;
 		break;
 	case RTE_PROC_AUTO:
 	case RTE_PROC_INVALID:
-		rte_panic("Invalid process type\n");
+	default:
+		RTE_LOG(CRIT, EAL, "%s(): Invalid process type %d\n",
+				__func__, rte_config.process_type);
+		return -1;
 	}
+
+	return 0;
 }
 
 /* Unlocks hugepage directories that were locked by eal_hugepage_info_init */
@@ -820,7 +849,8 @@ static void rte_eal_init_alert(const char *msg)
 
 	rte_srand(rte_rdtsc());
 
-	rte_config_init();
+	if (rte_config_init() != 0)
+		return -1;
 
 	if (rte_eal_log_init(logid, internal_config.syslog_facility) < 0) {
 		rte_eal_init_alert("Cannot init logging.");
@@ -911,6 +941,7 @@ static void rte_eal_init_alert(const char *msg)
 		 */
 		if (pipe(lcore_config[i].pipe_master2slave) < 0)
 			rte_panic("Cannot create pipe\n");
+
 		if (pipe(lcore_config[i].pipe_slave2master) < 0)
 			rte_panic("Cannot create pipe\n");
 
-- 
1.8.3.1

[dpdk-dev] [PATCH v6 11/11] devtools: prevent new instances of rte_panic and rte_exit

[Arnon Warshavsky]

This patch adds a new function that is called
per every checked patch,
and alerts for new instances of rte_panic/rte_exit.
The check excludes comments, and alerts in the case
of a positive balance between additions and removals.

Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
---
 devtools/checkpatches.sh | 95 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 1 deletion(-)

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 7676a6b..48b2685 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -61,6 +61,91 @@ print_usage () {
 	END_OF_HELP
 }
 
+check_forbidden_additions() { # <file>
+    # ---------------------------------
+    #This awk script receives a list of expressions to monitor
+    #and a list of folders to search these expressions in
+    # - No search is done inside comments
+    # - Both additions and removals of the expressions are checked
+    #   A positive balance of additions fails the check
+	read -d '' awk_script << 'EOF'
+	BEGIN{
+		split(FOLDERS,deny_folders," ");
+		split(EXPRESSIONS,deny_expr," ");
+		in_file=0;
+		in_comment=0;
+		count=0;
+		comment_start="/*"
+		comment_end="*/"
+    }
+    # search for add/remove instances in current file
+    # state machine assumes the comments structure is enforced by
+    # checkpatches.pl
+    (in_file) {
+		# comment start
+		if (index($0,comment_start) > 0){
+			in_comment = 1
+		}
+		# non comment code
+		if (in_comment == 0) {
+			for (i in deny_expr) {
+				forbidden_added = "^\+.*" deny_expr[i];
+				forbidden_removed="^-.*" deny_expr[i];
+				current = expressions[deny_expr[i]]
+				if ($0 ~ forbidden_added) {
+					count = count + 1;
+					expressions[deny_expr[i]] = current + 1
+				}
+				if ($0 ~ forbidden_removed) {
+					count = count - 1;
+					expressions[deny_expr[i]] = current - 1
+				}
+			}
+		}
+
+		# comment end
+		if (index($0,comment_end) > 0)  {
+			in_comment = 0
+		}
+    }
+	# switch to next file , check if the balance of add/remove
+	# of previous filehad new additions
+	($0 ~ "^\+\+\+ b/") {
+		in_file = 0;
+		if (count > 0){
+			exit;
+		}
+		for (i in deny_folders){
+			re = "^\+\+\+ b/" deny_folders[i];
+			if ($0 ~ deny_folders[i]) {
+				in_file = 1
+				last_file = $0
+			}
+		}
+	}
+	END{
+		if (count > 0){
+			print "Warning: in " substr(last_file,6) ":"
+			print "are you sure you want to add the following:"
+			for (key in expressions) {
+				if (expressions[key] > 0) {
+					print key
+				}
+			}
+			exit 1
+		}
+	}
+EOF
+	# ---------------------------------
+
+	# refrain from new additions of rte_panic() and rte_exit()
+	# under lib and net
+	# multiple folders and expressions are separated by spaces
+	awk -v FOLDERS="lib net" \
+		-v EXPRESSIONS="rte_panic\\\( rte_exit\\\(" \
+		"$awk_script" -
+}
+
 number=0
 quiet=false
 verbose=false
@@ -89,11 +174,19 @@ check () { # <patch> <commit> <title>
 	total=$(($total + 1))
 	! $verbose || printf '\n### %s\n\n' "$3"
 	if [ -n "$1" ] ; then
+		cat "$1" | check_forbidden_additions
+		[ $? -eq 0 ] || return 0
 		report=$($DPDK_CHECKPATCH_PATH $options "$1" 2>/dev/null)
 	elif [ -n "$2" ] ; then
-		report=$(git format-patch --find-renames --no-stat --stdout -1 $commit |
+		params=$(echo "--find-renames --no-stat --stdout -1")
+		body=$(git format-patch $params $commit)
+		echo "$body" | check_forbidden_additions
+		[ $? -eq 0 ] || return 0
+		report=$(echo "$body" |
 			$DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
 	else
+		check_forbidden_additions -
+		[ $? -eq 0 ] || return 0
 		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
 	fi
 	[ $? -ne 0 ] || return 0
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Arnon Warshavsky]

please ignore this patchset.
The v6 formatting is messed up. resending
Sorry for the mess

On Tue, Apr 24, 2018 at 9:41 AM, Arnon Warshavsky <arnon@qwilt.com> wrote:

> The purpose of this patch series 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 teardown or other mitigation of the event.
>
> This patch modifies the majority of rte_panic calls
> under lib and drivers, and replaces them with a log message
> and an error return code according to context,
> 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 temination.
>   These should be handled by the driver maintainers..
> - local void functions with no api were changed to retrun a value
>   where needed
> - No change took place in example and test files
> - No change took place for debug assertions calling panic
> - A new function was added to devtools/checkpatches.sh
>   in order to prevent new additions of calls to rte_panic
>   under lib and drivers.
>
> Keep calm and don't panic
>
> ---
>
> v2:
> - reformat error messages so that literal string are in the same line
> - fix typo in commit message
> - add new return code to doxigen of rte_memzone_free()
>
> v3:
> - submit  all 13 patches changed and unchanged in the same patchset
>
> v4:
> - remove 2 patches that are no more relevant
> - fix split literal string in error message
> - change return value -1 to enum
> - split value and success code in a static function
>
> v5:
> - reword commit messages
> - revert thread related instances back to panicing
> - handle file descriptors with state to reset after eal init failure
>   in case re initialization takes place
>
> v6:
> - Use pmd log macro rather than rte_log
>
>
>
> Arnon Warshavsky (11):
>   crypto/dpaa: replace rte_panic instances in crypto/dpaa driver
>   bond: replace rte_panic instances in bonding driver
>   e1000: replace rte_panic instances in e1000 driver
>   ixgbe: replace rte_panic instances in ixgbe driver
>   eal: replace rte_panic instances in eventdev
>   kni: replace rte_panic instances in kni
>   eal: replace rte_panic instances in hugepage_info
>   eal: replace rte_panic instances in interrupts thread
>   eal: replace rte_panic instances in ethdev
>   eal: replace rte_panic instances in init sequence
>   devtools: prevent new instances of rte_panic and rte_exit
>
>  devtools/checkpatches.sh                          | 95
> +++++++++++++++++++++-
>  drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c       |  8 +-
>  drivers/crypto/dpaa_sec/dpaa_sec.c                |  8 +-
>  drivers/net/bonding/rte_eth_bond_8023ad.c         | 29 ++++---
>  drivers/net/bonding/rte_eth_bond_8023ad_private.h |  2 +-
>  drivers/net/bonding/rte_eth_bond_api.c            | 22 +++--
>  drivers/net/bonding/rte_eth_bond_pmd.c            |  9 ++-
>  drivers/net/bonding/rte_eth_bond_private.h        |  2 +-
>  drivers/net/e1000/e1000_ethdev.h                  |  2 +-
>  drivers/net/e1000/igb_ethdev.c                    |  4 +-
>  drivers/net/e1000/igb_pf.c                        | 15 ++--
>  drivers/net/ixgbe/ixgbe_ethdev.c                  |  6 +-
>  drivers/net/ixgbe/ixgbe_ethdev.h                  |  2 +-
>  drivers/net/ixgbe/ixgbe_pf.c                      | 15 ++--
>  lib/librte_eal/bsdapp/eal/eal.c                   | 70 +++++++++++-----
>  lib/librte_eal/linuxapp/eal/eal.c                 | 97
> +++++++++++++++--------
>  lib/librte_eal/linuxapp/eal/eal_hugepage_info.c   | 37 ++++++---
>  lib/librte_eal/linuxapp/eal/eal_interrupts.c      | 25 ++++--
>  lib/librte_ether/rte_ethdev.c                     | 42 +++++++---
>  lib/librte_ether/rte_ethdev.h                     |  4 +-
>  lib/librte_eventdev/rte_eventdev_pmd_pci.h        |  8 +-
>  lib/librte_eventdev/rte_eventdev_pmd_vdev.h       |  8 +-
>  lib/librte_kni/rte_kni.c                          | 18 +++--
>  lib/librte_kni/rte_kni_fifo.h                     | 11 ++-
>  24 files changed, 396 insertions(+), 143 deletions(-)
>
> --
> 1.8.3.1
>
>


-- 

*Arnon Warshavsky*
*Qwilt | work: +972-72-2221634 | mobile: +972-50-8583058 | arnon@qwilt.com
<arnon@qwilt.com>*

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Arnon Warshavsky]

On Tue, Apr 24, 2018 at 9:44 AM, Arnon Warshavsky <arnon@qwilt.com> wrote:

> please ignore this patchset.
> The v6 formatting is messed up. resending
> Sorry for the mess
>

My bad. Ignore the ignore request
Its gmail thread aggregation view that made me think I mixed v5 and v6.
thanks
/Arnon

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Thomas Monjalon]

Hi,

24/04/2018 08:41, Arnon Warshavsky:
> The purpose of this patch series 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 teardown or other mitigation of the event.
> 
> This patch modifies the majority of rte_panic calls
> under lib and drivers, and replaces them with a log message
> and an error return code according to context,
> 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 temination.
>   These should be handled by the driver maintainers..
> - local void functions with no api were changed to retrun a value
>   where needed
> - No change took place in example and test files
> - No change took place for debug assertions calling panic
> - A new function was added to devtools/checkpatches.sh
>   in order to prevent new additions of calls to rte_panic
>   under lib and drivers.
> 
> Keep calm and don't panic

What happened to this patchset?

This is definitely an improvement. We must remove rte_panic from libs.
Arnon, are you still available to rebase this patchset in preparation
of 19.08? Or someone else?

What are the required API breakages? I see one in ethdev which requires
a deprecation notice to be sent for publishing in 19.05.

Is there more rte_panic to remove?

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Thomas Monjalon]

Hi,

24/04/2018 08:41, Arnon Warshavsky:
> The purpose of this patch series 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 teardown or other mitigation of the event.
> 
> This patch modifies the majority of rte_panic calls
> under lib and drivers, and replaces them with a log message
> and an error return code according to context,
> 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 temination.
>   These should be handled by the driver maintainers..
> - local void functions with no api were changed to retrun a value
>   where needed
> - No change took place in example and test files
> - No change took place for debug assertions calling panic
> - A new function was added to devtools/checkpatches.sh
>   in order to prevent new additions of calls to rte_panic
>   under lib and drivers.
> 
> Keep calm and don't panic

What happened to this patchset?

This is definitely an improvement. We must remove rte_panic from libs.
Arnon, are you still available to rebase this patchset in preparation
of 19.08? Or someone else?

What are the required API breakages? I see one in ethdev which requires
a deprecation notice to be sent for publishing in 19.05.

Is there more rte_panic to remove?

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Arnon Warshavsky]

>
>
> What happened to this patchset?
>
> This is definitely an improvement. We must remove rte_panic from libs.
> Arnon, are you still available to rebase this patchset in preparation
> of 19.08? Or someone else?
>
> What are the required API breakages? I see one in ethdev which requires
> a deprecation notice to be sent for publishing in 19.05.
>
> Is there more rte_panic to remove?
>
>
>
Hi,
I should be able to address this for 19.08, at least the straight forward
parts.
I need to touch base again with this patchset later this week,
to see what changed since then, and see what deprecation notices are
required.
I would like to address the ones that are a direct part of the
initialization sequence,
and mostly change functions and their callers from void to return a value
that propagates upwards.
The 2nd kind under lib that I wanted to remove at the time are the ones
that live in threads and I would not like to handle them now.
Given a 3rd kind that is found inside PMDs that may panic during callbacks,
the former poses a similar challenge of managing the device state after a
panic event which is not trivial,
and tmho deserves either a separate patchset or a defeat recognition.

In this respect , In addition to removing the ones from the initialization
sequence,
I would like to revive my original proposal to add a callback registration
to the panic event.
As I do not expect all the PMD callback panics to disappear completely,
I still need to allow the running process to do some kind of orderly
tear-down to other modules when possible.

Does this sound ok for 19.08?

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Arnon Warshavsky]

>
>
> What happened to this patchset?
>
> This is definitely an improvement. We must remove rte_panic from libs.
> Arnon, are you still available to rebase this patchset in preparation
> of 19.08? Or someone else?
>
> What are the required API breakages? I see one in ethdev which requires
> a deprecation notice to be sent for publishing in 19.05.
>
> Is there more rte_panic to remove?
>
>
>
Hi,
I should be able to address this for 19.08, at least the straight forward
parts.
I need to touch base again with this patchset later this week,
to see what changed since then, and see what deprecation notices are
required.
I would like to address the ones that are a direct part of the
initialization sequence,
and mostly change functions and their callers from void to return a value
that propagates upwards.
The 2nd kind under lib that I wanted to remove at the time are the ones
that live in threads and I would not like to handle them now.
Given a 3rd kind that is found inside PMDs that may panic during callbacks,
the former poses a similar challenge of managing the device state after a
panic event which is not trivial,
and tmho deserves either a separate patchset or a defeat recognition.

In this respect , In addition to removing the ones from the initialization
sequence,
I would like to revive my original proposal to add a callback registration
to the panic event.
As I do not expect all the PMD callback panics to disappear completely,
I still need to allow the running process to do some kind of orderly
tear-down to other modules when possible.

Does this sound ok for 19.08?

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Thomas Monjalon]

21/04/2019 21:16, Arnon Warshavsky:
> Hi,
> I should be able to address this for 19.08, at least the straight forward
> parts.
> I need to touch base again with this patchset later this week,
> to see what changed since then, and see what deprecation notices are
> required.
> I would like to address the ones that are a direct part of the
> initialization sequence,
> and mostly change functions and their callers from void to return a value
> that propagates upwards.
> The 2nd kind under lib that I wanted to remove at the time are the ones
> that live in threads and I would not like to handle them now.
> Given a 3rd kind that is found inside PMDs that may panic during callbacks,
> the former poses a similar challenge of managing the device state after a
> panic event which is not trivial,
> and tmho deserves either a separate patchset or a defeat recognition.
> 
> In this respect , In addition to removing the ones from the initialization
> sequence,
> I would like to revive my original proposal to add a callback registration
> to the panic event.
> As I do not expect all the PMD callback panics to disappear completely,
> I still need to allow the running process to do some kind of orderly
> tear-down to other modules when possible.
> 
> Does this sound ok for 19.08?

That would be great to get some progress in 19.08.
Do not hesitate to split in several patchsets,
getting easy ones first and open discussions other ones.

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Thomas Monjalon]

21/04/2019 21:16, Arnon Warshavsky:
> Hi,
> I should be able to address this for 19.08, at least the straight forward
> parts.
> I need to touch base again with this patchset later this week,
> to see what changed since then, and see what deprecation notices are
> required.
> I would like to address the ones that are a direct part of the
> initialization sequence,
> and mostly change functions and their callers from void to return a value
> that propagates upwards.
> The 2nd kind under lib that I wanted to remove at the time are the ones
> that live in threads and I would not like to handle them now.
> Given a 3rd kind that is found inside PMDs that may panic during callbacks,
> the former poses a similar challenge of managing the device state after a
> panic event which is not trivial,
> and tmho deserves either a separate patchset or a defeat recognition.
> 
> In this respect , In addition to removing the ones from the initialization
> sequence,
> I would like to revive my original proposal to add a callback registration
> to the panic event.
> As I do not expect all the PMD callback panics to disappear completely,
> I still need to allow the running process to do some kind of orderly
> tear-down to other modules when possible.
> 
> Does this sound ok for 19.08?

That would be great to get some progress in 19.08.
Do not hesitate to split in several patchsets,
getting easy ones first and open discussions other ones.

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Thomas Monjalon]

24/04/2018 08:41, Arnon Warshavsky:
> The purpose of this patch series 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 teardown or other mitigation of the event.
> 
> This patch modifies the majority of rte_panic calls
> under lib and drivers, and replaces them with a log message
> and an error return code according to context,
> 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 temination.
>   These should be handled by the driver maintainers..
> - local void functions with no api were changed to retrun a value
>   where needed
> - No change took place in example and test files
> - No change took place for debug assertions calling panic

I did a status of rte_panic/rte_exit calls in libs.

There are a lot of cleanups to do in EAL.
We may apply the same kind of solution for Linux, FreeBSD and Windows.

The status is described below in a kind of call tree:

librte_eal:

    int rte_eal_init
        rte_panic
        void rte_config_init
            rte_panic
            void rte_eal_config_create
                rte_exit
                rte_panic
            void rte_eal_config_attach
                rte_panic
            void rte_eal_config_reattach
                rte_panic
        void eal_thread_init_master
            rte_panic
        -> internal change
    
    int rte_eal_remote_launch
            rte_panic
                -> add a return code
    
    eal_thread_loop
        rte_panic
            -> abort thread?
    eal_intr_thread_main
        rte_panic
            -> abort thread?
    
    int get_hugepage_dir
        rte_panic
            -> no public API
    
    uint64_t rte_get_timer_cycles
    uint64_t rte_get_hpet_hz
    uint64_t rte_get_hpet_cycles
        rte_panic
            -> return 0 / no API change
    
librte_mempool:
    void rte_mempool_*
        RTE_LIBRTE_MEMPOOL_DEBUG
            rte_panic

librte_mbuf:
    void rte_mbuf_sanity_check
        rte_panic

librte_lpm:
    RTE_LIBRTE_LPM_DEBUG
        VERIFY_DEPTH
            rte_panic

librte_acl:
    RTE_ACL_VERIFY
        rte_panic
            -> debug check? / no API change?

librte_sched:
    void debug_check_queue_slab
        rte_panic
            -> debug assert

void rte_metrics_init
        rte_exit
            -> API breakage

librte_kni:
    struct rte_kni *rte_kni_alloc
        void kni_fifo_init
            rte_panic
                -> no public API change

librte_eventdev:
    struct rte_eventdev *rte_event_pmd_vdev_init
        rte_panic
            -> no API change
    int rte_event_pmd_pci_probe
        rte_panic
            -> no API change

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Thomas Monjalon]

24/04/2018 08:41, Arnon Warshavsky:
> The purpose of this patch series 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 teardown or other mitigation of the event.
> 
> This patch modifies the majority of rte_panic calls
> under lib and drivers, and replaces them with a log message
> and an error return code according to context,
> 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 temination.
>   These should be handled by the driver maintainers..
> - local void functions with no api were changed to retrun a value
>   where needed
> - No change took place in example and test files
> - No change took place for debug assertions calling panic

I did a status of rte_panic/rte_exit calls in libs.

There are a lot of cleanups to do in EAL.
We may apply the same kind of solution for Linux, FreeBSD and Windows.

The status is described below in a kind of call tree:

librte_eal:

    int rte_eal_init
        rte_panic
        void rte_config_init
            rte_panic
            void rte_eal_config_create
                rte_exit
                rte_panic
            void rte_eal_config_attach
                rte_panic
            void rte_eal_config_reattach
                rte_panic
        void eal_thread_init_master
            rte_panic
        -> internal change
    
    int rte_eal_remote_launch
            rte_panic
                -> add a return code
    
    eal_thread_loop
        rte_panic
            -> abort thread?
    eal_intr_thread_main
        rte_panic
            -> abort thread?
    
    int get_hugepage_dir
        rte_panic
            -> no public API
    
    uint64_t rte_get_timer_cycles
    uint64_t rte_get_hpet_hz
    uint64_t rte_get_hpet_cycles
        rte_panic
            -> return 0 / no API change
    
librte_mempool:
    void rte_mempool_*
        RTE_LIBRTE_MEMPOOL_DEBUG
            rte_panic

librte_mbuf:
    void rte_mbuf_sanity_check
        rte_panic

librte_lpm:
    RTE_LIBRTE_LPM_DEBUG
        VERIFY_DEPTH
            rte_panic

librte_acl:
    RTE_ACL_VERIFY
        rte_panic
            -> debug check? / no API change?

librte_sched:
    void debug_check_queue_slab
        rte_panic
            -> debug assert

void rte_metrics_init
        rte_exit
            -> API breakage

librte_kni:
    struct rte_kni *rte_kni_alloc
        void kni_fifo_init
            rte_panic
                -> no public API change

librte_eventdev:
    struct rte_eventdev *rte_event_pmd_vdev_init
        rte_panic
            -> no API change
    int rte_event_pmd_pci_probe
        rte_panic
            -> no API change

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[David Marchand]

On Wed, May 8, 2019 at 1:16 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> I did a status of rte_panic/rte_exit calls in libs.
>
> There are a lot of cleanups to do in EAL.
> We may apply the same kind of solution for Linux, FreeBSD and Windows.
>
>
Thanks for the list.
Only checked at the mbuf part for now.


librte_mbuf:
>     void rte_mbuf_sanity_check
>         rte_panic
>

This function does panic yes, but we have a non-panicking equivalent
version of this function: rte_mbuf_check().
When RTE_LIBRTE_MBUF_DEBUG is set, a lot of internals in rte_mbuf call
_rte_mbuf_sanity_check which calls rte_mbuf_sanity_check.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[David Marchand]

On Wed, May 8, 2019 at 1:16 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> I did a status of rte_panic/rte_exit calls in libs.
>
> There are a lot of cleanups to do in EAL.
> We may apply the same kind of solution for Linux, FreeBSD and Windows.
>
>
Thanks for the list.
Only checked at the mbuf part for now.


librte_mbuf:
>     void rte_mbuf_sanity_check
>         rte_panic
>

This function does panic yes, but we have a non-panicking equivalent
version of this function: rte_mbuf_check().
When RTE_LIBRTE_MBUF_DEBUG is set, a lot of internals in rte_mbuf call
_rte_mbuf_sanity_check which calls rte_mbuf_sanity_check.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Burakov, Anatoly]

On 08-May-19 12:15 PM, Thomas Monjalon wrote:
> 24/04/2018 08:41, Arnon Warshavsky:
>> The purpose of this patch series 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 teardown or other mitigation of the event.
>>
>> This patch modifies the majority of rte_panic calls
>> under lib and drivers, and replaces them with a log message
>> and an error return code according to context,
>> 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 temination.
>>    These should be handled by the driver maintainers..
>> - local void functions with no api were changed to retrun a value
>>    where needed
>> - No change took place in example and test files
>> - No change took place for debug assertions calling panic
> 
> I did a status of rte_panic/rte_exit calls in libs.
> 
> There are a lot of cleanups to do in EAL.
> We may apply the same kind of solution for Linux, FreeBSD and Windows.
> 
> The status is described below in a kind of call tree:
> 

<snip>

> librte_mempool:
>      void rte_mempool_*
>          RTE_LIBRTE_MEMPOOL_DEBUG
>              rte_panic
> 

(and other similar places)

Could an argument not be made that when debugging options are enabled, 
having rte_panic there is actually useful?

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Burakov, Anatoly]

On 08-May-19 12:15 PM, Thomas Monjalon wrote:
> 24/04/2018 08:41, Arnon Warshavsky:
>> The purpose of this patch series 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 teardown or other mitigation of the event.
>>
>> This patch modifies the majority of rte_panic calls
>> under lib and drivers, and replaces them with a log message
>> and an error return code according to context,
>> 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 temination.
>>    These should be handled by the driver maintainers..
>> - local void functions with no api were changed to retrun a value
>>    where needed
>> - No change took place in example and test files
>> - No change took place for debug assertions calling panic
> 
> I did a status of rte_panic/rte_exit calls in libs.
> 
> There are a lot of cleanups to do in EAL.
> We may apply the same kind of solution for Linux, FreeBSD and Windows.
> 
> The status is described below in a kind of call tree:
> 

<snip>

> librte_mempool:
>      void rte_mempool_*
>          RTE_LIBRTE_MEMPOOL_DEBUG
>              rte_panic
> 

(and other similar places)

Could an argument not be made that when debugging options are enabled, 
having rte_panic there is actually useful?

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Thomas Monjalon]

09/05/2019 14:05, Burakov, Anatoly:
> On 08-May-19 12:15 PM, Thomas Monjalon wrote:
> > 24/04/2018 08:41, Arnon Warshavsky:
> >> The purpose of this patch series 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 teardown or other mitigation of the event.
> >>
> >> This patch modifies the majority of rte_panic calls
> >> under lib and drivers, and replaces them with a log message
> >> and an error return code according to context,
> >> 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 temination.
> >>    These should be handled by the driver maintainers..
> >> - local void functions with no api were changed to retrun a value
> >>    where needed
> >> - No change took place in example and test files
> >> - No change took place for debug assertions calling panic
> > 
> > I did a status of rte_panic/rte_exit calls in libs.
> > 
> > There are a lot of cleanups to do in EAL.
> > We may apply the same kind of solution for Linux, FreeBSD and Windows.
> > 
> > The status is described below in a kind of call tree:
> 
> <snip>
> 
> > librte_mempool:
> >      void rte_mempool_*
> >          RTE_LIBRTE_MEMPOOL_DEBUG
> >              rte_panic
> > 
> 
> (and other similar places)
> 
> Could an argument not be made that when debugging options are enabled, 
> having rte_panic there is actually useful?

Yes I think we can keep them.
In order to make it clear, we could replace them
with RTE_ASSERT or RTE_VERIFY (which calls rte_panic).

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Thomas Monjalon]

09/05/2019 14:05, Burakov, Anatoly:
> On 08-May-19 12:15 PM, Thomas Monjalon wrote:
> > 24/04/2018 08:41, Arnon Warshavsky:
> >> The purpose of this patch series 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 teardown or other mitigation of the event.
> >>
> >> This patch modifies the majority of rte_panic calls
> >> under lib and drivers, and replaces them with a log message
> >> and an error return code according to context,
> >> 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 temination.
> >>    These should be handled by the driver maintainers..
> >> - local void functions with no api were changed to retrun a value
> >>    where needed
> >> - No change took place in example and test files
> >> - No change took place for debug assertions calling panic
> > 
> > I did a status of rte_panic/rte_exit calls in libs.
> > 
> > There are a lot of cleanups to do in EAL.
> > We may apply the same kind of solution for Linux, FreeBSD and Windows.
> > 
> > The status is described below in a kind of call tree:
> 
> <snip>
> 
> > librte_mempool:
> >      void rte_mempool_*
> >          RTE_LIBRTE_MEMPOOL_DEBUG
> >              rte_panic
> > 
> 
> (and other similar places)
> 
> Could an argument not be made that when debugging options are enabled, 
> having rte_panic there is actually useful?

Yes I think we can keep them.
In order to make it clear, we could replace them
with RTE_ASSERT or RTE_VERIFY (which calls rte_panic).

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Thomas Monjalon]

Hi Arnon,

Any update about this work?
Would be good to merge these patches early in 19.08 release.
If you need help or more time for some of the patches,
do not hesitate to split the work.
We could merge obvious patches first.

Thanks


09/05/2019 15:16, Thomas Monjalon:
> 09/05/2019 14:05, Burakov, Anatoly:
> > On 08-May-19 12:15 PM, Thomas Monjalon wrote:
> > > 24/04/2018 08:41, Arnon Warshavsky:
> > >> The purpose of this patch series 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 teardown or other mitigation of the event.
> > >>
> > >> This patch modifies the majority of rte_panic calls
> > >> under lib and drivers, and replaces them with a log message
> > >> and an error return code according to context,
> > >> 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 temination.
> > >>    These should be handled by the driver maintainers..
> > >> - local void functions with no api were changed to retrun a value
> > >>    where needed
> > >> - No change took place in example and test files
> > >> - No change took place for debug assertions calling panic
> > > 
> > > I did a status of rte_panic/rte_exit calls in libs.
> > > 
> > > There are a lot of cleanups to do in EAL.
> > > We may apply the same kind of solution for Linux, FreeBSD and Windows.
> > > 
> > > The status is described below in a kind of call tree:
> > 
> > <snip>
> > 
> > > librte_mempool:
> > >      void rte_mempool_*
> > >          RTE_LIBRTE_MEMPOOL_DEBUG
> > >              rte_panic
> > > 
> > 
> > (and other similar places)
> > 
> > Could an argument not be made that when debugging options are enabled, 
> > having rte_panic there is actually useful?
> 
> Yes I think we can keep them.
> In order to make it clear, we could replace them
> with RTE_ASSERT or RTE_VERIFY (which calls rte_panic).

Re: [dpdk-dev] [PATCH v6 00/11] al: replace calls to rte_panic and refrain from new instances

[Arnon Warshavsky]

Hi,  I plan to work on it on the first week of June. Will be pretty much
off-grid till then.
thanks
/Arnon