Re: [PATCH 3/3] tracing/function-return-tracer: add the overrun field

[Ingo Molnar <mingo@elte.hu>]

* Frédéric Weisbecker <fweisbec@gmail.com> wrote:

> 2008/11/18 Ingo Molnar <mingo@elte.hu>:
> >
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> >>
> >> On Tue, 18 Nov 2008, Ingo Molnar wrote:
> >> > > >
> >> > > > that reminds me: ti->ret_stack[] should be moved to task->ret_stack[].
> >> > > > That way we decouple its size from any kernel stack size limits.
> >> > > > (thread-info resides at one end of the kernel stack, on x86)
> >> > >
> >> > > Yeah, I recommended that to Frederic to save space. But that can be
> >> > > dangerous. Using task instead would be safer with the downside of
> >> > > making the task struct even bigger.
> >> >
> >> > We almost never put new stuff into thread_info - we have the
> >> > lockdep lock stack in the task structure too, for similar reasons.
> >>
> >> Yeah, it was just a recommendation, and perhaps not a good one ;-)
> >>
> >> Frederic, it is better if you move the array from the thread info to
> >> the task struct. It will take up more memory but it is a hell of a
> >> lot safer. The pro here definitely outways the con.
> >
> > if the memory footprint starts mattering we could turn this into a
> > single pointer to an array - and add/remove these arrays (from all
> > tasks currently running) as the tracer is turned on/off.
> >
> >        Ingo
> >
> 
> Ok. So what do you suggest once? Do I begin to move the array from 
> thread info to struct task but by keeping the static array or should 
> I directly use a dynamic allocation and add/remove dynamically?

Would be nice to have the dynamic allocation straight away - the 
tracer looks rather useful but people wont enable it by default for 
sure. This way distros could enable it all by default without worrying 
about the memory overhead.

And with a dynamic array the tracer could even have a tunable 'max 
depth' option [only changeable when the tracer is not active].

So it all looks much nicer to me that way ... if you succeed in coding 
it up that is ;-) Changing all tasks at once is tricky business: you'd 
have to take the tasklist_lock and iterate over every user and kernel 
task - including idle tasks.

	Ingo