On Mon, 24 Mar 2014 15:36:58 -0400, Don Zickus wrote:
> Now that the infrastructure is set, add in the support to use
> hist_entry to sort on physid.
>
> V2: use new mmap2 sort
>
> Signed-off-by: Don Zickus <[email protected]>
> ---
> tools/perf/builtin-c2c.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index d7eaf81..b5742bd 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -7,6 +7,7 @@
> #include "util/tool.h"
> #include "util/debug.h"
> #include <api/fs/fs.h>
> +#include "util/annotate.h"
Why is this #include needed?
[SNIP]
> @@ -163,7 +169,33 @@ static int perf_c2c__process_load_store(struct perf_c2c *c2c,
> return 0;
> }
>
> - return 0;
> + cost = sample->weight;
> + if (!cost)
> + cost = 1;
> +
> + /*
> + * must pass period=weight in order to get the correct
> + * sorting from hists__collapse_resort() which is solely
> + * based on periods. We want sorting be done on nr_events * weight
> + * and this is indirectly achieved by passing period=weight here
> + * and the he_stat__add_period() function.
> + */
> + he = __hists__add_entry(&c2c->hists, al, parent, NULL, mi,
> + cost, cost, 0);
> + if (!he) {
> + err = -ENOMEM;
> + goto out_mem;
> + }
> +
> + c2c->hists.stats.total_period += cost;
> + hists__inc_nr_events(&c2c->hists, PERF_RECORD_SAMPLE);
> + return err;
> +
> +out_mem:
> + /* implicitly freed by __hists__add_entry */
> + free(mi);
> +out:
It seems this 'out' lable is not used anywhere..
> + return err;
> }
>
> /* setup the basic events for most coverage, options added later */
> @@ -266,10 +298,28 @@ out:
> return err;
> }
>
> +static int perf_c2c__init(struct perf_c2c *c2c)
> +{
> + sort__mode = SORT_MODE__PHYSID;
> + sort__wants_unique = 1;
> + sort_order = "daddr,iaddr,pid,tid";
Where are the SORT_MODE__PHYSID, sort__wants_unique and "daddr", "iaddr"
sort keys defined?
Also, more importantly, I think the sort order should contain at least
"mem" and "snoop" keys to group samples based on the exact hitm
information.
In my understanding, if two samples are captured at exactly same
position with a same data access but different hitm info, they cannot be
classified and just use the hitm info of first entry.
Thanks,
Namhyung
> +
> + if (setup_sorting() < 0) {
> + pr_err("can not setup sorting\n");
> + return -1;
> + }
> +
> + hists__init(&c2c->hists);
> +
> + return 0;
> +}
> static int perf_c2c__report(struct perf_c2c *c2c)
> {
> setup_pager();
>
> + if (perf_c2c__init(c2c))
> + return -1;
> +
> if (c2c->raw_records)
> perf_c2c__fprintf_header(stdout);
On Tue, Apr 08, 2014 at 04:56:25PM +0900, Namhyung Kim wrote:
> On Mon, 24 Mar 2014 15:36:58 -0400, Don Zickus wrote:
> > Now that the infrastructure is set, add in the support to use
> > hist_entry to sort on physid.
> >
> > V2: use new mmap2 sort
> >
> > Signed-off-by: Don Zickus <[email protected]>
> > ---
> > tools/perf/builtin-c2c.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 51 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> > index d7eaf81..b5742bd 100644
> > --- a/tools/perf/builtin-c2c.c
> > +++ b/tools/perf/builtin-c2c.c
> > @@ -7,6 +7,7 @@
> > #include "util/tool.h"
> > #include "util/debug.h"
> > #include <api/fs/fs.h>
> > +#include "util/annotate.h"
>
> Why is this #include needed?
>
>
> [SNIP]
> > @@ -163,7 +169,33 @@ static int perf_c2c__process_load_store(struct perf_c2c *c2c,
> > return 0;
> > }
> >
> > - return 0;
> > + cost = sample->weight;
> > + if (!cost)
> > + cost = 1;
> > +
> > + /*
> > + * must pass period=weight in order to get the correct
> > + * sorting from hists__collapse_resort() which is solely
> > + * based on periods. We want sorting be done on nr_events * weight
> > + * and this is indirectly achieved by passing period=weight here
> > + * and the he_stat__add_period() function.
> > + */
> > + he = __hists__add_entry(&c2c->hists, al, parent, NULL, mi,
> > + cost, cost, 0);
> > + if (!he) {
> > + err = -ENOMEM;
> > + goto out_mem;
> > + }
> > +
> > + c2c->hists.stats.total_period += cost;
> > + hists__inc_nr_events(&c2c->hists, PERF_RECORD_SAMPLE);
> > + return err;
> > +
> > +out_mem:
> > + /* implicitly freed by __hists__add_entry */
> > + free(mi);
> > +out:
>
> It seems this 'out' lable is not used anywhere..
>
>
> > + return err;
> > }
> >
> > /* setup the basic events for most coverage, options added later */
> > @@ -266,10 +298,28 @@ out:
> > return err;
> > }
> >
> > +static int perf_c2c__init(struct perf_c2c *c2c)
> > +{
> > + sort__mode = SORT_MODE__PHYSID;
> > + sort__wants_unique = 1;
> > + sort_order = "daddr,iaddr,pid,tid";
>
> Where are the SORT_MODE__PHYSID, sort__wants_unique and "daddr", "iaddr"
> sort keys defined?
In a previous patchset that enables the mmap2 interface.
>
> Also, more importantly, I think the sort order should contain at least
> "mem" and "snoop" keys to group samples based on the exact hitm
> information.
I can look into it, but after iaddr, pid and tid, sorting on snoop doesn't
really change anything if I recall. The hitms are scattered across iaddr.
>
> In my understanding, if two samples are captured at exactly same
> position with a same data access but different hitm info, they cannot be
> classified and just use the hitm info of first entry.
Why? If the first hitm access was local and the second one remote,
doesn't that indicate the accessed data is being pulled onto different
nodes?
Cheers,
Don
On Tue, 8 Apr 2014 10:17:58 -0400, Don Zickus wrote:
> On Tue, Apr 08, 2014 at 04:56:25PM +0900, Namhyung Kim wrote:
>> On Mon, 24 Mar 2014 15:36:58 -0400, Don Zickus wrote:
>> > +static int perf_c2c__init(struct perf_c2c *c2c)
>> > +{
>> > + sort__mode = SORT_MODE__PHYSID;
>> > + sort__wants_unique = 1;
>> > + sort_order = "daddr,iaddr,pid,tid";
>>
>> Where are the SORT_MODE__PHYSID, sort__wants_unique and "daddr", "iaddr"
>> sort keys defined?
>
> In a previous patchset that enables the mmap2 interface.
Ah, missed that.. will look at it soon.
>
>>
>> Also, more importantly, I think the sort order should contain at least
>> "mem" and "snoop" keys to group samples based on the exact hitm
>> information.
>
> I can look into it, but after iaddr, pid and tid, sorting on snoop doesn't
> really change anything if I recall. The hitms are scattered across iaddr.
But it doesn't guarantee that all hitms are scattered, right? Also if
it's the case I guess adding more sort keys are not harmful since they
don't even have a chance to test.
I think you can check hist_entry->stat.nr_events always being 1.
>
>>
>> In my understanding, if two samples are captured at exactly same
>> position with a same data access but different hitm info, they cannot be
>> classified and just use the hitm info of first entry.
>
> Why? If the first hitm access was local and the second one remote,
> doesn't that indicate the accessed data is being pulled onto different
> nodes?
But "hist_entry" won't have the information after calling
__hists__add__entry() called unless it has 'mem' and 'snoop' sort keys.
Since two samples have same daddr, iaddr, pid and tid, it'd think those
two samples are same and then add stats of second one to the first and
finally discard the second. So first one will have a double weight for
the local hitm case only.
This is the case what I worry about. Am I missing something?
Thanks,
Namhyung
On Wed, Apr 09, 2014 at 10:30:56AM +0900, Namhyung Kim wrote:
> On Tue, 8 Apr 2014 10:17:58 -0400, Don Zickus wrote:
> > On Tue, Apr 08, 2014 at 04:56:25PM +0900, Namhyung Kim wrote:
> >> On Mon, 24 Mar 2014 15:36:58 -0400, Don Zickus wrote:
> >> > +static int perf_c2c__init(struct perf_c2c *c2c)
> >> > +{
> >> > + sort__mode = SORT_MODE__PHYSID;
> >> > + sort__wants_unique = 1;
> >> > + sort_order = "daddr,iaddr,pid,tid";
> >>
> >> Where are the SORT_MODE__PHYSID, sort__wants_unique and "daddr", "iaddr"
> >> sort keys defined?
> >
> > In a previous patchset that enables the mmap2 interface.
>
> Ah, missed that.. will look at it soon.
>
> >
> >>
> >> Also, more importantly, I think the sort order should contain at least
> >> "mem" and "snoop" keys to group samples based on the exact hitm
> >> information.
> >
> > I can look into it, but after iaddr, pid and tid, sorting on snoop doesn't
> > really change anything if I recall. The hitms are scattered across iaddr.
>
> But it doesn't guarantee that all hitms are scattered, right? Also if
> it's the case I guess adding more sort keys are not harmful since they
> don't even have a chance to test.
>
> I think you can check hist_entry->stat.nr_events always being 1.
>
> >
> >>
> >> In my understanding, if two samples are captured at exactly same
> >> position with a same data access but different hitm info, they cannot be
> >> classified and just use the hitm info of first entry.
> >
> > Why? If the first hitm access was local and the second one remote,
> > doesn't that indicate the accessed data is being pulled onto different
> > nodes?
>
> But "hist_entry" won't have the information after calling
> __hists__add__entry() called unless it has 'mem' and 'snoop' sort keys.
> Since two samples have same daddr, iaddr, pid and tid, it'd think those
> two samples are same and then add stats of second one to the first and
> finally discard the second. So first one will have a double weight for
> the local hitm case only.
>
> This is the case what I worry about. Am I missing something?
My patch 6/6 of the enable mmap2 support. :-) It specifically forces all
the data to remain separate to avoid this issue. We couldn't have the
data merged because it messed up our stats.
Cheers,
Don