Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Joakim Zhang <qiangqing.zhang@nxp.com>
To:     kajoljain <kjain@linux.ibm.com>,
        "acme@kernel.org" <acme@kernel.org>, Jiri Olsa <jolsa@kernel.org>,
        Andi Kleen <ak@linux.intel.com>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-perf-users@vger.kernel.org" <linux-perf-users@vger.kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Kan Liang <kan.liang@linux.intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Jin Yao <yao.jin@linux.intel.com>,
        Madhavan Srinivasan <maddy@linux.vnet.ibm.com>,
        Anju T Sudhakar <anju@linux.vnet.ibm.com>,
        Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Subject: RE: [PATCH v4] tools/perf/metricgroup: Fix printing event names of
 metric group with multiple events incase of overlapping events
Thread-Topic: [PATCH v4] tools/perf/metricgroup: Fix printing event names of
 metric group with multiple events incase of overlapping events
Thread-Index: AQHV4WcV2Y0K+6EYb0mNmxH19eS7KageuXkAgAUq9oCAAAnqYIARApyAgBxNslA=
Date:   Fri, 20 Mar 2020 06:47:58 +0000
Message-ID: <DB7PR04MB4618D0696D39AC5D44FF5A51E6F50@DB7PR04MB4618.eurprd04.prod.outlook.com>
References: <20200212054102.9259-1-kjain@linux.ibm.com>
 <DB7PR04MB46186AB5557F4D04FD5C4FEAE6160@DB7PR04MB4618.eurprd04.prod.outlook.com>
 <be86ba99-ab5a-c845-46b6-8081edee00ca@linux.ibm.com>
 <DB7PR04MB461807389FDF9629ACA04533E6130@DB7PR04MB4618.eurprd04.prod.outlook.com>
 <cb9b353b-c18a-0064-eb72-a6c91d5fdec9@linux.ibm.com>
In-Reply-To: <cb9b353b-c18a-0064-eb72-a6c91d5fdec9@linux.ibm.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 0d39ccb6-0d5a-4e80-04c0-08d7cc9aa068
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Mar 2020 06:47:58.3176
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: FOZkBqK/njXWQ6JHUJfOZSQFZXAYvXExFJpwzo+VadBxh8vcpncPDlg7aKYzZzua5crOlkxLaZmul4avToXFXg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB7PR04MB5564
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


[...]
> >>> Hi Kajol,
> >>>
> >>> I am not sure if it is good to ask a question here :-)
> >>>
> >>> I encountered a perf metricgroup issue, the result is incorrect when
> >>> the
> >> metric includes more than 2 events.
> >>>
> >>> git log --oneline tools/perf/util/metricgroup.c
> >>> 3635b27cc058 perf metricgroup: Fix printing event names of metric
> >>> group with multiple events f01642e4912b perf metricgroup: Support
> >>> multiple events for metricgroup
> >>> 287f2649f791 perf metricgroup: Scale the metric result
> >>>
> >>> I did a simple test, below is the JSON file and result.
> >>> [
> >>>         {
> >>>              "PublicDescription": "Calculate DDR0 bus actual
> >>> utilization
> >> which vary from DDR0 controller clock frequency",
> >>>              "BriefDescription": "imx8qm: ddr0 bus actual utilization=
",
> >>>              "MetricName": "imx8qm-ddr0-bus-util",
> >>>              "MetricExpr": "( imx8_ddr0\\/read\\-cycles\\/ +
> >> imx8_ddr0\\/write\\-cycles\\/ )",
> >>>              "MetricGroup": "i.MX8QM_DDR0_BUS_UTIL"
> >>>         }
> >>> ]
> >>> ./perf stat -I 1000 -M imx8qm-ddr0-bus-util
> >>> #           time             counts unit events
> >>>      1.000104250              16720      imx8_ddr0/read-cycles/
> >> #  22921.0 imx8qm-ddr0-bus-util
> >>>      1.000104250               6201
> imx8_ddr0/write-cycles/
> >>>      2.000525625               8316      imx8_ddr0/read-cycles/
> >> #  12785.5 imx8qm-ddr0-bus-util
> >>>      2.000525625               2738
> imx8_ddr0/write-cycles/
> >>>      3.000819125               1056      imx8_ddr0/read-cycles/
> >> #   4136.7 imx8qm-ddr0-bus-util
> >>>      3.000819125                303
> imx8_ddr0/write-cycles/
> >>>      4.001103750               6260      imx8_ddr0/read-cycles/
> >> #   9149.8 imx8qm-ddr0-bus-util
> >>>      4.001103750               2317
> imx8_ddr0/write-cycles/
> >>>      5.001392750               2084      imx8_ddr0/read-cycles/
> >> #   4516.0 imx8qm-ddr0-bus-util
> >>>      5.001392750                601
> imx8_ddr0/write-cycles/
> >>>
> >>> You can see that only the first result is correct, could this be
> >>> reproduced at
> >> you side?
> >>
> >> Hi Joakim,
> >>         Will try to look into it from my side.
> >
>=20
> > Thanks Kajol for your help, I look into this issue, but don't know how =
to fix it.
> >
> > The results are always correct if signal event used in "MetricExpr" wit=
h "-I"
> parameters, but the results are incorrect when more than one events used =
in
> "MetricExpr".
> >
>=20
> Hi Joakim,
>     So, I try to look into this issue and understand the flow. From my
> understanding, whenever we do
>     calculation of metric expression we don't use exact count we are gett=
ing.
>     Basically we use mean value of each event in the calculation of metri=
c
> expression.
>=20
> So, I am taking same example you refer.
>=20
> Metric Event: imx8qm-ddr0-bus-util
> MetricExpr": "( imx8_ddr0\\/read\\-cycles\\/ + imx8_ddr0\\/write\\-cycles=
\\/ )"
>=20
> command#: ./perf stat -I 1000 -M imx8qm-ddr0-bus-util
>=20
> #           time             counts unit events
>      1.000104250              16720      imx8_ddr0/read-cycles/
> #  22921.0 imx8qm-ddr0-bus-util
>      1.000104250               6201      imx8_ddr0/write-cycles/
>      2.000525625               8316      imx8_ddr0/read-cycles/
> #  12785.5 imx8qm-ddr0-bus-util
>      2.000525625               2738      imx8_ddr0/write-cycles/
>      3.000819125               1056      imx8_ddr0/read-cycles/
> #   4136.7 imx8qm-ddr0-bus-util
>      3.000819125                303      imx8_ddr0/write-cycles/
>      4.001103750               6260      imx8_ddr0/read-cycles/
> #   9149.8 imx8qm-ddr0-bus-util
>      4.001103750               2317      imx8_ddr0/write-cycles/
>      5.001392750               2084      imx8_ddr0/read-cycles/
> #   4516.0 imx8qm-ddr0-bus-util
>      5.001392750                601      imx8_ddr0/write-cycles/
>=20
> If you see we have a function called 'update_stats' in file util/stat.c w=
here we
> do this calculation and updating stats->mean value. And this mean value i=
s
> what we are using actually in our metric expression calculation.
>=20
> We call this function in each iteration where we update stats->mean and
> stats->n for each event.
> But one weird issue is, for very first event, stat->n is always 1 that is=
 why we
> are getting mean same as count.
> So this is the reason for single event you get exact aggregate of metric
> expression.
> So doesn't matter how many events you have in your metric expression, eve=
ry
> time you take exact count for first one and normalized value for rest whi=
ch is
> weird.
>=20
> According to update_stats function:  We are updating mean as:
>=20
> stats->mean +=3D delta / stats->n where,  delta =3D val - stats->mean.
>=20
> If we take write-cycles here. Initially mean =3D 0 and n =3D 1.
>=20
> 1st iteration: n=3D1, write cycle : 6201 and mean =3D 6201  (Final agg va=
lue: 16720
> + 6201 =3D 22921) 2nd iteration: n=3D2, write cycles:  6201 + (2738 - 620=
1)/2 =3D
> 4469.5  (Final aggr value: 8316 + 4469.5 =3D 12785.5) 3rd iteration: n=3D=
3, write
> cycles: 4469.5 + (303 - 4469.5)/3 =3D 3080.6667 (Final aggr value: 1056 +
> 3080.6667 =3D 4136.7)
>=20
> Andi and Jiri, I am not sure if its expected behavior. I mean shouldn't w=
e either
> take mean value of each event or take n as 1 for each event. And one more
> question, Should we add an option to say whether user want exact aggregat=
e
> or this normalize aggregate to remove the confusion? I try to find it out=
 if we
> already have one but didn't get.
> Please let me know if my understanding is fine.

Hi Kajol,

Sorry, your reply was buried in a sea of emails, it comes into my eyes when=
 I searched any feedback from you. Much thanks for your great details!!!!!

I can quite understand what you explained. As a user, I think we always wan=
t to get the exact result according to the metric expression.

Can you take this case as an example then send out a formal email into mail=
ing list to reflect this weird issue, more people can participate and discu=
ss about it. Or you need me clear up and sent out the email?
This could attract maintainers' attention.

Best Regards,
Joakim Zhang