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

On Mon, 20 May 2024, John Baldwin wrote:

> On 5/17/24 2:36 PM, Bjoern A. Zeeb wrote:
>> 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?
>
> You could only do it safely if you added local modifications to the driver,
> yes.  My understanding is that while we aim for as little modifications to
> drivers as possible to permit merging in updates, we don't mandate absolutely
> zero local changes.  The approach I described above would permit selectively
> using WITNESS at the expense of local diffs that would have to be maintained
> across updates.

Okay.  Makes sense.   In that case I think we should more than we do
today mark the spin_lock_destroy(), the WTINESS enabled ones, etc.
"FreeBSD specific" even in LinuxKPI (marking them with #ifdef or at
least comments or calling them slightly differently).

-- 
Bjoern A. Zeeb                                                     r15:7