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

[Heinrich Schuchardt]

On 11/4/23 03:04, Sean Edmond wrote:
>
> 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?

If you need rand(), you have to ensure that CONFIG_LIB_RAND=y, e.g. let
CONFIG_CMD_BOOTP select CONFIG_LIB_RAND.

Best regards

Heinrich

>
>> 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