Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1443562 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RT PATCH 1/2] timers: wakeup all timer waiters Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-07-14 18:10 +0200
    Re: [RT PATCH 1/2] timers: wakeup all timer waiters Steven Rostedt <rostedt@goodmis.org> - 2016-07-14 18:20 +0200
      Re: [RT PATCH 1/2] timers: wakeup all timer waiters Steven Rostedt <rostedt@goodmis.org> - 2016-07-14 19:20 +0200
        Re: [RT PATCH 1/2] timers: wakeup all timer waiters Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-07-14 20:40 +0200
      Re: [RT PATCH 1/2] timers: wakeup all timer waiters Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-07-14 19:20 +0200

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

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

[toc] | [next] | [standalone]

#1443568

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)
>  {

[toc] | [prev] | [next] | [standalone]

#1443612

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

[toc] | [prev] | [next] | [standalone]

#1443680

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

[toc] | [prev] | [next] | [standalone]

#1443617

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web