IPoIB fixes for 4.11

---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/infiniband/ulp/ipoib/ipoib_main.c +4 -0

index 3ce0765..a550cc6 100644
@@ -229,6 +229,10 @@ static int ipoib_change_mtu(struct net_device *dev, int new_mtu)
 
 	priv->admin_mtu = new_mtu;
 
+	if (priv->mcast_mtu < priv->admin_mtu)
+		ipoib_warn(priv, "MTU must be smaller than mcast_mtu (%u)\n",
+			   priv->mcast_mtu);
+
 	dev->mtu = min(priv->mcast_mtu, priv->admin_mtu);
 
 	return 0;

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2016-12-27 13:39 UTC #3 lore raw

PATCH 2/9 IB/ipoib: Set device connection mode only when needed

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

When changing the connection mode, the ipoib_set_mode function did not check if the previous connection mode equals to the new one. This commit adds the required check and return 0 if the new mode equals to the previous one.

Fixes 839fcaba355a ("IPoIB: Connected mode experimental support")

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/infiniband/ulp/ipoib/ipoib_main.c +8 -0

index a550cc6..1787f6b 100644
@@ -474,6 +474,14 @@ int ipoib_set_mode(struct net_device *dev, const char *buf)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
 
+	if ((test_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags) &&
+	     !strcmp(buf, "connected\n")) ||
+	     (!test_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags) &&
+	     !strcmp(buf, "datagram\n"))) {
+		ipoib_dbg(priv, "already in that mode, goes out.\n");
+		return 0;
+	}
+
 	/* flush paths if we switch modes so that connections are restarted */
 	if (IPOIB_CM_SUPPORTED(dev->dev_addr) && !strcmp(buf, "connected\n")) {
 		set_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags);

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20161227133911.14340-3-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Yuval Shaia

Reviewed-by

2016-12-27 20:15 UTC #5 lore raw

Re: [PATCH rdma-next 2/9] IB/ipoib: Set device connection mode only when needed

Extremely minor comment inline, feel free to ignore.

Quoted 21 lines · Leon Romanovsky

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
When changing the connection mode, the ipoib_set_mode function did not check if the previous connection mode equals to the new one. This commit adds the required check and return 0 if the new mode equals to the previous one.
Fixes 839fcaba355a ("IPoIB: Connected mode experimental support")
Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>
Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/infiniband/ulp/ipoib/ipoib_main.c +0 -0
index a550cc6..1787f6b 100644

drivers/infiniband/ulp/ipoib/ipoib_main.c int ipoib_set_mode(struct net_device *dev, const char *buf) · line 481

Quoted 8 lines

drivers/infiniband/ulp/ipoib/ipoib_main.c +5 -0

@@ -474,6 +474,14 @@ int ipoib_set_mode(struct net_device *dev, const char *buf)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
 
+	if ((test_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags) &&
+	     !strcmp(buf, "connected\n")) ||
+	     (!test_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags) &&
+	     !strcmp(buf, "datagram\n"))) {
+		ipoib_dbg(priv, "already in that mode, goes out.\n");

Why not ipoib_warn/warn_once?

Quoted 13 lines

drivers/infiniband/ulp/ipoib/ipoib_main.c +3 -0

+		return 0;
+	}
+
 	/* flush paths if we switch modes so that connections are restarted */
 	if (IPOIB_CM_SUPPORTED(dev->dev_addr) && !strcmp(buf, "connected\n")) {
 		set_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags);

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2016-12-28 05:43 UTC #6 lore raw

Re: [PATCH rdma-next 2/9] IB/ipoib: Set device connection mode only when needed

Quoted 29 lines · Yuval Shaia

Extremely minor comment inline, feel free to ignore.
Reviewed-by Yuval Shaia <[email protected]>
Quoted 24 lines · Leon Romanovsky
From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
When changing the connection mode, the ipoib_set_mode function did not check if the previous connection mode equals to the new one. This commit adds the required check and return 0 if the new mode equals to the previous one.
Fixes 839fcaba355a ("IPoIB: Connected mode experimental support")
Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>
Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/infiniband/ulp/ipoib/ipoib_main.c +0 -0
index a550cc6..1787f6b 100644
@@ -474,6 +474,14 @@ int ipoib_set_mode(struct net_device *dev, const char *buf)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
 

Quoted 7 lines

drivers/infiniband/ulp/ipoib/ipoib_main.c int ipoib_set_mode(struct net_device *dev, const char *buf) · line 481
Quoted 5 lines
drivers/infiniband/ulp/ipoib/ipoib_main.c +5 -0
+	if ((test_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags) &&
+	     !strcmp(buf, "connected\n")) ||
+	     (!test_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags) &&
+	     !strcmp(buf, "datagram\n"))) {
+		ipoib_dbg(priv, "already in that mode, goes out.\n");
Why not ipoib_warn/warn_once?

We didn't use warn(), because setting the same mode more than once is a valid flow. However, it makes no sense to continue and this is why we are returning zero (sucesss) without doing a thing.

Quoted 14 lines

Quoted 14 lines
drivers/infiniband/ulp/ipoib/ipoib_main.c +3 -1
+		return 0;
+	}
+
 	/* flush paths if we switch modes so that connections are restarted */
 	if (IPOIB_CM_SUPPORTED(dev->dev_addr) && !strcmp(buf, "connected\n")) {
 		set_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags);
--
2.10.2
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2016-12-27 13:39 UTC #7 lore raw

PATCH 3/9 IB/ipoib: Fix deadlock over vlan_mutex

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

This patch fixes Deadlock while executing ipoib_vlan_delete.

The function takes the vlan_rwsem semaphore and calls unregister_netdevice. The later function calls ipoib_mcast_stop_thread that cause workqueue flush.

When the queue has one of the ipoib_ib_dev_flush_xxx events, a deadlock occur because these events also tries to catch the same vlan_rwsem semaphore.

To fix, unregister_netdevice should be called after releasing the semaphore.

Fixes cbbe1efa4972 ("IPoIB: Fix deadlock between ipoib_open() and child interface create")

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/infiniband/ulp/ipoib/ipoib_vlan.c +5 -1

index fd81111..9682183 100644
@@ -196,7 +196,6 @@ int ipoib_vlan_delete(struct net_device *pdev, unsigned short pkey)
 	list_for_each_entry_safe(priv, tpriv, &ppriv->child_intfs, list) {
 		if (priv->pkey == pkey &&
 		    priv->child_type == IPOIB_LEGACY_CHILD) {
-			unregister_netdevice(priv->dev);
 			list_del(&priv->list);
 			dev = priv->dev;
 			break;

@@ -204,6 +203,11 @@ int ipoib_vlan_delete(struct net_device *pdev, unsigned short pkey)
 	}
 	up_write(&ppriv->vlan_rwsem);
 
+	if (dev) {
+		ipoib_dbg(ppriv, "delete child vlan %s\n", dev->name);
+		unregister_netdevice(dev);
+	}
+
 	rtnl_unlock();
 
 	if (dev) {

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2016-12-27 13:39 UTC #8 lore raw

PATCH 4/9 IB/ipoib: Fix deadlock between rmmod and set_mode

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

When calling set_mode from sys/fs, the call flow locks the sys/fs lock first and then tries to lock rtnl_lock (when calling ipoib_set_mod). On the other hand, the rmmod call flow takes the rtnl_lock first (when calling unregister_netdev) and then tries to take the sys/fs lock. Deadlock a->b, b->a.

The problem starts when ipoib_set_mod frees it's rtnl_lck and tries to get it after that.

    set_mod:
    [<ffffffff8104f2bd>] ? check_preempt_curr+0x6d/0x90
    [<ffffffff814fee8e>] __mutex_lock_slowpath+0x13e/0x180
    [<ffffffff81448655>] ? __rtnl_unlock+0x15/0x20
    [<ffffffff814fed2b>] mutex_lock+0x2b/0x50
    [<ffffffff81448675>] rtnl_lock+0x15/0x20
    [<ffffffffa02ad807>] ipoib_set_mode+0x97/0x160 [ib_ipoib]
    [<ffffffffa02b5f5b>] set_mode+0x3b/0x80 [ib_ipoib]
    [<ffffffff8134b840>] dev_attr_store+0x20/0x30
    [<ffffffff811f0fe5>] sysfs_write_file+0xe5/0x170
    [<ffffffff8117b068>] vfs_write+0xb8/0x1a0
    [<ffffffff8117ba81>] sys_write+0x51/0x90
    [<ffffffff8100b0f2>] system_call_fastpath+0x16/0x1b

    rmmod:
    [<ffffffff81279ffc>] ? put_dec+0x10c/0x110
    [<ffffffff8127a2ee>] ? number+0x2ee/0x320
    [<ffffffff814fe6a5>] schedule_timeout+0x215/0x2e0
    [<ffffffff8127cc04>] ? vsnprintf+0x484/0x5f0
    [<ffffffff8127b550>] ? string+0x40/0x100
    [<ffffffff814fe323>] wait_for_common+0x123/0x180
    [<ffffffff81060250>] ? default_wake_function+0x0/0x20
    [<ffffffff8119661e>] ? ifind_fast+0x5e/0xb0
    [<ffffffff814fe43d>] wait_for_completion+0x1d/0x20
    [<ffffffff811f2e68>] sysfs_addrm_finish+0x228/0x270
    [<ffffffff811f2fb3>] sysfs_remove_dir+0xa3/0xf0
    [<ffffffff81273f66>] kobject_del+0x16/0x40
    [<ffffffff8134cd14>] device_del+0x184/0x1e0
    [<ffffffff8144e59b>] netdev_unregister_kobject+0xab/0xc0
    [<ffffffff8143c05e>] rollback_registered+0xae/0x130
    [<ffffffff8143c102>] unregister_netdevice+0x22/0x70
    [<ffffffff8143c16e>] unregister_netdev+0x1e/0x30
    [<ffffffffa02a91b0>] ipoib_remove_one+0xe0/0x120 [ib_ipoib]
    [<ffffffffa01ed95f>] ib_unregister_device+0x4f/0x100 [ib_core]
    [<ffffffffa021f5e1>] mlx4_ib_remove+0x41/0x180 [mlx4_ib]
    [<ffffffffa01ab771>] mlx4_remove_device+0x71/0x90 [mlx4_core]

Fixes 862096a8bbf8 ("IB/ipoib: Add more rtnl_link_ops callbacks")

Cc <[email protected]> # v3.6+

Cc Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c   | 12 +++++++-----
 drivers/infiniband/ulp/ipoib/ipoib_main.c |  6 ++----
 2 files changed, 9 insertions(+), 9 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_cm.c +7 -5

index 096c4f6..1c7a9a1 100644
@@ -1507,12 +1507,14 @@ static ssize_t set_mode(struct device *d, struct device_attribute *attr,
 
 	ret = ipoib_set_mode(dev, buf);
 
-	rtnl_unlock();
-
-	if (!ret)
-		return count;
+	/* The assumption is that the function ipoib_set_mode returned
+	 * with the rtnl held by it, if not the value -EBUSY returned,
+	 * then no need to rtnl_unlock
+	 */
+	if (ret != -EBUSY)
+		rtnl_unlock();
 
-	return ret;
+	return (!ret || ret == -EBUSY) ? count : ret;
 }
 
 static DEVICE_ATTR(mode, S_IWUSR | S_IRUGO, show_mode, set_mode);

drivers/infiniband/ulp/ipoib/ipoib_main.c +2 -4

index 1787f6b..1090fe2 100644
@@ -493,8 +493,7 @@ int ipoib_set_mode(struct net_device *dev, const char *buf)
 		priv->tx_wr.wr.send_flags &= ~IB_SEND_IP_CSUM;
 
 		ipoib_flush_paths(dev);
-		rtnl_lock();
-		return 0;
+		return (!rtnl_trylock()) ? -EBUSY : 0;
 	}
 
 	if (!strcmp(buf, "datagram\n")) {

@@ -503,8 +502,7 @@ int ipoib_set_mode(struct net_device *dev, const char *buf)
 		dev_set_mtu(dev, min(priv->mcast_mtu, dev->mtu));
 		rtnl_unlock();
 		ipoib_flush_paths(dev);
-		rtnl_lock();
-		return 0;
+		return (!rtnl_trylock()) ? -EBUSY : 0;
 	}
 
 	return -EINVAL;

--
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2016-12-27 13:39 UTC #9 lore raw

PATCH 5/9 IB/ipoib: rtnl_unlock can not come after free_netdev

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

The ipoib_vlan_add function calls rtnl_unlock after free_netdev, rtnl_unlock not only releases the lock, but also calls netdev_run_todo. The latter function browses the net_todo_list array and completes the unregistration of all its net_device instances. If we call free_netdev before rtnl_unlock, then netdev_run_todo call over the freed device causes panic.
To fix, move rtnl_unlock call before free_netdev call.

Fixes 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")

Cc Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_vlan.c +2 -2

index 9682183..d9dab4a 100644
@@ -168,11 +168,11 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
 out:
 	up_write(&ppriv->vlan_rwsem);
 
+	rtnl_unlock();
+
 	if (result)
 		free_netdev(priv->dev);
 
-	rtnl_unlock();
-
 	return result;
 }

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20161227133911.14340-6-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Yuval Shaia

Reviewed-by

2016-12-28 12:05 UTC #11 lore raw

Re: [PATCH rdma-next 5/9] IB/ipoib: rtnl_unlock can not come after free_netdev

Quoted 44 lines · Leon Romanovsky

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
The ipoib_vlan_add function calls rtnl_unlock after free_netdev, rtnl_unlock not only releases the lock, but also calls netdev_run_todo. The latter function browses the net_todo_list array and completes the unregistration of all its net_device instances. If we call free_netdev before rtnl_unlock, then netdev_run_todo call over the freed device causes panic.
To fix, move rtnl_unlock call before free_netdev call.
Fixes 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
Cc Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib_vlan.c +2 -2
index 9682183..d9dab4a 100644
@@ -168,11 +168,11 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
 out:
 	up_write(&ppriv->vlan_rwsem);
 
+	rtnl_unlock();
+
 	if (result)
 		free_netdev(priv->dev);
 
-	rtnl_unlock();
-
 	return result;
 }
 
Signature · 6 lines
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2016-12-27 13:39 UTC #12 lore raw

PATCH 6/9 IB/ipoib: Add detailed error message to dev_queue_xmit call

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Add a detailed return code to dev_queue_xmit function when calling to requeue packet via __skb_dequeue.

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c        | 7 ++++---
 drivers/infiniband/ulp/ipoib/ipoib_main.c      | 8 +++++---
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 6 ++++--
 3 files changed, 13 insertions(+), 8 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_cm.c +4 -3

index 1c7a9a1..a720d2d 100644
@@ -1015,9 +1015,10 @@ static int ipoib_cm_rep_handler(struct ib_cm_id *cm_id, struct ib_cm_event *even
 
 	while ((skb = __skb_dequeue(&skqueue))) {
 		skb->dev = p->dev;
-		if (dev_queue_xmit(skb))
-			ipoib_warn(priv, "dev_queue_xmit failed "
-				   "to requeue packet\n");
+		ret = dev_queue_xmit(skb);
+		if (ret)
+			ipoib_warn(priv, "%s:dev_queue_xmit failed to re-queue packet, ret:%d\n",
+				   __func__, ret);
 	}
 
 	ret = ib_send_cm_rtu(cm_id, NULL, 0);

drivers/infiniband/ulp/ipoib/ipoib_main.c +5 -3

index 1090fe2..b5e1e4d 100644
@@ -844,10 +844,12 @@ static void path_rec_completion(int status,
 		ipoib_put_ah(old_ah);
 
 	while ((skb = __skb_dequeue(&skqueue))) {
+		int ret;
 		skb->dev = dev;
-		if (dev_queue_xmit(skb))
-			ipoib_warn(priv, "dev_queue_xmit failed "
-				   "to requeue packet\n");
+		ret = dev_queue_xmit(skb);
+		if (ret)
+			ipoib_warn(priv, "%s: dev_queue_xmit failed to re-queue packet, ret:%d\n",
+				   __func__, ret);
 	}
 }

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +4 -2

index fddff40..7c6c67b 100644
@@ -314,9 +314,11 @@ static int ipoib_mcast_join_finish(struct ipoib_mcast *mcast,
 		netif_tx_unlock_bh(dev);
 
 		skb->dev = dev;
-		if (dev_queue_xmit(skb))
-			ipoib_warn(priv, "dev_queue_xmit failed to requeue packet\n");
 
+		ret = dev_queue_xmit(skb);
+		if (ret)
+			ipoib_warn(priv, "%s:dev_queue_xmit failed to re-queue packet, ret:%d\n",
+				   __func__, ret);
 		netif_tx_lock_bh(dev);
 	}
 	netif_tx_unlock_bh(dev);

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20161227133911.14340-7-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Yuval Shaia

Reviewed-by

2016-12-28 12:11 UTC #14 lore raw

Re: [PATCH rdma-next 6/9] IB/ipoib: Add detailed error message to dev_queue_xmit call

Quoted 77 lines · Leon Romanovsky

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Add a detailed return code to dev_queue_xmit function when calling to requeue packet via __skb_dequeue.

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c        | 7 ++++---
 drivers/infiniband/ulp/ipoib/ipoib_main.c      | 8 +++++---
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 6 ++++--
 3 files changed, 13 insertions(+), 8 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_cm.c +4 -3

index 1c7a9a1..a720d2d 100644
@@ -1015,9 +1015,10 @@ static int ipoib_cm_rep_handler(struct ib_cm_id *cm_id, struct ib_cm_event *even
 
 	while ((skb = __skb_dequeue(&skqueue))) {
 		skb->dev = p->dev;
-		if (dev_queue_xmit(skb))
-			ipoib_warn(priv, "dev_queue_xmit failed "
-				   "to requeue packet\n");
+		ret = dev_queue_xmit(skb);
+		if (ret)
+			ipoib_warn(priv, "%s:dev_queue_xmit failed to re-queue packet, ret:%d\n",
+				   __func__, ret);
 	}
 
 	ret = ib_send_cm_rtu(cm_id, NULL, 0);

drivers/infiniband/ulp/ipoib/ipoib_main.c +5 -3

index 1090fe2..b5e1e4d 100644
@@ -844,10 +844,12 @@ static void path_rec_completion(int status,
 		ipoib_put_ah(old_ah);
 
 	while ((skb = __skb_dequeue(&skqueue))) {
+		int ret;
 		skb->dev = dev;
-		if (dev_queue_xmit(skb))
-			ipoib_warn(priv, "dev_queue_xmit failed "
-				   "to requeue packet\n");
+		ret = dev_queue_xmit(skb);
+		if (ret)
+			ipoib_warn(priv, "%s: dev_queue_xmit failed to re-queue packet, ret:%d\n",
+				   __func__, ret);
 	}
 }

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +4 -2

index fddff40..7c6c67b 100644
@@ -314,9 +314,11 @@ static int ipoib_mcast_join_finish(struct ipoib_mcast *mcast,
 		netif_tx_unlock_bh(dev);
 
 		skb->dev = dev;
-		if (dev_queue_xmit(skb))
-			ipoib_warn(priv, "dev_queue_xmit failed to requeue packet\n");
 
+		ret = dev_queue_xmit(skb);
+		if (ret)
+			ipoib_warn(priv, "%s:dev_queue_xmit failed to re-queue packet, ret:%d\n",
+				   __func__, ret);
 		netif_tx_lock_bh(dev);
 	}
 	netif_tx_unlock_bh(dev);

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2016-12-27 13:39 UTC #15 lore raw

PATCH 7/9 IB/ipoib: Use debug prints instead of warnings in RNR WC status

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

If a receive request has not been posted to the work queue, the incoming message is rejected and the peer will receive a receiver-not-ready (RNR) error. In IPoIB, IB_WC_RNR_RETRY_EXC_ERR error is part of the life cycle therefore ipoib_cm_handle_tx_wc function will print to debug instead of warnings.

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_cm.c +6 -3

index a720d2d..b6444b2 100644
@@ -820,9 +820,12 @@ void ipoib_cm_handle_tx_wc(struct net_device *dev, struct ib_wc *wc)
 	    wc->status != IB_WC_WR_FLUSH_ERR) {
 		struct ipoib_neigh *neigh;
 
-		ipoib_dbg(priv, "failed cm send event "
-			   "(status=%d, wrid=%d vend_err %x)\n",
-			   wc->status, wr_id, wc->vendor_err);
+		if (wc->status != IB_WC_RNR_RETRY_EXC_ERR)
+			ipoib_warn(priv, "%s: failed cm send event (status=%d, wrid=%d vend_err %x)\n",
+				   __func__, wc->status, wr_id, wc->vendor_err);
+		else
+			ipoib_dbg(priv, "%s: failed cm send event (status=%d, wrid=%d vend_err %x)\n",
+				  __func__, wc->status, wr_id, wc->vendor_err);
 
 		spin_lock_irqsave(&priv->lock, flags);
 		neigh = tx->neigh;

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20161227133911.14340-8-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Yuval Shaia

2016-12-28 12:15 UTC #17 lore raw

Re: [PATCH rdma-next 7/9] IB/ipoib: Use debug prints instead of warnings in RNR WC status

Quoted 20 lines · Leon Romanovsky

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
If a receive request has not been posted to the work queue, the incoming message is rejected and the peer will receive a receiver-not-ready (RNR) error. In IPoIB, IB_WC_RNR_RETRY_EXC_ERR error is part of the life cycle therefore ipoib_cm_handle_tx_wc function will print to debug instead of warnings.
Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib_cm.c +0 -0
index a720d2d..b6444b2 100644

drivers/infiniband/ulp/ipoib/ipoib_cm.c void ipoib_cm_handle_tx_wc(struct net_device *dev, struct ib_wc *wc) · line 825

Quoted 9 lines

drivers/infiniband/ulp/ipoib/ipoib_cm.c +3 -3

@@ -820,9 +820,12 @@ void ipoib_cm_handle_tx_wc(struct net_device *dev, struct ib_wc *wc)
 	    wc->status != IB_WC_WR_FLUSH_ERR) {
 		struct ipoib_neigh *neigh;
 
-		ipoib_dbg(priv, "failed cm send event "
-			   "(status=%d, wrid=%d vend_err %x)\n",
-			   wc->status, wr_id, wc->vendor_err);
+		if (wc->status != IB_WC_RNR_RETRY_EXC_ERR)
+			ipoib_warn(priv, "%s: failed cm send event (status=%d, wrid=%d vend_err %x)\n",
+				   __func__, wc->status, wr_id, wc->vendor_err);

Do we really need function-name to be printed here?

Quoted 13 lines

drivers/infiniband/ulp/ipoib/ipoib_cm.c +3 -0

+		else
+			ipoib_dbg(priv, "%s: failed cm send event (status=%d, wrid=%d vend_err %x)\n",
+				  __func__, wc->status, wr_id, wc->vendor_err);
 
 		spin_lock_irqsave(&priv->lock, flags);
 		neigh = tx->neigh;

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2016-12-28 12:22 UTC #18 lore raw

Re: [PATCH rdma-next 7/9] IB/ipoib: Use debug prints instead of warnings in RNR WC status

Quoted 24 lines · Yuval Shaia

Quoted 23 lines · Leon Romanovsky
From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
If a receive request has not been posted to the work queue, the incoming message is rejected and the peer will receive a receiver-not-ready (RNR) error. In IPoIB, IB_WC_RNR_RETRY_EXC_ERR error is part of the life cycle therefore ipoib_cm_handle_tx_wc function will print to debug instead of warnings.
Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib_cm.c +0 -0
index a720d2d..b6444b2 100644
@@ -820,9 +820,12 @@ void ipoib_cm_handle_tx_wc(struct net_device *dev, struct ib_wc *wc)
 	    wc->status != IB_WC_WR_FLUSH_ERR) {
 		struct ipoib_neigh *neigh;
 

Quoted 8 lines

drivers/infiniband/ulp/ipoib/ipoib_cm.c void ipoib_cm_handle_tx_wc(struct net_device *dev, struct ib_wc *wc) · line 825
Quoted 6 lines
drivers/infiniband/ulp/ipoib/ipoib_cm.c +3 -3
-		ipoib_dbg(priv, "failed cm send event "
-			   "(status=%d, wrid=%d vend_err %x)\n",
-			   wc->status, wr_id, wc->vendor_err);
+		if (wc->status != IB_WC_RNR_RETRY_EXC_ERR)
+			ipoib_warn(priv, "%s: failed cm send event (status=%d, wrid=%d vend_err %x)\n",
+				   __func__, wc->status, wr_id, wc->vendor_err);
Do we really need function-name to be printed here?

You are right,
Sorry, I missed it in review and will respin without __func__.

Quoted 14 lines

Quoted 14 lines
drivers/infiniband/ulp/ipoib/ipoib_cm.c +3 -1
+		else
+			ipoib_dbg(priv, "%s: failed cm send event (status=%d, wrid=%d vend_err %x)\n",
+				  __func__, wc->status, wr_id, wc->vendor_err);
 
 		spin_lock_irqsave(&priv->lock, flags);
 		neigh = tx->neigh;
--
2.10.2
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2016-12-27 13:39 UTC #19 lore raw

PATCH 8/9 IB/ipoib: Replace list_del of the neigh->list with list_del_init

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

In order to resolve a situation where a few process delete the same list element in sequence and cause panic, list_del is replaced with list_del_init. In this case if the first process that calls list_del releases the lock before acquiring it again, other processes who can acquire the lock will call list_del_init.

Fixes b63b70d87741 ("IPoIB: Use a private hash table for path lookup")

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_main.c +4 -4

index b5e1e4d..d8af197 100644
@@ -1298,7 +1298,7 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
 						   rcu_dereference_protected(neigh->hnext,
 									     lockdep_is_held(&priv->lock)));
 				/* remove from path/mc list */
-				list_del(&neigh->list);
+				list_del_init(&neigh->list);
 				call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 			} else {
 				np = &neigh->hnext;

@@ -1462,7 +1462,7 @@ void ipoib_neigh_free(struct ipoib_neigh *neigh)
 					   rcu_dereference_protected(neigh->hnext,
 								     lockdep_is_held(&priv->lock)));
 			/* remove from parent list */
-			list_del(&neigh->list);
+			list_del_init(&neigh->list);
 			call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 			return;
 		} else {

@@ -1547,7 +1547,7 @@ void ipoib_del_neighs_by_gid(struct net_device *dev, u8 *gid)
 						   rcu_dereference_protected(neigh->hnext,
 									     lockdep_is_held(&priv->lock)));
 				/* remove from parent list */
-				list_del(&neigh->list);
+				list_del_init(&neigh->list);
 				call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 			} else {
 				np = &neigh->hnext;

@@ -1589,7 +1589,7 @@ static void ipoib_flush_neighs(struct ipoib_dev_priv *priv)
 					   rcu_dereference_protected(neigh->hnext,
 								     lockdep_is_held(&priv->lock)));
 			/* remove from path/mc list */
-			list_del(&neigh->list);
+			list_del_init(&neigh->list);
 			call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 		}
 	}

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20161227133911.14340-9-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Yuval Shaia

Reviewed-by

2016-12-28 13:01 UTC #21 lore raw

Re: [PATCH rdma-next 8/9] IB/ipoib: Replace list_del of the neigh->list with list_del_init

Quoted 4 lines · Leon Romanovsky

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
In order to resolve a situation where a few process delete the same list element in sequence and cause panic, list_del

s/"the same list element"/"the same neigh element"

Assuming this applies to handling neigh list and not a general rule we shall stick to. In this case suggesting a minor rephrasing of the commit message.

Besides that:

Quoted 61 lines

is replaced with list_del_init. In this case if the first process that calls list_del releases the lock before acquiring it again, other processes who can acquire the lock will call list_del_init.

Fixes b63b70d87741 ("IPoIB: Use a private hash table for path lookup")

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_main.c +4 -4

index b5e1e4d..d8af197 100644
@@ -1298,7 +1298,7 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
 						   rcu_dereference_protected(neigh->hnext,
 									     lockdep_is_held(&priv->lock)));
 				/* remove from path/mc list */
-				list_del(&neigh->list);
+				list_del_init(&neigh->list);
 				call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 			} else {
 				np = &neigh->hnext;

@@ -1462,7 +1462,7 @@ void ipoib_neigh_free(struct ipoib_neigh *neigh)
 					   rcu_dereference_protected(neigh->hnext,
 								     lockdep_is_held(&priv->lock)));
 			/* remove from parent list */
-			list_del(&neigh->list);
+			list_del_init(&neigh->list);
 			call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 			return;
 		} else {

@@ -1547,7 +1547,7 @@ void ipoib_del_neighs_by_gid(struct net_device *dev, u8 *gid)
 						   rcu_dereference_protected(neigh->hnext,
 									     lockdep_is_held(&priv->lock)));
 				/* remove from parent list */
-				list_del(&neigh->list);
+				list_del_init(&neigh->list);
 				call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 			} else {
 				np = &neigh->hnext;

@@ -1589,7 +1589,7 @@ static void ipoib_flush_neighs(struct ipoib_dev_priv *priv)
 					   rcu_dereference_protected(neigh->hnext,
 								     lockdep_is_held(&priv->lock)));
 			/* remove from path/mc list */
-			list_del(&neigh->list);
+			list_del_init(&neigh->list);
 			call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 		}
 	}

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2016-12-27 13:39 UTC #22 lore raw

PATCH 9/9 IB/ipoib: Change list_del to list_del_init in the tx object

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Since ipoib_cm_tx_start function and ipoib_cm_tx_reap function belong to different work queues, they can run in parallel.
In this case if ipoib_cm_tx_reap calls list_del and release the lock, ipoib_cm_tx_start may acquire it and call list_del_init on the already deleted object.
Changing list_del to list_del_init in ipoib_cm_tx_reap fixes the problem.

Fixes 839fcaba355a ("IPoIB: Connected mode experimental support")

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/infiniband/ulp/ipoib/ipoib_cm.c +1 -1

index b6444b2..cc08379 100644
@@ -1392,7 +1392,7 @@ static void ipoib_cm_tx_reap(struct work_struct *work)
 
 	while (!list_empty(&priv->cm.reap_list)) {
 		p = list_entry(priv->cm.reap_list.next, typeof(*p), list);
-		list_del(&p->list);
+		list_del_init(&p->list);
 		spin_unlock_irqrestore(&priv->lock, flags);
 		netif_tx_unlock_bh(dev);
 		ipoib_cm_tx_destroy(p);

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20161227133911.14340-10-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Yuval Shaia

Reviewed-by

2016-12-28 13:06 UTC #24 lore raw

Re: [PATCH rdma-next 9/9] IB/ipoib: Change list_del to list_del_init in the tx object

Quoted 24 lines · Leon Romanovsky

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Since ipoib_cm_tx_start function and ipoib_cm_tx_reap function belong to different work queues, they can run in parallel.
In this case if ipoib_cm_tx_reap calls list_del and release the lock, ipoib_cm_tx_start may acquire it and call list_del_init on the already deleted object.
Changing list_del to list_del_init in ipoib_cm_tx_reap fixes the problem.
Fixes 839fcaba355a ("IPoIB: Connected mode experimental support")
Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>
Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/ulp/ipoib/ipoib_cm.c +0 -0
index b6444b2..cc08379 100644

drivers/infiniband/ulp/ipoib/ipoib_cm.c static void ipoib_cm_tx_reap(struct work_struct *work) · line 1398

Quoted 8 lines

drivers/infiniband/ulp/ipoib/ipoib_cm.c +1 -1

@@ -1392,7 +1392,7 @@ static void ipoib_cm_tx_reap(struct work_struct *work)
 
 	while (!list_empty(&priv->cm.reap_list)) {
 		p = list_entry(priv->cm.reap_list.next, typeof(*p), list);
-		list_del(&p->list);
+		list_del_init(&p->list);
 		spin_unlock_irqrestore(&priv->lock, flags);
 		netif_tx_unlock_bh(dev);
 		ipoib_cm_tx_destroy(p);