Re: [Xen-devel] [PATCH v2 1/1] xen/netback: correctly calculate required slots of skb.

[annie li <annie.li@xxxxxxxxxx>]

On 2013-7-11 16:11, Ian Campbell wrote:
> On Wed, 2013-07-10 at 17:15 +0800, Annie Li wrote:
> > +static int netbk_count_slots(struct xenvif *vif, struct sk_buff *skb,
> > +                           int *copy_off, unsigned long size,
> > +                           unsigned long offset, int *head)
> >   {
> > -       unsigned int count;
> > -       int i, copy_off;
> > +       unsigned long bytes;
> > +       int count = 0;
> >   
> > -	count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
> > +       offset &= ~PAGE_MASK;
> >   
> > -	copy_off = skb_headlen(skb) % PAGE_SIZE;
> > +       while (size > 0) {
> > +               BUG_ON(offset >= PAGE_SIZE);
> > +               BUG_ON(*copy_off > MAX_BUFFER_OFFSET);
> >   
> > -	if (skb_shinfo(skb)->gso_size)
> > -               count++;
> > +               bytes = PAGE_SIZE - offset;
> >   
> > -	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
> > -               unsigned long size = skb_frag_size(&skb_shinfo(skb)->frags[i]);
> > -               unsigned long offset = skb_shinfo(skb)->frags[i].page_offset;
> > -               unsigned long bytes;
> > +               if (bytes > size)
> > +                       bytes = size;
> >   
> > -		offset &= ~PAGE_MASK;
> > +               if (start_new_rx_buffer(*copy_off, bytes, *head)) {
> > +                       count++;
> > +                       *copy_off = 0;
> > +               }
> >   
> > -		while (size > 0) {
> > -                       BUG_ON(offset >= PAGE_SIZE);
> > -                       BUG_ON(copy_off > MAX_BUFFER_OFFSET);
> > +               if (*copy_off + bytes > MAX_BUFFER_OFFSET)
> > +                       bytes = MAX_BUFFER_OFFSET - *copy_off;
> >   
> > -			bytes = PAGE_SIZE - offset;
> > +               *copy_off += bytes;
> >   
> > -			if (bytes > size)
> > -                               bytes = size;
> > +               offset += bytes;
> > +               size -= bytes;
> >   
> > -			if (start_new_rx_buffer(copy_off, bytes, 0)) {
> > -                               count++;
> > -                               copy_off = 0;
> > -                       }
> > +               /* Next frame */
> > +               if (offset == PAGE_SIZE && size)
> > +                       offset = 0;
> > +
> > +               if (*head)
> > +                       count++;
> This little bit corresponds to the "/* Leave a gap for the GSO
> descriptor. */" in gop_frag_copy?
No, it does not correspond to this in gop_frag_copy. The code here only 
increase count for the first time. I thought to initialize the count in 
xen_netbk_count_skb_slots with 1 to avoid this. But thinking of the 
extreme case when the header size is zero(not sure whether this case 
could be true), I increase the count here to keep safe in case header 
size is zero.
There is code correspond to that in gop_frag_copy in 
xen_netbk_count_skb_slots, see following,
+       if (skb_shinfo(skb)->gso_size && !vif->gso_prefix)
+               count++;
(The original code does not have gso_prefix, I added it in this patch too based 
on Wei's suggestion)


> If so then it would be useful to duplicate the comment, but more
> importantly the condition on gop_frag_copy is:
>          (head && skb_shinfo(skb)->gso_size && !vif->gso_prefix)
> why the difference?
Actually it is similar with that in xen_netbk_count_skb_slots, see above 
comments.
Thanks
Annie

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel