On Wed, Jul 28, 2021 at 01:59:18PM +0100, Mark Brown wrote:
On Wed, Jul 28, 2021 at 10:41:58AM +0100, Dave Martin wrote:
On Tue, Jul 27, 2021 at 07:06:49PM +0100, Mark Brown wrote:
We provide interfaces for configuring the SVE vector length seen by processes using prctl and also via /proc for configuring the default values. Provide tests that exercise all these interfaces and verify that they take effect as expected, though at present no test fully enumerates all the possible vector lengths.
Does "at present" mean that this test doesn't do it either?
(It doesn't seem to try every VL, unless I've missed something? Is this planned?)
Nothing currently does it, and nor does this patch. Planned is a strong term but yes, ideally we should probe all the VLs.
+#include <stddef.h> +#include <stdio.h> +#include <stdlib.h>
Not used? ^
We call exit() which is declared in stdlib.h.
Ignore me, I confused exit() with _exit().
+#define MIN_VL 16
<asm/sigcontext.h> has SVE_MIN_VL. Maybe we can use that everywhere these days?
I partly wanted the vector type neutral name, and I'm considering modifying the sysfs ABI file to define 0 as a valid vector length for consistency with accepting -1 as the maximum since SME doesn't have any architected guarantees as to which particular vector lengths are defined.
I see what you mean, but it is more than mere coincidence that this is the same value as SVE_MIN_VL.
You could view SVE as defining the base architecture which SME then extends.
Perhaps
#define ARCH_MIN_VL SVE_MIN_VL /* architectural minimim VL */
would be neutral enough. Anyway, I won't lose sleep over it.
+/* Verify that we can write a minimum value and have it take effect */ +void proc_write_min(struct vec_data *data)
Could be proc_write_check_min() (though the "check" is a bit more redundant here; from "write" it's clear that this function actually does something nontrivial).
TBH I'm not sure people will be excssively confused by the idea that a test would validate the values it was trying to read or write were accurate.
It's not 100% clear that this is a test until one has read all the way to the end of the file. But now that I understand the pattern here I wouldn't be too concerned, and the comment accurately describes what the function does.
+/* Can we read back a VL from prctl? */
It's certainly possible.
The comment is describing what the test is verifying.
Ignore that, I somehow read the comment as something like
[TODO] Can we read back a VL via ptrace?
which is not what the comment says.
Since this would test different kernel paths from getting the child itself to do RVDL / PR_SVE_GET_VL, it would be a different test though. I think this diff is still good, but beefing up the ptrace tests to do the appropriate checks would be good too (if we don't have that already).
Yes, the ptrace stuff could have a bit more coverage.
- proc_write_min,
- proc_write_max,
Can we also check what happens when writing unsupported values here?
We could.
If this patch is more about establishing the framework, these could be TODOs for now.
It definitely feels like something we can do incrementally.
Is it worth committing a TODO list somewhere? There's always the possibility that someone else gets interested and contributes some tests for us; otherwise, at least it makes it harder to forget them.
Can we be a good citizen and restore sve_default_vector_length to its original value?
We do that in the tests that fiddle with the default vector length, it seems useful to keep it at a value different from min and max as much as possible to increase the chance that we notice a failure to set things.
Ah right, I hadn't understood that proc_read_default() reads the default and then the subsequent tests write it back.
This might be a bit clearer if the setup code was clearly separate from the tests, but so long as the ordering requirements are clearly documented that seems reasonably OK.
In:
+test_type tests[] = {
- /*
* The default/min/max tests must be first to provide data for* other tests.*/- proc_read_default,
- proc_write_min,
- proc_write_max,
can you also comment that proc_read_default needs to come first among these?
- prctl_get,
- prctl_set,
- prctl_set_no_child,
- prctl_set_for_child,
- prctl_set_onexec,
+};
[...]
Cheers ---Dave