Re: [PATCH] aacraid: reply queue mapping to CPUs based of IRQ affinity

[John Garry <john.g.garry@oracle.com>]

On 19/04/2023 00:55, Sagar.Biradar@microchip.com wrote:
> [I'm with Jon: your email style makes digging information out of the emails very hard, which is why I only quote this section] On Mon, 2023-04-10 at 21:17 +0000,Sagar.Biradar@microchip.com  wrote:

I stopped replying as this wasn't fixed... and still isn't :((

>> ***blk-mq already does what you want here, including handling for the
>> case I mention above. It maintains a CPU -> HW queue mapping, and
>> using a reply map in the LLD is the old way of doing this.
>>
>> We also tried implementing the blk-mq mechanism in the driver and we
>> saw command timeouts.
>> The firmware has limitation of fixed number of queues per vector and
>> the blk-mq changes would saturate that limit.
>> That answers the possible command timeout.
> Could we have more details on this, please?  The problem is that this is a very fragile area of the kernel, so you rolling your own special snowflake implementation in the driver is going to be an ongoing maintenance problem (and the fact that you need this at all indicates you have long tail customers who will be around for a while yet).  If the only issue is limiting the number of queues per vector, we can look at getting the block layer to do that.  Although I was under the impression that you can do it yourself with the ->map_queues() callback.  Can you say a bit about why this didn't work?
> 
> [Sagar Biradar]
> Thank you for your response.
> We did venture trying into something like what you pointed to.
> We mapped the hardware queues, and we still see command timeout. This change doesn’t work for us at this stage.
> Also, we observed that the load is not balanced across all the CPUs.
> I am pasting the code snippets for better understanding.
> 
> 
> During the probe, we assigned the hardware queues.
> shost->nr_hw_queues = shost->can_queue; //inside aac_probe_one().

That is wrong - .can_queue is the number of IOs which the scsi host may 
be sent at any given time, while .nr_hw_queues should be the number of 
MSI(X) vectors returned from pci_alloc_vectors()

You should also set shost->host_tagset = 1.

Since the driver has reserved commands, the .can_queue should be reduced 
by the amount of reserved commands and the driver needs to manually 
choose which HW queue to send those reserved commands on - see example 
in other driver here: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/hisi_sas/hisi_sas_main.c?h=v6.3-rc7#n523

JFYI, We have tried to add reserved command support to SCSI ML, but it 
still has not been finished - see 
https://lore.kernel.org/linux-scsi/20211125151048.103910-1-hare@suse.de/

> 
> We also wrote a new routine "blk_mq_pci_map_queues" (mapped to .map_queues in scsi_host_template).
> static void aac_map_queues(struct Scsi_Host *shost)
> {
>                  struct aac_dev *aac = (struct aac_dev *)shost->hostdata;
>                  blk_mq_pci_map_queues(&shost->tag_set.map[HCTX_TYPE_DEFAULT],
>                                                        aac->pdev, 0);
> }

This looks ok.

> 
> With the above changes, we see command timeouts in the firmware space and the commands never return to the driver.
> This may need some changes in the firmware, but the firmware changes are restricted (since this is EOL product).
> Also, we saw that the load was entirely upon one CPU and it was not balanced across other CPUs.
> 
> We have had this reply_queue mechanism (patch) in our Out Of Box driver (OOB) for more than three years.
> We(vendors/customers included) have not observed any issues.