Re: [PATCH v7 2/2] drivers/perf: hisi: Add driver for HiSilicon PCIe PMU

[liuqi (BA)]

On 2021/6/28 20:11, John Garry wrote:
> On 28/06/2021 11:49, liuqi (BA) wrote:
> > > > Hardware counter and ext_counter work together for bandwidth, latency,
> > > > bus utilization and buffer occupancy events. For example, for latency
> > > > events(idx = 0x10), counter counts total delay cycles and ext_counter
> > > > counts PCIe packets number.
> > > >
> > > > As we don't want PMU driver to process these two data, "delay cycles"
> > > > can be treated as an event(id = 0x10), "packets number" as another 
> > > > event
> > > > (id = 0x10 << 8), and driver could export these data separately.
> > > >
> > > > if the user want to calculate latency of rx memory read, they should:
> > > > ./perf stat -v -e '{hisi_pcieX/event=0x10,
> > > > subevent=0x01/,hisi_pcieX/event=0x0400, subevent=0x01/
> > > >
> > > > and for bandwidth event:
> > > > ./perf stat -v -e '{hisi_pcieX/event=0x4,
> > > > subevent=0x02/,hisi_pcieX/event=0x1000, subevent=0x02/
> > Hi John,
> > > I would suggest supporting a perf metric for this then, which would be
> > > like:
> > >
> > > {
> > >      "BriefDescription": "Latency for inbound traffic...",
> > >      "MetricName": "hisi_pcie_lat_rx_mrd",
> > >      "MetricExpr": "hisi_pcieX@event\\=0x4@subevent\\=0x02 \
> > > hisi_pcieX@event\\=0x1000@subevent\\=0x02 \",
> > >      "Unit": "hisi_pci",
> > >      "Compat": "v1"
> > > },
> > >
> > > (syntax may be incorrect - illustration only)
> > yes, we could add these metrics in json file, thanks.
>
> The syntax is actually like:
>     "MetricExpr": "hisi_pcieX@event\\=0x4\\,subevent\\=0x2@ / 
> hisi_pcieX@event\\=0x1000\\,subevent\\=0x2@",
>
> > > > Then the value in HISI_PCIE_CNT and HISI_PCIE_EXT_CNT returned
> > > > separately, and userspace could do the calculation.
> > > But I am still curious about lat_rx_mrd and the other events which we
> > > continue to advertise. They don't really provide latency or bandwidth on
> > > their own, but only half the necessary data. So I doubt their purpose.
> > So how about changing the event name to show the real purpose of this
> > event, like changing "bw_rx_mrd" to "flux_rx_mrd", and changing
> > "lat_rx_mrd" to "delay_rx_mrd"?
>
> eh, I suppose you could, but I am not sure of the value. However  > assume that the driver will detect and reject invalid or nonsense
> combinations of events if you did want this.
Hi John,

As content in two counters are registered as two independent different 
events, these events perhaps should be selected without limitation 
(Although it make sense only when using in combination).

So perhaps we don't need to do this rejection if user only get the value 
in HISI_PCIE_CNT or only get HISI_PCIE_EXT_CNT.

Thanks,
Qi
> Thanks,
> John
> .