From: Don Zickus <dzickus@redhat.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: acme@ghostprotocols.net, LKML <linux-kernel@vger.kernel.org>,
jmario@redhat.com, fowles@inreach.com, eranian@google.com
Subject: Re: [PATCH 02/19] perf, sort: Add physid sorting based on mmap2 data
Date: Wed, 19 Mar 2014 09:36:53 -0400 [thread overview]
Message-ID: <20140319133653.GZ25953@redhat.com> (raw)
In-Reply-To: <20140319104515.GA7423@krava.brq.redhat.com>
On Wed, Mar 19, 2014 at 11:45:15AM +0100, Jiri Olsa wrote:
> > +static int64_t
> > +sort__physid_cmp(struct hist_entry *left, struct hist_entry *right)
> > +{
> > + u64 l, r;
> > + struct map *l_map = left->mem_info->daddr.map;
> > + struct map *r_map = right->mem_info->daddr.map;
> > +
> > + /* store all NULL mem maps at the bottom */
> > + /* shouldn't even need this check, should have stubs */
> > + if (!left->mem_info->daddr.map || !right->mem_info->daddr.map)
> > + return 1;
> > +
> > + /* group event types together */
> > + if (left->cpumode > right->cpumode) return -1;
> > + if (left->cpumode < right->cpumode) return 1;
> > +
> > + if (l_map->maj > r_map->maj) return -1;
> > + if (l_map->maj < r_map->maj) return 1;
> > +
> > + if (l_map->min > r_map->min) return -1;
> > + if (l_map->min < r_map->min) return 1;
> > +
> > + if (l_map->ino > r_map->ino) return -1;
> > + if (l_map->ino < r_map->ino) return 1;
> > +
> > + if (l_map->ino_generation > r_map->ino_generation) return -1;
> > + if (l_map->ino_generation < r_map->ino_generation) return 1;
> > +
> > + /*
> > + * Addresses with no major/minor numbers are assumed to be
> > + * anonymous in userspace. Sort those on pid then address.
> > + *
> > + * The kernel and non-zero major/minor mapped areas are
> > + * assumed to be unity mapped. Sort those on address then pid.
> > + */
> > +
> > + /* al_addr does all the right addr - start + offset calculations */
> > + l = left->mem_info->daddr.al_addr;
> > + r = right->mem_info->daddr.al_addr;
> > +
> > + if (l_map->maj || l_map->min || l_map->ino || l_map-> ino_generation) {
> > + /* mmapped areas */
> > +
> > + /* hack to mark similar regions, 'right' is new entry */
> > + /* entries with same maj/min/ino/inogen are in same address space */
> > + right->color = TRUE;
> > +
> > + if (l > r) return -1;
> > + if (l < r) return 1;
> > +
> > + /* sorting by iaddr makes calculations easier later */
> > + if (left->mem_info->iaddr.al_addr > right->mem_info->iaddr.al_addr) return -1;
> > + if (left->mem_info->iaddr.al_addr < right->mem_info->iaddr.al_addr) return 1;
> > +
> > + if (left->thread->pid_ > right->thread->pid_) return -1;
> > + if (left->thread->pid_ < right->thread->pid_) return 1;
> > +
> > + if (left->thread->tid > right->thread->tid) return -1;
> > + if (left->thread->tid < right->thread->tid) return 1;
> > + } else if (left->cpumode == PERF_RECORD_MISC_KERNEL) {
> > + /* kernel mapped areas where 'start' doesn't matter */
> > +
> > + /* hack to mark similar regions, 'right' is new entry */
> > + /* whole kernel region is in the same address space */
> > + right->color = TRUE;
> > +
> > + if (l > r) return -1;
> > + if (l < r) return 1;
> > +
> > + /* sorting by iaddr makes calculations easier later */
> > + if (left->mem_info->iaddr.al_addr > right->mem_info->iaddr.al_addr) return -1;
> > + if (left->mem_info->iaddr.al_addr < right->mem_info->iaddr.al_addr) return 1;
> > +
> > + if (left->thread->pid_ > right->thread->pid_) return -1;
> > + if (left->thread->pid_ < right->thread->pid_) return 1;
> > +
> > + if (left->thread->tid > right->thread->tid) return -1;
> > + if (left->thread->tid < right->thread->tid) return 1;
> > + } else {
> > + /* userspace anonymous */
> > + if (left->thread->pid_ > right->thread->pid_) return -1;
> > + if (left->thread->pid_ < right->thread->pid_) return 1;
> > +
> > + if (left->thread->tid > right->thread->tid) return -1;
> > + if (left->thread->tid < right->thread->tid) return 1;
> > +
> > + /* hack to mark similar regions, 'right' is new entry */
> > + /* userspace anonymous address space is contained within pid */
> > + right->color = TRUE;
> > +
> > + if (l > r) return -1;
> > + if (l < r) return 1;
> > +
> > + /* sorting by iaddr makes calculations easier later */
> > + if (left->mem_info->iaddr.al_addr > right->mem_info->iaddr.al_addr) return -1;
> > + if (left->mem_info->iaddr.al_addr < right->mem_info->iaddr.al_addr) return 1;
> > + }
>
> do you need single column for 'physid' ?
>
> my first idea was to have separate sort entries for all checked entries:
> (same way like for memory memory_sort_dimensions)
>
> - mem_info->daddr.al_addr (we already have 'addr' check)
> - mem_info->iaddr.al_addr
> - thread->pid_ (we have only 'tid' check so far)
> - l_map->maj, l_map->min, l_map->ino, l_map, ino_generation (we could probably group these)
>
> and init sort order with:
>
> sort_order = "physid,pid,...";
>
> '...' is whatever name you choose for above entries
The problem is (as you can see in the code above), physid is _not_ just
one piece of data. It is dependent on multiple things. It is mainly
dependent on four things:
- cpumode
- major, minor, inode, inode_generation (call it mmap2 data)
- data address
- pid
If cpumode == KERNEL, sort in this order
- mmap2 data
- data address
- pid (optional)
If cpumode == USERSPACE and mmap2 data != 0, sort in this order
- mmap2 data
- data address
- pid (optional)
If cpumode == USERSAPCE and mmap2 data == 0, sort in this order
- pid
- data address
Notice how sorting on the pid is different depending on the scenario.
I'll agree that sorting on iaddr and tid can be filtered out as a nice to
have.
But in order to sort on physid, you really need lots of pieces.
Otherwise, what do you consider the definition of 'physid'? :-)
Cheers,
Don
next prev parent reply other threads:[~2014-03-19 13:37 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-02-28 17:42 [PATCH 00/19 V2] perf, c2c: Add new tool to analyze cacheline contention on NUMA systems Don Zickus
2014-02-28 17:42 ` [PATCH 01/19] Revert "perf: Disable PERF_RECORD_MMAP2 support" Don Zickus
2014-02-28 17:42 ` [PATCH 02/19] perf, sort: Add physid sorting based on mmap2 data Don Zickus
2014-03-19 10:45 ` Jiri Olsa
2014-03-19 13:36 ` Don Zickus [this message]
2014-02-28 17:42 ` [PATCH 03/19] perf, sort: Allow unique sorting instead of combining hist_entries Don Zickus
2014-02-28 17:42 ` [PATCH 04/19] perf: Allow ability to map cpus to nodes easily Don Zickus
2014-03-19 12:48 ` Jiri Olsa
2014-03-19 13:38 ` Don Zickus
2014-03-19 13:22 ` Jiri Olsa
2014-02-28 17:42 ` [PATCH 05/19] perf, kmem: Utilize the new generic cpunode_map Don Zickus
2014-02-28 17:42 ` [PATCH 06/19] perf: Fix stddev calculation Don Zickus
2014-02-28 17:42 ` [PATCH 07/19] perf, callchain: Add generic callchain print handler for stdio Don Zickus
2014-02-28 17:42 ` [PATCH 08/19] perf c2c: Shared data analyser Don Zickus
2014-02-28 19:08 ` Andi Kleen
2014-02-28 19:46 ` Don Zickus
2014-02-28 21:03 ` Davidlohr Bueso
2014-02-28 22:28 ` Joe Mario
2014-03-01 0:50 ` Andi Kleen
2014-03-03 14:13 ` Don Zickus
2014-03-03 15:05 ` Don Zickus
2014-03-03 17:23 ` Andi Kleen
2014-03-03 18:07 ` Joe Mario
2014-03-03 18:41 ` Peter Zijlstra
2014-03-03 18:58 ` Andi Kleen
2014-03-03 19:48 ` Peter Zijlstra
2014-03-03 20:32 ` Don Zickus
2014-03-03 21:38 ` Andi Kleen
2014-03-03 21:41 ` Don Zickus
2014-03-03 20:30 ` Don Zickus
2014-03-03 20:26 ` Don Zickus
2014-03-03 21:36 ` Andi Kleen
2014-03-04 9:42 ` Peter Zijlstra
2014-03-03 18:21 ` Davidlohr Bueso
2014-02-28 17:42 ` [PATCH 09/19] perf c2c: Dump raw records, decode data_src bits Don Zickus
2014-02-28 17:42 ` [PATCH 10/19] perf, c2c: Rework setup code to prepare for features Don Zickus
2014-02-28 17:43 ` [PATCH 11/19] perf, c2c: Add in sort on physid Don Zickus
2014-02-28 18:59 ` Andi Kleen
2014-02-28 19:44 ` Don Zickus
2014-03-01 1:07 ` Andi Kleen
2014-03-01 1:27 ` Namhyung Kim
2014-02-28 17:43 ` [PATCH 12/19] perf, c2c: Add stats to track data source bits and cpu to node maps Don Zickus
2014-02-28 17:43 ` [PATCH 13/19] perf, c2c: Sort based on hottest cache line Don Zickus
2014-02-28 17:43 ` [PATCH 14/19] perf, c2c: Display cacheline HITM analysis to stdout Don Zickus
2014-02-28 17:43 ` [PATCH 15/19] perf, c2c: Add callchain support Don Zickus
2014-03-19 13:00 ` Jiri Olsa
2014-03-19 13:53 ` Don Zickus
2014-03-19 14:05 ` Jiri Olsa
2014-02-28 17:43 ` [PATCH 16/19] perf, c2c: Output summary stats Don Zickus
2014-02-28 17:43 ` [PATCH 17/19] perf, c2c: Dump rbtree for debugging Don Zickus
2014-02-28 17:43 ` [PATCH 18/19] perf, c2c: Add symbol count table Don Zickus
2014-02-28 17:43 ` [PATCH 19/19] perf, c2c: Add shared cachline summary table Don Zickus
2014-02-28 18:57 ` [PATCH 00/19 V2] perf, c2c: Add new tool to analyze cacheline contention on NUMA systems Andi Kleen
2014-02-28 19:42 ` Don Zickus
2014-02-28 21:54 ` Andi Kleen
2014-03-03 14:04 ` Don Zickus
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=20140319133653.GZ25953@redhat.com \
--to=dzickus@redhat.com \
--cc=acme@ghostprotocols.net \
--cc=eranian@google.com \
--cc=fowles@inreach.com \
--cc=jmario@redhat.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).