Re: [dpdk-dev] [PATCH v4 2/2] eal: add additional info if core mask 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 -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