From patchwork Thu Jan 11 22:15:36 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Steven Rostedt X-Patchwork-Id: 13517804 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4F58258AAE for ; Thu, 11 Jan 2024 22:20:58 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id E4DB7C43390; Thu, 11 Jan 2024 22:20:57 +0000 (UTC) Received: from rostedt by gandalf with local (Exim 4.97) (envelope-from ) id 1rO3R3-00000000eGd-3DFG; Thu, 11 Jan 2024 17:22:01 -0500 From: Steven Rostedt To: linux-trace-devel@vger.kernel.org Cc: "Steven Rostedt (Google)" Subject: [PATCH 1/3] libtracecmd: Use cpu_data[cpu]->cpus and not ->max_cpu Date: Thu, 11 Jan 2024 17:15:36 -0500 Message-ID: <20240111222201.154686-2-rostedt@goodmis.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240111222201.154686-1-rostedt@goodmis.org> References: <20240111222201.154686-1-rostedt@goodmis.org> Precedence: bulk X-Mailing-List: linux-trace-devel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 From: "Steven Rostedt (Google)" 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) --- 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; } From patchwork Thu Jan 11 22:15:37 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Steven Rostedt X-Patchwork-Id: 13517803 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4F54558AAD for ; Thu, 11 Jan 2024 22:20:58 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id ED6D4C433F1; Thu, 11 Jan 2024 22:20:57 +0000 (UTC) Received: from rostedt by gandalf with local (Exim 4.97) (envelope-from ) id 1rO3R3-00000000eGg-3KnC; Thu, 11 Jan 2024 17:22:01 -0500 From: Steven Rostedt To: linux-trace-devel@vger.kernel.org Cc: "Steven Rostedt (Google)" Subject: [PATCH 2/3] libtracecmd: Do not free records at end of iterator Date: Thu, 11 Jan 2024 17:15:37 -0500 Message-ID: <20240111222201.154686-3-rostedt@goodmis.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240111222201.154686-1-rostedt@goodmis.org> References: <20240111222201.154686-1-rostedt@goodmis.org> Precedence: bulk X-Mailing-List: linux-trace-devel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 From: "Steven Rostedt (Google)" 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) --- 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; From patchwork Thu Jan 11 22:15:38 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Steven Rostedt X-Patchwork-Id: 13517802 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4F4C858AA6 for ; Thu, 11 Jan 2024 22:20:58 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id F2FBBC43394; Thu, 11 Jan 2024 22:20:57 +0000 (UTC) Received: from rostedt by gandalf with local (Exim 4.97) (envelope-from ) id 1rO3R3-00000000eGl-3RZl; Thu, 11 Jan 2024 17:22:01 -0500 From: Steven Rostedt To: linux-trace-devel@vger.kernel.org Cc: "Steven Rostedt (Google)" Subject: [PATCH 3/3] libtracecmd: Just save timestamps and not the records in iterators Date: Thu, 11 Jan 2024 17:15:38 -0500 Message-ID: <20240111222201.154686-4-rostedt@goodmis.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240111222201.154686-1-rostedt@goodmis.org> References: <20240111222201.154686-1-rostedt@goodmis.org> Precedence: bulk X-Mailing-List: linux-trace-devel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 From: "Steven Rostedt (Google)" 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) --- 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);