[kvm-unit-tests 1/1] arm64: microbench: Move the read of the count register and the ISB operation out of the while loop

[kvm-unit-tests 1/1] arm64: microbench: Move the read of the count register and the ISB operation out of the while loop

[heqiong]

Reducing the impact of the cntvct_el0 register and isb() operation
on microbenchmark test results to improve testing accuracy and reduce
latency in test results.
---
 arm/micro-bench.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/arm/micro-bench.c b/arm/micro-bench.c
index fbe59d03..6b940d56 100644
--- a/arm/micro-bench.c
+++ b/arm/micro-bench.c
@@ -346,17 +346,21 @@ static void loop_test(struct exit_test *test)
 		}
 	}
 
+	dsb(ish);
+	isb();
+	start = read_sysreg(cntpct_el0);
+	isb();
 	while (ntimes < test->times && total_ns.ns < NS_5_SECONDS) {
-		isb();
-		start = read_sysreg(cntvct_el0);
 		test->exec();
-		isb();
-		end = read_sysreg(cntvct_el0);
 
 		ntimes++;
-		total_ticks += (end - start);
-		ticks_to_ns_time(total_ticks, &total_ns);
 	}
+	dsb(ish);
+	isb();
+	end = read_sysreg(cntpct_el0);
+
+	total_ticks = end - start;
+	ticks_to_ns_time(total_ticks, &total_ns);
 
 	if (test->post) {
 		test->post(ntimes, &total_ticks);
-- 
2.31.1

Re: [kvm-unit-tests 1/1] arm64: microbench: Move the read of the count register and the ISB operation out of the while loop

[Andrew Jones]

Thanks for submitting the patch more correctly, but there's still two
more problems with the patch submission. The patch summary (email subject)
is too long. It also simply describes the change of implementation, which
is easy to see when looking at the patch. It should instead describe the
purpose of the patch, e.g.

  arm64: microbench: Improve measurement accuracy of tests

The second problem is it's missing your signed-off-by (which I think I
pointed out last time too).

Please see [1] for more information about patch formatting. You can also
run the Linux kernel's scripts/checkpatch.pl on the patch to catch these
types of things as well as other code style issues.

[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format

Thanks,
drew

On Tue, Nov 07, 2023 at 02:40:06PM +0800, heqiong wrote:
> Reducing the impact of the cntvct_el0 register and isb() operation
> on microbenchmark test results to improve testing accuracy and reduce
> latency in test results.
> ---
>  arm/micro-bench.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/arm/micro-bench.c b/arm/micro-bench.c
> index fbe59d03..6b940d56 100644
> --- a/arm/micro-bench.c
> +++ b/arm/micro-bench.c
> @@ -346,17 +346,21 @@ static void loop_test(struct exit_test *test)
>  		}
>  	}
>  
> +	dsb(ish);
> +	isb();
> +	start = read_sysreg(cntpct_el0);
> +	isb();
>  	while (ntimes < test->times && total_ns.ns < NS_5_SECONDS) {
> -		isb();
> -		start = read_sysreg(cntvct_el0);
>  		test->exec();
> -		isb();
> -		end = read_sysreg(cntvct_el0);
>  
>  		ntimes++;
> -		total_ticks += (end - start);
> -		ticks_to_ns_time(total_ticks, &total_ns);
>  	}
> +	dsb(ish);
> +	isb();
> +	end = read_sysreg(cntpct_el0);
> +
> +	total_ticks = end - start;
> +	ticks_to_ns_time(total_ticks, &total_ns);
>  
>  	if (test->post) {
>  		test->post(ntimes, &total_ticks);
> -- 
> 2.31.1
> 

Re: [kvm-unit-tests 1/1] arm64: microbench: Move the read of the count register and the ISB operation out of the while loop

[Alexandru Elisei]

Hi,

On Tue, Nov 07, 2023 at 02:40:06PM +0800, heqiong wrote:
> Reducing the impact of the cntvct_el0 register and isb() operation
> on microbenchmark test results to improve testing accuracy and reduce
> latency in test results.
> ---
>  arm/micro-bench.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/arm/micro-bench.c b/arm/micro-bench.c
> index fbe59d03..6b940d56 100644
> --- a/arm/micro-bench.c
> +++ b/arm/micro-bench.c
> @@ -346,17 +346,21 @@ static void loop_test(struct exit_test *test)
>  		}
>  	}
>  
> +	dsb(ish);
> +	isb();
> +	start = read_sysreg(cntpct_el0);

I still think it would be interesting to have an explanation why CNTVCT_EL0
was replaced with CNTPCT_EL0.

Thanks,
Alex

> +	isb();
>  	while (ntimes < test->times && total_ns.ns < NS_5_SECONDS) {
> -		isb();
> -		start = read_sysreg(cntvct_el0);
>  		test->exec();
> -		isb();
> -		end = read_sysreg(cntvct_el0);
>  
>  		ntimes++;
> -		total_ticks += (end - start);
> -		ticks_to_ns_time(total_ticks, &total_ns);
>  	}
> +	dsb(ish);
> +	isb();
> +	end = read_sysreg(cntpct_el0);
> +
> +	total_ticks = end - start;
> +	ticks_to_ns_time(total_ticks, &total_ns);
>  
>  	if (test->post) {
>  		test->post(ntimes, &total_ticks);
> -- 
> 2.31.1
> 

[kvm-unit-tests PATCH 1/1] arm64: microbench: Move the read of the count register and the ISB operation out of the while loop

[heqiong]

Reducing the impact of the cntvct_el0 register and isb() operation
on microbenchmark test results to improve testing accuracy and reduce
latency in test results.
---
 arm/micro-bench.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/arm/micro-bench.c b/arm/micro-bench.c
index fbe59d03..65f4c4dd 100644
--- a/arm/micro-bench.c
+++ b/arm/micro-bench.c
@@ -346,17 +346,21 @@ static void loop_test(struct exit_test *test)
 		}
 	}
 
+	dsb(ish);
+	isb();
+	start = read_sysreg(cntvct_el0);
+	isb();
 	while (ntimes < test->times && total_ns.ns < NS_5_SECONDS) {
-		isb();
-		start = read_sysreg(cntvct_el0);
 		test->exec();
-		isb();
-		end = read_sysreg(cntvct_el0);
 
 		ntimes++;
-		total_ticks += (end - start);
-		ticks_to_ns_time(total_ticks, &total_ns);
 	}
+	dsb(ish);
+	isb();
+	end = read_sysreg(cntvct_el0);
+
+	total_ticks = end - start;
+	ticks_to_ns_time(total_ticks, &total_ns);
 
 	if (test->post) {
 		test->post(ntimes, &total_ticks);
-- 
2.39.3

Re: [kvm-unit-tests PATCH 1/1] arm64: microbench: Move the read of the count register and the ISB operation out of the while loop

[Alexandru Elisei]

Hi,

On Tue, Nov 07, 2023 at 05:51:15PM +0800, heqiong wrote:
> Reducing the impact of the cntvct_el0 register and isb() operation
> on microbenchmark test results to improve testing accuracy and reduce
> latency in test results.
> ---
>  arm/micro-bench.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/arm/micro-bench.c b/arm/micro-bench.c
> index fbe59d03..65f4c4dd 100644
> --- a/arm/micro-bench.c
> +++ b/arm/micro-bench.c
> @@ -346,17 +346,21 @@ static void loop_test(struct exit_test *test)
>  		}
>  	}
>  
> +	dsb(ish);
> +	isb();
> +	start = read_sysreg(cntvct_el0);
> +	isb();
>  	while (ntimes < test->times && total_ns.ns < NS_5_SECONDS) {
				       ^^^^^^^^^^^^^^^^^^^^^^^^^^
This will always evaluate to true because total_ns is now computed at the
end of the loop instead of every iteration.

Do we want to drop the upper bound on how long a test takes to execute? I
don't have an opinion about it.

Thanks,
Alex

> -		isb();
> -		start = read_sysreg(cntvct_el0);
>  		test->exec();
> -		isb();
> -		end = read_sysreg(cntvct_el0);
>  
>  		ntimes++;
> -		total_ticks += (end - start);
> -		ticks_to_ns_time(total_ticks, &total_ns);
>  	}
> +	dsb(ish);
> +	isb();
> +	end = read_sysreg(cntvct_el0);
> +
> +	total_ticks = end - start;
> +	ticks_to_ns_time(total_ticks, &total_ns);
>  
>  	if (test->post) {
>  		test->post(ntimes, &total_ticks);
> -- 
> 2.39.3
> 

Re: [kvm-unit-tests PATCH 1/1] arm64: microbench: Move the read of the count register and the ISB operation out of the while loop

[Zenghui Yu]

Hi,

In case that you may have trouble receiving emails from the @linux.dev
addresses (?), please check your inbox again and read the comments that
Drew had replied to your previous versions. You can otherwise read them
on lore, see below.

Zenghui

[1] https://lore.kernel.org/kvm/20231101-923f359769ccf8db69c25c4f@orel
[2] https://lore.kernel.org/kvm/20231107-9b361591b5d43284d4394f8a@orel

[kvm-unit-tests PATCH 1/1] arm64: microbench: Improve measurement accuracy of tests

[heqiong]

Reducing the impact of the cntvct_el0 register and isb() operation
on microbenchmark test results to improve testing accuracy and reduce
latency in test results.

Signed-off-by: heqiong <heqiong1557@phytium.com.cn>
---
 arm/micro-bench.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/arm/micro-bench.c b/arm/micro-bench.c
index fbe59d03..22408955 100644
--- a/arm/micro-bench.c
+++ b/arm/micro-bench.c
@@ -24,7 +24,6 @@
 #include <asm/gic-v3-its.h>
 #include <asm/timer.h>
 
-#define NS_5_SECONDS		(5 * 1000 * 1000 * 1000UL)
 #define QEMU_MMIO_ADDR		0x0a000008
 
 static u32 cntfrq;
@@ -346,17 +345,21 @@ static void loop_test(struct exit_test *test)
 		}
 	}
 
-	while (ntimes < test->times && total_ns.ns < NS_5_SECONDS) {
-		isb();
-		start = read_sysreg(cntvct_el0);
+	dsb(ish);
+	isb();
+	start = read_sysreg(cntvct_el0);
+	isb();
+	while (ntimes < test->times) {
 		test->exec();
-		isb();
-		end = read_sysreg(cntvct_el0);
 
 		ntimes++;
-		total_ticks += (end - start);
-		ticks_to_ns_time(total_ticks, &total_ns);
 	}
+	dsb(ish);
+	isb();
+	end = read_sysreg(cntvct_el0);
+
+	total_ticks = end - start;
+	ticks_to_ns_time(total_ticks, &total_ns);
 
 	if (test->post) {
 		test->post(ntimes, &total_ticks);
-- 
2.39.3

Re: [kvm-unit-tests PATCH 1/1] arm64: microbench: Improve measurement accuracy of tests

[Andrew Jones]

Thanks, Heqiong. The patch is looking much better. The only thing missing
now is the patch version. This is v3, so it should have a prefix like this

 [kvm-unit-tests PATCH v3 1/1]

There's no need to respin for that though. afaict you've addressed
Alexandru's comments, but I'll let him take a look before merging.

Thanks,
drew

On Thu, Nov 16, 2023 at 12:53:55PM +0800, heqiong wrote:
> Reducing the impact of the cntvct_el0 register and isb() operation
> on microbenchmark test results to improve testing accuracy and reduce
> latency in test results.
> 
> Signed-off-by: heqiong <heqiong1557@phytium.com.cn>
> ---
>  arm/micro-bench.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/arm/micro-bench.c b/arm/micro-bench.c
> index fbe59d03..22408955 100644
> --- a/arm/micro-bench.c
> +++ b/arm/micro-bench.c
> @@ -24,7 +24,6 @@
>  #include <asm/gic-v3-its.h>
>  #include <asm/timer.h>
>  
> -#define NS_5_SECONDS		(5 * 1000 * 1000 * 1000UL)
>  #define QEMU_MMIO_ADDR		0x0a000008
>  
>  static u32 cntfrq;
> @@ -346,17 +345,21 @@ static void loop_test(struct exit_test *test)
>  		}
>  	}
>  
> -	while (ntimes < test->times && total_ns.ns < NS_5_SECONDS) {
> -		isb();
> -		start = read_sysreg(cntvct_el0);
> +	dsb(ish);
> +	isb();
> +	start = read_sysreg(cntvct_el0);
> +	isb();
> +	while (ntimes < test->times) {
>  		test->exec();
> -		isb();
> -		end = read_sysreg(cntvct_el0);
>  
>  		ntimes++;
> -		total_ticks += (end - start);
> -		ticks_to_ns_time(total_ticks, &total_ns);
>  	}
> +	dsb(ish);
> +	isb();
> +	end = read_sysreg(cntvct_el0);
> +
> +	total_ticks = end - start;
> +	ticks_to_ns_time(total_ticks, &total_ns);
>  
>  	if (test->post) {
>  		test->post(ntimes, &total_ticks);
> -- 
> 2.39.3
> 

Re: [kvm-unit-tests PATCH 1/1] arm64: microbench: Improve measurement accuracy of tests

[Alexandru Elisei]

Hi,

On Thu, Nov 16, 2023 at 12:53:55PM +0800, heqiong wrote:
> Reducing the impact of the cntvct_el0 register and isb() operation
> on microbenchmark test results to improve testing accuracy and reduce
> latency in test results.

Sorry, lost track of which version is the latest - that's why patch version
numbers are really useful!

Everything look alright to me:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,
Alex

> 
> Signed-off-by: heqiong <heqiong1557@phytium.com.cn>
> ---
>  arm/micro-bench.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/arm/micro-bench.c b/arm/micro-bench.c
> index fbe59d03..22408955 100644
> --- a/arm/micro-bench.c
> +++ b/arm/micro-bench.c
> @@ -24,7 +24,6 @@
>  #include <asm/gic-v3-its.h>
>  #include <asm/timer.h>
>  
> -#define NS_5_SECONDS		(5 * 1000 * 1000 * 1000UL)
>  #define QEMU_MMIO_ADDR		0x0a000008
>  
>  static u32 cntfrq;
> @@ -346,17 +345,21 @@ static void loop_test(struct exit_test *test)
>  		}
>  	}
>  
> -	while (ntimes < test->times && total_ns.ns < NS_5_SECONDS) {
> -		isb();
> -		start = read_sysreg(cntvct_el0);
> +	dsb(ish);
> +	isb();
> +	start = read_sysreg(cntvct_el0);
> +	isb();
> +	while (ntimes < test->times) {
>  		test->exec();
> -		isb();
> -		end = read_sysreg(cntvct_el0);
>  
>  		ntimes++;
> -		total_ticks += (end - start);
> -		ticks_to_ns_time(total_ticks, &total_ns);
>  	}
> +	dsb(ish);
> +	isb();
> +	end = read_sysreg(cntvct_el0);
> +
> +	total_ticks = end - start;
> +	ticks_to_ns_time(total_ticks, &total_ns);
>  
>  	if (test->post) {
>  		test->post(ntimes, &total_ticks);
> -- 
> 2.39.3
> 

Re: [kvm-unit-tests PATCH 1/1] arm64: microbench: Improve measurement accuracy of tests

[Andrew Jones]

On Thu, Nov 16, 2023 at 12:53:55PM +0800, heqiong wrote:
> Reducing the impact of the cntvct_el0 register and isb() operation
> on microbenchmark test results to improve testing accuracy and reduce
> latency in test results.
> 
> Signed-off-by: heqiong <heqiong1557@phytium.com.cn>
> ---
>  arm/micro-bench.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/arm/micro-bench.c b/arm/micro-bench.c
> index fbe59d03..22408955 100644
> --- a/arm/micro-bench.c
> +++ b/arm/micro-bench.c
> @@ -24,7 +24,6 @@
>  #include <asm/gic-v3-its.h>
>  #include <asm/timer.h>
>  
> -#define NS_5_SECONDS		(5 * 1000 * 1000 * 1000UL)
>  #define QEMU_MMIO_ADDR		0x0a000008
>  
>  static u32 cntfrq;
> @@ -346,17 +345,21 @@ static void loop_test(struct exit_test *test)
>  		}
>  	}
>  
> -	while (ntimes < test->times && total_ns.ns < NS_5_SECONDS) {
> -		isb();
> -		start = read_sysreg(cntvct_el0);
> +	dsb(ish);
> +	isb();
> +	start = read_sysreg(cntvct_el0);
> +	isb();
> +	while (ntimes < test->times) {
>  		test->exec();
> -		isb();
> -		end = read_sysreg(cntvct_el0);
>  
>  		ntimes++;
> -		total_ticks += (end - start);
> -		ticks_to_ns_time(total_ticks, &total_ns);
>  	}
> +	dsb(ish);
> +	isb();
> +	end = read_sysreg(cntvct_el0);
> +
> +	total_ticks = end - start;
> +	ticks_to_ns_time(total_ticks, &total_ns);
>  
>  	if (test->post) {
>  		test->post(ntimes, &total_ticks);
> -- 
> 2.39.3
>

Merged into https://gitlab.com/kvm-unit-tests/kvm-unit-tests master

Thanks,
drew