I think my mails were not getting sent out properly earlier, resending
so it hits the list.
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.
> 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).
> 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?
> /* 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?
<snip>
> diff -ur libhugetlbfs-dev-20080319.orig/tests/hugetests.h
> libhugetlbfs-dev-20080319/tests/hugetests.h
> --- libhugetlbfs-dev-20080319.orig/tests/hugetests.h 2008-03-19
> 10:25:28.000000000 -0500
> +++ libhugetlbfs-dev-20080319/tests/hugetests.h 2008-04-09
> 14:55:54.000162480 -0500
> @@ -20,6 +20,8 @@
> #ifndef _HUGETESTS_H
> #define _HUGETESTS_H
>
> +#include <errno.h>
> +#include <string.h>
> #define DEBUG
>
> /* Test return codes */
> @@ -32,6 +34,7 @@
>
> extern int verbose_test;
> extern char *test_name;
> +long call_gethugepagesize(void);
> void check_free_huge_pages(int nr_pages_needed);
> void test_init(int argc, char *argv[]);
> int test_addr_huge(void *p);
> @@ -107,4 +110,18 @@
> /* stressutils.c stuff */
> int remove_shmid(int shmid);
>
> +static inline long check_hugepagesize()
> +{
> + long hpage_size = call_gethugepagesize();
> + if (hpage_size < 0) {
> + if (errno == ENOSYS)
> + CONFIG("No hugepage kernel support\n");
> + else if (errno == EOVERFLOW)
> + CONFIG("Hugepage size too large");
> + else
> + CONFIG("Hugepage size (%s)", strerror(errno));
> + }
> + return hpage_size;
> +}
Great, this looks much cleaner.
<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.
<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?
> + 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
--
Nishanth Aravamudan <[EMAIL PROTECTED]>
IBM Linux Technology Center
-------------------------------------------------------------------------
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
