Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751563AbdIOR3T convert rfc822-to-8bit (ORCPT <rfc822;w@1wt.eu>);
        Fri, 15 Sep 2017 13:29:19 -0400
Received: from mga01.intel.com ([192.55.52.88]:23629 "EHLO mga01.intel.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1751190AbdIOR3S (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 15 Sep 2017 13:29:18 -0400
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.42,398,1500966000"; 
   d="scan'208";a="129123862"
From: "Liang, Kan" <kan.liang@intel.com>
To: Arnaldo Carvalho de Melo <acme@kernel.org>
CC: "peterz@infradead.org" <peterz@infradead.org>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "jolsa@kernel.org" <jolsa@kernel.org>,
        "namhyung@kernel.org" <namhyung@kernel.org>,
        "Hunter, Adrian" <adrian.hunter@intel.com>,
        "Odzioba, Lukasz" <lukasz.odzioba@intel.com>,
        "ak@linux.intel.com" <ak@linux.intel.com>
Subject: RE: [PATCH RFC V2 00/10] perf top optimization
Thread-Topic: [PATCH RFC V2 00/10] perf top optimization
Thread-Index: AQHTKqUGKqa3XFGJ30eMjpDMlJ7smaKybY4AgACGmHD//30ZgIAB8bsAgAGsAaD//6UjAIAAhlcQ
Date: Fri, 15 Sep 2017 17:29:13 +0000
Message-ID: <37D7C6CF3E00A74B8858931C1DB2F077537BF150@SHSMSX103.ccr.corp.intel.com>
References: <1505096603-215017-1-git-send-email-kan.liang@intel.com>
 <20170913152506.GK5866@kernel.org>
 <37D7C6CF3E00A74B8858931C1DB2F077537AF31A@SHSMSX103.ccr.corp.intel.com>
 <20170913153819.GL5866@kernel.org> <20170914211946.GB10371@kernel.org>
 <37D7C6CF3E00A74B8858931C1DB2F077537BF0A8@SHSMSX103.ccr.corp.intel.com>
 <20170915172625.GA14469@kernel.org>
In-Reply-To: <20170915172625.GA14469@kernel.org>
Accept-Language: zh-CN, en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZDQzZTZkYjktMWU1MC00Y2ExLWFhYjQtMzdmZmEyOTE2OWE2IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6ImY5U2NIMUI2V2Z3VXp4dlJOckhVQ01MOElhUExMQXFXTHdcL25FNTduOGdnPSJ9
x-ctpclassification: CTP_IC
dlp-product: dlpe-windows
dlp-version: 11.0.0.116
dlp-reaction: no-action
x-originating-ip: [10.239.127.40]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3205
Lines: 89

> Em Fri, Sep 15, 2017 at 03:11:51PM +0000, Liang, Kan escreveu:
> > > Em Wed, Sep 13, 2017 at 12:38:19PM -0300, Arnaldo Carvalho de Melo
> > > escreveu:
> > > > Em Wed, Sep 13, 2017 at 03:29:44PM +0000, Liang, Kan escreveu:
> > > > > >
> > > > > > Em Sun, Sep 10, 2017 at 07:23:13PM -0700, kan.liang@intel.com
> > > escreveu:
> > > > > >
> > > > > > So I got the first two patches already merged, and made some
> > > > > > comments about the other patches, please check those,
> > > > > >
> > > > >
> > > > > Thanks for the review Arnaldo.
> > > > >
> > > > > I will take a close look for the comments.
> > > > > For the next version, I only need to include patch 3-10, correct?
> > > >
> > > > Right, and go from my perf/core branch. The hashtable patch is
> > > > still not there as I am running tests before pushing out, but it
> > > > should be there later today.
> > >
> > > So, its at my repo, branch tmp.perf/threads_hashtable
> > >
> > > But 'perf trace' is broken, please take a look below:
> > >
> > > [root@jouet ~]# gdb -c core
> > > GNU gdb (GDB) Fedora 8.0-20.fc26
> > > <SNIP>
> > > Core was generated by `perf trace -e block:block_bio_queue'.
> > > Program terminated with signal SIGSEGV, Segmentation fault.
> > > #0  0x000000000051089a in ?? ()
> > > (gdb) file perf
> > > Reading symbols from perf...done.
> > > (gdb) bt
> > > #0  0x000000000051089a in ____machine__findnew_thread
> > > (machine=0x3dfcab0, threads=0x3dfca78, pid=-1, tid=-1, create=false)
> > > at
> > > util/machine.c:429
> >
> > I think the root cause is tid==-1. So the index of hashtable will be -1.
> > The patch as below should fix it.
> >
> > diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> > index e6d5381..3c564b8 100644
> > --- a/tools/perf/util/machine.h
> > +++ b/tools/perf/util/machine.h
> > @@ -57,7 +57,7 @@ struct machine {
> >
> >  static inline struct threads *machine__threads(struct machine
> > *machine, pid_t tid)  {
> > -	return &machine->threads[tid % THREADS__TABLE_SIZE];
> > +	return &machine->threads[(unsigned int)tid %
> THREADS__TABLE_SIZE];
> >  }
> >
> >  static inline
> >
> >
> > There should be another issue which was introduced by
> > 33013b9a5607 ("perf machine: Optimize a bit the
> > machine__findnew_thread() methods") It should use tid not pid to get the
> threads.
> >
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index 90ae9c7..ddeea05 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -473,7 +473,7 @@ static struct thread
> > *____machine__findnew_thread(struct machine *machine,
> >
> >  struct thread *__machine__findnew_thread(struct machine *machine,
> > pid_t pid, pid_t tid)  {
> > -	return ____machine__findnew_thread(machine,
> machine__threads(machine, pid), pid, tid, true);
> > +	return ____machine__findnew_thread(machine,
> > +machine__threads(machine, tid), pid, tid, true);
> >  }
> >
> > They are small fixes. I think it's better to merge them with the old patches.
> > Should I include the modified hashtable patches in V3?
> 
> I'll add these now and test, then push another branch, ok?
>

Sure. Thanks.
I will prepare the V3 for the new branch then. 

Thanks,
Kan