* [PATCH 0/4] kernel-shark: Add sched_waking event for sched plugin processing @ 2019-02-11 1:18 Steven Rostedt 2019-02-11 1:18 ` [PATCH 1/4] kernel-shark: Remove testing of "success" field of wakeup events Steven Rostedt ` (3 more replies) 0 siblings, 4 replies; 8+ messages in thread From: Steven Rostedt @ 2019-02-11 1:18 UTC (permalink / raw) To: linux-trace-devel; +Cc: Yordan Karadzhov Hi Yordan, I was using kernel shark to examine vsock events, and noticed that when I only enabled sched_waking events, it did not give me the wake up boxes in the task plots. I decided to look at the code and modify it to handle it. When doing that, I noticed that the "success" field of the event was being used. This is an obsolete field that will hopefully someday be removed. So I added a patch to remove that. I also did not want to just add cut and paste code to add the sched_waking, and instead made helper functions to process the events all the same. And finally, I added the sched_waking code. Can you review these patches and if they are fine by you, give your "Reviewed-by" tag. Otherwise let me know if there's an issue that I should fix. Thanks! -- Steve Tag SHA1: 746c45bb20a4c860d36c48d279286c67202248a0 Head SHA1: 746c45bb20a4c860d36c48d279286c67202248a0 Steven Rostedt (VMware) (4): kernel-shark: Remove testing of "success" field of wakeup events kernel-shark: Consolidate duplicate code of the sched_wakeup events kernel-shark: Remove plugin_get_rec_wakeup_pid() kernel-shark: Add sched_waking event processing to sched_waking ---- kernel-shark/src/plugins/sched_events.c | 147 +++++++++++++++----------------- kernel-shark/src/plugins/sched_events.h | 12 ++- 2 files changed, 76 insertions(+), 83 deletions(-) ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/4] kernel-shark: Remove testing of "success" field of wakeup events 2019-02-11 1:18 [PATCH 0/4] kernel-shark: Add sched_waking event for sched plugin processing Steven Rostedt @ 2019-02-11 1:18 ` Steven Rostedt 2019-02-11 1:18 ` [PATCH 2/4] kernel-shark: Consolidate duplicate code of the sched_wakeup events Steven Rostedt ` (2 subsequent siblings) 3 siblings, 0 replies; 8+ messages in thread From: Steven Rostedt @ 2019-02-11 1:18 UTC (permalink / raw) To: linux-trace-devel; +Cc: Yordan Karadzhov From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> Since 2011 (or Linux version 3.0) the sched wakeup event "success" field has been hardcoded to true (or 1). There's no reason to waste cycles checking it in any newer kernel, which is most of them. Just ignore it. Even when running on older kernels which may have a success=0 value, the algorithm should still work. Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- kernel-shark/src/plugins/sched_events.c | 25 +++---------------------- kernel-shark/src/plugins/sched_events.h | 8 -------- 2 files changed, 3 insertions(+), 30 deletions(-) diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c index c0328e804911..5b1af7722624 100644 --- a/kernel-shark/src/plugins/sched_events.c +++ b/kernel-shark/src/plugins/sched_events.c @@ -68,9 +68,6 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx) plugin_ctx->sched_wakeup_pid_field = tep_find_any_field(event, "pid"); - plugin_ctx->sched_wakeup_success_field = - tep_find_field(event, "success"); - event = tep_find_event_by_name(plugin_ctx->pevent, "sched", "sched_wakeup_new"); if (!event) @@ -80,9 +77,6 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx) plugin_ctx->sched_wakeup_new_pid_field = tep_find_any_field(event, "pid"); - plugin_ctx->sched_wakeup_new_success_field = - tep_find_field(event, "success"); - plugin_ctx->second_pass_hash = tracecmd_filter_id_hash_alloc(); return true; @@ -175,8 +169,7 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx, { struct plugin_sched_context *plugin_ctx; struct tep_record *record = NULL; - unsigned long long val; - int ret, wakeup_pid = -1; + int wakeup_pid = -1; plugin_ctx = plugin_sched_context_handler; if (!plugin_ctx) @@ -185,25 +178,13 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx, if (plugin_ctx->sched_wakeup_event && e->event_id == plugin_ctx->sched_wakeup_event->id) { record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL); - - /* We only want those that actually woke up the task. */ - ret = tep_read_number_field(plugin_ctx->sched_wakeup_success_field, - record->data, &val); - - if (ret == 0 && val) - wakeup_pid = plugin_get_rec_wakeup_pid(record); + wakeup_pid = plugin_get_rec_wakeup_pid(record); } if (plugin_ctx->sched_wakeup_new_event && e->event_id == plugin_ctx->sched_wakeup_new_event->id) { record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL); - - /* We only want those that actually woke up the task. */ - ret = tep_read_number_field(plugin_ctx->sched_wakeup_new_success_field, - record->data, &val); - - if (ret == 0 && val) - wakeup_pid = plugin_get_rec_wakeup_new_pid(record); + wakeup_pid = plugin_get_rec_wakeup_new_pid(record); } free_record(record); diff --git a/kernel-shark/src/plugins/sched_events.h b/kernel-shark/src/plugins/sched_events.h index fb1849ee8ffb..0beb63fe2b48 100644 --- a/kernel-shark/src/plugins/sched_events.h +++ b/kernel-shark/src/plugins/sched_events.h @@ -45,20 +45,12 @@ struct plugin_sched_context { /** Pointer to the sched_wakeup_pid_field format descriptor. */ struct tep_format_field *sched_wakeup_pid_field; - /** Pointer to the sched_wakeup_success_field format descriptor. */ - struct tep_format_field *sched_wakeup_success_field; - /** Pointer to the sched_wakeup_new_event object. */ struct tep_event *sched_wakeup_new_event; /** Pointer to the sched_wakeup_new_pid_field format descriptor. */ struct tep_format_field *sched_wakeup_new_pid_field; - /** - * Pointer to the sched_wakeup_new_success_field format descriptor. - */ - struct tep_format_field *sched_wakeup_new_success_field; - /** List of Data collections used by this plugin. */ struct kshark_entry_collection *collections; -- 2.20.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/4] kernel-shark: Consolidate duplicate code of the sched_wakeup events 2019-02-11 1:18 [PATCH 0/4] kernel-shark: Add sched_waking event for sched plugin processing Steven Rostedt 2019-02-11 1:18 ` [PATCH 1/4] kernel-shark: Remove testing of "success" field of wakeup events Steven Rostedt @ 2019-02-11 1:18 ` Steven Rostedt 2019-02-11 8:53 ` Yordan Karadzhov 2019-02-11 1:18 ` [PATCH 3/4] kernel-shark: Remove plugin_get_rec_wakeup_pid() Steven Rostedt 2019-02-11 1:18 ` [PATCH 4/4] kernel-shark: Add sched_waking event processing to sched_waking Steven Rostedt 3 siblings, 1 reply; 8+ messages in thread From: Steven Rostedt @ 2019-02-11 1:18 UTC (permalink / raw) To: linux-trace-devel; +Cc: Yordan Karadzhov From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> The logic used for sched_wakeup and sched_wakeup_new events are basically the same. Create helper functions to be called to do the duplicate code with just passing in the parameters that are unique to the events. Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- kernel-shark/src/plugins/sched_events.c | 106 +++++++++++++++--------- 1 file changed, 66 insertions(+), 40 deletions(-) diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c index 5b1af7722624..0f5c08e3508d 100644 --- a/kernel-shark/src/plugins/sched_events.c +++ b/kernel-shark/src/plugins/sched_events.c @@ -23,10 +23,27 @@ /** Plugin context instance. */ struct plugin_sched_context *plugin_sched_context_handler = NULL; +static bool define_wakeup_event(struct tep_handle *tep, const char *wakeup_name, + struct tep_event **wakeup_event, + struct tep_format_field **pid_field) +{ + struct tep_event *event; + + event = tep_find_event_by_name(tep, "sched", wakeup_name); + if (!event) + return false; + + *wakeup_event = event; + *pid_field = tep_find_any_field(event, "pid"); + + return true; +} + static bool plugin_sched_init_context(struct kshark_context *kshark_ctx) { struct plugin_sched_context *plugin_ctx; struct tep_event *event; + bool wakeup_found; /* No context should exist when we initialize the plugin. */ assert(plugin_sched_context_handler == NULL); @@ -59,23 +76,17 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx) plugin_ctx->sched_switch_prev_state_field = tep_find_field(event, "prev_state"); - event = tep_find_event_by_name(plugin_ctx->pevent, - "sched", "sched_wakeup"); - if (!event) - return false; - plugin_ctx->sched_wakeup_event = event; - plugin_ctx->sched_wakeup_pid_field = - tep_find_any_field(event, "pid"); + wakeup_found = define_wakeup_event(kshark_ctx->pevent, "sched_wakeup", + &plugin_ctx->sched_wakeup_event, + &plugin_ctx->sched_wakeup_pid_field); - event = tep_find_event_by_name(plugin_ctx->pevent, - "sched", "sched_wakeup_new"); - if (!event) - return false; + wakeup_found |= define_wakeup_event(kshark_ctx->pevent, "sched_wakeup_new", + &plugin_ctx->sched_wakeup_new_event, + &plugin_ctx->sched_wakeup_new_pid_field); - plugin_ctx->sched_wakeup_new_event = event; - plugin_ctx->sched_wakeup_new_pid_field = - tep_find_any_field(event, "pid"); + if (!wakeup_found) + return false; plugin_ctx->second_pass_hash = tracecmd_filter_id_hash_alloc(); @@ -139,17 +150,51 @@ static void plugin_register_command(struct kshark_context *kshark_ctx, tep_register_comm(kshark_ctx->pevent, comm, pid); } -static int plugin_get_rec_wakeup_new_pid(struct tep_record *record) +int find_wakeup_pid(struct kshark_context *kshark_ctx, struct kshark_entry *e, + struct tep_event *wakeup_event, struct tep_format_field *pid_field) { - struct plugin_sched_context *plugin_ctx = - plugin_sched_context_handler; + struct tep_record *record; unsigned long long val; int ret; - ret = tep_read_number_field(plugin_ctx->sched_wakeup_new_pid_field, - record->data, &val); + if (!wakeup_event || e->event_id != wakeup_event->id) + return -1; - return ret ? : val; + record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL); + ret = tep_read_number_field(pid_field, record->data, &val); + free_record(record); + + if (ret) + return -1; + + return val; +} + +static bool wakeup_match_rec_pid(struct plugin_sched_context *plugin_ctx, + struct kshark_context *kshark_ctx, + struct kshark_entry *e, + int pid) +{ + struct tep_event *wakeup_events[] = { + plugin_ctx->sched_wakeup_event, + plugin_ctx->sched_wakeup_new_event, + }; + struct tep_format_field *wakeup_fields[] = { + plugin_ctx->sched_wakeup_pid_field, + plugin_ctx->sched_wakeup_new_pid_field, + }; + int i, wakeup_pid = -1; + + for (i = 0; i < sizeof(wakeup_events) / sizeof(wakeup_events[0]); i++) { + wakeup_pid = find_wakeup_pid(kshark_ctx, e, wakeup_events[i], wakeup_fields[i]); + if (wakeup_pid >= 0) + break; + } + + if (wakeup_pid >= 0 && wakeup_pid == pid) + return true; + + return false; } /** @@ -168,31 +213,12 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx, int pid) { struct plugin_sched_context *plugin_ctx; - struct tep_record *record = NULL; - int wakeup_pid = -1; plugin_ctx = plugin_sched_context_handler; if (!plugin_ctx) return false; - if (plugin_ctx->sched_wakeup_event && - e->event_id == plugin_ctx->sched_wakeup_event->id) { - record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL); - wakeup_pid = plugin_get_rec_wakeup_pid(record); - } - - if (plugin_ctx->sched_wakeup_new_event && - e->event_id == plugin_ctx->sched_wakeup_new_event->id) { - record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL); - wakeup_pid = plugin_get_rec_wakeup_new_pid(record); - } - - free_record(record); - - if (wakeup_pid >= 0 && wakeup_pid == pid) - return true; - - return false; + return wakeup_match_rec_pid(plugin_ctx, kshark_ctx, e, pid); } /** -- 2.20.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/4] kernel-shark: Consolidate duplicate code of the sched_wakeup events 2019-02-11 1:18 ` [PATCH 2/4] kernel-shark: Consolidate duplicate code of the sched_wakeup events Steven Rostedt @ 2019-02-11 8:53 ` Yordan Karadzhov 2019-02-11 14:44 ` Steven Rostedt 0 siblings, 1 reply; 8+ messages in thread From: Yordan Karadzhov @ 2019-02-11 8:53 UTC (permalink / raw) To: Steven Rostedt, linux-trace-devel On 11.02.19 г. 3:18 ч., Steven Rostedt wrote: > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> > > The logic used for sched_wakeup and sched_wakeup_new events are basically > the same. Create helper functions to be called to do the duplicate code with > just passing in the parameters that are unique to the events. > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > --- > kernel-shark/src/plugins/sched_events.c | 106 +++++++++++++++--------- > 1 file changed, 66 insertions(+), 40 deletions(-) > > diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c > index 5b1af7722624..0f5c08e3508d 100644 > --- a/kernel-shark/src/plugins/sched_events.c > +++ b/kernel-shark/src/plugins/sched_events.c > @@ -23,10 +23,27 @@ > /** Plugin context instance. */ > struct plugin_sched_context *plugin_sched_context_handler = NULL; > > +static bool define_wakeup_event(struct tep_handle *tep, const char *wakeup_name, > + struct tep_event **wakeup_event, > + struct tep_format_field **pid_field) > +{ > + struct tep_event *event; > + > + event = tep_find_event_by_name(tep, "sched", wakeup_name); > + if (!event) > + return false; > + > + *wakeup_event = event; > + *pid_field = tep_find_any_field(event, "pid"); > + > + return true; > +} > + > static bool plugin_sched_init_context(struct kshark_context *kshark_ctx) > { > struct plugin_sched_context *plugin_ctx; > struct tep_event *event; > + bool wakeup_found; > > /* No context should exist when we initialize the plugin. */ > assert(plugin_sched_context_handler == NULL); > @@ -59,23 +76,17 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx) > plugin_ctx->sched_switch_prev_state_field = > tep_find_field(event, "prev_state"); > > - event = tep_find_event_by_name(plugin_ctx->pevent, > - "sched", "sched_wakeup"); > - if (!event) > - return false; > > - plugin_ctx->sched_wakeup_event = event; > - plugin_ctx->sched_wakeup_pid_field = > - tep_find_any_field(event, "pid"); > + wakeup_found = define_wakeup_event(kshark_ctx->pevent, "sched_wakeup", > + &plugin_ctx->sched_wakeup_event, > + &plugin_ctx->sched_wakeup_pid_field); > > - event = tep_find_event_by_name(plugin_ctx->pevent, > - "sched", "sched_wakeup_new"); > - if (!event) > - return false; > + wakeup_found |= define_wakeup_event(kshark_ctx->pevent, "sched_wakeup_new", > + &plugin_ctx->sched_wakeup_new_event, > + &plugin_ctx->sched_wakeup_new_pid_field); > > - plugin_ctx->sched_wakeup_new_event = event; > - plugin_ctx->sched_wakeup_new_pid_field = > - tep_find_any_field(event, "pid"); > + if (!wakeup_found) > + return false; > > plugin_ctx->second_pass_hash = tracecmd_filter_id_hash_alloc(); > > @@ -139,17 +150,51 @@ static void plugin_register_command(struct kshark_context *kshark_ctx, > tep_register_comm(kshark_ctx->pevent, comm, pid); > } > > -static int plugin_get_rec_wakeup_new_pid(struct tep_record *record) This patch silently removes the "plugin_get_rec_wakeup_new_pid()" function which is the equivalent of "plugin_get_rec_wakeup_pid()", but for "wakeup_new" events. I guess, you are removing "plugin_get_rec_wakeup_new_pid()" here, because it is defined static but in fact there is no reason for "plugin_get_rec_wakeup_pid()" being non-static (this is my mistake). So just to keep everything consistent, please do the removal of "plugin_get_rec_wakeup_new_pid()" in the next patch (together with the removal of "plugin_get_rec_wakeup_pid()"). > +int find_wakeup_pid(struct kshark_context *kshark_ctx, struct kshark_entry *e, > + struct tep_event *wakeup_event, struct tep_format_field *pid_field) > { > - struct plugin_sched_context *plugin_ctx = > - plugin_sched_context_handler; > + struct tep_record *record; > unsigned long long val; > int ret; > > - ret = tep_read_number_field(plugin_ctx->sched_wakeup_new_pid_field, > - record->data, &val); > + if (!wakeup_event || e->event_id != wakeup_event->id) > + return -1; > > - return ret ? : val; > + record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL); > + ret = tep_read_number_field(pid_field, record->data, &val); > + free_record(record); > + > + if (ret) > + return -1; > + > + return val; > +} > + > +static bool wakeup_match_rec_pid(struct plugin_sched_context *plugin_ctx, > + struct kshark_context *kshark_ctx, > + struct kshark_entry *e, > + int pid) > +{ > + struct tep_event *wakeup_events[] = { > + plugin_ctx->sched_wakeup_event, > + plugin_ctx->sched_wakeup_new_event, > + }; > + struct tep_format_field *wakeup_fields[] = { > + plugin_ctx->sched_wakeup_pid_field, > + plugin_ctx->sched_wakeup_new_pid_field, > + }; > + int i, wakeup_pid = -1; > + > + for (i = 0; i < sizeof(wakeup_events) / sizeof(wakeup_events[0]); i++) { > + wakeup_pid = find_wakeup_pid(kshark_ctx, e, wakeup_events[i], wakeup_fields[i]); > + if (wakeup_pid >= 0) > + break; > + } > + > + if (wakeup_pid >= 0 && wakeup_pid == pid) > + return true; > + > + return false; > } > > /** > @@ -168,31 +213,12 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx, > int pid) > { > struct plugin_sched_context *plugin_ctx; > - struct tep_record *record = NULL; > - int wakeup_pid = -1; > > plugin_ctx = plugin_sched_context_handler; > if (!plugin_ctx) > return false; > > - if (plugin_ctx->sched_wakeup_event && > - e->event_id == plugin_ctx->sched_wakeup_event->id) { > - record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL); > - wakeup_pid = plugin_get_rec_wakeup_pid(record); > - } > - > - if (plugin_ctx->sched_wakeup_new_event && > - e->event_id == plugin_ctx->sched_wakeup_new_event->id) { > - record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL); > - wakeup_pid = plugin_get_rec_wakeup_new_pid(record); > - } > - > - free_record(record); > - > - if (wakeup_pid >= 0 && wakeup_pid == pid) > - return true; > - > - return false; > + return wakeup_match_rec_pid(plugin_ctx, kshark_ctx, e, pid); > } > I see no point in defining "static bool wakeup_match_rec_pid()" and then having "plugin_wakeup_match_rec_pid()" which is doing nothing but just calling this static function. Everything else in this patch series is OK. Thank you very much! Yordan Reviewed-by: Yordan Karadzhov <ykaradzhov@vmware.com> > /** > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/4] kernel-shark: Consolidate duplicate code of the sched_wakeup events 2019-02-11 8:53 ` Yordan Karadzhov @ 2019-02-11 14:44 ` Steven Rostedt 2019-02-12 17:09 ` Yordan Karadzhov 0 siblings, 1 reply; 8+ messages in thread From: Steven Rostedt @ 2019-02-11 14:44 UTC (permalink / raw) To: Yordan Karadzhov; +Cc: linux-trace-devel On Mon, 11 Feb 2019 08:53:16 +0000 Yordan Karadzhov <ykaradzhov@vmware.com> wrote: Hi Yordan, Thanks for reviewing, you pointed out things I expected you to ;-) > > @@ -139,17 +150,51 @@ static void plugin_register_command(struct kshark_context *kshark_ctx, > > tep_register_comm(kshark_ctx->pevent, comm, pid); > > } > > > > -static int plugin_get_rec_wakeup_new_pid(struct tep_record *record) > > This patch silently removes the "plugin_get_rec_wakeup_new_pid()" > function which is the equivalent of "plugin_get_rec_wakeup_pid()", but > for "wakeup_new" events. I guess, you are removing > "plugin_get_rec_wakeup_new_pid()" here, because it is defined static but > in fact there is no reason for "plugin_get_rec_wakeup_pid()" being > non-static (this is my mistake). So just to keep everything consistent, > please do the removal of "plugin_get_rec_wakeup_new_pid()" in the next > patch (together with the removal of "plugin_get_rec_wakeup_pid()"). > Actually, I didn't really silently remove it. Without removing it, gcc would complain that there's a static function not being used, and that's a no-no for a patch to cause a new warning. It's fine to remove static functions when removing their users in the code that is being modified. Now, I would have also removed plugin_get_rec_wakeup_pid(), but because that's a global function, it doesn't cause the warning, and may also be possibly used outside of that file (and by code I'm not working with). That's why I removed the static one here. It's required because without doing so, it causes gcc to generate a warning. And the reason for removing the other function was because it is global outside of the code that I changed, which requires a separate patch. Now if both were global, I would be able to remove them together in a separate patch, or if both were static, I would have removed both here. But since one's global and one's static, they need to be removed in separate places. Make sense? > > > +int find_wakeup_pid(struct kshark_context *kshark_ctx, struct kshark_entry *e, > > + struct tep_event *wakeup_event, struct tep_format_field *pid_field) > > { > > - struct plugin_sched_context *plugin_ctx = > > - plugin_sched_context_handler; > > + struct tep_record *record; > > unsigned long long val; > > int ret; > > > > - ret = tep_read_number_field(plugin_ctx->sched_wakeup_new_pid_field, > > - record->data, &val); > > + if (!wakeup_event || e->event_id != wakeup_event->id) > > + return -1; > > > > - return ret ? : val; > > + record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL); > > + ret = tep_read_number_field(pid_field, record->data, &val); > > + free_record(record); > > + > > + if (ret) > > + return -1; > > + > > + return val; > > +} > > + > > +static bool wakeup_match_rec_pid(struct plugin_sched_context *plugin_ctx, > > + struct kshark_context *kshark_ctx, > > + struct kshark_entry *e, > > + int pid) > > +{ > > + struct tep_event *wakeup_events[] = { > > + plugin_ctx->sched_wakeup_event, > > + plugin_ctx->sched_wakeup_new_event, > > + }; > > + struct tep_format_field *wakeup_fields[] = { > > + plugin_ctx->sched_wakeup_pid_field, > > + plugin_ctx->sched_wakeup_new_pid_field, > > + }; > > + int i, wakeup_pid = -1; > > + > > + for (i = 0; i < sizeof(wakeup_events) / sizeof(wakeup_events[0]); i++) { > > + wakeup_pid = find_wakeup_pid(kshark_ctx, e, wakeup_events[i], wakeup_fields[i]); > > + if (wakeup_pid >= 0) > > + break; > > + } > > + > > + if (wakeup_pid >= 0 && wakeup_pid == pid) > > + return true; > > + > > + return false; > > } > > > > /** > > @@ -168,31 +213,12 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx, > > int pid) > > { > > struct plugin_sched_context *plugin_ctx; > > - struct tep_record *record = NULL; > > - int wakeup_pid = -1; > > > > plugin_ctx = plugin_sched_context_handler; > > if (!plugin_ctx) > > return false; > > > > - if (plugin_ctx->sched_wakeup_event && > > - e->event_id == plugin_ctx->sched_wakeup_event->id) { > > - record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL); > > - wakeup_pid = plugin_get_rec_wakeup_pid(record); > > - } > > - > > - if (plugin_ctx->sched_wakeup_new_event && > > - e->event_id == plugin_ctx->sched_wakeup_new_event->id) { > > - record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL); > > - wakeup_pid = plugin_get_rec_wakeup_new_pid(record); > > - } > > - > > - free_record(record); > > - > > - if (wakeup_pid >= 0 && wakeup_pid == pid) > > - return true; > > - > > - return false; > > + return wakeup_match_rec_pid(plugin_ctx, kshark_ctx, e, pid); > > } > > > > I see no point in defining "static bool wakeup_match_rec_pid()" and then > having "plugin_wakeup_match_rec_pid()" which is doing nothing but just > calling this static function. It's because of the initialization of wakeup_events and wakeup_fields. struct tep_event *wakeup_events[] = { plugin_ctx->sched_waking_event, plugin_ctx->sched_wakeup_event, plugin_ctx->sched_wakeup_new_event, }; struct tep_format_field *wakeup_fields[] = { plugin_ctx->sched_waking_pid_field, plugin_ctx->sched_wakeup_pid_field, plugin_ctx->sched_wakeup_new_pid_field, }; Which are initialized by the values in plugin_ctx, but plugin_ctx is not initialized until after the static is set up: plugin_ctx = plugin_sched_context_handler; if (!plugin_ctx) return false; Thus to handle this case, I simply moved the code into a helper function, and had the main function just initialize plugin_ctx (return if NULL), and then call the helper function that can initialize the arrays on the stack. Make sense? -- Steve > > Everything else in this patch series is OK. > > Thank you very much! > Yordan > > > Reviewed-by: Yordan Karadzhov <ykaradzhov@vmware.com> > > > > /** > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/4] kernel-shark: Consolidate duplicate code of the sched_wakeup events 2019-02-11 14:44 ` Steven Rostedt @ 2019-02-12 17:09 ` Yordan Karadzhov 0 siblings, 0 replies; 8+ messages in thread From: Yordan Karadzhov @ 2019-02-12 17:09 UTC (permalink / raw) To: Steven Rostedt, Yordan Karadzhov; +Cc: linux-trace-devel On 11.02.19 г. 16:44 ч., Steven Rostedt wrote: > On Mon, 11 Feb 2019 08:53:16 +0000 > Yordan Karadzhov <ykaradzhov@vmware.com> wrote: > > Hi Yordan, > > Thanks for reviewing, you pointed out things I expected you to ;-) > >>> @@ -139,17 +150,51 @@ static void plugin_register_command(struct kshark_context *kshark_ctx, >>> tep_register_comm(kshark_ctx->pevent, comm, pid); >>> } >>> >>> -static int plugin_get_rec_wakeup_new_pid(struct tep_record *record) >> >> This patch silently removes the "plugin_get_rec_wakeup_new_pid()" >> function which is the equivalent of "plugin_get_rec_wakeup_pid()", but >> for "wakeup_new" events. I guess, you are removing >> "plugin_get_rec_wakeup_new_pid()" here, because it is defined static but >> in fact there is no reason for "plugin_get_rec_wakeup_pid()" being >> non-static (this is my mistake). So just to keep everything consistent, >> please do the removal of "plugin_get_rec_wakeup_new_pid()" in the next >> patch (together with the removal of "plugin_get_rec_wakeup_pid()"). >> > > Actually, I didn't really silently remove it. Without removing it, gcc > would complain that there's a static function not being used, and > that's a no-no for a patch to cause a new warning. It's fine to remove > static functions when removing their users in the code that is being > modified. > > Now, I would have also removed plugin_get_rec_wakeup_pid(), but because > that's a global function, it doesn't cause the warning, and may also be > possibly used outside of that file (and by code I'm not working with). > > That's why I removed the static one here. It's required because without > doing so, it causes gcc to generate a warning. And the reason for > removing the other function was because it is global outside of the > code that I changed, which requires a separate patch. Now if both were > global, I would be able to remove them together in a separate patch, or > if both were static, I would have removed both here. But since one's > global and one's static, they need to be removed in separate places. > > Make sense? Yes, this makes sense. > >> >>> +int find_wakeup_pid(struct kshark_context *kshark_ctx, struct kshark_entry *e, >>> + struct tep_event *wakeup_event, struct tep_format_field *pid_field) >>> { >>> - struct plugin_sched_context *plugin_ctx = >>> - plugin_sched_context_handler; >>> + struct tep_record *record; >>> unsigned long long val; >>> int ret; >>> >>> - ret = tep_read_number_field(plugin_ctx->sched_wakeup_new_pid_field, >>> - record->data, &val); >>> + if (!wakeup_event || e->event_id != wakeup_event->id) >>> + return -1; >>> >>> - return ret ? : val; >>> + record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL); >>> + ret = tep_read_number_field(pid_field, record->data, &val); >>> + free_record(record); >>> + >>> + if (ret) >>> + return -1; >>> + >>> + return val; >>> +} >>> + >>> +static bool wakeup_match_rec_pid(struct plugin_sched_context *plugin_ctx, >>> + struct kshark_context *kshark_ctx, >>> + struct kshark_entry *e, >>> + int pid) >>> +{ >>> + struct tep_event *wakeup_events[] = { >>> + plugin_ctx->sched_wakeup_event, >>> + plugin_ctx->sched_wakeup_new_event, >>> + }; >>> + struct tep_format_field *wakeup_fields[] = { >>> + plugin_ctx->sched_wakeup_pid_field, >>> + plugin_ctx->sched_wakeup_new_pid_field, >>> + }; >>> + int i, wakeup_pid = -1; >>> + >>> + for (i = 0; i < sizeof(wakeup_events) / sizeof(wakeup_events[0]); i++) { >>> + wakeup_pid = find_wakeup_pid(kshark_ctx, e, wakeup_events[i], wakeup_fields[i]); >>> + if (wakeup_pid >= 0) >>> + break; >>> + } >>> + >>> + if (wakeup_pid >= 0 && wakeup_pid == pid) >>> + return true; >>> + >>> + return false; >>> } >>> >>> /** >>> @@ -168,31 +213,12 @@ bool plugin_wakeup_match_rec_pid(struct kshark_context *kshark_ctx, >>> int pid) >>> { >>> struct plugin_sched_context *plugin_ctx; >>> - struct tep_record *record = NULL; >>> - int wakeup_pid = -1; >>> >>> plugin_ctx = plugin_sched_context_handler; >>> if (!plugin_ctx) >>> return false; >>> >>> - if (plugin_ctx->sched_wakeup_event && >>> - e->event_id == plugin_ctx->sched_wakeup_event->id) { >>> - record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL); >>> - wakeup_pid = plugin_get_rec_wakeup_pid(record); >>> - } >>> - >>> - if (plugin_ctx->sched_wakeup_new_event && >>> - e->event_id == plugin_ctx->sched_wakeup_new_event->id) { >>> - record = tracecmd_read_at(kshark_ctx->handle, e->offset, NULL); >>> - wakeup_pid = plugin_get_rec_wakeup_new_pid(record); >>> - } >>> - >>> - free_record(record); >>> - >>> - if (wakeup_pid >= 0 && wakeup_pid == pid) >>> - return true; >>> - >>> - return false; >>> + return wakeup_match_rec_pid(plugin_ctx, kshark_ctx, e, pid); >>> } >>> >> >> I see no point in defining "static bool wakeup_match_rec_pid()" and then >> having "plugin_wakeup_match_rec_pid()" which is doing nothing but just >> calling this static function. > > It's because of the initialization of wakeup_events and wakeup_fields. > > struct tep_event *wakeup_events[] = { > plugin_ctx->sched_waking_event, > plugin_ctx->sched_wakeup_event, > plugin_ctx->sched_wakeup_new_event, > }; > struct tep_format_field *wakeup_fields[] = { > plugin_ctx->sched_waking_pid_field, > plugin_ctx->sched_wakeup_pid_field, > plugin_ctx->sched_wakeup_new_pid_field, > }; > > Which are initialized by the values in plugin_ctx, but plugin_ctx is > not initialized until after the static is set up: > > plugin_ctx = plugin_sched_context_handler; > if (!plugin_ctx) > return false; > > Thus to handle this case, I simply moved the code into a helper > function, and had the main function just initialize plugin_ctx (return > if NULL), and then call the helper function that can initialize the > arrays on the stack. > > Make sense? > This is OK as well. Thank you very much! Yordan > -- Steve > > >> >> Everything else in this patch series is OK. >> >> Thank you very much! >> Yordan >> >> >> Reviewed-by: Yordan Karadzhov <ykaradzhov@vmware.com> >> >> >>> /** >>> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/4] kernel-shark: Remove plugin_get_rec_wakeup_pid() 2019-02-11 1:18 [PATCH 0/4] kernel-shark: Add sched_waking event for sched plugin processing Steven Rostedt 2019-02-11 1:18 ` [PATCH 1/4] kernel-shark: Remove testing of "success" field of wakeup events Steven Rostedt 2019-02-11 1:18 ` [PATCH 2/4] kernel-shark: Consolidate duplicate code of the sched_wakeup events Steven Rostedt @ 2019-02-11 1:18 ` Steven Rostedt 2019-02-11 1:18 ` [PATCH 4/4] kernel-shark: Add sched_waking event processing to sched_waking Steven Rostedt 3 siblings, 0 replies; 8+ messages in thread From: Steven Rostedt @ 2019-02-11 1:18 UTC (permalink / raw) To: linux-trace-devel; +Cc: Yordan Karadzhov From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> The function plugin_get_rec_wakeup_pid() is not used by anything. Remove it. Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- kernel-shark/src/plugins/sched_events.c | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c index 0f5c08e3508d..0cfe5b5367e2 100644 --- a/kernel-shark/src/plugins/sched_events.c +++ b/kernel-shark/src/plugins/sched_events.c @@ -111,24 +111,6 @@ int plugin_get_next_pid(struct tep_record *record) return ret ? : val; } -/** - * @brief Get the Process Id of the task being woke up. - * - * @param record: Input location for a sched_wakeup record. - */ -int plugin_get_rec_wakeup_pid(struct tep_record *record) -{ - struct plugin_sched_context *plugin_ctx = - plugin_sched_context_handler; - unsigned long long val; - int ret; - - ret = tep_read_number_field(plugin_ctx->sched_wakeup_pid_field, - record->data, &val); - - return ret ? : val; -} - static void plugin_register_command(struct kshark_context *kshark_ctx, struct tep_record *record, int pid) -- 2.20.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 4/4] kernel-shark: Add sched_waking event processing to sched_waking 2019-02-11 1:18 [PATCH 0/4] kernel-shark: Add sched_waking event for sched plugin processing Steven Rostedt ` (2 preceding siblings ...) 2019-02-11 1:18 ` [PATCH 3/4] kernel-shark: Remove plugin_get_rec_wakeup_pid() Steven Rostedt @ 2019-02-11 1:18 ` Steven Rostedt 3 siblings, 0 replies; 8+ messages in thread From: Steven Rostedt @ 2019-02-11 1:18 UTC (permalink / raw) To: linux-trace-devel; +Cc: Yordan Karadzhov From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> It is now more common to enable the sched_waking event than to use the sched_wakeup or sched_wakeup_new, as it encompasses both. But the sched plugin does not handle that. Fix that by including the sched_waking event as an event to use for processing schedule wakeup events. Signen-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- kernel-shark/src/plugins/sched_events.c | 6 ++++++ kernel-shark/src/plugins/sched_events.h | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c index 0cfe5b5367e2..14f8edb2836b 100644 --- a/kernel-shark/src/plugins/sched_events.c +++ b/kernel-shark/src/plugins/sched_events.c @@ -85,6 +85,10 @@ static bool plugin_sched_init_context(struct kshark_context *kshark_ctx) &plugin_ctx->sched_wakeup_new_event, &plugin_ctx->sched_wakeup_new_pid_field); + wakeup_found |= define_wakeup_event(kshark_ctx->pevent, "sched_waking", + &plugin_ctx->sched_waking_event, + &plugin_ctx->sched_waking_pid_field); + if (!wakeup_found) return false; @@ -158,10 +162,12 @@ static bool wakeup_match_rec_pid(struct plugin_sched_context *plugin_ctx, int pid) { struct tep_event *wakeup_events[] = { + plugin_ctx->sched_waking_event, plugin_ctx->sched_wakeup_event, plugin_ctx->sched_wakeup_new_event, }; struct tep_format_field *wakeup_fields[] = { + plugin_ctx->sched_waking_pid_field, plugin_ctx->sched_wakeup_pid_field, plugin_ctx->sched_wakeup_new_pid_field, }; diff --git a/kernel-shark/src/plugins/sched_events.h b/kernel-shark/src/plugins/sched_events.h index 0beb63fe2b48..dbc9963cbc4e 100644 --- a/kernel-shark/src/plugins/sched_events.h +++ b/kernel-shark/src/plugins/sched_events.h @@ -51,6 +51,12 @@ struct plugin_sched_context { /** Pointer to the sched_wakeup_new_pid_field format descriptor. */ struct tep_format_field *sched_wakeup_new_pid_field; + /** Pointer to the sched_waking_event object. */ + struct tep_event *sched_waking_event; + + /** Pointer to the sched_waking_pid_field format descriptor. */ + struct tep_format_field *sched_waking_pid_field; + /** List of Data collections used by this plugin. */ struct kshark_entry_collection *collections; -- 2.20.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-02-12 17:09 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-11 1:18 [PATCH 0/4] kernel-shark: Add sched_waking event for sched plugin processing Steven Rostedt 2019-02-11 1:18 ` [PATCH 1/4] kernel-shark: Remove testing of "success" field of wakeup events Steven Rostedt 2019-02-11 1:18 ` [PATCH 2/4] kernel-shark: Consolidate duplicate code of the sched_wakeup events Steven Rostedt 2019-02-11 8:53 ` Yordan Karadzhov 2019-02-11 14:44 ` Steven Rostedt 2019-02-12 17:09 ` Yordan Karadzhov 2019-02-11 1:18 ` [PATCH 3/4] kernel-shark: Remove plugin_get_rec_wakeup_pid() Steven Rostedt 2019-02-11 1:18 ` [PATCH 4/4] kernel-shark: Add sched_waking event processing to sched_waking Steven Rostedt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).