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

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

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.
  

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 -
  

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
  

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

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>
  

> -----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.
--
  

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 -
  

> -----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.
--
  

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.
  

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

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
  

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 -