Re: [PATCH 0/4] Remove link debounce delays by default

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Damien,


Am 24.03.22 um 07:15 schrieb Damien Le Moal:
> On 3/24/22 14:30, Paul Menzel wrote:

>> Am 24.03.22 um 00:58 schrieb Damien Le Moal:
>>> On 3/23/22 19:59, Paul Menzel wrote:
>>>>>> It’d be great if you gave an example benchmark.
>>>>>
>>>>> No need for a benchmark. This is not hot path stuff. This code runs only
>>>>> during device discovery on boot and on resume after suspend.
>>>>> So apply the patches and reboot, check dmesg if you see errors or not and
>>>>> if your disks are all there. Same after a suspend+resume.
>>>>
>>>> Yes, I know what I need to check. I meant, that you write without this
>>>> patchset my system with HBA controller X and 32 [vendor/model] disks
>>>> attached reaches the message Y in Z1 seconds, and with the series Z2
>>>> seconds.
>>>
>>> There is going to be too much variation from machine to machine as that
>>> depends on the adapter & devices used for testing. The only sensible thing
>>> to do is to compare timing before patching with timing after patching and
>>> see if there are some gains. On my test rig, I have so many drives and
>>> various HBAs connected that the boot time gains overall are nil. But I do
>>> see faster per-ata drive scan times.
>>
>> And for one of these it’d be great to have exact numbers. ;-)
>>
>>>>>>> Comments and lots of testing are welcome !
>>>>>>>
>>>>>>> Damien Le Moal (4):
>>>>>>>       ata: libata-sata: Simplify sata_link_resume() interface
>>>>>>>       ata: libata-sata: Introduce struct sata_deb_timing
>>>>>>>       ata: libata-sata: Remove debounce delay by default
>>>>>>>       ata: libata-sata: Improve sata_link_debounce()
>>>>>>
>>>>>> […]
>>>>>>
>>>>>> I am wondering how sure we can be, there won’t be any regressions? Not
>>>>>> having the boot disk detected is quite a serious issue/regression, and
>>>>>> it should be made easy for users to fix that without having to rebuild
>>>>>> the Linux kernel. A Linux kernel CLI parameter to enable the delay would
>>>>>> be helpful for effected users.
>>>>>
>>>>> I am working on another series for that. The patches will allow
>>>>> controlling most horkage and link flags on/off using libata.force kernel
>>>>> boot parameter. That will allow figuring out problems without patching in
>>>>> the field, for patches to be later added.
>>>>
>>>> Sounds good. But this needs to be available before the changes at hand,
>>>> doesn’t it?
>>>
>>> Not really. For now, we need to check if these patches break anything,
>>> regardless of the libata.force changes. I consider libata.force for field
>>> debugging. A user should not have to use it to get a system running. The
>>> kernel should have sensible defaults for that and things should run out of
>>> the box without the need for additional kernel boot parameters.
>>
>> Sorry, I have the feeling we misunderstand each other. Just to be clear,
>> you are saying before shipping this to users, we can be 100 % certain
>> that these changes won’t break any systems out there?
> 
> The patches are only an improvement for what can be controlled using the
> libata.force boot parameter. No other change. So nothing will break with
> these patches.

Still we are misunderstanding each other? I am talking about the testing 
and possible regressions of the debounce delay patches, and not the 
“libata.force/horkage patches”.

> See Documentation/admin-guide/kernel-parameters.txt and compare the list
> of possible arguments that libata.force can take to the number of
> ATA_HORKAGE and ATA_LFLAGS defined... There are many that cannot be
> tweaked using libata.force, which is a pain when debugging a problem in
> the field. With more arguments for libata.force, we can test adding by
> default horkages/lflags for a device without having to patch and recompile
> a kernel, tasks that many end user do not know how to do.

Yes, I agree, that the “libata.force/horkage patches” are going to be safe.


Kind regards,

Paul