From nobody Wed Dec 08 20:27:35 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 7450D18DA985; Wed, 8 Dec 2021 20:27:42 +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 4J8THd4sn7z3jtC; Wed, 8 Dec 2021 20:27:41 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: by spindle.one-eyed-alien.net (Postfix, from userid 3001) id 55F783C0199; Wed, 8 Dec 2021 20:27:35 +0000 (UTC) Date: Wed, 8 Dec 2021 20:27:35 +0000 From: Brooks Davis To: Brooks Davis Cc: Konstantin Belousov , 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: <20211208202735.GA7977@spindle.one-eyed-alien.net> References: <202112042221.1B4ML7Ov002151@gitrepo.freebsd.org> <20211206172124.GA40392@spindle.one-eyed-alien.net> <20211206184414.GA37105@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="PNTmBPCT7hxwcZjr" Content-Disposition: inline In-Reply-To: <20211206184414.GA37105@spindle.one-eyed-alien.net> User-Agent: Mutt/1.9.4 (2018-02-28) X-Rspamd-Queue-Id: 4J8THd4sn7z3jtC X-Spamd-Bar: / Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=none (mx1.freebsd.org: domain of brooks@spindle.one-eyed-alien.net has no SPF policy when checking 199.48.129.229) smtp.mailfrom=brooks@spindle.one-eyed-alien.net X-Spamd-Result: default: False [0.81 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-0.28)[-0.284]; FREEFALL_USER(0.00)[brooks]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; NEURAL_SPAM_SHORT(0.99)[0.994]; MIME_GOOD(-0.20)[multipart/signed,text/plain]; DMARC_NA(0.00)[freebsd.org]; AUTH_NA(1.00)[]; RCPT_COUNT_FIVE(0.00)[5]; TO_MATCH_ENVRCPT_SOME(0.00)[]; NEURAL_SPAM_LONG(1.00)[0.999]; SIGNED_PGP(-2.00)[]; FORGED_SENDER(0.30)[brooks@freebsd.org,brooks@spindle.one-eyed-alien.net]; RCVD_COUNT_ZERO(0.00)[0]; R_SPF_NA(0.00)[no SPF record]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+,1:+,2:~]; ASN(0.00)[asn:36236, ipnet:199.48.128.0/22, country:US]; FROM_NEQ_ENVFROM(0.00)[brooks@freebsd.org,brooks@spindle.one-eyed-alien.net]; FREEMAIL_CC(0.00)[gmail.com,freebsd.org] X-ThisMailContainsUnwantedMimeParts: N --PNTmBPCT7hxwcZjr Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable The more I think about the new interface the more I hate it. PLEASE just add a new syscall and remove this hack. -- Brooks On Mon, Dec 06, 2021 at 06:44:14PM +0000, Brooks Davis wrote: > 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=3Da4e4132fa3bfadb6047f= c0fa5f399f4640460300 > > > >=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 structu= re > > > > =20 > > > > For compatibility, add a placeholder pointer to the start of the > > > > added struct swapoff_new_args, and use it to distinguish old vs= =2E 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 memo= ry > > > 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?= view=3Dmsvc-170 > >=20 > > I can add some annotations there, but I am really surprised to see 'mus= t' > > statements about it. >=20 > 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. >=20 > 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 > > >=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 saf= e. >=20 > > Again, this is something new and relatively arbitrary. I will do this, > > I do not see much harm from changing this on HEAD still. >=20 > 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. >=20 > All this being said, please just add a swapoff2(const char *name, > u_int flags) and avoid all this hassle. --PNTmBPCT7hxwcZjr Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBAgAGBQJhsRU2AAoJEKzQXbSebgfAyeUH/1YBm2QdnXwKSgZFEDA7i+E0 EppmwUtkFkpdQm9lVEFtN00OsNxPuSJA+JYbuSJuG/jJtOAo0XWB1RuMCldPXstS Ak7MUc0m1+dzwLpaKu9yF1QYvQa+pq/P8+W9EPHIa4Uxo2vn1rFXz7+Hr5ee+uq7 TZAu4eLV/xin20tcwGvS6oqm6CQiUi9WhI2OWnth4EZ9RTSJp6uZFiwuwnbfr7JR NaQeNaFYkoVhvKlDx18D2JAXUqn5LJQJspyF1zqNuw75tGk2zUqSFynadiW2+TOl 4cCeZPAMUR1TejPbMVzw9rOIeJJQ9JdHn0vsWJyHm64KXStkXfKP38GTS3GsTi0= =Av1b -----END PGP SIGNATURE----- --PNTmBPCT7hxwcZjr--