Re: [U-Boot] [PATCH] Add simple IP/UDP fragmentation support

27 Aug 2008

Hi Josh & Frank,
A few nits.  Overall a nice patch.
One question: Will multicast TFTP still work when this is applied?
Josh Boyer wrote:
...
From: Frank Haverkamp haver@vnet.ibm.com
http://tools.ietf.org/html/rfc2348 describes the TFTP block size option
which allows larger packtes than the 512 byte default. This reduces the
s/packtes/packets/
...
number of TFTP ACKs significantly and improves performance.
To get the most benefit out of the tftp block size option the support
of defragementation of IP/UDP packet is helpful. The current implemenation
s/defragementation/defragmentation/
s/implemenation/implementation/
...
should work even with packets received out of order. To enable the large
packet size the user should set "tftp_block_size" so a value like 16352.
s/so/to/
...
We experimented with different packet sizes and found that more than those
16KiB do not contribute much to the performance anymore. Therefor I limited
s/Therefor/Therefore/
...
the defragmentation buffer to 16KiB no too waste memory.
so as to not waste memory
...
Signed-off-by: Frank Haverkamp haver@vnet.ibm.com
Signed-off-by: Josh Boyer jwboyer@linux.vnet.ibm.com

include/net.h |   17 ++++++
 net/net.c     |  156 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
 net/tftp.c    |   22 ++++++++
 net/tftp.h    |   10 +++
 4 files changed, 185 insertions(+), 20 deletions(-)

--- u-boot.git.orig/include/net.h
+++ u-boot.git/include/net.h
@@ -200,6 +200,13 @@ typedef struct {
   ushort		udp_xsum;	/* Checksum			*/
 } IP_t;
+#define IP_OFFS			 0x1FFF	/* ip offset *= 8 */
+#define	  IP_OFFS_SHIFT	  3	/* in 8 byte steps */
+#define IP_FLAGS	 0xE000	/* first 3 bits */
+#define		 IP_FLAGS_RES	 0x8000	/* reserved */
+#define		 IP_FLAGS_DFRAG	 0x4000	/* don't fragments */
+#define		 IP_FLAGS_MFRAG	 0x2000	/* more fragments */



Please align these better.  After applying, it's:
#define IP_OFFS			 0x1FFF	/* ip offset *= 8 */
#define	  IP_OFFS_SHIFT	  3	/* in 8 byte steps */
#define IP_FLAGS	 0xE000	/* first 3 bits */
#define		 IP_FLAGS_RES	 0x8000	/* reserved */
#define		 IP_FLAGS_DFRAG	 0x4000	/* don't fragments */
#define		 IP_FLAGS_MFRAG	 0x2000	/* more fragments */
...
#define IP_HDR_SIZE_NO_UDP	(sizeof (IP_t) - 8)
 #define IP_HDR_SIZE		(sizeof (IP_t))
@@ -282,6 +289,16 @@ typedef struct icmphdr {
 #define PKTSIZE_ALIGN		1536
 /*#define PKTSIZE		608*/

/*

IP/UDP Fragmentation support



See: http://en.wikipedia.org/wiki/IPv4#Fragmentation_and_reassembly



MAX possible UDP packet size is 64 KiB, if there is memory available.


*/

+#define NET_ETH_MTU		1500
+#define NET_FRAG_BUF_SIZE	(16 * 1024) /* MAX is 64 KiB */
+#define NET_UDP_FRAG_SIZE	(NET_ETH_MTU - IP_HDR_SIZE_NO_UDP) /* 1480 */
+#define NET_FRAG_BUF_USED	(NET_FRAG_BUF_SIZE / NET_UDP_FRAG_SIZE + 1)



/*

Maximum receive ring size; that is, the number of packets
we can buffer before overflow happens. Basically, this just

--- u-boot.git.orig/net/net.c
+++ u-boot.git/net/net.c
@@ -192,6 +192,15 @@ volatile uchar	PktBuf[(PKTBUFSRX+1) * PK
volatile uchar *NetRxPackets[PKTBUFSRX]; /* Receive packets			*/
+/* Packet fragmentation support */
+static uint16_t ip_id = 0; /* sequence number */
+static uint16_t udp_len = 0;
+static uint16_t udp_src = 0;
+static uint16_t udp_dst = 0;
+static int max_idx = 0;
+static uchar NetFragBuf[NET_FRAG_BUF_SIZE];
+static char NetFragBufUsed[NET_FRAG_BUF_USED] = { 0, };



static rxhand_f *packetHandler;		/* Current RX packet handler		*/
 static thand_f *timeHandler;		/* Current timeout handler		*/
 static ulong	timeStart;		/* Time base value			*/
@@ -288,6 +297,13 @@ NetLoop(proto_t protocol)
 {
   bd_t *bd = gd->bd;

/* Packet fragmentation support */
ip_id = udp_len = udp_src = udp_dst = max_idx = 0;
memset(NetFragBuf, 0xFF, sizeof(NetFragBuf));
memset(NetFragBufUsed, 0, sizeof(NetFragBufUsed));
printf("NetFragBuf @ %08x max tftp_block_size=%d udp_frag_size=%d\n",
      NetFragBuf, TFTP_BLOCK_SIZE_MAX, NET_UDP_FRAG_SIZE);




#ifdef CONFIG_NET_MULTI
   NetRestarted = 0;
   NetDevExists = 0;
@@ -1150,6 +1166,39 @@ static void CDPStart(void)
 }
 #endif
+#ifdef CONFIG_UDP_CHECKSUM
+/*


@sumptr: Points to UDP data



@sumlen: Size of UDP data



@xsum: UDP checksum across IP source, destination address, protocol and size







Returns 0 when checksum is correct and 1 if it is not



Can you return -1 on failure?  Not a big deal, just more conventional.
...

*/

+static int udp_checksum(ushort *sumptr, ushort sumlen, ulong xsum)
+{

  while (sumlen > 1) {


      ushort sumdata;



      sumdata = *sumptr++;


      xsum += ntohs(sumdata);


      sumlen -= 2;


  }


  if (sumlen > 0) {


      ushort sumdata;



      sumdata = *(unsigned char *) sumptr;


      sumdata = (sumdata << 8) & 0xff00;


      xsum += sumdata;


  }


  while ((xsum >> 16) != 0) {


      xsum = (xsum & 0x0000ffff) + ((xsum >> 16) & 0x0000ffff);


  }


  if ((xsum != 0x00000000) && (xsum != 0x0000ffff))


      return 1;



  return 0;



+}
+#endif /* CONFIG_UDP_CHECKSUM */
void
 NetReceive(volatile uchar * inpkt, int len)
@@ -1164,6 +1213,7 @@ NetReceive(volatile uchar * inpkt, int l
   int iscdp;
 #endif
   ushort cti = 0, vlanid = VLAN_NONE, myvlanid, mynvlanid;

uint32_t off;		/* ip_off for fragmentation */

Can you pick a better variable name than 'off'?  'offset' maybe?
...
#ifdef ET_DEBUG
   printf("packet received\n");
@@ -1404,9 +1454,11 @@ NetReceive(volatile uchar * inpkt, int l
   	if ((ip->ip_hl_v & 0xf0) != 0x40) {
   		return;
   	}
+#if 0		/* Obsolete after adding the fragmentation support */
   	if (ip->ip_off & htons(0x1fff)) { /* Can't deal w/ fragments */
   		return;
   	}
+#endif
Please delete this block if it's now dead code.
...
/* can't deal with headers > 20 bytes */
if ((ip->ip_hl_v & 0x0f) > 0x05) {
	return;

@@ -1422,6 +1474,88 @@ NetReceive(volatile uchar * inpkt, int l
 #endif
   		return;
   	}


/*


 * Fragmentation support. We need to check the ip_id


 * and if all fragments were received correctly.


 */


off = (ntohs(ip->ip_off) & IP_OFFS) << IP_OFFS_SHIFT;


if ((off != 0) || (ip->ip_off & htons(IP_FLAGS_MFRAG))) {


	int size, idx, complete;


	char *start;



	/* New fragmented packet arrived, clear data. */


	if (ntohs(ip->ip_id) != ip_id) {


		ip_id = ntohs(ip->ip_id);


		memset(NetFragBufUsed, 0, sizeof(NetFragBufUsed));


		udp_len = udp_src = udp_dst = max_idx = 0;


	}



	idx = off / NET_UDP_FRAG_SIZE;



	/* Packet does not fit into IP/UDP fragmentation buf */


	if (idx >= NET_FRAG_BUF_USED) {


		return;


	}



	NetFragBufUsed[idx] = 1;



	/* Copy the UDP hdr with the data for 1st


	   fragment, else copy just payload */


	if (off == 0) {


		udp_len = ntohs(ip->udp_len);


		udp_src = ntohs(ip->udp_src);


		udp_dst = ntohs(ip->udp_dst);


	}


	size = ntohs(ip->ip_len) - IP_HDR_SIZE_NO_UDP;


	start = (char *)ip + IP_HDR_SIZE_NO_UDP;


	memcpy(NetFragBuf + off, start, size);



	/*


	 * When last fragement has been received we




s/fragement/fragment/
...

	 * know the number of fragments we expect. If


	 * all have arrived we process the packet.


	 */


	if (((off != 0) && !(ip->ip_off & htons(IP_FLAGS_MFRAG))))


		max_idx = idx;



	if (max_idx == 0)


		return;



	complete = 1;


	for (idx = 0; idx < max_idx; idx++) {


		if (NetFragBufUsed[idx] == 0) {


			complete = 0;


			break;


		}


	}


	if (!complete)


		return;



+#ifdef CONFIG_UDP_CHECKSUM

	if (ip->udp_xsum != 0) {


		ulong xsum = ip->ip_p;


		uint16_t *sumptr;



		xsum += udp_len;


		xsum += (ntohl(ip->ip_src) >> 16) & 0xffff;


		xsum += (ntohl(ip->ip_src) >>  0) & 0xffff;


		xsum += (ntohl(ip->ip_dst) >> 16) & 0xffff;


		xsum += (ntohl(ip->ip_dst) >>  0) & 0xffff;


		sumptr = (ushort *)NetFragBuf;



		if (udp_checksum(sumptr, udp_len, xsum)) {


			putc('U');


			return;


		}


	}



+#endif /* CONFIG_UDP_CHECKSUM */

	(*packetHandler)(NetFragBuf + 8,


			 udp_dst,


			 udp_src,


			 udp_len - 8);




Can some of these arguments share lines?
...

	return;


}


/*
watch for ICMP host redirects




@@ -1502,26 +1636,8 @@ NetReceive(volatile uchar * inpkt, int l
   		sumlen = ntohs(ip->udp_len);
   		sumptr = (ushort *) &(ip->udp_src);

	while (sumlen > 1) {


		ushort sumdata;



		sumdata = *sumptr++;


		xsum += ntohs(sumdata);


		sumlen -= 2;


	}


	if (sumlen > 0) {


		ushort sumdata;



		sumdata = *(unsigned char *) sumptr;


		sumdata = (sumdata << 8) & 0xff00;


		xsum += sumdata;


	}


	while ((xsum >> 16) != 0) {


		xsum = (xsum & 0x0000ffff) + ((xsum >> 16) & 0x0000ffff);


	}


	if ((xsum != 0x00000000) && (xsum != 0x0000ffff)) {


		printf(" UDP wrong checksum %08lx %08x\n",


			xsum, ntohs(ip->udp_xsum));




	if (udp_checksum(sumptr, sumlen, xsum)) {


		putc('U');
	return;
}

}

--- u-boot.git.orig/net/tftp.c
+++ u-boot.git/net/tftp.c
@@ -456,6 +456,7 @@ TftpTimeout (void)
 void
 TftpStart (void)
 {

char *s, *err;

#ifdef CONFIG_TFTP_PORT
   char *ep;             /* Environment pointer */
 #endif
@@ -518,6 +519,27 @@ TftpStart (void)
puts ("Loading: *\b");

/* Get alternate tftp_block_size */
if ((s = getenv("tftp_block_size")) != NULL) {

Kind of a long environment variable name.  Maybe "tftp_bs"?
...

err = NULL;



TftpBlkSizeOption = simple_strtoul(s, &err, 10);


if (*err) {


	printf("ERR: \"tftp_block_size\" is not a number\n");


	TftpBlkSizeOption = TFTP_BLOCK_SIZE;


}


/*


 * Reject values which require extensive handling.


 * block size of 1428 octets (Ethernet MTU, less


 * the TFTP, UDP and IP header lengths).


 */


if (TftpBlkSizeOption > TFTP_BLOCK_SIZE_MAX) {


	printf("ERR: tftp_block_sizes larger than %d not "


	       "supported\n", TFTP_BLOCK_SIZE_MAX);


	TftpBlkSizeOption = TFTP_BLOCK_SIZE;


}


}
NetSetTimeout (TIMEOUT * CFG_HZ, TftpTimeout);
 NetSetHandler (TftpHandler);

--- u-boot.git.orig/net/tftp.h
+++ u-boot.git/net/tftp.h
@@ -8,11 +8,21 @@
 #ifndef __TFTP_H__
 #define __TFTP_H__
+#include <net.h>



/**********************************************************************/
 /*

Global functions and variables.

*/
+/*


Maximum TFTP block size bound to max size of fragmented IP/UDP



packets minus TFTP and UDP/IP overhead. TFTP overhead is 2 byte



opcode and 2 byte block-number.


*/

+#define TFTP_BLOCK_SIZE_MAX     (NET_FRAG_BUF_SIZE - sizeof(IP_t) - 4)




/* tftp.c */
 extern void	TftpStart (void);	/* Begin TFTP get */
regards,
Ben

    

Re: [U-Boot] [PATCH] Add simple IP/UDP fragmentation support

Ben Warren