From: fengchengwen <fengchengwen@huawei.com>
To: Don Wallwork <donw@xsightlabs.com>, <dev@dpdk.org>
Cc: <stephen@networkplumber.org>, <mb@smartsharesystems.com>,
<anatoly.burakov@intel.com>, <dmitry.kozliuk@gmail.com>,
<bruce.richardson@intel.com>, <Honnappa.Nagarahalli@arm.com>,
<nd@arm.com>, <haiyue.wang@intel.com>
Subject: Re: [PATCH v2] eal: allow worker lcore stacks to be allocated from hugepage memory
Date: Sat, 14 May 2022 11:31:20 +0800 [thread overview]
Message-ID: <28354e90-57a1-9e30-4b4b-43d639c5f9bc@huawei.com> (raw)
In-Reply-To: <20220513175822.69905-1-donw@xsightlabs.com>
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?
next prev parent reply other threads:[~2022-05-14 3:31 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-02 14:10 [PATCH] " Don Wallwork
2022-05-03 6:10 ` Morten Brørup
2022-05-03 13:08 ` Wang, Haiyue
2022-05-03 19:46 ` Don Wallwork
2022-05-04 3:08 ` Wang, Haiyue
2022-05-13 17:58 ` [PATCH v2] " Don Wallwork
2022-05-13 21:38 ` Stephen Hemminger
2022-05-16 19:43 ` Don Wallwork
2022-05-13 21:41 ` Stephen Hemminger
2022-05-14 3:31 ` fengchengwen [this message]
2022-05-16 19:47 ` Don Wallwork
2022-05-17 6:28 ` Morten Brørup
2022-05-16 19:50 ` [PATCH v3] " Don Wallwork
2022-05-16 20:28 ` Stephen Hemminger
2022-05-16 20:29 ` Don Wallwork
2022-05-17 15:31 ` [PATCH v4] " Don Wallwork
2022-05-17 15:56 ` Stephen Hemminger
2022-05-18 14:10 ` Don Wallwork
2022-05-20 8:30 ` fengchengwen
2022-05-23 22:35 ` Kathleen Capella
2022-05-24 13:48 ` Don Wallwork
2022-05-24 14:40 ` Burakov, Anatoly
2022-05-24 19:38 ` Don Wallwork
2022-05-24 19:46 ` [PATCH v5] " Don Wallwork
2022-05-24 19:51 ` [PATCH v6] " Don Wallwork
2022-06-01 0:05 ` Kathleen Capella
2022-06-20 8:35 ` David Marchand
2022-06-21 10:37 ` Thomas Monjalon
2022-06-21 12:31 ` Don Wallwork
2022-06-21 14:42 ` Thomas Monjalon
2022-06-21 14:52 ` Don Wallwork
2022-06-21 15:00 ` Thomas Monjalon
2022-06-21 16:32 ` Honnappa Nagarahalli
2022-06-21 19:33 ` David Marchand
2022-06-23 11:21 ` [PATCH v7] " Don Wallwork
2022-06-23 20:32 ` David Marchand
Reply instructions:
You may reply publicly 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=28354e90-57a1-9e30-4b4b-43d639c5f9bc@huawei.com \
--to=fengchengwen@huawei.com \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=anatoly.burakov@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=dmitry.kozliuk@gmail.com \
--cc=donw@xsightlabs.com \
--cc=haiyue.wang@intel.com \
--cc=mb@smartsharesystems.com \
--cc=nd@arm.com \
--cc=stephen@networkplumber.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).