[ofa-general] Re: [PATCH] connected mode leaks pages on receive & reuses old skb

[Ralph Campbell]

On Sun, 2007-08-26 at 08:03 +0300, Michael S. Tsirkin wrote:
> The following claim seems wrong, too:
> 
> > NOTE that [frags..IPOIB_CM_RX_SG-1] are *not* copied to newskb and
> > thus leaked. Also, newskb's nr_frags is set based on the message size
> > rather than the full size.
> 
> Not really. nr_frags *starts* at the message size.
> skb_fill_page_desc will fill in the fargment in toskb, and
> increment nr_frags until it matches the full size.
> 
> > @@ -378,23 +382,26 @@ static void skb_put_frags(struct sk_buff *skb, unsigned int hdr_space,
> >  	skb->len += size;
> >  	length -= size;
> >  
> > -	num_frags = skb_shinfo(skb)->nr_frags;
> > +	num_frags = skb_shinfo(toskb)->nr_frags;
> >  	for (i = 0; i < num_frags; i++) {
> >  		skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
> >  
> > -		if (length == 0) {
> > -			/* don't need this page */
> > -			skb_fill_page_desc(toskb, i, frag->page, 0, PAGE_SIZE);
> > -			--skb_shinfo(skb)->nr_frags;
> > -		} else {
> > -			size = min(length, (unsigned) PAGE_SIZE);
> > -
> > -			frag->size = size;
> > -			skb->data_len += size;
> > -			skb->truesize += size;
> > -			skb->len += size;
> > -			length -= size;
> > -		}
> > +		size = min(length, (unsigned) PAGE_SIZE);
> > +		WARN_ON(size == 0);
> > +		frag->size = size;
> > +		skb->data_len += size;
> > +		skb->truesize += size;
> > +		skb->len += size;
> > +		length -= size;
> > +	}
> > +	WARN_ON(length != 0);
> > +	skb_shinfo(skb)->nr_frags = num_frags;
> > +
> > +	/* move unused pages from skb to toskb */
> > +	for (; i < IPOIB_CM_RX_SG - 1; i++) {
> > +		skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
> > +
> > +		skb_fill_page_desc(toskb, i, frag->page, 0, PAGE_SIZE);
> >  	}
> >  }
> >  
> 
> I don't see how this chunk changes anything.
> It seems you have basically replaced a single loop with
> a condition by two loops.

Looking at the orginial code again (after some more sleep :-) ),
I agree that new and old are equivalent. I just found the
original a bit confusing.