Re: [PATCH] Add support for deferrable timers (respun)

[Venki Pallipadi <venkatesh.pallipadi@intel.com>]

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