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....@gmail.com>
> Reviewed-by: Marek Vasut <ma...@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.ceres...@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 */
>
