From: Wei Liu <wei.liu2@citrix•com>
To: Paul Durrant <Paul.Durrant@citrix•com>
Cc: Wei Liu <wei.liu2@citrix•com>,
"xen-devel@lists•xen.org" <xen-devel@lists•xen.org>,
"netdev@vger•kernel.org" <netdev@vger•kernel.org>,
David Vrabel <david.vrabel@citrix•com>,
Ian Campbell <Ian.Campbell@citrix•com>
Subject: Re: [PATCH net-next v2 2/5] xen-netback: Add support for IPv6 checksum offload from guest
Date: Tue, 8 Oct 2013 17:19:25 +0100 [thread overview]
Message-ID: <20131008161925.GL28411@zion.uk.xensource.com> (raw)
In-Reply-To: <9AAE0902D5BC7E449B7C8E4E778ABCD012F345@AMSPEX01CL01.citrite.net>
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.
> > > 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}?
>
[...]
> >
> > Can you make the logic explicit here?
> >
> > case IPPROTO_TCP:
> > case IPPROTO_UDP:
> > done = true;
> > break;
> > default:
> > break;
> >
> > Another minor suggestion is that change "done" to "found" because you're
> > trying to find the two type of headers.
> >
>
> Yes, I could code it that way if you prefer.
>
Explicity is better than implicity IMHO. After this change could you
also move the default branch (netdev_err) in the following "switch" to
the first "switch".
Wei.
next prev parent reply other threads:[~2013-10-08 16:19 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-10-08 10:58 [PATCH net-next v2 0/5] xen-netback: IPv6 offload support Paul Durrant
2013-10-08 10:58 ` [PATCH net-next v2 1/5] xen-netback: add IPv6 checksum offload to guest Paul Durrant
2013-10-08 13:10 ` Wei Liu
2013-10-08 13:14 ` Paul Durrant
2013-10-08 10:58 ` [PATCH net-next v2 2/5] xen-netback: Add support for IPv6 checksum offload from guest Paul Durrant
2013-10-08 13:30 ` Wei Liu
2013-10-08 13:50 ` Paul Durrant
2013-10-08 16:19 ` Wei Liu [this message]
2013-10-10 9:37 ` Paul Durrant
2013-10-08 10:58 ` [PATCH net-next v2 3/5] xen-netback: Unconditionally set NETIF_F_RXCSUM Paul Durrant
2013-10-08 10:58 ` [PATCH net-next v2 4/5] xen-netback: handle IPv6 TCP GSO packets from the guest Paul Durrant
2013-10-08 13:31 ` Wei Liu
2013-10-08 13:42 ` Paul Durrant
2013-10-08 13:50 ` [Xen-devel] " Jan Beulich
2013-10-08 10:58 ` [PATCH net-next v2 5/5] xen-netback: enable IPv6 TCP GSO to " Paul Durrant
2013-10-08 13:34 ` Wei Liu
2013-10-08 13:40 ` Paul Durrant
2013-10-09 4:41 ` [Xen-devel] " annie li
2013-10-09 10:26 ` Paul Durrant
2013-10-10 2:19 ` annie li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131008161925.GL28411@zion.uk.xensource.com \
--to=wei.liu2@citrix$(echo .)com \
--cc=Ian.Campbell@citrix$(echo .)com \
--cc=Paul.Durrant@citrix$(echo .)com \
--cc=david.vrabel@citrix$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=xen-devel@lists$(echo .)xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox