[v3,1/1] fbarray: get fbarrays from containerized secondary

Message ID 20190711103148.9187-2-yasufum.o@gmail.com
State New
Delegated to: Thomas Monjalon
Headers show
Series
  • fbarray: get fbarrays from containerized secondary
Related show

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/checkpatch success coding style OK

Commit Message

Ogawa Yasufumi July 11, 2019, 10:31 a.m.
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(-)

Comments

Burakov, Anatoly July 11, 2019, 10:53 a.m. | #1
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?
Ogawa Yasufumi July 11, 2019, 11:57 a.m. | #2
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
Burakov, Anatoly July 11, 2019, 1:14 p.m. | #3
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?).
Ogawa Yasufumi July 12, 2019, 2:22 a.m. | #4
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
Ogawa Yasufumi July 22, 2019, 1:06 a.m. | #5
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
>

Patch

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,