On Mon, Mar 24, 2025 at 6:31 PM Bruce Richardson <bruce.richard...@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.richard...@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