On Fri, 17 May 2024, John Baldwin wrote:
On 5/17/24 9:33 AM, Emmanuel Vadot wrote:
On Fri, 17 May 2024 09:07:53 -0700
John Baldwin <j...@freebsd.org> wrote:
On 5/16/24 10:59 PM, Emmanuel Vadot wrote:
The branch main has been updated by manu:
URL:
https://cgit.FreeBSD.org/src/commit/?id=cff79fd02636f34010d8b835cc9e55401fa76e74
commit cff79fd02636f34010d8b835cc9e55401fa76e74
Author: Emmanuel Vadot <m...@freebsd.org>
AuthorDate: 2024-05-17 04:52:53 +0000
Commit: Emmanuel Vadot <m...@freebsd.org>
CommitDate: 2024-05-17 05:58:59 +0000
linuxkpi: Fix spin_lock_init
Some linux code re-init some spinlock so add MTX_NEW to
mtx_init.
Reported by: David Wolfskill <da...@catwhisker.org>
Fixes: ae38a1a1bfdf ("linuxkpi: spinlock: Simplify code")
---
sys/compat/linuxkpi/common/include/linux/spinlock.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sys/compat/linuxkpi/common/include/linux/spinlock.h
b/sys/compat/linuxkpi/common/include/linux/spinlock.h
index 3f6eb4bb70f6..2992e41c9c02 100644
--- a/sys/compat/linuxkpi/common/include/linux/spinlock.h
+++ b/sys/compat/linuxkpi/common/include/linux/spinlock.h
@@ -140,7 +140,7 @@ typedef struct mtx spinlock_t;
#define spin_lock_name(name) _spin_lock_name(name,
__FILE__, __LINE__)
#define spin_lock_init(lock) mtx_init(lock,
spin_lock_name("lnxspin"), \
- NULL, MTX_DEF | MTX_NOWITNESS)
+ NULL, MTX_DEF | MTX_NOWITNESS | MTX_NEW)
#define spin_lock_destroy(_l) mtx_destroy(_l)
This is only ok because of MTX_NOWITNESS. Reiniting locks without
destroying
them corrupts the internal linked lists in WITNESS for locks using
witness.
That may warrant a comment here explaining why we disable witness.
I'll try to look at what linux expect for spinlocks, it could also be
that we need to do this because some drivers via linuxkpi does weird
things ...
It might be nice to add an extension to the various lock inits for code
that
wants to opt-int to using WITNESS where a name can be passed. Using those
would
be relatively small diffs in the client code and let select locks opt into
using WITNESS. You could make it work by adding an optional second
argument
to spin_lock_init, etc. that takes the name.
We can't change spin_lock_init, we need to follow linux api here.
You can use macro magic to add support for an optional second argument.
#define _spin_lock_init2(lock, name) mtx_init(lock, name, NULL, MTX_DEF)
#define _spin_lock_init1(lock) mtx_init(lock, spin_lock_name("lnxspin"), ...)
#define _spin_lock_init_macro(lock, name, NAME, ...)
NAME
#define spin_lock_init(...) \
_spin_lock_init_macro(__VA_ARGS__, _spin_lock_init2,
_spin_lock_init1)(__VA_ARGS__)
Then you can choose to specifically annotate certain locks with a name
instead in which case they will use WITNESS.
I think the real confusing here comes from the fact that FreeBSD has
spin_lock_destroy in LinuxKPI which I believe is a local addition
(though not documented as such) for semi-native drivers using parts of
LinuxKPI and in order to use spinlocks according to "FreeBSD
expectations".
I believe (and still hope someone would correct me) that Linux has not
functions to destroy locks like we do? I believe for rwlocks I had left
that remark on the review:
https://reviews.freebsd.org/D45206#1031316
So if you use WITNESS anywhere you could only really do so for
"internal" parts but nowhere in Linux driver code as that will likely
simply break assumptions?
--
Bjoern A. Zeeb r15:7
