[PATCH v1 0/7] support reinit flow

[PATCH v1 0/7] support reinit flow

[okaya]

From: Sinan Kaya <okaya@kernel.org>

We want to be able to call rte_eal_init() and rte_eal_cleanup()
APIs back to back for maintanance reasons.

Here is a summary of the code we have seen so far:

1. some code support getting called multiple times by keeping
a static variable.
2. some code initializes once but never clean up after them and
don't have a cleanup API.
3. some code assumes that they only get called once during the
lifecycle of the process.

Most changes in this patch center around following the #1 design
principle.

Graham Whyte (1):
  eal: fixes for re-initialization issues

Sinan Kaya (6):
  tailq: skip init if already initialized
  eal_memzone: bail out on initialized
  memseg: init once
  eal_memory: skip initialization
  eal_interrupts: don't reinitialize threads
  eal: initialize worker threads once

 lib/eal/common/eal_common_memory.c  |  5 ++
 lib/eal/common/eal_common_memzone.c |  6 +++
 lib/eal/common/eal_common_options.c |  7 +++
 lib/eal/common/eal_common_tailqs.c  | 21 ++++++---
 lib/eal/common/malloc_heap.c        |  6 +++
 lib/eal/linux/eal.c                 | 72 ++++++++++++++++-------------
 lib/eal/linux/eal_interrupts.c      |  5 ++
 lib/eal/linux/eal_memory.c          | 11 ++++-
 8 files changed, 92 insertions(+), 41 deletions(-)

-- 
2.25.1

[PATCH v1 1/7] eal: fixes for re-initialization issues

[okaya]

From: Graham Whyte <grwhyte@microsoft.com>

reinitialize the solib link list and clean the globals holding
state for parsing.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
Signed-off by: Graham Whyte <graham.whyte@microsoft.com>
---
 lib/eal/common/eal_common_options.c | 7 +++++++
 lib/eal/linux/eal.c                 | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
index 2d6535781b..b58df1bc5f 100644
--- a/lib/eal/common/eal_common_options.c
+++ b/lib/eal/common/eal_common_options.c
@@ -1983,6 +1983,13 @@ compute_ctrl_threads_cpuset(struct internal_config *internal_cfg)
 		memcpy(cpuset, &lcore_config[rte_get_main_lcore()].cpuset,
 			sizeof(*cpuset));
 	}
+
+	// Reinitialize solib_list
+	TAILQ_INIT(&solib_list);
+
+	master_lcore_parsed = 0;
+	mem_parsed = 0;
+	core_parsed = 0;
 }
 
 int
diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index 8c118d0d9f..5fd81d71cb 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -78,6 +78,8 @@ struct lcore_config lcore_config[RTE_MAX_LCORE];
 int rte_cycles_vmware_tsc_map;
 
 
+static uint32_t run_once = 0;
+
 int
 eal_clean_runtime_dir(void)
 {
@@ -504,6 +506,7 @@ eal_parse_socket_arg(char *strval, volatile uint64_t *socket_arg)
 		socket_arg[i] = val;
 	}
 
+	__atomic_store_n(&run_once, 0, __ATOMIC_RELAXED);
 	return 0;
 }
 
@@ -960,7 +963,6 @@ int
 rte_eal_init(int argc, char **argv)
 {
 	int i, fctret, ret;
-	static uint32_t run_once;
 	uint32_t has_run = 0;
 	const char *p;
 	static char logid[PATH_MAX];
-- 
2.25.1

Re: [PATCH v1 1/7] eal: fixes for re-initialization issues

[Stephen Hemminger]

On Mon, 14 Aug 2023 21:38:20 -0400
okaya@kernel.org wrote:

> From: Graham Whyte <grwhyte@microsoft.com>
> 
> reinitialize the solib link list and clean the globals holding
> state for parsing.
> 
> Signed-off-by: Sinan Kaya <okaya@kernel.org>
> Signed-off by: Graham Whyte <graham.whyte@microsoft.com>

Please fix all the checkpatch issues.

Re: [PATCH v1 1/7] eal: fixes for re-initialization issues

[Stephen Hemminger]

> +	master_lcore_parsed = 0;

The main lcore is no longer called master. DPDK has been working to elimnate
all non-inclusive naming.

[PATCH v1 2/7] tailq: skip init if already initialized

[okaya]

From: Sinan Kaya <okaya@kernel.org>

Allows tailq to be reinitialied multiple times
by looking up previously registered tailqs

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/eal_common_tailqs.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/lib/eal/common/eal_common_tailqs.c b/lib/eal/common/eal_common_tailqs.c
index 580fbf24bc..3995aa2717 100644
--- a/lib/eal/common/eal_common_tailqs.c
+++ b/lib/eal/common/eal_common_tailqs.c
@@ -73,9 +73,10 @@ rte_eal_tailq_create(const char *name)
 		strlcpy(head->name, name, sizeof(head->name) - 1);
 		TAILQ_INIT(&head->tailq_head);
 		rte_tailqs_count++;
+		return head;
 	}
 
-	return head;
+	return rte_eal_tailq_lookup(name);
 }
 
 /* local register, used to store "early" tailqs before rte_eal_init() and to
@@ -99,7 +100,10 @@ rte_eal_tailq_update(struct rte_tailq_elem *t)
 {
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 		/* primary process is the only one that creates */
-		t->head = rte_eal_tailq_create(t->name);
+		t->head = rte_eal_tailq_lookup(t->name);
+		if (t->head == NULL) {
+			t->head = rte_eal_tailq_create(t->name);
+		}
 	} else {
 		t->head = rte_eal_tailq_lookup(t->name);
 	}
@@ -108,15 +112,13 @@ rte_eal_tailq_update(struct rte_tailq_elem *t)
 int
 rte_eal_tailq_register(struct rte_tailq_elem *t)
 {
-	if (rte_eal_tailq_local_register(t) < 0) {
-		RTE_LOG(ERR, EAL,
-			"%s tailq is already registered\n", t->name);
-		goto error;
-	}
+	rte_eal_tailq_local_register(t);
 
 	/* if a register happens after rte_eal_tailqs_init(), then we can update
 	 * tailq head */
 	if (rte_tailqs_count >= 0) {
+		RTE_LOG(INFO, EAL,
+			"%s tailq is registered\n", t->name);
 		rte_eal_tailq_update(t);
 		if (t->head == NULL) {
 			RTE_LOG(ERR, EAL,
@@ -138,6 +140,11 @@ rte_eal_tailqs_init(void)
 {
 	struct rte_tailq_elem *t;
 
+	if (rte_tailqs_count > 0) {
+		RTE_LOG(INFO, EAL, "tailq already initialized \n");
+		return 0;
+	}
+
 	rte_tailqs_count = 0;
 
 	TAILQ_FOREACH(t, &rte_tailq_elem_head, next) {
-- 
2.25.1

Re: [PATCH v1 2/7] tailq: skip init if already initialized

[Stephen Hemminger]

On Mon, 14 Aug 2023 21:38:21 -0400
okaya@kernel.org wrote:

> +		if (t->head == NULL) {
> +			t->head = rte_eal_tailq_create(t->name);
> +		}

Brackets not wanted on single statement

[PATCH v1 3/7] eal_memzone: bail out on initialized

[okaya]

From: Sinan Kaya <okaya@kernel.org>

Initialize memzone once and bail out if someone calls init
multiple times.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/eal_common_memzone.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/eal/common/eal_common_memzone.c b/lib/eal/common/eal_common_memzone.c
index 930fee5fdc..c76f298267 100644
--- a/lib/eal/common/eal_common_memzone.c
+++ b/lib/eal/common/eal_common_memzone.c
@@ -395,8 +395,12 @@ int
 rte_eal_memzone_init(void)
 {
 	struct rte_mem_config *mcfg;
+	static int initialized = 0;
 	int ret = 0;
 
+	if (initialized)
+		return 0;
+
 	/* get pointer to global configuration */
 	mcfg = rte_eal_get_configuration()->mem_config;
 
@@ -415,6 +419,8 @@ rte_eal_memzone_init(void)
 
 	rte_rwlock_write_unlock(&mcfg->mlock);
 
+	initialized = 1;
+
 	return ret;
 }
 
-- 
2.25.1

Re: [PATCH v1 3/7] eal_memzone: bail out on initialized

[Stephen Hemminger]

On Mon, 14 Aug 2023 21:38:22 -0400
okaya@kernel.org wrote:

> +	static int initialized = 0;
>  	int ret = 0;
>  
> +	if (initialized)
> +		return 0;
> +
>  	/* get pointer to global configuration */
>  	mcfg = rte_eal_get_configuration()->mem_config;
>  
> @@ -415,6 +419,8 @@ rte_eal_memzone_init(void)
>  
>  	rte_rwlock_write_unlock(&mcfg->mlock);
>  
> +	initialized = 1;
> +
>  	return ret;
>  }

Use a bool for this.

[PATCH v1 4/7] memseg: init once

[okaya]

From: Sinan Kaya <okaya@kernel.org>

Initialize memory segments just once and bail out if called
multiple times.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/linux/eal_memory.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/lib/eal/linux/eal_memory.c b/lib/eal/linux/eal_memory.c
index 60fc8cc6ca..c7bf879d15 100644
--- a/lib/eal/linux/eal_memory.c
+++ b/lib/eal/linux/eal_memory.c
@@ -1902,6 +1902,11 @@ rte_eal_memseg_init(void)
 {
 	/* increase rlimit to maximum */
 	struct rlimit lim;
+	static bool memseg_initialized = 0;
+	int ret;
+
+	if (memseg_initialized)
+		return 0;
 
 #ifndef RTE_EAL_NUMA_AWARE_HUGEPAGES
 	const struct internal_config *internal_conf =
@@ -1930,11 +1935,15 @@ rte_eal_memseg_init(void)
 	}
 #endif
 
-	return rte_eal_process_type() == RTE_PROC_PRIMARY ?
+	ret = rte_eal_process_type() == RTE_PROC_PRIMARY ?
 #ifndef RTE_ARCH_64
 			memseg_primary_init_32() :
 #else
 			memseg_primary_init() :
 #endif
 			memseg_secondary_init();
+
+	memseg_initialized = true;
+
+	return ret;
 }
-- 
2.25.1

[PATCH v1 5/7] eal_memory: skip initialization

[okaya]

From: Sinan Kaya <okaya@kernel.org>

Initialize heap area just once.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/common/eal_common_memory.c | 5 +++++
 lib/eal/common/malloc_heap.c       | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/lib/eal/common/eal_common_memory.c b/lib/eal/common/eal_common_memory.c
index 688dc615d7..e342eeb806 100644
--- a/lib/eal/common/eal_common_memory.c
+++ b/lib/eal/common/eal_common_memory.c
@@ -1084,10 +1084,14 @@ rte_eal_memory_init(void)
 
 	int retval;
 	RTE_LOG(DEBUG, EAL, "Setting up physically contiguous memory...\n");
+	static int initialized = 0;
 
 	if (!mcfg)
 		return -1;
 
+	if (initialized)
+		return 0;
+
 	/* lock mem hotplug here, to prevent races while we init */
 	rte_mcfg_mem_read_lock();
 
@@ -1106,6 +1110,7 @@ rte_eal_memory_init(void)
 	if (internal_conf->no_shconf == 0 && rte_eal_memdevice_init() < 0)
 		goto fail;
 
+	initialized = true;
 	return 0;
 fail:
 	rte_mcfg_mem_read_unlock();
diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index 6eb6fcda5e..4c5f8aed0b 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -1405,6 +1405,10 @@ rte_eal_malloc_heap_init(void)
 	unsigned int i;
 	const struct internal_config *internal_conf =
 		eal_get_internal_configuration();
+	static int initialized = 0;
+
+	if (initialized)
+		return 0;
 
 	if (internal_conf->match_allocations)
 		RTE_LOG(DEBUG, EAL, "Hugepages will be freed exactly as allocated.\n");
@@ -1443,6 +1447,8 @@ rte_eal_malloc_heap_init(void)
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
+	initialized = true;
+
 	/* add all IOVA-contiguous areas to the heap */
 	return rte_memseg_contig_walk(malloc_add_seg, NULL);
 }
-- 
2.25.1

[PATCH v1 6/7] eal_interrupts: don't reinitialize threads

[okaya]

From: Sinan Kaya <okaya@kernel.org>

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/linux/eal_interrupts.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/eal/linux/eal_interrupts.c b/lib/eal/linux/eal_interrupts.c
index d52ec8eb4c..aa296f153b 100644
--- a/lib/eal/linux/eal_interrupts.c
+++ b/lib/eal/linux/eal_interrupts.c
@@ -1174,6 +1174,10 @@ int
 rte_eal_intr_init(void)
 {
 	int ret = 0;
+	static int initialized = 0;
+
+	if (initialized)
+		return 0;
 
 	/* init the global interrupt source head */
 	TAILQ_INIT(&intr_sources);
@@ -1196,6 +1200,7 @@ rte_eal_intr_init(void)
 			"Failed to create thread for interrupt handling\n");
 	}
 
+	initialized = true;
 	return ret;
 }
 
-- 
2.25.1

Re: [PATCH v1 6/7] eal_interrupts: don't reinitialize threads

[Stephen Hemminger]

On Mon, 14 Aug 2023 21:38:25 -0400
okaya@kernel.org wrote:

> diff --git a/lib/eal/linux/eal_interrupts.c b/lib/eal/linux/eal_interrupts.c
> index d52ec8eb4c..aa296f153b 100644
> --- a/lib/eal/linux/eal_interrupts.c
> +++ b/lib/eal/linux/eal_interrupts.c
> @@ -1174,6 +1174,10 @@ int
>  rte_eal_intr_init(void)
>  {
>  	int ret = 0;
> +	static int initialized = 0;
> +
> +	if (initialized)
> +		return 0;
>  
>  	/* init the global interrupt source head */
>  	TAILQ_INIT(&intr_sources);
> @@ -1196,6 +1200,7 @@ rte_eal_intr_init(void)
>  			"Failed to create thread for interrupt handling\n");
>  	}
>  
> +	initialized = true;

If you want a bool, use a bool

[PATCH v1 7/7] eal: initialize worker threads once

[okaya]

From: Sinan Kaya <okaya@kernel.org>

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 lib/eal/linux/eal.c | 68 ++++++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 32 deletions(-)

diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index 5fd81d71cb..63190a4e70 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -1236,46 +1236,50 @@ rte_eal_init(int argc, char **argv)
 		config->main_lcore, (uintptr_t)pthread_self(), cpuset,
 		ret == 0 ? "" : "...");
 
-	RTE_LCORE_FOREACH_WORKER(i) {
+	static int worker_initialized = 0;
+	if (worker_initialized == 0) {
+		RTE_LCORE_FOREACH_WORKER(i) {
 
 		/*
 		 * create communication pipes between main thread
 		 * and children
 		 */
-		if (pipe(lcore_config[i].pipe_main2worker) < 0)
-			rte_panic("Cannot create pipe\n");
-		if (pipe(lcore_config[i].pipe_worker2main) < 0)
-			rte_panic("Cannot create pipe\n");
-
-		lcore_config[i].state = WAIT;
-
-		/* create a thread for each lcore */
-		ret = eal_worker_thread_create(i);
-		if (ret != 0)
-			rte_panic("Cannot create thread\n");
-
-		/* Set thread_name for aid in debugging. */
-		snprintf(thread_name, sizeof(thread_name),
-			"rte-worker-%d", i);
-		ret = rte_thread_setname(lcore_config[i].thread_id,
-						thread_name);
-		if (ret != 0)
-			RTE_LOG(DEBUG, EAL,
-				"Cannot set name for lcore thread\n");
+			if (pipe(lcore_config[i].pipe_main2worker) < 0)
+				rte_panic("Cannot create pipe\n");
+			if (pipe(lcore_config[i].pipe_worker2main) < 0)
+				rte_panic("Cannot create pipe\n");
+
+			lcore_config[i].state = WAIT;
+
+			/* create a thread for each lcore */
+			ret = eal_worker_thread_create(i);
+			if (ret != 0)
+				rte_panic("Cannot create thread\n");
+
+			/* Set thread_name for aid in debugging. */
+			snprintf(thread_name, sizeof(thread_name),
+				"rte-worker-%d", i);
+			ret = rte_thread_setname(lcore_config[i].thread_id,
+							thread_name);
+			if (ret != 0)
+				RTE_LOG(DEBUG, EAL,
+					"Cannot set name for lcore thread\n");
+
+			ret = pthread_setaffinity_np(lcore_config[i].thread_id,
+				sizeof(rte_cpuset_t), &lcore_config[i].cpuset);
+			if (ret != 0)
+				rte_panic("Cannot set affinity\n");
+		}
 
-		ret = pthread_setaffinity_np(lcore_config[i].thread_id,
-			sizeof(rte_cpuset_t), &lcore_config[i].cpuset);
-		if (ret != 0)
-			rte_panic("Cannot set affinity\n");
+		/*
+		 * Launch a dummy function on all worker lcores, so that main lcore
+		 * knows they are all ready when this function returns.
+		 */
+		rte_eal_mp_remote_launch(sync_func, NULL, SKIP_MAIN);
+		rte_eal_mp_wait_lcore();
+		worker_initialized = 1;
 	}
 
-	/*
-	 * Launch a dummy function on all worker lcores, so that main lcore
-	 * knows they are all ready when this function returns.
-	 */
-	rte_eal_mp_remote_launch(sync_func, NULL, SKIP_MAIN);
-	rte_eal_mp_wait_lcore();
-
 	/* initialize services so vdevs register service during bus_probe. */
 	ret = rte_service_init();
 	if (ret) {
-- 
2.25.1

Re: [PATCH v1 7/7] eal: initialize worker threads once

[Stephen Hemminger]

On Mon, 14 Aug 2023 21:38:26 -0400
okaya@kernel.org wrote:

> From: Sinan Kaya <okaya@kernel.org>
> 
> Signed-off-by: Sinan Kaya <okaya@kernel.org>

DPDK supports FreeBSD and Linux as well. Need to fix all platforms.

Re: [PATCH v1 0/7] support reinit flow

[Stephen Hemminger]

On Mon, 14 Aug 2023 21:38:19 -0400
okaya@kernel.org wrote:

> From: Sinan Kaya <okaya@kernel.org>
> 
> We want to be able to call rte_eal_init() and rte_eal_cleanup()
> APIs back to back for maintanance reasons.

Why?
This change exposes lots of drivers, API's and other things to untested code paths.
If you just want to do a hard restart, maybe just reexecing the same application would
be better.

Re: [PATCH v1 0/7] support reinit flow

[Sinan Kaya]

On Mon, 2023-08-14 at 19:12 -0700, Stephen Hemminger wrote:
> On Mon, 14 Aug 2023 21:38:19 -0400
> 
> okaya@kernel.org wrote:
> 
> 
> 
> > From: Sinan Kaya <okaya@kernel.org>
> > We want to be able to call rte_eal_init() and rte_eal_cleanup()
> > APIs back to back for maintanance reasons.
> 
> 
> Why?
> 
> This change exposes lots of drivers, API's and other things to
> untested code paths.
> 
> If you just want to do a hard restart, maybe just reexecing the same
> application would
> 
> be better.

It is not always ideal to reinitialize a DPDK process. Memory needs
to be reinitialized, hugetables need to warm up etc.

Given the cost of change is not huge, I believe this will be
important contribution for certain application types.
Changeset is not doing anything different from what already exists.

Re: [PATCH v1 0/7] support reinit flow

[Stephen Hemminger]

On Mon, 14 Aug 2023 21:38:19 -0400
okaya@kernel.org wrote:

> From: Sinan Kaya <okaya@kernel.org>
> 
> We want to be able to call rte_eal_init() and rte_eal_cleanup()
> APIs back to back for maintanance reasons.

One other note:
This sequence could only be done by main lcore, and never ever in a signal handler.
Do not try and trap signals like abort, bus error, illegal instruction and try to
use this for recovery. It is a recipe for failure.