On Wed, May 16, 2018 at 03:12:28PM +0200, Sebastian Andrzej Siewior wrote:
> 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?
Well, it just doesn't make any sense in this context becuase the code
doesn't take the netif_tx_lock and netif_addr_lock is not 'meant to be
a BH disabling lock'
> > > 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.
Send a v2 with a tighter commit message please
Jason
--
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
