[PATCH v4] net: tftp: Add client support for RFC 7440

[Matthias Brugger <matthias.bgg@gmail.com>]

On 19/05/2020 21:25, Ramon Fried wrote:
> Add support for RFC 7440: "TFTP Windowsize Option".
> 
> This optional feature allows the client and server
> to negotiate a window size of consecutive blocks to send as an
> alternative for replacing the single-block lockstep schema.
> 
> windowsize can be defined statically during compilation by
> setting CONFIG_TFTP_WINDOWSIZE, or defined in runtime by
> setting an environment variable: "tftpwindowsize"
> If not defined, the windowsize is set to 1, meaning that it
> behaves as it was never defined.
> 
> Choosing the appropriate windowsize depends on the specific
> network topology, underlying NIC.
> You should test various windowsize scenarios and see which
> best work for you.
> 
> Setting a windowsize too big can actually decreases performance.
> 
> Signed-off-by: Ramon Fried <rfried.dev@gmail.com>
> Reviewed-by: Marek Vasut <marex@denx.de>
> ---
> v2:
>  * Don't send windowsize option on tftpput, as it's not implemented yet.
>  * Don't send NACK for every out of order block that arrives, one nack
>    is enough.
> v3:
>  * Add option CONFIG_TFTP_WINDOWSIZE to kconfig with default 1.
>  * Fixed some spelling errors.
>  * Took assignment out of a loop.
>  * simplified variable increment.
> v4:
>  * send ack for last packet, so the server can finish
>    the tranfer gracefully and not in timeout.
> 
>  README      |  5 ++++
>  net/Kconfig |  9 ++++++
>  net/tftp.c  | 81 +++++++++++++++++++++++++++++++++++++++++++++++------
>  3 files changed, 87 insertions(+), 8 deletions(-)
> 
> diff --git a/README b/README
> index be9e6391d6..686474a2f1 100644
> --- a/README
> +++ b/README
> @@ -3522,6 +3522,11 @@ List of environment variables (most likely not complete):
>  		  downloads succeed with high packet loss rates, or with
>  		  unreliable TFTP servers or client hardware.
>  
> +  tftpwindowsize	- if this is set, the value is used for TFTP's
> +		  window size as described by RFC 7440.
> +		  This means the count of blocks we can receive before
> +		  sending ack to server.
> +
>    vlan		- When set to a value < 4095 the traffic over
>  		  Ethernet is encapsulated/received over 802.1q
>  		  VLAN tagged frames.
> diff --git a/net/Kconfig b/net/Kconfig
> index ac6d0cf8a6..7916ae305f 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -49,4 +49,13 @@ config TFTP_BLOCKSIZE
>  	  almost-MTU block sizes.
>  	  You can also activate CONFIG_IP_DEFRAG to set a larger block.
>  
> +config TFTP_WINDOWSIZE
> +	int "TFTP window size"
> +	default 1
> +	help
> +	  Default TFTP window size.
> +	  RFC7440 defines an optional window size of transmits,
> +	  before an ack response is required.
> +	  The default TFTP implementation implies a window size of 1.
> +
>  endif   # if NET
> diff --git a/net/tftp.c b/net/tftp.c
> index be24e63075..72d23e1574 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -5,7 +5,6 @@
>   * Copyright 2011 Comelit Group SpA,
>   *                Luca Ceresoli <luca.ceresoli@comelit.it>
>   */
> -
>  #include <common.h>
>  #include <command.h>
>  #include <efi_loader.h>
> @@ -95,6 +94,12 @@ static int	tftp_tsize;
>  /* The number of hashes we printed */
>  static short	tftp_tsize_num_hash;
>  #endif
> +/* The window size negotiated */
> +static ushort	tftp_windowsize;
> +/* Next block to send ack to */
> +static ushort	tftp_next_ack;
> +/* Last nack block we send */
> +static ushort	tftp_last_nack;
>  #ifdef CONFIG_CMD_TFTPPUT
>  /* 1 if writing, else 0 */
>  static int	tftp_put_active;
> @@ -134,8 +139,19 @@ static char tftp_filename[MAX_LEN];
>   * (but those using CONFIG_IP_DEFRAG may want to set a larger block in cfg file)
>   */
>  
> +/* When windowsize is defined to 1,
> + * tftp behaves the same way as it was
> + * never declared
> + */
> +#ifdef CONFIG_TFTP_WINDOWSIZE
> +#define TFTP_WINDOWSIZE CONFIG_TFTP_WINDOWSIZE
> +#else
> +#define TFTP_WINDOWSIZE 1
> +#endif
> +
>  static unsigned short tftp_block_size = TFTP_BLOCK_SIZE;
>  static unsigned short tftp_block_size_option = CONFIG_TFTP_BLOCKSIZE;
> +static unsigned short tftp_window_size_option = TFTP_WINDOWSIZE;
>  
>  static inline int store_block(int block, uchar *src, unsigned int len)
>  {
> @@ -348,6 +364,14 @@ static void tftp_send(void)
>  		/* try for more effic. blk size */
>  		pkt += sprintf((char *)pkt, "blksize%c%d%c",
>  				0, tftp_block_size_option, 0);
> +
> +		/* try for more effic. window size.
> +		 * Implemented only for tftp get.
> +		 * Don't bother sending if it's 1
> +		 */
> +		if (tftp_state == STATE_SEND_RRQ && tftp_window_size_option > 1)

I think it makes more sense to check:
if (tftp_window_size_option > 1 && tftp_state == STATE_SEND_RRQ)

Because I understand that the tftp_state will change while the
tftp_window_size_option is set or at compile time or through the environment. So
we can save the check of the tftp_state if we have the default value.

Regards,
Matthias

> +			pkt += sprintf((char *)pkt, "windowsize%c%d%c",
> +					0, tftp_window_size_option, 0);
>  		len = pkt - xp;
>  		break;
>  
> @@ -500,7 +524,18 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>  				      (char *)pkt + i + 6, tftp_tsize);
>  			}
>  #endif
> +			if (strcmp((char *)pkt + i,  "windowsize") == 0) {
> +				tftp_windowsize =
> +					simple_strtoul((char *)pkt + i + 11,
> +						       NULL, 10);
> +			    debug("windowsize = %s, %d\n",
> +				  (char *)pkt + i + 11, tftp_windowsize);
> +			}
> +
>  		}
> +
> +		tftp_next_ack = tftp_windowsize;
> +
>  #ifdef CONFIG_CMD_TFTPPUT
>  		if (tftp_put_active) {
>  			/* Get ready to send the first block */
> @@ -514,12 +549,32 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>  		if (len < 2)
>  			return;
>  		len -= 2;
> -		tftp_cur_block = ntohs(*(__be16 *)pkt);
> +
> +		if (ntohs(*(__be16 *)pkt) != (ushort)(tftp_cur_block + 1)) {
> +			debug("Received unexpected block: %d, expected: %d\n",
> +			      ntohs(*(__be16 *)pkt),
> +			      (ushort)(tftp_cur_block + 1));
> +			/*
> +			 * If one packet is dropped most likely
> +			 * all other buffers in the window
> +			 * that will arrive will cause a sending NACK.
> +			 * This just overwellms the server, let's just send one.
> +			 */
> +			if (tftp_last_nack != tftp_cur_block) {
> +				tftp_send();
> +				tftp_last_nack = tftp_cur_block;
> +				tftp_next_ack = (ushort)(tftp_cur_block +
> +							 tftp_windowsize);
> +			}
> +			break;
> +		}
> +
> +		tftp_cur_block++;
>  
>  		update_block_number();
>  
>  		if (tftp_state == STATE_SEND_RRQ)
> -			debug("Server did not acknowledge timeout option!\n");
> +			debug("Server did not acknowledge the options!\n");
>  
>  		if (tftp_state == STATE_SEND_RRQ || tftp_state == STATE_OACK ||
>  		    tftp_state == STATE_RECV_WRQ) {
> @@ -557,10 +612,15 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>  		 *	Acknowledge the block just received, which will prompt
>  		 *	the remote for the next one.
>  		 */
> -		tftp_send();
> +		if (tftp_cur_block == tftp_next_ack) {
> +			tftp_send();
> +			tftp_next_ack += tftp_windowsize;
> +		}
>  
> -		if (len < tftp_block_size)
> +		if (len < tftp_block_size) {
> +			tftp_send();
>  			tftp_complete();
> +		}
>  		break;
>  
>  	case TFTP_ERROR:
> @@ -634,6 +694,10 @@ void tftp_start(enum proto_t protocol)
>  	if (ep != NULL)
>  		tftp_block_size_option = simple_strtol(ep, NULL, 10);
>  
> +	ep = env_get("tftpwindowsize");
> +	if (ep != NULL)
> +		tftp_window_size_option = simple_strtol(ep, NULL, 10);
> +
>  	ep = env_get("tftptimeout");
>  	if (ep != NULL)
>  		timeout_ms = simple_strtol(ep, NULL, 10);
> @@ -655,8 +719,8 @@ void tftp_start(enum proto_t protocol)
>  	}
>  #endif
>  
> -	debug("TFTP blocksize = %i, timeout = %ld ms\n",
> -	      tftp_block_size_option, timeout_ms);
> +	debug("TFTP blocksize = %i, TFTP windowsize = %d timeout = %ld ms\n",
> +	      tftp_block_size_option, tftp_window_size_option, timeout_ms);
>  
>  	tftp_remote_ip = net_server_ip;
>  	if (!net_parse_bootfile(&tftp_remote_ip, tftp_filename, MAX_LEN)) {
> @@ -752,7 +816,8 @@ void tftp_start(enum proto_t protocol)
>  		tftp_our_port = simple_strtol(ep, NULL, 10);
>  #endif
>  	tftp_cur_block = 0;
> -
> +	tftp_windowsize = 1;
> +	tftp_last_nack = 0;
>  	/* zero out server ether in case the server ip has changed */
>  	memset(net_server_ethaddr, 0, 6);
>  	/* Revert tftp_block_size to dflt */
>