[LTP] [PATCH] hugemmap05 issues

[LTP] [PATCH] hugemmap05 issues

[Garrett Cooper]

[-- Attachment #1: Type: text/plain, Size: 6513 bytes --]

    Looking over the code, there are a number of flawed assumptions in
terms of how things are executed and it seems that the code is overly
complex. Before I go and do something rash though, I wanted others to
look at the code (in particular the areas I'm touching) and see
whether or not I'm just blowing something out my rear.
    As-is the code doesn't compile due to warnings.
    This is a WIP patch (please ignore the whitespace and style to a
small degree as I know gmail will fubar the formatting).
Thanks,
-Garrett

diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
index 8893f84..2ea1363 100644
--- a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
+++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
@@ -97,7 +97,7 @@ static option_t options[] = {
 	{ NULL, NULL,		NULL}
 };
 static void setup(void);
-static void cleanup(void) LTP_ATTRIBUTE_NORETURN;
+static void cleanup(void);
 static void overcommit(void);
 static void write_bytes(void *addr);
 static void read_bytes(void *addr);
@@ -135,6 +135,7 @@ int main(int argc, char *argv[])
 		overcommit();
 	}
 	cleanup();
+	tst_exit();
 }

 static void overcommit(void)
@@ -154,8 +155,15 @@ static void overcommit(void)
 		if (shmid == -1)
 			tst_brkm(TBROK|TERRNO, cleanup, "shmget");
 	} else {
-		snprintf(s, BUFSIZ, "%s/hugemmap05/file", get_tmp_tstdir());
-		fd = open(s, O_CREAT | O_RDWR, 0755);
+		char *tmp_tstdir;
+
+		tmp_tstdir = get_tst_tmpdir();
+		if (tmp_tstdir == NULL)
+			tst_brkm(TBROK|TERRNO, cleanup, "get_tmp_tstdir failed");
+		snprintf(s, BUFSIZ, "%s/hugemmap05/file", tmp_tstdir);
+		free(tmp_tstdir);
+
+		fd = open(s, O_CREAT|O_RDWR, 0755);
 		if (fd == -1)
 			tst_brkm(TBROK|TERRNO, cleanup, "open");
 		addr = mmap(ADDR, (long)(length * MB), PROTECTION, FLAGS, fd,
@@ -195,19 +203,14 @@ static void overcommit(void)
 			return;
 		fclose(fp);
 	}
-	if (shmid != -1) {
-		tst_resm(TINFO, "shmid: 0x%x", shmid);
-		shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
-		if (shmaddr == (void *)-1)
-			tst_brkm(TBROK|TERRNO, cleanup, "shmat");
-		write_bytes(shmaddr);
-		read_bytes(shmaddr);
-        } else {
-		write_bytes(addr);
-		read_bytes(addr);
-	}
+	tst_resm(TINFO, "shmid: 0x%x", shmid);
+	shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
+	if (shmaddr == (void *)-1)
+		tst_brkm(TBROK|TERRNO, cleanup, "shmat");
+	write_bytes(shmaddr);
+	read_bytes(shmaddr);
 	if (opt_sysfs) {
-		tst_resm(TINFO, "check sysfs.");
+		tst_resm(TINFO, "checking sysfs.");
 		if (checksys(_PATH_SYS_2M_HUGE, "HugePages_Total",
 				length / 2) != 0)
 			return;
@@ -221,7 +224,7 @@ static void overcommit(void)
 			!= 0)
 			return;
 	} else {
-		tst_resm(TINFO, "check /proc/meminfo.");
+		tst_resm(TINFO, "checking /proc/meminfo.");
 		fp = fopen(_PATH_MEMINFO, "r");
 		if (fp == NULL)
 			tst_brkm(TBROK|TERRNO, cleanup, "fopen");
@@ -236,18 +239,17 @@ static void overcommit(void)
 			return;
 		fclose(fp);
 	}
-	if (shmid != -1) {
-		if (shmdt(shmaddr) != 0)
-			tst_brkm(TBROK|TERRNO, cleanup, "shmdt");
-	} else {
-		munmap(addr, (long)(length * MB));
-		close(fd);
-		unlink(s);
-	}
+	shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
+	if (shmaddr == (void *)-1)
+		tst_brkm(TBROK|TERRNO, cleanup, "shmat failed");
+	if (shmdt(shmaddr) != 0)
+		tst_brkm(TBROK|TERRNO, cleanup, "shmdt failed");
 }

 static void cleanup(void)
 {
+	char *tmp_tstdir;
+	void *shmaddr;
 	int fd;

 	TEST_CLEANUP;
@@ -262,38 +264,49 @@ static void cleanup(void)
 	fd = open(path, O_WRONLY);
 	if (fd == -1)
 		tst_resm(TWARN|TERRNO, "open");
-	tst_resm(TINFO, "restore nr_hugepages to %s.", nr_hugepages);
-	if (write(fd, nr_hugepages,
-			strlen(nr_hugepages)) != strlen(nr_hugepages))
+	tst_resm(TINFO, "restoring nr_hugepages to %s", nr_hugepages);
+	if (write(fd, nr_hugepages, strlen(nr_hugepages)) !=
+	    strlen(nr_hugepages))
 		tst_resm(TWARN|TERRNO, "write");
 	close(fd);

 	fd = open(pathover, O_WRONLY);
 	if (fd == -1)
 		tst_resm(TWARN|TERRNO, "open");
-	tst_resm(TINFO, "restore nr_overcommit_hugepages to %s.",
+	tst_resm(TINFO, "restoring nr_overcommit_hugepages to %s.",
 		nr_overcommit_hugepages);
 	if (write(fd, nr_overcommit_hugepages, strlen(nr_overcommit_hugepages))
 		!= strlen(nr_overcommit_hugepages))
 		tst_resm(TWARN|TERRNO, "write");
 	close(fd);

-	snprintf(buf, BUFSIZ, "%s/hugemmap05", get_tst_tmpdir());
+	tmp_tstdir = get_tst_tmpdir();
+	if (tmp_tstdir == NULL)
+		tst_brkm(TBROK|TERRNO, NULL, "tmp_tstdir failed");
+	snprintf(buf, BUFSIZ, "%s/hugemmap05", tmp_tstdir);
+	free(tmp_tstdir);
+	shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
+	if (shmaddr == (void *)-1)
+		tst_brkm(TBROK|TERRNO, cleanup, "shmat");
+	else if (shmdt(shmaddr) == -1)
+		tst_resm(TBROK|TERRNO, "shmdt failed");
+	else if (shmctl(shmid, IPC_RMID, NULL) == -1)
+		tst_resm(TBROK|TERRNO,
+		    "shmctl(..., IPC_RMID, ...) failed");
 	if (umount(buf) == -1)
 		tst_resm(TWARN|TERRNO, "umount");
-	if (shmid != -1) {
-		tst_resm(TINFO|TERRNO, "shmdt");
-		shmctl(shmid, IPC_RMID, NULL);
-	}
+	tst_rmdir();

 }

 static void setup(void)
 {
 	FILE *fp;
+	char *tmp_tstdir;
 	int fd;

 	tst_sig(FORK, DEF_HANDLER, cleanup);
+	tst_tmpdir();
 	TEST_PAUSE;
 	if (shmid != -1) {
 		fp = fopen(_PATH_SHMMAX, "r");
@@ -332,7 +345,7 @@ static void setup(void)
 		tst_brkm(TBROK|TERRNO, cleanup, "write");
 	if (lseek(fd, 0, SEEK_SET) == -1)
 		tst_brkm(TBROK|TERRNO, cleanup, "lseek");
-	snprintf(buf, BUFSIZ, "%ld", size);
+	snprintf(buf, BUFSIZ, "%zd", size);
 	if (write(fd, buf, strlen(buf)) != strlen(buf))
 		tst_brkm(TBROK|TERRNO, cleanup,
 			"failed to change nr_hugepages.");
@@ -356,13 +369,18 @@ static void setup(void)
 		tst_brkm(TBROK|TERRNO, cleanup, "write");
 	if (lseek(fd, 0, SEEK_SET) == -1)
 		tst_brkm(TBROK|TERRNO, cleanup, "lseek");
-	snprintf(buf, BUFSIZ, "%ld", size);
+	snprintf(buf, BUFSIZ, "%zd", size);
 	if (write(fd, buf, strlen(buf)) != strlen(buf))
 		tst_brkm(TBROK|TERRNO, cleanup,
 			"failed to change nr_hugepages.");
 	close(fd);

-	snprintf(buf, BUFSIZ, "%s/hugemmap05", get_tst_tmpdir());
+	tmp_tstdir = get_tst_tmpdir();
+	if (tmp_tstdir == NULL)
+		tst_brkm(TBROK|TERRNO, NULL, "tmp_tstdir failed");
+	snprintf(buf, BUFSIZ, "%s/hugemmap05", tmp_tstdir);
+	free(tmp_tstdir);
+
 	if (mkdir(buf, 0700) == -1)
 		tst_brkm(TBROK|TERRNO, cleanup, "mkdir");
 	if (mount(NULL, buf, "hugetlbfs", 0, NULL) == -1)
@@ -447,4 +465,4 @@ static int checkproc(FILE *fp, char *pattern, int value)
 		return 1;
 	}
 	return 0;
-}
\ No newline at end of file
+}

[-- Attachment #2: hugemmap05.patch --]
[-- Type: application/octet-stream, Size: 5996 bytes --]

diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
index 8893f84..2ea1363 100644
--- a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
+++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
@@ -97,7 +97,7 @@ static option_t options[] = {
 	{ NULL, NULL,		NULL}
 };
 static void setup(void);
-static void cleanup(void) LTP_ATTRIBUTE_NORETURN;
+static void cleanup(void);
 static void overcommit(void);
 static void write_bytes(void *addr);
 static void read_bytes(void *addr);
@@ -135,6 +135,7 @@ int main(int argc, char *argv[])
 		overcommit();
 	}
 	cleanup();
+	tst_exit();
 }
 
 static void overcommit(void)
@@ -154,8 +155,15 @@ static void overcommit(void)
 		if (shmid == -1)
 			tst_brkm(TBROK|TERRNO, cleanup, "shmget");
 	} else {
-		snprintf(s, BUFSIZ, "%s/hugemmap05/file", get_tmp_tstdir());
-		fd = open(s, O_CREAT | O_RDWR, 0755);
+		char *tmp_tstdir;
+
+		tmp_tstdir = get_tst_tmpdir();
+		if (tmp_tstdir == NULL)
+			tst_brkm(TBROK|TERRNO, cleanup, "get_tmp_tstdir failed");
+		snprintf(s, BUFSIZ, "%s/hugemmap05/file", tmp_tstdir);
+		free(tmp_tstdir);
+
+		fd = open(s, O_CREAT|O_RDWR, 0755);
 		if (fd == -1)
 			tst_brkm(TBROK|TERRNO, cleanup, "open");
 		addr = mmap(ADDR, (long)(length * MB), PROTECTION, FLAGS, fd,
@@ -195,19 +203,14 @@ static void overcommit(void)
 			return;
 		fclose(fp);
 	}
-	if (shmid != -1) {
-		tst_resm(TINFO, "shmid: 0x%x", shmid);
-		shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
-		if (shmaddr == (void *)-1)
-			tst_brkm(TBROK|TERRNO, cleanup, "shmat");
-		write_bytes(shmaddr);
-		read_bytes(shmaddr);
-        } else {
-		write_bytes(addr);
-		read_bytes(addr);
-	}
+	tst_resm(TINFO, "shmid: 0x%x", shmid);
+	shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
+	if (shmaddr == (void *)-1)
+		tst_brkm(TBROK|TERRNO, cleanup, "shmat");
+	write_bytes(shmaddr);
+	read_bytes(shmaddr);
 	if (opt_sysfs) {
-		tst_resm(TINFO, "check sysfs.");
+		tst_resm(TINFO, "checking sysfs.");
 		if (checksys(_PATH_SYS_2M_HUGE, "HugePages_Total",
 				length / 2) != 0)
 			return;
@@ -221,7 +224,7 @@ static void overcommit(void)
 			!= 0)
 			return;
 	} else {
-		tst_resm(TINFO, "check /proc/meminfo.");
+		tst_resm(TINFO, "checking /proc/meminfo.");
 		fp = fopen(_PATH_MEMINFO, "r");
 		if (fp == NULL)
 			tst_brkm(TBROK|TERRNO, cleanup, "fopen");
@@ -236,18 +239,17 @@ static void overcommit(void)
 			return;
 		fclose(fp);
 	}
-	if (shmid != -1) {
-		if (shmdt(shmaddr) != 0)
-			tst_brkm(TBROK|TERRNO, cleanup, "shmdt");
-	} else {
-		munmap(addr, (long)(length * MB));
-		close(fd);
-		unlink(s);
-	}
+	shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
+	if (shmaddr == (void *)-1)
+		tst_brkm(TBROK|TERRNO, cleanup, "shmat failed");
+	if (shmdt(shmaddr) != 0)
+		tst_brkm(TBROK|TERRNO, cleanup, "shmdt failed");
 }
 
 static void cleanup(void)
 {
+	char *tmp_tstdir;
+	void *shmaddr;
 	int fd;
 
 	TEST_CLEANUP;
@@ -262,38 +264,49 @@ static void cleanup(void)
 	fd = open(path, O_WRONLY);
 	if (fd == -1)
 		tst_resm(TWARN|TERRNO, "open");
-	tst_resm(TINFO, "restore nr_hugepages to %s.", nr_hugepages);
-	if (write(fd, nr_hugepages,
-			strlen(nr_hugepages)) != strlen(nr_hugepages))
+	tst_resm(TINFO, "restoring nr_hugepages to %s", nr_hugepages);
+	if (write(fd, nr_hugepages, strlen(nr_hugepages)) !=
+	    strlen(nr_hugepages))
 		tst_resm(TWARN|TERRNO, "write");
 	close(fd);
 
 	fd = open(pathover, O_WRONLY);
 	if (fd == -1)
 		tst_resm(TWARN|TERRNO, "open");
-	tst_resm(TINFO, "restore nr_overcommit_hugepages to %s.",
+	tst_resm(TINFO, "restoring nr_overcommit_hugepages to %s.",
 		nr_overcommit_hugepages);
 	if (write(fd, nr_overcommit_hugepages, strlen(nr_overcommit_hugepages))
 		!= strlen(nr_overcommit_hugepages))
 		tst_resm(TWARN|TERRNO, "write");
 	close(fd);
 
-	snprintf(buf, BUFSIZ, "%s/hugemmap05", get_tst_tmpdir());
+	tmp_tstdir = get_tst_tmpdir();
+	if (tmp_tstdir == NULL)
+		tst_brkm(TBROK|TERRNO, NULL, "tmp_tstdir failed");
+	snprintf(buf, BUFSIZ, "%s/hugemmap05", tmp_tstdir);
+	free(tmp_tstdir);
+	shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
+	if (shmaddr == (void *)-1)
+		tst_brkm(TBROK|TERRNO, cleanup, "shmat");
+	else if (shmdt(shmaddr) == -1)
+		tst_resm(TBROK|TERRNO, "shmdt failed");
+	else if (shmctl(shmid, IPC_RMID, NULL) == -1)
+		tst_resm(TBROK|TERRNO,
+		    "shmctl(..., IPC_RMID, ...) failed");
 	if (umount(buf) == -1)
 		tst_resm(TWARN|TERRNO, "umount");
-	if (shmid != -1) {
-		tst_resm(TINFO|TERRNO, "shmdt");
-		shmctl(shmid, IPC_RMID, NULL);
-	}
+	tst_rmdir();
 
 }
 
 static void setup(void)
 {
 	FILE *fp;
+	char *tmp_tstdir;
 	int fd;
 
 	tst_sig(FORK, DEF_HANDLER, cleanup);
+	tst_tmpdir();
 	TEST_PAUSE;
 	if (shmid != -1) {
 		fp = fopen(_PATH_SHMMAX, "r");
@@ -332,7 +345,7 @@ static void setup(void)
 		tst_brkm(TBROK|TERRNO, cleanup, "write");
 	if (lseek(fd, 0, SEEK_SET) == -1)
 		tst_brkm(TBROK|TERRNO, cleanup, "lseek");
-	snprintf(buf, BUFSIZ, "%ld", size);
+	snprintf(buf, BUFSIZ, "%zd", size);
 	if (write(fd, buf, strlen(buf)) != strlen(buf))
 		tst_brkm(TBROK|TERRNO, cleanup,
 			"failed to change nr_hugepages.");
@@ -356,13 +369,18 @@ static void setup(void)
 		tst_brkm(TBROK|TERRNO, cleanup, "write");
 	if (lseek(fd, 0, SEEK_SET) == -1)
 		tst_brkm(TBROK|TERRNO, cleanup, "lseek");
-	snprintf(buf, BUFSIZ, "%ld", size);
+	snprintf(buf, BUFSIZ, "%zd", size);
 	if (write(fd, buf, strlen(buf)) != strlen(buf))
 		tst_brkm(TBROK|TERRNO, cleanup,
 			"failed to change nr_hugepages.");
 	close(fd);
 
-	snprintf(buf, BUFSIZ, "%s/hugemmap05", get_tst_tmpdir());
+	tmp_tstdir = get_tst_tmpdir();
+	if (tmp_tstdir == NULL)
+		tst_brkm(TBROK|TERRNO, NULL, "tmp_tstdir failed");
+	snprintf(buf, BUFSIZ, "%s/hugemmap05", tmp_tstdir);
+	free(tmp_tstdir);
+
 	if (mkdir(buf, 0700) == -1)
 		tst_brkm(TBROK|TERRNO, cleanup, "mkdir");
 	if (mount(NULL, buf, "hugetlbfs", 0, NULL) == -1)
@@ -447,4 +465,4 @@ static int checkproc(FILE *fp, char *pattern, int value)
 		return 1;
 	}
 	return 0;
-}
\ No newline at end of file
+}

[-- Attachment #3: Type: text/plain, Size: 371 bytes --]

------------------------------------------------------------------------------
Learn how Oracle Real Application Clusters (RAC) One Node allows customers
to consolidate database storage, standardize their database environment, and, 
should the need arise, upgrade to a full multi-node Oracle RAC database 
without downtime or disruption
http://p.sf.net/sfu/oracle-sfdevnl

[-- Attachment #4: Type: text/plain, Size: 155 bytes --]

_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] hugemmap05 issues

[CAI Qian]

Hi Garrett,

----- Original Message -----
> Looking over the code, there are a number of flawed assumptions in
> terms of how things are executed and it seems that the code is overly
> complex. Before I go and do something rash though, I wanted others to
> look at the code (in particular the areas I'm touching) and see
> whether or not I'm just blowing something out my rear.
> As-is the code doesn't compile due to warnings.
> This is a WIP patch (please ignore the whitespace and style to a
> small degree as I know gmail will fubar the formatting).
> Thanks,
> -Garrett
> 
> diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
> b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
> index 8893f84..2ea1363 100644
> --- a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
> +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
> @@ -97,7 +97,7 @@ static option_t options[] = {
> { NULL, NULL, NULL}
> };
> static void setup(void);
> -static void cleanup(void) LTP_ATTRIBUTE_NORETURN;
> +static void cleanup(void);
Cyril suggested me to change this way to use LTP_ATTRIBUTE_NORETURN, as
cleanup() will call tst_exit() anyway. What's the issues are you
facing?
> static void overcommit(void);
> static void write_bytes(void *addr);
> static void read_bytes(void *addr);
> @@ -135,6 +135,7 @@ int main(int argc, char *argv[])
> overcommit();
> }
> cleanup();
> + tst_exit();
This is not needed as mentioned before.
> }
> 
> static void overcommit(void)
> @@ -154,8 +155,15 @@ static void overcommit(void)
> if (shmid == -1)
> tst_brkm(TBROK|TERRNO, cleanup, "shmget");
> } else {
> - snprintf(s, BUFSIZ, "%s/hugemmap05/file", get_tmp_tstdir());
> - fd = open(s, O_CREAT | O_RDWR, 0755);
I did not write those code. My code is this,
        } else {
                snprintf(s, BUFSIZ, "%s/hugemmap05/file", TESTDIR);
                fd = open(s, O_CREAT | O_RDWR, 0755);

Are you using the latest patch?
http://marc.info/?l=ltp-list&m=129170706228725&w=2

Again, what issues are your facing with this code?
> + char *tmp_tstdir;
> +
> + tmp_tstdir = get_tst_tmpdir();
> + if (tmp_tstdir == NULL)
> + tst_brkm(TBROK|TERRNO, cleanup, "get_tmp_tstdir failed");
> + snprintf(s, BUFSIZ, "%s/hugemmap05/file", tmp_tstdir);
> + free(tmp_tstdir);
> +
> + fd = open(s, O_CREAT|O_RDWR, 0755);
> if (fd == -1)
> tst_brkm(TBROK|TERRNO, cleanup, "open");
> addr = mmap(ADDR, (long)(length * MB), PROTECTION, FLAGS, fd,
> @@ -195,19 +203,14 @@ static void overcommit(void)
> return;
> fclose(fp);
> }
> - if (shmid != -1) {
> - tst_resm(TINFO, "shmid: 0x%x", shmid);
> - shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
> - if (shmaddr == (void *)-1)
> - tst_brkm(TBROK|TERRNO, cleanup, "shmat");
> - write_bytes(shmaddr);
> - read_bytes(shmaddr);
> - } else {
> - write_bytes(addr);
> - read_bytes(addr);
> - }
As you can read from the help(), the test can allocate hugepages either
from shmget() or mmap(). In case of shmget(), a global variable shmid with
the default value -1 will be change to 1 via command-line,

static option_t options[] = {
        { "s", &opt_sysfs,      NULL},
        { "m", &shmid,          NULL},
        { "a:", &opt_alloc,     &opt_allocstr},
        { NULL, NULL,           NULL}
};

Therefore, there is a need to call shmat(). That is why it only call it
when shmid != -1. In case of mmap(), there is no need to call shmat().
> + tst_resm(TINFO, "shmid: 0x%x", shmid);
> + shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
> + if (shmaddr == (void *)-1)
> + tst_brkm(TBROK|TERRNO, cleanup, "shmat");
> + write_bytes(shmaddr);
> + read_bytes(shmaddr);
> if (opt_sysfs) {
> - tst_resm(TINFO, "check sysfs.");
> + tst_resm(TINFO, "checking sysfs.");
> if (checksys(_PATH_SYS_2M_HUGE, "HugePages_Total",
> length / 2) != 0)
> return;
> @@ -221,7 +224,7 @@ static void overcommit(void)
> != 0)
> return;
> } else {
> - tst_resm(TINFO, "check /proc/meminfo.");
> + tst_resm(TINFO, "checking /proc/meminfo.");
If you are going to change the syntax here, there are many other places
also need to changed to be consistent like,
"check sysfs."
"check /proc/meminfo before allocation."
etc
> fp = fopen(_PATH_MEMINFO, "r");
> if (fp == NULL)
> tst_brkm(TBROK|TERRNO, cleanup, "fopen");
> @@ -236,18 +239,17 @@ static void overcommit(void)
> return;
> fclose(fp);
> }
> - if (shmid != -1) {
> - if (shmdt(shmaddr) != 0)
> - tst_brkm(TBROK|TERRNO, cleanup, "shmdt");
> - } else {
> - munmap(addr, (long)(length * MB));
> - close(fd);
> - unlink(s);
> - }
Same here. We call shmdt only in case of shmget(), and call munmap for
mmap().

Thanks.

CAI Qian
> + shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
> + if (shmaddr == (void *)-1)
> + tst_brkm(TBROK|TERRNO, cleanup, "shmat failed");
> + if (shmdt(shmaddr) != 0)
> + tst_brkm(TBROK|TERRNO, cleanup, "shmdt failed");
> }
> 
> static void cleanup(void)
> {
> + char *tmp_tstdir;
> + void *shmaddr;
> int fd;
> 
> TEST_CLEANUP;
> @@ -262,38 +264,49 @@ static void cleanup(void)
> fd = open(path, O_WRONLY);
> if (fd == -1)
> tst_resm(TWARN|TERRNO, "open");
> - tst_resm(TINFO, "restore nr_hugepages to %s.", nr_hugepages);
> - if (write(fd, nr_hugepages,
> - strlen(nr_hugepages)) != strlen(nr_hugepages))
> + tst_resm(TINFO, "restoring nr_hugepages to %s", nr_hugepages);
> + if (write(fd, nr_hugepages, strlen(nr_hugepages)) !=
> + strlen(nr_hugepages))
> tst_resm(TWARN|TERRNO, "write");
> close(fd);
> 
> fd = open(pathover, O_WRONLY);
> if (fd == -1)
> tst_resm(TWARN|TERRNO, "open");
> - tst_resm(TINFO, "restore nr_overcommit_hugepages to %s.",
> + tst_resm(TINFO, "restoring nr_overcommit_hugepages to %s.",
> nr_overcommit_hugepages);
> if (write(fd, nr_overcommit_hugepages,
> strlen(nr_overcommit_hugepages))
> != strlen(nr_overcommit_hugepages))
> tst_resm(TWARN|TERRNO, "write");
> close(fd);
> 
> - snprintf(buf, BUFSIZ, "%s/hugemmap05", get_tst_tmpdir());
> + tmp_tstdir = get_tst_tmpdir();
> + if (tmp_tstdir == NULL)
> + tst_brkm(TBROK|TERRNO, NULL, "tmp_tstdir failed");
> + snprintf(buf, BUFSIZ, "%s/hugemmap05", tmp_tstdir);
> + free(tmp_tstdir);
> + shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
> + if (shmaddr == (void *)-1)
> + tst_brkm(TBROK|TERRNO, cleanup, "shmat");
> + else if (shmdt(shmaddr) == -1)
> + tst_resm(TBROK|TERRNO, "shmdt failed");
> + else if (shmctl(shmid, IPC_RMID, NULL) == -1)
> + tst_resm(TBROK|TERRNO,
> + "shmctl(..., IPC_RMID, ...) failed");
> if (umount(buf) == -1)
> tst_resm(TWARN|TERRNO, "umount");
> - if (shmid != -1) {
> - tst_resm(TINFO|TERRNO, "shmdt");
> - shmctl(shmid, IPC_RMID, NULL);
> - }
> + tst_rmdir();
> 
> }
> 
> static void setup(void)
> {
> FILE *fp;
> + char *tmp_tstdir;
> int fd;
> 
> tst_sig(FORK, DEF_HANDLER, cleanup);
> + tst_tmpdir();
> TEST_PAUSE;
> if (shmid != -1) {
> fp = fopen(_PATH_SHMMAX, "r");
> @@ -332,7 +345,7 @@ static void setup(void)
> tst_brkm(TBROK|TERRNO, cleanup, "write");
> if (lseek(fd, 0, SEEK_SET) == -1)
> tst_brkm(TBROK|TERRNO, cleanup, "lseek");
> - snprintf(buf, BUFSIZ, "%ld", size);
> + snprintf(buf, BUFSIZ, "%zd", size);
> if (write(fd, buf, strlen(buf)) != strlen(buf))
> tst_brkm(TBROK|TERRNO, cleanup,
> "failed to change nr_hugepages.");
> @@ -356,13 +369,18 @@ static void setup(void)
> tst_brkm(TBROK|TERRNO, cleanup, "write");
> if (lseek(fd, 0, SEEK_SET) == -1)
> tst_brkm(TBROK|TERRNO, cleanup, "lseek");
> - snprintf(buf, BUFSIZ, "%ld", size);
> + snprintf(buf, BUFSIZ, "%zd", size);
> if (write(fd, buf, strlen(buf)) != strlen(buf))
> tst_brkm(TBROK|TERRNO, cleanup,
> "failed to change nr_hugepages.");
> close(fd);
> 
> - snprintf(buf, BUFSIZ, "%s/hugemmap05", get_tst_tmpdir());
> + tmp_tstdir = get_tst_tmpdir();
> + if (tmp_tstdir == NULL)
> + tst_brkm(TBROK|TERRNO, NULL, "tmp_tstdir failed");
> + snprintf(buf, BUFSIZ, "%s/hugemmap05", tmp_tstdir);
> + free(tmp_tstdir);
> +
> if (mkdir(buf, 0700) == -1)
> tst_brkm(TBROK|TERRNO, cleanup, "mkdir");
> if (mount(NULL, buf, "hugetlbfs", 0, NULL) == -1)
> @@ -447,4 +465,4 @@ static int checkproc(FILE *fp, char *pattern, int
> value)
> return 1;
> }
> return 0;
> -}
> \ No newline at end of file
> +}

------------------------------------------------------------------------------
Learn how Oracle Real Application Clusters (RAC) One Node allows customers
to consolidate database storage, standardize their database environment, and, 
should the need arise, upgrade to a full multi-node Oracle RAC database 
without downtime or disruption
http://p.sf.net/sfu/oracle-sfdevnl
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] hugemmap05 issues

[Garrett Cooper]

On Mon, Jan 3, 2011 at 7:09 PM, CAI Qian <caiqian@redhat.com> wrote:
> Hi Garrett,
>
> ----- Original Message -----
>> Looking over the code, there are a number of flawed assumptions in
>> terms of how things are executed and it seems that the code is overly
>> complex. Before I go and do something rash though, I wanted others to
>> look at the code (in particular the areas I'm touching) and see
>> whether or not I'm just blowing something out my rear.
>> As-is the code doesn't compile due to warnings.
>> This is a WIP patch (please ignore the whitespace and style to a
>> small degree as I know gmail will fubar the formatting).
>> Thanks,
>> -Garrett
>>
>> diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
>> b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
>> index 8893f84..2ea1363 100644
>> --- a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
>> +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap05.c
>> @@ -97,7 +97,7 @@ static option_t options[] = {
>> { NULL, NULL, NULL}
>> };
>> static void setup(void);
>> -static void cleanup(void) LTP_ATTRIBUTE_NORETURN;
>> +static void cleanup(void);
> Cyril suggested me to change this way to use LTP_ATTRIBUTE_NORETURN, as
> cleanup() will call tst_exit() anyway. What's the issues are you
> facing?
>> static void overcommit(void);
>> static void write_bytes(void *addr);
>> static void read_bytes(void *addr);
>> @@ -135,6 +135,7 @@ int main(int argc, char *argv[])
>> overcommit();
>> }
>> cleanup();
>> + tst_exit();
> This is not needed as mentioned before.

As Cyril pointed out a few weeks ago (and I agreed), it doesn't make
sense for tst_brkm to not call tst_exit at the end of each call (it
makes things unintuitive API-wise), thus this all trickles down to
calling cleanup, then tst_exit in every test that uses the libltp
APIs. That's why there is so much churn and a fair amount of breakage
right now... I'm working my way down the list -- manually going and
fixing testcases and then verifying them for sanity.

>> static void overcommit(void)
>> @@ -154,8 +155,15 @@ static void overcommit(void)
>> if (shmid == -1)
>> tst_brkm(TBROK|TERRNO, cleanup, "shmget");
>> } else {
>> - snprintf(s, BUFSIZ, "%s/hugemmap05/file", get_tmp_tstdir());
>> - fd = open(s, O_CREAT | O_RDWR, 0755);
> I did not write those code. My code is this,
>        } else {
>                snprintf(s, BUFSIZ, "%s/hugemmap05/file", TESTDIR);
>                fd = open(s, O_CREAT | O_RDWR, 0755);
>
> Are you using the latest patch?
> http://marc.info/?l=ltp-list&m=129170706228725&w=2

TESTDIR isn't publicly visible anymore because people really shouldn't
be mucking around with that value, or querying it if they haven't
called tst_tmpdir (which is actually what you did in this test!).

> Again, what issues are your facing with this code?
>> + char *tmp_tstdir;
>> +
>> + tmp_tstdir = get_tst_tmpdir();
>> + if (tmp_tstdir == NULL)
>> + tst_brkm(TBROK|TERRNO, cleanup, "get_tmp_tstdir failed");
>> + snprintf(s, BUFSIZ, "%s/hugemmap05/file", tmp_tstdir);
>> + free(tmp_tstdir);
>> +
>> + fd = open(s, O_CREAT|O_RDWR, 0755);
>> if (fd == -1)
>> tst_brkm(TBROK|TERRNO, cleanup, "open");
>> addr = mmap(ADDR, (long)(length * MB), PROTECTION, FLAGS, fd,
>> @@ -195,19 +203,14 @@ static void overcommit(void)
>> return;
>> fclose(fp);
>> }
>> - if (shmid != -1) {
>> - tst_resm(TINFO, "shmid: 0x%x", shmid);
>> - shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
>> - if (shmaddr == (void *)-1)
>> - tst_brkm(TBROK|TERRNO, cleanup, "shmat");
>> - write_bytes(shmaddr);
>> - read_bytes(shmaddr);
>> - } else {
>> - write_bytes(addr);
>> - read_bytes(addr);
>> - }
> As you can read from the help(), the test can allocate hugepages either
> from shmget() or mmap(). In case of shmget(), a global variable shmid with
> the default value -1 will be change to 1 via command-line,

Ok, I must have missed that part. Sorry...

> static option_t options[] = {
>        { "s", &opt_sysfs,      NULL},
>        { "m", &shmid,          NULL},
>        { "a:", &opt_alloc,     &opt_allocstr},
>        { NULL, NULL,           NULL}
> };
>
> Therefore, there is a need to call shmat(). That is why it only call it
> when shmid != -1. In case of mmap(), there is no need to call shmat().
>> + tst_resm(TINFO, "shmid: 0x%x", shmid);
>> + shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
>> + if (shmaddr == (void *)-1)
>> + tst_brkm(TBROK|TERRNO, cleanup, "shmat");
>> + write_bytes(shmaddr);
>> + read_bytes(shmaddr);
>> if (opt_sysfs) {
>> - tst_resm(TINFO, "check sysfs.");
>> + tst_resm(TINFO, "checking sysfs.");
>> if (checksys(_PATH_SYS_2M_HUGE, "HugePages_Total",
>> length / 2) != 0)
>> return;
>> @@ -221,7 +224,7 @@ static void overcommit(void)
>> != 0)
>> return;
>> } else {
>> - tst_resm(TINFO, "check /proc/meminfo.");
>> + tst_resm(TINFO, "checking /proc/meminfo.");
> If you are going to change the syntax here, there are many other places
> also need to changed to be consistent like,
> "check sysfs."
> "check /proc/meminfo before allocation."
> etc

Agreed. It was just an example, and the point was to clearly state
with proper verb tenses so that people could get a clear idea of what
the test was doing instead of having to read the code to decypher the
intent of the test writer. Granted, I've gotten lazy and stopped
fixing some of these items in later commits because of all of the LOCs
I've made, but it needs to be fixed later because the grammar and
spelling used by test writers is less than desirable, which makes it
hard for folks trying to understand what the intention is behind these
tests.

>> fp = fopen(_PATH_MEMINFO, "r");
>> if (fp == NULL)
>> tst_brkm(TBROK|TERRNO, cleanup, "fopen");
>> @@ -236,18 +239,17 @@ static void overcommit(void)
>> return;
>> fclose(fp);
>> }
>> - if (shmid != -1) {
>> - if (shmdt(shmaddr) != 0)
>> - tst_brkm(TBROK|TERRNO, cleanup, "shmdt");
>> - } else {
>> - munmap(addr, (long)(length * MB));
>> - close(fd);
>> - unlink(s);
>> - }
> Same here. We call shmdt only in case of shmget(), and call munmap for
> mmap().

Ok -- thanks!
-Garrett

------------------------------------------------------------------------------
Learn how Oracle Real Application Clusters (RAC) One Node allows customers
to consolidate database storage, standardize their database environment, and, 
should the need arise, upgrade to a full multi-node Oracle RAC database 
without downtime or disruption
http://p.sf.net/sfu/oracle-sfdevnl
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] hugemmap05 issues

[CAI Qian]

> TESTDIR isn't publicly visible anymore because people really shouldn't
> be mucking around with that value, or querying it if they haven't
> called tst_tmpdir (which is actually what you did in this test!).
I am sorry about that. That is why I had sent a follow-up patch to address
it,
http://marc.info/?l=ltp-list&m=129379291515012&w=2

> > Again, what issues are your facing with this code?
> >> + char *tmp_tstdir;
> >> +
> >> + tmp_tstdir = get_tst_tmpdir();
> >> + if (tmp_tstdir == NULL)
> >> + tst_brkm(TBROK|TERRNO, cleanup, "get_tmp_tstdir failed");
> >> + snprintf(s, BUFSIZ, "%s/hugemmap05/file", tmp_tstdir);
> >> + free(tmp_tstdir);
> >> +
> >> + fd = open(s, O_CREAT|O_RDWR, 0755);
> >> if (fd == -1)
> >> tst_brkm(TBROK|TERRNO, cleanup, "open");
> >> addr = mmap(ADDR, (long)(length * MB), PROTECTION, FLAGS, fd,
> >> @@ -195,19 +203,14 @@ static void overcommit(void)
> >> return;
> >> fclose(fp);
> >> }
> >> - if (shmid != -1) {
> >> - tst_resm(TINFO, "shmid: 0x%x", shmid);
> >> - shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
> >> - if (shmaddr == (void *)-1)
> >> - tst_brkm(TBROK|TERRNO, cleanup, "shmat");
> >> - write_bytes(shmaddr);
> >> - read_bytes(shmaddr);
> >> - } else {
> >> - write_bytes(addr);
> >> - read_bytes(addr);
> >> - }
> > As you can read from the help(), the test can allocate hugepages
> > either
> > from shmget() or mmap(). In case of shmget(), a global variable
> > shmid with
> > the default value -1 will be change to 1 via command-line,
> 
> Ok, I must have missed that part. Sorry...
> 
> > static option_t options[] = {
> >        { "s", &opt_sysfs, NULL},
> >        { "m", &shmid, NULL},
> >        { "a:", &opt_alloc, &opt_allocstr},
> >        { NULL, NULL, NULL}
> > };
> >
> > Therefore, there is a need to call shmat(). That is why it only call
> > it
> > when shmid != -1. In case of mmap(), there is no need to call
> > shmat().
> >> + tst_resm(TINFO, "shmid: 0x%x", shmid);
> >> + shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
> >> + if (shmaddr == (void *)-1)
> >> + tst_brkm(TBROK|TERRNO, cleanup, "shmat");
> >> + write_bytes(shmaddr);
> >> + read_bytes(shmaddr);
> >> if (opt_sysfs) {
> >> - tst_resm(TINFO, "check sysfs.");
> >> + tst_resm(TINFO, "checking sysfs.");
> >> if (checksys(_PATH_SYS_2M_HUGE, "HugePages_Total",
> >> length / 2) != 0)
> >> return;
> >> @@ -221,7 +224,7 @@ static void overcommit(void)
> >> != 0)
> >> return;
> >> } else {
> >> - tst_resm(TINFO, "check /proc/meminfo.");
> >> + tst_resm(TINFO, "checking /proc/meminfo.");
> > If you are going to change the syntax here, there are many other
> > places
> > also need to changed to be consistent like,
> > "check sysfs."
> > "check /proc/meminfo before allocation."
> > etc
> 
> Agreed. It was just an example, and the point was to clearly state
> with proper verb tenses so that people could get a clear idea of what
> the test was doing instead of having to read the code to decypher the
> intent of the test writer. Granted, I've gotten lazy and stopped
> fixing some of these items in later commits because of all of the LOCs
> I've made, but it needs to be fixed later because the grammar and
> spelling used by test writers is less than desirable, which makes it
> hard for folks trying to understand what the intention is behind these
> tests.
OK.

Thanks.

CAI Qian

------------------------------------------------------------------------------
Learn how Oracle Real Application Clusters (RAC) One Node allows customers
to consolidate database storage, standardize their database environment, and, 
should the need arise, upgrade to a full multi-node Oracle RAC database 
without downtime or disruption
http://p.sf.net/sfu/oracle-sfdevnl
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] hugemmap05 issues

[Garrett Cooper]

    Ack -- gotcha. Too much stuff going on... I'll revert some of the
changes and fix things up a bit.
Thanks,
-Garrett

On Mon, Jan 3, 2011 at 8:48 PM, CAI Qian <caiqian@redhat.com> wrote:
>
>> TESTDIR isn't publicly visible anymore because people really shouldn't
>> be mucking around with that value, or querying it if they haven't
>> called tst_tmpdir (which is actually what you did in this test!).
> I am sorry about that. That is why I had sent a follow-up patch to address
> it,
> http://marc.info/?l=ltp-list&m=129379291515012&w=2
>
>> > Again, what issues are your facing with this code?
>> >> + char *tmp_tstdir;
>> >> +
>> >> + tmp_tstdir = get_tst_tmpdir();
>> >> + if (tmp_tstdir == NULL)
>> >> + tst_brkm(TBROK|TERRNO, cleanup, "get_tmp_tstdir failed");
>> >> + snprintf(s, BUFSIZ, "%s/hugemmap05/file", tmp_tstdir);
>> >> + free(tmp_tstdir);
>> >> +
>> >> + fd = open(s, O_CREAT|O_RDWR, 0755);
>> >> if (fd == -1)
>> >> tst_brkm(TBROK|TERRNO, cleanup, "open");
>> >> addr = mmap(ADDR, (long)(length * MB), PROTECTION, FLAGS, fd,
>> >> @@ -195,19 +203,14 @@ static void overcommit(void)
>> >> return;
>> >> fclose(fp);
>> >> }
>> >> - if (shmid != -1) {
>> >> - tst_resm(TINFO, "shmid: 0x%x", shmid);
>> >> - shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
>> >> - if (shmaddr == (void *)-1)
>> >> - tst_brkm(TBROK|TERRNO, cleanup, "shmat");
>> >> - write_bytes(shmaddr);
>> >> - read_bytes(shmaddr);
>> >> - } else {
>> >> - write_bytes(addr);
>> >> - read_bytes(addr);
>> >> - }
>> > As you can read from the help(), the test can allocate hugepages
>> > either
>> > from shmget() or mmap(). In case of shmget(), a global variable
>> > shmid with
>> > the default value -1 will be change to 1 via command-line,
>>
>> Ok, I must have missed that part. Sorry...
>>
>> > static option_t options[] = {
>> >        { "s", &opt_sysfs, NULL},
>> >        { "m", &shmid, NULL},
>> >        { "a:", &opt_alloc, &opt_allocstr},
>> >        { NULL, NULL, NULL}
>> > };
>> >
>> > Therefore, there is a need to call shmat(). That is why it only call
>> > it
>> > when shmid != -1. In case of mmap(), there is no need to call
>> > shmat().
>> >> + tst_resm(TINFO, "shmid: 0x%x", shmid);
>> >> + shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
>> >> + if (shmaddr == (void *)-1)
>> >> + tst_brkm(TBROK|TERRNO, cleanup, "shmat");
>> >> + write_bytes(shmaddr);
>> >> + read_bytes(shmaddr);
>> >> if (opt_sysfs) {
>> >> - tst_resm(TINFO, "check sysfs.");
>> >> + tst_resm(TINFO, "checking sysfs.");
>> >> if (checksys(_PATH_SYS_2M_HUGE, "HugePages_Total",
>> >> length / 2) != 0)
>> >> return;
>> >> @@ -221,7 +224,7 @@ static void overcommit(void)
>> >> != 0)
>> >> return;
>> >> } else {
>> >> - tst_resm(TINFO, "check /proc/meminfo.");
>> >> + tst_resm(TINFO, "checking /proc/meminfo.");
>> > If you are going to change the syntax here, there are many other
>> > places
>> > also need to changed to be consistent like,
>> > "check sysfs."
>> > "check /proc/meminfo before allocation."
>> > etc
>>
>> Agreed. It was just an example, and the point was to clearly state
>> with proper verb tenses so that people could get a clear idea of what
>> the test was doing instead of having to read the code to decypher the
>> intent of the test writer. Granted, I've gotten lazy and stopped
>> fixing some of these items in later commits because of all of the LOCs
>> I've made, but it needs to be fixed later because the grammar and
>> spelling used by test writers is less than desirable, which makes it
>> hard for folks trying to understand what the intention is behind these
>> tests.
> OK.
>
> Thanks.
>
> CAI Qian
>

------------------------------------------------------------------------------
Learn how Oracle Real Application Clusters (RAC) One Node allows customers
to consolidate database storage, standardize their database environment, and, 
should the need arise, upgrade to a full multi-node Oracle RAC database 
without downtime or disruption
http://p.sf.net/sfu/oracle-sfdevnl
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] hugemmap05 issues

[Garrett Cooper]

On Jan 3, 2011, at 8:48 PM, CAI Qian wrote:

> 
>> TESTDIR isn't publicly visible anymore because people really shouldn't
>> be mucking around with that value, or querying it if they haven't
>> called tst_tmpdir (which is actually what you did in this test!).
> I am sorry about that. That is why I had sent a follow-up patch to address
> it,
> http://marc.info/?l=ltp-list&m=129379291515012&w=2
> 
>>> Again, what issues are your facing with this code?
>>>> + char *tmp_tstdir;
>>>> +
>>>> + tmp_tstdir = get_tst_tmpdir();
>>>> + if (tmp_tstdir == NULL)
>>>> + tst_brkm(TBROK|TERRNO, cleanup, "get_tmp_tstdir failed");
>>>> + snprintf(s, BUFSIZ, "%s/hugemmap05/file", tmp_tstdir);
>>>> + free(tmp_tstdir);
>>>> +
>>>> + fd = open(s, O_CREAT|O_RDWR, 0755);
>>>> if (fd == -1)
>>>> tst_brkm(TBROK|TERRNO, cleanup, "open");
>>>> addr = mmap(ADDR, (long)(length * MB), PROTECTION, FLAGS, fd,
>>>> @@ -195,19 +203,14 @@ static void overcommit(void)
>>>> return;
>>>> fclose(fp);
>>>> }
>>>> - if (shmid != -1) {
>>>> - tst_resm(TINFO, "shmid: 0x%x", shmid);
>>>> - shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
>>>> - if (shmaddr == (void *)-1)
>>>> - tst_brkm(TBROK|TERRNO, cleanup, "shmat");
>>>> - write_bytes(shmaddr);
>>>> - read_bytes(shmaddr);
>>>> - } else {
>>>> - write_bytes(addr);
>>>> - read_bytes(addr);
>>>> - }
>>> As you can read from the help(), the test can allocate hugepages
>>> either
>>> from shmget() or mmap(). In case of shmget(), a global variable
>>> shmid with
>>> the default value -1 will be change to 1 via command-line,
>> 
>> Ok, I must have missed that part. Sorry...
>> 
>>> static option_t options[] = {
>>>        { "s", &opt_sysfs, NULL},
>>>        { "m", &shmid, NULL},
>>>        { "a:", &opt_alloc, &opt_allocstr},
>>>        { NULL, NULL, NULL}
>>> };
>>> 
>>> Therefore, there is a need to call shmat(). That is why it only call
>>> it
>>> when shmid != -1. In case of mmap(), there is no need to call
>>> shmat().
>>>> + tst_resm(TINFO, "shmid: 0x%x", shmid);
>>>> + shmaddr = shmat(shmid, ADDR, SHMAT_FLAGS);
>>>> + if (shmaddr == (void *)-1)
>>>> + tst_brkm(TBROK|TERRNO, cleanup, "shmat");
>>>> + write_bytes(shmaddr);
>>>> + read_bytes(shmaddr);
>>>> if (opt_sysfs) {
>>>> - tst_resm(TINFO, "check sysfs.");
>>>> + tst_resm(TINFO, "checking sysfs.");
>>>> if (checksys(_PATH_SYS_2M_HUGE, "HugePages_Total",
>>>> length / 2) != 0)
>>>> return;
>>>> @@ -221,7 +224,7 @@ static void overcommit(void)
>>>> != 0)
>>>> return;
>>>> } else {
>>>> - tst_resm(TINFO, "check /proc/meminfo.");
>>>> + tst_resm(TINFO, "checking /proc/meminfo.");
>>> If you are going to change the syntax here, there are many other
>>> places
>>> also need to changed to be consistent like,
>>> "check sysfs."
>>> "check /proc/meminfo before allocation."
>>> etc
>> 
>> Agreed. It was just an example, and the point was to clearly state
>> with proper verb tenses so that people could get a clear idea of what
>> the test was doing instead of having to read the code to decypher the
>> intent of the test writer. Granted, I've gotten lazy and stopped
>> fixing some of these items in later commits because of all of the LOCs
>> I've made, but it needs to be fixed later because the grammar and
>> spelling used by test writers is less than desirable, which makes it
>> hard for folks trying to understand what the intention is behind these
>> tests.
> OK.

I applied your fixes but the test is still buggy in the following ways:

1. It has to be run as root (otherwise it fails with EPERM opening the file).
2. Even when the test is run as root it leaves around allocated hugepages, despite the fact that the temporary filesystem has been unmounted.

Please address these issues.
Thanks,
-Garrett
------------------------------------------------------------------------------
Gaining the trust of online customers is vital for the success of any company
that requires sensitive data to be transmitted over the Web.   Learn how to 
best implement a security strategy that keeps consumers' information secure 
and instills the confidence they need to proceed with transactions.
http://p.sf.net/sfu/oracle-sfdevnl 
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] hugemmap05 issues

[CAI Qian]

> I applied your fixes but the test is still buggy in the following
> ways:
> 
> 1. It has to be run as root (otherwise it fails with EPERM opening the
> file).
That is true for lots of LTP tests. To get the most of our of tests, it
requests root privilege. We can even setup config scripts to warning people
about the non-root users. Again, what is the guideline here? We probably
don't want to add many code to each of the test that required root.

> 2. Even when the test is run as root it leaves around allocated
> hugepages, despite the fact that the temporary filesystem has been
> unmounted.
Would you please be elaborate the failure? I can't see why it would work
that way. The cleanup routine does this.

- restore nr_hugepages
- restore nr_overcommit_hugepages
- unlink && umount (mmap case)
- shmdt (shmget case)

Thanks.

CAI Qian

------------------------------------------------------------------------------
Gaining the trust of online customers is vital for the success of any company
that requires sensitive data to be transmitted over the Web.   Learn how to 
best implement a security strategy that keeps consumers' information secure 
and instills the confidence they need to proceed with transactions.
http://p.sf.net/sfu/oracle-sfdevnl 
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] hugemmap05 issues

[Garrett Cooper]

On Mon, Jan 10, 2011 at 4:21 PM, CAI Qian <caiqian@redhat.com> wrote:
>
>> I applied your fixes but the test is still buggy in the following
>> ways:
>>
>> 1. It has to be run as root (otherwise it fails with EPERM opening the
>> file).
> That is true for lots of LTP tests. To get the most of our of tests, it
> requests root privilege. We can even setup config scripts to warning people
> about the non-root users. Again, what is the guideline here? We probably
> don't want to add many code to each of the test that required root.

tst_require_root(NULL) is all you need to do in setup() if you need
it. You can test a lot of EACCES and EPERM cases if you use users
other than root.

Most of the testcases that slack in this area that I've seen are the
newer ones. The older testcases by IBM and SGI do the right thing when
it comes to requiring root privileges.

>> 2. Even when the test is run as root it leaves around allocated
>> hugepages, despite the fact that the temporary filesystem has been
>> unmounted.
> Would you please be elaborate the failure? I can't see why it would work
> that way. The cleanup routine does this.
>
> - restore nr_hugepages
> - restore nr_overcommit_hugepages
> - unlink && umount (mmap case)
> - shmdt (shmget case)

It's a little better now that it's using a temporary directory, but it
still fails when I call it like:

./hugemmap05

on Fedora 13/i386 (first the mmap fails, then the test tries to do a
bunch of stuff in cleanup, including unmount the hugemmap filesystem
-- which fails with EBUSY -- and remove the temporary directory --
which fails because the mountpoint isn't unmounted -- and fails).

Thanks,
-Garrett

------------------------------------------------------------------------------
Gaining the trust of online customers is vital for the success of any company
that requires sensitive data to be transmitted over the Web.   Learn how to 
best implement a security strategy that keeps consumers' information secure 
and instills the confidence they need to proceed with transactions.
http://p.sf.net/sfu/oracle-sfdevnl 
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] hugemmap05 issues

[CAI Qian]

> It's a little better now that it's using a temporary directory, but it
> still fails when I call it like:
> 
> ./hugemmap05
> 
> on Fedora 13/i386 (first the mmap fails, then the test tries to do a
> bunch of stuff in cleanup, including unmount the hugemmap filesystem
> -- which fails with EBUSY -- and remove the temporary directory --
> which fails because the mountpoint isn't unmounted -- and fails).
OK, the most important thing is to figure out why mmap() failed in the
first place. What did it return via strace?

It is hard to ensure the cleanup properly in those situation since it is
un-predictable what went wrong. The cleanup routine just do its best to
cleaning up the system. There was no grantee it would be succeed.

Thanks.

CAI Qian 

------------------------------------------------------------------------------
Gaining the trust of online customers is vital for the success of any company
that requires sensitive data to be transmitted over the Web.   Learn how to 
best implement a security strategy that keeps consumers' information secure 
and instills the confidence they need to proceed with transactions.
http://p.sf.net/sfu/oracle-sfdevnl 
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list