From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 48AC64CBD for ; Wed, 21 Mar 2018 11:24:48 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Mar 2018 03:24:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,339,1517904000"; d="scan'208";a="40874168" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.112]) ([10.237.220.112]) by orsmga001.jf.intel.com with ESMTP; 21 Mar 2018 03:24:45 -0700 To: gowrishankar muthukrishnan Cc: dev@dpdk.org, Bruce Richardson , Chao Zhu References: <18deafea-5662-88ef-2ddc-3a1970d67405@linux.vnet.ibm.com> From: "Burakov, Anatoly" Message-ID: Date: Wed, 21 Mar 2018 10:24:44 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <18deafea-5662-88ef-2ddc-3a1970d67405@linux.vnet.ibm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 18.05 v4] eal: add function to return number of detected sockets 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: , X-List-Received-Date: Wed, 21 Mar 2018 10:24:48 -0000 On 21-Mar-18 4:59 AM, gowrishankar muthukrishnan wrote: > On Wednesday 07 February 2018 03:28 PM, Anatoly Burakov wrote: >> During lcore scan, find maximum socket ID and store it. This will >> break the ABI, so bump ABI version. >> >> Signed-off-by: Anatoly Burakov >> --- >> >> Notes: >>      v4: >>      - Remove backwards ABI compatibility, bump ABI instead >>      v3: >>      - Added ABI compatibility >>      v2: >>      - checkpatch changes >>      - check socket before deciding if the core is not to be used >> >>   lib/librte_eal/bsdapp/eal/Makefile        |  2 +- >>   lib/librte_eal/common/eal_common_lcore.c  | 37 >> +++++++++++++++++++++---------- >>   lib/librte_eal/common/include/rte_eal.h   |  1 + >>   lib/librte_eal/common/include/rte_lcore.h |  8 +++++++ >>   lib/librte_eal/linuxapp/eal/Makefile      |  2 +- >>   lib/librte_eal/rte_eal_version.map        |  9 +++++++- >>   6 files changed, 44 insertions(+), 15 deletions(-) >> >> diff --git a/lib/librte_eal/bsdapp/eal/Makefile >> b/lib/librte_eal/bsdapp/eal/Makefile >> index dd455e6..ed1d17b 100644 >> --- a/lib/librte_eal/bsdapp/eal/Makefile >> +++ b/lib/librte_eal/bsdapp/eal/Makefile >> @@ -21,7 +21,7 @@ LDLIBS += -lgcc_s >> >>   EXPORT_MAP := ../../rte_eal_version.map >> >> -LIBABIVER := 6 >> +LIBABIVER := 7 >> >>   # specific to bsdapp exec-env >>   SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) := eal.c >> diff --git a/lib/librte_eal/common/eal_common_lcore.c >> b/lib/librte_eal/common/eal_common_lcore.c >> index 7724fa4..827ddeb 100644 >> --- a/lib/librte_eal/common/eal_common_lcore.c >> +++ b/lib/librte_eal/common/eal_common_lcore.c >> @@ -28,6 +28,7 @@ rte_eal_cpu_init(void) >>       struct rte_config *config = rte_eal_get_configuration(); >>       unsigned lcore_id; >>       unsigned count = 0; >> +    unsigned int socket_id, max_socket_id = 0; >> >>       /* >>        * Parse the maximum set of logical cores, detect the subset of >> running >> @@ -39,6 +40,19 @@ rte_eal_cpu_init(void) >>           /* init cpuset for per lcore config */ >>           CPU_ZERO(&lcore_config[lcore_id].cpuset); >> >> +        /* find socket first */ >> +        socket_id = eal_cpu_socket_id(lcore_id); >> +        if (socket_id >= RTE_MAX_NUMA_NODES) { >> +#ifdef RTE_EAL_ALLOW_INV_SOCKET_ID >> +            socket_id = 0; >> +#else >> +            RTE_LOG(ERR, EAL, "Socket ID (%u) is greater than >> RTE_MAX_NUMA_NODES (%d)\n", >> +                    socket_id, RTE_MAX_NUMA_NODES); >> +            return -1; >> +#endif >> +        } >> +        max_socket_id = RTE_MAX(max_socket_id, socket_id); >> + >>           /* in 1:1 mapping, record related cpu detected state */ >>           lcore_config[lcore_id].detected = eal_cpu_detected(lcore_id); >>           if (lcore_config[lcore_id].detected == 0) { >> @@ -54,18 +68,7 @@ rte_eal_cpu_init(void) >>           config->lcore_role[lcore_id] = ROLE_RTE; >>           lcore_config[lcore_id].core_role = ROLE_RTE; >>           lcore_config[lcore_id].core_id = eal_cpu_core_id(lcore_id); >> -        lcore_config[lcore_id].socket_id = eal_cpu_socket_id(lcore_id); >> -        if (lcore_config[lcore_id].socket_id >= RTE_MAX_NUMA_NODES) { >> -#ifdef RTE_EAL_ALLOW_INV_SOCKET_ID >> -            lcore_config[lcore_id].socket_id = 0; >> -#else >> -            RTE_LOG(ERR, EAL, "Socket ID (%u) is greater than " >> -                "RTE_MAX_NUMA_NODES (%d)\n", >> -                lcore_config[lcore_id].socket_id, >> -                RTE_MAX_NUMA_NODES); >> -            return -1; >> -#endif >> -        } >> +        lcore_config[lcore_id].socket_id = socket_id; >>           RTE_LOG(DEBUG, EAL, "Detected lcore %u as " >>                   "core %u on socket %u\n", >>                   lcore_id, lcore_config[lcore_id].core_id, >> @@ -79,5 +82,15 @@ rte_eal_cpu_init(void) >>           RTE_MAX_LCORE); >>       RTE_LOG(INFO, EAL, "Detected %u lcore(s)\n", config->lcore_count); >> >> +    config->numa_node_count = max_socket_id + 1; > > In some IBM servers, socket ID number does not seem to be in sequence. > For an instance, 0 and 8 for a 2 node server. > > In this case, numa_node_count would mislead users if wrongly understood > by its variable name IMO (see below) >> +    RTE_LOG(INFO, EAL, "Detected %u NUMA nodes\n", >> config->numa_node_count); > > For an instance, reading above message would tell 'EAL detected 8 nodes' > in my server, but actually there are only two nodes. > > Could its name better be 'numa_node_id_max' ?. Also, we store in actual > count of numa nodes in _count variable. > > Also, there could be a case when there is no local memory available to a > numa node too. > > Thanks, > Gowrishankar The point of this patchset is to (pre)allocate memory only on existing sockets. If we don't know how many sockets there are, we are forced to preallocate VA space per each *possible* NUMA node - that is, reserve e.g. 8x128G of memory, 6 of which will go unused on a 2-socket system. We can't know if there is no memory on socket in advance, but we can at least avoid preallocating VA space for sockets that don't exist in the first place. How about we store all possible socket id's instead? e.g. something like: static int numa_node_ids[MAX_NUMA_NODES]; <...> int rte_eal_cpu_init() { int sockets[RTE_MAX_LCORE]; <...> for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) { core_to_socket[lcore_id] = socket; } <...> qsort(sockets); <...> // store all unique sockets in numa_node_ids in ascending order } <...> on a 2 socket system we then get: rte_num_sockets() => return 2 rte_get_socket_id(int idx) => return numa_node_ids[idx] Would that be suitable? -- Thanks, Anatoly