From: "liuqi (BA)" <liuqi115@huawei.com> To: Will Deacon <will@kernel.org>, Linuxarm <linuxarm@huawei.com> Cc: <mark.rutland@arm.com>, <bhelgaas@google.com>, <linux-pci@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>, <zhangshaokun@hisilicon.com> Subject: Re: [PATCH v6 2/2] drivers/perf: hisi: Add driver for HiSilicon PCIe PMU Date: Wed, 16 Jun 2021 09:54:23 +0800 [thread overview] Message-ID: <8e15e8d6-cfe8-0926-0ca1-b162302e52a5@huawei.com> (raw) In-Reply-To: <20210615093519.GB19878@willie-the-truck> Hi Will, On 2021/6/15 17:35, Will Deacon wrote: > On Tue, Jun 15, 2021 at 04:57:09PM +0800, liuqi (BA) wrote: >> On 2021/6/12 0:23, Will Deacon wrote: >>> On Mon, May 31, 2021 at 09:32:31PM +0800, Qi Liu wrote: >>>> PCIe PMU Root Complex Integrated End Point(RCiEP) device is supported >>>> to sample bandwidth, latency, buffer occupation etc. >>>> >>>> Each PMU RCiEP device monitors multiple Root Ports, and each RCiEP is >>>> registered as a PMU in /sys/bus/event_source/devices, so users can >>>> select target PMU, and use filter to do further sets. >>>> >>>> Filtering options contains: >>>> event - select the event. >>>> subevent - select the subevent. >>>> port - select target Root Ports. Information of Root Ports >>>> are shown under sysfs. >>>> bdf - select requester_id of target EP device. >>>> trig_len - set trigger condition for starting event statistics. >>>> trigger_mode - set trigger mode. 0 means starting to statistic when >>>> bigger than trigger condition, and 1 means smaller. >>>> thr_len - set threshold for statistics. >>>> thr_mode - set threshold mode. 0 means count when bigger than >>>> threshold, and 1 means smaller. >>>> >>>> Reviewed-by: John Garry <john.garry@huawei.com> >>>> Signed-off-by: Qi Liu <liuqi115@huawei.com> >>>> --- >>>> MAINTAINERS | 6 + >>>> drivers/perf/Kconfig | 2 + >>>> drivers/perf/Makefile | 1 + >>>> drivers/perf/pci/Kconfig | 16 + >>>> drivers/perf/pci/Makefile | 2 + >>>> drivers/perf/pci/hisilicon/Makefile | 3 + >>>> drivers/perf/pci/hisilicon/hisi_pcie_pmu.c | 1019 ++++++++++++++++++++++++++++ >>> >>> Can we keep this under drivers/perf/hisilicon/ please? I don't see the >>> need to create a 'pci' directory here. >>> >> So how about drivers/perf/hisilicon/pci? as hisi_pcie_pmu.c do not use >> hisi_uncore_pmu framework. > > That's up to you. As long as it's _somewhere_ under drivers/perf/hisilicon/, > then I'm not too fussed. > ok, got it, will move the driver to perf/hisilicon in next version. >>>> +static void hisi_pcie_parse_reg_value(struct hisi_pcie_pmu *pcie_pmu, >>>> + u32 reg_off, u16 *arg0, u16 *arg1) >>>> +{ >>>> + u32 val = readl(pcie_pmu->base + reg_off); >>>> + >>>> + *arg0 = val & 0xffff; >>>> + *arg1 = (val & 0xffff0000) >> 16; >>>> +} >>> >>> Define a new type for the pair of values and return that directly? >>> >> Sorry, I'm not sure about how to fix this, do you mean add a union like >> this? >> union reg_val { >> struct { >> u16 arg0; >> u16 arg1; >> } >> u32 val; >> } > > I was just thinking along the lines of: > > struct hisi_pcie_reg_pair { > u16 lo; > u16 hi; > }; > > static struct hisi_pcie_reg_pair > hisi_pcie_parse_reg_value(struct hisi_pcie_pmu *pcie_pmum u32 reg_off) > { > u32 val = readl_relaxed(pcie_pmu->base + reg_off); > struct hisi_pcie_reg_pair regs = { > .lo = val, > .hi = val >> 16, > }; > > return regs; > } > > Does that work? > yes, will fix this, thanks. >>>> +/* >>>> + * The bandwidth, latency, bus utilization and buffer occupancy features are >>>> + * calculated from data in HISI_PCIE_CNT and extended data in HISI_PCIE_EXT_CNT. >>>> + * Other features are obtained only by HISI_PCIE_CNT. >>>> + * So data and data_ext are processed in this function to get performanace >>>> + * value like, bandwidth, latency, etc. >>>> + */ >>>> +static u64 hisi_pcie_pmu_get_performance(struct perf_event *event, u64 data, >>>> + u64 data_ext) >>>> +{ >>>> +#define CONVERT_DW_TO_BYTE(x) (sizeof(u32) * (x)) >>> >>> I don't know what a "DW" is, but this macro adds nothing... >> >> DW means double words, and 1DW = 4Bytes, value in hardware counter means DW >> so I wanna change it into Byte. >> So how about using 4*data here and adding code comment to explain it. > > Just remove the macro and replace it's single user with sizeof(u32) * x > ok, thanks. >>>> + /* Process data to set unit of latency as "us". */ >>>> + if (is_latency_event(idx)) >>>> + return div64_u64(data * us_per_cycle, data_ext); >>>> + >>>> + if (is_bus_util_event(idx)) >>>> + return div64_u64(data * us_per_cycle, data_ext); >>>> + >>>> + if (is_buf_util_event(idx)) >>>> + return div64_u64(data, data_ext * us_per_cycle); >>> >>> Why do we need to do all this division in the kernel? Can't we just expose >>> the underlying values and let userspace figure out what it wants to do with >>> the numbers? >>> >> Our PMU hardware support 8 sets of counters to count bandwidth, latency and >> utilization events. >> >> For example, when users set latency event, common counter will count delay >> cycles, and extern counter count number of PCIe packets automaticly. And we >> do not have a event number for counting number of PCIe packets. >> >> So this division cannot move to userspace tool. > > Why can't you expose the packet counter as an extra event to userspace? > Maybe I didn’t express it clearly. As there is no hardware event number for PCIe packets counting, extern counter count packets *automaticly* when latency events is selected by users. This means users cannot set "config=0xXX" to start packets counting event. So we can only get the value of counter and extern counter in driver and do the division, then pass the result to userspace. > Will > . >
WARNING: multiple messages have this Message-ID (diff)
From: "liuqi (BA)" <liuqi115@huawei.com> To: Will Deacon <will@kernel.org>, Linuxarm <linuxarm@huawei.com> Cc: <mark.rutland@arm.com>, <bhelgaas@google.com>, <linux-pci@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>, <zhangshaokun@hisilicon.com> Subject: Re: [PATCH v6 2/2] drivers/perf: hisi: Add driver for HiSilicon PCIe PMU Date: Wed, 16 Jun 2021 09:54:23 +0800 [thread overview] Message-ID: <8e15e8d6-cfe8-0926-0ca1-b162302e52a5@huawei.com> (raw) In-Reply-To: <20210615093519.GB19878@willie-the-truck> Hi Will, On 2021/6/15 17:35, Will Deacon wrote: > On Tue, Jun 15, 2021 at 04:57:09PM +0800, liuqi (BA) wrote: >> On 2021/6/12 0:23, Will Deacon wrote: >>> On Mon, May 31, 2021 at 09:32:31PM +0800, Qi Liu wrote: >>>> PCIe PMU Root Complex Integrated End Point(RCiEP) device is supported >>>> to sample bandwidth, latency, buffer occupation etc. >>>> >>>> Each PMU RCiEP device monitors multiple Root Ports, and each RCiEP is >>>> registered as a PMU in /sys/bus/event_source/devices, so users can >>>> select target PMU, and use filter to do further sets. >>>> >>>> Filtering options contains: >>>> event - select the event. >>>> subevent - select the subevent. >>>> port - select target Root Ports. Information of Root Ports >>>> are shown under sysfs. >>>> bdf - select requester_id of target EP device. >>>> trig_len - set trigger condition for starting event statistics. >>>> trigger_mode - set trigger mode. 0 means starting to statistic when >>>> bigger than trigger condition, and 1 means smaller. >>>> thr_len - set threshold for statistics. >>>> thr_mode - set threshold mode. 0 means count when bigger than >>>> threshold, and 1 means smaller. >>>> >>>> Reviewed-by: John Garry <john.garry@huawei.com> >>>> Signed-off-by: Qi Liu <liuqi115@huawei.com> >>>> --- >>>> MAINTAINERS | 6 + >>>> drivers/perf/Kconfig | 2 + >>>> drivers/perf/Makefile | 1 + >>>> drivers/perf/pci/Kconfig | 16 + >>>> drivers/perf/pci/Makefile | 2 + >>>> drivers/perf/pci/hisilicon/Makefile | 3 + >>>> drivers/perf/pci/hisilicon/hisi_pcie_pmu.c | 1019 ++++++++++++++++++++++++++++ >>> >>> Can we keep this under drivers/perf/hisilicon/ please? I don't see the >>> need to create a 'pci' directory here. >>> >> So how about drivers/perf/hisilicon/pci? as hisi_pcie_pmu.c do not use >> hisi_uncore_pmu framework. > > That's up to you. As long as it's _somewhere_ under drivers/perf/hisilicon/, > then I'm not too fussed. > ok, got it, will move the driver to perf/hisilicon in next version. >>>> +static void hisi_pcie_parse_reg_value(struct hisi_pcie_pmu *pcie_pmu, >>>> + u32 reg_off, u16 *arg0, u16 *arg1) >>>> +{ >>>> + u32 val = readl(pcie_pmu->base + reg_off); >>>> + >>>> + *arg0 = val & 0xffff; >>>> + *arg1 = (val & 0xffff0000) >> 16; >>>> +} >>> >>> Define a new type for the pair of values and return that directly? >>> >> Sorry, I'm not sure about how to fix this, do you mean add a union like >> this? >> union reg_val { >> struct { >> u16 arg0; >> u16 arg1; >> } >> u32 val; >> } > > I was just thinking along the lines of: > > struct hisi_pcie_reg_pair { > u16 lo; > u16 hi; > }; > > static struct hisi_pcie_reg_pair > hisi_pcie_parse_reg_value(struct hisi_pcie_pmu *pcie_pmum u32 reg_off) > { > u32 val = readl_relaxed(pcie_pmu->base + reg_off); > struct hisi_pcie_reg_pair regs = { > .lo = val, > .hi = val >> 16, > }; > > return regs; > } > > Does that work? > yes, will fix this, thanks. >>>> +/* >>>> + * The bandwidth, latency, bus utilization and buffer occupancy features are >>>> + * calculated from data in HISI_PCIE_CNT and extended data in HISI_PCIE_EXT_CNT. >>>> + * Other features are obtained only by HISI_PCIE_CNT. >>>> + * So data and data_ext are processed in this function to get performanace >>>> + * value like, bandwidth, latency, etc. >>>> + */ >>>> +static u64 hisi_pcie_pmu_get_performance(struct perf_event *event, u64 data, >>>> + u64 data_ext) >>>> +{ >>>> +#define CONVERT_DW_TO_BYTE(x) (sizeof(u32) * (x)) >>> >>> I don't know what a "DW" is, but this macro adds nothing... >> >> DW means double words, and 1DW = 4Bytes, value in hardware counter means DW >> so I wanna change it into Byte. >> So how about using 4*data here and adding code comment to explain it. > > Just remove the macro and replace it's single user with sizeof(u32) * x > ok, thanks. >>>> + /* Process data to set unit of latency as "us". */ >>>> + if (is_latency_event(idx)) >>>> + return div64_u64(data * us_per_cycle, data_ext); >>>> + >>>> + if (is_bus_util_event(idx)) >>>> + return div64_u64(data * us_per_cycle, data_ext); >>>> + >>>> + if (is_buf_util_event(idx)) >>>> + return div64_u64(data, data_ext * us_per_cycle); >>> >>> Why do we need to do all this division in the kernel? Can't we just expose >>> the underlying values and let userspace figure out what it wants to do with >>> the numbers? >>> >> Our PMU hardware support 8 sets of counters to count bandwidth, latency and >> utilization events. >> >> For example, when users set latency event, common counter will count delay >> cycles, and extern counter count number of PCIe packets automaticly. And we >> do not have a event number for counting number of PCIe packets. >> >> So this division cannot move to userspace tool. > > Why can't you expose the packet counter as an extra event to userspace? > Maybe I didn’t express it clearly. As there is no hardware event number for PCIe packets counting, extern counter count packets *automaticly* when latency events is selected by users. This means users cannot set "config=0xXX" to start packets counting event. So we can only get the value of counter and extern counter in driver and do the division, then pass the result to userspace. > Will > . > _______________________________________________ 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-16 1:54 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-31 13:32 [PATCH v6 0/2] drivers/perf: hisi: Add support for PCIe PMU Qi Liu 2021-05-31 13:32 ` Qi Liu 2021-05-31 13:32 ` [PATCH v6 1/2] docs: perf: Add description for HiSilicon PCIe PMU driver Qi Liu 2021-05-31 13:32 ` Qi Liu 2021-05-31 13:32 ` [PATCH v6 2/2] drivers/perf: hisi: Add driver for HiSilicon PCIe PMU Qi Liu 2021-05-31 13:32 ` Qi Liu 2021-06-11 16:23 ` Will Deacon 2021-06-11 16:23 ` Will Deacon 2021-06-14 9:20 ` Jonathan Cameron 2021-06-14 9:20 ` Jonathan Cameron 2021-06-15 9:26 ` Will Deacon 2021-06-15 9:26 ` Will Deacon 2021-06-15 8:57 ` liuqi (BA) 2021-06-15 8:57 ` liuqi (BA) 2021-06-15 9:35 ` Will Deacon 2021-06-15 9:35 ` Will Deacon 2021-06-16 1:54 ` liuqi (BA) [this message] 2021-06-16 1:54 ` liuqi (BA) 2021-06-16 13:42 ` Will Deacon 2021-06-16 13:42 ` Will Deacon 2021-06-17 11:00 ` liuqi (BA) 2021-06-17 11:00 ` liuqi (BA) 2021-06-17 17:57 ` Will Deacon 2021-06-17 17:57 ` Will Deacon 2021-06-18 9:32 ` liuqi (BA) 2021-06-18 9:32 ` liuqi (BA) 2021-06-21 17:59 ` Will Deacon 2021-06-21 17:59 ` Will Deacon 2021-06-23 9:59 ` liuqi (BA) 2021-06-23 9:59 ` liuqi (BA) 2021-06-11 23:33 ` Krzysztof Wilczyński 2021-06-11 23:33 ` Krzysztof Wilczyński 2021-06-16 1:09 ` liuqi (BA) 2021-06-16 1:09 ` liuqi (BA) 2021-06-16 15:23 ` Bjorn Helgaas 2021-06-16 15:23 ` Bjorn Helgaas 2021-06-16 17:27 ` Will Deacon 2021-06-16 17:27 ` Will Deacon 2021-06-16 14:14 ` Bjorn Helgaas 2021-06-16 14:14 ` Bjorn Helgaas 2021-06-11 10:04 ` [PATCH v6 0/2] drivers/perf: hisi: Add support for " liuqi (BA) 2021-06-11 10:04 ` 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=8e15e8d6-cfe8-0926-0ca1-b162302e52a5@huawei.com \ --to=liuqi115@huawei.com \ --cc=bhelgaas@google.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.