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

[Martin Lucina <martin@xxxxxxxxxx>]

Hi Samuel,

Comments and updated patch below.

samuel.thibault@xxxxxxxxxxxx said:
> > -        if (rx->flags & NETRXF_extra_info)
> > -        {
> > -            printk("+++++++++++++++++++++ we have extras!\n");
> > -            continue;
> > -        }
> > -
> > -
> > -        if (rx->status == NETIF_RSP_NULL) continue;
> 
> I believe we want to keep them.  Or turn them into
> if (rx->status != NETIF_RSP_OKAY)
>   continue;

If we keep them then the following code calling gnttab_end_access() will
not get called and the second loops calls to gnttab_grant_access() will
not match up.

Rather, shouldn't the final if() in the loop (where data is sent up) be:

  if(rx->status > NETIF_RSP_NULL) { ... }

*we can't use NETIF_RSP_OKAY as NETIF_RSP_NULL is defined to be 1 (yuck).

I don't see any value in keeping the printk(), the netfront driver already
disclaims supporting extras in comments.

> Yes, and I introduced the "some" variable especially for this. The
> idea was to implement a basic support for opening the device with a
> tap behavior.  But we didn't want to introduce dynamic buffers to
> store incoming packets.  In that case we don't run network_rx from the
> interrupt, but netfront_select_handler instead, which will wake the
> select() call, the application then calls netfront_receive(), which eats
> exactly one packet only from the ring.  Thus the "some" variable in
> order to break the loop when exactly one packet was eaten (and no, we
> can't use break or set cons to rp, since that'd would eat 0 or more than
> 1 packets).

Ah, now I understand. The presence of the "some" variable was making it
really hard to reason about the logic in that loop :-/

Assuming you don't want to duplicate some code and write a separate
receive() function for the HAVE_LIBC codepath entirely then a minimal
change which clears things up for future readers is to:

- make "some" explicit using #ifdef HAVE_LIBC 
- take it out of the for() loop initialisation and condition
- set some = 1 and use break to exit the loop

I've done this in the updated patch, take a look and let me know what you
think.

Martin

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


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