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

[Oleg Nesterov <oleg@tv-sign.ru>]

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.