Re: git: cff79fd02636 - main - linuxkpi: Fix spin_lock_init
Date: Fri, 17 May 2024 21:36:02 UTC
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 <jhb@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 <manu@FreeBSD.org> >>>> AuthorDate: 2024-05-17 04:52:53 +0000 >>>> Commit: Emmanuel Vadot <manu@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 <david@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