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

[David Marchand <david.marchand@redhat.com>]

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