futex/futex_wake04.c: fix issues with hugepages and usleep

Message ID 20181009210602.23261-1-rafael.tinoco@linaro.org
State New
Headers show
Series
  • futex/futex_wake04.c: fix issues with hugepages and usleep
Related show

Commit Message

Rafael David Tinoco Oct. 9, 2018, 9:06 p.m.
This commit fixes 2 observed issues:

1) usleep() time is too small if test is being executed in slower
terminal devices (specially embedded systems). Raising it to 0.001
seconds was enough to finish 10240 iterations in around 90 seconds in a
4 vcpu kvm guest (fully emulated serial being used as console).

2) Test was changing number of hugepages during setup()/cleanup() phase
despite the system had (or not) available hugepages. This was causing
overhead of destroying (or creating) hugepages during the test
execution.  Now, if system has > 0 hugepages available, the test doesn't
touch it.

Link: https://bugs.linaro.org/show_bug.cgi?id=3984
Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
---
 testcases/kernel/syscalls/futex/futex_wake04.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Cyril Hrubis Oct. 10, 2018, 10:43 a.m. | #1
Hi!
> This commit fixes 2 observed issues:
> 
> 1) usleep() time is too small if test is being executed in slower
> terminal devices (specially embedded systems). Raising it to 0.001
> seconds was enough to finish 10240 iterations in around 90 seconds in a
> 4 vcpu kvm guest (fully emulated serial being used as console).
> 
> 2) Test was changing number of hugepages during setup()/cleanup() phase
> despite the system had (or not) available hugepages. This was causing
> overhead of destroying (or creating) hugepages during the test
> execution.  Now, if system has > 0 hugepages available, the test doesn't
> touch it.

Can you please split this into separate patches? Generally it's
considered a good practice to split unrelated changes like these.

> Link: https://bugs.linaro.org/show_bug.cgi?id=3984
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
> ---
>  testcases/kernel/syscalls/futex/futex_wake04.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/futex/futex_wake04.c b/testcases/kernel/syscalls/futex/futex_wake04.c
> index 3c7038317..93d36ac33 100644
> --- a/testcases/kernel/syscalls/futex/futex_wake04.c
> +++ b/testcases/kernel/syscalls/futex/futex_wake04.c
> @@ -76,14 +76,17 @@ static void setup(void)
>  	tst_tmpdir();
>  
>  	SAFE_FILE_SCANF(NULL, PATH_NR_HUGEPAGES, "%ld", &orig_hugepages);
> -	SAFE_FILE_PRINTF(NULL, PATH_NR_HUGEPAGES, "%d", 1);
> +
> +	if (!orig_hugepages)

Maybe <= 0 would be a bit more robust.

> +		SAFE_FILE_PRINTF(NULL, PATH_NR_HUGEPAGES, "%d", 1);
>  
>  	TEST_PAUSE;
>  }
>  
>  static void cleanup(void)
>  {
> -	SAFE_FILE_PRINTF(NULL, PATH_NR_HUGEPAGES, "%ld", orig_hugepages);
> +	if (!orig_hugepages)
> +		SAFE_FILE_PRINTF(NULL, PATH_NR_HUGEPAGES, "%d", 0);
>  
>  	tst_rmdir();
>  }
> @@ -172,8 +175,9 @@ static void wakeup_thread2(void)
>  				tst_strerrno(res));
>  	}
>  
> +	/* 0.001 seconds: less might cause lockups for slower terminals */

I would avoid comments like this, you can check the git changelog to
figure out why the value was changed.

>  	while (wait_for_threads(2))
> -		usleep(100);
> +		usleep(1000);
>  
>  	futex_wake(futex2, 1, 0);
>  
> -- 
> 2.19.1
>
Rafael David Tinoco Oct. 10, 2018, 11:14 a.m. | #2
On 10/10/18 7:43 AM, Cyril Hrubis wrote:
> Hi!
>> This commit fixes 2 observed issues:
>>
>> 1) usleep() time is too small if test is being executed in slower
>> terminal devices (specially embedded systems). Raising it to 0.001
>> seconds was enough to finish 10240 iterations in around 90 seconds in a
>> 4 vcpu kvm guest (fully emulated serial being used as console).
>>
>> 2) Test was changing number of hugepages during setup()/cleanup() phase
>> despite the system had (or not) available hugepages. This was causing
>> overhead of destroying (or creating) hugepages during the test
>> execution.  Now, if system has > 0 hugepages available, the test doesn't
>> touch it.
> 
> Can you please split this into separate patches? Generally it's
> considered a good practice to split unrelated changes like these.

Yes, will do. Ignore my last sentence in the other e-mail on the same 
thread (about this patch). Will post 2 patches "v2".

Thanks a lot Cyril!
Cyril Hrubis Oct. 10, 2018, 12:06 p.m. | #3
Hi!
> > Can you please split this into separate patches? Generally it's
> > considered a good practice to split unrelated changes like these.
> 
> Yes, will do. Ignore my last sentence in the other e-mail on the same 
> thread (about this patch). Will post 2 patches "v2".

There were some minor comments, if you agree with them I can amend the
patches you send before pushing them...
Rafael David Tinoco Oct. 10, 2018, 2:20 p.m. | #4
On 10/10/18 7:43 AM, Cyril Hrubis wrote:
> Hi!
>> This commit fixes 2 observed issues:
>>
>> 1) usleep() time is too small if test is being executed in slower
>> terminal devices (specially embedded systems). Raising it to 0.001
>> seconds was enough to finish 10240 iterations in around 90 seconds in a
>> 4 vcpu kvm guest (fully emulated serial being used as console).
>>
>> 2) Test was changing number of hugepages during setup()/cleanup() phase
>> despite the system had (or not) available hugepages. This was causing
>> overhead of destroying (or creating) hugepages during the test
>> execution.  Now, if system has > 0 hugepages available, the test doesn't
>> touch it.
> 
> Can you please split this into separate patches? Generally it's
> considered a good practice to split unrelated changes like these.
> 
>> Link: https://bugs.linaro.org/show_bug.cgi?id=3984
>> Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
>> ---
>>   testcases/kernel/syscalls/futex/futex_wake04.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/testcases/kernel/syscalls/futex/futex_wake04.c b/testcases/kernel/syscalls/futex/futex_wake04.c
>> index 3c7038317..93d36ac33 100644
>> --- a/testcases/kernel/syscalls/futex/futex_wake04.c
>> +++ b/testcases/kernel/syscalls/futex/futex_wake04.c
>> @@ -76,14 +76,17 @@ static void setup(void)
>>   	tst_tmpdir();
>>   
>>   	SAFE_FILE_SCANF(NULL, PATH_NR_HUGEPAGES, "%ld", &orig_hugepages);
>> -	SAFE_FILE_PRINTF(NULL, PATH_NR_HUGEPAGES, "%d", 1);
>> +
>> +	if (!orig_hugepages)
> 
> Maybe <= 0 would be a bit more robust.
> 
>> +		SAFE_FILE_PRINTF(NULL, PATH_NR_HUGEPAGES, "%d", 1);
>>   
>>   	TEST_PAUSE;
>>   }
>>   
>>   static void cleanup(void)
>>   {
>> -	SAFE_FILE_PRINTF(NULL, PATH_NR_HUGEPAGES, "%ld", orig_hugepages);
>> +	if (!orig_hugepages)
>> +		SAFE_FILE_PRINTF(NULL, PATH_NR_HUGEPAGES, "%d", 0);
>>   
>>   	tst_rmdir();
>>   }
>> @@ -172,8 +175,9 @@ static void wakeup_thread2(void)
>>   				tst_strerrno(res));
>>   	}
>>   
>> +	/* 0.001 seconds: less might cause lockups for slower terminals */
> 
> I would avoid comments like this, you can check the git changelog to
> figure out why the value was changed.
> 
>>   	while (wait_for_threads(2))
>> -		usleep(100);
>> +		usleep(1000);
>>   
>>   	futex_wake(futex2, 1, 0);
>>   
>> -- 
>> 2.19.1
>>
> 

I missed these comments in the 2 v2 I sent. Please feel free to change 
or let me know if you prefer a v3. Tks a lot.
Cyril Hrubis Oct. 11, 2018, 12:23 p.m. | #5
Hi!
> I missed these comments in the 2 v2 I sent. Please feel free to change 
> or let me know if you prefer a v3. Tks a lot.

Ammended and pushed, thanks.

Patch

diff --git a/testcases/kernel/syscalls/futex/futex_wake04.c b/testcases/kernel/syscalls/futex/futex_wake04.c
index 3c7038317..93d36ac33 100644
--- a/testcases/kernel/syscalls/futex/futex_wake04.c
+++ b/testcases/kernel/syscalls/futex/futex_wake04.c
@@ -76,14 +76,17 @@  static void setup(void)
 	tst_tmpdir();
 
 	SAFE_FILE_SCANF(NULL, PATH_NR_HUGEPAGES, "%ld", &orig_hugepages);
-	SAFE_FILE_PRINTF(NULL, PATH_NR_HUGEPAGES, "%d", 1);
+
+	if (!orig_hugepages)
+		SAFE_FILE_PRINTF(NULL, PATH_NR_HUGEPAGES, "%d", 1);
 
 	TEST_PAUSE;
 }
 
 static void cleanup(void)
 {
-	SAFE_FILE_PRINTF(NULL, PATH_NR_HUGEPAGES, "%ld", orig_hugepages);
+	if (!orig_hugepages)
+		SAFE_FILE_PRINTF(NULL, PATH_NR_HUGEPAGES, "%d", 0);
 
 	tst_rmdir();
 }
@@ -172,8 +175,9 @@  static void wakeup_thread2(void)
 				tst_strerrno(res));
 	}
 
+	/* 0.001 seconds: less might cause lockups for slower terminals */
 	while (wait_for_threads(2))
-		usleep(100);
+		usleep(1000);
 
 	futex_wake(futex2, 1, 0);