Date: Fri, 14 Nov 2014 06:58:09 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: Petr Mladek <pmladek@suse.cz>
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Jiri Kosina <jkosina@suse.cz>
Subject: Re: [RFC][PATCH 02/23 v4] tracing: Add trace_seq_has_overflowed()
 and trace_handle_return()
Message-ID: <20141114065809.17168a1a@gandalf.local.home>
In-Reply-To: <20141114112522.GA2988@dhcp128.suse.cz>
References: <20141114011244.256115061@goodmis.org>
	<20141114011410.365183157@goodmis.org>
	<20141114112522.GA2988@dhcp128.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org

On Fri, 14 Nov 2014 12:25:23 +0100
Petr Mladek <pmladek@suse.cz> wrote:

> On Thu 2014-11-13 20:12:46, Steven Rostedt wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > 
> > Adding a trace_seq_has_overflowed() which returns true if the trace_seq
> > had too much written into it allows us to simplify the code.
> > 
> > Instead of checking the return value of every call to trace_seq_printf()
> > and friends, they can all be called normally, and at the end we can
> > return !trace_seq_has_overflowed() instead.
> > 
> > Several functions also return TRACE_TYPE_PARTIAL_LINE when the trace_seq
> > overflowed and TRACE_TYPE_HANDLED otherwise. Another helper function
> > was created called trace_handle_return() which takes a trace_seq and
> > returns these enums. Using this helper function also simplifies the
> > code.
> > 
> > This change also makes it possible to remove the return values of
> > trace_seq_printf() and friends. They should instead just be
> > void functions.
> > 
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > ---
> >  include/linux/ftrace_event.h |  11 ++
> >  include/linux/trace_seq.h    |  12 ++
> >  include/trace/ftrace.h       |   6 +-
> >  kernel/trace/trace.c         |  69 +++----
> >  kernel/trace/trace.h         |   1 +
> >  kernel/trace/trace_output.c  | 416 +++++++++++++++++--------------------------
> >  kernel/trace/trace_output.h  |  16 +-
> >  7 files changed, 231 insertions(+), 300 deletions(-)
> 
> [...]
> 
> > diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
> > index cfa91de22e27..163c11b6b8ff 100644
> > --- a/kernel/trace/trace_output.c
> > +++ b/kernel/trace/trace_output.c
> [...]
> > @@ -193,7 +184,6 @@ int ftrace_raw_output_prep(struct trace_iterator *iter,
> >  	struct trace_seq *s = &iter->seq;
> >  	struct trace_seq *p = &iter->tmp_seq;
> >  	struct trace_entry *entry;
> > -	int ret;
> >  
> >  	event = container_of(trace_event, struct ftrace_event_call, event);
> >  	entry = iter->ent;
> > @@ -204,8 +194,9 @@ int ftrace_raw_output_prep(struct trace_iterator *iter,
> >  	}
> >  
> >  	trace_seq_init(p);
> > -	ret = trace_seq_printf(s, "%s: ", ftrace_event_name(event));
> > -	if (!ret)
> > +	trace_seq_printf(s, "%s: ", ftrace_event_name(event));
> > +
> > +	if (trace_seq_has_overflowed(s))
> >  		return TRACE_TYPE_PARTIAL_LINE;
> >  
> >  	return 0;
> 
> This looks like a bug in the original code. It returns 0 in each case
> because TRACE_TYPE_PARTIAL_LINE == 0. I guess that the last three
> lines should get replaced by
> 
> 	return trace_handle_return(s);
> 
> as it is done in the other functions.

Yes, I noticed this too. As it was a bug in the original code, I left
it. I do plan on changing it though. Thanks for the reminder.


> 
> [...]
> 
> > @@ -320,14 +302,14 @@ int seq_print_user_ip(struct trace_seq *s, struct mm_struct *mm,
> >  		if (file) {
> >  			ret = trace_seq_path(s, &file->f_path);
> >  			if (ret)
> 
> I think that we could ignore the return value from trace_seq_path() as
> well. The other trace_seq*() writes will be NOP if there is an
> overflow.

Yes I know this too, and since trace_seq_path() still has a return
value, I left it.

> 
> Or do you have some special plans with trace_seq_path(), please?

I want to understand what the seq_path() code does and why Al Viro said
it requires having a return value. I'm just not touching that code
until I have a full understanding. It's not needed for this patch set.


> 
> > -				ret = trace_seq_printf(s, "[+0x%lx]",
> > -						       ip - vmstart);
> > +				trace_seq_printf(s, "[+0x%lx]",
> > +						 ip - vmstart);
> >  		}
> >  		up_read(&mm->mmap_sem);
> >  	}
> >  	if (ret && ((sym_flags & TRACE_ITER_SYM_ADDR) || !file))
> > -		ret = trace_seq_printf(s, " <" IP_FMT ">", ip);
> > -	return ret;
> > +		trace_seq_printf(s, " <" IP_FMT ">", ip);
> > +	return !trace_seq_has_overflowed(s);
> >  }
> 
> The wrong return value was there even before. The other problem is
> just another optimization. Feel free to solve them later in another
> patch(set).

Do you mean your first comment? If so, yeah, that's why I didn't change
it. But I forgot to write a patch to fix it, so thanks for the reminder.


> 
> I really like the simplification.
> 
> Reviewed-by: Petr Mladek <pmladek@suse.cz>

Thanks,

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