| Message ID | 4AB20C6C.9090005@Voltaire.COM (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
> + if (ib_query_port(priv->ca, priv->port, &attr) || > + attr.state != IB_PORT_ACTIVE) { > + ipoib_dbg(priv, "wait with carrier until IB port is active\n"); > + if (test_bit(IPOIB_FLAG_OPER_UP, &priv->flags)) > + queue_delayed_work(ipoib_workqueue, &priv->carrier_on_task, HZ); > + return; > + } This queueing delayed work to poll the port state seems a bit odd to me... we get an event when the port changes state anyway, right? So can't we just turn the carrier on when we get an active event? - R. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
And by the way, this current patch has a deadlock I think: > @@ -724,6 +724,8 @@ int ipoib_ib_dev_down(struct net_device *dev, int flush) > ipoib_dbg(priv, "downing ib_dev\n"); > > clear_bit(IPOIB_FLAG_OPER_UP, &priv->flags); > + cancel_delayed_work(&priv->carrier_on_task); ipoib_ib_dev_down() is called with rtnl held but carrier_on_task() does rtn_lock(). So if carrier_on_task() is running but about to take the rtnl when we try to do cancel_delayed_work() here, then it will wait forever. I think using lockdep on a new enough kernel (2.6.30 or maybe 2.6.31) will report workqueue / timer vs. lock deadlocks. - R. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Roland Dreier wrote: > > + if (ib_query_port(priv->ca, priv->port, &attr) || > > + attr.state != IB_PORT_ACTIVE) { > > + ipoib_dbg(priv, "wait with carrier until IB port is active\n"); > > + if (test_bit(IPOIB_FLAG_OPER_UP, &priv->flags)) > > + queue_delayed_work(ipoib_workqueue, &priv->carrier_on_task, HZ); > > + return; > > + } > > This queueing delayed work to poll the port state seems a bit odd to > me... we get an event when the port changes state anyway, right? So > can't we just turn the carrier on when we get an active event? > > - R. You're right. I've complicated things where I shouldn't need. The call to __ipoib_ib_dev_flush() from ipoib_event() will requeue the carrier_on_task in join completion of the broadcast group. I'll resend Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Roland Dreier wrote: > And by the way, this current patch has a deadlock I think: > > > @@ -724,6 +724,8 @@ int ipoib_ib_dev_down(struct net_device *dev, int flush) > > ipoib_dbg(priv, "downing ib_dev\n"); > > > > clear_bit(IPOIB_FLAG_OPER_UP, &priv->flags); > > + cancel_delayed_work(&priv->carrier_on_task); > > ipoib_ib_dev_down() is called with rtnl held but carrier_on_task() does > rtn_lock(). So if carrier_on_task() is running but about to take the > rtnl when we try to do cancel_delayed_work() here, then it will wait > forever. > > I think using lockdep on a new enough kernel (2.6.30 or maybe 2.6.31) > will report workqueue / timer vs. lock deadlocks. > > - R. I may miss this but I don't see how ipoib_ib_dev_down() is called with rtnl held. Anyway, the new patch doesn't use delayed work. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I may miss this but I don't see how ipoib_ib_dev_down() is called with rtnl held. It's called from ipoib_stop(), and .ndo_stop is called with rtnl held. > Anyway, the new patch doesn't use delayed work. great -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h index 753a983..f29ce14 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib.h +++ b/drivers/infiniband/ulp/ipoib/ipoib.h @@ -292,7 +292,7 @@ struct ipoib_dev_priv { struct delayed_work pkey_poll_task; struct delayed_work mcast_task; - struct work_struct carrier_on_task; + struct delayed_work carrier_on_task; struct work_struct flush_light; struct work_struct flush_normal; struct work_struct flush_heavy; diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c b/drivers/infiniband/ulp/ipoib/ipoib_ib.c index e35f4a0..c452089 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c @@ -724,6 +724,8 @@ int ipoib_ib_dev_down(struct net_device *dev, int flush) ipoib_dbg(priv, "downing ib_dev\n"); clear_bit(IPOIB_FLAG_OPER_UP, &priv->flags); + cancel_delayed_work(&priv->carrier_on_task); + netif_carrier_off(dev); /* Shutdown the P_Key thread if still active */ diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c index 2bf5116..5242e0d 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c @@ -1079,7 +1079,7 @@ static void ipoib_setup(struct net_device *dev) INIT_DELAYED_WORK(&priv->pkey_poll_task, ipoib_pkey_poll); INIT_DELAYED_WORK(&priv->mcast_task, ipoib_mcast_join_task); - INIT_WORK(&priv->carrier_on_task, ipoib_mcast_carrier_on_task); + INIT_DELAYED_WORK(&priv->carrier_on_task, ipoib_mcast_carrier_on_task); INIT_WORK(&priv->flush_light, ipoib_ib_dev_flush_light); INIT_WORK(&priv->flush_normal, ipoib_ib_dev_flush_normal); INIT_WORK(&priv->flush_heavy, ipoib_ib_dev_flush_heavy); diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c index 25874fc..b4b4016 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c @@ -361,13 +361,22 @@ static int ipoib_mcast_sendonly_join(struct ipoib_mcast *mcast) void ipoib_mcast_carrier_on_task(struct work_struct *work) { struct ipoib_dev_priv *priv = container_of(work, struct ipoib_dev_priv, - carrier_on_task); + carrier_on_task.work); + struct ib_port_attr attr; /* * Take rtnl_lock to avoid racing with ipoib_stop() and * turning the carrier back on while a device is being * removed. */ + + if (ib_query_port(priv->ca, priv->port, &attr) || + attr.state != IB_PORT_ACTIVE) { + ipoib_dbg(priv, "wait with carrier until IB port is active\n"); + if (test_bit(IPOIB_FLAG_OPER_UP, &priv->flags)) + queue_delayed_work(ipoib_workqueue, &priv->carrier_on_task, HZ); + return; + } rtnl_lock(); netif_carrier_on(priv->dev); rtnl_unlock(); @@ -403,7 +412,7 @@ static int ipoib_mcast_join_complete(int status, * deadlock on rtnl_lock here. */ if (mcast == priv->broadcast) - queue_work(ipoib_workqueue, &priv->carrier_on_task); + queue_delayed_work(ipoib_workqueue, &priv->carrier_on_task, 0); return 0; }
This patch fixes https://bugs.openfabrics.org/show_bug.cgi?id=1726 Multicast join can succeed even if IB port is down. This happens when OpenSM runs on the same port as the requesting port. IPoIB on the other hand, calls netif_carrier_on() when join succeeded without caring about the state of the IB port. The result is - an IPoIB interface in RUNNING state but without active IB port to support it. If a bonding interface uses this IPoIB interface as a slave it might not detect that this slave is almost useless and failover functionality will be damaged. The fix here is to check the state of the IB port in the carrier_task before calling netif_carrier_on(). Signed-off-by: Moni Shoua <monis@voltaire.com> --- drivers/infiniband/ulp/ipoib/ipoib.h | 2 +- drivers/infiniband/ulp/ipoib/ipoib_ib.c | 2 ++ drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 +- drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 13 +++++++++++-- 4 files changed, 15 insertions(+), 4 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html