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

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)


> 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.

> 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