[U-Boot] netloop: speed up NetLoop

[U-Boot] netloop: speed up NetLoop

[Heiko Schocher]

NetLoop polls every cycle with getenv some environment variables.
This is horribly slow, especially when the environment is big.

This patch reads only the environment variables in NetLoop,
when they were changed.

Also moved the init part of the NetLoop function in a seperate
function.

Signed-off-by: Heiko Schocher <hs@denx.de>
---

changes since first patch:
- added comments from Wolfgang Denk
- added comments from Kim Philips without:
  I keep get_env_id (), because it looks better so to me

 common/cmd_nvedit.c |    6 ++
 include/common.h    |    1 +
 net/eth.c           |   11 ++++-
 net/net.c           |  134 ++++++++++++++++++++++++++++----------------------
 4 files changed, 91 insertions(+), 61 deletions(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 1fcb4c9..9070b72 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -75,7 +75,12 @@ DECLARE_GLOBAL_DATA_PTR;
 static const unsigned long baudrate_table[] = CONFIG_SYS_BAUDRATE_TABLE;
 #define	N_BAUDRATES (sizeof(baudrate_table) / sizeof(baudrate_table[0]))

+static int env_id = 1;

+int get_env_id (void)
+{
+	return env_id;
+}
 /************************************************************************
  * Command interface: print one or all environment variables
  */
@@ -160,6 +165,7 @@ int _do_setenv (int flag, int argc, char *argv[])
 		return 1;
 	}

+	env_id++;
 	/*
 	 * search if variable with this name already exists
 	 */
diff --git a/include/common.h b/include/common.h
index afee188..b75ea60 100644
--- a/include/common.h
+++ b/include/common.h
@@ -269,6 +269,7 @@ void	forceenv     (char *, char *);
 #ifdef CONFIG_AUTO_COMPLETE
 int env_complete(char *var, int maxv, char *cmdv[], int maxsz, char *buf);
 #endif
+int get_env_id (void);

 void	pci_init      (void);
 void	pci_init_board(void);
diff --git a/net/eth.c b/net/eth.c
index b7ef09f..ff5bd85 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -28,6 +28,9 @@

 #if defined(CONFIG_CMD_NET) && defined(CONFIG_NET_MULTI)

+static char *act = NULL;
+static int  env_changed_id = 0;
+
 /*
  * CPU and board-specific Ethernet initializations.  Aliased function
  * signals caller to move on
@@ -439,13 +442,17 @@ void eth_try_another(int first_restart)
 #ifdef CONFIG_NET_MULTI
 void eth_set_current(void)
 {
-	char *act;
 	struct eth_device* old_current;
+	int	env_id;

 	if (!eth_current)	/* XXX no current */
 		return;

-	act = getenv("ethact");
+	env_id = get_env_id();
+	if ((act == NULL) || (env_changed_id != env_id)) {
+		act = getenv("ethact");
+		env_changed_id = env_id;
+	}
 	if (act != NULL) {
 		old_current = eth_current;
 		do {
diff --git a/net/net.c b/net/net.c
index e6547f9..797d156 100644
--- a/net/net.c
+++ b/net/net.c
@@ -209,6 +209,8 @@ uchar		NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN];
 ulong		NetArpWaitTimerStart;
 int		NetArpWaitTry;

+int		env_changed_id = 0;
+
 void ArpRequest (void)
 {
 	int i;
@@ -276,6 +278,78 @@ void ArpTimeoutCheck(void)
 	}
 }

+int
+NetInitLoop(proto_t protocol)
+{
+	bd_t *bd = gd->bd;
+	int env_id = get_env_id ();
+
+	/* update only when the environment has changed */
+	if (env_changed_id == env_id)
+		return 0;
+
+	switch (protocol) {
+#if defined(CONFIG_CMD_NFS)
+	case NFS:
+#endif
+#if defined(CONFIG_CMD_PING)
+	case PING:
+#endif
+#if defined(CONFIG_CMD_SNTP)
+	case SNTP:
+#endif
+	case NETCONS:
+	case TFTP:
+		NetCopyIP(&NetOurIP, &bd->bi_ip_addr);
+		NetOurGatewayIP = getenv_IPaddr ("gatewayip");
+		NetOurSubnetMask= getenv_IPaddr ("netmask");
+		NetOurVLAN = getenv_VLAN("vlan");
+		NetOurNativeVLAN = getenv_VLAN("nvlan");
+
+		switch (protocol) {
+#if defined(CONFIG_CMD_NFS)
+		case NFS:
+#endif
+		case NETCONS:
+		case TFTP:
+			NetServerIP = getenv_IPaddr ("serverip");
+			break;
+#if defined(CONFIG_CMD_PING)
+		case PING:
+			/* nothing */
+			break;
+#endif
+#if defined(CONFIG_CMD_SNTP)
+		case SNTP:
+			/* nothing */
+			break;
+#endif
+		default:
+			break;
+		}
+
+		break;
+	case BOOTP:
+	case RARP:
+		/*
+		 * initialize our IP addr to 0 in order to accept ANY
+		 * IP addr assigned to us by the BOOTP / RARP server
+		 */
+		NetOurIP = 0;
+		NetServerIP = getenv_IPaddr ("serverip");
+		NetOurVLAN = getenv_VLAN("vlan");	/* VLANs must be read */
+		NetOurNativeVLAN = getenv_VLAN("nvlan");
+	case CDP:
+		NetOurVLAN = getenv_VLAN("vlan");	/* VLANs must be read */
+		NetOurNativeVLAN = getenv_VLAN("nvlan");
+		break;
+	default:
+		break;
+	}
+	env_changed_id = env_id;
+	return 0;
+}
+
 /**********************************************************************/
 /*
  *	Main network processing loop.
@@ -340,65 +414,7 @@ restart:
 	 *	here on, this code is a state machine driven by received
 	 *	packets and timer events.
 	 */
-
-	switch (protocol) {
-#if defined(CONFIG_CMD_NFS)
-	case NFS:
-#endif
-#if defined(CONFIG_CMD_PING)
-	case PING:
-#endif
-#if defined(CONFIG_CMD_SNTP)
-	case SNTP:
-#endif
-	case NETCONS:
-	case TFTP:
-		NetCopyIP(&NetOurIP, &bd->bi_ip_addr);
-		NetOurGatewayIP = getenv_IPaddr ("gatewayip");
-		NetOurSubnetMask= getenv_IPaddr ("netmask");
-		NetOurVLAN = getenv_VLAN("vlan");
-		NetOurNativeVLAN = getenv_VLAN("nvlan");
-
-		switch (protocol) {
-#if defined(CONFIG_CMD_NFS)
-		case NFS:
-#endif
-		case NETCONS:
-		case TFTP:
-			NetServerIP = getenv_IPaddr ("serverip");
-			break;
-#if defined(CONFIG_CMD_PING)
-		case PING:
-			/* nothing */
-			break;
-#endif
-#if defined(CONFIG_CMD_SNTP)
-		case SNTP:
-			/* nothing */
-			break;
-#endif
-		default:
-			break;
-		}
-
-		break;
-	case BOOTP:
-	case RARP:
-		/*
-		 * initialize our IP addr to 0 in order to accept ANY
-		 * IP addr assigned to us by the BOOTP / RARP server
-		 */
-		NetOurIP = 0;
-		NetServerIP = getenv_IPaddr ("serverip");
-		NetOurVLAN = getenv_VLAN("vlan");	/* VLANs must be read */
-		NetOurNativeVLAN = getenv_VLAN("nvlan");
-	case CDP:
-		NetOurVLAN = getenv_VLAN("vlan");	/* VLANs must be read */
-		NetOurNativeVLAN = getenv_VLAN("nvlan");
-		break;
-	default:
-		break;
-	}
+	NetInitLoop(protocol);

 	switch (net_check_prereq (protocol)) {
 	case 1:
-- 
1.6.0.6

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] netloop: speed up NetLoop

[Wolfgang Denk]

Dear Ben,

In message <49913D1C.6090905@denx.de> Heiko Schocher wrote:
> NetLoop polls every cycle with getenv some environment variables.
> This is horribly slow, especially when the environment is big.
> 
> This patch reads only the environment variables in NetLoop,
> when they were changed.
> 
> Also moved the init part of the NetLoop function in a seperate
> function.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
> 
> changes since first patch:
> - added comments from Wolfgang Denk
> - added comments from Kim Philips without:
>   I keep get_env_id (), because it looks better so to me
> 
>  common/cmd_nvedit.c |    6 ++
>  include/common.h    |    1 +
>  net/eth.c           |   11 ++++-
>  net/net.c           |  134 ++++++++++++++++++++++++++++----------------------
>  4 files changed, 91 insertions(+), 61 deletions(-)

What about this patch? Will it go into v2009.03 ?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
And now remains  That we find out the cause of this effect, Or rather
say, the cause of this defect...           -- Hamlet, Act II, Scene 2

[U-Boot] netloop: speed up NetLoop

[Ben Warren]

Heiko Schocher wrote:
> NetLoop polls every cycle with getenv some environment variables.
> This is horribly slow, especially when the environment is big.
>
> This patch reads only the environment variables in NetLoop,
> when they were changed.
>
> Also moved the init part of the NetLoop function in a seperate
> function.
>
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
>   
<snip>
Applied.

thanks,
Ben

[U-Boot] netloop: speed up NetLoop

[Mike Frysinger]

On Tuesday 10 February 2009 03:38:52 Heiko Schocher wrote:
> NetLoop polls every cycle with getenv some environment variables.
> This is horribly slow, especially when the environment is big.
>
> This patch reads only the environment variables in NetLoop,
> when they were changed.
>
> Also moved the init part of the NetLoop function in a seperate
> function.

a bit late, but oh well ...

> --- a/common/cmd_nvedit.c
> +++ b/common/cmd_nvedit.c
> +static int env_id = 1;
>
> +int get_env_id (void)
> +{
> +	return env_id;
> +}

there's no documentation anywhere in the source about these new functions.  
people have to read the changelog to figure out what the function is for and 
how it is used.  at least to me, the usage is not conveyed in any way by the 
function name.  a simple comment block right above these would go a long way.

> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -28,6 +28,9 @@
>
>  #if defined(CONFIG_CMD_NET) && defined(CONFIG_NET_MULTI)
>
> +static char *act = NULL;
> +static int  env_changed_id = 0;

since these are only used by eth_set_current(), shouldnt they be in that 
function instead of file scope ?  nothing else in eth.c uses them.

> --- a/net/net.c
> +++ b/net/net.c
> @@ -209,6 +209,8 @@ uchar		NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN];
> +int		env_changed_id = 0;

why isnt marked static ?  and since only NetInitLoop() uses it, it should be 
in that function rather than file scope.

> +int
> +NetInitLoop(proto_t protocol)
> +{

this function always returns 0, and the only caller is in this file, and it 
doesnt actually check the return value.  so this function should be marked 
static and changed to void.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090224/7038770c/attachment.pgp 

[U-Boot] netloop: speed up NetLoop

[Mike Frysinger]

On Tuesday 10 February 2009 03:38:52 Heiko Schocher wrote:
> +int NetInitLoop(proto_t protocol)

and some comments not really specific to your code ...

> +		switch (protocol) {
> +#if defined(CONFIG_CMD_NFS)
> +		case NFS:
> +#endif
> +		case NETCONS:
> +		case TFTP:
> +			NetServerIP = getenv_IPaddr ("serverip");
> +			break;
> +#if defined(CONFIG_CMD_PING)
> +		case PING:
> +			/* nothing */
> +			break;
> +#endif
> +#if defined(CONFIG_CMD_SNTP)
> +		case SNTP:
> +			/* nothing */
> +			break;
> +#endif
> +		default:
> +			break;
> +		}

am i missing something, or are the PING/SNTP parts here completely useless

> +	case BOOTP:
> +	case RARP:
> +		/*
> +		 * initialize our IP addr to 0 in order to accept ANY
> +		 * IP addr assigned to us by the BOOTP / RARP server
> +		 */
> +		NetOurIP = 0;
> +		NetServerIP = getenv_IPaddr ("serverip");
> +		NetOurVLAN = getenv_VLAN("vlan");	/* VLANs must be read */
> +		NetOurNativeVLAN = getenv_VLAN("nvlan");
> +	case CDP:
> +		NetOurVLAN = getenv_VLAN("vlan");	/* VLANs must be read */
> +		NetOurNativeVLAN = getenv_VLAN("nvlan");
> +		break;

looks to me like BOOTP/RARP are pointlessly reading/setting NetOurVLAN and 
NetOurNativeVLAN twice ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090224/bc67484c/attachment.pgp 

[U-Boot] netloop: speed up NetLoop

[Heiko Schocher]

Hello Mike,

Mike Frysinger wrote:
> On Tuesday 10 February 2009 03:38:52 Heiko Schocher wrote:
>> NetLoop polls every cycle with getenv some environment variables.
>> This is horribly slow, especially when the environment is big.
>>
>> This patch reads only the environment variables in NetLoop,
>> when they were changed.
>>
>> Also moved the init part of the NetLoop function in a seperate
>> function.
> 
> a bit late, but oh well ...

better late then never ;-)

>> --- a/common/cmd_nvedit.c
>> +++ b/common/cmd_nvedit.c
>> +static int env_id = 1;
>>
>> +int get_env_id (void)
>> +{
>> +	return env_id;
>> +}
> 
> there's no documentation anywhere in the source about these new functions.  
> people have to read the changelog to figure out what the function is for and 
> how it is used.  at least to me, the usage is not conveyed in any way by the 
> function name.  a simple comment block right above these would go a long way.

Ok, I try to add a comment.

>> --- a/net/eth.c
>> +++ b/net/eth.c
>> @@ -28,6 +28,9 @@
>>
>>  #if defined(CONFIG_CMD_NET) && defined(CONFIG_NET_MULTI)
>>
>> +static char *act = NULL;
>> +static int  env_changed_id = 0;
> 
> since these are only used by eth_set_current(), shouldnt they be in that 
> function instead of file scope ?  nothing else in eth.c uses them.

Yes, you are right.

>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -209,6 +209,8 @@ uchar		NetArpWaitPacketBuf[PKTSIZE_ALIGN + PKTALIGN];
>> +int		env_changed_id = 0;
> 
> why isnt marked static ?  and since only NetInitLoop() uses it, it should be 
> in that function rather than file scope.

Yep.

>> +int
>> +NetInitLoop(proto_t protocol)
>> +{
> 
> this function always returns 0, and the only caller is in this file, and it 
> doesnt actually check the return value.  so this function should be marked 
> static and changed to void.
> -mike

OK, I sent a patch for this.

thanks
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] netloop: speed up NetLoop

[Heiko Schocher]

Hello Mike,

Mike Frysinger wrote:
> On Tuesday 10 February 2009 03:38:52 Heiko Schocher wrote:
>> +int NetInitLoop(proto_t protocol)
> 
> and some comments not really specific to your code ...
> 
>> +		switch (protocol) {
>> +#if defined(CONFIG_CMD_NFS)
>> +		case NFS:
>> +#endif
>> +		case NETCONS:
>> +		case TFTP:
>> +			NetServerIP = getenv_IPaddr ("serverip");
>> +			break;
>> +#if defined(CONFIG_CMD_PING)
>> +		case PING:
>> +			/* nothing */
>> +			break;
>> +#endif
>> +#if defined(CONFIG_CMD_SNTP)
>> +		case SNTP:
>> +			/* nothing */
>> +			break;
>> +#endif
>> +		default:
>> +			break;
>> +		}
> 
> am i missing something, or are the PING/SNTP parts here completely useless

Yes, seems so, but I am not a net expert ... but I think it is not needed, so
I fix this too when I am fixing the other issues you mentioned.

>> +	case BOOTP:
>> +	case RARP:
>> +		/*
>> +		 * initialize our IP addr to 0 in order to accept ANY
>> +		 * IP addr assigned to us by the BOOTP / RARP server
>> +		 */
>> +		NetOurIP = 0;
>> +		NetServerIP = getenv_IPaddr ("serverip");
>> +		NetOurVLAN = getenv_VLAN("vlan");	/* VLANs must be read */
>> +		NetOurNativeVLAN = getenv_VLAN("nvlan");
>> +	case CDP:
>> +		NetOurVLAN = getenv_VLAN("vlan");	/* VLANs must be read */
>> +		NetOurNativeVLAN = getenv_VLAN("nvlan");
>> +		break;
> 
> looks to me like BOOTP/RARP are pointlessly reading/setting NetOurVLAN and 
> NetOurNativeVLAN twice ...

Yep, I fix this also.

thanks
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany