[LTP] [PATCH] hugeshmctl01: Revert "hugeshmctl: Use loop from the API"

[LTP] [PATCH] hugeshmctl01: Revert "hugeshmctl: Use loop from the API"

[Yang Xu]

Currently, when I run this case, it failed as below:
./hugeshmctl01 -i 2
tst_test.c:1229: INFO: Timeout per run is 0h 05m 00s
mem.c:817: INFO: set nr_hugepages to 128
hugeshmctl01.c:198: PASS: pid, size, # of attaches and mode are correct - pass #0
hugeshmctl01.c:198: PASS: pid, size, # of attaches and mode are correct - pass #1
hugeshmctl01.c:263: PASS: new mode and change time are correct
hugeshmctl01.c:280: PASS: shmctl in func_rmid() failed as expected,...
tst_checkpoint.c:149: BROK: hugeshmctl01.c:152: tst_checkpoint_wait(0, 10000):ETIMEDOUT (110)

dmesg
hugeshmctl01[12232]: segfault at 7fe9d0000000 ip 0000000000404041 sp
00007fff69d8a8e0 error 6 in hugeshmctl01[400000+21000]

addr2line -e hugeshmctl01 -f  0000000000404041
stat_setup
/root/ltp/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c:139 (discriminator 4)

test = (stat_time == FIRST) ? set_shmat() : set_shared;

			/* do an assignement for fun */
139line			*(int *)test = i;

Case doesn't reset STAT_TIME(test used a null set_shared, and then test got value from
a null pointer), so it got segfault.

Also, if I reset this STAT_TIME in test_hugeshmctl when i is equal to 0, this case
still fails because shm_id_1 is disabled in func_rmid. If I don't disable shm_id_1,
it will report the following warning
WARNING: shared memory deletion failed
hugetlb.c:150: INFO: WARNING: shared memory deletion failed.
hugetlb.c:151: INFO: This could lead to IPC resource problems.

So I think reverting patch "hugeshmctl: Use loop from the API " for this case maybe a
good choice because old hugeshmctl works fine.

ps: If I am wrong, please correct me.

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 .../mem/hugetlb/hugeshmctl/hugeshmctl01.c     | 53 ++++++++++---------
 1 file changed, 28 insertions(+), 25 deletions(-)

diff --git a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
index e6cf8bf09..03dd46ea7 100644
--- a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
+++ b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (c) International Business Machines  Corp., 2004
- * Copyright (c) Linux Test Project, 2004-2020
+ * Copyright (c) Linux Test Project, 2004-2017
  */
 
 /*
@@ -73,20 +73,37 @@ struct tcase {
 	{IPC_RMID, func_rmid, NULL}
 };
 
-static void test_hugeshmctl(unsigned int i)
+static void test_hugeshmctl(void)
 {
+	unsigned int i;
+
+	/* initialize stat_time */
+	stat_time = FIRST;
+
 	/*
-	 * if needed, set up any required conditions by
-	 * calling the appropriate setup function
+	 * Create a shared memory segment with read and write
+	 * permissions.  Do this here instead of in setup()
+	 * so that looping (-i) will work correctly.
 	 */
-	if (tcases[i].func_setup != NULL)
-		(*tcases[i].func_setup) ();
+	shm_id_1 = shmget(shmkey, shm_size,
+			SHM_HUGETLB | IPC_CREAT | IPC_EXCL | SHM_RW);
+	if (shm_id_1 == -1)
+		tst_brk(TBROK | TERRNO, "shmget #main");
 
-	if (shmctl(shm_id_1, tcases[i].cmd, &buf) == -1) {
-		tst_res(TFAIL | TERRNO, "shmctl #main");
-		return;
+	for (i = 0; i < ARRAY_SIZE(tcases); i++) {
+		/*
+		 * if needed, set up any required conditions by
+		 * calling the appropriate setup function
+		 */
+		if (tcases[i].func_setup != NULL)
+			(*tcases[i].func_setup) ();
+
+		if (shmctl(shm_id_1, tcases[i].cmd, &buf) == -1) {
+			tst_res(TFAIL | TERRNO, "shmctl #main");
+			continue;
+		}
+		(*tcases[i].func_test) ();
 	}
-	(*tcases[i].func_test)();
 }
 
 /*
@@ -296,19 +313,6 @@ void setup(void)
 	shm_size = hpage_size * hugepages / 2;
 	update_shm_size(&shm_size);
 	shmkey = getipckey();
-
-	/* initialize stat_time */
-	stat_time = FIRST;
-
-	/*
-	 * Create a shared memory segment with read and write
-	 * permissions.  Do this here instead of in setup()
-	 * so that looping (-i) will work correctly.
-	 */
-	shm_id_1 = shmget(shmkey, shm_size,
-			SHM_HUGETLB | IPC_CREAT | IPC_EXCL | SHM_RW);
-	if (shm_id_1 == -1)
-		tst_brk(TBROK | TERRNO, "shmget #main");
 }
 
 void cleanup(void)
@@ -318,12 +322,11 @@ void cleanup(void)
 }
 
 static struct tst_test test = {
-	.tcnt = ARRAY_SIZE(tcases),
 	.needs_root = 1,
 	.forks_child = 1,
 	.options = options,
 	.setup = setup,
 	.cleanup = cleanup,
-	.test = test_hugeshmctl,
+	.test_all = test_hugeshmctl,
 	.needs_checkpoints = 1,
 };
-- 
2.18.0

[LTP] [PATCH] hugeshmctl01: Revert "hugeshmctl: Use loop from the API"

[Petr Vorel]

Hi Xu,

I'm sorry for introducing a regression.
Thank you for a report and fixing the test.
I'd personally prefer to keep .tcnt = ARRAY_SIZE(tcases),
that's why I sent patch which just fixes this issue,
but maybe others will prefer to keep loop in test_hugeshmctl()
as it was before my change.

Kind regards,
Petr

[1] https://patchwork.ozlabs.org/patch/1249779/

[LTP] [PATCH] hugeshmctl01: Revert "hugeshmctl: Use loop from the API"

[Li Wang]

On Fri, Mar 6, 2020 at 1:21 AM Petr Vorel <pvorel@suse.cz> wrote:

> ..
> I'd personally prefer to keep .tcnt = ARRAY_SIZE(tcases),
>

+1 Me too. I don't think we should revert the patch. Just fix the problem
and do refactor or improvement will make the code more readable.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200306/be828b3f/attachment.htm>

[LTP] [PATCH] hugeshmctl01: Revert "hugeshmctl: Use loop from the API"

[Petr Vorel]

Hi Li, Xu,
> > I'd personally prefer to keep .tcnt = ARRAY_SIZE(tcases),

> +1 Me too. I don't think we should revert the patch. Just fix the problem
> and do refactor or improvement will make the code more readable.

Thank you both for review and additional info.
Xu, you did the original debugging, so feel free to send a patch which just
fixes the problem.

Kind regards,
Petr

[LTP] [PATCH] hugeshmctl01: Revert "hugeshmctl: Use loop from the API"

[Yang Xu]

Hi Petr,Li


> Hi Li, Xu,
>>> I'd personally prefer to keep .tcnt = ARRAY_SIZE(tcases),
> 
>> +1 Me too. I don't think we should revert the patch. Just fix the problem
>> and do refactor or improvement will make the code more readable.
> 
> Thank you both for review and additional info.
> Xu, you did the original debugging, so feel free to send a patch which just
> fixes the problem.
> 
I plan to refactor this case and make case more cleaner.

Best Regards
Yang Xu
> Kind regards,
> Petr
> 
> 

[LTP] [PATCH v2] hugeshmctl01: Small refactor and remove stat_time

[Yang Xu]

c7a2d296b didn't reset stat_time, thus uninitialized set_shared was
assigned to test variable and test got value from a null pointer,
which leaded to segfault.

hugeshmctl01.c:279: PASS: shmctl in func_rmid() failed as expected, shared memory appears to be removed
tst_checkpoint.c:147: BROK: hugeshmctl01.c:152: tst_checkpoint_wait(0, 10000): ETIMEDOUT (110)
mem.c:817: INFO: set nr_hugepages to 0

dmesg:
segfault at 7f73f8c00000 ip 00000000004051e1 sp 00007ffef375f9a0 error 6 in hugeshmctl01.master[404000+13000]
addr2line -e hugeshmctl01 -f  00000000004051e1
stat_setup
hugeshmctl01.c:139 (discriminator 4)

test = (stat_time == FIRST) ? set_shmat() : set_shared;

/* do an assignement for fun */
*(int *)test = i; // error here

Since the stat_time makes code looks a bit complex, refactor this part instead of resetting it.

Fixes: c7a2d296b ("hugeshmctl: Use loop from the API")

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 .../mem/hugetlb/hugeshmctl/hugeshmctl01.c     | 102 ++++++++----------
 1 file changed, 47 insertions(+), 55 deletions(-)

diff --git a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
index e6cf8bf09..3f985a1b3 100644
--- a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
+++ b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
@@ -35,8 +35,6 @@
 #include <limits.h>
 #include "hugetlb.h"
 
-#define FIRST		0
-#define SECOND		1
 #define N_ATTACH	4U
 #define NEWMODE		0066
 
@@ -44,11 +42,11 @@ static size_t shm_size;
 static int shm_id_1 = -1;
 static struct shmid_ds buf;
 static time_t save_time;
-static int stat_time;
-static void *set_shared;
+static void *attach_to_parent;
 
-static void stat_setup(void);
+static void stat_setup_1(void);
 static void stat_cleanup(void);
+static void stat_setup_2(void);
 static void set_setup(void);
 static void func_stat(void);
 static void func_set(void);
@@ -67,8 +65,8 @@ struct tcase {
 	void (*func_test) (void);
 	void (*func_setup) (void);
 } tcases[] = {
-	{IPC_STAT, func_stat, stat_setup},
-	{IPC_STAT, func_stat, stat_setup},
+	{IPC_STAT, func_stat, stat_setup_1},
+	{IPC_STAT, func_stat, stat_setup_2},
 	{IPC_SET,  func_set,  set_setup},
 	{IPC_RMID, func_rmid, NULL}
 };
@@ -76,9 +74,16 @@ struct tcase {
 static void test_hugeshmctl(unsigned int i)
 {
 	/*
-	 * if needed, set up any required conditions by
-	 * calling the appropriate setup function
+	 * Create a shared memory segment with read and write
+	 * permissions.  Do this here instead of in setup()
+	 * so that looping (-i) will work correctly.
 	 */
+	if (i == 0)
+		shm_id_1 = shmget(shmkey, shm_size,
+			SHM_HUGETLB | IPC_CREAT | IPC_EXCL | SHM_RW);
+	if (shm_id_1 == -1)
+		tst_brk(TBROK | TERRNO, "shmget #main");
+
 	if (tcases[i].func_setup != NULL)
 		(*tcases[i].func_setup) ();
 
@@ -90,9 +95,7 @@ static void test_hugeshmctl(unsigned int i)
 }
 
 /*
- * set_shmat() - Attach the shared memory and return the pointer.  Use
- *		 this seperate routine to avoid code duplication in
- *		 stat_setup() below.
+ * set_shmat() - Attach the shared memory and return the pointer.
  */
 void *set_shmat(void)
 {
@@ -106,35 +109,33 @@ void *set_shmat(void)
 }
 
 /*
- * stat_setup() - Set up for the IPC_STAT command with shmctl().
- *		  Make things interesting by forking some children
- *		  that will either attach or inherit the shared memory.
+ * stat_setup_2() - Set up for the IPC_STAT command with shmctl().
+ * 		  Attach the shared memory to parent process and
+ * 		  some children will inherit the shared memory.
  */
-static void stat_setup(void)
+static void stat_setup_2(void)
+{
+	if (!attach_to_parent)
+		attach_to_parent = set_shmat();
+	stat_setup_1();
+}
+
+/*
+ * stat_setup_1() - Set up for the IPC_STAT command with shmctl().
+ *                some children will inherit or attatch the shared memory.
+ *                It deponds on whther we attach the shared memory
+ *                to parent process.
+ */
+static void stat_setup_1(void)
 {
 	unsigned int i;
 	void *test;
 	pid_t pid;
 
-	/*
-	 * The first time through, let the children attach the memory.
-	 * The second time through, attach the memory first and let
-	 * the children inherit the memory.
-	 */
-
-	if (stat_time == SECOND) {
-		/*
-		 * use the global "set_shared" variable here so that
-		 * it can be removed in the stat_func() routine.
-		 */
-		set_shared = set_shmat();
-	}
-
 	for (i = 0; i < N_ATTACH; i++) {
 		switch (pid = SAFE_FORK()) {
 		case 0:
-			test = (stat_time == FIRST) ? set_shmat() : set_shared;
-
+			test = (attach_to_parent == NULL) ? set_shmat() : attach_to_parent;
 			/* do an assignement for fun */
 			*(int *)test = i;
 
@@ -154,6 +155,7 @@ static void stat_setup(void)
 	}
 }
 
+
 /*
  * func_stat() - check the functionality of the IPC_STAT command with shmctl()
  *		 by looking@the pid of the creator, the segement size,
@@ -162,6 +164,7 @@ static void stat_setup(void)
 static void func_stat(void)
 {
 	pid_t pid;
+	unsigned int num;
 
 	/* check perm, pid, nattach and size */
 	pid = getpid();
@@ -177,12 +180,13 @@ static void func_stat(void)
 	}
 
 	/*
-	 * The first time through, only the children attach the memory, so
-	 * the attaches equal N_ATTACH + stat_time (0).  The second time
-	 * through, the parent attaches the memory and the children inherit
-	 * that memory so the attaches equal N_ATTACH + stat_time (1).
+	 * The first case, only the children attach the memory, so
+	 * the attaches equal N_ATTACH. The second case, the parent
+	 * attaches the memory and the children inherit that memory
+	 * so the attaches equal N_ATTACH + 1.
 	 */
-	if (buf.shm_nattch != N_ATTACH + stat_time) {
+	num = (attach_to_parent == NULL) ? 0 : 1;
+	if (buf.shm_nattch != N_ATTACH + num) {
 		tst_res(TFAIL, "# of attaches is incorrect - %lu",
 			 (unsigned long)buf.shm_nattch);
 		goto fail;
@@ -195,7 +199,7 @@ static void func_stat(void)
 	}
 
 	tst_res(TPASS, "pid, size, # of attaches and mode are correct "
-		 "- pass #%d", stat_time);
+		 "- pass #%d", num);
 
 fail:
 	stat_cleanup();
@@ -220,11 +224,12 @@ static void stat_cleanup(void)
 	for (i = 0; i < N_ATTACH; i++)
 		SAFE_WAIT(&status);
 
-	/* remove the parent's shared memory the second time through */
-	if (stat_time == SECOND)
-		if (shmdt(set_shared) == -1)
+	/* remove the parent's shared memory if we set*/
+	if (attach_to_parent) {
+		if (shmdt(attach_to_parent) == -1)
 			tst_res(TFAIL | TERRNO, "shmdt in stat_cleanup()");
-	stat_time++;
+		attach_to_parent = NULL;
+	}
 }
 
 /*
@@ -296,19 +301,6 @@ void setup(void)
 	shm_size = hpage_size * hugepages / 2;
 	update_shm_size(&shm_size);
 	shmkey = getipckey();
-
-	/* initialize stat_time */
-	stat_time = FIRST;
-
-	/*
-	 * Create a shared memory segment with read and write
-	 * permissions.  Do this here instead of in setup()
-	 * so that looping (-i) will work correctly.
-	 */
-	shm_id_1 = shmget(shmkey, shm_size,
-			SHM_HUGETLB | IPC_CREAT | IPC_EXCL | SHM_RW);
-	if (shm_id_1 == -1)
-		tst_brk(TBROK | TERRNO, "shmget #main");
 }
 
 void cleanup(void)
-- 
2.18.0

[LTP] [PATCH v2] hugeshmctl01: Small refactor and remove stat_time

[Petr Vorel]

Hi Xu,

nice cleanup & refactoring, thanks!

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> @@ -154,6 +155,7 @@ static void stat_setup(void)
>  	}
>  }

> +
nit: this is extra new line (whoever merges this, please remove it).
>  /*
>   * func_stat() - check the functionality of the IPC_STAT command with shmctl()
>   *		 by looking at the pid of the creator, the segement size,

Kind regards,
Petr

[LTP] [PATCH v2] hugeshmctl01: Small refactor and remove stat_time

[Li Wang]

Hi XuYang,

Thanks for the quick work on refactoring. I'd suggest using the new
'.request_hugepages'  in the testcase, would mind having a try?

--- a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
+++ b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
@@ -53,8 +53,6 @@ static void func_set(void);
 static void func_rmid(void);
 static void *set_shmat(void);

-static long hugepages = 128;
-
 static struct tst_option options[] = {
        {"s:", &nr_opt, "-s   num  Set the number of the been allocated
hugepages"},
        {NULL, NULL, NULL}
@@ -290,15 +288,16 @@ void setup(void)
 {
        long hpage_size;

-       save_nr_hugepages();
-       if (nr_opt)
-               hugepages = SAFE_STRTOL(nr_opt, 0, LONG_MAX);
+       if (nr_opt) {
+               tst_hugepages = SAFE_STRTOL(nr_opt, 0, LONG_MAX);
+               tst_request_hugepages(tst_hugepages);
+       }
+
+       if (tst_hugepages == 0)
+               tst_brk(TCONF, "No enough hugepages for testing");

-       limit_hugepages(&hugepages);
-       set_sys_tune("nr_hugepages", hugepages, 1);
        hpage_size = SAFE_READ_MEMINFO("Hugepagesize:") * 1024;
-
-       shm_size = hpage_size * hugepages / 2;
+       shm_size = hpage_size * tst_hugepages / 2;
        update_shm_size(&shm_size);
        shmkey = getipckey();
 }
@@ -306,7 +305,6 @@ void setup(void)
 void cleanup(void)
 {
        rm_shm(shm_id_1);
-       restore_nr_hugepages();
 }

 static struct tst_test test = {
@@ -318,4 +316,5 @@ static struct tst_test test = {
        .cleanup = cleanup,
        .test = test_hugeshmctl,
        .needs_checkpoints = 1,
+       .request_hugepages = 128,
 };


-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200306/4b81feff/attachment.htm>

[LTP] [PATCH v2] hugeshmctl01: Small refactor and remove stat_time

[Yang Xu]

Hi Li

> Hi XuYang,
> 
> Thanks for the quick work on refactoring. I'd suggest using the new 
> '.request_hugepages'? in the testcase, would mind having a try?
OK. But I think we can merge this patch( it has a new line before 
func_stat, remove ..) firstly and then I will use this library api in 
next patch for all related  hugepage cases.

Best Regards
Yang Xu
> 
> --- a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
> +++ b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c
> @@ -53,8 +53,6 @@ static void func_set(void);
>  ?static void func_rmid(void);
>  ?static void *set_shmat(void);
> 
> -static long hugepages = 128;
> -
>  ?static struct tst_option options[] = {
>  ? ? ? ? {"s:", &nr_opt, "-s ? num ?Set the number of the been allocated 
> hugepages"},
>  ? ? ? ? {NULL, NULL, NULL}
> @@ -290,15 +288,16 @@ void setup(void)
>  ?{
>  ? ? ? ? long hpage_size;
> 
> - ? ? ? save_nr_hugepages();
> - ? ? ? if (nr_opt)
> - ? ? ? ? ? ? ? hugepages = SAFE_STRTOL(nr_opt, 0, LONG_MAX);
> + ? ? ? if (nr_opt) {
> + ? ? ? ? ? ? ? tst_hugepages = SAFE_STRTOL(nr_opt, 0, LONG_MAX);
> + ? ? ? ? ? ? ? tst_request_hugepages(tst_hugepages);
> + ? ? ? }
> +
> + ? ? ? if (tst_hugepages == 0)
> + ? ? ? ? ? ? ? tst_brk(TCONF, "No enough hugepages for testing");
> 
> - ? ? ? limit_hugepages(&hugepages);
> - ? ? ? set_sys_tune("nr_hugepages", hugepages, 1);
>  ? ? ? ? hpage_size = SAFE_READ_MEMINFO("Hugepagesize:") * 1024;
> -
> - ? ? ? shm_size = hpage_size * hugepages / 2;
> + ? ? ? shm_size = hpage_size * tst_hugepages / 2;
>  ? ? ? ? update_shm_size(&shm_size);
>  ? ? ? ? shmkey = getipckey();
>  ?}
> @@ -306,7 +305,6 @@ void setup(void)
>  ?void cleanup(void)
>  ?{
>  ? ? ? ? rm_shm(shm_id_1);
> - ? ? ? restore_nr_hugepages();
>  ?}
> 
>  ?static struct tst_test test = {
> @@ -318,4 +316,5 @@ static struct tst_test test = {
>  ? ? ? ? .cleanup = cleanup,
>  ? ? ? ? .test = test_hugeshmctl,
>  ? ? ? ? .needs_checkpoints = 1,
> + ? ? ? .request_hugepages = 128,
>  ?};
> 
> 
> -- 
> Regards,
> Li Wang

[LTP] [PATCH v2] hugeshmctl01: Small refactor and remove stat_time

[Petr Vorel]

Hi Li, Xu,

> > Thanks for the quick work on refactoring. I'd suggest using the new
> > '.request_hugepages'? in the testcase, would mind having a try?
+1

> OK. But I think we can merge this patch( it has a new line before func_stat,
> remove ..) firstly and then I will use this library api in next patch for
> all related  hugepage cases.
Yes, I also think this should be separated, thus merged this fix.

Kind regards,
Petr

[LTP] [PATCH v2] hugeshmctl01: Small refactor and remove stat_time

[Li Wang]

On Mon, Mar 9, 2020 at 12:56 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Li, Xu,
>
> > > Thanks for the quick work on refactoring. I'd suggest using the new
> > > '.request_hugepages'  in the testcase, would mind having a try?
> +1
>
> > OK. But I think we can merge this patch( it has a new line before
> func_stat,
> > remove ..) firstly and then I will use this library api in next patch for
> > all related  hugepage cases.
>

That would be great!

> Yes, I also think this should be separated, thus merged this fix.
>

Thanks Petr for helping merge that.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200309/2b589033/attachment.htm>