Re: [PATCH v2] eal: allow worker lcore stacks to be allocated from hugepage memory

[fengchengwen <fengchengwen@huawei.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?