From: Yicong Yang <[email protected]>
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/[email protected]/
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
From: Yicong Yang <[email protected]>
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 <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
---
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
From: Junhao He <[email protected]>
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 <[email protected]>
Signed-off-by: Yicong Yang <[email protected]>
---
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
From: Junhao He <[email protected]>
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 <[email protected]>
Signed-off-by: Yicong Yang <[email protected]>
---
.../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
From: Yicong Yang <[email protected]>
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 <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
---
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
From: Yicong Yang <[email protected]>
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 <[email protected]>
---
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
From: Yicong Yang <[email protected]>
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 <[email protected]>
---
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
On Fri, 23 Feb 2024 18:33:52 +0800
Yicong Yang <[email protected]> wrote:
> From: Yicong Yang <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
> ---
> 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;
>
On Fri, 23 Feb 2024 18:33:54 +0800
Yicong Yang <[email protected]> wrote:
> From: Yicong Yang <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
On Fri, 23 Feb 2024 18:33:59 +0800
Yicong Yang <[email protected]> wrote:
> From: Junhao He <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Yicong Yang <[email protected]>
LGTM
Reviewed-by: Jonathan Cameron <[email protected]>
On Fri, 23 Feb 2024 18:33:57 +0800
Yicong Yang <[email protected]> wrote:
> From: Junhao He <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Yicong Yang <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> 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;
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