[v8,1/1] fbarray: fix duplicated fbarray file in secondary
Checks
Commit Message
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(-)
Comments
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>
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>
>
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>
>
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?
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.
> 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
@@ -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 */
@@ -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,