arm64/sve: Document that __SVE_VQ_MAX is much larger than needed

  __SVE_VQ_MAX is defined without comment as 512 but the actual
architectural maximum is 16, a substantial difference which might not
be obvious to readers especially given the several different units used
for specifying vector sizes in various contexts and the fact that it's
often used via macros.  In an effort to minimise surprises for users who
might assume the value is the architectural maximum and use it to do
things like size allocations add a comment noting the difference.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/uapi/asm/sve_context.h | 4 ++++
 1 file changed, 4 insertions(+)


---
base-commit: 54be6c6c5ae8e0d93a6c4641cb7528eb0b6ba478
change-id: 20240206-arm64-sve-vl-max-comment-64efa3f03625

Best regards,
  

On Tue, Feb 06, 2024 at 04:27:01PM +0000, Mark Brown wrote:
> __SVE_VQ_MAX is defined without comment as 512 but the actual
> architectural maximum is 16, a substantial difference which might not
> be obvious to readers especially given the several different units used
> for specifying vector sizes in various contexts and the fact that it's
> often used via macros.  In an effort to minimise surprises for users who
> might assume the value is the architectural maximum and use it to do
> things like size allocations add a comment noting the difference.

Well, the value 512 was semi-deliberately chosen to be surprising.

But the point about units is valid: to the casual reader, "512" does
look suspiciously like a bit count when it really really isn't...

> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/uapi/asm/sve_context.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/include/uapi/asm/sve_context.h b/arch/arm64/include/uapi/asm/sve_context.h
> index 754ab751b523..59f283f373a6 100644
> --- a/arch/arm64/include/uapi/asm/sve_context.h
> +++ b/arch/arm64/include/uapi/asm/sve_context.h
> @@ -13,6 +13,10 @@
>  
>  #define __SVE_VQ_BYTES		16	/* number of bytes per quadword */
>  
> +/*
> + * Note that for future proofing __SVE_VQ_MAX is defined much larger
> + * than the actual architecture maximum of 16.
> + */

I think that putting shadow #defines in comments in UAPI headers is a
really bad idea...  is this a normative statement about the user API,
or what?

My concern is that if we muddy the waters here different bits of
software will do different things and we will get a mess with no
advantages.

Portability issues may ensue if userspace software feels it can
substitute some other value for this constant, since we can't control
what userspace uses it for.

>  #define __SVE_VQ_MIN		1

Would it be sufficient to say something like:

/*
 * Yes, this is 512 QUADWORDS.
 * Never allocate memory or size structures based on the value of this
 * constant.
 */
>  #define __SVE_VQ_MAX		512

Though comments might be better placed alongsize SVE_VQ_MAX at al., in
ptrace.h and sigcontext.h rather than here.  The leading __ should at
least be a hint that __SVE_VQ_MAX shouldn't be used directly by
anyone...

Cheers
---Dave
  

On Wed, Feb 07, 2024 at 12:01:43PM +0000, Dave Martin wrote:
> On Tue, Feb 06, 2024 at 04:27:01PM +0000, Mark Brown wrote:

> > +/*
> > + * Note that for future proofing __SVE_VQ_MAX is defined much larger
> > + * than the actual architecture maximum of 16.
> > + */

> I think that putting shadow #defines in comments in UAPI headers is a
> really bad idea...  is this a normative statement about the user API,
> or what?

Well, the only reason I'm mentioning the constant here is that
__SVE_VQ_MIN is defined too and has a perfectly good value, things look
a bit neater with a shared comment block.  I'm not sure there's a hugely
meaingful difference between having a comment adjacent to a named
constant in a header and one a couple of lines away that mentions the
constant by name.

> My concern is that if we muddy the waters here different bits of
> software will do different things and we will get a mess with no
> advantages.

> Portability issues may ensue if userspace software feels it can
> substitute some other value for this constant, since we can't control
> what userspace uses it for.

I don't think we want people using this at all, ideally we'd remove it
but it's in the uapi.

> Would it be sufficient to say something like:

> /*
>  * Yes, this is 512 QUADWORDS.
>  * Never allocate memory or size structures based on the value of this
>  * constant.
>  */
> >  #define __SVE_VQ_MAX		512

I think the fact that this vector length is more than an order of
magnitude more than is architecturally supported at present needs to be
conveyed, it's perfectly reasonable for people to not want to do dynamic
allocation/sizing of buffers in some applications and the above sounds
more like stylistic guidance about using dynamic sizing to improve
memory usage.

> Though comments might be better placed alongsize SVE_VQ_MAX at al., in
> ptrace.h and sigcontext.h rather than here.  The leading __ should at
> least be a hint that __SVE_VQ_MAX shouldn't be used directly by
> anyone...

Yeah, the multiple layers of indirection aren't helpful here.  We
probably need to comment it in both places TBH.
  

On Wed, Feb 07, 2024 at 12:48:58PM +0000, Mark Brown wrote:
> On Wed, Feb 07, 2024 at 12:01:43PM +0000, Dave Martin wrote:
> > On Tue, Feb 06, 2024 at 04:27:01PM +0000, Mark Brown wrote:
> 
> > > +/*
> > > + * Note that for future proofing __SVE_VQ_MAX is defined much larger
> > > + * than the actual architecture maximum of 16.
> > > + */
> 
> > I think that putting shadow #defines in comments in UAPI headers is a
> > really bad idea...  is this a normative statement about the user API,
> > or what?
> 
> Well, the only reason I'm mentioning the constant here is that
> __SVE_VQ_MIN is defined too and has a perfectly good value, things look
> a bit neater with a shared comment block.  I'm not sure there's a hugely
> meaingful difference between having a comment adjacent to a named
> constant in a header and one a couple of lines away that mentions the
> constant by name.

It wasn't so much the exact location that concerned me, rather putting
it in a UAPI header at all.

Maybe so long as the comment doesn't quote a literal value for the arch
max VQ that would be better.  If there is a value there, we may be kind
of legitimising its use even if it's in a comment...

> 
> > My concern is that if we muddy the waters here different bits of
> > software will do different things and we will get a mess with no
> > advantages.
> 
> > Portability issues may ensue if userspace software feels it can
> > substitute some other value for this constant, since we can't control
> > what userspace uses it for.
> 
> I don't think we want people using this at all, ideally we'd remove it
> but it's in the uapi.

I think the main legitimate uses are for implementing sve_vl_valid() and
for type selection purposes (analogous to the C <limits.h> constants --
though all the "obvious" types are fine so this is a but redundant).
But yeah, it's there now.

> > Would it be sufficient to say something like:
> 
> > /*
> >  * Yes, this is 512 QUADWORDS.
> >  * Never allocate memory or size structures based on the value of this
> >  * constant.
> >  */
> > >  #define __SVE_VQ_MAX		512
> 
> I think the fact that this vector length is more than an order of
> magnitude more than is architecturally supported at present needs to be
> conveyed, it's perfectly reasonable for people to not want to do dynamic
> allocation/sizing of buffers in some applications and the above sounds
> more like stylistic guidance about using dynamic sizing to improve
> memory usage.

I guess that's true; people need to know that they'll be allocating a
silly amount of memory if they use the existing _MAX constants directly.
Laziness is a perfectly good reason for doing this for development hacks
that aren't going to be published, less so for code that ends up in
libraries or otherwise gets into the wild...

I preferred to encourage people to size dynamically, but we don't have
a way to enforce it.

Ideally there would be a direct way to read out the system-wide max VL
to provide userspace with a sensible default allocation size, but that
doesn't really exist today (though ptrace and PR_SVE_{SET,GEL}_VL
provide ways to find out, but it's a bit grungy).

How about something along the lines of:

/*
 * Yes, this is 512 QUADWORDS.
 * To help ensure forward portability, this is much larger than the
 * current maximum value defined by the SVE architecture.
 * While arrays or static allocations can be sized based on this value,
 * watch out!  It will waste a surprisingly large amount of memory.
 * Dynamic sizing based on the actual runtime vector length is likely to
 * be preferable for most purposes.
 */

> 
> > Though comments might be better placed alongsize SVE_VQ_MAX at al., in
> > ptrace.h and sigcontext.h rather than here.  The leading __ should at
> > least be a hint that __SVE_VQ_MAX shouldn't be used directly by
> > anyone...
> 
> Yeah, the multiple layers of indirection aren't helpful here.  We
> probably need to comment it in both places TBH.

Agreed, part of that came from a desire to avoid duplicating
information.  I think the indirection via sve_context.h was introduced
to work around a bad interaction with user C library headers, but I'm a
bit hazy on that now without digging through the history.

Cheers
---Dave
  

On Wed, Feb 07, 2024 at 01:39:13PM +0000, Dave Martin wrote:

> How about something along the lines of:

> /*
>  * Yes, this is 512 QUADWORDS.
>  * To help ensure forward portability, this is much larger than the
>  * current maximum value defined by the SVE architecture.
>  * While arrays or static allocations can be sized based on this value,
>  * watch out!  It will waste a surprisingly large amount of memory.
>  * Dynamic sizing based on the actual runtime vector length is likely to
>  * be preferable for most purposes.
>  */

That works for me.  The cost of initialising can also add up in
emulation.