Date: Sun, 21 May 2023 20:41:40 +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: <2BF2F7CF-82E6-4D98-800D-C35D7C29E948@freebsd.org> In-Reply-To: <ffb3f6ea-ef74-ebe3-884d-da567defd2e6@freebsd.org> References: <202305211621.34LGLsup059861@gitrepo.freebsd.org> <54EF67D8-2A79-4EAB-8EFB-232F14FFE792@freebsd.org> <21c9532e-4ca7-a7fe-1ff6-07a94cbad6ab@freebsd.org> <3066464F-E4C6-4B84-ADFF-E578AFAFE622@freebsd.org> <e802dfc0-0a16-add1-cf5d-a8811b3bd3e6@freebsd.org> <26465813-3B51-4E52-9E9D-F93A0F2AF6BD@freebsd.org> <ffb3f6ea-ef74-ebe3-884d-da567defd2e6@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 21 May 2023, at 20:13, Hans Petter Selasky <hselasky@freebsd.org> =
wrote:
>=20
> On 5/21/23 20:02, Jessica Clarke wrote:
>> On 21 May 2023, at 18:46, Hans Petter Selasky <hselasky@freebsd.org> =
wrote:
>>>=20
>>> On 5/21/23 19:05, Jessica Clarke wrote:
>>>> On 21 May 2023, at 17:57, Hans Petter Selasky =
<hselasky@freebsd.org> wrote:
>>>>> 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.
>>>=20
>>> Hi Jess,
>>>=20
>>> To me the word "avoid" is agnostic of time. And that's why I used it =
there. It doesn't mean there is a bug, but there may easily be a bug =
there.
>> I strongly disagree. Your wording heavily implied that you were =
avoiding something that currently occurs.
>=20
> Hi Jess,
>=20
> Thanks for sharing your view. I know some words in English have =
multiple meanings, and should not be used, because the receiver may pick =
a different interpretation than you intended at the time of writing. The =
way you react makes me think "avoid" is one more of those words. I have =
a list of do-not-use words already. And it grows from time to time.
>=20
> Maybe an automated tool exists, to analyze texts for frequent multiple =
meanings, not just spell-checking.
>=20
>>> If you have time, I can add you for review more often. Let me know =
what you prefer.
>> Code review is encouraged by the project, whether from me or anyone =
else.
>=20
> OK, no problem.
>=20
>>> When the kernel uses dynamic linking, you end up having tons of =
relocation entries in the resulting ELF file. Getting rid of symbol =
names doesn't stop relocation entries from piling up.
>>>=20
>>> For example declaring a static mutex:
>>>=20
>>> At first you have the static mutex. Then the sysinit structure needs =
one relocation to refer to the static mutex. Then after that the dataset =
mechanism needs another relocation to refer to the sysinit structure.
>>>=20
>>> sysinit's work, but there may be better ways to do it.
>> I am well aware of how relocations work. I am a compiler and linker =
developer (and co-chair of RISC-V=E2=80=99s psABI task group); my =
expertise lies in the world of linking. Relocations serve a purpose at =
run time. Symbols like this do not. Moreover they now risk clashing as =
they=E2=80=99re now visible outside the translation unit. For example, =
ib_addr.c and ib_cma.c both have static DEFINE_MUTEX(lock); and that =
needs to work as-is because Linux=E2=80=99s DEFINE_MUTEX lets you do =
that. So shoving a bunch of symbols into the global namespace is a =
non-starter.
>=20
> Yes, that is correct for Linux (and possibly other OS'es), but not for =
FreeBSD! Maybe I can take this opportunity to give you some insight.
>=20
> In the Linux kernel there may be multiple C- and object files =
resulting from compilation, sharing the same basename. Linux and FreeBSD =
both compile C-files into object files by substituting the ".c" suffix =
by ".o" (or something very similar). It depends a bit.
>=20
> In FreeBSD there are two modes of compilation:
>=20
> 1) Monotolith: /boot/kernel/kernel
> 2) Modules: /boot/kernel/*.ko
>=20
> If there is a kernel module, there is also a corresponding =
configuration keyword to include that kernel module into the =
monotolithic kernel. See for example "sys/amd64/conf/GENERIC". There are =
very few exceptions. The LINT target builds almost all sources into a =
monotolithic kernel.
>=20
> One difference between Linux and FreeBSD when doing the monotolithic =
kernel build: FreeBSD puts all object files resulting from compilation =
into the same object directory. This implies there cannot be two object =
files sharing the same name as a result of compilation. This in turn =
implies all source file names are unique across the whole of "sys/*", =
because converting a source file name into the corresponding object file =
name, is simply done by changing the suffix of the file in question.
I am aware of all of this. Please do not talk to me like I am an idiot. =
Your tone here is extremely patronising.
> If you are worried multiple DEFINE_MUTEX(lock) will result in multiple =
global symbols having the same "lock" name, all you need to do is pass =
through the ${.TARGET} variable from "make" as a define, stripping a few =
invalid characters, and macro-concat that to the locally generated =
global variable name, and you are all good.
You could. But that=E2=80=99s a pretty gross hack IMO, and depending on =
how you do it may still not be unique. Not to mention it=E2=80=99s going =
to further bloat the symbol tables with these long names.
Given the sysinit subsystem still needs to be able to merge in new =
sysinits registered dynamically, one might as well just do the sort =
dynamically, it=E2=80=99s very little added complexity on top, =
especially when sorting functions are just a libkern call away. Much =
less complexity than all this scripting to generate tables.
> Your comment is correct based on your prior knowledge about Linux and =
compilers. But at this point FreeBSD is different. That's why porting =
code from Linux to FreeBSD is sometimes difficult, because you need to =
keep track of how source files are renamed. You cannot just use "meld =
Linux/blah FreeBSD/blah" to compare directories ...
DEFINE_MUTEX is Linux=E2=80=99s API, implemented in LinuxKPI. So =
Linux=E2=80=99s behaviour is absolutely relevant. Any deviation from =
Linux=E2=80=99s semantics is an incompatibility that requires patching =
any sources that are built for FreeBSD using LinuxKPI. It is generally =
best to minimise that.
So, once again, I am asking you to revert your change.
Jess
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2BF2F7CF-82E6-4D98-800D-C35D7C29E948>