Re: [PATCH V6 15/17] perf tools: Intel BTS to always update thread stack trace number

[Arnaldo Carvalho de Melo]

Em Mon, Jun 22, 2015 at 03:38:35PM +0300, Adrian Hunter escreveu:
> On 19/06/15 19:11, Arnaldo Carvalho de Melo wrote:
> > Em Fri, May 29, 2015 at 04:33:43PM +0300, Adrian Hunter escreveu:
> >> The enhanced thread stack is used by higher layers but still requires
> >> the trace number.  The trace number is used to distinguish discontinuous
> >> sections of trace (for example from Snapshot mode or Sample mode), which
> >> cause the thread stack to be flushed.
> >>
> >> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> >> ---
> >>  tools/perf/util/intel-bts.c | 18 ++++++++++++++----
> >>  1 file changed, 14 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
> >> index b068860..cd7bde3 100644
> >> --- a/tools/perf/util/intel-bts.c
> >> +++ b/tools/perf/util/intel-bts.c
> >> @@ -27,6 +27,8 @@
> >>  #include "machine.h"
> >>  #include "session.h"
> >>  #include "util.h"
> >> +#include "thread.h"
> >> +#include "thread-stack.h"
> >>  #include "debug.h"
> >>  #include "tsc.h"
> >>  #include "auxtrace.h"
> >> @@ -443,19 +445,22 @@ static int intel_bts_process_buffer(struct intel_bts_queue *btsq,
> >>  
> >>  static int intel_bts_process_queue(struct intel_bts_queue *btsq, u64 *timestamp)
> >>  {
> >> -	struct auxtrace_buffer *buffer = btsq->buffer;
> >> +	struct auxtrace_buffer *buffer = btsq->buffer, *old_buffer = buffer;
> >>  	struct auxtrace_queue *queue;
> >> +	struct thread *thread;
> >>  	int err;
> >>  
> >>  	if (btsq->done)
> >>  		return 1;
> >>  
> >>  	if (btsq->pid == -1) {
> >> -		struct thread *thread;
> >> -
> >> -		thread = machine__find_thread(btsq->bts->machine, -1, btsq->tid);
> >> +		thread = machine__find_thread(btsq->bts->machine, -1,
> >> +					      btsq->tid);
> >>  		if (thread)
> >>  			btsq->pid = thread->pid_;
> >> +	} else {
> >> +		thread = machine__findnew_thread(btsq->bts->machine, btsq->pid,
> >> +						 btsq->tid);
> > 
> > Humm, so what will be done with the reference count you got from
> > machine__findnew_thread()? You have to drop it when you're done with
> > using this thread.
> > 
> 
> Thought I fixed that. Went looking and, yes, the chunks got lost when
> rolling V6 of the patches. Anyway here are the fixes as a separate patch.

Thanks, I'll try and fold that into that patch, to keep bisect happy, as
refcount bugs sometimes are hard to find, I guess I'll even try writing
a generic perf based refcounting debugging tool at some point...

- Arnaldo
 
> From: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> Date: Mon, 22 Jun 2015 15:02:04 +0300
> Subject: [PATCH] perf tools: Fix missing thread__put()s
> 
> Processing for Intel BTS and Intel PT are using machine__find_thread() and
> machine__findnew_thread() which increase the struct thread reference count.
> Add missing thread__put()s when finished with that struct thread reference.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> ---
>  tools/perf/util/intel-bts.c | 36 ++++++++++++++++++++++++------------
>  tools/perf/util/intel-pt.c  |  5 +++--
>  2 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
> index cd7bde33b635..dce99cfb1309 100644
> --- a/tools/perf/util/intel-bts.c
> +++ b/tools/perf/util/intel-bts.c
> @@ -318,6 +318,7 @@ static int intel_bts_get_next_insn(struct
> intel_bts_queue *btsq, u64 ip)
>  	ssize_t len;
>  	int x86_64;
>  	uint8_t cpumode;
> +	int err = -1;
> 
>  	bufsz = intel_pt_insn_max_size();
> 
> @@ -332,11 +333,11 @@ static int intel_bts_get_next_insn(struct
> intel_bts_queue *btsq, u64 ip)
> 
>  	thread__find_addr_map(thread, cpumode, MAP__FUNCTION, ip, &al);
>  	if (!al.map || !al.map->dso)
> -		return -1;
> +		goto out_put;
> 
>  	len = dso__data_read_addr(al.map->dso, al.map, machine, ip, buf, bufsz);
>  	if (len <= 0)
> -		return -1;
> +		goto out_put;
> 
>  	/* Load maps to ensure dso->is_64_bit has been updated */
>  	map__load(al.map, machine->symbol_filter);
> @@ -344,9 +345,12 @@ static int intel_bts_get_next_insn(struct
> intel_bts_queue *btsq, u64 ip)
>  	x86_64 = al.map->dso->is_64_bit;
> 
>  	if (intel_pt_get_insn(buf, len, x86_64, &btsq->intel_pt_insn))
> -		return -1;
> +		goto out_put;
> 
> -	return 0;
> +	err = 0;
> +out_put:
> +	thread__put(thread);
> +	return err;
>  }
> 
>  static int intel_bts_synth_error(struct intel_bts *bts, int cpu, pid_t pid,
> @@ -471,24 +475,31 @@ static int intel_bts_process_queue(struct
> intel_bts_queue *btsq, u64 *timestamp)
>  	if (!buffer) {
>  		if (!btsq->bts->sampling_mode)
>  			btsq->done = 1;
> -		return 1;
> +		err = 1;
> +		goto out_put;
>  	}
> 
>  	/* Currently there is no support for split buffers */
> -	if (buffer->consecutive)
> -		return -EINVAL;
> +	if (buffer->consecutive) {
> +		err = -EINVAL;
> +		goto out_put;
> +	}
> 
>  	if (!buffer->data) {
>  		int fd = perf_data_file__fd(btsq->bts->session->file);
> 
>  		buffer->data = auxtrace_buffer__get_data(buffer, fd);
> -		if (!buffer->data)
> -			return -ENOMEM;
> +		if (!buffer->data) {
> +			err = -ENOMEM;
> +			goto out_put;
> +		}
>  	}
> 
>  	if (btsq->bts->snapshot_mode && !buffer->consecutive &&
> -	    intel_bts_do_fix_overlap(queue, buffer))
> -		return -ENOMEM;
> +	    intel_bts_do_fix_overlap(queue, buffer)) {
> +		err = -ENOMEM;
> +		goto out_put;
> +	}
> 
>  	if (!btsq->bts->synth_opts.callchain && thread &&
>  	    (!old_buffer || btsq->bts->sampling_mode ||
> @@ -507,7 +518,8 @@ static int intel_bts_process_queue(struct
> intel_bts_queue *btsq, u64 *timestamp)
>  		if (!btsq->bts->sampling_mode)
>  			btsq->done = 1;
>  	}
> -
> +out_put:
> +	thread__put(thread);
>  	return err;
>  }
> 
> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index 751c43a1fbcc..8c8559615666 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -200,7 +200,7 @@ static void intel_pt_use_buffer_pid_tid(struct
> intel_pt_queue *ptq,
>  	intel_pt_log("queue %u cpu %d pid %d tid %d\n",
>  		     ptq->queue_nr, ptq->cpu, ptq->pid, ptq->tid);
> 
> -	ptq->thread = NULL;
> +	thread__zput(ptq->thread);
> 
>  	if (ptq->tid != -1) {
>  		if (ptq->pid != -1)
> @@ -713,6 +713,7 @@ static void intel_pt_free_queue(void *priv)
> 
>  	if (!ptq)
>  		return;
> +	thread__zput(ptq->thread);
>  	intel_pt_decoder_free(ptq->decoder);
>  	zfree(&ptq->event_buf);
>  	zfree(&ptq->chain);
> @@ -726,7 +727,7 @@ static void intel_pt_set_pid_tid_cpu(struct intel_pt *pt,
> 
>  	if (queue->tid == -1 || pt->have_sched_switch) {
>  		ptq->tid = machine__get_current_tid(pt->machine, ptq->cpu);
> -		ptq->thread = NULL;
> +		thread__zput(ptq->thread);
>  	}
> 
>  	if (!ptq->thread && ptq->tid != -1)
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/