On Mon, 11 Feb 2019 08:53:16 +0000
Yordan Karadzhov <ykaradzhov@xxxxxxxxxx> 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@xxxxxxxxxx>
>
>
> > /**
> >
|