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

On 21 May 2023, at 20:13, Hans Petter Selasky <hselasky@freebsd.org> wrote:
> 
> On 5/21/23 20:02, Jessica Clarke wrote:
>> On 21 May 2023, at 18:46, Hans Petter Selasky <hselasky@freebsd.org> wrote:
>>> 
>>> 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’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.
>>> 
>>> Hi Jess,
>>> 
>>> 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.
> 
> Hi Jess,
> 
> 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.
> 
> Maybe an automated tool exists, to analyze texts for frequent multiple meanings, not just spell-checking.
> 
>>> 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.
> 
> OK, no problem.
> 
>>> 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.
>>> 
>>> For example declaring a static mutex:
>>> 
>>> 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.
>>> 
>>> 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’s 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’re 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’s DEFINE_MUTEX lets you do that. So shoving a bunch of symbols into the global namespace is a non-starter.
> 
> 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.
> 
> 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.
> 
> In FreeBSD there are two modes of compilation:
> 
> 1) Monotolith: /boot/kernel/kernel
> 2) Modules: /boot/kernel/*.ko
> 
> 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.
> 
> 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’s a pretty gross hack IMO, and depending on how you do it may still not be unique. Not to mention it’s 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’s 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’s API, implemented in LinuxKPI. So Linux’s behaviour is absolutely relevant. Any deviation from Linux’s 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