[RFC,v7,11/12] smartpqi: enable host tagset

Message ID 1591810159-240929-12-git-send-email-john.garry@huawei.com
State New
Headers show
Series
  • blk-mq/scsi: Provide hostwide shared tags for SCSI HBAs
Related show

Commit Message

John Garry June 10, 2020, 5:29 p.m.
From: Hannes Reinecke <hare@suse.de>


Enable host tagset for smartpqi; with this we can use the request
tag to look command from the pool avoiding the list iteration in
the hot path.

Signed-off-by: Hannes Reinecke <hare@suse.de>

---
 drivers/scsi/smartpqi/smartpqi_init.c | 38 ++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 10 deletions(-)

-- 
2.26.2

Comments

John Garry July 14, 2020, 1:16 p.m. | #1
Hi Hannes,

>   static struct pqi_io_request *pqi_alloc_io_request(

> -	struct pqi_ctrl_info *ctrl_info)

> +	struct pqi_ctrl_info *ctrl_info, struct scsi_cmnd *scmd)

>   {

>   	struct pqi_io_request *io_request;

> +	unsigned int limit = PQI_RESERVED_IO_SLOTS;

>   	u16 i = ctrl_info->next_io_request_slot;	/* benignly racy */

>   

> -	while (1) {

> +	if (scmd) {

> +		u32 blk_tag = blk_mq_unique_tag(scmd->request);

> +

> +		i = blk_mq_unique_tag_to_tag(blk_tag) + limit;

>   		io_request = &ctrl_info->io_request_pool[i];


This looks ok

> -		if (atomic_inc_return(&io_request->refcount) == 1)

> -			break;

> -		atomic_dec(&io_request->refcount);

> -		i = (i + 1) % ctrl_info->max_io_slots;

> +		if (WARN_ON(atomic_inc_return(&io_request->refcount) > 1)) {

> +			atomic_dec(&io_request->refcount);

> +			return NULL;

> +		}

> +	} else {

> +		while (1) {

> +			io_request = &ctrl_info->io_request_pool[i];

> +			if (atomic_inc_return(&io_request->refcount) == 1)

> +				break;

> +			atomic_dec(&io_request->refcount);

> +			i = (i + 1) % limit;


To me, the range we use here looks incorrect. I would assume we should 
restrict range to [max_io_slots - PQI_RESERVED_IO_SLOTS, max_io_slots).

But then your reserved commands support would solve that.

> +		}

>   	}

>   


Thanks,
John
John Garry July 14, 2020, 1:31 p.m. | #2
On 14/07/2020 14:16, John Garry wrote:
> Hi Hannes,

> 

>>   static struct pqi_io_request *pqi_alloc_io_request(

>> -    struct pqi_ctrl_info *ctrl_info)

>> +    struct pqi_ctrl_info *ctrl_info, struct scsi_cmnd *scmd)

>>   {

>>       struct pqi_io_request *io_request;

>> +    unsigned int limit = PQI_RESERVED_IO_SLOTS;

>>       u16 i = ctrl_info->next_io_request_slot;    /* benignly racy */

>> -    while (1) {

>> +    if (scmd) {

>> +        u32 blk_tag = blk_mq_unique_tag(scmd->request);

>> +

>> +        i = blk_mq_unique_tag_to_tag(blk_tag) + limit;

>>           io_request = &ctrl_info->io_request_pool[i];

> 

> This looks ok

> 

>> -        if (atomic_inc_return(&io_request->refcount) == 1)

>> -            break;

>> -        atomic_dec(&io_request->refcount);

>> -        i = (i + 1) % ctrl_info->max_io_slots;

>> +        if (WARN_ON(atomic_inc_return(&io_request->refcount) > 1)) {

>> +            atomic_dec(&io_request->refcount);

>> +            return NULL;

>> +        }

>> +    } else {

>> +        while (1) {

>> +            io_request = &ctrl_info->io_request_pool[i];

>> +            if (atomic_inc_return(&io_request->refcount) == 1)

>> +                break;

>> +            atomic_dec(&io_request->refcount);

>> +            i = (i + 1) % limit;

> 

> To me, the range we use here looks incorrect. I would assume we should 

> restrict range to [max_io_slots - PQI_RESERVED_IO_SLOTS, max_io_slots).

> 


Sorry, I missed that you use i = blk_mq_unique_tag_to_tag(blk_tag) + 
limit for regular command.

But we still set next_io_request_slot for regular command and maybe then 
read and use it for reserved command.

Thanks
Hannes Reinecke July 14, 2020, 2:02 p.m. | #3
On 7/14/20 3:16 PM, John Garry wrote:
> Hi Hannes,

> 

>>   static struct pqi_io_request *pqi_alloc_io_request(

>> -    struct pqi_ctrl_info *ctrl_info)

>> +    struct pqi_ctrl_info *ctrl_info, struct scsi_cmnd *scmd)

>>   {

>>       struct pqi_io_request *io_request;

>> +    unsigned int limit = PQI_RESERVED_IO_SLOTS;

>>       u16 i = ctrl_info->next_io_request_slot;    /* benignly racy */

>>   -    while (1) {

>> +    if (scmd) {

>> +        u32 blk_tag = blk_mq_unique_tag(scmd->request);

>> +

>> +        i = blk_mq_unique_tag_to_tag(blk_tag) + limit;

>>           io_request = &ctrl_info->io_request_pool[i];

> 

> This looks ok

> 

>> -        if (atomic_inc_return(&io_request->refcount) == 1)

>> -            break;

>> -        atomic_dec(&io_request->refcount);

>> -        i = (i + 1) % ctrl_info->max_io_slots;

>> +        if (WARN_ON(atomic_inc_return(&io_request->refcount) > 1)) {

>> +            atomic_dec(&io_request->refcount);

>> +            return NULL;

>> +        }

>> +    } else {

>> +        while (1) {

>> +            io_request = &ctrl_info->io_request_pool[i];

>> +            if (atomic_inc_return(&io_request->refcount) == 1)

>> +                break;

>> +            atomic_dec(&io_request->refcount);

>> +            i = (i + 1) % limit;

> 

> To me, the range we use here looks incorrect. I would assume we should

> restrict range to [max_io_slots - PQI_RESERVED_IO_SLOTS, max_io_slots).

> 

> But then your reserved commands support would solve that.

> 

This branch of the 'if' condition will only be taken for internal
commands, for which we only allow up to PQI_RESERVED_IO_SLOTS.
And we set the 'normal' I/O commands above at an offset, so we're fine here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
Don.Brace@microchip.com July 14, 2020, 6:16 p.m. | #4
Subject: Re: [PATCH RFC v7 11/12] smartpqi: enable host tagset


Both the driver author and myself do not want to change how commands are processed in the smartpqi driver.

Nak.
John Garry July 15, 2020, 7:28 a.m. | #5
On 14/07/2020 19:16, Don.Brace@microchip.com wrote:
> Subject: Re: [PATCH RFC v7 11/12] smartpqi: enable host tagset

> 

> 

> Both the driver author and myself do not want to change how commands are processed in the smartpqi driver.

> 

> Nak.

> 


ok, your call. But it still seems that the driver should set the 
host_tagset flag.

Anyway, can you also let us know your stance on the hpsa change in this 
series?

https://lore.kernel.org/linux-scsi/1591810159-240929-1-git-send-email-john.garry@huawei.com/T/#m1057b8a8c23a9bf558a048817a1cda4a576291b2

thanks

Patch

diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
index cd157f11eb22..1f4de4c2d876 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -575,17 +575,29 @@  static inline void pqi_reinit_io_request(struct pqi_io_request *io_request)
 }
 
 static struct pqi_io_request *pqi_alloc_io_request(
-	struct pqi_ctrl_info *ctrl_info)
+	struct pqi_ctrl_info *ctrl_info, struct scsi_cmnd *scmd)
 {
 	struct pqi_io_request *io_request;
+	unsigned int limit = PQI_RESERVED_IO_SLOTS;
 	u16 i = ctrl_info->next_io_request_slot;	/* benignly racy */
 
-	while (1) {
+	if (scmd) {
+		u32 blk_tag = blk_mq_unique_tag(scmd->request);
+
+		i = blk_mq_unique_tag_to_tag(blk_tag) + limit;
 		io_request = &ctrl_info->io_request_pool[i];
-		if (atomic_inc_return(&io_request->refcount) == 1)
-			break;
-		atomic_dec(&io_request->refcount);
-		i = (i + 1) % ctrl_info->max_io_slots;
+		if (WARN_ON(atomic_inc_return(&io_request->refcount) > 1)) {
+			atomic_dec(&io_request->refcount);
+			return NULL;
+		}
+	} else {
+		while (1) {
+			io_request = &ctrl_info->io_request_pool[i];
+			if (atomic_inc_return(&io_request->refcount) == 1)
+				break;
+			atomic_dec(&io_request->refcount);
+			i = (i + 1) % limit;
+		}
 	}
 
 	/* benignly racy */
@@ -4075,7 +4087,7 @@  static int pqi_submit_raid_request_synchronous(struct pqi_ctrl_info *ctrl_info,
 
 	atomic_inc(&ctrl_info->sync_cmds_outstanding);
 
-	io_request = pqi_alloc_io_request(ctrl_info);
+	io_request = pqi_alloc_io_request(ctrl_info, NULL);
 
 	put_unaligned_le16(io_request->index,
 		&(((struct pqi_raid_path_request *)request)->request_id));
@@ -5032,7 +5044,9 @@  static inline int pqi_raid_submit_scsi_cmd(struct pqi_ctrl_info *ctrl_info,
 {
 	struct pqi_io_request *io_request;
 
-	io_request = pqi_alloc_io_request(ctrl_info);
+	io_request = pqi_alloc_io_request(ctrl_info, scmd);
+	if (!io_request)
+		return SCSI_MLQUEUE_HOST_BUSY;
 
 	return pqi_raid_submit_scsi_cmd_with_io_request(ctrl_info, io_request,
 		device, scmd, queue_group);
@@ -5230,7 +5244,10 @@  static int pqi_aio_submit_io(struct pqi_ctrl_info *ctrl_info,
 	struct pqi_io_request *io_request;
 	struct pqi_aio_path_request *request;
 
-	io_request = pqi_alloc_io_request(ctrl_info);
+	io_request = pqi_alloc_io_request(ctrl_info, scmd);
+	if (!io_request)
+		return SCSI_MLQUEUE_HOST_BUSY;
+
 	io_request->io_complete_callback = pqi_aio_io_complete;
 	io_request->scmd = scmd;
 	io_request->raid_bypass = raid_bypass;
@@ -5657,7 +5674,7 @@  static int pqi_lun_reset(struct pqi_ctrl_info *ctrl_info,
 	DECLARE_COMPLETION_ONSTACK(wait);
 	struct pqi_task_management_request *request;
 
-	io_request = pqi_alloc_io_request(ctrl_info);
+	io_request = pqi_alloc_io_request(ctrl_info, NULL);
 	io_request->io_complete_callback = pqi_lun_reset_complete;
 	io_request->context = &wait;
 
@@ -6504,6 +6521,7 @@  static struct scsi_host_template pqi_driver_template = {
 	.map_queues = pqi_map_queues,
 	.sdev_attrs = pqi_sdev_attrs,
 	.shost_attrs = pqi_shost_attrs,
+	.host_tagset = 1,
 };
 
 static int pqi_register_scsi(struct pqi_ctrl_info *ctrl_info)