Re: [Xen-devel] [PATCH] Mini-OS: netfront: Fix rx ring starvation in network_rx

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

Martin Lucina, le Thu 11 Dec 2014 13:10:00 +0100, a écrit :
> I've done this in the updated patch, take a look and let me know what you
> think.

I like that :)

> From 3880f59159bf0a05b47b6e723091b1e7e4fb6814 Mon Sep 17 00:00:00 2001
> From: Martin Lucina <martin@xxxxxxxxxx>
> Date: Thu, 4 Dec 2014 14:33:53 +0100
> Subject: [PATCH] Mini-OS: netfront: Fix rx ring starvation in network_rx
> 
> In network_rx() we must push the same amount of requests back onto the
> ring in the second loop that we consumed in the first loop. Otherwise
> the ring will eventually starve itself of free request slots and no
> packets will be delivered.
> 
> Further, we make the HAVE_LIBC codepath clearer to follow by removing
> the "some" variable from the for loop initialisation and conditions.
> 
> Signed-off-by: Martin Lucina <martin@xxxxxxxxxx>

Acked-by: Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>

> ---
>  extras/mini-os/netfront.c |   36 +++++++++++++++++-------------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/extras/mini-os/netfront.c b/extras/mini-os/netfront.c
> index 44c3995..42bb103 100644
> --- a/extras/mini-os/netfront.c
> +++ b/extras/mini-os/netfront.c
> @@ -96,42 +96,35 @@ static inline int xennet_rxidx(RING_IDX idx)
>  void network_rx(struct netfront_dev *dev)
>  {
>      RING_IDX rp,cons,req_prod;
> -    struct netif_rx_response *rx;
> -    int nr_consumed, some, more, i, notify;
> -
> +    int nr_consumed, more, i, notify;
> +#ifdef HAVE_LIBC
> +    int some;
> +#endif
>  
> +    nr_consumed = 0;
>  moretodo:
>      rp = dev->rx.sring->rsp_prod;
>      rmb(); /* Ensure we see queued responses up to 'rp'. */
> -    cons = dev->rx.rsp_cons;
>  
> -    for (nr_consumed = 0, some = 0;
> -         (cons != rp) && !some;
> -         nr_consumed++, cons++)
> +#ifdef HAVE_LIBC
> +    some = 0;
> +#endif
> +    for (cons = dev->rx.rsp_cons; cons != rp; nr_consumed++, cons++)
>      {
>          struct net_buffer* buf;
>          unsigned char* page;
>          int id;
>  
> -        rx = RING_GET_RESPONSE(&dev->rx, cons);
> -
> -        if (rx->flags & NETRXF_extra_info)
> -        {
> -            printk("+++++++++++++++++++++ we have extras!\n");
> -            continue;
> -        }
> -
> -
> -        if (rx->status == NETIF_RSP_NULL) continue;
> +        struct netif_rx_response *rx = RING_GET_RESPONSE(&dev->rx, cons);
>  
>          id = rx->id;
> -        BUG_ON(id >= NET_TX_RING_SIZE);
> +        BUG_ON(id >= NET_RX_RING_SIZE);
>  
>          buf = &dev->rx_buffers[id];
>          page = (unsigned char*)buf->page;
>          gnttab_end_access(buf->gref);
>  
> -        if(rx->status>0)
> +        if (rx->status > NETIF_RSP_NULL)
>          {
>  #ifdef HAVE_LIBC
>           if (dev->netif_rx == NETIF_SELECT_RX) {
> @@ -142,6 +135,7 @@ moretodo:
>               memcpy(dev->data, page+rx->offset, len);
>               dev->rlen = len;
>               some = 1;
> +                break;
>           } else
>  #endif
>               dev->netif_rx(page+rx->offset,rx->status);
> @@ -150,7 +144,11 @@ moretodo:
>      dev->rx.rsp_cons=cons;
>  
>      RING_FINAL_CHECK_FOR_RESPONSES(&dev->rx,more);
> +#ifdef HAVE_LIBC
>      if(more && !some) goto moretodo;
> +#else
> +    if(more) goto moretodo;
> +#endif
>  
>      req_prod = dev->rx.req_prod_pvt;
>  
> -- 
> 1.7.10.4
> 

-- 
Samuel
<T> csp.tar.gz:     ascii text
 -+- #ens-mim - vive les browsers qui prennent des initiatives à la con -+-

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