[PATCH 0/3] libtracecmd: Fix iterators

[PATCH 0/3] libtracecmd: Fix iterators

[Steven Rostedt]

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

The iterators had three issues.

1) It used handle->max_cpu instead of handle->cpus

  max_cpu is the max CPU number that was traced. But if a CPU had no data,
  it was not included in the handle->cpu_data[] array, which is the
  size of handle->cpus. The loop over the cpu_data[] array in the iterators
  stopped at max_cpu and not cpus meaning it could overflow if one of the
  CPUs had no tracing data.

2) There's no reason to free the records at the end of the iterator.
   Not sure why that was done, but it was likely due to another bug that
   was fixed. That freeing just fixed a symptom and not the issue.
   Remove the freeing.

3) The next record for each of the CPUs was cached in a records[] array,
   and when the next record to use was found, it would use that record
   from the records[] array. The issue is if the callback called
   tracecmd_read_data() on any of those records, it would invalidate the
   one returned by "peek". Since the iterator does not know what the
   callback might have done, it would have to refresh all the cached
   records after each call to the callback() function.

   Instead, just save the timestamps of the records for each CPU, and pick
   the CPU with the next timestamp. Do the tracecmd_peek_data() on that
   CPU and make sure it still matches the given timestamp. If it does not,
   then try again.

Steven Rostedt (Google) (3):
  libtracecmd: Use cpu_data[cpu]->cpus and not ->max_cpu
  libtracecmd: Do not free records at end of iterator
  libtracecmd: Just save timestamps and not the records in iterators

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

-- 
2.43.0

[PATCH 1/3] libtracecmd: Use cpu_data[cpu]->cpus and not ->max_cpu

[Steven Rostedt]

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

The handle->cpu_data[cpu]->max_cpu is the largest number CPU that is
recorded in the trace, where as cpu_data[cpu]->cpus is the number of
handle->cpu_data[] entries.

The iterator loops mistakenly used the max_cpu field instead of the cpus
field and this can cause errors if the two are not the same. This is the
case if one of the CPUs contained no data, it will not have a cpu_data[]
entry and will be skipped.

Fixes: 2cb6cc2f4 ("tracecmd library: Add tracecmd_iterate_events()")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 lib/trace-cmd/trace-input.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 6297e34bde38..0dc3dbdc676a 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -2819,11 +2819,11 @@ int tracecmd_iterate_events(struct tracecmd_input *handle,
 		return -1;
 	}
 
-	records = calloc(handle->max_cpu, sizeof(*records));
+	records = calloc(handle->cpus, sizeof(*records));
 	if (!records)
 		return -1;
 
-	for (cpu = 0; cpu < handle->max_cpu; cpu++) {
+	for (cpu = 0; cpu < handle->cpus; cpu++) {
 		if (cpus && !CPU_ISSET_S(cpu, cpu_size, cpus))
 			continue;
 
@@ -2832,7 +2832,7 @@ int tracecmd_iterate_events(struct tracecmd_input *handle,
 
 	do {
 		next_cpu = -1;
-		for (cpu = 0; cpu < handle->max_cpu; cpu++) {
+		for (cpu = 0; cpu < handle->cpus; cpu++) {
 			record = records[cpu];
 			if (!record)
 				continue;
@@ -2855,7 +2855,7 @@ int tracecmd_iterate_events(struct tracecmd_input *handle,
 	} while (next_cpu >= 0 && ret == 0);
 
 	/* Need to unlock and free the records */
-	for (cpu = 0; cpu < handle->max_cpu; cpu++) {
+	for (cpu = 0; cpu < handle->cpus; cpu++) {
 		int offset;
 
 		if (!records[cpu])
@@ -3025,7 +3025,7 @@ int tracecmd_iterate_events_reverse(struct tracecmd_input *handle,
 	struct tep_record **records;
 	struct tep_record *record;
 	int next_cpu;
-	int max_cpus = handle->max_cpu;
+	int max_cpus = handle->cpus;
 	int cpu;
 	int ret = 0;
 
@@ -3119,7 +3119,7 @@ int tracecmd_iterate_events_multi(struct tracecmd_input **handles,
 
 	for (i = 0; i < nr_handles; i++) {
 		handle = handles[i];
-		cpus += handle->max_cpu;
+		cpus += handle->cpus;
 	}
 
 	records = calloc(cpus, sizeof(*records));
@@ -3129,7 +3129,7 @@ int tracecmd_iterate_events_multi(struct tracecmd_input **handles,
 	for (i = 0; i < nr_handles; i++) {
 		handle = handles[i];
 		handle->start_cpu = all_cpus;
-		for (cpu = 0; cpu < handle->max_cpu; cpu++) {
+		for (cpu = 0; cpu < handle->cpus; cpu++) {
 			records[all_cpus + cpu].record = tracecmd_peek_data(handle, cpu);
 			records[all_cpus + cpu].handle = handle;
 		}
-- 
2.43.0

[PATCH 2/3] libtracecmd: Do not free records at end of iterator

[Steven Rostedt]

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

I'm not sure why the records that were saved in the "peek" were freed, as
they should be freed when the tracecmd_input is closed.

Remove the freeing of the "peek" records, as it should be unneeded.

This basically reverts:

  890855541 ("tracecmd library: Unlock records in tracecmd_iterate_events()")

And commits that simulated it.

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

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index 0dc3dbdc676a..bc937b0491d7 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -2854,19 +2854,6 @@ int tracecmd_iterate_events(struct tracecmd_input *handle,
 		}
 	} while (next_cpu >= 0 && ret == 0);
 
-	/* Need to unlock and free the records */
-	for (cpu = 0; cpu < handle->cpus; cpu++) {
-		int offset;
-
-		if (!records[cpu])
-			continue;
-
-		offset = (int)(records[cpu]->offset & (handle->page_size - 1));
-		free_next(handle, cpu);
-		/* Reset the buffer to read the cached record again */
-		kbuffer_read_at_offset(handle->cpu_data[cpu].kbuf, offset, NULL);
-	}
-
 	free(records);
 
 	return ret;
@@ -2909,25 +2896,6 @@ load_records(struct tracecmd_input *handle, int cpu,
 	return last_record;
 }
 
-static void free_last_record(struct tracecmd_input *handle, struct tep_record *record,
-			     int cpu)
-{
-	record->priv = handle->cpu_data[cpu].page;
-	tracecmd_free_record(record);
-}
-
-static void free_last_records(struct tracecmd_input *handle, struct tep_record *records,
-			      int cpu)
-{
-	struct tep_record *last_record;
-
-	while (records) {
-		last_record = records;
-		records = last_record->priv;
-		free_last_record(handle, last_record, cpu);
-	}
-}
-
 static void initialize_last_events(struct tracecmd_input *handle,
 				   struct tep_record **last_records,
 				   cpu_set_t *cpu_set, int cpu_size,
@@ -3062,20 +3030,6 @@ int tracecmd_iterate_events_reverse(struct tracecmd_input *handle,
 		}
 	} while (next_cpu >= 0 && ret == 0);
 
-	for (cpu = 0; cpu < max_cpus; cpu++) {
-		int offset;
-
-		/* Get the next record to set the index to. */
-		record = peek_last_event(handle, records, cpu);
-		if (!record)
-			continue;
-		/* Reset the buffer to read the cached record again */
-		offset = record->offset & (handle->page_size - 1);
-		free_last_records(handle, records[cpu], cpu);
-		/* Reset the buffer to read the cached record again */
-		kbuffer_read_at_offset(handle->cpu_data[cpu].kbuf, offset, NULL);
-	}
-
 	free(records);
 
 	return ret;
@@ -3164,23 +3118,6 @@ int tracecmd_iterate_events_multi(struct tracecmd_input **handles,
 
 	} while (next_cpu >= 0 && ret == 0);
 
-	/* Unlock and free the records */
-	for (cpu = 0; cpu < all_cpus; cpu++) {
-		int local_cpu;
-		int offset;
-
-		if (!records[cpu].record)
-			continue;
-
-		handle = records[cpu].handle;
-		local_cpu = cpu - handle->start_cpu;
-
-		offset = (int)(records[cpu].record->offset & (handle->page_size - 1));
-		free_next(handle, local_cpu);
-		/* Reset the buffer to read the cached record again */
-		kbuffer_read_at_offset(handle->cpu_data[local_cpu].kbuf, offset, NULL);
-	}
-
 	free(records);
 
 	return ret;
-- 
2.43.0

[PATCH 3/3] libtracecmd: Just save timestamps and not the records in iterators

[Steven Rostedt]

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

The iterators cache the records via a tracecmd_peek_data() to find the next
CPU record to use. The problem is that the callback() may do a
tracecmd_read_data() (the function graph plugin does) and this may
invalidate the cached record. It would require refreshing every record of
every cached CPU after every callback, which can be expensive.

Since only the timestamps of the records are needed, simply cache the
timestamp of the record to find which is the next CPU record to use. After
finding the CPU that has the earliest timestamp, do a tracecmd_peek_data()
test again to make sure the timestamp still matches the record. If it does,
then simply use that record, if not, refresh the timestamp for that CPU and
try again.

Fixes: 2cb6cc2f4 ("tracecmd library: Add tracecmd_iterate_events()")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 lib/trace-cmd/trace-input.c | 60 ++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 20 deletions(-)

diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
index bc937b0491d7..dbcbf3613686 100644
--- a/lib/trace-cmd/trace-input.c
+++ b/lib/trace-cmd/trace-input.c
@@ -2807,9 +2807,9 @@ int tracecmd_iterate_events(struct tracecmd_input *handle,
 					    int, void *),
 			    void *callback_data)
 {
-	struct tep_record **records;
 	struct tep_record *record;
-	unsigned long long last_timestamp = 0;
+	unsigned long long *timestamps;
+	unsigned long long ts, last_timestamp = 0;
 	int next_cpu;
 	int cpu;
 	int ret = 0;
@@ -2819,42 +2819,53 @@ int tracecmd_iterate_events(struct tracecmd_input *handle,
 		return -1;
 	}
 
-	records = calloc(handle->cpus, sizeof(*records));
-	if (!records)
+	timestamps = calloc(handle->cpus, sizeof(*timestamps));
+	if (!timestamps)
 		return -1;
 
 	for (cpu = 0; cpu < handle->cpus; cpu++) {
 		if (cpus && !CPU_ISSET_S(cpu, cpu_size, cpus))
 			continue;
 
-		records[cpu] = tracecmd_peek_data(handle, cpu);
+		record = tracecmd_peek_data(handle, cpu);
+		timestamps[cpu] = record ? record->ts : -1ULL;
 	}
 
 	do {
 		next_cpu = -1;
 		for (cpu = 0; cpu < handle->cpus; cpu++) {
-			record = records[cpu];
-			if (!record)
+			ts = timestamps[cpu];
+			if (ts == -1ULL)
 				continue;
 
-			if (next_cpu < 0 || record->ts < last_timestamp) {
+			if (next_cpu < 0 || ts < last_timestamp) {
 				next_cpu = cpu;
-				last_timestamp = record->ts;
+				last_timestamp = ts;
 			}
 		}
 		if (next_cpu >= 0) {
+			record = tracecmd_peek_data(handle, next_cpu);
+
+			/* Make sure the record is still what we expect it to be */
+			if (!record || record->ts != last_timestamp) {
+				timestamps[next_cpu] = record ? record->ts : -1ULL;
+				continue;
+			}
+
 			/* Need to call read_data to increment to the next record */
 			record = tracecmd_read_data(handle, next_cpu);
 
 			ret = call_callbacks(handle, record, next_cpu,
 					     callback, callback_data);
 
-			records[next_cpu] = tracecmd_peek_data(handle, next_cpu);
 			tracecmd_free_record(record);
+
+			record = tracecmd_peek_data(handle, next_cpu);
+			timestamps[next_cpu] = record ? record->ts : -1ULL;
 		}
 	} while (next_cpu >= 0 && ret == 0);
 
-	free(records);
+	free(timestamps);
 
 	return ret;
 }
@@ -3036,7 +3047,7 @@ int tracecmd_iterate_events_reverse(struct tracecmd_input *handle,
 }
 
 struct record_handle {
-	struct tep_record		*record;
+	unsigned long long		ts;
 	struct tracecmd_input		*handle;
 };
 
@@ -3063,7 +3074,7 @@ int tracecmd_iterate_events_multi(struct tracecmd_input **handles,
 	struct tracecmd_input *handle;
 	struct record_handle *records;
 	struct tep_record *record;
-	unsigned long long last_timestamp = 0;
+	unsigned long long ts, last_timestamp = 0;
 	int next_cpu;
 	int cpus = 0;
 	int all_cpus = 0;
@@ -3084,7 +3095,8 @@ int tracecmd_iterate_events_multi(struct tracecmd_input **handles,
 		handle = handles[i];
 		handle->start_cpu = all_cpus;
 		for (cpu = 0; cpu < handle->cpus; cpu++) {
-			records[all_cpus + cpu].record = tracecmd_peek_data(handle, cpu);
+			record = tracecmd_peek_data(handle, cpu);
+			records[all_cpus + cpu].ts = record ? record->ts : -1ULL;
 			records[all_cpus + cpu].handle = handle;
 		}
 		all_cpus += cpu;
@@ -3093,19 +3105,28 @@ int tracecmd_iterate_events_multi(struct tracecmd_input **handles,
 	do {
 		next_cpu = -1;
 		for (cpu = 0; cpu < all_cpus; cpu++) {
-			record = records[cpu].record;
-			if (!record)
+			ts = records[cpu].ts;
+			if (ts == -1ULL)
 				continue;
 
-			if (next_cpu < 0 || record->ts < last_timestamp) {
+			if (next_cpu < 0 || ts < last_timestamp) {
 				next_cpu = cpu;
-				last_timestamp = record->ts;
+				last_timestamp = ts;
 			}
 		}
 		if (next_cpu >= 0) {
-			record = records[next_cpu].record;
 			handle = records[next_cpu].handle;
 			cpu = next_cpu - handle->start_cpu;
+
+			/* Refresh record as callback could have changed */
+			record = tracecmd_peek_data(handle, cpu);
+
+			/* If the record updated, try again */
+			if (!record || record->ts != last_timestamp) {
+				records[next_cpu].ts = record ? record->ts : -1ULL;
+				continue;
+			}
+
 			/* Need to call read_data to increment to the next record */
 			record = tracecmd_read_data(handle, cpu);
 
@@ -3113,7 +3134,6 @@ int tracecmd_iterate_events_multi(struct tracecmd_input **handles,
 					     callback, callback_data);
 
 			tracecmd_free_record(record);
-			records[next_cpu].record = tracecmd_peek_data(handle, cpu);
 		}
 
 	} while (next_cpu >= 0 && ret == 0);
-- 
2.43.0