Re: git: cff79fd02636 - main - linuxkpi: Fix spin_lock_init

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