[PATCH] Partially revert "perf/arm-cmn: Optimise DTC counter accesses"

[PATCH] Partially revert "perf/arm-cmn: Optimise DTC counter accesses"

[Robin Murphy]

It turns out the optimisation implemented by commit 4f2c3872dde5 is
totally broken, since all the places that consume hw->dtcs_used for
events other than cycle count are still not expecting it to be sparsely
populated, and fail to read all the relevant DTC counters correctly if
so.

If implemented correctly, the optimisation potentially saves up to 3
register reads per event update, which is reasonably significant for
events targeting a single node, but still not worth a massive amount of
additional code complexity overall. Getting it right within the current
design looks a fair bit more involved than it was ever intended to be,
so let's just make a functional revert which restores the old behaviour
while still backporting easily.

Fixes: 4f2c3872dde5 ("perf/arm-cmn: Optimise DTC counter accesses")
Reported-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/perf/arm-cmn.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
index b80a9b74662b..1deb61b22bc7 100644
--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -1576,7 +1576,6 @@ static int arm_cmn_event_init(struct perf_event *event)
 			hw->dn++;
 			continue;
 		}
-		hw->dtcs_used |= arm_cmn_node_to_xp(cmn, dn)->dtc;
 		hw->num_dns++;
 		if (bynodeid)
 			break;
@@ -1589,6 +1588,12 @@ static int arm_cmn_event_init(struct perf_event *event)
 			nodeid, nid.x, nid.y, nid.port, nid.dev, type);
 		return -EINVAL;
 	}
+	/*
+	 * Keep assuming non-cycles events count in all DTC domains; turns out
+	 * it's hard to make a worthwhile optimisation around this, short of
+	 * going all-in with domain-local counter allocation as well.
+	 */
+	hw->dtcs_used = (1U << cmn->num_dtcs) - 1;
 
 	return arm_cmn_validate_group(cmn, event);
 }
-- 
2.36.1.dirty

[PATCH] Partially revert "perf/arm-cmn: Optimise DTC counter accesses"

[Robin Murphy]

It turns out the optimisation implemented by commit 4f2c3872dde5 is
totally broken, since all the places that consume hw->dtcs_used for
events other than cycle count are still not expecting it to be sparsely
populated, and fail to read all the relevant DTC counters correctly if
so.

If implemented correctly, the optimisation potentially saves up to 3
register reads per event update, which is reasonably significant for
events targeting a single node, but still not worth a massive amount of
additional code complexity overall. Getting it right within the current
design looks a fair bit more involved than it was ever intended to be,
so let's just make a functional revert which restores the old behaviour
while still backporting easily.

Fixes: 4f2c3872dde5 ("perf/arm-cmn: Optimise DTC counter accesses")
Reported-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/perf/arm-cmn.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
index b80a9b74662b..1deb61b22bc7 100644
--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -1576,7 +1576,6 @@ static int arm_cmn_event_init(struct perf_event *event)
 			hw->dn++;
 			continue;
 		}
-		hw->dtcs_used |= arm_cmn_node_to_xp(cmn, dn)->dtc;
 		hw->num_dns++;
 		if (bynodeid)
 			break;
@@ -1589,6 +1588,12 @@ static int arm_cmn_event_init(struct perf_event *event)
 			nodeid, nid.x, nid.y, nid.port, nid.dev, type);
 		return -EINVAL;
 	}
+	/*
+	 * Keep assuming non-cycles events count in all DTC domains; turns out
+	 * it's hard to make a worthwhile optimisation around this, short of
+	 * going all-in with domain-local counter allocation as well.
+	 */
+	hw->dtcs_used = (1U << cmn->num_dtcs) - 1;
 
 	return arm_cmn_validate_group(cmn, event);
 }
-- 
2.36.1.dirty


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

Re: [PATCH] Partially revert "perf/arm-cmn: Optimise DTC counter accesses"

[Ilkka Koskinen]

Hi Robin,

On Mon, 23 Jan 2023, Robin Murphy wrote:
> It turns out the optimisation implemented by commit 4f2c3872dde5 is
> totally broken, since all the places that consume hw->dtcs_used for
> events other than cycle count are still not expecting it to be sparsely
> populated, and fail to read all the relevant DTC counters correctly if
> so.
>
> If implemented correctly, the optimisation potentially saves up to 3
> register reads per event update, which is reasonably significant for
> events targeting a single node, but still not worth a massive amount of
> additional code complexity overall. Getting it right within the current
> design looks a fair bit more involved than it was ever intended to be,
> so let's just make a functional revert which restores the old behaviour
> while still backporting easily.
>
> Fixes: 4f2c3872dde5 ("perf/arm-cmn: Optimise DTC counter accesses")
> Reported-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Thanks for the patch. It looks good to me and seems to work fine.

Cheers, Ilkka

> ---
> drivers/perf/arm-cmn.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index b80a9b74662b..1deb61b22bc7 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -1576,7 +1576,6 @@ static int arm_cmn_event_init(struct perf_event *event)
> 			hw->dn++;
> 			continue;
> 		}
> -		hw->dtcs_used |= arm_cmn_node_to_xp(cmn, dn)->dtc;
> 		hw->num_dns++;
> 		if (bynodeid)
> 			break;
> @@ -1589,6 +1588,12 @@ static int arm_cmn_event_init(struct perf_event *event)
> 			nodeid, nid.x, nid.y, nid.port, nid.dev, type);
> 		return -EINVAL;
> 	}
> +	/*
> +	 * Keep assuming non-cycles events count in all DTC domains; turns out
> +	 * it's hard to make a worthwhile optimisation around this, short of
> +	 * going all-in with domain-local counter allocation as well.
> +	 */
> +	hw->dtcs_used = (1U << cmn->num_dtcs) - 1;
>
> 	return arm_cmn_validate_group(cmn, event);
> }
> -- 
> 2.36.1.dirty
>
>

Re: [PATCH] Partially revert "perf/arm-cmn: Optimise DTC counter accesses"

[Ilkka Koskinen]

Hi Robin,

On Mon, 23 Jan 2023, Robin Murphy wrote:
> It turns out the optimisation implemented by commit 4f2c3872dde5 is
> totally broken, since all the places that consume hw->dtcs_used for
> events other than cycle count are still not expecting it to be sparsely
> populated, and fail to read all the relevant DTC counters correctly if
> so.
>
> If implemented correctly, the optimisation potentially saves up to 3
> register reads per event update, which is reasonably significant for
> events targeting a single node, but still not worth a massive amount of
> additional code complexity overall. Getting it right within the current
> design looks a fair bit more involved than it was ever intended to be,
> so let's just make a functional revert which restores the old behaviour
> while still backporting easily.
>
> Fixes: 4f2c3872dde5 ("perf/arm-cmn: Optimise DTC counter accesses")
> Reported-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Thanks for the patch. It looks good to me and seems to work fine.

Cheers, Ilkka

> ---
> drivers/perf/arm-cmn.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index b80a9b74662b..1deb61b22bc7 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -1576,7 +1576,6 @@ static int arm_cmn_event_init(struct perf_event *event)
> 			hw->dn++;
> 			continue;
> 		}
> -		hw->dtcs_used |= arm_cmn_node_to_xp(cmn, dn)->dtc;
> 		hw->num_dns++;
> 		if (bynodeid)
> 			break;
> @@ -1589,6 +1588,12 @@ static int arm_cmn_event_init(struct perf_event *event)
> 			nodeid, nid.x, nid.y, nid.port, nid.dev, type);
> 		return -EINVAL;
> 	}
> +	/*
> +	 * Keep assuming non-cycles events count in all DTC domains; turns out
> +	 * it's hard to make a worthwhile optimisation around this, short of
> +	 * going all-in with domain-local counter allocation as well.
> +	 */
> +	hw->dtcs_used = (1U << cmn->num_dtcs) - 1;
>
> 	return arm_cmn_validate_group(cmn, event);
> }
> -- 
> 2.36.1.dirty
>
>

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

Re: [PATCH] Partially revert "perf/arm-cmn: Optimise DTC counter accesses"

[Will Deacon]

On Mon, 23 Jan 2023 18:30:38 +0000, Robin Murphy wrote:
> It turns out the optimisation implemented by commit 4f2c3872dde5 is
> totally broken, since all the places that consume hw->dtcs_used for
> events other than cycle count are still not expecting it to be sparsely
> populated, and fail to read all the relevant DTC counters correctly if
> so.
> 
> If implemented correctly, the optimisation potentially saves up to 3
> register reads per event update, which is reasonably significant for
> events targeting a single node, but still not worth a massive amount of
> additional code complexity overall. Getting it right within the current
> design looks a fair bit more involved than it was ever intended to be,
> so let's just make a functional revert which restores the old behaviour
> while still backporting easily.
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] Partially revert "perf/arm-cmn: Optimise DTC counter accesses"
      https://git.kernel.org/arm64/c/a428eb4b99ab

Cheers,
-- 
Will

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

Re: [PATCH] Partially revert "perf/arm-cmn: Optimise DTC counter accesses"

[Will Deacon]

On Mon, 23 Jan 2023 18:30:38 +0000, Robin Murphy wrote:
> It turns out the optimisation implemented by commit 4f2c3872dde5 is
> totally broken, since all the places that consume hw->dtcs_used for
> events other than cycle count are still not expecting it to be sparsely
> populated, and fail to read all the relevant DTC counters correctly if
> so.
> 
> If implemented correctly, the optimisation potentially saves up to 3
> register reads per event update, which is reasonably significant for
> events targeting a single node, but still not worth a massive amount of
> additional code complexity overall. Getting it right within the current
> design looks a fair bit more involved than it was ever intended to be,
> so let's just make a functional revert which restores the old behaviour
> while still backporting easily.
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] Partially revert "perf/arm-cmn: Optimise DTC counter accesses"
      https://git.kernel.org/arm64/c/a428eb4b99ab

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