Re: [Xen-devel] Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-03-12 at 20:13 +0000, Wei Liu wrote:
> On Tue, 2013-03-12 at 15:25 +0000, Wei Liu wrote:
> > On Tue, 2013-03-12 at 15:07 +0000, ANNIE LI wrote:
> > > 
> > > On 2013-3-12 20:18, Wei Liu wrote:
> > > > On Tue, 2013-03-12 at 11:40 +0000, Ian Campbell wrote:
> > > >> On Sun, 2013-03-10 at 19:18 +0000, Wei Liu wrote:
> > > >>> On Sat, Mar 09, 2013 at 02:57:16AM +0000, Ian Campbell wrote:
> > > >>>>>> - change MAX_SKB_FRAGS to 19 to accommodate all guests
> > > >>>> Changing MAX_SKB_FRAGS is *not* an option upstream. This might be a
> > > >>>> useful local hack but we need to drop the idea as a long term fix.
> > > >>>>
> > > >>>>> Ugh. The negotiations between host and guest is probably the best
> > > >>>>> choice. The issues you are going to hit are that you might need
> > > >>>>> to redo the skbs to match what the frontend's max is.
> > > >>>> IMHO the right fix is for netback to coalesce as it copies from the
> > > >>>> frontend if it needs to do so, it is copying anyway so it should be
> > > >>>> cheap enough. I thought we had discussed this and someone was 
> > > >>>> working on
> > > >>>> implementing it. If not Annie then perhaps it was Matt or Siva (both 
> > > >>>> now
> > > >>>> CC'd)
> > > >>>>
> > > >>> As a short term fix, can we use skb_linearize() if skb->nr_frags>=
> > > >>> MAX_SKB_FRAGS?
> > > >> No, because that would require changing the frontend, while this fix
> > > >> needs to be in the backend if older guests are to continue working.
> > > >>
> > > >> You can't use skb_linearize in netback as is because you would first
> > > >> need to be able to build the skb with nr_frags>= MAX_SKB_FRAGS in order
> > > >> to pass it to that function.
> > > >>
> > > > Yes, the idea is to define NETBK_MAX_SKB_FRAGS to some bigger number
> > > > (say 20) to accommodate the possible maximum number of frags in
> > > > frontend. The thing that truly matters it the skb->len, which should be
> > > > <64K, nr_frags is not important.
> > > 
> > > I doubt this would work since you can not build out skb with nr_frags >= 
> > > MAX_SKB_FRAGS. See following code in skbuff.h,
> > > 
> > > #if (65536/PAGE_SIZE + 1) < 16
> > > #define MAX_SKB_FRAGS 16UL
> > > #else
> > > #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1)
> > > #endif
> > > 
> > > and every skb contains MAX_SKB_FRAGS frags,
> > > 
> > > struct skb_shared_info {
> > > ....
> > > skb_frag_t      frags[MAX_SKB_FRAGS];
> > > ...
> > > }
> > > 
> > 
> > So I haven't gone this far to trigger this. :-(
> > 
> > I just double checked with printk, nr_frags has not exceeded backend
> > MAX_SBK_FRAGS even if I have frontend MAX_SKB_FRAGS = 25.
> > 
> > > Coalescing frags before building skb could avoid this issue.
> > > 
> > 
> > "before building skb" is a bit vague to me. Is it netbk_count_request?
> > Is it xen_netbk_fill_frags? If it is the latter, we are already too late
> > to fix this. If it is the former, we don't even start copying.
> > 
> > 
> 
> Actually the copy is done with Hypervisor, I don't know how it is
> possible to do coalesce while copying.

This can be done trivially by making the appropriate gnttab_copy
hypercalls to copy from the guest memory into dom0 buffers, coalescing
as you go. IOW you split each ring slot request into multiple (batched)
copies in order to fill buffers etc.

> FWIW I came up with an idea. Netback maintains a ring of skb_frag_t
> groups. Each group has NETBK_SKB_MAX_FRAGS frags. So overall the size of
> this ring is MAX_PENGING_REQS * NETBK_SKB_MAX_FRAGS *
> sizeof(skb_frag_t). If NETBK_SKB_MAX_FRAGS == 20, skb_frag_t size == 16,
> this ring is 80K large.
> 
> If we detect frontend's frags > backend's SKB_MAX_FRAGS, we switch to
> use the new ring, and set skb->nr_frags to some specific number (say -1)
> and has frag[0].page.p set to ring index. Then we copy data as usual.
> Later in before xen_netbk_fill_frags if we detect this skb is should be
> constructed via previous skb frag ring, we accommodate those frags to
> that skb.
> 
> if (skb should be constructed via frag ring)
>     construct_skb()
> else /* normal path
>     xen_netbk_fill_frags
> 
> This idea is essentially adding a slow path though, because
> construct_skb() potentially needs to copy / move data around.

This is insanely complex for the problem at hand IMHO.

Ian.


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