From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v2 4/4] xentrace: Implement cpu mask range parsing of human values (-c). Date: Mon, 16 Jun 2014 15:36:13 +0100 Message-ID: <539F00DD.6020804@eu.citrix.com> References: <1402692084-931-1-git-send-email-konrad.wilk@oracle.com> <1402692084-931-5-git-send-email-konrad.wilk@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1402692084-931-5-git-send-email-konrad.wilk@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk , xen-devel@lists.xen.org, ian.campbell@citrix.com, ian.jackson@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 06/13/2014 09:41 PM, Konrad Rzeszutek Wilk wrote: > Instead of just using -c 0x we can > also use -c - or -c , > or a combination of them. > > That should make it easier to trace the right CPU if > using this along with 'xl vcpu-list'. > > The code has been lifted from the Linux kernel, see file > lib/bitmap.c, function __bitmap_parselist. > > To make the old behavior and the new function work, we check > to see if the arguments have '0x' in them. If they do > we use the old style parsing (limited to 32 CPUs). If that > does not exist we use the new parsing. > > Signed-off-by: Konrad Rzeszutek Wilk > --- > tools/xentrace/xentrace.8 | 21 +++++- > tools/xentrace/xentrace.c | 155 ++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 165 insertions(+), 11 deletions(-) > > diff --git a/tools/xentrace/xentrace.8 b/tools/xentrace/xentrace.8 > index c176a96..9beb72d 100644 > --- a/tools/xentrace/xentrace.8 > +++ b/tools/xentrace/xentrace.8 > @@ -36,11 +36,26 @@ all new records to the output > set the time, p, (in milliseconds) to sleep between polling the buffers > for new data. > .TP > -.B -c, --cpu-mask=c > -set bitmask of CPUs to trace. It is limited to 32-bits. > +.B -c, --cpu-mask=[\fIc\fP|\fICPU-LIST\fP] > +set bitmask of CPUs to trace. > It is limited to 32-bits if using hex digits. > If not specified, the cpu-mask of all of the available CPUs will be I think something like this would be better: "This can be either a hex value (of the form 0xNNNN...), or a set of cpu ranges as described below. Hex values are limited to 32 bits. If not specified..." > -constructed. > +constructed. If using the \fICPU-LIST\fP it expects decimal numbers, which > +may be specified as follows: > > +.RS 4 > +.ie n .IP """0-3""" 4 > +.el .IP "``0-3''" 4 > +.IX Item "0-3" > +Trace only on CPUs 0 through 3 > +.ie n .IP """0,2,5-7""" 4 > +.el .IP "``0,2,5-7''" 4 > +.IX Item "0,2,5-7" > +Trace only on CPUs 0, 2, and 5 through 7. > +.RE > +.Sp > + > +If this option is not specified, xentrace will trace all of the physical > +CPUs on the machine. This was already said above. I'm not sure whether here or at the beginning is better, but it should only be said once. > .TP > .B -e, --evt-mask=e > set event capture mask. If not specified the TRC_ALL will be used. > diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c > index 03b5537..45b0597 100644 > --- a/tools/xentrace/xentrace.c > +++ b/tools/xentrace/xentrace.c > @@ -23,6 +23,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -31,6 +32,7 @@ > > #include > #include "xc_private.h" > +#include "xc_bitops.h" /* for set_bit */ > > /***** Compile time configuration of defaults ********************************/ > > @@ -45,6 +47,7 @@ typedef struct settings_st { > unsigned long poll_sleep; /* milliseconds to sleep between polls */ > uint32_t evt_mask; > xc_cpumap_t cpu_mask; > + char *cpu_mask_str; > unsigned long tbuf_size; > unsigned long disk_rsvd; > unsigned long timeout; > @@ -809,7 +812,7 @@ static void usage(void) > "Usage: xentrace [OPTION...] [output file]\n" \ > "Tool to capture Xen trace buffer data\n" \ > "\n" \ > -" -c, --cpu-mask=c Set cpu-mask\n" \ > +" -c, --cpu-mask=c Set cpu-mask using hex and CPU ranges\n" \ "Set cpu-mask, using either hex or CPU ranges" > +static int check_for_hex(const char *s) > +{ > + int buflen; > + int c_old, c; > + > + buflen = strlen(s); > + c_old = c = 0; > + do { > + c = *s++; > + buflen--; > + > + if ( isspace(c) && c_old != ZERO_DIGIT ) > + continue; > + > + if ( c == '\0' ) > + return -1; > + > + if ( c == ZERO_DIGIT ) > + c_old = c; > + > + if ( c == 'x' && c_old == ZERO_DIGIT ) > + return 1; > + > + } while ( buflen ); I'm a bit confused why you don't just do strcmp("0x", s) here. > @@ -989,14 +1112,25 @@ static void parse_args(int argc, char **argv) > break; > > case 'c': /* set new cpu mask for filtering*/ > - /* Set opts.cpu_mask later as we don't have 'xch' set yet. */ > - if ( parse_cpumask(optarg) ) > { > - perror("Not enough memory!"); > - exit(EXIT_FAILURE); > + int ret = check_for_hex(optarg); > + > + if ( ret < 0 ) > + usage(); > + > + if ( ret ) > + ret = parse_cpumask(optarg); > + else > + /* Set opts.cpu_mask later as we don't have 'xch' set yet.*/ > + opts.cpu_mask_str = strdup(optarg); > + > + if ( ret ) > + { > + perror("Not enough memory!"); > + exit(EXIT_FAILURE); > + } > + break; While we're at it, is there any chance we could put all the parsing stuff in the same place? If we can't call parse_cpumask_range here, I'd prefer to just do a strdup() always, and then do the check_for_hex() and parse_cpumask() or parse_cpumask_range() down below. -George