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

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.

-- 
John Baldwin