[dpdk-dev] build: Increase the default value of RTE_MAX_LCORE

[dpdk-dev] build: Increase the default value of RTE_MAX_LCORE

[David Hunt]

This patch set increases the default value of RTE_MAX_LCORE from 128
to 512, and reduces the memory footprint of several files in the
power library, as the max lcore change would bloat the amount of
static memory they use.

[dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

[David Hunt]

Modern processors are coming with an ever increasing number of cores,
and 128 does not seem like a sensible max limit any more, especially
when you consider multi-socket systems with Hyper-Threading enabled.

This patch increases max_lcores default from 128 to 512.

Signed-off-by: David Hunt <david.hunt@intel.com>
---
 meson_options.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meson_options.txt b/meson_options.txt
index 0e92734c49..0ae03677f1 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -34,7 +34,7 @@ option('machine', type: 'string', value: 'auto', description:
        'Alias of cpu_instruction_set.')
 option('max_ethports', type: 'integer', value: 32, description:
        'maximum number of Ethernet devices')
-option('max_lcores', type: 'integer', value: 128, description:
+option('max_lcores', type: 'integer', value: 512, description:
        'maximum number of cores/threads supported by EAL')
 option('max_numa_nodes', type: 'integer', value: 32, description:
        'maximum number of NUMA nodes supported by EAL')
-- 
2.17.1

[dpdk-dev] [PATCH v1 2/6] lib/power: reduce memory footprint of acpi lib

[David Hunt]

Switch from static memory allocation of core info structs to dynamic
allocation. The library used to statically allocate max_lcores number
of lcore_power_info structs, so change this to rte_malloc as needed.
Reduces static footprint from 192K to 1K.

Desirable, especially if we're changing max_lcores default from 128 to 512.

Signed-off-by: David Hunt <david.hunt@intel.com>
---
 lib/power/power_acpi_cpufreq.c | 109 +++++++++++++++++++++++++++------
 1 file changed, 90 insertions(+), 19 deletions(-)

diff --git a/lib/power/power_acpi_cpufreq.c b/lib/power/power_acpi_cpufreq.c
index 1e8aeb8403..2fa239d02c 100644
--- a/lib/power/power_acpi_cpufreq.c
+++ b/lib/power/power_acpi_cpufreq.c
@@ -14,6 +14,7 @@
 
 #include <rte_memcpy.h>
 #include <rte_memory.h>
+#include <rte_malloc.h>
 #include <rte_string_fns.h>
 
 #include "power_acpi_cpufreq.h"
@@ -48,7 +49,7 @@ enum power_state {
  * Power info per lcore.
  */
 struct acpi_power_info {
-	unsigned int lcore_id;                   /**< Logical core id */
+	unsigned int lcore_id;               /**< Logical core id */
 	uint32_t freqs[RTE_MAX_LCORE_FREQS]; /**< Frequency array */
 	uint32_t nb_freqs;                   /**< number of available freqs */
 	FILE *f;                             /**< FD of scaling_setspeed */
@@ -59,7 +60,7 @@ struct acpi_power_info {
 	uint16_t turbo_enable;               /**< Turbo Boost enable/disable */
 } __rte_cache_aligned;
 
-static struct acpi_power_info lcore_power_info[RTE_MAX_LCORE];
+static struct acpi_power_info *lcore_power_info[RTE_MAX_LCORE] = { NULL };
 
 /**
  * It is to set specific freq for specific logical core, according to the index
@@ -248,7 +249,17 @@ power_acpi_cpufreq_init(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		lcore_power_info[lcore_id] =
+			rte_malloc(NULL, sizeof(struct acpi_power_info), 0);
+		if (lcore_power_info[lcore_id] == NULL) {
+			RTE_LOG(ERR, POWER, "Cannot allocate core %u\n",
+					lcore_id);
+			return -1;
+		}
+	}
+
+	pi = lcore_power_info[lcore_id];
 	exp_state = POWER_IDLE;
 	/* The power in use state works as a guard variable between
 	 * the CPU frequency control initialization and exit process.
@@ -320,7 +331,13 @@ power_acpi_cpufreq_exit(unsigned int lcore_id)
 				lcore_id, RTE_MAX_LCORE - 1U);
 		return -1;
 	}
-	pi = &lcore_power_info[lcore_id];
+
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	exp_state = POWER_USED;
 	/* The power in use state works as a guard variable between
 	 * the CPU frequency control initialization and exit process.
@@ -354,6 +371,9 @@ power_acpi_cpufreq_exit(unsigned int lcore_id)
 	__atomic_compare_exchange_n(&(pi->state), &exp_state, POWER_IDLE,
 				    0, __ATOMIC_RELEASE, __ATOMIC_RELAXED);
 
+	if (lcore_power_info[lcore_id] != NULL)
+		rte_free(lcore_power_info[lcore_id]);
+
 	return 0;
 
 fail:
@@ -379,7 +399,12 @@ power_acpi_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
 		return 0;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (num < pi->nb_freqs) {
 		RTE_LOG(ERR, POWER, "Buffer size is not enough\n");
 		return 0;
@@ -397,7 +422,12 @@ power_acpi_cpufreq_get_freq(unsigned int lcore_id)
 		return RTE_POWER_INVALID_FREQ_INDEX;
 	}
 
-	return lcore_power_info[lcore_id].curr_idx;
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	return lcore_power_info[lcore_id]->curr_idx;
 }
 
 int
@@ -408,7 +438,12 @@ power_acpi_cpufreq_set_freq(unsigned int lcore_id, uint32_t index)
 		return -1;
 	}
 
-	return set_freq_internal(&(lcore_power_info[lcore_id]), index);
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	return set_freq_internal(lcore_power_info[lcore_id], index);
 }
 
 int
@@ -421,7 +456,12 @@ power_acpi_cpufreq_freq_down(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (pi->curr_idx + 1 == pi->nb_freqs)
 		return 0;
 
@@ -439,7 +479,12 @@ power_acpi_cpufreq_freq_up(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (pi->curr_idx == 0 ||
 	    (pi->curr_idx == 1 && pi->turbo_available && !pi->turbo_enable))
 		return 0;
@@ -457,17 +502,17 @@ power_acpi_cpufreq_freq_max(unsigned int lcore_id)
 	}
 
 	/* Frequencies in the array are from high to low. */
-	if (lcore_power_info[lcore_id].turbo_available) {
-		if (lcore_power_info[lcore_id].turbo_enable)
+	if (lcore_power_info[lcore_id]->turbo_available) {
+		if (lcore_power_info[lcore_id]->turbo_enable)
 			/* Set to Turbo */
 			return set_freq_internal(
-					&lcore_power_info[lcore_id], 0);
+					lcore_power_info[lcore_id], 0);
 		else
 			/* Set to max non-turbo */
 			return set_freq_internal(
-					&lcore_power_info[lcore_id], 1);
+					lcore_power_info[lcore_id], 1);
 	} else
-		return set_freq_internal(&lcore_power_info[lcore_id], 0);
+		return set_freq_internal(lcore_power_info[lcore_id], 0);
 }
 
 int
@@ -480,7 +525,12 @@ power_acpi_cpufreq_freq_min(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	/* Frequencies in the array are from high to low. */
 	return set_freq_internal(pi, pi->nb_freqs - 1);
@@ -497,7 +547,12 @@ power_acpi_turbo_status(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	return pi->turbo_enable;
 }
@@ -513,7 +568,12 @@ power_acpi_enable_turbo(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	if (pi->turbo_available)
 		pi->turbo_enable = 1;
@@ -546,7 +606,12 @@ power_acpi_disable_turbo(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	 pi->turbo_enable = 0;
 
@@ -572,12 +637,18 @@ int power_acpi_get_capabilities(unsigned int lcore_id,
 		RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
 		return -1;
 	}
+
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
 	if (caps == NULL) {
 		RTE_LOG(ERR, POWER, "Invalid argument\n");
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	pi = lcore_power_info[lcore_id];
 	caps->capabilities = 0;
 	caps->turbo = !!(pi->turbo_available);
 
-- 
2.17.1

[dpdk-dev] [PATCH v1 3/6] lib/power: reduce memory footprint of pstate lib

[David Hunt]

Switch from static memory allocation of core info structs to dynamic
allocation. The library used to statically allocate max_lcores number
of lcore_power_info structs, so change this to rte_malloc as needed.
Reduces static footprint from 192K to 1K.

Desirable, especially if we're changing max_lcores default from 128 to 512.

Signed-off-by: David Hunt <david.hunt@intel.com>
---
 lib/power/power_pstate_cpufreq.c | 111 ++++++++++++++++++++++++++-----
 1 file changed, 93 insertions(+), 18 deletions(-)

diff --git a/lib/power/power_pstate_cpufreq.c b/lib/power/power_pstate_cpufreq.c
index 86f8a76e46..3e6529f589 100644
--- a/lib/power/power_pstate_cpufreq.c
+++ b/lib/power/power_pstate_cpufreq.c
@@ -16,6 +16,7 @@
 
 #include <rte_memcpy.h>
 #include <rte_memory.h>
+#include <rte_malloc.h>
 #include <rte_string_fns.h>
 
 #include "power_pstate_cpufreq.h"
@@ -76,7 +77,7 @@ struct pstate_power_info {
 } __rte_cache_aligned;
 
 
-static struct pstate_power_info lcore_power_info[RTE_MAX_LCORE];
+static struct pstate_power_info *lcore_power_info[RTE_MAX_LCORE] = { NULL };
 
 /**
  * It is to read the specific MSR.
@@ -518,7 +519,17 @@ power_pstate_cpufreq_init(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		lcore_power_info[lcore_id] =
+			rte_malloc(NULL, sizeof(struct pstate_power_info), 0);
+		if (lcore_power_info[lcore_id] == NULL) {
+			RTE_LOG(ERR, POWER, "Cannot allocate core %u\n",
+					lcore_id);
+			return -1;
+		}
+	}
+
+	pi = lcore_power_info[lcore_id];
 	exp_state = POWER_IDLE;
 	/* The power in use state works as a guard variable between
 	 * the CPU frequency control initialization and exit process.
@@ -595,7 +606,13 @@ power_pstate_cpufreq_exit(unsigned int lcore_id)
 				lcore_id, RTE_MAX_LCORE - 1U);
 		return -1;
 	}
-	pi = &lcore_power_info[lcore_id];
+
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	exp_state = POWER_USED;
 	/* The power in use state works as a guard variable between
@@ -632,6 +649,9 @@ power_pstate_cpufreq_exit(unsigned int lcore_id)
 	__atomic_compare_exchange_n(&(pi->state), &exp_state, POWER_IDLE,
 				    0, __ATOMIC_RELEASE, __ATOMIC_RELAXED);
 
+	if (lcore_power_info[lcore_id] != NULL)
+		rte_free(lcore_power_info[lcore_id]);
+
 	return 0;
 
 fail:
@@ -658,7 +678,12 @@ power_pstate_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
 		return 0;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (num < pi->nb_freqs) {
 		RTE_LOG(ERR, POWER, "Buffer size is not enough\n");
 		return 0;
@@ -676,7 +701,12 @@ power_pstate_cpufreq_get_freq(unsigned int lcore_id)
 		return RTE_POWER_INVALID_FREQ_INDEX;
 	}
 
-	return lcore_power_info[lcore_id].curr_idx;
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	return lcore_power_info[lcore_id]->curr_idx;
 }
 
 
@@ -688,7 +718,12 @@ power_pstate_cpufreq_set_freq(unsigned int lcore_id, uint32_t index)
 		return -1;
 	}
 
-	return set_freq_internal(&(lcore_power_info[lcore_id]), index);
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	return set_freq_internal(lcore_power_info[lcore_id], index);
 }
 
 int
@@ -701,7 +736,12 @@ power_pstate_cpufreq_freq_up(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (pi->curr_idx == 0 ||
 	    (pi->curr_idx == 1 && pi->turbo_available && !pi->turbo_enable))
 		return 0;
@@ -720,7 +760,12 @@ power_pstate_cpufreq_freq_down(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (pi->curr_idx + 1 == pi->nb_freqs)
 		return 0;
 
@@ -736,18 +781,23 @@ power_pstate_cpufreq_freq_max(unsigned int lcore_id)
 		return -1;
 	}
 
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
 	/* Frequencies in the array are from high to low. */
-	if (lcore_power_info[lcore_id].turbo_available) {
-		if (lcore_power_info[lcore_id].turbo_enable)
+	if (lcore_power_info[lcore_id]->turbo_available) {
+		if (lcore_power_info[lcore_id]->turbo_enable)
 			/* Set to Turbo */
 			return set_freq_internal(
-					&lcore_power_info[lcore_id], 0);
+					lcore_power_info[lcore_id], 0);
 		else
 			/* Set to max non-turbo */
 			return set_freq_internal(
-					&lcore_power_info[lcore_id], 1);
+					lcore_power_info[lcore_id], 1);
 	} else
-		return set_freq_internal(&lcore_power_info[lcore_id], 0);
+		return set_freq_internal(lcore_power_info[lcore_id], 0);
 }
 
 
@@ -761,7 +811,12 @@ power_pstate_cpufreq_freq_min(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	/* Frequencies in the array are from high to low. */
 	return set_freq_internal(pi, pi->nb_freqs - 1);
@@ -778,7 +833,12 @@ power_pstate_turbo_status(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	return pi->turbo_enable;
 }
@@ -793,7 +853,12 @@ power_pstate_enable_turbo(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	if (pi->turbo_available)
 		pi->turbo_enable = 1;
@@ -819,7 +884,12 @@ power_pstate_disable_turbo(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	pi->turbo_enable = 0;
 
@@ -851,7 +921,12 @@ int power_pstate_get_capabilities(unsigned int lcore_id,
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	caps->capabilities = 0;
 	caps->turbo = !!(pi->turbo_available);
 	caps->priority = pi->priority_core;
-- 
2.17.1

[dpdk-dev] [PATCH v1 4/6] lib/power: reduce memory footprint of cppc lib

[David Hunt]

Switch from static memory allocation of core info structs to dynamic
allocation. The library used to statically allocate max_lcores number
of lcore_power_info structs, so change this to rte_malloc as needed.
Reduces static footprint from 192K to 1K.

Desirable, especially if we're changing max_lcores default from 128 to 512.

Signed-off-by: David Hunt <david.hunt@intel.com>
---
 lib/power/power_cppc_cpufreq.c | 111 +++++++++++++++++++++++++++------
 1 file changed, 93 insertions(+), 18 deletions(-)

diff --git a/lib/power/power_cppc_cpufreq.c b/lib/power/power_cppc_cpufreq.c
index 6afd310e4e..952daf312e 100644
--- a/lib/power/power_cppc_cpufreq.c
+++ b/lib/power/power_cppc_cpufreq.c
@@ -5,6 +5,7 @@
 
 #include <rte_memcpy.h>
 #include <rte_memory.h>
+#include <rte_malloc.h>
 
 #include "power_cppc_cpufreq.h"
 #include "power_common.h"
@@ -61,7 +62,7 @@ struct cppc_power_info {
 	uint32_t freqs[RTE_MAX_LCORE_FREQS]; /**< Frequency array */
 } __rte_cache_aligned;
 
-static struct cppc_power_info lcore_power_info[RTE_MAX_LCORE];
+static struct cppc_power_info *lcore_power_info[RTE_MAX_LCORE] = { NULL };
 
 /**
  * It is to set specific freq for specific logical core, according to the index
@@ -344,7 +345,17 @@ power_cppc_cpufreq_init(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		lcore_power_info[lcore_id] =
+			rte_malloc(NULL, sizeof(struct cppc_power_info), 0);
+		if (lcore_power_info[lcore_id] == NULL) {
+			RTE_LOG(ERR, POWER, "Cannot allocate core %u\n",
+					lcore_id);
+			return -1;
+		}
+	}
+
+	pi = lcore_power_info[lcore_id];
 	exp_state = POWER_IDLE;
 	/* The power in use state works as a guard variable between
 	 * the CPU frequency control initialization and exit process.
@@ -422,7 +433,13 @@ power_cppc_cpufreq_exit(unsigned int lcore_id)
 				lcore_id, RTE_MAX_LCORE - 1U);
 		return -1;
 	}
-	pi = &lcore_power_info[lcore_id];
+
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	exp_state = POWER_USED;
 	/* The power in use state works as a guard variable between
 	 * the CPU frequency control initialization and exit process.
@@ -454,6 +471,9 @@ power_cppc_cpufreq_exit(unsigned int lcore_id)
 			"original\n", lcore_id);
 	__atomic_store_n(&(pi->state), POWER_IDLE, __ATOMIC_RELEASE);
 
+	if (lcore_power_info[lcore_id] != NULL)
+		rte_free(lcore_power_info[lcore_id]);
+
 	return 0;
 
 fail:
@@ -477,7 +497,12 @@ power_cppc_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
 		return 0;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (num < pi->nb_freqs) {
 		RTE_LOG(ERR, POWER, "Buffer size is not enough\n");
 		return 0;
@@ -495,7 +520,12 @@ power_cppc_cpufreq_get_freq(unsigned int lcore_id)
 		return RTE_POWER_INVALID_FREQ_INDEX;
 	}
 
-	return lcore_power_info[lcore_id].curr_idx;
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	return lcore_power_info[lcore_id]->curr_idx;
 }
 
 int
@@ -506,7 +536,12 @@ power_cppc_cpufreq_set_freq(unsigned int lcore_id, uint32_t index)
 		return -1;
 	}
 
-	return set_freq_internal(&(lcore_power_info[lcore_id]), index);
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	return set_freq_internal(lcore_power_info[lcore_id], index);
 }
 
 int
@@ -519,7 +554,12 @@ power_cppc_cpufreq_freq_down(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (pi->curr_idx + 1 == pi->nb_freqs)
 		return 0;
 
@@ -537,7 +577,12 @@ power_cppc_cpufreq_freq_up(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	if (pi->curr_idx == 0 || (pi->curr_idx == 1 &&
 		pi->turbo_available && !pi->turbo_enable))
 		return 0;
@@ -554,18 +599,23 @@ power_cppc_cpufreq_freq_max(unsigned int lcore_id)
 		return -1;
 	}
 
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
 	/* Frequencies in the array are from high to low. */
-	if (lcore_power_info[lcore_id].turbo_available) {
-		if (lcore_power_info[lcore_id].turbo_enable)
+	if (lcore_power_info[lcore_id]->turbo_available) {
+		if (lcore_power_info[lcore_id]->turbo_enable)
 			/* Set to Turbo */
 			return set_freq_internal(
-				&lcore_power_info[lcore_id], 0);
+				lcore_power_info[lcore_id], 0);
 		else
 			/* Set to max non-turbo */
 			return set_freq_internal(
-				&lcore_power_info[lcore_id], 1);
+				lcore_power_info[lcore_id], 1);
 	} else
-		return set_freq_internal(&lcore_power_info[lcore_id], 0);
+		return set_freq_internal(lcore_power_info[lcore_id], 0);
 }
 
 int
@@ -578,7 +628,12 @@ power_cppc_cpufreq_freq_min(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	/* Frequencies in the array are from high to low. */
 	return set_freq_internal(pi, pi->nb_freqs - 1);
@@ -594,7 +649,12 @@ power_cppc_turbo_status(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	return pi->turbo_enable;
 }
@@ -609,7 +669,12 @@ power_cppc_enable_turbo(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	if (pi->turbo_available)
 		pi->turbo_enable = 1;
@@ -645,7 +710,12 @@ power_cppc_disable_turbo(unsigned int lcore_id)
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 
 	pi->turbo_enable = 0;
 
@@ -677,7 +747,12 @@ power_cppc_get_capabilities(unsigned int lcore_id,
 		return -1;
 	}
 
-	pi = &lcore_power_info[lcore_id];
+	if (lcore_power_info[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "core %u not initialised\n", lcore_id);
+		return RTE_POWER_INVALID_FREQ_INDEX;
+	}
+
+	pi = lcore_power_info[lcore_id];
 	caps->capabilities = 0;
 	caps->turbo = !!(pi->turbo_available);
 
-- 
2.17.1

[dpdk-dev] [PATCH v1 5/6] lib/power: reduce memory footprint of channels

[David Hunt]

Switch from static memory allocation of channel pkt structs to dynamic
allocation. The library used to statically allocate max_lcores number
of rte_power_channel_packet structs, so change this to rte_malloc as
needed. Reduces static footprint from 236K to 1K.

Desirable, especially if we're changing max_lcores default from 128 to 512.

Signed-off-by: David Hunt <david.hunt@intel.com>
---
 lib/power/power_kvm_vm.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/lib/power/power_kvm_vm.c b/lib/power/power_kvm_vm.c
index ab7d4b8cee..c60e7acf16 100644
--- a/lib/power/power_kvm_vm.c
+++ b/lib/power/power_kvm_vm.c
@@ -5,6 +5,7 @@
 #include <string.h>
 
 #include <rte_log.h>
+#include <rte_malloc.h>
 
 #include "rte_power_guest_channel.h"
 #include "guest_channel.h"
@@ -13,7 +14,8 @@
 
 #define FD_PATH "/dev/virtio-ports/virtio.serial.port.poweragent"
 
-static struct rte_power_channel_packet pkt[RTE_MAX_LCORE];
+static struct rte_power_channel_packet
+		*power_channel_pkt[RTE_MAX_LCORE] = { NULL };
 
 int
 power_kvm_vm_check_supported(void)
@@ -29,8 +31,21 @@ power_kvm_vm_init(unsigned int lcore_id)
 				lcore_id, RTE_MAX_LCORE-1);
 		return -1;
 	}
-	pkt[lcore_id].command = RTE_POWER_CPU_POWER;
-	pkt[lcore_id].resource_id = lcore_id;
+
+	if (power_channel_pkt[lcore_id] == NULL) {
+		power_channel_pkt[lcore_id] =
+			rte_malloc(NULL,
+				sizeof(struct rte_power_channel_packet), 0);
+		if (power_channel_pkt[lcore_id] == NULL) {
+			RTE_LOG(ERR, POWER,
+				"Cannot allocate channel for core %u\n",
+				lcore_id);
+			return -1;
+		}
+	}
+
+	power_channel_pkt[lcore_id]->command = RTE_POWER_CPU_POWER;
+	power_channel_pkt[lcore_id]->resource_id = lcore_id;
 	return guest_channel_host_connect(FD_PATH, lcore_id);
 }
 
@@ -38,6 +53,10 @@ int
 power_kvm_vm_exit(unsigned int lcore_id)
 {
 	guest_channel_host_disconnect(lcore_id);
+
+	if (power_channel_pkt[lcore_id] != NULL)
+		rte_free(power_channel_pkt[lcore_id]);
+
 	return 0;
 }
 
@@ -78,8 +97,15 @@ send_msg(unsigned int lcore_id, uint32_t scale_direction)
 				lcore_id, RTE_MAX_LCORE-1);
 		return -1;
 	}
-	pkt[lcore_id].unit = scale_direction;
-	ret = guest_channel_send_msg(&pkt[lcore_id], lcore_id);
+
+	if (power_channel_pkt[lcore_id] == NULL) {
+		RTE_LOG(ERR, POWER, "channel for core %u not initialised\n",
+				lcore_id);
+		return -1;
+	}
+
+	power_channel_pkt[lcore_id]->unit = scale_direction;
+	ret = guest_channel_send_msg(power_channel_pkt[lcore_id], lcore_id);
 	if (ret == 0)
 		return 1;
 	RTE_LOG(DEBUG, POWER, "Error sending message: %s\n",
-- 
2.17.1

[dpdk-dev] [PATCH v1 6/6] lib/power: switch empty poll to max cores config

[David Hunt]

empty poll algorithm uses NUM_NODES as 256, but should really use
RTE_MAX_LCORE, which is configurable.

Signed-off-by: David Hunt <david.hunt@intel.com>
---
 lib/power/rte_power_empty_poll.c | 12 ++++++------
 lib/power/rte_power_empty_poll.h |  4 +---
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/lib/power/rte_power_empty_poll.c b/lib/power/rte_power_empty_poll.c
index 975aa92997..fe20b9f7b2 100644
--- a/lib/power/rte_power_empty_poll.c
+++ b/lib/power/rte_power_empty_poll.c
@@ -373,7 +373,7 @@ rte_empty_poll_detection(struct rte_timer *tim, void *arg)
 
 	RTE_SET_USED(arg);
 
-	for (i = 0; i < NUM_NODES; i++) {
+	for (i = 0; i < RTE_MAX_LCORE; i++) {
 
 		poll_stats = &(ep_params->wrk_data.wrk_stats[i]);
 
@@ -436,7 +436,7 @@ rte_power_empty_poll_stat_init(struct ep_params **eptr, uint8_t *freq_tlb,
 	*eptr = ep_params;
 
 	/* initialize all wrk_stats state */
-	for (i = 0; i < NUM_NODES; i++) {
+	for (i = 0; i < RTE_MAX_LCORE; i++) {
 
 		if (rte_lcore_is_enabled(i) == 0)
 			continue;
@@ -476,7 +476,7 @@ rte_power_empty_poll_stat_update(unsigned int lcore_id)
 {
 	struct priority_worker *poll_stats;
 
-	if (lcore_id >= NUM_NODES)
+	if (lcore_id >= RTE_MAX_LCORE)
 		return -1;
 
 	poll_stats = &(ep_params->wrk_data.wrk_stats[lcore_id]);
@@ -495,7 +495,7 @@ rte_power_poll_stat_update(unsigned int lcore_id, uint8_t nb_pkt)
 
 	struct priority_worker *poll_stats;
 
-	if (lcore_id >= NUM_NODES)
+	if (lcore_id >= RTE_MAX_LCORE)
 		return -1;
 
 	poll_stats = &(ep_params->wrk_data.wrk_stats[lcore_id]);
@@ -514,7 +514,7 @@ rte_power_empty_poll_stat_fetch(unsigned int lcore_id)
 {
 	struct priority_worker *poll_stats;
 
-	if (lcore_id >= NUM_NODES)
+	if (lcore_id >= RTE_MAX_LCORE)
 		return -1;
 
 	poll_stats = &(ep_params->wrk_data.wrk_stats[lcore_id]);
@@ -530,7 +530,7 @@ rte_power_poll_stat_fetch(unsigned int lcore_id)
 {
 	struct priority_worker *poll_stats;
 
-	if (lcore_id >= NUM_NODES)
+	if (lcore_id >= RTE_MAX_LCORE)
 		return -1;
 
 	poll_stats = &(ep_params->wrk_data.wrk_stats[lcore_id]);
diff --git a/lib/power/rte_power_empty_poll.h b/lib/power/rte_power_empty_poll.h
index 6ba0a37074..22678b4f4b 100644
--- a/lib/power/rte_power_empty_poll.h
+++ b/lib/power/rte_power_empty_poll.h
@@ -31,8 +31,6 @@ extern "C" {
 
 #define NUM_PRIORITIES          2
 
-#define NUM_NODES         256  /* Max core number*/
-
 /* Processor Power State */
 enum freq_val {
 	LOW,
@@ -98,7 +96,7 @@ struct priority_worker {
 
 struct stats_data {
 
-	struct priority_worker wrk_stats[NUM_NODES];
+	struct priority_worker wrk_stats[RTE_MAX_LCORE];
 
 	/* flag to stop rx threads processing packets until training over */
 	bool start_rx;
-- 
2.17.1

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

[Bruce Richardson]

On Thu, Sep 09, 2021 at 02:45:06PM +0100, David Hunt wrote:
> Modern processors are coming with an ever increasing number of cores,
> and 128 does not seem like a sensible max limit any more, especially
> when you consider multi-socket systems with Hyper-Threading enabled.
> 
> This patch increases max_lcores default from 128 to 512.
> 
> Signed-off-by: David Hunt <david.hunt@intel.com>
> ---
Thanks Dave, I think this is needed for future-proofing. Question is,
though, is 512 enough, or should we push it higher to 768 or 1k even?

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

[David Marchand]

On Thu, Sep 9, 2021 at 4:38 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Thu, Sep 09, 2021 at 02:45:06PM +0100, David Hunt wrote:
> > Modern processors are coming with an ever increasing number of cores,
> > and 128 does not seem like a sensible max limit any more, especially
> > when you consider multi-socket systems with Hyper-Threading enabled.
> >
> > This patch increases max_lcores default from 128 to 512.
> >
> > Signed-off-by: David Hunt <david.hunt@intel.com>

Why should we need this?

--lcores makes it possible to pin 128 lcores to any physical core on
your system.
And for applications that have their own thread management, they can
pin thread, then use rte_thread_register.

Do you have applications that require more than 128 lcores?


-- 
David Marchand

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

[Bruce Richardson]

On Fri, Sep 10, 2021 at 08:51:04AM +0200, David Marchand wrote:
> On Thu, Sep 9, 2021 at 4:38 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Thu, Sep 09, 2021 at 02:45:06PM +0100, David Hunt wrote:
> > > Modern processors are coming with an ever increasing number of cores,
> > > and 128 does not seem like a sensible max limit any more, especially
> > > when you consider multi-socket systems with Hyper-Threading enabled.
> > >
> > > This patch increases max_lcores default from 128 to 512.
> > >
> > > Signed-off-by: David Hunt <david.hunt@intel.com>
> 
> Why should we need this?
> 
> --lcores makes it possible to pin 128 lcores to any physical core on
> your system.
> And for applications that have their own thread management, they can
> pin thread, then use rte_thread_register.
> 
> Do you have applications that require more than 128 lcores?
>
The trouble is that using the --lcores syntax for mapping high core numbers
to low lcore ids is much more awkward to use. Every case of DPDK use I've
seen uses -c with a coremask, or -l with just giving a few core numbers on
it. This simple scheme won't work with core numbers greater than 128, and
there are already systems available with more than that number of cores.

Apart from the memory footprint issues - which this patch is already making
a good start in addressing, why would we not increase the default
max_lcores to that seen on real systems?

/Bruce

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

[David Marchand]

On Fri, Sep 10, 2021 at 9:54 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Fri, Sep 10, 2021 at 08:51:04AM +0200, David Marchand wrote:
> > On Thu, Sep 9, 2021 at 4:38 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Thu, Sep 09, 2021 at 02:45:06PM +0100, David Hunt wrote:
> > > > Modern processors are coming with an ever increasing number of cores,
> > > > and 128 does not seem like a sensible max limit any more, especially
> > > > when you consider multi-socket systems with Hyper-Threading enabled.
> > > >
> > > > This patch increases max_lcores default from 128 to 512.
> > > >
> > > > Signed-off-by: David Hunt <david.hunt@intel.com>
> >
> > Why should we need this?
> >
> > --lcores makes it possible to pin 128 lcores to any physical core on
> > your system.
> > And for applications that have their own thread management, they can
> > pin thread, then use rte_thread_register.
> >
> > Do you have applications that require more than 128 lcores?
> >
> The trouble is that using the --lcores syntax for mapping high core numbers
> to low lcore ids is much more awkward to use. Every case of DPDK use I've
> seen uses -c with a coremask, or -l with just giving a few core numbers on
> it. This simple scheme won't work with core numbers greater than 128, and
> there are already systems available with more than that number of cores.
>
> Apart from the memory footprint issues - which this patch is already making
> a good start in addressing, why would we not increase the default
> max_lcores to that seen on real systems?

The memory footprint is a major issue to me, and reserving all those
lcores won't be needed in any system.
We will also have to decide on a "640k ought to be enough" value to
avoid ABI issue with the next processor that comes out and has more
than 512 cores.

Could we wire the -c / -l options to --lcores behavior ?
It breaks the 1:1 lcore/physical core assumption, but it solves your
usability issue.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

[Thomas Monjalon]

10/09/2021 10:06, David Marchand:
> On Fri, Sep 10, 2021 at 9:54 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Fri, Sep 10, 2021 at 08:51:04AM +0200, David Marchand wrote:
> > > On Thu, Sep 9, 2021 at 4:38 PM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > >
> > > > On Thu, Sep 09, 2021 at 02:45:06PM +0100, David Hunt wrote:
> > > > > Modern processors are coming with an ever increasing number of cores,
> > > > > and 128 does not seem like a sensible max limit any more, especially
> > > > > when you consider multi-socket systems with Hyper-Threading enabled.
> > > > >
> > > > > This patch increases max_lcores default from 128 to 512.
> > > > >
> > > > > Signed-off-by: David Hunt <david.hunt@intel.com>
> > >
> > > Why should we need this?
> > >
> > > --lcores makes it possible to pin 128 lcores to any physical core on
> > > your system.
> > > And for applications that have their own thread management, they can
> > > pin thread, then use rte_thread_register.
> > >
> > > Do you have applications that require more than 128 lcores?
> > >
> > The trouble is that using the --lcores syntax for mapping high core numbers
> > to low lcore ids is much more awkward to use. Every case of DPDK use I've
> > seen uses -c with a coremask, or -l with just giving a few core numbers on
> > it. This simple scheme won't work with core numbers greater than 128, and
> > there are already systems available with more than that number of cores.
> >
> > Apart from the memory footprint issues - which this patch is already making
> > a good start in addressing, why would we not increase the default
> > max_lcores to that seen on real systems?
> 
> The memory footprint is a major issue to me, and reserving all those
> lcores won't be needed in any system.
> We will also have to decide on a "640k ought to be enough" value to
> avoid ABI issue with the next processor that comes out and has more
> than 512 cores.
> 
> Could we wire the -c / -l options to --lcores behavior ?
> It breaks the 1:1 lcore/physical core assumption, but it solves your
> usability issue.

Why would we change existing options while we already have an option
(--lcores) which solves the issue above?
I think the only issue is to educate users.
Is there something to improve in the documentation?

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

[David Hunt]

On 10/9/2021 9:24 AM, Thomas Monjalon wrote:
> 10/09/2021 10:06, David Marchand:
>> On Fri, Sep 10, 2021 at 9:54 AM Bruce Richardson
>> <bruce.richardson@intel.com> wrote:
>>> On Fri, Sep 10, 2021 at 08:51:04AM +0200, David Marchand wrote:
>>>> On Thu, Sep 9, 2021 at 4:38 PM Bruce Richardson
>>>> <bruce.richardson@intel.com> wrote:
>>>>> On Thu, Sep 09, 2021 at 02:45:06PM +0100, David Hunt wrote:
>>>>>> Modern processors are coming with an ever increasing number of cores,
>>>>>> and 128 does not seem like a sensible max limit any more, especially
>>>>>> when you consider multi-socket systems with Hyper-Threading enabled.
>>>>>>
>>>>>> This patch increases max_lcores default from 128 to 512.
>>>>>>
>>>>>> Signed-off-by: David Hunt <david.hunt@intel.com>
>>>> Why should we need this?
>>>>
>>>> --lcores makes it possible to pin 128 lcores to any physical core on
>>>> your system.
>>>> And for applications that have their own thread management, they can
>>>> pin thread, then use rte_thread_register.
>>>>
>>>> Do you have applications that require more than 128 lcores?
>>>>
>>> The trouble is that using the --lcores syntax for mapping high core numbers
>>> to low lcore ids is much more awkward to use. Every case of DPDK use I've
>>> seen uses -c with a coremask, or -l with just giving a few core numbers on
>>> it. This simple scheme won't work with core numbers greater than 128, and
>>> there are already systems available with more than that number of cores.
>>>
>>> Apart from the memory footprint issues - which this patch is already making
>>> a good start in addressing, why would we not increase the default
>>> max_lcores to that seen on real systems?
>> The memory footprint is a major issue to me, and reserving all those
>> lcores won't be needed in any system.
>> We will also have to decide on a "640k ought to be enough" value to
>> avoid ABI issue with the next processor that comes out and has more
>> than 512 cores.
>>
>> Could we wire the -c / -l options to --lcores behavior ?
>> It breaks the 1:1 lcore/physical core assumption, but it solves your
>> usability issue.
> Why would we change existing options while we already have an option
> (--lcores) which solves the issue above?
> I think the only issue is to educate users.
> Is there something to improve in the documentation?
>

Hi all,
I agree that it’s a good idea to switch to using the “--lcrores” option 
for cores above the default, that’s already future proofed.
However, I’m still a little concerned about usability, if our users are 
accustomed to the “-c” and “-l” options, I suggest that we add a warning 
to suggest using the “--lcores” option if any of the cores provided on 
the command line are above RTE_MAX_LCORE. That would help them with the 
solution to using physical cores above 128 (or whatever the compiled 
default is).

Example:

“ERROR: logical core 212 is above the maximum lcore number permitted.
Please use the --lcores option to map lcores onto physical cores, e.g. 
--lcores="(0-3)@(212-215).”

I’ll replace the first patch in the set with a patch that adds the 
additional information in the error message.

Thanks,
Dave.

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

[David Marchand]

On Tue, Sep 14, 2021 at 11:34 AM David Hunt <david.hunt@intel.com> wrote:
>
>
> On 10/9/2021 9:24 AM, Thomas Monjalon wrote:
> > 10/09/2021 10:06, David Marchand:
> >> On Fri, Sep 10, 2021 at 9:54 AM Bruce Richardson
> >> <bruce.richardson@intel.com> wrote:
> >>> On Fri, Sep 10, 2021 at 08:51:04AM +0200, David Marchand wrote:
> >>>> On Thu, Sep 9, 2021 at 4:38 PM Bruce Richardson
> >>>> <bruce.richardson@intel.com> wrote:
> >>>>> On Thu, Sep 09, 2021 at 02:45:06PM +0100, David Hunt wrote:
> >>>>>> Modern processors are coming with an ever increasing number of cores,
> >>>>>> and 128 does not seem like a sensible max limit any more, especially
> >>>>>> when you consider multi-socket systems with Hyper-Threading enabled.
> >>>>>>
> >>>>>> This patch increases max_lcores default from 128 to 512.
> >>>>>>
> >>>>>> Signed-off-by: David Hunt <david.hunt@intel.com>
> >>>> Why should we need this?
> >>>>
> >>>> --lcores makes it possible to pin 128 lcores to any physical core on
> >>>> your system.
> >>>> And for applications that have their own thread management, they can
> >>>> pin thread, then use rte_thread_register.
> >>>>
> >>>> Do you have applications that require more than 128 lcores?
> >>>>
> >>> The trouble is that using the --lcores syntax for mapping high core numbers
> >>> to low lcore ids is much more awkward to use. Every case of DPDK use I've
> >>> seen uses -c with a coremask, or -l with just giving a few core numbers on
> >>> it. This simple scheme won't work with core numbers greater than 128, and
> >>> there are already systems available with more than that number of cores.
> >>>
> >>> Apart from the memory footprint issues - which this patch is already making
> >>> a good start in addressing, why would we not increase the default
> >>> max_lcores to that seen on real systems?
> >> The memory footprint is a major issue to me, and reserving all those
> >> lcores won't be needed in any system.
> >> We will also have to decide on a "640k ought to be enough" value to
> >> avoid ABI issue with the next processor that comes out and has more
> >> than 512 cores.
> >>
> >> Could we wire the -c / -l options to --lcores behavior ?
> >> It breaks the 1:1 lcore/physical core assumption, but it solves your
> >> usability issue.
> > Why would we change existing options while we already have an option
> > (--lcores) which solves the issue above?
> > I think the only issue is to educate users.
> > Is there something to improve in the documentation?
> >
>
> Hi all,
> I agree that it’s a good idea to switch to using the “--lcrores” option

Let's avoid typo in the error message you'll add :-).


> for cores above the default, that’s already future proofed.
> However, I’m still a little concerned about usability, if our users are
> accustomed to the “-c” and “-l” options, I suggest that we add a warning
> to suggest using the “--lcores” option if any of the cores provided on
> the command line are above RTE_MAX_LCORE. That would help them with the
> solution to using physical cores above 128 (or whatever the compiled
> default is).
>
> Example:
>
> “ERROR: logical core 212 is above the maximum lcore number permitted.
> Please use the --lcores option to map lcores onto physical cores, e.g.
> --lcores="(0-3)@(212-215).”

If you could directly provide the right --lcores syntax based on what
user provided with -c or -l, it would be even better.
This should be not that difficult.


>
> I’ll replace the first patch in the set with a patch that adds the
> additional information in the error message.



-- 
David Marchand

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

[David Hunt]

On 14/9/2021 11:00 AM, David Marchand wrote:
> On Tue, Sep 14, 2021 at 11:34 AM David Hunt <david.hunt@intel.com> wrote:
>>
>> On 10/9/2021 9:24 AM, Thomas Monjalon wrote:
>>> 10/09/2021 10:06, David Marchand:
>>>> On Fri, Sep 10, 2021 at 9:54 AM Bruce Richardson
>>>> <bruce.richardson@intel.com> wrote:
>>>>> On Fri, Sep 10, 2021 at 08:51:04AM +0200, David Marchand wrote:
>>>>>> On Thu, Sep 9, 2021 at 4:38 PM Bruce Richardson
>>>>>> <bruce.richardson@intel.com> wrote:
>>>>>>> On Thu, Sep 09, 2021 at 02:45:06PM +0100, David Hunt wrote:
>>>>>>>> Modern processors are coming with an ever increasing number of cores,
>>>>>>>> and 128 does not seem like a sensible max limit any more, especially
>>>>>>>> when you consider multi-socket systems with Hyper-Threading enabled.
>>>>>>>>
>>>>>>>> This patch increases max_lcores default from 128 to 512.
>>>>>>>>
>>>>>>>> Signed-off-by: David Hunt <david.hunt@intel.com>
>>>>>> Why should we need this?
>>>>>>
>>>>>> --lcores makes it possible to pin 128 lcores to any physical core on
>>>>>> your system.
>>>>>> And for applications that have their own thread management, they can
>>>>>> pin thread, then use rte_thread_register.
>>>>>>
>>>>>> Do you have applications that require more than 128 lcores?
>>>>>>
>>>>> The trouble is that using the --lcores syntax for mapping high core numbers
>>>>> to low lcore ids is much more awkward to use. Every case of DPDK use I've
>>>>> seen uses -c with a coremask, or -l with just giving a few core numbers on
>>>>> it. This simple scheme won't work with core numbers greater than 128, and
>>>>> there are already systems available with more than that number of cores.
>>>>>
>>>>> Apart from the memory footprint issues - which this patch is already making
>>>>> a good start in addressing, why would we not increase the default
>>>>> max_lcores to that seen on real systems?
>>>> The memory footprint is a major issue to me, and reserving all those
>>>> lcores won't be needed in any system.
>>>> We will also have to decide on a "640k ought to be enough" value to
>>>> avoid ABI issue with the next processor that comes out and has more
>>>> than 512 cores.
>>>>
>>>> Could we wire the -c / -l options to --lcores behavior ?
>>>> It breaks the 1:1 lcore/physical core assumption, but it solves your
>>>> usability issue.
>>> Why would we change existing options while we already have an option
>>> (--lcores) which solves the issue above?
>>> I think the only issue is to educate users.
>>> Is there something to improve in the documentation?
>>>
>> Hi all,
>> I agree that it’s a good idea to switch to using the “--lcrores” option
> Let's avoid typo in the error message you'll add :-).
>
>
>> for cores above the default, that’s already future proofed.
>> However, I’m still a little concerned about usability, if our users are
>> accustomed to the “-c” and “-l” options, I suggest that we add a warning
>> to suggest using the “--lcores” option if any of the cores provided on
>> the command line are above RTE_MAX_LCORE. That would help them with the
>> solution to using physical cores above 128 (or whatever the compiled
>> default is).
>>
>> Example:
>>
>> “ERROR: logical core 212 is above the maximum lcore number permitted.
>> Please use the --lcores option to map lcores onto physical cores, e.g.
>> --lcores="(0-3)@(212-215).”
> If you could directly provide the right --lcores syntax based on what
> user provided with -c or -l, it would be even better.
> This should be not that difficult.


Agreed. I now have something working that when given "-l 12-16,130,132", 
will output the following:

EAL: One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
EAL: Please use --lcores instead, e.g. --lcores "(0-6)@(12-16,130,132)"

So you can just cut-and-paste that option into your command line. Makes 
it very easy for users to migrate.


>
>> I’ll replace the first patch in the set with a patch that adds the
>> additional information in the error message.
>
>

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

[David Marchand]

On Tue, Sep 14, 2021 at 1:07 PM David Hunt <david.hunt@intel.com> wrote:
> >> “ERROR: logical core 212 is above the maximum lcore number permitted.
> >> Please use the --lcores option to map lcores onto physical cores, e.g.
> >> --lcores="(0-3)@(212-215).”
> > If you could directly provide the right --lcores syntax based on what
> > user provided with -c or -l, it would be even better.
> > This should be not that difficult.
>
>
> Agreed. I now have something working that when given "-l 12-16,130,132",
> will output the following:
>
> EAL: One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
> EAL: Please use --lcores instead, e.g. --lcores "(0-6)@(12-16,130,132)"

That's not equivalent.

(0-6)@(12-16,130,132) means 7 lcores with each lcore running on the
same group of physical cores.
-l 12-16,130,132 means 7 lcores running on dedicated physical cores.
I would expect 0@12,1@13,2@14,3@15,4@16,5@130,6@132


You can see with debug logs:

$ echo quit | ./build/app/dpdk-testpmd --log-level=*:debug --no-huge
-m 512 --lcores '(0-2)@(0-2)' -- --total-num-mbufs 2048 |& grep
lcore.*is.ready
EAL: Main lcore 0 is ready (tid=7feb9550bc00;cpuset=[0,1,2])
EAL: lcore 1 is ready (tid=7feb909ce700;cpuset=[0,1,2])
EAL: lcore 2 is ready (tid=7feb901cd700;cpuset=[0,1,2])

vs

$ echo quit | ./build/app/dpdk-testpmd --log-level=*:debug --no-huge
-m 512 --lcores 0@0,1@1,2@2 -- --total-num-mbufs 2048 |& grep
lcore.*is.ready
EAL: Main lcore 0 is ready (tid=7fba1cd1ac00;cpuset=[0])
EAL: lcore 2 is ready (tid=7fba179dc700;cpuset=[2])
EAL: lcore 1 is ready (tid=7fba181dd700;cpuset=[1])


-- 
David Marchand

[dpdk-dev] [PATCH v2] eal: add additional info if lcore exceeds max cores

[David Hunt]

If the user requests to use an lcore above 128 using -l or -c,
the eal will exit with "EAL: invalid core list syntax" and
very little other useful information.

This patch adds some extra information suggesting to use --lcores
so that physical cores above RTE_MAX_LCORE (default 128) can be
used. This is achieved by using the --lcores option by mapping
the logical cores in the application onto to physical cores.

There is no change in functionalty, just additional messages
suggesting how the --lcores option might be used for the supplied
list of lcores. For example, if "-l 12-14,130,132" is used, we
see the following additional output on the command line:

EAL: Error = One of the 5 cores provided exceeds RTE_MAX_LCORE (128)
EAL: Please use --lcores instead, e.g. --lcores 0@12,1@13,2@14,3@130,4@132

Signed-off-by: David Hunt <david.hunt@intel.com>

---
changes in v2
   * Rather than increasing the default max lcores (as in v1),
     it was agreed to do this instead (switch to --lcores).
   * As the other patches in the v1 of the set are no longer related
     to this change, I'll submit as a separate patch set.
---
 lib/eal/common/eal_common_options.c | 31 +++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
index ff5861b5f3..5c7a5a45a5 100644
--- a/lib/eal/common/eal_common_options.c
+++ b/lib/eal/common/eal_common_options.c
@@ -836,6 +836,8 @@ eal_parse_service_corelist(const char *corelist)
 	return 0;
 }
 
+#define MAX_LCORES_STRING 512
+
 static int
 eal_parse_corelist(const char *corelist, int *cores)
 {
@@ -843,6 +845,9 @@ eal_parse_corelist(const char *corelist, int *cores)
 	char *end = NULL;
 	int min, max;
 	int idx;
+	bool overflow = false;
+	char lcores[MAX_LCORES_STRING] = "";
+	int len = 0;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
@@ -862,8 +867,10 @@ eal_parse_corelist(const char *corelist, int *cores)
 		idx = strtol(corelist, &end, 10);
 		if (errno || end == NULL)
 			return -1;
-		if (idx < 0 || idx >= RTE_MAX_LCORE)
+		if (idx < 0)
 			return -1;
+		if (idx >= RTE_MAX_LCORE)
+			overflow = true;
 		while (isblank(*end))
 			end++;
 		if (*end == '-') {
@@ -873,10 +880,19 @@ eal_parse_corelist(const char *corelist, int *cores)
 			if (min == RTE_MAX_LCORE)
 				min = idx;
 			for (idx = min; idx <= max; idx++) {
-				if (cores[idx] == -1) {
-					cores[idx] = count;
-					count++;
+				if (idx < RTE_MAX_LCORE) {
+					if (cores[idx] == -1)
+						cores[idx] = count;
 				}
+				count++;
+				if (count == 1)
+					len = len + snprintf(&lcores[len],
+							MAX_LCORES_STRING - len,
+							"%d@%d", count-1, idx);
+				else
+					len = len + snprintf(&lcores[len],
+							MAX_LCORES_STRING - len,
+							",%d@%d", count-1, idx);
 			}
 			min = RTE_MAX_LCORE;
 		} else
@@ -886,6 +902,13 @@ eal_parse_corelist(const char *corelist, int *cores)
 
 	if (count == 0)
 		return -1;
+	if (overflow) {
+		RTE_LOG(ERR, EAL, "Error = One of the %d cores provided exceeds RTE_MAX_LCORE (%d)\n",
+				count, RTE_MAX_LCORE);
+		RTE_LOG(ERR, EAL, "Please use --lcores instead, e.g. --lcores %s\n",
+				lcores);
+		return -1;
+	}
 	return 0;
 }
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

[David Hunt]

On 14/9/2021 12:29 PM, David Marchand wrote:
> On Tue, Sep 14, 2021 at 1:07 PM David Hunt <david.hunt@intel.com> wrote:
>>>> “ERROR: logical core 212 is above the maximum lcore number permitted.
>>>> Please use the --lcores option to map lcores onto physical cores, e.g.
>>>> --lcores="(0-3)@(212-215).”
>>> If you could directly provide the right --lcores syntax based on what
>>> user provided with -c or -l, it would be even better.
>>> This should be not that difficult.
>>
>> Agreed. I now have something working that when given "-l 12-16,130,132",
>> will output the following:
>>
>> EAL: One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
>> EAL: Please use --lcores instead, e.g. --lcores "(0-6)@(12-16,130,132)"
> That's not equivalent.
>
> (0-6)@(12-16,130,132) means 7 lcores with each lcore running on the
> same group of physical cores.
> -l 12-16,130,132 means 7 lcores running on dedicated physical cores.
> I would expect 0@12,1@13,2@14,3@15,4@16,5@130,6@132
>
>
> You can see with debug logs:
>
> $ echo quit | ./build/app/dpdk-testpmd --log-level=*:debug --no-huge
> -m 512 --lcores '(0-2)@(0-2)' -- --total-num-mbufs 2048 |& grep
> lcore.*is.ready
> EAL: Main lcore 0 is ready (tid=7feb9550bc00;cpuset=[0,1,2])
> EAL: lcore 1 is ready (tid=7feb909ce700;cpuset=[0,1,2])
> EAL: lcore 2 is ready (tid=7feb901cd700;cpuset=[0,1,2])
>
> vs
>
> $ echo quit | ./build/app/dpdk-testpmd --log-level=*:debug --no-huge
> -m 512 --lcores 0@0,1@1,2@2 -- --total-num-mbufs 2048 |& grep
> lcore.*is.ready
> EAL: Main lcore 0 is ready (tid=7fba1cd1ac00;cpuset=[0])
> EAL: lcore 2 is ready (tid=7fba179dc700;cpuset=[2])
> EAL: lcore 1 is ready (tid=7fba181dd700;cpuset=[1])
>

Hi David,

    Thanks for the clarification. I've made the relevant changes and 
submitted a v2. Hopefully the suggested parameters are correct this time! :)

Regards,
Dave.

Re: [dpdk-dev] [PATCH v2] eal: add additional info if lcore exceeds max cores

[David Marchand]

On Wed, Sep 15, 2021 at 2:11 PM David Hunt <david.hunt@intel.com> wrote:
>
> If the user requests to use an lcore above 128 using -l or -c,
> the eal will exit with "EAL: invalid core list syntax" and
> very little other useful information.
>
> This patch adds some extra information suggesting to use --lcores
> so that physical cores above RTE_MAX_LCORE (default 128) can be
> used. This is achieved by using the --lcores option by mapping
> the logical cores in the application onto to physical cores.
>
> There is no change in functionalty, just additional messages
> suggesting how the --lcores option might be used for the supplied
> list of lcores. For example, if "-l 12-14,130,132" is used, we
> see the following additional output on the command line:
>
> EAL: Error = One of the 5 cores provided exceeds RTE_MAX_LCORE (128)
> EAL: Please use --lcores instead, e.g. --lcores 0@12,1@13,2@14,3@130,4@132
>
> Signed-off-by: David Hunt <david.hunt@intel.com>
>
> ---
> changes in v2
>    * Rather than increasing the default max lcores (as in v1),
>      it was agreed to do this instead (switch to --lcores).
>    * As the other patches in the v1 of the set are no longer related
>      to this change, I'll submit as a separate patch set.

The -c option can use the same kind of warning.


> ---
>  lib/eal/common/eal_common_options.c | 31 +++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
> index ff5861b5f3..5c7a5a45a5 100644
> --- a/lib/eal/common/eal_common_options.c
> +++ b/lib/eal/common/eal_common_options.c
> @@ -836,6 +836,8 @@ eal_parse_service_corelist(const char *corelist)
>         return 0;
>  }
>
> +#define MAX_LCORES_STRING 512
> +
>  static int
>  eal_parse_corelist(const char *corelist, int *cores)
>  {
> @@ -843,6 +845,9 @@ eal_parse_corelist(const char *corelist, int *cores)
>         char *end = NULL;
>         int min, max;
>         int idx;
> +       bool overflow = false;
> +       char lcores[MAX_LCORES_STRING] = "";

This code is not performance sensitive.
In the worst case, like for RTE_MAX_LCORES lcores, it gives this:
0@0,1@1,2@2,3@3,4@4,5@5,6@6,7@7,8@8,9@9,10@10,11@11,12@12,13@13,14@14,15@15,16@16,17@17,18@18,19@19,20@20,21@21,22@22,23@23,24@24,25@25,26@26,27@27,28@28,29@29,30@30,31@31,32@32,33@33,34@34,35@35,36@36,37@37,38@38,39@39,40@40,41@41,42@42,43@43,44@44,45@45,46@46,47@47,48@48,49@49,50@50,51@51,52@52,53@53,54@54,55@55,56@56,57@57,58@58,59@59,60@60,61@61,62@62,63@63,64@64,65@65,66@66,67@67,68@68,69@69,70@70,71@71,72@72,73@73,74@74,75@75,76@76,77@77,78@78,79@79,80@80,81@81,82@82,83@83,84@84,85@85,86@86,87@87,88@88,89@89,90@90,91@91,92@92,93@93,94@94,95@95,96@96,97@97,98@98,99@99,100@100,101@101,102@102,103@103,104@104,105@105,106@106,107@107,108@108,109@109,110@110,111@111,112@112,113@113,114@114,115@115,116@116,117@117,118@118,119@119,120@120,121@121,122@122,123@123,124@124,125@125,126@126,127@127,

Which is 800+ bytes long, let's switch do dynamic allocations.



> +       int len = 0;
>
>         for (idx = 0; idx < RTE_MAX_LCORE; idx++)
>                 cores[idx] = -1;
> @@ -862,8 +867,10 @@ eal_parse_corelist(const char *corelist, int *cores)
>                 idx = strtol(corelist, &end, 10);
>                 if (errno || end == NULL)
>                         return -1;
> -               if (idx < 0 || idx >= RTE_MAX_LCORE)
> +               if (idx < 0)
>                         return -1;
> +               if (idx >= RTE_MAX_LCORE)
> +                       overflow = true;

The code before was intermixing parsing and validation of values.
This intermix was not that great.
Let's separate those concerns.


>                 while (isblank(*end))
>                         end++;
>                 if (*end == '-') {
> @@ -873,10 +880,19 @@ eal_parse_corelist(const char *corelist, int *cores)
>                         if (min == RTE_MAX_LCORE)
>                                 min = idx;
>                         for (idx = min; idx <= max; idx++) {
> -                               if (cores[idx] == -1) {
> -                                       cores[idx] = count;
> -                                       count++;
> +                               if (idx < RTE_MAX_LCORE) {
> +                                       if (cores[idx] == -1)
> +                                               cores[idx] = count;
>                                 }
> +                               count++;
> +                               if (count == 1)
> +                                       len = len + snprintf(&lcores[len],
> +                                                       MAX_LCORES_STRING - len,
> +                                                       "%d@%d", count-1, idx);
> +                               else
> +                                       len = len + snprintf(&lcores[len],
> +                                                       MAX_LCORES_STRING - len,
> +                                                       ",%d@%d", count-1, idx);

Always appending a , is easier to read, then after the loop, you just
need to trim the last ,.


>                         }
>                         min = RTE_MAX_LCORE;
>                 } else
> @@ -886,6 +902,13 @@ eal_parse_corelist(const char *corelist, int *cores)
>
>         if (count == 0)
>                 return -1;
> +       if (overflow) {
> +               RTE_LOG(ERR, EAL, "Error = One of the %d cores provided exceeds RTE_MAX_LCORE (%d)\n",
> +                               count, RTE_MAX_LCORE);
> +               RTE_LOG(ERR, EAL, "Please use --lcores instead, e.g. --lcores %s\n",
> +                               lcores);
> +               return -1;
> +       }
>         return 0;


I'd rework both -c and -l parsing to fill a common data structure,
then validate and generate the suggestion in common helpers.

Something like: https://github.com/david-marchand/dpdk/commit/lcores
This probably needs some time to look at to enhance style and
carefully check for mem leaks.
Tested with max_lcores = 4 (for my 8 cores laptop):

$ for opt in "-c 0x" "-c 0x0" "-c 0x1" "-c 0xf" "-c 0x10" "-c 0x1f"
"-c 0x11" "-c 0x30" "-l 0" "-l 0-3" "-l 0-3,2" "-l 4" "-l 0-4" "-l
0,4" "-l 4,5"
do
  echo $opt
  echo quit | build/app/dpdk-testpmd $opt --log-level=lib.eal:debug
--no-huge -m 20 -a 0:0.0 -- --total-num-mbufs=2048 -ia |&
    grep -E '(ready|RTE_MAX_LCORE|Please use|No lcore|Too many)'
  echo
done

-c 0x
EAL: No lcore in coremask: 0x

-c 0x0
EAL: No lcore in coremask: 0x0

-c 0x1
EAL: Main lcore 0 is ready (tid=7f03956d1c00;cpuset=[0])

-c 0xf
EAL: Main lcore 0 is ready (tid=7fe464461c00;cpuset=[0])
EAL: lcore 1 is ready (tid=7fe45f924700;cpuset=[1])
EAL: lcore 2 is ready (tid=7fe45f123700;cpuset=[2])
EAL: lcore 3 is ready (tid=7fe45e922700;cpuset=[3])

-c 0x10
EAL: lcore 4 >= RTE_MAX_LCORE (4)
EAL: Please use --lcores 0@4

-c 0x1f
EAL: Too many lcores in coremask: 0x1f

-c 0x11
EAL: lcore 4 >= RTE_MAX_LCORE (4)
EAL: Please use --lcores 0@0,1@4

-c 0x30
EAL: lcore 4 >= RTE_MAX_LCORE (4)
EAL: lcore 5 >= RTE_MAX_LCORE (4)
EAL: Please use --lcores 0@4,1@5

-l 0
EAL: Main lcore 0 is ready (tid=7f833b17ac00;cpuset=[0])

-l 0-3
EAL: Main lcore 0 is ready (tid=7f9ff5216c00;cpuset=[0])
EAL: lcore 2 is ready (tid=7f9fefed8700;cpuset=[2])
EAL: lcore 3 is ready (tid=7f9fef6d7700;cpuset=[3])
EAL: lcore 1 is ready (tid=7f9ff06d9700;cpuset=[1])

-l 0-3,2
EAL: Main lcore 0 is ready (tid=7f106b937c00;cpuset=[0])
EAL: lcore 1 is ready (tid=7f1066dfa700;cpuset=[1])
EAL: lcore 2 is ready (tid=7f10665f9700;cpuset=[2])
EAL: lcore 3 is ready (tid=7f1065df8700;cpuset=[3])

-l 4
EAL: lcore 4 >= RTE_MAX_LCORE (4)
EAL: Please use --lcores 0@4

-l 0-4
EAL: Too many lcores in core list: 0-4

-l 0,4
EAL: lcore 4 >= RTE_MAX_LCORE (4)
EAL: Please use --lcores 0@0,1@4

-l 4,5
EAL: lcore 4 >= RTE_MAX_LCORE (4)
EAL: lcore 5 >= RTE_MAX_LCORE (4)
EAL: Please use --lcores 0@4,1@5


-- 
David Marchand

Re: [dpdk-dev] [PATCH v2] eal: add additional info if lcore exceeds max cores

[David Hunt]

Hi David,

On 16/9/2021 1:34 PM, David Marchand wrote:
> On Wed, Sep 15, 2021 at 2:11 PM David Hunt <david.hunt@intel.com> wrote:
>> If the user requests to use an lcore above 128 using -l or -c,
>> the eal will exit with "EAL: invalid core list syntax" and
>> very little other useful information.
>>
>> This patch adds some extra information suggesting to use --lcores
>> so that physical cores above RTE_MAX_LCORE (default 128) can be
>> used. This is achieved by using the --lcores option by mapping
>> the logical cores in the application onto to physical cores.
>>
>> There is no change in functionalty, just additional messages
>> suggesting how the --lcores option might be used for the supplied
>> list of lcores. For example, if "-l 12-14,130,132" is used, we
>> see the following additional output on the command line:
>>
>> EAL: Error = One of the 5 cores provided exceeds RTE_MAX_LCORE (128)
>> EAL: Please use --lcores instead, e.g. --lcores 0@12,1@13,2@14,3@130,4@132
>>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>>
>> ---
>> changes in v2
>>     * Rather than increasing the default max lcores (as in v1),
>>       it was agreed to do this instead (switch to --lcores).
>>     * As the other patches in the v1 of the set are no longer related
>>       to this change, I'll submit as a separate patch set.
> The -c option can use the same kind of warning.


Agreed, I'll include in the next version.


>
>> ---
>>   lib/eal/common/eal_common_options.c | 31 +++++++++++++++++++++++++----
>>   1 file changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
>> index ff5861b5f3..5c7a5a45a5 100644
>> --- a/lib/eal/common/eal_common_options.c
>> +++ b/lib/eal/common/eal_common_options.c
>> @@ -836,6 +836,8 @@ eal_parse_service_corelist(const char *corelist)
>>          return 0;
>>   }
>>
>> +#define MAX_LCORES_STRING 512
>> +
>>   static int
>>   eal_parse_corelist(const char *corelist, int *cores)
>>   {
>> @@ -843,6 +845,9 @@ eal_parse_corelist(const char *corelist, int *cores)
>>          char *end = NULL;
>>          int min, max;
>>          int idx;
>> +       bool overflow = false;
>> +       char lcores[MAX_LCORES_STRING] = "";
> This code is not performance sensitive.
> In the worst case, like for RTE_MAX_LCORES lcores, it gives this:
> 0@0,1@1,2@2,3@3,4@4,5@5,6@6,7@7,8@8,9@9,10@10,11@11,12@12,13@13,14@14,15@15,16@16,17@17,18@18,19@19,20@20,21@21,22@22,23@23,24@24,25@25,26@26,27@27,28@28,29@29,30@30,31@31,32@32,33@33,34@34,35@35,36@36,37@37,38@38,39@39,40@40,41@41,42@42,43@43,44@44,45@45,46@46,47@47,48@48,49@49,50@50,51@51,52@52,53@53,54@54,55@55,56@56,57@57,58@58,59@59,60@60,61@61,62@62,63@63,64@64,65@65,66@66,67@67,68@68,69@69,70@70,71@71,72@72,73@73,74@74,75@75,76@76,77@77,78@78,79@79,80@80,81@81,82@82,83@83,84@84,85@85,86@86,87@87,88@88,89@89,90@90,91@91,92@92,93@93,94@94,95@95,96@96,97@97,98@98,99@99,100@100,101@101,102@102,103@103,104@104,105@105,106@106,107@107,108@108,109@109,110@110,111@111,112@112,113@113,114@114,115@115,116@116,117@117,118@118,119@119,120@120,121@121,122@122,123@123,124@124,125@125,126@126,127@127,
>
> Which is 800+ bytes long, let's switch do dynamic allocations.
>

Good point. I'll allocate a dozen bytes or so for each physical core 
detected, that should be enough.


>
>> +       int len = 0;
>>
>>          for (idx = 0; idx < RTE_MAX_LCORE; idx++)
>>                  cores[idx] = -1;
>> @@ -862,8 +867,10 @@ eal_parse_corelist(const char *corelist, int *cores)
>>                  idx = strtol(corelist, &end, 10);
>>                  if (errno || end == NULL)
>>                          return -1;
>> -               if (idx < 0 || idx >= RTE_MAX_LCORE)
>> +               if (idx < 0)
>>                          return -1;
>> +               if (idx >= RTE_MAX_LCORE)
>> +                       overflow = true;
> The code before was intermixing parsing and validation of values.
> This intermix was not that great.
> Let's separate those concerns.

I see what you mean (in your comments below). Agreed this would be a 
good idea.


>
>>                  while (isblank(*end))
>>                          end++;
>>                  if (*end == '-') {
>> @@ -873,10 +880,19 @@ eal_parse_corelist(const char *corelist, int *cores)
>>                          if (min == RTE_MAX_LCORE)
>>                                  min = idx;
>>                          for (idx = min; idx <= max; idx++) {
>> -                               if (cores[idx] == -1) {
>> -                                       cores[idx] = count;
>> -                                       count++;
>> +                               if (idx < RTE_MAX_LCORE) {
>> +                                       if (cores[idx] == -1)
>> +                                               cores[idx] = count;
>>                                  }
>> +                               count++;
>> +                               if (count == 1)
>> +                                       len = len + snprintf(&lcores[len],
>> +                                                       MAX_LCORES_STRING - len,
>> +                                                       "%d@%d", count-1, idx);
>> +                               else
>> +                                       len = len + snprintf(&lcores[len],
>> +                                                       MAX_LCORES_STRING - len,
>> +                                                       ",%d@%d", count-1, idx);
> Always appending a , is easier to read, then after the loop, you just
> need to trim the last ,.

Sure.


>
>>                          }
>>                          min = RTE_MAX_LCORE;
>>                  } else
>> @@ -886,6 +902,13 @@ eal_parse_corelist(const char *corelist, int *cores)
>>
>>          if (count == 0)
>>                  return -1;
>> +       if (overflow) {
>> +               RTE_LOG(ERR, EAL, "Error = One of the %d cores provided exceeds RTE_MAX_LCORE (%d)\n",
>> +                               count, RTE_MAX_LCORE);
>> +               RTE_LOG(ERR, EAL, "Please use --lcores instead, e.g. --lcores %s\n",
>> +                               lcores);
>> +               return -1;
>> +       }
>>          return 0;
>
> I'd rework both -c and -l parsing to fill a common data structure,
> then validate and generate the suggestion in common helpers.


OK, I'll take a look.



> Something like: https://github.com/david-marchand/dpdk/commit/lcores
> This probably needs some time to look at to enhance style and
> carefully check for mem leaks.
> Tested with max_lcores = 4 (for my 8 cores laptop):
>
> $ for opt in "-c 0x" "-c 0x0" "-c 0x1" "-c 0xf" "-c 0x10" "-c 0x1f"
> "-c 0x11" "-c 0x30" "-l 0" "-l 0-3" "-l 0-3,2" "-l 4" "-l 0-4" "-l
> 0,4" "-l 4,5"
> do
>    echo $opt
>    echo quit | build/app/dpdk-testpmd $opt --log-level=lib.eal:debug
> --no-huge -m 20 -a 0:0.0 -- --total-num-mbufs=2048 -ia |&
>      grep -E '(ready|RTE_MAX_LCORE|Please use|No lcore|Too many)'
>    echo
> done
>
> -c 0x
> EAL: No lcore in coremask: 0x
>
> -c 0x0
> EAL: No lcore in coremask: 0x0
>
> -c 0x1
> EAL: Main lcore 0 is ready (tid=7f03956d1c00;cpuset=[0])
>
> -c 0xf
> EAL: Main lcore 0 is ready (tid=7fe464461c00;cpuset=[0])
> EAL: lcore 1 is ready (tid=7fe45f924700;cpuset=[1])
> EAL: lcore 2 is ready (tid=7fe45f123700;cpuset=[2])
> EAL: lcore 3 is ready (tid=7fe45e922700;cpuset=[3])
>
> -c 0x10
> EAL: lcore 4 >= RTE_MAX_LCORE (4)
> EAL: Please use --lcores 0@4
>
> -c 0x1f
> EAL: Too many lcores in coremask: 0x1f
>
> -c 0x11
> EAL: lcore 4 >= RTE_MAX_LCORE (4)
> EAL: Please use --lcores 0@0,1@4
>
> -c 0x30
> EAL: lcore 4 >= RTE_MAX_LCORE (4)
> EAL: lcore 5 >= RTE_MAX_LCORE (4)
> EAL: Please use --lcores 0@4,1@5
>
> -l 0
> EAL: Main lcore 0 is ready (tid=7f833b17ac00;cpuset=[0])
>
> -l 0-3
> EAL: Main lcore 0 is ready (tid=7f9ff5216c00;cpuset=[0])
> EAL: lcore 2 is ready (tid=7f9fefed8700;cpuset=[2])
> EAL: lcore 3 is ready (tid=7f9fef6d7700;cpuset=[3])
> EAL: lcore 1 is ready (tid=7f9ff06d9700;cpuset=[1])
>
> -l 0-3,2
> EAL: Main lcore 0 is ready (tid=7f106b937c00;cpuset=[0])
> EAL: lcore 1 is ready (tid=7f1066dfa700;cpuset=[1])
> EAL: lcore 2 is ready (tid=7f10665f9700;cpuset=[2])
> EAL: lcore 3 is ready (tid=7f1065df8700;cpuset=[3])
>
> -l 4
> EAL: lcore 4 >= RTE_MAX_LCORE (4)
> EAL: Please use --lcores 0@4
>
> -l 0-4
> EAL: Too many lcores in core list: 0-4
>
> -l 0,4
> EAL: lcore 4 >= RTE_MAX_LCORE (4)
> EAL: Please use --lcores 0@0,1@4
>
> -l 4,5
> EAL: lcore 4 >= RTE_MAX_LCORE (4)
> EAL: lcore 5 >= RTE_MAX_LCORE (4)
> EAL: Please use --lcores 0@4,1@5
>
>

[dpdk-dev] [PATCH v3 1/2] eal: add additional info if core list too long

[David Hunt]

If the user requests to use an lcore above 128 using -l,
the eal will exit with "EAL: invalid core list syntax" and
very little else useful information.

THis patch adds some extra information suggesting to use --lcores
so that physical cores above RTE_MAX_LCORE (default 128) can be
used. This is achieved by using the --lcores option by mapping
the logical cores in the application to physical cores.

There is no change in functionalty, just additional messages
suggesting how the --lcores option might be used for the supplied
list of lcores. For example, if "-l 12-16,130,132" is used, we
see the following additional output on the command line:

EAL: Error = One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
EAL: Please use --lcores instead, e.g.
     --lcores 0@12,1@13,2@14,3@15,4@16,5@130,6@132

Signed-off-by: David Hunt <david.hunt@intel.com>

---
changes in v2
   * Rather than increasing the default max lcores (as in v1),
     it was agreed to do this instead (switch to --lcores).
   * As the other patches in the v1 of the set are no longer related
     to this change, I'll submit as a separate patch set.
changes in v3
   * separated out some of the corelist cheking into separate function
   * added extra messages for the different failure conditions.
   * changed allocation of the core strings from static to dynamic
   * now prints out a message for each core above RTE_MAX_LCORE
---
 lib/eal/common/eal_common_options.c | 102 ++++++++++++++++++++++++----
 1 file changed, 89 insertions(+), 13 deletions(-)

diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
index ff5861b5f3..6139b2b21e 100644
--- a/lib/eal/common/eal_common_options.c
+++ b/lib/eal/common/eal_common_options.c
@@ -709,6 +709,47 @@ update_lcore_config(int *cores)
 	return ret;
 }
 
+static int
+check_core_list(int *lcores, unsigned int count)
+{
+	unsigned int i, j;
+	char *lcorestr;
+	int len = 0;
+	bool overflow = false;
+
+	for (j = 0; j < count; j++) {
+		if (lcores[j] >= RTE_MAX_LCORE) {
+			RTE_LOG(ERR, EAL, "lcore %d >= RTE_MAX_LCORE (%d)\n",
+					lcores[j], RTE_MAX_LCORE);
+			overflow = true;
+		}
+	}
+	if (overflow) {
+		/*
+		 * If we've encountered a core that's greater than
+		 * RTE_MAX_LCORE, suggest using --lcores option to
+		 * map lcores onto physical cores greater than
+		 * RTE_MAX_LCORE, then return.
+		 */
+		lcorestr = calloc(1, RTE_MAX_LCORE * 10);
+		if (lcorestr == NULL) {
+			RTE_LOG(ERR, EAL, "Unable to allocate lcore string\n");
+			return -ENOMEM;
+		}
+		for (i = 0; i < count; i++)
+			len = len + snprintf(&lcorestr[len],
+					RTE_MAX_LCORE * 10 - len,
+					"%d@%d,", i, lcores[i]);
+		if (len > 0)
+			lcorestr[len-1] = 0;
+		RTE_LOG(ERR, EAL, "Please use --lcores instead, e.g. --lcores %s\n",
+				lcorestr);
+		free(lcorestr);
+		return -1;
+	}
+	return 0;
+}
+
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {
@@ -839,54 +880,89 @@ eal_parse_service_corelist(const char *corelist)
 static int
 eal_parse_corelist(const char *corelist, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0, k;
 	char *end = NULL;
 	int min, max;
 	int idx;
+	int lcores[RTE_MAX_LCORE];
+	char *corelist_copy;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
 
+	corelist_copy = calloc(1, strlen(corelist)+1);
+	if (corelist_copy == NULL) {
+		RTE_LOG(ERR, EAL, "Unable to allocate corelist copy\n");
+		return -ENOMEM;
+	}
+
+	strlcpy(corelist_copy, corelist, strlen(corelist)+1);
+
 	/* Remove all blank characters ahead */
 	while (isblank(*corelist))
 		corelist++;
 
 	/* Get list of cores */
-	min = RTE_MAX_LCORE;
+	min = -1;
 	do {
 		while (isblank(*corelist))
 			corelist++;
 		if (*corelist == '\0')
-			return -1;
+			goto err;
 		errno = 0;
 		idx = strtol(corelist, &end, 10);
 		if (errno || end == NULL)
-			return -1;
-		if (idx < 0 || idx >= RTE_MAX_LCORE)
-			return -1;
+			goto err;
+		if (idx < 0)
+			goto err;
 		while (isblank(*end))
 			end++;
 		if (*end == '-') {
 			min = idx;
 		} else if ((*end == ',') || (*end == '\0')) {
 			max = idx;
-			if (min == RTE_MAX_LCORE)
+			if (min == -1)
 				min = idx;
 			for (idx = min; idx <= max; idx++) {
-				if (cores[idx] == -1) {
-					cores[idx] = count;
-					count++;
+				lcores[count] = idx;
+				count++;
+				if (count > RTE_MAX_LCORE) {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					goto err;
 				}
 			}
-			min = RTE_MAX_LCORE;
+			min = -1;
 		} else
-			return -1;
+			goto err;
 		corelist = end + 1;
 	} while (*end != '\0');
 
 	if (count == 0)
-		return -1;
+		goto err;
+
+	if (check_core_list(lcores, count))
+		goto err;
+
+	/*
+	 * Now that we've gto a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+
+	for (k = 0; k < count; k++)
+		cores[lcores[k]] = k;
+
+	if (corelist_copy)
+		free(corelist_copy);
+
 	return 0;
+err:
+	if (corelist_copy)
+		free(corelist_copy);
+
+	return -1;
 }
 
 /* Changes the lcore id of the main thread */
-- 
2.17.1

[dpdk-dev] [PATCH v3 2/2] eal: add additional info if core mask too long

[David Hunt]

If the user requests to use an lcore above 128 using -c,
the eal will exit with "EAL: invalid core list syntax" and
very little else useful information.

This patch adds some extra information suggesting to use --lcores
so that physical cores above RTE_MAX_LCORE (default 128) can be
used. This is achieved by using the --lcores option by mapping
the logical cores in the application to physical cores.

There is no change in functionalty, just additional messages
suggesting how the --lcores option might be used for the supplied
list of lcores. For example, if
"-c 0xf00000000000000000000000000000000" is used, we see the
following additional output on the command line:

EAL: Error = One of the 4 cores provided exceeds RTE_MAX_LCORE (128)
EAL: Please use --lcores instead, e.g. --lcores 0@128,1@129,2@130,3@131

Signed-off-by: David Hunt <david.hunt@intel.com>

---
changes in v3
   * added this patch to the set. Addresses the changes for
     the -c option.
---
 lib/eal/common/eal_common_options.c | 67 +++++++++++++++++++++++------
 1 file changed, 55 insertions(+), 12 deletions(-)

diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
index 6139b2b21e..331dca1e38 100644
--- a/lib/eal/common/eal_common_options.c
+++ b/lib/eal/common/eal_common_options.c
@@ -753,15 +753,25 @@ check_core_list(int *lcores, unsigned int count)
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0, k;
 	int i, j, idx;
 	int val;
 	char c;
+	int lcores[RTE_MAX_LCORE];
+	char *coremask_copy = NULL;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
 	idx = 0;
 
+	coremask_copy = calloc(1, strlen(coremask)+1);
+	if (coremask_copy == NULL) {
+		RTE_LOG(ERR, EAL, "Unable to allocate coremask copy\n");
+		return -ENOMEM;
+	}
+
+	strlcpy(coremask_copy, coremask, strlen(coremask)+1);
+
 	/* Remove all blank characters ahead and after .
 	 * Remove 0x/0X if exists.
 	 */
@@ -773,30 +783,63 @@ eal_parse_coremask(const char *coremask, int *cores)
 	i = strlen(coremask);
 	while ((i > 0) && isblank(coremask[i - 1]))
 		i--;
-	if (i == 0)
-		return -1;
+	if (i == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
+		goto err;
+	}
 
-	for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
+	for (i = i - 1; i >= 0; i--) {
 		c = coremask[i];
 		if (isxdigit(c) == 0) {
 			/* invalid characters */
-			return -1;
+			RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
+					coremask_copy);
+			goto err;
 		}
 		val = xdigit2val(c);
-		for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
+		for (j = 0; j < BITS_PER_HEX; j++, idx++)
 		{
 			if ((1 << j) & val) {
-				cores[idx] = count;
-				count++;
+				lcores[count++] = idx;
+				if (count > RTE_MAX_LCORE) {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					goto err;
+				}
 			}
 		}
 	}
 	for (; i >= 0; i--)
-		if (coremask[i] != '0')
-			return -1;
-	if (count == 0)
-		return -1;
+		if (coremask[i] != '0') {
+			RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
+					coremask_copy);
+			goto err;
+		}
+	if (count == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
+		goto err;
+	}
+
+	if (check_core_list(lcores, count))
+		goto err;
+
+	/*
+	 * Now that we've gto a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+	for (k = 0; k < count; k++)
+		cores[lcores[k]] = k;
+
+	if (coremask_copy)
+		free(coremask_copy);
+
 	return 0;
+err:
+	if (coremask_copy)
+		free(coremask_copy);
+	return -1;
 }
 
 static int
-- 
2.17.1

Re: [dpdk-dev] [PATCH v3 1/2] eal: add additional info if core list too long

[Bruce Richardson]

On Tue, Sep 21, 2021 at 12:50:14PM +0100, David Hunt wrote:
> If the user requests to use an lcore above 128 using -l,
> the eal will exit with "EAL: invalid core list syntax" and
> very little else useful information.
> 
> THis patch adds some extra information suggesting to use --lcores
> so that physical cores above RTE_MAX_LCORE (default 128) can be
> used. This is achieved by using the --lcores option by mapping
> the logical cores in the application to physical cores.
> 
> There is no change in functionalty, just additional messages
> suggesting how the --lcores option might be used for the supplied
> list of lcores. For example, if "-l 12-16,130,132" is used, we
> see the following additional output on the command line:
> 
> EAL: Error = One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
> EAL: Please use --lcores instead, e.g.

Minor suggestion: it would be good to clarify how to use lcores and what is
happening here in the example. Something like: "Please use --lcores
instead, to map lower lcore ids onto higher-numbered cores", could help the
user understand better what is happening.

>      --lcores 0@12,1@13,2@14,3@15,4@16,5@130,6@132
> 
> Signed-off-by: David Hunt <david.hunt@intel.com>

With some more info to help the user:

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH v3 2/2] eal: add additional info if core mask too long

[Bruce Richardson]

On Tue, Sep 21, 2021 at 12:50:15PM +0100, David Hunt wrote:
> If the user requests to use an lcore above 128 using -c,
> the eal will exit with "EAL: invalid core list syntax" and
> very little else useful information.
> 
> This patch adds some extra information suggesting to use --lcores
> so that physical cores above RTE_MAX_LCORE (default 128) can be
> used. This is achieved by using the --lcores option by mapping
> the logical cores in the application to physical cores.
> 
> There is no change in functionalty, just additional messages
> suggesting how the --lcores option might be used for the supplied
> list of lcores. For example, if
> "-c 0xf00000000000000000000000000000000" is used, we see the
> following additional output on the command line:
> 
> EAL: Error = One of the 4 cores provided exceeds RTE_MAX_LCORE (128)

"=" ?

> EAL: Please use --lcores instead, e.g. --lcores 0@128,1@129,2@130,3@131
> 
> Signed-off-by: David Hunt <david.hunt@intel.com>
> 
While I really can't see many people using "-c" to use cores >128 (unless
from a script, I suppose), this change is worthwhile having for
completeness. I suggest aligning the error message with that for "-l" flag.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH v3 1/2] eal: add additional info if core list too long

[David Hunt]

On 21/9/2021 12:57 PM, Bruce Richardson wrote:
> On Tue, Sep 21, 2021 at 12:50:14PM +0100, David Hunt wrote:
>> If the user requests to use an lcore above 128 using -l,
>> the eal will exit with "EAL: invalid core list syntax" and
>> very little else useful information.
>>
>> THis patch adds some extra information suggesting to use --lcores
>> so that physical cores above RTE_MAX_LCORE (default 128) can be
>> used. This is achieved by using the --lcores option by mapping
>> the logical cores in the application to physical cores.
>>
>> There is no change in functionalty, just additional messages
>> suggesting how the --lcores option might be used for the supplied
>> list of lcores. For example, if "-l 12-16,130,132" is used, we
>> see the following additional output on the command line:
>>
>> EAL: Error = One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
>> EAL: Please use --lcores instead, e.g.
> Minor suggestion: it would be good to clarify how to use lcores and what is
> happening here in the example. Something like: "Please use --lcores
> instead, to map lower lcore ids onto higher-numbered cores", could help the
> user understand better what is happening.


Hi Bruce, how about:

EAL: Please use --lcores to map logical cores onto cores > RTE_LCORE_MAX 
,e.g. --lcores 0@12,1@13,2@14,3@15,4@16,5@130,6@132

Rgds,
Dave.



>
>>       --lcores 0@12,1@13,2@14,3@15,4@16,5@130,6@132
>>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
> With some more info to help the user:
>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH v3 1/2] eal: add additional info if core list too long

[David Hunt]

On 21/9/2021 1:04 PM, David Hunt wrote:
>
> On 21/9/2021 12:57 PM, Bruce Richardson wrote:
>> On Tue, Sep 21, 2021 at 12:50:14PM +0100, David Hunt wrote:
>>> If the user requests to use an lcore above 128 using -l,
>>> the eal will exit with "EAL: invalid core list syntax" and
>>> very little else useful information.
>>>
>>> THis patch adds some extra information suggesting to use --lcores
>>> so that physical cores above RTE_MAX_LCORE (default 128) can be
>>> used. This is achieved by using the --lcores option by mapping
>>> the logical cores in the application to physical cores.
>>>
>>> There is no change in functionalty, just additional messages
>>> suggesting how the --lcores option might be used for the supplied
>>> list of lcores. For example, if "-l 12-16,130,132" is used, we
>>> see the following additional output on the command line:
>>>
>>> EAL: Error = One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
>>> EAL: Please use --lcores instead, e.g.
>> Minor suggestion: it would be good to clarify how to use lcores and 
>> what is
>> happening here in the example. Something like: "Please use --lcores
>> instead, to map lower lcore ids onto higher-numbered cores", could 
>> help the
>> user understand better what is happening.
>
>
> Hi Bruce, how about:
>
> EAL: Please use --lcores to map logical cores onto cores > 
> RTE_LCORE_MAX ,e.g. --lcores 0@12,1@13,2@14,3@15,4@16,5@130,6@132
>
> Rgds,
> Dave.
>
>
>

I think this should do it, as it clarifies the mapping:

EAL: lcore 130 >= RTE_MAX_LCORE (128)
EAL: lcore 132 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to map them to 
lcore ids below RTE_LCORE_MAX, e.g.'--lcores 
0@12,1@13,2@14,3@15,4@16,5@130,6@132'

Thanks,
Dave.



>>
>>>       --lcores 0@12,1@13,2@14,3@15,4@16,5@130,6@132
>>>
>>> Signed-off-by: David Hunt <david.hunt@intel.com>
>> With some more info to help the user:
>>
>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH v3 1/2] eal: add additional info if core list too long

[Bruce Richardson]

On Tue, Sep 21, 2021 at 02:16:35PM +0100, David Hunt wrote:
>    On 21/9/2021 1:04 PM, David Hunt wrote:
> 
>      On 21/9/2021 12:57 PM, Bruce Richardson wrote:
> 
>      On Tue, Sep 21, 2021 at 12:50:14PM +0100, David Hunt wrote:
> 
>      If the user requests to use an lcore above 128 using -l,
>      the eal will exit with "EAL: invalid core list syntax" and
>      very little else useful information.
>      THis patch adds some extra information suggesting to use --lcores
>      so that physical cores above RTE_MAX_LCORE (default 128) can be
>      used. This is achieved by using the --lcores option by mapping
>      the logical cores in the application to physical cores.
>      There is no change in functionalty, just additional messages
>      suggesting how the --lcores option might be used for the supplied
>      list of lcores. For example, if "-l 12-16,130,132" is used, we
>      see the following additional output on the command line:
>      EAL: Error = One of the 7 cores provided exceeds RTE_MAX_LCORE (128)
>      EAL: Please use --lcores instead, e.g.
> 
>      Minor suggestion: it would be good to clarify how to use lcores and
>      what is
>      happening here in the example. Something like: "Please use --lcores
>      instead, to map lower lcore ids onto higher-numbered cores", could
>      help the
>      user understand better what is happening.
> 
>      Hi Bruce, how about:
>      EAL: Please use --lcores to map logical cores onto cores >
>      RTE_LCORE_MAX ,e.g. --lcores 0@12,1@13,2@14,3@15,4@16,5@130,6@132
>      Rgds,
>      Dave.
> 
>    I think this should do it, as it clarifies the mapping:
> 
>    EAL: lcore 130 >= RTE_MAX_LCORE (128)
>    EAL: lcore 132 >= RTE_MAX_LCORE (128)
>    EAL: to use high physical core ids , please use --lcores to map them to
>    lcore ids below RTE_LCORE_MAX, e.g. '--lcores
>    0@12,1@13,2@14,3@15,4@16,5@130,6@132'
> 
Text looks good to me.

Again minor nits: I think the continued lines should be indented,
and you should probably wrap immediately after the "e.g." rather than in
the middle of the parameter set.

/Bruce

Re: [dpdk-dev] [PATCH v3 1/2] eal: add additional info if core list too long

[David Marchand]

On Tue, Sep 21, 2021 at 1:50 PM David Hunt <david.hunt@intel.com> wrote:
>  static int
>  eal_parse_coremask(const char *coremask, int *cores)
>  {
> @@ -839,54 +880,89 @@ eal_parse_service_corelist(const char *corelist)
>  static int
>  eal_parse_corelist(const char *corelist, int *cores)
>  {
> -       unsigned count = 0;
> +       unsigned int count = 0, k;
>         char *end = NULL;
>         int min, max;
>         int idx;
> +       int lcores[RTE_MAX_LCORE];

Static array...

"-l 0-RTE_MAX_LCORE" / "-c 0x1<enough f to fill RTE_MAX_LCORE>" / "-l
0-(RTE_MAX_LCORE-1),0" crash.

Please set RTE_MAX_LCORE to 4 (or something that is smaller than your
system core count) and run the tests I provided in my previous mail.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v3 1/2] eal: add additional info if core list too long

[David Hunt]

On 21/9/2021 2:51 PM, David Marchand wrote:
> On Tue, Sep 21, 2021 at 1:50 PM David Hunt <david.hunt@intel.com> wrote:
>>   static int
>>   eal_parse_coremask(const char *coremask, int *cores)
>>   {
>> @@ -839,54 +880,89 @@ eal_parse_service_corelist(const char *corelist)
>>   static int
>>   eal_parse_corelist(const char *corelist, int *cores)
>>   {
>> -       unsigned count = 0;
>> +       unsigned int count = 0, k;
>>          char *end = NULL;
>>          int min, max;
>>          int idx;
>> +       int lcores[RTE_MAX_LCORE];
> Static array...
>
> "-l 0-RTE_MAX_LCORE" / "-c 0x1<enough f to fill RTE_MAX_LCORE>" / "-l
> 0-(RTE_MAX_LCORE-1),0" crash.
>
> Please set RTE_MAX_LCORE to 4 (or something that is smaller than your
> system core count) and run the tests I provided in my previous mail.
>
Hi David,

I did set the lcore_max to 4, and ran the tests you provided, and they 
all looked OK.

However, as you have pointed out, there is a problem with the lcores 
array, which I have now fixed, I'll push up shortly. I'm not expecting 
to populate with more than RTE_MAX_LCORE elements, but there was an edge 
case where one extra was being added.

Thanks again,
Dave.

[dpdk-dev] [PATCH v4 1/2] eal: add additional info if core list too long

[David Hunt]

If the user requests to use an lcore above 128 using -l,
the eal will exit with "EAL: invalid core list syntax" and
very little else useful information.

This patch adds some extra information suggesting to use --lcores
so that physical cores above RTE_MAX_LCORE (default 128) can be
used. This is achieved by using the --lcores option by mapping
the logical cores in the application to physical cores.

For example, if "-l 12-16,130,132" is used, we see the following
additional output on the command line:

EAL: lcore 132 >= RTE_MAX_LCORE (128)
EAL: lcore 133 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to map
them to lcore ids below RTE_MAX_LCORE,
EAL:     e.g. --lcores 0@12,1@13,2@14,3@15,4@16,5@132,6@133

Signed-off-by: David Hunt <david.hunt@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
changes in v2
   * Rather than increasing the default max lcores (as in v1),
     it was agreed to do this instead (switch to --lcores).
   * As the other patches in the v1 of the set are no longer related
     to this change, I'll submit as a separate patch set.
changes in v3
   * separated out some of the corelist cheking into separate function
   * added extra messages for the different failure conditions.
   * changed allocation of the core strings from static to dynamic
   * now prints out a message for each core above RTE_MAX_LCORE
changes in v4
   * tweaked log messages to be a bit clearer about mapping lcores
     to physical cores.
   * improved indenting of log messages.
   * fixed bug in overrunning end of lcore array
   * switched from strlcpy to strdup because of a clang error
---
---
 lib/eal/common/eal_common_options.c | 100 ++++++++++++++++++++++++----
 1 file changed, 87 insertions(+), 13 deletions(-)

diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
index eaef57312f..de1717946f 100644
--- a/lib/eal/common/eal_common_options.c
+++ b/lib/eal/common/eal_common_options.c
@@ -703,6 +703,47 @@ update_lcore_config(int *cores)
 	return ret;
 }
 
+static int
+check_core_list(int *lcores, unsigned int count)
+{
+	unsigned int i, j;
+	char *lcorestr;
+	int len = 0;
+	bool overflow = false;
+
+	for (j = 0; j < count; j++) {
+		if (lcores[j] >= RTE_MAX_LCORE) {
+			RTE_LOG(ERR, EAL, "lcore %d >= RTE_MAX_LCORE (%d)\n",
+					lcores[j], RTE_MAX_LCORE);
+			overflow = true;
+		}
+	}
+	if (overflow) {
+		/*
+		 * If we've encountered a core that's greater than
+		 * RTE_MAX_LCORE, suggest using --lcores option to
+		 * map lcores onto physical cores greater than
+		 * RTE_MAX_LCORE, then return.
+		 */
+		lcorestr = calloc(1, RTE_MAX_LCORE * 10);
+		if (lcorestr == NULL) {
+			RTE_LOG(ERR, EAL, "Unable to allocate lcore string\n");
+			return -ENOMEM;
+		}
+		for (i = 0; i < count; i++)
+			len = len + snprintf(&lcorestr[len],
+					RTE_MAX_LCORE * 10 - len,
+					"%d@%d,", i, lcores[i]);
+		if (len > 0)
+			lcorestr[len-1] = 0;
+		RTE_LOG(ERR, EAL, "to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,\n");
+		RTE_LOG(ERR, EAL, "    e.g. --lcores %s\n", lcorestr);
+		free(lcorestr);
+		return -1;
+	}
+	return 0;
+}
+
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {
@@ -833,54 +874,87 @@ eal_parse_service_corelist(const char *corelist)
 static int
 eal_parse_corelist(const char *corelist, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0, k;
 	char *end = NULL;
 	int min, max;
 	int idx;
+	int lcores[RTE_MAX_LCORE];
+	char *corelist_copy;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
 
+	corelist_copy = strdup(corelist);
+	if (corelist_copy == NULL) {
+		RTE_LOG(ERR, EAL, "Unable to duplicate corelist\n");
+		return -ENOMEM;
+	}
+
 	/* Remove all blank characters ahead */
 	while (isblank(*corelist))
 		corelist++;
 
 	/* Get list of cores */
-	min = RTE_MAX_LCORE;
+	min = -1;
 	do {
 		while (isblank(*corelist))
 			corelist++;
 		if (*corelist == '\0')
-			return -1;
+			goto err;
 		errno = 0;
 		idx = strtol(corelist, &end, 10);
 		if (errno || end == NULL)
-			return -1;
-		if (idx < 0 || idx >= RTE_MAX_LCORE)
-			return -1;
+			goto err;
+		if (idx < 0)
+			goto err;
 		while (isblank(*end))
 			end++;
 		if (*end == '-') {
 			min = idx;
 		} else if ((*end == ',') || (*end == '\0')) {
 			max = idx;
-			if (min == RTE_MAX_LCORE)
+			if (min == -1)
 				min = idx;
 			for (idx = min; idx <= max; idx++) {
-				if (cores[idx] == -1) {
-					cores[idx] = count;
-					count++;
+				if (count < RTE_MAX_LCORE)
+					lcores[count++] = idx;
+				else {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					goto err;
 				}
 			}
-			min = RTE_MAX_LCORE;
+			min = -1;
 		} else
-			return -1;
+			goto err;
 		corelist = end + 1;
 	} while (*end != '\0');
 
 	if (count == 0)
-		return -1;
+		goto err;
+
+	if (check_core_list(lcores, count))
+		goto err;
+
+	/*
+	 * Now that we've gto a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+
+	for (k = 0; k < count; k++)
+		cores[lcores[k]] = k;
+
+	if (corelist_copy)
+		free(corelist_copy);
+
 	return 0;
+err:
+	if (corelist_copy)
+		free(corelist_copy);
+
+	return -1;
 }
 
 /* Changes the lcore id of the main thread */
-- 
2.17.1

[dpdk-dev] [PATCH v4 2/2] eal: add additional info if core mask too long

[David Hunt]

If the user requests to use an lcore above 128 using -c,
the eal will exit with "EAL: invalid core list syntax" and
very little else useful information.

This patch adds some extra information suggesting to use --lcores
so that physical cores above RTE_MAX_LCORE (default 128) can be
used. This is achieved by using the --lcores option by mapping
the logical cores in the application to physical cores.

For example, if "-c 0x300000000000000000000000000000000" is
used, we see the following additional output on the command line:

EAL: lcore 128 >= RTE_MAX_LCORE (128)
EAL: lcore 129 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to
map them to lcore ids below RTE_MAX_LCORE,
EAL:     e.g. --lcores 0@128,1@129

Signed-off-by: David Hunt <david.hunt@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
changes in v3
   * added this patch to the set. Addresses the changes for
     the -c option.
changes in v4
   * fixed buffer overrun in populating lcore array
   * switched from strlcpy to strdup due to a clang error
---
---
 lib/eal/common/eal_common_options.c | 66 +++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 12 deletions(-)

diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
index de1717946f..19d3588f78 100644
--- a/lib/eal/common/eal_common_options.c
+++ b/lib/eal/common/eal_common_options.c
@@ -747,15 +747,23 @@ check_core_list(int *lcores, unsigned int count)
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0, k;
 	int i, j, idx;
 	int val;
 	char c;
+	int lcores[RTE_MAX_LCORE];
+	char *coremask_copy = NULL;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
 	idx = 0;
 
+	coremask_copy = strdup(coremask);
+	if (coremask_copy == NULL) {
+		RTE_LOG(ERR, EAL, "Unable to duplicate coremask\n");
+		return -ENOMEM;
+	}
+
 	/* Remove all blank characters ahead and after .
 	 * Remove 0x/0X if exists.
 	 */
@@ -767,30 +775,64 @@ eal_parse_coremask(const char *coremask, int *cores)
 	i = strlen(coremask);
 	while ((i > 0) && isblank(coremask[i - 1]))
 		i--;
-	if (i == 0)
-		return -1;
+	if (i == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
+		goto err;
+	}
 
-	for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
+	for (i = i - 1; i >= 0; i--) {
 		c = coremask[i];
 		if (isxdigit(c) == 0) {
 			/* invalid characters */
-			return -1;
+			RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
+					coremask_copy);
+			goto err;
 		}
 		val = xdigit2val(c);
-		for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
+		for (j = 0; j < BITS_PER_HEX; j++, idx++)
 		{
 			if ((1 << j) & val) {
-				cores[idx] = count;
-				count++;
+				if (count < RTE_MAX_LCORE) {
+					lcores[count++] = idx;
+				} else {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					goto err;
+				}
 			}
 		}
 	}
 	for (; i >= 0; i--)
-		if (coremask[i] != '0')
-			return -1;
-	if (count == 0)
-		return -1;
+		if (coremask[i] != '0') {
+			RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
+					coremask_copy);
+			goto err;
+		}
+	if (count == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
+		goto err;
+	}
+
+	if (check_core_list(lcores, count))
+		goto err;
+
+	/*
+	 * Now that we've gto a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+	for (k = 0; k < count; k++)
+		cores[lcores[k]] = k;
+
+	if (coremask_copy)
+		free(coremask_copy);
+
 	return 0;
+err:
+	if (coremask_copy)
+		free(coremask_copy);
+	return -1;
 }
 
 static int
-- 
2.17.1

Re: [dpdk-dev] [PATCH v4 1/2] eal: add additional info if core list too long

[David Marchand]

On Wed, Sep 22, 2021 at 2:29 PM David Hunt <david.hunt@intel.com> wrote:
>
> If the user requests to use an lcore above 128 using -l,
> the eal will exit with "EAL: invalid core list syntax" and
> very little else useful information.
>
> This patch adds some extra information suggesting to use --lcores
> so that physical cores above RTE_MAX_LCORE (default 128) can be
> used. This is achieved by using the --lcores option by mapping
> the logical cores in the application to physical cores.
>
> For example, if "-l 12-16,130,132" is used, we see the following
> additional output on the command line:
>
> EAL: lcore 132 >= RTE_MAX_LCORE (128)
> EAL: lcore 133 >= RTE_MAX_LCORE (128)
> EAL: to use high physical core ids , please use --lcores to map
> them to lcore ids below RTE_MAX_LCORE,
> EAL:     e.g. --lcores 0@12,1@13,2@14,3@15,4@16,5@132,6@133
>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

This series is there to help users, it should not break existing
working configurations.

I mentionned the "-l 0-3,0" case before.
This syntax is debatable, but it worked before (see comment below) and
this patch now refuses it.


> ---
> changes in v2
>    * Rather than increasing the default max lcores (as in v1),
>      it was agreed to do this instead (switch to --lcores).
>    * As the other patches in the v1 of the set are no longer related
>      to this change, I'll submit as a separate patch set.
> changes in v3
>    * separated out some of the corelist cheking into separate function
>    * added extra messages for the different failure conditions.
>    * changed allocation of the core strings from static to dynamic
>    * now prints out a message for each core above RTE_MAX_LCORE
> changes in v4
>    * tweaked log messages to be a bit clearer about mapping lcores
>      to physical cores.
>    * improved indenting of log messages.
>    * fixed bug in overrunning end of lcore array
>    * switched from strlcpy to strdup because of a clang error
> ---
> ---
>  lib/eal/common/eal_common_options.c | 100 ++++++++++++++++++++++++----
>  1 file changed, 87 insertions(+), 13 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
> index eaef57312f..de1717946f 100644
> --- a/lib/eal/common/eal_common_options.c
> +++ b/lib/eal/common/eal_common_options.c
> @@ -703,6 +703,47 @@ update_lcore_config(int *cores)
>         return ret;
>  }
>
> +static int
> +check_core_list(int *lcores, unsigned int count)
> +{
> +       unsigned int i, j;

One index variable is enough.


> +       char *lcorestr;
> +       int len = 0;
> +       bool overflow = false;
> +
> +       for (j = 0; j < count; j++) {
> +               if (lcores[j] >= RTE_MAX_LCORE) {
> +                       RTE_LOG(ERR, EAL, "lcore %d >= RTE_MAX_LCORE (%d)\n",
> +                                       lcores[j], RTE_MAX_LCORE);
> +                       overflow = true;
> +               }
> +       }

if (!overflow)
    return 0;

> +       if (overflow) {
> +               /*
> +                * If we've encountered a core that's greater than
> +                * RTE_MAX_LCORE, suggest using --lcores option to
> +                * map lcores onto physical cores greater than
> +                * RTE_MAX_LCORE, then return.
> +                */
> +               lcorestr = calloc(1, RTE_MAX_LCORE * 10);

Ugly but works as long as RTE_MAX_LCORE < 10k.
I hope static analysis tools won't complain with the snprintf below
where there is no check on return value.


> +               if (lcorestr == NULL) {
> +                       RTE_LOG(ERR, EAL, "Unable to allocate lcore string\n");
> +                       return -ENOMEM;
> +               }
> +               for (i = 0; i < count; i++)
> +                       len = len + snprintf(&lcorestr[len],
> +                                       RTE_MAX_LCORE * 10 - len,
> +                                       "%d@%d,", i, lcores[i]);
> +               if (len > 0)
> +                       lcorestr[len-1] = 0;

len - 1


> +               RTE_LOG(ERR, EAL, "to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,\n");
> +               RTE_LOG(ERR, EAL, "    e.g. --lcores %s\n", lcorestr);
> +               free(lcorestr);
> +               return -1;
> +       }
> +       return 0;
> +}
> +
>  static int
>  eal_parse_coremask(const char *coremask, int *cores)
>  {
> @@ -833,54 +874,87 @@ eal_parse_service_corelist(const char *corelist)
>  static int
>  eal_parse_corelist(const char *corelist, int *cores)
>  {
> -       unsigned count = 0;
> +       unsigned int count = 0, k;

No need for k, count is enough.


>         char *end = NULL;
>         int min, max;
>         int idx;
> +       int lcores[RTE_MAX_LCORE];
> +       char *corelist_copy;

corelist_copy is unused.


>
>         for (idx = 0; idx < RTE_MAX_LCORE; idx++)
>                 cores[idx] = -1;
>
> +       corelist_copy = strdup(corelist);
> +       if (corelist_copy == NULL) {
> +               RTE_LOG(ERR, EAL, "Unable to duplicate corelist\n");
> +               return -ENOMEM;
> +       }
> +
>         /* Remove all blank characters ahead */
>         while (isblank(*corelist))
>                 corelist++;
>
>         /* Get list of cores */
> -       min = RTE_MAX_LCORE;
> +       min = -1;
>         do {
>                 while (isblank(*corelist))
>                         corelist++;
>                 if (*corelist == '\0')
> -                       return -1;
> +                       goto err;
>                 errno = 0;
>                 idx = strtol(corelist, &end, 10);
>                 if (errno || end == NULL)
> -                       return -1;
> -               if (idx < 0 || idx >= RTE_MAX_LCORE)
> -                       return -1;
> +                       goto err;
> +               if (idx < 0)
> +                       goto err;
>                 while (isblank(*end))
>                         end++;
>                 if (*end == '-') {
>                         min = idx;
>                 } else if ((*end == ',') || (*end == '\0')) {
>                         max = idx;
> -                       if (min == RTE_MAX_LCORE)
> +                       if (min == -1)
>                                 min = idx;
>                         for (idx = min; idx <= max; idx++) {
> -                               if (cores[idx] == -1) {
> -                                       cores[idx] = count;
> -                                       count++;

Here, in the original code, "duplicates" were accepted/ignored.


> +                               if (count < RTE_MAX_LCORE)
> +                                       lcores[count++] = idx;
> +                               else {
> +                                       RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
> +                                                       RTE_MAX_LCORE);
> +                                       goto err;
>                                 }
>                         }
> -                       min = RTE_MAX_LCORE;
> +                       min = -1;
>                 } else
> -                       return -1;
> +                       goto err;
>                 corelist = end + 1;
>         } while (*end != '\0');
>
>         if (count == 0)
> -               return -1;
> +               goto err;
> +
> +       if (check_core_list(lcores, count))
> +               goto err;
> +
> +       /*
> +        * Now that we've gto a list of cores no longer than

typo.

> +        * RTE_MAX_LCORE, and no lcore in that list is greater
> +        * than RTE_MAX_LCORE, populate the cores
> +        * array and return.
> +        */
> +
> +       for (k = 0; k < count; k++)
> +               cores[lcores[k]] = k;

do {
   count--;
   cores[lcores[count]] = count;
while (count != 0);


    cores[count]

> +
> +       if (corelist_copy)
> +               free(corelist_copy);
> +
>         return 0;
> +err:
> +       if (corelist_copy)
> +               free(corelist_copy);
> +
> +       return -1;
>  }
>
>  /* Changes the lcore id of the main thread */
> --
> 2.17.1
>


-- 
David Marchand

Re: [dpdk-dev] [PATCH v4 2/2] eal: add additional info if core mask too long

[David Marchand]

On Wed, Sep 22, 2021 at 2:29 PM David Hunt <david.hunt@intel.com> wrote:
>
> If the user requests to use an lcore above 128 using -c,
> the eal will exit with "EAL: invalid core list syntax" and
> very little else useful information.

invalid coremask*


>
> This patch adds some extra information suggesting to use --lcores
> so that physical cores above RTE_MAX_LCORE (default 128) can be
> used. This is achieved by using the --lcores option by mapping
> the logical cores in the application to physical cores.
>
> For example, if "-c 0x300000000000000000000000000000000" is
> used, we see the following additional output on the command line:
>
> EAL: lcore 128 >= RTE_MAX_LCORE (128)
> EAL: lcore 129 >= RTE_MAX_LCORE (128)
> EAL: to use high physical core ids , please use --lcores to
> map them to lcore ids below RTE_MAX_LCORE,
> EAL:     e.g. --lcores 0@128,1@129
>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> changes in v3
>    * added this patch to the set. Addresses the changes for
>      the -c option.
> changes in v4
>    * fixed buffer overrun in populating lcore array
>    * switched from strlcpy to strdup due to a clang error
> ---
> ---
>  lib/eal/common/eal_common_options.c | 66 +++++++++++++++++++++++------
>  1 file changed, 54 insertions(+), 12 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
> index de1717946f..19d3588f78 100644
> --- a/lib/eal/common/eal_common_options.c
> +++ b/lib/eal/common/eal_common_options.c
> @@ -747,15 +747,23 @@ check_core_list(int *lcores, unsigned int count)
>  static int
>  eal_parse_coremask(const char *coremask, int *cores)
>  {
> -       unsigned count = 0;
> +       unsigned int count = 0, k;

Same as patch 1, no need for count.


>         int i, j, idx;
>         int val;
>         char c;
> +       int lcores[RTE_MAX_LCORE];
> +       char *coremask_copy = NULL;

Contrary to patch 1, coremask_copy is used.
But, coremask is const, we can directly use it in log messages.

So same as patch 1, please remove coremask_copy.


>
>         for (idx = 0; idx < RTE_MAX_LCORE; idx++)
>                 cores[idx] = -1;
>         idx = 0;
>
> +       coremask_copy = strdup(coremask);
> +       if (coremask_copy == NULL) {
> +               RTE_LOG(ERR, EAL, "Unable to duplicate coremask\n");
> +               return -ENOMEM;
> +       }
> +
>         /* Remove all blank characters ahead and after .
>          * Remove 0x/0X if exists.
>          */
> @@ -767,30 +775,64 @@ eal_parse_coremask(const char *coremask, int *cores)
>         i = strlen(coremask);
>         while ((i > 0) && isblank(coremask[i - 1]))
>                 i--;
> -       if (i == 0)
> -               return -1;
> +       if (i == 0) {
> +               RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
> +               goto err;
> +       }
>
> -       for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
> +       for (i = i - 1; i >= 0; i--) {
>                 c = coremask[i];
>                 if (isxdigit(c) == 0) {
>                         /* invalid characters */
> -                       return -1;
> +                       RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
> +                                       coremask_copy);
> +                       goto err;
>                 }
>                 val = xdigit2val(c);
> -               for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
> +               for (j = 0; j < BITS_PER_HEX; j++, idx++)
>                 {
>                         if ((1 << j) & val) {
> -                               cores[idx] = count;
> -                               count++;
> +                               if (count < RTE_MAX_LCORE) {
> +                                       lcores[count++] = idx;
> +                               } else {
> +                                       RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
> +                                                       RTE_MAX_LCORE);
> +                                       goto err;
> +                               }

You can invert those blocks:

if (count >= RTE_MAX_LCORE) {
   RTE_LOG();
   goto err;
}

lcores[count] = idx;
count++;



>                         }
>                 }
>         }
>         for (; i >= 0; i--)
> -               if (coremask[i] != '0')
> -                       return -1;
> -       if (count == 0)
> -               return -1;
> +               if (coremask[i] != '0') {
> +                       RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
> +                                       coremask_copy);
> +                       goto err;
> +               }
> +       if (count == 0) {
> +               RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
> +               goto err;
> +       }
> +
> +       if (check_core_list(lcores, count))
> +               goto err;
> +
> +       /*
> +        * Now that we've gto a list of cores no longer than
> +        * RTE_MAX_LCORE, and no lcore in that list is greater
> +        * than RTE_MAX_LCORE, populate the cores
> +        * array and return.
> +        */
> +       for (k = 0; k < count; k++)
> +               cores[lcores[k]] = k;
> +
> +       if (coremask_copy)
> +               free(coremask_copy);
> +
>         return 0;
> +err:
> +       if (coremask_copy)
> +               free(coremask_copy);
> +       return -1;
>  }
>
>  static int
> --
> 2.17.1
>



-- 
David Marchand

Re: [dpdk-dev] [PATCH v4 1/2] eal: add additional info if core list too long

[David Hunt]

On 23/9/2021 9:11 AM, David Marchand wrote:
> On Wed, Sep 22, 2021 at 2:29 PM David Hunt <david.hunt@intel.com> wrote:
>> If the user requests to use an lcore above 128 using -l,
>> the eal will exit with "EAL: invalid core list syntax" and
>> very little else useful information.
>>
>> This patch adds some extra information suggesting to use --lcores
>> so that physical cores above RTE_MAX_LCORE (default 128) can be
>> used. This is achieved by using the --lcores option by mapping
>> the logical cores in the application to physical cores.
>>
>> For example, if "-l 12-16,130,132" is used, we see the following
>> additional output on the command line:
>>
>> EAL: lcore 132 >= RTE_MAX_LCORE (128)
>> EAL: lcore 133 >= RTE_MAX_LCORE (128)
>> EAL: to use high physical core ids , please use --lcores to map
>> them to lcore ids below RTE_MAX_LCORE,
>> EAL:     e.g. --lcores 0@12,1@13,2@14,3@15,4@16,5@132,6@133
>>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> This series is there to help users, it should not break existing
> working configurations.
>
> I mentionned the "-l 0-3,0" case before.
> This syntax is debatable, but it worked before (see comment below) and
> this patch now refuses it.
>

Hi David,

Good point that the patch should not change behaviour. Will address, 
along with your other comments.

Thanks,
Dave.



--snip--

Re: [dpdk-dev] [PATCH v4 2/2] eal: add additional info if core mask too long

[David Hunt]

On 23/9/2021 9:12 AM, David Marchand wrote:
> On Wed, Sep 22, 2021 at 2:29 PM David Hunt <david.hunt@intel.com> wrote:
>>
>>          int i, j, idx;
>>          int val;
>>          char c;
>> +       int lcores[RTE_MAX_LCORE];
>> +       char *coremask_copy = NULL;
> Contrary to patch 1, coremask_copy is used.
> But, coremask is const, we can directly use it in log messages.
>
> So same as patch 1, please remove coremask_copy.


const protects the data being pointed at from being modified, but the 
pointer itself can still be modified, and the pointer is indeed 
incremented to cut off leading zeroes and spaces.

For example, if I provide "  0x0" as a parameter,coremask prints as "0", 
where as coremask_copy prints as "  0x0".

Similarly, if I provide "0x" as a parameter,coremask prints as "", where 
as coremask_copy prints as "  0x".

Instead of using strdup, I can use "const char *coremask_orig = 
coremask;", and remove the strdup and the frees...


>
>>          for (idx = 0; idx < RTE_MAX_LCORE; idx++)
>>                  cores[idx] = -1;
>>          idx = 0;
>>
>> +       coremask_copy = strdup(coremask);
>> +       if (coremask_copy == NULL) {
>> +               RTE_LOG(ERR, EAL, "Unable to duplicate coremask\n");
>> +               return -ENOMEM;
>> +       }
>> +
>>          /* Remove all blank characters ahead and after .
>>           * Remove 0x/0X if exists.
>>           */
>> @@ -767,30 +775,64 @@ eal_parse_coremask(const char *coremask, int *cores)
>>          i = strlen(coremask);
>>          while ((i > 0) && isblank(coremask[i - 1]))
>>                  i--;
>> -       if (i == 0)
>> -               return -1;
>> +       if (i == 0) {
>> +               RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
>> +               goto err;
>> +       }
>>
>> -       for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
>> +       for (i = i - 1; i >= 0; i--) {
>>                  c = coremask[i];
>>                  if (isxdigit(c) == 0) {
>>                          /* invalid characters */
>> -                       return -1;
>> +                       RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
>> +                                       coremask_copy);
>> +                       goto err;
>>                  }
>>                  val = xdigit2val(c);
>> -               for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
>> +               for (j = 0; j < BITS_PER_HEX; j++, idx++)
>>                  {
>>                          if ((1 << j) & val) {
>> -                               cores[idx] = count;
>> -                               count++;
>> +                               if (count < RTE_MAX_LCORE) {
>> +                                       lcores[count++] = idx;
>> +                               } else {
>> +                                       RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
>> +                                                       RTE_MAX_LCORE);
>> +                                       goto err;
>> +                               }
> You can invert those blocks:
>
> if (count >= RTE_MAX_LCORE) {
>     RTE_LOG();
>     goto err;
> }
>
> lcores[count] = idx;
> count++;
>

+1


>
>>                          }
>>                  }
>>          }
>>          for (; i >= 0; i--)
>> -               if (coremask[i] != '0')
>> -                       return -1;
>> -       if (count == 0)
>> -               return -1;
>> +               if (coremask[i] != '0') {
>> +                       RTE_LOG(ERR, EAL, "invalid characters in coremask: %s\n",
>> +                                       coremask_copy);
>> +                       goto err;
>> +               }
>> +       if (count == 0) {
>> +               RTE_LOG(ERR, EAL, "No lcores in coremask: %s\n", coremask_copy);
>> +               goto err;
>> +       }
>> +
>> +       if (check_core_list(lcores, count))
>> +               goto err;
>> +
>> +       /*
>> +        * Now that we've gto a list of cores no longer than
>> +        * RTE_MAX_LCORE, and no lcore in that list is greater
>> +        * than RTE_MAX_LCORE, populate the cores
>> +        * array and return.
>> +        */
>> +       for (k = 0; k < count; k++)
>> +               cores[lcores[k]] = k;
>> +
>> +       if (coremask_copy)
>> +               free(coremask_copy);
>> +
>>          return 0;
>> +err:
>> +       if (coremask_copy)
>> +               free(coremask_copy);
>> +       return -1;
>>   }
>>
>>   static int
>> --
>> 2.17.1
>>
>
>

[dpdk-dev] [PATCH v5 1/2] eal: add additional info if core list too long

[David Hunt]

If the user requests to use an lcore above 128 using -l,
the eal will exit with "EAL: invalid core list syntax" and
very little else useful information.

This patch adds some extra information suggesting to use --lcores
so that physical cores above RTE_MAX_LCORE (default 128) can be
used. This is achieved by using the --lcores option by mapping
the logical cores in the application to physical cores.

For example, if "-l 12-16,130,132" is used, we see the following
additional output on the command line:

EAL: lcore 132 >= RTE_MAX_LCORE (128)
EAL: lcore 133 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to map
them to lcore ids below RTE_MAX_LCORE,
EAL:     e.g. --lcores 0@12,1@13,2@14,3@15,4@16,5@132,6@133

Signed-off-by: David Hunt <david.hunt@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
changes in v2
   * Rather than increasing the default max lcores (as in v1),
     it was agreed to do this instead (switch to --lcores).
   * As the other patches in the v1 of the set are no longer related
     to this change, I'll submit as a separate patch set.
changes in v3
   * separated out some of the corelist cheking into separate function
   * added extra messages for the different failure conditions.
   * changed allocation of the core strings from static to dynamic
   * now prints out a message for each core above RTE_MAX_LCORE
changes in v4
   * tweaked log messages to be a bit clearer about mapping lcores
     to physical cores.
   * improved indenting of log messages.
   * fixed bug in overrunning end of lcore array
   * switched from strlcpy to strdup because of a clang error
changes in v5
   * removed un-needed index variables
   * now ensures snprintf ret > 0 before using
   * removed corelist_copy, not needed.
   * reverted err: usage to -1, as no free() needed.
   * added in code to allow duplicates
   * other code cleanups
---
---
 lib/eal/common/eal_common_options.c | 87 ++++++++++++++++++++++++++---
 1 file changed, 79 insertions(+), 8 deletions(-)

diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
index eaef57312f..72735e0b09 100644
--- a/lib/eal/common/eal_common_options.c
+++ b/lib/eal/common/eal_common_options.c
@@ -703,6 +703,50 @@ update_lcore_config(int *cores)
 	return ret;
 }
 
+static int
+check_core_list(int *lcores, unsigned int count)
+{
+	unsigned int i;
+	char *lcorestr;
+	int len = 0, ret;
+	bool overflow = false;
+
+	for (i = 0; i < count; i++) {
+		if (lcores[i] >= RTE_MAX_LCORE) {
+			RTE_LOG(ERR, EAL, "lcore %d >= RTE_MAX_LCORE (%d)\n",
+					lcores[i], RTE_MAX_LCORE);
+			overflow = true;
+		}
+	}
+	if (!overflow)
+		return 0;
+
+	/*
+	 * We've encountered a core that's greater than
+	 * RTE_MAX_LCORE, suggest using --lcores option to
+	 * map lcores onto physical cores greater than
+	 * RTE_MAX_LCORE, then return.
+	 */
+	lcorestr = calloc(1, RTE_MAX_LCORE * 10);
+	if (lcorestr == NULL) {
+		RTE_LOG(ERR, EAL, "Unable to allocate lcore string\n");
+		return -ENOMEM;
+	}
+	for (i = 0; i < count; i++) {
+		ret = snprintf(&lcorestr[len],
+				RTE_MAX_LCORE * 10 - len,
+				"%d@%d,", i, lcores[i]);
+		if (ret > 0)
+			len = len + ret;
+	}
+	if (len > 0)
+		lcorestr[len - 1] = 0;
+	RTE_LOG(ERR, EAL, "to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,\n");
+	RTE_LOG(ERR, EAL, "    e.g. --lcores %s\n", lcorestr);
+	free(lcorestr);
+	return -1;
+}
+
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {
@@ -833,10 +877,12 @@ eal_parse_service_corelist(const char *corelist)
 static int
 eal_parse_corelist(const char *corelist, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0, i;
 	char *end = NULL;
 	int min, max;
 	int idx;
+	int lcores[RTE_MAX_LCORE];
+	bool dup;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
@@ -846,7 +892,7 @@ eal_parse_corelist(const char *corelist, int *cores)
 		corelist++;
 
 	/* Get list of cores */
-	min = RTE_MAX_LCORE;
+	min = -1;
 	do {
 		while (isblank(*corelist))
 			corelist++;
@@ -856,7 +902,7 @@ eal_parse_corelist(const char *corelist, int *cores)
 		idx = strtol(corelist, &end, 10);
 		if (errno || end == NULL)
 			return -1;
-		if (idx < 0 || idx >= RTE_MAX_LCORE)
+		if (idx < 0)
 			return -1;
 		while (isblank(*end))
 			end++;
@@ -864,15 +910,25 @@ eal_parse_corelist(const char *corelist, int *cores)
 			min = idx;
 		} else if ((*end == ',') || (*end == '\0')) {
 			max = idx;
-			if (min == RTE_MAX_LCORE)
+			if (min == -1)
 				min = idx;
 			for (idx = min; idx <= max; idx++) {
-				if (cores[idx] == -1) {
-					cores[idx] = count;
-					count++;
+				dup = false;
+				/* Check if this idx is already present */
+				for (i = 0; i < count; i++)
+					if (lcores[i] == idx)
+						dup = true;
+				if (dup)
+					continue;
+				if (count < RTE_MAX_LCORE)
+					lcores[count++] = idx;
+				else {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					return -1;
 				}
 			}
-			min = RTE_MAX_LCORE;
+			min = -1;
 		} else
 			return -1;
 		corelist = end + 1;
@@ -880,6 +936,21 @@ eal_parse_corelist(const char *corelist, int *cores)
 
 	if (count == 0)
 		return -1;
+
+	if (check_core_list(lcores, count))
+		return -1;
+
+	/*
+	 * Now that we've got a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+	do {
+		count--;
+		cores[lcores[count]] = count;
+	} while (count != 0);
+
 	return 0;
 }
 
-- 
2.17.1

[dpdk-dev] [PATCH v5 2/2] eal: add additional info if core mask too long

[David Hunt]

If the user requests to use an lcore above 128 using -c,
the eal will exit with "EAL: invalid coremask syntax" and
very little else useful information.

This patch adds some extra information suggesting to use --lcores
so that physical cores above RTE_MAX_LCORE (default 128) can be
used. This is achieved by using the --lcores option by mapping
the logical cores in the application to physical cores.

For example, if "-c 0x300000000000000000000000000000000" is
used, we see the following additional output on the command line:

EAL: lcore 128 >= RTE_MAX_LCORE (128)
EAL: lcore 129 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to
map them to lcore ids below RTE_MAX_LCORE,
EAL:     e.g. --lcores 0@128,1@129

Signed-off-by: David Hunt <david.hunt@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
changes in v3
   * added this patch to the set. Addresses the changes for
     the -c option.
changes in v4
   * fixed buffer overrun in populating lcore array.
   * switched from strlcpy to strdup due to a clang error.
changes in v5
   * replaced strdup and frees with a const char *, as we
     just need to keep track of original pointer location.
   * reverted err: usage to return -1, as no free() needed.
   * other minod code cleanups.
---
---
 lib/eal/common/eal_common_options.c | 47 ++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
index 72735e0b09..7f715e4c15 100644
--- a/lib/eal/common/eal_common_options.c
+++ b/lib/eal/common/eal_common_options.c
@@ -750,10 +750,12 @@ check_core_list(int *lcores, unsigned int count)
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0;
 	int i, j, idx;
 	int val;
 	char c;
+	int lcores[RTE_MAX_LCORE];
+	const char *coremask_orig = coremask;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
@@ -770,29 +772,60 @@ eal_parse_coremask(const char *coremask, int *cores)
 	i = strlen(coremask);
 	while ((i > 0) && isblank(coremask[i - 1]))
 		i--;
-	if (i == 0)
+	if (i == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
+				coremask_orig);
 		return -1;
+	}
 
-	for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
+	for (i = i - 1; i >= 0; i--) {
 		c = coremask[i];
 		if (isxdigit(c) == 0) {
 			/* invalid characters */
+			RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
+					coremask_orig);
 			return -1;
 		}
 		val = xdigit2val(c);
-		for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
+		for (j = 0; j < BITS_PER_HEX; j++, idx++)
 		{
 			if ((1 << j) & val) {
-				cores[idx] = count;
+				if (count >= RTE_MAX_LCORE) {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					return -1;
+				}
+				lcores[count] = idx;
 				count++;
 			}
 		}
 	}
 	for (; i >= 0; i--)
-		if (coremask[i] != '0')
+		if (coremask[i] != '0') {
+			RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
+					coremask_orig);
 			return -1;
-	if (count == 0)
+		}
+	if (count == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
+				coremask_orig);
+		return -1;
+	}
+
+	if (check_core_list(lcores, count))
 		return -1;
+
+	/*
+	 * Now that we've gto a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+	do {
+		count--;
+		cores[lcores[count]] = count;
+	} while (count != 0);
+
 	return 0;
 }
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH v5 2/2] eal: add additional info if core mask too long

[David Marchand]

On Thu, Sep 23, 2021 at 1:03 PM David Hunt <david.hunt@intel.com> wrote:
>
> If the user requests to use an lcore above 128 using -c,
> the eal will exit with "EAL: invalid coremask syntax" and
> very little else useful information.
>
> This patch adds some extra information suggesting to use --lcores
> so that physical cores above RTE_MAX_LCORE (default 128) can be
> used. This is achieved by using the --lcores option by mapping
> the logical cores in the application to physical cores.
>
> For example, if "-c 0x300000000000000000000000000000000" is
> used, we see the following additional output on the command line:
>
> EAL: lcore 128 >= RTE_MAX_LCORE (128)
> EAL: lcore 129 >= RTE_MAX_LCORE (128)
> EAL: to use high physical core ids , please use --lcores to
> map them to lcore ids below RTE_MAX_LCORE,
> EAL:     e.g. --lcores 0@128,1@129
>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> changes in v3
>    * added this patch to the set. Addresses the changes for
>      the -c option.
> changes in v4
>    * fixed buffer overrun in populating lcore array.
>    * switched from strlcpy to strdup due to a clang error.
> changes in v5
>    * replaced strdup and frees with a const char *, as we
>      just need to keep track of original pointer location.
>    * reverted err: usage to return -1, as no free() needed.
>    * other minod code cleanups.
> ---
> ---
>  lib/eal/common/eal_common_options.c | 47 ++++++++++++++++++++++++-----
>  1 file changed, 40 insertions(+), 7 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
> index 72735e0b09..7f715e4c15 100644
> --- a/lib/eal/common/eal_common_options.c
> +++ b/lib/eal/common/eal_common_options.c
> @@ -750,10 +750,12 @@ check_core_list(int *lcores, unsigned int count)
>  static int
>  eal_parse_coremask(const char *coremask, int *cores)
>  {
> -       unsigned count = 0;
> +       unsigned int count = 0;
>         int i, j, idx;
>         int val;
>         char c;
> +       int lcores[RTE_MAX_LCORE];
> +       const char *coremask_orig = coremask;
>
>         for (idx = 0; idx < RTE_MAX_LCORE; idx++)
>                 cores[idx] = -1;
> @@ -770,29 +772,60 @@ eal_parse_coremask(const char *coremask, int *cores)
>         i = strlen(coremask);
>         while ((i > 0) && isblank(coremask[i - 1]))
>                 i--;
> -       if (i == 0)
> +       if (i == 0) {
> +               RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
> +                               coremask_orig);
>                 return -1;
> +       }
>
> -       for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
> +       for (i = i - 1; i >= 0; i--) {

This loop exit condition changes here: this ensures that, once we
leave the loop, i == -1.
As a consequence... (see below)


>                 c = coremask[i];
>                 if (isxdigit(c) == 0) {
>                         /* invalid characters */
> +                       RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
> +                                       coremask_orig);
>                         return -1;
>                 }
>                 val = xdigit2val(c);
> -               for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
> +               for (j = 0; j < BITS_PER_HEX; j++, idx++)
>                 {
>                         if ((1 << j) & val) {
> -                               cores[idx] = count;
> +                               if (count >= RTE_MAX_LCORE) {
> +                                       RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
> +                                                       RTE_MAX_LCORE);
> +                                       return -1;
> +                               }
> +                               lcores[count] = idx;
>                                 count++;
>                         }
>                 }
>         }

... this loop below is dead code.

>         for (; i >= 0; i--)
> -               if (coremask[i] != '0')
> +               if (coremask[i] != '0') {
> +                       RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
> +                                       coremask_orig);

Nit: capital letter.

>                         return -1;
> -       if (count == 0)
> +               }
> +       if (count == 0) {
> +               RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
> +                               coremask_orig);
> +               return -1;
> +       }
> +
> +       if (check_core_list(lcores, count))
>                 return -1;
> +
> +       /*
> +        * Now that we've gto a list of cores no longer than

Same typo I commented on patch 1 for v4.



> +        * RTE_MAX_LCORE, and no lcore in that list is greater
> +        * than RTE_MAX_LCORE, populate the cores
> +        * array and return.
> +        */
> +       do {
> +               count--;
> +               cores[lcores[count]] = count;
> +       } while (count != 0);
> +
>         return 0;
>  }
>
> --
> 2.17.1
>

-- 
David Marchand

Re: [dpdk-dev] [PATCH v5 2/2] eal: add additional info if core mask too long

[David Hunt]

Hi David,

On 2/11/2021 5:45 PM, David Marchand wrote:
> On Thu, Sep 23, 2021 at 1:03 PM David Hunt <david.hunt@intel.com> wrote:
>> If the user requests to use an lcore above 128 using -c,
>> the eal will exit with "EAL: invalid coremask syntax" and
>> very little else useful information.
>>
>> This patch adds some extra information suggesting to use --lcores
>> so that physical cores above RTE_MAX_LCORE (default 128) can be
>> used. This is achieved by using the --lcores option by mapping
>> the logical cores in the application to physical cores.
>>
>> For example, if "-c 0x300000000000000000000000000000000" is
>> used, we see the following additional output on the command line:
>>
>> EAL: lcore 128 >= RTE_MAX_LCORE (128)
>> EAL: lcore 129 >= RTE_MAX_LCORE (128)
>> EAL: to use high physical core ids , please use --lcores to
>> map them to lcore ids below RTE_MAX_LCORE,
>> EAL:     e.g. --lcores 0@128,1@129
>>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>> ---
>> changes in v3
>>     * added this patch to the set. Addresses the changes for
>>       the -c option.
>> changes in v4
>>     * fixed buffer overrun in populating lcore array.
>>     * switched from strlcpy to strdup due to a clang error.
>> changes in v5
>>     * replaced strdup and frees with a const char *, as we
>>       just need to keep track of original pointer location.
>>     * reverted err: usage to return -1, as no free() needed.
>>     * other minod code cleanups.
>> ---
>> ---
>>   lib/eal/common/eal_common_options.c | 47 ++++++++++++++++++++++++-----
>>   1 file changed, 40 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
>> index 72735e0b09..7f715e4c15 100644
>> --- a/lib/eal/common/eal_common_options.c
>> +++ b/lib/eal/common/eal_common_options.c
>> @@ -750,10 +750,12 @@ check_core_list(int *lcores, unsigned int count)
>>   static int
>>   eal_parse_coremask(const char *coremask, int *cores)
>>   {
>> -       unsigned count = 0;
>> +       unsigned int count = 0;
>>          int i, j, idx;
>>          int val;
>>          char c;
>> +       int lcores[RTE_MAX_LCORE];
>> +       const char *coremask_orig = coremask;
>>
>>          for (idx = 0; idx < RTE_MAX_LCORE; idx++)
>>                  cores[idx] = -1;
>> @@ -770,29 +772,60 @@ eal_parse_coremask(const char *coremask, int *cores)
>>          i = strlen(coremask);
>>          while ((i > 0) && isblank(coremask[i - 1]))
>>                  i--;
>> -       if (i == 0)
>> +       if (i == 0) {
>> +               RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
>> +                               coremask_orig);
>>                  return -1;
>> +       }
>>
>> -       for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
>> +       for (i = i - 1; i >= 0; i--) {
> This loop exit condition changes here: this ensures that, once we
> leave the loop, i == -1.
> As a consequence... (see below)
>
>
>>                  c = coremask[i];
>>                  if (isxdigit(c) == 0) {
>>                          /* invalid characters */
>> +                       RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
>> +                                       coremask_orig);
>>                          return -1;
>>                  }
>>                  val = xdigit2val(c);
>> -               for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
>> +               for (j = 0; j < BITS_PER_HEX; j++, idx++)
>>                  {
>>                          if ((1 << j) & val) {
>> -                               cores[idx] = count;
>> +                               if (count >= RTE_MAX_LCORE) {
>> +                                       RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
>> +                                                       RTE_MAX_LCORE);
>> +                                       return -1;
>> +                               }
>> +                               lcores[count] = idx;
>>                                  count++;
>>                          }
>>                  }
>>          }
> ... this loop below is dead code.


Sure, no need to loop. I'll take out the loop, and just check for the 
first two characters to be '0x', as they're already trimmed.


>>          for (; i >= 0; i--)
>> -               if (coremask[i] != '0')
>> +               if (coremask[i] != '0') {
>> +                       RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
>> +                                       coremask_orig);
> Nit: capital letter.


Many other instances of 'invalid' in this file, will I change them all 
to "Invalid", or just this one?


>
>>                          return -1;
>> -       if (count == 0)
>> +               }
>> +       if (count == 0) {
>> +               RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
>> +                               coremask_orig);
>> +               return -1;
>> +       }
>> +
>> +       if (check_core_list(lcores, count))
>>                  return -1;
>> +
>> +       /*
>> +        * Now that we've gto a list of cores no longer than
> Same typo I commented on patch 1 for v4.

Will fix.


>
>
>> +        * RTE_MAX_LCORE, and no lcore in that list is greater
>> +        * than RTE_MAX_LCORE, populate the cores
>> +        * array and return.
>> +        */
>> +       do {
>> +               count--;
>> +               cores[lcores[count]] = count;
>> +       } while (count != 0);
>> +
>>          return 0;
>>   }
>>
>> --
>> 2.17.1
>>

Re: [dpdk-dev] [PATCH v5 2/2] eal: add additional info if core mask too long

[David Marchand]

On Wed, Nov 3, 2021 at 11:27 AM David Hunt <david.hunt@intel.com> wrote:
>
> >>          for (; i >= 0; i--)
> >> -               if (coremask[i] != '0')
> >> +               if (coremask[i] != '0') {
> >> +                       RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
> >> +                                       coremask_orig);
> > Nit: capital letter.
>
>
> Many other instances of 'invalid' in this file, will I change them all
> to "Invalid", or just this one?

Let's be consistent for additions of this patch.
Thanks.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v5 2/2] eal: add additional info if core mask too long

[David Hunt]

On 3/11/2021 10:27 AM, David Hunt wrote:
> Hi David,
>
> On 2/11/2021 5:45 PM, David Marchand wrote:
>> On Thu, Sep 23, 2021 at 1:03 PM David Hunt <david.hunt@intel.com> wrote:
>>> If the user requests to use an lcore above 128 using -c,
>>> the eal will exit with "EAL: invalid coremask syntax" and
>>> very little else useful information.
>>>
>>> This patch adds some extra information suggesting to use --lcores
>>> so that physical cores above RTE_MAX_LCORE (default 128) can be
>>> used. This is achieved by using the --lcores option by mapping
>>> the logical cores in the application to physical cores.
>>>
>>> For example, if "-c 0x300000000000000000000000000000000" is
>>> used, we see the following additional output on the command line:
>>>
>>> EAL: lcore 128 >= RTE_MAX_LCORE (128)
>>> EAL: lcore 129 >= RTE_MAX_LCORE (128)
>>> EAL: to use high physical core ids , please use --lcores to
>>> map them to lcore ids below RTE_MAX_LCORE,
>>> EAL:     e.g. --lcores 0@128,1@129
>>>
>>> Signed-off-by: David Hunt <david.hunt@intel.com>
>>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>>> ---
>>> changes in v3
>>>     * added this patch to the set. Addresses the changes for
>>>       the -c option.
>>> changes in v4
>>>     * fixed buffer overrun in populating lcore array.
>>>     * switched from strlcpy to strdup due to a clang error.
>>> changes in v5
>>>     * replaced strdup and frees with a const char *, as we
>>>       just need to keep track of original pointer location.
>>>     * reverted err: usage to return -1, as no free() needed.
>>>     * other minod code cleanups.
>>> ---
>>> ---
>>>   lib/eal/common/eal_common_options.c | 47 
>>> ++++++++++++++++++++++++-----
>>>   1 file changed, 40 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/lib/eal/common/eal_common_options.c 
>>> b/lib/eal/common/eal_common_options.c
>>> index 72735e0b09..7f715e4c15 100644
>>> --- a/lib/eal/common/eal_common_options.c
>>> +++ b/lib/eal/common/eal_common_options.c
>>> @@ -750,10 +750,12 @@ check_core_list(int *lcores, unsigned int count)
>>>   static int
>>>   eal_parse_coremask(const char *coremask, int *cores)
>>>   {
>>> -       unsigned count = 0;
>>> +       unsigned int count = 0;
>>>          int i, j, idx;
>>>          int val;
>>>          char c;
>>> +       int lcores[RTE_MAX_LCORE];
>>> +       const char *coremask_orig = coremask;
>>>
>>>          for (idx = 0; idx < RTE_MAX_LCORE; idx++)
>>>                  cores[idx] = -1;
>>> @@ -770,29 +772,60 @@ eal_parse_coremask(const char *coremask, int 
>>> *cores)
>>>          i = strlen(coremask);
>>>          while ((i > 0) && isblank(coremask[i - 1]))
>>>                  i--;
>>> -       if (i == 0)
>>> +       if (i == 0) {
>>> +               RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
>>> +                               coremask_orig);
>>>                  return -1;
>>> +       }
>>>
>>> -       for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
>>> +       for (i = i - 1; i >= 0; i--) {
>> This loop exit condition changes here: this ensures that, once we
>> leave the loop, i == -1.
>> As a consequence... (see below)
>>
>>
>>>                  c = coremask[i];
>>>                  if (isxdigit(c) == 0) {
>>>                          /* invalid characters */
>>> +                       RTE_LOG(ERR, EAL, "invalid characters in 
>>> coremask: [%s]\n",
>>> +                                       coremask_orig);
>>>                          return -1;
>>>                  }
>>>                  val = xdigit2val(c);
>>> -               for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; 
>>> j++, idx++)
>>> +               for (j = 0; j < BITS_PER_HEX; j++, idx++)
>>>                  {
>>>                          if ((1 << j) & val) {
>>> -                               cores[idx] = count;
>>> +                               if (count >= RTE_MAX_LCORE) {
>>> +                                       RTE_LOG(ERR, EAL, "Too many 
>>> lcores provided. Cannot exceed %d\n",
>>> + RTE_MAX_LCORE);
>>> +                                       return -1;
>>> +                               }
>>> +                               lcores[count] = idx;
>>>                                  count++;
>>>                          }
>>>                  }
>>>          }
>> ... this loop below is dead code.
>
>
> Sure, no need to loop. I'll take out the loop, and just check for the 
> first two characters to be '0x', as they're already trimmed.
>
>

On second thoughts, looking at this closer, the any '0x' or '0X' at the 
start is skipped earlier in the code, so all we're checking for here is 
a leading zero to the hex, which does not seem valid, as that would mean 
that 0x0ff is valid, but 0xff is not.

Take the following 2 cases:

-c f
EAL: Invalid start [0] to coremask: [f]

and even worse:

-c 0xf
EAL: Invalid start [0] to coremask: [0xf]

So I think it makes sense to remove that particular check altogether, in 
which case both "-c f" and "-c 0xf" will work as expected.

I will make this change in next version.

--snip--

[dpdk-dev] [PATCH v6 1/2] eal: add additional info if core list too long

[David Hunt]

If the user requests to use an lcore above 128 using -l,
the eal will exit with "EAL: invalid core list syntax" and
very little else useful information.

This patch adds some extra information suggesting to use --lcores
so that physical cores above RTE_MAX_LCORE (default 128) can be
used. This is achieved by using the --lcores option by mapping
the logical cores in the application to physical cores.

For example, if "-l 12-16,130,132" is used, we see the following
additional output on the command line:

EAL: lcore 132 >= RTE_MAX_LCORE (128)
EAL: lcore 133 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to map
them to lcore ids below RTE_MAX_LCORE,
EAL:     e.g. --lcores 0@12,1@13,2@14,3@15,4@16,5@132,6@133

Signed-off-by: David Hunt <david.hunt@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

---
changes in v6
   * none
changes in v5
   * removed un-needed index variables
   * now ensures snprintf ret > 0 before using
   * removed corelist_copy, not needed.
   * reverted err: usage to -1, as no free() needed.
   * added in code to allow duplicates
   * other code cleanups
changes in v4
   * tweaked log messages to be a bit clearer about mapping lcores
     to physical cores.
   * improved indenting of log messages.
   * fixed bug in overrunning end of lcore array
   * switched from strlcpy to strdup because of a clang error
changes in v3
   * separated out some of the corelist cheking into separate function
   * added extra messages for the different failure conditions.
   * changed allocation of the core strings from static to dynamic
   * now prints out a message for each core above RTE_MAX_LCORE
changes in v2
   * Rather than increasing the default max lcores (as in v1),
     it was agreed to do this instead (switch to --lcores).
   * As the other patches in the v1 of the set are no longer related
     to this change, I'll submit as a separate patch set.
---
 lib/eal/common/eal_common_options.c | 87 ++++++++++++++++++++++++++---
 1 file changed, 79 insertions(+), 8 deletions(-)

diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
index 1802e3d9e1..c35798b288 100644
--- a/lib/eal/common/eal_common_options.c
+++ b/lib/eal/common/eal_common_options.c
@@ -703,6 +703,50 @@ update_lcore_config(int *cores)
 	return ret;
 }
 
+static int
+check_core_list(int *lcores, unsigned int count)
+{
+	unsigned int i;
+	char *lcorestr;
+	int len = 0, ret;
+	bool overflow = false;
+
+	for (i = 0; i < count; i++) {
+		if (lcores[i] >= RTE_MAX_LCORE) {
+			RTE_LOG(ERR, EAL, "lcore %d >= RTE_MAX_LCORE (%d)\n",
+					lcores[i], RTE_MAX_LCORE);
+			overflow = true;
+		}
+	}
+	if (!overflow)
+		return 0;
+
+	/*
+	 * We've encountered a core that's greater than
+	 * RTE_MAX_LCORE, suggest using --lcores option to
+	 * map lcores onto physical cores greater than
+	 * RTE_MAX_LCORE, then return.
+	 */
+	lcorestr = calloc(1, RTE_MAX_LCORE * 10);
+	if (lcorestr == NULL) {
+		RTE_LOG(ERR, EAL, "Unable to allocate lcore string\n");
+		return -ENOMEM;
+	}
+	for (i = 0; i < count; i++) {
+		ret = snprintf(&lcorestr[len],
+				RTE_MAX_LCORE * 10 - len,
+				"%d@%d,", i, lcores[i]);
+		if (ret > 0)
+			len = len + ret;
+	}
+	if (len > 0)
+		lcorestr[len - 1] = 0;
+	RTE_LOG(ERR, EAL, "to use high physical core ids , please use --lcores to map them to lcore ids below RTE_MAX_LCORE,\n");
+	RTE_LOG(ERR, EAL, "    e.g. --lcores %s\n", lcorestr);
+	free(lcorestr);
+	return -1;
+}
+
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {
@@ -833,10 +877,12 @@ eal_parse_service_corelist(const char *corelist)
 static int
 eal_parse_corelist(const char *corelist, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0, i;
 	char *end = NULL;
 	int min, max;
 	int idx;
+	int lcores[RTE_MAX_LCORE];
+	bool dup;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
@@ -846,7 +892,7 @@ eal_parse_corelist(const char *corelist, int *cores)
 		corelist++;
 
 	/* Get list of cores */
-	min = RTE_MAX_LCORE;
+	min = -1;
 	do {
 		while (isblank(*corelist))
 			corelist++;
@@ -856,7 +902,7 @@ eal_parse_corelist(const char *corelist, int *cores)
 		idx = strtol(corelist, &end, 10);
 		if (errno || end == NULL)
 			return -1;
-		if (idx < 0 || idx >= RTE_MAX_LCORE)
+		if (idx < 0)
 			return -1;
 		while (isblank(*end))
 			end++;
@@ -864,15 +910,25 @@ eal_parse_corelist(const char *corelist, int *cores)
 			min = idx;
 		} else if ((*end == ',') || (*end == '\0')) {
 			max = idx;
-			if (min == RTE_MAX_LCORE)
+			if (min == -1)
 				min = idx;
 			for (idx = min; idx <= max; idx++) {
-				if (cores[idx] == -1) {
-					cores[idx] = count;
-					count++;
+				dup = false;
+				/* Check if this idx is already present */
+				for (i = 0; i < count; i++)
+					if (lcores[i] == idx)
+						dup = true;
+				if (dup)
+					continue;
+				if (count < RTE_MAX_LCORE)
+					lcores[count++] = idx;
+				else {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					return -1;
 				}
 			}
-			min = RTE_MAX_LCORE;
+			min = -1;
 		} else
 			return -1;
 		corelist = end + 1;
@@ -880,6 +936,21 @@ eal_parse_corelist(const char *corelist, int *cores)
 
 	if (count == 0)
 		return -1;
+
+	if (check_core_list(lcores, count))
+		return -1;
+
+	/*
+	 * Now that we've got a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+	do {
+		count--;
+		cores[lcores[count]] = count;
+	} while (count != 0);
+
 	return 0;
 }
 
-- 
2.17.1

[dpdk-dev] [PATCH v6 2/2] eal: add additional info if core mask too long

[David Hunt]

If the user requests to use an lcore above 128 using -c,
the eal will exit with "EAL: invalid coremask syntax" and
very little else useful information.

This patch adds some extra information suggesting to use --lcores
so that physical cores above RTE_MAX_LCORE (default 128) can be
used. This is achieved by using the --lcores option by mapping
the logical cores in the application to physical cores.

For example, if "-c 0x300000000000000000000000000000000" is
used, we see the following additional output on the command line:

EAL: lcore 128 >= RTE_MAX_LCORE (128)
EAL: lcore 129 >= RTE_MAX_LCORE (128)
EAL: to use high physical core ids , please use --lcores to
map them to lcore ids below RTE_MAX_LCORE,
EAL:     e.g. --lcores 0@128,1@129

Signed-off-by: David Hunt <david.hunt@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
changes in v6
   * Fixed typo (gto -> got).
   * Removed dead code for which the intention was to check for 0 at
     the start of the coremask. Also now means that '0xf' is now as
     valid coremask (previously needed to be '0x0f').
changes in v5
   * replaced strdup and frees with a const char *, as we
     just need to keep track of original pointer location.
   * reverted err: usage to return -1, as no free() needed.
   * other minod code cleanups.
changes in v4
   * fixed buffer overrun in populating lcore array.
   * switched from strlcpy to strdup due to a clang error.
changes in v3
   * added this patch to the set. Addresses the changes for
     the -c option.
---
 lib/eal/common/eal_common_options.c | 45 +++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
index c35798b288..0fe3220892 100644
--- a/lib/eal/common/eal_common_options.c
+++ b/lib/eal/common/eal_common_options.c
@@ -750,10 +750,12 @@ check_core_list(int *lcores, unsigned int count)
 static int
 eal_parse_coremask(const char *coremask, int *cores)
 {
-	unsigned count = 0;
+	unsigned int count = 0;
 	int i, j, idx;
 	int val;
 	char c;
+	int lcores[RTE_MAX_LCORE];
+	const char *coremask_orig = coremask;
 
 	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
 		cores[idx] = -1;
@@ -770,29 +772,54 @@ eal_parse_coremask(const char *coremask, int *cores)
 	i = strlen(coremask);
 	while ((i > 0) && isblank(coremask[i - 1]))
 		i--;
-	if (i == 0)
+	if (i == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
+				coremask_orig);
 		return -1;
+	}
 
-	for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) {
+	for (i = i - 1; i >= 0; i--) {
 		c = coremask[i];
 		if (isxdigit(c) == 0) {
 			/* invalid characters */
+			RTE_LOG(ERR, EAL, "invalid characters in coremask: [%s]\n",
+					coremask_orig);
 			return -1;
 		}
 		val = xdigit2val(c);
-		for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; j++, idx++)
+		for (j = 0; j < BITS_PER_HEX; j++, idx++)
 		{
 			if ((1 << j) & val) {
-				cores[idx] = count;
+				if (count >= RTE_MAX_LCORE) {
+					RTE_LOG(ERR, EAL, "Too many lcores provided. Cannot exceed %d\n",
+							RTE_MAX_LCORE);
+					return -1;
+				}
+				lcores[count] = idx;
 				count++;
 			}
 		}
 	}
-	for (; i >= 0; i--)
-		if (coremask[i] != '0')
-			return -1;
-	if (count == 0)
+	if (count == 0) {
+		RTE_LOG(ERR, EAL, "No lcores in coremask: [%s]\n",
+				coremask_orig);
+		return -1;
+	}
+
+	if (check_core_list(lcores, count))
 		return -1;
+
+	/*
+	 * Now that we've got a list of cores no longer than
+	 * RTE_MAX_LCORE, and no lcore in that list is greater
+	 * than RTE_MAX_LCORE, populate the cores
+	 * array and return.
+	 */
+	do {
+		count--;
+		cores[lcores[count]] = count;
+	} while (count != 0);
+
 	return 0;
 }
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH v6 2/2] eal: add additional info if core mask too long

[David Marchand]

On Wed, Nov 3, 2021 at 3:33 PM David Hunt <david.hunt@intel.com> wrote:
>
> If the user requests to use an lcore above 128 using -c,
> the eal will exit with "EAL: invalid coremask syntax" and
> very little else useful information.
>
> This patch adds some extra information suggesting to use --lcores
> so that physical cores above RTE_MAX_LCORE (default 128) can be
> used. This is achieved by using the --lcores option by mapping
> the logical cores in the application to physical cores.
>
> For example, if "-c 0x300000000000000000000000000000000" is
> used, we see the following additional output on the command line:
>
> EAL: lcore 128 >= RTE_MAX_LCORE (128)
> EAL: lcore 129 >= RTE_MAX_LCORE (128)
> EAL: to use high physical core ids , please use --lcores to
> map them to lcore ids below RTE_MAX_LCORE,
> EAL:     e.g. --lcores 0@128,1@129
>
> Signed-off-by: David Hunt <david.hunt@intel.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Series applied, thanks.


-- 
David Marchand

RE: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

[Morten Brørup]

My microphone was not working during the Techboard meeting, so here goes instead:

Please consider if the default of max 128 assigned lcores suffices for the expected lifetime of the 21.11 LTS, or if the default should be bumped to 512 as suggested.

@David Hunt, do you have idea about the memory cost of increasing from 128 to 512? This seems to be the primary objection.

-Morten

Re: [dpdk-dev] [PATCH v1 1/6] build: increase default of max lcores to 512

[David Hunt]

On 17/11/2021 3:55 PM, Morten Brørup wrote:
> My microphone was not working during the Techboard meeting, so here goes instead:
>
> Please consider if the default of max 128 assigned lcores suffices for the expected lifetime of the 21.11 LTS, or if the default should be bumped to 512 as suggested.
>
> @David Hunt, do you have idea about the memory cost of increasing from 128 to 512? This seems to be the primary objection.
>
> -Morten


Hi Morten,

I seem to recall that a simple increase from 128 to 512 cores would 
result in an increased static memory footprint of 2.5MBs per 
application, but there were additional patches in the opriginal patch 
set that switch from static allocation to dynamic allocation reducing 
this to a few hundred kilobytes. This was not deemed the best direction, 
so we went with the improvement of informing the user more clearly on 
how to use the command line syntax to use the physical core numbers 
above 128 as lcores in your application.

I believe the thinking is that 128 lcores is plenty for a single 
application, and if there is a case where more than that is needed (for 
a single application), then the default can be over-ridden by changing 
the max_lcores setting and recompiling.

Rgds,
Dave.