[PATCH] net: remove redundant checking for sock timer state

[PATCH] net: remove redundant checking for sock timer state

[Ying Xue]

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(-)

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);
-- 
1.7.1

Re: [PATCH] net: remove redundant checking for sock timer state

[David Miller]

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.

Re: [PATCH] net: remove redundant checking for sock timer state

[Eric Dumazet]

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.

Re: [PATCH] net: remove redundant checking for sock timer state

[Ying Xue]

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,

Re: [PATCH] net: remove redundant checking for sock timer state

[Xue Ying]

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
>
>   

Re: [PATCH] net: remove redundant checking for sock timer state

[Eric Dumazet]

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.

Re: [PATCH] net: remove redundant checking for sock timer state

[Ying Xue]

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

> 
> 
> 

RE: [PATCH] net: remove redundant checking for sock timer state

[David Laight]

> I said that the old construct :
> 
> if (timer_pending(X))
>     del_timer(X);
> 
> could be changed to
> 
> del_timer(X);

If timer_pending() is an inline function then the additional check
might be beneficial in very hot paths.

	David

RE: [PATCH] net: remove redundant checking for sock timer state

[Eric Dumazet]

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.