| Message ID | 20180919143657.19472-3-y.karadz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add infrastructure for plugins. | expand |
On Wed, 19 Sep 2018 17:36:52 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > Plugin event handlers are added to the Session context descriptor. > The handlers are used to execute plugin-specific action (callback > function) during the processing of the trace data. > > Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> > --- > kernel-shark-qt/src/libkshark.c | 21 +++++++++++++++++++++ > kernel-shark-qt/src/libkshark.h | 12 ++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/kernel-shark-qt/src/libkshark.c b/kernel-shark-qt/src/libkshark.c > index b4a76ae..6fb08b2 100644 > --- a/kernel-shark-qt/src/libkshark.c > +++ b/kernel-shark-qt/src/libkshark.c > @@ -33,6 +33,9 @@ static bool kshark_default_context(struct kshark_context **context) > if (!kshark_ctx) > return false; > > + kshark_ctx->event_handlers = NULL; > + kshark_ctx->plugins = NULL; > + > kshark_ctx->show_task_filter = tracecmd_filter_id_hash_alloc(); > kshark_ctx->hide_task_filter = tracecmd_filter_id_hash_alloc(); > > @@ -217,6 +220,12 @@ void kshark_free(struct kshark_context *kshark_ctx) > tracecmd_filter_id_hash_free(kshark_ctx->show_event_filter); > tracecmd_filter_id_hash_free(kshark_ctx->hide_event_filter); > > + if(kshark_ctx->plugins) { Need space between "if" and "(" > + kshark_handle_plugins(kshark_ctx, KSHARK_PLUGIN_CLOSE); > + kshark_free_plugin_list(kshark_ctx->plugins); > + kshark_free_event_handler_list(kshark_ctx->event_handlers); > + } > + > kshark_free_task_list(kshark_ctx); > > if (seq.buffer) > @@ -564,6 +573,7 @@ static void free_rec_list(struct rec_list **rec_list, int n_cpus, > static size_t get_records(struct kshark_context *kshark_ctx, > struct rec_list ***rec_list, enum rec_type type) > { > + struct kshark_event_handler *evt_handler; > struct event_filter *adv_filter; > struct kshark_task_list *task; > struct tep_record *rec; > @@ -608,6 +618,17 @@ static size_t get_records(struct kshark_context *kshark_ctx, > > entry = &temp_rec->entry; > kshark_set_entry_values(kshark_ctx, rec, entry); > + > + /* Execute all plugin-provided actions (if any). */ > + evt_handler = kshark_ctx->event_handlers; > + while ((evt_handler = find_event_handler(evt_handler, > + entry->event_id))) { > + evt_handler->event_func(kshark_ctx, rec, entry); > + > + if ((evt_handler = evt_handler->next)) Please break the above if, to remove the assignment. It's just easier to read. That is: event_handler = event_hanndler->next; if (event_handler) > + entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK; > + } > + > pid = entry->pid; > /* Apply event filtering. */ > ret = FILTER_MATCH; > diff --git a/kernel-shark-qt/src/libkshark.h b/kernel-shark-qt/src/libkshark.h > index fda133c..203c812 100644 > --- a/kernel-shark-qt/src/libkshark.h > +++ b/kernel-shark-qt/src/libkshark.h > @@ -29,6 +29,9 @@ extern "C" { > #include "event-parse.h" > #include "trace-filter-hash.h" > > +// KernelShark > +#include "libkshark-plugin.h" > + > /** > * Kernel Shark entry contains all information from one trace record needed > * in order to visualize the time-series of trace records. The part of the > @@ -124,6 +127,9 @@ struct kshark_context { > > /** List of Plugins. */ > struct kshark_plugin_list *plugins; > + > + /** List of Plugin Event handlers. */ > + struct kshark_event_handler *event_handlers; > }; > > bool kshark_instance(struct kshark_context **kshark_ctx); > @@ -160,6 +166,12 @@ enum kshark_filter_masks { > > /** Special mask used whene filtering events. */ > KS_EVENT_VIEW_FILTER_MASK = 1 << 2, > + > + /** > + * Use this mask to check if the content of the entry has been accessed > + * by a plugin-defined function. > + */ > + KS_PLUGIN_UNTOUCHED_MASK = 1 << 7 Why the jump to 7? -- Steve > }; > > /** Filter type identifier. */
On 20.09.2018 06:24, Steven Rostedt wrote: >> /** Special mask used whene filtering events. */ >> KS_EVENT_VIEW_FILTER_MASK = 1 << 2, >> + >> + /** >> + * Use this mask to check if the content of the entry has been accessed >> + * by a plugin-defined function. >> + */ >> + KS_PLUGIN_UNTOUCHED_MASK = 1 << 7 > Why the jump to 7? The "visible" field of the entry will use 8 bits. I know that it is uint16_t now, but my plan is to use 8 of its bits for the stream_id field. Currently we use the first 3 bits as different visibility flags and we have 4 unused bits available for adding more visibility flags in the future. Because the PLUGIN_UNTOUCHED flag has a different meaning I decided to place it at the very end. Is this a problem? Thanks! Yordan
On 20.09.2018 06:24, Steven Rostedt wrote: >> static size_t get_records(struct kshark_context *kshark_ctx, >> struct rec_list ***rec_list, enum rec_type type) >> { >> + struct kshark_event_handler *evt_handler; >> struct event_filter *adv_filter; >> struct kshark_task_list *task; >> struct tep_record *rec; >> @@ -608,6 +618,17 @@ static size_t get_records(struct kshark_context *kshark_ctx, >> >> entry = &temp_rec->entry; >> kshark_set_entry_values(kshark_ctx, rec, entry); >> + >> + /* Execute all plugin-provided actions (if any). */ >> + evt_handler = kshark_ctx->event_handlers; >> + while ((evt_handler = find_event_handler(evt_handler, >> + entry->event_id))) { >> + evt_handler->event_func(kshark_ctx, rec, entry); >> + >> + if ((evt_handler = evt_handler->next)) > Please break the above if, to remove the assignment. It's just easier > to read. That is: > > event_handler = event_hanndler->next; > if (event_handler) >> + entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK; >> + } >> + Actually this is a bug (typo). I meant to have evt_handler = evt_handler->next; if (!evt_handler) entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK; This way the KS_PLUGIN_UNTOUCHED flag gets unset only once and it is guaranteed that the value of the bit cannot be changed by the plugin callback function. Thanks! Yordan >> pid = entry->pid; >> /* Apply event filtering. */ >> ret = FILTER_MATCH;
On Thu, 20 Sep 2018 16:26:11 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > On 20.09.2018 06:24, Steven Rostedt wrote: > >> /** Special mask used whene filtering events. */ > >> KS_EVENT_VIEW_FILTER_MASK = 1 << 2, > >> + > >> + /** > >> + * Use this mask to check if the content of the entry has been accessed > >> + * by a plugin-defined function. > >> + */ > >> + KS_PLUGIN_UNTOUCHED_MASK = 1 << 7 > > Why the jump to 7? > > The "visible" field of the entry will use 8 bits. I know that it is > uint16_t now, but my plan is to use 8 of its bits for the stream_id field. > > Currently we use the first 3 bits as different visibility flags and we > have 4 unused bits available for adding more visibility flags in the > future. Because the PLUGIN_UNTOUCHED flag has a different meaning I > decided to place it at the very end. > > Is this a problem? No, it just seems out of place. Can you add a comment after the VIEW_FILTER_MASK stating that the next 4 bits are reserved for more VIEW bits. -- Steve
On Thu, 20 Sep 2018 16:29:00 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > On 20.09.2018 06:24, Steven Rostedt wrote: > >> static size_t get_records(struct kshark_context *kshark_ctx, > >> struct rec_list ***rec_list, enum rec_type type) > >> { > >> + struct kshark_event_handler *evt_handler; > >> struct event_filter *adv_filter; > >> struct kshark_task_list *task; > >> struct tep_record *rec; > >> @@ -608,6 +618,17 @@ static size_t get_records(struct kshark_context *kshark_ctx, > >> > >> entry = &temp_rec->entry; > >> kshark_set_entry_values(kshark_ctx, rec, entry); > >> + > >> + /* Execute all plugin-provided actions (if any). */ > >> + evt_handler = kshark_ctx->event_handlers; > >> + while ((evt_handler = find_event_handler(evt_handler, > >> + entry->event_id))) { > >> + evt_handler->event_func(kshark_ctx, rec, entry); > >> + > >> + if ((evt_handler = evt_handler->next)) > > Please break the above if, to remove the assignment. It's just easier > > to read. That is: > > > > event_handler = event_hanndler->next; > > if (event_handler) > >> + entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK; > >> + } > >> + > > Actually this is a bug (typo). I meant to have :-) That's actually one of the reasons the combination is looked down upon in the Linux kernel. It's more likely to hide bugs from reviewers. -- Steve > > evt_handler = evt_handler->next; > if (!evt_handler) > entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK; > > This way the KS_PLUGIN_UNTOUCHED flag gets unset only once and it is > guaranteed that the value of the bit cannot be changed by the plugin > callback function. >
diff --git a/kernel-shark-qt/src/libkshark.c b/kernel-shark-qt/src/libkshark.c index b4a76ae..6fb08b2 100644 --- a/kernel-shark-qt/src/libkshark.c +++ b/kernel-shark-qt/src/libkshark.c @@ -33,6 +33,9 @@ static bool kshark_default_context(struct kshark_context **context) if (!kshark_ctx) return false; + kshark_ctx->event_handlers = NULL; + kshark_ctx->plugins = NULL; + kshark_ctx->show_task_filter = tracecmd_filter_id_hash_alloc(); kshark_ctx->hide_task_filter = tracecmd_filter_id_hash_alloc(); @@ -217,6 +220,12 @@ void kshark_free(struct kshark_context *kshark_ctx) tracecmd_filter_id_hash_free(kshark_ctx->show_event_filter); tracecmd_filter_id_hash_free(kshark_ctx->hide_event_filter); + if(kshark_ctx->plugins) { + kshark_handle_plugins(kshark_ctx, KSHARK_PLUGIN_CLOSE); + kshark_free_plugin_list(kshark_ctx->plugins); + kshark_free_event_handler_list(kshark_ctx->event_handlers); + } + kshark_free_task_list(kshark_ctx); if (seq.buffer) @@ -564,6 +573,7 @@ static void free_rec_list(struct rec_list **rec_list, int n_cpus, static size_t get_records(struct kshark_context *kshark_ctx, struct rec_list ***rec_list, enum rec_type type) { + struct kshark_event_handler *evt_handler; struct event_filter *adv_filter; struct kshark_task_list *task; struct tep_record *rec; @@ -608,6 +618,17 @@ static size_t get_records(struct kshark_context *kshark_ctx, entry = &temp_rec->entry; kshark_set_entry_values(kshark_ctx, rec, entry); + + /* Execute all plugin-provided actions (if any). */ + evt_handler = kshark_ctx->event_handlers; + while ((evt_handler = find_event_handler(evt_handler, + entry->event_id))) { + evt_handler->event_func(kshark_ctx, rec, entry); + + if ((evt_handler = evt_handler->next)) + entry->visible &= ~KS_PLUGIN_UNTOUCHED_MASK; + } + pid = entry->pid; /* Apply event filtering. */ ret = FILTER_MATCH; diff --git a/kernel-shark-qt/src/libkshark.h b/kernel-shark-qt/src/libkshark.h index fda133c..203c812 100644 --- a/kernel-shark-qt/src/libkshark.h +++ b/kernel-shark-qt/src/libkshark.h @@ -29,6 +29,9 @@ extern "C" { #include "event-parse.h" #include "trace-filter-hash.h" +// KernelShark +#include "libkshark-plugin.h" + /** * Kernel Shark entry contains all information from one trace record needed * in order to visualize the time-series of trace records. The part of the @@ -124,6 +127,9 @@ struct kshark_context { /** List of Plugins. */ struct kshark_plugin_list *plugins; + + /** List of Plugin Event handlers. */ + struct kshark_event_handler *event_handlers; }; bool kshark_instance(struct kshark_context **kshark_ctx); @@ -160,6 +166,12 @@ enum kshark_filter_masks { /** Special mask used whene filtering events. */ KS_EVENT_VIEW_FILTER_MASK = 1 << 2, + + /** + * Use this mask to check if the content of the entry has been accessed + * by a plugin-defined function. + */ + KS_PLUGIN_UNTOUCHED_MASK = 1 << 7 }; /** Filter type identifier. */
Plugin event handlers are added to the Session context descriptor. The handlers are used to execute plugin-specific action (callback function) during the processing of the trace data. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- kernel-shark-qt/src/libkshark.c | 21 +++++++++++++++++++++ kernel-shark-qt/src/libkshark.h | 12 ++++++++++++ 2 files changed, 33 insertions(+)