Re: [PATCH] nohz: fix race allowing use of stale jiffies when waking

[Eric Dumazet]

Le jeudi 12 janvier 2012 Ã 02:55 -0600, Milton Miller a Ãcrit :
> When waking up from nohz mode, all cpus call tick_do_update_jiffies64
> regardless of tick_do_timer_cpu as it could be no cpu was assigned.
> 
> At the start of the function there is a quick lockless check to
> determine if jiffies is current.  The check uses last_jiffies_update,
> which is used to calculate when to perform the next increment.
> Unfortunately it is updated when how many jiffies to advance the
> clock is calculated, before the call to do_timer which actually
> updates jiffies.  A second cpu waking up could use the (potentially
> very) stale jiffies value during this window.
> 
> This patch changes the check to be against tick_next_period, which
> is updated after the call to do_timer completes.  It compares the
> result of subtraction to zero, but this is safe as ktime_sub returns
> ktime_t which is s64, as signed type.
> 
> I found this race while trying to track down reports of network adapter
> hangs on a large system.  I suspected premature false detection so
> I added logging when the locked region determined a multiple jiffie
> update would be required.  I noticed that it happened frequently when
> tick_do_timer_cpu was NONE (-1), and realized the large update was
> when all cpus were previously in nohz.  I then thought about what
> would happen if multiple cpus woke up near close to each other in
> time and decided the stale jiffies would be used.  (I later found at
> least part of the hung adapter reports were due to faulty detection
> logic that has since changed upstream.)
> 
> Signed-off-by: Milton Miller <miltonm@xxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> --- 
> Patch was generated and tested against 2.6.36; I verified it applies
> with offset -1 line to next-20120111.
> 
> Index: src/kernel/time/tick-sched.c
> ===================================================================
> --- src.orig/kernel/time/tick-sched.c	2011-10-13 17:42:16.000000000 -0500
> +++ src/kernel/time/tick-sched.c	2011-10-13 17:45:31.000000000 -0500
> @@ -52,8 +52,8 @@ static void tick_do_update_jiffies64(kti
>  	/*
>  	 * Do a quick check without holding xtime_lock:
>  	 */
> -	delta = ktime_sub(now, last_jiffies_update);
> -	if (delta.tv64 < tick_period.tv64)
> +	delta = ktime_sub(now, tick_next_period);
> +	if (delta.tv64 < 0)
>  		return;
>  

Given ktime_t on 32bit arches is not an atomic type, I wonder how safe
is this anyway...



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/