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: Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>
Subject: [Xen-devel] Re: [3/11] [NET] front: Stop using rx->id
From: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Date: Thu, 27 Jul 2006 23:12:34 +1000
Cc: Xen Development Mailing List <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Thu, 27 Jul 2006 06:13:26 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <1c6ca61a39886933a523840193ff31a8@xxxxxxxxxxxx>
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> <1c6ca61a39886933a523840193ff31a8@xxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.9i
On Thu, Jul 27, 2006 at 01:49:51PM +0100, Keir Fraser wrote:
> 
> 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 

The first loop deals with the initial segment of np where rx_skbs isn't
NULL.  The reason it's dealt with separately from the rest is because
unlike the second loop we don't relocate the entry in np->rx_skbs and
np->grant_rx_ref.

Had we used the body of the second loop for the initial segment we would
end up wiping out the values of rx_skbs and grant_rx_ref when we do the
assignments

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

because for the initial segment requeue_idx is always the same as i.

> destructive xennet_get_* functions to read skb/ref info looks 
> incorrect).

We're relocating entries from the back of rx_skbs/grant_rx_ref to the
front.  After relocating it we need to clear the original position as
otherwise we may use the entry incorrectly if the backend gives us a
bogus response.  By clearing it we ensure that it is caught by the warning
"Bad rx response id %d.\n".  This is also needed for the sanity checks
on rx_skbs before we store a new skb into it.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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