From mboxrd@z Thu Jan  1 00:00:00 1970
From: Heiko Schocher <hs@denx.de>
Date: Wed, 28 Jan 2009 12:55:30 +0100
Subject: [U-Boot] [PATCH 03/31] netloop: speed up NetLoop
In-Reply-To: <20090128114238.A34BC832E416@gemini.denx.de>
References: <498027A2.8060104@denx.de>
	<20090128114238.A34BC832E416@gemini.denx.de>
Message-ID: <498047B2.1020109@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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