[ovs-dev] [PATCH] [rfc] datapath: Treat GSO skbs as if they were fragments

[Simon Horman]

On Sat, Jan 01, 2011 at 06:40:41PM -0500, Jesse Gross wrote:
> On Sat, Jan 1, 2011 at 7:29 AM, Simon Horman <horms at verge.net.au> wrote:
> > In dp_output_control() GSO skbs are split into fragments which are passed
> > to userspace. So the resulting flow set-up by the controller (I am using
> > ovs-vswitchd) is created based on a fragment. In the case of UDP and TCP,
> > this means the UDP/TCP source and destination port is zero.
> >
> > In order for the datapath to match the resulting flow flow_extract() needs
> > to treat GSO skbs as if they are fragments.  That is, set the UDP/TCP
> > source and destination port to 0.
> >
> > A flow established for a GSO skb with this change won't match any
> > subsequent non-GSO skbs, they will need to be passed to the controller and
> > a new flow established. But without this change no GSO skbs will ever match
> > any flow.
> >
> > I noticed this while using KVM using virtio with VhostNet and netperf's
> > UDP_STREAM test. The result was that the test sent ~5Gbit/s but only a
> > small fraction of that was received by the other side. Much less than the
> > 1Gbit/s available on the physical link between the host (and guest) and the
> > machine running netserver. 100% of one of the host's CPUs was consumed, 50%
> > for the host and 50% for the guest.  The host consumption was contributed
> > to largely by ovs-vswitchd.
> >
> > With this change I get a much nicer result of a fraction under 1Gbit/s sent
> > and almost all packets ending up at the other end.
> 
> Hmm, you're right - this isn't currently being handled correctly.  GSO
> is just a performance optimization, so we just behave the same as if
> it didn't exist.
> 
> I think this change should only apply to UDP though - GSO for TCP
> implies TCP segmentation, whereas as UDP is IP fragmentation.  If we
> are doing segmentation we will have the TCP header, including the
> ports.

Agreed, I will send an updated patch with does that.

> Also, for consistency we should probably also set the is_frag field.

I believe that is already handled outside the context of the diff.

	if (!(nh->frag_off & htons(IP_MF | IP_OFFSET)) &&
	     !skb_is_gso(skb)) {
		...
	} else
		*is_frag = true;