[v4,1/1] fbarray: get fbarrays from containerized secondary
diff mbox series

Message ID 20190724082031.45546-2-yasufum.o@gmail.com
State Superseded
Delegated to: David Marchand
Headers show
Series
  • fbarray: get fbarrays from containerized secondary
Related show

Checks

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

Commit Message

Yasufumi Ogawa July 24, 2019, 8:20 a.m. UTC
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 24, 2019, 9:59 a.m. UTC | #1
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>
Thomas Monjalon July 30, 2019, 8:16 a.m. UTC | #2
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?
Burakov, Anatoly July 30, 2019, 9:18 a.m. UTC | #3
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.
Yasufumi Ogawa July 31, 2019, 5:48 a.m. UTC | #4
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.
David Marchand Oct. 11, 2019, 9:36 a.m. UTC | #5
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 Oct. 25, 2019, 3:36 p.m. UTC | #6
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,
Yasufumi Ogawa Oct. 25, 2019, 7:54 p.m. UTC | #7
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,
> 
>
David Marchand Oct. 26, 2019, 4:15 p.m. UTC | #8
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
Yasufumi Ogawa Oct. 26, 2019, 6:11 p.m. UTC | #9
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
>

Patch
diff mbox series

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,