public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] perf stat: fix per-pkg event reporting bug
@ 2015-09-02 13:17 Stephane Eranian
  2015-09-02 20:26 ` Andi Kleen
  2015-09-03 10:01 ` Jiri Olsa
  0 siblings, 2 replies; 14+ messages in thread
From: Stephane Eranian @ 2015-09-02 13:17 UTC (permalink / raw)
  To: linux-kernel
  Cc: acme, peterz, mingo, ak, jolsa, namhyung, kan.liang, dsahern,
	adrian.hunter

Per-pkg events need to be captured once per processor
socket. The code in check_per_pkg() ensures only one
value per processor package is used. However there is
a problem with this function in case the first CPU of
the package does not measure anything for the per-pkg event,
but other CPUs do.

Consider the following

$ create cgroup FOO; echo $$ >FOO/tasks; taskset -c 1 noploop &
$ perf stat -a -I 1000 -e intel_cqm/llc_occupancy/ -G FOO sleep 100
  1.00000 <not counted> Bytes intel_cqm/llc_occupancy/  FOO

The reason for this is that CPU0 in the cgrop has nothing running on it.
Yet check_per_plg() will mark socket0 as processed and no other event
value will be considered for the socket.

This patch fixes the problem by having check_per_pkg() only consider
events which actually ran.

Patch is relative to tip.git.

Signed-off-by: Stephane Eranian <eranian@google.com>
---
 tools/perf/util/stat.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
index 415c359..f1d8359 100644
--- a/tools/perf/util/stat.c
+++ b/tools/perf/util/stat.c
@@ -196,7 +196,8 @@ static void zero_per_pkg(struct perf_evsel *counter)
 		memset(counter->per_pkg_mask, 0, MAX_NR_CPUS);
 }
 
-static int check_per_pkg(struct perf_evsel *counter, int cpu, bool *skip)
+static int check_per_pkg(struct perf_evsel *counter,
+			 struct perf_counts_values *vals, int cpu, bool *skip)
 {
 	unsigned long *mask = counter->per_pkg_mask;
 	struct cpu_map *cpus = perf_evsel__cpus(counter);
@@ -218,6 +219,17 @@ static int check_per_pkg(struct perf_evsel *counter, int cpu, bool *skip)
 		counter->per_pkg_mask = mask;
 	}
 
+	/*
+	 * we do not consider an event that has not run as a good
+	 * instance to mark a package as used (skip=1). Otherwise
+	 * we may run into a situation where the first CPU in a package
+	 * is not running anything, yet the second is, and this function
+	 * would mark the package as used after the first CPU and would
+	 * not read the values from the second CPU.
+	 */
+	if (!(vals->run && vals->ena))
+		return 0;
+
 	s = cpu_map__get_socket(cpus, cpu);
 	if (s < 0)
 		return -1;
@@ -235,7 +247,7 @@ process_counter_values(struct perf_stat_config *config, struct perf_evsel *evsel
 	static struct perf_counts_values zero;
 	bool skip = false;
 
-	if (check_per_pkg(evsel, cpu, &skip)) {
+	if (check_per_pkg(evsel, aggr, cpu, &skip)) {
 		pr_err("failed to read per-pkg counter\n");
 		return -1;
 	}
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] perf stat: fix per-pkg event reporting bug
  2015-09-02 13:17 [PATCH] perf stat: fix per-pkg event reporting bug Stephane Eranian
@ 2015-09-02 20:26 ` Andi Kleen
  2015-09-03 10:05   ` Jiri Olsa
  2015-09-03 10:01 ` Jiri Olsa
  1 sibling, 1 reply; 14+ messages in thread
From: Andi Kleen @ 2015-09-02 20:26 UTC (permalink / raw)
  To: Stephane Eranian
  Cc: linux-kernel, acme, peterz, mingo, jolsa, namhyung, kan.liang,
	dsahern, adrian.hunter

On Wed, Sep 02, 2015 at 03:17:51PM +0200, Stephane Eranian wrote:
> Per-pkg events need to be captured once per processor
> socket. The code in check_per_pkg() ensures only one
> value per processor package is used. However there is
> a problem with this function in case the first CPU of
> the package does not measure anything for the per-pkg event,
> but other CPUs do.

I've seen a similar(?) bug with -C and --per-core combined.
Some logical threads are not correctly accounted there either.
Do you think that's another instance of such a bug?

-Andi

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] perf stat: fix per-pkg event reporting bug
  2015-09-02 13:17 [PATCH] perf stat: fix per-pkg event reporting bug Stephane Eranian
  2015-09-02 20:26 ` Andi Kleen
@ 2015-09-03 10:01 ` Jiri Olsa
  2015-09-03 11:48   ` Stephane Eranian
  1 sibling, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2015-09-03 10:01 UTC (permalink / raw)
  To: Stephane Eranian
  Cc: linux-kernel, acme, peterz, mingo, ak, namhyung, kan.liang,
	dsahern, adrian.hunter

On Wed, Sep 02, 2015 at 03:17:51PM +0200, Stephane Eranian wrote:

SNIP

> +	/*
> +	 * we do not consider an event that has not run as a good
> +	 * instance to mark a package as used (skip=1). Otherwise
> +	 * we may run into a situation where the first CPU in a package
> +	 * is not running anything, yet the second is, and this function
> +	 * would mark the package as used after the first CPU and would
> +	 * not read the values from the second CPU.
> +	 */
> +	if (!(vals->run && vals->ena))
> +		return 0;
> +
>  	s = cpu_map__get_socket(cpus, cpu);
>  	if (s < 0)
>  		return -1;
> @@ -235,7 +247,7 @@ process_counter_values(struct perf_stat_config *config, struct perf_evsel *evsel
>  	static struct perf_counts_values zero;
>  	bool skip = false;
>  
> -	if (check_per_pkg(evsel, cpu, &skip)) {
> +	if (check_per_pkg(evsel, aggr, cpu, &skip)) {

should we pass 'count' instead o 'aggr' ?

jirka

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] perf stat: fix per-pkg event reporting bug
  2015-09-02 20:26 ` Andi Kleen
@ 2015-09-03 10:05   ` Jiri Olsa
  0 siblings, 0 replies; 14+ messages in thread
From: Jiri Olsa @ 2015-09-03 10:05 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Stephane Eranian, linux-kernel, acme, peterz, mingo, namhyung,
	kan.liang, dsahern, adrian.hunter

On Wed, Sep 02, 2015 at 01:26:57PM -0700, Andi Kleen wrote:
> On Wed, Sep 02, 2015 at 03:17:51PM +0200, Stephane Eranian wrote:
> > Per-pkg events need to be captured once per processor
> > socket. The code in check_per_pkg() ensures only one
> > value per processor package is used. However there is
> > a problem with this function in case the first CPU of
> > the package does not measure anything for the per-pkg event,
> > but other CPUs do.
> 
> I've seen a similar(?) bug with -C and --per-core combined.
> Some logical threads are not correctly accounted there either.
> Do you think that's another instance of such a bug?

AFAICS this is related strictly to per-pkg events,
is this what you were doing?

jirka

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] perf stat: fix per-pkg event reporting bug
  2015-09-03 10:01 ` Jiri Olsa
@ 2015-09-03 11:48   ` Stephane Eranian
  2015-09-03 12:01     ` Jiri Olsa
  0 siblings, 1 reply; 14+ messages in thread
From: Stephane Eranian @ 2015-09-03 11:48 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: LKML, Arnaldo Carvalho de Melo, Peter Zijlstra, mingo@elte.hu,
	ak@linux.intel.com, Namhyung Kim, Liang, Kan, David Ahern,
	Adrian Hunter

On Thu, Sep 3, 2015 at 3:01 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Wed, Sep 02, 2015 at 03:17:51PM +0200, Stephane Eranian wrote:
>
> SNIP
>
>> +     /*
>> +      * we do not consider an event that has not run as a good
>> +      * instance to mark a package as used (skip=1). Otherwise
>> +      * we may run into a situation where the first CPU in a package
>> +      * is not running anything, yet the second is, and this function
>> +      * would mark the package as used after the first CPU and would
>> +      * not read the values from the second CPU.
>> +      */
>> +     if (!(vals->run && vals->ena))
>> +             return 0;
>> +
>>       s = cpu_map__get_socket(cpus, cpu);
>>       if (s < 0)
>>               return -1;
>> @@ -235,7 +247,7 @@ process_counter_values(struct perf_stat_config *config, struct perf_evsel *evsel
>>       static struct perf_counts_values zero;
>>       bool skip = false;
>>
>> -     if (check_per_pkg(evsel, cpu, &skip)) {
>> +     if (check_per_pkg(evsel, aggr, cpu, &skip)) {
>
> should we pass 'count' instead o 'aggr' ?
>
the reason I passed counts_values is in case this function needs to be
called from other places which do
not use aggr mode.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] perf stat: fix per-pkg event reporting bug
  2015-09-03 11:48   ` Stephane Eranian
@ 2015-09-03 12:01     ` Jiri Olsa
  2015-09-03 12:05       ` Stephane Eranian
  0 siblings, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2015-09-03 12:01 UTC (permalink / raw)
  To: Stephane Eranian
  Cc: LKML, Arnaldo Carvalho de Melo, Peter Zijlstra, mingo@elte.hu,
	ak@linux.intel.com, Namhyung Kim, Liang, Kan, David Ahern,
	Adrian Hunter

On Thu, Sep 03, 2015 at 04:48:52AM -0700, Stephane Eranian wrote:
> On Thu, Sep 3, 2015 at 3:01 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> > On Wed, Sep 02, 2015 at 03:17:51PM +0200, Stephane Eranian wrote:
> >
> > SNIP
> >
> >> +     /*
> >> +      * we do not consider an event that has not run as a good
> >> +      * instance to mark a package as used (skip=1). Otherwise
> >> +      * we may run into a situation where the first CPU in a package
> >> +      * is not running anything, yet the second is, and this function
> >> +      * would mark the package as used after the first CPU and would
> >> +      * not read the values from the second CPU.
> >> +      */
> >> +     if (!(vals->run && vals->ena))
> >> +             return 0;
> >> +
> >>       s = cpu_map__get_socket(cpus, cpu);
> >>       if (s < 0)
> >>               return -1;
> >> @@ -235,7 +247,7 @@ process_counter_values(struct perf_stat_config *config, struct perf_evsel *evsel
> >>       static struct perf_counts_values zero;
> >>       bool skip = false;
> >>
> >> -     if (check_per_pkg(evsel, cpu, &skip)) {
> >> +     if (check_per_pkg(evsel, aggr, cpu, &skip)) {
> >
> > should we pass 'count' instead o 'aggr' ?
> >
> the reason I passed counts_values is in case this function needs to be
> called from other places which do
> not use aggr mode.

sure, but 'aggr' is being computed within process_counter_values

process_counter_values gets 'count' argument with values read
for given cpu/thread for further processing, and it seems to
me that 'count' values should be passed to check_per_pkg

jirka

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] perf stat: fix per-pkg event reporting bug
  2015-09-03 12:01     ` Jiri Olsa
@ 2015-09-03 12:05       ` Stephane Eranian
  2015-09-03 12:13         ` Jiri Olsa
  0 siblings, 1 reply; 14+ messages in thread
From: Stephane Eranian @ 2015-09-03 12:05 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: LKML, Arnaldo Carvalho de Melo, Peter Zijlstra, mingo@elte.hu,
	ak@linux.intel.com, Namhyung Kim, Liang, Kan, David Ahern,
	Adrian Hunter

On Thu, Sep 3, 2015 at 5:01 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Thu, Sep 03, 2015 at 04:48:52AM -0700, Stephane Eranian wrote:
>> On Thu, Sep 3, 2015 at 3:01 AM, Jiri Olsa <jolsa@redhat.com> wrote:
>> > On Wed, Sep 02, 2015 at 03:17:51PM +0200, Stephane Eranian wrote:
>> >
>> > SNIP
>> >
>> >> +     /*
>> >> +      * we do not consider an event that has not run as a good
>> >> +      * instance to mark a package as used (skip=1). Otherwise
>> >> +      * we may run into a situation where the first CPU in a package
>> >> +      * is not running anything, yet the second is, and this function
>> >> +      * would mark the package as used after the first CPU and would
>> >> +      * not read the values from the second CPU.
>> >> +      */
>> >> +     if (!(vals->run && vals->ena))
>> >> +             return 0;
>> >> +
>> >>       s = cpu_map__get_socket(cpus, cpu);
>> >>       if (s < 0)
>> >>               return -1;
>> >> @@ -235,7 +247,7 @@ process_counter_values(struct perf_stat_config *config, struct perf_evsel *evsel
>> >>       static struct perf_counts_values zero;
>> >>       bool skip = false;
>> >>
>> >> -     if (check_per_pkg(evsel, cpu, &skip)) {
>> >> +     if (check_per_pkg(evsel, aggr, cpu, &skip)) {
>> >
>> > should we pass 'count' instead o 'aggr' ?
>> >
>> the reason I passed counts_values is in case this function needs to be
>> called from other places which do
>> not use aggr mode.
>
> sure, but 'aggr' is being computed within process_counter_values
>
> process_counter_values gets 'count' argument with values read
> for given cpu/thread for further processing, and it seems to
> me that 'count' values should be passed to check_per_pkg
>
You do not want to aggregate values, you want to look at the individual events
for each CPU because you need to look at their run/ena fields.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] perf stat: fix per-pkg event reporting bug
  2015-09-03 12:05       ` Stephane Eranian
@ 2015-09-03 12:13         ` Jiri Olsa
  2015-09-03 12:16           ` Stephane Eranian
  0 siblings, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2015-09-03 12:13 UTC (permalink / raw)
  To: Stephane Eranian
  Cc: LKML, Arnaldo Carvalho de Melo, Peter Zijlstra, mingo@elte.hu,
	ak@linux.intel.com, Namhyung Kim, Liang, Kan, David Ahern,
	Adrian Hunter

On Thu, Sep 03, 2015 at 05:05:32AM -0700, Stephane Eranian wrote:
> On Thu, Sep 3, 2015 at 5:01 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> > On Thu, Sep 03, 2015 at 04:48:52AM -0700, Stephane Eranian wrote:
> >> On Thu, Sep 3, 2015 at 3:01 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> >> > On Wed, Sep 02, 2015 at 03:17:51PM +0200, Stephane Eranian wrote:
> >> >
> >> > SNIP
> >> >
> >> >> +     /*
> >> >> +      * we do not consider an event that has not run as a good
> >> >> +      * instance to mark a package as used (skip=1). Otherwise
> >> >> +      * we may run into a situation where the first CPU in a package
> >> >> +      * is not running anything, yet the second is, and this function
> >> >> +      * would mark the package as used after the first CPU and would
> >> >> +      * not read the values from the second CPU.
> >> >> +      */
> >> >> +     if (!(vals->run && vals->ena))
> >> >> +             return 0;
> >> >> +
> >> >>       s = cpu_map__get_socket(cpus, cpu);
> >> >>       if (s < 0)
> >> >>               return -1;
> >> >> @@ -235,7 +247,7 @@ process_counter_values(struct perf_stat_config *config, struct perf_evsel *evsel
> >> >>       static struct perf_counts_values zero;
> >> >>       bool skip = false;
> >> >>
> >> >> -     if (check_per_pkg(evsel, cpu, &skip)) {
> >> >> +     if (check_per_pkg(evsel, aggr, cpu, &skip)) {
> >> >
> >> > should we pass 'count' instead o 'aggr' ?
> >> >
> >> the reason I passed counts_values is in case this function needs to be
> >> called from other places which do
> >> not use aggr mode.
> >
> > sure, but 'aggr' is being computed within process_counter_values
> >
> > process_counter_values gets 'count' argument with values read
> > for given cpu/thread for further processing, and it seems to
> > me that 'count' values should be passed to check_per_pkg
> >
> You do not want to aggregate values, you want to look at the individual events
> for each CPU because you need to look at their run/ena fields.

yes, but for 'count' not 'aggr'

jirka


---
diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
index f1d83599217b..2d065d065b67 100644
--- a/tools/perf/util/stat.c
+++ b/tools/perf/util/stat.c
@@ -247,7 +247,7 @@ process_counter_values(struct perf_stat_config *config, struct perf_evsel *evsel
 	static struct perf_counts_values zero;
 	bool skip = false;
 
-	if (check_per_pkg(evsel, aggr, cpu, &skip)) {
+	if (check_per_pkg(evsel, count, cpu, &skip)) {
 		pr_err("failed to read per-pkg counter\n");
 		return -1;
 	}

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] perf stat: fix per-pkg event reporting bug
  2015-09-03 12:13         ` Jiri Olsa
@ 2015-09-03 12:16           ` Stephane Eranian
  2015-09-03 12:25             ` Jiri Olsa
  0 siblings, 1 reply; 14+ messages in thread
From: Stephane Eranian @ 2015-09-03 12:16 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: LKML, Arnaldo Carvalho de Melo, Peter Zijlstra, mingo@elte.hu,
	ak@linux.intel.com, Namhyung Kim, Liang, Kan, David Ahern,
	Adrian Hunter

On Thu, Sep 3, 2015 at 5:13 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Thu, Sep 03, 2015 at 05:05:32AM -0700, Stephane Eranian wrote:
>> On Thu, Sep 3, 2015 at 5:01 AM, Jiri Olsa <jolsa@redhat.com> wrote:
>> > On Thu, Sep 03, 2015 at 04:48:52AM -0700, Stephane Eranian wrote:
>> >> On Thu, Sep 3, 2015 at 3:01 AM, Jiri Olsa <jolsa@redhat.com> wrote:
>> >> > On Wed, Sep 02, 2015 at 03:17:51PM +0200, Stephane Eranian wrote:
>> >> >
>> >> > SNIP
>> >> >
>> >> >> +     /*
>> >> >> +      * we do not consider an event that has not run as a good
>> >> >> +      * instance to mark a package as used (skip=1). Otherwise
>> >> >> +      * we may run into a situation where the first CPU in a package
>> >> >> +      * is not running anything, yet the second is, and this function
>> >> >> +      * would mark the package as used after the first CPU and would
>> >> >> +      * not read the values from the second CPU.
>> >> >> +      */
>> >> >> +     if (!(vals->run && vals->ena))
>> >> >> +             return 0;
>> >> >> +
>> >> >>       s = cpu_map__get_socket(cpus, cpu);
>> >> >>       if (s < 0)
>> >> >>               return -1;
>> >> >> @@ -235,7 +247,7 @@ process_counter_values(struct perf_stat_config *config, struct perf_evsel *evsel
>> >> >>       static struct perf_counts_values zero;
>> >> >>       bool skip = false;
>> >> >>
>> >> >> -     if (check_per_pkg(evsel, cpu, &skip)) {
>> >> >> +     if (check_per_pkg(evsel, aggr, cpu, &skip)) {
>> >> >
>> >> > should we pass 'count' instead o 'aggr' ?
>> >> >
>> >> the reason I passed counts_values is in case this function needs to be
>> >> called from other places which do
>> >> not use aggr mode.
>> >
>> > sure, but 'aggr' is being computed within process_counter_values
>> >
>> > process_counter_values gets 'count' argument with values read
>> > for given cpu/thread for further processing, and it seems to
>> > me that 'count' values should be passed to check_per_pkg
>> >
>> You do not want to aggregate values, you want to look at the individual events
>> for each CPU because you need to look at their run/ena fields.
>
> yes, but for 'count' not 'aggr'
>
Ah, yes, sorry, has to be count and not aggr. Sent the wrong version.
Can you fix it? Or do you want me to resubmit?

> jirka
>
>
> ---
> diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
> index f1d83599217b..2d065d065b67 100644
> --- a/tools/perf/util/stat.c
> +++ b/tools/perf/util/stat.c
> @@ -247,7 +247,7 @@ process_counter_values(struct perf_stat_config *config, struct perf_evsel *evsel
>         static struct perf_counts_values zero;
>         bool skip = false;
>
> -       if (check_per_pkg(evsel, aggr, cpu, &skip)) {
> +       if (check_per_pkg(evsel, count, cpu, &skip)) {
>                 pr_err("failed to read per-pkg counter\n");
>                 return -1;
>         }

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] perf stat: fix per-pkg event reporting bug
  2015-09-03 12:16           ` Stephane Eranian
@ 2015-09-03 12:25             ` Jiri Olsa
  2015-09-03 16:53               ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2015-09-03 12:25 UTC (permalink / raw)
  To: Stephane Eranian
  Cc: LKML, Arnaldo Carvalho de Melo, Peter Zijlstra, mingo@elte.hu,
	ak@linux.intel.com, Namhyung Kim, Liang, Kan, David Ahern,
	Adrian Hunter

On Thu, Sep 03, 2015 at 05:16:41AM -0700, Stephane Eranian wrote:
> On Thu, Sep 3, 2015 at 5:13 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> > On Thu, Sep 03, 2015 at 05:05:32AM -0700, Stephane Eranian wrote:
> >> On Thu, Sep 3, 2015 at 5:01 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> >> > On Thu, Sep 03, 2015 at 04:48:52AM -0700, Stephane Eranian wrote:
> >> >> On Thu, Sep 3, 2015 at 3:01 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> >> >> > On Wed, Sep 02, 2015 at 03:17:51PM +0200, Stephane Eranian wrote:
> >> >> >
> >> >> > SNIP
> >> >> >
> >> >> >> +     /*
> >> >> >> +      * we do not consider an event that has not run as a good
> >> >> >> +      * instance to mark a package as used (skip=1). Otherwise
> >> >> >> +      * we may run into a situation where the first CPU in a package
> >> >> >> +      * is not running anything, yet the second is, and this function
> >> >> >> +      * would mark the package as used after the first CPU and would
> >> >> >> +      * not read the values from the second CPU.
> >> >> >> +      */
> >> >> >> +     if (!(vals->run && vals->ena))
> >> >> >> +             return 0;
> >> >> >> +
> >> >> >>       s = cpu_map__get_socket(cpus, cpu);
> >> >> >>       if (s < 0)
> >> >> >>               return -1;
> >> >> >> @@ -235,7 +247,7 @@ process_counter_values(struct perf_stat_config *config, struct perf_evsel *evsel
> >> >> >>       static struct perf_counts_values zero;
> >> >> >>       bool skip = false;
> >> >> >>
> >> >> >> -     if (check_per_pkg(evsel, cpu, &skip)) {
> >> >> >> +     if (check_per_pkg(evsel, aggr, cpu, &skip)) {
> >> >> >
> >> >> > should we pass 'count' instead o 'aggr' ?
> >> >> >
> >> >> the reason I passed counts_values is in case this function needs to be
> >> >> called from other places which do
> >> >> not use aggr mode.
> >> >
> >> > sure, but 'aggr' is being computed within process_counter_values
> >> >
> >> > process_counter_values gets 'count' argument with values read
> >> > for given cpu/thread for further processing, and it seems to
> >> > me that 'count' values should be passed to check_per_pkg
> >> >
> >> You do not want to aggregate values, you want to look at the individual events
> >> for each CPU because you need to look at their run/ena fields.
> >
> > yes, but for 'count' not 'aggr'
> >
> Ah, yes, sorry, has to be count and not aggr. Sent the wrong version.
> Can you fix it? Or do you want me to resubmit?

well, Arnaldo will queue it.. leaving up to him ;-)

jirka

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] perf stat: fix per-pkg event reporting bug
  2015-09-03 12:25             ` Jiri Olsa
@ 2015-09-03 16:53               ` Arnaldo Carvalho de Melo
  2015-09-03 17:12                 ` Stephane Eranian
  0 siblings, 1 reply; 14+ messages in thread
From: Arnaldo Carvalho de Melo @ 2015-09-03 16:53 UTC (permalink / raw)
  To: Stephane Eranian
  Cc: Jiri Olsa, LKML, Peter Zijlstra, mingo@elte.hu,
	ak@linux.intel.com, Namhyung Kim, Liang, Kan, David Ahern,
	Adrian Hunter

Em Thu, Sep 03, 2015 at 02:25:44PM +0200, Jiri Olsa escreveu:
> On Thu, Sep 03, 2015 at 05:16:41AM -0700, Stephane Eranian wrote:
> > On Thu, Sep 3, 2015 at 5:13 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> > > yes, but for 'count' not 'aggr'

> > Ah, yes, sorry, has to be count and not aggr. Sent the wrong version.
> > Can you fix it? Or do you want me to resubmit?
> 
> well, Arnaldo will queue it.. leaving up to him ;-)

Please resubmit, with a [PATCH v2 ...]  in it, and with a v2 right
before your Signed-off-by: stating what you changed, that helps when I
see multiple patches, i.e. you document what was changed and I don't
have to follow that many threads :-)

- Arnaldo

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] perf stat: fix per-pkg event reporting bug
  2015-09-03 16:53               ` Arnaldo Carvalho de Melo
@ 2015-09-03 17:12                 ` Stephane Eranian
  2015-09-03 20:42                   ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 14+ messages in thread
From: Stephane Eranian @ 2015-09-03 17:12 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Jiri Olsa, LKML, Peter Zijlstra, mingo@elte.hu,
	ak@linux.intel.com, Namhyung Kim, Liang, Kan, David Ahern,
	Adrian Hunter

On Thu, Sep 3, 2015 at 9:53 AM, Arnaldo Carvalho de Melo
<acme@redhat.com> wrote:
> Em Thu, Sep 03, 2015 at 02:25:44PM +0200, Jiri Olsa escreveu:
>> On Thu, Sep 03, 2015 at 05:16:41AM -0700, Stephane Eranian wrote:
>> > On Thu, Sep 3, 2015 at 5:13 AM, Jiri Olsa <jolsa@redhat.com> wrote:
>> > > yes, but for 'count' not 'aggr'
>
>> > Ah, yes, sorry, has to be count and not aggr. Sent the wrong version.
>> > Can you fix it? Or do you want me to resubmit?
>>
>> well, Arnaldo will queue it.. leaving up to him ;-)
>
> Please resubmit, with a [PATCH v2 ...]  in it, and with a v2 right
> before your Signed-off-by: stating what you changed, that helps when I
> see multiple patches, i.e. you document what was changed and I don't
> have to follow that many threads :-)
>
I already sent the V2, but I forgot to state the small change.
Do you want a V3? Or you add: fix perf_counter_value argument to check_per_pkg.
Thanks.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] perf stat: fix per-pkg event reporting bug
  2015-09-03 17:12                 ` Stephane Eranian
@ 2015-09-03 20:42                   ` Arnaldo Carvalho de Melo
  2015-09-03 21:37                     ` Stephane Eranian
  0 siblings, 1 reply; 14+ messages in thread
From: Arnaldo Carvalho de Melo @ 2015-09-03 20:42 UTC (permalink / raw)
  To: Stephane Eranian
  Cc: Jiri Olsa, LKML, Peter Zijlstra, mingo@elte.hu,
	ak@linux.intel.com, Namhyung Kim, Liang, Kan, David Ahern,
	Adrian Hunter

Em Thu, Sep 03, 2015 at 10:12:12AM -0700, Stephane Eranian escreveu:
> On Thu, Sep 3, 2015 at 9:53 AM, Arnaldo Carvalho de Melo
> <acme@redhat.com> wrote:
> > Em Thu, Sep 03, 2015 at 02:25:44PM +0200, Jiri Olsa escreveu:
> >> On Thu, Sep 03, 2015 at 05:16:41AM -0700, Stephane Eranian wrote:
> >> > On Thu, Sep 3, 2015 at 5:13 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> >> > > yes, but for 'count' not 'aggr'
> >
> >> > Ah, yes, sorry, has to be count and not aggr. Sent the wrong version.
> >> > Can you fix it? Or do you want me to resubmit?
> >>
> >> well, Arnaldo will queue it.. leaving up to him ;-)
> >
> > Please resubmit, with a [PATCH v2 ...]  in it, and with a v2 right
> > before your Signed-off-by: stating what you changed, that helps when I
> > see multiple patches, i.e. you document what was changed and I don't
> > have to follow that many threads :-)
> >
> I already sent the V2, but I forgot to state the small change.
> Do you want a V3? Or you add: fix perf_counter_value argument to check_per_pkg.
> Thanks.

Ok, I can do it...

- Arnaldo

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] perf stat: fix per-pkg event reporting bug
  2015-09-03 20:42                   ` Arnaldo Carvalho de Melo
@ 2015-09-03 21:37                     ` Stephane Eranian
  0 siblings, 0 replies; 14+ messages in thread
From: Stephane Eranian @ 2015-09-03 21:37 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Jiri Olsa, LKML, Peter Zijlstra, mingo@elte.hu,
	ak@linux.intel.com, Namhyung Kim, Liang, Kan, David Ahern,
	Adrian Hunter

On Thu, Sep 3, 2015 at 1:42 PM, Arnaldo Carvalho de Melo
<acme@redhat.com> wrote:
> Em Thu, Sep 03, 2015 at 10:12:12AM -0700, Stephane Eranian escreveu:
>> On Thu, Sep 3, 2015 at 9:53 AM, Arnaldo Carvalho de Melo
>> <acme@redhat.com> wrote:
>> > Em Thu, Sep 03, 2015 at 02:25:44PM +0200, Jiri Olsa escreveu:
>> >> On Thu, Sep 03, 2015 at 05:16:41AM -0700, Stephane Eranian wrote:
>> >> > On Thu, Sep 3, 2015 at 5:13 AM, Jiri Olsa <jolsa@redhat.com> wrote:
>> >> > > yes, but for 'count' not 'aggr'
>> >
>> >> > Ah, yes, sorry, has to be count and not aggr. Sent the wrong version.
>> >> > Can you fix it? Or do you want me to resubmit?
>> >>
>> >> well, Arnaldo will queue it.. leaving up to him ;-)
>> >
>> > Please resubmit, with a [PATCH v2 ...]  in it, and with a v2 right
>> > before your Signed-off-by: stating what you changed, that helps when I
>> > see multiple patches, i.e. you document what was changed and I don't
>> > have to follow that many threads :-)
>> >
>> I already sent the V2, but I forgot to state the small change.
>> Do you want a V3? Or you add: fix perf_counter_value argument to check_per_pkg.
>> Thanks.
>
> Ok, I can do it...
>
Thanks.

> - Arnaldo

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-09-03 21:38 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-02 13:17 [PATCH] perf stat: fix per-pkg event reporting bug Stephane Eranian
2015-09-02 20:26 ` Andi Kleen
2015-09-03 10:05   ` Jiri Olsa
2015-09-03 10:01 ` Jiri Olsa
2015-09-03 11:48   ` Stephane Eranian
2015-09-03 12:01     ` Jiri Olsa
2015-09-03 12:05       ` Stephane Eranian
2015-09-03 12:13         ` Jiri Olsa
2015-09-03 12:16           ` Stephane Eranian
2015-09-03 12:25             ` Jiri Olsa
2015-09-03 16:53               ` Arnaldo Carvalho de Melo
2015-09-03 17:12                 ` Stephane Eranian
2015-09-03 20:42                   ` Arnaldo Carvalho de Melo
2015-09-03 21:37                     ` Stephane Eranian

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox