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

[Wolfgang Denk]

Dear Heiko Schocher,

In message <498027A2.8060104 at 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