[ofa-general] Re: [PATCH] Updated - Use vmalloc to alloc the rx_ring

[David J. Wilder]

On Fri, 2008-08-08 at 15:56 -0700, Roland Dreier wrote:
> So I applied this patch (slightly munged as below), but a few questions
> and comments:
> 
>  > To prevent allocation failures for the rx_ring when using 
>  > non-srq and large recv_queue_size (1K or larger) use 
>  > vmalloc instead of kcalloc to alocate the rx_ring.
> 
> Do you really have people running connected mode on non-SRQ-capable HCAs
> with receive queue lengths >= 1K?  It seems that each connection is
> going to consume ~64MB of memory in such a case, so with even a small
> number of remote peers, we get into GBs of memory tied up in receive
> rings.  Is this really the best way to get performance?

Yep, big systems lots of memory, these monsters can be configured with
up to 128GB.  The 1K size is just where we happen to see the problem,
the allocation can fail on smaller kmallocs if system memory is small
and/or fragmented.

> 
> Also you specifically mention non-SRQ in the changelog but then change
> the SRQ ring allocation too.  I think that change makes sense but I
> wonder why it is there.

I change both because the free (now vfree) in ipoib_cm_free_rx_ring() is
used to free both types of rings.  

> 
>  > +		printk(KERN_WARNING "ipoib_cm:Allocation of rx_ring failed, %s",
>  > +			"try using a lower value of recv_queue_size.\n");
> 
> the %s continuation line idiom is interesting... I think it is more
> idiomatic just to let the compiler concatenate strings, and it generates
> better code too (it saves having to set up a second string constant and
> pass the address of that to printk... plus it saves the (trivial)
> runtime cost of handling the %s format).

I thought I already changed that, thanks for fixing it.
> 
> 
> commit b1404069f64457c94de241738fdca142c2e5698f
> Author: David J. Wilder <dwilder at us.ibm.com>
> Date:   Fri Aug 8 15:51:29 2008 -0700
> 
>     IPoIB/cm: Use vmalloc() to allocate rx_rings
>     
>     There are users that are running UDP applications that require a large
>     receive queue size in order to get good performance.  To prevent
>     allocation failures for rx_rings when using non-SRQ mode and large
>     recv_queue_size (1K or larger), use vmalloc() instead of kcalloc() to
>     alocate rx_rings.
>     
>     Signed-off-by: David Wilder <dwilder at us.ibm.com>
>     Signed-off-by: Roland Dreier <rolandd at cisco.com>
> 
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> index 7ebc400..341ffed 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> @@ -202,7 +202,7 @@ static void ipoib_cm_free_rx_ring(struct net_device *dev,
>  			dev_kfree_skb_any(rx_ring[i].skb);
>  		}
> 
> -	kfree(rx_ring);
> +	vfree(rx_ring);
>  }
> 
>  static void ipoib_cm_start_rx_drain(struct ipoib_dev_priv *priv)
> @@ -352,9 +352,14 @@ static int ipoib_cm_nonsrq_init_rx(struct net_device *dev, struct ib_cm_id *cm_i
>  	int ret;
>  	int i;
> 
> -	rx->rx_ring = kcalloc(ipoib_recvq_size, sizeof *rx->rx_ring, GFP_KERNEL);
> -	if (!rx->rx_ring)
> +	rx->rx_ring = vmalloc(ipoib_recvq_size * sizeof *rx->rx_ring);
> +	if (!rx->rx_ring) {
> +		printk(KERN_WARNING "%s: failed to allocate CM non-SRQ ring (%d entries)\n",
> +		       priv->ca->name, ipoib_recvq_size);
>  		return -ENOMEM;
> +	}
> +
> +	memset(rx->rx_ring, 0, ipoib_recvq_size * sizeof *rx->rx_ring);
> 
>  	t = kmalloc(sizeof *t, GFP_KERNEL);
>  	if (!t) {
> @@ -1494,14 +1499,16 @@ static void ipoib_cm_create_srq(struct net_device *dev, int max_sge)
>  		return;
>  	}
> 
> -	priv->cm.srq_ring = kzalloc(ipoib_recvq_size * sizeof *priv->cm.srq_ring,
> -				    GFP_KERNEL);
> +	priv->cm.srq_ring = vmalloc(ipoib_recvq_size * sizeof *priv->cm.srq_ring);
>  	if (!priv->cm.srq_ring) {
>  		printk(KERN_WARNING "%s: failed to allocate CM SRQ ring (%d entries)\n",
>  		       priv->ca->name, ipoib_recvq_size);
>  		ib_destroy_srq(priv->cm.srq);
>  		priv->cm.srq = NULL;
> +		return;
>  	}
> +
> +	memset(priv->cm.srq_ring, 0, ipoib_recvq_size * sizeof *priv->cm.srq_ring);
>  }
> 
>  int ipoib_cm_dev_init(struct net_device *dev)