Date: Fri, 28 Oct 2016 17:14:56 -0400
From: Luiz Capitulino <lcapitulino@redhat.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] trace-cmd record: add --cpu-list option
Message-ID: <20161028171456.3643d626@redhat.com>
In-Reply-To: <20161028154922.64e97208@gandalf.local.home>
References: <1475858831-32687-1-git-send-email-lcapitulino@redhat.com>
        <1475858831-32687-4-git-send-email-lcapitulino@redhat.com>
        <20161028154922.64e97208@gandalf.local.home>
Organization: Red Hat
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6686
Lines: 259

On Fri, 28 Oct 2016 15:49:22 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Sorry it took so long to look at this, but I finally got around to
> it ;-)
> 
> 
> On Fri,  7 Oct 2016 12:47:11 -0400
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
> > With --cpu-list you can do:
> > 
> >   # trace-cmd record --cpu-list 1,4,10-15 [...]
> > 
> > Which is much more human friendly than -M.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  Documentation/trace-cmd-record.1.txt |   4 +
> >  trace-record.c                       | 138 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 142 insertions(+)
> > 
> > diff --git a/Documentation/trace-cmd-record.1.txt b/Documentation/trace-cmd-record.1.txt
> > index b80520e..d7e806a 100644
> > --- a/Documentation/trace-cmd-record.1.txt
> > +++ b/Documentation/trace-cmd-record.1.txt
> > @@ -304,6 +304,10 @@ OPTIONS
> >      executed will not be changed. This is useful if you want to monitor the
> >      output of the command being executed, but not see the output from trace-cmd.
> >  
> > +*--cpu-list list*::
> > +    List of CPUs to be traced. The "list" argument can be comma separated
> > +    (eg. 1,2,4,5), a range (eg. 1-10) or a mix of the two (eg. 1,2,10-15).
> > +
> >  EXAMPLES
> >  --------
> >  
> > diff --git a/trace-record.c b/trace-record.c
> > index b042993..4452979 100644
> > --- a/trace-record.c
> > +++ b/trace-record.c
> > @@ -23,6 +23,7 @@
> >  #include <string.h>
> >  #include <stdarg.h>
> >  #include <getopt.h>
> > +#include <limits.h>
> >  #include <sys/types.h>
> >  #include <sys/stat.h>
> >  #include <sys/time.h>
> > @@ -47,6 +48,7 @@
> >  
> >  #include "trace-local.h"
> >  #include "trace-msg.h"
> > +#include "cpu.h"
> >  
> >  #define _STR(x) #x
> >  #define STR(x) _STR(x)
> > @@ -179,6 +181,118 @@ static struct tracecmd_recorder *recorder;
> >  
> >  static int ignore_event_not_found = 0;
> >  
> > +static int read_cpu_nr(const char *str, int *ret)
> > +{
> > +	char *endptr;
> > +	int val;
> > +
> > +	errno = 0;
> > +	val = strtol(str, &endptr, 10);
> > +
> > +	if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN))
> > +			|| (errno != 0 && val == 0))
> > +				return -1;
> > +
> > +	if (endptr == str)
> > +		return -1;
> > +
> > +	if (*endptr != '\0')
> > +		return -1;
> > +
> > +	*ret = val;
> > +	return 0;
> > +}
> > +
> > +static int set_single_cpu(const char *str, uint64_t *ret_mask)
> > +{
> > +	int cpu, err;
> > +
> > +	err = read_cpu_nr(str, &cpu);
> > +	if (err || cpu < 0)
> > +		return -1;
> > +
> > +	cpu_set(ret_mask, cpu);
> > +	return 0;
> > +}
> > +
> > +static int parse_one(char *str, char **saveptr)
> > +{
> > +	int err, cpu;
> > +	char *p;
> > +
> > +	p = strtok_r(str, "-", saveptr);
> > +	if (!p)
> > +		return -1;
> > +
> > +	err = read_cpu_nr(p, &cpu);
> > +	if (err)
> > +		return -1;
> > +
> > +	return cpu;
> > +}
> > +
> > +static int set_range_cpu(const char *str, uint64_t *ret_mask)  
> 
> ret_mask is a reference to uint64_t cpumask below.
> 
> > +{
> > +	int i, begin, end, err = -1;
> > +	char *saveptr, *range;
> > +
> > +	range = strdup(str);
> > +	if (!range)
> > +		return -1;
> > +
> > +	begin = parse_one(range, &saveptr);
> > +	if (begin < 0)
> > +		goto out;
> > +
> > +	end = parse_one(NULL, &saveptr);
> > +	if (begin < 0)
> > +		goto out;
> > +
> > +	if (begin > end)
> > +		goto out;
> > +
> > +	for (i = begin; i <= end; i++)
> > +		cpu_set(ret_mask, i);  
> 
> cpu_set(ret_mask, i) sets bits from begin to end in uint64_t cpumask
> from below.
> 
> What happens if this range is greater than 64? We already have boxes
> that run this with 80 CPUs. Looks to be out of memory range to me.

The best solution is probably to detect the number of CPUs at run-time
and use the CPU_SET() API. The lazy and ugly solution is to just fail.

Any objections to the CPU_SET() solution?

> 
> > +
> > +	err = 0;
> > +
> > +out:
> > +	free(range);
> > +	return err;
> > +}
> > +
> > +static int has_range(const char *str)
> > +{
> > +	return strchr(str, '-') != NULL;
> > +}
> > +
> > +static int parse_cpumask_str(const char *cpumask_str, uint64_t *ret_mask)  
> 
> ret_mask is a reference to uint64_t cpumask below.
> 
> > +{
> > +	char *saveptr, *str, *p;
> > +	int err = 0;
> > +
> > +	str = strdup(cpumask_str);
> > +	if (!str)
> > +		return -1;
> > +
> > +	*ret_mask = 0;
> > +
> > +	p = strtok_r(str, ",", &saveptr);
> > +	while (p) {
> > +		if (has_range(p))
> > +			err = set_range_cpu(p, ret_mask);  
> 
> Passing the reference of uint64_t cpumask from below to set_range_cpu()
> 
> > +		else
> > +			err = set_single_cpu(p, ret_mask);
> > +		if (err)
> > +			goto out;
> > +		p = strtok_r(NULL, ",", &saveptr);
> > +	}
> > +
> > +out:
> > +	free(str);
> > +	return err;
> > +}
> > +
> >  static inline int is_top_instance(struct buffer_instance *instance)
> >  {
> >  	return instance == &top_instance;
> > @@ -2114,6 +2228,25 @@ static char *alloc_mask_from_hex(const char *str)
> >  	return cpumask;
> >  }
> >  
> > +static char *alloc_mask_from_list(const char *cpu_list)
> > +{
> > +	char *cpumask_str;
> > +	uint64_t cpumask;  
> 
> cpumask is a simple uint64_t (64 bits)
> 
> > +	int ret;
> > +
> > +	ret = parse_cpumask_str(cpu_list, &cpumask);  
> 
> Passed to parse_cpumask_str() by reference.
> 
> -- Steve
> 
> > +	if (ret < 0)
> > +		die("can't parse cpumask");
> > +
> > +	cpumask_str = malloc(MASK_STR_MAX);
> > +	if (!cpumask_str)
> > +		die("can't allocate cpumask");
> > +
> > +	snprintf(cpumask_str, MASK_STR_MAX-1, "%lx", cpumask);
> > +	return cpumask_str;
> > +}
> > +
> > +
> >  static void set_mask(struct buffer_instance *instance)
> >  {
> >  	struct stat st;
> > @@ -4136,6 +4269,7 @@ enum {
> >  	OPT_nosplice		= 253,
> >  	OPT_funcstack		= 254,
> >  	OPT_date		= 255,
> > +	OPT_cpulist			= 256,
> >  };
> >  
> >  void trace_record (int argc, char **argv)
> > @@ -4337,6 +4471,7 @@ void trace_record (int argc, char **argv)
> >  			{"stderr", no_argument, NULL, OPT_stderr},
> >  			{"by-comm", no_argument, NULL, OPT_bycomm},
> >  			{"ts-offset", required_argument, NULL, OPT_tsoffset},
> > +			{"cpu-list", required_argument, NULL, OPT_cpulist},
> >  			{"max-graph-depth", required_argument, NULL, OPT_max_graph_depth},
> >  			{"debug", no_argument, NULL, OPT_debug},
> >  			{"help", no_argument, NULL, '?'},
> > @@ -4545,6 +4680,9 @@ void trace_record (int argc, char **argv)
> >  		case 'M':
> >  			instance->cpumask = alloc_mask_from_hex(optarg);
> >  			break;
> > +		case OPT_cpulist:
> > +			instance->cpumask = alloc_mask_from_list(optarg);
> > +			break;
> >  		case 't':
> >  			if (extract)
> >  				topt = 1; /* Extract top instance also */  
>