lkml.org 
[lkml]   [2007]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] Add support for deferrable timers (respun)
    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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-03-28 00:25    [W:0.026 / U:1.008 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site