| Message ID | 20240206-arm64-sve-vl-max-comment-v1-1-dddf16414412@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/sve: Document that __SVE_VQ_MAX is much larger than needed | expand |
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.
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. + */ #define __SVE_VQ_MIN 1 #define __SVE_VQ_MAX 512
__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,