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

  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(-)
  

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.