[PATCH] eal/linux: redefine the name for rte_fbarray_init()

[PATCH] eal/linux: redefine the name for rte_fbarray_init()

[Congjie Zhou]

add the current time into the name

Signed-off-by: Congjie Zhou <zcjie0802@qq.com>
---

When multiple secondary processes run in different containers, names
identified by PIDs are not unique due to the pid namespace.
Add current time to redefine a unique name.

 lib/eal/linux/eal_memalloc.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/lib/eal/linux/eal_memalloc.c b/lib/eal/linux/eal_memalloc.c
index e354efc..3fae534 100644
--- a/lib/eal/linux/eal_memalloc.c
+++ b/lib/eal/linux/eal_memalloc.c
@@ -16,6 +16,7 @@
 #include <fcntl.h>
 #include <signal.h>
 #include <setjmp.h>
+#include <inttypes.h>
 #ifdef F_ADD_SEALS /* if file sealing is supported, so is memfd */
 #include <linux/memfd.h>
 #define MEMFD_SUPPORTED
@@ -1430,6 +1431,18 @@ eal_memalloc_sync_with_primary(void)
 	return 0;
 }
 
+static uint64_t
+current_time(void)
+{
+	struct timespec ts;
+	if (clock_gettime(CLOCK_MONOTONIC_RAW, &ts) == -1) {
+		EAL_LOG(ERR, "Fail to get current time");
+		return -1ULL;
+	}
+	uint64_t time_ns = (uint64_t)ts.tv_sec * 1000000000ULL + ts.tv_nsec;
+	return time_ns;
+}
+
 static int
 secondary_msl_create_walk(const struct rte_memseg_list *msl,
 		void *arg __rte_unused)
@@ -1447,8 +1460,8 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	local_msl = &local_memsegs[msl_idx];
 
 	/* create distinct fbarrays for each secondary */
-	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i",
-		primary_msl->memseg_arr.name, getpid());
+	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i_%"PRIx64,
+		primary_msl->memseg_arr.name, getpid(), current_time());
 
 	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
 		primary_msl->memseg_arr.len,
-- 
2.34.1

Re: [PATCH] eal/linux: redefine the name for rte_fbarray_init()

[Stephen Hemminger]

On Thu, 14 Nov 2024 16:37:10 +0800
Congjie Zhou <zcjie0802@qq.com> wrote:

>  
> +static uint64_t
> +current_time(void)
> +{
> +	struct timespec ts;
> +	if (clock_gettime(CLOCK_MONOTONIC_RAW, &ts) == -1) {
> +		EAL_LOG(ERR, "Fail to get current time");
> +		return -1ULL;
> +	}
> +	uint64_t time_ns = (uint64_t)ts.tv_sec * 1000000000ULL + ts.tv_nsec;
> +	return time_ns;
> +}
> +
>  static int
>  secondary_msl_create_walk(const struct rte_memseg_list *msl,
>  		void *arg __rte_unused)
> @@ -1447,8 +1460,8 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>  	local_msl = &local_memsegs[msl_idx];
>  
>  	/* create distinct fbarrays for each secondary */
> -	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i",
> -		primary_msl->memseg_arr.name, getpid());
> +	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i_%"PRIx64,
> +		primary_msl->memseg_arr.name, getpid(), current_time());
>  

In general DPDK uses tsc instead of monotonic time, since it is faster and platform
independent (ie Windows).

Why not use use a global counter instead?

Re: [PATCH] eal/linux: redefine the name for rte_fbarray_init()

[Stephen Hemminger]

What about using thread id instead?

From d1687ffbf865ba0b2d64c35acd602ca43329691e Mon Sep 17 00:00:00 2001
From: Stephen Hemminger <stephen@networkplumber.org>
Date: Thu, 14 Nov 2024 08:48:54 -0800
Subject: [PATCH] eal: fix fbarray name with multiple secondary processes

When multiple secondary processes run in different containers, names
identified by PIDs are not unique due to the pid namespace.
Add current thread id to the name to be unique.

Fixes: 046aa5c4477b ("mem: add memalloc init stage")
Cc: anatoly.burakov@intel.com

Reported-by: Congjie Zhou <zcjie0802@qq.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/linux/eal_memalloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/eal/linux/eal_memalloc.c b/lib/eal/linux/eal_memalloc.c
index e354efc95d..776260e14f 100644
--- a/lib/eal/linux/eal_memalloc.c
+++ b/lib/eal/linux/eal_memalloc.c
@@ -1447,8 +1447,8 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	local_msl = &local_memsegs[msl_idx];
 
 	/* create distinct fbarrays for each secondary */
-	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i",
-		primary_msl->memseg_arr.name, getpid());
+	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i_%i",
+		 primary_msl->memseg_arr.name, getpid(), rte_sys_gettid());
 
 	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
 		primary_msl->memseg_arr.len,
-- 
2.45.2

Re: [PATCH] eal/linux: redefine the name for rte_fbarray_init()

[Zhou congjie]

On Thu, 14 Nov 2024, Stephen Hemminger wrote:

> Date: Fri, 15 Nov 2024 01:06:27
> From: Stephen Hemminger <stephen@networkplumber.org>
> To: zcjie0802 <zcjie0802@qq.com>
> Cc: dev <dev@dpdk.org>, anatoly.burakov <anatoly.burakov@intel.com>
> Subject: Re: [PATCH] eal/linux: redefine the name for rte_fbarray_init()
> 
> What about using thread id instead?
> 
> From d1687ffbf865ba0b2d64c35acd602ca43329691e Mon Sep 17 00:00:00 2001
> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Thu, 14 Nov 2024 08:48:54 -0800
> Subject: [PATCH] eal: fix fbarray name with multiple secondary processes
> 
> When multiple secondary processes run in different containers, names
> identified by PIDs are not unique due to the pid namespace.
> Add current thread id to the name to be unique.
> 
> Fixes: 046aa5c4477b ("mem: add memalloc init stage")
> Cc: anatoly.burakov@intel.com
> 
> Reported-by: Congjie Zhou <zcjie0802@qq.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/eal/linux/eal_memalloc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/eal/linux/eal_memalloc.c b/lib/eal/linux/eal_memalloc.c
> index e354efc95d..776260e14f 100644
> --- a/lib/eal/linux/eal_memalloc.c
> +++ b/lib/eal/linux/eal_memalloc.c
> @@ -1447,8 +1447,8 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>  	local_msl = &local_memsegs[msl_idx];
>  
>  	/* create distinct fbarrays for each secondary */
> -	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i",
> -		primary_msl->memseg_arr.name, getpid());
> +	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i_%i",
> +		 primary_msl->memseg_arr.name, getpid(), rte_sys_gettid());
>  
>  	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
>  		primary_msl->memseg_arr.len,
> 

As far as know, the tid is pid independent, so programs in different PID 
namespaces may have the same pid and tid.

For the solution that uses a global counter, I feel it can only be 
implemented by adding variables into rte_config or rte_mem_config, which 
involves modifying multiple files. Adding the current time would be more 
simple. This code is excuted only once when the secondary process is created, so 
it will not cause performance issues.

Re: [PATCH] eal/linux: redefine the name for rte_fbarray_init()

[Stephen Hemminger]

On Fri, 15 Nov 2024 10:26:00 +0800
Zhou congjie <zcjie0802@qq.com> wrote:

> On Thu, 14 Nov 2024, Stephen Hemminger wrote:
> 
> > Date: Fri, 15 Nov 2024 01:06:27
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > To: zcjie0802 <zcjie0802@qq.com>
> > Cc: dev <dev@dpdk.org>, anatoly.burakov <anatoly.burakov@intel.com>
> > Subject: Re: [PATCH] eal/linux: redefine the name for rte_fbarray_init()
> > 
> > What about using thread id instead?
> > 
> > From d1687ffbf865ba0b2d64c35acd602ca43329691e Mon Sep 17 00:00:00 2001
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Date: Thu, 14 Nov 2024 08:48:54 -0800
> > Subject: [PATCH] eal: fix fbarray name with multiple secondary processes
> > 
> > When multiple secondary processes run in different containers, names
> > identified by PIDs are not unique due to the pid namespace.
> > Add current thread id to the name to be unique.
> > 
> > Fixes: 046aa5c4477b ("mem: add memalloc init stage")
> > Cc: anatoly.burakov@intel.com
> > 
> > Reported-by: Congjie Zhou <zcjie0802@qq.com>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  lib/eal/linux/eal_memalloc.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/eal/linux/eal_memalloc.c b/lib/eal/linux/eal_memalloc.c
> > index e354efc95d..776260e14f 100644
> > --- a/lib/eal/linux/eal_memalloc.c
> > +++ b/lib/eal/linux/eal_memalloc.c
> > @@ -1447,8 +1447,8 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
> >  	local_msl = &local_memsegs[msl_idx];
> >  
> >  	/* create distinct fbarrays for each secondary */
> > -	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i",
> > -		primary_msl->memseg_arr.name, getpid());
> > +	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i_%i",
> > +		 primary_msl->memseg_arr.name, getpid(), rte_sys_gettid());
> >  
> >  	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
> >  		primary_msl->memseg_arr.len,
> >   
> 
> As far as know, the tid is pid independent, so programs in different PID 
> namespaces may have the same pid and tid.
> 
> For the solution that uses a global counter, I feel it can only be 
> implemented by adding variables into rte_config or rte_mem_config, which 
> involves modifying multiple files. Adding the current time would be more 
> simple. This code is excuted only once when the secondary process is created, so 
> it will not cause performance issues.

The combination of thread id and process id will be unique but
there might be issues with some container implementations which will
have overlapping pid's (see Docker --pid flag).

The TSC is available in all of DPDK via rte_get_tsc_cycles()
or use rte_get_timer_cycles().

But you could still have collisions from two containers using hidden pid
doing function at same exact time. Not ever likely, but it is possible.

Re: [PATCH] eal/linux: redefine the name for rte_fbarray_init()

[Zhou congjie]

On Thu, 14 Nov 2024, Stephen Hemminger wrote:

> 
> In general DPDK uses tsc instead of monotonic time, since it is faster and platform
> independent (ie Windows).
> 
> Why not use use a global counter instead?
> 

I have tried tsc, but it only works on x86 and does not function on other archs.

A global counter seems to be a good idea.
 

Re: [PATCH] eal/linux: redefine the name for rte_fbarray_init()

[Zhou congjie]

On Thu, 14 Nov 2024, Stephen Hemminger wrote:
 
> 
> In general DPDK uses tsc instead of monotonic time, since it is faster and platform
> independent (ie Windows).
> 
> Why not use use a global counter instead?
> 

I have tried tsc, but it only works on x86 and does not function on other 
architectures.

A global counter seems to be a good idea.

[PATCH v2] eal/linux: fix fbarray name with multiple secondary processes

[Congjie Zhou]

add the tsc into the name.

Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Congjie Zhou <zcjie0802@qq.com>
---

When multiple secondary processes run in different containers, names
identified by PIDs are not unique due to the pid namespace. 
So Add tsc to redefine a unique name.

v1: use monotonic time to redefine the name
v2: use tsc to redefine the name

 lib/eal/linux/eal_memalloc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/eal/linux/eal_memalloc.c b/lib/eal/linux/eal_memalloc.c
index e354efc..367d401 100644
--- a/lib/eal/linux/eal_memalloc.c
+++ b/lib/eal/linux/eal_memalloc.c
@@ -16,6 +16,7 @@
 #include <fcntl.h>
 #include <signal.h>
 #include <setjmp.h>
+#include <inttypes.h>
 #ifdef F_ADD_SEALS /* if file sealing is supported, so is memfd */
 #include <linux/memfd.h>
 #define MEMFD_SUPPORTED
@@ -1447,8 +1448,8 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	local_msl = &local_memsegs[msl_idx];
 
 	/* create distinct fbarrays for each secondary */
-	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i",
-		primary_msl->memseg_arr.name, getpid());
+	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i_%"PRIx64,
+		primary_msl->memseg_arr.name, getpid(), rte_get_tsc_cycles());
 
 	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
 		primary_msl->memseg_arr.len,
-- 
2.34.1

Re: [PATCH v2] eal/linux: fix fbarray name with multiple secondary processes

[Stephen Hemminger]

On Fri, 15 Nov 2024 15:50:08 +0800
Congjie Zhou <zcjie0802@qq.com> wrote:

> add the tsc into the name.
> 
> Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Congjie Zhou <zcjie0802@qq.com>
> ---
> 
> When multiple secondary processes run in different containers, names
> identified by PIDs are not unique due to the pid namespace. 
> So Add tsc to redefine a unique name.
> 
> v1: use monotonic time to redefine the name
> v2: use tsc to redefine the name
> 
>  lib/eal/linux/eal_memalloc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/eal/linux/eal_memalloc.c b/lib/eal/linux/eal_memalloc.c
> index e354efc..367d401 100644
> --- a/lib/eal/linux/eal_memalloc.c
> +++ b/lib/eal/linux/eal_memalloc.c
> @@ -16,6 +16,7 @@
>  #include <fcntl.h>
>  #include <signal.h>
>  #include <setjmp.h>
> +#include <inttypes.h>
>  #ifdef F_ADD_SEALS /* if file sealing is supported, so is memfd */
>  #include <linux/memfd.h>
>  #define MEMFD_SUPPORTED
> @@ -1447,8 +1448,8 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>  	local_msl = &local_memsegs[msl_idx];
>  
>  	/* create distinct fbarrays for each secondary */
> -	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i",
> -		primary_msl->memseg_arr.name, getpid());
> +	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i_%"PRIx64,
> +		primary_msl->memseg_arr.name, getpid(), rte_get_tsc_cycles());
>  

Worst case name is now:
'memseg-1048576k-128-128-1234567890-ffffffffffffffff' = 51 characters


Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH v2] eal/linux: fix fbarray name with multiple secondary processes

[Stephen Hemminger]

On Fri, 15 Nov 2024 15:50:08 +0800
Congjie Zhou <zcjie0802@qq.com> wrote:

> diff --git a/lib/eal/linux/eal_memalloc.c b/lib/eal/linux/eal_memalloc.c
> index e354efc..367d401 100644
> --- a/lib/eal/linux/eal_memalloc.c
> +++ b/lib/eal/linux/eal_memalloc.c
> @@ -16,6 +16,7 @@
>  #include <fcntl.h>
>  #include <signal.h>
>  #include <setjmp.h>
> +#include <inttypes.h>
>  #ifdef F_ADD_SEALS /* if file sealing is supported, so is memfd */
>  #include <linux/memfd.h>
>  #define MEMFD_SUPPORTED
> @@ -1447,8 +1448,8 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>  	local_msl = &local_memsegs[msl_idx];
>  
>  	/* create distinct fbarrays for each secondary */
> -	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i",
> -		primary_msl->memseg_arr.name, getpid());
> +	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i_%"PRIx64,
> +		primary_msl->memseg_arr.name, getpid(), rte_get_tsc_cycles());
>  
>  	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
>  		primary_msl->memseg_arr.len,


Need to include <rte_cycles.h> to get the prototype rte_get_tsc_cycles()

Re: [PATCH v2] eal/linux: fix fbarray name with multiple secondaryprocesses

[Zhou congjie]

On Fri, 15 Nov 2024, Stephen Hemminger wrote:

> Need to include <rte_cycles.h> to get the prototype rte_get_tsc_cycles()

When I compile locally (on x86), it compiles successfully whether or not 
<rte_cycles.h> is included. 

However, cloud testing indicates that compilation fails on other 
architectures when <rte_cycles.h> is not included.
 
Is <rte_cycles.h> the cause of the compilation failure? 
If so, I'm curious why this is the case.

Re: [PATCH v2] eal/linux: fix fbarray name with multiple secondaryprocesses

[Zhou congjie]

On Fri, 15 Nov 2024, Stephen Hemminger wrote:

> Need to include <rte_cycles.h> to get the prototype rte_get_tsc_cycles()


For the code without <rte_cycles.h> could run on x86, I think I have found 
the reason. Through the path <rte_memory.h> -> <rte_eal_memconfig.h> -> 
<rte_spinlock.h>, the <rte_spinlock.h> is included.

<rte_spinlock.h> is independent on each architecture. And on x86 the 
<rte_spinlock.h> includes its own "rte_cycles.h", while other 
architectures do not.

[PATCH v3] eal/linux: fix fbarray name with multiple secondary processes

[Congjie Zhou]

add the tsc into the name.

Suggested-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Congjie Zhou <zcjie0802@qq.com>
---

When multiple secondary processes run in different containers, names
identified by PIDs are not unique due to the pid namespace. 
So Add tsc to redefine a unique name.

v1: use monotonic time to redefine the name
v2: use tsc to redefine the name
v3: include <rte_cycles.h> to support all architectures

 lib/eal/linux/eal_memalloc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/eal/linux/eal_memalloc.c b/lib/eal/linux/eal_memalloc.c
index e354efc..ae1e024 100644
--- a/lib/eal/linux/eal_memalloc.c
+++ b/lib/eal/linux/eal_memalloc.c
@@ -16,6 +16,7 @@
 #include <fcntl.h>
 #include <signal.h>
 #include <setjmp.h>
+#include <inttypes.h>
 #ifdef F_ADD_SEALS /* if file sealing is supported, so is memfd */
 #include <linux/memfd.h>
 #define MEMFD_SUPPORTED
@@ -31,6 +32,7 @@
 #include <rte_log.h>
 #include <rte_eal.h>
 #include <rte_memory.h>
+#include <rte_cycles.h>
 
 #include "eal_filesystem.h"
 #include "eal_internal_cfg.h"
@@ -1447,8 +1449,8 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	local_msl = &local_memsegs[msl_idx];
 
 	/* create distinct fbarrays for each secondary */
-	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i",
-		primary_msl->memseg_arr.name, getpid());
+	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i_%"PRIx64,
+		primary_msl->memseg_arr.name, getpid(), rte_get_tsc_cycles());
 
 	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
 		primary_msl->memseg_arr.len,
-- 
2.34.1

[PATCH] eal/linux: redefine the name for rte_fbarray_init()

[Congjie Zhou]

add the current time into the name

Signed-off-by: Congjie Zhou <zcjie0802@qq.com>
---

When multiple secondary processes run in different containers, names
identified by PIDs are not unique due to the pid namespace.
Add current time to redefine a unique name.

 lib/eal/linux/eal_memalloc.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/lib/eal/linux/eal_memalloc.c b/lib/eal/linux/eal_memalloc.c
index e354efc..017bf67 100644
--- a/lib/eal/linux/eal_memalloc.c
+++ b/lib/eal/linux/eal_memalloc.c
@@ -16,6 +16,7 @@
 #include <fcntl.h>
 #include <signal.h>
 #include <setjmp.h>
+#include <inttypes.h>
 #ifdef F_ADD_SEALS /* if file sealing is supported, so is memfd */
 #include <linux/memfd.h>
 #define MEMFD_SUPPORTED
@@ -1430,6 +1431,17 @@ eal_memalloc_sync_with_primary(void)
 	return 0;
 }
 
+static uint64_t 
+current_time(void) {
+    struct timespec ts;
+    if (clock_gettime(CLOCK_MONOTONIC_RAW, &ts) == -1) {
+        EAL_LOG(ERR, "Fail to get current time");
+		return -1ULL;
+    }
+    uint64_t time_ns = (uint64_t)ts.tv_sec * 1000000000ULL + ts.tv_nsec;
+    return time_ns;
+}
+
 static int
 secondary_msl_create_walk(const struct rte_memseg_list *msl,
 		void *arg __rte_unused)
@@ -1447,8 +1459,8 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	local_msl = &local_memsegs[msl_idx];
 
 	/* create distinct fbarrays for each secondary */
-	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i",
-		primary_msl->memseg_arr.name, getpid());
+	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%i_%"PRIx64,
+		primary_msl->memseg_arr.name, getpid(), current_time());
 
 	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
 		primary_msl->memseg_arr.len,
-- 
2.34.1