[PATCH 0/2] trace-cmd library: Fix sparse cpu_data

[PATCH 0/2] trace-cmd library: Fix sparse cpu_data

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

I have a new box that has 128 logical CPUs, and when I do trace-cmd record, as
most of my CPUs are idle, they are not recorded into the CPU file.

I had a test run that only had 35 of the 128 CPUs have data, and trace-cmd
created a 35 cpu_data array to hold this information. But when I tried to load
this into KernelShark, it crashed.

It crashed because it uses the CPU number to retrieve information. Thus, when
it asked for CPU 45 and the internal trace-cmd library only had 35 cpu_data
items, it overlooked the array.

Make the array non sparse. That is, if there's only one CPU in the file, and
it's CPU 100, make an array of 101 (including CPU 0) where all the CPUs with
no data just have a cpu_data filled with zeros.

Also, fix tracecmd_read_cpu_first() return NULL if the CPU passed to it is
greater than the number of items in the cpu_data array. This is because it
would ask for cpu 64 but the last CPU with data in it was CPU 63, and
trace-cmd library only made an array of 64 (0-63) items.

Steven Rostedt (Google) (2):
  trace-cmd library: Make cpu_data[] match the cpus
  trace-cmd library: Do not read CPU greater than CPUs registered

 lib/trace-cmd/trace-input.c | 54 ++++++++++++++++++++++++++++++-------
 1 file changed, 45 insertions(+), 9 deletions(-)

-- 
2.35.1

[PATCH 1/2] trace-cmd library: Make cpu_data[] match the cpus

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

There's logic that expects cpu_data[cpu] to match the cpu buffer for the
given cpu. But now that the cpu_data array only holds the CPUs that exist
in the trace.dat file, it breaks this logic. Create the array for all cpus
regardless if a cpu exists or not. Just have the data structure be zero.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 lib/trace-cmd/trace-input.c | 46 ++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 8 deletions(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 6e59a12f7807..da922834de1f 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -3196,7 +3196,9 @@ static int handle_buffer_option(struct tracecmd_input *handle,
 				unsigned short id, char *data, int size)
 {
 	struct input_buffer_instance *buff;
+	struct cpu_file_data *cpu_data;
 	unsigned long long tmp;
+	long long max_cpu = -1;
 	int rsize = 0;
 	char *name;
 	int i;
@@ -3247,24 +3249,52 @@ static int handle_buffer_option(struct tracecmd_input *handle,
 		buff->cpus = tmp;
 		if (!buff->cpus)
 			return 0;
-		buff->cpu_data = calloc(buff->cpus, sizeof(struct cpu_file_data));
-		if (!buff->cpu_data)
+		cpu_data = calloc(buff->cpus, sizeof(*cpu_data));
+		if (!cpu_data)
 			return -1;
 		for (i = 0; i < buff->cpus; i++) {
 			if (save_read_number(handle->pevent, data, &size, &rsize, 4, &tmp))
-				return -1;
-			buff->cpu_data[i].cpu = tmp;
+				goto fail;
+			if ((long long)tmp > max_cpu)
+				max_cpu = tmp;
+			cpu_data[i].cpu = tmp;
 			if (save_read_number(handle->pevent, data,
-					     &size, &rsize, 8, &buff->cpu_data[i].offset))
-				return -1;
+					     &size, &rsize, 8, &cpu_data[i].offset))
+				goto fail;
 			if (save_read_number(handle->pevent, data,
-					     &size, &rsize, 8, &buff->cpu_data[i].size))
-				return -1;
+					     &size, &rsize, 8, &cpu_data[i].size))
+				goto fail;
+		}
+		if (buff->cpus == max_cpu + 1) {
+			/* Check to make sure cpus match the index */
+			for (i = 0; i < buff->cpus; i++) {
+				if (cpu_data[i].cpu != i)
+					goto copy_buffer;
+			}
+			buff->cpu_data = cpu_data;
+		} else {
+ copy_buffer:
+			buff->cpu_data = calloc(max_cpu + 1, sizeof(*cpu_data));
+			if (!buff->cpu_data)
+				goto fail;
+			for (i = 0; i < buff->cpus; i++) {
+				if (buff->cpu_data[cpu_data[i].cpu].size) {
+					tracecmd_warning("More than one buffer defined for CPU %d (buffer %d)\n",
+							 cpu_data[i].cpu, i);
+					goto fail;
+				}
+				buff->cpu_data[cpu_data[i].cpu] = cpu_data[i];
+			}
+			buff->cpus = max_cpu + 1;
+			free(cpu_data);
 		}
 	} else {
 		buff->latency = true;
 	}
 	return 0;
+fail:
+	free(cpu_data);
+	return -1;
 }
 
 static int handle_options(struct tracecmd_input *handle)
-- 
2.35.1

[PATCH 2/2] trace-cmd library: Do not read CPU greater than CPUs registered

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

If tracecmd_read_cpu_first() is called with a CPU that is greater than the
number of CPUs in the handle, just return NULL.

This happened when kernelshark would get the number of CPUs returned by
the tep handler, but they are not stored in the trace.dat file.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 lib/trace-cmd/trace-input.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index da922834de1f..313534d09e86 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -2025,9 +2025,15 @@ int tracecmd_refresh_record(struct tracecmd_input *handle,
 struct tep_record *
 tracecmd_read_cpu_first(struct tracecmd_input *handle, int cpu)
 {
+	unsigned long long page_offset;
 	int ret;
 
-	ret = get_page(handle, cpu, handle->cpu_data[cpu].file_offset);
+	if (cpu > handle->cpus)
+		return NULL;
+
+	page_offset = calc_page_offset(handle, handle->cpu_data[cpu].file_offset);
+
+	ret = get_page(handle, cpu, page_offset);
 	if (ret < 0)
 		return NULL;
 
-- 
2.35.1

Re: [PATCH 0/2] trace-cmd library: Fix sparse cpu_data

[Yordan Karadzhov]

Hi Steven,
Is this fixing the issue with kernelshark crashing when trying to open the new format?
thanks,
Yordan


On 13.03.22 г. 1:40 ч., Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> I have a new box that has 128 logical CPUs, and when I do trace-cmd record, as
> most of my CPUs are idle, they are not recorded into the CPU file.
> 
> I had a test run that only had 35 of the 128 CPUs have data, and trace-cmd
> created a 35 cpu_data array to hold this information. But when I tried to load
> this into KernelShark, it crashed.
> 
> It crashed because it uses the CPU number to retrieve information. Thus, when
> it asked for CPU 45 and the internal trace-cmd library only had 35 cpu_data
> items, it overlooked the array.
> 
> Make the array non sparse. That is, if there's only one CPU in the file, and
> it's CPU 100, make an array of 101 (including CPU 0) where all the CPUs with
> no data just have a cpu_data filled with zeros.
> 
> Also, fix tracecmd_read_cpu_first() return NULL if the CPU passed to it is
> greater than the number of items in the cpu_data array. This is because it
> would ask for cpu 64 but the last CPU with data in it was CPU 63, and
> trace-cmd library only made an array of 64 (0-63) items.
> 
> Steven Rostedt (Google) (2):
>    trace-cmd library: Make cpu_data[] match the cpus
>    trace-cmd library: Do not read CPU greater than CPUs registered
> 
>   lib/trace-cmd/trace-input.c | 54 ++++++++++++++++++++++++++++++-------
>   1 file changed, 45 insertions(+), 9 deletions(-)
> 

Re: [PATCH 0/2] trace-cmd library: Fix sparse cpu_data

[Steven Rostedt]

On Mon, 14 Mar 2022 16:59:13 +0200
Yordan Karadzhov <y.karadz@gmail.com> wrote:

> Is this fixing the issue with kernelshark crashing when trying to open the new format?

Yes, it was.

-- Steve

Re: [PATCH 0/2] trace-cmd library: Fix sparse cpu_data

[Yordan Karadzhov]

On 14.03.22 г. 18:01 ч., Steven Rostedt wrote:
> On Mon, 14 Mar 2022 16:59:13 +0200
> Yordan Karadzhov <y.karadz@gmail.com> wrote:
> 
>> Is this fixing the issue with kernelshark crashing when trying to open the new format?
> 
> Yes, it was.

And are all this CPUs (active or idle) being properly displayed by KS?
Y.

> 
> -- Steve

Re: [PATCH 0/2] trace-cmd library: Fix sparse cpu_data

[Steven Rostedt]

On Mon, 14 Mar 2022 18:05:44 +0200
Yordan Karadzhov <y.karadz@gmail.com> wrote:

> > On Mon, 14 Mar 2022 16:59:13 +0200
> > Yordan Karadzhov <y.karadz@gmail.com> wrote:
> >   
> >> Is this fixing the issue with kernelshark crashing when trying to open the new format?  
> > 
> > Yes, it was.  
> 
> And are all this CPUs (active or idle) being properly displayed by KS?

After the fix it seems to be.

You can check it out yourself:

  http://rostedt.org/private/trace-data/gentoo/trace.dat


-- Steve