[2/2] timer: Reschedule timer if OPAL receives early timer interrupt

Message ID 20180503132126.6830-2-hegdevasant@linux.vnet.ibm.com
State New
Headers show
Series
  • [1/2] timer: Move update_timer_expiry call to separate function
Related show

Commit Message

Vasant Hegde May 3, 2018, 1:21 p.m.
If we receive timer interrupt before timer expiry, we are not rescheduling
timer again. Lets fix this by rescheduling timer.

Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
---
 core/timer.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Benjamin Herrenschmidt May 3, 2018, 9:03 p.m. | #1
On Thu, 2018-05-03 at 18:51 +0530, Vasant Hegde wrote:
> If we receive timer interrupt before timer expiry, we are not rescheduling
> timer again. Lets fix this by rescheduling timer.

Doesn't that mean we'll reschedule every time we call check timer in a
loop ? I'd be keen on only doing that from an actual timer interrupt
and only if we haven't already rescheduled (we could cache the
scheduled time, and clear it on interrupt entry, so we reschedule at
most once per interrupt).

> Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> ---
>  core/timer.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/core/timer.c b/core/timer.c
> index 1c5395178..5890b203c 100644
> --- a/core/timer.c
> +++ b/core/timer.c
> @@ -203,10 +203,15 @@ static void __check_timers(uint64_t now)
>  	for (;;) {
>  		t = list_top(&timer_list, struct timer, link);
>  
> -		/* Top of list not expired ? that's it ... */
> -		if (!t || t->target > now)
> +		if (!t)
>  			break;
>  
> +		/* Top of list not expired ? re-schedule timer */
> +		if (t->target > now) {
> +			update_timer_expiry(t->target);
> +			break;
> +		}
> +
>  		/* Top of list still running, we have to delay handling
>  		 * it. For now just skip until the next poll, when we have
>  		 * SLW interrupts, we'll probably want to trip another one
Vasant Hegde May 4, 2018, 11:07 a.m. | #2
On 05/04/2018 02:33 AM, Benjamin Herrenschmidt wrote:
> On Thu, 2018-05-03 at 18:51 +0530, Vasant Hegde wrote:
>> If we receive timer interrupt before timer expiry, we are not rescheduling
>> timer again. Lets fix this by rescheduling timer.
> 
> Doesn't that mean we'll reschedule every time we call check timer in a
> loop ?

update_timer_expiry() compares with previously scheduled timer value. I thought
that's enough.

> I'd be keen on only doing that from an actual timer interrupt

Ok. I can add that check.

> and only if we haven't already rescheduled (we could cache the
> scheduled time, and clear it on interrupt entry, so we reschedule at
> most once per interrupt).

Sorry. I don't understand why we should cache schedule time.

- I will add check to make sure we reschedule only in interrupt path.
- So we will be rescheduling only when we get early response from SBE.
   And once we reschedule we will not enter this path until we get next 
interrupt from SBE.

  something like below works ?

commit dbf2f5e2fa465433b0ee54162d69179bd1148032
Author: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
Date:   Thu Apr 19 11:32:01 2018 +0530

     timer: Reschedule timer if OPAL receives early timer interrupt

     If we receive timer interrupt before timer expiry, we are not rescheduling
     timer again. Lets fix this by rescheduling timer.

     Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
     Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>

diff --git a/core/timer.c b/core/timer.c
index 1c5395178..d7f9dae00 100644
--- a/core/timer.c
+++ b/core/timer.c
@@ -196,17 +196,23 @@ static void __check_poll_timers(uint64_t now)
  	timer_in_poll = false;
  }

-static void __check_timers(uint64_t now)
+static void __check_timers(bool from_interrupt, uint64_t now)
  {
  	struct timer *t;

  	for (;;) {
  		t = list_top(&timer_list, struct timer, link);

-		/* Top of list not expired ? that's it ... */
-		if (!t || t->target > now)
+		if (!t)
  			break;

+		/* Top of list not expired ? re-schedule timer */
+		if (t->target > now) {
+			if (from_interrupt)
+				update_timer_expiry(t->target);
+			break;
+		}
+
  		/* Top of list still running, we have to delay handling
  		 * it. For now just skip until the next poll, when we have
  		 * SLW interrupts, we'll probably want to trip another one
@@ -252,7 +258,7 @@ void check_timers(bool from_interrupt)
  	lock(&timer_lock);
  	if (!from_interrupt)
  		__check_poll_timers(now);
-	__check_timers(now);
+	__check_timers(from_interrupt, now);
  	unlock(&timer_lock);
  }


-Vasant

Patch

diff --git a/core/timer.c b/core/timer.c
index 1c5395178..5890b203c 100644
--- a/core/timer.c
+++ b/core/timer.c
@@ -203,10 +203,15 @@  static void __check_timers(uint64_t now)
 	for (;;) {
 		t = list_top(&timer_list, struct timer, link);
 
-		/* Top of list not expired ? that's it ... */
-		if (!t || t->target > now)
+		if (!t)
 			break;
 
+		/* Top of list not expired ? re-schedule timer */
+		if (t->target > now) {
+			update_timer_expiry(t->target);
+			break;
+		}
+
 		/* Top of list still running, we have to delay handling
 		 * it. For now just skip until the next poll, when we have
 		 * SLW interrupts, we'll probably want to trip another one