Hello!
[ Andi, please, look at the end ]
> --- linux-2.4.0test1-ac5/net/ipv4/ipmr.c Mon May 15 21:24:15 2000
> +++ linux-akpm/net/ipv4/ipmr.c Tue May 30 21:29:45 2000
> @@ -756,10 +756,15 @@
> uc->mfc_mcastgrp == c->mfc_mcastgrp) {
> *cp = uc->next;
> if (atomic_dec_and_test(&cache_resolve_queue_len))
> - del_timer(&ipmr_expire_timer);
> + del_timer_async(&ipmr_expire_timer);
> break;
> }
> }
> + /*
> + * REVIEWME: this function can be called from process context, yet I
> + * did a del_timer_async. Can we use del_timer_sync? Can the handler
> + * and this code fight each other?
> + */
ipmr_expire_timer is static stateless timer. There are no problems with it.
Even not counting that del_timer_sync is deadlocky,
it should not be ever used for static timers, especially doing large work.
It would be useless loss of time, not more.
> + * REVIEWME: rt_cache_flush() can be called from process context. The above
> + * spinlock won't help us - rt_check_expire() could be running now (and
> doesn't use
> + * the lock anyway
> + * Should we use del_timer_sync() here?
> + */
It is the same as above.
> @@ -110,14 +110,17 @@
> {
> struct tcp_opt *tp = &sk->tp_pinfo.af_tcp;
>
> - if(timer_pending(&tp->retransmit_timer) &&
> del_timer(&tp->retransmit_timer))
> +/*
> + * REVIEWME: this function can be called from process context. See ipv4.txt
> + */
Certainly. So what?
> @@ -368,8 +371,12 @@
> tw->pprev_death = NULL;
> tcp_tw_put(tw);
> if (--tcp_tw_count == 0)
> - del_timer(&tcp_tw_timer);
> + del_timer_async(&tcp_tw_timer);
> }
> +/*
> + * REVIEWME: this function can be called from process context. Can it race
> with tcp_twkill()?
> + * I think it can, because tcp_twkill could be spinning on tw_death_lock()
> on another CPU right now!
> + */
> spin_unlock(&tw_death_lock);
> }
>
Let it to spin. No problems.
> @@ -710,7 +717,13 @@
>
> void tcp_delete_keepalive_timer (struct sock *sk)
> {
> - if (timer_pending(&sk->timer) && del_timer (&sk->timer))
> +/*
> + * REVIEWME. tcp_keepalive_timer can be called from process context. Can it
> race
> + * against the timer handler?
Of course. I repeat again and again, TCP use _reference_ _counting_.
so that it is insensitive to any races and orthogonal to the problem
under discussion.
> + *
> + * Also, do we need timer_pending here? It'll usually return true, perhaps?
> + */
> + if (timer_pending(&sk->timer) && del_timer_async(&sk->timer))
> __sock_put(sk);
It is optimization, del_timer() is too expensive to be used in TCP.
> igmp.c
> ======
>
> igmp_start_timer()
>
> OK as long as always called from BH context.
>
> igmp_mod_timer()
>
> OK as long as always called from BH context.
Forget about this argument.
Words "always called from BH context" are non-sense in 2.4 net/ipv4.
The code is invariant wrt BH<->process context and 99% of code
is called from both of contexts.
> ip_fragment.c
> =============
>
> ip_find()
>
> It says "We are always in BH context", so let's trust that.
It lies, it has the same bug as ipv6/reassembly.c
> I used del_timer_async(), but this needs reviewing.
Andrew, I repeat again, all the timers in net/ipv4 are supposed
to be del_timer_async(). See?
Bugs in ipv6 and ipv4 defragmenter are noticed, they will be fixed after
talking to Andi, he had some massive changes to defragmenter.
Andi, what is status of that your patch? May I change defragmenter
to fix timers there?
Alexey
|