On 03/27, Venki Pallipadi wrote: > > Incremental patch below eliminates this race. > > Index: new/kernel/timer.c > =================================================================== > --- new.orig/kernel/timer.c 2007-03-26 15:19:35.000000000 -0800 > +++ new/kernel/timer.c 2007-03-27 13:00:33.000000000 -0800 > @@ -96,9 +96,9 @@ > return tbase_get_deferrable(timer->base); > } > > -static inline struct tvec_t_base_s *timer_get_base(struct timer_list *timer) > +static inline struct tvec_t_base_s *tbase_get_base(struct tvec_t_base_s > *base) > { > - return ((struct tvec_t_base_s *)((unsigned long)(timer->base) & > + return ((struct tvec_t_base_s *)((unsigned long)base & > ~TBASE_DEFERRABLE_FLAG)); > } > > @@ -368,7 +368,7 @@ > > for (;;) { > tvec_base_t *prelock_base = timer->base; > - base = timer_get_base(timer); > + base = tbase_get_base(prelock_base); > if (likely(base != NULL)) { > spin_lock_irqsave(&base->lock, *flags); > if (likely(prelock_base == timer->base))
Looks correct to me... Personally, I'd prefer static tvec_base_t *lock_timer_base(struct timer_list *timer, unsigned long *flags) __acquires(timer->base->lock) { tvec_base_t *base; for (;;) { base = timer_get_base(timer); if (likely(base != NULL)) { spin_lock_irqsave(&base->lock, *flags); if (likely(base == timer_get_base(timer)) return base; /* The timer has migrated to another CPU */ spin_unlock_irqrestore(&base->lock, *flags); } cpu_relax(); } } but this is a matter of taste. A minor nitpick, > +/* new_base is guaranteed to have last bit not set, in all callers below */ > +static inline void timer_set_base(struct timer_list *timer, > + struct tvec_t_base_s *old_base, > + struct tvec_t_base_s *new_base) > +{ > + timer->base = (struct tvec_t_base_s *)((unsigned long)(new_base) | > + > tbase_get_deferrable(old_base)); > +} looks a little bit ugly, but may be this is just me. How about void timer_set_base(struct timer_list *timer, struct tvec_t_base_s *new_base) { timer->base = (struct tvec_t_base_s *) ((unsigned long)(new_base) | tbase_get_deferrable(timer->base)); } __mod_timer: - tvec_base_t *old_base = timer->base; - timer->base = NULL; + timer_set_base(timer, NULL); ? > + /* Make sure that tvec_base is 2 byte aligned */ > + if (tbase_get_deferrable(base)) { > + WARN_ON(1); > + kfree(base); > + return -ENOMEM; > + } Not a comment, but a question: do we really need this? Oleg. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/