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

[Andrea Cervesato]

Hi!

On 11/4/22 13:46, Martin Doucha wrote:
> 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;
Here we can't take in consideration CLOCK_MONOTONIC_COARSE resolution by 
default, because on ITIMER_REAL we are having a clock resolution given 
by CLOCK_MONOTONIC. And unfortunately we are not sure it's under the 
millisecond resolution all the times, which means margin > 0. For this 
reason, in the patch we are fetching clock resolution in a different 
way, according with the counter timer. We can fetch different 
resolutions from setup tho and using inside the test code.
>     ...
> }
>
> 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)},
>
Andrea