[PATCH rdma-next V1 0/9] IPoIB fixes for 4.11

[PATCH rdma-next V1 0/9] IPoIB fixes for 4.11

[Leon Romanovsky]

Hi Doug,

This is secnd version of 8 patches from Feras for IPoIB.

The patchset was generated against v4.10-rc1 and targeted for 4.11.

The patch "IB/ipoib: Fix deadlock between rmmod and set_mode" which fix kernel panic
is marked as candidate for stable tree inclusion.

Available in the "topic/ipoib-fixes-4.11" topic branch of this git repo:
git://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git

Or for browsing:
https://git.kernel.org/cgit/linux/kernel/git/leon/linux-rdma.git/log/?h=topic/ipoib-fixes-4.11

Thanks

Changelog v0 -> v1:
 * Patch 2,5,6 - add Yuval Shaia's ROB tags
 * Patch 7 - remove __func__ prints

Feras Daoud (9):
  IB/ipoib: Add warning message when changing the MTU in UD over the max
    range
  IB/ipoib: Set device connection mode only when needed
  IB/ipoib: Fix deadlock over vlan_mutex
  IB/ipoib: Fix deadlock between rmmod and set_mode
  IB/ipoib: rtnl_unlock can not come after free_netdev
  IB/ipoib: Add detailed error message to dev_queue_xmit call
  IB/ipoib: Use debug prints instead of warnings in RNR WC status
  IB/ipoib: Replace list_del of the neigh->list with list_del_init
  IB/ipoib: Change list_del to list_del_init in the tx object

 drivers/infiniband/ulp/ipoib/ipoib_cm.c        | 30 ++++++++++++++---------
 drivers/infiniband/ulp/ipoib/ipoib_main.c      | 34 +++++++++++++++++---------
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c |  6 +++--
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c      | 10 +++++---
 4 files changed, 52 insertions(+), 28 deletions(-)

--
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Leon Romanovsky]

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

In datagram mode, the IB UD (Unreliable Datagram) transport is used
so the MTU of the interface is equal to the IB L2 MTU minus the
IPoIB encapsulation header. Any request to change the MTU value
above the maximum range will change the MTU to the max allowed, but
will not show any warning message. An ipoib_warn is issued in such
cases, letting the user know that even though the value is legal,
it can't be currently applied.

Signed-off-by: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Noa Osherovich <noaos-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 3ce0765..a550cc6 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -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;
-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Leon Romanovsky]

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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/w@public.gmane.org>
Signed-off-by: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index a550cc6..1787f6b 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -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);
--
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next V1 3/9] IB/ipoib: Fix deadlock over vlan_mutex

[Leon Romanovsky]

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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/w@public.gmane.org>
Signed-off-by: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index fd81111..9682183 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -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) {
-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next V1 4/9] IB/ipoib: Fix deadlock between rmmod and set_mode

[Leon Romanovsky]

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> # v3.6+
Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c   | 12 +++++++-----
 drivers/infiniband/ulp/ipoib/ipoib_main.c |  6 ++----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index 096c4f6..1c7a9a1 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -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);
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 1787f6b..1090fe2 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Leon Romanovsky]

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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/w@public.gmane.org>
Signed-off-by: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index 9682183..d9dab4a 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -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;
 }

--
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Leon Romanovsky]

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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/w@public.gmane.org>
Signed-off-by: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 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(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index 1c7a9a1..a720d2d 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -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);
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 1090fe2..b5e1e4d 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -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);
 	}
 }

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index fddff40..7c6c67b 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -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);
--
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Leon Romanovsky]

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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/w@public.gmane.org>
Signed-off-by: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index a720d2d..c433e72 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -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, "failed cm send event (status=%d, wrid=%d vend_err %x)\n",
+				   wc->status, wr_id, wc->vendor_err);
+		else
+			ipoib_dbg(priv, "failed cm send event (status=%d, wrid=%d vend_err %x)\n",
+				  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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Leon Romanovsky]

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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/w@public.gmane.org>
Signed-off-by: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index b5e1e4d..d8af197 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -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);
 		}
 	}
-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Leon Romanovsky]

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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/w@public.gmane.org>
Signed-off-by: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index c433e72..53d69d7 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -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);
-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Yuval Shaia]

On Wed, Dec 28, 2016 at 02:47:26PM +0200, Leon Romanovsky wrote:
> From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> 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/w@public.gmane.org>
> Signed-off-by: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/infiniband/ulp/ipoib/ipoib_cm.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> index a720d2d..c433e72 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> @@ -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, "failed cm send event (status=%d, wrid=%d vend_err %x)\n",
> +				   wc->status, wr_id, wc->vendor_err);
> +		else
> +			ipoib_dbg(priv, "failed cm send event (status=%d, wrid=%d vend_err %x)\n",
> +				  wc->status, wr_id, wc->vendor_err);
>  
>  		spin_lock_irqsave(&priv->lock, flags);
>  		neigh = tx->neigh;

Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

> -- 
> 2.10.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Or Gerlitz]

On Wed, Dec 28, 2016 at 2:47 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>
> 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.


RU claiming that we are crashing 100.0% here? since when? ever?

> To fix, move rtnl_unlock call before free_netdev call.
>
> Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
> Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> index 9682183..d9dab4a 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> @@ -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;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1822 bytes --]

On Thu, Dec 29, 2016 at 10:55:34PM +0200, Or Gerlitz wrote:
> On Wed, Dec 28, 2016 at 2:47 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > 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.
>
>
> RU claiming that we are crashing 100.0% here? since when? ever?

According to our internal bugtracker (see #131352), this bug was
literaly forever (4 years old), and it was fixed internaly in
MOFED 2.0.

>
> > To fix, move rtnl_unlock call before free_netdev call.
> >
> > Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
> > Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>
> > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> > index 9682183..d9dab4a 100644
> > --- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> > +++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> > @@ -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;
> >  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Or Gerlitz]

On Sun, Jan 1, 2017 at 8:39 AM, Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> On Thu, Dec 29, 2016 at 10:55:34PM +0200, Or Gerlitz wrote:
>> On Wed, Dec 28, 2016 at 2:47 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> > From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> >
>> > 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.
>>
>>
>> RU claiming that we are crashing 100.0% here? since when? ever?
>
> According to our internal bugtracker (see #131352), this bug was
> literaly forever (4 years old), and it was fixed internaly in
> MOFED 2.0.

Did you try it out? e.g to see that this crashes 100% as you claim?

I am pretty much sure this is not correct, and I'd like to see Erez commenting
on that and supporting your claim.

>> > To fix, move rtnl_unlock call before free_netdev call.
>> > Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
>> > Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Erez Shitrit]

On Sun, Jan 1, 2017 at 9:19 AM, Or Gerlitz <gerlitz.or-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Sun, Jan 1, 2017 at 8:39 AM, Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
>> On Thu, Dec 29, 2016 at 10:55:34PM +0200, Or Gerlitz wrote:
>>> On Wed, Dec 28, 2016 at 2:47 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>> > From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>> >
>>> > 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.
>>>
>>>
>>> RU claiming that we are crashing 100.0% here? since when? ever?
>>
>> According to our internal bugtracker (see #131352), this bug was
>> literaly forever (4 years old), and it was fixed internaly in
>> MOFED 2.0.
>
> Did you try it out? e.g to see that this crashes 100% as you claim?
>
> I am pretty much sure this is not correct, and I'd like to see Erez commenting
> on that and supporting your claim.
>
It happens in error flow only, when the call for __ipoib_vlan_add
returns with error, in that case we will call free_netdev before
rtnl_unlock.

The fix is similar to what we have in ipoib_vlan_delete function:

"
         ....
         rtnl_unlock();

         if (dev) {
                 free_netdev(dev);
                 return 0;
         }
"

First rtnl_unlock() and then  free_netdev

Thanks, Erez

>>> > To fix, move rtnl_unlock call before free_netdev call.
>>> > Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
>>> > Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Yuval Shaia]

On Wed, Dec 28, 2016 at 02:47:28PM +0200, Leon Romanovsky wrote:
> From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> 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/w@public.gmane.org>
> Signed-off-by: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/infiniband/ulp/ipoib/ipoib_cm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> index c433e72..53d69d7 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> @@ -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);

Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

> -- 
> 2.10.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Yuval Shaia]

On Wed, Dec 28, 2016 at 02:47:27PM +0200, Leon Romanovsky wrote:
> From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Any reason why this patch, out of 3 in this patch set uses "Replace"
instead of "Change" in commit header?

Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

> 
> 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/w@public.gmane.org>
> Signed-off-by: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index b5e1e4d..d8af197 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -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);
>  		}
>  	}
> -- 
> 2.10.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 444 bytes --]

On Mon, Jan 02, 2017 at 02:29:18PM +0200, Yuval Shaia wrote:
> On Wed, Dec 28, 2016 at 02:47:27PM +0200, Leon Romanovsky wrote:
> > From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>
> Any reason why this patch, out of 3 in this patch set uses "Replace"
> instead of "Change" in commit header?

No real reason.

>
> Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

Thank you for your reviewes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2369 bytes --]

On Wed, 2016-12-28 at 14:47 +0200, Leon Romanovsky wrote:
> From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> In datagram mode, the IB UD (Unreliable Datagram) transport is used
> so the MTU of the interface is equal to the IB L2 MTU minus the
> IPoIB encapsulation header. Any request to change the MTU value
> above the maximum range will change the MTU to the max allowed, but
> will not show any warning message. An ipoib_warn is issued in such
> cases, letting the user know that even though the value is legal,
> it can't be currently applied.
> 
> Signed-off-by: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Noa Osherovich <noaos-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/infiniband/ulp/ipoib/ipoib_main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index 3ce0765..a550cc6 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -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;

I don't like this patch.  First, there's no need for a warning here.
 That's entirely too noisy for this issue.  Second, the wording of the
message is poor.  The user thinks they are setting the MTU, and there
is no means of setting a multicast MTU, so telling them that their new
MTU must be less than the mcast MTU that they can't do anything about
and don't necessarily know how it is generated makes no sense.  This
should be no more than ipoib_dbg if we even print anything out at all,
and the message should be more like "MTU must be <= the link layer MTU
- 4, use ibv_devinfo on the RDMA device to get the link layer MTU"

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1884 bytes --]

On Wed, 2016-12-28 at 14:47 +0200, Leon Romanovsky wrote:
> From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> 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/w@public.gmane.org>
> Signed-off-by: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index a550cc6..1787f6b 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -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");

There is no need for any message here.  Nothing unexpected is
happening.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-next V1 0/9] IPoIB fixes for 4.11

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]

On Wed, 2016-12-28 at 14:47 +0200, Leon Romanovsky wrote:
> Hi Doug,
> 
> This is secnd version of 8 patches from Feras for IPoIB.
> 
> The patchset was generated against v4.10-rc1 and targeted for 4.11.
> 
> The patch "IB/ipoib: Fix deadlock between rmmod and set_mode" which
> fix kernel panic
> is marked as candidate for stable tree inclusion.
> 
> Available in the "topic/ipoib-fixes-4.11" topic branch of this git
> repo:
> git://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git
> 
> Or for browsing:
> https://git.kernel.org/cgit/linux/kernel/git/leon/linux-rdma.git/log/
> ?h=topic/ipoib-fixes-4.11
> 
> Thanks
> 
> Changelog v0 -> v1:
>  * Patch 2,5,6 - add Yuval Shaia's ROB tags
>  * Patch 7 - remove __func__ prints
> 
> Feras Daoud (9):
>   IB/ipoib: Add warning message when changing the MTU in UD over the
> max
>     range
>   IB/ipoib: Set device connection mode only when needed

I fixed up both of these patches according to my comments.

>   IB/ipoib: Fix deadlock over vlan_mutex
>   IB/ipoib: Fix deadlock between rmmod and set_mode
>   IB/ipoib: rtnl_unlock can not come after free_netdev
>   IB/ipoib: Add detailed error message to dev_queue_xmit call
>   IB/ipoib: Use debug prints instead of warnings in RNR WC status
>   IB/ipoib: Replace list_del of the neigh->list with list_del_init
>   IB/ipoib: Change list_del to list_del_init in the tx object
> 
>  drivers/infiniband/ulp/ipoib/ipoib_cm.c        | 30 ++++++++++++++
> ---------
>  drivers/infiniband/ulp/ipoib/ipoib_main.c      | 34
> +++++++++++++++++---------
>  drivers/infiniband/ulp/ipoib/ipoib_multicast.c |  6 +++--
>  drivers/infiniband/ulp/ipoib/ipoib_vlan.c      | 10 +++++---
>  4 files changed, 52 insertions(+), 28 deletions(-)

Thanks, series (after modifications) applied.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-next V1 0/9] IPoIB fixes for 4.11

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2196 bytes --]

On Thu, Jan 12, 2017 at 02:02:17PM -0500, Doug Ledford wrote:
> On Wed, 2016-12-28 at 14:47 +0200, Leon Romanovsky wrote:
> > Hi Doug,
> >
> > This is secnd version of 8 patches from Feras for IPoIB.
> >
> > The patchset was generated against v4.10-rc1 and targeted for 4.11.
> >
> > The patch "IB/ipoib: Fix deadlock between rmmod and set_mode" which
> > fix kernel panic
> > is marked as candidate for stable tree inclusion.
> >
> > Available in the "topic/ipoib-fixes-4.11" topic branch of this git
> > repo:
> > git://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git
> >
> > Or for browsing:
> > https://git.kernel.org/cgit/linux/kernel/git/leon/linux-rdma.git/log/
> > ?h=topic/ipoib-fixes-4.11
> >
> > Thanks
> >
> > Changelog v0 -> v1:
> >  * Patch 2,5,6 - add Yuval Shaia's ROB tags
> >  * Patch 7 - remove __func__ prints
> >
> > Feras Daoud (9):
> >   IB/ipoib: Add warning message when changing the MTU in UD over the
> > max
> >     range
> >   IB/ipoib: Set device connection mode only when needed
>
> I fixed up both of these patches according to my comments.
>
> >   IB/ipoib: Fix deadlock over vlan_mutex
> >   IB/ipoib: Fix deadlock between rmmod and set_mode
> >   IB/ipoib: rtnl_unlock can not come after free_netdev
> >   IB/ipoib: Add detailed error message to dev_queue_xmit call
> >   IB/ipoib: Use debug prints instead of warnings in RNR WC status
> >   IB/ipoib: Replace list_del of the neigh->list with list_del_init
> >   IB/ipoib: Change list_del to list_del_init in the tx object
> >
> >  drivers/infiniband/ulp/ipoib/ipoib_cm.c        | 30 ++++++++++++++
> > ---------
> >  drivers/infiniband/ulp/ipoib/ipoib_main.c      | 34
> > +++++++++++++++++---------
> >  drivers/infiniband/ulp/ipoib/ipoib_multicast.c |  6 +++--
> >  drivers/infiniband/ulp/ipoib/ipoib_vlan.c      | 10 +++++---
> >  4 files changed, 52 insertions(+), 28 deletions(-)
>
> Thanks, series (after modifications) applied.

Thanks Doug.

>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG KeyID: B826A3330E572FDD
>    
> Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2039 bytes --]

On Thu, Jan 12, 2017 at 01:47:06PM -0500, Doug Ledford wrote:
> On Wed, 2016-12-28 at 14:47 +0200, Leon Romanovsky wrote:
> > From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > 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/w@public.gmane.org>
> > Signed-off-by: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Reviewed-by: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > index a550cc6..1787f6b 100644
> > --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > @@ -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");
>
> There is no need for any message here.  Nothing unexpected is
> happening.

Right,
Thank you fixing it.

>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG KeyID: B826A3330E572FDD
>    
> Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 3075 bytes --]

On Thu, Jan 12, 2017 at 01:46:16PM -0500, Doug Ledford wrote:
> On Wed, 2016-12-28 at 14:47 +0200, Leon Romanovsky wrote:
> > From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > In datagram mode, the IB UD (Unreliable Datagram) transport is used
> > so the MTU of the interface is equal to the IB L2 MTU minus the
> > IPoIB encapsulation header. Any request to change the MTU value
> > above the maximum range will change the MTU to the max allowed, but
> > will not show any warning message. An ipoib_warn is issued in such
> > cases, letting the user know that even though the value is legal,
> > it can't be currently applied.
> >
> > Signed-off-by: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Noa Osherovich <noaos-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > index 3ce0765..a550cc6 100644
> > --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > @@ -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;
>
> I don't like this patch.  First, there's no need for a warning here.
>  That's entirely too noisy for this issue.  Second, the wording of the
> message is poor.  The user thinks they are setting the MTU, and there
> is no means of setting a multicast MTU, so telling them that their new
> MTU must be less than the mcast MTU that they can't do anything about
> and don't necessarily know how it is generated makes no sense.  This
> should be no more than ipoib_dbg if we even print anything out at all,
> and the message should be more like "MTU must be <= the link layer MTU
> - 4, use ibv_devinfo on the RDMA device to get the link layer MTU"

First of all, thank you for fixing wording, for me it is the hardest
part of every commit.

Second, I have a different view from you on the issue. User configured
some value, which is not correct for IPoIB. In ideal world (without legacy),
we were supposed to return error to him with proper message, but in our
case (legacy applications) we can't (we tried and it broke some legacy
ifcongfig, if I remember well). So it leaves us with one available
option is to warn user about improper value.

User should know that he supplied wrong parameter.

>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG KeyID: B826A3330E572FDD
>    
> Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Jason Gunthorpe]

On Thu, Jan 12, 2017 at 09:35:08PM +0200, Leon Romanovsky wrote:

> > > In datagram mode, the IB UD (Unreliable Datagram) transport is used
> > > so the MTU of the interface is equal to the IB L2 MTU minus the
> > > IPoIB encapsulation header. Any request to change the MTU value
> > > above the maximum range will change the MTU to the max allowed, but
> > > will not show any warning message. An ipoib_warn is issued in such
> > > cases, letting the user know that even though the value is legal,
> > > it can't be currently applied.

How does RC mode work then?

> Second, I have a different view from you on the issue. User configured
> some value, which is not correct for IPoIB. In ideal world (without legacy),
> we were supposed to return error to him with proper message, but in our
> case (legacy applications) we can't (we tried and it broke some legacy
> ifcongfig, if I remember well). So it leaves us with one available
> option is to warn user about improper value.

This is a legitimate configuration though, the user could have a mixed
MTU fabric and rely on path MTU discovery, so sets the MTU to the
largest.

AFAIK the way to handle such configurations is with a multicast route that
has a reduced mtu.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2069 bytes --]

On Thu, 2017-01-12 at 21:35 +0200, Leon Romanovsky wrote:
> 
> First of all, thank you for fixing wording, for me it is the hardest
> part of every commit.

No worries.

> Second, I have a different view from you on the issue. User
> configured
> some value, which is not correct for IPoIB. In ideal world (without
> legacy),
> we were supposed to return error to him with proper message,

If you set an impossible MTU on an ethernet adapter, you get a return
of EINVAL.  But it doesn't mess with the kernel log at all, it's *just*
EINVAL to the calling program.

>  but in our
> case (legacy applications) we can't (we tried and it broke some
> legacy
> ifcongfig, if I remember well).

ifconfig is what I used to test what Ethernet does, so it should be
well and truly used to EINVAL as a return.

>  So it leaves us with one available
> option is to warn user about improper value.

Even if you want to alert the user, there is no need to warn.  Warn is
reserved for something that is a serious error or condition, this is an
EINVAL of something that is, at best, a performance issue.  Unless path
MTU support is broken, failing to set a larger MTU will not ever hinder
actual operation.  So we should never be dumping out KERN_WARN messages
into the kernel log that will ping up on any root login session as well
as clutter the kernel dmesg output.

> User should know that he supplied wrong parameter.

User can still find out, they just won't get proactive pings on all
root sessions, instead they will have to look in the dmesg output
because they are trying to figure out why their command didn't work.

> > 
> > 
> > --
> > Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >     GPG KeyID: B826A3330E572FDD
> >    
> > Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57
> > 2FDD
> 
> 
-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1636 bytes --]

On Thu, Jan 12, 2017 at 01:16:22PM -0700, Jason Gunthorpe wrote:
> On Thu, Jan 12, 2017 at 09:35:08PM +0200, Leon Romanovsky wrote:
>
> > > > In datagram mode, the IB UD (Unreliable Datagram) transport is used
> > > > so the MTU of the interface is equal to the IB L2 MTU minus the
> > > > IPoIB encapsulation header. Any request to change the MTU value
> > > > above the maximum range will change the MTU to the max allowed, but
> > > > will not show any warning message. An ipoib_warn is issued in such
> > > > cases, letting the user know that even though the value is legal,
> > > > it can't be currently applied.
>
> How does RC mode work then?

RC mode doesn't have limitation on MTU size and it allows messages
larger than IB link-layer MTU.

>
> > Second, I have a different view from you on the issue. User configured
> > some value, which is not correct for IPoIB. In ideal world (without legacy),
> > we were supposed to return error to him with proper message, but in our
> > case (legacy applications) we can't (we tried and it broke some legacy
> > ifcongfig, if I remember well). So it leaves us with one available
> > option is to warn user about improper value.
>
> This is a legitimate configuration though, the user could have a mixed
> MTU fabric and rely on path MTU discovery, so sets the MTU to the
> largest.
>
> AFAIK the way to handle such configurations is with a multicast route that
> has a reduced mtu.

Multicast traffic is sent in datagram mode hence the limit will imply.

Maybe, I went too far by calling it error, but IMHO absence of indication to
user of decreased MTU is not good either.

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2308 bytes --]

On Thu, Jan 12, 2017 at 04:58:28PM -0500, Doug Ledford wrote:
> On Thu, 2017-01-12 at 21:35 +0200, Leon Romanovsky wrote:
> > 
> > First of all, thank you for fixing wording, for me it is the hardest
> > part of every commit.
>
> No worries.
>
> > Second, I have a different view from you on the issue. User
> > configured
> > some value, which is not correct for IPoIB. In ideal world (without
> > legacy),
> > we were supposed to return error to him with proper message,
>
> If you set an impossible MTU on an ethernet adapter, you get a return
> of EINVAL.  But it doesn't mess with the kernel log at all, it's *just*
> EINVAL to the calling program.
>
> >  but in our
> > case (legacy applications) we can't (we tried and it broke some
> > legacy
> > ifcongfig, if I remember well).
>
> ifconfig is what I used to test what Ethernet does, so it should be
> well and truly used to EINVAL as a return.

I'll recheck with Feras next week which legacy tool
didn't work and will post it.

>
> >  So it leaves us with one available
> > option is to warn user about improper value.
>
> Even if you want to alert the user, there is no need to warn.  Warn is
> reserved for something that is a serious error or condition, this is an
> EINVAL of something that is, at best, a performance issue.  Unless path
> MTU support is broken, failing to set a larger MTU will not ever hinder
> actual operation.  So we should never be dumping out KERN_WARN messages
> into the kernel log that will ping up on any root login session as well
> as clutter the kernel dmesg output.
>
> > User should know that he supplied wrong parameter.
>
> User can still find out, they just won't get proactive pings on all
> root sessions, instead they will have to look in the dmesg output
> because they are trying to figure out why their command didn't work.
>
> > >
> > >
> > > --
> > > Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > >     GPG KeyID: B826A3330E572FDD
> > >    
> > > Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57
> > > 2FDD
> >
> >
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG KeyID: B826A3330E572FDD
>    
> Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Jason Gunthorpe]

On Fri, Jan 13, 2017 at 05:08:24PM +0200, Leon Romanovsky wrote:
> On Thu, Jan 12, 2017 at 01:16:22PM -0700, Jason Gunthorpe wrote:
> > On Thu, Jan 12, 2017 at 09:35:08PM +0200, Leon Romanovsky wrote:
> >
> > > > > In datagram mode, the IB UD (Unreliable Datagram) transport is used
> > > > > so the MTU of the interface is equal to the IB L2 MTU minus the
> > > > > IPoIB encapsulation header. Any request to change the MTU value
> > > > > above the maximum range will change the MTU to the max allowed, but
> > > > > will not show any warning message. An ipoib_warn is issued in such
> > > > > cases, letting the user know that even though the value is legal,
> > > > > it can't be currently applied.
> >
> > How does RC mode work then?
> 
> RC mode doesn't have limitation on MTU size and it allows messages
> larger than IB link-layer MTU.

This is about what happens if the multicast MTU < unicast MTU - which
is *exactly* the same case on RC and UD.

IPoIB cannot send a 64k multicast MTU on RC either.

> Maybe, I went too far by calling it error, but IMHO absence of indication to
> user of decreased MTU is not good either.

I don't see why we should 'solve' this for UD and ignore the same
problem in RC.

IMHO, the correct fix is to figure out how to limit multicast - there
is something called multicast path MTU discovery - so maybe we are
doing something wrong and breaking that (are we erroring too large
packets inside ipoib properly?), or it is not working in the kernel or
it isn't supported by the kernel ...

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

On Fri, Jan 13, 2017 at 10:12:34AM -0700, Jason Gunthorpe wrote:
> On Fri, Jan 13, 2017 at 05:08:24PM +0200, Leon Romanovsky wrote:
> > On Thu, Jan 12, 2017 at 01:16:22PM -0700, Jason Gunthorpe wrote:
> > > On Thu, Jan 12, 2017 at 09:35:08PM +0200, Leon Romanovsky wrote:
> > >
> > > > > > In datagram mode, the IB UD (Unreliable Datagram) transport is used
> > > > > > so the MTU of the interface is equal to the IB L2 MTU minus the
> > > > > > IPoIB encapsulation header. Any request to change the MTU value
> > > > > > above the maximum range will change the MTU to the max allowed, but
> > > > > > will not show any warning message. An ipoib_warn is issued in such
> > > > > > cases, letting the user know that even though the value is legal,
> > > > > > it can't be currently applied.
> > >
> > > How does RC mode work then?
> >
> > RC mode doesn't have limitation on MTU size and it allows messages
> > larger than IB link-layer MTU.
>
> This is about what happens if the multicast MTU < unicast MTU - which
> is *exactly* the same case on RC and UD.
>
> IPoIB cannot send a 64k multicast MTU on RC either.

Multicast is sent in datagram despite being in connected mode and for
datagram the MTU is limited by IB

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Jason Gunthorpe]

On Fri, Jan 13, 2017 at 10:31:27PM +0200, Leon Romanovsky wrote:

> > This is about what happens if the multicast MTU < unicast MTU - which
> > is *exactly* the same case on RC and UD.
> >
> > IPoIB cannot send a 64k multicast MTU on RC either.
> 
> Multicast is sent in datagram despite being in connected mode and for
> datagram the MTU is limited by IB

I am aware of that. Read you patch again:

> +	if (priv->mcast_mtu < priv->admin_mtu)
> +		ipoib_warn(priv, "MTU must be smaller than mcast_mtu (%u)\n",

This check is fails in RC mode as well.

And we already check if the requested MTU is beyond the port
capability:

+        if (new_mtu > IPOIB_UD_MTU(priv->max_ib_mtu))
+                return -EINVAL;

So I have *no idea* why you'd want to check it against the multicast
group too..

It is OK for the multicast group to have a smaller MTU than the
unicast side. This is how RC operates after all.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1302 bytes --]

On Fri, Jan 13, 2017 at 02:27:21PM -0700, Jason Gunthorpe wrote:
> On Fri, Jan 13, 2017 at 10:31:27PM +0200, Leon Romanovsky wrote:
>
> > > This is about what happens if the multicast MTU < unicast MTU - which
> > > is *exactly* the same case on RC and UD.
> > >
> > > IPoIB cannot send a 64k multicast MTU on RC either.
> >
> > Multicast is sent in datagram despite being in connected mode and for
> > datagram the MTU is limited by IB
>
> I am aware of that. Read you patch again:
>
> > +	if (priv->mcast_mtu < priv->admin_mtu)
> > +		ipoib_warn(priv, "MTU must be smaller than mcast_mtu (%u)\n",
>
> This check is fails in RC mode as well.
>
> And we already check if the requested MTU is beyond the port
> capability:
>
> +        if (new_mtu > IPOIB_UD_MTU(priv->max_ib_mtu))
> +                return -EINVAL;
>
> So I have *no idea* why you'd want to check it against the multicast
> group too..
>

Port MTU capability can be higher than link MTU. In this flow, the user
can try to set manually MTU which will be higher than SM provided.

Immediately after that print, we have a following piece of code:
 236         dev->mtu = min(priv->mcast_mtu, priv->admin_mtu);

> It is OK for the multicast group to have a smaller MTU than the
> unicast side. This is how RC operates after all.
>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Jason Gunthorpe]

On Sun, Jan 15, 2017 at 10:35:43AM +0200, Leon Romanovsky wrote:
> On Fri, Jan 13, 2017 at 02:27:21PM -0700, Jason Gunthorpe wrote:
> > On Fri, Jan 13, 2017 at 10:31:27PM +0200, Leon Romanovsky wrote:
> >
> > > > This is about what happens if the multicast MTU < unicast MTU - which
> > > > is *exactly* the same case on RC and UD.
> > > >
> > > > IPoIB cannot send a 64k multicast MTU on RC either.
> > >
> > > Multicast is sent in datagram despite being in connected mode and for
> > > datagram the MTU is limited by IB
> >
> > I am aware of that. Read you patch again:
> >
> > > +	if (priv->mcast_mtu < priv->admin_mtu)
> > > +		ipoib_warn(priv, "MTU must be smaller than mcast_mtu (%u)\n",
> >
> > This check is fails in RC mode as well.
> >
> > And we already check if the requested MTU is beyond the port
> > capability:
> >
> > +        if (new_mtu > IPOIB_UD_MTU(priv->max_ib_mtu))
> > +                return -EINVAL;
> >
> > So I have *no idea* why you'd want to check it against the multicast
> > group too..
> >
> 
> Port MTU capability can be higher than link MTU. In this flow, the user
> can try to set manually MTU which will be higher than SM provided.
> 
> Immediately after that print, we have a following piece of code:
>  236         dev->mtu = min(priv->mcast_mtu, priv->admin_mtu);

Maybe the better fix is to take that min out than generate a
warning.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1591 bytes --]

On Mon, Jan 16, 2017 at 01:12:18PM -0700, Jason Gunthorpe wrote:
> On Sun, Jan 15, 2017 at 10:35:43AM +0200, Leon Romanovsky wrote:
> > On Fri, Jan 13, 2017 at 02:27:21PM -0700, Jason Gunthorpe wrote:
> > > On Fri, Jan 13, 2017 at 10:31:27PM +0200, Leon Romanovsky wrote:
> > >
> > > > > This is about what happens if the multicast MTU < unicast MTU - which
> > > > > is *exactly* the same case on RC and UD.
> > > > >
> > > > > IPoIB cannot send a 64k multicast MTU on RC either.
> > > >
> > > > Multicast is sent in datagram despite being in connected mode and for
> > > > datagram the MTU is limited by IB
> > >
> > > I am aware of that. Read you patch again:
> > >
> > > > +	if (priv->mcast_mtu < priv->admin_mtu)
> > > > +		ipoib_warn(priv, "MTU must be smaller than mcast_mtu (%u)\n",
> > >
> > > This check is fails in RC mode as well.
> > >
> > > And we already check if the requested MTU is beyond the port
> > > capability:
> > >
> > > +        if (new_mtu > IPOIB_UD_MTU(priv->max_ib_mtu))
> > > +                return -EINVAL;
> > >
> > > So I have *no idea* why you'd want to check it against the multicast
> > > group too..
> > >
> >
> > Port MTU capability can be higher than link MTU. In this flow, the user
> > can try to set manually MTU which will be higher than SM provided.
> >
> > Immediately after that print, we have a following piece of code:
> >  236         dev->mtu = min(priv->mcast_mtu, priv->admin_mtu);
>
> Maybe the better fix is to take that min out than generate a
> warning.

It looks like that I need more time to grasp it.
Sorry about that.

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2634 bytes --]

On Fri, Jan 13, 2017 at 05:15:07PM +0200, Leon Romanovsky wrote:
> On Thu, Jan 12, 2017 at 04:58:28PM -0500, Doug Ledford wrote:
> > On Thu, 2017-01-12 at 21:35 +0200, Leon Romanovsky wrote:
> > > 
> > > First of all, thank you for fixing wording, for me it is the hardest
> > > part of every commit.
> >
> > No worries.
> >
> > > Second, I have a different view from you on the issue. User
> > > configured
> > > some value, which is not correct for IPoIB. In ideal world (without
> > > legacy),
> > > we were supposed to return error to him with proper message,
> >
> > If you set an impossible MTU on an ethernet adapter, you get a return
> > of EINVAL.  But it doesn't mess with the kernel log at all, it's *just*
> > EINVAL to the calling program.
> >
> > >  but in our
> > > case (legacy applications) we can't (we tried and it broke some
> > > legacy
> > > ifcongfig, if I remember well).
> >
> > ifconfig is what I used to test what Ethernet does, so it should be
> > well and truly used to EINVAL as a return.
>
> I'll recheck with Feras next week which legacy tool
> didn't work and will post it.

At the end, it was ethtool and according to Jason's comments it was
correct thing do not to put return value, so we are fine.

>
> >
> > >  So it leaves us with one available
> > > option is to warn user about improper value.
> >
> > Even if you want to alert the user, there is no need to warn.  Warn is
> > reserved for something that is a serious error or condition, this is an
> > EINVAL of something that is, at best, a performance issue.  Unless path
> > MTU support is broken, failing to set a larger MTU will not ever hinder
> > actual operation.  So we should never be dumping out KERN_WARN messages
> > into the kernel log that will ping up on any root login session as well
> > as clutter the kernel dmesg output.
> >
> > > User should know that he supplied wrong parameter.
> >
> > User can still find out, they just won't get proactive pings on all
> > root sessions, instead they will have to look in the dmesg output
> > because they are trying to figure out why their command didn't work.
> >
> > > >
> > > >
> > > > --
> > > > Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > >     GPG KeyID: B826A3330E572FDD
> > > >    
> > > > Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57
> > > > 2FDD
> > >
> > >
> > --
> > Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >     GPG KeyID: B826A3330E572FDD
> >    
> > Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
>
>



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1759 bytes --]

On Mon, Jan 16, 2017 at 01:12:18PM -0700, Jason Gunthorpe wrote:
> On Sun, Jan 15, 2017 at 10:35:43AM +0200, Leon Romanovsky wrote:
> > On Fri, Jan 13, 2017 at 02:27:21PM -0700, Jason Gunthorpe wrote:
> > > On Fri, Jan 13, 2017 at 10:31:27PM +0200, Leon Romanovsky wrote:
> > >
> > > > > This is about what happens if the multicast MTU < unicast MTU - which
> > > > > is *exactly* the same case on RC and UD.
> > > > >
> > > > > IPoIB cannot send a 64k multicast MTU on RC either.
> > > >
> > > > Multicast is sent in datagram despite being in connected mode and for
> > > > datagram the MTU is limited by IB
> > >
> > > I am aware of that. Read you patch again:
> > >
> > > > +	if (priv->mcast_mtu < priv->admin_mtu)
> > > > +		ipoib_warn(priv, "MTU must be smaller than mcast_mtu (%u)\n",
> > >
> > > This check is fails in RC mode as well.
> > >
> > > And we already check if the requested MTU is beyond the port
> > > capability:
> > >
> > > +        if (new_mtu > IPOIB_UD_MTU(priv->max_ib_mtu))
> > > +                return -EINVAL;
> > >
> > > So I have *no idea* why you'd want to check it against the multicast
> > > group too..
> > >
> >
> > Port MTU capability can be higher than link MTU. In this flow, the user
> > can try to set manually MTU which will be higher than SM provided.
> >
> > Immediately after that print, we have a following piece of code:
> >  236         dev->mtu = min(priv->mcast_mtu, priv->admin_mtu);
>
> Maybe the better fix is to take that min out than generate a
> warning.

I agree with you, in theory it should work.

Let's hope that we will find such brave man who will
remove this line. The removal of this one line will
require from him to do extensive testing and reviewing
different code paths.

Thanks

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Jason Gunthorpe]

On Thu, Jan 19, 2017 at 03:12:58PM +0200, Leon Romanovsky wrote:

> Let's hope that we will find such brave man who will
> remove this line. The removal of this one line will
> require from him to do extensive testing and reviewing
> different code paths.

Why? We already fully support this configuration for RC

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 765 bytes --]

On Thu, Jan 19, 2017 at 10:33:52AM -0700, Jason Gunthorpe wrote:
> On Thu, Jan 19, 2017 at 03:12:58PM +0200, Leon Romanovsky wrote:
>
> > Let's hope that we will find such brave man who will
> > remove this line. The removal of this one line will
> > require from him to do extensive testing and reviewing
> > different code paths.
>
> Why? We already fully support this configuration for RC

At minimum, i see here one new path which should be tested.
Configure small MTU -> Join multicast group -> Configure large MTU

Before removing min(..), in third step, we ensured that MTU won't exceed
multicast MTU in all flows.

After removing min(..), you will have a chance of flows there MTU limit
won't work.

And I just saying that it needs to be tested.

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]