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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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).

>  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.

> @@ -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.

Jan


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