Re: git: a4e4132fa3bf - main - swapoff(2): replace special device name argument with a structure

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=a4e4132fa3bfadb6047fc0fa5f399f4640460300
> > > 
> > > 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 structure
> > >     
> > >     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.
> > 
> > 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 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.
> 
> 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.
> 
> 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=msvc-170
> 
> 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.

> > 
> > > 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 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