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

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

Checks

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

Commit Message

Yasufumi Ogawa Nov. 27, 2019, 8:48 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 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

Burakov, Anatoly Dec. 6, 2019, 10:44 a.m. UTC | #1
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>
Yasufumi Ogawa Dec. 6, 2019, 1:18 p.m. UTC | #2
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>
>
Yasufumi Ogawa Feb. 14, 2020, 7:46 a.m. UTC | #3
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>
>
David Marchand Feb. 14, 2020, 3:08 p.m. UTC | #4
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?
Thomas Monjalon Feb. 14, 2020, 3:29 p.m. UTC | #5
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.
Yasufumi Ogawa Feb. 17, 2020, 12:54 p.m. UTC | #6
> 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

Patch
diff mbox series

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,