[dpdk-dev] [PATCH] eal: default to one memory channel if not specified

[dpdk-dev] [PATCH] eal: default to one memory channel if not specified

[Panu Matilainen]

Obtaining the correct value, especially from a running system, can
be anything from difficult to plain impossible.  Since the value is
merely an optimization and does not affect functionality otherwise,
its pointless to force such a guess on users initially, such things
belong to performance tuning phase.

Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
 lib/librte_eal/common/eal_common_options.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 1f459ac..28f10a2 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -104,7 +104,7 @@ eal_reset_internal_config(struct internal_config *internal_cfg)
 
 	internal_cfg->memory = 0;
 	internal_cfg->force_nrank = 0;
-	internal_cfg->force_nchannel = 0;
+	internal_cfg->force_nchannel = 1;
 	internal_cfg->hugefile_prefix = HUGEFILE_PREFIX_DEFAULT;
 	internal_cfg->hugepage_dir = NULL;
 	internal_cfg->force_sockets = 0;
@@ -834,12 +834,6 @@ eal_check_common_options(struct internal_config *internal_cfg)
 		RTE_LOG(ERR, EAL, "Invalid process type specified\n");
 		return -1;
 	}
-	if (internal_cfg->process_type == RTE_PROC_PRIMARY &&
-			internal_cfg->force_nchannel == 0) {
-		RTE_LOG(ERR, EAL, "Number of memory channels (-n) not "
-			"specified\n");
-		return -1;
-	}
 	if (index(internal_cfg->hugefile_prefix, '%') != NULL) {
 		RTE_LOG(ERR, EAL, "Invalid char, '%%', in --"OPT_FILE_PREFIX" "
 			"option\n");
@@ -869,7 +863,7 @@ eal_check_common_options(struct internal_config *internal_cfg)
 void
 eal_common_usage(void)
 {
-	printf("-c COREMASK|-l CORELIST -n CHANNELS [options]\n\n"
+	printf("-c COREMASK|-l CORELIST [options]\n\n"
 	       "EAL common options:\n"
 	       "  -c COREMASK         Hexadecimal bitmask of cores to run on\n"
 	       "  -l CORELIST         List of cores to run on\n"
-- 
2.4.3

Re: [dpdk-dev] [PATCH] eal: default to one memory channel if not specified

[David Marchand]

Hello Panu,

On Wed, Oct 14, 2015 at 12:22 PM, Panu Matilainen <pmatilai@redhat.com>
wrote:

> Obtaining the correct value, especially from a running system, can
> be anything from difficult to plain impossible.  Since the value is
> merely an optimization and does not affect functionality otherwise,
> its pointless to force such a guess on users initially, such things
> belong to performance tuning phase.
>
> Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
> ---
>  lib/librte_eal/common/eal_common_options.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_common_options.c
> b/lib/librte_eal/common/eal_common_options.c
> index 1f459ac..28f10a2 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -104,7 +104,7 @@ eal_reset_internal_config(struct internal_config
> *internal_cfg)
>
>         internal_cfg->memory = 0;
>         internal_cfg->force_nrank = 0;
> -       internal_cfg->force_nchannel = 0;
> +       internal_cfg->force_nchannel = 1;
>

Well, not too sure about this default value.

- mempool code is already checking for the 0 value.
- API already tells for rte_memory_get_nchannel() :
 *
@return

 *   The number of memory channels on the system. The value is 0 if
unknown
 *   or not the same on all
devices.

So, I would let it 0.


-- 
David Marchand

Re: [dpdk-dev] [PATCH] eal: default to one memory channel if not specified

[Panu Matilainen]

On 10/14/2015 02:45 PM, David Marchand wrote:
> Hello Panu,
>
> On Wed, Oct 14, 2015 at 12:22 PM, Panu Matilainen <pmatilai@redhat.com
> <mailto:pmatilai@redhat.com>> wrote:
>
>     Obtaining the correct value, especially from a running system, can
>     be anything from difficult to plain impossible.  Since the value is
>     merely an optimization and does not affect functionality otherwise,
>     its pointless to force such a guess on users initially, such things
>     belong to performance tuning phase.
>
>     Signed-off-by: Panu Matilainen <pmatilai@redhat.com
>     <mailto:pmatilai@redhat.com>>
>     ---
>       lib/librte_eal/common/eal_common_options.c | 10 ++--------
>       1 file changed, 2 insertions(+), 8 deletions(-)
>
>     diff --git a/lib/librte_eal/common/eal_common_options.c
>     b/lib/librte_eal/common/eal_common_options.c
>     index 1f459ac..28f10a2 100644
>     --- a/lib/librte_eal/common/eal_common_options.c
>     +++ b/lib/librte_eal/common/eal_common_options.c
>     @@ -104,7 +104,7 @@ eal_reset_internal_config(struct internal_config
>     *internal_cfg)
>
>              internal_cfg->memory = 0;
>              internal_cfg->force_nrank = 0;
>     -       internal_cfg->force_nchannel = 0;
>     +       internal_cfg->force_nchannel = 1;
>
>
> Well, not too sure about this default value.
>
> - mempool code is already checking for the 0 value.

Yeah, I noticed it already handles the zero case.

> - API already tells for rte_memory_get_nchannel() :
>   * @return
>   *   The number of memory channels on the system. The value is 0 if
> unknown
>   *   or not the same on all devices.

...but missed this one, and thought it'd be "safer" to return some 
non-zero value since callers might be expecting it to be a valid -n value.

>
> So, I would let it 0.

Right, so just drop the default value, reword commit message accordingly 
and resend. Will do unless there are other objections.

	- Panu -

Re: [dpdk-dev] [PATCH] eal: default to one memory channel if not specified

[Bruce Richardson]

On Wed, Oct 14, 2015 at 03:04:43PM +0300, Panu Matilainen wrote:
> On 10/14/2015 02:45 PM, David Marchand wrote:
> >Hello Panu,
> >
> >On Wed, Oct 14, 2015 at 12:22 PM, Panu Matilainen <pmatilai@redhat.com
> ><mailto:pmatilai@redhat.com>> wrote:
> >
> >    Obtaining the correct value, especially from a running system, can
> >    be anything from difficult to plain impossible.  Since the value is
> >    merely an optimization and does not affect functionality otherwise,
> >    its pointless to force such a guess on users initially, such things
> >    belong to performance tuning phase.
> >
> >    Signed-off-by: Panu Matilainen <pmatilai@redhat.com
> >    <mailto:pmatilai@redhat.com>>
> >    ---
> >      lib/librte_eal/common/eal_common_options.c | 10 ++--------
> >      1 file changed, 2 insertions(+), 8 deletions(-)
> >
> >    diff --git a/lib/librte_eal/common/eal_common_options.c
> >    b/lib/librte_eal/common/eal_common_options.c
> >    index 1f459ac..28f10a2 100644
> >    --- a/lib/librte_eal/common/eal_common_options.c
> >    +++ b/lib/librte_eal/common/eal_common_options.c
> >    @@ -104,7 +104,7 @@ eal_reset_internal_config(struct internal_config
> >    *internal_cfg)
> >
> >             internal_cfg->memory = 0;
> >             internal_cfg->force_nrank = 0;
> >    -       internal_cfg->force_nchannel = 0;
> >    +       internal_cfg->force_nchannel = 1;
> >
> >
> >Well, not too sure about this default value.
> >
> >- mempool code is already checking for the 0 value.
> 
> Yeah, I noticed it already handles the zero case.
> 
> >- API already tells for rte_memory_get_nchannel() :
> >  * @return
> >  *   The number of memory channels on the system. The value is 0 if
> >unknown
> >  *   or not the same on all devices.
> 
> ...but missed this one, and thought it'd be "safer" to return some non-zero
> value since callers might be expecting it to be a valid -n value.
> 
> >
> >So, I would let it 0.
> 
> Right, so just drop the default value, reword commit message accordingly and
> resend. Will do unless there are other objections.
> 
> 	- Panu -

I was going to suggest using 4 as the default value, since the channel spreading
should work as designed on systems with either 1, 2 or 4 active channels.

However, given the zero-check inside the mempool code, maybe the default should
be set there instead of in the EAL. [I just don't think the default should be 1.]

Anyone else any other thoughts on this?

/Bruce

[dpdk-dev] [PATCH 0/2] Provide reasonable default to -n

[Panu Matilainen]

The number of memory channels is a truly obscure thing as a mandatory
command line argument when its really just an optimization.
Provide a reasonable default in mempool as suggested by Bruce Richardson
and make the -n argument optional in EAL to make DPDK that little bit
easier to use for a first-timer.

Panu Matilainen (2):
  mempool: use a better default for number of memory channels
  eal: make the -n argument optional

 lib/librte_eal/common/eal_common_options.c | 8 +-------
 lib/librte_mempool/rte_mempool.c           | 2 +-
 2 files changed, 2 insertions(+), 8 deletions(-)

-- 
2.4.3

[dpdk-dev] [PATCH 1/2] mempool: use a better default for number of memory channels

[Panu Matilainen]

Optimize for quad-channel by default, this should work well for
all the cases, better than the previous value of one anyway.

Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
 lib/librte_mempool/rte_mempool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 8e185c5..e57cbbd 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -113,7 +113,7 @@ static unsigned optimize_object_size(unsigned obj_size)
 	/* get number of channels */
 	nchan = rte_memory_get_nchannel();
 	if (nchan == 0)
-		nchan = 1;
+		nchan = 4;
 
 	nrank = rte_memory_get_nrank();
 	if (nrank == 0)
-- 
2.4.3

[dpdk-dev] [PATCH 2/2] eal: make the -n argument optional

[Panu Matilainen]

Obtaining the correct value of memory channels, especially from a
running system, can be anything from difficult to plain impossible.
Since the value is merely an optimization and does not affect functionality
otherwise, its pointless to force such a guess on users initially, such
things belong to performance tuning phase.

Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
 lib/librte_eal/common/eal_common_options.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 1f459ac..a4cdbaa 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -834,12 +834,6 @@ eal_check_common_options(struct internal_config *internal_cfg)
 		RTE_LOG(ERR, EAL, "Invalid process type specified\n");
 		return -1;
 	}
-	if (internal_cfg->process_type == RTE_PROC_PRIMARY &&
-			internal_cfg->force_nchannel == 0) {
-		RTE_LOG(ERR, EAL, "Number of memory channels (-n) not "
-			"specified\n");
-		return -1;
-	}
 	if (index(internal_cfg->hugefile_prefix, '%') != NULL) {
 		RTE_LOG(ERR, EAL, "Invalid char, '%%', in --"OPT_FILE_PREFIX" "
 			"option\n");
@@ -869,7 +863,7 @@ eal_check_common_options(struct internal_config *internal_cfg)
 void
 eal_common_usage(void)
 {
-	printf("-c COREMASK|-l CORELIST -n CHANNELS [options]\n\n"
+	printf("-c COREMASK|-l CORELIST [options]\n\n"
 	       "EAL common options:\n"
 	       "  -c COREMASK         Hexadecimal bitmask of cores to run on\n"
 	       "  -l CORELIST         List of cores to run on\n"
-- 
2.4.3

Re: [dpdk-dev] [PATCH 0/2] Provide reasonable default to -n

[David Marchand]

Hello,

On Thu, Oct 15, 2015 at 1:49 PM, Panu Matilainen <pmatilai@redhat.com>
wrote:

> The number of memory channels is a truly obscure thing as a mandatory
> command line argument when its really just an optimization.
> Provide a reasonable default in mempool as suggested by Bruce Richardson
> and make the -n argument optional in EAL to make DPDK that little bit
> easier to use for a first-timer.
>
> Panu Matilainen (2):
>   mempool: use a better default for number of memory channels
>   eal: make the -n argument optional
>
>  lib/librte_eal/common/eal_common_options.c | 8 +-------
>  lib/librte_mempool/rte_mempool.c           | 2 +-
>  2 files changed, 2 insertions(+), 8 deletions(-)
>

Looks good to me.
Thanks Panu.

Acked-by: David Marchand <david.marchand@6wind.com>


-- 
David Marchand

Re: [dpdk-dev] [PATCH 0/2] Provide reasonable default to -n

[Mcnamara, John]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Panu Matilainen
> Sent: Thursday, October 15, 2015 12:49 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 0/2] Provide reasonable default to -n
> 
> The number of memory channels is a truly obscure thing as a mandatory
> command line argument when its really just an optimization.
> Provide a reasonable default in mempool as suggested by Bruce Richardson
> and make the -n argument optional in EAL to make DPDK that little bit
> easier to use for a first-timer.

Hi,

In the Linux and FreeBSD user guides there is the following statement that will
need to change as well, either as part of this patchset, or separately:

    "The -c and the -n options are mandatory; the others are optional."

http://www.dpdk.org/doc/guides/linux_gsg/build_sample_apps.html#running-a-sample-application
http://www.dpdk.org/doc/guides/freebsd_gsg/build_sample_apps.html#running-a-sample-application

John.
-- 

Re: [dpdk-dev] [PATCH 0/2] Provide reasonable default to -n

[Panu Matilainen]

On 10/15/2015 03:10 PM, Mcnamara, John wrote:
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Panu Matilainen
>> Sent: Thursday, October 15, 2015 12:49 PM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH 0/2] Provide reasonable default to -n
>>
>> The number of memory channels is a truly obscure thing as a mandatory
>> command line argument when its really just an optimization.
>> Provide a reasonable default in mempool as suggested by Bruce Richardson
>> and make the -n argument optional in EAL to make DPDK that little bit
>> easier to use for a first-timer.
>
> Hi,
>
> In the Linux and FreeBSD user guides there is the following statement that will
> need to change as well, either as part of this patchset, or separately:
>
>      "The -c and the -n options are mandatory; the others are optional."
>
> http://www.dpdk.org/doc/guides/linux_gsg/build_sample_apps.html#running-a-sample-application
> http://www.dpdk.org/doc/guides/freebsd_gsg/build_sample_apps.html#running-a-sample-application

Sure. I was planning on going through the docs and updating them 
(separately) if the change is otherwise accepted, I suspect there are 
more than those two places needing changes.

	- Panu -

Re: [dpdk-dev] [PATCH 0/2] Provide reasonable default to -n

[Mcnamara, John]

> -----Original Message-----
> From: Panu Matilainen [mailto:pmatilai@redhat.com]
> Sent: Thursday, October 15, 2015 1:37 PM
> To: Mcnamara, John; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 0/2] Provide reasonable default to -n
> 
> Sure. I was planning on going through the docs and updating them
> (separately) if the change is otherwise accepted, I suspect there are more
> than those two places needing changes.

Hi,

Good stuff.

I counted ~ 100 places in the docs where -n is used. I don't know if they all
have to be removed. The 2 examples I gave were the only ones that I found,
in a quick scan, that explicitly say -n is required. The rest are in the
"mostly harmless" category but if you wanted to remove the majority of the
references then that is probably okay.

John.
-- 


 

Re: [dpdk-dev] [PATCH 0/2] Provide reasonable default to -n

[Thomas Monjalon]

2015-10-15 13:12, Mcnamara, John:
> From: Panu Matilainen [mailto:pmatilai@redhat.com]
> > Sure. I was planning on going through the docs and updating them
> > (separately) if the change is otherwise accepted, I suspect there are more
> > than those two places needing changes.

Actually the docs about command line are redundant and outdated.
We should try to keep them only in the startup section of the GSG (Linux and BSD).

> I counted ~ 100 places in the docs where -n is used. I don't know if they all
> have to be removed. The 2 examples I gave were the only ones that I found,
> in a quick scan, that explicitly say -n is required. The rest are in the
> "mostly harmless" category but if you wanted to remove the majority of the
> references then that is probably okay.

I think we should remove most of them to keep the doc simple and maintainable.

These patches will be applied even if the doc is not updated
because a doc rework is needed.

Re: [dpdk-dev] [PATCH 0/2] Provide reasonable default to -n

[Thomas Monjalon]

> > The number of memory channels is a truly obscure thing as a mandatory
> > command line argument when its really just an optimization.
> > Provide a reasonable default in mempool as suggested by Bruce Richardson
> > and make the -n argument optional in EAL to make DPDK that little bit
> > easier to use for a first-timer.
> >
> > Panu Matilainen (2):
> >   mempool: use a better default for number of memory channels
> >   eal: make the -n argument optional
> 
> Acked-by: David Marchand <david.marchand@6wind.com>

Applied, thanks

Re: [dpdk-dev] [PATCH 0/2] Provide reasonable default to -n

[Thomas Monjalon]

Panu,
Please use --subject-prefix 'PATCH v2' to ease patch management,
as explained here:
	http://dpdk.org/dev#send

git send-email --subject-prefix 'PATCH vX+1' --annotate --cover-letter --in-reply-to <vX-email-id>

It should appear on the cover letter and the patches.

Thanks

Re: [dpdk-dev] [PATCH 0/2] Provide reasonable default to -n

[Panu Matilainen]

On 10/26/2015 06:56 PM, Thomas Monjalon wrote:
> Panu,
> Please use --subject-prefix 'PATCH v2' to ease patch management,
> as explained here:
> 	http://dpdk.org/dev#send
>
> git send-email --subject-prefix 'PATCH vX+1' --annotate --cover-letter --in-reply-to <vX-email-id>
>
> It should appear on the cover letter and the patches.

Oh, I thought I was doing it by the book but apparently not so.
Apologies, I'll fix my ways.

	- Panu -