[ofa-general] Re: A question about tx lock in ipoib_flush_paths

[Roland Dreier]

 > 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.