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

[Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>]

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