[PATCH v2] ipv4 : igmp : optimize timer modify logic in igmp_mod_timer()

[PATCH v2] ipv4 : igmp : optimize timer modify logic in igmp_mod_timer()

[Jun Zhao]

When timer is pending and expires less-than-or-equal-to new delay,
we need not used del_timer()/add_timer().

Signed-off-by: Jun Zhao <mypopydev@gmail.com>
---
 net/ipv4/igmp.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index c7472ef..5f2aedf 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -215,14 +215,14 @@ static void igmp_mod_timer(struct ip_mc_list *im, int max_delay)
 {
 	spin_lock_bh(&im->lock);
 	im->unsolicit_count = 0;
-	if (del_timer(&im->timer)) {
-		if ((long)(im->timer.expires-jiffies) < max_delay) {
-			add_timer(&im->timer);
-			im->tm_running = 1;
+	if (timer_pending(&im->timer)) {
+		if (time_before_eq(im->timer.expires, (jiffies + max_delay))) {
 			spin_unlock_bh(&im->lock);
 			return;
+		} else {
+			del_timer(&im->timer);
+			atomic_dec(&im->refcnt);
 		}
-		atomic_dec(&im->refcnt);
 	}
 	igmp_start_timer(im, max_delay);
 	spin_unlock_bh(&im->lock);
-- 
1.7.2.5

Re: [PATCH v2] ipv4 : igmp : optimize timer modify logic in igmp_mod_timer()

[David Miller]

From: Jun Zhao <mypopydev@gmail.com>
Date: Thu, 24 Nov 2011 00:38:42 +0800

> When timer is pending and expires less-than-or-equal-to new delay,
> we need not used del_timer()/add_timer().
> 
> Signed-off-by: Jun Zhao <mypopydev@gmail.com>

You did not answer Eric's question, why are you optimizing this
less-used code path?

Re: [PATCH v2] ipv4 : igmp : optimize timer modify logic in igmp_mod_timer()

[Jun Zhao]

On Wed, 2011-11-23 at 17:28 -0500, David Miller wrote:
> From: Jun Zhao <mypopydev@gmail.com>
> Date: Thu, 24 Nov 2011 00:38:42 +0800
> 
> > When timer is pending and expires less-than-or-equal-to new delay,
> > we need not used del_timer()/add_timer().
> > 
> > Signed-off-by: Jun Zhao <mypopydev@gmail.com>
> 
> You did not answer Eric's question, why are you optimizing this
> less-used code path?

1). Oh, in the RFC 3376 $5.2, Page 23:

"
   4. If there already is a pending response to a previous Query
      scheduled for this group, and either the new Query is a Group-
      Specific Query or the recorded source-list associated with the
      group is empty, then the group source-list is cleared and a single
      response is scheduled using the group timer.  The new response is
      scheduled to be sent at the earliest of the remaining time for the
      pending report and the selected delay.

   5. If the received Query is a Group-and-Source-Specific Query and
      there is a pending response for this group with a non-empty
      source-list, then the group source list is augmented to contain
      the list of sources in the new Query and a single response is
      scheduled using the group timer.  The new response is scheduled to
      be sent at the earliest of the remaining time for the pending
      report and the selected delay.
"
I think this patch more conform to the RFC.

2). Maybe this is a less-used code path, but we can do better than
current.

3). In the current implementation, "im->tm_running = 1;" is redundant 

Re: [PATCH v2] ipv4 : igmp : optimize timer modify logic in igmp_mod_timer()

[David Miller]

From: Jun Zhao <mypopydev@gmail.com>
Date: Thu, 24 Nov 2011 06:54:45 +0800

> On Wed, 2011-11-23 at 17:28 -0500, David Miller wrote:
>> From: Jun Zhao <mypopydev@gmail.com>
>> Date: Thu, 24 Nov 2011 00:38:42 +0800
>> 
>> > When timer is pending and expires less-than-or-equal-to new delay,
>> > we need not used del_timer()/add_timer().
>> > 
>> > Signed-off-by: Jun Zhao <mypopydev@gmail.com>
>> 
>> You did not answer Eric's question, why are you optimizing this
>> less-used code path?
> 
> 1). Oh, in the RFC 3376 $5.2, Page 23:

Then your commit message is terrible.

Your commit message, one the one hand, talks about optimizing the code.

Your explanation here talks about RFC conformance.

Your inconsistencies, and how you ignore important questions posed to
you like Eric's (until I point it out to you) makes your work
incredibly irritating to review and process.

Your patch submissions need to be more well formed and your commit
messages need to explain exactly what your goals are with your change
and how those goals are being met by the patch you are proposing.

When we read "optimize timer modify logic" how the heck are we
supposed to know what this change is actually doing?  Why should we
think that we actually need your change?  How am we supposed to figure
out that you are fixing an RFC conformance issue?

I'm sorry, this patch submission is junk.  Don't send us junk.

Re: [PATCH v2] ipv4 : igmp : optimize timer modify logic in igmp_mod_timer()

[Jun Zhao]

On Wed, 2011-11-23 at 19:04 -0500, David Miller wrote:
> From: Jun Zhao <mypopydev@gmail.com>
> Date: Thu, 24 Nov 2011 06:54:45 +0800
> 
> > On Wed, 2011-11-23 at 17:28 -0500, David Miller wrote:
> >> From: Jun Zhao <mypopydev@gmail.com>
> >> Date: Thu, 24 Nov 2011 00:38:42 +0800
> >> 
> >> > When timer is pending and expires less-than-or-equal-to new delay,
> >> > we need not used del_timer()/add_timer().
> >> > 
> >> > Signed-off-by: Jun Zhao <mypopydev@gmail.com>
> >> 
> >> You did not answer Eric's question, why are you optimizing this
> >> less-used code path?
> > 
> > 1). Oh, in the RFC 3376 $5.2, Page 23:
> 
> Then your commit message is terrible.
> 
> Your commit message, one the one hand, talks about optimizing the code.
> 
> Your explanation here talks about RFC conformance.
> 
> Your inconsistencies, and how you ignore important questions posed to
> you like Eric's (until I point it out to you) makes your work
> incredibly irritating to review and process.

I think Eric's means the v1 patch have a obvious bug about lock, I
didn't ignore it. ：（

> 
> Your patch submissions need to be more well formed and your commit
> messages need to explain exactly what your goals are with your change
> and how those goals are being met by the patch you are proposing.
> 
> When we read "optimize timer modify logic" how the heck are we
> supposed to know what this change is actually doing?  Why should we
> think that we actually need your change?  How am we supposed to figure
> out that you are fixing an RFC conformance issue?
> 

I got it, Tks. Maybe the terrible commit message lead to the problem,
I just try to make this function more readability.

Yes, I need to describe the goals more accurately in the commit.

> I'm sorry, this patch submission is junk.  Don't send us junk.
> 

I got it.