[dpdk-dev] [PATCH] fbarray: get fbarrays from containerized secondary

[dpdk-dev] [PATCH] fbarray: get fbarrays from containerized secondary

[ogawa.yasufumi]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work as expected if secondary is run as app container becuase
each of containerized secondary has PID 1. To reserve unique name, use
hostname instead of PID if the value is 1.

Cc: stable@dpdk.org

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 lib/librte_eal/linux/eal/eal_memalloc.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
index 1e9ebb86d..beec03648 100644
--- a/lib/librte_eal/linux/eal/eal_memalloc.c
+++ b/lib/librte_eal/linux/eal/eal_memalloc.c
@@ -1362,6 +1362,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	struct rte_memseg_list *primary_msl, *local_msl;
 	char name[PATH_MAX];
 	int msl_idx, ret;
+	char proc_id[16];
 
 	if (msl->external)
 		return 0;
@@ -1371,8 +1372,28 @@ 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());
+	/* if run secondary in a container, the name of fbarray file cannot
+	 * be decided with pid because getpid() always returns 1, so use
+	 * hostname as a unique identifier among containers instead.
+	 */
+	if (getpid() == 1) {
+		FILE *hn_fp;
+		hn_fp = fopen("/etc/hostname", "r");
+		if (hn_fp == NULL) {
+			RTE_LOG(ERR, EAL,
+				"Cannot open '/etc/hostname' for secondary\n");
+			return -1;
+		}
+
+		/* with docker, /etc/hostname just has one entry of hostname */
+		if (fscanf(hn_fp, "%s", proc_id) == EOF)
+			return -1;
+		fclose(hn_fp);
+	} else
+		sprintf(proc_id, "%d", (int)getpid());
+
+	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
+			primary_msl->memseg_arr.name, proc_id);
 
 	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
 		primary_msl->memseg_arr.len,
-- 
2.17.1

[dpdk-dev] [PATCH] fbarray: get fbarrays from containerized secondary

[ogawa.yasufumi]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work as expected if secondary is run as app container becuase
each of containerized secondary has PID 1. To reserve unique name, use
hostname instead of PID if the value is 1.

Cc: stable@dpdk.org

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 lib/librte_eal/linux/eal/eal_memalloc.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
index 1e9ebb86d..beec03648 100644
--- a/lib/librte_eal/linux/eal/eal_memalloc.c
+++ b/lib/librte_eal/linux/eal/eal_memalloc.c
@@ -1362,6 +1362,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	struct rte_memseg_list *primary_msl, *local_msl;
 	char name[PATH_MAX];
 	int msl_idx, ret;
+	char proc_id[16];
 
 	if (msl->external)
 		return 0;
@@ -1371,8 +1372,28 @@ 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());
+	/* if run secondary in a container, the name of fbarray file cannot
+	 * be decided with pid because getpid() always returns 1, so use
+	 * hostname as a unique identifier among containers instead.
+	 */
+	if (getpid() == 1) {
+		FILE *hn_fp;
+		hn_fp = fopen("/etc/hostname", "r");
+		if (hn_fp == NULL) {
+			RTE_LOG(ERR, EAL,
+				"Cannot open '/etc/hostname' for secondary\n");
+			return -1;
+		}
+
+		/* with docker, /etc/hostname just has one entry of hostname */
+		if (fscanf(hn_fp, "%s", proc_id) == EOF)
+			return -1;
+		fclose(hn_fp);
+	} else
+		sprintf(proc_id, "%d", (int)getpid());
+
+	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
+			primary_msl->memseg_arr.name, proc_id);
 
 	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
 		primary_msl->memseg_arr.len,
-- 
2.17.1

[dpdk-dev] [PATCH v2 0/1] Get fbarrays from containerized secondary

[ogawa.yasufumi]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

Fix spell miss in commit message.

Yasufumi Ogawa (1):
  fbarray: get fbarrays from containerized secondary

 lib/librte_eal/linux/eal/eal_memalloc.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v2 0/1] Get fbarrays from containerized secondary

[ogawa.yasufumi]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

Fix spell miss in commit message.

Yasufumi Ogawa (1):
  fbarray: get fbarrays from containerized secondary

 lib/librte_eal/linux/eal/eal_memalloc.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v2 1/1] fbarray: get fbarrays from containerized secondary

[ogawa.yasufumi]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work if secondary is run as app container because each of
containerized secondary has PID 1. To reserve unique name, use hostname
instead of PID if the value is 1.

Cc: stable@dpdk.org

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 lib/librte_eal/linux/eal/eal_memalloc.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
index 1e9ebb86d..beec03648 100644
--- a/lib/librte_eal/linux/eal/eal_memalloc.c
+++ b/lib/librte_eal/linux/eal/eal_memalloc.c
@@ -1362,6 +1362,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	struct rte_memseg_list *primary_msl, *local_msl;
 	char name[PATH_MAX];
 	int msl_idx, ret;
+	char proc_id[16];
 
 	if (msl->external)
 		return 0;
@@ -1371,8 +1372,28 @@ 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());
+	/* if run secondary in a container, the name of fbarray file cannot
+	 * be decided with pid because getpid() always returns 1, so use
+	 * hostname as a unique identifier among containers instead.
+	 */
+	if (getpid() == 1) {
+		FILE *hn_fp;
+		hn_fp = fopen("/etc/hostname", "r");
+		if (hn_fp == NULL) {
+			RTE_LOG(ERR, EAL,
+				"Cannot open '/etc/hostname' for secondary\n");
+			return -1;
+		}
+
+		/* with docker, /etc/hostname just has one entry of hostname */
+		if (fscanf(hn_fp, "%s", proc_id) == EOF)
+			return -1;
+		fclose(hn_fp);
+	} else
+		sprintf(proc_id, "%d", (int)getpid());
+
+	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
+			primary_msl->memseg_arr.name, proc_id);
 
 	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
 		primary_msl->memseg_arr.len,
-- 
2.17.1

[dpdk-dev] [PATCH v2 1/1] fbarray: get fbarrays from containerized secondary

[ogawa.yasufumi]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work if secondary is run as app container because each of
containerized secondary has PID 1. To reserve unique name, use hostname
instead of PID if the value is 1.

Cc: stable@dpdk.org

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 lib/librte_eal/linux/eal/eal_memalloc.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
index 1e9ebb86d..beec03648 100644
--- a/lib/librte_eal/linux/eal/eal_memalloc.c
+++ b/lib/librte_eal/linux/eal/eal_memalloc.c
@@ -1362,6 +1362,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	struct rte_memseg_list *primary_msl, *local_msl;
 	char name[PATH_MAX];
 	int msl_idx, ret;
+	char proc_id[16];
 
 	if (msl->external)
 		return 0;
@@ -1371,8 +1372,28 @@ 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());
+	/* if run secondary in a container, the name of fbarray file cannot
+	 * be decided with pid because getpid() always returns 1, so use
+	 * hostname as a unique identifier among containers instead.
+	 */
+	if (getpid() == 1) {
+		FILE *hn_fp;
+		hn_fp = fopen("/etc/hostname", "r");
+		if (hn_fp == NULL) {
+			RTE_LOG(ERR, EAL,
+				"Cannot open '/etc/hostname' for secondary\n");
+			return -1;
+		}
+
+		/* with docker, /etc/hostname just has one entry of hostname */
+		if (fscanf(hn_fp, "%s", proc_id) == EOF)
+			return -1;
+		fclose(hn_fp);
+	} else
+		sprintf(proc_id, "%d", (int)getpid());
+
+	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
+			primary_msl->memseg_arr.name, proc_id);
 
 	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
 		primary_msl->memseg_arr.len,
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2 1/1] fbarray: get fbarrays from containerized secondary

[Thomas Monjalon]

We need a review here.

16/04/2019 05:43, ogawa.yasufumi@lab.ntt.co.jp:
> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> 
> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> PID for reserving unique name among secondary processes. However, it
> does not work if secondary is run as app container because each of
> containerized secondary has PID 1. To reserve unique name, use hostname
> instead of PID if the value is 1.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

Re: [dpdk-dev] [PATCH v2 1/1] fbarray: get fbarrays from containerized secondary

[Burakov, Anatoly]

On 16-Apr-19 4:43 AM, ogawa.yasufumi@lab.ntt.co.jp wrote:
> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> 
> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> PID for reserving unique name among secondary processes. However, it
> does not work if secondary is run as app container because each of
> containerized secondary has PID 1. To reserve unique name, use hostname
> instead of PID if the value is 1.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> ---

I'm not too well versed in containers - is this hostname 1) always set, 
and 2) always unique?

<snip>

> +	if (getpid() == 1) {
> +		FILE *hn_fp;
> +		hn_fp = fopen("/etc/hostname", "r");
> +		if (hn_fp == NULL) {
> +			RTE_LOG(ERR, EAL,
> +				"Cannot open '/etc/hostname' for secondary\n");
> +			return -1;
> +		}
> +
> +		/* with docker, /etc/hostname just has one entry of hostname */
> +		if (fscanf(hn_fp, "%s", proc_id) == EOF)
> +			return -1;

Wouldn't an error in fscanf() leak the file handle? I think you need to 
fclose() before checking the result.

> +		fclose(hn_fp);
> +	} else
> +		sprintf(proc_id, "%d", (int)getpid());
> +
> +	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
> +			primary_msl->memseg_arr.name, proc_id);
>   
>   	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
>   		primary_msl->memseg_arr.len,
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2 1/1] fbarray: get fbarrays from containerized secondary

[Yasufumi Ogawa]

Hi Anatoly,

On 2019/07/05 17:53, Burakov, Anatoly wrote:
> On 16-Apr-19 4:43 AM, ogawa.yasufumi@lab.ntt.co.jp wrote:
>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>
>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
>> PID for reserving unique name among secondary processes. However, it
>> does not work if secondary is run as app container because each of
>> containerized secondary has PID 1. To reserve unique name, use hostname
>> instead of PID if the value is 1.
>>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>> ---
> 
> I'm not too well versed in containers - is this hostname 1) always set, 
> and 2) always unique?
For docker, 1) hostname is always set. 2) The hostname is decided as 
short form of container ID, so it might not be unique even though very 
low possibility.

I found that we can get whole container ID in `/proc/self/cgroup` as 
discussed [1]. I think using hostname is reasonable way without running 
many secondary processes. However, it might be better to use 64 digits 
full container ID instead of 12 digits short ID if ensure uniqueness 
strongly. What do yo think?

[1] 
https://forums.docker.com/t/get-a-containers-full-id-from-inside-of-itself/37237
> 
> <snip>
> 
>> +    if (getpid() == 1) {
>> +        FILE *hn_fp;
>> +        hn_fp = fopen("/etc/hostname", "r");
>> +        if (hn_fp == NULL) {
>> +            RTE_LOG(ERR, EAL,
>> +                "Cannot open '/etc/hostname' for secondary\n");
>> +            return -1;
>> +        }
>> +
>> +        /* with docker, /etc/hostname just has one entry of hostname */
>> +        if (fscanf(hn_fp, "%s", proc_id) == EOF)
>> +            return -1;
> 
> Wouldn't an error in fscanf() leak the file handle? I think you need to 
> fclose() before checking the result.
I would like to fix it.

Regards,
Yasufumi
> 
>> +        fclose(hn_fp);
>> +    } else
>> +        sprintf(proc_id, "%d", (int)getpid());
>> +
>> +    snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
>> +            primary_msl->memseg_arr.name, proc_id);
>>       ret = rte_fbarray_init(&local_msl->memseg_arr, name,
>>           primary_msl->memseg_arr.len,
>>
> 
> 

Re: [dpdk-dev] [PATCH v2 1/1] fbarray: get fbarrays from containerized secondary

[Burakov, Anatoly]

On 09-Jul-19 11:22 AM, Yasufumi Ogawa wrote:
> Hi Anatoly,
> 
> On 2019/07/05 17:53, Burakov, Anatoly wrote:
>> On 16-Apr-19 4:43 AM, ogawa.yasufumi@lab.ntt.co.jp wrote:
>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>
>>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
>>> PID for reserving unique name among secondary processes. However, it
>>> does not work if secondary is run as app container because each of
>>> containerized secondary has PID 1. To reserve unique name, use hostname
>>> instead of PID if the value is 1.
>>>
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>> ---
>>
>> I'm not too well versed in containers - is this hostname 1) always 
>> set, and 2) always unique?
> For docker, 1) hostname is always set. 2) The hostname is decided as 
> short form of container ID, so it might not be unique even though very 
> low possibility.
> 
> I found that we can get whole container ID in `/proc/self/cgroup` as 
> discussed [1]. I think using hostname is reasonable way without running 
> many secondary processes. However, it might be better to use 64 digits 
> full container ID instead of 12 digits short ID if ensure uniqueness 
> strongly. What do yo think?
> 
> [1] 
> https://forums.docker.com/t/get-a-containers-full-id-from-inside-of-itself/37237 
> 

I think it's better to err on the side of caution and guarantee better 
uniqueness. This code will get into an LTS and will be used for years to 
come :)

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2 1/1] fbarray: get fbarrays from containerized secondary

[Burakov, Anatoly]

On 09-Jul-19 11:24 AM, Burakov, Anatoly wrote:
> On 09-Jul-19 11:22 AM, Yasufumi Ogawa wrote:
>> Hi Anatoly,
>>
>> On 2019/07/05 17:53, Burakov, Anatoly wrote:
>>> On 16-Apr-19 4:43 AM, ogawa.yasufumi@lab.ntt.co.jp wrote:
>>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>
>>>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
>>>> PID for reserving unique name among secondary processes. However, it
>>>> does not work if secondary is run as app container because each of
>>>> containerized secondary has PID 1. To reserve unique name, use hostname
>>>> instead of PID if the value is 1.
>>>>
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>> ---
>>>
>>> I'm not too well versed in containers - is this hostname 1) always 
>>> set, and 2) always unique?
>> For docker, 1) hostname is always set. 2) The hostname is decided as 
>> short form of container ID, so it might not be unique even though very 
>> low possibility.
>>
>> I found that we can get whole container ID in `/proc/self/cgroup` as 
>> discussed [1]. I think using hostname is reasonable way without 
>> running many secondary processes. However, it might be better to use 
>> 64 digits full container ID instead of 12 digits short ID if ensure 
>> uniqueness strongly. What do yo think?
>>
>> [1] 
>> https://forums.docker.com/t/get-a-containers-full-id-from-inside-of-itself/37237 
>>
> 
> I think it's better to err on the side of caution and guarantee better 
> uniqueness. This code will get into an LTS and will be used for years to 
> come :)
> 

...however, i think a full 64-digit ID won't even fit into the fbarray 
filename, as i believe it's limited to something like 64 chars. Perhaps 
hostname would be enough after all... or we can increase fbarray name 
length - that would require ABI breakage but the ABI is already broken 
in this release, so it's OK i think.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2 1/1] fbarray: get fbarrays from containerized secondary

[Yasufumi Ogawa]

On 2019/07/09 19:26, Burakov, Anatoly wrote:
> On 09-Jul-19 11:24 AM, Burakov, Anatoly wrote:
>> On 09-Jul-19 11:22 AM, Yasufumi Ogawa wrote:
>>> Hi Anatoly,
>>>
>>> On 2019/07/05 17:53, Burakov, Anatoly wrote:
>>>> On 16-Apr-19 4:43 AM, ogawa.yasufumi@lab.ntt.co.jp wrote:
>>>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>>
>>>>> In secondary_msl_create_walk(), it creates a file for fbarrays with 
>>>>> its
>>>>> PID for reserving unique name among secondary processes. However, it
>>>>> does not work if secondary is run as app container because each of
>>>>> containerized secondary has PID 1. To reserve unique name, use 
>>>>> hostname
>>>>> instead of PID if the value is 1.
>>>>>
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>> ---
>>>>
>>>> I'm not too well versed in containers - is this hostname 1) always 
>>>> set, and 2) always unique?
>>> For docker, 1) hostname is always set. 2) The hostname is decided as 
>>> short form of container ID, so it might not be unique even though 
>>> very low possibility.
>>>
>>> I found that we can get whole container ID in `/proc/self/cgroup` as 
>>> discussed [1]. I think using hostname is reasonable way without 
>>> running many secondary processes. However, it might be better to use 
>>> 64 digits full container ID instead of 12 digits short ID if ensure 
>>> uniqueness strongly. What do yo think?
>>>
>>> [1] 
>>> https://forums.docker.com/t/get-a-containers-full-id-from-inside-of-itself/37237 
>>>
>>
>> I think it's better to err on the side of caution and guarantee better 
>> uniqueness. This code will get into an LTS and will be used for years 
>> to come :)
>>
> 
> ...however, i think a full 64-digit ID won't even fit into the fbarray 
> filename, as i believe it's limited to something like 64 chars. Perhaps 
> hostname would be enough after all... or we can increase fbarray name 
> length - that would require ABI breakage but the ABI is already broken 
> in this release, so it's OK i think.
OK.

 >> Wouldn't an error in fscanf() leak the file handle? I think you need 
to fclose() before checking the result.
 > I would like to fix it.
I would like send v3 patch for fixing for fclose().

Thanks,
Yasufumi

Re: [dpdk-dev] [PATCH v2 1/1] fbarray: get fbarrays from containerized secondary

[Burakov, Anatoly]

On 11-Jul-19 10:37 AM, Yasufumi Ogawa wrote:
> On 2019/07/09 19:26, Burakov, Anatoly wrote:
>> On 09-Jul-19 11:24 AM, Burakov, Anatoly wrote:
>>> On 09-Jul-19 11:22 AM, Yasufumi Ogawa wrote:
>>>> Hi Anatoly,
>>>>
>>>> On 2019/07/05 17:53, Burakov, Anatoly wrote:
>>>>> On 16-Apr-19 4:43 AM, ogawa.yasufumi@lab.ntt.co.jp wrote:
>>>>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>>>
>>>>>> In secondary_msl_create_walk(), it creates a file for fbarrays 
>>>>>> with its
>>>>>> PID for reserving unique name among secondary processes. However, it
>>>>>> does not work if secondary is run as app container because each of
>>>>>> containerized secondary has PID 1. To reserve unique name, use 
>>>>>> hostname
>>>>>> instead of PID if the value is 1.
>>>>>>
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>>> ---
>>>>>
>>>>> I'm not too well versed in containers - is this hostname 1) always 
>>>>> set, and 2) always unique?
>>>> For docker, 1) hostname is always set. 2) The hostname is decided as 
>>>> short form of container ID, so it might not be unique even though 
>>>> very low possibility.
>>>>
>>>> I found that we can get whole container ID in `/proc/self/cgroup` as 
>>>> discussed [1]. I think using hostname is reasonable way without 
>>>> running many secondary processes. However, it might be better to use 
>>>> 64 digits full container ID instead of 12 digits short ID if ensure 
>>>> uniqueness strongly. What do yo think?
>>>>
>>>> [1] 
>>>> https://forums.docker.com/t/get-a-containers-full-id-from-inside-of-itself/37237 
>>>>
>>>
>>> I think it's better to err on the side of caution and guarantee 
>>> better uniqueness. This code will get into an LTS and will be used 
>>> for years to come :)
>>>
>>
>> ...however, i think a full 64-digit ID won't even fit into the fbarray 
>> filename, as i believe it's limited to something like 64 chars. 
>> Perhaps hostname would be enough after all... or we can increase 
>> fbarray name length - that would require ABI breakage but the ABI is 
>> already broken in this release, so it's OK i think.
> OK.

Just a note: you're targetting this fix towards stable too. For stable, 
you cannot break ABI, so we would have to do with the shorter hostname. 
It's only for 19.08 that you can change fbarray length and use the full 
64-char container ID for uniqueness.

> 
>  >> Wouldn't an error in fscanf() leak the file handle? I think you need 
> to fclose() before checking the result.
>  > I would like to fix it.
> I would like send v3 patch for fixing for fclose().

Please do :)

> 
> Thanks,
> Yasufumi
> 
> 


-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v3 0/1] fbarray: get fbarrays from containerized secondary

[yasufum.o]

From: Yasufumi Ogawa <yasufum.o@gmail.com>

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work if secondary is run as app container because each of
containerized secondary has PID 1. To reserve unique name, use hostname
instead of PID because hostname is assigned as a short form of 64
digits full container ID in docker.

---
v2:
  * fix typo in commit message
v3:
  * add fclose() after if getting hostname with fscan() is failed
---

Yasufumi Ogawa (1):
  fbarray: get fbarrays from containerized secondary

 lib/librte_eal/linux/eal/eal_memalloc.c | 28 +++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v3 1/1] fbarray: get fbarrays from containerized secondary

[yasufum.o]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work if secondary is run as app container because each of
containerized secondary has PID 1. To reserve unique name, use hostname
instead of PID because hostname is assigned as a short form of 64
digits full container ID in docker.

Cc: stable@dpdk.org

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 lib/librte_eal/linux/eal/eal_memalloc.c | 28 +++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
index 1f6a7c18f..7cbc80718 100644
--- a/lib/librte_eal/linux/eal/eal_memalloc.c
+++ b/lib/librte_eal/linux/eal/eal_memalloc.c
@@ -1366,6 +1366,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	struct rte_memseg_list *primary_msl, *local_msl;
 	char name[PATH_MAX];
 	int msl_idx, ret;
+	char proc_id[16];
 
 	if (msl->external)
 		return 0;
@@ -1375,8 +1376,31 @@ 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());
+	/* If run secondary in a container, the name of fbarray file should
+	 * not be decided with pid because getpid() always returns 1.
+	 * In docker, hostname is assigned as a short form of full container
+	 * ID. So use hostname as unique ID among containers instead.
+	 */
+	if (getpid() == 1) {
+		FILE *hn_fp;
+		hn_fp = fopen("/etc/hostname", "r");
+		if (hn_fp == NULL) {
+			RTE_LOG(ERR, EAL,
+				"Cannot open '/etc/hostname' for secondary\n");
+			return -1;
+		}
+
+		/* with docker, /etc/hostname just has one entry of hostname */
+		if (fscanf(hn_fp, "%s", proc_id) == EOF) {
+			fclose(hn_fp);
+			return -1;
+		}
+		fclose(hn_fp);
+	} else
+		sprintf(proc_id, "%d", (int)getpid());
+
+	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
+			primary_msl->memseg_arr.name, proc_id);
 
 	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
 		primary_msl->memseg_arr.len,
-- 
2.17.1

Re: [dpdk-dev] [PATCH v3 1/1] fbarray: get fbarrays from containerized secondary

[Burakov, Anatoly]

On 11-Jul-19 11:31 AM, yasufum.o@gmail.com wrote:
> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> 
> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> PID for reserving unique name among secondary processes. However, it
> does not work if secondary is run as app container because each of
> containerized secondary has PID 1. To reserve unique name, use hostname
> instead of PID because hostname is assigned as a short form of 64
> digits full container ID in docker.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> ---

<...>

> +	if (getpid() == 1) {
> +		FILE *hn_fp;
> +		hn_fp = fopen("/etc/hostname", "r");
> +		if (hn_fp == NULL) {
> +			RTE_LOG(ERR, EAL,
> +				"Cannot open '/etc/hostname' for secondary\n");
> +			return -1;
> +		}
> +
> +		/* with docker, /etc/hostname just has one entry of hostname */
> +		if (fscanf(hn_fp, "%s", proc_id) == EOF) {

Apologies for not pointing this out earlier, but do i understand 
correctly that there's no bounds checking here, and fscanf() will write 
however many bytes it wants?

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3 1/1] fbarray: get fbarrays from containerized secondary

[Yasufumi Ogawa]

On 2019/07/11 19:53, Burakov, Anatoly wrote:
> On 11-Jul-19 11:31 AM, yasufum.o@gmail.com wrote:
>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>
> <...>
> 
>> +    if (getpid() == 1) {
>> +        FILE *hn_fp;
>> +        hn_fp = fopen("/etc/hostname", "r");
>> +        if (hn_fp == NULL) {
>> +            RTE_LOG(ERR, EAL,
>> +                "Cannot open '/etc/hostname' for secondary\n");
>> +            return -1;
>> +        }
>> +
>> +        /* with docker, /etc/hostname just has one entry of hostname */
>> +        if (fscanf(hn_fp, "%s", proc_id) == EOF) {
> 
> Apologies for not pointing this out earlier, but do i understand 
> correctly that there's no bounds checking here, and fscanf() will write 
> however many bytes it wants?
I understand "%s" is not appropriate. hostname is 12 bytes char and I 
thought proc_id[16] is enough, but it is unsafe. In addition, hostname 
can be defined by user with docker's option, so it should be enough for 
user defined name.

How do you think expecting max 32 chars of hostname and set boundary 
"%32s" as following?

     proc_id[33];  /* define proc id from hostname less than 33 bytes. */
     ...
     if (fscanf(hn_fp, "%32s", proc_id) == EOF) {

Thanks,
Yasufumi

Re: [dpdk-dev] [PATCH v3 1/1] fbarray: get fbarrays from containerized secondary

[Burakov, Anatoly]

On 11-Jul-19 12:57 PM, Yasufumi Ogawa wrote:
> On 2019/07/11 19:53, Burakov, Anatoly wrote:
>> On 11-Jul-19 11:31 AM, yasufum.o@gmail.com wrote:
>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>
>> <...>
>>
>>> +    if (getpid() == 1) {
>>> +        FILE *hn_fp;
>>> +        hn_fp = fopen("/etc/hostname", "r");
>>> +        if (hn_fp == NULL) {
>>> +            RTE_LOG(ERR, EAL,
>>> +                "Cannot open '/etc/hostname' for secondary\n");
>>> +            return -1;
>>> +        }
>>> +
>>> +        /* with docker, /etc/hostname just has one entry of hostname */
>>> +        if (fscanf(hn_fp, "%s", proc_id) == EOF) {
>>
>> Apologies for not pointing this out earlier, but do i understand 
>> correctly that there's no bounds checking here, and fscanf() will 
>> write however many bytes it wants?
> I understand "%s" is not appropriate. hostname is 12 bytes char and I 
> thought proc_id[16] is enough, but it is unsafe. In addition, hostname 
> can be defined by user with docker's option, so it should be enough for 
> user defined name.
> 
> How do you think expecting max 32 chars of hostname and set boundary 
> "%32s" as following?
> 
>      proc_id[33];  /* define proc id from hostname less than 33 bytes. */
>      ...
>      if (fscanf(hn_fp, "%32s", proc_id) == EOF) {
> 

As long as it takes NULL-termination into account as well, it should be 
OK. I can't recall off the top of my head if %32s includes NULL 
terminator (probably not?).

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3 1/1] fbarray: get fbarrays from containerized secondary

[Yasufumi Ogawa]

On 2019/07/11 22:14, Burakov, Anatoly wrote:
> On 11-Jul-19 12:57 PM, Yasufumi Ogawa wrote:
>> On 2019/07/11 19:53, Burakov, Anatoly wrote:
>>> On 11-Jul-19 11:31 AM, yasufum.o@gmail.com wrote:
>>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>
>>> <...>
>>>
>>>> +    if (getpid() == 1) {
>>>> +        FILE *hn_fp;
>>>> +        hn_fp = fopen("/etc/hostname", "r");
>>>> +        if (hn_fp == NULL) {
>>>> +            RTE_LOG(ERR, EAL,
>>>> +                "Cannot open '/etc/hostname' for secondary\n");
>>>> +            return -1;
>>>> +        }
>>>> +
>>>> +        /* with docker, /etc/hostname just has one entry of 
>>>> hostname */
>>>> +        if (fscanf(hn_fp, "%s", proc_id) == EOF) {
>>>
>>> Apologies for not pointing this out earlier, but do i understand 
>>> correctly that there's no bounds checking here, and fscanf() will 
>>> write however many bytes it wants?
>> I understand "%s" is not appropriate. hostname is 12 bytes char and I 
>> thought proc_id[16] is enough, but it is unsafe. In addition, hostname 
>> can be defined by user with docker's option, so it should be enough 
>> for user defined name.
>>
>> How do you think expecting max 32 chars of hostname and set boundary 
>> "%32s" as following?
>>
>>      proc_id[33];  /* define proc id from hostname less than 33 bytes. */
>>      ...
>>      if (fscanf(hn_fp, "%32s", proc_id) == EOF) {
>>
> 
> As long as it takes NULL-termination into account as well, it should be 
> OK. I can't recall off the top of my head if %32s includes NULL 
> terminator (probably not?).
Do you agree if initialize with NULL chars to ensure proc_id is 
NULL-terminated? As tested on my environment, "%Ns" sets next of Nth 
char as NULL, but it seems more reliable.
     proc_id[33] = { 0 };

Yasufumi

Re: [dpdk-dev] [PATCH v3 1/1] fbarray: get fbarrays from containerized secondary

[Ogawa Yasufumi]

2019年7月12日(金) 11:22 Yasufumi Ogawa <yasufum.o@gmail.com>:

> On 2019/07/11 22:14, Burakov, Anatoly wrote:
> > On 11-Jul-19 12:57 PM, Yasufumi Ogawa wrote:
> >> On 2019/07/11 19:53, Burakov, Anatoly wrote:
> >>> On 11-Jul-19 11:31 AM, yasufum.o@gmail.com wrote:
> >>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> >>>>
> >>> <...>
> >>>
> >>>> +    if (getpid() == 1) {
> >>>> +        FILE *hn_fp;
> >>>> +        hn_fp = fopen("/etc/hostname", "r");
> >>>> +        if (hn_fp == NULL) {
> >>>> +            RTE_LOG(ERR, EAL,
> >>>> +                "Cannot open '/etc/hostname' for secondary\n");
> >>>> +            return -1;
> >>>> +        }
> >>>> +
> >>>> +        /* with docker, /etc/hostname just has one entry of
> >>>> hostname */
> >>>> +        if (fscanf(hn_fp, "%s", proc_id) == EOF) {
> >>>
> >>> Apologies for not pointing this out earlier, but do i understand
> >>> correctly that there's no bounds checking here, and fscanf() will
> >>> write however many bytes it wants?
> >> I understand "%s" is not appropriate. hostname is 12 bytes char and I
> >> thought proc_id[16] is enough, but it is unsafe. In addition, hostname
> >> can be defined by user with docker's option, so it should be enough
> >> for user defined name.
> >>
> >> How do you think expecting max 32 chars of hostname and set boundary
> >> "%32s" as following?
> >>
> >>      proc_id[33];  /* define proc id from hostname less than 33 bytes.
> */
> >>      ...
> >>      if (fscanf(hn_fp, "%32s", proc_id) == EOF) {
> >>
> >
> > As long as it takes NULL-termination into account as well, it should be
> > OK. I can't recall off the top of my head if %32s includes NULL
> > terminator (probably not?).
> Do you agree if initialize with NULL chars to ensure proc_id is
> NULL-terminated? As tested on my environment, "%Ns" sets next of Nth
> char as NULL, but it seems more reliable.
>      proc_id[33] = { 0 };
>
Hi Anatoly,

I would like to send v4 patch if it is agreeable.

>
> Yasufumi
>

Re: [dpdk-dev] [PATCH v3 1/1] fbarray: get fbarrays from containerized secondary

[Burakov, Anatoly]

On 12-Jul-19 3:22 AM, Yasufumi Ogawa wrote:
> On 2019/07/11 22:14, Burakov, Anatoly wrote:
>> On 11-Jul-19 12:57 PM, Yasufumi Ogawa wrote:
>>> On 2019/07/11 19:53, Burakov, Anatoly wrote:
>>>> On 11-Jul-19 11:31 AM, yasufum.o@gmail.com wrote:
>>>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>>
>>>> <...>
>>>>
>>>>> +    if (getpid() == 1) {
>>>>> +        FILE *hn_fp;
>>>>> +        hn_fp = fopen("/etc/hostname", "r");
>>>>> +        if (hn_fp == NULL) {
>>>>> +            RTE_LOG(ERR, EAL,
>>>>> +                "Cannot open '/etc/hostname' for secondary\n");
>>>>> +            return -1;
>>>>> +        }
>>>>> +
>>>>> +        /* with docker, /etc/hostname just has one entry of 
>>>>> hostname */
>>>>> +        if (fscanf(hn_fp, "%s", proc_id) == EOF) {
>>>>
>>>> Apologies for not pointing this out earlier, but do i understand 
>>>> correctly that there's no bounds checking here, and fscanf() will 
>>>> write however many bytes it wants?
>>> I understand "%s" is not appropriate. hostname is 12 bytes char and I 
>>> thought proc_id[16] is enough, but it is unsafe. In addition, 
>>> hostname can be defined by user with docker's option, so it should be 
>>> enough for user defined name.
>>>
>>> How do you think expecting max 32 chars of hostname and set boundary 
>>> "%32s" as following?
>>>
>>>      proc_id[33];  /* define proc id from hostname less than 33 
>>> bytes. */
>>>      ...
>>>      if (fscanf(hn_fp, "%32s", proc_id) == EOF) {
>>>
>>
>> As long as it takes NULL-termination into account as well, it should 
>> be OK. I can't recall off the top of my head if %32s includes NULL 
>> terminator (probably not?).
> Do you agree if initialize with NULL chars to ensure proc_id is 
> NULL-terminated? As tested on my environment, "%Ns" sets next of Nth 
> char as NULL, but it seems more reliable.
>      proc_id[33] = { 0 };
> 
> Yasufumi
> 

Yes, that should be OK.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3 1/1] fbarray: get fbarrays from containerized secondary

[Burakov, Anatoly]

On 22-Jul-19 2:06 AM, Ogawa Yasufumi wrote:
> 
> 
> 2019年7月12日(金) 11:22 Yasufumi Ogawa <yasufum.o@gmail.com 
> <mailto:yasufum.o@gmail.com>>:
> 
>     On 2019/07/11 22:14, Burakov, Anatoly wrote:
>      > On 11-Jul-19 12:57 PM, Yasufumi Ogawa wrote:
>      >> On 2019/07/11 19:53, Burakov, Anatoly wrote:
>      >>> On 11-Jul-19 11:31 AM, yasufum.o@gmail.com
>     <mailto:yasufum.o@gmail.com> wrote:
>      >>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp
>     <mailto:ogawa.yasufumi@lab.ntt.co.jp>>
>      >>>>
>      >>> <...>
>      >>>
>      >>>> +    if (getpid() == 1) {
>      >>>> +        FILE *hn_fp;
>      >>>> +        hn_fp = fopen("/etc/hostname", "r");
>      >>>> +        if (hn_fp == NULL) {
>      >>>> +            RTE_LOG(ERR, EAL,
>      >>>> +                "Cannot open '/etc/hostname' for secondary\n");
>      >>>> +            return -1;
>      >>>> +        }
>      >>>> +
>      >>>> +        /* with docker, /etc/hostname just has one entry of
>      >>>> hostname */
>      >>>> +        if (fscanf(hn_fp, "%s", proc_id) == EOF) {
>      >>>
>      >>> Apologies for not pointing this out earlier, but do i understand
>      >>> correctly that there's no bounds checking here, and fscanf() will
>      >>> write however many bytes it wants?
>      >> I understand "%s" is not appropriate. hostname is 12 bytes char
>     and I
>      >> thought proc_id[16] is enough, but it is unsafe. In addition,
>     hostname
>      >> can be defined by user with docker's option, so it should be enough
>      >> for user defined name.
>      >>
>      >> How do you think expecting max 32 chars of hostname and set
>     boundary
>      >> "%32s" as following?
>      >>
>      >>      proc_id[33];  /* define proc id from hostname less than 33
>     bytes. */
>      >>      ...
>      >>      if (fscanf(hn_fp, "%32s", proc_id) == EOF) {
>      >>
>      >
>      > As long as it takes NULL-termination into account as well, it
>     should be
>      > OK. I can't recall off the top of my head if %32s includes NULL
>      > terminator (probably not?).
>     Do you agree if initialize with NULL chars to ensure proc_id is
>     NULL-terminated? As tested on my environment, "%Ns" sets next of Nth
>     char as NULL, but it seems more reliable.
>           proc_id[33] = { 0 };
> 
> Hi Anatoly,
> 
> I would like to send v4 patch if it is agreeable.

Yes, please do.

As a side note, you don't need to ask anyone's permission to send a patch :)

> 
> 
>     Yasufumi
> 


-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v4 0/1] fbarray: get fbarrays from containerized secondary

[yasufum.o]

From: Yasufumi Ogawa <yasufum.o@gmail.com>

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work if secondary is run as app container because each of
containerized secondary has PID 1. To reserve unique name, use hostname
instead of PID because hostname is assigned as a short form of 64
digits full container ID in docker.

---
v2:
  * fix typo in commit message
v3:
  * add fclose() after if getting hostname with fscan() is failed
v4:
  * Increase the size of proc_id to 33 and add boundary in calling
    fscan()
---

Yasufumi Ogawa (1):
  fbarray: get fbarrays from containerized secondary

 lib/librte_eal/linux/eal/eal_memalloc.c | 28 +++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v4 1/1] fbarray: get fbarrays from containerized secondary

[yasufum.o]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work if secondary is run as app container because each of
containerized secondary has PID 1. To reserve unique name, use hostname
instead of PID because hostname is assigned as a short form of 64
digits full container ID in docker.

Cc: stable@dpdk.org

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 lib/librte_eal/linux/eal/eal_memalloc.c | 28 +++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
index 1f6a7c18f..356b304a8 100644
--- a/lib/librte_eal/linux/eal/eal_memalloc.c
+++ b/lib/librte_eal/linux/eal/eal_memalloc.c
@@ -1366,6 +1366,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	struct rte_memseg_list *primary_msl, *local_msl;
 	char name[PATH_MAX];
 	int msl_idx, ret;
+	char proc_id[33] = { 0 };  /* 32bytes is enough if using hostname */
 
 	if (msl->external)
 		return 0;
@@ -1375,8 +1376,31 @@ 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());
+	/* If run secondary in a container, the name of fbarray file should
+	 * not be decided with pid because getpid() always returns 1.
+	 * In docker, hostname is assigned as a short form of full container
+	 * ID. So use hostname as unique ID among containers instead.
+	 */
+	if (getpid() == 1) {
+		FILE *hn_fp;
+		hn_fp = fopen("/etc/hostname", "r");
+		if (hn_fp == NULL) {
+			RTE_LOG(ERR, EAL,
+				"Cannot open '/etc/hostname' for secondary\n");
+			return -1;
+		}
+
+		/* with docker, /etc/hostname just has one entry of hostname */
+		if (fscanf(hn_fp, "%32s", proc_id) == EOF) {
+			fclose(hn_fp);
+			return -1;
+		}
+		fclose(hn_fp);
+	} else
+		sprintf(proc_id, "%d", (int)getpid());
+
+	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
+			primary_msl->memseg_arr.name, proc_id);
 
 	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
 		primary_msl->memseg_arr.len,
-- 
2.17.1

Re: [dpdk-dev] [PATCH v4 1/1] fbarray: get fbarrays from containerized secondary

[Burakov, Anatoly]

On 24-Jul-19 9:20 AM, yasufum.o@gmail.com wrote:
> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> 
> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> PID for reserving unique name among secondary processes. However, it
> does not work if secondary is run as app container because each of
> containerized secondary has PID 1. To reserve unique name, use hostname
> instead of PID because hostname is assigned as a short form of 64
> digits full container ID in docker.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [dpdk-stable] [PATCH v4 1/1] fbarray: get fbarrays from containerized secondary

[Thomas Monjalon]

24/07/2019 11:59, Burakov, Anatoly:
> On 24-Jul-19 9:20 AM, yasufum.o@gmail.com wrote:
> > From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> > 
> > In secondary_msl_create_walk(), it creates a file for fbarrays with its
> > PID for reserving unique name among secondary processes. However, it
> > does not work if secondary is run as app container because each of
> > containerized secondary has PID 1. To reserve unique name, use hostname
> > instead of PID because hostname is assigned as a short form of 64
> > digits full container ID in docker.
> > 
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

This may change the behaviour, so this fix is deferred to 19.11
release cycle. OK?

Re: [dpdk-dev] [dpdk-stable] [PATCH v4 1/1] fbarray: get fbarrays from containerized secondary

[Burakov, Anatoly]

On 30-Jul-19 9:16 AM, Thomas Monjalon wrote:
> 24/07/2019 11:59, Burakov, Anatoly:
>> On 24-Jul-19 9:20 AM, yasufum.o@gmail.com wrote:
>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>
>>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
>>> PID for reserving unique name among secondary processes. However, it
>>> does not work if secondary is run as app container because each of
>>> containerized secondary has PID 1. To reserve unique name, use hostname
>>> instead of PID because hostname is assigned as a short form of 64
>>> digits full container ID in docker.
>>>
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>
>> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> This may change the behaviour, so this fix is deferred to 19.11
> release cycle. OK?
> 

I'm fine with that.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [dpdk-stable] [PATCH v4 1/1] fbarray: get fbarrays from containerized secondary

[Yasufumi Ogawa]

On 2019/07/30 18:18, Burakov, Anatoly wrote:
> On 30-Jul-19 9:16 AM, Thomas Monjalon wrote:
>> 24/07/2019 11:59, Burakov, Anatoly:
>>> On 24-Jul-19 9:20 AM, yasufum.o@gmail.com wrote:
>>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>
>>>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
>>>> PID for reserving unique name among secondary processes. However, it
>>>> does not work if secondary is run as app container because each of
>>>> containerized secondary has PID 1. To reserve unique name, use hostname
>>>> instead of PID because hostname is assigned as a short form of 64
>>>> digits full container ID in docker.
>>>>
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>
>>> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>
>> This may change the behaviour, so this fix is deferred to 19.11
>> release cycle. OK?
>>
> 
> I'm fine with that.
I am also fine.

Re: [dpdk-dev] [PATCH v4 1/1] fbarray: get fbarrays from containerized secondary

[David Marchand]

Some comments.

The title does not reflect the observed issue.
I understand that secondary processeses can't be started from a docker
container.
The patch title should reflect this.

On Wed, Jul 24, 2019 at 10:20 AM <yasufum.o@gmail.com> wrote:
>
> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>
> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> PID for reserving unique name among secondary processes. However, it
> does not work if secondary is run as app container because each of
> containerized secondary has PID 1. To reserve unique name, use hostname
> instead of PID because hostname is assigned as a short form of 64
> digits full container ID in docker.
>
> Cc: stable@dpdk.org

I don't think we want to backport this behavior change.

>
> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> ---
>  lib/librte_eal/linux/eal/eal_memalloc.c | 28 +++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
> index 1f6a7c18f..356b304a8 100644
> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
> @@ -1366,6 +1366,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>         struct rte_memseg_list *primary_msl, *local_msl;
>         char name[PATH_MAX];
>         int msl_idx, ret;
> +       char proc_id[33] = { 0 };  /* 32bytes is enough if using hostname */

This variable only makes sense in the if (getpid() == 1) branch,
please move it there, and see below comment about using gethostname().

>
>         if (msl->external)
>                 return 0;
> @@ -1375,8 +1376,31 @@ 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());
> +       /* If run secondary in a container, the name of fbarray file should
> +        * not be decided with pid because getpid() always returns 1.
> +        * In docker, hostname is assigned as a short form of full container
> +        * ID. So use hostname as unique ID among containers instead.

I understand this is how it works for docker.
Is this the same in other container environments?


> +        */
> +       if (getpid() == 1) {
> +               FILE *hn_fp;
> +               hn_fp = fopen("/etc/hostname", "r");

Why not use gethostname() ?
Plus, this api defines the maximum size of the hostname as HOST_NAME_MAX bytes.

> +               if (hn_fp == NULL) {
> +                       RTE_LOG(ERR, EAL,
> +                               "Cannot open '/etc/hostname' for secondary\n");
> +                       return -1;
> +               }
> +
> +               /* with docker, /etc/hostname just has one entry of hostname */
> +               if (fscanf(hn_fp, "%32s", proc_id) == EOF) {
> +                       fclose(hn_fp);
> +                       return -1;
> +               }
> +               fclose(hn_fp);
> +       } else
> +               sprintf(proc_id, "%d", (int)getpid());
> +
> +       snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
> +                       primary_msl->memseg_arr.name, proc_id);
>
>         ret = rte_fbarray_init(&local_msl->memseg_arr, name,
>                 primary_msl->memseg_arr.len,
> --
> 2.17.1
>


-- 
David Marchand

Re: [dpdk-dev] [PATCH v4 1/1] fbarray: get fbarrays from containerized secondary

[David Marchand]

On Fri, Oct 11, 2019 at 11:36 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> Some comments.

ping.


>
> The title does not reflect the observed issue.
> I understand that secondary processeses can't be started from a docker
> container.
> The patch title should reflect this.
>
> On Wed, Jul 24, 2019 at 10:20 AM <yasufum.o@gmail.com> wrote:
> >
> > From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> >
> > In secondary_msl_create_walk(), it creates a file for fbarrays with its
> > PID for reserving unique name among secondary processes. However, it
> > does not work if secondary is run as app container because each of
> > containerized secondary has PID 1. To reserve unique name, use hostname
> > instead of PID because hostname is assigned as a short form of 64
> > digits full container ID in docker.
> >
> > Cc: stable@dpdk.org
>
> I don't think we want to backport this behavior change.
>
> >
> > Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> > ---
> >  lib/librte_eal/linux/eal/eal_memalloc.c | 28 +++++++++++++++++++++++--
> >  1 file changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
> > index 1f6a7c18f..356b304a8 100644
> > --- a/lib/librte_eal/linux/eal/eal_memalloc.c
> > +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
> > @@ -1366,6 +1366,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
> >         struct rte_memseg_list *primary_msl, *local_msl;
> >         char name[PATH_MAX];
> >         int msl_idx, ret;
> > +       char proc_id[33] = { 0 };  /* 32bytes is enough if using hostname */
>
> This variable only makes sense in the if (getpid() == 1) branch,
> please move it there, and see below comment about using gethostname().
>
> >
> >         if (msl->external)
> >                 return 0;
> > @@ -1375,8 +1376,31 @@ 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());
> > +       /* If run secondary in a container, the name of fbarray file should
> > +        * not be decided with pid because getpid() always returns 1.
> > +        * In docker, hostname is assigned as a short form of full container
> > +        * ID. So use hostname as unique ID among containers instead.
>
> I understand this is how it works for docker.
> Is this the same in other container environments?
>
>
> > +        */
> > +       if (getpid() == 1) {
> > +               FILE *hn_fp;
> > +               hn_fp = fopen("/etc/hostname", "r");
>
> Why not use gethostname() ?
> Plus, this api defines the maximum size of the hostname as HOST_NAME_MAX bytes.
>
> > +               if (hn_fp == NULL) {
> > +                       RTE_LOG(ERR, EAL,
> > +                               "Cannot open '/etc/hostname' for secondary\n");
> > +                       return -1;
> > +               }
> > +
> > +               /* with docker, /etc/hostname just has one entry of hostname */
> > +               if (fscanf(hn_fp, "%32s", proc_id) == EOF) {
> > +                       fclose(hn_fp);
> > +                       return -1;
> > +               }
> > +               fclose(hn_fp);
> > +       } else
> > +               sprintf(proc_id, "%d", (int)getpid());
> > +
> > +       snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
> > +                       primary_msl->memseg_arr.name, proc_id);
> >
> >         ret = rte_fbarray_init(&local_msl->memseg_arr, name,
> >                 primary_msl->memseg_arr.len,


-- 
David Marchand

Re: [dpdk-dev] [PATCH v4 1/1] fbarray: get fbarrays from containerized secondary

[Yasufumi Ogawa]

Hi David,

Thank you for your comments.

On 2019/10/26 0:36, David Marchand wrote:
> On Fri, Oct 11, 2019 at 11:36 AM David Marchand
> <david.marchand@redhat.com> wrote:
>>
>> Some comments.
> 
> ping.
> 
> 
>>
>> The title does not reflect the observed issue.
I would like to consider to revise it.

>> I understand that secondary processeses can't be started from a docker
>> container.
I've confirmed that secondary process can be started as a container app 
with SPP, and DPDK v18.08 and v19.08. SPP is a multi-process app 
supporting container usecases.
http://git.dpdk.org/apps/spp/

>> The patch title should reflect this.
>>
>> On Wed, Jul 24, 2019 at 10:20 AM <yasufum.o@gmail.com> wrote:
>>>
>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>
>>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
>>> PID for reserving unique name among secondary processes. However, it
>>> does not work if secondary is run as app container because each of
>>> containerized secondary has PID 1. To reserve unique name, use hostname
>>> instead of PID because hostname is assigned as a short form of 64
>>> digits full container ID in docker.
>>>
>>> Cc: stable@dpdk.org
>>
>> I don't think we want to backport this behavior change.
This issue was included from DPDK v18.08, and some users of SPP are 
still using stable 18.11. So, I would appreciate if you agree to backport.

>>
>>>
>>> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>> ---
>>>   lib/librte_eal/linux/eal/eal_memalloc.c | 28 +++++++++++++++++++++++--
>>>   1 file changed, 26 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
>>> index 1f6a7c18f..356b304a8 100644
>>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
>>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
>>> @@ -1366,6 +1366,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>>>          struct rte_memseg_list *primary_msl, *local_msl;
>>>          char name[PATH_MAX];
>>>          int msl_idx, ret;
>>> +       char proc_id[33] = { 0 };  /* 32bytes is enough if using hostname */
>>
>> This variable only makes sense in the if (getpid() == 1) branch,
>> please move it there, and see below comment about using gethostname().
Sure. It works correctly in secondary app container and I should replace it.

>>
>>>
>>>          if (msl->external)
>>>                  return 0;
>>> @@ -1375,8 +1376,31 @@ 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());
>>> +       /* If run secondary in a container, the name of fbarray file should
>>> +        * not be decided with pid because getpid() always returns 1.
>>> +        * In docker, hostname is assigned as a short form of full container
>>> +        * ID. So use hostname as unique ID among containers instead.
>>
>> I understand this is how it works for docker.
>> Is this the same in other container environments?
Umm... I've tested on other than docker because I don't have test 
environment. I am not sure which of container runtimes should be 
supported actually. I think it is enough as the first step to fix this 
issue of docker. Moreover, the essential problem is that getpid() 
returns 1 in docker or not.

I am also not sure which of environments other than docker should be 
supported if necessary. What do you think?

>>
>>
>>> +        */
>>> +       if (getpid() == 1) {
>>> +               FILE *hn_fp;
>>> +               hn_fp = fopen("/etc/hostname", "r");
>>
>> Why not use gethostname() ?
>> Plus, this api defines the maximum size of the hostname as HOST_NAME_MAX bytes.
Yes. I should use gethostname() and replace hardcoded size with 
HOST_NAME_MAX.

Thanks,
Yasufumi

>>
>>> +               if (hn_fp == NULL) {
>>> +                       RTE_LOG(ERR, EAL,
>>> +                               "Cannot open '/etc/hostname' for secondary\n");
>>> +                       return -1;
>>> +               }
>>> +
>>> +               /* with docker, /etc/hostname just has one entry of hostname */
>>> +               if (fscanf(hn_fp, "%32s", proc_id) == EOF) {
>>> +                       fclose(hn_fp);
>>> +                       return -1;
>>> +               }
>>> +               fclose(hn_fp);
>>> +       } else
>>> +               sprintf(proc_id, "%d", (int)getpid());
>>> +
>>> +       snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
>>> +                       primary_msl->memseg_arr.name, proc_id);
>>>
>>>          ret = rte_fbarray_init(&local_msl->memseg_arr, name,
>>>                  primary_msl->memseg_arr.len,
> 
> 

Re: [dpdk-dev] [PATCH v4 1/1] fbarray: get fbarrays from containerized secondary

[David Marchand]

On Fri, Oct 25, 2019 at 9:55 PM Yasufumi Ogawa <yasufum.o@gmail.com> wrote:
> >>
> >> The title does not reflect the observed issue.
> I would like to consider to revise it.
>
> >> I understand that secondary processeses can't be started from a docker
> >> container.
> I've confirmed that secondary process can be started as a container app
> with SPP, and DPDK v18.08 and v19.08. SPP is a multi-process app
> supporting container usecases.
> http://git.dpdk.org/apps/spp/

Sorry, I don't understand.
Do you mean the secondary processes can be run from containers without
this patch?
Or once this patch is applied?


>
> >> The patch title should reflect this.
> >>
> >> On Wed, Jul 24, 2019 at 10:20 AM <yasufum.o@gmail.com> wrote:
> >>>
> >>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> >>>
> >>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> >>> PID for reserving unique name among secondary processes. However, it
> >>> does not work if secondary is run as app container because each of
> >>> containerized secondary has PID 1. To reserve unique name, use hostname
> >>> instead of PID because hostname is assigned as a short form of 64
> >>> digits full container ID in docker.
> >>>
> >>> Cc: stable@dpdk.org
> >>
> >> I don't think we want to backport this behavior change.
> This issue was included from DPDK v18.08, and some users of SPP are
> still using stable 18.11. So, I would appreciate if you agree to backport.

This can be discussed later.
Ok to keep stable in CC: then.


>
> >>
> >>>
> >>> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> >>> ---
> >>>   lib/librte_eal/linux/eal/eal_memalloc.c | 28 +++++++++++++++++++++++--
> >>>   1 file changed, 26 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
> >>> index 1f6a7c18f..356b304a8 100644
> >>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
> >>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
> >>> @@ -1366,6 +1366,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
> >>>          struct rte_memseg_list *primary_msl, *local_msl;
> >>>          char name[PATH_MAX];
> >>>          int msl_idx, ret;
> >>> +       char proc_id[33] = { 0 };  /* 32bytes is enough if using hostname */
> >>
> >> This variable only makes sense in the if (getpid() == 1) branch,
> >> please move it there, and see below comment about using gethostname().
> Sure. It works correctly in secondary app container and I should replace it.

Great, can you send a new version of this patch?

>
> >>
> >>>
> >>>          if (msl->external)
> >>>                  return 0;
> >>> @@ -1375,8 +1376,31 @@ 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());
> >>> +       /* If run secondary in a container, the name of fbarray file should
> >>> +        * not be decided with pid because getpid() always returns 1.
> >>> +        * In docker, hostname is assigned as a short form of full container
> >>> +        * ID. So use hostname as unique ID among containers instead.
> >>
> >> I understand this is how it works for docker.
> >> Is this the same in other container environments?
> Umm... I've tested on other than docker because I don't have test
> environment. I am not sure which of container runtimes should be
> supported actually. I think it is enough as the first step to fix this
> issue of docker. Moreover, the essential problem is that getpid()
> returns 1 in docker or not.
>
> I am also not sure which of environments other than docker should be
> supported if necessary. What do you think?

No problem if you don't know.



--
David Marchand

Re: [dpdk-dev] [PATCH v4 1/1] fbarray: get fbarrays from containerized secondary

[Yasufumi Ogawa]

On 2019/10/27 1:15, David Marchand wrote:
> On Fri, Oct 25, 2019 at 9:55 PM Yasufumi Ogawa <yasufum.o@gmail.com> wrote:
>>>>
>>>> The title does not reflect the observed issue.
>> I would like to consider to revise it.
>>
>>>> I understand that secondary processeses can't be started from a docker
>>>> container.
>> I've confirmed that secondary process can be started as a container app
>> with SPP, and DPDK v18.08 and v19.08. SPP is a multi-process app
>> supporting container usecases.
>> http://git.dpdk.org/apps/spp/
> 
> Sorry, I don't understand.
> Do you mean the secondary processes can be run from containers without
> this patch?
> Or once this patch is applied?
Secondary processes can be run from a container. The problem is we 
cannot run two or more secondary app containers. It is because each of 
secondary app container tries to create its metadata file and the name 
is decided with PID, and PID is 1 in all of containers. It means that 
all of secondaries try to have the same name of metadata file, and 
failed to create the same file. The first container app can create 
metadata file with PID 1, but failed to create second one with the same 
PID 1.

This patch is to change creating metadata file from using PID 1 to 
hostname which is unique in each of secondary containers.

To summarize, we can run just one secondary app container without this 
patch, but cannot two or more secondary app containers.

> 
> 
>>
>>>> The patch title should reflect this.
>>>>
>>>> On Wed, Jul 24, 2019 at 10:20 AM <yasufum.o@gmail.com> wrote:
>>>>>
>>>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>>
>>>>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
>>>>> PID for reserving unique name among secondary processes. However, it
>>>>> does not work if secondary is run as app container because each of
>>>>> containerized secondary has PID 1. To reserve unique name, use hostname
>>>>> instead of PID because hostname is assigned as a short form of 64
>>>>> digits full container ID in docker.
>>>>>
>>>>> Cc: stable@dpdk.org
>>>>
>>>> I don't think we want to backport this behavior change.
>> This issue was included from DPDK v18.08, and some users of SPP are
>> still using stable 18.11. So, I would appreciate if you agree to backport.
> 
> This can be discussed later.
> Ok to keep stable in CC: then.
Thanks.

> 
> 
>>
>>>>
>>>>>
>>>>> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>> ---
>>>>>    lib/librte_eal/linux/eal/eal_memalloc.c | 28 +++++++++++++++++++++++--
>>>>>    1 file changed, 26 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>> index 1f6a7c18f..356b304a8 100644
>>>>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>> @@ -1366,6 +1366,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>>>>>           struct rte_memseg_list *primary_msl, *local_msl;
>>>>>           char name[PATH_MAX];
>>>>>           int msl_idx, ret;
>>>>> +       char proc_id[33] = { 0 };  /* 32bytes is enough if using hostname */
>>>>
>>>> This variable only makes sense in the if (getpid() == 1) branch,
>>>> please move it there, and see below comment about using gethostname().
>> Sure. It works correctly in secondary app container and I should replace it.
> 
> Great, can you send a new version of this patch?
Yes, I will fix it soon.

> 
>>
>>>>
>>>>>
>>>>>           if (msl->external)
>>>>>                   return 0;
>>>>> @@ -1375,8 +1376,31 @@ 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());
>>>>> +       /* If run secondary in a container, the name of fbarray file should
>>>>> +        * not be decided with pid because getpid() always returns 1.
>>>>> +        * In docker, hostname is assigned as a short form of full container
>>>>> +        * ID. So use hostname as unique ID among containers instead.
>>>>
>>>> I understand this is how it works for docker.
>>>> Is this the same in other container environments?
>> Umm... I've tested on other than docker because I don't have test
>> environment. I am not sure which of container runtimes should be
>> supported actually. I think it is enough as the first step to fix this
>> issue of docker. Moreover, the essential problem is that getpid()
>> returns 1 in docker or not.
>>
>> I am also not sure which of environments other than docker should be
>> supported if necessary. What do you think?
> 
> No problem if you don't know.
Thanks.

Yasufumi
> 
> 
> 
> --
> David Marchand
> 

[dpdk-dev] [PATCH v5 0/1] fbarray: fix duplicated fbarray file in secondary

[yasufum.o]

From: Yasufumi Ogawa <yasufum.o@gmail.com>

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work if secondary is run as app container because each of
containerized secondary has PID 1. To reserve unique name, use hostname
instead of PID because hostname is assigned as a short form of 64
digits full container ID in docker.

---
v2:
  * fix typo in commit message
v3:
  * add fclose() after if getting hostname with fscan() is failed
v4:
  * Increase the size of proc_id to 33 and add boundary in calling
    fscan()
v5:
  * revise title to reflect the issue
  * use gethostname() instead of getting from `etc/hostname`
  * use HOST_NAME_MAX for size of string for hostname
---

Yasufumi Ogawa (1):
  fbarray: fix duplicated fbarray file in secondary

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

-- 
2.17.1

[dpdk-dev] [PATCH v5 1/1] fbarray: fix duplicated fbarray file in secondary

[yasufum.o]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work if several secondaries run as app containers because each
of containerized secondary has PID 1, and failed to reserve unique name
other than first one. To reserve unique name in each of containers, use
hostname instead of PID only if PID is 1.

Cc: stable@dpdk.org

Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
---
 lib/librte_eal/linux/eal/eal_memalloc.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
index af6d0d023..699079791 100644
--- a/lib/librte_eal/linux/eal/eal_memalloc.c
+++ b/lib/librte_eal/linux/eal/eal_memalloc.c
@@ -1365,6 +1365,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	struct rte_memseg_list *primary_msl, *local_msl;
 	char name[PATH_MAX];
 	int msl_idx, ret;
+	char proc_id[HOST_NAME_MAX] = { 0 };
 
 	if (msl->external)
 		return 0;
@@ -1374,8 +1375,18 @@ 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());
+	/* If run secondary in a container, the name of fbarray file should
+	 * not be decided with pid because getpid() always returns 1.
+	 * In docker, hostname is assigned as a short form of full container
+	 * ID. So use hostname as unique ID among containers instead.
+	 */
+	if (getpid() == 1)
+		gethostname(proc_id, HOST_NAME_MAX);
+	else
+		sprintf(proc_id, "%d", (int)getpid());
+
+	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
+			primary_msl->memseg_arr.name, proc_id);
 
 	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
 		primary_msl->memseg_arr.len,
-- 
2.17.1

Re: [dpdk-dev] [PATCH v5 1/1] fbarray: fix duplicated fbarray file in secondary

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of yasufum.o@gmail.com
> Sent: Monday, October 28, 2019 8:08 AM
> To: Burakov, Anatoly <anatoly.burakov@intel.com>; david.marchand@redhat.com
> Cc: dev@dpdk.org; stable@dpdk.org; yasufum.o@gmail.com; Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> Subject: [dpdk-dev] [PATCH v5 1/1] fbarray: fix duplicated fbarray file in secondary
> 
> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> 
> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> PID for reserving unique name among secondary processes. However, it
> does not work if several secondaries run as app containers because each
> of containerized secondary has PID 1, and failed to reserve unique name
> other than first one. To reserve unique name in each of containers, use
> hostname instead of PID only if PID is 1.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
> ---
>  lib/librte_eal/linux/eal/eal_memalloc.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
> index af6d0d023..699079791 100644
> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
> @@ -1365,6 +1365,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>  	struct rte_memseg_list *primary_msl, *local_msl;
>  	char name[PATH_MAX];
>  	int msl_idx, ret;
> +	char proc_id[HOST_NAME_MAX] = { 0 };
> 
>  	if (msl->external)
>  		return 0;
> @@ -1374,8 +1375,18 @@ 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());
> +	/* If run secondary in a container, the name of fbarray file should
> +	 * not be decided with pid because getpid() always returns 1.


I wonder why is that?
What will prevent user to do something like:
docker run -it --cpuset-cpus=7-8 -v /local/kananye1:/local/kananye1 ubuntu-dpdk-local:latest /bin/bash
And then start dpdk app manually within the container?
 
> +	 * In docker, hostname is assigned as a short form of full container
> +	 * ID. So use hostname as unique ID among containers instead.
> +	 */
> +	if (getpid() == 1)
> +		gethostname(proc_id, HOST_NAME_MAX);
> +	else
> +		sprintf(proc_id, "%d", (int)getpid());
> +
> +	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
> +			primary_msl->memseg_arr.name, proc_id);
> 
>  	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
>  		primary_msl->memseg_arr.len,
> --
> 2.17.1

Re: [dpdk-dev] [PATCH v5 1/1] fbarray: fix duplicated fbarray file in secondary

[Yasufumi Ogawa]

On 2019/10/29 21:03, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of yasufum.o@gmail.com
>> Sent: Monday, October 28, 2019 8:08 AM
>> To: Burakov, Anatoly <anatoly.burakov@intel.com>; david.marchand@redhat.com
>> Cc: dev@dpdk.org; stable@dpdk.org; yasufum.o@gmail.com; Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>> Subject: [dpdk-dev] [PATCH v5 1/1] fbarray: fix duplicated fbarray file in secondary
>>
>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>
>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
>> PID for reserving unique name among secondary processes. However, it
>> does not work if several secondaries run as app containers because each
>> of containerized secondary has PID 1, and failed to reserve unique name
>> other than first one. To reserve unique name in each of containers, use
>> hostname instead of PID only if PID is 1.
>>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
>> ---
>>   lib/librte_eal/linux/eal/eal_memalloc.c | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
>> index af6d0d023..699079791 100644
>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
>> @@ -1365,6 +1365,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>>   	struct rte_memseg_list *primary_msl, *local_msl;
>>   	char name[PATH_MAX];
>>   	int msl_idx, ret;
>> +	char proc_id[HOST_NAME_MAX] = { 0 };
>>
>>   	if (msl->external)
>>   		return 0;
>> @@ -1374,8 +1375,18 @@ 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());
>> +	/* If run secondary in a container, the name of fbarray file should
>> +	 * not be decided with pid because getpid() always returns 1.
> 
> 
> I wonder why is that?
> What will prevent user to do something like:
> docker run -it --cpuset-cpus=7-8 -v /local/kananye1:/local/kananye1 ubuntu-dpdk-local:latest /bin/bash
> And then start dpdk app manually within the container?
Hi Konstantin,

Thank you for your comment.

My concern is running secondary as app container. In current 
implementation, the name of fbarray file is decided by using PID and it 
must be overlapped with other process because assigning PID is started 
from 1 in each of app container. This patch is to fix the issue.

I think it is doable running app from bash for testing, but not 
acceptable for a realistic usecase in which user manages several app 
containers.

Regards,
Yasufumi

>   
>> +	 * In docker, hostname is assigned as a short form of full container
>> +	 * ID. So use hostname as unique ID among containers instead.
>> +	 */
>> +	if (getpid() == 1)
>> +		gethostname(proc_id, HOST_NAME_MAX);
>> +	else
>> +		sprintf(proc_id, "%d", (int)getpid());
>> +
>> +	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
>> +			primary_msl->memseg_arr.name, proc_id);
>>
>>   	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
>>   		primary_msl->memseg_arr.len,
>> --
>> 2.17.1
> 

Re: [dpdk-dev] [PATCH v5 1/1] fbarray: fix duplicated fbarray file in secondary

[Ananyev, Konstantin]

> -----Original Message-----
> From: Yasufumi Ogawa <yasufum.o@gmail.com>
> Sent: Wednesday, October 30, 2019 1:42 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Burakov, Anatoly <anatoly.burakov@intel.com>; david.marchand@redhat.com
> Cc: dev@dpdk.org; stable@dpdk.org; Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> Subject: Re: [dpdk-dev] [PATCH v5 1/1] fbarray: fix duplicated fbarray file in secondary
> 
> On 2019/10/29 21:03, Ananyev, Konstantin wrote:
> >
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of yasufum.o@gmail.com
> >> Sent: Monday, October 28, 2019 8:08 AM
> >> To: Burakov, Anatoly <anatoly.burakov@intel.com>; david.marchand@redhat.com
> >> Cc: dev@dpdk.org; stable@dpdk.org; yasufum.o@gmail.com; Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> >> Subject: [dpdk-dev] [PATCH v5 1/1] fbarray: fix duplicated fbarray file in secondary
> >>
> >> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> >>
> >> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> >> PID for reserving unique name among secondary processes. However, it
> >> does not work if several secondaries run as app containers because each
> >> of containerized secondary has PID 1, and failed to reserve unique name
> >> other than first one. To reserve unique name in each of containers, use
> >> hostname instead of PID only if PID is 1.
> >>
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
> >> ---
> >>   lib/librte_eal/linux/eal/eal_memalloc.c | 15 +++++++++++++--
> >>   1 file changed, 13 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
> >> index af6d0d023..699079791 100644
> >> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
> >> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
> >> @@ -1365,6 +1365,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
> >>   	struct rte_memseg_list *primary_msl, *local_msl;
> >>   	char name[PATH_MAX];
> >>   	int msl_idx, ret;
> >> +	char proc_id[HOST_NAME_MAX] = { 0 };
> >>
> >>   	if (msl->external)
> >>   		return 0;
> >> @@ -1374,8 +1375,18 @@ 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());
> >> +	/* If run secondary in a container, the name of fbarray file should
> >> +	 * not be decided with pid because getpid() always returns 1.
> >
> >
> > I wonder why is that?
> > What will prevent user to do something like:
> > docker run -it --cpuset-cpus=7-8 -v /local/kananye1:/local/kananye1 ubuntu-dpdk-local:latest /bin/bash
> > And then start dpdk app manually within the container?
> Hi Konstantin,
> 
> Thank you for your comment.
> 
> My concern is running secondary as app container. In current
> implementation, the name of fbarray file is decided by using PID and it
> must be overlapped with other process because assigning PID is started
> from 1 in each of app container. This patch is to fix the issue.
> 
> I think it is doable running app from bash for testing, but not
> acceptable for a realistic usecase in which user manages several app
> containers.

User can have a bash script to start inside container first, that
would do some preparation work (setup env variables, etc.)....
Or some different scenario when user needs/wants to
spawn several processes within container. 
Inside the lib you can't assume that your usage scenario is the
only possible one. 
I think solution needs to be generic enough to cover all such cases.
BTW, why we can't always use hostname in fbarray format?

> 
> Regards,
> Yasufumi
> 
> >
> >> +	 * In docker, hostname is assigned as a short form of full container
> >> +	 * ID. So use hostname as unique ID among containers instead.
> >> +	 */
> >> +	if (getpid() == 1)
> >> +		gethostname(proc_id, HOST_NAME_MAX);
> >> +	else
> >> +		sprintf(proc_id, "%d", (int)getpid());
> >> +
> >> +	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
> >> +			primary_msl->memseg_arr.name, proc_id);
> >>
> >>   	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
> >>   		primary_msl->memseg_arr.len,
> >> --
> >> 2.17.1
> >

Re: [dpdk-dev] [PATCH v5 1/1] fbarray: fix duplicated fbarray file in secondary

[Yasufumi Ogawa]

>> -----Original Message-----
>> From: Yasufumi Ogawa <yasufum.o@gmail.com>
>> Sent: Wednesday, October 30, 2019 1:42 PM
>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Burakov, Anatoly <anatoly.burakov@intel.com>; david.marchand@redhat.com
>> Cc: dev@dpdk.org; stable@dpdk.org; Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>> Subject: Re: [dpdk-dev] [PATCH v5 1/1] fbarray: fix duplicated fbarray file in secondary
>>
>> On 2019/10/29 21:03, Ananyev, Konstantin wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of yasufum.o@gmail.com
>>>> Sent: Monday, October 28, 2019 8:08 AM
>>>> To: Burakov, Anatoly <anatoly.burakov@intel.com>; david.marchand@redhat.com
>>>> Cc: dev@dpdk.org; stable@dpdk.org; yasufum.o@gmail.com; Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>> Subject: [dpdk-dev] [PATCH v5 1/1] fbarray: fix duplicated fbarray file in secondary
>>>>
>>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>
>>>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
>>>> PID for reserving unique name among secondary processes. However, it
>>>> does not work if several secondaries run as app containers because each
>>>> of containerized secondary has PID 1, and failed to reserve unique name
>>>> other than first one. To reserve unique name in each of containers, use
>>>> hostname instead of PID only if PID is 1.
>>>>
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
>>>> ---
>>>>    lib/librte_eal/linux/eal/eal_memalloc.c | 15 +++++++++++++--
>>>>    1 file changed, 13 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>> index af6d0d023..699079791 100644
>>>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
>>>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>> @@ -1365,6 +1365,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>>>>    	struct rte_memseg_list *primary_msl, *local_msl;
>>>>    	char name[PATH_MAX];
>>>>    	int msl_idx, ret;
>>>> +	char proc_id[HOST_NAME_MAX] = { 0 };
>>>>
>>>>    	if (msl->external)
>>>>    		return 0;
>>>> @@ -1374,8 +1375,18 @@ 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());
>>>> +	/* If run secondary in a container, the name of fbarray file should
>>>> +	 * not be decided with pid because getpid() always returns 1.
>>>
>>>
>>> I wonder why is that?
>>> What will prevent user to do something like:
>>> docker run -it --cpuset-cpus=7-8 -v /local/kananye1:/local/kananye1 ubuntu-dpdk-local:latest /bin/bash
>>> And then start dpdk app manually within the container?
>> Hi Konstantin,
>>
>> Thank you for your comment.
>>
>> My concern is running secondary as app container. In current
>> implementation, the name of fbarray file is decided by using PID and it
>> must be overlapped with other process because assigning PID is started
>> from 1 in each of app container. This patch is to fix the issue.
>>
>> I think it is doable running app from bash for testing, but not
>> acceptable for a realistic usecase in which user manages several app
>> containers.
> 
> User can have a bash script to start inside container first, that
> would do some preparation work (setup env variables, etc.)....
> Or some different scenario when user needs/wants to
> spawn several processes within container.
I don't know how to avoid to overlap PID from bash script because I 
think PIDs on host cannot see from inside of a container. So, I'm not 
sure it is possible.

> Inside the lib you can't assume that your usage scenario is the
> only possible one.
I don't want to modify it for a specific usecase, but just to avoid to 
overlap filenames. I think it is better this filename is decided by lib 
itself without any of user configuration because user don't need to care 
about the file which is used by DPDK internally.

> I think solution needs to be generic enough to cover all such cases.

> BTW, why we can't always use hostname in fbarray format?
Sorry, I don't know. But I wonder using PID is more assured to get 
unique name, but does not work only for secondary app container 
accidentally.

Yasufumi
> 
>>
>> Regards,
>> Yasufumi
>>
>>>
>>>> +	 * In docker, hostname is assigned as a short form of full container
>>>> +	 * ID. So use hostname as unique ID among containers instead.
>>>> +	 */
>>>> +	if (getpid() == 1)
>>>> +		gethostname(proc_id, HOST_NAME_MAX);
>>>> +	else
>>>> +		sprintf(proc_id, "%d", (int)getpid());
>>>> +
>>>> +	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
>>>> +			primary_msl->memseg_arr.name, proc_id);
>>>>
>>>>    	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
>>>>    		primary_msl->memseg_arr.len,
>>>> --
>>>> 2.17.1
>>>

Re: [dpdk-dev] [PATCH v5 1/1] fbarray: fix duplicated fbarray file in secondary

[Ananyev, Konstantin]

> >> -----Original Message-----
> >> From: Yasufumi Ogawa <yasufum.o@gmail.com>
> >> Sent: Wednesday, October 30, 2019 1:42 PM
> >> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Burakov, Anatoly <anatoly.burakov@intel.com>;
> david.marchand@redhat.com
> >> Cc: dev@dpdk.org; stable@dpdk.org; Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> >> Subject: Re: [dpdk-dev] [PATCH v5 1/1] fbarray: fix duplicated fbarray file in secondary
> >>
> >> On 2019/10/29 21:03, Ananyev, Konstantin wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: dev <dev-bounces@dpdk.org> On Behalf Of yasufum.o@gmail.com
> >>>> Sent: Monday, October 28, 2019 8:08 AM
> >>>> To: Burakov, Anatoly <anatoly.burakov@intel.com>; david.marchand@redhat.com
> >>>> Cc: dev@dpdk.org; stable@dpdk.org; yasufum.o@gmail.com; Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> >>>> Subject: [dpdk-dev] [PATCH v5 1/1] fbarray: fix duplicated fbarray file in secondary
> >>>>
> >>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> >>>>
> >>>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> >>>> PID for reserving unique name among secondary processes. However, it
> >>>> does not work if several secondaries run as app containers because each
> >>>> of containerized secondary has PID 1, and failed to reserve unique name
> >>>> other than first one. To reserve unique name in each of containers, use
> >>>> hostname instead of PID only if PID is 1.
> >>>>
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
> >>>> ---
> >>>>    lib/librte_eal/linux/eal/eal_memalloc.c | 15 +++++++++++++--
> >>>>    1 file changed, 13 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
> >>>> index af6d0d023..699079791 100644
> >>>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
> >>>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
> >>>> @@ -1365,6 +1365,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
> >>>>    	struct rte_memseg_list *primary_msl, *local_msl;
> >>>>    	char name[PATH_MAX];
> >>>>    	int msl_idx, ret;
> >>>> +	char proc_id[HOST_NAME_MAX] = { 0 };
> >>>>
> >>>>    	if (msl->external)
> >>>>    		return 0;
> >>>> @@ -1374,8 +1375,18 @@ 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());
> >>>> +	/* If run secondary in a container, the name of fbarray file should
> >>>> +	 * not be decided with pid because getpid() always returns 1.
> >>>
> >>>
> >>> I wonder why is that?
> >>> What will prevent user to do something like:
> >>> docker run -it --cpuset-cpus=7-8 -v /local/kananye1:/local/kananye1 ubuntu-dpdk-local:latest /bin/bash
> >>> And then start dpdk app manually within the container?
> >> Hi Konstantin,
> >>
> >> Thank you for your comment.
> >>
> >> My concern is running secondary as app container. In current
> >> implementation, the name of fbarray file is decided by using PID and it
> >> must be overlapped with other process because assigning PID is started
> >> from 1 in each of app container. This patch is to fix the issue.
> >>
> >> I think it is doable running app from bash for testing, but not
> >> acceptable for a realistic usecase in which user manages several app
> >> containers.
> >
> > User can have a bash script to start inside container first, that
> > would do some preparation work (setup env variables, etc.)....
> > Or some different scenario when user needs/wants to
> > spawn several processes within container.
> I don't know how to avoid to overlap PID from bash script because I
> think PIDs on host cannot see from inside of a container. So, I'm not
> sure it is possible.
> 
> > Inside the lib you can't assume that your usage scenario is the
> > only possible one.
> I don't want to modify it for a specific usecase, but just to avoid to
> overlap filenames. I think it is better this filename is decided by lib
> itself without any of user configuration because user don't need to care
> about the file which is used by DPDK internally.
> 
> > I think solution needs to be generic enough to cover all such cases.
> 
> > BTW, why we can't always use hostname in fbarray format?
> Sorry, I don't know. But I wonder using PID is more assured to get
> unique name, but does not work only for secondary app container
> accidentally.


My thought was - probably we always can use the same format:
snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s_%u",
	primary_msl->memseg_arr.name,  gethostname(), getpid());
to fix that problem?

Konstantin


> 
> Yasufumi
> >
> >>
> >> Regards,
> >> Yasufumi
> >>
> >>>
> >>>> +	 * In docker, hostname is assigned as a short form of full container
> >>>> +	 * ID. So use hostname as unique ID among containers instead.
> >>>> +	 */
> >>>> +	if (getpid() == 1)
> >>>> +		gethostname(proc_id, HOST_NAME_MAX);
> >>>> +	else
> >>>> +		sprintf(proc_id, "%d", (int)getpid());
> >>>> +
> >>>> +	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s",
> >>>> +			primary_msl->memseg_arr.name, proc_id);
> >>>>
> >>>>    	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
> >>>>    		primary_msl->memseg_arr.len,
> >>>> --
> >>>> 2.17.1
> >>>

[dpdk-dev] [PATCH v6 0/1] fbarray: fix duplicated fbarray file in secondary

[yasufum.o]

From: Yasufumi Ogawa <yasufum.o@gmail.com>

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work if several secondaries run as app containers because each
of containerized secondary has PID 1, and failed to reserve unique name
other than first one. To reserve unique name in each of containers, use
hostname in addition to PID.

---
v2:
  * fix typo in commit message
v3:
  * add fclose() after if getting hostname with fscan() is failed
v4:
  * Increase the size of proc_id to 33 and add boundary in calling
    fscan()
v5:
  * revise title to reflect the issue
  * use gethostname() instead of getting from `etc/hostname`
  * use HOST_NAME_MAX for size of string for hostname
v6:
  * change to use hostname and pid to cover both of host and container
    cases
  * change RTE_FBARRAY_NAME_LEN to NAME_MAX to reserve enough size for
    filename
---

Yasufumi Ogawa (1):
  fbarray: fix duplicated fbarray file in secondary

 lib/librte_eal/common/include/rte_fbarray.h |  2 +-
 lib/librte_eal/linux/eal/eal_memalloc.c     | 11 ++++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v6 1/1] fbarray: fix duplicated fbarray file in secondary

[yasufum.o]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work if several secondaries run as app containers because each
of containerized secondary has PID 1, and failed to reserve unique name
other than first one. To reserve unique name in each of containers, use
hostname in addition to PID.

Cc: stable@dpdk.org

Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
---
 lib/librte_eal/common/include/rte_fbarray.h |  2 +-
 lib/librte_eal/linux/eal/eal_memalloc.c     | 11 ++++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_fbarray.h b/lib/librte_eal/common/include/rte_fbarray.h
index 6dccdbec9..5c2815093 100644
--- a/lib/librte_eal/common/include/rte_fbarray.h
+++ b/lib/librte_eal/common/include/rte_fbarray.h
@@ -39,7 +39,7 @@ extern "C" {
 #include <rte_compat.h>
 #include <rte_rwlock.h>
 
-#define RTE_FBARRAY_NAME_LEN 64
+#define RTE_FBARRAY_NAME_LEN NAME_MAX
 
 struct rte_fbarray {
 	char name[RTE_FBARRAY_NAME_LEN]; /**< name associated with an array */
diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
index af6d0d023..24f0275c9 100644
--- a/lib/librte_eal/linux/eal/eal_memalloc.c
+++ b/lib/librte_eal/linux/eal/eal_memalloc.c
@@ -1365,6 +1365,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	struct rte_memseg_list *primary_msl, *local_msl;
 	char name[PATH_MAX];
 	int msl_idx, ret;
+	char hostname[HOST_NAME_MAX] = { 0 };
 
 	if (msl->external)
 		return 0;
@@ -1373,9 +1374,13 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	primary_msl = &mcfg->memsegs[msl_idx];
 	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());
+	/* Create distinct fbarrays for each secondary by using PID and
+	 * hostname. The reason why using hostname is because PID could be
+	 * duplicated among secondaries if it is launched in a container.
+	 */
+	gethostname(hostname, HOST_NAME_MAX);
+	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s_%d",
+			primary_msl->memseg_arr.name, hostname, (int)getpid());
 
 	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
 		primary_msl->memseg_arr.len,
-- 
2.17.1

Re: [dpdk-dev] [PATCH v6 1/1] fbarray: fix duplicated fbarray file in secondary

[Ananyev, Konstantin]

> 
> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> 
> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> PID for reserving unique name among secondary processes. However, it
> does not work if several secondaries run as app containers because each
> of containerized secondary has PID 1, and failed to reserve unique name
> other than first one. To reserve unique name in each of containers, use
> hostname in addition to PID.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
> ---
>  lib/librte_eal/common/include/rte_fbarray.h |  2 +-
>  lib/librte_eal/linux/eal/eal_memalloc.c     | 11 ++++++++---
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_fbarray.h b/lib/librte_eal/common/include/rte_fbarray.h
> index 6dccdbec9..5c2815093 100644
> --- a/lib/librte_eal/common/include/rte_fbarray.h
> +++ b/lib/librte_eal/common/include/rte_fbarray.h
> @@ -39,7 +39,7 @@ extern "C" {
>  #include <rte_compat.h>
>  #include <rte_rwlock.h>
> 
> -#define RTE_FBARRAY_NAME_LEN 64
> +#define RTE_FBARRAY_NAME_LEN NAME_MAX
> 
>  struct rte_fbarray {
>  	char name[RTE_FBARRAY_NAME_LEN]; /**< name associated with an array */
> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
> index af6d0d023..24f0275c9 100644
> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
> @@ -1365,6 +1365,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>  	struct rte_memseg_list *primary_msl, *local_msl;
>  	char name[PATH_MAX];
>  	int msl_idx, ret;
> +	char hostname[HOST_NAME_MAX] = { 0 };
> 
>  	if (msl->external)
>  		return 0;
> @@ -1373,9 +1374,13 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>  	primary_msl = &mcfg->memsegs[msl_idx];
>  	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());
> +	/* Create distinct fbarrays for each secondary by using PID and
> +	 * hostname. The reason why using hostname is because PID could be
> +	 * duplicated among secondaries if it is launched in a container.
> +	 */
> +	gethostname(hostname, HOST_NAME_MAX);
> +	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s_%d",
> +			primary_msl->memseg_arr.name, hostname, (int)getpid());
> 
>  	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
>  		primary_msl->memseg_arr.len,
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.17.1

Re: [dpdk-dev] [PATCH v6 1/1] fbarray: fix duplicated fbarray file in secondary

[Burakov, Anatoly]

On 01-Nov-19 9:04 AM, yasufum.o@gmail.com wrote:
> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> 
> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> PID for reserving unique name among secondary processes. However, it
> does not work if several secondaries run as app containers because each
> of containerized secondary has PID 1, and failed to reserve unique name
> other than first one. To reserve unique name in each of containers, use
> hostname in addition to PID.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
> ---
>   lib/librte_eal/common/include/rte_fbarray.h |  2 +-
>   lib/librte_eal/linux/eal/eal_memalloc.c     | 11 ++++++++---
>   2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_fbarray.h b/lib/librte_eal/common/include/rte_fbarray.h
> index 6dccdbec9..5c2815093 100644
> --- a/lib/librte_eal/common/include/rte_fbarray.h
> +++ b/lib/librte_eal/common/include/rte_fbarray.h
> @@ -39,7 +39,7 @@ extern "C" {
>   #include <rte_compat.h>
>   #include <rte_rwlock.h>
>   
> -#define RTE_FBARRAY_NAME_LEN 64
> +#define RTE_FBARRAY_NAME_LEN NAME_MAX
>   
>   struct rte_fbarray {
>   	char name[RTE_FBARRAY_NAME_LEN]; /**< name associated with an array */
> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
> index af6d0d023..24f0275c9 100644
> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
> @@ -1365,6 +1365,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>   	struct rte_memseg_list *primary_msl, *local_msl;
>   	char name[PATH_MAX];
>   	int msl_idx, ret;
> +	char hostname[HOST_NAME_MAX] = { 0 };
>   
>   	if (msl->external)
>   		return 0;
> @@ -1373,9 +1374,13 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>   	primary_msl = &mcfg->memsegs[msl_idx];
>   	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());
> +	/* Create distinct fbarrays for each secondary by using PID and
> +	 * hostname. The reason why using hostname is because PID could be
> +	 * duplicated among secondaries if it is launched in a container.
> +	 */
> +	gethostname(hostname, HOST_NAME_MAX);
> +	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s_%d",
> +			primary_msl->memseg_arr.name, hostname, (int)getpid());
>   
>   	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
>   		primary_msl->memseg_arr.len,
> 

I think the order should be reversed. Both containers and non-containers 
can have their hostname set, and RTE_FBARRAY_NAME_LEN is of fairly 
limited length, so if the hostname is long enough, the PID never gets 
into the name string, resulting in duplicates. It is better have pid first.

Once that's fixed,

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v6 1/1] fbarray: fix duplicated fbarray file in secondary

[David Marchand]

Hello Anatoly, Yasufumi,

On Mon, Nov 4, 2019 at 11:20 AM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 01-Nov-19 9:04 AM, yasufum.o@gmail.com wrote:
> > From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> >
> > In secondary_msl_create_walk(), it creates a file for fbarrays with its
> > PID for reserving unique name among secondary processes. However, it
> > does not work if several secondaries run as app containers because each
> > of containerized secondary has PID 1, and failed to reserve unique name
> > other than first one. To reserve unique name in each of containers, use
> > hostname in addition to PID.
> >
> > Cc: stable@dpdk.org

We can't backport this as is, see below.


> >
> > Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
> > ---
> >   lib/librte_eal/common/include/rte_fbarray.h |  2 +-
> >   lib/librte_eal/linux/eal/eal_memalloc.c     | 11 ++++++++---
> >   2 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/include/rte_fbarray.h b/lib/librte_eal/common/include/rte_fbarray.h
> > index 6dccdbec9..5c2815093 100644
> > --- a/lib/librte_eal/common/include/rte_fbarray.h
> > +++ b/lib/librte_eal/common/include/rte_fbarray.h
> > @@ -39,7 +39,7 @@ extern "C" {
> >   #include <rte_compat.h>
> >   #include <rte_rwlock.h>
> >
> > -#define RTE_FBARRAY_NAME_LEN 64
> > +#define RTE_FBARRAY_NAME_LEN NAME_MAX

The change on RTE_FBARRAY_NAME_LEN breaks the ABI, so we cannot
backport this as is.
For 19.11, we can allow this breakage, but we need an update of the
release notes.

Besides, what is the impact in terms of memory consumption?


> >
> >   struct rte_fbarray {
> >       char name[RTE_FBARRAY_NAME_LEN]; /**< name associated with an array */
> > diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
> > index af6d0d023..24f0275c9 100644
> > --- a/lib/librte_eal/linux/eal/eal_memalloc.c
> > +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
> > @@ -1365,6 +1365,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
> >       struct rte_memseg_list *primary_msl, *local_msl;
> >       char name[PATH_MAX];
> >       int msl_idx, ret;
> > +     char hostname[HOST_NAME_MAX] = { 0 };
> >
> >       if (msl->external)
> >               return 0;
> > @@ -1373,9 +1374,13 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
> >       primary_msl = &mcfg->memsegs[msl_idx];
> >       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());
> > +     /* Create distinct fbarrays for each secondary by using PID and
> > +      * hostname. The reason why using hostname is because PID could be
> > +      * duplicated among secondaries if it is launched in a container.
> > +      */
> > +     gethostname(hostname, HOST_NAME_MAX);

Personal preference, s/HOST_NAME_MAX/sizeof(hostname)/.


hostname[] is HOST_NAME_MAX bytes long.
In the worst case, we can get a non NULL terminated hostname string.
"
       gethostname() returns the null-terminated hostname in the
character array name, which has a length of len bytes.  If the
null-terminated hostname is too large to fit, then the name is
truncated, and
       no error is returned (but see NOTES below).  POSIX.1-2001 says
that if such truncation occurs, then it is unspecified whether the
returned buffer includes a terminating null byte.
...
NOTES
       SUSv2 guarantees that "Host names are limited to 255 bytes".
POSIX.1-2001 guarantees that "Host names (not including the
terminating null byte) are  limited  to  HOST_NAME_MAX  bytes".   On
Linux,
       HOST_NAME_MAX is defined with the value 64, which has been the
limit since Linux 1.0 (earlier kernels imposed a limit of 8 bytes).
"

How about making hostname[] HOST_NAME_MAX+1 bytes long?

> > +     snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s_%d",
> > +                     primary_msl->memseg_arr.name, hostname, (int)getpid());
> >
> >       ret = rte_fbarray_init(&local_msl->memseg_arr, name,
> >               primary_msl->memseg_arr.len,
> >
>
> I think the order should be reversed. Both containers and non-containers
> can have their hostname set, and RTE_FBARRAY_NAME_LEN is of fairly
> limited length, so if the hostname is long enough, the PID never gets
> into the name string, resulting in duplicates. It is better have pid first.

Anatoly,

On the principle, it seems better, yes.
Just the comment on RTE_FBARRAY_NAME_LEN indicates that you missed the
change at the top of the patch.
What do you think of this change?


-- 
David Marchand

Re: [dpdk-dev] [PATCH v6 1/1] fbarray: fix duplicated fbarray file in secondary

[Burakov, Anatoly]

On 05-Nov-19 10:13 AM, David Marchand wrote:
> Hello Anatoly, Yasufumi,
> 
> On Mon, Nov 4, 2019 at 11:20 AM Burakov, Anatoly
> <anatoly.burakov@intel.com> wrote:
>>
>> On 01-Nov-19 9:04 AM, yasufum.o@gmail.com wrote:
>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>
>>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
>>> PID for reserving unique name among secondary processes. However, it
>>> does not work if several secondaries run as app containers because each
>>> of containerized secondary has PID 1, and failed to reserve unique name
>>> other than first one. To reserve unique name in each of containers, use
>>> hostname in addition to PID.
>>>
>>> Cc: stable@dpdk.org
> 
> We can't backport this as is, see below.
> 
> 
>>>
>>> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
>>> ---
>>>    lib/librte_eal/common/include/rte_fbarray.h |  2 +-
>>>    lib/librte_eal/linux/eal/eal_memalloc.c     | 11 ++++++++---
>>>    2 files changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/librte_eal/common/include/rte_fbarray.h b/lib/librte_eal/common/include/rte_fbarray.h
>>> index 6dccdbec9..5c2815093 100644
>>> --- a/lib/librte_eal/common/include/rte_fbarray.h
>>> +++ b/lib/librte_eal/common/include/rte_fbarray.h
>>> @@ -39,7 +39,7 @@ extern "C" {
>>>    #include <rte_compat.h>
>>>    #include <rte_rwlock.h>
>>>
>>> -#define RTE_FBARRAY_NAME_LEN 64
>>> +#define RTE_FBARRAY_NAME_LEN NAME_MAX
> 
> The change on RTE_FBARRAY_NAME_LEN breaks the ABI, so we cannot
> backport this as is.
> For 19.11, we can allow this breakage, but we need an update of the
> release notes.
> 
> Besides, what is the impact in terms of memory consumption?
> 
> 
>>>
>>>    struct rte_fbarray {
>>>        char name[RTE_FBARRAY_NAME_LEN]; /**< name associated with an array */
>>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
>>> index af6d0d023..24f0275c9 100644
>>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
>>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
>>> @@ -1365,6 +1365,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>>>        struct rte_memseg_list *primary_msl, *local_msl;
>>>        char name[PATH_MAX];
>>>        int msl_idx, ret;
>>> +     char hostname[HOST_NAME_MAX] = { 0 };
>>>
>>>        if (msl->external)
>>>                return 0;
>>> @@ -1373,9 +1374,13 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>>>        primary_msl = &mcfg->memsegs[msl_idx];
>>>        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());
>>> +     /* Create distinct fbarrays for each secondary by using PID and
>>> +      * hostname. The reason why using hostname is because PID could be
>>> +      * duplicated among secondaries if it is launched in a container.
>>> +      */
>>> +     gethostname(hostname, HOST_NAME_MAX);
> 
> Personal preference, s/HOST_NAME_MAX/sizeof(hostname)/.
> 
> 
> hostname[] is HOST_NAME_MAX bytes long.
> In the worst case, we can get a non NULL terminated hostname string.
> "
>         gethostname() returns the null-terminated hostname in the
> character array name, which has a length of len bytes.  If the
> null-terminated hostname is too large to fit, then the name is
> truncated, and
>         no error is returned (but see NOTES below).  POSIX.1-2001 says
> that if such truncation occurs, then it is unspecified whether the
> returned buffer includes a terminating null byte.
> ...
> NOTES
>         SUSv2 guarantees that "Host names are limited to 255 bytes".
> POSIX.1-2001 guarantees that "Host names (not including the
> terminating null byte) are  limited  to  HOST_NAME_MAX  bytes".   On
> Linux,
>         HOST_NAME_MAX is defined with the value 64, which has been the
> limit since Linux 1.0 (earlier kernels imposed a limit of 8 bytes).
> "
> 
> How about making hostname[] HOST_NAME_MAX+1 bytes long?
> 
>>> +     snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s_%d",
>>> +                     primary_msl->memseg_arr.name, hostname, (int)getpid());
>>>
>>>        ret = rte_fbarray_init(&local_msl->memseg_arr, name,
>>>                primary_msl->memseg_arr.len,
>>>
>>
>> I think the order should be reversed. Both containers and non-containers
>> can have their hostname set, and RTE_FBARRAY_NAME_LEN is of fairly
>> limited length, so if the hostname is long enough, the PID never gets
>> into the name string, resulting in duplicates. It is better have pid first.
> 
> Anatoly,
> 
> On the principle, it seems better, yes.
> Just the comment on RTE_FBARRAY_NAME_LEN indicates that you missed the
> change at the top of the patch.
> What do you think of this change?
> 

Yes, i did miss that, apologies.

I don't have a strong opinion on this change, however the above comment 
would still be true if we make fbarray size to be hostname_max + 1 - we 
still potentially get no space for a pid. So if we're going to have pid 
in there as well, it should be hostname_max + pid_max (5 digits?) + 
whatever underscores we have + null terminator, to ensure it fits under 
any and all circumstances.

Wrt memory usage, honestly, we don't live in a "640K should be enough 
for everyone" era any more. I don't see this being a major issue. This 
is not a hotpath, and we reserve half a terabyte of virtual memory at 
startup as it is. A few kilo/megabytes more isn't going to make much of 
a difference here.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v6 1/1] fbarray: fix duplicated fbarray file in secondary

[Ananyev, Konstantin]

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Tuesday, November 5, 2019 11:31 AM
> To: David Marchand <david.marchand@redhat.com>; Yasufumi Ogawa <yasufum.o@gmail.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev <dev@dpdk.org>; dpdk stable <stable@dpdk.org>; Yasufumi Ogawa
> <ogawa.yasufumi@lab.ntt.co.jp>
> Subject: Re: [PATCH v6 1/1] fbarray: fix duplicated fbarray file in secondary
> 
> On 05-Nov-19 10:13 AM, David Marchand wrote:
> > Hello Anatoly, Yasufumi,
> >
> > On Mon, Nov 4, 2019 at 11:20 AM Burakov, Anatoly
> > <anatoly.burakov@intel.com> wrote:
> >>
> >> On 01-Nov-19 9:04 AM, yasufum.o@gmail.com wrote:
> >>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> >>>
> >>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> >>> PID for reserving unique name among secondary processes. However, it
> >>> does not work if several secondaries run as app containers because each
> >>> of containerized secondary has PID 1, and failed to reserve unique name
> >>> other than first one. To reserve unique name in each of containers, use
> >>> hostname in addition to PID.
> >>>
> >>> Cc: stable@dpdk.org
> >
> > We can't backport this as is, see below.
> >
> >
> >>>
> >>> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
> >>> ---
> >>>    lib/librte_eal/common/include/rte_fbarray.h |  2 +-
> >>>    lib/librte_eal/linux/eal/eal_memalloc.c     | 11 ++++++++---
> >>>    2 files changed, 9 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/lib/librte_eal/common/include/rte_fbarray.h b/lib/librte_eal/common/include/rte_fbarray.h
> >>> index 6dccdbec9..5c2815093 100644
> >>> --- a/lib/librte_eal/common/include/rte_fbarray.h
> >>> +++ b/lib/librte_eal/common/include/rte_fbarray.h
> >>> @@ -39,7 +39,7 @@ extern "C" {
> >>>    #include <rte_compat.h>
> >>>    #include <rte_rwlock.h>
> >>>
> >>> -#define RTE_FBARRAY_NAME_LEN 64
> >>> +#define RTE_FBARRAY_NAME_LEN NAME_MAX
> >
> > The change on RTE_FBARRAY_NAME_LEN breaks the ABI, so we cannot
> > backport this as is.
> > For 19.11, we can allow this breakage, but we need an update of the
> > release notes.
> >
> > Besides, what is the impact in terms of memory consumption?
> >
> >
> >>>
> >>>    struct rte_fbarray {
> >>>        char name[RTE_FBARRAY_NAME_LEN]; /**< name associated with an array */
> >>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
> >>> index af6d0d023..24f0275c9 100644
> >>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
> >>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
> >>> @@ -1365,6 +1365,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
> >>>        struct rte_memseg_list *primary_msl, *local_msl;
> >>>        char name[PATH_MAX];
> >>>        int msl_idx, ret;
> >>> +     char hostname[HOST_NAME_MAX] = { 0 };
> >>>
> >>>        if (msl->external)
> >>>                return 0;
> >>> @@ -1373,9 +1374,13 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
> >>>        primary_msl = &mcfg->memsegs[msl_idx];
> >>>        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());
> >>> +     /* Create distinct fbarrays for each secondary by using PID and
> >>> +      * hostname. The reason why using hostname is because PID could be
> >>> +      * duplicated among secondaries if it is launched in a container.
> >>> +      */
> >>> +     gethostname(hostname, HOST_NAME_MAX);
> >
> > Personal preference, s/HOST_NAME_MAX/sizeof(hostname)/.
> >
> >
> > hostname[] is HOST_NAME_MAX bytes long.
> > In the worst case, we can get a non NULL terminated hostname string.
> > "
> >         gethostname() returns the null-terminated hostname in the
> > character array name, which has a length of len bytes.  If the
> > null-terminated hostname is too large to fit, then the name is
> > truncated, and
> >         no error is returned (but see NOTES below).  POSIX.1-2001 says
> > that if such truncation occurs, then it is unspecified whether the
> > returned buffer includes a terminating null byte.
> > ...
> > NOTES
> >         SUSv2 guarantees that "Host names are limited to 255 bytes".
> > POSIX.1-2001 guarantees that "Host names (not including the
> > terminating null byte) are  limited  to  HOST_NAME_MAX  bytes".   On
> > Linux,
> >         HOST_NAME_MAX is defined with the value 64, which has been the
> > limit since Linux 1.0 (earlier kernels imposed a limit of 8 bytes).
> > "
> >
> > How about making hostname[] HOST_NAME_MAX+1 bytes long?
> >
> >>> +     snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s_%d",
> >>> +                     primary_msl->memseg_arr.name, hostname, (int)getpid());
> >>>
> >>>        ret = rte_fbarray_init(&local_msl->memseg_arr, name,
> >>>                primary_msl->memseg_arr.len,
> >>>
> >>
> >> I think the order should be reversed. Both containers and non-containers
> >> can have their hostname set, and RTE_FBARRAY_NAME_LEN is of fairly
> >> limited length, so if the hostname is long enough, the PID never gets
> >> into the name string, resulting in duplicates. It is better have pid first.
> >
> > Anatoly,
> >
> > On the principle, it seems better, yes.
> > Just the comment on RTE_FBARRAY_NAME_LEN indicates that you missed the
> > change at the top of the patch.
> > What do you think of this change?
> >
> 
> Yes, i did miss that, apologies.
> 
> I don't have a strong opinion on this change, however the above comment
> would still be true if we make fbarray size to be hostname_max + 1 - we
> still potentially get no space for a pid. So if we're going to have pid
> in there as well, it should be hostname_max + pid_max (5 digits?) +
> whatever underscores we have + null terminator, to ensure it fits under
> any and all circumstances.#

I think that at least on linux we have more than enough space here:

$ find /usr/include -type f | xargs grep ' NAME_MAX' | grep define
/usr/include/linux/limits.h:#define NAME_MAX         255        /* # chars in a file name */

$ find /usr/include -type f | xargs grep ' HOST_NAME_MAX' | grep define
/usr/include/i386-linux-gnu/bits/local_lim.h:#define HOST_NAME_MAX             64
/usr/include/x86_64-linux-gnu/bits/local_lim.h:#define HOST_NAME_MAX           64

> 
> Wrt memory usage, honestly, we don't live in a "640K should be enough
> for everyone" era any more. I don't see this being a major issue. This
> is not a hotpath, and we reserve half a terabyte of virtual memory at
> startup as it is. A few kilo/megabytes more isn't going to make much of
> a difference here.
> 
> --
> Thanks,
> Anatoly

Re: [dpdk-dev] [PATCH v6 1/1] fbarray: fix duplicated fbarray file in secondary

[Burakov, Anatoly]

On 05-Nov-19 11:41 AM, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Sent: Tuesday, November 5, 2019 11:31 AM
>> To: David Marchand <david.marchand@redhat.com>; Yasufumi Ogawa <yasufum.o@gmail.com>
>> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev <dev@dpdk.org>; dpdk stable <stable@dpdk.org>; Yasufumi Ogawa
>> <ogawa.yasufumi@lab.ntt.co.jp>
>> Subject: Re: [PATCH v6 1/1] fbarray: fix duplicated fbarray file in secondary
>>
>> On 05-Nov-19 10:13 AM, David Marchand wrote:
>>> Hello Anatoly, Yasufumi,
>>>
>>> On Mon, Nov 4, 2019 at 11:20 AM Burakov, Anatoly
>>> <anatoly.burakov@intel.com> wrote:
>>>>
>>>> On 01-Nov-19 9:04 AM, yasufum.o@gmail.com wrote:
>>>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>>
>>>>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
>>>>> PID for reserving unique name among secondary processes. However, it
>>>>> does not work if several secondaries run as app containers because each
>>>>> of containerized secondary has PID 1, and failed to reserve unique name
>>>>> other than first one. To reserve unique name in each of containers, use
>>>>> hostname in addition to PID.
>>>>>
>>>>> Cc: stable@dpdk.org
>>>
>>> We can't backport this as is, see below.
>>>
>>>
>>>>>
>>>>> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
>>>>> ---
>>>>>     lib/librte_eal/common/include/rte_fbarray.h |  2 +-
>>>>>     lib/librte_eal/linux/eal/eal_memalloc.c     | 11 ++++++++---
>>>>>     2 files changed, 9 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/lib/librte_eal/common/include/rte_fbarray.h b/lib/librte_eal/common/include/rte_fbarray.h
>>>>> index 6dccdbec9..5c2815093 100644
>>>>> --- a/lib/librte_eal/common/include/rte_fbarray.h
>>>>> +++ b/lib/librte_eal/common/include/rte_fbarray.h
>>>>> @@ -39,7 +39,7 @@ extern "C" {
>>>>>     #include <rte_compat.h>
>>>>>     #include <rte_rwlock.h>
>>>>>
>>>>> -#define RTE_FBARRAY_NAME_LEN 64
>>>>> +#define RTE_FBARRAY_NAME_LEN NAME_MAX
>>>
>>> The change on RTE_FBARRAY_NAME_LEN breaks the ABI, so we cannot
>>> backport this as is.
>>> For 19.11, we can allow this breakage, but we need an update of the
>>> release notes.
>>>
>>> Besides, what is the impact in terms of memory consumption?
>>>
>>>
>>>>>
>>>>>     struct rte_fbarray {
>>>>>         char name[RTE_FBARRAY_NAME_LEN]; /**< name associated with an array */
>>>>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>> index af6d0d023..24f0275c9 100644
>>>>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>> @@ -1365,6 +1365,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>>>>>         struct rte_memseg_list *primary_msl, *local_msl;
>>>>>         char name[PATH_MAX];
>>>>>         int msl_idx, ret;
>>>>> +     char hostname[HOST_NAME_MAX] = { 0 };
>>>>>
>>>>>         if (msl->external)
>>>>>                 return 0;
>>>>> @@ -1373,9 +1374,13 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>>>>>         primary_msl = &mcfg->memsegs[msl_idx];
>>>>>         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());
>>>>> +     /* Create distinct fbarrays for each secondary by using PID and
>>>>> +      * hostname. The reason why using hostname is because PID could be
>>>>> +      * duplicated among secondaries if it is launched in a container.
>>>>> +      */
>>>>> +     gethostname(hostname, HOST_NAME_MAX);
>>>
>>> Personal preference, s/HOST_NAME_MAX/sizeof(hostname)/.
>>>
>>>
>>> hostname[] is HOST_NAME_MAX bytes long.
>>> In the worst case, we can get a non NULL terminated hostname string.
>>> "
>>>          gethostname() returns the null-terminated hostname in the
>>> character array name, which has a length of len bytes.  If the
>>> null-terminated hostname is too large to fit, then the name is
>>> truncated, and
>>>          no error is returned (but see NOTES below).  POSIX.1-2001 says
>>> that if such truncation occurs, then it is unspecified whether the
>>> returned buffer includes a terminating null byte.
>>> ...
>>> NOTES
>>>          SUSv2 guarantees that "Host names are limited to 255 bytes".
>>> POSIX.1-2001 guarantees that "Host names (not including the
>>> terminating null byte) are  limited  to  HOST_NAME_MAX  bytes".   On
>>> Linux,
>>>          HOST_NAME_MAX is defined with the value 64, which has been the
>>> limit since Linux 1.0 (earlier kernels imposed a limit of 8 bytes).
>>> "
>>>
>>> How about making hostname[] HOST_NAME_MAX+1 bytes long?
>>>
>>>>> +     snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s_%d",
>>>>> +                     primary_msl->memseg_arr.name, hostname, (int)getpid());
>>>>>
>>>>>         ret = rte_fbarray_init(&local_msl->memseg_arr, name,
>>>>>                 primary_msl->memseg_arr.len,
>>>>>
>>>>
>>>> I think the order should be reversed. Both containers and non-containers
>>>> can have their hostname set, and RTE_FBARRAY_NAME_LEN is of fairly
>>>> limited length, so if the hostname is long enough, the PID never gets
>>>> into the name string, resulting in duplicates. It is better have pid first.
>>>
>>> Anatoly,
>>>
>>> On the principle, it seems better, yes.
>>> Just the comment on RTE_FBARRAY_NAME_LEN indicates that you missed the
>>> change at the top of the patch.
>>> What do you think of this change?
>>>
>>
>> Yes, i did miss that, apologies.
>>
>> I don't have a strong opinion on this change, however the above comment
>> would still be true if we make fbarray size to be hostname_max + 1 - we
>> still potentially get no space for a pid. So if we're going to have pid
>> in there as well, it should be hostname_max + pid_max (5 digits?) +
>> whatever underscores we have + null terminator, to ensure it fits under
>> any and all circumstances.#
> 
> I think that at least on linux we have more than enough space here:
> 
> $ find /usr/include -type f | xargs grep ' NAME_MAX' | grep define
> /usr/include/linux/limits.h:#define NAME_MAX         255        /* # chars in a file name */
> 
> $ find /usr/include -type f | xargs grep ' HOST_NAME_MAX' | grep define
> /usr/include/i386-linux-gnu/bits/local_lim.h:#define HOST_NAME_MAX             64
> /usr/include/x86_64-linux-gnu/bits/local_lim.h:#define HOST_NAME_MAX           64
> 

Okay, works for me :)

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v6 1/1] fbarray: fix duplicated fbarray file in secondary

[Yasufumi Ogawa]

>>> -----Original Message-----
>>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>>> Sent: Tuesday, November 5, 2019 11:31 AM
>>> To: David Marchand <david.marchand@redhat.com>; Yasufumi Ogawa 
>>> <yasufum.o@gmail.com>
>>> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev 
>>> <dev@dpdk.org>; dpdk stable <stable@dpdk.org>; Yasufumi Ogawa
>>> <ogawa.yasufumi@lab.ntt.co.jp>
>>> Subject: Re: [PATCH v6 1/1] fbarray: fix duplicated fbarray file in 
>>> secondary
>>>
>>> On 05-Nov-19 10:13 AM, David Marchand wrote:
>>>> Hello Anatoly, Yasufumi,
>>>>
>>>> On Mon, Nov 4, 2019 at 11:20 AM Burakov, Anatoly
>>>> <anatoly.burakov@intel.com> wrote:
>>>>>
>>>>> On 01-Nov-19 9:04 AM, yasufum.o@gmail.com wrote:
>>>>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>>>
>>>>>> In secondary_msl_create_walk(), it creates a file for fbarrays 
>>>>>> with its
>>>>>> PID for reserving unique name among secondary processes. However, it
>>>>>> does not work if several secondaries run as app containers because 
>>>>>> each
>>>>>> of containerized secondary has PID 1, and failed to reserve unique 
>>>>>> name
>>>>>> other than first one. To reserve unique name in each of 
>>>>>> containers, use
>>>>>> hostname in addition to PID.
>>>>>>
>>>>>> Cc: stable@dpdk.org
>>>>
>>>> We can't backport this as is, see below.
>>>>
>>>>
>>>>>>
>>>>>> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
>>>>>> ---
>>>>>>     lib/librte_eal/common/include/rte_fbarray.h |  2 +-
>>>>>>     lib/librte_eal/linux/eal/eal_memalloc.c     | 11 ++++++++---
>>>>>>     2 files changed, 9 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/librte_eal/common/include/rte_fbarray.h 
>>>>>> b/lib/librte_eal/common/include/rte_fbarray.h
>>>>>> index 6dccdbec9..5c2815093 100644
>>>>>> --- a/lib/librte_eal/common/include/rte_fbarray.h
>>>>>> +++ b/lib/librte_eal/common/include/rte_fbarray.h
>>>>>> @@ -39,7 +39,7 @@ extern "C" {
>>>>>>     #include <rte_compat.h>
>>>>>>     #include <rte_rwlock.h>
>>>>>>
>>>>>> -#define RTE_FBARRAY_NAME_LEN 64
>>>>>> +#define RTE_FBARRAY_NAME_LEN NAME_MAX
>>>>
>>>> The change on RTE_FBARRAY_NAME_LEN breaks the ABI, so we cannot
>>>> backport this as is.
>>>> For 19.11, we can allow this breakage, but we need an update of the
>>>> release notes.
OK. I wasn't careful for the ABI change. I think RTE_FBARRAY_NAME_LEN 64 
is enough without using hostname as a part of fbarray. So I would like 
to discard this change.

>>>>
>>>> Besides, what is the impact in terms of memory consumption?
>>>>
>>>>
>>>>>>
>>>>>>     struct rte_fbarray {
>>>>>>         char name[RTE_FBARRAY_NAME_LEN]; /**< name associated with 
>>>>>> an array */
>>>>>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c 
>>>>>> b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>>> index af6d0d023..24f0275c9 100644
>>>>>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>>> @@ -1365,6 +1365,7 @@ secondary_msl_create_walk(const struct 
>>>>>> rte_memseg_list *msl,
>>>>>>         struct rte_memseg_list *primary_msl, *local_msl;
>>>>>>         char name[PATH_MAX];
>>>>>>         int msl_idx, ret;
>>>>>> +     char hostname[HOST_NAME_MAX] = { 0 };
>>>>>>
>>>>>>         if (msl->external)
>>>>>>                 return 0;
>>>>>> @@ -1373,9 +1374,13 @@ secondary_msl_create_walk(const struct 
>>>>>> rte_memseg_list *msl,
>>>>>>         primary_msl = &mcfg->memsegs[msl_idx];
>>>>>>         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());
>>>>>> +     /* Create distinct fbarrays for each secondary by using PID and
>>>>>> +      * hostname. The reason why using hostname is because PID 
>>>>>> could be
>>>>>> +      * duplicated among secondaries if it is launched in a 
>>>>>> container.
>>>>>> +      */
>>>>>> +     gethostname(hostname, HOST_NAME_MAX);
>>>>
>>>> Personal preference, s/HOST_NAME_MAX/sizeof(hostname)/.
>>>>
>>>>
>>>> hostname[] is HOST_NAME_MAX bytes long.
>>>> In the worst case, we can get a non NULL terminated hostname string.
>>>> "
>>>>          gethostname() returns the null-terminated hostname in the
>>>> character array name, which has a length of len bytes.  If the
>>>> null-terminated hostname is too large to fit, then the name is
>>>> truncated, and
>>>>          no error is returned (but see NOTES below).  POSIX.1-2001 says
>>>> that if such truncation occurs, then it is unspecified whether the
>>>> returned buffer includes a terminating null byte.
>>>> ...
>>>> NOTES
>>>>          SUSv2 guarantees that "Host names are limited to 255 bytes".
>>>> POSIX.1-2001 guarantees that "Host names (not including the
>>>> terminating null byte) are  limited  to  HOST_NAME_MAX  bytes".   On
>>>> Linux,
>>>>          HOST_NAME_MAX is defined with the value 64, which has been the
>>>> limit since Linux 1.0 (earlier kernels imposed a limit of 8 bytes).
>>>> "
>>>>
>>>> How about making hostname[] HOST_NAME_MAX+1 bytes long?
>>>>
>>>>>> +     snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s_%d",
>>>>>> +                     primary_msl->memseg_arr.name, hostname, 
>>>>>> (int)getpid());
>>>>>>
>>>>>>         ret = rte_fbarray_init(&local_msl->memseg_arr, name,
>>>>>>                 primary_msl->memseg_arr.len,
>>>>>>
>>>>>
>>>>> I think the order should be reversed. Both containers and 
>>>>> non-containers
>>>>> can have their hostname set, and RTE_FBARRAY_NAME_LEN is of fairly
>>>>> limited length, so if the hostname is long enough, the PID never gets
>>>>> into the name string, resulting in duplicates. It is better have 
>>>>> pid first.
>>>>
>>>> Anatoly,
>>>>
>>>> On the principle, it seems better, yes.
>>>> Just the comment on RTE_FBARRAY_NAME_LEN indicates that you missed the
>>>> change at the top of the patch.
>>>> What do you think of this change?
>>>>
>>>
>>> Yes, i did miss that, apologies.
>>>
>>> I don't have a strong opinion on this change, however the above comment
>>> would still be true if we make fbarray size to be hostname_max + 1 - we
>>> still potentially get no space for a pid. So if we're going to have pid
>>> in there as well, it should be hostname_max + pid_max (5 digits?) +
>>> whatever underscores we have + null terminator, to ensure it fits under
>>> any and all circumstances.#
In "man 5 proc", it says the default value of pid_max is 32768, but can 
be set up to 2^22 (=4194304) on 64-bit systems. So, I think it is safer 
to consider pid_max is 7 digits.

I can find secondary's fbarray file named as
$ sudo ls /var/run/dpdk/rte/
...
fbarray_memseg-1048576k-0-0_24118
fbarray_memseg-1048576k-0-0_24191
fbarray_memseg-1048576k-0-0_24199
...

It consists of [prefix]-[hugepage size]-[numa node?]-[memchan?]_[PID]", 
and size of the name before PID is totally 28 digits. If another 
underscore and hostname are included in the name, the max size of 
fbarray file name is
   28 + 7(PID) + 1(underscore) + HOST_NAME_MAX+1(null terminator)

Considering 28 can be larger, how about using 32 as following?
+     int fbarray_sec_name_len = 32 + 7 + 1 + HOST_NAME_MAX + 1;

+     snprintf(name, fbarray_sec_name_len, "%s_%d_%s",
+                     primary_msl->memseg_arr.name, getpid(), hostname);

>>
>> I think that at least on linux we have more than enough space here:
>>
>> $ find /usr/include -type f | xargs grep ' NAME_MAX' | grep define
>> /usr/include/linux/limits.h:#define NAME_MAX         255        /* # 
>> chars in a file name */
>>
>> $ find /usr/include -type f | xargs grep ' HOST_NAME_MAX' | grep define
>> /usr/include/i386-linux-gnu/bits/local_lim.h:#define 
>> HOST_NAME_MAX             64
>> /usr/include/x86_64-linux-gnu/bits/local_lim.h:#define 
>> HOST_NAME_MAX           64
>>
> 
> Okay, works for me :)

Thanks,
Yasufumi

[dpdk-dev] [PATCH v7 0/1] fbarray: fix duplicated fbarray file in secondary

[yasufum.o]

From: Yasufumi Ogawa <yasufum.o@gmail.com>

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work if several secondaries run as app containers because each
of containerized secondary has PID 1, and failed to reserve unique name
other than first one. To reserve unique name in each of containers, use
hostname in addition to PID.

---
v2:
  * fix typo in commit message
v3:
  * add fclose() after if getting hostname with fscan() is failed
v4:
  * Increase the size of proc_id to 33 and add boundary in calling
    fscan()
v5:
  * revise title to reflect the issue
  * use gethostname() instead of getting from `etc/hostname`
  * use HOST_NAME_MAX for size of string for hostname
v6:
  * change to use hostname and pid to cover both of host and container
    cases
  * change RTE_FBARRAY_NAME_LEN to NAME_MAX to reserve enough size for
    filename
v7:
  * discard changing RTE_FBARRAY_NAME_LEN to NAME_MAX to avoid breaking
    ABI
  * introduce int fbarray_sec_name_len instead of RTE_FBARRAY_NAME_LEN
    to define long filename only for secondary process
  * replace the order of postfixes of pid and hostname
---

Yasufumi Ogawa (1):
  fbarray: fix duplicated fbarray file in secondary

 lib/librte_eal/linux/eal/eal_memalloc.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v7 1/1] fbarray: fix duplicated fbarray file in secondary

[yasufum.o]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work if several secondaries run as app containers because each
of containerized secondary has PID 1, and failed to reserve unique name
other than first one. To reserve unique name in each of containers, use
hostname in addition to PID.

Cc: stable@dpdk.org

Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
---
 lib/librte_eal/linux/eal/eal_memalloc.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
index af6d0d023..11de6d4d6 100644
--- a/lib/librte_eal/linux/eal/eal_memalloc.c
+++ b/lib/librte_eal/linux/eal/eal_memalloc.c
@@ -1365,6 +1365,12 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	struct rte_memseg_list *primary_msl, *local_msl;
 	char name[PATH_MAX];
 	int msl_idx, ret;
+	char hostname[HOST_NAME_MAX+1] = { 0 };
+	/* filename of secondary's fbarray is defined such as
+	 * "fbarray_memseg-1048576k-0-0_PID_HOSTNAME" and length of PID
+	 * can be 7 digits maximumly.
+	 */
+	int fbarray_sec_name_len = 32 + 7 + 1 + HOST_NAME_MAX + 1;
 
 	if (msl->external)
 		return 0;
@@ -1373,9 +1379,13 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	primary_msl = &mcfg->memsegs[msl_idx];
 	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());
+	/* Create distinct fbarrays for each secondary by using PID and
+	 * hostname. The reason why using hostname is because PID could be
+	 * duplicated among secondaries if it is launched in a container.
+	 */
+	gethostname(hostname, sizeof(hostname));
+	snprintf(name, fbarray_sec_name_len, "%s_%d_%s",
+			primary_msl->memseg_arr.name, (int)getpid(), hostname);
 
 	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
 		primary_msl->memseg_arr.len,
-- 
2.17.1

Re: [dpdk-dev] [PATCH v7 1/1] fbarray: fix duplicated fbarray file in secondary

[Burakov, Anatoly]

On 13-Nov-19 9:43 PM, yasufum.o@gmail.com wrote:
> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> 
> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> PID for reserving unique name among secondary processes. However, it
> does not work if several secondaries run as app containers because each
> of containerized secondary has PID 1, and failed to reserve unique name
> other than first one. To reserve unique name in each of containers, use
> hostname in addition to PID.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
> ---
>   lib/librte_eal/linux/eal/eal_memalloc.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
> index af6d0d023..11de6d4d6 100644
> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
> @@ -1365,6 +1365,12 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>   	struct rte_memseg_list *primary_msl, *local_msl;
>   	char name[PATH_MAX];
>   	int msl_idx, ret;
> +	char hostname[HOST_NAME_MAX+1] = { 0 };
> +	/* filename of secondary's fbarray is defined such as
> +	 * "fbarray_memseg-1048576k-0-0_PID_HOSTNAME" and length of PID
> +	 * can be 7 digits maximumly.
> +	 */
> +	int fbarray_sec_name_len = 32 + 7 + 1 + HOST_NAME_MAX + 1;

What does 32 stand for? Maybe #define both 32 and 7 values?

Other than that,

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v7 1/1] fbarray: fix duplicated fbarray file in secondary

[Yasufumi Ogawa]

On 2019/11/14 2:01, Burakov, Anatoly wrote:
> On 13-Nov-19 9:43 PM, yasufum.o@gmail.com wrote:
>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>
>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
>> PID for reserving unique name among secondary processes. However, it
>> does not work if several secondaries run as app containers because each
>> of containerized secondary has PID 1, and failed to reserve unique name
>> other than first one. To reserve unique name in each of containers, use
>> hostname in addition to PID.
>>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
>> ---
>>   lib/librte_eal/linux/eal/eal_memalloc.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c 
>> b/lib/librte_eal/linux/eal/eal_memalloc.c
>> index af6d0d023..11de6d4d6 100644
>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
>> @@ -1365,6 +1365,12 @@ secondary_msl_create_walk(const struct 
>> rte_memseg_list *msl,
>>       struct rte_memseg_list *primary_msl, *local_msl;
>>       char name[PATH_MAX];
>>       int msl_idx, ret;
>> +    char hostname[HOST_NAME_MAX+1] = { 0 };
>> +    /* filename of secondary's fbarray is defined such as
>> +     * "fbarray_memseg-1048576k-0-0_PID_HOSTNAME" and length of PID
>> +     * can be 7 digits maximumly.
>> +     */
>> +    int fbarray_sec_name_len = 32 + 7 + 1 + HOST_NAME_MAX + 1;
> 
> What does 32 stand for? Maybe #define both 32 and 7 values?
Hi Anatoly,

Thank you for your comments! If my understanding is correct, the prefix 
"fbarray_memseg-1048576k-0-0_" is 28 digits and it could be larger if 
using the size of hugepage or the number of NUMA nodes are larger 
possibly. However, I think 32 digits is still enough.

 > Maybe #define both 32 and 7 values?
Yes. I think it should be better to use #define if this values are 
referred several times.

Thanks,
Yasufumi

> 
> Other than that,
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 

Re: [dpdk-dev] [PATCH v7 1/1] fbarray: fix duplicated fbarray file in secondary

[David Marchand]

On Thu, Nov 14, 2019 at 12:42 PM Yasufumi Ogawa <yasufum.o@gmail.com> wrote:
>
> On 2019/11/14 2:01, Burakov, Anatoly wrote:
> > On 13-Nov-19 9:43 PM, yasufum.o@gmail.com wrote:
> >> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> >>
> >> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> >> PID for reserving unique name among secondary processes. However, it
> >> does not work if several secondaries run as app containers because each
> >> of containerized secondary has PID 1, and failed to reserve unique name
> >> other than first one. To reserve unique name in each of containers, use
> >> hostname in addition to PID.
> >>
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
> >> ---
> >>   lib/librte_eal/linux/eal/eal_memalloc.c | 16 +++++++++++++---
> >>   1 file changed, 13 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c
> >> b/lib/librte_eal/linux/eal/eal_memalloc.c
> >> index af6d0d023..11de6d4d6 100644
> >> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
> >> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
> >> @@ -1365,6 +1365,12 @@ secondary_msl_create_walk(const struct
> >> rte_memseg_list *msl,
> >>       struct rte_memseg_list *primary_msl, *local_msl;
> >>       char name[PATH_MAX];
> >>       int msl_idx, ret;
> >> +    char hostname[HOST_NAME_MAX+1] = { 0 };
> >> +    /* filename of secondary's fbarray is defined such as
> >> +     * "fbarray_memseg-1048576k-0-0_PID_HOSTNAME" and length of PID
> >> +     * can be 7 digits maximumly.
> >> +     */
> >> +    int fbarray_sec_name_len = 32 + 7 + 1 + HOST_NAME_MAX + 1;
> >
> > What does 32 stand for? Maybe #define both 32 and 7 values?
> Hi Anatoly,
>
> Thank you for your comments! If my understanding is correct, the prefix
> "fbarray_memseg-1048576k-0-0_" is 28 digits and it could be larger if
> using the size of hugepage or the number of NUMA nodes are larger
> possibly. However, I think 32 digits is still enough.
>
>  > Maybe #define both 32 and 7 values?
> Yes. I think it should be better to use #define if this values are
> referred several times.


We can truncate to RTE_FBARRAY_NAME_LEN in all cases.
And iiuc, rte_fbarray_init will refuse any longer name anyway.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v7 1/1] fbarray: fix duplicated fbarray file in secondary

[David Marchand]

On Wed, Nov 13, 2019 at 10:44 PM <yasufum.o@gmail.com> wrote:
> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>
> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> PID for reserving unique name among secondary processes. However, it
> does not work if several secondaries run as app containers because each
> of containerized secondary has PID 1, and failed to reserve unique name
> other than first one. To reserve unique name in each of containers, use
> hostname in addition to PID.
>
> Cc: stable@dpdk.org
>
> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>

Your SoB line is not aligned with the Author.

-- 
David Marchand

Re: [dpdk-dev] [PATCH v7 1/1] fbarray: fix duplicated fbarray file in secondary

[Ananyev, Konstantin]

> -----Original Message-----
> From: yasufum.o@gmail.com <yasufum.o@gmail.com>
> Sent: Wednesday, November 13, 2019 9:44 PM
> To: Burakov, Anatoly <anatoly.burakov@intel.com>; david.marchand@redhat.com; Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; yasufum.o@gmail.com; Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> Subject: [PATCH v7 1/1] fbarray: fix duplicated fbarray file in secondary
> 
> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> 
> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> PID for reserving unique name among secondary processes. However, it
> does not work if several secondaries run as app containers because each
> of containerized secondary has PID 1, and failed to reserve unique name
> other than first one. To reserve unique name in each of containers, use
> hostname in addition to PID.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
> ---
>  lib/librte_eal/linux/eal/eal_memalloc.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
> index af6d0d023..11de6d4d6 100644
> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
> @@ -1365,6 +1365,12 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>  	struct rte_memseg_list *primary_msl, *local_msl;
>  	char name[PATH_MAX];
>  	int msl_idx, ret;
> +	char hostname[HOST_NAME_MAX+1] = { 0 };
> +	/* filename of secondary's fbarray is defined such as
> +	 * "fbarray_memseg-1048576k-0-0_PID_HOSTNAME" and length of PID
> +	 * can be 7 digits maximumly.
> +	 */
> +	int fbarray_sec_name_len = 32 + 7 + 1 + HOST_NAME_MAX + 1;

Not sure, why do we need this calculation at all?
Konstantin


> 
>  	if (msl->external)
>  		return 0;
> @@ -1373,9 +1379,13 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
>  	primary_msl = &mcfg->memsegs[msl_idx];
>  	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());
> +	/* Create distinct fbarrays for each secondary by using PID and
> +	 * hostname. The reason why using hostname is because PID could be
> +	 * duplicated among secondaries if it is launched in a container.
> +	 */
> +	gethostname(hostname, sizeof(hostname));
> +	snprintf(name, fbarray_sec_name_len, "%s_%d_%s",
> +			primary_msl->memseg_arr.name, (int)getpid(), hostname);
> 
>  	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
>  		primary_msl->memseg_arr.len,
> --
> 2.17.1

Re: [dpdk-dev] [PATCH v7 1/1] fbarray: fix duplicated fbarray file in secondary

[Yasufumi Ogawa]

Hi David,

Sorry for slow reply.

On 2019/11/14 21:27, David Marchand wrote:
> On Thu, Nov 14, 2019 at 12:42 PM Yasufumi Ogawa <yasufum.o@gmail.com> wrote:
>>
>> On 2019/11/14 2:01, Burakov, Anatoly wrote:
>>> On 13-Nov-19 9:43 PM, yasufum.o@gmail.com wrote:
>>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>
>>>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
>>>> PID for reserving unique name among secondary processes. However, it
>>>> does not work if several secondaries run as app containers because each
>>>> of containerized secondary has PID 1, and failed to reserve unique name
>>>> other than first one. To reserve unique name in each of containers, use
>>>> hostname in addition to PID.
>>>>
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
>>>> ---
>>>>    lib/librte_eal/linux/eal/eal_memalloc.c | 16 +++++++++++++---
>>>>    1 file changed, 13 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c
>>>> b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>> index af6d0d023..11de6d4d6 100644
>>>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
>>>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>> @@ -1365,6 +1365,12 @@ secondary_msl_create_walk(const struct
>>>> rte_memseg_list *msl,
>>>>        struct rte_memseg_list *primary_msl, *local_msl;
>>>>        char name[PATH_MAX];
>>>>        int msl_idx, ret;
>>>> +    char hostname[HOST_NAME_MAX+1] = { 0 };
>>>> +    /* filename of secondary's fbarray is defined such as
>>>> +     * "fbarray_memseg-1048576k-0-0_PID_HOSTNAME" and length of PID
>>>> +     * can be 7 digits maximumly.
>>>> +     */
>>>> +    int fbarray_sec_name_len = 32 + 7 + 1 + HOST_NAME_MAX + 1;
>>>
>>> What does 32 stand for? Maybe #define both 32 and 7 values?
>> Hi Anatoly,
>>
>> Thank you for your comments! If my understanding is correct, the prefix
>> "fbarray_memseg-1048576k-0-0_" is 28 digits and it could be larger if
>> using the size of hugepage or the number of NUMA nodes are larger
>> possibly. However, I think 32 digits is still enough.
>>
>>   > Maybe #define both 32 and 7 values?
>> Yes. I think it should be better to use #define if this values are
>> referred several times.
> 
> 
> We can truncate to RTE_FBARRAY_NAME_LEN in all cases.
> And iiuc, rte_fbarray_init will refuse any longer name anyway.
Could I confirm the issue? I've understood that it is failed to validate 
the name of fbarray in fully_validate() at 
"lib/librte_eal/common/eal_common_fbarray.c:697".

static int
fully_validate(const char *name, unsigned int elt_sz, unsigned int len)
{
         if (name == NULL || elt_sz == 0 || len == 0 || len > INT_MAX) {
                 rte_errno = EINVAL;
                 return -1;
         }

         if (strnlen(name, RTE_FBARRAY_NAME_LEN) == RTE_FBARRAY_NAME_LEN) {
                 rte_errno = ENAMETOOLONG;
                 return -1;
         }
         return 0;
}

I should overwrite the definition of RTE_FBARRAY_NAME_LEN as previous 
patch in this case, and it causes an ABI breakage, right? If so, I would 
like to make the change and give up to update stable release.

Thanks,
Yasufumi

> 
> 

[dpdk-dev] [PATCH v8 0/1] fbarray: fix duplicated fbarray file in secondary

[Yasufumi Ogawa]

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work if several secondaries run as app containers because each
of containerized secondary has PID 1, and failed to reserve unique name
other than first one. To reserve unique name in each of containers, use
hostname in addition to PID.

---
v2:
  * fix typo in commit message
v3:
  * add fclose() after if getting hostname with fscan() is failed
v4:
  * Increase the size of proc_id to 33 and add boundary in calling
    fscan()
v5:
  * revise title to reflect the issue
  * use gethostname() instead of getting from `etc/hostname`
  * use HOST_NAME_MAX for size of string for hostname
v6:
  * change to use hostname and pid to cover both of host and container
    cases
  * change RTE_FBARRAY_NAME_LEN to NAME_MAX to reserve enough size for
    filename
v7:
  * discard changing RTE_FBARRAY_NAME_LEN to NAME_MAX to avoid breaking
    ABI
  * introduce int fbarray_sec_name_len instead of RTE_FBARRAY_NAME_LEN
    to define long filename only for secondary process
  * replace the order of postfixes of pid and hostname
v8:
  * change RTE_FBARRAY_NAME_LEN to the maximum size for secondary
  * fix warning of Signed-off-by
---

Yasufumi Ogawa (1):
  fbarray: fix duplicated fbarray file in secondary

 lib/librte_eal/common/include/rte_fbarray.h |  7 ++++++-
 lib/librte_eal/linux/eal/eal_memalloc.c     | 11 ++++++++---
 2 files changed, 14 insertions(+), 4 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v8 1/1] fbarray: fix duplicated fbarray file in secondary

[Yasufumi Ogawa]

From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>

In secondary_msl_create_walk(), it creates a file for fbarrays with its
PID for reserving unique name among secondary processes. However, it
does not work if several secondaries run as app containers because each
of containerized secondary has PID 1, and failed to reserve unique name
other than first one. To reserve unique name in each of containers, use
hostname in addition to PID.

Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
---
 lib/librte_eal/common/include/rte_fbarray.h |  7 ++++++-
 lib/librte_eal/linux/eal/eal_memalloc.c     | 11 ++++++++---
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_fbarray.h b/lib/librte_eal/common/include/rte_fbarray.h
index 6dccdbec9..df003b8dc 100644
--- a/lib/librte_eal/common/include/rte_fbarray.h
+++ b/lib/librte_eal/common/include/rte_fbarray.h
@@ -39,7 +39,12 @@ extern "C" {
 #include <rte_compat.h>
 #include <rte_rwlock.h>
 
-#define RTE_FBARRAY_NAME_LEN 64
+/* Filename of fbarray is defined as a combination of several params
+ * such as "fbarray_memseg-1048576k-0-0_PID_HOSTNAME".
+ * The length of string before PID can be 32bytes, and the length of
+ * PID can be 7bytes maximamly. Final 1 byte is for null terminator.
+ */
+#define RTE_FBARRAY_NAME_LEN (32 + 7 + 1 + HOST_NAME_MAX + 1)
 
 struct rte_fbarray {
 	char name[RTE_FBARRAY_NAME_LEN]; /**< name associated with an array */
diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c b/lib/librte_eal/linux/eal/eal_memalloc.c
index af6d0d023..8c50d3355 100644
--- a/lib/librte_eal/linux/eal/eal_memalloc.c
+++ b/lib/librte_eal/linux/eal/eal_memalloc.c
@@ -1365,6 +1365,7 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	struct rte_memseg_list *primary_msl, *local_msl;
 	char name[PATH_MAX];
 	int msl_idx, ret;
+	char hostname[HOST_NAME_MAX+1] = { 0 };
 
 	if (msl->external)
 		return 0;
@@ -1373,9 +1374,13 @@ secondary_msl_create_walk(const struct rte_memseg_list *msl,
 	primary_msl = &mcfg->memsegs[msl_idx];
 	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());
+	/* Create distinct fbarrays for each secondary by using PID and
+	 * hostname. The reason why using hostname is because PID could be
+	 * duplicated among secondaries if it is launched in a container.
+	 */
+	gethostname(hostname, sizeof(hostname));
+	snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%d_%s",
+			primary_msl->memseg_arr.name, (int)getpid(), hostname);
 
 	ret = rte_fbarray_init(&local_msl->memseg_arr, name,
 		primary_msl->memseg_arr.len,
-- 
2.17.1

Re: [dpdk-dev] [PATCH v7 1/1] fbarray: fix duplicated fbarray file in secondary

[Burakov, Anatoly]

On 26-Nov-19 7:40 PM, Yasufumi Ogawa wrote:
> Hi David,
> 
> Sorry for slow reply.
> 
> On 2019/11/14 21:27, David Marchand wrote:
>> On Thu, Nov 14, 2019 at 12:42 PM Yasufumi Ogawa <yasufum.o@gmail.com> 
>> wrote:
>>>
>>> On 2019/11/14 2:01, Burakov, Anatoly wrote:
>>>> On 13-Nov-19 9:43 PM, yasufum.o@gmail.com wrote:
>>>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>>
>>>>> In secondary_msl_create_walk(), it creates a file for fbarrays with 
>>>>> its
>>>>> PID for reserving unique name among secondary processes. However, it
>>>>> does not work if several secondaries run as app containers because 
>>>>> each
>>>>> of containerized secondary has PID 1, and failed to reserve unique 
>>>>> name
>>>>> other than first one. To reserve unique name in each of containers, 
>>>>> use
>>>>> hostname in addition to PID.
>>>>>
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
>>>>> ---
>>>>>    lib/librte_eal/linux/eal/eal_memalloc.c | 16 +++++++++++++---
>>>>>    1 file changed, 13 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>> b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>> index af6d0d023..11de6d4d6 100644
>>>>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>> @@ -1365,6 +1365,12 @@ secondary_msl_create_walk(const struct
>>>>> rte_memseg_list *msl,
>>>>>        struct rte_memseg_list *primary_msl, *local_msl;
>>>>>        char name[PATH_MAX];
>>>>>        int msl_idx, ret;
>>>>> +    char hostname[HOST_NAME_MAX+1] = { 0 };
>>>>> +    /* filename of secondary's fbarray is defined such as
>>>>> +     * "fbarray_memseg-1048576k-0-0_PID_HOSTNAME" and length of PID
>>>>> +     * can be 7 digits maximumly.
>>>>> +     */
>>>>> +    int fbarray_sec_name_len = 32 + 7 + 1 + HOST_NAME_MAX + 1;
>>>>
>>>> What does 32 stand for? Maybe #define both 32 and 7 values?
>>> Hi Anatoly,
>>>
>>> Thank you for your comments! If my understanding is correct, the prefix
>>> "fbarray_memseg-1048576k-0-0_" is 28 digits and it could be larger if
>>> using the size of hugepage or the number of NUMA nodes are larger
>>> possibly. However, I think 32 digits is still enough.
>>>
>>>   > Maybe #define both 32 and 7 values?
>>> Yes. I think it should be better to use #define if this values are
>>> referred several times.
>>
>>
>> We can truncate to RTE_FBARRAY_NAME_LEN in all cases.
>> And iiuc, rte_fbarray_init will refuse any longer name anyway.
> Could I confirm the issue? I've understood that it is failed to validate 
> the name of fbarray in fully_validate() at 
> "lib/librte_eal/common/eal_common_fbarray.c:697".
> 
> static int
> fully_validate(const char *name, unsigned int elt_sz, unsigned int len)
> {
>          if (name == NULL || elt_sz == 0 || len == 0 || len > INT_MAX) {
>                  rte_errno = EINVAL;
>                  return -1;
>          }
> 
>          if (strnlen(name, RTE_FBARRAY_NAME_LEN) == RTE_FBARRAY_NAME_LEN) {
>                  rte_errno = ENAMETOOLONG;
>                  return -1;
>          }
>          return 0;
> }
> 
> I should overwrite the definition of RTE_FBARRAY_NAME_LEN as previous 
> patch in this case, and it causes an ABI breakage, right? If so, I would 
> like to make the change and give up to update stable release.
> 
> Thanks,
> Yasufumi
> 

It seems we're getting into bikeshedding...

We can do this without ABI breakage. You could have just used 
RTE_FBARRAY_NAME_LEN as max fbarray name length for fbarray_sec_name_len 
(i.e. that would include hostname + pid + whatever else there is). The 
name, as David has pointed out, would be truncated to 
RTE_FBARRAY_NAME_LEN anyway (or, more precisely, it will be refused if 
it's longer than that), so this is the most you can have - so you can 
just use that as the maximum.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v7 1/1] fbarray: fix duplicated fbarray file in secondary

[Yasufumi Ogawa]

Hi Anatoly,

On 2019/11/27 19:26, Burakov, Anatoly wrote:
> On 26-Nov-19 7:40 PM, Yasufumi Ogawa wrote:
>> Hi David,
>>
>> Sorry for slow reply.
>>
>> On 2019/11/14 21:27, David Marchand wrote:
>>> On Thu, Nov 14, 2019 at 12:42 PM Yasufumi Ogawa <yasufum.o@gmail.com> 
>>> wrote:
>>>>
>>>> On 2019/11/14 2:01, Burakov, Anatoly wrote:
>>>>> On 13-Nov-19 9:43 PM, yasufum.o@gmail.com wrote:
>>>>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>>>
>>>>>> In secondary_msl_create_walk(), it creates a file for fbarrays 
>>>>>> with its
>>>>>> PID for reserving unique name among secondary processes. However, it
>>>>>> does not work if several secondaries run as app containers because 
>>>>>> each
>>>>>> of containerized secondary has PID 1, and failed to reserve unique 
>>>>>> name
>>>>>> other than first one. To reserve unique name in each of 
>>>>>> containers, use
>>>>>> hostname in addition to PID.
>>>>>>
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
>>>>>> ---
>>>>>>    lib/librte_eal/linux/eal/eal_memalloc.c | 16 +++++++++++++---
>>>>>>    1 file changed, 13 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>>> b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>>> index af6d0d023..11de6d4d6 100644
>>>>>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>>> @@ -1365,6 +1365,12 @@ secondary_msl_create_walk(const struct
>>>>>> rte_memseg_list *msl,
>>>>>>        struct rte_memseg_list *primary_msl, *local_msl;
>>>>>>        char name[PATH_MAX];
>>>>>>        int msl_idx, ret;
>>>>>> +    char hostname[HOST_NAME_MAX+1] = { 0 };
>>>>>> +    /* filename of secondary's fbarray is defined such as
>>>>>> +     * "fbarray_memseg-1048576k-0-0_PID_HOSTNAME" and length of PID
>>>>>> +     * can be 7 digits maximumly.
>>>>>> +     */
>>>>>> +    int fbarray_sec_name_len = 32 + 7 + 1 + HOST_NAME_MAX + 1;
>>>>>
>>>>> What does 32 stand for? Maybe #define both 32 and 7 values?
>>>> Hi Anatoly,
>>>>
>>>> Thank you for your comments! If my understanding is correct, the prefix
>>>> "fbarray_memseg-1048576k-0-0_" is 28 digits and it could be larger if
>>>> using the size of hugepage or the number of NUMA nodes are larger
>>>> possibly. However, I think 32 digits is still enough.
>>>>
>>>>   > Maybe #define both 32 and 7 values?
>>>> Yes. I think it should be better to use #define if this values are
>>>> referred several times.
>>>
>>>
>>> We can truncate to RTE_FBARRAY_NAME_LEN in all cases.
>>> And iiuc, rte_fbarray_init will refuse any longer name anyway.
>> Could I confirm the issue? I've understood that it is failed to 
>> validate the name of fbarray in fully_validate() at 
>> "lib/librte_eal/common/eal_common_fbarray.c:697".
>>
>> static int
>> fully_validate(const char *name, unsigned int elt_sz, unsigned int len)
>> {
>>          if (name == NULL || elt_sz == 0 || len == 0 || len > INT_MAX) {
>>                  rte_errno = EINVAL;
>>                  return -1;
>>          }
>>
>>          if (strnlen(name, RTE_FBARRAY_NAME_LEN) == 
>> RTE_FBARRAY_NAME_LEN) {
>>                  rte_errno = ENAMETOOLONG;
>>                  return -1;
>>          }
>>          return 0;
>> }
>>
>> I should overwrite the definition of RTE_FBARRAY_NAME_LEN as previous 
>> patch in this case, and it causes an ABI breakage, right? If so, I 
>> would like to make the change and give up to update stable release.
>>
>> Thanks,
>> Yasufumi
>>
> 
> It seems we're getting into bikeshedding...
> 
> We can do this without ABI breakage. You could have just used 
> RTE_FBARRAY_NAME_LEN as max fbarray name length for fbarray_sec_name_len 
> (i.e. that would include hostname + pid + whatever else there is). The 
> name, as David has pointed out, would be truncated to 
> RTE_FBARRAY_NAME_LEN anyway (or, more precisely, it will be refused if 
> it's longer than that), so this is the most you can have - so you can 
> just use that as the maximum.
I sent v8 patch to change the size of RTE_FBARRAY_NAME_LEN itself to be 
allowed the size of secondary's fbarray over 64 bytes. I appreciate if 
you agree that.

Thanks,
Yasufumi

Re: [dpdk-dev] [PATCH v7 1/1] fbarray: fix duplicated fbarray file in secondary

[Burakov, Anatoly]

On 29-Nov-19 5:44 AM, Yasufumi Ogawa wrote:
> Hi Anatoly,
> 
> On 2019/11/27 19:26, Burakov, Anatoly wrote:
>> On 26-Nov-19 7:40 PM, Yasufumi Ogawa wrote:
>>> Hi David,
>>>
>>> Sorry for slow reply.
>>>
>>> On 2019/11/14 21:27, David Marchand wrote:
>>>> On Thu, Nov 14, 2019 at 12:42 PM Yasufumi Ogawa 
>>>> <yasufum.o@gmail.com> wrote:
>>>>>
>>>>> On 2019/11/14 2:01, Burakov, Anatoly wrote:
>>>>>> On 13-Nov-19 9:43 PM, yasufum.o@gmail.com wrote:
>>>>>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>>>>
>>>>>>> In secondary_msl_create_walk(), it creates a file for fbarrays 
>>>>>>> with its
>>>>>>> PID for reserving unique name among secondary processes. However, it
>>>>>>> does not work if several secondaries run as app containers 
>>>>>>> because each
>>>>>>> of containerized secondary has PID 1, and failed to reserve 
>>>>>>> unique name
>>>>>>> other than first one. To reserve unique name in each of 
>>>>>>> containers, use
>>>>>>> hostname in addition to PID.
>>>>>>>
>>>>>>> Cc: stable@dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
>>>>>>> ---
>>>>>>>    lib/librte_eal/linux/eal/eal_memalloc.c | 16 +++++++++++++---
>>>>>>>    1 file changed, 13 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>>>> b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>>>> index af6d0d023..11de6d4d6 100644
>>>>>>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>>>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>>>> @@ -1365,6 +1365,12 @@ secondary_msl_create_walk(const struct
>>>>>>> rte_memseg_list *msl,
>>>>>>>        struct rte_memseg_list *primary_msl, *local_msl;
>>>>>>>        char name[PATH_MAX];
>>>>>>>        int msl_idx, ret;
>>>>>>> +    char hostname[HOST_NAME_MAX+1] = { 0 };
>>>>>>> +    /* filename of secondary's fbarray is defined such as
>>>>>>> +     * "fbarray_memseg-1048576k-0-0_PID_HOSTNAME" and length of PID
>>>>>>> +     * can be 7 digits maximumly.
>>>>>>> +     */
>>>>>>> +    int fbarray_sec_name_len = 32 + 7 + 1 + HOST_NAME_MAX + 1;
>>>>>>
>>>>>> What does 32 stand for? Maybe #define both 32 and 7 values?
>>>>> Hi Anatoly,
>>>>>
>>>>> Thank you for your comments! If my understanding is correct, the 
>>>>> prefix
>>>>> "fbarray_memseg-1048576k-0-0_" is 28 digits and it could be larger if
>>>>> using the size of hugepage or the number of NUMA nodes are larger
>>>>> possibly. However, I think 32 digits is still enough.
>>>>>
>>>>>   > Maybe #define both 32 and 7 values?
>>>>> Yes. I think it should be better to use #define if this values are
>>>>> referred several times.
>>>>
>>>>
>>>> We can truncate to RTE_FBARRAY_NAME_LEN in all cases.
>>>> And iiuc, rte_fbarray_init will refuse any longer name anyway.
>>> Could I confirm the issue? I've understood that it is failed to 
>>> validate the name of fbarray in fully_validate() at 
>>> "lib/librte_eal/common/eal_common_fbarray.c:697".
>>>
>>> static int
>>> fully_validate(const char *name, unsigned int elt_sz, unsigned int len)
>>> {
>>>          if (name == NULL || elt_sz == 0 || len == 0 || len > INT_MAX) {
>>>                  rte_errno = EINVAL;
>>>                  return -1;
>>>          }
>>>
>>>          if (strnlen(name, RTE_FBARRAY_NAME_LEN) == 
>>> RTE_FBARRAY_NAME_LEN) {
>>>                  rte_errno = ENAMETOOLONG;
>>>                  return -1;
>>>          }
>>>          return 0;
>>> }
>>>
>>> I should overwrite the definition of RTE_FBARRAY_NAME_LEN as previous 
>>> patch in this case, and it causes an ABI breakage, right? If so, I 
>>> would like to make the change and give up to update stable release.
>>>
>>> Thanks,
>>> Yasufumi
>>>
>>
>> It seems we're getting into bikeshedding...
>>
>> We can do this without ABI breakage. You could have just used 
>> RTE_FBARRAY_NAME_LEN as max fbarray name length for 
>> fbarray_sec_name_len (i.e. that would include hostname + pid + 
>> whatever else there is). The name, as David has pointed out, would be 
>> truncated to RTE_FBARRAY_NAME_LEN anyway (or, more precisely, it will 
>> be refused if it's longer than that), so this is the most you can have 
>> - so you can just use that as the maximum.
> I sent v8 patch to change the size of RTE_FBARRAY_NAME_LEN itself to be 
> allowed the size of secondary's fbarray over 64 bytes. I appreciate if 
> you agree that.
> 

Why not just limit the name to RTE_FBARRAY_NAME_LEN instead of changing 
the definition of RTE_FBARRAY_NAME_LEN?

One the other hand, technically, fbarray API is experimental. The only 
structure that uses rte_fbarray is rte_memseg_list, but API's using 
either rte_fbarray or rte_memseg_list are either internal (memory/VFIO 
subsystem), or are marked as experimental (walk functions).

So i *think* we're actually OK with changing the length of 
RTE_FBARRAY_NAME_LEN as far as ABI policy goes: nothing in the stable 
ABI gets affected. David, thoughts?

(i think it's probably time to make experimental memory/fbarray stuff 
stable, but that's a different conversation...)

> Thanks,
> Yasufumi
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v7 1/1] fbarray: fix duplicated fbarray file in secondary

[Yasufumi Ogawa]

Hi Anatoly,

On 2019/12/02 19:43, Burakov, Anatoly wrote:
> On 29-Nov-19 5:44 AM, Yasufumi Ogawa wrote:
>> Hi Anatoly,
>>
>> On 2019/11/27 19:26, Burakov, Anatoly wrote:
>>> On 26-Nov-19 7:40 PM, Yasufumi Ogawa wrote:
>>>> Hi David,
>>>>
>>>> Sorry for slow reply.
>>>>
>>>> On 2019/11/14 21:27, David Marchand wrote:
>>>>> On Thu, Nov 14, 2019 at 12:42 PM Yasufumi Ogawa 
>>>>> <yasufum.o@gmail.com> wrote:
>>>>>>
>>>>>> On 2019/11/14 2:01, Burakov, Anatoly wrote:
>>>>>>> On 13-Nov-19 9:43 PM, yasufum.o@gmail.com wrote:
>>>>>>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>>>>>>>
>>>>>>>> In secondary_msl_create_walk(), it creates a file for fbarrays 
>>>>>>>> with its
>>>>>>>> PID for reserving unique name among secondary processes. 
>>>>>>>> However, it
>>>>>>>> does not work if several secondaries run as app containers 
>>>>>>>> because each
>>>>>>>> of containerized secondary has PID 1, and failed to reserve 
>>>>>>>> unique name
>>>>>>>> other than first one. To reserve unique name in each of 
>>>>>>>> containers, use
>>>>>>>> hostname in addition to PID.
>>>>>>>>
>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>
>>>>>>>> Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
>>>>>>>> ---
>>>>>>>>    lib/librte_eal/linux/eal/eal_memalloc.c | 16 +++++++++++++---
>>>>>>>>    1 file changed, 13 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>>>>> b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>>>>> index af6d0d023..11de6d4d6 100644
>>>>>>>> --- a/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>>>>> +++ b/lib/librte_eal/linux/eal/eal_memalloc.c
>>>>>>>> @@ -1365,6 +1365,12 @@ secondary_msl_create_walk(const struct
>>>>>>>> rte_memseg_list *msl,
>>>>>>>>        struct rte_memseg_list *primary_msl, *local_msl;
>>>>>>>>        char name[PATH_MAX];
>>>>>>>>        int msl_idx, ret;
>>>>>>>> +    char hostname[HOST_NAME_MAX+1] = { 0 };
>>>>>>>> +    /* filename of secondary's fbarray is defined such as
>>>>>>>> +     * "fbarray_memseg-1048576k-0-0_PID_HOSTNAME" and length of 
>>>>>>>> PID
>>>>>>>> +     * can be 7 digits maximumly.
>>>>>>>> +     */
>>>>>>>> +    int fbarray_sec_name_len = 32 + 7 + 1 + HOST_NAME_MAX + 1;
>>>>>>>
>>>>>>> What does 32 stand for? Maybe #define both 32 and 7 values?
>>>>>> Hi Anatoly,
>>>>>>
>>>>>> Thank you for your comments! If my understanding is correct, the 
>>>>>> prefix
>>>>>> "fbarray_memseg-1048576k-0-0_" is 28 digits and it could be larger if
>>>>>> using the size of hugepage or the number of NUMA nodes are larger
>>>>>> possibly. However, I think 32 digits is still enough.
>>>>>>
>>>>>>   > Maybe #define both 32 and 7 values?
>>>>>> Yes. I think it should be better to use #define if this values are
>>>>>> referred several times.
>>>>>
>>>>>
>>>>> We can truncate to RTE_FBARRAY_NAME_LEN in all cases.
>>>>> And iiuc, rte_fbarray_init will refuse any longer name anyway.
>>>> Could I confirm the issue? I've understood that it is failed to 
>>>> validate the name of fbarray in fully_validate() at 
>>>> "lib/librte_eal/common/eal_common_fbarray.c:697".
>>>>
>>>> static int
>>>> fully_validate(const char *name, unsigned int elt_sz, unsigned int len)
>>>> {
>>>>          if (name == NULL || elt_sz == 0 || len == 0 || len > 
>>>> INT_MAX) {
>>>>                  rte_errno = EINVAL;
>>>>                  return -1;
>>>>          }
>>>>
>>>>          if (strnlen(name, RTE_FBARRAY_NAME_LEN) == 
>>>> RTE_FBARRAY_NAME_LEN) {
>>>>                  rte_errno = ENAMETOOLONG;
>>>>                  return -1;
>>>>          }
>>>>          return 0;
>>>> }
>>>>
>>>> I should overwrite the definition of RTE_FBARRAY_NAME_LEN as 
>>>> previous patch in this case, and it causes an ABI breakage, right? 
>>>> If so, I would like to make the change and give up to update stable 
>>>> release.
>>>>
>>>> Thanks,
>>>> Yasufumi
>>>>
>>>
>>> It seems we're getting into bikeshedding...
>>>
>>> We can do this without ABI breakage. You could have just used 
>>> RTE_FBARRAY_NAME_LEN as max fbarray name length for 
>>> fbarray_sec_name_len (i.e. that would include hostname + pid + 
>>> whatever else there is). The name, as David has pointed out, would be 
>>> truncated to RTE_FBARRAY_NAME_LEN anyway (or, more precisely, it will 
>>> be refused if it's longer than that), so this is the most you can 
>>> have - so you can just use that as the maximum.
>> I sent v8 patch to change the size of RTE_FBARRAY_NAME_LEN itself to 
>> be allowed the size of secondary's fbarray over 64 bytes. I appreciate 
>> if you agree that.
>>
> 
> Why not just limit the name to RTE_FBARRAY_NAME_LEN instead of changing 
> the definition of RTE_FBARRAY_NAME_LEN?

Could I confirm my understanding? I understand that RTE_FBARRAY_NAME_LEN 
is 64 currently and it is not enough for secondary in a container if 
hostname is added to the name of secondary's fbarray.

Regards,
Yasufumi

> 
> One the other hand, technically, fbarray API is experimental. The only 
> structure that uses rte_fbarray is rte_memseg_list, but API's using 
> either rte_fbarray or rte_memseg_list are either internal (memory/VFIO 
> subsystem), or are marked as experimental (walk functions).
> 
> So i *think* we're actually OK with changing the length of 
> RTE_FBARRAY_NAME_LEN as far as ABI policy goes: nothing in the stable 
> ABI gets affected. David, thoughts?
> 
> (i think it's probably time to make experimental memory/fbarray stuff 
> stable, but that's a different conversation...)
> 
>> Thanks,
>> Yasufumi
>>
> 
> 

Re: [dpdk-dev] [PATCH v8 1/1] fbarray: fix duplicated fbarray file in secondary

[Burakov, Anatoly]

On 27-Nov-19 8:48 AM, Yasufumi Ogawa wrote:
> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> 
> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> PID for reserving unique name among secondary processes. However, it
> does not work if several secondaries run as app containers because each
> of containerized secondary has PID 1, and failed to reserve unique name
> other than first one. To reserve unique name in each of containers, use
> hostname in addition to PID.
> 
> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> ---
>   lib/librte_eal/common/include/rte_fbarray.h |  7 ++++++-
>   lib/librte_eal/linux/eal/eal_memalloc.c     | 11 ++++++++---
>   2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_fbarray.h b/lib/librte_eal/common/include/rte_fbarray.h
> index 6dccdbec9..df003b8dc 100644
> --- a/lib/librte_eal/common/include/rte_fbarray.h
> +++ b/lib/librte_eal/common/include/rte_fbarray.h
> @@ -39,7 +39,12 @@ extern "C" {
>   #include <rte_compat.h>
>   #include <rte_rwlock.h>
>   
> -#define RTE_FBARRAY_NAME_LEN 64
> +/* Filename of fbarray is defined as a combination of several params
> + * such as "fbarray_memseg-1048576k-0-0_PID_HOSTNAME".
> + * The length of string before PID can be 32bytes, and the length of
> + * PID can be 7bytes maximamly. Final 1 byte is for null terminator.
> + */
> +#define RTE_FBARRAY_NAME_LEN (32 + 7 + 1 + HOST_NAME_MAX + 1)
>   

This breaks ABI, but i believe this doesn't break *stable* ABI, so it is OK.

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v8 1/1] fbarray: fix duplicated fbarray file in secondary

[Yasufumi Ogawa]

On 2019/12/06 19:44, Burakov, Anatoly wrote:
> On 27-Nov-19 8:48 AM, Yasufumi Ogawa wrote:
>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>
>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
>> PID for reserving unique name among secondary processes. However, it
>> does not work if several secondaries run as app containers because each
>> of containerized secondary has PID 1, and failed to reserve unique name
>> other than first one. To reserve unique name in each of containers, use
>> hostname in addition to PID.
>>
>> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>> ---
>>   lib/librte_eal/common/include/rte_fbarray.h |  7 ++++++-
>>   lib/librte_eal/linux/eal/eal_memalloc.c     | 11 ++++++++---
>>   2 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/include/rte_fbarray.h 
>> b/lib/librte_eal/common/include/rte_fbarray.h
>> index 6dccdbec9..df003b8dc 100644
>> --- a/lib/librte_eal/common/include/rte_fbarray.h
>> +++ b/lib/librte_eal/common/include/rte_fbarray.h
>> @@ -39,7 +39,12 @@ extern "C" {
>>   #include <rte_compat.h>
>>   #include <rte_rwlock.h>
>> -#define RTE_FBARRAY_NAME_LEN 64
>> +/* Filename of fbarray is defined as a combination of several params
>> + * such as "fbarray_memseg-1048576k-0-0_PID_HOSTNAME".
>> + * The length of string before PID can be 32bytes, and the length of
>> + * PID can be 7bytes maximamly. Final 1 byte is for null terminator.
>> + */
>> +#define RTE_FBARRAY_NAME_LEN (32 + 7 + 1 + HOST_NAME_MAX + 1)
> 
> This breaks ABI, but i believe this doesn't break *stable* ABI, so it is 
> OK.
Thank you so much!

Yasufumi
> 
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 

Re: [dpdk-dev] [PATCH v8 1/1] fbarray: fix duplicated fbarray file in secondary

[Yasufumi Ogawa]

Hi,

Could I confirm that this patch is going to be merged in 20.02?

Regards,
Yasufumi

On 2019/12/06 19:44, Burakov, Anatoly wrote:
> On 27-Nov-19 8:48 AM, Yasufumi Ogawa wrote:
>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>>
>> In secondary_msl_create_walk(), it creates a file for fbarrays with its
>> PID for reserving unique name among secondary processes. However, it
>> does not work if several secondaries run as app containers because each
>> of containerized secondary has PID 1, and failed to reserve unique name
>> other than first one. To reserve unique name in each of containers, use
>> hostname in addition to PID.
>>
>> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
>> ---
>>   lib/librte_eal/common/include/rte_fbarray.h |  7 ++++++-
>>   lib/librte_eal/linux/eal/eal_memalloc.c     | 11 ++++++++---
>>   2 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/include/rte_fbarray.h 
>> b/lib/librte_eal/common/include/rte_fbarray.h
>> index 6dccdbec9..df003b8dc 100644
>> --- a/lib/librte_eal/common/include/rte_fbarray.h
>> +++ b/lib/librte_eal/common/include/rte_fbarray.h
>> @@ -39,7 +39,12 @@ extern "C" {
>>   #include <rte_compat.h>
>>   #include <rte_rwlock.h>
>> -#define RTE_FBARRAY_NAME_LEN 64
>> +/* Filename of fbarray is defined as a combination of several params
>> + * such as "fbarray_memseg-1048576k-0-0_PID_HOSTNAME".
>> + * The length of string before PID can be 32bytes, and the length of
>> + * PID can be 7bytes maximamly. Final 1 byte is for null terminator.
>> + */
>> +#define RTE_FBARRAY_NAME_LEN (32 + 7 + 1 + HOST_NAME_MAX + 1)
> 
> This breaks ABI, but i believe this doesn't break *stable* ABI, so it is 
> OK.
> 
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 

Re: [dpdk-dev] [PATCH v8 1/1] fbarray: fix duplicated fbarray file in secondary

[David Marchand]

On Fri, Feb 14, 2020 at 8:46 AM Yasufumi Ogawa <yasufum.o@gmail.com> wrote:
>
> Hi,
>
> Could I confirm that this patch is going to be merged in 20.02?

Sorry, but I can't take this patch in 20.02.
It breaks compilation on FreeBSD.
http://mails.dpdk.org/archives/test-report/2019-November/109435.html


I am still unconvinced on the need to change the size to something so
huge to accommodate with this new use case (secondary processes in
containers).
Why can't we truncate the container hostname so that it fits in 64 bytes?


Thomas, opinion?

-- 
David Marchand

Re: [dpdk-dev] [PATCH v8 1/1] fbarray: fix duplicated fbarray file in secondary

[Thomas Monjalon]

14/02/2020 16:08, David Marchand:
> On Fri, Feb 14, 2020 at 8:46 AM Yasufumi Ogawa <yasufum.o@gmail.com> wrote:
> >
> > Hi,
> >
> > Could I confirm that this patch is going to be merged in 20.02?
> 
> Sorry, but I can't take this patch in 20.02.
> It breaks compilation on FreeBSD.
> http://mails.dpdk.org/archives/test-report/2019-November/109435.html
> 
> 
> I am still unconvinced on the need to change the size to something so
> huge to accommodate with this new use case (secondary processes in
> containers).
> Why can't we truncate the container hostname so that it fits in 64 bytes?
> 
> 
> Thomas, opinion?

If the use case is justified enough, I would prefer merging such change in
20.11 avoiding an ABI breakage in a core library, even if it is experimental.

Re: [dpdk-dev] [PATCH v8 1/1] fbarray: fix duplicated fbarray file in secondary

[Yasufumi Ogawa]

> 14/02/2020 16:08, David Marchand:
>> On Fri, Feb 14, 2020 at 8:46 AM Yasufumi Ogawa <yasufum.o@gmail.com> wrote:
>>>
>>> Hi,
>>>
>>> Could I confirm that this patch is going to be merged in 20.02?
>>
>> Sorry, but I can't take this patch in 20.02.
>> It breaks compilation on FreeBSD.
>> http://mails.dpdk.org/archives/test-report/2019-November/109435.html
Sorry. I didn't find it. I'd see it.

>>
>>
>> I am still unconvinced on the need to change the size to something so
>> huge to accommodate with this new use case (secondary processes in
>> containers).
It is not so common actually, but serious issue for some NFV usecases. I 
remember, in a talk in the last DPDK summit, ZTE was also suffered from 
the same problem.

>> Why can't we truncate the container hostname so that it fits in 64 bytes?
It is just a possible maximum length of format of 
"fbarray_memseg-1048576k-0-0_PID_HOSTNAME", so I think it can be 
truncated if dropping some information.

>>
>>
>> Thomas, opinion?
> 
> If the use case is justified enough, I would prefer merging such change in
> 20.11 avoiding an ABI breakage in a core library, even if it is experimental.I understand, thanks.

Yasufumi

Re: [dpdk-dev] [PATCH] fbarray: get fbarrays from containerized secondary

[Stephen Hemminger]

On Tue, 16 Apr 2019 10:59:12 +0900
ogawa.yasufumi@lab.ntt.co.jp wrote:

> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> 
> In secondary_msl_create_walk(), it creates a file for fbarrays with its
> PID for reserving unique name among secondary processes. However, it
> does not work as expected if secondary is run as app container becuase
> each of containerized secondary has PID 1. To reserve unique name, use
> hostname instead of PID if the value is 1.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
> ---

Since this is an ABI break. I propose that a more invasive solution
would be better. Either change to using something more unique GUID
or change the fbarray structure to be a variable length array.
The internals of fbarray should also be hidden (ie not in rte_fbarray.h)
and the init() function changed into something that allocates the array.

The current patch has not gotten any followup or acceptance in 4 years.
So marking it as changes requested.