The following suggestions are based on trying to _run_ this code ...
foolish, I know.
On 2020-05-28 12:36 p.m., Hannes Reinecke wrote:
Use an xarray instead of lists for holding the scsi targets.
I've also shortened the 'channel' and 'id' values to 16 bit
as none of the drivers requires a full 32bit range for either
of them, and by shortening them we can use them as the index
into the xarray for storing the scsi_target pointer.
Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
---
drivers/scsi/hosts.c | 2 +-
drivers/scsi/scsi.c | 32 ++++++++++++++++++++------------
drivers/scsi/scsi_scan.c | 43 ++++++++++++++++---------------------------
drivers/scsi/scsi_sysfs.c | 15 +++++++++++----
include/scsi/scsi_device.h | 4 ++--
include/scsi/scsi_host.h | 2 +-
6 files changed, 51 insertions(+), 47 deletions(-)
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 7ec91c3a66ca..7109afad0183 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -383,7 +383,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
spin_lock_init(shost->host_lock);
shost->shost_state = SHOST_CREATED;
INIT_LIST_HEAD(&shost->__devices);
- INIT_LIST_HEAD(&shost->__targets);
+ xa_init(&shost->__targets);
Please use: xa_init_flags(&shost->__targets, XA_FLAGS_LOCK_IRQ);
If you ever touch (modify?) that array in interrupt context then I
believe you need this. See the first 50 or so lines in lib/xarray.c
to see that it is used for xarray internals (e.g. xas_lock_type() ).
The xarray interface (include/linux/xarray.h) also has *_irq() and
*_bh() variants (e.g. xa_insert(), xa_insert_bh(), xa_insert_irq() )
but setting that flag in the xa_init_flags() call seems to be a
safer approach. IOWs it is an alternative to using the *_irq()
variants on all the _modifying_ xarray calls.
INIT_LIST_HEAD(&shost->eh_cmd_q);
INIT_LIST_HEAD(&shost->starved_list);
init_waitqueue_head(&shost->host_wait);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 56c24a73e0c7..d601424e32b2 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -575,6 +575,19 @@ struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
}
EXPORT_SYMBOL(__scsi_iterate_devices);
+/**
+ * __scsi_target_lookup - find a target based on channel and target id
+ * @shost: SCSI host pointer
+ * @channel: channel number of the target
+ * @id: ID of the target
+ *
+ */
+static struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost,
+ u16 channel, u16 id)
+{
+ return xa_load(&shost->__targets, (channel << 16) | id);
+}
+
/**
* starget_for_each_device - helper to walk all devices of a target
* @starget: target whose devices we want to iterate over.
@@ -701,19 +714,14 @@ EXPORT_SYMBOL(scsi_device_lookup_by_target);
* really want to use scsi_device_lookup instead.
**/
struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost,
- uint channel, uint id, u64 lun)
+ u16 channel, u16 id, u64 lun)
{
- struct scsi_device *sdev;
+ struct scsi_target *starget;
- list_for_each_entry(sdev, &shost->__devices, siblings) {
- if (sdev->sdev_state == SDEV_DEL)
- continue;
- if (sdev->channel == channel && sdev->id == id &&
- sdev->lun ==lun)
- return sdev;
- }
-
- return NULL;
+ starget = __scsi_target_lookup(shost, channel, id);
+ if (!starget)
+ return NULL;
+ return __scsi_device_lookup_by_target(starget, lun);
}
EXPORT_SYMBOL(__scsi_device_lookup);
@@ -729,7 +737,7 @@ EXPORT_SYMBOL(__scsi_device_lookup);
* needs to be released with scsi_device_put once you're done with it.
**/
struct scsi_device *scsi_device_lookup(struct Scsi_Host *shost,
- uint channel, uint id, u64 lun)
+ u16 channel, u16 id, u64 lun)
{
struct scsi_device *sdev;
unsigned long flags;
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f2437a7570ce..dc2656df495b 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -304,11 +304,15 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
return NULL;
}
+#define scsi_target_index(s) \
+ ((((unsigned long)(s)->channel) << 16) | (s)->id)
+
static void scsi_target_destroy(struct scsi_target *starget)
{
struct device *dev = &starget->dev;
struct Scsi_Host *shost = dev_to_shost(dev->parent);
unsigned long flags;
+ unsigned long tid = scsi_target_index(starget);
BUG_ON(starget->state == STARGET_DEL);
starget->state = STARGET_DEL;
@@ -316,7 +320,7 @@ static void scsi_target_destroy(struct scsi_target *starget)
spin_lock_irqsave(shost->host_lock, flags);
if (shost->hostt->target_destroy)
shost->hostt->target_destroy(starget);
- list_del_init(&starget->siblings);
+ xa_erase(&shost->__targets, tid);
spin_unlock_irqrestore(shost->host_lock, flags);
put_device(dev);
}
@@ -341,27 +345,6 @@ int scsi_is_target_device(const struct device *dev)
}
EXPORT_SYMBOL(scsi_is_target_device);
-static struct scsi_target *__scsi_find_target(struct device *parent,
- int channel, uint id)
-{
- struct scsi_target *starget, *found_starget = NULL;
- struct Scsi_Host *shost = dev_to_shost(parent);
- /*
- * Search for an existing target for this sdev.
- */
- list_for_each_entry(starget, &shost->__targets, siblings) {
- if (starget->id == id &&
- starget->channel == channel) {
- found_starget = starget;
- break;
- }
- }
- if (found_starget)
- get_device(&found_starget->dev);
-
- return found_starget;
-}
-
/**
* scsi_target_reap_ref_release - remove target from visibility
* @kref: the reap_ref in the target being released
@@ -417,6 +400,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
struct scsi_target *starget;
struct scsi_target *found_target;
int error, ref_got;
+ unsigned long tid;
starget = kzalloc(size, GFP_KERNEL);
if (!starget) {
@@ -433,19 +417,24 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
starget->id = id;
starget->channel = channel;
starget->can_queue = 0;
- INIT_LIST_HEAD(&starget->siblings);
INIT_LIST_HEAD(&starget->devices);
starget->state = STARGET_CREATED;
starget->scsi_level = SCSI_2;
starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
+ tid = scsi_target_index(starget);
retry:
spin_lock_irqsave(shost->host_lock, flags);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Interrupts now masked on this CPU.
- found_target = __scsi_find_target(parent, channel, id);
- if (found_target)
+ found_target = xa_load(&shost->__targets, tid);
+ if (found_target) {
+ get_device(&found_target->dev);
goto found;
-
- list_add_tail(&starget->siblings, &shost->__targets);
+ }
+ if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {
You have interrupts masked (on this CPU), you can't use GFP_KERNEL.
If you run this code, you will be told :-)
In this case, you must use GFP_ATOMIC.
+ dev_printk(KERN_ERR, dev, "target index busy\n");
+ kfree(starget);
+ return NULL;
+ }
spin_unlock_irqrestore(shost->host_lock, flags);
^^^^^^^^^^^^^^^^^^^^
Now you can use GFP_KERNEL, if you want.
/* allocate and add */
transport_setup_device(dev);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 163dbcb741c1..95aaa96ce03b 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1512,15 +1512,19 @@ void scsi_remove_target(struct device *dev)
{
struct Scsi_Host *shost = dev_to_shost(dev->parent);
struct scsi_target *starget;
+ unsigned long tid = 0;
unsigned long flags;
-restart:
spin_lock_irqsave(shost->host_lock, flags);
- list_for_each_entry(starget, &shost->__targets, siblings) {
+ starget = xa_find(&shost->__targets, &tid, ULONG_MAX, XA_PRESENT);
+ while (starget) {
if (starget->state == STARGET_DEL ||
starget->state == STARGET_REMOVE ||
- starget->state == STARGET_CREATED_REMOVE)
+ starget->state == STARGET_CREATED_REMOVE) {
+ starget = xa_find_after(&shost->__targets, &tid,
+ ULONG_MAX, XA_PRESENT);
continue;
+ }
if (starget->dev.parent == dev || &starget->dev == dev) {
kref_get(&starget->reap_ref);
if (starget->state == STARGET_CREATED)
@@ -1530,7 +1534,10 @@ void scsi_remove_target(struct device *dev)
spin_unlock_irqrestore(shost->host_lock, flags);
__scsi_remove_target(starget);
scsi_target_reap(starget);
- goto restart;
+ spin_lock_irqsave(shost->host_lock, flags);
+ starget = xa_find_after(&shost->__targets, &tid,
+ ULONG_MAX, XA_PRESENT);
+ continue;
}
}
spin_unlock_irqrestore(shost->host_lock, flags);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index c3cba2aaf934..28034cc0fce5 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -345,9 +345,9 @@ extern struct scsi_device *scsi_device_from_queue(struct request_queue *q);
extern int __must_check scsi_device_get(struct scsi_device *);
extern void scsi_device_put(struct scsi_device *);
extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
- uint, uint, u64);
+ u16, u16, u64);
extern struct scsi_device *__scsi_device_lookup(struct Scsi_Host *,
- uint, uint, u64);
+ u16, u16, u64);
extern struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *,
u64);
extern struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *,
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 822e8cda8d9b..b9395676c75b 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -521,7 +521,7 @@ struct Scsi_Host {
* access this list directly from a driver.
*/
struct list_head __devices;
- struct list_head __targets;
+ struct xarray __targets;
struct list_head starved_list;
