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

[Enrico Scholz <enrico.scholz@sigma-chemnitz.de>]

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