On Wed, Mar 28, 2007 at 02:22:27AM +0400, Oleg Nesterov wrote: > On 03/27, Venki Pallipadi wrote: > > > > @@ -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.
I thought about this. But, chose the other one just to save one additional 'and' overhead. > > 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); > > ? I agree the above suggestion is clean. But, it will have one additional 'and' operation when we set NULL. I saw some concern from Andrew earlier on overhead this patch was adding. > > > + /* 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? AFAIK, kmalloc_node should return an even address always. I was just being paranoid and wanted to assert it here as otherwise some normal timer may end up being deferred timer. Thanks, Venki - 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/