diff mbox series

[1/4] scsi: convert target lookup to xarray

Message ID 20200528163625.110184-2-hare@suse.de (mailing list archive)
State Superseded
Headers show
Series scsi: use xarray for devices and targets | expand

Commit Message

Hannes Reinecke May 28, 2020, 4:36 p.m. UTC
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@suse.de>
---
 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(-)

Comments

Douglas Gilbert May 28, 2020, 5:18 p.m. UTC | #1
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@suse.de>
> ---
>   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;
>   
>
Bart Van Assche May 28, 2020, 5:48 p.m. UTC | #2
On 2020-05-28 09:36, Hannes Reinecke wrote:
> +#define scsi_target_index(s) \
> +	((((unsigned long)(s)->channel) << 16) | (s)->id)

Please define scsi_target_index() as an inline function instead of a macro.

> +	if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {
> +		dev_printk(KERN_ERR, dev, "target index busy\n");
> +		kfree(starget);
> +		return NULL;
> +	}

So the above code passes GFP_KERNEL to xa_insert() while holding a
spinlock? That doesn't seem correct to me. Since xa_insert() provides
locking itself, can the spin_lock_irqsave(shost->host_lock, flags) /
spin_unlock_irqrestore(shost->host_lock, flags) pair be removed and can
the xa_load() and xa_insert() calls be combined into a single
xa_insert() call? I think xa_insert() returns -EBUSY if an entry already
exists.

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

How about using a for loop instead of a while loop such that the
xa_find_after() statement does not have to be duplicated?

Thanks,

Bart.
Douglas Gilbert May 28, 2020, 7:08 p.m. UTC | #3
On 2020-05-28 1:18 p.m., Douglas Gilbert wrote:
> The following suggestions are based on trying to _run_ this code ...
> foolish, I know.

Now with the patches I've suggested it runs. So I ran my modprobe,
rmmod test building gradually out to 6000 odd devices. It ran
for a few minutes then got clobbered by the OOM killer.

Perhaps there are some misplaced xa_destroy()s, I thought.
After grep-ing, "un-placed" would be a better description.

So Hannes, for each xa_init*() call made, there needs to be a
xa_destroy() at the appropriate place and time, perferably before
the OOM killer strikes :-)

Doug Gilbert

> 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@suse.de>
>> ---
>>   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;
>>
>
Hannes Reinecke May 29, 2020, 6:24 a.m. UTC | #4
On 5/28/20 7:48 PM, Bart Van Assche wrote:
> On 2020-05-28 09:36, Hannes Reinecke wrote:
>> +#define scsi_target_index(s) \
>> +	((((unsigned long)(s)->channel) << 16) | (s)->id)
> 
> Please define scsi_target_index() as an inline function instead of a macro.
> 
>> +	if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {
>> +		dev_printk(KERN_ERR, dev, "target index busy\n");
>> +		kfree(starget);
>> +		return NULL;
>> +	}
> 
> So the above code passes GFP_KERNEL to xa_insert() while holding a
> spinlock? That doesn't seem correct to me. Since xa_insert() provides
> locking itself, can the spin_lock_irqsave(shost->host_lock, flags) /
> spin_unlock_irqrestore(shost->host_lock, flags) pair be removed and can
> the xa_load() and xa_insert() calls be combined into a single
> xa_insert() call? I think xa_insert() returns -EBUSY if an entry already
> exists.
> 
>> -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;
>>   		}
>>   	}
> 
> How about using a for loop instead of a while loop such that the
> xa_find_after() statement does not have to be duplicated?
> 
Ok, will do for the next round.

Cheers,

Hannes
diff mbox series

Patch

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);
 	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);
 
-	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)) {
+		dev_printk(KERN_ERR, dev, "target index busy\n");
+		kfree(starget);
+		return NULL;
+	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	/* 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;