On Thu, Nov 4, 2010 at 12:42 AM, CAI Qian <[email protected]> wrote:
> libhugetlbfs allows to overcommit hugepages and there are tunables in
> sysfs and procfs. The test here want to ensure it is possible to
> overcommit by either mmap or shared memory. Also ensure those
> reservation can be read/write, and several statistics work correctly.
>
> First, it resets nr_hugepages and nr_overcommit_hugepages. Then, set
> both to a specify value - N, and allocate N + %50 x N hugepages.
> Finally, it reads and writes every page. There are command options to
> choose either to manage hugepages from sysfs or procfs, and reserve them
> by mmap or shmget.
>
> Signed-off-by: CAI Qian <[email protected]>
> ---
> v3: fix -a option description; fix a nasty bug with tst_exit which skips
> cleanup at the end of the test; add more information to assist test
> debug.
No... you just didn't understand the API :)... if the cleanup function
is NULL, tst_exit is automatically used; otherwise, the function is
called, and cleanup is assumed to be called.
> v2: add more explanation and a command-line option to specify memory
> allocation size.
>
> testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c | 487
> ++++++++++++++++++++
> 1 files changed, 487 insertions(+), 0 deletions(-)
> create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
>
> diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
> b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
> new file mode 100644
> index 0000000..770f664
> --- /dev/null
> +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
> @@ -0,0 +1,487 @@
> +/*
> + * overcommit libhugetlbfs and check the statistics.
> + *
> + * libhugetlbfs allows to overcommit hugepages and there are tunables in
> + * sysfs and procfs. The test here want to ensure it is possible to
> + * overcommit by either mmap or shared memory. Also ensure those
> + * reservation can be read/write, and several statistics work correctly.
Is there a document that describes this behavior?
> + * First, it resets nr_hugepages and nr_overcommit_hugepages. Then, set
> + * both to a specify value - N, and allocate N + %50 x N hugepages.
> + * Finally, it reads and writes every page. There are command options to
> + * choose either to manage hugepages from sysfs or procfs, and reserve
> + * them by mmap or shmget.
What happens when you overcommit and meet this maximum though?
...
> +#define PROTECTION (PROT_READ | PROT_WRITE)
> +#define _PATH_MEMINFO "/proc/meminfo"
> +#define _PATH_SYS_HUGE "/sys/kernel/mm/hugepages"
> +#define _PATH_SYS_2M _PATH_SYS_HUGE "/hugepages-2048kB/"
> +#define _PATH_SYS_2M_OVER _PATH_SYS_2M "nr_overcommit_hugepages"
> +#define _PATH_SYS_2M_FREE _PATH_SYS_2M "free_hugepages"
> +#define _PATH_SYS_2M_RESV _PATH_SYS_2M "resv_hugepages"
> +#define _PATH_SYS_2M_SURP _PATH_SYS_2M "surplus_hugepages"
> +#define _PATH_SYS_2M_HUGE _PATH_SYS_2M "nr_hugepages"
> +#define _PATH_PROC_VM "/proc/sys/vm/"
> +#define _PATH_PROC_OVER _PATH_PROC_VM "nr_overcommit_hugepages"
> +#define _PATH_PROC_HUGE _PATH_PROC_VM "nr_hugepages"
> +#define _PATH_SHMMAX "/proc/sys/kernel/shmmax"
> +#define NEED_RESTORE 2
Just make this a boolean type, i.e.:
int restore_shmmax;
...
/* cleanup */
if (need_shmmax) {
/* ... */
}
/* ... */
need_shmmax = 1;
> +
> +/* Only ia64 requires this */
Why only ia64?
> +#ifdef __ia64__
> +#define ADDR (void *)(0x8000000000000000UL)
> +#define FLAGS (MAP_SHARED | MAP_FIXED)
> +#define SHMAT_FLAGS (SHM_RND)
> +#else
> +#define ADDR (void *)(0x0UL)
> +#define FLAGS (MAP_SHARED)
> +#define SHMAT_FLAGS (0)
> +#endif
> +
> +#ifndef SHM_HUGETLB
> +#define SHM_HUGETLB 04000
> +#endif
> +
> +char *TCID = "hugemmap05";
> +int TST_TOTAL = 1;
> +extern int Tst_count;
> +extern char *TESTDIR;
> +static char nr_hugepages[BUFSIZ], nr_overcommit_hugepages[BUFSIZ];
> +static char shmmax[BUFSIZ], *opt_allocstr;
> +static char buf[BUFSIZ], line[BUFSIZ], path[BUFSIZ], pathover[BUFSIZ];
> +static int opt_sysfs, opt_shmget, opt_alloc, size = 128, length = 384;
length and size should be size_t. Also, instead of having opt_shmget
as a separate variable, why not just test for shmid > 0 (below in the
relevant code block, etc)?
...
> +static void checksys(char *path, char *pattern, int value);
The value of pattern is going to generate more harm than good because
they're currently /proc analogs. Please better humanize the value (or
point to relevant documentation in the source stating what the tunable
/sys nodes do.
...
> + if (opt_alloc) {
> + size = atoi(opt_allocstr);
What happens if size <= 0? atoi is probably not what you want to use anyhow...
> + length = (int)(size + size * 0.5) * 2;
> + }
...
> + if(TEST_RETURN != 0)
> + tst_resm(TFAIL, "overcommit() failed with %ld.",
> + TEST_RETURN);
No reason in outputting the return value (it's not like you don't have
any control over what it is).
...
> + cleanup();
/* NOTREACHED */ // -- just to be pedantic.
> + return 0;
> +}
> +
> +int overcommit(void)
> +{
> + void *addr = NULL;
> + int fd = -1, shmid = -1;
> + char s[BUFSIZ];
> + FILE *fp;
> + unsigned long i;
> + char *shmaddr = NULL;
> +
> + if (opt_shmget) {
> + if ((shmid = shmget(2, (long)(length * 1024 * 1024),
So you're going to stomp all over someone else's shm key potentially?
> + SHM_HUGETLB | IPC_CREAT | SHM_R | SHM_W)) < 0)
> + tst_brkm(TBROK|TERRNO, cleanup, "shmget");
> + } else {
> + snprintf(s, strlen(TESTDIR) + 17, "%s/hugemmap05/file",
> + TESTDIR);
YAY! A magic number (-_-)!
> + fd = open(s, O_CREAT | O_RDWR, 0755);
> + if (fd < 0) {
> + tst_resm(TFAIL|TERRNO, "open");
> + return 1;
> + }
> + addr = mmap(ADDR, (long)(length * 1024 * 1024),
> + PROTECTION, FLAGS, fd, 0);
Why do you have to specify an ADDR? Why not let the OS do this for you?
> + if (addr == MAP_FAILED)
> + tst_brkm(TBROK|TERRNO, cleanup, "mmap");
...
> + tst_resm(TINFO|TERRNO, "shmat");
> + shmctl(shmid, IPC_RMID, NULL);
Why not just add this to cleanup, and merge the tst_resm and tst_brkm calls?
> + tst_brkm(TBROK, cleanup,
> + "shared memory attach failure");
> + }
> + tst_resm(TINFO, "shmaddr: %p", shmaddr);
> + tst_resm(TINFO, "starting the writes...");
This could be one line.
> + for (i = 0; i < (long)(length * 1024 * 1024); i++)
> + shmaddr[i] = (char)(i);
NO. NO. NO. NO. NO. void* != char !!!
> + tst_resm(TINFO, "starting the check...");
> + for (i = 0; i < (long)(length * 1024 * 1024); i++)
> + if (shmaddr[i] != (char)i)
NO. NO. NO. NO. NO. void* != char !!!
> + tst_resm(TINFO, "index %lu mismatched",
> + i);
...
> + shmctl(shmid, IPC_RMID, NULL);
Writing the same instruction twice screams "put me in cleanup!".
...
> +}
> +
> +static void check_bytes(char *addr)
Huh? What purpose does this really serve in the overall scheme of
things, point being, having a function call to alias tst_resm(TINFO,
...) in only two spots in the code seems kind of silly, as the
function declaration and body is longer than the 2 times tst_resm is
invoked...).
> +{
> + tst_resm(TINFO, "First hex is %x", *((unsigned int *)addr));
> +}
> +
> +static void write_bytes(char *addr)
> +{
> + unsigned long i;
> +
> + for (i = 0; i < (long)(length * 1024 * 1024); i++)
> + *(addr + i) = (char)i;
NO. NO. NO. NO. NO. void* != char !!! Pointer arithmetic can be bad as well...
> +}
> +
> +static void read_bytes(char *addr)
> +{
> + unsigned long i;
Oops... overflow?
> +
> + check_bytes(addr);
> + for (i = 0; i < (long)(length * 1024 * 1024); i++)
> + if (*(addr + i) != (char)i) {
Please see above.
> + tst_resm(TFAIL, "mismatch at %lu\n", i);
> + break;
> + }
> +}
> +
> +/* Lookup a pattern and get the value from file*/
> +int lookup (char *line, char *pattern)
> +{
> + char buf2[BUFSIZ];
> +
> + /* empty line */
> + if (line[0] == '\0')
> + return 0;
> +
> + snprintf(buf2, BUFSIZ, "%s: %%s", pattern);
> + if (sscanf(line, buf2, buf) != 1)
> + return 0;
That ain't gonna work so well with variable whitespace.
> + return 1;
> +}
> +
> +void help(void)
usage is more standard than help.
...
> +void checkproc(FILE *fp, char *pattern, int value)
> +{
> + memset(buf, -1, BUFSIZ);
> + rewind(fp);
> + while (fgets(line, BUFSIZ, fp) != NULL)
> + if (lookup(line, pattern))
If you're just looking for a string, why not strstr the line?
Thanks,
-Garrett
------------------------------------------------------------------------------
The Next 800 Companies to Lead America's Growth: New Video Whitepaper
David G. Thomson, author of the best-selling book "Blueprint to a
Billion" shares his insights and actions to help propel your
business during the next growth cycle. Listen Now!
http://p.sf.net/sfu/SAP-dev2dev
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list
