Re: [RFC][PATCH 06/23 v4] tracing: Have function_graph use trace_seq_has_overflowed()

[Petr Mladek]

On Thu 2014-11-13 20:12:50, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@xxxxxxxxxxx>
> 
> Instead of doing individual checks all over the place that makes the code
> very messy. Just check trace_seq_has_overflowed() at the end or in
> strategic places.
> 
> This makes the code much cleaner and also helps with getting closer
> to removing the return values of trace_seq_printf() and friends.
> 
> Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> ---
>  kernel/trace/trace.h                 |   2 +-
>  kernel/trace/trace_functions_graph.c | 374 +++++++++++------------------------
>  2 files changed, 116 insertions(+), 260 deletions(-)
> 
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 19418221b302..c3a37e55ec8b 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -726,7 +726,7 @@ extern unsigned long trace_flags;
>  extern enum print_line_t
>  print_graph_function_flags(struct trace_iterator *iter, u32 flags);
>  extern void print_graph_headers_flags(struct seq_file *s, u32 flags);
> -extern enum print_line_t
> +extern void
>  trace_print_graph_duration(unsigned long long duration, struct trace_seq *s);
>  extern void graph_trace_open(struct trace_iterator *iter);
>  extern void graph_trace_close(struct trace_iterator *iter);
> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index f0a0c982cde3..477a7c65cf08 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
[...]
> @@ -682,94 +639,69 @@ get_return_for_leaf(struct trace_iterator *iter,
>  	return next;
>  }
>  
> -static int print_graph_abs_time(u64 t, struct trace_seq *s)
> +static void print_graph_abs_time(u64 t, struct trace_seq *s)
>  {
>  	unsigned long usecs_rem;
>  
>  	usecs_rem = do_div(t, NSEC_PER_SEC);
>  	usecs_rem /= 1000;
>  
> -	return trace_seq_printf(s, "%5lu.%06lu |  ",
> -			(unsigned long)t, usecs_rem);
> +	trace_seq_printf(s, "%5lu.%06lu |  ",
> +			 (unsigned long)t, usecs_rem);
>  }
>  
> -static enum print_line_t
> +static void
>  print_graph_irq(struct trace_iterator *iter, unsigned long addr,
>  		enum trace_type type, int cpu, pid_t pid, u32 flags)
>  {
> -	int ret;
>  	struct trace_seq *s = &iter->seq;
>  
>  	if (addr < (unsigned long)__irqentry_text_start ||
>  		addr >= (unsigned long)__irqentry_text_end)
> -		return TRACE_TYPE_UNHANDLED;
> +		return;

I was curious if the TRACE_TYPE_UNHANDLED return value has any special
effect. But it seems to be ignored. There are only two callers and
they take care only of the PARTIAL_LINE return value.

[...]

> @@ -953,62 +856,43 @@ print_graph_entry_nested(struct trace_iterator *iter,
>  	return TRACE_TYPE_NO_CONSUME;
>  }
>  
> -static enum print_line_t
> +static void
>  print_graph_prologue(struct trace_iterator *iter, struct trace_seq *s,
>  		     int type, unsigned long addr, u32 flags)
>  {
>  	struct fgraph_data *data = iter->private;
>  	struct trace_entry *ent = iter->ent;
>  	int cpu = iter->cpu;
> -	int ret;
>  
>  	/* Pid */
> -	if (verif_pid(s, ent->pid, cpu, data) == TRACE_TYPE_PARTIAL_LINE)
> -		return TRACE_TYPE_PARTIAL_LINE;
> +	verif_pid(s, ent->pid, cpu, data);
>  
> -	if (type) {
> +	if (type)
>  		/* Interrupt */
> -		ret = print_graph_irq(iter, addr, type, cpu, ent->pid, flags);
> -		if (ret == TRACE_TYPE_PARTIAL_LINE)
> -			return TRACE_TYPE_PARTIAL_LINE;
> -	}
> +		print_graph_irq(iter, addr, type, cpu, ent->pid, flags);
>  
>  	if (!(trace_flags & TRACE_ITER_CONTEXT_INFO))
> -		return 0;
> +		return;
>  
>  	/* Absolute time */
> -	if (flags & TRACE_GRAPH_PRINT_ABS_TIME) {
> -		ret = print_graph_abs_time(iter->ts, s);
> -		if (!ret)
> -			return TRACE_TYPE_PARTIAL_LINE;
> -	}
> +	if (flags & TRACE_GRAPH_PRINT_ABS_TIME)
> +		print_graph_abs_time(iter->ts, s);
>  
>  	/* Cpu */
> -	if (flags & TRACE_GRAPH_PRINT_CPU) {
> -		ret = print_graph_cpu(s, cpu);
> -		if (ret == TRACE_TYPE_PARTIAL_LINE)
> -			return TRACE_TYPE_PARTIAL_LINE;
> -	}
> +	if (flags & TRACE_GRAPH_PRINT_CPU)
> +		print_graph_cpu(s, cpu);
>  
>  	/* Proc */
>  	if (flags & TRACE_GRAPH_PRINT_PROC) {
> -		ret = print_graph_proc(s, ent->pid);
> -		if (ret == TRACE_TYPE_PARTIAL_LINE)
> -			return TRACE_TYPE_PARTIAL_LINE;
> -
> -		ret = trace_seq_puts(s, " | ");
> -		if (!ret)
> -			return TRACE_TYPE_PARTIAL_LINE;
> +		print_graph_proc(s, ent->pid);
> +		trace_seq_puts(s, " | ");
>  	}
>  
>  	/* Latency format */
> -	if (trace_flags & TRACE_ITER_LATENCY_FMT) {
> -		ret = print_graph_lat_fmt(s, ent);
> -		if (ret == TRACE_TYPE_PARTIAL_LINE)
> -			return TRACE_TYPE_PARTIAL_LINE;
> -	}
> +	if (trace_flags & TRACE_ITER_LATENCY_FMT)
> +		print_graph_lat_fmt(s, ent);
>  
> -	return 0;
> +	return;

This probably even fixed a bug. The function returned
TRACE_TYPE_PARTIAL_LINE (0) even when it did not print anything.

The simplification is really cool.

Reviewed-by: Petr Mladek <pmladek@xxxxxxx>

Best Regards,
Petr Mladek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/