[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH net-next v3 2/5] xen-netback: add support for IPv6 checksum offload from guest
> -----Original Message----- > From: xen-devel-bounces@xxxxxxxxxxxxx [mailto:xen-devel- > bounces@xxxxxxxxxxxxx] On Behalf Of Paul Durrant > Sent: 11 October 2013 12:10 > To: Wei Liu > Cc: netdev@xxxxxxxxxxxxxxx; Ian Campbell; Wei Liu; David Vrabel; xen- > devel@xxxxxxxxxxxxx > Subject: Re: [Xen-devel] [PATCH net-next v3 2/5] xen-netback: add support > for IPv6 checksum offload from guest > > > -----Original Message----- > > From: Wei Liu [mailto:wei.liu2@xxxxxxxxxx] > > Sent: 11 October 2013 11:10 > > To: Paul Durrant > > Cc: xen-devel@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; Wei Liu; David > Vrabel; > > Ian Campbell > > Subject: Re: [PATCH net-next v3 2/5] xen-netback: add support for IPv6 > > checksum offload from guest > > > > On Thu, Oct 10, 2013 at 04:25:30PM +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 > > > > I'm not against changing this, but could you please add comment on why > > 128 is chosen. > > > > Hmm. I did mod the comment, but that change seems to have got lost > somewhere. > > > > > > [...] > > > + while ((off <= sizeof(struct ipv6hdr) + ntohs(ipv6h->payload_len)) > > && > > > + !done) { > > > + switch (nexthdr) { > > > + case IPPROTO_FRAGMENT: { > > > + struct frag_hdr *hp = (void *)(skb->data + off); > > > + > > > + header_size = skb->network_header + > > > + off + > > > + sizeof(struct frag_hdr); > > > + maybe_pull_tail(skb, header_size); > > > + > > > + nexthdr = hp->nexthdr; > > > + off += 8; > > > > Can you use sizeof(frag_hdr) instead of 8? > > > > > + break; > > > + } > > > + case IPPROTO_DSTOPTS: > > > + case IPPROTO_HOPOPTS: > > > + case IPPROTO_ROUTING: { > > > + struct ipv6_opt_hdr *hp = (void *)(skb->data + off); > > > + > > > + header_size = skb->network_header + > > > + off + > > > + sizeof(struct ipv6_opt_hdr); > > > + maybe_pull_tail(skb, header_size); > > > + > > > + nexthdr = hp->nexthdr; > > > + off += ipv6_optlen(hp); > > > + break; > > > + } > > > + case IPPROTO_AH: { > > > + struct ip_auth_hdr *hp = (void *)(skb->data + off); > > > + > > > + header_size = skb->network_header + > > > + off + > > > + sizeof(struct ip_auth_hdr); > > > + maybe_pull_tail(skb, header_size); > > > + > > > + nexthdr = hp->nexthdr; > > > + off += (hp->hdrlen+2)<<2; > > > + break; > > > + } > > > + case IPPROTO_TCP: > > > + case IPPROTO_UDP: > > > + found = true; > > > + /* Fall through */ > > > + default: > > > + done = true; > > > + break; > > > > The above 'switch' doesn't seem to cover all IPPROTO_*. Will it cause > > the loop to exit too early? In other words, does any IPPROTO_* not > > listed above marks the end of parsing? > > > > AFAIK, hitting anything not in that switch would mean we don't have a TCP or > UDP packet. I think the original code structure made that clearer so I'm going > to go back to that. > Looking at this again I realize I should not parse the fragment header; there's no way anyone should be sending a fragment with partial checksum. Paul > > [...] > > > unsigned long now = jiffies; > > > @@ -1428,12 +1598,7 @@ static int xenvif_tx_submit(struct xenvif *vif, > int > > budget) > > > > > > xenvif_fill_frags(vif, skb); > > > > > > - /* > > > - * If the initial fragment was < PKT_PROT_LEN then > > > - * pull through some bytes from the other fragments to > > > - * increase the linear region to PKT_PROT_LEN bytes. > > > - */ > > > - if (skb_headlen(skb) < PKT_PROT_LEN && > > skb_is_nonlinear(skb)) { > > > + if (skb_is_nonlinear(skb) && skb_headlen(skb) < > > PKT_PROT_LEN) { > > > > This change is not necessary: the comment is useful, and swapping two > > operands of && doesn't have any effect. > > Sorry, that was too much cutting and pasting around. I'll just ditch that > hunk. > > Paul > > > > > Wei. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@xxxxxxxxxxxxx > http://lists.xen.org/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |