Re: [PATCH 13/14] prctl.2: Add SVE prctls (arm64)

["Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>]

Hi,

On 5/13/20 12:46 PM, Dave Martin wrote:
> On Wed, May 13, 2020 at 09:43:52AM +0100, Will Deacon wrote:
>> Hi Dave,
>>
>> On Tue, May 12, 2020 at 05:36:58PM +0100, Dave Martin wrote:
>>> diff --git a/man2/prctl.2 b/man2/prctl.2
>>> index 7f511d2..dd16227 100644
>>> --- a/man2/prctl.2
>>> +++ b/man2/prctl.2
>>> @@ -1291,6 +1291,104 @@ call failing with the error
>>>  .BR ENXIO .
>>>  For further details, see the kernel source file
>>>  .IR Documentation/admin-guide/kernel-parameters.txt .
>>> +.\" prctl PR_SVE_SET_VL
>>> +.\" commit 2d2123bc7c7f843aa9db87720de159a049839862
>>> +.\" linux-5.6/Documentation/arm64/sve.rst
>>> +.TP
>>> +.BR PR_SVE_SET_VL " (since Linux 4.15, only on arm64)"
>>> +Configure the thread's SVE vector length,
>>> +as specified by
>>> +.IR arg2 .
>>> +Arguments
>>> +.IR arg3 ", " arg4 " and " arg5
>>> +are ignored.
>>
>> Bugger, did we forget to force these to zero? I guess we should write the
>> man-page first next time :(

Quite...

> Not an accident, but there does seem to be some inconsistency in policy
> among the various prctls here.

The whole 5-args-for-prctl thing was a bit of a misdesign.

The general preference is that, for new prctls, unused arguments 
should be required to be zero. Historically, there was much
inconsistency.

> glibc explicitly has
> 
> 	extern int prctl (int __option, ...);
> 
> (and nobody has to write _exit(0, 0, 0, 0, 0, 0) after all.)
> 
> Is there some agreed rationale for requiring redundant arguments to be
> supplied explicitly as zero?  For now there are likely to be few users
> of this, so we _might_ get away with changing the behaviour here if it's
> considered important enough.

See above.

> There is no forwards compatibility problem with this prctl though,
> because there are spare bits in arg2 which can "turn on" additional
> args if needed.
> 
> Also, it's implausible that PR_SVE_GET_VL will ever want an argument.
> 
> There are still 2 billion unallocated prctl numbers, so new prctls can
> always be added if there's ever a need to work around these limitations,
> but it seems extremely unlikely.
> 
>>
>>> +.IP
>>> +The bits of
>>> +.I arg2
>>> +corresponding to
>>> +.B SVE_VL_LEN_MASK
>>
>> PR_SVE_LEN_MASK
> 
> Hmm, not sure how that happened.  Good spot!
> 
> I'll recheck that all the names are real when reposting.
> 
>>> +must be set to the desired vector length in bytes.
>>> +In addition,
>>> +.I arg2
>>> +may include zero or more of the following flags:
>>> +.RS
>>> +.TP
>>> +.B PR_SVE_VL_INHERIT
>>> +Inherit the configured vector length across
>>> +.BR execve (2).
>>> +.TP
>>> +.B PR_SVE_SET_VL_ONEXEC
>>> +Defer the change until the next
>>> +.BR execve (2)
>>> +in this thread.
>>
>> (aside, it's weird that we didn't allocate (1<<16) for one of these flags)
> 
> The flag definitions are shared with ptrace: ptrace is the
> SVE_PT_REGS_SVE format selection bit, which isn't relevant to the prctl.
> 
> Maybe it would have made more sense to keep the definitions completely
> separate, but it's there now.
> 
>>> +If
>>> +.B PR_SVE_VL_INHERIT
>>> +is also included in
>>> +.IR arg2 ,
>>> +it takes effect
>>> +.I after
>>> +this deferred change.
>>
>> I find this a bit hard to follow, since it's not clear to me whether the
>> INHERIT flag is effectively set before or after the next execve(). In other
>> words, if both PR_SVE_SET_VL_ONEXEC and PR_SVE_VL_INHERIT are specified,
>> is the vector length preserved or reset on the next execve()?
> 
> It makes no difference, because the ONEXEC handling takes priority over
> the INHERIT handling. But either way INHERIT is never cleared by execve()
> and will apply at subsequent execs().
> 
> Explaining all this properly seems out of scope here.  Maybe this should
> be trimmed down rather than elaborated?  Or perhaps just explain it in
> terms of what the kernel does instead of futile attempts to make it
> intuitive?
> 
> Ultimately I'll probably write a separate page or pages for SVE and other
> arm64 specifics.

(okay.)

>>> +.RE
>>> +.IP
>>> +On success, the vector length and flags are set as requested,
>>> +and any deferred change that was pending immediately before the
>>> +.B PR_SVE_SET_VL
>>> +call is canceled.
>>
>> Huh, turns out 'canceled' is a valid US spelling. Fair enough, but it looks
>> wrong to me ;)
> 
> Yeah, I know, but the man pages do have a documented policy on this...
> 
> prctl.2 has a lot of authors, so having mixed spellings could get
> particularly messy.

Quite. Indeed, that was how things were when I took over as
maintainer: a hodge-podge of British and American spellings,
occasionally even in the same page. I decided we needed
consistency, and though American is not my native spelling,
it seemed the most appropriate convention.

>>
>>> +If
>>> +.B PR_SVE_SET_VL_ONEXEC
>>> +was included in
>>> +.IR arg2 ,
>>> +the returned value describes the configuration
>>> +scheduled to take effect at the next
>>> +.BR execve (2).
>>
>> "describes the configuration" how?
>>
>>> +Otherwise, the effect is immediate and
>>> +the returned value describes the new configuration.
>>> +The returned value is encoded in the same way as the return value of
>>> +.BR PR_SVE_GET_VL .
>>
>> Aha. Maybe move this bit up slightly?
> 
> Yes, I'll reorder that.
> 
>>
>>> +.IP
>>> +If neither of the above flags is included in
>>
>> are included
> 
> Debatable.
> 
> The subject of the verb here is not "flags" (plural), but "neither of
> the above flags" (which is more nuanced, though it can be interpreted
> as singular).  Usage varies, and I don't consider this wrong.

As far as I know, the grammarians are with you on this one,
Dave, and if I was writing carefully, I'd do the same as you.
But, the plural here is also frequent (and so common that I would
hesitate to call it "wrong").

[...]


Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/