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

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

[Luiz Capitulino]

This series adds support for a --cpu-list option, which is
much more human friendly than -M:

  # trace-cmd record --cpu-list 1,4,10-15 [...]

The first two patches are refactorings needed to make
--cpu-list support fit nicely. The third patch adds the
new option.

Luiz Capitulino (3):
  cpu.h: use standard types instead of glib's
  trace-cmd record: refactor set_mask()
  trace-cmd record: add --cpu-list option

 Documentation/trace-cmd-record.1.txt |   4 +
 cpu.h                                |  47 +++++----
 trace-local.h                        |   2 +-
 trace-record.c                       | 191 +++++++++++++++++++++++++++++++----
 4 files changed, 201 insertions(+), 43 deletions(-)

-- 
2.5.5

[PATCH 1/3] cpu.h: use standard types instead of glib's

[Luiz Capitulino]

Future commits will introduce standard C code that
wants to use cpu.h. Let's convert cpu.h to standard
types.

I'm choosing not to change the gui code that uses
cpu.h though. I think that in all platforms where
glib is supported, guint64 should always be uint64_t
and guint should always be unsigned int.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 cpu.h | 47 +++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/cpu.h b/cpu.h
index 05c7f17..fc0b3c4 100644
--- a/cpu.h
+++ b/cpu.h
@@ -20,45 +20,48 @@
 #ifndef _CPU_H
 #define _CPU_H
 
-static inline int cpu_isset(guint64 *cpu_mask, gint cpu)
+#include <stdint.h>
+#include <stdbool.h>
+
+static inline int cpu_isset(uint64_t *cpu_mask, int cpu)
 {
-	guint64 mask;
+	uint64_t mask;
 
 	mask = *(cpu_mask + (cpu >> 6));
 
 	return mask & (1ULL << (cpu & ((1ULL << 6) - 1)));
 }
 
-static inline gboolean cpu_allset(guint64 *cpu_mask, gint max_cpus)
+static inline bool cpu_allset(uint64_t *cpu_mask, int max_cpus)
 {
-	gint cpu;
+	int cpu;
 
 	for (cpu = 0; cpu < max_cpus; cpu++) {
 		if (!cpu_isset(cpu_mask, cpu))
-			return FALSE;
+			return false;
 	}
-	return TRUE;
+	return true;
 }
 
-static inline void cpu_set(guint64 *cpu_mask, gint cpu)
+static inline void cpu_set(uint64_t *cpu_mask, int cpu)
 {
-	guint64 *mask;
+	uint64_t *mask;
 
 	mask = cpu_mask + (cpu >> 6);
 	*mask |= (1ULL << (cpu & ((1ULL << 6) - 1)));
 }
 
-static inline void cpu_clear(guint64 *cpu_mask, gint cpu)
+static inline void cpu_clear(uint64_t *cpu_mask, int cpu)
 {
-	guint64 *mask;
+	uint64_t *mask;
 
 	mask = cpu_mask + (cpu >> 6);
 	*mask &= ~(1ULL << (cpu & ((1ULL << 6) - 1)));
 }
 
-static inline void set_cpus(guint64 *cpu_mask, gint cpus)
+static inline void set_cpus(uint64_t *cpu_mask, int cpus)
 {
-	gint idx;
+	int idx;
 
 	for (idx = 0; idx < (cpus >> 6); idx++) {
 		*(cpu_mask + idx) = -1ULL;
@@ -67,22 +70,22 @@ static inline void set_cpus(guint64 *cpu_mask, gint cpus)
 	*(cpu_mask) = (1ULL << (cpus & ((1ULL << 6) - 1))) - 1;
 }
 
-static inline gboolean cpus_equal(guint64 *a_mask, guint64 *b_mask,
-				  gint cpus)
+static inline bool cpus_equal(uint64_t *a_mask, uint64_t *b_mask,
+				  int cpus)
 {
-	gint idx;
+	int idx;
 
 	for (idx = 0; idx < (cpus >> 6) + 1; idx++) {
 		if (*(a_mask + idx) != *(b_mask + idx))
-			return FALSE;
+			return false;
 	}
-	return TRUE;
+	return true;
 }
 
 /* Hamming weight */
-static inline guint hweight(guint mask)
+static inline unsigned int hweight(unsigned int mask)
 {
-	guint64 w = mask;
+	uint64_t w = mask;
 
 	w -= (w >> 1) & 0x5555555555555555ul;
 	w =  (w & 0x3333333333333333ul) + ((w >> 2) & 0x3333333333333333ul);
@@ -90,10 +93,10 @@ static inline guint hweight(guint mask)
 	return (w * 0x0101010101010101ul) >> 56;
 }
 
-static inline guint cpu_weight(guint64 *cpu_mask, guint cpus)
+static inline unsigned int cpu_weight(uint64_t *cpu_mask, unsigned int cpus)
 {
-	guint weight = 0;
-	gint idx;
+	unsigned int weight = 0;
+	int idx;
 
 	for (idx = 0; idx < (cpus >> 6) + 1; idx++)
 		weight += hweight(*(cpu_mask + idx));
-- 
2.5.5

[PATCH 2/3] trace-cmd record: refactor set_mask()

[Luiz Capitulino]

This commit does the following:

 1. Move the code that treats "-M -1" out of set_mask()
 2. Drop uneeded checks from set_mask()
 3. Make instance->cpumask point to dynamic memory

This is a preparation for supporting the "--cpu-list"
option in the next commit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 trace-local.h  |  2 +-
 trace-record.c | 53 +++++++++++++++++++++++++++++++++--------------------
 2 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/trace-local.h b/trace-local.h
index 62363d0..0ac39bf 100644
--- a/trace-local.h
+++ b/trace-local.h
@@ -143,7 +143,7 @@ struct func_list {
 struct buffer_instance {
 	struct buffer_instance	*next;
 	const char		*name;
-	const char		*cpumask;
+	char			*cpumask;
 	struct event_list	*events;
 	struct event_list	**event_next;
 
diff --git a/trace-record.c b/trace-record.c
index 22b6835..b042993 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -2078,27 +2078,25 @@ static void update_pid_event_filters(struct buffer_instance *instance)
 	update_event_filters(instance);
 }
 
-static void set_mask(struct buffer_instance *instance)
-{
-	const char *mask = instance->cpumask;
-	struct stat st;
-	char cpumask[4096]; /* Don't expect more than 32768 CPUS */
-	char *path;
-	int fd;
-	int ret;
+#define MASK_STR_MAX 4096 /* Don't expect more than 32768 CPUS */
 
-	if (!mask)
-		return;
+static char *alloc_mask_from_hex(const char *str)
+{
+	char *cpumask;
 
-	if (strcmp(mask, "-1") == 0) {
+	if (strcmp(str, "-1") == 0) {
 		/* set all CPUs */
 		int bytes = (cpu_count + 7) / 8;
 		int last = cpu_count % 8;
 		int i;
 
-		if (bytes > 4095) {
+		cpumask = malloc(MASK_STR_MAX);
+		if (!cpumask)
+			die("can't allocate cpumask");
+
+		if (bytes > (MASK_STR_MAX-1)) {
 			warning("cpumask can't handle more than 32768 CPUS!");
-			bytes = 4095;
+			bytes = MASK_STR_MAX-1;
 		}
 
 		sprintf(cpumask, "%x", (1 << last) - 1);
@@ -2107,18 +2105,32 @@ static void set_mask(struct buffer_instance *instance)
 			cpumask[i] = 'f';
 
 		cpumask[i+1] = 0;
-
-		mask = cpumask;
+	} else {
+		cpumask = strdup(str);
+		if (!cpumask)
+			die("can't allocate cpumask");
 	}
 
+	return cpumask;
+}
+
+static void set_mask(struct buffer_instance *instance)
+{
+	struct stat st;
+	char *path;
+	int fd;
+	int ret;
+
+	if (!instance->cpumask)
+		return;
+
 	path = get_instance_file(instance, "tracing_cpumask");
 	if (!path)
 		die("could not allocate path");
 
 	ret = stat(path, &st);
 	if (ret < 0) {
-		if (mask)
-			warning("%s not found", path);
+		warning("%s not found", path);
 		goto out;
 	}
 
@@ -2126,12 +2138,13 @@ static void set_mask(struct buffer_instance *instance)
 	if (fd < 0)
 		die("could not open %s\n", path);
 
-	if (mask)
-		write(fd, mask, strlen(mask));
+	write(fd, instance->cpumask, strlen(instance->cpumask));
 	
 	close(fd);
  out:
 	tracecmd_put_tracing_file(path);
+	free(instance->cpumask);
+	instance->cpumask = NULL;
 }
 
 static void enable_events(struct buffer_instance *instance)
@@ -4530,7 +4543,7 @@ void trace_record (int argc, char **argv)
 			max_kb = atoi(optarg);
 			break;
 		case 'M':
-			instance->cpumask = optarg;
+			instance->cpumask = alloc_mask_from_hex(optarg);
 			break;
 		case 't':
 			if (extract)
-- 
2.5.5

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

[Luiz Capitulino]

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)
+{
+	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);
+
+	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)
+{
+	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);
+		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;
+	int ret;
+
+	ret = parse_cpumask_str(cpu_list, &cpumask);
+	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 */
-- 
2.5.5

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

[Steven Rostedt]

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

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

[Steven Rostedt]

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 ;-)
> 

I did apply patches 1 and 2. I'm hoping to get them out tomorrow, or
while I'm at plumbers.

-- Steve

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

[Luiz Capitulino]

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

> 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 ;-)
> >   
> 
> I did apply patches 1 and 2. I'm hoping to get them out tomorrow, or
> while I'm at plumbers.

OK, I also posted a minor fix:

 [PATCH] trace-cmd: Documentation: ignore all man sections

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

[Steven Rostedt]

On Fri, 28 Oct 2016 17:15:57 -0400
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Fri, 28 Oct 2016 15:50:10 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > 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 ;-)
> > >   
> > 
> > I did apply patches 1 and 2. I'm hoping to get them out tomorrow, or
> > while I'm at plumbers.
> 
> OK, I also posted a minor fix:
> 
>  [PATCH] trace-cmd: Documentation: ignore all man sections

Yep, I included that one already.

Thanks!

-- Steve

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

[Luiz Capitulino]

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

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

[Steven Rostedt]

On Fri, 28 Oct 2016 17:14:56 -0400
Luiz Capitulino <lcapitulino@redhat.com> wrote:


> > 
> > 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?
> 

I thought I replied to this, but I'm guessing the reply is still
pending.

I have no objecting. I'd like to see a patch though.

Thanks,

-- Steve

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

[Luiz Capitulino]

On Tue, 15 Nov 2016 16:38:19 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 28 Oct 2016 17:14:56 -0400
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
> 
> > > 
> > > 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?
> >   
> 
> I thought I replied to this, but I'm guessing the reply is still
> pending.
> 
> I have no objecting. I'd like to see a patch though.

I imagined you didn't have objections :) I haven't started working
on this yet because I'm very busy at other stuff. But I'll go
back to this soon.

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

[Luiz Capitulino]

On Fri,  7 Oct 2016 12:47:08 -0400
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> This series adds support for a --cpu-list option, which is
> much more human friendly than -M:
> 
>   # trace-cmd record --cpu-list 1,4,10-15 [...]
> 
> The first two patches are refactorings needed to make
> --cpu-list support fit nicely. The third patch adds the
> new option.

ping?

> 
> Luiz Capitulino (3):
>   cpu.h: use standard types instead of glib's
>   trace-cmd record: refactor set_mask()
>   trace-cmd record: add --cpu-list option
> 
>  Documentation/trace-cmd-record.1.txt |   4 +
>  cpu.h                                |  47 +++++----
>  trace-local.h                        |   2 +-
>  trace-record.c                       | 191 +++++++++++++++++++++++++++++++----
>  4 files changed, 201 insertions(+), 43 deletions(-)
> 

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

[Steven Rostedt]

On Wed, 19 Oct 2016 09:30:23 -0400
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Fri,  7 Oct 2016 12:47:08 -0400
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
> > This series adds support for a --cpu-list option, which is
> > much more human friendly than -M:
> > 
> >   # trace-cmd record --cpu-list 1,4,10-15 [...]
> > 
> > The first two patches are refactorings needed to make
> > --cpu-list support fit nicely. The third patch adds the
> > new option.  
> 
> ping?

It's in the todo list, just need to work on some other things first.
Thanks for the reminder though.

-- Steve

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

[Luiz Capitulino]

On Wed, 19 Oct 2016 09:55:23 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 19 Oct 2016 09:30:23 -0400
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
> > On Fri,  7 Oct 2016 12:47:08 -0400
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> >   
> > > This series adds support for a --cpu-list option, which is
> > > much more human friendly than -M:
> > > 
> > >   # trace-cmd record --cpu-list 1,4,10-15 [...]
> > > 
> > > The first two patches are refactorings needed to make
> > > --cpu-list support fit nicely. The third patch adds the
> > > new option.    
> > 
> > ping?  
> 
> It's in the todo list, just need to work on some other things first.
> Thanks for the reminder though.

Ah, OK. I was afraid it got lost.