From nobody Wed Dec 08 22:01:41 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 01D7718C6497; Wed, 8 Dec 2021 22:01:43 +0000 (UTC) (envelope-from mike@karels.net) Received: from mail.karels.net (mail.karels.net [216.160.39.52]) by mx1.freebsd.org (Postfix) with ESMTP id 4J8WN70Y7gz4TBP; Wed, 8 Dec 2021 22:01:42 +0000 (UTC) (envelope-from mike@karels.net) Received: from mail.karels.net (localhost [127.0.0.1]) by mail.karels.net (8.16.1/8.16.1) with ESMTP id 1B8M1ggY049760; Wed, 8 Dec 2021 16:01:42 -0600 (CST) (envelope-from mike@karels.net) Received: from [10.0.2.130] ([10.0.1.1]) by mail.karels.net with ESMTPSA id FqU6BEYrsWFewgAA4+wvSQ (envelope-from ); Wed, 08 Dec 2021 16:01:42 -0600 From: Mike Karels 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 Date: Wed, 08 Dec 2021 16:01:41 -0600 X-Mailer: MailMate (1.14r5818) Message-ID: <1FA60635-1D40-4055-9854-27772101470B@karels.net> In-Reply-To: <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> <20211208202735.GA7977@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: text/plain Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4J8WN70Y7gz4TBP X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-ThisMailContainsUnwantedMimeParts: N On 8 Dec 2021, at 14:27, Brooks Davis wrote: > The more I think about the new interface the more I hate it. PLEASE > just add a new syscall and remove this hack. > > -- Brooks I agree with Brooks, please add a new syscall. Mike > 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: >>>>> >>>>> URL: https://cgit.FreeBSD.org/src/commit/?id=3Da4e4132fa3bfadb6047f= c0fa5f399f4640460300 >>>>> >>>>> commit a4e4132fa3bfadb6047fc0fa5f399f4640460300 >>>>> Author: Konstantin Belousov >>>>> AuthorDate: 2021-11-29 16:26:31 +0000 >>>>> Commit: Konstantin Belousov >>>>> CommitDate: 2021-12-04 22:20:58 +0000 >>>>> >>>>> swapoff(2): replace special device name argument with a structu= re >>>>> >>>>> For compatibility, add a placeholder pointer to the start of th= e >>>>> added struct swapoff_new_args, and use it to distinguish old vs= =2E new >>>>> style of syscall invocation. >>>> >>>> I agree with Jess that this should be a new syscall. >>> >>>> The entry in >>>> sycalls.master now fails to describe the memory footprint of the nam= e >>>> argument. No system call should be created or altered to have a mem= ory >>>> 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 utilize= d >>> in any way by the system. >>> >>> Also, I do not remember a discussion anywhere which would indicate th= at >>> 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 bee= n >>> added to the discussion for it to be fair. >>> >>> The only reference I can find that defines what SAL is, is >>> https://docs.microsoft.com/en-us/cpp/c-runtime-library/sal-annotation= s?view=3Dmsvc-170 >>> >>> I can add some annotations there, but I am really surprised to see 'm= ust' >>> 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. >> >>>> >>>>> 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 >>>>> >>>>> +struct swapoff_new_args { >>>>> + const char *name_old_syscall; >>>>> + const char *name; >>>>> + u_int flags; >>>>> + u_int pad0; >>>>> + uintptr_t pad1[8]; >>>>> +}; >>>> >>>> If you're going to attempt to add future-proofing, please pad with t= he >>>> assumption that pointers are 128-bit sized and aligned. In this >>>> case, that would mean an uint64_t pad before pad1. If there were do= ne >>>> in place, adding the pad and dropping pad1 to 6 elements would be sa= fe. >> >>> Again, this is something new and relatively arbitrary. I will do thi= s, >>> 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.