* Re: [PATCH 2/2] kernelshark: Adding a GUI plugin for xenomai events
2017-11-01 11:04 ` [PATCH 2/2] kernelshark: Adding a GUI plugin for xenomai events Yordan Karadzhov (VMware)
@ 2017-11-01 15:19 ` Steven Rostedt
0 siblings, 0 replies; 4+ messages in thread
From: Steven Rostedt @ 2017-11-01 15:19 UTC (permalink / raw)
To: Yordan Karadzhov (VMware); +Cc: jan.kiszka, linux-kernel
On Wed, 1 Nov 2017 13:04:23 +0200
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> 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) <y.karadz@gmail.com>
> ---
> 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 <y.karadz@gmail.com>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + * 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 <y.karadz@gmail.com>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + * 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);
^ permalink raw reply [flat|nested] 4+ messages in thread