From: "liuqi (BA)" <liuqi115@huawei.com> To: John Garry <john.garry@huawei.com>, Linuxarm <linuxarm@huawei.com>, <will@kernel.org>, <mark.rutland@arm.com>, <bhelgaas@google.com> Cc: <linux-pci@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>, <zhangshaokun@hisilicon.com> Subject: Re: [PATCH v7 2/2] drivers/perf: hisi: Add driver for HiSilicon PCIe PMU Date: Mon, 28 Jun 2021 18:49:57 +0800 [thread overview] Message-ID: <403d09f8-6fe8-c04c-151b-40816c344b55@huawei.com> (raw) In-Reply-To: <1b164e4b-b30b-f071-51fa-841cc76ec017@huawei.com> On 2021/6/26 19:44, John Garry wrote: > On 26/06/2021 03:13, liuqi (BA) wrote: >> >> >> On 2021/6/25 23:53, John Garry wrote: >>> On 24/06/2021 11:59, Qi Liu wrote: >>>> + >>>> +/* >>>> + * Events with the "dl" suffix in their names count performance in >>>> DL layer, >>>> + * otherswise, events count performance in TL layer. >>>> + */ >>>> +static struct attribute *hisi_pcie_pmu_events_attr[] = { >>>> + HISI_PCIE_PMU_EVENT_ATTR(bw_rx_mwr, 0x010004), >>>> + HISI_PCIE_PMU_EVENT_ATTR(bw_rx_mrd, 0x100005), >>>> + HISI_PCIE_PMU_EVENT_ATTR(bw_tx_mwr, 0x010005), >>>> + HISI_PCIE_PMU_EVENT_ATTR(bw_tx_mrd, 0x200004), >>>> + HISI_PCIE_PMU_EVENT_ATTR(lat_rx_mwr, 0x000010), >>>> + HISI_PCIE_PMU_EVENT_ATTR(lat_rx_mrd, 0x020010), >>>> + HISI_PCIE_PMU_EVENT_ATTR(lat_tx_mrd, 0x000011), >>>> + HISI_PCIE_PMU_EVENT_ATTR(bw_rx_dl, 0x010084), >>>> + HISI_PCIE_PMU_EVENT_ATTR(bw_tx_dl, 0x030084), >>>> + NULL >>>> +}; >>>> + >>>> +static struct attribute_group hisi_pcie_pmu_events_group = { >>>> + .name = "events", >>>> + .attrs = hisi_pcie_pmu_events_attr, >>>> +}; >>>> + >>>> +static struct attribute *hisi_pcie_pmu_format_attr[] = { >>>> + HISI_PCIE_PMU_FORMAT_ATTR(event, "config:0-15"), >>>> + HISI_PCIE_PMU_FORMAT_ATTR(subevent, "config:16-23"), >>>> + HISI_PCIE_PMU_FORMAT_ATTR(thr_len, "config1:0-3"), >>>> + HISI_PCIE_PMU_FORMAT_ATTR(thr_mode, "config1:4"), >>>> + HISI_PCIE_PMU_FORMAT_ATTR(trig_len, "config1:5-8"), >>>> + HISI_PCIE_PMU_FORMAT_ATTR(trig_mode, "config1:9"), >>>> + HISI_PCIE_PMU_FORMAT_ATTR(port, "config2:0-15"), >>>> + HISI_PCIE_PMU_FORMAT_ATTR(bdf, "config2:16-31"), >>>> + NULL >>>> +}; >>> >>> I am just wondering how this now works. >>> >>> So if the user programs the following: >>> ./perf stat -v -e hisi_pcieX/lat_rx_mrd/ >>> >>> Then the value (incremented) in HISI_PCIE_CNT (I think that's the >>> right one) is returned as the event count. But one would expect >>> bandwidth from that event, while we only return here the delay cycles >>> - how is the count in HISI_PCIE_CNT_EXT exposed, so userspace can do >>> the calc for bw? >>> >> > > Hi Qi, > >> >> 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. >> >> 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"? Thanks, Qi > Thanks, > John > > .
WARNING: multiple messages have this Message-ID (diff)
From: "liuqi (BA)" <liuqi115@huawei.com> To: John Garry <john.garry@huawei.com>, Linuxarm <linuxarm@huawei.com>, <will@kernel.org>, <mark.rutland@arm.com>, <bhelgaas@google.com> Cc: <linux-pci@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>, <zhangshaokun@hisilicon.com> Subject: Re: [PATCH v7 2/2] drivers/perf: hisi: Add driver for HiSilicon PCIe PMU Date: Mon, 28 Jun 2021 18:49:57 +0800 [thread overview] Message-ID: <403d09f8-6fe8-c04c-151b-40816c344b55@huawei.com> (raw) In-Reply-To: <1b164e4b-b30b-f071-51fa-841cc76ec017@huawei.com> On 2021/6/26 19:44, John Garry wrote: > On 26/06/2021 03:13, liuqi (BA) wrote: >> >> >> On 2021/6/25 23:53, John Garry wrote: >>> On 24/06/2021 11:59, Qi Liu wrote: >>>> + >>>> +/* >>>> + * Events with the "dl" suffix in their names count performance in >>>> DL layer, >>>> + * otherswise, events count performance in TL layer. >>>> + */ >>>> +static struct attribute *hisi_pcie_pmu_events_attr[] = { >>>> + HISI_PCIE_PMU_EVENT_ATTR(bw_rx_mwr, 0x010004), >>>> + HISI_PCIE_PMU_EVENT_ATTR(bw_rx_mrd, 0x100005), >>>> + HISI_PCIE_PMU_EVENT_ATTR(bw_tx_mwr, 0x010005), >>>> + HISI_PCIE_PMU_EVENT_ATTR(bw_tx_mrd, 0x200004), >>>> + HISI_PCIE_PMU_EVENT_ATTR(lat_rx_mwr, 0x000010), >>>> + HISI_PCIE_PMU_EVENT_ATTR(lat_rx_mrd, 0x020010), >>>> + HISI_PCIE_PMU_EVENT_ATTR(lat_tx_mrd, 0x000011), >>>> + HISI_PCIE_PMU_EVENT_ATTR(bw_rx_dl, 0x010084), >>>> + HISI_PCIE_PMU_EVENT_ATTR(bw_tx_dl, 0x030084), >>>> + NULL >>>> +}; >>>> + >>>> +static struct attribute_group hisi_pcie_pmu_events_group = { >>>> + .name = "events", >>>> + .attrs = hisi_pcie_pmu_events_attr, >>>> +}; >>>> + >>>> +static struct attribute *hisi_pcie_pmu_format_attr[] = { >>>> + HISI_PCIE_PMU_FORMAT_ATTR(event, "config:0-15"), >>>> + HISI_PCIE_PMU_FORMAT_ATTR(subevent, "config:16-23"), >>>> + HISI_PCIE_PMU_FORMAT_ATTR(thr_len, "config1:0-3"), >>>> + HISI_PCIE_PMU_FORMAT_ATTR(thr_mode, "config1:4"), >>>> + HISI_PCIE_PMU_FORMAT_ATTR(trig_len, "config1:5-8"), >>>> + HISI_PCIE_PMU_FORMAT_ATTR(trig_mode, "config1:9"), >>>> + HISI_PCIE_PMU_FORMAT_ATTR(port, "config2:0-15"), >>>> + HISI_PCIE_PMU_FORMAT_ATTR(bdf, "config2:16-31"), >>>> + NULL >>>> +}; >>> >>> I am just wondering how this now works. >>> >>> So if the user programs the following: >>> ./perf stat -v -e hisi_pcieX/lat_rx_mrd/ >>> >>> Then the value (incremented) in HISI_PCIE_CNT (I think that's the >>> right one) is returned as the event count. But one would expect >>> bandwidth from that event, while we only return here the delay cycles >>> - how is the count in HISI_PCIE_CNT_EXT exposed, so userspace can do >>> the calc for bw? >>> >> > > Hi Qi, > >> >> 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. >> >> 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"? Thanks, Qi > Thanks, > John > > . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-06-28 10:50 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-24 10:59 [PATCH v7 0/2] drivers/perf: hisi: Add support for PCIe PMU Qi Liu 2021-06-24 10:59 ` Qi Liu 2021-06-24 10:59 ` [PATCH v7 1/2] docs: perf: Add description for HiSilicon PCIe PMU driver Qi Liu 2021-06-24 10:59 ` Qi Liu 2021-06-24 10:59 ` [PATCH v7 2/2] drivers/perf: hisi: Add driver for HiSilicon PCIe PMU Qi Liu 2021-06-24 10:59 ` Qi Liu 2021-06-25 15:53 ` John Garry 2021-06-25 15:53 ` John Garry 2021-06-26 2:13 ` liuqi (BA) 2021-06-26 2:13 ` liuqi (BA) 2021-06-26 11:44 ` John Garry 2021-06-26 11:44 ` John Garry 2021-06-28 10:49 ` liuqi (BA) [this message] 2021-06-28 10:49 ` liuqi (BA) 2021-06-28 12:11 ` John Garry 2021-06-28 12:11 ` John Garry 2021-06-28 13:17 ` liuqi (BA) 2021-06-28 13:17 ` liuqi (BA) 2021-06-28 12:03 ` kajoljain 2021-06-28 12:03 ` kajoljain 2021-06-30 8:37 ` liuqi (BA) 2021-06-30 8:37 ` liuqi (BA) 2021-07-12 12:54 ` liuqi (BA) 2021-07-12 12:54 ` liuqi (BA)
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=403d09f8-6fe8-c04c-151b-40816c344b55@huawei.com \ --to=liuqi115@huawei.com \ --cc=bhelgaas@google.com \ --cc=john.garry@huawei.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=linuxarm@huawei.com \ --cc=mark.rutland@arm.com \ --cc=will@kernel.org \ --cc=zhangshaokun@hisilicon.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.