[PATCH rdma-rc 0/7] IPoIB and IB core fixes for 4.7

[PATCH rdma-rc 0/7] IPoIB and IB core fixes for 4.7

[Leon Romanovsky]

Hi Doug,

Please find below the patches with fixes for IPoIB and IB core.

This patchset is generated against Linus's v4.7-rc1 tag.

Available in the "topic/fixes-ib" 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/fixes-ib

Thanks

Aviv Heller (1):
  IB/core: Fix removal of default GID cache entry

Eli Cohen (1):
  IB/core: Fix query port failure in RoCE

Erez Shitrit (2):
  IB/IPoIB: Don't update neigh validity for unresolved entries
  IB/IPoIB: Fix race between ipoib_remove_one to sysfs functions

Mark Bloch (3):
  IB/IPoIB: Disable bottom half when dealing with device address
  IB/core: Fix incorrect array allocation
  IB/core: Initialize sysfs attributes before sysfs create group

 drivers/infiniband/core/cache.c                | 10 ++++++++--
 drivers/infiniband/core/device.c               |  3 +++
 drivers/infiniband/core/sysfs.c                | 10 ++++++++--
 drivers/infiniband/ulp/ipoib/ipoib.h           |  1 +
 drivers/infiniband/ulp/ipoib/ipoib_cm.c        |  4 ++++
 drivers/infiniband/ulp/ipoib/ipoib_ib.c        |  8 ++++----
 drivers/infiniband/ulp/ipoib/ipoib_main.c      | 15 ++++++++++-----
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c |  6 +++---
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c      |  6 ++++++
 9 files changed, 47 insertions(+), 16 deletions(-)

-- 
2.1.4

--
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-rc 1/7] IB/core: Fix query port failure in RoCE

[Leon Romanovsky]

From: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Currently ib_query_port always attempts to to read the subnet prefix by
calling ib_query_gid(). For RoCE there is no subnet manager and no
subnet prefix. Fix this by querying GID[0] only for IB networks.

Fixes: fad61ad4e755 ('IB/core: Add subnet prefix to port info')
Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 5516fb0..05e25a3 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -661,6 +661,9 @@ int ib_query_port(struct ib_device *device,
 	if (err || port_attr->subnet_prefix)
 		return err;
 
+	if (rdma_port_get_link_layer(device, port_num) != IB_LINK_LAYER_INFINIBAND)
+		return 0;
+
 	err = ib_query_gid(device, port_num, 0, &gid, NULL);
 	if (err)
 		return err;
-- 
2.1.4

--
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-rc 2/7] IB/IPoIB: Don't update neigh validity for unresolved entries

[Leon Romanovsky]

From: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

ipoib_neigh_get whenever tries to access specific neigh updates its
"alive" variable member, the neigh now considered as updated and won't
be cleaned by the GC process even if it needs to be resolved again.

Fixes: b63b70d87741 ("IPoIB: Use a private hash table for path lookup in xmit path")
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_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 2d7c163..c558c32 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1206,7 +1206,9 @@ struct ipoib_neigh *ipoib_neigh_get(struct net_device *dev, u8 *daddr)
 				neigh = NULL;
 				goto out_unlock;
 			}
-			neigh->alive = jiffies;
+
+			if (likely(skb_queue_len(&neigh->queue) < IPOIB_MAX_PATH_REC_QUEUE))
+				neigh->alive = jiffies;
 			goto out_unlock;
 		}
 	}
-- 
2.1.4

--
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-rc 3/7] IB/IPoIB: Fix race between ipoib_remove_one to sysfs functions

[Leon Romanovsky]

From: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

In ipoib_remove_one the driver holds the rtnl_lock and tries to do some
operation like dev_change_flags or unregister_netdev, while sysfs
callback like ipoib_vlan_delete holds sysfs mutex and tries to hold the
rtnl_lock via rtnl_trylock() and restart_syscall() if the lock is not
free, meanwhile ipoib_remove_one tries to get the sysfs lock in order to
free its sysfs directory, and we will get  a->b, b->a deadlock.

    Trace like the following:

        schedule+0x37/0x80
        schedule_preempt_disabled+0xe/0x10
        __mutex_lock_slowpath+0xb5/0x120
        mutex_lock+0x23/0x40
        rtnl_lock+0x15/0x20
        netdev_run_todo+0x17c/0x320
        rtnl_unlock+0xe/0x10
        ipoib_vlan_delete+0x11b/0x1b0 [ib_ipoib]
        delete_child+0x54/0x80 [ib_ipoib]
        dev_attr_store+0x18/0x30
        sysfs_kf_write+0x37/0x40
        mutex_lock+0x16/0x40
        SyS_write+0x55/0xc0
        entry_SYSCALL_64_fastpath+0x16/0x75
    And
        schedule+0x37/0x80
        __kernfs_remove+0x1a8/0x260
        ? wake_atomic_t_function+0x60/0x60
        kernfs_remove+0x25/0x40
        sysfs_remove_dir+0x50/0x80
        kobject_del+0x18/0x50
        device_del+0x19f/0x260
        netdev_unregister_kobject+0x6a/0x80
        rollback_registered_many+0x1fd/0x340
        rollback_registered+0x3c/0x70
        unregister_netdevice_queue+0x55/0xc0
        unregister_netdev+0x20/0x30
        ipoib_remove_one+0x114/0x1b0 [ib_ipoib]
        ib_unregister_client+0x4a/0x170 [ib_core]
        ? find_module_all+0x71/0xa0
        ipoib_cleanup_module+0x10/0x94 [ib_ipoib]
        SyS_delete_module+0x1b5/0x210
        entry_SYSCALL_64_fastpath+0x16/0x75

The fix is by checking the flag IPOIB_FLAG_INTF_ON_DESTROY in order to
get out from the sysfs function.

Fixes: 862096a8bbf8 ("IB/ipoib: Add more rtnl_link_ops callbacks")
Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
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.h      | 1 +
 drivers/infiniband/ulp/ipoib/ipoib_cm.c   | 4 ++++
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 3 +++
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 6 ++++++
 4 files changed, 14 insertions(+)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h
index bab7db6..4f7d9b4 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib.h
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h
@@ -94,6 +94,7 @@ enum {
 	IPOIB_NEIGH_TBL_FLUSH	  = 12,
 	IPOIB_FLAG_DEV_ADDR_SET	  = 13,
 	IPOIB_FLAG_DEV_ADDR_CTRL  = 14,
+	IPOIB_FLAG_GOING_DOWN	  = 15,
 
 	IPOIB_MAX_BACKOFF_SECONDS = 16,
 
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index b2f4283..951d9ab 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -1486,6 +1486,10 @@ static ssize_t set_mode(struct device *d, struct device_attribute *attr,
 {
 	struct net_device *dev = to_net_dev(d);
 	int ret;
+	struct ipoib_dev_priv *priv = netdev_priv(dev);
+
+	if (test_bit(IPOIB_FLAG_GOING_DOWN, &priv->flags))
+		return -EPERM;
 
 	if (!rtnl_trylock())
 		return restart_syscall();
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index c558c32..1cd569f 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -2143,6 +2143,9 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
 		ib_unregister_event_handler(&priv->event_handler);
 		flush_workqueue(ipoib_workqueue);
 
+		/* mark interface in the middle of destruction */
+		set_bit(IPOIB_FLAG_GOING_DOWN, &priv->flags);
+
 		rtnl_lock();
 		dev_change_flags(priv->dev, priv->dev->flags & ~IFF_UP);
 		rtnl_unlock();
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index 64a3559..a2f9f29 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -131,6 +131,9 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
 
 	ppriv = netdev_priv(pdev);
 
+	if (test_bit(IPOIB_FLAG_GOING_DOWN, &ppriv->flags))
+		return -EPERM;
+
 	snprintf(intf_name, sizeof intf_name, "%s.%04x",
 		 ppriv->dev->name, pkey);
 	priv = ipoib_intf_alloc(intf_name);
@@ -183,6 +186,9 @@ int ipoib_vlan_delete(struct net_device *pdev, unsigned short pkey)
 
 	ppriv = netdev_priv(pdev);
 
+	if (test_bit(IPOIB_FLAG_GOING_DOWN, &ppriv->flags))
+		return -EPERM;
+
 	if (!rtnl_trylock())
 		return restart_syscall();
 
-- 
2.1.4

--
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-rc 4/7] IB/core: Fix removal of default GID cache entry

[Leon Romanovsky]

From: Aviv Heller <avivh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

When deleting a default GID from the cache, its gid_type field is set
to 0.

This could set the gid_type to RoCE v1 for a RoCE v2 default GID,
essentially making it inaccessible to future modifications, since it
is no longer found by find_gid().

This fix preserves the gid_type value for default gids during cache
operations.

Fixes: b39ffa1df505 ('IB/core: Add gid_type to gid attribute')
Signed-off-by: Aviv Heller <avivh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/cache.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index c2e257d..0409667 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -178,6 +178,7 @@ static int write_gid(struct ib_device *ib_dev, u8 port,
 {
 	int ret = 0;
 	struct net_device *old_net_dev;
+	enum ib_gid_type old_gid_type;
 
 	/* in rdma_cap_roce_gid_table, this funciton should be protected by a
 	 * sleep-able lock.
@@ -199,6 +200,7 @@ static int write_gid(struct ib_device *ib_dev, u8 port,
 	}
 
 	old_net_dev = table->data_vec[ix].attr.ndev;
+	old_gid_type = table->data_vec[ix].attr.gid_type;
 	if (old_net_dev && old_net_dev != attr->ndev)
 		dev_put(old_net_dev);
 	/* if modify_gid failed, just delete the old gid */
@@ -207,10 +209,14 @@ static int write_gid(struct ib_device *ib_dev, u8 port,
 		attr = &zattr;
 		table->data_vec[ix].context = NULL;
 	}
-	if (default_gid)
-		table->data_vec[ix].props |= GID_TABLE_ENTRY_DEFAULT;
+
 	memcpy(&table->data_vec[ix].gid, gid, sizeof(*gid));
 	memcpy(&table->data_vec[ix].attr, attr, sizeof(*attr));
+	if (default_gid) {
+		table->data_vec[ix].props |= GID_TABLE_ENTRY_DEFAULT;
+		if (action == GID_TABLE_WRITE_ACTION_DEL)
+			table->data_vec[ix].attr.gid_type = old_gid_type;
+	}
 	if (table->data_vec[ix].attr.ndev &&
 	    table->data_vec[ix].attr.ndev != old_net_dev)
 		dev_hold(table->data_vec[ix].attr.ndev);
-- 
2.1.4

--
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-rc 5/7] IB/IPoIB: Disable bottom half when dealing with device address

[Leon Romanovsky]

From: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Align locking usage when touching device address with rest
of the kernel. Lock the bottom half when doing so using
netif_addr_lock_bh.

This also solves the following case as reported by lockdep:
	CPU0                    CPU1
	----                    ----
lock(_xmit_INFINIBAND);
				local_irq_disable();
				lock(&(&mc->mca_lock)->rlock);
				lock(_xmit_INFINIBAND);
<Interrupt>
lock(&(&mc->mca_lock)->rlock);

*** DEADLOCK ***

Fixes: 492a7e67ff83 ("IB/IPoIB: Allow setting the device address")
Signed-off-by: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_ib.c        | 8 ++++----
 drivers/infiniband/ulp/ipoib/ipoib_main.c      | 8 ++++----
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 6 +++---
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ib.c b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
index 45c40a1..dc6d241 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_ib.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_ib.c
@@ -1015,7 +1015,7 @@ static bool ipoib_dev_addr_changed_valid(struct ipoib_dev_priv *priv)
 	if (ib_query_gid(priv->ca, priv->port, 0, &gid0, NULL))
 		return false;
 
-	netif_addr_lock(priv->dev);
+	netif_addr_lock_bh(priv->dev);
 
 	/* The subnet prefix may have changed, update it now so we won't have
 	 * to do it later
@@ -1026,12 +1026,12 @@ static bool ipoib_dev_addr_changed_valid(struct ipoib_dev_priv *priv)
 
 	search_gid.global.interface_id = priv->local_gid.global.interface_id;
 
-	netif_addr_unlock(priv->dev);
+	netif_addr_unlock_bh(priv->dev);
 
 	err = ib_find_gid(priv->ca, &search_gid, IB_GID_TYPE_IB,
 			  priv->dev, &port, &index);
 
-	netif_addr_lock(priv->dev);
+	netif_addr_lock_bh(priv->dev);
 
 	if (search_gid.global.interface_id !=
 	    priv->local_gid.global.interface_id)
@@ -1092,7 +1092,7 @@ static bool ipoib_dev_addr_changed_valid(struct ipoib_dev_priv *priv)
 	}
 
 out:
-	netif_addr_unlock(priv->dev);
+	netif_addr_unlock_bh(priv->dev);
 
 	return ret;
 }
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 1cd569f..5f58c41 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1853,7 +1853,7 @@ static void set_base_guid(struct ipoib_dev_priv *priv, union ib_gid *gid)
 	struct ipoib_dev_priv *child_priv;
 	struct net_device *netdev = priv->dev;
 
-	netif_addr_lock(netdev);
+	netif_addr_lock_bh(netdev);
 
 	memcpy(&priv->local_gid.global.interface_id,
 	       &gid->global.interface_id,
@@ -1861,7 +1861,7 @@ static void set_base_guid(struct ipoib_dev_priv *priv, union ib_gid *gid)
 	memcpy(netdev->dev_addr + 4, &priv->local_gid, sizeof(priv->local_gid));
 	clear_bit(IPOIB_FLAG_DEV_ADDR_SET, &priv->flags);
 
-	netif_addr_unlock(netdev);
+	netif_addr_unlock_bh(netdev);
 
 	if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) {
 		down_read(&priv->vlan_rwsem);
@@ -1877,7 +1877,7 @@ static int ipoib_check_lladdr(struct net_device *dev,
 	union ib_gid *gid = (union ib_gid *)(ss->__data + 4);
 	int ret = 0;
 
-	netif_addr_lock(dev);
+	netif_addr_lock_bh(dev);
 
 	/* Make sure the QPN, reserved and subnet prefix match the current
 	 * lladdr, it also makes sure the lladdr is unicast.
@@ -1887,7 +1887,7 @@ static int ipoib_check_lladdr(struct net_device *dev,
 	    gid->global.interface_id == 0)
 		ret = -EINVAL;
 
-	netif_addr_unlock(dev);
+	netif_addr_unlock_bh(dev);
 
 	return ret;
 }
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 82fbc94..d3394b6 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -582,13 +582,13 @@ void ipoib_mcast_join_task(struct work_struct *work)
 		return;
 	}
 	priv->local_lid = port_attr.lid;
-	netif_addr_lock(dev);
+	netif_addr_lock_bh(dev);
 
 	if (!test_bit(IPOIB_FLAG_DEV_ADDR_SET, &priv->flags)) {
-		netif_addr_unlock(dev);
+		netif_addr_unlock_bh(dev);
 		return;
 	}
-	netif_addr_unlock(dev);
+	netif_addr_unlock_bh(dev);
 
 	spin_lock_irq(&priv->lock);
 	if (!test_bit(IPOIB_FLAG_OPER_UP, &priv->flags))
-- 
2.1.4

--
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-rc 6/7] IB/core: Fix incorrect array allocation

[Leon Romanovsky]

From: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

In a attribute group struct, attrs should point to a NULL
terminated list. Which means we need to allocate one more
slot in the array.

Fixes: b40f4757daa1 ("IB/core: Make device counter infrastructure dynamic")
Signed-off-by: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/sysfs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
index 5e573bb..c68f132 100644
--- a/drivers/infiniband/core/sysfs.c
+++ b/drivers/infiniband/core/sysfs.c
@@ -902,8 +902,11 @@ static void setup_hw_stats(struct ib_device *device, struct ib_port *port,
 		goto err;
 
 	hsag = kzalloc(sizeof(*hsag) +
-		       // 1 extra for the lifespan config entry
-		       sizeof(void *) * (stats->num_counters + 1),
+		       /* 1 extra for the lifespan config entry,
+			* and 1 more because attrs should be
+			* NULL terminated.
+			*/
+		       sizeof(void *) * (stats->num_counters + 2),
 		       GFP_KERNEL);
 	if (!hsag)
 		return;
-- 
2.1.4

--
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-rc 7/7] IB/core: Initialize sysfs attributes before sysfs create group

[Leon Romanovsky]

From: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

For dynamically allocated sysfs attributes there is a need to call
sysfs_attr_init in order to comply with lockdep, not calling it
will result in error complaining key is not in .data section.

Fixes: b40f4757daa1 ("IB/core: Make device counter infrastructure dynamic")
Signed-off-by: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/sysfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
index c68f132..44b53e4 100644
--- a/drivers/infiniband/core/sysfs.c
+++ b/drivers/infiniband/core/sysfs.c
@@ -925,10 +925,13 @@ static void setup_hw_stats(struct ib_device *device, struct ib_port *port,
 		hsag->attrs[i] = alloc_hsa(i, port_num, stats->names[i]);
 		if (!hsag->attrs[i])
 			goto err;
+		sysfs_attr_init(hsag->attrs[i]);
 	}
 
 	/* treat an error here as non-fatal */
 	hsag->attrs[i] = alloc_hsa_lifespan("lifespan", port_num);
+	if (hsag->attrs[i])
+		sysfs_attr_init(hsag->attrs[i]);
 
 	if (port) {
 		struct kobject *kobj = &port->kobj;
-- 
2.1.4

--
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-rc 2/7] IB/IPoIB: Don't update neigh validity for unresolved entries

[Or Gerlitz]

On Sat, Jun 4, 2016 at 3:15 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> From: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>
> ipoib_neigh_get whenever tries to access specific neigh updates its
> "alive" variable member, the neigh now considered as updated and won't
> be cleaned by the GC process even if it needs to be resolved again.

Erez,

After reading the above few times, I think you wanted to say something like:

Whenever ipoib_neigh_get tries to access a specific neigh, it updates
the "alive" variable member. The neigh now considered as updated and
won't be cleaned by the GC process even if it needs to be resolved
again.

correct?





> Fixes: b63b70d87741 ("IPoIB: Use a private hash table for path lookup in xmit path")
> 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_main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index 2d7c163..c558c32 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -1206,7 +1206,9 @@ struct ipoib_neigh *ipoib_neigh_get(struct net_device *dev, u8 *daddr)
>                                 neigh = NULL;
>                                 goto out_unlock;
>                         }
> -                       neigh->alive = jiffies;
> +
> +                       if (likely(skb_queue_len(&neigh->queue) < IPOIB_MAX_PATH_REC_QUEUE))
> +                               neigh->alive = jiffies;

why testing the queue len makes things right?

>                         goto out_unlock;
>                 }
>         }
--
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-rc 1/7] IB/core: Fix query port failure in RoCE

[Steve Wise]

> 
> From: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> Currently ib_query_port always attempts to to read the subnet prefix by
> calling ib_query_gid(). For RoCE there is no subnet manager and no
> subnet prefix. Fix this by querying GID[0] only for IB networks.
> 
> Fixes: fad61ad4e755 ('IB/core: Add subnet prefix to port info')
> Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/infiniband/core/device.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/infiniband/core/device.c
b/drivers/infiniband/core/device.c
> index 5516fb0..05e25a3 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -661,6 +661,9 @@ int ib_query_port(struct ib_device *device,
>  	if (err || port_attr->subnet_prefix)
>  		return err;
> 
> +	if (rdma_port_get_link_layer(device, port_num) !=
> IB_LINK_LAYER_INFINIBAND)
> +		return 0;
> +
>  	err = ib_query_gid(device, port_num, 0, &gid, NULL);
>  	if (err)
>  		return err;

This change assumes IB_LINK_LAYER_ETHERNET == RoCE, which isn't true for iWARP
devices.  Still, the change is good for iWARP too, but the comment should
perhaps reflect this.

Reviewed-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>

Steve.

--
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-rc 6/7] IB/core: Fix incorrect array allocation

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1887 bytes --]

On 6/4/2016 8:15 AM, Leon Romanovsky wrote:
> From: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> In a attribute group struct, attrs should point to a NULL
> terminated list. Which means we need to allocate one more
> slot in the array.

This patch is both right and wrong.  You're right with the intent (that
there should be a total of 2 extra entries in the array size, one for
the NULL termination and one for the lifespan entry), but wrong with the
mechanics (unless I've missed something).  We already have two extra
entries because sizeof(*hsag) includes our first counter, so just
num_counters is actually enough to NULL terminate the list, and + 1
gives us lifespan plus a NULL terminate spot.  The comment could be
cleaned up to make this more clear though, so I'll do that.

> Fixes: b40f4757daa1 ("IB/core: Make device counter infrastructure dynamic")
> Signed-off-by: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/infiniband/core/sysfs.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
> index 5e573bb..c68f132 100644
> --- a/drivers/infiniband/core/sysfs.c
> +++ b/drivers/infiniband/core/sysfs.c
> @@ -902,8 +902,11 @@ static void setup_hw_stats(struct ib_device *device, struct ib_port *port,
>  		goto err;
>  
>  	hsag = kzalloc(sizeof(*hsag) +
> -		       // 1 extra for the lifespan config entry
> -		       sizeof(void *) * (stats->num_counters + 1),
> +		       /* 1 extra for the lifespan config entry,
> +			* and 1 more because attrs should be
> +			* NULL terminated.
> +			*/
> +		       sizeof(void *) * (stats->num_counters + 2),
>  		       GFP_KERNEL);
>  	if (!hsag)
>  		return;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-rc 2/7] IB/IPoIB: Don't update neigh validity for unresolved entries

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 647 bytes --]

On 6/5/2016 5:10 PM, Or Gerlitz wrote:

>> -                       neigh->alive = jiffies;
>> +
>> +                       if (likely(skb_queue_len(&neigh->queue) < IPOIB_MAX_PATH_REC_QUEUE))
>> +                               neigh->alive = jiffies;
> 
> why testing the queue len makes things right?

I'm with Or on this one.  This test doesn't make sense unless you assume
that the neighbor is being hit with a steady stream of packets.  If
someone just runs ping -c 1 to a dead neighbor, this test will fail.
The idea of the patch is OK, but the test needs to be reworked for
situations other than a blasting stream of data.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-rc 0/7] IPoIB and IB core fixes for 4.7

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1704 bytes --]

On 6/4/2016 8:15 AM, Leon Romanovsky wrote:
> Hi Doug,
> 
> Please find below the patches with fixes for IPoIB and IB core.
> 
> This patchset is generated against Linus's v4.7-rc1 tag.
> 
> Available in the "topic/fixes-ib" 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/fixes-ib
> 
> Thanks
> 
> Aviv Heller (1):
>   IB/core: Fix removal of default GID cache entry
> 
> Eli Cohen (1):
>   IB/core: Fix query port failure in RoCE
> 
> Erez Shitrit (2):
>   IB/IPoIB: Don't update neigh validity for unresolved entries
>   IB/IPoIB: Fix race between ipoib_remove_one to sysfs functions
> 
> Mark Bloch (3):
>   IB/IPoIB: Disable bottom half when dealing with device address
>   IB/core: Fix incorrect array allocation
>   IB/core: Initialize sysfs attributes before sysfs create group
> 
>  drivers/infiniband/core/cache.c                | 10 ++++++++--
>  drivers/infiniband/core/device.c               |  3 +++
>  drivers/infiniband/core/sysfs.c                | 10 ++++++++--
>  drivers/infiniband/ulp/ipoib/ipoib.h           |  1 +
>  drivers/infiniband/ulp/ipoib/ipoib_cm.c        |  4 ++++
>  drivers/infiniband/ulp/ipoib/ipoib_ib.c        |  8 ++++----
>  drivers/infiniband/ulp/ipoib/ipoib_main.c      | 15 ++++++++++-----
>  drivers/infiniband/ulp/ipoib/ipoib_multicast.c |  6 +++---
>  drivers/infiniband/ulp/ipoib/ipoib_vlan.c      |  6 ++++++
>  9 files changed, 47 insertions(+), 16 deletions(-)
> 

Thanks Leon, I applied 5 of the 7 (the two rejected patches were replied
to individually).


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-rc 2/7] IB/IPoIB: Don't update neigh validity for unresolved entries

[Erez Shitrit]

On Tue, Jun 7, 2016 at 2:59 AM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 6/5/2016 5:10 PM, Or Gerlitz wrote:
>
>>> -                       neigh->alive = jiffies;
>>> +
>>> +                       if (likely(skb_queue_len(&neigh->queue) < IPOIB_MAX_PATH_REC_QUEUE))
>>> +                               neigh->alive = jiffies;
>>
>> why testing the queue len makes things right?
>
> I'm with Or on this one.  This test doesn't make sense unless you assume
> that the neighbor is being hit with a steady stream of packets.  If
> someone just runs ping -c 1 to a dead neighbor, this test will fail.
> The idea of the patch is OK, but the test needs to be reworked for
> situations other than a blasting stream of data.
>

I will try to explain the idea behind that test, and why it works (I
hope I'll make it clear this time :))

there are 2 objects that are taking place here, the garbage-collector
that removes neigh if was not used for defined time, and the
path-query for each neigh.
if the path-query failed the packets for specific neigh are kept in
the neigh queue, but only if there are no more than
IPOIB_MAX_PATH_REC_QUEUE, and the neigh is still kept for ever (let's
assume CM connected that was stopped without any notification with
DREQ etc.)
The only way to know that there is an unresolved neigh is by checking
its queue, if it is full we can assume that this is an unresolved
neigh otherwise it is resolved.
So, the test doesn't take any assumption on the shape of the traffic,
even in a ping requests once a second it will failed after 3 times the
request was sent, and the neigh time will not be updated which will
leave it to the garbage-collector to delete it, that gives the driver
the possibility to recreate the neigh and to update its ah/pr,
otherwise the driver will try over and over to resend to that
unresolved neigh.

>
--
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-rc 6/7] IB/core: Fix incorrect array allocation

[Leon Romanovsky]

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

On Mon, Jun 06, 2016 at 07:53:43PM -0400, Doug Ledford wrote:
> On 6/4/2016 8:15 AM, Leon Romanovsky wrote:
> > From: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > 
> > In a attribute group struct, attrs should point to a NULL
> > terminated list. Which means we need to allocate one more
> > slot in the array.
> 
> This patch is both right and wrong.  You're right with the intent (that
> there should be a total of 2 extra entries in the array size, one for
> the NULL termination and one for the lifespan entry), but wrong with the
> mechanics (unless I've missed something).  We already have two extra
> entries because sizeof(*hsag) includes our first counter, so just
> num_counters is actually enough to NULL terminate the list, and + 1
> gives us lifespan plus a NULL terminate spot.  The comment could be
> cleaned up to make this more clear though, so I'll do that.

Thanks Doug.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-rc 0/7] IPoIB and IB core fixes for 4.7

[Leon Romanovsky]

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

On Mon, Jun 06, 2016 at 08:02:15PM -0400, Doug Ledford wrote:
> On 6/4/2016 8:15 AM, Leon Romanovsky wrote:
> > Hi Doug,
> > 
> > Please find below the patches with fixes for IPoIB and IB core.
> > 
> > This patchset is generated against Linus's v4.7-rc1 tag.
> > 
> > Available in the "topic/fixes-ib" 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/fixes-ib
> > 
> > Thanks
> > 
> > Aviv Heller (1):
> >   IB/core: Fix removal of default GID cache entry
> > 
> > Eli Cohen (1):
> >   IB/core: Fix query port failure in RoCE
> > 
> > Erez Shitrit (2):
> >   IB/IPoIB: Don't update neigh validity for unresolved entries
> >   IB/IPoIB: Fix race between ipoib_remove_one to sysfs functions
> > 
> > Mark Bloch (3):
> >   IB/IPoIB: Disable bottom half when dealing with device address
> >   IB/core: Fix incorrect array allocation
> >   IB/core: Initialize sysfs attributes before sysfs create group
> > 
> >  drivers/infiniband/core/cache.c                | 10 ++++++++--
> >  drivers/infiniband/core/device.c               |  3 +++
> >  drivers/infiniband/core/sysfs.c                | 10 ++++++++--
> >  drivers/infiniband/ulp/ipoib/ipoib.h           |  1 +
> >  drivers/infiniband/ulp/ipoib/ipoib_cm.c        |  4 ++++
> >  drivers/infiniband/ulp/ipoib/ipoib_ib.c        |  8 ++++----
> >  drivers/infiniband/ulp/ipoib/ipoib_main.c      | 15 ++++++++++-----
> >  drivers/infiniband/ulp/ipoib/ipoib_multicast.c |  6 +++---
> >  drivers/infiniband/ulp/ipoib/ipoib_vlan.c      |  6 ++++++
> >  9 files changed, 47 insertions(+), 16 deletions(-)
> > 
> 
> Thanks Leon, I applied 5 of the 7 (the two rejected patches were replied
> to individually).

Thanks Doug.

> 




[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-rc 1/7] IB/core: Fix query port failure in RoCE

[Leon Romanovsky]

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

Resending to the ML.

On Mon, Jun 06, 2016 at 10:13:50PM +0300, Leon Romanovsky wrote:
> Sent from mobile.
> On Jun 6, 2016 6:43 PM, "Steve Wise" <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org> wrote:
> >
> > >
> > > From: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > >
> > > Currently ib_query_port always attempts to to read the subnet prefix by
> > > calling ib_query_gid(). For RoCE there is no subnet manager and no
> > > subnet prefix. Fix this by querying GID[0] only for IB networks.
> > >
> > > Fixes: fad61ad4e755 ('IB/core: Add subnet prefix to port info')
> > > Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > > Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > ---
> > >  drivers/infiniband/core/device.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/infiniband/core/device.c
> > b/drivers/infiniband/core/device.c
> > > index 5516fb0..05e25a3 100644
> > > --- a/drivers/infiniband/core/device.c
> > > +++ b/drivers/infiniband/core/device.c
> > > @@ -661,6 +661,9 @@ int ib_query_port(struct ib_device *device,
> > >       if (err || port_attr->subnet_prefix)
> > >               return err;
> > >
> > > +     if (rdma_port_get_link_layer(device, port_num) !=
> > > IB_LINK_LAYER_INFINIBAND)
> > > +             return 0;
> > > +
> > >       err = ib_query_gid(device, port_num, 0, &gid, NULL);
> > >       if (err)
> > >               return err;
> >
> > This change assumes IB_LINK_LAYER_ETHERNET == RoCE, which isn't true for
> iWARP
> > devices.  Still, the change is good for iWARP too,
> 
> Thanks Steve,
> We spotted it during internal review too. We wanted to see this patch is
> accepted before sending followup patch which will remove such check from
> iwarp drivers.
> 
> > but the comment should
> > perhaps reflect this.
> >
> > Reviewed-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
> >
> > Steve.
> >

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-rc 3/7] IB/IPoIB: Fix race between ipoib_remove_one to sysfs functions

[Or Gerlitz]

On Sat, Jun 4, 2016 at 3:15 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> Fixes: 862096a8bbf8 ("IB/ipoib: Add more rtnl_link_ops callbacks")
> Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")

These two commits go back to 3.7-rc1 / Oct 2012 -- so I wonder how come we never
got reports on this ABBA deadlock happens in real life - any idea, did
you see the deadlock in
action, ever? We did had lockdep warnings and thanks you Erez for
finally getting there to fix after me... I will look on your patch.

Or.
--
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-rc 6/7] IB/core: Fix incorrect array allocation

[Mark Bloch]

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Doug Ledford
> Sent: Tuesday, June 07, 2016 2:54 AM
> To: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Subject: Re: [PATCH rdma-rc 6/7] IB/core: Fix incorrect array allocation
> 
> This patch is both right and wrong.  You're right with the intent (that
> there should be a total of 2 extra entries in the array size, one for
> the NULL termination and one for the lifespan entry), but wrong with the
> mechanics (unless I've missed something).  We already have two extra
> entries because sizeof(*hsag) includes our first counter, so just
> num_counters is actually enough to NULL terminate the list, and + 1
> gives us lifespan plus a NULL terminate spot.  The comment could be
> cleaned up to make this more clear though, so I'll do that.

Hi doug,
I might be missing something, but:

hsag = kzalloc(sizeof(*hsag) +
		sizeof(void *) * (stats->num_counters + 1)
                       	GFP_KERNEL);

So now we have hsag and after it num_counters + 1 slots.
Now we need attrs to point to a memory location, so we do:

hsag->attrs = (void *)hsag + sizeof(*hsag);

which means hsag->attrs is now pointing to a memory location right
after hsag (remember we have num_counters + 1) slots there.

for (i = 0; i < stats->num_counters; i++) {
                hsag->attrs[i] = alloc_hsa(i, port_num, stats->names[i]);
                if (!hsag->attrs[i])
                        goto err;
                sysfs_attr_init(hsag->attrs[i]);
}

/* treat an error here as non-fatal */
 hsag->attrs[i] = alloc_hsa_lifespan("lifespan", port_num);

The for loop fills num_counters slots, and after that alloc_hsa_lifespan uses another one.
Which means we used all our slots and we are missing one as the NULL slot.

Of course I might be wrong/missing something, it wouldn't be the first time :)

Mark

--
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-rc 3/7] IB/IPoIB: Fix race between ipoib_remove_one to sysfs functions

[Erez Shitrit]

On Tue, Jun 7, 2016 at 10:31 AM, Or Gerlitz <gerlitz.or-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Sat, Jun 4, 2016 at 3:15 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
>> Fixes: 862096a8bbf8 ("IB/ipoib: Add more rtnl_link_ops callbacks")
>> Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
>
> These two commits go back to 3.7-rc1 / Oct 2012 -- so I wonder how come we never
> got reports on this ABBA deadlock happens in real life - any idea, did
> you see the deadlock in

Sure, it is from the real life, you can see the stack that i added to
the commit message.
If you want to simulate that just add sleep in the remove_one while
trying to change mode/add-child etc.

> action, ever? We did had lockdep warnings and thanks you Erez for

welcome.

> finally getting there to fix after me... I will look on your patch.

>
> Or.
> --
> 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-rc 6/7] IB/core: Fix incorrect array allocation

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1349 bytes --]

On 6/7/2016 3:16 AM, Leon Romanovsky wrote:
> On Mon, Jun 06, 2016 at 07:53:43PM -0400, Doug Ledford wrote:
>> On 6/4/2016 8:15 AM, Leon Romanovsky wrote:
>>> From: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>>
>>> In a attribute group struct, attrs should point to a NULL
>>> terminated list. Which means we need to allocate one more
>>> slot in the array.
>>
>> This patch is both right and wrong.  You're right with the intent (that
>> there should be a total of 2 extra entries in the array size, one for
>> the NULL termination and one for the lifespan entry), but wrong with the
>> mechanics (unless I've missed something).  We already have two extra
>> entries because sizeof(*hsag) includes our first counter, so just
>> num_counters is actually enough to NULL terminate the list, and + 1
>> gives us lifespan plus a NULL terminate spot.  The comment could be
>> cleaned up to make this more clear though, so I'll do that.
> 
> Thanks Doug.
> 

I was wrong with this BTW (I tested to make sure).  Even though the
struct includes the name for the array, because it's 0 length, it
reserves 0 space.  I thought it would reserve one u64.  In any case, I
ended up writing a fix of my own since I had already marked your patch
as not needed in patchworks.  I listed Mark as the reporter in that patch.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-rc 6/7] IB/core: Fix incorrect array allocation

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 2401 bytes --]

On 6/7/2016 4:26 AM, Mark Bloch wrote:
> 
> 
>> -----Original Message-----
>> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
>> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Doug Ledford
>> Sent: Tuesday, June 07, 2016 2:54 AM
>> To: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> Subject: Re: [PATCH rdma-rc 6/7] IB/core: Fix incorrect array allocation
>>
>> This patch is both right and wrong.  You're right with the intent (that
>> there should be a total of 2 extra entries in the array size, one for
>> the NULL termination and one for the lifespan entry), but wrong with the
>> mechanics (unless I've missed something).  We already have two extra
>> entries because sizeof(*hsag) includes our first counter, so just
>> num_counters is actually enough to NULL terminate the list, and + 1
>> gives us lifespan plus a NULL terminate spot.  The comment could be
>> cleaned up to make this more clear though, so I'll do that.
> 
> Hi doug,
> I might be missing something, but:
> 
> hsag = kzalloc(sizeof(*hsag) +
> 		sizeof(void *) * (stats->num_counters + 1)
>                        	GFP_KERNEL);
> 
> So now we have hsag and after it num_counters + 1 slots.
> Now we need attrs to point to a memory location, so we do:
> 
> hsag->attrs = (void *)hsag + sizeof(*hsag);
> 
> which means hsag->attrs is now pointing to a memory location right
> after hsag (remember we have num_counters + 1) slots there.
> 
> for (i = 0; i < stats->num_counters; i++) {
>                 hsag->attrs[i] = alloc_hsa(i, port_num, stats->names[i]);
>                 if (!hsag->attrs[i])
>                         goto err;
>                 sysfs_attr_init(hsag->attrs[i]);
> }
> 
> /* treat an error here as non-fatal */
>  hsag->attrs[i] = alloc_hsa_lifespan("lifespan", port_num);
> 
> The for loop fills num_counters slots, and after that alloc_hsa_lifespan uses another one.
> Which means we used all our slots and we are missing one as the NULL slot.
> 
> Of course I might be wrong/missing something, it wouldn't be the first time :)
> 
> Mark
> 

Nope, you're right.  It's fixed now though.  with num_counters + 2 and
kzalloc, the final NULL terminator is in place.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-rc 2/7] IB/IPoIB: Don't update neigh validity for unresolved entries

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 2521 bytes --]

On 6/7/2016 2:01 AM, Erez Shitrit wrote:
> On Tue, Jun 7, 2016 at 2:59 AM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On 6/5/2016 5:10 PM, Or Gerlitz wrote:
>>
>>>> -                       neigh->alive = jiffies;
>>>> +
>>>> +                       if (likely(skb_queue_len(&neigh->queue) < IPOIB_MAX_PATH_REC_QUEUE))
>>>> +                               neigh->alive = jiffies;
>>>
>>> why testing the queue len makes things right?
>>
>> I'm with Or on this one.  This test doesn't make sense unless you assume
>> that the neighbor is being hit with a steady stream of packets.  If
>> someone just runs ping -c 1 to a dead neighbor, this test will fail.
>> The idea of the patch is OK, but the test needs to be reworked for
>> situations other than a blasting stream of data.
>>
> 
> I will try to explain the idea behind that test, and why it works (I
> hope I'll make it clear this time :))
> 
> there are 2 objects that are taking place here, the garbage-collector
> that removes neigh if was not used for defined time, and the
> path-query for each neigh.
> if the path-query failed the packets for specific neigh are kept in
> the neigh queue, but only if there are no more than
> IPOIB_MAX_PATH_REC_QUEUE, and the neigh is still kept for ever (let's
> assume CM connected that was stopped without any notification with
> DREQ etc.)
> The only way to know that there is an unresolved neigh is by checking
> its queue, if it is full we can assume that this is an unresolved
> neigh otherwise it is resolved.

That's not true.  Reread what I wrote above (I was specific when I
picked that command).  Ping -c 1 will only send one ping.  It will not
trip this test.  As I said, your test relies on a stream of packets, a
single packet to a gone neighbor will never cause it to get deleted.
You need a timeout on the queue and then if the packet in the queue
times out, regardless of whether the queue is full or not, then you mark
the neighbor for garbage collection and drop the packet(s).

> So, the test doesn't take any assumption on the shape of the traffic,
> even in a ping requests once a second it will failed after 3 times the
> request was sent, and the neigh time will not be updated which will
> leave it to the garbage-collector to delete it, that gives the driver
> the possibility to recreate the neigh and to update its ah/pr,
> otherwise the driver will try over and over to resend to that
> unresolved neigh.
> 
>>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

RE: [PATCH rdma-rc 6/7] IB/core: Fix incorrect array allocation

[Mark Bloch]

> -----Original Message-----
> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> Sent: Tuesday, June 07, 2016 4:46 PM
> To: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Leon Romanovsky
> <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH rdma-rc 6/7] IB/core: Fix incorrect array allocation
> 
> Nope, you're right.  It's fixed now though.  with num_counters + 2 and
> kzalloc, the final NULL terminator is in place.

Cool, thank you,

Mark


--
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-rc 2/7] IB/IPoIB: Don't update neigh validity for unresolved entries

[Erez Shitrit]

On Tue, Jun 7, 2016 at 4:48 PM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 6/7/2016 2:01 AM, Erez Shitrit wrote:
>> On Tue, Jun 7, 2016 at 2:59 AM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>> On 6/5/2016 5:10 PM, Or Gerlitz wrote:
>>>
>>>>> -                       neigh->alive = jiffies;
>>>>> +
>>>>> +                       if (likely(skb_queue_len(&neigh->queue) < IPOIB_MAX_PATH_REC_QUEUE))
>>>>> +                               neigh->alive = jiffies;
>>>>
>>>> why testing the queue len makes things right?
>>>
>>> I'm with Or on this one.  This test doesn't make sense unless you assume
>>> that the neighbor is being hit with a steady stream of packets.  If
>>> someone just runs ping -c 1 to a dead neighbor, this test will fail.
>>> The idea of the patch is OK, but the test needs to be reworked for
>>> situations other than a blasting stream of data.
>>>
>>
>> I will try to explain the idea behind that test, and why it works (I
>> hope I'll make it clear this time :))
>>
>> there are 2 objects that are taking place here, the garbage-collector
>> that removes neigh if was not used for defined time, and the
>> path-query for each neigh.
>> if the path-query failed the packets for specific neigh are kept in
>> the neigh queue, but only if there are no more than
>> IPOIB_MAX_PATH_REC_QUEUE, and the neigh is still kept for ever (let's
>> assume CM connected that was stopped without any notification with
>> DREQ etc.)
>> The only way to know that there is an unresolved neigh is by checking
>> its queue, if it is full we can assume that this is an unresolved
>> neigh otherwise it is resolved.
>
> That's not true.  Reread what I wrote above (I was specific when I
> picked that command).  Ping -c 1 will only send one ping.  It will not
> trip this test.  As I said, your test relies on a stream of packets, a
> single packet to a gone neighbor will never cause it to get deleted.

If you ping one time (ping -c 1) there is no problem at all, the neigh
will be deleted by the GC anyway, because no (unresolved) packet
updates the neigh validity and the GC will check the last time it was
refreshed and since only one ping refreshed it long ago, it will be
deleted. (the GC always running)

the problem is when there are non stop traffic to that neigh. IMHO
that test works for that.

> You need a timeout on the queue and then if the packet in the queue
> times out, regardless of whether the queue is full or not, then you mark
> the neighbor for garbage collection and drop the packet(s).
>
>> So, the test doesn't take any assumption on the shape of the traffic,
>> even in a ping requests once a second it will failed after 3 times the
>> request was sent, and the neigh time will not be updated which will
>> leave it to the garbage-collector to delete it, that gives the driver
>> the possibility to recreate the neigh and to update its ah/pr,
>> otherwise the driver will try over and over to resend to that
>> unresolved neigh.
>>
>>>
>
>
--
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-rc 2/7] IB/IPoIB: Don't update neigh validity for unresolved entries

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 3239 bytes --]

On 6/7/2016 10:32 AM, Erez Shitrit wrote:
> On Tue, Jun 7, 2016 at 4:48 PM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On 6/7/2016 2:01 AM, Erez Shitrit wrote:
>>> On Tue, Jun 7, 2016 at 2:59 AM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>> On 6/5/2016 5:10 PM, Or Gerlitz wrote:
>>>>
>>>>>> -                       neigh->alive = jiffies;
>>>>>> +
>>>>>> +                       if (likely(skb_queue_len(&neigh->queue) < IPOIB_MAX_PATH_REC_QUEUE))
>>>>>> +                               neigh->alive = jiffies;
>>>>>
>>>>> why testing the queue len makes things right?
>>>>
>>>> I'm with Or on this one.  This test doesn't make sense unless you assume
>>>> that the neighbor is being hit with a steady stream of packets.  If
>>>> someone just runs ping -c 1 to a dead neighbor, this test will fail.
>>>> The idea of the patch is OK, but the test needs to be reworked for
>>>> situations other than a blasting stream of data.
>>>>
>>>
>>> I will try to explain the idea behind that test, and why it works (I
>>> hope I'll make it clear this time :))
>>>
>>> there are 2 objects that are taking place here, the garbage-collector
>>> that removes neigh if was not used for defined time, and the
>>> path-query for each neigh.
>>> if the path-query failed the packets for specific neigh are kept in
>>> the neigh queue, but only if there are no more than
>>> IPOIB_MAX_PATH_REC_QUEUE, and the neigh is still kept for ever (let's
>>> assume CM connected that was stopped without any notification with
>>> DREQ etc.)
>>> The only way to know that there is an unresolved neigh is by checking
>>> its queue, if it is full we can assume that this is an unresolved
>>> neigh otherwise it is resolved.
>>
>> That's not true.  Reread what I wrote above (I was specific when I
>> picked that command).  Ping -c 1 will only send one ping.  It will not
>> trip this test.  As I said, your test relies on a stream of packets, a
>> single packet to a gone neighbor will never cause it to get deleted.
> 
> If you ping one time (ping -c 1) there is no problem at all, the neigh
> will be deleted by the GC anyway, because no (unresolved) packet
> updates the neigh validity and the GC will check the last time it was
> refreshed and since only one ping refreshed it long ago, it will be
> deleted. (the GC always running)
> 
> the problem is when there are non stop traffic to that neigh. IMHO
> that test works for that.

Ok, that makes sense.

>> You need a timeout on the queue and then if the packet in the queue
>> times out, regardless of whether the queue is full or not, then you mark
>> the neighbor for garbage collection and drop the packet(s).
>>
>>> So, the test doesn't take any assumption on the shape of the traffic,
>>> even in a ping requests once a second it will failed after 3 times the
>>> request was sent, and the neigh time will not be updated which will
>>> leave it to the garbage-collector to delete it, that gives the driver
>>> the possibility to recreate the neigh and to update its ah/pr,
>>> otherwise the driver will try over and over to resend to that
>>> unresolved neigh.
>>>
>>>>
>>
>>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-rc 2/7] IB/IPoIB: Don't update neigh validity for unresolved entries

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 2755 bytes --]

On 6/7/2016 10:43 AM, Doug Ledford wrote:
> On 6/7/2016 10:32 AM, Erez Shitrit wrote:
>> On Tue, Jun 7, 2016 at 4:48 PM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>> On 6/7/2016 2:01 AM, Erez Shitrit wrote:
>>>> On Tue, Jun 7, 2016 at 2:59 AM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>>> On 6/5/2016 5:10 PM, Or Gerlitz wrote:
>>>>>
>>>>>>> -                       neigh->alive = jiffies;
>>>>>>> +
>>>>>>> +                       if (likely(skb_queue_len(&neigh->queue) < IPOIB_MAX_PATH_REC_QUEUE))
>>>>>>> +                               neigh->alive = jiffies;
>>>>>>
>>>>>> why testing the queue len makes things right?
>>>>>
>>>>> I'm with Or on this one.  This test doesn't make sense unless you assume
>>>>> that the neighbor is being hit with a steady stream of packets.  If
>>>>> someone just runs ping -c 1 to a dead neighbor, this test will fail.
>>>>> The idea of the patch is OK, but the test needs to be reworked for
>>>>> situations other than a blasting stream of data.
>>>>>
>>>>
>>>> I will try to explain the idea behind that test, and why it works (I
>>>> hope I'll make it clear this time :))
>>>>
>>>> there are 2 objects that are taking place here, the garbage-collector
>>>> that removes neigh if was not used for defined time, and the
>>>> path-query for each neigh.
>>>> if the path-query failed the packets for specific neigh are kept in
>>>> the neigh queue, but only if there are no more than
>>>> IPOIB_MAX_PATH_REC_QUEUE, and the neigh is still kept for ever (let's
>>>> assume CM connected that was stopped without any notification with
>>>> DREQ etc.)
>>>> The only way to know that there is an unresolved neigh is by checking
>>>> its queue, if it is full we can assume that this is an unresolved
>>>> neigh otherwise it is resolved.
>>>
>>> That's not true.  Reread what I wrote above (I was specific when I
>>> picked that command).  Ping -c 1 will only send one ping.  It will not
>>> trip this test.  As I said, your test relies on a stream of packets, a
>>> single packet to a gone neighbor will never cause it to get deleted.
>>
>> If you ping one time (ping -c 1) there is no problem at all, the neigh
>> will be deleted by the GC anyway, because no (unresolved) packet
>> updates the neigh validity and the GC will check the last time it was
>> refreshed and since only one ping refreshed it long ago, it will be
>> deleted. (the GC always running)
>>
>> the problem is when there are non stop traffic to that neigh. IMHO
>> that test works for that.
> 
> Ok, that makes sense.

Sorry, in case it wasn't clear, I grabbed this patch now (although I
reworded the commit message significantly).



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-rc 2/7] IB/IPoIB: Don't update neigh validity for unresolved entries

[Leon Romanovsky]

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

On Tue, Jun 07, 2016 at 10:50:54AM -0400, Doug Ledford wrote:
> On 6/7/2016 10:43 AM, Doug Ledford wrote:
> > On 6/7/2016 10:32 AM, Erez Shitrit wrote:
> >> On Tue, Jun 7, 2016 at 4:48 PM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >>> On 6/7/2016 2:01 AM, Erez Shitrit wrote:
> >>>> On Tue, Jun 7, 2016 at 2:59 AM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >>>>> On 6/5/2016 5:10 PM, Or Gerlitz wrote:
> >>>>>
> >>>>>>> -                       neigh->alive = jiffies;
> >>>>>>> +
> >>>>>>> +                       if (likely(skb_queue_len(&neigh->queue) < IPOIB_MAX_PATH_REC_QUEUE))
> >>>>>>> +                               neigh->alive = jiffies;
> >>>>>>
> >>>>>> why testing the queue len makes things right?
> >>>>>
> >>>>> I'm with Or on this one.  This test doesn't make sense unless you assume
> >>>>> that the neighbor is being hit with a steady stream of packets.  If
> >>>>> someone just runs ping -c 1 to a dead neighbor, this test will fail.
> >>>>> The idea of the patch is OK, but the test needs to be reworked for
> >>>>> situations other than a blasting stream of data.
> >>>>>
> >>>>
> >>>> I will try to explain the idea behind that test, and why it works (I
> >>>> hope I'll make it clear this time :))
> >>>>
> >>>> there are 2 objects that are taking place here, the garbage-collector
> >>>> that removes neigh if was not used for defined time, and the
> >>>> path-query for each neigh.
> >>>> if the path-query failed the packets for specific neigh are kept in
> >>>> the neigh queue, but only if there are no more than
> >>>> IPOIB_MAX_PATH_REC_QUEUE, and the neigh is still kept for ever (let's
> >>>> assume CM connected that was stopped without any notification with
> >>>> DREQ etc.)
> >>>> The only way to know that there is an unresolved neigh is by checking
> >>>> its queue, if it is full we can assume that this is an unresolved
> >>>> neigh otherwise it is resolved.
> >>>
> >>> That's not true.  Reread what I wrote above (I was specific when I
> >>> picked that command).  Ping -c 1 will only send one ping.  It will not
> >>> trip this test.  As I said, your test relies on a stream of packets, a
> >>> single packet to a gone neighbor will never cause it to get deleted.
> >>
> >> If you ping one time (ping -c 1) there is no problem at all, the neigh
> >> will be deleted by the GC anyway, because no (unresolved) packet
> >> updates the neigh validity and the GC will check the last time it was
> >> refreshed and since only one ping refreshed it long ago, it will be
> >> deleted. (the GC always running)
> >>
> >> the problem is when there are non stop traffic to that neigh. IMHO
> >> that test works for that.
> > 
> > Ok, that makes sense.
> 
> Sorry, in case it wasn't clear, I grabbed this patch now (although I
> reworded the commit message significantly).

Thank you,
I truly appreciate your help.

> 
> 




[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-rc 6/7] IB/core: Fix incorrect array allocation

[Leon Romanovsky]

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

On Tue, Jun 07, 2016 at 09:43:29AM -0400, Doug Ledford wrote:
> On 6/7/2016 3:16 AM, Leon Romanovsky wrote:
> > On Mon, Jun 06, 2016 at 07:53:43PM -0400, Doug Ledford wrote:
> >> On 6/4/2016 8:15 AM, Leon Romanovsky wrote:
> >>> From: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >>>
> >>> In a attribute group struct, attrs should point to a NULL
> >>> terminated list. Which means we need to allocate one more
> >>> slot in the array.
> >>
> >> This patch is both right and wrong.  You're right with the intent (that
> >> there should be a total of 2 extra entries in the array size, one for
> >> the NULL termination and one for the lifespan entry), but wrong with the
> >> mechanics (unless I've missed something).  We already have two extra
> >> entries because sizeof(*hsag) includes our first counter, so just
> >> num_counters is actually enough to NULL terminate the list, and + 1
> >> gives us lifespan plus a NULL terminate spot.  The comment could be
> >> cleaned up to make this more clear though, so I'll do that.
> > 
> > Thanks Doug.
> > 
> 
> I was wrong with this BTW (I tested to make sure).  Even though the
> struct includes the name for the array, because it's 0 length, it
> reserves 0 space.  I thought it would reserve one u64.  In any case, I
> ended up writing a fix of my own since I had already marked your patch
> as not needed in patchworks.  I listed Mark as the reporter in that patch.

Since I know that you are testing the patches, and your "I'll do that."
was enough for me.

Thanks.

> 




[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-rc 3/7] IB/IPoIB: Fix race between ipoib_remove_one to sysfs functions

[Or Gerlitz]

On Tue, Jun 7, 2016 at 1:45 PM, Erez Shitrit <erezsh-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
> On Tue, Jun 7, 2016 at 10:31 AM, Or Gerlitz <gerlitz.or-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Sat, Jun 4, 2016 at 3:15 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>
>>> Fixes: 862096a8bbf8 ("IB/ipoib: Add more rtnl_link_ops callbacks")
>>> Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
>>
>> These two commits go back to 3.7-rc1 / Oct 2012 -- so I wonder how come we never
>> got reports on this ABBA deadlock happens in real life - any idea, did
>> you see the deadlock in
>
> Sure, it is from the real life, you can see the stack that i added to
> the commit message.

yes, but the bug you're pointing on is out there for whole four years
and no one complained including you, can you explain that?

> If you want to simulate that just add sleep in the remove_one while
> trying to change mode/add-child etc.

so why need to simulate? is that impossible to get it on non simulated env?
--
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-rc 3/7] IB/IPoIB: Fix race between ipoib_remove_one to sysfs functions

[Erez Shitrit]

On Tue, Jun 7, 2016 at 11:22 PM, Or Gerlitz <gerlitz.or-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Jun 7, 2016 at 1:45 PM, Erez Shitrit <erezsh-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
>> On Tue, Jun 7, 2016 at 10:31 AM, Or Gerlitz <gerlitz.or-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On Sat, Jun 4, 2016 at 3:15 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>>
>>>> Fixes: 862096a8bbf8 ("IB/ipoib: Add more rtnl_link_ops callbacks")
>>>> Fixes: 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
>>>
>>> These two commits go back to 3.7-rc1 / Oct 2012 -- so I wonder how come we never
>>> got reports on this ABBA deadlock happens in real life - any idea, did
>>> you see the deadlock in
>>
>> Sure, it is from the real life, you can see the stack that i added to
>> the commit message.
>
> yes, but the bug you're pointing on is out there for whole four years
> and no one complained including you, can you explain that?

Not really, it is a race, not easy to see it, but it is there, waiting..

>
>> If you want to simulate that just add sleep in the remove_one while
>> trying to change mode/add-child etc.
>
> so why need to simulate? is that impossible to get it on non simulated env?

It happened in our regression system.
--
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-rc 3/7] IB/IPoIB: Fix race between ipoib_remove_one to sysfs functions

[Or Gerlitz]

On Wed, Jun 8, 2016 at 10:01 AM, Erez Shitrit <erezsh-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
> On Tue, Jun 7, 2016 at 11:22 PM, Or Gerlitz <gerlitz.or-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

>> yes, but the bug you're pointing on is out there for whole four years
>> and no one complained including you, can you explain that?

> Not really, it is a race, not easy to see it, but it is there, waiting..

yes, seems this is the case,

>> so why need to simulate? is that impossible to get it on non simulated env?

> It happened in our regression system.

so after four years the deadlock happened once in the regression
system and no one
else complained, that sounds like hard to reproduce one... good catch.

Or.
--
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