[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 <***@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"

[David Marchand]

Hello Panu,

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

[Panu Matilainen]

Post by David Marchand
Hello Panu,
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.
---
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.

Post by David Marchand
* 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.

Post by David Marchand
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 -

[Bruce Richardson]

Post by Panu Matilainen

Post by David Marchand
Hello Panu,
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.
---
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.

Post by David Marchand
* 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.

Post by David Marchand
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

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

[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 <***@intel.com>
Signed-off-by: Panu Matilainen <***@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)

[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 <***@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"

[David Marchand]

Hello,

Post by 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.
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 <***@6wind.com>

[Thomas Monjalon]

Post by 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.
mempool: use a better default for number of memory channels
eal: make the -n argument optional

Applied, thanks

[Mcnamara, John]

-----Original Message-----
Sent: Thursday, October 15, 2015 12:49 PM
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.
--

[Panu Matilainen]

Post by Mcnamara, John

-----Original Message-----
Sent: Thursday, October 15, 2015 12:49 PM
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
"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 -

[Mcnamara, John]

-----Original Message-----
Sent: Thursday, October 15, 2015 1:37 PM
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

[Thomas Monjalon]

Post by Mcnamara, John

Post by Panu Matilainen
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).

Post by Mcnamara, John
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.

[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

[Panu Matilainen]

Post by Thomas Monjalon
Panu,
Please use --subject-prefix 'PATCH v2' to ease patch management,
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 -