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

[Sascha Hauer <sha@pengutronix.de>]

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 |