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

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.

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

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

Jess

>>> 
>>> This change also allows for:
>>> 
>>> https://reviews.freebsd.org/D40193
>> Which looks like a mess to me.
> If you have a better way in your mind to do this, then implement it, and let me know.
> 
> --HPS