On Wed, 1 Nov 2017 13:04:23 +0200 "Yordan Karadzhov (VMware)" <[email protected]> wrote:
> Makefile modified in order to support building of gui plugins. > > Function handlers for processing of plugin-specific events (xenomai > events "cobalt_switch_context" and "cobalt_thread_resume" in this case) > are addet to trace_view_store and graph_info. Very nice. > > Signed-off-by: Yordan Karadzhov (VMware) <[email protected]> > --- > Makefile | 35 +++++-- > kernel-shark.c | 8 +- > kshark-plugin.c | 65 +++++++++++++ > kshark-plugin.h | 49 +++++++++- > plugin_xenomai_gui.c | 257 > +++++++++++++++++++++++++++++++++++++++++++++++++++ > trace-graph.c | 48 ++++++++++ > trace-graph.h | 7 ++ > trace-plot-task.c | 17 +++- > trace-view-store.c | 24 +++++ > trace-view-store.h | 5 + > 10 files changed, 500 insertions(+), 15 deletions(-) > create mode 100644 kshark-plugin.c > create mode 100644 plugin_xenomai_gui.c > > diff --git a/Makefile b/Makefile > index 5c35143..96f30e2 100644 > --- a/Makefile > +++ b/Makefile > @@ -293,6 +293,8 @@ else > print_shared_lib_compile = echo ' $(GUI)COMPILE SHARED LIB '$(GOBJ); > print_plugin_obj_compile = echo ' $(GUI)COMPILE PLUGIN OBJ '$(GOBJ); > print_plugin_build = echo ' $(GUI)BUILD PLUGIN > '$(GOBJ); > + print_gui_plugin_obj_compile = echo ' $(GUI)COMPILE GUI_PLUGIN OBJ > '$(GOBJ); > + print_gui_plugin_build = echo ' $(GUI)BUILD GUI_PLUGIN > '$(GOBJ); > print_static_lib_build = echo ' $(GUI)BUILD STATIC LIB '$(GOBJ); > print_install = echo ' $(GUI)INSTALL '$(GSPACE)$1' to > $(DESTDIR_SQ)$2'; > endif > @@ -317,6 +319,14 @@ do_plugin_build = \ > ($(print_plugin_build) \ > $(CC) $(CFLAGS) $(LDFLAGS) -shared -nostartfiles -o $@ $<) > > +do_compile_gui_plugin_obj = \ > + ($(print_gui_plugin_obj_compile) \ > + $(CC) -c $(CPPFLAGS) $(CFLAGS) -fPIC -o $@ $<) > + > +do_gui_plugin_build = \ > + ($(print_gui_plugin_build) \ > + $(CC) $(CFLAGS) $(LDFLAGS) -shared -nostartfiles -o $@ $<) > + > do_build_static_lib = \ > ($(print_static_lib_build) \ > $(RM) $@; $(AR) rcs $@ $^) > @@ -338,17 +348,17 @@ $(obj)/%.o: $(src)/%.c > $(Q)$(call check_gui) > > TRACE_GUI_OBJS = trace-filter.o trace-compat.o trace-filter-hash.o > trace-dialog.o \ > - trace-xml.o > + trace-xml.o kshark-plugin.o > TRACE_CMD_OBJS = trace-cmd.o trace-record.o trace-read.o trace-split.o > trace-listen.o \ > trace-stack.o trace-hist.o trace-mem.o trace-snapshot.o trace-stat.o \ > trace-hash.o trace-profile.o trace-stream.o trace-record.o > trace-restore.o \ > trace-check-events.o trace-show.o trace-list.o > -TRACE_VIEW_OBJS = trace-view.o trace-view-store.o > +TRACE_VIEW_OBJS = trace-view.o trace-view-store.o kshark-plugin.o > TRACE_GRAPH_OBJS = trace-graph.o trace-plot.o trace-plot-cpu.o > trace-plot-task.o > TRACE_VIEW_MAIN_OBJS = trace-view-main.o $(TRACE_VIEW_OBJS) $(TRACE_GUI_OBJS) > TRACE_GRAPH_MAIN_OBJS = trace-graph-main.o $(TRACE_GRAPH_OBJS) > $(TRACE_GUI_OBJS) > KERNEL_SHARK_OBJS = $(TRACE_VIEW_OBJS) $(TRACE_GRAPH_OBJS) $(TRACE_GUI_OBJS) > \ > - trace-capture.o kernel-shark.o > + trace-capture.o kernel-shark.o kshark-plugin.o > > PEVENT_LIB_OBJS = event-parse.o trace-seq.o parse-filter.o parse-utils.o > str_error_r.o > TCMD_LIB_OBJS = $(PEVENT_LIB_OBJS) trace-util.o trace-input.o trace-ftrace.o > \ > @@ -373,13 +383,19 @@ PLUGIN_OBJS += plugin_tlb.o > > PLUGINS := $(PLUGIN_OBJS:.o=.so) > > + > +GUI_PLUGIN_OBJS = > +GUI_PLUGIN_OBJS += plugin_xenomai_gui.o > + > +GUI_PLUGINS := $(GUI_PLUGIN_OBJS:.o=.so) > + > ALL_OBJS = $(TRACE_CMD_OBJS) $(KERNEL_SHARK_OBJS) $(TRACE_VIEW_MAIN_OBJS) \ > - $(TRACE_GRAPH_MAIN_OBJS) $(TCMD_LIB_OBJS) $(PLUGIN_OBJS) > + $(TRACE_GRAPH_MAIN_OBJS) $(TCMD_LIB_OBJS) $(PLUGIN_OBJS) > $(GUI_PLUGIN_OBJS) > > CMD_TARGETS = trace_plugin_dir trace_python_dir tc_version.h libparsevent.a > $(LIB_FILE) \ > trace-cmd $(PLUGINS) $(BUILD_PYTHON) > > -GUI_TARGETS = ks_version.h trace-graph trace-view kernelshark > +GUI_TARGETS = ks_version.h trace-graph trace-view kernelshark $(GUI_PLUGINS) > > TARGETS = $(CMD_TARGETS) $(GUI_TARGETS) > > @@ -401,7 +417,7 @@ gui: $(CMD_TARGETS) > > all_gui: $(GUI_TARGETS) show_gui_done > > -GUI_OBJS = $(KERNEL_SHARK_OBJS) $(TRACE_VIEW_MAIN_OBJS) > $(TRACE_GRAPH_MAIN_OBJS) > +GUI_OBJS = $(KERNEL_SHARK_OBJS) $(TRACE_VIEW_MAIN_OBJS) > $(TRACE_GRAPH_MAIN_OBJS) $(GUI_PLUGIN_OBJS) > > gui_objs := $(sort $(GUI_OBJS)) > > @@ -447,6 +463,13 @@ $(PLUGIN_OBJS): %.o : $(src)/%.c > $(PLUGINS): %.so: %.o > $(Q)$(do_plugin_build) > > + > +$(GUI_PLUGIN_OBJS): %.o : $(src)/%.c > + $(Q)$(do_compile_gui_plugin_obj) > + > +$(GUI_PLUGINS): %.so: %.o > + $(Q)$(do_gui_plugin_build) > + > define make_version.h > (echo '/* This file is automatically generated. Do not modify. */'; > \ > echo \#define VERSION_CODE $(shell > \ The above is also very nicely done. The one thing that is missing is the install phase when "make install_gui" is performed. The gui plugins should also be installed. But I can add a patch to do that on top of this one. Maybe I'll send it to you for v2. (some minor nits so far). > diff --git a/kernel-shark.c b/kernel-shark.c > index 89723c3..9454ea0 100644 > --- a/kernel-shark.c > +++ b/kernel-shark.c > @@ -1830,7 +1830,7 @@ static void add_plugin(const char *file) > plugin_next = &(*plugin_next)->next; > } > > -static void handle_plugins(struct shark_info *info) > +static void handle_plugins(struct shark_info *info, struct trace_view_store > *store) > { > kshark_plugin_load_func func; > struct plugin_list *plugin; > @@ -1852,7 +1852,7 @@ static void handle_plugins(struct shark_info *info) > KSHARK_PLUGIN_LOADER_NAME, plugin->file, > dlerror()); > continue; > } > - func(info); > + func(info, store); > } > } > > @@ -2495,7 +2495,9 @@ void kernel_shark(int argc, char **argv) > > gtk_widget_set_size_request(window, TRACE_WIDTH, TRACE_HEIGHT); > > - handle_plugins(info); > + GtkTreeModel *model = > gtk_tree_view_get_model(GTK_TREE_VIEW(info->treeview)); > + handle_plugins(info, TRACE_VIEW_STORE(model)); > + > > gdk_threads_enter(); > > diff --git a/kshark-plugin.c b/kshark-plugin.c > new file mode 100644 > index 0000000..07d1a87 > --- /dev/null > +++ b/kshark-plugin.c > @@ -0,0 +1,65 @@ > +/* > + * Copyright (C) 2017 VMware Inc, Yordan Karadzhov <[email protected]> > + * > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; version 2 of the License (not later!) I wonder if we could make this part of a library, and then license this under LGPL. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, see <http://www.gnu.org/licenses> > + * > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + */ > + > +#include <stdlib.h> > + > +#include "kshark-plugin.h" > + > +struct gui_event_handler *make_gui_event_handler(int event_id, int type, If this does become a library (maybe not, not sure yet), we probably need to prefix all the functions with "kshark_". > + > kshark_plugin_event_handler_func evt_func, > + > kshark_plugin_context_update_func ctx_func) > +{ > + struct gui_event_handler *handler = malloc(sizeof(struct > gui_event_handler)); Add a empty line between the declarations and the code. Also, you need to check if handler failed to allocate. if (!handler) return NULL; Hopefully all callers of this check the return value too. > + handler->next = NULL; > + handler->id = event_id; > + handler->type = type; > + handler->event_func = evt_func; > + handler->context_func = ctx_func; > + > + return handler; > +} > + > +struct gui_event_handler *find_gui_event_handler(struct gui_event_handler > *handlers, > + int event_id) > +{ > + while (handlers) { > + if (handlers->id == event_id) > + return handlers; > + > + handlers = handlers->next; > + } > + > + return NULL; This is fine for now. But if there is a bunch of handlers created, we are going to have to optimize it more than searching a list. > +} > + > +void remove_gui_event_handler(struct gui_event_handler **handlers, int > event_id) > +{ > + struct gui_event_handler *list = *handlers; Add an empty line here. > + if (list == NULL) > + return; > + > + if (list->id == event_id) { > + *handlers = list->next; Hmm, where do we free list? > + return; > + } > + > + remove_gui_event_handler(&list->next, event_id); Just because I'm a kernel programmer, I hate recursive logic like this. In the kernel, there's a limited stack, and recursive functions can cause a kernel crash. Although, this works fine as is, I rather have a loop to handle this. The "kernel" way to perform this is like so: struct gui_event_handler **last = handlers; struct gui_event_handler *list; for (list = *handlers; list; list = list->next) { if (list->id == event_id) { *last = list->next; free(list); break; } last = &list->next; } > +} > + > diff --git a/kshark-plugin.h b/kshark-plugin.h > index 95ba797..65e1e93 100644 > --- a/kshark-plugin.h > +++ b/kshark-plugin.h > @@ -17,6 +17,9 @@ > * > * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > */ > + > +#include "event-parse.h" > + > #ifndef _KSHARK_PLUGIN_H > #define _KSHARK_PLUGIN_H > > @@ -28,8 +31,50 @@ > #define KSHARK_PLUGIN_LOADER_NAME MAKE_STR(KSHARK_PLUGIN_LOADER) > #define KSHARK_PLUGIN_UNLOADER_NAME MAKE_STR(KSHARK_PLUGIN_UNLOADER) > > -typedef int (*kshark_plugin_load_func)(void *info); > -typedef int (*kshark_plugin_unload_func)(void *info); > +typedef int (*kshark_plugin_load_func)(void *info, void *store); > +typedef int (*kshark_plugin_unload_func)(void *info, void *store); > + > +typedef int (*kshark_plugin_event_handler_func)(struct pevent_record *record, > + int task_id, > + void *val); > + > +typedef int (*kshark_plugin_context_update_func)(struct pevent *pevent, int > val); > + > +enum gui_plugin_actions { > + KSHARK_PLUGIN_GET_PID, > + KSHARK_PLUGIN_GET_PREV_STATE, > + KSHARK_PLUGIN_GET_COMMAND > +}; > + > +enum gui_plugin_ctx_updates { > + KSHARK_PLUGIN_UPDATE_SWITCH_EVENT = 0x1, > + KSHARK_PLUGIN_UPDATE_WAKEUP_EVENT = 0x2, > + KSHARK_PLUGIN_UPDATE_WAKEUP_PID = 0x4, > + KSHARK_PLUGIN_UPDATE_SWITCH_PID = 0x8, > + KSHARK_PLUGIN_UPDATE_PREV_STATE = 0x10, > + KSHARK_PLUGIN_UPDATE_NEXT_NAME = 0x20 You want to use (1<<x) notation and also tab align the assignments: KSHARK_PLUGIN_UPDATE_SWITCH_EVENT = (1 << 0), KSHARK_PLUGIN_UPDATE_WAKEUP_EVENT = (1 << 1), KSHARK_PLUGIN_UPDATE_WAKEUP_PID = (1 << 2), KSHARK_PLUGIN_UPDATE_SWITCH_PID = (1 << 3), KSHARK_PLUGIN_UPDATE_PREV_STATE = (1 << 4), KSHARK_PLUGIN_UPDATE_NEXT_NAME = (1 << 5), Notice I ended with a comma. enums are fine with the last element ending with a comma. We do that because it makes it easier to add new elements later. > +}; > + > +enum gui_event_types { > + KSHARK_PLUGIN_SWITCH_EVENT, > + KSHARK_PLUGIN_WAKEUP_EVENT > +}; > + > +struct gui_event_handler { > + struct gui_event_handler *next; > + int id; > + int type; > + kshark_plugin_event_handler_func event_func; > + kshark_plugin_context_update_func context_func; > +}; > + > +struct gui_event_handler *make_gui_event_handler(int event_id, int type, > + > kshark_plugin_event_handler_func evt_func, > + > kshark_plugin_context_update_func ctx_func); > + > +struct gui_event_handler *find_gui_event_handler(struct gui_event_handler > *handlers, > + int event_id); > > +void remove_gui_event_handler(struct gui_event_handler **handlers, int > event_id); > > #endif > diff --git a/plugin_xenomai_gui.c b/plugin_xenomai_gui.c > new file mode 100644 > index 0000000..f22a7a7 > --- /dev/null > +++ b/plugin_xenomai_gui.c > @@ -0,0 +1,257 @@ > +/* > + * Copyright (C) 2017 VMware Inc, Yordan Karadzhov <[email protected]> > + * > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; > + * version 2.1 of the License (not later!) > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this program; if not, see > <http://www.gnu.org/licenses> > + * > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + */ > + > +#include <stdlib.h> > +#include <stdio.h> > + > +#include "event-parse.h" > +#include "kernel-shark.h" > +#include "kshark-plugin.h" > + > +struct xenomai_context { > + struct shark_info *info; > + struct trace_view_store *store; > + struct pevent *pevent; > + > + struct event_format *cobalt_switch_event; > + struct format_field *cobalt_switch_next_pid_field; > + struct format_field *cobalt_switch_prev_state_field; > + struct format_field *cobalt_switch_next_name_field; > + > + struct event_format *cobalt_wakeup_event; > + struct format_field *cobalt_wakeup_pid_field; > +}; > + > +struct xenomai_context *xenomai_context_handler = NULL; > + > +static gboolean xenomai_update_context(struct pevent *pevent, int task_id) > +{ > + struct xenomai_context *ctx = xenomai_context_handler; Add empty line here. > + if (!ctx) > + return FALSE; > + > + struct event_format *event; Also, declare all variables at the start of the function, as old C style would require. I find it easier to understand code this way than having variables declared in the middle of functions, and having to search for what types they are. > + > + if (task_id & KSHARK_PLUGIN_UPDATE_SWITCH_EVENT) { > + event = > pevent_find_event_by_name(xenomai_context_handler->pevent, > + "cobalt_core", > + "cobalt_switch_context"); > + if (!event) > + return FALSE; > + > + ctx->cobalt_switch_event = event; > + } > + > + if (task_id & KSHARK_PLUGIN_UPDATE_WAKEUP_EVENT) { > + event = > pevent_find_event_by_name(xenomai_context_handler->pevent, > + "cobalt_core", > + "cobalt_thread_resume"); > + if (!event) > + return FALSE; > + > + ctx->cobalt_wakeup_event = event; > + } > + > + if (task_id & KSHARK_PLUGIN_UPDATE_SWITCH_PID) { > + ctx->cobalt_switch_next_pid_field = > + pevent_find_field(ctx->cobalt_switch_event, "next_pid"); Indent more "pevent_find_field()" as it is a continuation of the previous assignment. > + } > + > + if (task_id & KSHARK_PLUGIN_UPDATE_PREV_STATE) { > + ctx->cobalt_switch_prev_state_field = > + pevent_find_field(ctx->cobalt_switch_event, "prev_state"); Do it for all of them too. > + } > + > + if (task_id & KSHARK_PLUGIN_UPDATE_NEXT_NAME) { > + ctx->cobalt_switch_next_name_field = > + pevent_find_field(ctx->cobalt_switch_event, "next_name"); > + } > + > + if (task_id & KSHARK_PLUGIN_UPDATE_WAKEUP_PID) { > + ctx->cobalt_wakeup_pid_field = > + pevent_find_field(ctx->cobalt_wakeup_event, "pid"); > + } > + > + return TRUE; > +} > + > +static void xenomai_context_new(struct shark_info *info, struct > trace_view_store *store) > +{ > + if (!xenomai_context_handler) { > + xenomai_context_handler = > + (struct xenomai_context*) malloc(sizeof(struct > xenomai_context)); > + } > + > + xenomai_context_handler->info = info; > + xenomai_context_handler->store = store; > + xenomai_context_handler->pevent = tracecmd_get_pevent(info->handle); > + > + int status = xenomai_update_context(xenomai_context_handler->pevent, > + KSHARK_PLUGIN_UPDATE_SWITCH_EVENT | > + KSHARK_PLUGIN_UPDATE_WAKEUP_EVENT); > + if (status == FALSE) { > + free(xenomai_context_handler); > + xenomai_context_handler = NULL; > + } > +} > + > +static int cobalt_get_next_pid(struct xenomai_context *ctx, > + struct pevent_record *record, > + int *pid) > +{ > + long long unsigned int val; > + int status = pevent_read_number_field(ctx->cobalt_switch_next_pid_field, > + record->data, &val); > + if (pid) > + *pid = val; > + > + return status; > +} > + > + > +static int cobalt_get_prev_state(struct xenomai_context *ctx, > + struct pevent_record *record, > + int *state) > +{ > + long long unsigned int val; > + pevent_read_number_field(ctx->cobalt_switch_prev_state_field, > + record->data, &val); > + > + if (state) > + *state = val; > + > + return (val & 0x00000008) ? 1 : 0; > +} > + > +static void cobalt_get_command(struct xenomai_context *ctx, > + struct pevent_record *record, > + const char **comm) > +{ > + int offset = > + data2host4(ctx->pevent, record->data + > ctx->cobalt_switch_next_name_field->offset); > + > + offset &= 0xffff; > + *comm = record->data + offset; > +} > + > +static gboolean xenomai_switch_handler(struct pevent_record *record, > + int task_id, > + void *output) > +{ > + struct xenomai_context *ctx = xenomai_context_handler; > + switch (task_id) { > + case KSHARK_PLUGIN_GET_PID: > + cobalt_get_next_pid(ctx, record, output); > + return TRUE; > + > + case KSHARK_PLUGIN_GET_PREV_STATE: > + return cobalt_get_prev_state(ctx, record, output); > + > + case KSHARK_PLUGIN_GET_COMMAND: > + cobalt_get_command(ctx, record, (const char**) output); > + return TRUE; > + > + default: > + return FALSE; > + } > +} > + > +static int cobalt_get_wakeup_pid(struct xenomai_context *ctx, > + struct pevent_record *record, > + int *pid) > +{ > + long long unsigned int val; > + int status = > + pevent_read_number_field(ctx->cobalt_wakeup_pid_field, record->data, > &val); > + > + if (pid) > + *pid = val; > + > + return status; > +} > + > +static gboolean xenomai_wakeup_handler(struct pevent_record *record, > + int task_id, > + void *output) > +{ > + struct xenomai_context *ctx = xenomai_context_handler; > + switch (task_id) { > + case KSHARK_PLUGIN_GET_PID: > + cobalt_get_wakeup_pid(ctx, record, output); > + return TRUE; > + > + default: > + return FALSE; > + } > +} > + > + > +void KSHARK_PLUGIN_LOADER(void *info, void *store) > +{ > + struct shark_info *ks_info = info; > + struct trace_view_store *ks_store = store; > + > + xenomai_context_new(ks_info, ks_store); > + if (!xenomai_context_handler) > + return; > + > + struct xenomai_context *ctx = xenomai_context_handler; > + > + struct gui_event_handler *switch_handler = > + make_gui_event_handler(ctx->cobalt_switch_event->id, > + KSHARK_PLUGIN_SWITCH_EVENT, > + xenomai_switch_handler, > + xenomai_update_context); > + > + struct gui_event_handler *wakeup_handler = > + make_gui_event_handler(ctx->cobalt_wakeup_event->id, > + KSHARK_PLUGIN_WAKEUP_EVENT, > + xenomai_wakeup_handler, > + xenomai_update_context); > + > + trace_view_store_register_gui_handler(ks_store, switch_handler); > + trace_view_store_register_gui_handler(ks_store, wakeup_handler); > + > + trace_graph_register_gui_handler(ks_info->ginfo, switch_handler); > + trace_graph_register_gui_handler(ks_info->ginfo, wakeup_handler); > +} > + > +void KSHARK_PLUGIN_UNLOADER(void *info, void *store) > +{ > + struct shark_info *ks_info = info; > + struct trace_view_store *ks_store = store; > + struct xenomai_context *ctx = xenomai_context_handler; > + > + remove_gui_event_handler(&ks_store->event_handlers, > + ctx->cobalt_switch_event->id); > + > + remove_gui_event_handler(&ks_store->event_handlers, > + ctx->cobalt_wakeup_event->id); > + > + remove_gui_event_handler(&ks_info->ginfo->event_handlers, > + ctx->cobalt_switch_event->id); > + > + remove_gui_event_handler(&ks_info->ginfo->event_handlers, > + ctx->cobalt_wakeup_event->id); > + > + free(ctx); > + xenomai_context_handler = NULL; > +} > + > diff --git a/trace-graph.c b/trace-graph.c > index 1db342f..19f02c0 100644 > --- a/trace-graph.c > +++ b/trace-graph.c > @@ -129,6 +129,12 @@ static void free_task_hash(struct graph_info *ginfo) > } > } > > +void trace_graph_register_gui_handler(struct graph_info *info, > + struct gui_event_handler *handler) { > + handler->next = info->event_handlers; > + info->event_handlers = handler; > +} > + > /** > * trace_graph_task_list - return an allocated list of all found tasks > * @ginfo: The graph info structure > @@ -1053,6 +1059,12 @@ int trace_graph_check_sched_wakeup(struct graph_info > *ginfo, > > id = pevent_data_type(ginfo->pevent, record); > > + struct gui_event_handler *handler = > find_gui_event_handler(ginfo->event_handlers, id); The handler needs to be declared at the beginning of the function. (Old C style). And yes, we need to optimize this, because this is in the fast path. > + if (handler) { > + if (handler->type == KSHARK_PLUGIN_WAKEUP_EVENT) > + handler->context_func(ginfo->pevent, > KSHARK_PLUGIN_UPDATE_WAKEUP_PID); > + } Hmm. The context_func() is used to search if the required event exists. We don't want to do that for every wake up event. Just once. Perhaps the handler should have a "seen" mask that can skip calling the context_func. if (handler && !(handler->seen & KSHARK_PLUGIN_WAKEUP_EVENT) { handler->seen |= KSHARK_PLUGIN_WAKEUP_EVENT; if (handler->type == KSHARK_PLUGIN_WAKEUP_EVENT) handler->context_func(...); } Something like that. The event then needs to be a bitmask. > + > if (id == ginfo->event_wakeup_id) { > /* We only want those that actually woke up the task */ > if (ginfo->wakeup_success_field) { > @@ -1079,6 +1091,16 @@ int trace_graph_check_sched_wakeup(struct graph_info > *ginfo, > return 1; > } > > + if (handler) { > + if (handler->type == KSHARK_PLUGIN_WAKEUP_EVENT) { > + handler->event_func(record, KSHARK_PLUGIN_GET_PID, > &val); > + if (pid) > + *pid = val; > + > + return 1; > + } > + } > + > return 0; > } > > @@ -1118,7 +1140,20 @@ int trace_graph_check_sched_switch(struct graph_info > *ginfo, > } > } > > + > id = pevent_data_type(ginfo->pevent, record); > + struct gui_event_handler *handler = > + find_gui_event_handler(ginfo->event_handlers, id); Indent find_gui_event_handler() and all declarations must be at the begging of the function. And didn't we already find the handler? > + > + if (handler) { > + if (handler->type == KSHARK_PLUGIN_SWITCH_EVENT) { > + handler->context_func(ginfo->pevent, > + KSHARK_PLUGIN_UPDATE_SWITCH_PID | > + KSHARK_PLUGIN_UPDATE_PREV_STATE | > + KSHARK_PLUGIN_UPDATE_NEXT_NAME); We should be able to consolidate the updating of context_func. > + } > + } > + > if (id == ginfo->event_sched_switch_id) { > pevent_read_number_field(ginfo->event_pid_field, record->data, > &val); > if (comm) > @@ -1139,6 +1174,17 @@ int trace_graph_check_sched_switch(struct graph_info > *ginfo, > goto out; > } > > + if (handler) { > + if (handler->type == KSHARK_PLUGIN_SWITCH_EVENT) { > + handler->event_func(record, KSHARK_PLUGIN_GET_PID, > &val); > + if (pid) > + *pid = val; > + if(comm) > + handler->event_func(record, > KSHARK_PLUGIN_GET_COMMAND, &comm); > + goto out; > + } > + } > + > ret = 0; > out: > if (ret && comm && ginfo->read_comms) { > @@ -2781,6 +2827,8 @@ trace_graph_create_with_callbacks(struct tracecmd_input > *handle, > > ginfo->callbacks = cbs; > > + ginfo->event_handlers = NULL; > + > ginfo->task_filter = filter_task_hash_alloc(); > ginfo->hide_tasks = filter_task_hash_alloc(); > > diff --git a/trace-graph.h b/trace-graph.h > index 7e66838..21e8feb 100644 > --- a/trace-graph.h > +++ b/trace-graph.h > @@ -24,6 +24,7 @@ > #include "trace-cmd.h" > #include "trace-filter-hash.h" > #include "trace-xml.h" > +#include "kshark-plugin.h" > > struct graph_info; > > @@ -258,6 +259,8 @@ struct graph_info { > gint plot_data_y; > gint plot_data_w; > gint plot_data_h; > + > + struct gui_event_handler *event_handlers; > }; > > > @@ -266,6 +269,10 @@ trace_graph_create(struct tracecmd_input *handle); > struct graph_info * > trace_graph_create_with_callbacks(struct tracecmd_input *handle, > struct graph_callbacks *cbs); > + > +void trace_graph_register_gui_handler(struct graph_info *info, > + struct gui_event_handler *handler); > + > void trace_graph_select_by_time(struct graph_info *ginfo, guint64 time); > > void trace_graph_event_filter_callback(gboolean accept, > diff --git a/trace-plot-task.c b/trace-plot-task.c > index 3b7e81f..56c3e96 100644 > --- a/trace-plot-task.c > +++ b/trace-plot-task.c > @@ -68,11 +68,20 @@ static gboolean is_running(struct graph_info *ginfo, > struct pevent_record *recor > int id; > > id = pevent_data_type(ginfo->pevent, record); > - if (id != ginfo->event_sched_switch_id) > - return FALSE; > > - pevent_read_number_field(ginfo->event_prev_state, record->data, &val); > - return val & ((1 << 11) - 1)? FALSE : TRUE; Ug. Not your issues, but I need to remove those hard coded numbers. > + if (id == ginfo->event_sched_switch_id) { > + pevent_read_number_field(ginfo->event_prev_state, record->data, > &val); > + return val & ((1 << 11) - 1)? FALSE : TRUE; > + } > + > + struct gui_event_handler *handler = > + find_gui_event_handler(ginfo->event_handlers, id); > + if (handler) { > + if (handler->type == KSHARK_PLUGIN_SWITCH_EVENT) > + return handler->event_func(record, > KSHARK_PLUGIN_GET_PREV_STATE, NULL); > + } > + > + return FALSE; > } > > static gboolean record_matches_pid(struct graph_info *ginfo, > diff --git a/trace-view-store.c b/trace-view-store.c > index f5d0a04..d7293c3 100644 > --- a/trace-view-store.c > +++ b/trace-view-store.c > @@ -70,6 +70,12 @@ static gboolean trace_view_store_iter_parent > (GtkTreeModel *tree_model, > static GObjectClass *parent_class = NULL; /* GObject stuff - nothing to > worry about */ > > > +void trace_view_store_register_gui_handler(TraceViewStore *store, struct > gui_event_handler *handler) > +{ > + handler->next = store->event_handlers; > + store->event_handlers = handler; > +} > + > > /***************************************************************************** > * > * trace_view_store_get_type: here we register our new type and its > interfaces > @@ -866,6 +872,7 @@ trace_view_store_new (struct tracecmd_input *handle) > > g_assert( newstore != NULL ); > > + newstore->event_handlers = NULL; > newstore->handle = handle; > newstore->cpus = tracecmd_cpus(handle); > tracecmd_ref(handle); > @@ -1224,6 +1231,14 @@ static gboolean show_task(TraceViewStore *store, > struct pevent *pevent, > return TRUE; > } > > + struct gui_event_handler *handler = > + find_gui_event_handler(store->event_handlers, event_id); > + if (handler) { > + handler->event_func(record, KSHARK_PLUGIN_GET_PID, &pid); > + if (view_task(store, pid)) > + return TRUE; > + } > + > return FALSE; > } > > @@ -1261,6 +1276,15 @@ static void update_filter_tasks(TraceViewStore *store) > "pid"); > } > > + struct gui_event_handler *handler = store->event_handlers; Also, declare handler at the beginning of the function. It's fine to do assignments here. > + while (handler) { Can you make this a for loop. > + handler->context_func( pevent, > + KSHARK_PLUGIN_UPDATE_SWITCH_PID | > + KSHARK_PLUGIN_UPDATE_WAKEUP_PID); We could add the "seen" here too. Thanks! -- Steve > + > + handler = handler->next; > + } > + > for (cpu = 0; cpu < store->cpus; cpu++) { > record = tracecmd_read_cpu_first(handle, cpu); > > diff --git a/trace-view-store.h b/trace-view-store.h > index 03141b1..333116c 100644 > --- a/trace-view-store.h > +++ b/trace-view-store.h > @@ -23,6 +23,7 @@ > #include <gtk/gtk.h> > #include "trace-cmd.h" > #include "trace-filter-hash.h" > +#include "kshark-plugin.h" > > /* Some boilerplate GObject defines. 'klass' is used > * instead of 'class', because 'class' is a C++ keyword */ > @@ -125,8 +126,12 @@ struct trace_view_store > guint64 *cpu_mask; /* cpus that are enabled */ > > gint stamp; /* Random integer to check whether an iter > belongs to our model */ > + > + struct gui_event_handler *event_handlers; > }; > > +void trace_view_store_register_gui_handler(TraceViewStore *store, struct > gui_event_handler *handler); > + > gboolean trace_view_store_cpu_isset(TraceViewStore *store, gint cpu); > > void trace_view_store_set_all_cpus(TraceViewStore *store);
