#10 windowsize option

[Andi]

Hi,
the following patch 'windowsize_option.patch ' implements the windowsize option. It seems to work fine (tested with the atftp client and ipxe which uses windowsize=4) and it would be great if it could be reviewed and further tested. It also prevents the 'sorcerers apprentice syndrom' which in my opinion should be prevented in any case (c.f. RFC1123).

The patch applies cleanly after the 'typos'-patch which just fixes some typos.

Thanks and best regards, Andi

[Andi]

Hi,
I added a few minor fixes and improvements. The patch is also available at Debian's Salsa [1].
A Debian package to ease testing has been uploaded to sid just a few minutes ago.

Best regards, Andi

[1] https://salsa.debian.org/debian/atftp/-/blob/master/debian/patches/windowsize_option.patch

[Andi]

The latest changes modify the windowsize lock-step algorithm to check for
ACKs within a window.

Before, ACKs were only expected after a full window. They reported the
last block successfully transferred in the sequence. However, iPXE
sends an ACK for the last correct block immediately, when a wrong block
is received. This commit implements the same and checks for ACKs
between sending the blocks of a window. For the time being, the time
waiting for unsolicited ACKs is set to 50us.

I am not sure if there is a better way to check in between blocks if an unexpected ACK is received. I did not test multicast so far.

[Andi]

Hi,
here is an updated/refurbished windowsize patch that applies and integrates into the existing code. I tried to drop white space changes and some cleanup into an extra patch to simplify comparing the modifications. I'll do some more testing in the next days/weeks, also on real hardware. Testers and reviewers are welcome, especially for multicast setups!

2021-10-23: I added the congestion patch here; the post below is waiting for moderation…

[Andi]

Note: An additional congestion control patch is available, see above.

[Martin Dummer]

Hello Andi,
I'm very pleased you fixed the windowsize patch. I will start integrating and testing.
I assume you did not discover problems with your windowsize addition in production until now?
Bye
Martin

[Andi]

Hi Martin,
so far it seems to work fine. It would be interesting to test in a network with congestion as it looks like we have too much bandwidth and we have no package loss ;-). I tried to simulate that locally with examples from https://wiki.linuxfoundation.org/networking/netem , but it's hard to choose a realistic scenario. I implemented some kind of simple congestion control on top of the other patches: When windows are not completely received because frames got lost, the delay between the datagrams sent within a window is increased by 10us. If a window is transferred fine, a non-zero delay is decreased by 1us See the attached patch.

Best regards, Andi

[Martin Dummer]

One Question:
Here:

--- a/atftp.1
+++ b/atftp.1
@@ -69,12 +69,14 @@
 arguments as the interactive one. For example, use: --option "blksize 1428"
 to configure block size.
 .br
-Possible Values are:
+Possible settings are:
 .br
-  --option "tsize enable"
-  --option "tsize disable"
+  --option "disable blksize"
   --option "blksize 8"
   --option "blksize 65464"
+  --option "timeout 1"
+  --option "disable windowsize"
+  --option "windowsize 4"

you remove the mention of the tsize option from the man page - is there a reason why?

And - I propose "windowsize disable" before "disable windowsize" - that's more straight forward.

[Andi]

Hi Martin,
I just checked, probably I got confused with the interactive mode:

$ atftp --option "windowsize hallo"
Option windowsize = hallo
tftp> option
Usage: option <option name> [option value]
       option disable <option name>
  tsize:      disabled
  blksize:    disabled
  windowsize:   hallo
  timeout:    disabled
  multicast:  disabled
  password:   disabled
tftp> option disable windowsize
Option windowsize disabled
tftp> option
Usage: option <option name> [option value]
       option disable <option name>
  tsize:      disabled
  blksize:    disabled
  windowsize: disabled
  timeout:    disabled
  multicast:  disabled
  password:   disabled

But you are right for the tsize:

  $ atftp --option "tsize hallo"
Option tsize = hallo
tftp> option
Usage: option <option name> [option value]
       option disable <option name>
  tsize:      enabled
  blksize:    disabled
  windowsize: disabled
  timeout:    disabled
  multicast:  disabled
  password:   disabled
tftp>

So this is rather confusing. The problem is probably, tsize has never a value here, it only enables asking for the transmission size later, whereas blksize needs a value. And it looks like all options are disable by default anyway, so --option "disable windowsize" seems not to be needed at all.
In the interactive mode it's 'option disable OPTION'. It would be nice to keep that consistent with the switch '--option "disable OPTION"'. And values should be checked … :-/

[Andi]

The attached patch improves the robustness of the atftp-client in case of package loss or duplication.

Updates:

• patch simplified.
• add Server part (upload)
• another patch fix-last-window.patch is needed to handle losses in the last window (apply that after handle-loss-duplication-better.patch)
• multicast did not work anymore, but is fixed now: fix-multicast.patch . Tested thoroughly with the new test script and wireshark. ToDo: multicast with windowsize != 1

[Andi]

I guess this bug can now be closed.