Make net watchdog timers 1 sec jiffy aligned

PATCH In review Venki Pallipadi 7 participants 16 messages May 29, 2007 – Jun 1, 2007

Subsystems (3)

Networking Network drivers Traffic control (tc)

lore

Share & tools

lore

Venki Pallipadi

author

2007-05-29 18:04 UTC #0 lore raw

PATCH 3/4 Make net watchdog timers 1 sec jiffy aligned

round_jiffies for net dev watchdog timer.

Signed-off-by Venkatesh Pallipadi <[email protected]>

net/sched/sch_generic.c +2 -1

Index: linux-2.6.22-rc-mm/net/sched/sch_generic.c
===================================================================
@@ -224,7 +224,8 @@
 	if (dev->tx_timeout) {
 		if (dev->watchdog_timeo <= 0)
 			dev->watchdog_timeo = 5*HZ;
-		if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo))
+		if (!mod_timer(&dev->watchdog_timer,
+			       round_jiffies(jiffies + dev->watchdog_timeo)))
 			dev_hold(dev);
 	}
 }

Andrew Morton

2007-05-30 17:59 UTC #1 lore raw

Quoted 9 lines · On Tue, 29 May 2007 11:01:13 -0700 Venki Pallipadi…

round_jiffies for net dev watchdog timer.
Signed-off-by Venkatesh Pallipadi <[email protected]>
Index: linux-2.6.22-rc-mm/net/sched/sch_generic.c
===================================================================

net/sched/sch_generic.c line 231

Quoted 9 lines

net/sched/sch_generic.c +2 -1

@@ -224,7 +224,8 @@
 	if (dev->tx_timeout) {
 		if (dev->watchdog_timeo <= 0)
 			dev->watchdog_timeo = 5*HZ;
-		if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo))
+		if (!mod_timer(&dev->watchdog_timer,
+			       round_jiffies(jiffies + dev->watchdog_timeo)))
 			dev_hold(dev);
 	}
 }

Please cc netdev on net patches.

Again, I worry that if people set the watchdog timeout to, say, 0.1 seconds then they will get one second, which is grossly different.

And if they were to set it to 1.5 seconds, they'd get 2.0 which is pretty significant, too.

Stephen Hemminger

2007-05-30 18:32 UTC #2 lore raw

Quoted 24 lines · On Wed, 30 May 2007 10:59:36 -0700 Andrew Morton <[email protected]>…

Quoted 9 lines · On Tue, 29 May 2007 11:01:13 -0700 Venki Pallipadi…
round_jiffies for net dev watchdog timer.
Signed-off-by Venkatesh Pallipadi <[email protected]>
Index: linux-2.6.22-rc-mm/net/sched/sch_generic.c
===================================================================
net/sched/sch_generic.c line 231
Quoted 9 lines
net/sched/sch_generic.c +2 -1
@@ -224,7 +224,8 @@
 	if (dev->tx_timeout) {
 		if (dev->watchdog_timeo <= 0)
 			dev->watchdog_timeo = 5*HZ;
-		if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo))
+		if (!mod_timer(&dev->watchdog_timer,
+			       round_jiffies(jiffies + dev->watchdog_timeo)))
 			dev_hold(dev);
 	}
 }
Please cc netdev on net patches.

Quoted 5 lines

Again, I worry that if people set the watchdog timeout to, say, 0.1 seconds then they will get one second, which is grossly different.
And if they were to set it to 1.5 seconds, they'd get 2.0 which is pretty significant, too.

Alternatively, we could change to a timer that is pushed forward after each TX, maybe using hrtimer and hrtimer_forward(). That way the timer would never run in normal case.

Signature · 1 line

Stephen Hemminger <[email protected]>

Patrick McHardy

2007-05-30 18:43 UTC #3 lore raw

Quoted 20 lines · Stephen Hemminger

Quoted 20 lines
Quoted 5 lines
Index: linux-2.6.22-rc-mm/net/sched/sch_generic.c
===================================================================
net/sched/sch_generic.c line 231
Quoted 9 lines
net/sched/sch_generic.c +2 -1
@@ -224,7 +224,8 @@
 	if (dev->tx_timeout) {
 		if (dev->watchdog_timeo <= 0)
 			dev->watchdog_timeo = 5*HZ;
-		if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo))
+		if (!mod_timer(&dev->watchdog_timer,
+			       round_jiffies(jiffies + dev->watchdog_timeo)))
 			dev_hold(dev);
 	}
 }
Please cc netdev on net patches.
Again, I worry that if people set the watchdog timeout to, say, 0.1 seconds then they will get one second, which is grossly different.

Quoted 7 lines

Quoted 2 lines
And if they were to set it to 1.5 seconds, they'd get 2.0 which is pretty significant, too.
Alternatively, we could change to a timer that is pushed forward after each TX, maybe using hrtimer and hrtimer_forward(). That way the timer would never run in normal case.

It seems wasteful to add per-packet overhead for tx timeouts, which should be an exception. Do drivers really care about the exact timeout value? Compared to a packet transmission time its incredibly long anyways ..

Venki Pallipadi

author

2007-05-30 19:18 UTC #4 lore raw

Quoted 30 lines · Patrick McHardy

Quoted 20 lines · Stephen Hemminger
Quoted 20 lines
Quoted 5 lines
Index: linux-2.6.22-rc-mm/net/sched/sch_generic.c
===================================================================
net/sched/sch_generic.c line 231
Quoted 9 lines
net/sched/sch_generic.c +2 -1
@@ -224,7 +224,8 @@
 	if (dev->tx_timeout) {
 		if (dev->watchdog_timeo <= 0)
 			dev->watchdog_timeo = 5*HZ;
-		if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo))
+		if (!mod_timer(&dev->watchdog_timer,
+			       round_jiffies(jiffies + dev->watchdog_timeo)))
 			dev_hold(dev);
 	}
 }
Please cc netdev on net patches.
Again, I worry that if people set the watchdog timeout to, say, 0.1 seconds then they will get one second, which is grossly different.
Quoted 7 lines
Quoted 2 lines
And if they were to set it to 1.5 seconds, they'd get 2.0 which is pretty significant, too.
Alternatively, we could change to a timer that is pushed forward after each TX, maybe using hrtimer and hrtimer_forward(). That way the timer would never run in normal case.

Quoted 4 lines

It seems wasteful to add per-packet overhead for tx timeouts, which should be an exception. Do drivers really care about the exact timeout value? Compared to a packet transmission time its incredibly long anyways ..

I agree. Doing a mod_timer or hrtimer_forward to push forward may add to the complexity depending on how often TX happens.

Are the drivers really worried about exact timeouts here? Can we use rounding for the timers that are more than a second, at least?

Thanks,
Venki

Patrick McHardy

2007-05-30 19:33 UTC #5 lore raw

Quoted 9 lines · Venki Pallipadi

Quoted 5 lines · Patrick McHardy
It seems wasteful to add per-packet overhead for tx timeouts, which should be an exception. Do drivers really care about the exact timeout value? Compared to a packet transmission time its incredibly long anyways ..

Quoted 4 lines

I agree. Doing a mod_timer or hrtimer_forward to push forward may add to the complexity depending on how often TX happens.
Are the drivers really worried about exact timeouts here?

Just guessing, but I don't think they are, after all timers can be late.

Quoted 1 line

Can we use rounding for the timers that are more than a second, at least?

Also sounds reasonable.

David Miller

maintainer

2007-05-30 19:55 UTC #6 lore raw

In reply to Patrick McHardy

From: Patrick McHardy <[email protected]>
Date: Wed, 30 May 2007 20:42:32 +0200

Quoted 30 lines

Quoted 20 lines · Stephen Hemminger
Quoted 20 lines
Quoted 5 lines
Index: linux-2.6.22-rc-mm/net/sched/sch_generic.c
===================================================================
net/sched/sch_generic.c line 231
Quoted 9 lines
net/sched/sch_generic.c +2 -1
@@ -224,7 +224,8 @@
 	if (dev->tx_timeout) {
 		if (dev->watchdog_timeo <= 0)
 			dev->watchdog_timeo = 5*HZ;
-		if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo))
+		if (!mod_timer(&dev->watchdog_timer,
+			       round_jiffies(jiffies + dev->watchdog_timeo)))
 			dev_hold(dev);
 	}
 }
Please cc netdev on net patches.
Again, I worry that if people set the watchdog timeout to, say, 0.1 seconds then they will get one second, which is grossly different.
Quoted 7 lines
Quoted 2 lines
And if they were to set it to 1.5 seconds, they'd get 2.0 which is pretty significant, too.
Alternatively, we could change to a timer that is pushed forward after each TX, maybe using hrtimer and hrtimer_forward(). That way the timer would never run in normal case.

Quoted 4 lines

It seems wasteful to add per-packet overhead for tx timeouts, which should be an exception. Do drivers really care about the exact timeout value? Compared to a packet transmission time its incredibly long anyways ..

I agree, this change is absolutely rediculious and is just a blind cookie-cutter change made without consideration of what the code is doing and what it's requirements are.

Stephen Hemminger

2007-05-30 20:42 UTC #7 lore raw

Quoted 33 lines · On Wed, 30 May 2007 12:55:51 -0700 (PDT) David Miller <[email protected]>…

From: Patrick McHardy <[email protected]>
Date: Wed, 30 May 2007 20:42:32 +0200
Quoted 30 lines
Quoted 27 lines · Stephen Hemminger
Quoted 17 lines
Quoted 5 lines
Index: linux-2.6.22-rc-mm/net/sched/sch_generic.c
===================================================================
net/sched/sch_generic.c line 231
Quoted 9 lines
net/sched/sch_generic.c +2 -1
@@ -224,7 +224,8 @@
 	if (dev->tx_timeout) {
 		if (dev->watchdog_timeo <= 0)
 			dev->watchdog_timeo = 5*HZ;
-		if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo))
+		if (!mod_timer(&dev->watchdog_timer,
+			       round_jiffies(jiffies + dev->watchdog_timeo)))
 			dev_hold(dev);
 	}
 }
Please cc netdev on net patches.
Quoted 5 lines
Again, I worry that if people set the watchdog timeout to, say, 0.1 seconds then they will get one second, which is grossly different.
And if they were to set it to 1.5 seconds, they'd get 2.0 which is pretty significant, too.
Alternatively, we could change to a timer that is pushed forward after each TX, maybe using hrtimer and hrtimer_forward(). That way the timer would never run in normal case.

Quoted 9 lines

Quoted 4 lines
It seems wasteful to add per-packet overhead for tx timeouts, which should be an exception. Do drivers really care about the exact timeout value? Compared to a packet transmission time its incredibly long anyways ..
I agree, this change is absolutely rediculious and is just a blind cookie-cutter change made without consideration of what the code is doing and what it's requirements are.

what about the obvious compromise:

net/sched/sch_generic.c +5 -1

@@ -203,7 +203,11 @@ static void dev_watchdog(unsigned long a
 				       dev->name);
 				dev->tx_timeout(dev);
 			}
-			if (!mod_timer(&dev->watchdog_timer, round_jiffies(jiffies + dev->watchdog_timeo)))
+
+			if (!mod_timer(&dev->watchdog_timer,
+				       dev->watchdog_timeo > 2 * HZ
+				       ? round_jiffies(jiffies + dev->watchdog_timeo)
+				       : jiffies + dev->watchdog_timeo))
 				dev_hold(dev);
 		}
 	}

Signature · 1 line

Stephen Hemminger <[email protected]>

Venki Pallipadi

author

2007-05-30 21:13 UTC #10 lore raw

In reply to David Miller

Quoted 33 lines · David Miller

From: Patrick McHardy <[email protected]>
Date: Wed, 30 May 2007 20:42:32 +0200
Quoted 30 lines
Quoted 27 lines · Stephen Hemminger
Quoted 17 lines
Quoted 5 lines
Index: linux-2.6.22-rc-mm/net/sched/sch_generic.c
===================================================================
net/sched/sch_generic.c line 231
Quoted 9 lines
net/sched/sch_generic.c +2 -1
@@ -224,7 +224,8 @@
 	if (dev->tx_timeout) {
 		if (dev->watchdog_timeo <= 0)
 			dev->watchdog_timeo = 5*HZ;
-		if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo))
+		if (!mod_timer(&dev->watchdog_timer,
+			       round_jiffies(jiffies + dev->watchdog_timeo)))
 			dev_hold(dev);
 	}
 }
Please cc netdev on net patches.
Quoted 5 lines
Again, I worry that if people set the watchdog timeout to, say, 0.1 seconds then they will get one second, which is grossly different.
And if they were to set it to 1.5 seconds, they'd get 2.0 which is pretty significant, too.
Alternatively, we could change to a timer that is pushed forward after each TX, maybe using hrtimer and hrtimer_forward(). That way the timer would never run in normal case.

Quoted 8 lines

Quoted 4 lines
It seems wasteful to add per-packet overhead for tx timeouts, which should be an exception. Do drivers really care about the exact timeout value? Compared to a packet transmission time its incredibly long anyways ..
I agree, this change is absolutely rediculious and is just a blind cookie-cutter change made without consideration of what the code is doing and what it's requirements are.

I hope I could atleast highlight the issue here despite the cookie-cutter patch..
On a totally idle system I have something like 85 wakeups for every 5 seconds which I am trying to reduce (to reduce the power consumption and increase battery life. And 1 interrupt out of 85 happens to be netdev watchdog timer.

Thanks,
Venki

Venki Pallipadi

author

2007-05-30 21:39 UTC #8 lore raw

In reply to Stephen Hemminger

Quoted 55 lines · Stephen Hemminger

Quoted 33 lines · On Wed, 30 May 2007 12:55:51 -0700 (PDT) David Miller <[email protected]>…
From: Patrick McHardy <[email protected]>
Date: Wed, 30 May 2007 20:42:32 +0200
Quoted 30 lines
Quoted 27 lines · Stephen Hemminger
Quoted 17 lines
Quoted 5 lines
Index: linux-2.6.22-rc-mm/net/sched/sch_generic.c
===================================================================
net/sched/sch_generic.c line 231
Quoted 9 lines
net/sched/sch_generic.c +2 -1
@@ -224,7 +224,8 @@
 	if (dev->tx_timeout) {
 		if (dev->watchdog_timeo <= 0)
 			dev->watchdog_timeo = 5*HZ;
-		if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo))
+		if (!mod_timer(&dev->watchdog_timer,
+			       round_jiffies(jiffies + dev->watchdog_timeo)))
 			dev_hold(dev);
 	}
 }
Please cc netdev on net patches.
Quoted 5 lines
Again, I worry that if people set the watchdog timeout to, say, 0.1 seconds then they will get one second, which is grossly different.
And if they were to set it to 1.5 seconds, they'd get 2.0 which is pretty significant, too.
Alternatively, we could change to a timer that is pushed forward after each TX, maybe using hrtimer and hrtimer_forward(). That way the timer would never run in normal case.
Quoted 9 lines
Quoted 4 lines
It seems wasteful to add per-packet overhead for tx timeouts, which should be an exception. Do drivers really care about the exact timeout value? Compared to a packet transmission time its incredibly long anyways ..
I agree, this change is absolutely rediculious and is just a blind cookie-cutter change made without consideration of what the code is doing and what it's requirements are.
what about the obvious compromise:
net/sched/sch_generic.c +0 -0
@@ -203,7 +203,11 @@ static void dev_watchdog(unsigned long a
 				       dev->name);
 				dev->tx_timeout(dev);

net/sched/sch_generic.c static void dev_watchdog(unsigned long a · line 213

Quoted 10 lines

net/sched/sch_generic.c +5 -1

 			}
-			if (!mod_timer(&dev->watchdog_timer, round_jiffies(jiffies + dev->watchdog_timeo)))
+
+			if (!mod_timer(&dev->watchdog_timer,
+				       dev->watchdog_timeo > 2 * HZ
+				       ? round_jiffies(jiffies + dev->watchdog_timeo)
+				       : jiffies + dev->watchdog_timeo))
 				dev_hold(dev);
 		}
 	}

If this does not work:
Another option is to use 'deferrable timer' here which will be called at same as before time when CPU is busy and on idle CPU it will be delayed until CPU comes out of idle due to any other events.

Thanks,
Venki

Matt Mackall

2007-05-30 22:11 UTC #11 lore raw

In reply to David Miller

Quoted 33 lines · David Miller

From: Patrick McHardy <[email protected]>
Date: Wed, 30 May 2007 20:42:32 +0200
Quoted 30 lines
Quoted 27 lines · Stephen Hemminger
Quoted 17 lines
Quoted 5 lines
Index: linux-2.6.22-rc-mm/net/sched/sch_generic.c
===================================================================
net/sched/sch_generic.c line 231
Quoted 9 lines
net/sched/sch_generic.c +2 -1
@@ -224,7 +224,8 @@
 	if (dev->tx_timeout) {
 		if (dev->watchdog_timeo <= 0)
 			dev->watchdog_timeo = 5*HZ;
-		if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo))
+		if (!mod_timer(&dev->watchdog_timer,
+			       round_jiffies(jiffies + dev->watchdog_timeo)))
 			dev_hold(dev);
 	}
 }
Please cc netdev on net patches.
Quoted 5 lines
Again, I worry that if people set the watchdog timeout to, say, 0.1 seconds then they will get one second, which is grossly different.
And if they were to set it to 1.5 seconds, they'd get 2.0 which is pretty significant, too.
Alternatively, we could change to a timer that is pushed forward after each TX, maybe using hrtimer and hrtimer_forward(). That way the timer would never run in normal case.

Quoted 8 lines

Quoted 4 lines
It seems wasteful to add per-packet overhead for tx timeouts, which should be an exception. Do drivers really care about the exact timeout value? Compared to a packet transmission time its incredibly long anyways ..
I agree, this change is absolutely rediculious and is just a blind cookie-cutter change made without consideration of what the code is doing and what it's requirements are.

What are you agreeing with, Dave?

Are you agreeing that "it seems wasteful to add per-packet overhead"? This patch is not doing that.

A quick grep shows that most things are using multi-second timeouts here. Of the ones that aren't, a number are using .4s, and many more aren't even in units of HZ. Makes me wonder if the various boards using 50ms are being overzealous.

Signature · 1 line

Mathematics is the supreme nostalgia of our time.

David Miller

maintainer

2007-05-30 22:29 UTC #12 lore raw

From: Matt Mackall <[email protected]>
Date: Wed, 30 May 2007 17:10:39 -0500

Quoted 2 lines

Are you agreeing that "it seems wasteful to add per-packet overhead"? This patch is not doing that.

Yes, and I know that :-)

Matt Mackall

2007-05-30 22:36 UTC #13 lore raw

Quoted 7 lines · David Miller

From: Matt Mackall <[email protected]>
Date: Wed, 30 May 2007 17:10:39 -0500
Quoted 2 lines
Are you agreeing that "it seems wasteful to add per-packet overhead"? This patch is not doing that.
Yes, and I know that :-)

Is there a real reason for watchdog_timeo to be all over the map?

Signature · 1 line

Mathematics is the supreme nostalgia of our time.

David Miller

maintainer

2007-05-30 23:01 UTC #14 lore raw

From: Matt Mackall <[email protected]>
Date: Wed, 30 May 2007 17:36:12 -0500

Quoted 10 lines · David Miller

Quoted 7 lines
From: Matt Mackall <[email protected]>
Date: Wed, 30 May 2007 17:10:39 -0500
Quoted 2 lines
Are you agreeing that "it seems wasteful to add per-packet overhead"? This patch is not doing that.
Yes, and I know that :-)
Is there a real reason for watchdog_timeo to be all over the map?

In theory it's a hardware implementation and link speed specific value, in reality nearly everyone just copies values from other drivers.

Andi Kleen

2007-05-31 09:39 UTC #9 lore raw

In reply to Venki Pallipadi

Quoted 5 lines · Venki Pallipadi

If this does not work:
Another option is to use 'deferrable timer' here which will be called at same as before time when CPU is busy and on idle CPU it will be delayed until CPU comes out of idle due to any other events.

That would sound like a good idea here and at least power wise it should be nearly free

-Andi

David Miller

maintainer

2007-06-01 04:30 UTC #15 lore raw

In reply to Venki Pallipadi

From: Venki Pallipadi <[email protected]>
Date: Tue, 29 May 2007 11:01:13 -0700

Quoted 4 lines

round_jiffies for net dev watchdog timer.
Signed-off-by Venkatesh Pallipadi <[email protected]>

Patch applied, thanks.