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/
