On 2018-05-15 17:00:25 [-0600], Jason Gunthorpe wrote:
> 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..
I referred to what happened during those two commits (back then, ages
ago). Are you sure this still needs rewording?
> > 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.
I meant the part where netif_addr_lock() is no longer acquired with
disabled interrupts. Now that I read it myself again I understand the
confusion. I will try to reword it.
> > 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..
good.
Sebastian
--
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
