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

Sean Edmond seanedmond at linux.microsoft.com
Sat Nov 4 02:04:55 CET 2023


On 2023-10-23 11:06 p.m., Heinrich Schuchardt wrote:
> On 10/24/23 02:21, seanedmond at linux.microsoft.com wrote:
>> From: Sean Edmond <seanedmond at 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
>
> %s/maximize at/be limited to/
>
>> patch allows the interval to be configurable using the environment
>> variable "bootpretransmitperiodmax"
>
> If there is an RFC defining the value, why do we need an environment
> variable?
>
>>
>> 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 at microsoft.com>
>> ---
>>   net/bootp.c | 56 ++++++++++++++++++++++++++++++++++++++---------------
>>   1 file changed, 40 insertions(+), 16 deletions(-)
>>
>> diff --git a/net/bootp.c b/net/bootp.c
>> index 6800290963..bab17a9ceb 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
>
> This does not match RFC 951. Please, why shouldn't we use the standard
> value by default?
>
>> +#define RETRANSMIT_PERIOD_INIT_MS    250
>
> This constant should be described too.
>
>> +
>>   #ifndef CFG_DHCP_MIN_EXT_LEN        /* minimal length of extension 
>> list */
>>   #define CFG_DHCP_MIN_EXT_LEN 64
>>   #endif
>> @@ -53,6 +64,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 */
>> @@ -60,6 +72,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;
>> @@ -414,8 +427,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;
>
> RFC 951 requires that the backoff time is randomized.
>
I added randomization in v3.  Note, in that update I haven't put any 
guards around the use of rand()/srand() (as is the case in net_rand.h).  
Perhaps it's safe to assume it will always be present for inclusion of net?

> Best regards
>
> Heinrich
>
>> net_set_timeout_handler(bootp_timeout, bootp_timeout_handler);
>>           bootp_request();
>>       }
>> @@ -711,10 +724,14 @@ 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;
>> +
>> +    bootp_timeout = env_get_ulong("bootpretransmitperiodinit", 10, 
>> RETRANSMIT_PERIOD_INIT_MS);
>> +
>>   }
>>
>>   void bootp_request(void)
>> @@ -726,7 +743,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 */
>> @@ -742,6 +758,9 @@ void bootp_request(void)
>>       else
>>           time_taken_max = TIMEOUT_MS;
>>
>> +    retransmit_period_max_ms = 
>> env_get_ulong("bootpretransmitperiodmax", 10,
>> +                         RETRANSMIT_PERIOD_MAX_MS);
>> +
>>   #ifdef CONFIG_BOOTP_RANDOM_DELAY        /* Random BOOTP delay */
>>       if (bootp_try == 0)
>>           srand_mac();
>> @@ -801,18 +820,23 @@ void bootp_request(void)
>>       extlen = bootp_extended((u8 *)bp->bp_vend);
>>   #endif
>>
>> -    /*
>> -     *    Bootp ID is the lower 4 bytes of our ethernet address
>> -     *    plus the current time in ms.
>> -     */
>> -    bootp_id = ((u32)net_ethaddr[2] << 24)
>> -        | ((u32)net_ethaddr[3] << 16)
>> -        | ((u32)net_ethaddr[4] << 8)
>> -        | (u32)net_ethaddr[5];
>> -    bootp_id += get_timer(0);
>> -    bootp_id = htonl(bootp_id);
>> -    bootp_add_id(bootp_id);
>> -    net_copy_u32(&bp->bp_id, &bootp_id);
>> +    /* Only generate a new transaction ID for each new BOOTP request */
>> +    if (bootp_try == 1) {
>> +        /*
>> +         *    Bootp ID is the lower 4 bytes of our ethernet address
>> +         *    plus the current time in ms.
>> +         */
>> +        bootp_id = ((u32)net_ethaddr[2] << 24)
>> +            | ((u32)net_ethaddr[3] << 16)
>> +            | ((u32)net_ethaddr[4] << 8)
>> +            | (u32)net_ethaddr[5];
>> +        bootp_id += get_timer(0);
>> +        bootp_id = htonl(bootp_id);
>> +        bootp_add_id(bootp_id);
>> +        net_copy_u32(&bp->bp_id, &bootp_id);
>> +    } else {
>> +        net_copy_u32(&bp->bp_id, &bootp_id);
>> +    }
>>
>>       /*
>>        * Calculate proper packet lengths taking into account the


More information about the U-Boot mailing list