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

[Yossi Etigin]

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