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

[Alessandro Rubini <rubini-list@gnudd.com>]

Thanks for your comments.

>> +#ifndef CONFIG_TFTP_MAXBLOCK
>> +#define CONFIG_TFTP_MAXBLOCK 16384
> 
> It is more than tftp - nfs could also use the same.

Yes, I know. But most users are tftp ones. And if you want an even
number (like 16k) as a tftp packet you need to add the headers and the
sequence count. And I prefer to have the useful number in the config.
So I used "TFTP" in the name in order for NFS users to know they must
make some calculation.
 
> How about CONFIG_NET_MAXDEFRAG instead?

We could have MAXPAYLOAD if we count in NFS overhead as well (I don't
know how much it is, currently.  Hope you see my point.

>> +static IP_t *__NetDefragment(IP_t *ip, int *lenp)
>> +{
> 
> I don't understand the purpose of the lenp.
> 
> The calling function doesn't use the len var, except for ICMP_ECHO_REQUEST, 
> which are not allowed to be fragmented.
> 
> I eliminated it - and suffered no side effects.

Well, since the caller has this "len" variable, I didn't want to leave
it corrupted. But if it's actually unused after this point, we can well
discard it.

>> +	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);
>> +	}
> 
> I'm not sure the reset if we loose a packet, or get a bad one - start over is 
> a great idea.

Well, either we keep more than one in-reassembly packet (and storage begins
to be a problem here) or not. I prefer not. 
 
> For some reason - why I'm ping flooding when tftping a large file (with large 
> tftp block size) - things hang. If I set the block size to under the MTU - it 
> works fine. Do you get the same?

Didn't try, and I can't do that today. I suspect either your ping is
over-mtu, so each new fragment triggers the above code, or simply your
ether+uboot can't keep up with the data rate.

As eaplained in the cover letter <cover.1248943812.git.rubini@unipv.it>
some fragments can be lost in high traffic, as polling mode doesn't allow
to enqueue packets.  So I think you just loose some fragments, as target
CPU time is eaten by the ping packets, and you don't get the complete
reassembled packet any more.

I'm pretty sure it's like this.

On the other hand, I found a minor issue in this situation:
	- start a tftp transfer
	- ctrl-C it
	- start another

Server retransmissions for the first transfer go into the defrag
engine e that reset-defrag-data code is triggered, so a packet may be
lost, and I get a sporadic T in the receiving u-boot.  I think it's
not a real problem, though --- or, now that I rethink about it, it
can be the same issue as above: my ether can't enqueue 8k of stuff
so a fragment is lost in that case.  
 
>> +#else /* !CONFIG_IP_DEFRAG */
>> +
>> +static inline IP_t *NetDefragment(IP_t *ip, int *lenp)
>> +{
>> +	return ip;
>> +}
>> +#endif
> 
> This needs to have the same logic (ip_off & (IP_OFFS | IP_FLAGS_MFRAG)) as the 
> above function. See comment below.

Yes, correct. Thanks.

/alessandro