From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id C8B3EA00C3; Sat, 14 May 2022 05:31:25 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 67EE5410E0; Sat, 14 May 2022 05:31:25 +0200 (CEST) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 821AF40395 for ; Sat, 14 May 2022 05:31:23 +0200 (CEST) Received: from dggpeml500024.china.huawei.com (unknown [172.30.72.53]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4L0WHd62DkzhZ6F; Sat, 14 May 2022 11:30:37 +0800 (CST) Received: from [127.0.0.1] (10.67.100.224) by dggpeml500024.china.huawei.com (7.185.36.10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Sat, 14 May 2022 11:31:20 +0800 Subject: Re: [PATCH v2] eal: allow worker lcore stacks to be allocated from hugepage memory To: Don Wallwork , CC: , , , , , , , References: <20220502141058.12707-1-donw@xsightlabs.com> <20220513175822.69905-1-donw@xsightlabs.com> From: fengchengwen Message-ID: <28354e90-57a1-9e30-4b4b-43d639c5f9bc@huawei.com> Date: Sat, 14 May 2022 11:31:20 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <20220513175822.69905-1-donw@xsightlabs.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.100.224] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To dggpeml500024.china.huawei.com (7.185.36.10) X-CFilter-Loop: Reflected X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 2022/5/14 1:58, Don Wallwork wrote: > Add support for using hugepages for worker lcore stack memory. The > intent is to improve performance by reducing stack memory related TLB > misses and also by using memory local to the NUMA node of each lcore. > > EAL option '--huge-worker-stack [stack-size-in-KiB]' is added to allow > the feature to be enabled at runtime. If the size is not specified, > the system pthread stack size will be used. > > Signed-off-by: Don Wallwork > Acked-by: Morten Brørup > --- > doc/guides/linux_gsg/eal_args.include.rst | 6 ++ > .../prog_guide/env_abstraction_layer.rst | 21 ++++++ > lib/eal/common/eal_common_options.c | 28 ++++++++ > lib/eal/common/eal_internal_cfg.h | 4 ++ > lib/eal/common/eal_options.h | 2 + > lib/eal/linux/eal.c | 65 ++++++++++++++++++- > 6 files changed, 124 insertions(+), 2 deletions(-) > > diff --git a/doc/guides/linux_gsg/eal_args.include.rst b/doc/guides/linux_gsg/eal_args.include.rst > index 3549a0cf56..d189109a55 100644 > --- a/doc/guides/linux_gsg/eal_args.include.rst > +++ b/doc/guides/linux_gsg/eal_args.include.rst > @@ -116,6 +116,12 @@ Memory-related options > > Force IOVA mode to a specific value. > > +* ``--huge-worker-stack[=size]`` > + > + Allocate worker stack memory from hugepage memory. Stack size defaults Two consecutive spaces befor 'Stack' ? > + to system pthread stack size unless the optional size (in kbytes) is > + specified. > + > Debugging options > ~~~~~~~~~~~~~~~~~ > > diff --git a/doc/guides/prog_guide/env_abstraction_layer.rst b/doc/guides/prog_guide/env_abstraction_layer.rst > index 5f0748fba1..e74516f0cf 100644 > --- a/doc/guides/prog_guide/env_abstraction_layer.rst > +++ b/doc/guides/prog_guide/env_abstraction_layer.rst > @@ -329,6 +329,27 @@ Another option is to use bigger page sizes. Since fewer pages are required to > cover the same memory area, fewer file descriptors will be stored internally > by EAL. > > +.. _huge-worker-stack: > + > +Hugepage Worker Stacks > +^^^^^^^^^^^^^^^^^^^^^^ > + > +When the ``--huge-worker-stack[=size]`` EAL option is specified, worker > +thread stacks are allocated from hugepage memory local to the NUMA node > +of the thread. Worker stack size defaults to system pthread stack size > +if the optional size parameter is not specified. > + > +.. warning:: > + Stacks allocated from hugepage memory are not protected by guard > + pages. Worker stacks must be sufficiently sized to prevent stack > + overflow when this option is used. > + > + As with normal thread stacks, hugepage worker thread stack size is > + fixed and is not dynamically resized. Therefore, an application that > + is free of stack page faults under a given load should be safe with > + hugepage worker thread stacks given the same thread stack size and > + loading conditions. > + > Support for Externally Allocated Memory > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c > index f247a42455..7fc5e10928 100644 > --- a/lib/eal/common/eal_common_options.c > +++ b/lib/eal/common/eal_common_options.c > @@ -103,6 +103,7 @@ eal_long_options[] = { > {OPT_TELEMETRY, 0, NULL, OPT_TELEMETRY_NUM }, > {OPT_NO_TELEMETRY, 0, NULL, OPT_NO_TELEMETRY_NUM }, > {OPT_FORCE_MAX_SIMD_BITWIDTH, 1, NULL, OPT_FORCE_MAX_SIMD_BITWIDTH_NUM}, > + {OPT_HUGE_WORKER_STACK, 2, NULL, OPT_HUGE_WORKER_STACK_NUM }, > > {0, 0, NULL, 0 } > }; > @@ -1618,6 +1619,22 @@ eal_parse_huge_unlink(const char *arg, struct hugepage_file_discipline *out) > return -1; > } > > +static int > +eal_parse_huge_worker_stack(const char *arg, size_t *huge_worker_stack_size) > +{ > + size_t worker_stack_size; > + if (arg == NULL) { Also consider arg[0] = '\0', maybe: if (arg == NULL || arg[0] == '\0') > + *huge_worker_stack_size = USE_OS_STACK_SIZE; > + return 0; > + } > + worker_stack_size = atoi(arg); Suggest use strtoul because atoi does not detect errors. also suggest check for convert error. Suggest refer eal_parse_simd_bitwidth() > + if (worker_stack_size == 0) > + return -1; > + > + *huge_worker_stack_size = worker_stack_size * 1024; Should consider overflow with multiple 1024 ? > + return 0; > +} > + > int > eal_parse_common_option(int opt, const char *optarg, > struct internal_config *conf) > @@ -1921,6 +1938,17 @@ eal_parse_common_option(int opt, const char *optarg, > } > break; > > +#ifndef RTE_EXEC_ENV_WINDOWS > + case OPT_HUGE_WORKER_STACK_NUM: > + if (eal_parse_huge_worker_stack(optarg, > + &conf->huge_worker_stack_size) < 0) { > + RTE_LOG(ERR, EAL, "invalid parameter for --" > + OPT_HUGE_WORKER_STACK"\n"); > + return -1; > + } > + break; > +#endif /* !RTE_EXEC_ENV_WINDOWS */ > + > /* don't know what to do, leave this to caller */ > default: > return 1; > diff --git a/lib/eal/common/eal_internal_cfg.h b/lib/eal/common/eal_internal_cfg.h > index b71faadd18..8ac710da02 100644 > --- a/lib/eal/common/eal_internal_cfg.h > +++ b/lib/eal/common/eal_internal_cfg.h > @@ -48,6 +48,9 @@ struct hugepage_file_discipline { > bool unlink_existing; > }; > > +/** Worker hugepage stack size should default to OS value. */ > +#define USE_OS_STACK_SIZE ((size_t)~0) the USE is verb, suggest HUGE_WORKER_STACK_DEFAULT_SIZE or HUGE_WORKER_STACK_DEFAULT_OS_SIZE > + > /** > * internal configuration > */ > @@ -102,6 +105,7 @@ struct internal_config { > unsigned int no_telemetry; /**< true to disable Telemetry */ > struct simd_bitwidth max_simd_bitwidth; > /**< max simd bitwidth path to use */ > + size_t huge_worker_stack_size; /**< worker thread stack size in KiB */ the huge_worker_stack_size already multi 1024, so it unit is byte not KiB. > }; > > void eal_reset_internal_config(struct internal_config *internal_cfg); > diff --git a/lib/eal/common/eal_options.h b/lib/eal/common/eal_options.h > index 8e4f7202a2..3cc9cb6412 100644 > --- a/lib/eal/common/eal_options.h > +++ b/lib/eal/common/eal_options.h > @@ -87,6 +87,8 @@ enum { > OPT_NO_TELEMETRY_NUM, > #define OPT_FORCE_MAX_SIMD_BITWIDTH "force-max-simd-bitwidth" > OPT_FORCE_MAX_SIMD_BITWIDTH_NUM, > +#define OPT_HUGE_WORKER_STACK "huge-worker-stack" > + OPT_HUGE_WORKER_STACK_NUM, > > OPT_LONG_MAX_NUM > }; > diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c > index 1ef263434a..e8c872ef7b 100644 > --- a/lib/eal/linux/eal.c > +++ b/lib/eal/linux/eal.c > @@ -1144,8 +1144,69 @@ rte_eal_init(int argc, char **argv) > lcore_config[i].state = WAIT; > > /* create a thread for each lcore */ > - ret = pthread_create(&lcore_config[i].thread_id, NULL, > - eal_thread_loop, (void *)(uintptr_t)i); > + if (internal_conf->huge_worker_stack_size == 0) { > + ret = pthread_create(&lcore_config[i].thread_id, NULL, > + eal_thread_loop, > + (void *)(uintptr_t)i); > + } else { > + /* Allocate NUMA aware stack memory and set > + * pthread attributes > + */ > + pthread_attr_t attr; > + size_t stack_size; > + void *stack_ptr; > + > + if (pthread_attr_init(&attr) != 0) { > + rte_eal_init_alert("Cannot init pthread " > + "attributes"); > + rte_errno = EFAULT; > + return -1; > + } > + if (internal_conf->huge_worker_stack_size == > + USE_OS_STACK_SIZE) { > + if (pthread_attr_getstacksize(&attr, > + &stack_size) != 0) { > + rte_errno = EFAULT; > + return -1; > + } > + } else { > + stack_size = > + internal_conf->huge_worker_stack_size; > + } > + stack_ptr = > + rte_zmalloc_socket("lcore_stack", > + stack_size, > + stack_size, > + rte_lcore_to_socket_id(i)); > + > + if (stack_ptr == NULL) { > + rte_eal_init_alert("Cannot allocate stack " > + "memory for worker lcore"); > + rte_errno = ENOMEM; > + return -1; > + } > + > + if (pthread_attr_setstack(&attr, > + stack_ptr, > + stack_size) != 0) { > + rte_eal_init_alert("Cannot set pthread " > + "stack attributes"); > + rte_errno = EFAULT; > + return -1; > + } > + > + /* create a thread for each lcore */ > + ret = pthread_create(&lcore_config[i].thread_id, &attr, > + eal_thread_loop, > + (void *)(uintptr_t)i); > + > + if (pthread_attr_destroy(&attr) != 0) { > + rte_eal_init_alert("Cannot destroy pthread " > + "attributes"); > + rte_errno = EFAULT; > + return -1; > + } > + } > if (ret != 0) > rte_panic("Cannot create thread\n"); it's recommended that the function be independent. > > Also, this patch seem only adapt linux, what about freebsd/windows?