All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH kvm-unit-tests] arm64: timer: Speed up gic-timer-state check
@ 2020-02-11 12:35 ` Andrew Jones
  0 siblings, 0 replies; 6+ messages in thread
From: Andrew Jones @ 2020-02-11 12:35 UTC (permalink / raw)
  To: kvm, kvmarm; +Cc: alexandru.elisei, yuzenghui

Let's bail out of the wait loop if we see the expected state
to save about seven seconds of run time. Make sure we wait a
bit before reading the registers, though, to somewhat mitigate
the chance of the expected state being stale.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arm/timer.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index f5cf775ce50f..c2262c112c09 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -183,7 +183,8 @@ static bool timer_pending(struct timer_info *info)
 		(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS);
 }
 
-static enum gic_state gic_timer_state(struct timer_info *info)
+static bool gic_timer_check_state(struct timer_info *info,
+				  enum gic_state expected_state)
 {
 	enum gic_state state = GIC_STATE_INACTIVE;
 	int i;
@@ -191,6 +192,7 @@ static enum gic_state gic_timer_state(struct timer_info *info)
 
 	/* Wait for up to 1s for the GIC to sample the interrupt. */
 	for (i = 0; i < 10; i++) {
+		mdelay(100);
 		pending = readl(gic_ispendr) & (1 << PPI(info->irq));
 		active = readl(gic_isactiver) & (1 << PPI(info->irq));
 		if (!active && !pending)
@@ -201,10 +203,11 @@ static enum gic_state gic_timer_state(struct timer_info *info)
 			state = GIC_STATE_ACTIVE;
 		if (active && pending)
 			state = GIC_STATE_ACTIVE_PENDING;
-		mdelay(100);
+		if (state == expected_state)
+			return true;
 	}
 
-	return state;
+	return false;
 }
 
 static bool test_cval_10msec(struct timer_info *info)
@@ -253,11 +256,11 @@ static void test_timer(struct timer_info *info)
 	/* Enable the timer, but schedule it for much later */
 	info->write_cval(later);
 	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
-	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
+	report(!timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
 			"not pending before");
 
 	info->write_cval(now - 1);
-	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_PENDING,
+	report(timer_pending(info) && gic_timer_check_state(info, GIC_STATE_PENDING),
 			"interrupt signal pending");
 
 	/* Disable the timer again and prepare to take interrupts */
@@ -265,12 +268,12 @@ static void test_timer(struct timer_info *info)
 	info->irq_received = false;
 	set_timer_irq_enabled(info, true);
 	report(!info->irq_received, "no interrupt when timer is disabled");
-	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
+	report(!timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
 			"interrupt signal no longer pending");
 
 	info->write_cval(now - 1);
 	info->write_ctl(ARCH_TIMER_CTL_ENABLE | ARCH_TIMER_CTL_IMASK);
-	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
+	report(timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
 			"interrupt signal not pending");
 
 	report(test_cval_10msec(info), "latency within 10 ms");
-- 
2.21.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH kvm-unit-tests] arm64: timer: Speed up gic-timer-state check
@ 2020-02-11 12:35 ` Andrew Jones
  0 siblings, 0 replies; 6+ messages in thread
From: Andrew Jones @ 2020-02-11 12:35 UTC (permalink / raw)
  To: kvm, kvmarm

Let's bail out of the wait loop if we see the expected state
to save about seven seconds of run time. Make sure we wait a
bit before reading the registers, though, to somewhat mitigate
the chance of the expected state being stale.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arm/timer.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index f5cf775ce50f..c2262c112c09 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -183,7 +183,8 @@ static bool timer_pending(struct timer_info *info)
 		(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS);
 }
 
-static enum gic_state gic_timer_state(struct timer_info *info)
+static bool gic_timer_check_state(struct timer_info *info,
+				  enum gic_state expected_state)
 {
 	enum gic_state state = GIC_STATE_INACTIVE;
 	int i;
@@ -191,6 +192,7 @@ static enum gic_state gic_timer_state(struct timer_info *info)
 
 	/* Wait for up to 1s for the GIC to sample the interrupt. */
 	for (i = 0; i < 10; i++) {
+		mdelay(100);
 		pending = readl(gic_ispendr) & (1 << PPI(info->irq));
 		active = readl(gic_isactiver) & (1 << PPI(info->irq));
 		if (!active && !pending)
@@ -201,10 +203,11 @@ static enum gic_state gic_timer_state(struct timer_info *info)
 			state = GIC_STATE_ACTIVE;
 		if (active && pending)
 			state = GIC_STATE_ACTIVE_PENDING;
-		mdelay(100);
+		if (state == expected_state)
+			return true;
 	}
 
-	return state;
+	return false;
 }
 
 static bool test_cval_10msec(struct timer_info *info)
@@ -253,11 +256,11 @@ static void test_timer(struct timer_info *info)
 	/* Enable the timer, but schedule it for much later */
 	info->write_cval(later);
 	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
-	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
+	report(!timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
 			"not pending before");
 
 	info->write_cval(now - 1);
-	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_PENDING,
+	report(timer_pending(info) && gic_timer_check_state(info, GIC_STATE_PENDING),
 			"interrupt signal pending");
 
 	/* Disable the timer again and prepare to take interrupts */
@@ -265,12 +268,12 @@ static void test_timer(struct timer_info *info)
 	info->irq_received = false;
 	set_timer_irq_enabled(info, true);
 	report(!info->irq_received, "no interrupt when timer is disabled");
-	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
+	report(!timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
 			"interrupt signal no longer pending");
 
 	info->write_cval(now - 1);
 	info->write_ctl(ARCH_TIMER_CTL_ENABLE | ARCH_TIMER_CTL_IMASK);
-	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
+	report(timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
 			"interrupt signal not pending");
 
 	report(test_cval_10msec(info), "latency within 10 ms");
-- 
2.21.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH kvm-unit-tests] arm64: timer: Speed up gic-timer-state check
  2020-02-11 12:35 ` Andrew Jones
@ 2020-02-11 12:49   ` Alexandru Elisei
  -1 siblings, 0 replies; 6+ messages in thread
From: Alexandru Elisei @ 2020-02-11 12:49 UTC (permalink / raw)
  To: Andrew Jones, kvm, kvmarm; +Cc: yuzenghui

Hi,

On 2/11/20 12:35 PM, Andrew Jones wrote:
> Let's bail out of the wait loop if we see the expected state
> to save about seven seconds of run time. Make sure we wait a
> bit before reading the registers, though, to somewhat mitigate
> the chance of the expected state being stale.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arm/timer.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/arm/timer.c b/arm/timer.c
> index f5cf775ce50f..c2262c112c09 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -183,7 +183,8 @@ static bool timer_pending(struct timer_info *info)
>  		(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS);
>  }
>  
> -static enum gic_state gic_timer_state(struct timer_info *info)
> +static bool gic_timer_check_state(struct timer_info *info,
> +				  enum gic_state expected_state)
>  {
>  	enum gic_state state = GIC_STATE_INACTIVE;
>  	int i;
> @@ -191,6 +192,7 @@ static enum gic_state gic_timer_state(struct timer_info *info)
>  
>  	/* Wait for up to 1s for the GIC to sample the interrupt. */
>  	for (i = 0; i < 10; i++) {
> +		mdelay(100);
>  		pending = readl(gic_ispendr) & (1 << PPI(info->irq));
>  		active = readl(gic_isactiver) & (1 << PPI(info->irq));
>  		if (!active && !pending)
> @@ -201,10 +203,11 @@ static enum gic_state gic_timer_state(struct timer_info *info)
>  			state = GIC_STATE_ACTIVE;
>  		if (active && pending)
>  			state = GIC_STATE_ACTIVE_PENDING;
> -		mdelay(100);
> +		if (state == expected_state)
> +			return true;
>  	}
>  
> -	return state;
> +	return false;
>  }

The first version I wrote looked similar. However I decided to wait the entire 1s
because I could imagine a situation where the interrupt was pending, but if I were
to wait a bit longer, it would have become active and pending, which is not what
we would want. Same thing with inactive.

How about after the state matches what we expect, we wait for an extra 100ms and
check that the state hasn't changed?

Also, you probably also want to lower the timeout in arm/unittests.cfg.

Thanks,
Alex
>  
>  static bool test_cval_10msec(struct timer_info *info)
> @@ -253,11 +256,11 @@ static void test_timer(struct timer_info *info)
>  	/* Enable the timer, but schedule it for much later */
>  	info->write_cval(later);
>  	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
> -	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
> +	report(!timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
>  			"not pending before");
>  
>  	info->write_cval(now - 1);
> -	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_PENDING,
> +	report(timer_pending(info) && gic_timer_check_state(info, GIC_STATE_PENDING),
>  			"interrupt signal pending");
>  
>  	/* Disable the timer again and prepare to take interrupts */
> @@ -265,12 +268,12 @@ static void test_timer(struct timer_info *info)
>  	info->irq_received = false;
>  	set_timer_irq_enabled(info, true);
>  	report(!info->irq_received, "no interrupt when timer is disabled");
> -	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
> +	report(!timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
>  			"interrupt signal no longer pending");
>  
>  	info->write_cval(now - 1);
>  	info->write_ctl(ARCH_TIMER_CTL_ENABLE | ARCH_TIMER_CTL_IMASK);
> -	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
> +	report(timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
>  			"interrupt signal not pending");
>  
>  	report(test_cval_10msec(info), "latency within 10 ms");

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH kvm-unit-tests] arm64: timer: Speed up gic-timer-state check
@ 2020-02-11 12:49   ` Alexandru Elisei
  0 siblings, 0 replies; 6+ messages in thread
From: Alexandru Elisei @ 2020-02-11 12:49 UTC (permalink / raw)
  To: Andrew Jones, kvm, kvmarm

Hi,

On 2/11/20 12:35 PM, Andrew Jones wrote:
> Let's bail out of the wait loop if we see the expected state
> to save about seven seconds of run time. Make sure we wait a
> bit before reading the registers, though, to somewhat mitigate
> the chance of the expected state being stale.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arm/timer.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/arm/timer.c b/arm/timer.c
> index f5cf775ce50f..c2262c112c09 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -183,7 +183,8 @@ static bool timer_pending(struct timer_info *info)
>  		(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS);
>  }
>  
> -static enum gic_state gic_timer_state(struct timer_info *info)
> +static bool gic_timer_check_state(struct timer_info *info,
> +				  enum gic_state expected_state)
>  {
>  	enum gic_state state = GIC_STATE_INACTIVE;
>  	int i;
> @@ -191,6 +192,7 @@ static enum gic_state gic_timer_state(struct timer_info *info)
>  
>  	/* Wait for up to 1s for the GIC to sample the interrupt. */
>  	for (i = 0; i < 10; i++) {
> +		mdelay(100);
>  		pending = readl(gic_ispendr) & (1 << PPI(info->irq));
>  		active = readl(gic_isactiver) & (1 << PPI(info->irq));
>  		if (!active && !pending)
> @@ -201,10 +203,11 @@ static enum gic_state gic_timer_state(struct timer_info *info)
>  			state = GIC_STATE_ACTIVE;
>  		if (active && pending)
>  			state = GIC_STATE_ACTIVE_PENDING;
> -		mdelay(100);
> +		if (state == expected_state)
> +			return true;
>  	}
>  
> -	return state;
> +	return false;
>  }

The first version I wrote looked similar. However I decided to wait the entire 1s
because I could imagine a situation where the interrupt was pending, but if I were
to wait a bit longer, it would have become active and pending, which is not what
we would want. Same thing with inactive.

How about after the state matches what we expect, we wait for an extra 100ms and
check that the state hasn't changed?

Also, you probably also want to lower the timeout in arm/unittests.cfg.

Thanks,
Alex
>  
>  static bool test_cval_10msec(struct timer_info *info)
> @@ -253,11 +256,11 @@ static void test_timer(struct timer_info *info)
>  	/* Enable the timer, but schedule it for much later */
>  	info->write_cval(later);
>  	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
> -	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
> +	report(!timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
>  			"not pending before");
>  
>  	info->write_cval(now - 1);
> -	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_PENDING,
> +	report(timer_pending(info) && gic_timer_check_state(info, GIC_STATE_PENDING),
>  			"interrupt signal pending");
>  
>  	/* Disable the timer again and prepare to take interrupts */
> @@ -265,12 +268,12 @@ static void test_timer(struct timer_info *info)
>  	info->irq_received = false;
>  	set_timer_irq_enabled(info, true);
>  	report(!info->irq_received, "no interrupt when timer is disabled");
> -	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
> +	report(!timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
>  			"interrupt signal no longer pending");
>  
>  	info->write_cval(now - 1);
>  	info->write_ctl(ARCH_TIMER_CTL_ENABLE | ARCH_TIMER_CTL_IMASK);
> -	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
> +	report(timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
>  			"interrupt signal not pending");
>  
>  	report(test_cval_10msec(info), "latency within 10 ms");
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH kvm-unit-tests] arm64: timer: Speed up gic-timer-state check
  2020-02-11 12:49   ` Alexandru Elisei
@ 2020-02-11 12:54     ` Andrew Jones
  -1 siblings, 0 replies; 6+ messages in thread
From: Andrew Jones @ 2020-02-11 12:54 UTC (permalink / raw)
  To: Alexandru Elisei; +Cc: kvm, kvmarm, yuzenghui

On Tue, Feb 11, 2020 at 12:49:23PM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On 2/11/20 12:35 PM, Andrew Jones wrote:
> > Let's bail out of the wait loop if we see the expected state
> > to save about seven seconds of run time. Make sure we wait a
> > bit before reading the registers, though, to somewhat mitigate
> > the chance of the expected state being stale.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  arm/timer.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/arm/timer.c b/arm/timer.c
> > index f5cf775ce50f..c2262c112c09 100644
> > --- a/arm/timer.c
> > +++ b/arm/timer.c
> > @@ -183,7 +183,8 @@ static bool timer_pending(struct timer_info *info)
> >  		(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS);
> >  }
> >  
> > -static enum gic_state gic_timer_state(struct timer_info *info)
> > +static bool gic_timer_check_state(struct timer_info *info,
> > +				  enum gic_state expected_state)
> >  {
> >  	enum gic_state state = GIC_STATE_INACTIVE;
> >  	int i;
> > @@ -191,6 +192,7 @@ static enum gic_state gic_timer_state(struct timer_info *info)
> >  
> >  	/* Wait for up to 1s for the GIC to sample the interrupt. */
> >  	for (i = 0; i < 10; i++) {
> > +		mdelay(100);
> >  		pending = readl(gic_ispendr) & (1 << PPI(info->irq));
> >  		active = readl(gic_isactiver) & (1 << PPI(info->irq));
> >  		if (!active && !pending)
> > @@ -201,10 +203,11 @@ static enum gic_state gic_timer_state(struct timer_info *info)
> >  			state = GIC_STATE_ACTIVE;
> >  		if (active && pending)
> >  			state = GIC_STATE_ACTIVE_PENDING;
> > -		mdelay(100);
> > +		if (state == expected_state)
> > +			return true;
> >  	}
> >  
> > -	return state;
> > +	return false;
> >  }
> 
> The first version I wrote looked similar. However I decided to wait the entire 1s
> because I could imagine a situation where the interrupt was pending, but if I were
> to wait a bit longer, it would have become active and pending, which is not what
> we would want. Same thing with inactive.
> 
> How about after the state matches what we expect, we wait for an extra 100ms and
> check that the state hasn't changed?

That sounds good. I'll send a v2 with that.

> 
> Also, you probably also want to lower the timeout in arm/unittests.cfg.

The timeout is fine, because if we need to loop the worst-case scenario
amount of time then we want the timeout high enough. It's not really a
goal to make the timeout just barely long enough to cover the test anyway.
If the timeout is too long, and we need it to fire, then it just means
test runners will have to wait longer for it.

Thanks,
drew

> 
> Thanks,
> Alex
> >  
> >  static bool test_cval_10msec(struct timer_info *info)
> > @@ -253,11 +256,11 @@ static void test_timer(struct timer_info *info)
> >  	/* Enable the timer, but schedule it for much later */
> >  	info->write_cval(later);
> >  	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
> > -	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
> > +	report(!timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
> >  			"not pending before");
> >  
> >  	info->write_cval(now - 1);
> > -	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_PENDING,
> > +	report(timer_pending(info) && gic_timer_check_state(info, GIC_STATE_PENDING),
> >  			"interrupt signal pending");
> >  
> >  	/* Disable the timer again and prepare to take interrupts */
> > @@ -265,12 +268,12 @@ static void test_timer(struct timer_info *info)
> >  	info->irq_received = false;
> >  	set_timer_irq_enabled(info, true);
> >  	report(!info->irq_received, "no interrupt when timer is disabled");
> > -	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
> > +	report(!timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
> >  			"interrupt signal no longer pending");
> >  
> >  	info->write_cval(now - 1);
> >  	info->write_ctl(ARCH_TIMER_CTL_ENABLE | ARCH_TIMER_CTL_IMASK);
> > -	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
> > +	report(timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
> >  			"interrupt signal not pending");
> >  
> >  	report(test_cval_10msec(info), "latency within 10 ms");
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH kvm-unit-tests] arm64: timer: Speed up gic-timer-state check
@ 2020-02-11 12:54     ` Andrew Jones
  0 siblings, 0 replies; 6+ messages in thread
From: Andrew Jones @ 2020-02-11 12:54 UTC (permalink / raw)
  To: Alexandru Elisei; +Cc: kvmarm, kvm

On Tue, Feb 11, 2020 at 12:49:23PM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On 2/11/20 12:35 PM, Andrew Jones wrote:
> > Let's bail out of the wait loop if we see the expected state
> > to save about seven seconds of run time. Make sure we wait a
> > bit before reading the registers, though, to somewhat mitigate
> > the chance of the expected state being stale.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  arm/timer.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/arm/timer.c b/arm/timer.c
> > index f5cf775ce50f..c2262c112c09 100644
> > --- a/arm/timer.c
> > +++ b/arm/timer.c
> > @@ -183,7 +183,8 @@ static bool timer_pending(struct timer_info *info)
> >  		(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS);
> >  }
> >  
> > -static enum gic_state gic_timer_state(struct timer_info *info)
> > +static bool gic_timer_check_state(struct timer_info *info,
> > +				  enum gic_state expected_state)
> >  {
> >  	enum gic_state state = GIC_STATE_INACTIVE;
> >  	int i;
> > @@ -191,6 +192,7 @@ static enum gic_state gic_timer_state(struct timer_info *info)
> >  
> >  	/* Wait for up to 1s for the GIC to sample the interrupt. */
> >  	for (i = 0; i < 10; i++) {
> > +		mdelay(100);
> >  		pending = readl(gic_ispendr) & (1 << PPI(info->irq));
> >  		active = readl(gic_isactiver) & (1 << PPI(info->irq));
> >  		if (!active && !pending)
> > @@ -201,10 +203,11 @@ static enum gic_state gic_timer_state(struct timer_info *info)
> >  			state = GIC_STATE_ACTIVE;
> >  		if (active && pending)
> >  			state = GIC_STATE_ACTIVE_PENDING;
> > -		mdelay(100);
> > +		if (state == expected_state)
> > +			return true;
> >  	}
> >  
> > -	return state;
> > +	return false;
> >  }
> 
> The first version I wrote looked similar. However I decided to wait the entire 1s
> because I could imagine a situation where the interrupt was pending, but if I were
> to wait a bit longer, it would have become active and pending, which is not what
> we would want. Same thing with inactive.
> 
> How about after the state matches what we expect, we wait for an extra 100ms and
> check that the state hasn't changed?

That sounds good. I'll send a v2 with that.

> 
> Also, you probably also want to lower the timeout in arm/unittests.cfg.

The timeout is fine, because if we need to loop the worst-case scenario
amount of time then we want the timeout high enough. It's not really a
goal to make the timeout just barely long enough to cover the test anyway.
If the timeout is too long, and we need it to fire, then it just means
test runners will have to wait longer for it.

Thanks,
drew

> 
> Thanks,
> Alex
> >  
> >  static bool test_cval_10msec(struct timer_info *info)
> > @@ -253,11 +256,11 @@ static void test_timer(struct timer_info *info)
> >  	/* Enable the timer, but schedule it for much later */
> >  	info->write_cval(later);
> >  	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
> > -	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
> > +	report(!timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
> >  			"not pending before");
> >  
> >  	info->write_cval(now - 1);
> > -	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_PENDING,
> > +	report(timer_pending(info) && gic_timer_check_state(info, GIC_STATE_PENDING),
> >  			"interrupt signal pending");
> >  
> >  	/* Disable the timer again and prepare to take interrupts */
> > @@ -265,12 +268,12 @@ static void test_timer(struct timer_info *info)
> >  	info->irq_received = false;
> >  	set_timer_irq_enabled(info, true);
> >  	report(!info->irq_received, "no interrupt when timer is disabled");
> > -	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
> > +	report(!timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
> >  			"interrupt signal no longer pending");
> >  
> >  	info->write_cval(now - 1);
> >  	info->write_ctl(ARCH_TIMER_CTL_ENABLE | ARCH_TIMER_CTL_IMASK);
> > -	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
> > +	report(timer_pending(info) && gic_timer_check_state(info, GIC_STATE_INACTIVE),
> >  			"interrupt signal not pending");
> >  
> >  	report(test_cval_10msec(info), "latency within 10 ms");
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-11 12:54 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-11 12:35 [PATCH kvm-unit-tests] arm64: timer: Speed up gic-timer-state check Andrew Jones
2020-02-11 12:35 ` Andrew Jones
2020-02-11 12:49 ` Alexandru Elisei
2020-02-11 12:49   ` Alexandru Elisei
2020-02-11 12:54   ` Andrew Jones
2020-02-11 12:54     ` Andrew Jones

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.