From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/8] perf thread-stack: Avoid direct reference to the thread's stack
Date: Wed, 2 Jan 2019 10:47:29 -0300 [thread overview]
Message-ID: <20190102134729.GB15460@kernel.org> (raw)
In-Reply-To: <20181221120620.9659-4-adrian.hunter@intel.com>
Em Fri, Dec 21, 2018 at 02:06:15PM +0200, Adrian Hunter escreveu:
> In preparation for fixing thread stack processing for the idle task,
> avoid direct reference to the thread's stack. The thread stack will change
> to an array of thread stacks, at which point the meaning of the direct
> reference will change.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
> tools/perf/util/thread-stack.c | 81 ++++++++++++++++++++--------------
> 1 file changed, 49 insertions(+), 32 deletions(-)
>
> diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
> index 068c7c8db4be..e13127755293 100644
> --- a/tools/perf/util/thread-stack.c
> +++ b/tools/perf/util/thread-stack.c
> @@ -111,9 +111,16 @@ static struct thread_stack *thread_stack__new(struct thread *thread,
> ts->kernel_start = 1ULL << 63;
> ts->crp = crp;
>
> + thread->ts = ts;
> +
> return ts;
> }
>
> +static inline struct thread_stack *thread_stack__ts(struct thread *thread)
> +{
> + return thread ? thread->ts : NULL;
> +}
So, this is a 'thread' method, so it should be instead:
static inline struct thread_stack *thread__stack(struct thread *thread)
perhaps?
Or thread__ts() or the longer thread__thread_stack(), I think
thread__stack() is ok, I'm doing it tentatively in my local branch,
holler if you disagree.
- Arnaldo
> +
> static int thread_stack__push(struct thread_stack *ts, u64 ret_addr,
> bool trace_end)
> {
> @@ -226,8 +233,10 @@ static int __thread_stack__flush(struct thread *thread, struct thread_stack *ts)
>
> int thread_stack__flush(struct thread *thread)
> {
> - if (thread->ts)
> - return __thread_stack__flush(thread, thread->ts);
> + struct thread_stack *ts = thread->ts;
> +
> + if (ts)
> + return __thread_stack__flush(thread, ts);
>
> return 0;
> }
> @@ -235,16 +244,18 @@ int thread_stack__flush(struct thread *thread)
> int thread_stack__event(struct thread *thread, u32 flags, u64 from_ip,
> u64 to_ip, u16 insn_len, u64 trace_nr)
> {
> + struct thread_stack *ts = thread_stack__ts(thread);
> +
> if (!thread)
> return -EINVAL;
>
> - if (!thread->ts) {
> - thread->ts = thread_stack__new(thread, NULL);
> - if (!thread->ts) {
> + if (!ts) {
> + ts = thread_stack__new(thread, NULL);
> + if (!ts) {
> pr_warning("Out of memory: no thread stack\n");
> return -ENOMEM;
> }
> - thread->ts->trace_nr = trace_nr;
> + ts->trace_nr = trace_nr;
> }
>
> /*
> @@ -252,14 +263,14 @@ int thread_stack__event(struct thread *thread, u32 flags, u64 from_ip,
> * the stack might be completely invalid. Better to report nothing than
> * to report something misleading, so flush the stack.
> */
> - if (trace_nr != thread->ts->trace_nr) {
> - if (thread->ts->trace_nr)
> - __thread_stack__flush(thread, thread->ts);
> - thread->ts->trace_nr = trace_nr;
> + if (trace_nr != ts->trace_nr) {
> + if (ts->trace_nr)
> + __thread_stack__flush(thread, ts);
> + ts->trace_nr = trace_nr;
> }
>
> /* Stop here if thread_stack__process() is in use */
> - if (thread->ts->crp)
> + if (ts->crp)
> return 0;
>
> if (flags & PERF_IP_FLAG_CALL) {
> @@ -270,7 +281,7 @@ int thread_stack__event(struct thread *thread, u32 flags, u64 from_ip,
> ret_addr = from_ip + insn_len;
> if (ret_addr == to_ip)
> return 0; /* Zero-length calls are excluded */
> - return thread_stack__push(thread->ts, ret_addr,
> + return thread_stack__push(ts, ret_addr,
> flags & PERF_IP_FLAG_TRACE_END);
> } else if (flags & PERF_IP_FLAG_TRACE_BEGIN) {
> /*
> @@ -280,10 +291,10 @@ int thread_stack__event(struct thread *thread, u32 flags, u64 from_ip,
> * address, so try to pop that. Also, do not expect a call made
> * when the trace ended, to return, so pop that.
> */
> - thread_stack__pop(thread->ts, to_ip);
> - thread_stack__pop_trace_end(thread->ts);
> + thread_stack__pop(ts, to_ip);
> + thread_stack__pop_trace_end(ts);
> } else if ((flags & PERF_IP_FLAG_RETURN) && from_ip) {
> - thread_stack__pop(thread->ts, to_ip);
> + thread_stack__pop(ts, to_ip);
> }
>
> return 0;
> @@ -291,21 +302,25 @@ int thread_stack__event(struct thread *thread, u32 flags, u64 from_ip,
>
> void thread_stack__set_trace_nr(struct thread *thread, u64 trace_nr)
> {
> - if (!thread || !thread->ts)
> + struct thread_stack *ts = thread_stack__ts(thread);
> +
> + if (!ts)
> return;
>
> - if (trace_nr != thread->ts->trace_nr) {
> - if (thread->ts->trace_nr)
> - __thread_stack__flush(thread, thread->ts);
> - thread->ts->trace_nr = trace_nr;
> + if (trace_nr != ts->trace_nr) {
> + if (ts->trace_nr)
> + __thread_stack__flush(thread, ts);
> + ts->trace_nr = trace_nr;
> }
> }
>
> void thread_stack__free(struct thread *thread)
> {
> - if (thread->ts) {
> - __thread_stack__flush(thread, thread->ts);
> - zfree(&thread->ts->stack);
> + struct thread_stack *ts = thread->ts;
> +
> + if (ts) {
> + __thread_stack__flush(thread, ts);
> + zfree(&ts->stack);
> zfree(&thread->ts);
> }
> }
> @@ -318,6 +333,7 @@ static inline u64 callchain_context(u64 ip, u64 kernel_start)
> void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
> size_t sz, u64 ip, u64 kernel_start)
> {
> + struct thread_stack *ts = thread_stack__ts(thread);
> u64 context = callchain_context(ip, kernel_start);
> u64 last_context;
> size_t i, j;
> @@ -330,15 +346,15 @@ void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
> chain->ips[0] = context;
> chain->ips[1] = ip;
>
> - if (!thread || !thread->ts) {
> + if (!ts) {
> chain->nr = 2;
> return;
> }
>
> last_context = context;
>
> - for (i = 2, j = 1; i < sz && j <= thread->ts->cnt; i++, j++) {
> - ip = thread->ts->stack[thread->ts->cnt - j].ret_addr;
> + for (i = 2, j = 1; i < sz && j <= ts->cnt; i++, j++) {
> + ip = ts->stack[ts->cnt - j].ret_addr;
> context = callchain_context(ip, kernel_start);
> if (context != last_context) {
> if (i >= sz - 1)
> @@ -590,7 +606,7 @@ int thread_stack__process(struct thread *thread, struct comm *comm,
> struct addr_location *to_al, u64 ref,
> struct call_return_processor *crp)
> {
> - struct thread_stack *ts = thread->ts;
> + struct thread_stack *ts = thread_stack__ts(thread);
> int err = 0;
>
> if (ts && !ts->crp) {
> @@ -600,10 +616,9 @@ int thread_stack__process(struct thread *thread, struct comm *comm,
> }
>
> if (!ts) {
> - thread->ts = thread_stack__new(thread, crp);
> - if (!thread->ts)
> + ts = thread_stack__new(thread, crp);
> + if (!ts)
> return -ENOMEM;
> - ts = thread->ts;
> ts->comm = comm;
> }
>
> @@ -668,7 +683,9 @@ int thread_stack__process(struct thread *thread, struct comm *comm,
>
> size_t thread_stack__depth(struct thread *thread)
> {
> - if (!thread->ts)
> + struct thread_stack *ts = thread_stack__ts(thread);
> +
> + if (!ts)
> return 0;
> - return thread->ts->cnt;
> + return ts->cnt;
> }
> --
> 2.17.1
--
- Arnaldo
next prev parent reply other threads:[~2019-01-02 13:47 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-21 12:06 [PATCH 0/8] perf thread-stack: Fix thread stack processing for the idle task Adrian Hunter
2018-12-21 12:06 ` [PATCH 1/8] perf thread-stack: Simplify some code in thread_stack__process() Adrian Hunter
2019-01-03 13:22 ` [tip:perf/urgent] " tip-bot for Adrian Hunter
2018-12-21 12:06 ` [PATCH 2/8] perf thread-stack: Tidy thread_stack__bottom() usage Adrian Hunter
2019-01-03 13:23 ` [tip:perf/urgent] " tip-bot for Adrian Hunter
2018-12-21 12:06 ` [PATCH 3/8] perf thread-stack: Avoid direct reference to the thread's stack Adrian Hunter
2019-01-02 13:47 ` Arnaldo Carvalho de Melo [this message]
2019-01-03 13:23 ` [tip:perf/urgent] " tip-bot for Adrian Hunter
2018-12-21 12:06 ` [PATCH 4/8] perf thread-stack: Allow for a thread stack array Adrian Hunter
2019-01-03 13:24 ` [tip:perf/urgent] " tip-bot for Adrian Hunter
2018-12-21 12:06 ` [PATCH 5/8] perf thread-stack: Factor out thread_stack__init() Adrian Hunter
2019-01-03 13:24 ` [tip:perf/urgent] " tip-bot for Adrian Hunter
2018-12-21 12:06 ` [PATCH 6/8] perf thread-stack: Allocate an array of thread stacks Adrian Hunter
2019-01-01 18:28 ` Jiri Olsa
2019-01-02 13:43 ` Arnaldo Carvalho de Melo
2019-01-03 13:25 ` [tip:perf/urgent] " tip-bot for Adrian Hunter
2018-12-21 12:06 ` [PATCH 8/8] perf session: Add comment for perf_session__register_idle_thread() Adrian Hunter
2019-01-03 13:26 ` [tip:perf/urgent] " tip-bot for Adrian Hunter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190102134729.GB15460@kernel.org \
--to=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).