[openib-general] Re: ipoib_mcast_send.patch

[Michael S. Tsirkin]

Quoting r. Roland Dreier <rdreier at cisco.com>:
> Subject: Re: ipoib_mcast_send.patch
> 
> So something like this should be good enough:
> 
> --- infiniband/ulp/ipoib/ipoib_multicast.c	(revision 5337)
> +++ infiniband/ulp/ipoib/ipoib_multicast.c	(working copy)
> @@ -533,8 +533,10 @@ void ipoib_mcast_join_task(void *dev_ptr
>  	}
>  
>  	if (!priv->broadcast) {
> -		priv->broadcast = ipoib_mcast_alloc(dev, 1);
> -		if (!priv->broadcast) {
> +		struct ipoib_mcast *broadcast;
> +
> +		broadcast = ipoib_mcast_alloc(dev, 1);
> +		if (!broadcast) {
>  			ipoib_warn(priv, "failed to allocate broadcast group\n");
>  			mutex_lock(&mcast_mutex);
>  			if (test_bit(IPOIB_MCAST_RUN, &priv->flags))
> @@ -544,10 +546,11 @@ void ipoib_mcast_join_task(void *dev_ptr
>  			return;
>  		}
>  
> -		memcpy(priv->broadcast->mcmember.mgid.raw, priv->dev->broadcast + 4,
> +		spin_lock_irq(&priv->lock);
> +		memcpy(broadcast->mcmember.mgid.raw, priv->dev->broadcast + 4,
>  		       sizeof (union ib_gid));
> +		priv->broadcast = broadcast;
>  
> -		spin_lock_irq(&priv->lock);
>  		__ipoib_mcast_add(dev, priv->broadcast);
>  		spin_unlock_irq(&priv->lock);
>  	}

Thats identical to what I posted till this point - right?

> @@ -701,7 +704,9 @@ void ipoib_mcast_send(struct net_device 
>  	 */
>  	spin_lock(&priv->lock);
>  
> -	if (!test_bit(IPOIB_MCAST_STARTED, &priv->flags) || !priv->broadcast) {
> +	if (!test_bit(IPOIB_MCAST_STARTED, &priv->flags)	||
> +	    !priv->broadcast					||
> +	    !test_bit(IPOIB_MCAST_FLAG_ATTACHED, &priv->broadcast->flags)) {
>  		++priv->stats.tx_dropped;
>  		dev_kfree_skb_any(skb);
>  		goto unlock;
> 

I thought its important for performance to queue packets under
mcast->pkt_queue? If not why do we do it?
Maybe we shouldnt call netif_carrier_on if we drop all packets?

-- 
Michael S. Tsirkin
Staff Engineer, Mellanox Technologies