Re: [PATCH] powerpc: Support auxilliary vector components for cache geometries

[Paul Clarke <pc at us dot ibm dot com>]

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