On Tue, Apr 26, 2016 at 6:04 PM, Jesse Gross <je...@kernel.org> wrote:
> On Mon, Apr 25, 2016 at 2:35 PM, Pravin B Shelar <pshe...@ovn.org> wrote:
>> diff --git a/datapath/linux/compat/stt.c b/datapath/linux/compat/stt.c
>> index eb397e8..a1b309a 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 = *headp;
>> + int tot_len = FRAG_CB(head)->first.tot_len;
>> + int err;
>> +
>> + if (!head->next)
>> + return 0;
>> +
>> + err = pskb_expand_head(head, skb_headroom(head), tot_len,
>> GFP_ATOMIC);
>> + if (err)
>> + return err;
>
> The headroom and tailroom size seem a little big - I think they are
> supposed to be the delta amount, not the absolute value that you need.
>
>> + if (unlikely(!__pskb_pull_tail(head, head->data_len)))
>> + BUG();
>> +
>> + for (frag = head->next; frag; frag = frag->next) {
>> + skb_copy_bits(frag, 0, skb_put(head, frag->len), frag->len);
>> + }
>
> Are we missing a free somewhere of the old fragments that we copied in?
>
>> +#ifdef SKIP_ZERO_COPY
>> +static int __copy_skb(struct sk_buff *to, struct sk_buff *from,
>> + int *delta, bool *headstolen)
> [...]
>> + *headstolen = false;
>> + err = pskb_expand_head(to, skb_headroom(to),
>> + to->len + from->len, GFP_ATOMIC);
>
> I think that this allocates more memory than is necessary. I believe
> that it already takes the existing headroom into account so this would
> double it. It looks like in the tailroom we should be using
> to->data_len instead of to->len as well.
>
>> + if (unlikely(to->data_len || (from->len > skb_tailroom(to))))
>> + return -EINVAL;
>
> This is probably a little overly cautious for a fast path check given
> that we just allocated space.
>
>> @@ -1154,8 +1247,15 @@ static struct sk_buff *reassemble(struct sk_buff *skb)
> [...]
>> + if (copied_skb) {
>> + if (headstolen) {
>> + skb->len = 0;
>> + skb->data_len = 0;
>> + skb->truesize -= delta;
>> + }
>> + }
>
> Do we need to make these adjustments at all? The packet is about to be
> freed shortly.
Thanks for review. I have sent updated patch.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
