Re: [timers] net/core/*

[Andrew Morton <andrewm@xxxxxxxxxx>]

kuznet@xxxxxxxxxxxxx wrote:
> 
> >   I'm dazed and confused.
> 
> I do not understand, what confused you. __dst_free can be called
> from any context.

I believe there's a very unlikely race in __dst_free():

void __dst_free(struct dst_entry * dst)
{
// dst_run_gc() could now be running, but hasn't hit the spin_trylock()
yet.
        spin_lock_bh(&dst_lock);

        /* The first case (dev==NULL) is required, when
           protocol module is unloaded.
         */
        if (dst->dev == NULL || !(dst->dev->flags&IFF_UP)) {
                dst->input = dst_discard;
                dst->output = dst_blackhole;
        }
// still hasn't hit the spin_trylock()
        dst->obsolete = 2;
        dst->next = dst_garbage_list;
        dst_garbage_list = dst;
        if (dst_gc_timer_inc > DST_GC_INC) {
                del_timer_async(&dst_gc_timer);
                dst_gc_timer_inc = DST_GC_INC;
                dst_gc_timer_expires = DST_GC_MIN;
                dst_gc_timer.expires = jiffies + dst_gc_timer_expires;
                add_timer(&dst_gc_timer);
        }
        spin_unlock_bh(&dst_lock);
// NOW hits spin_trylock().  It succeeds.  dst_gc_timer is added a
second time.
}

> >   I used del_timer_async().  Added REVIEWME.
> 
> It does reference counting.
> 
> Andrew, did you read my mails or you did not?

Avidly.

I think I now see what you mean by "reference counting":

static int neigh_del_timer(struct neighbour *n)
{
        if (n->nud_state & NUD_IN_TIMER) {
                if (del_timer_async(&n->timer)) {
                        neigh_release(n);
                        return 1;
                }
        }
        return 0;
}

If del_timer_async() returns non-zero then we KNOW that
the timer was deleted and we KNOW that it hasn't run
and we KNOW that it isn't running now.  So that's a
reference count to the timer and we can safely do
the neigh_release().


BTW:

asmlinkage void do_softirq()
{
        ...
restart:
        ...
                if ((active &= mask) != 0)
                        goto retry;
        ...
retry:
        goto restart;
}

Doesn't need the double-goto.