[PATCH for-next 0/3] Change per-entry locks in GID cache to table lock

[PATCH for-next 0/3] Change per-entry locks in GID cache to table lock

[Matan Barak]

Hi Doug,

During the RoCE GID cache changes, we used a per-entry rwlock. This could cause
a major overhead when traversing the GID table. We could spend thousands of
cycles just by locking and unlocking entries. This change was requested by
Doug.

In order to solve that, we moved to one table lock. find_gid is also optimized
to find an empty slot while traversing the GID table (when needed).

Regards,
Matan

Matan Barak (3):
  IB/core: Refactor GID cache's ib_dispatch_event
  IB/core: Change per-entry lock in RoCE GID table to one lock
  IB/core: Find gid searches for empty slot while traversing the GID
    table

 drivers/infiniband/core/cache.c | 209 ++++++++++++++++++++++++++--------------
 1 file changed, 134 insertions(+), 75 deletions(-)

-- 
2.1.0

--
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 for-next 1/3] IB/core: Refactor GID cache's ib_dispatch_event

[Matan Barak]

Refactor ib_dispatch_event into a new function in order to avoid
duplicating code in the next patch.

Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/cache.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index 89bebea..f0703d2 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -112,6 +112,19 @@ struct ib_gid_table {
 	struct ib_gid_table_entry *data_vec;
 };
 
+static void dispatch_gid_change_event(struct ib_device *ib_dev, u8 port)
+{
+	if (rdma_cap_roce_gid_table(ib_dev, port)) {
+		struct ib_event event;
+
+		event.device		= ib_dev;
+		event.element.port_num	= port;
+		event.event		= IB_EVENT_GID_CHANGE;
+
+		ib_dispatch_event(&event);
+	}
+}
+
 static int write_gid(struct ib_device *ib_dev, u8 port,
 		     struct ib_gid_table *table, int ix,
 		     const union ib_gid *gid,
@@ -164,15 +177,9 @@ static int write_gid(struct ib_device *ib_dev, u8 port,
 
 	write_unlock_irqrestore(&table->data_vec[ix].lock, flags);
 
-	if (!ret && rdma_cap_roce_gid_table(ib_dev, port)) {
-		struct ib_event event;
+	if (!ret)
+		dispatch_gid_change_event(ib_dev, port);
 
-		event.device		= ib_dev;
-		event.element.port_num	= port;
-		event.event		= IB_EVENT_GID_CHANGE;
-
-		ib_dispatch_event(&event);
-	}
 	return ret;
 }
 
-- 
2.1.0

--
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 for-next 2/3] IB/core: Change per-entry lock in RoCE GID table to one lock

[Matan Barak]

Previously, IB GID cached used a lock per entry. This could result
in spending a lot of CPU cycles for locking and unlocking just
in order to find a GID. Changing this in favor of one lock per
a GID table.

Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/cache.c | 143 +++++++++++++++++++++++++---------------
 1 file changed, 89 insertions(+), 54 deletions(-)

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index f0703d2..83aee7c 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -81,10 +81,6 @@ enum gid_table_write_action {
 };
 
 struct ib_gid_table_entry {
-	/* This lock protects an entry from being
-	 * read and written simultaneously.
-	 */
-	rwlock_t	    lock;
 	unsigned long	    props;
 	union ib_gid        gid;
 	struct ib_gid_attr  attr;
@@ -109,6 +105,10 @@ struct ib_gid_table {
 	 * are locked by this lock.
 	 **/
 	struct mutex         lock;
+	/* This lock protects the table entries from being
+	 * read and written simultaneously.
+	 */
+	rwlock_t	     rwlock;
 	struct ib_gid_table_entry *data_vec;
 };
 
@@ -125,6 +125,10 @@ static void dispatch_gid_change_event(struct ib_device *ib_dev, u8 port)
 	}
 }
 
+/* This function expects that rwlock will be write locked in all
+ * scenarios and that lock will be locked in sleep-able (RoCE)
+ * scenarios.
+ */
 static int write_gid(struct ib_device *ib_dev, u8 port,
 		     struct ib_gid_table *table, int ix,
 		     const union ib_gid *gid,
@@ -134,16 +138,14 @@ static int write_gid(struct ib_device *ib_dev, u8 port,
 {
 	int ret = 0;
 	struct net_device *old_net_dev;
-	unsigned long flags;
 
 	/* in rdma_cap_roce_gid_table, this funciton should be protected by a
 	 * sleep-able lock.
 	 */
-	write_lock_irqsave(&table->data_vec[ix].lock, flags);
 
 	if (rdma_cap_roce_gid_table(ib_dev, port)) {
 		table->data_vec[ix].props |= GID_TABLE_ENTRY_INVALID;
-		write_unlock_irqrestore(&table->data_vec[ix].lock, flags);
+		write_unlock_irq(&table->rwlock);
 		/* GID_TABLE_WRITE_ACTION_MODIFY currently isn't supported by
 		 * RoCE providers and thus only updates the cache.
 		 */
@@ -153,7 +155,7 @@ static int write_gid(struct ib_device *ib_dev, u8 port,
 		else if (action == GID_TABLE_WRITE_ACTION_DEL)
 			ret = ib_dev->del_gid(ib_dev, port, ix,
 					      &table->data_vec[ix].context);
-		write_lock_irqsave(&table->data_vec[ix].lock, flags);
+		write_lock_irq(&table->rwlock);
 	}
 
 	old_net_dev = table->data_vec[ix].attr.ndev;
@@ -175,11 +177,6 @@ static int write_gid(struct ib_device *ib_dev, u8 port,
 
 	table->data_vec[ix].props &= ~GID_TABLE_ENTRY_INVALID;
 
-	write_unlock_irqrestore(&table->data_vec[ix].lock, flags);
-
-	if (!ret)
-		dispatch_gid_change_event(ib_dev, port);
-
 	return ret;
 }
 
@@ -208,6 +205,7 @@ static int del_gid(struct ib_device *ib_dev, u8 port,
 			 GID_TABLE_WRITE_ACTION_DEL, default_gid);
 }
 
+/* rwlock should be read locked */
 static int find_gid(struct ib_gid_table *table, const union ib_gid *gid,
 		    const struct ib_gid_attr *val, bool default_gid,
 		    unsigned long mask)
@@ -215,34 +213,29 @@ static int find_gid(struct ib_gid_table *table, const union ib_gid *gid,
 	int i;
 
 	for (i = 0; i < table->sz; i++) {
-		unsigned long flags;
 		struct ib_gid_attr *attr = &table->data_vec[i].attr;
 
-		read_lock_irqsave(&table->data_vec[i].lock, flags);
 
 		if (table->data_vec[i].props & GID_TABLE_ENTRY_INVALID)
-			goto next;
+			continue;
 
 		if (mask & GID_ATTR_FIND_MASK_GID &&
 		    memcmp(gid, &table->data_vec[i].gid, sizeof(*gid)))
-			goto next;
+			continue;
 
 		if (mask & GID_ATTR_FIND_MASK_NETDEV &&
 		    attr->ndev != val->ndev)
-			goto next;
+			continue;
 
 		if (mask & GID_ATTR_FIND_MASK_DEFAULT &&
 		    !!(table->data_vec[i].props & GID_TABLE_ENTRY_DEFAULT) !=
 		    default_gid)
-			goto next;
+			continue;
 
-		read_unlock_irqrestore(&table->data_vec[i].lock, flags);
-		return i;
-next:
-		read_unlock_irqrestore(&table->data_vec[i].lock, flags);
+		break;
 	}
 
-	return -1;
+	return i == table->sz ? -1 : i;
 }
 
 static void make_default_gid(struct  net_device *dev, union ib_gid *gid)
@@ -282,6 +275,7 @@ int ib_cache_gid_add(struct ib_device *ib_dev, u8 port,
 	}
 
 	mutex_lock(&table->lock);
+	write_lock_irq(&table->rwlock);
 
 	ix = find_gid(table, gid, attr, false, GID_ATTR_FIND_MASK_GID |
 		      GID_ATTR_FIND_MASK_NETDEV);
@@ -295,9 +289,12 @@ int ib_cache_gid_add(struct ib_device *ib_dev, u8 port,
 		goto out_unlock;
 	}
 
-	add_gid(ib_dev, port, table, ix, gid, attr, false);
+	ret = add_gid(ib_dev, port, table, ix, gid, attr, false);
+	if (!ret)
+		dispatch_gid_change_event(ib_dev, port);
 
 out_unlock:
+	write_unlock_irq(&table->rwlock);
 	mutex_unlock(&table->lock);
 	return ret;
 }
@@ -312,6 +309,7 @@ int ib_cache_gid_del(struct ib_device *ib_dev, u8 port,
 	table = ports_table[port - rdma_start_port(ib_dev)];
 
 	mutex_lock(&table->lock);
+	write_lock_irq(&table->rwlock);
 
 	ix = find_gid(table, gid, attr, false,
 		      GID_ATTR_FIND_MASK_GID	  |
@@ -320,9 +318,11 @@ int ib_cache_gid_del(struct ib_device *ib_dev, u8 port,
 	if (ix < 0)
 		goto out_unlock;
 
-	del_gid(ib_dev, port, table, ix, false);
+	if (!del_gid(ib_dev, port, table, ix, false))
+		dispatch_gid_change_event(ib_dev, port);
 
 out_unlock:
+	write_unlock_irq(&table->rwlock);
 	mutex_unlock(&table->lock);
 	return 0;
 }
@@ -333,16 +333,24 @@ int ib_cache_gid_del_all_netdev_gids(struct ib_device *ib_dev, u8 port,
 	struct ib_gid_table **ports_table = ib_dev->cache.gid_cache;
 	struct ib_gid_table *table;
 	int ix;
+	bool deleted = false;
 
 	table  = ports_table[port - rdma_start_port(ib_dev)];
 
 	mutex_lock(&table->lock);
+	write_lock_irq(&table->rwlock);
 
 	for (ix = 0; ix < table->sz; ix++)
 		if (table->data_vec[ix].attr.ndev == ndev)
-			del_gid(ib_dev, port, table, ix, false);
+			if (!del_gid(ib_dev, port, table, ix, false))
+				deleted = true;
 
+	write_unlock_irq(&table->rwlock);
 	mutex_unlock(&table->lock);
+
+	if (deleted)
+		dispatch_gid_change_event(ib_dev, port);
+
 	return 0;
 }
 
@@ -351,18 +359,14 @@ static int __ib_cache_gid_get(struct ib_device *ib_dev, u8 port, int index,
 {
 	struct ib_gid_table **ports_table = ib_dev->cache.gid_cache;
 	struct ib_gid_table *table;
-	unsigned long flags;
 
 	table = ports_table[port - rdma_start_port(ib_dev)];
 
 	if (index < 0 || index >= table->sz)
 		return -EINVAL;
 
-	read_lock_irqsave(&table->data_vec[index].lock, flags);
-	if (table->data_vec[index].props & GID_TABLE_ENTRY_INVALID) {
-		read_unlock_irqrestore(&table->data_vec[index].lock, flags);
+	if (table->data_vec[index].props & GID_TABLE_ENTRY_INVALID)
 		return -EAGAIN;
-	}
 
 	memcpy(gid, &table->data_vec[index].gid, sizeof(*gid));
 	if (attr) {
@@ -371,7 +375,6 @@ static int __ib_cache_gid_get(struct ib_device *ib_dev, u8 port, int index,
 			dev_hold(attr->ndev);
 	}
 
-	read_unlock_irqrestore(&table->data_vec[index].lock, flags);
 	return 0;
 }
 
@@ -385,17 +388,21 @@ static int _ib_cache_gid_table_find(struct ib_device *ib_dev,
 	struct ib_gid_table *table;
 	u8 p;
 	int local_index;
+	unsigned long flags;
 
 	for (p = 0; p < ib_dev->phys_port_cnt; p++) {
 		table = ports_table[p];
+		read_lock_irqsave(&table->rwlock, flags);
 		local_index = find_gid(table, gid, val, false, mask);
 		if (local_index >= 0) {
 			if (index)
 				*index = local_index;
 			if (port)
 				*port = p + rdma_start_port(ib_dev);
+			read_unlock_irqrestore(&table->rwlock, flags);
 			return 0;
 		}
+		read_unlock_irqrestore(&table->rwlock, flags);
 	}
 
 	return -ENOENT;
@@ -426,6 +433,7 @@ int ib_find_cached_gid_by_port(struct ib_device *ib_dev,
 	struct ib_gid_table *table;
 	unsigned long mask = GID_ATTR_FIND_MASK_GID;
 	struct ib_gid_attr val = {.ndev = ndev};
+	unsigned long flags;
 
 	if (port < rdma_start_port(ib_dev) ||
 	    port > rdma_end_port(ib_dev))
@@ -436,13 +444,16 @@ int ib_find_cached_gid_by_port(struct ib_device *ib_dev,
 	if (ndev)
 		mask |= GID_ATTR_FIND_MASK_NETDEV;
 
+	read_lock_irqsave(&table->rwlock, flags);
 	local_index = find_gid(table, gid, &val, false, mask);
 	if (local_index >= 0) {
 		if (index)
 			*index = local_index;
+		read_unlock_irqrestore(&table->rwlock, flags);
 		return 0;
 	}
 
+	read_unlock_irqrestore(&table->rwlock, flags);
 	return -ENOENT;
 }
 EXPORT_SYMBOL(ib_find_cached_gid_by_port);
@@ -479,6 +490,7 @@ static int ib_cache_gid_find_by_filter(struct ib_device *ib_dev,
 	struct ib_gid_table **ports_table = ib_dev->cache.gid_cache;
 	struct ib_gid_table *table;
 	unsigned int i;
+	unsigned long flags;
 	bool found = false;
 
 	if (!ports_table)
@@ -491,11 +503,10 @@ static int ib_cache_gid_find_by_filter(struct ib_device *ib_dev,
 
 	table = ports_table[port - rdma_start_port(ib_dev)];
 
+	read_lock_irqsave(&table->rwlock, flags);
 	for (i = 0; i < table->sz; i++) {
 		struct ib_gid_attr attr;
-		unsigned long flags;
 
-		read_lock_irqsave(&table->data_vec[i].lock, flags);
 		if (table->data_vec[i].props & GID_TABLE_ENTRY_INVALID)
 			goto next;
 
@@ -508,11 +519,10 @@ static int ib_cache_gid_find_by_filter(struct ib_device *ib_dev,
 			found = true;
 
 next:
-		read_unlock_irqrestore(&table->data_vec[i].lock, flags);
-
 		if (found)
 			break;
 	}
+	read_unlock_irqrestore(&table->rwlock, flags);
 
 	if (!found)
 		return -ENOENT;
@@ -524,9 +534,9 @@ next:
 
 static struct ib_gid_table *alloc_gid_table(int sz)
 {
-	unsigned int i;
 	struct ib_gid_table *table =
 		kzalloc(sizeof(struct ib_gid_table), GFP_KERNEL);
+
 	if (!table)
 		return NULL;
 
@@ -537,9 +547,7 @@ static struct ib_gid_table *alloc_gid_table(int sz)
 	mutex_init(&table->lock);
 
 	table->sz = sz;
-
-	for (i = 0; i < sz; i++)
-		rwlock_init(&table->data_vec[i].lock);
+	rwlock_init(&table->rwlock);
 
 	return table;
 
@@ -560,17 +568,24 @@ static void cleanup_gid_table_port(struct ib_device *ib_dev, u8 port,
 				   struct ib_gid_table *table)
 {
 	int i;
+	bool deleted = false;
 
 	if (!table)
 		return;
 
+	write_lock_irq(&table->rwlock);
 	for (i = 0; i < table->sz; ++i) {
 		if (memcmp(&table->data_vec[i].gid, &zgid,
 			   sizeof(table->data_vec[i].gid)))
-			del_gid(ib_dev, port, table, i,
-				table->data_vec[i].props &
-				GID_ATTR_FIND_MASK_DEFAULT);
+			if (!del_gid(ib_dev, port, table, i,
+				     table->data_vec[i].props &
+				     GID_ATTR_FIND_MASK_DEFAULT))
+				deleted = true;
 	}
+	write_unlock_irq(&table->rwlock);
+
+	if (deleted)
+		dispatch_gid_change_event(ib_dev, port);
 }
 
 void ib_cache_gid_set_default_gid(struct ib_device *ib_dev, u8 port,
@@ -592,6 +607,7 @@ void ib_cache_gid_set_default_gid(struct ib_device *ib_dev, u8 port,
 	gid_attr.ndev = ndev;
 
 	mutex_lock(&table->lock);
+	write_lock_irq(&table->rwlock);
 	ix = find_gid(table, NULL, NULL, true, GID_ATTR_FIND_MASK_DEFAULT);
 
 	/* Coudn't find default GID location */
@@ -604,23 +620,31 @@ void ib_cache_gid_set_default_gid(struct ib_device *ib_dev, u8 port,
 	    !memcmp(&gid_attr, &current_gid_attr, sizeof(gid_attr)))
 		goto unlock;
 
-	if ((memcmp(&current_gid, &zgid, sizeof(current_gid)) ||
-	     memcmp(&current_gid_attr, &zattr,
-		    sizeof(current_gid_attr))) &&
-	    del_gid(ib_dev, port, table, ix, true)) {
-		pr_warn("ib_cache_gid: can't delete index %d for default gid %pI6\n",
-			ix, gid.raw);
-		goto unlock;
+	if (memcmp(&current_gid, &zgid, sizeof(current_gid)) ||
+	    memcmp(&current_gid_attr, &zattr,
+		   sizeof(current_gid_attr))) {
+		if (del_gid(ib_dev, port, table, ix, true)) {
+			pr_warn("ib_cache_gid: can't delete index %d for default gid %pI6\n",
+				ix, gid.raw);
+			goto unlock;
+		} else {
+			dispatch_gid_change_event(ib_dev, port);
+		}
 	}
 
-	if (mode == IB_CACHE_GID_DEFAULT_MODE_SET)
-		if (add_gid(ib_dev, port, table, ix, &gid, &gid_attr, true))
+	if (mode == IB_CACHE_GID_DEFAULT_MODE_SET) {
+		if (add_gid(ib_dev, port, table, ix, &gid, &gid_attr, true)) {
 			pr_warn("ib_cache_gid: unable to add default gid %pI6\n",
 				gid.raw);
+		} else {
+			dispatch_gid_change_event(ib_dev, port);
+		}
+	}
 
 unlock:
 	if (current_gid_attr.ndev)
 		dev_put(current_gid_attr.ndev);
+	write_unlock_irq(&table->rwlock);
 	mutex_unlock(&table->lock);
 }
 
@@ -735,10 +759,19 @@ int ib_get_cached_gid(struct ib_device *device,
 		      union ib_gid     *gid,
 		      struct ib_gid_attr *gid_attr)
 {
+	int res;
+	unsigned long flags;
+	struct ib_gid_table **ports_table = device->cache.gid_cache;
+	struct ib_gid_table *table = ports_table[port_num - rdma_start_port(device)];
+
 	if (port_num < rdma_start_port(device) || port_num > rdma_end_port(device))
 		return -EINVAL;
 
-	return __ib_cache_gid_get(device, port_num, index, gid, gid_attr);
+	read_lock_irqsave(&table->rwlock, flags);
+	res = __ib_cache_gid_get(device, port_num, index, gid, gid_attr);
+	read_unlock_irqrestore(&table->rwlock, flags);
+
+	return res;
 }
 EXPORT_SYMBOL(ib_get_cached_gid);
 
@@ -963,10 +996,12 @@ static void ib_cache_update(struct ib_device *device,
 
 	device->cache.pkey_cache[port - rdma_start_port(device)] = pkey_cache;
 	if (!use_roce_gid_table) {
+		write_lock(&table->rwlock);
 		for (i = 0; i < gid_cache->table_len; i++) {
 			modify_gid(device, port, table, i, gid_cache->table + i,
 				   &zattr, false);
 		}
+		write_unlock(&table->rwlock);
 	}
 
 	device->cache.lmc_cache[port - rdma_start_port(device)] = tprops->lmc;
-- 
2.1.0

--
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 for-next 2/3] IB/core: Change per-entry lock in RoCE GID table to one lock

[Or Gerlitz]

On 10/28/2015 4:52 PM, Matan Barak wrote:
> @@ -134,16 +138,14 @@ static int write_gid(struct ib_device *ib_dev, u8 port,
>   {
>   	int ret = 0;
>   	struct net_device *old_net_dev;
> -	unsigned long flags;
>   
>   	/* in rdma_cap_roce_gid_table, this funciton should be protected by a
>   	 * sleep-able lock.
>   	 */
> -	write_lock_irqsave(&table->data_vec[ix].lock, flags);
>   
>   	if (rdma_cap_roce_gid_table(ib_dev, port)) {
>   		table->data_vec[ix].props |= GID_TABLE_ENTRY_INVALID;
> -		write_unlock_irqrestore(&table->data_vec[ix].lock, flags);
> +		write_unlock_irq(&table->rwlock);
>   		/* GID_TABLE_WRITE_ACTION_MODIFY currently isn't supported by
>   		 * RoCE providers and thus only updates the cache.
>   		 */
> @@ -153,7 +155,7 @@ static int write_gid(struct ib_device *ib_dev, u8 port,
>   		else if (action == GID_TABLE_WRITE_ACTION_DEL)
>   			ret = ib_dev->del_gid(ib_dev, port, ix,
>   					      &table->data_vec[ix].context);
> -		write_lock_irqsave(&table->data_vec[ix].lock, flags);
> +		write_lock_irq(&table->rwlock);
>   	}

sparse complains on

drivers/infiniband/core/cache.c:186:17: warning: context imbalance in 
'write_gid' - unexpected unlock

is this false positive?

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 for-next 2/3] IB/core: Change per-entry lock in RoCE GID table to one lock

[Bart Van Assche]

On 12/30/2015 07:01 AM, Or Gerlitz wrote:
> On 10/28/2015 4:52 PM, Matan Barak wrote:
>> @@ -134,16 +138,14 @@ static int write_gid(struct ib_device *ib_dev,
>> u8 port,
>>   {
>>       int ret = 0;
>>       struct net_device *old_net_dev;
>> -    unsigned long flags;
>>       /* in rdma_cap_roce_gid_table, this funciton should be protected
>> by a
>>        * sleep-able lock.
>>        */
>> -    write_lock_irqsave(&table->data_vec[ix].lock, flags);
>>       if (rdma_cap_roce_gid_table(ib_dev, port)) {
>>           table->data_vec[ix].props |= GID_TABLE_ENTRY_INVALID;
>> -        write_unlock_irqrestore(&table->data_vec[ix].lock, flags);
>> +        write_unlock_irq(&table->rwlock);
>>           /* GID_TABLE_WRITE_ACTION_MODIFY currently isn't supported by
>>            * RoCE providers and thus only updates the cache.
>>            */
>> @@ -153,7 +155,7 @@ static int write_gid(struct ib_device *ib_dev, u8
>> port,
>>           else if (action == GID_TABLE_WRITE_ACTION_DEL)
>>               ret = ib_dev->del_gid(ib_dev, port, ix,
>>                             &table->data_vec[ix].context);
>> -        write_lock_irqsave(&table->data_vec[ix].lock, flags);
>> +        write_lock_irq(&table->rwlock);
>>       }
>
> sparse complains on
>
> drivers/infiniband/core/cache.c:186:17: warning: context imbalance in
> 'write_gid' - unexpected unlock
>
> is this false positive?

Hello Or,

sparse expects __release() and __acquire() annotations for functions 
that unlock a lock object that has been locked by its caller. See e.g. 
http://lists.kernelnewbies.org/pipermail/kernelnewbies/2011-October/003541.html.

Bart.
--
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 for-next 2/3] IB/core: Change per-entry lock in RoCE GID table to one lock

[Matan Barak]

On 12/30/2015 9:36 AM, Bart Van Assche wrote:
> On 12/30/2015 07:01 AM, Or Gerlitz wrote:
>> On 10/28/2015 4:52 PM, Matan Barak wrote:
>>> @@ -134,16 +138,14 @@ static int write_gid(struct ib_device *ib_dev,
>>> u8 port,
>>>   {
>>>       int ret = 0;
>>>       struct net_device *old_net_dev;
>>> -    unsigned long flags;
>>>       /* in rdma_cap_roce_gid_table, this funciton should be protected
>>> by a
>>>        * sleep-able lock.
>>>        */
>>> -    write_lock_irqsave(&table->data_vec[ix].lock, flags);
>>>       if (rdma_cap_roce_gid_table(ib_dev, port)) {
>>>           table->data_vec[ix].props |= GID_TABLE_ENTRY_INVALID;
>>> -        write_unlock_irqrestore(&table->data_vec[ix].lock, flags);
>>> +        write_unlock_irq(&table->rwlock);
>>>           /* GID_TABLE_WRITE_ACTION_MODIFY currently isn't supported by
>>>            * RoCE providers and thus only updates the cache.
>>>            */
>>> @@ -153,7 +155,7 @@ static int write_gid(struct ib_device *ib_dev, u8
>>> port,
>>>           else if (action == GID_TABLE_WRITE_ACTION_DEL)
>>>               ret = ib_dev->del_gid(ib_dev, port, ix,
>>>                             &table->data_vec[ix].context);
>>> -        write_lock_irqsave(&table->data_vec[ix].lock, flags);
>>> +        write_lock_irq(&table->rwlock);
>>>       }
>>
>> sparse complains on
>>
>> drivers/infiniband/core/cache.c:186:17: warning: context imbalance in
>> 'write_gid' - unexpected unlock
>>
>> is this false positive?
>

It is false positive.

> Hello Or,
>
> sparse expects __release() and __acquire() annotations for functions
> that unlock a lock object that has been locked by its caller. See e.g.
> http://lists.kernelnewbies.org/pipermail/kernelnewbies/2011-October/003541.html.
>

Thanks - adding __releases and __acquires eliminates this sparse warning.

>
> Bart.

Matan
--
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 for-next 3/3] IB/core: Find gid searches for empty slot while traversing the GID table

[Matan Barak]

Previously, we've searched the GID table twice, once - in order to
find the GID itself and afterwards in order to find a free GID
entry. Replacing it with finding the GID and the empty index
in one traversal.

Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/cache.c | 53 +++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 18 deletions(-)

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index 83aee7c..097e9df 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -208,19 +208,33 @@ static int del_gid(struct ib_device *ib_dev, u8 port,
 /* rwlock should be read locked */
 static int find_gid(struct ib_gid_table *table, const union ib_gid *gid,
 		    const struct ib_gid_attr *val, bool default_gid,
-		    unsigned long mask)
+		    unsigned long mask, int *pempty)
 {
-	int i;
+	int i = 0;
+	int found = -1;
+	int empty = pempty ? -1 : 0;
 
-	for (i = 0; i < table->sz; i++) {
-		struct ib_gid_attr *attr = &table->data_vec[i].attr;
+	while (i < table->sz && (found < 0 || empty < 0)) {
+		struct ib_gid_table_entry *data = &table->data_vec[i];
+		struct ib_gid_attr *attr = &data->attr;
+		int curr_index = i;
 
+		i++;
 
-		if (table->data_vec[i].props & GID_TABLE_ENTRY_INVALID)
+		if (data->props & GID_TABLE_ENTRY_INVALID)
+			continue;
+
+		if (empty < 0)
+			if (!memcmp(&data->gid, &zgid, sizeof(*gid)) &&
+			    !memcmp(attr, &zattr, sizeof(*attr)) &&
+			    !data->props)
+				empty = curr_index;
+
+		if (found >=0)
 			continue;
 
 		if (mask & GID_ATTR_FIND_MASK_GID &&
-		    memcmp(gid, &table->data_vec[i].gid, sizeof(*gid)))
+		    memcmp(gid, &data->gid, sizeof(*gid)))
 			continue;
 
 		if (mask & GID_ATTR_FIND_MASK_NETDEV &&
@@ -228,14 +242,17 @@ static int find_gid(struct ib_gid_table *table, const union ib_gid *gid,
 			continue;
 
 		if (mask & GID_ATTR_FIND_MASK_DEFAULT &&
-		    !!(table->data_vec[i].props & GID_TABLE_ENTRY_DEFAULT) !=
+		    !!(data->props & GID_TABLE_ENTRY_DEFAULT) !=
 		    default_gid)
 			continue;
 
-		break;
+		found = curr_index;
 	}
 
-	return i == table->sz ? -1 : i;
+	if (pempty)
+		*pempty = empty;
+
+	return found;
 }
 
 static void make_default_gid(struct  net_device *dev, union ib_gid *gid)
@@ -252,6 +269,7 @@ int ib_cache_gid_add(struct ib_device *ib_dev, u8 port,
 	int ix;
 	int ret = 0;
 	struct net_device *idev;
+	int empty;
 
 	table = ports_table[port - rdma_start_port(ib_dev)];
 
@@ -278,18 +296,16 @@ int ib_cache_gid_add(struct ib_device *ib_dev, u8 port,
 	write_lock_irq(&table->rwlock);
 
 	ix = find_gid(table, gid, attr, false, GID_ATTR_FIND_MASK_GID |
-		      GID_ATTR_FIND_MASK_NETDEV);
+		      GID_ATTR_FIND_MASK_NETDEV, &empty);
 	if (ix >= 0)
 		goto out_unlock;
 
-	ix = find_gid(table, &zgid, NULL, false, GID_ATTR_FIND_MASK_GID |
-		      GID_ATTR_FIND_MASK_DEFAULT);
-	if (ix < 0) {
+	if (empty < 0) {
 		ret = -ENOSPC;
 		goto out_unlock;
 	}
 
-	ret = add_gid(ib_dev, port, table, ix, gid, attr, false);
+	ret = add_gid(ib_dev, port, table, empty, gid, attr, false);
 	if (!ret)
 		dispatch_gid_change_event(ib_dev, port);
 
@@ -314,7 +330,8 @@ int ib_cache_gid_del(struct ib_device *ib_dev, u8 port,
 	ix = find_gid(table, gid, attr, false,
 		      GID_ATTR_FIND_MASK_GID	  |
 		      GID_ATTR_FIND_MASK_NETDEV	  |
-		      GID_ATTR_FIND_MASK_DEFAULT);
+		      GID_ATTR_FIND_MASK_DEFAULT,
+		      NULL);
 	if (ix < 0)
 		goto out_unlock;
 
@@ -393,7 +410,7 @@ static int _ib_cache_gid_table_find(struct ib_device *ib_dev,
 	for (p = 0; p < ib_dev->phys_port_cnt; p++) {
 		table = ports_table[p];
 		read_lock_irqsave(&table->rwlock, flags);
-		local_index = find_gid(table, gid, val, false, mask);
+		local_index = find_gid(table, gid, val, false, mask, NULL);
 		if (local_index >= 0) {
 			if (index)
 				*index = local_index;
@@ -445,7 +462,7 @@ int ib_find_cached_gid_by_port(struct ib_device *ib_dev,
 		mask |= GID_ATTR_FIND_MASK_NETDEV;
 
 	read_lock_irqsave(&table->rwlock, flags);
-	local_index = find_gid(table, gid, &val, false, mask);
+	local_index = find_gid(table, gid, &val, false, mask, NULL);
 	if (local_index >= 0) {
 		if (index)
 			*index = local_index;
@@ -608,7 +625,7 @@ void ib_cache_gid_set_default_gid(struct ib_device *ib_dev, u8 port,
 
 	mutex_lock(&table->lock);
 	write_lock_irq(&table->rwlock);
-	ix = find_gid(table, NULL, NULL, true, GID_ATTR_FIND_MASK_DEFAULT);
+	ix = find_gid(table, NULL, NULL, true, GID_ATTR_FIND_MASK_DEFAULT, NULL);
 
 	/* Coudn't find default GID location */
 	WARN_ON(ix < 0);
-- 
2.1.0

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