[dpdk-dev] [PATCH v2] eal/service: improve error checking of coremasks
Van Haaren, Harry
harry.van.haaren at intel.com
Fri Jul 13 19:27:03 CEST 2018
Hi Thomas,
I've sent a v3 for this patch. Twice actually, including dev at dpdk.org the 2nd time :)
Thanks for the ping, -Harry
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas at monjalon.net]
> Sent: Thursday, July 12, 2018 8:35 AM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>
> Cc: dev at dpdk.org; Varghese, Vipin <vipin.varghese at 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 at dpdk.org
> > > Cc: Van Haaren, Harry <harry.van.haaren at intel.com>; thomas at monjalon.net;
> > > Varghese, Vipin <vipin.varghese at 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.varghese at intel.com>
> > > Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
> > >
> > > ---
> > >
> > > v2, thanks for review:
> > > - Consistency in message endings - vs . (Thomas)
> > > - Wrap lines as they're very long otherwise (Thomas)
> > >
> > > Cc: thomas at monjalon.net
> > > Cc: vipin.varghese at 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
> >
>
>
>
>
More information about the dev
mailing list