From nobody Mon Dec 06 18:44:14 2021 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 EAB8918B5100; Mon, 6 Dec 2021 18:44:14 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: from spindle.one-eyed-alien.net (spindle.one-eyed-alien.net [199.48.129.229]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4J7C5B6Bhvz4RsP; Mon, 6 Dec 2021 18:44:14 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: by spindle.one-eyed-alien.net (Postfix, from userid 3001) id 3C13F3C0199; Mon, 6 Dec 2021 18:44:14 +0000 (UTC) Date: Mon, 6 Dec 2021 18:44:14 +0000 From: Brooks Davis To: Konstantin Belousov Cc: Brooks Davis , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: a4e4132fa3bf - main - swapoff(2): replace special device name argument with a structure Message-ID: <20211206184414.GA37105@spindle.one-eyed-alien.net> References: <202112042221.1B4ML7Ov002151@gitrepo.freebsd.org> <20211206172124.GA40392@spindle.one-eyed-alien.net> 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 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="qMm9M+Fa2AknHoGS" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-Rspamd-Queue-Id: 4J7C5B6Bhvz4RsP X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-ThisMailContainsUnwantedMimeParts: N --qMm9M+Fa2AknHoGS Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Dec 06, 2021 at 08:05:18PM +0200, Konstantin Belousov wrote: > On Mon, Dec 06, 2021 at 05:21:24PM +0000, Brooks Davis wrote: > > On Sat, Dec 04, 2021 at 10:21:07PM +0000, Konstantin Belousov wrote: > > > The branch main has been updated by kib: > > >=20 > > > URL: https://cgit.FreeBSD.org/src/commit/?id=3Da4e4132fa3bfadb6047fc0= fa5f399f4640460300 > > >=20 > > > commit a4e4132fa3bfadb6047fc0fa5f399f4640460300 > > > Author: Konstantin Belousov > > > AuthorDate: 2021-11-29 16:26:31 +0000 > > > Commit: Konstantin Belousov > > > CommitDate: 2021-12-04 22:20:58 +0000 > > >=20 > > > swapoff(2): replace special device name argument with a structure > > > =20 > > > For compatibility, add a placeholder pointer to the start of the > > > added struct swapoff_new_args, and use it to distinguish old vs. = new > > > style of syscall invocation. > >=20 > > I agree with Jess that this should be a new syscall. >=20 > > The entry in > > sycalls.master now fails to describe the memory footprint of the name > > argument. No system call should be created or altered to have a memory > > footprint not describable with SAL annotations unless an applicable > > standard such as POSIX requires it. > Why? Such requirement is not enforced in any way by the syscall > processing infrastructure, and more, that annotations are not utilized > in any way by the system. >=20 > Also, I do not remember a discussion anywhere which would indicate that > community agreed to this requirement. Since arguably I am the person > that added enough new syscalls in recent times (I do not claim that I > added the majority but probably quite close to it), I should have been > added to the discussion for it to be fair. >=20 > The only reference I can find that defines what SAL is, is > https://docs.microsoft.com/en-us/cpp/c-runtime-library/sal-annotations?vi= ew=3Dmsvc-170 >=20 > I can add some annotations there, but I am really surprised to see 'must' > statements about it. I believe we discussed this when adding the SAL annotations several years ago. We probably didn't do enough to make the requirements explicit (and some existing syscalls are impossible to annotate), but IMO it's implicit that if annotations are required, you shouldn't add system calls (and by implication alter existing ones) such that the annotations can't describe them. SAL annotations should be sufficiently documented in syscalls.master's big top comment. I do not believe there is any way to describe this interface which IMO is a pretty strong warning it's not a good design choice. > >=20 > > > diff --git a/sys/vm/swap_pager.h b/sys/vm/swap_pager.h > > > index 395fbc9957c4..469de3e8eaf4 100644 > > > --- a/sys/vm/swap_pager.h > > > +++ b/sys/vm/swap_pager.h > > > @@ -69,6 +69,14 @@ struct swdevt { > > > #define SW_UNMAPPED 0x01 > > > #define SW_CLOSING 0x04 > > > =20 > > > +struct swapoff_new_args { > > > + const char *name_old_syscall; > > > + const char *name; > > > + u_int flags; > > > + u_int pad0; > > > + uintptr_t pad1[8]; > > > +}; > >=20 > > If you're going to attempt to add future-proofing, please pad with the > > assumption that pointers are 128-bit sized and aligned. In this > > case, that would mean an uint64_t pad before pad1. If there were done > > in place, adding the pad and dropping pad1 to 6 elements would be safe. > Again, this is something new and relatively arbitrary. I will do this, > I do not see much harm from changing this on HEAD still. Real (if limited production) hardware that runs FreeBSD and has this constraint exists today (Arm Morello boards), to ignore it when adding rather speculative future-proofing seems to miss the point. All this being said, please just add a swapoff2(const char *name, u_int flags) and avoid all this hassle. -- Brooks --qMm9M+Fa2AknHoGS Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBAgAGBQJhrln9AAoJEKzQXbSebgfASrsH/RvS1XIU3xcw7J4wYJMHaZn8 6OyvOWmCORf5zTTGDdbaBZE2qSPmSYmmqdFJXf/Z6tKBIRxrXilb00UPPBPYkeeG 8tvC1MUdiNMADJPPpRgJQ41m1HEAjZee+EJfVMAxE4elay7PqFJUEmcHljBbhle+ 8ydiTYvqOvNbCsf20ZX5fRFtgbj3l4UfU7w/d5sisOhBpg9FJ3p8g0m46rK5Qaow V8lpK1/4C6RNejr22YU/udE/tBIgziruYODpIeEBQT4ii5qJF7Q6yKVDhy/xQ351 oDr5EtXoW56Kb8Br4E8JL1mczcbBhVUPmeSO4hjoiqd5P+8llwvhsepcLfP9wg8= =CrmL -----END PGP SIGNATURE----- --qMm9M+Fa2AknHoGS--