WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [3/11] [NET] front: Stop using rx->id

To: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Subject: [Xen-devel] Re: [3/11] [NET] front: Stop using rx->id
From: Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>
Date: Thu, 27 Jul 2006 13:49:51 +0100
Cc: Xen Development Mailing List <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Thu, 27 Jul 2006 05:50:13 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <20060707141814.GD12031@xxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <20060707141634.GA12031@xxxxxxxxxxxxxxxxxxx> <20060707141814.GD12031@xxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx

Some comments (inline) on this patch. I checked in the trivial first two patches.

 Thanks,
 Keir

On 7 Jul 2006, at 15:18, Herbert Xu wrote:

        /* Step 2: Rebuild the RX buffer freelist and the RX ring itself. */
-       for (requeue_idx = 0, i = 1; i <= NET_RX_RING_SIZE; i++) {
-               if ((unsigned long)np->rx_skbs[i] < PAGE_OFFSET)
-                       continue;
+       for (i = 0; i < NET_RX_RING_SIZE; i++) {
+               if (!np->rx_skbs[i])
+                       break;
                gnttab_grant_foreign_transfer_ref(
                        np->grant_rx_ref[i], np->xbdev->otherend_id,
                        __pa(np->rx_skbs[i]->data) >> PAGE_SHIFT);
-               RING_GET_REQUEST(&np->rx, requeue_idx)->gref =
-                       np->grant_rx_ref[i];
-               RING_GET_REQUEST(&np->rx, requeue_idx)->id = i;
+               RING_GET_REQUEST(&np->rx, i)->gref = np->grant_rx_ref[i];
+               RING_GET_REQUEST(&np->rx, i)->id = i;
+       }
+       for (requeue_idx = i++; i < NET_RX_RING_SIZE; i++) {
+               if (!np->rx_skbs[i])
+                       continue;
+               skb = np->rx_skbs[requeue_idx] = xennet_get_rx_skb(np, i);
+               ref = np->grant_rx_ref[requeue_idx] = xennet_get_rx_ref(np, i);
+               gnttab_grant_foreign_transfer_ref(
+                       ref, np->xbdev->otherend_id,
+                       __pa(skb->data) >> PAGE_SHIFT);
+               RING_GET_REQUEST(&np->rx, requeue_idx)->gref = ref;
+               RING_GET_REQUEST(&np->rx, requeue_idx)->id = requeue_idx;
                requeue_idx++;
        }

Why two loops to fill the receive ring? The header of the second loop with the body of the first loop would seem more correct (use of destructive xennet_get_* functions to read skb/ref info looks incorrect). Have you tested this patch with 'xm save/restore' on a domU receiving a bulk network transfer? That would be very worthwhile.

@@ -1391,11 +1433,6 @@ static struct net_device * __devinit cre
                np->grant_tx_ref[i] = GRANT_INVALID_REF;
        }

-       for (i = 0; i <= NET_RX_RING_SIZE; i++) {
-               np->rx_skbs[i] = (void *)((unsigned long) i+1);
-               np->grant_rx_ref[i] = GRANT_INVALID_REF;
-       }
-
        /* A grant for every tx ring slot */
        if (gnttab_alloc_grant_references(TX_MAX_TARGET,
                                          &np->gref_tx_head) < 0) {

Initialisation still would be nice for sanity. We should initialise rx_skbs[i] to NULL, but apart from that I think we should keep this initialisation loop as-is. Oh, except < NET_RX_RING_SIZE rather than <=.

 -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel