[RT PATCH 1/2] timers: wakeup all timer waiters

[RT PATCH 1/2] timers: wakeup all timer waiters

[Sebastian Andrzej Siewior]

The base lock is dropped during the invocation if the timer. That means
it is possible that we have one waiter while timer1 is running and once
this one finished, we get another waiter while timer2 is running. Since
we wake up only one waiter it is possible that we miss the other one.
This will probably heal itself over time because most of the time we
complete timers without an active wake up.
To avoid the scenario where we don't wake up all waiters at once,
wake_up_all() is used.

Cc: stable-rt@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/time/timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 5f9d3599ef0a..b3c3d3a6216f 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1051,7 +1051,7 @@ static void wait_for_running_timer(struct timer_list *timer)
 		   base->running_timer != timer);
 }
 
-# define wakeup_timer_waiters(b)	wake_up(&(b)->wait_for_running_timer)
+# define wakeup_timer_waiters(b)	wake_up_all(&(b)->wait_for_running_timer)
 #else
 static inline void wait_for_running_timer(struct timer_list *timer)
 {
-- 
2.8.1

[RT PATCH 2/2] timers: wakeup all timer waiters without holding the base lock

[Sebastian Andrzej Siewior]

There should be no need to hold the base lock during the wakeup. There
should be no boosting involved, the wakeup list has its own lock so it
should be safe to do this without the lock.

Cc: stable-rt@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/time/timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index b3c3d3a6216f..716ef84a5d87 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1313,8 +1313,8 @@ static inline void __run_timers(struct tvec_base *base)
 			}
 		}
 	}
-	wakeup_timer_waiters(base);
 	spin_unlock_irq(&base->lock);
+	wakeup_timer_waiters(base);
 }
 
 #ifdef CONFIG_NO_HZ_COMMON
-- 
2.8.1

Re: [RT PATCH 2/2] timers: wakeup all timer waiters without holding the base lock

[Steven Rostedt]

On Thu, 14 Jul 2016 18:05:04 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> There should be no need to hold the base lock during the wakeup. There
> should be no boosting involved, the wakeup list has its own lock so it
> should be safe to do this without the lock.
> 
> Cc: stable-rt@vger.kernel.org

Nothing against this patch, but as you marked it for stable, can you
add to the change log what issue you had that caused you to make this
change?

-- Steve

> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  kernel/time/timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index b3c3d3a6216f..716ef84a5d87 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1313,8 +1313,8 @@ static inline void __run_timers(struct tvec_base *base)
>  			}
>  		}
>  	}
> -	wakeup_timer_waiters(base);
>  	spin_unlock_irq(&base->lock);
> +	wakeup_timer_waiters(base);
>  }
>  
>  #ifdef CONFIG_NO_HZ_COMMON

Re: [RT PATCH 1/2] timers: wakeup all timer waiters

[Steven Rostedt]

On Thu, 14 Jul 2016 18:05:03 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> The base lock is dropped during the invocation if the timer. That means
> it is possible that we have one waiter while timer1 is running and once
> this one finished, we get another waiter while timer2 is running. Since
> we wake up only one waiter it is possible that we miss the other one.
> This will probably heal itself over time because most of the time we
> complete timers without an active wake up.
> To avoid the scenario where we don't wake up all waiters at once,
> wake_up_all() is used.
> 
> Cc: stable-rt@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  kernel/time/timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index 5f9d3599ef0a..b3c3d3a6216f 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1051,7 +1051,7 @@ static void wait_for_running_timer(struct timer_list *timer)
>  		   base->running_timer != timer);
>  }
>  
> -# define wakeup_timer_waiters(b)	wake_up(&(b)->wait_for_running_timer)
> +# define wakeup_timer_waiters(b)	wake_up_all(&(b)->wait_for_running_timer)

OK, I just received this patch (way after patch 2)

I'm assuming that patch two was done such that you don't do a
"wake_up_all" under a spinlock.

-- Steve

>  #else
>  static inline void wait_for_running_timer(struct timer_list *timer)
>  {

Re: [RT PATCH 1/2] timers: wakeup all timer waiters

[Sebastian Andrzej Siewior]

On 07/14/2016 06:13 PM, Steven Rostedt wrote:
>>  
>> -# define wakeup_timer_waiters(b)	wake_up(&(b)->wait_for_running_timer)
>> +# define wakeup_timer_waiters(b)	wake_up_all(&(b)->wait_for_running_timer)
> 
> OK, I just received this patch (way after patch 2)
> 
> I'm assuming that patch two was done such that you don't do a
> "wake_up_all" under a spinlock.

No. I pulled in new timer code in and had to redo this part of RT.

While doing so I noticed that we drop the base lock during timer
invocations and so it could be possible that we have two invocations
of del_timer_sync() on a timer on the same "base" (one after the
other). This is patch #1.

After that I saw that we do the wake up under the base lock but there
is no reason for it. So here is patch #2.

Patch #1 is something that could happen in theory and I did not run in
any problem.

Sebastian

Re: [RT PATCH 1/2] timers: wakeup all timer waiters

[Steven Rostedt]

On Thu, 14 Jul 2016 19:14:58 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> On 07/14/2016 06:13 PM, Steven Rostedt wrote:
> >>  
> >> -# define wakeup_timer_waiters(b)	wake_up(&(b)->wait_for_running_timer)
> >> +# define wakeup_timer_waiters(b)	wake_up_all(&(b)->wait_for_running_timer)  
> > 
> > OK, I just received this patch (way after patch 2)
> > 
> > I'm assuming that patch two was done such that you don't do a
> > "wake_up_all" under a spinlock.  
> 
> No. I pulled in new timer code in and had to redo this part of RT.
> 
> While doing so I noticed that we drop the base lock during timer
> invocations and so it could be possible that we have two invocations
> of del_timer_sync() on a timer on the same "base" (one after the
> other). This is patch #1.
> 
> After that I saw that we do the wake up under the base lock but there
> is no reason for it. So here is patch #2.
> 
> Patch #1 is something that could happen in theory and I did not run in
> any problem.
>

OK, so patch 2 was just discovered by reviewing code?

-- Steve

Re: [RT PATCH 2/2] timers: wakeup all timer waiters without holding the base lock

[Sebastian Andrzej Siewior]

On 07/14/2016 06:09 PM, Steven Rostedt wrote:
> On Thu, 14 Jul 2016 18:05:04 +0200
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
>> There should be no need to hold the base lock during the wakeup. There
>> should be no boosting involved, the wakeup list has its own lock so it
>> should be safe to do this without the lock.
>>
>> Cc: stable-rt@vger.kernel.org
> 
> Nothing against this patch, but as you marked it for stable, can you
> add to the change log what issue you had that caused you to make this
> change?

#1 was noticed while looking at the code, it *might* happen.
#2 Is not strictly required for back porting.
I just don't see a reason for holding the lock, that is all.

> -- Steve

Sebastian

Re: [RT PATCH 2/2] timers: wakeup all timer waiters without holding the base lock

[Steven Rostedt]

On Thu, 14 Jul 2016 19:19:47 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> On 07/14/2016 06:09 PM, Steven Rostedt wrote:
> > On Thu, 14 Jul 2016 18:05:04 +0200
> > Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> >   
> >> There should be no need to hold the base lock during the wakeup. There
> >> should be no boosting involved, the wakeup list has its own lock so it
> >> should be safe to do this without the lock.
> >>
> >> Cc: stable-rt@vger.kernel.org  
> > 
> > Nothing against this patch, but as you marked it for stable, can you
> > add to the change log what issue you had that caused you to make this
> > change?  
> 
> #1 was noticed while looking at the code, it *might* happen.
> #2 Is not strictly required for back porting.
> I just don't see a reason for holding the lock, that is all.
> 

I have no problem backporting them. I just thought that the change log
might have been missing some useful information.

-- Steve

Re: [RT PATCH 1/2] timers: wakeup all timer waiters

[Sebastian Andrzej Siewior]

On 07/14/2016 07:19 PM, Steven Rostedt wrote:
>> While doing so I noticed that we drop the base lock during timer
>> invocations and so it could be possible that we have two invocations
>> of del_timer_sync() on a timer on the same "base" (one after the
>> other). This is patch #1.
>>
>> After that I saw that we do the wake up under the base lock but there
>> is no reason for it. So here is patch #2.
>>
>> Patch #1 is something that could happen in theory and I did not run in
>> any problem.
>>
> 
> OK, so patch 2 was just discovered by reviewing code?

both, not just patch 2.

> 
> -- Steve

Sebastian