[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH net-next v2 2/5] xen-netback: Add support for IPv6 checksum offload from guest
> -----Original Message----- > From: Wei Liu [mailto:wei.liu2@xxxxxxxxxx] > Sent: 08 October 2013 17:19 > To: Paul Durrant > Cc: Wei Liu; xen-devel@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; David Vrabel; > Ian Campbell > Subject: Re: [PATCH net-next v2 2/5] xen-netback: Add support for IPv6 > checksum offload from guest > > On Tue, Oct 08, 2013 at 02:50:27PM +0100, Paul Durrant wrote: > [...] > > > > -#define PKT_PROT_LEN (ETH_HLEN + \ > > > > - VLAN_HLEN + \ > > > > - sizeof(struct iphdr) + MAX_IPOPTLEN + \ > > > > - sizeof(struct tcphdr) + MAX_TCP_OPTION_SPACE) > > > > +#define PKT_PROT_LEN 128 > > > > > > > > > > Where does 128 come from? > > > > > > > It's just an arbitrary power of 2 that was chosen because it seems to > > cover most likely v6 headers and all v4 headers. > > > > Hmm... How about using the value of MAX_TCP_HEADER? I guess that can > cover all V4 / V6 headers. > > MAX_TCP_HEADER varies, depending on configuration. To make sure we can > accommodate all guests packet we might need to use its maximum value > which can be as big as 128 + 128 + 48. > Because we always double-copy (as the grant copy doesn't copy direct to the linear area) I was concerned about making the pullup too big. I'd rather optimize around a smaller header so how about we stick with 128 but if maybe_pull_tail() finds it needs to pullup then it just pulls up to MAX_TCP_HEADER, so we limit to pullups to maximum of 2? > > > > if (recalculate_partial_csum) { > > > > struct tcphdr *tcph = tcp_hdr(skb); > > > > + > > > > + header_size = skb->network_header + > > > > + off + > > > > + sizeof(struct tcphdr) + > > > > + MAX_TCP_OPTION_SPACE; > > > > + maybe_pull_tail(skb, header_size); > > > > + > > > > > > I presume this function is checksum_setup stripped down to handle IPv4 > > > packet. What's the purpose of changing its behaviour? Why the pull_tail > > > here? > > > > > > > We have to make sure that the TCP header is in the linear area as we > > are about to write to the checksum field. In practice, the 128 byte > > pull should guarantee this but in case that is varied later I wanted > > to make sure things did not start to fail in an add way. > > > > If you already set the pull size to maximum possible value then this > will not be necessary anymore, right? > > > > > + while ((off <= sizeof(struct ipv6hdr) + > > > > ntohs(ipv6h->payload_len)) > > > && > > > > + !done) { > > > > + /* We only access at most the first 2 bytes of any > > > > option > > > header > > > > + * to determine the next one. > > > > + */ > > > > + header_size = skb->network_header + off + 2; > > > > + maybe_pull_tail(skb, header_size); > > > > + > > > > > > Will this cause performance problem? Is it possible that you pull too > > > many times? > > > > > > > I guess it means we may get two pulls for the TCP/UDP headers rather > > than one so could push the pulls into the individual cases if you > > think it will affect performance that badly. > > Hmm... Not sure I get what you mean here. The main problem I'm seeing is > that maybe_pull_tail is called in every loop. > > I would like to see as few pulls as possible because __pskb_pull_tail > can be expensive and only expected to use in "exceptional cases" (quoted > from the comment above that function). > > Is it possible to pull TCP_MAX_HEADER bytes once to eliminate all other > pulls in checksum_setup{,_ipv4,_ipv6}? > Note that the function is called *maybe*_pull_tail(). It only pulls if it needs to :-) Paul _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |