Date: Sun, 21 May 2023 18:05:41 +0100 From: Jessica Clarke <jrtc27@freebsd.org> To: Hans Petter Selasky <hselasky@freebsd.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: 805d759338a2 - main - mlx4: Move DEFINE_MUTEX() outside function body. Message-ID: <3066464F-E4C6-4B84-ADFF-E578AFAFE622@freebsd.org> In-Reply-To: <21c9532e-4ca7-a7fe-1ff6-07a94cbad6ab@freebsd.org> References: <202305211621.34LGLsup059861@gitrepo.freebsd.org> <54EF67D8-2A79-4EAB-8EFB-232F14FFE792@freebsd.org> <21c9532e-4ca7-a7fe-1ff6-07a94cbad6ab@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 21 May 2023, at 17:57, Hans Petter Selasky <hselasky@freebsd.org> = wrote: >=20 > On 5/21/23 18:33, Jessica Clarke wrote: >> On 21 May 2023, at 17:21, Hans Petter Selasky <hselasky@FreeBSD.org> = wrote: >>>=20 >>> The branch main has been updated by hselasky: >>>=20 >>> URL: = https://cgit.FreeBSD.org/src/commit/?id=3D805d759338a2be939fffc8bf3f25cfaa= b981a9be >>>=20 >>> 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 >>>=20 >>> mlx4: Move DEFINE_MUTEX() outside function body. >>>=20 >>> Move static mutex declaration outside function body, to avoid = global >>> variables being declared on the stack, when using SYSINITs. >> What? This is nonsense. It=E2=80=99s not on the stack either way = round. >> Please revert this. >> Jess >=20 > Hi Jess, >=20 > I think this is a false positive of yours. You need to look through = all the macros used there. No it=E2=80=99s 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=E2=80=99t state = this as your goal, you stated it as a behavioural change *today*, which = it=E2=80=99s 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: >=20 > static DEFINE_MUTEX(xxx); >=20 > Expands to something like: >=20 > static struct yyy xxx; static struct sysinit zzz; .... >=20 >=20 >=20 > 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=E2=80=99t going to work, yes. > You can't declare global variables inside a function or it is not good = style. >=20 >=20 >=20 > =46rom 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. >=20 > This change also allows for: >=20 > https://reviews.freebsd.org/D40193 Which looks like a mess to me. Jess > --HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3066464F-E4C6-4B84-ADFF-E578AFAFE622>