diff mbox series

RDMA/device: Add ib_device_get_by_name() and use it in rxe

Message ID 20190112023752.GA24713@ziepe.ca (mailing list archive)
State Changes Requested
Headers show
Series RDMA/device: Add ib_device_get_by_name() and use it in rxe | expand

Commit Message

Jason Gunthorpe Jan. 12, 2019, 2:37 a.m. UTC 
rxe has an open coded version of this that is not as safe as the core
version can be.

Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
 drivers/infiniband/core/device.c      | 28 +++++++++++++++++++++++++++
 drivers/infiniband/sw/rxe/rxe.h       |  1 -
 drivers/infiniband/sw/rxe/rxe_net.c   | 17 ----------------
 drivers/infiniband/sw/rxe/rxe_sysfs.c |  8 +++++---
 include/rdma/ib_verbs.h               |  2 ++
 5 files changed, 35 insertions(+), 21 deletions(-)

More usage of device_try_get/put()

Requires the advise_mr series sent to for-rc

Comments

Zhu Yanjun Jan. 14, 2019, 10 a.m. UTC | #1 
When apply this patch to v5.0-rc2, the following will appear.

patching file include/rdma/ib_verbs.h
Hunk #1 FAILED at 3944.
1 out of 1 hunk FAILED -- saving rejects to file include/rdma/ib_verbs.h.rej

It seems that "ib_device_put" does not exist in v5.0-rc2

Zhu Yanjun

On 2019/1/12 10:37, Jason Gunthorpe wrote:
> rxe has an open coded version of this that is not as safe as the core
> version can be.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
>   drivers/infiniband/core/device.c      | 28 +++++++++++++++++++++++++++
>   drivers/infiniband/sw/rxe/rxe.h       |  1 -
>   drivers/infiniband/sw/rxe/rxe_net.c   | 17 ----------------
>   drivers/infiniband/sw/rxe/rxe_sysfs.c |  8 +++++---
>   include/rdma/ib_verbs.h               |  2 ++
>   5 files changed, 35 insertions(+), 21 deletions(-)
>
> More usage of device_try_get/put()
>
> Requires the advise_mr series sent to for-rc
>
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 9b5c72d3c85a88..1311d6e5f28c03 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -181,6 +181,34 @@ static struct ib_device *__ib_device_get_by_name(const char *name)
>   	return NULL;
>   }
>   
> +/**
> + * ib_device_get_by_name - Find an IB device by name
> + * @name: The name to look for
> + * @driver_id: The driver ID that must match (RDMA_DRIVER_UNKNOWN matches all)
> + *
> + * Find and hold an ib_device by its name. The caller must call
> + * ib_device_put() on the returned pointer.
> + */
> +struct ib_device *ib_device_get_by_name(const char *name,
> +					enum rdma_driver_id driver_id)
> +{
> +	struct ib_device *device;
> +
> +	down_read(&lists_rwsem);
> +	device = __ib_device_get_by_name(name);
> +	if (device && driver_id != RDMA_DRIVER_UNKNOWN &&
> +	    device->driver_id != driver_id)
> +		device = NULL;
> +
> +	if (device) {
> +		if (!ib_device_try_get(device))
> +			device = NULL;
> +	}
> +	up_read(&lists_rwsem);
> +	return device;
> +}
> +EXPORT_SYMBOL(ib_device_get_by_name);
> +
>   int ib_device_rename(struct ib_device *ibdev, const char *name)
>   {
>   	struct ib_device *device;
> diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
> index 5bde2ad964d277..4f8653c5d1ca5e 100644
> --- a/drivers/infiniband/sw/rxe/rxe.h
> +++ b/drivers/infiniband/sw/rxe/rxe.h
> @@ -106,7 +106,6 @@ static inline void rxe_dev_put(struct rxe_dev *rxe)
>   	kref_put(&rxe->ref_cnt, rxe_release);
>   }
>   struct rxe_dev *net_to_rxe(struct net_device *ndev);
> -struct rxe_dev *get_rxe_by_name(const char *name);
>   
>   void rxe_port_up(struct rxe_dev *rxe);
>   void rxe_port_down(struct rxe_dev *rxe);
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index 8fd03ae20efc17..6a4f58a8656578 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -65,23 +65,6 @@ struct rxe_dev *net_to_rxe(struct net_device *ndev)
>   	return found;
>   }
>   
> -struct rxe_dev *get_rxe_by_name(const char *name)
> -{
> -	struct rxe_dev *rxe;
> -	struct rxe_dev *found = NULL;
> -
> -	spin_lock_bh(&dev_list_lock);
> -	list_for_each_entry(rxe, &rxe_dev_list, list) {
> -		if (!strcmp(name, dev_name(&rxe->ib_dev.dev))) {
> -			found = rxe;
> -			break;
> -		}
> -	}
> -	spin_unlock_bh(&dev_list_lock);
> -	return found;
> -}
> -
> -
>   static struct rxe_recv_sockets recv_sockets;
>   
>   struct device *rxe_dma_device(struct rxe_dev *rxe)
> diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> index 95a15892f7e659..66c39049cb2d18 100644
> --- a/drivers/infiniband/sw/rxe/rxe_sysfs.c
> +++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> @@ -100,6 +100,7 @@ static int rxe_param_set_remove(const char *val, const struct kernel_param *kp)
>   {
>   	int len;
>   	char intf[32];
> +	struct ib_device *ib_dev;
>   	struct rxe_dev *rxe;
>   
>   	len = sanitize_arg(val, intf, sizeof(intf));
> @@ -114,15 +115,16 @@ static int rxe_param_set_remove(const char *val, const struct kernel_param *kp)
>   		return 0;
>   	}
>   
> -	rxe = get_rxe_by_name(intf);
> -
> -	if (!rxe) {
> +	ib_dev = ib_device_get_by_name(intf, RDMA_DRIVER_RXE);
> +	if (!ib_dev) {
>   		pr_err("not configured on %s\n", intf);
>   		return -EINVAL;
>   	}
>   
> +	rxe = container_of(ib_dev, struct rxe_dev, ib_dev);
>   	list_del(&rxe->list);
>   	rxe_remove(rxe);
> +	ib_device_put(ib_dev);
>   
>   	return 0;
>   }
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 85e9dab17b9b92..d4d6bd6c0ae121 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -3944,6 +3944,8 @@ static inline bool ib_device_try_get(struct ib_device *dev)
>   }
>   
>   void ib_device_put(struct ib_device *device);
> +struct ib_device *ib_device_get_by_name(const char *name,
> +					enum rdma_driver_id driver_id);
>   struct net_device *ib_get_net_dev_by_params(struct ib_device *dev, u8 port,
>   					    u16 pkey, const union ib_gid *gid,
>   					    const struct sockaddr *addr);
Jason Gunthorpe Jan. 14, 2019, 7:27 p.m. UTC | #2 
On Mon, Jan 14, 2019 at 06:00:09PM +0800, Yanjun Zhu wrote:
> When apply this patch to v5.0-rc2, the following will appear.
> 
> patching file include/rdma/ib_verbs.h
> Hunk #1 FAILED at 3944.
> 1 out of 1 hunk FAILED -- saving rejects to file include/rdma/ib_verbs.h.rej
> 
> It seems that "ib_device_put" does not exist in v5.0-rc2

Yes, this needs the patch I sent earlier

Jason
Zhu Yanjun Jan. 15, 2019, 2:51 a.m. UTC | #3 
On 2019/1/15 3:27, Jason Gunthorpe wrote:
> On Mon, Jan 14, 2019 at 06:00:09PM +0800, Yanjun Zhu wrote:
>> When apply this patch to v5.0-rc2, the following will appear.
>>
>> patching file include/rdma/ib_verbs.h
>> Hunk #1 FAILED at 3944.
>> 1 out of 1 hunk FAILED -- saving rejects to file include/rdma/ib_verbs.h.rej
>>
>> It seems that "ib_device_put" does not exist in v5.0-rc2
> Yes, this needs the patch I sent earlier
Sure. I found it in "[PATCH rdma-next v1] RDMA/core: Sync unregistration 
with netlink commands"

+void ib_device_put(struct ib_device *device)
+{
+    if (refcount_dec_and_test(&device->refcount))
+        complete(&device->unreg_completion);
+}

+

Reviewed-by: Zhu Yanjun <yanjun.zhu@oracle.com>

>
> Jason
>
Zhu Yanjun Jan. 15, 2019, 3:06 a.m. UTC | #4 
On 2019/1/12 10:37, Jason Gunthorpe wrote:
> rxe has an open coded version of this that is not as safe as the core
> version can be.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
>   drivers/infiniband/core/device.c      | 28 +++++++++++++++++++++++++++
>   drivers/infiniband/sw/rxe/rxe.h       |  1 -
>   drivers/infiniband/sw/rxe/rxe_net.c   | 17 ----------------
>   drivers/infiniband/sw/rxe/rxe_sysfs.c |  8 +++++---
>   include/rdma/ib_verbs.h               |  2 ++
>   5 files changed, 35 insertions(+), 21 deletions(-)
>
> More usage of device_try_get/put()
>
> Requires the advise_mr series sent to for-rc
>
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 9b5c72d3c85a88..1311d6e5f28c03 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -181,6 +181,34 @@ static struct ib_device *__ib_device_get_by_name(const char *name)
>   	return NULL;
>   }
>   
> +/**
> + * ib_device_get_by_name - Find an IB device by name
> + * @name: The name to look for
> + * @driver_id: The driver ID that must match (RDMA_DRIVER_UNKNOWN matches all)
> + *
> + * Find and hold an ib_device by its name. The caller must call
> + * ib_device_put() on the returned pointer.
> + */
> +struct ib_device *ib_device_get_by_name(const char *name,
> +					enum rdma_driver_id driver_id)
> +{
> +	struct ib_device *device;
> +
> +	down_read(&lists_rwsem);
> +	device = __ib_device_get_by_name(name);
> +	if (device && driver_id != RDMA_DRIVER_UNKNOWN &&
> +	    device->driver_id != driver_id)
> +		device = NULL;
> +
> +	if (device) {
> +		if (!ib_device_try_get(device))
In the patch [PATCH for-rc 1/2] RDMA/device: Expose ib_device_try_get((),

+/**
+ * ib_device_try_get: Hold a registration lock
+ * device: The device to lock
+ *
+ * A device under an active registration lock cannot become 
unregistered. It
+ * is only possible to obtain a registration lock on a device that is fully
+ * registered, otherwise this function returns false.
+ *
+ * The registration lock is only necessary for actions which require the
+ * device to still be registered. Uses that only require the device 
pointer to
+ * be valid should use get_device(&ibdev->dev) to hold the memory.
+ *
+ */
+static inline bool ib_device_try_get(struct ib_device *dev)
+{
+    return refcount_inc_not_zero(&dev->refcount);
+}
+
> +			device = NULL;
> +	}
> +	up_read(&lists_rwsem);
> +	return device;
> +}
> +EXPORT_SYMBOL(ib_device_get_by_name);
> +
>   int ib_device_rename(struct ib_device *ibdev, const char *name)
>   {
>   	struct ib_device *device;
> diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
> index 5bde2ad964d277..4f8653c5d1ca5e 100644
> --- a/drivers/infiniband/sw/rxe/rxe.h
> +++ b/drivers/infiniband/sw/rxe/rxe.h
> @@ -106,7 +106,6 @@ static inline void rxe_dev_put(struct rxe_dev *rxe)
>   	kref_put(&rxe->ref_cnt, rxe_release);
>   }
>   struct rxe_dev *net_to_rxe(struct net_device *ndev);
> -struct rxe_dev *get_rxe_by_name(const char *name);
>   
>   void rxe_port_up(struct rxe_dev *rxe);
>   void rxe_port_down(struct rxe_dev *rxe);
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index 8fd03ae20efc17..6a4f58a8656578 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -65,23 +65,6 @@ struct rxe_dev *net_to_rxe(struct net_device *ndev)
>   	return found;
>   }
>   
> -struct rxe_dev *get_rxe_by_name(const char *name)
> -{
> -	struct rxe_dev *rxe;
> -	struct rxe_dev *found = NULL;
> -
> -	spin_lock_bh(&dev_list_lock);
> -	list_for_each_entry(rxe, &rxe_dev_list, list) {
> -		if (!strcmp(name, dev_name(&rxe->ib_dev.dev))) {
> -			found = rxe;
> -			break;
> -		}
> -	}
> -	spin_unlock_bh(&dev_list_lock);
> -	return found;
> -}
> -
> -
>   static struct rxe_recv_sockets recv_sockets;
>   
>   struct device *rxe_dma_device(struct rxe_dev *rxe)
> diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> index 95a15892f7e659..66c39049cb2d18 100644
> --- a/drivers/infiniband/sw/rxe/rxe_sysfs.c
> +++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> @@ -100,6 +100,7 @@ static int rxe_param_set_remove(const char *val, const struct kernel_param *kp)
>   {
>   	int len;
>   	char intf[32];
> +	struct ib_device *ib_dev;
>   	struct rxe_dev *rxe;
>   
>   	len = sanitize_arg(val, intf, sizeof(intf));
> @@ -114,15 +115,16 @@ static int rxe_param_set_remove(const char *val, const struct kernel_param *kp)
>   		return 0;
>   	}
>   
> -	rxe = get_rxe_by_name(intf);
> -
> -	if (!rxe) {
> +	ib_dev = ib_device_get_by_name(intf, RDMA_DRIVER_RXE);
> +	if (!ib_dev) {
>   		pr_err("not configured on %s\n", intf);
>   		return -EINVAL;
>   	}
>   
> +	rxe = container_of(ib_dev, struct rxe_dev, ib_dev);
>   	list_del(&rxe->list);
>   	rxe_remove(rxe);
> +	ib_device_put(ib_dev);
>   
>   	return 0;
>   }
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 85e9dab17b9b92..d4d6bd6c0ae121 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -3944,6 +3944,8 @@ static inline bool ib_device_try_get(struct ib_device *dev)
>   }
>   
>   void ib_device_put(struct ib_device *device);
> +struct ib_device *ib_device_get_by_name(const char *name,
> +					enum rdma_driver_id driver_id);
>   struct net_device *ib_get_net_dev_by_params(struct ib_device *dev, u8 port,
>   					    u16 pkey, const union ib_gid *gid,
>   					    const struct sockaddr *addr);
diff mbox series

Patch

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 9b5c72d3c85a88..1311d6e5f28c03 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -181,6 +181,34 @@  static struct ib_device *__ib_device_get_by_name(const char *name)
 	return NULL;
 }
 
+/**
+ * ib_device_get_by_name - Find an IB device by name
+ * @name: The name to look for
+ * @driver_id: The driver ID that must match (RDMA_DRIVER_UNKNOWN matches all)
+ *
+ * Find and hold an ib_device by its name. The caller must call
+ * ib_device_put() on the returned pointer.
+ */
+struct ib_device *ib_device_get_by_name(const char *name,
+					enum rdma_driver_id driver_id)
+{
+	struct ib_device *device;
+
+	down_read(&lists_rwsem);
+	device = __ib_device_get_by_name(name);
+	if (device && driver_id != RDMA_DRIVER_UNKNOWN &&
+	    device->driver_id != driver_id)
+		device = NULL;
+
+	if (device) {
+		if (!ib_device_try_get(device))
+			device = NULL;
+	}
+	up_read(&lists_rwsem);
+	return device;
+}
+EXPORT_SYMBOL(ib_device_get_by_name);
+
 int ib_device_rename(struct ib_device *ibdev, const char *name)
 {
 	struct ib_device *device;
diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
index 5bde2ad964d277..4f8653c5d1ca5e 100644
--- a/drivers/infiniband/sw/rxe/rxe.h
+++ b/drivers/infiniband/sw/rxe/rxe.h
@@ -106,7 +106,6 @@  static inline void rxe_dev_put(struct rxe_dev *rxe)
 	kref_put(&rxe->ref_cnt, rxe_release);
 }
 struct rxe_dev *net_to_rxe(struct net_device *ndev);
-struct rxe_dev *get_rxe_by_name(const char *name);
 
 void rxe_port_up(struct rxe_dev *rxe);
 void rxe_port_down(struct rxe_dev *rxe);
diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
index 8fd03ae20efc17..6a4f58a8656578 100644
--- a/drivers/infiniband/sw/rxe/rxe_net.c
+++ b/drivers/infiniband/sw/rxe/rxe_net.c
@@ -65,23 +65,6 @@  struct rxe_dev *net_to_rxe(struct net_device *ndev)
 	return found;
 }
 
-struct rxe_dev *get_rxe_by_name(const char *name)
-{
-	struct rxe_dev *rxe;
-	struct rxe_dev *found = NULL;
-
-	spin_lock_bh(&dev_list_lock);
-	list_for_each_entry(rxe, &rxe_dev_list, list) {
-		if (!strcmp(name, dev_name(&rxe->ib_dev.dev))) {
-			found = rxe;
-			break;
-		}
-	}
-	spin_unlock_bh(&dev_list_lock);
-	return found;
-}
-
-
 static struct rxe_recv_sockets recv_sockets;
 
 struct device *rxe_dma_device(struct rxe_dev *rxe)
diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c
index 95a15892f7e659..66c39049cb2d18 100644
--- a/drivers/infiniband/sw/rxe/rxe_sysfs.c
+++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c
@@ -100,6 +100,7 @@  static int rxe_param_set_remove(const char *val, const struct kernel_param *kp)
 {
 	int len;
 	char intf[32];
+	struct ib_device *ib_dev;
 	struct rxe_dev *rxe;
 
 	len = sanitize_arg(val, intf, sizeof(intf));
@@ -114,15 +115,16 @@  static int rxe_param_set_remove(const char *val, const struct kernel_param *kp)
 		return 0;
 	}
 
-	rxe = get_rxe_by_name(intf);
-
-	if (!rxe) {
+	ib_dev = ib_device_get_by_name(intf, RDMA_DRIVER_RXE);
+	if (!ib_dev) {
 		pr_err("not configured on %s\n", intf);
 		return -EINVAL;
 	}
 
+	rxe = container_of(ib_dev, struct rxe_dev, ib_dev);
 	list_del(&rxe->list);
 	rxe_remove(rxe);
+	ib_device_put(ib_dev);
 
 	return 0;
 }
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 85e9dab17b9b92..d4d6bd6c0ae121 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -3944,6 +3944,8 @@  static inline bool ib_device_try_get(struct ib_device *dev)
 }
 
 void ib_device_put(struct ib_device *device);
+struct ib_device *ib_device_get_by_name(const char *name,
+					enum rdma_driver_id driver_id);
 struct net_device *ib_get_net_dev_by_params(struct ib_device *dev, u8 port,
 					    u16 pkey, const union ib_gid *gid,
 					    const struct sockaddr *addr);