mem: ksm: race condition fixes
cover mem: ksm: race condition fixes
Hello,
some false positives triggered the LTP ksm?? tests on some arch with slower CPU. The volatile pages weren't zero during the checks, as they should have been.
Volatile pages left around means there are orphaned rmap_items, that can happen if the KSM scans aren't complete.
I believe the problem is with the waiting that wasn't waiting two full passes, but it only waited until the values stopped changing after 10 seconds of wait time. Plus the ksm?? children were still running by the time the wait time started.
The first patch takes care of waiting two scans (more efficiently, with a polling of 1 sec). That change then exposes the problem with the ksm?? children not being stopped before we start waiting, and the second patch fixes that.
After applying these two fixes, we had hundred of successful runs on the same system that showed volatile pages not zero, without any further failures.
Try #2, first submit bounced because I thought I subscribed, but I was still one click away.
Thanks,
Andrea
Andrea Arcangeli (2):
mem: ksm: fix volatile page false positive
mem: ksm: fixes for race conditions
testcases/kernel/mem/lib/mem.c | 93 ++++++++++++++++++++++-------------------- 1 file changed, 49 insertions(+), 44 deletions(-)
PATCH 1/2 mem: ksm: fix volatile page false positive
To guarantee all volatile rmap_items are collected, the full scan must run at least once. The other pages_* value not changing doesn't mean all volatile rmap_items have been taken down.
To guarantee merging we need to wait two full scans as the first scan will only build the unstable tree.
So waiting at least two KSM scans is going to satisfy both above requirements.
After setting run to "2" all KSM pages have been taken down already so no need to wait any further.
--- testcases/kernel/mem/lib/mem.c | 79 ++++++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 37 deletions(-)
testcases/kernel/mem/lib/mem.c +42 -37
@@ -266,46 +266,38 @@ static void check(char *path, long int value)tst_res (TPASS ,"%s is %ld." , path, actual_val); } -static void wait_ksmd_done (void ) +static void wait_ksmd_full_scan (void ) { -long pages_shared, pages_sharing, pages_volatile, pages_unshared; -long old_pages_shared =0 , old_pages_sharing =0 ; -long old_pages_volatile =0 , old_pages_unshared =0 ; -int changing =1 , count =0 ; +unsigned long full_scans, at_least_one_full_scan; +int count =0 ; -while (changing) { -sleep (10 ); +SAFE_FILE_SCANF (PATH_KSM "full_scans" ,"%lu" , &full_scans); +/* +* The current scan is already in progress so we wouldn't +* guarantee to call get_user_pages() on every existing +* rmap_item if we only waited the remaining part of the +* current scan. +* +* The actual merging happens after the unstable tree has been +* built so we need to wait at least two full scans to +* guarantee merging, so wait full_scans to increment by 3 so +* at least two full scans will run. +*/ + at_least_one_full_scan = full_scans +3 ; +while (full_scans < at_least_one_full_scan) { +sleep (1 ); count++; - -SAFE_FILE_SCANF (PATH_KSM "pages_shared" ,"%ld" , &pages_shared); -SAFE_FILE_SCANF (PATH_KSM "pages_sharing" ,"%ld" , &pages_sharing); -SAFE_FILE_SCANF (PATH_KSM "pages_volatile" ,"%ld" , &pages_volatile); -SAFE_FILE_SCANF (PATH_KSM "pages_unshared" ,"%ld" , &pages_unshared); - -if (pages_shared != old_pages_shared || - pages_sharing != old_pages_sharing || - pages_volatile != old_pages_volatile || - pages_unshared != old_pages_unshared) { - old_pages_shared = pages_shared; - old_pages_sharing = pages_sharing; - old_pages_volatile = pages_volatile; - old_pages_unshared = pages_unshared; - }else { - changing =0 ; - } +SAFE_FILE_SCANF (PATH_KSM "full_scans" ,"%lu" , &full_scans); } -tst_res (TINFO ,"ksm daemon takes %ds to scan all mergeable pages" , - count *10 ); +tst_res (TINFO ,"ksm daemon takes %ds to run two full scans" , + count); } -static void group_check (int run,int pages_shared,int pages_sharing, -int pages_volatile,int pages_unshared, -int sleep_millisecs,int pages_to_scan) +static void final_group_check (int run,int pages_shared,int pages_sharing, +int pages_volatile,int pages_unshared, +int sleep_millisecs,int pages_to_scan) { -/* wait for ksm daemon to scan all mergeable pages. */ -wait_ksmd_done (); -tst_res (TINFO ,"check!" );check ("run" , run);check ("pages_shared" , pages_shared);
@@ -316,6 +308,22 @@ static void group_check(int run, int pages_shared, int pages_sharing,check ("pages_to_scan" , pages_to_scan); } +static void group_check (int run,int pages_shared,int pages_sharing, +int pages_volatile,int pages_unshared, +int sleep_millisecs,int pages_to_scan) +{ +if (run !=1 ) { +tst_res (TFAIL ,"group_check run is not 1, %d." , run); + }else { +/* wait for ksm daemon to scan all mergeable pages. */ +wait_ksmd_full_scan (); + } + +final_group_check (run, pages_shared, pages_sharing, + pages_volatile, pages_unshared, + sleep_millisecs, pages_to_scan); +} +static void verify (char **memory,char value,int proc,int start,int end,int start2,int end2) {
@@ -535,11 +543,11 @@ void create_same_memory(int size, int num, int unit)SAFE_FILE_PRINTF (PATH_KSM "run" ,"2" );resume_ksm_children (child, num); -group_check (2 ,0 ,0 ,0 ,0 ,0 , size * pages * num); +final_group_check (2 ,0 ,0 ,0 ,0 ,0 , size * pages * num);tst_res (TINFO ,"stop KSM." );SAFE_FILE_PRINTF (PATH_KSM "run" ,"0" ); -group_check (0 ,0 ,0 ,0 ,0 ,0 , size * pages * num); +final_group_check (0 ,0 ,0 ,0 ,0 ,0 , size * pages * num);while (waitpid (-1 , &status,WUNTRACED |WCONTINUED ) >0 )if (WEXITSTATUS (status) !=0 )
@@ -610,7 +618,6 @@ void test_ksm_merge_across_nodes(unsigned long nr_pages)* to remerge according to the new setting. */ SAFE_FILE_PRINTF (PATH_KSM "run" ,"2" ); -wait_ksmd_done ();tst_res (TINFO ,"Start to test KSM with merge_across_nodes=1" );SAFE_FILE_PRINTF (PATH_KSM "merge_across_nodes" ,"1" );SAFE_FILE_PRINTF (PATH_KSM "run" ,"1" );
@@ -618,7 +625,6 @@ void test_ksm_merge_across_nodes(unsigned long nr_pages) nr_pages * num_nodes);SAFE_FILE_PRINTF (PATH_KSM "run" ,"2" ); -wait_ksmd_done ();tst_res (TINFO ,"Start to test KSM with merge_across_nodes=0" );SAFE_FILE_PRINTF (PATH_KSM "merge_across_nodes" ,"0" );SAFE_FILE_PRINTF (PATH_KSM "run" ,"1" );
@@ -626,7 +632,6 @@ void test_ksm_merge_across_nodes(unsigned long nr_pages) 0, 0, 0, nr_pages * num_nodes);SAFE_FILE_PRINTF (PATH_KSM "run" ,"2" ); -wait_ksmd_done (); }/* THP */
PATCH 1/2 mem: ksm: fix volatile page false positive
Hello,
testcases/kernel/mem/lib/mem.c static void check(char *path, long int value) · line 279
Quoted 5 lines · Andrea Arcangeli
testcases/kernel/mem/lib/mem.c +5
-0+/* +* The current scan is already in progress so we wouldn't +* guarantee to call get_user_pages() on every existing +* rmap_item if we only waited the remaining part of the +* current scan.
Only a minor nit, but the wording is a bit unnatural here, it should probably be: "... in progress so we can't guarantee that get_user_pages() is is called on every existing rmap_item if ..."
testcases/kernel/mem/lib/mem.c static void check(char *path, long int value) · line 285
Quoted 6 lines
testcases/kernel/mem/lib/mem.c +6
-0+* +* The actual merging happens after the unstable tree has been +* built so we need to wait at least two full scans to +* guarantee merging, so wait full_scans to increment by 3 so +* at least two full scans will run. +*/
--
Thank you,
Richard.
PATCH 1/2 mem: ksm: fix volatile page false positive
In reply to Andrea ArcangeliHi!
testcases/kernel/mem/lib/mem.c static void check(char *path, long int value) · line 285
Quoted 11 lines
testcases/kernel/mem/lib/mem.c +11
-0+/* +* The current scan is already in progress so we wouldn't +* guarantee to call get_user_pages() on every existing +* rmap_item if we only waited the remaining part of the +* current scan. +* +* The actual merging happens after the unstable tree has been +* built so we need to wait at least two full scans to +* guarantee merging, so wait full_scans to increment by 3 so +* at least two full scans will run. +*/
I've took a liberty in rewording this comment a bit and pushed the patchset, thanks.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH 1/2 mem: ksm: fix volatile page false positive
Hello Cyril,
Quoted 6 lines · Cyril Hrubis
Quoted 6 lines
testcases/kernel/mem/lib/mem.c +6
-0+/* +* The current scan is already in progress so we wouldn't +* guarantee to call get_user_pages() on every existing +* rmap_item if we only waited the remaining part of the +* current scan. +*
Quoted 8 lines
testcases/kernel/mem/lib/mem.c static void check(char *path, long int value) · line 285
Quoted 5 lines
testcases/kernel/mem/lib/mem.c +5
-0+* The actual merging happens after the unstable tree has been +* built so we need to wait at least two full scans to +* guarantee merging, so wait full_scans to increment by 3 so +* at least two full scans will run. +*/ I've took a liberty in rewording this comment a bit and pushed the patchset, thanks.
Perfect, thanks!
Andrea
PATCH 2/2 mem: ksm: fixes for race conditions
In reply to Andrea Arcangelistop_ksm_children doesn't actually stop the children, it waits the children to stop themselves after they finished setting up the memory.
Problem is the test was waiting for two passes of KSM (to merge all the memory) in wait_ksmd_full_scan before waiting the child to stop setting up the memory. Two passes of KSM aren't enough if the children aren't settled yet. This fixes it by waiting the children to stop operating before waiting KSM to merge the memory.
waitpid WCONTINUED also doesn't make sense, it's better and more strict to wait until all children truly exited, otherwise it would just return immediately because the children were waken by SIGCONT at the last resume. Removing WCONTINUED will let the parent truly wait the children to verify the memory and exit after it.
These issues showed up after speeding up the wait_ksmd_full_scan and after removing other unnecessary wait times.
The test is now faster and at the same time more accurate by waiting at least two passes of KSM which by the kernel implementation we know is enough to reach a settled merged state without the need of other artificial delays.
In case of failure of the checks, this now also prints the actual value to provide more debug info in the log.
--- testcases/kernel/mem/lib/mem.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
testcases/kernel/mem/lib/mem.c +7 -7
@@ -261,7 +261,8 @@ static void check(char *path, long int value)SAFE_FILE_SCANF (fullpath,"%ld" , &actual_val);if (actual_val != value) -tst_res (TFAIL ,"%s is not %ld." , path, value); +tst_res (TFAIL ,"%s is not %ld but %ld." , path, value, + actual_val);else tst_res (TPASS ,"%s is %ld." , path, actual_val); }
@@ -523,21 +524,20 @@ void create_same_memory(int size, int num, int unit)SAFE_FILE_PRINTF (PATH_KSM "sleep_millisecs" ,"0" );resume_ksm_children (child, num); +stop_ksm_children (child, num);group_check (1 ,2 , size * num * pages -2 ,0 ,0 ,0 , size * pages * num); -stop_ksm_children (child, num);resume_ksm_children (child, num); +stop_ksm_children (child, num);group_check (1 ,3 , size * num * pages -3 ,0 ,0 ,0 , size * pages * num); -stop_ksm_children (child, num);resume_ksm_children (child, num); +stop_ksm_children (child, num);group_check (1 ,1 , size * num * pages -1 ,0 ,0 ,0 , size * pages * num); -stop_ksm_children (child, num);resume_ksm_children (child, num); -group_check (1 ,1 , size * num * pages -2 ,0 ,1 ,0 , size * pages * num); -stop_ksm_children (child, num); +group_check (1 ,1 , size * num * pages -2 ,0 ,1 ,0 , size * pages * num);tst_res (TINFO ,"KSM unmerging..." );SAFE_FILE_PRINTF (PATH_KSM "run" ,"2" );
@@ -549,7 +549,7 @@ void create_same_memory(int size, int num, int unit)SAFE_FILE_PRINTF (PATH_KSM "run" ,"0" );final_group_check (0 ,0 ,0 ,0 ,0 ,0 , size * pages * num); -while (waitpid (-1 , &status,WUNTRACED |WCONTINUED ) >0 ) +while (waitpid (-1 , &status,0 ) >0 )if (WEXITSTATUS (status) !=0 )tst_res (TFAIL ,"child exit status is %d" ,WEXITSTATUS (status));
PATCH 2/2 mem: ksm: fixes for race conditions
Quoted 72 lines · Andrea Arcangeli
stop_ksm_childrendoesn't actually stop the children, it waits the children to stop themselves after they finished setting up the memory.Problem is the test was waiting for two passes of KSM (to merge all the memory) in
wait_ksmd_full_scanbefore waiting the child to stop setting up the memory. Two passes of KSM aren't enough if the children aren't settled yet. This fixes it by waiting the children to stop operating before waiting KSM to merge the memory.waitpid WCONTINUED also doesn't make sense, it's better and more strict to wait until all children truly exited, otherwise it would just return immediately because the children were waken by SIGCONT at the last resume. Removing WCONTINUED will let the parent truly wait the children to verify the memory and exit after it.
These issues showed up after speeding up the
wait_ksmd_full_scanand after removing other unnecessary wait times.The test is now faster and at the same time more accurate by waiting at least two passes of KSM which by the kernel implementation we know is enough to reach a settled merged state without the need of other artificial delays.
In case of failure of the checks, this now also prints the actual value to provide more debug info in the log.
Signed-off-by Andrea Arcangeli <[email protected]>--- testcases/kernel/mem/lib/mem.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)testcases/kernel/mem/lib/mem.c +6
-6@@ -261,7 +261,8 @@ static void check(char *path, long int value)SAFE_FILE_SCANF (fullpath,"%ld" , &actual_val);if (actual_val != value) -tst_res (TFAIL ,"%s is not %ld." , path, value); +tst_res (TFAIL ,"%s is not %ld but %ld." , path, value, + actual_val);else tst_res (TPASS ,"%s is %ld." , path, actual_val); }@@ -523,21 +524,20 @@ void create_same_memory(int size, int num, int unit)SAFE_FILE_PRINTF (PATH_KSM "sleep_millisecs" ,"0" );resume_ksm_children (child, num); +stop_ksm_children (child, num);group_check (1 ,2 , size * num * pages -2 ,0 ,0 ,0 , size * pages * num); -stop_ksm_children (child, num);resume_ksm_children (child, num); +stop_ksm_children (child, num);group_check (1 ,3 , size * num * pages -3 ,0 ,0 ,0 , size * pages * num); -stop_ksm_children (child, num);resume_ksm_children (child, num); +stop_ksm_children (child, num);group_check (1 ,1 , size * num * pages -1 ,0 ,0 ,0 , size * pages * num); -stop_ksm_children (child, num);resume_ksm_children (child, num); -group_check (1 ,1 , size * num * pages -2 ,0 ,1 ,0 , size * pages * num); -stop_ksm_children (child, num); +group_check (1 ,1 , size * num * pages -2 ,0 ,1 ,0 , size * pages * num);tst_res (TINFO ,"KSM unmerging..." );SAFE_FILE_PRINTF (PATH_KSM "run" ,"2" );
testcases/kernel/mem/lib/mem.c void create_same_memory(int size, int num, int unit) · line 555
Quoted 8 lines
testcases/kernel/mem/lib/mem.c +1
-1@@ -549,7 +549,7 @@ void create_same_memory(int size, int num, int unit)SAFE_FILE_PRINTF (PATH_KSM "run" ,"0" );final_group_check (0 ,0 ,0 ,0 ,0 ,0 , size * pages * num); -while (waitpid (-1 , &status,WUNTRACED |WCONTINUED ) >0 ) +while (waitpid (-1 , &status,0 ) >0 )if (WEXITSTATUS (status) !=0 )tst_res (TFAIL ,"child exit status is %d" ,WEXITSTATUS (status));
Ack, patch-set looks good to me.
Signature · 2 lines
Thank you, Richard.
cover mem: ksm: race condition fixes
In reply to Andrea ArcangeliHi Andrea,
Quoted 29 lines · Andrea Arcangeli
Hello,
some false positives triggered the LTP ksm?? tests on some arch with slower CPU. The volatile pages weren't zero during the checks, as they should have been.
Volatile pages left around means there are orphaned
rmap_items, that can happen if the KSM scans aren't complete.I believe the problem is with the waiting that wasn't waiting two full passes, but it only waited until the values stopped changing after 10 seconds of wait time. Plus the ksm?? children were still running by the time the wait time started.
The first patch takes care of waiting two scans (more efficiently, with a polling of 1 sec). That change then exposes the problem with the ksm?? children not being stopped before we start waiting, and the second patch fixes that.
After applying these two fixes, we had hundred of successful runs on the same system that showed volatile pages not zero, without any further failures.
Try #2, first submit bounced because I thought I subscribed, but I was still one click away.
Thanks,
Andrea
Quoted 6 lines
Andrea Arcangeli (2):
mem: ksm: fix volatile page false positive
mem: ksm: fixes for race conditionstestcases/kernel/mem/lib/mem.c | 93 ++++++++++++++++++++++-------------------- 1 file changed, 49 insertions(+), 44 deletions(-)
Thanks for the patchset. I have tested the same on both x86_64 and aarch64 host machine and also reviewed the same: