Re: [dpdk-dev] [PATCH v13 09/10] eal: add EAL argument for setting thread priority

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

2021-08-03 12:01 (UTC-0700), Narcisa Ana Maria Vasile:
> From: Narcisa Vasile <navasile@microsoft.com>
> 
> Allow the user to choose the thread priority through an EAL
> command line argument.

EAL options documentation update is needed.
With current wording the user may ask: the priority of which thread?

> The user can choose thread priority through an EAL parameter,
> when starting an application.  If EAL parameter is not used,
> the per-platform default value for thread priority is used.
> Otherwise administrator has an option to set one of available options:
>  --thread-prio normal
>  --thread-prio realtime
> 
>  Example:
> ./dpdk-l2fwd -l 0-3 -n 4 --thread-prio normal -- -q 8 -p ffff

IIUC, with this patch --thread-prio affects nothing,
because the value it fills is not used anywhere.
Why is it needed in this patchset then?

> 
> Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> ---
>  lib/eal/common/eal_common_options.c | 28 +++++++++++++++++++++++++++-
>  lib/eal/common/eal_internal_cfg.h   |  2 ++
>  lib/eal/common/eal_options.h        |  2 ++
>  3 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
> index ff5861b5f3..9d29696b84 100644
> --- a/lib/eal/common/eal_common_options.c
> +++ b/lib/eal/common/eal_common_options.c
> @@ -107,6 +107,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_THREAD_PRIORITY,   1, NULL, OPT_THREAD_PRIORITY_NUM},
>  
>  	/* legacy options that will be removed in future */
>  	{OPT_PCI_BLACKLIST,     1, NULL, OPT_PCI_BLACKLIST_NUM    },
> @@ -1412,6 +1413,24 @@ eal_parse_simd_bitwidth(const char *arg)
>  	return 0;
>  }
>  
> +static int
> +eal_parse_thread_priority(const char *arg)
> +{
> +	struct internal_config *internal_conf =
> +		eal_get_internal_configuration();
> +	enum rte_thread_priority priority;
> +
> +	if (!strncmp("normal", arg, sizeof("normal")))
> +		priority = RTE_THREAD_PRIORITY_NORMAL;
> +	else if (!strncmp("realtime", arg, sizeof("realtime")))
> +		priority = RTE_THREAD_PRIORITY_REALTIME_CRITICAL;
> +	else
> +		return -1;

Why not just `strcmp()`?

> +
> +	internal_conf->thread_priority = priority;
> +	return 0;
> +}
> +
>  static int
>  eal_parse_base_virtaddr(const char *arg)
>  {
> @@ -1825,7 +1844,13 @@ eal_parse_common_option(int opt, const char *optarg,
>  			return -1;
>  		}
>  		break;
> -
> +	case OPT_THREAD_PRIORITY_NUM:
> +		if (eal_parse_thread_priority(optarg) < 0) {
> +			RTE_LOG(ERR, EAL, "invalid parameter for --"
> +					OPT_THREAD_PRIORITY "\n");
> +			return -1;
> +		}
> +		break;
>  	/* don't know what to do, leave this to caller */
>  	default:
>  		return 1;
> @@ -2088,6 +2113,7 @@ eal_common_usage(void)
>  	       "                      (can be used multiple times)\n"
>  	       "  --"OPT_VMWARE_TSC_MAP"    Use VMware TSC map instead of native RDTSC\n"
>  	       "  --"OPT_PROC_TYPE"         Type of this process (primary|secondary|auto)\n"
> +	       "  --"OPT_THREAD_PRIORITY"   Set threads priority (normal|realtime)\n"
>  #ifndef RTE_EXEC_ENV_WINDOWS
>  	       "  --"OPT_SYSLOG"            Set syslog facility\n"
>  #endif
> diff --git a/lib/eal/common/eal_internal_cfg.h b/lib/eal/common/eal_internal_cfg.h
> index d6c0470eb8..b2996cd65b 100644
> --- a/lib/eal/common/eal_internal_cfg.h
> +++ b/lib/eal/common/eal_internal_cfg.h
> @@ -94,6 +94,8 @@ struct internal_config {
>  	unsigned int no_telemetry; /**< true to disable Telemetry */
>  	struct simd_bitwidth max_simd_bitwidth;
>  	/**< max simd bitwidth path to use */
> +	enum rte_thread_priority thread_priority;
> +	/**< thread priority to configure */

Where is the default set?
If you remove RTE_THREAD_PRIORITY_UNDEFINED in patch 2/10,
it will be RTE_THREAD_PRIORITY_NORMAL in zeroed-out structure, which is OK.

>  };
>  
>  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 7b348e707f..9f5b209f64 100644
> --- a/lib/eal/common/eal_options.h
> +++ b/lib/eal/common/eal_options.h
> @@ -93,6 +93,8 @@ enum {
>  	OPT_NO_TELEMETRY_NUM,
>  #define OPT_FORCE_MAX_SIMD_BITWIDTH  "force-max-simd-bitwidth"
>  	OPT_FORCE_MAX_SIMD_BITWIDTH_NUM,
> +#define OPT_THREAD_PRIORITY          "thread-prio"
> +	OPT_THREAD_PRIORITY_NUM,
>  
>  	/* legacy option that will be removed in future */
>  #define OPT_PCI_BLACKLIST     "pci-blacklist"