lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Wed, 1 Nov 2017 11:19:07 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     "Yordan Karadzhov (VMware)" <y.karadz@...il.com>
Cc:     jan.kiszka@...mens.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] kernelshark: Adding a GUI plugin for xenomai events

On Wed,  1 Nov 2017 13:04:23 +0200
"Yordan Karadzhov (VMware)" <y.karadz@...il.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@...il.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@...il.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@...il.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);

Powered by blists - more mailing lists