[v4,1/2] eal: add additional info if core list too long
Checks
Commit Message
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(-)
Comments
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
>
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--
@@ -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 */