> 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. Yes, what I am trying to say that it should use tst_brkm instead of tst_exit because it is intentional to call cleanup there.
> Is there a document that describes this behavior? Yes, http://lxr.linux.no/#linux+v2.6.35/Documentation/vm/hugetlbpage.txt > What happens when you overcommit and meet this maximum though? mmap/shm should fail with ENOMEM according to manpages. > Just make this a boolean type, i.e.: > > int restore_shmmax; > > ... > > /* cleanup */ > if (need_shmmax) { > /* ... */ > } > > /* ... */ > need_shmmax = 1; OK. > > > + > > +/* Only ia64 requires this */ > > Why only ia64? From kernel docs, http://lxr.linux.no/#linux+v2.6.35/Documentation/vm/hugepage-mmap.c http://lxr.linux.no/#linux+v2.6.35/Documentation/vm/hugepage-shm.c > > > +#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)? OK. > > ... > > > +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. How do you want to humanize here? The sysfs was discussed here, http://lxr.linux.no/#linux+v2.6.35/Documentation/vm/hugetlbpage.txt > > ... > > > + if (opt_alloc) { > > + size = atoi(opt_allocstr); > > What happens if size <= 0? atoi is probably not what you want to use > anyhow... This might be used for negative testing to set to a negative value. It is also garbage in garbage out. > > > + 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). OK. > > ... > > > + cleanup(); > /* NOTREACHED */ // -- just to be pedantic. > > + return 0; OK. > > +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? OK, I'll be trying IPC_PRIVATE. > > > + 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 (-_-)! OK. > > > + 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? It is only ia64 needs to specify it that mentioned by the kernel docs above. > > > + 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? OK. > > > + 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. OK. > > > + for (i = 0; i < (long)(length * 1024 * 1024); i++) > > + shmaddr[i] = (char)(i); > > NO. NO. NO. NO. NO. void* != char !!! OK. > > > + 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 !!! OK. > > > + tst_resm(TINFO, "index %lu > mismatched", > > + i); > > ... > > > + shmctl(shmid, IPC_RMID, NULL); > > Writing the same instruction twice screams "put me in cleanup!". OK. > > ... > > > +} > > + > > +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...). This function was modified but it is not as needed now as it was designed. > > > +{ > > + 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... OK. > > > +} > > + > > +static void read_bytes(char *addr) > > +{ > > + unsigned long i; > > Oops... overflow? OK. > > > + > > + check_bytes(addr); > > + for (i = 0; i < (long)(length * 1024 * 1024); i++) > > + if (*(addr + i) != (char)i) { > > Please see above. OK. > > > + 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. This is an internal function that is only used for the entries in procfs and sysfs, so I would not expect variable whitespace there. > > > + return 1; > > +} > > + > > +void help(void) > > usage is more standard than help. There are several existing LTP tests used help(), but it is fine to call it usage() too. > > ... > > > +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? I had hard time to use strstr() at first, but the above was working for me. Thanks. CAI ------------------------------------------------------------------------------ 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
