[v3,1/1] fbarray: get fbarrays from containerized 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 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
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?
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
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?).
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
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
>
On 12-Jul-19 3:22 AM, Yasufumi Ogawa wrote:
> 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
>
Yes, that should be OK.
On 22-Jul-19 2:06 AM, Ogawa Yasufumi wrote:
>
>
> 2019年7月12日(金) 11:22 Yasufumi Ogawa <yasufum.o@gmail.com
> <mailto: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
> <mailto:yasufum.o@gmail.com> wrote:
> >>>> From: Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp
> <mailto: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.
Yes, please do.
As a side note, you don't need to ask anyone's permission to send a patch :)
>
>
> Yasufumi
>
@@ -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,