Date: Wed, 08 Dec 2021 16:01:41 -0600 From: Mike Karels <mike@karels.net> 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: <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> <Ya5Q3kqFeVgbgZtw@kib.kiev.ua> <20211206184414.GA37105@spindle.one-eyed-alien.net> <20211208202735.GA7977@spindle.one-eyed-alien.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <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 >>>>> >>>>> 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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1FA60635-1D40-4055-9854-27772101470B>