Messages in this thread | | | From | "Liang, Kan" <> | Subject | RE: [PATCH RFC V2 00/10] perf top optimization | Date | Fri, 15 Sep 2017 17:29:13 +0000 |
| |
> 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
| |