[PATCH v2 0/8] drivers/perf: hisi_pcie: Several updates for HiSilicon PCIe PMU driver

[PATCH v2 0/8] drivers/perf: hisi_pcie: Several updates for HiSilicon PCIe PMU driver

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

This series mainly fix and optimize the several usage of the driver:
- Add more events to complement the TLP bandwidth counting
- Fix the wrong counting on using the event group
- Properly check the target filter to avoid invalid filter value
- Optimize the handling of related events which are not in an event group
- Update the docs

Change since v1:
- Rename hisi_pcie_pmu_{config,clear}_filter() to properly reflect its function.
- Add some test case logs to the Patch 5/8 comments.
- Avoid use HISI_PCIE_MAX_COUNTER outside of functions in Patch 7/8.
Link: https://lore.kernel.org/all/20240204074527.47110-1-yangyicong@huawei.com/

Junhao He (4):
  drivers/perf: hisi_pcie: Check the target filter properly
  drivers/perf: hisi_pcie: Relax the check on related events
  drivers/perf: hisi_pcie: Merge find_related_event() and
    get_event_idx()
  docs: perf: Update usage for target filter of hisi-pcie-pmu

Yicong Yang (4):
  drivers/perf: hisi_pcie: Rename hisi_pcie_pmu_{config,clear}_filter()
  drivers/perf: hisi_pcie: Introduce hisi_pcie_pmu_get_event_ctrl_val()
  drivers/perf: hisi_pcie: Fix incorrect counting under metric mode
  drivers/perf: hisi_pcie: Add more events for counting TLP bandwidth

 .../admin-guide/perf/hisi-pcie-pmu.rst        |  31 ++++--
 drivers/perf/hisilicon/hisi_pcie_pmu.c        | 102 +++++++++---------
 2 files changed, 76 insertions(+), 57 deletions(-)

-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/8] drivers/perf: hisi_pcie: Rename hisi_pcie_pmu_{config,clear}_filter()

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

hisi_pcie_pmu_{config,clear}_filter() are config/clear HISI_PCIE_EVENT_CTRL
register which contains not only the filter but also the event code. The
function names are bit misleading. Rename it to
hisi_pcie_pmu_{config,clear}_event_ctrl() to reflects their functions
more accurately.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/perf/hisilicon/hisi_pcie_pmu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
index b90ba8aca3fa..9760ddde46fd 100644
--- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
+++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
@@ -216,7 +216,7 @@ static void hisi_pcie_pmu_writeq(struct hisi_pcie_pmu *pcie_pmu, u32 reg_offset,
 	writeq_relaxed(val, pcie_pmu->base + offset);
 }
 
-static void hisi_pcie_pmu_config_filter(struct perf_event *event)
+static void hisi_pcie_pmu_config_event_ctrl(struct perf_event *event)
 {
 	struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
 	struct hw_perf_event *hwc = &event->hw;
@@ -259,7 +259,7 @@ static void hisi_pcie_pmu_config_filter(struct perf_event *event)
 	hisi_pcie_pmu_writeq(pcie_pmu, HISI_PCIE_EVENT_CTRL, hwc->idx, reg);
 }
 
-static void hisi_pcie_pmu_clear_filter(struct perf_event *event)
+static void hisi_pcie_pmu_clear_event_ctrl(struct perf_event *event)
 {
 	struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
 	struct hw_perf_event *hwc = &event->hw;
@@ -505,7 +505,7 @@ static void hisi_pcie_pmu_start(struct perf_event *event, int flags)
 	WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE));
 	hwc->state = 0;
 
-	hisi_pcie_pmu_config_filter(event);
+	hisi_pcie_pmu_config_event_ctrl(event);
 	hisi_pcie_pmu_enable_counter(pcie_pmu, hwc);
 	hisi_pcie_pmu_enable_int(pcie_pmu, hwc);
 	hisi_pcie_pmu_set_period(event);
@@ -526,7 +526,7 @@ static void hisi_pcie_pmu_stop(struct perf_event *event, int flags)
 	hisi_pcie_pmu_event_update(event);
 	hisi_pcie_pmu_disable_int(pcie_pmu, hwc);
 	hisi_pcie_pmu_disable_counter(pcie_pmu, hwc);
-	hisi_pcie_pmu_clear_filter(event);
+	hisi_pcie_pmu_clear_event_ctrl(event);
 	WARN_ON_ONCE(hwc->state & PERF_HES_STOPPED);
 	hwc->state |= PERF_HES_STOPPED;
 
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/8] drivers/perf: hisi_pcie: Rename hisi_pcie_pmu_{config,clear}_filter()

[Jonathan Cameron]

On Fri, 23 Feb 2024 18:33:52 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> hisi_pcie_pmu_{config,clear}_filter() are config/clear HISI_PCIE_EVENT_CTRL
> register which contains not only the filter but also the event code. The
> function names are bit misleading. Rename it to
> hisi_pcie_pmu_{config,clear}_event_ctrl() to reflects their functions
> more accurately.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Definitely an improvement on readability.  As discussed offline I'm
not sure the 'clear' part is strictly right either, but in some
sense that's a separate issue.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/perf/hisilicon/hisi_pcie_pmu.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
> index b90ba8aca3fa..9760ddde46fd 100644
> --- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
> @@ -216,7 +216,7 @@ static void hisi_pcie_pmu_writeq(struct hisi_pcie_pmu *pcie_pmu, u32 reg_offset,
>  	writeq_relaxed(val, pcie_pmu->base + offset);
>  }
>  
> -static void hisi_pcie_pmu_config_filter(struct perf_event *event)
> +static void hisi_pcie_pmu_config_event_ctrl(struct perf_event *event)
>  {
>  	struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
>  	struct hw_perf_event *hwc = &event->hw;
> @@ -259,7 +259,7 @@ static void hisi_pcie_pmu_config_filter(struct perf_event *event)
>  	hisi_pcie_pmu_writeq(pcie_pmu, HISI_PCIE_EVENT_CTRL, hwc->idx, reg);
>  }
>  
> -static void hisi_pcie_pmu_clear_filter(struct perf_event *event)
> +static void hisi_pcie_pmu_clear_event_ctrl(struct perf_event *event)
>  {
>  	struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
>  	struct hw_perf_event *hwc = &event->hw;
> @@ -505,7 +505,7 @@ static void hisi_pcie_pmu_start(struct perf_event *event, int flags)
>  	WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE));
>  	hwc->state = 0;
>  
> -	hisi_pcie_pmu_config_filter(event);
> +	hisi_pcie_pmu_config_event_ctrl(event);
>  	hisi_pcie_pmu_enable_counter(pcie_pmu, hwc);
>  	hisi_pcie_pmu_enable_int(pcie_pmu, hwc);
>  	hisi_pcie_pmu_set_period(event);
> @@ -526,7 +526,7 @@ static void hisi_pcie_pmu_stop(struct perf_event *event, int flags)
>  	hisi_pcie_pmu_event_update(event);
>  	hisi_pcie_pmu_disable_int(pcie_pmu, hwc);
>  	hisi_pcie_pmu_disable_counter(pcie_pmu, hwc);
> -	hisi_pcie_pmu_clear_filter(event);
> +	hisi_pcie_pmu_clear_event_ctrl(event);
>  	WARN_ON_ONCE(hwc->state & PERF_HES_STOPPED);
>  	hwc->state |= PERF_HES_STOPPED;
>  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/8] drivers/perf: hisi_pcie: Introduce hisi_pcie_pmu_get_event_ctrl_val()

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

Factor out retrieving of the register value for the
corresponding event from hisi_pcie_config_event_ctrl() into a
new function hisi_pcie_pmu_get_event_ctrl_val() allowing future
reuse.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/perf/hisilicon/hisi_pcie_pmu.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
index 9760ddde46fd..2468cf3b007c 100644
--- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
+++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
@@ -216,10 +216,8 @@ static void hisi_pcie_pmu_writeq(struct hisi_pcie_pmu *pcie_pmu, u32 reg_offset,
 	writeq_relaxed(val, pcie_pmu->base + offset);
 }
 
-static void hisi_pcie_pmu_config_event_ctrl(struct perf_event *event)
+static u64 hisi_pcie_pmu_get_event_ctrl_val(struct perf_event *event)
 {
-	struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
-	struct hw_perf_event *hwc = &event->hw;
 	u64 port, trig_len, thr_len, len_mode;
 	u64 reg = HISI_PCIE_INIT_SET;
 
@@ -256,6 +254,15 @@ static void hisi_pcie_pmu_config_event_ctrl(struct perf_event *event)
 	else
 		reg |= FIELD_PREP(HISI_PCIE_LEN_M, HISI_PCIE_LEN_M_DEFAULT);
 
+	return reg;
+}
+
+static void hisi_pcie_pmu_config_event_ctrl(struct perf_event *event)
+{
+	struct hisi_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	u64 reg = hisi_pcie_pmu_get_event_ctrl_val(event);
+
 	hisi_pcie_pmu_writeq(pcie_pmu, HISI_PCIE_EVENT_CTRL, hwc->idx, reg);
 }
 
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/8] drivers/perf: hisi_pcie: Fix incorrect counting under metric mode

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

The metric counting shows incorrect results if the events in the
metric group using the same event but different filter options.
This is because we only judge the event code to decide whether
the event in the metric group should share the same hardware
counter, but ignore the settings of the filter.

For example, on a platform of 2 ports 0x1 and 0x2 but only port
0x1 has a downstream PCIe NVME device. The metric counting
shows both ports have the same counts because we misassign these
two events to one same hardware counter:
[root@localhost perf-iostat]# ./perf stat -e '{hisi_pcie0_core1/event=0x0104,port=0x2/,hisi_pcie0_core1/event=0x0104,port=0x1/}'

 Performance counter stats for 'system wide':

        7907484924      hisi_pcie0_core1/event=0x0104,port=0x2/
        7907484924      hisi_pcie0_core1/event=0x0104,port=0x1/

      10.153863691 seconds time elapsed

Fix this by using the whole config rather than the event only
to judge whether two events are the same and should share the
same hardware counter. With this patch, the metric counting in
the above case tends to be corrected:

[root@localhost perf-iostat]# ./perf stat -e '{hisi_pcie0_core1/event=0x0104,port=0x2/,hisi_pcie0_core1/event=0x0104,port=0x1/}'

 Performance counter stats for 'system wide':

                 0      hisi_pcie0_core1/event=0x0104,port=0x2/
        8123122077      hisi_pcie0_core1/event=0x0104,port=0x1/

      10.152875631 seconds time elapsed

Fixes: 8404b0fbc7fb ("drivers/perf: hisi: Add driver for HiSilicon PCIe PMU")
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/perf/hisilicon/hisi_pcie_pmu.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
index 2468cf3b007c..9176242eadb3 100644
--- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
+++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
@@ -314,10 +314,16 @@ static bool hisi_pcie_pmu_valid_filter(struct perf_event *event,
 	return true;
 }
 
+/*
+ * Check Whether two events share the same config. The same config means not
+ * only the event code, but also the filter settings of the two events are
+ * the same.
+ */
 static bool hisi_pcie_pmu_cmp_event(struct perf_event *target,
 					struct perf_event *event)
 {
-	return hisi_pcie_get_real_event(target) == hisi_pcie_get_real_event(event);
+	return hisi_pcie_pmu_get_event_ctrl_val(target) ==
+	       hisi_pcie_pmu_get_event_ctrl_val(event);
 }
 
 static bool hisi_pcie_pmu_validate_event_group(struct perf_event *event)
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/8] drivers/perf: hisi_pcie: Fix incorrect counting under metric mode

[Jonathan Cameron]

On Fri, 23 Feb 2024 18:33:54 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> The metric counting shows incorrect results if the events in the
> metric group using the same event but different filter options.
> This is because we only judge the event code to decide whether
> the event in the metric group should share the same hardware
> counter, but ignore the settings of the filter.
> 
> For example, on a platform of 2 ports 0x1 and 0x2 but only port
> 0x1 has a downstream PCIe NVME device. The metric counting
> shows both ports have the same counts because we misassign these
> two events to one same hardware counter:
> [root@localhost perf-iostat]# ./perf stat -e '{hisi_pcie0_core1/event=0x0104,port=0x2/,hisi_pcie0_core1/event=0x0104,port=0x1/}'
> 
>  Performance counter stats for 'system wide':
> 
>         7907484924      hisi_pcie0_core1/event=0x0104,port=0x2/
>         7907484924      hisi_pcie0_core1/event=0x0104,port=0x1/
> 
>       10.153863691 seconds time elapsed
> 
> Fix this by using the whole config rather than the event only
> to judge whether two events are the same and should share the
> same hardware counter. With this patch, the metric counting in
> the above case tends to be corrected:
> 
> [root@localhost perf-iostat]# ./perf stat -e '{hisi_pcie0_core1/event=0x0104,port=0x2/,hisi_pcie0_core1/event=0x0104,port=0x1/}'
> 
>  Performance counter stats for 'system wide':
> 
>                  0      hisi_pcie0_core1/event=0x0104,port=0x2/
>         8123122077      hisi_pcie0_core1/event=0x0104,port=0x1/
> 
>       10.152875631 seconds time elapsed
> 
> Fixes: 8404b0fbc7fb ("drivers/perf: hisi: Add driver for HiSilicon PCIe PMU")
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/8] drivers/perf: hisi_pcie: Add more events for counting TLP bandwidth

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

A typical PCIe transaction is consisted of various TLP packets in both
direction. For counting bandwidth only memory read events are exported
currently. Add memory write and completion counting events of both
direction to complete the bandwidth counting.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/perf/hisilicon/hisi_pcie_pmu.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
index 9176242eadb3..6f39cb82661e 100644
--- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
+++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
@@ -727,10 +727,18 @@ static struct attribute *hisi_pcie_pmu_events_attr[] = {
 	HISI_PCIE_PMU_EVENT_ATTR(rx_mrd_cnt, 0x10210),
 	HISI_PCIE_PMU_EVENT_ATTR(tx_mrd_latency, 0x0011),
 	HISI_PCIE_PMU_EVENT_ATTR(tx_mrd_cnt, 0x10011),
+	HISI_PCIE_PMU_EVENT_ATTR(rx_mwr_flux, 0x0104),
+	HISI_PCIE_PMU_EVENT_ATTR(rx_mwr_time, 0x10104),
 	HISI_PCIE_PMU_EVENT_ATTR(rx_mrd_flux, 0x0804),
 	HISI_PCIE_PMU_EVENT_ATTR(rx_mrd_time, 0x10804),
+	HISI_PCIE_PMU_EVENT_ATTR(rx_cpl_flux, 0x2004),
+	HISI_PCIE_PMU_EVENT_ATTR(rx_cpl_time, 0x12004),
+	HISI_PCIE_PMU_EVENT_ATTR(tx_mwr_flux, 0x0105),
+	HISI_PCIE_PMU_EVENT_ATTR(tx_mwr_time, 0x10105),
 	HISI_PCIE_PMU_EVENT_ATTR(tx_mrd_flux, 0x0405),
 	HISI_PCIE_PMU_EVENT_ATTR(tx_mrd_time, 0x10405),
+	HISI_PCIE_PMU_EVENT_ATTR(tx_cpl_flux, 0x1005),
+	HISI_PCIE_PMU_EVENT_ATTR(tx_cpl_time, 0x11005),
 	NULL
 };
 
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/8] drivers/perf: hisi_pcie: Check the target filter properly

[Yicong Yang]

From: Junhao He <hejunhao3@huawei.com>

The PMU can monitor traffic of certain target Root Port or downstream
target Endpoint. User can specify the target filter by the "port" or
"bdf" option respectively. The PMU can only monitor the Root Port or
Endpoint on the same PCIe core so the value of "port" or "bdf" should
be valid and will be checked by the driver.

Currently at least and only one of "port" and "bdf" option must be set.
If "port" filter is not set or is set explicitly to zero (default),
driver will regard the user specifies a "bdf" option since "port" option
is a bitmask of the target Root Ports and zero is not a valid
value.

If user not explicitly set "port" or "bdf" filter, the driver uses "bdf"
default value (zero) to set target filter, but driver will skip the
check of bdf=0, although it's a valid value (meaning 0000:000:00.0).
Then the user just gets zero.

Therefore, we need to check if both "port" and "bdf" are invalid, then
return failure and report warning.

Testing:
before the patch:
                   0      hisi_pcie0_core1/rx_mrd_flux/
                   0      hisi_pcie0_core1/rx_mrd_flux,port=0/
              24,124      hisi_pcie0_core1/rx_mrd_flux,port=1/
                   0      hisi_pcie0_core1/rx_mrd_flux,bdf=0/
                   0      hisi_pcie0_core1/rx_mrd_flux,port=0x800/
     <not supported>      hisi_pcie0_core1/rx_mrd_flux,bdf=1/
              24,132      hisi_pcie0_core1/rx_mrd_flux,bdf=0x1700/
     <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x0/
     <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x1/
              24,138      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x1700/
              24,126      hisi_pcie0_core1/rx_mrd_flux,port=0x1,bdf=0x0/

after the patch:
     <not supported>      hisi_pcie0_core1/rx_mrd_flux/
     <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0/
              24,153      hisi_pcie0_core1/rx_mrd_flux,port=1/
                   0      hisi_pcie0_core1/rx_mrd_flux,port=0x800/
     <not supported>      hisi_pcie0_core1/rx_mrd_flux,bdf=0/
     <not supported>      hisi_pcie0_core1/rx_mrd_flux,bdf=1/
              24,117      hisi_pcie0_core1/rx_mrd_flux,bdf=0x1700/
     <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x0/
     <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x1/
              24,120      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x1700/
              24,123      hisi_pcie0_core1/rx_mrd_flux,port=0x1,bdf=0x0/

Signed-off-by: Junhao He <hejunhao3@huawei.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/perf/hisilicon/hisi_pcie_pmu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
index 6f39cb82661e..b2dde7559639 100644
--- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
+++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
@@ -306,10 +306,10 @@ static bool hisi_pcie_pmu_valid_filter(struct perf_event *event,
 	if (hisi_pcie_get_trig_len(event) > HISI_PCIE_TRIG_MAX_VAL)
 		return false;
 
-	if (requester_id) {
-		if (!hisi_pcie_pmu_valid_requester_id(pcie_pmu, requester_id))
-			return false;
-	}
+	/* Need to explicitly set filter of "port" or "bdf" */
+	if (!hisi_pcie_get_port(event) &&
+	    !hisi_pcie_pmu_valid_requester_id(pcie_pmu, requester_id))
+		return false;
 
 	return true;
 }
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/8] drivers/perf: hisi_pcie: Check the target filter properly

[Jonathan Cameron]

On Fri, 23 Feb 2024 18:33:56 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Junhao He <hejunhao3@huawei.com>
> 
> The PMU can monitor traffic of certain target Root Port or downstream
> target Endpoint. User can specify the target filter by the "port" or
> "bdf" option respectively. The PMU can only monitor the Root Port or
> Endpoint on the same PCIe core so the value of "port" or "bdf" should
> be valid and will be checked by the driver.
> 
> Currently at least and only one of "port" and "bdf" option must be set.
> If "port" filter is not set or is set explicitly to zero (default),
> driver will regard the user specifies a "bdf" option since "port" option
> is a bitmask of the target Root Ports and zero is not a valid
> value.
> 
> If user not explicitly set "port" or "bdf" filter, the driver uses "bdf"
> default value (zero) to set target filter, but driver will skip the
> check of bdf=0, although it's a valid value (meaning 0000:000:00.0).
> Then the user just gets zero.
> 
> Therefore, we need to check if both "port" and "bdf" are invalid, then
> return failure and report warning.
> 
> Testing:
> before the patch:
>                    0      hisi_pcie0_core1/rx_mrd_flux/
>                    0      hisi_pcie0_core1/rx_mrd_flux,port=0/
>               24,124      hisi_pcie0_core1/rx_mrd_flux,port=1/
>                    0      hisi_pcie0_core1/rx_mrd_flux,bdf=0/
>                    0      hisi_pcie0_core1/rx_mrd_flux,port=0x800/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,bdf=1/
>               24,132      hisi_pcie0_core1/rx_mrd_flux,bdf=0x1700/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x0/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x1/
>               24,138      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x1700/
>               24,126      hisi_pcie0_core1/rx_mrd_flux,port=0x1,bdf=0x0/
> 
> after the patch:
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0/
>               24,153      hisi_pcie0_core1/rx_mrd_flux,port=1/
>                    0      hisi_pcie0_core1/rx_mrd_flux,port=0x800/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,bdf=0/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,bdf=1/
>               24,117      hisi_pcie0_core1/rx_mrd_flux,bdf=0x1700/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x0/
>      <not supported>      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x1/
>               24,120      hisi_pcie0_core1/rx_mrd_flux,port=0x0,bdf=0x1700/
>               24,123      hisi_pcie0_core1/rx_mrd_flux,port=0x1,bdf=0x0/
> 
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Thanks for explanation on v1. I'm fine with this now.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/perf/hisilicon/hisi_pcie_pmu.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
> index 6f39cb82661e..b2dde7559639 100644
> --- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
> @@ -306,10 +306,10 @@ static bool hisi_pcie_pmu_valid_filter(struct perf_event *event,
>  	if (hisi_pcie_get_trig_len(event) > HISI_PCIE_TRIG_MAX_VAL)
>  		return false;
>  
> -	if (requester_id) {
> -		if (!hisi_pcie_pmu_valid_requester_id(pcie_pmu, requester_id))
> -			return false;
> -	}
> +	/* Need to explicitly set filter of "port" or "bdf" */
> +	if (!hisi_pcie_get_port(event) &&
> +	    !hisi_pcie_pmu_valid_requester_id(pcie_pmu, requester_id))
> +		return false;
>  
>  	return true;
>  }


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 6/8] drivers/perf: hisi_pcie: Relax the check on related events

[Yicong Yang]

From: Junhao He <hejunhao3@huawei.com>

If we use two events with the same filter and related event type
(see the following example), the driver check whether they are related
events and are in the same group, otherwise the function
hisi_pcie_pmu_find_related_event() return -EINVAL, then the 2nd event
cannot count but the 1st event is running, although the PCIe PMU has
other idle counters.

In this case, The perf event scheduler will make the two events to
multiplex a counter, if the user use the formula
(1st event_value / 2nd event_value) to calculate the bandwidth, he/she
won't get the correct value, because they are not counting at the
same period.

This patch tries to fix this by making the related events to use
different idle counters if they are not in the same event group.

And finally, I'm going to say. The related events are best used in the
same group [1]. There are two ways to know if they are related events.
a) By event name, such as the latency events "xxx_latency, xxx_cnt" or
bandwidth events "xxx_flux, xxx_time".
b) By event type, such as "event=0xXXXX, event=0x1XXXX".

Use group to count the related events:
  [1] -e "{pmu_name/xxx_latency,port=1/,pmu_name/xxx_cnt,port=1/}"

  example:
    1st event: hisi_pcie0_core1/event=0x804,port=1
    2nd event: hisi_pcie0_core1/event=0x10804,port=1

  test cmd:
    perf stat -e hisi_pcie0_core1/event=0x804,port=1/ \
               -e hisi_pcie0_core1/event=0x10804,port=1/

  before patch:
            25,281      hisi_pcie0_core1/event=0x804,port=1/    (49.91%)
           470,598      hisi_pcie0_core1/event=0x10804,port=1/    (50.09%)

  after patch:
            24,147      hisi_pcie0_core1/event=0x804,port=1/
           474,558      hisi_pcie0_core1/event=0x10804,port=1/

Signed-off-by: Junhao He <hejunhao3@huawei.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/perf/hisilicon/hisi_pcie_pmu.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
index b2dde7559639..5b15f3698188 100644
--- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
+++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
@@ -409,14 +409,10 @@ static int hisi_pcie_pmu_find_related_event(struct hisi_pcie_pmu *pcie_pmu,
 		if (!sibling)
 			continue;
 
-		if (!hisi_pcie_pmu_cmp_event(sibling, event))
-			continue;
-
 		/* Related events must be used in group */
-		if (sibling->group_leader == event->group_leader)
+		if (hisi_pcie_pmu_cmp_event(sibling, event) &&
+		    sibling->group_leader == event->group_leader)
 			return idx;
-		else
-			return -EINVAL;
 	}
 
 	return idx;
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 6/8] drivers/perf: hisi_pcie: Relax the check on related events

[Jonathan Cameron]

On Fri, 23 Feb 2024 18:33:57 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Junhao He <hejunhao3@huawei.com>
> 
> If we use two events with the same filter and related event type
> (see the following example), the driver check whether they are related
> events and are in the same group, otherwise the function
> hisi_pcie_pmu_find_related_event() return -EINVAL, then the 2nd event
> cannot count but the 1st event is running, although the PCIe PMU has
> other idle counters.
> 
> In this case, The perf event scheduler will make the two events to
> multiplex a counter, if the user use the formula
> (1st event_value / 2nd event_value) to calculate the bandwidth, he/she
> won't get the correct value, because they are not counting at the
> same period.
> 
> This patch tries to fix this by making the related events to use
> different idle counters if they are not in the same event group.
> 
> And finally, I'm going to say. The related events are best used in the
> same group [1]. There are two ways to know if they are related events.
> a) By event name, such as the latency events "xxx_latency, xxx_cnt" or
> bandwidth events "xxx_flux, xxx_time".
> b) By event type, such as "event=0xXXXX, event=0x1XXXX".
> 
> Use group to count the related events:
>   [1] -e "{pmu_name/xxx_latency,port=1/,pmu_name/xxx_cnt,port=1/}"
> 
>   example:
>     1st event: hisi_pcie0_core1/event=0x804,port=1
>     2nd event: hisi_pcie0_core1/event=0x10804,port=1
> 
>   test cmd:
>     perf stat -e hisi_pcie0_core1/event=0x804,port=1/ \
>                -e hisi_pcie0_core1/event=0x10804,port=1/
> 
>   before patch:
>             25,281      hisi_pcie0_core1/event=0x804,port=1/    (49.91%)
>            470,598      hisi_pcie0_core1/event=0x10804,port=1/    (50.09%)
> 
>   after patch:
>             24,147      hisi_pcie0_core1/event=0x804,port=1/
>            474,558      hisi_pcie0_core1/event=0x10804,port=1/
> 
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huwei.com>

> ---
>  drivers/perf/hisilicon/hisi_pcie_pmu.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
> index b2dde7559639..5b15f3698188 100644
> --- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
> @@ -409,14 +409,10 @@ static int hisi_pcie_pmu_find_related_event(struct hisi_pcie_pmu *pcie_pmu,
>  		if (!sibling)
>  			continue;
>  
> -		if (!hisi_pcie_pmu_cmp_event(sibling, event))
> -			continue;
> -
>  		/* Related events must be used in group */
> -		if (sibling->group_leader == event->group_leader)
> +		if (hisi_pcie_pmu_cmp_event(sibling, event) &&
> +		    sibling->group_leader == event->group_leader)
>  			return idx;
> -		else
> -			return -EINVAL;
>  	}
>  
>  	return idx;


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 7/8] drivers/perf: hisi_pcie: Merge find_related_event() and get_event_idx()

[Yicong Yang]

From: Junhao He <hejunhao3@huawei.com>

The function xxx_find_related_event() scan all working events to find
related events. During this process, we also can find the idle counters.
If not found related events, return the first idle counter to simplify
the code.

Signed-off-by: Junhao He <hejunhao3@huawei.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/perf/hisilicon/hisi_pcie_pmu.c | 51 ++++++++++----------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
index 5b15f3698188..5d1f0e9fdb08 100644
--- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
+++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
@@ -398,16 +398,24 @@ static u64 hisi_pcie_pmu_read_counter(struct perf_event *event)
 	return hisi_pcie_pmu_readq(pcie_pmu, event->hw.event_base, idx);
 }
 
-static int hisi_pcie_pmu_find_related_event(struct hisi_pcie_pmu *pcie_pmu,
-					    struct perf_event *event)
+/*
+ * Check all work events, if a relevant event is found then we return it
+ * first, otherwise return the first idle counter (need to reset).
+ */
+static int hisi_pcie_pmu_get_event_idx(struct hisi_pcie_pmu *pcie_pmu,
+					struct perf_event *event)
 {
+	int first_idle = -EAGAIN;
 	struct perf_event *sibling;
 	int idx;
 
 	for (idx = 0; idx < HISI_PCIE_MAX_COUNTERS; idx++) {
 		sibling = pcie_pmu->hw_events[idx];
-		if (!sibling)
+		if (!sibling) {
+			if (first_idle == -EAGAIN)
+				first_idle = idx;
 			continue;
+		}
 
 		/* Related events must be used in group */
 		if (hisi_pcie_pmu_cmp_event(sibling, event) &&
@@ -415,19 +423,7 @@ static int hisi_pcie_pmu_find_related_event(struct hisi_pcie_pmu *pcie_pmu,
 			return idx;
 	}
 
-	return idx;
-}
-
-static int hisi_pcie_pmu_get_event_idx(struct hisi_pcie_pmu *pcie_pmu)
-{
-	int idx;
-
-	for (idx = 0; idx < HISI_PCIE_MAX_COUNTERS; idx++) {
-		if (!pcie_pmu->hw_events[idx])
-			return idx;
-	}
-
-	return -EINVAL;
+	return first_idle;
 }
 
 static void hisi_pcie_pmu_event_update(struct perf_event *event)
@@ -553,27 +549,18 @@ static int hisi_pcie_pmu_add(struct perf_event *event, int flags)
 
 	hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
 
-	/* Check all working events to find a related event. */
-	idx = hisi_pcie_pmu_find_related_event(pcie_pmu, event);
-	if (idx < 0)
-		return idx;
-
-	/* Current event shares an enabled counter with the related event */
-	if (idx < HISI_PCIE_MAX_COUNTERS) {
-		hwc->idx = idx;
-		goto start_count;
-	}
-
-	idx = hisi_pcie_pmu_get_event_idx(pcie_pmu);
+	idx = hisi_pcie_pmu_get_event_idx(pcie_pmu, event);
 	if (idx < 0)
 		return idx;
 
 	hwc->idx = idx;
-	pcie_pmu->hw_events[idx] = event;
-	/* Reset Counter to avoid previous statistic interference. */
-	hisi_pcie_pmu_reset_counter(pcie_pmu, idx);
 
-start_count:
+	/* No enabled counter found with related event, reset it */
+	if (!pcie_pmu->hw_events[idx]) {
+		hisi_pcie_pmu_reset_counter(pcie_pmu, idx);
+		pcie_pmu->hw_events[idx] = event;
+	}
+
 	if (flags & PERF_EF_START)
 		hisi_pcie_pmu_start(event, PERF_EF_RELOAD);
 
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 7/8] drivers/perf: hisi_pcie: Merge find_related_event() and get_event_idx()

[Jonathan Cameron]

On Fri, 23 Feb 2024 18:33:58 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Junhao He <hejunhao3@huawei.com>
> 
> The function xxx_find_related_event() scan all working events to find
> related events. During this process, we also can find the idle counters.
> If not found related events, return the first idle counter to simplify
> the code.
> 
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Seems like a small optimization, but worthwhile.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 8/8] docs: perf: Update usage for target filter of hisi-pcie-pmu

[Yicong Yang]

From: Junhao He <hejunhao3@huawei.com>

One of the "port" and "bdf" target filter interface must be set, and
the related events should preferably used in the same group.
Update the usage in the documentation.

Signed-off-by: Junhao He <hejunhao3@huawei.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 .../admin-guide/perf/hisi-pcie-pmu.rst        | 31 ++++++++++++++-----
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/Documentation/admin-guide/perf/hisi-pcie-pmu.rst b/Documentation/admin-guide/perf/hisi-pcie-pmu.rst
index 7e863662e2d4..678d3865560c 100644
--- a/Documentation/admin-guide/perf/hisi-pcie-pmu.rst
+++ b/Documentation/admin-guide/perf/hisi-pcie-pmu.rst
@@ -37,9 +37,20 @@ Example usage of perf::
   hisi_pcie0_core0/rx_mwr_cnt/ [kernel PMU event]
   ------------------------------------------
 
-  $# perf stat -e hisi_pcie0_core0/rx_mwr_latency/
-  $# perf stat -e hisi_pcie0_core0/rx_mwr_cnt/
-  $# perf stat -g -e hisi_pcie0_core0/rx_mwr_latency/ -e hisi_pcie0_core0/rx_mwr_cnt/
+  $# perf stat -e hisi_pcie0_core0/rx_mwr_latency,port=0xffff/
+  $# perf stat -e hisi_pcie0_core0/rx_mwr_cnt,port=0xffff/
+
+The related events usually used to calculate the bandwidth, latency or others.
+They need to start and end counting at the same time, therefore related events
+are best used in the same event group to get the expected value. There are two
+ways to know if they are related events:
+a) By event name, such as the latency events "xxx_latency, xxx_cnt" or
+   bandwidth events "xxx_flux, xxx_time".
+b) By event type, such as "event=0xXXXX, event=0x1XXXX".
+
+Example usage of perf group::
+
+  $# perf stat -e "{hisi_pcie0_core0/rx_mwr_latency,port=0xffff/,hisi_pcie0_core0/rx_mwr_cnt,port=0xffff/}"
 
 The current driver does not support sampling. So "perf record" is unsupported.
 Also attach to a task is unsupported for PCIe PMU.
@@ -51,8 +62,12 @@ Filter options
 
    PMU could only monitor the performance of traffic downstream target Root
    Ports or downstream target Endpoint. PCIe PMU driver support "port" and
-   "bdf" interfaces for users, and these two interfaces aren't supported at the
-   same time.
+   "bdf" interfaces for users.
+   Please notice that, one of these two interfaces must be set, and these two
+   interfaces aren't supported at the same time. If they are both set, only
+   "port" filter is valid.
+   If "port" filter not being set or is set explicitly to zero (default), the
+   "bdf" filter will be in effect, because "bdf=0" meaning 0000:000:00.0.
 
    - port
 
@@ -95,7 +110,7 @@ Filter options
 
    Example usage of perf::
 
-     $# perf stat -e hisi_pcie0_core0/rx_mrd_flux,trig_len=0x4,trig_mode=1/ sleep 5
+     $# perf stat -e hisi_pcie0_core0/rx_mrd_flux,port=0xffff,trig_len=0x4,trig_mode=1/ sleep 5
 
 3. Threshold filter
 
@@ -109,7 +124,7 @@ Filter options
 
    Example usage of perf::
 
-     $# perf stat -e hisi_pcie0_core0/rx_mrd_flux,thr_len=0x4,thr_mode=1/ sleep 5
+     $# perf stat -e hisi_pcie0_core0/rx_mrd_flux,port=0xffff,thr_len=0x4,thr_mode=1/ sleep 5
 
 4. TLP Length filter
 
@@ -127,4 +142,4 @@ Filter options
 
    Example usage of perf::
 
-     $# perf stat -e hisi_pcie0_core0/rx_mrd_flux,len_mode=0x1/ sleep 5
+     $# perf stat -e hisi_pcie0_core0/rx_mrd_flux,port=0xffff,len_mode=0x1/ sleep 5
-- 
2.24.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 8/8] docs: perf: Update usage for target filter of hisi-pcie-pmu

[Jonathan Cameron]

On Fri, 23 Feb 2024 18:33:59 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Junhao He <hejunhao3@huawei.com>
> 
> One of the "port" and "bdf" target filter interface must be set, and
> the related events should preferably used in the same group.
> Update the usage in the documentation.
> 
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/8] drivers/perf: hisi_pcie: Several updates for HiSilicon PCIe PMU driver

[Will Deacon]

On Fri, 23 Feb 2024 18:33:51 +0800, Yicong Yang wrote:
> This series mainly fix and optimize the several usage of the driver:
> - Add more events to complement the TLP bandwidth counting
> - Fix the wrong counting on using the event group
> - Properly check the target filter to avoid invalid filter value
> - Optimize the handling of related events which are not in an event group
> - Update the docs
> 
> [...]

Applied to will (for-next/perf), thanks!

[1/8] drivers/perf: hisi_pcie: Rename hisi_pcie_pmu_{config,clear}_filter()
      https://git.kernel.org/will/c/54a9e47eebb9
[2/8] drivers/perf: hisi_pcie: Introduce hisi_pcie_pmu_get_event_ctrl_val()
      https://git.kernel.org/will/c/4d473461e094
[3/8] drivers/perf: hisi_pcie: Fix incorrect counting under metric mode
      https://git.kernel.org/will/c/b6693ad68e27
[4/8] drivers/perf: hisi_pcie: Add more events for counting TLP bandwidth
      https://git.kernel.org/will/c/00ca69b856ba
[5/8] drivers/perf: hisi_pcie: Check the target filter properly
      https://git.kernel.org/will/c/2f864fee0851
[6/8] drivers/perf: hisi_pcie: Relax the check on related events
      https://git.kernel.org/will/c/2fbf96ed883a
[7/8] drivers/perf: hisi_pcie: Merge find_related_event() and get_event_idx()
      https://git.kernel.org/will/c/7da377059ee6
[8/8] docs: perf: Update usage for target filter of hisi-pcie-pmu
      https://git.kernel.org/will/c/89a032923d4b

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel