From owner-svn-src-all@freebsd.org Wed Nov 25 11:11:30 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id CD81E47E227; Wed, 25 Nov 2020 11:11:30 +0000 (UTC) (envelope-from se@freebsd.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4CgyrL5KZPz4t3J; Wed, 25 Nov 2020 11:11:30 +0000 (UTC) (envelope-from se@freebsd.org) Received: from Stefans-MBP-WLAN.fritz.box (p200300cd5f0ff700ac60d7652b2f1231.dip0.t-ipconnect.de [IPv6:2003:cd:5f0f:f700:ac60:d765:2b2f:1231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: se/mail) by smtp.freebsd.org (Postfix) with ESMTPSA id EE5A95B38; Wed, 25 Nov 2020 11:11:29 +0000 (UTC) (envelope-from se@freebsd.org) To: Mateusz Guzik Cc: Jessica Clarke , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <202011181944.0AIJiUU3003699@repo.freebsd.org> <81fd2ce6-b5b1-7f05-c575-6b233e78b739@freebsd.org> From: Stefan Esser Subject: Re: svn commit: r367813 - head/lib/libutil Message-ID: <09a2828f-75ce-9cba-3687-b7f8001c3e96@freebsd.org> Date: Wed, 25 Nov 2020 12:11:26 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.16; rv:78.0) Gecko/20100101 Thunderbird/78.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="AV3ZeQ2jb8vhhkulWX1xjSZf8PODlyWZ4" X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Nov 2020 11:11:30 -0000 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --AV3ZeQ2jb8vhhkulWX1xjSZf8PODlyWZ4 Content-Type: multipart/mixed; boundary="bdjZ9S9Gk4NkJpstnxXD4ibwFF685BzDf"; protected-headers="v1" From: Stefan Esser To: Mateusz Guzik Cc: Jessica Clarke , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Message-ID: <09a2828f-75ce-9cba-3687-b7f8001c3e96@freebsd.org> Subject: Re: svn commit: r367813 - head/lib/libutil References: <202011181944.0AIJiUU3003699@repo.freebsd.org> <81fd2ce6-b5b1-7f05-c575-6b233e78b739@freebsd.org> In-Reply-To: --bdjZ9S9Gk4NkJpstnxXD4ibwFF685BzDf Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable Am 19.11.20 um 01:37 schrieb Mateusz Guzik: > On 11/19/20, Stefan Esser wrote: [...] >> I just wanted to provide an implementation of this functionality to >> be used in a number of programs where other developers had expressed >> interest in such a feature (and one of these programs has been worked >> on by me in recent weeks, so I'm now able to make use of it myself). >=20 > The entire localbase saga is getting way out of hand. Yes, apparently. > To address this e-mail and things you wrote in another reply, my > comlaints are very simple and are not getting less valid for not being > raised sooner. I just was not watching any of this until recent > fallout. >=20 > For the change at hand, there has to be a reason to use a static > symbol. Standard example is catching otherwise expensive to obtain > data. For that the static localbase pointer makes perfect sense, while > the static lookup array does not have any justification that I see. The reason to use a static symbol for the return value is that I do not want to have to use the getlocalbase() on my systems at all. The static lookup array is trivially changed to be stack allocated and filled at run-time, this is a non-issue, IMHO. As explained in an earlier discussion, I'd rather build my systems with a _PATH_LOCALBASE set to a non-default value than use any run-time setting of this value. And thus, my implementation will be just to define getlocalbase in the libutil.h on my systems: #define getlocalbase() _PATH_LOCALBASE Copying into a user provided buffer is of course possible, and such an implementation had been committed before. It does not allow to enforce a compiled in _PATH_LOCALBASE in the way I want to use it, but I would find a way around this. But the diffs to programs that use getlocalbase with caller supplied buffers were significantly larger. My version can just replace the getenv("LOCALBASE") found in a number of places by getlocalbase() (and since getlocalbase() returns the default value if the environment variable has not been set, remove the fallback code for that case). > Bringing cache, TLB or whatever microarchitectural details into the > discussion is beyond not warranted. I did not start bringing in such issues, see the mail I responded to. And I argued that it just did not matter which way the argument array was defined, since either method has minor advantages and disadvantages. > More importantly though the commit comes with a self-confessed memory > leak and is a show stopper. There is no memory leak, actually. Only if you called getlocalbase() in a multi-threaded environment multiple times and in such a way that the non-NULL test before the assignment overlaps with an assignment in another thread, there could have been a leak of a few bytes. But you could not exploit this leak in any way, since there is a limited number of cores that can execute threads in parallel. My supposed initial version did not have any memory leak and had been rejected due to the pre-allocation of a static buffer. I do not agree that this is an issue, since the number of VM pages allocated for the data segment of the library does not grow, and this is what counts. It is easily possible to limit the user.localbase variable to a useful size in the kernel and the library (e.g. 64 bytes, which should be sufficient as a PREFIX). But even at MAXPATHLEN, the memory usage is not increased by this buffer. > That said, I'll see about patching this up. I have created a new review with a static buffer. It does not have any memory leaks, does not increase the memory usage of libutil, and it is fully thread safe and guaranteed to return the same value on each successive invocation (which seems to make sense for a system parameter like LOCALBASE - the code can easily be changed to perform the getenv and sysctl calls on each invocation again). https://reviews.freebsd.org/D27370 Regards, STefan --bdjZ9S9Gk4NkJpstnxXD4ibwFF685BzDf-- --AV3ZeQ2jb8vhhkulWX1xjSZf8PODlyWZ4 Content-Type: application/pgp-signature; name="OpenPGP_signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="OpenPGP_signature" -----BEGIN PGP SIGNATURE----- wsB5BAABCAAjFiEEo3HqZZwL7MgrcVMTR+u171r99UQFAl++O94FAwAAAAAACgkQR+u171r99USs +wgAuJpC8WYHIiKllw/8eSQuBtEqcjXYJanl0f3MN/IQZFciI26kvi4kp/icqJFdLst0ZTGTc2OA htweKTDvvEScuyiK/ndzcPnlJW9b3aKNZU5b/sci4wpyjDi9CvwQh1IX71lzb6EHNQKYuWVp7ff6 KMbxfTZSSc9o4FHvmDUA7khjFQYM+t2WEvTuPDrzJKWXYk/5iwp2W6TxS3XUgRsARF07VPqIODM7 1m4L7BtKnYTLH3cyxlYanI3hgsNNlGgXyskRtKZN1DalvMkmOHbFfJ8BKbswcjqyubeHpdjFe2Bq CyzZzth7QcudNriH7y2qYD4ois0fXUaomChHVrSHUg== =K1vD -----END PGP SIGNATURE----- --AV3ZeQ2jb8vhhkulWX1xjSZf8PODlyWZ4--