diff mbox series

selftests: mm: restore settings from only parent process

Message ID 20240314094045.157149-1-usama.anjum@collabora.com (mailing list archive)
State New
Headers show
Series selftests: mm: restore settings from only parent process | expand

Commit Message

Muhammad Usama Anjum March 14, 2024, 9:40 a.m. UTC
The atexit() is called from parent process as well as forked processes.
Hence the child restores the settings at exit while the parent is still
executing. Fix this by checking pid of atexit() calling process and only
restore THP number from parent process.

Fixes: c23ea61726d5 ("selftests/mm: protection_keys: save/restore nr_hugepages settings")
Tested-by: Joey Gouly <joey.gouly@arm.com>
Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
---
 tools/testing/selftests/mm/protection_keys.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Joey Gouly April 18, 2024, 12:52 p.m. UTC | #1
Hi again,

On Thu, Mar 14, 2024 at 02:40:45PM +0500, Muhammad Usama Anjum wrote:
> The atexit() is called from parent process as well as forked processes.
> Hence the child restores the settings at exit while the parent is still
> executing. Fix this by checking pid of atexit() calling process and only
> restore THP number from parent process.
> 
> Fixes: c23ea61726d5 ("selftests/mm: protection_keys: save/restore nr_hugepages settings")
> Tested-by: Joey Gouly <joey.gouly@arm.com>
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
>  tools/testing/selftests/mm/protection_keys.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c
> index f822ae31af22e..374a308174d2b 100644
> --- a/tools/testing/selftests/mm/protection_keys.c
> +++ b/tools/testing/selftests/mm/protection_keys.c
> @@ -1745,9 +1745,12 @@ void pkey_setup_shadow(void)
>  	shadow_pkey_reg = __read_pkey_reg();
>  }
>  
> +pid_t parent_pid;
> +
>  void restore_settings_atexit(void)
>  {
> -	cat_into_file(buf, "/proc/sys/vm/nr_hugepages");
> +	if (parent_pid == getpid())
> +		cat_into_file(buf, "/proc/sys/vm/nr_hugepages");
>  }
>  
>  void save_settings(void)
> @@ -1773,6 +1776,7 @@ void save_settings(void)
>  		exit(__LINE__);
>  	}
>  
> +	parent_pid = getpid();
>  	atexit(restore_settings_atexit);
>  	close(fd);
>  }

After more testing, this is not actually enough. It passes sometimes, which is
why I gave my Tested-by, but it can still fail the same way as I originally
said.

assert() at protection_keys.c::812 test_nr: 19 iteration: 1
running abort_hooks()...
errno at assert: 12

I think the parent process needs to wait for all it's child processes (recursively).
This is due to the test_pkey_alloc_exhaust() calling become_child(), where it
exits, and that exit may be from the original PID.

Thanks,
Joey
Muhammad Usama Anjum April 18, 2024, 5:25 p.m. UTC | #2
On 4/18/24 5:52 PM, Joey Gouly wrote:> Hi again,
> 
> On Thu, Mar 14, 2024 at 02:40:45PM +0500, Muhammad Usama Anjum wrote:
>> The atexit() is called from parent process as well as forked processes.
>> Hence the child restores the settings at exit while the parent is still
>> executing. Fix this by checking pid of atexit() calling process and only
>> restore THP number from parent process.
>>
>> Fixes: c23ea61726d5 ("selftests/mm: protection_keys: save/restore nr_hugepages settings")
>> Tested-by: Joey Gouly <joey.gouly@arm.com>
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>> ---
>>  tools/testing/selftests/mm/protection_keys.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c
>> index f822ae31af22e..374a308174d2b 100644
>> --- a/tools/testing/selftests/mm/protection_keys.c
>> +++ b/tools/testing/selftests/mm/protection_keys.c
>> @@ -1745,9 +1745,12 @@ void pkey_setup_shadow(void)
>>  	shadow_pkey_reg = __read_pkey_reg();
>>  }
>>  
>> +pid_t parent_pid;
>> +
>>  void restore_settings_atexit(void)
>>  {
>> -	cat_into_file(buf, "/proc/sys/vm/nr_hugepages");
>> +	if (parent_pid == getpid())
>> +		cat_into_file(buf, "/proc/sys/vm/nr_hugepages");
>>  }
>>  
>>  void save_settings(void)
>> @@ -1773,6 +1776,7 @@ void save_settings(void)
>>  		exit(__LINE__);
>>  	}
>>  
>> +	parent_pid = getpid();
>>  	atexit(restore_settings_atexit);
>>  	close(fd);
>>  }
> 
> After more testing, this is not actually enough. It passes sometimes, which is
> why I gave my Tested-by, but it can still fail the same way as I originally
> said.
> 
> assert() at protection_keys.c::812 test_nr: 19 iteration: 1
> running abort_hooks()...
> errno at assert: 12
> 
> I think the parent process needs to wait for all it's child processes (recursively).
> This is due to the test_pkey_alloc_exhaust() calling become_child(), where it
> exits, and that exit may be from the original PID.
Thanks for reporting. I'll have a look at it.

> 
> Thanks,
> Joey
diff mbox series

Patch

diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c
index f822ae31af22e..374a308174d2b 100644
--- a/tools/testing/selftests/mm/protection_keys.c
+++ b/tools/testing/selftests/mm/protection_keys.c
@@ -1745,9 +1745,12 @@  void pkey_setup_shadow(void)
 	shadow_pkey_reg = __read_pkey_reg();
 }
 
+pid_t parent_pid;
+
 void restore_settings_atexit(void)
 {
-	cat_into_file(buf, "/proc/sys/vm/nr_hugepages");
+	if (parent_pid == getpid())
+		cat_into_file(buf, "/proc/sys/vm/nr_hugepages");
 }
 
 void save_settings(void)
@@ -1773,6 +1776,7 @@  void save_settings(void)
 		exit(__LINE__);
 	}
 
+	parent_pid = getpid();
 	atexit(restore_settings_atexit);
 	close(fd);
 }