Adding AGGR_PCIE_PORT mode to be able to distinguish aggr_mode
for root ports in following patches.
Signed-off-by: Alexander Antonov <[email protected]>
---
tools/perf/builtin-stat.c | 5 ++++-
.../util/scripting-engines/trace-event-python.c | 3 ++-
tools/perf/util/stat-display.c | 13 +++++++++++--
tools/perf/util/stat.c | 4 +++-
tools/perf/util/stat.h | 1 +
5 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 468fc49420ce..60fdb6a0805f 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -908,6 +908,7 @@ static int perf_stat_init_aggr_mode(void)
break;
case AGGR_GLOBAL:
case AGGR_THREAD:
+ case AGGR_PCIE_PORT:
case AGGR_UNSET:
default:
break;
@@ -1072,6 +1073,7 @@ static int perf_stat_init_aggr_mode_file(struct perf_stat *st)
case AGGR_NONE:
case AGGR_GLOBAL:
case AGGR_THREAD:
+ case AGGR_PCIE_PORT:
case AGGR_UNSET:
default:
break;
@@ -1844,7 +1846,8 @@ int cmd_stat(int argc, const char **argv)
* --per-thread is aggregated per thread, we dont mix it with cpu mode
*/
if (((stat_config.aggr_mode != AGGR_GLOBAL &&
- stat_config.aggr_mode != AGGR_THREAD) || nr_cgroups) &&
+ stat_config.aggr_mode != AGGR_THREAD &&
+ stat_config.aggr_mode != AGGR_PCIE_PORT) || nr_cgroups) &&
!target__has_cpu(&target)) {
fprintf(stderr, "both cgroup and no-aggregation "
"modes only available in system-wide mode\n");
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index 5d341efc3237..e604c199f493 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -1396,7 +1396,8 @@ static void python_process_stat(struct perf_stat_config *config,
struct perf_cpu_map *cpus = counter->core.cpus;
int cpu, thread;
- if (config->aggr_mode == AGGR_GLOBAL) {
+ if (config->aggr_mode == AGGR_GLOBAL ||
+ config->aggr_mode == AGGR_PCIE_PORT) {
process_stat(counter, -1, -1, tstamp,
&counter->counts->aggr);
return;
diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index ed3b0ac2f785..db1bec115d0b 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -123,6 +123,7 @@ static void aggr_printout(struct perf_stat_config *config,
config->csv_sep);
break;
case AGGR_GLOBAL:
+ case AGGR_PCIE_PORT:
case AGGR_UNSET:
default:
break;
@@ -322,7 +323,8 @@ static int first_shadow_cpu(struct perf_stat_config *config,
if (config->aggr_mode == AGGR_NONE)
return id;
- if (config->aggr_mode == AGGR_GLOBAL)
+ if (config->aggr_mode == AGGR_GLOBAL ||
+ config->aggr_mode == AGGR_PCIE_PORT)
return 0;
for (i = 0; i < perf_evsel__nr_cpus(evsel); i++) {
@@ -416,6 +418,7 @@ static void printout(struct perf_stat_config *config, int id, int nr,
if (config->csv_output && !config->metric_only) {
static int aggr_fields[] = {
[AGGR_GLOBAL] = 0,
+ [AGGR_PCIE_PORT] = 0,
[AGGR_THREAD] = 1,
[AGGR_NONE] = 1,
[AGGR_SOCKET] = 2,
@@ -899,6 +902,7 @@ static int aggr_header_lens[] = {
[AGGR_NONE] = 6,
[AGGR_THREAD] = 24,
[AGGR_GLOBAL] = 0,
+ [AGGR_PCIE_PORT] = 0,
};
static const char *aggr_header_csv[] = {
@@ -907,7 +911,8 @@ static const char *aggr_header_csv[] = {
[AGGR_SOCKET] = "socket,cpus",
[AGGR_NONE] = "cpu,",
[AGGR_THREAD] = "comm-pid,",
- [AGGR_GLOBAL] = ""
+ [AGGR_GLOBAL] = "",
+ [AGGR_PCIE_PORT] = "port,"
};
static void print_metric_headers(struct perf_stat_config *config,
@@ -990,6 +995,8 @@ static void print_interval(struct perf_stat_config *config,
if (!metric_only)
fprintf(output, " counts %*s events\n", unit_width, "unit");
break;
+ case AGGR_PCIE_PORT:
+ break;
case AGGR_GLOBAL:
default:
fprintf(output, "# time");
@@ -1214,6 +1221,8 @@ perf_evlist__print_counters(struct evlist *evlist,
}
}
break;
+ case AGGR_PCIE_PORT:
+ break;
case AGGR_UNSET:
default:
break;
diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
index ebdd130557fb..9ffb97b31710 100644
--- a/tools/perf/util/stat.c
+++ b/tools/perf/util/stat.c
@@ -318,6 +318,7 @@ process_counter_values(struct perf_stat_config *config, struct evsel *evsel,
}
break;
case AGGR_GLOBAL:
+ case AGGR_PCIE_PORT:
aggr->val += count->val;
aggr->ena += count->ena;
aggr->run += count->run;
@@ -377,7 +378,8 @@ int perf_stat_process_counter(struct perf_stat_config *config,
if (ret)
return ret;
- if (config->aggr_mode != AGGR_GLOBAL)
+ if (config->aggr_mode != AGGR_GLOBAL &&
+ config->aggr_mode != AGGR_PCIE_PORT)
return 0;
if (!counter->snapshot)
diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
index edbeb2f63e8d..c7544c28c02a 100644
--- a/tools/perf/util/stat.h
+++ b/tools/perf/util/stat.h
@@ -46,6 +46,7 @@ enum aggr_mode {
AGGR_DIE,
AGGR_CORE,
AGGR_THREAD,
+ AGGR_PCIE_PORT,
AGGR_UNSET,
};
--
2.19.1
Hello,
On Wed, Feb 3, 2021 at 10:58 PM Alexander Antonov
<[email protected]> wrote:
>
> Adding AGGR_PCIE_PORT mode to be able to distinguish aggr_mode
> for root ports in following patches.
I'm not sure adding the AGGR_PCIE_PORT is the right way.
In my understanding, the aggr mode is to specify how we aggregate
counter values of a single event from different cpus. But this seems
to aggregate counter values from different events. Also the new
mode is basically the same as AGGR_GLOBAL.
As you will add stat_config.iostat_run to distinguish the iostat
command, probably we just want to use the global aggr mode
(and it's the default!) and get rid of the AGGR_PCIE_PORT.
Thoughts?
Thanks,
Namhyung
On 2/4/2021 3:07 PM, Namhyung Kim wrote:
> Hello,
>
> On Wed, Feb 3, 2021 at 10:58 PM Alexander Antonov
> <[email protected]> wrote:
>> Adding AGGR_PCIE_PORT mode to be able to distinguish aggr_mode
>> for root ports in following patches.
> I'm not sure adding the AGGR_PCIE_PORT is the right way.
> In my understanding, the aggr mode is to specify how we aggregate
> counter values of a single event from different cpus. But this seems
> to aggregate counter values from different events. Also the new
> mode is basically the same as AGGR_GLOBAL.
>
> As you will add stat_config.iostat_run to distinguish the iostat
> command, probably we just want to use the global aggr mode
> (and it's the default!) and get rid of the AGGR_PCIE_PORT.
>
> Thoughts?
>
> Thanks,
> Namhyung
Hello Namhyung,
Actually, you are right. We aggregate counter values from different
events of a
single IIO stack (PCIe root port) to calculate metrics for this IO stack.
But the reason is to prevent using of '-e' and '-M' options in 'iostat' mode
because it can be a reason for the mess in the output that can confuse
users.
There is an idea to use your suggestion for this part:
status = iostat_prepare(...);
if (status < 0)
goto out;
if (status == IOSTAT_LIST)
iostat_list(...);
else
...
So, we can check if evlist is empty inside iostat_prepare(). If not, print
a warning, for example, "The -e and -M options are not supported. All chosen
events/metrics will be dropped". Then we can free of evlist by using
evlist__delete(), create new one by using evlist__new() and fill the evlist.
In this case the body of iostat_prepare() function would be:
iostat_prepare()
{
If (!is_evlist_empty) {
pr_warning();
evlist__delete();
evlist__new()
}
iostat_event_group();
}
It will allow to get rid of the AGGR_PCIE_PORT.
What do you think?
Thank you,
Alexander
On Mon, Feb 8, 2021 at 8:31 PM Alexander Antonov
<[email protected]> wrote:
>
> On 2/4/2021 3:07 PM, Namhyung Kim wrote:
> > Hello,
> >
> > On Wed, Feb 3, 2021 at 10:58 PM Alexander Antonov
> > <[email protected]> wrote:
> >> Adding AGGR_PCIE_PORT mode to be able to distinguish aggr_mode
> >> for root ports in following patches.
> > I'm not sure adding the AGGR_PCIE_PORT is the right way.
> > In my understanding, the aggr mode is to specify how we aggregate
> > counter values of a single event from different cpus. But this seems
> > to aggregate counter values from different events. Also the new
> > mode is basically the same as AGGR_GLOBAL.
> >
> > As you will add stat_config.iostat_run to distinguish the iostat
> > command, probably we just want to use the global aggr mode
> > (and it's the default!) and get rid of the AGGR_PCIE_PORT.
> >
> > Thoughts?
> >
> > Thanks,
> > Namhyung
> Hello Namhyung,
>
> Actually, you are right. We aggregate counter values from different
> events of a
> single IIO stack (PCIe root port) to calculate metrics for this IO stack.
> But the reason is to prevent using of '-e' and '-M' options in 'iostat' mode
> because it can be a reason for the mess in the output that can confuse
> users.
>
> There is an idea to use your suggestion for this part:
>
> status = iostat_prepare(...);
> if (status < 0)
> goto out;
> if (status == IOSTAT_LIST)
> iostat_list(...);
> else
> ...
>
> So, we can check if evlist is empty inside iostat_prepare(). If not, print
> a warning, for example, "The -e and -M options are not supported. All chosen
> events/metrics will be dropped". Then we can free of evlist by using
> evlist__delete(), create new one by using evlist__new() and fill the evlist.
>
> In this case the body of iostat_prepare() function would be:
>
> iostat_prepare()
> {
> If (!is_evlist_empty) {
> pr_warning();
> evlist__delete();
> evlist__new()
> }
>
> iostat_event_group();
> }
>
> It will allow to get rid of the AGGR_PCIE_PORT.
> What do you think?
LGTM :)
Thanks,
Namhyung