[v7,1/1] fbarray: fix duplicated fbarray file in secondary
diff mbox series

Message ID 20191113214346.33749-2-yasufum.o@gmail.com
State Superseded, archived
Delegated to: David Marchand
Headers show
Series
  • fbarray: fix duplicated fbarray file in secondary
Related show

Checks

Context Check Description
ci/travis-robot warning Travis build: failed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-compilation success Compile Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/checkpatch warning coding style issues

Commit Message

Yasufumi Ogawa Nov. 13, 2019, 9:43 p.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 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(-)

Comments

Burakov, Anatoly Nov. 14, 2019, 10:01 a.m. UTC | #1
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>
Yasufumi Ogawa Nov. 14, 2019, 11:42 a.m. UTC | #2
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>
>
David Marchand Nov. 14, 2019, 12:27 p.m. UTC | #3
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 Nov. 14, 2019, 12:55 p.m. UTC | #4
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.
Ananyev, Konstantin Nov. 14, 2019, 5:32 p.m. UTC | #5
> -----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
Yasufumi Ogawa Nov. 26, 2019, 7:40 p.m. UTC | #6
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

> 
>
Burakov, Anatoly Nov. 27, 2019, 10:26 a.m. UTC | #7
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.
Yasufumi Ogawa Nov. 29, 2019, 5:44 a.m. UTC | #8
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
Burakov, Anatoly Dec. 2, 2019, 10:43 a.m. UTC | #9
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
>
Yasufumi Ogawa Dec. 5, 2019, 8:13 p.m. UTC | #10
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
>>
> 
>

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 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,