diff mbox series

[21/32] hfi1: Convert hfi1_unit_table to XArray

Message ID 20190221002107.22625-22-willy@infradead.org (mailing list archive)
State Accepted
Delegated to: Jason Gunthorpe
Headers show
Series Convert the Infiniband subsystem to XArray | expand

Commit Message

Matthew Wilcox (Oracle) Feb. 21, 2019, 12:20 a.m. UTC
Also remove hfi1_devs_list.

Signed-off-by: Matthew Wilcox <willy@infradead.org>
---
 drivers/infiniband/hw/hfi1/chip.c    | 16 ++++-----
 drivers/infiniband/hw/hfi1/debugfs.c |  8 ++---
 drivers/infiniband/hw/hfi1/driver.c  | 10 +++---
 drivers/infiniband/hw/hfi1/hfi.h     |  5 ++-
 drivers/infiniband/hw/hfi1/init.c    | 51 +++++-----------------------
 drivers/infiniband/hw/hfi1/verbs.c   |  8 ++---
 6 files changed, 30 insertions(+), 68 deletions(-)

Comments

Dennis Dalessandro April 1, 2019, 1:39 p.m. UTC | #1
On 2/20/2019 7:20 PM, Matthew Wilcox wrote:
> Also remove hfi1_devs_list.
> 
> Signed-off-by: Matthew Wilcox <willy@infradead.org>
> ---
>   drivers/infiniband/hw/hfi1/chip.c    | 16 ++++-----
>   drivers/infiniband/hw/hfi1/debugfs.c |  8 ++---
>   drivers/infiniband/hw/hfi1/driver.c  | 10 +++---
>   drivers/infiniband/hw/hfi1/hfi.h     |  5 ++-
>   drivers/infiniband/hw/hfi1/init.c    | 51 +++++-----------------------
>   drivers/infiniband/hw/hfi1/verbs.c   |  8 ++---
>   6 files changed, 30 insertions(+), 68 deletions(-)
>
Not able to apply on top of rdma/for-next so I didn't test it but it 
looks mostly OK to me except one issue noted below.

> @@ -1293,21 +1275,10 @@ static struct hfi1_devdata *hfi1_alloc_devdata(struct pci_dev *pdev,
>   	dd->pport = (struct hfi1_pportdata *)(dd + 1);
>   	dd->pcidev = pdev;
>   	pci_set_drvdata(pdev, dd);
> -
> -	INIT_LIST_HEAD(&dd->list);
> -	idr_preload(GFP_KERNEL);
> -	spin_lock_irqsave(&hfi1_devs_lock, flags);
> -
> -	ret = idr_alloc(&hfi1_unit_table, dd, 0, 0, GFP_NOWAIT);
> -	if (ret >= 0) {
> -		dd->unit = ret;
> -		list_add(&dd->list, &hfi1_dev_list);
> -	}
>   	dd->node = -1;
>   
> -	spin_unlock_irqrestore(&hfi1_devs_lock, flags);
> -	idr_preload_end();
> -
> +	ret = xa_alloc_irq(&hfi1_dev_table, &dd->unit, dd, xa_limit_32b,
> +			GFP_KERNEL);
>   	if (ret < 0) {
>   		dev_err(&pdev->dev,
>   			"Could not allocate unit ID: error %d\n", -ret);

Why change this from a no sleep to being able to sleep?


-Denny
Matthew Wilcox (Oracle) April 1, 2019, 1:54 p.m. UTC | #2
On Mon, Apr 01, 2019 at 09:39:33AM -0400, Dennis Dalessandro wrote:
> > -	INIT_LIST_HEAD(&dd->list);
> > -	idr_preload(GFP_KERNEL);
> > -	spin_lock_irqsave(&hfi1_devs_lock, flags);
> > -
> > -	ret = idr_alloc(&hfi1_unit_table, dd, 0, 0, GFP_NOWAIT);
> > -	if (ret >= 0) {
> > -		dd->unit = ret;
> > -		list_add(&dd->list, &hfi1_dev_list);
> > -	}
> >   	dd->node = -1;
> > -	spin_unlock_irqrestore(&hfi1_devs_lock, flags);
> > -	idr_preload_end();
> > -
> > +	ret = xa_alloc_irq(&hfi1_dev_table, &dd->unit, dd, xa_limit_32b,
> > +			GFP_KERNEL);
> 
> Why change this from a no sleep to being able to sleep?

Look closer; I didn't.
Dennis Dalessandro April 1, 2019, 1:58 p.m. UTC | #3
On 4/1/2019 9:54 AM, Matthew Wilcox wrote:
> On Mon, Apr 01, 2019 at 09:39:33AM -0400, Dennis Dalessandro wrote:
>>> -	INIT_LIST_HEAD(&dd->list);
>>> -	idr_preload(GFP_KERNEL);
>>> -	spin_lock_irqsave(&hfi1_devs_lock, flags);
>>> -
>>> -	ret = idr_alloc(&hfi1_unit_table, dd, 0, 0, GFP_NOWAIT);
>>> -	if (ret >= 0) {
>>> -		dd->unit = ret;
>>> -		list_add(&dd->list, &hfi1_dev_list);
>>> -	}
>>>    	dd->node = -1;
>>> -	spin_unlock_irqrestore(&hfi1_devs_lock, flags);
>>> -	idr_preload_end();
>>> -
>>> +	ret = xa_alloc_irq(&hfi1_dev_table, &dd->unit, dd, xa_limit_32b,
>>> +			GFP_KERNEL);
>>
>> Why change this from a no sleep to being able to sleep?
> 
> Look closer; I didn't.
> 

Ah I see it now. Ok looks good. Will make sure it gets tested when it 
makes it's way into the rdma-next tree.

Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
Jason Gunthorpe April 1, 2019, 4:38 p.m. UTC | #4
On Mon, Apr 01, 2019 at 09:58:36AM -0400, Dennis Dalessandro wrote:
> On 4/1/2019 9:54 AM, Matthew Wilcox wrote:
> > On Mon, Apr 01, 2019 at 09:39:33AM -0400, Dennis Dalessandro wrote:
> > > > -	INIT_LIST_HEAD(&dd->list);
> > > > -	idr_preload(GFP_KERNEL);
> > > > -	spin_lock_irqsave(&hfi1_devs_lock, flags);
> > > > -
> > > > -	ret = idr_alloc(&hfi1_unit_table, dd, 0, 0, GFP_NOWAIT);
> > > > -	if (ret >= 0) {
> > > > -		dd->unit = ret;
> > > > -		list_add(&dd->list, &hfi1_dev_list);
> > > > -	}
> > > >    	dd->node = -1;
> > > > -	spin_unlock_irqrestore(&hfi1_devs_lock, flags);
> > > > -	idr_preload_end();
> > > > -
> > > > +	ret = xa_alloc_irq(&hfi1_dev_table, &dd->unit, dd, xa_limit_32b,
> > > > +			GFP_KERNEL);
> > > 
> > > Why change this from a no sleep to being able to sleep?
> > 
> > Look closer; I didn't.
> > 
> 
> Ah I see it now. Ok looks good. Will make sure it gets tested when it makes
> it's way into the rdma-next tree.
> 
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com>

Applied to for-next, thanks

Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/hw/hfi1/chip.c b/drivers/infiniband/hw/hfi1/chip.c
index b443642eac02..ad8a3216cde4 100644
--- a/drivers/infiniband/hw/hfi1/chip.c
+++ b/drivers/infiniband/hw/hfi1/chip.c
@@ -14641,8 +14641,8 @@  void hfi1_start_cleanup(struct hfi1_devdata *dd)
  */
 static int init_asic_data(struct hfi1_devdata *dd)
 {
-	unsigned long flags;
-	struct hfi1_devdata *tmp, *peer = NULL;
+	unsigned long index;
+	struct hfi1_devdata *peer;
 	struct hfi1_asic_data *asic_data;
 	int ret = 0;
 
@@ -14651,14 +14651,12 @@  static int init_asic_data(struct hfi1_devdata *dd)
 	if (!asic_data)
 		return -ENOMEM;
 
-	spin_lock_irqsave(&hfi1_devs_lock, flags);
+	xa_lock_irq(&hfi1_dev_table);
 	/* Find our peer device */
-	list_for_each_entry(tmp, &hfi1_dev_list, list) {
-		if ((HFI_BASE_GUID(dd) == HFI_BASE_GUID(tmp)) &&
-		    dd->unit != tmp->unit) {
-			peer = tmp;
+	xa_for_each(&hfi1_dev_table, index, peer) {
+		if ((HFI_BASE_GUID(dd) == HFI_BASE_GUID(peer)) &&
+		    dd->unit != peer->unit)
 			break;
-		}
 	}
 
 	if (peer) {
@@ -14670,7 +14668,7 @@  static int init_asic_data(struct hfi1_devdata *dd)
 		mutex_init(&dd->asic_data->asic_resource_mutex);
 	}
 	dd->asic_data->dds[dd->hfi1_id] = dd; /* self back-pointer */
-	spin_unlock_irqrestore(&hfi1_devs_lock, flags);
+	xa_unlock_irq(&hfi1_dev_table);
 
 	/* first one through - set up i2c devices */
 	if (!peer)
diff --git a/drivers/infiniband/hw/hfi1/debugfs.c b/drivers/infiniband/hw/hfi1/debugfs.c
index 0a557795563c..6d904636a6b0 100644
--- a/drivers/infiniband/hw/hfi1/debugfs.c
+++ b/drivers/infiniband/hw/hfi1/debugfs.c
@@ -1304,15 +1304,15 @@  static void _driver_stats_seq_stop(struct seq_file *s, void *v)
 
 static u64 hfi1_sps_ints(void)
 {
-	unsigned long flags;
+	unsigned long index, flags;
 	struct hfi1_devdata *dd;
 	u64 sps_ints = 0;
 
-	spin_lock_irqsave(&hfi1_devs_lock, flags);
-	list_for_each_entry(dd, &hfi1_dev_list, list) {
+	xa_lock_irqsave(&hfi1_dev_table, flags);
+	xa_for_each(&hfi1_dev_table, index, dd) {
 		sps_ints += get_all_cpu_total(dd->int_counter);
 	}
-	spin_unlock_irqrestore(&hfi1_devs_lock, flags);
+	xa_unlock_irqrestore(&hfi1_dev_table, flags);
 	return sps_ints;
 }
 
diff --git a/drivers/infiniband/hw/hfi1/driver.c b/drivers/infiniband/hw/hfi1/driver.c
index a8ad70730203..fcc64c7ba0d8 100644
--- a/drivers/infiniband/hw/hfi1/driver.c
+++ b/drivers/infiniband/hw/hfi1/driver.c
@@ -72,8 +72,6 @@ 
  */
 const char ib_hfi1_version[] = HFI1_DRIVER_VERSION "\n";
 
-DEFINE_SPINLOCK(hfi1_devs_lock);
-LIST_HEAD(hfi1_dev_list);
 DEFINE_MUTEX(hfi1_mutex);	/* general driver use */
 
 unsigned int hfi1_max_mtu = HFI1_DEFAULT_MAX_MTU;
@@ -175,11 +173,11 @@  int hfi1_count_active_units(void)
 {
 	struct hfi1_devdata *dd;
 	struct hfi1_pportdata *ppd;
-	unsigned long flags;
+	unsigned long index, flags;
 	int pidx, nunits_active = 0;
 
-	spin_lock_irqsave(&hfi1_devs_lock, flags);
-	list_for_each_entry(dd, &hfi1_dev_list, list) {
+	xa_lock_irqsave(&hfi1_dev_table, flags);
+	xa_for_each(&hfi1_dev_table, index, dd) {
 		if (!(dd->flags & HFI1_PRESENT) || !dd->kregbase1)
 			continue;
 		for (pidx = 0; pidx < dd->num_pports; ++pidx) {
@@ -190,7 +188,7 @@  int hfi1_count_active_units(void)
 			}
 		}
 	}
-	spin_unlock_irqrestore(&hfi1_devs_lock, flags);
+	xa_unlock_irqrestore(&hfi1_dev_table, flags);
 	return nunits_active;
 }
 
diff --git a/drivers/infiniband/hw/hfi1/hfi.h b/drivers/infiniband/hw/hfi1/hfi.h
index 6db2276f5c13..728f3d6fa179 100644
--- a/drivers/infiniband/hw/hfi1/hfi.h
+++ b/drivers/infiniband/hw/hfi1/hfi.h
@@ -65,6 +65,7 @@ 
 #include <linux/kthread.h>
 #include <linux/i2c.h>
 #include <linux/i2c-algo-bit.h>
+#include <linux/xarray.h>
 #include <rdma/ib_hdrs.h>
 #include <rdma/opa_addr.h>
 #include <linux/rhashtable.h>
@@ -1021,7 +1022,6 @@  struct sdma_vl_map;
 typedef int (*send_routine)(struct rvt_qp *, struct hfi1_pkt_state *, u64);
 struct hfi1_devdata {
 	struct hfi1_ibdev verbs_dev;     /* must be first */
-	struct list_head list;
 	/* pointers to related structs for this device */
 	/* pci access data structure */
 	struct pci_dev *pcidev;
@@ -1406,8 +1406,7 @@  struct hfi1_filedata {
 	struct mm_struct *mm;
 };
 
-extern struct list_head hfi1_dev_list;
-extern spinlock_t hfi1_devs_lock;
+extern struct xarray hfi1_dev_table;
 struct hfi1_devdata *hfi1_lookup(int unit);
 
 static inline unsigned long uctxt_offset(struct hfi1_ctxtdata *uctxt)
diff --git a/drivers/infiniband/hw/hfi1/init.c b/drivers/infiniband/hw/hfi1/init.c
index 7835eb52e7c5..090f948fdeb0 100644
--- a/drivers/infiniband/hw/hfi1/init.c
+++ b/drivers/infiniband/hw/hfi1/init.c
@@ -49,7 +49,7 @@ 
 #include <linux/netdevice.h>
 #include <linux/vmalloc.h>
 #include <linux/delay.h>
-#include <linux/idr.h>
+#include <linux/xarray.h>
 #include <linux/module.h>
 #include <linux/printk.h>
 #include <linux/hrtimer.h>
@@ -124,7 +124,7 @@  MODULE_PARM_DESC(user_credit_return_threshold, "Credit return threshold for user
 
 static inline u64 encode_rcv_header_entry_size(u16 size);
 
-static struct idr hfi1_unit_table;
+DEFINE_XARRAY_FLAGS(hfi1_dev_table, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ);
 
 static int hfi1_create_kctxt(struct hfi1_devdata *dd,
 			     struct hfi1_pportdata *ppd)
@@ -1004,21 +1004,9 @@  int hfi1_init(struct hfi1_devdata *dd, int reinit)
 	return ret;
 }
 
-static inline struct hfi1_devdata *__hfi1_lookup(int unit)
-{
-	return idr_find(&hfi1_unit_table, unit);
-}
-
 struct hfi1_devdata *hfi1_lookup(int unit)
 {
-	struct hfi1_devdata *dd;
-	unsigned long flags;
-
-	spin_lock_irqsave(&hfi1_devs_lock, flags);
-	dd = __hfi1_lookup(unit);
-	spin_unlock_irqrestore(&hfi1_devs_lock, flags);
-
-	return dd;
+	return xa_load(&hfi1_dev_table, unit);
 }
 
 /*
@@ -1186,7 +1174,7 @@  void hfi1_free_ctxtdata(struct hfi1_devdata *dd, struct hfi1_ctxtdata *rcd)
 /*
  * Release our hold on the shared asic data.  If we are the last one,
  * return the structure to be finalized outside the lock.  Must be
- * holding hfi1_devs_lock.
+ * holding hfi1_dev_table lock.
  */
 static struct hfi1_asic_data *release_asic_data(struct hfi1_devdata *dd)
 {
@@ -1222,13 +1210,10 @@  static void hfi1_clean_devdata(struct hfi1_devdata *dd)
 	struct hfi1_asic_data *ad;
 	unsigned long flags;
 
-	spin_lock_irqsave(&hfi1_devs_lock, flags);
-	if (!list_empty(&dd->list)) {
-		idr_remove(&hfi1_unit_table, dd->unit);
-		list_del_init(&dd->list);
-	}
+	xa_lock_irqsave(&hfi1_dev_table, flags);
+	__xa_erase(&hfi1_dev_table, dd->unit);
 	ad = release_asic_data(dd);
-	spin_unlock_irqrestore(&hfi1_devs_lock, flags);
+	xa_unlock_irqrestore(&hfi1_dev_table, flags);
 
 	finalize_asic_data(dd, ad);
 	free_platform_config(dd);
@@ -1272,13 +1257,10 @@  void hfi1_free_devdata(struct hfi1_devdata *dd)
  * Must be done via verbs allocator, because the verbs cleanup process
  * both does cleanup and free of the data structure.
  * "extra" is for chip-specific data.
- *
- * Use the idr mechanism to get a unit number for this unit.
  */
 static struct hfi1_devdata *hfi1_alloc_devdata(struct pci_dev *pdev,
 					       size_t extra)
 {
-	unsigned long flags;
 	struct hfi1_devdata *dd;
 	int ret, nports;
 
@@ -1293,21 +1275,10 @@  static struct hfi1_devdata *hfi1_alloc_devdata(struct pci_dev *pdev,
 	dd->pport = (struct hfi1_pportdata *)(dd + 1);
 	dd->pcidev = pdev;
 	pci_set_drvdata(pdev, dd);
-
-	INIT_LIST_HEAD(&dd->list);
-	idr_preload(GFP_KERNEL);
-	spin_lock_irqsave(&hfi1_devs_lock, flags);
-
-	ret = idr_alloc(&hfi1_unit_table, dd, 0, 0, GFP_NOWAIT);
-	if (ret >= 0) {
-		dd->unit = ret;
-		list_add(&dd->list, &hfi1_dev_list);
-	}
 	dd->node = -1;
 
-	spin_unlock_irqrestore(&hfi1_devs_lock, flags);
-	idr_preload_end();
-
+	ret = xa_alloc_irq(&hfi1_dev_table, &dd->unit, dd, xa_limit_32b,
+			GFP_KERNEL);
 	if (ret < 0) {
 		dev_err(&pdev->dev,
 			"Could not allocate unit ID: error %d\n", -ret);
@@ -1501,8 +1472,6 @@  static int __init hfi1_mod_init(void)
 	 * These must be called before the driver is registered with
 	 * the PCI subsystem.
 	 */
-	idr_init(&hfi1_unit_table);
-
 	hfi1_dbg_init();
 	ret = pci_register_driver(&hfi1_pci_driver);
 	if (ret < 0) {
@@ -1513,7 +1482,6 @@  static int __init hfi1_mod_init(void)
 
 bail_dev:
 	hfi1_dbg_exit();
-	idr_destroy(&hfi1_unit_table);
 	dev_cleanup();
 bail:
 	return ret;
@@ -1530,7 +1498,6 @@  static void __exit hfi1_mod_cleanup(void)
 	node_affinity_destroy_all();
 	hfi1_dbg_exit();
 
-	idr_destroy(&hfi1_unit_table);
 	dispose_firmware();	/* asymmetric with obtain_firmware() */
 	dev_cleanup();
 }
diff --git a/drivers/infiniband/hw/hfi1/verbs.c b/drivers/infiniband/hw/hfi1/verbs.c
index ec582d86025f..7201087d1dfb 100644
--- a/drivers/infiniband/hw/hfi1/verbs.c
+++ b/drivers/infiniband/hw/hfi1/verbs.c
@@ -1579,15 +1579,15 @@  static struct rdma_hw_stats *alloc_hw_stats(struct ib_device *ibdev,
 
 static u64 hfi1_sps_ints(void)
 {
-	unsigned long flags;
+	unsigned long index, flags;
 	struct hfi1_devdata *dd;
 	u64 sps_ints = 0;
 
-	spin_lock_irqsave(&hfi1_devs_lock, flags);
-	list_for_each_entry(dd, &hfi1_dev_list, list) {
+	xa_lock_irqsave(&hfi1_dev_table, flags);
+	xa_for_each(&hfi1_dev_table, index, dd) {
 		sps_ints += get_all_cpu_total(dd->int_counter);
 	}
-	spin_unlock_irqrestore(&hfi1_devs_lock, flags);
+	xa_unlock_irqrestore(&hfi1_dev_table, flags);
 	return sps_ints;
 }