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/
