Re: [PATCH 2/3] net: bootp: BOOTP/DHCPv4 retransmission improvements

[Sean Edmond <seanedmond@linux.microsoft.com>]

On 2023-09-26 9:11 p.m., Lothar Waßmann wrote:
> Hi,
>
> On Mon, 25 Sep 2023 13:29:34 -0700 seanedmond@linux.microsoft.com wrote:
>> From: Sean Edmond <seanedmond@microsoft.com>
>>
>> This patch introduces 3 improvements to align with RFC 951:
>> - retransmission backoff interval maximum is configurable
>> - initial retranmission backoff interval is configurable
>> - transaction ID is kept the same for each BOOTP/DHCPv4 request
>>
>> In applications where thousands of nodes are serviced by a single DHCP
>> server, maximizing the retransmission backoff interval at 2 seconds (the
>> current u-boot default) exerts high pressure on the DHCP server and
>> network layer.
>>
>> RFC 951 “7.2. Client Retransmission Strategy” states that the
>> retransmission backoff interval should maximize at 60 seconds.  This
>> patch allows the interval to be configurable using the environment
>> variable "bootpretransmitperiodmax"
>>
>> The initial retranmission backoff period defaults to 250ms, which is
>> also too small for these scenarios with many clients.  This patch makes
>> the initial retransmission interval to be configurable using the
>> environment variable "bootpretransmitperiodinit".
>>
>> Also, on a retransmission it is not expected for the transaction ID to
>> change (only the 'secs' field should be updated). Let's save the
>> transaction ID and use the same transaction ID for each BOOTP/DHCPv4
>> exchange.
>>
>> Signed-off-by: Sean Edmond <seanedmond@microsoft.com>
>> ---
>>   net/bootp.c | 63 +++++++++++++++++++++++++++++++++++++++--------------
>>   1 file changed, 47 insertions(+), 16 deletions(-)
>>
>> diff --git a/net/bootp.c b/net/bootp.c
>> index 013d54c7ed..7248536cc4 100644
>> --- a/net/bootp.c
>> +++ b/net/bootp.c
>> @@ -42,6 +42,17 @@
>>    */
>>   #define TIMEOUT_MS	((3 + (CONFIG_NET_RETRY_COUNT * 5)) * 1000)
>>   
>> +/*
>> + * According to rfc951 : 7.2. Client Retransmission Strategy
>> + * "After the 'average' backoff reaches about 60 seconds, it should be
>> + * increased no further, but still randomized."
>> + *
>> + * U-Boot has saturated this backoff at 2 seconds for a long time.
>> + * To modify, set the environment variable "bootpretransmitperiodmax"
>> + */
>> +#define RETRANSMIT_PERIOD_MAX_MS	2000
>> +#define RETRANSMIT_PERIOD_INIT_MS	250
>> +
>>   #define PORT_BOOTPS	67		/* BOOTP server UDP port */
>>   #define PORT_BOOTPC	68		/* BOOTP client UDP port */
>>   
>> @@ -56,6 +67,7 @@
>>   u32		bootp_ids[CFG_BOOTP_ID_CACHE_SIZE];
>>   unsigned int	bootp_num_ids;
>>   int		bootp_try;
>> +u32		bootp_id;
>>   ulong		bootp_start;
>>   ulong		bootp_timeout;
>>   char net_nis_domain[32] = {0,}; /* Our NIS domain */
>> @@ -63,6 +75,7 @@ char net_hostname[32] = {0,}; /* Our hostname */
>>   char net_root_path[CONFIG_BOOTP_MAX_ROOT_PATH_LEN] = {0,}; /* Our bootpath */
>>   
>>   static ulong time_taken_max;
>> +static u32   retransmit_period_max_ms;
>>   
>>   #if defined(CONFIG_CMD_DHCP)
>>   static dhcp_state_t dhcp_state = INIT;
>> @@ -417,8 +430,8 @@ static void bootp_timeout_handler(void)
>>   		}
>>   	} else {
>>   		bootp_timeout *= 2;
>> -		if (bootp_timeout > 2000)
>> -			bootp_timeout = 2000;
>> +		if (bootp_timeout > retransmit_period_max_ms)
>> +			bootp_timeout = retransmit_period_max_ms;
>>   		net_set_timeout_handler(bootp_timeout, bootp_timeout_handler);
>>   		bootp_request();
>>   	}
>> @@ -714,10 +727,18 @@ static int bootp_extended(u8 *e)
>>   
>>   void bootp_reset(void)
>>   {
>> +	char *ep;  /* Environment pointer */
>> +
>>   	bootp_num_ids = 0;
>>   	bootp_try = 0;
>>   	bootp_start = get_timer(0);
>> -	bootp_timeout = 250;
>> +
>> +	ep = env_get("bootpretransmitperiodinit");
>> +	if (ep)
>> +		bootp_timeout = dectoul(ep, NULL);
>> +	else
>> +		bootp_timeout = RETRANSMIT_PERIOD_INIT_MS;
>> +
> bootp_timeout = env_get_ulong("bootpretransmitperiodinit", 0, RETRANSMIT_PERIOD_INIT_MS);
> does the same...
Thanks for the suggestion, will fix in v2.
>>   }
>>   
>>   void bootp_request(void)
>> @@ -729,7 +750,6 @@ void bootp_request(void)
>>   #ifdef CONFIG_BOOTP_RANDOM_DELAY
>>   	ulong rand_ms;
>>   #endif
>> -	u32 bootp_id;
>>   	struct in_addr zero_ip;
>>   	struct in_addr bcast_ip;
>>   	char *ep;  /* Environment pointer */
>> @@ -745,6 +765,12 @@ void bootp_request(void)
>>   	else
>>   		time_taken_max = TIMEOUT_MS;
>>   
>> +	ep = env_get("bootpretransmitperiodmax");
>> +	if (ep)
>> +		retransmit_period_max_ms = dectoul(ep, NULL);
>> +	else
>> +		retransmit_period_max_ms = RETRANSMIT_PERIOD_MAX_MS;
>> +
> see above
>
>
> Lothar Waßmann