[PATCH] eal/linuxapp: Add parameter to specify master lcore id

[PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Simon Kuenzer]

This commit enables users to specify the lcore id that
is used as master lcore.

Signed-off-by: Simon Kuenzer <simon.kuenzer-kcmmt4fgdiuHXe+LvDLADg@public.gmane.org>
---
 lib/librte_eal/linuxapp/eal/eal.c |   33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 573fd06..4ad5b9b 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -101,6 +101,7 @@
 #define OPT_XEN_DOM0    "xen-dom0"
 #define OPT_CREATE_UIO_DEV "create-uio-dev"
 #define OPT_VFIO_INTR    "vfio-intr"
+#define OPT_MASTER_LCORE "master-lcore"
 
 #define RTE_EAL_BLACKLIST_SIZE	0x100
 
@@ -336,6 +337,7 @@ eal_usage(const char *prgname)
 	       "[--proc-type primary|secondary|auto] \n\n"
 	       "EAL options:\n"
 	       "  -c COREMASK  : A hexadecimal bitmask of cores to run on\n"
+	       "  --"OPT_MASTER_LCORE" ID: Core ID that is used as master\n"
 	       "  -n NUM       : Number of memory channels\n"
 		   "  -v           : Display version information on startup\n"
 	       "  -d LIB.so    : add driver (can be used multiple times)\n"
@@ -468,6 +470,21 @@ eal_parse_coremask(const char *coremask)
 	return 0;
 }
 
+/* Changes the lcore id of the master thread */
+static int
+eal_parse_master_lcore(const char *arg)
+{
+	struct rte_config *cfg = rte_eal_get_configuration();
+	int master_lcore = atoi(arg);
+
+	if (!(master_lcore >= 0 && master_lcore < RTE_MAX_LCORE))
+		return -1;
+	if (cfg->lcore_role[master_lcore] != ROLE_RTE)
+		return -1;
+	cfg->master_lcore = master_lcore;
+	return 0;
+}
+
 static int
 eal_parse_syslog(const char *facility)
 {
@@ -653,6 +670,7 @@ eal_parse_args(int argc, char **argv)
 		{OPT_HUGE_DIR, 1, 0, 0},
 		{OPT_NO_SHCONF, 0, 0, 0},
 		{OPT_PROC_TYPE, 1, 0, 0},
+		{OPT_MASTER_LCORE, 1, 0, 0},
 		{OPT_FILE_PREFIX, 1, 0, 0},
 		{OPT_SOCKET_MEM, 1, 0, 0},
 		{OPT_PCI_WHITELIST, 1, 0, 0},
@@ -802,6 +820,21 @@ eal_parse_args(int argc, char **argv)
 			else if (!strcmp(lgopts[option_index].name, OPT_PROC_TYPE)) {
 				internal_config.process_type = eal_parse_proc_type(optarg);
 			}
+			else if (!strcmp(lgopts[option_index].name, OPT_MASTER_LCORE)) {
+				if (!coremask_ok) {
+					RTE_LOG(ERR, EAL, "please specify the master "
+							"lcore id after specifying "
+							"the coremask\n");
+					eal_usage(prgname);
+					return -1;
+				}
+				if (eal_parse_master_lcore(optarg) < 0) {
+					RTE_LOG(ERR, EAL, "invalid parameter for --"
+							OPT_MASTER_LCORE "\n");
+					eal_usage(prgname);
+					return -1;
+				}
+			}
 			else if (!strcmp(lgopts[option_index].name, OPT_FILE_PREFIX)) {
 				internal_config.hugefile_prefix = optarg;
 			}
-- 
1.7.10.4

Re: [PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Simon Kuenzer]

Here are some comments about the use case of this patch:

This patch is especially useful in cases where DPDK applications scale 
their CPU resources at runtime via starting and stopping slave lcores. 
Since the coremask defines the maximum scale-out for such a application, 
the master lcore becomes to the minimum scale-in.
Imagine, running multiple primary processed of such DPDK applications, 
users might want to overlap the coremasks for scaling. However, it would 
still make sense to run the master lcores on different CPU cores.

In DPDK vSwitch we might end up in such a scenario with a future release:
   https://lists.01.org/pipermail/dpdk-ovs/2014-March/000770.html
   https://lists.01.org/pipermail/dpdk-ovs/2014-March/000773.html

Thanks,

Simon

On 08.07.2014 10:28, Simon Kuenzer wrote:
> This commit enables users to specify the lcore id that
> is used as master lcore.
>
> Signed-off-by: Simon Kuenzer <simon.kuenzer-kcmmt4fgdiuHXe+LvDLADg@public.gmane.org>
> ---
>   lib/librte_eal/linuxapp/eal/eal.c |   33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index 573fd06..4ad5b9b 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -101,6 +101,7 @@
>   #define OPT_XEN_DOM0    "xen-dom0"
>   #define OPT_CREATE_UIO_DEV "create-uio-dev"
>   #define OPT_VFIO_INTR    "vfio-intr"
> +#define OPT_MASTER_LCORE "master-lcore"
>
>   #define RTE_EAL_BLACKLIST_SIZE	0x100
>
> @@ -336,6 +337,7 @@ eal_usage(const char *prgname)
>   	       "[--proc-type primary|secondary|auto] \n\n"
>   	       "EAL options:\n"
>   	       "  -c COREMASK  : A hexadecimal bitmask of cores to run on\n"
> +	       "  --"OPT_MASTER_LCORE" ID: Core ID that is used as master\n"
>   	       "  -n NUM       : Number of memory channels\n"
>   		   "  -v           : Display version information on startup\n"
>   	       "  -d LIB.so    : add driver (can be used multiple times)\n"
> @@ -468,6 +470,21 @@ eal_parse_coremask(const char *coremask)
>   	return 0;
>   }
>
> +/* Changes the lcore id of the master thread */
> +static int
> +eal_parse_master_lcore(const char *arg)
> +{
> +	struct rte_config *cfg = rte_eal_get_configuration();
> +	int master_lcore = atoi(arg);
> +
> +	if (!(master_lcore >= 0 && master_lcore < RTE_MAX_LCORE))
> +		return -1;
> +	if (cfg->lcore_role[master_lcore] != ROLE_RTE)
> +		return -1;
> +	cfg->master_lcore = master_lcore;
> +	return 0;
> +}
> +
>   static int
>   eal_parse_syslog(const char *facility)
>   {
> @@ -653,6 +670,7 @@ eal_parse_args(int argc, char **argv)
>   		{OPT_HUGE_DIR, 1, 0, 0},
>   		{OPT_NO_SHCONF, 0, 0, 0},
>   		{OPT_PROC_TYPE, 1, 0, 0},
> +		{OPT_MASTER_LCORE, 1, 0, 0},
>   		{OPT_FILE_PREFIX, 1, 0, 0},
>   		{OPT_SOCKET_MEM, 1, 0, 0},
>   		{OPT_PCI_WHITELIST, 1, 0, 0},
> @@ -802,6 +820,21 @@ eal_parse_args(int argc, char **argv)
>   			else if (!strcmp(lgopts[option_index].name, OPT_PROC_TYPE)) {
>   				internal_config.process_type = eal_parse_proc_type(optarg);
>   			}
> +			else if (!strcmp(lgopts[option_index].name, OPT_MASTER_LCORE)) {
> +				if (!coremask_ok) {
> +					RTE_LOG(ERR, EAL, "please specify the master "
> +							"lcore id after specifying "
> +							"the coremask\n");
> +					eal_usage(prgname);
> +					return -1;
> +				}
> +				if (eal_parse_master_lcore(optarg) < 0) {
> +					RTE_LOG(ERR, EAL, "invalid parameter for --"
> +							OPT_MASTER_LCORE "\n");
> +					eal_usage(prgname);
> +					return -1;
> +				}
> +			}
>   			else if (!strcmp(lgopts[option_index].name, OPT_FILE_PREFIX)) {
>   				internal_config.hugefile_prefix = optarg;
>   			}
>

Re: [PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Simon Kuenzer]

Comments?

On 08.07.2014 11:42, Simon Kuenzer wrote:
> Here are some comments about the use case of this patch:
>
> This patch is especially useful in cases where DPDK applications scale
> their CPU resources at runtime via starting and stopping slave lcores.
> Since the coremask defines the maximum scale-out for such a application,
> the master lcore becomes to the minimum scale-in.
> Imagine, running multiple primary processed of such DPDK applications,
> users might want to overlap the coremasks for scaling. However, it would
> still make sense to run the master lcores on different CPU cores.
>
> In DPDK vSwitch we might end up in such a scenario with a future release:
>    https://lists.01.org/pipermail/dpdk-ovs/2014-March/000770.html
>    https://lists.01.org/pipermail/dpdk-ovs/2014-March/000773.html
>
> Thanks,
>
> Simon
>
> On 08.07.2014 10:28, Simon Kuenzer wrote:
>> This commit enables users to specify the lcore id that
>> is used as master lcore.
>>
>> Signed-off-by: Simon Kuenzer <simon.kuenzer-kcmmt4fgdiuHXe+LvDLADg@public.gmane.org>
>> ---
>>   lib/librte_eal/linuxapp/eal/eal.c |   33
>> +++++++++++++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/lib/librte_eal/linuxapp/eal/eal.c
>> b/lib/librte_eal/linuxapp/eal/eal.c
>> index 573fd06..4ad5b9b 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal.c
>> @@ -101,6 +101,7 @@
>>   #define OPT_XEN_DOM0    "xen-dom0"
>>   #define OPT_CREATE_UIO_DEV "create-uio-dev"
>>   #define OPT_VFIO_INTR    "vfio-intr"
>> +#define OPT_MASTER_LCORE "master-lcore"
>>
>>   #define RTE_EAL_BLACKLIST_SIZE    0x100
>>
>> @@ -336,6 +337,7 @@ eal_usage(const char *prgname)
>>              "[--proc-type primary|secondary|auto] \n\n"
>>              "EAL options:\n"
>>              "  -c COREMASK  : A hexadecimal bitmask of cores to run
>> on\n"
>> +           "  --"OPT_MASTER_LCORE" ID: Core ID that is used as master\n"
>>              "  -n NUM       : Number of memory channels\n"
>>              "  -v           : Display version information on startup\n"
>>              "  -d LIB.so    : add driver (can be used multiple times)\n"
>> @@ -468,6 +470,21 @@ eal_parse_coremask(const char *coremask)
>>       return 0;
>>   }
>>
>> +/* Changes the lcore id of the master thread */
>> +static int
>> +eal_parse_master_lcore(const char *arg)
>> +{
>> +    struct rte_config *cfg = rte_eal_get_configuration();
>> +    int master_lcore = atoi(arg);
>> +
>> +    if (!(master_lcore >= 0 && master_lcore < RTE_MAX_LCORE))
>> +        return -1;
>> +    if (cfg->lcore_role[master_lcore] != ROLE_RTE)
>> +        return -1;
>> +    cfg->master_lcore = master_lcore;
>> +    return 0;
>> +}
>> +
>>   static int
>>   eal_parse_syslog(const char *facility)
>>   {
>> @@ -653,6 +670,7 @@ eal_parse_args(int argc, char **argv)
>>           {OPT_HUGE_DIR, 1, 0, 0},
>>           {OPT_NO_SHCONF, 0, 0, 0},
>>           {OPT_PROC_TYPE, 1, 0, 0},
>> +        {OPT_MASTER_LCORE, 1, 0, 0},
>>           {OPT_FILE_PREFIX, 1, 0, 0},
>>           {OPT_SOCKET_MEM, 1, 0, 0},
>>           {OPT_PCI_WHITELIST, 1, 0, 0},
>> @@ -802,6 +820,21 @@ eal_parse_args(int argc, char **argv)
>>               else if (!strcmp(lgopts[option_index].name,
>> OPT_PROC_TYPE)) {
>>                   internal_config.process_type =
>> eal_parse_proc_type(optarg);
>>               }
>> +            else if (!strcmp(lgopts[option_index].name,
>> OPT_MASTER_LCORE)) {
>> +                if (!coremask_ok) {
>> +                    RTE_LOG(ERR, EAL, "please specify the master "
>> +                            "lcore id after specifying "
>> +                            "the coremask\n");
>> +                    eal_usage(prgname);
>> +                    return -1;
>> +                }
>> +                if (eal_parse_master_lcore(optarg) < 0) {
>> +                    RTE_LOG(ERR, EAL, "invalid parameter for --"
>> +                            OPT_MASTER_LCORE "\n");
>> +                    eal_usage(prgname);
>> +                    return -1;
>> +                }
>> +            }
>>               else if (!strcmp(lgopts[option_index].name,
>> OPT_FILE_PREFIX)) {
>>                   internal_config.hugefile_prefix = optarg;
>>               }
>>
>

Re: [PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Hiroshi Shimamoto]

Hi all,

does anyone have interest in this functionality?

I think this is important and useful.
Since we should care about core assignment to get high performance
and the master lcore thread is special in DPDK, we will want to
assign the master to the target core.
For example, with hyperthreading I'd like to make a pair of packet
processing threads into one physical core and separate the master
thread which does some management.

thanks,
Hiroshi

> Subject: Re: [dpdk-dev] [PATCH] eal/linuxapp: Add parameter to specify master lcore id
> 
> Comments?
> 
> On 08.07.2014 11:42, Simon Kuenzer wrote:
> > Here are some comments about the use case of this patch:
> >
> > This patch is especially useful in cases where DPDK applications scale
> > their CPU resources at runtime via starting and stopping slave lcores.
> > Since the coremask defines the maximum scale-out for such a application,
> > the master lcore becomes to the minimum scale-in.
> > Imagine, running multiple primary processed of such DPDK applications,
> > users might want to overlap the coremasks for scaling. However, it would
> > still make sense to run the master lcores on different CPU cores.
> >
> > In DPDK vSwitch we might end up in such a scenario with a future release:
> >    https://lists.01.org/pipermail/dpdk-ovs/2014-March/000770.html
> >    https://lists.01.org/pipermail/dpdk-ovs/2014-March/000773.html
> >
> > Thanks,
> >
> > Simon
> >
> > On 08.07.2014 10:28, Simon Kuenzer wrote:
> >> This commit enables users to specify the lcore id that
> >> is used as master lcore.
> >>
> >> Signed-off-by: Simon Kuenzer <simon.kuenzer-kcmmt4fgdiuHXe+LvDLADg@public.gmane.org>
> >> ---
> >>   lib/librte_eal/linuxapp/eal/eal.c |   33
> >> +++++++++++++++++++++++++++++++++
> >>   1 file changed, 33 insertions(+)
> >>
> >> diff --git a/lib/librte_eal/linuxapp/eal/eal.c
> >> b/lib/librte_eal/linuxapp/eal/eal.c
> >> index 573fd06..4ad5b9b 100644
> >> --- a/lib/librte_eal/linuxapp/eal/eal.c
> >> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> >> @@ -101,6 +101,7 @@
> >>   #define OPT_XEN_DOM0    "xen-dom0"
> >>   #define OPT_CREATE_UIO_DEV "create-uio-dev"
> >>   #define OPT_VFIO_INTR    "vfio-intr"
> >> +#define OPT_MASTER_LCORE "master-lcore"
> >>
> >>   #define RTE_EAL_BLACKLIST_SIZE    0x100
> >>
> >> @@ -336,6 +337,7 @@ eal_usage(const char *prgname)
> >>              "[--proc-type primary|secondary|auto] \n\n"
> >>              "EAL options:\n"
> >>              "  -c COREMASK  : A hexadecimal bitmask of cores to run
> >> on\n"
> >> +           "  --"OPT_MASTER_LCORE" ID: Core ID that is used as master\n"
> >>              "  -n NUM       : Number of memory channels\n"
> >>              "  -v           : Display version information on startup\n"
> >>              "  -d LIB.so    : add driver (can be used multiple times)\n"
> >> @@ -468,6 +470,21 @@ eal_parse_coremask(const char *coremask)
> >>       return 0;
> >>   }
> >>
> >> +/* Changes the lcore id of the master thread */
> >> +static int
> >> +eal_parse_master_lcore(const char *arg)
> >> +{
> >> +    struct rte_config *cfg = rte_eal_get_configuration();
> >> +    int master_lcore = atoi(arg);
> >> +
> >> +    if (!(master_lcore >= 0 && master_lcore < RTE_MAX_LCORE))
> >> +        return -1;
> >> +    if (cfg->lcore_role[master_lcore] != ROLE_RTE)
> >> +        return -1;
> >> +    cfg->master_lcore = master_lcore;
> >> +    return 0;
> >> +}
> >> +
> >>   static int
> >>   eal_parse_syslog(const char *facility)
> >>   {
> >> @@ -653,6 +670,7 @@ eal_parse_args(int argc, char **argv)
> >>           {OPT_HUGE_DIR, 1, 0, 0},
> >>           {OPT_NO_SHCONF, 0, 0, 0},
> >>           {OPT_PROC_TYPE, 1, 0, 0},
> >> +        {OPT_MASTER_LCORE, 1, 0, 0},
> >>           {OPT_FILE_PREFIX, 1, 0, 0},
> >>           {OPT_SOCKET_MEM, 1, 0, 0},
> >>           {OPT_PCI_WHITELIST, 1, 0, 0},
> >> @@ -802,6 +820,21 @@ eal_parse_args(int argc, char **argv)
> >>               else if (!strcmp(lgopts[option_index].name,
> >> OPT_PROC_TYPE)) {
> >>                   internal_config.process_type =
> >> eal_parse_proc_type(optarg);
> >>               }
> >> +            else if (!strcmp(lgopts[option_index].name,
> >> OPT_MASTER_LCORE)) {
> >> +                if (!coremask_ok) {
> >> +                    RTE_LOG(ERR, EAL, "please specify the master "
> >> +                            "lcore id after specifying "
> >> +                            "the coremask\n");
> >> +                    eal_usage(prgname);
> >> +                    return -1;
> >> +                }
> >> +                if (eal_parse_master_lcore(optarg) < 0) {
> >> +                    RTE_LOG(ERR, EAL, "invalid parameter for --"
> >> +                            OPT_MASTER_LCORE "\n");
> >> +                    eal_usage(prgname);
> >> +                    return -1;
> >> +                }
> >> +            }
> >>               else if (!strcmp(lgopts[option_index].name,
> >> OPT_FILE_PREFIX)) {
> >>                   internal_config.hugefile_prefix = optarg;
> >>               }
> >>
> >

Re: [PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Thomas Monjalon]

Hi Hiroshi,

2014-07-22 23:40, Hiroshi Shimamoto:
> does anyone have interest in this functionality?
> 
> I think this is important and useful.
> Since we should care about core assignment to get high performance
> and the master lcore thread is special in DPDK, we will want to
> assign the master to the target core.
> For example, with hyperthreading I'd like to make a pair of packet
> processing threads into one physical core and separate the master
> thread which does some management.

Thank you for showing your interest.
Does it mean you carefully reviewed this patch? In this case, I'd appreciate
a note "Reviewed-by:".

Thanks
-- 
Thomas

Re: [PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Hiroshi Shimamoto]

Hi,

> Subject: Re: [dpdk-dev] [PATCH] eal/linuxapp: Add parameter to specify master lcore id
> 
> Hi Hiroshi,
> 
> 2014-07-22 23:40, Hiroshi Shimamoto:
> > does anyone have interest in this functionality?
> >
> > I think this is important and useful.
> > Since we should care about core assignment to get high performance
> > and the master lcore thread is special in DPDK, we will want to
> > assign the master to the target core.
> > For example, with hyperthreading I'd like to make a pair of packet
> > processing threads into one physical core and separate the master
> > thread which does some management.
> 
> Thank you for showing your interest.
> Does it mean you carefully reviewed this patch? In this case, I'd appreciate
> a note "Reviewed-by:".

Not yet deeply, wait a bit, we're testing this patch in our application.
Will report if it works fine.

By the way, we should add the same code into the BSD code, right?

thanks,
Hiroshi

> 
> Thanks
> --
> Thomas

Re: [PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Thomas Monjalon]

2014-07-23 08:53, Hiroshi Shimamoto:
> 2014-07-23 09:50, Thomas Monjalon:
> > 2014-07-22 23:40, Hiroshi Shimamoto:
> > > does anyone have interest in this functionality?
> > >
> > > I think this is important and useful.
> > > Since we should care about core assignment to get high performance
> > > and the master lcore thread is special in DPDK, we will want to
> > > assign the master to the target core.
> > > For example, with hyperthreading I'd like to make a pair of packet
> > > processing threads into one physical core and separate the master
> > > thread which does some management.
> > 
> > Thank you for showing your interest.
> > Does it mean you carefully reviewed this patch? In this case, I'd appreciate
> > a note "Reviewed-by:".
> 
> Not yet deeply, wait a bit, we're testing this patch in our application.
> Will report if it works fine.
> 
> By the way, we should add the same code into the BSD code, right?

Right.
I'd prefer to reduce the duplicated footprint and have more common code
between BSD and Linux. But waiting this enhancement, we have to maintain
the duplicated code for BSD.

-- 
Thomas

Re: [PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Simon Kuenzer]

On 23.07.2014 11:04, Thomas Monjalon wrote:
> 2014-07-23 08:53, Hiroshi Shimamoto:
>> 2014-07-23 09:50, Thomas Monjalon:
>>> 2014-07-22 23:40, Hiroshi Shimamoto:
>>>> does anyone have interest in this functionality?
>>>>
>>>> I think this is important and useful.
>>>> Since we should care about core assignment to get high performance
>>>> and the master lcore thread is special in DPDK, we will want to
>>>> assign the master to the target core.
>>>> For example, with hyperthreading I'd like to make a pair of packet
>>>> processing threads into one physical core and separate the master
>>>> thread which does some management.
>>>
>>> Thank you for showing your interest.
>>> Does it mean you carefully reviewed this patch? In this case, I'd appreciate
>>> a note "Reviewed-by:".
>>
>> Not yet deeply, wait a bit, we're testing this patch in our application.
>> Will report if it works fine.
>>
>> By the way, we should add the same code into the BSD code, right?
>
> Right.
> I'd prefer to reduce the duplicated footprint and have more common code
> between BSD and Linux. But waiting this enhancement, we have to maintain
> the duplicated code for BSD.
>

Hi all,

I can provide the same patch also for BSD. However, I do not have a 
machine to test it. Interested?

Thanks,

Simon

Re: [PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Hiroshi Shimamoto]

Hi,

> Subject: Re: [dpdk-dev] [PATCH] eal/linuxapp: Add parameter to specify master lcore id
> 
> 2014-07-23 08:53, Hiroshi Shimamoto:
> > 2014-07-23 09:50, Thomas Monjalon:
> > > 2014-07-22 23:40, Hiroshi Shimamoto:
> > > > does anyone have interest in this functionality?
> > > >
> > > > I think this is important and useful.
> > > > Since we should care about core assignment to get high performance
> > > > and the master lcore thread is special in DPDK, we will want to
> > > > assign the master to the target core.
> > > > For example, with hyperthreading I'd like to make a pair of packet
> > > > processing threads into one physical core and separate the master
> > > > thread which does some management.
> > >
> > > Thank you for showing your interest.
> > > Does it mean you carefully reviewed this patch? In this case, I'd appreciate
> > > a note "Reviewed-by:".
> >
> > Not yet deeply, wait a bit, we're testing this patch in our application.
> > Will report if it works fine.

Sorry a delay, I had confirmed the functionality.
I'm fine to add
Reviewed-by: Hiroshi Shimamoto <h-shimamoto-ehU+Cx/zZe18UrSeD/g0lQ@public.gmane.org>

thanks,
Hiroshi

> >
> > By the way, we should add the same code into the BSD code, right?
> 
> Right.
> I'd prefer to reduce the duplicated footprint and have more common code
> between BSD and Linux. But waiting this enhancement, we have to maintain
> the duplicated code for BSD.
> 
> --
> Thomas

Re: [PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Simon Kuenzer]

Hi all,

the only issue I could imagine is that current DPDK applications are
utilizing the implicit assumption that the master lcore is always set to
the first available lcore. I would consider this as a "bug" in the
application because it sets up its worker threads not "properly".

However, as far I could check it, the DPDK framework seems to cope with
it correctly.
It would be nice if somebody else could confirm my statement.

Thanks,

Simon

On 23.07.2014 10:53, Hiroshi Shimamoto wrote:
> Hi,
> 
>> Subject: Re: [dpdk-dev] [PATCH] eal/linuxapp: Add parameter to specify master lcore id
>>
>> Hi Hiroshi,
>>
>> 2014-07-22 23:40, Hiroshi Shimamoto:
>>> does anyone have interest in this functionality?
>>>
>>> I think this is important and useful.
>>> Since we should care about core assignment to get high performance
>>> and the master lcore thread is special in DPDK, we will want to
>>> assign the master to the target core.
>>> For example, with hyperthreading I'd like to make a pair of packet
>>> processing threads into one physical core and separate the master
>>> thread which does some management.
>>
>> Thank you for showing your interest.
>> Does it mean you carefully reviewed this patch? In this case, I'd appreciate
>> a note "Reviewed-by:".
> 
> Not yet deeply, wait a bit, we're testing this patch in our application.
> Will report if it works fine.
> 
> By the way, we should add the same code into the BSD code, right?
> 
> thanks,
> Hiroshi
> 
>>
>> Thanks
>> --
>> Thomas

Re: [PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Aaron Campbell]

Hi Simon,

Thanks for the patch, this will be useful for us.  I responded separately to your original post with one suggestion.

Our application currently assumes that DPDK will assign the first bit set in the coremask to the master lcore.  As far as I can tell, this is hard-coded as of 1.7.1.  But we would like the ability for our application to specify any bit from the coremask to serve as the master lcore.

I don’t see any compatibility issues with this.  Existing applications should behave as before.

Thomas, could this be accepted for the 1.8 release?  Or will that only happen if the BSD side can be patched as well?

-Aaron

> On Jul 23, 2014, at 9:10 AM, Simon Kuenzer <simon.kuenzer-kcmmt4fgdiuHXe+LvDLADg@public.gmane.org> wrote:
> 
> Hi all,
> 
> the only issue I could imagine is that current DPDK applications are
> utilizing the implicit assumption that the master lcore is always set to
> the first available lcore. I would consider this as a "bug" in the
> application because it sets up its worker threads not "properly".
> 
> However, as far I could check it, the DPDK framework seems to cope with
> it correctly.
> It would be nice if somebody else could confirm my statement.
> 
> Thanks,
> 
> Simon
> 
> On 23.07.2014 10:53, Hiroshi Shimamoto wrote:
>> Hi,
>> 
>>> Subject: Re: [dpdk-dev] [PATCH] eal/linuxapp: Add parameter to specify master lcore id
>>> 
>>> Hi Hiroshi,
>>> 
>>> 2014-07-22 23:40, Hiroshi Shimamoto:
>>>> does anyone have interest in this functionality?
>>>> 
>>>> I think this is important and useful.
>>>> Since we should care about core assignment to get high performance
>>>> and the master lcore thread is special in DPDK, we will want to
>>>> assign the master to the target core.
>>>> For example, with hyperthreading I'd like to make a pair of packet
>>>> processing threads into one physical core and separate the master
>>>> thread which does some management.
>>> 
>>> Thank you for showing your interest.
>>> Does it mean you carefully reviewed this patch? In this case, I'd appreciate
>>> a note "Reviewed-by:".
>> 
>> Not yet deeply, wait a bit, we're testing this patch in our application.
>> Will report if it works fine.
>> 
>> By the way, we should add the same code into the BSD code, right?
>> 
>> thanks,
>> Hiroshi
>> 
>>> 
>>> Thanks
>>> --
>>> Thomas
> 

Re: [PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Thomas Monjalon]

2014-11-03 13:02, Aaron Campbell:
> Hi Simon,
> 
> Thanks for the patch, this will be useful for us.  I responded separately to your original post with one suggestion.
> 
> Our application currently assumes that DPDK will assign the first bit set in the coremask to the master lcore.  As far as I can tell, this is hard-coded as of 1.7.1.  But we would like the ability for our application to specify any bit from the coremask to serve as the master lcore.
> 
> I don’t see any compatibility issues with this.  Existing applications should behave as before.
> 
> Thomas, could this be accepted for the 1.8 release?  Or will that only happen if the BSD side can be patched as well?

No need for BSD side patch because option management is now common between
BSD and Linux. I'm going to send an updated version of this patch.

> > On Jul 23, 2014, at 9:10 AM, Simon Kuenzer <simon.kuenzer-kcmmt4fgdiuHXe+LvDLADg@public.gmane.org> wrote:
> > 
> > Hi all,
> > 
> > the only issue I could imagine is that current DPDK applications are
> > utilizing the implicit assumption that the master lcore is always set to
> > the first available lcore. I would consider this as a "bug" in the
> > application because it sets up its worker threads not "properly".
> > 
> > However, as far I could check it, the DPDK framework seems to cope with
> > it correctly.
> > It would be nice if somebody else could confirm my statement.
> > 
> > Thanks,
> > 
> > Simon
> > 
> > On 23.07.2014 10:53, Hiroshi Shimamoto wrote:
> >> Hi,
> >> 
> >>> Subject: Re: [dpdk-dev] [PATCH] eal/linuxapp: Add parameter to specify master lcore id
> >>> 
> >>> Hi Hiroshi,
> >>> 
> >>> 2014-07-22 23:40, Hiroshi Shimamoto:
> >>>> does anyone have interest in this functionality?
> >>>> 
> >>>> I think this is important and useful.
> >>>> Since we should care about core assignment to get high performance
> >>>> and the master lcore thread is special in DPDK, we will want to
> >>>> assign the master to the target core.
> >>>> For example, with hyperthreading I'd like to make a pair of packet
> >>>> processing threads into one physical core and separate the master
> >>>> thread which does some management.
> >>> 
> >>> Thank you for showing your interest.
> >>> Does it mean you carefully reviewed this patch? In this case, I'd appreciate
> >>> a note "Reviewed-by:".
> >> 
> >> Not yet deeply, wait a bit, we're testing this patch in our application.
> >> Will report if it works fine.
> >> 
> >> By the way, we should add the same code into the BSD code, right?
> >> 
> >> thanks,
> >> Hiroshi

Re: [PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Gray, Mark D]

> Hi all,
> 
> does anyone have interest in this functionality?

Yes, I think this is useful for DPDK vSwitch.
> 
> I think this is important and useful.
> Since we should care about core assignment to get high performance and the
> master lcore thread is special in DPDK, we will want to assign the master to
> the target core.
> For example, with hyperthreading I'd like to make a pair of packet processing
> threads into one physical core and separate the master thread which does
> some management.
> 
> thanks,
> Hiroshi
> 

Re: [PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Aaron Campbell]

> On Jul 8, 2014, at 5:28 AM, Simon Kuenzer <simon.kuenzer-kcmmt4fgdiuHXe+LvDLADg@public.gmane.org> wrote:
> 
> +			else if (!strcmp(lgopts[option_index].name, OPT_MASTER_LCORE)) {
> +				if (!coremask_ok) {
> +					RTE_LOG(ERR, EAL, "please specify the master "
> +							"lcore id after specifying "
> +							"the coremask\n");
> +					eal_usage(prgname);
> +					return -1;
> +				}


Hi Simon,

I think that forcing a particular command line order is not that clean.  It might be better to remove the cfg->master_lcore setting from eal_parse_coremask(), and defer the selection of the master lcore until all of the command-line options have been parsed.  If —master-lcore was specified, save the value and use that, otherwise rte_get_next_lcore(-1, 0, 0) can return the first bit set in the coremask.

-Aaron

Re: [PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Thomas Monjalon]

2014-11-03 13:02, Aaron Campbell:
> > On Jul 8, 2014, at 5:28 AM, Simon Kuenzer <simon.kuenzer-kcmmt4fgdiuHXe+LvDLADg@public.gmane.org> wrote:
> > 
> > +			else if (!strcmp(lgopts[option_index].name, OPT_MASTER_LCORE)) {
> > +				if (!coremask_ok) {
> > +					RTE_LOG(ERR, EAL, "please specify the master "
> > +							"lcore id after specifying "
> > +							"the coremask\n");
> > +					eal_usage(prgname);
> > +					return -1;
> > +				}
> 
> 
> Hi Simon,
> 
> I think that forcing a particular command line order is not that clean.
> It might be better to remove the cfg->master_lcore setting from
> eal_parse_coremask(), and defer the selection of the master lcore until
> all of the command-line options have been parsed.  If —master-lcore was
> specified, save the value and use that, otherwise
> rte_get_next_lcore(-1, 0, 0) can return the first bit set in the coremask.

It's not sufficient: eal_parse_master_lcore() requires cfg->lcore_role
to be set. There is a real dependency between these 2 options.
I'm going to submit a v2. Feel free to improve it with another patch.

-- 
Thomas

Re: [PATCH] eal/linuxapp: Add parameter to specify master lcore id

[Aaron Campbell]

> On Nov 4, 2014, at 3:00 PM, Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org> wrote:
> 
> 2014-11-03 13:02, Aaron Campbell:
>>> On Jul 8, 2014, at 5:28 AM, Simon Kuenzer <simon.kuenzer-kcmmt4fgdiuHXe+LvDLADg@public.gmane.org> wrote:
>>> 
>>> +			else if (!strcmp(lgopts[option_index].name, OPT_MASTER_LCORE)) {
>>> +				if (!coremask_ok) {
>>> +					RTE_LOG(ERR, EAL, "please specify the master "
>>> +							"lcore id after specifying "
>>> +							"the coremask\n");
>>> +					eal_usage(prgname);
>>> +					return -1;
>>> +				}
>> 
>> 
>> Hi Simon,
>> 
>> I think that forcing a particular command line order is not that clean.
>> It might be better to remove the cfg->master_lcore setting from
>> eal_parse_coremask(), and defer the selection of the master lcore until
>> all of the command-line options have been parsed.  If —master-lcore was
>> specified, save the value and use that, otherwise
>> rte_get_next_lcore(-1, 0, 0) can return the first bit set in the coremask.
> 
> It's not sufficient: eal_parse_master_lcore() requires cfg->lcore_role
> to be set. There is a real dependency between these 2 options.
> I'm going to submit a v2. Feel free to improve it with another patch.

I was nit-picking; although it might be nice if the new option is given, to verify the specified lcore is in the coremask.  I will ack v2 though and this can be improved some other time.

-Aaron

[PATCH v2] eal: add option --master-lcore

[Thomas Monjalon]

From: Simon Kuenzer <simon.kuenzer-kcmmt4fgdiuHXe+LvDLADg@public.gmane.org>

Enable users to specify the lcore id that is used as master lcore.

Signed-off-by: Simon Kuenzer <simon.kuenzer-kcmmt4fgdiuHXe+LvDLADg@public.gmane.org>
Signed-off-by: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
---

changes in v2:
- rebase on HEAD including common options for BSD and Linux
- use strtol() instead of atoi() to check syntax errors
- unit tests

 app/test/test.c                             |  1 +
 app/test/test_eal_flags.c                   | 49 +++++++++++++++++++++++++++++
 lib/librte_eal/bsdapp/eal/eal.c             |  7 +++++
 lib/librte_eal/common/eal_common_options.c  | 31 ++++++++++++++++++
 lib/librte_eal/common/include/eal_options.h |  2 ++
 lib/librte_eal/linuxapp/eal/eal.c           |  7 +++++
 6 files changed, 97 insertions(+)

diff --git a/app/test/test.c b/app/test/test.c
index 9bee6bb..2fecff5 100644
--- a/app/test/test.c
+++ b/app/test/test.c
@@ -82,6 +82,7 @@ do_recursive_call(void)
 	} actions[] =  {
 			{ "run_secondary_instances", test_mp_secondary },
 			{ "test_missing_c_flag", no_action },
+			{ "test_master_lcore_flag", no_action },
 			{ "test_missing_n_flag", no_action },
 			{ "test_no_hpet_flag", no_action },
 			{ "test_whitelist_flag", no_action },
diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
index 21e6cca..45020b8 100644
--- a/app/test/test_eal_flags.c
+++ b/app/test/test_eal_flags.c
@@ -520,6 +520,49 @@ test_missing_c_flag(void)
 }
 
 /*
+ * Test --master-lcore option with matching coremask
+ */
+static int
+test_master_lcore_flag(void)
+{
+#ifdef RTE_EXEC_ENV_BSDAPP
+	/* BSD target doesn't support prefixes at this point */
+	const char * prefix = "";
+#else
+	char prefix[PATH_MAX], tmp[PATH_MAX];
+	if (get_current_prefix(tmp, sizeof(tmp)) == NULL) {
+		printf("Error - unable to get current prefix!\n");
+		return -1;
+	}
+	snprintf(prefix, sizeof(prefix), "--file-prefix=%s", tmp);
+#endif
+
+	/* --master-lcore flag but no value */
+	const char *argv1[] = { prgname, prefix, mp_flag, "-n", "1", "-c", "3", "--master-lcore"};
+	/* --master-lcore flag with invalid value */
+	const char *argv2[] = { prgname, prefix, mp_flag, "-n", "1", "-c", "3", "--master-lcore", "-1"};
+	/* --master-lcore flag with invalid value */
+	const char *argv3[] = { prgname, prefix, mp_flag, "-n", "1", "-c", "3", "--master-lcore", "X"};
+	/* master lcore not in coremask */
+	const char *argv4[] = { prgname, prefix, mp_flag, "-n", "1", "-c", "3", "--master-lcore", "2"};
+	/* valid value */
+	const char *argv5[] = { prgname, prefix, mp_flag, "-n", "1", "-c", "3", "--master-lcore", "1"};
+
+	if (launch_proc(argv1) == 0
+			|| launch_proc(argv2) == 0
+			|| launch_proc(argv3) == 0
+			|| launch_proc(argv4) == 0) {
+		printf("Error - process ran without error with wrong --master-lcore\n");
+		return -1;
+	}
+	if (launch_proc(argv5) != 0) {
+		printf("Error - process did not run ok with valid --master-lcore\n");
+		return -1;
+	}
+	return 0;
+}
+
+/*
  * Test that the app doesn't run without the -n flag. In all cases
  * should give an error and fail to run.
  * Since -n is not compulsory for MP, we instead use --no-huge and --no-shconf
@@ -1214,6 +1257,12 @@ test_eal_flags(void)
 		return ret;
 	}
 
+	ret = test_master_lcore_flag();
+	if (ret < 0) {
+		printf("Error in test_master_lcore_flag()\n");
+		return ret;
+	}
+
 	ret = test_missing_n_flag();
 	if (ret < 0) {
 		printf("Error in test_missing_n_flag()\n");
diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index ca99cb9..c764fec 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -354,6 +354,13 @@ eal_parse_args(int argc, char **argv)
 		if (opt == '?')
 			return -1;
 
+		if (opt == OPT_MASTER_LCORE_NUM && !coremask_ok) {
+			RTE_LOG(ERR, EAL, "please specify the master lcore id"
+				"after specifying the coremask\n");
+			eal_usage(prgname);
+			return -1;
+		}
+
 		ret = eal_parse_common_option(opt, optarg, &internal_config);
 		/* common parser is not happy */
 		if (ret < 0) {
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 7a5d55e..9166ea1 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -64,6 +64,7 @@ eal_short_options[] =
 const struct option
 eal_long_options[] = {
 	{OPT_HUGE_DIR, 1, 0, OPT_HUGE_DIR_NUM},
+	{OPT_MASTER_LCORE, 1, 0, OPT_MASTER_LCORE_NUM},
 	{OPT_PROC_TYPE, 1, 0, OPT_PROC_TYPE_NUM},
 	{OPT_NO_SHCONF, 0, 0, OPT_NO_SHCONF_NUM},
 	{OPT_NO_HPET, 0, 0, OPT_NO_HPET_NUM},
@@ -163,6 +164,27 @@ eal_parse_coremask(const char *coremask)
 	return 0;
 }
 
+/* Changes the lcore id of the master thread */
+static int
+eal_parse_master_lcore(const char *arg)
+{
+	long master_lcore;
+	char *parsing_end;
+	struct rte_config *cfg = rte_eal_get_configuration();
+
+	errno = 0;
+	master_lcore = strtol(arg, &parsing_end, 0);
+	if (errno || parsing_end == arg)
+		return -1;
+
+	if (!(master_lcore >= 0 && master_lcore < RTE_MAX_LCORE))
+		return -1;
+	if (cfg->lcore_role[master_lcore] != ROLE_RTE)
+		return -1;
+	cfg->master_lcore = master_lcore;
+	return 0;
+}
+
 static int
 eal_parse_syslog(const char *facility, struct internal_config *conf)
 {
@@ -320,6 +342,14 @@ eal_parse_common_option(int opt, const char *optarg,
 		conf->process_type = eal_parse_proc_type(optarg);
 		break;
 
+	case OPT_MASTER_LCORE_NUM:
+		if (eal_parse_master_lcore(optarg) < 0) {
+			RTE_LOG(ERR, EAL, "invalid parameter for --"
+					OPT_MASTER_LCORE "\n");
+			return -1;
+		}
+		break;
+
 	case OPT_VDEV_NUM:
 		if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL,
 				optarg) < 0) {
@@ -364,6 +394,7 @@ eal_common_usage(void)
 	       "[--proc-type primary|secondary|auto]\n\n"
 	       "EAL common options:\n"
 	       "  -c COREMASK  : A hexadecimal bitmask of cores to run on\n"
+	       "  --"OPT_MASTER_LCORE" ID: Core ID that is used as master\n"
 	       "  -n NUM       : Number of memory channels\n"
 	       "  -v           : Display version information on startup\n"
 	       "  -m MB        : memory to allocate (see also --"OPT_SOCKET_MEM")\n"
diff --git a/lib/librte_eal/common/include/eal_options.h b/lib/librte_eal/common/include/eal_options.h
index 22819ec..deb872d 100644
--- a/lib/librte_eal/common/include/eal_options.h
+++ b/lib/librte_eal/common/include/eal_options.h
@@ -43,6 +43,8 @@ enum {
 	OPT_LONG_MIN_NUM = 256,
 #define OPT_HUGE_DIR    "huge-dir"
 	OPT_HUGE_DIR_NUM = OPT_LONG_MIN_NUM,
+#define OPT_MASTER_LCORE "master-lcore"
+	OPT_MASTER_LCORE_NUM,
 #define OPT_PROC_TYPE   "proc-type"
 	OPT_PROC_TYPE_NUM,
 #define OPT_NO_SHCONF   "no-shconf"
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 7a1d087..7fcb349 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -550,6 +550,13 @@ eal_parse_args(int argc, char **argv)
 		if (opt == '?')
 			return -1;
 
+		if (opt == OPT_MASTER_LCORE_NUM && !coremask_ok) {
+			RTE_LOG(ERR, EAL, "please specify the master lcore id"
+				"after specifying the coremask\n");
+			eal_usage(prgname);
+			return -1;
+		}
+
 		ret = eal_parse_common_option(opt, optarg, &internal_config);
 		/* common parser is not happy */
 		if (ret < 0) {
-- 
2.1.3

Re: [PATCH v2] eal: add option --master-lcore

[Ananyev, Konstantin]

Hi Thomas,
Few questions/comments below.
Konstantin

> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, November 04, 2014 9:41 PM
> To: dev-VfR2kkLFssw@public.gmane.org
> Subject: [dpdk-dev] [PATCH v2] eal: add option --master-lcore
> 
> From: Simon Kuenzer <simon.kuenzer-kcmmt4fgdiuHXe+LvDLADg@public.gmane.org>
> 
> Enable users to specify the lcore id that is used as master lcore.
> 
> Signed-off-by: Simon Kuenzer <simon.kuenzer-kcmmt4fgdiuHXe+LvDLADg@public.gmane.org>
> Signed-off-by: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
> ---
> 
> changes in v2:
> - rebase on HEAD including common options for BSD and Linux
> - use strtol() instead of atoi() to check syntax errors
> - unit tests
> 
>  app/test/test.c                             |  1 +
>  app/test/test_eal_flags.c                   | 49 +++++++++++++++++++++++++++++
>  lib/librte_eal/bsdapp/eal/eal.c             |  7 +++++
>  lib/librte_eal/common/eal_common_options.c  | 31 ++++++++++++++++++
>  lib/librte_eal/common/include/eal_options.h |  2 ++
>  lib/librte_eal/linuxapp/eal/eal.c           |  7 +++++
>  6 files changed, 97 insertions(+)
> 
> diff --git a/app/test/test.c b/app/test/test.c
> index 9bee6bb..2fecff5 100644
> --- a/app/test/test.c
> +++ b/app/test/test.c
> @@ -82,6 +82,7 @@ do_recursive_call(void)
>  	} actions[] =  {
>  			{ "run_secondary_instances", test_mp_secondary },
>  			{ "test_missing_c_flag", no_action },
> +			{ "test_master_lcore_flag", no_action },
>  			{ "test_missing_n_flag", no_action },
>  			{ "test_no_hpet_flag", no_action },
>  			{ "test_whitelist_flag", no_action },
> diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
> index 21e6cca..45020b8 100644
> --- a/app/test/test_eal_flags.c
> +++ b/app/test/test_eal_flags.c
> @@ -520,6 +520,49 @@ test_missing_c_flag(void)
>  }
> 
>  /*
> + * Test --master-lcore option with matching coremask
> + */
> +static int
> +test_master_lcore_flag(void)
> +{
> +#ifdef RTE_EXEC_ENV_BSDAPP
> +	/* BSD target doesn't support prefixes at this point */
> +	const char * prefix = "";
> +#else
> +	char prefix[PATH_MAX], tmp[PATH_MAX];
> +	if (get_current_prefix(tmp, sizeof(tmp)) == NULL) {
> +		printf("Error - unable to get current prefix!\n");
> +		return -1;
> +	}
> +	snprintf(prefix, sizeof(prefix), "--file-prefix=%s", tmp);
> +#endif
> +
> +	/* --master-lcore flag but no value */
> +	const char *argv1[] = { prgname, prefix, mp_flag, "-n", "1", "-c", "3", "--master-lcore"};
> +	/* --master-lcore flag with invalid value */
> +	const char *argv2[] = { prgname, prefix, mp_flag, "-n", "1", "-c", "3", "--master-lcore", "-1"};
> +	/* --master-lcore flag with invalid value */
> +	const char *argv3[] = { prgname, prefix, mp_flag, "-n", "1", "-c", "3", "--master-lcore", "X"};
> +	/* master lcore not in coremask */
> +	const char *argv4[] = { prgname, prefix, mp_flag, "-n", "1", "-c", "3", "--master-lcore", "2"};
> +	/* valid value */
> +	const char *argv5[] = { prgname, prefix, mp_flag, "-n", "1", "-c", "3", "--master-lcore", "1"};
> +
> +	if (launch_proc(argv1) == 0
> +			|| launch_proc(argv2) == 0
> +			|| launch_proc(argv3) == 0
> +			|| launch_proc(argv4) == 0) {
> +		printf("Error - process ran without error with wrong --master-lcore\n");
> +		return -1;
> +	}
> +	if (launch_proc(argv5) != 0) {
> +		printf("Error - process did not run ok with valid --master-lcore\n");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +/*
>   * Test that the app doesn't run without the -n flag. In all cases
>   * should give an error and fail to run.
>   * Since -n is not compulsory for MP, we instead use --no-huge and --no-shconf
> @@ -1214,6 +1257,12 @@ test_eal_flags(void)
>  		return ret;
>  	}
> 
> +	ret = test_master_lcore_flag();
> +	if (ret < 0) {
> +		printf("Error in test_master_lcore_flag()\n");
> +		return ret;
> +	}
> +
>  	ret = test_missing_n_flag();
>  	if (ret < 0) {
>  		printf("Error in test_missing_n_flag()\n");
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
> index ca99cb9..c764fec 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -354,6 +354,13 @@ eal_parse_args(int argc, char **argv)
>  		if (opt == '?')
>  			return -1;
> 
> +		if (opt == OPT_MASTER_LCORE_NUM && !coremask_ok) {
> +			RTE_LOG(ERR, EAL, "please specify the master lcore id"
> +				"after specifying the coremask\n");
> +			eal_usage(prgname);
> +			return -1;
> +		}
> +
>  		ret = eal_parse_common_option(opt, optarg, &internal_config);
>  		/* common parser is not happy */
>  		if (ret < 0) {
> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index 7a5d55e..9166ea1 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -64,6 +64,7 @@ eal_short_options[] =
>  const struct option
>  eal_long_options[] = {
>  	{OPT_HUGE_DIR, 1, 0, OPT_HUGE_DIR_NUM},
> +	{OPT_MASTER_LCORE, 1, 0, OPT_MASTER_LCORE_NUM},
>  	{OPT_PROC_TYPE, 1, 0, OPT_PROC_TYPE_NUM},
>  	{OPT_NO_SHCONF, 0, 0, OPT_NO_SHCONF_NUM},
>  	{OPT_NO_HPET, 0, 0, OPT_NO_HPET_NUM},
> @@ -163,6 +164,27 @@ eal_parse_coremask(const char *coremask)
>  	return 0;
>  }
> 
> +/* Changes the lcore id of the master thread */
> +static int
> +eal_parse_master_lcore(const char *arg)
> +{
> +	long master_lcore;
> +	char *parsing_end;
> +	struct rte_config *cfg = rte_eal_get_configuration();
> +
> +	errno = 0;
> +	master_lcore = strtol(arg, &parsing_end, 0);
> +	if (errno || parsing_end == arg)
> +		return -1;

Why not: "errno || parsing_end[0] != 0"
?

Otherwise something like "1blah" would be considered as valid input.

> +
> +	if (!(master_lcore >= 0 && master_lcore < RTE_MAX_LCORE))
> +		return -1;

If negative values are not allowed, then why not:

unsigned long master_lcore;
...
master_lcore = strtoul(...)
...
if(master_clore > RTE_MAX_LCORE)
    return -1;   

> +	if (cfg->lcore_role[master_lcore] != ROLE_RTE)
> +		return -1;
> +	cfg->master_lcore = master_lcore;
> +	return 0;
> +}
> +
>  static int
>  eal_parse_syslog(const char *facility, struct internal_config *conf)
>  {
> @@ -320,6 +342,14 @@ eal_parse_common_option(int opt, const char *optarg,
>  		conf->process_type = eal_parse_proc_type(optarg);
>  		break;
> 
> +	case OPT_MASTER_LCORE_NUM:
> +		if (eal_parse_master_lcore(optarg) < 0) {
> +			RTE_LOG(ERR, EAL, "invalid parameter for --"
> +					OPT_MASTER_LCORE "\n");
> +			return -1;
> +		}
> +		break;
> +
>  	case OPT_VDEV_NUM:
>  		if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL,
>  				optarg) < 0) {
> @@ -364,6 +394,7 @@ eal_common_usage(void)
>  	       "[--proc-type primary|secondary|auto]\n\n"
>  	       "EAL common options:\n"
>  	       "  -c COREMASK  : A hexadecimal bitmask of cores to run on\n"
> +	       "  --"OPT_MASTER_LCORE" ID: Core ID that is used as master\n"
>  	       "  -n NUM       : Number of memory channels\n"
>  	       "  -v           : Display version information on startup\n"
>  	       "  -m MB        : memory to allocate (see also --"OPT_SOCKET_MEM")\n"
> diff --git a/lib/librte_eal/common/include/eal_options.h b/lib/librte_eal/common/include/eal_options.h
> index 22819ec..deb872d 100644
> --- a/lib/librte_eal/common/include/eal_options.h
> +++ b/lib/librte_eal/common/include/eal_options.h
> @@ -43,6 +43,8 @@ enum {
>  	OPT_LONG_MIN_NUM = 256,
>  #define OPT_HUGE_DIR    "huge-dir"
>  	OPT_HUGE_DIR_NUM = OPT_LONG_MIN_NUM,
> +#define OPT_MASTER_LCORE "master-lcore"
> +	OPT_MASTER_LCORE_NUM,
>  #define OPT_PROC_TYPE   "proc-type"
>  	OPT_PROC_TYPE_NUM,
>  #define OPT_NO_SHCONF   "no-shconf"
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index 7a1d087..7fcb349 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -550,6 +550,13 @@ eal_parse_args(int argc, char **argv)
>  		if (opt == '?')
>  			return -1;
> 
> +		if (opt == OPT_MASTER_LCORE_NUM && !coremask_ok) {
> +			RTE_LOG(ERR, EAL, "please specify the master lcore id"
> +				"after specifying the coremask\n");
> +			eal_usage(prgname);
> +			return -1;
> +		}
> +

I don't really like an idea of introducing strict order between -c and  "--master-lcore..
Can we move check for coremask_ok/ and assignment of cfg->master_lcore out of  
while (getopt_long(...)) loop?

>  		ret = eal_parse_common_option(opt, optarg, &internal_config);
>  		/* common parser is not happy */
>  		if (ret < 0) {
> --
> 2.1.3

Re: [PATCH v2] eal: add option --master-lcore

[Thomas Monjalon]

2014-11-05 11:54, Ananyev, Konstantin:
> From: Thomas Monjalon
> > +	long master_lcore;
> > +	char *parsing_end;
> > +	struct rte_config *cfg = rte_eal_get_configuration();
> > +
> > +	errno = 0;
> > +	master_lcore = strtol(arg, &parsing_end, 0);
> > +	if (errno || parsing_end == arg)
> > +		return -1;
> 
> Why not: "errno || parsing_end[0] != 0"
> ?
> Otherwise something like "1blah" would be considered as valid input.

Good point.

> > +	if (!(master_lcore >= 0 && master_lcore < RTE_MAX_LCORE))
> > +		return -1;
> 
> If negative values are not allowed, then why not:
> 
> unsigned long master_lcore;
> ...
> master_lcore = strtoul(...)
> ...
> if(master_clore > RTE_MAX_LCORE)
>     return -1;   

Matter of taste. Your code is less explicit.
But it should be
	if(master_clore >= RTE_MAX_LCORE)
Anyone else to vote for 1 solution or the other?

> > +		if (opt == OPT_MASTER_LCORE_NUM && !coremask_ok) {
> > +			RTE_LOG(ERR, EAL, "please specify the master lcore id"
> > +				"after specifying the coremask\n");
> > +			eal_usage(prgname);
> > +			return -1;
> > +		}
> > +
> 
> I don't really like an idea of introducing strict order between -c and  "--master-lcore..

Me too. And Aaron too :)

> Can we move check for coremask_ok/ and assignment of cfg->master_lcore out of  
> while (getopt_long(...)) loop?
> 
> >  		ret = eal_parse_common_option(opt, optarg, &internal_config);
> >  		/* common parser is not happy */
> >  		if (ret < 0) {

Yes we should move the check outside of the loop.
First we should migrate all flags check in a common function for BSD and Linux.

Simon made the v1. I made the v2. Any volunteer for the v3?

-- 
Thomas

Re: [PATCH v2] eal: add option --master-lcore

[Aaron Campbell]

Acked-by: Aaron Campbell <aaron-rd7evPjynkNeoWH0uzbU5w@public.gmane.org <mailto:aaron-rd7evPjynkNeoWH0uzbU5w@public.gmane.org>>

Minor comments inline below, I don’t need to see another patch.

Thanks,
-Aaron

> On Nov 4, 2014, at 5:40 PM, Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org> wrote:
> 
> +			RTE_LOG(ERR, EAL, "please specify the master lcore id"
> +				"after specifying the coremask\n”);

Missing a space between “id” and “after”.

> +			RTE_LOG(ERR, EAL, "please specify the master lcore id"
> +				"after specifying the coremask\n”);

Same here in the Linux version of the code.

Re: [PATCH v2] eal: add option --master-lcore

[Simon Kuenzer]

Thanks, that was quick! Quicker than me, actually. ;-)

I started to rebase and modify my previous patch as well but I got a bit 
stuck because I wanted to try to solve the command order issue that 
Aaron mentioned. In fact, I agree with him in this point. However, I 
figured out that more preceding patches might be required to get this 
proberly done.
The issue is that I would like to have all code that changes the master 
lcore placed completely in eal_common_options.c.
Something like eal_common_sanity_check() and eal_common_configure() 
functions would be needed because eal_parse_common_option() is just 
called within the option parsing loop. Maybe we can improve this later?

I am fine with this patch but I have some comments inlined.
In general: acknowledged from my side.

Thanks a lot,

Simon

On 04.11.2014 22:40, Thomas Monjalon wrote:
> From: Simon Kuenzer <simon.kuenzer-kcmmt4fgdiuHXe+LvDLADg@public.gmane.org>
>
> Enable users to specify the lcore id that is used as master lcore.
>
> Signed-off-by: Simon Kuenzer <simon.kuenzer-kcmmt4fgdiuHXe+LvDLADg@public.gmane.org>
> Signed-off-by: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
> ---
>
> changes in v2:
> - rebase on HEAD including common options for BSD and Linux
> - use strtol() instead of atoi() to check syntax errors
> - unit tests
>
>   app/test/test.c                             |  1 +
>   app/test/test_eal_flags.c                   | 49 +++++++++++++++++++++++++++++
>   lib/librte_eal/bsdapp/eal/eal.c             |  7 +++++
>   lib/librte_eal/common/eal_common_options.c  | 31 ++++++++++++++++++
>   lib/librte_eal/common/include/eal_options.h |  2 ++
>   lib/librte_eal/linuxapp/eal/eal.c           |  7 +++++
>   6 files changed, 97 insertions(+)
>
> diff --git a/app/test/test.c b/app/test/test.c
> index 9bee6bb..2fecff5 100644
> --- a/app/test/test.c
> +++ b/app/test/test.c
> @@ -82,6 +82,7 @@ do_recursive_call(void)
>   	} actions[] =  {
>   			{ "run_secondary_instances", test_mp_secondary },
>   			{ "test_missing_c_flag", no_action },
> +			{ "test_master_lcore_flag", no_action },
>   			{ "test_missing_n_flag", no_action },
>   			{ "test_no_hpet_flag", no_action },
>   			{ "test_whitelist_flag", no_action },
> diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
> index 21e6cca..45020b8 100644
> --- a/app/test/test_eal_flags.c
> +++ b/app/test/test_eal_flags.c
> @@ -520,6 +520,49 @@ test_missing_c_flag(void)
>   }
>
>   /*
> + * Test --master-lcore option with matching coremask
> + */
> +static int
> +test_master_lcore_flag(void)
> +{
> +#ifdef RTE_EXEC_ENV_BSDAPP
> +	/* BSD target doesn't support prefixes at this point */
> +	const char * prefix = "";
> +#else
> +	char prefix[PATH_MAX], tmp[PATH_MAX];
> +	if (get_current_prefix(tmp, sizeof(tmp)) == NULL) {
> +		printf("Error - unable to get current prefix!\n");
> +		return -1;
> +	}
> +	snprintf(prefix, sizeof(prefix), "--file-prefix=%s", tmp);
> +#endif
> +
> +	/* --master-lcore flag but no value */
> +	const char *argv1[] = { prgname, prefix, mp_flag, "-n", "1", "-c", "3", "--master-lcore"};
> +	/* --master-lcore flag with invalid value */
> +	const char *argv2[] = { prgname, prefix, mp_flag, "-n", "1", "-c", "3", "--master-lcore", "-1"};
> +	/* --master-lcore flag with invalid value */
> +	const char *argv3[] = { prgname, prefix, mp_flag, "-n", "1", "-c", "3", "--master-lcore", "X"};
> +	/* master lcore not in coremask */
> +	const char *argv4[] = { prgname, prefix, mp_flag, "-n", "1", "-c", "3", "--master-lcore", "2"};
> +	/* valid value */
> +	const char *argv5[] = { prgname, prefix, mp_flag, "-n", "1", "-c", "3", "--master-lcore", "1"};
> +
> +	if (launch_proc(argv1) == 0
> +			|| launch_proc(argv2) == 0
> +			|| launch_proc(argv3) == 0
> +			|| launch_proc(argv4) == 0) {
> +		printf("Error - process ran without error with wrong --master-lcore\n");
> +		return -1;
> +	}
> +	if (launch_proc(argv5) != 0) {
> +		printf("Error - process did not run ok with valid --master-lcore\n");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +/*
>    * Test that the app doesn't run without the -n flag. In all cases
>    * should give an error and fail to run.
>    * Since -n is not compulsory for MP, we instead use --no-huge and --no-shconf
> @@ -1214,6 +1257,12 @@ test_eal_flags(void)
>   		return ret;
>   	}
>
> +	ret = test_master_lcore_flag();
> +	if (ret < 0) {
> +		printf("Error in test_master_lcore_flag()\n");
> +		return ret;
> +	}
> +
>   	ret = test_missing_n_flag();
>   	if (ret < 0) {
>   		printf("Error in test_missing_n_flag()\n");
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
> index ca99cb9..c764fec 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -354,6 +354,13 @@ eal_parse_args(int argc, char **argv)
>   		if (opt == '?')
>   			return -1;
>
> +		if (opt == OPT_MASTER_LCORE_NUM && !coremask_ok) {
> +			RTE_LOG(ERR, EAL, "please specify the master lcore id"
> +				"after specifying the coremask\n");

A space is missing in this message.

> +			eal_usage(prgname);
> +			return -1;
> +		}
> +
>   		ret = eal_parse_common_option(opt, optarg, &internal_config);
>   		/* common parser is not happy */
>   		if (ret < 0) {
> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index 7a5d55e..9166ea1 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -64,6 +64,7 @@ eal_short_options[] =
>   const struct option
>   eal_long_options[] = {
>   	{OPT_HUGE_DIR, 1, 0, OPT_HUGE_DIR_NUM},
> +	{OPT_MASTER_LCORE, 1, 0, OPT_MASTER_LCORE_NUM},
>   	{OPT_PROC_TYPE, 1, 0, OPT_PROC_TYPE_NUM},
>   	{OPT_NO_SHCONF, 0, 0, OPT_NO_SHCONF_NUM},
>   	{OPT_NO_HPET, 0, 0, OPT_NO_HPET_NUM},
> @@ -163,6 +164,27 @@ eal_parse_coremask(const char *coremask)
>   	return 0;
>   }
>
> +/* Changes the lcore id of the master thread */
> +static int
> +eal_parse_master_lcore(const char *arg)
> +{
> +	long master_lcore;
> +	char *parsing_end;
> +	struct rte_config *cfg = rte_eal_get_configuration();
> +
> +	errno = 0;
> +	master_lcore = strtol(arg, &parsing_end, 0);
> +	if (errno || parsing_end == arg)
> +		return -1;
> +
> +	if (!(master_lcore >= 0 && master_lcore < RTE_MAX_LCORE))
> +		return -1;
> +	if (cfg->lcore_role[master_lcore] != ROLE_RTE)
> +		return -1;

It might be helpful to return another error code (e.g., -2) that signals 
that the specified lcore is not part of the lcore mask.

> +	cfg->master_lcore = master_lcore;
> +	return 0;
> +}
> +
>   static int
>   eal_parse_syslog(const char *facility, struct internal_config *conf)
>   {
> @@ -320,6 +342,14 @@ eal_parse_common_option(int opt, const char *optarg,
>   		conf->process_type = eal_parse_proc_type(optarg);
>   		break;
>
> +	case OPT_MASTER_LCORE_NUM:
> +		if (eal_parse_master_lcore(optarg) < 0) {
> +			RTE_LOG(ERR, EAL, "invalid parameter for --"
> +					OPT_MASTER_LCORE "\n");
> +			return -1;

It might be nice here to get another message displayed whenever the 
specified lcore is not part of the core mask (see comment ahead).

> +		}
> +		break;
> +
>   	case OPT_VDEV_NUM:
>   		if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL,
>   				optarg) < 0) {
> @@ -364,6 +394,7 @@ eal_common_usage(void)
>   	       "[--proc-type primary|secondary|auto]\n\n"
>   	       "EAL common options:\n"
>   	       "  -c COREMASK  : A hexadecimal bitmask of cores to run on\n"
> +	       "  --"OPT_MASTER_LCORE" ID: Core ID that is used as master\n"
>   	       "  -n NUM       : Number of memory channels\n"
>   	       "  -v           : Display version information on startup\n"
>   	       "  -m MB        : memory to allocate (see also --"OPT_SOCKET_MEM")\n"
> diff --git a/lib/librte_eal/common/include/eal_options.h b/lib/librte_eal/common/include/eal_options.h
> index 22819ec..deb872d 100644
> --- a/lib/librte_eal/common/include/eal_options.h
> +++ b/lib/librte_eal/common/include/eal_options.h
> @@ -43,6 +43,8 @@ enum {
>   	OPT_LONG_MIN_NUM = 256,
>   #define OPT_HUGE_DIR    "huge-dir"
>   	OPT_HUGE_DIR_NUM = OPT_LONG_MIN_NUM,
> +#define OPT_MASTER_LCORE "master-lcore"
> +	OPT_MASTER_LCORE_NUM,
>   #define OPT_PROC_TYPE   "proc-type"
>   	OPT_PROC_TYPE_NUM,
>   #define OPT_NO_SHCONF   "no-shconf"
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index 7a1d087..7fcb349 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -550,6 +550,13 @@ eal_parse_args(int argc, char **argv)
>   		if (opt == '?')
>   			return -1;
>
> +		if (opt == OPT_MASTER_LCORE_NUM && !coremask_ok) {
> +			RTE_LOG(ERR, EAL, "please specify the master lcore id"
> +				"after specifying the coremask\n");
> +			eal_usage(prgname);
> +			return -1;
> +		}
> +
>   		ret = eal_parse_common_option(opt, optarg, &internal_config);
>   		/* common parser is not happy */
>   		if (ret < 0) {
>