diff mbox series

[04/24] scsi: core: Implement reserved command handling

Message ID 20250403211937.2225615-5-bvanassche@acm.org
State New
Headers show
Series Optimize the hot path in the UFS driver | expand

Commit Message

Bart Van Assche April 3, 2025, 9:17 p.m. UTC 
From: Hannes Reinecke <hare@suse.de>

Quite some drivers are using management commands internally. These commands
typically use the same tag pool as regular SCSI commands. Tags for these
management commands are set aside before allocating the block-mq tag bitmap
for regular SCSI commands. The block layer already supports this via the
reserved tag mechanism. Add a new field 'nr_reserved_cmds' to the SCSI host
template to instruct the block layer to set aside a tag space for these
management commands by using reserved tags. Exclude reserved commands from
.can_queue because .can_queue is visible in sysfs.

Signed-off-by: Hannes Reinecke <hare@suse.de>
[ bvanassche: modified patch description ]
Cc: John Garry <john.g.garry@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/hosts.c     |  3 +++
 drivers/scsi/scsi_lib.c  |  4 +++-
 include/scsi/scsi_host.h | 22 +++++++++++++++++++++-
 3 files changed, 27 insertions(+), 2 deletions(-)

Comments

John Garry April 4, 2025, 10:49 a.m. UTC | #1 
On 03/04/2025 22:17, Bart Van Assche wrote:
> From: Hannes Reinecke<hare@suse.de>
> 
> Quite some drivers are using management commands internally. These commands
> typically use the same tag pool as regular SCSI commands. Tags for these
> management commands are set aside before allocating the block-mq tag bitmap
> for regular SCSI commands. The block layer already supports this via the
> reserved tag mechanism. Add a new field 'nr_reserved_cmds' to the SCSI host
> template to instruct the block layer to set aside a tag space for these
> management commands by using reserved tags. Exclude reserved commands from
> .can_queue because .can_queue is visible in sysfs.
> 
> Signed-off-by: Hannes Reinecke<hare@suse.de>
> [ bvanassche: modified patch description ]
> Cc: John Garry<john.g.garry@oracle.com>
> Signed-off-by: Bart Van Assche<bvanassche@acm.org>

How is this related to the rest of the series?

To allocate a reserved-tag request we need to use BLK_MQ_REQ_RESERVED, 
right? I don't see that used...
Bart Van Assche April 4, 2025, 4:34 p.m. UTC | #2 
On 4/4/25 3:49 AM, John Garry wrote:
> On 03/04/2025 22:17, Bart Van Assche wrote:
>> From: Hannes Reinecke<hare@suse.de>
>>
>> Quite some drivers are using management commands internally. These 
>> commands
>> typically use the same tag pool as regular SCSI commands. Tags for these
>> management commands are set aside before allocating the block-mq tag 
>> bitmap
>> for regular SCSI commands. The block layer already supports this via the
>> reserved tag mechanism. Add a new field 'nr_reserved_cmds' to the SCSI 
>> host
>> template to instruct the block layer to set aside a tag space for these
>> management commands by using reserved tags. Exclude reserved commands 
>> from
>> .can_queue because .can_queue is visible in sysfs.
>>
>> Signed-off-by: Hannes Reinecke<hare@suse.de>
>> [ bvanassche: modified patch description ]
>> Cc: John Garry<john.g.garry@oracle.com>
>> Signed-off-by: Bart Van Assche<bvanassche@acm.org>
> 
> How is this related to the rest of the series?
> 
> To allocate a reserved-tag request we need to use BLK_MQ_REQ_RESERVED, 
> right? I don't see that used...

Hi John,

Calling blk_mq_alloc_request(..., BLK_MQ_REQ_RESERVED) is not the only 
way to allocated a reserved tag. Another possibility is to let the
driver manage reserved tags. The UFS driver only needs a single reserved
tag and already serializes use of that tag. See also the following
change in patch 21/24:

-	hba->reserved_slot = hba->nutrs - 1;
+	hba->reserved_slot = 0;

Use of hba->reserved_slot is serialized by calling ufshcd_dev_man_lock()
and ufshcd_dev_man_unlock(). More code is serialized by these calls than
only the region in which hba->reserved_slot is used so I don't think
that the UFS driver code would become shorter by using block layer
functions for allocating / freeing reserved tags.

Thanks,

Bart.
John Garry April 4, 2025, 5:34 p.m. UTC | #3 
On 04/04/2025 17:34, Bart Van Assche wrote:
>>> Quite some drivers are using management commands internally. These 
>>> commands
>>> typically use the same tag pool as regular SCSI commands. Tags for these
>>> management commands are set aside before allocating the block-mq tag 
>>> bitmap
>>> for regular SCSI commands. The block layer already supports this via the
>>> reserved tag mechanism. Add a new field 'nr_reserved_cmds' to the 
>>> SCSI host
>>> template to instruct the block layer to set aside a tag space for these
>>> management commands by using reserved tags. Exclude reserved commands 
>>> from
>>> .can_queue because .can_queue is visible in sysfs.
>>>
>>> Signed-off-by: Hannes Reinecke<hare@suse.de>
>>> [ bvanassche: modified patch description ]
>>> Cc: John Garry<john.g.garry@oracle.com>
>>> Signed-off-by: Bart Van Assche<bvanassche@acm.org>
>>
>> How is this related to the rest of the series?
>>
>> To allocate a reserved-tag request we need to use BLK_MQ_REQ_RESERVED, 
>> right? I don't see that used...
> 
> Hi John,
> 
> Calling blk_mq_alloc_request(..., BLK_MQ_REQ_RESERVED) is not the only 
> way to allocated a reserved tag. Another possibility is to let the
> driver manage reserved tags. The UFS driver only needs a single reserved
> tag and already serializes use of that tag. See also the following
> change in patch 21/24:
> 
> -    hba->reserved_slot = hba->nutrs - 1;
> +    hba->reserved_slot = 0;
> 
> Use of hba->reserved_slot is serialized by calling ufshcd_dev_man_lock()
> and ufshcd_dev_man_unlock(). More code is serialized by these calls than
> only the region in which hba->reserved_slot is used so I don't think
> that the UFS driver code would become shorter by using block layer
> functions for allocating / freeing reserved tags.

Now I see this in 23/24:

+/*
+ * Convert a block layer tag into a SCSI command pointer. This function is
+ * called once per I/O completion path and is also called from error paths.
+ */
+static inline struct scsi_cmnd *ufshcd_tag_to_cmd(struct ufs_hba *hba, 
u32 tag)
+{
+	struct blk_mq_tags *tags = hba->host->tag_set.tags[0];
+	struct request *rq;
+
+	/*
+	 * Use .static_rqs[] for reserved commands because blk_mq_get_tag()
+	 * is not called for reserved commands by the UFS driver.
+	 */
+	rq = tag < UFSHCD_NUM_RESERVED ? tags->static_rqs[tag] :
+					 blk_mq_tag_to_rq(tags, tag);
+
+	if (WARN_ON_ONCE(!rq))
+		return NULL;
+
+	return blk_mq_rq_to_pdu(rq);
+}
+

Do you really think that it is ok that anything outside the block layer 
should be referencing tags->static_rqs[] directly?

Even using blk_mq_alloc_request() would seem better than that.
diff mbox series

Patch

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index e021f1106bea..c2c6c96781e3 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -495,6 +495,9 @@  struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, int priv
 	if (sht->virt_boundary_mask)
 		shost->virt_boundary_mask = sht->virt_boundary_mask;
 
+	if (sht->nr_reserved_cmds)
+		shost->nr_reserved_cmds = sht->nr_reserved_cmds;
+
 	device_initialize(&shost->shost_gendev);
 	dev_set_name(&shost->shost_gendev, "host%d", shost->host_no);
 	shost->shost_gendev.bus = &scsi_bus_type;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 1da26f300287..94dafa5ceaaa 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2083,7 +2083,9 @@  int scsi_mq_setup_tags(struct Scsi_Host *shost)
 		tag_set->ops = &scsi_mq_ops_no_commit;
 	tag_set->nr_hw_queues = shost->nr_hw_queues ? : 1;
 	tag_set->nr_maps = shost->nr_maps ? : 1;
-	tag_set->queue_depth = shost->can_queue;
+	tag_set->queue_depth =
+		shost->can_queue + shost->nr_reserved_cmds;
+	tag_set->reserved_tags = shost->nr_reserved_cmds;
 	tag_set->cmd_size = cmd_size;
 	tag_set->numa_node = dev_to_node(shost->dma_dev);
 	if (shost->hostt->tag_alloc_policy_rr)
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 26bc23419cfd..2c0f5ec1046e 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -375,10 +375,19 @@  struct scsi_host_template {
 	/*
 	 * This determines if we will use a non-interrupt driven
 	 * or an interrupt driven scheme.  It is set to the maximum number
-	 * of simultaneous commands a single hw queue in HBA will accept.
+	 * of simultaneous commands a single hw queue in HBA will accept
+	 * excluding internal commands.
 	 */
 	int can_queue;
 
+	/*
+	 * This determines how many commands the HBA will set aside
+	 * for internal commands. This number will be added to
+	 * @can_queue to calcumate the maximum number of simultaneous
+	 * commands sent to the host.
+	 */
+	int nr_reserved_cmds;
+
 	/*
 	 * In many instances, especially where disconnect / reconnect are
 	 * supported, our host also has an ID on the SCSI bus.  If this is
@@ -611,6 +620,11 @@  struct Scsi_Host {
 	unsigned short max_cmd_len;
 
 	int this_id;
+
+	/*
+	 * Number of commands this host can handle at the same time.
+	 * This excludes reserved commands as specified by nr_reserved_cmds.
+	 */
 	int can_queue;
 	short cmd_per_lun;
 	short unsigned int sg_tablesize;
@@ -631,6 +645,12 @@  struct Scsi_Host {
 	 */
 	unsigned nr_hw_queues;
 	unsigned nr_maps;
+
+	/*
+	 * Number of reserved commands to allocate, if any.
+	 */
+	unsigned nr_reserved_cmds;
+
 	unsigned active_mode:2;
 
 	/*