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

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