| Message ID | 1359697980-28613-1-git-send-email-ying.xue@windriver.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Ying Xue <ying.xue@windriver.com> Date: Fri, 1 Feb 2013 13:53:00 +0800 > It's unnecessary to check whether the sock timer to be stopped is > pending or not in sk_stop_timer() as del_timer() will do the same > thing later. > > Signed-off-by: Ying Xue <ying.xue@windriver.com> Did it even occur to you that when this code was written, this "redundant" testing was also redundant, but that it might have been done on purpose? If you are going to change this code, you must understand why it was written this way, because that is the only context in which you will be able to justify removing the test. Otherwise I'm ignoring this patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-02-01 at 01:09 -0500, David Miller wrote: > From: Ying Xue <ying.xue@windriver.com> > Date: Fri, 1 Feb 2013 13:53:00 +0800 > > > It's unnecessary to check whether the sock timer to be stopped is > > pending or not in sk_stop_timer() as del_timer() will do the same > > thing later. > > > > Signed-off-by: Ying Xue <ying.xue@windriver.com> > > Did it even occur to you that when this code was written, this > "redundant" testing was also redundant, but that it might have been > done on purpose? > > If you are going to change this code, you must understand why it was > written this way, because that is the only context in which you will > be able to justify removing the test. > I had the same reaction but maybe its not anymore a valid thing. Before commit 55c888d6d ([PATCH] timers fixes/improvements) there was indeed a significant cost calling del_timer() because of unconditional spinlock acquisition. But nowadays del_timer() doesn't blindly lock the spinlock. So I guess we could change all occurrences of : if (timer_pending(X)) del_timer(X); It would save some bytes of code. But please Ying, do a complete patch for net tree, don't send 30 patches. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > From: Ying Xue <ying.xue@windriver.com> > Date: Fri, 1 Feb 2013 13:53:00 +0800 > >> It's unnecessary to check whether the sock timer to be stopped is >> pending or not in sk_stop_timer() as del_timer() will do the same >> thing later. >> >> Signed-off-by: Ying Xue <ying.xue@windriver.com> > > Did it even occur to you that when this code was written, this > "redundant" testing was also redundant, but that it might have been > done on purpose? > Sorry, at least I really cannot figure out the redundant test has some special purpose. However, it seems I found some clues which may prove the "redundant" test is really redundant: The sk_stop_timer() function is never changed after kernel initial git repository is built. But calling timer_pending() in del_timer() is involved by 55c888d6 commit. Regards, Ying > If you are going to change this code, you must understand why it was > written this way, because that is the only context in which you will > be able to justify removing the test. > > Otherwise I'm ignoring this patch. > Hi Dave, -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric Dumazet wrote: > On Fri, 2013-02-01 at 01:09 -0500, David Miller wrote: > >> From: Ying Xue <ying.xue@windriver.com> >> Date: Fri, 1 Feb 2013 13:53:00 +0800 >> >> >>> It's unnecessary to check whether the sock timer to be stopped is >>> pending or not in sk_stop_timer() as del_timer() will do the same >>> thing later. >>> >>> Signed-off-by: Ying Xue <ying.xue@windriver.com> >>> >> Did it even occur to you that when this code was written, this >> "redundant" testing was also redundant, but that it might have been >> done on purpose? >> >> If you are going to change this code, you must understand why it was >> written this way, because that is the only context in which you will >> be able to justify removing the test. >> >> > > I had the same reaction but maybe its not anymore a valid thing. > > Before commit 55c888d6d ([PATCH] timers fixes/improvements) there was > indeed a significant cost calling del_timer() because of unconditional > spinlock acquisition. > > But nowadays del_timer() doesn't blindly lock the spinlock. > > So I guess we could change all occurrences of : > > if (timer_pending(X)) > del_timer(X); > > It would save some bytes of code. > Eric, thanks for your explanation and suggestion. But I cannot understand why we should first call timer_pending() before del_timer() in your proposal. By my understanding, we might get an unreal timer pending state out of timer base lock (ie, lock_timer_base()), and the "unreal" is only for pending state, on the contrary, the value is real for inactive sate. So calling timer_pending() out of timer base lock scope can make us avoid some unnecessary grabbing spin lock operations. However, in del_timer() there already has placed a timer_pending() before lock_timer_base() is called. So why do we need another before calling del_timer()? Please you explain more. Thanks, Ying > But please Ying, do a complete patch for net tree, don't send 30 > patches. > > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-02-01 at 15:14 +0800, Xue Ying wrote: > Eric Dumazet wrote: > > I had the same reaction but maybe its not anymore a valid thing. > > > > Before commit 55c888d6d ([PATCH] timers fixes/improvements) there was > > indeed a significant cost calling del_timer() because of unconditional > > spinlock acquisition. > > > > But nowadays del_timer() doesn't blindly lock the spinlock. > > > > So I guess we could change all occurrences of : > > > > if (timer_pending(X)) > > del_timer(X); > > > > It would save some bytes of code. > > > Eric, thanks for your explanation and suggestion. > > But I cannot understand why we should first call timer_pending() before > del_timer() in your proposal. > By my understanding, we might get an unreal timer pending state out of > timer base lock (ie, lock_timer_base()), > and the "unreal" is only for pending state, on the contrary, the value > is real for inactive sate. > So calling timer_pending() out of timer base lock scope can make us > avoid some unnecessary grabbing spin lock operations. > However, in del_timer() there already has placed a timer_pending() > before lock_timer_base() is called. So why do we need > another before calling del_timer()? > > Please you explain more. I think you misunderstood me. I said that the old construct : if (timer_pending(X)) del_timer(X); could be changed to del_timer(X); Thats what your patch does. But instead of changing sk_stop_timer(), I suggested you do a patch on whole net tree. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric Dumazet wrote: > On Fri, 2013-02-01 at 15:14 +0800, Xue Ying wrote: >> Eric Dumazet wrote: > >>> I had the same reaction but maybe its not anymore a valid thing. >>> >>> Before commit 55c888d6d ([PATCH] timers fixes/improvements) there was >>> indeed a significant cost calling del_timer() because of unconditional >>> spinlock acquisition. >>> >>> But nowadays del_timer() doesn't blindly lock the spinlock. >>> >>> So I guess we could change all occurrences of : >>> >>> if (timer_pending(X)) >>> del_timer(X); >>> >>> It would save some bytes of code. >>> >> Eric, thanks for your explanation and suggestion. >> >> But I cannot understand why we should first call timer_pending() before >> del_timer() in your proposal. >> By my understanding, we might get an unreal timer pending state out of >> timer base lock (ie, lock_timer_base()), >> and the "unreal" is only for pending state, on the contrary, the value >> is real for inactive sate. >> So calling timer_pending() out of timer base lock scope can make us >> avoid some unnecessary grabbing spin lock operations. >> However, in del_timer() there already has placed a timer_pending() >> before lock_timer_base() is called. So why do we need >> another before calling del_timer()? >> >> Please you explain more. > > I think you misunderstood me. > > I said that the old construct : > > if (timer_pending(X)) > del_timer(X); > > could be changed to > > del_timer(X); > > Thats what your patch does. > > But instead of changing sk_stop_timer(), I suggested you do a patch on > whole net tree. > Fine :) I will do. Regards, Ying > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-02-01 at 09:26 +0000, David Laight wrote: > If timer_pending() is an inline function then the additional check > might be beneficial in very hot paths. The 'If' word you use show you didn't even read the code, and you are talking to linux netdev mailing list. We already know these things. So what are these very hot paths you identified ? If this was a real concern, the right thing to do is to have a generic helper so that its real clear and self documented. BTW sk_stop_timer() could be inlined. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/sock.c b/net/core/sock.c index bc131d4..33144bd 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -2212,7 +2212,7 @@ EXPORT_SYMBOL(sk_reset_timer); void sk_stop_timer(struct sock *sk, struct timer_list* timer) { - if (timer_pending(timer) && del_timer(timer)) + if (del_timer(timer)) __sock_put(sk); } EXPORT_SYMBOL(sk_stop_timer);
It's unnecessary to check whether the sock timer to be stopped is pending or not in sk_stop_timer() as del_timer() will do the same thing later. Signed-off-by: Ying Xue <ying.xue@windriver.com> --- net/core/sock.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)