Re: [Xen-devel] [PATCH RFC] xen-netback: coalesce frags before copying

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, 2013-03-15 at 13:25 +0000, Ian Campbell wrote:
> > > > -       for (i = start; i < shinfo->nr_frags; i++, txp++) {
> > > > -               struct page *page;
> > > > -               pending_ring_idx_t index;
> > > > +       /* Coalesce */
> > > > +       total_frags_merged = 0;
> > > > +       for (i = j = start; i < MAX_SKB_FRAGS && j < nr_frags; i++) {
> > > 
> > > I can't see any code which causes us to drop the frag if we end up with
> > > i >= MAX_SKB_FRAGS? Is it just too subtle for me?
> > > 
> > 
> > The basic assumption that it is not possible to reach i >= MAX_SKB_FRAGS
> > since the maximum packet size supported in backend is 64K and we can
> > always accommodate a packet. Any packet larger than 64K causes fatal
> > error early on.
> 
> Does that early check also account for frontends which lie in the
> initial slot->size or is that otherwise caught later too? i.e. do we do
> the right thing when there are more bytes in the additional slots than
> the initially header said there would be? I expect we already do, I just
> want to be sure this doesn't subtly alter the conditions.
> 

We already do that in netbk_count_requests().

> > I could remove "i < MAX_SKB_FRAGS" if this causes confusion, it is just
> > a redundant check.
> 
> Is it an actual invariant? i.e. could it be BUG_ON?
> 

Yes. I've added BUG_ON(shinfo->nr_frags > MAX_SKB_FRAGS) before
returning to caller.

> > > > +                       gop->flags = GNTCOPY_source_gref;
> > > > +
> > > > +                       gop->source.u.ref = txp->gref;
> > > > +                       gop->source.domid = vif->domid;
> > > > +                       gop->source.offset = txp->offset;
> > > > +
> > > > +                       gop->dest.domid = DOMID_SELF;
> > > > +
> > > > +                       gop->dest.offset = dst_offset;
> > > > +                       gop->dest.u.gmfn = 
> > > > virt_to_mfn(page_address(page));
> > > > +
> > > > +                       if (dst_offset + txp->size > PAGE_SIZE) {
> > > > +                               /* This page can only merge a portion 
> > > > of txp */
> > > > +                               gop->len = PAGE_SIZE - dst_offset;
> > > > +                               txp->offset += gop->len;
> > > > +                               txp->size -= gop->len;
> > > > +                               dst_offset += gop->len; /* == 
> > > > PAGE_SIZE, will quit loop */
> > > 
> > > In this case we don't touch pending_cons etc? Is that because when we
> > > hit this case we always go around again and will eventually hit the else
> > > case below ensuring we do the pending_cons stuff exactly once for the
> > > tail end of the buffer? I think that is OK but it is worthy of a
> > > comment.
> > > 
> > 
> > Yes, we cannot touch pending_cons in this case, because this slot needs
> > to be coped with in next loop, using another gop.
> > 
> > > What happens if we hit SKB_FRAG_MAX right as we hit this case? Might we
> > > fail to respond to the final request which has been split in this way?
> > > 
> > 
> > As stated above, we won't hit i >= MAX_SKB_FRAGS.
> 
> OK, same question for i == MAX_SKB_FRAGS - 1 then ;-) Or whatever
> condition would be needed for us to exit here without ever getting to
> this else clause.
> 

If we exit outer for-loop before processing all tx requests for a
packet, that means this packet is larger than 64K. But at this point,
we've already know that this packet will not be larger than 64K.

After removing "i < MAX_SKB_FRAGS" check, we can only exit the for-loop
given that we finish processing all tx requests for a packet. And the
counter only decrements after we finish processing a tx request in the
'else' clause.


Wei.


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