eal: default to one memory channel if not specified

PATCH In review Panu Matilainen 5 participants 15 messages Oct 14, 2015 – Oct 27, 2015

Share & tools

Panu Matilainen

author

2015-10-14 10:23 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

---
 lib/librte_eal/common/eal_common_options.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

lib/librte_eal/common/eal_common_options.c +2 -8

index 1f459ac..28f10a2 100644
@@ -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"

Signature · 1 line

2.4.3

David Marchand

2015-10-14 11:45 UTC #1 lore raw

Hello Panu,

Quoted 17 lines · 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 <[email protected]>
---
 lib/librte_eal/common/eal_common_options.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)
lib/librte_eal/common/eal_common_options.c +0 -0
index 1f459ac..28f10a2 100644

lib/librte_eal/common/eal_common_options.c eal_reset_internal_config(struct internal_config · line 109

Quoted 7 lines

lib/librte_eal/common/eal_common_options.c +1 -1

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

Signature · 1 line

David Marchand

Panu Matilainen

author

2015-10-14 12:04 UTC #2 lore raw

Quoted 31 lines · David Marchand

Hello Panu,
On Wed, Oct 14, 2015 at 12:22 PM, Panu Matilainen <[email protected] <mailto:[email protected]>> 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 <[email protected]
<mailto:[email protected]>>
---
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;

Quoted 3 lines

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.

Quoted 5 lines

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

Quoted 2 lines

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

2015-10-14 13:05 UTC #3 lore raw

Quoted 48 lines · Panu Matilainen

Quoted 31 lines · David Marchand
Hello Panu,
On Wed, Oct 14, 2015 at 12:22 PM, Panu Matilainen <[email protected] <mailto:[email protected]>> 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 <[email protected]
   <mailto:[email protected]>>
   ---
     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;
Quoted 3 lines
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.
Quoted 5 lines
- 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.
Quoted 1 line

Quoted 6 lines

Quoted 1 line
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

author

2015-10-15 11:49 UTC #4 lore raw

cover Provide reasonable default to -n

In reply to 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(-)

Signature · 1 line

2.4.3

Panu Matilainen

author

2015-10-15 11:49 UTC #5 lore raw

PATCH 1/2 mempool: use a better default for number of memory channels

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

Signed-off-by Panu Matilainen <[email protected]>

---
 lib/librte_mempool/rte_mempool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

lib/librte_mempool/rte_mempool.c +1 -1

index 8e185c5..e57cbbd 100644
@@ -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)

Signature · 1 line

2.4.3

Panu Matilainen

author

2015-10-15 11:49 UTC #6 lore raw

PATCH 2/2 eal: make the -n argument optional

In reply to 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 <[email protected]>

---
 lib/librte_eal/common/eal_common_options.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

lib/librte_eal/common/eal_common_options.c +1 -7

index 1f459ac..a4cdbaa 100644
@@ -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"

Signature · 1 line

2.4.3

David Marchand

Acked-by on the v1 series

2015-10-15 12:03 UTC #7 lore raw

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

In reply to Panu Matilainen

Hello,

Quoted 6 lines · 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.

Quoted 8 lines

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 <[email protected]>

Signature · 1 line

David Marchand

Thomas Monjalon

Applied

2015-10-26 16:52 UTC #8 lore raw

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

Quoted 11 lines

Quoted 9 lines
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 <[email protected]>

Applied, thanks

Mcnamara, John

2015-10-15 12:10 UTC #9 lore raw

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

In reply to Panu Matilainen

Quoted 11 lines

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Panu Matilainen
Sent: Thursday, October 15, 2015 12:49 PM
To: [email protected]
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

author

2015-10-15 12:36 UTC #10 lore raw

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

Quoted 14 lines · John

Quoted 11 lines
-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Panu Matilainen
Sent: Thursday, October 15, 2015 12:49 PM
To: [email protected]
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,

Quoted 7 lines

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 -

Mcnamara, John

2015-10-15 13:12 UTC #11 lore raw

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

Quoted 11 lines

-----Original Message-----
From: Panu Matilainen [mailto:[email protected]]
Sent: Thursday, October 15, 2015 1:37 PM
To: Mcnamara, John; [email protected]
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.

Thomas Monjalon

2015-10-26 16:51 UTC #12 lore raw

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

2015-10-15 13:12, Mcnamara, John:

Quoted 4 lines

From: Panu Matilainen [mailto:[email protected]]
Quoted 3 lines
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).

Quoted 5 lines

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

2015-10-26 16:57 UTC #13 lore raw

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

In reply to Panu Matilainen

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

author

2015-10-27 07:19 UTC #14 lore raw

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

Quoted 8 lines · 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.

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

- Panu -