Nishanth Aravamudan wrote:
> On 09.04.2008 [16:22:34 -0500], Jon Tollefson wrote:
>
>> This patch moves the check of the return value from gethugepagesize() to a
>> common function, check_hugepagesize(),
>> rather then repeating it in each test. gethugepagesize() is updated to set
>> errno depending on the reason for the failure.
>>
>> Changes since v3
>> -incorporated Mel Gorman's function that uses dlopen so that tests
>> don't have to be linked against libhugetlbfs
>> -used suggestions from Nishanth Aravamudan to use a static inline
>> function and rename to check_hugepagesize()
>> -added check_hugepagesize() call to linkhuge and linkshare tests
>>
>
> Wow, looks much better... You're missing a Signed-off-by in this
> version, though.
>
oops, will repost with that
>
>> diff -ur libhugetlbfs-dev-20080319.orig/hugeutils.c
>> libhugetlbfs-dev-20080319/hugeutils.c
>> --- libhugetlbfs-dev-20080319.orig/hugeutils.c 2008-03-19
>> 10:25:28.000000000 -0500
>> +++ libhugetlbfs-dev-20080319/hugeutils.c 2008-04-03 11:40:13.630958744
>> -0500
>> @@ -112,14 +112,18 @@
>>
>> if (hpage_size)
>> return hpage_size;
>> + errno = 0;
>>
>
> Should we perhaps unconditionally set errno = 0 before any possible
> returns? That is, move this line to before if (hpage_size).
>
If getting the huge page succeeded or failed the first time it will
always return the same value. So if anything it seems we should save
the errno value from the first call and set the same on future calls.
>
>> hpage_kb = read_meminfo("Hugepagesize:");
>> if (hpage_kb < 0) {
>> hpage_size = -1;
>> + errno = ENOSYS;
>> } else {
>> - if (hpage_kb > max_hpage_kb)
>> + if (hpage_kb > max_hpage_kb) {
>> /* would overflow if converted to bytes */
>> hpage_size = -1;
>> + errno = EOVERFLOW;
>> + }
>> else
>>
>
> Can you put this else on the same line as the previous and add
> curly-braces to match the paired-if?
>
ok
>
>> /* convert from kb to bytes */
>> hpage_size = 1024 * hpage_kb;
>>
>
> <snip>
>
>
>> diff -ur libhugetlbfs-dev-20080319.orig/tests/empty_mounts.c
>> libhugetlbfs-dev-20080319/tests/empty_mounts.c
>> --- libhugetlbfs-dev-20080319.orig/tests/empty_mounts.c 2008-03-19
>> 10:25:28.000000000 -0500
>> +++ libhugetlbfs-dev-20080319/tests/empty_mounts.c 2008-04-03
>> 09:41:49.914014480 -0500
>> @@ -57,15 +57,10 @@
>>
>> int main(int argc, char *argv[])
>> {
>> - long hpage_size;
>> int fd;
>>
>> test_init(argc, argv);
>>
>> - hpage_size = gethugepagesize();
>> - if (hpage_size < 0)
>> - CONFIG("No hugepage kernel support");
>> -
>> fd = hugetlbfs_unlinked_fd();
>> if (fd < 0)
>> PASS();
>>
>
> I'm not sure I follow why we're removing this check altogether from this
> test?
>
This test doesn't use the hugepage size so its value doesn't seem to be
relevant, but if you want me to check I can put that in here.
<snip>
>
>
>> diff -ur libhugetlbfs-dev-20080319.orig/tests/linkhuge.c
>> libhugetlbfs-dev-20080319/tests/linkhuge.c
>> --- libhugetlbfs-dev-20080319.orig/tests/linkhuge.c 2008-03-19
>> 10:25:28.000000000 -0500
>> +++ libhugetlbfs-dev-20080319/tests/linkhuge.c 2008-04-09
>> 15:05:34.076091800 -0500
>> @@ -132,6 +132,7 @@
>> int elfmap_inhibited;
>>
>> test_init(argc, argv);
>> + check_hugepagesize();
>>
>> get_link_string(argv[0]);
>>
>> diff -ur libhugetlbfs-dev-20080319.orig/tests/linkshare.c
>> libhugetlbfs-dev-20080319/tests/linkshare.c
>> --- libhugetlbfs-dev-20080319.orig/tests/linkshare.c 2008-03-19
>> 10:25:28.000000000 -0500
>> +++ libhugetlbfs-dev-20080319/tests/linkshare.c 2008-04-09
>> 15:06:58.926096744 -0500
>> @@ -289,6 +289,7 @@
>> int main(int argc, char *argv[], char *envp[])
>> {
>> test_init(argc, argv);
>> + check_hugepagesize();
>>
>> if (argc == 1) {
>> /*
>>
>
> I'm torn in these two cases. I believe we have a pre-existing issue
> which I'd like to see your patch fix -- elflink.c doesn't check the
> return value of gethugepagesize(), so we're aborting a test early via a
> CONFIG() message, which should still fail, but won't. Can you fix up
> elflink.c and then maybe not require us to call check_hugepagesize()
> here? I'm not sure, I can see us going either way:
>
> 1) regardless of the test, if gethugepagesize() indicates hugepges are
> unusable, bail out via check_hugepagesize()
> 2) only do 1) for tests that currently call gethugepagesize(). all other
> tests should be FAIL()'ing internally in libhugetlbfs
>
> Obviously 2) will pollute the 16G test log with FAILs for several
> testcases. Maybe those test cases could have their own specific checks
> internally to make it a FAIL/IRRELEVANT if gethugepagesize() returns -1?
> But in either case, morecore.c and elflink.c should be fixed up to check
> the return value of gethugepagesize() and output useful debugging
> messages based upon errno, I think.
>
With the changes to elflink/morecore these tests report back that the
size is too large so I'll remove the checks from these tests. I'll post
the changes to elflink and morecore too, but as a 2nd patch.
> <snip>
>
>
>> diff -ur libhugetlbfs-dev-20080319.orig/tests/testutils.c
>> libhugetlbfs-dev-20080319/tests/testutils.c
>> --- libhugetlbfs-dev-20080319.orig/tests/testutils.c 2008-03-19
>> 10:25:28.000000000 -0500
>> +++ libhugetlbfs-dev-20080319/tests/testutils.c 2008-04-09
>> 14:06:18.747046760 -0500
>> @@ -34,6 +34,7 @@
>> #include <sys/shm.h>
>> #include <sys/stat.h>
>> #include <fcntl.h>
>> +#include <dlfcn.h>
>>
>> #include "hugetests.h"
>>
>> @@ -69,6 +70,44 @@
>> exit(RC_BUG);
>> }
>>
>> +extern long gethugepagesize (void) __attribute__ ((weak));
>> +
>> +long call_gethugepagesize(void)
>> +{
>> + static long (*lib_gethugepagesize)(void) = NULL;
>> + void *handle;
>> + char *error;
>> +
>> + if (gethugepagesize)
>> + return gethugepagesize();
>> +
>> + if (!lib_gethugepagesize) {
>> +
>> + /* Open the library */
>> + if (sizeof(int) == 4)
>> + handle = dlopen("../obj32/libhugetlbfs.so", RTLD_LAZY);
>> + else
>> + handle = dlopen("../obj64/libhugetlbfs.so", RTLD_LAZY);
>>
>
> Hrm, I think here you're now going to open the library even if the
> library is not PRELOAD'd (malloc) or linked in via linker scripts
> (linkhuge, linkshare), since you're not relying on that path being set.
> Why can't we just do the simple
>
> dlsym(RTLD_DEFAULT, "gethugepagesize")
>
> and rely on the test-suite to set things up correctly?
>
That seems to work so I'll make that change too.
>
>> + if (!handle) {
>> + fprintf(stderr, "%s: %s (pid=%d)\n", test_name,
>> + "Failed to load libhugetlbfs.so",
>> + getpid());
>> + exit(RC_BUG);
>> + }
>>
>
> That would make it so we don't need to deal with the handle. I'm not
> sure if it will work for all the cases we care about, but it might be
> enough.
>
> Doing all of this dlopen() stuff also may be simply unimportant
> depending on how we answer my question above.
>
> <snip>
>
> Thanks,
> Nish
>
>
Jon
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference
Don't miss this year's exciting event. There's still time to save $100.
Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
Libhugetlbfs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/libhugetlbfs-devel
