Date: Wed, 8 Dec 2021 20:27:35 +0000 From: Brooks Davis <brooks@freebsd.org> To: Brooks Davis <brooks@freebsd.org> Cc: Konstantin Belousov <kostikbel@gmail.com>, 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> In-Reply-To: <20211206184414.GA37105@spindle.one-eyed-alien.net> References: <202112042221.1B4ML7Ov002151@gitrepo.freebsd.org> <20211206172124.GA40392@spindle.one-eyed-alien.net> <Ya5Q3kqFeVgbgZtw@kib.kiev.ua> <20211206184414.GA37105@spindle.one-eyed-alien.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <kib@FreeBSD.org> > > > > AuthorDate: 2021-11-29 16:26:31 +0000 > > > > Commit: Konstantin Belousov <kib@FreeBSD.org> > > > > 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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20211208202735.GA7977>