Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Namhyung Kim <namhyung@gmail.com>]

Hi Don,

On Wed, 7 May 2014 11:22:19 -0400, Don Zickus wrote:
> On Wed, May 07, 2014 at 12:05:58PM +0900, Namhyung Kim wrote:
>> Hi Don,
>> 
>> On Wed, 30 Apr 2014 09:35:55 -0400, Don Zickus wrote:
>> > On Wed, Apr 30, 2014 at 08:38:10AM +0900, Namhyung Kim wrote:
>> >> Hi Don,
>> >> 
>> >> On Tue, 29 Apr 2014 13:27:35 -0400, Don Zickus wrote:
>> >> > On Tue, Apr 29, 2014 at 10:13:35AM +0900, Namhyung Kim wrote:
>> >> >> >        /*
>> >> >> >         * 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.
>> >> >> >         */
>> >> >> >
>> >> >> >        if ((left->cpumode != PERF_RECORD_MISC_KERNEL) &&
>> >> >> >            !l_map->maj && !l_map->min && !l_map->ino &&
>> >> >> >            !l_map->ino_generation) {
>> >> >> >                /* userspace anonymous */
>> >> >> >
>> >> >> >                if (left->thread->pid_ > right->thread->pid_) return -1;
>> >> >> >                if (left->thread->pid_ < right->thread->pid_) return 1;
>> >> >> 
>> >> >> Isn't it necessary to check whether the address is in a same map in case
>> >> >> of anon pages?  I mean the daddr.al_addr is a map-relative offset so it
>> >> >> might have same value for different maps.
>> >> >
>> >> > That's why I sort on pids here.  Because the anon address might have the
>> >> > same value for different maps.  The thought was to group all the pid
>> >> > addresses together to keep things seperated.
>> >> >
>> >> > Do you see a different way to solve the problem?  I am not sure al_addr
>> >> > vs. addr will make much difference here.
>> >> 
>> >> I'm not saying to get rid of the pid check, I'm saying that it might
>> >> need to add another check for maps (i.e. start address) as there might
>> >> be many maps in a single address space.
>> >
>> > Hmm, I guess I would need to see an example.  While I agree there might be
>> > many maps in a single address space (/proc/<pid>/maps demonstrates that),
>> > I understood them to map to a unique location (ie no overlap) unless they
>> > are shared.
>> >
>> > I am willing to believe I missed scenario when sorting, I just can't think
>> > of it (so I wouldn't know how to fix it).  That's why I was looking for an
>> > example to make it more obvious to me.  Sorry for being slow..
>> 
>> I'm also sorry for being late.  Looking at the code, it seems to use
>> identity__map_ip() for anon maps so my concern is bogus.  Please just
>> forget about it and keep going.  Sorry for interrupting your work..
>
> No worries.  Always good to have an extra pair of eyes.
>
> I am a little stuck about where to go.  I think it might make sense to
> submit this as a standalone patch (despite the possible group limitation
> we are discussing).  Thoughts?

You meant to submit the 'dcacheline' sort key patch?  I'm okay with it.

Things are getting more confusing when thinking about complex
scenarios..  so I'd rather finish this -F option patchset and start to
think about how to integrate group sorting with it.

Thanks,
Namhyung