| Message ID | 20220513175822.69905-1-donw@xsightlabs.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Thomas Monjalon |
| Headers | show |
| Series | [v2] eal: allow worker lcore stacks to be allocated from hugepage memory | expand |
| Context | Check | Description |
|---|---|---|
| ci/iol-x86_64-unit-testing | success | Testing PASS |
| ci/iol-aarch64-compile-testing | success | Testing PASS |
| ci/iol-x86_64-compile-testing | fail | Testing issues |
| ci/iol-aarch64-unit-testing | success | Testing PASS |
| ci/iol-abi-testing | success | Testing PASS |
| ci/github-robot: build | fail | github build: failed |
| ci/iol-intel-Performance | success | Performance Testing PASS |
| ci/iol-intel-Functional | success | Functional Testing PASS |
| ci/intel-Testing | success | Testing PASS |
| ci/Intel-compilation | success | Compilation OK |
| ci/checkpatch | success | coding style OK |
On Fri, 13 May 2022 13:58:22 -0400 Don Wallwork <donw@xsightlabs.com> wrote: > +static int > +eal_parse_huge_worker_stack(const char *arg, size_t *huge_worker_stack_size) > +{ > + size_t worker_stack_size; > + if (arg == NULL) { > + *huge_worker_stack_size = USE_OS_STACK_SIZE; > + return 0; > + } > + worker_stack_size = atoi(arg); > + if (worker_stack_size == 0) > + return -1; > + Since worker_stack_size is size_t you are better off using something like strtoul() and check for more errors from that
On Fri, 13 May 2022 13:58:22 -0400 Don Wallwork <donw@xsightlabs.com> wrote: > + 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; > + } The indentation is getting kind of deep here, and to me that indicates a good place to split this into a helper function?
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 <donw@xsightlabs.com> > Acked-by: Morten Brørup <mb@smartsharesystems.com> > --- > 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?
On 5/13/2022 5:38 PM, Stephen Hemminger wrote: > On Fri, 13 May 2022 13:58:22 -0400 > Don Wallwork <donw@xsightlabs.com> wrote: > >> +static int >> +eal_parse_huge_worker_stack(const char *arg, size_t *huge_worker_stack_size) >> +{ >> + size_t worker_stack_size; >> + if (arg == NULL) { >> + *huge_worker_stack_size = USE_OS_STACK_SIZE; >> + return 0; >> + } >> + worker_stack_size = atoi(arg); >> + if (worker_stack_size == 0) >> + return -1; >> + > Since worker_stack_size is size_t you are better off using something > like strtoul() and check for more errors from that This and your other comment are addressed in the v3 patch.
On 5/13/2022 11:31 PM, fengchengwen wrote:
> Also, this patch seem only adapt linux, what about freebsd/windows?
The intent was to add this support for this optimization for Linix only
initially. Support for other OSes can be added later. I currently
don't have a means to test the other environments.
All of your other comments are addressed by the v3 patch.
> From: Don Wallwork [mailto:donw@xsightlabs.com] > Sent: Monday, 16 May 2022 21.48 > > On 5/13/2022 11:31 PM, fengchengwen wrote: > > Also, this patch seem only adapt linux, what about freebsd/windows? > > The intent was to add this support for this optimization for Linix only > initially. Support for other OSes can be added later. I currently > don't have a means to test the other environments. I agree with this approach. And it should suffice if the EAL aborts with an error message if the OPT_HUGE_WORKER_STACK parameter is used in a non-supported environment.
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 + 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) { + *huge_worker_stack_size = USE_OS_STACK_SIZE; + return 0; + } + worker_stack_size = atoi(arg); + if (worker_stack_size == 0) + return -1; + + *huge_worker_stack_size = worker_stack_size * 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) + /** * 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 */ }; 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");