Hi Duncan, Still some issues, but getting closer to parsable.
The subject of this should be something like, "net: Adjust UDP implementation to prepare for TCP support" On Wed, Mar 7, 2018 at 10:43 PM, <d...@synoia.com> wrote: > From: Duncan Hare <duncanch...@yahoo.com> > >>>>>> > <<<< I think these are coming from your commit log. You should remove them. > > cover-letter: You need to use the exact tags in the documentation. That means that you need to capitalize the 'C'. > Why netboot: The first line here is the subject for the series, so you should have the line that you used to have as the patches' titles in v7. > Central management, including logs and change control, > coupled with with enhanced security and unauthorized > change detection and remediation by exposing a > small attack surface. > > Why TCP: > > Currently file transfer are done using tftp or NFS both > over udp. This requires a request to be sent from client > (u-boot) to the boot server. > > For a 4 Mbyte kernel, with a 1k block size this requires > 4,000 request for a block. > > Using a large block size, one greater than the Ethernet > maximum frame size limitation, would require fragmentation, > which u-boot supports. However missing fragment recovery > requires timeout detection and re-transmission requests > for missing fragments. > > UDP is ideally suited to fast single packet exchanges, > inquiry/response, for example dns, becuse of the lack of > connection overhead. > > UDP as a file transport mechanism is slow, even in low > latency networks, because file transfer with udp requires > poll/response mechanism to provide transfer integrity. > > In networks with large latency, for example: the internet, > UDP is even slower. What is a 30 second transfer on a local > boot server and LAN increase to over 3 minutes, because of > all the requests/response traffic. > > This was anticipated in the evolution of the IP protocols > and TCP was developed and then enhanced for high latency high > bandwidth networks. > > The current standard is TCP with selective acknowledgment. > > In our testing we have reduce kernel transmission time to > around 0.4 seconds for a 4Mbyte kernel, with a 100 Mbps > downlink. > > Why http and wget: > > HTTP is the most efficient file retrieval protocol in common > use. The client send a single request, after TCP connection, > to receive a file of any length. > > WGET is the application which implements http file transfer > outside browsers as a file transfer protocol. Versions of > wget exists on many operating systems. > END > This stuff is great for a cover letter. I would recommend that you also include the relevant TCP section and wget section in those patches. > Signed-off-by: Duncan Hare <dh...@synoia.com> > --- > > Changed in this patch: > include/net.h > net/net.c This isn't really needed since the patch already lists the files changed... see below about 12 lines. > > Added a protocol parameter to ip packet sending in net.c > Added UDP protocol for current applications to minimize > code changes to existing net apps. This stuff should not be in a patman tag. The default (untagged) is what ends up in the commit message. In other words, don't put this stuff in the "Commit-notes:" tag. In patman, "notes" means that you are providing notes to the reviewer that you do not want to end up in git. > > All the code is new, and not copied from any source. > > > Changes in v8: > Initial changes for adding TCP > > include/net.h | 25 +++++++++++++++++++------ > net/net.c | 52 ++++++++++++++++++++++++++++++++++------------------ > net/ping.c | 9 ++------- > 3 files changed, 55 insertions(+), 31 deletions(-) > > diff --git a/include/net.h b/include/net.h > index 455b48f6c7..7e5f5a6a5b 100644 > --- a/include/net.h > +++ b/include/net.h > @@ -15,17 +15,26 @@ > #include <asm/cache.h> > #include <asm/byteorder.h> /* for nton* / ntoh* stuff */ > > -#define DEBUG_LL_STATE 0 /* Link local state machine changes */ > -#define DEBUG_DEV_PKT 0 /* Packets or info directed to the > device */ > -#define DEBUG_NET_PKT 0 /* Packets on info on the network at > large */ > +#define DEBUG_LL_STATE 0 /* Link local state machine changes */ > +#define DEBUG_DEV_PKT 0 /* Packets or info directed to the device */ > +#define DEBUG_NET_PKT 0 /* Packets on info on the network at large */ > #define DEBUG_INT_STATE 0 /* Internal network state changes */ > > /* > * The number of receive packet buffers, and the required packet buffer > * alignment in memory. > * > + * The number of buffers for TCP is used to calculate a static TCP window > + * size, becuse TCP window size is a promise to the sending TCP to be > able > + * to buffer up to the window size of data. > + * When the sending TCP has a window size of outstanding unacknowledged > + * data, the sending TCP will stop sending. > */ > > +#if defined(CONFIG_TCP) > +#define CONFIG_SYS_RX_ETH_BUFFER 12 /* For TCP */ > +#endif > + > #ifdef CONFIG_SYS_RX_ETH_BUFFER > # define PKTBUFSRX CONFIG_SYS_RX_ETH_BUFFER > #else > @@ -354,6 +363,7 @@ struct vlan_ethernet_hdr { > > #define IPPROTO_ICMP 1 /* Internet Control Message Protocol */ > #define IPPROTO_UDP 17 /* User Datagram Protocol */ > +#define IPPROTO_TCP 6 /* Transmission Control Protocol */ > > /* > * Internet Protocol (IP) header. > @@ -596,10 +606,10 @@ int net_set_ether(uchar *xet, const uchar > *dest_ethaddr, uint prot); > int net_update_ether(struct ethernet_hdr *et, uchar *addr, uint prot); > > /* Set IP header */ > -void net_set_ip_header(uchar *pkt, struct in_addr dest, struct in_addr > source); > +void net_set_ip_header(uchar *pkt, struct in_addr dest, struct in_addr > source, > + u16 pkt_len, u8 prot); > void net_set_udp_header(uchar *pkt, struct in_addr dest, int dport, > - int sport, int len); > - > + int sport, int len); > /** > * compute_ip_checksum() - Compute IP checksum > * > @@ -670,6 +680,9 @@ static inline void net_send_packet(uchar *pkt, int len) > * @param sport Source UDP port > * @param payload_len Length of data after the UDP header > */ > +int net_send_ip_packet(uchar *ether, struct in_addr dest, int dport, int > sport, > + int payload_len, int proto); > + > int net_send_udp_packet(uchar *ether, struct in_addr dest, int dport, > int sport, int payload_len); > > diff --git a/net/net.c b/net/net.c > index 4259c9e321..df4e7317e7 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -181,6 +181,7 @@ int net_ntp_time_offset; > static uchar net_pkt_buf[(PKTBUFSRX+1) * PKTSIZE_ALIGN + PKTALIGN]; > /* Receive packets */ > uchar *net_rx_packets[PKTBUFSRX]; > + > /* Current UDP RX packet handler */ > static rxhand_f *udp_packet_handler; > /* Current ARP RX packet handler */ > @@ -777,11 +778,23 @@ void net_set_timeout_handler(ulong iv, thand_f *f) > } > > int net_send_udp_packet(uchar *ether, struct in_addr dest, int dport, int > sport, > - int payload_len) > + int payload_len) > +{ > + return net_send_ip_packet(ether, dest, dport, sport, payload_len, > + IPPROTO_UDP); > +} > + > +int net_send_ip_packet(uchar *ether, struct in_addr dest, int dport, int > sport, > + int payload_len, int proto) > { > uchar *pkt; > int eth_hdr_size; > int pkt_hdr_size; > + if (proto == IPPROTO_UDP) { > + debug_cond(DEBUG_DEV_PKT, > + "UDP Send (to=%pI4, from=%pI4, len=%d)\n", > + &dest, &net_ip, payload_len); > + } > > /* make sure the net_tx_packet is initialized (net_init() was called) > */ > assert(net_tx_packet != NULL); > @@ -799,9 +812,15 @@ int net_send_udp_packet(uchar *ether, struct in_addr > dest, int dport, int sport, > pkt = (uchar *)net_tx_packet; > > eth_hdr_size = net_set_ether(pkt, ether, PROT_IP); > - pkt += eth_hdr_size; > - net_set_udp_header(pkt, dest, dport, sport, payload_len); > - pkt_hdr_size = eth_hdr_size + IP_UDP_HDR_SIZE; > + switch (proto) { > + case IPPROTO_UDP: > + net_set_udp_header(pkt + eth_hdr_size, dest, > + dport, sport, payload_len); > + pkt_hdr_size = eth_hdr_size + IP_UDP_HDR_SIZE; > + break; > + default: > + return -1; > + } > > /* if MAC address was not discovered yet, do an ARP request */ > if (memcmp(ether, net_null_ethaddr, 6) == 0) { > @@ -1157,9 +1176,6 @@ void net_process_received_packet(uchar *in_packet, int > len) > /* Can't deal with anything except IPv4 */ > if ((ip->ip_hl_v & 0xf0) != 0x40) > return; > - /* Can't deal with IP options (headers != 20 bytes) */ > - if ((ip->ip_hl_v & 0x0f) > 0x05) > - return; > /* Check the Checksum of the header */ > if (!ip_checksum_ok((uchar *)ip, IP_HDR_SIZE)) { > debug("checksum bad\n"); > @@ -1205,6 +1221,7 @@ void net_process_received_packet(uchar *in_packet, int > len) > * we send a tftp packet to a dead connection, or when > * there is no server at the other end. > */ > + > if (ip->ip_p == IPPROTO_ICMP) { > receive_icmp(ip, len, src_ip, et); > return; > @@ -1365,8 +1382,7 @@ common: > } > /**********************************************************************/ > > -int > -net_eth_hdr_size(void) > +int net_eth_hdr_size(void) > { > ushort myvlanid; > > @@ -1426,25 +1442,28 @@ int net_update_ether(struct ethernet_hdr *et, uchar > *addr, uint prot) > } > } > > -void net_set_ip_header(uchar *pkt, struct in_addr dest, struct in_addr > source) > +void net_set_ip_header(uchar *pkt, struct in_addr dest, struct in_addr > source, > + u16 pkt_len, u8 prot) > { > struct ip_udp_hdr *ip = (struct ip_udp_hdr *)pkt; > > /* > - * Construct an IP header. > + * Construct an IP header. > */ > /* IP_HDR_SIZE / 4 (not including UDP) */ > ip->ip_hl_v = 0x45; > ip->ip_tos = 0; > - ip->ip_len = htons(IP_HDR_SIZE); > + ip->ip_len = htons(pkt_len); > ip->ip_id = htons(net_ip_id++); > - ip->ip_off = htons(IP_FLAGS_DFRAG); /* Don't fragment */ > + ip->ip_off = htons(IP_FLAGS_DFRAG); /* Don't fragment */ > ip->ip_ttl = 255; > + ip->ip_p = prot; > ip->ip_sum = 0; > /* already in network byte order */ > net_copy_ip((void *)&ip->ip_src, &source); > /* already in network byte order */ > net_copy_ip((void *)&ip->ip_dst, &dest); > + ip->ip_sum = compute_ip_checksum(ip, IP_HDR_SIZE); > } > > void net_set_udp_header(uchar *pkt, struct in_addr dest, int dport, int > sport, > @@ -1460,11 +1479,8 @@ void net_set_udp_header(uchar *pkt, struct in_addr > dest, int dport, int sport, > if (len & 1) > pkt[IP_UDP_HDR_SIZE + len] = 0; > > - net_set_ip_header(pkt, dest, net_ip); > - ip->ip_len = htons(IP_UDP_HDR_SIZE + len); > - ip->ip_p = IPPROTO_UDP; > - ip->ip_sum = compute_ip_checksum(ip, IP_HDR_SIZE); > - > + net_set_ip_header(pkt, dest, net_ip, IP_UDP_HDR_SIZE + len, > + IPPROTO_UDP); > ip->udp_src = htons(sport); > ip->udp_dst = htons(dport); > ip->udp_len = htons(UDP_HDR_SIZE + len); > diff --git a/net/ping.c b/net/ping.c > index 9508cf1160..254b646193 100644 > --- a/net/ping.c > +++ b/net/ping.c > @@ -20,16 +20,11 @@ struct in_addr net_ping_ip; > static void set_icmp_header(uchar *pkt, struct in_addr dest) > { > /* > - * Construct an IP and ICMP header. > + * Construct an ICMP header. > */ > - struct ip_hdr *ip = (struct ip_hdr *)pkt; > struct icmp_hdr *icmp = (struct icmp_hdr *)(pkt + IP_HDR_SIZE); > > - net_set_ip_header(pkt, dest, net_ip); > - > - ip->ip_len = htons(IP_ICMP_HDR_SIZE); > - ip->ip_p = IPPROTO_ICMP; > - ip->ip_sum = compute_ip_checksum(ip, IP_HDR_SIZE); > + net_set_ip_header(pkt, dest, net_ip, IP_ICMP_HDR_SIZE, IPPROTO_ICMP); > > icmp->type = ICMP_ECHO_REQUEST; > icmp->code = 0; > -- > 2.11.0 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot