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

Groups > linux.kernel > #1698088

Re: [Question]: try to fix contention between expire_timers and try_to_del_timer_sync

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

On Thu, 27 Jul 2017, Will Deacon wrote:
> On Thu, Jul 27, 2017 at 09:29:20AM +0800, qiaozhou wrote:
> > On 2017年07月26日 22:16, Thomas Gleixner wrote:
> > >--- a/kernel/time/timer.c
> > >+++ b/kernel/time/timer.c
> > >@@ -1301,10 +1301,12 @@ static void expire_timers(struct timer_b
> > >  		if (timer->flags & TIMER_IRQSAFE) {
> > >  			raw_spin_unlock(&base->lock);
> > >  			call_timer_fn(timer, fn, data);
> > >+			base->running_timer = NULL;
> > >  			raw_spin_lock(&base->lock);
> > >  		} else {
> > >  			raw_spin_unlock_irq(&base->lock);
> > >  			call_timer_fn(timer, fn, data);
> > >+			base->running_timer = NULL;
> > >  			raw_spin_lock_irq(&base->lock);
> > >  		}
> > >  	}
> > It should work for this particular issue and I'll test it. Previously I
> > thought it was unsafe to touch base->running_timer without holding lock.
> 
> I think it works out in practice because base->lock and base->running_timer
> share a cacheline, so end up being ordered correctly. We should probably be
> using READ_ONCE/WRITE_ONCE for accessing the running_time field though.
> 
> One thing I don't get though, is why try_to_del_timer_sync needs to check
> base->running_timer at all. Given that it holds the base->lock, can't it
> be the person that sets it to NULL?

No. The timer callback code does:

    base->running_timer = timer;
    spin_unlock(base->lock);
    fn(timer);
    spin_lock(base->lock);
    base->running_timer = NULL;

So for del_timer_sync() the only way to figure out whether the timer
callback is running is to check base->running_timer. We cannot store state
in the timer itself because we cannot clear that state when the callback
return as the timer might have been freed in the callback. Yes, that's
nasty, but reality.

Thanks,

	tglx

    

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: [Question]: try to fix contention between expire_timers and  try_to_del_timer_sync Thomas Gleixner <tglx@linutronix.de> - 2017-07-26 16:20 +0200
  Re: [Question]: try to fix contention between expire_timers and  try_to_del_timer_sync qiaozhou <qiaozhou@asrmicro.com> - 2017-07-27 03:40 +0200
    Re: [Question]: try to fix contention between expire_timers and  try_to_del_timer_sync Thomas Gleixner <tglx@linutronix.de> - 2017-07-27 17:20 +0200
    Re: [Question]: try to fix contention between expire_timers and  try_to_del_timer_sync Will Deacon <will.deacon@arm.com> - 2017-07-27 17:20 +0200
    Re: [Question]: try to fix contention between expire_timers and  try_to_del_timer_sync Vikram Mulukutla <markivx@codeaurora.org> - 2017-07-28 03:20 +0200
      Re: [Question]: try to fix contention between expire_timers and  try_to_del_timer_sync Will Deacon <will.deacon@arm.com> - 2017-07-28 11:30 +0200
        Re: [Question]: try to fix contention between expire_timers and  try_to_del_timer_sync Vikram Mulukutla <markivx@codeaurora.org> - 2017-07-28 21:10 +0200
          Re: [Question]: try to fix contention between expire_timers and  try_to_del_timer_sync qiaozhou <qiaozhou@asrmicro.com> - 2017-07-31 13:30 +0200
      Re: [Question]: try to fix contention between expire_timers and  try_to_del_timer_sync Peter Zijlstra <peterz@infradead.org> - 2017-07-28 11:30 +0200
        Re: [Question]: try to fix contention between expire_timers and  try_to_del_timer_sync Vikram Mulukutla <markivx@codeaurora.org> - 2017-07-28 21:20 +0200

csiph-web