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

[PATCH v3 00/18] 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 speeds, 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 |


The window size related parts of the patchset (with deactivated
selftest) increase the barebox binary size by

| add/remove: 4/0 grow/shrink: 8/2 up/down: 1269/-32 (1237)
| Function                                     old     new   delta
| tftp_handler                                 756    1200    +444
| tftp_put_data                                  -     184    +184
| tftp_do_open                                 428     608    +180
| tftp_window_cache_remove                       -     124    +124
| tftp_window_cache_get_pos                      -     120    +120
| tftp_send                                    296     392     +96
| tftp_do_close                                260     312     +52
| tftp_init                                     16      60     +44
| __FUNCTION__                                 610     623     +13
| tftp_open                                     64      68      +4
| tftp_lookup                                  136     140      +4
| g_tftp_window_size                             -       4      +4
| tftp_read                                    180     164     -16
| tftp_poll                                    180     164     -16
| Total: Before=626114, After=627351, chg +0.20%

Turning of the datagram cache (CONFIG_FS_TFTP_REORDER_CACHE_SIZE=0)
reduces the overhead to

| add/remove: 1/0 grow/shrink: 7/2 up/down: 537/-32 (505)
| Function                                     old     new   delta
| tftp_handler                                 756     992    +236
| tftp_do_open                                 428     564    +136
| tftp_send                                    296     392     +96
| tftp_init                                     16      60     +44
| __FUNCTION__                                 610     623     +13
| tftp_open                                     64      68      +4
| tftp_lookup                                  136     140      +4
| g_tftp_window_size                             -       4      +4
| tftp_read                                    180     164     -16
| tftp_poll                                    180     164     -16
| Total: Before=626114, After=626619, chg +0.08%

Restoring the old behaviour by CONFIG_FS_TFTP_MAX_WINDOW_SIZE=1 shows
an overhead of

| add/remove: 1/0 grow/shrink: 7/2 up/down: 449/-32 (417)
| Function                                     old     new   delta
| tftp_handler                                 756     988    +232
| tftp_do_open                                 428     564    +136
| tftp_init                                     16      60     +44
| __FUNCTION__                                 610     623     +13
| tftp_send                                    296     308     +12
| tftp_open                                     64      68      +4
| tftp_lookup                                  136     140      +4
| g_tftp_window_size                             -       4      +4
| tftp_read                                    180     164     -16
| tftp_poll                                    180     164     -16
| Total: Before=626114, After=626531, chg +0.07%


---
v2 -> v3
  - use "port=XX" mount options instead of global 'tftp.port' variable
  - allocate fifo and send buffer dynamically based on block- and
    window size of the transfer.  Do not use fixed constants anymore
  - rewritten cache code; use bitmap based functions with O(1)
    complexity instead of iterating over (small) arrays
  - unittest for cache functions
  - add information about binary sizes

v1 -> v2
  - fixes for non rfc7440 servers
---
Enrico Scholz (18):
  progress: add close_progress() to display some statistics
  libfile:copy_file: show statistics in verbose mode
  tftp: add some 'const' annotations
  tftp: allow to change tftp port
  cmd:tftp: add '-P' option to set tftp server port number
  tftp: minor refactoring of RRQ/WRQ packet generation code
  tftp: replace hardcoded blksize by global constant
  tftp: allocate buffers and fifo dynamically
  tftp: add sanity check for OACK response
  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: add debug_assert() macro
  tftp: reorder tftp packets
  tftp: add selftest

 commands/tftp.c     |  22 +-
 fs/Kconfig          |  36 +++
 fs/tftp-selftest.h  |  56 ++++
 fs/tftp.c           | 640 +++++++++++++++++++++++++++++++++++++++-----
 include/progress.h  |   1 +
 lib/libfile.c       |   3 +
 lib/show_progress.c |  25 ++
 test/self/Kconfig   |   7 +
 8 files changed, 717 insertions(+), 73 deletions(-)
 create mode 100644 fs/tftp-selftest.h

-- 
2.37.1

[PATCH v3 01/18] 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.37.1

Re: [PATCH v3 01/18] progress: add close_progress() to display some statistics

[Sascha Hauer]

On Mon, Aug 15, 2022 at 10:42:05AM +0200, Enrico Scholz wrote:
> 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);
> +}

show_progress() doesn't necessarily show the progress in bytes, it could
also be percent, megabytes or anything else. We could maybe rename this
to close_progress_bytes(). Also it would be nice to print the sizes in
human readable form, maybe size_human_readable() could be used here.

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 v3 01/18] progress: add close_progress() to display some statistics

[Enrico Scholz]

Sascha Hauer <sha@pengutronix.de> writes:

>> +void close_progress(loff_t total)
>> +{
>
> show_progress() doesn't necessarily show the progress in bytes, it could
> also be percent, megabytes or anything else. We could maybe rename this
> to close_progress_bytes().

When I started this patch, I had an

| enum progress_summary {
|     SUMMARY_NONE,
|     SUMMARY_BYTES,
| }

argument in this function.  But it felt too overdesigned at this time.


> Also it would be nice to print the sizes in human readable form, maybe
> size_human_readable() could be used here.

Yes; I played with human readable sizes/speeds too.  But for developing,
I needed the exact size.

Support for the "'" grouping flag in printf() might be a more generic
approach.



Enrico

[PATCH v3 02/18] 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.37.1

[PATCH v3 03/18] tftp: add some 'const' annotations

[Enrico Scholz]

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

diff --git a/fs/tftp.c b/fs/tftp.c
index d186e7983a6d..c26204ae76e6 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -94,7 +94,7 @@ static int tftp_truncate(struct device_d *dev, FILE *f, loff_t size)
 	return 0;
 }
 
-static char *tftp_states[] = {
+static char const * const tftp_states[] = {
 	[STATE_INVALID] = "INVALID",
 	[STATE_RRQ] = "RRQ",
 	[STATE_WRQ] = "WRQ",
-- 
2.37.1

[PATCH v3 04/18] tftp: allow to change tftp port

[Enrico Scholz]

This adds a 'port=<port>' mount option for tftp filesystems.

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 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index c26204ae76e6..913ca1df6e42 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 <parseopt.h>
 #include <linux/sizes.h>
 
 #define TFTP_PORT	69	/* Well known TFTP port number */
@@ -376,6 +377,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
 	struct file_priv *priv;
 	struct tftp_priv *tpriv = dev->priv;
 	int ret;
+	unsigned short port = TFTP_PORT;
 
 	priv = xzalloc(sizeof(*priv));
 
@@ -405,8 +407,9 @@ 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);
+	parseopt_hu(fsdev->options, "port", &port);
+
+	priv->tftp_con = net_udp_new(tpriv->server, port, tftp_handler, priv);
 	if (IS_ERR(priv->tftp_con)) {
 		ret = PTR_ERR(priv->tftp_con);
 		goto out1;
-- 
2.37.1

[PATCH v3 05/18] cmd:tftp: add '-P' option to set tftp server port number

[Enrico Scholz]

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

diff --git a/commands/tftp.c b/commands/tftp.c
index 48ff00c6217c..6ac822c9e832 100644
--- a/commands/tftp.c
+++ b/commands/tftp.c
@@ -21,15 +21,24 @@ static int do_tftpb(int argc, char *argv[])
 	char *source, *dest, *freep;
 	int opt;
 	int tftp_push = 0;
+	int port = -1;
 	int ret;
 	IPaddr_t ip;
 	char ip4_str[sizeof("255.255.255.255")];
+	char mount_opts[sizeof("port=12345")];
 
-	while ((opt = getopt(argc, argv, "p")) > 0) {
+	while ((opt = getopt(argc, argv, "pP:")) > 0) {
 		switch(opt) {
 		case 'p':
 			tftp_push = 1;
 			break;
+		case 'P':
+			port = simple_strtoul(optarg, NULL, 0);
+			if (port <= 0 || port > 0xffff) {
+				pr_err("invalid port '%s'\n", optarg);
+				return COMMAND_ERROR_USAGE;
+			}
+			break;
 		default:
 			return COMMAND_ERROR_USAGE;
 		}
@@ -59,7 +68,13 @@ static int do_tftpb(int argc, char *argv[])
 
 	ip = net_get_serverip();
 	sprintf(ip4_str, "%pI4", &ip);
-	ret = mount(ip4_str, "tftp", TFTP_MOUNT_PATH, NULL);
+
+	if (port >= 0)
+		sprintf(mount_opts, "port=%u", port);
+	else
+		mount_opts[0] = '\0';
+
+	ret = mount(ip4_str, "tftp", TFTP_MOUNT_PATH, mount_opts);
 	if (ret)
 		goto err_rmdir;
 
@@ -84,12 +99,13 @@ BAREBOX_CMD_HELP_TEXT("server address is taken from the environment (ethX.server
 BAREBOX_CMD_HELP_TEXT("")
 BAREBOX_CMD_HELP_TEXT("Options:")
 BAREBOX_CMD_HELP_OPT ("-p", "push to TFTP server")
+BAREBOX_CMD_HELP_OPT ("-P PORT", "tftp server port number")
 BAREBOX_CMD_HELP_END
 
 BAREBOX_CMD_START(tftp)
 	.cmd		= do_tftpb,
 	BAREBOX_CMD_DESC("load (or save) a file using TFTP")
-	BAREBOX_CMD_OPTS("[-p] SOURCE [DEST]")
+	BAREBOX_CMD_OPTS("[-p] [-P <port>] SOURCE [DEST]")
 	BAREBOX_CMD_GROUP(CMD_GRP_NET)
 	BAREBOX_CMD_HELP(cmd_tftp_help)
 BAREBOX_CMD_END
-- 
2.37.1

[PATCH v3 06/18] 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 913ca1df6e42..7627f3f59669 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -136,13 +136,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.37.1

[PATCH v3 07/18] 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 7627f3f59669..b552a5dc2f63 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -65,6 +65,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
@@ -135,14 +136,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.37.1

[PATCH v3 08/18] tftp: allocate buffers and fifo dynamically

[Enrico Scholz]

Use the actual blocksize for allocating buffers instead of assuming an
hardcoded value.

This requires to add an additional 'START' state which is entered
after receiving (RRQ) or sending (WRQ) the OACK.  Without it, the next
state would be entered and the (not allocated yet) fifo be used.

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

diff --git a/fs/tftp.c b/fs/tftp.c
index b552a5dc2f63..0b6d57c4603f 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -63,10 +63,12 @@
 #define STATE_WAITACK	6
 #define STATE_LAST	7
 #define STATE_DONE	8
+/* OACK from server has been received and we can begin to sent either the ACK
+   (for RRQ) or data (for WRQ) */
+#define STATE_START	9
 
 #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
 
@@ -106,6 +108,7 @@ static char const * const tftp_states[] = {
 	[STATE_WAITACK] = "WAITACK",
 	[STATE_LAST] = "LAST",
 	[STATE_DONE] = "DONE",
+	[STATE_START] = "START",
 };
 
 static int tftp_send(struct file_priv *priv)
@@ -294,19 +297,9 @@ static void tftp_recv(struct file_priv *priv,
 	case TFTP_OACK:
 		tftp_parse_oack(priv, pkt, len);
 		priv->tftp_con->udp->uh_dport = uh_sport;
-
-		if (priv->push) {
-			/* send first block */
-			priv->state = STATE_WDATA;
-			priv->block = 1;
-		} else {
-			/* send ACK */
-			priv->state = STATE_OACK;
-			priv->block = 0;
-			tftp_send(priv);
-		}
-
+		priv->state = STATE_START;
 		break;
+
 	case TFTP_DATA:
 		len -= 2;
 		priv->block = ntohs(*(uint16_t *)pkt);
@@ -330,6 +323,12 @@ static void tftp_recv(struct file_priv *priv,
 			/* Same block again; ignore it. */
 			break;
 
+		if (len > priv->blocksize) {
+			pr_warn("tftp: oversized packet (%u > %d) received\n",
+				len, priv->blocksize);
+			break;
+		}
+
 		priv->last_block = priv->block;
 
 		tftp_timer_reset(priv);
@@ -372,6 +371,36 @@ static void tftp_handler(void *ctx, char *packet, unsigned len)
 	tftp_recv(priv, pkt, net_eth_to_udplen(packet), udp->uh_sport);
 }
 
+
+static int tftp_start_transfer(struct file_priv *priv)
+{
+	priv->fifo = kfifo_alloc(priv->blocksize);
+	if (!priv->fifo)
+		return -ENOMEM;
+
+	if (priv->push) {
+		priv->buf = xmalloc(priv->blocksize);
+		if (!priv->buf) {
+			kfifo_free(priv->fifo);
+			priv->fifo = NULL;
+			return -ENOMEM;
+		}
+	}
+
+	if (priv->push) {
+		/* send first block */
+		priv->state = STATE_WDATA;
+		priv->block = 1;
+	} else {
+		/* send ACK */
+		priv->state = STATE_OACK;
+		priv->block = 0;
+		tftp_send(priv);
+	}
+
+	return 0;
+}
+
 static struct file_priv *tftp_do_open(struct device_d *dev,
 		int accmode, struct dentry *dentry)
 {
@@ -403,47 +432,45 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
 	priv->blocksize = TFTP_BLOCK_SIZE;
 	priv->block_requested = -1;
 
-	priv->fifo = kfifo_alloc(TFTP_FIFO_SIZE);
-	if (!priv->fifo) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
 	parseopt_hu(fsdev->options, "port", &port);
 
 	priv->tftp_con = net_udp_new(tpriv->server, port, tftp_handler, priv);
 	if (IS_ERR(priv->tftp_con)) {
 		ret = PTR_ERR(priv->tftp_con);
-		goto out1;
+		goto out;
 	}
 
 	ret = tftp_send(priv);
 	if (ret)
-		goto out2;
+		goto out1;
 
 	tftp_timer_reset(priv);
-	while (priv->state != STATE_RDATA &&
-			priv->state != STATE_DONE &&
-			priv->state != STATE_WDATA) {
+	while (priv->state != STATE_DONE && priv->state != STATE_START) {
 		ret = tftp_poll(priv);
 		if (ret == TFTP_ERR_RESEND)
 			tftp_send(priv);
 		if (ret < 0)
-			goto out2;
+			goto out1;
 	}
 
-	if (priv->state == STATE_DONE && priv->err) {
+	if (priv->state == STATE_DONE) {
+		/* this should not happen; STATE_DONE without error happens
+		   after completing the transfer but this has not been started
+		   yet */
+		if (WARN_ON(priv->err == 0))
+			priv->err = -EIO;
+
 		ret = priv->err;
-		goto out2;
+		goto out1;
 	}
 
-	priv->buf = xmalloc(priv->blocksize);
+	ret = tftp_start_transfer(priv);
+	if (ret < 0)
+		goto out1;
 
 	return priv;
-out2:
-	net_unregister(priv->tftp_con);
 out1:
-	kfifo_free(priv->fifo);
+	net_unregister(priv->tftp_con);
 out:
 	free(priv);
 
@@ -561,7 +588,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 (kfifo_len(priv->fifo) == 0)
 			tftp_send(priv);
 
 		ret = tftp_poll(priv);
-- 
2.37.1

[PATCH v3 09/18] 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, 16 insertions(+), 3 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index 0b6d57c4603f..38a9e955ee92 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -218,7 +218,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;
 
@@ -235,7 +235,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"))
@@ -243,6 +243,13 @@ 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) {
+		pr_warn("tftp: invalid oack response\n");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static void tftp_timer_reset(struct file_priv *priv)
@@ -295,8 +302,14 @@ 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 (tftp_parse_oack(priv, pkt, len) < 0) {
+			priv->err = -EINVAL;
+			priv->state = STATE_DONE;
+			break;
+		}
+
 		priv->state = STATE_START;
 		break;
 
-- 
2.37.1

[PATCH v3 10/18] 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 38a9e955ee92..9f881249ce07 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -87,6 +87,7 @@ struct file_priv {
 	void *buf;
 	int blocksize;
 	int block_requested;
+	bool is_getattr;
 };
 
 struct tftp_priv {
@@ -415,7 +416,7 @@ static int tftp_start_transfer(struct file_priv *priv)
 }
 
 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;
@@ -444,6 +445,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;
 
 	parseopt_hu(fsdev->options, "port", &port);
 
@@ -494,7 +496,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);
 
@@ -710,7 +712,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.37.1

[PATCH v3 11/18] 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 9f881249ce07..2ed62b77901b 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -148,7 +148,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.37.1

[PATCH v3 12/18] 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 | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index 2ed62b77901b..2bb9361d8929 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -260,6 +260,26 @@ 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)
+{
+	if (len > priv->blocksize) {
+		pr_warn("tftp: oversized packet (%zu > %d) received\n",
+			len, priv->blocksize);
+		return;
+	}
+
+	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)
 {
@@ -339,23 +359,8 @@ static void tftp_recv(struct file_priv *priv,
 			/* Same block again; ignore it. */
 			break;
 
-		if (len > priv->blocksize) {
-			pr_warn("tftp: oversized packet (%u > %d) received\n",
-				len, priv->blocksize);
-			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.37.1

[PATCH v3 13/18] 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 2bb9361d8929..5c3e56a6cb22 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -263,6 +263,8 @@ 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;
+
 	if (len > priv->blocksize) {
 		pr_warn("tftp: oversized packet (%zu > %d) received\n",
 			len, priv->blocksize);
@@ -271,9 +273,14 @@ static void tftp_put_data(struct file_priv *priv, uint16_t block,
 
 	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.37.1

[PATCH v3 14/18] 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.

This commit increases barebox size by

| add/remove: 1/0 grow/shrink: 4/1 up/down: 144/-16 (128)
| Function                                     old     new   delta
| tftp_send                                    316     364     +48
| tftp_init                                     16      60     +44
| tftp_handler                                 824     868     +44
| tftp_do_open                                 532     536      +4
| g_tftp_window_size                             -       4      +4
| tftp_poll                                    180     164     -16
| Total: Before=626311, After=626439, chg +0.02%

Setting FS_TFTP_MAX_WINDOW_SIZE to zero reduces it to

| add/remove: 1/0 grow/shrink: 3/2 up/down: 92/-52 (40)
| Function                                     old     new   delta
| tftp_init                                     16      60     +44
| tftp_handler                                 824     864     +40
| tftp_do_open                                 532     536      +4
| g_tftp_window_size                             -       4      +4
| tftp_poll                                    180     164     -16
| tftp_send                                    316     280     -36
| Total: Before=626311, After=626351, chg +0.01%

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/Kconfig | 14 ++++++++++++++
 fs/tftp.c  | 54 +++++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 55 insertions(+), 13 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 5c3e56a6cb22..4c3980404dc1 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -26,9 +26,11 @@
 #include <libgen.h>
 #include <fcntl.h>
 #include <getopt.h>
+#include <globalvar.h>
 #include <init.h>
 #include <linux/stat.h>
 #include <linux/err.h>
+#include <linux/kernel.h>
 #include <kfifo.h>
 #include <parseopt.h>
 #include <linux/sizes.h>
@@ -69,14 +71,18 @@
 
 #define TFTP_BLOCK_SIZE		512	/* default TFTP block size */
 #define TFTP_MTU_SIZE		1432	/* MTU based block size */
+#define TFTP_MAX_WINDOW_SIZE	CONFIG_FS_TFTP_MAX_WINDOW_SIZE
 
 #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;
@@ -86,7 +92,7 @@ struct file_priv {
 	struct kfifo *fifo;
 	void *buf;
 	int blocksize;
-	int block_requested;
+	unsigned int windowsize;
 	bool is_getattr;
 };
 
@@ -118,6 +124,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]);
@@ -125,6 +132,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 supported 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)
@@ -152,18 +168,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;
@@ -206,7 +228,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;
 	}
 
@@ -243,11 +264,15 @@ static int 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;
 	}
 
-	if (priv->blocksize > TFTP_MTU_SIZE) {
+	if (priv->blocksize > TFTP_MTU_SIZE ||
+	    priv->windowsize > TFTP_MAX_WINDOW_SIZE ||
+	    priv->windowsize == 0) {
 		pr_warn("tftp: invalid oack response\n");
 		return -EINVAL;
 	}
@@ -362,8 +387,7 @@ 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);
@@ -402,7 +426,9 @@ static void tftp_handler(void *ctx, char *packet, unsigned len)
 
 static int tftp_start_transfer(struct file_priv *priv)
 {
-	priv->fifo = kfifo_alloc(priv->blocksize);
+	/* multiplication is safe; both operands where checked in tftp_parse_oack()
+	   and are small integers */
+	priv->fifo = kfifo_alloc(priv->blocksize * priv->windowsize);
 	if (!priv->fifo)
 		return -ENOMEM;
 
@@ -422,7 +448,7 @@ static int tftp_start_transfer(struct file_priv *priv)
 	} else {
 		/* send ACK */
 		priv->state = STATE_OACK;
-		priv->block = 0;
+		priv->last_block = 0;
 		tftp_send(priv);
 	}
 
@@ -458,7 +484,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;
 
 	parseopt_hu(fsdev->options, "port", &port);
@@ -617,7 +643,7 @@ static int tftp_read(struct device_d *dev, FILE *f, void *buf, size_t insize)
 		if (priv->state == STATE_DONE)
 			return outsize;
 
-		if (kfifo_len(priv->fifo) == 0)
+		if (priv->last_block == priv->ack_block)
 			tftp_send(priv);
 
 		ret = tftp_poll(priv);
@@ -809,6 +835,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

[PATCH v3 15/18] 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 | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index 4c3980404dc1..c96c8917633a 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -316,6 +316,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)
@@ -370,7 +371,7 @@ static void tftp_recv(struct file_priv *priv,
 
 	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 */
@@ -378,20 +379,20 @@ 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.37.1

[PATCH v3 16/18] tftp: add debug_assert() macro

[Enrico Scholz]

Is a noop in normal cases (when compiler sees that condition can be
evaluated without sideeffects) but allows optimizations based on the
condition.

E.g. in

| void foo(int a)
| {
|     debug_assert(a == 23);
|
|     if (a == 23)
|         return;
|
|     bar();
| }

the call to 'bar()' will be optimized away.

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

diff --git a/fs/tftp.c b/fs/tftp.c
index c96c8917633a..d02ca750cd6d 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -75,6 +75,15 @@
 
 #define TFTP_ERR_RESEND	1
 
+#ifdef DEBUG
+#  define debug_assert(_cond)	BUG_ON(!(_cond))
+#else
+#  define debug_assert(_cond) do {			\
+		if (!(_cond))				\
+			__builtin_unreachable();	\
+	} while (0)
+#endif
+
 static int g_tftp_window_size = DIV_ROUND_UP(TFTP_MAX_WINDOW_SIZE, 2);
 
 struct file_priv {
-- 
2.37.1

[PATCH v3 17/18] 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 and barebox binary size, it is an
Kconfig option.

bloat-o-meter reports with a non-zero FS_TFTP_REORDER_CACHE_SIZE

| add/remove: 3/0 grow/shrink: 4/0 up/down: 920/0 (920)
| Function                                     old     new   delta
| tftp_handler                                 860    1200    +340
| tftp_put_data                                  -     184    +184
| tftp_window_cache_remove                       -     124    +124
| tftp_window_cache_get_pos                      -     120    +120
| tftp_do_open                                 536     608     +72
| tftp_do_close                                260     312     +52
| tftp_send                                    364     392     +28
| Total: Before=626431, After=627351, chg +0.15%

After setting FS_TFTP_REORDER_CACHE_SIZE Kconfig option to 0, numbers
are going down to

| add/remove: 0/0 grow/shrink: 3/0 up/down: 188/0 (188)
| Function                                     old     new   delta
| tftp_handler                                 860     992    +132
| tftp_send                                    364     392     +28
| tftp_do_open                                 536     564     +28
| Total: Before=626431, After=626619, chg +0.03%

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

diff --git a/fs/Kconfig b/fs/Kconfig
index 0c4934285942..cf884e0646a1 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) and barebox binary size (around 700 bytes).
+
+	  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 d02ca750cd6d..8a2c2299a52f 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -28,6 +28,7 @@
 #include <getopt.h>
 #include <globalvar.h>
 #include <init.h>
+#include <linux/bitmap.h>
 #include <linux/stat.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
@@ -73,6 +74,12 @@
 #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
+
+/* marker for an emtpy 'tftp_cache' */
+#define TFTP_CACHE_NO_ID	(-1)
+
 #define TFTP_ERR_RESEND	1
 
 #ifdef DEBUG
@@ -86,6 +93,30 @@
 
 static int g_tftp_window_size = DIV_ROUND_UP(TFTP_MAX_WINDOW_SIZE, 2);
 
+struct tftp_block {
+	uint16_t id;
+	uint16_t len;
+
+	struct list_head head;
+	uint8_t data[];
+};
+
+struct tftp_cache {
+	/* The id located at @pos or TFTP_CACHE_NO_ID when cache is empty.  It
+	   is possible that the corresponding bit in @used is NOT set. */
+	unsigned int		id;
+	unsigned int		pos;
+
+	/* bitmask */
+	unsigned long		used[BITS_TO_LONGS(TFTP_WINDOW_CACHE_NUM)];
+
+	/* size of the cache; is limited by TFTP_WINDOW_CACHE_NUM and the
+	   actual window size */
+	unsigned int		size;
+	unsigned int		block_len;
+	struct tftp_block	*blocks[TFTP_WINDOW_CACHE_NUM];
+};
+
 struct file_priv {
 	struct net_connection *tftp_con;
 	int push;
@@ -103,12 +134,222 @@ struct file_priv {
 	int blocksize;
 	unsigned int windowsize;
 	bool is_getattr;
+	struct tftp_cache cache;
 };
 
 struct tftp_priv {
 	IPaddr_t server;
 };
 
+static inline bool is_block_before(uint16_t a, uint16_t b)
+{
+	return (int16_t)(b - a) > 0;
+}
+
+static inline uint16_t get_block_delta(uint16_t a, uint16_t b)
+{
+	debug_assert(!is_block_before(b, a));
+
+	return b - a;
+}
+
+static bool in_window(uint16_t block, uint16_t start, uint16_t end)
+{
+	/* handle the three cases:
+	   - [ ......... | start | .. | BLOCK | .. | end | ......... ]
+	   - [ ..| BLOCK | .. | end | ................. | start | .. ]
+	   - [ ..| end | ................. | start | .. | BLOCK | .. ]
+	*/
+	return ((start <= block && block <= end) ||
+		(block <= end   && end   <= start) ||
+		(end   <= start && start <= block));
+}
+
+static inline size_t tftp_window_cache_size(struct tftp_cache const *cache)
+{
+	/* allows to optimize away the cache code when TFTP_WINDOW_CACHE_SIZE
+	   is 0 */
+	return TFTP_WINDOW_CACHE_NUM == 0 ? 0 : cache->size;
+}
+
+static void tftp_window_cache_init(struct tftp_cache *cache,
+				   uint16_t block_len, uint16_t window_size)
+{
+	debug_assert(window_size > 0);
+
+	*cache = (struct tftp_cache) {
+		.id		= TFTP_CACHE_NO_ID,
+		.block_len	= block_len,
+		.size		= min_t(uint16_t, window_size - 1,
+					ARRAY_SIZE(cache->blocks)),
+	};
+}
+
+static void tftp_window_cache_free(struct tftp_cache *cache)
+{
+	size_t	cache_size = tftp_window_cache_size(cache);
+	size_t	i;
+
+	for (i = 0; i < cache_size; ++i) {
+		free(cache->blocks[i]);
+		cache->blocks[i] = NULL;
+	}
+}
+
+static void tftp_window_cache_reset(struct tftp_cache *cache)
+{
+	cache->id = TFTP_CACHE_NO_ID;
+	memset(cache->used, 0, sizeof cache->used);
+}
+
+static int tftp_window_cache_get_pos(struct tftp_cache const *cache, uint16_t id)
+{
+	size_t		cache_size = tftp_window_cache_size(cache);
+	unsigned int	pos;
+
+	if (cache_size == 0)
+		return -1;
+
+	if (cache->id == TFTP_CACHE_NO_ID)
+		return -1;
+
+	if (!in_window(id, cache->id, cache->id + cache_size - 1))
+		return -1;
+
+	pos  = cache->pos + get_block_delta(cache->id, id);
+	pos %= cache_size;
+
+	return pos;
+}
+
+/* returns whether the first cached element has the given @id */
+static bool tftp_window_cache_starts_with(struct tftp_cache const *cache,
+					  uint16_t id)
+{
+	return (TFTP_WINDOW_CACHE_NUM > 0 &&
+		cache->id != TFTP_CACHE_NO_ID &&
+		cache->id == id &&
+		test_bit(cache->pos, cache->used));
+}
+
+static bool tftp_window_cache_is_empty(struct tftp_cache const *cache)
+{
+	/* use this indirection to avoid warnings about a '0 < 0' comparison
+	   in the loop condition when TFTP_WINDOW_CACHE_NUM is zero */
+	size_t	cache_size = ARRAY_SIZE(cache->used);
+	size_t	i;
+
+	for (i = 0; i < cache_size; ++i) {
+		if (cache->used[i] != 0)
+			return false;
+	}
+
+	return true;
+}
+
+static int tftp_window_cache_insert(struct tftp_cache *cache, uint16_t id,
+				    void const *data, size_t len)
+{
+	size_t const		cache_size = tftp_window_cache_size(cache);
+	int			pos;
+	struct tftp_block	*block;
+
+	if (cache_size == 0)
+		return -ENOSPC;
+
+	if (cache->id == TFTP_CACHE_NO_ID) {
+		/* initialize cache when it does not contain elements yet */
+		cache->id = id;
+		cache->pos = 0;
+
+		/* sanity check; cache is expected to be empty */
+		debug_assert(tftp_window_cache_is_empty(cache));
+	}
+
+	pos = tftp_window_cache_get_pos(cache, id);
+	if (pos < 0)
+		return -ENOSPC;
+
+	debug_assert(pos < cache_size);
+
+	if (test_bit(pos, cache->used))
+		/* block already cached */
+		return 0;
+
+	block = cache->blocks[pos];
+	if (!block) {
+		/* allocate space for the block; after being released, this
+		   memory can be reused for other blocks during the same tftp
+		   transfer */
+		block = malloc(sizeof *block + cache->block_len);
+		if (!block)
+			return -ENOMEM;
+
+		cache->blocks[pos] = block;
+	}
+
+	__set_bit(pos, cache->used);
+	memcpy(block->data, data, len);
+	block->id = id;
+	block->len = len;
+
+	return 0;
+}
+
+/* Marks the element at 'pos' as unused and updates internal cache information.
+   Returns true iff element at pos was valid. */
+static bool tftp_window_cache_remove(struct tftp_cache *cache, unsigned int pos)
+{
+	size_t const	cache_size = tftp_window_cache_size(cache);
+	bool		res;
+
+	if (cache_size == 0)
+		return 0;
+
+	res = __test_and_clear_bit(pos, cache->used);
+
+	if (tftp_window_cache_is_empty(cache)) {
+		/* cache has been cleared; reset pointers */
+		cache->id = TFTP_CACHE_NO_ID;
+	} else if (pos != cache->pos) {
+		/* noop; elements has been removed from the middle of cache */
+	} else {
+		/* first element of cache has been removed; proceed to the
+		   next one */
+		cache->id  += 1;
+		cache->pos += 1;
+		cache->pos %= cache_size;
+	}
+
+	return res;
+}
+
+/* Releases the first element from the cache and returns its content.
+ *
+ * Function can return NULL when the element is not cached
+ */
+static struct tftp_block *tftp_window_cache_pop(struct tftp_cache *cache)
+{
+	unsigned int	pos = cache->pos;
+
+	debug_assert(cache->id != TFTP_CACHE_NO_ID);
+
+	if (!tftp_window_cache_remove(cache, pos))
+		return NULL;
+
+	return cache->blocks[pos];
+}
+
+static bool tftp_window_cache_remove_id(struct tftp_cache *cache, uint16_t id)
+{
+	int		pos = tftp_window_cache_get_pos(cache, id);
+
+	if (pos < 0)
+		return false;
+
+	return tftp_window_cache_remove(cache, pos);
+}
+
 static int tftp_truncate(struct device_d *dev, FILE *f, loff_t size)
 {
 	return 0;
@@ -197,6 +438,7 @@ static int tftp_send(struct file_priv *priv)
 		priv->ack_block += priv->windowsize;
 		pkt = (unsigned char *)s;
 		len = pkt - xp;
+		tftp_window_cache_reset(&priv->cache);
 		break;
 	}
 
@@ -321,6 +563,60 @@ static void tftp_put_data(struct file_priv *priv, uint16_t block,
 	}
 }
 
+static void tftp_apply_window_cache(struct file_priv *priv)
+{
+	struct tftp_cache *cache = &priv->cache;
+
+	while (tftp_window_cache_starts_with(cache, priv->last_block + 1)) {
+		struct tftp_block *block;
+
+		/* can be changed by tftp_put_data() below and must be
+		   checked in each loop */
+		if (priv->state != STATE_RDATA)
+			return;
+
+		block = tftp_window_cache_pop(cache);
+
+		debug_assert(block);
+		debug_assert(block->id == (uint16_t)(priv->last_block + 1));
+
+		tftp_put_data(priv, block->id, block->data, block->len);
+	}
+}
+
+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);
+		tftp_window_cache_remove_id(&priv->cache, block);
+		tftp_apply_window_cache(priv);
+	} 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->cache, 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)
 {
@@ -387,6 +683,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_reset(&priv->cache);
 
 			if (block != 1) {	/* Assertion */
 				pr_err("error: First block is not block 1 (%d)\n",
@@ -397,11 +694,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;
 
@@ -449,6 +742,9 @@ static int tftp_start_transfer(struct file_priv *priv)
 			priv->fifo = NULL;
 			return -ENOMEM;
 		}
+	} else {
+		tftp_window_cache_init(&priv->cache,
+				       priv->blocksize, priv->windowsize);
 	}
 
 	if (priv->push) {
@@ -586,6 +882,7 @@ static int tftp_do_close(struct file_priv *priv)
 	}
 
 	net_unregister(priv->tftp_con);
+	tftp_window_cache_free(&priv->cache);
 	kfifo_free(priv->fifo);
 	free(priv->filename);
 	free(priv->buf);
-- 
2.37.1

[PATCH v3 18/18] tftp: add selftest

[Enrico Scholz]

Unittest for window cache functions.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 fs/tftp-selftest.h |  56 ++++++++++++++++++++++++
 fs/tftp.c          | 104 +++++++++++++++++++++++++++++++++++++++++++++
 test/self/Kconfig  |   7 +++
 3 files changed, 167 insertions(+)
 create mode 100644 fs/tftp-selftest.h

diff --git a/fs/tftp-selftest.h b/fs/tftp-selftest.h
new file mode 100644
index 000000000000..2406ed329ecc
--- /dev/null
+++ b/fs/tftp-selftest.h
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0+
+// SPDX-FileCopyrightText: 2022 Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
+
+#ifndef H_BAREBOX_FS_TFTP_SELFTEST_H
+#define H_BAREBOX_FS_TFTP_SELFTEST_H
+
+#include <bselftest.h>
+
+#define _expect_fmt(_v) _Generic(_v,				\
+				 void const *: "%p",		\
+				 void *: "%p",			\
+				 bool: "%d",			\
+				 long int: "%lld",		\
+				 long unsigned int: "%llu",	\
+				 unsigned short: "%h",		\
+				 signed int: "%d",		\
+				 unsigned int: "%u")
+
+#define _expect_op(_a, _b, _op)						\
+	({								\
+		__typeof__(_a)	__a = (_a);				\
+		__typeof__(_b)	__b = (_b);				\
+									\
+		total_tests++;						\
+									\
+		if (!(__a _op __b)) {					\
+			char fmt[sizeof "%s:%d: failed: %XXX  %XXX\n" # _op]; \
+			strcpy(fmt, "%s:%d: failed: ");			\
+			strcat(fmt, _expect_fmt(__a));			\
+			strcat(fmt, " " # _op " ");			\
+			strcat(fmt, _expect_fmt(__b));			\
+			strcat(fmt, "\n");				\
+									\
+			failed_tests++;					\
+			printf(fmt, __func__, __LINE__, __a, __b);	\
+		}							\
+	})
+
+#define _as_void(_p) ({					\
+			void const *__res = (_p);	\
+			__res;				\
+		})
+
+#define expect_eq(_a, _b)	_expect_op(_a, _b, ==);
+#define expect_ne(_a, _b)	_expect_op(_a, _b, !=);
+#define expect_it(_a)		_expect_op(_a, true, ==);
+
+#define expect_err(_a)		_expect_op(_a, 0, <);
+#define expect_ok(_a)		_expect_op(_a, 0, ==);
+
+/* _Generic() does not match 'void *' for typed pointers; cast them to raw
+   'void *' first */
+#define expect_NULL(_a)		_expect_op(_as_void(_a), NULL, ==);
+#define expect_PTR(_a)		_expect_op(_as_void(_a), NULL, !=);
+
+#endif	/* H_BAREBOX_FS_TFTP_SELFTEST_H */
diff --git a/fs/tftp.c b/fs/tftp.c
index 8a2c2299a52f..e01aafce47b5 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -36,6 +36,8 @@
 #include <parseopt.h>
 #include <linux/sizes.h>
 
+#include "tftp-selftest.h"
+
 #define TFTP_PORT	69	/* Well known TFTP port number */
 
 /* Seconds to wait before remote server is allowed to resend a lost packet */
@@ -1147,3 +1149,105 @@ static int tftp_init(void)
 	return register_fs_driver(&tftp_driver);
 }
 coredevice_initcall(tftp_init);
+
+
+BSELFTEST_GLOBALS();
+
+static int __maybe_unused tftp_window_cache_selftest(void)
+{
+	struct tftp_cache	*cache = malloc(sizeof *cache);
+
+	if (!cache)
+		return -ENOMEM;
+
+	(void)skipped_tests;
+
+	expect_it( is_block_before(0, 1));
+	expect_it(!is_block_before(1, 0));
+	expect_it( is_block_before(65535, 0));
+	expect_it(!is_block_before(0, 65535));
+
+	expect_eq(get_block_delta(0, 1),     1);
+	expect_eq(get_block_delta(65535, 0), 1);
+	expect_eq(get_block_delta(65535, 1), 2);
+
+	expect_it(!in_window(0, 1, 3));
+	expect_it( in_window(1, 1, 3));
+	expect_it( in_window(2, 1, 3));
+	expect_it( in_window(3, 1, 3));
+	expect_it(!in_window(4, 1, 3));
+
+	expect_it(!in_window(65534, 65535, 1));
+	expect_it( in_window(65535, 65535, 1));
+	expect_it( in_window(    0, 65535, 1));
+	expect_it( in_window(    1, 65535, 1));
+	expect_it(!in_window(    2, 65535, 1));
+
+
+	tftp_window_cache_init(cache, 512, 5);
+
+	if (tftp_window_cache_size(cache) < 4)
+		goto out;
+
+	expect_eq(tftp_window_cache_size(cache), 4);
+
+	/* sequence 1 */
+	expect_ok (tftp_window_cache_insert(cache, 20, "20", 2));
+	expect_ok (tftp_window_cache_insert(cache, 22, "22", 2));
+	expect_ok (tftp_window_cache_insert(cache, 21, "21", 2));
+	expect_ok (tftp_window_cache_insert(cache, 23, "23", 2));
+	expect_err(tftp_window_cache_insert(cache, 24, "24", 2));
+	expect_err(tftp_window_cache_insert(cache, 19, "19", 2));
+	expect_ok (tftp_window_cache_insert(cache, 22, "22", 2));
+	expect_ok (tftp_window_cache_insert(cache, 20, "20", 2));
+
+	expect_eq(tftp_window_cache_pop(cache)->id, 20);
+	expect_eq(tftp_window_cache_pop(cache)->id, 21);
+	expect_eq(tftp_window_cache_pop(cache)->id, 22);
+	expect_eq(tftp_window_cache_pop(cache)->id, 23);
+	expect_eq(cache->id, TFTP_CACHE_NO_ID);
+
+	/* sequence 2 */
+	expect_ok (tftp_window_cache_insert(cache, 30, "30", 2));
+	expect_ok (tftp_window_cache_insert(cache, 32, "32", 2));
+	expect_err(tftp_window_cache_insert(cache, 34, "34", 2));
+
+	expect_it(tftp_window_cache_starts_with(cache, 30));
+	expect_eq(tftp_window_cache_pop(cache)->id, 30);
+
+	expect_ok (tftp_window_cache_insert(cache, 34, "34", 2));
+	expect_err(tftp_window_cache_insert(cache, 35, "35", 2));
+
+	expect_it(!tftp_window_cache_starts_with(cache, 30));
+	expect_it(!tftp_window_cache_starts_with(cache, 31));
+	expect_it(!tftp_window_cache_starts_with(cache, 32));
+	expect_NULL(tftp_window_cache_pop(cache));
+
+	expect_it(tftp_window_cache_starts_with(cache, 32));
+	expect_eq(tftp_window_cache_pop(cache)->id, 32);
+
+	expect_NULL(tftp_window_cache_pop(cache));
+	expect_eq(tftp_window_cache_pop(cache)->id, 34);
+
+	expect_eq(cache->id, TFTP_CACHE_NO_ID);
+
+	/* sequence 3 */
+	expect_ok(tftp_window_cache_insert(cache, 40, "40", 2));
+	expect_ok(tftp_window_cache_insert(cache, 42, "42", 2));
+	expect_ok(tftp_window_cache_insert(cache, 43, "43", 2));
+
+	expect_it(!tftp_window_cache_remove_id(cache, 30));
+	expect_it(!tftp_window_cache_remove_id(cache, 41));
+	expect_it(!tftp_window_cache_remove_id(cache, 44));
+
+	expect_it( tftp_window_cache_remove_id(cache, 42));
+	expect_it(!tftp_window_cache_remove_id(cache, 42));
+
+out:
+	tftp_window_cache_free(cache);
+
+	return 0;
+}
+#ifdef CONFIG_SELFTEST_TFTP
+bselftest(core, tftp_window_cache_selftest);
+#endif
diff --git a/test/self/Kconfig b/test/self/Kconfig
index 680196a4fe29..03cfa89987d2 100644
--- a/test/self/Kconfig
+++ b/test/self/Kconfig
@@ -32,6 +32,7 @@ config SELFTEST_ENABLE_ALL
 	select SELFTEST_PROGRESS_NOTIFIER
 	select SELFTEST_OF_MANIPULATION
 	select SELFTEST_ENVIRONMENT_VARIABLES if ENVIRONMENT_VARIABLES
+	imply SELFTEST_TFTP
 	help
 	  Selects all self-tests compatible with current configuration
 
@@ -57,4 +58,10 @@ config SELFTEST_PROGRESS_NOTIFIER
 config SELFTEST_ENVIRONMENT_VARIABLES
 	bool "environment variable selftest"
 
+config SELFTEST_TFTP
+	bool "tftp selftest"
+	depends on FS_TFTP
+	help
+	  Tests tftp functionality
+
 endif
-- 
2.37.1

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

[Sascha Hauer]

Hi Enrico,

On Mon, Aug 15, 2022 at 10:42:04AM +0200, Enrico Scholz wrote:
> The tftp "windowsize" greatly improves the performance of tftp
> transfers.  This patchset adds support for it.

There's a variant of the tftp-hpa package with RFC7440 support here:
https://github.com/ClausKlein/tftp-hpa.git

I tried to test your series with that one. So far I wasn't successful
with running that tftpd, mostly because I am on a machine that already
has a different tftpd running at port 69 and I don't have root access
(or rather I didn't want to use it) on that machine. I'll have a second
look at it later. I just wanted to notice that at least theoretically
you could test your series against another server then your own one.

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 v3 00/18] 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.
>
> There's a variant of the tftp-hpa package with RFC7440 support here:
> https://github.com/ClausKlein/tftp-hpa.git

thx; seems to work with it.  I had to comment out the setuid() stuff to
run it as an ordinary user but can start it then as

| ./tftpd/tftpd --address [::]:1230 -s /var/lib/tftpboot/ -vvv -l -L  -p


In barebox then

:/ tftp -P 1230 data-100MiB /tmp/a && sha1sum /tmp/a
        [################################################################] 104857600 bytes, 100631094 bytes/s


Enrico

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

[Sascha Hauer]

On Tue, Aug 16, 2022 at 11:40:49AM +0200, Enrico Scholz wrote:
> Sascha Hauer <sha@pengutronix.de> writes:
> 
> >> The tftp "windowsize" greatly improves the performance of tftp
> >> transfers.  This patchset adds support for it.
> >
> > There's a variant of the tftp-hpa package with RFC7440 support here:
> > https://github.com/ClausKlein/tftp-hpa.git
> 
> thx; seems to work with it.  I had to comment out the setuid() stuff to
> run it as an ordinary user but can start it then as
> 
> | ./tftpd/tftpd --address [::]:1230 -s /var/lib/tftpboot/ -vvv -l -L  -p

I didn't manage to run it as non-root. As root it works as expected
though. The speed gain is really impressive.

I saw that atftp already contains a patch for RFC7440 support, so sooner
or later RFC7440 will find its way to the distributions.

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 v3 00/18] add "windowsize" (RFC 7440) support for tftp

[Sascha Hauer]

On Mon, Aug 15, 2022 at 10:42:04AM +0200, Enrico Scholz wrote:
> 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 speeds, smaller files (1MiB, 4 MiB + 20 MiB) were
> used.

Applied this series beginning with patch 3 for now.

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 |