Re: [dpdk-dev] [PATCH] eal: rename --lcores option to --threads

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Thomas,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, March 31, 2015 11:34 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] eal: rename --lcores option to --threads
> 
> Threads are no longer strictly assigned to a lcore with a 1:1 map.
> So they really should be called threads.
> The term lcore should be reserved for a physical hyperthreaded CPU part.
> 
> This patch only renames the option --lcores to avoid confusion from a
> user perspective. But a deeper cleanup should be done later in EAL:
> e.g. lcore_id should be renamed to thread_id.
> 

I don't see much point in such renaming.
As I remember, it was already discussed should we rename 'lcore' to 'lthread'
(or something similar) and the conclusion was to avoid that.
Such renaming would require too much changes across whole DPDK and external apps, and it seems no real benefit in doing that.    
It seems much easier to document the new notion of 'lcore' means. 
Again probably 'lcore' is not a best name, but it was there for a while, and people get used to it.
While 'thread' seems a bit overloaded to me - not clear what exactly we mean here:
is it an EAL thread, is it just a pthread the user can create on its will?   

Konstantin

> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> ---
>  app/test/test_eal_flags.c                       | 34 ++++-----
>  doc/guides/prog_guide/env_abstraction_layer.rst | 25 ++++---
>  doc/guides/testpmd_app_ug/run_app.rst           | 10 +--
>  lib/librte_eal/common/eal_common_options.c      | 96 ++++++++++++-------------
>  lib/librte_eal/common/eal_options.h             |  4 +-
>  5 files changed, 84 insertions(+), 85 deletions(-)
> 
> diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
> index 0352f87..1096bea 100644
> --- a/app/test/test_eal_flags.c
> +++ b/app/test/test_eal_flags.c
> @@ -512,7 +512,7 @@ test_missing_c_flag(void)
> 
>  	/* -c flag but no coremask value */
>  	const char *argv1[] = { prgname, prefix, mp_flag, "-n", "3", "-c"};
> -	/* No -c, -l or --lcores flag at all */
> +	/* No -c, -l or --threads flag at all */
>  	const char *argv2[] = { prgname, prefix, mp_flag, "-n", "3"};
>  	/* bad coremask value */
>  	const char *argv3[] = { prgname, prefix, mp_flag,
> @@ -538,37 +538,37 @@ test_missing_c_flag(void)
>  	const char *argv11[] = { prgname, prefix, mp_flag,
>  				 "-n", "3", "-l", "1-2,3" };
> 
> -	/* --lcores flag but no lcores value */
> +	/* --threads flag but no lcores value */
>  	const char *argv12[] = { prgname, prefix, mp_flag,
> -				 "-n", "3", "--lcores" };
> +				 "-n", "3", "--threads" };
>  	const char *argv13[] = { prgname, prefix, mp_flag,
> -				 "-n", "3", "--lcores", " " };
> +				 "-n", "3", "--threads", " " };
>  	/* bad lcores value */
>  	const char *argv14[] = { prgname, prefix, mp_flag,
> -				 "-n", "3", "--lcores", "1-3-5" };
> +				 "-n", "3", "--threads", "1-3-5" };
>  	const char *argv15[] = { prgname, prefix, mp_flag,
> -				 "-n", "3", "--lcores", "0-1,,2" };
> +				 "-n", "3", "--threads", "0-1,,2" };
>  	const char *argv16[] = { prgname, prefix, mp_flag,
> -				 "-n", "3", "--lcores", "0-,1" };
> +				 "-n", "3", "--threads", "0-,1" };
>  	const char *argv17[] = { prgname, prefix, mp_flag,
> -				 "-n", "3", "--lcores", "(0-,2-4)" };
> +				 "-n", "3", "--threads", "(0-,2-4)" };
>  	const char *argv18[] = { prgname, prefix, mp_flag,
> -				 "-n", "3", "--lcores", "(-1,2)" };
> +				 "-n", "3", "--threads", "(-1,2)" };
>  	const char *argv19[] = { prgname, prefix, mp_flag,
> -				 "-n", "3", "--lcores", "(2-4)@(2-4-6)" };
> +				 "-n", "3", "--threads", "(2-4)@(2-4-6)" };
>  	const char *argv20[] = { prgname, prefix, mp_flag,
> -				 "-n", "3", "--lcores", "(a,2)" };
> +				 "-n", "3", "--threads", "(a,2)" };
>  	const char *argv21[] = { prgname, prefix, mp_flag,
> -				 "-n", "3", "--lcores", "1-3@(1,3)" };
> +				 "-n", "3", "--threads", "1-3@(1,3)" };
>  	const char *argv22[] = { prgname, prefix, mp_flag,
> -				 "-n", "3", "--lcores", "3@((1,3)" };
> +				 "-n", "3", "--threads", "3@((1,3)" };
>  	const char *argv23[] = { prgname, prefix, mp_flag,
> -				 "-n", "3", "--lcores", "(4-7)=(1,3)" };
> +				 "-n", "3", "--threads", "(4-7)=(1,3)" };
>  	const char *argv24[] = { prgname, prefix, mp_flag,
> -				 "-n", "3", "--lcores", "[4-7]@(1,3)" };
> +				 "-n", "3", "--threads", "[4-7]@(1,3)" };
>  	/* sanity check of tests - valid lcores value */
>  	const char *argv25[] = { prgname, prefix, mp_flag,
> -				 "-n", "3", "--lcores",
> +				 "-n", "3", "--threads",
>  				 "0-1,2@(5-7),(3-5)@(0,2),(0,6),7"};
> 
>  	if (launch_proc(argv1) == 0
> @@ -601,7 +601,7 @@ test_missing_c_flag(void)
>  		return -1;
>  	}
> 
> -	/* start --lcores tests */
> +	/* start --threads tests */
>  	if (launch_proc(argv12) == 0 || launch_proc(argv13) == 0 ||
>  	    launch_proc(argv14) == 0 || launch_proc(argv15) == 0 ||
>  	    launch_proc(argv16) == 0 || launch_proc(argv17) == 0 ||
> diff --git a/doc/guides/prog_guide/env_abstraction_layer.rst b/doc/guides/prog_guide/env_abstraction_layer.rst
> index 1b531e2..8509774 100644
> --- a/doc/guides/prog_guide/env_abstraction_layer.rst
> +++ b/doc/guides/prog_guide/env_abstraction_layer.rst
> @@ -232,35 +232,34 @@ For further flexibility, it is useful to set pthread affinity not only to a CPU
>  EAL pthread and lcore Affinity
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> -The term "lcore" refers to an EAL thread, which is really a Linux/FreeBSD pthread.
>  "EAL pthreads"  are created and managed by EAL and execute the tasks issued by *remote_launch*.
>  In each EAL pthread, there is a TLS (Thread Local Storage) called *_lcore_id* for unique identification.
>  As EAL pthreads usually bind 1:1 to the physical CPU, the *_lcore_id* is typically equal to the CPU ID.
> 
>  When using multiple pthreads, however, the binding is no longer always 1:1 between an EAL pthread and a specified physical CPU.
>  The EAL pthread may have affinity to a CPU set, and as such the *_lcore_id* will not be the same as the CPU ID.
> -For this reason, there is an EAL long option '--lcores' defined to assign the CPU affinity of lcores.
> +For this reason, there is an EAL long option '--threads' defined to assign the CPU affinity of lcores.
>  For a specified lcore ID or ID group, the option allows setting the CPU set for that EAL pthread.
> 
>  The format pattern:
> -	--lcores='<lcore_set>[@cpu_set][,<lcore_set>[@cpu_set],...]'
> +	--threads='<thread_set>[@cpu_set][,<thread_set>[@cpu_set],...]'
> 
> -'lcore_set' and 'cpu_set' can be a single number, range or a group.
> +'thread_set' and 'cpu_set' can be a single number, range or a group.
> 
>  A number is a "digit([0-9]+)"; a range is "<number>-<number>"; a group is "(<number|range>[,<number|range>,...])".
> 
> -If a '\@cpu_set' value is not supplied, the value of 'cpu_set' will default to the value of 'lcore_set'.
> +If a '\@cpu_set' value is not supplied, the value of 'cpu_set' will default to the value of 'thread_set'.
> 
>      ::
> 
> -    	For example, "--lcores='1,2@(5-7),(3-5)@(0,2),(0,6),7-8'" which means start 9 EAL thread;
> -    	    lcore 0 runs on cpuset 0x41 (cpu 0,6);
> -    	    lcore 1 runs on cpuset 0x2 (cpu 1);
> -    	    lcore 2 runs on cpuset 0xe0 (cpu 5,6,7);
> -    	    lcore 3,4,5 runs on cpuset 0x5 (cpu 0,2);
> -    	    lcore 6 runs on cpuset 0x41 (cpu 0,6);
> -    	    lcore 7 runs on cpuset 0x80 (cpu 7);
> -    	    lcore 8 runs on cpuset 0x100 (cpu 8).
> +    	For example, "--threads='1,2@(5-7),(3-5)@(0,2),(0,6),7-8'" which means start 9 EAL thread;
> +    	    thread 0 runs on cpuset 0x41 (cpu 0,6);
> +    	    thread 1 runs on cpuset 0x2 (cpu 1);
> +    	    thread 2 runs on cpuset 0xe0 (cpu 5,6,7);
> +    	    thread 3,4,5 runs on cpuset 0x5 (cpu 0,2);
> +    	    thread 6 runs on cpuset 0x41 (cpu 0,6);
> +    	    thread 7 runs on cpuset 0x80 (cpu 7);
> +    	    thread 8 runs on cpuset 0x100 (cpu 8).
> 
>  Using this option, for each given lcore ID, the associated CPUs can be assigned.
>  It's also compatible with the pattern of corelist('-l') option.
> diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
> index 6cbbf40..830df56 100644
> --- a/doc/guides/testpmd_app_ug/run_app.rst
> +++ b/doc/guides/testpmd_app_ug/run_app.rst
> @@ -49,18 +49,18 @@ See the DPDK Getting Started Guide for more information on these options.
>      The argument format is <c1>[-c2][,c3[-c4],...]
>      where c1, c2, etc are core indexes between 0 and 128
> 
> -*   --lcores COREMAP
> +*   --threads COREMAP
> 
> -    Map lcore set to physical cpu set
> +    Map thread set to physical cpu set
> 
>      The argument format is
> -        '<lcores[@cpus]>[<,lcores[@cpus]>...]'
> +        '<threads[@cpus]>[<,threads[@cpus]>...]'
> 
> -    lcores and cpus list are grouped by '(' and ')'
> +    threads and cpus list are grouped by '(' and ')'
>      Within the group, '-' is used for range separator,
>      ',' is used for single number separator.
>      '( )' can be omitted for single element group,
> -    '@' can be omitted if cpus and lcores have the same value
> +    '@' can be omitted if cpus and threads have the same value
> 
>  *   --master-lcore ID
> 
> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index 8fcb1ab..68799d0 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -74,7 +74,6 @@ eal_long_options[] = {
>  	{OPT_FILE_PREFIX,       1, NULL, OPT_FILE_PREFIX_NUM      },
>  	{OPT_HELP,              0, NULL, OPT_HELP_NUM             },
>  	{OPT_HUGE_DIR,          1, NULL, OPT_HUGE_DIR_NUM         },
> -	{OPT_LCORES,            1, NULL, OPT_LCORES_NUM           },
>  	{OPT_LOG_LEVEL,         1, NULL, OPT_LOG_LEVEL_NUM        },
>  	{OPT_MASTER_LCORE,      1, NULL, OPT_MASTER_LCORE_NUM     },
>  	{OPT_NO_HPET,           0, NULL, OPT_NO_HPET_NUM          },
> @@ -86,6 +85,7 @@ eal_long_options[] = {
>  	{OPT_PROC_TYPE,         1, NULL, OPT_PROC_TYPE_NUM        },
>  	{OPT_SOCKET_MEM,        1, NULL, OPT_SOCKET_MEM_NUM       },
>  	{OPT_SYSLOG,            1, NULL, OPT_SYSLOG_NUM           },
> +	{OPT_THREADS,           1, NULL, OPT_THREADS_NUM          },
>  	{OPT_VDEV,              1, NULL, OPT_VDEV_NUM             },
>  	{OPT_VFIO_INTR,         1, NULL, OPT_VFIO_INTR_NUM        },
>  	{OPT_VMWARE_TSC_MAP,    0, NULL, OPT_VMWARE_TSC_MAP_NUM   },
> @@ -442,42 +442,42 @@ convert_to_cpuset(rte_cpuset_t *cpusetp,
>  }
> 
>  /*
> - * The format pattern: --lcores='<lcores[@cpus]>[<,lcores[@cpus]>...]'
> - * lcores, cpus could be a single digit/range or a group.
> + * The format pattern: --threads='<threads[@cpus]>[<,threads[@cpus]>...]'
> + * threads, cpus could be a single digit/range or a group.
>   * '(' and ')' are necessary if it's a group.
> - * If not supply '@cpus', the value of cpus uses the same as lcores.
> + * If not supply '@cpus', the value of cpus uses the same as threads.
>   * e.g. '1,2@(5-7),(3-5)@(0,2),(0,6),7-8' means start 9 EAL thread as below
> - *   lcore 0 runs on cpuset 0x41 (cpu 0,6)
> - *   lcore 1 runs on cpuset 0x2 (cpu 1)
> - *   lcore 2 runs on cpuset 0xe0 (cpu 5,6,7)
> - *   lcore 3,4,5 runs on cpuset 0x5 (cpu 0,2)
> - *   lcore 6 runs on cpuset 0x41 (cpu 0,6)
> - *   lcore 7 runs on cpuset 0x80 (cpu 7)
> - *   lcore 8 runs on cpuset 0x100 (cpu 8)
> + *   thread 0 runs on cpuset 0x41 (cpu 0,6)
> + *   thread 1 runs on cpuset 0x2 (cpu 1)
> + *   thread 2 runs on cpuset 0xe0 (cpu 5,6,7)
> + *   thread 3,4,5 runs on cpuset 0x5 (cpu 0,2)
> + *   thread 6 runs on cpuset 0x41 (cpu 0,6)
> + *   thread 7 runs on cpuset 0x80 (cpu 7)
> + *   thread 8 runs on cpuset 0x100 (cpu 8)
>   */
>  static int
> -eal_parse_lcores(const char *lcores)
> +eal_parse_threads(const char *threads)
>  {
>  	struct rte_config *cfg = rte_eal_get_configuration();
>  	static uint16_t set[RTE_MAX_LCORE];
>  	unsigned idx = 0;
>  	int i;
>  	unsigned count = 0;
> -	const char *lcore_start = NULL;
> +	const char *thread_start = NULL;
>  	const char *end = NULL;
>  	int offset;
>  	rte_cpuset_t cpuset;
>  	int lflags = 0;
>  	int ret = -1;
> 
> -	if (lcores == NULL)
> +	if (threads == NULL)
>  		return -1;
> 
>  	/* Remove all blank characters ahead and after */
> -	while (isblank(*lcores))
> -		lcores++;
> -	i = strlen(lcores);
> -	while ((i > 0) && isblank(lcores[i - 1]))
> +	while (isblank(*threads))
> +		threads++;
> +	i = strlen(threads);
> +	while ((i > 0) && isblank(threads[i - 1]))
>  		i--;
> 
>  	CPU_ZERO(&cpuset);
> @@ -491,27 +491,27 @@ eal_parse_lcores(const char *lcores)
> 
>  	/* Get list of cores */
>  	do {
> -		while (isblank(*lcores))
> -			lcores++;
> -		if (*lcores == '\0')
> +		while (isblank(*threads))
> +			threads++;
> +		if (*threads == '\0')
>  			goto err;
> 
> -		/* record lcore_set start point */
> -		lcore_start = lcores;
> +		/* record thread_set start point */
> +		thread_start = threads;
> 
>  		/* go across a complete bracket */
> -		if (*lcore_start == '(') {
> -			lcores += strcspn(lcores, ")");
> -			if (*lcores++ == '\0')
> +		if (*thread_start == '(') {
> +			threads += strcspn(threads, ")");
> +			if (*threads++ == '\0')
>  				goto err;
>  		}
> 
>  		/* scan the separator '@', ','(next) or '\0'(finish) */
> -		lcores += strcspn(lcores, "@,");
> +		threads += strcspn(threads, "@,");
> 
> -		if (*lcores == '@') {
> +		if (*threads == '@') {
>  			/* explicit assign cpu_set */
> -			offset = eal_parse_set(lcores + 1, set, RTE_DIM(set));
> +			offset = eal_parse_set(threads + 1, set, RTE_DIM(set));
>  			if (offset < 0)
>  				goto err;
> 
> @@ -519,31 +519,31 @@ eal_parse_lcores(const char *lcores)
>  			if (0 > convert_to_cpuset(&cpuset,
>  						  set, RTE_DIM(set)))
>  				goto err;
> -			end = lcores + 1 + offset;
> +			end = threads + 1 + offset;
>  		} else { /* ',' or '\0' */
>  			/* haven't given cpu_set, current loop done */
> -			end = lcores;
> +			end = threads;
> 
>  			/* go back to check <number>-<number> */
> -			offset = strcspn(lcore_start, "(-");
> -			if (offset < (end - lcore_start) &&
> -			    *(lcore_start + offset) != '(')
> +			offset = strcspn(thread_start, "(-");
> +			if (offset < (end - thread_start) &&
> +			    *(thread_start + offset) != '(')
>  				lflags = 1;
>  		}
> 
>  		if (*end != ',' && *end != '\0')
>  			goto err;
> 
> -		/* parse lcore_set from start point */
> -		if (0 > eal_parse_set(lcore_start, set, RTE_DIM(set)))
> +		/* parse thread_set from start point */
> +		if (0 > eal_parse_set(thread_start, set, RTE_DIM(set)))
>  			goto err;
> 
> -		/* without '@', by default using lcore_set as cpu_set */
> -		if (*lcores != '@' &&
> +		/* without '@', by default using thread_set as cpu_set */
> +		if (*threads != '@' &&
>  		    0 > convert_to_cpuset(&cpuset, set, RTE_DIM(set)))
>  			goto err;
> 
> -		/* start to update lcore_set */
> +		/* start to update thread_set */
>  		for (idx = 0; idx < RTE_MAX_LCORE; idx++) {
>  			if (!set[idx])
>  				continue;
> @@ -562,7 +562,7 @@ eal_parse_lcores(const char *lcores)
>  				   sizeof(rte_cpuset_t));
>  		}
> 
> -		lcores = end + 1;
> +		threads = end + 1;
>  	} while (*end != '\0');
> 
>  	if (count == 0)
> @@ -777,10 +777,10 @@ eal_parse_common_option(int opt, const char *optarg,
>  		conf->log_level = log;
>  		break;
>  	}
> -	case OPT_LCORES_NUM:
> -		if (eal_parse_lcores(optarg) < 0) {
> +	case OPT_THREADS_NUM:
> +		if (eal_parse_threads(optarg) < 0) {
>  			RTE_LOG(ERR, EAL, "invalid parameter for --"
> -				OPT_LCORES "\n");
> +				OPT_THREADS "\n");
>  			return -1;
>  		}
>  		break;
> @@ -822,7 +822,7 @@ eal_check_common_options(struct internal_config *internal_cfg)
> 
>  	if (!lcores_parsed) {
>  		RTE_LOG(ERR, EAL, "CPU cores must be enabled with options "
> -			"-c, -l or --lcores\n");
> +			"-c, -l or --threads\n");
>  		return -1;
>  	}
>  	if (cfg->lcore_role[cfg->master_lcore] != ROLE_RTE) {
> @@ -876,14 +876,14 @@ eal_common_usage(void)
>  	       "  -l CORELIST         List of cores to run on\n"
>  	       "                      The argument format is <c1>[-c2][,c3[-c4],...]\n"
>  	       "                      where c1, c2, etc are core indexes between 0 and %d\n"
> -	       "  --"OPT_LCORES" COREMAP    Map lcore set to physical cpu set\n"
> +	       "  --"OPT_THREADS" COREMAP   Map thread set to physical cpu set\n"
>  	       "                      The argument format is\n"
> -	       "                            '<lcores[@cpus]>[<,lcores[@cpus]>...]'\n"
> -	       "                      lcores and cpus list are grouped by '(' and ')'\n"
> +	       "                            '<threads[@cpus]>[<,threads[@cpus]>...]'\n"
> +	       "                      threads and cpus list are grouped by '(' and ')'\n"
>  	       "                      Within the group, '-' is used for range separator,\n"
>  	       "                      ',' is used for single number separator.\n"
>  	       "                      '( )' can be omitted for single element group,\n"
> -	       "                      '@' can be omitted if cpus and lcores have the same value\n"
> +	       "                      '@' can be omitted if cpus and threads have the same value\n"
>  	       "  --"OPT_MASTER_LCORE" ID   Core ID that is used as master\n"
>  	       "  -n CHANNELS         Number of memory channels\n"
>  	       "  -m MB               Memory to allocate (see also --"OPT_SOCKET_MEM")\n"
> diff --git a/lib/librte_eal/common/eal_options.h b/lib/librte_eal/common/eal_options.h
> index f6714d9..d979c62 100644
> --- a/lib/librte_eal/common/eal_options.h
> +++ b/lib/librte_eal/common/eal_options.h
> @@ -53,8 +53,6 @@ enum {
>  	OPT_FILE_PREFIX_NUM,
>  #define OPT_HUGE_DIR          "huge-dir"
>  	OPT_HUGE_DIR_NUM,
> -#define OPT_LCORES            "lcores"
> -	OPT_LCORES_NUM,
>  #define OPT_LOG_LEVEL         "log-level"
>  	OPT_LOG_LEVEL_NUM,
>  #define OPT_MASTER_LCORE      "master-lcore"
> @@ -73,6 +71,8 @@ enum {
>  	OPT_SOCKET_MEM_NUM,
>  #define OPT_SYSLOG            "syslog"
>  	OPT_SYSLOG_NUM,
> +#define OPT_THREADS           "threads"
> +	OPT_THREADS_NUM,
>  #define OPT_VDEV              "vdev"
>  	OPT_VDEV_NUM,
>  #define OPT_VFIO_INTR         "vfio-intr"
> --
> 2.2.2