netpoll/netconsole minor tweaks

netpoll/netconsole minor tweaks

[Chris Wright]

Hi Matt,

Here's a couple small tweaks.  The first is to netpoll_setup.  The settle
time was too short for my e100, and the system would hang.  The second
is to netconsole so that it registers a console with CON_PRINTBUFFER.
This helps debugging early bootup issues where you want to capture data
from before netconsole is initialized.  Perhaps it should be a param
to netconsole?

thanks,
-chris

--- 2.6.0-test5-mm2/net/core/netpoll.c.wait_fix	2003-09-15 15:46:28.000000000 -0700
+++ 2.6.0-test5-mm2/net/core/netpoll.c	2003-09-15 16:14:20.000000000 -0700
@@ -526,7 +526,7 @@ int netpoll_setup(struct netpoll *np)
 		rtnl_shunlock();
 
 		/* Give driver a chance to settle */
-		jiff = jiffies + 2*HZ;
+		jiff = jiffies + 4*HZ;
 		while (time_before(jiffies, jiff))
 			;
 	}

--- 2.6.0-test5-mm2/drivers/net/netconsole.c.print_buf	2003-09-15 16:21:31.000000000 -0700
+++ 2.6.0-test5-mm2/drivers/net/netconsole.c	2003-09-17 11:09:59.000000000 -0700
@@ -95,7 +95,7 @@
 }
 
 static struct console netconsole = {
-	.flags = CON_ENABLED,
+	.flags = CON_ENABLED | CON_PRINTBUFFER,
 	.write = write_msg
 };

Re: netpoll/netconsole minor tweaks

[Matt Mackall]

On Wed, Sep 17, 2003 at 11:24:47AM -0700, Chris Wright wrote:
> Hi Matt,
> 
> Here's a couple small tweaks.  The first is to netpoll_setup.  The settle
> time was too short for my e100, and the system would hang.  

This probably ought to be a command line arg, my tg3 apparently
doesn't need it at all. One second is what's in the nfs-root stuff,
perhaps they need to share an arg, I changed it to two because my tlan
was dropping stuff (but not hanging - that may be a driver bug).
Ideally, I'd like to find a way to wait until the damn thing is up
before sending.

net/ipv4/ipconfig.c:
 /* Define the friendly delay before and after opening net devices */
 #define CONF_PRE_OPEN           (HZ/2)  /* Before opening: 1/2 second */
 #define CONF_POST_OPEN          (1*HZ)  /* After opening: 1 second */
 
Anyway, I'm still struggling with getting stuff working on my Opteron
box, care to take a stab at it?

> The second
> is to netconsole so that it registers a console with CON_PRINTBUFFER.
> This helps debugging early bootup issues where you want to capture data
> from before netconsole is initialized.  Perhaps it should be a param
> to netconsole?

I think this can probably be unconditional. Merged.

-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon

Re: netpoll/netconsole minor tweaks

[Chris Wright]

* Matt Mackall (mpm@selenic.com) wrote:
> On Wed, Sep 17, 2003 at 11:24:47AM -0700, Chris Wright wrote:
> > Here's a couple small tweaks.  The first is to netpoll_setup.  The settle
> > time was too short for my e100, and the system would hang.  
> 
> This probably ought to be a command line arg, my tg3 apparently
> doesn't need it at all. One second is what's in the nfs-root stuff,
> perhaps they need to share an arg, I changed it to two because my tlan
> was dropping stuff (but not hanging - that may be a driver bug).
> Ideally, I'd like to find a way to wait until the damn thing is up
> before sending.

Yeah, I agree, that seems best.

> net/ipv4/ipconfig.c:
>  /* Define the friendly delay before and after opening net devices */
>  #define CONF_PRE_OPEN           (HZ/2)  /* Before opening: 1/2 second */
>  #define CONF_POST_OPEN          (1*HZ)  /* After opening: 1 second */
>  
> Anyway, I'm still struggling with getting stuff working on my Opteron
> box, care to take a stab at it?

Sure, I'll poke at it tomorrow, unless you get to it first.

> > The second
> > is to netconsole so that it registers a console with CON_PRINTBUFFER.
> > This helps debugging early bootup issues where you want to capture data
> > from before netconsole is initialized.  Perhaps it should be a param
> > to netconsole?
> 
> I think this can probably be unconditional. Merged.

Ok, cool.
thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: netpoll/netconsole minor tweaks

[Alan Cox]

On Mer, 2003-09-17 at 19:24, Chris Wright wrote: 
>  		/* Give driver a chance to settle */
> -		jiff = jiffies + 2*HZ;
> +		jiff = jiffies + 4*HZ;
>  		while (time_before(jiffies, jiff))
<pedant>
>  			;
should be cpu_relax();
</pedant>

Re: netpoll/netconsole minor tweaks

[Chris Wright]

* Alan Cox (alan@lxorguk.ukuu.org.uk) wrote:
> On Mer, 2003-09-17 at 19:24, Chris Wright wrote: 
> >  		/* Give driver a chance to settle */
> > -		jiff = jiffies + 2*HZ;
> > +		jiff = jiffies + 4*HZ;
> >  		while (time_before(jiffies, jiff))
> <pedant>
> should be cpu_relax();
> </pedant>

Hrm, there's many spots that aren't using it.  What's the benefit, less
power consumption?  Is it worth a patch to convert other things over?

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: netpoll/netconsole minor tweaks

[Arjan van de Ven]

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


> Hrm, there's many spots that aren't using it.  What's the benefit, less
> power consumption? 

Less power consumption, and on HT/SMT CPU's it's a "yield" to the other
half/halves.
>
>  Is it worth a patch to convert other things over?
> 

yes


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: netpoll/netconsole minor tweaks

[Chris Wright]

* Arjan van de Ven (arjanv@redhat.com) wrote:
> Less power consumption, and on HT/SMT CPU's it's a "yield" to the other
> half/halves.

I see, thanks.

> >  Is it worth a patch to convert other things over?
> yes

OK, I'll spin one up, thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: netpoll/netconsole minor tweaks

[Alan Cox]

On Iau, 2003-09-18 at 17:48, Chris Wright wrote:
> > <pedant>
> > should be cpu_relax();
> > </pedant>
> 
> Hrm, there's many spots that aren't using it.  What's the benefit, less
> power consumption?  Is it worth a patch to convert other things over?

It speeds up hyperthreading CPUs by letting them know that this
paticular thread is waiting for stuff (sched_yield for silicon)

Re: netpoll/netconsole minor tweaks

[Jesper Juhl]

On Thu, 18 Sep 2003, Alan Cox wrote:

> On Iau, 2003-09-18 at 17:48, Chris Wright wrote:
> > > <pedant>
> > > should be cpu_relax();
> > > </pedant>
> >
> > Hrm, there's many spots that aren't using it.  What's the benefit, less
> > power consumption?  Is it worth a patch to convert other things over?
>
> It speeds up hyperthreading CPUs by letting them know that this
> paticular thread is waiting for stuff (sched_yield for silicon)
>

Does that mean that it would be benneficial to do something like this in
for example eepro100.c ??

diff -up linux-2.6.0-test5-orig/drivers/net/eepro100.c
linux-2.6.0-test5/drivers/net/eepro100.c
--- linux-2.6.0-test5-orig/drivers/net/eepro100.c       2003-09-08 21:50:09.000000000 +0200
+++ linux-2.6.0-test5/drivers/net/eepro100.c    2003-09-19 01:03:19.000000000 +0200
@@ -913,9 +913,11 @@ static void do_slow_command(struct net_d

        for (wait = 0; wait <= 100; wait++)
                if (inb(cmd_ioaddr) == 0) return;
-       for (; wait <= 20000; wait++)
+       for (; wait <= 20000; wait++) {
                if (inb(cmd_ioaddr) == 0) return;
                else udelay(1);
+               cpu_relax();
+       }
        printk(KERN_ERR "Command %4.4x was not accepted after %d polls!"
               "  Current status %8.8x.\n",
               cmd, wait, inl(dev->base_addr + SCBStatus));



or maybe even take it to the extreme like


diff -up linux-2.6.0-test5-orig/drivers/net/eepro100.c
linux-2.6.0-test5/drivers/net/eepro100.c
--- linux-2.6.0-test5-orig/drivers/net/eepro100.c       2003-09-08 21:50:09.000000000 +0200
+++ linux-2.6.0-test5/drivers/net/eepro100.c    2003-09-19 01:07:50.000000000 +0200
@@ -902,20 +902,25 @@ static void do_slow_command(struct net_d
 {
        long cmd_ioaddr = dev->base_addr + SCBCmd;
        int wait = 0;
-       do
+       do {
                if (inb(cmd_ioaddr) == 0) break;
-       while(++wait <= 200);
+               cpu_relax();
+       } while(++wait <= 200);
        if (wait > 100)
                printk(KERN_ERR "Command %4.4x never accepted (%d
polls)!\n",
                       inb(cmd_ioaddr), wait);

        outb(cmd, cmd_ioaddr);

-       for (wait = 0; wait <= 100; wait++)
+       for (wait = 0; wait <= 100; wait++) {
                if (inb(cmd_ioaddr) == 0) return;
-       for (; wait <= 20000; wait++)
+               cpu_relax();
+       }
+       for (; wait <= 20000; wait++) {
                if (inb(cmd_ioaddr) == 0) return;
                else udelay(1);
+               cpu_relax();
+       }
        printk(KERN_ERR "Command %4.4x was not accepted after %d polls!"
               "  Current status %8.8x.\n",
               cmd, wait, inl(dev->base_addr + SCBStatus));


How short/long waits are we talking about before it is benneficial to be
calling cpu_relax() ?


Kind regards,

Jesper Juhl <jju@dif.dk>

Re: netpoll/netconsole minor tweaks

[Arjan van de Ven]

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

On Fri, 2003-09-19 at 01:10, Jesper Juhl wrote:
> 00; wait++)
>                 if (inb(cmd_ioaddr) == 0) return;
> -       for (; wait <= 20000; wait++)
> +       for (; wait <= 20000; wait++) {
>                 if (inb(cmd_ioaddr) == 0) return;
>                 else udelay(1);
> +               cpu_relax();
> +       }

udelay() should have cpu_relax() inside it already basically..

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]