Re: [U-Boot] [PATCH/RFC] net: defragment IP packets

On Fri 24 Jul 2009 04:04, Alessandro Rubini pondered:

This patch add a quick and dirty defrag step in IP reception. This allows to increase the TFTP block size and get more performance in slow links (but at that point it should be made configurable).

The overhead is negligible, verified with an ARM9 CPU and 10MB data file, changing the server MTU from 1500 to 800 and then 550. However, on a LAN connection, I didn't see advantes with using a 4k block size with default MTU.

Signed-off-by: Alessandro Rubini rubini@gnudd.com

This patch is over mainline, without the (much appreciated) cleanup patch that reached the list these days.

net/net.c | 46 ++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/net/net.c b/net/net.c index 641c37c..5034a2e 100644 --- a/net/net.c +++ b/net/net.c @@ -1117,6 +1117,46 @@ static void CDPStart(void) } #endif

Should have a CONFIG_ something - to make this conditional.

+/* This only reassembles fragments that come in proper order */ +static inline IP_t *NetDefragment(IP_t *ip, int *lenp) +{

• static uchar pkt_buff[16384]; /*temporary arbitrary limit */
• static int next_fragment;
• static ushort pkt_id;

pkt_buff needs to be aligned - in case you want to start poking in it (which you are going to want to do...) just add a:

__attribute__((aligned(PKTALIGN)));

• #define IPSZ 20

I think what you want is "IP_HDR_SIZE_NO_UDP" - from include/net.h

• uchar *pkt = (uchar *)ip;
• ushort ip_off;
• int offset, len = *lenp -2;

Some ethernet drivers are messed up, and provide bogus lengths (add a few bytes that shouldn't be there).

It's better to get the length from the IP header. Adjusted to dump the header:

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

• ip_off = ntohs(ip->ip_off);
• if (!(ip_off & (IP_OFFS | IP_FLAGS_MFRAG)))

• return ip;
  

Why not do this in NetReceive()?

JMP/RTS are much more expensive on most archs than a if()

• offset = (ip_off & IP_OFFS) * 8;
• if (!offset) { /* new packet begins, discard any we might have

*/

• pkt_id = ip->ip_id;
  

• memcpy(pkt_buff, ip, len);
  

• next_fragment = len;
  

• return NULL;
  

• }
• /* further fragment: discard IP header */
• offset += IPSZ; len -= IPSZ; pkt += IPSZ;
• if (ip->ip_id != pkt_id || offset != next_fragment)

• return NULL; /* out of order */
  

We should check more than packet id - in case it is coming from a different host...

if (ip->ip_id != (IP_t *)pkt_buff->ip_id || /* check the packet ID */ ip->ip_p != (IP_t *)pkt_buff->ip_p || /* check the protocol */ ip->ip_src != (IP_t *)pkt_buff->ip_src || /* check the source */ ip->ip_dst != (IP_t *)pkt_buff->ip_dst ) /* check the dest */

• /* further fragment: skip ip header (we miss offset_of...) */
• memcpy(pkt_buff + next_fragment, pkt, len);
• next_fragment += len;
• if (ip_off & IP_FLAGS_MFRAG)

• return NULL; /* more expected */
  

• *lenp = next_fragment;
• return (IP_t *)pkt_buff;

+}

void NetReceive(volatile uchar * inpkt, int len) @@ -1360,6 +1400,8 @@ NetReceive(volatile uchar * inpkt, int len) break;

case PROT_IP:

• if (!(ip = NetDefragment(ip, &len)))
  

• 	return;
  

#ifdef ET_DEBUG puts ("Got IP\n"); #endif

I guess this is includes some generic cleanup - but at least the reassembly should happen _after_ the sanity checks are done. Something like:

case PROT_IP: #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; } /* can't deal with anything except IPv4 */ if ((ip->ip_hl_v & 0xf0) != 0x40) { debug("I only understand IPv4\n"); return; } /* can't deal with IP options (headers != 20 bytes) */ if ((ip->ip_hl_v & 0x0f) * 4 != IP_HDR_SIZE_NO_UDP) { debug("Can't deal with IP options\n"); return; } /* Check the Checksum of the header */ if (!NetCksumOk((uchar *)ip, IP_HDR_SIZE_NO_UDP / 2)) { puts ("IP header checksum bad\n"); return; } /* Check the packet length */ if (len < ntohs(ip->ip_len)) { printf("len bad %d < %d\n", len, ntohs(ip->ip_len)); return; } /* If it is not for us, ignore it */ tmp = NetReadIP(&ip->ip_dst); if (NetOurIP && tmp != NetOurIP && tmp != 0xFFFFFFFF) { #ifdef CONFIG_MCAST_TFTP if (Mcast_addr != tmp) #endif return; } #ifdef CONFIG_NET_FRAGMENT /* If it is a IP fragment, try to combine it with it */ if (ntohs(ip->ip_off) & (IP_OFFS | IP_FLAGS_MFRAG)) { debug("received fragmented packet\n"); ip = NetDefragment(ip); if (!ip) return; } #endif len = ntohs(ip->ip_len); #ifdef ET_DEBUG printf("len=%d, v=%02x\n", len, ip->ip_hl_v & 0xff); #endif

@@ -1378,10 +1420,6 @@ NetReceive(volatile uchar * inpkt, int len) 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 */ if ((ip->ip_hl_v & 0x0f) > 0x05) { return;

I also had (for easier testing)

@@ -478,8 +497,15 @@ TftpStart (void) #ifdef CONFIG_TFTP_PORT char *ep; /* Environment pointer */ #endif

TftpServerIP = NetServerIP; +#ifdef CONFIG_NET_FRAGMENT + { + char *tmp; + tmp = getenv("tftpblocksize") ; + if (tmp) + TftpBlkSizeOption = simple_strtol(tmp, NULL, 10); + else + TftpBlkSizeOption = TFTP_MTU_BLOCKSIZE; + debug("using TftpBlkSizeOption = %d\n", TftpBlkSizeOption); + } +#endif if (BootFile[0] == '\0') { sprintf(default_filename, "%02lX%02lX%02lX%02lX.img", NetOurIP & 0xFF,

and I was doing md5 or sha1 on things to make sure that things came over properly...

-Robin