Re: [PATCH v2 2/3] eal: convert core masks and lists to core sets

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

On Mon, Mar 24, 2025 at 6:31 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> Rather than directly parsing and working with the core mask and core
> list parameters, convert them into core maps, and centralise all core
> parsing there. This allows future work to adjust the mappings of cores
> when generating that mapping parameter.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>
> NOTE: this patch changes the behaviour of the exported, but internal API
> rte_eal_parse_core_mask, used by dlb driver. Since this is an internal
> API changing behaviour is allowed. However, to prevent issues -  in case
> any external app is using the API - we rename the function to
> rte_eal_expand_coremask.

An external app is not supposed to call this but I agree a build error
is more explicit.


> ---
>  drivers/event/dlb2/dlb2_priv.h             |   2 -
>  drivers/event/dlb2/pf/base/dlb2_resource.c |  48 ++---
>  lib/eal/common/eal_common_options.c        | 195 +++++----------------
>  lib/eal/include/rte_eal.h                  |  12 +-
>  lib/eal/version.map                        |   2 +-
>  5 files changed, 60 insertions(+), 199 deletions(-)
>
> diff --git a/drivers/event/dlb2/dlb2_priv.h b/drivers/event/dlb2/dlb2_priv.h
> index 52da31ed31..63c0bea155 100644
> --- a/drivers/event/dlb2/dlb2_priv.h
> +++ b/drivers/event/dlb2/dlb2_priv.h
> @@ -737,8 +737,6 @@ void dlb2_event_build_hcws(struct dlb2_port *qm_port,
>                            uint8_t *sched_type,
>                            uint8_t *queue_id);
>
> -/* Extern functions */
> -extern int rte_eal_parse_coremask(const char *coremask, int *cores);

Ugly...

>
>  /* Extern globals */
>  extern struct process_local_port_data dlb2_port[][DLB2_NUM_PORT_TYPES];
> diff --git a/drivers/event/dlb2/pf/base/dlb2_resource.c b/drivers/event/dlb2/pf/base/dlb2_resource.c
> index 3004902118..9b087b03b5 100644
> --- a/drivers/event/dlb2/pf/base/dlb2_resource.c
> +++ b/drivers/event/dlb2/pf/base/dlb2_resource.c
> @@ -922,49 +922,31 @@ dlb2_resource_probe(struct dlb2_hw *hw, const void *probe_args)
>  {
>         const struct dlb2_devargs *args = (const struct dlb2_devargs *)probe_args;
>         const char *mask = args ? args->producer_coremask : NULL;
> -       int cpu = 0, cnt = 0, cores[RTE_MAX_LCORE], i;
> +       int cpu = 0;
>
>         if (args) {
>                 mask = (const char *)args->producer_coremask;
>         }

I have some trouble with this code:

        const char *mask = args ? args->producer_coremask : NULL;
...
        if (args) {
                mask = (const char *)args->producer_coremask;
        }

It seems redundant...



>
> -       if (mask && rte_eal_parse_coremask(mask, cores)) {
> -               DLB2_HW_ERR(hw, ": Invalid producer coremask=%s\n", mask);
> -               return -1;
> -       }
> -
> +       cpu = rte_get_next_lcore(-1, 1, 0);
>         hw->num_prod_cores = 0;
> -       for (i = 0; i < RTE_MAX_LCORE; i++) {
> -               bool is_pcore = (mask && cores[i] != -1);
> -
> -               if (rte_lcore_is_enabled(i)) {
> -                       if (is_pcore) {
> -                               /*
> -                                * Populate the producer cores from parsed
> -                                * coremask
> -                                */
> -                               hw->prod_core_list[cores[i]] = i;
> -                               hw->num_prod_cores++;
> -
> -                       } else if ((++cnt == DLB2_EAL_PROBE_CORE ||
> -                          rte_lcore_count() < DLB2_EAL_PROBE_CORE)) {
> -                               /*
> -                                * If no producer coremask is provided, use the
> -                                * second EAL core to probe
> -                                */
> -                               cpu = i;
> -                               break;
> -                       }
> -               } else if (is_pcore) {
> -                       DLB2_HW_ERR(hw, "Producer coremask(%s) must be a subset of EAL coremask\n",
> -                                    mask);
> +       if (mask) {
> +               int n = rte_eal_expand_coremask(mask, hw->prod_core_list);
> +               if (n <= 0) {
> +                       DLB2_HW_ERR(hw, ": Invalid producer coremask=%s\n", mask);
>                         return -1;
>                 }
> +               hw->num_prod_cores = n;
> +               cpu = hw->prod_core_list[0];
>
> +               for (u8 i = 0; i < hw->num_prod_cores; i++) {
> +                       if (!rte_lcore_is_enabled(hw->prod_core_list[i])) {
> +                               DLB2_HW_ERR(hw, "Producer coremask(%s) must be a subset of EAL coremask\n",
> +                                            mask);
> +                               return -1;
> +                       }
> +               }
>         }
> -       /* Use the first core in producer coremask to probe */
> -       if (hw->num_prod_cores)
> -               cpu = hw->prod_core_list[0];
>
>         dlb2_get_pp_allocation(hw, cpu, DLB2_LDB_PORT);
>         dlb2_get_pp_allocation(hw, cpu, DLB2_DIR_PORT);
> diff --git a/lib/eal/common/eal_common_options.c b/lib/eal/common/eal_common_options.c
> index 55c49a923f..22ea6b24e4 100644
> --- a/lib/eal/common/eal_common_options.c
> +++ b/lib/eal/common/eal_common_options.c
> @@ -690,33 +690,6 @@ eal_parse_service_coremask(const char *coremask)
>         return 0;
>  }
>
> -static int
> -update_lcore_config(int *cores)
> -{
> -       struct rte_config *cfg = rte_eal_get_configuration();
> -       unsigned int count = 0;
> -       unsigned int i;
> -       int ret = 0;
> -
> -       for (i = 0; i < RTE_MAX_LCORE; i++) {
> -               if (cores[i] != -1) {
> -                       if (eal_cpu_detected(i) == 0) {
> -                               EAL_LOG(ERR, "lcore %u unavailable", i);
> -                               ret = -1;
> -                               continue;
> -                       }
> -                       cfg->lcore_role[i] = ROLE_RTE;
> -                       count++;
> -               } else {
> -                       cfg->lcore_role[i] = ROLE_OFF;
> -               }
> -               lcore_config[i].core_index = cores[i];
> -       }
> -       if (!ret)
> -               cfg->lcore_count = count;
> -       return ret;
> -}
> -
>  static int
>  check_core_list(int *lcores, unsigned int count)
>  {
> @@ -756,10 +729,9 @@ check_core_list(int *lcores, unsigned int count)
>  }
>
>  int
> -rte_eal_parse_coremask(const char *coremask, int *cores)
> +rte_eal_expand_coremask(const char *coremask, int *cores)
>  {
>         const char *coremask_orig = coremask;
> -       int lcores[RTE_MAX_LCORE];
>         unsigned int count = 0;
>         int i, j, idx;
>         int val;
> @@ -803,30 +775,19 @@ rte_eal_parse_coremask(const char *coremask, int *cores)
>                                                 RTE_MAX_LCORE);
>                                         return -1;
>                                 }
> -                               lcores[count++] = idx;
> +                               cores[count++] = idx;
>                         }
>                 }
>         }
>         if (count == 0) {
> -               EAL_LOG(ERR, "No lcores in coremask: [%s]",
> -                       coremask_orig);
> +               EAL_LOG(ERR, "No lcores in coremask: [%s]", coremask_orig);
>                 return -1;
>         }
>
> -       if (check_core_list(lcores, count))
> +       if (check_core_list(cores, count) != 0)
>                 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.
> -        */
> -       do {
> -               count--;
> -               cores[lcores[count]] = count;
> -       } while (count != 0);
> -
> -       return 0;
> +       return count;
>  }
>
>  static int
> @@ -911,7 +872,6 @@ static int
>  eal_parse_corelist(const char *corelist, int *cores)
>  {
>         unsigned int count = 0, i;
> -       int lcores[RTE_MAX_LCORE];
>         char *end = NULL;
>         int min, max;
>         int idx;
> @@ -949,7 +909,7 @@ eal_parse_corelist(const char *corelist, int *cores)
>
>                                 /* Check if this idx is already present */
>                                 for (i = 0; i < count; i++) {
> -                                       if (lcores[i] == idx)
> +                                       if (cores[i] == idx)
>                                                 dup = true;
>                                 }
>                                 if (dup)
> @@ -959,7 +919,7 @@ eal_parse_corelist(const char *corelist, int *cores)
>                                                 RTE_MAX_LCORE);
>                                         return -1;
>                                 }
> -                               lcores[count++] = idx;
> +                               cores[count++] = idx;
>                         }
>                         min = -1;
>                 } else
> @@ -967,23 +927,15 @@ eal_parse_corelist(const char *corelist, int *cores)
>                 corelist = end + 1;
>         } while (*end != '\0');
>
> -       if (count == 0)
> +       if (count == 0) {
> +               EAL_LOG(ERR, "No lcores in corelist");
>                 return -1;
> +       }
>
> -       if (check_core_list(lcores, count))
> +       if (check_core_list(cores, 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.
> -        */
> -       do {
> -               count--;
> -               cores[lcores[count]] = count;
> -       } while (count != 0);
> -
> -       return 0;
> +       return count;
>  }
>
>  /* Changes the lcore id of the main thread */
> @@ -1500,75 +1452,6 @@ eal_parse_base_virtaddr(const char *arg)
>         return 0;
>  }
>
> -/* caller is responsible for freeing the returned string */
> -static char *
> -available_cores(void)
> -{
> -       char *str = NULL;
> -       int previous;
> -       int sequence;
> -       char *tmp;
> -       int idx;
> -
> -       /* find the first available cpu */
> -       for (idx = 0; idx < RTE_MAX_LCORE; idx++) {
> -               if (eal_cpu_detected(idx) == 0)
> -                       continue;
> -               break;
> -       }
> -       if (idx >= RTE_MAX_LCORE)
> -               return NULL;
> -
> -       /* first sequence */
> -       if (asprintf(&str, "%d", idx) < 0)
> -               return NULL;
> -       previous = idx;
> -       sequence = 0;
> -
> -       for (idx++ ; idx < RTE_MAX_LCORE; idx++) {
> -               if (eal_cpu_detected(idx) == 0)
> -                       continue;
> -
> -               if (idx == previous + 1) {
> -                       previous = idx;
> -                       sequence = 1;
> -                       continue;
> -               }
> -
> -               /* finish current sequence */
> -               if (sequence) {
> -                       if (asprintf(&tmp, "%s-%d", str, previous) < 0) {
> -                               free(str);
> -                               return NULL;
> -                       }
> -                       free(str);
> -                       str = tmp;
> -               }
> -
> -               /* new sequence */
> -               if (asprintf(&tmp, "%s,%d", str, idx) < 0) {
> -                       free(str);
> -                       return NULL;
> -               }
> -               free(str);
> -               str = tmp;
> -               previous = idx;
> -               sequence = 0;
> -       }
> -
> -       /* finish last sequence */
> -       if (sequence) {
> -               if (asprintf(&tmp, "%s-%d", str, previous) < 0) {
> -                       free(str);
> -                       return NULL;
> -               }
> -               free(str);
> -               str = tmp;
> -       }
> -
> -       return str;
> -}
> -

Not sure why you remove this helper, it was supposed to help users
with better logs.


>  #define HUGE_UNLINK_NEVER "never"
>
>  static int
> @@ -1981,43 +1864,43 @@ eal_adjust_config(struct internal_config *internal_cfg)
>                 return -1;
>         }
>
> -       if (lcore_options.core_mask_opt) {
> -               int lcore_indexes[RTE_MAX_LCORE];
> -
> -               if (rte_eal_parse_coremask(lcore_options.core_mask_opt, lcore_indexes) < 0) {
> -                       EAL_LOG(ERR, "invalid coremask syntax");
> -                       return -1;
> -               }
> -               if (update_lcore_config(lcore_indexes) < 0) {
> -                       char *available = available_cores();
>
> -                       EAL_LOG(ERR,
> -                               "invalid coremask, please check specified cores are part of %s",
> -                               available);
> -                       free(available);
> -                       return -1;
> -               }
> -       } else if (lcore_options.core_list_opt) {
> +       if (lcore_options.core_mask_opt || lcore_options.core_list_opt) {
>                 int lcore_indexes[RTE_MAX_LCORE];
> +               int nb_indexes = lcore_options.core_list_opt ?
> +                               eal_parse_corelist(lcore_options.core_list_opt, lcore_indexes) :
> +                               rte_eal_expand_coremask(lcore_options.core_mask_opt, lcore_indexes);
>
> -               if (eal_parse_corelist(lcore_options.core_list_opt, lcore_indexes) < 0) {
> -                       EAL_LOG(ERR, "invalid core list syntax");
> +               if (nb_indexes < 0)
>                         return -1;
> -               }
> -               if (update_lcore_config(lcore_indexes) < 0) {
> -                       char *available = available_cores();
>
> -                       EAL_LOG(ERR,
> -                               "invalid core list, please check specified cores are part of %s",
> -                               available);
> -                       free(available);
> -                       return -1;
> +               char *core_map_opt = malloc(RTE_MAX_LCORE * 10);
> +               size_t core_map_len = 0;
> +               for (i = 0; i < nb_indexes; i++) {
> +                       if (!eal_cpu_detected(lcore_indexes[i])) {
> +                               EAL_LOG(ERR, "core %d not present", lcore_indexes[i]);

core_map_opt is leaked (idem in next return).


> +                               return -1;

I would not return here, but instead provide a full report of which
cores are invalid before returning an error.


> +                       }
> +                       int n = snprintf(core_map_opt + core_map_len,
> +                                       (RTE_MAX_LCORE * 10) - core_map_len,
> +                                       "%s%d", i == 0 ? "" : ",", lcore_indexes[i]);
> +                       if (n < 0 || (size_t)n >= (RTE_MAX_LCORE * 10) - core_map_len) {
> +                               EAL_LOG(ERR, "core map string too long");
> +                               return -1;

Or use asprintf.


> +                       }
> +                       core_map_len += n;
>                 }
> -       } else if (lcore_options.core_map_opt) {
> +               lcore_options.core_map_opt = core_map_opt;
> +               EAL_LOG(DEBUG, "Generated core map: '%s'", lcore_options.core_map_opt);
> +       }
> +
> +       if (lcore_options.core_map_opt) {
>                 if (eal_parse_lcores(lcore_options.core_map_opt) < 0) {
>                         EAL_LOG(ERR, "invalid parameter for --" OPT_LCORES);
>                         return -1;
>                 }
> +               if (lcore_options.core_mask_opt || lcore_options.core_list_opt)
> +                       free(RTE_CAST_PTR(void *, lcore_options.core_map_opt));
>         } else {
>                 eal_auto_detect_cores(cfg);
>         }
> diff --git a/lib/eal/include/rte_eal.h b/lib/eal/include/rte_eal.h
> index c826e143f1..013075487c 100644
> --- a/lib/eal/include/rte_eal.h
> +++ b/lib/eal/include/rte_eal.h
> @@ -493,22 +493,20 @@ rte_eal_get_runtime_dir(void);
>  /**
>   * Convert a string describing a mask of core ids into an array of core ids.
>   *
> - * On success, the passed array is filled with the orders of the core ids
> - * present in the mask (-1 indicating that a core id is absent).
> - * For example, passing a 0xa coremask results in cores[1] = 0, cores[3] = 1,
> - * and the rest of the array is set to -1.
> + * On success, the passed array is filled with the core ids present in the mask.
>   *
>   * @param coremask
>   *   A string describing a mask of core ids.
>   * @param cores
> - *   An array where to store the core ids orders.
> + *   The output array to store the core ids.
>   *   This array must be at least RTE_MAX_LCORE large.
>   * @return
> - *   0 on success, -1 if the string content was invalid.
> + *  The number of cores in the coremask, and in the returned "cores" array,
> + *  -1 if the string content was invalid.
>   */
>  __rte_internal
>  int
> -rte_eal_parse_coremask(const char *coremask, int *cores);
> +rte_eal_expand_coremask(const char *coremask, int *cores);
>
>  #ifdef __cplusplus
>  }
> diff --git a/lib/eal/version.map b/lib/eal/version.map
> index a20c713eb1..b51051ee38 100644
> --- a/lib/eal/version.map
> +++ b/lib/eal/version.map
> @@ -405,8 +405,8 @@ INTERNAL {
>
>         rte_bus_register;
>         rte_bus_unregister;
> +       rte_eal_expand_coremask;
>         rte_eal_get_baseaddr;
> -       rte_eal_parse_coremask;
>         rte_firmware_read;
>         rte_intr_allow_others;
>         rte_intr_cap_multiple;
> --
> 2.45.2
>

-- 
David Marchand