Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1225431 > unrolled thread

[PATCH 00/17] Create and use ida and idr helper routines

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/17] Create and use ida and idr helper routines Lee Duncan <lduncan@suse.com> - 2015-09-15 19:00 +0200
    [PATCH 04/17] Update the ch driver to use idr helper functions. Lee Duncan <lduncan@suse.com> - 2015-09-15 19:00 +0200
    [PATCH 11/17] Update the rtsx multifunction driver to use idr helper functions. Lee Duncan <lduncan@suse.com> - 2015-09-15 19:00 +0200
    [PATCH 12/17] Update the TI Flash Media driver to use idr helper functions. Lee Duncan <lduncan@suse.com> - 2015-09-15 19:00 +0200
    [PATCH 02/17] Update scsi hosts to use idr for host number mgmt Lee Duncan <lduncan@suse.com> - 2015-09-15 19:00 +0200
    [PATCH 01/17] Add ida and idr helper routines. Lee Duncan <lduncan@suse.com> - 2015-09-15 19:00 +0200
      Re: [PATCH 01/17] Add ida and idr helper routines. James Bottomley <James.Bottomley@HansenPartnership.com> - 2015-09-15 20:30 +0200
      Re: [PATCH 01/17] Add ida and idr helper routines. Tejun Heo <tj@kernel.org> - 2015-09-15 20:30 +0200
        Re: [PATCH 01/17] Add ida and idr helper routines. James Bottomley <James.Bottomley@HansenPartnership.com> - 2015-09-15 20:40 +0200
          Re: [PATCH 01/17] Add ida and idr helper routines. Tejun Heo <tj@kernel.org> - 2015-09-15 20:50 +0200
            Re: [PATCH 01/17] Add ida and idr helper routines. Tejun Heo <tj@kernel.org> - 2015-09-18 17:50 +0200
            Re: [PATCH 01/17] Add ida and idr helper routines. Lee Duncan <lduncan@suse.com> - 2015-09-18 17:50 +0200
    [PATCH 03/17] Update the st driver to use idr helper functions. Lee Duncan <lduncan@suse.com> - 2015-09-15 19:00 +0200
    [PATCH 10/17] Update the DCA DMA driver to use idr helper functions. Lee Duncan <lduncan@suse.com> - 2015-09-15 19:00 +0200

#1225431 — [PATCH 00/17] Create and use ida and idr helper routines

The idr index management library supplies two sets of routines
for managing monotonically increasing index numbers. The "ida"
set of routines manage allocating and freeing simple index
numbers. The "idr" set of routines add the ability to save
an arbitrary pointer with each index.

Both sets of routines are used throughout the kernel, and it
was noted that many of them use the same or similar calling
sequences, making a helper function a useful addition.

This set of patches adds some helper functions, defined
as inline in <linux/idr.h>. In addition, any of the clients
of these idr library functions that could benefit from
using these helper functions where modified to use them.

In addition to cleaning up the code in the clients of these
functions, the SCSI hosts module, which used to use a simple
atomic integer for index management is converted to using
the idr set of routines to manage its index values as
well as to simplify and speed up host number to instance
lookups.

I have functionally tested the SCSI host indexing change,
and I have compile tested all of the other changes. The
maintainers of each driver are cc-ed on the patch series,
where available.

Note: I did not mark this patch series as "v2" since
the scope of the patch set has grown considerably
since my first submissions.

Summary: There is one patch that adds helper functions, 11
patches that use the new "idr" helper functions, and 5 that
use the new "ida" helper functions.

Lee Duncan (17):
   1. Add ida and idr helper routines.
   2. Update scsi hosts to use idr for host number mgmt
   3. Update the st driver to use idr helper functions.
   4. Update the ch driver to use idr helper functions.
   5. Update the md driver to use idr helper functions.
   6. Update the infiniband uverbs driver to use idr helper functions.
   7. Update the memstick driver to use idr helper functions.
   8. Update the mmc driver to use idr helper functions.
   9. Update the virtgpu driver to use idr helper functions.
  10. Update the DCA DMA driver to use idr helper functions.
  11. Update the rtsx multifunction driver to use idr helper functions.
  12. Update the TI Flash Media driver to use idr helper functions.
  13. Update the SCSI disk driver to use ida helper functions.
  14. Update the rsxx flash adapter driver to use ida helper functions.
  15. Update the NVMe SSD driver to use ida helper functions.
  16. Update the Micron PCIe SSD driver to use ida helper functions.
  17. Update the ARM soc base driver to use ida helper functions.

 drivers/base/soc.c                   |  17 ++----
 drivers/block/mtip32xx/mtip32xx.c    |  22 ++------
 drivers/block/nvme-core.c            |  14 +----
 drivers/block/rsxx/core.c            |  16 +-----
 drivers/dca/dca-sysfs.c              |  19 ++-----
 drivers/gpu/drm/virtio/virtgpu_vq.c  |  15 ++----
 drivers/infiniband/core/uverbs_cmd.c |  12 +----
 drivers/md/dm.c                      |  22 ++------
 drivers/memstick/core/memstick.c     |  19 ++-----
 drivers/mfd/rtsx_pcr.c               |  13 ++---
 drivers/misc/tifm_core.c             |  17 ++----
 drivers/mmc/core/host.c              |  14 ++---
 drivers/scsi/ch.c                    |  14 ++---
 drivers/scsi/hosts.c                 |  59 ++++++++++----------
 drivers/scsi/sd.c                    |  20 ++-----
 drivers/scsi/st.c                    |  15 ++----
 include/linux/idr.h                  | 102 +++++++++++++++++++++++++++++++++++
 17 files changed, 182 insertions(+), 228 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1225438 — [PATCH 04/17] Update the ch driver to use idr helper functions.

Note: when allocating an index, in the error case,
where the just-allocated index has to be released,
the required locking around the index removal was
not present, so this conversion has the side effect
of adding locking for that error condition. This
should close a possible race condition that could
have resulted in duplicate index allocation.

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/ch.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index dad959fcf6d8..2edf1f8883f9 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -909,12 +909,8 @@ static int ch_probe(struct device *dev)
 	if (NULL == ch)
 		return -ENOMEM;
 
-	idr_preload(GFP_KERNEL);
-	spin_lock(&ch_index_lock);
-	ret = idr_alloc(&ch_index_idr, ch, 0, CH_MAX_DEVS + 1, GFP_NOWAIT);
-	spin_unlock(&ch_index_lock);
-	idr_preload_end();
-
+	ret = idr_get_index_in_range(&ch_index_idr, &ch_index_lock, ch,
+				     0, CH_MAX_DEVS + 1);
 	if (ret < 0) {
 		if (ret == -ENOSPC)
 			ret = -ENODEV;
@@ -945,7 +941,7 @@ static int ch_probe(struct device *dev)
 
 	return 0;
 remove_idr:
-	idr_remove(&ch_index_idr, ch->minor);
+	idr_put_index(&ch_index_idr, &ch_index_lock, ch->minor);
 free_ch:
 	kfree(ch);
 	return ret;
@@ -955,9 +951,7 @@ static int ch_remove(struct device *dev)
 {
 	scsi_changer *ch = dev_get_drvdata(dev);
 
-	spin_lock(&ch_index_lock);
-	idr_remove(&ch_index_idr, ch->minor);
-	spin_unlock(&ch_index_lock);
+	idr_put_index(&ch_index_idr, &ch_index_lock, ch->minor);
 
 	device_destroy(ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR,ch->minor));
 	kfree(ch->dt);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225439 — [PATCH 11/17] Update the rtsx multifunction driver to use idr helper functions.

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/mfd/rtsx_pcr.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
index a66540a49079..8ddefb8c5e64 100644
--- a/drivers/mfd/rtsx_pcr.c
+++ b/drivers/mfd/rtsx_pcr.c
@@ -1191,15 +1191,10 @@ static int rtsx_pci_probe(struct pci_dev *pcidev,
 	}
 	handle->pcr = pcr;
 
-	idr_preload(GFP_KERNEL);
-	spin_lock(&rtsx_pci_lock);
-	ret = idr_alloc(&rtsx_pci_idr, pcr, 0, 0, GFP_NOWAIT);
-	if (ret >= 0)
-		pcr->id = ret;
-	spin_unlock(&rtsx_pci_lock);
-	idr_preload_end();
+	ret = idr_get_index(&rtsx_pci_idr, &rtsc_pci_lock, pcr);
 	if (ret < 0)
 		goto free_handle;
+	pcr->id = ret;
 
 	pcr->pci = pcidev;
 	dev_set_drvdata(&pcidev->dev, handle);
@@ -1311,9 +1306,7 @@ static void rtsx_pci_remove(struct pci_dev *pcidev)
 	pci_release_regions(pcidev);
 	pci_disable_device(pcidev);
 
-	spin_lock(&rtsx_pci_lock);
-	idr_remove(&rtsx_pci_idr, pcr->id);
-	spin_unlock(&rtsx_pci_lock);
+	idr_put_index(&rtsx_pci_idr, &rtsx_pci_lock, pcr->id);
 
 	kfree(pcr->slots);
 	kfree(pcr);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225440 — [PATCH 12/17] Update the TI Flash Media driver to use idr helper functions.

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/misc/tifm_core.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/misc/tifm_core.c b/drivers/misc/tifm_core.c
index a511b2a713b3..46385f828a8f 100644
--- a/drivers/misc/tifm_core.c
+++ b/drivers/misc/tifm_core.c
@@ -198,22 +198,15 @@ int tifm_add_adapter(struct tifm_adapter *fm)
 {
 	int rc;
 
-	idr_preload(GFP_KERNEL);
-	spin_lock(&tifm_adapter_lock);
-	rc = idr_alloc(&tifm_adapter_idr, fm, 0, 0, GFP_NOWAIT);
-	if (rc >= 0)
-		fm->id = rc;
-	spin_unlock(&tifm_adapter_lock);
-	idr_preload_end();
+	rc = idr_get_index(&tifm_adapter_idr, &tifm_adapter_lock, fm);
 	if (rc < 0)
 		return rc;
+	fm->id = rc;
 
 	dev_set_name(&fm->dev, "tifm%u", fm->id);
 	rc = device_add(&fm->dev);
 	if (rc) {
-		spin_lock(&tifm_adapter_lock);
-		idr_remove(&tifm_adapter_idr, fm->id);
-		spin_unlock(&tifm_adapter_lock);
+		idr_put_index(&tifm_adapter_idr, &tifm_adapter_lock, fm->id);
 	}
 
 	return rc;
@@ -230,9 +223,7 @@ void tifm_remove_adapter(struct tifm_adapter *fm)
 			device_unregister(&fm->sockets[cnt]->dev);
 	}
 
-	spin_lock(&tifm_adapter_lock);
-	idr_remove(&tifm_adapter_idr, fm->id);
-	spin_unlock(&tifm_adapter_lock);
+	idr_put_index(&tifm_adapter_idr, &tifm_adapter_lock, fm->id);
 	device_del(&fm->dev);
 }
 EXPORT_SYMBOL(tifm_remove_adapter);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225443 — [PATCH 02/17] Update scsi hosts to use idr for host number mgmt

Each Scsi_Host instance gets a host number starting
at 0, but this was implemented with an atomic integer,
and rollover wasn't considered. Another problem with
this design is that scsi host numbers used by iscsi
are never reused, thereby making rollover more likely.
This patch converts Scsi_Host instances to use idr
to manage their instance numbers and to simplify
instance number to pointer lookups.

This also means that host instance numbers will be
reused, when available.

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/hosts.c | 59 ++++++++++++++++++++++++----------------------------
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 8bb173e01084..61201bc03b98 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -33,7 +33,7 @@
 #include <linux/transport_class.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
-
+#include <linux/idr.h>
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_transport.h>
@@ -42,8 +42,6 @@
 #include "scsi_logging.h"
 
 
-static atomic_t scsi_host_next_hn = ATOMIC_INIT(0);	/* host_no for next new host */
-
 
 static void scsi_host_cls_release(struct device *dev)
 {
@@ -304,6 +302,19 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 }
 EXPORT_SYMBOL(scsi_add_host_with_dma);
 
+static DEFINE_SPINLOCK(host_index_lock);
+static DEFINE_IDR(host_index_idr);
+
+static inline int host_get_index(void *ptr)
+{
+	return idr_get_index(&host_index_idr, &host_index_lock, ptr);
+}
+
+static inline void host_put_index(int index)
+{
+	idr_put_index(&host_index_idr, &host_index_lock, index);
+}
+
 static void scsi_host_dev_release(struct device *dev)
 {
 	struct Scsi_Host *shost = dev_to_shost(dev);
@@ -337,6 +348,8 @@ static void scsi_host_dev_release(struct device *dev)
 
 	kfree(shost->shost_data);
 
+	host_put_index(shost->host_no);
+
 	if (parent)
 		put_device(parent);
 	kfree(shost);
@@ -370,6 +383,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 {
 	struct Scsi_Host *shost;
 	gfp_t gfp_mask = GFP_KERNEL;
+	int index;
 
 	if (sht->unchecked_isa_dma && privsize)
 		gfp_mask |= __GFP_DMA;
@@ -388,11 +402,11 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	init_waitqueue_head(&shost->host_wait);
 	mutex_init(&shost->scan_mutex);
 
-	/*
-	 * subtract one because we increment first then return, but we need to
-	 * know what the next host number was before increment
-	 */
-	shost->host_no = atomic_inc_return(&scsi_host_next_hn) - 1;
+	index = host_get_index(shost);
+	if (index < 0)
+		goto fail_kfree;
+	shost->host_no = index;
+
 	shost->dma_channel = 0xff;
 
 	/* These three are default values which can be overridden */
@@ -477,7 +491,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 		shost_printk(KERN_WARNING, shost,
 			"error handler thread failed to spawn, error = %ld\n",
 			PTR_ERR(shost->ehandler));
-		goto fail_kfree;
+		goto fail_idr_remove;
 	}
 
 	shost->tmf_work_q = alloc_workqueue("scsi_tmf_%d",
@@ -493,6 +507,8 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 
  fail_kthread:
 	kthread_stop(shost->ehandler);
+ fail_idr_remove:
+	host_put_index(shost->host_no);
  fail_kfree:
 	kfree(shost);
 	return NULL;
@@ -522,38 +538,16 @@ void scsi_unregister(struct Scsi_Host *shost)
 }
 EXPORT_SYMBOL(scsi_unregister);
 
-static int __scsi_host_match(struct device *dev, const void *data)
-{
-	struct Scsi_Host *p;
-	const unsigned short *hostnum = data;
-
-	p = class_to_shost(dev);
-	return p->host_no == *hostnum;
-}
-
 /**
  * scsi_host_lookup - get a reference to a Scsi_Host by host no
  * @hostnum:	host number to locate
  *
  * Return value:
  *	A pointer to located Scsi_Host or NULL.
- *
- *	The caller must do a scsi_host_put() to drop the reference
- *	that scsi_host_get() took. The put_device() below dropped
- *	the reference from class_find_device().
  **/
 struct Scsi_Host *scsi_host_lookup(unsigned short hostnum)
 {
-	struct device *cdev;
-	struct Scsi_Host *shost = NULL;
-
-	cdev = class_find_device(&shost_class, NULL, &hostnum,
-				 __scsi_host_match);
-	if (cdev) {
-		shost = scsi_host_get(class_to_shost(cdev));
-		put_device(cdev);
-	}
-	return shost;
+	return idr_find(&host_index_idr, hostnum);
 }
 EXPORT_SYMBOL(scsi_host_lookup);
 
@@ -588,6 +582,7 @@ int scsi_init_hosts(void)
 void scsi_exit_hosts(void)
 {
 	class_unregister(&shost_class);
+	idr_destroy(&host_index_idr);
 }
 
 int scsi_is_host_device(const struct device *dev)
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225445 — [PATCH 01/17] Add ida and idr helper routines.

Clients of the ida and idr index-management routines
tend to use the same calling sequences much of the time,
so this change adds helper functions for allocating and
releasing indexes of either flavor, i.e. with or
without pointer management.

Inline functions added for idr:
  idr_get_index_in_range
  idr_get_index (in range 0,0)
  idr_put_index
And for ida:
  ida_get_index
  ida_put_index

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 include/linux/idr.h | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/include/linux/idr.h b/include/linux/idr.h
index 013fd9bc4cb6..341c4f2d9874 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -16,6 +16,8 @@
 #include <linux/bitops.h>
 #include <linux/init.h>
 #include <linux/rcupdate.h>
+#include <linux/spinlock.h>
+#include <linux/gfp.h>
 
 /*
  * We want shallower trees and thus more bits covered at each layer.  8
@@ -183,4 +185,104 @@ static inline int ida_get_new(struct ida *ida, int *p_id)
 
 void __init idr_init_cache(void);
 
+/**
+ * ida_get_index - allocate a ida index value
+ * @ida		idr handle
+ * @lock	spinlock handle protecting this index
+ * @p_id	pointer to allocated index value
+ *
+ * A helper function for safely allocating an index value (id),
+ * returning a negative errno value on failure, else 0.
+ */
+static inline int ida_get_index(struct ida *ida, spinlock_t *lock, int *p_id)
+{
+	int error = -ENOMEM;
+
+	do {
+		if (!ida_pre_get(ida, GFP_KERNEL))
+			break;
+		spin_lock(lock);
+		error = ida_get_new(ida, p_id);
+		spin_unlock(lock);
+	} while (error == -EAGAIN);
+
+	return error;
+}
+
+/**
+ * ida_put_index - free an allocated ida index value
+ * @ida		idr handle
+ * @lock	spinlock handle protecting this index
+ * @id		the value of the allocated index
+ *
+ * A helper function that goes with @ida_get_index, which safely
+ * frees a previously-allocated index value.
+ */
+static inline void ida_put_index(struct ida *ida, spinlock_t *lock, int id)
+{
+	spin_lock(lock);
+	ida_remove(ida, id);
+	spin_unlock(lock);
+}
+
+/**
+ * idr_get_index_in_range - allocate a new index, with locking
+ *		            within a range
+ * @idr:	idr handle
+ * @lock:	spin lock handle protecting the index
+ * @ptr:	pointer to associate with allocated index
+ * @start:	starting index (see idr_alloc)
+ * @end:	ending index (0 -> use default max)
+ *
+ * This is a helper routine meant to make the common
+ * calling sequence to allocate an idr index easier.
+ * It uses a spin lock, and allocates a positive index
+ * in the range of [start,end), returning a negative
+ * errno on failure, else 0.
+ */
+static inline int idr_get_index_in_range(struct idr *idr, spinlock_t *lock,
+					 void *ptr, int start, int end)
+{
+	int ret;
+
+	idr_preload(GFP_KERNEL);
+	spin_lock(lock);
+	ret = idr_alloc(idr, ptr, start, end, GFP_NOWAIT);
+	spin_unlock(lock);
+	idr_preload_end();
+
+	return ret;
+}
+
+/**
+ * idr_get_index - allocate new index, with locking, using the
+ *		   default range (zero to max-1)
+ * @idr:	idr handle
+ * @lock:	spin lock handle protecting the index
+ * @ptr:	pointer to associate with allocated index
+ *
+ * Simple wrapper around idr_get_index_in_range() w/ @start and
+ * @end of 0, since this is a common case
+ */
+static inline int idr_get_index(struct idr *idr, spinlock_t *lock, void *ptr)
+{
+	return idr_get_index_in_range(idr, lock, ptr, 0, 0);
+}
+
+/**
+ * idr_put_index - free an allocated idr index value
+ * @idr		idr handle
+ * @lock	spinlock handle protecting this index
+ * @id		the value of the allocated index
+ *
+ * A helper function that goes with @idr_get_index, which safely
+ * frees a previously-allocated index value.
+ */
+static inline void idr_put_index(struct idr *idr, spinlock_t *lock, int id)
+{
+	spin_lock(lock);
+	idr_remove(idr, id);
+	spin_unlock(lock);
+}
+
 #endif /* __IDR_H__ */
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225491 — Re: [PATCH 01/17] Add ida and idr helper routines.

On Tue, 2015-09-15 at 09:46 -0700, Lee Duncan wrote:
> Clients of the ida and idr index-management routines
> tend to use the same calling sequences much of the time,
> so this change adds helper functions for allocating and
> releasing indexes of either flavor, i.e. with or
> without pointer management.
> 
> Inline functions added for idr:
>   idr_get_index_in_range
>   idr_get_index (in range 0,0)
>   idr_put_index
> And for ida:
>   ida_get_index
>   ida_put_index

Every consumer of this I've seen seems to have the pattern of allocating
the ida and the protecting spinlock together.  If that's the case, why
not move the spinlock into struct ida so it doesn't have to be
separately allocated and passed in to all the helpers?

Also, you need a cc of Tejun (added on this one) because he's the one
who last did significant work in ida/idr.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225498 — Re: [PATCH 01/17] Add ida and idr helper routines.

Hello,

On Tue, Sep 15, 2015 at 09:46:01AM -0700, Lee Duncan wrote:
> +/**
> + * ida_get_index - allocate a ida index value
> + * @ida		idr handle
> + * @lock	spinlock handle protecting this index
> + * @p_id	pointer to allocated index value
> + *
> + * A helper function for safely allocating an index value (id),
> + * returning a negative errno value on failure, else 0.
> + */
> +static inline int ida_get_index(struct ida *ida, spinlock_t *lock, int *p_id)
> +{
> +	int error = -ENOMEM;
> +
> +	do {
> +		if (!ida_pre_get(ida, GFP_KERNEL))
> +			break;
> +		spin_lock(lock);
> +		error = ida_get_new(ida, p_id);
> +		spin_unlock(lock);
> +	} while (error == -EAGAIN);
> +
> +	return error;
> +}

Obviously ida allocation doesn't need to be synchronized against
anything else.  Why not just use ida_simple_get/remove()?

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225501 — Re: [PATCH 01/17] Add ida and idr helper routines.

On Tue, 2015-09-15 at 14:27 -0400, Tejun Heo wrote:
> Hello,
> 
> On Tue, Sep 15, 2015 at 09:46:01AM -0700, Lee Duncan wrote:
> > +/**
> > + * ida_get_index - allocate a ida index value
> > + * @ida		idr handle
> > + * @lock	spinlock handle protecting this index
> > + * @p_id	pointer to allocated index value
> > + *
> > + * A helper function for safely allocating an index value (id),
> > + * returning a negative errno value on failure, else 0.
> > + */
> > +static inline int ida_get_index(struct ida *ida, spinlock_t *lock, int *p_id)
> > +{
> > +	int error = -ENOMEM;
> > +
> > +	do {
> > +		if (!ida_pre_get(ida, GFP_KERNEL))
> > +			break;
> > +		spin_lock(lock);
> > +		error = ida_get_new(ida, p_id);
> > +		spin_unlock(lock);
> > +	} while (error == -EAGAIN);
> > +
> > +	return error;
> > +}
> 
> Obviously ida allocation doesn't need to be synchronized against
> anything else.  Why not just use ida_simple_get/remove()?

For most of the SCSI stuff, yes.  I'm less sure about the sd numbers.
They go up very high and get hammered a lot during system bring up and
hot plug.  I think having their own lock rather than wrapping everything
around simple_ida_lock makes more sense here just because the system is
heavily contended on getting indexes at bring up.

To continue the thought, why not move simple_ida_lock into struct ida so
we don't have to worry about the contention and can sue ida_simple_...
everywhere?

James



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225503 — Re: [PATCH 01/17] Add ida and idr helper routines.

Hello,

On Tue, Sep 15, 2015 at 11:38:42AM -0700, James Bottomley wrote:
> For most of the SCSI stuff, yes.  I'm less sure about the sd numbers.
> They go up very high and get hammered a lot during system bring up and
> hot plug.  I think having their own lock rather than wrapping everything
> around simple_ida_lock makes more sense here just because the system is
> heavily contended on getting indexes at bring up.
> 
> To continue the thought, why not move simple_ida_lock into struct ida so
> we don't have to worry about the contention and can sue ida_simple_...
> everywhere?

We sure can do that if necessary but I'm rather doubtful that even
with sd number hammering this is likely to be a problem.  Let's
convert the users to the simple interface and make the lock per-ida if
we actually see contention on the lock.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1228084 — Re: [PATCH 01/17] Add ida and idr helper routines.

Hello,

On Fri, Sep 18, 2015 at 08:42:26AM -0700, Lee Duncan wrote:
> To be clear: you would like a patch series that converts the users of
> the ida_* routines in my patches to instead use the ida_simple_*
> routines, correct? And of course the ida_* helper routines I was adding
> in idr.h would not be needed.

Yeap.

> If this is correct, I will supply a version 2 patch series that
> addresses this issue as well as the two patch-naming issues that were
> raised.

Sounds good to me.  If the shared lock blows up, we can make that
per-ida.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1228088 — Re: [PATCH 01/17] Add ida and idr helper routines.

On 09/15/2015 11:41 AM, Tejun Heo wrote:
> Hello,
> 
> On Tue, Sep 15, 2015 at 11:38:42AM -0700, James Bottomley wrote:
>> For most of the SCSI stuff, yes.  I'm less sure about the sd numbers.
>> They go up very high and get hammered a lot during system bring up and
>> hot plug.  I think having their own lock rather than wrapping everything
>> around simple_ida_lock makes more sense here just because the system is
>> heavily contended on getting indexes at bring up.
>>
>> To continue the thought, why not move simple_ida_lock into struct ida so
>> we don't have to worry about the contention and can sue ida_simple_...
>> everywhere?
> 
> We sure can do that if necessary but I'm rather doubtful that even
> with sd number hammering this is likely to be a problem.  Let's
> convert the users to the simple interface and make the lock per-ida if
> we actually see contention on the lock.
> 
> Thanks.
> 

To be clear: you would like a patch series that converts the users of
the ida_* routines in my patches to instead use the ida_simple_*
routines, correct? And of course the ida_* helper routines I was adding
in idr.h would not be needed.

If this is correct, I will supply a version 2 patch series that
addresses this issue as well as the two patch-naming issues that were
raised.
-- 
Lee Duncan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225446 — [PATCH 03/17] Update the st driver to use idr helper functions.

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/st.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index b37b9b00c4b4..51e1ce721d9f 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -4265,11 +4265,8 @@ static int st_probe(struct device *dev)
 	    tpnt->blksize_changed = 0;
 	mutex_init(&tpnt->lock);
 
-	idr_preload(GFP_KERNEL);
-	spin_lock(&st_index_lock);
-	error = idr_alloc(&st_index_idr, tpnt, 0, ST_MAX_TAPES + 1, GFP_NOWAIT);
-	spin_unlock(&st_index_lock);
-	idr_preload_end();
+	error = idr_get_index_in_range(&st_index_idr, &st_index_lock, tpnt,
+				       0, ST_MAX_TAPES + 1);
 	if (error < 0) {
 		pr_warn("st: idr allocation failed: %d\n", error);
 		goto out_put_queue;
@@ -4303,9 +4300,7 @@ out_remove_devs:
 	remove_cdevs(tpnt);
 	kfree(tpnt->stats);
 out_idr_remove:
-	spin_lock(&st_index_lock);
-	idr_remove(&st_index_idr, tpnt->index);
-	spin_unlock(&st_index_lock);
+	idr_put_index(&st_index_idr, &st_index_lock, tpnt->index);
 out_put_queue:
 	blk_put_queue(disk->queue);
 out_put_disk:
@@ -4330,9 +4325,7 @@ static int st_remove(struct device *dev)
 	mutex_lock(&st_ref_mutex);
 	kref_put(&tpnt->kref, scsi_tape_release);
 	mutex_unlock(&st_ref_mutex);
-	spin_lock(&st_index_lock);
-	idr_remove(&st_index_idr, index);
-	spin_unlock(&st_index_lock);
+	idr_put_index(&st_index_idr, &st_index_lock, index);
 	return 0;
 }
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225447 — [PATCH 10/17] Update the DCA DMA driver to use idr helper functions.

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/dca/dca-sysfs.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/dca/dca-sysfs.c b/drivers/dca/dca-sysfs.c
index 126cf295b198..8930707df295 100644
--- a/drivers/dca/dca-sysfs.c
+++ b/drivers/dca/dca-sysfs.c
@@ -55,23 +55,14 @@ int dca_sysfs_add_provider(struct dca_provider *dca, struct device *dev)
 	struct device *cd;
 	int ret;
 
-	idr_preload(GFP_KERNEL);
-	spin_lock(&dca_idr_lock);
-
-	ret = idr_alloc(&dca_idr, dca, 0, 0, GFP_NOWAIT);
-	if (ret >= 0)
-		dca->id = ret;
-
-	spin_unlock(&dca_idr_lock);
-	idr_preload_end();
+	ret = idr_get_index(&dca_idr, &dca_idr_lock, dca);
 	if (ret < 0)
 		return ret;
+	dca->id = ret;
 
 	cd = device_create(dca_class, dev, MKDEV(0, 0), NULL, "dca%d", dca->id);
 	if (IS_ERR(cd)) {
-		spin_lock(&dca_idr_lock);
-		idr_remove(&dca_idr, dca->id);
-		spin_unlock(&dca_idr_lock);
+		idr_put_index(&dca_idr, &dca_idr_lock, dca->id);
 		return PTR_ERR(cd);
 	}
 	dca->cd = cd;
@@ -82,9 +73,7 @@ void dca_sysfs_remove_provider(struct dca_provider *dca)
 {
 	device_unregister(dca->cd);
 	dca->cd = NULL;
-	spin_lock(&dca_idr_lock);
-	idr_remove(&dca_idr, dca->id);
-	spin_unlock(&dca_idr_lock);
+	idr_put_index(&dca_idr, &dca_idr_lock, dca->id);
 }
 
 int __init dca_sysfs_init(void)
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web