Collection of fixes
v2 cover Collection of fixes
Hi,
This is the second posting of that collects fixes for issues encountered when running ltp on internal test platforms. Previous version can be found at [0]. This update addresses all the feedback received on the previous version.
Changes:
v2:
* removed check_hugepage() prototype from hugetlb.h (patch 1)
* hugeshmctl01 - migrate to using checkpoint synchronisation primitives
* sigwaitinfo - use child process to execute test instead of using longjmp based solution
* Dropped patches that are no longer applicable or already merged
If there aren't anymore comments, please consider them for merging.
Thanks,
Punit
[0] http://lists.linux.it/pipermail/ltp/2017-October/006225.html
James Morse (2):
hotplug/cpu_hotplug: Repopulate cgroup:cpusets after testing hotplug hotplug/cpu_hotplug: Remove bashism disown from kill_pid()
Lorenzo Pieralisi (1):
hugeshmget02: add missing SHM_HUGETLB flag on segment creation
Punit Agrawal (4):
Move check_hugepage() helper to mem/lib
hugeshmctl01: Convert to LTP synchronisation primitives
hugeshmctl01: Fix warning about signed/unsigned comparison
sigwaitinfo01: catch SEGV and report success for bad_address2 testcase
Suzuki K. Poulose (3):
hugeshmctl02: Fix allocation size for odd number of hugepages
getdtablesize01: Handle ENFILE errno
perf_event_open: Handle absence of PMU gracefully
Will Deacon (3):
thp: ensure THP/hugetlbfs is available
sigwaitinfo01: fix race between sending and dequeueing RT signals syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'
.../cpu_hotplug/include/cpuhotplug_hotplug.sh | 36 ++++++++++ .../cpu_hotplug/include/cpuhotplug_testsuite.sh | 1 - .../kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c | 77 ++++------------------ .../kernel/mem/hugetlb/hugeshmctl/hugeshmctl02.c | 2 +- .../kernel/mem/hugetlb/hugeshmget/hugeshmget02.c | 3 +- testcases/kernel/mem/hugetlb/lib/hugetlb.c | 6 -- testcases/kernel/mem/hugetlb/lib/hugetlb.h | 3 - testcases/kernel/mem/include/mem.h | 2 + testcases/kernel/mem/lib/mem.c | 6 ++ testcases/kernel/mem/thp/thp01.c | 3 + testcases/kernel/mem/thp/thp02.c | 2 + testcases/kernel/mem/thp/thp03.c | 2 + .../syscalls/getdtablesize/getdtablesize01.c | 17 +++-- testcases/kernel/syscalls/mount/mount03.c | 4 +- .../syscalls/perf_event_open/perf_event_open01.c | 3 +- .../syscalls/perf_event_open/perf_event_open02.c | 4 +- .../kernel/syscalls/sigwaitinfo/sigwaitinfo01.c | 34 +++++++++- 17 files changed, 117 insertions(+), 88 deletions(-)
Signature · 1 line
2.14.2
PATCH v2 1/13 Move check_hugepage() helper to mem/lib
Move the check_hugepage() helper to mem/lib to be more widely available to testcases. Specifically, it will be used in a subsequent commit to check for the presence of hugepage support when running the thp tests.
--- testcases/kernel/mem/hugetlb/lib/hugetlb.c | 6 ------ testcases/kernel/mem/hugetlb/lib/hugetlb.h | 3 --- testcases/kernel/mem/include/mem.h | 2 ++ testcases/kernel/mem/lib/mem.c | 6 ++++++ 4 files changed, 8 insertions(+), 9 deletions(-)
testcases/kernel/mem/hugetlb/lib/hugetlb.c +0 -6
@@ -40,12 +40,6 @@#include <pwd.h> #include "hugetlb.h" -void check_hugepage (void ) -{ -if (access (PATH_HUGEPAGES ,F_OK )) -tst_brk (TCONF ,"Huge page is not supported." ); -} -/* * getipckey() - generates and returns a message key used by the "get" * calls to create an IPC resource.
testcases/kernel/mem/hugetlb/lib/hugetlb.h +0 -3
@@ -45,12 +45,9 @@* from shmid_ds.ipc_perm.mode */ #define MODE_MASK 0x01FF -#define PATH_HUGEPAGES "/sys/kernel/mm/hugepages/"key_t shmkey;/* an IPC key generated by ftok() */ -void check_hugepage (void ); -int getipckey (void );int getuserid (char *user);void rm_shm (int shm_id);
testcases/kernel/mem/include/mem.h +2 -0
@@ -56,8 +56,10 @@ void test_ksm_merge_across_nodes(unsigned long nr_pages);/* HUGETLB */ +#define PATH_HUGEPAGES "/sys/kernel/mm/hugepages/"#define PATH_SHMMAX "/proc/sys/kernel/shmmax" +void check_hugepage (void );void write_memcg (void );/* cpuset/memcg */
testcases/kernel/mem/lib/mem.c +6 -0
@@ -347,6 +347,12 @@ static void verify(char **memory, char value, int proc,free (s); } +void check_hugepage (void ) +{ +if (access (PATH_HUGEPAGES ,F_OK )) +tst_brk (TCONF ,"Huge page is not supported." ); +} +void write_memcg (void ) {SAFE_FILE_PRINTF (MEMCG_LIMIT ,"%ld" ,TESTMEM );
Signature · 1 line
2.14.2
PATCH v2 2/13 thp: ensure THP/hugetlbfs is available
In reply to Punit AgrawalFrom: Will Deacon <[email protected]>
The THP tests can't pass if THP is unavailable or disabled. Add the missing detection to thp01.c.
Also add tests for the presence of hugetlbfs for tests relying on this kernel feature.
--- testcases/kernel/mem/thp/thp01.c | 3 +++ testcases/kernel/mem/thp/thp02.c | 2 ++ testcases/kernel/mem/thp/thp03.c | 2 ++ 3 files changed, 7 insertions(+)
testcases/kernel/mem/thp/thp01.c +3 -0
@@ -116,6 +116,9 @@ static void setup(void)int i;long arg_len, arg_count; +if (access (PATH_THP ,F_OK ) ==-1 ) +tst_brk (TCONF ,"THP not enabled in kernel?" ); + bst =SAFE_MMAP (NULL ,sizeof (*bst),PROT_READ |PROT_WRITE ,MAP_SHARED |MAP_ANONYMOUS ,-1 ,0 );
testcases/kernel/mem/thp/thp02.c +2 -0
@@ -90,6 +90,8 @@ static void setup(void)if (access (PATH_THP ,F_OK ) ==-1 )tst_brk (TCONF ,"THP not enabled in kernel?" ); +check_hugepage (); + ps =sysconf (_SC_PAGESIZE); hps =SAFE_READ_MEMINFO ("Hugepagesize:" ) *1024 ; size = hps *4 ;
testcases/kernel/mem/thp/thp03.c +2 -0
@@ -82,6 +82,8 @@ static void setup(void)if (access (PATH_THP ,F_OK ) ==-1 )tst_brk (TCONF ,"THP not enabled in kernel?" ); +check_hugepage (); + hugepage_size =SAFE_READ_MEMINFO ("Hugepagesize:" ) *KB ; unaligned_size = hugepage_size *4 -1 ; page_size =SAFE_SYSCONF (_SC_PAGESIZE);
Signature · 1 line
2.14.2
PATCH v2 2/13 thp: ensure THP/hugetlbfs is available
Hi!
Quoted 5 lines
testcases/kernel/mem/thp/thp01.c +0
-0
testcases/kernel/mem/thp/thp01.c static void setup(void) · line 124
Quoted 9 lines
testcases/kernel/mem/thp/thp01.c +3
-0@@ -116,6 +116,9 @@ static void setup(void)int i;long arg_len, arg_count; +if (access (PATH_THP ,F_OK ) ==-1 ) +tst_brk (TCONF ,"THP not enabled in kernel?" ); + bst =SAFE_MMAP (NULL ,sizeof (*bst),PROT_READ |PROT_WRITE ,MAP_SHARED |MAP_ANONYMOUS ,-1 ,0 );
I do not get what is the problem with this test on a machine without hugepages or transparent hugepages. The test just tries to execute true with excessively large array of command line arguments, if that fails without hugepages it's a kernel bug.
Quoted 18 lines
testcases/kernel/mem/thp/thp02.c +2
-0@@ -90,6 +90,8 @@ static void setup(void)if (access (PATH_THP ,F_OK ) ==-1 )tst_brk (TCONF ,"THP not enabled in kernel?" ); +check_hugepage (); + ps =sysconf (_SC_PAGESIZE); hps =SAFE_READ_MEMINFO ("Hugepagesize:" ) *1024 ; size = hps *4 ;testcases/kernel/mem/thp/thp03.c +0
-0
testcases/kernel/mem/thp/thp03.c static void setup(void) · line 89
Quoted 8 lines
testcases/kernel/mem/thp/thp03.c +2
-0@@ -82,6 +82,8 @@ static void setup(void)if (access (PATH_THP ,F_OK ) ==-1 )tst_brk (TCONF ,"THP not enabled in kernel?" ); +check_hugepage (); + hugepage_size =SAFE_READ_MEMINFO ("Hugepagesize:" ) *KB ; unaligned_size = hugepage_size *4 -1 ; page_size =SAFE_SYSCONF (_SC_PAGESIZE);
Does this two fail on an attempt to find "Hugepagesize:" in meminfo file? Otherwise I do not see how they can fail.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 2/13 thp: ensure THP/hugetlbfs is available
Hi Cyril,
Thanks for taking a look.
Quoted 9 lines · Cyril Hrubis
Hi!
Quoted 8 lines
testcases/kernel/mem/thp/thp01.c +0
-0@@ -116,6 +116,9 @@ static void setup(void)int i;long arg_len, arg_count;
Quoted 11 lines
testcases/kernel/mem/thp/thp01.c static void setup(void) · line 124
Quoted 6 lines
testcases/kernel/mem/thp/thp01.c +3
-0+if (access (PATH_THP ,F_OK ) ==-1 ) +tst_brk (TCONF ,"THP not enabled in kernel?" ); + bst =SAFE_MMAP (NULL ,sizeof (*bst),PROT_READ |PROT_WRITE ,MAP_SHARED |MAP_ANONYMOUS ,-1 ,0 );I do not get what is the problem with this test on a machine without hugepages or transparent hugepages. The test just tries to execute true with excessively large array of command line arguments, if that fails without hugepages it's a kernel bug.
The test passes even if THP is not enabled on the test kernel. This gives the false impression that THP is working as expected.
The explicit check is making sure the functionality under test is is enabled in the test kernel.
Quoted 22 lines
Quoted 22 lines
testcases/kernel/mem/thp/thp02.c +2
-0@@ -90,6 +90,8 @@ static void setup(void)if (access (PATH_THP ,F_OK ) ==-1 )tst_brk (TCONF ,"THP not enabled in kernel?" ); +check_hugepage (); + ps =sysconf (_SC_PAGESIZE); hps =SAFE_READ_MEMINFO ("Hugepagesize:" ) *1024 ; size = hps *4 ;testcases/kernel/mem/thp/thp03.c +0
-0@@ -82,6 +82,8 @@ static void setup(void)if (access (PATH_THP ,F_OK ) ==-1 )tst_brk (TCONF ,"THP not enabled in kernel?" );
Quoted 8 lines
testcases/kernel/mem/thp/thp03.c static void setup(void) · line 89
Quoted 5 lines
testcases/kernel/mem/thp/thp03.c +2
-0+check_hugepage (); + hugepage_size =SAFE_READ_MEMINFO ("Hugepagesize:" ) *KB ; unaligned_size = hugepage_size *4 -1 ; page_size =SAFE_SYSCONF (_SC_PAGESIZE);Does this two fail on an attempt to find "Hugepagesize:" in meminfo file? Otherwise I do not see how they can fail.
Yes, "Hugepagesize:" goes missing in /proc/meminfo. When the kernel doesn't have hugepage support enabled, the tests fail with message like -
safe_file_ops.c:220: BROK: Expected 1 conversions got 0 at thp03.c:85
With the patch, the message helps identify the cause of the failure.
Thanks,
Punit
PATCH v2 2/13 thp: ensure THP/hugetlbfs is available
Hi!
Quoted 10 lines
Quoted 4 lines
I do not get what is the problem with this test on a machine without hugepages or transparent hugepages. The test just tries to execute true with excessively large array of command line arguments, if that fails without hugepages it's a kernel bug.
The test passes even if THP is not enabled on the test kernel. This gives the false impression that THP is working as expected.
The explicit check is making sure the functionality under test is is enabled in the test kernel.
I see where that came from, but I do not think that the test name is a reason to disable this stress test. However I'm not sure if there is a better name for the test. I guess that we may possibly move it to the cve directory.
Quoted 30 lines
Quoted 21 lines
Quoted 21 lines
testcases/kernel/mem/thp/thp02.c +2
-0@@ -90,6 +90,8 @@ static void setup(void)if (access (PATH_THP ,F_OK ) ==-1 )tst_brk (TCONF ,"THP not enabled in kernel?" ); +check_hugepage (); + ps =sysconf (_SC_PAGESIZE); hps =SAFE_READ_MEMINFO ("Hugepagesize:" ) *1024 ; size = hps *4 ;testcases/kernel/mem/thp/thp03.c +0
-0@@ -82,6 +82,8 @@ static void setup(void)if (access (PATH_THP ,F_OK ) ==-1 )tst_brk (TCONF ,"THP not enabled in kernel?" );Quoted 8 lines
testcases/kernel/mem/thp/thp03.c static void setup(void) · line 89
Quoted 5 lines
testcases/kernel/mem/thp/thp03.c +2
-0+check_hugepage (); + hugepage_size =SAFE_READ_MEMINFO ("Hugepagesize:" ) *KB ; unaligned_size = hugepage_size *4 -1 ; page_size =SAFE_SYSCONF (_SC_PAGESIZE);Does this two fail on an attempt to find "Hugepagesize:" in meminfo file? Otherwise I do not see how they can fail.
Quoted 7 lines
Yes, "Hugepagesize:" goes missing in
/proc/meminfo. When the kernel doesn't have hugepage support enabled, the tests fail with message like -safe_file_ops.c:220: BROK: Expected 1 conversions got 0 at thp03.c:85
With the patch, the message helps identify the cause of the failure.
That's fine. Can you plese include this description in the commit message as well?
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 2/13 thp: ensure THP/hugetlbfs is available
Quoted 9 lines · Cyril Hrubis
Hi!
Quoted 8 lines
Quoted 4 lines
I do not get what is the problem with this test on a machine without hugepages or transparent hugepages. The test just tries to execute true with excessively large array of command line arguments, if that fails without hugepages it's a kernel bug.
The test passes even if THP is not enabled on the test kernel. This gives the false impression that THP is working as expected.
Quoted 8 lines
Quoted 2 lines
The explicit check is making sure the functionality under test is is enabled in the test kernel.
I see where that came from, but I do not think that the test name is a reason to disable this stress test. However I'm not sure if there is a better name for the test. I guess that we may possibly move it to the cve directory.
So you're saying the test should run irrespective of whether THP is enabled or not in the kernel. I'll drop the thp01.c hunk from this patch then.
Quoted 34 lines
Quoted 34 lines
Quoted 21 lines
Quoted 21 lines
testcases/kernel/mem/thp/thp02.c +2
-0@@ -90,6 +90,8 @@ static void setup(void)if (access (PATH_THP ,F_OK ) ==-1 )tst_brk (TCONF ,"THP not enabled in kernel?" ); +check_hugepage (); + ps =sysconf (_SC_PAGESIZE); hps =SAFE_READ_MEMINFO ("Hugepagesize:" ) *1024 ; size = hps *4 ;testcases/kernel/mem/thp/thp03.c +0
-0@@ -82,6 +82,8 @@ static void setup(void)if (access (PATH_THP ,F_OK ) ==-1 )tst_brk (TCONF ,"THP not enabled in kernel?" );Quoted 8 lines
testcases/kernel/mem/thp/thp03.c static void setup(void) · line 89
Quoted 5 lines
testcases/kernel/mem/thp/thp03.c +2
-0+check_hugepage (); + hugepage_size =SAFE_READ_MEMINFO ("Hugepagesize:" ) *KB ; unaligned_size = hugepage_size *4 -1 ; page_size =SAFE_SYSCONF (_SC_PAGESIZE);Does this two fail on an attempt to find "Hugepagesize:" in meminfo file? Otherwise I do not see how they can fail.
Yes, "Hugepagesize:" goes missing in
/proc/meminfo. When the kernel doesn't have hugepage support enabled, the tests fail with message like -
Quoted 6 lines
Quoted 3 lines
safe_file_ops.c:220: BROK: Expected 1 conversions got 0 at thp03.c:85
With the patch, the message helps identify the cause of the failure.
That's fine. Can you plese include this description in the commit message as well?
Done for the next version.
Thanks for the review.
PATCH v2 2/13 thp: ensure THP/hugetlbfs is available
Hi!
Quoted 12 lines
Quoted 11 lines
Quoted 5 lines
The test passes even if THP is not enabled on the test kernel. This gives the false impression that THP is working as expected.
The explicit check is making sure the functionality under test is is enabled in the test kernel.
I see where that came from, but I do not think that the test name is a reason to disable this stress test. However I'm not sure if there is a better name for the test. I guess that we may possibly move it to the cve directory.
Quoted 3 lines
So you're saying the test should run irrespective of whether THP is enabled or not in the kernel. I'll drop the
thp01.chunk from this patch then.
Exactly, it's basically test that tries to pass as large argument list to exec() as possible, the fact that this has been broken with THP enabled does not make it irrelevant for kernel without it.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 3/13 hugeshmctl01: Convert to LTP synchronisation primitives
In reply to Punit Agrawalhugeshmctl01 spawns child processes to perform shm operations and uses signal (SIGUSR1) to communicate the completion of test 1, to the children waiting at sigprocmask(). However, there is no guarantee that a child has reached sigprocmask() before the parent issues a SIGUSR1 and things go wrong.
Fix this by migrating hugeshmctl to LTP synchronisation primitives TST_CHECKPOINT_{WAIT,WAKE,WAIT2} and removing the code that was used for signals based synchronisation.
--- .../kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c | 75 ++++------------------ 1 file changed, 13 insertions(+), 62 deletions(-)
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +13 -62
@@ -56,9 +56,7 @@ static struct shmid_ds buf;static time_t save_time;static int stat_time;static void *set_shared; -static pid_t pid_arr[N_ATTACH ]; -static void sighandler (int sig);static void stat_setup (void );static void stat_cleanup (void );static void set_setup (void );
@@ -141,19 +139,10 @@ void *set_shmat(void)*/ static void stat_setup (void ) { -int i, rval; +int i;void *test;pid_t pid; -sigset_t newmask, oldmask; -struct sigaction sa; - -memset (&sa,'\0' ,sizeof (sa)); - sa.sa_handler = sighandler; - sa.sa_flags =0 ; -TEST (sigaction (SIGUSR1 , &sa,NULL )); -if (TEST_RETURN ==-1 ) -tst_brk (TBROK |TRERRNO , -"SIGSEGV signal setup failed" ); +/* * The first time through, let the children attach the memory. * The second time through, attach the memory first and let
@@ -168,21 +157,6 @@ static void stat_setup(void) set_shared =set_shmat (); } -/* -* Block SIGUSR1 before children pause for a signal -* Doing so to avoid the risk that the parent cleans up -* children by calling stat_cleanup() before children call -* call pause() so that children sleep forever(this is a -* side effect of the arbitrary usleep time below). -* In FIRST, children call shmat. If children sleep forever, -* those attached shm can't be released so some other shm -* tests will fail a lot. -*/ -sigemptyset (&newmask); -sigaddset (&newmask,SIGUSR1 ); -if (sigprocmask (SIG_BLOCK , &newmask, &oldmask) <0 ) -tst_brk (TBROK |TERRNO ,"block SIGUSR1 error" ); -for (i =0 ; i <N_ATTACH ; i++) {switch (pid =SAFE_FORK ()) {case 0 :
@@ -191,39 +165,24 @@ static void stat_setup(void)/* do an assignement for fun */ *(int *)test = i; -/* -* sigsuspend until we get a signal from stat_cleanup() -* use sigsuspend instead of pause to avoid children -* infinite sleep without getting SIGUSR1 from parent -*/ - rval =sigsuspend (&oldmask); -if (rval !=-1 ) -tst_brk (TBROK |TERRNO ,"sigsuspend" ); - -/* -* don't have to block SIGUSR1 any more, -* recover the mask -*/ -if (sigprocmask (SIG_SETMASK , &oldmask,NULL ) <0 ) -tst_brk (TBROK |TERRNO , -"child sigprocmask" ); +/* If last child then wake the parent */ +if (i ==N_ATTACH -1 ) +TST_CHECKPOINT_WAKE (0 ); + +TST_CHECKPOINT_WAIT (1 );/* now we're back - detach the memory and exit */ if (shmdt (test) ==-1 )tst_brk (TBROK |TERRNO ,"shmdt in stat_setup()" ); + +/* Child process quits here */ exit (0 ); -default : -/* save the child's pid for cleanup later */ - pid_arr[i] = pid; } } -/* parent doesn't have to block SIGUSR1, recover the mask */ -if (sigprocmask (SIG_SETMASK , &oldmask,NULL ) <0 ) -tst_brk (TBROK ,"parent sigprocmask" ); - -usleep (250000 ); +/* Wait for all the children to attach to shared memory */ +TST_CHECKPOINT_WAIT (0 ); }/*
@@ -283,11 +242,8 @@ fail:*/ static void stat_cleanup (void ) { -int i; -/* wake up the childern so they can detach the memory and exit */ -for (i =0 ; i <N_ATTACH ; i++) -SAFE_KILL (pid_arr[i],SIGUSR1 ); +TST_CHECKPOINT_WAKE2 (1 ,N_ATTACH );/* remove the parent's shared memory the second time through */ if (stat_time ==SECOND )
@@ -296,12 +252,6 @@ static void stat_cleanup(void) stat_time++; } -static void sighandler (int sig) -{ -if (sig !=SIGUSR1 ) -tst_res (TFAIL ,"received unexpected signal %d" , sig); -} -/* * set_setup() - set up for the IPC_SET command with shmctl() */
@@ -387,4 +337,5 @@ static struct tst_test test = { .setup = setup, .cleanup = cleanup, .test_all = test_hugeshmctl, + .needs_checkpoints = 1, };
Signature · 1 line
2.14.2
PATCH v2 3/13 hugeshmctl01: Convert to LTP synchronisation primitives
Hi!
Quoted 60 lines
static void
stat_setup(void);
static voidstat_cleanup(void);
static voidset_setup(void);testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +2
-35@@ -141,19 +139,10 @@ void *set_shmat(void)*/ static void stat_setup (void ) { -int i, rval; +int i;void *test;pid_t pid; -sigset_t newmask, oldmask; -struct sigaction sa; - -memset (&sa,'\0' ,sizeof (sa)); - sa.sa_handler = sighandler; - sa.sa_flags =0 ; -TEST (sigaction (SIGUSR1 , &sa,NULL )); -if (TEST_RETURN ==-1 ) -tst_brk (TBROK |TRERRNO , -"SIGSEGV signal setup failed" ); +/* * The first time through, let the children attach the memory. * The second time through, attach the memory first and let @@ -168,21 +157,6 @@ static void stat_setup(void) set_shared =set_shmat (); } -/* -* Block SIGUSR1 before children pause for a signal -* Doing so to avoid the risk that the parent cleans up -* children by calling stat_cleanup() before children call -* call pause() so that children sleep forever(this is a -* side effect of the arbitrary usleep time below). -* In FIRST, children call shmat. If children sleep forever, -* those attached shm can't be released so some other shm -* tests will fail a lot. -*/ -sigemptyset (&newmask); -sigaddset (&newmask,SIGUSR1 ); -if (sigprocmask (SIG_BLOCK , &newmask, &oldmask) <0 ) -tst_brk (TBROK |TERRNO ,"block SIGUSR1 error" ); -for (i =0 ; i <N_ATTACH ; i++) {switch (pid =SAFE_FORK ()) {case 0 :@@ -191,39 +165,24 @@ static void stat_setup(void)/* do an assignement for fun */ *(int *)test = i; -/* -* sigsuspend until we get a signal from stat_cleanup() -* use sigsuspend instead of pause to avoid children -* infinite sleep without getting SIGUSR1 from parent -*/ - rval =sigsuspend (&oldmask); -if (rval !=-1 ) -tst_brk (TBROK |TERRNO ,"sigsuspend" ); -
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c static void stat_setup(void) · line 170
Quoted 10 lines
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +3
-7-/* -* don't have to block SIGUSR1 any more, -* recover the mask -*/ -if (sigprocmask (SIG_SETMASK , &oldmask,NULL ) <0 ) -tst_brk (TBROK |TERRNO , -"child sigprocmask" ); +/* If last child then wake the parent */ +if (i ==N_ATTACH -1 ) +TST_CHECKPOINT_WAKE (0 );
As far as I can tell the assumption that the children are executed in any order does not hold at all. So the parent should wait for _each_ of them separately.
I guess that we have to suspend the parent process with TST_CHECKPOINT_WAIT() after each fork() and wait till the children wakes up the parent before we attempt to fork next child.
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c static void stat_setup(void) · line 179
Quoted 8 lines
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +3
-0+TST_CHECKPOINT_WAIT (1 );/* now we're back - detach the memory and exit */ if (shmdt (test) ==-1 )tst_brk (TBROK |TERRNO ,"shmdt in stat_setup()" ); + +/* Child process quits here */
Please avoid adding obvious comments like this one. The original code is not a good example and should be cleaned up.
Quoted 18 lines
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +2
-8exit (0 ); -default : -/* save the child's pid for cleanup later */ - pid_arr[i] = pid; } } -/* parent doesn't have to block SIGUSR1, recover the mask */ -if (sigprocmask (SIG_SETMASK , &oldmask,NULL ) <0 ) -tst_brk (TBROK ,"parent sigprocmask" ); - -usleep (250000 ); +/* Wait for all the children to attach to shared memory */ +TST_CHECKPOINT_WAIT (0 ); }/*
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c fail: · line 246
Quoted 9 lines
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +1
-4@@ -283,11 +242,8 @@ fail:*/ static void stat_cleanup (void ) { -int i; -/* wake up the childern so they can detach the memory and exit */ -for (i =0 ; i <N_ATTACH ; i++) -SAFE_KILL (pid_arr[i],SIGUSR1 ); +TST_CHECKPOINT_WAKE2 (1 ,N_ATTACH );
We should still SAFE_WAIT() each of the processses here otherwise we cannot guarantee that the memory was detached and that the child really exitted.
Quoted 24 lines
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +1
-6/* remove the parent's shared memory the second time through */ if (stat_time ==SECOND )@@ -296,12 +252,6 @@ static void stat_cleanup(void) stat_time++; } -static void sighandler (int sig) -{ -if (sig !=SIGUSR1 ) -tst_res (TFAIL ,"received unexpected signal %d" , sig); -} -/* * set_setup() - set up for the IPC_SET command with shmctl() */ @@ -387,4 +337,5 @@ static struct tst_test test = { .setup = setup, .cleanup = cleanup, .test_all = test_hugeshmctl, + .needs_checkpoints = 1, };Signature · 1 line
2.14.2
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 3/13 hugeshmctl01: Convert to LTP synchronisation primitives
Quoted 67 lines · Cyril Hrubis
Hi!
Quoted 66 lines
static void
stat_setup(void);
static voidstat_cleanup(void);
static voidset_setup(void);testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +2
-41@@ -141,19 +139,10 @@ void *set_shmat(void)*/ static void stat_setup (void ) { -int i, rval; +int i;void *test;pid_t pid; -sigset_t newmask, oldmask; -struct sigaction sa; - -memset (&sa,'\0' ,sizeof (sa)); - sa.sa_handler = sighandler; - sa.sa_flags =0 ; -TEST (sigaction (SIGUSR1 , &sa,NULL )); -if (TEST_RETURN ==-1 ) -tst_brk (TBROK |TRERRNO , -"SIGSEGV signal setup failed" ); +/* * The first time through, let the children attach the memory. * The second time through, attach the memory first and let @@ -168,21 +157,6 @@ static void stat_setup(void) set_shared =set_shmat (); } -/* -* Block SIGUSR1 before children pause for a signal -* Doing so to avoid the risk that the parent cleans up -* children by calling stat_cleanup() before children call -* call pause() so that children sleep forever(this is a -* side effect of the arbitrary usleep time below). -* In FIRST, children call shmat. If children sleep forever, -* those attached shm can't be released so some other shm -* tests will fail a lot. -*/ -sigemptyset (&newmask); -sigaddset (&newmask,SIGUSR1 ); -if (sigprocmask (SIG_BLOCK , &newmask, &oldmask) <0 ) -tst_brk (TBROK |TERRNO ,"block SIGUSR1 error" ); -for (i =0 ; i <N_ATTACH ; i++) {switch (pid =SAFE_FORK ()) {case 0 :@@ -191,39 +165,24 @@ static void stat_setup(void)/* do an assignement for fun */ *(int *)test = i; -/* -* sigsuspend until we get a signal from stat_cleanup() -* use sigsuspend instead of pause to avoid children -* infinite sleep without getting SIGUSR1 from parent -*/ - rval =sigsuspend (&oldmask); -if (rval !=-1 ) -tst_brk (TBROK |TERRNO ,"sigsuspend" ); - -/* -* don't have to block SIGUSR1 any more, -* recover the mask -*/ -if (sigprocmask (SIG_SETMASK , &oldmask,NULL ) <0 ) - tst_brk(TBROK | TERRNO,
Quoted 8 lines
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c static void stat_setup(void) · line 170
Quoted 4 lines
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +3
-1- "child sigprocmask"); +/* If last child then wake the parent */ +if (i ==N_ATTACH -1 ) +TST_CHECKPOINT_WAKE (0 );As far as I can tell the assumption that the children are executed in any order does not hold at all. So the parent should wait for _each_ of them separately.
Good catch. I see the problem with the patch in the scenario where the N-1 th child executes TST_CHECKPOINT_WAKE(0) before the other children have progressed past the write to "test".
I'll fix it as per your suggestion below.
Quoted 8 lines
I guess that we have to suspend the parent process with
TST_CHECKPOINT_WAIT()after eachfork()and wait till the children wakes up the parent before we attempt to fork next child.Quoted 2 lines
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +1
-0+TST_CHECKPOINT_WAIT (1 );
Quoted 9 lines
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c static void stat_setup(void) · line 179
Quoted 6 lines
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +2
-0/* now we're back - detach the memory and exit */ if (shmdt (test) ==-1 )tst_brk (TBROK |TERRNO ,"shmdt in stat_setup()" ); + +/* Child process quits here */ Please avoid adding obvious comments like this one. The original code is not a good example and should be cleaned up.
Dropped :)
Quoted 17 lines
Quoted 17 lines
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +2
-8exit (0 ); -default : -/* save the child's pid for cleanup later */ - pid_arr[i] = pid; } } -/* parent doesn't have to block SIGUSR1, recover the mask */ -if (sigprocmask (SIG_SETMASK , &oldmask,NULL ) <0 ) -tst_brk (TBROK ,"parent sigprocmask" ); - -usleep (250000 ); +/* Wait for all the children to attach to shared memory */ +TST_CHECKPOINT_WAIT (0 ); }
Quoted 15 lines
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c fail: · line 246
Quoted 11 lines
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +1
-4/* @@ -283,11 +242,8 @@ fail:*/ static void stat_cleanup (void ) { -int i; -/* wake up the childern so they can detach the memory and exit */ -for (i =0 ; i <N_ATTACH ; i++) -SAFE_KILL (pid_arr[i],SIGUSR1 ); +TST_CHECKPOINT_WAKE2 (1 ,N_ATTACH );We should still
SAFE_WAIT()each of the processses here otherwise we cannot guarantee that the memory was detached and that the child really exitted.
Makes sense. I've added a SAFE_WAITPID() after the TST_CHECKPOINT_WAKE2().
I'll wait for you to go through the remaining patches before sending an update.
Thanks for the review.
Punit
Quoted 25 lines
Quoted 25 lines
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +1
-6/* remove the parent's shared memory the second time through */ if (stat_time ==SECOND )@@ -296,12 +252,6 @@ static void stat_cleanup(void) stat_time++; } -static void sighandler (int sig) -{ -if (sig !=SIGUSR1 ) -tst_res (TFAIL ,"received unexpected signal %d" , sig); -} -/* * set_setup() - set up for the IPC_SET command with shmctl() */ @@ -387,4 +337,5 @@ static struct tst_test test = { .setup = setup, .cleanup = cleanup, .test_all = test_hugeshmctl, + .needs_checkpoints = 1, };Signature · 1 line
2.14.2
PATCH v2 4/13 hugeshmctl01: Fix warning about signed/unsigned comparison
In reply to Punit AgrawalThe macro N_ATTACH is assumed to be a signed intenger which results in the below warning when compiling -
warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
if (buf.shm_nattch != N_ATTACH + stat_time) {
^~Fix this by explicitly marking N_ATTACH to be an unsigned integer and fixing the resulting fallout.
--- testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +2 -2
@@ -47,7 +47,7 @@#define FIRST 0#define SECOND 1 -#define N_ATTACH 4 +#define N_ATTACH 4U#define NEWMODE 0066static size_t shm_size;
@@ -139,7 +139,7 @@ void *set_shmat(void)*/ static void stat_setup (void ) { -int i; +unsigned int i;void *test;pid_t pid;
Signature · 1 line
2.14.2
PATCH v2 5/13 hugeshmctl02: Fix allocation size for odd number of hugepages
In reply to Punit AgrawalFrom: "Suzuki K. Poulose" <[email protected]>
The test tries to allocate 2 * (half the number of available hugepages). If we have odd number hugepages, the calculated allocation size is not page-aligned and allocation fails.
e.g, (use -s option to trigger it manually)
$ hugeshmctl02 -s 7 -i 5 hugeshmctl02 0 TINFO : set nr_hugepages to 7 hugeshmctl02 1 TBROK : hugeshmctl02.c:153: shmget #2: errno=ENOMEM(12): Cannot allocate memory hugeshmctl02 2 TBROK : hugeshmctl02.c:153: Remaining cases broken hugeshmctl02 0 TINFO : set nr_hugepages to 0
Align the size to hugepage_size.
--- testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl02.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl02.c +1 -1
@@ -101,7 +101,7 @@ static void setup(void)set_sys_tune ("nr_hugepages" , hugepages,1 ); hpage_size =SAFE_READ_MEMINFO ("Hugepagesize:" ) *1024 ; - shm_size = hpage_size * hugepages /2 ; + shm_size = hpage_size * (hugepages /2 );update_shm_size (&shm_size); shmkey =getipckey ();
Signature · 1 line
2.14.2
PATCH v2 6/13 hugeshmget02: add missing SHM_HUGETLB flag on segment creation
In reply to Punit AgrawalFrom: Lorenzo Pieralisi <[email protected]>
On test setup, hugeshmget02 detects the number of huge pages available and uses that number to size a shared memory segment that is allocated to carry out subsequent syscalls checks.
The setup() function detects the number of huge pages available so that it can size the share memory segment to allocate with a reasonable size value, but fails to pass the SHM_HUGETLB on shared memory segment creation which defeats the whole purpose of detecting the available number of huge pages before creating the segment.
This omission can result in test failures, eg:
hugeshmget02 0 TINFO : set nr_hugepages to 128 hugeshmget02 0 TINFO : Using 21 hugepages hugeshmget02 1 TBROK : hugeshmget02.c:155: shmget #setup: errno=ENOMEM(12): Cannot allocate memory hugeshmget02 2 TBROK : hugeshmget02.c:155: Remaining cases broken hugeshmget02 0 TINFO : set nr_hugepages to 0
This patch adds the missing SHM_HUGETLB flag to the shmget call in the setup() function.
--- testcases/kernel/mem/hugetlb/hugeshmget/hugeshmget02.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
testcases/kernel/mem/hugetlb/hugeshmget/hugeshmget02.c +2 -1
@@ -99,7 +99,8 @@ void setup(void) shmkey =getipckey (); shmkey2 = shmkey +1 ; - shm_id_1 =shmget (shmkey, shm_size,IPC_CREAT |IPC_EXCL |SHM_RW ); + 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 #setup" ); }
Signature · 1 line
2.14.2
PATCH v2 7/13 sigwaitinfo01: fix race between sending and dequeueing RT signals
In reply to Punit AgrawalFrom: Will Deacon <[email protected]>
Although RT signals are dequeued in order, there is a race where only the higher signal may have been sent and will therefore be dequeued first, leading to a false failure.
This patch waits on the signal sending tasks so that we can be sure both of the signals have been sent before dequeuing them.
--- testcases/kernel/syscalls/sigwaitinfo/sigwaitinfo01.c | 5 +++++ 1 file changed, 5 insertions(+)
testcases/kernel/syscalls/sigwaitinfo/sigwaitinfo01.c +5 -0
@@ -249,6 +249,7 @@ void test_masked_matching_rt(swi_func sigwaitinfo, int signo)sigset_t sigs, oldmask;siginfo_t si;pid_t child[2 ]; +int status; signo =SIGRTMIN +1 ;
@@ -268,6 +269,10 @@ void test_masked_matching_rt(swi_func sigwaitinfo, int signo) child[0 ] =create_sig_proc (0 , signo,1 ); child[1 ] =create_sig_proc (0 , signo +1 ,1 ); +/* Ensure that the signals have been sent */ +waitpid (child[0 ], &status,0 ); +waitpid (child[1 ], &status,0 ); +TEST (sigwaitinfo (&sigs, &si,NULL ));REPORT_SUCCESS_COND (signo,0 , si.si_pid == child[0 ] && si.si_code == SI_USER
Signature · 1 line
2.14.2
PATCH v2 8/13 sigwaitinfo01: catch SEGV and report success for bad_address2 testcase
In reply to Punit AgrawalThe bad_address2 testcase passes (void *)1 as a sigset pointer to the sigwaitinfo syscall. Unsurprisingly, this segfaults in libc rather than returning -1 (errno = EFAULT) as LTP expects.
Instead, fork a child process which registers a SIGSEGV handler to catch any SEGV's generated by the sigwaitinfo syscall. The signal handler exits the child process with a return code which is checked by the parent to evaluate test outcome.
--- .../kernel/syscalls/sigwaitinfo/sigwaitinfo01.c | 29 ++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)
testcases/kernel/syscalls/sigwaitinfo/sigwaitinfo01.c +27 -2
@@ -361,10 +361,35 @@ void test_bad_address(swi_func sigwaitinfo, int signo)kill (child,SIGTERM ); } +static void segv_handler (int signo) +{ +/* SIGSEGV is expected, exit with 0xff */ +_exit (0xff ); +} +void test_bad_address2 (swi_func sigwaitinfo,int signo) { -TEST (sigwaitinfo ((void *)1 ,NULL ,NULL )); -REPORT_SUCCESS (-1 ,EFAULT ); +pid_t pid; +int status; + +switch (pid =FORK_OR_VFORK ()) { +case -1 : +tst_brkm (TBROK |TERRNO ,NULL ,"fork() failed" ); +case 0 : +signal (SIGSEGV , segv_handler); +TEST (sigwaitinfo ((void *)1 ,NULL ,NULL )); + +_exit (0 ); +break ; +default : +break ; + } + +SUCCEED_OR_DIE (waitpid,"waitpid failed" , pid, &status,0 ); +if (WIFEXITED (status) &&WEXITSTATUS (status) ==0xff ) +tst_resm (TPASS ,"Test passed" ); +else +tst_resm (TFAIL ,"Unrecognised child exit code" ); }void test_bad_address3 (swi_func sigwaitinfo,int signo)
Signature · 1 line
2.14.2
PATCH v2 8/13 sigwaitinfo01: catch SEGV and report success for bad_address2 testcase
Hi!
Quoted 13 lines
void
test_bad_address2(swi_func sigwaitinfo, int signo){testcases/kernel/syscalls/sigwaitinfo/sigwaitinfo01.c +9
-2-TEST (sigwaitinfo ((void *)1 ,NULL ,NULL )); -REPORT_SUCCESS (-1 ,EFAULT ); +pid_t pid; +int status; + +switch (pid =FORK_OR_VFORK ()) { +case -1 : +tst_brkm (TBROK |TERRNO ,NULL ,"fork() failed" ); +case 0 : +signal (SIGSEGV , segv_handler); +TEST (sigwaitinfo ((void *)1 ,NULL ,NULL ));
testcases/kernel/syscalls/sigwaitinfo/sigwaitinfo01.c void test_bad_address(swi_func sigwaitinfo, int signo) · line 390
Quoted 10 lines
testcases/kernel/syscalls/sigwaitinfo/sigwaitinfo01.c +10
-0+ +_exit (0 ); +break ; +default : +break ; + } + +SUCCEED_OR_DIE (waitpid,"waitpid failed" , pid, &status,0 ); +if (WIFEXITED (status) &&WEXITSTATUS (status) ==0xff ) +tst_resm (TPASS ,"Test passed" );
Why don't we just check for WIFSIGNALED() and WTERMSIG() instead?
We do not have to install any handler that way...
Quoted 8 lines
testcases/kernel/syscalls/sigwaitinfo/sigwaitinfo01.c +2
-0+else +tst_resm (TFAIL ,"Unrecognised child exit code" ); }void test_bad_address3 (swi_func sigwaitinfo,int signo)Signature · 1 line
2.14.2
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 8/13 sigwaitinfo01: catch SEGV and report success for bad_address2 testcase
Quoted 27 lines · Cyril Hrubis
Hi!
Quoted 13 lines
void
test_bad_address2(swi_func sigwaitinfo, int signo){testcases/kernel/syscalls/sigwaitinfo/sigwaitinfo01.c +9
-2-TEST (sigwaitinfo ((void *)1 ,NULL ,NULL )); -REPORT_SUCCESS (-1 ,EFAULT ); +pid_t pid; +int status; + +switch (pid =FORK_OR_VFORK ()) { +case -1 : +tst_brkm (TBROK |TERRNO ,NULL ,"fork() failed" ); +case 0 : +signal (SIGSEGV , segv_handler); +TEST (sigwaitinfo ((void *)1 ,NULL ,NULL ));testcases/kernel/syscalls/sigwaitinfo/sigwaitinfo01.c void test_bad_address(swi_func sigwaitinfo, int signo) · line 390
Quoted 10 lines
testcases/kernel/syscalls/sigwaitinfo/sigwaitinfo01.c +10
-0+ +_exit (0 ); +break ; +default : +break ; + } + +SUCCEED_OR_DIE (waitpid,"waitpid failed" , pid, &status,0 ); +if (WIFEXITED (status) &&WEXITSTATUS (status) ==0xff ) +tst_resm (TPASS ,"Test passed" );Why don't we just check for
WIFSIGNALED()andWTERMSIG()instead?
Quoted 1 line
We do not have to install any handler that way...
I think that is what I tried initially but for the tst_sig() call in setup() that installs a default handler and catches the SIGSEGV.
Quoted 9 lines
Quoted 9 lines
testcases/kernel/syscalls/sigwaitinfo/sigwaitinfo01.c +2
-0+else +tst_resm (TFAIL ,"Unrecognised child exit code" ); }void test_bad_address3 (swi_func sigwaitinfo,int signo)Signature · 1 line
2.14.2
PATCH v2 8/13 sigwaitinfo01: catch SEGV and report success for bad_address2 testcase
Hi!
Quoted 6 lines
Quoted 3 lines
Why don't we just check for
WIFSIGNALED()andWTERMSIG()instead?We do not have to install any handler that way...
I think that is what I tried initially but for the
tst_sig()call insetup()that installs a default handler and catches the SIGSEGV.
Then just do signal(SIGSEGV, SIGDFL) in the child just before do do the test. Also it does not make sense to use FORK_OR_VFORK() there because the thest will not work on uClinux anyway, just use plain old fork() instead.
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
testcases/kernel/syscalls/sigwaitinfo/sigwaitinfo01.c +2
-0+else +tst_resm (TFAIL ,"Unrecognised child exit code" ); }void test_bad_address3 (swi_func sigwaitinfo,int signo)Signature · 1 line
2.14.2
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 9/13 syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'
In reply to Punit AgrawalFrom: Will Deacon <[email protected]>
When testing the MS_NOSUID mount flag, mount03 forgets to copy setuid_test into the new filesystem. Instead it writes 'TEST FILE' into a new file and attempts to execute it with S_ISUID. This fails, but not for the reasons ltp expects.
--- testcases/kernel/syscalls/mount/mount03.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
testcases/kernel/syscalls/mount/mount03.c +3 -1
@@ -261,8 +261,10 @@ int test_rwflag(int i, int cnt)case 5 :/* Validate MS_NOSUID flag of mount call */ +tst_resource_copy (__FILE__, __LINE__, cleanup,"setuid_test" , + path_name); +snprintf (file,PATH_MAX ,"%ssetuid_test" , path_name); -SAFE_FILE_PRINTF (cleanup, file,"TEST FILE" );SAFE_STAT (cleanup, file, &file_stat);
Signature · 1 line
2.14.2
PATCH v2 9/13 syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'
Hi!
Quoted 4 lines
When testing the
MS_NOSUIDmount flag, mount03 forgets to copysetuid_testinto the new filesystem. Instead it writes 'TEST FILE' into a new file and attempts to execute it withS_ISUID. This fails, but not for the reasons ltp expects.
What is the exact error here? I guess that the file does not have execute bit set and that check kicks in first?
Anyway we really should check the errno from the exec() is EPERM which should have caught this problem.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 9/13 syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'
Quoted 8 lines · Cyril Hrubis
Hi!
Quoted 4 lines
When testing the
MS_NOSUIDmount flag, mount03 forgets to copysetuid_testinto the new filesystem. Instead it writes 'TEST FILE' into a new file and attempts to execute it withS_ISUID. This fails, but not for the reasons ltp expects.What is the exact error here? I guess that the file does not have execute bit set and that check kicks in first?
I have a feeling that we might be cross-talking on this one.
IIUC, the intention for the test is to try and execute the binary setuid_test (built from setuid_test.c). Instead, the test creates a file named "setuid_test" at the mount location and writes "TEST FILE" into it.
snprintf(file, PATH_MAX, "%ssetuid_test", path_name);
SAFE_FILE_PRINTF(cleanup, file, "TEST FILE");It then tries to execute this file containing the text string which fails with the message -
/bin/sh: 0: Can't open /tmp/mouc0EQuB/mntpoint/setuid_test
mount03 6 TPASS : mount(2) passed with rwflag = 2
The patch copies the file setuid_test into the mounted filesystem at which point we no longer get the "/bin/sh:" message.
Quoted 3 lines
Anyway we really should check the errno from the
exec()is EPERM which should have caught this problem.
I would argue that this is another problem with the test and should be fixed in a separate patch on top.
Let me now if that makes sense. Or have I missed something?
Thanks,
Punit
PATCH v2 9/13 syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'
Hi!
Quoted 13 lines
Quoted 2 lines
What is the exact error here? I guess that the file does not have execute bit set and that check kicks in first?
I have a feeling that we might be cross-talking on this one.
IIUC, the intention for the test is to try and execute the binary
setuid_test(built fromsetuid_test.c). Instead, the test creates a file named "setuid_test" at the mount location and writes "TEST FILE" into it.snprintf(file, PATH_MAX, "%ssetuid_test", path_name); SAFE_FILE_PRINTF(cleanup, file, "TEST FILE");
Quoted 8 lines
It then tries to execute this file containing the text string which fails with the message -
/bin/sh: 0: Can't open/tmp/mouc0EQuB/mntpoint/setuid_test
mount03 6 TPASS : mount(2) passed with rwflag = 2The patch copies the file
setuid_testinto the mounted filesystem at which point we no longer get the "/bin/sh:" message.
Ah, right, I was under an impression that suid programs cannot be executed on MS_NOSUID mounted FS. But after consulting mount() manual page that was true for ancient kernels before 2.4 and the suid bits are simply ignored in this case on newer kernels.
So yes we need to execute a helper that tries to set privileged uid.
The idea of this patch is correct then, but we need to clean up a bit.
First of all all binary helpers should start with the prefix of the test that uses them, so in this case the helper should be renamed to mount03_setuid.c, secondly you are supposed to use the TST_RESOURCE_COPY() macro instead of the function itself and it should be done once in the test setup as well as the code that sets the actual setuid bit.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 9/13 syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'
Quoted 30 lines · Cyril Hrubis
Hi!
Quoted 13 lines
Quoted 2 lines
What is the exact error here? I guess that the file does not have execute bit set and that check kicks in first?
I have a feeling that we might be cross-talking on this one.
IIUC, the intention for the test is to try and execute the binary
setuid_test(built fromsetuid_test.c). Instead, the test creates a file named "setuid_test" at the mount location and writes "TEST FILE" into it.snprintf(file, PATH_MAX, "%ssetuid_test", path_name); SAFE_FILE_PRINTF(cleanup, file, "TEST FILE");Quoted 8 lines
It then tries to execute this file containing the text string which fails with the message -
/bin/sh: 0: Can't open/tmp/mouc0EQuB/mntpoint/setuid_test
mount03 6 TPASS : mount(2) passed with rwflag = 2The patch copies the file
setuid_testinto the mounted filesystem at which point we no longer get the "/bin/sh:" message.Ah, right, I was under an impression that suid programs cannot be executed on
MS_NOSUIDmounted FS. But after consultingmount()manual page that was true for ancient kernels before 2.4 and the suid bits are simply ignored in this case on newer kernels.So yes we need to execute a helper that tries to set privileged uid.
Quoted 8 lines
The idea of this patch is correct then, but we need to clean up a bit.
First of all all binary helpers should start with the prefix of the test that uses them, so in this case the helper should be renamed to
mount03_setuid.c, secondly you are supposed to use theTST_RESOURCE_COPY()macro instead of the function itself and it should be done once in the test setup as well as the code that sets the actual setuid bit.
The copying can't be done in setup() as the test filesystem hasn't been mounted yet.
I've folded the rest of your suggestions into the patch.
Let me know if you're done reviewing the rest of the series - I can send out an update with the changes we've discussed in the next day or so after a bit of testing and tidy-up.
Thanks,
Punit
PATCH v2 9/13 syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'
Hi!
Quoted 2 lines
The copying can't be done in
setup()as the test filesystem hasn't been mounted yet.
Well we can always mount it in the setup, prepare it for the testing, then umount it again...
Quoted 3 lines
Let me know if you're done reviewing the rest of the series - I can send out an update with the changes we've discussed in the next day or so after a bit of testing and tidy-up.
I will look closely on the cpu_hotplug patches then I'm done. The rest of the patches I haven't commeted are OK.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 9/13 syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'
Quoted 6 lines · Cyril Hrubis
Hi!
Quoted 2 lines
The copying can't be done in
setup()as the test filesystem hasn't been mounted yet.Well we can always mount it in the setup, prepare it for the testing, then umount it again...
Ok, done that now.
Quoted 7 lines
Quoted 4 lines
Let me know if you're done reviewing the rest of the series - I can send out an update with the changes we've discussed in the next day or so after a bit of testing and tidy-up.
I will look closely on the
cpu_hotplugpatches then I'm done. The rest of the patches I haven't commeted are OK.
Cool, thanks!
PATCH v2 10/13 getdtablesize01: Handle ENFILE errno
In reply to Punit AgrawalFrom: "Suzuki K. Poulose" <[email protected]>
getdtablesize01 testcase attempts to open RLIMIT_NOFILE-1 (Maximum open files for a process) file descriptors. However if we hit an ENFILE (system wide limit of maximum number of open files) we should break the test, rather than failing. This is more relevant for runs on VMs where the rootfs could be a 9p fs or any other emulated fs.
--- .../kernel/syscalls/getdtablesize/getdtablesize01.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
testcases/kernel/syscalls/getdtablesize/getdtablesize01.c +10 -7
@@ -54,7 +54,7 @@ int TST_TOTAL = 1;int main (void ) { -int table_size, loop, fd, count =0 ; +int table_size, loop, fd =0 , count =0 ;int max_val_opfiles;struct rlimit rlp;
@@ -82,24 +82,27 @@ int main(void) "Checking Max num of files that can be opened by a process.Should be: RLIMIT_NOFILE - 1");for (loop =1 ; loop <= max_val_opfiles; loop++) { fd =open ("/etc/hosts" ,O_RDONLY ); + +if (fd ==-1 ) +break ; + count = fd;#ifdef DEBUG printf ("Opened file num %d\n" , fd);#endif -if (fd ==-1 ) -break ; -else - count = fd; }//Now the max files opened should be RLIMIT_NOFILE - 1 , why ? read getdtablesize man page -if (count >0 ) -close (count);if (count == (max_val_opfiles -1 ))tst_resm (TPASS ,"%d = %d" , count, (max_val_opfiles -1 )); +else if (fd <0 && errno ==ENFILE ) +tst_brkm (TBROK , cleanup,"Reached maximum number of open files for the system" );else tst_resm (TFAIL ,"%d != %d" , count, (max_val_opfiles -1 )); +if (count >0 ) +close (count); +cleanup ();tst_exit (); }
Signature · 1 line
2.14.2
PATCH v2 10/13 getdtablesize01: Handle ENFILE errno
Hi!
testcases/kernel/syscalls/getdtablesize/getdtablesize01.c int main(void) · line 97
Quoted 6 lines
//Now the max files opened should be
RLIMIT_NOFILE- 1 , why ? read getdtablesize man pagetestcases/kernel/syscalls/getdtablesize/getdtablesize01.c +0
-2-if (count >0 ) -close (count);if (count == (max_val_opfiles -1 ))tst_resm (TPASS ,"%d = %d" , count, (max_val_opfiles -1 ));
We may also check for EMFILE here.
testcases/kernel/syscalls/getdtablesize/getdtablesize01.c int main(void) · line 99
Quoted 2 lines
testcases/kernel/syscalls/getdtablesize/getdtablesize01.c +2
-0+else if (fd <0 && errno ==ENFILE ) +tst_brkm (TBROK , cleanup,"Reached maximum number of open files for the system" );
^
This should rather be TCONF i.e. skipped.
Quoted 12 lines
testcases/kernel/syscalls/getdtablesize/getdtablesize01.c +3
-0else tst_resm (TFAIL ,"%d != %d" , count, (max_val_opfiles -1 )); +if (count >0 ) +close (count); +cleanup ();tst_exit (); }Signature · 1 line
2.14.2
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 10/13 getdtablesize01: Handle ENFILE errno
Quoted 9 lines · Cyril Hrubis
Hi!
testcases/kernel/syscalls/getdtablesize/getdtablesize01.c int main(void) · line 97
Quoted 6 lines
//Now the max files opened should be
RLIMIT_NOFILE- 1 , why ? read getdtablesize man pagetestcases/kernel/syscalls/getdtablesize/getdtablesize01.c +0
-2-if (count >0 ) -close (count);if (count == (max_val_opfiles -1 ))tst_resm (TPASS ,"%d = %d" , count, (max_val_opfiles -1 ));We may also check for EMFILE here.
I don't understand what you're suggesting here.
Quoted 5 lines
testcases/kernel/syscalls/getdtablesize/getdtablesize01.c int main(void) · line 99
Quoted 3 lines
testcases/kernel/syscalls/getdtablesize/getdtablesize01.c +2
-0+else if (fd <0 && errno ==ENFILE ) +tst_brkm (TBROK , cleanup,"Reached maximum number of open files for the system" );^
This should rather be TCONF i.e. skipped.
I've changed TBROK to TCONF now.
Thanks.
[...]
PATCH v2 10/13 getdtablesize01: Handle ENFILE errno
Hi!
Quoted 10 lines
Quoted 8 lines
testcases/kernel/syscalls/getdtablesize/getdtablesize01.c int main(void) · line 97
Quoted 6 lines
//Now the max files opened should be
RLIMIT_NOFILE- 1 , why ? read getdtablesize man pagetestcases/kernel/syscalls/getdtablesize/getdtablesize01.c +0
-2-if (count >0 ) -close (count);if (count == (max_val_opfiles -1 ))tst_resm (TPASS ,"%d = %d" , count, (max_val_opfiles -1 ));We may also check for EMFILE here.
I don't understand what you're suggesting here.
When process has exhausted the maximal number of file descriptors it can open we are supposted to fail with EMFILE, when that happened to the whole system before we hit the per process limit we get ENFILE. So I was suggesting that we may as well check that we were able to open expected number of file descriptors and that the last open() failed with EMFILE.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 10/13 getdtablesize01: Handle ENFILE errno
Quoted 10 lines · Cyril Hrubis
Hi!
Quoted 9 lines
Quoted 8 lines
testcases/kernel/syscalls/getdtablesize/getdtablesize01.c int main(void) · line 97
Quoted 6 lines
//Now the max files opened should be
RLIMIT_NOFILE- 1 , why ? read getdtablesize man pagetestcases/kernel/syscalls/getdtablesize/getdtablesize01.c +0
-2-if (count >0 ) -close (count);if (count == (max_val_opfiles -1 ))tst_resm (TPASS ,"%d = %d" , count, (max_val_opfiles -1 ));We may also check for EMFILE here.
Quoted 7 lines
Quoted 1 line
I don't understand what you're suggesting here.
When process has exhausted the maximal number of file descriptors it can open we are supposted to fail with EMFILE, when that happened to the whole system before we hit the per process limit we get ENFILE. So I was suggesting that we may as well check that we were able to open expected number of file descriptors and that the last
open()failed with EMFILE.
But isn't that we check already ? I mean, the test already gets the limit on number of open files for this process and tries to open one less fd. So I don't think we should check that error. If we get that error, something else is wrong in the accounting and should be treated as an error.
Suzuki
Quoted 1 line
PATCH v2 10/13 getdtablesize01: Handle ENFILE errno
Hi!
Quoted 8 lines
Quoted 5 lines
When process has exhausted the maximal number of file descriptors it can open we are supposted to fail with EMFILE, when that happened to the whole system before we hit the per process limit we get ENFILE. So I was suggesting that we may as well check that we were able to open expected number of file descriptors and that the last
open()failed with EMFILE.But isn't that we check already ? I mean, the test already gets the limit on number of open files for this process and tries to open one less fd.
As far as I can tell it tries to open as much fds as is the limit, the loop is kind of silly since it starts at 1 and the condition is <= but that should call open the open number of limit times.
Quoted 2 lines
So I don't think we should check that error. If we get that error, something else is wrong in the accounting and should be treated as an error.
But the loop never finishes and the open() always fails with EMFILE because process has at least three fds opened when it starts (the stdin/stdout/stderr are inherited from the parent). Hence the loop is always broken on fd == -1.
I guess that we may as well change the loop to infinite one, which would be less confusing in this case.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 10/13 getdtablesize01: Handle ENFILE errno
Quoted 17 lines · Cyril Hrubis
Hi!
Quoted 8 lines
Quoted 5 lines
When process has exhausted the maximal number of file descriptors it can open we are supposted to fail with EMFILE, when that happened to the whole system before we hit the per process limit we get ENFILE. So I was suggesting that we may as well check that we were able to open expected number of file descriptors and that the last
open()failed with EMFILE.But isn't that we check already ? I mean, the test already gets the limit on number of open files for this process and tries to open one less fd.
As far as I can tell it tries to open as much fds as is the limit, the loop is kind of silly since it starts at 1 and the condition is <= but that should call open the open number of limit times.
Quoted 2 lines
So I don't think we should check that error. If we get that error, something else is wrong in the accounting and should be treated as an error.
Quoted 7 lines
But the loop never finishes and the
open()always fails with EMFILE because process has at least three fds opened when it starts (the stdin/stdout/stderr are inherited from the parent). Hence the loop is always broken on fd == -1.I guess that we may as well change the loop to infinite one, which would be less confusing in this case.
Ok, I've changed the for loop to start from 0 and dropped the stopping condition.
Thanks,
Punit
PATCH v2 11/13 perf_event_open: Handle absence of PMU gracefully
In reply to Punit AgrawalFrom: "Suzuki K. Poulose" <[email protected]>
The perf_event_open0{1,2} syscall tests fail for an absence of a PMU (which returns ENODEV). Handle the return code gracefully as we check for ENOENT.
Without the patch:
$ perf_event_open01 perf_event_open01 1 TFAIL : perf_event_open01.c:162: perf_event_open \ failed unexpectedly: TEST_ERRNO=ENODEV(19): No such device
With the patch:
$ perf_event_open01 perf_event_open01 1 TCONF : perf_event_open01.c:159: perf_event_open for PERF_COUNT_HW_INSTRUCTIONS : No such device perf_event_open01 2 TCONF : perf_event_open01.c:159: perf_event_open for PERF_COUNT_HW_CACHE_REFERENCES : No such device perf_event_open01 3 TCONF : perf_event_open01.c:159: perf_event_open for PERF_COUNT_HW_CACHE_MISSES : No such device perf_event_open01 4 TCONF : perf_event_open01.c:159: perf_event_open for PERF_COUNT_HW_BRANCH_INSTRUCTIONS : No such file or directory perf_event_open01 5 TCONF : perf_event_open01.c:159: perf_event_open for PERF_COUNT_HW_BRANCH_MISSES : No such device perf_event_open01 0 TINFO : read event counter succeeded, value: 833765520 perf_event_open01 6 TPASS : test PERF_TYPE_HARDWARE: PERF_COUNT_SW_CPU_CLOCK succeeded perf_event_open01 0 TINFO : read event counter succeeded, value: 833967380 perf_event_open01 7 TPASS : test PERF_TYPE_HARDWARE: PERF_COUNT_SW_TASK_CLOCK succeeded
--- testcases/kernel/syscalls/perf_event_open/perf_event_open01.c | 3 ++- testcases/kernel/syscalls/perf_event_open/perf_event_open02.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)
testcases/kernel/syscalls/perf_event_open/perf_event_open01.c +2 -1
@@ -149,7 +149,8 @@ static void verify(struct test_case_t *tc)TEST (perf_event_open (&pe,0 ,-1 ,-1 ,0 ));if (TEST_RETURN ==-1 ) { -if (TEST_ERRNO ==ENOENT ||TEST_ERRNO ==EOPNOTSUPP ) { +if (TEST_ERRNO ==ENOENT ||TEST_ERRNO ==EOPNOTSUPP || +TEST_ERRNO ==ENODEV ) {tst_resm (TCONF |TTERRNO ,"perf_event_open for %s not supported" , tc->config_name);
testcases/kernel/syscalls/perf_event_open/perf_event_open02.c +2 -2
@@ -160,8 +160,8 @@ static int count_hardware_counters(void)for (i =0 ; i <MAX_CTRS ; i++) { fdarry[i] =perf_event_open (&hw_event,0 ,-1 ,-1 ,0 );if (fdarry[i] ==-1 ) { -if (errno ==ENOENT ) { -tst_brkm (TCONF , cleanup, +if (errno ==ENOENT || errno ==ENODEV ) { +tst_brkm (TCONF |TERRNO , cleanup,"PERF_COUNT_HW_INSTRUCTIONS not supported" ); } tst_brkm(TBROK | TERRNO, cleanup,
Signature · 1 line
2.14.2
PATCH v2 12/13 hotplug/cpu_hotplug: Repopulate cgroup:cpusets after testing hotplug
In reply to Punit AgrawalFrom: James Morse <[email protected]>
When LTP is started over ssh, systemd will use cgroups to group all the tasks started by this session. LTPs cpuhotplug04 will remove all but one CPU as part of its test.
In the kernel cpuset_hotplug_workfn() is documented thus: "Non-root cpusets are only affected by offlining". This means cpuhotplug04 will cause all future tasks to only have one available CPU. cpuhotplug04 effectively looses those CPUs for all users until the next reboot.
This problem has been reported, and was fixed for the unified hierarchy by be4c9dd7aee5 ("cpuset: enable onlined cpu/node in effective masks"). Since v3.17, this behaviour could be selected by passing '-o __DEVEL__sane_behavior' to the cgroup mount. Linux v4.4 grouped all the ABI breakage together into a new filesystem: cgroup2, removing the __DEVEL__sane_behavior flag.
With v226 Systemd can be forced to use cgroup2 by passing 'systemd.unified_cgroup_hierarchy=1' on the kernel command line.
Ubuntu 15.04 ships with systemd v219.
LTP tests that online CPUs must test for the legacy cgroup hierarchy and update the cpusets for all users/sessions. Add this code the cpu_online() helper.
--- .../cpu_hotplug/include/cpuhotplug_hotplug.sh | 36 ++++++++++++++++++++++ 1 file changed, 36 insertions(+)
testcases/kernel/hotplug/cpu_hotplug/include/cpuhotplug_hotplug.sh +36 -0
@@ -58,6 +58,39 @@ set_affinity() return $? } +# repopulate_cpusets() +# +# If the legacy cgroup hierarchy is in use, and cpuset is mounted, copy +# the permitted cpus from the root group, which always has all available +# cpus and memory, into the other hierarchy nodes. This prevents hotplug +# tests from losing CPUs, and affecting the results of later tests. +# +CPUSET="/sys/fs/cgroup/cpuset" +repopulate_cpusets() +{ + # Only if we have cpusets + if [ ! -d ${CPUSET} ] ; then + return; + fi + + # And then only legacy cpusets + if grep --quiet "/sys/fs/cgroup/cpuset[ ]*cgroup2" /proc/mounts ; then + return; + fi + + # Copy root cpus into system and user slices + cat ${CPUSET}/cpuset.cpus > ${CPUSET}/system.slice/cpuset.cpus + cat ${CPUSET}/cpuset.cpus > ${CPUSET}/user.slice/cpuset.cpus + + # Fix each session of each user + for TUSER in ${CPUSET}/user.slice/user-*.slice ; do + cat ${CPUSET}/cpuset.cpus > ${TUSER}/cpuset.cpus + for SESSION in ${TUSER}/session-*.scope; do + cat ${CPUSET}/cpuset.cpus > ${SESSION}/cpuset.cpus + done + done +} + # online_cpu(CPU) # # Onlines the given CPU. Returns a true value if it was able
@@ -77,6 +110,9 @@ online_cpu() $TIME echo 1 > /sys/devices/system/cpu/cpu${CPU}/online RC=$? + + repopulate_cpusets + report_timing "Online cpu ${CPU}" return $RC }
Signature · 1 line
2.14.2
PATCH v2 12/13 hotplug/cpu_hotplug: Repopulate cgroup:cpusets after testing hotplug
Hi!
Quoted 3 lines
LTP tests that online CPUs must test for the legacy cgroup hierarchy and update the cpusets for all users/sessions. Add this code the
cpu_online()helper.
I'm not sure if we want to work around kernel/systemd bugs like this, but even if we do we probably want to inform user about the bug. Maybe we should just check if the cpus are disabled after the test, if so, restore them and fail the test.
Quoted 27 lines
Signed-off-by James Morse <[email protected]>Signed-off-by Punit Agrawal <[email protected]>--- .../cpu_hotplug/include/cpuhotplug_hotplug.sh | 36 ++++++++++++++++++++++ 1 file changed, 36 insertions(+)testcases/kernel/hotplug/cpu_hotplug/include/cpuhotplug_hotplug.sh +13
-0@@ -58,6 +58,39 @@ set_affinity() return $? } +# repopulate_cpusets() +# +# If the legacy cgroup hierarchy is in use, and cpuset is mounted, copy +# the permitted cpus from the root group, which always has all available +# cpus and memory, into the other hierarchy nodes. This prevents hotplug +# tests from losing CPUs, and affecting the results of later tests. +# +CPUSET="/sys/fs/cgroup/cpuset" +repopulate_cpusets() +{ + # Only if we have cpusets + if [ ! -d ${CPUSET} ] ; then + return;
testcases/kernel/hotplug/cpu_hotplug/include/cpuhotplug_hotplug.sh set_affinity() · line 83
Quoted 10 lines
testcases/kernel/hotplug/cpu_hotplug/include/cpuhotplug_hotplug.sh +10
-0+ fi + + # And then only legacy cpusets + if grep --quiet "/sys/fs/cgroup/cpuset[ ]*cgroup2" /proc/mounts ; then + return; + fi + + # Copy root cpus into system and user slices + cat ${CPUSET}/cpuset.cpus > ${CPUSET}/system.slice/cpuset.cpus + cat ${CPUSET}/cpuset.cpus > ${CPUSET}/user.slice/cpuset.cpus
This will fail on distros that do not use systemd, we should check first if system.slice and user.slice directories are present.
Quoted 26 lines
testcases/kernel/hotplug/cpu_hotplug/include/cpuhotplug_hotplug.sh +13
-0+ # Fix each session of each user + for TUSER in ${CPUSET}/user.slice/user-*.slice ; do + cat ${CPUSET}/cpuset.cpus > ${TUSER}/cpuset.cpus + for SESSION in ${TUSER}/session-*.scope; do + cat ${CPUSET}/cpuset.cpus > ${SESSION}/cpuset.cpus + done + done + +} + # online_cpu(CPU) # # Onlines the given CPU. Returns a true value if it was able@@ -77,6 +110,9 @@ online_cpu() $TIME echo 1 > /sys/devices/system/cpu/cpu${CPU}/online RC=$? + + repopulate_cpusets + report_timing "Online cpu ${CPU}" return $RC }Signature · 1 line
2.14.2
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v2 12/13 hotplug/cpu_hotplug: Repopulate cgroup:cpusets after testing hotplug
Hi Cyril,
Quoted 9 lines · Cyril Hrubis
Hi!
Quoted 3 lines
LTP tests that online CPUs must test for the legacy cgroup hierarchy and update the cpusets for all users/sessions. Add this code the
cpu_online()helper.I'm not sure if we want to work around kernel/systemd bugs like this, but even if we do we probably want to inform user about the bug. Maybe we should just check if the cpus are disabled after the test, if so, restore them and fail the test.
It turns out that the problem behaviour is specific to the filesystem (Ubuntu 14.04).
On a recent debian filesystem I tested with, systemd doesn't even seem to use cpusets by default. I imagine other distros make similarly different choices. :)
If anybody faces the same (or similar) problem with reduced performance after runnnig ltp, they can find their way to this commit now that it's on the list (and you're aware of the problem).
I am going to drop this patch from the next version.
Thanks,
Punit
Quoted 68 lines
Quoted 28 lines
Signed-off-by James Morse <[email protected]>Signed-off-by Punit Agrawal <[email protected]>--- .../cpu_hotplug/include/cpuhotplug_hotplug.sh | 36 ++++++++++++++++++++++ 1 file changed, 36 insertions(+)testcases/kernel/hotplug/cpu_hotplug/include/cpuhotplug_hotplug.sh +13
-0@@ -58,6 +58,39 @@ set_affinity() return $? } +# repopulate_cpusets() +# +# If the legacy cgroup hierarchy is in use, and cpuset is mounted, copy +# the permitted cpus from the root group, which always has all available +# cpus and memory, into the other hierarchy nodes. This prevents hotplug +# tests from losing CPUs, and affecting the results of later tests. +# +CPUSET="/sys/fs/cgroup/cpuset" +repopulate_cpusets() +{ + # Only if we have cpusets + if [ ! -d ${CPUSET} ] ; then + return;testcases/kernel/hotplug/cpu_hotplug/include/cpuhotplug_hotplug.sh set_affinity() · line 83
Quoted 10 lines
testcases/kernel/hotplug/cpu_hotplug/include/cpuhotplug_hotplug.sh +10
-0+ fi + + # And then only legacy cpusets + if grep --quiet "/sys/fs/cgroup/cpuset[ ]*cgroup2" /proc/mounts ; then + return; + fi + + # Copy root cpus into system and user slices + cat ${CPUSET}/cpuset.cpus > ${CPUSET}/system.slice/cpuset.cpus + cat ${CPUSET}/cpuset.cpus > ${CPUSET}/user.slice/cpuset.cpusThis will fail on distros that do not use systemd, we should check first if system.slice and user.slice directories are present.
Quoted 26 lines
testcases/kernel/hotplug/cpu_hotplug/include/cpuhotplug_hotplug.sh +13
-0+ # Fix each session of each user + for TUSER in ${CPUSET}/user.slice/user-*.slice ; do + cat ${CPUSET}/cpuset.cpus > ${TUSER}/cpuset.cpus + for SESSION in ${TUSER}/session-*.scope; do + cat ${CPUSET}/cpuset.cpus > ${SESSION}/cpuset.cpus + done + done + +} + # online_cpu(CPU) # # Onlines the given CPU. Returns a true value if it was able@@ -77,6 +110,9 @@ online_cpu() $TIME echo 1 > /sys/devices/system/cpu/cpu${CPU}/online RC=$? + + repopulate_cpusets + report_timing "Online cpu ${CPU}" return $RC }Signature · 1 line
2.14.2
PATCH v2 13/13 hotplug/cpu_hotplug: Remove bashism disown from kill_pid()
In reply to Punit AgrawalFrom: James Morse <[email protected]>
disown is a bashism not present in dash. kill_pid() already sends the process SIGKILL.
Remove the "disown" to allow running in shells that don't support disown.
--- testcases/kernel/hotplug/cpu_hotplug/include/cpuhotplug_testsuite.sh | 1 - 1 file changed, 1 deletion(-)
testcases/kernel/hotplug/cpu_hotplug/include/cpuhotplug_testsuite.sh +0 -1
@@ -74,7 +74,6 @@ pid_is_valid() kill_pid() { PID=$1 - disown $PID kill -9 $PID > /dev/null 2>&1 }
Signature · 1 line
2.14.2
v2 cover Collection of fixes
In reply to Punit AgrawalHi,
Quoted 9 lines · Punit Agrawal
Hi,
This is the second posting of that collects fixes for issues encountered when running ltp on internal test platforms. Previous version can be found at [0]. This update addresses all the feedback received on the previous version.
Changes:
Quoted 8 lines
v2:
* removedcheck_hugepage()prototype fromhugetlb.h(patch 1)
* hugeshmctl01 - migrate to using checkpoint synchronisation primitives
* sigwaitinfo - use child process to execute test instead of using longjmp based solution
* Dropped patches that are no longer applicable or already mergedIf there aren't anymore comments, please consider them for merging.
I'd appreciate any feedback on this patch set. This update addresses all the comments received on the previous version.
If the patches look good, please consider merging them.
Quoted 47 lines
Thanks,
Punit[0] http://lists.linux.it/pipermail/ltp/2017-October/006225.html
James Morse (2):
hotplug/cpu_hotplug: Repopulate cgroup:cpusets after testing hotplug hotplug/cpu_hotplug: Remove bashism disown fromkill_pid()Lorenzo Pieralisi (1):
hugeshmget02: add missingSHM_HUGETLBflag on segment creationPunit Agrawal (4):
Movecheck_hugepage()helper to mem/lib
hugeshmctl01: Convert to LTP synchronisation primitives
hugeshmctl01: Fix warning about signed/unsigned comparison
sigwaitinfo01: catch SEGV and report success forbad_address2testcaseSuzuki K. Poulose (3):
hugeshmctl02: Fix allocation size for odd number of hugepages
getdtablesize01: Handle ENFILE errno
perf_event_open: Handle absence of PMU gracefullyWill Deacon (3):
thp: ensure THP/hugetlbfs is available
sigwaitinfo01: fix race between sending and dequeueing RT signals syscalls/mount03: Copysetuid_testto execute instead of 'TEST FILE'.../cpu_hotplug/include/cpuhotplug_hotplug.sh | 36 ++++++++++ .../cpu_hotplug/include/cpuhotplug_testsuite.sh | 1 - .../kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c | 77 ++++------------------ .../kernel/mem/hugetlb/hugeshmctl/hugeshmctl02.c | 2 +- .../kernel/mem/hugetlb/hugeshmget/hugeshmget02.c | 3 +- testcases/kernel/mem/hugetlb/lib/hugetlb.c | 6 -- testcases/kernel/mem/hugetlb/lib/hugetlb.h | 3 - testcases/kernel/mem/include/mem.h | 2 + testcases/kernel/mem/lib/mem.c | 6 ++ testcases/kernel/mem/thp/thp01.c | 3 + testcases/kernel/mem/thp/thp02.c | 2 + testcases/kernel/mem/thp/thp03.c | 2 + .../syscalls/getdtablesize/getdtablesize01.c | 17 +++-- testcases/kernel/syscalls/mount/mount03.c | 4 +- .../syscalls/perf_event_open/perf_event_open01.c | 3 +- .../syscalls/perf_event_open/perf_event_open02.c | 4 +- .../kernel/syscalls/sigwaitinfo/sigwaitinfo01.c | 34 +++++++++- 17 files changed, 117 insertions(+), 88 deletions(-)