Re: [PATCHv3 10/10] mpt3sas: lockless command submission for scsi-mq
On Tue, Feb 21, 2017 at 03:58:08PM +0100, Hannes Reinecke wrote:
> >> + if (ioc->shost->use_blk_mq) {
> >> + u32 unique_tag = blk_mq_unique_tag(scmd->request);
> >> +
> >> + tag = blk_mq_unique_tag_to_tag(unique_tag);
> >> + } else
> >> + tag = scmd->request->tag;
> >
> > All that unique_tag stuff is only required for SCSI LLDDs that
> > set nr_hw_queues > 1, which isn't the ase with this patch.
> >
> Abstraction.
> I would not presume anything about SCSI-MQ/block-mq here; calling
> blk_mq_unique_tag() insulates me against any changes blk-mq might be
> doing for the tag allocation or management.
cmd->request->tag has the same meaning with blk-mq or not, so this
check is obviously wrong.
> >> + ioc->shost->reserved_cmds = INTERNAL_SCSIIO_CMDS_COUNT;
> >
> > Why? You're never calling the block layer to allocate a reserved
> > request.
> >
> The idea was to have a stub command for any ioctl passthrough commands,
> so they'll show up when traversing the list of active commands (hence it
> always uses 'scsiio_depth' and not 'can_queue').
Sure. But setting reserved_cmds has nothing to do with that - it only
sets requests aside so they can be allocated using BLK_MQ_REQ_RESERVED.
If you reserve tags for purely driver internal usage without a struct
request just don't advertise them to the block layer at all. This
is what we do in hpsa, and that's what mpt3sas should do as well.
> >> + if (ioc->shost->use_blk_mq)
> >> + smid = 1;
> >> + else
> >> + smid = ioc->scsiio_depth - ioc->shost->reserved_cmds;
> >
> > Huh? Explanation, please.
> >
> When enabling reserved tags with blk-mq the reserved tag range is at the
> start of the tagspace; for legacy sq there is no reserved range, but
> 'can_queue' is reduced by the number of reserved commands, effectively
> locating the reserved tag range at the end of the tag space.
internal tag number are a blk-mq implementation detail, and drivers
should not second guess it. As said above - just don't use the
reserved_cmds field and set the smids aside inside mpt3sas.
> Figuring out if there are still pending commands on the given target.
starget->target_busy
> Figuring out if there are pending commands on that LUN.
sdev->device_busy
Re: [PATCHv3 10/10] mpt3sas: lockless command submission for scsi-mq
On 02/21/2017 03:34 PM, Christoph Hellwig wrote:
> On Tue, Feb 21, 2017 at 01:27:09PM +0100, Hannes Reinecke wrote:
>> Enable lockless command submission for scsi-mq by moving the
>> command structure into the payload for struct request.
>
> No dependency on scsi-mq, so the changelog could use a little update.
>
>> @@ -2345,26 +2354,22 @@ struct scsiio_tracker *
>> mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx,
>> struct scsi_cmnd *scmd)
>> {
>> +struct scsiio_tracker *request = scsi_cmd_priv(scmd);
>> +unsigned int tag;
>> u16 smid;
>>
>> +if (ioc->shost->use_blk_mq) {
>> +u32 unique_tag = blk_mq_unique_tag(scmd->request);
>> +
>> +tag = blk_mq_unique_tag_to_tag(unique_tag);
>> +} else
>> +tag = scmd->request->tag;
>
> All that unique_tag stuff is only required for SCSI LLDDs that
> set nr_hw_queues > 1, which isn't the ase with this patch.
>
Abstraction.
I would not presume anything about SCSI-MQ/block-mq here; calling
blk_mq_unique_tag() insulates me against any changes blk-mq might be
doing for the tag allocation or management.
>> * @ioc: per adapter object
>> @@ -2423,23 +2444,21 @@ struct scsiio_tracker *
>> unsigned long flags;
>> int i;
>>
>> if (smid < ioc->hi_priority_smid) {
>> +struct scsiio_tracker *st;
>>
>> +st = mpt3sas_get_st_from_smid(ioc, smid);
>> +if (WARN_ON(!st)) {
>> +_base_recovery_check(ioc);
>> +return;
>> +}
>> +mpt3sas_base_clear_st(ioc, st);
>> _base_recovery_check(ioc);
>> return;
>
> This looks fine, but it would be good if could derive the scsiio_tracker
> structure from the scsi_cmnd in as many as possible places, most notably
> all the fast path calls and then just clal mpt3sas_base_clear_st directly.
>
Okay, will be revisiting that.
>> +ioc->shost->reserved_cmds = INTERNAL_SCSIIO_CMDS_COUNT;
>
> Why? You're never calling the block layer to allocate a reserved
> request.
>
The idea was to have a stub command for any ioctl passthrough commands,
so they'll show up when traversing the list of active commands (hence it
always uses 'scsiio_depth' and not 'can_queue').
But it might be that we're not setting the scsiio_tracker for ioctl
passthrough commands; will be checking.
>> +ioc->shost->can_queue = ioc->scsiio_depth - ioc->shost->reserved_cmds;
>
> Just assign ioc->shost->can_queue to ioc->scsiio_depth -
> INTERNAL_SCSIIO_CMDS_COUNT directly.
>
Okay.
>> +for (smid = 1; smid <= ioc->scsiio_depth; smid++) {
>> +struct scsiio_tracker *st;
>>
>> +st = mpt3sas_get_st_from_smid(ioc, smid);
>> +if (st->cb_idx != 0xFF)
>> +ioc->pending_io_count++;
>> +}
>
> How is this different from ioc->host->host_busy (except for maybe
> the ioctl smid).
>
This is just a plain conversion from the existing code; but yes, one
could be using host_busy here.
>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
>> b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
>> index 23e0ef1..bcc6a51 100644
>> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
>> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
>> @@ -563,11 +563,10 @@ enum block_state {
>> Mpi2SCSITaskManagementRequest_t *tm_request)
>> {
>> u8 found = 0;
>> -u16 i;
>> +u16 smid;
>> u16 handle;
>> struct scsi_cmnd *scmd;
>> struct MPT3SAS_DEVICE *priv_data;
>> -unsigned long flags;
>> Mpi2SCSITaskManagementReply_t *tm_reply;
>> u32 sz;
>> u32 lun;
>> @@ -583,11 +582,11 @@ enum block_state {
>> lun = scsilun_to_int((struct scsi_lun *)tm_request->LUN);
>>
>> handle = le16_to_cpu(tm_request->DevHandle);
>> -spin_lock_irqsave(&ioc->scsi_lookup_lock, flags);
>> -for (i = ioc->scsiio_depth; i && !found; i--) {
>> -scmd = ioc->scsi_lookup[i - 1].scmd;
>> -if (scmd == NULL || scmd->device == NULL ||
>> -scmd->device->hostdata == NULL)
>> +for (smid = ioc->scsiio_depth; smid && !found; smid--) {
>> +struct scsiio_tracker *st;
>> +
>> +scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid);
>> +if (!scmd)
>> continue;
>> if (lun != scmd->device->lun)
>> continue;
>> @@ -596,10 +595,10 @@ enum block_state {
>> continue;
>> if (priv_data->sas_target->handle != handle)
>> continue;
>> -tm_request->TaskMID = cpu_to_le16(ioc->scsi_lookup[i - 1].smid);
>> +st = scsi_cmd_priv(scmd);
>> +tm_request->TaskMID = cpu_to_le16(st->smid);
>> found = 1;
>> }
>> -spin_unlock_irqrestore(&ioc->scsi_lookup_lock, flags);
>
> This looks like you're keeping the existing behavior, but can someone
> please explain why we need to find a random used smid here? Whatever
> requests i
Re: [PATCHv3 10/10] mpt3sas: lockless command submission for scsi-mq
On Tue, Feb 21, 2017 at 01:27:09PM +0100, Hannes Reinecke wrote:
> Enable lockless command submission for scsi-mq by moving the
> command structure into the payload for struct request.
No dependency on scsi-mq, so the changelog could use a little update.
> @@ -2345,26 +2354,22 @@ struct scsiio_tracker *
> mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx,
> struct scsi_cmnd *scmd)
> {
> + struct scsiio_tracker *request = scsi_cmd_priv(scmd);
> + unsigned int tag;
> u16 smid;
>
> + if (ioc->shost->use_blk_mq) {
> + u32 unique_tag = blk_mq_unique_tag(scmd->request);
> +
> + tag = blk_mq_unique_tag_to_tag(unique_tag);
> + } else
> + tag = scmd->request->tag;
All that unique_tag stuff is only required for SCSI LLDDs that
set nr_hw_queues > 1, which isn't the ase with this patch.
> * @ioc: per adapter object
> @@ -2423,23 +2444,21 @@ struct scsiio_tracker *
> unsigned long flags;
> int i;
>
> if (smid < ioc->hi_priority_smid) {
> + struct scsiio_tracker *st;
>
> + st = mpt3sas_get_st_from_smid(ioc, smid);
> + if (WARN_ON(!st)) {
> + _base_recovery_check(ioc);
> + return;
> + }
> + mpt3sas_base_clear_st(ioc, st);
> _base_recovery_check(ioc);
> return;
This looks fine, but it would be good if could derive the scsiio_tracker
structure from the scsi_cmnd in as many as possible places, most notably
all the fast path calls and then just clal mpt3sas_base_clear_st directly.
> + ioc->shost->reserved_cmds = INTERNAL_SCSIIO_CMDS_COUNT;
Why? You're never calling the block layer to allocate a reserved
request.
> + ioc->shost->can_queue = ioc->scsiio_depth - ioc->shost->reserved_cmds;
Just assign ioc->shost->can_queue to ioc->scsiio_depth -
INTERNAL_SCSIIO_CMDS_COUNT directly.
> + for (smid = 1; smid <= ioc->scsiio_depth; smid++) {
> + struct scsiio_tracker *st;
>
> + st = mpt3sas_get_st_from_smid(ioc, smid);
> + if (st->cb_idx != 0xFF)
> + ioc->pending_io_count++;
> + }
How is this different from ioc->host->host_busy (except for maybe
the ioctl smid).
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index 23e0ef1..bcc6a51 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> @@ -563,11 +563,10 @@ enum block_state {
> Mpi2SCSITaskManagementRequest_t *tm_request)
> {
> u8 found = 0;
> - u16 i;
> + u16 smid;
> u16 handle;
> struct scsi_cmnd *scmd;
> struct MPT3SAS_DEVICE *priv_data;
> - unsigned long flags;
> Mpi2SCSITaskManagementReply_t *tm_reply;
> u32 sz;
> u32 lun;
> @@ -583,11 +582,11 @@ enum block_state {
> lun = scsilun_to_int((struct scsi_lun *)tm_request->LUN);
>
> handle = le16_to_cpu(tm_request->DevHandle);
> - spin_lock_irqsave(&ioc->scsi_lookup_lock, flags);
> - for (i = ioc->scsiio_depth; i && !found; i--) {
> - scmd = ioc->scsi_lookup[i - 1].scmd;
> - if (scmd == NULL || scmd->device == NULL ||
> - scmd->device->hostdata == NULL)
> + for (smid = ioc->scsiio_depth; smid && !found; smid--) {
> + struct scsiio_tracker *st;
> +
> + scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid);
> + if (!scmd)
> continue;
> if (lun != scmd->device->lun)
> continue;
> @@ -596,10 +595,10 @@ enum block_state {
> continue;
> if (priv_data->sas_target->handle != handle)
> continue;
> - tm_request->TaskMID = cpu_to_le16(ioc->scsi_lookup[i - 1].smid);
> + st = scsi_cmd_priv(scmd);
> + tm_request->TaskMID = cpu_to_le16(st->smid);
> found = 1;
> }
> - spin_unlock_irqrestore(&ioc->scsi_lookup_lock, flags);
This looks like you're keeping the existing behavior, but can someone
please explain why we need to find a random used smid here? Whatever
requests is using the smid could be completed at any point in time,
so this whole loop looks rather bogus both in the old and new version.
a
> + if (ioc->shost->use_blk_mq)
> + smid = 1;
> + else
> + smid = ioc->scsiio_depth - ioc->shost->reserved_cmds;
Huh? Explanation, please.
> @@ -1146,22 +1101,21 @@ struct _sas_node *
> _scsih_scsi_lookup_find_by_target(struct MPT3SAS_ADAPTER *ioc, int id,
> int channel)
> {
> + u8 found = 0;
> + u16 smid;
>
> + for (smid = 1; smid <= ioc->scsiio_depth; smid++) {
> + struct scsi_cmnd *scmd;
> +
> + scmd = mpt3sas_scsih_scsi_lookup_get(ioc, smid);
> + if (!scmd)
> + continue;
> +
[PATCHv3 10/10] mpt3sas: lockless command submission for scsi-mq
Enable lockless command submission for scsi-mq by moving the
command structure into the payload for struct request.
Signed-off-by: Hannes Reinecke
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 123 -
drivers/scsi/mpt3sas/mpt3sas_base.h | 19 ++--
drivers/scsi/mpt3sas/mpt3sas_ctl.c | 22 +++--
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 153 ++-
drivers/scsi/mpt3sas/mpt3sas_warpdrive.c | 35 +--
5 files changed, 128 insertions(+), 224 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 0875e58..0177d37 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -865,10 +865,19 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
struct scsiio_tracker *
mpt3sas_get_st_from_smid(struct MPT3SAS_ADAPTER *ioc, u16 smid)
{
+ u32 unique_tag;
+ struct scsi_cmnd *cmd;
+
if (WARN_ON(!smid) ||
WARN_ON(smid >= ioc->hi_priority_smid))
return NULL;
- return &ioc->scsi_lookup[smid - 1];
+
+ unique_tag = smid - 1;
+ cmd = scsi_host_find_tag(ioc->shost, unique_tag);
+ if (cmd)
+ return scsi_cmd_priv(cmd);
+
+ return NULL;
}
/**
@@ -2345,26 +2354,22 @@ struct scsiio_tracker *
mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx,
struct scsi_cmnd *scmd)
{
- unsigned long flags;
- struct scsiio_tracker *request;
+ struct scsiio_tracker *request = scsi_cmd_priv(scmd);
+ unsigned int tag;
u16 smid;
- spin_lock_irqsave(&ioc->scsi_lookup_lock, flags);
- if (list_empty(&ioc->free_list)) {
- spin_unlock_irqrestore(&ioc->scsi_lookup_lock, flags);
- pr_err(MPT3SAS_FMT "%s: smid not available\n",
- ioc->name, __func__);
- return 0;
- }
+ if (ioc->shost->use_blk_mq) {
+ u32 unique_tag = blk_mq_unique_tag(scmd->request);
+
+ tag = blk_mq_unique_tag_to_tag(unique_tag);
+ } else
+ tag = scmd->request->tag;
- request = list_entry(ioc->free_list.next,
- struct scsiio_tracker, tracker_list);
- request->scmd = scmd;
+ smid = tag + 1;
request->cb_idx = cb_idx;
- smid = request->smid;
request->msix_io = _base_get_msix_index(ioc);
- list_del(&request->tracker_list);
- spin_unlock_irqrestore(&ioc->scsi_lookup_lock, flags);
+ request->smid = smid;
+ INIT_LIST_HEAD(&request->chain_list);
return smid;
}
@@ -2410,6 +2415,22 @@ struct scsiio_tracker *
}
}
+void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc,
+ struct scsiio_tracker *st)
+{
+ if (WARN_ON(st->smid == 0))
+ return;
+ st->cb_idx = 0xFF;
+ st->direct_io = 0;
+ if (!list_empty(&st->chain_list)) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&ioc->scsi_lookup_lock, flags);
+ list_splice_init(&st->chain_list, &ioc->free_chain_list);
+ spin_unlock_irqrestore(&ioc->scsi_lookup_lock, flags);
+ }
+}
+
/**
* mpt3sas_base_free_smid - put smid back on free_list
* @ioc: per adapter object
@@ -2423,23 +2444,21 @@ struct scsiio_tracker *
unsigned long flags;
int i;
- spin_lock_irqsave(&ioc->scsi_lookup_lock, flags);
if (smid < ioc->hi_priority_smid) {
- /* scsiio queue */
- i = smid - 1;
- list_splice_init(&ioc->scsi_lookup[i].chain_list,
-&ioc->free_chain_list);
- ioc->scsi_lookup[i].cb_idx = 0xFF;
- ioc->scsi_lookup[i].scmd = NULL;
- ioc->scsi_lookup[i].direct_io = 0;
- if (i < ioc->scsiio_depth - INTERNAL_SCSIIO_CMDS_COUNT)
- list_add(&ioc->scsi_lookup[i].tracker_list,
-&ioc->free_list);
- spin_unlock_irqrestore(&ioc->scsi_lookup_lock, flags);
+ struct scsiio_tracker *st;
+ st = mpt3sas_get_st_from_smid(ioc, smid);
+ if (WARN_ON(!st)) {
+ _base_recovery_check(ioc);
+ return;
+ }
+ mpt3sas_base_clear_st(ioc, st);
_base_recovery_check(ioc);
return;
- } else if (smid < ioc->internal_smid) {
+ }
+
+ spin_lock_irqsave(&ioc->scsi_lookup_lock, flags);
+ if (smid < ioc->internal_smid) {
/* hi-priority */
i = smid - ioc->hi_priority_smid;
ioc->hpr_lookup[i].cb_idx = 0xFF;
@@ -3272,10 +3291,6 @@ struct scsiio_tracker *
ioc->config_page, ioc->config_page_dma);
}
- if (ioc->scsi_lookup) {
- free_pages((ulong)ioc->scsi_lookup, ioc->scsi_lookup_pages);
