Hi, On Fri, 11 Oct 2013 08:31:45 -0400, Steven Rostedt wrote: > On Fri, 11 Oct 2013 17:19:46 +0900 > Namhyung Kim <namhy...@kernel.org> wrote: > >> Hi Steve, >> >> On Fri, 11 Oct 2013 00:17:17 -0400, Steven Rostedt wrote: >> > Sorry for the very late reply, finally got some time to look at other >> > peoples code. >> >> Thank you for taking your time to review this carefully. :) > > Sorry for it taking so long.
Nevermind. :) > >> > I would be a bit more specific in your comment. Explain that >> > curr_ret_stack is negative when we hit a function in the >> > set_graph_notrace file. >> >> How about this: >> >> /* >> * curr_ret_stack is initialized to -1 and gets increased in >> * this function. So it can be less than -1 only if it was >> * filtered out via ftrace_graph_notrace_addr() which can be >> * set from set_graph_notrace file in debugfs by user. >> */ > > Looks good. > >> >> > >> >> + >> >> calltime = trace_clock_local(); >> >> >> >> index = ++current->curr_ret_stack; >> >> + if (ftrace_graph_notrace_addr(func)) >> >> + current->curr_ret_stack -= FTRACE_NOTRACE_DEPTH; >> > >> > I really hate this double call to ftrace_graph_notrace_addr(). The >> > first one in trace_graph_entry(), and then here too. >> > >> > Isn't there a way we could pass the state? Hmm, I think we could use >> > depth to do that. As depth is a pointer to trace.depth and not used >> > before then. We could make it negative and then check that. >> > >> > /me looks at other archs. >> > >> > Darn it, s390 calls ftrace_push_return_trace() before >> > ftrace_graph_entry(). They got fancy, as they must have noticed that >> > trace.depth is set by ftrace_push_return_trace() and they just figured >> > to let the ftrace_push_return_trace() do the work. >> >> Yes, I thought about it before but as you can see other archs go to the >> other way around so I just ended up to call it twice. >> >> > >> > Hmm, we could add a config to do the check on one side or the other >> > depending on how the arch handles it. >> > >> > It needs to be well commented though. >> >> Hmm.. maybe. But it'd better if this function call order is fixed >> IMHO. Anyway, I'll add comments. > > Well, as you probably already saw, s390 changed to help us out :-) Is > there other archs you know about. I haven't looked at all the others > yet. s390 was the first one I saw that didn't follow suit. As you can see that most other archs don't follow the ordering. > > Anyway, lets keep your double check for now. I'll look at making the > two function calls from arch into one, where we can optimize this a bit > more. Okay. > [SNIP] >> >> @@ -259,10 +272,14 @@ int trace_graph_entry(struct ftrace_graph_ent >> >> *trace) >> >> >> >> /* trace it when it is-nested-in or is a function enabled. */ >> >> if ((!(trace->depth || ftrace_graph_addr(trace->func)) || >> >> - ftrace_graph_ignore_irqs()) || >> >> + ftrace_graph_ignore_irqs()) || (trace->depth < 0) || >> >> (max_depth && trace->depth >= max_depth)) >> >> return 0; >> >> >> >> + /* need to reserve a ret_stack entry to recover the depth */ >> >> + if (ftrace_graph_notrace_addr(trace->func)) >> >> + return 1; >> >> + >> > >> > Also, I understand what you are doing here, with making curr_ret_stack >> > negative to denote we are in a state to not do tracing. But it's more >> > of a hack (not a bad one), and really needs to be documented somewhere. >> > That is, the design should be in the comments where it's used, and 5 >> > years from now, nobody will understand how the notrace works without >> > spending days trying to figure it out. Let's be nice to that poor soul, >> > and write up what is going on so they don't need to pray to the holy >> > tuna hoping to get a fish of enlightenment on how turning >> > curr_ret_stack negative magically makes the function graph tracing not >> > trace down functions, and magically starts tracing again when it comes >> > back up. >> >> Fully agreed. How about this: >> >> /* >> * The curr_ret_stack is an index to ftrace return stack of current >> * task. Its value should be in [0, FTRACE_RETFUNC_DEPTH) when the >> * function graph tracer is used. To support filtering out specific >> * functions, it makes the index negative by subtracting huge value > > s/huge/the max/ Hmm.. I introduced FTRACE_NOTRACE_DEPTH (not FTRACE_RETFUNC_DEPTH) in order to prevent possible off-by-one bug in any case. Do you think just using FTRACE_RETFUNC_DEPTH is enough? > >> * (FTRACE_NOTRACE_DEPTH) so when it sees a negative index the ftrace >> * will ignore the record. And the index gets recovered when returning >> * from the filtered function by adding the FTRACE_NOTRACE_DEPTH and >> * then it will continue to record functions normally. >> */ > > Sounds good. Thank you for the review! Namhyung -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/