All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 for-next 0/3] IB/core: Obtaining subnet_prefix from cache in
@ 2021-06-16  6:52 Anand Khoje
  2021-06-16  6:52 ` [PATCH v4 for-next 1/3] IB/core: Removed port validity check from ib_get_cached_subnet_prefix Anand Khoje
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Anand Khoje @ 2021-06-16  6:52 UTC (permalink / raw)
  To: linux-rdma, linux-kernel; +Cc: dledford, jgg, haakon.bugge, leon

This v4 patch series is used to read the port_attribute subnet_prefix
from a valid cache entry instead of having to call
device->ops.query_gid() in Infiniband link-layer devices. This requires
addition of a flag used to check that the cache entry is initialized and
that a valid value is being read.

1. Removed the port validity check from ib_get_cached_subnet_prefix.
This check was not useful as the port_num is always valid.

2. Shuffled locks pkey_lost_lock and netdev_lock in struct ib_port_data.
This was done as output of pahole showed two 4-byte holes in the
structure ib_port_data after pkey_list_lock and netdev_lock. Moving
netdev_lock shaved off 8 bytes from the structure.

3. Added a flag to struct ib_port_data. This is used to validate the
status of cached subnet_prefix. This valid cache entry of subnet_prefix
is used in function __ib_query_port().
This allows the utilization of the cache entry and hence avoids a call
into device->ops.query_gid(). We also ensure that in the event of a
cache update, the value for subnet_prefix gets read from the newly updated
GID cache and not via ib_query_port(), so that we do not end up reading a
stale cache value.

Anand Khoje (3):
  IB/core: Removed port validity check from ib_get_cached_subnet_prefix
  IB/core: Shuffle locks in ib_port_data to save memory
  IB/core: Obtain subnet_prefix from cache in IB devices

 drivers/infiniband/core/cache.c     | 25 ++++++++++++++++---------
 drivers/infiniband/core/core_priv.h |  2 +-
 drivers/infiniband/core/device.c    | 20 +++++++++++---------
 drivers/infiniband/core/security.c  |  7 ++-----
 include/rdma/ib_cache.h             |  5 +++++
 include/rdma/ib_verbs.h             |  6 +++++-
 6 files changed, 40 insertions(+), 25 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v4 for-next 1/3] IB/core: Removed port validity check from ib_get_cached_subnet_prefix
  2021-06-16  6:52 [PATCH v4 for-next 0/3] IB/core: Obtaining subnet_prefix from cache in Anand Khoje
@ 2021-06-16  6:52 ` Anand Khoje
  2021-06-16  6:52 ` [PATCH v4 for-next 2/3] IB/core: Shuffle locks in ib_port_data to save memory Anand Khoje
  2021-06-16  6:52 ` [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices Anand Khoje
  2 siblings, 0 replies; 14+ messages in thread
From: Anand Khoje @ 2021-06-16  6:52 UTC (permalink / raw)
  To: linux-rdma, linux-kernel; +Cc: dledford, jgg, haakon.bugge, leon

Removed port validity check from ib_get_cached_subnet_prefix()
as this check is not needed because "port_num" is valid.

Suggested-by: Leon Romanovsky <leonro@nvidia.com>
Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
---

v1 -> v2:
    -   Added changes as per Leon's suggestion of removing port
    validity check from ib_get_cached_subnet_prefix().
    -   Split the v1 patch in 3 patches as per Leon's suggestion.
v2 -> v3:
    -   Added some formatting changes per Leon's suggestions
    and removed return from ib_get_cached_subnet_prefix.
v3 -> v4:
    -   Removed a newline in ib_policy_change_task().

---
 drivers/infiniband/core/cache.c     |  7 +------
 drivers/infiniband/core/core_priv.h |  2 +-
 drivers/infiniband/core/device.c    | 11 ++---------
 drivers/infiniband/core/security.c  |  7 ++-----
 4 files changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index d320459..2325171 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -1069,19 +1069,14 @@ int ib_get_cached_pkey(struct ib_device *device,
 }
 EXPORT_SYMBOL(ib_get_cached_pkey);
 
-int ib_get_cached_subnet_prefix(struct ib_device *device, u32 port_num,
+void ib_get_cached_subnet_prefix(struct ib_device *device, u32 port_num,
 				u64 *sn_pfx)
 {
 	unsigned long flags;
 
-	if (!rdma_is_port_valid(device, port_num))
-		return -EINVAL;
-
 	read_lock_irqsave(&device->cache_lock, flags);
 	*sn_pfx = device->port_data[port_num].cache.subnet_prefix;
 	read_unlock_irqrestore(&device->cache_lock, flags);
-
-	return 0;
 }
 EXPORT_SYMBOL(ib_get_cached_subnet_prefix);
 
diff --git a/drivers/infiniband/core/core_priv.h b/drivers/infiniband/core/core_priv.h
index 29809dd..0b23f50 100644
--- a/drivers/infiniband/core/core_priv.h
+++ b/drivers/infiniband/core/core_priv.h
@@ -214,7 +214,7 @@ int ib_nl_handle_ip_res_resp(struct sk_buff *skb,
 			     struct nlmsghdr *nlh,
 			     struct netlink_ext_ack *extack);
 
-int ib_get_cached_subnet_prefix(struct ib_device *device,
+void ib_get_cached_subnet_prefix(struct ib_device *device,
 				u32 port_num,
 				u64 *sn_pfx);
 
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index c660cef..7a617e4 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -886,15 +886,8 @@ static void ib_policy_change_task(struct work_struct *work)
 
 		rdma_for_each_port (dev, i) {
 			u64 sp;
-			int ret = ib_get_cached_subnet_prefix(dev,
-							      i,
-							      &sp);
-
-			WARN_ONCE(ret,
-				  "ib_get_cached_subnet_prefix err: %d, this should never happen here\n",
-				  ret);
-			if (!ret)
-				ib_security_cache_change(dev, i, sp);
+			ib_get_cached_subnet_prefix(dev, i, &sp);
+			ib_security_cache_change(dev, i, sp);
 		}
 	}
 	up_read(&devices_rwsem);
diff --git a/drivers/infiniband/core/security.c b/drivers/infiniband/core/security.c
index e5a78d1..5433912 100644
--- a/drivers/infiniband/core/security.c
+++ b/drivers/infiniband/core/security.c
@@ -72,7 +72,7 @@ static int get_pkey_and_subnet_prefix(struct ib_port_pkey *pp,
 	if (ret)
 		return ret;
 
-	ret = ib_get_cached_subnet_prefix(dev, pp->port_num, subnet_prefix);
+	ib_get_cached_subnet_prefix(dev, pp->port_num, subnet_prefix);
 
 	return ret;
 }
@@ -664,10 +664,7 @@ static int ib_security_pkey_access(struct ib_device *dev,
 	if (ret)
 		return ret;
 
-	ret = ib_get_cached_subnet_prefix(dev, port_num, &subnet_prefix);
-
-	if (ret)
-		return ret;
+	ib_get_cached_subnet_prefix(dev, port_num, &subnet_prefix);
 
 	return security_ib_pkey_access(sec, subnet_prefix, pkey);
 }
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v4 for-next 2/3] IB/core: Shuffle locks in ib_port_data to save memory
  2021-06-16  6:52 [PATCH v4 for-next 0/3] IB/core: Obtaining subnet_prefix from cache in Anand Khoje
  2021-06-16  6:52 ` [PATCH v4 for-next 1/3] IB/core: Removed port validity check from ib_get_cached_subnet_prefix Anand Khoje
@ 2021-06-16  6:52 ` Anand Khoje
  2021-06-16  6:52 ` [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices Anand Khoje
  2 siblings, 0 replies; 14+ messages in thread
From: Anand Khoje @ 2021-06-16  6:52 UTC (permalink / raw)
  To: linux-rdma, linux-kernel; +Cc: dledford, jgg, haakon.bugge, leon

pahole shows two 4-byte holes in struct ib_port_data after
pkey_list_lock and netdev_lock respectively.

Shuffling the netdev_lock to be after pkey_list_lock, this
shaves off eight bytes from the struct.

Suggested-by: Haakon Bugge <haakon.bugge@oracle.com>
Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
---

v1 -> v2:
    -   Split the v1 patch in 3 patches as per Leon's suggestion.
v2 -> v3:
    -   No changes.
v3 -> v4:
    -   No changes.

---
 include/rdma/ib_verbs.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 05dbc21..c96d601 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2174,11 +2174,13 @@ struct ib_port_data {
 	struct ib_port_immutable immutable;
 
 	spinlock_t pkey_list_lock;
+
+	spinlock_t netdev_lock;
+
 	struct list_head pkey_list;
 
 	struct ib_port_cache cache;
 
-	spinlock_t netdev_lock;
 	struct net_device __rcu *netdev;
 	struct hlist_node ndev_hash_link;
 	struct rdma_port_counter port_counter;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices
  2021-06-16  6:52 [PATCH v4 for-next 0/3] IB/core: Obtaining subnet_prefix from cache in Anand Khoje
  2021-06-16  6:52 ` [PATCH v4 for-next 1/3] IB/core: Removed port validity check from ib_get_cached_subnet_prefix Anand Khoje
  2021-06-16  6:52 ` [PATCH v4 for-next 2/3] IB/core: Shuffle locks in ib_port_data to save memory Anand Khoje
@ 2021-06-16  6:52 ` Anand Khoje
  2021-06-16  7:27   ` Leon Romanovsky
                     ` (3 more replies)
  2 siblings, 4 replies; 14+ messages in thread
From: Anand Khoje @ 2021-06-16  6:52 UTC (permalink / raw)
  To: linux-rdma, linux-kernel; +Cc: dledford, jgg, haakon.bugge, leon

ib_query_port() calls device->ops.query_port() to get the port
attributes. The method of querying is device driver specific.
The same function calls device->ops.query_gid() to get the GID and
extract the subnet_prefix (gid_prefix).

The GID and subnet_prefix are stored in a cache. But they do not get
read from the cache if the device is an Infiniband device. The
following change takes advantage of the cached subnet_prefix.
Testing with RDBMS has shown a significant improvement in performance
with this change.

The function ib_cache_is_initialised() is introduced because
ib_query_port() gets called early in the stage when the cache is not
built while reading port immutable property.

In that case, the default GID still gets read from HCA for IB link-
layer devices.

In the situation of an event causing cache update, the subnet_prefix
will get retrieved from newly updated GID cache in ib_cache_update(),
so that we do not end up reading a stale value from cache via
ib_query_port().

Fixes: fad61ad ("IB/core: Add subnet prefix to port info")
Suggested-by: Leon Romanovsky <leonro@nvidia.com>
Suggested-by: Aru Kolappan <aru.kolappan@oracle.com>
Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
---

v1 -> v2:
    -   Split the v1 patch in 3 patches as per Leon's suggestion.

v2 -> v3:
    -   Added changes as per Mark Zhang's suggestion of clearing
        flags in git_table_cleanup_one().
v3 -> v4:
    -   Removed the enum ib_port_data_flags and 8 byte flags from
        struct ib_port_data, and the set_bit()/clear_bit() API
        used to update this flag as that was not necessary.
        Done to keep the code simple.
    -   Added code to read subnet_prefix from updated GID cache in the
        event of cache update. Prior to this change, ib_cache_update
        was reading the value for subnet_prefix via ib_query_port(),
        due to this patch, we ended up reading a stale cached value of
        subnet_prefix.

---
 drivers/infiniband/core/cache.c  | 18 +++++++++++++++---
 drivers/infiniband/core/device.c |  9 +++++++++
 include/rdma/ib_cache.h          |  5 +++++
 include/rdma/ib_verbs.h          |  1 +
 4 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index 2325171..cd99c46 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -917,9 +917,11 @@ static void gid_table_cleanup_one(struct ib_device *ib_dev)
 {
 	u32 p;
 
-	rdma_for_each_port (ib_dev, p)
+	rdma_for_each_port (ib_dev, p) {
+		ib_dev->port_data[p].cache_is_initialized = 0;
 		cleanup_gid_table_port(ib_dev, p,
 				       ib_dev->port_data[p].cache.gid);
+	}
 }
 
 static int gid_table_setup_one(struct ib_device *ib_dev)
@@ -1466,6 +1468,7 @@ static int config_non_roce_gid_cache(struct ib_device *device,
 	struct ib_port_attr       *tprops = NULL;
 	struct ib_pkey_cache      *pkey_cache = NULL;
 	struct ib_pkey_cache      *old_pkey_cache = NULL;
+	union ib_gid               gid;
 	int                        i;
 	int                        ret;
 
@@ -1523,13 +1526,21 @@ static int config_non_roce_gid_cache(struct ib_device *device,
 	device->port_data[port].cache.lmc = tprops->lmc;
 	device->port_data[port].cache.port_state = tprops->state;
 
-	device->port_data[port].cache.subnet_prefix = tprops->subnet_prefix;
+	ret = rdma_query_gid(device, port, 0, &gid);
+	if (ret) {
+		write_unlock_irq(&device->cache.lock);
+		goto err;
+	}
+
+	device->port_data[port].cache.subnet_prefix =
+			be64_to_cpu(gid.global.subnet_prefix);
+
 	write_unlock_irq(&device->cache_lock);
 
 	if (enforce_security)
 		ib_security_cache_change(device,
 					 port,
-					 tprops->subnet_prefix);
+					 be64_to_cpu(gid.global.subnet_prefix));
 
 	kfree(old_pkey_cache);
 	kfree(tprops);
@@ -1629,6 +1640,7 @@ int ib_cache_setup_one(struct ib_device *device)
 		err = ib_cache_update(device, p, true, true, true);
 		if (err)
 			return err;
+		device->port_data[p].cache_is_initialized = 1;
 	}
 
 	return 0;
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 7a617e4..57b9039 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -2057,6 +2057,15 @@ static int __ib_query_port(struct ib_device *device,
 	    IB_LINK_LAYER_INFINIBAND)
 		return 0;
 
+	if (!ib_cache_is_initialised(device, port_num))
+		goto query_gid_from_device;
+
+	ib_get_cached_subnet_prefix(device, port_num,
+				    &port_attr->subnet_prefix);
+
+	return 0;
+
+query_gid_from_device:
 	err = device->ops.query_gid(device, port_num, 0, &gid);
 	if (err)
 		return err;
diff --git a/include/rdma/ib_cache.h b/include/rdma/ib_cache.h
index 226ae37..46b43a7 100644
--- a/include/rdma/ib_cache.h
+++ b/include/rdma/ib_cache.h
@@ -114,4 +114,9 @@ ssize_t rdma_query_gid_table(struct ib_device *device,
 			     struct ib_uverbs_gid_entry *entries,
 			     size_t max_entries);
 
+static inline bool ib_cache_is_initialised(struct ib_device *device,
+					u32 port_num)
+{
+	return device->port_data[port_num].cache_is_initialized;
+}
 #endif /* _IB_CACHE_H */
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index c96d601..405f7da 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2177,6 +2177,7 @@ struct ib_port_data {
 
 	spinlock_t netdev_lock;
 
+	u8 cache_is_initialized:1;
 	struct list_head pkey_list;
 
 	struct ib_port_cache cache;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices
  2021-06-16  6:52 ` [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices Anand Khoje
@ 2021-06-16  7:27   ` Leon Romanovsky
  2021-06-16  7:42     ` Anand Khoje
  2021-06-16  7:30   ` Leon Romanovsky
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Leon Romanovsky @ 2021-06-16  7:27 UTC (permalink / raw)
  To: Anand Khoje; +Cc: linux-rdma, linux-kernel, dledford, jgg, haakon.bugge

On Wed, Jun 16, 2021 at 12:22:13PM +0530, Anand Khoje wrote:
> ib_query_port() calls device->ops.query_port() to get the port
> attributes. The method of querying is device driver specific.
> The same function calls device->ops.query_gid() to get the GID and
> extract the subnet_prefix (gid_prefix).
> 
> The GID and subnet_prefix are stored in a cache. But they do not get
> read from the cache if the device is an Infiniband device. The
> following change takes advantage of the cached subnet_prefix.
> Testing with RDBMS has shown a significant improvement in performance
> with this change.
> 
> The function ib_cache_is_initialised() is introduced because
> ib_query_port() gets called early in the stage when the cache is not
> built while reading port immutable property.
> 
> In that case, the default GID still gets read from HCA for IB link-
> layer devices.
> 
> In the situation of an event causing cache update, the subnet_prefix
> will get retrieved from newly updated GID cache in ib_cache_update(),
> so that we do not end up reading a stale value from cache via
> ib_query_port().
> 
> Fixes: fad61ad ("IB/core: Add subnet prefix to port info")
> Suggested-by: Leon Romanovsky <leonro@nvidia.com>
> Suggested-by: Aru Kolappan <aru.kolappan@oracle.com>
> Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
> Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
> ---
> 
> v1 -> v2:
>     -   Split the v1 patch in 3 patches as per Leon's suggestion.
> 
> v2 -> v3:
>     -   Added changes as per Mark Zhang's suggestion of clearing
>         flags in git_table_cleanup_one().
> v3 -> v4:
>     -   Removed the enum ib_port_data_flags and 8 byte flags from
>         struct ib_port_data, and the set_bit()/clear_bit() API
>         used to update this flag as that was not necessary.
>         Done to keep the code simple.
>     -   Added code to read subnet_prefix from updated GID cache in the
>         event of cache update. Prior to this change, ib_cache_update
>         was reading the value for subnet_prefix via ib_query_port(),
>         due to this patch, we ended up reading a stale cached value of
>         subnet_prefix.
> 
> ---
>  drivers/infiniband/core/cache.c  | 18 +++++++++++++++---
>  drivers/infiniband/core/device.c |  9 +++++++++
>  include/rdma/ib_cache.h          |  5 +++++
>  include/rdma/ib_verbs.h          |  1 +
>  4 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> index 2325171..cd99c46 100644
> --- a/drivers/infiniband/core/cache.c
> +++ b/drivers/infiniband/core/cache.c
> @@ -917,9 +917,11 @@ static void gid_table_cleanup_one(struct ib_device *ib_dev)
>  {
>  	u32 p;
>  
> -	rdma_for_each_port (ib_dev, p)
> +	rdma_for_each_port (ib_dev, p) {
> +		ib_dev->port_data[p].cache_is_initialized = 0;

I think that this line is not needed, we are removing device anyway and
and query_port is not allowed at this stage.

>  		cleanup_gid_table_port(ib_dev, p,
>  				       ib_dev->port_data[p].cache.gid);
> +	}
>  }
>  
>  static int gid_table_setup_one(struct ib_device *ib_dev)
> @@ -1466,6 +1468,7 @@ static int config_non_roce_gid_cache(struct ib_device *device,
>  	struct ib_port_attr       *tprops = NULL;
>  	struct ib_pkey_cache      *pkey_cache = NULL;
>  	struct ib_pkey_cache      *old_pkey_cache = NULL;
> +	union ib_gid               gid;
>  	int                        i;
>  	int                        ret;
>  
> @@ -1523,13 +1526,21 @@ static int config_non_roce_gid_cache(struct ib_device *device,
>  	device->port_data[port].cache.lmc = tprops->lmc;
>  	device->port_data[port].cache.port_state = tprops->state;
>  
> -	device->port_data[port].cache.subnet_prefix = tprops->subnet_prefix;
> +	ret = rdma_query_gid(device, port, 0, &gid);
> +	if (ret) {
> +		write_unlock_irq(&device->cache.lock);
> +		goto err;
> +	}
> +
> +	device->port_data[port].cache.subnet_prefix =
> +			be64_to_cpu(gid.global.subnet_prefix);
> +
>  	write_unlock_irq(&device->cache_lock);
>  
>  	if (enforce_security)
>  		ib_security_cache_change(device,
>  					 port,
> -					 tprops->subnet_prefix);
> +					 be64_to_cpu(gid.global.subnet_prefix));
>  
>  	kfree(old_pkey_cache);
>  	kfree(tprops);
> @@ -1629,6 +1640,7 @@ int ib_cache_setup_one(struct ib_device *device)
>  		err = ib_cache_update(device, p, true, true, true);
>  		if (err)
>  			return err;
> +		device->port_data[p].cache_is_initialized = 1;
>  	}
>  
>  	return 0;
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 7a617e4..57b9039 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -2057,6 +2057,15 @@ static int __ib_query_port(struct ib_device *device,
>  	    IB_LINK_LAYER_INFINIBAND)
>  		return 0;
>  
> +	if (!ib_cache_is_initialised(device, port_num))
> +		goto query_gid_from_device;

IMHO, we don't need this new function and can access ib_port_data
directly. In device.c, we have plenty of places that does it.

Not critical.

> +
> +	ib_get_cached_subnet_prefix(device, port_num,
> +				    &port_attr->subnet_prefix);
> +
> +	return 0;
> +
> +query_gid_from_device:
>  	err = device->ops.query_gid(device, port_num, 0, &gid);
>  	if (err)
>  		return err;
> diff --git a/include/rdma/ib_cache.h b/include/rdma/ib_cache.h
> index 226ae37..46b43a7 100644
> --- a/include/rdma/ib_cache.h
> +++ b/include/rdma/ib_cache.h
> @@ -114,4 +114,9 @@ ssize_t rdma_query_gid_table(struct ib_device *device,
>  			     struct ib_uverbs_gid_entry *entries,
>  			     size_t max_entries);
>  
> +static inline bool ib_cache_is_initialised(struct ib_device *device,
> +					u32 port_num)
> +{
> +	return device->port_data[port_num].cache_is_initialized;
> +}
>  #endif /* _IB_CACHE_H */
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index c96d601..405f7da 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -2177,6 +2177,7 @@ struct ib_port_data {
>  
>  	spinlock_t netdev_lock;
>  
> +	u8 cache_is_initialized:1;
>  	struct list_head pkey_list;
>  
>  	struct ib_port_cache cache;
> -- 
> 1.8.3.1
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices
  2021-06-16  6:52 ` [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices Anand Khoje
  2021-06-16  7:27   ` Leon Romanovsky
@ 2021-06-16  7:30   ` Leon Romanovsky
  2021-06-16  7:43     ` Anand Khoje
  2021-06-17 10:05     ` kernel test robot
  2021-06-17 10:11     ` kernel test robot
  3 siblings, 1 reply; 14+ messages in thread
From: Leon Romanovsky @ 2021-06-16  7:30 UTC (permalink / raw)
  To: Anand Khoje; +Cc: linux-rdma, linux-kernel, dledford, jgg, haakon.bugge

On Wed, Jun 16, 2021 at 12:22:13PM +0530, Anand Khoje wrote:
> ib_query_port() calls device->ops.query_port() to get the port
> attributes. The method of querying is device driver specific.
> The same function calls device->ops.query_gid() to get the GID and
> extract the subnet_prefix (gid_prefix).
> 
> The GID and subnet_prefix are stored in a cache. But they do not get
> read from the cache if the device is an Infiniband device. The
> following change takes advantage of the cached subnet_prefix.
> Testing with RDBMS has shown a significant improvement in performance
> with this change.
> 
> The function ib_cache_is_initialised() is introduced because
> ib_query_port() gets called early in the stage when the cache is not
> built while reading port immutable property.
> 
> In that case, the default GID still gets read from HCA for IB link-
> layer devices.
> 
> In the situation of an event causing cache update, the subnet_prefix
> will get retrieved from newly updated GID cache in ib_cache_update(),
> so that we do not end up reading a stale value from cache via
> ib_query_port().
> 
> Fixes: fad61ad ("IB/core: Add subnet prefix to port info")
> Suggested-by: Leon Romanovsky <leonro@nvidia.com>
> Suggested-by: Aru Kolappan <aru.kolappan@oracle.com>
> Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
> Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
> ---

<...>

> @@ -1523,13 +1526,21 @@ static int config_non_roce_gid_cache(struct ib_device *device,
>  	device->port_data[port].cache.lmc = tprops->lmc;
>  	device->port_data[port].cache.port_state = tprops->state;
>  
> -	device->port_data[port].cache.subnet_prefix = tprops->subnet_prefix;
> +	ret = rdma_query_gid(device, port, 0, &gid);
> +	if (ret) {
> +		write_unlock_irq(&device->cache.lock);

And this patch can't compile. It should be cache_lock and not cache.lock.

Thanks

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices
  2021-06-16  7:27   ` Leon Romanovsky
@ 2021-06-16  7:42     ` Anand Khoje
  2021-06-16  8:05       ` Anand Khoje
  2021-06-16  8:41       ` Leon Romanovsky
  0 siblings, 2 replies; 14+ messages in thread
From: Anand Khoje @ 2021-06-16  7:42 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: linux-rdma, linux-kernel, dledford, jgg, haakon.bugge

On 6/16/2021 12:57 PM, Leon Romanovsky wrote:
> On Wed, Jun 16, 2021 at 12:22:13PM +0530, Anand Khoje wrote:
>> ib_query_port() calls device->ops.query_port() to get the port
>> attributes. The method of querying is device driver specific.
>> The same function calls device->ops.query_gid() to get the GID and
>> extract the subnet_prefix (gid_prefix).
>>
>> The GID and subnet_prefix are stored in a cache. But they do not get
>> read from the cache if the device is an Infiniband device. The
>> following change takes advantage of the cached subnet_prefix.
>> Testing with RDBMS has shown a significant improvement in performance
>> with this change.
>>
>> The function ib_cache_is_initialised() is introduced because
>> ib_query_port() gets called early in the stage when the cache is not
>> built while reading port immutable property.
>>
>> In that case, the default GID still gets read from HCA for IB link-
>> layer devices.
>>
>> In the situation of an event causing cache update, the subnet_prefix
>> will get retrieved from newly updated GID cache in ib_cache_update(),
>> so that we do not end up reading a stale value from cache via
>> ib_query_port().
>>
>> Fixes: fad61ad ("IB/core: Add subnet prefix to port info")
>> Suggested-by: Leon Romanovsky <leonro@nvidia.com>
>> Suggested-by: Aru Kolappan <aru.kolappan@oracle.com>
>> Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
>> Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
>> ---
>>
>> v1 -> v2:
>>      -   Split the v1 patch in 3 patches as per Leon's suggestion.
>>
>> v2 -> v3:
>>      -   Added changes as per Mark Zhang's suggestion of clearing
>>          flags in git_table_cleanup_one().
>> v3 -> v4:
>>      -   Removed the enum ib_port_data_flags and 8 byte flags from
>>          struct ib_port_data, and the set_bit()/clear_bit() API
>>          used to update this flag as that was not necessary.
>>          Done to keep the code simple.
>>      -   Added code to read subnet_prefix from updated GID cache in the
>>          event of cache update. Prior to this change, ib_cache_update
>>          was reading the value for subnet_prefix via ib_query_port(),
>>          due to this patch, we ended up reading a stale cached value of
>>          subnet_prefix.
>>
>> ---
>>   drivers/infiniband/core/cache.c  | 18 +++++++++++++++---
>>   drivers/infiniband/core/device.c |  9 +++++++++
>>   include/rdma/ib_cache.h          |  5 +++++
>>   include/rdma/ib_verbs.h          |  1 +
>>   4 files changed, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
>> index 2325171..cd99c46 100644
>> --- a/drivers/infiniband/core/cache.c
>> +++ b/drivers/infiniband/core/cache.c
>> @@ -917,9 +917,11 @@ static void gid_table_cleanup_one(struct ib_device *ib_dev)
>>   {
>>   	u32 p;
>>   
>> -	rdma_for_each_port (ib_dev, p)
>> +	rdma_for_each_port (ib_dev, p) {
>> +		ib_dev->port_data[p].cache_is_initialized = 0;
> 
> I think that this line is not needed, we are removing device anyway and
> and query_port is not allowed at this stage.
> 
We have kept this for code completeness purposes. Just as we did with 
set_bit() and clear_bit() APIs.

>>   		cleanup_gid_table_port(ib_dev, p,
>>   				       ib_dev->port_data[p].cache.gid);
>> +	}
>>   }
>>   
>>   static int gid_table_setup_one(struct ib_device *ib_dev)
>> @@ -1466,6 +1468,7 @@ static int config_non_roce_gid_cache(struct ib_device *device,
>>   	struct ib_port_attr       *tprops = NULL;
>>   	struct ib_pkey_cache      *pkey_cache = NULL;
>>   	struct ib_pkey_cache      *old_pkey_cache = NULL;
>> +	union ib_gid               gid;
>>   	int                        i;
>>   	int                        ret;
>>   
>> @@ -1523,13 +1526,21 @@ static int config_non_roce_gid_cache(struct ib_device *device,
>>   	device->port_data[port].cache.lmc = tprops->lmc;
>>   	device->port_data[port].cache.port_state = tprops->state;
>>   
>> -	device->port_data[port].cache.subnet_prefix = tprops->subnet_prefix;
>> +	ret = rdma_query_gid(device, port, 0, &gid);
>> +	if (ret) {
>> +		write_unlock_irq(&device->cache.lock);
>> +		goto err;
>> +	}
>> +
>> +	device->port_data[port].cache.subnet_prefix =
>> +			be64_to_cpu(gid.global.subnet_prefix);
>> +
>>   	write_unlock_irq(&device->cache_lock);
>>   
>>   	if (enforce_security)
>>   		ib_security_cache_change(device,
>>   					 port,
>> -					 tprops->subnet_prefix);
>> +					 be64_to_cpu(gid.global.subnet_prefix));
>>   
>>   	kfree(old_pkey_cache);
>>   	kfree(tprops);
>> @@ -1629,6 +1640,7 @@ int ib_cache_setup_one(struct ib_device *device)
>>   		err = ib_cache_update(device, p, true, true, true);
>>   		if (err)
>>   			return err;
>> +		device->port_data[p].cache_is_initialized = 1;
>>   	}
>>   
>>   	return 0;
>> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
>> index 7a617e4..57b9039 100644
>> --- a/drivers/infiniband/core/device.c
>> +++ b/drivers/infiniband/core/device.c
>> @@ -2057,6 +2057,15 @@ static int __ib_query_port(struct ib_device *device,
>>   	    IB_LINK_LAYER_INFINIBAND)
>>   		return 0;
>>   
>> +	if (!ib_cache_is_initialised(device, port_num))
>> +		goto query_gid_from_device;
> 
> IMHO, we don't need this new function and can access ib_port_data
> directly. In device.c, we have plenty of places that does it.
> 
> Not critical.
> 
Added this function to have a way to check validity of cache, such that 
it could be used in future for the same check in areas to which 
ib_port_data is opaque.

>> +
>> +	ib_get_cached_subnet_prefix(device, port_num,
>> +				    &port_attr->subnet_prefix);
>> +
>> +	return 0;
>> +
>> +query_gid_from_device:
>>   	err = device->ops.query_gid(device, port_num, 0, &gid);
>>   	if (err)
>>   		return err;
>> diff --git a/include/rdma/ib_cache.h b/include/rdma/ib_cache.h
>> index 226ae37..46b43a7 100644
>> --- a/include/rdma/ib_cache.h
>> +++ b/include/rdma/ib_cache.h
>> @@ -114,4 +114,9 @@ ssize_t rdma_query_gid_table(struct ib_device *device,
>>   			     struct ib_uverbs_gid_entry *entries,
>>   			     size_t max_entries);
>>   
>> +static inline bool ib_cache_is_initialised(struct ib_device *device,
>> +					u32 port_num)
>> +{
>> +	return device->port_data[port_num].cache_is_initialized;
>> +}
>>   #endif /* _IB_CACHE_H */
>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>> index c96d601..405f7da 100644
>> --- a/include/rdma/ib_verbs.h
>> +++ b/include/rdma/ib_verbs.h
>> @@ -2177,6 +2177,7 @@ struct ib_port_data {
>>   
>>   	spinlock_t netdev_lock;
>>   
>> +	u8 cache_is_initialized:1;
>>   	struct list_head pkey_list;
>>   
>>   	struct ib_port_cache cache;
>> -- 
>> 1.8.3.1
>>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices
  2021-06-16  7:30   ` Leon Romanovsky
@ 2021-06-16  7:43     ` Anand Khoje
  0 siblings, 0 replies; 14+ messages in thread
From: Anand Khoje @ 2021-06-16  7:43 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: linux-rdma, linux-kernel, dledford, jgg, haakon.bugge

On 6/16/2021 1:00 PM, Leon Romanovsky wrote:
> On Wed, Jun 16, 2021 at 12:22:13PM +0530, Anand Khoje wrote:
>> ib_query_port() calls device->ops.query_port() to get the port
>> attributes. The method of querying is device driver specific.
>> The same function calls device->ops.query_gid() to get the GID and
>> extract the subnet_prefix (gid_prefix).
>>
>> The GID and subnet_prefix are stored in a cache. But they do not get
>> read from the cache if the device is an Infiniband device. The
>> following change takes advantage of the cached subnet_prefix.
>> Testing with RDBMS has shown a significant improvement in performance
>> with this change.
>>
>> The function ib_cache_is_initialised() is introduced because
>> ib_query_port() gets called early in the stage when the cache is not
>> built while reading port immutable property.
>>
>> In that case, the default GID still gets read from HCA for IB link-
>> layer devices.
>>
>> In the situation of an event causing cache update, the subnet_prefix
>> will get retrieved from newly updated GID cache in ib_cache_update(),
>> so that we do not end up reading a stale value from cache via
>> ib_query_port().
>>
>> Fixes: fad61ad ("IB/core: Add subnet prefix to port info")
>> Suggested-by: Leon Romanovsky <leonro@nvidia.com>
>> Suggested-by: Aru Kolappan <aru.kolappan@oracle.com>
>> Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
>> Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
>> ---
> 
> <...>
> 
>> @@ -1523,13 +1526,21 @@ static int config_non_roce_gid_cache(struct ib_device *device,
>>   	device->port_data[port].cache.lmc = tprops->lmc;
>>   	device->port_data[port].cache.port_state = tprops->state;
>>   
>> -	device->port_data[port].cache.subnet_prefix = tprops->subnet_prefix;
>> +	ret = rdma_query_gid(device, port, 0, &gid);
>> +	if (ret) {
>> +		write_unlock_irq(&device->cache.lock);
> 
> And this patch can't compile. It should be cache_lock and not cache.lock.
> 
> Thanks
> 
Unfortunate typo from my end. Thanks for pointing this out. I will share 
the updated patch.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices
  2021-06-16  7:42     ` Anand Khoje
@ 2021-06-16  8:05       ` Anand Khoje
  2021-06-16  8:41       ` Leon Romanovsky
  1 sibling, 0 replies; 14+ messages in thread
From: Anand Khoje @ 2021-06-16  8:05 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: linux-rdma, linux-kernel, dledford, jgg, haakon.bugge

On 6/16/2021 1:12 PM, Anand Khoje wrote:
> On 6/16/2021 12:57 PM, Leon Romanovsky wrote:
>> On Wed, Jun 16, 2021 at 12:22:13PM +0530, Anand Khoje wrote:
>>> ib_query_port() calls device->ops.query_port() to get the port
>>> attributes. The method of querying is device driver specific.
>>> The same function calls device->ops.query_gid() to get the GID and
>>> extract the subnet_prefix (gid_prefix).
>>>
>>> The GID and subnet_prefix are stored in a cache. But they do not get
>>> read from the cache if the device is an Infiniband device. The
>>> following change takes advantage of the cached subnet_prefix.
>>> Testing with RDBMS has shown a significant improvement in performance
>>> with this change.
>>>
>>> The function ib_cache_is_initialised() is introduced because
>>> ib_query_port() gets called early in the stage when the cache is not
>>> built while reading port immutable property.
>>>
>>> In that case, the default GID still gets read from HCA for IB link-
>>> layer devices.
>>>
>>> In the situation of an event causing cache update, the subnet_prefix
>>> will get retrieved from newly updated GID cache in ib_cache_update(),
>>> so that we do not end up reading a stale value from cache via
>>> ib_query_port().
>>>
>>> Fixes: fad61ad ("IB/core: Add subnet prefix to port info")
>>> Suggested-by: Leon Romanovsky <leonro@nvidia.com>
>>> Suggested-by: Aru Kolappan <aru.kolappan@oracle.com>
>>> Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
>>> Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
>>> ---
>>>
>>> v1 -> v2:
>>>      -   Split the v1 patch in 3 patches as per Leon's suggestion.
>>>
>>> v2 -> v3:
>>>      -   Added changes as per Mark Zhang's suggestion of clearing
>>>          flags in git_table_cleanup_one().
>>> v3 -> v4:
>>>      -   Removed the enum ib_port_data_flags and 8 byte flags from
>>>          struct ib_port_data, and the set_bit()/clear_bit() API
>>>          used to update this flag as that was not necessary.
>>>          Done to keep the code simple.
>>>      -   Added code to read subnet_prefix from updated GID cache in the
>>>          event of cache update. Prior to this change, ib_cache_update
>>>          was reading the value for subnet_prefix via ib_query_port(),
>>>          due to this patch, we ended up reading a stale cached value of
>>>          subnet_prefix.
>>>
>>> ---
>>>   drivers/infiniband/core/cache.c  | 18 +++++++++++++++---
>>>   drivers/infiniband/core/device.c |  9 +++++++++
>>>   include/rdma/ib_cache.h          |  5 +++++
>>>   include/rdma/ib_verbs.h          |  1 +
>>>   4 files changed, 30 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/core/cache.c 
>>> b/drivers/infiniband/core/cache.c
>>> index 2325171..cd99c46 100644
>>> --- a/drivers/infiniband/core/cache.c
>>> +++ b/drivers/infiniband/core/cache.c
>>> @@ -917,9 +917,11 @@ static void gid_table_cleanup_one(struct 
>>> ib_device *ib_dev)
>>>   {
>>>       u32 p;
>>> -    rdma_for_each_port (ib_dev, p)
>>> +    rdma_for_each_port (ib_dev, p) {
>>> +        ib_dev->port_data[p].cache_is_initialized = 0;
>>
>> I think that this line is not needed, we are removing device anyway and
>> and query_port is not allowed at this stage.
>>
> We have kept this for code completeness purposes. Just as we did with 
> set_bit() and clear_bit() APIs.
> 
>>>           cleanup_gid_table_port(ib_dev, p,
>>>                          ib_dev->port_data[p].cache.gid);
>>> +    }
>>>   }
>>>   static int gid_table_setup_one(struct ib_device *ib_dev)
>>> @@ -1466,6 +1468,7 @@ static int config_non_roce_gid_cache(struct 
>>> ib_device *device,
>>>       struct ib_port_attr       *tprops = NULL;
>>>       struct ib_pkey_cache      *pkey_cache = NULL;
>>>       struct ib_pkey_cache      *old_pkey_cache = NULL;
>>> +    union ib_gid               gid;
>>>       int                        i;
>>>       int                        ret;
>>> @@ -1523,13 +1526,21 @@ static int config_non_roce_gid_cache(struct 
>>> ib_device *device,
>>>       device->port_data[port].cache.lmc = tprops->lmc;
>>>       device->port_data[port].cache.port_state = tprops->state;
>>> -    device->port_data[port].cache.subnet_prefix = 
>>> tprops->subnet_prefix;
>>> +    ret = rdma_query_gid(device, port, 0, &gid);
>>> +    if (ret) {
>>> +        write_unlock_irq(&device->cache.lock);
>>> +        goto err;
>>> +    }
>>> +
>>> +    device->port_data[port].cache.subnet_prefix =
>>> +            be64_to_cpu(gid.global.subnet_prefix);
>>> +
>>>       write_unlock_irq(&device->cache_lock);
>>>       if (enforce_security)
>>>           ib_security_cache_change(device,
>>>                        port,
>>> -                     tprops->subnet_prefix);
>>> +                     be64_to_cpu(gid.global.subnet_prefix));
>>>       kfree(old_pkey_cache);
>>>       kfree(tprops);
>>> @@ -1629,6 +1640,7 @@ int ib_cache_setup_one(struct ib_device *device)
>>>           err = ib_cache_update(device, p, true, true, true);
>>>           if (err)
>>>               return err;
>>> +        device->port_data[p].cache_is_initialized = 1;
>>>       }
>>>       return 0;
>>> diff --git a/drivers/infiniband/core/device.c 
>>> b/drivers/infiniband/core/device.c
>>> index 7a617e4..57b9039 100644
>>> --- a/drivers/infiniband/core/device.c
>>> +++ b/drivers/infiniband/core/device.c
>>> @@ -2057,6 +2057,15 @@ static int __ib_query_port(struct ib_device 
>>> *device,
>>>           IB_LINK_LAYER_INFINIBAND)
>>>           return 0;
>>> +    if (!ib_cache_is_initialised(device, port_num))
>>> +        goto query_gid_from_device;
>>
>> IMHO, we don't need this new function and can access ib_port_data
>> directly. In device.c, we have plenty of places that does it.
>>
>> Not critical.
>>
> Added this function to have a way to check validity of cache, such that 
> it could be used in future for the same check in areas to which 
> ib_port_data is opaque.
> 
Also, there was a review comment from Mark Zhang for patch version 2, 
where he had suggested to shift clear_bit(IB_PORT_CACHE_INITIALIZED, 
&device->port_data[p].flags);
to gid_table_cleanup_one().

Thanks,
Anand
>>> +
>>> +    ib_get_cached_subnet_prefix(device, port_num,
>>> +                    &port_attr->subnet_prefix);
>>> +
>>> +    return 0;
>>> +
>>> +query_gid_from_device:
>>>       err = device->ops.query_gid(device, port_num, 0, &gid);
>>>       if (err)
>>>           return err;
>>> diff --git a/include/rdma/ib_cache.h b/include/rdma/ib_cache.h
>>> index 226ae37..46b43a7 100644
>>> --- a/include/rdma/ib_cache.h
>>> +++ b/include/rdma/ib_cache.h
>>> @@ -114,4 +114,9 @@ ssize_t rdma_query_gid_table(struct ib_device 
>>> *device,
>>>                    struct ib_uverbs_gid_entry *entries,
>>>                    size_t max_entries);
>>> +static inline bool ib_cache_is_initialised(struct ib_device *device,
>>> +                    u32 port_num)
>>> +{
>>> +    return device->port_data[port_num].cache_is_initialized;
>>> +}
>>>   #endif /* _IB_CACHE_H */
>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>>> index c96d601..405f7da 100644
>>> --- a/include/rdma/ib_verbs.h
>>> +++ b/include/rdma/ib_verbs.h
>>> @@ -2177,6 +2177,7 @@ struct ib_port_data {
>>>       spinlock_t netdev_lock;
>>> +    u8 cache_is_initialized:1;
>>>       struct list_head pkey_list;
>>>       struct ib_port_cache cache;
>>> -- 
>>> 1.8.3.1
>>>
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices
  2021-06-16  7:42     ` Anand Khoje
  2021-06-16  8:05       ` Anand Khoje
@ 2021-06-16  8:41       ` Leon Romanovsky
  1 sibling, 0 replies; 14+ messages in thread
From: Leon Romanovsky @ 2021-06-16  8:41 UTC (permalink / raw)
  To: Anand Khoje; +Cc: linux-rdma, linux-kernel, dledford, jgg, haakon.bugge

On Wed, Jun 16, 2021 at 01:12:51PM +0530, Anand Khoje wrote:
> On 6/16/2021 12:57 PM, Leon Romanovsky wrote:
> > On Wed, Jun 16, 2021 at 12:22:13PM +0530, Anand Khoje wrote:
> > > ib_query_port() calls device->ops.query_port() to get the port
> > > attributes. The method of querying is device driver specific.
> > > The same function calls device->ops.query_gid() to get the GID and
> > > extract the subnet_prefix (gid_prefix).
> > > 
> > > The GID and subnet_prefix are stored in a cache. But they do not get
> > > read from the cache if the device is an Infiniband device. The
> > > following change takes advantage of the cached subnet_prefix.
> > > Testing with RDBMS has shown a significant improvement in performance
> > > with this change.
> > > 
> > > The function ib_cache_is_initialised() is introduced because
> > > ib_query_port() gets called early in the stage when the cache is not
> > > built while reading port immutable property.
> > > 
> > > In that case, the default GID still gets read from HCA for IB link-
> > > layer devices.
> > > 
> > > In the situation of an event causing cache update, the subnet_prefix
> > > will get retrieved from newly updated GID cache in ib_cache_update(),
> > > so that we do not end up reading a stale value from cache via
> > > ib_query_port().
> > > 
> > > Fixes: fad61ad ("IB/core: Add subnet prefix to port info")
> > > Suggested-by: Leon Romanovsky <leonro@nvidia.com>
> > > Suggested-by: Aru Kolappan <aru.kolappan@oracle.com>
> > > Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
> > > Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
> > > ---
> > > 
> > > v1 -> v2:
> > >      -   Split the v1 patch in 3 patches as per Leon's suggestion.
> > > 
> > > v2 -> v3:
> > >      -   Added changes as per Mark Zhang's suggestion of clearing
> > >          flags in git_table_cleanup_one().
> > > v3 -> v4:
> > >      -   Removed the enum ib_port_data_flags and 8 byte flags from
> > >          struct ib_port_data, and the set_bit()/clear_bit() API
> > >          used to update this flag as that was not necessary.
> > >          Done to keep the code simple.
> > >      -   Added code to read subnet_prefix from updated GID cache in the
> > >          event of cache update. Prior to this change, ib_cache_update
> > >          was reading the value for subnet_prefix via ib_query_port(),
> > >          due to this patch, we ended up reading a stale cached value of
> > >          subnet_prefix.
> > > 
> > > ---
> > >   drivers/infiniband/core/cache.c  | 18 +++++++++++++++---
> > >   drivers/infiniband/core/device.c |  9 +++++++++
> > >   include/rdma/ib_cache.h          |  5 +++++
> > >   include/rdma/ib_verbs.h          |  1 +
> > >   4 files changed, 30 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> > > index 2325171..cd99c46 100644
> > > --- a/drivers/infiniband/core/cache.c
> > > +++ b/drivers/infiniband/core/cache.c
> > > @@ -917,9 +917,11 @@ static void gid_table_cleanup_one(struct ib_device *ib_dev)
> > >   {
> > >   	u32 p;
> > > -	rdma_for_each_port (ib_dev, p)
> > > +	rdma_for_each_port (ib_dev, p) {
> > > +		ib_dev->port_data[p].cache_is_initialized = 0;
> > 
> > I think that this line is not needed, we are removing device anyway and
> > and query_port is not allowed at this stage.
> > 
> We have kept this for code completeness purposes. Just as we did with
> set_bit() and clear_bit() APIs.

You are not using *_bit() API now, so let's not clear here.
It is not completeness, but misleading. It gives false assumption
that cache_is_initialized is used later in the code.

> 
> > >   		cleanup_gid_table_port(ib_dev, p,
> > >   				       ib_dev->port_data[p].cache.gid);
> > > +	}
> > >   }
> > >   static int gid_table_setup_one(struct ib_device *ib_dev)
> > > @@ -1466,6 +1468,7 @@ static int config_non_roce_gid_cache(struct ib_device *device,
> > >   	struct ib_port_attr       *tprops = NULL;
> > >   	struct ib_pkey_cache      *pkey_cache = NULL;
> > >   	struct ib_pkey_cache      *old_pkey_cache = NULL;
> > > +	union ib_gid               gid;
> > >   	int                        i;
> > >   	int                        ret;
> > > @@ -1523,13 +1526,21 @@ static int config_non_roce_gid_cache(struct ib_device *device,
> > >   	device->port_data[port].cache.lmc = tprops->lmc;
> > >   	device->port_data[port].cache.port_state = tprops->state;
> > > -	device->port_data[port].cache.subnet_prefix = tprops->subnet_prefix;
> > > +	ret = rdma_query_gid(device, port, 0, &gid);
> > > +	if (ret) {
> > > +		write_unlock_irq(&device->cache.lock);
> > > +		goto err;
> > > +	}
> > > +
> > > +	device->port_data[port].cache.subnet_prefix =
> > > +			be64_to_cpu(gid.global.subnet_prefix);
> > > +
> > >   	write_unlock_irq(&device->cache_lock);
> > >   	if (enforce_security)
> > >   		ib_security_cache_change(device,
> > >   					 port,
> > > -					 tprops->subnet_prefix);
> > > +					 be64_to_cpu(gid.global.subnet_prefix));
> > >   	kfree(old_pkey_cache);
> > >   	kfree(tprops);
> > > @@ -1629,6 +1640,7 @@ int ib_cache_setup_one(struct ib_device *device)
> > >   		err = ib_cache_update(device, p, true, true, true);
> > >   		if (err)
> > >   			return err;
> > > +		device->port_data[p].cache_is_initialized = 1;
> > >   	}
> > >   	return 0;
> > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > > index 7a617e4..57b9039 100644
> > > --- a/drivers/infiniband/core/device.c
> > > +++ b/drivers/infiniband/core/device.c
> > > @@ -2057,6 +2057,15 @@ static int __ib_query_port(struct ib_device *device,
> > >   	    IB_LINK_LAYER_INFINIBAND)
> > >   		return 0;
> > > +	if (!ib_cache_is_initialised(device, port_num))
> > > +		goto query_gid_from_device;
> > 
> > IMHO, we don't need this new function and can access ib_port_data
> > directly. In device.c, we have plenty of places that does it.
> > 
> > Not critical.
> > 
> Added this function to have a way to check validity of cache, such that it
> could be used in future for the same check in areas to which ib_port_data is
> opaque.


It is ok, just call directly if (!device->port_data[port_num].cache_is_initialized).

Thanks

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices
  2021-06-16  6:52 ` [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices Anand Khoje
@ 2021-06-17 10:05     ` kernel test robot
  2021-06-16  7:30   ` Leon Romanovsky
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2021-06-17 10:05 UTC (permalink / raw)
  To: Anand Khoje, linux-rdma, linux-kernel
  Cc: kbuild-all, clang-built-linux, dledford, jgg, haakon.bugge, leon

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

Hi Anand,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on rdma/for-next]
[also build test ERROR on next-20210616]
[cannot apply to linux/master linus/master v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Anand-Khoje/IB-core-Obtaining-subnet_prefix-from-cache-in/20210617-102611
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next
config: powerpc64-randconfig-r024-20210617 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc64 cross compiling tool for clang build
        # apt-get install binutils-powerpc64-linux-gnu
        # https://github.com/0day-ci/linux/commit/495253a987df586d8c5f4c525999cdf39c5823f0
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Anand-Khoje/IB-core-Obtaining-subnet_prefix-from-cache-in/20210617-102611
        git checkout 495253a987df586d8c5f4c525999cdf39c5823f0
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/infiniband/core/cache.c:36:
   In file included from include/linux/module.h:12:
   In file included from include/linux/list.h:9:
   In file included from include/linux/kernel.h:12:
   In file included from include/linux/bitops.h:32:
   In file included from arch/powerpc/include/asm/bitops.h:62:
   arch/powerpc/include/asm/barrier.h:49:9: warning: '__lwsync' macro redefined [-Wmacro-redefined]
   #define __lwsync()      __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory")
           ^
   <built-in>:308:9: note: previous definition is here
   #define __lwsync __builtin_ppc_lwsync
           ^
>> drivers/infiniband/core/cache.c:1531:29: error: no member named 'cache' in 'struct ib_device'
                   write_unlock_irq(&device->cache.lock);
                                     ~~~~~~  ^
   include/linux/rwlock.h:108:55: note: expanded from macro 'write_unlock_irq'
   #define write_unlock_irq(lock)          _raw_write_unlock_irq(lock)
                                                                 ^~~~
   1 warning and 1 error generated.


vim +1531 drivers/infiniband/core/cache.c

  1463	
  1464	static int
  1465	ib_cache_update(struct ib_device *device, u32 port, bool update_gids,
  1466			bool update_pkeys, bool enforce_security)
  1467	{
  1468		struct ib_port_attr       *tprops = NULL;
  1469		struct ib_pkey_cache      *pkey_cache = NULL;
  1470		struct ib_pkey_cache      *old_pkey_cache = NULL;
  1471		union ib_gid               gid;
  1472		int                        i;
  1473		int                        ret;
  1474	
  1475		if (!rdma_is_port_valid(device, port))
  1476			return -EINVAL;
  1477	
  1478		tprops = kmalloc(sizeof *tprops, GFP_KERNEL);
  1479		if (!tprops)
  1480			return -ENOMEM;
  1481	
  1482		ret = ib_query_port(device, port, tprops);
  1483		if (ret) {
  1484			dev_warn(&device->dev, "ib_query_port failed (%d)\n", ret);
  1485			goto err;
  1486		}
  1487	
  1488		if (!rdma_protocol_roce(device, port) && update_gids) {
  1489			ret = config_non_roce_gid_cache(device, port,
  1490							tprops->gid_tbl_len);
  1491			if (ret)
  1492				goto err;
  1493		}
  1494	
  1495		update_pkeys &= !!tprops->pkey_tbl_len;
  1496	
  1497		if (update_pkeys) {
  1498			pkey_cache = kmalloc(struct_size(pkey_cache, table,
  1499							 tprops->pkey_tbl_len),
  1500					     GFP_KERNEL);
  1501			if (!pkey_cache) {
  1502				ret = -ENOMEM;
  1503				goto err;
  1504			}
  1505	
  1506			pkey_cache->table_len = tprops->pkey_tbl_len;
  1507	
  1508			for (i = 0; i < pkey_cache->table_len; ++i) {
  1509				ret = ib_query_pkey(device, port, i,
  1510						    pkey_cache->table + i);
  1511				if (ret) {
  1512					dev_warn(&device->dev,
  1513						 "ib_query_pkey failed (%d) for index %d\n",
  1514						 ret, i);
  1515					goto err;
  1516				}
  1517			}
  1518		}
  1519	
  1520		write_lock_irq(&device->cache_lock);
  1521	
  1522		if (update_pkeys) {
  1523			old_pkey_cache = device->port_data[port].cache.pkey;
  1524			device->port_data[port].cache.pkey = pkey_cache;
  1525		}
  1526		device->port_data[port].cache.lmc = tprops->lmc;
  1527		device->port_data[port].cache.port_state = tprops->state;
  1528	
  1529		ret = rdma_query_gid(device, port, 0, &gid);
  1530		if (ret) {
> 1531			write_unlock_irq(&device->cache.lock);
  1532			goto err;
  1533		}
  1534	
  1535		device->port_data[port].cache.subnet_prefix =
  1536				be64_to_cpu(gid.global.subnet_prefix);
  1537	
  1538		write_unlock_irq(&device->cache_lock);
  1539	
  1540		if (enforce_security)
  1541			ib_security_cache_change(device,
  1542						 port,
  1543						 be64_to_cpu(gid.global.subnet_prefix));
  1544	
  1545		kfree(old_pkey_cache);
  1546		kfree(tprops);
  1547		return 0;
  1548	
  1549	err:
  1550		kfree(pkey_cache);
  1551		kfree(tprops);
  1552		return ret;
  1553	}
  1554	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26457 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices
@ 2021-06-17 10:05     ` kernel test robot
  0 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2021-06-17 10:05 UTC (permalink / raw)
  To: kbuild-all

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

Hi Anand,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on rdma/for-next]
[also build test ERROR on next-20210616]
[cannot apply to linux/master linus/master v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Anand-Khoje/IB-core-Obtaining-subnet_prefix-from-cache-in/20210617-102611
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next
config: powerpc64-randconfig-r024-20210617 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc64 cross compiling tool for clang build
        # apt-get install binutils-powerpc64-linux-gnu
        # https://github.com/0day-ci/linux/commit/495253a987df586d8c5f4c525999cdf39c5823f0
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Anand-Khoje/IB-core-Obtaining-subnet_prefix-from-cache-in/20210617-102611
        git checkout 495253a987df586d8c5f4c525999cdf39c5823f0
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/infiniband/core/cache.c:36:
   In file included from include/linux/module.h:12:
   In file included from include/linux/list.h:9:
   In file included from include/linux/kernel.h:12:
   In file included from include/linux/bitops.h:32:
   In file included from arch/powerpc/include/asm/bitops.h:62:
   arch/powerpc/include/asm/barrier.h:49:9: warning: '__lwsync' macro redefined [-Wmacro-redefined]
   #define __lwsync()      __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory")
           ^
   <built-in>:308:9: note: previous definition is here
   #define __lwsync __builtin_ppc_lwsync
           ^
>> drivers/infiniband/core/cache.c:1531:29: error: no member named 'cache' in 'struct ib_device'
                   write_unlock_irq(&device->cache.lock);
                                     ~~~~~~  ^
   include/linux/rwlock.h:108:55: note: expanded from macro 'write_unlock_irq'
   #define write_unlock_irq(lock)          _raw_write_unlock_irq(lock)
                                                                 ^~~~
   1 warning and 1 error generated.


vim +1531 drivers/infiniband/core/cache.c

  1463	
  1464	static int
  1465	ib_cache_update(struct ib_device *device, u32 port, bool update_gids,
  1466			bool update_pkeys, bool enforce_security)
  1467	{
  1468		struct ib_port_attr       *tprops = NULL;
  1469		struct ib_pkey_cache      *pkey_cache = NULL;
  1470		struct ib_pkey_cache      *old_pkey_cache = NULL;
  1471		union ib_gid               gid;
  1472		int                        i;
  1473		int                        ret;
  1474	
  1475		if (!rdma_is_port_valid(device, port))
  1476			return -EINVAL;
  1477	
  1478		tprops = kmalloc(sizeof *tprops, GFP_KERNEL);
  1479		if (!tprops)
  1480			return -ENOMEM;
  1481	
  1482		ret = ib_query_port(device, port, tprops);
  1483		if (ret) {
  1484			dev_warn(&device->dev, "ib_query_port failed (%d)\n", ret);
  1485			goto err;
  1486		}
  1487	
  1488		if (!rdma_protocol_roce(device, port) && update_gids) {
  1489			ret = config_non_roce_gid_cache(device, port,
  1490							tprops->gid_tbl_len);
  1491			if (ret)
  1492				goto err;
  1493		}
  1494	
  1495		update_pkeys &= !!tprops->pkey_tbl_len;
  1496	
  1497		if (update_pkeys) {
  1498			pkey_cache = kmalloc(struct_size(pkey_cache, table,
  1499							 tprops->pkey_tbl_len),
  1500					     GFP_KERNEL);
  1501			if (!pkey_cache) {
  1502				ret = -ENOMEM;
  1503				goto err;
  1504			}
  1505	
  1506			pkey_cache->table_len = tprops->pkey_tbl_len;
  1507	
  1508			for (i = 0; i < pkey_cache->table_len; ++i) {
  1509				ret = ib_query_pkey(device, port, i,
  1510						    pkey_cache->table + i);
  1511				if (ret) {
  1512					dev_warn(&device->dev,
  1513						 "ib_query_pkey failed (%d) for index %d\n",
  1514						 ret, i);
  1515					goto err;
  1516				}
  1517			}
  1518		}
  1519	
  1520		write_lock_irq(&device->cache_lock);
  1521	
  1522		if (update_pkeys) {
  1523			old_pkey_cache = device->port_data[port].cache.pkey;
  1524			device->port_data[port].cache.pkey = pkey_cache;
  1525		}
  1526		device->port_data[port].cache.lmc = tprops->lmc;
  1527		device->port_data[port].cache.port_state = tprops->state;
  1528	
  1529		ret = rdma_query_gid(device, port, 0, &gid);
  1530		if (ret) {
> 1531			write_unlock_irq(&device->cache.lock);
  1532			goto err;
  1533		}
  1534	
  1535		device->port_data[port].cache.subnet_prefix =
  1536				be64_to_cpu(gid.global.subnet_prefix);
  1537	
  1538		write_unlock_irq(&device->cache_lock);
  1539	
  1540		if (enforce_security)
  1541			ib_security_cache_change(device,
  1542						 port,
  1543						 be64_to_cpu(gid.global.subnet_prefix));
  1544	
  1545		kfree(old_pkey_cache);
  1546		kfree(tprops);
  1547		return 0;
  1548	
  1549	err:
  1550		kfree(pkey_cache);
  1551		kfree(tprops);
  1552		return ret;
  1553	}
  1554	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 26457 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices
  2021-06-16  6:52 ` [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices Anand Khoje
@ 2021-06-17 10:11     ` kernel test robot
  2021-06-16  7:30   ` Leon Romanovsky
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2021-06-17 10:11 UTC (permalink / raw)
  To: Anand Khoje, linux-rdma, linux-kernel
  Cc: kbuild-all, clang-built-linux, dledford, jgg, haakon.bugge, leon

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

Hi Anand,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on rdma/for-next]
[also build test ERROR on next-20210616]
[cannot apply to linux/master linus/master v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Anand-Khoje/IB-core-Obtaining-subnet_prefix-from-cache-in/20210617-102611
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next
config: powerpc64-randconfig-r012-20210617 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc64 cross compiling tool for clang build
        # apt-get install binutils-powerpc64-linux-gnu
        # https://github.com/0day-ci/linux/commit/495253a987df586d8c5f4c525999cdf39c5823f0
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Anand-Khoje/IB-core-Obtaining-subnet_prefix-from-cache-in/20210617-102611
        git checkout 495253a987df586d8c5f4c525999cdf39c5823f0
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/infiniband/core/cache.c:36:
   In file included from include/linux/module.h:12:
   In file included from include/linux/list.h:9:
   In file included from include/linux/kernel.h:12:
   In file included from include/linux/bitops.h:32:
   In file included from arch/powerpc/include/asm/bitops.h:62:
   arch/powerpc/include/asm/barrier.h:49:9: warning: '__lwsync' macro redefined [-Wmacro-redefined]
   #define __lwsync()      __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory")
           ^
   <built-in>:310:9: note: previous definition is here
   #define __lwsync __builtin_ppc_lwsync
           ^
>> drivers/infiniband/core/cache.c:1531:29: error: no member named 'cache' in 'struct ib_device'
                   write_unlock_irq(&device->cache.lock);
                                     ~~~~~~  ^
   include/linux/rwlock.h:108:55: note: expanded from macro 'write_unlock_irq'
   #define write_unlock_irq(lock)          _raw_write_unlock_irq(lock)
                                                                 ^~~~
   1 warning and 1 error generated.


vim +1531 drivers/infiniband/core/cache.c

  1463	
  1464	static int
  1465	ib_cache_update(struct ib_device *device, u32 port, bool update_gids,
  1466			bool update_pkeys, bool enforce_security)
  1467	{
  1468		struct ib_port_attr       *tprops = NULL;
  1469		struct ib_pkey_cache      *pkey_cache = NULL;
  1470		struct ib_pkey_cache      *old_pkey_cache = NULL;
  1471		union ib_gid               gid;
  1472		int                        i;
  1473		int                        ret;
  1474	
  1475		if (!rdma_is_port_valid(device, port))
  1476			return -EINVAL;
  1477	
  1478		tprops = kmalloc(sizeof *tprops, GFP_KERNEL);
  1479		if (!tprops)
  1480			return -ENOMEM;
  1481	
  1482		ret = ib_query_port(device, port, tprops);
  1483		if (ret) {
  1484			dev_warn(&device->dev, "ib_query_port failed (%d)\n", ret);
  1485			goto err;
  1486		}
  1487	
  1488		if (!rdma_protocol_roce(device, port) && update_gids) {
  1489			ret = config_non_roce_gid_cache(device, port,
  1490							tprops->gid_tbl_len);
  1491			if (ret)
  1492				goto err;
  1493		}
  1494	
  1495		update_pkeys &= !!tprops->pkey_tbl_len;
  1496	
  1497		if (update_pkeys) {
  1498			pkey_cache = kmalloc(struct_size(pkey_cache, table,
  1499							 tprops->pkey_tbl_len),
  1500					     GFP_KERNEL);
  1501			if (!pkey_cache) {
  1502				ret = -ENOMEM;
  1503				goto err;
  1504			}
  1505	
  1506			pkey_cache->table_len = tprops->pkey_tbl_len;
  1507	
  1508			for (i = 0; i < pkey_cache->table_len; ++i) {
  1509				ret = ib_query_pkey(device, port, i,
  1510						    pkey_cache->table + i);
  1511				if (ret) {
  1512					dev_warn(&device->dev,
  1513						 "ib_query_pkey failed (%d) for index %d\n",
  1514						 ret, i);
  1515					goto err;
  1516				}
  1517			}
  1518		}
  1519	
  1520		write_lock_irq(&device->cache_lock);
  1521	
  1522		if (update_pkeys) {
  1523			old_pkey_cache = device->port_data[port].cache.pkey;
  1524			device->port_data[port].cache.pkey = pkey_cache;
  1525		}
  1526		device->port_data[port].cache.lmc = tprops->lmc;
  1527		device->port_data[port].cache.port_state = tprops->state;
  1528	
  1529		ret = rdma_query_gid(device, port, 0, &gid);
  1530		if (ret) {
> 1531			write_unlock_irq(&device->cache.lock);
  1532			goto err;
  1533		}
  1534	
  1535		device->port_data[port].cache.subnet_prefix =
  1536				be64_to_cpu(gid.global.subnet_prefix);
  1537	
  1538		write_unlock_irq(&device->cache_lock);
  1539	
  1540		if (enforce_security)
  1541			ib_security_cache_change(device,
  1542						 port,
  1543						 be64_to_cpu(gid.global.subnet_prefix));
  1544	
  1545		kfree(old_pkey_cache);
  1546		kfree(tprops);
  1547		return 0;
  1548	
  1549	err:
  1550		kfree(pkey_cache);
  1551		kfree(tprops);
  1552		return ret;
  1553	}
  1554	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 46577 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices
@ 2021-06-17 10:11     ` kernel test robot
  0 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2021-06-17 10:11 UTC (permalink / raw)
  To: kbuild-all

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

Hi Anand,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on rdma/for-next]
[also build test ERROR on next-20210616]
[cannot apply to linux/master linus/master v5.13-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Anand-Khoje/IB-core-Obtaining-subnet_prefix-from-cache-in/20210617-102611
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next
config: powerpc64-randconfig-r012-20210617 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc64 cross compiling tool for clang build
        # apt-get install binutils-powerpc64-linux-gnu
        # https://github.com/0day-ci/linux/commit/495253a987df586d8c5f4c525999cdf39c5823f0
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Anand-Khoje/IB-core-Obtaining-subnet_prefix-from-cache-in/20210617-102611
        git checkout 495253a987df586d8c5f4c525999cdf39c5823f0
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/infiniband/core/cache.c:36:
   In file included from include/linux/module.h:12:
   In file included from include/linux/list.h:9:
   In file included from include/linux/kernel.h:12:
   In file included from include/linux/bitops.h:32:
   In file included from arch/powerpc/include/asm/bitops.h:62:
   arch/powerpc/include/asm/barrier.h:49:9: warning: '__lwsync' macro redefined [-Wmacro-redefined]
   #define __lwsync()      __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory")
           ^
   <built-in>:310:9: note: previous definition is here
   #define __lwsync __builtin_ppc_lwsync
           ^
>> drivers/infiniband/core/cache.c:1531:29: error: no member named 'cache' in 'struct ib_device'
                   write_unlock_irq(&device->cache.lock);
                                     ~~~~~~  ^
   include/linux/rwlock.h:108:55: note: expanded from macro 'write_unlock_irq'
   #define write_unlock_irq(lock)          _raw_write_unlock_irq(lock)
                                                                 ^~~~
   1 warning and 1 error generated.


vim +1531 drivers/infiniband/core/cache.c

  1463	
  1464	static int
  1465	ib_cache_update(struct ib_device *device, u32 port, bool update_gids,
  1466			bool update_pkeys, bool enforce_security)
  1467	{
  1468		struct ib_port_attr       *tprops = NULL;
  1469		struct ib_pkey_cache      *pkey_cache = NULL;
  1470		struct ib_pkey_cache      *old_pkey_cache = NULL;
  1471		union ib_gid               gid;
  1472		int                        i;
  1473		int                        ret;
  1474	
  1475		if (!rdma_is_port_valid(device, port))
  1476			return -EINVAL;
  1477	
  1478		tprops = kmalloc(sizeof *tprops, GFP_KERNEL);
  1479		if (!tprops)
  1480			return -ENOMEM;
  1481	
  1482		ret = ib_query_port(device, port, tprops);
  1483		if (ret) {
  1484			dev_warn(&device->dev, "ib_query_port failed (%d)\n", ret);
  1485			goto err;
  1486		}
  1487	
  1488		if (!rdma_protocol_roce(device, port) && update_gids) {
  1489			ret = config_non_roce_gid_cache(device, port,
  1490							tprops->gid_tbl_len);
  1491			if (ret)
  1492				goto err;
  1493		}
  1494	
  1495		update_pkeys &= !!tprops->pkey_tbl_len;
  1496	
  1497		if (update_pkeys) {
  1498			pkey_cache = kmalloc(struct_size(pkey_cache, table,
  1499							 tprops->pkey_tbl_len),
  1500					     GFP_KERNEL);
  1501			if (!pkey_cache) {
  1502				ret = -ENOMEM;
  1503				goto err;
  1504			}
  1505	
  1506			pkey_cache->table_len = tprops->pkey_tbl_len;
  1507	
  1508			for (i = 0; i < pkey_cache->table_len; ++i) {
  1509				ret = ib_query_pkey(device, port, i,
  1510						    pkey_cache->table + i);
  1511				if (ret) {
  1512					dev_warn(&device->dev,
  1513						 "ib_query_pkey failed (%d) for index %d\n",
  1514						 ret, i);
  1515					goto err;
  1516				}
  1517			}
  1518		}
  1519	
  1520		write_lock_irq(&device->cache_lock);
  1521	
  1522		if (update_pkeys) {
  1523			old_pkey_cache = device->port_data[port].cache.pkey;
  1524			device->port_data[port].cache.pkey = pkey_cache;
  1525		}
  1526		device->port_data[port].cache.lmc = tprops->lmc;
  1527		device->port_data[port].cache.port_state = tprops->state;
  1528	
  1529		ret = rdma_query_gid(device, port, 0, &gid);
  1530		if (ret) {
> 1531			write_unlock_irq(&device->cache.lock);
  1532			goto err;
  1533		}
  1534	
  1535		device->port_data[port].cache.subnet_prefix =
  1536				be64_to_cpu(gid.global.subnet_prefix);
  1537	
  1538		write_unlock_irq(&device->cache_lock);
  1539	
  1540		if (enforce_security)
  1541			ib_security_cache_change(device,
  1542						 port,
  1543						 be64_to_cpu(gid.global.subnet_prefix));
  1544	
  1545		kfree(old_pkey_cache);
  1546		kfree(tprops);
  1547		return 0;
  1548	
  1549	err:
  1550		kfree(pkey_cache);
  1551		kfree(tprops);
  1552		return ret;
  1553	}
  1554	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 46577 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-06-17 10:12 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-16  6:52 [PATCH v4 for-next 0/3] IB/core: Obtaining subnet_prefix from cache in Anand Khoje
2021-06-16  6:52 ` [PATCH v4 for-next 1/3] IB/core: Removed port validity check from ib_get_cached_subnet_prefix Anand Khoje
2021-06-16  6:52 ` [PATCH v4 for-next 2/3] IB/core: Shuffle locks in ib_port_data to save memory Anand Khoje
2021-06-16  6:52 ` [PATCH v4 for-next 3/3] IB/core: Obtain subnet_prefix from cache in IB devices Anand Khoje
2021-06-16  7:27   ` Leon Romanovsky
2021-06-16  7:42     ` Anand Khoje
2021-06-16  8:05       ` Anand Khoje
2021-06-16  8:41       ` Leon Romanovsky
2021-06-16  7:30   ` Leon Romanovsky
2021-06-16  7:43     ` Anand Khoje
2021-06-17 10:05   ` kernel test robot
2021-06-17 10:05     ` kernel test robot
2021-06-17 10:11   ` kernel test robot
2021-06-17 10:11     ` kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.