[PATCH 00/13] add "windowsize" (RFC 7440) support for tftp

[PATCH 00/13] add "windowsize" (RFC 7440) support for tftp

[Enrico Scholz]

The tftp "windowsize" greatly improves the performance of tftp
transfers.  This patchset adds support for it.

The first two patches are a little bit unrelated and enhance the 'cp
-v' output by giving information about the transfer speed.  They can
be dropped if they are unwanted.

I tested the function with an iMX8MP platform in three environments:

  - at home over OpenVPN on an ADSL 50 line  -->  27x speedup
  - 1 Gb/s connection --> 9x speedup
  - connection over 100 Mb/s switch  -->  4x speedup

In the test, I downloaded variable sized files which were filled from
/dev/urandom.  E.g.

| :/ global tftp.windowsize=128
| :/ cp -v /mnt/tftp/data-100MiB /tmp/data && sha1sum /tmp/data
|         [################################################################] 104857600 bytes, 98550375 bytes/s

For slow connection speed, smaller files (1MiB, 4 MiB + 20 MiB) were
used.


The numbers (bytes/s) are

 | windowsize | VPN       | 1 Gb/s     | 100 Mb/s   |
 |------------|-----------|------------|------------|
 | 128        | 3.869.284 | 98.643.085 | 11.434.852 |
 |  64        | 3.863.581 | 98.550.375 | 11.434.852 |
 |  48        | 3.431.580 | 94.211.680 | 11.275.010 |
 |  32        | 2.835.129 | 85.250.081 | 10.985.605 |
 |  24        | 2.344.858 | 77.787.537 | 10.765.667 |
 |  16        | 1.734.186 | 67.519.381 | 10.210.087 |
 |  12        | 1.403.340 | 61.972.576 |  9.915.612 |
 |   8        | 1.002.462 | 50.852.376 |  9.016.130 |
 |   6        |   775.573 | 42.781.558 |  8.422.297 |
 |   4        |   547.845 | 32.066.544 |  6.835.567 |
 |   3        |   412.987 | 26.526.081 |  6.322.435 |
 |   2        |   280.987 | 19.120.641 |  5.494.241 |
 |   1        |   141.699 | 10.431.516 |  2.967.224 |
 |------------|-----------|------------|------------|
 | unpatched  |   140.587 | 10.553.301 |  2.978.063 |



Enrico Scholz (13):
  progress: add close_progress() to display some statistics
  libfile:copy_file: show statistics in verbose mode
  tftp: minor refactoring of RRQ/WRQ packet generation code
  tftp: replace hardcoded blksize by global constant
  tftp: record whether tftp file is opened for lookup operation only
  tftp: reduce block size on lookup requests
  tftp: refactor data processing
  tftp: detect out-of-memory situations
  tftp: implement 'windowsize' (RFC 7440) support
  tftp: do not use 'priv->block' for RRQ
  tftp: reorder tftp packets
  tftp: allow to change tftp port
  tftp: add sanity check for OACK response

 fs/Kconfig          |  36 ++++++
 fs/tftp.c           | 298 +++++++++++++++++++++++++++++++++++++-------
 include/progress.h  |   1 +
 lib/libfile.c       |   3 +
 lib/show_progress.c |  25 ++++
 5 files changed, 319 insertions(+), 44 deletions(-)

-- 
2.36.1

[PATCH 01/13] progress: add close_progress() to display some statistics

[Enrico Scholz]

With a later patch, this will produce output like

| :/ cp -v /mnt/tftp/tmp /tmp/
|         [########...#########] 29138411 bytes, 127413 bytes/s

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 include/progress.h  |  1 +
 lib/show_progress.c | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/progress.h b/include/progress.h
index 7230bd3a9bd5..219d772201bf 100644
--- a/include/progress.h
+++ b/include/progress.h
@@ -16,6 +16,7 @@ void init_progression_bar(loff_t max);
  * spinner is printed.
  */
 void show_progress(loff_t now);
+void close_progress(loff_t total);
 
 extern struct notifier_head progress_notifier;
 
diff --git a/lib/show_progress.c b/lib/show_progress.c
index 1b624bcb9af8..5b42a2d74b8f 100644
--- a/lib/show_progress.c
+++ b/lib/show_progress.c
@@ -24,6 +24,7 @@
 static loff_t printed;
 static loff_t progress_max;
 static unsigned spin;
+static uint64_t start_tm;
 
 void show_progress(loff_t now)
 {
@@ -47,11 +48,35 @@ void show_progress(loff_t now)
 	}
 }
 
+void close_progress(loff_t total)
+{
+	uint64_t now = get_time_ns();
+	unsigned long speed;
+	unsigned long delta;
+
+	if (now <= start_tm) {
+		pr_crit("tm mismatch");
+		return;
+	}
+
+	if (total < 0)
+		return;
+
+	/* convert to ms and add '1' to avoid div-by-zero */
+	delta = div_u64(now - start_tm, 1000000) + 1;
+
+	/* speed is bytes/s */
+	speed = div_u64(total * 1000, delta);
+
+	printf("] %llu bytes, %lu bytes/s", (unsigned long long)total, speed);
+}
+
 void init_progression_bar(loff_t max)
 {
 	printed = 0;
 	progress_max = max;
 	spin = 0;
+	start_tm = get_time_ns();
 	if (progress_max && progress_max != FILESIZE_MAX)
 		printf("\t[%*s]\r\t[", HASHES_PER_LINE, "");
 	else
-- 
2.36.1

[PATCH 02/13] libfile:copy_file: show statistics in verbose mode

[Enrico Scholz]

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 lib/libfile.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/libfile.c b/lib/libfile.c
index 3b7985fbcabb..9eb06de02945 100644
--- a/lib/libfile.c
+++ b/lib/libfile.c
@@ -454,6 +454,9 @@ int copy_file(const char *src, const char *dst, int verbose)
 		}
 	}
 
+	if (verbose)
+		close_progress(total);
+
 	ret = 0;
 out:
 	if (verbose)
-- 
2.36.1

[PATCH 03/13] tftp: minor refactoring of RRQ/WRQ packet generation code

[Enrico Scholz]

Having 11 printf arguments with lot of them being 0, makes it
difficulty to read and extend.

Add some comments and use '\0' for %c.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index d186e7983a6d..13334baaf892 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -135,13 +135,13 @@ static int tftp_send(struct file_priv *priv)
 				"%lld%c"
 				"blksize%c"
 				"1432",
-				priv->filename + 1, 0,
-				0,
-				0,
-				TIMEOUT, 0,
-				0,
-				priv->filesize, 0,
-				0);
+				priv->filename + 1, '\0',
+				'\0',	/* "octet" */
+				'\0',	/* "timeout" */
+				TIMEOUT, '\0',
+			        '\0',	/* "tsize" */
+				priv->filesize, '\0',
+			       '\0');	/* "blksize" */
 		pkt++;
 		len = pkt - xp;
 		break;
-- 
2.36.1

[PATCH 04/13] tftp: replace hardcoded blksize by global constant

[Enrico Scholz]

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index 13334baaf892..7ad03ae5cdc6 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -64,6 +64,7 @@
 #define STATE_DONE	8
 
 #define TFTP_BLOCK_SIZE		512	/* default TFTP block size */
+#define TFTP_MTU_SIZE		1432	/* MTU based block size */
 #define TFTP_FIFO_SIZE		4096
 
 #define TFTP_ERR_RESEND	1
@@ -134,14 +135,15 @@ static int tftp_send(struct file_priv *priv)
 				"tsize%c"
 				"%lld%c"
 				"blksize%c"
-				"1432",
+				"%u",
 				priv->filename + 1, '\0',
 				'\0',	/* "octet" */
 				'\0',	/* "timeout" */
 				TIMEOUT, '\0',
 			        '\0',	/* "tsize" */
 				priv->filesize, '\0',
-			       '\0');	/* "blksize" */
+			       '\0',	/* "blksize" */
+			       TFTP_MTU_SIZE);
 		pkt++;
 		len = pkt - xp;
 		break;
-- 
2.36.1

[PATCH 05/13] tftp: record whether tftp file is opened for lookup operation only

[Enrico Scholz]

Opening a tftp is done in two steps: at first `tftp_lookup()` is
called to get the filesize and then it is opened again and data are
read.

The `tftp_lookup()` call sends only a RRQ/WRQ, reads then the "tsize"
from the response and closes the transfer by sending an error datagram.
The tftp server will send a full data window.

To prevent unneeded traffic, later patches set parameters to reduce
the size of the server response.

We need knowledge about type of operation which is recorded in an
"is_getattr" attribute.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index 7ad03ae5cdc6..ab1af0e9084c 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -84,6 +84,7 @@ struct file_priv {
 	void *buf;
 	int blocksize;
 	int block_requested;
+	bool is_getattr;
 };
 
 struct tftp_priv {
@@ -372,7 +373,7 @@ static void tftp_handler(void *ctx, char *packet, unsigned len)
 }
 
 static struct file_priv *tftp_do_open(struct device_d *dev,
-		int accmode, struct dentry *dentry)
+		int accmode, struct dentry *dentry, bool is_getattr)
 {
 	struct fs_device_d *fsdev = dev_to_fs_device(dev);
 	struct file_priv *priv;
@@ -400,6 +401,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
 	priv->filename = dpath(dentry, fsdev->vfsmount.mnt_root);
 	priv->blocksize = TFTP_BLOCK_SIZE;
 	priv->block_requested = -1;
+	priv->is_getattr = is_getattr;
 
 	priv->fifo = kfifo_alloc(TFTP_FIFO_SIZE);
 	if (!priv->fifo) {
@@ -451,7 +453,7 @@ static int tftp_open(struct device_d *dev, FILE *file, const char *filename)
 {
 	struct file_priv *priv;
 
-	priv = tftp_do_open(dev, file->flags, file->dentry);
+	priv = tftp_do_open(dev, file->flags, file->dentry, false);
 	if (IS_ERR(priv))
 		return PTR_ERR(priv);
 
@@ -667,7 +669,7 @@ static struct dentry *tftp_lookup(struct inode *dir, struct dentry *dentry,
 	struct file_priv *priv;
 	loff_t filesize;
 
-	priv = tftp_do_open(&fsdev->dev, O_RDONLY, dentry);
+	priv = tftp_do_open(&fsdev->dev, O_RDONLY, dentry, true);
 	if (IS_ERR(priv))
 		return NULL;
 
-- 
2.36.1

[PATCH 06/13] tftp: reduce block size on lookup requests

[Enrico Scholz]

Save some bytes on network traffic by reducing the server response for
lookup requests.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index ab1af0e9084c..01d3beff14bf 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -144,7 +144,9 @@ static int tftp_send(struct file_priv *priv)
 			        '\0',	/* "tsize" */
 				priv->filesize, '\0',
 			       '\0',	/* "blksize" */
-			       TFTP_MTU_SIZE);
+			        /* use only a minimal blksize for getattr
+				   operations, */
+			       priv->is_getattr ? TFTP_BLOCK_SIZE : TFTP_MTU_SIZE);
 		pkt++;
 		len = pkt - xp;
 		break;
-- 
2.36.1

[PATCH 07/13] tftp: refactor data processing

[Enrico Scholz]

move block handling into dedicated function

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index 01d3beff14bf..81141626ab91 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -249,6 +249,20 @@ static void tftp_timer_reset(struct file_priv *priv)
 	priv->progress_timeout = priv->resend_timeout = get_time_ns();
 }
 
+static void tftp_put_data(struct file_priv *priv, uint16_t block,
+			  void const *pkt, size_t len)
+{
+	priv->last_block = block;
+
+	kfifo_put(priv->fifo, pkt, len);
+
+	if (len < priv->blocksize) {
+		tftp_send(priv);
+		priv->err = 0;
+		priv->state = STATE_DONE;
+	}
+}
+
 static void tftp_recv(struct file_priv *priv,
 			uint8_t *pkt, unsigned len, uint16_t uh_sport)
 {
@@ -332,17 +346,8 @@ static void tftp_recv(struct file_priv *priv,
 			/* Same block again; ignore it. */
 			break;
 
-		priv->last_block = priv->block;
-
 		tftp_timer_reset(priv);
-
-		kfifo_put(priv->fifo, pkt + 2, len);
-
-		if (len < priv->blocksize) {
-			tftp_send(priv);
-			priv->err = 0;
-			priv->state = STATE_DONE;
-		}
+		tftp_put_data(priv, priv->block, pkt + 2, len);
 
 		break;
 
-- 
2.36.1

[PATCH 08/13] tftp: detect out-of-memory situations

[Enrico Scholz]

it should never happen due to the program logic; but detect a failed
kfifo_put() just in case...

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index 81141626ab91..ef2ce0200b38 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -252,11 +252,18 @@ static void tftp_timer_reset(struct file_priv *priv)
 static void tftp_put_data(struct file_priv *priv, uint16_t block,
 			  void const *pkt, size_t len)
 {
+	unsigned int sz;
+
 	priv->last_block = block;
 
-	kfifo_put(priv->fifo, pkt, len);
+	sz = kfifo_put(priv->fifo, pkt, len);
 
-	if (len < priv->blocksize) {
+	if (sz != len) {
+		pr_err("tftp: not enough room in kfifo (only %u out of %zu written\n",
+		       sz, len);
+		priv->err = -ENOMEM;
+		priv->state = STATE_DONE;
+	} else if (len < priv->blocksize) {
 		tftp_send(priv);
 		priv->err = 0;
 		priv->state = STATE_DONE;
-- 
2.36.1

[PATCH 09/13] tftp: implement 'windowsize' (RFC 7440) support

[Enrico Scholz]

Results (with the reorder patch; numbers in bytes/s) on an iMX8MP are:

 | windowsize | VPN       | 1 Gb/s     | 100 Mb/s   |
 |------------|-----------|------------|------------|
 | 128        | 3.869.284 | 98.643.085 | 11.434.852 |
 |  64        | 3.863.581 | 98.550.375 | 11.434.852 |
 |  48        | 3.431.580 | 94.211.680 | 11.275.010 |
 |  32        | 2.835.129 | 85.250.081 | 10.985.605 |
 |  24        | 2.344.858 | 77.787.537 | 10.765.667 |
 |  16        | 1.734.186 | 67.519.381 | 10.210.087 |
 |  12        | 1.403.340 | 61.972.576 |  9.915.612 |
 |   8        | 1.002.462 | 50.852.376 |  9.016.130 |
 |   6        |   775.573 | 42.781.558 |  8.422.297 |
 |   4        |   547.845 | 32.066.544 |  6.835.567 |
 |   3        |   412.987 | 26.526.081 |  6.322.435 |
 |   2        |   280.987 | 19.120.641 |  5.494.241 |
 |   1        |   141.699 | 10.431.516 |  2.967.224 |

(VPN = OpenVPN on ADSL 50 Mb/s).

The window size can be configured at runtime.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/Kconfig | 14 ++++++++++++++
 fs/tftp.c  | 54 ++++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 54 insertions(+), 14 deletions(-)

diff --git a/fs/Kconfig b/fs/Kconfig
index aeba00073eed..0c4934285942 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -43,6 +43,20 @@ config FS_TFTP
 	prompt "tftp support"
 	depends on NET
 
+config FS_TFTP_MAX_WINDOW_SIZE
+	int
+	prompt "maximum tftp window size (RFC 7440)"
+	depends on FS_TFTP
+	default 128
+	range 1 128
+	help
+	  The maximum allowed tftp "windowsize" (RFC 7440).  Higher
+	  value increase speed of the tftp download with the cost of
+	  memory (1432 bytes per slot).
+
+	  Requires tftp "windowsize" (RFC 7440) support on server side
+	  to have an effect.
+
 config FS_OMAP4_USBBOOT
 	bool
 	prompt "Filesystem over usb boot"
diff --git a/fs/tftp.c b/fs/tftp.c
index ef2ce0200b38..7f836d36b7df 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -30,6 +30,7 @@
 #include <linux/stat.h>
 #include <linux/err.h>
 #include <kfifo.h>
+#include <globalvar.h>
 #include <linux/sizes.h>
 
 #define TFTP_PORT	69	/* Well known TFTP port number */
@@ -65,15 +66,22 @@
 
 #define TFTP_BLOCK_SIZE		512	/* default TFTP block size */
 #define TFTP_MTU_SIZE		1432	/* MTU based block size */
-#define TFTP_FIFO_SIZE		4096
+#define TFTP_MAX_WINDOW_SIZE	CONFIG_FS_TFTP_MAX_WINDOW_SIZE
+
+/* calculate fifo so that it can hold the complete window plus the incoming
+   packet.  Add some extra space just in case...  */
+#define TFTP_FIFO_SIZE		(TFTP_MTU_SIZE * TFTP_MAX_WINDOW_SIZE + 2048)
 
 #define TFTP_ERR_RESEND	1
 
+static int g_tftp_window_size = TFTP_MAX_WINDOW_SIZE / 1;
+
 struct file_priv {
 	struct net_connection *tftp_con;
 	int push;
 	uint16_t block;
 	uint16_t last_block;
+	uint16_t ack_block;
 	int state;
 	int err;
 	char *filename;
@@ -83,7 +91,7 @@ struct file_priv {
 	struct kfifo *fifo;
 	void *buf;
 	int blocksize;
-	int block_requested;
+	unsigned int windowsize;
 	bool is_getattr;
 };
 
@@ -114,6 +122,7 @@ static int tftp_send(struct file_priv *priv)
 	int len = 0;
 	uint16_t *s;
 	unsigned char *pkt = net_udp_get_payload(priv->tftp_con);
+	unsigned int window_size;
 	int ret;
 
 	pr_vdebug("%s: state %s\n", __func__, tftp_states[priv->state]);
@@ -121,6 +130,15 @@ static int tftp_send(struct file_priv *priv)
 	switch (priv->state) {
 	case STATE_RRQ:
 	case STATE_WRQ:
+		if (priv->push || priv->is_getattr)
+			/* atm, windowsize is suported only for RRQ and there
+			   is no need to request a full window when we are
+			   just looking up file attributes */
+			window_size = 1;
+		else
+			window_size = min_t(unsigned int, g_tftp_window_size,
+					    TFTP_MAX_WINDOW_SIZE);
+
 		xp = pkt;
 		s = (uint16_t *)pkt;
 		if (priv->state == STATE_RRQ)
@@ -136,30 +154,35 @@ static int tftp_send(struct file_priv *priv)
 				"tsize%c"
 				"%lld%c"
 				"blksize%c"
-				"%u",
+				"%u%c"
+			        "windowsize%c"
+			        "%u",
 				priv->filename + 1, '\0',
 				'\0',	/* "octet" */
 				'\0',	/* "timeout" */
 				TIMEOUT, '\0',
 			        '\0',	/* "tsize" */
 				priv->filesize, '\0',
-			       '\0',	/* "blksize" */
+				'\0',	/* "blksize" */
 			        /* use only a minimal blksize for getattr
 				   operations, */
-			       priv->is_getattr ? TFTP_BLOCK_SIZE : TFTP_MTU_SIZE);
+			        priv->is_getattr ? TFTP_BLOCK_SIZE : TFTP_MTU_SIZE,
+			        '\0',
+				'\0',	/* windowsize */
+				window_size
+			);
 		pkt++;
 		len = pkt - xp;
 		break;
 
 	case STATE_RDATA:
-		if (priv->block == priv->block_requested)
-			return 0;
 	case STATE_OACK:
 		xp = pkt;
 		s = (uint16_t *)pkt;
 		*s++ = htons(TFTP_ACK);
-		*s++ = htons(priv->block);
-		priv->block_requested = priv->block;
+		*s++ = htons(priv->last_block);
+		priv->ack_block  = priv->last_block;
+		priv->ack_block += priv->windowsize;
 		pkt = (unsigned char *)s;
 		len = pkt - xp;
 		break;
@@ -202,7 +225,6 @@ static int tftp_poll(struct file_priv *priv)
 	if (is_timeout(priv->resend_timeout, TFTP_RESEND_TIMEOUT)) {
 		printf("T ");
 		priv->resend_timeout = get_time_ns();
-		priv->block_requested = -1;
 		return TFTP_ERR_RESEND;
 	}
 
@@ -239,6 +261,8 @@ static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
 			priv->filesize = simple_strtoull(val, NULL, 10);
 		if (!strcmp(opt, "blksize"))
 			priv->blocksize = simple_strtoul(val, NULL, 10);
+		if (!strcmp(opt, "windowsize"))
+			priv->windowsize = simple_strtoul(val, NULL, 10);
 		pr_debug("OACK opt: %s val: %s\n", opt, val);
 		s = val + strlen(val) + 1;
 	}
@@ -326,6 +350,7 @@ static void tftp_recv(struct file_priv *priv,
 			/* send ACK */
 			priv->state = STATE_OACK;
 			priv->block = 0;
+			priv->last_block = 0;
 			tftp_send(priv);
 		}
 
@@ -349,9 +374,9 @@ static void tftp_recv(struct file_priv *priv,
 			}
 		}
 
-		if (priv->block == priv->last_block)
-			/* Same block again; ignore it. */
+		if (priv->block != (uint16_t)(priv->last_block + 1)) {
 			break;
+		}
 
 		tftp_timer_reset(priv);
 		tftp_put_data(priv, priv->block, pkt + 2, len);
@@ -414,7 +439,6 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
 	priv->err = -EINVAL;
 	priv->filename = dpath(dentry, fsdev->vfsmount.mnt_root);
 	priv->blocksize = TFTP_BLOCK_SIZE;
-	priv->block_requested = -1;
 	priv->is_getattr = is_getattr;
 
 	priv->fifo = kfifo_alloc(TFTP_FIFO_SIZE);
@@ -574,7 +598,7 @@ static int tftp_read(struct device_d *dev, FILE *f, void *buf, size_t insize)
 		if (priv->state == STATE_DONE)
 			return outsize;
 
-		if (TFTP_FIFO_SIZE - kfifo_len(priv->fifo) >= priv->blocksize)
+		if (priv->last_block == priv->ack_block)
 			tftp_send(priv);
 
 		ret = tftp_poll(priv);
@@ -766,6 +790,8 @@ static struct fs_driver_d tftp_driver = {
 
 static int tftp_init(void)
 {
+	globalvar_add_simple_int("tftp.windowsize", &g_tftp_window_size, "%u");
+
 	return register_fs_driver(&tftp_driver);
 }
 coredevice_initcall(tftp_init);
-- 
2.36.1

[PATCH v2 09/13] tftp: implement 'windowsize' (RFC 7440) support

[Enrico Scholz]

Results (with the reorder patch; numbers in bytes/s) on an iMX8MP are:

 | windowsize | VPN       | 1 Gb/s     | 100 Mb/s   |
 |------------|-----------|------------|------------|
 | 128        | 3.869.284 | 98.643.085 | 11.434.852 |
 |  64        | 3.863.581 | 98.550.375 | 11.434.852 |
 |  48        | 3.431.580 | 94.211.680 | 11.275.010 |
 |  32        | 2.835.129 | 85.250.081 | 10.985.605 |
 |  24        | 2.344.858 | 77.787.537 | 10.765.667 |
 |  16        | 1.734.186 | 67.519.381 | 10.210.087 |
 |  12        | 1.403.340 | 61.972.576 |  9.915.612 |
 |   8        | 1.002.462 | 50.852.376 |  9.016.130 |
 |   6        |   775.573 | 42.781.558 |  8.422.297 |
 |   4        |   547.845 | 32.066.544 |  6.835.567 |
 |   3        |   412.987 | 26.526.081 |  6.322.435 |
 |   2        |   280.987 | 19.120.641 |  5.494.241 |
 |   1        |   141.699 | 10.431.516 |  2.967.224 |

(VPN = OpenVPN on ADSL 50 Mb/s).

The window size can be configured at runtime.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/Kconfig | 14 ++++++++++++++
 fs/tftp.c  | 51 ++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 54 insertions(+), 11 deletions(-)

diff --git a/fs/Kconfig b/fs/Kconfig
index aeba00073..0c4934285 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -43,6 +43,20 @@ config FS_TFTP
 	prompt "tftp support"
 	depends on NET
 
+config FS_TFTP_MAX_WINDOW_SIZE
+	int
+	prompt "maximum tftp window size (RFC 7440)"
+	depends on FS_TFTP
+	default 128
+	range 1 128
+	help
+	  The maximum allowed tftp "windowsize" (RFC 7440).  Higher
+	  value increase speed of the tftp download with the cost of
+	  memory (1432 bytes per slot).
+
+	  Requires tftp "windowsize" (RFC 7440) support on server side
+	  to have an effect.
+
 config FS_OMAP4_USBBOOT
 	bool
 	prompt "Filesystem over usb boot"
diff --git a/fs/tftp.c b/fs/tftp.c
index ef2ce0200..b8950f250 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -29,7 +29,9 @@
 #include <init.h>
 #include <linux/stat.h>
 #include <linux/err.h>
+#include <linux/kernel.h>
 #include <kfifo.h>
+#include <globalvar.h>
 #include <linux/sizes.h>
 
 #define TFTP_PORT	69	/* Well known TFTP port number */
@@ -65,15 +67,22 @@
 
 #define TFTP_BLOCK_SIZE		512	/* default TFTP block size */
 #define TFTP_MTU_SIZE		1432	/* MTU based block size */
-#define TFTP_FIFO_SIZE		4096
+#define TFTP_MAX_WINDOW_SIZE	CONFIG_FS_TFTP_MAX_WINDOW_SIZE
+
+/* calculate fifo so that it can hold the complete window plus the incoming
+   packet.  Add some extra space just in case...  */
+#define TFTP_FIFO_SIZE		(TFTP_MTU_SIZE * TFTP_MAX_WINDOW_SIZE + 2048)
 
 #define TFTP_ERR_RESEND	1
 
+static int g_tftp_window_size = DIV_ROUND_UP(TFTP_MAX_WINDOW_SIZE, 2);
+
 struct file_priv {
 	struct net_connection *tftp_con;
 	int push;
 	uint16_t block;
 	uint16_t last_block;
+	uint16_t ack_block;
 	int state;
 	int err;
 	char *filename;
@@ -83,7 +92,7 @@ struct file_priv {
 	struct kfifo *fifo;
 	void *buf;
 	int blocksize;
-	int block_requested;
+	unsigned int windowsize;
 	bool is_getattr;
 };
 
@@ -114,6 +123,7 @@ static int tftp_send(struct file_priv *priv)
 	int len = 0;
 	uint16_t *s;
 	unsigned char *pkt = net_udp_get_payload(priv->tftp_con);
+	unsigned int window_size;
 	int ret;
 
 	pr_vdebug("%s: state %s\n", __func__, tftp_states[priv->state]);
@@ -121,6 +131,15 @@ static int tftp_send(struct file_priv *priv)
 	switch (priv->state) {
 	case STATE_RRQ:
 	case STATE_WRQ:
+		if (priv->push || priv->is_getattr)
+			/* atm, windowsize is suported only for RRQ and there
+			   is no need to request a full window when we are
+			   just looking up file attributes */
+			window_size = 1;
+		else
+			window_size = min_t(unsigned int, g_tftp_window_size,
+					    TFTP_MAX_WINDOW_SIZE);
+
 		xp = pkt;
 		s = (uint16_t *)pkt;
 		if (priv->state == STATE_RRQ)
@@ -148,18 +167,24 @@ static int tftp_send(struct file_priv *priv)
 				   operations, */
 			       priv->is_getattr ? TFTP_BLOCK_SIZE : TFTP_MTU_SIZE);
 		pkt++;
+
+		if (window_size > 1)
+			pkt += sprintf((unsigned char *)pkt,
+				       "windowsize%c%u%c",
+				       '\0', window_size,
+				       '\0');
+
 		len = pkt - xp;
 		break;
 
 	case STATE_RDATA:
-		if (priv->block == priv->block_requested)
-			return 0;
 	case STATE_OACK:
 		xp = pkt;
 		s = (uint16_t *)pkt;
 		*s++ = htons(TFTP_ACK);
-		*s++ = htons(priv->block);
-		priv->block_requested = priv->block;
+		*s++ = htons(priv->last_block);
+		priv->ack_block  = priv->last_block;
+		priv->ack_block += priv->windowsize;
 		pkt = (unsigned char *)s;
 		len = pkt - xp;
 		break;
@@ -202,7 +227,6 @@ static int tftp_poll(struct file_priv *priv)
 	if (is_timeout(priv->resend_timeout, TFTP_RESEND_TIMEOUT)) {
 		printf("T ");
 		priv->resend_timeout = get_time_ns();
-		priv->block_requested = -1;
 		return TFTP_ERR_RESEND;
 	}
 
@@ -239,6 +263,8 @@ static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
 			priv->filesize = simple_strtoull(val, NULL, 10);
 		if (!strcmp(opt, "blksize"))
 			priv->blocksize = simple_strtoul(val, NULL, 10);
+		if (!strcmp(opt, "windowsize"))
+			priv->windowsize = simple_strtoul(val, NULL, 10);
 		pr_debug("OACK opt: %s val: %s\n", opt, val);
 		s = val + strlen(val) + 1;
 	}
@@ -326,6 +352,7 @@ static void tftp_recv(struct file_priv *priv,
 			/* send ACK */
 			priv->state = STATE_OACK;
 			priv->block = 0;
+			priv->last_block = 0;
 			tftp_send(priv);
 		}
 
@@ -349,9 +376,9 @@ static void tftp_recv(struct file_priv *priv,
 			}
 		}
 
-		if (priv->block == priv->last_block)
-			/* Same block again; ignore it. */
+		if (priv->block != (uint16_t)(priv->last_block + 1)) {
 			break;
+		}
 
 		tftp_timer_reset(priv);
 		tftp_put_data(priv, priv->block, pkt + 2, len);
@@ -414,7 +441,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
 	priv->err = -EINVAL;
 	priv->filename = dpath(dentry, fsdev->vfsmount.mnt_root);
 	priv->blocksize = TFTP_BLOCK_SIZE;
-	priv->block_requested = -1;
+	priv->windowsize = 1;
 	priv->is_getattr = is_getattr;
 
 	priv->fifo = kfifo_alloc(TFTP_FIFO_SIZE);
@@ -574,7 +601,7 @@ static int tftp_read(struct device_d *dev, FILE *f, void *buf, size_t insize)
 		if (priv->state == STATE_DONE)
 			return outsize;
 
-		if (TFTP_FIFO_SIZE - kfifo_len(priv->fifo) >= priv->blocksize)
+		if (priv->last_block == priv->ack_block)
 			tftp_send(priv);
 
 		ret = tftp_poll(priv);
@@ -766,6 +793,8 @@ static struct fs_driver_d tftp_driver = {
 
 static int tftp_init(void)
 {
+	globalvar_add_simple_int("tftp.windowsize", &g_tftp_window_size, "%u");
+
 	return register_fs_driver(&tftp_driver);
 }
 coredevice_initcall(tftp_init);
-- 
2.37.1

Re: [PATCH v2 09/13] tftp: implement 'windowsize' (RFC 7440) support

[Sascha Hauer]

On Sun, Jul 31, 2022 at 01:36:44PM +0200, Enrico Scholz wrote:
> Results (with the reorder patch; numbers in bytes/s) on an iMX8MP are:
> 
>  | windowsize | VPN       | 1 Gb/s     | 100 Mb/s   |
>  |------------|-----------|------------|------------|
>  | 128        | 3.869.284 | 98.643.085 | 11.434.852 |
>  |  64        | 3.863.581 | 98.550.375 | 11.434.852 |
>  |  48        | 3.431.580 | 94.211.680 | 11.275.010 |
>  |  32        | 2.835.129 | 85.250.081 | 10.985.605 |
>  |  24        | 2.344.858 | 77.787.537 | 10.765.667 |
>  |  16        | 1.734.186 | 67.519.381 | 10.210.087 |
>  |  12        | 1.403.340 | 61.972.576 |  9.915.612 |
>  |   8        | 1.002.462 | 50.852.376 |  9.016.130 |
>  |   6        |   775.573 | 42.781.558 |  8.422.297 |
>  |   4        |   547.845 | 32.066.544 |  6.835.567 |
>  |   3        |   412.987 | 26.526.081 |  6.322.435 |
>  |   2        |   280.987 | 19.120.641 |  5.494.241 |
>  |   1        |   141.699 | 10.431.516 |  2.967.224 |
> 
> (VPN = OpenVPN on ADSL 50 Mb/s).
> 
> The window size can be configured at runtime.
> 
> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> ---
>  fs/Kconfig | 14 ++++++++++++++
>  fs/tftp.c  | 51 ++++++++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 54 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/Kconfig b/fs/Kconfig
> index aeba00073..0c4934285 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -43,6 +43,20 @@ config FS_TFTP
>  	prompt "tftp support"
>  	depends on NET
>  
> +config FS_TFTP_MAX_WINDOW_SIZE
> +	int
> +	prompt "maximum tftp window size (RFC 7440)"
> +	depends on FS_TFTP
> +	default 128
> +	range 1 128
> +	help
> +	  The maximum allowed tftp "windowsize" (RFC 7440).  Higher
> +	  value increase speed of the tftp download with the cost of
> +	  memory (1432 bytes per slot).
> +
> +	  Requires tftp "windowsize" (RFC 7440) support on server side
> +	  to have an effect.

Can we agree on some sane default here and drop this from Kconfig?
Same for FS_TFTP_REORDER_CACHE_SIZE.

> +
>  config FS_OMAP4_USBBOOT
>  	bool
>  	prompt "Filesystem over usb boot"
> diff --git a/fs/tftp.c b/fs/tftp.c
> index ef2ce0200..b8950f250 100644
> --- a/fs/tftp.c
> +++ b/fs/tftp.c
> @@ -29,7 +29,9 @@
>  #include <init.h>
>  #include <linux/stat.h>
>  #include <linux/err.h>
> +#include <linux/kernel.h>
>  #include <kfifo.h>
> +#include <globalvar.h>
>  #include <linux/sizes.h>
>  
>  #define TFTP_PORT	69	/* Well known TFTP port number */
> @@ -65,15 +67,22 @@
>  
>  #define TFTP_BLOCK_SIZE		512	/* default TFTP block size */
>  #define TFTP_MTU_SIZE		1432	/* MTU based block size */
> -#define TFTP_FIFO_SIZE		4096
> +#define TFTP_MAX_WINDOW_SIZE	CONFIG_FS_TFTP_MAX_WINDOW_SIZE
> +
> +/* calculate fifo so that it can hold the complete window plus the incoming
> +   packet.  Add some extra space just in case...  */
> +#define TFTP_FIFO_SIZE		(TFTP_MTU_SIZE * TFTP_MAX_WINDOW_SIZE + 2048)

Memory should be allocated according to the actual window size, not to
the maximum window size. Otherwise I don't see a reason to decrease the
window size using global.tftp.windowsize when the memory allocated is
always the same.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 09/13] tftp: implement 'windowsize' (RFC 7440) support

[Enrico Scholz]

Sascha Hauer <sha@pengutronix.de> writes:

>> +config FS_TFTP_MAX_WINDOW_SIZE
>> +	int
>> +	prompt "maximum tftp window size (RFC 7440)"
>> +	depends on FS_TFTP
>> +	default 128
>> +	range 1 128
>> +	help
>> +	  The maximum allowed tftp "windowsize" (RFC 7440).  Higher
>> +	  value increase speed of the tftp download with the cost of
>> +	  memory (1432 bytes per slot).
>> +
>> +	  Requires tftp "windowsize" (RFC 7440) support on server side
>> +	  to have an effect.
>
> Can we agree on some sane default here and drop this from Kconfig?

I think, it is difficult to find a sane default.  Processors like iMX8
can deal with large window sizes, but low end ones like iMX6ULL can not
handle data fast enough and require smaller sizes (around max 30).

Workloads like writing to slow memory (cp /mnt/tftp/... /dev/nand) might
require smaller sizes too.  Or networks with high drop rates.

That's why, I think it should be possible to configure it in some
_defconfig.


>> +/* calculate fifo so that it can hold the complete window plus the incoming
>> +   packet.  Add some extra space just in case...  */
>> +#define TFTP_FIFO_SIZE		(TFTP_MTU_SIZE * TFTP_MAX_WINDOW_SIZE + 2048)
>
> Memory should be allocated according to the actual window size, not to
> the maximum window size.

ok; I will try it.  Moving 'kfifo_alloc()' should be possible in

| static struct file_priv *tftp_do_open(struct device_d *dev,
| 		int accmode, struct dentry *dentry, bool is_getattr)
| 
| 	priv->fifo = kfifo_alloc(TFTP_FIFO_SIZE);
| 
| 		ret = tftp_poll(priv);


> Otherwise I don't see a reason to decrease the window size using
> global.tftp.windowsize when the memory allocated is always the same.

As I said above: some workloads or networks with a high packet drop rate
work better with lower window sizes.



Enrco

Re: [PATCH v2 09/13] tftp: implement 'windowsize' (RFC 7440) support

[Sascha Hauer]

On Tue, Aug 09, 2022 at 11:28:16AM +0200, Enrico Scholz wrote:
> Sascha Hauer <sha@pengutronix.de> writes:
> 
> >> +config FS_TFTP_MAX_WINDOW_SIZE
> >> +	int
> >> +	prompt "maximum tftp window size (RFC 7440)"
> >> +	depends on FS_TFTP
> >> +	default 128
> >> +	range 1 128
> >> +	help
> >> +	  The maximum allowed tftp "windowsize" (RFC 7440).  Higher
> >> +	  value increase speed of the tftp download with the cost of
> >> +	  memory (1432 bytes per slot).
> >> +
> >> +	  Requires tftp "windowsize" (RFC 7440) support on server side
> >> +	  to have an effect.
> >
> > Can we agree on some sane default here and drop this from Kconfig?
> 
> I think, it is difficult to find a sane default.  Processors like iMX8
> can deal with large window sizes, but low end ones like iMX6ULL can not
> handle data fast enough and require smaller sizes (around max 30).
> 
> Workloads like writing to slow memory (cp /mnt/tftp/... /dev/nand) might
> require smaller sizes too.  Or networks with high drop rates.
> 
> That's why, I think it should be possible to configure it in some
> _defconfig.

It's exposed to global.tftp.windowsize and with this you can store a
good value on the device and you can provide different defaults for different
boards in their default environments during compile time. It's much more
flexible than Kconfig.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 10/13] tftp: do not use 'priv->block' for RRQ

[Enrico Scholz]

attribute is not used outside tftp_recv() for RRQ.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index 7f836d36b7df..f85136f03e22 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -298,6 +298,7 @@ static void tftp_recv(struct file_priv *priv,
 			uint8_t *pkt, unsigned len, uint16_t uh_sport)
 {
 	uint16_t opcode;
+	uint16_t block;
 
 	/* according to RFC1350 minimal tftp packet length is 4 bytes */
 	if (len < 4)
@@ -349,7 +350,6 @@ static void tftp_recv(struct file_priv *priv,
 		} else {
 			/* send ACK */
 			priv->state = STATE_OACK;
-			priv->block = 0;
 			priv->last_block = 0;
 			tftp_send(priv);
 		}
@@ -357,7 +357,7 @@ static void tftp_recv(struct file_priv *priv,
 		break;
 	case TFTP_DATA:
 		len -= 2;
-		priv->block = ntohs(*(uint16_t *)pkt);
+		block = ntohs(*(uint16_t *)pkt);
 
 		if (priv->state == STATE_RRQ || priv->state == STATE_OACK) {
 			/* first block received */
@@ -365,21 +365,21 @@ static void tftp_recv(struct file_priv *priv,
 			priv->tftp_con->udp->uh_dport = uh_sport;
 			priv->last_block = 0;
 
-			if (priv->block != 1) {	/* Assertion */
+			if (block != 1) {	/* Assertion */
 				pr_err("error: First block is not block 1 (%d)\n",
-					priv->block);
+					block);
 				priv->err = -EINVAL;
 				priv->state = STATE_DONE;
 				break;
 			}
 		}
 
-		if (priv->block != (uint16_t)(priv->last_block + 1)) {
+		if (block != (uint16_t)(priv->last_block + 1)) {
 			break;
 		}
 
 		tftp_timer_reset(priv);
-		tftp_put_data(priv, priv->block, pkt + 2, len);
+		tftp_put_data(priv, block, pkt + 2, len);
 
 		break;
 
-- 
2.36.1

[PATCH 11/13] tftp: reorder tftp packets

[Enrico Scholz]

With the tftp "windowsize" option, reordering of udp datagrams becomes
an issue.  Depending on the network topology, this reordering occurs
several times with large tftp transfers and will heavily reduce the
transfer speed.

This patch adds a packet cache so that datagrams can be reassembled in
the correct order.

Because it increases memory usage, it is an Kconfig option.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/Kconfig |  22 +++++++
 fs/tftp.c  | 165 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 181 insertions(+), 6 deletions(-)

diff --git a/fs/Kconfig b/fs/Kconfig
index 0c4934285942..02aa25d6abf7 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -57,6 +57,28 @@ config FS_TFTP_MAX_WINDOW_SIZE
 	  Requires tftp "windowsize" (RFC 7440) support on server side
 	  to have an effect.
 
+config FS_TFTP_REORDER_CACHE_SIZE
+	int
+	prompt "number of out-of-order tftp packets to be cached"
+	depends on FS_TFTP
+	default 16 if FS_TFTP_MAX_WINDOW_SIZE > 16
+	default  0 if FS_TFTP_MAX_WINDOW_SIZE = 1
+        ## TODO: it should be 'FS_TFTP_MAX_WINDOW_SIZE - 1' but this
+        ## is not supported by Kconfig
+	default FS_TFTP_MAX_WINDOW_SIZE
+	range 0 FS_TFTP_MAX_WINDOW_SIZE
+	help
+	  UDP allows reordering of datagrams; with this option,
+	  unexpected tftp packets will be cached and later
+	  reassembled.  This increases stability of the tftp download
+	  with the cost of memory (around 1440 bytes per cache
+	  element).
+
+          A value of 0 disables caching.
+
+	  Requires tftp "windowsize" (RFC 7440) support on server side
+	  to have an effect.
+
 config FS_OMAP4_USBBOOT
 	bool
 	prompt "Filesystem over usb boot"
diff --git a/fs/tftp.c b/fs/tftp.c
index f85136f03e22..2c2ff081be51 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -68,6 +68,9 @@
 #define TFTP_MTU_SIZE		1432	/* MTU based block size */
 #define TFTP_MAX_WINDOW_SIZE	CONFIG_FS_TFTP_MAX_WINDOW_SIZE
 
+/* size of cache which deals with udp reordering */
+#define TFTP_WINDOW_CACHE_NUM	CONFIG_FS_TFTP_REORDER_CACHE_SIZE
+
 /* calculate fifo so that it can hold the complete window plus the incoming
    packet.  Add some extra space just in case...  */
 #define TFTP_FIFO_SIZE		(TFTP_MTU_SIZE * TFTP_MAX_WINDOW_SIZE + 2048)
@@ -76,6 +79,15 @@
 
 static int g_tftp_window_size = TFTP_MAX_WINDOW_SIZE / 1;
 
+struct tftp_block {
+	uint16_t id;
+	uint8_t data[TFTP_MTU_SIZE];
+
+	/* len will not exceed TFTP_MTU_SIZE; 14 bits should suffice... */
+	uint16_t len:14;
+	bool valid:1;
+};
+
 struct file_priv {
 	struct net_connection *tftp_con;
 	int push;
@@ -93,12 +105,49 @@ struct file_priv {
 	int blocksize;
 	unsigned int windowsize;
 	bool is_getattr;
+#if TFTP_WINDOW_CACHE_NUM > 0
+	struct tftp_block window_cache[TFTP_WINDOW_CACHE_NUM];
+#endif
 };
 
 struct tftp_priv {
 	IPaddr_t server;
 };
 
+inline static size_t tftp_window_cache_size(struct file_priv const *priv)
+{
+#if TFTP_WINDOW_CACHE_NUM > 0
+	return min_t(unsigned int, priv->windowsize - 1,
+		     ARRAY_SIZE(priv->window_cache));
+#else
+	return 0;
+#endif
+}
+
+inline static struct tftp_block *tftp_window_cache_get(struct file_priv *priv,
+						       size_t idx)
+{
+#if TFTP_WINDOW_CACHE_NUM > 0
+	return &priv->window_cache[idx];
+#else
+	return NULL;
+#endif
+}
+
+static void tftp_window_cache_clear(struct file_priv *priv)
+{
+	size_t i;
+
+	for (i = 0; i < tftp_window_cache_size(priv); ++i) {
+		struct tftp_block *block = tftp_window_cache_get(priv, i);
+
+		if (block->valid)
+			pr_debug("cached item #%d still valid\n", block->id);
+
+		block->valid = false;
+	}
+}
+
 static int tftp_truncate(struct device_d *dev, FILE *f, loff_t size)
 {
 	return 0;
@@ -185,6 +234,7 @@ static int tftp_send(struct file_priv *priv)
 		priv->ack_block += priv->windowsize;
 		pkt = (unsigned char *)s;
 		len = pkt - xp;
+		tftp_window_cache_clear(priv);
 		break;
 	}
 
@@ -294,6 +344,113 @@ static void tftp_put_data(struct file_priv *priv, uint16_t block,
 	}
 }
 
+static bool in_window(uint16_t block, uint16_t start, uint16_t end)
+{
+	return ((start <= block && block <= end) ||
+		(block <= end   && end   <= start));
+}
+
+static int tftp_window_cache_insert(struct file_priv *priv, uint16_t id,
+				    void const *data, size_t len)
+{
+	size_t		i;
+
+	if (len > sizeof tftp_window_cache_get(priv, 0)->data) {
+		pr_err("datagram too large\n");
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < tftp_window_cache_size(priv); ++i) {
+		struct tftp_block *block = tftp_window_cache_get(priv, i);
+
+		if (block->valid && block->id == id) {
+			if (len == block->len &&
+			    memcmp(data, block->data, len) == 0) {
+				return 0;
+			}
+
+			pr_err("cached block #%d differs from actual one\n",
+			       id);
+
+			return -EBADMSG;
+		}
+
+		if (!block->valid) {
+			memcpy(block->data, data, len);
+			block->id = id;
+			block->len = len;
+			block->valid = true;
+
+			return 0;
+		}
+	}
+
+	return -ENOMEM;
+}
+
+static bool tftp_window_cache_apply(struct file_priv *priv)
+{
+	uint16_t exp_id = priv->last_block + 1;
+	bool res = false;
+	size_t i;
+
+	for (i = 0; i < tftp_window_cache_size(priv); ++i) {
+		struct tftp_block *block = tftp_window_cache_get(priv, i);
+
+		/* can be already the case when entering the function, or
+		   caused by tftp_put_data() below */
+		if (priv->state != STATE_RDATA)
+			break;
+
+		if (!block->valid || block->id != exp_id)
+			continue;
+
+		tftp_put_data(priv, block->id, block->data, block->len);
+
+		block->valid = false;
+		exp_id = priv->last_block + 1;
+
+		res = true;
+	}
+
+	return res;
+}
+
+static void tftp_handle_data(struct file_priv *priv, uint16_t block,
+			     void const *data, size_t len)
+{
+	uint16_t exp_block;
+	int rc;
+
+	exp_block = priv->last_block + 1;
+
+	if (exp_block == block) {
+		/* datagram over network is the expected one; put it in the
+		   fifo directly and try to apply cached items then */
+		tftp_timer_reset(priv);
+		tftp_put_data(priv, block, data, len);
+
+		while (tftp_window_cache_apply(priv)) {
+			/* noop */
+		}
+	} else if (!in_window(block, exp_block, priv->ack_block)) {
+		/* completely unexpected and unrelated to actual window;
+		   ignore the packet. */
+		printf("B");
+	} else {
+		/* The 'rc < 0' below happens e.g. when datagrams in the first
+		   part of the transfer window are dropped.
+
+		   TODO: this will usually result in a timeout
+		   (TFTP_RESEND_TIMEOUT).  It should be possible to bypass
+		   this timeout by acknowledging the last packet (e.g. by
+		   doing 'priv->ack_block = priv->last_block' here). */
+		rc = tftp_window_cache_insert(priv, block, data, len);
+		if (rc < 0)
+			printf("M");
+	}
+}
+
 static void tftp_recv(struct file_priv *priv,
 			uint8_t *pkt, unsigned len, uint16_t uh_sport)
 {
@@ -364,6 +521,7 @@ static void tftp_recv(struct file_priv *priv,
 			priv->state = STATE_RDATA;
 			priv->tftp_con->udp->uh_dport = uh_sport;
 			priv->last_block = 0;
+			tftp_window_cache_clear(priv);
 
 			if (block != 1) {	/* Assertion */
 				pr_err("error: First block is not block 1 (%d)\n",
@@ -374,12 +532,7 @@ static void tftp_recv(struct file_priv *priv,
 			}
 		}
 
-		if (block != (uint16_t)(priv->last_block + 1)) {
-			break;
-		}
-
-		tftp_timer_reset(priv);
-		tftp_put_data(priv, block, pkt + 2, len);
+		tftp_handle_data(priv, block, pkt + 2, len);
 
 		break;
 
-- 
2.36.1

Re: [PATCH 11/13] tftp: reorder tftp packets

[Sascha Hauer]

On Mon, Jul 18, 2022 at 02:22:26PM +0200, Enrico Scholz wrote:
> With the tftp "windowsize" option, reordering of udp datagrams becomes
> an issue.  Depending on the network topology, this reordering occurs
> several times with large tftp transfers and will heavily reduce the
> transfer speed.
> 
> This patch adds a packet cache so that datagrams can be reassembled in
> the correct order.
> 
> Because it increases memory usage, it is an Kconfig option.
> 
> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> ---
>  fs/Kconfig |  22 +++++++
>  fs/tftp.c  | 165 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 181 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/Kconfig b/fs/Kconfig
> index 0c4934285942..02aa25d6abf7 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -57,6 +57,28 @@ config FS_TFTP_MAX_WINDOW_SIZE
>  	  Requires tftp "windowsize" (RFC 7440) support on server side
>  	  to have an effect.
>  
> +config FS_TFTP_REORDER_CACHE_SIZE
> +	int
> +	prompt "number of out-of-order tftp packets to be cached"
> +	depends on FS_TFTP
> +	default 16 if FS_TFTP_MAX_WINDOW_SIZE > 16
> +	default  0 if FS_TFTP_MAX_WINDOW_SIZE = 1
> +        ## TODO: it should be 'FS_TFTP_MAX_WINDOW_SIZE - 1' but this
> +        ## is not supported by Kconfig
> +	default FS_TFTP_MAX_WINDOW_SIZE
> +	range 0 FS_TFTP_MAX_WINDOW_SIZE
> +	help
> +	  UDP allows reordering of datagrams; with this option,
> +	  unexpected tftp packets will be cached and later
> +	  reassembled.  This increases stability of the tftp download
> +	  with the cost of memory (around 1440 bytes per cache
> +	  element).
> +
> +          A value of 0 disables caching.
> +
> +	  Requires tftp "windowsize" (RFC 7440) support on server side
> +	  to have an effect.
> +
>  config FS_OMAP4_USBBOOT
>  	bool
>  	prompt "Filesystem over usb boot"
> diff --git a/fs/tftp.c b/fs/tftp.c
> index f85136f03e22..2c2ff081be51 100644
> --- a/fs/tftp.c
> +++ b/fs/tftp.c
> @@ -68,6 +68,9 @@
>  #define TFTP_MTU_SIZE		1432	/* MTU based block size */
>  #define TFTP_MAX_WINDOW_SIZE	CONFIG_FS_TFTP_MAX_WINDOW_SIZE
>  
> +/* size of cache which deals with udp reordering */
> +#define TFTP_WINDOW_CACHE_NUM	CONFIG_FS_TFTP_REORDER_CACHE_SIZE
> +
>  /* calculate fifo so that it can hold the complete window plus the incoming
>     packet.  Add some extra space just in case...  */
>  #define TFTP_FIFO_SIZE		(TFTP_MTU_SIZE * TFTP_MAX_WINDOW_SIZE + 2048)
> @@ -76,6 +79,15 @@
>  
>  static int g_tftp_window_size = TFTP_MAX_WINDOW_SIZE / 1;
>  
> +struct tftp_block {
> +	uint16_t id;
> +	uint8_t data[TFTP_MTU_SIZE];
> +
> +	/* len will not exceed TFTP_MTU_SIZE; 14 bits should suffice... */
> +	uint16_t len:14;
> +	bool valid:1;
> +};
> +
>  struct file_priv {
>  	struct net_connection *tftp_con;
>  	int push;
> @@ -93,12 +105,49 @@ struct file_priv {
>  	int blocksize;
>  	unsigned int windowsize;
>  	bool is_getattr;
> +#if TFTP_WINDOW_CACHE_NUM > 0
> +	struct tftp_block window_cache[TFTP_WINDOW_CACHE_NUM];
> +#endif
>  };

Can you use a struct list_head here rather than an array? With that
you can use list_add_sort() to sort the cached packets by id and it
becomes easy to see if the next packet is already there or not without
iterating over the cache multiple times. Also by allocating a tftp_block
dynamically when you need it you only occupy memory for packets that are
received in the wrong order, not for a fixed number of blocks.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 12/13] tftp: allow to change tftp port

[Enrico Scholz]

useful e.g. when working with a local, non-privileged tftp server

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index 2c2ff081be51..400209c26023 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -78,6 +78,7 @@
 #define TFTP_ERR_RESEND	1
 
 static int g_tftp_window_size = TFTP_MAX_WINDOW_SIZE / 1;
+static int g_tftp_port = TFTP_PORT;
 
 struct tftp_block {
 	uint16_t id;
@@ -600,7 +601,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
 		goto out;
 	}
 
-	priv->tftp_con = net_udp_new(tpriv->server, TFTP_PORT, tftp_handler,
+	priv->tftp_con = net_udp_new(tpriv->server, g_tftp_port, tftp_handler,
 			priv);
 	if (IS_ERR(priv->tftp_con)) {
 		ret = PTR_ERR(priv->tftp_con);
@@ -944,6 +945,7 @@ static struct fs_driver_d tftp_driver = {
 static int tftp_init(void)
 {
 	globalvar_add_simple_int("tftp.windowsize", &g_tftp_window_size, "%u");
+	globalvar_add_simple_int("tftp.port", &g_tftp_port, "%u");
 
 	return register_fs_driver(&tftp_driver);
 }
-- 
2.36.1

Re: [PATCH 12/13] tftp: allow to change tftp port

[Sascha Hauer]

On Mon, Jul 18, 2022 at 02:22:27PM +0200, Enrico Scholz wrote:
> useful e.g. when working with a local, non-privileged tftp server
> 
> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> ---
>  fs/tftp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/tftp.c b/fs/tftp.c
> index 2c2ff081be51..400209c26023 100644
> --- a/fs/tftp.c
> +++ b/fs/tftp.c
> @@ -78,6 +78,7 @@
>  #define TFTP_ERR_RESEND	1
>  
>  static int g_tftp_window_size = TFTP_MAX_WINDOW_SIZE / 1;
> +static int g_tftp_port = TFTP_PORT;
>  
>  struct tftp_block {
>  	uint16_t id;
> @@ -600,7 +601,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
>  		goto out;
>  	}
>  
> -	priv->tftp_con = net_udp_new(tpriv->server, TFTP_PORT, tftp_handler,
> +	priv->tftp_con = net_udp_new(tpriv->server, g_tftp_port, tftp_handler,
>  			priv);
>  	if (IS_ERR(priv->tftp_con)) {
>  		ret = PTR_ERR(priv->tftp_con);
> @@ -944,6 +945,7 @@ static struct fs_driver_d tftp_driver = {
>  static int tftp_init(void)
>  {
>  	globalvar_add_simple_int("tftp.windowsize", &g_tftp_window_size, "%u");
> +	globalvar_add_simple_int("tftp.port", &g_tftp_port, "%u");

The port is connection specific. I think we should rather pass the port
through mount using -o, something like mount -o port=4711 <server>
<path>. Grep for "parseopt_hu" to see how this can be done.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 13/13] tftp: add sanity check for OACK response

[Enrico Scholz]

Catch bad 'blocksize' or 'windowsize' responses from the server.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index 400209c26023..c6491b537481 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -290,7 +290,7 @@ static int tftp_poll(struct file_priv *priv)
 	return 0;
 }
 
-static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
+static int tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
 {
 	unsigned char *opt, *val, *s;
 
@@ -307,7 +307,7 @@ static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
 		opt = s;
 		val = s + strlen(s) + 1;
 		if (val > s + len)
-			return;
+			break;
 		if (!strcmp(opt, "tsize"))
 			priv->filesize = simple_strtoull(val, NULL, 10);
 		if (!strcmp(opt, "blksize"))
@@ -317,6 +317,15 @@ static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
 		pr_debug("OACK opt: %s val: %s\n", opt, val);
 		s = val + strlen(val) + 1;
 	}
+
+	if (priv->blocksize > TFTP_MTU_SIZE ||
+	    priv->windowsize > TFTP_MAX_WINDOW_SIZE ||
+	    priv->windowsize == 0) {
+		pr_warn("tftp: invalid oack response");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static void tftp_timer_reset(struct file_priv *priv)
@@ -498,10 +507,12 @@ static void tftp_recv(struct file_priv *priv,
 		break;
 
 	case TFTP_OACK:
-		tftp_parse_oack(priv, pkt, len);
 		priv->tftp_con->udp->uh_dport = uh_sport;
 
-		if (priv->push) {
+		if (tftp_parse_oack(priv, pkt, len) < 0) {
+			priv->err = -EINVAL;
+			priv->state = STATE_DONE;
+		} else if (priv->push) {
 			/* send first block */
 			priv->state = STATE_WDATA;
 			priv->block = 1;
-- 
2.36.1

[PATCH v2 13/13] tftp: add sanity check for OACK response

[Enrico Scholz]

Catch bad 'blocksize' or 'windowsize' responses from the server.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index cfeb94b21..93b0606d3 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -292,7 +292,7 @@ static int tftp_poll(struct file_priv *priv)
 	return 0;
 }
 
-static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
+static int tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
 {
 	unsigned char *opt, *val, *s;
 
@@ -309,7 +309,7 @@ static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
 		opt = s;
 		val = s + strlen(s) + 1;
 		if (val > s + len)
-			return;
+			break;
 		if (!strcmp(opt, "tsize"))
 			priv->filesize = simple_strtoull(val, NULL, 10);
 		if (!strcmp(opt, "blksize"))
@@ -319,6 +319,15 @@ static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
 		pr_debug("OACK opt: %s val: %s\n", opt, val);
 		s = val + strlen(val) + 1;
 	}
+
+	if (priv->blocksize > TFTP_MTU_SIZE ||
+	    priv->windowsize > TFTP_MAX_WINDOW_SIZE ||
+	    priv->windowsize == 0) {
+		pr_warn("tftp: invalid oack response\n");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static void tftp_timer_reset(struct file_priv *priv)
@@ -500,10 +509,12 @@ static void tftp_recv(struct file_priv *priv,
 		break;
 
 	case TFTP_OACK:
-		tftp_parse_oack(priv, pkt, len);
 		priv->tftp_con->udp->uh_dport = uh_sport;
 
-		if (priv->push) {
+		if (tftp_parse_oack(priv, pkt, len) < 0) {
+			priv->err = -EINVAL;
+			priv->state = STATE_DONE;
+		} else if (priv->push) {
 			/* send first block */
 			priv->state = STATE_WDATA;
 			priv->block = 1;
-- 
2.37.1

Re: [PATCH 00/13] add "windowsize" (RFC 7440) support for tftp

[Sascha Hauer]

Hi Enrico,

On Mon, Jul 18, 2022 at 02:22:15PM +0200, Enrico Scholz wrote:
> The tftp "windowsize" greatly improves the performance of tftp
> transfers.  This patchset adds support for it.

TCP for the poor, nice ;)

Very nice stuff indeed. I gave it a quick test and TFTP works as before,
that's good to start with, but to gain speed improvements I need a
RFC7440 capable server on the other end. Which server did you test it
with? Have any RFC7440 capable servers made it to the distributions
already?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 00/13] add "windowsize" (RFC 7440) support for tftp

[Enrico Scholz]

Sascha Hauer <sha@pengutronix.de> writes:

>> The tftp "windowsize" greatly improves the performance of tftp
>> transfers.  This patchset adds support for it.
>
> TCP for the poor, nice ;)
>
> Very nice stuff indeed. I gave it a quick test and TFTP works as before,
> that's good to start with, but to gain speed improvements I need a
> RFC7440 capable server on the other end. Which server did you test it
> with? Have any RFC7440 capable servers made it to the distributions
> already?

https://github.com/ensc/r-tftpd ;)

yes... it is problem to implement both server and client side.  But the
window size feature also works with PXE clients and u-boot, so I think
it is correct.

I started to implement it with transparent proxying support in mind
(e.g. translate tftp requests to http ones, or fetch cached images with
'rsync').

But when I saw the effects of windows size support, I focused on that.


Enrico