Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1511376 > unrolled thread

Re: [PATCH 3/3] trace-cmd record: add --cpu-list option

Started bySteven Rostedt <rostedt@goodmis.org>
First post2016-10-28 22:00 +0200
Last post2016-10-28 23:20 +0200
Articles 2 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] trace-cmd record: add --cpu-list option Steven Rostedt <rostedt@goodmis.org> - 2016-10-28 22:00 +0200
    Re: [PATCH 3/3] trace-cmd record: add --cpu-list option Luiz Capitulino <lcapitulino@redhat.com> - 2016-10-28 23:20 +0200

#1511376 — Re: [PATCH 3/3] trace-cmd record: add --cpu-list option

FromSteven Rostedt <rostedt@goodmis.org>
Date2016-10-28 22:00 +0200
SubjectRe: [PATCH 3/3] trace-cmd record: add --cpu-list option
Message-ID<sxltL-49Z-9@gated-at.bofh.it>
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 */

[toc] | [next] | [standalone]

#1511425

FromLuiz Capitulino <lcapitulino@redhat.com>
Date2016-10-28 23:20 +0200
Message-ID<sxmJb-5cR-17@gated-at.bofh.it>
In reply to#1511376
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 */  
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web