On 3/24/22 14:30, Paul Menzel wrote: > Dear Damien, > > > 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. 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. -- Damien Le Moal Western Digital Research
