Messages in this thread |  | | Date | Thu, 28 Nov 2013 09:01:23 -0700 | From | David Ahern <> | Subject | Re: [PATCH 6/8] perf sched: Introduce timehist command |
| |
On 11/28/13, 8:38 AM, Namhyung Kim wrote: > On Tue, Nov 19, 2013 at 5:32 AM, David Ahern <dsahern@gmail.com> wrote: > > [SNIP] >> +static bool is_idle_sample(struct perf_sample *sample, >> + struct perf_evsel *evsel, >> + struct machine *machine) >> +{ >> + struct thread *thread; >> + struct callchain_cursor *cursor = &callchain_cursor; >> + struct callchain_cursor_node *node; >> + struct addr_location al; >> + int iter = 5; > > Shouldn't it be sched->max_stack somehow?
max_stack is used to dump callstack to the user. In this case we are walking the stack looking for an idle symbol.
> >> + >> + /* pid 0 == swapper == idle task */ >> + if (sample->pid == 0) >> + return true; >> + >> + /* want main thread for process - has maps */ >> + thread = machine__findnew_thread(machine, sample->pid, sample->pid); >> + if (thread == NULL) { >> + pr_debug("Failed to get thread for pid %d.\n", sample->pid); >> + return false; >> + } >> + >> + if (!symbol_conf.use_callchain || sample->callchain == NULL) >> + return false; >> + >> + if (machine__resolve_callchain(machine, evsel, thread, >> + sample, NULL, &al, PERF_MAX_STACK_DEPTH) != 0) { >> + if (verbose) >> + error("Failed to resolve callchain. Skipping\n"); >> + >> + return false; >> + } > > I think this callchain resolving logic should be moved to the > beginning of perf_hist__process_sample() like other commands do. It's > not for idle threads only.
I'll see what can be done.
> > And it also needs to pass sched->max_stack.
Per above, max_stack has a different purpose
> > >> + callchain_cursor_commit(cursor); >> + >> + /* idle symbol should be early in the stack */ >> + while (iter) { >> + node = callchain_cursor_current(cursor); >> + if (!node) >> + break; >> + >> + if (symbol__is_idle(node->sym)) >> + return true; >> + >> + callchain_cursor_advance(cursor); >> + >> + iter--; >> + } >> + >> + return false; >> +} > > [SNIP] >> + /* show wakeups if requested */ >> + if (sched->show_wakeups && !sched->summary_only) > > Hmm.. maybe we can emit a warning if -w and -s options are given at > the same time.
ok.
> > >> + timehist_print_wakeup_event(sched, sample, machine, thread); >> + >> + return 0; >> +} > > [SNIP] >> +static int parse_target_str(struct perf_sched *sched) >> +{ >> + if (sched->target.pid) { >> + sched->pid = intlist__new(sched->target.pid); >> + if (sched->pid == NULL) { >> + pr_err("Error parsing process id string\n"); >> + return -EINVAL; >> + } >> + } >> + >> + if (sched->target.tid) { >> + sched->tid = intlist__new(sched->target.tid); >> + if (sched->tid == NULL) { >> + pr_err("Error parsing thread id string\n"); > > > Need to call intlist__delete(sched->pid) here?
indeed. thanks.
>> + const struct option timehist_options[] = { >> + OPT_STRING('i', "input", &input_name, "file", >> + "input file name"), >> + OPT_INCR('v', "verbose", &verbose, >> + "be more verbose (show symbol address, etc)"), >> + OPT_STRING('k', "vmlinux", &symbol_conf.vmlinux_name, >> + "file", "vmlinux pathname"), >> + OPT_STRING(0, "kallsyms", &symbol_conf.kallsyms_name, >> + "file", "kallsyms pathname"), >> + OPT_STRING('c', "comms", &symbol_conf.comm_list_str, "comm[,comm...]", >> + "only display events for these comms"), >> + OPT_STRING('p', "pid", &sched.target.pid, "pid", >> + "analyze events only for given process id(s)"), >> + OPT_STRING('t', "tid", &sched.target.tid, "tid", >> + "analyze events only for given thread id(s)"), >> + OPT_BOOLEAN('g', "call-graph", &sched.show_callchain, >> + "Display call chains if present (default on)"), >> + OPT_UINTEGER(0, "max-stack", &sched.max_stack, >> + "Maximum number of functions to display backtrace."), >> + OPT_STRING('x', "excl", &excl_sym_list_str, "sym[,sym...]", > > Why not renaming long option name to --exclude or --exclude-symbols?
ok.
David
|  |