Skip to contentSkip to conversation

Additions to full lowlatency patch

PATCH In review Jussi Laako 4 participants 7 messages Jan 12, 2002 – Jan 13, 2002

Subsystems (2)
Network drivers Char and misc drivers
lore
Share & tools
lore

Jussi Laako

author
#0 lore raw

PATCH 1/1 Additions to full lowlatency patch

Hi,

I've done some changes to lowlatency patched kernel. Mainly "fixes" to DRM drivers and few network drivers. Most probably I have done something really stupid, but those work here(tm). Especially the Radeon driver patch has got a lot of testing and seems to have huge impact to latencies in my system.

Best regards,

- Jussi Laako

Signature · 2 lines
PGP key fingerprint: 161D 6FED 6A92 39E2 EB5B  39DD A4DE 63EB C216 1E4B
Available at PGP keyservers
drivers/char/drm/mga_dma.c +3 -0
diff -ur linux-2.4.17-lowlatency/drivers/char/drm/mga_dma.c linux-2.4.17-lowlatency-jl/drivers/char/drm/mga_dma.c
@@ -61,6 +61,7 @@
 			MGA_WRITE8( MGA_CRTC_INDEX, 0 );
 			return 0;
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -80,6 +81,7 @@
 	for ( i = 0 ; i < dev_priv->usec_timeout ; i++ ) {
 		status = MGA_READ( MGA_STATUS ) & MGA_DMA_IDLE_MASK;
 		if ( status == MGA_ENDPRDMASTS ) return 0;
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -121,6 +123,7 @@
 	 * How about we clean up after ourselves?
 	 */
 	MGA_WRITE( MGA_RST, MGA_SOFTRESET );
+	conditional_schedule();		/* We shouldn't get here anyway... */
 	udelay( 15 );				/* Wait at least 10 usecs */
 	MGA_WRITE( MGA_RST, 0 );
drivers/char/drm/r128_cce.c +6 -0
diff -ur linux-2.4.17-lowlatency/drivers/char/drm/r128_cce.c linux-2.4.17-lowlatency-jl/drivers/char/drm/r128_cce.c
@@ -128,6 +128,7 @@
 		if ( !(R128_READ( R128_PC_NGUI_CTLSTAT ) & R128_PC_BUSY) ) {
 			return 0;
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -144,6 +145,7 @@
 	for ( i = 0 ; i < dev_priv->usec_timeout ; i++ ) {
 		int slots = R128_READ( R128_GUI_STAT ) & R128_GUI_FIFOCNT_MASK;
 		if ( slots >= entries ) return 0;
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -165,6 +167,7 @@
 			r128_do_pixcache_flush( dev_priv );
 			return 0;
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -225,6 +228,7 @@
 				return r128_do_pixcache_flush( dev_priv );
 			}
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -928,6 +932,7 @@
 				return buf;
 			}
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -961,6 +966,7 @@
 		r128_update_ring_snapshot( ring );
 		if ( ring->space >= n )
 			return 0;
+		conditional_schedule();
 		udelay( 1 );
 	}
drivers/char/drm/radeon_cp.c +5 -0
diff -ur linux-2.4.17-lowlatency/drivers/char/drm/radeon_cp.c linux-2.4.17-lowlatency-jl/drivers/char/drm/radeon_cp.c
@@ -356,6 +356,7 @@
 		       & RADEON_RB2D_DC_BUSY) ) {
 			return 0;
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -375,6 +376,7 @@
 		int slots = ( RADEON_READ( RADEON_RBBM_STATUS )
 			      & RADEON_RBBM_FIFOCNT_MASK );
 		if ( slots >= entries ) return 0;
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -398,6 +400,7 @@
 			radeon_do_pixcache_flush( dev_priv );
 			return 0;
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -1316,6 +1319,7 @@
 			start = 0;
 #endif
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -1355,6 +1359,7 @@
 		radeon_update_ring_snapshot( ring );
 		if ( ring->space > n )
 			return 0;
+		conditional_schedule();
 		udelay( 1 );
 	}
drivers/net/3c59x.c +1 -0
diff -ur linux-2.4.17-lowlatency/drivers/net/3c59x.c linux-2.4.17-lowlatency-jl/drivers/net/3c59x.c
@@ -1368,6 +1368,7 @@
 					   dev->name, cmd, i * 10);
 			return;
 		}
+		conditional_schedule();
 		udelay(10);
 	}
 	printk(KERN_ERR "%s: command 0x%04x did not complete! Status=0x%x\n",
drivers/net/8139too.c +3 -0
diff -ur linux-2.4.17-lowlatency/drivers/net/8139too.c linux-2.4.17-lowlatency-jl/drivers/net/8139too.c
@@ -707,6 +707,7 @@
 		barrier();
 		if ((RTL_R8 (ChipCmd) & CmdReset) == 0)
 			break;
+		conditional_schedule();
 		udelay (10);
 	}
 }

@@ -1774,6 +1775,7 @@
 	RTL_W8_F (ChipCmd, CmdTxEnb);
 	tmp_work = 200;
 	while (--tmp_work > 0) {
+		conditional_schedule();
 		udelay(1);
 		tmp8 = RTL_R8 (ChipCmd);
 		if (!(tmp8 & CmdRxEnb))

@@ -1785,6 +1787,7 @@
 	tmp_work = 200;
 	while (--tmp_work > 0) {
 		RTL_W8_F (ChipCmd, CmdRxEnb | CmdTxEnb);
+		conditional_schedule();
 		udelay(1);
 		tmp8 = RTL_R8 (ChipCmd);
 		if ((tmp8 & CmdRxEnb) && (tmp8 & CmdTxEnb))
drivers/net/eepro100.c +5 -1
diff -ur linux-2.4.17-lowlatency/drivers/net/eepro100.c linux-2.4.17-lowlatency-jl/drivers/net/eepro100.c
@@ -324,7 +324,11 @@
 static inline void wait_for_cmd_done(long cmd_ioaddr)
 {
 	int wait = 1000;
-	do  udelay(1) ;
+	do
+	{
+			conditional_schedule();
+			udelay(1) ;
+	}
 	while(inb(cmd_ioaddr) && --wait >= 0);
 #ifndef final_version
 	if (wait < 0)
#1 lore raw
Quoted 5 lines · In article <[email protected]> you wrote:
drivers/net/eepro100.c Mon Dec 31 22:41:42 2001 renamed from drivers/net/eepro100.c Fri Dec 21 19:41:54 2001 +0 -0
@@ -324,7 +324,11 @@
 static inline void wait_for_cmd_done(long cmd_ioaddr)
 {

drivers/net/eepro100.c Mon Dec 31 22:41:42 2001 line 334

Quoted 10 lines
drivers/net/eepro100.c Mon Dec 31 22:41:42 2001 renamed from drivers/net/eepro100.c Fri Dec 21 19:41:54 2001 +5 -1
       int wait = 1000;
-       do  udelay(1) ;
+       do
+       {
+                       conditional_schedule();
+                       udelay(1) ;
+       }
       while(inb(cmd_ioaddr) && --wait >= 0);
 #ifndef final_version
       if (wait < 0)

Did you audit all uses of this function ? It sort of looks like you're doing "hey there's a udelay lets add a schedule".. ok that's a bit rude but I'm not totally convinced that this function isn't called with spinlocks helt...

author
#2 lore raw
Quoted 5 lines · [email protected] wrote:

Did you audit all uses of this function ? It sort of looks like you're doing "hey there's a udelay lets add a schedule".. ok that's a bit rude but I'm not totally convinced that this function isn't called with spinlocks helt...

I have not done deeper research on the codepaths, but I have tested the drivers after change and they work. I tried to look for for/while { usleep(n); } timeout waits and no dangerous looking spinlocks held or interrupts disabled.

I'm ready to do more extensive work to fix similar cases if I get theoretical "OK" for the changes. I won't continue if people point out that this is wrong way(tm).

The patch is quick hack for my own use and for review and shouldn't be used unless proven correct. It's just my first try to hack linux kernel... :)

I just dislike

while (!hardware_completed()) usleep(1000);

-style code in some drivers (a bit extreme example).

- Jussi Laako

Signature · 2 lines
PGP key fingerprint: 161D 6FED 6A92 39E2 EB5B  39DD A4DE 63EB C216 1E4B
Available at PGP keyservers
#3 lore raw
In reply to Jussi Laako
Quoted 6 lines · On Sat, 12 Jan 2002 23:48:19 +0200 Jussi Laako <[email protected]> wrote:

Hi,

I've done some changes to lowlatency patched kernel. Mainly "fixes" to DRM drivers and few network drivers. Most probably I have done something really stupid, but those work here(tm). Especially the Radeon driver patch has got a lot of testing and seems to have huge impact to latencies in my system.

That leaves us with just about another 4000 source files in the tree to fill up with conditional_schedule()s. If we made that up, we should start talking about a single event queue throughout the kernel, because this is a very successful and unbelievably elegant piece of software design found in the market of OSs either. :-(

You are just kidding, aren't you?

Regards,
Stephan

#4 lore raw
In reply to Jussi Laako
Quoted 8 lines · Jussi Laako

Hi,

I've done some changes to lowlatency patched kernel. Mainly "fixes" to DRM drivers and few network drivers. Most probably I have done something really stupid, but those work here(tm). Especially the Radeon driver patch has got a lot of testing and seems to have huge impact to latencies in my system.

Thanks, Jussi - I'll crunch on this, merge the bits I agree with :)

As Arjan points out, the eepro100 change will cause deadlocks on SMP, and general badness on uniprocessor. But I've done a heap of testing on a eepro100 machine and it hasn't been a problem. I expect that wait_for_cmd_done() is only a problem during device startup and shutdown. And possibly in error recovery.

I take the position that device driver startup and shutdown functions are a complete basket case, and they are on the "don't do that" list. Generally, this is OK. Latency-critical applications should be careful to ensure that all required kernel modules are loaded beforehand, and that the cron jobs which reap idle kernel modules be disabled. Maybe, they should also ensure that any device-special files are held open across the life of the application.

I used to take the same position on fileystem mount and unmount, however things like autofs and some applciations which poll cdrom drives made this impractical, so filesystem mounts and unmounts are on the "do do that" list. I hope.

-

author
#5 lore raw
In reply to Jussi Laako
Quoted 6 lines · Andrew Morton

As Arjan points out, the eepro100 change will cause deadlocks on SMP, and general badness on uniprocessor. But I've done a heap of testing on a eepro100 machine and it hasn't been a problem. I expect that wait_for_cmd_done() is only a problem during device startup and shutdown. And possibly in error recovery.

Yes, I was unsure about the eepro100 driver, but didn't revert the patch because it worked in my UP tests.

I usually test the network drivers in 100 Mbps switched network and with 10 Mbps overloaded hub with huge amounts of collisions. The 3c90x driver has been badly behaving in very congested networks. It looks like it can spend worst case 1 second in timeout loop.

I have soft realtime application which needs to have simultaneous soundcard/disk/network/GUI access and that's the motivation for the patch.

Quoted 4 lines

I take the position that device driver startup and shutdown functions are a complete basket case, and they are on the "don't do that" list. Generally, this is OK. Latency-critical applications should be careful to ensure that all required kernel modules are loaded beforehand,

I completely agree, the startup code is usually too timing critical. That's why I left the initialization parts out of scope.

- Jussi Laako

Signature · 2 lines
PGP key fingerprint: 161D 6FED 6A92 39E2 EB5B  39DD A4DE 63EB C216 1E4B
Available at PGP keyservers
author
#6 lore raw
In reply to Jussi Laako

Hi,

Here's my second try. I read through the code once more and made some fixes. Is this correct way to avoid conditional_schedule() when holding spinlock? Both eepro100 and 3c90x drivers call the command completion wait routine with spinlock held in few places. I didn't find such situations in 8139too nor the DRM drivers.

- Jussi Laako

Signature · 2 lines
PGP key fingerprint: 161D 6FED 6A92 39E2 EB5B  39DD A4DE 63EB C216 1E4B
Available at PGP keyservers
drivers/char/drm/mga_dma.c +3 -0
diff -urN linux/drivers/char/drm/mga_dma.c linux-2.4.17-jl7-ll/drivers/char/drm/mga_dma.c
@@ -61,6 +61,7 @@
 			MGA_WRITE8( MGA_CRTC_INDEX, 0 );
 			return 0;
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -80,6 +81,7 @@
 	for ( i = 0 ; i < dev_priv->usec_timeout ; i++ ) {
 		status = MGA_READ( MGA_STATUS ) & MGA_DMA_IDLE_MASK;
 		if ( status == MGA_ENDPRDMASTS ) return 0;
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -121,6 +123,7 @@
 	 * How about we clean up after ourselves?
 	 */
 	MGA_WRITE( MGA_RST, MGA_SOFTRESET );
+	conditional_schedule();		/* We shouldn't get here anyway... */
 	udelay( 15 );				/* Wait at least 10 usecs */
 	MGA_WRITE( MGA_RST, 0 );
drivers/char/drm/r128_cce.c +6 -0
diff -urN linux/drivers/char/drm/r128_cce.c linux-2.4.17-jl7-ll/drivers/char/drm/r128_cce.c
@@ -128,6 +128,7 @@
 		if ( !(R128_READ( R128_PC_NGUI_CTLSTAT ) & R128_PC_BUSY) ) {
 			return 0;
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -144,6 +145,7 @@
 	for ( i = 0 ; i < dev_priv->usec_timeout ; i++ ) {
 		int slots = R128_READ( R128_GUI_STAT ) & R128_GUI_FIFOCNT_MASK;
 		if ( slots >= entries ) return 0;
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -165,6 +167,7 @@
 			r128_do_pixcache_flush( dev_priv );
 			return 0;
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -225,6 +228,7 @@
 				return r128_do_pixcache_flush( dev_priv );
 			}
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -928,6 +932,7 @@
 				return buf;
 			}
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -961,6 +966,7 @@
 		r128_update_ring_snapshot( ring );
 		if ( ring->space >= n )
 			return 0;
+		conditional_schedule();
 		udelay( 1 );
 	}
drivers/char/drm/radeon_cp.c +5 -0
diff -urN linux/drivers/char/drm/radeon_cp.c linux-2.4.17-jl7-ll/drivers/char/drm/radeon_cp.c
@@ -356,6 +356,7 @@
 		       & RADEON_RB2D_DC_BUSY) ) {
 			return 0;
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -375,6 +376,7 @@
 		int slots = ( RADEON_READ( RADEON_RBBM_STATUS )
 			      & RADEON_RBBM_FIFOCNT_MASK );
 		if ( slots >= entries ) return 0;
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -398,6 +400,7 @@
 			radeon_do_pixcache_flush( dev_priv );
 			return 0;
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -1316,6 +1319,7 @@
 			start = 0;
 #endif
 		}
+		conditional_schedule();
 		udelay( 1 );
 	}
 

@@ -1355,6 +1359,7 @@
 		radeon_update_ring_snapshot( ring );
 		if ( ring->space > n )
 			return 0;
+		conditional_schedule();
 		udelay( 1 );
 	}
drivers/net/3c59x.c +3 -0
diff -urN linux/drivers/net/3c59x.c linux-2.4.17-jl7-ll/drivers/net/3c59x.c
@@ -1353,6 +1353,7 @@
 issue_and_wait(struct net_device *dev, int cmd)
 {
 	int i;
+	struct vortex_private *vp = (struct vortex_private *)dev->priv
 
 	outw(cmd, dev->base_addr + EL3_CMD);
 	for (i = 0; i < 2000; i++) {

@@ -1368,6 +1369,8 @@
 					   dev->name, cmd, i * 10);
 			return;
 		}
+		if (!spin_is_locked(&vp->lock))
+			conditional_schedule();
 		udelay(10);
 	}
 	printk(KERN_ERR "%s: command 0x%04x did not complete! Status=0x%x\n",
drivers/net/8139too.c +3 -0
diff -urN linux/drivers/net/8139too.c linux-2.4.17-jl7-ll/drivers/net/8139too.c
@@ -707,6 +707,7 @@
 		barrier();
 		if ((RTL_R8 (ChipCmd) & CmdReset) == 0)
 			break;
+		conditional_schedule();
 		udelay (10);
 	}
 }

@@ -1774,6 +1775,7 @@
 	RTL_W8_F (ChipCmd, CmdTxEnb);
 	tmp_work = 200;
 	while (--tmp_work > 0) {
+		conditional_schedule();
 		udelay(1);
 		tmp8 = RTL_R8 (ChipCmd);
 		if (!(tmp8 & CmdRxEnb))

@@ -1785,6 +1787,7 @@
 	tmp_work = 200;
 	while (--tmp_work > 0) {
 		RTL_W8_F (ChipCmd, CmdRxEnb | CmdTxEnb);
+		conditional_schedule();
 		udelay(1);
 		tmp8 = RTL_R8 (ChipCmd);
 		if ((tmp8 & CmdRxEnb) && (tmp8 & CmdTxEnb))
drivers/net/eepro100.c +9 -2
diff -urN linux/drivers/net/eepro100.c linux-2.4.17-jl7-ll/drivers/net/eepro100.c
@@ -321,10 +321,17 @@
 
 /* How to wait for the command unit to accept a command.
    Typically this takes 0 ticks. */
-static inline void wait_for_cmd_done(long cmd_ioaddr)
+#define wait_for_cmd_done(x)	_wait_for_cmd_done(&sp->lock, x)
+static inline void _wait_for_cmd_done(spinlock_t *lelock, 
+	long cmd_ioaddr)
 {
 	int wait = 1000;
-	do  udelay(1) ;
+
+	do {
+		if (!spin_is_locked(lelock))
+			conditional_schedule();
+		udelay(1) ;
+	}
 	while(inb(cmd_ioaddr) && --wait >= 0);
 #ifndef final_version
 	if (wait < 0)