Re: git: 805d759338a2 - main - mlx4: Move DEFINE_MUTEX() outside function body.
Date: Sun, 21 May 2023 17:05:41 UTC
On 21 May 2023, at 17:57, Hans Petter Selasky <hselasky@freebsd.org> wrote: > > 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. No it’s not. All the definitions are static. > Basically DEFINE_MUTEX() expands to a bunch of structures, which are not in any block. They are all static. > The "static" you see in patch just covers the first mutex structure. Because only the first one is ever not static. The rest always are. > SYSINITs use "static" in front of all structure definitions. Exactly. > If you want to change from static structures to global symbols, then my change is correct. Which will bloat symbol tables excessively. But you didn’t state this as your goal, you stated it as a behavioural change *today*, which it’s not (other than changing the scope). Your commit message was entirely nonsense, and so I told you that. If your goal is instead to allow for future changes, put that in your commit message. I am not a mind reader, nor is anyone else. It is extremely unhelpful to have commits that say one thing but intend to achieve a different thing. > Before: > > static DEFINE_MUTEX(xxx); > > Expands to something like: > > static struct yyy xxx; static struct sysinit zzz; .... > > > > 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. Clearly trying to define a variable with global scope at function scope isn’t going to work, yes. > 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 Which looks like a mess to me. Jess > --HPS