Re: [Xen-devel] [PATCH RFC] xen-netback: remove guest RX path dependence on MAX_SKB_FRAGS

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Jul 02, 2013 at 03:19:12PM +0100, Jan Beulich wrote:
> >>> On 02.07.13 at 15:40, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> > This dependence is undesirable and logically incorrect.
> > 
> > It's undesirable because Xen network protocol should not depend on a
> > OS-specific constant.
> 
> But this is not making the protocol dependent upon the constant;
> at least in one case the check is merely used as a worst case
> estimation (there can't possibly be an skb with more than
> MAX_SKB_FRAGS, and hence having as many slots available
> implies that at least one more skb can be processed).
> 

The "worst case" is a wrong estimation -- consider compound page frags
and RX coalescing, a frag can take up multiple ring slots.

The assumption seems to work at the moment because we haven't hit the
corner cases, we should probably not rely on that.

> >  int xen_netbk_rx_ring_full(struct xenvif *vif)
> >  {
> > -   RING_IDX peek   = vif->rx_req_cons_peek;
> > -   RING_IDX needed = max_required_rx_slots(vif);
> > +   RING_IDX peek = vif->rx_req_cons_peek;
> >  
> > -   return ((vif->rx.sring->req_prod - peek) < needed) ||
> > -          ((vif->rx.rsp_prod_pvt + XEN_NETIF_RX_RING_SIZE - peek) < 
> > needed);
> > +   return ((vif->rx.sring->req_prod < peek) ||
> > +           (vif->rx.rsp_prod_pvt + XEN_NETIF_RX_RING_SIZE < peek));
> 
> This transformation is definitely wrong: You need to retain the
> code's capability of dealing with the indexes wrapping, i.e.
> simple comparisons won't do.
> 

Hmm... so I don't quite understand this wrapping around thing and I have
a question here, what if req_prod < peek so that (req_prod - peek)
results in a very large number. In that case the ring is actually full
(cannot accommodate that packet), however "needed" is only like 20-ish
something, so the first comparision would fail. Are we relying on the
second comparion to tell the ring is full?

The only thing I need to do here is to tell whether rep_prod is ahead of
peek in the first comparison and in the second case there is enough
slots between rsp_prod_pvt and peek, what's the correct line?

> > @@ -690,20 +676,23 @@ static void xen_netbk_rx_action(struct xen_netbk 
> > *netbk)
> >     count = 0;
> >  
> >     while ((skb = skb_dequeue(&netbk->rx_queue)) != NULL) {
> > +           unsigned int ring_slots_required;
> >             vif = netdev_priv(skb->dev);
> > -           nr_frags = skb_shinfo(skb)->nr_frags;
> >  
> >             sco = (struct skb_cb_overlay *)skb->cb;
> > -           sco->meta_slots_used = netbk_gop_skb(skb, &npo);
> > -
> > -           count += nr_frags + 1;
> >  
> > -           __skb_queue_tail(&rxq, skb);
> > +           ring_slots_required = xen_netbk_count_skb_slots(vif, skb);
> >  
> > -           /* Filled the batch queue? */
> > -           /* XXX FIXME: RX path dependent on MAX_SKB_FRAGS */
> > -           if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
> > +           if (count + ring_slots_required >= XEN_NETIF_RX_RING_SIZE) {
> 
> Is this really still >= here? The old code, iiuc, used >= as being
> equivalent to count + MAX_SKB_FRAGS + 1 > XEN_NETIF_RX_RING_SIZE.
> 

Yes I should use > here.


Wei.

> Jan

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