Roland Dreier wrote: > > In ipoib_flush_paths(), we take the netif_tx_lock to remove a path > > My question is - what data does this lock protect? > > It isn't path->list and path->rb_node, because priv->lock is enough to > protect them. > > > > It might be neigh and neigh->ah, to avoid freeing the neighbour and its > address > > handle while ipoib_start_xmit() is using it, but this particular part is > done *outside* > > the tx lock. > > You probably already saw this from the history of the file, but this > locking was added in 9217b27b: > > IB/ipoib: Fix flush/start xmit race (from code review) > > Prevent flush task from freeing the ipoib_neigh pointer, while > ipoib_start_xmit() is accessing the ipoib_neigh through the pointer it > has loaded from the skb's hardware address. > > > Unless I'm missing something - shouldn't the code: > > > > spin_unlock_irqrestore(&priv->lock, flags); > > netif_tx_unlock_bh(dev); > > wait_for_completion(&path->done); > > release >> path_free(dev, path); > > lock >> netif_tx_lock_bh(dev); > > spin_lock_irqsave(&priv->lock, flags); > > > > Be like this: > > > > spin_unlock_irqrestore(&priv->lock, flags); > > netif_tx_unlock_bh(dev); > > wait_for_completion(&path->done); > > lock >> netif_tx_lock_bh(dev); > > release >> path_free(dev, path); > > spin_lock_irqsave(&priv->lock, flags); > > I don't think it matters -- the path has been removed from every > externally visible list/rbtree already so there's no way anyone could > grab it after we drop the tx lock. > > - R.
Couldn't ipoib_start_xmit() grab ipoib_neigh, and cause the same thing the commit above was intended to fix? I saw in the mails that dropping the lock is the v2 of the patch, the original one did not drop it. --Yossi _______________________________________________ general mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
