Re: [PATCH 3/3] tracing: Consolidate trace() functions

From: Masami Hiramatsu
Date: Tue Feb 11 2020 - 22:37:24 EST

Next message: Eric Biggers: "Re: [PATCH v7 1/8] unicode: Add utf8_casefold_iter"
Previous message: Steven Rostedt: "Re: [PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events"
In reply to: Tom Zanussi: "[PATCH 3/3] tracing: Consolidate trace() functions"
Next in thread: Tom Zanussi: "[PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Tom,

On Mon, 10 Feb 2020 17:06:50 -0600
Tom Zanussi <zanussi@xxxxxxxxxx> wrote:

> Move the checking, buffer reserve and buffer commit code in
> synth_event_trace_start/end() into inline functions
> __synth_event_trace_start/end() so they can also be used by
> synth_event_trace() and synth_event_trace_array(), and then have all
> those functions use them.
>
> Also, change synth_event_trace_state.enabled to disabled so it only
> needs to be set if the event is disabled, which is not normally the
> case.

I have some comments :)

> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 99a02168599b..65b54d6a1422 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1791,6 +1791,60 @@ void synth_event_cmd_init(struct dynevent_cmd *cmd, char *buf, int maxlen)
> }
> EXPORT_SYMBOL_GPL(synth_event_cmd_init);
>
> +static inline int
> +__synth_event_trace_start(struct trace_event_file *file,
> + struct synth_event_trace_state *trace_state)
> +{
> + int entry_size, fields_size = 0;
> + int ret = 0;
> +
> + /*
> + * Normal event tracing doesn't get called at all unless the
> + * ENABLED bit is set (which attaches the probe thus allowing
> + * this code to be called, etc). Because this is called
> + * directly by the user, we don't have that but we still need
> + * to honor not logging when disabled. For the the iterated
> + * trace case, we save the enabed state upon start and just
> + * ignore the following data calls.
> + */
> + if (!(file->flags & EVENT_FILE_FL_ENABLED) ||
> + trace_trigger_soft_disabled(file)) {
> + trace_state->disabled = true;
> + ret = -ENOENT;

If the state.disabled is set, I think __synth_event_trace_start() might
be better to return 0 because all caller change the result.

> + goto out;
> + }
> +
> + trace_state->event = file->event_call->data;
> +
> + fields_size = trace_state->event->n_u64 * sizeof(u64);
> +
> + /*
> + * Avoid ring buffer recursion detection, as this event
> + * is being performed within another event.
> + */
> + trace_state->buffer = file->tr->array_buffer.buffer;
> + ring_buffer_nest_start(trace_state->buffer);
> +
> + entry_size = sizeof(*trace_state->entry) + fields_size;
> + trace_state->entry = trace_event_buffer_reserve(&trace_state->fbuffer,
> + file,
> + entry_size);
> + if (!trace_state->entry) {
> + ring_buffer_nest_end(trace_state->buffer);
> + ret = -EINVAL;
> + }
> +out:
> + return ret;
> +}
> +
> +static inline void
> +__synth_event_trace_end(struct synth_event_trace_state *trace_state)
> +{

Should we check trace_state->disabled here too?

> + trace_event_buffer_commit(&trace_state->fbuffer);
> +
> + ring_buffer_nest_end(trace_state->buffer);
> +}
> +
> /**
> * synth_event_trace - Trace a synthetic event
> * @file: The trace_event_file representing the synthetic event
> @@ -1812,69 +1866,38 @@ EXPORT_SYMBOL_GPL(synth_event_cmd_init);
> */
> int synth_event_trace(struct trace_event_file *file, unsigned int n_vals, ...)
> {
> - struct trace_event_buffer fbuffer;
> - struct synth_trace_event *entry;
> - struct trace_buffer *buffer;
> - struct synth_event *event;
> + struct synth_event_trace_state state;
> unsigned int i, n_u64;
> - int fields_size = 0;
> va_list args;
> - int ret = 0;
> -
> - /*
> - * Normal event generation doesn't get called at all unless
> - * the ENABLED bit is set (which attaches the probe thus
> - * allowing this code to be called, etc). Because this is
> - * called directly by the user, we don't have that but we
> - * still need to honor not logging when disabled.
> - */
> - if (!(file->flags & EVENT_FILE_FL_ENABLED) ||
> - trace_trigger_soft_disabled(file))
> - return 0;
> -
> - event = file->event_call->data;
> -
> - if (n_vals != event->n_fields)
> - return -EINVAL;
> -
> - fields_size = event->n_u64 * sizeof(u64);
> -
> - /*
> - * Avoid ring buffer recursion detection, as this event
> - * is being performed within another event.
> - */
> - buffer = file->tr->array_buffer.buffer;
> - ring_buffer_nest_start(buffer);
> + int ret;
>
> - entry = trace_event_buffer_reserve(&fbuffer, file,
> - sizeof(*entry) + fields_size);
> - if (!entry) {
> - ret = -EINVAL;
> - goto out;
> + ret = __synth_event_trace_start(file, &state);
> + if (ret) {

And here, we can check "if (ret || state.disabled)" then

> + if (ret == -ENOENT)
> + ret = 0; /* just disabled, not really an error */

We don't need this check.

> + return ret;
> }
>
> va_start(args, n_vals);
> - for (i = 0, n_u64 = 0; i < event->n_fields; i++) {
> + for (i = 0, n_u64 = 0; i < state.event->n_fields; i++) {
> u64 val;
>
> val = va_arg(args, u64);
>
> - if (event->fields[i]->is_string) {
> + if (state.event->fields[i]->is_string) {
> char *str_val = (char *)(long)val;
> - char *str_field = (char *)&entry->fields[n_u64];
> + char *str_field = (char *)&state.entry->fields[n_u64];
>
> strscpy(str_field, str_val, STR_VAR_LEN_MAX);
> n_u64 += STR_VAR_LEN_MAX / sizeof(u64);
> } else {
> - entry->fields[n_u64] = val;
> + state.entry->fields[n_u64] = val;
> n_u64++;
> }
> }
> va_end(args);
>
> - trace_event_buffer_commit(&fbuffer);
> -out:
> - ring_buffer_nest_end(buffer);
> + __synth_event_trace_end(&state);
>
> return ret;
> }
> @@ -1901,62 +1924,31 @@ EXPORT_SYMBOL_GPL(synth_event_trace);
> int synth_event_trace_array(struct trace_event_file *file, u64 *vals,
> unsigned int n_vals)
> {
> - struct trace_event_buffer fbuffer;
> - struct synth_trace_event *entry;
> - struct trace_buffer *buffer;
> - struct synth_event *event;
> + struct synth_event_trace_state state;
> unsigned int i, n_u64;
> - int fields_size = 0;
> - int ret = 0;
> -
> - /*
> - * Normal event generation doesn't get called at all unless
> - * the ENABLED bit is set (which attaches the probe thus
> - * allowing this code to be called, etc). Because this is
> - * called directly by the user, we don't have that but we
> - * still need to honor not logging when disabled.
> - */
> - if (!(file->flags & EVENT_FILE_FL_ENABLED) ||
> - trace_trigger_soft_disabled(file))
> - return 0;
> -
> - event = file->event_call->data;
> -
> - if (n_vals != event->n_fields)
> - return -EINVAL;
> -
> - fields_size = event->n_u64 * sizeof(u64);
> -
> - /*
> - * Avoid ring buffer recursion detection, as this event
> - * is being performed within another event.
> - */
> - buffer = file->tr->array_buffer.buffer;
> - ring_buffer_nest_start(buffer);
> + int ret;
>
> - entry = trace_event_buffer_reserve(&fbuffer, file,
> - sizeof(*entry) + fields_size);
> - if (!entry) {
> - ret = -EINVAL;
> - goto out;
> + ret = __synth_event_trace_start(file, &state);
> + if (ret) {
> + if (ret == -ENOENT)
> + ret = 0; /* just disabled, not really an error */

Ditto.

> + return ret;
> }
>
> - for (i = 0, n_u64 = 0; i < event->n_fields; i++) {
> - if (event->fields[i]->is_string) {
> + for (i = 0, n_u64 = 0; i < state.event->n_fields; i++) {
> + if (state.event->fields[i]->is_string) {
> char *str_val = (char *)(long)vals[i];
> - char *str_field = (char *)&entry->fields[n_u64];
> + char *str_field = (char *)&state.entry->fields[n_u64];
>
> strscpy(str_field, str_val, STR_VAR_LEN_MAX);
> n_u64 += STR_VAR_LEN_MAX / sizeof(u64);
> } else {
> - entry->fields[n_u64] = vals[i];
> + state.entry->fields[n_u64] = vals[i];
> n_u64++;
> }
> }
>
> - trace_event_buffer_commit(&fbuffer);
> -out:
> - ring_buffer_nest_end(buffer);
> + __synth_event_trace_end(&state);
>
> return ret;
> }
> @@ -1993,55 +1985,17 @@ EXPORT_SYMBOL_GPL(synth_event_trace_array);
> int synth_event_trace_start(struct trace_event_file *file,
> struct synth_event_trace_state *trace_state)
> {
> - struct synth_trace_event *entry;
> - int fields_size = 0;
> - int ret = 0;
> + int ret;
>
> - if (!trace_state) {
> - ret = -EINVAL;
> - goto out;
> - }
> + if (!trace_state)
> + return -EINVAL;
>
> memset(trace_state, '\0', sizeof(*trace_state));
>
> - /*
> - * Normal event tracing doesn't get called at all unless the
> - * ENABLED bit is set (which attaches the probe thus allowing
> - * this code to be called, etc). Because this is called
> - * directly by the user, we don't have that but we still need
> - * to honor not logging when disabled. For the the iterated
> - * trace case, we save the enabed state upon start and just
> - * ignore the following data calls.
> - */
> - if (!(file->flags & EVENT_FILE_FL_ENABLED) ||
> - trace_trigger_soft_disabled(file)) {
> - trace_state->enabled = false;
> - goto out;
> - }
> -
> - trace_state->enabled = true;
> + ret = __synth_event_trace_start(file, trace_state);
> + if (ret == -ENOENT)
> + ret = 0; /* just disabled, not really an error */

We can also skip this tweak.

Thank you,

>
> - trace_state->event = file->event_call->data;
> -
> - fields_size = trace_state->event->n_u64 * sizeof(u64);
> -
> - /*
> - * Avoid ring buffer recursion detection, as this event
> - * is being performed within another event.
> - */
> - trace_state->buffer = file->tr->array_buffer.buffer;
> - ring_buffer_nest_start(trace_state->buffer);
> -
> - entry = trace_event_buffer_reserve(&trace_state->fbuffer, file,
> - sizeof(*entry) + fields_size);
> - if (!entry) {
> - ring_buffer_nest_end(trace_state->buffer);
> - ret = -EINVAL;
> - goto out;
> - }
> -
> - trace_state->entry = entry;
> -out:
> return ret;
> }
> EXPORT_SYMBOL_GPL(synth_event_trace_start);
> @@ -2074,7 +2028,7 @@ static int __synth_event_add_val(const char *field_name, u64 val,
> trace_state->add_next = true;
> }
>
> - if (!trace_state->enabled)
> + if (trace_state->disabled)
> goto out;
>
> event = trace_state->event;
> @@ -2209,9 +2163,7 @@ int synth_event_trace_end(struct synth_event_trace_state *trace_state)
> if (!trace_state)
> return -EINVAL;
>
> - trace_event_buffer_commit(&trace_state->fbuffer);
> -
> - ring_buffer_nest_end(trace_state->buffer);
> + __synth_event_trace_end(trace_state);
>
> return 0;
> }
> --
> 2.14.1
>

--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>

Next message: Eric Biggers: "Re: [PATCH v7 1/8] unicode: Add utf8_casefold_iter"
Previous message: Steven Rostedt: "Re: [PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events"
In reply to: Tom Zanussi: "[PATCH 3/3] tracing: Consolidate trace() functions"
Next in thread: Tom Zanussi: "[PATCH 2/3] tracing: Don't return -EINVAL when tracing soft disabled synth events"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]