• Thomas Gleixner's avatar
    tick/sched: Do not mess with an enqueued hrtimer · 1f71addd
    Thomas Gleixner authored
    Kaike reported that in tests rdma hrtimers occasionaly stopped working. He
    did great debugging, which provided enough context to decode the problem.
    
    CPU 3			     	      	     CPU 2
    
    idle
    start sched_timer expires = 712171000000
     queue->next = sched_timer
    					    start rdmavt timer. expires = 712172915662
    					    lock(baseof(CPU3))
    tick_nohz_stop_tick()
    tick = 716767000000			    timerqueue_add(tmr)
    
    hrtimer_set_expires(sched_timer, tick);
      sched_timer->expires = 716767000000  <---- FAIL
    					     if (tmr->expires < queue->next->expires)
    hrtimer_start(sched_timer)		          queue->next = tmr;
    lock(baseof(CPU3))
    					     unlock(baseof(CPU3))
    timerqueue_remove()
    timerqueue_add()
    
    ts->sched_timer is queued and queue->next is pointing to it, but then
    ts->sched_timer.expires is modified.
    
    This not only corrupts the ordering of the timerqueue RB tree, it also
    makes CPU2 see the new expiry time of timerqueue->next->expires when
    checking whether timerqueue->next needs to be updated. So CPU2 sees that
    the rdma timer is earlier than timerqueue->next and sets the rdma timer as
    new next.
    
    Depending on whether it had also seen the new time at RB tree enqueue, it
    might have queued the rdma timer at the wrong place and then after removing
    the sched_timer the RB tree is completely hosed.
    
    The problem was introduced with a commit which tried to solve inconsistency
    between the hrtimer in the tick_sched data and the underlying hardware
    clockevent. It split out hrtimer_set_expires() to store the new tick time
    in both the NOHZ and the NOHZ + HIGHRES case, but missed the fact that in
    the NOHZ + HIGHRES case the hrtimer might still be queued.
    
    Use hrtimer_start(timer, tick...) for the NOHZ + HIGHRES case which sets
    timer->expires after canceling the timer and move the hrtimer_set_expires()
    invocation into the NOHZ only code path which is not affected as it merily
    uses the hrtimer as next event storage so code pathes can be shared with
    the NOHZ + HIGHRES case.
    
    Fixes: d4af6d93 ("nohz: Fix spurious warning when hrtimer and clockevent get out of sync")
    Reported-by: default avatar"Wan Kaike" <kaike.wan@intel.com>
    Signed-off-by: default avatarThomas Gleixner <tglx@linutronix.de>
    Acked-by: default avatarFrederic Weisbecker <frederic@kernel.org>
    Cc: "Marciniszyn Mike" <mike.marciniszyn@intel.com>
    Cc: Anna-Maria Gleixner <anna-maria@linutronix.de>
    Cc: linux-rdma@vger.kernel.org
    Cc: "Dalessandro Dennis" <dennis.dalessandro@intel.com>
    Cc: "Fleck John" <john.fleck@intel.com>
    Cc: stable@vger.kernel.org
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: Frederic Weisbecker <fweisbec@gmail.com>
    Cc: "Weiny Ira" <ira.weiny@intel.com>
    Cc: "linux-rdma@vger.kernel.org"
    Link: https://lkml.kernel.org/r/alpine.DEB.2.21.1804241637390.1679@nanos.tec.linutronix.de
    Link: https://lkml.kernel.org/r/alpine.DEB.2.21.1804242119210.1597@nanos.tec.linutronix.de
    
    1f71addd
tick-sched.c 34.1 KB