On 5/31/19 3:46 PM, John Garry wrote:
> On 31/05/2019 14:18, Hannes Reinecke wrote:
>> On 5/31/19 11:42 AM, John Garry wrote:
>>>>>>>
>>>>>>> -static int hisi_sas_slot_index_alloc(struct hisi_hba *hisi_hba,
>>>>>>> - struct scsi_cmnd *scsi_cmnd)
>>>>>>> +static int hisi_sas_slot_index_alloc(struct hisi_hba *hisi_hba)
>>>>>>> {
>>>>>>> int index;
>>>>>>> void *bitmap = hisi_hba->slot_index_tags;
>>>>>>> unsigned long flags;
>>>>>>>
>>>>>>> - if (scsi_cmnd)
>>>>>>> - return scsi_cmnd->request->tag;
>>>>>>> -
>>>>>>> spin_lock_irqsave(&hisi_hba->lock, flags);
>>>>>>> index = find_next_zero_bit(bitmap, hisi_hba->slot_index_count,
>>>>>>> hisi_hba->last_slot_index + 1);
>>>>>>
>>>>>> Then you switch to hisi_sas_slot_index_alloc() for allocating the
>>>>>> unique
>>>>>> tag via spin_lock & find_next_zero_bit(). Do you think this way is
>>>>>> more
>>>>>> efficient than blk-mq's sbitmap?
>>>
>>> These are not fast path, as used only for TMF, internal IO, etc.
>>>
>>> Having said that, hopefully we can move to scsi_{get,put}_reserved_cmd()
>>> when available, so that the LLDD has to stop managing them.
>>>
>>>>>>
>>>>> slot_index_alloc() is only used for commands which do _not_ have a tag
>>>>> (eg internal commands), or for v2 hardware which has weird allocation
>>>>> rules.
>>>>
>>>> But this patch has switched to this allocation unconditionally for all
>>>> commands:
>>>>
>>>
>>> As Hannes said, v2 had a few bugs which meant that we had to make a
>>> specific version of this function for that hw revision, cf.
>>> slot_index_alloc_quirk_v2_hw(), and it cannot use request queue tag.
>>>
>>> But, indeed, we could consider sbitmap for v2 hw. I'm not sure if it
>>> would help, considering the weird rules.
>>>
>> We should be able to get away by shifting all tags by 1 to the left,
>> and adding '1' to SMP/SAS commands, and '2' to STP commands, no?
>> Then index '0' would be free, and the allocation rules are satisfied.
>>
>
> The crazy (escalating from weird) rules to workaround the HW bug(s) mean
> that we need to chop up the command tag range into blocks of 32 even tag
> indexes per SATA device; this means that SATA device #0 can use 64, 66,
> 68, 70...126. device #1 can use 128, 130, 132,..., device #2 can use
> 192, 194,...
>
> I don't know how you can take a rq tag and generate a command tag
> suitable for a SATA device.
> Actually, you can.
We can setup a _distinct_ tagset per SATA device.
Eg we can setup a shared tagset for SAS (which is half the size of the
original tagset), and shift the tags by one to get a valid SAS tag.
For SATA we can setup a _distinct_ tagset per device, containing 32
tags. The we can invoke some calculation to transform the tag (which is
not guaranteed to be in the range of 0-31) into a valid hardware tag.
Should actually work.
Problem is that we'd need to set aside some tags for TMF, but I really
don't think that we can or should do command aborts on SATA; while there
is the 'abort NCQ' command, it'll work only for NCQ commands, and won't
help us for 'normal' commands.
And seeing that on error all NCQ commands will be aborted anyway, plus
the standard ATA error handler will be resulting into a device reset, I
guess we should skip command abort on SATA and escalate to device reset
straightaway. That would also have the nice benefit that we need only to
set _one_ tag aside for TMF, as we will only send one TMF at a time.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
