[LTP] [PATCH v1] Correctly handle user time in setitimer01

[Martin Doucha]

Hi,
suggestion below.

On 04. 11. 22 10:24, Andrea Cervesato via ltp wrote:
> Since ITIMER_VIRTUAL and ITIMER_PROF are counting down in user time, we
> need to take in consideration CLOCK_MONOTONIC_COARSE resolution. This is
> requested by the syscall, since it's considering context switch from
> user to kernel mode by using a higher clock resolution.
> 
> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>   .../kernel/syscalls/setitimer/setitimer01.c   | 54 +++++++++++--------
>   1 file changed, 33 insertions(+), 21 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/setitimer/setitimer01.c b/testcases/kernel/syscalls/setitimer/setitimer01.c
> index eb62f02c6..5fcae53e8 100644
> --- a/testcases/kernel/syscalls/setitimer/setitimer01.c
> +++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
> @@ -8,9 +8,16 @@
>   /*\
>    * [Description]
>    *
> - * Spawn a child and verify that setitimer() syscall passes, and it ends up
> + * Spawn a child, verify that setitimer() syscall passes and it ends up
>    * counting inside expected boundaries. Then verify from the parent that our
>    * syscall sent the correct signal to the child.
> + *
> + * Boundaries are choosen accordingly with system clock. In particular, when
> + * timer counts down in real time, CLOCK_MONOTONIC resolution has taken into
> + * account as our time step. When timer counts down in user time,
> + * CLOCK_MONOTONIC_COARSE is used. The reason is that CLOCK_MONOTONIC_COARSE
> + * is our system resolution in user space, since it's taking in consideration
> + * context switches from user to kernel space.
>    */
>   
>   #include <time.h>
> @@ -22,7 +29,6 @@
>   #include "tst_safe_clocks.h"
>   
>   static struct itimerval *value, *ovalue;
> -static unsigned long time_step;
>   
>   static struct tcase {
>   	int which;
> @@ -55,9 +61,31 @@ static void set_setitimer_value(int usec, int o_usec)
>   static void verify_setitimer(unsigned int i)
>   {
>   	pid_t pid;
> -	int status;
> -	int usec = 3 * time_step;
> +	struct timespec res;
>   	struct tcase *tc = &tcases[i];
> +	int status, usec, time_step, error;
> +
> +	if (tc->which == ITIMER_REAL)
> +		SAFE_CLOCK_GETRES(CLOCK_MONOTONIC, &res);
> +	else
> +		SAFE_CLOCK_GETRES(CLOCK_MONOTONIC_COARSE, &res);
> +
> +	time_step = res.tv_nsec / 1000;
> +	error = time_step;
> +
> +	if (time_step <= 0) {
> +		time_step = 1000;
> +		error = 0;
> +	}

This approach looks like it'll lead to some bad edge cases when
0 < time_step < 1000. It'd be better to keep the original time_step 
detection and initialize "error" variable like this (and also rename it 
to "margin"):

int jiffy;

verify_setitimer()
{
	...
	margin = (tc->which == ITIMER_REAL) ? 0 : jiffy;
	...
}

setup()
{
	...
	SAFE_CLOCK_GETRES(CLOCK_MONOTONIC_COARSE, &res);
	jiffy = (res.tv_nsec + 999) / 1000;
	...
}

> +
> +	usec = 3 * time_step;
> +
> +	tst_res(TINFO, "clock resolution: %luns, "
> +		"time step: %ius, "
> +		"counter time: %ius",
> +		res.tv_nsec,
> +		time_step,
> +		usec);
>   
>   	pid = SAFE_FORK();
>   
> @@ -76,7 +104,7 @@ static void verify_setitimer(unsigned int i)
>   			ovalue->it_value.tv_sec,
>   			ovalue->it_value.tv_usec);
>   
> -		if (ovalue->it_value.tv_sec != 0 || ovalue->it_value.tv_usec > usec)
> +		if (ovalue->it_value.tv_sec != 0 || ovalue->it_value.tv_usec > usec + error)
>   			tst_res(TFAIL, "Ending counters are out of range");
>   
>   		for (;;)
> @@ -91,26 +119,10 @@ static void verify_setitimer(unsigned int i)
>   		tst_res(TFAIL, "Child: %s", tst_strstatus(status));
>   }
>   
> -static void setup(void)
> -{
> -	struct timespec res;
> -
> -	SAFE_CLOCK_GETRES(CLOCK_MONOTONIC, &res);
> -
> -	time_step = res.tv_nsec / 1000;
> -	if (time_step < 10000)
> -		time_step = 10000;
> -
> -	tst_res(TINFO, "clock resolution: %luns, time step: %luus",
> -		res.tv_nsec,
> -		time_step);
> -}
> -
>   static struct tst_test test = {
>   	.tcnt = ARRAY_SIZE(tcases),
>   	.forks_child = 1,
>   	.test = verify_setitimer,
> -	.setup = setup,
>   	.bufs = (struct tst_buffers[]) {
>   		{&value,  .size = sizeof(struct itimerval)},
>   		{&ovalue, .size = sizeof(struct itimerval)},

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic