[dpdk-dev] [PATCH] eal: remove non-thread panic calls from init sequence

[dpdk-dev] [PATCH] eal: remove non-thread panic calls from init sequence

[Arnon Warshavsky]

This patch changes some void functions to return a value,
so that the init sequence may tear down orderly
instead of calling panic.
The calls for launching core messaging threads were left in tact
in all 3 eal implementations.
This should be addressed in a different patchset.
There are still cases where the PMDs or message threads
may call panic with no context for the user.
For these I will submit a patch suggesting a callback registration,
allowing the user to register a context to be called
in case of a panic that takes place outside the init sequence.

Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
---
 lib/librte_eal/freebsd/eal/eal.c | 57 ++++++++++++++++++-------
 lib/librte_eal/linux/eal/eal.c   | 89 ++++++++++++++++++++++++++++------------
 2 files changed, 104 insertions(+), 42 deletions(-)

diff --git a/lib/librte_eal/freebsd/eal/eal.c b/lib/librte_eal/freebsd/eal/eal.c
index c6ac902..13fe4e6 100644
--- a/lib/librte_eal/freebsd/eal/eal.c
+++ b/lib/librte_eal/freebsd/eal/eal.c
@@ -215,7 +215,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;
@@ -228,56 +228,73 @@ enum rte_iova_mode
 
 	if (mem_cfg_fd < 0){
 		mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0600);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+					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);
+		RTE_LOG(ERR, EAL, "Cannot resize '%s' for rte_mem_config\n",
+				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 "
+		RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another primary ",
 				"process running?\n", 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(ERR, EAL, "Cannot mmap memory for rte_config\n");
+		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(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+					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 (mem_config == MAP_FAILED) {
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",
+				errno, strerror(errno));
+		return -1;
+	}
 
 	rte_config.mem_config = rte_mem_cfg_addr;
+
+	return 0;
 }
 
 /* Detect if we are a primary or a secondary process */
@@ -313,16 +330,26 @@ enum rte_proc_type_t
 
 	switch (rte_config.process_type){
 	case RTE_PROC_PRIMARY:
-		rte_eal_config_create();
+		if (rte_eal_config_create() < 0) {
+			RTE_LOG(ERR, EAL, "Failed to create config\n");
+			return -1;
+		}
 		break;
 	case RTE_PROC_SECONDARY:
-		rte_eal_config_attach();
+		if (rte_eal_config_attach() < 0) {
+			RTE_LOG(ERR, EAL, "Failed to attach config\n");
+			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");
+		RTE_LOG(ERR, EAL, "Invalid process type %d\n",
+				rte_config.process_type);
+		return -1;
 	}
+
+	return 0;
 }
 
 /* display usage */
diff --git a/lib/librte_eal/linux/eal/eal.c b/lib/librte_eal/linux/eal/eal.c
index 1613996..57abc4f 100644
--- a/lib/librte_eal/linux/eal/eal.c
+++ b/lib/librte_eal/linux/eal/eal.c
@@ -300,7 +300,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;
@@ -309,7 +309,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)
@@ -321,28 +321,35 @@ enum rte_iova_mode
 
 	if (mem_cfg_fd < 0){
 		mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0600);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+					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);
+		RTE_LOG(ERR, EAL, "Cannot resize '%s' for rte_mem_config\n",
+				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 "
+		RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another primary "
 				"process running?\n", 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");
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config\n");
+		return -1;
 	}
 	memcpy(rte_mem_cfg_addr, &early_mem_config, sizeof(early_mem_config));
 	rte_config.mem_config = rte_mem_cfg_addr;
@@ -353,10 +360,11 @@ enum rte_iova_mode
 
 	rte_config.mem_config->dma_maskbits = 0;
 
+	return 0;
 }
 
 /* attach to an existing shared memory config */
-static void
+static int
 rte_eal_config_attach(void)
 {
 	struct rte_mem_config *mem_config;
@@ -364,33 +372,40 @@ 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);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+					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) {
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",
+				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;
@@ -403,18 +418,22 @@ enum rte_iova_mode
 			sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
 			mem_cfg_fd, 0);
 	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
-		if (mem_config != MAP_FAILED)
+		if (mem_config != MAP_FAILED) {
 			/* errno is stale, don't use */
-			rte_panic("Cannot mmap memory for rte_config at [%p], got [%p]"
+			RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config at [%p], got [%p]"
 				  " - please use '--base-virtaddr' option\n",
 				  rte_mem_cfg_addr, mem_config);
-		else
-			rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
-				  errno, strerror(errno));
+			return -1;
+		}
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",
+			  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 */
@@ -461,26 +480,39 @@ 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() < 0) {
+			RTE_LOG(ERR, EAL, "Failed to create config\n");
+			return -1;
+		}
 		eal_update_mem_config();
 		break;
 	case RTE_PROC_SECONDARY:
-		rte_eal_config_attach();
+		if (rte_eal_config_attach() < 0) {
+			RTE_LOG(ERR, EAL, "Failed to attach config\n");
+			return -1;
+		}
 		rte_eal_mcfg_wait_complete(rte_config.mem_config);
-		rte_eal_config_reattach();
+		if (rte_eal_config_reattach() < 0) {
+			RTE_LOG(ERR, EAL, "Failed to reattach config\n");
+			return -1;
+		}
 		eal_update_internal_config();
 		break;
 	case RTE_PROC_AUTO:
 	case RTE_PROC_INVALID:
-		rte_panic("Invalid process type\n");
+		RTE_LOG(ERR, EAL, "Invalid process type %d\n",
+				rte_config.process_type);
+		return -1;
 	}
+
+	return 0;
 }
 
 /* Unlocks hugepage directories that were locked by eal_hugepage_info_init */
@@ -998,7 +1030,10 @@ static void rte_eal_init_alert(const char *msg)
 		return -1;
 	}
 
-	rte_config_init();
+	if (rte_config_init() < 0) {
+		rte_eal_init_alert("Cannot init config");
+		return -1;
+	}
 
 	if (rte_eal_intr_init() < 0) {
 		rte_eal_init_alert("Cannot init interrupt-handling thread");
-- 
1.8.3.1

[dpdk-dev] [PATCH v2] eal: remove non-thread panic calls from init sequence

[Arnon Warshavsky]

This patch changes some void functions to return a value,
so that the init sequence may tear down orderly
instead of calling panic.
The calls for launching core messaging threads were left in tact
in all 3 eal implementations.
This should be addressed in a different patchset.
There are still cases where the PMDs or message threads
may call panic with no context for the user.
For these I will submit a patch suggesting a callback registration,
allowing the user to register a context to be called
in case of a panic that takes place outside the init sequence.

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

-v2 fix freebsd compilation

 lib/librte_eal/freebsd/eal/eal.c | 59 ++++++++++++++++++--------
 lib/librte_eal/linux/eal/eal.c   | 89 ++++++++++++++++++++++++++++------------
 2 files changed, 105 insertions(+), 43 deletions(-)

diff --git a/lib/librte_eal/freebsd/eal/eal.c b/lib/librte_eal/freebsd/eal/eal.c
index c6ac902..d4a5c08 100644
--- a/lib/librte_eal/freebsd/eal/eal.c
+++ b/lib/librte_eal/freebsd/eal/eal.c
@@ -215,7 +215,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;
@@ -224,60 +224,77 @@ 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, 0600);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+					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);
+		RTE_LOG(ERR, EAL, "Cannot resize '%s' for rte_mem_config\n",
+				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 "
+		RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another primary ",
 				"process running?\n", 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(ERR, EAL, "Cannot mmap memory for rte_config\n");
+		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(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+					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 (mem_config == MAP_FAILED) {
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",
+				errno, strerror(errno));
+		return -1;
+	}
 
 	rte_config.mem_config = rte_mem_cfg_addr;
+
+	return 0;
 }
 
 /* Detect if we are a primary or a secondary process */
@@ -313,16 +330,26 @@ enum rte_proc_type_t
 
 	switch (rte_config.process_type){
 	case RTE_PROC_PRIMARY:
-		rte_eal_config_create();
+		if (rte_eal_config_create() < 0) {
+			RTE_LOG(ERR, EAL, "Failed to create config\n");
+			return -1;
+		}
 		break;
 	case RTE_PROC_SECONDARY:
-		rte_eal_config_attach();
+		if (rte_eal_config_attach() < 0) {
+			RTE_LOG(ERR, EAL, "Failed to attach config\n");
+			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");
+		RTE_LOG(ERR, EAL, "Invalid process type %d\n",
+				rte_config.process_type);
+		return -1;
 	}
+
+	return 0;
 }
 
 /* display usage */
diff --git a/lib/librte_eal/linux/eal/eal.c b/lib/librte_eal/linux/eal/eal.c
index 1613996..57abc4f 100644
--- a/lib/librte_eal/linux/eal/eal.c
+++ b/lib/librte_eal/linux/eal/eal.c
@@ -300,7 +300,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;
@@ -309,7 +309,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)
@@ -321,28 +321,35 @@ enum rte_iova_mode
 
 	if (mem_cfg_fd < 0){
 		mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0600);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+					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);
+		RTE_LOG(ERR, EAL, "Cannot resize '%s' for rte_mem_config\n",
+				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 "
+		RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another primary "
 				"process running?\n", 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");
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config\n");
+		return -1;
 	}
 	memcpy(rte_mem_cfg_addr, &early_mem_config, sizeof(early_mem_config));
 	rte_config.mem_config = rte_mem_cfg_addr;
@@ -353,10 +360,11 @@ enum rte_iova_mode
 
 	rte_config.mem_config->dma_maskbits = 0;
 
+	return 0;
 }
 
 /* attach to an existing shared memory config */
-static void
+static int
 rte_eal_config_attach(void)
 {
 	struct rte_mem_config *mem_config;
@@ -364,33 +372,40 @@ 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);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+					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) {
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",
+				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;
@@ -403,18 +418,22 @@ enum rte_iova_mode
 			sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
 			mem_cfg_fd, 0);
 	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
-		if (mem_config != MAP_FAILED)
+		if (mem_config != MAP_FAILED) {
 			/* errno is stale, don't use */
-			rte_panic("Cannot mmap memory for rte_config at [%p], got [%p]"
+			RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config at [%p], got [%p]"
 				  " - please use '--base-virtaddr' option\n",
 				  rte_mem_cfg_addr, mem_config);
-		else
-			rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
-				  errno, strerror(errno));
+			return -1;
+		}
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",
+			  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 */
@@ -461,26 +480,39 @@ 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() < 0) {
+			RTE_LOG(ERR, EAL, "Failed to create config\n");
+			return -1;
+		}
 		eal_update_mem_config();
 		break;
 	case RTE_PROC_SECONDARY:
-		rte_eal_config_attach();
+		if (rte_eal_config_attach() < 0) {
+			RTE_LOG(ERR, EAL, "Failed to attach config\n");
+			return -1;
+		}
 		rte_eal_mcfg_wait_complete(rte_config.mem_config);
-		rte_eal_config_reattach();
+		if (rte_eal_config_reattach() < 0) {
+			RTE_LOG(ERR, EAL, "Failed to reattach config\n");
+			return -1;
+		}
 		eal_update_internal_config();
 		break;
 	case RTE_PROC_AUTO:
 	case RTE_PROC_INVALID:
-		rte_panic("Invalid process type\n");
+		RTE_LOG(ERR, EAL, "Invalid process type %d\n",
+				rte_config.process_type);
+		return -1;
 	}
+
+	return 0;
 }
 
 /* Unlocks hugepage directories that were locked by eal_hugepage_info_init */
@@ -998,7 +1030,10 @@ static void rte_eal_init_alert(const char *msg)
 		return -1;
 	}
 
-	rte_config_init();
+	if (rte_config_init() < 0) {
+		rte_eal_init_alert("Cannot init config");
+		return -1;
+	}
 
 	if (rte_eal_intr_init() < 0) {
 		rte_eal_init_alert("Cannot init interrupt-handling thread");
-- 
1.8.3.1

[dpdk-dev] [PATCH v3] eal: remove non-thread panic calls from init sequence

[Arnon Warshavsky]

This patch changes some void functions to return a value,
so that the init sequence may tear down orderly
instead of calling panic.
The calls for launching core messaging threads were left in tact
in all 3 eal implementations.
This should be addressed in a different patchset.
There are still cases where the PMDs or message threads
may call panic with no context for the user.
For these I will submit a patch suggesting a callback registration,
allowing the user to register a context to be called
in case of a panic that takes place outside the init sequence.

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

-v2 fix freebsd compilation
-v3 fix freebsd compilation, haste from the devil

 lib/librte_eal/freebsd/eal/eal.c | 61 +++++++++++++++++++--------
 lib/librte_eal/linux/eal/eal.c   | 89 ++++++++++++++++++++++++++++------------
 2 files changed, 106 insertions(+), 44 deletions(-)

diff --git a/lib/librte_eal/freebsd/eal/eal.c b/lib/librte_eal/freebsd/eal/eal.c
index c6ac902..04a1033 100644
--- a/lib/librte_eal/freebsd/eal/eal.c
+++ b/lib/librte_eal/freebsd/eal/eal.c
@@ -215,7 +215,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;
@@ -224,60 +224,77 @@ 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, 0600);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+					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);
+		RTE_LOG(ERR, EAL, "Cannot resize '%s' for rte_mem_config\n",
+				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 "
+		RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another primary "
 				"process running?\n", 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(ERR, EAL, "Cannot mmap memory for rte_config\n");
+		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(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+					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) {
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",
+				errno, strerror(errno));
+		return -1;
+	}
 
 	rte_config.mem_config = rte_mem_cfg_addr;
+
+	return 0;
 }
 
 /* Detect if we are a primary or a secondary process */
@@ -306,23 +323,33 @@ 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() < 0) {
+			RTE_LOG(ERR, EAL, "Failed to create config\n");
+			return -1;
+		}
 		break;
 	case RTE_PROC_SECONDARY:
-		rte_eal_config_attach();
+		if (rte_eal_config_attach() < 0) {
+			RTE_LOG(ERR, EAL, "Failed to attach config\n");
+			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");
+		RTE_LOG(ERR, EAL, "Invalid process type %d\n",
+				rte_config.process_type);
+		return -1;
 	}
+
+	return 0;
 }
 
 /* display usage */
diff --git a/lib/librte_eal/linux/eal/eal.c b/lib/librte_eal/linux/eal/eal.c
index 1613996..57abc4f 100644
--- a/lib/librte_eal/linux/eal/eal.c
+++ b/lib/librte_eal/linux/eal/eal.c
@@ -300,7 +300,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;
@@ -309,7 +309,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)
@@ -321,28 +321,35 @@ enum rte_iova_mode
 
 	if (mem_cfg_fd < 0){
 		mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0600);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+					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);
+		RTE_LOG(ERR, EAL, "Cannot resize '%s' for rte_mem_config\n",
+				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 "
+		RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another primary "
 				"process running?\n", 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");
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config\n");
+		return -1;
 	}
 	memcpy(rte_mem_cfg_addr, &early_mem_config, sizeof(early_mem_config));
 	rte_config.mem_config = rte_mem_cfg_addr;
@@ -353,10 +360,11 @@ enum rte_iova_mode
 
 	rte_config.mem_config->dma_maskbits = 0;
 
+	return 0;
 }
 
 /* attach to an existing shared memory config */
-static void
+static int
 rte_eal_config_attach(void)
 {
 	struct rte_mem_config *mem_config;
@@ -364,33 +372,40 @@ 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);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+					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) {
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",
+				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;
@@ -403,18 +418,22 @@ enum rte_iova_mode
 			sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
 			mem_cfg_fd, 0);
 	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
-		if (mem_config != MAP_FAILED)
+		if (mem_config != MAP_FAILED) {
 			/* errno is stale, don't use */
-			rte_panic("Cannot mmap memory for rte_config at [%p], got [%p]"
+			RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config at [%p], got [%p]"
 				  " - please use '--base-virtaddr' option\n",
 				  rte_mem_cfg_addr, mem_config);
-		else
-			rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
-				  errno, strerror(errno));
+			return -1;
+		}
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",
+			  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 */
@@ -461,26 +480,39 @@ 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() < 0) {
+			RTE_LOG(ERR, EAL, "Failed to create config\n");
+			return -1;
+		}
 		eal_update_mem_config();
 		break;
 	case RTE_PROC_SECONDARY:
-		rte_eal_config_attach();
+		if (rte_eal_config_attach() < 0) {
+			RTE_LOG(ERR, EAL, "Failed to attach config\n");
+			return -1;
+		}
 		rte_eal_mcfg_wait_complete(rte_config.mem_config);
-		rte_eal_config_reattach();
+		if (rte_eal_config_reattach() < 0) {
+			RTE_LOG(ERR, EAL, "Failed to reattach config\n");
+			return -1;
+		}
 		eal_update_internal_config();
 		break;
 	case RTE_PROC_AUTO:
 	case RTE_PROC_INVALID:
-		rte_panic("Invalid process type\n");
+		RTE_LOG(ERR, EAL, "Invalid process type %d\n",
+				rte_config.process_type);
+		return -1;
 	}
+
+	return 0;
 }
 
 /* Unlocks hugepage directories that were locked by eal_hugepage_info_init */
@@ -998,7 +1030,10 @@ static void rte_eal_init_alert(const char *msg)
 		return -1;
 	}
 
-	rte_config_init();
+	if (rte_config_init() < 0) {
+		rte_eal_init_alert("Cannot init config");
+		return -1;
+	}
 
 	if (rte_eal_intr_init() < 0) {
 		rte_eal_init_alert("Cannot init interrupt-handling thread");
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v3] eal: remove non-thread panic calls from init sequence

[David Marchand]

Hello,

Thanks for looking at this.

First round of review, there is still something fishy about mem_cfg_fd in
secondary process implementation for Linux to me.
Maybe Anatoly can review this patch as well.


On Mon, Jun 3, 2019 at 9:21 AM Arnon Warshavsky <arnon@qwilt.com> wrote:

> This patch changes some void functions to return a value,
> so that the init sequence may tear down orderly
> instead of calling panic.
>

The part below can go after a --- marker, this is more a comment for the
work in progress rather than something to put in this patch commitlog.

The calls for launching core messaging threads were left in tact
> in all 3 eal implementations.
> This should be addressed in a different patchset.
> There are still cases where the PMDs or message threads
> may call panic with no context for the user.
> For these I will submit a patch suggesting a callback registration,
> allowing the user to register a context to be called
> in case of a panic that takes place outside the init sequence.
>

Not sure I understand what you want to do, but at least this patch is a
first step.


> Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
> ---
>
> -v2 fix freebsd compilation
> -v3 fix freebsd compilation, haste from the devil
>
>  lib/librte_eal/freebsd/eal/eal.c | 61 +++++++++++++++++++--------
>  lib/librte_eal/linux/eal/eal.c   | 89
> ++++++++++++++++++++++++++++------------
>  2 files changed, 106 insertions(+), 44 deletions(-)
>
> diff --git a/lib/librte_eal/freebsd/eal/eal.c
> b/lib/librte_eal/freebsd/eal/eal.c
> index c6ac902..04a1033 100644
> --- a/lib/librte_eal/freebsd/eal/eal.c
> +++ b/lib/librte_eal/freebsd/eal/eal.c
> @@ -215,7 +215,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;
> @@ -224,60 +224,77 @@ 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, 0600);
> -               if (mem_cfg_fd < 0)
> -                       rte_panic("Cannot open '%s' for rte_mem_config\n",
> pathname);
> +               if (mem_cfg_fd < 0) {
> +                       RTE_LOG(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                                       pathname);
>

Fix indent please, and in subsequent uses of RTE_LOG in this patch.


+                       return -1;
> +               }
>         }
>
>         retval = ftruncate(mem_cfg_fd, sizeof(*rte_config.mem_config));
>         if (retval < 0){
>                 close(mem_cfg_fd);
>

When we close this fd, we need to reset the variable pointing to it to -1
or subsequent call would reuse an incorrect mem_cfg_fd.
The problem is general in both linux and freebsd eal.c but something is
still not clear to me in how we are supposed to keep this fd opened or not
in normal successfull init case.


-               rte_panic("Cannot resize '%s' for rte_mem_config\n",
> pathname);
> +               RTE_LOG(ERR, EAL, "Cannot resize '%s' for
> rte_mem_config\n",
> +                               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 "
> +               RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another
> primary "
>                                 "process running?\n", 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(ERR, EAL, "Cannot mmap memory for rte_config\n");
> +               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(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                                       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) {
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config!
> error %i (%s)\n",
> +                               errno, strerror(errno));
>
+               return -1;
> +       }
>
>         rte_config.mem_config = rte_mem_cfg_addr;
> +
> +       return 0;
>  }
>
>  /* Detect if we are a primary or a secondary process */
> @@ -306,23 +323,33 @@ 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() < 0) {
> +                       RTE_LOG(ERR, EAL, "Failed to create config\n");
>

All error paths in rte_eal_config_create() have a log.
Adding one more here won't help and we have another one coming with the
rte_eal_init_alert later.

This comment goes for linux changes as well.


+                       return -1;
> +               }
>                 break;
>         case RTE_PROC_SECONDARY:
> -               rte_eal_config_attach();
> +               if (rte_eal_config_attach() < 0) {
> +                       RTE_LOG(ERR, EAL, "Failed to attach config\n");
>

Idem no log needed.


+                       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");
> +               RTE_LOG(ERR, EAL, "Invalid process type %d\n",
> +                               rte_config.process_type);
> +               return -1;
>         }
> +
> +       return 0;
>  }
>
>  /* display usage */
> diff --git a/lib/librte_eal/linux/eal/eal.c
> b/lib/librte_eal/linux/eal/eal.c
> index 1613996..57abc4f 100644
> --- a/lib/librte_eal/linux/eal/eal.c
> +++ b/lib/librte_eal/linux/eal/eal.c
> @@ -300,7 +300,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;
> @@ -309,7 +309,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)
> @@ -321,28 +321,35 @@ enum rte_iova_mode
>
>         if (mem_cfg_fd < 0){
>                 mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0600);
> -               if (mem_cfg_fd < 0)
> -                       rte_panic("Cannot open '%s' for rte_mem_config\n",
> pathname);
> +               if (mem_cfg_fd < 0) {
> +                       RTE_LOG(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                                       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);
> +               RTE_LOG(ERR, EAL, "Cannot resize '%s' for
> rte_mem_config\n",
> +                               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 "
> +               RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another
> primary "
>                                 "process running?\n", 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");
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config\n");
> +               return -1;
>         }
>         memcpy(rte_mem_cfg_addr, &early_mem_config,
> sizeof(early_mem_config));
>         rte_config.mem_config = rte_mem_cfg_addr;
> @@ -353,10 +360,11 @@ enum rte_iova_mode
>
>         rte_config.mem_config->dma_maskbits = 0;
>
> +       return 0;
>  }
>
>  /* attach to an existing shared memory config */
> -static void
> +static int
>  rte_eal_config_attach(void)
>  {
>         struct rte_mem_config *mem_config;
> @@ -364,33 +372,40 @@ 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);
> -               if (mem_cfg_fd < 0)
> -                       rte_panic("Cannot open '%s' for rte_mem_config\n",
> pathname);
> +               if (mem_cfg_fd < 0) {
> +                       RTE_LOG(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                                       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) {
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config!
> error %i (%s)\n",
> +                               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;
> @@ -403,18 +418,22 @@ enum rte_iova_mode
>                         sizeof(*mem_config), PROT_READ | PROT_WRITE,
> MAP_SHARED,
>                         mem_cfg_fd, 0);
>         if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
> -               if (mem_config != MAP_FAILED)
> +               if (mem_config != MAP_FAILED) {
>                         /* errno is stale, don't use */
> -                       rte_panic("Cannot mmap memory for rte_config at
> [%p], got [%p]"
> +                       RTE_LOG(ERR, EAL, "Cannot mmap memory for
> rte_config at [%p], got [%p]"
>                                   " - please use '--base-virtaddr'
> option\n",
>                                   rte_mem_cfg_addr, mem_config);
> -               else
> -                       rte_panic("Cannot mmap memory for rte_config!
> error %i (%s)\n",
> -                                 errno, strerror(errno));
> +                       return -1;
> +               }
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config!
> error %i (%s)\n",
> +                         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 */
> @@ -461,26 +480,39 @@ 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() < 0) {
> +                       RTE_LOG(ERR, EAL, "Failed to create config\n");
> +                       return -1;
> +               }
>                 eal_update_mem_config();
>                 break;
>         case RTE_PROC_SECONDARY:
> -               rte_eal_config_attach();
> +               if (rte_eal_config_attach() < 0) {
> +                       RTE_LOG(ERR, EAL, "Failed to attach config\n");
> +                       return -1;
> +               }
>                 rte_eal_mcfg_wait_complete(rte_config.mem_config);
> -               rte_eal_config_reattach();
> +               if (rte_eal_config_reattach() < 0) {
> +                       RTE_LOG(ERR, EAL, "Failed to reattach config\n");
> +                       return -1;
> +               }
>                 eal_update_internal_config();
>                 break;
>         case RTE_PROC_AUTO:
>         case RTE_PROC_INVALID:
> -               rte_panic("Invalid process type\n");
> +               RTE_LOG(ERR, EAL, "Invalid process type %d\n",
> +                               rte_config.process_type);
> +               return -1;
>         }
> +
> +       return 0;
>  }
>
>  /* Unlocks hugepage directories that were locked by
> eal_hugepage_info_init */
> @@ -998,7 +1030,10 @@ static void rte_eal_init_alert(const char *msg)
>                 return -1;
>         }
>
> -       rte_config_init();
> +       if (rte_config_init() < 0) {
> +               rte_eal_init_alert("Cannot init config");
> +               return -1;
> +       }
>
>         if (rte_eal_intr_init() < 0) {
>                 rte_eal_init_alert("Cannot init interrupt-handling
> thread");
> --
> 1.8.3.1
>
>


-- 
David Marchand

Re: [dpdk-dev] [PATCH v3] eal: remove non-thread panic calls from init sequence

[Arnon Warshavsky]

Hi


>
> The part below can go after a --- marker, this is more a comment for the
> work in progress rather than something to put in this patch commitlog.
>
Ack

>
> The calls for launching core messaging threads were left in tact
>> in all 3 eal implementations.
>>
>

> For these I will submit a patch suggesting a callback registration,
>> allowing the user to register a context to be called
>> in case of a panic that takes place outside the init sequence.
>>
>
> Not sure I understand what you want to do, but at least this patch is a
> first step.
>
Promise to be less vague in the actual patch :)

>
>
>> +                       RTE_LOG(ERR, EAL, "Cannot open '%s' for
>> rte_mem_config\n",
>> +                                       pathname);
>>
>
> Fix indent please, and in subsequent uses of RTE_LOG in this patch.
>
Ack

>
>
>> When we close this fd, we need to reset the variable pointing to it to -1
> or subsequent call would reuse an incorrect mem_cfg_fd.
> The problem is general in both linux and freebsd eal.c but something is
> still not clear to me in how we are supposed to keep this fd opened or not
> in normal successfull init case.
>

In this case we are on the way out from the process in the shortest way out
so no subsequent calls expected , but I agree for the sake of good order.
Will zero those

>
> +               if (rte_eal_config_create() < 0) {
>>
> +                       RTE_LOG(ERR, EAL, "Failed to create config\n");
>>
>
> All error paths in rte_eal_config_create() have a log.
> Adding one more here won't help and we have another one coming with the
> rte_eal_init_alert later.
>
> This comment goes for linux changes as well.
>
Agree. Will fix that

thanks
/Arnon

[dpdk-dev] [PATCH v4] eal: remove non-thread panic calls from init sequence

[Arnon Warshavsky]

This patch changes some void functions to return a value,
so that the init sequence may tear down orderly
instead of calling panic.

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

The calls for launching core messaging threads were left in tact
in all 3 eal implementations.
This should be addressed in a different patchset.
There are still cases where the PMDs or message threads
may call panic with no context for the user.
For these I will submit a patch suggesting a callback registration,
allowing the user to register a context to be called
in case of a panic that takes place outside the init sequence.

-v2 fix freebsd compilation
-v3 fix freebsd compilation, haste from the devil
-v4 fix comments by David

 lib/librte_eal/freebsd/eal/eal.c | 59 ++++++++++++++++++--------
 lib/librte_eal/linux/eal/eal.c   | 91 +++++++++++++++++++++++++++-------------
 2 files changed, 103 insertions(+), 47 deletions(-)

diff --git a/lib/librte_eal/freebsd/eal/eal.c b/lib/librte_eal/freebsd/eal/eal.c
index c6ac902..4b4db3b 100644
--- a/lib/librte_eal/freebsd/eal/eal.c
+++ b/lib/librte_eal/freebsd/eal/eal.c
@@ -215,7 +215,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;
@@ -224,60 +224,79 @@ 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, 0600);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+				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_id = -1;
+		RTE_LOG(ERR, EAL, "Cannot resize '%s' for rte_mem_config\n",
+			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 "
+		mem_cfg_fd = -1;
+		RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another primary "
 				"process running?\n", 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(ERR, EAL, "Cannot mmap memory for rte_config\n");
+		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(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+				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) {
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",
+			errno, strerror(errno));
+		return -1;
+	}
 
 	rte_config.mem_config = rte_mem_cfg_addr;
+
+	return 0;
 }
 
 /* Detect if we are a primary or a secondary process */
@@ -306,23 +325,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() < 0)
+			return -1;
 		break;
 	case RTE_PROC_SECONDARY:
-		rte_eal_config_attach();
+		if (rte_eal_config_attach() < 0)
+			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");
+		RTE_LOG(ERR, EAL, "Invalid process type %d\n",
+			rte_config.process_type);
+		return -1;
 	}
+
+	return 0;
 }
 
 /* display usage */
diff --git a/lib/librte_eal/linux/eal/eal.c b/lib/librte_eal/linux/eal/eal.c
index 1613996..1ec264f 100644
--- a/lib/librte_eal/linux/eal/eal.c
+++ b/lib/librte_eal/linux/eal/eal.c
@@ -300,7 +300,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;
@@ -309,7 +309,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)
@@ -321,28 +321,37 @@ enum rte_iova_mode
 
 	if (mem_cfg_fd < 0){
 		mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0600);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+				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(ERR, EAL, "Cannot resize '%s' for rte_mem_config\n",
+			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(ERR, EAL, "Cannot create lock on '%s'. Is another primary "
+			"process running?\n", 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");
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config\n");
+		return -1;
 	}
 	memcpy(rte_mem_cfg_addr, &early_mem_config, sizeof(early_mem_config));
 	rte_config.mem_config = rte_mem_cfg_addr;
@@ -353,10 +362,11 @@ enum rte_iova_mode
 
 	rte_config.mem_config->dma_maskbits = 0;
 
+	return 0;
 }
 
 /* attach to an existing shared memory config */
-static void
+static int
 rte_eal_config_attach(void)
 {
 	struct rte_mem_config *mem_config;
@@ -364,33 +374,40 @@ 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);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+				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) {
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",
+			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;
@@ -403,18 +420,22 @@ enum rte_iova_mode
 			sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
 			mem_cfg_fd, 0);
 	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
-		if (mem_config != MAP_FAILED)
+		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);
-		else
-			rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
-				  errno, strerror(errno));
+			RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config at [%p], got [%p]"
+				" - please use '--base-virtaddr' option\n",
+				rte_mem_cfg_addr, mem_config);
+			return -1;
+		}
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",
+			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 */
@@ -461,26 +482,33 @@ 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() < 0)
+			return -1;
 		eal_update_mem_config();
 		break;
 	case RTE_PROC_SECONDARY:
-		rte_eal_config_attach();
+		if (rte_eal_config_attach() < 0)
+			return -1;
 		rte_eal_mcfg_wait_complete(rte_config.mem_config);
-		rte_eal_config_reattach();
+		if (rte_eal_config_reattach() < 0)
+			return -1;
 		eal_update_internal_config();
 		break;
 	case RTE_PROC_AUTO:
 	case RTE_PROC_INVALID:
-		rte_panic("Invalid process type\n");
+		RTE_LOG(ERR, EAL, "Invalid process type %d\n",
+			rte_config.process_type);
+		return -1;
 	}
+
+	return 0;
 }
 
 /* Unlocks hugepage directories that were locked by eal_hugepage_info_init */
@@ -998,7 +1026,10 @@ static void rte_eal_init_alert(const char *msg)
 		return -1;
 	}
 
-	rte_config_init();
+	if (rte_config_init() < 0) {
+		rte_eal_init_alert("Cannot init config");
+		return -1;
+	}
 
 	if (rte_eal_intr_init() < 0) {
 		rte_eal_init_alert("Cannot init interrupt-handling thread");
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v4] eal: remove non-thread panic calls from init sequence

[David Marchand]

On Tue, Jun 4, 2019 at 5:45 PM Arnon Warshavsky <arnon@qwilt.com> wrote:

> This patch changes some void functions to return a value,
> so that the init sequence may tear down orderly
> instead of calling panic.
>

All we care about in this patch are the panics wrt the shared configuration
init.
Can the commit title and description refer to this ?


> Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
> ---
>
> The calls for launching core messaging threads were left in tact
> in all 3 eal implementations.
> This should be addressed in a different patchset.
> There are still cases where the PMDs or message threads
> may call panic with no context for the user.
> For these I will submit a patch suggesting a callback registration,
> allowing the user to register a context to be called
> in case of a panic that takes place outside the init sequence.
>
> -v2 fix freebsd compilation
> -v3 fix freebsd compilation, haste from the devil
> -v4 fix comments by David
>
>  lib/librte_eal/freebsd/eal/eal.c | 59 ++++++++++++++++++--------
>  lib/librte_eal/linux/eal/eal.c   | 91
> +++++++++++++++++++++++++++-------------
>  2 files changed, 103 insertions(+), 47 deletions(-)
>
> diff --git a/lib/librte_eal/freebsd/eal/eal.c
> b/lib/librte_eal/freebsd/eal/eal.c
> index c6ac902..4b4db3b 100644
> --- a/lib/librte_eal/freebsd/eal/eal.c
> +++ b/lib/librte_eal/freebsd/eal/eal.c
> @@ -215,7 +215,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;
> @@ -224,60 +224,79 @@ 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, 0600);
> -               if (mem_cfg_fd < 0)
> -                       rte_panic("Cannot open '%s' for rte_mem_config\n",
> pathname);
> +               if (mem_cfg_fd < 0) {
> +                       RTE_LOG(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                               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_id = -1;
>

I bet you did not compile FreeBSD :-).


+               RTE_LOG(ERR, EAL, "Cannot resize '%s' for rte_mem_config\n",
> +                       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 "
> +               mem_cfg_fd = -1;
> +               RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another
> primary "
>                                 "process running?\n", pathname);
>

Indent.

Not relevant to this patch, but I find it odd to truncate before trying to
take the lock.
Might be something to look at some day, anyway.. :-)


+               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(ERR, EAL, "Cannot mmap memory for rte_config\n");
> +               return -1;
>

We failed to mmap.
Since eal init is going to fail, we should not leave this fd open.


        }
>         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(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                               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);
>

At this point, we are in a secondary process.
We don't need to keep this fd open which is fine.
But at least for consistency, we should reset it to -1.


-       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(ERR, EAL, "Cannot mmap memory for rte_config!
> error %i (%s)\n",
> +                       errno, strerror(errno));
> +               return -1;
> +       }
>
>         rte_config.mem_config = rte_mem_cfg_addr;
> +
> +       return 0;
>  }
>
>  /* Detect if we are a primary or a secondary process */
> @@ -306,23 +325,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() < 0)
> +                       return -1;
>                 break;
>         case RTE_PROC_SECONDARY:
> -               rte_eal_config_attach();
> +               if (rte_eal_config_attach() < 0)
> +                       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");
> +               RTE_LOG(ERR, EAL, "Invalid process type %d\n",
> +                       rte_config.process_type);
> +               return -1;
>         }
> +
> +       return 0;
>  }
>
>  /* display usage */
> diff --git a/lib/librte_eal/linux/eal/eal.c
> b/lib/librte_eal/linux/eal/eal.c
> index 1613996..1ec264f 100644
> --- a/lib/librte_eal/linux/eal/eal.c
> +++ b/lib/librte_eal/linux/eal/eal.c
> @@ -300,7 +300,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;
> @@ -309,7 +309,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)
> @@ -321,28 +321,37 @@ enum rte_iova_mode
>
>         if (mem_cfg_fd < 0){
>                 mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0600);
> -               if (mem_cfg_fd < 0)
> -                       rte_panic("Cannot open '%s' for rte_mem_config\n",
> pathname);
> +               if (mem_cfg_fd < 0) {
> +                       RTE_LOG(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                               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(ERR, EAL, "Cannot resize '%s' for
> rte_mem_config\n",
> +                       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(ERR, EAL, "Cannot create lock on '%s'. Is another
> primary "
> +                       "process running?\n", 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");
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config\n");
> +               return -1;
>

Same comment than FreeBSD, mmap failed, we can close/reset mem_cfg_fd.


        }
>         memcpy(rte_mem_cfg_addr, &early_mem_config,
> sizeof(early_mem_config));
>         rte_config.mem_config = rte_mem_cfg_addr;
> @@ -353,10 +362,11 @@ enum rte_iova_mode
>
>         rte_config.mem_config->dma_maskbits = 0;
>
> +       return 0;
>  }
>
>  /* attach to an existing shared memory config */
> -static void
> +static int
>  rte_eal_config_attach(void)
>  {
>         struct rte_mem_config *mem_config;
> @@ -364,33 +374,40 @@ 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);
> -               if (mem_cfg_fd < 0)
> -                       rte_panic("Cannot open '%s' for rte_mem_config\n",
> pathname);
> +               if (mem_cfg_fd < 0) {
> +                       RTE_LOG(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                               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) {
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config!
> error %i (%s)\n",
> +                       errno, strerror(errno));
> +               return -1;
> +       }
>

We can close/reset mem_cfg_fd but in the error path _only_: we are going to
reuse it in rte_eal_config_reattach.


>         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;
> @@ -403,18 +420,22 @@ enum rte_iova_mode
>                         sizeof(*mem_config), PROT_READ | PROT_WRITE,
> MAP_SHARED,
>                         mem_cfg_fd, 0);
>         if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
> -               if (mem_config != MAP_FAILED)
> +               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);
> -               else
> -                       rte_panic("Cannot mmap memory for rte_config!
> error %i (%s)\n",
> -                                 errno, strerror(errno));
> +                       RTE_LOG(ERR, EAL, "Cannot mmap memory for
> rte_config at [%p], got [%p]"
> +                               " - please use '--base-virtaddr' option\n",
> +                               rte_mem_cfg_addr, mem_config);
> +                       return -1;
> +               }
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config!
> error %i (%s)\n",
> +                       errno, strerror(errno));
> +               return -1;
>         }
>         close(mem_cfg_fd);
>

Let's move this close and add a reset to -1 before the branch.


>         rte_config.mem_config = mem_config;
> +
> +       return 0;
>  }
>
>  /* Detect if we are a primary or a secondary process */
> @@ -461,26 +482,33 @@ 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() < 0)
> +                       return -1;
>                 eal_update_mem_config();
>                 break;
>         case RTE_PROC_SECONDARY:
> -               rte_eal_config_attach();
> +               if (rte_eal_config_attach() < 0)
> +                       return -1;
>                 rte_eal_mcfg_wait_complete(rte_config.mem_config);
> -               rte_eal_config_reattach();
> +               if (rte_eal_config_reattach() < 0)
> +                       return -1;
>                 eal_update_internal_config();
>                 break;
>         case RTE_PROC_AUTO:
>         case RTE_PROC_INVALID:
> -               rte_panic("Invalid process type\n");
> +               RTE_LOG(ERR, EAL, "Invalid process type %d\n",
> +                       rte_config.process_type);
> +               return -1;
>         }
> +
> +       return 0;
>  }
>
>  /* Unlocks hugepage directories that were locked by
> eal_hugepage_info_init */
> @@ -998,7 +1026,10 @@ static void rte_eal_init_alert(const char *msg)
>                 return -1;
>         }
>
> -       rte_config_init();
> +       if (rte_config_init() < 0) {
> +               rte_eal_init_alert("Cannot init config");
> +               return -1;
> +       }
>

This hunk is missing for FreeBSD.


>         if (rte_eal_intr_init() < 0) {
>                 rte_eal_init_alert("Cannot init interrupt-handling
> thread");
> --
> 1.8.3.1
>
>

-- 
David Marchand

Re: [dpdk-dev] [PATCH v4] eal: remove non-thread panic calls from init sequence

[Arnon Warshavsky]

On Wed, Jun 5, 2019 at 10:50 AM David Marchand <david.marchand@redhat.com>
wrote:

>
>
> On Tue, Jun 4, 2019 at 5:45 PM Arnon Warshavsky <arnon@qwilt.com> wrote:
>
>> This patch changes some void functions to return a value,
>> so that the init sequence may tear down orderly
>> instead of calling panic.
>>
>
> All we care about in this patch are the panics wrt the shared
> configuration init.
> Can the commit title and description refer to this ?
>
Will remove the non-thread from the title.
Can you be more specific/ offer your view per the description?

>
>
>> Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
>> ---
>>
>> The calls for launching core messaging threads were left in tact
>> in all 3 eal implementations.
>>
>>

>         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_id = -1;
>>
>
> I bet you did not compile FreeBSD :-).
> Yup...Not only haste is from the devil..Getting myself a fbsd vm now..
>
> +               RTE_LOG(ERR, EAL, "Cannot resize '%s' for
>> rte_mem_config\n",
>> +                       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 "
>> +               mem_cfg_fd = -1;
>> +               RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another
>> primary "
>>                                 "process running?\n", pathname);
>>
>
> Indent.
>
Ack

>
> Not relevant to this patch, but I find it odd to truncate before trying to
> take the lock.
> Might be something to look at some day, anyway.. :-)
>
>
> +               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(ERR, EAL, "Cannot mmap memory for rte_config\n");
>> +               return -1;
>>
>
> We failed to mmap.
> Since eal init is going to fail, we should not leave this fd open.
>
 Ok. In general I tried to stick with previous functionality by just
replacing the panic with the exit path,
rather than improve it, but this one is indeed straight forward.

>
>> +               }
>>         }
>>
>>         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);
>>
>
> At this point, we are in a secondary process.
> We don't need to keep this fd open which is fine.
> But at least for consistency, we should reset it to -1.
>
 Ack. Same as above

>
>
>>         if (rte_mem_cfg_addr == MAP_FAILED){
>> -               rte_panic("Cannot mmap memory for rte_config\n");
>> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config\n");
>> +               return -1;
>>
>
> Same comment than FreeBSD, mmap failed, we can close/reset mem_cfg_fd.
>
> Ack

>
>
>> +       }
>>
>
> We can close/reset mem_cfg_fd but in the error path _only_: we are going
> to reuse it in rte_eal_config_reattach.
>
>
>>         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;
>> @@ -403,18 +420,22 @@ enum rte_iova_mode
>>                         sizeof(*mem_config), PROT_READ | PROT_WRITE,
>> MAP_SHARED,
>>                         mem_cfg_fd, 0);
>>         if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
>> -               if (mem_config != MAP_FAILED)
>> +               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);
>> -               else
>> -                       rte_panic("Cannot mmap memory for rte_config!
>> error %i (%s)\n",
>> -                                 errno, strerror(errno));
>> +                       RTE_LOG(ERR, EAL, "Cannot mmap memory for
>> rte_config at [%p], got [%p]"
>> +                               " - please use '--base-virtaddr'
>> option\n",
>> +                               rte_mem_cfg_addr, mem_config);
>> +                       return -1;
>> +               }
>> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config!
>> error %i (%s)\n",
>> +                       errno, strerror(errno));
>> +               return -1;
>>         }
>>         close(mem_cfg_fd);
>>
>
> Let's move this close and add a reset to -1 before the branch.
>
Ack

>
>
>> -       rte_config_init();
>> +       if (rte_config_init() < 0) {
>> +               rte_eal_init_alert("Cannot init config");
>> +               return -1;
>> +       }
>>
>
> This hunk is missing for FreeBSD.
>


> mmmmm , not enough coffee in the process :-/
>
thanks

Re: [dpdk-dev] [PATCH v4] eal: remove non-thread panic calls from init sequence

[David Marchand]

On Wed, Jun 5, 2019 at 5:44 PM Arnon Warshavsky <arnon@qwilt.com> wrote:
> On Wed, Jun 5, 2019 at 10:50 AM David Marchand <david.marchand@redhat.com>
wrote:
>> On Tue, Jun 4, 2019 at 5:45 PM Arnon Warshavsky <arnon@qwilt.com> wrote:
>>> This patch changes some void functions to return a value,
>>> so that the init sequence may tear down orderly
>>> instead of calling panic.
>>
>> All we care about in this patch are the panics wrt the shared
configuration init.
>> Can the commit title and description refer to this ?
>
> Will remove the non-thread from the title.
> Can you be more specific/ offer your view per the description?

Looking at the git history, how about:
eal: do not panic on shared memory init

--
David Marchand

[dpdk-dev] [PATCH v5] eal: do not panic on shared memory init

[Arnon Warshavsky]

This patch changes some void functions to return a value,
so that the init sequence may tear down orderly
instead of calling panic.

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

The calls for launching core messaging threads were left in tact
in all 3 eal implementations.
This should be addressed in a different patchset.
There are still cases where the PMDs or message threads
may call panic with no context for the user.
For these I will submit a patch suggesting a callback registration,
allowing the user to register a context to be called
in case of a panic that takes place outside the init sequence.

-v2 fix freebsd compilation
-v3 fix freebsd compilation, haste from the devil
-v4 fix comments by David
-v5 fix more comments by david. rename patch

 lib/librte_eal/freebsd/eal/eal.c |  69 ++++++++++++++++++--------
 lib/librte_eal/linux/eal/eal.c   | 101 +++++++++++++++++++++++++++------------
 2 files changed, 120 insertions(+), 50 deletions(-)

diff --git a/lib/librte_eal/freebsd/eal/eal.c b/lib/librte_eal/freebsd/eal/eal.c
index c6ac902..f18f08e 100644
--- a/lib/librte_eal/freebsd/eal/eal.c
+++ b/lib/librte_eal/freebsd/eal/eal.c
@@ -215,7 +215,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;
@@ -224,60 +224,82 @@ 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, 0600);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+				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(ERR, EAL, "Cannot resize '%s' for rte_mem_config\n",
+			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(ERR, EAL, "Cannot create lock on '%s'. Is another primary "
+			"process running?\n", 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(ERR, EAL, "Cannot mmap memory for rte_config\n");
+		close(mem_cfg_fd);
+		mem_cfg_fd = -1;
+		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(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+				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");
+	mem_cfg_fd = -1;
+	if (rte_mem_cfg_addr == MAP_FAILED) {
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",
+			errno, strerror(errno));
+		return -1;
+	}
 
 	rte_config.mem_config = rte_mem_cfg_addr;
+
+	return 0;
 }
 
 /* Detect if we are a primary or a secondary process */
@@ -306,23 +328,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() < 0)
+			return -1;
 		break;
 	case RTE_PROC_SECONDARY:
-		rte_eal_config_attach();
+		if (rte_eal_config_attach() < 0)
+			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");
+		RTE_LOG(ERR, EAL, "Invalid process type %d\n",
+			rte_config.process_type);
+		return -1;
 	}
+
+	return 0;
 }
 
 /* display usage */
@@ -655,7 +683,10 @@ static void rte_eal_init_alert(const char *msg)
 		return -1;
 	}
 
-	rte_config_init();
+	if (rte_config_init() < 0) {
+		rte_eal_init_alert("Cannot init config");
+		return -1;
+	}
 
 	if (rte_eal_intr_init() < 0) {
 		rte_eal_init_alert("Cannot init interrupt-handling thread");
diff --git a/lib/librte_eal/linux/eal/eal.c b/lib/librte_eal/linux/eal/eal.c
index 1613996..224ba90 100644
--- a/lib/librte_eal/linux/eal/eal.c
+++ b/lib/librte_eal/linux/eal/eal.c
@@ -300,7 +300,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;
@@ -309,7 +309,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)
@@ -321,29 +321,41 @@ enum rte_iova_mode
 
 	if (mem_cfg_fd < 0){
 		mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0600);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+				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(ERR, EAL, "Cannot resize '%s' for rte_mem_config\n",
+			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(ERR, EAL, "Cannot create lock on '%s'. Is another primary "
+			"process running?\n", 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");
+		close(mem_cfg_fd);
+		mem_cfg_fd = -1;
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config\n");
+		return -1;
 	}
+
 	memcpy(rte_mem_cfg_addr, &early_mem_config, sizeof(early_mem_config));
 	rte_config.mem_config = rte_mem_cfg_addr;
 
@@ -353,10 +365,11 @@ enum rte_iova_mode
 
 	rte_config.mem_config->dma_maskbits = 0;
 
+	return 0;
 }
 
 /* attach to an existing shared memory config */
-static void
+static int
 rte_eal_config_attach(void)
 {
 	struct rte_mem_config *mem_config;
@@ -364,33 +377,42 @@ 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);
-		if (mem_cfg_fd < 0)
-			rte_panic("Cannot open '%s' for rte_mem_config\n", pathname);
+		if (mem_cfg_fd < 0) {
+			RTE_LOG(ERR, EAL, "Cannot open '%s' for rte_mem_config\n",
+				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) {
+		close(mem_cfg_fd);
+		mem_cfg_fd = -1;
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",
+			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;
@@ -402,19 +424,26 @@ enum rte_iova_mode
 	mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
 			sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
 			mem_cfg_fd, 0);
+
+	close(mem_cfg_fd);
+	mem_cfg_fd = -1;
+
 	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
-		if (mem_config != MAP_FAILED)
+		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);
-		else
-			rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
-				  errno, strerror(errno));
+			RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config at [%p], got [%p]"
+				" - please use '--base-virtaddr' option\n",
+				rte_mem_cfg_addr, mem_config);
+			return -1;
+		}
+		RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",
+			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 */
@@ -461,26 +490,33 @@ 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() < 0)
+			return -1;
 		eal_update_mem_config();
 		break;
 	case RTE_PROC_SECONDARY:
-		rte_eal_config_attach();
+		if (rte_eal_config_attach() < 0)
+			return -1;
 		rte_eal_mcfg_wait_complete(rte_config.mem_config);
-		rte_eal_config_reattach();
+		if (rte_eal_config_reattach() < 0)
+			return -1;
 		eal_update_internal_config();
 		break;
 	case RTE_PROC_AUTO:
 	case RTE_PROC_INVALID:
-		rte_panic("Invalid process type\n");
+		RTE_LOG(ERR, EAL, "Invalid process type %d\n",
+			rte_config.process_type);
+		return -1;
 	}
+
+	return 0;
 }
 
 /* Unlocks hugepage directories that were locked by eal_hugepage_info_init */
@@ -998,7 +1034,10 @@ static void rte_eal_init_alert(const char *msg)
 		return -1;
 	}
 
-	rte_config_init();
+	if (rte_config_init() < 0) {
+		rte_eal_init_alert("Cannot init config");
+		return -1;
+	}
 
 	if (rte_eal_intr_init() < 0) {
 		rte_eal_init_alert("Cannot init interrupt-handling thread");
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v5] eal: do not panic on shared memory init

[David Marchand]

On Mon, Jun 10, 2019 at 9:09 AM Arnon Warshavsky <arnon@qwilt.com> wrote:

> This patch changes some void functions to return a value,
> so that the init sequence may tear down orderly
> instead of calling panic.
>
> Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
> ---
>
> The calls for launching core messaging threads were left in tact
> in all 3 eal implementations.
> This should be addressed in a different patchset.
> There are still cases where the PMDs or message threads
> may call panic with no context for the user.
> For these I will submit a patch suggesting a callback registration,
> allowing the user to register a context to be called
> in case of a panic that takes place outside the init sequence.
>
> -v2 fix freebsd compilation
> -v3 fix freebsd compilation, haste from the devil
> -v4 fix comments by David
> -v5 fix more comments by david. rename patch
>
>  lib/librte_eal/freebsd/eal/eal.c |  69 ++++++++++++++++++--------
>  lib/librte_eal/linux/eal/eal.c   | 101
> +++++++++++++++++++++++++++------------
>  2 files changed, 120 insertions(+), 50 deletions(-)
>
> diff --git a/lib/librte_eal/freebsd/eal/eal.c
> b/lib/librte_eal/freebsd/eal/eal.c
> index c6ac902..f18f08e 100644
> --- a/lib/librte_eal/freebsd/eal/eal.c
> +++ b/lib/librte_eal/freebsd/eal/eal.c
> @@ -215,7 +215,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;
> @@ -224,60 +224,82 @@ 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, 0600);
> -               if (mem_cfg_fd < 0)
> -                       rte_panic("Cannot open '%s' for rte_mem_config\n",
> pathname);
> +               if (mem_cfg_fd < 0) {
> +                       RTE_LOG(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                               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(ERR, EAL, "Cannot resize '%s' for
> rte_mem_config\n",
> +                       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(ERR, EAL, "Cannot create lock on '%s'. Is another
> primary "
> +                       "process running?\n", 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(ERR, EAL, "Cannot mmap memory for rte_config\n");
> +               close(mem_cfg_fd);
> +               mem_cfg_fd = -1;
> +               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(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                               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");
> +       mem_cfg_fd = -1;
> +       if (rte_mem_cfg_addr == MAP_FAILED) {
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config!
> error %i (%s)\n",
> +                       errno, strerror(errno));
> +               return -1;
> +       }
>
>         rte_config.mem_config = rte_mem_cfg_addr;
> +
> +       return 0;
>  }
>
>  /* Detect if we are a primary or a secondary process */
> @@ -306,23 +328,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() < 0)
> +                       return -1;
>                 break;
>         case RTE_PROC_SECONDARY:
> -               rte_eal_config_attach();
> +               if (rte_eal_config_attach() < 0)
> +                       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");
> +               RTE_LOG(ERR, EAL, "Invalid process type %d\n",
> +                       rte_config.process_type);
> +               return -1;
>         }
> +
> +       return 0;
>  }
>
>  /* display usage */
> @@ -655,7 +683,10 @@ static void rte_eal_init_alert(const char *msg)
>                 return -1;
>         }
>
> -       rte_config_init();
> +       if (rte_config_init() < 0) {
> +               rte_eal_init_alert("Cannot init config");
> +               return -1;
> +       }
>
>         if (rte_eal_intr_init() < 0) {
>                 rte_eal_init_alert("Cannot init interrupt-handling
> thread");
> diff --git a/lib/librte_eal/linux/eal/eal.c
> b/lib/librte_eal/linux/eal/eal.c
> index 1613996..224ba90 100644
> --- a/lib/librte_eal/linux/eal/eal.c
> +++ b/lib/librte_eal/linux/eal/eal.c
> @@ -300,7 +300,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;
> @@ -309,7 +309,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)
> @@ -321,29 +321,41 @@ enum rte_iova_mode
>
>         if (mem_cfg_fd < 0){
>                 mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0600);
> -               if (mem_cfg_fd < 0)
> -                       rte_panic("Cannot open '%s' for rte_mem_config\n",
> pathname);
> +               if (mem_cfg_fd < 0) {
> +                       RTE_LOG(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                               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(ERR, EAL, "Cannot resize '%s' for
> rte_mem_config\n",
> +                       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(ERR, EAL, "Cannot create lock on '%s'. Is another
> primary "
> +                       "process running?\n", 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");
> +               close(mem_cfg_fd);
> +               mem_cfg_fd = -1;
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config\n");
> +               return -1;
>         }
> +
>         memcpy(rte_mem_cfg_addr, &early_mem_config,
> sizeof(early_mem_config));
>         rte_config.mem_config = rte_mem_cfg_addr;
>
> @@ -353,10 +365,11 @@ enum rte_iova_mode
>
>         rte_config.mem_config->dma_maskbits = 0;
>
> +       return 0;
>  }
>
>  /* attach to an existing shared memory config */
> -static void
> +static int
>  rte_eal_config_attach(void)
>  {
>         struct rte_mem_config *mem_config;
> @@ -364,33 +377,42 @@ 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);
> -               if (mem_cfg_fd < 0)
> -                       rte_panic("Cannot open '%s' for rte_mem_config\n",
> pathname);
> +               if (mem_cfg_fd < 0) {
> +                       RTE_LOG(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                               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) {
> +               close(mem_cfg_fd);
> +               mem_cfg_fd = -1;
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config!
> error %i (%s)\n",
> +                       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;
> @@ -402,19 +424,26 @@ enum rte_iova_mode
>         mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
>                         sizeof(*mem_config), PROT_READ | PROT_WRITE,
> MAP_SHARED,
>                         mem_cfg_fd, 0);
> +
> +       close(mem_cfg_fd);
> +       mem_cfg_fd = -1;
> +
>         if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
> -               if (mem_config != MAP_FAILED)
> +               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);
> -               else
> -                       rte_panic("Cannot mmap memory for rte_config!
> error %i (%s)\n",
> -                                 errno, strerror(errno));
> +                       RTE_LOG(ERR, EAL, "Cannot mmap memory for
> rte_config at [%p], got [%p]"
> +                               " - please use '--base-virtaddr' option\n",
> +                               rte_mem_cfg_addr, mem_config);
> +                       return -1;
> +               }
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config!
> error %i (%s)\n",
> +                       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 */
> @@ -461,26 +490,33 @@ 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() < 0)
> +                       return -1;
>                 eal_update_mem_config();
>                 break;
>         case RTE_PROC_SECONDARY:
> -               rte_eal_config_attach();
> +               if (rte_eal_config_attach() < 0)
> +                       return -1;
>                 rte_eal_mcfg_wait_complete(rte_config.mem_config);
> -               rte_eal_config_reattach();
> +               if (rte_eal_config_reattach() < 0)
> +                       return -1;
>                 eal_update_internal_config();
>                 break;
>         case RTE_PROC_AUTO:
>         case RTE_PROC_INVALID:
> -               rte_panic("Invalid process type\n");
> +               RTE_LOG(ERR, EAL, "Invalid process type %d\n",
> +                       rte_config.process_type);
> +               return -1;
>         }
> +
> +       return 0;
>  }
>
>  /* Unlocks hugepage directories that were locked by
> eal_hugepage_info_init */
> @@ -998,7 +1034,10 @@ static void rte_eal_init_alert(const char *msg)
>                 return -1;
>         }
>
> -       rte_config_init();
> +       if (rte_config_init() < 0) {
> +               rte_eal_init_alert("Cannot init config");
> +               return -1;
> +       }
>
>         if (rte_eal_intr_init() < 0) {
>                 rte_eal_init_alert("Cannot init interrupt-handling
> thread");
> --
> 1.8.3.1
>


Reviewed-by: David Marchand <david.marchand@redhat.com>

Thanks Arnon!


-- 
David Marchand

Re: [dpdk-dev] [PATCH v5] eal: do not panic on shared memory init

[Thomas Monjalon]

11/06/2019 10:07, David Marchand:
> On Mon, Jun 10, 2019 at 9:09 AM Arnon Warshavsky <arnon@qwilt.com> wrote:
> 
> > This patch changes some void functions to return a value,
> > so that the init sequence may tear down orderly
> > instead of calling panic.
> >
> > Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
> > ---
> >
> > The calls for launching core messaging threads were left in tact
> > in all 3 eal implementations.
> > This should be addressed in a different patchset.
> > There are still cases where the PMDs or message threads
> > may call panic with no context for the user.
> > For these I will submit a patch suggesting a callback registration,
> > allowing the user to register a context to be called
> > in case of a panic that takes place outside the init sequence.
> 
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> Thanks Arnon!

Applied, thanks

For the record, below is the updated status of
rte_panic/rte_exit calls in libs:

librte_eal:

    int rte_eal_init
        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
                -> debug assert

librte_mbuf:
    void rte_mbuf_sanity_check
        rte_panic
            -> debug assert

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