Date: Sun, 21 May 2023 22:58:45 +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: <A66F073E-06F4-4A91-82F3-01467D7A65BC@freebsd.org> In-Reply-To: <ac11ff8e-eeb9-a7e6-28d3-da5c9fad6567@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> <2BF2F7CF-82E6-4D98-800D-C35D7C29E948@freebsd.org> <ac11ff8e-eeb9-a7e6-28d3-da5c9fad6567@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 21 May 2023, at 22:37, Hans Petter Selasky <hselasky@freebsd.org> =
wrote:
>=20
> On 5/21/23 21:41, Jessica Clarke wrote:
>>> 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.
>=20
> Hi Jess,
>=20
>> I am aware of all of this. Please do not talk to me like I am an =
idiot. Your tone here is extremely patronising.
>=20
> I can not know what you know before you tell me. I'm not a mind reader =
and I for sure don't think you are an idiot. Those are in such case your =
own words about yourself and not mine, to make it clear.
You can=E2=80=99t know, though you could go look at commit history to =
see that I have made changes to the build system and thus probably have =
an understanding. Or you could assume that I know what I=E2=80=99m =
talking about when I choose to engage. Both are good things to do that =
show some amount of respect, otherwise you are at risk of insulting =
people by assuming that you need to explain the basics to them.
>>> 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.
>=20
>> 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.
>=20
> Why do you think I don't know what a hack is?
I didn=E2=80=99t. I said =E2=80=9CIMO=E2=80=9D. *My* opinion.
> In the case about the "lock" name being global:
>=20
> - My solution is to pass a define to the compiler to help make the =
identifier unique across the monotolithic kernel build.
>=20
> - When building the LINT target, the linker will fail if there are =
duplicate global symbols. Then the debug information output by the =
linker will tell the file and line. Go there and change the identifier. =
It should be rare. Problem solved.
>=20
> - All symbols being made global for the sake of processing them, are =
listed by name. And after the final sorted sysinit table is created and =
linked, all those symbol names can be stripped away from the resulting =
binary, and then only the relocations remain, if I'm not mistaken. I =
didn't do that in my differential review, because it is a prototype. The =
argument about symbol table bloat, because of prefixing or suffixing =
global symbols is not valid the way I see it.
>=20
>> 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.
>=20
> Do you agree about the following statement or not?
>=20
> If you have two already sorted lists and a pointer to each of them, =
then:
>=20
> Method a) place the two lists after each other in memory and then call =
qsort() on the resulting list, is relatively slow.
>=20
> Method b) look at the next element from each list, and store the =
smallest one into the destination list, and repeat until the end of both =
source lists are reached, is relatively much faster.
Clearly merging is faster. But compared with qsort it=E2=80=99s unlikely =
to be all that significant; lg n grows slowly.
>>> 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.
>=20
> One side of this is compile time issues. If a developer needs to =
change something to make the code compile for FreeBSD, that's =
acceptable. And then the developer can also say the code was made for =
FreeBSD and not Linux, which is good with regards to the GPLv2 license, =
and not just copying solutions from Linux or being force to implement =
things exactly like on Linux, so-called bug-by-bug compliance.
>=20
> The other side is runtime issues. Here the goal should be to minimise =
issues to zero and try to outperform Linux.
I don=E2=80=99t have any clue what you=E2=80=99re going on about here. =
Tweaking Linux code to be compatible with FreeBSD=E2=80=99s LinuxKPI =
doesn=E2=80=99t suddenly render the code free from GPLv2, it=E2=80=99s =
still Linux-derived code, but you=E2=80=99ve had to modify it rather =
than be able to use it as-is, which is the goal of LinuxKPI, where =
possible. And so ideally *no* changes would be acceptable. So I disagree =
with your statement that needing to change things is acceptable; it=E2=80=99=
s only acceptable where there is no good alternative.
I don=E2=80=99t see what =E2=80=9Coutperform Linux=E2=80=9D has to do =
with any of this discussion.
>> So, once again, I am asking you to revert your change.
>=20
> I still cannot see why this change should be reverted.
Because it serves no purpose today, and should always remain valid code, =
because Linux=E2=80=99s API makes that valid, and thus LinuxKPI should =
ensure it=E2=80=99s valid too. If the change is required then LinuxKPI's =
compatibility has been broken, which is a regression that should not be =
permitted. Therefore it is totally unnecessary, and if anything is worse =
because it unnecessarily increases the scope of a variable that is only =
needed within a single function.
> Does the following code example and compiler warning explain why this =
change should remain:
>=20
> # cat << EOF > test.c
> int
> main()
> {
> extern char string[];
> char string[] =3D { "test" };
>=20
> return (0);
> }
> EOF
>=20
> # cc -O2 -Wall test.c
> test.c:11:7: error: non-extern declaration of 'string' follows extern =
declaration
> char string[] =3D { "test" };
> ^
> test.c:10:14: note: previous declaration is here
> extern char string[];
>=20
>=20
>=20
> Like my commit message says, you cannot declare global variables on =
the stack. When variables are on the stack, they are inside a function =
body. There is no stack outside any C-functions. It's just another way =
to say it and what is the problem about that?
I understood what you were *actually* trying to achieve with the change =
from your first reply. I have never disputed that. What I disputed was =
your commit message, which at best is extremely misleading and at worst =
makes no sense, and the necessity of the change. But this case should =
not be hit here, because if it is then you=E2=80=99ve broken LinuxKPI.
Please revert the commit. It should never be necessary. I=E2=80=99m not =
going to ask a fourth time.
Jess
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?A66F073E-06F4-4A91-82F3-01467D7A65BC>