[U-Boot] [PATCH 0/4] Network defrag

[U-Boot] [PATCH 0/4] Network defrag

[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

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

[U-Boot] [PATCH 1/4] net: defragment IP packets

[Alessandro Rubini]

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 <rubini@gnudd.com>
---
 net/net.c |  188 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 183 insertions(+), 5 deletions(-)

diff --git a/net/net.c b/net/net.c
index 641c37c..986d614 100644
--- a/net/net.c
+++ b/net/net.c
@@ -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
-- 
1.6.0.2

[U-Boot] [PATCH 2/4] tftp: get the tftp block size from config file and from the environment

[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 <rubini@gnudd.com>
---
 net/tftp.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/tftp.c b/net/tftp.c
index b0f1cca..34b79c4 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -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') {
-- 
1.6.0.2

[U-Boot] [PATCH 3/4] nfs: accept CONFIG_NFS_READ_SIZE from config file

[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 <rubini@gnudd.com>
---
 net/nfs.h |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/nfs.h b/net/nfs.h
index 712afa0..de8a0c6 100644
--- a/net/nfs.h
+++ b/net/nfs.h
@@ -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
 
-- 
1.6.0.2

[U-Boot] [PATCH 4/4] arm nomadik: activate defrag choose 4k transfer block size

[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 <rubini@gnudd.com>
---
 include/configs/nhk8815.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/configs/nhk8815.h b/include/configs/nhk8815.h
index 67b72dc..4c3c4f8 100644
--- a/include/configs/nhk8815.h
+++ b/include/configs/nhk8815.h
@@ -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
-- 
1.6.0.2

[U-Boot] [PATCH 0/4] Network defrag

[Ben Warren]

Allesandro,

Alessandro Rubini wrote:
> 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(-)
>   
patches 1-4 applied to net/next.

thanks!
Ben

[U-Boot] [PATCH 0/4] Network defrag

[Robin Getz]

On Sat 8 Aug 2009 05:50, Ben Warren pondered:
> Allesandro,
> 
> Alessandro Rubini wrote:
> > 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

[U-Boot] [PATCH 0/4] Network defrag

[Ben Warren]

Robin Getz wrote:
> On Sat 8 Aug 2009 05:50, Ben Warren pondered:
>   
>> Allesandro,
>>
>> Alessandro Rubini wrote:
>>     
>>> 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!
>
>   
Wicked cool.
> 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).
>
>   
I'll do that.  Thanks for all your help.
> -Robin
>   

regards,
Ben

[U-Boot] [PATCH 0/4] Network defrag

[Wolfgang Denk]

Dear Robin Getz,

In message <200908101551.52052.rgetz@blackfin.uclinux.org> 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.
...
> So, that is 17-seconds (default - on the master), to 2 seconds. Wow - a 87% 
> reduction!

Thanks for testing & reporting!

Best regards,

Wolfgang Denk

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

[U-Boot] [PATCH 0/4] Network defrag

[Robin Getz]

On Mon 10 Aug 2009 15:57, Ben Warren pondered:
> Robin Getz wrote:
> > 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

[U-Boot] [PATCH 0/4] Network defrag

[Wolfgang Denk]

Dear Robin Getz,

In message <200908121148.16431.rgetz@blackfin.uclinux.org> 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

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

[U-Boot] [PATCH 0/4] Network defrag

[Ben Warren]

Hi Robin,

Robin Getz wrote:
> On Mon 10 Aug 2009 15:57, Ben Warren pondered:
>   
>> Robin Getz wrote:
>>     
>>> 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/
>   
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

[U-Boot] [PATCH 0/4] Network defrag

[Robin Getz]

On Wed 12 Aug 2009 16:05, Ben Warren pondered:
> Hi Robin,
> 
> Robin Getz wrote:
> > On Mon 10 Aug 2009 15:57, Ben Warren pondered:
> >   
> >> Robin Getz wrote:
> >>     
> >>> 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/ 
> >   
> 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

[U-Boot] [PATCH 0/4] Network defrag

[Ben Warren]

Robin Getz wrote:
> On Wed 12 Aug 2009 16:05, Ben Warren pondered:
>   
>> Hi Robin,
>>
>> Robin Getz wrote:
>>     
>>> On Mon 10 Aug 2009 15:57, Ben Warren pondered:
>>>   
>>>       
>>>> Robin Getz wrote:
>>>>     
>>>>         
>>>>> 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/ 
>>>   
>>>       
>> 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.
> 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.
> 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()
> -Robin
>   
regards,
Ben

[U-Boot] [PATCH 0/4] Network defrag

[Wolfgang Denk]

Dear Ben Warren,

In message <4A832BCE.9060100@gmail.com> 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

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

[U-Boot] [PATCH 0/4] Network defrag

[Robin Getz]

On Wed 12 Aug 2009 16:04, Wolfgang Denk pondered:
> Dear Robin Getz,
> 
> In message <200908121148.16431.rgetz@blackfin.uclinux.org> 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:
> Robin Getz wrote:
> > 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

[U-Boot] [PATCH 0/4] Network defrag

[Robin Getz]

On Wed 12 Aug 2009 17:30, Wolfgang Denk pondered:
> Dear Ben Warren,
> 
> In message <4A832BCE.9060100@gmail.com> 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?

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.

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

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

[U-Boot] [PATCH 0/4] Network defrag

[Wolfgang Denk]

Dear Robin Getz,

In message <200908131747.20194.rgetz@blackfin.uclinux.org> you wrote:
>
> > 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.

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

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

Thanks :-)

> 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

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

[U-Boot] [PATCH 0/4] Network defrag

[Robin Getz]

On Thu 13 Aug 2009 18:01, Wolfgang Denk pondered:
> Dear Robin Getz,
> In message <200908131747.20194.rgetz@blackfin.uclinux.org> 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...



diff --git a/net/tftp.c b/net/tftp.c
index 9544691..56db247 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -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;

[U-Boot] [PATCH 0/4] Network defrag

[Robin Getz]

On Sat 8 Aug 2009 05:50, Ben Warren pondered:
> Allesandro,
> 
> Alessandro Rubini wrote:
> > 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?


diff --git a/net/tftp.c b/net/tftp.c
index 9544691..56db247 100644
--- a/net/tftp.c
+++ b/net/tftp.c
+#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

[U-Boot] [PATCH 0/4] Network defrag

[Wolfgang Denk]

Dear Robin Getz,

In message <200908171315.40365.rgetz@blackfin.uclinux.org> you wrote:
>
> Comments welcome...

I guess the code is largely untested?

> diff --git a/net/tftp.c b/net/tftp.c
> index 9544691..56db247 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -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.

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

> +#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);
> +			}

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

> -		} 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?)

> +#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".

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

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

[U-Boot] [PATCH 0/4] Network defrag

[Wolfgang Denk]

Dear Robin Getz,

In message <200908171321.44317.rgetz@blackfin.uclinux.org> 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

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

[U-Boot] [PATCH 0/4] Network defrag

[Robin Getz]

On Mon 17 Aug 2009 15:05, Wolfgang Denk pondered:
> Dear Robin Getz,
> 
> In message <200908171315.40365.rgetz@blackfin.uclinux.org> you wrote:
> >
> > Comments welcome...
> 
> I guess the code is largely untested?

I tested it on a single machine.

> > diff --git a/net/tftp.c b/net/tftp.c
> > index 9544691..56db247 100644
> > --- a/net/tftp.c
> > +++ b/net/tftp.c
> > @@ -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.

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

> > @@ -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).

> > +#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);
> > +			}
> 
> 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.

> > -		} 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?)

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

> > +#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".

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

> > @@ -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).

[U-Boot] [PATCH 0/4] Network defrag

[Wolfgang Denk]

Dear Robin Getz,

In message <200908171555.31016.rgetz@blackfin.uclinux.org> you wrote:
>
> > 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 ;-)

> > > +#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;

> > > +			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 :)
> 
> 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 ;-)

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

Thanks.

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

Right. Thanks a lot.

Best regards,

Wolfgang Denk

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

[U-Boot] [PATCH 0/4] Network defrag

[Robin Getz]

On Mon 17 Aug 2009 16:20, Wolfgang Denk pondered:
> Dear Robin Getz,
> 
> In message <200908171555.31016.rgetz@blackfin.uclinux.org> you wrote:
> >
> > > 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 ;-)
> 
> > > > +#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;

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

> > > > +			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 :)
> > 
> > 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 ... :)

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

Np.

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

New version tomorrow...

Robin