[U-Boot] [PATCH 03/31] netloop: speed up NetLoop

[U-Boot] [PATCH 03/31] 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.

Signed-off-by: Heiko Schocher <hs@denx.de>
---
 common/cmd_nvedit.c |    6 +++
 include/common.h    |    1 +
 net/eth.c           |   11 +++++-
 net/net.c           |   88 +++++++++++++++++++++++++++-----------------------
 4 files changed, 63 insertions(+), 43 deletions(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 8f6310a..9218945 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
  */
@@ -168,6 +173,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 5968036..85ee090 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..d1d73cb 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -28,7 +28,11 @@

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

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

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

-	act = getenv("ethact");
+	if ((*act == 0) || (env_changed_id < get_env_id()))
+	{
+		act = getenv("ethact");
+		env_changed_id = get_env_id();
+	}
 	if (act != NULL) {
 		old_current = eth_current;
 		do {
diff --git a/net/net.c b/net/net.c
index e6547f9..eca149c 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;
@@ -341,63 +343,67 @@ restart:
 	 *	packets and timer events.
 	 */

-	switch (protocol) {
+	/* update just, if the environment has changed */
+	if (env_changed_id < get_env_id ()) {
+		switch (protocol) {
 #if defined(CONFIG_CMD_NFS)
-	case NFS:
+		case NFS:
 #endif
 #if defined(CONFIG_CMD_PING)
-	case PING:
+		case PING:
 #endif
 #if defined(CONFIG_CMD_SNTP)
-	case 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");
+		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) {
+			switch (protocol) {
 #if defined(CONFIG_CMD_NFS)
-		case NFS:
+			case NFS:
 #endif
-		case NETCONS:
-		case TFTP:
-			NetServerIP = getenv_IPaddr ("serverip");
-			break;
+			case NETCONS:
+			case TFTP:
+				NetServerIP = getenv_IPaddr ("serverip");
+				break;
 #if defined(CONFIG_CMD_PING)
-		case PING:
-			/* nothing */
-			break;
+			case PING:
+				/* nothing */
+				break;
 #endif
 #if defined(CONFIG_CMD_SNTP)
-		case SNTP:
-			/* nothing */
-			break;
+			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;
 		}
-
-		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 = get_env_id ();
 	}

 	switch (net_check_prereq (protocol)) {
-- 
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] [PATCH 03/31] netloop: speed up NetLoop

[Wolfgang Denk]

Dear Heiko Schocher,

In message <498027A2.8060104@denx.de> you 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.
...
> 
> -	act = getenv("ethact");
> +	if ((*act == 0) || (env_changed_id < get_env_id()))
> +	{
> +		act = getenv("ethact");
> +		env_changed_id = get_env_id();
> +	}

Incorrect brace style.

Note that you are calling get_env_id() twice - in the test and then
again. This can be avoided. Also, I recommend to use "!=" instead of
"<".

> --- a/net/net.c
> +++ b/net/net.c
...
> +int		env_changed_id = 0;
> +
>  void ArpRequest (void)
>  {
>  	int i;
> @@ -341,63 +343,67 @@ restart:
>  	 *	packets and timer events.
>  	 */
> 
> -	switch (protocol) {
> +	/* update just, if the environment has changed */
> +	if (env_changed_id < get_env_id ()) {
> +		switch (protocol) {

Please change the comment into something like "update only when the
environment has changed" or so.

Better use "!=" instead of "<".

>  #if defined(CONFIG_CMD_NFS)
> -	case NFS:
> +		case NFS:
>  #endif
>  #if defined(CONFIG_CMD_PING)
> -	case PING:
> +		case PING:
>  #endif
>  #if defined(CONFIG_CMD_SNTP)
> -	case 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");
> +		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) {
> +			switch (protocol) {
>  #if defined(CONFIG_CMD_NFS)
> -		case NFS:
> +			case NFS:
>  #endif
> -		case NETCONS:
> -		case TFTP:
> -			NetServerIP = getenv_IPaddr ("serverip");
> -			break;
> +			case NETCONS:
> +			case TFTP:
> +				NetServerIP = getenv_IPaddr ("serverip");
> +				break;
>  #if defined(CONFIG_CMD_PING)
> -		case PING:
> -			/* nothing */
> -			break;
> +			case PING:
> +				/* nothing */
> +				break;
>  #endif
>  #if defined(CONFIG_CMD_SNTP)
> -		case SNTP:
> -			/* nothing */
> -			break;
> +			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;
>  		}
> -
> -		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 = get_env_id ();

You are calling get_env_id() twice - avoid that.

Instead of adding yet anothe rlevel of indentation to that switch I
recommend to split it in a separate fuinction - it's big enough for
that anyway.

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
Here is an Appalachian version of management's answer  to  those  who
are  concerned  with  the fate of the project: "Don't worry about the
mule. Just load the wagon."         - Mike Dennison's hillbilly uncle

[U-Boot] [PATCH 03/31] netloop: speed up NetLoop

[Heiko Schocher]

Hello Wolfgang,

Wolfgang Denk wrote:
> In message <498027A2.8060104@denx.de> you 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.
> ...
>> -	act = getenv("ethact");
>> +	if ((*act == 0) || (env_changed_id < get_env_id()))
>> +	{
>> +		act = getenv("ethact");
>> +		env_changed_id = get_env_id();
>> +	}
> 
> Incorrect brace style.

Argh.

> Note that you are calling get_env_id() twice - in the test and then
> again. This can be avoided. Also, I recommend to use "!=" instead of
> "<".

OK, I change this.

>> --- a/net/net.c
>> +++ b/net/net.c
> ...
>> +int		env_changed_id = 0;
>> +
>>  void ArpRequest (void)
>>  {
>>  	int i;
>> @@ -341,63 +343,67 @@ restart:
>>  	 *	packets and timer events.
>>  	 */
>>
>> -	switch (protocol) {
>> +	/* update just, if the environment has changed */
>> +	if (env_changed_id < get_env_id ()) {
>> +		switch (protocol) {
> 
> Please change the comment into something like "update only when the
> environment has changed" or so.
> 
> Better use "!=" instead of "<".

OK.

>>  #if defined(CONFIG_CMD_NFS)
>> -	case NFS:
>> +		case NFS:
>>  #endif
>>  #if defined(CONFIG_CMD_PING)
>> -	case PING:
>> +		case PING:
>>  #endif
>>  #if defined(CONFIG_CMD_SNTP)
>> -	case 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");
>> +		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) {
>> +			switch (protocol) {
>>  #if defined(CONFIG_CMD_NFS)
>> -		case NFS:
>> +			case NFS:
>>  #endif
>> -		case NETCONS:
>> -		case TFTP:
>> -			NetServerIP = getenv_IPaddr ("serverip");
>> -			break;
>> +			case NETCONS:
>> +			case TFTP:
>> +				NetServerIP = getenv_IPaddr ("serverip");
>> +				break;
>>  #if defined(CONFIG_CMD_PING)
>> -		case PING:
>> -			/* nothing */
>> -			break;
>> +			case PING:
>> +				/* nothing */
>> +				break;
>>  #endif
>>  #if defined(CONFIG_CMD_SNTP)
>> -		case SNTP:
>> -			/* nothing */
>> -			break;
>> +			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;
>>  		}
>> -
>> -		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 = get_env_id ();
> 
> You are calling get_env_id() twice - avoid that.

OK, I try it.

> Instead of adding yet anothe rlevel of indentation to that switch I
> recommend to split it in a separate fuinction - it's big enough for
> that anyway.

OK, will change this.

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

[U-Boot] [PATCH 03/31] netloop: speed up NetLoop

[Kim Phillips]

On Wed, 28 Jan 2009 10:38:42 +0100
Heiko Schocher <hs@denx.de> wrote:

> +++ 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;
> +}

does getting env_id really need be a function?

> diff --git a/net/eth.c b/net/eth.c
> index b7ef09f..d1d73cb 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -28,7 +28,11 @@
> 
>  #if defined(CONFIG_CMD_NET) && defined(CONFIG_NET_MULTI)
> 
> +static char *act = 0;
> +static int  env_changed_id = 0;
> +
>  /*
> +*

please don't mess with the already good comment coding style.

>   * CPU and board-specific Ethernet initializations.  Aliased function
>   * signals caller to move on
>   */
> @@ -439,13 +443,16 @@ void eth_try_another(int first_restart)
>  #ifdef CONFIG_NET_MULTI
>  void eth_set_current(void)
>  {
> -	char *act;
>  	struct eth_device* old_current;
> 
>  	if (!eth_current)	/* XXX no current */
>  		return;
> 
> -	act = getenv("ethact");
> +	if ((*act == 0) || (env_changed_id < get_env_id()))

If I'm not mistaken, this will unintentionally dereference address 0
the first time this is executed.

> +	{
> +		act = getenv("ethact");
> +		env_changed_id = get_env_id();
> +	}
>  	if (act != NULL) {
>  		old_current = eth_current;
>  		do {

Kim

[U-Boot] [PATCH 03/31] netloop: speed up NetLoop

[Heiko Schocher]

Hello Kim,

Kim Phillips:
> On Wed, 28 Jan 2009 10:38:42 +0100
> Heiko Schocher <hs@denx.de> wrote:
>> +++ 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;
>> +}
> 
> does getting env_id really need be a function?

not absolute, but accesing this var from another file by a
function seems better to me.

>> diff --git a/net/eth.c b/net/eth.c
>> index b7ef09f..d1d73cb 100644
>> --- a/net/eth.c
>> +++ b/net/eth.c
>> @@ -28,7 +28,11 @@
>>
>>  #if defined(CONFIG_CMD_NET) && defined(CONFIG_NET_MULTI)
>>
>> +static char *act = 0;
>> +static int  env_changed_id = 0;
>> +
>>  /*
>> +*
> 
> please don't mess with the already good comment coding style.

Argh, sorry.

>>   * CPU and board-specific Ethernet initializations.  Aliased function
>>   * signals caller to move on
>>   */
>> @@ -439,13 +443,16 @@ void eth_try_another(int first_restart)
>>  #ifdef CONFIG_NET_MULTI
>>  void eth_set_current(void)
>>  {
>> -	char *act;
>>  	struct eth_device* old_current;
>>
>>  	if (!eth_current)	/* XXX no current */
>>  		return;
>>
>> -	act = getenv("ethact");
>> +	if ((*act == 0) || (env_changed_id < get_env_id()))
> 
> If I'm not mistaken, this will unintentionally dereference address 0
> the first time this is executed.

Hmm.. must think about it.

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