DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: "Van Haaren, Harry" <harry.van.haaren@intel.com>
Cc: dev@dpdk.org, "Varghese, Vipin" <vipin.varghese@intel.com>
Subject: Re: [dpdk-dev] [PATCH v2] eal/service: improve error checking of coremasks
Date: Thu, 12 Jul 2018 09:35:20 +0200	[thread overview]
Message-ID: <30628394.ucWzCd8NzF@xps> (raw)
In-Reply-To: <4C9E0AB70F954A408CC4ADDBF0F8FA7D4D1F7A83@BGSMSX101.gar.corp.intel.com>

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.haaren@intel.com>; thomas@monjalon.net;
> > Varghese, Vipin <vipin.varghese@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@intel.com>
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > 
> > ---
> > 
> > v2, thanks for review:
> > - Consistency in message endings - vs . (Thomas)
> > - Wrap lines as they're very long otherwise (Thomas)
> > 
> > Cc: thomas@monjalon.net
> > Cc: vipin.varghese@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
> 

  reply	other threads:[~2018-07-12  7:35 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-15 14:52 [dpdk-dev] [PATCH] " Harry van Haaren
2018-05-15 15:38 ` Thomas Monjalon
2018-05-15 15:56 ` [dpdk-dev] [PATCH v2] " Harry van Haaren
2018-05-21  9:41   ` Varghese, Vipin
2018-07-12  7:35     ` Thomas Monjalon [this message]
2018-07-13 17:27       ` Van Haaren, Harry
2018-07-13 17:25   ` [dpdk-dev] [PATCH v3] " Harry van Haaren
2018-07-13 17:33     ` Thomas Monjalon
2018-07-26 14:18       ` Thomas Monjalon
2018-07-13 17:45     ` Varghese, Vipin
2018-07-26 16:16     ` [dpdk-dev] [PATCH v4] " Harry van Haaren
2018-07-26 16:31       ` [dpdk-dev] [PATCH v5] " Harry van Haaren
2018-07-26 16:40         ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=30628394.ucWzCd8NzF@xps \
    --to=thomas@monjalon.net \
    --cc=dev@dpdk.org \
    --cc=harry.van.haaren@intel.com \
    --cc=vipin.varghese@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).