Re: [PATCH V4 1/2] scsi: core: avoid host-wide host_busy counter for scsi_mq

[John Garry <john.garry@huawei.com>]

On 25/10/2019 10:43, Ming Lei wrote:
> On Fri, Oct 25, 2019 at 09:58:16AM +0100, John Garry wrote:
>>
>>>> In scsi_host.h, we have for scsi_host_template.can_queue: "It is set to the
>>>> maximum number of simultaneous commands a given host adapter will accept.",
>>>> so that should be honoured.
>>>
>>
>> Hi Ming,
>>
>>> That words should have been changed to:
>>>
>>> "It is set to the maximum number of simultaneous commands a given host adapter's
>>> hw queue will accept."
>>

Hi Ming,

>> I find this definition misleading. As you know, some MQ SAS HBAs can accept
>> .can_queue commands on a given hw queue, but can still only accept
>> .can_queue commands over all hw queues.
> 
> I don't know there are such MQ HBA driver in tree, 

HiSilicon SAS HBA can accept 4096 commands on any given hw queue but can 
also only accept 4096 commands over all queues simultaneously.  In fact, 
the hw queue depth is configurable.

That's why I think that the definition is misleading.

I think I sound like a broken record now :)

at least that is the
> current blk-mq/scsi-mq model: each hw queue has its own independent
> tags, so there can't be the limit for MQ HBA, which should allow to
> accept (.can_queue * nr_hw_queues) commands. And I did hear people
> complains bad performance caused by the atomic .host_busy counter.
> 

ok, seems reasonable for now.

> If you are talking about the current SQ(from blk-mq or scsi-mq viewpoint) HBA
> which has multiple reply queue(HPSA, hisilicon SAS, mpt3sas, and megaraid_sas),
> they are just the special type. According to scsi-mq's model, they should
> belong to SQ HBA.
> 
>>
>>>
>>>>
>>>> And Scsi_host.nr_hw_queues: "it is assumed that each hardware queue has a
>>>> queue depth of can_queue. In other words, the total queue depth per host is
>>>> nr_hw_queues * can_queue."
>>>
>>> The above is correct.
>>>
>>>>
>>>> I don't read "total queue depth per host" same as "maximum number of
>>>> simultaneous commands a given host adapter will accept". If anything, the
>>>> nr_hw_queues comment is ambiguous.
>>>>
>>>>>
>>>>> The point is simple, because each hw queue has its own independent tags,
>>>>> that is why I mentioned your Hisilicon SAS can't be converted to MQ
>>>>> easily cause this hardware has only single shared tags.
>>>>
>>>> Please be aware that HiSilicon SAS HW would not be unique for SCSI HBAs in
>>>> this regard, in that the unique hostwide tag is not just for HBA HW IO
>>>> management, but also is used as the tag for SCSI TMFs.
>>>
>>> Right.
>>>
>>>>
>>>> Just checking mpt3sas seems similar:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/mpt3sas/mpt3sas_scsih.c#n2918
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/mpt3sas/mpt3sas_base.c#n3546
>>>
>>> Not only mpt3sas, there are also HPSA and more. And these drivers have to
>>> support single hw queue of blk-mq, instead of real MQ. And the reason is that
>>> these HBA has single tags.
>>
>> We should be able to do better than that.
>>
>> For a start, at least doesn't the check you remove in scsi_host_is_busy()
>> limit commands the HBA accepts to .can_queue?
> 
> As I mentioned above, that is current blk-mq/scsi-mq's model, each hw
> queue has its own independent tags, so the check really doesn't make
> sense. >
>>
>> And if you make the change in this patch, then the changes to improve blk-mq
>> for CPU hotplug are pointless, as we can't change the SAS HBAs to expose
>> multiple queues.
> 
> No, just the small number of special type SCSI HBAs with multiple reply queue
> and single tags can't benefit from the patchset of 'improve blk-mq for CPU hotplug',
> and all other normal MQ device/drivers do get improved wrt. CPU hotplug.
> 

TBH, I'm not sure on the group of SCSI drivers which could benefit then.

As I see, only qla2xxx driver sets Scsi_host.nr_hw_queues and also uses 
pci_alloc_irq_vectors_affinity().

> We have tried hosttags approach for the several drivers, but looks it is
> too messy. Given there are only 3 or 4 such device, we still can improve
> them via driver private approach in future if no generic way is doable.

ok, I'd rather not go on this path - you may say I'm digging my head in 
the sand. Anyway, I'll continue to support the 'improve blk-mq for CPU 
hotplug' effort.

Thanks,
John


> 
> 
> Thanks,
> Ming
> 
> .
>