From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id AB5A4A0353; Tue, 5 Nov 2019 12:31:33 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7F3C3378E; Tue, 5 Nov 2019 12:31:33 +0100 (CET) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 572442C08; Tue, 5 Nov 2019 12:31:31 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Nov 2019 03:31:30 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.68,271,1569308400"; d="scan'208";a="223387418" Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.92]) ([10.237.220.92]) by FMSMGA003.fm.intel.com with ESMTP; 05 Nov 2019 03:31:28 -0800 To: David Marchand , Yasufumi Ogawa Cc: "Ananyev, Konstantin" , dev , dpdk stable , Yasufumi Ogawa References: <20190724082031.45546-1-yasufum.o@gmail.com> <20191101090413.17997-1-yasufum.o@gmail.com> <20191101090413.17997-2-yasufum.o@gmail.com> <05605cac-693e-042a-a3cb-6506b1ec653e@intel.com> From: "Burakov, Anatoly" Message-ID: Date: Tue, 5 Nov 2019 11:31:28 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 1/1] fbarray: fix duplicated fbarray file in secondary X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 05-Nov-19 10:13 AM, David Marchand wrote: > Hello Anatoly, Yasufumi, > > On Mon, Nov 4, 2019 at 11:20 AM Burakov, Anatoly > wrote: >> >> On 01-Nov-19 9:04 AM, yasufum.o@gmail.com wrote: >>> From: Yasufumi Ogawa >>> >>> 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 >>> --- >>> 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 >>> #include >>> >>> -#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? > Yes, i did miss that, apologies. I don't have a strong opinion on this change, however the above comment would still be true if we make fbarray size to be hostname_max + 1 - we still potentially get no space for a pid. So if we're going to have pid in there as well, it should be hostname_max + pid_max (5 digits?) + whatever underscores we have + null terminator, to ensure it fits under any and all circumstances. Wrt memory usage, honestly, we don't live in a "640K should be enough for everyone" era any more. I don't see this being a major issue. This is not a hotpath, and we reserve half a terabyte of virtual memory at startup as it is. A few kilo/megabytes more isn't going to make much of a difference here. -- Thanks, Anatoly