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

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