Re: [Xen-devel] [PATCH net-next] xen-netback: allocate xenvif arrays using vzalloc.

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Oct 29, 2013 at 03:27:13PM +0000, Joby Poriyath wrote:
[...]
> +
> +static int allocate_xenvif_arrays(struct xenvif *vif)
> +{
> +     vif->mmap_pages = vif->pending_tx_info = NULL;
> +     vif->tx_copy_ops = vif->grant_copy_op = vif->meta = NULL;
> +
> +     vif->mmap_pages = vzalloc(MAX_PENDING_REQS * sizeof(struct page *));
> +     if (! vif->mmap_pages)

No space after "!".

> +             goto fail;
> +
> +     vif->pending_tx_info = vzalloc(MAX_PENDING_REQS * 
> +                                     sizeof(struct pending_tx_info));
> +     if (! vif->pending_tx_info)
> +             goto fail;
> +
> +     vif->tx_copy_ops = vzalloc(2 * MAX_PENDING_REQS *
> +                                     sizeof(struct gnttab_copy));
> +     if (! vif->tx_copy_ops)
> +             goto fail;
> +
> +     vif->grant_copy_op = vzalloc(2 * XEN_NETIF_RX_RING_SIZE *
> +                                     sizeof(struct gnttab_copy));
> +     if (! vif->grant_copy_op)
> +             goto fail;
> +
> +     vif->meta = vzalloc(2 * XEN_NETIF_RX_RING_SIZE *
> +                                     sizeof(struct xenvif_rx_meta));

Indentation.

> +     if (! vif->meta)
> +             goto fail;
> +
> +     return 0;
> +
> +fail:
> +     deallocate_xenvif_arrays(vif);
> +     return 1;

return -ENOMEM;

> +}
> +
>  struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
>                           unsigned int handle)
>  {
> @@ -313,6 +367,12 @@ struct xenvif *xenvif_alloc(struct device *parent, 
> domid_t domid,
>       vif->ip_csum = 1;
>       vif->dev = dev;
>  
> +     if (allocate_xenvif_arrays(vif)) {
> +             netdev_warn(dev, "Could not create device: out of memory\n");
> +             free_netdev(dev);
> +             return ERR_PTR(-ENOMEM);
> +     }
> +
>       vif->credit_bytes = vif->remaining_credit = ~0UL;
>       vif->credit_usec  = 0UL;
>       init_timer(&vif->credit_timeout);
> @@ -484,6 +544,7 @@ void xenvif_free(struct xenvif *vif)
>  
>       unregister_netdev(vif->dev);
>  
> +     deallocate_xenvif_arrays(vif);
>       free_netdev(vif->dev);
>  
>       module_put(THIS_MODULE);
> diff --git a/drivers/net/xen-netback/netback.c 
> b/drivers/net/xen-netback/netback.c
> index 828fdab..34c0c05 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -602,12 +602,12 @@ void xenvif_rx_action(struct xenvif *vif)
>                       break;
>       }
>  
> -     BUG_ON(npo.meta_prod > ARRAY_SIZE(vif->meta));
> +     BUG_ON(npo.meta_prod > 2*XEN_NETIF_RX_RING_SIZE);
>  
>       if (!npo.copy_prod)
>               return;
>  
> -     BUG_ON(npo.copy_prod > ARRAY_SIZE(vif->grant_copy_op));
> +     BUG_ON(npo.copy_prod > 2*XEN_NETIF_RX_RING_SIZE);

It's better to

  #define XEN_NETBK_RX_ARRAY_SIZE (2*XEN_NETIF_RX_RING_SIZE)

And use it consistently in code (allocation / comparison). Otherwise
it's easy to change one site and forget about others.

>       gnttab_batch_copy(vif->grant_copy_op, npo.copy_prod);
>  
>       while ((skb = __skb_dequeue(&rxq)) != NULL) {
> @@ -1571,7 +1571,7 @@ static unsigned xenvif_tx_build_gops(struct xenvif *vif)
>  
>               vif->tx.req_cons = idx;
>  
> -             if ((gop-vif->tx_copy_ops) >= ARRAY_SIZE(vif->tx_copy_ops))
> +             if ((gop-vif->tx_copy_ops) >= 2*MAX_PENDING_REQS)

Same here.

  #define XEN_NETBK_TX_ARRAY_SIZE

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel