[PATCH] Additions to full lowlatency patch

[PATCH] Additions to full lowlatency patch

[Jussi Laako]

[-- Attachment #1: Type: text/plain, Size: 442 bytes --]

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

-- 
PGP key fingerprint: 161D 6FED 6A92 39E2 EB5B  39DD A4DE 63EB C216 1E4B
Available at PGP keyservers

[-- Attachment #2: all-ll.patch --]
[-- Type: text/plain, Size: 4791 bytes --]

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
--- linux-2.4.17-lowlatency/drivers/char/drm/mga_dma.c	Wed Aug  8 19:42:15 2001
+++ linux-2.4.17-lowlatency-jl/drivers/char/drm/mga_dma.c	Mon Dec 31 22:05:06 2001
@@ -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 );
 
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
--- linux-2.4.17-lowlatency/drivers/char/drm/r128_cce.c	Mon Aug 27 17:40:33 2001
+++ linux-2.4.17-lowlatency-jl/drivers/char/drm/r128_cce.c	Mon Dec 31 22:08:38 2001
@@ -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 );
 	}
 
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
--- linux-2.4.17-lowlatency/drivers/char/drm/radeon_cp.c	Sat Sep 15 00:29:41 2001
+++ linux-2.4.17-lowlatency-jl/drivers/char/drm/radeon_cp.c	Mon Dec 31 21:59:45 2001
@@ -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 );
 	}
 
diff -ur linux-2.4.17-lowlatency/drivers/net/3c59x.c linux-2.4.17-lowlatency-jl/drivers/net/3c59x.c
--- linux-2.4.17-lowlatency/drivers/net/3c59x.c	Fri Dec 21 19:41:54 2001
+++ linux-2.4.17-lowlatency-jl/drivers/net/3c59x.c	Mon Dec 31 22:31:10 2001
@@ -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",
diff -ur linux-2.4.17-lowlatency/drivers/net/8139too.c linux-2.4.17-lowlatency-jl/drivers/net/8139too.c
--- linux-2.4.17-lowlatency/drivers/net/8139too.c	Fri Dec 21 19:41:54 2001
+++ linux-2.4.17-lowlatency-jl/drivers/net/8139too.c	Mon Dec 31 22:33:00 2001
@@ -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))
diff -ur linux-2.4.17-lowlatency/drivers/net/eepro100.c linux-2.4.17-lowlatency-jl/drivers/net/eepro100.c
--- linux-2.4.17-lowlatency/drivers/net/eepro100.c	Fri Dec 21 19:41:54 2001
+++ linux-2.4.17-lowlatency-jl/drivers/net/eepro100.c	Mon Dec 31 22:41:42 2001
@@ -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)

Re: [PATCH] Additions to full lowlatency patch

[arjan]

In article <3C40AF23.18C811A8@kolumbus.fi> you wrote:
> --- linux-2.4.17-lowlatency/drivers/net/eepro100.c      Fri Dec 21 19:41:54 2001
> +++ linux-2.4.17-lowlatency-jl/drivers/net/eepro100.c   Mon Dec 31 22:41:42 2001
> @@ -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)

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

Re: [PATCH] Additions to full lowlatency patch

[Jussi Laako]

arjan@fenrus.demon.nl 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

-- 
PGP key fingerprint: 161D 6FED 6A92 39E2 EB5B  39DD A4DE 63EB C216 1E4B
Available at PGP keyservers

Re: [PATCH] Additions to full lowlatency patch

[Andrew Morton]

Jussi Laako 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.
> 

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.

-

Re: [PATCH] Additions to full lowlatency patch

[Jussi Laako]

Andrew Morton wrote:
> 
> 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.

> 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

-- 
PGP key fingerprint: 161D 6FED 6A92 39E2 EB5B  39DD A4DE 63EB C216 1E4B
Available at PGP keyservers

Re: [PATCH] Additions to full lowlatency patch

[Jussi Laako]

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

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

-- 
PGP key fingerprint: 161D 6FED 6A92 39E2 EB5B  39DD A4DE 63EB C216 1E4B
Available at PGP keyservers

[-- Attachment #2: all-ll-2.patch --]
[-- Type: text/plain, Size: 5024 bytes --]

diff -urN linux/drivers/char/drm/mga_dma.c linux-2.4.17-jl7-ll/drivers/char/drm/mga_dma.c
--- linux/drivers/char/drm/mga_dma.c	Wed Aug  8 19:42:15 2001
+++ linux-2.4.17-jl7-ll/drivers/char/drm/mga_dma.c	Sun Jan 13 02:07:13 2002
@@ -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 );
 
diff -urN linux/drivers/char/drm/r128_cce.c linux-2.4.17-jl7-ll/drivers/char/drm/r128_cce.c
--- linux/drivers/char/drm/r128_cce.c	Mon Aug 27 17:40:33 2001
+++ linux-2.4.17-jl7-ll/drivers/char/drm/r128_cce.c	Sun Jan 13 02:07:14 2002
@@ -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 );
 	}
 
diff -urN linux/drivers/char/drm/radeon_cp.c linux-2.4.17-jl7-ll/drivers/char/drm/radeon_cp.c
--- linux/drivers/char/drm/radeon_cp.c	Sat Sep 15 00:29:41 2001
+++ linux-2.4.17-jl7-ll/drivers/char/drm/radeon_cp.c	Sun Jan 13 02:07:14 2002
@@ -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 );
 	}
 
diff -urN linux/drivers/net/3c59x.c linux-2.4.17-jl7-ll/drivers/net/3c59x.c
--- linux/drivers/net/3c59x.c	Fri Dec 21 19:41:54 2001
+++ linux-2.4.17-jl7-ll/drivers/net/3c59x.c	Sun Jan 13 03:16:30 2002
@@ -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",
diff -urN linux/drivers/net/8139too.c linux-2.4.17-jl7-ll/drivers/net/8139too.c
--- linux/drivers/net/8139too.c	Fri Dec 21 19:41:54 2001
+++ linux-2.4.17-jl7-ll/drivers/net/8139too.c	Sun Jan 13 02:07:14 2002
@@ -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))
diff -urN linux/drivers/net/eepro100.c linux-2.4.17-jl7-ll/drivers/net/eepro100.c
--- linux/drivers/net/eepro100.c	Sun Jan 13 01:20:33 2002
+++ linux-2.4.17-jl7-ll/drivers/net/eepro100.c	Sun Jan 13 03:59:25 2002
@@ -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)

Re: [PATCH] Additions to full lowlatency patch

[Stephan von Krawczynski]

On Sat, 12 Jan 2002 23:48:19 +0200
Jussi Laako <jussi.laako@kolumbus.fi> 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