[ofa-general] [PATCH] ipoib: null tx/rx_ring skb pointers on free

[Eli Cohen]

On Wed, Nov 05, 2008 at 05:23:07PM -0800, akepner at sgi.com wrote:

Hi Arthur,
looking a the patch I don't understand why it should fix the problem
you're seeing. I suspect we may be hiding the problem.

> 
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> index 7b14c2c..8f8650b 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> @@ -200,6 +200,7 @@ static void ipoib_cm_free_rx_ring(struct net_device *dev,
>  			ipoib_cm_dma_unmap_rx(priv, IPOIB_CM_RX_SG - 1,
>  					      rx_ring[i].mapping);
>  			dev_kfree_skb_any(rx_ring[i].skb);
> +			rx_ring[i].skb = NULL;
>  		}
>  
>  	vfree(rx_ring);

This is not needed since the ring is being freed.

> @@ -736,6 +737,7 @@ void ipoib_cm_send(struct net_device *dev, struct sk_buff *skb, struct ipoib_cm_
>  	if (unlikely(ib_dma_mapping_error(priv->ca, addr))) {
>  		++dev->stats.tx_errors;
>  		dev_kfree_skb_any(skb);
> +		tx_req->skb = NULL;
>  		return;
>  	}

Here we will never get completion so why do we need this?

>  
> @@ -747,6 +749,7 @@ void ipoib_cm_send(struct net_device *dev, struct sk_buff *skb, struct ipoib_cm_
>  		++dev->stats.tx_errors;
>  		ib_dma_unmap_single(priv->ca, addr, skb->len, DMA_TO_DEVICE);
>  		dev_kfree_skb_any(skb);
> +		tx_req->skb = NULL;

Also here, we don't get a completion.

>  	} else {
>  		dev->trans_start = jiffies;
>  		++tx->tx_head;
> @@ -785,6 +788,7 @@ void ipoib_cm_handle_tx_wc(struct net_device *dev, struct ib_wc *wc)
>  	dev->stats.tx_bytes += tx_req->skb->len;
>  
>  	dev_kfree_skb_any(tx_req->skb);
> +	tx_req->skb = NULL;
And here we already got the completion so we shouldn't exptect another
free of the SKB.
>  
>  	netif_tx_lock(dev);
>  
> @@ -1179,6 +1183,7 @@ timeout:
>  		ib_dma_unmap_single(priv->ca, tx_req->mapping, tx_req->skb->len,
>  				    DMA_TO_DEVICE);
>  		dev_kfree_skb_any(tx_req->skb);
> +		tx_req->skb = NULL;
and here we're freeing the ring
>  		++p->tx_tail;
>  		netif_tx_lock_bh(p->dev);
>  		if (unlikely(--priv->tx_outstanding == ipoib_sendq_size >> 1) &&
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> index 28eb6f0..f7e3497 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> @@ -383,6 +383,7 @@ static void ipoib_ib_handle_tx_wc(struct net_device *dev, struct ib_wc *wc)
>  	dev->stats.tx_bytes += tx_req->skb->len;
>  
>  	dev_kfree_skb_any(tx_req->skb);
> +	tx_req->skb = NULL;
>  
>  	++priv->tx_tail;
>  	if (unlikely(--priv->tx_outstanding == ipoib_sendq_size >> 1) &&
> @@ -572,6 +573,7 @@ void ipoib_send(struct net_device *dev, struct sk_buff *skb,
>  	if (unlikely(ipoib_dma_map_tx(priv->ca, tx_req))) {
>  		++dev->stats.tx_errors;
>  		dev_kfree_skb_any(skb);
> +		tx_req->skb = NULL;
>  		return;
>  	}
>  
> @@ -594,6 +596,7 @@ void ipoib_send(struct net_device *dev, struct sk_buff *skb,
>  		--priv->tx_outstanding;
>  		ipoib_dma_unmap_tx(priv->ca, tx_req);
>  		dev_kfree_skb_any(skb);
> +		tx_req->skb = NULL;
>  		if (netif_queue_stopped(dev))
>  			netif_wake_queue(dev);
>  	} else {
> @@ -833,6 +836,7 @@ int ipoib_ib_dev_stop(struct net_device *dev, int flush)
>  							(ipoib_sendq_size - 1)];
>  				ipoib_dma_unmap_tx(priv->ca, tx_req);
>  				dev_kfree_skb_any(tx_req->skb);
> +				tx_req->skb = NULL;
>  				++priv->tx_tail;
>  				--priv->tx_outstanding;
>  			}
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
> 
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general