Message ID | 3a5ed534-95d2-4d66-533e-aaed56134344@us.ibm.com |
---|---|
State | Committed |
Headers |
Received: (qmail 62169 invoked by alias); 15 Jun 2017 20:45:48 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 62153 invoked by uid 89); 15 Jun 2017 20:45:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-27.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KHOP_DYNAMIC, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx0a-001b2d01.pphosted.com Reply-To: pc@us.ibm.com To: "libc-alpha@sourceware.org" <libc-alpha@sourceware.org>, Richard Henderson <rth@twiddle.net>, Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com> From: Paul Clarke <pc@us.ibm.com> Subject: [PATCH] powerpc: fix sysconf support for cache geometries Date: Thu, 15 Jun 2017 15:45:44 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17061520-0028-0000-0000-000007D1CA85 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007239; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000212; SDB=6.00875287; UDB=6.00435781; IPR=6.00655376; BA=6.00005423; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00015840; XFM=3.00000015; UTC=2017-06-15 20:45:47 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17061520-0029-0000-0000-0000363CB617 Message-Id: <3a5ed534-95d2-4d66-533e-aaed56134344@us.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2017-06-15_10:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1706150359 |
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
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: >
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~
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
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~
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
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
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).
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. ;-)
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.
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/).
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
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?
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.
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 --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: