From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751492AbbFVOeF (ORCPT ); Mon, 22 Jun 2015 10:34:05 -0400 Received: from mail.kernel.org ([198.145.29.136]:40149 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751094AbbFVOeB (ORCPT ); Mon, 22 Jun 2015 10:34:01 -0400 Date: Mon, 22 Jun 2015 11:33:54 -0300 From: Arnaldo Carvalho de Melo To: Adrian Hunter Cc: Ingo Molnar , linux-kernel@vger.kernel.org, Jiri Olsa Subject: Re: [PATCH V6 15/17] perf tools: Intel BTS to always update thread stack trace number Message-ID: <20150622143354.GA13937@kernel.org> References: <1432906425-9911-1-git-send-email-adrian.hunter@intel.com> <1432906425-9911-16-git-send-email-adrian.hunter@intel.com> <20150619161118.GN3079@kernel.org> <558801CB.6070203@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <558801CB.6070203@intel.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >> --- > >> 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 > 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 > --- > 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/