On 12/22/2023 4:14 PM, Lukas Wunner wrote:
On Fri, Dec 22, 2023 at 09:56:39AM +0800, Ethan Zhao wrote:
I don't know if the polling along sleeping for completion of meanningless
devTLB invalidation request blindly sent to (removed/powered down/link down)
device makes sense or not.
If you have a way to get to the struct pci_dev * which you're waiting for
in qi_submit_sync() then I guess you could check for its presence and bail
out if it's gone, instead of issuing a cpu_relax().
One option to bail out the loop.
Again, the proposed patch is not a proper solution. It will paper over
the issue most of the time but every once in a while someone will still
get a hard lockup splat and it will then be more difficult to reproduce
and fix if the proposed patch is accepted.
Could you point out why is not proper ? Is there any other window
the hard lockup still could happen with the ATS capable devcie
supprise_removal case if we checked the connection state first ?
Please help to elaberate it.
Even though user space may have initiated orderly removal via sysfs,
the device may be yanked from the slot (surprise removed) while the
orderly removal is happening.
Yes, just after the wait descripor is submitted and before waiting in loop.
the rare but worst case.
Yes, this is the old kernel stack trace, but customer also tried lasted
6.7rc4
If you could provide a stacktrace for a contemporary kernel,
I think that would be preferred.
Customer tried, but they didn't provide me the lastest trace.
(doesn't work) and the patched 6.7rc4 (fixed).
Why is it fixed in v6.7-rc4? Is the present patch thus unnecessary?
Not fixed in v6.7rc4, with this patch, they said the unplug works.
Finally, it is common to adhere to terms
used in the PCIe Base Spec in commit messages, so "ATC Invalidate Request"
might be preferable to "devTLB flush request".
ATS Invalidate Request ? devTLB flush request has the same meaning,
I thought all iommu/PCIe guys could understand.
I'm just pointing out the preferred way to write commit messages
in the PCI subsystem (as I've perceived it over the years) so that
you can reduce the number of iterations you have to go through
due to maintainer feedback. I'm just trying to be helpful.
Understand.
How to define the point "some" msec to timeout while software
break out the waiting loop ? or polling if the target is gone ?
I'd say adhere to the 1 min + 50% number provided in the spec.
If you know the device is gone before that then you can break out
of the loop in qi_submit_sync() of course.
I am trying to find a way to break it out in this qi_submit_sync().
checking the device state in this loop, but seems not good in this
iommu low level code and need some interfaces to be modified.
That would cost me much more hours to make the rare case work,
to be perfect:
1. check the pci device state in the loop
2. modify the invalidation descriptor status in
pciehp_ist()->intel_iommu_release_device() call.
The question is, does the Intel IOMMU have a timeout at all for
Invalidate Requests? I guess we don't really know that because
in the stack trace you've provided, the watchdog stops the machine
before a timeout occurs. So it's at least 12 sec. Or there's
no timeout at all.
The calltrace wouldn't tell us there is really timeout of 1min+50%
or not, event there is, meanlingless.
If the Intel IOMMU doesn't enforce a timeout, you should probably amend
qi_submit_sync() to break out of the loop once the 1 min + 50% limit
is exceeded. And you need to amend the function to sleep instead of
polling in interrupt context.
Too many paths to call this function, and revise it to non-sync, to much
things impacted.
Can you check with hardware engineers whether there's a timeout?
Combinated with third party PCIe switch chips ?
Thanks,
Ethan
Thanks,
Lukas
