[PATCH v1 1/2] arm64: perf: Correct per-thread mode if the event is not supported

[PATCH v1 1/2] arm64: perf: Correct per-thread mode if the event is not supported

[Leo Yan]

When the perf tool runs in per-thread mode, armpmu_event_init() defers
to handle events in armpmu_add(), the main reason is the selected PMU in
the init phase can mismatch with the CPUs when the profiled task
is scheduled on.

For example, on an Arm big.LTTILE platform with two clusters, every
cluster has its dedicated PMU; the event initialization happens on the
LITTLE cluster and its corresponding PMU is selected, but the profiled
task is scheduled on big cluster, it's deferred to handle this case in
armpmu_add().

Usually, we should report failure in the first place so this can allow
users to easily locate the issue they are facing.  For the per-thread
mode, the profiled task can be migrated on any CPU, therefore the event
can be enabled on any CPU.  In other words, if a PMU detects it fails to
support the process-following event, it can directly returns -EOPNOTSUPP
so can stop profiling.

This patch adds the checking for per-thread mode, if the event is not
supported, return -EOPNOTSUPP.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/perf/arm_pmu.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index d4f7f1f9cc77..aedea060ca8b 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -502,9 +502,9 @@ static int armpmu_event_init(struct perf_event *event)
 	/*
 	 * Reject CPU-affine events for CPUs that are of a different class to
 	 * that which this PMU handles. Process-following events (where
-	 * event->cpu == -1) can be migrated between CPUs, and thus we have to
-	 * reject them later (in armpmu_add) if they're scheduled on a
-	 * different class of CPU.
+	 * event->cpu == -1) can be migrated between CPUs, and thus we will
+	 * reject them when map_event() detects absent entry at below or later
+	 * (in armpmu_add) if they're scheduled on a different class of CPU.
 	 */
 	if (event->cpu != -1 &&
 		!cpumask_test_cpu(event->cpu, &armpmu->supported_cpus))
@@ -514,8 +514,16 @@ static int armpmu_event_init(struct perf_event *event)
 	if (has_branch_stack(event))
 		return -EOPNOTSUPP;
 
-	if (armpmu->map_event(event) == -ENOENT)
+	if (armpmu->map_event(event) == -ENOENT) {
+		/*
+		 * Process-following event is not supported on current PMU,
+		 * returns -EOPNOTSUPP to stop perf at the initialization
+		 * phase.
+		 */
+		if (event->cpu == -1)
+			return -EOPNOTSUPP;
 		return -ENOENT;
+	}
 
 	return __hw_perf_event_init(event);
 }
-- 
2.25.1


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

[PATCH v1 2/2] arm64: perf: Report error if PMU fails to support current CPU

[Leo Yan]

When run perf command on the Arm big.LITTLE system (Juno-r2 board):

  perf record -e cycles --per-thread program

The command executes normally without any errors; but when report the
result with "perf report", it fails to parse the symbols.  This is
because the perf data file doesn't contain MMAP2 events, thus it cannot
find the correct object file for parsing symbols.

On the big.LITTLE system, if the initialized PMU doesn't match with the
CPU the profiled task is scheduled on; for example, the initialized PMU
is on CPU0 in the LITTLE cluster, when invoke the function
perf_event_mmap_event() on CPU2 in the big cluster, the event is always
filtered out due to the CPU2 is not supported by the initialized PMU.
Finally, there have no any MMAP2 samples are generated for the
profiling.

This patch doesn't fix for this issue, alternatively, it simply reports
an error when detect the current CPU is not supported by PMU, so can
remind the user for the abnormal situation.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/perf/arm_pmu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index aedea060ca8b..99ddc8bf6466 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -565,6 +565,9 @@ static int armpmu_filter_match(struct perf_event *event)
 	int ret;
 
 	ret = cpumask_test_cpu(cpu, &armpmu->supported_cpus);
+	if (!ret)
+		pr_err("PMU doesn't support current CPU %d\n", cpu);
+
 	if (ret && armpmu->filter_match)
 		return armpmu->filter_match(event);
 
-- 
2.25.1


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

Re: [PATCH v1 2/2] arm64: perf: Report error if PMU fails to support current CPU

[Mark Rutland]

Hi Leo,

On Tue, Jun 08, 2021 at 10:52:28PM +0800, Leo Yan wrote:
> When run perf command on the Arm big.LITTLE system (Juno-r2 board):
> 
>   perf record -e cycles --per-thread program
> 
> The command executes normally without any errors; but when report the
> result with "perf report", it fails to parse the symbols.  This is
> because the perf data file doesn't contain MMAP2 events, thus it cannot
> find the correct object file for parsing symbols.
> 
> On the big.LITTLE system, if the initialized PMU doesn't match with the
> CPU the profiled task is scheduled on; for example, the initialized PMU
> is on CPU0 in the LITTLE cluster, when invoke the function
> perf_event_mmap_event() on CPU2 in the big cluster, the event is always
> filtered out due to the CPU2 is not supported by the initialized PMU.
> Finally, there have no any MMAP2 samples are generated for the
> profiling.
> 
> This patch doesn't fix for this issue, alternatively, it simply reports
> an error when detect the current CPU is not supported by PMU, so can
> remind the user for the abnormal situation.

This behaviour is by design, and is not a kernel error, so it isn't
appropriate to use pr_err() here. NAK to adding the pr_err().

I agree that the behaviour for PERF_TYPE_HARDWARE is confusing and not
all that great, but as it's ABI it's not something that we can change.

What really needs to happen here is for userspace to gain some awareness
of big.LITTLE. For example, for the command:

$ perf record -e cycles --per-thread program

... what we should do is have userspace identify the set of CPU PMUs
(based on the `cpus` attribute in sysfs), and open a per-thread event on
each of those, which we can then summarize together.

Either that, or print a warning that the system is big.LITTLE and an
event will not count on all CPUs.

Thanks,
Mark.

 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  drivers/perf/arm_pmu.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index aedea060ca8b..99ddc8bf6466 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -565,6 +565,9 @@ static int armpmu_filter_match(struct perf_event *event)
>  	int ret;
>  
>  	ret = cpumask_test_cpu(cpu, &armpmu->supported_cpus);
> +	if (!ret)
> +		pr_err("PMU doesn't support current CPU %d\n", cpu);
> +
>  	if (ret && armpmu->filter_match)
>  		return armpmu->filter_match(event);
>  
> -- 
> 2.25.1
> 

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

Re: [PATCH v1 1/2] arm64: perf: Correct per-thread mode if the event is not supported

[Mark Rutland]

On Tue, Jun 08, 2021 at 10:52:27PM +0800, Leo Yan wrote:
> When the perf tool runs in per-thread mode, armpmu_event_init() defers
> to handle events in armpmu_add(), the main reason is the selected PMU in
> the init phase can mismatch with the CPUs when the profiled task
> is scheduled on.
> 
> For example, on an Arm big.LTTILE platform with two clusters, every
> cluster has its dedicated PMU; the event initialization happens on the
> LITTLE cluster and its corresponding PMU is selected, but the profiled
> task is scheduled on big cluster, it's deferred to handle this case in
> armpmu_add().
> 
> Usually, we should report failure in the first place so this can allow
> users to easily locate the issue they are facing.  For the per-thread
> mode, the profiled task can be migrated on any CPU, therefore the event
> can be enabled on any CPU.  In other words, if a PMU detects it fails to
> support the process-following event, it can directly returns -EOPNOTSUPP
> so can stop profiling.
> 
> This patch adds the checking for per-thread mode, if the event is not
> supported, return -EOPNOTSUPP.

I don't understand the rationale for this patch. We call
armpmu_event_init() from perf_try_init_event(), and if we return *any*
error code that will be returned to userspace, or at least that used to
be the case.

What problem are you trying to solve here?

Is this some fallout of commit:

  55bcf6ef314ae8ba ("perf: Extend PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE")

... ?

Thanks,
Mark.

> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  drivers/perf/arm_pmu.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index d4f7f1f9cc77..aedea060ca8b 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -502,9 +502,9 @@ static int armpmu_event_init(struct perf_event *event)
>  	/*
>  	 * Reject CPU-affine events for CPUs that are of a different class to
>  	 * that which this PMU handles. Process-following events (where
> -	 * event->cpu == -1) can be migrated between CPUs, and thus we have to
> -	 * reject them later (in armpmu_add) if they're scheduled on a
> -	 * different class of CPU.
> +	 * event->cpu == -1) can be migrated between CPUs, and thus we will
> +	 * reject them when map_event() detects absent entry at below or later
> +	 * (in armpmu_add) if they're scheduled on a different class of CPU.
>  	 */
>  	if (event->cpu != -1 &&
>  		!cpumask_test_cpu(event->cpu, &armpmu->supported_cpus))
> @@ -514,8 +514,16 @@ static int armpmu_event_init(struct perf_event *event)
>  	if (has_branch_stack(event))
>  		return -EOPNOTSUPP;
>  
> -	if (armpmu->map_event(event) == -ENOENT)
> +	if (armpmu->map_event(event) == -ENOENT) {
> +		/*
> +		 * Process-following event is not supported on current PMU,
> +		 * returns -EOPNOTSUPP to stop perf at the initialization
> +		 * phase.
> +		 */
> +		if (event->cpu == -1)
> +			return -EOPNOTSUPP;
>  		return -ENOENT;
> +	}
>  
>  	return __hw_perf_event_init(event);
>  }
> -- 
> 2.25.1
> 

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