On 3/24/22 15:35, Paul Menzel wrote: > 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”. Ah. OK. Then the answer is no, there is a possibility that the patches break the detection of drives with some adapters. Most likely with old systems. This is not 100% safe. But the alternative is to keep the delays as is and keep patching as you did for disabling the delays on systems that are identified as OK. And I do not like this alternative either since I suspect that the majority of recent drives+AHCI adapters will be OK. I keep looking at the code to see how to reduce the risks. However, since the code being patched runs before we even know what drive is connected, we cannot for example rely on the drive specs version (old drive == old specs). The safe approach will be to mark most adapters with ATA_LFLAG_DEBOUNCE_DELAY and remove that flag for tested adapters, which will be easier to do with the libata.force changes, which will allow specifying "nodebounce_delay". The problem though is that there are lots of AHCI adapters that do not have an specific entry in ahci_port_info and use the default entry. Changing that one is the main risk. Still looking at the code to see what can be done to minimize that. -- Damien Le Moal Western Digital Research
