[ofa-general] Re: [RFC] [PATCH v3] IB/ipoib: Add bonding support to IPoIB

[Michael S. Tsirkin]

> Quoting Moni Shoua <monisonlists at gmail.com>:
> Subject: Re: [ofa-general] Re: [RFC] [PATCH v3] IB/ipoib: Add bonding support to?IPoIB
> 
> Michael S. Tsirkin wrote:
> > Another question.
> > 
> >> @@ -769,32 +768,32 @@ static void ipoib_set_mcast_list(struct 
> >>  static void ipoib_neigh_destructor(struct neighbour *n)
> >>  {
> >>  	struct ipoib_neigh *neigh;
> >> -	struct ipoib_dev_priv *priv = netdev_priv(n->dev);
> >> +	struct ipoib_dev_priv *priv;
> >>  	unsigned long flags;
> >>  	struct ipoib_ah *ah = NULL;
> >>  
> >> -	ipoib_dbg(priv,
> >> -		  "neigh_destructor for %06x " IPOIB_GID_FMT "\n",
> >> -		  IPOIB_QPN(n->ha),
> >> -		  IPOIB_GID_RAW_ARG(n->ha + 4));
> >> -
> >> -	spin_lock_irqsave(&priv->lock, flags);
> >>  
> >>  	neigh = *to_ipoib_neigh(n);
> >>  	if (neigh) {
> >> +		priv = netdev_priv(neigh->dev);
> >> +		ipoib_dbg(priv,
> >> +			  "neigh_destructor for %06x " IPOIB_GID_FMT "\n",
> >> +			  IPOIB_QPN(n->ha),
> >> +			  IPOIB_GID_RAW_ARG(n->ha + 4));
> >> +
> >> +		spin_lock_irqsave(&priv->lock, flags);
> >>  		if (neigh->ah)
> >>  			ah = neigh->ah;
> >>  		list_del(&neigh->list);
> >>  		ipoib_neigh_free(n->dev, neigh);
> >> +		spin_unlock_irqrestore(&priv->lock, flags);
> >>  	}
> >> -
> >> -	spin_unlock_irqrestore(&priv->lock, flags);
> >> -
> >>  	if (ah)
> >>  		ipoib_put_ah(ah);
> >>  }
> > 
> > Using to_ipoib_neigh outside priv->lock looks problematic.
> > Can you convince me this does not introduce new races?
> > 
> > 
> I can try...
> ipoib_neigh_destructor is called from neigh_destroy() and this is when the
> kernel neighbour is under destruction itself and no one holds a reference to
> it. 

OK but we might have references to ipoib_neigh. Specifically path and mcast
group all might have it - that's what neigh_list is.

> My opinion is that if I can't assume that no one is touching ipoib_neigh when kernel 
> neighbour is being destroyed then we have a bigger problem.

That's what locks you remove seem to be there for.

-- 
MST