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

[Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>]

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