Re: [PATCH] IB/ipoib: replace local_irq_disable() with proper locking

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux