Re: git: 805d759338a2 - main - mlx4: Move DEFINE_MUTEX() outside function body.

From: Mark Millard <marklmi_at_yahoo.com>
Date: Sun, 21 May 2023 17:44:24 UTC
On May 21, 2023, at 10:14, Mark Millard <marklmi@yahoo.com> wrote:

> Hans Petter Selasky <hselasky_at_freebsd.org> wrote on
> Date: Sun, 21 May 2023 16:57:47 UTC  :
> 
>> On 5/21/23 18:33, Jessica Clarke wrote:
>>> On 21 May 2023, at 17:21, Hans Petter Selasky <hselasky@FreeBSD.org> wrote:
>>>> 
>>>> The branch main has been updated by hselasky:
>>>> 
>>>> URL: https://cgit.FreeBSD.org/src/commit/?id=805d759338a2be939fffc8bf3f25cfaab981a9be
>>>> 
>>>> commit 805d759338a2be939fffc8bf3f25cfaab981a9be
>>>> Author: Hans Petter Selasky <hselasky@FreeBSD.org>
>>>> AuthorDate: 2023-05-21 11:25:28 +0000
>>>> Commit: Hans Petter Selasky <hselasky@FreeBSD.org>
>>>> CommitDate: 2023-05-21 16:20:16 +0000
>>>> 
>>>> mlx4: Move DEFINE_MUTEX() outside function body.
>>>> 
>>>> Move static mutex declaration outside function body, to avoid global
>>>> variables being declared on the stack, when using SYSINITs.
>>> 
>>> What? This is nonsense. It’s not on the stack either way round.
>>> 
>>> Please revert this.
>>> 
>>> Jess
>> 
>> Hi Jess,
>> 
>> I think this is a false positive of yours. You need to look through all 
>> the macros used there.
>> 
>> Basically DEFINE_MUTEX() expands to a bunch of structures, which are not 
>> in any block.
> 
> DEFINE_MUTEX is from/for linux related code:
> 
> # grep -r "define.*DEFINE_MUTEX" /usr/main-src/sys/ | more
> /usr/main-src/sys/compat/linuxkpi/common/include/linux/mutex.h:#define  DEFINE_MUTEX(lock)                                              \
> 
> and looks like:
> 
> #define DEFINE_MUTEX(lock)                                              \
>        mutex_t lock;                                                   \
>        SX_SYSINIT_FLAGS(lock, &(lock).sx, mutex_name(#lock), SX_DUPOK)

Never mind: starting with mutex_t was not essential.

>> The "static" you see in patch just covers the first mutex structure.
>> 
>> SYSINITs use "static" in front of all structure definitions.
>> If you want to change from static structures to global symbols, then my 
>> change is correct.
>> 
>> Before:
>> 
>> static DEFINE_MUTEX(xxx);
>> 
>> Expands to something like:
>> 
>> static struct yyy xxx; static struct sysinit zzz; ....
> 
> Not true.

Never mind: starting with mutex_t was not essential, despite
it not having a "struct" explicitly.


Sorry for the noise.

>> If you want to change from "static struct sysinit zzz;" to "extern 
>> struct sysinit zzz" and also initialize the structure there, then that 
>> won't work, based on what I currently know about C-programming. I tried, 
>> but clang gave me a warning about it.
>> 
>> 
>> 
>> You can't declare global variables inside a function or it is not good 
>> style.
>> 
>> 
>> 
>> From what I can see, this location is the only place I've come accross 
>> in the FreeBSD kernel, where a SYSINIT() is used inside a function, and 
>> I thought I would just move that outside the function instead.
>> 
>> This change also allows for:
>> 
>> https://reviews.freebsd.org/D40193
>> 
> 
> 


===
Mark Millard
marklmi at yahoo.com