On Fri, May 04, 2018 at 04:45:54PM +0200, Sebastian Andrzej Siewior wrote:
> Commit 78bfe0b5b67f ("IPoIB: Take dev->xmit_lock around mc_list accesses")
> introduced xmit_lock lock in ipoib_mcast_restart_task() and commit
> 932ff279a43a ("[NET]: Add netif_tx_lock") preserved the locking order while
> dev->xmit_lock has been replaced with a helper. The netif_tx_lock should
> not be acquired with disabled interrupts because it is meant to be a BH
> disabling lock.
This commenting is talking about the tx_lock, which was true long ago,
but these days we are taking the netif_addr_lock, which is
different.. So at least the last sentence needs to be reworded..
> The priv->lock is always acquired with interrupts disabled. The only
> place where netif_addr_lock() and priv->lock nest ist
> ipoib_mcast_restart_task(). By reversing the lock order and taking
> netif_addr lock with bottom halfs disabled it is possible to get rid of
> the local_irq_save() completely.
I'm also having trouble following this, where did the locking odering
get reversed in this patch? I see the ordering is the same, but the
irq disable has moved.
> This requires to take priv->lock with spin_lock_irq() inside the netif_addr
> locked section. It's safe to do so because the caller is either a worker
> function or __ipoib_ib_dev_flush() which are both calling with interrupts
> enabled.
Otherwise the patch seems fine to me, I also can think of no reason
why we need to have the IRQ disabled prior to getting the addr_lock,
it looks like a holdover from 10 years ago, maybe it made sense when
this was the xmit lock..
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
