diff mbox

powerpc: fix sysconf support for cache geometries

Message ID 3a5ed534-95d2-4d66-533e-aaed56134344@us.ibm.com
State New
Headers show

Commit Message

Paul A. Clarke June 15, 2017, 8:45 p.m. UTC
Commit cdfbe5037f2f67bf5f560b73732b69d0fabe2314 added sysconf support
for cache geometries on powerpc, but mishandled errno.  For valid input
parameters, sysconf() should not set errno.

2017-06-15  Paul A. Clarke  <pc@us.ibm.com>

	* sysdeps/unix/sysv/linux/powerpc/sysconf.c: Remove references
	to errno, and simplify remaining related code.
---
 sysdeps/unix/sysv/linux/powerpc/sysconf.c | 37 +++++++------------------------
 1 file changed, 8 insertions(+), 29 deletions(-)

Comments

Adhemerval Zanella June 15, 2017, 8:55 p.m. UTC | #1
On 15/06/2017 17:45, Paul Clarke wrote:
> Commit cdfbe5037f2f67bf5f560b73732b69d0fabe2314 added sysconf support
> for cache geometries on powerpc, but mishandled errno.  For valid input
> parameters, sysconf() should not set errno.

Do we have a testcase for it? If not I think we should add it.

> 
> 2017-06-15  Paul A. Clarke  <pc@us.ibm.com>
> 
> 	* sysdeps/unix/sysv/linux/powerpc/sysconf.c: Remove references
> 	to errno, and simplify remaining related code.
> ---
>  sysdeps/unix/sysv/linux/powerpc/sysconf.c | 37 +++++++------------------------
>  1 file changed, 8 insertions(+), 29 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/powerpc/sysconf.c b/sysdeps/unix/sysv/linux/powerpc/sysconf.c
> index 10c4aa0..7bed701 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/sysconf.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/sysconf.c
> @@ -22,37 +22,16 @@
>  
>  static long linux_sysconf (int name);
>  
> -static long
> -auxv2sysconf (unsigned long type)
> -{
> -  long rc;
> -  rc = __getauxval (type);
> -  if (rc == 0)
> -  {
> -    __set_errno (EINVAL);
> -    rc = -1;
> -  }
> -  return rc;
> -}
> -
> -static long
> +static inline long
>  auxv2sysconf_cache_associativity (unsigned long type)
>  {
> -  long rc;
> -  rc = auxv2sysconf (type);
> -  if (rc != -1)
> -    rc = (rc & 0xffff0000) >> 16;
> -  return rc;
> +  return (__getauxval (type) & 0xffff0000) >> 16;
>  }
>  
> -static long
> +static inline long
>  auxv2sysconf_cache_linesize (unsigned long type)
>  {
> -  long rc;
> -  rc = auxv2sysconf (type);
> -  if (rc != -1)
> -    rc = rc & 0xffff;
> -  return rc;
> +  return __getauxval (type) & 0xffff;
>  }
>  
>  /* Get the value of the system variable NAME.  */
> @@ -62,25 +41,25 @@ __sysconf (int name)
>    switch (name)
>      {
>        case _SC_LEVEL1_ICACHE_SIZE:
> -	return auxv2sysconf (AT_L1I_CACHESIZE);
> +	return __getauxval (AT_L1I_CACHESIZE);
>        case _SC_LEVEL1_ICACHE_ASSOC:
>  	return auxv2sysconf_cache_associativity (AT_L1I_CACHEGEOMETRY);
>        case _SC_LEVEL1_ICACHE_LINESIZE:
>  	return auxv2sysconf_cache_linesize (AT_L1I_CACHEGEOMETRY);
>        case _SC_LEVEL1_DCACHE_SIZE:
> -	return auxv2sysconf (AT_L1D_CACHESIZE);
> +	return __getauxval (AT_L1D_CACHESIZE);
>        case _SC_LEVEL1_DCACHE_ASSOC:
>  	return auxv2sysconf_cache_associativity (AT_L1D_CACHEGEOMETRY);
>        case _SC_LEVEL1_DCACHE_LINESIZE:
>  	return auxv2sysconf_cache_linesize (AT_L1D_CACHEGEOMETRY);
>        case _SC_LEVEL2_CACHE_SIZE:
> -	return auxv2sysconf (AT_L2_CACHESIZE);
> +	return __getauxval (AT_L2_CACHESIZE);
>        case _SC_LEVEL2_CACHE_ASSOC:
>  	return auxv2sysconf_cache_associativity (AT_L2_CACHEGEOMETRY);
>        case _SC_LEVEL2_CACHE_LINESIZE:
>  	return auxv2sysconf_cache_linesize (AT_L2_CACHEGEOMETRY);
>        case _SC_LEVEL3_CACHE_SIZE:
> -	return auxv2sysconf (AT_L3_CACHESIZE);
> +	return __getauxval (AT_L3_CACHESIZE);
>        case _SC_LEVEL3_CACHE_ASSOC:
>  	return auxv2sysconf_cache_associativity (AT_L3_CACHEGEOMETRY);
>        case _SC_LEVEL3_CACHE_LINESIZE:
>
Richard Henderson June 15, 2017, 9:03 p.m. UTC | #2
On 06/15/2017 01:45 PM, Paul Clarke wrote:
> Commit cdfbe5037f2f67bf5f560b73732b69d0fabe2314 added sysconf support
> for cache geometries on powerpc, but mishandled errno.  For valid input
> parameters, sysconf() should not set errno.
> 
> 2017-06-15  Paul A. Clarke<pc@us.ibm.com>
> 
> 	* sysdeps/unix/sysv/linux/powerpc/sysconf.c: Remove references
> 	to errno, and simplify remaining related code.
> ---
>   sysdeps/unix/sysv/linux/powerpc/sysconf.c | 37 +++++++------------------------
>   1 file changed, 8 insertions(+), 29 deletions(-)

LGTM.

Any thoughts on falling back to AT_*CACHEBSIZE for _SC_LEVEL1_*CACHE_LINESIZE 
for older kernels where AT_L1*_CACHEGEOMETRY is not present?


r~
Paul A. Clarke June 15, 2017, 11:53 p.m. UTC | #3
On 06/15/2017 04:03 PM, Richard Henderson wrote:
> On 06/15/2017 01:45 PM, Paul Clarke wrote:
> Any thoughts on falling back to AT_*CACHEBSIZE for _SC_LEVEL1_*CACHE_LINESIZE 
> for older kernels where AT_L1*_CACHEGEOMETRY is not present?

Ben'll tell you (as he told me ;-) that those aren't the same thing.  The former, "CACHEBSIZE", is the cache block size, which is the unit size for cache operations (e.g. "Cache Block Touch", "Cache Block Clear", etc.).  The latter, "CACHE_LINESIZE", is, well, the cache line size (with respect to associativity, etc.).

That being said, it's probably fine to conflate the two for older kernels and L1 only.

Did you want to respin your sysconf patch?  I notice you also included L4, which always returns 0, a value, unlike my patch, which will fail (return -1, EINVAL).  Yours may be preferred in that regard, since the "names" for the L4 queries are not completely unknown.

PC
Richard Henderson June 16, 2017, 3:07 a.m. UTC | #4
On 06/15/2017 04:53 PM, Paul Clarke wrote:
> On 06/15/2017 04:03 PM, Richard Henderson wrote:
>> On 06/15/2017 01:45 PM, Paul Clarke wrote:
>> Any thoughts on falling back to AT_*CACHEBSIZE for _SC_LEVEL1_*CACHE_LINESIZE
>> for older kernels where AT_L1*_CACHEGEOMETRY is not present?
> 
> Ben'll tell you (as he told me ;-) that those aren't the same thing.  The
> former, "CACHEBSIZE", is the cache block size, which is the unit size for
> cache operations (e.g. "Cache Block Touch", "Cache Block Clear", etc.).  The
> latter, "CACHE_LINESIZE", is, well, the cache line size (with respect to
> associativity, etc.).
Yeah, yeah, but... show me where it's different, for L1, in practice.

> Did you want to respin your sysconf patch? I notice you also included L4,
which always returns 0, a value, unlike my patch, which will fail (return -1,
EINVAL). Yours may be preferred in that regard, since the "names" for the L4
queries are not completely unknown.

-1 without EINVAL is, afaik, an indication that L4 does not exist.
Whereas 0 indicates that we don't know.

That said, I really don't care about the deep cache hierarchy, so I'm fine with 
either as a result.


r~
Paul A. Clarke June 16, 2017, noon UTC | #5
On 06/15/2017 03:55 PM, Adhemerval Zanella wrote:
> On 15/06/2017 17:45, Paul Clarke wrote:
>> Commit cdfbe5037f2f67bf5f560b73732b69d0fabe2314 added sysconf support
>> for cache geometries on powerpc, but mishandled errno.  For valid input
>> parameters, sysconf() should not set errno.
> 
> Do we have a testcase for it? If not I think we should add it.

We don't.  There are three obvious test cases for sysconf:
- ./nptl/tst-sysconf.c: ignores errno
- ./posix/tst-sysconf.c:  ignores errno
- ./sysdeps/unix/sysv/linux/powerpc/tst-sysconf.c
  Ironically (given the need for the subject patch), this correctly tests for errno changes but only for the _SC_LEVEL*, and only for powerpc.

PC
Paul A. Clarke June 16, 2017, 12:09 p.m. UTC | #6
On 06/15/2017 10:07 PM, Richard Henderson wrote:
> On 06/15/2017 04:53 PM, Paul Clarke wrote:
>> On 06/15/2017 04:03 PM, Richard Henderson wrote:
>>> On 06/15/2017 01:45 PM, Paul Clarke wrote:
>>> Any thoughts on falling back to AT_*CACHEBSIZE for _SC_LEVEL1_*CACHE_LINESIZE
>>> for older kernels where AT_L1*_CACHEGEOMETRY is not present?
>> 
>> Ben'll tell you (as he told me ;-) that those aren't the same thing.  The
>> former, "CACHEBSIZE", is the cache block size, which is the unit size for
>> cache operations (e.g. "Cache Block Touch", "Cache Block Clear", etc.).  The
>> latter, "CACHE_LINESIZE", is, well, the cache line size (with respect to
>> associativity, etc.).
> Yeah, yeah, but... show me where it's different, for L1, in practice.

Yep, I can't.

>> Did you want to respin your sysconf patch? I notice you also included L4,
> which always returns 0, a value, unlike my patch, which will fail (return -1,
> EINVAL). Yours may be preferred in that regard, since the "names" for the L4
> queries are not completely unknown.
> 
> -1 without EINVAL is, afaik, an indication that L4 does not exist.
> Whereas 0 indicates that we don't know.

No.  Per the manpage, "If name is invalid, -1 is returned, and errno is set to EINVAL.  Otherwise, the value returned is the value of the system resource and errno is not changed.  In the case of options, a positive value is returned if a queried option is available, and -1 if it is not."

With a valid "name", the return value is the value.  "options", above, has a special meaning, for "posixoptions(7)", so that phrase doesn't apply.

So, possible return values for L4 are >0 (exists) and =0 (doesn't).

> That said, I really don't care about the deep cache hierarchy, so I'm fine with 
> either as a result.

I think it's reasonable to include both of your changes:
- return the L1 cache block sizes if the L1 cache line sizes are not available
- return 0 for L4 (not -1/EINVAL)

PC
Adhemerval Zanella June 16, 2017, 1:27 p.m. UTC | #7
On 16/06/2017 09:00, Paul Clarke wrote:
> On 06/15/2017 03:55 PM, Adhemerval Zanella wrote:
>> On 15/06/2017 17:45, Paul Clarke wrote:
>>> Commit cdfbe5037f2f67bf5f560b73732b69d0fabe2314 added sysconf support
>>> for cache geometries on powerpc, but mishandled errno.  For valid input
>>> parameters, sysconf() should not set errno.
>>
>> Do we have a testcase for it? If not I think we should add it.
> 
> We don't.  There are three obvious test cases for sysconf:
> - ./nptl/tst-sysconf.c: ignores errno
> - ./posix/tst-sysconf.c:  ignores errno
> - ./sysdeps/unix/sysv/linux/powerpc/tst-sysconf.c
>   Ironically (given the need for the subject patch), this correctly tests for errno changes but only for the _SC_LEVEL*, and only for powerpc.
> 
> PC
> 

I think you might have a staged change in your repo, I do not see
the powerpc specific one in master.  In any case, I think it worth
to add some errno testing on nptl and posix tests (maybe even
incorporate nptl on posix) and add powerpc one (be aware to avoid
add a testcase with same name in same folder, as this powerpc seems
to be doing).
Tulio Magno Quites Machado Filho June 16, 2017, 1:36 p.m. UTC | #8
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> [ text/plain ]
>
>
> On 16/06/2017 09:00, Paul Clarke wrote:
>> On 06/15/2017 03:55 PM, Adhemerval Zanella wrote:
>>> On 15/06/2017 17:45, Paul Clarke wrote:
>>>> Commit cdfbe5037f2f67bf5f560b73732b69d0fabe2314 added sysconf support
>>>> for cache geometries on powerpc, but mishandled errno.  For valid input
>>>> parameters, sysconf() should not set errno.
>>>
>>> Do we have a testcase for it? If not I think we should add it.
>> 
>> We don't.  There are three obvious test cases for sysconf:
>> - ./nptl/tst-sysconf.c: ignores errno
>> - ./posix/tst-sysconf.c:  ignores errno
>> - ./sysdeps/unix/sysv/linux/powerpc/tst-sysconf.c
>>   Ironically (given the need for the subject patch), this correctly tests for errno changes but only for the _SC_LEVEL*, and only for powerpc.
>
> I think you might have a staged change in your repo, I do not see
> the powerpc specific one in master.  In any case, I think it worth
> to add some errno testing on nptl and posix tests (maybe even
> incorporate nptl on posix) and add powerpc one (be aware to avoid
> add a testcase with same name in same folder, as this powerpc seems
> to be doing).

I guess Paul meant to say
sysdeps/unix/sysv/linux/powerpc/test-powerpc-linux-sysconf.c, which is the
new name of the test after solving the name clash issue.  ;-)
Adhemerval Zanella June 16, 2017, 1:40 p.m. UTC | #9
On 16/06/2017 10:36, Tulio Magno Quites Machado Filho wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> [ text/plain ]
>>
>>
>> On 16/06/2017 09:00, Paul Clarke wrote:
>>> On 06/15/2017 03:55 PM, Adhemerval Zanella wrote:
>>>> On 15/06/2017 17:45, Paul Clarke wrote:
>>>>> Commit cdfbe5037f2f67bf5f560b73732b69d0fabe2314 added sysconf support
>>>>> for cache geometries on powerpc, but mishandled errno.  For valid input
>>>>> parameters, sysconf() should not set errno.
>>>>
>>>> Do we have a testcase for it? If not I think we should add it.
>>>
>>> We don't.  There are three obvious test cases for sysconf:
>>> - ./nptl/tst-sysconf.c: ignores errno
>>> - ./posix/tst-sysconf.c:  ignores errno
>>> - ./sysdeps/unix/sysv/linux/powerpc/tst-sysconf.c
>>>   Ironically (given the need for the subject patch), this correctly tests for errno changes but only for the _SC_LEVEL*, and only for powerpc.
>>
>> I think you might have a staged change in your repo, I do not see
>> the powerpc specific one in master.  In any case, I think it worth
>> to add some errno testing on nptl and posix tests (maybe even
>> incorporate nptl on posix) and add powerpc one (be aware to avoid
>> add a testcase with same name in same folder, as this powerpc seems
>> to be doing).
> 
> I guess Paul meant to say
> sysdeps/unix/sysv/linux/powerpc/test-powerpc-linux-sysconf.c, which is the
> new name of the test after solving the name clash issue.  ;-)
> 

Right, so that's the one that caused some trouble in the past. We can use
this file to add a regression test then.
Joseph Myers June 16, 2017, 3:50 p.m. UTC | #10
On Fri, 16 Jun 2017, Paul Clarke wrote:

> On 06/15/2017 03:55 PM, Adhemerval Zanella wrote:
> > On 15/06/2017 17:45, Paul Clarke wrote:
> >> Commit cdfbe5037f2f67bf5f560b73732b69d0fabe2314 added sysconf support
> >> for cache geometries on powerpc, but mishandled errno.  For valid input
> >> parameters, sysconf() should not set errno.
> > 
> > Do we have a testcase for it? If not I think we should add it.
> 
> We don't.  There are three obvious test cases for sysconf:
> - ./nptl/tst-sysconf.c: ignores errno
> - ./posix/tst-sysconf.c:  ignores errno
> - ./sysdeps/unix/sysv/linux/powerpc/tst-sysconf.c

I don't see sysdeps/unix/sysv/linux/powerpc/tst-sysconf.c in current 
sources (and that's a bad name as it would override the tst-sysconf.c 
files in nptl/ and posix/).
Paul A. Clarke June 16, 2017, 3:55 p.m. UTC | #11
On 06/16/2017 10:50 AM, Joseph Myers wrote:
> On Fri, 16 Jun 2017, Paul Clarke wrote:
> On 06/15/2017 03:55 PM, Adhemerval Zanella wrote:
>> > Do we have a testcase for it? If not I think we should add it.
>> 
>> We don't.  There are three obvious test cases for sysconf:
>> - ./nptl/tst-sysconf.c: ignores errno
>> - ./posix/tst-sysconf.c:  ignores errno
>> - ./sysdeps/unix/sysv/linux/powerpc/tst-sysconf.c
> 
> I don't see sysdeps/unix/sysv/linux/powerpc/tst-sysconf.c in current 
> sources (and that's a bad name as it would override the tst-sysconf.c 
> files in nptl/ and posix/).

Sorry, cut and paste error from an older version of the patch that actually added 
sysdeps/unix/sysv/linux/powerpc/test-powerpc-linux-sysconf.c. (Tulio also pointed this out).

PC
Tulio Magno Quites Machado Filho June 20, 2017, 9:08 p.m. UTC | #12
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> On 16/06/2017 10:36, Tulio Magno Quites Machado Filho wrote:
>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>> 
>>> On 16/06/2017 09:00, Paul Clarke wrote:
>>>> On 06/15/2017 03:55 PM, Adhemerval Zanella wrote:
>>>>> On 15/06/2017 17:45, Paul Clarke wrote:
>>>>>> Commit cdfbe5037f2f67bf5f560b73732b69d0fabe2314 added sysconf support
>>>>>> for cache geometries on powerpc, but mishandled errno.  For valid input
>>>>>> parameters, sysconf() should not set errno.
>>>>>
>>>>> Do we have a testcase for it? If not I think we should add it.
>>>>
>>>> We don't.  There are three obvious test cases for sysconf:
>>>> - ./nptl/tst-sysconf.c: ignores errno
>>>> - ./posix/tst-sysconf.c:  ignores errno
>>>> - ./sysdeps/unix/sysv/linux/powerpc/tst-sysconf.c
>>>>   Ironically (given the need for the subject patch), this correctly tests for errno changes but only for the _SC_LEVEL*, and only for powerpc.
>>>
>>> I think you might have a staged change in your repo, I do not see
>>> the powerpc specific one in master.  In any case, I think it worth
>>> to add some errno testing on nptl and posix tests (maybe even
>>> incorporate nptl on posix) and add powerpc one (be aware to avoid
>>> add a testcase with same name in same folder, as this powerpc seems
>>> to be doing).
>> 
>> I guess Paul meant to say
>> sysdeps/unix/sysv/linux/powerpc/test-powerpc-linux-sysconf.c, which is the
>> new name of the test after solving the name clash issue.  ;-)
>
> Right, so that's the one that caused some trouble in the past. We can use
> this file to add a regression test then.

I was planning to push this patch, but your last message wasn't entirely clear
to me.
Do you think there is something else to be done in this patch?
Adhemerval Zanella June 20, 2017, 9:14 p.m. UTC | #13
On 20/06/2017 18:08, Tulio Magno Quites Machado Filho wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> On 16/06/2017 10:36, Tulio Magno Quites Machado Filho wrote:
>>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>>>
>>>> On 16/06/2017 09:00, Paul Clarke wrote:
>>>>> On 06/15/2017 03:55 PM, Adhemerval Zanella wrote:
>>>>>> On 15/06/2017 17:45, Paul Clarke wrote:
>>>>>>> Commit cdfbe5037f2f67bf5f560b73732b69d0fabe2314 added sysconf support
>>>>>>> for cache geometries on powerpc, but mishandled errno.  For valid input
>>>>>>> parameters, sysconf() should not set errno.
>>>>>>
>>>>>> Do we have a testcase for it? If not I think we should add it.
>>>>>
>>>>> We don't.  There are three obvious test cases for sysconf:
>>>>> - ./nptl/tst-sysconf.c: ignores errno
>>>>> - ./posix/tst-sysconf.c:  ignores errno
>>>>> - ./sysdeps/unix/sysv/linux/powerpc/tst-sysconf.c
>>>>>   Ironically (given the need for the subject patch), this correctly tests for errno changes but only for the _SC_LEVEL*, and only for powerpc.
>>>>
>>>> I think you might have a staged change in your repo, I do not see
>>>> the powerpc specific one in master.  In any case, I think it worth
>>>> to add some errno testing on nptl and posix tests (maybe even
>>>> incorporate nptl on posix) and add powerpc one (be aware to avoid
>>>> add a testcase with same name in same folder, as this powerpc seems
>>>> to be doing).
>>>
>>> I guess Paul meant to say
>>> sysdeps/unix/sysv/linux/powerpc/test-powerpc-linux-sysconf.c, which is the
>>> new name of the test after solving the name clash issue.  ;-)
>>
>> Right, so that's the one that caused some trouble in the past. We can use
>> this file to add a regression test then.
> 
> I was planning to push this patch, but your last message wasn't entirely clear
> to me.
> Do you think there is something else to be done in this patch?
> 

My understanding based on Paul's answer is we do not have regression tests
for the very issues this patch intend, but test-powerpc-linux-sysconf.c does
seem to have tests for errno handling (which may only be trigger in recent
kernel due underlying support). If it is the case the patch lgtm, otherwise
I think we need to add some regression tests.
Tulio Magno Quites Machado Filho June 22, 2017, 1:52 p.m. UTC | #14
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> On 20/06/2017 18:08, Tulio Magno Quites Machado Filho wrote:
>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>> 
>>> On 16/06/2017 10:36, Tulio Magno Quites Machado Filho wrote:
>>>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>>>>
>>>>> On 16/06/2017 09:00, Paul Clarke wrote:
>>>>>> On 06/15/2017 03:55 PM, Adhemerval Zanella wrote:
>>>>>>> On 15/06/2017 17:45, Paul Clarke wrote:
>>>>>>>> Commit cdfbe5037f2f67bf5f560b73732b69d0fabe2314 added sysconf support
>>>>>>>> for cache geometries on powerpc, but mishandled errno.  For valid input
>>>>>>>> parameters, sysconf() should not set errno.
>>>>>>>
>>>>>>> Do we have a testcase for it? If not I think we should add it.
>>>>>>
>>>>>> We don't.  There are three obvious test cases for sysconf:
>>>>>> - ./nptl/tst-sysconf.c: ignores errno
>>>>>> - ./posix/tst-sysconf.c:  ignores errno
>>>>>> - ./sysdeps/unix/sysv/linux/powerpc/tst-sysconf.c
>>>>>>   Ironically (given the need for the subject patch), this correctly tests for errno changes but only for the _SC_LEVEL*, and only for powerpc.
>>>>>
>>>>> I think you might have a staged change in your repo, I do not see
>>>>> the powerpc specific one in master.  In any case, I think it worth
>>>>> to add some errno testing on nptl and posix tests (maybe even
>>>>> incorporate nptl on posix) and add powerpc one (be aware to avoid
>>>>> add a testcase with same name in same folder, as this powerpc seems
>>>>> to be doing).
>>>>
>>>> I guess Paul meant to say
>>>> sysdeps/unix/sysv/linux/powerpc/test-powerpc-linux-sysconf.c, which is the
>>>> new name of the test after solving the name clash issue.  ;-)
>>>
>>> Right, so that's the one that caused some trouble in the past. We can use
>>> this file to add a regression test then.
>> 
>> I was planning to push this patch, but your last message wasn't entirely clear
>> to me.
>> Do you think there is something else to be done in this patch?
>> 
>
> My understanding based on Paul's answer is we do not have regression tests
> for the very issues this patch intend, but test-powerpc-linux-sysconf.c does
> seem to have tests for errno handling (which may only be trigger in recent
> kernel due underlying support). If it is the case the patch lgtm, otherwise
> I think we need to add some regression tests.

I confirmed that's indeed the case, so I'm pushing this patch.
diff mbox

Patch

diff --git a/sysdeps/unix/sysv/linux/powerpc/sysconf.c b/sysdeps/unix/sysv/linux/powerpc/sysconf.c
index 10c4aa0..7bed701 100644
--- a/sysdeps/unix/sysv/linux/powerpc/sysconf.c
+++ b/sysdeps/unix/sysv/linux/powerpc/sysconf.c
@@ -22,37 +22,16 @@ 
 
 static long linux_sysconf (int name);
 
-static long
-auxv2sysconf (unsigned long type)
-{
-  long rc;
-  rc = __getauxval (type);
-  if (rc == 0)
-  {
-    __set_errno (EINVAL);
-    rc = -1;
-  }
-  return rc;
-}
-
-static long
+static inline long
 auxv2sysconf_cache_associativity (unsigned long type)
 {
-  long rc;
-  rc = auxv2sysconf (type);
-  if (rc != -1)
-    rc = (rc & 0xffff0000) >> 16;
-  return rc;
+  return (__getauxval (type) & 0xffff0000) >> 16;
 }
 
-static long
+static inline long
 auxv2sysconf_cache_linesize (unsigned long type)
 {
-  long rc;
-  rc = auxv2sysconf (type);
-  if (rc != -1)
-    rc = rc & 0xffff;
-  return rc;
+  return __getauxval (type) & 0xffff;
 }
 
 /* Get the value of the system variable NAME.  */
@@ -62,25 +41,25 @@  __sysconf (int name)
   switch (name)
     {
       case _SC_LEVEL1_ICACHE_SIZE:
-	return auxv2sysconf (AT_L1I_CACHESIZE);
+	return __getauxval (AT_L1I_CACHESIZE);
       case _SC_LEVEL1_ICACHE_ASSOC:
 	return auxv2sysconf_cache_associativity (AT_L1I_CACHEGEOMETRY);
       case _SC_LEVEL1_ICACHE_LINESIZE:
 	return auxv2sysconf_cache_linesize (AT_L1I_CACHEGEOMETRY);
       case _SC_LEVEL1_DCACHE_SIZE:
-	return auxv2sysconf (AT_L1D_CACHESIZE);
+	return __getauxval (AT_L1D_CACHESIZE);
       case _SC_LEVEL1_DCACHE_ASSOC:
 	return auxv2sysconf_cache_associativity (AT_L1D_CACHEGEOMETRY);
       case _SC_LEVEL1_DCACHE_LINESIZE:
 	return auxv2sysconf_cache_linesize (AT_L1D_CACHEGEOMETRY);
       case _SC_LEVEL2_CACHE_SIZE:
-	return auxv2sysconf (AT_L2_CACHESIZE);
+	return __getauxval (AT_L2_CACHESIZE);
       case _SC_LEVEL2_CACHE_ASSOC:
 	return auxv2sysconf_cache_associativity (AT_L2_CACHEGEOMETRY);
       case _SC_LEVEL2_CACHE_LINESIZE:
 	return auxv2sysconf_cache_linesize (AT_L2_CACHEGEOMETRY);
       case _SC_LEVEL3_CACHE_SIZE:
-	return auxv2sysconf (AT_L3_CACHESIZE);
+	return __getauxval (AT_L3_CACHESIZE);
       case _SC_LEVEL3_CACHE_ASSOC:
 	return auxv2sysconf_cache_associativity (AT_L3_CACHEGEOMETRY);
       case _SC_LEVEL3_CACHE_LINESIZE: