DPDK patches and discussions
 help / color / Atom feed
From: David Marchand <david.marchand@redhat.com>
To: "Burakov, Anatoly" <anatoly.burakov@intel.com>,
	Yasufumi Ogawa <yasufum.o@gmail.com>
Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	dev <dev@dpdk.org>,  dpdk stable <stable@dpdk.org>,
	Yasufumi Ogawa <ogawa.yasufumi@lab.ntt.co.jp>
Subject: Re: [dpdk-dev] [PATCH v6 1/1] fbarray: fix duplicated fbarray file in secondary
Date: Tue, 5 Nov 2019 11:13:57 +0100
Message-ID: <CAJFAV8xPPu8kDNcjUycvWKjUBc3YuOT_ZQ_aDjrD4-yW3JgiEg@mail.gmail.com> (raw)
In-Reply-To: <05605cac-693e-042a-a3cb-6506b1ec653e@intel.com>

Hello Anatoly, Yasufumi,

On Mon, Nov 4, 2019 at 11:20 AM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 01-Nov-19 9:04 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 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

We can't backport this as is, see below.


> >
> > Signed-off-by: Yasufumi Ogawa <yasufum.o@gmail.com>
> > ---
> >   lib/librte_eal/common/include/rte_fbarray.h |  2 +-
> >   lib/librte_eal/linux/eal/eal_memalloc.c     | 11 ++++++++---
> >   2 files changed, 9 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..5c2815093 100644
> > --- a/lib/librte_eal/common/include/rte_fbarray.h
> > +++ b/lib/librte_eal/common/include/rte_fbarray.h
> > @@ -39,7 +39,7 @@ extern "C" {
> >   #include <rte_compat.h>
> >   #include <rte_rwlock.h>
> >
> > -#define RTE_FBARRAY_NAME_LEN 64
> > +#define RTE_FBARRAY_NAME_LEN NAME_MAX

The change on RTE_FBARRAY_NAME_LEN breaks the ABI, so we cannot
backport this as is.
For 19.11, we can allow this breakage, but we need an update of the
release notes.

Besides, what is the impact in terms of memory consumption?


> >
> >   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..24f0275c9 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] = { 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, HOST_NAME_MAX);

Personal preference, s/HOST_NAME_MAX/sizeof(hostname)/.


hostname[] is HOST_NAME_MAX bytes long.
In the worst case, we can get a non NULL terminated hostname string.
"
       gethostname() returns the null-terminated hostname in the
character array name, which has a length of len bytes.  If the
null-terminated hostname is too large to fit, then the name is
truncated, and
       no error is returned (but see NOTES below).  POSIX.1-2001 says
that if such truncation occurs, then it is unspecified whether the
returned buffer includes a terminating null byte.
...
NOTES
       SUSv2 guarantees that "Host names are limited to 255 bytes".
POSIX.1-2001 guarantees that "Host names (not including the
terminating null byte) are  limited  to  HOST_NAME_MAX  bytes".   On
Linux,
       HOST_NAME_MAX is defined with the value 64, which has been the
limit since Linux 1.0 (earlier kernels imposed a limit of 8 bytes).
"

How about making hostname[] HOST_NAME_MAX+1 bytes long?

> > +     snprintf(name, RTE_FBARRAY_NAME_LEN, "%s_%s_%d",
> > +                     primary_msl->memseg_arr.name, hostname, (int)getpid());
> >
> >       ret = rte_fbarray_init(&local_msl->memseg_arr, name,
> >               primary_msl->memseg_arr.len,
> >
>
> I think the order should be reversed. Both containers and non-containers
> can have their hostname set, and RTE_FBARRAY_NAME_LEN is of fairly
> limited length, so if the hostname is long enough, the PID never gets
> into the name string, resulting in duplicates. It is better have pid first.

Anatoly,

On the principle, it seems better, yes.
Just the comment on RTE_FBARRAY_NAME_LEN indicates that you missed the
change at the top of the patch.
What do you think of this change?


-- 
David Marchand


  reply index

Thread overview: 65+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-16  1:59 [dpdk-dev] [PATCH] fbarray: get fbarrays from containerized secondary ogawa.yasufumi
2019-04-16  1:59 ` ogawa.yasufumi
2019-04-16  3:43 ` [dpdk-dev] [PATCH v2 0/1] Get " ogawa.yasufumi
2019-04-16  3:43   ` ogawa.yasufumi
2019-04-16  3:43   ` [dpdk-dev] [PATCH v2 1/1] fbarray: get " ogawa.yasufumi
2019-04-16  3:43     ` ogawa.yasufumi
2019-07-04 20:17     ` Thomas Monjalon
2019-07-05  8:53     ` Burakov, Anatoly
2019-07-09 10:22       ` Yasufumi Ogawa
2019-07-09 10:24         ` Burakov, Anatoly
2019-07-09 10:26           ` Burakov, Anatoly
2019-07-11  9:37             ` Yasufumi Ogawa
2019-07-11  9:43               ` Burakov, Anatoly
2019-07-11 10:31 ` [dpdk-dev] [PATCH v3 0/1] " yasufum.o
2019-07-11 10:31   ` [dpdk-dev] [PATCH v3 1/1] " yasufum.o
2019-07-11 10:53     ` Burakov, Anatoly
2019-07-11 11:57       ` Yasufumi Ogawa
2019-07-11 13:14         ` Burakov, Anatoly
2019-07-12  2:22           ` Yasufumi Ogawa
2019-07-22  1:06             ` Ogawa Yasufumi
2019-07-22  9:33               ` Burakov, Anatoly
2019-07-22  9:25             ` Burakov, Anatoly
2019-07-24  8:20   ` [dpdk-dev] [PATCH v4 0/1] " yasufum.o
2019-07-24  8:20     ` [dpdk-dev] [PATCH v4 1/1] " yasufum.o
2019-07-24  9:59       ` Burakov, Anatoly
2019-07-30  8:16         ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2019-07-30  9:18           ` Burakov, Anatoly
2019-07-31  5:48             ` Yasufumi Ogawa
2019-10-11  9:36       ` [dpdk-dev] " David Marchand
2019-10-25 15:36         ` David Marchand
2019-10-25 19:54           ` Yasufumi Ogawa
2019-10-26 16:15             ` David Marchand
2019-10-26 18:11               ` Yasufumi Ogawa
2019-10-28  8:07     ` [dpdk-dev] [PATCH v5 0/1] fbarray: fix duplicated fbarray file in secondary yasufum.o
2019-10-28  8:07       ` [dpdk-dev] [PATCH v5 1/1] " yasufum.o
2019-10-29 12:03         ` Ananyev, Konstantin
2019-10-30 13:42           ` Yasufumi Ogawa
2019-10-30 19:00             ` Ananyev, Konstantin
2019-10-31 10:03               ` Yasufumi Ogawa
2019-10-31 10:32                 ` Ananyev, Konstantin
2019-11-01  9:04     ` [dpdk-dev] [PATCH v6 0/1] " yasufum.o
2019-11-01  9:04       ` [dpdk-dev] [PATCH v6 1/1] " yasufum.o
2019-11-01 12:01         ` Ananyev, Konstantin
2019-11-04 10:20         ` Burakov, Anatoly
2019-11-05 10:13           ` David Marchand [this message]
2019-11-05 11:31             ` Burakov, Anatoly
2019-11-05 11:41               ` Ananyev, Konstantin
2019-11-06 10:37                 ` Burakov, Anatoly
2019-11-08  3:19                   ` Yasufumi Ogawa
2019-11-13 21:43     ` [dpdk-dev] [PATCH v7 0/1] " yasufum.o
2019-11-13 21:43       ` [dpdk-dev] [PATCH v7 1/1] " yasufum.o
2019-11-14 10:01         ` Burakov, Anatoly
2019-11-14 11:42           ` Yasufumi Ogawa
2019-11-14 12:27             ` David Marchand
2019-11-26 19:40               ` Yasufumi Ogawa
2019-11-27 10:26                 ` Burakov, Anatoly
2019-11-29  5:44                   ` Yasufumi Ogawa
2019-12-02 10:43                     ` Burakov, Anatoly
2019-12-05 20:13                       ` Yasufumi Ogawa
2019-11-14 12:55         ` David Marchand
2019-11-14 17:32         ` Ananyev, Konstantin
2019-11-27  8:48       ` [dpdk-dev] [PATCH v8 0/1] " Yasufumi Ogawa
2019-11-27  8:48         ` [dpdk-dev] [PATCH v8 1/1] " Yasufumi Ogawa
2019-12-06 10:44           ` Burakov, Anatoly
2019-12-06 13:18             ` Yasufumi Ogawa

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAJFAV8xPPu8kDNcjUycvWKjUBc3YuOT_ZQ_aDjrD4-yW3JgiEg@mail.gmail.com \
    --to=david.marchand@redhat.com \
    --cc=anatoly.burakov@intel.com \
    --cc=dev@dpdk.org \
    --cc=konstantin.ananyev@intel.com \
    --cc=ogawa.yasufumi@lab.ntt.co.jp \
    --cc=stable@dpdk.org \
    --cc=yasufum.o@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox