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