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