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

[PATCH v2 0/2] 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 patch is a small refectoring needed to
make --cpu-list support fit nicely. The second patch
adds the new option.

v2
--

- Use the CPU_SET() API instead of cpu.h, which
  makes the cpumask dynamic and avoid various bugs
- Small cleanups and renamings

Luiz Capitulino (2):
  trace-cmd record: refactor set_mask()
  trace-cmd record: add --cpu-list option

 Documentation/trace-cmd-record.1.txt |   4 +
 trace-local.h                        |   2 +-
 trace-record.c                       | 258 ++++++++++++++++++++++++++++++++---
 3 files changed, 245 insertions(+), 19 deletions(-)

-- 
2.5.5

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

[Luiz Capitulino]

This commit makes set_mask() more specialized: all
it does now is to write the cpumask to tracing_cpumask.
The handling of "-M -1" is now done by the newly
added alloc_mask_from_hex().

Also, uneeded checks are dropped and
buffer_instance->cpumask points to dynamic memory.

This work 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 | 54 ++++++++++++++++++++++++++++++++++--------------------
 2 files changed, 35 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..0f1f2c4 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -69,6 +69,8 @@ enum trace_type {
 	TRACE_TYPE_PROFILE	= (1 << 4) | TRACE_TYPE_STREAM,
 };
 
+#define CPUMASK_STR_MAX 4096 /* Don't expect more than 32768 CPUS */
+
 static int rt_prio;
 
 static int keep;
@@ -2078,27 +2080,24 @@ 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;
 
-	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(CPUMASK_STR_MAX);
+		if (!cpumask)
+			die("can't allocate cpumask");
+
+		if (bytes > (CPUMASK_STR_MAX-1)) {
 			warning("cpumask can't handle more than 32768 CPUS!");
-			bytes = 4095;
+			bytes = CPUMASK_STR_MAX-1;
 		}
 
 		sprintf(cpumask, "%x", (1 << last) - 1);
@@ -2107,18 +2106,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 +2139,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 +4544,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 2/2] 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.

Support for --cpu-list is implemented by dynamically
allocating a cpu_set_t object and setting the parsed
CPUs. Using the CPU_SET API allows for more robost
error detection.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 Documentation/trace-cmd-record.1.txt |   4 +
 trace-record.c                       | 208 +++++++++++++++++++++++++++++++++++
 2 files changed, 212 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 0f1f2c4..49d76db 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>
@@ -2080,6 +2081,208 @@ static void update_pid_event_filters(struct buffer_instance *instance)
 	update_event_filters(instance);
 }
 
+struct cpuset {
+	int nr_cpus;
+	size_t size;
+	cpu_set_t *set;
+};
+
+static void set_cpu(int cpu, struct cpuset *set)
+{
+	if (cpu < 0 || cpu > set->nr_cpus - 1)
+		die("invalid cpu in range");
+	CPU_SET_S(cpu, set->size, set->set);
+}
+
+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 parse_single_cpu(const char *str, struct cpuset *set)
+{
+	int cpu, err;
+
+	err = read_cpu_nr(str, &cpu);
+	if (err)
+		return -1;
+
+	set_cpu(cpu, set);
+	return 0;
+}
+
+static int parse_range_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 parse_range(const char *str, int *begin, int *end)
+{
+	char *saveptr, *range;
+	int ret;
+
+	range = strdup(str);
+	if (!range)
+		return -1;
+
+	ret = parse_range_one(range, &saveptr);
+	if (ret < 0)
+		goto out;
+	*begin = ret;
+
+	ret = parse_range_one(NULL, &saveptr);
+	if (ret < 0)
+		goto out;
+	*end = ret;
+
+out:
+	free(range);
+	return ret < 0 ? -1 : 0;
+}
+
+static int parse_cpu_range(const char *str, struct cpuset *set)
+{
+	int i, ret, begin, end;
+
+	ret = parse_range(str, &begin, &end);
+	if (ret < 0 || begin > end)
+		return -1;
+
+	for (i = begin; i <= end; i++)
+		set_cpu(i, set);
+
+	return 0;
+}
+
+static int has_range(const char *str)
+{
+	return strchr(str, '-') != NULL;
+}
+
+static int parse_cpu_list(const char *cpu_list, struct cpuset *set)
+{
+	char *saveptr, *str, *p;
+	int err = 0;
+
+	str = strdup(cpu_list);
+	if (!str)
+		return -1;
+
+	p = strtok_r(str, ",", &saveptr);
+	while (p) {
+		if (has_range(p))
+			err = parse_cpu_range(p, set);
+		else
+			err = parse_single_cpu(p, set);
+		if (err)
+			goto out;
+		p = strtok_r(NULL, ",", &saveptr);
+	}
+
+out:
+	free(str);
+	return err;
+}
+
+static int val_to_char(int v)
+{
+	if (v >= 0 && v < 10)
+		return '0' + v;
+	else if (v >= 10 && v < 16)
+		return ('a' - 10) + v;
+	else
+		return -1;
+}
+
+/* From util-linux */
+static char *cpu_set_to_str(char *str, size_t len, struct cpuset *set)
+{
+	char *ptr = str;
+	char *ret = NULL;
+	int cpu;
+
+	for (cpu = (8 * set->size) - 4; cpu >= 0; cpu -= 4) {
+		char val = 0;
+
+		if (len == (size_t) (ptr - str))
+			break;
+
+		if (CPU_ISSET_S(cpu, set->size, set->set))
+			val |= 1;
+		if (CPU_ISSET_S(cpu + 1, set->size, set->set))
+			val |= 2;
+		if (CPU_ISSET_S(cpu + 2, set->size, set->set))
+			val |= 4;
+		if (CPU_ISSET_S(cpu + 3, set->size, set->set))
+			val |= 8;
+
+		if (!ret && val)
+			ret = ptr;
+		*ptr++ = val_to_char(val);
+	}
+
+	*ptr = '\0';
+	return ret ? ret : ptr - 1;
+}
+
+static char *alloc_mask_from_list(const char *cpu_list)
+{
+	struct cpuset set;
+	char *str;
+	int ret;
+
+	set.nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
+	if (set.nr_cpus < 0)
+		die("can't get number of processors\n");
+
+	set.set = CPU_ALLOC(set.nr_cpus);
+	if (!set.set)
+		die("can't allocate cpu_set\n");
+
+	set.size = CPU_ALLOC_SIZE(set.nr_cpus);
+	CPU_ZERO_S(set.size, set.set);
+
+	ret = parse_cpu_list(cpu_list, &set);
+	if (ret < 0)
+		die("invalid cpu list specified");
+
+	str = malloc(CPUMASK_STR_MAX);
+	if (!str)
+		die("can't allocate memory\n");
+
+	cpu_set_to_str(str, CPUMASK_STR_MAX, &set);
+	CPU_FREE(set.set);
+
+	return str;
+}
 
 static char *alloc_mask_from_hex(const char *str)
 {
@@ -4137,6 +4340,7 @@ enum {
 	OPT_nosplice		= 253,
 	OPT_funcstack		= 254,
 	OPT_date		= 255,
+	OPT_cpulist			= 256,
 };
 
 void trace_record (int argc, char **argv)
@@ -4338,6 +4542,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, '?'},
@@ -4546,6 +4751,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 v2 0/2] trace-cmd record: add --cpu-list option

[Luiz Capitulino]

On Tue, 22 Nov 2016 15:20:50 -0500
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 patch is a small refectoring needed to
> make --cpu-list support fit nicely. The second patch
> adds the new option.

ping?

> 
> v2
> --
> 
> - Use the CPU_SET() API instead of cpu.h, which
>   makes the cpumask dynamic and avoid various bugs
> - Small cleanups and renamings
> 
> Luiz Capitulino (2):
>   trace-cmd record: refactor set_mask()
>   trace-cmd record: add --cpu-list option
> 
>  Documentation/trace-cmd-record.1.txt |   4 +
>  trace-local.h                        |   2 +-
>  trace-record.c                       | 258 ++++++++++++++++++++++++++++++++---
>  3 files changed, 245 insertions(+), 19 deletions(-)
> 

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

[Steven Rostedt]

On Tue, 3 Jan 2017 11:32:58 -0500
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Tue, 22 Nov 2016 15:20:50 -0500
> 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 patch is a small refectoring needed to
> > make --cpu-list support fit nicely. The second patch
> > adds the new option.  
> 
> ping?
> 

Thanks for the reminder ;-)

I'm going to try to get to this this week, but as this is my first week
on my new job (not to mention the new year), you may need to ping me
again.

-- Steve

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

[Luiz Capitulino]

On Tue, 3 Jan 2017 11:45:30 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 3 Jan 2017 11:32:58 -0500
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
> > On Tue, 22 Nov 2016 15:20:50 -0500
> > 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 patch is a small refectoring needed to
> > > make --cpu-list support fit nicely. The second patch
> > > adds the new option.    
> > 
> > ping?
> >   
> 
> Thanks for the reminder ;-)
> 
> I'm going to try to get to this this week, but as this is my first week
> on my new job (not to mention the new year), you may need to ping me
> again.

I see, no rush then.

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

[Steven Rostedt]

Looking through my inbox, I stumbled on this.

On Tue, 3 Jan 2017 11:54:06 -0500
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Tue, 3 Jan 2017 11:45:30 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Tue, 3 Jan 2017 11:32:58 -0500
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> >   
> > > On Tue, 22 Nov 2016 15:20:50 -0500
> > > 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 patch is a small refectoring needed to
> > > > make --cpu-list support fit nicely. The second patch
> > > > adds the new option.      
> > > 
> > > ping?
> > >     
> > 
> > Thanks for the reminder ;-)
> > 
> > I'm going to try to get to this this week, but as this is my first week
> > on my new job (not to mention the new year), you may need to ping me
> > again.  
> 
> I see, no rush then.

I hope you really meant that, as I really did need another ping.

I think this is a good option to have. I've Cc'd linux-trace-devel, and
hopefully we'll get this feature added much sooner than the next 3
years!

-- Steve

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

[Luiz Capitulino]

On 2020-03-03 10:07 p.m., Steven Rostedt wrote:
> Looking through my inbox, I stumbled on this.
> 
> On Tue, 3 Jan 2017 11:54:06 -0500
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
>> On Tue, 3 Jan 2017 11:45:30 -0500
>> Steven Rostedt <rostedt@goodmis.org> wrote:
>>
>>> On Tue, 3 Jan 2017 11:32:58 -0500
>>> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>>>    
>>>> On Tue, 22 Nov 2016 15:20:50 -0500
>>>> 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 patch is a small refectoring needed to
>>>>> make --cpu-list support fit nicely. The second patch
>>>>> adds the new option.
>>>>
>>>> ping?
>>>>      
>>>
>>> Thanks for the reminder ;-)
>>>
>>> I'm going to try to get to this this week, but as this is my first week
>>> on my new job (not to mention the new year), you may need to ping me
>>> again.
>>
>> I see, no rush then.
> 
> I hope you really meant that, as I really did need another ping.
> 
> I think this is a good option to have. I've Cc'd linux-trace-devel, and
> hopefully we'll get this feature added much sooner than the next 3
> years!

That's great to hear! :)

- Luiz