Network defrag

PATCH New Alessandro Rubini 4 participants 25 messages Aug 7, 2009 – Aug 17, 2009 Networking

lore

Share & tools

lore

Alessandro Rubini

author

2009-08-07 11:58 UTC #0 lore raw

cover Network defrag

I finally fixed the defrag code, testing with NFS as well. Didn't take performance figures, tough, for lack of time.

I wanted to do "config + environment" for the NFS case, like tftp, but didnt' do the second step out of laziness (also, the source file has long lines while I have 80 columns).

For the record, I added the check on ip_src and ip_dst, but everything stopped working. So I reverted that instead of entering a long debugging session.

The CONFIG_NET_MAXDEFRAG argument is the actual payload, so I add NFS overhead to that figure, which is expected to a beautiful 4096 or 8192. I feel this is better than other options, as the person writing the config is not expected to know how much protocol overhead is there.

Alessandro Rubini (4):
net: defragment IP packets
tftp: get the tftp block size from config file and from the
environment
nfs: accept CONFIG_NFS_READ_SIZE from config file arm nomadik: activate defrag choose 4k transfer block size

 include/configs/nhk8815.h |    4 +
 net/net.c                 |  188 +++++++++++++++++++++++++++++++++++++++++++-
 net/nfs.h                 |   10 ++-
 net/tftp.c                |   13 +++-
 4 files changed, 206 insertions(+), 9 deletions(-)

Alessandro Rubini

author

2009-08-07 11:58 UTC #1 lore raw

PATCH 1/4 net: defragment IP packets

The defragmenting code is enabled by CONFIG_IP_DEFRAG; the code is useful for TFTP and NFS transfers. The user can specify the maximum defragmented payload as CONFIG_NET_MAXDEFRAG (default 16k). Since NFS has a bigger per-packet overhead than TFTP, the static reassembly buffer can hold CONFIG_NET_MAXDEFRAG + the NFS overhead.

The packet buffer is used as an array of "hole" structures, acting as a double-linked list. Each new fragment can split a hole in two, reduce a hole or fill a hole. No support is there for a fragment overlapping two diffrent holes (i.e., thre new fragment is across an already-received fragment).

Signed-off-by Alessandro Rubini <[email protected]>

---
 net/net.c |  188 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 183 insertions(+), 5 deletions(-)

net/net.c +183 -5

index 641c37c..986d614 100644
@@ -1117,6 +1117,176 @@ static void CDPStart(void)
 }
 #endif
 
+#ifdef CONFIG_IP_DEFRAG
+/*
+ * This function collects fragments in a single packet, according
+ * to the algorithm in RFC815. It returns NULL or the pointer to
+ * a complete packet, in static storage
+ */
+#ifndef CONFIG_NET_MAXDEFRAG
+#define CONFIG_NET_MAXDEFRAG 16384
+#endif
+/*
+ * MAXDEFRAG, above, is chosen in the config file and  is real data
+ * so we need to add the NFS overhead, which is more than TFTP.
+ * To use sizeof in the internal unnamed structures, we need a real
+ * instance (can't do "sizeof(struct rpc_t.u.reply))", unfortunately).
+ * The compiler doesn't complain nor allocates the actual structure
+ */
+static struct rpc_t rpc_specimen;
+#define IP_PKTSIZE (CONFIG_NET_MAXDEFRAG + sizeof(rpc_specimen.u.reply))
+
+#define IP_MAXUDP (IP_PKTSIZE - IP_HDR_SIZE_NO_UDP)
+
+/*
+ * this is the packet being assembled, either data or frag control.
+ * Fragments go by 8 bytes, so this union must be 8 bytes long
+ */
+struct hole {
+	/* first_byte is address of this structure */
+	u16 last_byte;	/* last byte in this hole + 1 (begin of next hole) */
+	u16 next_hole;	/* index of next (in 8-b blocks), 0 == none */
+	u16 prev_hole;	/* index of prev, 0 == none */
+	u16 unused;
+};
+
+static IP_t *__NetDefragment(IP_t *ip, int *lenp)
+{
+	static uchar pkt_buff[IP_PKTSIZE] __attribute__((aligned(PKTALIGN)));
+	static u16 first_hole, total_len;
+	struct hole *payload, *thisfrag, *h, *newh;
+	IP_t *localip = (IP_t *)pkt_buff;
+	uchar *indata = (uchar *)ip;
+	int offset8, start, len, done = 0;
+	u16 ip_off = ntohs(ip->ip_off);
+
+	/* payload starts after IP header, this fragment is in there */
+	payload = (struct hole *)(pkt_buff + IP_HDR_SIZE_NO_UDP);
+	offset8 =  (ip_off & IP_OFFS);
+	thisfrag = payload + offset8;
+	start = offset8 * 8;
+	len = ntohs(ip->ip_len) - IP_HDR_SIZE_NO_UDP;
+
+	if (start + len > IP_MAXUDP) /* fragment extends too far */
+		return NULL;
+
+	if (!total_len || localip->ip_id != ip->ip_id) {
+		/* new (or different) packet, reset structs */
+		total_len = 0xffff;
+		payload[0].last_byte = ~0;
+		payload[0].next_hole = 0;
+		payload[0].prev_hole = 0;
+		first_hole = 0;
+		/* any IP header will work, copy the first we received */
+		memcpy(localip, ip, IP_HDR_SIZE_NO_UDP);
+	}
+
+	/*
+	 * What follows is the reassembly algorithm. We use the payload
+	 * array as a linked list of hole descriptors, as each hole starts
+	 * at a multiple of 8 bytes. However, last byte can be whatever value,
+	 * so it is represented as byte count, not as 8-byte blocks.
+	 */
+
+	h = payload + first_hole;
+	while (h->last_byte < start) {
+		if (!h->next_hole) {
+			/* no hole that far away */
+			return NULL;
+		}
+		h = payload + h->next_hole;
+	}
+
+	if (offset8 + (len / 8) <= h - payload) {
+		/* no overlap with holes (dup fragment?) */
+		return NULL;
+	}
+
+	if (!(ip_off & IP_FLAGS_MFRAG)) {
+		/* no more fragmentss: truncate this (last) hole */
+		total_len = start + len;
+		h->last_byte = start + len;
+	}
+
+	/*
+	 * There is some overlap: fix the hole list. This code doesn't
+	 * deal with a fragment that overlaps with two different holes
+	 * (thus being a superset of a previously-received fragment).
+	 */
+
+	if ( (h >= thisfrag) && (h->last_byte <= start + len) ) {
+		/* complete overlap with hole: remove hole */
+		if (!h->prev_hole && !h->next_hole) {
+			/* last remaining hole */
+			done = 1;
+		} else if (!h->prev_hole) {
+			/* first hole */
+			first_hole = h->next_hole;
+			payload[h->next_hole].prev_hole = 0;
+		} else if (!h->next_hole) {
+			/* last hole */
+			payload[h->prev_hole].next_hole = 0;
+		} else {
+			/* in the middle of the list */
+			payload[h->next_hole].prev_hole = h->prev_hole;
+			payload[h->prev_hole].next_hole = h->next_hole;
+		}
+
+	} else if (h->last_byte <= start + len) {
+		/* overlaps with final part of the hole: shorten this hole */
+		h->last_byte = start;
+
+	} else if (h >= thisfrag) {
+		/* overlaps with initial part of the hole: move this hole */
+		newh = thisfrag + (len / 8);
+		*newh = *h;
+		h = newh;
+		if (h->next_hole)
+			payload[h->next_hole].prev_hole = (h - payload);
+		if (h->prev_hole)
+			payload[h->prev_hole].next_hole = (h - payload);
+		else
+			first_hole = (h - payload);
+
+	} else {
+		/* fragment sits in the middle: split the hole */
+		newh = thisfrag + (len / 8);
+		*newh = *h;
+		h->last_byte = start;
+		h->next_hole = (newh - payload);
+		newh->prev_hole = (h - payload);
+		if (newh->next_hole)
+			payload[newh->next_hole].prev_hole = (newh - payload);
+	}
+
+	/* finally copy this fragment and possibly return whole packet */
+	memcpy((uchar *)thisfrag, indata + IP_HDR_SIZE_NO_UDP, len);
+	if (!done)
+		return NULL;
+
+	localip->ip_len = htons(total_len);
+	*lenp = total_len + IP_HDR_SIZE_NO_UDP;
+	return localip;
+}
+
+static inline IP_t *NetDefragment(IP_t *ip, int *lenp)
+{
+	u16 ip_off = ntohs(ip->ip_off);
+	if (!(ip_off & (IP_OFFS | IP_FLAGS_MFRAG)))
+		return ip; /* not a fragment */
+	return __NetDefragment(ip, lenp);
+}
+
+#else /* !CONFIG_IP_DEFRAG */
+
+static inline IP_t *NetDefragment(IP_t *ip, int *lenp)
+{
+	u16 ip_off = ntohs(ip->ip_off);
+	if (!(ip_off & (IP_OFFS | IP_FLAGS_MFRAG)))
+		return ip; /* not a fragment */
+	return NULL;
+}
+#endif
 
 void
 NetReceive(volatile uchar * inpkt, int len)

@@ -1363,10 +1533,12 @@ NetReceive(volatile uchar * inpkt, int len)
 #ifdef ET_DEBUG
 		puts ("Got IP\n");
 #endif
+		/* Before we start poking the header, make sure it is there */
 		if (len < IP_HDR_SIZE) {
 			debug ("len bad %d < %lu\n", len, (ulong)IP_HDR_SIZE);
 			return;
 		}
+		/* Check the packet length */
 		if (len < ntohs(ip->ip_len)) {
 			printf("len bad %d < %d\n", len, ntohs(ip->ip_len));
 			return;

@@ -1375,21 +1547,20 @@ NetReceive(volatile uchar * inpkt, int len)
 #ifdef ET_DEBUG
 		printf("len=%d, v=%02x\n", len, ip->ip_hl_v & 0xff);
 #endif
+		/* Can't deal with anything except IPv4 */
 		if ((ip->ip_hl_v & 0xf0) != 0x40) {
 			return;
 		}
-		/* Can't deal with fragments */
-		if (ip->ip_off & htons(IP_OFFS | IP_FLAGS_MFRAG)) {
-			return;
-		}
-		/* can't deal with headers > 20 bytes */
+		/* Can't deal with IP options (headers != 20 bytes) */
 		if ((ip->ip_hl_v & 0x0f) > 0x05) {
 			return;
 		}
+		/* Check the Checksum of the header */
 		if (!NetCksumOk((uchar *)ip, IP_HDR_SIZE_NO_UDP / 2)) {
 			puts ("checksum bad\n");
 			return;
 		}
+		/* If it is not for us, ignore it */
 		tmp = NetReadIP(&ip->ip_dst);
 		if (NetOurIP && tmp != NetOurIP && tmp != 0xFFFFFFFF) {
 #ifdef CONFIG_MCAST_TFTP

@@ -1398,6 +1569,13 @@ NetReceive(volatile uchar * inpkt, int len)
 			return;
 		}
 		/*
+		 * The function returns the unchanged packet if it's not
+		 * a fragment, and either the complete packet or NULL if
+		 * it is a fragment (if !CONFIG_IP_DEFRAG, it returns NULL)
+		 */
+		if (!(ip = NetDefragment(ip, &len)))
+			return;
+		/*
 		 * watch for ICMP host redirects
 		 *
 		 * There is no real handler code (yet). We just watch

Signature · 1 line

1.6.0.2

Alessandro Rubini

author

2009-08-07 11:59 UTC #2 lore raw

PATCH 2/4 tftp: get the tftp block size from config file and from the environment

In reply to Alessandro Rubini

Increasing the block size is useful if CONFIG_IP_DEFRAG is used. Howerver, the last fragments in a burst may overflow the receiving ethernet, so the default is left at 1468, with thre new CONFIG_TFTP_BLOCKSIZE for config files. Further, "tftpblocksize" can be set in the environment.

Signed-off-by Alessandro Rubini <[email protected]>

---
 net/tftp.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

net/tftp.c +11 -2

index b0f1cca..34b79c4 100644
@@ -86,8 +86,14 @@ extern flash_info_t flash_info[];
 /* 512 is poor choice for ethernet, MTU is typically 1500.
  * Minus eth.hdrs thats 1468.  Can get 2x better throughput with
  * almost-MTU block sizes.  At least try... fall back to 512 if need be.
+ * (but those using CONFIG_IP_DEFRAG may want to set a larger block in cfg file)
  */
+#ifdef CONFIG_TFTP_BLOCKSIZE
+#define TFTP_MTU_BLOCKSIZE CONFIG_TFTP_BLOCKSIZE
+#else
 #define TFTP_MTU_BLOCKSIZE 1468
+#endif
+
 static unsigned short TftpBlkSize=TFTP_BLOCK_SIZE;
 static unsigned short TftpBlkSizeOption=TFTP_MTU_BLOCKSIZE;

@@ -475,9 +481,12 @@ TftpTimeout (void)
 void
 TftpStart (void)
 {
-#ifdef CONFIG_TFTP_PORT
 	char *ep;             /* Environment pointer */
-#endif
+
+	/* Allow the user to choose tftpblocksize */
+	if ((ep = getenv("tftpblocksize")) != NULL)
+		TftpBlkSizeOption = simple_strtol(ep, NULL, 10);
+	debug("tftp block size is %i\n", TftpBlkSizeOption);
 
 	TftpServerIP = NetServerIP;
 	if (BootFile[0] == '\0') {

Signature · 1 line

1.6.0.2

Alessandro Rubini

author

2009-08-07 11:59 UTC #3 lore raw

PATCH 3/4 nfs: accept CONFIG_NFS_READ_SIZE from config file

In reply to Alessandro Rubini

To take advantage of defragmented packets, the config file can define CONFIG_NFS_READ_SIZE to override the 1kB default. No support is there for an environment variable by now.

Signed-off-by Alessandro Rubini <[email protected]>

---
 net/nfs.h |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

net/nfs.h +8 -2

index 712afa0..de8a0c6 100644
@@ -38,8 +38,14 @@
 
 /* Block size used for NFS read accesses.  A RPC reply packet (including  all
  * headers) must fit within a single Ethernet frame to avoid fragmentation.
- * Chosen to be a power of two, as most NFS servers are optimized for this.  */
-#define NFS_READ_SIZE   1024
+ * However, if CONFIG_IP_DEFRAG is set, the config file may want to use a
+ * bigger value. In any case, most NFS servers are optimized for a power of 2.
+ */
+#ifdef CONFIG_NFS_READ_SIZE
+#define NFS_READ_SIZE CONFIG_NFS_READ_SIZE
+#else
+#define NFS_READ_SIZE 1024 /* biggest power of two that fits Ether frame */
+#endif
 
 #define NFS_MAXLINKDEPTH 16

Signature · 1 line

1.6.0.2

Alessandro Rubini

author

2009-08-07 11:59 UTC #4 lore raw

PATCH 4/4 arm nomadik: activate defrag choose 4k transfer block size

In reply to Alessandro Rubini

This chooses 4kB data size for both TFTP and NFS, as an example about how to use support for IP fragments.

Signed-off-by Alessandro Rubini <[email protected]>

---
 include/configs/nhk8815.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

include/configs/nhk8815.h +4 -0

index 67b72dc..4c3c4f8 100644
@@ -147,6 +147,10 @@
 #define CONFIG_SMC_USE_32_BIT
 #define CONFIG_BOOTFILE		"uImage"
 
+#define CONFIG_IP_DEFRAG	/* Allows faster download, TFTP and NFS */
+#define CONFIG_TFTP_BLOCKSIZE	4096
+#define CONFIG_NFS_READ_SIZE	4096
+
 /* Storage information: onenand and nand */
 #define CONFIG_CMD_ONENAND
 #define CONFIG_MTD_ONENAND_VERIFY_WRITE

Signature · 1 line

1.6.0.2

Ben Warren

2009-08-08 09:50 UTC #5 lore raw

cover Network defrag

In reply to Alessandro Rubini

Allesandro,

Quoted 24 lines · Alessandro Rubini

I finally fixed the defrag code, testing with NFS as well. Didn't take performance figures, tough, for lack of time.
I wanted to do "config + environment" for the NFS case, like tftp, but didnt' do the second step out of laziness (also, the source file has long lines while I have 80 columns).
For the record, I added the check on ip_src and ip_dst, but everything stopped working. So I reverted that instead of entering a long debugging session.
The CONFIG_NET_MAXDEFRAG argument is the actual payload, so I add NFS overhead to that figure, which is expected to a beautiful 4096 or 8192. I feel this is better than other options, as the person writing the config is not expected to know how much protocol overhead is there.
Alessandro Rubini (4):
net: defragment IP packets
tftp: get the tftp block size from config file and from the
environment
nfs: accept CONFIG_NFS_READ_SIZE from config file arm nomadik: activate defrag choose 4k transfer block size

Quoted 6 lines

 include/configs/nhk8815.h |    4 +
 net/net.c                 |  188 +++++++++++++++++++++++++++++++++++++++++++-
 net/nfs.h                 |   10 ++-
 net/tftp.c                |   13 +++-
 4 files changed, 206 insertions(+), 9 deletions(-)

patches 1-4 applied to net/next.

thanks!
Ben

Robin Getz

2009-08-10 19:51 UTC #6 lore raw

cover Network defrag

On Sat 8 Aug 2009 05:50, Ben Warren pondered:

Quoted 5 lines

Allesandro,
Quoted 2 lines · Alessandro Rubini
I finally fixed the defrag code, testing with NFS as well. Didn't take performance figures, tough, for lack of time.

Checking out performance with tftp - server is Linux 2.6.27 located in Germany, client (U-Boot running net/next) is located in the USA. Packets fragmented, and arriving out of order.

U-Boot image (214268 bytes)

tftpblocksize download rate
(bytes) (ms) (bytes/second)

   512              48,246         4,441
  1024              24,482         8,752
  1468 ( 1MTU)      17,243        12,426
  2048              12,517        17,118
  2948 ( 2MTU)       8,912        24,042
  4428 ( 3MTU)       6,167        34,744
  4096               6,608        32,425
  5908 ( 4MTU)       4,840        44,270
  7388 ( 5MTU)       4,042        53,010
  8192               3,641        58,848
  8868 ( 6MTU)       3,425        62,560
 10348 ( 7MTU)       2,974        72,047
 11828 ( 8MTU)       2,736        78,314
 13308 ( 9MTU)       2,508        85,433
 14788 (10MTU)       2,281        93,935
 16000               2,233        95,955
 16268 (11MTU)       2,174        98,559

So, that is 17-seconds (default - on the master), to 2 seconds. Wow - a 87% reduction!

Doing the same with a larger image (default kernel + ext2 file system == 12Meg), gets similar results - goes from 928,688 ms / 12,493 bytes/second (yeah, that is 15 minutes with a tftpblocksize == 1468) to 107,626 ms / 107,866 bytes/second (that is under two minutes with a tftpblocksize == 16000)... Again - an 88% reduction in time spent waiting...

So - I think this is well worth the effort for those people who want to use tftp outside of a local network. (on a local network - things do not change that drastically - An 18Meg file with default tftpblocksize (1468 bytes) took 5084ms to download, and with a tftpblocksize of 16268, it about half the time - 2625 ms...

Thanks to Alessandro for putting it together.

Feel free to add my Signed-off (once the docs have been updated explaining what this is all for).

-Robin

Ben Warren

2009-08-10 19:57 UTC #7 lore raw

cover Network defrag

Quoted 36 lines · Robin Getz

On Sat 8 Aug 2009 05:50, Ben Warren pondered:
Quoted 7 lines
Allesandro,
Quoted 3 lines · Alessandro Rubini
I finally fixed the defrag code, testing with NFS as well. Didn't take performance figures, tough, for lack of time.
Checking out performance with tftp - server is Linux 2.6.27 located in Germany, client (U-Boot running net/next) is located in the USA. Packets fragmented, and arriving out of order.
U-Boot image (214268 bytes)
tftpblocksize download rate
(bytes) (ms) (bytes/second)
   512              48,246         4,441
  1024              24,482         8,752
  1468 ( 1MTU)      17,243        12,426
  2048              12,517        17,118
  2948 ( 2MTU)       8,912        24,042
  4428 ( 3MTU)       6,167        34,744
  4096               6,608        32,425
  5908 ( 4MTU)       4,840        44,270
  7388 ( 5MTU)       4,042        53,010
  8192               3,641        58,848
  8868 ( 6MTU)       3,425        62,560
 10348 ( 7MTU)       2,974        72,047
 11828 ( 8MTU)       2,736        78,314
 13308 ( 9MTU)       2,508        85,433
 14788 (10MTU)       2,281        93,935
 16000               2,233        95,955
 16268 (11MTU)       2,174        98,559

Quoted 4 lines

So, that is 17-seconds (default - on the master), to 2 seconds. Wow - a 87% reduction!

Wicked cool.

Quoted 12 lines

Doing the same with a larger image (default kernel + ext2 file system == 12Meg), gets similar results - goes from 928,688 ms / 12,493 bytes/second (yeah, that is 15 minutes with a tftpblocksize == 1468) to 107,626 ms / 107,866 bytes/second (that is under two minutes with a tftpblocksize == 16000)... Again - an 88% reduction in time spent waiting...
So - I think this is well worth the effort for those people who want to use tftp outside of a local network. (on a local network - things do not change that drastically - An 18Meg file with default tftpblocksize (1468 bytes) took 5084ms to download, and with a tftpblocksize of 16268, it about half the time - 2625 ms...

Quoted 6 lines

Thanks to Alessandro for putting it together.
Feel free to add my Signed-off (once the docs have been updated explaining what this is all for).

I'll do that. Thanks for all your help.

Quoted 2 lines

-Robin

regards,
Ben

Wolfgang Denk

2009-08-10 19:59 UTC #22 lore raw

cover Network defrag

In reply to Robin Getz

Dear Robin Getz,

Quoted 4 lines · In message <[email protected]> you wrote:

Checking out performance with tftp - server is Linux 2.6.27 located in Germany, client (U-Boot running net/next) is located in the USA. Packets fragmented, and arriving out of order.

...

Quoted 2 lines

So, that is 17-seconds (default - on the master), to 2 seconds. Wow - a 87% reduction!

Thanks for testing & reporting!

Best regards,

Wolfgang Denk

Signature · 6 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Hi there! This is just a note from me, to you, to tell you, the  per-
son  reading this note, that I can't think up any more famous quotes,
jokes, nor bizarre stories, so you may as well go home.

Robin Getz

2009-08-12 15:48 UTC #8 lore raw

cover Network defrag

In reply to Ben Warren

On Mon 10 Aug 2009 15:57, Ben Warren pondered:

Quoted 8 lines

Quoted 6 lines · Robin Getz
Thanks to Alessandro for putting it together.
Feel free to add my Signed-off (once the docs have been updated explaining what this is all for).
I'll do that. Thanks for all your help.

Some info for the docs, when I was troubleshooting a Ubuntu 9.04 install.
======

It appears that some tftp servers (the older BSD version, Debian's "tftpd") doesn't support RFC 2348 (blksize), and always use a block size of 512 :( You need to make sure that you install the the Peter Anvin version, which does support RFC 2348, and blksize up to 65,464 bytes (Debian's "tftpd-hpa").
http://www.kernel.org/pub/software/network/tftp/

How to tell which you have? Check your man page: "man tftpd" or ask for the version:

Peter Anvin version:
rgetz at imhotep:~> /usr/sbin/in.tftpd -V
tftp-hpa 0.48, with remap, with tcpwrappers
rgetz at imhotep:~>

non-RFC 2348 one:
rgetz at imhotep:~> /usr/sbin/in.tftpd -V
rgetz at imhotep:~>

Even when using the Peter Anvin version, block size can still be reduced via two methods:
-B : limits the maximum permitted block size (-B 512)
-r : rejects RFC 2347 TFTP options (-r blksize)
Check your local /etc/xinetd.d/tftp or /etc/inetd.conf file to see how your tftpd server is being invoked.

===========

-Robin

Wolfgang Denk

2009-08-12 20:04 UTC #9 lore raw

cover Network defrag

Dear Robin Getz,

Quoted 2 lines · In message <[email protected]> you wrote:

Some info for the docs, when I was troubleshooting a Ubuntu 9.04 install.

Good info, but bad format. Can you please submit this as a patch?

Thanks in advance.

Best regards,

Wolfgang Denk

Signature · 5 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A year spent in artificial intelligence is enough to make one believe
in God.

Ben Warren

2009-08-12 20:05 UTC #11 lore raw

cover Network defrag

In reply to Robin Getz

Hi Robin,

Quoted 14 lines · Robin Getz

On Mon 10 Aug 2009 15:57, Ben Warren pondered:
Quoted 11 lines
Quoted 7 lines · Robin Getz
Thanks to Alessandro for putting it together.
Feel free to add my Signed-off (once the docs have been updated explaining what this is all for).
I'll do that. Thanks for all your help.

Quoted 9 lines

Some info for the docs, when I was troubleshooting a Ubuntu 9.04 install.
======
It appears that some tftp servers (the older BSD version, Debian's "tftpd") doesn't support RFC 2348 (blksize), and always use a block size of 512 :( You need to make sure that you install the the Peter Anvin version, which does support RFC 2348, and blksize up to 65,464 bytes (Debian's "tftpd-hpa").
http://www.kernel.org/pub/software/network/tftp/

Maybe it would make sense to have a message printed if the user specifies a higher blocksize but the TFTP server doesn't respond to the 'blksize' request. Thoughts?

Ben

Robin Getz

2009-08-12 20:53 UTC #12 lore raw

cover Network defrag

On Wed 12 Aug 2009 16:05, Ben Warren pondered:

Quoted 27 lines

Hi Robin,
Quoted 17 lines · Robin Getz
On Mon 10 Aug 2009 15:57, Ben Warren pondered:
Quoted 11 lines
Quoted 7 lines · Robin Getz
Thanks to Alessandro for putting it together.
Feel free to add my Signed-off (once the docs have been updated explaining what this is all for).
I'll do that. Thanks for all your help.
Some info for the docs, when I was troubleshooting a Ubuntu 9.04 install.
======
Quoted 7 lines
It appears that some tftp servers (the older BSD version, Debian's "tftpd") doesn't support RFC 2348 (blksize), and always use a block size of 512 :( You need to make sure that you install the the Peter Anvin version, which does support RFC 2348, and blksize up to 65,464 bytes (Debian's "tftpd-hpa"):
http://www.kernel.org/pub/software/network/tftp/

Quoted 3 lines

Maybe it would make sense to have a message printed if the user specifies a higher blocksize but the TFTP server doesn't respond to the 'blksize' request. Thoughts?

It doesn't happen today...

We request 1468 (today), and if don't get an respose to the blksize request, it falls back to 512 (with the BSD tftpd) - no message to the user - it just takes longer, and people complain about a crappy network driver...

To me - this is independent of the defragmentation patch. If we request 1468 (MTU) or 1469 (which will be fragmented) - the logic in tftp.c is the same...

There is already a debug("Blocksize ack... in the code - so I'm assuming that if someone wanted to figure it out - they just need to turn on DEBUG in tftp.c - rather than printing it out all the time...

-Robin

Ben Warren

2009-08-12 20:53 UTC #13 lore raw

cover Network defrag

Quoted 34 lines · Robin Getz

On Wed 12 Aug 2009 16:05, Ben Warren pondered:
Quoted 32 lines
Hi Robin,
Quoted 28 lines · Robin Getz
On Mon 10 Aug 2009 15:57, Ben Warren pondered:
Quoted 5 lines
Quoted 2 lines · Robin Getz
Thanks to Alessandro for putting it together.
Quoted 9 lines
Quoted 6 lines
Feel free to add my Signed-off (once the docs have been updated explaining what this is all for).
I'll do that. Thanks for all your help.
Some info for the docs, when I was troubleshooting a Ubuntu 9.04 install.
======
It appears that some tftp servers (the older BSD version, Debian's "tftpd") doesn't support RFC 2348 (blksize), and always use a block size of 512 :( You need to make sure that you install the the Peter Anvin version, which does support RFC 2348, and blksize up to 65,464 bytes (Debian's "tftpd-hpa"):
http://www.kernel.org/pub/software/network/tftp/

Quoted 8 lines

Quoted 4 lines
Maybe it would make sense to have a message printed if the user specifies a higher blocksize but the TFTP server doesn't respond to the 'blksize' request. Thoughts?
It doesn't happen today...

Obviously. I'm asking if you think it would be helpful.

Quoted 8 lines

We request 1468 (today), and if don't get an respose to the blksize request, it falls back to 512 (with the BSD tftpd) - no message to the user - it just takes longer, and people complain about a crappy network driver...
To me - this is independent of the defragmentation patch. If we request 1468 (MTU) or 1469 (which will be fragmented) - the logic in tftp.c is the same...

Agreed. The code to request 'blksize' has been in place for a while, but may be more relevant now that we can have blocks MUCH bigger than the default.

Quoted 5 lines

There is already a debug("Blocksize ack... in the code - so I'm assuming that if someone wanted to figure it out - they just need to turn on DEBUG in tftp.c - rather than printing it out all the time...

Sure, if you don't mind re-compiling. I think it might be an opt-outable message via puts_quiet()

Quoted 2 lines

-Robin

regards,
Ben

Wolfgang Denk

2009-08-12 21:30 UTC #14 lore raw

cover Network defrag

Dear Ben Warren,

Quoted 3 lines · In message <[email protected]> you wrote:

Sure, if you don't mind re-compiling. I think it might be an opt-outable message via puts_quiet()

It seems we start having a mess here, with features bound to other features that have not even been agreeds about yet.

I have to admit that I am no friend of this puts_quiet() thingy. How much time do we really save on a normal system? Is this worth the inconsistent behaviour and the (IMHO much uglier) code?

Best regards,

Wolfgang Denk

Signature · 6 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The face of war has never changed.  Surely it is more logical to heal
than to kill.
	-- Surak of Vulcan, "The Savage Curtain", stardate 5906.5

Robin Getz

2009-08-13 15:44 UTC #10 lore raw

cover Network defrag

In reply to Wolfgang Denk

On Wed 12 Aug 2009 16:04, Wolfgang Denk pondered:

Quoted 8 lines

Dear Robin Getz,
Quoted 2 lines · In message <[email protected]> you wrote:
Some info for the docs, when I was troubleshooting a Ubuntu 9.04
install.
Good info, but bad format. Can you please submit this as a patch?

There wasn't any existing docs to patch, and I thought that by the comment that Ben had made:

On Mon 10 Aug 2009 15:57, Ben Warren pondered:

Quoted 6 lines

Quoted 4 lines · Robin Getz
Feel free to add my Signed-off (once the docs have been updated explaining what this is all for).
I'll do that. Thanks for all your help.

Was that he was going to add a patch for the docs...

-Robin

Robin Getz

2009-08-13 21:47 UTC #15 lore raw

cover Network defrag

In reply to Wolfgang Denk

On Wed 12 Aug 2009 17:30, Wolfgang Denk pondered:

Quoted 4 lines

Dear Ben Warren,
Quoted 1 line · In message <[email protected]> you wrote:

Quoted 8 lines

Quoted 2 lines
Sure, if you don't mind re-compiling. I think it might be an opt-outable message via puts_quiet()
It seems we start having a mess here, with features bound to other features that have not even been agreeds about yet.
I have to admit that I am no friend of this puts_quiet() thingy. How much time do we really save on a normal system?

A small fraction.

On a high speed network, with a low speed UART, and block sizes of 512 (default with BSD tftp).

With printing hashes...
uart Download
57600 6657 (ms) 2,734,393 (bytes/sec)

Without
57600 6418 (ms) 2,836,219 (bytes/sec)

As the network gets faster, and the UART gets slower - it will make a bigger difference.

Quoted 1 line

Is this worth the inconsistent behaviour

Most likely not. I'm not going to be offended if you NAK it.

The better thing to do (IMHO) - would be to print out the proper number of hashes, depending on the size of the file (and implement RFC 2349 at the same time) - not the number of packets (which is what happens today)...

Quoted 1 line

and the (IMHO much uglier) code?

The code isn't uglier - one function is replaced with a different one.

The name of the function is kind of crappy - but that is what I came up with late one evening...

Wolfgang Denk

2009-08-13 22:01 UTC #16 lore raw

cover Network defrag

Dear Robin Getz,

Quoted 5 lines · In message <[email protected]> you wrote:

Quoted 3 lines
I have to admit that I am no friend of this puts_quiet() thingy. How much time do we really save on a normal system?
A small fraction.

Thanks for measuring it.

Quoted 2 lines

As the network gets faster, and the UART gets slower - it will make a bigger difference.

Indeed - OTOH, we usually see console baudrates of 115kbps these days...

Quoted 3 lines

Quoted 1 line
Is this worth the inconsistent behaviour
Most likely not. I'm not going to be offended if you NAK it.

Thanks :-)

Quoted 3 lines

The better thing to do (IMHO) - would be to print out the proper number of hashes, depending on the size of the file (and implement RFC 2349 at the same time) - not the number of packets (which is what happens today)...

Agreed. Patches welcome!

Best regards,

Wolfgang Denk

Signature · 4 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
People have one thing in common: they are all different.

Robin Getz

2009-08-17 17:15 UTC #17 lore raw

cover Network defrag

On Thu 13 Aug 2009 18:01, Wolfgang Denk pondered:

Quoted 7 lines

Dear Robin Getz,
Quoted 3 lines · In message <[email protected]> you wrote:
The better thing to do (IMHO) - would be to print out the proper number of hashes, depending on the size of the file (and implement RFC 2349 at the same time) - not the number of packets (which is what happens today)...
Agreed. Patches welcome!

Something like this?

- If turned on (CONFIG_TFTP_TSIZE), asks for the size of the file.
- if receives the file size, prints out the percentage of the file downloaded.
- when done, prints 100%
- if it doesn't receive the file size (the server doesn't support RFC 2349, prints standard hash marks.

Comments welcome...

net/tftp.c +27 -2

index 9544691..56db247 100644
@@ -66,6 +66,9 @@ static ulong	TftpLastBlock;		/* last packet sequence number received */
 static ulong	TftpBlockWrap;		/* count of sequence number wraparounds */
 static ulong	TftpBlockWrapOffset;	/* memory offset due to wrapping	*/
 static int	TftpState;
+#ifdef CONFIG_TFTP_TSIZE
+static int	TftpTsize;		/* Tsize */
+#endif
 
 #define STATE_RRQ	1
 #define STATE_DATA	2

@@ -212,6 +215,10 @@ TftpSend (void)
 		sprintf((char *)pkt, "%lu", TIMEOUT / 1000);
 		debug("send option \"timeout %s\"\n", (char *)pkt);
 		pkt += strlen((char *)pkt) + 1;
+#ifdef CONFIG_TFTP_TSIZE
+		pkt += sprintf((char *)pkt,"tsize%c%d", 0,0);
+		pkt += strlen((char *)pkt) + 1;
+#endif
 		/* try for more effic. blk size */
 		pkt += sprintf((char *)pkt,"blksize%c%d%c",
 				0,TftpBlkSizeOption,0);

@@ -321,8 +328,14 @@ TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len)
 					simple_strtoul((char*)pkt+i+8,NULL,10);
 				debug("Blocksize ack: %s, %d\n",
 					(char*)pkt+i+8,TftpBlkSize);
-				break;
 			}
+#ifdef CONFIG_TFTP_TSIZE
+			if (strcmp ((char*)pkt+i,"tsize") == 0) {
+				TftpTsize = simple_strtoul((char*)pkt+i+6,NULL,10);
+				debug("size = %s, %d\n",
+					 (char*)pkt+i+6, TftpTsize);
+			}
+#endif
 		}
 #ifdef CONFIG_MCAST_TFTP
 		parse_multicast_oack((char *)pkt,len-1);

@@ -348,7 +361,14 @@ TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len)
 			TftpBlockWrap++;
 			TftpBlockWrapOffset += TftpBlkSize * TFTP_SEQUENCE_SIZE;
 			printf ("\n\t %lu MB received\n\t ", TftpBlockWrapOffset>>20);
-		} else {
+		}
+#ifdef CONFIG_TFTP_TSIZE
+		else if (TftpTsize) {
+
+			printf("%2d\b\b", NetBootFileXferSize * 100 / TftpTsize);
+		}
+#endif
+		else {
 			if (((TftpBlock - 1) % 10) == 0) {
 				puts_quiet("#");
 			} else if ((TftpBlock % (10 * HASHES_PER_LINE)) == 0) {

@@ -442,6 +462,11 @@ TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len)
 			 *	We received the whole thing.  Try to
 			 *	run it.
 			 */
+#ifdef CONFIG_TFTP_TSIZE
+			if (TftpTsize)
+				puts("100%");
+#endif
+
 			puts_quiet("\ndone\n");
 #ifdef CONFIG_TFTP_TIME
 			end_time = get_timer(0);

@@ -550,6 +579,9 @@ TftpStart (void)
#ifdef CONFIG_TFTP_TIME
start_time = get_timer(0);
#endif
+#ifdef CONFIG_TFTP_TSIZE
+ TftpTsize = 0;
+#endif

TftpTimeoutMSecs = TftpRRQTimeoutMSecs; TftpTimeoutCountMax = TftpRRQTimeoutCountMax;

Robin Getz

2009-08-17 17:21 UTC #23 lore raw

cover Network defrag

In reply to Ben Warren

On Sat 8 Aug 2009 05:50, Ben Warren pondered:

Quoted 24 lines

Allesandro,
Quoted 21 lines · Alessandro Rubini
I finally fixed the defrag code, testing with NFS as well. Didn't take performance figures, tough, for lack of time.
I wanted to do "config + environment" for the NFS case, like tftp, but didnt' do the second step out of laziness (also, the source file has long lines while I have 80 columns).
For the record, I added the check on ip_src and ip_dst, but everything stopped working. So I reverted that instead of entering a long debugging session.
The CONFIG_NET_MAXDEFRAG argument is the actual payload, so I add NFS overhead to that figure, which is expected to a beautiful 4096 or 8192. I feel this is better than other options, as the person writing the config is not expected to know how much protocol overhead is there.
Alessandro Rubini (4):
net: defragment IP packets
tftp: get the tftp block size from config file and from the
environment

I noticed that while playing with this, that if you set the "tftpblocksize" environment, do a transfer, and then clear it, it does not go back to the default setting. I was not sure if this was the intended or not, but this fixes it (and provides a small code size reduction when this option is not activated).

Also wondering -- if the user sets the "tftpblocksize" to a number larger than IP_MAXUDP, the transfer will never finish. Should this be restricted here?

net/tftp.c +0 -0

index 9544691..56db247 100644

+#ifdef CONFIG_TFTP_BLOCKSIZE
/* Allow the user to choose tftpblocksize */ if ((ep = getenv("tftpblocksize")) != NULL)
TftpBlkSizeOption = simple_strtol(ep, NULL, 10);
+ else
+ TftpBlkSizeOption = TFTP_MTU_BLOCKSIZE;
debug("tftp block size is %i\n", TftpBlkSizeOption);
+#endif

Wolfgang Denk

2009-08-17 19:05 UTC #18 lore raw

cover Network defrag

In reply to Robin Getz

Dear Robin Getz,

Quoted 2 lines · In message <[email protected]> you wrote:

Comments welcome...

I guess the code is largely untested?

net/tftp.c static ulong TftpLastBlock; /* last packet sequence number received */ · line 71

Quoted 11 lines

net/tftp.c +3 -0

index 9544691..56db247 100644
@@ -66,6 +66,9 @@ static ulong	TftpLastBlock;		/* last packet sequence number received */
 static ulong	TftpBlockWrap;		/* count of sequence number wraparounds */
 static ulong	TftpBlockWrapOffset;	/* memory offset due to wrapping	*/
 static int	TftpState;
+#ifdef CONFIG_TFTP_TSIZE
+static int	TftpTsize;		/* Tsize */
+#endif

Why static int? This gives a random init value for the second and each following TFTP transfers.

net/tftp.c TftpSend (void) · line 220

Quoted 7 lines

net/tftp.c +3 -0

@@ -212,6 +215,10 @@ TftpSend (void)
 		sprintf((char *)pkt, "%lu", TIMEOUT / 1000);
 		debug("send option \"timeout %s\"\n", (char *)pkt);
 		pkt += strlen((char *)pkt) + 1;
+#ifdef CONFIG_TFTP_TSIZE
+		pkt += sprintf((char *)pkt,"tsize%c%d", 0,0);
+		pkt += strlen((char *)pkt) + 1;

Looks to me as if you were adding the length twice?

Quoted 6 lines

net/tftp.c +1 -0

+#endif
 		/* try for more effic. blk size */
 		pkt += sprintf((char *)pkt,"blksize%c%d%c",
 				0,TftpBlkSizeOption,0);

@@ -321,8 +328,14 @@ TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len)
 					simple_strtoul((char*)pkt+i+8,NULL,10);

net/tftp.c TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) · line 337

Quoted 10 lines

net/tftp.c +6 -1

 				debug("Blocksize ack: %s, %d\n",
 					(char*)pkt+i+8,TftpBlkSize);
-				break;
 			}
+#ifdef CONFIG_TFTP_TSIZE
+			if (strcmp ((char*)pkt+i,"tsize") == 0) {
+				TftpTsize = simple_strtoul((char*)pkt+i+6,NULL,10);
+				debug("size = %s, %d\n",
+					 (char*)pkt+i+6, TftpTsize);
+			}

It seems you rely on TftpTsize to be zero otherwise. The current code (with a static int) does not guarantee that, though.

net/tftp.c TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) · line 370

Quoted 8 lines

net/tftp.c +7 -1

-		} else {
+		}
+#ifdef CONFIG_TFTP_TSIZE
+		else if (TftpTsize) {
+
+			printf("%2d\b\b", NetBootFileXferSize * 100 / TftpTsize);
+		}
+#endif

Hm... maybe we should rather print hashes (say one '#' for each 2%, resulting in a maximum of 50 characters output.

Otherwise we actually increase the amount of characters sent over the serial line (and the intention was to reduce it, right?)

net/tftp.c TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) · line 470

Quoted 6 lines

net/tftp.c +5 -0

+#ifdef CONFIG_TFTP_TSIZE
+			if (TftpTsize)
+				puts("100%");
+#endif
+
 			puts_quiet("\ndone\n");

Here we see the bad consequences of this puts_quiet() stuff (which I really dislike. The longer I see it, the more I tend to reject it.

Here, the (necessary!) newline terminating the "100%" output, is not always sent, only when puts_quiet() is "active".

net/tftp.c TftpStart (void) · line 584

Quoted 7 lines

net/tftp.c +3 -0

@@ -550,6 +579,9 @@ TftpStart (void)
 #ifdef CONFIG_TFTP_TIME
 	start_time = get_timer(0);
 #endif
+#ifdef CONFIG_TFTP_TSIZE
+	TftpTsize = 0;
+#endif

Ah. I see :-)

Best regards,

Wolfgang Denk

Signature · 5 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Question: How does one get fresh air into a Russian church?
Answer:   One clicks on an icon, and a window opens!

Wolfgang Denk

2009-08-17 19:06 UTC #24 lore raw

cover Network defrag

In reply to Robin Getz

Dear Robin Getz,

Quoted 4 lines · In message <[email protected]> you wrote:

Also wondering -- if the user sets the "tftpblocksize" to a number larger than IP_MAXUDP, the transfer will never finish. Should this be restricted here?

Sounds like a good idea to me.

Best regards,

Wolfgang Denk

Signature · 5 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Plan to throw one away. You will anyway."
                              - Fred Brooks, "The Mythical Man Month"

Robin Getz

2009-08-17 19:55 UTC #19 lore raw

cover Network defrag

In reply to Wolfgang Denk

On Mon 17 Aug 2009 15:05, Wolfgang Denk pondered:

Quoted 7 lines

Dear Robin Getz,
Quoted 2 lines · In message <[email protected]> you wrote:
Comments welcome...
I guess the code is largely untested?

I tested it on a single machine.

Quoted 6 lines

net/tftp.c +0 -0

index 9544691..56db247 100644
@@ -66,6 +66,9 @@ static ulong	TftpLastBlock;		/* last packet sequence number received */
 static ulong	TftpBlockWrap;		/* count of sequence number wraparounds */

Quoted 8 lines

net/tftp.c static ulong TftpLastBlock; /* last packet sequence number received */ · line 71
Quoted 5 lines
net/tftp.c +3 -0
 static ulong	TftpBlockWrapOffset;	/* memory offset due to wrapping	*/
 static int	TftpState;
+#ifdef CONFIG_TFTP_TSIZE
+static int	TftpTsize;		/* Tsize */
+#endif
Why static int? This gives a random init value for the second and each following TFTP transfers.

Nope - it is set to zero on the start of every transfer.

Quoted 9 lines

net/tftp.c TftpSend (void) · line 220

Quoted 7 lines

net/tftp.c +3 -0

@@ -212,6 +215,10 @@ TftpSend (void)
 		sprintf((char *)pkt, "%lu", TIMEOUT / 1000);
 		debug("send option \"timeout %s\"\n", (char *)pkt);
 		pkt += strlen((char *)pkt) + 1;
+#ifdef CONFIG_TFTP_TSIZE
+		pkt += sprintf((char *)pkt,"tsize%c%d", 0,0);
+		pkt += strlen((char *)pkt) + 1;

Looks to me as if you were adding the length twice?

One zero is for the null char (delimiter), one zero is for ascii zero (length).

Quoted 11 lines

net/tftp.c +2 -1

+#endif
 		/* try for more effic. blk size */
 		pkt += sprintf((char *)pkt,"blksize%c%d%c",
 				0,TftpBlkSizeOption,0);

@@ -321,8 +328,14 @@ TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len)
 					simple_strtoul((char*)pkt+i+8,NULL,10);
 				debug("Blocksize ack: %s, %d\n",
 					(char*)pkt+i+8,TftpBlkSize);
-				break;
 			}
+#ifdef CONFIG_TFTP_TSIZE

Quoted 8 lines

net/tftp.c TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) · line 337
Quoted 5 lines
net/tftp.c +5 -0
+			if (strcmp ((char*)pkt+i,"tsize") == 0) {
+				TftpTsize = simple_strtoul((char*)pkt+i+6,NULL,10);
+				debug("size = %s, %d\n",
+					 (char*)pkt+i+6, TftpTsize);
+			}
It seems you rely on TftpTsize to be zero otherwise. The current code (with a static int) does not guarantee that, though.

It is set to zero below on start.

Quoted 12 lines

net/tftp.c TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) · line 370
Quoted 8 lines
net/tftp.c +7 -1
-		} else {
+		}
+#ifdef CONFIG_TFTP_TSIZE
+		else if (TftpTsize) {
+
+			printf("%2d\b\b", NetBootFileXferSize * 100 / TftpTsize);
+		}
+#endif
Hm... maybe we should rather print hashes (say one '#' for each 2%, resulting in a maximum of 50 characters output.

Quoted 2 lines

Otherwise we actually increase the amount of characters sent over the serial line (and the intention was to reduce it, right?)

Yeah, it was to reduce the output - this was easier :)

Plus spinning numbers are always nice. When you are doing a scp - do you look at the bar moving, or the numbers going up to 100%? I always look at the numbers...

I'll re-work it for a single line of 50 hashes. (one '#' == 2% of the file).

Quoted 10 lines

net/tftp.c TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) · line 470
Quoted 6 lines
net/tftp.c +5 -0
+#ifdef CONFIG_TFTP_TSIZE
+			if (TftpTsize)
+				puts("100%");
+#endif
+
 			puts_quiet("\ndone\n");
Here we see the bad consequences of this puts_quiet() stuff (which I really dislike. The longer I see it, the more I tend to reject it.

Quoted 2 lines

Here, the (necessary!) newline terminating the "100%" output, is not always sent, only when puts_quiet() is "active".

Yeah, sorry about that - I would switch the puts to the puts_quiet if that is picked up - I just never heard an ack or nack on it...

Quoted 9 lines

net/tftp.c TftpStart (void) · line 584

Quoted 7 lines

net/tftp.c +3 -0

@@ -550,6 +579,9 @@ TftpStart (void)
 #ifdef CONFIG_TFTP_TIME
 	start_time = get_timer(0);
 #endif
+#ifdef CONFIG_TFTP_TSIZE
+	TftpTsize = 0;
+#endif

Ah. I see :-)

So - you are OK with they way it is done (other comments still apply of course).

Wolfgang Denk

2009-08-17 20:20 UTC #20 lore raw

cover Network defrag

Dear Robin Getz,

Quoted 5 lines · In message <[email protected]> you wrote:

Quoted 3 lines
Why static int? This gives a random init value for the second and each following TFTP transfers.
Nope - it is set to zero on the start of every transfer.

Right, I saw this later, at the end of your patch, but was too lazy to change my whole message ;-)

Quoted 7 lines

Quoted 5 lines
net/tftp.c TftpSend (void) · line 220
Quoted 3 lines
net/tftp.c +3 -0
+#ifdef CONFIG_TFTP_TSIZE
+		pkt += sprintf((char *)pkt,"tsize%c%d", 0,0);
+		pkt += strlen((char *)pkt) + 1;
Looks to me as if you were adding the length twice?
One zero is for the null char (delimiter), one zero is for ascii zero (length).

Wel, the sprintf() already returns the number of output characters, i. e. 7.

pkt then points at the terminating '\0' character. strlen() should always return 0, then. This makes not much sense to me.

Also, why do you need sprintf() at all when the string is a constant anyway?

Why not simply:

	memcpy (pkt, "tsize\00", 7);
	pkt += 7;

Quoted 7 lines

Quoted 7 lines
net/tftp.c TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) · line 370
Quoted 3 lines
net/tftp.c +3 -0
+			printf("%2d\b\b", NetBootFileXferSize * 100 / TftpTsize);
+		}
+#endif
Hm... maybe we should rather print hashes (say one '#' for each 2%, resulting in a maximum of 50 characters output.

Quoted 8 lines

Quoted 2 lines
Otherwise we actually increase the amount of characters sent over the serial line (and the intention was to reduce it, right?)
Yeah, it was to reduce the output - this was easier :)
Plus spinning numbers are always nice. When you are doing a scp - do you look at the bar moving, or the numbers going up to 100%? I always look at the numbers...

I know what you mean. OTOH, I tend to dislike such characters sequences (with lots of backspace characters) because they always mess up log files ;-)

Quoted 1 line

I'll re-work it for a single line of 50 hashes. (one '#' == 2% of the file).

Thanks.

Quoted 2 lines

So - you are OK with they way it is done (other comments still apply of course).

Right. Thanks a lot.

Best regards,

Wolfgang Denk

Signature · 4 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Many aligators will be slain, but the swamp will remain.

Robin Getz

2009-08-17 20:35 UTC #21 lore raw

cover Network defrag

On Mon 17 Aug 2009 16:20, Wolfgang Denk pondered:

Quoted 26 lines

Dear Robin Getz,
Quoted 5 lines · In message <[email protected]> you wrote:
Quoted 3 lines
Why static int? This gives a random init value for the second and each following TFTP transfers.
Nope - it is set to zero on the start of every transfer.
Right, I saw this later, at the end of your patch, but was too lazy to change my whole message ;-)
Quoted 7 lines
Quoted 5 lines
net/tftp.c TftpSend (void) · line 220
Quoted 3 lines
net/tftp.c +3 -0
+#ifdef CONFIG_TFTP_TSIZE
+		pkt += sprintf((char *)pkt,"tsize%c%d", 0,0);
+		pkt += strlen((char *)pkt) + 1;
Looks to me as if you were adding the length twice?
One zero is for the null char (delimiter), one zero is for ascii zero (length).
Wel, the sprintf() already returns the number of output characters, i. e. 7.
pkt then points at the terminating '\0' character. strlen() should always return 0, then. This makes not much sense to me.

Quoted 7 lines

Also, why do you need sprintf() at all when the string is a constant anyway?
Why not simply:
	memcpy (pkt, "tsize\00", 7);
	pkt += 7;

That's is better - It was just a dumb copy/paste from the blksize option...

Quoted 12 lines

Quoted 12 lines
Quoted 9 lines
net/tftp.c TftpHandler (uchar * pkt, unsigned dest, unsigned src, unsigned len) · line 370
Quoted 3 lines
net/tftp.c +3 -0
+			printf("%2d\b\b", NetBootFileXferSize * 100 / TftpTsize);
+		}
+#endif
Hm... maybe we should rather print hashes (say one '#' for each 2%, resulting in a maximum of 50 characters output.
Otherwise we actually increase the amount of characters sent over the serial line (and the intention was to reduce it, right?)
Yeah, it was to reduce the output - this was easier :)

Quoted 7 lines

Quoted 3 lines
Plus spinning numbers are always nice. When you are doing a scp - do you look at the bar moving, or the numbers going up to 100%? I always look at the numbers...
I know what you mean. OTOH, I tend to dislike such characters sequences (with lots of backspace characters) because they always mess up log files ;-)

But they look pretty ... :)

Quoted 3 lines

Quoted 1 line
I'll re-work it for a single line of 50 hashes. (one '#' == 2% of the file).
Thanks.

Np.

Quoted 4 lines

Quoted 2 lines
So - you are OK with they way it is done (other comments still apply of course).
Right. Thanks a lot.

New version tomorrow...

Robin