Bug 165622
| Summary: | [ndis][panic][patch] Unregistered use of FPU in kernel on amd64 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Vladyslav Movchan <vladislav.movchan> | ||||||||||
| Component: | kern | Assignee: | Oleksandr Tymoshenko <gonzo> | ||||||||||
| Status: | Closed FIXED | ||||||||||||
| Severity: | Affects Some People | CC: | AWilcox, avilla, danfe, gonzo, jhb, kib, net | ||||||||||
| Priority: | Normal | Keywords: | crash, needs-qa, patch | ||||||||||
| Version: | CURRENT | Flags: | gonzo:
mfc-stable11+
gonzo: mfc-stable10- |
||||||||||
| Hardware: | Any | ||||||||||||
| OS: | Any | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Vladyslav Movchan
2012-03-02 13:50:09 UTC
Responsible Changed From-To: freebsd-bugs->freebsd-net Over to maintainer(s). I've reimplemented original patch to maintain cache of fpu_kern_ctx elements to reduce amount of allocations/deallocations done via fpu_kern_alloc_ctx/fpu_kern_free_ctx. It is complex to measure performance gain of this change due to deadlock in ndis code (what makes it complex or impossible to stress-test), but when the same change was tested with https://github.com/NDISulator/ it allowed to get about 10% higher bandwidth with 1Gbps NIC (which was CPU bound). This patch still applies cleanly to 11.0-CURRENT and is still needed for at least the bcm43xx 5.100.235.19 NDIS 5 driver. Comment on attachment 122422 [details]
fpu_patch3.txt
Did you consider removing the O(n) lookup by keeping separate "busy" and "free" lists? You could then also assert that that the busy list was empty in the windrv_libfini() routine.
Created attachment 153770 [details]
Fixed version
(In reply to John Baldwin from comment #4) Thanks for the advice. I've reimplemented the patch. Now it maintains separate "free" and "busy" lists what changes lookup for unused element from O(n) to O(1). Also I switched from singly-linked list to a doubly linked lists to make it possible to do an arbitrary element removal (necessary for "busy" list) in O(1). And also I've added assertion to windrv_libfini() to make sure busy list is empty. With NDIS'ified WinXP Broadcom BCM943228HMB WiFi driver, I observed similar panic on fresh FreeBSD/amd64 12.0-CURRENT r307735:308608M. Attached patch had fixed it. batch change of PRs untouched in 2018 marked "in progress" back to open. (Adding kib@ to Cc since he was part of the original discussion.) Looks like this patch is still relevant. I reviewed the patch and on it seems to be OK to me and since it was also tested by other users I'd like to commit it. But I am not NDIS/amd64 expert and would like second opinion. Kostik, could you take a look and let me know whether it can be committed as-is or some additional work required. Thanks (In reply to Oleksandr Tymoshenko from comment #9) I do not have a strong opinion there, if you want to commit this, go ahead. I do find it strange the unused fpu context list, why not simply call fpu_kern_alloc() as needed ? I suspect that the cost of allocation is lower than contending on the mutex. Thanks for the feedback and attention to this bug. I can't argue why using mutex-protected local cache of fpu_kern_ctx structures was faster than allocating/deallocating this structures when necessary, but in my tests mentioned in comment 2 it was so. Difference was clearly visible. Unfortunately I can't re-run same tests as it seems I no longer own 1Gbps card usable with NDIS - almost 7 years passed since. A commit references this bug: Author: gonzo Date: Tue Jan 22 03:53:43 UTC 2019 New revision: 343298 URL: https://svnweb.freebsd.org/changeset/base/343298 Log: [ndis] Fix unregistered use of FPU by NDIS in kernel on amd64 amd64 miniport drivers are allowed to use FPU which triggers "Unregistered use of FPU in kernel" panic. Wrap all variants of MSCALL with fpu_kern_enter/fpu_kern_leave. To reduce amount of allocations/deallocations done via fpu_kern_alloc_ctx/fpu_kern_free_ctx maintain cache of fpu_kern_ctx elements. Based on the patch by Paul B Mahol PR: 165622 Submitted by: Vlad Movchan <vladislav.movchan@gmail.com> MFC after: 1 month Changes: head/sys/compat/ndis/kern_windrv.c head/sys/compat/ndis/pe_var.h A commit references this bug: Author: gonzo Date: Sat Mar 23 22:44:12 UTC 2019 New revision: 345459 URL: https://svnweb.freebsd.org/changeset/base/345459 Log: MFC r343298: [ndis] Fix unregistered use of FPU by NDIS in kernel on amd64 amd64 miniport drivers are allowed to use FPU which triggers "Unregistered use of FPU in kernel" panic. Wrap all variants of MSCALL with fpu_kern_enter/fpu_kern_leave. To reduce amount of allocations/deallocations done via fpu_kern_alloc_ctx/fpu_kern_free_ctx maintain cache of fpu_kern_ctx elements. Based on the patch by Paul B Mahol PR: 165622 Submitted by: Vlad Movchan <vladislav.movchan@gmail.com> Changes: _U stable/12/ stable/12/sys/compat/ndis/kern_windrv.c stable/12/sys/compat/ndis/pe_var.h A commit references this bug: Author: gonzo Date: Thu Apr 25 00:58:12 UTC 2019 New revision: 346656 URL: https://svnweb.freebsd.org/changeset/base/346656 Log: MFC r343298: [ndis] Fix unregistered use of FPU by NDIS in kernel on amd64 amd64 miniport drivers are allowed to use FPU which triggers "Unregistered use of FPU in kernel" panic. Wrap all variants of MSCALL with fpu_kern_enter/fpu_kern_leave. To reduce amount of allocations/deallocations done via fpu_kern_alloc_ctx/fpu_kern_free_ctx maintain cache of fpu_kern_ctx elements. Based on the patch by Paul B Mahol PR: 165622 Submitted by: Vlad Movchan <vladislav.movchan@gmail.com> Changes: _U stable/11/ stable/11/sys/compat/ndis/kern_windrv.c stable/11/sys/compat/ndis/pe_var.h |
