[ovs-dev] [PATCH v2 2/2] datapath: Add Stateless TCP Tunneling protocol.

[Jesse Gross]

On Thu, Apr 2, 2015 at 12:18 PM, Pravin Shelar <pshelar at nicira.com> wrote:
> On Tue, Mar 31, 2015 at 5:07 PM, Jesse Gross <jesse at nicira.com> wrote:
>> On Thu, Mar 26, 2015 at 4:41 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
>>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>>> index 7f431ed..ea9c6ae 100644
>>> --- a/datapath/datapath.c
>>> +++ b/datapath/datapath.c
>>> @@ -2192,6 +2192,7 @@ static int __net_init ovs_init_net(struct net *net)
>>>
>>>         INIT_LIST_HEAD(&ovs_net->dps);
>>>         INIT_WORK(&ovs_net->dp_notify_work, ovs_dp_notify_wq);
>>> +       INIT_LIST_HEAD(&ovs_net->vport_net.stt_sock_list);
>>>         return 0;
>>>  }
>>>
>>
>> In this previous version, this was a little bit more self contained.
>> Did you run into a problem with that? If not, the other version seems
>> a little cleaner to me, especially since this won't be upstream and so
>> the less invasive it is, the better.
>>
>
> I saw dead lock if I register net-namesapce while adding vport. This
> due to ABBA locking sequence with ovs-lock and net-mutex. STT port add
> and net-exit can deadlock this way.
> vport_net already had pointers for different vport. so I think adding
> stt socket list is not much different.

OK, I see the problem. I agree it's not that bad if it causes a
problem otherwise (I guess this invalidates my other comment about
#including the STT header file in vport.h.)

>>> +static int __segment_skb(struct sk_buff **headp, bool ipv4, bool tcp, bool csum_partial, int l4_offset)
>>> +{
>>> +       int err;
>>> +
>>> +       err = straighten_frag_list(headp);
>>> +       if (unlikely(err))
>>> +               return err;
>>> +
>>> +       if (__linearize(*headp, ipv4, tcp, csum_partial))
>>> +               return skb_list_linearize(*headp, GFP_ATOMIC);
>>
>> I don't think this check is in the right place. The conditions for
>> linearizing only come into play if we can't fully coalesce things and
>> would otherwise generate a frag_list. But we don't know that before
>> trying to coalesce.
>>
>
> There is trade-off. If it is large skb we might not able to segment
> multiple skb depending on protocols and csum_partial. I decided to
> check for these parameters before so we do not waste cycles coalescing
> skbs. But I do not have strong opinion on this. It can be easily
> changed the way you suggested.

That's a good point. I think you're probably right (and hopefully this
is an edge case anyways).

>> At a higher level, is there a reason why STT is special in this regard
>> as far as inserting a header into a packet that has a frag_list? Don't
>> other tunnels need to deal with it?
>>
> frag_list is handled because not all drivers can handle skbs with
> frag_list. This can cause performance issues. Older kernel might not
> be able to handle skb geometry that STT generates.

What I'm trying to remember is how this case actually arose in the
first place as I guess that IP fragmentation wasn't really coming up.
STT is a little different in that the header that it inserts is not
applied evenly across all segments, so it can cause issues. However,
other tunneling protocols also weren't doing TSO at the time that this
code was originally written, so I'm trying to see if this isn't
something that might apply to them as well.