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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
