[ovs-dev] [PATCH] stt: linearize for CONFIG_SLUB case

[pravin shelar]

On Mon, Apr 4, 2016 at 1:56 PM, Jesse Gross <jesse at kernel.org> wrote:
> On Fri, Apr 1, 2016 at 4:58 PM, pravin shelar <pshelar at ovn.org> wrote:
>> On Thu, Mar 31, 2016 at 9:06 PM, Jesse Gross <jesse at kernel.org> wrote:
>>> On Thu, Mar 31, 2016 at 2:30 PM, Pravin B Shelar <pshelar at ovn.org> wrote:
>>>> diff --git a/datapath/linux/compat/stt.c b/datapath/linux/compat/stt.c
>>>> index eb397e8..ae33d5e 100644
>>>> --- a/datapath/linux/compat/stt.c
>>>> +++ b/datapath/linux/compat/stt.c
>>>>  static int coalesce_skb(struct sk_buff **headp)
>>>>  {
>>>> -       struct sk_buff *frag, *head, *prev;
>>>> +       struct sk_buff *frag, *head;
>>>> +#ifndef SKIP_ZERO_COPY
>>>> +       struct sk_buff *prev;
>>>> +#endif
>>>>         int err;
>>>>
>>>>         err = straighten_frag_list(headp);
>>>
>>> I don't think that we need to straighten the frag list in the
>>> SKIP_ZERO_COPY case. It's basically just undoing what the for loop
>>> that forms the frag list will do. __skb_linearize() will be able to
>>> handle it in any case.
>>>
>> I can skip straightening and work on skb with frag-list, but I do not
>> want to complicate the code.  This case is pretty rare after the
>> change in reassemble where complete skb is allocated on first set
>> segment.
>
> I don't think that it should increase complexity - I believe that we
> can just skip the call to straighten_frag_list() and everything else
> will continue to work.
>
One difference is that the fragments iterator reads the list from
skb->next, which is what straighten_frag_list() generate. If we skip
the call the list is in shinfo-frag-list.
Let me see if this can be simplified a bit.

>>>> +#ifdef SKIP_ZERO_COPY
>>>> +       if (skb_shinfo(head)->frag_list) {
>>>> +               err = __skb_linearize(head);
>>>> +               return err;
>>>> +       }
>>>> +#endif
>>>
>>> Maybe move this to try_to_segment()? It seems like it is a little more
>>> consistent.
>>>
>> But it is not same. I think we only need to linearize of there is
>> frag-list, AFAIK non linear data in skb_shinfo can be safely handled
>> in skb-segment.
>
> OK, that's true.
>
>>> This is effectively optimizing for the case where all packets are
>>> large and will generate a frag list after reassembly. However, it's
>>> possible in many situations that packets can be reassembled with zero
>>> copy that doesn't later need to be split (maybe the total packet is
>>> around 20k). Do you know how the performance compares vs. just
>>> linearizing at the end in that case? Is there a benefit to doing an
>>> early copy?
>>>
>> We are not trying to coalesce skb in case of SKIP_ZERO_COPY, so any
>> partial skb segment will generate multiple inner segments. I have seen
>> better performance with this approach rather than linearizing at the
>> end.
>
> Do you know how the performance compares for medium sized packets
> where we would be able to coalesce without linearizing? It would be an
> interesting intermediate datapoint between very small and very large
> packets.

ok, I will check it.