From nobody Sun May 21 21:58:45 2023 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4QPZGp3fqcz4CXJr for ; Sun, 21 May 2023 21:58:58 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: from mail-wr1-f47.google.com (mail-wr1-f47.google.com [209.85.221.47]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4QPZGp2SWBz4Lhm for ; Sun, 21 May 2023 21:58:58 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-wr1-f47.google.com with SMTP id ffacd0b85a97d-30a1fdde3d6so1470045f8f.0 for ; Sun, 21 May 2023 14:58:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684706336; x=1687298336; h=to:references:message-id:content-transfer-encoding:cc:date :in-reply-to:from:subject:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=z1KrN8S8Fr5ccnwuzvxQEbOWm4pD8ds7hWg4S1SGqSs=; b=eVlwxELmG3+C/cR2jP1FNnprUT7LyuX9PHv4ojJGjEarSe/M30xE5te1NMAVYpSn+S Aw+HhF1jJarS27pg5SGS9q05+GTUP7w2K+75tUMNGAR5FOBg/5YUJDjUDURYu97bAYW/ YPrVUVKexFhKNf5bFxZYytW/InXS3iPek0+AtDw7pL4ePEEcxcNZrKls8xsdSW+Pgpyo o5XKMwaSBOZ3mgB56k0R8OA9hCieiRn2W5C8uvGbRO/ZL4mQOROXDgBqa8Aq5+57Cru1 7CmX6igvueB+DGObOF5ZNATGrSW548CgX/AcGfdihVFS0H2Tnubi4gZwjQ6Bt3cLWLzy EfJw== X-Gm-Message-State: AC+VfDyK+Zc1FfOuiZ3zG5dmPip/MjzEP60TwdB399WJhgWfhgcEvd6K 4b26epCAoPjMOmks3tfo4F8atg== X-Google-Smtp-Source: ACHHUZ4DNmPOgFPkEdHKQXRQnHjx7rxbfLwkYvf3GahEkm6nVQP3Ry/5hWb8DizJfRTUXXsGoRUYmA== X-Received: by 2002:a5d:518a:0:b0:307:4836:64e4 with SMTP id k10-20020a5d518a000000b00307483664e4mr5934767wrv.52.1684706336569; Sun, 21 May 2023 14:58:56 -0700 (PDT) Received: from smtpclient.apple ([131.111.5.246]) by smtp.gmail.com with ESMTPSA id c4-20020a7bc004000000b003f42461ac75sm9518004wmb.12.2023.05.21.14.58.55 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 21 May 2023 14:58:55 -0700 (PDT) Content-Type: text/plain; charset=utf-8 List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.500.231\)) Subject: Re: git: 805d759338a2 - main - mlx4: Move DEFINE_MUTEX() outside function body. From: Jessica Clarke In-Reply-To: Date: Sun, 21 May 2023 22:58:45 +0100 Cc: "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Content-Transfer-Encoding: quoted-printable Message-Id: 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> <26465813-3B51-4E52-9E9D-F93A0F2AF6BD@freebsd.org> <2BF2F7CF-82E6-4D98-800D-C35D7C29E948@freebsd.org> To: Hans Petter Selasky X-Mailer: Apple Mail (2.3731.500.231) X-Rspamd-Queue-Id: 4QPZGp2SWBz4Lhm X-Spamd-Bar: ---- X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US] X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-ThisMailContainsUnwantedMimeParts: N On 21 May 2023, at 22:37, Hans Petter Selasky = 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