Re: 2.6.12-rc2-mm3

[Andrew Morton]

Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> Benjamin Herrenschmidt wrote:
> >
> > I'll have a look at the timer patch next week, they might have some
> > subtle race caused by a lack of memory barrier. I've had to debug some
> > of those in early timer code, and those are really nasty, they usually
> > only trigger under some subtle conditions, like ... heavy networking.
> 
> Before this timer patch del_timer(pending_timer) worked as
> a memory barrier for the caller, now it does not.
> 
> For example, sk_stop_timer() does:
> 
> 	if (del_timer(timer))
> 		// no more wmb() here
> 		atomic_dec(&sk->sk_refcnt);
> 
> I think that this particular case is ok, but may be there is
> some user of timers which lacks the memory barrier?
> 
> ...
> --- 2.6.12-rc2+timer_patches/kernel/timer.c~	Sun Apr 24 11:59:31 2005
> +++ 2.6.12-rc2+timer_patches/kernel/timer.c	Sun Apr 24 13:35:01 2005
> @@ -341,6 +341,9 @@ int del_timer(struct timer_list *timer)
>  		spin_unlock_irqrestore(&base->lock, flags);
>  	}
>  
> +	if (ret)
> +		smp_wmb();
> +
>  	return ret;
>  }
>  
> @@ -387,6 +390,10 @@ unlock:
>  		spin_unlock_irqrestore(&base->lock, flags);
>  	} while (ret < 0);
>  
> +	if (ret)
> +		smp_wmb();
> +	smp_rmb();
> +
>  	return ret;
>  }
>  
> @@ -457,6 +464,7 @@ repeat:
>  
>  			set_running_timer(base, timer);
>  			detach_timer(timer, 1);
> +			smp_wmb();
>  			spin_unlock_irq(&base->t_base.lock);
>  			{
>  				u32 preempt_count = preempt_count();

I wonder if we should still do this?  It would seem to be a safer approach.

(This barrier stuff continues to give me the creeps.  Imagine being
dependent upon accidental barriers in the add/del/mod_timer code.  Ugh).

-
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/