powerpc: Support auxilliary vector components for cache geometries

Submitted by Paul Clarke on March 9, 2017, 3:57 p.m.

Details

Message ID 09eaab05-8cfe-a800-a033-750a02e417a5@us.ibm.com
State New
Headers show

Commit Message

Paul Clarke March 9, 2017, 3:57 p.m.
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

Mike Frysinger March 12, 2017, 2:01 a.m.
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
Paul Clarke March 13, 2017, 2:20 a.m.
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
Florian Weimer March 30, 2017, 8:14 p.m.
* 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.
Paul Clarke March 31, 2017, 3:24 p.m.
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
Florian Weimer March 31, 2017, 4:52 p.m.
* 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.
Paul Clarke April 6, 2017, 3:49 a.m.
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
Florian Weimer April 6, 2017, 5:33 a.m.
* 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.

Patch hide | download patch | download mbox

diff --git a/elf/elf.h b/elf/elf.h
index 6d3b356..fff893d 100644
--- a/elf/elf.h
+++ b/elf/elf.h
@@ -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.  */
  
diff --git a/sysdeps/powerpc/Makefile b/sysdeps/powerpc/Makefile
index 933810f..5f10f68 100644
--- a/sysdeps/powerpc/Makefile
+++ b/sysdeps/powerpc/Makefile
@@ -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)))
diff --git a/sysdeps/powerpc/tst-getauxval.c b/sysdeps/powerpc/tst-getauxval.c
new file mode 100644
index 0000000..1f5e9f0
--- /dev/null
+++ b/sysdeps/powerpc/tst-getauxval.c
@@ -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>