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

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.

> +
> +	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 */