[PATCH v2 0/3] IB/core: Obtaining subnet_prefix from cache in IB devices.

[PATCH v2 0/3] IB/core: Obtaining subnet_prefix from cache in IB devices.

[Anand Khoje]

This v2 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 to add the 8 byte field flags used for checking the cache
entry validity. 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, which is used to add the 8 byte
field flags in patch 3.

3. Added flags to struct ib_port_data and enum ib_port_data_flags. These
are 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().

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

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

-- 
1.8.3.1

[PATCH v2 1/3] IB/core: Removed port validity check from ib_get_cached_subnet_prefix

[Anand Khoje]

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 <leon@kernel.org>
Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
---
 drivers/infiniband/core/cache.c     |  7 ++-----
 drivers/infiniband/core/core_priv.h |  2 +-
 drivers/infiniband/core/device.c    | 14 +++++---------
 drivers/infiniband/core/security.c  |  7 ++-----
 4 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index 3b0991f..b6700ad 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -1069,19 +1069,16 @@ 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;
+	return;
 }
 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..c2fa592 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -886,15 +886,11 @@ 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

Re: [PATCH v2 1/3] IB/core: Removed port validity check from ib_get_cached_subnet_prefix

[Leon Romanovsky]

On Thu, Jun 03, 2021 at 12:20:22PM +0530, Anand Khoje wrote:
> 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 <leon@kernel.org>
> Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
> Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
> ---
>  drivers/infiniband/core/cache.c     |  7 ++-----
>  drivers/infiniband/core/core_priv.h |  2 +-
>  drivers/infiniband/core/device.c    | 14 +++++---------
>  drivers/infiniband/core/security.c  |  7 ++-----
>  4 files changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> index 3b0991f..b6700ad 100644
> --- a/drivers/infiniband/core/cache.c
> +++ b/drivers/infiniband/core/cache.c
> @@ -1069,19 +1069,16 @@ 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;
> +	return;

"return" is not needed here.

>  }
>  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..c2fa592 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -886,15 +886,11 @@ static void ib_policy_change_task(struct work_struct *work)
>  
>  		rdma_for_each_port (dev, i) {
>  			u64 sp;

Please add extra blank line after variable declaration or simply move it
outside of rdma_for_each_port() loop.

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

Strange line formatting, please use clang-formatter and don't break the line.

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

[PATCH v2 2/3] IB/core: Shuffle locks in ib_port_data to save memory

[Anand Khoje]

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>
---
 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 7e2f369..41cbec5 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2175,11 +2175,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

Re: [PATCH v2 2/3] IB/core: Shuffle locks in ib_port_data to save memory

[Leon Romanovsky]

On Thu, Jun 03, 2021 at 12:20:23PM +0530, Anand Khoje wrote:
> 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>
> ---
>  include/rdma/ib_verbs.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

[PATCH v2 3/3] IB/core: Obtain subnet_prefix from cache in IB devices

[Anand Khoje]

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.

Fixes: fad61ad ("IB/core: Add subnet prefix to port info")
Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
---
 drivers/infiniband/core/cache.c  | 7 ++++++-
 drivers/infiniband/core/device.c | 9 +++++++++
 include/rdma/ib_cache.h          | 6 ++++++
 include/rdma/ib_verbs.h          | 6 ++++++
 4 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index b6700ad..724ac0e 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -1624,6 +1624,8 @@ int ib_cache_setup_one(struct ib_device *device)
 		err = ib_cache_update(device, p, true);
 		if (err)
 			return err;
+		set_bit(IB_PORT_CACHE_INITIALIZED,
+			&device->port_data[p].flags);
 	}
 
 	return 0;
@@ -1639,8 +1641,11 @@ void ib_cache_release_one(struct ib_device *device)
 	 * all the device's resources when the cache could no
 	 * longer be accessed.
 	 */
-	rdma_for_each_port (device, p)
+	rdma_for_each_port (device, p) {
+		clear_bit(IB_PORT_CACHE_INITIALIZED,
+			 &device->port_data[p].flags);
 		kfree(device->port_data[p].cache.pkey);
+	}
 
 	gid_table_release_one(device);
 }
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index c2fa592..b3e20ac 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -2060,6 +2060,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..1526fc6 100644
--- a/include/rdma/ib_cache.h
+++ b/include/rdma/ib_cache.h
@@ -114,4 +114,10 @@ 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,
+					  u8 port_num)
+{
+	return test_bit(IB_PORT_CACHE_INITIALIZED,
+			&device->port_data[port_num].flags);
+}
 #endif /* _IB_CACHE_H */
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 41cbec5..ad2a55e 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2169,6 +2169,10 @@ struct ib_port_immutable {
 	u32                           max_mad_size;
 };
 
+enum ib_port_data_flags {
+	IB_PORT_CACHE_INITIALIZED = 1 << 0,
+};
+
 struct ib_port_data {
 	struct ib_device *ib_dev;
 
@@ -2178,6 +2182,8 @@ struct ib_port_data {
 
 	spinlock_t netdev_lock;
 
+	unsigned long flags;
+
 	struct list_head pkey_list;
 
 	struct ib_port_cache cache;
-- 
1.8.3.1

Re: [PATCH v2 3/3] IB/core: Obtain subnet_prefix from cache in IB devices

[Leon Romanovsky]

On Thu, Jun 03, 2021 at 12:20:24PM +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.
> 
> Fixes: fad61ad ("IB/core: Add subnet prefix to port info")
> Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
> Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
> ---
>  drivers/infiniband/core/cache.c  | 7 ++++++-
>  drivers/infiniband/core/device.c | 9 +++++++++
>  include/rdma/ib_cache.h          | 6 ++++++
>  include/rdma/ib_verbs.h          | 6 ++++++
>  4 files changed, 27 insertions(+), 1 deletion(-)

Can you please help me to understand how cache is updated?

There are a lot of calls to ib_query_port() and I wonder how callers can
get new GID after it was changed in already initialized cache.

Thanks

Re: [PATCH v2 3/3] IB/core: Obtain subnet_prefix from cache in IB devices

[Haakon Bugge]

> On 3 Jun 2021, at 11:07, Leon Romanovsky <leon@kernel.org> wrote:
> 
> On Thu, Jun 03, 2021 at 12:20:24PM +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.
>> 
>> Fixes: fad61ad ("IB/core: Add subnet prefix to port info")
>> Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
>> Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
>> ---
>> drivers/infiniband/core/cache.c  | 7 ++++++-
>> drivers/infiniband/core/device.c | 9 +++++++++
>> include/rdma/ib_cache.h          | 6 ++++++
>> include/rdma/ib_verbs.h          | 6 ++++++
>> 4 files changed, 27 insertions(+), 1 deletion(-)
> 
> Can you please help me to understand how cache is updated?
> 
> There are a lot of calls to ib_query_port() and I wonder how callers can
> get new GID after it was changed in already initialized cache.

The cache is initialized when it is created, just before the bit IB_PORT_CACHE_INITIALIZED is set in flags.

After commit d58c23c92548 ("IB/core: Only update PKEY and GID caches on respective events"), the GID portion of the cache is updated when a IB_EVENT_GID_CHANGE event is received.

Before said commit, it was updated on any event.


Thxs, Håkon

Re: [PATCH v2 3/3] IB/core: Obtain subnet_prefix from cache in IB devices

[Leon Romanovsky]

On Thu, Jun 03, 2021 at 09:29:32AM +0000, Haakon Bugge wrote:
> 
> 
> > On 3 Jun 2021, at 11:07, Leon Romanovsky <leon@kernel.org> wrote:
> > 
> > On Thu, Jun 03, 2021 at 12:20:24PM +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.
> >> 
> >> Fixes: fad61ad ("IB/core: Add subnet prefix to port info")
> >> Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
> >> Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
> >> ---
> >> drivers/infiniband/core/cache.c  | 7 ++++++-
> >> drivers/infiniband/core/device.c | 9 +++++++++
> >> include/rdma/ib_cache.h          | 6 ++++++
> >> include/rdma/ib_verbs.h          | 6 ++++++
> >> 4 files changed, 27 insertions(+), 1 deletion(-)
> > 
> > Can you please help me to understand how cache is updated?
> > 
> > There are a lot of calls to ib_query_port() and I wonder how callers can
> > get new GID after it was changed in already initialized cache.
> 
> The cache is initialized when it is created, just before the bit IB_PORT_CACHE_INITIALIZED is set in flags.
> 
> After commit d58c23c92548 ("IB/core: Only update PKEY and GID caches on respective events"), the GID portion of the cache is updated when a IB_EVENT_GID_CHANGE event is received.
> 
> Before said commit, it was updated on any event.

This part is clear to me, the missing piece is to understand what will
happen if cache and GID are not in sync because of asynchronous nature of
events.

Thanks

> 
> 
> Thxs, Håkon
> 

Re: [PATCH v2 3/3] IB/core: Obtain subnet_prefix from cache in IB devices

[Haakon Bugge]

> On 3 Jun 2021, at 12:16, Leon Romanovsky <leon@kernel.org> wrote:
> 
> On Thu, Jun 03, 2021 at 09:29:32AM +0000, Haakon Bugge wrote:
>> 
>> 
>>> On 3 Jun 2021, at 11:07, Leon Romanovsky <leon@kernel.org> wrote:
>>> 
>>> On Thu, Jun 03, 2021 at 12:20:24PM +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.
>>>> 
>>>> Fixes: fad61ad ("IB/core: Add subnet prefix to port info")
>>>> Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
>>>> Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
>>>> ---
>>>> drivers/infiniband/core/cache.c  | 7 ++++++-
>>>> drivers/infiniband/core/device.c | 9 +++++++++
>>>> include/rdma/ib_cache.h          | 6 ++++++
>>>> include/rdma/ib_verbs.h          | 6 ++++++
>>>> 4 files changed, 27 insertions(+), 1 deletion(-)
>>> 
>>> Can you please help me to understand how cache is updated?
>>> 
>>> There are a lot of calls to ib_query_port() and I wonder how callers can
>>> get new GID after it was changed in already initialized cache.
>> 
>> The cache is initialized when it is created, just before the bit IB_PORT_CACHE_INITIALIZED is set in flags.
>> 
>> After commit d58c23c92548 ("IB/core: Only update PKEY and GID caches on respective events"), the GID portion of the cache is updated when a IB_EVENT_GID_CHANGE event is received.
>> 
>> Before said commit, it was updated on any event.
> 
> This part is clear to me, the missing piece is to understand what will
> happen if cache and GID are not in sync because of asynchronous nature of
> events.

The calls to ib_query_port() are asynchronous with GID change. Consider the time line:

Time    HCA     cache
t0      GIDa    GIDa
t1
t2      GIDb    GIDa
t3
t4      GIDb    GIDb
t5


Prior to this commit, if ib_query_port() was called at t1 or at t3, two different GIDs would be retrieved.

With this commit, if ib_query_port() was called at t3 or t5, two different GIDs would be retrieved.

The scenario is the same, only skewed in time.


Thxs, Håkon

Re: [PATCH v2 3/3] IB/core: Obtain subnet_prefix from cache in IB devices

[Mark Zhang]

On 6/3/2021 2:50 PM, Anand Khoje wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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.
> 
> Fixes: fad61ad ("IB/core: Add subnet prefix to port info")
> Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
> Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
> ---
>   drivers/infiniband/core/cache.c  | 7 ++++++-
>   drivers/infiniband/core/device.c | 9 +++++++++
>   include/rdma/ib_cache.h          | 6 ++++++
>   include/rdma/ib_verbs.h          | 6 ++++++
>   4 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> index b6700ad..724ac0e 100644
> --- a/drivers/infiniband/core/cache.c
> +++ b/drivers/infiniband/core/cache.c
> @@ -1624,6 +1624,8 @@ int ib_cache_setup_one(struct ib_device *device)
>                  err = ib_cache_update(device, p, true);
>                  if (err)
>                          return err;
> +               set_bit(IB_PORT_CACHE_INITIALIZED,
> +                       &device->port_data[p].flags);
>          }
> 
>          return 0;
> @@ -1639,8 +1641,11 @@ void ib_cache_release_one(struct ib_device *device)
>           * all the device's resources when the cache could no
>           * longer be accessed.
>           */
> -       rdma_for_each_port (device, p)
> +       rdma_for_each_port (device, p) {
> +               clear_bit(IB_PORT_CACHE_INITIALIZED,
> +                        &device->port_data[p].flags);
>                  kfree(device->port_data[p].cache.pkey);
> +       }
> 
>          gid_table_release_one(device);
>   }

Do we need to clear it in gid_table_cleanup_one()?

Re: [PATCH v2 3/3] IB/core: Obtain subnet_prefix from cache in IB devices

[Haakon Bugge]

> On 3 Jun 2021, at 14:10, Mark Zhang <markzhang@nvidia.com> wrote:
> 
> On 6/3/2021 2:50 PM, Anand Khoje wrote:
>> External email: Use caution opening links or attachments
>> 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.
>> Fixes: fad61ad ("IB/core: Add subnet prefix to port info")
>> Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
>> Signed-off-by: Haakon Bugge <haakon.bugge@oracle.com>
>> ---
>>  drivers/infiniband/core/cache.c  | 7 ++++++-
>>  drivers/infiniband/core/device.c | 9 +++++++++
>>  include/rdma/ib_cache.h          | 6 ++++++
>>  include/rdma/ib_verbs.h          | 6 ++++++
>>  4 files changed, 27 insertions(+), 1 deletion(-)
>> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
>> index b6700ad..724ac0e 100644
>> --- a/drivers/infiniband/core/cache.c
>> +++ b/drivers/infiniband/core/cache.c
>> @@ -1624,6 +1624,8 @@ int ib_cache_setup_one(struct ib_device *device)
>>                 err = ib_cache_update(device, p, true);
>>                 if (err)
>>                         return err;
>> +               set_bit(IB_PORT_CACHE_INITIALIZED,
>> +                       &device->port_data[p].flags);
>>         }
>>         return 0;
>> @@ -1639,8 +1641,11 @@ void ib_cache_release_one(struct ib_device *device)
>>          * all the device's resources when the cache could no
>>          * longer be accessed.
>>          */
>> -       rdma_for_each_port (device, p)
>> +       rdma_for_each_port (device, p) {
>> +               clear_bit(IB_PORT_CACHE_INITIALIZED,
>> +                        &device->port_data[p].flags);
>>                 kfree(device->port_data[p].cache.pkey);
>> +       }
>>         gid_table_release_one(device);
>>  }
> 
> Do we need to clear it in gid_table_cleanup_one()?

Good point. Is it feasible that ib_query_port() can be called on a device that has been removed? If yes, we need it in gid_table_cleanup_one() as well.


Thxs, Håkon