Hi Thomas, I've sent a v3 for this patch. Twice actually, including dev@dpdk.org the 2nd time :)
Thanks for the ping, -Harry > -----Original Message----- > From: Thomas Monjalon [mailto:tho...@monjalon.net] > Sent: Thursday, July 12, 2018 8:35 AM > To: Van Haaren, Harry <harry.van.haa...@intel.com> > Cc: dev@dpdk.org; Varghese, Vipin <vipin.vargh...@intel.com> > Subject: Re: [dpdk-dev] [PATCH v2] eal/service: improve error checking of > coremasks > > Hi Harry, > > What is the status of this patch? > > > 21/05/2018 11:41, Varghese, Vipin: > > Hi Harry, > > > > This look ok to me, except for one warning rewrite else its ACK from my > end. > > > > > -----Original Message----- > > > From: Van Haaren, Harry > > > Sent: Tuesday, May 15, 2018 9:26 PM > > > To: dev@dpdk.org > > > Cc: Van Haaren, Harry <harry.van.haa...@intel.com>; tho...@monjalon.net; > > > Varghese, Vipin <vipin.vargh...@intel.com> > > > Subject: [PATCH v2] eal/service: improve error checking of coremasks > > > > > > This commit improves the error checking performed on the core masks (or > lists) > > > of the service cores, in particular with respect to the data-plane (RTE) > cores of > > > DPDK. > > > > > > With this commit, invalid configurations are detected at runtime, and > warning > > > messages are printed to inform the user. > > > > > > For example specifying the coremask as 0xf, and the service coremask as > 0xff00 > > > is invalid as not all service-cores are contained within the coremask. A > warning is > > > now printed to inform the user. > > > > > > Reported-by: Vipin Varghese <vipin.vargh...@intel.com> > > > Signed-off-by: Harry van Haaren <harry.van.haa...@intel.com> > > > > > > --- > > > > > > v2, thanks for review: > > > - Consistency in message endings - vs . (Thomas) > > > - Wrap lines as they're very long otherwise (Thomas) > > > > > > Cc: tho...@monjalon.net > > > Cc: vipin.vargh...@intel.com > > > > > > @Thomas, please consider this patch for RC4, it adds checks and prints > > > warnings, better usability, no functional changes. > > > --- > > > lib/librte_eal/common/eal_common_options.c | 43 > > > ++++++++++++++++++++++++++++++ > > > 1 file changed, 43 insertions(+) > > > > > > diff --git a/lib/librte_eal/common/eal_common_options.c > > > b/lib/librte_eal/common/eal_common_options.c > > > index ecebb29..9f3a484 100644 > > > --- a/lib/librte_eal/common/eal_common_options.c > > > +++ b/lib/librte_eal/common/eal_common_options.c > > > @@ -315,6 +315,7 @@ eal_parse_service_coremask(const char *coremask) > > > unsigned int count = 0; > > > char c; > > > int val; > > > + uint32_t taken_lcore_count = 0; > > > > > > if (coremask == NULL) > > > return -1; > > > @@ -358,6 +359,10 @@ eal_parse_service_coremask(const char *coremask) > > > "lcore %u unavailable\n", idx); > > > return -1; > > > } > > > + > > > + if (cfg->lcore_role[idx] == ROLE_RTE) > > > + taken_lcore_count++; > > > + > > > lcore_config[idx].core_role = ROLE_SERVICE; > > > count++; > > > } > > > @@ -374,11 +379,28 @@ eal_parse_service_coremask(const char *coremask) > > > if (count == 0) > > > return -1; > > > > > > + if (core_parsed && taken_lcore_count != count) { > > > + RTE_LOG(ERR, EAL, > > > + "Warning: not all service cores were in the coremask. " > > > + "Please ensure -c or -l includes service cores\n"); > > > > Current execution will throw warning message as 'Warning: not all service > cores were in the coremask. Please ensure -c or -l includes service cores'. > > > > 1) Should we re-write this with ' RTE_LOG(WARN, EAL,' and removing > 'Warning: ' > > 2) Warning message as "service cores not in data plane core mask ". > > 3) If we share information "Please ensure -c or -l includes service > cores\n" is not it expected to rte_panic? So should we remove this line? > > > > > + } > > > + > > > cfg->service_lcore_count = count; > > > return 0; > > > } > > > > > > static int > > > +eal_service_cores_parsed(void) > > > +{ > > > + int idx; > > > + for (idx = 0; idx < RTE_MAX_LCORE; idx++) { > > > + if (lcore_config[idx].core_role == ROLE_SERVICE) > > > + return 1; > > > + } > > > + return 0; > > > +} > > > + > > > +static int > > > eal_parse_coremask(const char *coremask) { > > > struct rte_config *cfg = rte_eal_get_configuration(); @@ -387,6 > > > +409,11 @@ eal_parse_coremask(const char *coremask) > > > char c; > > > int val; > > > > > > + if (eal_service_cores_parsed()) > > > + RTE_LOG(ERR, EAL, > > > + "Warning: Service cores parsed before dataplane cores. > > > " > > > + "Please ensure -c is before -s or -S.\n"); > > > + > > > if (coremask == NULL) > > > return -1; > > > /* Remove all blank characters ahead and after . > > > @@ -418,6 +445,7 @@ eal_parse_coremask(const char *coremask) > > > "unavailable\n", idx); > > > return -1; > > > } > > > + > > > cfg->lcore_role[idx] = ROLE_RTE; > > > lcore_config[idx].core_index = count; > > > count++; > > > @@ -449,6 +477,7 @@ eal_parse_service_corelist(const char *corelist) > > > unsigned count = 0; > > > char *end = NULL; > > > int min, max; > > > + uint32_t taken_lcore_count = 0; > > > > > > if (corelist == NULL) > > > return -1; > > > @@ -490,6 +519,9 @@ eal_parse_service_corelist(const char *corelist) > > > idx); > > > return -1; > > > } > > > + if (cfg->lcore_role[idx] == ROLE_RTE) > > > + taken_lcore_count++; > > > + > > > lcore_config[idx].core_role = > > > ROLE_SERVICE; > > > count++; > > > @@ -504,6 +536,12 @@ eal_parse_service_corelist(const char *corelist) > > > if (count == 0) > > > return -1; > > > > > > + if (core_parsed && taken_lcore_count != count) { > > > + RTE_LOG(ERR, EAL, > > > + "Warning: not all service cores were in the coremask. " > > > + "Please ensure -c or -l includes service cores\n"); > > > + } > > > + > > > return 0; > > > } > > > > > > @@ -516,6 +554,11 @@ eal_parse_corelist(const char *corelist) > > > char *end = NULL; > > > int min, max; > > > > > > + if (eal_service_cores_parsed()) > > > + RTE_LOG(ERR, EAL, > > > + "Warning: Service cores parsed before dataplane cores. > > > " > > > + "Please ensure -l is before -s or -S.\n"); > > > + > > > if (corelist == NULL) > > > return -1; > > > > > > -- > > > 2.7.4 > > > > > >
