Re: [PATCH 2/2] tracing - putting cond_resched into tace_pipe loop

[Jiri Olsa <jolsa@redhat.com>]

On Mon, Mar 14, 2011 at 10:28:46AM -0400, Steven Rostedt wrote:
> On Mon, 2011-03-14 at 15:18 +0100, Jiri Olsa wrote:
> 
> 
> > > I'm not sure that this needs the signal_pending() or the break, or even
> > > the cond_resched(). Perhaps the first patch fixes the bug. But that
> > > while loop does not block, and it should just spin enough to fill a
> > > page. If it is not filling the page then that's a bug.
> > 
> > well, if there are no "event lost" messages and the flags are set
> > to the "bin or raw" and you trace events only, then the page
> > is not be filled.
> > 
> > But as there'll be allways lost events, the page will be populated
> > and it will get out of the loop.
> 
> There's two loops, I'm talking about the inner one, which will break out
> when there are no events left to process.
> 
> The "waitagain" calls into tracing_wait_pipe which has the
> signal_pending() attribute. It should also block, or return -EAGAIN if
> the caller is not blocking.
> 
> > 
> > > 
> > > Jiri,
> > > 
> > > Can you reproduce the bug with just he first patch? Actually, I can
> > > reproduce it on vanilla, I'll apply your first patch and see if that
> > > fixes things. If not, then we need to find out why and fix those.
> > 
> > The first patch fixies the 'lost evets' trace, so the loop is escaped
> > when the page is full.
> > (I was going to send some change for first patch WARN_ON -> WARN_ONCE)
> 
> Yes, I was going to recommend doing that too.
> 
> > 
> > I think in case there are no 'lost events' and we have the conditions
> > I describe, we need the cond_resched call.
> 
> No we don't.

ok, probably I'm missing something, but here's the path that dont produce
any data and will keeps us in the inner loop:
(assuming events only, disabled context info and no event lost)

while (trace_find_next_entry_inc(iter) != NULL)
..
	print_trace_line(iter);
		...
		if (trace_flags & TRACE_ITER_BIN)
	                return print_bin_fmt(iter);
			...
			print_bin_fmt
				return event ?event->funcs->binary(iter, 0, event) : TRACE_TYPE_HANDLED;

...


now, "event->funcs->binary" is trace_nop_print, which is defined as:

	enum print_line_t trace_nop_print(struct trace_iterator *iter, int
	flags,
					  struct trace_event *event)
	{
		return TRACE_TYPE_HANDLED;
	}


So we get back the the while loop with no data added and with
TRACE_TYPE_HANDLED return code, which will keep us in the loop.

Not sure this is bug or feature :)

jirka

> 
> As for non preempt kernels, honestly, I don't care about latencies. If
> you are worried about latencies, don't run a non preempt kernel. I do
> care if it goes into an infinite loop, or never processes signals, which
> would be a bug. But long running times in the kernel on a non preempt
> kernel. Well, that's just part of the game.
> 
> -- Steve
> 
> > 
> > I'll send new patch version shortly.
> > 
> > thanks,
> > jirka
> > > 
> > > -- Steve
> > > 
> > > 
> 
>