On 14.10.20 г. 21:56 ч., Steven Rostedt wrote:
On Mon, 12 Oct 2020 16:35:13 +0300 "Yordan Karadzhov (VMware)" <y.karadz@xxxxxxxxx> wrote:Here we switch to using the trace data readout, provided by the Data stream interface. The actual implementation of the new readout was done in the previous commits. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@xxxxxxxxx> --- --- a/src/libkshark.c +++ b/src/libkshark.c @@ -19,6 +19,7 @@// KernelShark#include "libkshark.h" +#include "libkshark-tepdata.h"static __thread struct trace_seq seq; @@ -32,6 +33,9 @@ static bool kshark_default_context(struct kshark_context **context)if (!kshark_ctx) return false;+ kshark_ctx->stream = calloc(KS_MAX_NUM_STREAMS,+ sizeof(*kshark_ctx->stream)); + kshark_ctx->event_handlers = NULL; kshark_ctx->collections = NULL; kshark_ctx->plugins = NULL; @@ -108,62 +112,28 @@ bool kshark_instance(struct kshark_context **kshark_ctx) return true; }-static void kshark_free_task_list(struct kshark_context *kshark_ctx)-{ - struct kshark_task_list *task; - int i; - - if (!kshark_ctx) - return; - - for (i = 0; i < KS_TASK_HASH_SIZE; ++i) { - while (kshark_ctx->tasks[i]) { - task = kshark_ctx->tasks[i]; - kshark_ctx->tasks[i] = task->next; - free(task); - } - } -} - /** * @brief Open and prepare for reading a trace data file specified by "file". - * If the specified file does not exist, or contains no trace data, - * the function returns false. * * @param kshark_ctx: Input location for context pointer. * @param file: The file to load. * - * @returns True on success, or false on failure. + * @returns The Id number of the data stream associated with this file on success. + * Otherwise a negative errno code. */ -bool kshark_open(struct kshark_context *kshark_ctx, const char *file) +int kshark_open(struct kshark_context *kshark_ctx, const char *file) { - struct tracecmd_input *handle; - - kshark_free_task_list(kshark_ctx); - - handle = tracecmd_open(file); - if (!handle) - return false; - - if (pthread_mutex_init(&kshark_ctx->input_mutex, NULL) != 0) { - tracecmd_close(handle); - return false; - } - - kshark_ctx->handle = handle; - kshark_ctx->pevent = tracecmd_get_pevent(handle); + int sd, rt;- kshark_ctx->advanced_event_filter =- tep_filter_alloc(kshark_ctx->pevent); + sd = kshark_add_stream(kshark_ctx); + if (sd < 0) + return sd;- /*- * Turn off function trace indent and turn on show parent - * if possible. - */ - tep_plugin_add_option("ftrace:parent", "1"); - tep_plugin_add_option("ftrace:indent", "0"); + rt = kshark_stream_open(kshark_ctx->stream[sd], file); + if (rt < 0)On failure, we should probably destroy the stream that was created.
Correct, will be fixed in v3.
+ return rt;- return true;+ return sd; }static void kshark_stream_free(struct kshark_data_stream *stream)@@ -253,6 +223,56 @@ int kshark_add_stream(struct kshark_context *kshark_ctx) return stream->stream_id; }+static bool is_tep(const char *filename)+{ + /* + * TODO: This is very naive. Implement more appropriate check. Ideally + * it should be part of the trace-cmd library. + */ + char *ext = strrchr(filename, '.'); + return ext && strcmp(ext, ".dat") == 0; +} + +static void set_format(struct kshark_context *kshark_ctx, + struct kshark_data_stream *stream, + const char *filename) +{ + stream->format = KS_INVALIDE_DATA; + + if (is_tep(filename)) { + stream->format = KS_TEP_DATA; + return; + } +} + +/** + * @brief Use an existing Trace data stream to open and prepare for reading + * a trace data file specified by "file". + * + * @param stream: Input location for a Trace data stream pointer. + * @param file: The file to load. + * + * @returns Zero on success or a negative error code in the case of an errno. + */ +int kshark_stream_open(struct kshark_data_stream *stream, const char *file) +{ + struct kshark_context *kshark_ctx = NULL; + + if (!stream || !kshark_instance(&kshark_ctx)) + return -EFAULT; + + stream->file = strdup(file); + set_format(kshark_ctx, stream, file); + + switch (stream->format) { + case KS_TEP_DATA: + return kshark_tep_init_input(stream, file); + + default: + return -ENODATA; + } +} + /** * @brief Get the Data stream object having given Id. * @@ -313,45 +333,76 @@ int *kshark_all_streams(struct kshark_context *kshark_ctx) return ids; }+static void kshark_stream_close(struct kshark_data_stream *stream)+{ + struct kshark_context *kshark_ctx = NULL; + + if (!stream || !kshark_instance(&kshark_ctx)) + return; + + /* + * All filters are file specific. Make sure that all Process Ids and + * Event Ids from this file are not going to be used with another file. + */ + kshark_hash_id_clear(stream->show_task_filter); + kshark_hash_id_clear(stream->hide_task_filter); + kshark_hash_id_clear(stream->show_event_filter); + kshark_hash_id_clear(stream->hide_event_filter); + kshark_hash_id_clear(stream->show_cpu_filter); + kshark_hash_id_clear(stream->hide_cpu_filter); + + switch (stream->format) { + case KS_TEP_DATA: + kshark_tep_close_interface(stream); + break; + + default: + break; + } + + pthread_mutex_destroy(&stream->input_mutex); +} + /** * @brief Close the trace data file and free the trace data handle. * * @param kshark_ctx: Input location for the session context pointer. + * @param sd: Data stream identifier. */ -void kshark_close(struct kshark_context *kshark_ctx) +void kshark_close(struct kshark_context *kshark_ctx, int sd) { - if (!kshark_ctx || !kshark_ctx->handle) + struct kshark_data_stream *stream; + + stream = kshark_get_data_stream(kshark_ctx, sd); + if (!stream) return;- /*- * All filters are file specific. Make sure that the Pids and Event Ids - * from this file are not going to be used with another file. - */ - tracecmd_filter_id_clear(kshark_ctx->show_task_filter); - tracecmd_filter_id_clear(kshark_ctx->hide_task_filter); - tracecmd_filter_id_clear(kshark_ctx->show_event_filter); - tracecmd_filter_id_clear(kshark_ctx->hide_event_filter); - tracecmd_filter_id_clear(kshark_ctx->show_cpu_filter); - tracecmd_filter_id_clear(kshark_ctx->hide_cpu_filter); - - if (kshark_ctx->advanced_event_filter) { - tep_filter_reset(kshark_ctx->advanced_event_filter); - tep_filter_free(kshark_ctx->advanced_event_filter); - kshark_ctx->advanced_event_filter = NULL; - } + kshark_stream_close(stream); + kshark_stream_free(stream); + kshark_ctx->stream[sd] = NULL; + kshark_ctx->n_streams--;So, if you have multiple streams, and you close one that's not the last, and then add a new one, this will cause the new one to be overwritten. As add_stream has: kshark_ctx->stream[kshark_ctx->n_streams++] = stream;
I see the problem. This is definitely wrong.What if in addition to "n_streams" I add another counter called "last_stream_added" and initialize this counter to -1?
Then I can add streams like this: kshark_ctx->stream[++kshark_ctx->last_stream_added] = stream; ++kshark_ctx->n_streams;
You may want to do instead:
kshark_ctx->stream[sd] = NULL;
while (!kshark->streams[kshark_ctx->n_streams - 1])
kshark_ctx->n_streams--;
You can also add a free store, where you store an index in the stream[sd]
field of the next free item. Then for adding you have:
if (kshark_ctx->free >= 0) {
sd = kshark_ctx->free;
kshark_ctx->free = (int)kshark_ctx->stream[kshark_ctx->free];
} else {
sd = kshark_ctx->n_streams++;
}
and on freeing:
kshark_ctx->streams[sd] = (void *)kshark_ctx->free;
kshark_ctx->free = sd;
Just need to initialize kshark_ctx->free to -1.
And never decrement n_streams. If you do any loops over the stream, you
could verify that it is a real stream by:
I really need n_streams to show the true number of active streams because the widgets are using this a lot.
The way I loop over the active streams is the following:
int *stream_ids = kshark_all_streams(kshark_ctx);
for (i = 0; i < kshark_ctx->n_streams; ++i) {
stream = kshark_ctx->stream[stream_ids[i]];
....
}
free(stream_ids);
and with the addition of "last_stream_added" kshark_all_streams() will
look like this:
int *kshark_all_streams(struct kshark_context *kshark_ctx)
{
int *ids, i, count = 0;
ids = calloc(kshark_ctx->n_streams, (sizeof(*ids)));
if (!ids)
return NULL;
for (i = 0; i <= kshark_ctx->last_stream_added; ++i)
if (kshark_ctx->stream[i])
ids[count++] = i;
return ids;
}
What do you think?
Thanks a lot!
Yordan
if ((unsigned long)kshark_ctx->streams[sd] > kshark_ctx->n_streams) /* pointer to a stream */ else /* a free item. */ Places like kshark_close() would need the above (if doing a free store). -- Steve+} + +/** + * @brief Close all currently open trace data file and free the trace data handle. + * + * @param kshark_ctx: Input location for the session context pointer. + */ +void kshark_close_all(struct kshark_context *kshark_ctx) +{ + int i, *stream_ids, n_streams; + + stream_ids = kshark_all_streams(kshark_ctx);/*- * All data collections are file specific. Make sure that collections - * from this file are not going to be used with another file. + * Get a copy of shark_ctx->n_streams befor you start closing. Be aware + * that kshark_close() will decrement shark_ctx->n_streams. */ - kshark_free_collection_list(kshark_ctx->collections); - kshark_ctx->collections = NULL; - - tracecmd_close(kshark_ctx->handle); - kshark_ctx->handle = NULL; - kshark_ctx->pevent = NULL; + n_streams = kshark_ctx->n_streams; + for (i = 0; i < n_streams; ++i) + kshark_close(kshark_ctx, stream_ids[i]);- pthread_mutex_destroy(&kshark_ctx->input_mutex);+ free(stream_ids); }/**
|