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