lkml.org 
[lkml]   [2018]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v7 05/16] tracing: Generalize hist trigger onmax and save action
    On Wed, Nov 14, 2018 at 02:18:02PM -0600, Tom Zanussi wrote:
    > From: Tom Zanussi <tom.zanussi@linux.intel.com>
    >
    > The action refactor code allowed actions and handlers to be separated,
    > but the existing onmax handler and save action code is still not
    > flexible enough to handle arbitrary coupling. This change generalizes
    > them and in the process makes additional handlers and actions easier
    > to implement.
    >
    > The onmax action can be broken up and thought of as two separate
    > components - a variable to be tracked (the parameter given to the
    > onmax($var_to_track) function) and an invisible variable created to
    > save the ongoing result of doing something with that variable, such as
    > saving the max value of that variable so far seen.
    >
    > Separating it out like this and renaming it appropriately allows us to
    > use the same code for similar tracking functions such as
    > onchange($var_to_track), which would just track the last value seen
    > rather than the max seen so far, which is useful in some situations.
    >
    > Additionally, because different handlers and actions may want to save
    > and access data differently e.g. save and retrieve tracking values as
    > local variables vs something more global, save_val() and get_val()
    > interface functions are introduced and max-specific implementations
    > are used instead.
    >
    > The same goes for the code that checks whether a maximum has been hit
    > - a generic check_val() interface and max-checking implementation is
    > used instead, which allows future patches to make use of he same code
    > using their own implemetations of similar functionality.
    >
    > Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
    > ---
    > kernel/trace/trace_events_hist.c | 225 ++++++++++++++++++++++++++-------------
    > 1 file changed, 151 insertions(+), 74 deletions(-)
    >
    > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
    > index 54b78cfe2766..ac48ad1482c8 100644
    > --- a/kernel/trace/trace_events_hist.c
    > +++ b/kernel/trace/trace_events_hist.c
    > @@ -319,6 +319,15 @@ typedef void (*action_fn_t) (struct hist_trigger_data *hist_data,
    > struct ring_buffer_event *rbe,
    > struct action_data *data, u64 *var_ref_vals);
    >
    > +typedef bool (*check_track_val_fn_t) (u64 track_val, u64 var_val);
    > +typedef bool (*save_track_val_fn_t) (struct hist_trigger_data *hist_data,
    > + struct tracing_map_elt *elt,
    > + struct action_data *data,
    > + unsigned int track_var_idx, u64 var_val);
    > +typedef u64 (*get_track_val_fn_t) (struct hist_trigger_data *hist_data,
    > + struct tracing_map_elt *elt,
    > + struct action_data *data);
    > +
    > enum handler_id {
    > HANDLER_ONMATCH = 1,
    > HANDLER_ONMAX,
    > @@ -349,14 +358,18 @@ struct action_data {
    >
    > struct {
    > char *var_str;
    > - unsigned int max_var_ref_idx;
    > - struct hist_field *max_var;
    > - struct hist_field *var;
    > - } onmax;
    > + struct hist_field *var_ref;
    > + unsigned int var_ref_idx;
    > +
    > + struct hist_field *track_var;
    > +
    > + check_track_val_fn_t check_val;
    > + save_track_val_fn_t save_val;
    > + get_track_val_fn_t get_val;
    > + } track_data;
    > };
    > };
    >
    > -
    > static char last_hist_cmd[MAX_FILTER_STR_VAL];
    > static char hist_err_str[MAX_FILTER_STR_VAL];
    >
    > @@ -3133,10 +3146,10 @@ static void update_field_vars(struct hist_trigger_data *hist_data,
    > hist_data->n_field_vars, 0);
    > }
    >
    > -static void update_max_vars(struct hist_trigger_data *hist_data,
    > - struct tracing_map_elt *elt,
    > - struct ring_buffer_event *rbe,
    > - void *rec)
    > +static void update_save_vars(struct hist_trigger_data *hist_data,
    > + struct tracing_map_elt *elt,
    > + struct ring_buffer_event *rbe,
    > + void *rec)
    > {
    > __update_field_vars(elt, rbe, rec, hist_data->save_vars,
    > hist_data->n_save_vars, hist_data->n_field_var_str);
    > @@ -3274,14 +3287,68 @@ create_target_field_var(struct hist_trigger_data *target_hist_data,
    > return create_field_var(target_hist_data, file, var_name);
    > }
    >
    > -static void onmax_print(struct seq_file *m,
    > - struct hist_trigger_data *hist_data,
    > - struct tracing_map_elt *elt,
    > - struct action_data *data)
    > +static bool check_track_val_max(u64 track_val, u64 var_val)
    > {
    > - unsigned int i, save_var_idx, max_idx = data->onmax.max_var->var.idx;
    > + if (var_val <= track_val)
    > + return false;
    > +
    > + return true;
    > +}
    >
    > - seq_printf(m, "\n\tmax: %10llu", tracing_map_read_var(elt, max_idx));
    > +static u64 get_track_val_local(struct hist_trigger_data *hist_data,
    > + struct tracing_map_elt *elt,
    > + struct action_data *data)
    > +{
    > + unsigned int track_var_idx = data->track_data.track_var->var.idx;
    > + u64 track_val;
    > +
    > + track_val = tracing_map_read_var(elt, track_var_idx);
    > +
    > + return track_val;
    > +}
    > +
    > +static bool save_track_val_local(struct hist_trigger_data *hist_data,
    > + struct tracing_map_elt *elt,
    > + struct action_data *data,
    > + unsigned int track_var_idx, u64 var_val)
    > +{
    > + bool ret = false;
    > + u64 track_val;
    > +
    > + track_val = tracing_map_read_var(elt, track_var_idx);
    > +
    > + if (data->track_data.check_val(track_val, var_val)) {
    > + tracing_map_set_var(elt, track_var_idx, var_val);
    > + ret = true;
    > + }
    > +
    > + return ret;
    > +}
    > +
    > +static bool update_track_val(struct hist_trigger_data *hist_data,
    > + struct tracing_map_elt *elt,
    > + struct action_data *data, u64 *var_ref_vals)
    > +{
    > + unsigned int track_var_idx = data->track_data.track_var->var.idx;
    > + unsigned int track_var_ref_idx = data->track_data.var_ref_idx;
    > + u64 var_val;
    > +
    > + var_val = var_ref_vals[track_var_ref_idx];
    > +
    > + return data->track_data.save_val(hist_data, elt, data,
    > + track_var_idx, var_val);
    > +}

    It's bit confusing for me update_track_val() calls ->save_val() and it
    in turn calls ->check_val(). Why not having wrappers corresponding to
    their callback name? - get_track_val(), check_track_val() and
    save_trace_val()...


    > +
    > +static void track_data_print(struct seq_file *m,
    > + struct hist_trigger_data *hist_data,
    > + struct tracing_map_elt *elt,
    > + struct action_data *data)
    > +{
    > + u64 track_val = data->track_data.get_val(hist_data, elt, data);
    > + unsigned int i, save_var_idx;
    > +
    > + if (data->handler == HANDLER_ONMAX)
    > + seq_printf(m, "\n\tmax: %10llu", track_val);
    >
    > for (i = 0; i < hist_data->n_save_vars; i++) {
    > struct hist_field *save_val = hist_data->save_vars[i]->val;
    > @@ -3300,25 +3367,13 @@ static void onmax_print(struct seq_file *m,
    > }
    > }
    >
    > -static void onmax_save(struct hist_trigger_data *hist_data,
    > - struct tracing_map_elt *elt, void *rec,
    > - struct ring_buffer_event *rbe,
    > - struct action_data *data, u64 *var_ref_vals)
    > +static void ontrack_save(struct hist_trigger_data *hist_data,
    > + struct tracing_map_elt *elt, void *rec,
    > + struct ring_buffer_event *rbe,
    > + struct action_data *data, u64 *var_ref_vals)
    > {
    > - unsigned int max_idx = data->onmax.max_var->var.idx;
    > - unsigned int max_var_ref_idx = data->onmax.max_var_ref_idx;
    > -
    > - u64 var_val, max_val;
    > -
    > - var_val = var_ref_vals[max_var_ref_idx];
    > - max_val = tracing_map_read_var(elt, max_idx);
    > -
    > - if (var_val <= max_val)
    > - return;
    > -
    > - tracing_map_set_var(elt, max_idx, var_val);
    > -
    > - update_max_vars(hist_data, elt, rbe, rec);
    > + if (update_track_val(hist_data, elt, data, var_ref_vals))
    > + update_save_vars(hist_data, elt, rbe, rec);

    ... and then it should look like:

    if (check_track_val()) {
    save_track_val();
    update_save_vars();
    }

    I also think update_save_vars() also needs to be renamed something
    like save_track_vars() or save_trace_data().

    Thanks,
    Namhyung


    > }

    \
     
     \ /
      Last update: 2018-11-23 08:01    [W:4.098 / U:0.252 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site