On Thu, Apr 19, 2012 at 03:53:35PM -0700, Jesse Gross wrote: > On Wed, Apr 18, 2012 at 9:50 PM, Simon Horman <ho...@verge.net.au> wrote: > > This is a not yet well exercised implementation of STT intended for review, > > I am sure there are numerous areas that need improvement. > > > > In particular: > > - The transmit path's generation of partial checksums needs to be tested > > - The VLAN stripping code needs to be excercised > > - The code needs to be exercised in the presence of HW checksumming > > - In general, the code has been exercised by running Open vSwtich in > > KVM guests on the same host. Testing between physucal hosts is needed. > > > > This implementation is based on the CAPWAP implementation and in particular > > includes defragmentation code almost identical to CAPWAP. It seems to me > > that while fragmentation can be handled by GSO/TSO, defragmentation code is > > needed in STT in the case where LRO/GRO doesn't reassemble an entire STT > > frame for some reason. > > > > If the defragmentation code, which is of non-trivial length, remains more > > or less in its present state then there is some scope for consolidation > > with CAPWAP. Other code that may possibly be consolidated with CAPWAP has > > been marked accordingly. > > > > This code depends on a encap_rcv hook being added to the Linux Kernel's TCP > > stack. A patch to add such a hook will be posted separately. Ultimately > > this change or some alternative will need to be applied to the mainline > > Linux kernel's TCP stack if STT is to be widely deployed. Motivating this > > change to the TCP stack is part of the purpose of this prototype STT > > implementation. > > > > The configuration of STT is analogous to that of other tunneling > > protocols such as GRE which are supported by Open vSwtich. > > > > e.g. > > > > ovs-vsctl add bridge project0 ports @newport \ > > -- --id=@newport create port name=stt0 interfaces=[@newinterface] \ > > -- --id=@newinterface create interface name=stt0 type=stt > > options="remote_ip=10.0.99.192,key=64" > > > > Signed-off-by: Simon Horman <ho...@verge.net.au> > > > > --- > > > > v3 > > * Correct stripping of vlan tag on transmit > > * Correct setting of vlan TCI on recieve > > - Use __vlan_hwaccel_put_tag instead of vlan_put_tag > > * Use encap_rcv_enable() to enable receiving packets from the TCP stack > > - This is an update for the new implementation of the TCP stack > > patch that adds encap_rcv > > * call pskb_may_pull() for STT_FRAME_HLEN + ETH_HLEN bytes in > > process_stt_proto() as this is required by ovs_flow_extract() > > * Include "stt: " in pr_fmt > > * Make use of pr_* instead of printk > > * Rate limit all packet-generated pr_* messages > > * STT flags are 8bits wide so don't define them using __cpu_to_be16() > > * Only include l4_offset if > > 1. get_ip_summed(skb) is OVS_CSUM_PARTIAL > > 2. skb->csum_start is non-zero > > 3. it is between 0 and 255 > > - Warn if the first two conditions are met but not the third one. > > * Only set STT_FLAG_CHECKSUM_VERIFIED if > > get_ip_summed(skb) is * OVS_CSUM_UNNECESSARY > > * Print a debug message if get_ip_summed(skb) is OVS_CSUM_UNNECESSARY, > > this case is yet to be exercised > > * In the rx path, adjust skb->csum_start to take into account pulling > > STT_FRAME_HLEN if get_ip_summed(skb) is OVS_CSUM_PARTIAL > > Hi Simon, > > It looks like most of things that I mentioned in comments on the > previous version are still present here. Is this just an intermediate > version?
Sorry, I hadn't noticed this email until now. In general my aim was to make a functionally correct implementation and then concentrate on acceleration. As it happens the implementation is not yet correct and I have ended up implementing some acceleration. So things are somewhat work in progress. My intention in marking this as [RFC] was to indicate that I don't think it is ready for merging yet. Ideally I would like STT considered for merging once it is correct, even if there is still (ample) scope for performance improvements. > > * Warn if skb->dev is NULL on defragmentation and stop processing the skb. > > - This fixes a crash bug > > - But how can this occur? > > The encap_rcv hook that you added to the TCP stack comes after the > skb->dev = NULL line. This is why I wanted to push it is as close as > possible to the socket lookup. Thanks! The reason I pushed it further down was because I was observing some locking issues. I'll revisit the TCP stack code. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
