powerpc: Support auxilliary vector components for cache geometries
Commit Message
Use getauxval() to get L1, L2, L3 cache sizes, cache line sizes, and
cache associativities. The new types for getauxval() were added in
the stream for Linux kernel v4.11 in commit
98a5f361b8625c6f4841d6ba013bbf0e80d08147.
Add test case which retrieves above values using getauxval().
* elf/elf.h: Add auxvec identifiers from kernel
arch/powerpc/include/uapi/asm/auxvec.h.
* glibc/sysdeps/powerpc/tst-getauxval.c: New test to
retrieve new auxvec values.
* glibc/sysdeps/powerpc/Makefile (tests): Add tst-getauxval.
---
elf/elf.h | 12 ++++++
sysdeps/powerpc/Makefile | 1 +
sysdeps/powerpc/tst-getauxval.c | 86 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 99 insertions(+)
create mode 100644 sysdeps/powerpc/tst-getauxval.c
Comments
On 09 Mar 2017 09:57, Paul Clarke wrote:
> Use getauxval() to get L1, L2, L3 cache sizes, cache line sizes, and
> cache associativities. The new types for getauxval() were added in
> the stream for Linux kernel v4.11 in commit
> 98a5f361b8625c6f4841d6ba013bbf0e80d08147.
>
> Add test case which retrieves above values using getauxval().
>
> * elf/elf.h: Add auxvec identifiers from kernel
> arch/powerpc/include/uapi/asm/auxvec.h.
> * glibc/sysdeps/powerpc/tst-getauxval.c: New test to
> retrieve new auxvec values.
> * glibc/sysdeps/powerpc/Makefile (tests): Add tst-getauxval.
the defines have been added to the common elf/elf.h. and the way
you've written the test is not arch specific. so should it live
in the common code paths instead of under powerpc/ ?
also, there is no "glibc/" subdir.
> +static int
> +do_test (void)
> +{
> + int rc = 0;
> + unsigned long val;
> + val = getauxval (AT_L1I_CACHESIZE);
> + if (val)
> + printf("AT_L1I_CACHESIZE: %ld (0x%lx)\n",val,val);
bad whitespace/style here. this comes up multiple times.
also, you want %lu, not %ld, with val. this comes up multiple times.
> + val = getauxval (AT_L1I_CACHEGEOMETRY);
> + if (val)
> + printf("AT_L1I_CACHEGEOMETRY: associativity %ld; line size %ld\n",
> + (val & 0xffff0000) >> 16, val & 0x0000ffff);
> + else
> + rc = EXIT_UNSUPPORTED;
honestly, what is the value of this test ? you basically just
printf the values everywhere, or you exit unsupported. there is
no actual "test" here that i can see as you don't validate the
results anywhere.
-mike
On 03/11/2017 08:01 PM, Mike Frysinger wrote:
> On 09 Mar 2017 09:57, Paul Clarke wrote:
>> Use getauxval() to get L1, L2, L3 cache sizes, cache line sizes, and
>> cache associativities. The new types for getauxval() were added in
>> the stream for Linux kernel v4.11 in commit
>> 98a5f361b8625c6f4841d6ba013bbf0e80d08147.
>>
>> Add test case which retrieves above values using getauxval().
>>
>> * elf/elf.h: Add auxvec identifiers from kernel
>> arch/powerpc/include/uapi/asm/auxvec.h.
>> * glibc/sysdeps/powerpc/tst-getauxval.c: New test to
>> retrieve new auxvec values.
>> * glibc/sysdeps/powerpc/Makefile (tests): Add tst-getauxval.
>
> the defines have been added to the common elf/elf.h. and the way
> you've written the test is not arch specific. so should it live
> in the common code paths instead of under powerpc/ ?
The support on the kernel side is arch specific. I didn't see a good reason to hide the new constants, though. They should probably be reserved across arches, though, as the new queries allow for larger cache sizes and could be readily adopted by other arches as needed.
> also, there is no "glibc/" subdir.
cut-and-paste error on my part. Thanks for the attention to detail!
>> +static int
>> +do_test (void)
>> +{
>> + int rc = 0;
>> + unsigned long val;
>> + val = getauxval (AT_L1I_CACHESIZE);
>> + if (val)
>> + printf("AT_L1I_CACHESIZE: %ld (0x%lx)\n",val,val);
>
> bad whitespace/style here. this comes up multiple times.
I presume space before parentheses, and after commas. Thanks!
> also, you want %lu, not %ld, with val. this comes up multiple times.
Yep, my mistakes. Will clean up and repost, subject to resolving the final issue, below...
>> + val = getauxval (AT_L1I_CACHEGEOMETRY);
>> + if (val)
>> + printf("AT_L1I_CACHEGEOMETRY: associativity %ld; line size %ld\n",
>> + (val & 0xffff0000) >> 16, val & 0x0000ffff);
>> + else
>> + rc = EXIT_UNSUPPORTED;
>
> honestly, what is the value of this test ? you basically just
> printf the values everywhere, or you exit unsupported. there is
> no actual "test" here that i can see as you don't validate the
> results anywhere.
I debated this with colleagues before sending (and perhaps should've deferred to their experience). I was reluctant to add new code without exercising it, at least a successful compile and run. However it's difficult to determine a true "failure" case without knowing too much about the kernel. I also like that it provides an example of use. If those reasons are not sufficient, I can also remove it from the patch.
Regards,
Paul Clarke
* Paul Clarke:
>>> + val = getauxval (AT_L1I_CACHEGEOMETRY);
>>> + if (val)
>>> + printf("AT_L1I_CACHEGEOMETRY: associativity %ld; line size %ld\n",
>>> + (val & 0xffff0000) >> 16, val & 0x0000ffff);
>>> + else
>>> + rc = EXIT_UNSUPPORTED;
>>
>> honestly, what is the value of this test ? you basically just
>> printf the values everywhere, or you exit unsupported. there is
>> no actual "test" here that i can see as you don't validate the
>> results anywhere.
>
> I debated this with colleagues before sending (and perhaps should've
> deferred to their experience). I was reluctant to add new code
> without exercising it, at least a successful compile and run. However
> it's difficult to determine a true "failure" case without knowing too
> much about the kernel. I also like that it provides an example of
> use. If those reasons are not sufficient, I can also remove it from
> the patch.
Can you at least add consistency checks which check that the values
use the right endianess? I think that part could be useful.
On 03/30/2017 03:14 PM, Florian Weimer wrote:
> * Paul Clarke:
>>>> + val = getauxval (AT_L1I_CACHEGEOMETRY);
>>>> + if (val)
>>>> + printf("AT_L1I_CACHEGEOMETRY: associativity %ld; line size %ld\n",
>>>> + (val & 0xffff0000) >> 16, val & 0x0000ffff);
>>>> + else
>>>> + rc = EXIT_UNSUPPORTED;
>>>
>>> honestly, what is the value of this test ? you basically just
>>> printf the values everywhere, or you exit unsupported. there is
>>> no actual "test" here that i can see as you don't validate the
>>> results anywhere.
>>
>> I debated this with colleagues before sending (and perhaps should've
>> deferred to their experience). I was reluctant to add new code
>> without exercising it, at least a successful compile and run. However
>> it's difficult to determine a true "failure" case without knowing too
>> much about the kernel. I also like that it provides an example of
>> use. If those reasons are not sufficient, I can also remove it from
>> the patch.
>
> Can you at least add consistency checks which check that the values
> use the right endianess? I think that part could be useful.
That seems error-prone. Given "all possibilities", the values returned in the lowest-order and next-to-lowest-order 16 bits of a long return value from getauxval() are basically arbitrary. I'm not sure how one could determine correct endianness of an arbitrary value. What values are guaranteed correct or incorrect for associativity and cache line size?
Perhaps:
cachelinesize > cachesize?
associativity > cachesize?
Those are certainly not catch-all.
I'm fine dropping the test completely (as I did in v2) if it won't have sufficient value.
Regards,
PC
* Paul Clarke:
> On 03/30/2017 03:14 PM, Florian Weimer wrote:
>> * Paul Clarke:
>>>>> + val = getauxval (AT_L1I_CACHEGEOMETRY);
>>>>> + if (val)
>>>>> + printf("AT_L1I_CACHEGEOMETRY: associativity %ld; line size %ld\n",
>>>>> + (val & 0xffff0000) >> 16, val & 0x0000ffff);
>>>>> + else
>>>>> + rc = EXIT_UNSUPPORTED;
>>>>
>>>> honestly, what is the value of this test ? you basically just
>>>> printf the values everywhere, or you exit unsupported. there is
>>>> no actual "test" here that i can see as you don't validate the
>>>> results anywhere.
>>>
>>> I debated this with colleagues before sending (and perhaps should've
>>> deferred to their experience). I was reluctant to add new code
>>> without exercising it, at least a successful compile and run. However
>>> it's difficult to determine a true "failure" case without knowing too
>>> much about the kernel. I also like that it provides an example of
>>> use. If those reasons are not sufficient, I can also remove it from
>>> the patch.
>>
>> Can you at least add consistency checks which check that the values
>> use the right endianess? I think that part could be useful.
>
> That seems error-prone. Given "all possibilities", the values
> returned in the lowest-order and next-to-lowest-order 16 bits of a
> long return value from getauxval() are basically arbitrary. I'm not
> sure how one could determine correct endianness of an arbitrary value.
> What values are guaranteed correct or incorrect for associativity and
> cache line size?
I think for 64-bit at least, a byte-swapped return value might be
larger than UINT_MAX. Not sure if it is worth detecting this.
On 03/31/2017 11:52 AM, Florian Weimer wrote:
> * Paul Clarke:
>
>> On 03/30/2017 03:14 PM, Florian Weimer wrote:
>>> * Paul Clarke:
>>>>>> + val = getauxval (AT_L1I_CACHEGEOMETRY);
>>>>>> + if (val)
>>>>>> + printf("AT_L1I_CACHEGEOMETRY: associativity %ld; line size %ld\n",
>>>>>> + (val & 0xffff0000) >> 16, val & 0x0000ffff);
>>>>>> + else
>>>>>> + rc = EXIT_UNSUPPORTED;
>>>>>
>>>>> honestly, what is the value of this test ? you basically just
>>>>> printf the values everywhere, or you exit unsupported. there is
>>>>> no actual "test" here that i can see as you don't validate the
>>>>> results anywhere.
>>>>
>>>> I debated this with colleagues before sending (and perhaps should've
>>>> deferred to their experience). I was reluctant to add new code
>>>> without exercising it, at least a successful compile and run. However
>>>> it's difficult to determine a true "failure" case without knowing too
>>>> much about the kernel. I also like that it provides an example of
>>>> use. If those reasons are not sufficient, I can also remove it from
>>>> the patch.
>>>
>>> Can you at least add consistency checks which check that the values
>>> use the right endianess? I think that part could be useful.
>>
>> That seems error-prone. Given "all possibilities", the values
>> returned in the lowest-order and next-to-lowest-order 16 bits of a
>> long return value from getauxval() are basically arbitrary. I'm not
>> sure how one could determine correct endianness of an arbitrary value.
>> What values are guaranteed correct or incorrect for associativity and
>> cache line size?
>
> I think for 64-bit at least, a byte-swapped return value might be
> larger than UINT_MAX. Not sure if it is worth detecting this.
That could catch the case where the entire 64-bit value is not native endianness, but not if just the 16-bit fields are non-native.
Florian, I share your concerns about whether a sufficiently valuable test case can be created with reasonable effort. Any objection to just skipping a test case here?
PC
* Paul Clarke:
> Florian, I share your concerns about whether a sufficiently valuable
> test case can be created with reasonable effort. Any objection to
> just skipping a test case here?
Okay, let's drop the test case.
@@ -1170,6 +1170,18 @@ typedef struct
#define AT_L2_CACHESHAPE 36
#define AT_L3_CACHESHAPE 37
+/* Shapes of the caches, with more room to describe them.
+ *GEOMETRY are comprised of cache line size in bytes in the bottom 16 bits
+ and the cache associativity in the next 16 bits. */
+#define AT_L1I_CACHESIZE 40
+#define AT_L1I_CACHEGEOMETRY 41
+#define AT_L1D_CACHESIZE 42
+#define AT_L1D_CACHEGEOMETRY 43
+#define AT_L2_CACHESIZE 44
+#define AT_L2_CACHEGEOMETRY 45
+#define AT_L3_CACHESIZE 46
+#define AT_L3_CACHEGEOMETRY 47
+
/* Note section contents. Each entry in the note section begins with
a header of a fixed form. */
@@ -35,6 +35,7 @@ ifeq ($(subdir),misc)
sysdep_headers += sys/platform/ppc.h
tests += test-gettimebase
tests += tst-set_ppr
+tests += tst-getauxval
endif
ifneq (,$(filter %le,$(config-machine)))
new file mode 100644
@@ -0,0 +1,86 @@
+/* Test the implementation of getauxval functions for
+ cache size, cache line size, and associativity for L1, L2, L3 caches.
+
+ Copyright (C) 2017 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include <sys/auxv.h>
+
+#include <support/test-driver.h>
+
+static int
+do_test (void)
+{
+ int rc = 0;
+ unsigned long val;
+ val = getauxval (AT_L1I_CACHESIZE);
+ if (val)
+ printf("AT_L1I_CACHESIZE: %ld (0x%lx)\n",val,val);
+ else
+ rc = EXIT_UNSUPPORTED;
+
+ val = getauxval (AT_L1I_CACHEGEOMETRY);
+ if (val)
+ printf("AT_L1I_CACHEGEOMETRY: associativity %ld; line size %ld\n",
+ (val & 0xffff0000) >> 16, val & 0x0000ffff);
+ else
+ rc = EXIT_UNSUPPORTED;
+
+ val = getauxval (AT_L1D_CACHESIZE);
+ if (val)
+ printf("AT_L1D_CACHESIZE: %ld (0x%lx)\n",val,val);
+ else
+ rc = EXIT_UNSUPPORTED;
+
+ val = getauxval (AT_L1D_CACHEGEOMETRY);
+ if (val)
+ printf("AT_L1D_CACHEGEOMETRY: associativity %ld; line size %ld\n",
+ (val & 0xffff0000) >> 16, val & 0x0000ffff);
+ else
+ rc = EXIT_UNSUPPORTED;
+
+ val = getauxval (AT_L2_CACHESIZE);
+ if (val)
+ printf("AT_L2_CACHESIZE: %ld (0x%lx)\n",val,val);
+ else
+ rc = EXIT_UNSUPPORTED;
+
+ val = getauxval (AT_L2_CACHEGEOMETRY);
+ if (val)
+ printf("AT_L2_CACHEGEOMETRY: associativity %ld; line size %ld\n",
+ (val & 0xffff0000) >> 16, val & 0x0000ffff);
+ else
+ rc = EXIT_UNSUPPORTED;
+
+ val = getauxval (AT_L3_CACHESIZE);
+ if (val)
+ printf("AT_L3_CACHESIZE: %ld (0x%lx)\n",val,val);
+ else
+ rc = EXIT_UNSUPPORTED;
+
+ val = getauxval (AT_L3_CACHEGEOMETRY);
+ if (val)
+ printf("AT_L3_CACHEGEOMETRY: associativity %ld; line size %ld\n",
+ (val & 0xffff0000) >> 16, val & 0x0000ffff);
+ else
+ rc = EXIT_UNSUPPORTED;
+
+ return rc;
+}
+
+#include <support/test-driver.c>